Re: [Qemu-devel] [PATCH] vhost-user: sync backend features

["Michael S. Tsirkin" <mst@redhat.com>]

On Wed, Jul 29, 2015 at 09:11:18PM +0300, Marcel Apfelbaum wrote:
> On 07/29/2015 09:05 PM, Michael S. Tsirkin wrote:
> >On Wed, Jul 29, 2015 at 08:43:47PM +0300, Marcel Apfelbaum wrote:
> >>On 07/29/2015 08:20 PM, Michael S. Tsirkin wrote:
> >>>On Wed, Jul 29, 2015 at 08:11:58PM +0300, Marcel Apfelbaum wrote:
> >>>>Complete vhost-user negotiation by syncing the features
> >>>>supported by the backend.
> >>>>
> >>>>Signed-off-by: Marcel Apfelbaum <marcel@redhat.com>
> >>>>---
> >>>>  To be used on top of:
> >>>>  [PATCH 0/4] vhost-user: protocol updates
> >>>>  https://lists.gnu.org/archive/html/qemu-devel/2015-07/msg03842.html
> >>>>
> >>>>  Currently the vhost-user supported features are not evaluated.
> >>>>  The way I see it, and please correct me, the best way to do
> >>>>  this is to:
> >>>>   1. get the backend features on vhost init
> >>>>   2. Instead of simply copying them during features ack,
> >>>>      check that that all backend features are supported by current QEMU
> >>>>   3. All other code should remain the same.
> >>>>
> >>>>   Thanks,
> >>>>   Marcel
> >>>>
> >>>>  hw/net/vhost_net.c     | 3 ++-
> >>>>  hw/virtio/vhost-user.c | 4 ++--
> >>>>  2 files changed, 4 insertions(+), 3 deletions(-)
> >>>>
> >>>>diff --git a/hw/net/vhost_net.c b/hw/net/vhost_net.c
> >>>>index c864237..1ea5866 100644
> >>>>--- a/hw/net/vhost_net.c
> >>>>+++ b/hw/net/vhost_net.c
> >>>>@@ -118,7 +118,8 @@ uint64_t vhost_net_get_features(struct vhost_net *net, uint64_t features)
> >>>>
> >>>>  void vhost_net_ack_features(struct vhost_net *net, uint64_t features)
> >>>>  {
> >>>>-    net->dev.acked_features = net->dev.backend_features;
> >>>>+    vhost_ack_features(&net->dev, vhost_net_get_feature_bits(net),
> >>>>+                       net->dev.backend_features);
> >>>>      vhost_ack_features(&net->dev, vhost_net_get_feature_bits(net), features);
> >>>
> >>>So you ack it twice?
> >>Not really, first call to vhost_ack_features acks the net->dev.backend_features,
> >>the second one acks the guest features.
> >>
> >>The first call replaces the previous assignment that assumes that all backend features are
> >>supported by QEMU.
> >>
> >>Thanks,
> >>Marcel
> >
> >I think it's cleaner to whitelist backend features QEMU supports.
> >I thought we did this, will look again.
> I thought to do it the same as you started in vhost-init:
> 
>   if (__virtio_has_feature(msg.u64, VHOST_USER_F_PROTOCOL_FEATURES)) {
>        dev->backend_features |= 1ULL << VHOST_USER_F_PROTOCOL_FEATURES;
> 
> However, we should check all possible flags, no? We have about 16 now.
> And we'll need to manually add to white-list every new feature, seems error prone.

But more secure :)

> Anyway, I am open to suggestions.
> 
> Thanks,
> Marcel

Can you describe the problem you are solving?
Maybe write a unit test patch that'll make unit test fail?

> 
> >>
> >>>
> >>>>  }
> >>>>
> >>>>diff --git a/hw/virtio/vhost-user.c b/hw/virtio/vhost-user.c
> >>>>index c4428a1..077457b 100644
> >>>>--- a/hw/virtio/vhost-user.c
> >>>>+++ b/hw/virtio/vhost-user.c
> >>>>@@ -358,9 +358,9 @@ static int vhost_user_init(struct vhost_dev *dev, void *opaque)
> >>>>          return err;
> >>>>      }
> >>>>
> >>>>-    if (__virtio_has_feature(msg.u64, VHOST_USER_F_PROTOCOL_FEATURES)) {
> >>>>-        dev->backend_features |= 1ULL << VHOST_USER_F_PROTOCOL_FEATURES;
> >>>>+    dev->backend_features = msg.u64;
> >>>>
> >>>>+    if (__virtio_has_feature(msg.u64, VHOST_USER_F_PROTOCOL_FEATURES)) {
> >>>>          msg.request = VHOST_USER_GET_PROTOCOL_FEATURES;
> >>>>          msg.flags = VHOST_USER_VERSION;
> >>>>          msg.size = 0;
> >>>>--
> >>>>2.1.0