Re: [PATCH v1 1/8] target/riscv: Fix the PMP is locked check when using TOR

[Bin Meng]

On Fri, Apr 2, 2021 at 8:49 PM Alistair Francis
<alistair.francis@wdc.com> wrote:
>
> The RISC-V spec says:
>     if PMP entry i is locked and pmpicfg.A is set to TOR, writes to
>     pmpaddri-1 are ignored.
>
> The current QEMU code ignores accesses to pmpaddri-1 and pmpcfgi-1 which
> is incorrect.
>
> Update the pmp_is_locked() function to not check the suppording fields

typo: supported?

> and instaed enforce the lock functionality in the pmpaddr write operation.

typo: instead

>
> Signed-off-by: Alistair Francis <alistair.francis@wdc.com>
> ---
>  target/riscv/pmp.c | 25 +++++++++++++++----------
>  1 file changed, 15 insertions(+), 10 deletions(-)
>
> diff --git a/target/riscv/pmp.c b/target/riscv/pmp.c
> index cff020122a..6141d0f8f9 100644
> --- a/target/riscv/pmp.c
> +++ b/target/riscv/pmp.c
> @@ -59,16 +59,6 @@ static inline int pmp_is_locked(CPURISCVState *env, 
> uint32_t pmp_index)
>          return 0;
>      }
>
> -    /* In TOR mode, need to check the lock bit of the next pmp
> -     * (if there is a next)
> -     */
> -    const uint8_t a_field =
> -        pmp_get_a_field(env->pmp_state.pmp[pmp_index + 1].cfg_reg);
> -    if ((env->pmp_state.pmp[pmp_index + 1u].cfg_reg & PMP_LOCK) &&
> -         (PMP_AMATCH_TOR == a_field)) {
> -        return 1;
> -    }
> -
>      return 0;
>  }
>
> @@ -380,7 +370,22 @@ void pmpaddr_csr_write(CPURISCVState *env, uint32_t 
> addr_index,
>      target_ulong val)
>  {
>      trace_pmpaddr_csr_write(env->mhartid, addr_index, val);
> +
>      if (addr_index < MAX_RISCV_PMPS) {
> +        /* In TOR mode, need to check the lock bit of the next pmp

nits: should use correct multi-line comment block format

> +         * (if there is a next).
> +         */
> +        if (addr_index + 1 < MAX_RISCV_PMPS) {
> +            uint8_t pmp_cfg = env->pmp_state.pmp[addr_index + 1].cfg_reg;
> +
> +            if (pmp_cfg & PMP_LOCK &&
> +                PMP_AMATCH_TOR == pmp_get_a_field(pmp_cfg)) {
> +                qemu_log_mask(LOG_GUEST_ERROR,
> +                              "ignoring pmpaddr write - pmpcfg + 1 
> locked\n");
> +                return;
> +            }
> +        }
> +
>          if (!pmp_is_locked(env, addr_index)) {
>              env->pmp_state.pmp[addr_index].addr_reg = val;
>              pmp_update_rule(env, addr_index);
> --

Otherwise,
Reviewed-by: Bin Meng <bmeng.cn@gmail.com>