Re: [RFC PATCH] virtio-mmio: add xenbus probing

[Jürgen Groß <jgross@xxxxxxxx>]

Some minor details from the Xen side of things:

On 29.04.26 15:52, Val Packett wrote:
> The experimental virtio-mmio support for Xen was initially developed
> on aarch64, so device trees were used to configure the mmio devices,
> with arbitrary vGIC interrupts used by the hypervisor. On x86_64
> however, the only reasonable way to interrupt the guest is over Xen
> event channels, which can only be acquired by children of xenbus,
More exact: interdomain event channels need to be connected to a xenbus
device. But you are needing those, so for your use case the above statement
is correct.

> the virtual bus driven by Xen's configuration database, XenStore.
> It is also a more convenient and "Xen-ish" way to provision devices.
>
> Implement a xenbus client for virtio-mmio which negotiates an
> event channel and provides it as a platform IRQ to the
> virtio-mmio driver.
>
>
> Signed-off-by: Val Packett <val@xxxxxxxxxxxxxxxxxxxxxx>
> ---
>
> Hi,
>
> I've been working on porting virtio-mmio support from Arm to x86_64,
> with the goal of running vhost-user-gpu to power Wayland/GPU integration
> for Qubes OS. (I'm aware of various proposals for alternative virtio
> transports but virtio-mmio seems to be the only one that *is* upstream
> already and just Works..) Setting up virtio-mmio through xenbus, initially
> motivated just by event channels being the only real way to get interrupts
> working on HVM, turned out to generally be quite pleasant and nice :)
>
> I'd like to get some early feedback for this patch, particularly
> the general stuff:
>
> * is this whole thing acceptable in general?
> * should it be extracted into a different file?
> * (from the Xen side) any input on the xenstore keys, what goes where?
You should add some documentation in the Xen source tree regarding the
Xenstore keys (see docs/misc/xenstore-paths.pandoc there).

> * anything else to keep in mind?
>
> It does seem simple enough, so hopefully this can be done?
>
> The corresponding userspace-side WIP is available at:
> https://github.com/QubesOS/xen-vhost-frontend
>
> And the required DMOP for firing the evtchn events will be sent
> to xen-devel shortly as well.
>
> Thanks,
> ~val
>
> ---
>   drivers/virtio/Kconfig       |   7 ++
>   drivers/virtio/virtio_mmio.c | 177 ++++++++++++++++++++++++++++++++++-
>   2 files changed, 183 insertions(+), 1 deletion(-)
>
> diff --git a/drivers/virtio/Kconfig b/drivers/virtio/Kconfig
> index ce5bc0d9ea28..56bc2b10526b 100644
> --- a/drivers/virtio/Kconfig
> +++ b/drivers/virtio/Kconfig
> @@ -171,6 +171,13 @@ config VIRTIO_MMIO_CMDLINE_DEVICES
>   
>   	 If unsure, say 'N'.
>   
> +config VIRTIO_MMIO_XENBUS
> +       bool "Memory mapped virtio devices parameter parsing"
> +       depends on VIRTIO_MMIO && XEN
> +       select XEN_XENBUS_FRONTEND
> +       help
> +        Allow virtio-mmio devices instantiation for Xen guests via xenbus.
> +
>   config VIRTIO_DMA_SHARED_BUFFER
>         tristate
>         depends on DMA_SHARED_BUFFER
> diff --git a/drivers/virtio/virtio_mmio.c b/drivers/virtio/virtio_mmio.c
> index 595c2274fbb5..32295284bdbf 100644
> --- a/drivers/virtio/virtio_mmio.c
> +++ b/drivers/virtio/virtio_mmio.c
> @@ -70,6 +70,11 @@
>   #include <uapi/linux/virtio_mmio.h>
>   #include <linux/virtio_ring.h>
>   
> +#ifdef CONFIG_VIRTIO_MMIO_XENBUS
> +#include <xen/xen.h>
> +#include <xen/xenbus.h>
> +#include <xen/events.h>
> +#endif
>   
>   
>   /* The alignment to use between consumer and producer parts of vring.
> @@ -810,13 +815,183 @@ static struct platform_driver virtio_mmio_driver = {
>         },
>   };
>   
> +#ifdef CONFIG_VIRTIO_MMIO_XENBUS
> +struct virtio_mmio_xen_info {
> +       struct resource resources[2];
> +       unsigned int evtchn;
> +       struct platform_device *pdev;
> +};
> +
> +static int virtio_mmio_xen_probe(struct xenbus_device *dev,
> +                       const struct xenbus_device_id *id)
> +{
> +       int err;
> +       long long base, size;
> +       char *mem;
> +       struct virtio_mmio_xen_info *info;
> +       struct xenbus_transaction xbt;
> +
> +       /* TODO: allocate an unused address here and pass it to the host 
> instead */
Indeed.

> +       err = xenbus_scanf(XBT_NIL, dev->otherend, "base", "0x%llx",
> +                          &base);
> +       if (err < 0) {
> +               xenbus_dev_fatal(dev, err, "reading base");
> +               return -EINVAL;
> +       }
> +
> +       mem = xenbus_read(XBT_NIL, dev->otherend, "size", NULL);
> +       if (XENBUS_IS_ERR_READ(mem))
> +               return PTR_ERR(mem);
> +       size = memparse(mem, NULL);
> +       kfree(mem);
> +
> +       info = kzalloc_obj(*info);
> +       if (!info) {
> +               xenbus_dev_fatal(dev, -ENOMEM, "allocating info structure");
> +               return -ENOMEM;
> +       }
> +
> +       info->resources[0].flags = IORESOURCE_MEM;
> +       info->resources[0].start = base;
> +       info->resources[0].end = base + size - 1;
> +
> +       err = xenbus_alloc_evtchn(dev, &info->evtchn);
> +       if (err) {
> +               xenbus_dev_fatal(dev, err, "xenbus_alloc_evtchn");
> +               goto error_info;
> +       }
> +
> +       err = bind_evtchn_to_irq(info->evtchn);
> +       if (err <= 0) {
> +               xenbus_dev_fatal(dev, err, "bind_evtchn_to_irq");
> +               goto error_evtchan;
> +       }
> +
> +       info->resources[1].flags = IORESOURCE_IRQ;
> +       info->resources[1].start = info->resources[1].end = err;
> +
> +again:
> +       err = xenbus_transaction_start(&xbt);
No need to use a Xenstore transaction here. The written node(s) are
regarded to be valid only after calling xenbus_switch_state() to set
the frontend state to XenbusStateInitialised.

> +       if (err) {
> +               xenbus_dev_fatal(dev, err, "starting transaction");
> +               goto error_irq;
> +       }
> +
> +       err = xenbus_printf(xbt, dev->nodename, "event-channel", "%u",
> +                           info->evtchn);
With allocation of the base address you'd want to write it to another node,
of course.

> +       if (err) {
> +               xenbus_transaction_end(xbt, 1);
> +               xenbus_dev_fatal(dev, err, "%s", "writing event-channel");
> +               goto error_irq;
> +       }
> +
> +       err = xenbus_transaction_end(xbt, 0);
> +       if (err) {
> +               if (err == -EAGAIN)
> +                       goto again;
> +               xenbus_dev_fatal(dev, err, "completing transaction");
> +               goto error_irq;
> +       }
> +
> +       dev_set_drvdata(&dev->dev, info);
> +       xenbus_switch_state(dev, XenbusStateInitialised);
> +       return 0;
> +
> +error_irq:
> +       unbind_from_irqhandler(info->resources[1].start, info);
> +error_evtchan:
> +       xenbus_free_evtchn(dev, info->evtchn);
> +error_info:
> +       kfree(info);
> +
> +       return err;
> +}
> +
> +static void virtio_mmio_xen_backend_changed(struct xenbus_device *dev,
> +                                  enum xenbus_state backend_state)
> +{
> +       struct virtio_mmio_xen_info *info = dev_get_drvdata(&dev->dev);
> +
> +       switch (backend_state) {
> +       case XenbusStateInitialising:
> +       case XenbusStateInitWait:
> +       case XenbusStateInitialised:
> +       case XenbusStateReconfiguring:
> +       case XenbusStateReconfigured:
> +       case XenbusStateUnknown:
> +               break;
> +
> +       case XenbusStateConnected:
> +               if (dev->state != XenbusStateInitialised) {
> +                       dev_warn(&dev->dev, "state %d on connect", dev->state);
> +                       break;
> +               }
> +               info->pdev = platform_device_register_resndata(&dev->dev,
> +                               "virtio-mmio", PLATFORM_DEVID_AUTO,
> +                               info->resources, ARRAY_SIZE(info->resources), 
> NULL, 0);
> +               xenbus_switch_state(dev, XenbusStateConnected);
> +               break;
> +
> +       case XenbusStateClosed:
> +               if (dev->state == XenbusStateClosed)
> +                       break;
> +               fallthrough;    /* Missed the backend's Closing state. */
> +       case XenbusStateClosing:
> +               platform_device_unregister(info->pdev);
> +               xenbus_switch_state(dev, XenbusStateClosed);
> +               break;
> +
> +       default:
> +               xenbus_dev_fatal(dev, -EINVAL, "saw state %d at frontend",
> +                                backend_state);
> +               break;
> +       }
> +}
> +
> +static void virtio_mmio_xen_remove(struct xenbus_device *dev)
> +{
> +       struct virtio_mmio_xen_info *info = dev_get_drvdata(&dev->dev);
> +
> +       kfree(info);
> +       dev_set_drvdata(&dev->dev, NULL);
> +}
> +
> +static const struct xenbus_device_id virtio_mmio_xen_ids[] = {
> +       { "virtio" },
Please use "virtio-mmio" here, as I could imagine "virtio-pci" devices, too.


Juergen

> +       { "" },
> +};
> +
> +static struct xenbus_driver virtio_mmio_xen_driver = {
> +       .ids                    = virtio_mmio_xen_ids,
> +       .probe                  = virtio_mmio_xen_probe,
> +       .otherend_changed       = virtio_mmio_xen_backend_changed,
> +       .remove                 = virtio_mmio_xen_remove,
> +};
> +#endif
> +
>   static int __init virtio_mmio_init(void)
>   {
> -       return platform_driver_register(&virtio_mmio_driver);
> +       int ret;
> +
> +       ret = platform_driver_register(&virtio_mmio_driver);
> +       if (ret)
> +               return ret;
> +
> +#ifdef CONFIG_VIRTIO_MMIO_XENBUS
> +       if (xen_domain())
> +               ret = xenbus_register_frontend(&virtio_mmio_xen_driver);
> +#endif
> +
> +       return ret;
>   }
>   
>   static void __exit virtio_mmio_exit(void)
>   {
> +#ifdef CONFIG_VIRTIO_MMIO_XENBUS
> +       if (xen_domain())
> +               xenbus_unregister_driver(&virtio_mmio_xen_driver);
> +#endif
> +
>         platform_driver_unregister(&virtio_mmio_driver);
>         vm_unregister_cmdline_devices();
>   }

Attachment: OpenPGP_0xB0DE9DD628BF132F.asc
Description: OpenPGP public key

Attachment: OpenPGP_signature.asc
Description: OpenPGP digital signature