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

[Thomas Huth <thuth@redhat.com>]

On 13/12/2022 09.32, Philippe Mathieu-Daudé wrote:
> 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();

This is where the virtio endianness depends on the CPU endianness. Looks 
like it is fortunately only taken into account after a device reset, and not 
for every access (as I originally thought).

  Thomas