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.
_______________________________________________
Virtualization mailing list
Virtualization@xxxxxxxxxxxxxxxxxxxxxxxxxx
https://lists.linuxfoundation.org/mailman/listinfo/virtualization
