| Message ID | 20221212230517.28872-7-philmd@linaro.org |
|---|---|
| State | New |
| Headers | show |
| Series | hw/virtio: Build most objects as target independent units | expand |
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.
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.
Have you considered that the bootloader and the kernel may use different endianness?
r~
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.
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. Thomas
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...
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.
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
On 12/13/22 01: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); This is set for both ARM and PPC, which checks current cpu endianness in both cases. > and once the features are negotiated it doesn't seem to change. Incorrect. r~
diff --git a/hw/virtio/virtio.c b/hw/virtio/virtio.c index 09b1a0e3d9..dbb1fe33f7 100644 --- a/hw/virtio/virtio.c +++ b/hw/virtio/virtio.c @@ -3193,6 +3193,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; } diff --git a/include/hw/virtio/virtio.h b/include/hw/virtio/virtio.h index acfd4df125..5f28e51e93 100644 --- a/include/hw/virtio/virtio.h +++ b/include/hw/virtio/virtio.h @@ -130,6 +130,7 @@ struct VirtIODevice bool vhost_started; VMChangeStateEntry *vmstate; char *bus_name; + bool access_is_big_endian; uint8_t device_endian; bool use_guest_notifier_mask; AddressSpace *dma_as;
The device endianness doesn't change during runtime. Cache it in the VirtIODevice state. Signed-off-by: Philippe Mathieu-Daudé <philmd@linaro.org> --- RFC: I'm not sure virtio_init() is the correct place to add this check. We want to initialize this field once the features are negociated. --- hw/virtio/virtio.c | 1 + include/hw/virtio/virtio.h | 1 + 2 files changed, 2 insertions(+)