Re: [PATCH V3 3/6] target/riscv: Add PMP state description

[Alistair Francis]

On Fri, Oct 23, 2020 at 2:16 AM Yifei Jiang <jiangyifei@huawei.com> wrote:
>
> In the case of supporting PMP feature, add PMP state description
> to vmstate_riscv_cpu.
>
> 'vmstate_pmp_addr' and 'num_rules' could be regenerated by
> pmp_update_rule(). But there exists the problem of updating
> num_rules repeatedly in pmp_update_rule(). So here extracts
> pmp_update_rule_addr() and pmp_update_rule_nums() to update
> 'vmstate_pmp_addr' and 'num_rules' respectively.
>
> Signed-off-by: Yifei Jiang <jiangyifei@huawei.com>
> Signed-off-by: Yipeng Yin <yinyipeng1@huawei.com>

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

Alistair

> ---
>  target/riscv/machine.c | 50 ++++++++++++++++++++++++++++++++++++++++++
>  target/riscv/pmp.c     | 29 ++++++++++++++----------
>  target/riscv/pmp.h     |  2 ++
>  3 files changed, 70 insertions(+), 11 deletions(-)
>
> diff --git a/target/riscv/machine.c b/target/riscv/machine.c
> index 32edbcba7c..fc1461d88e 100644
> --- a/target/riscv/machine.c
> +++ b/target/riscv/machine.c
> @@ -22,6 +22,52 @@
>  #include "sysemu/kvm.h"
>  #include "migration/cpu.h"
>
> +static bool pmp_needed(void *opaque)
> +{
> +    RISCVCPU *cpu = opaque;
> +    CPURISCVState *env = &cpu->env;
> +
> +    return riscv_feature(env, RISCV_FEATURE_PMP);
> +}
> +
> +static int pmp_post_load(void *opaque, int version_id)
> +{
> +    RISCVCPU *cpu = opaque;
> +    CPURISCVState *env = &cpu->env;
> +    int i;
> +
> +    for (i = 0; i < MAX_RISCV_PMPS; i++) {
> +        pmp_update_rule_addr(env, i);
> +    }
> +    pmp_update_rule_nums(env);
> +
> +    return 0;
> +}
> +
> +static const VMStateDescription vmstate_pmp_entry = {
> +    .name = "cpu/pmp/entry",
> +    .version_id = 1,
> +    .minimum_version_id = 1,
> +    .fields = (VMStateField[]) {
> +        VMSTATE_UINTTL(addr_reg, pmp_entry_t),
> +        VMSTATE_UINT8(cfg_reg, pmp_entry_t),
> +        VMSTATE_END_OF_LIST()
> +    }
> +};
> +
> +static const VMStateDescription vmstate_pmp = {
> +    .name = "cpu/pmp",
> +    .version_id = 1,
> +    .minimum_version_id = 1,
> +    .needed = pmp_needed,
> +    .post_load = pmp_post_load,
> +    .fields = (VMStateField[]) {
> +        VMSTATE_STRUCT_ARRAY(env.pmp_state.pmp, RISCVCPU, MAX_RISCV_PMPS,
> +                             0, vmstate_pmp_entry, pmp_entry_t),
> +        VMSTATE_END_OF_LIST()
> +    }
> +};
> +
>  const VMStateDescription vmstate_riscv_cpu = {
>      .name = "cpu",
>      .version_id = 1,
> @@ -70,5 +116,9 @@ const VMStateDescription vmstate_riscv_cpu = {
>          VMSTATE_UINT64(env.timecmp, RISCVCPU),
>
>          VMSTATE_END_OF_LIST()
> +    },
> +    .subsections = (const VMStateDescription * []) {
> +        &vmstate_pmp,
> +        NULL
>      }
>  };
> diff --git a/target/riscv/pmp.c b/target/riscv/pmp.c
> index c394e867f8..2eda8e1e2f 100644
> --- a/target/riscv/pmp.c
> +++ b/target/riscv/pmp.c
> @@ -136,18 +136,8 @@ static void pmp_decode_napot(target_ulong a, 
> target_ulong *sa, target_ulong *ea)
>      }
>  }
>
> -
> -/* Convert cfg/addr reg values here into simple 'sa' --> start address and 
> 'ea'
> - *   end address values.
> - *   This function is called relatively infrequently whereas the check that
> - *   an address is within a pmp rule is called often, so optimise that one
> - */
> -static void pmp_update_rule(CPURISCVState *env, uint32_t pmp_index)
> +void pmp_update_rule_addr(CPURISCVState *env, uint32_t pmp_index)
>  {
> -    int i;
> -
> -    env->pmp_state.num_rules = 0;
> -
>      uint8_t this_cfg = env->pmp_state.pmp[pmp_index].cfg_reg;
>      target_ulong this_addr = env->pmp_state.pmp[pmp_index].addr_reg;
>      target_ulong prev_addr = 0u;
> @@ -186,7 +176,13 @@ static void pmp_update_rule(CPURISCVState *env, uint32_t 
> pmp_index)
>
>      env->pmp_state.addr[pmp_index].sa = sa;
>      env->pmp_state.addr[pmp_index].ea = ea;
> +}
>
> +void pmp_update_rule_nums(CPURISCVState *env)
> +{
> +    int i;
> +
> +    env->pmp_state.num_rules = 0;
>      for (i = 0; i < MAX_RISCV_PMPS; i++) {
>          const uint8_t a_field =
>              pmp_get_a_field(env->pmp_state.pmp[i].cfg_reg);
> @@ -196,6 +192,17 @@ static void pmp_update_rule(CPURISCVState *env, uint32_t 
> pmp_index)
>      }
>  }
>
> +/* Convert cfg/addr reg values here into simple 'sa' --> start address and 
> 'ea'
> + *   end address values.
> + *   This function is called relatively infrequently whereas the check that
> + *   an address is within a pmp rule is called often, so optimise that one
> + */
> +static void pmp_update_rule(CPURISCVState *env, uint32_t pmp_index)
> +{
> +    pmp_update_rule_addr(env, pmp_index);
> +    pmp_update_rule_nums(env);
> +}
> +
>  static int pmp_is_in_range(CPURISCVState *env, int pmp_index, target_ulong 
> addr)
>  {
>      int result = 0;
> diff --git a/target/riscv/pmp.h b/target/riscv/pmp.h
> index 6a8f072871..6c6b4c9bef 100644
> --- a/target/riscv/pmp.h
> +++ b/target/riscv/pmp.h
> @@ -62,5 +62,7 @@ bool pmp_hart_has_privs(CPURISCVState *env, target_ulong 
> addr,
>      target_ulong size, pmp_priv_t priv, target_ulong mode);
>  bool pmp_is_range_in_tlb(CPURISCVState *env, hwaddr tlb_sa,
>                           target_ulong *tlb_size);
> +void pmp_update_rule_addr(CPURISCVState *env, uint32_t pmp_index);
> +void pmp_update_rule_nums(CPURISCVState *env);
>
>  #endif
> --
> 2.19.1
>
>