Re: [PATCH v7 03/10] target/riscv: allow MISA writes as experimental

[Daniel Henrique Barboza]

On 3/9/23 03:11, LIU Zhiwei wrote:
> On 2023/2/23 2:51, 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 properly 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.
> >
> > After discussions in the mailing list, most notably in [1], we reached
> > the consensus that this code is not suited to be exposed to users
> > because it's not well tested, but at the same time removing it is a bit
> > extreme because we would like to fix it, and it's easier to do so with
> > the code available to use instead of fetching it from git log.
> >
> > The approach taken here is to get rid of RISCV_FEATURE_MISA altogether
> > and use a new experimental flag called x-misa-w. The default value is
> > false, meaning that we're keeping the existing behavior of doing nothing
> > if a write_misa() is attempted. As with any existing experimental flag,
> > x-misa-w is also a temporary flag that we need to remove once we fix
> > write_misa().
> >
> > [1] https://lists.gnu.org/archive/html/qemu-devel/2023-02/msg05092.html
> >
> > Signed-off-by: Daniel Henrique Barboza <dbarboza@ventanamicro.com>
> > ---
> >   target/riscv/cpu.c | 6 ++++++
> >   target/riscv/cpu.h | 2 +-
> >   target/riscv/csr.c | 2 +-
> >   3 files changed, 8 insertions(+), 2 deletions(-)
> >
> > diff --git a/target/riscv/cpu.c b/target/riscv/cpu.c
> > index 93b52b826c..1d637b1acd 100644
> > --- a/target/riscv/cpu.c
> > +++ b/target/riscv/cpu.c
> > @@ -1210,6 +1210,12 @@ static Property riscv_cpu_properties[] = {
> >       DEFINE_PROP_BOOL("rvv_ta_all_1s", RISCVCPU, cfg.rvv_ta_all_1s, false),
> >       DEFINE_PROP_BOOL("rvv_ma_all_1s", RISCVCPU, cfg.rvv_ma_all_1s, false),
> > +
> > +    /*
> > +     * write_misa() is marked as experimental for now so mark
> > +     * it with -x and default to 'false'.
> > +     */
> > +    DEFINE_PROP_BOOL("x-misa-w", RISCVCPU, cfg.misa_w, false),
>
> Just a question  I got when I reviewed another patch set:
>
> Why put x-misa-w to riscv_cpu_properties instead of riscv_cpu_extensions?

I didn't think too much about it to be honest. I copied what was being done
with the now old RISCV_FEATURE_DEBUG. It made sense to me because x-misa-w
isn't a RISC-V extension but a property of our model.

I don't oppose moving it to a better place. Just be aware that this a property
that we're planning to eventually remove.


Thanks,


Daniel


> I think this property should have a fixed value for a named cpu(such as 
> sifive_e). Therefore IMHO, putting x-misa-w into riscv_cpu_extensions is more 
> proper. At the same time,  the debug, rvv_ta_all_1s, rvv_ma_all_1s, resetvec 
> properties should be moved to riscv_cpu_extentions. I am not sure why we should 
> give the marchid/mipid/mvendorid option for named cpus. Maybe @Frank knows.
>
> Have I missed something?
>
> Zhiwei
>
> >       DEFINE_PROP_END_OF_LIST(),
> >   };
> > diff --git a/target/riscv/cpu.h b/target/riscv/cpu.h
> > index 215423499e..9d3304bcda 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
> >   };
> > @@ -498,6 +497,7 @@ struct RISCVCPUConfig {
> >       bool pmp;
> >       bool epmp;
> >       bool debug;
> > +    bool misa_w;
> >       bool short_isa_string;
> >   };
> > diff --git a/target/riscv/csr.c b/target/riscv/csr.c
> > index e149b453da..3cb8d2ffad 100644
> > --- a/target/riscv/csr.c
> > +++ b/target/riscv/csr.c
> > @@ -1329,7 +1329,7 @@ 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)) {
> > +    if (!riscv_cpu_cfg(env)->misa_w) {
> >           /* drop write to misa */
> >           return RISCV_EXCP_NONE;
> >       }