On Mon, Aug 10, 2020 at 5:24 PM Hou Weiying <weiying_...@outlook.com> wrote:
>
> The ePMP can be found in:
> https://docs.google.com/document/d/1Mh_aiHYxemL0umN3GTTw8vsbmzHZ_nxZXgjgOUzbvc8/edit#heading=h.9wsr1lnxtwe2
>
> Signed-off-by: Hongzheng-Li <ethan.lee....@gmail.com>
> Signed-off-by: Hou Weiying <weiying_...@outlook.com>
> Signed-off-by: Myriad-Dreamin <camiy...@gmail.com>
Thanks for the patch!
I haven't forgotten about this. I have been hoping that the CSR
addresses would be finalised so then this can be merged. Unfortunately
that still hasn't happened!
> ---
> target/riscv/pmp.c | 134 ++++++++++++++++++++++++++++++++++----
> target/riscv/pmp.h | 12 ++++
> target/riscv/trace-events | 4 ++
> 3 files changed, 138 insertions(+), 12 deletions(-)
>
> diff --git a/target/riscv/pmp.c b/target/riscv/pmp.c
> index 2a2b9f5363..b1fa703aff 100644
> --- a/target/riscv/pmp.c
> +++ b/target/riscv/pmp.c
> @@ -34,6 +34,26 @@ static void pmp_write_cfg(CPURISCVState *env, uint32_t
> addr_index,
> static uint8_t pmp_read_cfg(CPURISCVState *env, uint32_t addr_index);
> static void pmp_update_rule(CPURISCVState *env, uint32_t pmp_index);
>
> +static char mode_to_char(int mode)
> +{
> + char ret = 0;
> + switch (mode) {
> + case PRV_U:
> + ret = 'u';
> + break;
> + case PRV_S:
> + ret = 's';
> + break;
> + case PRV_H:
> + ret = 'h';
> + break;
> + case PRV_M:
> + ret = 'm';
> + break;
> + }
> + return ret;
> +}
> +
> /*
> * Accessor method to extract address matching type 'a field' from cfg reg
> */
> @@ -99,7 +119,28 @@ static inline uint8_t pmp_read_cfg(CPURISCVState *env,
> uint32_t pmp_index)
> static void pmp_write_cfg(CPURISCVState *env, uint32_t pmp_index, uint8_t
> val)
> {
> if (pmp_index < MAX_RISCV_PMPS) {
> - if (!pmp_is_locked(env, pmp_index)) {
> + /*
> + * mseccfg.RLB is set
> + */
These can just be single line comments instead of three.
> + if (MSECCFG_RLB_ISSET(env) ||
> + /*
> + * mseccfg.MML is set
> + */
> + (MSECCFG_MML_ISSET(env) &&
> + /*
> + * m model and not adding X bit
> + */
> + (((val & PMP_LOCK) != 0 && (val & PMP_EXEC) != PMP_EXEC) ||
> + /*
> + * shared region and not adding X bit
> + */
> + ((val & PMP_LOCK) != PMP_LOCK &&
> + (val & 0x7) != (PMP_WRITE | PMP_EXEC)))) ||
> + /*
> + * mseccfg.MML is not set
> + */
> + (!MSECCFG_MML_ISSET(env) && !pmp_is_locked(env, pmp_index))
> + ){
> env->pmp_state.pmp[pmp_index].cfg_reg = val;
> pmp_update_rule(env, pmp_index);
> } else {
> @@ -230,6 +271,18 @@ bool pmp_hart_has_privs(CPURISCVState *env, target_ulong
> addr,
>
> /* Short cut if no rules */
> if (0 == pmp_get_num_rules(env)) {
> + if (MSECCFG_MMWP_ISSET(env)) {
> + qemu_log_mask(LOG_GUEST_ERROR,
> + "pmp violation - %c mode access denied\n",
> + mode_to_char(mode));
> + return false;
> + }
> + if (MSECCFG_MML_ISSET(env) && (mode != PRV_M || (privs & PMP_EXEC)))
> {
> + qemu_log_mask(LOG_GUEST_ERROR,
> + "pmp violation - %c mode access denied\n",
> + mode_to_char(mode));
> + return false;
> + }
> return true;
> }
>
> @@ -265,16 +318,65 @@ bool pmp_hart_has_privs(CPURISCVState *env,
> target_ulong addr,
> const uint8_t a_field =
> pmp_get_a_field(env->pmp_state.pmp[i].cfg_reg);
>
> - /*
> - * If the PMP entry is not off and the address is in range, do the
> priv
> - * check
> - */
> if (((s + e) == 2) && (PMP_AMATCH_OFF != a_field)) {
> - allowed_privs = PMP_READ | PMP_WRITE | PMP_EXEC;
> - if ((mode != PRV_M) || pmp_is_locked(env, i)) {
> - allowed_privs &= env->pmp_state.pmp[i].cfg_reg;
> + /*
> + * If the PMP entry is not off and the address is in range,
> + * do the priv check
> + */
> + if (!MSECCFG_MML_ISSET(env)) {
> + /*
> + * If mseccfg.MML Bit is not set, do pmp priv check
> + */
> + allowed_privs = PMP_READ | PMP_WRITE | PMP_EXEC;
> + if ((mode != PRV_M) || pmp_is_locked(env, i)) {
> + allowed_privs &= env->pmp_state.pmp[i].cfg_reg;
> + }
> + } else {
> + /*
> + * If mseccfg.MML Bit set, do the enhanced pmp priv check
> + */
> + if (env->pmp_state.pmp[i].cfg_reg & PMP_LOCK) {
> + /*
> + * Shared Region
> + */
> + if ((env->pmp_state.pmp[i].cfg_reg &
> + (PMP_READ | PMP_WRITE)) == PMP_WRITE) {
> + allowed_privs = PMP_EXEC | ((mode == PRV_M &&
> + (env->pmp_state.pmp[i].cfg_reg & PMP_EXEC)) ?
> + PMP_READ : 0);
> + } else {
> + allowed_privs = env->pmp_state.pmp[i].cfg_reg &
> + (PMP_READ | PMP_WRITE | PMP_EXEC);
> +
> + if (mode != PRV_M && allowed_privs) {
> + qemu_log_mask(LOG_GUEST_ERROR,
> + "pmp violation - %c mode access denied\n",
> + mode_to_char(mode));
> + ret = 0;
> + break;
> + }
> + }
> + } else {
> + /*
> + * Shared Region
> + */
> + if ((env->pmp_state.pmp[i].cfg_reg &
> + (PMP_READ | PMP_WRITE)) == PMP_WRITE) {
> + allowed_privs = PMP_READ | ((mode == PRV_M ||
> + (env->pmp_state.pmp[i].cfg_reg & PMP_EXEC)) ?
> + PMP_WRITE : 0);
> + } else {
> + allowed_privs = env->pmp_state.pmp[i].cfg_reg &
> + (PMP_READ | PMP_WRITE | PMP_EXEC);
> + if (mode == PRV_M && allowed_privs) {
> + qemu_log_mask(LOG_GUEST_ERROR,
> + "pmp violation - m mode access
> denied\n");
> + ret = 0;
> + break;
> + }
> + }
> + }
> }
> -
> if ((privs & allowed_privs) == privs) {
> ret = 1;
> break;
> @@ -288,15 +390,23 @@ bool pmp_hart_has_privs(CPURISCVState *env,
> target_ulong addr,
> /* No rule matched */
> if (ret == -1) {
> if (mode == PRV_M) {
> - ret = 1; /* Privileged spec v1.10 states if no PMP entry matches
> an
> - * M-Mode access, the access succeeds */
> + ret = !MSECCFG_MMWP_ISSET(env); /* PMP Enhancements */
> + if (MSECCFG_MML_ISSET(env) && (privs & PMP_EXEC)) {
> + ret = 0;
> + }
> } else {
> ret = 0; /* Other modes are not allowed to succeed if they don't
> * match a rule, but there are rules. We've checked for
> * no rule earlier in this function. */
> }
> }
> -
> + if (ret) {
> + trace_pmp_hart_has_privs_pass_match(
> + env->mhartid, addr, size, privs, mode);
> + } else {
> + trace_pmp_hart_has_privs_violation(
> + env->mhartid, addr, size, privs, mode);
> + }
> return ret == 1 ? true : false;
> }
>
> diff --git a/target/riscv/pmp.h b/target/riscv/pmp.h
> index 8e19793132..7db2069204 100644
> --- a/target/riscv/pmp.h
> +++ b/target/riscv/pmp.h
> @@ -36,6 +36,12 @@ typedef enum {
> PMP_AMATCH_NAPOT /* Naturally aligned power-of-two region */
> } pmp_am_t;
>
> +typedef enum {
> + MSECCFG_MML = 1 << 0,
> + MSECCFG_MMWP = 1 << 1,
> + MSECCFG_RLB = 1 << 2
> +} mseccfg_field_t;
> +
> typedef struct {
> target_ulong addr_reg;
> uint8_t cfg_reg;
> @@ -58,7 +64,13 @@ target_ulong pmpcfg_csr_read(CPURISCVState *env, uint32_t
> reg_index);
> void pmpaddr_csr_write(CPURISCVState *env, uint32_t addr_index,
> target_ulong val);
> target_ulong pmpaddr_csr_read(CPURISCVState *env, uint32_t addr_index);
> +void mseccfg_csr_write(CPURISCVState *env, target_ulong val);
> +target_ulong mseccfg_csr_read(CPURISCVState *env);
> bool pmp_hart_has_privs(CPURISCVState *env, target_ulong addr,
> target_ulong size, pmp_priv_t priv, target_ulong mode);
>
> +#define MSECCFG_MML_ISSET(env) get_field(env->mseccfg, MSECCFG_MML)
> +#define MSECCFG_MMWP_ISSET(env) get_field(env->mseccfg, MSECCFG_MMWP)
> +#define MSECCFG_RLB_ISSET(env) get_field(env->mseccfg, MSECCFG_RLB)
This fails to compile as the `target_ulong mseccfg` variable has not
been added in this patch.
Alistair
> +
> #endif
> diff --git a/target/riscv/trace-events b/target/riscv/trace-events
> index 4b6c652ae9..4f877f90f7 100644
> --- a/target/riscv/trace-events
> +++ b/target/riscv/trace-events
> @@ -6,3 +6,7 @@ pmpcfg_csr_read(uint64_t mhartid, uint32_t reg_index,
> uint64_t val) "hart %" PRI
> pmpcfg_csr_write(uint64_t mhartid, uint32_t reg_index, uint64_t val) "hart
> %" PRIu64 ": write reg%" PRIu32", val: 0x%" PRIx64
> pmpaddr_csr_read(uint64_t mhartid, uint32_t addr_index, uint64_t val) "hart
> %" PRIu64 ": read addr%" PRIu32", val: 0x%" PRIx64
> pmpaddr_csr_write(uint64_t mhartid, uint32_t addr_index, uint64_t val) "hart
> %" PRIu64 ": write addr%" PRIu32", val: 0x%" PRIx64
> +mseccfg_csr_read(uint64_t mhartid, uint64_t val) "hart %" PRIu64 ": read
> mseccfg, val: 0x%" PRIx64
> +mseccfg_csr_write(uint64_t mhartid, uint64_t val) "hart %" PRIu64 ": write
> mseccfg, val: 0x%" PRIx64
> +pmp_hart_has_privs_pass_match(uint64_t mhartid, uint64_t addr, uint64_t
> size, uint64_t privs, uint64_t mode) "hart %"PRId64 "pass PMP 0 match
> addr:%"PRIu64" size:%"PRIu64 "privs: %"PRIu64 "mode: %"PRIu64
> +pmp_hart_has_privs_violation(uint64_t mhartid, uint64_t addr, uint64_t size,
> uint64_t privs, uint64_t mode) "hart %"PRId64 "pass PMP 0 match
> addr:%"PRIu64" size:%"PRIu64 "privs: %"PRIu64 "mode: %"PRIu64
> --
> 2.20.1
>
>
