diff mbox series

[v6,1/5] target/riscv: Ignore reserved bits in PTE for RV64

Message ID 20220125064536.7869-2-liweiwei@iscas.ac.cn
State New
Headers show
Series support subsets of virtual memory extension | expand

Commit Message

Weiwei Li Jan. 25, 2022, 6:45 a.m. UTC
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>
Cc: Liu Zhiwei <zhiwei_liu@c-sky.com>
Cc: Bin Meng <bmeng.cn@gmail.com>
Cc: Alistair Francis <alistair.francis@wdc.com>
---
 target/riscv/cpu.h        | 13 +++++++++++++
 target/riscv/cpu_bits.h   |  7 +++++++
 target/riscv/cpu_helper.c | 14 +++++++++++++-
 3 files changed, 33 insertions(+), 1 deletion(-)

Comments

LIU Zhiwei Jan. 25, 2022, 8:13 a.m. UTC | #1
On 2022/1/25 14:45, Weiwei Li 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>
> Cc: Liu Zhiwei <zhiwei_liu@c-sky.com>
> Cc: Bin Meng <bmeng.cn@gmail.com>
> Cc: Alistair Francis <alistair.francis@wdc.com>
> ---
>   target/riscv/cpu.h        | 13 +++++++++++++
>   target/riscv/cpu_bits.h   |  7 +++++++
>   target/riscv/cpu_helper.c | 14 +++++++++++++-
>   3 files changed, 33 insertions(+), 1 deletion(-)
>
> diff --git a/target/riscv/cpu.h b/target/riscv/cpu.h
> index 55635d68d5..45de8faaca 100644
> --- a/target/riscv/cpu.h
> +++ b/target/riscv/cpu.h
> @@ -341,6 +341,8 @@ struct RISCVCPU {
>           bool ext_counters;
>           bool ext_ifencei;
>           bool ext_icsr;
> +        bool ext_svnapot;
> +        bool ext_svpbmt;
>           bool ext_zfh;
>           bool ext_zfhmin;
>           bool ext_zve32f;
> @@ -495,6 +497,17 @@ static inline int riscv_cpu_xlen(CPURISCVState *env)
>       return 16 << env->xl;
>   }
>   
> +#ifndef CONFIG_USER_ONLY
> +#ifdef TARGET_RISCV32
> +#define riscv_cpu_sxl(env)  ((void)(env), MXL_RV32)
> +#else
> +static inline RISCVMXL riscv_cpu_sxl(CPURISCVState *env)
> +{
> +    return get_field(env->mstatus, MSTATUS64_SXL);
> +}
> +#endif
> +#endif
> +

Perhaps an interface also works for user mode is better.

+#ifdef TARGET_RISCV32
+#define riscv_cpu_sxl(env)  ((void)(env), MXL_RV32)
+#else
+static inline RISCVMXL riscv_cpu_sxl(CPURISCVState *env)
+{
+#ifdef CONFIG_USER_ONLY
+    return env->misa_mxl;
+#else
+    return get_field(env->mstatus, MSTATUS64_SXL);
+#endif
+}
+#endif
+

>   /*
>    * Encode LMUL to lmul as follows:
>    *     LMUL    vlmul    lmul
> diff --git a/target/riscv/cpu_bits.h b/target/riscv/cpu_bits.h
> index 7c87433645..37b622fbfa 100644
> --- a/target/riscv/cpu_bits.h
> +++ b/target/riscv/cpu_bits.h
> @@ -493,6 +493,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        0xFFFFFC00UL
> +#elif defined(TARGET_RISCV64)
> +#define PTE_PPN_MASK        0x3FFFFFFFFFFC00ULL
> +#endif
> +

No need to define PTE_PPN_MASK for TARGET_RISCV32.

>   /* Leaf page shift amount */
>   #define PGSHIFT             12
>   
> diff --git a/target/riscv/cpu_helper.c b/target/riscv/cpu_helper.c
> index 327a2c4f1d..2a921bedfd 100644
> --- a/target/riscv/cpu_helper.c
> +++ b/target/riscv/cpu_helper.c
> @@ -622,7 +622,19 @@ restart:
>               return TRANSLATE_FAIL;
>           }
>   
> -        hwaddr ppn = pte >> PTE_PPN_SHIFT;
> +        hwaddr ppn;
> +        RISCVCPU *cpu = env_archcpu(env);
> +
> +        if (riscv_cpu_sxl(env) == MXL_RV32) {
> +            ppn = pte >> PTE_PPN_SHIFT;
> +        } else if (cpu->cfg.ext_svpbmt || cpu->cfg.ext_svnapot) {
> +            ppn = (pte & PTE_PPN_MASK) >> PTE_PPN_SHIFT;
> +        } else {
> +            ppn = pte >> PTE_PPN_SHIFT;
> +            if ((pte & ~PTE_PPN_MASK) >> PTE_PPN_SHIFT) {
> +                return TRANSLATE_FAIL;
> +            }
> +        }
>   
>           if (!(pte & PTE_V)) {
>               /* Invalid PTE */

Otherwise,

Reviewed-by: LIU Zhiwei <zhiwei_liu@c-sky.com>

Thanks,
Zhiwei
Guo Ren Jan. 25, 2022, 8:40 a.m. UTC | #2
On Tue, Jan 25, 2022 at 4:34 PM LIU Zhiwei <zhiwei_liu@c-sky.com> wrote:
>
>
> On 2022/1/25 14:45, Weiwei Li 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>
> > Cc: Liu Zhiwei <zhiwei_liu@c-sky.com>
> > Cc: Bin Meng <bmeng.cn@gmail.com>
> > Cc: Alistair Francis <alistair.francis@wdc.com>
> > ---
> >   target/riscv/cpu.h        | 13 +++++++++++++
> >   target/riscv/cpu_bits.h   |  7 +++++++
> >   target/riscv/cpu_helper.c | 14 +++++++++++++-
> >   3 files changed, 33 insertions(+), 1 deletion(-)
> >
> > diff --git a/target/riscv/cpu.h b/target/riscv/cpu.h
> > index 55635d68d5..45de8faaca 100644
> > --- a/target/riscv/cpu.h
> > +++ b/target/riscv/cpu.h
> > @@ -341,6 +341,8 @@ struct RISCVCPU {
> >           bool ext_counters;
> >           bool ext_ifencei;
> >           bool ext_icsr;
> > +        bool ext_svnapot;
> > +        bool ext_svpbmt;
> >           bool ext_zfh;
> >           bool ext_zfhmin;
> >           bool ext_zve32f;
> > @@ -495,6 +497,17 @@ static inline int riscv_cpu_xlen(CPURISCVState *env)
> >       return 16 << env->xl;
> >   }
> >
> > +#ifndef CONFIG_USER_ONLY
> > +#ifdef TARGET_RISCV32
> > +#define riscv_cpu_sxl(env)  ((void)(env), MXL_RV32)
> > +#else
> > +static inline RISCVMXL riscv_cpu_sxl(CPURISCVState *env)
> > +{
> > +    return get_field(env->mstatus, MSTATUS64_SXL);
> > +}
> > +#endif
> > +#endif
> > +
>
> Perhaps an interface also works for user mode is better.
>
> +#ifdef TARGET_RISCV32
> +#define riscv_cpu_sxl(env)  ((void)(env), MXL_RV32)
> +#else
> +static inline RISCVMXL riscv_cpu_sxl(CPURISCVState *env)
> +{
> +#ifdef CONFIG_USER_ONLY
> +    return env->misa_mxl;
> +#else
> +    return get_field(env->mstatus, MSTATUS64_SXL);
> +#endif
> +}
> +#endif
> +
>
> >   /*
> >    * Encode LMUL to lmul as follows:
> >    *     LMUL    vlmul    lmul
> > diff --git a/target/riscv/cpu_bits.h b/target/riscv/cpu_bits.h
> > index 7c87433645..37b622fbfa 100644
> > --- a/target/riscv/cpu_bits.h
> > +++ b/target/riscv/cpu_bits.h
> > @@ -493,6 +493,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        0xFFFFFC00UL
> > +#elif defined(TARGET_RISCV64)
> > +#define PTE_PPN_MASK        0x3FFFFFFFFFFC00ULL
> > +#endif
> > +
>
> No need to define PTE_PPN_MASK for TARGET_RISCV32.

ppn = (pte & PTE_PPN_MASK) >> PTE_PPN_SHIFT;

pte is target_ulong, so types are different.

TARGET_RISCV32: is 32bit.
TARGET_RISCV64: is 64bit.

>
> >   /* Leaf page shift amount */
> >   #define PGSHIFT             12
> >
> > diff --git a/target/riscv/cpu_helper.c b/target/riscv/cpu_helper.c
> > index 327a2c4f1d..2a921bedfd 100644
> > --- a/target/riscv/cpu_helper.c
> > +++ b/target/riscv/cpu_helper.c
> > @@ -622,7 +622,19 @@ restart:
> >               return TRANSLATE_FAIL;
> >           }
> >
> > -        hwaddr ppn = pte >> PTE_PPN_SHIFT;
> > +        hwaddr ppn;
> > +        RISCVCPU *cpu = env_archcpu(env);
> > +
> > +        if (riscv_cpu_sxl(env) == MXL_RV32) {
> > +            ppn = pte >> PTE_PPN_SHIFT;
> > +        } else if (cpu->cfg.ext_svpbmt || cpu->cfg.ext_svnapot) {
> > +            ppn = (pte & PTE_PPN_MASK) >> PTE_PPN_SHIFT;
> > +        } else {
> > +            ppn = pte >> PTE_PPN_SHIFT;
> > +            if ((pte & ~PTE_PPN_MASK) >> PTE_PPN_SHIFT) {
> > +                return TRANSLATE_FAIL;
> > +            }
> > +        }
> >
> >           if (!(pte & PTE_V)) {
> >               /* Invalid PTE */
>
> Otherwise,
>
> Reviewed-by: LIU Zhiwei <zhiwei_liu@c-sky.com>
>
> Thanks,
> Zhiwei
>
>
>
LIU Zhiwei Jan. 25, 2022, 8:54 a.m. UTC | #3
On 2022/1/25 16:40, Guo Ren wrote:
> On Tue, Jan 25, 2022 at 4:34 PM LIU Zhiwei <zhiwei_liu@c-sky.com> wrote:
>>
>> On 2022/1/25 14:45, Weiwei Li 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>
>>> Cc: Liu Zhiwei <zhiwei_liu@c-sky.com>
>>> Cc: Bin Meng <bmeng.cn@gmail.com>
>>> Cc: Alistair Francis <alistair.francis@wdc.com>
>>> ---
>>>    target/riscv/cpu.h        | 13 +++++++++++++
>>>    target/riscv/cpu_bits.h   |  7 +++++++
>>>    target/riscv/cpu_helper.c | 14 +++++++++++++-
>>>    3 files changed, 33 insertions(+), 1 deletion(-)
>>>
>>> diff --git a/target/riscv/cpu.h b/target/riscv/cpu.h
>>> index 55635d68d5..45de8faaca 100644
>>> --- a/target/riscv/cpu.h
>>> +++ b/target/riscv/cpu.h
>>> @@ -341,6 +341,8 @@ struct RISCVCPU {
>>>            bool ext_counters;
>>>            bool ext_ifencei;
>>>            bool ext_icsr;
>>> +        bool ext_svnapot;
>>> +        bool ext_svpbmt;
>>>            bool ext_zfh;
>>>            bool ext_zfhmin;
>>>            bool ext_zve32f;
>>> @@ -495,6 +497,17 @@ static inline int riscv_cpu_xlen(CPURISCVState *env)
>>>        return 16 << env->xl;
>>>    }
>>>
>>> +#ifndef CONFIG_USER_ONLY
>>> +#ifdef TARGET_RISCV32
>>> +#define riscv_cpu_sxl(env)  ((void)(env), MXL_RV32)
>>> +#else
>>> +static inline RISCVMXL riscv_cpu_sxl(CPURISCVState *env)
>>> +{
>>> +    return get_field(env->mstatus, MSTATUS64_SXL);
>>> +}
>>> +#endif
>>> +#endif
>>> +
>> Perhaps an interface also works for user mode is better.
>>
>> +#ifdef TARGET_RISCV32
>> +#define riscv_cpu_sxl(env)  ((void)(env), MXL_RV32)
>> +#else
>> +static inline RISCVMXL riscv_cpu_sxl(CPURISCVState *env)
>> +{
>> +#ifdef CONFIG_USER_ONLY
>> +    return env->misa_mxl;
>> +#else
>> +    return get_field(env->mstatus, MSTATUS64_SXL);
>> +#endif
>> +}
>> +#endif
>> +
>>
>>>    /*
>>>     * Encode LMUL to lmul as follows:
>>>     *     LMUL    vlmul    lmul
>>> diff --git a/target/riscv/cpu_bits.h b/target/riscv/cpu_bits.h
>>> index 7c87433645..37b622fbfa 100644
>>> --- a/target/riscv/cpu_bits.h
>>> +++ b/target/riscv/cpu_bits.h
>>> @@ -493,6 +493,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        0xFFFFFC00UL
>>> +#elif defined(TARGET_RISCV64)
>>> +#define PTE_PPN_MASK        0x3FFFFFFFFFFC00ULL
>>> +#endif
>>> +
>> No need to define PTE_PPN_MASK for TARGET_RISCV32.
> ppn = (pte & PTE_PPN_MASK) >> PTE_PPN_SHIFT;
>
> pte is target_ulong, so types are different.
>
> TARGET_RISCV32: is 32bit.
> TARGET_RISCV64: is 64bit.
>
I should make it more clear.  You will not use PTE_PPN_MASK on 
TARGET_RISCV32.
>>>    /* Leaf page shift amount */
>>>    #define PGSHIFT             12
>>>
>>> diff --git a/target/riscv/cpu_helper.c b/target/riscv/cpu_helper.c
>>> index 327a2c4f1d..2a921bedfd 100644
>>> --- a/target/riscv/cpu_helper.c
>>> +++ b/target/riscv/cpu_helper.c
>>> @@ -622,7 +622,19 @@ restart:
>>>                return TRANSLATE_FAIL;
>>>            }
>>>
>>> -        hwaddr ppn = pte >> PTE_PPN_SHIFT;
>>> +        hwaddr ppn;
>>> +        RISCVCPU *cpu = env_archcpu(env);
>>> +
>>> +        if (riscv_cpu_sxl(env) == MXL_RV32) {
>>> +            ppn = pte >> PTE_PPN_SHIFT;

TARGET_RISCV32 will always come here. So no need to define PTE_PPN_MASK 
for TARGET_RISCV32.

Thanks,
Zhiwei

>>> +        } else if (cpu->cfg.ext_svpbmt || cpu->cfg.ext_svnapot) {
>>> +            ppn = (pte & PTE_PPN_MASK) >> PTE_PPN_SHIFT;
>>> +        } else {
>>> +            ppn = pte >> PTE_PPN_SHIFT;
>>> +            if ((pte & ~PTE_PPN_MASK) >> PTE_PPN_SHIFT) {
>>> +                return TRANSLATE_FAIL;
>>> +            }
>>> +        }
>>>
>>>            if (!(pte & PTE_V)) {
>>>                /* Invalid PTE */
>> Otherwise,
>>
>> Reviewed-by: LIU Zhiwei <zhiwei_liu@c-sky.com>
>>
>> Thanks,
>> Zhiwei
>>
>>
>>
>
Guo Ren Jan. 25, 2022, 9 a.m. UTC | #4
On Tue, Jan 25, 2022 at 4:54 PM LIU Zhiwei <zhiwei_liu@c-sky.com> wrote:
>
>
> On 2022/1/25 16:40, Guo Ren wrote:
> > On Tue, Jan 25, 2022 at 4:34 PM LIU Zhiwei <zhiwei_liu@c-sky.com> wrote:
> >>
> >> On 2022/1/25 14:45, Weiwei Li 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>
> >>> Cc: Liu Zhiwei <zhiwei_liu@c-sky.com>
> >>> Cc: Bin Meng <bmeng.cn@gmail.com>
> >>> Cc: Alistair Francis <alistair.francis@wdc.com>
> >>> ---
> >>>    target/riscv/cpu.h        | 13 +++++++++++++
> >>>    target/riscv/cpu_bits.h   |  7 +++++++
> >>>    target/riscv/cpu_helper.c | 14 +++++++++++++-
> >>>    3 files changed, 33 insertions(+), 1 deletion(-)
> >>>
> >>> diff --git a/target/riscv/cpu.h b/target/riscv/cpu.h
> >>> index 55635d68d5..45de8faaca 100644
> >>> --- a/target/riscv/cpu.h
> >>> +++ b/target/riscv/cpu.h
> >>> @@ -341,6 +341,8 @@ struct RISCVCPU {
> >>>            bool ext_counters;
> >>>            bool ext_ifencei;
> >>>            bool ext_icsr;
> >>> +        bool ext_svnapot;
> >>> +        bool ext_svpbmt;
> >>>            bool ext_zfh;
> >>>            bool ext_zfhmin;
> >>>            bool ext_zve32f;
> >>> @@ -495,6 +497,17 @@ static inline int riscv_cpu_xlen(CPURISCVState *env)
> >>>        return 16 << env->xl;
> >>>    }
> >>>
> >>> +#ifndef CONFIG_USER_ONLY
> >>> +#ifdef TARGET_RISCV32
> >>> +#define riscv_cpu_sxl(env)  ((void)(env), MXL_RV32)
> >>> +#else
> >>> +static inline RISCVMXL riscv_cpu_sxl(CPURISCVState *env)
> >>> +{
> >>> +    return get_field(env->mstatus, MSTATUS64_SXL);
> >>> +}
> >>> +#endif
> >>> +#endif
> >>> +
> >> Perhaps an interface also works for user mode is better.
> >>
> >> +#ifdef TARGET_RISCV32
> >> +#define riscv_cpu_sxl(env)  ((void)(env), MXL_RV32)
> >> +#else
> >> +static inline RISCVMXL riscv_cpu_sxl(CPURISCVState *env)
> >> +{
> >> +#ifdef CONFIG_USER_ONLY
> >> +    return env->misa_mxl;
> >> +#else
> >> +    return get_field(env->mstatus, MSTATUS64_SXL);
> >> +#endif
> >> +}
> >> +#endif
> >> +
> >>
> >>>    /*
> >>>     * Encode LMUL to lmul as follows:
> >>>     *     LMUL    vlmul    lmul
> >>> diff --git a/target/riscv/cpu_bits.h b/target/riscv/cpu_bits.h
> >>> index 7c87433645..37b622fbfa 100644
> >>> --- a/target/riscv/cpu_bits.h
> >>> +++ b/target/riscv/cpu_bits.h
> >>> @@ -493,6 +493,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        0xFFFFFC00UL
> >>> +#elif defined(TARGET_RISCV64)
> >>> +#define PTE_PPN_MASK        0x3FFFFFFFFFFC00ULL
> >>> +#endif
> >>> +
> >> No need to define PTE_PPN_MASK for TARGET_RISCV32.
> > ppn = (pte & PTE_PPN_MASK) >> PTE_PPN_SHIFT;
> >
> > pte is target_ulong, so types are different.
> >
> > TARGET_RISCV32: is 32bit.
> > TARGET_RISCV64: is 64bit.
> >
> I should make it more clear.  You will not use PTE_PPN_MASK on
> TARGET_RISCV32.
> >>>    /* Leaf page shift amount */
> >>>    #define PGSHIFT             12
> >>>
> >>> diff --git a/target/riscv/cpu_helper.c b/target/riscv/cpu_helper.c
> >>> index 327a2c4f1d..2a921bedfd 100644
> >>> --- a/target/riscv/cpu_helper.c
> >>> +++ b/target/riscv/cpu_helper.c
> >>> @@ -622,7 +622,19 @@ restart:
> >>>                return TRANSLATE_FAIL;
> >>>            }
> >>>
> >>> -        hwaddr ppn = pte >> PTE_PPN_SHIFT;
> >>> +        hwaddr ppn;
> >>> +        RISCVCPU *cpu = env_archcpu(env);
> >>> +
> >>> +        if (riscv_cpu_sxl(env) == MXL_RV32) {
> >>> +            ppn = pte >> PTE_PPN_SHIFT;
>
> TARGET_RISCV32 will always come here. So no need to define PTE_PPN_MASK
> for TARGET_RISCV32.
Oops, maybe we should use TARGET_LONG_SIZE == 4

#if TARGET_LONG_SIZE == 4
typedef int32_t target_long;
typedef uint32_t target_ulong;
#define TARGET_FMT_lx "%08x"
#define TARGET_FMT_ld "%d"
#define TARGET_FMT_lu "%u"
#elif TARGET_LONG_SIZE == 8
typedef int64_t target_long;
typedef uint64_t target_ulong;
#define TARGET_FMT_lx "%016" PRIx64
#define TARGET_FMT_ld "%" PRId64
#define TARGET_FMT_lu "%" PRIu64
#else
#error TARGET_LONG_SIZE undefined
#endif

>
> Thanks,
> Zhiwei
>
> >>> +        } else if (cpu->cfg.ext_svpbmt || cpu->cfg.ext_svnapot) {
> >>> +            ppn = (pte & PTE_PPN_MASK) >> PTE_PPN_SHIFT;
> >>> +        } else {
> >>> +            ppn = pte >> PTE_PPN_SHIFT;
> >>> +            if ((pte & ~PTE_PPN_MASK) >> PTE_PPN_SHIFT) {
> >>> +                return TRANSLATE_FAIL;
> >>> +            }
> >>> +        }
> >>>
> >>>            if (!(pte & PTE_V)) {
> >>>                /* Invalid PTE */
> >> Otherwise,
> >>
> >> Reviewed-by: LIU Zhiwei <zhiwei_liu@c-sky.com>
> >>
> >> Thanks,
> >> Zhiwei
> >>
> >>
> >>
> >
Weiwei Li Jan. 25, 2022, 9:44 a.m. UTC | #5
在 2022/1/25 下午5:00, Guo Ren 写道:
> On Tue, Jan 25, 2022 at 4:54 PM LIU Zhiwei <zhiwei_liu@c-sky.com> wrote:
>>
>> On 2022/1/25 16:40, Guo Ren wrote:
>>> On Tue, Jan 25, 2022 at 4:34 PM LIU Zhiwei <zhiwei_liu@c-sky.com> wrote:
>>>> On 2022/1/25 14:45, Weiwei Li 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>
>>>>> Cc: Liu Zhiwei <zhiwei_liu@c-sky.com>
>>>>> Cc: Bin Meng <bmeng.cn@gmail.com>
>>>>> Cc: Alistair Francis <alistair.francis@wdc.com>
>>>>> ---
>>>>>     target/riscv/cpu.h        | 13 +++++++++++++
>>>>>     target/riscv/cpu_bits.h   |  7 +++++++
>>>>>     target/riscv/cpu_helper.c | 14 +++++++++++++-
>>>>>     3 files changed, 33 insertions(+), 1 deletion(-)
>>>>>
>>>>> diff --git a/target/riscv/cpu.h b/target/riscv/cpu.h
>>>>> index 55635d68d5..45de8faaca 100644
>>>>> --- a/target/riscv/cpu.h
>>>>> +++ b/target/riscv/cpu.h
>>>>> @@ -341,6 +341,8 @@ struct RISCVCPU {
>>>>>             bool ext_counters;
>>>>>             bool ext_ifencei;
>>>>>             bool ext_icsr;
>>>>> +        bool ext_svnapot;
>>>>> +        bool ext_svpbmt;
>>>>>             bool ext_zfh;
>>>>>             bool ext_zfhmin;
>>>>>             bool ext_zve32f;
>>>>> @@ -495,6 +497,17 @@ static inline int riscv_cpu_xlen(CPURISCVState *env)
>>>>>         return 16 << env->xl;
>>>>>     }
>>>>>
>>>>> +#ifndef CONFIG_USER_ONLY
>>>>> +#ifdef TARGET_RISCV32
>>>>> +#define riscv_cpu_sxl(env)  ((void)(env), MXL_RV32)
>>>>> +#else
>>>>> +static inline RISCVMXL riscv_cpu_sxl(CPURISCVState *env)
>>>>> +{
>>>>> +    return get_field(env->mstatus, MSTATUS64_SXL);
>>>>> +}
>>>>> +#endif
>>>>> +#endif
>>>>> +
>>>> Perhaps an interface also works for user mode is better.
>>>>
>>>> +#ifdef TARGET_RISCV32
>>>> +#define riscv_cpu_sxl(env)  ((void)(env), MXL_RV32)
>>>> +#else
>>>> +static inline RISCVMXL riscv_cpu_sxl(CPURISCVState *env)
>>>> +{
>>>> +#ifdef CONFIG_USER_ONLY
>>>> +    return env->misa_mxl;
>>>> +#else
>>>> +    return get_field(env->mstatus, MSTATUS64_SXL);
>>>> +#endif
>>>> +}
>>>> +#endif
>>>> +
>>>>
>>>>>     /*
>>>>>      * Encode LMUL to lmul as follows:
>>>>>      *     LMUL    vlmul    lmul
>>>>> diff --git a/target/riscv/cpu_bits.h b/target/riscv/cpu_bits.h
>>>>> index 7c87433645..37b622fbfa 100644
>>>>> --- a/target/riscv/cpu_bits.h
>>>>> +++ b/target/riscv/cpu_bits.h
>>>>> @@ -493,6 +493,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        0xFFFFFC00UL
>>>>> +#elif defined(TARGET_RISCV64)
>>>>> +#define PTE_PPN_MASK        0x3FFFFFFFFFFC00ULL
>>>>> +#endif
>>>>> +
>>>> No need to define PTE_PPN_MASK for TARGET_RISCV32.
>>> ppn = (pte & PTE_PPN_MASK) >> PTE_PPN_SHIFT;
>>>
>>> pte is target_ulong, so types are different.
>>>
>>> TARGET_RISCV32: is 32bit.
>>> TARGET_RISCV64: is 64bit.
>>>
>> I should make it more clear.  You will not use PTE_PPN_MASK on
>> TARGET_RISCV32.
>>>>>     /* Leaf page shift amount */
>>>>>     #define PGSHIFT             12
>>>>>
>>>>> diff --git a/target/riscv/cpu_helper.c b/target/riscv/cpu_helper.c
>>>>> index 327a2c4f1d..2a921bedfd 100644
>>>>> --- a/target/riscv/cpu_helper.c
>>>>> +++ b/target/riscv/cpu_helper.c
>>>>> @@ -622,7 +622,19 @@ restart:
>>>>>                 return TRANSLATE_FAIL;
>>>>>             }
>>>>>
>>>>> -        hwaddr ppn = pte >> PTE_PPN_SHIFT;
>>>>> +        hwaddr ppn;
>>>>> +        RISCVCPU *cpu = env_archcpu(env);
>>>>> +
>>>>> +        if (riscv_cpu_sxl(env) == MXL_RV32) {
>>>>> +            ppn = pte >> PTE_PPN_SHIFT;
>> TARGET_RISCV32 will always come here. So no need to define PTE_PPN_MASK
>> for TARGET_RISCV32.
> Oops, maybe we should use TARGET_LONG_SIZE == 4
>
> #if TARGET_LONG_SIZE == 4
> typedef int32_t target_long;
> typedef uint32_t target_ulong;
> #define TARGET_FMT_lx "%08x"
> #define TARGET_FMT_ld "%d"
> #define TARGET_FMT_lu "%u"
> #elif TARGET_LONG_SIZE == 8
> typedef int64_t target_long;
> typedef uint64_t target_ulong;
> #define TARGET_FMT_lx "%016" PRIx64
> #define TARGET_FMT_ld "%" PRId64
> #define TARGET_FMT_lu "%" PRIu64
> #else
> #error TARGET_LONG_SIZE undefined
> #endif
>
TARGET_LONG_SIZE is related to TARGET_RISCV32 and TARGET_RISCV64.

In RV32, the code will truely not reach there when executing. However the code itself have different types for pte and PTE_PPN_MASK, and may cause compiler warning.

So if we only define PTE_PPN_MASK for RV64, maybe we can take type casting here:

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

Regards,
Weiwei Li

>> Thanks,
>> Zhiwei
>>
>>>>> +        } else if (cpu->cfg.ext_svpbmt || cpu->cfg.ext_svnapot) {
>>>>> +            ppn = (pte & PTE_PPN_MASK) >> PTE_PPN_SHIFT;
>>>>> +        } else {
>>>>> +            ppn = pte >> PTE_PPN_SHIFT;
>>>>> +            if ((pte & ~PTE_PPN_MASK) >> PTE_PPN_SHIFT) {
>>>>> +                return TRANSLATE_FAIL;
>>>>> +            }
>>>>> +        }
>>>>>
>>>>>             if (!(pte & PTE_V)) {
>>>>>                 /* Invalid PTE */
>>>> Otherwise,
>>>>
>>>> Reviewed-by: LIU Zhiwei <zhiwei_liu@c-sky.com>
>>>>
>>>> Thanks,
>>>> Zhiwei
>>>>
>>>>
>>>>
>
>
Guo Ren Jan. 28, 2022, 3:56 a.m. UTC | #6
On Tue, Jan 25, 2022 at 5:49 PM Weiwei Li <liweiwei@iscas.ac.cn> wrote:
>
>
> 在 2022/1/25 下午5:00, Guo Ren 写道:
> > On Tue, Jan 25, 2022 at 4:54 PM LIU Zhiwei <zhiwei_liu@c-sky.com> wrote:
> >>
> >> On 2022/1/25 16:40, Guo Ren wrote:
> >>> On Tue, Jan 25, 2022 at 4:34 PM LIU Zhiwei <zhiwei_liu@c-sky.com> wrote:
> >>>> On 2022/1/25 14:45, Weiwei Li 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>
> >>>>> Cc: Liu Zhiwei <zhiwei_liu@c-sky.com>
> >>>>> Cc: Bin Meng <bmeng.cn@gmail.com>
> >>>>> Cc: Alistair Francis <alistair.francis@wdc.com>
> >>>>> ---
> >>>>>     target/riscv/cpu.h        | 13 +++++++++++++
> >>>>>     target/riscv/cpu_bits.h   |  7 +++++++
> >>>>>     target/riscv/cpu_helper.c | 14 +++++++++++++-
> >>>>>     3 files changed, 33 insertions(+), 1 deletion(-)
> >>>>>
> >>>>> diff --git a/target/riscv/cpu.h b/target/riscv/cpu.h
> >>>>> index 55635d68d5..45de8faaca 100644
> >>>>> --- a/target/riscv/cpu.h
> >>>>> +++ b/target/riscv/cpu.h
> >>>>> @@ -341,6 +341,8 @@ struct RISCVCPU {
> >>>>>             bool ext_counters;
> >>>>>             bool ext_ifencei;
> >>>>>             bool ext_icsr;
> >>>>> +        bool ext_svnapot;
> >>>>> +        bool ext_svpbmt;
> >>>>>             bool ext_zfh;
> >>>>>             bool ext_zfhmin;
> >>>>>             bool ext_zve32f;
> >>>>> @@ -495,6 +497,17 @@ static inline int riscv_cpu_xlen(CPURISCVState *env)
> >>>>>         return 16 << env->xl;
> >>>>>     }
> >>>>>
> >>>>> +#ifndef CONFIG_USER_ONLY
> >>>>> +#ifdef TARGET_RISCV32
> >>>>> +#define riscv_cpu_sxl(env)  ((void)(env), MXL_RV32)
> >>>>> +#else
> >>>>> +static inline RISCVMXL riscv_cpu_sxl(CPURISCVState *env)
> >>>>> +{
> >>>>> +    return get_field(env->mstatus, MSTATUS64_SXL);
> >>>>> +}
> >>>>> +#endif
> >>>>> +#endif
> >>>>> +
> >>>> Perhaps an interface also works for user mode is better.
> >>>>
> >>>> +#ifdef TARGET_RISCV32
> >>>> +#define riscv_cpu_sxl(env)  ((void)(env), MXL_RV32)
> >>>> +#else
> >>>> +static inline RISCVMXL riscv_cpu_sxl(CPURISCVState *env)
> >>>> +{
> >>>> +#ifdef CONFIG_USER_ONLY
> >>>> +    return env->misa_mxl;
> >>>> +#else
> >>>> +    return get_field(env->mstatus, MSTATUS64_SXL);
> >>>> +#endif
> >>>> +}
> >>>> +#endif
> >>>> +
> >>>>
> >>>>>     /*
> >>>>>      * Encode LMUL to lmul as follows:
> >>>>>      *     LMUL    vlmul    lmul
> >>>>> diff --git a/target/riscv/cpu_bits.h b/target/riscv/cpu_bits.h
> >>>>> index 7c87433645..37b622fbfa 100644
> >>>>> --- a/target/riscv/cpu_bits.h
> >>>>> +++ b/target/riscv/cpu_bits.h
> >>>>> @@ -493,6 +493,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        0xFFFFFC00UL
> >>>>> +#elif defined(TARGET_RISCV64)
> >>>>> +#define PTE_PPN_MASK        0x3FFFFFFFFFFC00ULL
> >>>>> +#endif
> >>>>> +
> >>>> No need to define PTE_PPN_MASK for TARGET_RISCV32.
> >>> ppn = (pte & PTE_PPN_MASK) >> PTE_PPN_SHIFT;
> >>>
> >>> pte is target_ulong, so types are different.
> >>>
> >>> TARGET_RISCV32: is 32bit.
> >>> TARGET_RISCV64: is 64bit.
> >>>
> >> I should make it more clear.  You will not use PTE_PPN_MASK on
> >> TARGET_RISCV32.
> >>>>>     /* Leaf page shift amount */
> >>>>>     #define PGSHIFT             12
> >>>>>
> >>>>> diff --git a/target/riscv/cpu_helper.c b/target/riscv/cpu_helper.c
> >>>>> index 327a2c4f1d..2a921bedfd 100644
> >>>>> --- a/target/riscv/cpu_helper.c
> >>>>> +++ b/target/riscv/cpu_helper.c
> >>>>> @@ -622,7 +622,19 @@ restart:
> >>>>>                 return TRANSLATE_FAIL;
> >>>>>             }
> >>>>>
> >>>>> -        hwaddr ppn = pte >> PTE_PPN_SHIFT;
> >>>>> +        hwaddr ppn;
> >>>>> +        RISCVCPU *cpu = env_archcpu(env);
> >>>>> +
> >>>>> +        if (riscv_cpu_sxl(env) == MXL_RV32) {
> >>>>> +            ppn = pte >> PTE_PPN_SHIFT;
> >> TARGET_RISCV32 will always come here. So no need to define PTE_PPN_MASK
> >> for TARGET_RISCV32.
> > Oops, maybe we should use TARGET_LONG_SIZE == 4
> >
> > #if TARGET_LONG_SIZE == 4
> > typedef int32_t target_long;
> > typedef uint32_t target_ulong;
> > #define TARGET_FMT_lx "%08x"
> > #define TARGET_FMT_ld "%d"
> > #define TARGET_FMT_lu "%u"
> > #elif TARGET_LONG_SIZE == 8
> > typedef int64_t target_long;
> > typedef uint64_t target_ulong;
> > #define TARGET_FMT_lx "%016" PRIx64
> > #define TARGET_FMT_ld "%" PRId64
> > #define TARGET_FMT_lu "%" PRIu64
> > #else
> > #error TARGET_LONG_SIZE undefined
> > #endif
> >
> TARGET_LONG_SIZE is related to TARGET_RISCV32 and TARGET_RISCV64.
>
> In RV32, the code will truely not reach there when executing. However the code itself have different types for pte and PTE_PPN_MASK, and may cause compiler  warning.
>
> So if we only define PTE_PPN_MASK for RV64, maybe we can take type casting here:
>
>    ppn = (pte & (target_ulong)PTE_PPN_MASK) >> PTE_PPN_SHIFT;

 (target_ulong)PTE_PPN_MASK seems (u32) 0x3FFFFFFFFFFC00ULL = 0xFFFFFC00UL

I'm okay with the above.

>
> Regards,
> Weiwei Li
>
> >> Thanks,
> >> Zhiwei
> >>
> >>>>> +        } else if (cpu->cfg.ext_svpbmt || cpu->cfg.ext_svnapot) {
> >>>>> +            ppn = (pte & PTE_PPN_MASK) >> PTE_PPN_SHIFT;
> >>>>> +        } else {
> >>>>> +            ppn = pte >> PTE_PPN_SHIFT;
> >>>>> +            if ((pte & ~PTE_PPN_MASK) >> PTE_PPN_SHIFT) {
> >>>>> +                return TRANSLATE_FAIL;
> >>>>> +            }
> >>>>> +        }
> >>>>>
> >>>>>             if (!(pte & PTE_V)) {
> >>>>>                 /* Invalid PTE */
> >>>> Otherwise,
> >>>>
> >>>> Reviewed-by: LIU Zhiwei <zhiwei_liu@c-sky.com>
> >>>>
> >>>> Thanks,
> >>>> Zhiwei
> >>>>
> >>>>
> >>>>
> >
> >
>
>
diff mbox series

Patch

diff --git a/target/riscv/cpu.h b/target/riscv/cpu.h
index 55635d68d5..45de8faaca 100644
--- a/target/riscv/cpu.h
+++ b/target/riscv/cpu.h
@@ -341,6 +341,8 @@  struct RISCVCPU {
         bool ext_counters;
         bool ext_ifencei;
         bool ext_icsr;
+        bool ext_svnapot;
+        bool ext_svpbmt;
         bool ext_zfh;
         bool ext_zfhmin;
         bool ext_zve32f;
@@ -495,6 +497,17 @@  static inline int riscv_cpu_xlen(CPURISCVState *env)
     return 16 << env->xl;
 }
 
+#ifndef CONFIG_USER_ONLY
+#ifdef TARGET_RISCV32
+#define riscv_cpu_sxl(env)  ((void)(env), MXL_RV32)
+#else
+static inline RISCVMXL riscv_cpu_sxl(CPURISCVState *env)
+{
+    return get_field(env->mstatus, MSTATUS64_SXL);
+}
+#endif
+#endif
+
 /*
  * Encode LMUL to lmul as follows:
  *     LMUL    vlmul    lmul
diff --git a/target/riscv/cpu_bits.h b/target/riscv/cpu_bits.h
index 7c87433645..37b622fbfa 100644
--- a/target/riscv/cpu_bits.h
+++ b/target/riscv/cpu_bits.h
@@ -493,6 +493,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        0xFFFFFC00UL
+#elif defined(TARGET_RISCV64)
+#define PTE_PPN_MASK        0x3FFFFFFFFFFC00ULL
+#endif
+
 /* Leaf page shift amount */
 #define PGSHIFT             12
 
diff --git a/target/riscv/cpu_helper.c b/target/riscv/cpu_helper.c
index 327a2c4f1d..2a921bedfd 100644
--- a/target/riscv/cpu_helper.c
+++ b/target/riscv/cpu_helper.c
@@ -622,7 +622,19 @@  restart:
             return TRANSLATE_FAIL;
         }
 
-        hwaddr ppn = pte >> PTE_PPN_SHIFT;
+        hwaddr ppn;
+        RISCVCPU *cpu = env_archcpu(env);
+
+        if (riscv_cpu_sxl(env) == MXL_RV32) {
+            ppn = pte >> PTE_PPN_SHIFT;
+        } else if (cpu->cfg.ext_svpbmt || cpu->cfg.ext_svnapot) {
+            ppn = (pte & PTE_PPN_MASK) >> PTE_PPN_SHIFT;
+        } else {
+            ppn = pte >> PTE_PPN_SHIFT;
+            if ((pte & ~PTE_PPN_MASK) >> PTE_PPN_SHIFT) {
+                return TRANSLATE_FAIL;
+            }
+        }
 
         if (!(pte & PTE_V)) {
             /* Invalid PTE */