Re: [Qemu-devel] [PATCH v6 1/4] vhost-user: add new vhost user messages to support virtio config space

[Liu, Changpeng]

> -----Original Message-----
> From: Stefan Hajnoczi [mailto:address@hidden
> Sent: Monday, December 11, 2017 9:39 PM
> To: Liu, Changpeng <address@hidden>
> Cc: address@hidden; address@hidden; address@hidden;
> address@hidden; address@hidden; Harris, James R
> <address@hidden>
> Subject: Re: [PATCH v6 1/4] vhost-user: add new vhost user messages to support
> virtio config space
> 
> On Tue, Dec 05, 2017 at 02:27:16PM +0800, Changpeng Liu wrote:
> > +* 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
> > +      filed, and slaves MUST NOT accept SET_CONFIG for read-only
> 
> s/filed/field/
Thanks.
> 
> > +static int vhost_user_set_config(struct vhost_dev *dev, const uint8_t 
> > *config,
> > +                                 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, config + offset, size);
> 
> This function can be made more general by changing the semantics of the
> config argument:
> 
>   memcpy(p, config, size);
> 
> Now the caller can pass just a single field instead of a whole 256-byte
> config buffer.  It might be clearer to name the argument "data" or
> "region" instead of "config" though.
Good suggestion, will change it.
> 
> > @@ -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 0;
> > +}
> > +
> > +int vhost_dev_set_config(struct vhost_dev *hdev, const uint8_t *config,
> > +                         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, config, offset,
> > +                                                 size, flags);
> > +    }
> > +
> > +    return 0;
> > +}
> 
> Both vhost_dev_get_config() and vhost_dev_set_config() cannot fail
> silently.  The device will not work properly if the configuration space
> feature is not supported.
> 
> Please make these functions return an error if the callback is NULL.
Ok, will add callback check.
> 
> The vhost-blk code should also check that
> hdev->vhost_ops->vhost_set_config != NULL during realize.  This way
> users see the error when adding the device instead of at runtime when
> the function gets called.
For vhost-blk, get_config is mandatory, but for set_config, it should depend
on the feature bit: VIRTIO_BLK_F_CONFIG_WCE is enabled or not. Of course,
migration should be another case. So running time error process should be
okay.