On Mon, Apr 11, 2022 at 2:46 PM Weiwei Li <liwei...@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 <luruibo2...@163.com> > > Co-authored-by: Zewen Ye <lust...@foxmail.com> > > Signed-off-by: Weiwei Li <liwei...@iscas.ac.cn> > > Signed-off-by: Junqiang Wang <wangjunqi...@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 { > >
