| Message ID | 20250310045842.2650784-16-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: > Signed-off-by: Pierrick Bouvier <pierrick.bouvier@linaro.org> > --- > system/memory.c | 22 +++++++++++++++------- > system/meson.build | 2 +- > 2 files changed, 16 insertions(+), 8 deletions(-) > > diff --git a/system/memory.c b/system/memory.c > index 4c829793a0a..b401be8b5f1 100644 > --- a/system/memory.c > +++ b/system/memory.c > @@ -355,11 +355,11 @@ static void flatview_simplify(FlatView *view) > > static bool memory_region_big_endian(MemoryRegion *mr) > { > -#if TARGET_BIG_ENDIAN > - return mr->ops->endianness != DEVICE_LITTLE_ENDIAN; > -#else > - return mr->ops->endianness == DEVICE_BIG_ENDIAN; > -#endif > + if (target_words_bigendian()) { > + return mr->ops->endianness != DEVICE_LITTLE_ENDIAN; > + } else { > + return mr->ops->endianness == DEVICE_BIG_ENDIAN; > + } > } This should use the same expression as for patch 4: return (end == DEVICE_NATIVE_ENDIAN ? target_words_bigendian() : end == DEVICE_BIG_ENDIAN); Which perhaps ought to be split out to it's own inline function? > > static void adjust_endianness(MemoryRegion *mr, uint64_t *data, MemOp op) > @@ -2584,7 +2584,11 @@ void memory_region_add_eventfd(MemoryRegion *mr, > unsigned i; > > if (size) { > - adjust_endianness(mr, &mrfd.data, size_memop(size) | MO_TE); > + if (target_words_bigendian()) { > + adjust_endianness(mr, &mrfd.data, size_memop(size) | MO_BE); > + } else { > + adjust_endianness(mr, &mrfd.data, size_memop(size) | MO_LE); > + } Maybe better as MemOp mop = (target_words_bigendian() ? MO_BE : MO_LE) | size_memop(size); adjust_endianness(mr, &mrfd.data, size_memop(size), mop); r~
On 3/10/25 10:43, Richard Henderson wrote: > On 3/9/25 21:58, Pierrick Bouvier wrote: >> Signed-off-by: Pierrick Bouvier <pierrick.bouvier@linaro.org> >> --- >> system/memory.c | 22 +++++++++++++++------- >> system/meson.build | 2 +- >> 2 files changed, 16 insertions(+), 8 deletions(-) >> >> diff --git a/system/memory.c b/system/memory.c >> index 4c829793a0a..b401be8b5f1 100644 >> --- a/system/memory.c >> +++ b/system/memory.c >> @@ -355,11 +355,11 @@ static void flatview_simplify(FlatView *view) >> >> static bool memory_region_big_endian(MemoryRegion *mr) >> { >> -#if TARGET_BIG_ENDIAN >> - return mr->ops->endianness != DEVICE_LITTLE_ENDIAN; >> -#else >> - return mr->ops->endianness == DEVICE_BIG_ENDIAN; >> -#endif >> + if (target_words_bigendian()) { >> + return mr->ops->endianness != DEVICE_LITTLE_ENDIAN; >> + } else { >> + return mr->ops->endianness == DEVICE_BIG_ENDIAN; >> + } >> } > > This should use the same expression as for patch 4: > > return (end == DEVICE_NATIVE_ENDIAN > ? target_words_bigendian() > : end == DEVICE_BIG_ENDIAN); > > Which perhaps ought to be split out to it's own inline function? > Good point, I'll add this. >> >> static void adjust_endianness(MemoryRegion *mr, uint64_t *data, MemOp op) >> @@ -2584,7 +2584,11 @@ void memory_region_add_eventfd(MemoryRegion *mr, >> unsigned i; >> >> if (size) { >> - adjust_endianness(mr, &mrfd.data, size_memop(size) | MO_TE); >> + if (target_words_bigendian()) { >> + adjust_endianness(mr, &mrfd.data, size_memop(size) | MO_BE); >> + } else { >> + adjust_endianness(mr, &mrfd.data, size_memop(size) | MO_LE); >> + } > > Maybe better as > > MemOp mop = (target_words_bigendian() ? MO_BE : MO_LE) | size_memop(size); > adjust_endianness(mr, &mrfd.data, size_memop(size), mop); > Do you think defining MO_TE as this expression is a good idea? I'm afraid it's a bit too much implicit though, but it would save us from the hassle to change a lot of code using MO_BE, MO_LE (and all other variants defined in MemOp enum). > > r~
On 3/10/25 10:47, Pierrick Bouvier wrote: >> Maybe better as >> >> MemOp mop = (target_words_bigendian() ? MO_BE : MO_LE) | size_memop(size); >> adjust_endianness(mr, &mrfd.data, size_memop(size), mop); >> > > Do you think defining MO_TE as this expression is a good idea? There are not so many references to MO_TE outside target/ or accel/tcg/. Indeed, after this change, the only ones left are in hw/arm/armv7m.c, which (because it's Arm) can be changed to MO_LE. r~
On 3/10/25 10:58, Richard Henderson wrote: > On 3/10/25 10:47, Pierrick Bouvier wrote: >>> Maybe better as >>> >>> MemOp mop = (target_words_bigendian() ? MO_BE : MO_LE) | size_memop(size); >>> adjust_endianness(mr, &mrfd.data, size_memop(size), mop); >>> >> >> Do you think defining MO_TE as this expression is a good idea? > > There are not so many references to MO_TE outside target/ or accel/tcg/. > > Indeed, after this change, the only ones left are in hw/arm/armv7m.c, > which (because it's Arm) can be changed to MO_LE. > I see a bit more than that (17 files): hw/arm/armv7m.c include/exec/memop.h target/arm/tcg/helper-a64.c target/arm/tcg/translate.c target/hexagon/idef-parser/parser-helpers.c target/hppa/translate.c target/i386/tcg/emit.c.inc target/loongarch/tcg/insn_trans/trans_vec.c.inc target/m68k/translate.c target/mips/tcg/mips16e_translate.c.inc target/riscv/translate.c target/rx/translate.c target/s390x/tcg/mem_helper.c target/s390x/tcg/translate.c target/s390x/tcg/translate_vx.c.inc target/sparc/ldst_helper.c target/sparc/translate.c Plus more (22 files) who relies on: MO_TE* variants (which relies on MO_TE transitively) Thus my proposal to have a first change to MO_TE definition, and eventually do the change later. What do you think? > > r~
On 3/10/25 11:04, Pierrick Bouvier wrote: > On 3/10/25 10:58, Richard Henderson wrote: >> On 3/10/25 10:47, Pierrick Bouvier wrote: >>>> Maybe better as >>>> >>>> MemOp mop = (target_words_bigendian() ? MO_BE : MO_LE) | size_memop(size); >>>> adjust_endianness(mr, &mrfd.data, size_memop(size), mop); >>>> >>> >>> Do you think defining MO_TE as this expression is a good idea? >> >> There are not so many references to MO_TE outside target/ or accel/tcg/. >> >> Indeed, after this change, the only ones left are in hw/arm/armv7m.c, >> which (because it's Arm) can be changed to MO_LE. >> > > I see a bit more than that (17 files): > hw/arm/armv7m.c > include/exec/memop.h > target/arm/tcg/helper-a64.c > target/arm/tcg/translate.c > target/hexagon/idef-parser/parser-helpers.c > target/hppa/translate.c > target/i386/tcg/emit.c.inc > target/loongarch/tcg/insn_trans/trans_vec.c.inc > target/m68k/translate.c > target/mips/tcg/mips16e_translate.c.inc > target/riscv/translate.c > target/rx/translate.c > target/s390x/tcg/mem_helper.c > target/s390x/tcg/translate.c > target/s390x/tcg/translate_vx.c.inc > target/sparc/ldst_helper.c > target/sparc/translate.c > > Plus more (22 files) who relies on: > MO_TE* variants (which relies on MO_TE transitively) As I said, *outside* target/ and accel/tcg/. > Thus my proposal to have a first change to MO_TE definition, and eventually do the change > later. > > What do you think? I don't think a change to MO_TE is necessary. r~
On 3/10/25 11:10, Richard Henderson wrote: > On 3/10/25 11:04, Pierrick Bouvier wrote: >> On 3/10/25 10:58, Richard Henderson wrote: >>> On 3/10/25 10:47, Pierrick Bouvier wrote: >>>>> Maybe better as >>>>> >>>>> MemOp mop = (target_words_bigendian() ? MO_BE : MO_LE) | size_memop(size); >>>>> adjust_endianness(mr, &mrfd.data, size_memop(size), mop); >>>>> >>>> >>>> Do you think defining MO_TE as this expression is a good idea? >>> >>> There are not so many references to MO_TE outside target/ or accel/tcg/. >>> >>> Indeed, after this change, the only ones left are in hw/arm/armv7m.c, >>> which (because it's Arm) can be changed to MO_LE. >>> >> >> I see a bit more than that (17 files): >> hw/arm/armv7m.c >> include/exec/memop.h >> target/arm/tcg/helper-a64.c >> target/arm/tcg/translate.c >> target/hexagon/idef-parser/parser-helpers.c >> target/hppa/translate.c >> target/i386/tcg/emit.c.inc >> target/loongarch/tcg/insn_trans/trans_vec.c.inc >> target/m68k/translate.c >> target/mips/tcg/mips16e_translate.c.inc >> target/riscv/translate.c >> target/rx/translate.c >> target/s390x/tcg/mem_helper.c >> target/s390x/tcg/translate.c >> target/s390x/tcg/translate_vx.c.inc >> target/sparc/ldst_helper.c >> target/sparc/translate.c >> >> Plus more (22 files) who relies on: >> MO_TE* variants (which relies on MO_TE transitively) > > As I said, *outside* target/ and accel/tcg/. > Got it. We can replace the hw/arm entry when tackling this part later. >> Thus my proposal to have a first change to MO_TE definition, and eventually do the change >> later. >> >> What do you think? > > I don't think a change to MO_TE is necessary. > > > r~
On 10/3/25 19:04, Pierrick Bouvier wrote: > On 3/10/25 10:58, Richard Henderson wrote: >> On 3/10/25 10:47, Pierrick Bouvier wrote: >>>> Maybe better as >>>> >>>> MemOp mop = (target_words_bigendian() ? MO_BE : MO_LE) | >>>> size_memop(size); >>>> adjust_endianness(mr, &mrfd.data, size_memop(size), mop); >>>> >>> >>> Do you think defining MO_TE as this expression is a good idea? >> >> There are not so many references to MO_TE outside target/ or accel/tcg/. >> >> Indeed, after this change, the only ones left are in hw/arm/armv7m.c, >> which (because it's Arm) can be changed to MO_LE. >> > > I see a bit more than that (17 files): > hw/arm/armv7m.c > include/exec/memop.h > target/arm/tcg/helper-a64.c > target/arm/tcg/translate.c > target/hexagon/idef-parser/parser-helpers.c > target/hppa/translate.c > target/i386/tcg/emit.c.inc > target/loongarch/tcg/insn_trans/trans_vec.c.inc > target/m68k/translate.c > target/mips/tcg/mips16e_translate.c.inc > target/riscv/translate.c > target/rx/translate.c > target/s390x/tcg/mem_helper.c > target/s390x/tcg/translate.c > target/s390x/tcg/translate_vx.c.inc > target/sparc/ldst_helper.c > target/sparc/translate.c For targets tied to single endianness, we can replace using gsed, but using a helper is clearer (see for example commit 415aae543ed target/microblaze: Consider endianness while translating code"). > Plus more (22 files) who relies on: > MO_TE* variants (which relies on MO_TE transitively) > > Thus my proposal to have a first change to MO_TE definition, and > eventually do the change later. > > What do you think? Removing MO_TE is in my TODO list. I started with Microblaze (now merged) to get familiar, then had a look at ARM (see i.e. https://lore.kernel.org/qemu-devel/20240924191932.49386-1-philmd@linaro.org/ and https://lore.kernel.org/qemu-devel/d831600a-9a61-45c1-a535-f75bb64cdff4@linaro.org/). I also took care of MIPS few years ago but I need to rebase, however it isn't in the priority list.
On 3/10/25 11:27, Philippe Mathieu-Daudé wrote: > On 10/3/25 19:04, Pierrick Bouvier wrote: >> On 3/10/25 10:58, Richard Henderson wrote: >>> On 3/10/25 10:47, Pierrick Bouvier wrote: >>>>> Maybe better as >>>>> >>>>> MemOp mop = (target_words_bigendian() ? MO_BE : MO_LE) | >>>>> size_memop(size); >>>>> adjust_endianness(mr, &mrfd.data, size_memop(size), mop); >>>>> >>>> >>>> Do you think defining MO_TE as this expression is a good idea? >>> >>> There are not so many references to MO_TE outside target/ or accel/tcg/. >>> >>> Indeed, after this change, the only ones left are in hw/arm/armv7m.c, >>> which (because it's Arm) can be changed to MO_LE. >>> >> >> I see a bit more than that (17 files): >> hw/arm/armv7m.c >> include/exec/memop.h >> target/arm/tcg/helper-a64.c >> target/arm/tcg/translate.c >> target/hexagon/idef-parser/parser-helpers.c >> target/hppa/translate.c >> target/i386/tcg/emit.c.inc >> target/loongarch/tcg/insn_trans/trans_vec.c.inc >> target/m68k/translate.c >> target/mips/tcg/mips16e_translate.c.inc >> target/riscv/translate.c >> target/rx/translate.c >> target/s390x/tcg/mem_helper.c >> target/s390x/tcg/translate.c >> target/s390x/tcg/translate_vx.c.inc >> target/sparc/ldst_helper.c >> target/sparc/translate.c > > For targets tied to single endianness, we can replace using gsed, > but using a helper is clearer (see for example commit 415aae543ed > target/microblaze: Consider endianness while translating code"). > That's good, I just want to keep this series focus on minimal changes to achieve the current goal, and not bring any additional refactoring here. >> Plus more (22 files) who relies on: >> MO_TE* variants (which relies on MO_TE transitively) >> >> Thus my proposal to have a first change to MO_TE definition, and >> eventually do the change later. >> >> What do you think? > > Removing MO_TE is in my TODO list. > > I started with Microblaze (now merged) to get familiar, then had > a look at ARM (see i.e. > https://lore.kernel.org/qemu-devel/20240924191932.49386-1-philmd@linaro.org/ > and > https://lore.kernel.org/qemu-devel/d831600a-9a61-45c1-a535-f75bb64cdff4@linaro.org/). > I also took care of MIPS few years ago but I need to rebase, > however it isn't in the priority list. Instead of doing a full codebase refactoring/cleanup, we can adopt a "as needed basis" approach. Basically architectures that can have varying endianness must be handled (since their compilation units are duplicated for variants). For the rest, as Richard mentioned on this series, the code will stay target specific, compiling with a single set of defines, which is what we really aim for. Same discussion will happen for architectures with files duplicated between 32/64 bits variants.
diff --git a/system/memory.c b/system/memory.c index 4c829793a0a..b401be8b5f1 100644 --- a/system/memory.c +++ b/system/memory.c @@ -355,11 +355,11 @@ static void flatview_simplify(FlatView *view) static bool memory_region_big_endian(MemoryRegion *mr) { -#if TARGET_BIG_ENDIAN - return mr->ops->endianness != DEVICE_LITTLE_ENDIAN; -#else - return mr->ops->endianness == DEVICE_BIG_ENDIAN; -#endif + if (target_words_bigendian()) { + return mr->ops->endianness != DEVICE_LITTLE_ENDIAN; + } else { + return mr->ops->endianness == DEVICE_BIG_ENDIAN; + } } static void adjust_endianness(MemoryRegion *mr, uint64_t *data, MemOp op) @@ -2584,7 +2584,11 @@ void memory_region_add_eventfd(MemoryRegion *mr, unsigned i; if (size) { - adjust_endianness(mr, &mrfd.data, size_memop(size) | MO_TE); + if (target_words_bigendian()) { + adjust_endianness(mr, &mrfd.data, size_memop(size) | MO_BE); + } else { + adjust_endianness(mr, &mrfd.data, size_memop(size) | MO_LE); + } } memory_region_transaction_begin(); for (i = 0; i < mr->ioeventfd_nb; ++i) { @@ -2619,7 +2623,11 @@ void memory_region_del_eventfd(MemoryRegion *mr, unsigned i; if (size) { - adjust_endianness(mr, &mrfd.data, size_memop(size) | MO_TE); + if (target_words_bigendian()) { + adjust_endianness(mr, &mrfd.data, size_memop(size) | MO_BE); + } else { + adjust_endianness(mr, &mrfd.data, size_memop(size) | MO_LE); + } } memory_region_transaction_begin(); for (i = 0; i < mr->ioeventfd_nb; ++i) { diff --git a/system/meson.build b/system/meson.build index 9d0b0122e54..881cb2736fe 100644 --- a/system/meson.build +++ b/system/meson.build @@ -1,7 +1,6 @@ specific_ss.add(when: 'CONFIG_SYSTEM_ONLY', if_true: [files( 'arch_init.c', 'ioport.c', - 'memory.c', )]) system_ss.add(files( @@ -14,6 +13,7 @@ system_ss.add(files( 'dma-helpers.c', 'globals.c', 'memory_mapping.c', + 'memory.c', 'physmem.c', 'qdev-monitor.c', 'qtest.c',
Signed-off-by: Pierrick Bouvier <pierrick.bouvier@linaro.org> --- system/memory.c | 22 +++++++++++++++------- system/meson.build | 2 +- 2 files changed, 16 insertions(+), 8 deletions(-)