qemu-devel
[Top][All Lists]
Advanced

[Date Prev][Date Next][Thread Prev][Thread Next][Date Index][Thread Index]

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


From: Marcel Apfelbaum
Subject: Re: [Qemu-devel] [PATCH] vhost-user: sync backend features
Date: Wed, 29 Jul 2015 21:20:47 +0300
User-agent: Mozilla/5.0 (X11; Linux x86_64; rv:38.0) Gecko/20100101 Thunderbird/38.1.0

On 07/29/2015 09:15 PM, Michael S. Tsirkin wrote:
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 <address@hidden>
---
  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?
vhost-user backend features are not enabled if are not negotiated, for example 
virtio 1.0 .

Maybe write a unit test patch that'll make unit test fail?
I'll have a look to the current unit tests to see if we can load a guest virtio 
driver
and check what features are negotiated. If we have something like that, it will 
not be a problem.

Thanks,
Marcel





  }

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




reply via email to

[Prev in Thread] Current Thread [Next in Thread]