Re: [PATCH v4 7/9] vhost-user-devive: Add shmem BAR

[Stefan Hajnoczi <stefanha@redhat.com>]

[-- Attachment #1: Type: text/plain, Size: 8686 bytes --]

On Tue, Feb 18, 2025 at 11:55:33AM +0100, Albert Esteve wrote:
> On Tue, Feb 18, 2025 at 11:41 AM Stefan Hajnoczi <stefanha@redhat.com> wrote:
> >
> > On Mon, Feb 17, 2025 at 05:40:10PM +0100, Albert Esteve wrote:
> > > Add a shmem BAR block in the vhost-user-device,
> > > which files can be directly mapped into.
> > >
> > > The number, shmid, and size of the VIRTIO Shared
> > > Memory subregions is retrieved through a
> > > get_shmem_config message sent by the
> > > vhost-user-base module on the realize step,
> > > after virtio_init().
> > >
> > > By default, if VHOST_USER_PROTOCOL_F_SHMEM
> > > feature is not supported by the backend,
> > > there is no cache.
> > >
> > > Signed-off-by: Albert Esteve <aesteve@redhat.com>
> > > ---
> > >  hw/virtio/vhost-user-base.c       | 47 +++++++++++++++++++++++++++++--
> > >  hw/virtio/vhost-user-device-pci.c | 36 +++++++++++++++++++++--
> > >  2 files changed, 78 insertions(+), 5 deletions(-)
> > >
> > > diff --git a/hw/virtio/vhost-user-base.c b/hw/virtio/vhost-user-base.c
> > > index 2bc3423326..8d4bca98a8 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)
> > >  {
> > > @@ -271,7 +272,8 @@ 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];
> > > +    int i, ret, nregions;
> > >
> > >      if (!vub->chardev.chr) {
> > >          error_setg(errp, "vhost-user-base: missing chardev");
> > > @@ -314,7 +316,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));
> > > @@ -328,11 +330,50 @@ 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;
> > > +            }
> > > +
> > > +            memory_region_init(virtio_new_shmem_region(vdev)->mr,
> >
> > Does this code support non-contiguous shmids? For example, if a device
> > has two Shared Memory Regions defined in its spec but the first one is
> > optional, then the device might have memory_sizes[0] == 0 and
> > memory_sizes[1] > 0. In that case the Shared Memory Region must have
> > shmid 1 and not shmid 0.
> 
> Yes, it does. That is guarded by ` if (memory_sizes[i]) {`, which only
> initializes the region if memory_sizes[i] > 0. The main downsize of
> that, is that it requires to send as many `memory_sizes` elements as
> the highest shmid for the device. But as it is, it is supported by
> this code.

shmids are not preserved when there are gaps:

  for (i = 0; i < vdev->n_shmem_regions; i++) {
      memory_region_add_subregion(&dev->shmembar, offset, mr);
      virtio_pci_add_shm_cap(vpci_dev, VIRTIO_DEVICE_PCI_SHMEM_BAR,
                             offset, mr->size, i);
			                       ^

vdev->n_shmem_regions is incremented by virtio_new_shmem_region().
virtio_new_shmem_region() is only called on non-empty Shared Memory
Regions.

In the example I gave with empty shmid 0 and non-empty shmid 1 I think
we end up with vdev->n_shmem_regions == 1. shmdid 1 is exposed to the
guest with shmid 0.

Have I missed something?

> >
> > > +                               OBJECT(vdev), "vub-shm-" + i,
> > > +                               memory_sizes[i]);
> > > +        }
> > >      }
> > >
> > >      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 efaf55d3dd..f215cae925 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 2
> > > +
> > >  struct VHostUserDevicePCI {
> > >      VirtIOPCIProxy parent_obj;
> > >
> > >      VHostUserBase vub;
> > > +    MemoryRegion shmembar;
> > >  };
> > >
> > >  #define TYPE_VHOST_USER_DEVICE_PCI "vhost-user-device-pci-base"
> > > @@ -25,10 +29,38 @@ 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);
> > > +    MemoryRegion *mr;
> > > +    uint64_t offset = 0, shmem_size = 0;
> > > +    int i;
> > >
> > >      vpci_dev->nvectors = 1;
> > > -    qdev_realize(vdev, BUS(&vpci_dev->bus), errp);
> > > +    qdev_realize(dev_state, BUS(&vpci_dev->bus), errp);
> > > +
> > > +    for (i = 0; i < vdev->n_shmem_regions; i++) {
> > > +        mr = vdev->shmem_list[i].mr;
> > > +        if (mr->size > UINT64_MAX - shmem_size) {
> > > +            error_setg(errp, "Total shared memory required overflow");
> > > +            return;
> > > +        }
> > > +        shmem_size = shmem_size + mr->size;
> > > +    }
> > > +    if (shmem_size) {
> > > +        memory_region_init(&dev->shmembar, OBJECT(vpci_dev),
> > > +                           "vhost-device-pci-shmembar", shmem_size);
> > > +        for (i = 0; i < vdev->n_shmem_regions; i++) {
> > > +            memory_region_add_subregion(&dev->shmembar, offset, mr);
> > > +            virtio_pci_add_shm_cap(vpci_dev, VIRTIO_DEVICE_PCI_SHMEM_BAR,
> > > +                                   offset, mr->size, i);
> > > +            offset = offset + 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);
> > > +    }
> > >  }
> > >
> > >  static void vhost_user_device_pci_class_init(ObjectClass *klass, void *data)
> > > --
> > > 2.48.1
> > >
> 

[-- Attachment #2: signature.asc --]
[-- Type: application/pgp-signature, Size: 488 bytes --]