Re: [PATCH 1/4] target/riscv: Remove misa_mxl validation

[Akihiko Odaki <akihiko.odaki@daynix.com>]

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.