| Message ID | 20250311040838.3937136-1-pierrick.bouvier@linaro.org |
|---|---|
| Headers | show |
| Series | make system memory API available for common code | expand |
On 11/3/25 05:08, Pierrick Bouvier wrote: > Reviewed-by: Richard Henderson <richard.henderson@linaro.org> > Signed-off-by: Pierrick Bouvier <pierrick.bouvier@linaro.org> Missing the "why" justification we couldn't do that before. > --- > include/exec/exec-all.h | 1 - > 1 file changed, 1 deletion(-) > > diff --git a/include/exec/exec-all.h b/include/exec/exec-all.h > index dd5c40f2233..19b0eda44a7 100644 > --- a/include/exec/exec-all.h > +++ b/include/exec/exec-all.h > @@ -20,7 +20,6 @@ > #ifndef EXEC_ALL_H > #define EXEC_ALL_H > > -#include "cpu.h" > #if defined(CONFIG_USER_ONLY) > #include "exec/cpu_ldst.h" > #endif
On 11/3/25 05:08, Pierrick Bouvier wrote: > Reviewed-by: Richard Henderson <richard.henderson@linaro.org> > Signed-off-by: Pierrick Bouvier <pierrick.bouvier@linaro.org> Missing the "why" justification we couldn't do that before. > --- > include/exec/memory-internal.h | 2 -- > 1 file changed, 2 deletions(-) > > diff --git a/include/exec/memory-internal.h b/include/exec/memory-internal.h > index 100c1237ac2..b729f3b25ad 100644 > --- a/include/exec/memory-internal.h > +++ b/include/exec/memory-internal.h > @@ -20,8 +20,6 @@ > #ifndef MEMORY_INTERNAL_H > #define MEMORY_INTERNAL_H > > -#include "cpu.h" > - > #ifndef CONFIG_USER_ONLY > static inline AddressSpaceDispatch *flatview_to_dispatch(FlatView *fv) > {
On 11/3/25 05:08, Pierrick Bouvier wrote: > Reviewed-by: Richard Henderson <richard.henderson@linaro.org> > Signed-off-by: Pierrick Bouvier <pierrick.bouvier@linaro.org> I didn't follow this direction because Richard had a preference on removing unnecessary inlined functions: https://lore.kernel.org/qemu-devel/9151205a-13d3-401e-b403-f9195cdb1a45@linaro.org/ > --- > include/exec/ram_addr.h | 8 ++++++-- > 1 file changed, 6 insertions(+), 2 deletions(-) > > diff --git a/include/exec/ram_addr.h b/include/exec/ram_addr.h > index 7c011fadd11..098fccb5835 100644 > --- a/include/exec/ram_addr.h > +++ b/include/exec/ram_addr.h > @@ -342,7 +342,9 @@ static inline void cpu_physical_memory_set_dirty_range(ram_addr_t start, > } > } > > - xen_hvm_modified_memory(start, length); > + if (xen_enabled()) { > + xen_hvm_modified_memory(start, length); > + } > }
On 3/11/25 00:26, Philippe Mathieu-Daudé wrote: > On 11/3/25 05:08, Pierrick Bouvier wrote: >> Reviewed-by: Richard Henderson <richard.henderson@linaro.org> >> Signed-off-by: Pierrick Bouvier <pierrick.bouvier@linaro.org> > > Missing the "why" justification we couldn't do that before. > >> --- >> include/exec/exec-all.h | 1 - >> 1 file changed, 1 deletion(-) >> >> diff --git a/include/exec/exec-all.h b/include/exec/exec-all.h >> index dd5c40f2233..19b0eda44a7 100644 >> --- a/include/exec/exec-all.h >> +++ b/include/exec/exec-all.h >> @@ -20,7 +20,6 @@ >> #ifndef EXEC_ALL_H >> #define EXEC_ALL_H >> >> -#include "cpu.h" >> #if defined(CONFIG_USER_ONLY) >> #include "exec/cpu_ldst.h" >> #endif > Previous commit is named: codebase: prepare to remove cpu.h from exec/exec-all.h So before those changes, it's not possible. I can repeat that here, or squash the patches together, as you prefer.
On 3/11/25 00:26, Philippe Mathieu-Daudé wrote: > On 11/3/25 05:08, Pierrick Bouvier wrote: >> Reviewed-by: Richard Henderson <richard.henderson@linaro.org> >> Signed-off-by: Pierrick Bouvier <pierrick.bouvier@linaro.org> > > Missing the "why" justification we couldn't do that before. > >> --- >> include/exec/memory-internal.h | 2 -- >> 1 file changed, 2 deletions(-) >> >> diff --git a/include/exec/memory-internal.h b/include/exec/memory-internal.h >> index 100c1237ac2..b729f3b25ad 100644 >> --- a/include/exec/memory-internal.h >> +++ b/include/exec/memory-internal.h >> @@ -20,8 +20,6 @@ >> #ifndef MEMORY_INTERNAL_H >> #define MEMORY_INTERNAL_H >> >> -#include "cpu.h" >> - >> #ifndef CONFIG_USER_ONLY >> static inline AddressSpaceDispatch *flatview_to_dispatch(FlatView *fv) >> { > No direct dependency, but when a common code will include that (like system/memory.c), we can't have a dependency on cpu.h anymore. I can reorder or squash commits if you prefer.
On 3/11/25 00:29, Philippe Mathieu-Daudé wrote: > On 11/3/25 05:08, Pierrick Bouvier wrote: >> Reviewed-by: Richard Henderson <richard.henderson@linaro.org> >> Signed-off-by: Pierrick Bouvier <pierrick.bouvier@linaro.org> > > I didn't follow this direction because Richard had a preference > on removing unnecessary inlined functions: > https://lore.kernel.org/qemu-devel/9151205a-13d3-401e-b403-f9195cdb1a45@linaro.org/ > The patch you mention was moving code, which can be arguably different from simply editing existing one. That said, and even though the concern is real, I would appreciate to keep this series focused on achieving the goal, and not doing a refactor of the involved headers. >> --- >> include/exec/ram_addr.h | 8 ++++++-- >> 1 file changed, 6 insertions(+), 2 deletions(-) >> >> diff --git a/include/exec/ram_addr.h b/include/exec/ram_addr.h >> index 7c011fadd11..098fccb5835 100644 >> --- a/include/exec/ram_addr.h >> +++ b/include/exec/ram_addr.h >> @@ -342,7 +342,9 @@ static inline void cpu_physical_memory_set_dirty_range(ram_addr_t start, >> } >> } >> >> - xen_hvm_modified_memory(start, length); >> + if (xen_enabled()) { >> + xen_hvm_modified_memory(start, length); >> + } >> } >
On 3/11/25 00:36, Philippe Mathieu-Daudé wrote: > On 11/3/25 05:08, Pierrick Bouvier wrote: >> we'll use it in system/memory.c. > > Having part of the commit description separated in its subject is a > bit annoying. But then I'm probably using 20-years too old tools in > my patch workflow. > > Only used in system/{memory,physmem}.c, worth move to a local > system/memory-internal.h header? Or even simpler, move > include/exec/memory-internal.h -> exec/memory-internal.h first. > Good point, I'll move them to the existing exec/memory-internal.h in an additional commit. >> Signed-off-by: Pierrick Bouvier <pierrick.bouvier@linaro.org> >> --- >> include/exec/memory.h | 18 ++++++++++++------ >> 1 file changed, 12 insertions(+), 6 deletions(-) >> >> diff --git a/include/exec/memory.h b/include/exec/memory.h >> index 60c0fb6ccd4..57661283684 100644 >> --- a/include/exec/memory.h >> +++ b/include/exec/memory.h >> @@ -3138,16 +3138,22 @@ address_space_write_cached(MemoryRegionCache *cache, hwaddr addr, >> MemTxResult address_space_set(AddressSpace *as, hwaddr addr, >> uint8_t c, hwaddr len, MemTxAttrs attrs); >> >> -/* enum device_endian to MemOp. */ >> -static inline MemOp devend_memop(enum device_endian end) >> +/* returns true if end is big endian. */ >> +static inline bool devend_big_endian(enum device_endian end) >> { >> QEMU_BUILD_BUG_ON(DEVICE_HOST_ENDIAN != DEVICE_LITTLE_ENDIAN && >> DEVICE_HOST_ENDIAN != DEVICE_BIG_ENDIAN); >> >> - bool big_endian = (end == DEVICE_NATIVE_ENDIAN >> - ? target_words_bigendian() >> - : end == DEVICE_BIG_ENDIAN); >> - return big_endian ? MO_BE : MO_LE; >> + if (end == DEVICE_NATIVE_ENDIAN) { >> + return target_words_bigendian(); >> + } >> + return end == DEVICE_BIG_ENDIAN; >> +} >> + >> +/* enum device_endian to MemOp. */ >> +static inline MemOp devend_memop(enum device_endian end) >> +{ >> + return devend_big_endian(end) ? MO_BE : MO_LE; >> } >> >> /* >
On 3/11/25 00:36, Philippe Mathieu-Daudé wrote: > On 11/3/25 05:08, Pierrick Bouvier wrote: >> we'll use it in system/memory.c. > > Having part of the commit description separated in its subject is a > bit annoying. But then I'm probably using 20-years too old tools in > my patch workflow. Can you please share what would be the message you (or the tool) would prefer in this case? It's just a single line subject (saying "we extract the function") + an additional line justifying why it's needed. > > Only used in system/{memory,physmem}.c, worth move to a local > system/memory-internal.h header? Or even simpler, move > include/exec/memory-internal.h -> exec/memory-internal.h first. > >> Signed-off-by: Pierrick Bouvier <pierrick.bouvier@linaro.org> >> --- >> include/exec/memory.h | 18 ++++++++++++------ >> 1 file changed, 12 insertions(+), 6 deletions(-) >> >> diff --git a/include/exec/memory.h b/include/exec/memory.h >> index 60c0fb6ccd4..57661283684 100644 >> --- a/include/exec/memory.h >> +++ b/include/exec/memory.h >> @@ -3138,16 +3138,22 @@ address_space_write_cached(MemoryRegionCache *cache, hwaddr addr, >> MemTxResult address_space_set(AddressSpace *as, hwaddr addr, >> uint8_t c, hwaddr len, MemTxAttrs attrs); >> >> -/* enum device_endian to MemOp. */ >> -static inline MemOp devend_memop(enum device_endian end) >> +/* returns true if end is big endian. */ >> +static inline bool devend_big_endian(enum device_endian end) >> { >> QEMU_BUILD_BUG_ON(DEVICE_HOST_ENDIAN != DEVICE_LITTLE_ENDIAN && >> DEVICE_HOST_ENDIAN != DEVICE_BIG_ENDIAN); >> >> - bool big_endian = (end == DEVICE_NATIVE_ENDIAN >> - ? target_words_bigendian() >> - : end == DEVICE_BIG_ENDIAN); >> - return big_endian ? MO_BE : MO_LE; >> + if (end == DEVICE_NATIVE_ENDIAN) { >> + return target_words_bigendian(); >> + } >> + return end == DEVICE_BIG_ENDIAN; >> +} >> + >> +/* enum device_endian to MemOp. */ >> +static inline MemOp devend_memop(enum device_endian end) >> +{ >> + return devend_big_endian(end) ? MO_BE : MO_LE; >> } >> >> /* >
On 3/10/25 21:08, Pierrick Bouvier wrote: > Defining functions allows to use them from common code, by not depending > on TARGET_BIG_ENDIAN. > Remove previous macros from exec/cpu-all.h. > By moving them out of cpu-all.h, we'll be able to break dependency on > cpu.h for memory related functions coming in next commits. > > Signed-off-by: Pierrick Bouvier<pierrick.bouvier@linaro.org> > --- > include/exec/cpu-all.h | 25 --------------- > include/exec/tswap.h | 70 ++++++++++++++++++++++++++++++++++++++++++ > 2 files changed, 70 insertions(+), 25 deletions(-) Reviewed-by: Richard Henderson <richard.henderson@linaro.org> r~
On 3/10/25 21:08, Pierrick Bouvier wrote: > They are now accessible through exec/memory.h instead, and we make sure > all variants are available for common or target dependent code. > > Move stl_phys_notdirty function as well. > Cached endianness agnostic version rely on st/ld*_p, which is available > through tswap.h. > > Signed-off-by: Pierrick Bouvier<pierrick.bouvier@linaro.org> > --- > include/exec/cpu-all.h | 29 ----------------------------- > include/exec/memory.h | 10 ++++++++++ > include/exec/memory_ldst_phys.h.inc | 5 +---- > 3 files changed, 11 insertions(+), 33 deletions(-) Reviewed-by: Richard Henderson <richard.henderson@linaro.org> r~
On 3/10/25 21:08, Pierrick Bouvier wrote: > we'll use it in system/memory.c. > > Signed-off-by: Pierrick Bouvier<pierrick.bouvier@linaro.org> > --- > include/exec/memory.h | 18 ++++++++++++------ > 1 file changed, 12 insertions(+), 6 deletions(-) Reviewed-by: Richard Henderson <richard.henderson@linaro.org> r~
The main goal of this series is to be able to call any memory ld/st function from code that is *not* target dependent. As a positive side effect, we can turn related system compilation units into common code. The first 5 patches remove dependency of memory API to cpu headers and remove dependency to target specific code. This could be a series on its own, but it's great to be able to turn system memory compilation units into common code to make sure it can't regress, and prove it achieves the desired result. The next patches remove more dependencies on cpu headers (exec-all, memory-internal, ram_addr). Then, we add access to a needed function from kvm, some xen stubs, and we finally can turn our compilation units into common code. Every commit was tested to build correctly for all targets (on windows, linux, macos), and the series was fully tested by running all tests we have (linux, x86_64 host). v2: - reorder first commits (tswap change first, so memory cached functions can use it) - move st/ld*_p functions to tswap instead of bswap - add define for target_words_bigendian when COMPILING_PER_TARGET, equals to TARGET_BIG_ENDIAN (avoid overhead in target code) - rewrite devend_memop - remove useless exec-all.h in concerned patch - extract devend_big_endian function to reuse in system/memory.c - rewrite changes to system/memory.c Pierrick Bouvier (16): exec/tswap: target code can use TARGET_BIG_ENDIAN instead of target_words_bigendian() exec/tswap: implement {ld,st}.*_p as functions instead of macros exec/memory_ldst: extract memory_ldst declarations from cpu-all.h exec/memory_ldst_phys: extract memory_ldst_phys declarations from cpu-all.h exec/memory.h: make devend_memop "target defines" agnostic codebase: prepare to remove cpu.h from exec/exec-all.h exec/exec-all: remove dependency on cpu.h exec/memory-internal: remove dependency on cpu.h exec/ram_addr: remove dependency on cpu.h system/kvm: make kvm_flush_coalesced_mmio_buffer() accessible for common code exec/ram_addr: call xen_hvm_modified_memory only if xen is enabled hw/xen: add stubs for various functions system/physmem: compilation unit is now common to all targets include/exec/memory: extract devend_big_endian from devend_memop system/memory: make compilation unit common system/ioport: make compilation unit common include/exec/cpu-all.h | 66 ----------------------- include/exec/exec-all.h | 1 - include/exec/memory-internal.h | 2 - include/exec/memory.h | 34 +++++++----- include/exec/ram_addr.h | 11 ++-- include/exec/tswap.h | 81 +++++++++++++++++++++++++++-- include/system/kvm.h | 6 +-- include/tcg/tcg-op.h | 1 + target/ppc/helper_regs.h | 2 + include/exec/memory_ldst.h.inc | 4 -- include/exec/memory_ldst_phys.h.inc | 5 +- cpu-target.c | 1 + hw/ppc/spapr_nested.c | 1 + hw/sh4/sh7750.c | 1 + hw/xen/xen_stubs.c | 56 ++++++++++++++++++++ page-vary-target.c | 2 +- system/ioport.c | 1 - system/memory.c | 17 ++---- target/riscv/bitmanip_helper.c | 2 +- hw/xen/meson.build | 3 ++ system/meson.build | 6 +-- 21 files changed, 184 insertions(+), 119 deletions(-) create mode 100644 hw/xen/xen_stubs.c