diff mbox series

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

Message ID 20221212230517.28872-7-philmd@linaro.org
State New
Headers show
Series hw/virtio: Build most objects as target independent units | expand

Commit Message

Philippe Mathieu-Daudé Dec. 12, 2022, 11:05 p.m. UTC
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(+)

Comments

Richard Henderson Dec. 13, 2022, 12:14 a.m. UTC | #1
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~
Philippe Mathieu-Daudé Dec. 13, 2022, 7:30 a.m. UTC | #2
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.
Thomas Huth Dec. 13, 2022, 8:03 a.m. UTC | #3
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
Philippe Mathieu-Daudé Dec. 13, 2022, 8:22 a.m. UTC | #4
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...
Philippe Mathieu-Daudé Dec. 13, 2022, 8:32 a.m. UTC | #5
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.
Thomas Huth Dec. 13, 2022, 8:47 a.m. UTC | #6
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
Richard Henderson Dec. 13, 2022, 3:41 p.m. UTC | #7
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 mbox series

Patch

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;