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

[Michael S. Tsirkin]

On Mon, Oct 10, 2016 at 03:31:05PM +0000, Felipe Franciosi wrote:
> 
> > 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.

Yes, but that's the cost of compatibility unfortunately.
If you don't care about that, you don't have to implement it.

> 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