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

[Bin Meng <bmeng.cn@gmail.com>]

On Thu, Feb 16, 2023 at 6:08 PM Andrew Jones <ajones@ventanamicro.com> wrote:
>
> 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.

This policy sounds good to me.

> 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.

Probably this does not apply to machines that have fixed configuration
RISC-V processor. But let's see what happens.

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

Regards,
Bin