Re: [PATCH v5 1/5] target/riscv: Ignore reserved bits in PTE for RV64

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

On 2022/1/18 下午7:29, Weiwei Li wrote:
>
> 在 2022/1/18 下午7:15, Guo Ren 写道:
>> On Tue, Jan 18, 2022 at 4:51 PM Anup Patel <apatel@ventanamicro.com> 
>> wrote:
>>> On Tue, Jan 18, 2022 at 2:16 PM Guo Ren <guoren@kernel.org> wrote:
>>>> On Tue, Jan 18, 2022 at 11:32 AM Anup Patel <anup@brainfault.org> 
>>>> wrote:
>>>>> On Tue, Jan 18, 2022 at 6:47 AM Weiwei Li <liweiwei@iscas.ac.cn> 
>>>>> wrote:
>>>>>> From: Guo Ren <ren_guo@c-sky.com>
>>>>>>
>>>>>> Highest bits of PTE has been used for svpbmt, ref: [1], [2], so we
>>>>>> need to ignore them. They cannot be a part of ppn.
>>>>>>
>>>>>> 1: The RISC-V Instruction Set Manual, Volume II: Privileged 
>>>>>> Architecture
>>>>>>     4.4 Sv39: Page-Based 39-bit Virtual-Memory System
>>>>>>     4.5 Sv48: Page-Based 48-bit Virtual-Memory System
>>>>>>
>>>>>> 2: 
>>>>>> https://github.com/riscv/virtual-memory/blob/main/specs/663-Svpbmt-diff.pdf
>>>>>>
>>>>>> Signed-off-by: Guo Ren <ren_guo@c-sky.com>
>>>>>> Tested-by: Bin Meng <bmeng.cn@gmail.com>
>>>>>> Reviewed-by: Liu Zhiwei <zhiwei_liu@c-sky.com>
>>>>>> Reviewed-by: Bin Meng <bmeng.cn@gmail.com>
>>>>>> Reviewed-by: Alistair Francis <alistair.francis@wdc.com>
>>>>>> ---
>>>>>>   target/riscv/cpu_bits.h   | 7 +++++++
>>>>>>   target/riscv/cpu_helper.c | 2 +-
>>>>>>   2 files changed, 8 insertions(+), 1 deletion(-)
>>>>>>
>>>>>> diff --git a/target/riscv/cpu_bits.h b/target/riscv/cpu_bits.h
>>>>>> index 5a6d49aa64..282cd8eecd 100644
>>>>>> --- a/target/riscv/cpu_bits.h
>>>>>> +++ b/target/riscv/cpu_bits.h
>>>>>> @@ -490,6 +490,13 @@ typedef enum {
>>>>>>   /* Page table PPN shift amount */
>>>>>>   #define PTE_PPN_SHIFT       10
>>>>>>
>>>>>> +/* Page table PPN mask */
>>>>>> +#if defined(TARGET_RISCV32)
>>>>>> +#define PTE_PPN_MASK        0xffffffffUL
>>>>>> +#elif defined(TARGET_RISCV64)
>>>>>> +#define PTE_PPN_MASK        0x3fffffffffffffULL
>>>>>> +#endif
>>>>>> +
>>>>> Going forward we should avoid using target specific "#if"
>>>>> so that we can use the same qemu-system-riscv64 for both
>>>>> RV32 and RV64.
>>>>>
>>>>>>   /* Leaf page shift amount */
>>>>>>   #define PGSHIFT             12
>>>>>>
>>>>>> diff --git a/target/riscv/cpu_helper.c b/target/riscv/cpu_helper.c
>>>>>> index 434a83e66a..26608ddf1c 100644
>>>>>> --- a/target/riscv/cpu_helper.c
>>>>>> +++ b/target/riscv/cpu_helper.c
>>>>>> @@ -619,7 +619,7 @@ restart:
>>>>>>               return TRANSLATE_FAIL;
>>>>>>           }
>>>>>>
>>>>>> -        hwaddr ppn = pte >> PTE_PPN_SHIFT;
>>>>>> +        hwaddr ppn = (pte & PTE_PPN_MASK) >> PTE_PPN_SHIFT;
>>>>> Rather than using "#if", please use "xlen" comparison to extract
>>>>> PPN correctly from PTE.
>>>> Do you mean?
>>>>
>>>> diff --git a/target/riscv/cpu_helper.c b/target/riscv/cpu_helper.c
>>>> index 9fffaccffb..071b7ea4cf 100644
>>>> --- a/target/riscv/cpu_helper.c
>>>> +++ b/target/riscv/cpu_helper.c
>>>> @@ -619,7 +619,11 @@ restart:
>>>>               return TRANSLATE_FAIL;
>>>>           }
>>>>
>>>> -        hwaddr ppn = (pte & PTE_PPN_MASK) >> PTE_PPN_SHIFT;
>>>> +        if (riscv_cpu_mxl(env) == MXL_RV32) {
>>>> +               hwaddr ppn = pte  >> PTE_PPN_SHIFT;
>>>> +       } else {
>>>> +               hwaddr ppn = (pte & 0x3fffffffffffffULL) >> 
>>>> PTE_PPN_SHIFT;
>>>> +       }
>>> Yes, something like this but use a define for 0x3fffffffffffffULL
>> Just as Alistair said: This will need to be dynamic based on get_xl()
>>
>>   I still prefer:
>> +#if defined(TARGET_RISCV32)
>> +#define PTE_PPN_MASK        0xffffffffUL
>> +#elif defined(TARGET_RISCV64)
>> +#define PTE_PPN_MASK        0x3fffffffffffffULL
>> +#endif
>>
>> +        hwaddr ppn = (pte & PTE_PPN_MASK) >> PTE_PPN_SHIFT;
>
> I think the difference may be xl = MXL_RV32 in RV64.
Agree.
>
> Or we may  define  PTE_PPN_MASK as  0x3fffffffffffffULL, and use type 
> contrast
>
> +        hwaddr ppn = (pte & (target_ulong)PTE_PPN_MASK) >> 
> PTE_PPN_SHIFT;

It is absolutely not right to use target_ulong here.

Although we don't support dynamic SXLEN currently,   we should avoid 
assuming  that SXLEN is always 64 on qemu-system-riscv64. Get_cpu_xl is 
the right way.

I have  moved  xl to CPURISCVState in my patch set v6 for UXL. I think 
it will be much convenient for this situation and you can just use 
env->xl to get right XLEN.
But I don't know when it will be merged.

Thanks,
Zhiwei

>
> Regards,
> Weiwei Li
>
>>> Regards,
>>> Anup
>>>
>>>>           RISCVCPU *cpu = env_archcpu(env);
>>>>           if (!(pte & PTE_V)) {
>>>>
>>>>> Regards,
>>>>> Anup
>>>>>
>>>>>>           if (!(pte & PTE_V)) {
>>>>>>               /* Invalid PTE */
>>>>>> -- 
>>>>>> 2.17.1
>>>>>>
>>>>
>>>> -- 
>>>> Best Regards
>>>>   Guo Ren
>>>>
>>>> ML: https://lore.kernel.org/linux-csky/
>>>>
>>
>>
>
>