Re: [PULL v4 76/83] vhost-user: Support vhost_dev_start

[Michael S. Tsirkin]

On Fri, Jan 06, 2023 at 03:21:43PM +0100, Laurent Vivier wrote:
> Hi,
> 
> it seems this patch breaks vhost-user with DPDK.
> 
> See https://bugzilla.redhat.com/show_bug.cgi?id=2155173
> 
> it seems QEMU doesn't receive the expected commands sequence:
> 
> Received unexpected msg type. Expected 22 received 40
> Fail to update device iotlb
> Received unexpected msg type. Expected 40 received 22
> Received unexpected msg type. Expected 22 received 11
> Fail to update device iotlb
> Received unexpected msg type. Expected 11 received 22
> vhost VQ 1 ring restore failed: -71: Protocol error (71)
> Received unexpected msg type. Expected 22 received 11
> Fail to update device iotlb
> Received unexpected msg type. Expected 11 received 22
> vhost VQ 0 ring restore failed: -71: Protocol error (71)
> unable to start vhost net: 71: falling back on userspace virtio
> 
> It receives VHOST_USER_GET_STATUS (40) when it expects VHOST_USER_IOTLB_MSG 
> (22)
> and VHOST_USER_IOTLB_MSG when it expects VHOST_USER_GET_STATUS.
> and VHOST_USER_GET_VRING_BASE (11) when it expect VHOST_USER_GET_STATUS and 
> so on.
> 
> Any idea?
> 
> Thanks,
> Laurent


So I am guessing it's coming from:

    if (msg.hdr.request != request) {
        error_report("Received unexpected msg type. Expected %d received %d",
                     request, msg.hdr.request); 
        return -EPROTO;  
    }       

in process_message_reply and/or in vhost_user_get_u64.


> On 11/7/22 23:53, Michael S. Tsirkin wrote:
> > From: Yajun Wu <yajunw@nvidia.com>
> > 
> > The motivation of adding vhost-user vhost_dev_start support is to
> > improve backend configuration speed and reduce live migration VM
> > downtime.
> > 
> > Today VQ configuration is issued one by one. For virtio net with
> > multi-queue support, backend needs to update RSS (Receive side
> > scaling) on every rx queue enable. Updating RSS is time-consuming
> > (typical time like 7ms).
> > 
> > Implement already defined vhost status and message in the vhost
> > specification [1].
> > (a) VHOST_USER_PROTOCOL_F_STATUS
> > (b) VHOST_USER_SET_STATUS
> > (c) VHOST_USER_GET_STATUS
> > 
> > Send message VHOST_USER_SET_STATUS with VIRTIO_CONFIG_S_DRIVER_OK for
> > device start and reset(0) for device stop.
> > 
> > On reception of the DRIVER_OK message, backend can apply the needed setting
> > only once (instead of incremental) and also utilize parallelism on enabling
> > queues.
> > 
> > This improves QEMU's live migration downtime with vhost user backend
> > implementation by great margin, specially for the large number of VQs of 64
> > from 800 msec to 250 msec.
> > 
> > [1] https://qemu-project.gitlab.io/qemu/interop/vhost-user.html
> > 
> > Signed-off-by: Yajun Wu <yajunw@nvidia.com>
> > Acked-by: Parav Pandit <parav@nvidia.com>
> > Message-Id: <20221017064452.1226514-3-yajunw@nvidia.com>
> > Reviewed-by: Michael S. Tsirkin <mst@redhat.com>
> > Signed-off-by: Michael S. Tsirkin <mst@redhat.com>

Probably easiest to debug from dpdk side.
Does the problem go away if you disable the feature 
VHOST_USER_PROTOCOL_F_STATUS in dpdk?

> > ---
> >   hw/virtio/vhost-user.c | 74 +++++++++++++++++++++++++++++++++++++++++-
> >   1 file changed, 73 insertions(+), 1 deletion(-)
> > 
> > diff --git a/hw/virtio/vhost-user.c b/hw/virtio/vhost-user.c
> > index d256ce589b..abe23d4ebe 100644
> > --- a/hw/virtio/vhost-user.c
> > +++ b/hw/virtio/vhost-user.c
> > @@ -81,6 +81,7 @@ enum VhostUserProtocolFeature {
> >       VHOST_USER_PROTOCOL_F_RESET_DEVICE = 13,
> >       /* Feature 14 reserved for 
> > VHOST_USER_PROTOCOL_F_INBAND_NOTIFICATIONS. */
> >       VHOST_USER_PROTOCOL_F_CONFIGURE_MEM_SLOTS = 15,
> > +    VHOST_USER_PROTOCOL_F_STATUS = 16,
> >       VHOST_USER_PROTOCOL_F_MAX
> >   };
> > @@ -126,6 +127,8 @@ typedef enum VhostUserRequest {
> >       VHOST_USER_GET_MAX_MEM_SLOTS = 36,
> >       VHOST_USER_ADD_MEM_REG = 37,
> >       VHOST_USER_REM_MEM_REG = 38,
> > +    VHOST_USER_SET_STATUS = 39,
> > +    VHOST_USER_GET_STATUS = 40,
> >       VHOST_USER_MAX
> >   } VhostUserRequest;
> > @@ -1452,6 +1455,43 @@ static int vhost_user_set_u64(struct vhost_dev *dev, 
> > int request, uint64_t u64,
> >       return 0;
> >   }
> > +static int vhost_user_set_status(struct vhost_dev *dev, uint8_t status)
> > +{
> > +    return vhost_user_set_u64(dev, VHOST_USER_SET_STATUS, status, false);
> > +}
> > +
> > +static int vhost_user_get_status(struct vhost_dev *dev, uint8_t *status)
> > +{
> > +    uint64_t value;
> > +    int ret;
> > +
> > +    ret = vhost_user_get_u64(dev, VHOST_USER_GET_STATUS, &value);
> > +    if (ret < 0) {
> > +        return ret;
> > +    }
> > +    *status = value;
> > +
> > +    return 0;
> > +}
> > +
> > +static int vhost_user_add_status(struct vhost_dev *dev, uint8_t status)
> > +{
> > +    uint8_t s;
> > +    int ret;
> > +
> > +    ret = vhost_user_get_status(dev, &s);
> > +    if (ret < 0) {
> > +        return ret;
> > +    }
> > +
> > +    if ((s & status) == status) {
> > +        return 0;
> > +    }
> > +    s |= status;
> > +
> > +    return vhost_user_set_status(dev, s);
> > +}
> > +
> >   static int vhost_user_set_features(struct vhost_dev *dev,
> >                                      uint64_t features)
> >   {
> > @@ -1460,6 +1500,7 @@ static int vhost_user_set_features(struct vhost_dev 
> > *dev,
> >        * backend is actually logging changes
> >        */
> >       bool log_enabled = features & (0x1ULL << VHOST_F_LOG_ALL);
> > +    int ret;
> >       /*
> >        * We need to include any extra backend only feature bits that
> > @@ -1467,9 +1508,18 @@ static int vhost_user_set_features(struct vhost_dev 
> > *dev,
> >        * VHOST_USER_F_PROTOCOL_FEATURES bit for enabling protocol
> >        * features.
> >        */
> > -    return vhost_user_set_u64(dev, VHOST_USER_SET_FEATURES,
> > +    ret = vhost_user_set_u64(dev, VHOST_USER_SET_FEATURES,
> >                                 features | dev->backend_features,
> >                                 log_enabled);
> > +
> > +    if (virtio_has_feature(dev->protocol_features,
> > +                           VHOST_USER_PROTOCOL_F_STATUS)) {
> > +        if (!ret) {
> > +            return vhost_user_add_status(dev, VIRTIO_CONFIG_S_FEATURES_OK);
> > +        }
> > +    }
> > +
> > +    return ret;
> >   }
> >   static int vhost_user_set_protocol_features(struct vhost_dev *dev,
> > @@ -2620,6 +2670,27 @@ void vhost_user_cleanup(VhostUserState *user)
> >       user->chr = NULL;
> >   }
> > +static int vhost_user_dev_start(struct vhost_dev *dev, bool started)
> > +{
> > +    if (!virtio_has_feature(dev->protocol_features,
> > +                            VHOST_USER_PROTOCOL_F_STATUS)) {
> > +        return 0;
> > +    }
> > +
> > +    /* Set device status only for last queue pair */
> > +    if (dev->vq_index + dev->nvqs != dev->vq_index_end) {
> > +        return 0;
> > +    }
> > +
> > +    if (started) {
> > +        return vhost_user_add_status(dev, VIRTIO_CONFIG_S_ACKNOWLEDGE |
> > +                                          VIRTIO_CONFIG_S_DRIVER |
> > +                                          VIRTIO_CONFIG_S_DRIVER_OK);
> > +    } else {
> > +        return vhost_user_set_status(dev, 0);
> > +    }
> > +}
> > +
> >   const VhostOps user_ops = {
> >           .backend_type = VHOST_BACKEND_TYPE_USER,
> >           .vhost_backend_init = vhost_user_backend_init,
> > @@ -2654,4 +2725,5 @@ const VhostOps user_ops = {
> >           .vhost_backend_mem_section_filter = vhost_user_mem_section_filter,
> >           .vhost_get_inflight_fd = vhost_user_get_inflight_fd,
> >           .vhost_set_inflight_fd = vhost_user_set_inflight_fd,
> > +        .vhost_dev_start = vhost_user_dev_start,
> >   };