Re: [PATCH v3 1/4] RISC-V: KVM: Add SBI extension validate callback

[sashiko-bot@kernel.org]

Thank you for your contribution! Sashiko AI review found 2 potential issue(s) to consider:
- [High] The validate callback is skipped for disabled extensions, defeating its stated purpose of cleaning up state.
- [High] The FWFT extension completely lacks the implementation of the new validate callback.
--

--- Patch [1]: [PATCH v3 1/4] RISC-V: KVM: Add SBI extension validate callback ---
commit 3b3c6bd10bd24fef09cd0cfc1f43717873d6c992
Author: Inochi Amaoto <inochiama@gmail.com>

RISC-V: KVM: Add SBI extension validate callback

This commit adds a generic SBI extension validate callback to allow extensions
to correct their parameters before the first run. It is specifically intended
to fix an issue where vCPU state remains broken if the FWFT extension is
disabled after its features have been configured.

> Add the generic SBI extension validate callback so the FWFT
> extension can fix its parameters before the first run.

[Severity: High]
Is the actual implementation of the validate callback for the FWFT extension 
missing from this patch? 

The generic framework is added here, but the validate function pointer 
assignment for vcpu_sbi_ext_fwft in arch/riscv/kvm/vcpu_sbi_fwft.c appears 
to be absent. 

Without implementing this callback in the FWFT extension itself, the 
parameters will not actually be fixed when the extension is disabled.

> diff --git a/arch/riscv/kvm/vcpu_sbi.c b/arch/riscv/kvm/vcpu_sbi.c
> index 46ab7b989432b..b737e9a7a12a2 100644
> --- a/arch/riscv/kvm/vcpu_sbi.c
> +++ b/arch/riscv/kvm/vcpu_sbi.c

[ ... ]

> @@ -723,3 +723,26 @@ void kvm_riscv_vcpu_sbi_reset(struct kvm_vcpu *vcpu)
>  		ext->reset(vcpu);
>  	}
>  }
> +
> +void kvm_riscv_vcpu_sbi_validate(struct kvm_vcpu *vcpu)
> +{
> +	struct kvm_vcpu_sbi_context *scontext = &vcpu->arch.sbi_context;
> +	const struct kvm_riscv_sbi_extension_entry *entry;
> +	const struct kvm_vcpu_sbi_extension *ext;
> +	int idx, i;
> +
> +	for (i = 0; i < ARRAY_SIZE(sbi_ext); i++) {
> +		entry = &sbi_ext[i];
> +		ext = entry->ext_ptr;
> +		idx = entry->ext_idx;
> +
> +		if (idx < 0 || idx >= ARRAY_SIZE(scontext->ext_status))
> +			continue;
> +
> +		if (scontext->ext_status[idx] != KVM_RISCV_SBI_EXT_STATUS_ENABLED ||
> +		    !ext->validate)
> +			continue;

[Severity: High]
Will this condition prevent disabled extensions from cleaning up their state?

The commit message notes this callback is needed to fix the vCPU state when a 
user disables the FWFT extension after setting a value. 

However, kvm_riscv_vcpu_sbi_validate() explicitly skips extensions unless 
their status is KVM_RISCV_SBI_EXT_STATUS_ENABLED. 

Does this mean a disabled extension will not have its validate callback 
invoked at all, leaving the vCPU in an inconsistent state?

> +
> +		ext->validate(vcpu);
> +	}
> +}

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