| Message ID | 4a238937ceb803f494e5633a3a779866383bd463.1661159474.git.viresh.kumar@linaro.org (mailing list archive) |
|---|---|
| State | New, archived |
| Headers | show |
| Series | Virtio toolstack support for I2C and GPIO on Arm | expand |
On Mon, Aug 22, 2022 at 02:45:18PM +0530, Viresh Kumar wrote: > This patch allocates Virtio MMIO params (IRQ and memory region) and pass > them to the backend, also update Guest device-tree based on Virtio GPIO > DT bindings [1]. > > [1] https://www.kernel.org/doc/Documentation/devicetree/bindings/gpio/gpio-virtio.yaml > > Reviewed-by: Oleksandr Tyshchenko <oleksandr_tyshchenko@epam.com> > Signed-off-by: Viresh Kumar <viresh.kumar@linaro.org> > --- > tools/libs/light/libxl_arm.c | 51 ++++++++++++++++++++++++++++++++++++ > 1 file changed, 51 insertions(+) > > diff --git a/tools/libs/light/libxl_arm.c b/tools/libs/light/libxl_arm.c > index 4c1012e56893..86c1e560900f 100644 > --- a/tools/libs/light/libxl_arm.c > +++ b/tools/libs/light/libxl_arm.c > @@ -121,6 +121,15 @@ int libxl__arch_domain_prepare_config(libxl__gc *gc, > return rc; > } > > + 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, Indentation seems wrong here. Also, you could declare "rc" (without an initial value) for the whole function rather than declaring it in each for loop scope. Then, this patch could be squash into the one that adds GPIO support to libxl. Thanks,
(+ Bertrand) Hi Viresh, On 22/08/2022 10:15, Viresh Kumar wrote: > This patch allocates Virtio MMIO params (IRQ and memory region) and pass > them to the backend, also update Guest device-tree based on Virtio GPIO > DT bindings [1]. > > [1] https://www.kernel.org/doc/Documentation/devicetree/bindings/gpio/gpio-virtio.yaml > > Reviewed-by: Oleksandr Tyshchenko <oleksandr_tyshchenko@epam.com> > Signed-off-by: Viresh Kumar <viresh.kumar@linaro.org> > --- > tools/libs/light/libxl_arm.c | 51 ++++++++++++++++++++++++++++++++++++ > 1 file changed, 51 insertions(+) > > diff --git a/tools/libs/light/libxl_arm.c b/tools/libs/light/libxl_arm.c > index 4c1012e56893..86c1e560900f 100644 > --- a/tools/libs/light/libxl_arm.c > +++ b/tools/libs/light/libxl_arm.c > @@ -121,6 +121,15 @@ int libxl__arch_domain_prepare_config(libxl__gc *gc, > return rc; > } > > + 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; > + } > + > /* > * Every virtio-mmio device uses one emulated SPI. If Virtio devices are > * present, make sure that we allocate enough SPIs for them. > @@ -984,6 +993,38 @@ static int make_virtio_mmio_node_i2c(libxl__gc *gc, void *fdt, uint64_t base, > return fdt_end_node(fdt); > } > > +static int make_virtio_mmio_node_gpio(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, "gpio"); > + if (res) return res; > + > + res = fdt_property_compat(gc, fdt, 1, "virtio,device29"); It is a very descriptive compatible :). And yes I realize this is the compatible chosen by upstream. > + 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; > + > + return fdt_end_node(fdt); > +} > + > static const struct arch_info *get_arch_info(libxl__gc *gc, > const struct xc_dom_image *dom) > { > @@ -1317,6 +1358,16 @@ static int libxl__prepare_dtb(libxl__gc *gc, libxl_domain_config *d_config, > i2c->backend_domid) ); > } > > + for (i = 0; i < d_config->num_gpios; i++) { > + libxl_device_gpio *gpio = &d_config->gpios[i]; > + > + if (gpio->backend_domid != LIBXL_TOOLSTACK_DOMID) > + iommu_needed = true; > + > + FDT( make_virtio_mmio_node_gpio(gc, fdt, gpio->base, gpio->irq, > + gpio->backend_domid) ); So this is exposing a GPIO interrupt controller but IIUC we will also need to describe the devices in the device-tree. Can you outline how you would expect it to work? To put some context, this is related to what I wrote in patch #1. How much code will be necessary in libxl to add new device? I am not the maintainer of this code, but I would expect some example how this can be used once this merged. This would help us to understand whether the interface is correct. Cheers,
On 07-09-22, 19:02, Julien Grall wrote: > It is a very descriptive compatible :). And yes I realize this is the > compatible chosen by upstream. :) > So this is exposing a GPIO interrupt controller but IIUC we will also need > to describe the devices in the device-tree. Can you outline how you would > expect it to work? The same is true for busses like I2C and SPI, which may have client devices in the DT as well. Though I don't use them currently in my setup where I need to test the backends. I am able to test both I2C and GPIO dynamically from the guest, without any client devices in the DT. > To put some context, this is related to what I wrote in patch #1. How much > code will be necessary in libxl to add new device? > > I am not the maintainer of this code, but I would expect some example how > this can be used once this merged. This would help us to understand whether > the interface is correct. I understand what you are looking for and that we may be required to pass the relation of a client device with the GPIO controller via the "virtio" entry you suggested for a common device. I don't have an answer for that at the moment.
diff --git a/tools/libs/light/libxl_arm.c b/tools/libs/light/libxl_arm.c index 4c1012e56893..86c1e560900f 100644 --- a/tools/libs/light/libxl_arm.c +++ b/tools/libs/light/libxl_arm.c @@ -121,6 +121,15 @@ int libxl__arch_domain_prepare_config(libxl__gc *gc, return rc; } + 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; + } + /* * Every virtio-mmio device uses one emulated SPI. If Virtio devices are * present, make sure that we allocate enough SPIs for them. @@ -984,6 +993,38 @@ static int make_virtio_mmio_node_i2c(libxl__gc *gc, void *fdt, uint64_t base, return fdt_end_node(fdt); } +static int make_virtio_mmio_node_gpio(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, "gpio"); + if (res) return res; + + res = fdt_property_compat(gc, fdt, 1, "virtio,device29"); + 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; + + return fdt_end_node(fdt); +} + static const struct arch_info *get_arch_info(libxl__gc *gc, const struct xc_dom_image *dom) { @@ -1317,6 +1358,16 @@ static int libxl__prepare_dtb(libxl__gc *gc, libxl_domain_config *d_config, i2c->backend_domid) ); } + for (i = 0; i < d_config->num_gpios; i++) { + libxl_device_gpio *gpio = &d_config->gpios[i]; + + if (gpio->backend_domid != LIBXL_TOOLSTACK_DOMID) + iommu_needed = true; + + FDT( make_virtio_mmio_node_gpio(gc, fdt, gpio->base, gpio->irq, + gpio->backend_domid) ); + } + /* * Note, this should be only called after creating all virtio-mmio * device nodes