Re: [PATCH for-8.1 v2 25/26] target/riscv: rework write_misa()

[liweiwei]

On 2023/3/16 04:37, Daniel Henrique Barboza wrote:
> On 3/15/23 02:25, liweiwei wrote:
> > On 2023/3/15 00:49, 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.
> > >
> > > Rewrite write_misa() to work as follows:
> > >
> > > - supress RVC right after verifying that we're not updating RVG;
> > >
> > > - mask the write using misa_ext_mask to avoid enabling unsupported
> > >    extensions;
> > >
> > > - emulate the steps done by realize(): validate the candidate misa_ext
> > >    val, then validate the configuration with the candidate misa_ext 
> > > val,
> > >    and finally commit the changes to cpu->cfg.
> > >
> > > If any of the validation steps fails simply ignore the write operation.
> > >
> > > Let's keep write_misa() as experimental for now until this logic gains
> > > enough mileage.
> > >
> > > Signed-off-by: Daniel Henrique Barboza <dbarboza@ventanamicro.com>
> > > ---
> > >   target/riscv/cpu.c | 12 +++++-------
> > >   target/riscv/cpu.h |  6 ++++++
> > >   target/riscv/csr.c | 47 
> > > +++++++++++++++++++++-------------------------
> > >   3 files changed, 32 insertions(+), 33 deletions(-)
> > >
> > > diff --git a/target/riscv/cpu.c b/target/riscv/cpu.c
> > > index 5bd92e1cda..4789a7b70d 100644
> > > --- a/target/riscv/cpu.c
> > > +++ b/target/riscv/cpu.c
> > > @@ -1027,9 +1027,8 @@ static void 
> > > riscv_cpu_disable_priv_spec_isa_exts(RISCVCPU *cpu)
> > >   }
> > > -static void riscv_cpu_validate_misa_ext(CPURISCVState *env,
> > > -                                        uint32_t misa_ext,
> > > -                                        Error **errp)
> > > +void riscv_cpu_validate_misa_ext(CPURISCVState *env, uint32_t 
> > > misa_ext,
> > > +                                 Error **errp)
> > >   {
> > >       Error *local_err = NULL;
> > > @@ -1134,9 +1133,8 @@ static void 
> > > riscv_cpu_validate_misa_mxl(RISCVCPU *cpu, Error **errp)
> > >    * candidate misa_ext value. No changes in env->misa_ext
> > >    * are made.
> > >    */
> > > -static void riscv_cpu_validate_extensions(RISCVCPU *cpu,
> > > -                                          uint32_t misa_ext,
> > > -                                          Error **errp)
> > > +void riscv_cpu_validate_extensions(RISCVCPU *cpu, uint32_t misa_ext,
> > > +                                   Error **errp)
> > >   {
> > >       if (cpu->cfg.epmp && !cpu->cfg.pmp) {
> > >           /*
> > > @@ -1227,7 +1225,7 @@ static void 
> > > riscv_cpu_validate_extensions(RISCVCPU *cpu,
> > >       }
> > >   }
> > > -static void riscv_cpu_commit_cpu_cfg(RISCVCPU *cpu)
> > > +void riscv_cpu_commit_cpu_cfg(RISCVCPU *cpu)
> > >   {
> > >       if (cpu->cfg.ext_zk) {
> > >           cpu->cfg.ext_zkn = true;
> > > diff --git a/target/riscv/cpu.h b/target/riscv/cpu.h
> > > index dbb4df9df0..ca2ba6a647 100644
> > > --- a/target/riscv/cpu.h
> > > +++ b/target/riscv/cpu.h
> > > @@ -593,6 +593,12 @@ bool riscv_cpu_tlb_fill(CPUState *cs, vaddr 
> > > address, int size,
> > >   char *riscv_isa_string(RISCVCPU *cpu);
> > >   void riscv_cpu_list(void);
> > > +void riscv_cpu_validate_misa_ext(CPURISCVState *env, uint32_t 
> > > misa_ext,
> > > +                                 Error **errp);
> > > +void riscv_cpu_validate_extensions(RISCVCPU *cpu, uint32_t misa_ext,
> > > +                                   Error **errp);
> > > +void riscv_cpu_commit_cpu_cfg(RISCVCPU *cpu);
> > > +
> > >   #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 918d442ebd..6f26e7dbcd 100644
> > > --- a/target/riscv/csr.c
> > > +++ b/target/riscv/csr.c
> > > @@ -1343,6 +1343,9 @@ static RISCVException read_misa(CPURISCVState 
> > > *env, int csrno,
> > >   static RISCVException write_misa(CPURISCVState *env, int csrno,
> > >                                    target_ulong val)
> > >   {
> > > +    RISCVCPU *cpu = env_archcpu(env);
> > > +    Error *local_err = NULL;
> > > +
> > >       if (!riscv_cpu_cfg(env)->misa_w) {
> > >           /* drop write to misa */
> > >           return RISCV_EXCP_NONE;
> > > @@ -1353,47 +1356,39 @@ static RISCVException 
> > > write_misa(CPURISCVState *env, int csrno,
> > >           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.
> > > +     * Suppress 'C' if next instruction is not aligned
> > > +     * TODO: this should check next_pc
> > >        */
> > > +    if ((val & RVC) && (GETPC() & ~3) != 0) {
> > > +        val &= ~RVC;
> > > +    }
> > >       /* 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;
> > > +    /* If nothing changed, do nothing. */
> > > +    if (val == env->misa_ext) {
> > > +        return RISCV_EXCP_NONE;
> > >       }
> > >       /*
> > > -     * Suppress 'C' if next instruction is not aligned
> > > -     * TODO: this should check next_pc
> > > +     * This flow is similar to what riscv_cpu_realize() does,
> > > +     * with the difference that we will update env->misa_ext
> > > +     * value if everything is ok.
> > >        */
> > > -    if ((val & RVC) && (GETPC() & ~3) != 0) {
> > > -        val &= ~RVC;
> > > +    riscv_cpu_validate_misa_ext(env, val, &local_err);
> > > +    if (local_err != NULL) {
> > > +        return RISCV_EXCP_NONE;
> > >       }
> > > -    /* If nothing changed, do nothing. */
> > > -    if (val == env->misa_ext) {
> > > +    riscv_cpu_validate_extensions(cpu, val, &local_err);
> > > +    if (local_err != NULL) {
> > >           return RISCV_EXCP_NONE;
> > >       }
> > > +    riscv_cpu_commit_cpu_cfg(cpu);
> > > +
> >
> > In this way, it seems that Disabling V in misa may be enabled but 
> > will not work, since Zve64d/f... is still true.
> >
> > The similar questions for C when Zc* extension is supported.
> >
> > And in this way, if multi-letter extensions(such as Zfh) which depend 
> > on misa extensions(F) are supported, whether the misa extensions can 
> > be disabled? The answer is 'NO' in current implementation.
>
>
> One problem we have here is that we can't re-enable Z extensions via 
> CSR writes (at
> least as far as I'm aware of). This means that if write_misa() 
> disables a Z extension
> we don't have a reliable way of bringing it back. Enabling a Z 
> extension via a
> write_misa() call is less problematic.
>
> So I believe we have this hard rule: we don't disable Z extensions in 
> write_misa().
>
> With this in mind, I took a look at all MISA bits. I believe it's good 
> to have some special
> cases that would be prioritized in the CSR write. By special cases I 
> mean bits that would
> cause more bits/z extensions to be enabled. We would prioritize 
> handling them in write_misa(),
> dropping the changes of all other bits. I.e. if a special case is 
> detected, we handle it
> and we finish the CSR write. This would spare us from covering a lot 
> of weird cases, e.g.
> RVG being enabled while RVA is being disabled. In this case RVG takes 
> precedence.
>
> - RVE
>
> RVE. RVE requires everything else to be disabled. IMO we can let the 
> user at least try - perhaps
> the hart doesn't have Z extensions enabled at that point. If 
> write_misa() tries to enable RVE,
> and only RVE, we proceed with the validation. This would be our first 
> check: RVE being enabled,
> and enabled by itself.  If a RVE write has any other bits enabled, 
> drop the write.
>
> - RVG
>
> All things considered, it's not that much of a big deal to support it. 
> Enabling RVG, assuming it
> wasn't enabled already, would cause us to mass enable 
> IMAFD_Zicsr_Zifencei. The only problem here
> is with F, which is mutually exclusive with Zfinx. If Zfinx is enabled 
> we can't enable F, thus
> we can't enable RVG, and we would simply drop the write. Enabling RVG 
> would also be a standalone
> action in write_misa().
>
> Disabling RVG has no side effects and it's not a special case.
>
> - RVV
>
> Enabling RVV requires enabling D, F, ext_zve64d, ext_zve64f and 
> ext_zve32f. The same F constraint
> (Zfinx) applies here as well.  Assuming we can enable F, we can enable 
> all these extensions,
> commit the RVV bit change and finish the write.
>
> Disabling RVV has no side-effects, at least as far as I can tell, 
> because all these other extensions
> can exist without RVV, so it's not a special case.
Vector instructions will never be really disabled in this way, only 
misa.V bit is cleared, since

zve64d/f  will be implicitly enabled when RVV is enabled, they will 
continue to work even if misa.V is cleared.

And we can never disabled F/D when V is firstly enabled, even if we 
disable them together with V.

> These are all the special cases that I can think of. RVE, then RVG, 
> and then RVV. If any of these
> bits are enabled we should just handle them standalone and finish the 
> write. I don't think we
> need to go through the regular validation workflow for them.
>
>
> The remaining cases would go through regular validation. We have 
> certain bits that would
> deactivate Z extensions if disabled:
>
> - RVA: would disable Zawrs
> - RVD: would disable zve64d
> - RVF: would disable Zfh/Zfhmin, zve64f, zve32f, zve64d
>
> We can allow these bits to be disabled, as long as there's no Z 
> extension being disabled
> in the process. If an enabled Z extension is impacted, we drop the 
> misa write.
> Finally, we have  I, M, A, F and D and their relation with RVG. RVG 
> would be disabled if any
> of these bits are disabled (and validation succeeds).
>
>
> That's all the caveats that I can think of. The code that enables a 
> certain MISA bit can be
> shared with the existing code that riscv_cpu_realize() uses. Code that 
> disables MISA bits would
> be new code that only write_misa() would use.
>
>
> Let me know what you all think. I intend to go this direction in v3.
Yeah, I agree this is an acceptable way for write_misa. However, I think 
it's better if we can distinguish

the Z extensions  implicitly enabled by misa extension(which can be 
disabled/re-enabled by write_misa) with

explicitly enabled Z extensions(which cannot).

Regards,

Weiwei Li

> Thanks,
>
>
> Daniel
>
>
> > Regards,
> >
> > Weiwei Li
> >
> > >       if (!(val & RVF)) {
> > >           env->mstatus &= ~MSTATUS_FS;
> > >       }