| Message ID | 20231014033545.15220-3-akihiko.odaki@daynix.com |
|---|---|
| State | New |
| Headers | show |
| Series | [v2,1/3] target/riscv: Do not allow MXL_RV32 for TARGET_RISCV64 | expand |
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. 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. Thanks, Daniel > #endif > default:
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).
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; #endif default:
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(-)