[Date Prev][Date Next][Thread Prev][Thread Next][Date Index][Thread Index]
Re: [PATCH v3 02/10] target/riscv: always allow write_misa() to write MI
From: |
Andrew Jones |
Subject: |
Re: [PATCH v3 02/10] target/riscv: always allow write_misa() to write MISA |
Date: |
Thu, 16 Feb 2023 11:07:58 +0100 |
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
[PATCH v3 04/10] target/riscv: remove RISCV_FEATURE_DEBUG, Daniel Henrique Barboza, 2023/02/15
[PATCH v3 05/10] target/riscv/cpu.c: error out if EPMP is enabled without PMP, Daniel Henrique Barboza, 2023/02/15
[PATCH v3 03/10] target/riscv: introduce riscv_cpu_cfg(), Daniel Henrique Barboza, 2023/02/15
[PATCH v3 07/10] target/riscv: remove RISCV_FEATURE_PMP, Daniel Henrique Barboza, 2023/02/15
[PATCH v3 08/10] hw/riscv/virt.c: do not use RISCV_FEATURE_MMU in create_fdt_socket_cpus(), Daniel Henrique Barboza, 2023/02/15
[PATCH v3 06/10] target/riscv: remove RISCV_FEATURE_EPMP, Daniel Henrique Barboza, 2023/02/15
[PATCH v3 09/10] target/riscv: remove RISCV_FEATURE_MMU, Daniel Henrique Barboza, 2023/02/15
[PATCH v3 10/10] target/riscv/cpu: remove CPUArchState::features and friends, Daniel Henrique Barboza, 2023/02/15