Re: [PATCH 4/6] target/riscv: Flush TLB only when pmpcfg/pmpaddr really changes

[Alistair Francis]

On Thu, Apr 13, 2023 at 7:04 PM Weiwei Li <liweiwei@iscas.ac.cn> wrote:
>
> TLB needn't be flushed when pmpcfg/pmpaddr don't changes.
>
> Signed-off-by: Weiwei Li <liweiwei@iscas.ac.cn>
> Signed-off-by: Junqiang Wang <wangjunqiang@iscas.ac.cn>

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

Alistair

> ---
>  target/riscv/pmp.c | 24 ++++++++++++++++--------
>  1 file changed, 16 insertions(+), 8 deletions(-)
>
> diff --git a/target/riscv/pmp.c b/target/riscv/pmp.c
> index 6d4813806b..aced23c4d5 100644
> --- a/target/riscv/pmp.c
> +++ b/target/riscv/pmp.c
> @@ -26,7 +26,7 @@
>  #include "trace.h"
>  #include "exec/exec-all.h"
>
> -static void pmp_write_cfg(CPURISCVState *env, uint32_t addr_index,
> +static bool pmp_write_cfg(CPURISCVState *env, uint32_t addr_index,
>                            uint8_t val);
>  static uint8_t pmp_read_cfg(CPURISCVState *env, uint32_t addr_index);
>  static void pmp_update_rule(CPURISCVState *env, uint32_t pmp_index);
> @@ -83,7 +83,7 @@ static inline uint8_t pmp_read_cfg(CPURISCVState *env, 
> uint32_t pmp_index)
>   * Accessor to set the cfg reg for a specific PMP/HART
>   * Bounds checks and relevant lock bit.
>   */
> -static void pmp_write_cfg(CPURISCVState *env, uint32_t pmp_index, uint8_t 
> val)
> +static bool pmp_write_cfg(CPURISCVState *env, uint32_t pmp_index, uint8_t 
> val)
>  {
>      if (pmp_index < MAX_RISCV_PMPS) {
>          bool locked = true;
> @@ -119,14 +119,17 @@ static void pmp_write_cfg(CPURISCVState *env, uint32_t 
> pmp_index, uint8_t val)
>
>          if (locked) {
>              qemu_log_mask(LOG_GUEST_ERROR, "ignoring pmpcfg write - 
> locked\n");
> -        } else {
> +        } else if (env->pmp_state.pmp[pmp_index].cfg_reg != val) {
>              env->pmp_state.pmp[pmp_index].cfg_reg = val;
>              pmp_update_rule(env, pmp_index);
> +            return true;
>          }
>      } else {
>          qemu_log_mask(LOG_GUEST_ERROR,
>                        "ignoring pmpcfg write - out of bounds\n");
>      }
> +
> +    return false;
>  }
>
>  static void pmp_decode_napot(target_ulong a, target_ulong *sa,
> @@ -477,16 +480,19 @@ void pmpcfg_csr_write(CPURISCVState *env, uint32_t 
> reg_index,
>      int i;
>      uint8_t cfg_val;
>      int pmpcfg_nums = 2 << riscv_cpu_mxl(env);
> +    bool modified = false;
>
>      trace_pmpcfg_csr_write(env->mhartid, reg_index, val);
>
>      for (i = 0; i < pmpcfg_nums; i++) {
>          cfg_val = (val >> 8 * i)  & 0xff;
> -        pmp_write_cfg(env, (reg_index * 4) + i, cfg_val);
> +        modified |= pmp_write_cfg(env, (reg_index * 4) + i, cfg_val);
>      }
>
>      /* If PMP permission of any addr has been changed, flush TLB pages. */
> -    tlb_flush(env_cpu(env));
> +    if (modified) {
> +        tlb_flush(env_cpu(env));
> +    }
>  }
>
>
> @@ -535,9 +541,11 @@ void pmpaddr_csr_write(CPURISCVState *env, uint32_t 
> addr_index,
>          }
>
>          if (!pmp_is_locked(env, addr_index)) {
> -            env->pmp_state.pmp[addr_index].addr_reg = val;
> -            pmp_update_rule(env, addr_index);
> -            tlb_flush(env_cpu(env));
> +            if (env->pmp_state.pmp[addr_index].addr_reg != val) {
> +                env->pmp_state.pmp[addr_index].addr_reg = val;
> +                pmp_update_rule(env, addr_index);
> +                tlb_flush(env_cpu(env));
> +            }
>          } else {
>              qemu_log_mask(LOG_GUEST_ERROR,
>                            "ignoring pmpaddr write - locked\n");
> --
> 2.25.1
>
>