Re: [PATCH V6 3/3] libxl: arm: make creation of iommu node independent of disk device

[Anthony PERARD <anthony.perard@citrix.com>]

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,

-- 
Anthony PERARD