Re: [Qemu-devel] [PATCHv4 3/6] RISC-V: Check for the effective memory privilege mode during PMP checks

[Alistair Francis <alistair23@gmail.com>]

On Thu, May 30, 2019 at 6:52 AM Hesham Almatary
<Hesham.Almatary@cl.cam.ac.uk> wrote:
>
> The current PMP check function checks for env->priv which is not the effective
> memory privilege mode.
>
> For example, mstatus.MPRV could be set while executing in M-Mode, and in that
> case the privilege mode for the PMP check should be S-Mode rather than M-Mode
> (in env->priv) if mstatus.MPP == PRV_S.
>
> This patch passes the effective memory privilege mode to the PMP check.
> Functions that call the PMP check should pass the correct memory privilege mode
> after reading mstatus' MPRV/MPP or hstatus.SPRV (if Hypervisor mode exists).
>
> Suggested-by: Alistair Francis <alistair.francis@wdc.com>
> Signed-off-by: Hesham Almatary <Hesham.Almatary@cl.cam.ac.uk>

Reviewed-by: Alistair Francis <alistair.francis@wdc.com>

Alistair

> ---
>  target/riscv/cpu_helper.c | 10 +++++++++-
>  target/riscv/pmp.c        |  6 +++---
>  target/riscv/pmp.h        |  2 +-
>  3 files changed, 13 insertions(+), 5 deletions(-)
>
> diff --git a/target/riscv/cpu_helper.c b/target/riscv/cpu_helper.c
> index 7c7282c680..5a1cd7cf96 100644
> --- a/target/riscv/cpu_helper.c
> +++ b/target/riscv/cpu_helper.c
> @@ -392,19 +392,27 @@ bool riscv_cpu_tlb_fill(CPUState *cs, vaddr address, int size,
>      int prot;
>      bool pmp_violation = false;
>      int ret = TRANSLATE_FAIL;
> +    int mode = mmu_idx;
>
>      qemu_log_mask(CPU_LOG_MMU, "%s ad %" VADDR_PRIx " rw %d mmu_idx %d\n",
>                    __func__, address, access_type, mmu_idx);
>
>      ret = get_physical_address(env, &pa, &prot, address, access_type, mmu_idx);
>
> +    if (mode == PRV_M && access_type != MMU_INST_FETCH) {
> +        if (get_field(env->mstatus, MSTATUS_MPRV)) {
> +            mode = get_field(env->mstatus, MSTATUS_MPP);
> +        }
> +    }
> +
>      qemu_log_mask(CPU_LOG_MMU,
>                    "%s address=%" VADDR_PRIx " ret %d physical " TARGET_FMT_plx
>                    " prot %d\n", __func__, address, ret, pa, prot);
>
>      if (riscv_feature(env, RISCV_FEATURE_PMP) &&
>          (ret == TRANSLATE_SUCCESS) &&
> -        !pmp_hart_has_privs(env, pa, TARGET_PAGE_SIZE, 1 << access_type)) {
> +        !pmp_hart_has_privs(env, pa, TARGET_PAGE_SIZE, 1 << access_type,
> +        mode)) {
>          pmp_violation = true;
>          ret = TRANSLATE_FAIL;
>      }
> diff --git a/target/riscv/pmp.c b/target/riscv/pmp.c
> index b11c4ae22f..89170bc11d 100644
> --- a/target/riscv/pmp.c
> +++ b/target/riscv/pmp.c
> @@ -229,7 +229,7 @@ static int pmp_is_in_range(CPURISCVState *env, int pmp_index, target_ulong addr)
>   * Check if the address has required RWX privs to complete desired operation
>   */
>  bool pmp_hart_has_privs(CPURISCVState *env, target_ulong addr,
> -    target_ulong size, pmp_priv_t privs)
> +    target_ulong size, pmp_priv_t privs, target_ulong mode)
>  {
>      int i = 0;
>      int ret = -1;
> @@ -265,7 +265,7 @@ bool pmp_hart_has_privs(CPURISCVState *env, target_ulong addr,
>              }
>
>              allowed_privs = PMP_READ | PMP_WRITE | PMP_EXEC;
> -            if ((env->priv != PRV_M) || pmp_is_locked(env, i)) {
> +            if ((mode != PRV_M) || pmp_is_locked(env, i)) {
>                  allowed_privs &= env->pmp_state.pmp[i].cfg_reg;
>              }
>
> @@ -281,7 +281,7 @@ bool pmp_hart_has_privs(CPURISCVState *env, target_ulong addr,
>
>      /* No rule matched */
>      if (ret == -1) {
> -        if (env->priv == PRV_M) {
> +        if (mode == PRV_M) {
>              ret = 1; /* Privileged spec v1.10 states if no PMP entry matches an
>                        * M-Mode access, the access succeeds */
>          } else {
> diff --git a/target/riscv/pmp.h b/target/riscv/pmp.h
> index 66790950eb..8e19793132 100644
> --- a/target/riscv/pmp.h
> +++ b/target/riscv/pmp.h
> @@ -59,6 +59,6 @@ void pmpaddr_csr_write(CPURISCVState *env, uint32_t addr_index,
>      target_ulong val);
>  target_ulong pmpaddr_csr_read(CPURISCVState *env, uint32_t addr_index);
>  bool pmp_hart_has_privs(CPURISCVState *env, target_ulong addr,
> -    target_ulong size, pmp_priv_t priv);
> +    target_ulong size, pmp_priv_t priv, target_ulong mode);
>
>  #endif
> --
> 2.17.1
>
>