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

[Michael S. Tsirkin]

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


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


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

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