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 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.

> 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);
     }

> Have you considered that the bootloader and the kernel may use different 
> endianness?

Certainly, but I'll revisit the code more thoughtfully.

Thanks,

Phil.