Re: [Bug Report] Possible Missing Endianness Conversion

[Peter Maydell]

On Mon, 24 Jun 2024 at 16:11, Stefano Garzarella <sgarzare@redhat.com> wrote:
>
> CCing Jason.
>
> On Mon, Jun 24, 2024 at 4:30 PM Xoykie <xoykie@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