Re: [PATCH V3 4/6] libxl: arm: Split make_virtio_mmio_node()

[Oleksandr <olekstysh@xxxxxxxxx>]

On 04.08.22 10:01, Viresh Kumar wrote:

Hello Viresh

> 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.
>
> Signed-off-by: Viresh Kumar <viresh.kumar@xxxxxxxxxx>
> ---
>   tools/libs/light/libxl_arm.c | 24 ++++++++++++++----------
>   1 file changed, 14 insertions(+), 10 deletions(-)
>
> diff --git a/tools/libs/light/libxl_arm.c b/tools/libs/light/libxl_arm.c
> index 2f64b9f0ebee..6a8c4d042bd9 100644
> --- a/tools/libs/light/libxl_arm.c
> +++ b/tools/libs/light/libxl_arm.c
> @@ -900,9 +900,8 @@ 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)
> +static int make_virtio_mmio_node_common(libxl__gc *gc, void *fdt, uint64_t 
> base,
> +                                        uint32_t irq)
>   {
>       int res;
>       gic_interrupt intr;
> @@ -922,7 +921,15 @@ static int make_virtio_mmio_node(libxl__gc *gc, void *fdt,
>       res = fdt_property_interrupts(gc, fdt, &intr, 1);
>       if (res) return res;
>   
> -    res = fdt_property(fdt, "dma-coherent", NULL, 0);
> +    return fdt_property(fdt, "dma-coherent", NULL, 0);
> +}
> +
> +static int make_virtio_mmio_node_simple(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);
>       if (res) return res;
>   
>       if (backend_domid != LIBXL_TOOLSTACK_DOMID) {
> @@ -935,10 +942,7 @@ static int make_virtio_mmio_node(libxl__gc *gc, void *fdt,
>           if (res) return res;
>       }
>   
> -    res = fdt_end_node(fdt);
> -    if (res) return res;
> -
> -    return 0;
> +    return fdt_end_node(fdt);
>   }
>   
>   static const struct arch_info *get_arch_info(libxl__gc *gc,
> @@ -1261,8 +1265,8 @@ static int libxl__prepare_dtb(libxl__gc *gc, 
> libxl_domain_config *d_config,
>                       iommu_created = true;
>                   }
>   
> -                FDT( make_virtio_mmio_node(gc, fdt, disk->base, disk->irq,
> -                                           disk->backend_domid) );
> +                FDT( make_virtio_mmio_node_simple(gc, fdt, disk->base,
> +                                            disk->irq, disk->backend_domid) );
>               }
>           }
[I failed to find a better place where to make a comment]


I think that the following chunk should be also moved to 
make_virtio_mmio_node_common() since it is not a virtio disk specific 
action, this "iommus" property pointing to "xen,grant-dma" IOMMU node is 
used to inform a guest that restricted memory access using Xen grant 
mappings needs to be enabled for the virtio device if it's backend is 
going to run in a non hardware domain, which I assume also applies for 
virtio i2c, etc.

    if (backend_domid != LIBXL_TOOLSTACK_DOMID) {
        uint32_t iommus_prop[2];

        iommus_prop[0] = cpu_to_fdt32(GUEST_PHANDLE_IOMMU);
        iommus_prop[1] = cpu_to_fdt32(backend_domid);

        res = fdt_property(fdt, "iommus", iommus_prop, 
sizeof(iommus_prop));
        if (res) return res;
    }


This means that "uint32_t backend_domid" should be also passed as 
argument to make_virtio_mmio_node_common()

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 a98cfe708b..1a6ace3d8d 100644
--- a/tools/libs/light/libxl_arm.c
+++ b/tools/libs/light/libxl_arm.c
@@ -900,7 +900,7 @@ static int make_xen_iommu_node(libxl__gc *gc, void *fdt)
 }

 static int make_virtio_mmio_node_common(libxl__gc *gc, void *fdt, 
uint64_t base,
-                                        uint32_t irq)
+                                        uint32_t irq, uint32_t 
backend_domid)
 {
     int res;
     gic_interrupt intr;
@@ -920,15 +920,7 @@ static int make_virtio_mmio_node_common(libxl__gc 
*gc, void *fdt, uint64_t base,
     res = fdt_property_interrupts(gc, fdt, &intr, 1);
     if (res) return res;

-    return fdt_property(fdt, "dma-coherent", NULL, 0);
-}
-
-static int make_virtio_mmio_node_simple(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);
+    res = fdt_property(fdt, "dma-coherent", NULL, 0);
     if (res) return res;

     if (backend_domid != LIBXL_TOOLSTACK_DOMID) {
@@ -941,6 +933,17 @@ static int make_virtio_mmio_node_simple(libxl__gc 
*gc, void *fdt, uint64_t base,
         if (res) return res;
     }

+    return res;
+}
+
+static int make_virtio_mmio_node_simple(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 fdt_end_node(fdt);
 }


Other changes look good.


>   
--
Regards,

Oleksandr Tyshchenko