On Mon, 24 Jun 2024 at 16:11, Stefano Garzarella <sgarz...@redhat.com> wrote:
>
> CCing Jason.
>
> On Mon, Jun 24, 2024 at 4:30 PM Xoykie <xoy...@gmail.com> wrote:
> >
> > The virtio packed virtqueue support patch[1] suggests converting
> > endianness by lines:
> >
> > virtio_tswap16s(vdev, &e->off_wrap);
> > virtio_tswap16s(vdev, &e->flags);
> >
> > Though both of these conversion statements aren't present in the
> > latest qemu code here[2]
> >
> > Is this intentional?
>
> Good catch!
>
> It looks like it was removed (maybe by mistake) by commit
> d152cdd6f6 ("virtio: use virtio accessor to access packed event")That commit changes from: - address_space_read_cached(cache, off_off, &e->off_wrap, - sizeof(e->off_wrap)); - virtio_tswap16s(vdev, &e->off_wrap); which does a byte read of 2 bytes and then swaps the bytes depending on the host endianness and the value of virtio_access_is_big_endian() to this: + e->off_wrap = virtio_lduw_phys_cached(vdev, cache, off_off); virtio_lduw_phys_cached() is a small function which calls either lduw_be_phys_cached() or lduw_le_phys_cached() depending on the value of virtio_access_is_big_endian(). (And lduw_be_phys_cached() and lduw_le_phys_cached() do the right thing for the host-endianness to do a "load a specifically big or little endian 16-bit value".) Which is to say that because we use a load/store function that's explicit about the size of the data type it is accessing, the function itself can handle doing the load as big or little endian, rather than the calling code having to do a manual swap after it has done a load-as-bag-of-bytes. This is generally preferable as it's less error-prone. (Explicit swap-after-loading still has a place where the code is doing a load of a whole structure out of the guest and then swapping each struct field after the fact, because it means we can do a single load-from-guest-memory rather than a whole sequence of calls all the way down through the memory subsystem.) thanks -- PMM
