[PATCH] target/riscv: Smepmp: Return error when access permission not allowed in PMP

[PATCH] target/riscv: Smepmp: Return error when access permission not allowed in PMP

[Himanshu Chauhan]

On an address match, skip checking for default permissions and return error
based on access defined in PMP configuration.

Fixes: 90b1fafce06 ("target/riscv: Smepmp: Skip applying default rules when address matches")
Signed-off-by: Himanshu Chauhan <hchauhan@ventanamicro.com>
---
 target/riscv/pmp.c | 5 +++--
 1 file changed, 3 insertions(+), 2 deletions(-)

diff --git a/target/riscv/pmp.c b/target/riscv/pmp.c
index 418738afd8..6238528282 100644
--- a/target/riscv/pmp.c
+++ b/target/riscv/pmp.c
@@ -435,8 +435,8 @@ bool pmp_hart_has_privs(CPURISCVState *env, target_ulong addr,
              * defined with PMP must be used. We shouldn't fallback on
              * finding default privileges.
              */
-            ret = true;
-            break;
+            ret = ((privs & *allowed_privs) == privs ? true : false);
+            goto _address_matched;
         }
     }
 
@@ -445,6 +445,7 @@ bool pmp_hart_has_privs(CPURISCVState *env, target_ulong addr,
         ret = pmp_hart_has_privs_default(env, privs, allowed_privs, mode);
     }
 
+ _address_matched:
     return ret;
 }
 
-- 
2.34.1

Re: [PATCH] target/riscv: Smepmp: Return error when access permission not allowed in PMP

[Weiwei Li]

On 2023/6/5 15:51, Himanshu Chauhan wrote:
> On an address match, skip checking for default permissions and return error
> based on access defined in PMP configuration.
>
> Fixes: 90b1fafce06 ("target/riscv: Smepmp: Skip applying default rules when address matches")
> Signed-off-by: Himanshu Chauhan <hchauhan@ventanamicro.com>
> ---
>   target/riscv/pmp.c | 5 +++--
>   1 file changed, 3 insertions(+), 2 deletions(-)
>
> diff --git a/target/riscv/pmp.c b/target/riscv/pmp.c
> index 418738afd8..6238528282 100644
> --- a/target/riscv/pmp.c
> +++ b/target/riscv/pmp.c
> @@ -435,8 +435,8 @@ bool pmp_hart_has_privs(CPURISCVState *env, target_ulong addr,
>                * defined with PMP must be used. We shouldn't fallback on
>                * finding default privileges.
>                */
> -            ret = true;
> -            break;
> +            ret = ((privs & *allowed_privs) == privs ? true : false);
> +            goto _address_matched;

goto seems unnecessary. We can directly return (privs & *allowed_privs) 
== privs here.

And then we can directly return pmp_hart_has_privs_default(env, privs, 
allowed_privs, mode) after this loop.

Regards,

Weiwei Li

>           }
>       }
>   
> @@ -445,6 +445,7 @@ bool pmp_hart_has_privs(CPURISCVState *env, target_ulong addr,
>           ret = pmp_hart_has_privs_default(env, privs, allowed_privs, mode);

>       }
>   
> + _address_matched:
>       return ret;
>   }
>   

[PATCH v2] target/riscv: Smepmp: Return error when access permission not allowed in PMP

[Himanshu Chauhan]

On an address match, skip checking for default permissions and return error
based on access defined in PMP configuration.

v2 Changes:
o Removed goto to return in place when address matches
o Call pmp_hart_has_privs_default at the end of the loop

Fixes: 90b1fafce06 ("target/riscv: Smepmp: Skip applying default rules when address matches")
Signed-off-by: Himanshu Chauhan <hchauhan@ventanamicro.com>
---
 target/riscv/pmp.c | 10 ++--------
 1 file changed, 2 insertions(+), 8 deletions(-)

diff --git a/target/riscv/pmp.c b/target/riscv/pmp.c
index 418738afd8..976b199156 100644
--- a/target/riscv/pmp.c
+++ b/target/riscv/pmp.c
@@ -291,7 +291,6 @@ bool pmp_hart_has_privs(CPURISCVState *env, target_ulong addr,
                         pmp_priv_t *allowed_privs, target_ulong mode)
 {
     int i = 0;
-    bool ret = false;
     int pmp_size = 0;
     target_ulong s = 0;
     target_ulong e = 0;
@@ -435,17 +434,12 @@ bool pmp_hart_has_privs(CPURISCVState *env, target_ulong addr,
              * defined with PMP must be used. We shouldn't fallback on
              * finding default privileges.
              */
-            ret = true;
-            break;
+            return (privs & *allowed_privs) == privs ? true : false;
         }
     }
 
     /* No rule matched */
-    if (!ret) {
-        ret = pmp_hart_has_privs_default(env, privs, allowed_privs, mode);
-    }
-
-    return ret;
+    return pmp_hart_has_privs_default(env, privs, allowed_privs, mode);
 }
 
 /*
-- 
2.34.1

Re: [PATCH v2] target/riscv: Smepmp: Return error when access permission not allowed in PMP

[Weiwei Li]

On 2023/6/5 19:49, Himanshu Chauhan wrote:
> On an address match, skip checking for default permissions and return error
> based on access defined in PMP configuration.
>
> v2 Changes:
> o Removed goto to return in place when address matches
> o Call pmp_hart_has_privs_default at the end of the loop
>
> Fixes: 90b1fafce06 ("target/riscv: Smepmp: Skip applying default rules when address matches")
> Signed-off-by: Himanshu Chauhan <hchauhan@ventanamicro.com>
> ---
>   target/riscv/pmp.c | 10 ++--------
>   1 file changed, 2 insertions(+), 8 deletions(-)
>
> diff --git a/target/riscv/pmp.c b/target/riscv/pmp.c
> index 418738afd8..976b199156 100644
> --- a/target/riscv/pmp.c
> +++ b/target/riscv/pmp.c
> @@ -291,7 +291,6 @@ bool pmp_hart_has_privs(CPURISCVState *env, target_ulong addr,
>                           pmp_priv_t *allowed_privs, target_ulong mode)
>   {
>       int i = 0;
> -    bool ret = false;
>       int pmp_size = 0;
>       target_ulong s = 0;
>       target_ulong e = 0;
> @@ -435,17 +434,12 @@ bool pmp_hart_has_privs(CPURISCVState *env, target_ulong addr,
>                * defined with PMP must be used. We shouldn't fallback on
>                * finding default privileges.
>                */
> -            ret = true;
> -            break;
> +            return (privs & *allowed_privs) == privs ? true : false;

This conditional assignment is unnecessary.

+                                  return  (privs & *allowed_privs) == 
privs;

Otherwise, Reviewed-by: Weiwei Li <liweiwei@iscas.ac.cn>

Weiwei Li

>           }
>       }
>   
>       /* No rule matched */
> -    if (!ret) {
> -        ret = pmp_hart_has_privs_default(env, privs, allowed_privs, mode);
> -    }
> -
> -    return ret;
> +    return pmp_hart_has_privs_default(env, privs, allowed_privs, mode);
>   }
>   
>   /*

Re: [PATCH] target/riscv: Smepmp: Return error when access permission not allowed in PMP

[Richard Henderson]

On 6/5/23 00:51, Himanshu Chauhan wrote:
> +            ret = ((privs & *allowed_privs) == privs ? true : false);

Never convert bool to bool in this way.


r~

[PATCH v3] target/riscv: Smepmp: Return error when access permission not allowed in PMP

[Himanshu Chauhan]

On an address match, skip checking for default permissions and return error
based on access defined in PMP configuration.

v3 Changes:
o Removed explicit return of boolean value from comparision
  of priv/allowed_priv

v2 Changes:
o Removed goto to return in place when address matches
o Call pmp_hart_has_privs_default at the end of the loop

Fixes: 90b1fafce06 ("target/riscv: Smepmp: Skip applying default rules when address matches")
Signed-off-by: Himanshu Chauhan <hchauhan@ventanamicro.com>
---
 target/riscv/pmp.c | 10 ++--------
 1 file changed, 2 insertions(+), 8 deletions(-)

diff --git a/target/riscv/pmp.c b/target/riscv/pmp.c
index 418738afd8..9d8db493e6 100644
--- a/target/riscv/pmp.c
+++ b/target/riscv/pmp.c
@@ -291,7 +291,6 @@ bool pmp_hart_has_privs(CPURISCVState *env, target_ulong addr,
                         pmp_priv_t *allowed_privs, target_ulong mode)
 {
     int i = 0;
-    bool ret = false;
     int pmp_size = 0;
     target_ulong s = 0;
     target_ulong e = 0;
@@ -435,17 +434,12 @@ bool pmp_hart_has_privs(CPURISCVState *env, target_ulong addr,
              * defined with PMP must be used. We shouldn't fallback on
              * finding default privileges.
              */
-            ret = true;
-            break;
+            return (privs & *allowed_privs) == privs;
         }
     }
 
     /* No rule matched */
-    if (!ret) {
-        ret = pmp_hart_has_privs_default(env, privs, allowed_privs, mode);
-    }
-
-    return ret;
+    return pmp_hart_has_privs_default(env, privs, allowed_privs, mode);
 }
 
 /*
-- 
2.34.1

Re: [PATCH v3] target/riscv: Smepmp: Return error when access permission not allowed in PMP

[Weiwei Li]

On 2023/6/6 00:45, Himanshu Chauhan wrote:
> On an address match, skip checking for default permissions and return error
> based on access defined in PMP configuration.
>
> v3 Changes:
> o Removed explicit return of boolean value from comparision
>    of priv/allowed_priv
>
> v2 Changes:
> o Removed goto to return in place when address matches
> o Call pmp_hart_has_privs_default at the end of the loop
>
> Fixes: 90b1fafce06 ("target/riscv: Smepmp: Skip applying default rules when address matches")
> Signed-off-by: Himanshu Chauhan <hchauhan@ventanamicro.com>
> ---

Reviewed-by: Weiwei Li <liweiwei@iscas.ac.cn>

Weiwei Li
>   target/riscv/pmp.c | 10 ++--------
>   1 file changed, 2 insertions(+), 8 deletions(-)
>
> diff --git a/target/riscv/pmp.c b/target/riscv/pmp.c
> index 418738afd8..9d8db493e6 100644
> --- a/target/riscv/pmp.c
> +++ b/target/riscv/pmp.c
> @@ -291,7 +291,6 @@ bool pmp_hart_has_privs(CPURISCVState *env, target_ulong addr,
>                           pmp_priv_t *allowed_privs, target_ulong mode)
>   {
>       int i = 0;
> -    bool ret = false;
>       int pmp_size = 0;
>       target_ulong s = 0;
>       target_ulong e = 0;
> @@ -435,17 +434,12 @@ bool pmp_hart_has_privs(CPURISCVState *env, target_ulong addr,
>                * defined with PMP must be used. We shouldn't fallback on
>                * finding default privileges.
>                */
> -            ret = true;
> -            break;
> +            return (privs & *allowed_privs) == privs;
>           }
>       }
>   
>       /* No rule matched */
> -    if (!ret) {
> -        ret = pmp_hart_has_privs_default(env, privs, allowed_privs, mode);
> -    }
> -
> -    return ret;
> +    return pmp_hart_has_privs_default(env, privs, allowed_privs, mode);
>   }
>   
>   /*

Re: [PATCH v3] target/riscv: Smepmp: Return error when access permission not allowed in PMP

[Daniel Henrique Barboza]

On 6/5/23 13:45, Himanshu Chauhan wrote:
> On an address match, skip checking for default permissions and return error
> based on access defined in PMP configuration.
> 
> v3 Changes:
> o Removed explicit return of boolean value from comparision
>    of priv/allowed_priv
> 
> v2 Changes:
> o Removed goto to return in place when address matches
> o Call pmp_hart_has_privs_default at the end of the loop
> 
> Fixes: 90b1fafce06 ("target/riscv: Smepmp: Skip applying default rules when address matches")
> Signed-off-by: Himanshu Chauhan <hchauhan@ventanamicro.com>
> ---

Reviewed-by: Daniel Henrique Barboza <dbarboza@ventanamicro.com>

>   target/riscv/pmp.c | 10 ++--------
>   1 file changed, 2 insertions(+), 8 deletions(-)
> 
> diff --git a/target/riscv/pmp.c b/target/riscv/pmp.c
> index 418738afd8..9d8db493e6 100644
> --- a/target/riscv/pmp.c
> +++ b/target/riscv/pmp.c
> @@ -291,7 +291,6 @@ bool pmp_hart_has_privs(CPURISCVState *env, target_ulong addr,
>                           pmp_priv_t *allowed_privs, target_ulong mode)
>   {
>       int i = 0;
> -    bool ret = false;
>       int pmp_size = 0;
>       target_ulong s = 0;
>       target_ulong e = 0;
> @@ -435,17 +434,12 @@ bool pmp_hart_has_privs(CPURISCVState *env, target_ulong addr,
>                * defined with PMP must be used. We shouldn't fallback on
>                * finding default privileges.
>                */
> -            ret = true;
> -            break;
> +            return (privs & *allowed_privs) == privs;
>           }
>       }
>   
>       /* No rule matched */
> -    if (!ret) {
> -        ret = pmp_hart_has_privs_default(env, privs, allowed_privs, mode);
> -    }
> -
> -    return ret;
> +    return pmp_hart_has_privs_default(env, privs, allowed_privs, mode);
>   }
>   
>   /*

Re: [PATCH v3] target/riscv: Smepmp: Return error when access permission not allowed in PMP

[Alistair Francis]

On Tue, Jun 6, 2023 at 2:47 AM Himanshu Chauhan
<hchauhan@ventanamicro.com> wrote:
>
> On an address match, skip checking for default permissions and return error
> based on access defined in PMP configuration.
>
> v3 Changes:
> o Removed explicit return of boolean value from comparision
>   of priv/allowed_priv
>
> v2 Changes:
> o Removed goto to return in place when address matches
> o Call pmp_hart_has_privs_default at the end of the loop
>
> Fixes: 90b1fafce06 ("target/riscv: Smepmp: Skip applying default rules when address matches")
> Signed-off-by: Himanshu Chauhan <hchauhan@ventanamicro.com>

Thanks!

Applied to riscv-to-apply.next

Alistair

> ---
>  target/riscv/pmp.c | 10 ++--------
>  1 file changed, 2 insertions(+), 8 deletions(-)
>
> diff --git a/target/riscv/pmp.c b/target/riscv/pmp.c
> index 418738afd8..9d8db493e6 100644
> --- a/target/riscv/pmp.c
> +++ b/target/riscv/pmp.c
> @@ -291,7 +291,6 @@ bool pmp_hart_has_privs(CPURISCVState *env, target_ulong addr,
>                          pmp_priv_t *allowed_privs, target_ulong mode)
>  {
>      int i = 0;
> -    bool ret = false;
>      int pmp_size = 0;
>      target_ulong s = 0;
>      target_ulong e = 0;
> @@ -435,17 +434,12 @@ bool pmp_hart_has_privs(CPURISCVState *env, target_ulong addr,
>               * defined with PMP must be used. We shouldn't fallback on
>               * finding default privileges.
>               */
> -            ret = true;
> -            break;
> +            return (privs & *allowed_privs) == privs;
>          }
>      }
>
>      /* No rule matched */
> -    if (!ret) {
> -        ret = pmp_hart_has_privs_default(env, privs, allowed_privs, mode);
> -    }
> -
> -    return ret;
> +    return pmp_hart_has_privs_default(env, privs, allowed_privs, mode);
>  }
>
>  /*
> --
> 2.34.1
>
>