| Message ID | 20250310045842.2650784-2-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: > They are now accessible through exec/memory.h instead, and we make sure > all variants are available for common or target dependent code. ... > diff --git a/include/exec/memory_ldst.h.inc b/include/exec/memory_ldst.h.inc > index 92ad74e9560..74519a88de0 100644 > --- a/include/exec/memory_ldst.h.inc > +++ b/include/exec/memory_ldst.h.inc > @@ -19,7 +19,8 @@ > * License along with this library; if not, see <http://www.gnu.org/licenses/>. > */ > > -#ifdef TARGET_ENDIANNESS > +uint8_t glue(address_space_ldub, SUFFIX)(ARG1_DECL, > + hwaddr addr, MemTxAttrs attrs, MemTxResult *result); > uint16_t glue(address_space_lduw, SUFFIX)(ARG1_DECL, > hwaddr addr, MemTxAttrs attrs, MemTxResult *result); You shouldn't be exposing address_space_lduw to common code, only address_space_lduw_be address_space_lduw_le etc. I'm not sure what you're trying to do here. r~
On 3/10/25 08:17, Richard Henderson wrote: > On 3/9/25 21:58, 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. > ... >> diff --git a/include/exec/memory_ldst.h.inc b/include/exec/memory_ldst.h.inc >> index 92ad74e9560..74519a88de0 100644 >> --- a/include/exec/memory_ldst.h.inc >> +++ b/include/exec/memory_ldst.h.inc >> @@ -19,7 +19,8 @@ >> * License along with this library; if not, see <http://www.gnu.org/licenses/>. >> */ >> >> -#ifdef TARGET_ENDIANNESS >> +uint8_t glue(address_space_ldub, SUFFIX)(ARG1_DECL, >> + hwaddr addr, MemTxAttrs attrs, MemTxResult *result); >> uint16_t glue(address_space_lduw, SUFFIX)(ARG1_DECL, >> hwaddr addr, MemTxAttrs attrs, MemTxResult *result); > > You shouldn't be exposing > > address_space_lduw > > to common code, only > > address_space_lduw_be > address_space_lduw_le > > etc. I'm not sure what you're trying to do here. > > > r~ As mentioned in the cover letter, the goal is to extract memory functions from cpu headers. The result is that we need to expose all functions for both common and target dependent code. In case you don't see the point, you can try to remove memory_ldst include from cpu-all.h (without touching memory_ldst.h) and see the compilation errors. This patch is the minimal change to get something working. If your point is that non be/le variants should be eliminated completely, yes, it can be done after this series.
On 3/10/25 08:17, Richard Henderson wrote: > On 3/9/25 21:58, 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. > ... >> diff --git a/include/exec/memory_ldst.h.inc b/include/exec/memory_ldst.h.inc >> index 92ad74e9560..74519a88de0 100644 >> --- a/include/exec/memory_ldst.h.inc >> +++ b/include/exec/memory_ldst.h.inc >> @@ -19,7 +19,8 @@ >> * License along with this library; if not, see <http://www.gnu.org/licenses/>. >> */ >> >> -#ifdef TARGET_ENDIANNESS >> +uint8_t glue(address_space_ldub, SUFFIX)(ARG1_DECL, >> + hwaddr addr, MemTxAttrs attrs, MemTxResult *result); >> uint16_t glue(address_space_lduw, SUFFIX)(ARG1_DECL, >> hwaddr addr, MemTxAttrs attrs, MemTxResult *result); > > You shouldn't be exposing > > address_space_lduw > > to common code, only > > address_space_lduw_be > address_space_lduw_le > > etc. I'm not sure what you're trying to do here. > > > r~ Taking another look at this, I don't see how we can avoid to expose those functions to common code. The goal of those first two patches is to break the dependency to cpu.h, which can't be included from common code, and thus prevent any common code wanting to do memory op to access it. All the calls to ld*_phys() and address_space_*() are indeed done from code related to a target (some of it in hw/). However, as we now move those compilation units to common, they still need to be accessed. The semantic stays exactly the same. From what I understand, non endian versions are simply passing DEVICE_NATIVE_ENDIAN as a parameter for address_space_ldl_internal, which will thus match the target endianness. So what is the risk for common code to call this?
On 3/10/25 17:04, Pierrick Bouvier wrote: > From what I understand, non endian versions are simply passing DEVICE_NATIVE_ENDIAN as a > parameter for address_space_ldl_internal, which will thus match the target endianness. > > So what is the risk for common code to call this? You're right. I failed to look at the current implementation to see that it would already work. Reviewed-by: Richard Henderson <richard.henderson@linaro.org> r~
diff --git a/include/exec/cpu-all.h b/include/exec/cpu-all.h index 8cd6c00cf89..17ea82518a0 100644 --- a/include/exec/cpu-all.h +++ b/include/exec/cpu-all.h @@ -69,18 +69,6 @@ #include "exec/hwaddr.h" -#define SUFFIX -#define ARG1 as -#define ARG1_DECL AddressSpace *as -#define TARGET_ENDIANNESS -#include "exec/memory_ldst.h.inc" - -#define SUFFIX _cached_slow -#define ARG1 cache -#define ARG1_DECL MemoryRegionCache *cache -#define TARGET_ENDIANNESS -#include "exec/memory_ldst.h.inc" - static inline void stl_phys_notdirty(AddressSpace *as, hwaddr addr, uint32_t val) { address_space_stl_notdirty(as, addr, val, diff --git a/include/exec/memory_ldst.h.inc b/include/exec/memory_ldst.h.inc index 92ad74e9560..74519a88de0 100644 --- a/include/exec/memory_ldst.h.inc +++ b/include/exec/memory_ldst.h.inc @@ -19,7 +19,8 @@ * License along with this library; if not, see <http://www.gnu.org/licenses/>. */ -#ifdef TARGET_ENDIANNESS +uint8_t glue(address_space_ldub, SUFFIX)(ARG1_DECL, + hwaddr addr, MemTxAttrs attrs, MemTxResult *result); uint16_t glue(address_space_lduw, SUFFIX)(ARG1_DECL, hwaddr addr, MemTxAttrs attrs, MemTxResult *result); uint32_t glue(address_space_ldl, SUFFIX)(ARG1_DECL, @@ -28,15 +29,15 @@ uint64_t glue(address_space_ldq, SUFFIX)(ARG1_DECL, hwaddr addr, MemTxAttrs attrs, MemTxResult *result); void glue(address_space_stl_notdirty, SUFFIX)(ARG1_DECL, hwaddr addr, uint32_t val, MemTxAttrs attrs, MemTxResult *result); +void glue(address_space_stb, SUFFIX)(ARG1_DECL, + hwaddr addr, uint8_t val, MemTxAttrs attrs, MemTxResult *result); void glue(address_space_stw, SUFFIX)(ARG1_DECL, hwaddr addr, uint16_t val, MemTxAttrs attrs, MemTxResult *result); void glue(address_space_stl, SUFFIX)(ARG1_DECL, hwaddr addr, uint32_t val, MemTxAttrs attrs, MemTxResult *result); void glue(address_space_stq, SUFFIX)(ARG1_DECL, hwaddr addr, uint64_t val, MemTxAttrs attrs, MemTxResult *result); -#else -uint8_t glue(address_space_ldub, SUFFIX)(ARG1_DECL, - hwaddr addr, MemTxAttrs attrs, MemTxResult *result); + uint16_t glue(address_space_lduw_le, SUFFIX)(ARG1_DECL, hwaddr addr, MemTxAttrs attrs, MemTxResult *result); uint16_t glue(address_space_lduw_be, SUFFIX)(ARG1_DECL, @@ -49,8 +50,6 @@ uint64_t glue(address_space_ldq_le, SUFFIX)(ARG1_DECL, hwaddr addr, MemTxAttrs attrs, MemTxResult *result); uint64_t glue(address_space_ldq_be, SUFFIX)(ARG1_DECL, hwaddr addr, MemTxAttrs attrs, MemTxResult *result); -void glue(address_space_stb, SUFFIX)(ARG1_DECL, - hwaddr addr, uint8_t val, MemTxAttrs attrs, MemTxResult *result); void glue(address_space_stw_le, SUFFIX)(ARG1_DECL, hwaddr addr, uint16_t val, MemTxAttrs attrs, MemTxResult *result); void glue(address_space_stw_be, SUFFIX)(ARG1_DECL, @@ -63,9 +62,7 @@ void glue(address_space_stq_le, SUFFIX)(ARG1_DECL, hwaddr addr, uint64_t val, MemTxAttrs attrs, MemTxResult *result); void glue(address_space_stq_be, SUFFIX)(ARG1_DECL, hwaddr addr, uint64_t val, MemTxAttrs attrs, MemTxResult *result); -#endif #undef ARG1_DECL #undef ARG1 #undef SUFFIX -#undef TARGET_ENDIANNESS
They are now accessible through exec/memory.h instead, and we make sure all variants are available for common or target dependent code. Signed-off-by: Pierrick Bouvier <pierrick.bouvier@linaro.org> --- include/exec/cpu-all.h | 12 ------------ include/exec/memory_ldst.h.inc | 13 +++++-------- 2 files changed, 5 insertions(+), 20 deletions(-)