1. Patchew
  2. QEMU
  3. [PATCH v4 0/9] single-binary: hw/virtio
  4. View patch

[PATCH v4 5/9] include/hw/virtio/virtio-access.h: remove target specifics define

Pierrick Bouvier posted 9 patches 6 days, 5 hours ago
Maintainers: "Michael S. Tsirkin" <mst@redhat.com>, Stefano Garzarella <sgarzare@redhat.com>, Nicholas Piggin <npiggin@gmail.com>, Harsh Prateek Bora <harshpb@linux.ibm.com>, Pierrick Bouvier <pierrick.bouvier@linaro.org>, "Philippe Mathieu-Daudé" <philmd@linaro.org>
There is a newer version of this series
[PATCH v4 5/9] include/hw/virtio/virtio-access.h: remove target specifics define
Posted by Pierrick Bouvier 6 days, 5 hours ago 
Signed-off-by: Pierrick Bouvier <pierrick.bouvier@linaro.org>
---
 include/hw/virtio/virtio-access.h | 26 +++++++++++++-------------
 1 file changed, 13 insertions(+), 13 deletions(-)

diff --git a/include/hw/virtio/virtio-access.h b/include/hw/virtio/virtio-access.h
index f3b4d0075b5..1bea3445979 100644
--- a/include/hw/virtio/virtio-access.h
+++ b/include/hw/virtio/virtio-access.h
@@ -17,27 +17,27 @@
 #define QEMU_VIRTIO_ACCESS_H
 
 #include "exec/hwaddr.h"
+#include "qemu/target-info.h"
 #include "system/memory_cached.h"
 #include "hw/virtio/virtio.h"
 #include "hw/virtio/virtio-bus.h"
 
-#if defined(TARGET_PPC64) || defined(TARGET_ARM)
-#define LEGACY_VIRTIO_IS_BIENDIAN 1
-#endif
-
 static inline 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;
+    if (target_ppc64() || target_base_arm()) {
+        return virtio_is_big_endian(vdev);
     }
-    return true;
-#else
+
+    if (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;
+        } else {
+            return true;
+        }
+    }
+
     return false;
-#endif
 }
 
 static inline void virtio_stw_p(VirtIODevice *vdev, void *ptr, uint16_t v)
-- 
2.47.3
Re: [PATCH v4 5/9] include/hw/virtio/virtio-access.h: remove target specifics define
Posted by Richard Henderson 6 days ago 
On 2/4/26 06:56, Pierrick Bouvier wrote:
> Signed-off-by: Pierrick Bouvier <pierrick.bouvier@linaro.org>
> ---
>   include/hw/virtio/virtio-access.h | 26 +++++++++++++-------------
>   1 file changed, 13 insertions(+), 13 deletions(-)
> 
> diff --git a/include/hw/virtio/virtio-access.h b/include/hw/virtio/virtio-access.h
> index f3b4d0075b5..1bea3445979 100644
> --- a/include/hw/virtio/virtio-access.h
> +++ b/include/hw/virtio/virtio-access.h
> @@ -17,27 +17,27 @@
>   #define QEMU_VIRTIO_ACCESS_H
>   
>   #include "exec/hwaddr.h"
> +#include "qemu/target-info.h"
>   #include "system/memory_cached.h"
>   #include "hw/virtio/virtio.h"
>   #include "hw/virtio/virtio-bus.h"
>   
> -#if defined(TARGET_PPC64) || defined(TARGET_ARM)
> -#define LEGACY_VIRTIO_IS_BIENDIAN 1
> -#endif
> -
>   static inline 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;
> +    if (target_ppc64() || target_base_arm()) {
> +        return virtio_is_big_endian(vdev);

Why use two predicates like this, effectively calling target_arch() twice?
Surely it's better to do

     switch (target_arch()) {
     case SYS_EMU_TARGET_ARM:
     case SYS_EMU_TARGET_AARCH64:
     case SYS_EMU_TARGET_PPC64:
         return virtio_is_big_endian(vdev);
     default:
         break;
     }

Definitely with a comment saying this is about legacy behaviour.
And does VIRTIO_F_VERSION_1 really not take priority?


r~
Re: [PATCH v4 5/9] include/hw/virtio/virtio-access.h: remove target specifics define
Posted by Pierrick Bouvier 5 days, 10 hours ago 
On 2/3/26 6:10 PM, Richard Henderson wrote:
> On 2/4/26 06:56, Pierrick Bouvier wrote:
>> Signed-off-by: Pierrick Bouvier <pierrick.bouvier@linaro.org>
>> ---
>>    include/hw/virtio/virtio-access.h | 26 +++++++++++++-------------
>>    1 file changed, 13 insertions(+), 13 deletions(-)
>>
>> diff --git a/include/hw/virtio/virtio-access.h b/include/hw/virtio/virtio-access.h
>> index f3b4d0075b5..1bea3445979 100644
>> --- a/include/hw/virtio/virtio-access.h
>> +++ b/include/hw/virtio/virtio-access.h
>> @@ -17,27 +17,27 @@
>>    #define QEMU_VIRTIO_ACCESS_H
>>    
>>    #include "exec/hwaddr.h"
>> +#include "qemu/target-info.h"
>>    #include "system/memory_cached.h"
>>    #include "hw/virtio/virtio.h"
>>    #include "hw/virtio/virtio-bus.h"
>>    
>> -#if defined(TARGET_PPC64) || defined(TARGET_ARM)
>> -#define LEGACY_VIRTIO_IS_BIENDIAN 1
>> -#endif
>> -
>>    static inline 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;
>> +    if (target_ppc64() || target_base_arm()) {
>> +        return virtio_is_big_endian(vdev);
> 
> Why use two predicates like this, effectively calling target_arch() twice?
> Surely it's better to do
>

Yes it can, I simply wrote the simplest change possible. Since this 
function is called only once now at reset time, it definitely looks like 
premature optimization IMHO.

>       switch (target_arch()) {
>       case SYS_EMU_TARGET_ARM:
>       case SYS_EMU_TARGET_AARCH64:
>       case SYS_EMU_TARGET_PPC64:
>           return virtio_is_big_endian(vdev);
>       default:
>           break;
>       }
> 
> Definitely with a comment saying this is about legacy behaviour.
> And does VIRTIO_F_VERSION_1 really not take priority?
>

Not sure about this one, but current implementation has same behaviour 
than previous ifdef based one.
In case that's wrong, I would be happy to change it.

> 
> r~

Regards,
Pierrick
Re: [PATCH v4 5/9] include/hw/virtio/virtio-access.h: remove target specifics define
Posted by Pierrick Bouvier 4 days, 7 hours ago 
On 2/4/26 8:37 AM, Pierrick Bouvier wrote:
> On 2/3/26 6:10 PM, Richard Henderson wrote:
>> On 2/4/26 06:56, Pierrick Bouvier wrote:
>>> Signed-off-by: Pierrick Bouvier <pierrick.bouvier@linaro.org>
>>> ---
>>>     include/hw/virtio/virtio-access.h | 26 +++++++++++++-------------
>>>     1 file changed, 13 insertions(+), 13 deletions(-)
>>>
>>> diff --git a/include/hw/virtio/virtio-access.h b/include/hw/virtio/virtio-access.h
>>> index f3b4d0075b5..1bea3445979 100644
>>> --- a/include/hw/virtio/virtio-access.h
>>> +++ b/include/hw/virtio/virtio-access.h
>>> @@ -17,27 +17,27 @@
>>>     #define QEMU_VIRTIO_ACCESS_H
>>>     
>>>     #include "exec/hwaddr.h"
>>> +#include "qemu/target-info.h"
>>>     #include "system/memory_cached.h"
>>>     #include "hw/virtio/virtio.h"
>>>     #include "hw/virtio/virtio-bus.h"
>>>     
>>> -#if defined(TARGET_PPC64) || defined(TARGET_ARM)
>>> -#define LEGACY_VIRTIO_IS_BIENDIAN 1
>>> -#endif
>>> -
>>>     static inline 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;
>>> +    if (target_ppc64() || target_base_arm()) {
>>> +        return virtio_is_big_endian(vdev);
>>
>> Why use two predicates like this, effectively calling target_arch() twice?
>> Surely it's better to do
>>
> 
> Yes it can, I simply wrote the simplest change possible. Since this
> function is called only once now at reset time, it definitely looks like
> premature optimization IMHO.
> 
>>        switch (target_arch()) {
>>        case SYS_EMU_TARGET_ARM:
>>        case SYS_EMU_TARGET_AARCH64:
>>        case SYS_EMU_TARGET_PPC64:
>>            return virtio_is_big_endian(vdev);
>>        default:
>>            break;
>>        }
>>
>> Definitely with a comment saying this is about legacy behaviour.
>> And does VIRTIO_F_VERSION_1 really not take priority?
>>
> 
> Not sure about this one, but current implementation has same behaviour
> than previous ifdef based one.
> In case that's wrong, I would be happy to change it.
> 
>>
>> r~
> 
> Regards,
> Pierrick

For what it's worth, that's the patch I wrote when I was optimizing this 
function and found -1.7% for performance:

commit d497f71b5b1bcc46de97436e1ce31641ff792eb9
Author: Pierrick Bouvier <pierrick.bouvier@linaro.org>
Date:   Mon Feb 2 13:53:06 2026 -0800

     include/hw/virtio/virtio-access.h: optimize virtio_access_is_big_endian

     By following benchmark recommended by Stefan [1], it was found that
     previous commit introduced an overhead of 3% for this scenario.
     By reading TargetInfo only once and using a jump table, we get to 1.7%.

     [1] https://lore.kernel.org/qemu-devel/20260202185233.GC405548@fedora/

     Signed-off-by: Pierrick Bouvier <pierrick.bouvier@linaro.org>

diff --git a/include/hw/virtio/virtio-access.h 
b/include/hw/virtio/virtio-access.h
index 1bea3445979..b7695f56fed 100644
--- a/include/hw/virtio/virtio-access.h
+++ b/include/hw/virtio/virtio-access.h
@@ -24,11 +24,17 @@

  static inline bool virtio_access_is_big_endian(VirtIODevice *vdev)
  {
-    if (target_ppc64() || target_base_arm()) {
+    const TargetInfo *info = target_info();
+    static const int may_be_big_endian[SYS_EMU_TARGET__MAX] = {
+        [SYS_EMU_TARGET_ARM] = 1,
+        [SYS_EMU_TARGET_AARCH64] = 1,
+        [SYS_EMU_TARGET_PPC64] = 1,
+    };
+    if (may_be_big_endian[info->target_arch]) {
          return virtio_is_big_endian(vdev);
      }

-    if (target_big_endian()) {
+    if (info->endianness == ENDIAN_MODE_BIG) {
          if (virtio_vdev_has_feature(vdev, VIRTIO_F_VERSION_1)) {
              /* Devices conforming to VIRTIO 1.0 or later are always LE. */
              return false;

Patchew 2.1-devel-b67e4c2

© 2016 - 2026 Red Hat, Inc.