| Message ID | 1474546421-8613-1-git-send-email-felipe@nutanix.com (mailing list archive) |
|---|---|
| State | New, archived |
| Headers | show |
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 <felipe@nutanix.com> Maxime, you changes things around feature negotiation recently, mind reviewing this? > --- > 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
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 <felipe@nutanix.com> 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. > --- > 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
> On 9 Oct 2016, at 23:33, Michael S. Tsirkin <mst@redhat.com> 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 <felipe@nutanix.com> > > > 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
On Mon, Oct 10, 2016 at 03:17:36PM +0000, Felipe Franciosi wrote: > > > On 9 Oct 2016, at 23:33, Michael S. Tsirkin <mst@redhat.com> 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 <felipe@nutanix.com> > > > > > > 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
> On 10 Oct 2016, at 16:24, Michael S. Tsirkin <mst@redhat.com> wrote: > > On Mon, Oct 10, 2016 at 03:17:36PM +0000, Felipe Franciosi wrote: >> >>> On 9 Oct 2016, at 23:33, Michael S. Tsirkin <mst@redhat.com> 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 <felipe@nutanix.com> >>> >>> >>> 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
On Mon, Oct 10, 2016 at 03:31:05PM +0000, Felipe Franciosi wrote: > > > On 10 Oct 2016, at 16:24, Michael S. Tsirkin <mst@redhat.com> wrote: > > > > On Mon, Oct 10, 2016 at 03:17:36PM +0000, Felipe Franciosi wrote: > >> > >>> On 9 Oct 2016, at 23:33, Michael S. Tsirkin <mst@redhat.com> 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 <felipe@nutanix.com> > >>> > >>> > >>> 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
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,
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 <felipe@nutanix.com> --- 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(-)