[QEMU,v4,1/2] xen_arm: Create virtio-mmio devices during initialization

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>
---
 hw/arm/xen_arm.c            | 35 +++++++++++++++++++++++++++++++++++
 include/hw/xen/xen_native.h | 16 ++++++++++++++++
 2 files changed, 51 insertions(+)

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,