Thanks for your comments.
在 2022/4/14 上午7:57, Alistair Francis 写道:
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.
Yeah, Maybe It can merge into following logic, like:
if (env->priv == PRV_M) { //M
return RISCV_EXCP_NONE;
} else if (riscv_cpu_virt_enabled(env)) { //VS/VU
if (env->mseccfg & MSECCFG_SSEED) {
return RISCV_EXCP_NONE;
} else {
return RISCV_EXCP_VIRT_INSTRUCTION_FAULT;
}
} else { //S/U
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;
}
}
+ 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?
Yeah. Only CSR write seems have no function. However, it's not limited
in the spec, so it's possible.
I'll add NULL judgement here.
+
+ 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?
They are changed to align with MSECCFG_USEED. Is it OK or just let them
unchanged?
Regards,
Weiwei Li
Alistair
+ MSECCFG_USEED = 1 << 8,
+ MSECCFG_SSEED = 1 << 9
} mseccfg_field_t;
typedef struct {
