write_misa() must use as much common logic as possible. We want to open
code just the bits that are exclusive to the CSR write operation and TCG
internals.

Our validation is done with riscv_cpu_validate_set_extensions(), but we
need a small tweak first. When enabling RVG we're doing:

        env->misa_ext |= RVI | RVM | RVA | RVF | RVD;
        env->misa_ext_mask = env->misa_ext;

This works fine for realize() time but this can potentially overwrite
env->misa_ext_mask if we reutilize the function for write_misa().

Instead of doing misa_ext_mask = misa_ext, sum up the RVG extensions in
misa_ext_mask as well. This won't change realize() time behavior
(misa_ext_mask will be == misa_ext) and will ensure that write_misa()
won't change misa_ext_mask by accident.

After that, rewrite write_misa() to work as follows:

- mask the write using misa_ext_mask to avoid enabling unsupported
  extensions;

- suppress RVC if the next insn isn't aligned;

- disable RVG if any of RVG dependencies are being disabled by the user;

- assign env->misa_ext and run riscv_cpu_validate_set_extensions(). On
  error, rollback env->misa_ext to its original value;

- handle RVF and MSTATUS_FS and continue as usual.

Let's keep write_misa() as experimental for now until this logic gains
enough mileage.

Signed-off-by: Daniel Henrique Barboza <dbarboza@ventanamicro.com>
Reviewed-by: Weiwei Li <liweiwei@iscas.ac.cn>
---
 target/riscv/cpu.c |  4 ++--
 target/riscv/cpu.h |  1 +
 target/riscv/csr.c | 47 ++++++++++++++++++++--------------------------
 3 files changed, 23 insertions(+), 29 deletions(-)

diff --git a/target/riscv/cpu.c b/target/riscv/cpu.c
index 7d407321aa..4fa720a39d 100644
--- a/target/riscv/cpu.c
+++ b/target/riscv/cpu.c
@@ -944,7 +944,7 @@ static void riscv_cpu_validate_misa_mxl(RISCVCPU *cpu, Error **errp)
  * Check consistency between chosen extensions while setting
  * cpu->cfg accordingly.
  */
-static void riscv_cpu_validate_set_extensions(RISCVCPU *cpu, Error **errp)
+void riscv_cpu_validate_set_extensions(RISCVCPU *cpu, Error **errp)
 {
     CPURISCVState *env = &cpu->env;
     Error *local_err = NULL;
@@ -960,7 +960,7 @@ static void riscv_cpu_validate_set_extensions(RISCVCPU *cpu, Error **errp)
         cpu->cfg.ext_ifencei = true;
 
         env->misa_ext |= RVI | RVM | RVA | RVF | RVD;
-        env->misa_ext_mask = env->misa_ext;
+        env->misa_ext_mask |= RVI | RVM | RVA | RVF | RVD;
     }
 
     if (riscv_has_ext(env, RVI) && riscv_has_ext(env, RVE)) {
diff --git a/target/riscv/cpu.h b/target/riscv/cpu.h
index 15423585d0..1f39edc687 100644
--- a/target/riscv/cpu.h
+++ b/target/riscv/cpu.h
@@ -548,6 +548,7 @@ bool riscv_cpu_tlb_fill(CPUState *cs, vaddr address, int size,
                         bool probe, uintptr_t retaddr);
 char *riscv_isa_string(RISCVCPU *cpu);
 void riscv_cpu_list(void);
+void riscv_cpu_validate_set_extensions(RISCVCPU *cpu, Error **errp);
 
 #define cpu_list riscv_cpu_list
 #define cpu_mmu_index riscv_cpu_mmu_index
diff --git a/target/riscv/csr.c b/target/riscv/csr.c
index 4451bd1263..4a3c57ea6f 100644
--- a/target/riscv/csr.c
+++ b/target/riscv/csr.c
@@ -1387,39 +1387,18 @@ static RISCVException read_misa(CPURISCVState *env, int csrno,
 static RISCVException write_misa(CPURISCVState *env, int csrno,
                                  target_ulong val)
 {
+    RISCVCPU *cpu = env_archcpu(env);
+    uint32_t orig_misa_ext = env->misa_ext;
+    Error *local_err = NULL;
+
     if (!riscv_cpu_cfg(env)->misa_w) {
         /* drop write to misa */
         return RISCV_EXCP_NONE;
     }
 
-    /* 'I' or 'E' must be present */
-    if (!(val & (RVI | RVE))) {
-        /* It is not, drop write to misa */
-        return RISCV_EXCP_NONE;
-    }
-
-    /* 'E' excludes all other extensions */
-    if (val & RVE) {
-        /*
-         * when we support 'E' we can do "val = RVE;" however
-         * for now we just drop writes if 'E' is present.
-         */
-        return RISCV_EXCP_NONE;
-    }
-
-    /*
-     * misa.MXL writes are not supported by QEMU.
-     * Drop writes to those bits.
-     */
-
     /* Mask extensions that are not supported by this hart */
     val &= env->misa_ext_mask;
 
-    /* 'D' depends on 'F', so clear 'D' if 'F' is not present */
-    if ((val & RVD) && !(val & RVF)) {
-        val &= ~RVD;
-    }
-
     /*
      * Suppress 'C' if next instruction is not aligned
      * TODO: this should check next_pc
@@ -1428,18 +1407,32 @@ static RISCVException write_misa(CPURISCVState *env, int csrno,
         val &= ~RVC;
     }
 
+    /* Disable RVG if any of its dependencies are disabled */
+    if (!(val & RVI && val & RVM && val & RVA &&
+          val & RVF && val & RVD)) {
+        val &= ~RVG;
+    }
+
     /* If nothing changed, do nothing. */
     if (val == env->misa_ext) {
         return RISCV_EXCP_NONE;
     }
 
-    if (!(val & RVF)) {
+    env->misa_ext = val;
+    riscv_cpu_validate_set_extensions(cpu, &local_err);
+    if (local_err != NULL) {
+        /* Rollback on validation error */
+        env->misa_ext = orig_misa_ext;
+
+        return RISCV_EXCP_NONE;
+    }
+
+    if (!(env->misa_ext & RVF)) {
         env->mstatus &= ~MSTATUS_FS;
     }
 
     /* flush translation cache */
     tb_flush(env_cpu(env));
-    env->misa_ext = val;
     env->xl = riscv_cpu_mxl(env);
     return RISCV_EXCP_NONE;
 }
-- 
2.40.0

Alistair,


Since this is the only patch that is being contested is it possible to apply all
the other ones to riscv-to-apply.next?

I'm asking because I'm going to send more code that will affect cpu_init() and
riscv_cpu_realize() functions, and it would be easier if the cleanups were already
in 'next'. Otherwise I'll either have to base the new code on top of this pending
series or I'll base them under riscv-to-apply.next and we'll have to deal with
conflicts.


Thanks,

Daniel

On 4/21/23 10:27, Daniel Henrique Barboza wrote:
> write_misa() must use as much common logic as possible. We want to open
> code just the bits that are exclusive to the CSR write operation and TCG
> internals.
> 
> Our validation is done with riscv_cpu_validate_set_extensions(), but we
> need a small tweak first. When enabling RVG we're doing:
> 
>          env->misa_ext |= RVI | RVM | RVA | RVF | RVD;
>          env->misa_ext_mask = env->misa_ext;
> 
> This works fine for realize() time but this can potentially overwrite
> env->misa_ext_mask if we reutilize the function for write_misa().
> 
> Instead of doing misa_ext_mask = misa_ext, sum up the RVG extensions in
> misa_ext_mask as well. This won't change realize() time behavior
> (misa_ext_mask will be == misa_ext) and will ensure that write_misa()
> won't change misa_ext_mask by accident.
> 
> After that, rewrite write_misa() to work as follows:
> 
> - mask the write using misa_ext_mask to avoid enabling unsupported
>    extensions;
> 
> - suppress RVC if the next insn isn't aligned;
> 
> - disable RVG if any of RVG dependencies are being disabled by the user;
> 
> - assign env->misa_ext and run riscv_cpu_validate_set_extensions(). On
>    error, rollback env->misa_ext to its original value;
> 
> - handle RVF and MSTATUS_FS and continue as usual.
> 
> Let's keep write_misa() as experimental for now until this logic gains
> enough mileage.
> 
> Signed-off-by: Daniel Henrique Barboza <dbarboza@ventanamicro.com>
> Reviewed-by: Weiwei Li <liweiwei@iscas.ac.cn>
> ---
>   target/riscv/cpu.c |  4 ++--
>   target/riscv/cpu.h |  1 +
>   target/riscv/csr.c | 47 ++++++++++++++++++++--------------------------
>   3 files changed, 23 insertions(+), 29 deletions(-)
> 
> diff --git a/target/riscv/cpu.c b/target/riscv/cpu.c
> index 7d407321aa..4fa720a39d 100644
> --- a/target/riscv/cpu.c
> +++ b/target/riscv/cpu.c
> @@ -944,7 +944,7 @@ static void riscv_cpu_validate_misa_mxl(RISCVCPU *cpu, Error **errp)
>    * Check consistency between chosen extensions while setting
>    * cpu->cfg accordingly.
>    */
> -static void riscv_cpu_validate_set_extensions(RISCVCPU *cpu, Error **errp)
> +void riscv_cpu_validate_set_extensions(RISCVCPU *cpu, Error **errp)
>   {
>       CPURISCVState *env = &cpu->env;
>       Error *local_err = NULL;
> @@ -960,7 +960,7 @@ static void riscv_cpu_validate_set_extensions(RISCVCPU *cpu, Error **errp)
>           cpu->cfg.ext_ifencei = true;
>   
>           env->misa_ext |= RVI | RVM | RVA | RVF | RVD;
> -        env->misa_ext_mask = env->misa_ext;
> +        env->misa_ext_mask |= RVI | RVM | RVA | RVF | RVD;
>       }
>   
>       if (riscv_has_ext(env, RVI) && riscv_has_ext(env, RVE)) {
> diff --git a/target/riscv/cpu.h b/target/riscv/cpu.h
> index 15423585d0..1f39edc687 100644
> --- a/target/riscv/cpu.h
> +++ b/target/riscv/cpu.h
> @@ -548,6 +548,7 @@ bool riscv_cpu_tlb_fill(CPUState *cs, vaddr address, int size,
>                           bool probe, uintptr_t retaddr);
>   char *riscv_isa_string(RISCVCPU *cpu);
>   void riscv_cpu_list(void);
> +void riscv_cpu_validate_set_extensions(RISCVCPU *cpu, Error **errp);
>   
>   #define cpu_list riscv_cpu_list
>   #define cpu_mmu_index riscv_cpu_mmu_index
> diff --git a/target/riscv/csr.c b/target/riscv/csr.c
> index 4451bd1263..4a3c57ea6f 100644
> --- a/target/riscv/csr.c
> +++ b/target/riscv/csr.c
> @@ -1387,39 +1387,18 @@ static RISCVException read_misa(CPURISCVState *env, int csrno,
>   static RISCVException write_misa(CPURISCVState *env, int csrno,
>                                    target_ulong val)
>   {
> +    RISCVCPU *cpu = env_archcpu(env);
> +    uint32_t orig_misa_ext = env->misa_ext;
> +    Error *local_err = NULL;
> +
>       if (!riscv_cpu_cfg(env)->misa_w) {
>           /* drop write to misa */
>           return RISCV_EXCP_NONE;
>       }
>   
> -    /* 'I' or 'E' must be present */
> -    if (!(val & (RVI | RVE))) {
> -        /* It is not, drop write to misa */
> -        return RISCV_EXCP_NONE;
> -    }
> -
> -    /* 'E' excludes all other extensions */
> -    if (val & RVE) {
> -        /*
> -         * when we support 'E' we can do "val = RVE;" however
> -         * for now we just drop writes if 'E' is present.
> -         */
> -        return RISCV_EXCP_NONE;
> -    }
> -
> -    /*
> -     * misa.MXL writes are not supported by QEMU.
> -     * Drop writes to those bits.
> -     */
> -
>       /* Mask extensions that are not supported by this hart */
>       val &= env->misa_ext_mask;
>   
> -    /* 'D' depends on 'F', so clear 'D' if 'F' is not present */
> -    if ((val & RVD) && !(val & RVF)) {
> -        val &= ~RVD;
> -    }
> -
>       /*
>        * Suppress 'C' if next instruction is not aligned
>        * TODO: this should check next_pc
> @@ -1428,18 +1407,32 @@ static RISCVException write_misa(CPURISCVState *env, int csrno,
>           val &= ~RVC;
>       }
>   
> +    /* Disable RVG if any of its dependencies are disabled */
> +    if (!(val & RVI && val & RVM && val & RVA &&
> +          val & RVF && val & RVD)) {
> +        val &= ~RVG;
> +    }
> +
>       /* If nothing changed, do nothing. */
>       if (val == env->misa_ext) {
>           return RISCV_EXCP_NONE;
>       }
>   
> -    if (!(val & RVF)) {
> +    env->misa_ext = val;
> +    riscv_cpu_validate_set_extensions(cpu, &local_err);
> +    if (local_err != NULL) {
> +        /* Rollback on validation error */
> +        env->misa_ext = orig_misa_ext;
> +
> +        return RISCV_EXCP_NONE;
> +    }
> +
> +    if (!(env->misa_ext & RVF)) {
>           env->mstatus &= ~MSTATUS_FS;
>       }
>   
>       /* flush translation cache */
>       tb_flush(env_cpu(env));
> -    env->misa_ext = val;
>       env->xl = riscv_cpu_mxl(env);
>       return RISCV_EXCP_NONE;
>   }

On Fri, May 12, 2023 at 10:42 PM Daniel Henrique Barboza
<dbarboza@ventanamicro.com> wrote:
>
> Alistair,
>
>
> Since this is the only patch that is being contested is it possible to apply all
> the other ones to riscv-to-apply.next?
>
> I'm asking because I'm going to send more code that will affect cpu_init() and
> riscv_cpu_realize() functions, and it would be easier if the cleanups were already
> in 'next'. Otherwise I'll either have to base the new code on top of this pending
> series or I'll base them under riscv-to-apply.next and we'll have to deal with
> conflicts.

Urgh, that's fair.

I just replied to your other thread. Let's throw a guest error print
in here and then we can merge it

Alistair

>
>
> Thanks,
>
> Daniel
>
> On 4/21/23 10:27, Daniel Henrique Barboza wrote:
> > write_misa() must use as much common logic as possible. We want to open
> > code just the bits that are exclusive to the CSR write operation and TCG
> > internals.
> >
> > Our validation is done with riscv_cpu_validate_set_extensions(), but we
> > need a small tweak first. When enabling RVG we're doing:
> >
> >          env->misa_ext |= RVI | RVM | RVA | RVF | RVD;
> >          env->misa_ext_mask = env->misa_ext;
> >
> > This works fine for realize() time but this can potentially overwrite
> > env->misa_ext_mask if we reutilize the function for write_misa().
> >
> > Instead of doing misa_ext_mask = misa_ext, sum up the RVG extensions in
> > misa_ext_mask as well. This won't change realize() time behavior
> > (misa_ext_mask will be == misa_ext) and will ensure that write_misa()
> > won't change misa_ext_mask by accident.
> >
> > After that, rewrite write_misa() to work as follows:
> >
> > - mask the write using misa_ext_mask to avoid enabling unsupported
> >    extensions;
> >
> > - suppress RVC if the next insn isn't aligned;
> >
> > - disable RVG if any of RVG dependencies are being disabled by the user;
> >
> > - assign env->misa_ext and run riscv_cpu_validate_set_extensions(). On
> >    error, rollback env->misa_ext to its original value;
> >
> > - handle RVF and MSTATUS_FS and continue as usual.
> >
> > Let's keep write_misa() as experimental for now until this logic gains
> > enough mileage.
> >
> > Signed-off-by: Daniel Henrique Barboza <dbarboza@ventanamicro.com>
> > Reviewed-by: Weiwei Li <liweiwei@iscas.ac.cn>
> > ---
> >   target/riscv/cpu.c |  4 ++--
> >   target/riscv/cpu.h |  1 +
> >   target/riscv/csr.c | 47 ++++++++++++++++++++--------------------------
> >   3 files changed, 23 insertions(+), 29 deletions(-)
> >
> > diff --git a/target/riscv/cpu.c b/target/riscv/cpu.c
> > index 7d407321aa..4fa720a39d 100644
> > --- a/target/riscv/cpu.c
> > +++ b/target/riscv/cpu.c
> > @@ -944,7 +944,7 @@ static void riscv_cpu_validate_misa_mxl(RISCVCPU *cpu, Error **errp)
> >    * Check consistency between chosen extensions while setting
> >    * cpu->cfg accordingly.
> >    */
> > -static void riscv_cpu_validate_set_extensions(RISCVCPU *cpu, Error **errp)
> > +void riscv_cpu_validate_set_extensions(RISCVCPU *cpu, Error **errp)
> >   {
> >       CPURISCVState *env = &cpu->env;
> >       Error *local_err = NULL;
> > @@ -960,7 +960,7 @@ static void riscv_cpu_validate_set_extensions(RISCVCPU *cpu, Error **errp)
> >           cpu->cfg.ext_ifencei = true;
> >
> >           env->misa_ext |= RVI | RVM | RVA | RVF | RVD;
> > -        env->misa_ext_mask = env->misa_ext;
> > +        env->misa_ext_mask |= RVI | RVM | RVA | RVF | RVD;
> >       }
> >
> >       if (riscv_has_ext(env, RVI) && riscv_has_ext(env, RVE)) {
> > diff --git a/target/riscv/cpu.h b/target/riscv/cpu.h
> > index 15423585d0..1f39edc687 100644
> > --- a/target/riscv/cpu.h
> > +++ b/target/riscv/cpu.h
> > @@ -548,6 +548,7 @@ bool riscv_cpu_tlb_fill(CPUState *cs, vaddr address, int size,
> >                           bool probe, uintptr_t retaddr);
> >   char *riscv_isa_string(RISCVCPU *cpu);
> >   void riscv_cpu_list(void);
> > +void riscv_cpu_validate_set_extensions(RISCVCPU *cpu, Error **errp);
> >
> >   #define cpu_list riscv_cpu_list
> >   #define cpu_mmu_index riscv_cpu_mmu_index
> > diff --git a/target/riscv/csr.c b/target/riscv/csr.c
> > index 4451bd1263..4a3c57ea6f 100644
> > --- a/target/riscv/csr.c
> > +++ b/target/riscv/csr.c
> > @@ -1387,39 +1387,18 @@ static RISCVException read_misa(CPURISCVState *env, int csrno,
> >   static RISCVException write_misa(CPURISCVState *env, int csrno,
> >                                    target_ulong val)
> >   {
> > +    RISCVCPU *cpu = env_archcpu(env);
> > +    uint32_t orig_misa_ext = env->misa_ext;
> > +    Error *local_err = NULL;
> > +
> >       if (!riscv_cpu_cfg(env)->misa_w) {
> >           /* drop write to misa */
> >           return RISCV_EXCP_NONE;
> >       }
> >
> > -    /* 'I' or 'E' must be present */
> > -    if (!(val & (RVI | RVE))) {
> > -        /* It is not, drop write to misa */
> > -        return RISCV_EXCP_NONE;
> > -    }
> > -
> > -    /* 'E' excludes all other extensions */
> > -    if (val & RVE) {
> > -        /*
> > -         * when we support 'E' we can do "val = RVE;" however
> > -         * for now we just drop writes if 'E' is present.
> > -         */
> > -        return RISCV_EXCP_NONE;
> > -    }
> > -
> > -    /*
> > -     * misa.MXL writes are not supported by QEMU.
> > -     * Drop writes to those bits.
> > -     */
> > -
> >       /* Mask extensions that are not supported by this hart */
> >       val &= env->misa_ext_mask;
> >
> > -    /* 'D' depends on 'F', so clear 'D' if 'F' is not present */
> > -    if ((val & RVD) && !(val & RVF)) {
> > -        val &= ~RVD;
> > -    }
> > -
> >       /*
> >        * Suppress 'C' if next instruction is not aligned
> >        * TODO: this should check next_pc
> > @@ -1428,18 +1407,32 @@ static RISCVException write_misa(CPURISCVState *env, int csrno,
> >           val &= ~RVC;
> >       }
> >
> > +    /* Disable RVG if any of its dependencies are disabled */
> > +    if (!(val & RVI && val & RVM && val & RVA &&
> > +          val & RVF && val & RVD)) {
> > +        val &= ~RVG;
> > +    }
> > +
> >       /* If nothing changed, do nothing. */
> >       if (val == env->misa_ext) {
> >           return RISCV_EXCP_NONE;
> >       }
> >
> > -    if (!(val & RVF)) {
> > +    env->misa_ext = val;
> > +    riscv_cpu_validate_set_extensions(cpu, &local_err);
> > +    if (local_err != NULL) {
> > +        /* Rollback on validation error */
> > +        env->misa_ext = orig_misa_ext;
> > +
> > +        return RISCV_EXCP_NONE;
> > +    }
> > +
> > +    if (!(env->misa_ext & RVF)) {
> >           env->mstatus &= ~MSTATUS_FS;
> >       }
> >
> >       /* flush translation cache */
> >       tb_flush(env_cpu(env));
> > -    env->misa_ext = val;
> >       env->xl = riscv_cpu_mxl(env);
> >       return RISCV_EXCP_NONE;
> >   }
>

On Fri, Apr 21, 2023 at 11:29 PM Daniel Henrique Barboza
<dbarboza@ventanamicro.com> wrote:
>
> write_misa() must use as much common logic as possible. We want to open
> code just the bits that are exclusive to the CSR write operation and TCG
> internals.
>
> Our validation is done with riscv_cpu_validate_set_extensions(), but we
> need a small tweak first. When enabling RVG we're doing:
>
>         env->misa_ext |= RVI | RVM | RVA | RVF | RVD;
>         env->misa_ext_mask = env->misa_ext;
>
> This works fine for realize() time but this can potentially overwrite
> env->misa_ext_mask if we reutilize the function for write_misa().
>
> Instead of doing misa_ext_mask = misa_ext, sum up the RVG extensions in
> misa_ext_mask as well. This won't change realize() time behavior
> (misa_ext_mask will be == misa_ext) and will ensure that write_misa()
> won't change misa_ext_mask by accident.
>
> After that, rewrite write_misa() to work as follows:
>
> - mask the write using misa_ext_mask to avoid enabling unsupported
>   extensions;
>
> - suppress RVC if the next insn isn't aligned;
>
> - disable RVG if any of RVG dependencies are being disabled by the user;
>
> - assign env->misa_ext and run riscv_cpu_validate_set_extensions(). On
>   error, rollback env->misa_ext to its original value;
>
> - handle RVF and MSTATUS_FS and continue as usual.
>
> Let's keep write_misa() as experimental for now until this logic gains
> enough mileage.
>
> Signed-off-by: Daniel Henrique Barboza <dbarboza@ventanamicro.com>
> Reviewed-by: Weiwei Li <liweiwei@iscas.ac.cn>
> ---
>  target/riscv/cpu.c |  4 ++--
>  target/riscv/cpu.h |  1 +
>  target/riscv/csr.c | 47 ++++++++++++++++++++--------------------------
>  3 files changed, 23 insertions(+), 29 deletions(-)
>
> diff --git a/target/riscv/cpu.c b/target/riscv/cpu.c
> index 7d407321aa..4fa720a39d 100644
> --- a/target/riscv/cpu.c
> +++ b/target/riscv/cpu.c
> @@ -944,7 +944,7 @@ static void riscv_cpu_validate_misa_mxl(RISCVCPU *cpu, Error **errp)
>   * Check consistency between chosen extensions while setting
>   * cpu->cfg accordingly.
>   */
> -static void riscv_cpu_validate_set_extensions(RISCVCPU *cpu, Error **errp)
> +void riscv_cpu_validate_set_extensions(RISCVCPU *cpu, Error **errp)
>  {
>      CPURISCVState *env = &cpu->env;
>      Error *local_err = NULL;
> @@ -960,7 +960,7 @@ static void riscv_cpu_validate_set_extensions(RISCVCPU *cpu, Error **errp)
>          cpu->cfg.ext_ifencei = true;
>
>          env->misa_ext |= RVI | RVM | RVA | RVF | RVD;
> -        env->misa_ext_mask = env->misa_ext;
> +        env->misa_ext_mask |= RVI | RVM | RVA | RVF | RVD;
>      }
>
>      if (riscv_has_ext(env, RVI) && riscv_has_ext(env, RVE)) {
> diff --git a/target/riscv/cpu.h b/target/riscv/cpu.h
> index 15423585d0..1f39edc687 100644
> --- a/target/riscv/cpu.h
> +++ b/target/riscv/cpu.h
> @@ -548,6 +548,7 @@ bool riscv_cpu_tlb_fill(CPUState *cs, vaddr address, int size,
>                          bool probe, uintptr_t retaddr);
>  char *riscv_isa_string(RISCVCPU *cpu);
>  void riscv_cpu_list(void);
> +void riscv_cpu_validate_set_extensions(RISCVCPU *cpu, Error **errp);
>
>  #define cpu_list riscv_cpu_list
>  #define cpu_mmu_index riscv_cpu_mmu_index
> diff --git a/target/riscv/csr.c b/target/riscv/csr.c
> index 4451bd1263..4a3c57ea6f 100644
> --- a/target/riscv/csr.c
> +++ b/target/riscv/csr.c
> @@ -1387,39 +1387,18 @@ static RISCVException read_misa(CPURISCVState *env, int csrno,
>  static RISCVException write_misa(CPURISCVState *env, int csrno,
>                                   target_ulong val)
>  {
> +    RISCVCPU *cpu = env_archcpu(env);
> +    uint32_t orig_misa_ext = env->misa_ext;
> +    Error *local_err = NULL;
> +
>      if (!riscv_cpu_cfg(env)->misa_w) {
>          /* drop write to misa */
>          return RISCV_EXCP_NONE;
>      }
>
> -    /* 'I' or 'E' must be present */
> -    if (!(val & (RVI | RVE))) {
> -        /* It is not, drop write to misa */
> -        return RISCV_EXCP_NONE;
> -    }
> -
> -    /* 'E' excludes all other extensions */
> -    if (val & RVE) {
> -        /*
> -         * when we support 'E' we can do "val = RVE;" however
> -         * for now we just drop writes if 'E' is present.
> -         */
> -        return RISCV_EXCP_NONE;
> -    }
> -
> -    /*
> -     * misa.MXL writes are not supported by QEMU.
> -     * Drop writes to those bits.
> -     */
> -
>      /* Mask extensions that are not supported by this hart */
>      val &= env->misa_ext_mask;
>
> -    /* 'D' depends on 'F', so clear 'D' if 'F' is not present */
> -    if ((val & RVD) && !(val & RVF)) {
> -        val &= ~RVD;
> -    }
> -
>      /*
>       * Suppress 'C' if next instruction is not aligned
>       * TODO: this should check next_pc
> @@ -1428,18 +1407,32 @@ static RISCVException write_misa(CPURISCVState *env, int csrno,
>          val &= ~RVC;
>      }
>
> +    /* Disable RVG if any of its dependencies are disabled */
> +    if (!(val & RVI && val & RVM && val & RVA &&
> +          val & RVF && val & RVD)) {
> +        val &= ~RVG;
> +    }
> +
>      /* If nothing changed, do nothing. */
>      if (val == env->misa_ext) {
>          return RISCV_EXCP_NONE;
>      }
>
> -    if (!(val & RVF)) {
> +    env->misa_ext = val;
> +    riscv_cpu_validate_set_extensions(cpu, &local_err);
> +    if (local_err != NULL) {
> +        /* Rollback on validation error */
> +        env->misa_ext = orig_misa_ext;

I don't think this is right though. The spec even states:

" An attempt to write an unsupported combination causes those bits to
be set to some supported combination."

So we should try to follow what the guest requested as closely as we
can, instead of just rolling back.

Alistair

> +
> +        return RISCV_EXCP_NONE;
> +    }
> +
> +    if (!(env->misa_ext & RVF)) {
>          env->mstatus &= ~MSTATUS_FS;
>      }
>
>      /* flush translation cache */
>      tb_flush(env_cpu(env));
> -    env->misa_ext = val;
>      env->xl = riscv_cpu_mxl(env);
>      return RISCV_EXCP_NONE;
>  }
> --
> 2.40.0
>
>

On 5/7/23 20:25, Alistair Francis wrote:
> On Fri, Apr 21, 2023 at 11:29 PM Daniel Henrique Barboza
> <dbarboza@ventanamicro.com> wrote:
>>
>> write_misa() must use as much common logic as possible. We want to open
>> code just the bits that are exclusive to the CSR write operation and TCG
>> internals.
>>
>> Our validation is done with riscv_cpu_validate_set_extensions(), but we
>> need a small tweak first. When enabling RVG we're doing:
>>
>>          env->misa_ext |= RVI | RVM | RVA | RVF | RVD;
>>          env->misa_ext_mask = env->misa_ext;
>>
>> This works fine for realize() time but this can potentially overwrite
>> env->misa_ext_mask if we reutilize the function for write_misa().
>>
>> Instead of doing misa_ext_mask = misa_ext, sum up the RVG extensions in
>> misa_ext_mask as well. This won't change realize() time behavior
>> (misa_ext_mask will be == misa_ext) and will ensure that write_misa()
>> won't change misa_ext_mask by accident.
>>
>> After that, rewrite write_misa() to work as follows:
>>
>> - mask the write using misa_ext_mask to avoid enabling unsupported
>>    extensions;
>>
>> - suppress RVC if the next insn isn't aligned;
>>
>> - disable RVG if any of RVG dependencies are being disabled by the user;
>>
>> - assign env->misa_ext and run riscv_cpu_validate_set_extensions(). On
>>    error, rollback env->misa_ext to its original value;
>>
>> - handle RVF and MSTATUS_FS and continue as usual.
>>
>> Let's keep write_misa() as experimental for now until this logic gains
>> enough mileage.
>>
>> Signed-off-by: Daniel Henrique Barboza <dbarboza@ventanamicro.com>
>> Reviewed-by: Weiwei Li <liweiwei@iscas.ac.cn>
>> ---
>>   target/riscv/cpu.c |  4 ++--
>>   target/riscv/cpu.h |  1 +
>>   target/riscv/csr.c | 47 ++++++++++++++++++++--------------------------
>>   3 files changed, 23 insertions(+), 29 deletions(-)
>>
>> diff --git a/target/riscv/cpu.c b/target/riscv/cpu.c
>> index 7d407321aa..4fa720a39d 100644
>> --- a/target/riscv/cpu.c
>> +++ b/target/riscv/cpu.c
>> @@ -944,7 +944,7 @@ static void riscv_cpu_validate_misa_mxl(RISCVCPU *cpu, Error **errp)
>>    * Check consistency between chosen extensions while setting
>>    * cpu->cfg accordingly.
>>    */
>> -static void riscv_cpu_validate_set_extensions(RISCVCPU *cpu, Error **errp)
>> +void riscv_cpu_validate_set_extensions(RISCVCPU *cpu, Error **errp)
>>   {
>>       CPURISCVState *env = &cpu->env;
>>       Error *local_err = NULL;
>> @@ -960,7 +960,7 @@ static void riscv_cpu_validate_set_extensions(RISCVCPU *cpu, Error **errp)
>>           cpu->cfg.ext_ifencei = true;
>>
>>           env->misa_ext |= RVI | RVM | RVA | RVF | RVD;
>> -        env->misa_ext_mask = env->misa_ext;
>> +        env->misa_ext_mask |= RVI | RVM | RVA | RVF | RVD;
>>       }
>>
>>       if (riscv_has_ext(env, RVI) && riscv_has_ext(env, RVE)) {
>> diff --git a/target/riscv/cpu.h b/target/riscv/cpu.h
>> index 15423585d0..1f39edc687 100644
>> --- a/target/riscv/cpu.h
>> +++ b/target/riscv/cpu.h
>> @@ -548,6 +548,7 @@ bool riscv_cpu_tlb_fill(CPUState *cs, vaddr address, int size,
>>                           bool probe, uintptr_t retaddr);
>>   char *riscv_isa_string(RISCVCPU *cpu);
>>   void riscv_cpu_list(void);
>> +void riscv_cpu_validate_set_extensions(RISCVCPU *cpu, Error **errp);
>>
>>   #define cpu_list riscv_cpu_list
>>   #define cpu_mmu_index riscv_cpu_mmu_index
>> diff --git a/target/riscv/csr.c b/target/riscv/csr.c
>> index 4451bd1263..4a3c57ea6f 100644
>> --- a/target/riscv/csr.c
>> +++ b/target/riscv/csr.c
>> @@ -1387,39 +1387,18 @@ static RISCVException read_misa(CPURISCVState *env, int csrno,
>>   static RISCVException write_misa(CPURISCVState *env, int csrno,
>>                                    target_ulong val)
>>   {
>> +    RISCVCPU *cpu = env_archcpu(env);
>> +    uint32_t orig_misa_ext = env->misa_ext;
>> +    Error *local_err = NULL;
>> +
>>       if (!riscv_cpu_cfg(env)->misa_w) {
>>           /* drop write to misa */
>>           return RISCV_EXCP_NONE;
>>       }
>>
>> -    /* 'I' or 'E' must be present */
>> -    if (!(val & (RVI | RVE))) {
>> -        /* It is not, drop write to misa */
>> -        return RISCV_EXCP_NONE;
>> -    }
>> -
>> -    /* 'E' excludes all other extensions */
>> -    if (val & RVE) {
>> -        /*
>> -         * when we support 'E' we can do "val = RVE;" however
>> -         * for now we just drop writes if 'E' is present.
>> -         */
>> -        return RISCV_EXCP_NONE;
>> -    }
>> -
>> -    /*
>> -     * misa.MXL writes are not supported by QEMU.
>> -     * Drop writes to those bits.
>> -     */
>> -
>>       /* Mask extensions that are not supported by this hart */
>>       val &= env->misa_ext_mask;
>>
>> -    /* 'D' depends on 'F', so clear 'D' if 'F' is not present */
>> -    if ((val & RVD) && !(val & RVF)) {
>> -        val &= ~RVD;
>> -    }
>> -
>>       /*
>>        * Suppress 'C' if next instruction is not aligned
>>        * TODO: this should check next_pc
>> @@ -1428,18 +1407,32 @@ static RISCVException write_misa(CPURISCVState *env, int csrno,
>>           val &= ~RVC;
>>       }
>>
>> +    /* Disable RVG if any of its dependencies are disabled */
>> +    if (!(val & RVI && val & RVM && val & RVA &&
>> +          val & RVF && val & RVD)) {
>> +        val &= ~RVG;
>> +    }
>> +
>>       /* If nothing changed, do nothing. */
>>       if (val == env->misa_ext) {
>>           return RISCV_EXCP_NONE;
>>       }
>>
>> -    if (!(val & RVF)) {
>> +    env->misa_ext = val;
>> +    riscv_cpu_validate_set_extensions(cpu, &local_err);
>> +    if (local_err != NULL) {
>> +        /* Rollback on validation error */
>> +        env->misa_ext = orig_misa_ext;
> 
> I don't think this is right though. The spec even states:
> 
> " An attempt to write an unsupported combination causes those bits to
> be set to some supported combination."

Rolling back to the previous state (which means do nothing) is a supported
combination. I don't think the spec forbids what we're doing here.


> 
> So we should try to follow what the guest requested as closely as we
> can, instead of just rolling back.

Let's say we have this:

RVI | RVM | RVA | RVF | RVD | RVC | RVU | RVV

And the userspace wants to write this (clear RVF):

RVI | RVM | RVA | RVD | RVC | RVU | RVV


What should we do in this case? Removing RVF would imply removing RVD and RVV, so
that's one alternative. Another alternative is consider that the user should
clear all bits of RVV explicitly, so we ignore the RVF clear and keep it as it
is (which we're already doing in this patch).

Note that both are right depending on the user intention. And this is why I don't
believe that follow what the user attempted to do, or "follow what the guest as
closely as we can" is viable.

An alternative, instead of keeping the original misa_ext value, is to do
misa_ext = misa_ext_mask. This would mean "in case of a failed validation,
since we don't know what the user tried to do, re-enable everything".

Anything other than keeping the existing misa_ext, like this patch is already
doing, or setting misa_ext to misa_ext_mask, will demand more code and assumptions
based on what we can't control (user intentions).


Thanks,


Daniel


> 
> Alistair
> 
>> +
>> +        return RISCV_EXCP_NONE;
>> +    }
>> +
>> +    if (!(env->misa_ext & RVF)) {
>>           env->mstatus &= ~MSTATUS_FS;
>>       }
>>
>>       /* flush translation cache */
>>       tb_flush(env_cpu(env));
>> -    env->misa_ext = val;
>>       env->xl = riscv_cpu_mxl(env);
>>       return RISCV_EXCP_NONE;
>>   }
>> --
>> 2.40.0
>>
>>

On Mon, May 8, 2023 at 8:29 PM Daniel Henrique Barboza
<dbarboza@ventanamicro.com> wrote:
>
>
>
> On 5/7/23 20:25, Alistair Francis wrote:
> > On Fri, Apr 21, 2023 at 11:29 PM Daniel Henrique Barboza
> > <dbarboza@ventanamicro.com> wrote:
> >>
> >> write_misa() must use as much common logic as possible. We want to open
> >> code just the bits that are exclusive to the CSR write operation and TCG
> >> internals.
> >>
> >> Our validation is done with riscv_cpu_validate_set_extensions(), but we
> >> need a small tweak first. When enabling RVG we're doing:
> >>
> >>          env->misa_ext |= RVI | RVM | RVA | RVF | RVD;
> >>          env->misa_ext_mask = env->misa_ext;
> >>
> >> This works fine for realize() time but this can potentially overwrite
> >> env->misa_ext_mask if we reutilize the function for write_misa().
> >>
> >> Instead of doing misa_ext_mask = misa_ext, sum up the RVG extensions in
> >> misa_ext_mask as well. This won't change realize() time behavior
> >> (misa_ext_mask will be == misa_ext) and will ensure that write_misa()
> >> won't change misa_ext_mask by accident.
> >>
> >> After that, rewrite write_misa() to work as follows:
> >>
> >> - mask the write using misa_ext_mask to avoid enabling unsupported
> >>    extensions;
> >>
> >> - suppress RVC if the next insn isn't aligned;
> >>
> >> - disable RVG if any of RVG dependencies are being disabled by the user;
> >>
> >> - assign env->misa_ext and run riscv_cpu_validate_set_extensions(). On
> >>    error, rollback env->misa_ext to its original value;
> >>
> >> - handle RVF and MSTATUS_FS and continue as usual.
> >>
> >> Let's keep write_misa() as experimental for now until this logic gains
> >> enough mileage.
> >>
> >> Signed-off-by: Daniel Henrique Barboza <dbarboza@ventanamicro.com>
> >> Reviewed-by: Weiwei Li <liweiwei@iscas.ac.cn>
> >> ---
> >>   target/riscv/cpu.c |  4 ++--
> >>   target/riscv/cpu.h |  1 +
> >>   target/riscv/csr.c | 47 ++++++++++++++++++++--------------------------
> >>   3 files changed, 23 insertions(+), 29 deletions(-)
> >>
> >> diff --git a/target/riscv/cpu.c b/target/riscv/cpu.c
> >> index 7d407321aa..4fa720a39d 100644
> >> --- a/target/riscv/cpu.c
> >> +++ b/target/riscv/cpu.c
> >> @@ -944,7 +944,7 @@ static void riscv_cpu_validate_misa_mxl(RISCVCPU *cpu, Error **errp)
> >>    * Check consistency between chosen extensions while setting
> >>    * cpu->cfg accordingly.
> >>    */
> >> -static void riscv_cpu_validate_set_extensions(RISCVCPU *cpu, Error **errp)
> >> +void riscv_cpu_validate_set_extensions(RISCVCPU *cpu, Error **errp)
> >>   {
> >>       CPURISCVState *env = &cpu->env;
> >>       Error *local_err = NULL;
> >> @@ -960,7 +960,7 @@ static void riscv_cpu_validate_set_extensions(RISCVCPU *cpu, Error **errp)
> >>           cpu->cfg.ext_ifencei = true;
> >>
> >>           env->misa_ext |= RVI | RVM | RVA | RVF | RVD;
> >> -        env->misa_ext_mask = env->misa_ext;
> >> +        env->misa_ext_mask |= RVI | RVM | RVA | RVF | RVD;
> >>       }
> >>
> >>       if (riscv_has_ext(env, RVI) && riscv_has_ext(env, RVE)) {
> >> diff --git a/target/riscv/cpu.h b/target/riscv/cpu.h
> >> index 15423585d0..1f39edc687 100644
> >> --- a/target/riscv/cpu.h
> >> +++ b/target/riscv/cpu.h
> >> @@ -548,6 +548,7 @@ bool riscv_cpu_tlb_fill(CPUState *cs, vaddr address, int size,
> >>                           bool probe, uintptr_t retaddr);
> >>   char *riscv_isa_string(RISCVCPU *cpu);
> >>   void riscv_cpu_list(void);
> >> +void riscv_cpu_validate_set_extensions(RISCVCPU *cpu, Error **errp);
> >>
> >>   #define cpu_list riscv_cpu_list
> >>   #define cpu_mmu_index riscv_cpu_mmu_index
> >> diff --git a/target/riscv/csr.c b/target/riscv/csr.c
> >> index 4451bd1263..4a3c57ea6f 100644
> >> --- a/target/riscv/csr.c
> >> +++ b/target/riscv/csr.c
> >> @@ -1387,39 +1387,18 @@ static RISCVException read_misa(CPURISCVState *env, int csrno,
> >>   static RISCVException write_misa(CPURISCVState *env, int csrno,
> >>                                    target_ulong val)
> >>   {
> >> +    RISCVCPU *cpu = env_archcpu(env);
> >> +    uint32_t orig_misa_ext = env->misa_ext;
> >> +    Error *local_err = NULL;
> >> +
> >>       if (!riscv_cpu_cfg(env)->misa_w) {
> >>           /* drop write to misa */
> >>           return RISCV_EXCP_NONE;
> >>       }
> >>
> >> -    /* 'I' or 'E' must be present */
> >> -    if (!(val & (RVI | RVE))) {
> >> -        /* It is not, drop write to misa */
> >> -        return RISCV_EXCP_NONE;
> >> -    }
> >> -
> >> -    /* 'E' excludes all other extensions */
> >> -    if (val & RVE) {
> >> -        /*
> >> -         * when we support 'E' we can do "val = RVE;" however
> >> -         * for now we just drop writes if 'E' is present.
> >> -         */
> >> -        return RISCV_EXCP_NONE;
> >> -    }
> >> -
> >> -    /*
> >> -     * misa.MXL writes are not supported by QEMU.
> >> -     * Drop writes to those bits.
> >> -     */
> >> -
> >>       /* Mask extensions that are not supported by this hart */
> >>       val &= env->misa_ext_mask;
> >>
> >> -    /* 'D' depends on 'F', so clear 'D' if 'F' is not present */
> >> -    if ((val & RVD) && !(val & RVF)) {
> >> -        val &= ~RVD;
> >> -    }
> >> -
> >>       /*
> >>        * Suppress 'C' if next instruction is not aligned
> >>        * TODO: this should check next_pc
> >> @@ -1428,18 +1407,32 @@ static RISCVException write_misa(CPURISCVState *env, int csrno,
> >>           val &= ~RVC;
> >>       }
> >>
> >> +    /* Disable RVG if any of its dependencies are disabled */
> >> +    if (!(val & RVI && val & RVM && val & RVA &&
> >> +          val & RVF && val & RVD)) {
> >> +        val &= ~RVG;
> >> +    }
> >> +
> >>       /* If nothing changed, do nothing. */
> >>       if (val == env->misa_ext) {
> >>           return RISCV_EXCP_NONE;
> >>       }
> >>
> >> -    if (!(val & RVF)) {
> >> +    env->misa_ext = val;
> >> +    riscv_cpu_validate_set_extensions(cpu, &local_err);
> >> +    if (local_err != NULL) {
> >> +        /* Rollback on validation error */
> >> +        env->misa_ext = orig_misa_ext;
> >
> > I don't think this is right though. The spec even states:
> >
> > " An attempt to write an unsupported combination causes those bits to
> > be set to some supported combination."
>
> Rolling back to the previous state (which means do nothing) is a supported
> combination. I don't think the spec forbids what we're doing here.

It doesn't forbid it, but it also doesn't really encourage it.

>
>
> >
> > So we should try to follow what the guest requested as closely as we
> > can, instead of just rolling back.
>
> Let's say we have this:
>
> RVI | RVM | RVA | RVF | RVD | RVC | RVU | RVV
>
> And the userspace wants to write this (clear RVF):
>
> RVI | RVM | RVA | RVD | RVC | RVU | RVV
>
>
> What should we do in this case? Removing RVF would imply removing RVD and RVV, so
> that's one alternative. Another alternative is consider that the user should
> clear all bits of RVV explicitly, so we ignore the RVF clear and keep it as it
> is (which we're already doing in this patch).

In this case we should keep RVF enabled, I agree.

What I'm thinking of, is what happens if we have:

RVI | RVM | RVA | RVF | RVD | RVC | RVU | RVV

and the user writes (clears RVF and RVV)

RVI | RVM | RVA | RVD | RVC | RVU

We can't disable RVF, because RVD requires it, but we can disable RVV.
So we should end up with just RVV disabled

RVI | RVM | RVA | RVF | RVD | RVC | RVU

From my interpretation of the spec, that's what we should be supporting.

>
> Note that both are right depending on the user intention. And this is why I don't
> believe that follow what the user attempted to do, or "follow what the guest as
> closely as we can" is viable.
>
> An alternative, instead of keeping the original misa_ext value, is to do
> misa_ext = misa_ext_mask. This would mean "in case of a failed validation,
> since we don't know what the user tried to do, re-enable everything".

I think that's even worse :)

>
> Anything other than keeping the existing misa_ext, like this patch is already
> doing, or setting misa_ext to misa_ext_mask, will demand more code and assumptions
> based on what we can't control (user intentions).

Hmm... That's fair. It does make the process more complex, although we
are already checking if the combination is valid.

Ok, how about we at least report a guest error if the combination is
invalid and we roll back. Then we can go ahead with the patch.

Alistair

>
>
> Thanks,
>
>
> Daniel
>
>
> >
> > Alistair
> >
> >> +
> >> +        return RISCV_EXCP_NONE;
> >> +    }
> >> +
> >> +    if (!(env->misa_ext & RVF)) {
> >>           env->mstatus &= ~MSTATUS_FS;
> >>       }
> >>
> >>       /* flush translation cache */
> >>       tb_flush(env_cpu(env));
> >> -    env->misa_ext = val;
> >>       env->xl = riscv_cpu_mxl(env);
> >>       return RISCV_EXCP_NONE;
> >>   }
> >> --
> >> 2.40.0
> >>
> >>

On 5/17/23 01:48, Alistair Francis wrote:
> On Mon, May 8, 2023 at 8:29 PM Daniel Henrique Barboza
> <dbarboza@ventanamicro.com> wrote:
>>
>>
>>
>> On 5/7/23 20:25, Alistair Francis wrote:
>>> On Fri, Apr 21, 2023 at 11:29 PM Daniel Henrique Barboza
>>> <dbarboza@ventanamicro.com> wrote:
>>>>
>>>> write_misa() must use as much common logic as possible. We want to open
>>>> code just the bits that are exclusive to the CSR write operation and TCG
>>>> internals.
>>>>
>>>> Our validation is done with riscv_cpu_validate_set_extensions(), but we
>>>> need a small tweak first. When enabling RVG we're doing:
>>>>
>>>>           env->misa_ext |= RVI | RVM | RVA | RVF | RVD;
>>>>           env->misa_ext_mask = env->misa_ext;
>>>>
>>>> This works fine for realize() time but this can potentially overwrite
>>>> env->misa_ext_mask if we reutilize the function for write_misa().
>>>>
>>>> Instead of doing misa_ext_mask = misa_ext, sum up the RVG extensions in
>>>> misa_ext_mask as well. This won't change realize() time behavior
>>>> (misa_ext_mask will be == misa_ext) and will ensure that write_misa()
>>>> won't change misa_ext_mask by accident.
>>>>
>>>> After that, rewrite write_misa() to work as follows:
>>>>
>>>> - mask the write using misa_ext_mask to avoid enabling unsupported
>>>>     extensions;
>>>>
>>>> - suppress RVC if the next insn isn't aligned;
>>>>
>>>> - disable RVG if any of RVG dependencies are being disabled by the user;
>>>>
>>>> - assign env->misa_ext and run riscv_cpu_validate_set_extensions(). On
>>>>     error, rollback env->misa_ext to its original value;
>>>>
>>>> - handle RVF and MSTATUS_FS and continue as usual.
>>>>
>>>> Let's keep write_misa() as experimental for now until this logic gains
>>>> enough mileage.
>>>>
>>>> Signed-off-by: Daniel Henrique Barboza <dbarboza@ventanamicro.com>
>>>> Reviewed-by: Weiwei Li <liweiwei@iscas.ac.cn>
>>>> ---
>>>>    target/riscv/cpu.c |  4 ++--
>>>>    target/riscv/cpu.h |  1 +
>>>>    target/riscv/csr.c | 47 ++++++++++++++++++++--------------------------
>>>>    3 files changed, 23 insertions(+), 29 deletions(-)
>>>>
>>>> diff --git a/target/riscv/cpu.c b/target/riscv/cpu.c
>>>> index 7d407321aa..4fa720a39d 100644
>>>> --- a/target/riscv/cpu.c
>>>> +++ b/target/riscv/cpu.c
>>>> @@ -944,7 +944,7 @@ static void riscv_cpu_validate_misa_mxl(RISCVCPU *cpu, Error **errp)
>>>>     * Check consistency between chosen extensions while setting
>>>>     * cpu->cfg accordingly.
>>>>     */
>>>> -static void riscv_cpu_validate_set_extensions(RISCVCPU *cpu, Error **errp)
>>>> +void riscv_cpu_validate_set_extensions(RISCVCPU *cpu, Error **errp)
>>>>    {
>>>>        CPURISCVState *env = &cpu->env;
>>>>        Error *local_err = NULL;
>>>> @@ -960,7 +960,7 @@ static void riscv_cpu_validate_set_extensions(RISCVCPU *cpu, Error **errp)
>>>>            cpu->cfg.ext_ifencei = true;
>>>>
>>>>            env->misa_ext |= RVI | RVM | RVA | RVF | RVD;
>>>> -        env->misa_ext_mask = env->misa_ext;
>>>> +        env->misa_ext_mask |= RVI | RVM | RVA | RVF | RVD;
>>>>        }
>>>>
>>>>        if (riscv_has_ext(env, RVI) && riscv_has_ext(env, RVE)) {
>>>> diff --git a/target/riscv/cpu.h b/target/riscv/cpu.h
>>>> index 15423585d0..1f39edc687 100644
>>>> --- a/target/riscv/cpu.h
>>>> +++ b/target/riscv/cpu.h
>>>> @@ -548,6 +548,7 @@ bool riscv_cpu_tlb_fill(CPUState *cs, vaddr address, int size,
>>>>                            bool probe, uintptr_t retaddr);
>>>>    char *riscv_isa_string(RISCVCPU *cpu);
>>>>    void riscv_cpu_list(void);
>>>> +void riscv_cpu_validate_set_extensions(RISCVCPU *cpu, Error **errp);
>>>>
>>>>    #define cpu_list riscv_cpu_list
>>>>    #define cpu_mmu_index riscv_cpu_mmu_index
>>>> diff --git a/target/riscv/csr.c b/target/riscv/csr.c
>>>> index 4451bd1263..4a3c57ea6f 100644
>>>> --- a/target/riscv/csr.c
>>>> +++ b/target/riscv/csr.c
>>>> @@ -1387,39 +1387,18 @@ static RISCVException read_misa(CPURISCVState *env, int csrno,
>>>>    static RISCVException write_misa(CPURISCVState *env, int csrno,
>>>>                                     target_ulong val)
>>>>    {
>>>> +    RISCVCPU *cpu = env_archcpu(env);
>>>> +    uint32_t orig_misa_ext = env->misa_ext;
>>>> +    Error *local_err = NULL;
>>>> +
>>>>        if (!riscv_cpu_cfg(env)->misa_w) {
>>>>            /* drop write to misa */
>>>>            return RISCV_EXCP_NONE;
>>>>        }
>>>>
>>>> -    /* 'I' or 'E' must be present */
>>>> -    if (!(val & (RVI | RVE))) {
>>>> -        /* It is not, drop write to misa */
>>>> -        return RISCV_EXCP_NONE;
>>>> -    }
>>>> -
>>>> -    /* 'E' excludes all other extensions */
>>>> -    if (val & RVE) {
>>>> -        /*
>>>> -         * when we support 'E' we can do "val = RVE;" however
>>>> -         * for now we just drop writes if 'E' is present.
>>>> -         */
>>>> -        return RISCV_EXCP_NONE;
>>>> -    }
>>>> -
>>>> -    /*
>>>> -     * misa.MXL writes are not supported by QEMU.
>>>> -     * Drop writes to those bits.
>>>> -     */
>>>> -
>>>>        /* Mask extensions that are not supported by this hart */
>>>>        val &= env->misa_ext_mask;
>>>>
>>>> -    /* 'D' depends on 'F', so clear 'D' if 'F' is not present */
>>>> -    if ((val & RVD) && !(val & RVF)) {
>>>> -        val &= ~RVD;
>>>> -    }
>>>> -
>>>>        /*
>>>>         * Suppress 'C' if next instruction is not aligned
>>>>         * TODO: this should check next_pc
>>>> @@ -1428,18 +1407,32 @@ static RISCVException write_misa(CPURISCVState *env, int csrno,
>>>>            val &= ~RVC;
>>>>        }
>>>>
>>>> +    /* Disable RVG if any of its dependencies are disabled */
>>>> +    if (!(val & RVI && val & RVM && val & RVA &&
>>>> +          val & RVF && val & RVD)) {
>>>> +        val &= ~RVG;
>>>> +    }
>>>> +
>>>>        /* If nothing changed, do nothing. */
>>>>        if (val == env->misa_ext) {
>>>>            return RISCV_EXCP_NONE;
>>>>        }
>>>>
>>>> -    if (!(val & RVF)) {
>>>> +    env->misa_ext = val;
>>>> +    riscv_cpu_validate_set_extensions(cpu, &local_err);
>>>> +    if (local_err != NULL) {
>>>> +        /* Rollback on validation error */
>>>> +        env->misa_ext = orig_misa_ext;
>>>
>>> I don't think this is right though. The spec even states:
>>>
>>> " An attempt to write an unsupported combination causes those bits to
>>> be set to some supported combination."
>>
>> Rolling back to the previous state (which means do nothing) is a supported
>> combination. I don't think the spec forbids what we're doing here.
> 
> It doesn't forbid it, but it also doesn't really encourage it.
> 
>>
>>
>>>
>>> So we should try to follow what the guest requested as closely as we
>>> can, instead of just rolling back.
>>
>> Let's say we have this:
>>
>> RVI | RVM | RVA | RVF | RVD | RVC | RVU | RVV
>>
>> And the userspace wants to write this (clear RVF):
>>
>> RVI | RVM | RVA | RVD | RVC | RVU | RVV
>>
>>
>> What should we do in this case? Removing RVF would imply removing RVD and RVV, so
>> that's one alternative. Another alternative is consider that the user should
>> clear all bits of RVV explicitly, so we ignore the RVF clear and keep it as it
>> is (which we're already doing in this patch).
> 
> In this case we should keep RVF enabled, I agree.
> 
> What I'm thinking of, is what happens if we have:
> 
> RVI | RVM | RVA | RVF | RVD | RVC | RVU | RVV
> 
> and the user writes (clears RVF and RVV)
> 
> RVI | RVM | RVA | RVD | RVC | RVU
> 
> We can't disable RVF, because RVD requires it, but we can disable RVV.
> So we should end up with just RVV disabled
> 
> RVI | RVM | RVA | RVF | RVD | RVC | RVU
> 
>  From my interpretation of the spec, that's what we should be supporting.
> 
>>
>> Note that both are right depending on the user intention. And this is why I don't
>> believe that follow what the user attempted to do, or "follow what the guest as
>> closely as we can" is viable.
>>
>> An alternative, instead of keeping the original misa_ext value, is to do
>> misa_ext = misa_ext_mask. This would mean "in case of a failed validation,
>> since we don't know what the user tried to do, re-enable everything".
> 
> I think that's even worse :)
> 
>>
>> Anything other than keeping the existing misa_ext, like this patch is already
>> doing, or setting misa_ext to misa_ext_mask, will demand more code and assumptions
>> based on what we can't control (user intentions).
> 
> Hmm... That's fair. It does make the process more complex, although we
> are already checking if the combination is valid.
> 
> Ok, how about we at least report a guest error if the combination is
> invalid and we roll back. Then we can go ahead with the patch.

Sure! I'll make the change and re-send.


Daniel


> 
> Alistair
> 
>>
>>
>> Thanks,
>>
>>
>> Daniel
>>
>>
>>>
>>> Alistair
>>>
>>>> +
>>>> +        return RISCV_EXCP_NONE;
>>>> +    }
>>>> +
>>>> +    if (!(env->misa_ext & RVF)) {
>>>>            env->mstatus &= ~MSTATUS_FS;
>>>>        }
>>>>
>>>>        /* flush translation cache */
>>>>        tb_flush(env_cpu(env));
>>>> -    env->misa_ext = val;
>>>>        env->xl = riscv_cpu_mxl(env);
>>>>        return RISCV_EXCP_NONE;
>>>>    }
>>>> --
>>>> 2.40.0
>>>>
>>>>