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 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;
>      #else
>          return false;
>      #endif
>      }
>
>      static inline bool virtio_is_big_endian(VirtIODevice *vdev)
>      {
>          if (!virtio_vdev_has_feature(vdev, VIRTIO_F_VERSION_1)) {
>              assert(vdev->device_endian != VIRTIO_DEVICE_ENDIAN_UNKNOWN);
>              return vdev->device_endian == VIRTIO_DEVICE_ENDIAN_BIG;
>          }
>          /* Devices conforming to VIRTIO 1.0 or later are always LE. */
>          return false;
>      }
>
> and once the features are negotiated it doesn't seem to change.
Per the spec, if the device changes its endianness, it must set the
VIRTIO_CONFIG_S_NEEDS_RESET bit:

  3.2.1 Notification of Device Configuration Changes

  For devices where the device-specific configuration information
  can be changed, a configuration change notification is sent when
  a device-specific configuration change occurs.
  In addition, this notification is triggered by the device setting
  DEVICE_NEEDS_RESET

However QEMU doesn't read this bit, only sets it:

hw/virtio/virtio.c:3551:        vdev->status = vdev->status | 
VIRTIO_CONFIG_S_NEEDS_RESET;
include/standard-headers/linux/virtio_config.h:44:#define 
VIRTIO_CONFIG_S_NEEDS_RESET   0x40
> > I mean, it doesn't often in practice, because the Linux kernel is 
> > compiled for one endianness and doesn't keep toggling state, but the 
> > hooks that you're replacing test for the *current* endianness state of 
> > the cpu.  So this is a behaviour change.
> I agree. Note however currently the CPU endianness is only checked once
> upon virtio device reset (or from a migration stream):
>
>      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();
>          }
>
>      bool cpu_virtio_is_big_endian(CPUState *cpu)
>      {
>          CPUClass *cc = CPU_GET_CLASS(cpu);
>
>          if (cc->sysemu_ops->virtio_is_big_endian) {
>              return cc->sysemu_ops->virtio_is_big_endian(cpu);
>          }
>          return target_words_bigendian();
>      }
>
> ARM being the single arch implementing a runtime endianness check:
>
>      static bool arm_cpu_virtio_is_big_endian(CPUState *cs)
>      {
>          ARMCPU *cpu = ARM_CPU(cs);
>          CPUARMState *env = &cpu->env;
>
>          cpu_synchronize_state(cs);
>          return arm_cpu_data_is_big_endian(env);
>      }
virtio_reset() is only called by virtio_bus_reset().

$ git grep -w virtio_bus_reset
hw/s390x/virtio-ccw.c:256:    virtio_bus_reset(&dev->bus);
hw/virtio/virtio-bus.c:102:void virtio_bus_reset(VirtioBusState *bus)
hw/virtio/virtio-mmio.c:75:    virtio_bus_reset(&proxy->bus);
hw/virtio/virtio-pci.c:1998:    virtio_bus_reset(bus);

So the result of virtio_access_is_big_endian() is only updated there,
after a virtio_bus_reset() call, and IIUC  isn't dependent on the CPU
endianness state, thus can be cached in VirtIODevice. But maybe the
current implementation is incomplete and my change is simply making
it worst...