Re: [PATCH v6 1/9] target/riscv: turn write_misa() into an official no-op

[Daniel Henrique Barboza]

Hey,

On 2/16/23 22:42, LIU Zhiwei wrote:
> On 2023/2/17 5:55, Daniel Henrique Barboza wrote:
> > At this moment, and apparently since ever, we have no way of enabling
> > RISCV_FEATURE_MISA. This means that all the code from write_misa(), all
> > the nuts and bolts that handles how to write this CSR, has always been a
> > no-op as well because write_misa() will always exit earlier.
> >
> > This seems to be benign in the majority of cases. Booting an Ubuntu
> > 'virt' guest and logging all the calls to 'write_misa' shows that no
> > writes to MISA CSR was attempted. Writing MISA, i.e. enabling/disabling
> > RISC-V extensions after the machine is powered on, seems to be a niche
> > use.
> >
> > Before proceeding, let's recap what the spec says about MISA. It is a
> > CSR that is divided in 3 fields:
> >
> > - MXL, Machine XLEN, described as "may be writable";
> >
> > - MXLEN, the XLEN in M-mode, which is given by the setting of MXL or a
> > fixed value if MISA is zero;
> >
> > - Extensions is defined as "a WARL field that can contain writable bits
> > where the implementation allows the supported ISA to be modified"
> >
> > Thus what we have today (write_misa() being a no-op) is already a valid
> > spec implementation. We're not obliged to have a particular set of MISA
> > writable bits, and at this moment we have none.
>
> Hi Daniel,
>
> I see there has been a discussion on this topic. And as no-op has no 
> harmfulness for current implementation.
> However, I still think we should make misa writable as default, which is also a 
> valid spec implementation.
>
> One reason is that may be we need to dynamic write  access for some cpus in the 
> future. The other is we should
> make QEMU a more useful implementation, not just a legal implementation. We 
> have done in many aspects on this direction.
>
> I prefer your implementation before v4. It's not a complicated implementation. 
> And I think the other extensions on QEMU currently
> can mostly be configurable already.

I don't have a strong opinion in this matter to be honest. My problems with the
existing code are:

- the code is untested. I cannot say that this was never tested, but I can say 
that
this has been mostly untested ever since introduced. Which is normal for a code 
that
is 'dormant'.

- the code is dormant and most likely with bugs, but it's still maintained. For
example we have e91a7227 ("target/riscv: Split misa.mxl and misa.ext") that had
to make changes here. So we have the upkeep but no benefits.

- we don't have an use case for it. Most OSes doesn't seem to care, and afaik no
applications seems to care either.


All this said, I think we can reach a consensus of keeping it if we can at 
least come
up with a way of testing it.


> Your work is a good step towards to unify the configuration and the check.  I 
> think two more steps we can go further.
>
> 1) Remove RVI/RVF and the similar macros, and add fields for them in the 
> configuration struct.
>
> 2) Unify the check about configuration. write_misa and cpu_realize_fn can use 
> the same check function.
>
>
> As we have done these two steps, I think we can go more closely for the profile 
> extension.


Is this the extension you're taking about?

https://github.com/riscv/riscv-profiles/blob/main/profiles.adoc


This looks like a good reason to keep the code. Let's see if anyone else has an 
opinion
about it. We can do the improvements you mentioned above as a follow-up (this 
series was
really about removing RISC_FEATURE_*) if we decide to keep it.



Thanks,


Daniel

> Zhiwei
>
> > Given that allowing the dormant code to write MISA can cause tricky bugs
> > to solve later on, and we don't have a particularly interesting case of
> > writing MISA to support today, and we're already not violating the
> > specification, let's erase all the body of write_misa() and turn it into
> > an official no-op instead of an accidental one. We'll keep consistent
> > with what we provide users today but with 50+ less lines to maintain.
> >
> > RISCV_FEATURE_MISA enum is erased in the process since there's no one
> > else using it.
> >
> > Signed-off-by: Daniel Henrique Barboza <dbarboza@ventanamicro.com>
> > Reviewed-by: Bin Meng <bmeng@tinylab.org>
> > Reviewed-by: Andrew Jones <ajones@ventanamicro.com>
> > ---
> >   target/riscv/cpu.h |  1 -
> >   target/riscv/csr.c | 55 ----------------------------------------------
> >   2 files changed, 56 deletions(-)
> >
> > diff --git a/target/riscv/cpu.h b/target/riscv/cpu.h
> > index 7128438d8e..01803a020d 100644
> > --- a/target/riscv/cpu.h
> > +++ b/target/riscv/cpu.h
> > @@ -89,7 +89,6 @@ enum {
> >       RISCV_FEATURE_MMU,
> >       RISCV_FEATURE_PMP,
> >       RISCV_FEATURE_EPMP,
> > -    RISCV_FEATURE_MISA,
> >       RISCV_FEATURE_DEBUG
> >   };
> > diff --git a/target/riscv/csr.c b/target/riscv/csr.c
> > index 1b0a0c1693..f7862ff4a4 100644
> > --- a/target/riscv/csr.c
> > +++ b/target/riscv/csr.c
> > @@ -1329,61 +1329,6 @@ static RISCVException read_misa(CPURISCVState *env, int 
> > csrno,
> >   static RISCVException write_misa(CPURISCVState *env, int csrno,
> >                                    target_ulong val)
> >   {
> > -    if (!riscv_feature(env, RISCV_FEATURE_MISA)) {
> > -        /* 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;
> > -
> > -    /* Mask extensions that are not supported by QEMU */
> > -    val &= (RVI | RVE | RVM | RVA | RVF | RVD | RVC | RVS | RVU | RVV);
> > -
> > -    /* '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
> > -     */
> > -    if ((val & RVC) && (GETPC() & ~3) != 0) {
> > -        val &= ~RVC;
> > -    }
> > -
> > -    /* If nothing changed, do nothing. */
> > -    if (val == env->misa_ext) {
> > -        return RISCV_EXCP_NONE;
> > -    }
> > -
> > -    if (!(val & 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;
> >   }