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

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.

Remove the RISCV_FEATURE_MISA verification at the start of write_misa(),
removing RISCV_FEATURE_MISA altogether since there will be no more
callers of this enum.

Signed-off-by: Daniel Henrique Barboza <dbarboza@ventanamicro.com>
---
 target/riscv/cpu.h | 1 -
 target/riscv/csr.c | 5 -----
 2 files changed, 6 deletions(-)

On Thu, Feb 16, 2023 at 2:58 AM Daniel Henrique Barboza
<dbarboza@ventanamicro.com> 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.
>
> Remove the RISCV_FEATURE_MISA verification at the start of write_misa(),
> removing RISCV_FEATURE_MISA altogether since there will be no more
> callers of this enum.
>
> Signed-off-by: Daniel Henrique Barboza <dbarboza@ventanamicro.com>
> ---
>  target/riscv/cpu.h | 1 -
>  target/riscv/csr.c | 5 -----
>  2 files changed, 6 deletions(-)
>

Reviewed-by: Bin Meng <bmeng@tinylab.org>

On 2023/2/16 02:57, 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.
>
> Remove the RISCV_FEATURE_MISA verification at the start of write_misa(),
> removing RISCV_FEATURE_MISA altogether since there will be no more
> callers of this enum.
>
> Signed-off-by: Daniel Henrique Barboza <dbarboza@ventanamicro.com>
> ---
>   target/riscv/cpu.h | 1 -
>   target/riscv/csr.c | 5 -----
>   2 files changed, 6 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 e149b453da..5bd4cdbef5 100644
> --- a/target/riscv/csr.c
> +++ b/target/riscv/csr.c
> @@ -1329,11 +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 have a question here:

If we directly remove this here without other limits, the bugs 
introduced by following code

will be exposed to users, such as:

- V may be still enabled when misa.D is cleared.

- Zfh/Zfhmin may be still enabled when misa.D is cleared

- Misa.U may be cleared when Misa.S is still set.

...

Should we fix these bugs before this patch? Or fix them in following 
patchset?

If we choose the latter, I think it's better to add a limitation(such 
as  writable_mask) to the changable fields of misa here.

Regards,

Weiwei Li

>       /* 'I' or 'E' must be present */
>       if (!(val & (RVI | RVE))) {
>           /* It is not, drop write to misa */

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;
 }

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

Thanks,
drew

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.

Regards,
Bin

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

On 2/16/23 06:29, Andrew Jones 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;
>   }
> 
> as it has effectively been implemented to this point.

I'm fine with that. We can always use git history to see the removed code and
reintroduce it back.


Thanks,


Daniel


> 
> 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.)
> 
> Thanks,
> drew

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