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

["Philippe Mathieu-Daudé" <philmd@linaro.org>]

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.