On 2025/3/31 17:44, Loïc Lefort wrote:
On Sat, Mar 29, 2025 at 10:03 AM LIU Zhiwei
<zhiwei_...@linux.alibaba.com> wrote:
On 2025/3/14 03:30, Loïc Lefort wrote:
> Signed-off-by: Loïc Lefort <l...@rivosinc.com>
> Reviewed-by: Daniel Henrique Barboza <dbarb...@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_...@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");
