Re: [PATCH v3 2/2] target/riscv: Optimize ambiguous local variable in pmp_hart_has_privs

[Michael Tokarev]

08.07.2023 12:10, Ruibo Lu пишет:
> These two values represents whether start/end address is in pmp_range.
> However, the type and name of them is ambiguous. This commit change the
> name and type of them to improve code readability and accuracy.
>
> Resolves: https://gitlab.com/qemu-project/qemu/-/issues/1735
> Reviewed-by: Weiwei Li <liweiwei@iscas.ac.cn>
> Reviewed-by: Philippe Mathieu-Daudé <philmd@linaro.org>
> Signed-off-by: Ruibo Lu <reaperlu@hust.edu.cn>
> ---
>   target/riscv/pmp.c | 22 +++++++++++-----------
>   1 file changed, 11 insertions(+), 11 deletions(-)
>
> diff --git a/target/riscv/pmp.c b/target/riscv/pmp.c
> index 1a9279ba88..ea3d29217a 100644
> --- a/target/riscv/pmp.c
> +++ b/target/riscv/pmp.c
> @@ -203,16 +203,16 @@ void pmp_update_rule_nums(CPURISCVState *env)
>       }
>   }
>   
> -static int pmp_is_in_range(CPURISCVState *env, int pmp_index,
> -                           target_ulong addr)
> +static bool pmp_is_in_range(CPURISCVState *env, int pmp_index,
> +                            target_ulong addr)
>   {
> -    int result = 0;
> +    bool result = false;
>   
>       if ((addr >= env->pmp_state.addr[pmp_index].sa) &&
>           (addr <= env->pmp_state.addr[pmp_index].ea)) {
> -        result = 1;
> +        result = true;
>       } else {
> -        result = 0;
> +        result = false;
>       }
>   
>       return result;

And the initial assignment of result isn't needed.  How about this:

 return (addr >= env->pmp_state.addr[pmp_index].sa) &&
        (addr <= env->pmp_state.addr[pmp_index].ea);

instead? :)

> @@ -287,8 +287,8 @@ bool pmp_hart_has_privs(CPURISCVState *env, target_ulong 
> addr,
>   {
>       int i = 0;
>       int pmp_size = 0;
> -    target_ulong s = 0;
> -    target_ulong e = 0;
> +    bool sa_in = false;
> +    bool ea_in = false;
>   
>       /* Short cut if no rules */
>       if (0 == pmp_get_num_rules(env)) {
> @@ -314,11 +314,11 @@ bool pmp_hart_has_privs(CPURISCVState *env, target_ulong 
> addr,
>        * from low to high
>        */
>       for (i = 0; i < MAX_RISCV_PMPS; i++) {
> -        s = pmp_is_in_range(env, i, addr);
> -        e = pmp_is_in_range(env, i, addr + pmp_size - 1);
> +        sa_in = pmp_is_in_range(env, i, addr);
> +        ea_in = pmp_is_in_range(env, i, addr + pmp_size - 1);
>   
>           /* partially inside */
> -        if ((s + e) == 1) {
> +        if (sa_in ^ ea_in) {

Dunno how for others, but to me this is a bit difficult to
read, as an exclusive or isn't a commonly used operation.
Maybe sa_in != ea_in ?

Thanks,

/mjt

>               qemu_log_mask(LOG_GUEST_ERROR,
>                             "pmp violation - access is partially inside\n");
>               *allowed_privs = 0;
> @@ -339,7 +339,7 @@ bool pmp_hart_has_privs(CPURISCVState *env, target_ulong 
> addr,
>               (env->pmp_state.pmp[i].cfg_reg & PMP_WRITE) |
>               ((env->pmp_state.pmp[i].cfg_reg & PMP_EXEC) >> 2);
>   
> -        if (((s + e) == 2) && (PMP_AMATCH_OFF != a_field)) {
> +        if (sa_in && ea_in && (PMP_AMATCH_OFF != a_field)) {
>               /*
>                * If the PMP entry is not off and the address is in range,
>                * do the priv check