Re: [RFC 4/5] target/riscv: Add *envcfg* CSRs support

[Atish Kumar Patra <atishp@rivosinc.com>]

[-- Attachment #1: Type: text/plain, Size: 11111 bytes --]

On Fri, Jan 28, 2022 at 5:50 PM angell1518 <angell1518@163.com> wrote:

>
> 在 2022/1/29 上午9:28, Atish Patra 写道:
>
>
>
> On Wed, Jan 26, 2022 at 12:37 AM Weiwei Li <liweiwei@iscas.ac.cn> wrote:
>
>>
>> 在 2022/1/21 上午4:07, Atish Patra 写道:
>> > The RISC-V privileged specification v1.12 defines few execution
>> > environment configuration CSRs that can be used enable/disable
>> > extensions per privilege levels.
>> >
>> > Add the basic support for these CSRs.
>> >
>> > Signed-off-by: Atish Patra <atishp@rivosinc.com>
>> > ---
>> >   target/riscv/cpu.h      |  8 ++++
>> >   target/riscv/cpu_bits.h | 31 +++++++++++++++
>> >   target/riscv/csr.c      | 84 +++++++++++++++++++++++++++++++++++++++++
>> >   target/riscv/machine.c  | 26 +++++++++++++
>> >   4 files changed, 149 insertions(+)
>> >
>> > diff --git a/target/riscv/cpu.h b/target/riscv/cpu.h
>> > index 7f87917204c5..b9462300a472 100644
>> > --- a/target/riscv/cpu.h
>> > +++ b/target/riscv/cpu.h
>> > @@ -264,6 +264,14 @@ struct CPURISCVState {
>> >       target_ulong spmbase;
>> >       target_ulong upmmask;
>> >       target_ulong upmbase;
>> > +
>> > +    /* CSRs for execution enviornment configuration */
>> > +
>> > +    target_ulong menvcfg;
>> > +    target_ulong menvcfgh;
>>
>> I think we needn't maintain seperate menvcfg and menvcfgh, just use
>> "uint64_t menvcfg" as the way of mstatus.
>>
>>
> unlike mstatush, menvcfgh/henvcfgh will be accessed directly to do runtime
> predicate for stimecmp/vstimecmp.
>
> We have to do the 32 bit shifting during every check which makes the code
> hard to read
> at the cost of 2 ulongs.
>
> IMO, having separate variables is much simpler.
>
> Do you mean check STCE/VSTCE bit in menvcfg/henvcfg?
>
> If so, I think use a simple "uint64_t menvcfg/henvcfg" may be better,
> then we can only check the 63 bit of them.
>

Which is a bit confusing as the STCE bit in mencfgh/henvcfgh is 31 not 63.
But that's my personal preference.

I will just leave a comment to clarify the confusion for now. I will send a
patch with unified menvcfg and wait for others's feedback.

> Or we should decide where to get this bit from(mencvfg/henvcfg, or
> mencfgh/henvcfgh) based on the MXLEN/HSXLEN.
>
> Regards,
>
> Weiwei Li
>
>
> Similar to  henvcfg and henvcfg.
>>
>> > +    target_ulong senvcfg;
>> > +    target_ulong henvcfg;
>> > +    target_ulong henvcfgh;
>> >   #endif
>> >
>> >       float_status fp_status;
>> > diff --git a/target/riscv/cpu_bits.h b/target/riscv/cpu_bits.h
>> > index f6f90b5cbd52..afb237c2313b 100644
>> > --- a/target/riscv/cpu_bits.h
>> > +++ b/target/riscv/cpu_bits.h
>> > @@ -177,6 +177,9 @@
>> >   #define CSR_STVEC           0x105
>> >   #define CSR_SCOUNTEREN      0x106
>> >
>> > +/* Supervisor Configuration CSRs */
>> > +#define CSR_SENVCFG         0x10A
>> > +
>> >   /* Supervisor Trap Handling */
>> >   #define CSR_SSCRATCH        0x140
>> >   #define CSR_SEPC            0x141
>> > @@ -204,6 +207,10 @@
>> >   #define CSR_HTIMEDELTA      0x605
>> >   #define CSR_HTIMEDELTAH     0x615
>> >
>> > +/* Hypervisor Configuration CSRs */
>> > +#define CSR_HENVCFG         0x60A
>> > +#define CSR_HENVCFGH        0x61A
>> > +
>> >   /* Virtual CSRs */
>> >   #define CSR_VSSTATUS        0x200
>> >   #define CSR_VSIE            0x204
>> > @@ -218,6 +225,10 @@
>> >   #define CSR_MTINST          0x34a
>> >   #define CSR_MTVAL2          0x34b
>> >
>> > +/* Machine Configuration CSRs */
>> > +#define CSR_MENVCFG         0x30A
>> > +#define CSR_MENVCFGH        0x31A
>> > +
>> >   /* Enhanced Physical Memory Protection (ePMP) */
>> >   #define CSR_MSECCFG         0x747
>> >   #define CSR_MSECCFGH        0x757
>> > @@ -578,6 +589,26 @@ typedef enum RISCVException {
>> >   #define PM_EXT_CLEAN    0x00000002ULL
>> >   #define PM_EXT_DIRTY    0x00000003ULL
>> >
>> > +/* Execution enviornment configuration bits */
>> > +#define MENVCFG_FIOM                       (1 << 0)
>> > +#define MENVCFG_CBE                        0x30000ULL
>> > +#define MENVCFG_CBCFE                      (1 << 6)
>> > +#define MENVCFG_CBZE                       (1 << 7)
>> > +#define MENVCFG_PBMTE                      (1 << 62)
>> > +#define MENVCFG_STCE                       (1 << 63)
>> > +
>> > +#define SENVCFG_FIOM                       MENVCFG_FIOM
>> > +#define SENVCFG_CBE                        MENVCFG_CBE
>> > +#define SENVCFG_CBCFE                      MENVCFG_CBCFE
>> > +#define SENVCFG_CBZE                       MENVCFG_CBZE
>> > +
>> > +#define HENVCFG_FIOM                       MENVCFG_FIOM
>> > +#define HENVCFG_CBE                        MENVCFG_CBE
>> > +#define HENVCFG_CBCFE                      MENVCFG_CBCFE
>> > +#define HENVCFG_CBZE                       MENVCFG_CBZE
>> > +#define HENVCFG_PBMTE                      MENVCFG_PBMTE
>> > +#define HENVCFG_STCE                       MENVCFG_STCE
>> > +
>> >   /* Offsets for every pair of control bits per each priv level */
>> >   #define XS_OFFSET    0ULL
>> >   #define U_OFFSET     2ULL
>> > diff --git a/target/riscv/csr.c b/target/riscv/csr.c
>> > index e66bf2201857..a4bbae7a1bbd 100644
>> > --- a/target/riscv/csr.c
>> > +++ b/target/riscv/csr.c
>> > @@ -853,6 +853,77 @@ static RISCVException write_mtval(CPURISCVState
>> *env, int csrno,
>> >       return RISCV_EXCP_NONE;
>> >   }
>> >
>> > +/* Execution environment configuration setup */
>> > +static RISCVException read_menvcfg(CPURISCVState *env, int csrno,
>> > +                                 target_ulong *val)
>> > +{
>> > +    *val = env->menvcfg;
>> > +    return RISCV_EXCP_NONE;
>> > +}
>> > +
>> > +static RISCVException write_menvcfg(CPURISCVState *env, int csrno,
>> > +                                  target_ulong val)
>> > +{
>> > +    env->menvcfg = val;
>> > +    return RISCV_EXCP_NONE;
>> > +}
>> > +
>> > +static RISCVException read_menvcfgh(CPURISCVState *env, int csrno,
>> > +                                 target_ulong *val)
>> > +{
>> > +    *val = env->menvcfgh;
>> > +    return RISCV_EXCP_NONE;
>> > +}
>> > +
>> > +static RISCVException write_menvcfgh(CPURISCVState *env, int csrno,
>> > +                                  target_ulong val)
>> > +{
>> > +    env->menvcfgh = val;
>> > +    return RISCV_EXCP_NONE;
>> > +}
>> > +
>> > +static RISCVException read_senvcfg(CPURISCVState *env, int csrno,
>> > +                                 target_ulong *val)
>> > +{
>> > +    *val = env->senvcfg;
>> > +    return RISCV_EXCP_NONE;
>> > +}
>> > +
>> > +static RISCVException write_senvcfg(CPURISCVState *env, int csrno,
>> > +                                  target_ulong val)
>> > +{
>> > +    env->senvcfg = val;
>> > +    return RISCV_EXCP_NONE;
>> > +}
>> > +
>> > +static RISCVException read_henvcfg(CPURISCVState *env, int csrno,
>> > +                                 target_ulong *val)
>> > +{
>> > +    *val = env->henvcfg;
>> > +    return RISCV_EXCP_NONE;
>> > +}
>> > +
>> > +static RISCVException write_henvcfg(CPURISCVState *env, int csrno,
>> > +                                  target_ulong val)
>> > +{
>> > +    env->henvcfg = val;
>> > +    return RISCV_EXCP_NONE;
>> > +}
>> > +
>> > +static RISCVException read_henvcfgh(CPURISCVState *env, int csrno,
>> > +                                 target_ulong *val)
>> > +{
>> > +    *val = env->henvcfgh;
>> > +    return RISCV_EXCP_NONE;
>> > +}
>> > +
>> > +static RISCVException write_henvcfgh(CPURISCVState *env, int csrno,
>> > +                                  target_ulong val)
>> > +{
>> > +    env->henvcfgh = val;
>> > +    return RISCV_EXCP_NONE;
>> > +}
>> > +
>> >   static RISCVException rmw_mip(CPURISCVState *env, int csrno,
>> >                                 target_ulong *ret_value,
>> >                                 target_ulong new_value, target_ulong
>> write_mask)
>> > @@ -2054,6 +2125,19 @@ riscv_csr_operations csr_ops[CSR_TABLE_SIZE] = {
>> >       [CSR_MTVAL]    = { "mtval",    any,  read_mtval,    write_mtval
>>   },
>> >       [CSR_MIP]      = { "mip",      any,  NULL,    NULL, rmw_mip
>>   },
>> >
>> > +    /* Execution environment configuration */
>> > +    [CSR_MENVCFG]  = { "menvcfg",  any,     read_menvcfg,
>> write_menvcfg, NULL,
>> > +                                            NULL, NULL,
>> PRIV_VERSION_1_12_0},
>> > +    [CSR_MENVCFGH] = { "menvcfgh", any32,   read_menvcfgh,
>> write_menvcfgh, NULL,
>> > +                                            NULL, NULL,
>> PRIV_VERSION_1_12_0},
>> > +    [CSR_SENVCFG]  = { "senvcfg",  smode,   read_senvcfg,
>> write_senvcfg, NULL,
>> > +                                            NULL, NULL,
>> PRIV_VERSION_1_12_0},
>> > +    [CSR_HENVCFG]  = { "henvcfg",  hmode,   read_henvcfg,
>> write_henvcfg, NULL,
>> > +                                            NULL, NULL,
>> PRIV_VERSION_1_12_0},
>> > +    [CSR_HENVCFGH] = { "henvcfgh", hmode32, read_henvcfgh,
>> write_henvcfgh, NULL,
>> > +                                            NULL, NULL,
>> PRIV_VERSION_1_12_0},
>> > +
>> > +
>> Two new lines here.
>>
>
> Fixed it. Thanks.
>
>
>> >       /* Supervisor Trap Setup */
>> >       [CSR_SSTATUS]    = { "sstatus",    smode, read_sstatus,
>> write_sstatus, NULL,
>> >                                                 read_sstatus_i128
>>            },
>> > diff --git a/target/riscv/machine.c b/target/riscv/machine.c
>> > index 13b9ab375b95..59479a999b87 100644
>> > --- a/target/riscv/machine.c
>> > +++ b/target/riscv/machine.c
>> > @@ -185,6 +185,31 @@ static const VMStateDescription vmstate_rv128 = {
>> >       }
>> >   };
>> >
>> > +/* TODO: henvcfg need both hyper_needed & envcfg_needed */
>> > +static bool envcfg_needed(void *opaque)
>> > +{
>> > +    RISCVCPU *cpu = opaque;
>> > +    CPURISCVState *env = &cpu->env;
>> > +
>> > +    return (env->priv_ver >= PRIV_VERSION_1_12_0 ? 1 : 0);
>> > +}
>> > +
>> > +static const VMStateDescription vmstate_envcfg = {
>> > +    .name = "cpu/envcfg",
>> > +    .version_id = 1,
>> > +    .minimum_version_id = 1,
>> > +    .needed = envcfg_needed,
>> > +    .fields = (VMStateField[]) {
>> > +        VMSTATE_UINTTL(env.menvcfg, RISCVCPU),
>> > +        VMSTATE_UINTTL(env.menvcfgh, RISCVCPU),
>> > +        VMSTATE_UINTTL(env.senvcfg, RISCVCPU),
>> > +        VMSTATE_UINTTL(env.henvcfg, RISCVCPU),
>> > +        VMSTATE_UINTTL(env.henvcfgh, RISCVCPU),
>> > +
>> > +        VMSTATE_END_OF_LIST()
>> > +    }
>> > +};
>> > +
>> >   const VMStateDescription vmstate_riscv_cpu = {
>> >       .name = "cpu",
>> >       .version_id = 3,
>> > @@ -240,6 +265,7 @@ const VMStateDescription vmstate_riscv_cpu = {
>> >           &vmstate_vector,
>> >           &vmstate_pointermasking,
>> >           &vmstate_rv128,
>> > +        &vmstate_envcfg,
>> >           NULL
>> >       }
>> >   };
>>
>> Regards,
>>
>> Weiwei Li
>>
>>
>>
>
> --
> Regards,
> Atish
>
>

[-- Attachment #2: Type: text/html, Size: 19081 bytes --]