[PATCH v3 0/4] RISC-V: KVM: Cleanup SBI extension status when disable

[PATCH v3 0/4] RISC-V: KVM: Cleanup SBI extension status when disable

[Inochi Amaoto]

KVM SBI FWFT extension has multiple internal state can be programmed
by the userspace. However, when the FWFT feature or the FWFT extension
is disabled, the internal state will be kept as it is, which may cause
some side effect for the vCPU, cleanup these state when a SBI extension
or FWFT feature is disabled.

Change from v2:
- https://lore.kernel.org/kvm/20260706021154.210289-1-inochiama@gmail.com
patch 1, 2:
1. Apply Anup's tag.
patch 3:
1. Remove the csr_dirty set.
patch 4:
1. Add a new patch that sets the csr_dirty for FWFT extension.

Change from v1:
- https://lore.kernel.org/kvm/20260701094731.1022163-1-inochiama@gmail.com
patch 2:
1. Check support callback existed before using it.

Inochi Amaoto (4):
  RISC-V: KVM: Add SBI extension validate callback
  RISC-V: KVM: Add SBI FWFT validation support
  RISC-V: KVM: Reset the SBI extension when disable it
  RISC-V: KVM: Mark the reset callback of FWFT extension always dirties
    CSR

 arch/riscv/include/asm/kvm_vcpu_sbi.h |  4 ++++
 arch/riscv/kvm/vcpu_config.c          |  2 ++
 arch/riscv/kvm/vcpu_sbi.c             | 30 +++++++++++++++++++++++++++
 arch/riscv/kvm/vcpu_sbi_fwft.c        | 27 ++++++++++++++++++++++++
 4 files changed, 63 insertions(+)

--
2.55.0

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

[Inochi Amaoto]

When user disable FWFT extension after setting any value of the
FWFT feature, the state of vCPU will be broken since the value
of disable FWFT feature is still functional.

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

Signed-off-by: Inochi Amaoto <inochiama@gmail.com>
Reviewed-by: Anup Patel <anup@brainfault.org>
---
 arch/riscv/include/asm/kvm_vcpu_sbi.h |  4 ++++
 arch/riscv/kvm/vcpu_config.c          |  2 ++
 arch/riscv/kvm/vcpu_sbi.c             | 23 +++++++++++++++++++++++
 3 files changed, 29 insertions(+)

diff --git a/arch/riscv/include/asm/kvm_vcpu_sbi.h b/arch/riscv/include/asm/kvm_vcpu_sbi.h
index c1a7e3b40d9c..f01a2860c751 100644
--- a/arch/riscv/include/asm/kvm_vcpu_sbi.h
+++ b/arch/riscv/include/asm/kvm_vcpu_sbi.h
@@ -60,6 +60,9 @@ struct kvm_vcpu_sbi_extension {
 
 	void (*reset)(struct kvm_vcpu *vcpu);
 
+	/* Allow the extension to correct its parameters before the first run */
+	void (*validate)(struct kvm_vcpu *vcpu);
+
 	unsigned long state_reg_subtype;
 	unsigned long (*get_state_reg_count)(struct kvm_vcpu *vcpu);
 	int (*get_state_reg_id)(struct kvm_vcpu *vcpu, int index, u64 *reg_id);
@@ -93,6 +96,7 @@ int kvm_riscv_vcpu_sbi_ecall(struct kvm_vcpu *vcpu, struct kvm_run *run);
 void kvm_riscv_vcpu_sbi_init(struct kvm_vcpu *vcpu);
 void kvm_riscv_vcpu_sbi_deinit(struct kvm_vcpu *vcpu);
 void kvm_riscv_vcpu_sbi_reset(struct kvm_vcpu *vcpu);
+void kvm_riscv_vcpu_sbi_validate(struct kvm_vcpu *vcpu);
 
 #ifdef CONFIG_RISCV_SBI_V01
 extern const struct kvm_vcpu_sbi_extension vcpu_sbi_ext_v01;
diff --git a/arch/riscv/kvm/vcpu_config.c b/arch/riscv/kvm/vcpu_config.c
index 238418fed2b9..b68aa830aaf5 100644
--- a/arch/riscv/kvm/vcpu_config.c
+++ b/arch/riscv/kvm/vcpu_config.c
@@ -69,6 +69,8 @@ void kvm_riscv_vcpu_config_ran_once(struct kvm_vcpu *vcpu)
 
 	if (vcpu->guest_debug)
 		cfg->hedeleg &= ~BIT(EXC_BREAKPOINT);
+
+	kvm_riscv_vcpu_sbi_validate(vcpu);
 }
 
 void kvm_riscv_vcpu_config_load(struct kvm_vcpu *vcpu)
diff --git a/arch/riscv/kvm/vcpu_sbi.c b/arch/riscv/kvm/vcpu_sbi.c
index 46ab7b989432..b737e9a7a12a 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;
+
+		ext->validate(vcpu);
+	}
+}
-- 
2.55.0

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

[sashiko-bot]

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

[PATCH v3 2/4] RISC-V: KVM: Add SBI FWFT validation support

[Inochi Amaoto]

Since the KVM SBI has extension parameters validation support,
implement it for the SBI FWFT support.

Signed-off-by: Inochi Amaoto <inochiama@gmail.com>
Reviewed-by: Anup Patel <anup@brainfault.org>
---
 arch/riscv/kvm/vcpu_sbi_fwft.c | 25 +++++++++++++++++++++++++
 1 file changed, 25 insertions(+)

diff --git a/arch/riscv/kvm/vcpu_sbi_fwft.c b/arch/riscv/kvm/vcpu_sbi_fwft.c
index 03153fd8d15f..56fb59afe02a 100644
--- a/arch/riscv/kvm/vcpu_sbi_fwft.c
+++ b/arch/riscv/kvm/vcpu_sbi_fwft.c
@@ -603,6 +603,30 @@ static void kvm_sbi_ext_fwft_reset(struct kvm_vcpu *vcpu)
 	}
 }
 
+static void kvm_sbi_ext_fwft_validate(struct kvm_vcpu *vcpu)
+{
+	struct kvm_sbi_fwft *fwft = vcpu_to_fwft(vcpu);
+	const struct kvm_sbi_fwft_feature *feature;
+	struct kvm_sbi_fwft_config *conf;
+	int i;
+
+	for (i = 0; i < ARRAY_SIZE(features); i++) {
+		feature = &features[i];
+		conf = &fwft->configs[i];
+		if (!conf->supported)
+			continue;
+
+		if (!feature->supported || feature->supported(vcpu))
+			continue;
+
+		conf->enabled = false;
+		conf->flags = 0;
+
+		if (feature->reset)
+			feature->reset(vcpu);
+	}
+}
+
 static unsigned long kvm_sbi_ext_fwft_get_reg_count(struct kvm_vcpu *vcpu)
 {
 	unsigned long max_reg_count = sizeof(struct kvm_riscv_sbi_fwft) / sizeof(unsigned long);
@@ -755,6 +779,7 @@ const struct kvm_vcpu_sbi_extension vcpu_sbi_ext_fwft = {
 	.init = kvm_sbi_ext_fwft_init,
 	.deinit = kvm_sbi_ext_fwft_deinit,
 	.reset = kvm_sbi_ext_fwft_reset,
+	.validate = kvm_sbi_ext_fwft_validate,
 	.state_reg_subtype = KVM_REG_RISCV_SBI_FWFT,
 	.get_state_reg_count = kvm_sbi_ext_fwft_get_reg_count,
 	.get_state_reg_id = kvm_sbi_ext_fwft_get_reg_id,
-- 
2.55.0

Re: [PATCH v3 2/4] RISC-V: KVM: Add SBI FWFT validation support

[sashiko-bot]

Thank you for your contribution! Sashiko AI review found 1 potential issue(s) to consider:

Pre-existing issues:
- [High] The FWFT SBI extension defaults to enabled, breaking live migration and violating KVM ABI guidelines.
--

--- Patch [2]: [PATCH v3 2/4] RISC-V: KVM: Add SBI FWFT validation support ---
commit 9a4432f98b4fe06897baf4b85a503109a0f2b7f8
Author: Inochi Amaoto <inochiama@gmail.com>

RISC-V: KVM: Add SBI FWFT validation support

This commit implements extension parameters validation support for the KVM
SBI FWFT extension.

> diff --git a/arch/riscv/kvm/vcpu_sbi_fwft.c b/arch/riscv/kvm/vcpu_sbi_fwft.c
> index 1342adb3180c6..2794e9e5b495e 100644
> --- a/arch/riscv/kvm/vcpu_sbi_fwft.c
> +++ b/arch/riscv/kvm/vcpu_sbi_fwft.c

[ ... ]

> @@ -574,6 +598,7 @@ const struct kvm_vcpu_sbi_extension vcpu_sbi_ext_fwft = {
>  	.init = kvm_sbi_ext_fwft_init,
>  	.deinit = kvm_sbi_ext_fwft_deinit,
>  	.reset = kvm_sbi_ext_fwft_reset,
> +	.validate = kvm_sbi_ext_fwft_validate,
[Severity: High]
This isn't a bug introduced by this patch, but does this extension default
to enabled because it lacks the .default_disabled = true flag in this
struct?

If an older userspace creates a VM on a new kernel with the FWFT extension,
the kernel silently exposes this capability to the guest. If the guest uses
this feature and is subsequently live-migrated to an older kernel that
lacks FWFT support, will the migration silently fail when the extension
unexpectedly disappears?

Could we add .default_disabled = true to vcpu_sbi_ext_fwft to ensure proper
capability negotiation?

>  	.state_reg_subtype = KVM_REG_RISCV_SBI_FWFT,
>  	.get_state_reg_count = kvm_sbi_ext_fwft_get_reg_count,
>  	.get_state_reg_id = kvm_sbi_ext_fwft_get_reg_id,

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

[PATCH v3 3/4] RISC-V: KVM: Reset the SBI extension when disable it

[Inochi Amaoto]

The SBI extension validation callback can only fix the parameter if
the extension is enabled. However, if the extension is disabled after
modifty some parameters, the state of the extension is still broken.

Reset the extension when the extension is disabled so it can have
a clear context.

Signed-off-by: Inochi Amaoto <inochiama@gmail.com>
---
 arch/riscv/kvm/vcpu_sbi.c | 7 +++++++
 1 file changed, 7 insertions(+)

diff --git a/arch/riscv/kvm/vcpu_sbi.c b/arch/riscv/kvm/vcpu_sbi.c
index b737e9a7a12a..1c4f874368fb 100644
--- a/arch/riscv/kvm/vcpu_sbi.c
+++ b/arch/riscv/kvm/vcpu_sbi.c
@@ -221,6 +221,7 @@ static int riscv_vcpu_set_sbi_ext_single(struct kvm_vcpu *vcpu,
 {
 	struct kvm_vcpu_sbi_context *scontext = &vcpu->arch.sbi_context;
 	const struct kvm_riscv_sbi_extension_entry *sext;
+	const struct kvm_vcpu_sbi_extension *ext;
 
 	if (reg_val != 1 && reg_val != 0)
 		return -EINVAL;
@@ -229,6 +230,12 @@ static int riscv_vcpu_set_sbi_ext_single(struct kvm_vcpu *vcpu,
 	if (!sext || scontext->ext_status[sext->ext_idx] == KVM_RISCV_SBI_EXT_STATUS_UNAVAILABLE)
 		return -ENOENT;
 
+	ext = sext->ext_ptr;
+
+	if (!reg_val && scontext->ext_status[sext->ext_idx] == KVM_RISCV_SBI_EXT_STATUS_ENABLED &&
+	    ext->reset)
+		ext->reset(vcpu);
+
 	scontext->ext_status[sext->ext_idx] = (reg_val) ?
 			KVM_RISCV_SBI_EXT_STATUS_ENABLED :
 			KVM_RISCV_SBI_EXT_STATUS_DISABLED;
-- 
2.55.0

[PATCH v3 4/4] RISC-V: KVM: Mark the reset callback of FWFT extension always dirties CSR

[Inochi Amaoto]

As the CSR is only flushed when the csr_dirty is set, always set the
csr_dirty field in kvm_sbi_ext_fwft_reset() so the CSR change can take
effect immediately. This unconditional set should be safe as all
supported FWFT features change the CSR state currently.

Signed-off-by: Inochi Amaoto <inochiama@gmail.com>
---
 arch/riscv/kvm/vcpu_sbi_fwft.c | 2 ++
 1 file changed, 2 insertions(+)

diff --git a/arch/riscv/kvm/vcpu_sbi_fwft.c b/arch/riscv/kvm/vcpu_sbi_fwft.c
index 56fb59afe02a..2e35e7e12327 100644
--- a/arch/riscv/kvm/vcpu_sbi_fwft.c
+++ b/arch/riscv/kvm/vcpu_sbi_fwft.c
@@ -601,6 +601,8 @@ static void kvm_sbi_ext_fwft_reset(struct kvm_vcpu *vcpu)
 		if (feature->reset)
 			feature->reset(vcpu);
 	}
+
+	vcpu->arch.csr_dirty = true;
 }
 
 static void kvm_sbi_ext_fwft_validate(struct kvm_vcpu *vcpu)
-- 
2.55.0

Re: [PATCH v3 4/4] RISC-V: KVM: Mark the reset callback of FWFT extension always dirties CSR

[sashiko-bot]

Thank you for your contribution! Sashiko AI review found 2 potential issue(s) to consider:

New issues:
- [Low] The addition of `vcpu->arch.csr_dirty = true` in `kvm_sbi_ext_fwft_reset()` is entirely redundant, contradicting the commit message premise.

Pre-existing issues:
- [High] Missing `vcpu_load()` around `KVM_SET_ONE_REG` for timer state causes physical CPU hardware CSR corruption.
--

--- Patch [4]: [PATCH v3 4/4] RISC-V: KVM: Mark the reset callback of FWFT extension always dirties CSR ---
commit 316b183f1319c7935d993137c0fbff524d4ecc07
Author: Inochi Amaoto <inochiama@gmail.com>

RISC-V: KVM: Mark the reset callback of FWFT extension always dirties CSR

As the CSR is only flushed when the csr_dirty is set, always set the
csr_dirty field in kvm_sbi_ext_fwft_reset() so the CSR change can take
effect immediately.

> diff --git a/arch/riscv/kvm/vcpu_sbi_fwft.c b/arch/riscv/kvm/vcpu_sbi_fwft.c
> index 2794e9e5b495e..8748df921f101 100644
> --- a/arch/riscv/kvm/vcpu_sbi_fwft.c
> +++ b/arch/riscv/kvm/vcpu_sbi_fwft.c
> @@ -420,6 +420,8 @@ static void kvm_sbi_ext_fwft_reset(struct kvm_vcpu *vcpu)
>  		if (feature->reset)
>  			feature->reset(vcpu);
>  	}
> +
> +	vcpu->arch.csr_dirty = true;

[Severity: Low]
Is this assignment actually needed?

The commit message states csr_dirty = true is added so the CSR change takes
effect immediately. However, kvm_sbi_ext_fwft_reset() is executed exclusively
via the .reset callback in two paths: riscv_vcpu_set_sbi_ext_single() (which
returns -EBUSY if the vCPU has already run) and kvm_riscv_vcpu_sbi_reset()
(called immediately after last_exit_cpu is explicitly set to -1).

In both scenarios, vcpu->arch.last_exit_cpu is guaranteed to be -1.
When kvm_arch_vcpu_load() later evaluates the fast-path condition:

arch/riscv/kvm/vcpu.c:kvm_arch_vcpu_load() {
    ...
	else if (vcpu->arch.last_exit_cpu == cpu && !vcpu->arch.csr_dirty)
		goto csr_restore_done;
    ...
}

It will always evaluate to false (since physical cpu >= 0), meaning KVM
unconditionally bypasses the skip and flushes the CSRs anyway. Does this make
the csr_dirty assignment redundant?

>  }

[Severity: High]
This is a pre-existing issue, but I noticed a potential hardware CSR
corruption when setting the timer state via ioctl.

Unlike x86, s390, MIPS, and LoongArch, the RISC-V implementation of
kvm_arch_vcpu_ioctl() handles KVM_SET_ONE_REG without acquiring
vcpu_load(vcpu):

arch/riscv/kvm/vcpu.c:kvm_arch_vcpu_ioctl() {
    ...
	if (ioctl == KVM_SET_ONE_REG)
		r = kvm_riscv_vcpu_set_reg(vcpu, &reg);
    ...
}

When an unprivileged userspace process sets the timer state (e.g.,
KVM_REG_RISCV_TIMER_REG(state) to KVM_RISCV_TIMER_STATE_ON) on a system with
the SSTC extension, it eventually invokes kvm_riscv_vcpu_update_vstimecmp(),
which executes a direct hardware write:

arch/riscv/kvm/vcpu_timer.c:kvm_riscv_vcpu_update_vstimecmp() {
    ...
	ncsr_write(CSR_VSTIMECMP, ncycles);
    ...
}

Because the vCPU is not loaded onto the host CPU and preempt notifiers are
not active, does this directly corrupt the physical CPU's VSTIMECMP hardware
register?

If this destroys the -1UL safeguard, could it result in spurious guest timer
interrupts on the host, degraded power-savings (where WFI becomes a NOP), or
corruption of another running vCPU's timer state?

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