On Tue, Aug 19, 2025 at 12:42 PM Stefan Hajnoczi <stefa...@redhat.com> wrote:
>
> On Mon, Aug 18, 2025 at 12:03:53PM +0200, Albert Esteve wrote:
> > Add shared memory BAR support to vhost-user-device-pci
> > to enable direct file mapping for VIRTIO Shared
> > Memory Regions.
> >
> > The implementation creates a consolidated shared
> > memory BAR that contains all VIRTIO Shared
> > Memory Regions as subregions. Each region is
> > configured with its proper shmid, size, and
> > offset within the BAR. The number and size of
> > regions are retrieved via VHOST_USER_GET_SHMEM_CONFIG
> > message sent by vhost-user-base during realization
> > after virtio_init().
> >
> > Specifiically, it uses BAR 3 to avoid conflicts, as
> > it is currently unused.
> >
> > The shared memory BAR is only created when the
> > backend supports VHOST_USER_PROTOCOL_F_SHMEM and
> > has configured shared memory regions. This maintains
> > backward compatibility with backends that do not
> > support shared memory functionality.
> >
> > Signed-off-by: Albert Esteve <aest...@redhat.com>
> > ---
> > hw/virtio/vhost-user-base.c | 49 +++++++++++++++++++++++++++++--
> > hw/virtio/vhost-user-device-pci.c | 34 +++++++++++++++++++--
> > 2 files changed, 78 insertions(+), 5 deletions(-)
> >
> > diff --git a/hw/virtio/vhost-user-base.c b/hw/virtio/vhost-user-base.c
> > index ff67a020b4..932f9b5596 100644
> > --- a/hw/virtio/vhost-user-base.c
> > +++ b/hw/virtio/vhost-user-base.c
> > @@ -16,6 +16,7 @@
> > #include "hw/virtio/virtio-bus.h"
> > #include "hw/virtio/vhost-user-base.h"
> > #include "qemu/error-report.h"
> > +#include "migration/blocker.h"
> >
> > static void vub_start(VirtIODevice *vdev)
> > {
> > @@ -276,7 +277,9 @@ static void vub_device_realize(DeviceState *dev, Error
> > **errp)
> > {
> > VirtIODevice *vdev = VIRTIO_DEVICE(dev);
> > VHostUserBase *vub = VHOST_USER_BASE(dev);
> > - int ret;
> > + uint64_t memory_sizes[VIRTIO_MAX_SHMEM_REGIONS];
> > + g_autofree char *name = NULL;
> > + int i, ret, nregions;
> >
> > if (!vub->chardev.chr) {
> > error_setg(errp, "vhost-user-base: missing chardev");
> > @@ -319,7 +322,7 @@ static void vub_device_realize(DeviceState *dev, Error
> > **errp)
> >
> > /* Allocate queues */
> > vub->vqs = g_ptr_array_sized_new(vub->num_vqs);
> > - for (int i = 0; i < vub->num_vqs; i++) {
> > + for (i = 0; i < vub->num_vqs; i++) {
> > g_ptr_array_add(vub->vqs,
> > virtio_add_queue(vdev, vub->vq_size,
> > vub_handle_output));
> > @@ -333,11 +336,51 @@ static void vub_device_realize(DeviceState *dev,
> > Error **errp)
> > VHOST_BACKEND_TYPE_USER, 0, errp);
> >
> > if (ret < 0) {
> > - do_vhost_user_cleanup(vdev, vub);
> > + goto err;
> > + }
> > +
> > + ret = vub->vhost_dev.vhost_ops->vhost_get_shmem_config(&vub->vhost_dev,
> > + &nregions,
> > + memory_sizes,
> > + errp);
> > +
> > + if (ret < 0) {
> > + goto err;
> > + }
> > +
> > + for (i = 0; i < nregions; i++) {
> > + if (memory_sizes[i]) {
> > + if (vub->vhost_dev.migration_blocker == NULL) {
> > + error_setg(&vub->vhost_dev.migration_blocker,
> > + "Migration disabled: devices with VIRTIO Shared
> > Memory "
> > + "Regions do not support migration yet.");
> > + ret = migrate_add_blocker_normal(
> > + &vub->vhost_dev.migration_blocker,
> > + errp);
> > +
> > + if (ret < 0) {
> > + goto err;
> > + }
> > + }
> > +
> > + if (memory_sizes[i] % qemu_real_host_page_size() != 0) {
> > + error_setg(errp, "Shared memory %d size must be a power of
> > 2 "
> > + "no smaller than the page size", i);
> > + goto err;
> > + }
> > +
> > + name = g_strdup_printf("vub-shm-%d", i);
>
> name is leaked because it's scope extends until the end of the function
> (after the loop) but a newly allocated string is assigned each time
> around the loop. This can be fixed by moving the local variable
> declaration inside the if statement body.
>
> > + memory_region_init(&virtio_new_shmem_region(vdev, i)->mr,
> > + OBJECT(vdev), name,
> > + memory_sizes[i]);
>
> ->mr is already initialized inside virtio_new_shmem_region(). I suggest
> changing the definition of virtio_new_shmem_region() like this:
>
> void virtio_add_shmem_region(VirtIODevice *vdev, uint8_t shmid,
> uint64_t size)
>
> and then calling it like this:
>
> virtio_add_shmem_region(vdev, shmid, memory_sizes[i]);
>
> ("new" usually returns a new instance whereas "add" modifies an owner
> object/container. I think "add" is more appropriate here.)
Yes, I was checking your comment in the first patch and came to this.
I was changing it as you suggested. I messed that up with double init
and max size.
>
> > + }
> > }
> >
> > qemu_chr_fe_set_handlers(&vub->chardev, NULL, NULL, vub_event, NULL,
> > dev, NULL, true);
> > + return;
> > +err:
> > + do_vhost_user_cleanup(vdev, vub);
> > }
> >
> > static void vub_device_unrealize(DeviceState *dev)
> > diff --git a/hw/virtio/vhost-user-device-pci.c
> > b/hw/virtio/vhost-user-device-pci.c
> > index f10bac874e..bac99e7c60 100644
> > --- a/hw/virtio/vhost-user-device-pci.c
> > +++ b/hw/virtio/vhost-user-device-pci.c
> > @@ -8,14 +8,18 @@
> > */
> >
> > #include "qemu/osdep.h"
> > +#include "qapi/error.h"
> > #include "hw/qdev-properties.h"
> > #include "hw/virtio/vhost-user-base.h"
> > #include "hw/virtio/virtio-pci.h"
> >
> > +#define VIRTIO_DEVICE_PCI_SHMEM_BAR 3
> > +
> > struct VHostUserDevicePCI {
> > VirtIOPCIProxy parent_obj;
> >
> > VHostUserBase vub;
> > + MemoryRegion shmembar;
> > };
> >
> > #define TYPE_VHOST_USER_DEVICE_PCI "vhost-user-device-pci-base"
> > @@ -25,10 +29,36 @@ OBJECT_DECLARE_SIMPLE_TYPE(VHostUserDevicePCI,
> > VHOST_USER_DEVICE_PCI)
> > static void vhost_user_device_pci_realize(VirtIOPCIProxy *vpci_dev, Error
> > **errp)
> > {
> > VHostUserDevicePCI *dev = VHOST_USER_DEVICE_PCI(vpci_dev);
> > - DeviceState *vdev = DEVICE(&dev->vub);
> > + DeviceState *dev_state = DEVICE(&dev->vub);
> > + VirtIODevice *vdev = VIRTIO_DEVICE(dev_state);
> > + VirtioSharedMemory *shmem, *next;
> > + uint64_t offset = 0, shmem_size = 0;
> >
> > vpci_dev->nvectors = 1;
> > - qdev_realize(vdev, BUS(&vpci_dev->bus), errp);
> > + qdev_realize(dev_state, BUS(&vpci_dev->bus), errp);
> > +
> > + QSIMPLEQ_FOREACH_SAFE(shmem, &vdev->shmem_list, entry, next) {
>
> This is not specific to vhost-user-device-pci.c. All VIRTIO devices with
> Shared Memory Regions need PCI BAR setup code. Since vdev->shmem_list is
> part of the core hw/virtio/ code, it would make sense to move this into
> into hw/virtio/virtio-pci.c.
>
> > + if (shmem->mr.size > UINT64_MAX - shmem_size) {
> > + error_setg(errp, "Total shared memory required overflow");
> > + return;
> > + }
> > + shmem_size = shmem_size + shmem->mr.size;
> > + }
> > + if (shmem_size) {
> > + memory_region_init(&dev->shmembar, OBJECT(vpci_dev),
> > + "vhost-device-pci-shmembar", shmem_size);
> > + QSIMPLEQ_FOREACH_SAFE(shmem, &vdev->shmem_list, entry, next) {
> > + memory_region_add_subregion(&dev->shmembar, offset,
> > &shmem->mr);
> > + virtio_pci_add_shm_cap(vpci_dev, VIRTIO_DEVICE_PCI_SHMEM_BAR,
> > + offset, shmem->mr.size, shmem->shmid);
> > + offset = offset + shmem->mr.size;
> > + }
> > + pci_register_bar(&vpci_dev->pci_dev, VIRTIO_DEVICE_PCI_SHMEM_BAR,
> > + PCI_BASE_ADDRESS_SPACE_MEMORY |
> > + PCI_BASE_ADDRESS_MEM_PREFETCH |
> > + PCI_BASE_ADDRESS_MEM_TYPE_64,
> > + &dev->shmembar);
>
> This does not follow the same approach as virtio-gpu-pci.c and
> virtio-vga.c. They config the VirtIOPCIProxy's BARs
> (->modern_io_bar_idx, ->modern_mem_bar_idx, and ->msix_bar_idx) to
> control the BAR layout first and then call qdev_realize().
>
> Why does this patch do things differently? It looks like it's assuming
> vpci_dev always has a specific BAR layout (it could change).
>
> > + }
>
> > }
> >
> > static void vhost_user_device_pci_class_init(ObjectClass *klass,
> > --
> > 2.49.0
> >
