Re: [Qemu-devel] [PATCH v2 2/2] vhost-user: back SET/GET_CONFIG requests with a protocol feature

[Maxime Coquelin]

On 03/29/2018 10:05 AM, Liu, Changpeng wrote:
> > -----Original Message-----
> > From: Maxime Coquelin [mailto:address@hidden
> > Sent: Thursday, March 29, 2018 3:56 PM
> > To: Liu, Changpeng <address@hidden>; address@hidden;
> > address@hidden; address@hidden
> > Subject: Re: [PATCH v2 2/2] vhost-user: back SET/GET_CONFIG requests with a
> > protocol feature
> >
> >
> >
> > On 03/29/2018 02:57 AM, Liu, Changpeng wrote:
> > > > -----Original Message-----
> > > > From: Maxime Coquelin [mailto:address@hidden
> > > > Sent: Thursday, March 29, 2018 3:28 AM
> > > > To: address@hidden; Liu, Changpeng <address@hidden>;
> > > > address@hidden; address@hidden
> > > > Cc: Maxime Coquelin <address@hidden>
> > > > Subject: [PATCH v2 2/2] vhost-user: back SET/GET_CONFIG requests with a
> > > > protocol feature
> > > >
> > > > Without a dedicated protocol feature, QEMU cannot know whether
> > > > the backend can handle VHOST_USER_SET_CONFIG and
> > > > VHOST_USER_GET_CONFIG messages.
> > > >
> > > > This patch adds a protocol feature that is only advertised by
> > > > QEMU if the device implements the config ops. Vhost user init
> > > > fails if the device support the feature but the backend doesn't.
> > > >
> > > > The backend should only send VHOST_USER_SLAVE_CONFIG_CHANGE_MSG
> > > > requests if the protocol feature has been negotiated.
> > > >
> > > > Signed-off-by: Maxime Coquelin <address@hidden>
> > >
> > > Passed our own vhost-user-blk target with the patch, I can submit a fix to
> > QEMU vhost-user-blk example
> > > after this commit.
> > >
> > > Tested-by: Changpeng Liu <address@hidden>
> >
> > Thanks for having tested, and for proposing to implement the example
> > part. Just notice I forgot to apply your Tested-by on the series.
> I have added the fix to the example based on your patch, I'm wondering
> I can send it out for review now or wait for your commit being merged ?

I think you can post, and add a note in comments that it depends on my
series.

Thanks,
Maxime

> > Maxime
> > > > ---
> > > >    docs/interop/vhost-user.txt | 21 ++++++++++++---------
> > > >    hw/virtio/vhost-user.c      | 22 ++++++++++++++++++++++
> > > >    2 files changed, 34 insertions(+), 9 deletions(-)
> > > >
> > > > diff --git a/docs/interop/vhost-user.txt b/docs/interop/vhost-user.txt
> > > > index c058c407df..534caab18a 100644
> > > > --- a/docs/interop/vhost-user.txt
> > > > +++ b/docs/interop/vhost-user.txt
> > > > @@ -379,6 +379,7 @@ Protocol features
> > > >    #define VHOST_USER_PROTOCOL_F_CROSS_ENDIAN   6
> > > >    #define VHOST_USER_PROTOCOL_F_CRYPTO_SESSION 7
> > > >    #define VHOST_USER_PROTOCOL_F_PAGEFAULT      8
> > > > +#define VHOST_USER_PROTOCOL_F_CONFIG         9
> > > >
> > > >    Master message types
> > > >    --------------------
> > > > @@ -664,7 +665,8 @@ Master message types
> > > >          Master payload: virtio device config space
> > > >          Slave payload: virtio device config space
> > > >
> > > > -      Submitted by the vhost-user master to fetch the contents of the virtio
> > > > +      When VHOST_USER_PROTOCOL_F_CONFIG is negotiated, this message is
> > > > +      submitted by the vhost-user master to fetch the contents of the virtio
> > > >          device configuration space, vhost-user slave's payload size MUST match
> > > >          master's request, vhost-user slave uses zero length of payload to
> > > >          indicate an error to vhost-user master. The vhost-user master may
> > > > @@ -677,7 +679,8 @@ Master message types
> > > >          Master payload: virtio device config space
> > > >          Slave payload: N/A
> > > >
> > > > -      Submitted by the vhost-user master when the Guest changes the virtio
> > > > +      When VHOST_USER_PROTOCOL_F_CONFIG is negotiated, this message is
> > > > +      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
> > > > @@ -766,13 +769,13 @@ Slave message types
> > > >         Slave payload: N/A
> > > >         Master payload: N/A
> > > >
> > > > -     Vhost-user slave sends such messages to notify that the virtio device's
> > > > -     configuration space has changed, for those host devices which can
> > support
> > > > -     such feature, host driver can send VHOST_USER_GET_CONFIG message to
> > > > slave
> > > > -     to get the latest content. If VHOST_USER_PROTOCOL_F_REPLY_ACK is
> > > > -     negotiated, and slave set the VHOST_USER_NEED_REPLY flag, master must
> > > > -     respond with zero when operation is successfully completed, or non-zero
> > > > -     otherwise.
> > > > +     When VHOST_USER_PROTOCOL_F_CONFIG is negotiated, vhost-user slave
> > > > sends
> > > > +     such messages to notify that the virtio device's configuration space has
> > > > +     changed, for those host devices which can support such feature, host
> > > > +     driver can send VHOST_USER_GET_CONFIG message to slave to get the
> > latest
> > > > +     content. If VHOST_USER_PROTOCOL_F_REPLY_ACK is negotiated, and
> > slave
> > > > set
> > > > +     the VHOST_USER_NEED_REPLY flag, master must respond with zero when
> > > > +     operation is successfully completed, or non-zero otherwise.
> > > >
> > > >    VHOST_USER_PROTOCOL_F_REPLY_ACK:
> > > >    -------------------------------
> > > > diff --git a/hw/virtio/vhost-user.c b/hw/virtio/vhost-user.c
> > > > index 44aea5c0a8..cc8a24aa31 100644
> > > > --- a/hw/virtio/vhost-user.c
> > > > +++ b/hw/virtio/vhost-user.c
> > > > @@ -46,6 +46,7 @@ enum VhostUserProtocolFeature {
> > > >        VHOST_USER_PROTOCOL_F_CROSS_ENDIAN = 6,
> > > >        VHOST_USER_PROTOCOL_F_CRYPTO_SESSION = 7,
> > > >        VHOST_USER_PROTOCOL_F_PAGEFAULT = 8,
> > > > +    VHOST_USER_PROTOCOL_F_CONFIG = 9,
> > > >        VHOST_USER_PROTOCOL_F_MAX
> > > >    };
> > > >
> > > > @@ -1211,6 +1212,17 @@ static int vhost_user_init(struct vhost_dev *dev,
> > void
> > > > *opaque)
> > > >
> > > >            dev->protocol_features =
> > > >                protocol_features & VHOST_USER_PROTOCOL_FEATURE_MASK;
> > > > +
> > > > +        if (!dev->config_ops || !dev->config_ops->vhost_dev_config_notifier) {
> > > > +            /* Dont acknowledge CONFIG feature if device doesn't support it */
> > > > +            dev->protocol_features &= ~(1ULL <<
> > > > VHOST_USER_PROTOCOL_F_CONFIG);
> > > > +        } else if (!(protocol_features &
> > > > +                    (1ULL << VHOST_USER_PROTOCOL_F_CONFIG))) {
> > > > +            error_report("Device expects VHOST_USER_PROTOCOL_F_CONFIG "
> > > > +                    "but backend does not support it.");
> > > > +            return -1;
> > > > +        }
> > > > +
> > > >            err = vhost_user_set_protocol_features(dev, dev->protocol_features);
> > > >            if (err < 0) {
> > > >                return err;
> > > > @@ -1405,6 +1417,11 @@ static int vhost_user_get_config(struct vhost_dev
> > > > *dev, uint8_t *config,
> > > >            .hdr.size = VHOST_USER_CONFIG_HDR_SIZE + config_len,
> > > >        };
> > > >
> > > > +    if (!virtio_has_feature(dev->protocol_features,
> > > > +                VHOST_USER_PROTOCOL_F_CONFIG)) {
> > > > +        return -1;
> > > > +    }
> > > > +
> > > >        if (config_len > VHOST_USER_MAX_CONFIG_SIZE) {
> > > >            return -1;
> > > >        }
> > > > @@ -1448,6 +1465,11 @@ static int vhost_user_set_config(struct vhost_dev
> > > > *dev, const uint8_t *data,
> > > >            .hdr.size = VHOST_USER_CONFIG_HDR_SIZE + size,
> > > >        };
> > > >
> > > > +    if (!virtio_has_feature(dev->protocol_features,
> > > > +                VHOST_USER_PROTOCOL_F_CONFIG)) {
> > > > +        return -1;
> > > > +    }
> > > > +
> > > >        if (reply_supported) {
> > > >            msg.hdr.flags |= VHOST_USER_NEED_REPLY_MASK;
> > > >        }
> > > > --
> > > > 2.14.3