Re: [PATCH] target/riscv/cpu.c: check priv_ver before auto-enable zca/zcd/zcf

[Daniel Henrique Barboza]

On 7/17/23 22:36, LIU Zhiwei wrote:
> On 2023/7/17 23:41, Daniel Henrique Barboza wrote:
> > Commit bd30559568 made changes in how we're checking and disabling
> > extensions based on env->priv_ver. One of the changes was to move the
> > extension disablement code to the end of realize(), being able to
> > disable extensions after we've auto-enabled some of them.
> >
> > An unfortunate side effect of this change started to happen with CPUs
> > that has an older priv version, like sifive-u54. Starting on commit
> > 2288a5ce43e5 we're auto-enabling zca, zcd and zcf if RVC is enabled,
> > but these extensions are priv version 1.12.0. When running a cpu that
> > has an older priv ver (like sifive-u54) the user is spammed with
> > warnings like these:
> >
> > qemu-system-riscv64: warning: disabling zca extension for hart 
> > 0x0000000000000000 because privilege spec version does not match
> > qemu-system-riscv64: warning: disabling zcd extension for hart 
> > 0x0000000000000000 because privilege spec version does not match
> >
> > The warnings are part of the code that disables the extension, but in this
> > case we're throwing user warnings for stuff that we enabled on our own,
> > without user intervention. Users are left wondering what they did wrong.
> >
> > A quick 8.1 fix for this nuisance is to check the CPU priv spec before
> > auto-enabling zca/zcd/zcf. A more appropriate fix will include a more
> > robust framework that will account for both priv_ver and user choice
> > when auto-enabling/disabling extensions, but for 8.1 we'll make it do
> > with this simple check.
> >
> > It's also worth noticing that this is the only case where we're
> > auto-enabling extensions based on a criteria (in this case RVC) that
> > doesn't match the priv spec of the extensions we're enabling. There's no
> > need for more 8.1 band-aids.
> >
> > Cc: Conor Dooley <conor@kernel.org>
> > Fixes: 2288a5ce43e5 ("target/riscv: add cfg properties for Zc* extension")
> > Signed-off-by: Daniel Henrique Barboza <dbarboza@ventanamicro.com>
> > ---
> >   target/riscv/cpu.c | 3 ++-
> >   1 file changed, 2 insertions(+), 1 deletion(-)
> >
> > diff --git a/target/riscv/cpu.c b/target/riscv/cpu.c
> > index 9339c0241d..6b93b04453 100644
> > --- a/target/riscv/cpu.c
> > +++ b/target/riscv/cpu.c
> > @@ -1225,7 +1225,8 @@ void riscv_cpu_validate_set_extensions(RISCVCPU *cpu, 
> > Error **errp)
> >           }
> >       }
> > -    if (riscv_has_ext(env, RVC)) {
> > +    /* zca, zcd and zcf has a PRIV 1.12.0 restriction */
>
> I think the Zca/zcd/zcf doesn't have much relationship with the privilege 
> specification. The privilege specification doesn't define any
> CSR or rules that Zca/zcd/zcf depend on. Maybe I missed something.  Does anyone 
>  know why we should check PRIV_VERSION_1_12_0 for zca/zcf/zcd?

I always thought about this priv spec filter as a way to determine the time
window that the extension was ratified/defined. In this example it's been used
to filter out zca/zcd/zcf from the sifive-u54 chip because this chip is older
than those extensions, so it doesn't make sense to enable them.

> I think we should remove the check for priv_ver for many user mode extensions. 
> We should set the checking privilege specification version for these extensions 
> to PRIV_VERSION_1_10_0.

I think it's hard to pick and choose which extensions will have a priv version 
check
or not. If we're bothered with the priv spec check per se then we should remove 
it
entirely. Here's my plan to do it:

- remove cfg.priv_ver. This is a very old attribute that allow users to set the 
priv_ver
for generic CPUs like rv64. I'm doing changes in the user options for TCG flags 
and the
very existence of this option forces me to make priv checks for all extensions 
we're
auto-enabling during realize() (because I can't be sure whether the user 
changed the
priv_ver of rv64 to something older);

- split the realize() functions between generic and vendor CPUs again. It was 
merged together
earlier this year (I did it) because, back then, we were doing too much stuff 
during
realize() that was needed for named CPUs, but the side effect is what we're 
seeing now:
the common code is enabling unwanted extensions for vendor CPUs. The code is 
very different
now, and I believe that we can at least skip validate_set_extensions() for 
vendor CPUs;

- at this point, vendor CPUs aren't auto-enabling any features and generic CPUs 
are always
set to PRIV_VER_LATEST. This means that we can remove all code related to 
disable extensions
via priv spec, and then all artifacts related to priv spec.


However, even if we're all onboard with removing it, this is still 8.2 work. 
For 8.1 I believe
this patch is a good fix to relief users from these warnings.


Thanks,

Daniel



> Zhiwei
>
> > +    if (riscv_has_ext(env, RVC) && env->priv_ver >= PRIV_VERSION_1_12_0) {
> >           cpu->cfg.ext_zca = true;
> >           if (riscv_has_ext(env, RVF) && env->misa_mxl_max == MXL_RV32) {
> >               cpu->cfg.ext_zcf = true;