[Date Prev][Date Next][Thread Prev][Thread Next][Date Index][Thread Index]
Re: [Qemu-devel] [PATCH v2 2/2] vhost-user: back SET/GET_CONFIG requests
From: |
Liu, Changpeng |
Subject: |
Re: [Qemu-devel] [PATCH v2 2/2] vhost-user: back SET/GET_CONFIG requests with a protocol feature |
Date: |
Thu, 29 Mar 2018 08:05:33 +0000 |
> -----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 ?
>
> 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
> >