qemu-devel
[Top][All Lists]
Advanced

[Date Prev][Date Next][Thread Prev][Thread Next][Date Index][Thread Index]

Re: [Qemu-devel] [PATCH v7 1/4] vhost-user: add new vhost user messages


From: Liu, Changpeng
Subject: Re: [Qemu-devel] [PATCH v7 1/4] vhost-user: add new vhost user messages to support virtio config space
Date: Thu, 21 Dec 2017 23:47:11 +0000


> -----Original Message-----
> From: Marc-André Lureau [mailto:address@hidden
> Sent: Thursday, December 21, 2017 6:48 PM
> To: Michael S. Tsirkin <address@hidden>
> Cc: Liu, Changpeng <address@hidden>; QEMU <qemu-
> address@hidden>; Harris, James R <address@hidden>; Stefan Hajnoczi
> <address@hidden>; Paolo Bonzini <address@hidden>; Felipe Franciosi
> <address@hidden>
> Subject: Re: [Qemu-devel] [PATCH v7 1/4] vhost-user: add new vhost user 
> messages
> to support virtio config space
> 
> Hi
> 
> On Thu, Dec 21, 2017 at 1:21 AM, Michael S. Tsirkin <address@hidden> wrote:
> > On Wed, Dec 20, 2017 at 04:47:13PM +0100, Marc-André Lureau wrote:
> >> Hi
> >>
> >> On Wed, Dec 13, 2017 at 3:29 AM, Changpeng Liu <address@hidden>
> wrote:
> >> > Add VHOST_USER_GET_CONFIG/VHOST_USER_SET_CONFIG messages which
> can be
> >> > used for live migration of vhost user devices, also vhost user devices
> >> > can benefit from the messages to get/set virtio config space from/to the
> >> > I/O target. For the purpose to support virtio config space change,
> >> > VHOST_USER_SET_CONFIG_FD message is added as the event notifier
> >> > in case virtio config space change in the I/O target.
> >>
> >> Why not use a new slave message instead of adding another fd to watch for?
> >>
> >> VHOST_USER_SLAVE_SET_CONFIG: notify the master of device configuration
> >> space change
> >
> >
> > Well that's a nice idea, but at v7 I'd expect it we are past such
> > fundamental suggestions.
> 
> I am sorry, I was quite busy with other work. (that happen to all of
> us). Hopefully, we still have some time for development for this
> release. Eventually, we could make this improvement on top during this
> cycle, but I would rather have protocol changes agreed before they hit
> master.
I can replace the event file descriptor for configuration space with a new 
slave message,
since the patch set was first developed based on QEMU 2.8, and the vhost slave 
channel
didn't support at that time, currently add a new slave message also make sense.
I will send v8 to cover this change and the comments you mentioned. 
Is everyone agreed that use a slave channel message about virtio configuration 
changes?
> 
> >
> > In particular Stefan wanted to require that slave is non-blocking,
> > and it's quite hard to support for the existing channel.
> 
> The notification could be non blocking, but the messages will go on
> the same channel.
> 
> The slave channel is actually not so busy and will require a single
> message (instead of notify + master get with current proposal)
> 
> Adding a slave channel message should also be easier than adding
> another event fd (both in master and slave).
> 
> >
> > It's up to the contributor whether to make this change, I'm fine
> > either way.
> >
> > Concerns such as overflow possibility below need to be addressed.
> 
> thanks
> 
> >
> >>
> >> >
> >> > Signed-off-by: Changpeng Liu <address@hidden>
> >> > ---
> >> >  docs/interop/vhost-user.txt       |  45 ++++++++++++++++
> >> >  hw/virtio/vhost-user.c            | 107
> ++++++++++++++++++++++++++++++++++++++
> >> >  hw/virtio/vhost.c                 |  64 +++++++++++++++++++++++
> >> >  include/hw/virtio/vhost-backend.h |  14 +++++
> >> >  include/hw/virtio/vhost.h         |  16 ++++++
> >> >  5 files changed, 246 insertions(+)
> >> >
> >> > diff --git a/docs/interop/vhost-user.txt b/docs/interop/vhost-user.txt
> >> > index 954771d..826ba18 100644
> >> > --- a/docs/interop/vhost-user.txt
> >> > +++ b/docs/interop/vhost-user.txt
> >> > @@ -116,6 +116,19 @@ Depending on the request type, payload can be:
> >> >      - 3: IOTLB invalidate
> >> >      - 4: IOTLB access fail
> >> >
> >> > + * Virtio device config space
> >> > +   -----------------------------------
> >> > +   | offset | size | flags | payload |
> >> > +   -----------------------------------
> >> > +
> >> > +   Offset: a 32-bit offset of virtio device's configuration space
> >> > +   Size: a 32-bit configuration space access size in bytes
> >> > +   Flags: a 32-bit value:
> >> > +    - 0: Vhost master messages used for writeable fields
> >> > +    - 1: Vhost master messages used for live migration
> >> > +   Payload: Size bytes array holding the contents of the virtio
> >> > +       device's configuration space
> >> > +
> >> >  In QEMU the vhost-user message is implemented with the following struct:
> >> >
> >> >  typedef struct VhostUserMsg {
> >> > @@ -129,6 +142,7 @@ typedef struct VhostUserMsg {
> >> >          VhostUserMemory memory;
> >> >          VhostUserLog log;
> >> >          struct vhost_iotlb_msg iotlb;
> >> > +        VhostUserConfig config;
> >> >      };
> >> >  } QEMU_PACKED VhostUserMsg;
> >> >
> >> > @@ -596,6 +610,37 @@ Master message types
> >> >        and expect this message once (per VQ) during device configuration
> >> >        (ie. before the master starts the VQ).
> >> >
> >> > + * VHOST_USER_GET_CONFIG
> >>
> >> Please add an empty line (same style as other messages). Same for
> >> other messages.
> >>
> >> > +      Id: 24
> >> > +      Equivalent ioctl: N/A
> >> > +      Master payload: virtio device config space
> >>
> >> Why is the master sending the current config space? I suppose the
> >> content should be meaningful then. Make that explicit in the spec
> >> please
> >>
> >> + missing Slave payload (make it explicit that size must match)
> >>
> >> > +
> >> > +      Submitted by the vhost-user master to fetch the contents of the 
> >> > virtio
> >> > +      device configuration space, vhost-user slave uses zero length of 
> >> > payload
> >> > +      to indicate an error to vhost-user master. The vhost-user master 
> >> > may
> >> > +      cache the contents to avoid repeated VHOST_USER_GET_CONFIG calls.
> >> > +
> >> > +* VHOST_USER_SET_CONFIG
> >> > +      Id: 25
> >> > +      Equivalent ioctl: N/A
> >> > +      Master payload: virtio device config space
> >> > +
> >> > +      Submitted by the vhost-user master when the Guest changes the 
> >> > virtio
> >> > +      device configuration space and also can be used for live migration
> >> > +      on the destination host. The vhost-user slave must check the flags
> >> > +      field, and slaves MUST NOT accept SET_CONFIG for read-only
> >> > +      configuration space fields unless the live migration bit is set.
> >>
> >> I would make reply mandatory for any newly introduced message. If not,
> >> please add the message to the list that don't in "Communication"
> >>
> >> > +
> >> > +* VHOST_USER_SET_CONFIG_FD
> >> > +      Id: 26
> >> > +      Equivalent ioctl: N/A
> >> > +      Master payload: N/A
> >> > +
> >> > +      Sets the notifier file descriptor, which is passed as ancillary 
> >> > data.
> >> > +      The vhost-user slave uses the file descriptor to notify the 
> >> > vhost-user
> >> > +      master of changes to the virtio configuration space. The 
> >> > vhost-user
> >> > +      master can read the virtio configuration space to get the latest 
> >> > update.
> >>
> >> Imho there should be a slave message instead.
> >>
> >> > +
> >> >  Slave message types
> >> >  -------------------
> >> >
> >> > diff --git a/hw/virtio/vhost-user.c b/hw/virtio/vhost-user.c
> >> > index 093675e..037c165 100644
> >> > --- a/hw/virtio/vhost-user.c
> >> > +++ b/hw/virtio/vhost-user.c
> >> > @@ -26,6 +26,11 @@
> >> >  #define VHOST_MEMORY_MAX_NREGIONS    8
> >> >  #define VHOST_USER_F_PROTOCOL_FEATURES 30
> >> >
> >> > +/*
> >> > + * Maximum size of virtio device config space
> >> > + */
> >> > +#define VHOST_USER_MAX_CONFIG_SIZE 256
> >> > +
> >> >  enum VhostUserProtocolFeature {
> >> >      VHOST_USER_PROTOCOL_F_MQ = 0,
> >> >      VHOST_USER_PROTOCOL_F_LOG_SHMFD = 1,
> >> > @@ -65,6 +70,9 @@ typedef enum VhostUserRequest {
> >> >      VHOST_USER_SET_SLAVE_REQ_FD = 21,
> >> >      VHOST_USER_IOTLB_MSG = 22,
> >> >      VHOST_USER_SET_VRING_ENDIAN = 23,
> >> > +    VHOST_USER_GET_CONFIG = 24,
> >> > +    VHOST_USER_SET_CONFIG = 25,
> >> > +    VHOST_USER_SET_CONFIG_FD = 26,
> >> >      VHOST_USER_MAX
> >> >  } VhostUserRequest;
> >> >
> >> > @@ -92,6 +100,18 @@ typedef struct VhostUserLog {
> >> >      uint64_t mmap_offset;
> >> >  } VhostUserLog;
> >> >
> >> > +typedef struct VhostUserConfig {
> >> > +    uint32_t offset;
> >> > +    uint32_t size;
> >> > +    uint32_t flags;
> >> > +    uint8_t region[VHOST_USER_MAX_CONFIG_SIZE];
> >> > +} VhostUserConfig;
> >> > +
> >> > +static VhostUserConfig c __attribute__ ((unused));
> >> > +#define VHOST_USER_CONFIG_HDR_SIZE (sizeof(c.offset) \
> >> > +                                   + sizeof(c.size) \
> >> > +                                   + sizeof(c.flags))
> >> > +
> >> >  typedef struct VhostUserMsg {
> >> >      VhostUserRequest request;
> >> >
> >> > @@ -109,6 +129,7 @@ typedef struct VhostUserMsg {
> >> >          VhostUserMemory memory;
> >> >          VhostUserLog log;
> >> >          struct vhost_iotlb_msg iotlb;
> >> > +        VhostUserConfig config;
> >> >      } payload;
> >> >  } QEMU_PACKED VhostUserMsg;
> >> >
> >> > @@ -922,6 +943,89 @@ static void vhost_user_set_iotlb_callback(struct
> vhost_dev *dev, int enabled)
> >> >      /* No-op as the receive channel is not dedicated to IOTLB messages. 
> >> > */
> >> >  }
> >> >
> >> > +static int vhost_user_get_config(struct vhost_dev *dev, uint8_t *config,
> >> > +                                 uint32_t config_len)
> >> > +{
> >> > +    VhostUserMsg msg = {
> >> > +        msg.request = VHOST_USER_GET_CONFIG,
> >> > +        msg.flags = VHOST_USER_VERSION,
> >> > +        msg.size = VHOST_USER_CONFIG_HDR_SIZE + config_len,
> >> > +    };
> >> > +
> >> > +    msg.payload.config.offset = 0;
> >> > +    msg.payload.config.size = config_len;
> >> > +    if (vhost_user_write(dev, &msg, NULL, 0) < 0) {
> >> > +        return -1;
> >> > +    }
> >>
> >> if config_len > VHOST_USER_MAX_CONFIG_SIZE, I think there will be a
> >> stack overflow. Add an assert() ?
> >
> > assert isn't a good way to handle failures. E.g. device could be added
> > by hotplug. How about disconnecting and failing device creation?
> >
> >> > +
> >> > +    if (vhost_user_read(dev, &msg) < 0) {
> >> > +        return -1;
> >> > +    }
> >> > +
> >> > +    if (msg.request != VHOST_USER_GET_CONFIG) {
> >> > +        error_report("Received unexpected msg type. Expected %d 
> >> > received %d",
> >> > +                     VHOST_USER_GET_CONFIG, msg.request);
> >> > +        return -1;
> >> > +    }
> >> > +
> >> > +    if (msg.size != VHOST_USER_CONFIG_HDR_SIZE + config_len) {
> >> > +        error_report("Received bad msg size.");
> >> > +        return -1;
> >> > +    }
> >> > +
> >> > +    memcpy(config, msg.payload.config.region, config_len);
> >> > +
> >> > +    return 0;
> >> > +}
> >> > +
> >> > +static int vhost_user_set_config(struct vhost_dev *dev, const uint8_t 
> >> > *data,
> >> > +                                 uint32_t offset, uint32_t size, 
> >> > uint32_t flags)
> >> > +{
> >> > +    uint8_t *p;
> >> > +    bool reply_supported = virtio_has_feature(dev->protocol_features,
> >> > +                                              
> >> > VHOST_USER_PROTOCOL_F_REPLY_ACK);
> >> > +
> >> > +    VhostUserMsg msg = {
> >> > +        msg.request = VHOST_USER_SET_CONFIG,
> >> > +        msg.flags = VHOST_USER_VERSION,
> >> > +        msg.size = VHOST_USER_CONFIG_HDR_SIZE + size,
> >> > +    };
> >> > +
> >> > +    if (reply_supported) {
> >> > +        msg.flags |= VHOST_USER_NEED_REPLY_MASK;
> >> > +    }
> >> > +
> >> > +    msg.payload.config.offset = offset,
> >> > +    msg.payload.config.size = size,
> >> > +    msg.payload.config.flags = flags,
> >> > +    p = msg.payload.config.region;
> >> > +    memcpy(p, data, size);
> >>
> >> here again, max config size should be checked.
> >>
> >> > +
> >> > +    if (vhost_user_write(dev, &msg, NULL, 0) < 0) {
> >> > +        return -1;
> >> > +    }
> >> > +
> >> > +    if (reply_supported) {
> >> > +        return process_message_reply(dev, &msg);
> >> > +    }
> >> > +
> >> > +    return 0;
> >> > +}
> >> > +
> >> > +static int vhost_user_set_config_fd(struct vhost_dev *dev, int fd)
> >> > +{
> >> > +    VhostUserMsg msg = {
> >> > +       .request = VHOST_USER_SET_CONFIG_FD,
> >> > +       .flags = VHOST_USER_VERSION,
> >> > +    };
> >> > +
> >> > +    if (vhost_user_write(dev, &msg, &fd, 1) < 0) {
> >> > +        return -1;
> >> > +    }
> >> > +
> >> > +    return 0;
> >> > +}
> >> > +
> >> >  const VhostOps user_ops = {
> >> >          .backend_type = VHOST_BACKEND_TYPE_USER,
> >> >          .vhost_backend_init = vhost_user_init,
> >> > @@ -948,4 +1052,7 @@ const VhostOps user_ops = {
> >> >          .vhost_net_set_mtu = vhost_user_net_set_mtu,
> >> >          .vhost_set_iotlb_callback = vhost_user_set_iotlb_callback,
> >> >          .vhost_send_device_iotlb_msg = vhost_user_send_device_iotlb_msg,
> >> > +        .vhost_get_config = vhost_user_get_config,
> >> > +        .vhost_set_config = vhost_user_set_config,
> >> > +        .vhost_set_config_fd = vhost_user_set_config_fd,
> >> >  };
> >> > diff --git a/hw/virtio/vhost.c b/hw/virtio/vhost.c
> >> > index e4290ce..aa93398 100644
> >> > --- a/hw/virtio/vhost.c
> >> > +++ b/hw/virtio/vhost.c
> >> > @@ -1358,6 +1358,9 @@ void vhost_dev_cleanup(struct vhost_dev *hdev)
> >> >      for (i = 0; i < hdev->nvqs; ++i) {
> >> >          vhost_virtqueue_cleanup(hdev->vqs + i);
> >> >      }
> >> > +    if (hdev->config_ops) {
> >> > +        event_notifier_cleanup(&hdev->config_notifier);
> >> > +    }
> >> >      if (hdev->mem) {
> >> >          /* those are only safe after successful init */
> >> >          memory_listener_unregister(&hdev->memory_listener);
> >> > @@ -1505,6 +1508,67 @@ void vhost_ack_features(struct vhost_dev *hdev,
> const int *feature_bits,
> >> >      }
> >> >  }
> >> >
> >> > +int vhost_dev_get_config(struct vhost_dev *hdev, uint8_t *config,
> >> > +                         uint32_t config_len)
> >> > +{
> >> > +    assert(hdev->vhost_ops);
> >> > +
> >> > +    if (hdev->vhost_ops->vhost_get_config) {
> >> > +        return hdev->vhost_ops->vhost_get_config(hdev, config, 
> >> > config_len);
> >> > +    }
> >> > +
> >> > +    return -1;
> >> > +}
> >> > +
> >> > +int vhost_dev_set_config(struct vhost_dev *hdev, const uint8_t *data,
> >> > +                         uint32_t offset, uint32_t size, uint32_t flags)
> >> > +{
> >> > +    assert(hdev->vhost_ops);
> >> > +
> >> > +    if (hdev->vhost_ops->vhost_set_config) {
> >> > +        return hdev->vhost_ops->vhost_set_config(hdev, data, offset,
> >> > +                                                 size, flags);
> >> > +    }
> >> > +
> >> > +    return -1;
> >> > +}
> >> > +
> >> > +static void vhost_dev_config_notifier_read(EventNotifier *n)
> >> > +{
> >> > +    struct vhost_dev *hdev = container_of(n, struct vhost_dev,
> >> > +                                         config_notifier);
> >> > +
> >> > +    if (event_notifier_test_and_clear(n)) {
> >> > +        if (hdev->config_ops) {
> >> > +            hdev->config_ops->vhost_dev_config_notifier(hdev);
> >> > +        }
> >> > +    }
> >> > +}
> >> > +
> >> > +int vhost_dev_set_config_notifier(struct vhost_dev *hdev,
> >> > +                                  const VhostDevConfigOps *ops)
> >> > +{
> >> > +    int r, fd;
> >> > +
> >> > +    assert(hdev->vhost_ops);
> >> > +
> >> > +    r = event_notifier_init(&hdev->config_notifier, 0);
> >> > +    if (r < 0) {
> >> > +        return r;
> >> > +    }
> >> > +
> >> > +    hdev->config_ops = ops;
> >> > +    event_notifier_set_handler(&hdev->config_notifier,
> >> > +                               vhost_dev_config_notifier_read);
> >> > +
> >> > +    if (hdev->vhost_ops->vhost_set_config_fd) {
> >> > +        fd  = event_notifier_get_fd(&hdev->config_notifier);
> >> > +        return hdev->vhost_ops->vhost_set_config_fd(hdev, fd);
> >> > +    }
> >> > +
> >> > +    return -1;
> >> > +}
> >> > +
> >> >  /* Host notifiers must be enabled at this point. */
> >> >  int vhost_dev_start(struct vhost_dev *hdev, VirtIODevice *vdev)
> >> >  {
> >> > diff --git a/include/hw/virtio/vhost-backend.h b/include/hw/virtio/vhost-
> backend.h
> >> > index a7a5f22..8996d5f 100644
> >> > --- a/include/hw/virtio/vhost-backend.h
> >> > +++ b/include/hw/virtio/vhost-backend.h
> >> > @@ -20,6 +20,11 @@ typedef enum VhostBackendType {
> >> >      VHOST_BACKEND_TYPE_MAX = 3,
> >> >  } VhostBackendType;
> >> >
> >> > +typedef enum VhostSetConfigType {
> >> > +    VHOST_SET_CONFIG_TYPE_MASTER = 0,
> >> > +    VHOST_SET_CONFIG_TYPE_MIGRATION = 1,
> >> > +} VhostSetConfigType;
> >> > +
> >> >  struct vhost_dev;
> >> >  struct vhost_log;
> >> >  struct vhost_memory;
> >> > @@ -84,6 +89,12 @@ typedef void (*vhost_set_iotlb_callback_op)(struct
> vhost_dev *dev,
> >> >                                             int enabled);
> >> >  typedef int (*vhost_send_device_iotlb_msg_op)(struct vhost_dev *dev,
> >> >                                                struct vhost_iotlb_msg 
> >> > *imsg);
> >> > +typedef int (*vhost_set_config_op)(struct vhost_dev *dev, const uint8_t
> *data,
> >> > +                                   uint32_t offset, uint32_t size,
> >> > +                                   uint32_t flags);
> >> > +typedef int (*vhost_get_config_op)(struct vhost_dev *dev, uint8_t 
> >> > *config,
> >> > +                                   uint32_t config_len);
> >> > +typedef int (*vhost_set_config_fd_op)(struct vhost_dev *dev, int fd);
> >> >
> >> >  typedef struct VhostOps {
> >> >      VhostBackendType backend_type;
> >> > @@ -118,6 +129,9 @@ typedef struct VhostOps {
> >> >      vhost_vsock_set_running_op vhost_vsock_set_running;
> >> >      vhost_set_iotlb_callback_op vhost_set_iotlb_callback;
> >> >      vhost_send_device_iotlb_msg_op vhost_send_device_iotlb_msg;
> >> > +    vhost_get_config_op vhost_get_config;
> >> > +    vhost_set_config_op vhost_set_config;
> >> > +    vhost_set_config_fd_op vhost_set_config_fd;
> >> >  } VhostOps;
> >> >
> >> >  extern const VhostOps user_ops;
> >> > diff --git a/include/hw/virtio/vhost.h b/include/hw/virtio/vhost.h
> >> > index 467dc77..7f43a82 100644
> >> > --- a/include/hw/virtio/vhost.h
> >> > +++ b/include/hw/virtio/vhost.h
> >> > @@ -46,6 +46,12 @@ struct vhost_iommu {
> >> >      QLIST_ENTRY(vhost_iommu) iommu_next;
> >> >  };
> >> >
> >> > +typedef struct VhostDevConfigOps {
> >> > +    /* Vhost device config space changed callback
> >> > +     */
> >> > +    void (*vhost_dev_config_notifier)(struct vhost_dev *dev);
> >> > +} VhostDevConfigOps;
> >> > +
> >> >  struct vhost_memory;
> >> >  struct vhost_dev {
> >> >      VirtIODevice *vdev;
> >> > @@ -76,6 +82,8 @@ struct vhost_dev {
> >> >      QLIST_ENTRY(vhost_dev) entry;
> >> >      QLIST_HEAD(, vhost_iommu) iommu_list;
> >> >      IOMMUNotifier n;
> >> > +    EventNotifier config_notifier;
> >> > +    const VhostDevConfigOps *config_ops;
> >> >  };
> >> >
> >> >  int vhost_dev_init(struct vhost_dev *hdev, void *opaque,
> >> > @@ -106,4 +114,12 @@ int vhost_net_set_backend(struct vhost_dev *hdev,
> >> >                            struct vhost_vring_file *file);
> >> >
> >> >  int vhost_device_iotlb_miss(struct vhost_dev *dev, uint64_t iova, int 
> >> > write);
> >> > +int vhost_dev_get_config(struct vhost_dev *dev, uint8_t *config,
> >> > +                         uint32_t config_len);
> >> > +int vhost_dev_set_config(struct vhost_dev *dev, const uint8_t *data,
> >> > +                         uint32_t offset, uint32_t size, uint32_t 
> >> > flags);
> >> > +/* notifier callback in case vhost device config space changed
> >> > + */
> >> > +int vhost_dev_set_config_notifier(struct vhost_dev *dev,
> >> > +                                  const VhostDevConfigOps *ops);
> >> >  #endif
> >> > --
> >> > 1.9.3
> >> >
> >> >
> >>
> >>
> >>
> >> --
> >> Marc-André Lureau
> 
> 
> 
> --
> Marc-André Lureau

reply via email to

[Prev in Thread] Current Thread [Next in Thread]