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

[Felipe Franciosi]

> 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?
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?
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

> 
>> ---
>> 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