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

[Alistair Francis]

On Tue, Jul 18, 2023 at 7:50 PM Daniel Henrique Barboza
<dbarboza@ventanamicro.com> wrote:
>
>
>
> 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.

That's true. The priv spec check is also there because we needed newer
versions of the priv spec for some extensions, and if we are going to
check some it's simpler to just check all of 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

Agreed

> entirely. Here's my plan to do it:

I think that'll work

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

I'm not sure about that though. As we see more CPUs being released and
then future spec changes I feel like differentiating between priv
specs is a useful feature.

Alistair

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