| Message ID | 20250109-tun-v2-3-388d7d5a287a@daynix.com |
|---|---|
| State | New |
| Headers | show |
| Series | [v2,1/3] tun: Unify vnet implementation | expand |
On 2025/01/10 12:27, Jason Wang wrote: > On Thu, Jan 9, 2025 at 2:59 PM Akihiko Odaki <akihiko.odaki@daynix.com> wrote: >> >> The specification says the device MUST set num_buffers to 1 if >> VIRTIO_NET_F_MRG_RXBUF has not been negotiated. > > Have we agreed on how to fix the spec or not? > > As I replied in the spec patch, if we just remove this "MUST", it > looks like we are all fine? My understanding is that we should fix the kernel and QEMU instead. There may be some driver implementations that assumes num_buffers is 1 so the kernel and QEMU should be fixed to be compatible with such potential implementations. It is also possible to make future drivers with existing kernels and QEMU by ensuring they will not read num_buffers when VIRTIO_NET_F_MRG_RXBUF has not negotiated, and that's what "[PATCH v3] virtio-net: Ignore num_buffers when unused" does. https://lore.kernel.org/r/20250110-reserved-v3-1-2ade0a5d2090@daynix.com Regards, Akihiko Odaki
On Fri, Jan 10, 2025 at 11:27:13AM +0800, Jason Wang wrote: > On Thu, Jan 9, 2025 at 2:59 PM Akihiko Odaki <akihiko.odaki@daynix.com> wrote: > > > > The specification says the device MUST set num_buffers to 1 if > > VIRTIO_NET_F_MRG_RXBUF has not been negotiated. > > Have we agreed on how to fix the spec or not? > > As I replied in the spec patch, if we just remove this "MUST", it > looks like we are all fine? > > Thanks We should replace MUST with SHOULD but it is not all fine, ignoring SHOULD is a quality of implementation issue.
On 2025/01/10 19:23, Michael S. Tsirkin wrote: > On Fri, Jan 10, 2025 at 11:27:13AM +0800, Jason Wang wrote: >> On Thu, Jan 9, 2025 at 2:59 PM Akihiko Odaki <akihiko.odaki@daynix.com> wrote: >>> >>> The specification says the device MUST set num_buffers to 1 if >>> VIRTIO_NET_F_MRG_RXBUF has not been negotiated. >> >> Have we agreed on how to fix the spec or not? >> >> As I replied in the spec patch, if we just remove this "MUST", it >> looks like we are all fine? >> >> Thanks > > We should replace MUST with SHOULD but it is not all fine, > ignoring SHOULD is a quality of implementation issue. > Should we really replace it? It would mean that a driver conformant with the current specification may not be compatible with a device conformant with the future specification. We are going to fix all implementations known to buggy (QEMU and Linux) anyway so I think it's just fine to leave that part of specification as is.
On Wed, Jan 15, 2025 at 1:07 PM Akihiko Odaki <akihiko.odaki@daynix.com> wrote: > > On 2025/01/13 12:04, Jason Wang wrote: > > On Fri, Jan 10, 2025 at 7:12 PM Akihiko Odaki <akihiko.odaki@daynix.com> wrote: > >> > >> On 2025/01/10 19:23, Michael S. Tsirkin wrote: > >>> On Fri, Jan 10, 2025 at 11:27:13AM +0800, Jason Wang wrote: > >>>> On Thu, Jan 9, 2025 at 2:59 PM Akihiko Odaki <akihiko.odaki@daynix.com> wrote: > >>>>> > >>>>> The specification says the device MUST set num_buffers to 1 if > >>>>> VIRTIO_NET_F_MRG_RXBUF has not been negotiated. > >>>> > >>>> Have we agreed on how to fix the spec or not? > >>>> > >>>> As I replied in the spec patch, if we just remove this "MUST", it > >>>> looks like we are all fine? > >>>> > >>>> Thanks > >>> > >>> We should replace MUST with SHOULD but it is not all fine, > >>> ignoring SHOULD is a quality of implementation issue. > >>> > > > > So is this something that the driver should notice? > > > >> > >> Should we really replace it? It would mean that a driver conformant with > >> the current specification may not be compatible with a device conformant > >> with the future specification. > > > > I don't get this. We are talking about devices and we want to relax so > > it should compatibile. > > > The problem is: > 1) On the device side, the num_buffers can be left uninitialized due to bugs > 2) On the driver side, the specification allows assuming the num_buffers > is set to one. > > Relaxing the device requirement will replace "due to bugs" with > "according to the specification" in 1). It still contradicts with 2) so > does not fix compatibility. Just to clarify I meant we can simply remove the following: """ The device MUST use only a single descriptor if VIRTIO_NET_F_MRG_RXBUF was not negotiated. Note: This means that num_buffers will always be 1 if VIRTIO_NET_F_MRG_RXBUF is not negotiated. """ And """ If VIRTIO_NET_F_MRG_RXBUF has not been negotiated, the device MUST set num_buffers to 1. """ This seems easier as it reflects the fact where some devices don't set it. And it eases the transitional device as it doesn't need to have any special care. Then we don't need any driver normative so I don't see any conflict. Michael suggests we use "SHOULD", but if this is something that the driver needs to be aware of I don't know how "SHOULD" can help a lot or not. > > Instead, we should make the driver requirement stricter to change 2). > That is what "[PATCH v3] virtio-net: Ignore num_buffers when unused" does: > https://lore.kernel.org/r/20250110-reserved-v3-1-2ade0a5d2090@daynix.com > > > > >> > >> We are going to fix all implementations known to buggy (QEMU and Linux) > >> anyway so I think it's just fine to leave that part of specification as is. > > > > I don't think we can fix it all. > > It essentially only requires storing 16 bits. There are details we need > to work out, but it should be possible to fix. I meant it's not realistic to fix all the hypervisors. Note that modern devices have been implemented for about a decade so we may have too many versions of various hypervisors. (E.g DPDK seems to stick with the same behaviour of the current kernel). > > Regards, > Akihiko Odaki > Thanks
On Thu, Jan 16, 2025 at 1:30 PM Akihiko Odaki <akihiko.odaki@daynix.com> wrote: > > On 2025/01/16 10:06, Jason Wang wrote: > > On Wed, Jan 15, 2025 at 1:07 PM Akihiko Odaki <akihiko.odaki@daynix.com> wrote: > >> > >> On 2025/01/13 12:04, Jason Wang wrote: > >>> On Fri, Jan 10, 2025 at 7:12 PM Akihiko Odaki <akihiko.odaki@daynix.com> wrote: > >>>> > >>>> On 2025/01/10 19:23, Michael S. Tsirkin wrote: > >>>>> On Fri, Jan 10, 2025 at 11:27:13AM +0800, Jason Wang wrote: > >>>>>> On Thu, Jan 9, 2025 at 2:59 PM Akihiko Odaki <akihiko.odaki@daynix.com> wrote: > >>>>>>> > >>>>>>> The specification says the device MUST set num_buffers to 1 if > >>>>>>> VIRTIO_NET_F_MRG_RXBUF has not been negotiated. > >>>>>> > >>>>>> Have we agreed on how to fix the spec or not? > >>>>>> > >>>>>> As I replied in the spec patch, if we just remove this "MUST", it > >>>>>> looks like we are all fine? > >>>>>> > >>>>>> Thanks > >>>>> > >>>>> We should replace MUST with SHOULD but it is not all fine, > >>>>> ignoring SHOULD is a quality of implementation issue. > >>>>> > >>> > >>> So is this something that the driver should notice? > >>> > >>>> > >>>> Should we really replace it? It would mean that a driver conformant with > >>>> the current specification may not be compatible with a device conformant > >>>> with the future specification. > >>> > >>> I don't get this. We are talking about devices and we want to relax so > >>> it should compatibile. > >> > >> > >> The problem is: > >> 1) On the device side, the num_buffers can be left uninitialized due to bugs > >> 2) On the driver side, the specification allows assuming the num_buffers > >> is set to one. > >> > >> Relaxing the device requirement will replace "due to bugs" with > >> "according to the specification" in 1). It still contradicts with 2) so > >> does not fix compatibility. > > > > Just to clarify I meant we can simply remove the following: > > > > """ > > The device MUST use only a single descriptor if VIRTIO_NET_F_MRG_RXBUF > > was not negotiated. Note: This means that num_buffers will always be 1 > > if VIRTIO_NET_F_MRG_RXBUF is not negotiated. > > """ > > > > And > > > > """ > > If VIRTIO_NET_F_MRG_RXBUF has not been negotiated, the device MUST set > > num_buffers to 1. > > """ > > > > This seems easier as it reflects the fact where some devices don't set > > it. And it eases the transitional device as it doesn't need to have > > any special care. > > That can potentially break existing drivers that are compliant with the > current and assumes the num_buffers is set to 1. Those drivers are already 'broken'. Aren't they? Thanks > > Regards, > Akihiko Odaki > > > > > Then we don't need any driver normative so I don't see any conflict. > > > > Michael suggests we use "SHOULD", but if this is something that the > > driver needs to be aware of I don't know how "SHOULD" can help a lot > > or not. > > > >> > >> Instead, we should make the driver requirement stricter to change 2). > >> That is what "[PATCH v3] virtio-net: Ignore num_buffers when unused" does: > >> https://lore.kernel.org/r/20250110-reserved-v3-1-2ade0a5d2090@daynix.com > >> > >>> > >>>> > >>>> We are going to fix all implementations known to buggy (QEMU and Linux) > >>>> anyway so I think it's just fine to leave that part of specification as is. > >>> > >>> I don't think we can fix it all. > >> > >> It essentially only requires storing 16 bits. There are details we need > >> to work out, but it should be possible to fix. > > > > I meant it's not realistic to fix all the hypervisors. Note that > > modern devices have been implemented for about a decade so we may have > > too many versions of various hypervisors. (E.g DPDK seems to stick > > with the same behaviour of the current kernel). > > >> > >> Regards, > >> Akihiko Odaki > >> > > > > Thanks > > >
diff --git a/drivers/net/tap.c b/drivers/net/tap.c index 60804855510b..fe9554ee5b8b 100644 --- a/drivers/net/tap.c +++ b/drivers/net/tap.c @@ -713,7 +713,7 @@ static ssize_t tap_put_user(struct tap_queue *q, int total; if (q->flags & IFF_VNET_HDR) { - struct virtio_net_hdr vnet_hdr; + struct virtio_net_hdr_v1 vnet_hdr; vnet_hdr_len = READ_ONCE(q->vnet_hdr_sz); diff --git a/drivers/net/tun.c b/drivers/net/tun.c index dbf0dee92e93..f211d0580887 100644 --- a/drivers/net/tun.c +++ b/drivers/net/tun.c @@ -1991,7 +1991,9 @@ static ssize_t tun_put_user_xdp(struct tun_struct *tun, size_t total; if (tun->flags & IFF_VNET_HDR) { - struct virtio_net_hdr gso = { 0 }; + struct virtio_net_hdr_v1 gso = { + .num_buffers = __virtio16_to_cpu(true, 1) + }; vnet_hdr_sz = READ_ONCE(tun->vnet_hdr_sz); ret = tun_vnet_hdr_put(vnet_hdr_sz, iter, &gso); @@ -2044,7 +2046,7 @@ static ssize_t tun_put_user(struct tun_struct *tun, } if (vnet_hdr_sz) { - struct virtio_net_hdr gso; + struct virtio_net_hdr_v1 gso; ret = tun_vnet_hdr_from_skb(tun->flags, tun->dev, skb, &gso); if (ret < 0) diff --git a/drivers/net/tun_vnet.c b/drivers/net/tun_vnet.c index ffb2186facd3..a7a7989fae56 100644 --- a/drivers/net/tun_vnet.c +++ b/drivers/net/tun_vnet.c @@ -130,15 +130,17 @@ int tun_vnet_hdr_get(int sz, unsigned int flags, struct iov_iter *from, EXPORT_SYMBOL_GPL(tun_vnet_hdr_get); int tun_vnet_hdr_put(int sz, struct iov_iter *iter, - const struct virtio_net_hdr *hdr) + const struct virtio_net_hdr_v1 *hdr) { + int content_sz = MIN(sizeof(*hdr), sz); + if (iov_iter_count(iter) < sz) return -EINVAL; - if (copy_to_iter(hdr, sizeof(*hdr), iter) != sizeof(*hdr)) + if (copy_to_iter(hdr, content_sz, iter) != content_sz) return -EFAULT; - if (iov_iter_zero(sz - sizeof(*hdr), iter) != sz - sizeof(*hdr)) + if (iov_iter_zero(sz - content_sz, iter) != sz - content_sz) return -EFAULT; return 0; @@ -154,11 +156,11 @@ EXPORT_SYMBOL_GPL(tun_vnet_hdr_to_skb); int tun_vnet_hdr_from_skb(unsigned int flags, const struct net_device *dev, const struct sk_buff *skb, - struct virtio_net_hdr *hdr) + struct virtio_net_hdr_v1 *hdr) { int vlan_hlen = skb_vlan_tag_present(skb) ? VLAN_HLEN : 0; - if (virtio_net_hdr_from_skb(skb, hdr, + if (virtio_net_hdr_from_skb(skb, (struct virtio_net_hdr *)hdr, tun_vnet_is_little_endian(flags), true, vlan_hlen)) { struct skb_shared_info *sinfo = skb_shinfo(skb); @@ -176,6 +178,8 @@ int tun_vnet_hdr_from_skb(unsigned int flags, const struct net_device *dev, return -EINVAL; } + hdr->num_buffers = 1; + return 0; } EXPORT_SYMBOL_GPL(tun_vnet_hdr_from_skb); diff --git a/drivers/net/tun_vnet.h b/drivers/net/tun_vnet.h index 2dfdbe92bb24..d8fd94094227 100644 --- a/drivers/net/tun_vnet.h +++ b/drivers/net/tun_vnet.h @@ -12,13 +12,13 @@ int tun_vnet_hdr_get(int sz, unsigned int flags, struct iov_iter *from, struct virtio_net_hdr *hdr); int tun_vnet_hdr_put(int sz, struct iov_iter *iter, - const struct virtio_net_hdr *hdr); + const struct virtio_net_hdr_v1 *hdr); int tun_vnet_hdr_to_skb(unsigned int flags, struct sk_buff *skb, const struct virtio_net_hdr *hdr); int tun_vnet_hdr_from_skb(unsigned int flags, const struct net_device *dev, const struct sk_buff *skb, - struct virtio_net_hdr *hdr); + struct virtio_net_hdr_v1 *hdr); #endif /* TUN_VNET_H */
The specification says the device MUST set num_buffers to 1 if VIRTIO_NET_F_MRG_RXBUF has not been negotiated. Signed-off-by: Akihiko Odaki <akihiko.odaki@daynix.com> --- drivers/net/tap.c | 2 +- drivers/net/tun.c | 6 ++++-- drivers/net/tun_vnet.c | 14 +++++++++----- drivers/net/tun_vnet.h | 4 ++-- 4 files changed, 16 insertions(+), 10 deletions(-)