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);
 
     riscv_cpu_validate_priv_spec(cpu, &local_err);
     if (local_err != NULL) {
-- 
2.42.0

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.

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) {