| Message ID | 20220228144810.7284-13-liweiwei@iscas.ac.cn (mailing list archive) |
|---|---|
| State | New, archived |
| Headers | show |
| Series | support subsets of scalar crypto extension | expand |
On 2/28/22 04:48, Weiwei Li wrote: > +/* Crypto Extension */ > +static RISCVException rmw_seed(CPURISCVState *env, int csrno, > + target_ulong *ret_value, > + target_ulong new_value, target_ulong write_mask) > +{ > + if (!write_mask) { > + return RISCV_EXCP_ILLEGAL_INST; > + } This is incorrect. The error should only be with a write-mask of the actual x0 register, not another register which happens to contain 0. There is in fact no way to diagnose exactly what you want here, which IIRC has an existing fixme comment somewhere. > + uint32_t return_status = SEED_OPST_ES16; > + > + *ret_value = return_status; > + if (return_status == SEED_OPST_ES16) { > + uint16_t random_number; > + qemu_guest_getrandom_nofail(&random_number, sizeof(random_number)); > + *ret_value = (*ret_value) | random_number; > + } else if (return_status == SEED_OPST_BIST) { > + /* Do nothing */ > + } else if (return_status == SEED_OPST_WAIT) { > + /* Do nothing */ > + } else if (return_status == SEED_OPST_DEAD) { > + /* Do nothing */ > + } This is also incorrect. This should be uint32_t result; uint16_t random_v; Error *random_e = NULL; int random_r; random_r = 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); result = SEED_OPST_DEAD; } else { result = random_v | SEED_OPST_ES16; } C.f. target/arm/helper.c, rndr_readfn. r~
在 2022/3/1 上午4:11, Richard Henderson 写道: > On 2/28/22 04:48, Weiwei Li wrote: >> +/* Crypto Extension */ >> +static RISCVException rmw_seed(CPURISCVState *env, int csrno, >> + target_ulong *ret_value, >> + target_ulong new_value, target_ulong >> write_mask) >> +{ >> + if (!write_mask) { >> + return RISCV_EXCP_ILLEGAL_INST; >> + } > > This is incorrect. The error should only be with a write-mask of the > actual x0 register, not another register which happens to contain 0. > There is in fact no way to diagnose exactly what you want here, which > IIRC has an existing fixme comment somewhere. Yeah. write_mask is also used in riscv_csrrw_check to check whether the read-only csr is written. We cannot distinguish x0 and reg which contains 0 here without changing total progress of csr read/write. > >> + uint32_t return_status = SEED_OPST_ES16; >> + >> + *ret_value = return_status; >> + if (return_status == SEED_OPST_ES16) { >> + uint16_t random_number; >> + qemu_guest_getrandom_nofail(&random_number, >> sizeof(random_number)); >> + *ret_value = (*ret_value) | random_number; >> + } else if (return_status == SEED_OPST_BIST) { >> + /* Do nothing */ >> + } else if (return_status == SEED_OPST_WAIT) { >> + /* Do nothing */ >> + } else if (return_status == SEED_OPST_DEAD) { >> + /* Do nothing */ >> + } > > This is also incorrect. This should be > > uint32_t result; > uint16_t random_v; > Error *random_e = NULL; > int random_r; > > random_r = 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); > result = SEED_OPST_DEAD; > } else { > result = random_v | SEED_OPST_ES16; > } > > C.f. target/arm/helper.c, rndr_readfn. OK. I'll fix this. Regards, Weiwei Li > > > r~
在 2022/3/1 上午9:44, Weiwei Li 写道: > > 在 2022/3/1 上午4:11, Richard Henderson 写道: >> On 2/28/22 04:48, Weiwei Li wrote: >>> +/* Crypto Extension */ >>> +static RISCVException rmw_seed(CPURISCVState *env, int csrno, >>> + target_ulong *ret_value, >>> + target_ulong new_value, target_ulong >>> write_mask) >>> +{ >>> + if (!write_mask) { >>> + return RISCV_EXCP_ILLEGAL_INST; >>> + } >> >> This is incorrect. The error should only be with a write-mask of the >> actual x0 register, not another register which happens to contain 0. >> There is in fact no way to diagnose exactly what you want here, which >> IIRC has an existing fixme comment somewhere. > Yeah. write_mask is also used in riscv_csrrw_check to check whether > the read-only csr is written. We cannot distinguish x0 and reg which > contains 0 here without changing total progress of csr read/write. >> I seems misunderstand the code for csr read/write: write_mask will be set zero only for read-only operation (CSRRS/CSRRC with rs1=x0 or CSRRSI/CSRRCI with uimm=0) via do_csrr --> helper_csrr -> riscv_csrrw call-chain. The write_mask for do_csrw and do_csrrw will not be zero. As said in the spec : "TheseedCSR must be accessed with a read-write instruction. A read-only instruction such asCSRRS/CSRRC withrs1=x0orCSRRSI/CSRRCIwithuimm=0will raise an illegal instruction exception. " So it's suitable to check write_mask here. >>> + uint32_t return_status = SEED_OPST_ES16; >>> + >>> + *ret_value = return_status; >>> + if (return_status == SEED_OPST_ES16) { >>> + uint16_t random_number; >>> + qemu_guest_getrandom_nofail(&random_number, >>> sizeof(random_number)); >>> + *ret_value = (*ret_value) | random_number; >>> + } else if (return_status == SEED_OPST_BIST) { >>> + /* Do nothing */ >>> + } else if (return_status == SEED_OPST_WAIT) { >>> + /* Do nothing */ >>> + } else if (return_status == SEED_OPST_DEAD) { >>> + /* Do nothing */ >>> + } >> >> This is also incorrect. This should be >> >> uint32_t result; >> uint16_t random_v; >> Error *random_e = NULL; >> int random_r; >> >> random_r = 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); >> result = SEED_OPST_DEAD; >> } else { >> result = random_v | SEED_OPST_ES16; >> } >> >> C.f. target/arm/helper.c, rndr_readfn. > > OK. I'll fix this. > > Regards, > > Weiwei Li > >> >> >> r~
On 2/28/22 16:27, Weiwei Li wrote: > > 在 2022/3/1 上午9:44, Weiwei Li 写道: >> >> 在 2022/3/1 上午4:11, Richard Henderson 写道: >>> On 2/28/22 04:48, Weiwei Li wrote: >>>> +/* Crypto Extension */ >>>> +static RISCVException rmw_seed(CPURISCVState *env, int csrno, >>>> + target_ulong *ret_value, >>>> + target_ulong new_value, target_ulong write_mask) >>>> +{ >>>> + if (!write_mask) { >>>> + return RISCV_EXCP_ILLEGAL_INST; >>>> + } >>> >>> This is incorrect. The error should only be with a write-mask of the actual x0 >>> register, not another register which happens to contain 0. There is in fact no way to >>> diagnose exactly what you want here, which IIRC has an existing fixme comment somewhere. >> Yeah. write_mask is also used in riscv_csrrw_check to check whether the read-only csr is >> written. We cannot distinguish x0 and reg which contains 0 here without changing total >> progress of csr read/write. >>> > I seems misunderstand the code for csr read/write: write_mask will be set zero only for > read-only operation (CSRRS/CSRRC with rs1=x0 or CSRRSI/CSRRCI with uimm=0) via do_csrr --> > helper_csrr -> riscv_csrrw call-chain. > > The write_mask for do_csrw and do_csrrw will not be zero. > > As said in the spec : > > "TheseedCSR must be accessed with a read-write instruction. A read-only instruction such > asCSRRS/CSRRC > withrs1=x0orCSRRSI/CSRRCIwithuimm=0will raise an illegal instruction exception. " > > So it's suitable to check write_mask here. Consider CSRRS with rs1=x31. In that case mask will be the value in x31. Even if the value is 0, this is still considered a read-write instruction. r~
在 2022/3/1 下午11:59, Richard Henderson 写道: > On 2/28/22 16:27, Weiwei Li wrote: >> >> 在 2022/3/1 上午9:44, Weiwei Li 写道: >>> >>> 在 2022/3/1 上午4:11, Richard Henderson 写道: >>>> On 2/28/22 04:48, Weiwei Li wrote: >>>>> +/* Crypto Extension */ >>>>> +static RISCVException rmw_seed(CPURISCVState *env, int csrno, >>>>> + target_ulong *ret_value, >>>>> + target_ulong new_value, >>>>> target_ulong write_mask) >>>>> +{ >>>>> + if (!write_mask) { >>>>> + return RISCV_EXCP_ILLEGAL_INST; >>>>> + } >>>> >>>> This is incorrect. The error should only be with a write-mask of >>>> the actual x0 register, not another register which happens to >>>> contain 0. There is in fact no way to diagnose exactly what you >>>> want here, which IIRC has an existing fixme comment somewhere. >>> Yeah. write_mask is also used in riscv_csrrw_check to check whether >>> the read-only csr is written. We cannot distinguish x0 and reg which >>> contains 0 here without changing total progress of csr read/write. >>>> >> I seems misunderstand the code for csr read/write: write_mask will >> be set zero only for read-only operation (CSRRS/CSRRC with rs1=x0 or >> CSRRSI/CSRRCI with uimm=0) via do_csrr --> helper_csrr -> riscv_csrrw >> call-chain. >> >> The write_mask for do_csrw and do_csrrw will not be zero. >> >> As said in the spec : >> >> "TheseedCSR must be accessed with a read-write instruction. A >> read-only instruction such asCSRRS/CSRRC >> withrs1=x0orCSRRSI/CSRRCIwithuimm=0will raise an illegal instruction >> exception. " >> >> So it's suitable to check write_mask here. > > Consider CSRRS with rs1=x31. In that case mask will be the value in > x31. Even if the value is 0, this is still considered a read-write > instruction. Yeah. I lost this kind of case . So this is a bug. Maybe we can add a new parameter like 'write' to helper_csrrw to indicate that write operation should be done. > > > r~
diff --git a/target/riscv/cpu_bits.h b/target/riscv/cpu_bits.h index 0fe01d7da5..d0a4a16c73 100644 --- a/target/riscv/cpu_bits.h +++ b/target/riscv/cpu_bits.h @@ -446,6 +446,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 @@ -760,4 +763,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 aea82dff4a..784fb6894d 100644 --- a/target/riscv/csr.c +++ b/target/riscv/csr.c @@ -23,6 +23,8 @@ #include "cpu.h" #include "qemu/main-loop.h" #include "exec/exec-all.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) @@ -291,6 +293,39 @@ static RISCVException epmp(CPURISCVState *env, int csrno) } #endif +/* Predicates */ +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)) { + 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) @@ -2861,6 +2896,32 @@ 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) +{ + if (!write_mask) { + return RISCV_EXCP_ILLEGAL_INST; + } + + uint32_t return_status = SEED_OPST_ES16; + + *ret_value = return_status; + if (return_status == SEED_OPST_ES16) { + uint16_t random_number; + qemu_guest_getrandom_nofail(&random_number, sizeof(random_number)); + *ret_value = (*ret_value) | random_number; + } else if (return_status == SEED_OPST_BIST) { + /* Do nothing */ + } else if (return_status == SEED_OPST_WAIT) { + /* Do nothing */ + } else if (return_status == SEED_OPST_DEAD) { + /* Do nothing */ + } + return RISCV_EXCP_NONE; +} + /* * riscv_csrrw - read and/or update control and status register * @@ -3087,6 +3148,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/pmp.h b/target/riscv/pmp.h index a9a0b363a7..83135849bb 100644 --- a/target/riscv/pmp.h +++ b/target/riscv/pmp.h @@ -37,9 +37,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, + MSECCFG_USEED = 1 << 8, + MSECCFG_SSEED = 1 << 9 } mseccfg_field_t; typedef struct {