[PATCH v2 00/10] disas: Have CPUClass::disas_set_info() callback set the endianness

[PATCH v2 00/10] disas: Have CPUClass::disas_set_info() callback set the endianness

[Philippe Mathieu-Daudé]

Missing review: patch #3

Since v1:
- Addressed Thomas & Richard comments

Targets are aware of their endianness. No need for a global
target_words_bigendian() call in disas/ where we call the
CPUClass::disas_set_info() handler which already update
disassemble_info fields. Specify the target endianness in
each CPUClass handler.

Philippe Mathieu-Daudé (10):
  target: Set disassemble_info::endian value for little-endian targets
  target: Set disassemble_info::endian value for big-endian targets
  target/arm: Set disassemble_info::endian value in disas_set_info()
  target/microblaze: Set disassemble_info::endian value in
    disas_set_info
  target/mips: Set disassemble_info::endian value in disas_set_info()
  target/ppc: Set disassemble_info::endian value in disas_set_info()
  target/riscv: Set disassemble_info::endian value in disas_set_info()
  target/sh4: Set disassemble_info::endian value in disas_set_info()
  target/xtensa: Set disassemble_info::endian value in disas_set_info()
  disas: Remove target_words_bigendian() call in
    initialize_debug_target()

 disas/disas-common.c    | 13 ++++---------
 target/alpha/cpu.c      |  1 +
 target/arm/cpu.c        | 10 +++-------
 target/avr/cpu.c        |  1 +
 target/hexagon/cpu.c    |  1 +
 target/hppa/cpu.c       |  1 +
 target/i386/cpu.c       |  1 +
 target/loongarch/cpu.c  |  1 +
 target/m68k/cpu.c       |  1 +
 target/microblaze/cpu.c |  1 +
 target/mips/cpu.c       |  3 +++
 target/openrisc/cpu.c   |  1 +
 target/ppc/cpu_init.c   |  2 ++
 target/riscv/cpu.c      |  9 +++++++++
 target/rx/cpu.c         |  1 +
 target/s390x/cpu.c      |  1 +
 target/sh4/cpu.c        |  5 +++++
 target/sparc/cpu.c      |  1 +
 target/tricore/cpu.c    |  6 ++++++
 target/xtensa/cpu.c     |  5 +++++
 20 files changed, 49 insertions(+), 16 deletions(-)

-- 
2.47.1

[PATCH v2 01/10] target: Set disassemble_info::endian value for little-endian targets

[Philippe Mathieu-Daudé]

Have the CPUClass::disas_set_info() callback set the
disassemble_info::endian field for little-endian targets.

Note, there was no disas_set_info() handler registered
for the TriCore target, so we implement one.

Signed-off-by: Philippe Mathieu-Daudé <philmd@linaro.org>
Reviewed-by: Thomas Huth <thuth@redhat.com>
---
 target/alpha/cpu.c     | 1 +
 target/avr/cpu.c       | 1 +
 target/hexagon/cpu.c   | 1 +
 target/i386/cpu.c      | 1 +
 target/loongarch/cpu.c | 1 +
 target/rx/cpu.c        | 1 +
 target/tricore/cpu.c   | 6 ++++++
 7 files changed, 12 insertions(+)

diff --git a/target/alpha/cpu.c b/target/alpha/cpu.c
index da21f99a6ac..acf81fda371 100644
--- a/target/alpha/cpu.c
+++ b/target/alpha/cpu.c
@@ -85,6 +85,7 @@ static int alpha_cpu_mmu_index(CPUState *cs, bool ifetch)
 
 static void alpha_cpu_disas_set_info(CPUState *cpu, disassemble_info *info)
 {
+    info->endian = BFD_ENDIAN_LITTLE;
     info->mach = bfd_mach_alpha_ev6;
     info->print_insn = print_insn_alpha;
 }
diff --git a/target/avr/cpu.c b/target/avr/cpu.c
index 5a0e21465e5..2871d30540a 100644
--- a/target/avr/cpu.c
+++ b/target/avr/cpu.c
@@ -102,6 +102,7 @@ static void avr_cpu_reset_hold(Object *obj, ResetType type)
 
 static void avr_cpu_disas_set_info(CPUState *cpu, disassemble_info *info)
 {
+    info->endian = BFD_ENDIAN_LITTLE;
     info->mach = bfd_arch_avr;
     info->print_insn = avr_print_insn;
 }
diff --git a/target/hexagon/cpu.c b/target/hexagon/cpu.c
index 238e63bcea4..a9beb9a1757 100644
--- a/target/hexagon/cpu.c
+++ b/target/hexagon/cpu.c
@@ -293,6 +293,7 @@ static void hexagon_cpu_reset_hold(Object *obj, ResetType type)
 static void hexagon_cpu_disas_set_info(CPUState *s, disassemble_info *info)
 {
     info->print_insn = print_insn_hexagon;
+    info->endian = BFD_ENDIAN_LITTLE;
 }
 
 static void hexagon_cpu_realize(DeviceState *dev, Error **errp)
diff --git a/target/i386/cpu.c b/target/i386/cpu.c
index b5dd60d2812..85815c0805d 100644
--- a/target/i386/cpu.c
+++ b/target/i386/cpu.c
@@ -8497,6 +8497,7 @@ static void x86_disas_set_info(CPUState *cs, disassemble_info *info)
     X86CPU *cpu = X86_CPU(cs);
     CPUX86State *env = &cpu->env;
 
+    info->endian = BFD_ENDIAN_LITTLE;
     info->mach = (env->hflags & HF_CS64_MASK ? bfd_mach_x86_64
                   : env->hflags & HF_CS32_MASK ? bfd_mach_i386_i386
                   : bfd_mach_i386_i8086);
diff --git a/target/loongarch/cpu.c b/target/loongarch/cpu.c
index 227870e2856..cb9b9f909f3 100644
--- a/target/loongarch/cpu.c
+++ b/target/loongarch/cpu.c
@@ -617,6 +617,7 @@ static void loongarch_cpu_reset_hold(Object *obj, ResetType type)
 
 static void loongarch_cpu_disas_set_info(CPUState *s, disassemble_info *info)
 {
+    info->endian = BFD_ENDIAN_LITTLE;
     info->print_insn = print_insn_loongarch;
 }
 
diff --git a/target/rx/cpu.c b/target/rx/cpu.c
index 154906ef5f4..acd5a6e12da 100644
--- a/target/rx/cpu.c
+++ b/target/rx/cpu.c
@@ -160,6 +160,7 @@ static void rx_cpu_set_irq(void *opaque, int no, int request)
 
 static void rx_cpu_disas_set_info(CPUState *cpu, disassemble_info *info)
 {
+    info->endian = BFD_ENDIAN_LITTLE;
     info->mach = bfd_mach_rx;
     info->print_insn = print_insn_rx;
 }
diff --git a/target/tricore/cpu.c b/target/tricore/cpu.c
index eb794674c8d..49c18a0cd92 100644
--- a/target/tricore/cpu.c
+++ b/target/tricore/cpu.c
@@ -35,6 +35,11 @@ static const gchar *tricore_gdb_arch_name(CPUState *cs)
     return "tricore";
 }
 
+static void tricore_cpu_disas_set_info(CPUState *cpu, disassemble_info *info)
+{
+    info->endian = BFD_ENDIAN_LITTLE;
+}
+
 static void tricore_cpu_set_pc(CPUState *cs, vaddr value)
 {
     cpu_env(cs)->PC = value & ~(target_ulong)1;
@@ -201,6 +206,7 @@ static void tricore_cpu_class_init(ObjectClass *c, void *data)
     cc->gdb_num_core_regs = 44;
     cc->gdb_arch_name = tricore_gdb_arch_name;
 
+    cc->disas_set_info = tricore_cpu_disas_set_info;
     cc->dump_state = tricore_cpu_dump_state;
     cc->set_pc = tricore_cpu_set_pc;
     cc->get_pc = tricore_cpu_get_pc;
-- 
2.47.1

[PATCH v2 02/10] target: Set disassemble_info::endian value for big-endian targets

[Philippe Mathieu-Daudé]

Have the CPUClass::disas_set_info() callback set the
disassemble_info::endian field for big-endian targets.

Signed-off-by: Philippe Mathieu-Daudé <philmd@linaro.org>
Reviewed-by: Thomas Huth <thuth@redhat.com>
---
 target/hppa/cpu.c     | 1 +
 target/m68k/cpu.c     | 1 +
 target/openrisc/cpu.c | 1 +
 target/s390x/cpu.c    | 1 +
 target/sparc/cpu.c    | 1 +
 5 files changed, 5 insertions(+)

diff --git a/target/hppa/cpu.c b/target/hppa/cpu.c
index 4bb5cff624e..d15f8c9c217 100644
--- a/target/hppa/cpu.c
+++ b/target/hppa/cpu.c
@@ -150,6 +150,7 @@ static int hppa_cpu_mmu_index(CPUState *cs, bool ifetch)
 static void hppa_cpu_disas_set_info(CPUState *cs, disassemble_info *info)
 {
     info->mach = bfd_mach_hppa20;
+    info->endian = BFD_ENDIAN_BIG;
     info->print_insn = print_insn_hppa;
 }
 
diff --git a/target/m68k/cpu.c b/target/m68k/cpu.c
index 5eac4a38c62..ff167aaea71 100644
--- a/target/m68k/cpu.c
+++ b/target/m68k/cpu.c
@@ -122,6 +122,7 @@ static void m68k_cpu_reset_hold(Object *obj, ResetType type)
 static void m68k_cpu_disas_set_info(CPUState *s, disassemble_info *info)
 {
     info->print_insn = print_insn_m68k;
+    info->endian = BFD_ENDIAN_BIG;
     info->mach = 0;
 }
 
diff --git a/target/openrisc/cpu.c b/target/openrisc/cpu.c
index a74fab43a91..33c81928370 100644
--- a/target/openrisc/cpu.c
+++ b/target/openrisc/cpu.c
@@ -83,6 +83,7 @@ static int openrisc_cpu_mmu_index(CPUState *cs, bool ifetch)
 
 static void openrisc_disas_set_info(CPUState *cpu, disassemble_info *info)
 {
+    info->endian = BFD_ENDIAN_BIG;
     info->print_insn = print_insn_or1k;
 }
 
diff --git a/target/s390x/cpu.c b/target/s390x/cpu.c
index 3bea014f9ee..972d265478d 100644
--- a/target/s390x/cpu.c
+++ b/target/s390x/cpu.c
@@ -243,6 +243,7 @@ static void s390_cpu_disas_set_info(CPUState *cpu, disassemble_info *info)
 {
     info->mach = bfd_mach_s390_64;
     info->cap_arch = CS_ARCH_SYSZ;
+    info->endian = BFD_ENDIAN_BIG;
     info->cap_insn_unit = 2;
     info->cap_insn_split = 6;
 }
diff --git a/target/sparc/cpu.c b/target/sparc/cpu.c
index e3b46137178..9fd222e4c82 100644
--- a/target/sparc/cpu.c
+++ b/target/sparc/cpu.c
@@ -106,6 +106,7 @@ static bool sparc_cpu_exec_interrupt(CPUState *cs, int interrupt_request)
 static void cpu_sparc_disas_set_info(CPUState *cpu, disassemble_info *info)
 {
     info->print_insn = print_insn_sparc;
+    info->endian = BFD_ENDIAN_BIG;
 #ifdef TARGET_SPARC64
     info->mach = bfd_mach_sparc_v9b;
 #endif
-- 
2.47.1

[PATCH v2 03/10] target/arm: Set disassemble_info::endian value in disas_set_info()

[Philippe Mathieu-Daudé]

Have the CPUClass::disas_set_info() callback set the
disassemble_info::endian field.

Signed-off-by: Philippe Mathieu-Daudé <philmd@linaro.org>
---
 target/arm/cpu.c | 10 +++-------
 1 file changed, 3 insertions(+), 7 deletions(-)

diff --git a/target/arm/cpu.c b/target/arm/cpu.c
index 94f1c55622b..68b3a9d3ab0 100644
--- a/target/arm/cpu.c
+++ b/target/arm/cpu.c
@@ -1188,7 +1188,7 @@ static void arm_disas_set_info(CPUState *cpu, disassemble_info *info)
 {
     ARMCPU *ac = ARM_CPU(cpu);
     CPUARMState *env = &ac->env;
-    bool sctlr_b;
+    bool sctlr_b = arm_sctlr_b(env);
 
     if (is_a64(env)) {
         info->cap_arch = CS_ARCH_ARM64;
@@ -1215,13 +1215,9 @@ static void arm_disas_set_info(CPUState *cpu, disassemble_info *info)
         info->cap_mode = cap_mode;
     }
 
-    sctlr_b = arm_sctlr_b(env);
+    info->endian = BFD_ENDIAN_LITTLE;
     if (bswap_code(sctlr_b)) {
-#if TARGET_BIG_ENDIAN
-        info->endian = BFD_ENDIAN_LITTLE;
-#else
-        info->endian = BFD_ENDIAN_BIG;
-#endif
+        info->endian = TARGET_BIG_ENDIAN ? BFD_ENDIAN_LITTLE : BFD_ENDIAN_BIG;
     }
     info->flags &= ~INSN_ARM_BE32;
 #ifndef CONFIG_USER_ONLY
-- 
2.47.1

[PATCH v2 04/10] target/microblaze: Set disassemble_info::endian value in disas_set_info

[Philippe Mathieu-Daudé]

Have the CPUClass::disas_set_info() callback set the
disassemble_info::endian field.

Signed-off-by: Philippe Mathieu-Daudé <philmd@linaro.org>
Reviewed-by: Thomas Huth <thuth@redhat.com>
---
 target/microblaze/cpu.c | 1 +
 1 file changed, 1 insertion(+)

diff --git a/target/microblaze/cpu.c b/target/microblaze/cpu.c
index 13d194cef88..27089e3c579 100644
--- a/target/microblaze/cpu.c
+++ b/target/microblaze/cpu.c
@@ -224,6 +224,7 @@ static void mb_disas_set_info(CPUState *cpu, disassemble_info *info)
 {
     info->mach = bfd_arch_microblaze;
     info->print_insn = print_insn_microblaze;
+    info->endian = TARGET_BIG_ENDIAN ? BFD_ENDIAN_BIG : BFD_ENDIAN_LITTLE;
 }
 
 static void mb_cpu_realizefn(DeviceState *dev, Error **errp)
-- 
2.47.1

[PATCH v2 05/10] target/mips: Set disassemble_info::endian value in disas_set_info()

[Philippe Mathieu-Daudé]

Have the CPUClass::disas_set_info() callback set the
disassemble_info::endian field.

Signed-off-by: Philippe Mathieu-Daudé <philmd@linaro.org>
Reviewed-by: Thomas Huth <thuth@redhat.com>
---
 target/mips/cpu.c | 3 +++
 1 file changed, 3 insertions(+)

diff --git a/target/mips/cpu.c b/target/mips/cpu.c
index 0b267d2e507..f6d247b530f 100644
--- a/target/mips/cpu.c
+++ b/target/mips/cpu.c
@@ -429,12 +429,15 @@ static void mips_cpu_disas_set_info(CPUState *s, disassemble_info *info)
 {
     if (!(cpu_env(s)->insn_flags & ISA_NANOMIPS32)) {
 #if TARGET_BIG_ENDIAN
+        info->endian = BFD_ENDIAN_BIG;
         info->print_insn = print_insn_big_mips;
 #else
+        info->endian = BFD_ENDIAN_LITTLE;
         info->print_insn = print_insn_little_mips;
 #endif
     } else {
         info->print_insn = print_insn_nanomips;
+        info->endian = BFD_ENDIAN_LITTLE;
     }
 }
 
-- 
2.47.1

[PATCH v2 06/10] target/ppc: Set disassemble_info::endian value in disas_set_info()

[Philippe Mathieu-Daudé]

Have the CPUClass::disas_set_info() callback always set\
the disassemble_info::endian field.

Reviewed-by: Thomas Huth <thuth@redhat.com>
Signed-off-by: Philippe Mathieu-Daudé <philmd@linaro.org>
---
 target/ppc/cpu_init.c | 2 ++
 1 file changed, 2 insertions(+)

diff --git a/target/ppc/cpu_init.c b/target/ppc/cpu_init.c
index 25e835d65e7..e816d30114b 100644
--- a/target/ppc/cpu_init.c
+++ b/target/ppc/cpu_init.c
@@ -7398,6 +7398,8 @@ static void ppc_disas_set_info(CPUState *cs, disassemble_info *info)
 
     if ((env->hflags >> MSR_LE) & 1) {
         info->endian = BFD_ENDIAN_LITTLE;
+    } else {
+        info->endian = BFD_ENDIAN_BIG;
     }
     info->mach = env->bfd_mach;
     if (!env->bfd_mach) {
-- 
2.47.1

[PATCH v2 07/10] target/riscv: Set disassemble_info::endian value in disas_set_info()

[Philippe Mathieu-Daudé]

Have the CPUClass::disas_set_info() callback set the
disassemble_info::endian field.

Signed-off-by: Philippe Mathieu-Daudé <philmd@linaro.org>
Reviewed-by: Thomas Huth <thuth@redhat.com>
---
 target/riscv/cpu.c | 9 +++++++++
 1 file changed, 9 insertions(+)

diff --git a/target/riscv/cpu.c b/target/riscv/cpu.c
index 3d4bd157d2c..b39a701d751 100644
--- a/target/riscv/cpu.c
+++ b/target/riscv/cpu.c
@@ -1156,6 +1156,15 @@ static void riscv_cpu_disas_set_info(CPUState *s, disassemble_info *info)
     CPURISCVState *env = &cpu->env;
     info->target_info = &cpu->cfg;
 
+    /*
+     * A couple of bits in MSTATUS set the endianness:
+     *  - MSTATUS_UBE (User-mode),
+     *  - MSTATUS_SBE (Supervisor-mode),
+     *  - MSTATUS_MBE (Machine-mode)
+     * but we don't implement that yet.
+     */
+    info->endian = BFD_ENDIAN_LITTLE;
+
     switch (env->xl) {
     case MXL_RV32:
         info->print_insn = print_insn_riscv32;
-- 
2.47.1

[PATCH v2 08/10] target/sh4: Set disassemble_info::endian value in disas_set_info()

[Philippe Mathieu-Daudé]

Have the CPUClass::disas_set_info() callback set the
disassemble_info::endian field.

Signed-off-by: Philippe Mathieu-Daudé <philmd@linaro.org>
Reviewed-by: Thomas Huth <thuth@redhat.com>
---
 target/sh4/cpu.c | 5 +++++
 1 file changed, 5 insertions(+)

diff --git a/target/sh4/cpu.c b/target/sh4/cpu.c
index e3c2aea1a64..9d3e6cb2fd7 100644
--- a/target/sh4/cpu.c
+++ b/target/sh4/cpu.c
@@ -134,6 +134,11 @@ static void superh_cpu_reset_hold(Object *obj, ResetType type)
 
 static void superh_cpu_disas_set_info(CPUState *cpu, disassemble_info *info)
 {
+#if TARGET_BIG_ENDIAN
+    info->endian = BFD_ENDIAN_BIG;
+#else
+    info->endian = BFD_ENDIAN_LITTLE;
+#endif
     info->mach = bfd_mach_sh4;
     info->print_insn = print_insn_sh;
 }
-- 
2.47.1

[PATCH v2 09/10] target/xtensa: Set disassemble_info::endian value in disas_set_info()

[Philippe Mathieu-Daudé]

Have the CPUClass::disas_set_info() callback set the
disassemble_info::endian field.

Signed-off-by: Philippe Mathieu-Daudé <philmd@linaro.org>
Reviewed-by: Thomas Huth <thuth@redhat.com>
---
 target/xtensa/cpu.c | 5 +++++
 1 file changed, 5 insertions(+)

diff --git a/target/xtensa/cpu.c b/target/xtensa/cpu.c
index efbfe73fcfb..bc170dbb5cc 100644
--- a/target/xtensa/cpu.c
+++ b/target/xtensa/cpu.c
@@ -159,6 +159,11 @@ static void xtensa_cpu_disas_set_info(CPUState *cs, disassemble_info *info)
 
     info->private_data = cpu->env.config->isa;
     info->print_insn = print_insn_xtensa;
+#if TARGET_BIG_ENDIAN
+    info->endian = BFD_ENDIAN_BIG;
+#else
+    info->endian = BFD_ENDIAN_LITTLE;
+#endif
 }
 
 static void xtensa_cpu_realizefn(DeviceState *dev, Error **errp)
-- 
2.47.1

[PATCH v2 10/10] disas: Remove target_words_bigendian() call in initialize_debug_target()

[Philippe Mathieu-Daudé]

All CPUClass implementations must implement disas_set_info()
which sets the disassemble_info::endian value.

Ensure that by:

1/ assert disas_set_info() handler is not NULL
2/ set %endian to BFD_ENDIAN_UNKNOWN before calling the
   CPUClass::disas_set_info() handler, then assert %endian
   is not BFD_ENDIAN_UNKNOWN after the call.

This allows removing the target_words_bigendian() call in disas/.

Suggested-by: Richard Henderson <richard.henderson@linaro.org>
Signed-off-by: Philippe Mathieu-Daudé <philmd@linaro.org>
Reviewed-by: Thomas Huth <thuth@redhat.com>
---
 disas/disas-common.c | 13 ++++---------
 1 file changed, 4 insertions(+), 9 deletions(-)

diff --git a/disas/disas-common.c b/disas/disas-common.c
index 57505823cb7..42e911e36be 100644
--- a/disas/disas-common.c
+++ b/disas/disas-common.c
@@ -7,7 +7,6 @@
 #include "disas/disas.h"
 #include "disas/capstone.h"
 #include "hw/core/cpu.h"
-#include "exec/tswap.h"
 #include "disas-internal.h"
 
 
@@ -61,15 +60,11 @@ void disas_initialize_debug_target(CPUDebug *s, CPUState *cpu)
 
     s->cpu = cpu;
     s->info.print_address_func = print_address;
-    if (target_words_bigendian()) {
-        s->info.endian = BFD_ENDIAN_BIG;
-    } else {
-        s->info.endian =  BFD_ENDIAN_LITTLE;
-    }
+    s->info.endian =  BFD_ENDIAN_UNKNOWN;
 
-    if (cpu->cc->disas_set_info) {
-        cpu->cc->disas_set_info(cpu, &s->info);
-    }
+    g_assert(cpu->cc->disas_set_info);
+    cpu->cc->disas_set_info(cpu, &s->info);
+    g_assert(s->info.endian !=  BFD_ENDIAN_UNKNOWN);
 }
 
 int disas_gstring_printf(FILE *stream, const char *fmt, ...)
-- 
2.47.1

Re: [PATCH v2 01/10] target: Set disassemble_info::endian value for little-endian targets

[Richard Henderson]

On 2/10/25 13:29, Philippe Mathieu-Daudé wrote:
> @@ -35,6 +35,11 @@ static const gchar *tricore_gdb_arch_name(CPUState *cs)
>       return "tricore";
>   }
>   
> +static void tricore_cpu_disas_set_info(CPUState *cpu, disassemble_info *info)
> +{
> +    info->endian = BFD_ENDIAN_LITTLE;
> +}

While this is not wrong, since there's no disassembler it's slightly disingenuous.

This is the only target without a callback function, correct?  Let's split this function 
out separately, along with the hunk in patch 10 which makes the disas_set_info call 
unconditional.

Otherwise,
Reviewed-by: Richard Henderson <richard.henderson@linaro.org>


r~

Re: [PATCH v2 02/10] target: Set disassemble_info::endian value for big-endian targets

[Richard Henderson]

On 2/10/25 13:29, Philippe Mathieu-Daudé wrote:
> Have theCPUClass::disas_set_info() callback set the
> disassemble_info::endian field for big-endian targets.
> 
> Signed-off-by: Philippe Mathieu-Daudé<philmd@linaro.org>
> Reviewed-by: Thomas Huth<thuth@redhat.com>
> ---
>   target/hppa/cpu.c     | 1 +
>   target/m68k/cpu.c     | 1 +
>   target/openrisc/cpu.c | 1 +
>   target/s390x/cpu.c    | 1 +
>   target/sparc/cpu.c    | 1 +
>   5 files changed, 5 insertions(+)

Reviewed-by: Richard Henderson <richard.henderson@linaro.org>

r~

Re: [PATCH v2 01/10] target: Set disassemble_info::endian value for little-endian targets

[Philippe Mathieu-Daudé]

On 10/2/25 22:54, Richard Henderson wrote:
> On 2/10/25 13:29, Philippe Mathieu-Daudé wrote:
>> @@ -35,6 +35,11 @@ static const gchar *tricore_gdb_arch_name(CPUState 
>> *cs)
>>       return "tricore";
>>   }
>> +static void tricore_cpu_disas_set_info(CPUState *cpu, 
>> disassemble_info *info)
>> +{
>> +    info->endian = BFD_ENDIAN_LITTLE;
>> +}
> 
> While this is not wrong, since there's no disassembler it's slightly 
> disingenuous.
> 
> This is the only target without a callback function, correct?  Let's 
> split this function out separately, along with the hunk in patch 10 
> which makes the disas_set_info call unconditional.

OK, I'll also move this line to the previous conditional call:

+    g_assert(s->info.endian != BFD_ENDIAN_UNKNOWN);

> 
> Otherwise,
> Reviewed-by: Richard Henderson <richard.henderson@linaro.org>

Thanks!

Re: [PATCH v2 03/10] target/arm: Set disassemble_info::endian value in disas_set_info()

[Richard Henderson]

On 2/10/25 13:29, Philippe Mathieu-Daudé wrote:
> Have the CPUClass::disas_set_info() callback set the
> disassemble_info::endian field.
> 
> Signed-off-by: Philippe Mathieu-Daudé <philmd@linaro.org>
> ---
>   target/arm/cpu.c | 10 +++-------
>   1 file changed, 3 insertions(+), 7 deletions(-)
> 
> diff --git a/target/arm/cpu.c b/target/arm/cpu.c
> index 94f1c55622b..68b3a9d3ab0 100644
> --- a/target/arm/cpu.c
> +++ b/target/arm/cpu.c
> @@ -1188,7 +1188,7 @@ static void arm_disas_set_info(CPUState *cpu, disassemble_info *info)
>   {
>       ARMCPU *ac = ARM_CPU(cpu);
>       CPUARMState *env = &ac->env;
> -    bool sctlr_b;
> +    bool sctlr_b = arm_sctlr_b(env);
>   
>       if (is_a64(env)) {
>           info->cap_arch = CS_ARCH_ARM64;
> @@ -1215,13 +1215,9 @@ static void arm_disas_set_info(CPUState *cpu, disassemble_info *info)
>           info->cap_mode = cap_mode;
>       }
>   
> -    sctlr_b = arm_sctlr_b(env);
> +    info->endian = BFD_ENDIAN_LITTLE;
>       if (bswap_code(sctlr_b)) {
> -#if TARGET_BIG_ENDIAN
> -        info->endian = BFD_ENDIAN_LITTLE;
> -#else
> -        info->endian = BFD_ENDIAN_BIG;
> -#endif
> +        info->endian = TARGET_BIG_ENDIAN ? BFD_ENDIAN_LITTLE : BFD_ENDIAN_BIG;
>       }
>       info->flags &= ~INSN_ARM_BE32;
>   #ifndef CONFIG_USER_ONLY

This is a faithful adjustment to the existing code, so,

Reviewed-by: Richard Henderson <richard.henderson@linaro.org>

However:

(1) aarch64 code is always little-endian,
(2) sctlr_b is always false from armv7 (and thus always false for aarch64)
(3) I think the BE32 logic is wrong -- CONFIG_USER_ONLY is irrelevant.
     See linux-user/arm/cpu_loop.c, target_cpu_copy_regs.


r~

Re: [PATCH v2 04/10] target/microblaze: Set disassemble_info::endian value in disas_set_info

[Richard Henderson]

On 2/10/25 13:29, Philippe Mathieu-Daudé wrote:
> Have the CPUClass::disas_set_info() callback set the
> disassemble_info::endian field.
> 
> Signed-off-by: Philippe Mathieu-Daudé <philmd@linaro.org>
> Reviewed-by: Thomas Huth <thuth@redhat.com>
> ---
>   target/microblaze/cpu.c | 1 +
>   1 file changed, 1 insertion(+)
> 
> diff --git a/target/microblaze/cpu.c b/target/microblaze/cpu.c
> index 13d194cef88..27089e3c579 100644
> --- a/target/microblaze/cpu.c
> +++ b/target/microblaze/cpu.c
> @@ -224,6 +224,7 @@ static void mb_disas_set_info(CPUState *cpu, disassemble_info *info)
>   {
>       info->mach = bfd_arch_microblaze;
>       info->print_insn = print_insn_microblaze;
> +    info->endian = TARGET_BIG_ENDIAN ? BFD_ENDIAN_BIG : BFD_ENDIAN_LITTLE;
>   }
>   
>   static void mb_cpu_realizefn(DeviceState *dev, Error **errp)

Reviewed-by: Richard Henderson <richard.henderson@linaro.org>

r~

Re: [PATCH v2 05/10] target/mips: Set disassemble_info::endian value in disas_set_info()

[Richard Henderson]

On 2/10/25 13:29, Philippe Mathieu-Daudé wrote:
> Have theCPUClass::disas_set_info() callback set the
> disassemble_info::endian field.
> 
> Signed-off-by: Philippe Mathieu-Daudé<philmd@linaro.org>
> Reviewed-by: Thomas Huth<thuth@redhat.com>
> ---
>   target/mips/cpu.c | 3 +++
>   1 file changed, 3 insertions(+)

Reviewed-by: Richard Henderson <richard.henderson@linaro.org>

r~

Re: [PATCH v2 06/10] target/ppc: Set disassemble_info::endian value in disas_set_info()

[Richard Henderson]

On 2/10/25 13:29, Philippe Mathieu-Daudé wrote:
> Have the CPUClass::disas_set_info() callback always set\
> the disassemble_info::endian field.
> 
> Reviewed-by: Thomas Huth <thuth@redhat.com>
> Signed-off-by: Philippe Mathieu-Daudé <philmd@linaro.org>
> ---
>   target/ppc/cpu_init.c | 2 ++
>   1 file changed, 2 insertions(+)
> 
> diff --git a/target/ppc/cpu_init.c b/target/ppc/cpu_init.c
> index 25e835d65e7..e816d30114b 100644
> --- a/target/ppc/cpu_init.c
> +++ b/target/ppc/cpu_init.c
> @@ -7398,6 +7398,8 @@ static void ppc_disas_set_info(CPUState *cs, disassemble_info *info)
>   
>       if ((env->hflags >> MSR_LE) & 1) {
>           info->endian = BFD_ENDIAN_LITTLE;
> +    } else {
> +        info->endian = BFD_ENDIAN_BIG;
>       }
>       info->mach = env->bfd_mach;
>       if (!env->bfd_mach) {

Reviewed-by: Richard Henderson <richard.henderson@linaro.org>

r~

Re: [PATCH v2 07/10] target/riscv: Set disassemble_info::endian value in disas_set_info()

[Richard Henderson]

On 2/10/25 13:29, Philippe Mathieu-Daudé wrote:
> Have the CPUClass::disas_set_info() callback set the
> disassemble_info::endian field.
> 
> Signed-off-by: Philippe Mathieu-Daudé <philmd@linaro.org>
> Reviewed-by: Thomas Huth <thuth@redhat.com>
> ---
>   target/riscv/cpu.c | 9 +++++++++
>   1 file changed, 9 insertions(+)
> 
> diff --git a/target/riscv/cpu.c b/target/riscv/cpu.c
> index 3d4bd157d2c..b39a701d751 100644
> --- a/target/riscv/cpu.c
> +++ b/target/riscv/cpu.c
> @@ -1156,6 +1156,15 @@ static void riscv_cpu_disas_set_info(CPUState *s, disassemble_info *info)
>       CPURISCVState *env = &cpu->env;
>       info->target_info = &cpu->cfg;
>   
> +    /*
> +     * A couple of bits in MSTATUS set the endianness:
> +     *  - MSTATUS_UBE (User-mode),
> +     *  - MSTATUS_SBE (Supervisor-mode),
> +     *  - MSTATUS_MBE (Machine-mode)
> +     * but we don't implement that yet.
> +     */
> +    info->endian = BFD_ENDIAN_LITTLE;
> +
>       switch (env->xl) {
>       case MXL_RV32:
>           info->print_insn = print_insn_riscv32;

Reviewed-by: Richard Henderson <richard.henderson@linaro.org>

r~

Re: [PATCH v2 08/10] target/sh4: Set disassemble_info::endian value in disas_set_info()

[Richard Henderson]

On 2/10/25 13:29, Philippe Mathieu-Daudé wrote:
> Have the CPUClass::disas_set_info() callback set the
> disassemble_info::endian field.
> 
> Signed-off-by: Philippe Mathieu-Daudé <philmd@linaro.org>
> Reviewed-by: Thomas Huth <thuth@redhat.com>
> ---
>   target/sh4/cpu.c | 5 +++++
>   1 file changed, 5 insertions(+)
> 
> diff --git a/target/sh4/cpu.c b/target/sh4/cpu.c
> index e3c2aea1a64..9d3e6cb2fd7 100644
> --- a/target/sh4/cpu.c
> +++ b/target/sh4/cpu.c
> @@ -134,6 +134,11 @@ static void superh_cpu_reset_hold(Object *obj, ResetType type)
>   
>   static void superh_cpu_disas_set_info(CPUState *cpu, disassemble_info *info)
>   {
> +#if TARGET_BIG_ENDIAN
> +    info->endian = BFD_ENDIAN_BIG;
> +#else
> +    info->endian = BFD_ENDIAN_LITTLE;
> +#endif

?:

With that,
Reviewed-by: Richard Henderson <richard.henderson@linaro.org>


r~

Re: [PATCH v2 09/10] target/xtensa: Set disassemble_info::endian value in disas_set_info()

[Richard Henderson]

On 2/10/25 13:29, Philippe Mathieu-Daudé wrote:
> Have the CPUClass::disas_set_info() callback set the
> disassemble_info::endian field.
> 
> Signed-off-by: Philippe Mathieu-Daudé <philmd@linaro.org>
> Reviewed-by: Thomas Huth <thuth@redhat.com>
> ---
>   target/xtensa/cpu.c | 5 +++++
>   1 file changed, 5 insertions(+)
> 
> diff --git a/target/xtensa/cpu.c b/target/xtensa/cpu.c
> index efbfe73fcfb..bc170dbb5cc 100644
> --- a/target/xtensa/cpu.c
> +++ b/target/xtensa/cpu.c
> @@ -159,6 +159,11 @@ static void xtensa_cpu_disas_set_info(CPUState *cs, disassemble_info *info)
>   
>       info->private_data = cpu->env.config->isa;
>       info->print_insn = print_insn_xtensa;
> +#if TARGET_BIG_ENDIAN
> +    info->endian = BFD_ENDIAN_BIG;
> +#else
> +    info->endian = BFD_ENDIAN_LITTLE;
> +#endif
>   }
>   
?:

With that,
Reviewed-by: Richard Henderson <richard.henderson@linaro.org>

r~

Re: [PATCH v2 10/10] disas: Remove target_words_bigendian() call in initialize_debug_target()

[Richard Henderson]

On 2/10/25 13:29, Philippe Mathieu-Daudé wrote:
> All CPUClass implementations must implement disas_set_info()
> which sets the disassemble_info::endian value.
> 
> Ensure that by:
> 
> 1/ assert disas_set_info() handler is not NULL

Can we do that earlier than here?

> @@ -61,15 +60,11 @@ void disas_initialize_debug_target(CPUDebug *s, CPUState *cpu)
>   
>       s->cpu = cpu;
>       s->info.print_address_func = print_address;
> -    if (target_words_bigendian()) {
> -        s->info.endian = BFD_ENDIAN_BIG;
> -    } else {
> -        s->info.endian =  BFD_ENDIAN_LITTLE;
> -    }
> +    s->info.endian =  BFD_ENDIAN_UNKNOWN;

Extra space.

> +    g_assert(s->info.endian !=  BFD_ENDIAN_UNKNOWN);

And another one.

With the spacing fixed,
Reviewed-by: Richard Henderson <richard.henderson@linaro.org>


r~

Re: [PATCH v2 03/10] target/arm: Set disassemble_info::endian value in disas_set_info()

[Philippe Mathieu-Daudé]

On 10/2/25 23:10, Richard Henderson wrote:
> On 2/10/25 13:29, Philippe Mathieu-Daudé wrote:
>> Have the CPUClass::disas_set_info() callback set the
>> disassemble_info::endian field.
>>
>> Signed-off-by: Philippe Mathieu-Daudé <philmd@linaro.org>
>> ---
>>   target/arm/cpu.c | 10 +++-------
>>   1 file changed, 3 insertions(+), 7 deletions(-)
>>
>> diff --git a/target/arm/cpu.c b/target/arm/cpu.c
>> index 94f1c55622b..68b3a9d3ab0 100644
>> --- a/target/arm/cpu.c
>> +++ b/target/arm/cpu.c
>> @@ -1188,7 +1188,7 @@ static void arm_disas_set_info(CPUState *cpu, 
>> disassemble_info *info)
>>   {
>>       ARMCPU *ac = ARM_CPU(cpu);
>>       CPUARMState *env = &ac->env;
>> -    bool sctlr_b;
>> +    bool sctlr_b = arm_sctlr_b(env);
>>       if (is_a64(env)) {
>>           info->cap_arch = CS_ARCH_ARM64;
>> @@ -1215,13 +1215,9 @@ static void arm_disas_set_info(CPUState *cpu, 
>> disassemble_info *info)
>>           info->cap_mode = cap_mode;
>>       }
>> -    sctlr_b = arm_sctlr_b(env);
>> +    info->endian = BFD_ENDIAN_LITTLE;
>>       if (bswap_code(sctlr_b)) {
>> -#if TARGET_BIG_ENDIAN
>> -        info->endian = BFD_ENDIAN_LITTLE;
>> -#else
>> -        info->endian = BFD_ENDIAN_BIG;
>> -#endif
>> +        info->endian = TARGET_BIG_ENDIAN ? BFD_ENDIAN_LITTLE : 
>> BFD_ENDIAN_BIG;
>>       }
>>       info->flags &= ~INSN_ARM_BE32;
>>   #ifndef CONFIG_USER_ONLY
> 
> This is a faithful adjustment to the existing code, so,
> 
> Reviewed-by: Richard Henderson <richard.henderson@linaro.org>
> 
> However:
> 
> (1) aarch64 code is always little-endian,
> (2) sctlr_b is always false from armv7 (and thus always false for aarch64)
> (3) I think the BE32 logic is wrong -- CONFIG_USER_ONLY is irrelevant.
>      See linux-user/arm/cpu_loop.c, target_cpu_copy_regs.

What about v7-R [*]? I don't see SCTLR_IE defined as 1<<31 for AArch32,
only:

#define SCTLR_EnIA    (1U << 31) /* v8.3, AArch64 only */
#define SCTLR_DSSBS_32 (1U << 31) /* v8.5, AArch32 only */

---

[*] 
https://developer.arm.com/documentation/ddi0406/cb/System-Level-Architecture/System-Control-Registers-in-a-PMSA-implementation/PMSA-System-control-registers-descriptions--in-register-order/SCTLR--System-Control-Register--PMSA

SCTLR_IE, bit[31]

Instruction Endianness. This bit indicates the endianness of the
instructions issued to the processor. The possible values of this bit are:

0: Little-endian byte ordering in the instructions.
1: Big-endian byte ordering in the instructions.

When set to 1, this bit causes the byte order of instructions to be 
reversed at runtime.

This bit is read-only. It is implementation defined which instruction 
endianness is used by an ARMv7-R implementation, and this bit must 
indicate the implemented endianness.

---

For the Cortex-r5* we have, SCTLR_IE is always 0 in reset_sctlr.

Is it OK to consider v7-R implemented as little-endian in QEMU?

Re: [PATCH v2 03/10] target/arm: Set disassemble_info::endian value in disas_set_info()

[Richard Henderson]

On 2/10/25 14:59, Philippe Mathieu-Daudé wrote:
> On 10/2/25 23:10, Richard Henderson wrote:
>> On 2/10/25 13:29, Philippe Mathieu-Daudé wrote:
>>> Have the CPUClass::disas_set_info() callback set the
>>> disassemble_info::endian field.
>>>
>>> Signed-off-by: Philippe Mathieu-Daudé <philmd@linaro.org>
>>> ---
>>>   target/arm/cpu.c | 10 +++-------
>>>   1 file changed, 3 insertions(+), 7 deletions(-)
>>>
>>> diff --git a/target/arm/cpu.c b/target/arm/cpu.c
>>> index 94f1c55622b..68b3a9d3ab0 100644
>>> --- a/target/arm/cpu.c
>>> +++ b/target/arm/cpu.c
>>> @@ -1188,7 +1188,7 @@ static void arm_disas_set_info(CPUState *cpu, disassemble_info 
>>> *info)
>>>   {
>>>       ARMCPU *ac = ARM_CPU(cpu);
>>>       CPUARMState *env = &ac->env;
>>> -    bool sctlr_b;
>>> +    bool sctlr_b = arm_sctlr_b(env);
>>>       if (is_a64(env)) {
>>>           info->cap_arch = CS_ARCH_ARM64;
>>> @@ -1215,13 +1215,9 @@ static void arm_disas_set_info(CPUState *cpu, disassemble_info 
>>> *info)
>>>           info->cap_mode = cap_mode;
>>>       }
>>> -    sctlr_b = arm_sctlr_b(env);
>>> +    info->endian = BFD_ENDIAN_LITTLE;
>>>       if (bswap_code(sctlr_b)) {
>>> -#if TARGET_BIG_ENDIAN
>>> -        info->endian = BFD_ENDIAN_LITTLE;
>>> -#else
>>> -        info->endian = BFD_ENDIAN_BIG;
>>> -#endif
>>> +        info->endian = TARGET_BIG_ENDIAN ? BFD_ENDIAN_LITTLE : BFD_ENDIAN_BIG;
>>>       }
>>>       info->flags &= ~INSN_ARM_BE32;
>>>   #ifndef CONFIG_USER_ONLY
>>
>> This is a faithful adjustment to the existing code, so,
>>
>> Reviewed-by: Richard Henderson <richard.henderson@linaro.org>
>>
>> However:
>>
>> (1) aarch64 code is always little-endian,
>> (2) sctlr_b is always false from armv7 (and thus always false for aarch64)
>> (3) I think the BE32 logic is wrong -- CONFIG_USER_ONLY is irrelevant.
>>      See linux-user/arm/cpu_loop.c, target_cpu_copy_regs.
> 
> What about v7-R [*]? I don't see SCTLR_IE defined as 1<<31 for AArch32,
> only:

BE32 was a really old arm thingy, and I it was removed in armv7 (see arm_sctlr_b).
With BE8 (armv6+), instructions are always little-endian, only data accesses change.

> For the Cortex-r5* we have, SCTLR_IE is always 0 in reset_sctlr.
> 
> Is it OK to consider v7-R implemented as little-endian in QEMU?

Yes.


r~

Re: [PATCH v2 00/10] disas: Have CPUClass::disas_set_info() callback set the endianness

[Philippe Mathieu-Daudé]

On 10/2/25 22:29, Philippe Mathieu-Daudé wrote:

> Philippe Mathieu-Daudé (10):
>    target: Set disassemble_info::endian value for little-endian targets
>    target: Set disassemble_info::endian value for big-endian targets
>    target/arm: Set disassemble_info::endian value in disas_set_info()
>    target/microblaze: Set disassemble_info::endian value in
>      disas_set_info
>    target/mips: Set disassemble_info::endian value in disas_set_info()
>    target/ppc: Set disassemble_info::endian value in disas_set_info()
>    target/riscv: Set disassemble_info::endian value in disas_set_info()
>    target/sh4: Set disassemble_info::endian value in disas_set_info()
>    target/xtensa: Set disassemble_info::endian value in disas_set_info()
>    disas: Remove target_words_bigendian() call in
>      initialize_debug_target()

Series queued with rth's comments addressed, thanks!

Re: [PATCH v2 00/10] disas: Have CPUClass::disas_set_info() callback set the endianness

[Philippe Mathieu-Daudé]

On 6/3/25 14:58, Philippe Mathieu-Daudé wrote:
> On 10/2/25 22:29, Philippe Mathieu-Daudé wrote:
> 
>> Philippe Mathieu-Daudé (10):
>>    target: Set disassemble_info::endian value for little-endian targets
>>    target: Set disassemble_info::endian value for big-endian targets
>>    target/arm: Set disassemble_info::endian value in disas_set_info()
>>    target/microblaze: Set disassemble_info::endian value in
>>      disas_set_info
>>    target/mips: Set disassemble_info::endian value in disas_set_info()
>>    target/ppc: Set disassemble_info::endian value in disas_set_info()
>>    target/riscv: Set disassemble_info::endian value in disas_set_info()
>>    target/sh4: Set disassemble_info::endian value in disas_set_info()
>>    target/xtensa: Set disassemble_info::endian value in disas_set_info()
>>    disas: Remove target_words_bigendian() call in
>>      initialize_debug_target()
> 
> Series queued with rth's comments addressed, thanks!

Oops I meant to reply this on v3:
https://lore.kernel.org/qemu-devel/20250210221830.69129-1-philmd@linaro.org/