vhost-user: don't ignore CTRL_VLAN feature

[Qemu-devel] [RFC] vhost-user: don't ignore CTRL_VLAN feature

Posted by Tiwei Bie 4 years, 10 months ago

The VIRTIO_NET_F_CTRL_VLAN feature requires the support of
vhost-user backend. But it will be advertised to guest driver
as long as it's enabled by users in QEMU, while it's not
supported by vhost-user backend. This patch fixes this issue.

Fixes: 72018d1e1917 ("vhost-user: ignore qemu-only features")
Cc: qemu-stable@nongnu.org

Signed-off-by: Tiwei Bie <tiwei.bie@intel.com>
---
It's not clear in the spec that, whether vlan filtering is
also best-effort:
https://github.com/oasis-tcs/virtio-spec/blob/37057052e7/content.tex#L3372

 hw/net/vhost_net.c | 2 ++
 1 file changed, 2 insertions(+)

diff --git a/hw/net/vhost_net.c b/hw/net/vhost_net.c
index a6b719035c..1444fc9230 100644
--- a/hw/net/vhost_net.c
+++ b/hw/net/vhost_net.c
@@ -75,6 +75,8 @@ static const int user_feature_bits[] = {
     VIRTIO_NET_F_MTU,
     VIRTIO_F_IOMMU_PLATFORM,
 
+    VIRTIO_NET_F_CTRL_VLAN,
+
     /* This bit implies RARP isn't sent by QEMU out of band */
     VIRTIO_NET_F_GUEST_ANNOUNCE,
 
-- 
2.17.1

Re: [Qemu-devel] [RFC] vhost-user: don't ignore CTRL_VLAN feature

Posted by Jason Wang 4 years, 10 months ago

On 2019/6/11 下午2:51, Tiwei Bie wrote:
> The VIRTIO_NET_F_CTRL_VLAN feature requires the support of
> vhost-user backend. But it will be advertised to guest driver
> as long as it's enabled by users in QEMU, while it's not
> supported by vhost-user backend. This patch fixes this issue.
>
> Fixes: 72018d1e1917 ("vhost-user: ignore qemu-only features")


My understanding is if may want to revert this patch.


> Cc: qemu-stable@nongnu.org
>
> Signed-off-by: Tiwei Bie <tiwei.bie@intel.com>
> ---
> It's not clear in the spec that, whether vlan filtering is
> also best-effort:
> https://github.com/oasis-tcs/virtio-spec/blob/37057052e7/content.tex#L3372


It should be a bug of the code, we should consider to implement ctrl 
command for vhost-user.

Thanks


>
>   hw/net/vhost_net.c | 2 ++
>   1 file changed, 2 insertions(+)
>
> diff --git a/hw/net/vhost_net.c b/hw/net/vhost_net.c
> index a6b719035c..1444fc9230 100644
> --- a/hw/net/vhost_net.c
> +++ b/hw/net/vhost_net.c
> @@ -75,6 +75,8 @@ static const int user_feature_bits[] = {
>       VIRTIO_NET_F_MTU,
>       VIRTIO_F_IOMMU_PLATFORM,
>   
> +    VIRTIO_NET_F_CTRL_VLAN,
> +
>       /* This bit implies RARP isn't sent by QEMU out of band */
>       VIRTIO_NET_F_GUEST_ANNOUNCE,
>

Re: [Qemu-devel] [RFC] vhost-user: don't ignore CTRL_VLAN feature

Posted by Tiwei Bie 4 years, 10 months ago

On Tue, Jun 11, 2019 at 03:22:47PM +0800, Jason Wang wrote:
> 
> On 2019/6/11 下午2:51, Tiwei Bie wrote:
> > The VIRTIO_NET_F_CTRL_VLAN feature requires the support of
> > vhost-user backend. But it will be advertised to guest driver
> > as long as it's enabled by users in QEMU, while it's not
> > supported by vhost-user backend. This patch fixes this issue.
> > 
> > Fixes: 72018d1e1917 ("vhost-user: ignore qemu-only features")
> 
> 
> My understanding is if may want to revert this patch.
> 
> 
> > Cc: qemu-stable@nongnu.org
> > 
> > Signed-off-by: Tiwei Bie <tiwei.bie@intel.com>
> > ---
> > It's not clear in the spec that, whether vlan filtering is
> > also best-effort:
> > https://github.com/oasis-tcs/virtio-spec/blob/37057052e7/content.tex#L3372
> 
> 
> It should be a bug of the code, we should consider to implement ctrl command
> for vhost-user.

Yeah, we need to pass these information to backends if we
want to make it possible to support it in vhost-user.


> 
> Thanks
> 
> 
> > 
> >   hw/net/vhost_net.c | 2 ++
> >   1 file changed, 2 insertions(+)
> > 
> > diff --git a/hw/net/vhost_net.c b/hw/net/vhost_net.c
> > index a6b719035c..1444fc9230 100644
> > --- a/hw/net/vhost_net.c
> > +++ b/hw/net/vhost_net.c
> > @@ -75,6 +75,8 @@ static const int user_feature_bits[] = {
> >       VIRTIO_NET_F_MTU,
> >       VIRTIO_F_IOMMU_PLATFORM,
> > +    VIRTIO_NET_F_CTRL_VLAN,
> > +
> >       /* This bit implies RARP isn't sent by QEMU out of band */
> >       VIRTIO_NET_F_GUEST_ANNOUNCE,

Re: [Qemu-devel] [RFC] vhost-user: don't ignore CTRL_VLAN feature

Posted by Michael S. Tsirkin 4 years, 10 months ago

On Tue, Jun 11, 2019 at 02:51:37PM +0800, Tiwei Bie wrote:
> The VIRTIO_NET_F_CTRL_VLAN feature requires the support of
> vhost-user backend. But it will be advertised to guest driver
> as long as it's enabled by users in QEMU, while it's not
> supported by vhost-user backend. This patch fixes this issue.

Fixes by making guest refuse to send vlan tags?
I agree it seems cleaner, but which guests does this actually help?

> Fixes: 72018d1e1917 ("vhost-user: ignore qemu-only features")
> Cc: qemu-stable@nongnu.org
> 
> Signed-off-by: Tiwei Bie <tiwei.bie@intel.com>

A change like that will break migration compatibility, will it not?
Maybe we need to tie it to a machine version somehow...



> ---
> It's not clear in the spec that, whether vlan filtering is
> also best-effort:
> https://github.com/oasis-tcs/virtio-spec/blob/37057052e7/content.tex#L3372

So what breaks if we declare it best effort for now?
And does it really help if we report that vlan filtering
is not supported to guests?



>  hw/net/vhost_net.c | 2 ++
>  1 file changed, 2 insertions(+)
> 
> diff --git a/hw/net/vhost_net.c b/hw/net/vhost_net.c
> index a6b719035c..1444fc9230 100644
> --- a/hw/net/vhost_net.c
> +++ b/hw/net/vhost_net.c
> @@ -75,6 +75,8 @@ static const int user_feature_bits[] = {
>      VIRTIO_NET_F_MTU,
>      VIRTIO_F_IOMMU_PLATFORM,
>  
> +    VIRTIO_NET_F_CTRL_VLAN,
> +
>      /* This bit implies RARP isn't sent by QEMU out of band */
>      VIRTIO_NET_F_GUEST_ANNOUNCE,
>  
> -- 
> 2.17.1

Re: [Qemu-devel] [RFC] vhost-user: don't ignore CTRL_VLAN feature

Posted by Tiwei Bie 4 years, 10 months ago

On Tue, Jun 11, 2019 at 10:10:14AM -0400, Michael S. Tsirkin wrote:
> On Tue, Jun 11, 2019 at 02:51:37PM +0800, Tiwei Bie wrote:
> > The VIRTIO_NET_F_CTRL_VLAN feature requires the support of
> > vhost-user backend. But it will be advertised to guest driver
> > as long as it's enabled by users in QEMU, while it's not
> > supported by vhost-user backend. This patch fixes this issue.
> 
> Fixes by making guest refuse to send vlan tags?

Fixes by not advertising this feature bit to guest driver
when it's not supported, and guest won't expect the device
to do vlan filtering then.

> I agree it seems cleaner, but which guests does this actually help?
> 
> > Fixes: 72018d1e1917 ("vhost-user: ignore qemu-only features")
> > Cc: qemu-stable@nongnu.org
> > 
> > Signed-off-by: Tiwei Bie <tiwei.bie@intel.com>
> 
> A change like that will break migration compatibility, will it not?

Yeah, that's a problem...

> Maybe we need to tie it to a machine version somehow...
> 
> 
> > ---
> > It's not clear in the spec that, whether vlan filtering is
> > also best-effort:
> > https://github.com/oasis-tcs/virtio-spec/blob/37057052e7/content.tex#L3372
> 
> So what breaks if we declare it best effort for now?
> And does it really help if we report that vlan filtering
> is not supported to guests?

If it's best effort, then it won't violate the spec to
advertise this feature when it's not supported in backends.


> 
> 
> 
> >  hw/net/vhost_net.c | 2 ++
> >  1 file changed, 2 insertions(+)
> > 
> > diff --git a/hw/net/vhost_net.c b/hw/net/vhost_net.c
> > index a6b719035c..1444fc9230 100644
> > --- a/hw/net/vhost_net.c
> > +++ b/hw/net/vhost_net.c
> > @@ -75,6 +75,8 @@ static const int user_feature_bits[] = {
> >      VIRTIO_NET_F_MTU,
> >      VIRTIO_F_IOMMU_PLATFORM,
> >  
> > +    VIRTIO_NET_F_CTRL_VLAN,
> > +
> >      /* This bit implies RARP isn't sent by QEMU out of band */
> >      VIRTIO_NET_F_GUEST_ANNOUNCE,
> >  
> > -- 
> > 2.17.1

Re: [Qemu-devel] [RFC] vhost-user: don't ignore CTRL_VLAN feature

Posted by Michael S. Tsirkin 4 years, 9 months ago

On Wed, Jun 12, 2019 at 10:11:57AM +0800, Tiwei Bie wrote:
> On Tue, Jun 11, 2019 at 10:10:14AM -0400, Michael S. Tsirkin wrote:
> > On Tue, Jun 11, 2019 at 02:51:37PM +0800, Tiwei Bie wrote:
> > > The VIRTIO_NET_F_CTRL_VLAN feature requires the support of
> > > vhost-user backend. But it will be advertised to guest driver
> > > as long as it's enabled by users in QEMU, while it's not
> > > supported by vhost-user backend. This patch fixes this issue.
> > 
> > Fixes by making guest refuse to send vlan tags?
> 
> Fixes by not advertising this feature bit to guest driver
> when it's not supported, and guest won't expect the device
> to do vlan filtering then.
> 
> > I agree it seems cleaner, but which guests does this actually help?
> > 
> > > Fixes: 72018d1e1917 ("vhost-user: ignore qemu-only features")
> > > Cc: qemu-stable@nongnu.org
> > > 
> > > Signed-off-by: Tiwei Bie <tiwei.bie@intel.com>
> > 
> > A change like that will break migration compatibility, will it not?
> 
> Yeah, that's a problem...
> 
> > Maybe we need to tie it to a machine version somehow...
> > 
> > 
> > > ---
> > > It's not clear in the spec that, whether vlan filtering is
> > > also best-effort:
> > > https://github.com/oasis-tcs/virtio-spec/blob/37057052e7/content.tex#L3372
> > 
> > So what breaks if we declare it best effort for now?
> > And does it really help if we report that vlan filtering
> > is not supported to guests?
> 
> If it's best effort, then it won't violate the spec to
> advertise this feature when it's not supported in backends.
> 

OK so it's up to you. Want to propose a spec patch?
We already say mac filter is best effort ...

> > 
> > 
> > 
> > >  hw/net/vhost_net.c | 2 ++
> > >  1 file changed, 2 insertions(+)
> > > 
> > > diff --git a/hw/net/vhost_net.c b/hw/net/vhost_net.c
> > > index a6b719035c..1444fc9230 100644
> > > --- a/hw/net/vhost_net.c
> > > +++ b/hw/net/vhost_net.c
> > > @@ -75,6 +75,8 @@ static const int user_feature_bits[] = {
> > >      VIRTIO_NET_F_MTU,
> > >      VIRTIO_F_IOMMU_PLATFORM,
> > >  
> > > +    VIRTIO_NET_F_CTRL_VLAN,
> > > +
> > >      /* This bit implies RARP isn't sent by QEMU out of band */
> > >      VIRTIO_NET_F_GUEST_ANNOUNCE,
> > >  
> > > -- 
> > > 2.17.1

Re: [Qemu-devel] [RFC] vhost-user: don't ignore CTRL_VLAN feature

Posted by Jason Wang 4 years, 9 months ago

On 2019/7/17 下午7:00, Michael S. Tsirkin wrote:
> On Wed, Jun 12, 2019 at 10:11:57AM +0800, Tiwei Bie wrote:
>> On Tue, Jun 11, 2019 at 10:10:14AM -0400, Michael S. Tsirkin wrote:
>>> On Tue, Jun 11, 2019 at 02:51:37PM +0800, Tiwei Bie wrote:
>>>> The VIRTIO_NET_F_CTRL_VLAN feature requires the support of
>>>> vhost-user backend. But it will be advertised to guest driver
>>>> as long as it's enabled by users in QEMU, while it's not
>>>> supported by vhost-user backend. This patch fixes this issue.
>>> Fixes by making guest refuse to send vlan tags?
>> Fixes by not advertising this feature bit to guest driver
>> when it's not supported, and guest won't expect the device
>> to do vlan filtering then.
>>
>>> I agree it seems cleaner, but which guests does this actually help?
>>>
>>>> Fixes: 72018d1e1917 ("vhost-user: ignore qemu-only features")
>>>> Cc: qemu-stable@nongnu.org
>>>>
>>>> Signed-off-by: Tiwei Bie <tiwei.bie@intel.com>
>>> A change like that will break migration compatibility, will it not?
>> Yeah, that's a problem...
>>
>>> Maybe we need to tie it to a machine version somehow...
>>>
>>>
>>>> ---
>>>> It's not clear in the spec that, whether vlan filtering is
>>>> also best-effort:
>>>> https://github.com/oasis-tcs/virtio-spec/blob/37057052e7/content.tex#L3372
>>> So what breaks if we declare it best effort for now?
>>> And does it really help if we report that vlan filtering
>>> is not supported to guests?
>> If it's best effort, then it won't violate the spec to
>> advertise this feature when it's not supported in backends.
>>
> OK so it's up to you. Want to propose a spec patch?
> We already say mac filter is best effort ...


This probably need more thought. Consider in the future, we want to have 
filters that can steering packet based on its mac address to a specific 
queue. Then we can add e.g macvlan offloading support. Saying "best 
effort" might be wrong in that cases.


Thanks


>>>
>>>
>>>>   hw/net/vhost_net.c | 2 ++
>>>>   1 file changed, 2 insertions(+)
>>>>
>>>> diff --git a/hw/net/vhost_net.c b/hw/net/vhost_net.c
>>>> index a6b719035c..1444fc9230 100644
>>>> --- a/hw/net/vhost_net.c
>>>> +++ b/hw/net/vhost_net.c
>>>> @@ -75,6 +75,8 @@ static const int user_feature_bits[] = {
>>>>       VIRTIO_NET_F_MTU,
>>>>       VIRTIO_F_IOMMU_PLATFORM,
>>>>   
>>>> +    VIRTIO_NET_F_CTRL_VLAN,
>>>> +
>>>>       /* This bit implies RARP isn't sent by QEMU out of band */
>>>>       VIRTIO_NET_F_GUEST_ANNOUNCE,
>>>>   
>>>> -- 
>>>> 2.17.1

Re: [Qemu-devel] [RFC] vhost-user: don't ignore CTRL_VLAN feature

Posted by Michael S. Tsirkin 4 years, 9 months ago

On Wed, Jul 17, 2019 at 08:30:29PM +0800, Jason Wang wrote:
> 
> On 2019/7/17 下午7:00, Michael S. Tsirkin wrote:
> > On Wed, Jun 12, 2019 at 10:11:57AM +0800, Tiwei Bie wrote:
> > > On Tue, Jun 11, 2019 at 10:10:14AM -0400, Michael S. Tsirkin wrote:
> > > > On Tue, Jun 11, 2019 at 02:51:37PM +0800, Tiwei Bie wrote:
> > > > > The VIRTIO_NET_F_CTRL_VLAN feature requires the support of
> > > > > vhost-user backend. But it will be advertised to guest driver
> > > > > as long as it's enabled by users in QEMU, while it's not
> > > > > supported by vhost-user backend. This patch fixes this issue.
> > > > Fixes by making guest refuse to send vlan tags?
> > > Fixes by not advertising this feature bit to guest driver
> > > when it's not supported, and guest won't expect the device
> > > to do vlan filtering then.
> > > 
> > > > I agree it seems cleaner, but which guests does this actually help?
> > > > 
> > > > > Fixes: 72018d1e1917 ("vhost-user: ignore qemu-only features")
> > > > > Cc: qemu-stable@nongnu.org
> > > > > 
> > > > > Signed-off-by: Tiwei Bie <tiwei.bie@intel.com>
> > > > A change like that will break migration compatibility, will it not?
> > > Yeah, that's a problem...
> > > 
> > > > Maybe we need to tie it to a machine version somehow...
> > > > 
> > > > 
> > > > > ---
> > > > > It's not clear in the spec that, whether vlan filtering is
> > > > > also best-effort:
> > > > > https://github.com/oasis-tcs/virtio-spec/blob/37057052e7/content.tex#L3372
> > > > So what breaks if we declare it best effort for now?
> > > > And does it really help if we report that vlan filtering
> > > > is not supported to guests?
> > > If it's best effort, then it won't violate the spec to
> > > advertise this feature when it's not supported in backends.
> > > 
> > OK so it's up to you. Want to propose a spec patch?
> > We already say mac filter is best effort ...
> 
> 
> This probably need more thought. Consider in the future, we want to have
> filters that can steering packet based on its mac address to a specific
> queue. Then we can add e.g macvlan offloading support. Saying "best effort"
> might be wrong in that cases.
> 
> 
> Thanks
> 


We can always add a feature bit. Spec should reflect the way
implementations behave now.

> > > > 
> > > > 
> > > > >   hw/net/vhost_net.c | 2 ++
> > > > >   1 file changed, 2 insertions(+)
> > > > > 
> > > > > diff --git a/hw/net/vhost_net.c b/hw/net/vhost_net.c
> > > > > index a6b719035c..1444fc9230 100644
> > > > > --- a/hw/net/vhost_net.c
> > > > > +++ b/hw/net/vhost_net.c
> > > > > @@ -75,6 +75,8 @@ static const int user_feature_bits[] = {
> > > > >       VIRTIO_NET_F_MTU,
> > > > >       VIRTIO_F_IOMMU_PLATFORM,
> > > > > +    VIRTIO_NET_F_CTRL_VLAN,
> > > > > +
> > > > >       /* This bit implies RARP isn't sent by QEMU out of band */
> > > > >       VIRTIO_NET_F_GUEST_ANNOUNCE,
> > > > > -- 
> > > > > 2.17.1

Re: [Qemu-devel] [RFC] vhost-user: don't ignore CTRL_VLAN feature

Posted by Tiwei Bie 4 years, 9 months ago

On Wed, Jul 17, 2019 at 07:00:25AM -0400, Michael S. Tsirkin wrote:
> On Wed, Jun 12, 2019 at 10:11:57AM +0800, Tiwei Bie wrote:
> > On Tue, Jun 11, 2019 at 10:10:14AM -0400, Michael S. Tsirkin wrote:
> > > On Tue, Jun 11, 2019 at 02:51:37PM +0800, Tiwei Bie wrote:
> > > > The VIRTIO_NET_F_CTRL_VLAN feature requires the support of
> > > > vhost-user backend. But it will be advertised to guest driver
> > > > as long as it's enabled by users in QEMU, while it's not
> > > > supported by vhost-user backend. This patch fixes this issue.
> > > 
> > > Fixes by making guest refuse to send vlan tags?
> > 
> > Fixes by not advertising this feature bit to guest driver
> > when it's not supported, and guest won't expect the device
> > to do vlan filtering then.
> > 
> > > I agree it seems cleaner, but which guests does this actually help?
> > > 
> > > > Fixes: 72018d1e1917 ("vhost-user: ignore qemu-only features")
> > > > Cc: qemu-stable@nongnu.org
> > > > 
> > > > Signed-off-by: Tiwei Bie <tiwei.bie@intel.com>
> > > 
> > > A change like that will break migration compatibility, will it not?
> > 
> > Yeah, that's a problem...
> > 
> > > Maybe we need to tie it to a machine version somehow...
> > > 
> > > 
> > > > ---
> > > > It's not clear in the spec that, whether vlan filtering is
> > > > also best-effort:
> > > > https://github.com/oasis-tcs/virtio-spec/blob/37057052e7/content.tex#L3372
> > > 
> > > So what breaks if we declare it best effort for now?
> > > And does it really help if we report that vlan filtering
> > > is not supported to guests?
> > 
> > If it's best effort, then it won't violate the spec to
> > advertise this feature when it's not supported in backends.
> > 
> 
> OK so it's up to you. Want to propose a spec patch?
> We already say mac filter is best effort ...

Done, below is the link:
https://lists.oasis-open.org/archives/virtio-dev/201907/msg00035.html

> 
> > > 
> > > 
> > > 
> > > >  hw/net/vhost_net.c | 2 ++
> > > >  1 file changed, 2 insertions(+)
> > > > 
> > > > diff --git a/hw/net/vhost_net.c b/hw/net/vhost_net.c
> > > > index a6b719035c..1444fc9230 100644
> > > > --- a/hw/net/vhost_net.c
> > > > +++ b/hw/net/vhost_net.c
> > > > @@ -75,6 +75,8 @@ static const int user_feature_bits[] = {
> > > >      VIRTIO_NET_F_MTU,
> > > >      VIRTIO_F_IOMMU_PLATFORM,
> > > >  
> > > > +    VIRTIO_NET_F_CTRL_VLAN,
> > > > +
> > > >      /* This bit implies RARP isn't sent by QEMU out of band */
> > > >      VIRTIO_NET_F_GUEST_ANNOUNCE,
> > > >  
> > > > -- 
> > > > 2.17.1