On Wed, Mar 28, 2018 at 07:08:32PM +0200, Maxime Coquelin wrote:
>
>
> On 03/28/2018 06:55 PM, Michael S. Tsirkin wrote:
> > On Wed, Mar 28, 2018 at 05:56:57PM +0200, Maxime Coquelin wrote:
> > > 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. The backend
> > > should only send VHOST_USER_SLAVE_CONFIG_CHANGE_MSG requests
> > > if the protocol feature has been negotiated.
> > >
> > > Signed-off-by: Maxime Coquelin <maxime.coque...@redhat.com>
> >
> > I presume vhost user blk should fail init if the
> > protocol feature isn't negotiated then.
>
> I did that and finally removed it.
> In the future, if for example we add config support for net device,
> we will want init to succeed even with old backend version that
> does not support it, right?
>
> For the vhost user blk case, its init will fail right after,
> because it tries to get config, but will get an error instead.
>
> As we only have vhost-user-blk supporting it for now, and since it
> is a mandatory feature, I fine to post a v2 that makes
> vhost_user_init() to fail.
Seems safer. We can remove restrictions but not add new ones.
> >
> > > ---
> > > docs/interop/vhost-user.txt | 21 ++++++++++++---------
> > > hw/virtio/vhost-user.c | 17 +++++++++++++++++
> > > 2 files changed, 29 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..a045203b26 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,12 @@ 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);
> > > + }
> > > +
> > > err = vhost_user_set_protocol_features(dev,
> > > dev->protocol_features);
> > > if (err < 0) {
> > > return err;
> > > @@ -1405,6 +1412,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 +1460,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
