[Date Prev][Date Next][Thread Prev][Thread Next][Date Index][Thread Index]
Re: [Qemu-devel] [PATCH v2] Avoid additional GET_FEATURES call on vhost-
From: |
Michael S. Tsirkin |
Subject: |
Re: [Qemu-devel] [PATCH v2] Avoid additional GET_FEATURES call on vhost-user |
Date: |
Mon, 10 Oct 2016 21:20:06 +0300 |
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