Guard the native endian APIs we want to remove by surrounding
them with TARGET_NOT_USING_LEGACY_NATIVE_ENDIAN_API #ifdef'ry.
Once a target gets cleaned we'll set the definition in the
target config, then the target won't be able to use the legacy
API anymore.
Signed-off-by: Philippe Mathieu-Daudé <philmd@linaro.org>
---
include/qemu/bswap.h | 5 +++++
1 file changed, 5 insertions(+)
diff --git a/include/qemu/bswap.h b/include/qemu/bswap.h
index 65a1b3634f4..d1c889e7bc9 100644
--- a/include/qemu/bswap.h
+++ b/include/qemu/bswap.h
@@ -412,7 +412,9 @@ static inline void stq_be_p(void *ptr, uint64_t v)
} \
}
+#ifndef TARGET_NOT_USING_LEGACY_NATIVE_ENDIAN_API
DO_STN_LDN_P(he)
+#endif
DO_STN_LDN_P(le)
DO_STN_LDN_P(be)
@@ -423,6 +425,7 @@ DO_STN_LDN_P(be)
#undef le_bswaps
#undef be_bswaps
+#ifndef TARGET_NOT_USING_LEGACY_NATIVE_ENDIAN_API
/* Return ld{word}_{le,be}_p following target endianness. */
#define LOAD_IMPL(word, args...) \
@@ -494,4 +497,6 @@ static inline void stn_p(void *ptr, int sz, uint64_t v)
#undef STORE_IMPL
+#endif /* TARGET_NOT_USING_LEGACY_NATIVE_ENDIAN_API */
+
#endif /* BSWAP_H */
--
2.52.0
Il mer 24 dic 2025, 16:24 Philippe Mathieu-Daudé <philmd@linaro.org> ha
scritto:
> Guard the native endian APIs we want to remove by surrounding
> them with TARGET_NOT_USING_LEGACY_NATIVE_ENDIAN_API #ifdef'ry.
>
> Once a target gets cleaned we'll set the definition in the
> target config, then the target won't be able to use the legacy
> API anymore.
>
Host endianness APIs are fine and are used when talking to the kernel.
These functions that take a void* should not be included in
TARGET_NOT_USING_LEGACY_NATIVE_ENDIAN_API.
(And also they are the same for all targets so they don't get in the way of
the single binary effort).
If the only change needed in the series is to drop this patch, don't bother
with reposting.
Paolo
> Signed-off-by: Philippe Mathieu-Daudé <philmd@linaro.org>
> ---
> include/qemu/bswap.h | 5 +++++
> 1 file changed, 5 insertions(+)
>
> diff --git a/include/qemu/bswap.h b/include/qemu/bswap.h
> index 65a1b3634f4..d1c889e7bc9 100644
> --- a/include/qemu/bswap.h
> +++ b/include/qemu/bswap.h
> @@ -412,7 +412,9 @@ static inline void stq_be_p(void *ptr, uint64_t v)
> } \
> }
>
> +#ifndef TARGET_NOT_USING_LEGACY_NATIVE_ENDIAN_API
> DO_STN_LDN_P(he)
> +#endif
> DO_STN_LDN_P(le)
> DO_STN_LDN_P(be)
>
> @@ -423,6 +425,7 @@ DO_STN_LDN_P(be)
> #undef le_bswaps
> #undef be_bswaps
>
> +#ifndef TARGET_NOT_USING_LEGACY_NATIVE_ENDIAN_API
>
> /* Return ld{word}_{le,be}_p following target endianness. */
> #define LOAD_IMPL(word, args...) \
> @@ -494,4 +497,6 @@ static inline void stn_p(void *ptr, int sz, uint64_t v)
>
> #undef STORE_IMPL
>
> +#endif /* TARGET_NOT_USING_LEGACY_NATIVE_ENDIAN_API */
> +
> #endif /* BSWAP_H */
> --
> 2.52.0
>
>
On 27/12/25 09:56, Paolo Bonzini wrote: > > > Il mer 24 dic 2025, 16:24 Philippe Mathieu-Daudé <philmd@linaro.org > <mailto:philmd@linaro.org>> ha scritto: > > Guard the native endian APIs we want to remove by surrounding > them with TARGET_NOT_USING_LEGACY_NATIVE_ENDIAN_API #ifdef'ry. > > Once a target gets cleaned we'll set the definition in the > target config, then the target won't be able to use the legacy > API anymore. > > > Host endianness APIs are fine and are used when talking to the kernel. > These functions that take a void* should not be included in > TARGET_NOT_USING_LEGACY_NATIVE_ENDIAN_API. > > (And also they are the same for all targets so they don't get in the way > of the single binary effort). Indeed they don't get in the way: I'm trying to have clearer APIs where everything is explicit. Anyway I can live keeping this for now. > If the only change needed in the series is to drop this patch, don't > bother with reposting. OK, thanks.
On 27/12/25 16:51, Philippe Mathieu-Daudé wrote:
> On 27/12/25 09:56, Paolo Bonzini wrote:
>>
>>
>> Il mer 24 dic 2025, 16:24 Philippe Mathieu-Daudé <philmd@linaro.org
>> <mailto:philmd@linaro.org>> ha scritto:
>>
>> Guard the native endian APIs we want to remove by surrounding
>> them with TARGET_NOT_USING_LEGACY_NATIVE_ENDIAN_API #ifdef'ry.
>>
>> Once a target gets cleaned we'll set the definition in the
>> target config, then the target won't be able to use the legacy
>> API anymore.
>>
>>
>> Host endianness APIs are fine and are used when talking to the kernel.
>> These functions that take a void* should not be included in
>> TARGET_NOT_USING_LEGACY_NATIVE_ENDIAN_API.
>>
>> (And also they are the same for all targets so they don't get in the
>> way of the single binary effort).
>
> Indeed they don't get in the way: I'm trying to have clearer APIs where
> everything is explicit. Anyway I can live keeping this for now.
I guess remembering my reasoning was:
1/ we can not have "guest native" endianness in single binary
2/ host endianness is only useful with "guest native" one,
otherwise if you know the guest endianness, you can just
use an explicit cpu_to_$endian method.
3/ thus once guest native endianness is gone there is no
usefulness of "host endianness", better to remove instead
of wondering "in which endianness is my host" and let the
compiler do better than us all possible optimisations.
I felt confident it was coherent because, except the ATI single
one-line case [*] which I believe is not the best implementation,
the rest of my series proved this API is easily removable, the
resulting code ending easier to understand IMHO.
I surely missed something and would like to be pointed at it,
so maybe I can revisit my approach.
[*]
https://lore.kernel.org/qemu-devel/1cdc2735-d9e0-27c1-90e3-e250bb73cad6@eik.bme.hu/
>
>> If the only change needed in the series is to drop this patch, don't
>> bother with reposting.
>
> OK, thanks.
Il sab 27 dic 2025, 21:34 Philippe Mathieu-Daudé <philmd@linaro.org> ha scritto: > 1/ we can not have "guest native" endianness in single binary > 2/ host endianness is only useful with "guest native" one, > otherwise if you know the guest endianness, you can just > use an explicit cpu_to_$endian method. > Host endianness is useful when not talking to the guest at all—e.g. for sockets or kernel APIs. *_he_* functions are basically just memcpy in that they support unaligned accesses; plus stn_he_p has the advantage of taking a value unlike memcpy which takes a pointer. Perhaps the source of the confusion is that they are in bswap.h but they (quite obviously since it's host endianness) never swap? I felt confident it was coherent because, except the ATI single > one-line case [*] which I believe is not the best implementation, > the rest of my series proved this API is easily removable, the > resulting code ending easier to understand IMHO. > It's easily removable because most of the time accesses are in guest endianness, or aligned, but the replacement for *_he_* functions is not picking a specific endianness; it's a normal pointer store. These are not super common but especially in hw/display/ you can find them. stn_he_p is the right choice for ATI. Paolo
On 28/12/25 11:44, Paolo Bonzini wrote: > > > Il sab 27 dic 2025, 21:34 Philippe Mathieu-Daudé <philmd@linaro.org > <mailto:philmd@linaro.org>> ha scritto: > > 1/ we can not have "guest native" endianness in single binary > 2/ host endianness is only useful with "guest native" one, > otherwise if you know the guest endianness, you can just > use an explicit cpu_to_$endian method. > > > Host endianness is useful when not talking to the guest at all—e.g. for > sockets or kernel APIs. > > *_he_* functions are basically just memcpy in that they support > unaligned accesses; plus stn_he_p has the advantage of taking a value > unlike memcpy which takes a pointer. I see. > Perhaps the source of the confusion is that they are in bswap.h but they > (quite obviously since it's host endianness) never swap? Hmm, maybe not well named API then. > I felt confident it was coherent because, except the ATI single > one-line case [*] which I believe is not the best implementation, > the rest of my series proved this API is easily removable, the > resulting code ending easier to understand IMHO. > > > It's easily removable because most of the time accesses are in guest > endianness, or aligned, but the replacement for *_he_* functions is not > picking a specific endianness; it's a normal pointer store. These are > not super common but especially in hw/display/ you can find them. > stn_he_p is the right choice for ATI. OK. Let's consider the following patches removed then: - 03/25 system: Use explicit endianness in subpage_ops::read/write() - 14/25 system: Use explicit endianness in ram_device::read/write() - 16/25 system: Allow restricting legacy ld/st_he() 'native-endian' API All the series I posted this week build fine without them.
Il dom 28 dic 2025, 16:14 Philippe Mathieu-Daudé <philmd@linaro.org> ha scritto: > > Perhaps the source of the confusion is that they are in bswap.h but they > > (quite obviously since it's host endianness) never swap? > > Hmm, maybe not well named API then. > The name is fine, the placement maybe a bit less; they could be moved out of bswap.h but it's not really necessary to do it now. OK. Let's consider the following patches removed then: > > - 03/25 system: Use explicit endianness in subpage_ops::read/write() > - 14/25 system: Use explicit endianness in ram_device::read/write() > - 16/25 system: Allow restricting legacy ld/st_he() 'native-endian' API > > All the series I posted this week build fine without them. > Great, the other change I suggested was about the handling of MO_BSWAP but it can be done separately. If you don't want to repost and prefer to drop patch 14, we will also remove DEVICE_NATIVE_ENDIAN from subpages in a second step, for example by using "HOST_BIG_ENDIAN ? DEVICE_BIG_ENDIAN : DEVICE_LITTLE_ENDIAN" as in the ram_device ops.
On 28/12/25 16:38, Paolo Bonzini wrote: > > > Il dom 28 dic 2025, 16:14 Philippe Mathieu-Daudé <philmd@linaro.org > <mailto:philmd@linaro.org>> ha scritto: > > > Perhaps the source of the confusion is that they are in bswap.h > but they > > (quite obviously since it's host endianness) never swap? > > Hmm, maybe not well named API then. > > > The name is fine, the placement maybe a bit less; they could be moved > out of bswap.h but it's not really necessary to do it now. Indeed not needed now, but already done to figure this API ;) This helped me to understand what we don't need is "DO_STN_LDN_P(he)" because this is a convoluted expansion to a plain memcpy(). > OK. Let's consider the following patches removed then: > > - 03/25 system: Use explicit endianness in subpage_ops::read/write() > - 14/25 system: Use explicit endianness in ram_device::read/write() > - 16/25 system: Allow restricting legacy ld/st_he() 'native-endian' API > > All the series I posted this week build fine without them. > > > Great, the other change I suggested was about the handling of MO_BSWAP > but it can be done separately. This request is not ignored, but I plan to address it on top to keep current changes simple enough. > > If you don't want to repost and prefer to drop patch 14, we will also > remove DEVICE_NATIVE_ENDIAN from subpages in a second step, for example > by using "HOST_BIG_ENDIAN ? DEVICE_BIG_ENDIAN : DEVICE_LITTLE_ENDIAN" as > in the ram_device ops. Yeah, that would even be smth like "DEVICE_ENDIANNESS_IS_IRRELEVANT", as we call directly the ld/st_unaligned helpers. I'll think about that later.
Il dom 28 dic 2025, 17:00 Philippe Mathieu-Daudé <philmd@linaro.org> ha scritto: > On 28/12/25 16:38, Paolo Bonzini wrote: > > > > > > Il dom 28 dic 2025, 16:14 Philippe Mathieu-Daudé <philmd@linaro.org > > <mailto:philmd@linaro.org>> ha scritto: > > > > > Perhaps the source of the confusion is that they are in bswap.h > > but they > > > (quite obviously since it's host endianness) never swap? > > > > Hmm, maybe not well named API then. > > > > > > The name is fine, the placement maybe a bit less; they could be moved > > out of bswap.h but it's not really necessary to do it now. > > Indeed not needed now, but already done to figure this API ;) This > helped me to understand what we don't need is "DO_STN_LDN_P(he)" > because this is a convoluted expansion to a plain memcpy(). > Without having seen your code, I will note that the simple conversion to memcpy() only works for little endian hosts. On big endian, you also need to adjust the first byte, like memcpy(p, ((uint8_t*)&val) + sizeof(val) - n, n); And likewise for ldn_he_p. (Apologize if you had noticed it, just trying to avoid a possible round trip over the holidays!) Paolo > > OK. Let's consider the following patches removed then: > > > > - 03/25 system: Use explicit endianness in subpage_ops::read/write() > > - 14/25 system: Use explicit endianness in ram_device::read/write() > > - 16/25 system: Allow restricting legacy ld/st_he() 'native-endian' > API > > > > All the series I posted this week build fine without them. > > > > > > Great, the other change I suggested was about the handling of MO_BSWAP > > but it can be done separately. > > This request is not ignored, but I plan to address it on top to keep > current changes simple enough. > > > > > If you don't want to repost and prefer to drop patch 14, we will also > > remove DEVICE_NATIVE_ENDIAN from subpages in a second step, for example > > by using "HOST_BIG_ENDIAN ? DEVICE_BIG_ENDIAN : DEVICE_LITTLE_ENDIAN" as > > in the ram_device ops. > > Yeah, that would even be smth like "DEVICE_ENDIANNESS_IS_IRRELEVANT", as > we call directly the ld/st_unaligned helpers. I'll think about that later. > >
On Sun, 28 Dec 2025 at 16:20, Paolo Bonzini <pbonzini@redhat.com> wrote: > > Il dom 28 dic 2025, 17:00 Philippe Mathieu-Daudé <philmd@linaro.org> ha scritto: >> >> On 28/12/25 16:38, Paolo Bonzini wrote: >> > >> > >> > Il dom 28 dic 2025, 16:14 Philippe Mathieu-Daudé <philmd@linaro.org >> > <mailto:philmd@linaro.org>> ha scritto: >> > >> > > Perhaps the source of the confusion is that they are in bswap.h >> > but they >> > > (quite obviously since it's host endianness) never swap? >> > >> > Hmm, maybe not well named API then. >> > >> > >> > The name is fine, the placement maybe a bit less; they could be moved >> > out of bswap.h but it's not really necessary to do it now. >> >> Indeed not needed now, but already done to figure this API ;) This >> helped me to understand what we don't need is "DO_STN_LDN_P(he)" >> because this is a convoluted expansion to a plain memcpy(). > > > Without having seen your code, I will note that the simple conversion to memcpy() only works for little endian hosts. On big endian, you also need to adjust the first byte, like > > memcpy(p, ((uint8_t*)&val) + sizeof(val) - n, n); > > And likewise for ldn_he_p. (Apologize if you had noticed it, just trying to avoid a possible round trip over the holidays!) But an inline memcpy() is hard to read and easy to get wrong: we have a pointer-cast and some pointer arithmetic going on here. What we want is to express our intent: "I am doing a load/store of N bytes which are in the host byte order and which might not be aligned". That's what the _he_p() functions are all for. thanks -- PMM
Il dom 28 dic 2025, 18:19 Peter Maydell <peter.maydell@linaro.org> ha scritto: > But an inline memcpy() is hard to read and easy to get wrong: > we have a pointer-cast and some pointer arithmetic going on here. > What we want is to express our intent: "I am doing a load/store > of N bytes which are in the host byte order and which might not > be aligned". That's what the _he_p() functions are all for. > Yes, I think (hope :)) that Phil wants to define ldn_he_p/stn_he_p as a memcpy instead of a switch statement. Paolo > thanks > -- PMM > >
On 28/12/25 17:20, Paolo Bonzini wrote: > Il dom 28 dic 2025, 17:00 Philippe Mathieu-Daudé <philmd@linaro.org > <mailto:philmd@linaro.org>> ha scritto: > > On 28/12/25 16:38, Paolo Bonzini wrote: > > > > > > Il dom 28 dic 2025, 16:14 Philippe Mathieu-Daudé > <philmd@linaro.org <mailto:philmd@linaro.org> > > <mailto:philmd@linaro.org <mailto:philmd@linaro.org>>> ha scritto: > > > > > Perhaps the source of the confusion is that they are in > bswap.h > > but they > > > (quite obviously since it's host endianness) never swap? > > > > Hmm, maybe not well named API then. > > > > > > The name is fine, the placement maybe a bit less; they could be > moved > > out of bswap.h but it's not really necessary to do it now. > > Indeed not needed now, but already done to figure this API ;) This > helped me to understand what we don't need is "DO_STN_LDN_P(he)" > because this is a convoluted expansion to a plain memcpy(). > > > Without having seen your code, I will note that the simple conversion to > memcpy() only works for little endian hosts. On big endian, you also > need to adjust the first byte, like > > memcpy(p, ((uint8_t*)&val) + sizeof(val) - n, n); Thanks, this is exactly what I had in mind for few replacements. > > And likewise for ldn_he_p. (Apologize if you had noticed it, just trying > to avoid a possible round trip over the holidays!) Thanks for your review and help :)
© 2016 - 2026 Red Hat, Inc.