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

[Oleksandr <olekstysh@xxxxxxxxx>]

On 04.08.22 10:01, 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].
>
> [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 | 35 +++++++++++++++++++++++++++++++++++
>   1 file changed, 35 insertions(+)
>
> diff --git a/tools/libs/light/libxl_arm.c b/tools/libs/light/libxl_arm.c
> index 6a8c4d042bd9..08a1499c9523 100644
> --- a/tools/libs/light/libxl_arm.c
> +++ b/tools/libs/light/libxl_arm.c
> @@ -112,6 +112,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];
> +
> +        int rc = alloc_virtio_mmio_params(gc, &i2c->base, &i2c->irq,
> +                &virtio_mmio_base, &virtio_mmio_irq);
> +        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.
> @@ -945,6 +954,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)
> +{
> +    int res;
> +
> +    res = make_virtio_mmio_node_common(gc, fdt, base, irq);
> +    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)
>   {
> @@ -1270,6 +1299,12 @@ static int libxl__prepare_dtb(libxl__gc *gc, 
> libxl_domain_config *d_config,
>               }
>           }
>   
> +        for (i = 0; i < d_config->num_i2cs; i++) {
> +            libxl_device_i2c *i2c = &d_config->i2cs[i];
> +
> +            FDT( make_virtio_mmio_node_i2c(gc, fdt, i2c->base, i2c->irq) );
> +        }
> +
>           if (pfdt)
>               FDT( copy_partial_fdt(gc, fdt, pfdt) );

I think that patch needs to be updated according to the suggestion 
provided for "[PATCH V3 4/6] libxl: arm: Split make_virtio_mmio_node()" 
(of course, if you agree with it).
If so, the make_virtio_mmio_node_i2c() should gain "uint32_t 
backend_domid" argument, etc. And with introducing new virtio i2c the 
make_xen_iommu_node() should also be called for it if not created yet 
(or maybe better to have only single invocation of make_xen_iommu_node() 
at the end)

Something like the diff on top of current patch below (not tested):

diff --git a/tools/libs/light/libxl_arm.c b/tools/libs/light/libxl_arm.c
index 0e448c486b..725ccb5f3f 100644
--- a/tools/libs/light/libxl_arm.c
+++ b/tools/libs/light/libxl_arm.c
@@ -957,11 +957,11 @@ static int make_virtio_mmio_node_simple(libxl__gc 
*gc, void *fdt, uint64_t base,
 }

 static int make_virtio_mmio_node_i2c(libxl__gc *gc, void *fdt, 
uint64_t base,
-                                     uint32_t irq)
+                                     uint32_t irq, uint32_t backend_domid)
 {
     int res;

-    res = make_virtio_mmio_node_common(gc, fdt, base, irq);
+    res = make_virtio_mmio_node_common(gc, fdt, base, irq, backend_domid);
     if (res) return res;

     res = fdt_begin_node(fdt, "i2c");
@@ -1177,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;
@@ -1285,16 +1285,13 @@ next_resize:
         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) );
@@ -1303,10 +1300,20 @@ next_resize:

         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) );
+            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) );

(END)


Other changes look good.


>   
--
Regards,

Oleksandr Tyshchenko