| Message ID | 1438189918-2087-1-git-send-email-marcel@redhat.com |
|---|---|
| State | New |
| Headers | show |
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? > } > > 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
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 > >> } >> >> 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
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. > > > > >> } > >> > >>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
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. Anyway, I am open to suggestions. 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
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
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 <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? 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
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); } 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;
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(-)