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 <co...@kernel.org> Fixes: 2288a5ce43e5 ("target/riscv: add cfg properties for Zc* extension") Signed-off-by: Daniel Henrique Barboza <dbarb...@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;
