| Message ID | 20230830043518.21584-2-vikram.garhwal@amd.com (mailing list archive) |
|---|---|
| State | New, archived |
| Headers | show |
| Series | Add Virtio support to Xenpvh machine for arm | expand |
On Tue, 29 Aug 2023, Vikram Garhwal wrote: > From: Oleksandr Tyshchenko <oleksandr_tyshchenko@epam.com> > > In order to use virtio backends we need to allocate virtio-mmio > parameters (irq and base) and register corresponding buses. > > Use the constants defined in public header arch-arm.h to be > aligned with the toolstack. So the number of current supported > virtio-mmio devices is 10. > > For the interrupts triggering use already existing on Arm > device-model hypercall. > > The toolstack should then insert the same amount of device nodes > into guest device-tree. > > Signed-off-by: Oleksandr Tyshchenko <oleksandr_tyshchenko@epam.com> > Signed-off-by: Vikram Garhwal <vikram.garhwal@amd.com> Reviewed-by: Stefano Stabellini <sstabellini@kernel.org> > --- > hw/arm/xen_arm.c | 35 +++++++++++++++++++++++++++++++++++ > include/hw/xen/xen_native.h | 16 ++++++++++++++++ > 2 files changed, 51 insertions(+) > > diff --git a/hw/arm/xen_arm.c b/hw/arm/xen_arm.c > index 1d3e6d481a..7393b37355 100644 > --- a/hw/arm/xen_arm.c > +++ b/hw/arm/xen_arm.c > @@ -26,6 +26,7 @@ > #include "qapi/qapi-commands-migration.h" > #include "qapi/visitor.h" > #include "hw/boards.h" > +#include "hw/irq.h" > #include "hw/sysbus.h" > #include "sysemu/block-backend.h" > #include "sysemu/tpm_backend.h" > @@ -59,6 +60,38 @@ struct XenArmState { > } cfg; > }; > > +/* > + * VIRTIO_MMIO_DEV_SIZE is imported from tools/libs/light/libxl_arm.c under Xen > + * repository. > + * > + * Origin: git://xenbits.xen.org/xen.git 2128143c114c > + */ > +#define VIRTIO_MMIO_DEV_SIZE 0x200 > + > +#define NR_VIRTIO_MMIO_DEVICES \ > + (GUEST_VIRTIO_MMIO_SPI_LAST - GUEST_VIRTIO_MMIO_SPI_FIRST) > + > +static void xen_set_irq(void *opaque, int irq, int level) > +{ > + xendevicemodel_set_irq_level(xen_dmod, xen_domid, irq, level); > +} > + > +static void xen_create_virtio_mmio_devices(XenArmState *xam) > +{ > + int i; > + > + for (i = 0; i < NR_VIRTIO_MMIO_DEVICES; i++) { > + hwaddr base = GUEST_VIRTIO_MMIO_BASE + i * VIRTIO_MMIO_DEV_SIZE; > + qemu_irq irq = qemu_allocate_irq(xen_set_irq, NULL, > + GUEST_VIRTIO_MMIO_SPI_FIRST + i); > + > + sysbus_create_simple("virtio-mmio", base, irq); > + > + DPRINTF("Created virtio-mmio device %d: irq %d base 0x%lx\n", > + i, GUEST_VIRTIO_MMIO_SPI_FIRST + i, base); > + } > +} > + > void arch_handle_ioreq(XenIOState *state, ioreq_t *req) > { > hw_error("Invalid ioreq type 0x%x\n", req->type); > @@ -110,6 +143,8 @@ static void xen_arm_init(MachineState *machine) > > xen_register_ioreq(xam->state, machine->smp.cpus, &xen_memory_listener); > > + xen_create_virtio_mmio_devices(xam); > + > #ifdef CONFIG_TPM > if (xam->cfg.tpm_base_addr) { > xen_enable_tpm(xam); > diff --git a/include/hw/xen/xen_native.h b/include/hw/xen/xen_native.h > index 4dce905fde..a4b1aa9e5d 100644 > --- a/include/hw/xen/xen_native.h > +++ b/include/hw/xen/xen_native.h > @@ -523,4 +523,20 @@ static inline int xen_set_ioreq_server_state(domid_t dom, > enable); > } > > +#if CONFIG_XEN_CTRL_INTERFACE_VERSION <= 41500 > +static inline int xendevicemodel_set_irq_level(xendevicemodel_handle *dmod, > + domid_t domid, uint32_t irq, > + unsigned int level) > +{ > + return 0; > +} > +#endif > + > +#if CONFIG_XEN_CTRL_INTERFACE_VERSION <= 41700 > +#define GUEST_VIRTIO_MMIO_BASE xen_mk_ullong(0x02000000) > +#define GUEST_VIRTIO_MMIO_SIZE xen_mk_ullong(0x00100000) > +#define GUEST_VIRTIO_MMIO_SPI_FIRST 33 > +#define GUEST_VIRTIO_MMIO_SPI_LAST 43 > +#endif > + > #endif /* QEMU_HW_XEN_NATIVE_H */ > -- > 2.17.1 >
Hi Vikram, This patch prevent QEMU from been build with Xen 4.15. See comments. Also, why didn't you CC all the maintainers of include/hw/xen/xen_native.h? On Tue, Aug 29, 2023 at 09:35:17PM -0700, Vikram Garhwal wrote: > diff --git a/include/hw/xen/xen_native.h b/include/hw/xen/xen_native.h > index 4dce905fde..a4b1aa9e5d 100644 > --- a/include/hw/xen/xen_native.h > +++ b/include/hw/xen/xen_native.h > @@ -523,4 +523,20 @@ static inline int xen_set_ioreq_server_state(domid_t dom, > enable); > } > > +#if CONFIG_XEN_CTRL_INTERFACE_VERSION <= 41500 xendevicemodel_set_irq_level() was introduced in Xen 4.15, so this should say '<' and not '<=', otherwise, we have: include/hw/xen/xen_native.h:527:19: error: static declaration of ‘xendevicemodel_set_irq_level’ follows non-static declaration > +static inline int xendevicemodel_set_irq_level(xendevicemodel_handle *dmod, > + domid_t domid, uint32_t irq, > + unsigned int level) > +{ > + return 0; Shouldn't this return something like -ENOSYS, instead of returning a success? > diff --git a/hw/arm/xen_arm.c b/hw/arm/xen_arm.c > index 1d3e6d481a..7393b37355 100644 > --- a/hw/arm/xen_arm.c > +++ b/hw/arm/xen_arm.c > + > +static void xen_set_irq(void *opaque, int irq, int level) > +{ > + xendevicemodel_set_irq_level(xen_dmod, xen_domid, irq, level); So, you just ignore the return value here. Shouldn't there be some kind of error check? And is it OK to create a virtio-mmio device without an error, even when we could find out that it never going to work (e.g. on Xen 4.14)? Cheers,
Hi Anthony, On Thu, Oct 05, 2023 at 11:40:57AM +0100, Anthony PERARD wrote: > Hi Vikram, > > This patch prevent QEMU from been build with Xen 4.15. See comments. > > Also, why didn't you CC all the maintainers of > include/hw/xen/xen_native.h? I missed it. Initial version didn't have this file change and i missed updating my cc list. > > On Tue, Aug 29, 2023 at 09:35:17PM -0700, Vikram Garhwal wrote: > > diff --git a/include/hw/xen/xen_native.h b/include/hw/xen/xen_native.h > > index 4dce905fde..a4b1aa9e5d 100644 > > --- a/include/hw/xen/xen_native.h > > +++ b/include/hw/xen/xen_native.h > > @@ -523,4 +523,20 @@ static inline int xen_set_ioreq_server_state(domid_t dom, > > enable); > > } > > > > +#if CONFIG_XEN_CTRL_INTERFACE_VERSION <= 41500 > > xendevicemodel_set_irq_level() was introduced in Xen 4.15, so this > should say '<' and not '<=', otherwise, we have: > include/hw/xen/xen_native.h:527:19: error: static declaration of ‘xendevicemodel_set_irq_level’ follows non-static declaration Thanks for spotting this. I will create a fix and gitlab-ci for the patch. > > > +static inline int xendevicemodel_set_irq_level(xendevicemodel_handle *dmod, > > + domid_t domid, uint32_t irq, > > + unsigned int level) > > +{ > > + return 0; > > Shouldn't this return something like -ENOSYS, instead of returning a > success? Changed return to -ENOSYS for older version. > > > diff --git a/hw/arm/xen_arm.c b/hw/arm/xen_arm.c > > index 1d3e6d481a..7393b37355 100644 > > --- a/hw/arm/xen_arm.c > > +++ b/hw/arm/xen_arm.c > > + > > +static void xen_set_irq(void *opaque, int irq, int level) > > +{ > > + xendevicemodel_set_irq_level(xen_dmod, xen_domid, irq, level); > > So, you just ignore the return value here. Shouldn't there be some kind > of error check? > > And is it OK to create a virtio-mmio device without an error, even when > we could find out that it never going to work (e.g. on Xen 4.14)? This is something Oleksandr can answer better as it was written by him. But I think we can print an error "virtio init failed" and exit the machine init. Does that aligns with your thinking? > > Cheers, > > -- > Anthony PERARD
On Wed, Oct 11, 2023 at 12:22:46PM -0700, Vikram Garhwal wrote: > Hi Anthony, > On Thu, Oct 05, 2023 at 11:40:57AM +0100, Anthony PERARD wrote: > > Hi Vikram, > > > > This patch prevent QEMU from been build with Xen 4.15. See comments. > > > > Also, why didn't you CC all the maintainers of > > include/hw/xen/xen_native.h? > I missed it. Initial version didn't have this file change and i missed updating > my cc list. I use `cccmd` to never miss anyone, and I don't have to build a cc list ;-) $ git config sendemail.cccmd scripts/get_maintainer.pl --noroles --norolestats --nogit --nogit-fallback > > > +static inline int xendevicemodel_set_irq_level(xendevicemodel_handle *dmod, > > > + domid_t domid, uint32_t irq, > > > + unsigned int level) > > > +{ > > > + return 0; > > > > Shouldn't this return something like -ENOSYS, instead of returning a > > success? > Changed return to -ENOSYS for older version. Actually, at least on linux, looks like the function would return either -1 or 0, and set errno. It seems that xendevicemodel_set_irq_level() ultimately called ioctl(), but also the code in xen.git/tools/libs/devicemodel/ also only returns -1 or 0. So it's probably best to set errno=ENOSYS and return -1. > > > > > diff --git a/hw/arm/xen_arm.c b/hw/arm/xen_arm.c > > > index 1d3e6d481a..7393b37355 100644 > > > --- a/hw/arm/xen_arm.c > > > +++ b/hw/arm/xen_arm.c > > > + > > > +static void xen_set_irq(void *opaque, int irq, int level) > > > +{ > > > + xendevicemodel_set_irq_level(xen_dmod, xen_domid, irq, level); > > > > So, you just ignore the return value here. Shouldn't there be some kind > > of error check? > > > > And is it OK to create a virtio-mmio device without an error, even when > > we could find out that it never going to work (e.g. on Xen 4.14)? > This is something Oleksandr can answer better as it was written by him. But > I think we can print an error "virtio init failed" and exit the > machine init. Does that aligns with your thinking? Something like that, yes, if possible. It would be a bit difficult because xen_set_irq() seems to only be a handler which might only be called after the machine as started. So I'm not sure what would be best to do here. Thanks,
diff --git a/hw/arm/xen_arm.c b/hw/arm/xen_arm.c index 1d3e6d481a..7393b37355 100644 --- a/hw/arm/xen_arm.c +++ b/hw/arm/xen_arm.c @@ -26,6 +26,7 @@ #include "qapi/qapi-commands-migration.h" #include "qapi/visitor.h" #include "hw/boards.h" +#include "hw/irq.h" #include "hw/sysbus.h" #include "sysemu/block-backend.h" #include "sysemu/tpm_backend.h" @@ -59,6 +60,38 @@ struct XenArmState { } cfg; }; +/* + * VIRTIO_MMIO_DEV_SIZE is imported from tools/libs/light/libxl_arm.c under Xen + * repository. + * + * Origin: git://xenbits.xen.org/xen.git 2128143c114c + */ +#define VIRTIO_MMIO_DEV_SIZE 0x200 + +#define NR_VIRTIO_MMIO_DEVICES \ + (GUEST_VIRTIO_MMIO_SPI_LAST - GUEST_VIRTIO_MMIO_SPI_FIRST) + +static void xen_set_irq(void *opaque, int irq, int level) +{ + xendevicemodel_set_irq_level(xen_dmod, xen_domid, irq, level); +} + +static void xen_create_virtio_mmio_devices(XenArmState *xam) +{ + int i; + + for (i = 0; i < NR_VIRTIO_MMIO_DEVICES; i++) { + hwaddr base = GUEST_VIRTIO_MMIO_BASE + i * VIRTIO_MMIO_DEV_SIZE; + qemu_irq irq = qemu_allocate_irq(xen_set_irq, NULL, + GUEST_VIRTIO_MMIO_SPI_FIRST + i); + + sysbus_create_simple("virtio-mmio", base, irq); + + DPRINTF("Created virtio-mmio device %d: irq %d base 0x%lx\n", + i, GUEST_VIRTIO_MMIO_SPI_FIRST + i, base); + } +} + void arch_handle_ioreq(XenIOState *state, ioreq_t *req) { hw_error("Invalid ioreq type 0x%x\n", req->type); @@ -110,6 +143,8 @@ static void xen_arm_init(MachineState *machine) xen_register_ioreq(xam->state, machine->smp.cpus, &xen_memory_listener); + xen_create_virtio_mmio_devices(xam); + #ifdef CONFIG_TPM if (xam->cfg.tpm_base_addr) { xen_enable_tpm(xam); diff --git a/include/hw/xen/xen_native.h b/include/hw/xen/xen_native.h index 4dce905fde..a4b1aa9e5d 100644 --- a/include/hw/xen/xen_native.h +++ b/include/hw/xen/xen_native.h @@ -523,4 +523,20 @@ static inline int xen_set_ioreq_server_state(domid_t dom, enable); } +#if CONFIG_XEN_CTRL_INTERFACE_VERSION <= 41500 +static inline int xendevicemodel_set_irq_level(xendevicemodel_handle *dmod, + domid_t domid, uint32_t irq, + unsigned int level) +{ + return 0; +} +#endif + +#if CONFIG_XEN_CTRL_INTERFACE_VERSION <= 41700 +#define GUEST_VIRTIO_MMIO_BASE xen_mk_ullong(0x02000000) +#define GUEST_VIRTIO_MMIO_SIZE xen_mk_ullong(0x00100000) +#define GUEST_VIRTIO_MMIO_SPI_FIRST 33 +#define GUEST_VIRTIO_MMIO_SPI_LAST 43 +#endif + #endif /* QEMU_HW_XEN_NATIVE_H */