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

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

[Daniel Henrique Barboza]

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 */
+    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;
-- 
2.41.0

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

[Conor Dooley]

[-- Attachment #1: Type: text/plain, Size: 1961 bytes --]

On Mon, Jul 17, 2023 at 12:41:41PM -0300, 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>

Does the job, thanks for doing this.
Tested-by: Conor Dooley <conor.dooley@microchip.com>

Cheers,
Conor.

[-- Attachment #2: signature.asc --]
[-- Type: application/pgp-signature, Size: 228 bytes --]

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

[Weiwei Li]

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

Reviewed-by: Weiwei Li <liweiwei@iscas.ac.cn>

Weiwei Li

>   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 */
> +    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;

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

[LIU Zhiwei]

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

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;

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;

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

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

[Alistair Francis]

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

Reviewed-by: Alistair Francis <alistair.francis@wdc.com>

Alistair

> ---
>  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 */
> +    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;
> --
> 2.41.0
>
>

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

[Alistair Francis]

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

Thanks!

Applied to riscv-to-apply.next

Alistair

> ---
>  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 */
> +    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;
> --
> 2.41.0
>
>