Messages in this thread | | | Date | Wed, 16 Aug 2023 14:31:15 -0400 | Subject | Re: [PATCH net v1] virtio_net: Introduce skb_vnet_common_hdr to avoid typecasting | From | Feng Liu <> |
| |
On 2023-08-16 a.m.10:53, Willem de Bruijn wrote: > External email: Use caution opening links or attachments > > >> >> Thanks for the detailed explanation. >> >> I kept virtio_net_hdr_mrg_rxbuf and virtio_net_hdr_v1_hash structures in >> virtio_net.h, which can be forward compatible with existing user >> applications which use these structures. > > They're UAPI, so we cannot modify or remove them anyway. > > Which is exactly why we want to be careful with adding anything new. > ok
>> virtio_net_hdr_v1_hash cannot use virtio_net_hdr as the first member, >> because in virtio_net_hdr_v1, csum_start and csum_offset are stored in >> union as a structure, and virtio_net_hdr cannot be used instead. > > Oh right. That wasn't always the case, or the reason for this. > Not super relevant but, commit ed9ecb0415b9 has the history > > virtio: Don't expose legacy net features when VIRTIO_NET_NO_LEGACY defined. > > In particular, the virtio header always has the u16 num_buffers field. > We define a new 'struct virtio_net_hdr_v1' for this (rather than > simply calling it 'struct virtio_net_hdr', to avoid nasty type errors > if some parts of a project define VIRTIO_NET_NO_LEGACY and some don't. > > Transitional devices (which can't define VIRTIO_NET_NO_LEGACY) will > have to keep using struct virtio_net_hdr_mrg_rxbuf, which has the same > byte layout as struct virtio_net_hdr_v1. > > The union was added to overload csum use on tx with RSC use on rx, in > commit 22b436c9b568. I don't quite follow why there now are three > structs, rather than two. The first two seem to both implement csum > partial. Anyway, not super important here. >ok
>> In addition, I put this new structure virtio_net_common_hdr in uapi, >> hoping it could be used in future user space application to avoid >> potential risks caused by type coercion (such as the problems mentioned >> in the patch description ). So I think it should be in this header file. >> What do you think? > > Adding anything to UAPI has a high bar. Do you have a concrete use > case for this?
In the scene of with and without VIRTIO_NET_F_HASH_REPORT feature, this patch has been tested on my setup, and the function is ok.
> > This does seem mostly a helper to simplify kernel logic to me, which > is better kept in non-UAPI headers. OK, will change it.
| |