> > > > Since legacy virtio will no longer be modified, I don't think there is > > much value is exposing this new union as UAPI. I do appreciate the > > benefit to the implementation. > > > > [1] https://patches.linaro.org/project/netdev/patch/20210208185558.995292-3-willemdebruijn.kernel@xxxxxxxxx/ > Hi, William and Simon > > 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. > 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. > 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? This does seem mostly a helper to simplify kernel logic to me, which is better kept in non-UAPI headers. _______________________________________________ Virtualization mailing list Virtualization@xxxxxxxxxxxxxxxxxxxxxxxxxx https://lists.linuxfoundation.org/mailman/listinfo/virtualization
