Re: [PATCH v2] virtio_net: CTRL_GUEST_OFFLOADS depends on CTRL_VQ
On Thu, Jan 09, 2020 at 06:33:39PM -0800, David Miller wrote:
> From: "Michael S. Tsirkin"
> Date: Sun, 5 Jan 2020 08:22:07 -0500
>
> > The only way for guest to control offloads (as enabled by
> > VIRTIO_NET_F_CTRL_GUEST_OFFLOADS) is by sending commands
> > through CTRL_VQ. So it does not make sense to
> > acknowledge VIRTIO_NET_F_CTRL_GUEST_OFFLOADS without
> > VIRTIO_NET_F_CTRL_VQ.
> >
> > The spec does not outlaw devices with such a configuration, so we have
> > to support it. Simply clear VIRTIO_NET_F_CTRL_GUEST_OFFLOADS.
> > Note that Linux is still crashing if it tries to
> > change the offloads when there's no control vq.
> > That needs to be fixed by another patch.
> >
> > Reported-by: Alistair Delva
> > Reported-by: Willem de Bruijn
> > Fixes: 3f93522ffab2 ("virtio-net: switch off offloads on demand if possible
> > on XDP set")
> > Signed-off-by: Michael S. Tsirkin
> > ---
> >
> > Same patch as v1 but update documentation so it's clear it's not
> > enough to fix the crash.
>
> Where are we with this patch? There seems to still be some unresolved
> discussion about how we should actually handle this case.
>
> Thanks.
Once discussion is resolved I'll post v3 with updated commit log.
___
Virtualization mailing list
Virtualization@lists.linux-foundation.org
https://lists.linuxfoundation.org/mailman/listinfo/virtualization
Re: [PATCH v2] virtio_net: CTRL_GUEST_OFFLOADS depends on CTRL_VQ
From: "Michael S. Tsirkin"
Date: Sun, 5 Jan 2020 08:22:07 -0500
> The only way for guest to control offloads (as enabled by
> VIRTIO_NET_F_CTRL_GUEST_OFFLOADS) is by sending commands
> through CTRL_VQ. So it does not make sense to
> acknowledge VIRTIO_NET_F_CTRL_GUEST_OFFLOADS without
> VIRTIO_NET_F_CTRL_VQ.
>
> The spec does not outlaw devices with such a configuration, so we have
> to support it. Simply clear VIRTIO_NET_F_CTRL_GUEST_OFFLOADS.
> Note that Linux is still crashing if it tries to
> change the offloads when there's no control vq.
> That needs to be fixed by another patch.
>
> Reported-by: Alistair Delva
> Reported-by: Willem de Bruijn
> Fixes: 3f93522ffab2 ("virtio-net: switch off offloads on demand if possible
> on XDP set")
> Signed-off-by: Michael S. Tsirkin
> ---
>
> Same patch as v1 but update documentation so it's clear it's not
> enough to fix the crash.
Where are we with this patch? There seems to still be some unresolved
discussion about how we should actually handle this case.
Thanks.
___
Virtualization mailing list
Virtualization@lists.linux-foundation.org
https://lists.linuxfoundation.org/mailman/listinfo/virtualization
Re: [PATCH v2] virtio_net: CTRL_GUEST_OFFLOADS depends on CTRL_VQ
On 2020/1/7 下午3:06, Michael S. Tsirkin wrote:
On Tue, Jan 07, 2020 at 10:29:08AM +0800, Jason Wang wrote:
On 2020/1/6 下午8:54, Michael S. Tsirkin wrote:
On Mon, Jan 06, 2020 at 10:47:35AM +0800, Jason Wang wrote:
On 2020/1/5 下午9:22, Michael S. Tsirkin wrote:
The only way for guest to control offloads (as enabled by
VIRTIO_NET_F_CTRL_GUEST_OFFLOADS) is by sending commands
through CTRL_VQ. So it does not make sense to
acknowledge VIRTIO_NET_F_CTRL_GUEST_OFFLOADS without
VIRTIO_NET_F_CTRL_VQ.
The spec does not outlaw devices with such a configuration, so we have
to support it. Simply clear VIRTIO_NET_F_CTRL_GUEST_OFFLOADS.
Note that Linux is still crashing if it tries to
change the offloads when there's no control vq.
That needs to be fixed by another patch.
Reported-by: Alistair Delva
Reported-by: Willem de Bruijn
Fixes: 3f93522ffab2 ("virtio-net: switch off offloads on demand if possible on XDP
set")
Signed-off-by: Michael S. Tsirkin
---
Same patch as v1 but update documentation so it's clear it's not
enough to fix the crash.
drivers/net/virtio_net.c | 9 +
1 file changed, 9 insertions(+)
diff --git a/drivers/net/virtio_net.c b/drivers/net/virtio_net.c
index 4d7d5434cc5d..7b8805b47f0d 100644
--- a/drivers/net/virtio_net.c
+++ b/drivers/net/virtio_net.c
@@ -2971,6 +2971,15 @@ static int virtnet_validate(struct virtio_device *vdev)
if (!virtnet_validate_features(vdev))
return -EINVAL;
+ /* VIRTIO_NET_F_CTRL_GUEST_OFFLOADS does not work without
+* VIRTIO_NET_F_CTRL_VQ. Unfortunately spec forgot to
+* specify that VIRTIO_NET_F_CTRL_GUEST_OFFLOADS depends
+* on VIRTIO_NET_F_CTRL_VQ so devices can set the later but
+* not the former.
+*/
+ if (!virtio_has_feature(vdev, VIRTIO_NET_F_CTRL_VQ))
+ __virtio_clear_bit(vdev,
VIRTIO_NET_F_CTRL_GUEST_OFFLOADS);
If it's just because a bug of spec, should we simply fix the bug and fail
the negotiation in virtnet_validate_feature()?
One man's bug is another man's feature: arguably leaving the features
independent in the spec might allow reuse of the feature bit without
breaking guests.
And even if we say it's a bug we can't simply fix the bug in the
spec: changing the text for a future version does not change the fact
that devices behaving according to the spec exist.
Otherwise there would be inconsistency in handling feature dependencies for
ctrl vq.
Thanks
That's a cosmetic problem ATM. It might be a good idea to generally
change our handling of dependencies, and clear feature bits instead of
failing probe on a mismatch.
Something like I proposed in the past ? [1]
[1] https://lore.kernel.org/patchwork/patch/519074/
No that still fails probe.
I am asking whether it's more future proof to fail probe
on feature combinations disallowed by spec, or to clear bits
to get to an expected combination.
Sorry wrong link.
It should be: https://lkml.org/lkml/2014/11/17/82
In any case, we should probably document in the spec how
drivers behave on such combinations.
Yes.
Thanks
It's worth thinking - at the spec level -
how we can best make the configuration extensible.
But that's not something spec should worry about.
+
if (virtio_has_feature(vdev, VIRTIO_NET_F_MTU)) {
int mtu = virtio_cread16(vdev,
offsetof(struct virtio_net_config,
___
Virtualization mailing list
Virtualization@lists.linux-foundation.org
https://lists.linuxfoundation.org/mailman/listinfo/virtualization
Re: [PATCH v2] virtio_net: CTRL_GUEST_OFFLOADS depends on CTRL_VQ
On Tue, Jan 07, 2020 at 10:29:08AM +0800, Jason Wang wrote:
>
> On 2020/1/6 下午8:54, Michael S. Tsirkin wrote:
> > On Mon, Jan 06, 2020 at 10:47:35AM +0800, Jason Wang wrote:
> > > On 2020/1/5 下午9:22, Michael S. Tsirkin wrote:
> > > > The only way for guest to control offloads (as enabled by
> > > > VIRTIO_NET_F_CTRL_GUEST_OFFLOADS) is by sending commands
> > > > through CTRL_VQ. So it does not make sense to
> > > > acknowledge VIRTIO_NET_F_CTRL_GUEST_OFFLOADS without
> > > > VIRTIO_NET_F_CTRL_VQ.
> > > >
> > > > The spec does not outlaw devices with such a configuration, so we have
> > > > to support it. Simply clear VIRTIO_NET_F_CTRL_GUEST_OFFLOADS.
> > > > Note that Linux is still crashing if it tries to
> > > > change the offloads when there's no control vq.
> > > > That needs to be fixed by another patch.
> > > >
> > > > Reported-by: Alistair Delva
> > > > Reported-by: Willem de Bruijn
> > > > Fixes: 3f93522ffab2 ("virtio-net: switch off offloads on demand if
> > > > possible on XDP set")
> > > > Signed-off-by: Michael S. Tsirkin
> > > > ---
> > > >
> > > > Same patch as v1 but update documentation so it's clear it's not
> > > > enough to fix the crash.
> > > >
> > > >drivers/net/virtio_net.c | 9 +
> > > >1 file changed, 9 insertions(+)
> > > >
> > > > diff --git a/drivers/net/virtio_net.c b/drivers/net/virtio_net.c
> > > > index 4d7d5434cc5d..7b8805b47f0d 100644
> > > > --- a/drivers/net/virtio_net.c
> > > > +++ b/drivers/net/virtio_net.c
> > > > @@ -2971,6 +2971,15 @@ static int virtnet_validate(struct virtio_device
> > > > *vdev)
> > > > if (!virtnet_validate_features(vdev))
> > > > return -EINVAL;
> > > > + /* VIRTIO_NET_F_CTRL_GUEST_OFFLOADS does not work without
> > > > +* VIRTIO_NET_F_CTRL_VQ. Unfortunately spec forgot to
> > > > +* specify that VIRTIO_NET_F_CTRL_GUEST_OFFLOADS depends
> > > > +* on VIRTIO_NET_F_CTRL_VQ so devices can set the later but
> > > > +* not the former.
> > > > +*/
> > > > + if (!virtio_has_feature(vdev, VIRTIO_NET_F_CTRL_VQ))
> > > > + __virtio_clear_bit(vdev,
> > > > VIRTIO_NET_F_CTRL_GUEST_OFFLOADS);
> > >
> > > If it's just because a bug of spec, should we simply fix the bug and fail
> > > the negotiation in virtnet_validate_feature()?
> > One man's bug is another man's feature: arguably leaving the features
> > independent in the spec might allow reuse of the feature bit without
> > breaking guests.
> >
> > And even if we say it's a bug we can't simply fix the bug in the
> > spec: changing the text for a future version does not change the fact
> > that devices behaving according to the spec exist.
> >
> > > Otherwise there would be inconsistency in handling feature dependencies
> > > for
> > > ctrl vq.
> > >
> > > Thanks
> > That's a cosmetic problem ATM. It might be a good idea to generally
> > change our handling of dependencies, and clear feature bits instead of
> > failing probe on a mismatch.
>
>
> Something like I proposed in the past ? [1]
>
> [1] https://lore.kernel.org/patchwork/patch/519074/
No that still fails probe.
I am asking whether it's more future proof to fail probe
on feature combinations disallowed by spec, or to clear bits
to get to an expected combination.
In any case, we should probably document in the spec how
drivers behave on such combinations.
>
> > It's worth thinking - at the spec level -
> > how we can best make the configuration extensible.
> > But that's not something spec should worry about.
> >
> >
> > > > +
> > > > if (virtio_has_feature(vdev, VIRTIO_NET_F_MTU)) {
> > > > int mtu = virtio_cread16(vdev,
> > > > offsetof(struct
> > > > virtio_net_config,
___
Virtualization mailing list
Virtualization@lists.linux-foundation.org
https://lists.linuxfoundation.org/mailman/listinfo/virtualization
Re: [PATCH v2] virtio_net: CTRL_GUEST_OFFLOADS depends on CTRL_VQ
On 2020/1/6 下午8:54, Michael S. Tsirkin wrote:
On Mon, Jan 06, 2020 at 10:47:35AM +0800, Jason Wang wrote:
On 2020/1/5 下午9:22, Michael S. Tsirkin wrote:
The only way for guest to control offloads (as enabled by
VIRTIO_NET_F_CTRL_GUEST_OFFLOADS) is by sending commands
through CTRL_VQ. So it does not make sense to
acknowledge VIRTIO_NET_F_CTRL_GUEST_OFFLOADS without
VIRTIO_NET_F_CTRL_VQ.
The spec does not outlaw devices with such a configuration, so we have
to support it. Simply clear VIRTIO_NET_F_CTRL_GUEST_OFFLOADS.
Note that Linux is still crashing if it tries to
change the offloads when there's no control vq.
That needs to be fixed by another patch.
Reported-by: Alistair Delva
Reported-by: Willem de Bruijn
Fixes: 3f93522ffab2 ("virtio-net: switch off offloads on demand if possible on XDP
set")
Signed-off-by: Michael S. Tsirkin
---
Same patch as v1 but update documentation so it's clear it's not
enough to fix the crash.
drivers/net/virtio_net.c | 9 +
1 file changed, 9 insertions(+)
diff --git a/drivers/net/virtio_net.c b/drivers/net/virtio_net.c
index 4d7d5434cc5d..7b8805b47f0d 100644
--- a/drivers/net/virtio_net.c
+++ b/drivers/net/virtio_net.c
@@ -2971,6 +2971,15 @@ static int virtnet_validate(struct virtio_device *vdev)
if (!virtnet_validate_features(vdev))
return -EINVAL;
+ /* VIRTIO_NET_F_CTRL_GUEST_OFFLOADS does not work without
+* VIRTIO_NET_F_CTRL_VQ. Unfortunately spec forgot to
+* specify that VIRTIO_NET_F_CTRL_GUEST_OFFLOADS depends
+* on VIRTIO_NET_F_CTRL_VQ so devices can set the later but
+* not the former.
+*/
+ if (!virtio_has_feature(vdev, VIRTIO_NET_F_CTRL_VQ))
+ __virtio_clear_bit(vdev,
VIRTIO_NET_F_CTRL_GUEST_OFFLOADS);
If it's just because a bug of spec, should we simply fix the bug and fail
the negotiation in virtnet_validate_feature()?
One man's bug is another man's feature: arguably leaving the features
independent in the spec might allow reuse of the feature bit without
breaking guests.
And even if we say it's a bug we can't simply fix the bug in the
spec: changing the text for a future version does not change the fact
that devices behaving according to the spec exist.
Otherwise there would be inconsistency in handling feature dependencies for
ctrl vq.
Thanks
That's a cosmetic problem ATM. It might be a good idea to generally
change our handling of dependencies, and clear feature bits instead of
failing probe on a mismatch.
Something like I proposed in the past ? [1]
[1] https://lore.kernel.org/patchwork/patch/519074/
It's worth thinking - at the spec level -
how we can best make the configuration extensible.
But that's not something spec should worry about.
+
if (virtio_has_feature(vdev, VIRTIO_NET_F_MTU)) {
int mtu = virtio_cread16(vdev,
offsetof(struct virtio_net_config,
___
Virtualization mailing list
Virtualization@lists.linux-foundation.org
https://lists.linuxfoundation.org/mailman/listinfo/virtualization
Re: [PATCH v2] virtio_net: CTRL_GUEST_OFFLOADS depends on CTRL_VQ
On Mon, Jan 06, 2020 at 10:47:35AM +0800, Jason Wang wrote:
>
> On 2020/1/5 下午9:22, Michael S. Tsirkin wrote:
> > The only way for guest to control offloads (as enabled by
> > VIRTIO_NET_F_CTRL_GUEST_OFFLOADS) is by sending commands
> > through CTRL_VQ. So it does not make sense to
> > acknowledge VIRTIO_NET_F_CTRL_GUEST_OFFLOADS without
> > VIRTIO_NET_F_CTRL_VQ.
> >
> > The spec does not outlaw devices with such a configuration, so we have
> > to support it. Simply clear VIRTIO_NET_F_CTRL_GUEST_OFFLOADS.
> > Note that Linux is still crashing if it tries to
> > change the offloads when there's no control vq.
> > That needs to be fixed by another patch.
> >
> > Reported-by: Alistair Delva
> > Reported-by: Willem de Bruijn
> > Fixes: 3f93522ffab2 ("virtio-net: switch off offloads on demand if possible
> > on XDP set")
> > Signed-off-by: Michael S. Tsirkin
> > ---
> >
> > Same patch as v1 but update documentation so it's clear it's not
> > enough to fix the crash.
> >
> > drivers/net/virtio_net.c | 9 +
> > 1 file changed, 9 insertions(+)
> >
> > diff --git a/drivers/net/virtio_net.c b/drivers/net/virtio_net.c
> > index 4d7d5434cc5d..7b8805b47f0d 100644
> > --- a/drivers/net/virtio_net.c
> > +++ b/drivers/net/virtio_net.c
> > @@ -2971,6 +2971,15 @@ static int virtnet_validate(struct virtio_device
> > *vdev)
> > if (!virtnet_validate_features(vdev))
> > return -EINVAL;
> > + /* VIRTIO_NET_F_CTRL_GUEST_OFFLOADS does not work without
> > +* VIRTIO_NET_F_CTRL_VQ. Unfortunately spec forgot to
> > +* specify that VIRTIO_NET_F_CTRL_GUEST_OFFLOADS depends
> > +* on VIRTIO_NET_F_CTRL_VQ so devices can set the later but
> > +* not the former.
> > +*/
> > + if (!virtio_has_feature(vdev, VIRTIO_NET_F_CTRL_VQ))
> > + __virtio_clear_bit(vdev,
> > VIRTIO_NET_F_CTRL_GUEST_OFFLOADS);
>
>
> If it's just because a bug of spec, should we simply fix the bug and fail
> the negotiation in virtnet_validate_feature()?
One man's bug is another man's feature: arguably leaving the features
independent in the spec might allow reuse of the feature bit without
breaking guests.
And even if we say it's a bug we can't simply fix the bug in the
spec: changing the text for a future version does not change the fact
that devices behaving according to the spec exist.
> Otherwise there would be inconsistency in handling feature dependencies for
> ctrl vq.
>
> Thanks
That's a cosmetic problem ATM. It might be a good idea to generally
change our handling of dependencies, and clear feature bits instead of
failing probe on a mismatch. It's worth thinking - at the spec level -
how we can best make the configuration extensible.
But that's not something spec should worry about.
>
> > +
> > if (virtio_has_feature(vdev, VIRTIO_NET_F_MTU)) {
> > int mtu = virtio_cread16(vdev,
> > offsetof(struct virtio_net_config,
___
Virtualization mailing list
Virtualization@lists.linux-foundation.org
https://lists.linuxfoundation.org/mailman/listinfo/virtualization
Re: [PATCH v2] virtio_net: CTRL_GUEST_OFFLOADS depends on CTRL_VQ
On 2020/1/5 下午9:22, Michael S. Tsirkin wrote:
The only way for guest to control offloads (as enabled by
VIRTIO_NET_F_CTRL_GUEST_OFFLOADS) is by sending commands
through CTRL_VQ. So it does not make sense to
acknowledge VIRTIO_NET_F_CTRL_GUEST_OFFLOADS without
VIRTIO_NET_F_CTRL_VQ.
The spec does not outlaw devices with such a configuration, so we have
to support it. Simply clear VIRTIO_NET_F_CTRL_GUEST_OFFLOADS.
Note that Linux is still crashing if it tries to
change the offloads when there's no control vq.
That needs to be fixed by another patch.
Reported-by: Alistair Delva
Reported-by: Willem de Bruijn
Fixes: 3f93522ffab2 ("virtio-net: switch off offloads on demand if possible on XDP
set")
Signed-off-by: Michael S. Tsirkin
---
Same patch as v1 but update documentation so it's clear it's not
enough to fix the crash.
drivers/net/virtio_net.c | 9 +
1 file changed, 9 insertions(+)
diff --git a/drivers/net/virtio_net.c b/drivers/net/virtio_net.c
index 4d7d5434cc5d..7b8805b47f0d 100644
--- a/drivers/net/virtio_net.c
+++ b/drivers/net/virtio_net.c
@@ -2971,6 +2971,15 @@ static int virtnet_validate(struct virtio_device *vdev)
if (!virtnet_validate_features(vdev))
return -EINVAL;
+ /* VIRTIO_NET_F_CTRL_GUEST_OFFLOADS does not work without
+* VIRTIO_NET_F_CTRL_VQ. Unfortunately spec forgot to
+* specify that VIRTIO_NET_F_CTRL_GUEST_OFFLOADS depends
+* on VIRTIO_NET_F_CTRL_VQ so devices can set the later but
+* not the former.
+*/
+ if (!virtio_has_feature(vdev, VIRTIO_NET_F_CTRL_VQ))
+ __virtio_clear_bit(vdev,
VIRTIO_NET_F_CTRL_GUEST_OFFLOADS);
If it's just because a bug of spec, should we simply fix the bug and
fail the negotiation in virtnet_validate_feature()?
Otherwise there would be inconsistency in handling feature dependencies
for ctrl vq.
Thanks
+
if (virtio_has_feature(vdev, VIRTIO_NET_F_MTU)) {
int mtu = virtio_cread16(vdev,
offsetof(struct virtio_net_config,
___
Virtualization mailing list
Virtualization@lists.linux-foundation.org
https://lists.linuxfoundation.org/mailman/listinfo/virtualization
[PATCH v2] virtio_net: CTRL_GUEST_OFFLOADS depends on CTRL_VQ
The only way for guest to control offloads (as enabled by
VIRTIO_NET_F_CTRL_GUEST_OFFLOADS) is by sending commands
through CTRL_VQ. So it does not make sense to
acknowledge VIRTIO_NET_F_CTRL_GUEST_OFFLOADS without
VIRTIO_NET_F_CTRL_VQ.
The spec does not outlaw devices with such a configuration, so we have
to support it. Simply clear VIRTIO_NET_F_CTRL_GUEST_OFFLOADS.
Note that Linux is still crashing if it tries to
change the offloads when there's no control vq.
That needs to be fixed by another patch.
Reported-by: Alistair Delva
Reported-by: Willem de Bruijn
Fixes: 3f93522ffab2 ("virtio-net: switch off offloads on demand if possible on
XDP set")
Signed-off-by: Michael S. Tsirkin
---
Same patch as v1 but update documentation so it's clear it's not
enough to fix the crash.
drivers/net/virtio_net.c | 9 +
1 file changed, 9 insertions(+)
diff --git a/drivers/net/virtio_net.c b/drivers/net/virtio_net.c
index 4d7d5434cc5d..7b8805b47f0d 100644
--- a/drivers/net/virtio_net.c
+++ b/drivers/net/virtio_net.c
@@ -2971,6 +2971,15 @@ static int virtnet_validate(struct virtio_device *vdev)
if (!virtnet_validate_features(vdev))
return -EINVAL;
+ /* VIRTIO_NET_F_CTRL_GUEST_OFFLOADS does not work without
+* VIRTIO_NET_F_CTRL_VQ. Unfortunately spec forgot to
+* specify that VIRTIO_NET_F_CTRL_GUEST_OFFLOADS depends
+* on VIRTIO_NET_F_CTRL_VQ so devices can set the later but
+* not the former.
+*/
+ if (!virtio_has_feature(vdev, VIRTIO_NET_F_CTRL_VQ))
+ __virtio_clear_bit(vdev,
VIRTIO_NET_F_CTRL_GUEST_OFFLOADS);
+
if (virtio_has_feature(vdev, VIRTIO_NET_F_MTU)) {
int mtu = virtio_cread16(vdev,
offsetof(struct virtio_net_config,
--
MST
___
Virtualization mailing list
Virtualization@lists.linux-foundation.org
https://lists.linuxfoundation.org/mailman/listinfo/virtualization
