[v2] target/riscv: Smepmp fixes to match specification

[PATCH v2 4/5] target/riscv: pmp: exit csr writes early if value was not changed

Posted by Loïc Lefort 2 weeks, 6 days ago

Signed-off-by: Loïc Lefort <loic@rivosinc.com>
Reviewed-by: Daniel Henrique Barboza <dbarboza@ventanamicro.com>
---
 target/riscv/pmp.c | 22 +++++++++++++++-------
 1 file changed, 15 insertions(+), 7 deletions(-)

diff --git a/target/riscv/pmp.c b/target/riscv/pmp.c
index c5f6cdaccb..845915e0c8 100644
--- a/target/riscv/pmp.c
+++ b/target/riscv/pmp.c
@@ -141,6 +141,11 @@ static inline uint8_t pmp_read_cfg(CPURISCVState *env, uint32_t pmp_index)
 static bool pmp_write_cfg(CPURISCVState *env, uint32_t pmp_index, uint8_t val)
 {
     if (pmp_index < MAX_RISCV_PMPS) {
+        if (env->pmp_state.pmp[pmp_index].cfg_reg == val) {
+            /* no change */
+            return false;
+        }
+
         if (pmp_is_readonly(env, pmp_index)) {
             qemu_log_mask(LOG_GUEST_ERROR,
                           "ignoring pmpcfg write - read only\n");
@@ -528,6 +533,11 @@ void pmpaddr_csr_write(CPURISCVState *env, uint32_t addr_index,
     bool is_next_cfg_tor = false;
 
     if (addr_index < MAX_RISCV_PMPS) {
+        if (env->pmp_state.pmp[addr_index].addr_reg == val) {
+            /* no change */
+            return;
+        }
+
         /*
          * In TOR mode, need to check the lock bit of the next pmp
          * (if there is a next).
@@ -544,14 +554,12 @@ void pmpaddr_csr_write(CPURISCVState *env, uint32_t addr_index,
         }
 
         if (!pmp_is_readonly(env, addr_index)) {
-            if (env->pmp_state.pmp[addr_index].addr_reg != val) {
-                env->pmp_state.pmp[addr_index].addr_reg = val;
-                pmp_update_rule_addr(env, addr_index);
-                if (is_next_cfg_tor) {
-                    pmp_update_rule_addr(env, addr_index + 1);
-                }
-                tlb_flush(env_cpu(env));
+            env->pmp_state.pmp[addr_index].addr_reg = val;
+            pmp_update_rule_addr(env, addr_index);
+            if (is_next_cfg_tor) {
+                pmp_update_rule_addr(env, addr_index + 1);
             }
+            tlb_flush(env_cpu(env));
         } else {
             qemu_log_mask(LOG_GUEST_ERROR,
                           "ignoring pmpaddr write - read only\n");
-- 
2.47.2

Re: [PATCH v2 4/5] target/riscv: pmp: exit csr writes early if value was not changed

Posted by LIU Zhiwei 5 days, 1 hour ago

On 2025/3/14 03:30, Loïc Lefort wrote:
> Signed-off-by: Loïc Lefort <loic@rivosinc.com>
> Reviewed-by: Daniel Henrique Barboza <dbarboza@ventanamicro.com>
> ---
>   target/riscv/pmp.c | 22 +++++++++++++++-------
>   1 file changed, 15 insertions(+), 7 deletions(-)
>
> diff --git a/target/riscv/pmp.c b/target/riscv/pmp.c
> index c5f6cdaccb..845915e0c8 100644
> --- a/target/riscv/pmp.c
> +++ b/target/riscv/pmp.c
> @@ -141,6 +141,11 @@ static inline uint8_t pmp_read_cfg(CPURISCVState *env, uint32_t pmp_index)
>   static bool pmp_write_cfg(CPURISCVState *env, uint32_t pmp_index, uint8_t val)
>   {
>       if (pmp_index < MAX_RISCV_PMPS) {
> +        if (env->pmp_state.pmp[pmp_index].cfg_reg == val) {
> +            /* no change */
> +            return false;
> +        }
> +
>           if (pmp_is_readonly(env, pmp_index)) {
>               qemu_log_mask(LOG_GUEST_ERROR,
>                             "ignoring pmpcfg write - read only\n");
> @@ -528,6 +533,11 @@ void pmpaddr_csr_write(CPURISCVState *env, uint32_t addr_index,
>       bool is_next_cfg_tor = false;
>   
>       if (addr_index < MAX_RISCV_PMPS) {
> +        if (env->pmp_state.pmp[addr_index].addr_reg == val) {
> +            /* no change */
> +            return;
> +        }
> +
>           /*
>            * In TOR mode, need to check the lock bit of the next pmp
>            * (if there is a next).
> @@ -544,14 +554,12 @@ void pmpaddr_csr_write(CPURISCVState *env, uint32_t addr_index,
>           }
>   
>           if (!pmp_is_readonly(env, addr_index)) {
> -            if (env->pmp_state.pmp[addr_index].addr_reg != val) {

Is there some benefit removing this if sentence?

Zhiwei

> -                env->pmp_state.pmp[addr_index].addr_reg = val;
> -                pmp_update_rule_addr(env, addr_index);
> -                if (is_next_cfg_tor) {
> -                    pmp_update_rule_addr(env, addr_index + 1);
> -                }
> -                tlb_flush(env_cpu(env));
> +            env->pmp_state.pmp[addr_index].addr_reg = val;
> +            pmp_update_rule_addr(env, addr_index);
> +            if (is_next_cfg_tor) {
> +                pmp_update_rule_addr(env, addr_index + 1);
>               }
> +            tlb_flush(env_cpu(env));
>           } else {
>               qemu_log_mask(LOG_GUEST_ERROR,
>                             "ignoring pmpaddr write - read only\n");

Re: [PATCH v2 4/5] target/riscv: pmp: exit csr writes early if value was not changed

Posted by Loïc Lefort 3 days ago

On Sat, Mar 29, 2025 at 10:03 AM LIU Zhiwei <zhiwei_liu@linux.alibaba.com>
wrote:

>
> On 2025/3/14 03:30, Loïc Lefort wrote:
> > Signed-off-by: Loïc Lefort <loic@rivosinc.com>
> > Reviewed-by: Daniel Henrique Barboza <dbarboza@ventanamicro.com>
> > ---
> >   target/riscv/pmp.c | 22 +++++++++++++++-------
> >   1 file changed, 15 insertions(+), 7 deletions(-)
> >
> > diff --git a/target/riscv/pmp.c b/target/riscv/pmp.c
> > index c5f6cdaccb..845915e0c8 100644
> > --- a/target/riscv/pmp.c
> > +++ b/target/riscv/pmp.c
> > @@ -141,6 +141,11 @@ static inline uint8_t pmp_read_cfg(CPURISCVState
> *env, uint32_t pmp_index)
> >   static bool pmp_write_cfg(CPURISCVState *env, uint32_t pmp_index,
> uint8_t val)
> >   {
> >       if (pmp_index < MAX_RISCV_PMPS) {
> > +        if (env->pmp_state.pmp[pmp_index].cfg_reg == val) {
> > +            /* no change */
> > +            return false;
> > +        }
> > +
> >           if (pmp_is_readonly(env, pmp_index)) {
> >               qemu_log_mask(LOG_GUEST_ERROR,
> >                             "ignoring pmpcfg write - read only\n");
> > @@ -528,6 +533,11 @@ void pmpaddr_csr_write(CPURISCVState *env, uint32_t
> addr_index,
> >       bool is_next_cfg_tor = false;
> >
> >       if (addr_index < MAX_RISCV_PMPS) {
> > +        if (env->pmp_state.pmp[addr_index].addr_reg == val) {
> > +            /* no change */
> > +            return;
> > +        }
> > +
> >           /*
> >            * In TOR mode, need to check the lock bit of the next pmp
> >            * (if there is a next).
> > @@ -544,14 +554,12 @@ void pmpaddr_csr_write(CPURISCVState *env,
> uint32_t addr_index,
> >           }
> >
> >           if (!pmp_is_readonly(env, addr_index)) {
> > -            if (env->pmp_state.pmp[addr_index].addr_reg != val) {
>
> Is there some benefit removing this if sentence?
>
>
> The if is not removed, it's moved to the top of the function. The goal is
to skip processing and warnings when the write would not change the value
already present in the register.For pmp_write_cfg, each pmpcfg register
contains 4 config values and some of them might be locked so we want to
avoid warnings when writing with unchanged values.
As you noted, the similar change in pmpaddr_csr_write has less benefit:
it's only a minor optimization and is done for symmetry with pmp_write_cfg.

Loïc.

>
> > -                env->pmp_state.pmp[addr_index].addr_reg = val;
> > -                pmp_update_rule_addr(env, addr_index);
> > -                if (is_next_cfg_tor) {
> > -                    pmp_update_rule_addr(env, addr_index + 1);
> > -                }
> > -                tlb_flush(env_cpu(env));
> > +            env->pmp_state.pmp[addr_index].addr_reg = val;
> > +            pmp_update_rule_addr(env, addr_index);
> > +            if (is_next_cfg_tor) {
> > +                pmp_update_rule_addr(env, addr_index + 1);
> >               }
> > +            tlb_flush(env_cpu(env));
> >           } else {
> >               qemu_log_mask(LOG_GUEST_ERROR,
> >                             "ignoring pmpaddr write - read only\n");
>

Re: [PATCH v2 4/5] target/riscv: pmp: exit csr writes early if value was not changed

Posted by LIU Zhiwei 2 days, 22 hours ago

On 2025/3/31 17:44, Loïc Lefort wrote:
>
>
> On Sat, Mar 29, 2025 at 10:03 AM LIU Zhiwei 
> <zhiwei_liu@linux.alibaba.com> wrote:
>
>
>     On 2025/3/14 03:30, Loïc Lefort wrote:
>     > Signed-off-by: Loïc Lefort <loic@rivosinc.com>
>     > Reviewed-by: Daniel Henrique Barboza <dbarboza@ventanamicro.com>
>     > ---
>     >   target/riscv/pmp.c | 22 +++++++++++++++-------
>     >   1 file changed, 15 insertions(+), 7 deletions(-)
>     >
>     > diff --git a/target/riscv/pmp.c b/target/riscv/pmp.c
>     > index c5f6cdaccb..845915e0c8 100644
>     > --- a/target/riscv/pmp.c
>     > +++ b/target/riscv/pmp.c
>     > @@ -141,6 +141,11 @@ static inline uint8_t
>     pmp_read_cfg(CPURISCVState *env, uint32_t pmp_index)
>     >   static bool pmp_write_cfg(CPURISCVState *env, uint32_t
>     pmp_index, uint8_t val)
>     >   {
>     >       if (pmp_index < MAX_RISCV_PMPS) {
>     > +        if (env->pmp_state.pmp[pmp_index].cfg_reg == val) {
>     > +            /* no change */
>     > +            return false;
>     > +        }
>     > +
>     >           if (pmp_is_readonly(env, pmp_index)) {
>     >               qemu_log_mask(LOG_GUEST_ERROR,
>     >                             "ignoring pmpcfg write - read only\n");
>     > @@ -528,6 +533,11 @@ void pmpaddr_csr_write(CPURISCVState *env,
>     uint32_t addr_index,
>     >       bool is_next_cfg_tor = false;
>     >
>     >       if (addr_index < MAX_RISCV_PMPS) {
>     > +        if (env->pmp_state.pmp[addr_index].addr_reg == val) {
>     > +            /* no change */
>     > +            return;
>     > +        }
>     > +
>     >           /*
>     >            * In TOR mode, need to check the lock bit of the next pmp
>     >            * (if there is a next).
>     > @@ -544,14 +554,12 @@ void pmpaddr_csr_write(CPURISCVState *env,
>     uint32_t addr_index,
>     >           }
>     >
>     >           if (!pmp_is_readonly(env, addr_index)) {
>     > -            if (env->pmp_state.pmp[addr_index].addr_reg != val) {
>
>     Is there some benefit removing this if sentence?
>
>
> The if is not removed, it's moved to the top of the function.

Oops! I miss it.

Reviewed-by: LIU Zhiwei <zhiwei_liu@linux.alibaba.com>

Thanks,
Zhiwei

> The goal is to skip processing and warnings when the write would not 
> change the value already present in the register.For pmp_write_cfg, 
> each pmpcfg register contains 4 config values and some of them might 
> be locked so we want to avoid warnings when writing with unchanged values.
> As you noted, the similar change in pmpaddr_csr_write has less 
> benefit: it's only a minor optimization and is done for symmetry with 
> pmp_write_cfg.

>
> Loïc.
>
>
>     > - env->pmp_state.pmp[addr_index].addr_reg = val;
>     > -                pmp_update_rule_addr(env, addr_index);
>     > -                if (is_next_cfg_tor) {
>     > -                    pmp_update_rule_addr(env, addr_index + 1);
>     > -                }
>     > -                tlb_flush(env_cpu(env));
>     > +            env->pmp_state.pmp[addr_index].addr_reg = val;
>     > +            pmp_update_rule_addr(env, addr_index);
>     > +            if (is_next_cfg_tor) {
>     > +                pmp_update_rule_addr(env, addr_index + 1);
>     >               }
>     > +            tlb_flush(env_cpu(env));
>     >           } else {
>     >               qemu_log_mask(LOG_GUEST_ERROR,
>     >                             "ignoring pmpaddr write - read only\n");
>

[PATCH v2 1/5] target/riscv: pmp: don't allow RLB to bypass rule privileges
[PATCH v2 2/5] target/riscv: pmp: move Smepmp operation conversion into a function
[PATCH v2 3/5] target/riscv: pmp: fix checks on writes to pmpcfg in Smepmp MML mode
[PATCH v2 4/5] target/riscv: pmp: exit csr writes early if value was not changed
[PATCH v2 5/5] target/riscv: pmp: remove redundant check in pmp_is_locked