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

[angell1518 <angell1518@163.com>]

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


在 2022/1/29 上午9:28, Atish Patra 写道:
>
>
> On Wed, Jan 26, 2022 at 12:37 AM Weiwei Li <liweiwei@iscas.ac.cn 
> <mailto: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
>     <mailto: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.

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: 18263 bytes --]