Re: [PATCH v9 12/14] target/riscv: rvk: add CSR support for Zkr

[Alistair Francis]

On Mon, Apr 11, 2022 at 2:46 PM Weiwei Li <liweiwei@iscas.ac.cn> wrote:
>
> Hi, any comments on this patch or patchset?
>
> Currently, read-only instruction to access Seed CSR is checked as a
> special case in helper_csrr as suggested in
>
> https://lists.nongnu.org/archive/html/qemu-riscv/2022-03/msg00146.html.

Ah sorry, I didn't realise you had updated this.

>
> (The new version for that patch is in
> https://lists.nongnu.org/archive/html/qemu-riscv/2022-03/msg00156.html)
>
> Regards,
>
> Weiwei Li
>
> 在 2022/3/18 下午12:19, Weiwei Li 写道:
> >   - add SEED CSR which must be accessed with a read-write instruction:
> >     A read-only instruction such as CSRRS/CSRRC with rs1=x0 or CSRRSI/CSRRCI
> > with uimm=0 will raise an illegal instruction exception.
> >   - add USEED, SSEED fields for MSECCFG CSR
> >
> > Co-authored-by: Ruibo Lu <luruibo2000@163.com>
> > Co-authored-by: Zewen Ye <lustrew@foxmail.com>
> > Signed-off-by: Weiwei Li <liweiwei@iscas.ac.cn>
> > Signed-off-by: Junqiang Wang <wangjunqiang@iscas.ac.cn>
> > ---
> >   target/riscv/cpu_bits.h  |  9 ++++++
> >   target/riscv/csr.c       | 68 ++++++++++++++++++++++++++++++++++++++++
> >   target/riscv/op_helper.c |  9 ++++++
> >   target/riscv/pmp.h       |  8 +++--
> >   4 files changed, 91 insertions(+), 3 deletions(-)
> >
> > diff --git a/target/riscv/cpu_bits.h b/target/riscv/cpu_bits.h
> > index bb47cf7e77..d401100f47 100644
> > --- a/target/riscv/cpu_bits.h
> > +++ b/target/riscv/cpu_bits.h
> > @@ -458,6 +458,9 @@
> >   #define CSR_VSPMMASK        0x2c1
> >   #define CSR_VSPMBASE        0x2c2
> >
> > +/* Crypto Extension */
> > +#define CSR_SEED            0x015
> > +
> >   /* mstatus CSR bits */
> >   #define MSTATUS_UIE         0x00000001
> >   #define MSTATUS_SIE         0x00000002
> > @@ -800,4 +803,10 @@ typedef enum RISCVException {
> >   #define HVICTL_VALID_MASK                  \
> >       (HVICTL_VTI | HVICTL_IID | HVICTL_IPRIOM | HVICTL_IPRIO)
> >
> > +/* seed CSR bits */
> > +#define SEED_OPST                        (0b11 << 30)
> > +#define SEED_OPST_BIST                   (0b00 << 30)
> > +#define SEED_OPST_WAIT                   (0b01 << 30)
> > +#define SEED_OPST_ES16                   (0b10 << 30)
> > +#define SEED_OPST_DEAD                   (0b11 << 30)
> >   #endif
> > diff --git a/target/riscv/csr.c b/target/riscv/csr.c
> > index 3c61dd69af..5717a51f56 100644
> > --- a/target/riscv/csr.c
> > +++ b/target/riscv/csr.c
> > @@ -24,6 +24,8 @@
> >   #include "qemu/main-loop.h"
> >   #include "exec/exec-all.h"
> >   #include "sysemu/cpu-timers.h"
> > +#include "qemu/guest-random.h"
> > +#include "qapi/error.h"
> >
> >   /* CSR function table public API */
> >   void riscv_get_csr_ops(int csrno, riscv_csr_operations *ops)
> > @@ -292,6 +294,40 @@ static RISCVException epmp(CPURISCVState *env, int 
> > csrno)
> >   }
> >   #endif
> >
> > +static RISCVException seed(CPURISCVState *env, int csrno)
> > +{
> > +    RISCVCPU *cpu = env_archcpu(env);
> > +
> > +    if (!cpu->cfg.ext_zkr) {
> > +        return RISCV_EXCP_ILLEGAL_INST;
> > +    }
> > +
> > +#if !defined(CONFIG_USER_ONLY)
> > +    if (riscv_has_ext(env, RVS) && riscv_has_ext(env, RVH)) {
> > +        /* Hypervisor extension is supported */
> > +        if (riscv_cpu_virt_enabled(env) && (env->priv != PRV_M)) {

You can simplify this to just riscv_cpu_virt_enabled(). You don't need
to check if we have the extension as well.

> > +            if (env->mseccfg & MSECCFG_SSEED) {
> > +                return RISCV_EXCP_VIRT_INSTRUCTION_FAULT;
> > +            } else {
> > +                return RISCV_EXCP_ILLEGAL_INST;
> > +            }
> > +        }
> > +    }
> > +
> > +    if (env->priv == PRV_M) {
> > +        return RISCV_EXCP_NONE;
> > +    } else if (env->priv == PRV_S && (env->mseccfg & MSECCFG_SSEED)) {
> > +        return RISCV_EXCP_NONE;
> > +    } else if (env->priv == PRV_U && (env->mseccfg & MSECCFG_USEED)) {
> > +        return RISCV_EXCP_NONE;
> > +    } else {
> > +        return RISCV_EXCP_ILLEGAL_INST;
> > +    }
> > +#else
> > +    return RISCV_EXCP_NONE;
> > +#endif
> > +}
> > +
> >   /* User Floating-Point CSRs */
> >   static RISCVException read_fflags(CPURISCVState *env, int csrno,
> >                                     target_ulong *val)
> > @@ -2961,6 +2997,35 @@ static RISCVException write_upmbase(CPURISCVState 
> > *env, int csrno,
> >
> >   #endif
> >
> > +/* Crypto Extension */
> > +static RISCVException rmw_seed(CPURISCVState *env, int csrno,
> > +                              target_ulong *ret_value,
> > +                              target_ulong new_value, target_ulong 
> > write_mask)
> > +{
> > +    uint16_t random_v;
> > +    Error *random_e = NULL;
> > +    int random_r;
> > +
> > +    random_r = qemu_guest_getrandom(&random_v, 2, &random_e);
> > +    if (unlikely(random_r < 0)) {
> > +        /*
> > +         * Failed, for unknown reasons in the crypto subsystem.
> > +         * The best we can do is log the reason and return a
> > +         * failure indication to the guest.  There is no reason
> > +         * we know to expect the failure to be transitory, so
> > +         * indicate DEAD to avoid having the guest spin on WAIT.
> > +         */
> > +        qemu_log_mask(LOG_UNIMP, "%s: Crypto failure: %s",
> > +                      __func__, error_get_pretty(random_e));
> > +        error_free(random_e);
> > +        *ret_value = SEED_OPST_DEAD;
> > +    } else {
> > +        *ret_value = random_v | SEED_OPST_ES16;
> > +    }

Won't this seg fault if a guest does a CSR write?

> > +
> > +    return RISCV_EXCP_NONE;
> > +}
> > +
> >   /*
> >    * riscv_csrrw - read and/or update control and status register
> >    *
> > @@ -3205,6 +3270,9 @@ riscv_csr_operations csr_ops[CSR_TABLE_SIZE] = {
> >       [CSR_TIME]  = { "time",  ctr,   read_time  },
> >       [CSR_TIMEH] = { "timeh", ctr32, read_timeh },
> >
> > +    /* Crypto Extension */
> > +    [CSR_SEED] = { "seed", seed, NULL, NULL, rmw_seed },
> > +
> >   #if !defined(CONFIG_USER_ONLY)
> >       /* Machine Timers and Counters */
> >       [CSR_MCYCLE]    = { "mcycle",    any,   read_instret  },
> > diff --git a/target/riscv/op_helper.c b/target/riscv/op_helper.c
> > index e0d708091e..3d8443416d 100644
> > --- a/target/riscv/op_helper.c
> > +++ b/target/riscv/op_helper.c
> > @@ -39,6 +39,15 @@ void helper_raise_exception(CPURISCVState *env, uint32_t 
> > exception)
> >
> >   target_ulong helper_csrr(CPURISCVState *env, int csr)
> >   {
> > +    /*
> > +     * The seed CSR must be accessed with a read-write instruction. A
> > +     * read-only instruction such as CSRRS/CSRRC with rs1=x0 or CSRRSI/
> > +     * CSRRCI with uimm=0 will raise an illegal instruction exception.
> > +     */
> > +    if (csr == CSR_SEED) {
> > +        riscv_raise_exception(env, RISCV_EXCP_ILLEGAL_INST, GETPC());
> > +    }
> > +
> >       target_ulong val = 0;
> >       RISCVException ret = riscv_csrrw(env, csr, &val, 0, 0, false);
> >
> > diff --git a/target/riscv/pmp.h b/target/riscv/pmp.h
> > index fcb6b7c467..a8dd797476 100644
> > --- a/target/riscv/pmp.h
> > +++ b/target/riscv/pmp.h
> > @@ -39,9 +39,11 @@ typedef enum {
> >   } pmp_am_t;
> >
> >   typedef enum {
> > -    MSECCFG_MML  = 1 << 0,
> > -    MSECCFG_MMWP = 1 << 1,
> > -    MSECCFG_RLB  = 1 << 2
> > +    MSECCFG_MML   = 1 << 0,
> > +    MSECCFG_MMWP  = 1 << 1,
> > +    MSECCFG_RLB   = 1 << 2,

Why are these ones being changed?

Alistair

> > +    MSECCFG_USEED = 1 << 8,
> > +    MSECCFG_SSEED = 1 << 9
> >   } mseccfg_field_t;
> >
> >   typedef struct {
>
>