Re: [PATCH 4/4] libxl: Allocate MMIO params for GPIO device and update DT

[Oleksandr <olekstysh@xxxxxxxxx>]

On 05.05.22 10:33, 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.
>
> Signed-off-by: Viresh Kumar <viresh.kumar@xxxxxxxxxx>
> ---
>   tools/libs/light/libxl_arm.c | 60 ++++++++++++++++++++++++++++++++++++
>   1 file changed, 60 insertions(+)
>
> diff --git a/tools/libs/light/libxl_arm.c b/tools/libs/light/libxl_arm.c
> index ea633d6f91df..89e5a1e5780d 100644
> --- a/tools/libs/light/libxl_arm.c
> +++ b/tools/libs/light/libxl_arm.c
> @@ -135,6 +135,26 @@ int libxl__arch_domain_prepare_config(libxl__gc *gc,
>               i2c->irq, i2c->base);
>       }
>   
> +    for (i = 0; i < d_config->num_gpios; i++) {
> +        libxl_device_gpio *gpio = &d_config->gpios[i];
> +
> +        gpio->base = alloc_virtio_mmio_base(gc);
> +        if (!gpio->base)
> +            return ERROR_FAIL;
> +
> +        gpio->irq = alloc_virtio_mmio_irq(gc);
> +        if (!gpio->irq)
> +            return ERROR_FAIL;
> +
> +        if (virtio_irq < gpio->irq)
> +            virtio_irq = gpio->irq;
> +
> +        virtio_enabled = true;
> +
> +        LOG(DEBUG, "Allocate Virtio MMIO params for GPIO: IRQ %u BASE 
> 0x%"PRIx64,
> +            gpio->irq, gpio->base);
> +    }
> +

Looks like, we are going to end up with some duplication in 
libxl__arch_domain_prepare_config(). This is already a third chunk (the 
second was in patch #3/4).
I would probably consider moving some code to a separate function.
Below the non-tested diff (based on the recent V8 patch [1]), how it 
could look like. I am not sure this is an ideal variant, but rather to 
demonstrate what I mean.
diff --git a/tools/libs/light/libxl_arm.c b/tools/libs/light/libxl_arm.c
index 25969e0..6e23c26 100644
--- a/tools/libs/light/libxl_arm.c
+++ b/tools/libs/light/libxl_arm.c
@@ -48,6 +48,23 @@ 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) {
@@ -85,25 +102,18 @@ 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;
-
-            LOG(DEBUG, "Allocate Virtio MMIO params for Vdev %s: IRQ %u 
BASE 0x%"PRIx64,
-                disk->vdev, disk->irq, disk->base);
+            int rc = alloc_virtio_mmio_params(gc, &disk->base, &disk->irq,
+ &virtio_mmio_base, &virtio_mmio_irq);
+            if (rc)
+                return rc;
         }
     }

-    if (virtio_enabled)
+    if (virtio_mmio_irq != GUEST_VIRTIO_MMIO_SPI_FIRST) {
+        virtio_irq = virtio_mmio_irq - 1;
         nr_spis += (virtio_irq - 32) + 1;
+        virtio_enabled = true;
+    }

     for (i = 0; i < d_config->b_info.num_irqs; i++) {
         uint32_t irq = d_config->b_info.irqs[i];
(END)


So, if this was done before (in a prereq patch), your new addition here 
would be just the following:

diff --git a/tools/libs/light/libxl_arm.c b/tools/libs/light/libxl_arm.c
index 6e23c26..ae64cbd 100644
--- a/tools/libs/light/libxl_arm.c
+++ b/tools/libs/light/libxl_arm.c
@@ -109,6 +109,14 @@ int libxl__arch_domain_prepare_config(libxl__gc *gc,
         }
     }

+    for (i = 0; i < d_config->num_gpios; i++) {
+        libxl_device_gpio *gpio = &d_config->gpios[i];
+        int rc = alloc_virtio_mmio_params(gc, &gpio->base, &gpio->irq,
+                                          &virtio_mmio_base, 
&virtio_mmio_irq);
+        if (rc)
+            return rc;
+    }
+
     if (virtio_mmio_irq != GUEST_VIRTIO_MMIO_SPI_FIRST) {
         virtio_irq = virtio_mmio_irq - 1;
         nr_spis += (virtio_irq - 32) + 1;


>       if (virtio_enabled)
>           nr_spis += (virtio_irq - 32) + 1;
>   
> @@ -954,6 +974,41 @@ static int make_virtio_mmio_node_i2c(libxl__gc *gc, void *fdt,
>       return 0;
>   }
>   
> +static int make_virtio_mmio_node_gpio(libxl__gc *gc, void *fdt,
> +                                 uint64_t base, uint32_t irq)
> +{
> +    int res;
> +
> +    res = _make_virtio_mmio_node(gc, fdt, base, irq);
> +    if (res) return res;
> +
> +    res = fdt_begin_node(fdt, "gpio");
> +    if (res) return res;
> +
> +    res = fdt_property_compat(gc, fdt, 1, "virtio,device29");
Please provide a link (in the commit description) to the corresponding
Linux device tree binding. I assume it is:

Documentation/devicetree/bindings/gpio/gpio-virtio.yaml


> +    if (res) return res;
> +
> +    res = fdt_property(fdt, "gpio-controller", NULL, 0);
> +    if (res) return res;
> +
> +    res = fdt_property_cell(fdt, "#gpio-cells", 2);
> +    if (res) return res;
> +
> +    res = fdt_property(fdt, "interrupt-controller", NULL, 0);
> +    if (res) return res;
> +
> +    res = fdt_property_cell(fdt, "#interrupt-cells", 2);
> +    if (res) return res;
> +
> +    res = fdt_end_node(fdt);
> +    if (res) return res;
> +
> +    res = fdt_end_node(fdt);
> +    if (res) return res;
> +
> +    return 0;
> +}
> +
>   static const struct arch_info *get_arch_info(libxl__gc *gc,
>                                                const struct xc_dom_image *dom)
>   {
> @@ -1269,6 +1324,11 @@ static int libxl__prepare_dtb(libxl__gc *gc, 
> libxl_domain_config *d_config,
>                   FDT( make_virtio_mmio_node(gc, fdt, disk->base, disk->irq) );
>           }
>   
> +        for (i = 0; i < d_config->num_gpios; i++) {
> +            libxl_device_gpio *gpio = &d_config->gpios[i];
> +            FDT( make_virtio_mmio_node_gpio(gc, fdt, gpio->base, gpio->irq) );
> +        }
> +
>           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) );

[1] 
https://lore.kernel.org/xen-devel/1651598763-12162-3-git-send-email-olekstysh@xxxxxxxxx/

--
Regards,

Oleksandr Tyshchenko