Re: [PATCH v2 12/14] target/riscv: Split out the vill from vtype

[LIU Zhiwei <zhiwei_liu@c-sky.com>]

On 2021/11/10 下午7:23, Richard Henderson wrote:
> On 11/10/21 8:04 AM, LIU Zhiwei wrote:
>> We need not specially process vtype when XLEN changes.
>> Signed-off-by: LIU Zhiwei <zhiwei_liu@c-sky.com>
>> ---
>>   target/riscv/cpu.h           |  1 +
>>   target/riscv/csr.c           | 15 ++++++++++++++-
>>   target/riscv/machine.c       |  1 +
>>   target/riscv/vector_helper.c |  7 ++-----
>>   4 files changed, 18 insertions(+), 6 deletions(-)
>
> This patch should come before patch 6, which is over-complicated.

Agree.

One question here. Even come before patch 6, we don't have a simple way 
to choose vill and reserved fields from s2 register in patch 6.

>
>> +    target_ulong vill;
>
> This could be bool, though there's no good place to slot it that does 
> not result in unused padding.

As env->vill will be used in read_vtype,  we still need to covert 
env->vill type to target_ulong there.
Is there any benefit to use bool instead of target_ulong?

> Comments should be added to show that this bit is now missing from vtype.
>
>> diff --git a/target/riscv/machine.c b/target/riscv/machine.c
>> index 19e982d3f0..cc4dda4b93 100644
>> --- a/target/riscv/machine.c
>> +++ b/target/riscv/machine.c
>> @@ -104,6 +104,7 @@ static const VMStateDescription vmstate_vector = {
>>               VMSTATE_UINTTL(env.vl, RISCVCPU),
>>               VMSTATE_UINTTL(env.vstart, RISCVCPU),
>>               VMSTATE_UINTTL(env.vtype, RISCVCPU),
>> +            VMSTATE_UINTTL(env.vill, RISCVCPU),
>>               VMSTATE_END_OF_LIST()
>
> This will need a bump to version.
>
>> @@ -45,11 +45,8 @@ target_ulong HELPER(vsetvl)(CPURISCVState *env, 
>> target_ulong s1,
>>       }
>>       if ((sew > cpu->cfg.elen) || vill || (ediv != 0) || (reserved 
>> != 0)) {
>>           /* only set vill bit. */
>> -        if (xlen < TARGET_LONG_BITS) {
>> -            env->vtype = FIELD_DP64(0, VTYPE, VILL_XLEN32, 1);
>> -        } else {
>> -            env->vtype = FIELD_DP64(0, VTYPE, VILL, 1);
>> -        }
>> +        env->vill = 1;
>> +        env->vtype = 0;
>
> This is fine.
>
> You're missing the updates to cpu_get_tb_cpu_state.

Yes.

Thanks,
Zhiwei

>
>
> r~