| Message ID | 20180328155657.6434-3-maxime.coquelin@redhat.com (mailing list archive) |
|---|---|
| State | New, archived |
| Headers | show |
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.coquelin@redhat.com> I presume vhost user blk should fail init if the protocol feature isn't negotiated then. > --- > 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
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.coquelin@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. > >> --- >> 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
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.coquelin@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
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; }
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.coquelin@redhat.com> --- docs/interop/vhost-user.txt | 21 ++++++++++++--------- hw/virtio/vhost-user.c | 17 +++++++++++++++++ 2 files changed, 29 insertions(+), 9 deletions(-)