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

[Maxime Coquelin <maxime.coquelin@redhat.com>]

On 03/29/2018 10:05 AM, Liu, Changpeng wrote:
> 
> 
>> -----Original Message-----
>> From: Maxime Coquelin [mailto:maxime.coquelin@redhat.com]
>> Sent: Thursday, March 29, 2018 3:56 PM
>> To: Liu, Changpeng <changpeng.liu@intel.com>; mst@redhat.com;
>> marcandre.lureau@redhat.com; qemu-devel@nongnu.org
>> 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:maxime.coquelin@redhat.com]
>>>> Sent: Thursday, March 29, 2018 3:28 AM
>>>> To: mst@redhat.com; Liu, Changpeng <changpeng.liu@intel.com>;
>>>> marcandre.lureau@redhat.com; qemu-devel@nongnu.org
>>>> Cc: Maxime Coquelin <maxime.coquelin@redhat.com>
>>>> 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 <maxime.coquelin@redhat.com>
>>>
>>> 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 <changpeng.liu@intel.com>
>>
>> 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
>>>