| Message ID | 20241004162118.84570-3-philmd@linaro.org |
|---|---|
| State | New |
| Headers | show |
| Series | hw/core/cpu: Expose cpu_is_big_endian() method | expand |
On Fri, 4 Oct 2024 at 17:22, Philippe Mathieu-Daudé <philmd@linaro.org> wrote: > > Introduce the CPUClass::is_big_endian() handler and its > common default. > > Signed-off-by: Philippe Mathieu-Daudé <philmd@linaro.org> > --- > include/hw/core/cpu.h | 3 ++- > hw/core/cpu-common.c | 7 +++++++ > 2 files changed, 9 insertions(+), 1 deletion(-) > > diff --git a/include/hw/core/cpu.h b/include/hw/core/cpu.h > index 04e9ad49968..22ef7a44e86 100644 > --- a/include/hw/core/cpu.h > +++ b/include/hw/core/cpu.h > @@ -150,6 +150,7 @@ struct CPUClass { > ObjectClass *(*class_by_name)(const char *cpu_model); > void (*parse_features)(const char *typename, char *str, Error **errp); > > + bool (*is_big_endian)(CPUState *cpu); > bool (*has_work)(CPUState *cpu); > int (*mmu_index)(CPUState *cpu, bool ifetch); > int (*memory_rw_debug)(CPUState *cpu, vaddr addr, What does this actually mean, though? Arm for instance has multiple different kinds of "big-endian" depending on the CPU: both BE32 and BE8, and data-endianness not necessarily being the same as instruction-endianness. This series doesn't introduce any users of this new API. It's hard to say without seeing what the intended use is, but I would expect that probably they would want to be testing something else, depending on what they're trying to do. It's pretty uncommon for anything out in the system to want to know what endianness a runtime-configurable CPU happens to be set to, because in real hardware a device has no way to tell. (This is why cpu_virtio_is_big_endian() is named the way it is -- to discourage anybody from trying to use it outside the virtio devices where we need it for legacy "the spec wasn't written thinking about endianness very hard" reasons.) thanks -- PMM
On 4/10/24 18:41, Peter Maydell wrote: > On Fri, 4 Oct 2024 at 17:22, Philippe Mathieu-Daudé <philmd@linaro.org> wrote: >> >> Introduce the CPUClass::is_big_endian() handler and its >> common default. >> >> Signed-off-by: Philippe Mathieu-Daudé <philmd@linaro.org> >> --- >> include/hw/core/cpu.h | 3 ++- >> hw/core/cpu-common.c | 7 +++++++ >> 2 files changed, 9 insertions(+), 1 deletion(-) >> >> diff --git a/include/hw/core/cpu.h b/include/hw/core/cpu.h >> index 04e9ad49968..22ef7a44e86 100644 >> --- a/include/hw/core/cpu.h >> +++ b/include/hw/core/cpu.h >> @@ -150,6 +150,7 @@ struct CPUClass { >> ObjectClass *(*class_by_name)(const char *cpu_model); >> void (*parse_features)(const char *typename, char *str, Error **errp); >> >> + bool (*is_big_endian)(CPUState *cpu); >> bool (*has_work)(CPUState *cpu); >> int (*mmu_index)(CPUState *cpu, bool ifetch); >> int (*memory_rw_debug)(CPUState *cpu, vaddr addr, > > What does this actually mean, though? Arm for instance > has multiple different kinds of "big-endian" depending > on the CPU: both BE32 and BE8, and data-endianness not > necessarily being the same as instruction-endianness. This is to be used in the data bus (I was wondering whether using 'data' in the method name). > This series doesn't introduce any users of this new API. > It's hard to say without seeing what the intended use is, > but I would expect that probably they would want to be testing > something else, depending on what they're trying to do. I'm trying to split my branch in "~20 patches series"; I should post example of API consumers in a few days. First conversion is the cpu_in/out() API in system/ioport.c, I'll try to post it ASAP so we can discuss there. > It's pretty uncommon for anything out in the system to > want to know what endianness a runtime-configurable CPU > happens to be set to, because in real hardware a device > has no way to tell. (This is why cpu_virtio_is_big_endian() > is named the way it is -- to discourage anybody from trying > to use it outside the virtio devices where we need it for > legacy "the spec wasn't written thinking about endianness > very hard" reasons.) I'm trying to convert implicit knowledge of target endianness to explicit one, propagated as argument from the machine.
On Fri, 4 Oct 2024 at 17:54, Philippe Mathieu-Daudé <philmd@linaro.org> wrote: > > On 4/10/24 18:41, Peter Maydell wrote: > > On Fri, 4 Oct 2024 at 17:22, Philippe Mathieu-Daudé <philmd@linaro.org> wrote: > >> > >> Introduce the CPUClass::is_big_endian() handler and its > >> common default. > >> > >> Signed-off-by: Philippe Mathieu-Daudé <philmd@linaro.org> > >> --- > >> include/hw/core/cpu.h | 3 ++- > >> hw/core/cpu-common.c | 7 +++++++ > >> 2 files changed, 9 insertions(+), 1 deletion(-) > >> > >> diff --git a/include/hw/core/cpu.h b/include/hw/core/cpu.h > >> index 04e9ad49968..22ef7a44e86 100644 > >> --- a/include/hw/core/cpu.h > >> +++ b/include/hw/core/cpu.h > >> @@ -150,6 +150,7 @@ struct CPUClass { > >> ObjectClass *(*class_by_name)(const char *cpu_model); > >> void (*parse_features)(const char *typename, char *str, Error **errp); > >> > >> + bool (*is_big_endian)(CPUState *cpu); > >> bool (*has_work)(CPUState *cpu); > >> int (*mmu_index)(CPUState *cpu, bool ifetch); > >> int (*memory_rw_debug)(CPUState *cpu, vaddr addr, > > > > What does this actually mean, though? Arm for instance > > has multiple different kinds of "big-endian" depending > > on the CPU: both BE32 and BE8, and data-endianness not > > necessarily being the same as instruction-endianness. > > This is to be used in the data bus (I was wondering whether using 'data' > in the method name). That sounds like what you actually want is (a non-compile-time version of) TARGET_ENDIANNESS, which is not related to the CPU's dynamic setting. > > This series doesn't introduce any users of this new API. > > It's hard to say without seeing what the intended use is, > > but I would expect that probably they would want to be testing > > something else, depending on what they're trying to do. > > I'm trying to split my branch in "~20 patches series"; > I should post example of API consumers in a few days. > > First conversion is the cpu_in/out() API in system/ioport.c, > I'll try to post it ASAP so we can discuss there. Yeah, I think we really need to look at the users, because my current feeling is "we don't want this API at all, the answer will always be to use something else". I suspect for cpu_in/out the answer is "this API only makes sense for x86, and whatever extent it's built for anything else is accidental". We could probably define it as always-little-endian. -- PMM
diff --git a/include/hw/core/cpu.h b/include/hw/core/cpu.h index 04e9ad49968..22ef7a44e86 100644 --- a/include/hw/core/cpu.h +++ b/include/hw/core/cpu.h @@ -150,6 +150,7 @@ struct CPUClass { ObjectClass *(*class_by_name)(const char *cpu_model); void (*parse_features)(const char *typename, char *str, Error **errp); + bool (*is_big_endian)(CPUState *cpu); bool (*has_work)(CPUState *cpu); int (*mmu_index)(CPUState *cpu, bool ifetch); int (*memory_rw_debug)(CPUState *cpu, vaddr addr, @@ -749,7 +750,7 @@ int cpu_asidx_from_attrs(CPUState *cpu, MemTxAttrs attrs); */ bool cpu_virtio_is_big_endian(CPUState *cpu); -#endif /* CONFIG_USER_ONLY */ +#endif /* !CONFIG_USER_ONLY */ /** * cpu_list_add: diff --git a/hw/core/cpu-common.c b/hw/core/cpu-common.c index 7982ecd39a5..aa5ea9761e4 100644 --- a/hw/core/cpu-common.c +++ b/hw/core/cpu-common.c @@ -26,6 +26,7 @@ #include "qemu/main-loop.h" #include "exec/log.h" #include "exec/gdbstub.h" +#include "exec/tswap.h" #include "sysemu/tcg.h" #include "hw/boards.h" #include "hw/qdev-properties.h" @@ -138,6 +139,11 @@ static void cpu_common_reset_hold(Object *obj, ResetType type) cpu_exec_reset_hold(cpu); } +static bool cpu_common_is_big_endian(CPUState *cs) +{ + return target_words_bigendian(); +} + static bool cpu_common_has_work(CPUState *cs) { return false; @@ -306,6 +312,7 @@ static void cpu_common_class_init(ObjectClass *klass, void *data) k->parse_features = cpu_common_parse_features; k->get_arch_id = cpu_common_get_arch_id; + k->is_big_endian = cpu_common_is_big_endian; k->has_work = cpu_common_has_work; k->gdb_read_register = cpu_common_gdb_read_register; k->gdb_write_register = cpu_common_gdb_write_register;
Introduce the CPUClass::is_big_endian() handler and its common default. Signed-off-by: Philippe Mathieu-Daudé <philmd@linaro.org> --- include/hw/core/cpu.h | 3 ++- hw/core/cpu-common.c | 7 +++++++ 2 files changed, 9 insertions(+), 1 deletion(-)