Re: [PATCH V4 5/6] libxl: Allocate MMIO params for I2c device and update DT

[Oleksandr <olekstysh@xxxxxxxxx>]

On 09.08.22 08:34, Viresh Kumar wrote:

Hello Viresh

> This patch allocates Virtio MMIO params (IRQ and memory region) and pass
> them to the backend, also update Guest device-tree based on Virtio I2C
> DT bindings [1].
Nit: Patch does more than it claims in the description, I am speaking 
about the changes
related to make_xen_iommu_node(). So I would add a sentence about that here.


> [1] 
> https://www.kernel.org/doc/Documentation/devicetree/bindings/i2c/i2c-virtio.yaml
>
> Signed-off-by: Viresh Kumar <viresh.kumar@xxxxxxxxxx>
> ---
>   tools/libs/light/libxl_arm.c | 57 +++++++++++++++++++++++++++++++-----
>   1 file changed, 50 insertions(+), 7 deletions(-)
>
> diff --git a/tools/libs/light/libxl_arm.c b/tools/libs/light/libxl_arm.c
> index 891cb6ef2674..93ea8e3d3fa3 100644
> --- a/tools/libs/light/libxl_arm.c
> +++ b/tools/libs/light/libxl_arm.c
> @@ -110,6 +110,15 @@ int libxl__arch_domain_prepare_config(libxl__gc *gc,
>           }
>       }
>   
> +    for (i = 0; i < d_config->num_i2cs; i++) {
> +        libxl_device_i2c *i2c = &d_config->i2cs[i];
> +
Nit: This blank line is not needed, I think


> +        int rc = alloc_virtio_mmio_params(gc, &i2c->base, &i2c->irq,
> +                &virtio_mmio_base, &virtio_mmio_irq);
Nit: Something wrong with the indentation and looks like the blank line 
is needed here.
> +        if (rc)
> +            return rc;
> +    }
> +
>       /*
>        * Every virtio-mmio device uses one emulated SPI. If Virtio devices are
>        * present, make sure that we allocate enough SPIs for them.
> @@ -947,6 +956,26 @@ static int make_virtio_mmio_node_simple(libxl__gc *gc, 
> void *fdt, uint64_t base,
>       return fdt_end_node(fdt);
>   }
>   
> +static int make_virtio_mmio_node_i2c(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;
> +
> +    res = fdt_begin_node(fdt, "i2c");
> +    if (res) return res;
> +
> +    res = fdt_property_compat(gc, fdt, 1, "virtio,device22");
> +    if (res) return res;
> +
> +    res = fdt_end_node(fdt);
> +    if (res) return res;
> +
> +    return fdt_end_node(fdt);
> +}
> +
>   static const struct arch_info *get_arch_info(libxl__gc *gc,
>                                                const struct xc_dom_image *dom)
>   {
> @@ -1148,7 +1177,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;
> @@ -1256,22 +1285,36 @@ 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_simple(gc, fdt, disk->base,
>                                               disk->irq, disk->backend_domid) );
>               }
>           }
>   
> +        for (i = 0; i < d_config->num_i2cs; i++) {
> +            libxl_device_i2c *i2c = &d_config->i2cs[i];
> +
> +            if (i2c->backend_domid != LIBXL_TOOLSTACK_DOMID)
> +                iommu_needed = true;
> +
> +            FDT( make_virtio_mmio_node_i2c(gc, fdt, i2c->base, i2c->irq,
> +                                           i2c->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) );

Preferably with above fixed:


Reviewed-by: Oleksandr Tyshchenko <oleksandr_tyshchenko@xxxxxxxx>


>   
--
Regards,

Oleksandr Tyshchenko