On Thu, Jul 11, 2024 at 10:10 AM Stefan Hajnoczi <stefa...@redhat.com>
wrote:
> On Fri, Jun 28, 2024 at 04:57:07PM +0200, Albert Esteve wrote:
> > The frontend can use this command to retrieve
> > VIRTIO Shared Memory Regions configuration from
> > the backend. The response contains the number of
> > shared memory regions, their size, and shmid.
> >
> > This is useful when the frontend is unaware of
> > specific backend type and configuration,
> > for example, in the `vhost-user-device` case.
> >
> > Signed-off-by: Albert Esteve <aest...@redhat.com>
> > ---
> > docs/interop/vhost-user.rst | 31 +++++++++++++++++++++++
> > hw/virtio/vhost-user.c | 42 +++++++++++++++++++++++++++++++
> > include/hw/virtio/vhost-backend.h | 6 +++++
> > include/hw/virtio/vhost-user.h | 1 +
> > 4 files changed, 80 insertions(+)
> >
> > diff --git a/docs/interop/vhost-user.rst b/docs/interop/vhost-user.rst
> > index d52ba719d5..51f01d1d84 100644
> > --- a/docs/interop/vhost-user.rst
> > +++ b/docs/interop/vhost-user.rst
> > @@ -348,6 +348,19 @@ Device state transfer parameters
> > In the future, additional phases might be added e.g. to allow
> > iterative migration while the device is running.
> >
> > +VIRTIO Shared Memory Region configuration
> > +^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^
> > +
> > ++-------------+---------+------------+----+------------+
> > +| num regions | padding | mem size 0 | .. | mem size 7 |
> > ++-------------+---------+------------+----+------------+
>
> 8 regions may not be enough. The max according to the VIRTIO spec is
> 256 because virtio-pci uses an 8-bit cap.id field for the shmid. I think
> the maximum number should be 256 here.
>
Ok, I'll set it to 255, as it starts at 0.
>
> (I haven't checked the QEMU vhost-user code to see whether it's
> reasonable to hardcode to 256 or some logic if needed to dynamically
> size the buffer depending on the "num regions" field.)
>
> > +
> > +:num regions: a 32-bit number of regions
> > +
> > +:padding: 32-bit
> > +
> > +:mem size: 64-bit size of VIRTIO Shared Memory Region
> > +
> > C structure
> > -----------
> >
> > @@ -369,6 +382,10 @@ In QEMU the vhost-user message is implemented with
> the following struct:
> > VhostUserConfig config;
> > VhostUserVringArea area;
> > VhostUserInflight inflight;
> > + VhostUserShared object;
> > + VhostUserTransferDeviceState transfer_state;
> > + VhostUserMMap mmap;
>
> Why are these added by this patch? Please add them in the same patch
> where they are introduced.
For object and transfer_state that's a ship that already sailed.
In order to still align the excerpt with the actual code, I will
split them into their own commit to avoid confusion.
> > + VhostUserShMemConfig shmem;
> > };
> > } QEMU_PACKED VhostUserMsg;
> >
> > @@ -1051,6 +1068,7 @@ Protocol features
> > #define VHOST_USER_PROTOCOL_F_XEN_MMAP 17
> > #define VHOST_USER_PROTOCOL_F_SHARED_OBJECT 18
> > #define VHOST_USER_PROTOCOL_F_DEVICE_STATE 19
> > + #define VHOST_USER_PROTOCOL_F_SHMEM 20
> >
> > Front-end message types
> > -----------------------
> > @@ -1725,6 +1743,19 @@ Front-end message types
> > Using this function requires prior negotiation of the
> > ``VHOST_USER_PROTOCOL_F_DEVICE_STATE`` feature.
> >
> > +``VHOST_USER_GET_SHMEM_CONFIG``
> > + :id: 44
> > + :equivalent ioctl: N/A
> > + :request payload: N/A
> > + :reply payload: ``struct VhostUserShMemConfig``
> > +
> > + When the ``VHOST_USER_PROTOCOL_F_SHMEM`` protocol feature has been
> > + successfully negotiated, this message can be submitted by the
> front-end
> > + to gather the VIRTIO Shared Memory Region configuration. Back-end
> will respond
> > + with the number of VIRTIO Shared Memory Regions it requires, and each
> shared memory
> > + region size in an array. The shared memory IDs are represented by the
> index
> > + of the array.
>
> Is the information returned by SHMEM_CONFIG valid and unchanged for the
> entire lifetime of the vhost-user connection?
>
> I think the answer is yes because the enumeration that virtio-pci and
> virtio-mmio transports support is basically a one-time operation at
> driver startup and it is static (Shared Memory Regions do not appear or
> go away at runtime). Please be explicit how VHOST_USER_GET_SHMEM_CONFIG
> is intended to be used.
>
Yes, I will be explicit.
>
> > +
> > Back-end message types
> > ----------------------
> >
> > diff --git a/hw/virtio/vhost-user.c b/hw/virtio/vhost-user.c
> > index 7ee8a472c6..57406dc8b4 100644
> > --- a/hw/virtio/vhost-user.c
> > +++ b/hw/virtio/vhost-user.c
> > @@ -104,6 +104,7 @@ typedef enum VhostUserRequest {
> > VHOST_USER_GET_SHARED_OBJECT = 41,
> > VHOST_USER_SET_DEVICE_STATE_FD = 42,
> > VHOST_USER_CHECK_DEVICE_STATE = 43,
> > + VHOST_USER_GET_SHMEM_CONFIG = 44,
> > VHOST_USER_MAX
> > } VhostUserRequest;
> >
> > @@ -138,6 +139,12 @@ typedef struct VhostUserMemRegMsg {
> > VhostUserMemoryRegion region;
> > } VhostUserMemRegMsg;
> >
> > +typedef struct VhostUserShMemConfig {
> > + uint32_t nregions;
> > + uint32_t padding;
> > + uint64_t memory_sizes[VHOST_MEMORY_BASELINE_NREGIONS];
> > +} VhostUserShMemConfig;
> > +
> > typedef struct VhostUserLog {
> > uint64_t mmap_size;
> > uint64_t mmap_offset;
> > @@ -245,6 +252,7 @@ typedef union {
> > VhostUserShared object;
> > VhostUserTransferDeviceState transfer_state;
> > VhostUserMMap mmap;
> > + VhostUserShMemConfig shmem;
> > } VhostUserPayload;
> >
> > typedef struct VhostUserMsg {
> > @@ -3136,6 +3144,39 @@ static int vhost_user_check_device_state(struct
> vhost_dev *dev, Error **errp)
> > return 0;
> > }
> >
> > +static int vhost_user_get_shmem_config(struct vhost_dev *dev,
> > + int *nregions,
> > + uint64_t *memory_sizes,
> > + Error **errp)
> > +{
> > + int ret;
> > + VhostUserMsg msg = {
> > + .hdr.request = VHOST_USER_GET_SHMEM_CONFIG,
> > + .hdr.flags = VHOST_USER_VERSION,
> > + };
> > +
> > + if (!virtio_has_feature(dev->protocol_features,
> > + VHOST_USER_PROTOCOL_F_SHMEM)) {
> > + return 0;
> > + }
> > +
> > + ret = vhost_user_write(dev, &msg, NULL, 0);
> > + if (ret < 0) {
> > + return ret;
> > + }
> > +
> > + ret = vhost_user_read(dev, &msg);
> > + if (ret < 0) {
> > + return ret;
> > + }
> > +
> > + *nregions = msg.payload.shmem.nregions;
>
> Missing input validation from the untrusted vhost-user backend. nregions
> may be out of range.
>
> > + memcpy(memory_sizes,
> > + &msg.payload.shmem.memory_sizes,
> > + sizeof(uint64_t) * VHOST_MEMORY_BASELINE_NREGIONS);
> > + return 0;
> > +}
> > +
> > const VhostOps user_ops = {
> > .backend_type = VHOST_BACKEND_TYPE_USER,
> > .vhost_backend_init = vhost_user_backend_init,
> > @@ -3174,4 +3215,5 @@ const VhostOps user_ops = {
> > .vhost_supports_device_state = vhost_user_supports_device_state,
> > .vhost_set_device_state_fd = vhost_user_set_device_state_fd,
> > .vhost_check_device_state = vhost_user_check_device_state,
> > + .vhost_get_shmem_config = vhost_user_get_shmem_config,
> > };
> > diff --git a/include/hw/virtio/vhost-backend.h
> b/include/hw/virtio/vhost-backend.h
> > index 70c2e8ffee..f9c2955420 100644
> > --- a/include/hw/virtio/vhost-backend.h
> > +++ b/include/hw/virtio/vhost-backend.h
> > @@ -159,6 +159,11 @@ typedef int (*vhost_set_device_state_fd_op)(struct
> vhost_dev *dev,
> > int *reply_fd,
> > Error **errp);
> > typedef int (*vhost_check_device_state_op)(struct vhost_dev *dev, Error
> **errp);
> > +typedef int (*vhost_get_shmem_config_op)(struct vhost_dev *dev,
> > + int *nregions,
> > + uint64_t *memory_sizes,
> > + Error **errp);
> > +
> >
> > typedef struct VhostOps {
> > VhostBackendType backend_type;
> > @@ -214,6 +219,7 @@ typedef struct VhostOps {
> > vhost_supports_device_state_op vhost_supports_device_state;
> > vhost_set_device_state_fd_op vhost_set_device_state_fd;
> > vhost_check_device_state_op vhost_check_device_state;
> > + vhost_get_shmem_config_op vhost_get_shmem_config;
> > } VhostOps;
> >
> > int vhost_backend_update_device_iotlb(struct vhost_dev *dev,
> > diff --git a/include/hw/virtio/vhost-user.h
> b/include/hw/virtio/vhost-user.h
> > index d7c09ffd34..e1b587a908 100644
> > --- a/include/hw/virtio/vhost-user.h
> > +++ b/include/hw/virtio/vhost-user.h
> > @@ -32,6 +32,7 @@ enum VhostUserProtocolFeature {
> > /* Feature 17 reserved for VHOST_USER_PROTOCOL_F_XEN_MMAP. */
> > VHOST_USER_PROTOCOL_F_SHARED_OBJECT = 18,
> > VHOST_USER_PROTOCOL_F_DEVICE_STATE = 19,
> > + VHOST_USER_PROTOCOL_F_SHMEM = 20,
> > VHOST_USER_PROTOCOL_F_MAX
> > };
> >
> > --
> > 2.45.2
> >
>
