| Message ID | 20250310045842.2650784-6-pierrick.bouvier@linaro.org |
|---|---|
| State | New |
| Headers | show |
| Series | make system memory API available for common code | expand |
On 3/9/25 21:58, Pierrick Bouvier wrote: > For now, they are duplicate of the same macros in cpu-all.h that we > eliminate in next commit. > > Keep code readable by not defining them with macros, but simply their > implementation. > > Signed-off-by: Pierrick Bouvier <pierrick.bouvier@linaro.org> Why do you want these in bswap.h, rather than tswap.h? They're target swaps, after all. r~
On 3/10/25 09:08, Richard Henderson wrote: > On 3/9/25 21:58, Pierrick Bouvier wrote: >> For now, they are duplicate of the same macros in cpu-all.h that we >> eliminate in next commit. >> >> Keep code readable by not defining them with macros, but simply their >> implementation. >> >> Signed-off-by: Pierrick Bouvier <pierrick.bouvier@linaro.org> > > Why do you want these in bswap.h, rather than tswap.h? > They're target swaps, after all. > > > r~ No preference on that, I simply added them to the same file than their explicit endianness variant. Would you prefer the endianness agnostic variant to be in tswap.h instead?
On 3/10/25 09:14, Pierrick Bouvier wrote: > On 3/10/25 09:08, Richard Henderson wrote: >> On 3/9/25 21:58, Pierrick Bouvier wrote: >>> For now, they are duplicate of the same macros in cpu-all.h that we >>> eliminate in next commit. >>> >>> Keep code readable by not defining them with macros, but simply their >>> implementation. >>> >>> Signed-off-by: Pierrick Bouvier <pierrick.bouvier@linaro.org> >> >> Why do you want these in bswap.h, rather than tswap.h? >> They're target swaps, after all. >> >> >> r~ > > No preference on that, I simply added them to the same file than their explicit endianness > variant. Would you prefer the endianness agnostic variant to be in tswap.h instead? I think I would. In addition, I think we want #ifdef COMPILING_PER_TARGET #define target_words_bigendian() TARGET_BIG_ENDIAN #else bool target_words_bigendian(void); #endif moving the conditional from around target_needs_bswap just below. With that, we eliminate the extra branch that you're otherwise adding to target-specific code with this patch. r~
On 3/10/25 09:37, Richard Henderson wrote: > On 3/10/25 09:14, Pierrick Bouvier wrote: >> On 3/10/25 09:08, Richard Henderson wrote: >>> On 3/9/25 21:58, Pierrick Bouvier wrote: >>>> For now, they are duplicate of the same macros in cpu-all.h that we >>>> eliminate in next commit. >>>> >>>> Keep code readable by not defining them with macros, but simply their >>>> implementation. >>>> >>>> Signed-off-by: Pierrick Bouvier <pierrick.bouvier@linaro.org> >>> >>> Why do you want these in bswap.h, rather than tswap.h? >>> They're target swaps, after all. >>> >>> >>> r~ >> >> No preference on that, I simply added them to the same file than their explicit endianness >> variant. Would you prefer the endianness agnostic variant to be in tswap.h instead? > > I think I would. Ok, I will move it. > > In addition, I think we want > > #ifdef COMPILING_PER_TARGET > #define target_words_bigendian() TARGET_BIG_ENDIAN > #else > bool target_words_bigendian(void); > #endif > > moving the conditional from around target_needs_bswap just below. > > With that, we eliminate the extra branch that you're otherwise > adding to target-specific code with this patch. > I understand the change requested, but should we really aim in that direction? In the end, if we pursue the compilation units deduplication, the branch will be present anyway. I'm ok with your change, just asking if we really want to preserve target specific code until the "end". > > r~
On 3/10/25 09:43, Pierrick Bouvier wrote: > On 3/10/25 09:37, Richard Henderson wrote: >> On 3/10/25 09:14, Pierrick Bouvier wrote: >>> On 3/10/25 09:08, Richard Henderson wrote: >>>> On 3/9/25 21:58, Pierrick Bouvier wrote: >>>>> For now, they are duplicate of the same macros in cpu-all.h that we >>>>> eliminate in next commit. >>>>> >>>>> Keep code readable by not defining them with macros, but simply their >>>>> implementation. >>>>> >>>>> Signed-off-by: Pierrick Bouvier <pierrick.bouvier@linaro.org> >>>> >>>> Why do you want these in bswap.h, rather than tswap.h? >>>> They're target swaps, after all. >>>> >>>> >>>> r~ >>> >>> No preference on that, I simply added them to the same file than their explicit endianness >>> variant. Would you prefer the endianness agnostic variant to be in tswap.h instead? >> >> I think I would. > > Ok, I will move it. > >> >> In addition, I think we want >> >> #ifdef COMPILING_PER_TARGET >> #define target_words_bigendian() TARGET_BIG_ENDIAN >> #else >> bool target_words_bigendian(void); >> #endif >> >> moving the conditional from around target_needs_bswap just below. >> >> With that, we eliminate the extra branch that you're otherwise >> adding to target-specific code with this patch. >> > > I understand the change requested, but should we really aim in that direction? In the end, > if we pursue the compilation units deduplication, the branch will be present anyway. > > I'm ok with your change, just asking if we really want to preserve target specific code > until the "end". All of target/ is target specific. De-duplication will not eliminate that. r~
On 3/10/25 09:53, Richard Henderson wrote: > On 3/10/25 09:43, Pierrick Bouvier wrote: >> On 3/10/25 09:37, Richard Henderson wrote: >>> On 3/10/25 09:14, Pierrick Bouvier wrote: >>>> On 3/10/25 09:08, Richard Henderson wrote: >>>>> On 3/9/25 21:58, Pierrick Bouvier wrote: >>>>>> For now, they are duplicate of the same macros in cpu-all.h that we >>>>>> eliminate in next commit. >>>>>> >>>>>> Keep code readable by not defining them with macros, but simply their >>>>>> implementation. >>>>>> >>>>>> Signed-off-by: Pierrick Bouvier <pierrick.bouvier@linaro.org> >>>>> >>>>> Why do you want these in bswap.h, rather than tswap.h? >>>>> They're target swaps, after all. >>>>> >>>>> >>>>> r~ >>>> >>>> No preference on that, I simply added them to the same file than their explicit endianness >>>> variant. Would you prefer the endianness agnostic variant to be in tswap.h instead? >>> >>> I think I would. >> >> Ok, I will move it. >> >>> >>> In addition, I think we want >>> >>> #ifdef COMPILING_PER_TARGET >>> #define target_words_bigendian() TARGET_BIG_ENDIAN >>> #else >>> bool target_words_bigendian(void); >>> #endif >>> >>> moving the conditional from around target_needs_bswap just below. >>> >>> With that, we eliminate the extra branch that you're otherwise >>> adding to target-specific code with this patch. >>> >> >> I understand the change requested, but should we really aim in that direction? In the end, >> if we pursue the compilation units deduplication, the branch will be present anyway. >> >> I'm ok with your change, just asking if we really want to preserve target specific code >> until the "end". > > All of target/ is target specific. De-duplication will not eliminate that. > My vocabulary was wrong here. I meant "if we want to preserve target specific macros" until the end. Sure, there will always be compilation units (devices, cpus, helpers, ...) specific to a target. I just wonder if sticking to ifdef paradigm for this kind of code is worth the "optimization" we are supposed to get. I'll add the change requested. > > r~
On Mon, 10 Mar 2025, Pierrick Bouvier wrote: > On 3/10/25 09:53, Richard Henderson wrote: >> On 3/10/25 09:43, Pierrick Bouvier wrote: >>> On 3/10/25 09:37, Richard Henderson wrote: >>>> On 3/10/25 09:14, Pierrick Bouvier wrote: >>>>> On 3/10/25 09:08, Richard Henderson wrote: >>>>>> On 3/9/25 21:58, Pierrick Bouvier wrote: >>>>>>> For now, they are duplicate of the same macros in cpu-all.h that we >>>>>>> eliminate in next commit. >>>>>>> >>>>>>> Keep code readable by not defining them with macros, but simply their >>>>>>> implementation. >>>>>>> >>>>>>> Signed-off-by: Pierrick Bouvier <pierrick.bouvier@linaro.org> >>>>>> >>>>>> Why do you want these in bswap.h, rather than tswap.h? >>>>>> They're target swaps, after all. >>>>>> >>>>>> >>>>>> r~ >>>>> >>>>> No preference on that, I simply added them to the same file than their >>>>> explicit endianness >>>>> variant. Would you prefer the endianness agnostic variant to be in >>>>> tswap.h instead? >>>> >>>> I think I would. >>> >>> Ok, I will move it. >>> >>>> >>>> In addition, I think we want >>>> >>>> #ifdef COMPILING_PER_TARGET >>>> #define target_words_bigendian() TARGET_BIG_ENDIAN >>>> #else >>>> bool target_words_bigendian(void); >>>> #endif >>>> >>>> moving the conditional from around target_needs_bswap just below. >>>> >>>> With that, we eliminate the extra branch that you're otherwise >>>> adding to target-specific code with this patch. >>>> >>> >>> I understand the change requested, but should we really aim in that >>> direction? In the end, >>> if we pursue the compilation units deduplication, the branch will be >>> present anyway. >>> >>> I'm ok with your change, just asking if we really want to preserve target >>> specific code >>> until the "end". >> >> All of target/ is target specific. De-duplication will not eliminate that. >> > > My vocabulary was wrong here. I meant "if we want to preserve target specific > macros" until the end. > Sure, there will always be compilation units (devices, cpus, helpers, ...) > specific to a target. I just wonder if sticking to ifdef paradigm for this > kind of code is worth the "optimization" we are supposed to get. I've already tried to say that in the previous reply but maybe I can explain it better here. I think keeping per target binaries would be desired so single binary would not replace it just become an additional option. For example when I want to play with old stuff I compile with --target-list=ppc-softmmu and don't want to wait compiling all the other targets I don't use and not even interested in PPC64. A distro may want to ship a single qemu-system binary instead but other distros may prefer per target packages not one huge package so users can decide which ones to install. All of these are valid use cases, therefore this single binary should be an additional option not the only true way from now on replacing existing per target builds. Regards, BALATON Zoltan > I'll add the change requested. > >> >> r~ > >
On 3/10/25 13:17, BALATON Zoltan wrote: > On Mon, 10 Mar 2025, Pierrick Bouvier wrote: >> On 3/10/25 09:53, Richard Henderson wrote: >>> On 3/10/25 09:43, Pierrick Bouvier wrote: >>>> On 3/10/25 09:37, Richard Henderson wrote: >>>>> On 3/10/25 09:14, Pierrick Bouvier wrote: >>>>>> On 3/10/25 09:08, Richard Henderson wrote: >>>>>>> On 3/9/25 21:58, Pierrick Bouvier wrote: >>>>>>>> For now, they are duplicate of the same macros in cpu-all.h that we >>>>>>>> eliminate in next commit. >>>>>>>> >>>>>>>> Keep code readable by not defining them with macros, but simply their >>>>>>>> implementation. >>>>>>>> >>>>>>>> Signed-off-by: Pierrick Bouvier <pierrick.bouvier@linaro.org> >>>>>>> >>>>>>> Why do you want these in bswap.h, rather than tswap.h? >>>>>>> They're target swaps, after all. >>>>>>> >>>>>>> >>>>>>> r~ >>>>>> >>>>>> No preference on that, I simply added them to the same file than their >>>>>> explicit endianness >>>>>> variant. Would you prefer the endianness agnostic variant to be in >>>>>> tswap.h instead? >>>>> >>>>> I think I would. >>>> >>>> Ok, I will move it. >>>> >>>>> >>>>> In addition, I think we want >>>>> >>>>> #ifdef COMPILING_PER_TARGET >>>>> #define target_words_bigendian() TARGET_BIG_ENDIAN >>>>> #else >>>>> bool target_words_bigendian(void); >>>>> #endif >>>>> >>>>> moving the conditional from around target_needs_bswap just below. >>>>> >>>>> With that, we eliminate the extra branch that you're otherwise >>>>> adding to target-specific code with this patch. >>>>> >>>> >>>> I understand the change requested, but should we really aim in that >>>> direction? In the end, >>>> if we pursue the compilation units deduplication, the branch will be >>>> present anyway. >>>> >>>> I'm ok with your change, just asking if we really want to preserve target >>>> specific code >>>> until the "end". >>> >>> All of target/ is target specific. De-duplication will not eliminate that. >>> >> >> My vocabulary was wrong here. I meant "if we want to preserve target specific >> macros" until the end. >> Sure, there will always be compilation units (devices, cpus, helpers, ...) >> specific to a target. I just wonder if sticking to ifdef paradigm for this >> kind of code is worth the "optimization" we are supposed to get. > > I've already tried to say that in the previous reply but maybe I can > explain it better here. I think keeping per target binaries would be > desired so single binary would not replace it just become an additional > option. For example when I want to play with old stuff I compile with > --target-list=ppc-softmmu and don't want to wait compiling all the other > targets I don't use and not even interested in PPC64. A distro may want to > ship a single qemu-system binary instead but other distros may prefer per > target packages not one huge package so users can decide which ones to > install. All of these are valid use cases, therefore this single binary > should be an additional option not the only true way from now on > replacing existing per target builds. > This valid use case is and *will* stay the default. There is no secret evil plan to break people habits here :). For now, we don't even introduce a "single binary" configure option, simply because the resulting code could not link, so there is no point. If you're curious about this, it is the command we use to check the work left before being able to do it: # configure with any target list, we use all in our case $ ./configure ... $ jq --raw-output < build/compile_commands.json '.[].file' | sort | uniq -c | sort -rn | grep -v '^\s*1 ' Once this list is empty, we'll be able to link and execute a single binary (and adding a additional new main() for that). But first things first. > Regards, > BALATON Zoltan > >> I'll add the change requested. >> >>> >>> r~ >> >>
diff --git a/include/qemu/bswap.h b/include/qemu/bswap.h index ebf6f9e5f5c..46ec62f716d 100644 --- a/include/qemu/bswap.h +++ b/include/qemu/bswap.h @@ -442,6 +442,76 @@ DO_STN_LDN_P(be) #undef DO_STN_LDN_P +/* Return ld{word}_{le,be}_p following target endianness. */ +#define LOAD_IMPL(word, args...) \ +do { \ + if (target_words_bigendian()) { \ + return glue(glue(ld, word), _be_p)(args); \ + } else { \ + return glue(glue(ld, word), _le_p)(args); \ + } \ +} while (0) + +static inline int lduw_p(const void *ptr) +{ + LOAD_IMPL(uw, ptr); +} + +static inline int ldsw_p(const void *ptr) +{ + LOAD_IMPL(sw, ptr); +} + +static inline int ldl_p(const void *ptr) +{ + LOAD_IMPL(l, ptr); +} + +static inline uint64_t ldq_p(const void *ptr) +{ + LOAD_IMPL(q, ptr); +} + +static inline uint64_t ldn_p(const void *ptr, int sz) +{ + LOAD_IMPL(n, ptr, sz); +} + +#undef LOAD_IMPL + +/* Call st{word}_{le,be}_p following target endianness. */ +#define STORE_IMPL(word, args...) \ +do { \ + if (target_words_bigendian()) { \ + glue(glue(st, word), _be_p)(args); \ + } else { \ + glue(glue(st, word), _le_p)(args); \ + } \ +} while (0) + + +static inline void stw_p(void *ptr, uint16_t v) +{ + STORE_IMPL(w, ptr, v); +} + +static inline void stl_p(void *ptr, uint32_t v) +{ + STORE_IMPL(l, ptr, v); +} + +static inline void stq_p(void *ptr, uint64_t v) +{ + STORE_IMPL(q, ptr, v); +} + +static inline void stn_p(void *ptr, int sz, uint64_t v) +{ + STORE_IMPL(n, ptr, sz, v); +} + +#undef STORE_IMPL + #undef le_bswap #undef be_bswap #undef le_bswaps
For now, they are duplicate of the same macros in cpu-all.h that we eliminate in next commit. Keep code readable by not defining them with macros, but simply their implementation. Signed-off-by: Pierrick Bouvier <pierrick.bouvier@linaro.org> --- include/qemu/bswap.h | 70 ++++++++++++++++++++++++++++++++++++++++++++ 1 file changed, 70 insertions(+)