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

From: Weiwei Li <liweiwei@iscas.ac.cn>
To: Guo Ren <guoren@kernel.org>, Anup Patel <apatel@ventanamicro.com>
Cc: "open list:RISC-V" <qemu-riscv@nongnu.org>,
	"Anup Patel" <anup@brainfault.org>,
	"Wang Junqiang" <wangjunqiang@iscas.ac.cn>,
	"Bin Meng" <bin.meng@windriver.com>,
	"QEMU Developers" <qemu-devel@nongnu.org>,
	"Alistair Francis" <alistair.francis@wdc.com>,
	"Guo Ren" <ren_guo@c-sky.com>,
	"Wei Wu (吴伟)" <lazyparser@gmail.com>,
	"Palmer Dabbelt" <palmer@dabbelt.com>
Subject: Re: [PATCH v5 1/5] target/riscv: Ignore reserved bits in PTE for RV64
Date: Tue, 18 Jan 2022 19:29:06 +0800	[thread overview]
Message-ID: <bdb141c7-e45c-b05e-60ff-9e60603df6e6@iscas.ac.cn> (raw)
In-Reply-To: <CAJF2gTQp3RTbj51-5J4md_6UWT7qTYxXvvyZmSK5LN91h4fB0w@mail.gmail.com>

在 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.

Or we may  define  PTE_PPN_MASK as  0x3fffffffffffffULL, and use type 
contrast

+        hwaddr ppn = (pte & (target_ulong)PTE_PPN_MASK) >> PTE_PPN_SHIFT;

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/
>>>
>
>