Re: [Qemu-devel] [PATCH v2] Avoid additional GET_FEATURES call on vhost-user

[Felipe Franciosi]

> On 10 Oct 2016, at 16:24, Michael S. Tsirkin <address@hidden> wrote:
> 
> On Mon, Oct 10, 2016 at 03:17:36PM +0000, Felipe Franciosi wrote:
>> 
>>> On 9 Oct 2016, at 23:33, Michael S. Tsirkin <address@hidden> wrote:
>>> 
>>> On Thu, Sep 22, 2016 at 01:13:41PM +0100, Felipe Franciosi wrote:
>>>> Vhost-user requires an early GET_FEATURES call to determine if the
>>>> slave supports protocol feature negotiation. An extra GET_FEATURES
>>>> call is made after vhost_backend_init() to actually set the device
>>>> features.
>>>> 
>>>> This patch moves the actual setting of the device features to both
>>>> implementations (kernel/user) of vhost_backend_init(), therefore
>>>> eliminating the need for a second call.
>>>> 
>>>> Signed-off-by: Felipe Franciosi <address@hidden>
>>> 
>>> 
>>> Thanks!
>>> Thinking about this, I think it makes sense to keep
>>> both messages around.
>>> This allow backend to change the feature bitmap
>>> depending on the protocol features negotiated.
>> 
>> Hi Michael,
>> 
>> Sounds interesting, but I'm struggling a bit to see how that would work. 
>> IIUC, the "feature" negotiation should relate to anything between device and 
>> driver. The "protocol feature" should relate to things between backend and 
>> Qemu. The obvious exception is the "protocol feature bit" itself, which is 
>> part of the "feature" since protocol features weren't part of the original 
>> spec.
>> 
>> If the protocol or the implementation have deviated from that, we should try 
>> to make it consistent and clear to avoid further problems.
>> 
>> So on the double message, I guess my questions would be:
>> 1) What features could a backend advertise differently and how that relates 
>> to protocol features? In other words, how would a protocol feature affect 
>> the actual relationship between the backend and the driver?
> 
> I have some vague ideas, e.g. dealing with migration. I'll Cc you when I do a 
> writeup (RSN).

Awesome. Look forward to it... I always had the feeling that all the stuff 
related with migration should really be on a "set protocol feature" message 
(and not "set feature"), for the reasons outlined above (it's really about 
Qemu<->Backend and not Driver<->Backend).

> 
> 
>> 2) What guarantees would a backend have that Qemu is going to ask again for 
>> the feature bitmap? In other words, if a backend is going to implement 
>> something differently (regarding handling the driver) depending on what 
>> protocol bits are supported by Qemu, can it count on Qemu asking for it 
>> again?
> 
> There would be a protocol feature for this. If it's not acked by qemu,
> it must assume it's done once.

But wouldn't that be too late in the negotiation? The first thing Qemu needs to 
do is ask for supported features (to determine whether protocol features are 
supported). Meaning, any backend that wants to do change their exposed features 
depending on what protocol features are supported will have to implement it 
both ways (in case the required protocol features aren't there). Sounds awfully 
complicated from a backend point of view. But perhaps it will make sense 
depending on what you have in mind for (1) above. Will ping you again on this 
when the text comes.

> 
> 
>> 3) If we stick to the double message, should we document it as intentional 
>> and exemplify how it could be used to avoid someone else spotting this and 
>> trying to "fix" it again?
>> 
>> Thanks,
>> Felipe
> 
> We can do that when we land such a feature.

Sure makes sense.

Thanks!
Felipe

> 
>>> 
>>>> ---
>>>> v2. Rebase on d1b4259f
>>>> 
>>>> hw/virtio/vhost-backend.c | 27 ++++++++++++++++++---------
>>>> hw/virtio/vhost-user.c    |  1 +
>>>> hw/virtio/vhost.c         |  9 ---------
>>>> 3 files changed, 19 insertions(+), 18 deletions(-)
>>>> 
>>>> diff --git a/hw/virtio/vhost-backend.c b/hw/virtio/vhost-backend.c
>>>> index 272a5ec..0ffa4d1 100644
>>>> --- a/hw/virtio/vhost-backend.c
>>>> +++ b/hw/virtio/vhost-backend.c
>>>> @@ -25,15 +25,6 @@ static int vhost_kernel_call(struct vhost_dev *dev, 
>>>> unsigned long int request,
>>>>    return ioctl(fd, request, arg);
>>>> }
>>>> 
>>>> -static int vhost_kernel_init(struct vhost_dev *dev, void *opaque)
>>>> -{
>>>> -    assert(dev->vhost_ops->backend_type == VHOST_BACKEND_TYPE_KERNEL);
>>>> -
>>>> -    dev->opaque = opaque;
>>>> -
>>>> -    return 0;
>>>> -}
>>>> -
>>>> static int vhost_kernel_cleanup(struct vhost_dev *dev)
>>>> {
>>>>    int fd = (uintptr_t) dev->opaque;
>>>> @@ -185,6 +176,24 @@ static int vhost_kernel_vsock_set_running(struct 
>>>> vhost_dev *dev, int start)
>>>> }
>>>> #endif /* CONFIG_VHOST_VSOCK */
>>>> 
>>>> +static int vhost_kernel_init(struct vhost_dev *dev, void *opaque)
>>>> +{
>>>> +    uint64_t features;
>>>> +    int r;
>>>> +
>>>> +    assert(dev->vhost_ops->backend_type == VHOST_BACKEND_TYPE_KERNEL);
>>>> +
>>>> +    dev->opaque = opaque;
>>>> +
>>>> +    r = vhost_kernel_get_features(dev, &features);
>>>> +    if (r < 0) {
>>>> +        return r;
>>>> +    }
>>>> +    dev->features = features;
>>>> +
>>>> +    return 0;
>>>> +}
>>>> +
>>>> static const VhostOps kernel_ops = {
>>>>        .backend_type = VHOST_BACKEND_TYPE_KERNEL,
>>>>        .vhost_backend_init = vhost_kernel_init,
>>>> diff --git a/hw/virtio/vhost-user.c b/hw/virtio/vhost-user.c
>>>> index b57454a..684f6d7 100644
>>>> --- a/hw/virtio/vhost-user.c
>>>> +++ b/hw/virtio/vhost-user.c
>>>> @@ -578,6 +578,7 @@ static int vhost_user_init(struct vhost_dev *dev, void 
>>>> *opaque)
>>>>    if (err < 0) {
>>>>        return err;
>>>>    }
>>>> +    dev->features = features;
>>>> 
>>>>    if (virtio_has_feature(features, VHOST_USER_F_PROTOCOL_FEATURES)) {
>>>>        dev->backend_features |= 1ULL << VHOST_USER_F_PROTOCOL_FEATURES;
>>>> diff --git a/hw/virtio/vhost.c b/hw/virtio/vhost.c
>>>> index bd051ab..36fd35f 100644
>>>> --- a/hw/virtio/vhost.c
>>>> +++ b/hw/virtio/vhost.c
>>>> @@ -1051,7 +1051,6 @@ static void vhost_virtqueue_cleanup(struct 
>>>> vhost_virtqueue *vq)
>>>> int vhost_dev_init(struct vhost_dev *hdev, void *opaque,
>>>>                   VhostBackendType backend_type, uint32_t busyloop_timeout)
>>>> {
>>>> -    uint64_t features;
>>>>    int i, r, n_initialized_vqs = 0;
>>>> 
>>>>    hdev->migration_blocker = NULL;
>>>> @@ -1077,12 +1076,6 @@ int vhost_dev_init(struct vhost_dev *hdev, void 
>>>> *opaque,
>>>>        goto fail;
>>>>    }
>>>> 
>>>> -    r = hdev->vhost_ops->vhost_get_features(hdev, &features);
>>>> -    if (r < 0) {
>>>> -        VHOST_OPS_DEBUG("vhost_get_features failed");
>>>> -        goto fail;
>>>> -    }
>>>> -
>>>>    for (i = 0; i < hdev->nvqs; ++i, ++n_initialized_vqs) {
>>>>        r = vhost_virtqueue_init(hdev, hdev->vqs + i, hdev->vq_index + i);
>>>>        if (r < 0) {
>>>> @@ -1100,8 +1093,6 @@ int vhost_dev_init(struct vhost_dev *hdev, void 
>>>> *opaque,
>>>>        }
>>>>    }
>>>> 
>>>> -    hdev->features = features;
>>>> -
>>>>    hdev->memory_listener = (MemoryListener) {
>>>>        .begin = vhost_begin,
>>>>        .commit = vhost_commit,
>>>> -- 
>>>> 1.9.5