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