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

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@linaro.org

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-olekstys...