Re: [PATCH v3 02/10] target/riscv: always allow write_misa() to write MISA

[Andrew Jones]

On Thu, Feb 16, 2023 at 05:33:55PM +0800, Bin Meng wrote:
> On Thu, Feb 16, 2023 at 5:29 PM Andrew Jones <ajones@ventanamicro.com> wrote:
> >
> > On Wed, Feb 15, 2023 at 03:57:18PM -0300, 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.
> > >
> > > Regardless, the spec says that MISA is a WARL read-write CSR, and gating
> > > the writes in the register doesn't make sense. OS and applications
> > > should be wary of the consequences when writing it, but the write itself
> > > must always be allowed.
> >
> > The write is already allowed, i.e. no exception is raised when writing it.
> > The spec only says that the fields may/can be writable. So we can
> > correctly implement the spec with just
> >
> >  write_misa()
> >  {
> >    return RISCV_EXCP_NONE;
> >  }
> 
> Agree. Such change is still spec compliant without worrying about the bugs.
> 
> >
> > as it has effectively been implemented to this point.
> >
> > Based on Weiwei Li's pointing out of known bugs, and the fact that
> > this function has likely never been tested, then maybe we should just
> > implement it as above for now. Once a better solution to extension
> > sanity checking exists and a use (or at least test) case arises, then
> > the function could be expanded with some actually writable bits. (Also,
> > I think that when/if we do the expansion, then something like the misa_w
> > config proposed in the previous version of this series may still be
> > needed in order to allow opting-in/out of the behavior change.)
> 
> In QEMU RISC-V we have some examples of implementing optional spec
> features without exposing a config parameter. Do we need to add config
> parameters for those cases too? If yes, I am afraid the parameters
> will grow a lot.
>

I agree, particularly for RISC-V, the options grow quickly. How about this
for a policy?

1) When adding an optional, on-by-default CPU feature, which applies to
   all currently existing CPU types, then just add the feature without a
   config.

2) When, later, a CPU type or use case needs to disable an optional
   CPU feature, which doesn't have a config, then the config is added
   at that time.

While that policy seems reasonable (to me), I have a feeling the "applies
to all currently existing CPU types" requirement won't be easily
satisfied, so we'll end up mostly always adding configs anyway.

We can always change RISCVCPUConfig.ext_* to a bitmap if we feel like the
CPU is getting too bloated.

Thanks,
drew