| Message ID | 20231012054223.37870-2-akihiko.odaki@daynix.com |
|---|---|
| State | New |
| Headers | show |
| Series | [1/4] target/riscv: Remove misa_mxl validation | expand |
On 10/12/23 02:42, Akihiko Odaki wrote: > It is initialized with a simple assignment and there is little room for > error. In fact, the validation is even more complex. > > Signed-off-by: Akihiko Odaki <akihiko.odaki@daynix.com> > --- > target/riscv/cpu.c | 13 ++----------- > 1 file changed, 2 insertions(+), 11 deletions(-) > > diff --git a/target/riscv/cpu.c b/target/riscv/cpu.c > index f5572704de..550b357fb7 100644 > --- a/target/riscv/cpu.c > +++ b/target/riscv/cpu.c > @@ -1042,7 +1042,7 @@ static void riscv_cpu_disable_priv_spec_isa_exts(RISCVCPU *cpu) > } > } > > -static void riscv_cpu_validate_misa_mxl(RISCVCPU *cpu, Error **errp) > +static void riscv_cpu_validate_misa_mxl(RISCVCPU *cpu) > { > RISCVCPUClass *mcc = RISCV_CPU_GET_CLASS(cpu); > CPUClass *cc = CPU_CLASS(mcc); > @@ -1062,11 +1062,6 @@ static void riscv_cpu_validate_misa_mxl(RISCVCPU *cpu, Error **errp) > default: > g_assert_not_reached(); > } > - > - if (env->misa_mxl_max != env->misa_mxl) { > - error_setg(errp, "misa_mxl_max must be equal to misa_mxl"); > - return; > - } > } Note that this patch isn't applicable anymore due to recent changes on master. These changes are intended to be in target/riscv/tcg/tcg-cpu.c. The changes LGTM since env->misa_mxl will always be == env->misa_mxl_max at this point. We do not have any instance in the code where they'll differ. I'd rename the helper to riscv_cpu_set_gdb_core() or similar given that there's no more validation happening in the function. Thanks, Daniel > > /* > @@ -1447,11 +1442,7 @@ static void riscv_cpu_realize_tcg(DeviceState *dev, Error **errp) > return; > } > > - riscv_cpu_validate_misa_mxl(cpu, &local_err); > - if (local_err != NULL) { > - error_propagate(errp, local_err); > - return; > - } > + riscv_cpu_validate_misa_mxl(cpu); > > riscv_cpu_validate_priv_spec(cpu, &local_err); > if (local_err != NULL) {
On 2023/10/12 13:42, Akihiko Odaki wrote: > It is initialized with a simple assignment and there is little room for > error. In fact, the validation is even more complex. > > Signed-off-by: Akihiko Odaki <akihiko.odaki@daynix.com> > --- > target/riscv/cpu.c | 13 ++----------- > 1 file changed, 2 insertions(+), 11 deletions(-) > > diff --git a/target/riscv/cpu.c b/target/riscv/cpu.c > index f5572704de..550b357fb7 100644 > --- a/target/riscv/cpu.c > +++ b/target/riscv/cpu.c > @@ -1042,7 +1042,7 @@ static void riscv_cpu_disable_priv_spec_isa_exts(RISCVCPU *cpu) > } > } > > -static void riscv_cpu_validate_misa_mxl(RISCVCPU *cpu, Error **errp) > +static void riscv_cpu_validate_misa_mxl(RISCVCPU *cpu) > { > RISCVCPUClass *mcc = RISCV_CPU_GET_CLASS(cpu); > CPUClass *cc = CPU_CLASS(mcc); > @@ -1062,11 +1062,6 @@ static void riscv_cpu_validate_misa_mxl(RISCVCPU *cpu, Error **errp) > default: > g_assert_not_reached(); > } > - > - if (env->misa_mxl_max != env->misa_mxl) { > - error_setg(errp, "misa_mxl_max must be equal to misa_mxl"); > - return; > - } > } > > /* > @@ -1447,11 +1442,7 @@ static void riscv_cpu_realize_tcg(DeviceState *dev, Error **errp) > return; > } > > - riscv_cpu_validate_misa_mxl(cpu, &local_err); > - if (local_err != NULL) { > - error_propagate(errp, local_err); > - return; > - } > + riscv_cpu_validate_misa_mxl(cpu); This it not right. As we are still working on the supporting for MXL32 or SXL32, this validation is needed. And we can't ensure the all RISC-V cpus have the same misa_mxl_max or misa_mxl, it is not right to move it to class. For example, in the future, riscv64-softmmu can run 32-bit cpu and 64-bit cpu. And maybe in heterogeneous SOC, we have 32-bit cpu and 64-bit cpu together. I am afraid that we can not accept this patch set. Thanks, Zhiwei > > riscv_cpu_validate_priv_spec(cpu, &local_err); > if (local_err != NULL) {
On 2023/10/17 11:29, LIU Zhiwei wrote: > > On 2023/10/12 13:42, Akihiko Odaki wrote: >> It is initialized with a simple assignment and there is little room for >> error. In fact, the validation is even more complex. >> >> Signed-off-by: Akihiko Odaki <akihiko.odaki@daynix.com> >> --- >> target/riscv/cpu.c | 13 ++----------- >> 1 file changed, 2 insertions(+), 11 deletions(-) >> >> diff --git a/target/riscv/cpu.c b/target/riscv/cpu.c >> index f5572704de..550b357fb7 100644 >> --- a/target/riscv/cpu.c >> +++ b/target/riscv/cpu.c >> @@ -1042,7 +1042,7 @@ static void >> riscv_cpu_disable_priv_spec_isa_exts(RISCVCPU *cpu) >> } >> } >> -static void riscv_cpu_validate_misa_mxl(RISCVCPU *cpu, Error **errp) >> +static void riscv_cpu_validate_misa_mxl(RISCVCPU *cpu) >> { >> RISCVCPUClass *mcc = RISCV_CPU_GET_CLASS(cpu); >> CPUClass *cc = CPU_CLASS(mcc); >> @@ -1062,11 +1062,6 @@ static void >> riscv_cpu_validate_misa_mxl(RISCVCPU *cpu, Error **errp) >> default: >> g_assert_not_reached(); >> } >> - >> - if (env->misa_mxl_max != env->misa_mxl) { >> - error_setg(errp, "misa_mxl_max must be equal to misa_mxl"); >> - return; >> - } >> } >> /* >> @@ -1447,11 +1442,7 @@ static void riscv_cpu_realize_tcg(DeviceState >> *dev, Error **errp) >> return; >> } >> - riscv_cpu_validate_misa_mxl(cpu, &local_err); >> - if (local_err != NULL) { >> - error_propagate(errp, local_err); >> - return; >> - } >> + riscv_cpu_validate_misa_mxl(cpu); > > This it not right. As we are still working on the supporting for MXL32 > or SXL32, this validation is needed. It's not preventing supporting MXL32 or SXL32. It's removing env->misa_mxl_max != env->misa_mxl just because it's initialized with a simple statement: env->misa_mxl_max = env->misa_mxl = mxl; It makes little sense to have a validation code that is more complex than the validated code. > > And we can't ensure the all RISC-V cpus have the same misa_mxl_max or > misa_mxl, it is not right to move it to class. > For example, in the future, riscv64-softmmu can run 32-bit cpu and > 64-bit cpu. And maybe in heterogeneous SOC, > we have 32-bit cpu and 64-bit cpu together. This patch series does not touch misa_mxl. We don't need to ensure that all CPUs have the same misa_mxl_max, but we just need to ensure that CPUs in the same class do. Creating a heterogeneous SoC is still possible by combining e.g. TYPE_RISCV_CPU_SIFIVE_E31 and TYPE_RISCV_CPU_SIFIVE_E51, for example.
+CC Richard On 2023/10/17 11:37, Akihiko Odaki wrote: > On 2023/10/17 11:29, LIU Zhiwei wrote: >> >> On 2023/10/12 13:42, Akihiko Odaki wrote: >>> It is initialized with a simple assignment and there is little room for >>> error. In fact, the validation is even more complex. >>> >>> Signed-off-by: Akihiko Odaki <akihiko.odaki@daynix.com> >>> --- >>> target/riscv/cpu.c | 13 ++----------- >>> 1 file changed, 2 insertions(+), 11 deletions(-) >>> >>> diff --git a/target/riscv/cpu.c b/target/riscv/cpu.c >>> index f5572704de..550b357fb7 100644 >>> --- a/target/riscv/cpu.c >>> +++ b/target/riscv/cpu.c >>> @@ -1042,7 +1042,7 @@ static void >>> riscv_cpu_disable_priv_spec_isa_exts(RISCVCPU *cpu) >>> } >>> } >>> -static void riscv_cpu_validate_misa_mxl(RISCVCPU *cpu, Error **errp) >>> +static void riscv_cpu_validate_misa_mxl(RISCVCPU *cpu) >>> { >>> RISCVCPUClass *mcc = RISCV_CPU_GET_CLASS(cpu); >>> CPUClass *cc = CPU_CLASS(mcc); >>> @@ -1062,11 +1062,6 @@ static void >>> riscv_cpu_validate_misa_mxl(RISCVCPU *cpu, Error **errp) >>> default: >>> g_assert_not_reached(); >>> } >>> - >>> - if (env->misa_mxl_max != env->misa_mxl) { >>> - error_setg(errp, "misa_mxl_max must be equal to misa_mxl"); >>> - return; >>> - } >>> } >>> /* >>> @@ -1447,11 +1442,7 @@ static void riscv_cpu_realize_tcg(DeviceState >>> *dev, Error **errp) >>> return; >>> } >>> - riscv_cpu_validate_misa_mxl(cpu, &local_err); >>> - if (local_err != NULL) { >>> - error_propagate(errp, local_err); >>> - return; >>> - } >>> + riscv_cpu_validate_misa_mxl(cpu); >> >> This it not right. As we are still working on the supporting for >> MXL32 or SXL32, this validation is needed. > > It's not preventing supporting MXL32 or SXL32. It's removing > env->misa_mxl_max != env->misa_mxl just because it's initialized with > a simple statement: > env->misa_mxl_max = env->misa_mxl = mxl; > > It makes little sense to have a validation code that is more complex > than the validated code. > >> >> And we can't ensure the all RISC-V cpus have the same misa_mxl_max or >> misa_mxl, it is not right to move it to class. >> For example, in the future, riscv64-softmmu can run 32-bit cpu and >> 64-bit cpu. And maybe in heterogeneous SOC, >> we have 32-bit cpu and 64-bit cpu together. > > This patch series does not touch misa_mxl. We don't need to ensure > that all CPUs have the same misa_mxl_max, but we just need to ensure > that CPUs in the same class do. Creating a heterogeneous SoC is still > possible by combining e.g. TYPE_RISCV_CPU_SIFIVE_E31 and > TYPE_RISCV_CPU_SIFIVE_E51, for example. I see what you mean. It makes sense to move the misa_mxl_max field from env to the class struct. The misa_mxl_max is always be set by cpu init or the migration. The former is OK. I don't know whether QEMU supports migration from 32-bit CPU to 64-bit CPU. Otherwise, Acked-by: LIU Zhiwei <zhiwei_liu@linux.alibaba.com> Zhiwei
On 2023/10/18 14:53, LIU Zhiwei wrote: > +CC Richard > > On 2023/10/17 11:37, Akihiko Odaki wrote: >> On 2023/10/17 11:29, LIU Zhiwei wrote: >>> >>> On 2023/10/12 13:42, Akihiko Odaki wrote: >>>> It is initialized with a simple assignment and there is little room for >>>> error. In fact, the validation is even more complex. >>>> >>>> Signed-off-by: Akihiko Odaki <akihiko.odaki@daynix.com> >>>> --- >>>> target/riscv/cpu.c | 13 ++----------- >>>> 1 file changed, 2 insertions(+), 11 deletions(-) >>>> >>>> diff --git a/target/riscv/cpu.c b/target/riscv/cpu.c >>>> index f5572704de..550b357fb7 100644 >>>> --- a/target/riscv/cpu.c >>>> +++ b/target/riscv/cpu.c >>>> @@ -1042,7 +1042,7 @@ static void >>>> riscv_cpu_disable_priv_spec_isa_exts(RISCVCPU *cpu) >>>> } >>>> } >>>> -static void riscv_cpu_validate_misa_mxl(RISCVCPU *cpu, Error **errp) >>>> +static void riscv_cpu_validate_misa_mxl(RISCVCPU *cpu) >>>> { >>>> RISCVCPUClass *mcc = RISCV_CPU_GET_CLASS(cpu); >>>> CPUClass *cc = CPU_CLASS(mcc); >>>> @@ -1062,11 +1062,6 @@ static void >>>> riscv_cpu_validate_misa_mxl(RISCVCPU *cpu, Error **errp) >>>> default: >>>> g_assert_not_reached(); >>>> } >>>> - >>>> - if (env->misa_mxl_max != env->misa_mxl) { >>>> - error_setg(errp, "misa_mxl_max must be equal to misa_mxl"); >>>> - return; >>>> - } >>>> } >>>> /* >>>> @@ -1447,11 +1442,7 @@ static void riscv_cpu_realize_tcg(DeviceState >>>> *dev, Error **errp) >>>> return; >>>> } >>>> - riscv_cpu_validate_misa_mxl(cpu, &local_err); >>>> - if (local_err != NULL) { >>>> - error_propagate(errp, local_err); >>>> - return; >>>> - } >>>> + riscv_cpu_validate_misa_mxl(cpu); >>> >>> This it not right. As we are still working on the supporting for >>> MXL32 or SXL32, this validation is needed. >> >> It's not preventing supporting MXL32 or SXL32. It's removing >> env->misa_mxl_max != env->misa_mxl just because it's initialized with >> a simple statement: >> env->misa_mxl_max = env->misa_mxl = mxl; >> >> It makes little sense to have a validation code that is more complex >> than the validated code. >> >>> >>> And we can't ensure the all RISC-V cpus have the same misa_mxl_max or >>> misa_mxl, it is not right to move it to class. >>> For example, in the future, riscv64-softmmu can run 32-bit cpu and >>> 64-bit cpu. And maybe in heterogeneous SOC, >>> we have 32-bit cpu and 64-bit cpu together. >> >> This patch series does not touch misa_mxl. We don't need to ensure >> that all CPUs have the same misa_mxl_max, but we just need to ensure >> that CPUs in the same class do. Creating a heterogeneous SoC is still >> possible by combining e.g. TYPE_RISCV_CPU_SIFIVE_E31 and >> TYPE_RISCV_CPU_SIFIVE_E51, for example. > > I see what you mean. It makes sense to move the misa_mxl_max field from > env to the class struct. The misa_mxl_max is always be set by cpu init > or the migration. > > The former is OK. I don't know whether QEMU supports migration from > 32-bit CPU to 64-bit CPU. Otherwise, > > Acked-by: LIU Zhiwei <zhiwei_liu@linux.alibaba.com> It doesn't. docs/devel/migration.rst states: > For this to work, QEMU has to be launched with the same arguments the > two times. I.e. it can only restore the state in one guest that has > the same devices that the one it was saved (this last requirement can > be relaxed a bit, but for now we can consider that configuration has > to be exactly the same). The corresponding CPUs in two QEMU instances launched with the same arguments will have the same misa_mxl_max values.
diff --git a/target/riscv/cpu.c b/target/riscv/cpu.c index f5572704de..550b357fb7 100644 --- a/target/riscv/cpu.c +++ b/target/riscv/cpu.c @@ -1042,7 +1042,7 @@ static void riscv_cpu_disable_priv_spec_isa_exts(RISCVCPU *cpu) } } -static void riscv_cpu_validate_misa_mxl(RISCVCPU *cpu, Error **errp) +static void riscv_cpu_validate_misa_mxl(RISCVCPU *cpu) { RISCVCPUClass *mcc = RISCV_CPU_GET_CLASS(cpu); CPUClass *cc = CPU_CLASS(mcc); @@ -1062,11 +1062,6 @@ static void riscv_cpu_validate_misa_mxl(RISCVCPU *cpu, Error **errp) default: g_assert_not_reached(); } - - if (env->misa_mxl_max != env->misa_mxl) { - error_setg(errp, "misa_mxl_max must be equal to misa_mxl"); - return; - } } /* @@ -1447,11 +1442,7 @@ static void riscv_cpu_realize_tcg(DeviceState *dev, Error **errp) return; } - riscv_cpu_validate_misa_mxl(cpu, &local_err); - if (local_err != NULL) { - error_propagate(errp, local_err); - return; - } + riscv_cpu_validate_misa_mxl(cpu); riscv_cpu_validate_priv_spec(cpu, &local_err); if (local_err != NULL) {
It is initialized with a simple assignment and there is little room for error. In fact, the validation is even more complex. Signed-off-by: Akihiko Odaki <akihiko.odaki@daynix.com> --- target/riscv/cpu.c | 13 ++----------- 1 file changed, 2 insertions(+), 11 deletions(-)