Messages in this thread |  | | Subject | Re: [PATCH v2] virtio_net: CTRL_GUEST_OFFLOADS depends on CTRL_VQ | From | Jason Wang <> | Date | Tue, 7 Jan 2020 16:31:14 +0800 |
| |
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 <adelva@google.com> >>>>> Reported-by: Willem de Bruijn <willemdebruijn.kernel@gmail.com> >>>>> Fixes: 3f93522ffab2 ("virtio-net: switch off offloads on demand if possible on XDP set") >>>>> Signed-off-by: Michael S. Tsirkin <mst@redhat.com> >>>>> --- >>>>> >>>>> 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,
|  |