Re: [RFC PATCH-for-8.0 06/10] hw/virtio: Cache access_is_big_endian value in VirtIODevice state

[Philippe Mathieu-Daudé]

On 13/12/22 09:03, Thomas Huth wrote:
> On 13/12/2022 08.30, Philippe Mathieu-Daudé wrote:
> > On 13/12/22 01:14, Richard Henderson wrote:
> > > On 12/12/22 17:05, Philippe Mathieu-Daudé wrote:
> > > > The device endianness doesn't change during runtime.
> > >
> > > What are you talking about?  Of course it does.
> >
> > The host CPU certainly does, but the virtio device doesn't... Does it?
> >
> > This check only consider the device, not the CPU:
> >
> >      bool virtio_access_is_big_endian(VirtIODevice *vdev)
> >      {
> >      #if defined(LEGACY_VIRTIO_IS_BIENDIAN)
> >          return virtio_is_big_endian(vdev);
> >      #elif TARGET_BIG_ENDIAN
> >          if (virtio_vdev_has_feature(vdev, VIRTIO_F_VERSION_1)) {
> >              /*Devices conforming to VIRTIO 1.0 or later are always LE.*/
> >              return false;
> >          }
> >          return true;
>
> Well, this part here means that the endianness can indeed change on the 
> device side during runtime. Depending on whether VIRTIO_F_VERSION_1 is 
> negotiated or not, the device is little or big endian. Happens on s390x 
> for example - for legacy virtio, big endian is used, and for modern 
> virtio, little endian is used instead.

virtio_is_big_endian() depends on vdev->device_endian which is set in:

1) virtio_init()

    void virtio_init(VirtIODevice *vdev, uint16_t device_id,
                     size_t config_size)
    {
        ....
        vdev->device_endian = virtio_default_endian();

2) virtio_load()

    int virtio_load(VirtIODevice *vdev, QEMUFile *f, int version_id)
    {
        int i, ret;
        int32_t config_len;
        uint32_t num;
        uint32_t features;
        BusState *qbus = qdev_get_parent_bus(DEVICE(vdev));
        VirtioBusClass *k = VIRTIO_BUS_GET_CLASS(qbus);
        VirtioDeviceClass *vdc = VIRTIO_DEVICE_GET_CLASS(vdev);

        /*
         * We poison the endianness to ensure it does not get
         * used before subsections have been loaded.
         */
        vdev->device_endian = VIRTIO_DEVICE_ENDIAN_UNKNOWN;
        ....

        if (vdev->device_endian == VIRTIO_DEVICE_ENDIAN_UNKNOWN) {
            vdev->device_endian = virtio_default_endian();
        }

3) virtio_reset()

    void virtio_reset(void *opaque)
    {
        VirtIODevice *vdev = opaque;
        VirtioDeviceClass *k = VIRTIO_DEVICE_GET_CLASS(vdev);
        int i;

        virtio_set_status(vdev, 0);
        if (current_cpu) {
            /* Guest initiated reset */
            vdev->device_endian = virtio_current_cpu_endian();
        } else {
            /* System reset */
            vdev->device_endian = virtio_default_endian();
        }

So the current patch is not complete and should be:

-- >8 --
diff --git a/hw/virtio/virtio.c b/hw/virtio/virtio.c
index 09b1a0e3d9..b02b9058f9 100644
--- a/hw/virtio/virtio.c
+++ b/hw/virtio/virtio.c
@@ -2124,6 +2124,7 @@ void virtio_reset(void *opaque)
         /* System reset */
         vdev->device_endian = virtio_default_endian();
     }
+    vdev->access_is_big_endian = virtio_access_is_big_endian(vdev);

     if (k->reset) {
         k->reset(vdev);
@@ -3018,6 +3019,7 @@ int virtio_load(VirtIODevice *vdev, QEMUFile *f, 
int version_id)

     if (vdev->device_endian == VIRTIO_DEVICE_ENDIAN_UNKNOWN) {
         vdev->device_endian = virtio_default_endian();
+        vdev->access_is_big_endian = virtio_access_is_big_endian(vdev);
     }

     if (virtio_64bit_features_needed(vdev)) {
@@ -3193,6 +3195,7 @@ void virtio_init(VirtIODevice *vdev, uint16_t 
device_id, size_t config_size)
     vdev->vmstate = qdev_add_vm_change_state_handler(DEVICE(vdev),
             virtio_vmstate_change, vdev);
     vdev->device_endian = virtio_default_endian();
+    vdev->access_is_big_endian = virtio_access_is_big_endian(vdev);
     vdev->use_guest_notifier_mask = true;
 }
---

Still, the result of virtio_access_is_big_endian() doesn't depend on
the CPU endianness in my analysis... What am I missing?

Thanks,

Phil.