Re: [PATCH v2 2/3] target/riscv: Initialize gdb_core_xml_file only once

[Akihiko Odaki]

On 2023/10/15 3:19, Daniel Henrique Barboza wrote:
> On 10/14/23 00:35, Akihiko Odaki wrote:
> > gdb_core_xml_file was assigned each time a CPU is instantiated before
> > this change.
> >
> > Signed-off-by: Akihiko Odaki <akihiko.odaki@daynix.com>
> > ---
> >   target/riscv/cpu.c         | 5 +++++
> >   target/riscv/tcg/tcg-cpu.c | 4 ----
> >   2 files changed, 5 insertions(+), 4 deletions(-)
> >
> > diff --git a/target/riscv/cpu.c b/target/riscv/cpu.c
> > index ac4a6c7eec..a811215150 100644
> > --- a/target/riscv/cpu.c
> > +++ b/target/riscv/cpu.c
> > @@ -1575,6 +1575,11 @@ static void riscv_cpu_class_init(ObjectClass 
> > *c, void *data)
> >       cc->get_pc = riscv_cpu_get_pc;
> >       cc->gdb_read_register = riscv_cpu_gdb_read_register;
> >       cc->gdb_write_register = riscv_cpu_gdb_write_register;
> > +#ifdef TARGET_RISCV64
> > +    cc->gdb_core_xml_file = "riscv-64bit-cpu.xml";
> > +#elif defined(TARGET_RISCV32)
> > +    cc->gdb_core_xml_file = "riscv-32bit-cpu.xml";
> > +#endif
> >       cc->gdb_num_core_regs = 33;
> >       cc->gdb_stop_before_watchpoint = true;
> >       cc->disas_set_info = riscv_cpu_disas_set_info;
> > diff --git a/target/riscv/tcg/tcg-cpu.c b/target/riscv/tcg/tcg-cpu.c
> > index e0cbc56320..626fb2acea 100644
> > --- a/target/riscv/tcg/tcg-cpu.c
> > +++ b/target/riscv/tcg/tcg-cpu.c
> > @@ -150,8 +150,6 @@ static void 
> > riscv_cpu_validate_misa_priv(CPURISCVState *env, Error **errp)
> >   static void riscv_cpu_validate_misa_mxl(RISCVCPU *cpu, Error **errp)
> >   {
> > -    RISCVCPUClass *mcc = RISCV_CPU_GET_CLASS(cpu);
> > -    CPUClass *cc = CPU_CLASS(mcc);
> >       CPURISCVState *env = &cpu->env;
> >       /* Validate that MISA_MXL is set properly. */
> > @@ -159,11 +157,9 @@ static void riscv_cpu_validate_misa_mxl(RISCVCPU 
> > *cpu, Error **errp)
> >   #ifdef TARGET_RISCV64
> >       case MXL_RV64:
> >       case MXL_RV128:
> > -        cc->gdb_core_xml_file = "riscv-64bit-cpu.xml";
> >           break;
> >   #elif defined(TARGET_RISCV32)
> >       case MXL_RV32:
> > -        cc->gdb_core_xml_file = "riscv-32bit-cpu.xml";
> >           break;
>
>
> Hmmm the issue here is that, in patch 1, you added an "elif 
> defined(TARGET_RISCV32)"
> based on an assumption that you changed here since there's no more 
> gdb_core files being
> set.

It's opposite. Patch 1 added "elif defined(TARGET_RISCV32)" because 
configs/targets/riscv64-linux-user.mak and 
configs/targets/riscv64-softmmu.mak do not list riscv-32bit-cpu.xml and 
referencing it from TARGET_RISCV64 results in an error.

Since patch 1 now ensures TARGET_RISCV64 will never have MXL_RV32 as 
misa_mxl_max, patch 2 can safely assume TARGET_RISCV64 always has 
riscv-64bit-cpu.xml as gdb_core_xml_file.

> My suggestion is to use patch 1 from v1, where you removed the 
> misa_mxl_max == misa_mxl
> check at the end of this function. And then in this patch you can remove 
> this function
> altogether since you're assigning gdb_core in riscv_cpu_class_init() and 
> the function will
> be left doing nothing of note.

Assigning gdb_core_xml_file is more like a side-effect of 
riscv_cpu_validate_misa_mxl(), and the main purpose of this function is 
to validate misa_mxl[_max]. I think it's still a good idea to validate 
misa_mxl_max in particular. Specifying MXL_RV32 as misa_mxl_max for 
TARGET_RISCV64 or specifying MXL_RV64/MXL_RV128 for TARGET_RISCV32 will 
not work because of the incompatible gdb_core_xml_file (and probably 
other reasons).