Re: [PATCH v6 4/8] RISC-V: KVM: Add ssp context save/restore

[sashiko-bot@kernel.org]

Thank you for your contribution! Sashiko AI review found 2 potential issue(s) to consider:
- [High] KVM exposes the Zicfiss extension (shadow stack) to the guest but fails to enable it in the hypervisor environment configuration, making it unusable.
- [Medium] The patch adds a new register to the ONE_REG UAPI but fails to update the KVM RISC-V `get-reg-list` selftest.
--

--- Patch [4]: [PATCH v6 4/8] RISC-V: KVM: Add ssp context save/restore ---
commit 1dccefe0c0e59cdb4b3a936e440649fc566ba10e
Author: Inochi Amaoto <inochiama@gmail.com>

RISC-V: KVM: Add ssp context save/restore
    
Add ssp context save/restore for guest VCPUs and also add it to the
ONE_REG interface to allow its access from user space.

> diff --git a/arch/riscv/include/uapi/asm/kvm.h b/arch/riscv/include/uapi/asm/kvm.h
> index a27de850fa4cc..fd4c816976177 100644
> --- a/arch/riscv/include/uapi/asm/kvm.h
> +++ b/arch/riscv/include/uapi/asm/kvm.h
[ ... ]
> @@ -266,12 +271,15 @@ struct kvm_riscv_sbi_fwft {
>  #define KVM_REG_RISCV_CSR_GENERAL	(0x0 << KVM_REG_RISCV_SUBTYPE_SHIFT)
>  #define KVM_REG_RISCV_CSR_AIA		(0x1 << KVM_REG_RISCV_SUBTYPE_SHIFT)
>  #define KVM_REG_RISCV_CSR_SMSTATEEN	(0x2 << KVM_REG_RISCV_SUBTYPE_SHIFT)
> +#define KVM_REG_RISCV_CSR_ZICFISS	(0x3 << KVM_REG_RISCV_SUBTYPE_SHIFT)

[Severity: Medium]
Will this new CSR subtype cause the KVM get-reg-list selftest to fail?

The get-reg-list test uses a strict list of expected registers. It appears
the new KVM_REG_RISCV_CSR_ZICFISS register subtype is missing from the CSR
identification logic in tools/testing/selftests/kvm/riscv/get-reg-list.c:csr_id_to_str(),
and the new ISA extensions (KVM_RISCV_ISA_EXT_ZICFISS and KVM_RISCV_ISA_EXT_ZICFILP)
are missing from the filter_reg() whitelist.

Could this lead to an "Unknown csr reg" failure when the tests are run on a
Zicfiss-capable host?

>  #define KVM_REG_RISCV_CSR_REG(name)	\
>  		(offsetof(struct kvm_riscv_csr, name) / sizeof(unsigned long))
>  #define KVM_REG_RISCV_CSR_AIA_REG(name)	\
>  	(offsetof(struct kvm_riscv_aia_csr, name) / sizeof(unsigned long))
>  #define KVM_REG_RISCV_CSR_SMSTATEEN_REG(name)  \
>  	(offsetof(struct kvm_riscv_smstateen_csr, name) / sizeof(unsigned long))
> +#define KVM_REG_RISCV_CSR_ZICFISS_REG(name)  \
> +	(offsetof(struct kvm_riscv_zicfiss_csr, name) / sizeof(unsigned long))
>  
>  /* Timer registers are mapped as type 4 */
>  #define KVM_REG_RISCV_TIMER		(0x04 << KVM_REG_RISCV_TYPE_SHIFT)
> diff --git a/arch/riscv/kvm/vcpu.c b/arch/riscv/kvm/vcpu.c
> index cf6e231e76e25..acdb12fcdb69a 100644
> --- a/arch/riscv/kvm/vcpu.c
> +++ b/arch/riscv/kvm/vcpu.c
[ ... ]
> @@ -720,6 +721,7 @@ static void kvm_riscv_update_hvip(struct kvm_vcpu *vcpu)
>  
>  static __always_inline void kvm_riscv_vcpu_swap_in_guest_state(struct kvm_vcpu *vcpu)
>  {
> +	struct kvm_vcpu_zicfiss_csr *zicficsr = &vcpu->arch.zicfiss_csr;
>  	struct kvm_vcpu_smstateen_csr *smcsr = &vcpu->arch.smstateen_csr;
>  	struct kvm_vcpu_csr *csr = &vcpu->arch.guest_csr;
>  
> @@ -727,10 +729,13 @@ static __always_inline void kvm_riscv_vcpu_swap_in_guest_state(struct kvm_vcpu *
>  	vcpu->arch.host_senvcfg = csr_swap(CSR_SENVCFG, csr->senvcfg);
>  	if (riscv_has_extension_unlikely(RISCV_ISA_EXT_SMSTATEEN))
>  		vcpu->arch.host_sstateen0 = csr_swap(CSR_SSTATEEN0, smcsr->sstateen0);
> +	if (riscv_has_extension_unlikely(RISCV_ISA_EXT_ZICFISS))
> +		csr_write(CSR_SSP, zicficsr->ssp);

[Severity: High]
Does KVM need to enable the shadow stack feature in henvcfg to allow the guest
to use it?

It looks like the henvcfg register must be updated to include ENVCFG_SSE when
the Zicfiss ISA extension is available, similar to the ZICBOZ/ZICBOM
handling in arch/riscv/kvm/vcpu_config.c:kvm_riscv_vcpu_config_ran_once():

	if (riscv_isa_extension_available(isa, ZICBOZ))
		cfg->henvcfg |= ENVCFG_CBZE;

Without ENVCFG_SSE set in the hypervisor environment configuration, when a
guest attempts to use shadow stack instructions, won't the hardware inject an
illegal instruction exception and potentially crash the guest application or
kernel?

Additionally, it seems SBI_FWFT_SHADOW_STACK is missing from the features[]
array in arch/riscv/kvm/vcpu_sbi_fwft.c. Would this prevent the guest from
dynamically enabling it via the SBI FWFT interface, causing it to receive
SBI_ERR_NOT_SUPPORTED instead?

>  }
>  
>  static __always_inline void kvm_riscv_vcpu_swap_in_host_state(struct kvm_vcpu *vcpu)

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