Hello,
This was earlier sent as part of a patch series [1] adding support for GPIO/I2C virtio devices. The device specific patches would require some rework and possibly several versions, and so this series separates out the generic independent patches into a series of their own.
This series makes some of the generic code independent of the disk device, since it can be used for other device types later on.
Rebased over staging branch from today.
V5->V6: - Separated into a patch series of their own. - Updated commit log of 1st patch to cover all changes. - Rename make_virtio_mmio_node_simple() as make_virtio_mmio_node(). - New patch 3/3, separated code from device specific patch.
-- Viresh
Viresh Kumar (3): libxl: arm: Create alloc_virtio_mmio_params() libxl: arm: Split make_virtio_mmio_node() libxl: arm: make creation of iommu node independent of disk device
tools/libs/light/libxl_arm.c | 83 +++++++++++++++++++++++++----------- 1 file changed, 57 insertions(+), 26 deletions(-)
In order to prepare for adding support for more device types, create a separate routine to allocate base and irq for a device as the same code will be required for other device types too.
Also move updates to virtio_irq and virtio_enabled out of the disk device specific block, as they will depend on other device types too.
Suggested-by: Oleksandr Tyshchenko oleksandr_tyshchenko@epam.com Reviewed-by: Oleksandr Tyshchenko oleksandr_tyshchenko@epam.com Signed-off-by: Viresh Kumar viresh.kumar@linaro.org --- tools/libs/light/libxl_arm.c | 47 +++++++++++++++++++++++++----------- 1 file changed, 33 insertions(+), 14 deletions(-)
diff --git a/tools/libs/light/libxl_arm.c b/tools/libs/light/libxl_arm.c index 1a3ac1646e94..cc30ba124918 100644 --- a/tools/libs/light/libxl_arm.c +++ b/tools/libs/light/libxl_arm.c @@ -48,6 +48,24 @@ static uint32_t alloc_virtio_mmio_irq(libxl__gc *gc, uint32_t *virtio_mmio_irq) return irq; }
+static int alloc_virtio_mmio_params(libxl__gc *gc, uint64_t *base, + uint32_t *irq, uint64_t *virtio_mmio_base, + uint32_t *virtio_mmio_irq) +{ + *base = alloc_virtio_mmio_base(gc, virtio_mmio_base); + if (!*base) + return ERROR_FAIL; + + *irq = alloc_virtio_mmio_irq(gc, virtio_mmio_irq); + if (!*irq) + return ERROR_FAIL; + + LOG(DEBUG, "Allocate Virtio MMIO params: IRQ %u BASE 0x%"PRIx64, *irq, + *base); + + return 0; +} + static const char *gicv_to_string(libxl_gic_version gic_version) { switch (gic_version) { @@ -70,6 +88,7 @@ int libxl__arch_domain_prepare_config(libxl__gc *gc, bool vuart_enabled = false, virtio_enabled = false; uint64_t virtio_mmio_base = GUEST_VIRTIO_MMIO_BASE; uint32_t virtio_mmio_irq = GUEST_VIRTIO_MMIO_SPI_FIRST; + int rc;
/* * If pl011 vuart is enabled then increment the nr_spis to allow allocation @@ -85,20 +104,12 @@ int libxl__arch_domain_prepare_config(libxl__gc *gc, libxl_device_disk *disk = &d_config->disks[i];
if (disk->specification == LIBXL_DISK_SPECIFICATION_VIRTIO) { - disk->base = alloc_virtio_mmio_base(gc, &virtio_mmio_base); - if (!disk->base) - return ERROR_FAIL; - - disk->irq = alloc_virtio_mmio_irq(gc, &virtio_mmio_irq); - if (!disk->irq) - return ERROR_FAIL; - - if (virtio_irq < disk->irq) - virtio_irq = disk->irq; - virtio_enabled = true; + rc = alloc_virtio_mmio_params(gc, &disk->base, &disk->irq, + &virtio_mmio_base, + &virtio_mmio_irq);
- LOG(DEBUG, "Allocate Virtio MMIO params for Vdev %s: IRQ %u BASE 0x%"PRIx64, - disk->vdev, disk->irq, disk->base); + if (rc) + return rc; } }
@@ -107,8 +118,16 @@ int libxl__arch_domain_prepare_config(libxl__gc *gc, * present, make sure that we allocate enough SPIs for them. * The resulting "nr_spis" needs to cover the highest possible SPI. */ - if (virtio_enabled) + if (virtio_mmio_irq != GUEST_VIRTIO_MMIO_SPI_FIRST) { + virtio_enabled = true; + + /* + * Assumes that "virtio_mmio_irq" is the highest allocated irq, which is + * updated from alloc_virtio_mmio_irq() currently. + */ + virtio_irq = virtio_mmio_irq - 1; nr_spis = max(nr_spis, virtio_irq - 32 + 1); + }
for (i = 0; i < d_config->b_info.num_irqs; i++) { uint32_t irq = d_config->b_info.irqs[i];
On Thu, Sep 08, 2022 at 02:22:59PM +0530, Viresh Kumar wrote:
In order to prepare for adding support for more device types, create a separate routine to allocate base and irq for a device as the same code will be required for other device types too.
Also move updates to virtio_irq and virtio_enabled out of the disk device specific block, as they will depend on other device types too.
Suggested-by: Oleksandr Tyshchenko oleksandr_tyshchenko@epam.com Reviewed-by: Oleksandr Tyshchenko oleksandr_tyshchenko@epam.com Signed-off-by: Viresh Kumar viresh.kumar@linaro.org
Reviewed-by: Anthony PERARD anthony.perard@citrix.com
Thanks,
make_virtio_mmio_node() creates the DT node for simple MMIO devices currently, i.e. the ones that don't require any additional properties.
In order to allow using it for other complex device types, split the functionality into two, one where the fdt node isn't closed and the other one to create a simple DT node.
Reviewed-by: Oleksandr Tyshchenko oleksandr_tyshchenko@epam.com Signed-off-by: Viresh Kumar viresh.kumar@linaro.org --- tools/libs/light/libxl_arm.c | 18 +++++++++++++----- 1 file changed, 13 insertions(+), 5 deletions(-)
diff --git a/tools/libs/light/libxl_arm.c b/tools/libs/light/libxl_arm.c index cc30ba124918..55aee15c10b4 100644 --- a/tools/libs/light/libxl_arm.c +++ b/tools/libs/light/libxl_arm.c @@ -907,9 +907,9 @@ static int make_xen_iommu_node(libxl__gc *gc, void *fdt) return 0; }
-static int make_virtio_mmio_node(libxl__gc *gc, void *fdt, - uint64_t base, uint32_t irq, - uint32_t backend_domid) +/* The caller is responsible to complete / close the fdt node */ +static int make_virtio_mmio_node_common(libxl__gc *gc, void *fdt, uint64_t base, + uint32_t irq, uint32_t backend_domid) { int res; gic_interrupt intr; @@ -942,10 +942,18 @@ static int make_virtio_mmio_node(libxl__gc *gc, void *fdt, if (res) return res; }
- res = fdt_end_node(fdt); + return res; +} + +static int make_virtio_mmio_node(libxl__gc *gc, void *fdt, uint64_t base, + uint32_t irq, uint32_t backend_domid) +{ + int res; + + res = make_virtio_mmio_node_common(gc, fdt, base, irq, backend_domid); if (res) return res;
- return 0; + return fdt_end_node(fdt); }
static const struct arch_info *get_arch_info(libxl__gc *gc,
On Thu, Sep 08, 2022 at 02:23:00PM +0530, Viresh Kumar wrote:
make_virtio_mmio_node() creates the DT node for simple MMIO devices currently, i.e. the ones that don't require any additional properties.
In order to allow using it for other complex device types, split the functionality into two, one where the fdt node isn't closed and the other one to create a simple DT node.
Reviewed-by: Oleksandr Tyshchenko oleksandr_tyshchenko@epam.com Signed-off-by: Viresh Kumar viresh.kumar@linaro.org
Reviewed-by: Anthony PERARD anthony.perard@citrix.com
Thanks,
The iommu node will be required for other virtio device types too, not just disk device.
Move the call to make_xen_iommu_node(), out of the disk device specific block and rename "iommu_created" variable to "iommu_needed", and set it to true for virtio disk device.
Signed-off-by: Viresh Kumar viresh.kumar@linaro.org --- tools/libs/light/libxl_arm.c | 18 +++++++++++------- 1 file changed, 11 insertions(+), 7 deletions(-)
diff --git a/tools/libs/light/libxl_arm.c b/tools/libs/light/libxl_arm.c index 55aee15c10b4..2637acafa358 100644 --- a/tools/libs/light/libxl_arm.c +++ b/tools/libs/light/libxl_arm.c @@ -1157,7 +1157,7 @@ static int libxl__prepare_dtb(libxl__gc *gc, libxl_domain_config *d_config, size_t fdt_size = 0; int pfdt_size = 0; libxl_domain_build_info *const info = &d_config->b_info; - bool iommu_created; + bool iommu_needed; unsigned int i;
const libxl_version_info *vers; @@ -1265,22 +1265,26 @@ static int libxl__prepare_dtb(libxl__gc *gc, libxl_domain_config *d_config, if (d_config->num_pcidevs) FDT( make_vpci_node(gc, fdt, ainfo, dom) );
- iommu_created = false; + iommu_needed = false; for (i = 0; i < d_config->num_disks; i++) { libxl_device_disk *disk = &d_config->disks[i];
if (disk->specification == LIBXL_DISK_SPECIFICATION_VIRTIO) { - if (disk->backend_domid != LIBXL_TOOLSTACK_DOMID && - !iommu_created) { - FDT( make_xen_iommu_node(gc, fdt) ); - iommu_created = true; - } + if (disk->backend_domid != LIBXL_TOOLSTACK_DOMID) + iommu_needed = true;
FDT( make_virtio_mmio_node(gc, fdt, disk->base, disk->irq, disk->backend_domid) ); } }
+ /* + * Note, this should be only called after creating all virtio-mmio + * device nodes + */ + if (iommu_needed) + FDT( make_xen_iommu_node(gc, fdt) ); + if (pfdt) FDT( copy_partial_fdt(gc, fdt, pfdt) );
On Thu, Sep 08, 2022 at 02:23:01PM +0530, Viresh Kumar wrote:
The iommu node will be required for other virtio device types too, not just disk device.
Move the call to make_xen_iommu_node(), out of the disk device specific block and rename "iommu_created" variable to "iommu_needed", and set it to true for virtio disk device.
Signed-off-by: Viresh Kumar viresh.kumar@linaro.org
tools/libs/light/libxl_arm.c | 18 +++++++++++------- 1 file changed, 11 insertions(+), 7 deletions(-)
diff --git a/tools/libs/light/libxl_arm.c b/tools/libs/light/libxl_arm.c index 55aee15c10b4..2637acafa358 100644 --- a/tools/libs/light/libxl_arm.c +++ b/tools/libs/light/libxl_arm.c @@ -1157,7 +1157,7 @@ static int libxl__prepare_dtb(libxl__gc *gc, libxl_domain_config *d_config, size_t fdt_size = 0; int pfdt_size = 0; libxl_domain_build_info *const info = &d_config->b_info;
- bool iommu_created;
- bool iommu_needed; unsigned int i;
const libxl_version_info *vers; @@ -1265,22 +1265,26 @@ static int libxl__prepare_dtb(libxl__gc *gc, libxl_domain_config *d_config, if (d_config->num_pcidevs) FDT( make_vpci_node(gc, fdt, ainfo, dom) );
iommu_created = false;
iommu_needed = false;
That variable could now be initialised at declaration rather than in the middle of the code, as it could be used for more than just virtio-disk.
for (i = 0; i < d_config->num_disks; i++) { libxl_device_disk *disk = &d_config->disks[i];if (disk->specification == LIBXL_DISK_SPECIFICATION_VIRTIO) {
if (disk->backend_domid != LIBXL_TOOLSTACK_DOMID &&!iommu_created) {FDT( make_xen_iommu_node(gc, fdt) );iommu_created = true;}
if (disk->backend_domid != LIBXL_TOOLSTACK_DOMID)iommu_needed = true;FDT( make_virtio_mmio_node(gc, fdt, disk->base, disk->irq, disk->backend_domid) ); } }
/** Note, this should be only called after creating all virtio-mmio* device nodes
I think this comment is confusing. Before this patch, it didn't seems to matter that the node was created only after the first device. But the comment seems to say that the node should only be created last. But it seems that all that matter is that the node is only created once. So maybe the comment is superfluous? Or we could comment that we will create a single iommu node for all virtio-mmio node/device...
*/if (iommu_needed)FDT( make_xen_iommu_node(gc, fdt) );if (pfdt) FDT( copy_partial_fdt(gc, fdt, pfdt) );
Thanks,
The iommu node will be required for other virtio device types too, not just disk device.
Move the call to make_xen_iommu_node(), out of the disk device specific block and rename "iommu_created" variable to "iommu_needed", and set it to true for virtio disk device.
Signed-off-by: Viresh Kumar viresh.kumar@linaro.org --- V6->V6.1: - Initialize iommu_needed at declaration time only. - Update comment.
tools/libs/light/libxl_arm.c | 17 ++++++++++------- 1 file changed, 10 insertions(+), 7 deletions(-)
diff --git a/tools/libs/light/libxl_arm.c b/tools/libs/light/libxl_arm.c index 55aee15c10b4..fe1c92383dd6 100644 --- a/tools/libs/light/libxl_arm.c +++ b/tools/libs/light/libxl_arm.c @@ -1157,7 +1157,7 @@ static int libxl__prepare_dtb(libxl__gc *gc, libxl_domain_config *d_config, size_t fdt_size = 0; int pfdt_size = 0; libxl_domain_build_info *const info = &d_config->b_info; - bool iommu_created; + bool iommu_needed = false; unsigned int i;
const libxl_version_info *vers; @@ -1265,22 +1265,25 @@ static int libxl__prepare_dtb(libxl__gc *gc, libxl_domain_config *d_config, if (d_config->num_pcidevs) FDT( make_vpci_node(gc, fdt, ainfo, dom) );
- iommu_created = false; for (i = 0; i < d_config->num_disks; i++) { libxl_device_disk *disk = &d_config->disks[i];
if (disk->specification == LIBXL_DISK_SPECIFICATION_VIRTIO) { - if (disk->backend_domid != LIBXL_TOOLSTACK_DOMID && - !iommu_created) { - FDT( make_xen_iommu_node(gc, fdt) ); - iommu_created = true; - } + if (disk->backend_domid != LIBXL_TOOLSTACK_DOMID) + iommu_needed = true;
FDT( make_virtio_mmio_node(gc, fdt, disk->base, disk->irq, disk->backend_domid) ); } }
+ /* + * The iommu node should be created only once for all virtio-mmio + * devices. + */ + if (iommu_needed) + FDT( make_xen_iommu_node(gc, fdt) ); + if (pfdt) FDT( copy_partial_fdt(gc, fdt, pfdt) );
On Fri, Sep 09, 2022 at 08:13:28PM +0530, Viresh Kumar wrote:
The iommu node will be required for other virtio device types too, not just disk device.
Move the call to make_xen_iommu_node(), out of the disk device specific block and rename "iommu_created" variable to "iommu_needed", and set it to true for virtio disk device.
Signed-off-by: Viresh Kumar viresh.kumar@linaro.org
Reviewed-by: Anthony PERARD anthony.perard@citrix.com
Thanks,
On 09-09-22, 16:02, Anthony PERARD wrote:
On Fri, Sep 09, 2022 at 08:13:28PM +0530, Viresh Kumar wrote:
The iommu node will be required for other virtio device types too, not just disk device.
Move the call to make_xen_iommu_node(), out of the disk device specific block and rename "iommu_created" variable to "iommu_needed", and set it to true for virtio disk device.
Signed-off-by: Viresh Kumar viresh.kumar@linaro.org
Reviewed-by: Anthony PERARD anthony.perard@citrix.com
I don't see these patches being applied yet, do I need to ping someone for that ?
Hi Viresh,
Apologies for the late reply. I was away for the past 2 weeks.
On 20/09/2022 11:29, Viresh Kumar wrote:
On 09-09-22, 16:02, Anthony PERARD wrote:
On Fri, Sep 09, 2022 at 08:13:28PM +0530, Viresh Kumar wrote:
The iommu node will be required for other virtio device types too, not just disk device.
Move the call to make_xen_iommu_node(), out of the disk device specific block and rename "iommu_created" variable to "iommu_needed", and set it to true for virtio disk device.
Signed-off-by: Viresh Kumar viresh.kumar@linaro.org
Reviewed-by: Anthony PERARD anthony.perard@citrix.com
I don't see these patches being applied yet, do I need to ping someone for that ?
We are currently preparing to release Xen 4.17 (plan for November) and have stopped accepting new code (other than bug fix) since the beginning of September.
Your series will be committed once the tree is re-opened (hopefully by the beginning of November). Please ping me mid-november if this is still not applied.
Cheers,
Hi Viresh,
On 04/10/2022 11:59, Julien Grall wrote:
On 20/09/2022 11:29, Viresh Kumar wrote:
On 09-09-22, 16:02, Anthony PERARD wrote:
On Fri, Sep 09, 2022 at 08:13:28PM +0530, Viresh Kumar wrote:
The iommu node will be required for other virtio device types too, not just disk device.
Move the call to make_xen_iommu_node(), out of the disk device specific block and rename "iommu_created" variable to "iommu_needed", and set it to true for virtio disk device.
Signed-off-by: Viresh Kumar viresh.kumar@linaro.org
Reviewed-by: Anthony PERARD anthony.perard@citrix.com
I don't see these patches being applied yet, do I need to ping someone for that ?
We are currently preparing to release Xen 4.17 (plan for November) and have stopped accepting new code (other than bug fix) since the beginning of September.
Your series will be committed once the tree is re-opened (hopefully by the beginning of November). Please ping me mid-november if this is still not applied.
Unfortunately, we had some delay for releasing 4.17. So I have pushed this series in a branch for-next/4.18. This will be applied to staging once the tree has re-opened.
Cheers,
On 24-11-22, 20:19, Julien Grall wrote:
Unfortunately, we had some delay for releasing 4.17. So I have pushed this series in a branch for-next/4.18.
Thanks.
This will be applied to staging once the tree has re-opened.
I don't see the branch here though. Is it not public yet ? Or should I be looking at a different tree ?
https://github.com/xen-project/xen
Hi Viresh,
On 25/11/2022 06:45, Viresh Kumar wrote:
On 24-11-22, 20:19, Julien Grall wrote:
Unfortunately, we had some delay for releasing 4.17. So I have pushed this series in a branch for-next/4.18.
Thanks.
This will be applied to staging once the tree has re-opened.
I don't see the branch here though. Is it not public yet ? Or should I be looking at a different tree ?
The branch has been created in my personal tree for now:
https://xenbits.xen.org/gitweb/?p=people/julieng/xen-unstable.git%3Ba=shortl...
Sorry I should have been clearer.
Cheers,
On 09.09.22 17:43, Viresh Kumar wrote:
Hello Viresh
The iommu node will be required for other virtio device types too, not just disk device.
Move the call to make_xen_iommu_node(), out of the disk device specific block and rename "iommu_created" variable to "iommu_needed", and set it to true for virtio disk device.
Signed-off-by: Viresh Kumar viresh.kumar@linaro.org
V6->V6.1:
- Initialize iommu_needed at declaration time only.
- Update comment.
Reviewed-by: Oleksandr Tyshchenko oleksandr_tyshchenko@epam.com
tools/libs/light/libxl_arm.c | 17 ++++++++++------- 1 file changed, 10 insertions(+), 7 deletions(-)
diff --git a/tools/libs/light/libxl_arm.c b/tools/libs/light/libxl_arm.c index 55aee15c10b4..fe1c92383dd6 100644 --- a/tools/libs/light/libxl_arm.c +++ b/tools/libs/light/libxl_arm.c @@ -1157,7 +1157,7 @@ static int libxl__prepare_dtb(libxl__gc *gc, libxl_domain_config *d_config, size_t fdt_size = 0; int pfdt_size = 0; libxl_domain_build_info *const info = &d_config->b_info;
- bool iommu_created;
- bool iommu_needed = false; unsigned int i;
const libxl_version_info *vers; @@ -1265,22 +1265,25 @@ static int libxl__prepare_dtb(libxl__gc *gc, libxl_domain_config *d_config, if (d_config->num_pcidevs) FDT( make_vpci_node(gc, fdt, ainfo, dom) );
iommu_created = false; for (i = 0; i < d_config->num_disks; i++) { libxl_device_disk *disk = &d_config->disks[i];if (disk->specification == LIBXL_DISK_SPECIFICATION_VIRTIO) {
if (disk->backend_domid != LIBXL_TOOLSTACK_DOMID &&!iommu_created) {FDT( make_xen_iommu_node(gc, fdt) );iommu_created = true;}
if (disk->backend_domid != LIBXL_TOOLSTACK_DOMID)iommu_needed = true;FDT( make_virtio_mmio_node(gc, fdt, disk->base, disk->irq, disk->backend_domid) ); } }
/** The iommu node should be created only once for all virtio-mmio* devices.*/if (iommu_needed)FDT( make_xen_iommu_node(gc, fdt) );if (pfdt) FDT( copy_partial_fdt(gc, fdt, pfdt) );
stratos-dev@op-lists.linaro.org
-
Anthony PERARD -
Julien Grall -
Oleksandr -
Viresh Kumar