Re: [PATCH v2 1/7] RISC-V: KVM: Add support for Svadu FWFT features

[sashiko-bot@kernel.org]

Thank you for your contribution! Sashiko AI review found 5 potential issue(s) to consider:
- [High] KVM_SET_ONE_REG for FWFT state registers modifies VCPU configuration in memory but fails to set `vcpu->arch.csr_dirty = true`, leading to stale hardware CSRs.
- [High] The FWFT support check `kvm_sbi_fwft_pte_ad_hw_updating_supported()` incorrectly disables the toggle when both SVADU and SVADE are present, and dangerously enables it when only SVADU is present.
- [High] The 'flag' parameter in henvcfg helper functions is 'unsigned long', causing truncation of 64-bit flags like ENVCFG_ADUE on 32-bit systems and permanently corrupting guest `henvcfg` state during vCPU reset.
- [High] Dynamically setting the Svadu FWFT feature on RV32 fails to update the physical CPU's `CSR_HENVCFGH` register, leaving the hardware state stale.
- [High] Direct modification of `CSR_HENVCFG` bypasses Nested Acceleration (NACL) shared memory, leading to state desynchronization in nested virtualization.
--

commit df2a6571e0257cf7e8c4ca0758aefd4d6f6c3a32
Author: Inochi Amaoto <inochiama@gmail.com>

RISC-V: KVM: Add support for Svadu FWFT features

This patch adds support for exposing the Svadu extension to guests via the
SBI FWFT interface. It introduces helpers to get, set, and reset henvcfg
flags, specifically for the ADUE bit which controls hardware updating of
PTE A/D bits.

> diff --git a/arch/riscv/kvm/vcpu_sbi_fwft.c b/arch/riscv/kvm/vcpu_sbi_fwft.c
> index 2eab15339694f..375269e43c13b 100644
> --- a/arch/riscv/kvm/vcpu_sbi_fwft.c
> +++ b/arch/riscv/kvm/vcpu_sbi_fwft.c

[ ... ]

> @@ -84,6 +84,42 @@ static bool kvm_fwft_is_defined_feature(enum sbi_fwft_feature_t feature)
>  	return false;
>  }
>  
> +static void kvm_sbi_fwft_env_flag_reset_helper(struct kvm_vcpu *vcpu,
> +					       unsigned long flag)
> +{
> +	vcpu->arch.cfg.henvcfg &= ~flag;
> +}

[Severity: High]
Does using unsigned long for the flag parameter truncate 64-bit flags
like ENVCFG_ADUE on 32-bit systems?

Since ENVCFG_ADUE is in the upper 32 bits, passing it as unsigned long on
CONFIG_32BIT systems would silently truncate it to 0. 

When vcpu->arch.cfg.henvcfg &= ~flag evaluates with flag as 0, it results
in henvcfg &= ~0UL. Because ~0UL is 0xFFFFFFFF, it zero-extends to u64,
resulting in henvcfg &= 0x00000000FFFFFFFFULL.

Will this unconditionally wipe out all upper 32-bit extensions like
ENVCFG_STCE and ENVCFG_PBMTE for the guest during vCPU reset?

> +
> +static long kvm_sbi_fwft_env_flag_set_helper(struct kvm_vcpu *vcpu,
> +					     struct kvm_sbi_fwft_config *conf,
> +					     bool one_reg_access,
> +					     unsigned long value, unsigned long flag)
> +{

[Severity: High]
Is this helper also affected by the same unsigned long truncation issue
for the flag parameter?

> +	struct kvm_vcpu_config *cfg = &vcpu->arch.cfg;
> +
> +	if (value == 0)
> +		cfg->henvcfg &= ~flag;
> +	else if (value == 1)
> +		cfg->henvcfg |= flag;
> +	else
> +		return SBI_ERR_INVALID_PARAM;
> +

[Severity: High]
When one_reg_access is true, does this omit setting vcpu->arch.csr_dirty
to true?

If the VCPU is later scheduled on the same physical CPU, vcpu_load() might
skip calling kvm_riscv_vcpu_config_load() due to the fast-path condition
checking the csr_dirty flag.

Could this cause the guest to run with a stale hardware configuration since
the updated henvcfg is never flushed to the hardware CSR_HENVCFG?

> +	if (!one_reg_access)
> +		csr_write(CSR_HENVCFG, vcpu->arch.cfg.henvcfg);

[Severity: High]
On 32-bit RISC-V systems, does csr_write(CSR_HENVCFG, ...) only write the
lower 32 bits?

Since ENVCFG_ADUE resides in the upper 32 bits, shouldn't there be an
explicit write to CSR_HENVCFGH as well? Without it, the physical CPU's
configuration might not be properly synchronized for upper 32-bit flags.

[Severity: High]
Does directly modifying CSR_HENVCFG bypass Nested Acceleration (NACL)?

When KVM runs as an L1 hypervisor with NACL enabled, it must synchronize
hypervisor CSRs via the shared memory region (nsh) rather than directly
writing to the physical CSRs.

If ncsr_write() is not used here, could the L0 hypervisor miss the update
and overwrite the CSR with a stale value from nsh on subsequent VM entries?

> +
> +	return SBI_SUCCESS;
> +}

[ ... ]

> +static bool kvm_sbi_fwft_pte_ad_hw_updating_supported(struct kvm_vcpu *vcpu)
> +{
> +	return riscv_isa_extension_available(vcpu->arch.isa, SVADU) &&
> +		!riscv_isa_extension_available(vcpu->arch.isa, SVADE);
> +}

[Severity: High]
Does this condition behave backward with respect to the SBI FWFT specification?

The specification requires that if a VM supports only Svadu, ADUE must be
hardwired to 1. If it supports both Svadu and Svade, the guest can toggle
the feature.

By returning true only when Svadu is present and Svade is absent, does this
disable the toggle for fully capable VMs, and incorrectly allow Svadu-only VMs
to set ADUE=0? 

If a Svadu-only VM sets ADUE=0, could it silently corrupt its memory since the
Svade software fallback is unavailable?

-- 
Sashiko AI review · https://sashiko.dev/#/patchset/20260601084336.2601675-1-inochiama@gmail.com?part=1