Re: [RFC PATCH v2 2/5] vhost_user: Add frontend command for shmem config

[Stefan Hajnoczi]

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 <aesteve@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.

(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.

> +          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.

> +
>  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
>

signature.asc
Description: PGP signature