[PATCH 0/5] KVM: RISC-V: VCPU reset fixes

[PATCH 0/5] KVM: RISC-V: VCPU reset fixes

[Radim Krčmář]

Hello,

this series started with a simple fix [5/5], which sadly didn't fix
enough without [4/5].  [1-3/5] are refactors to make the reset a bit
easier to follow.  ([1-3,5/5] are applicable without [4/5].)

[4/5] changes the userspace ABI and I'd like to gather your opinions on
how the ABI is supposed to work.

As another proposal, what do you think about an IOCTL that allows
userspace to invoke any KVM SBI ecall?
Userspace could call the KVM HSM implementation to reset and start a
VCPU, but I think that kvm_mp_state is an SBI HSM interface, so we have
an obscure IOCTL for it already.
I was also thinking about using KVM_MP_STATE_UNINITIALIZED to
distinguish that KVM should reset the state when becoming runnable.

I recommend to start with the following hunk in [4/5], as it's the only
significant part of this series:

  @@ -520,6 +525,10 @@ int kvm_arch_vcpu_ioctl_set_mpstate(struct kvm_vcpu *vcpu,
   
   	switch (mp_state->mp_state) {
   	case KVM_MP_STATE_RUNNABLE:
  +		if (riscv_vcpu_supports_sbi_ext(vcpu, KVM_RISCV_SBI_EXT_HSM) &&
  +				vcpu->arch.ran_atleast_once &&
  +				kvm_riscv_vcpu_stopped(vcpu))
  +			kvm_riscv_vcpu_sbi_request_reset_from_userspace(vcpu);

Thanks.

Radim Krčmář (5):
  KVM: RISC-V: refactor vector state reset
  KVM: RISC-V: refactor sbi reset request
  KVM: RISC-V: remove unnecessary SBI reset state
  KVM: RISC-V: reset VCPU state when becoming runnable
  KVM: RISC-V: reset smstateen CSRs

 arch/riscv/include/asm/kvm_aia.h         |  3 --
 arch/riscv/include/asm/kvm_host.h        | 13 +++--
 arch/riscv/include/asm/kvm_vcpu_sbi.h    |  5 ++
 arch/riscv/include/asm/kvm_vcpu_vector.h |  6 +--
 arch/riscv/kvm/aia_device.c              |  4 +-
 arch/riscv/kvm/vcpu.c                    | 69 +++++++++++++++---------
 arch/riscv/kvm/vcpu_sbi.c                | 28 ++++++++++
 arch/riscv/kvm/vcpu_sbi_hsm.c            | 13 +----
 arch/riscv/kvm/vcpu_sbi_system.c         | 10 +---
 arch/riscv/kvm/vcpu_vector.c             | 13 ++---
 10 files changed, 97 insertions(+), 67 deletions(-)

-- 
2.48.1


_______________________________________________
linux-riscv mailing list
linux-riscv@lists.infradead.org
http://lists.infradead.org/mailman/listinfo/linux-riscv

[PATCH 1/5] KVM: RISC-V: refactor vector state reset

[Radim Krčmář]

Do not depend on the reset structures.

vector.datap is a kernel memory pointer that needs to be preserved as it
is not a part of the guest vector data.

Signed-off-by: Radim Krčmář <rkrcmar@ventanamicro.com>
---
 arch/riscv/include/asm/kvm_vcpu_vector.h |  6 ++----
 arch/riscv/kvm/vcpu.c                    |  5 ++++-
 arch/riscv/kvm/vcpu_vector.c             | 13 +++++++------
 3 files changed, 13 insertions(+), 11 deletions(-)

diff --git a/arch/riscv/include/asm/kvm_vcpu_vector.h b/arch/riscv/include/asm/kvm_vcpu_vector.h
index 27f5bccdd8b0..57a798a4cb0d 100644
--- a/arch/riscv/include/asm/kvm_vcpu_vector.h
+++ b/arch/riscv/include/asm/kvm_vcpu_vector.h
@@ -33,8 +33,7 @@ void kvm_riscv_vcpu_guest_vector_restore(struct kvm_cpu_context *cntx,
 					 unsigned long *isa);
 void kvm_riscv_vcpu_host_vector_save(struct kvm_cpu_context *cntx);
 void kvm_riscv_vcpu_host_vector_restore(struct kvm_cpu_context *cntx);
-int kvm_riscv_vcpu_alloc_vector_context(struct kvm_vcpu *vcpu,
-					struct kvm_cpu_context *cntx);
+int kvm_riscv_vcpu_alloc_vector_context(struct kvm_vcpu *vcpu);
 void kvm_riscv_vcpu_free_vector_context(struct kvm_vcpu *vcpu);
 #else
 
@@ -62,8 +61,7 @@ static inline void kvm_riscv_vcpu_host_vector_restore(struct kvm_cpu_context *cn
 {
 }
 
-static inline int kvm_riscv_vcpu_alloc_vector_context(struct kvm_vcpu *vcpu,
-						      struct kvm_cpu_context *cntx)
+static inline int kvm_riscv_vcpu_alloc_vector_context(struct kvm_vcpu *vcpu)
 {
 	return 0;
 }
diff --git a/arch/riscv/kvm/vcpu.c b/arch/riscv/kvm/vcpu.c
index 60d684c76c58..2fb75288ecfe 100644
--- a/arch/riscv/kvm/vcpu.c
+++ b/arch/riscv/kvm/vcpu.c
@@ -57,6 +57,7 @@ static void kvm_riscv_reset_vcpu(struct kvm_vcpu *vcpu)
 	struct kvm_vcpu_csr *reset_csr = &vcpu->arch.guest_reset_csr;
 	struct kvm_cpu_context *cntx = &vcpu->arch.guest_context;
 	struct kvm_cpu_context *reset_cntx = &vcpu->arch.guest_reset_context;
+	void *vector_datap = cntx->vector.datap;
 	bool loaded;
 
 	/**
@@ -79,6 +80,8 @@ static void kvm_riscv_reset_vcpu(struct kvm_vcpu *vcpu)
 
 	kvm_riscv_vcpu_fp_reset(vcpu);
 
+	/* Restore datap as it's not a part of the guest context. */
+	cntx->vector.datap = vector_datap;
 	kvm_riscv_vcpu_vector_reset(vcpu);
 
 	kvm_riscv_vcpu_timer_reset(vcpu);
@@ -143,7 +146,7 @@ int kvm_arch_vcpu_create(struct kvm_vcpu *vcpu)
 	cntx->hstatus |= HSTATUS_SPV;
 	spin_unlock(&vcpu->arch.reset_cntx_lock);
 
-	if (kvm_riscv_vcpu_alloc_vector_context(vcpu, cntx))
+	if (kvm_riscv_vcpu_alloc_vector_context(vcpu))
 		return -ENOMEM;
 
 	/* By default, make CY, TM, and IR counters accessible in VU mode */
diff --git a/arch/riscv/kvm/vcpu_vector.c b/arch/riscv/kvm/vcpu_vector.c
index d92d1348045c..a5f88cb717f3 100644
--- a/arch/riscv/kvm/vcpu_vector.c
+++ b/arch/riscv/kvm/vcpu_vector.c
@@ -22,6 +22,9 @@ void kvm_riscv_vcpu_vector_reset(struct kvm_vcpu *vcpu)
 	struct kvm_cpu_context *cntx = &vcpu->arch.guest_context;
 
 	cntx->sstatus &= ~SR_VS;
+
+	cntx->vector.vlenb = riscv_v_vsize / 32;
+
 	if (riscv_isa_extension_available(isa, v)) {
 		cntx->sstatus |= SR_VS_INITIAL;
 		WARN_ON(!cntx->vector.datap);
@@ -70,13 +73,11 @@ void kvm_riscv_vcpu_host_vector_restore(struct kvm_cpu_context *cntx)
 		__kvm_riscv_vector_restore(cntx);
 }
 
-int kvm_riscv_vcpu_alloc_vector_context(struct kvm_vcpu *vcpu,
-					struct kvm_cpu_context *cntx)
+int kvm_riscv_vcpu_alloc_vector_context(struct kvm_vcpu *vcpu)
 {
-	cntx->vector.datap = kmalloc(riscv_v_vsize, GFP_KERNEL);
-	if (!cntx->vector.datap)
+	vcpu->arch.guest_context.vector.datap = kzalloc(riscv_v_vsize, GFP_KERNEL);
+	if (!vcpu->arch.guest_context.vector.datap)
 		return -ENOMEM;
-	cntx->vector.vlenb = riscv_v_vsize / 32;
 
 	vcpu->arch.host_context.vector.datap = kzalloc(riscv_v_vsize, GFP_KERNEL);
 	if (!vcpu->arch.host_context.vector.datap)
@@ -87,7 +88,7 @@ int kvm_riscv_vcpu_alloc_vector_context(struct kvm_vcpu *vcpu,
 
 void kvm_riscv_vcpu_free_vector_context(struct kvm_vcpu *vcpu)
 {
-	kfree(vcpu->arch.guest_reset_context.vector.datap);
+	kfree(vcpu->arch.guest_context.vector.datap);
 	kfree(vcpu->arch.host_context.vector.datap);
 }
 #endif
-- 
2.48.1


_______________________________________________
linux-riscv mailing list
linux-riscv@lists.infradead.org
http://lists.infradead.org/mailman/listinfo/linux-riscv

[PATCH 2/5] KVM: RISC-V: refactor sbi reset request

[Radim Krčmář]

The same code is used twice and SBI reset sets only two variables.

Signed-off-by: Radim Krčmář <rkrcmar@ventanamicro.com>
---
 arch/riscv/include/asm/kvm_vcpu_sbi.h |  2 ++
 arch/riscv/kvm/vcpu_sbi.c             | 12 ++++++++++++
 arch/riscv/kvm/vcpu_sbi_hsm.c         | 13 +------------
 arch/riscv/kvm/vcpu_sbi_system.c      | 10 +---------
 4 files changed, 16 insertions(+), 21 deletions(-)

diff --git a/arch/riscv/include/asm/kvm_vcpu_sbi.h b/arch/riscv/include/asm/kvm_vcpu_sbi.h
index 4ed6203cdd30..aaaa81355276 100644
--- a/arch/riscv/include/asm/kvm_vcpu_sbi.h
+++ b/arch/riscv/include/asm/kvm_vcpu_sbi.h
@@ -55,6 +55,8 @@ void kvm_riscv_vcpu_sbi_forward(struct kvm_vcpu *vcpu, struct kvm_run *run);
 void kvm_riscv_vcpu_sbi_system_reset(struct kvm_vcpu *vcpu,
 				     struct kvm_run *run,
 				     u32 type, u64 flags);
+void kvm_riscv_vcpu_sbi_request_reset(struct kvm_vcpu *vcpu,
+                                      unsigned long pc, unsigned long a1);
 int kvm_riscv_vcpu_sbi_return(struct kvm_vcpu *vcpu, struct kvm_run *run);
 int kvm_riscv_vcpu_set_reg_sbi_ext(struct kvm_vcpu *vcpu,
 				   const struct kvm_one_reg *reg);
diff --git a/arch/riscv/kvm/vcpu_sbi.c b/arch/riscv/kvm/vcpu_sbi.c
index d1c83a77735e..f58368f7df1d 100644
--- a/arch/riscv/kvm/vcpu_sbi.c
+++ b/arch/riscv/kvm/vcpu_sbi.c
@@ -156,6 +156,18 @@ void kvm_riscv_vcpu_sbi_system_reset(struct kvm_vcpu *vcpu,
 	run->exit_reason = KVM_EXIT_SYSTEM_EVENT;
 }
 
+void kvm_riscv_vcpu_sbi_request_reset(struct kvm_vcpu *vcpu,
+                                      unsigned long pc, unsigned long a1)
+{
+	spin_lock(&vcpu->arch.reset_cntx_lock);
+	vcpu->arch.guest_reset_context.sepc = pc;
+	vcpu->arch.guest_reset_context.a0 = vcpu->vcpu_id;
+	vcpu->arch.guest_reset_context.a1 = a1;
+	spin_unlock(&vcpu->arch.reset_cntx_lock);
+
+	kvm_make_request(KVM_REQ_VCPU_RESET, vcpu);
+}
+
 int kvm_riscv_vcpu_sbi_return(struct kvm_vcpu *vcpu, struct kvm_run *run)
 {
 	struct kvm_cpu_context *cp = &vcpu->arch.guest_context;
diff --git a/arch/riscv/kvm/vcpu_sbi_hsm.c b/arch/riscv/kvm/vcpu_sbi_hsm.c
index 3070bb31745d..f26207f84bab 100644
--- a/arch/riscv/kvm/vcpu_sbi_hsm.c
+++ b/arch/riscv/kvm/vcpu_sbi_hsm.c
@@ -15,7 +15,6 @@
 
 static int kvm_sbi_hsm_vcpu_start(struct kvm_vcpu *vcpu)
 {
-	struct kvm_cpu_context *reset_cntx;
 	struct kvm_cpu_context *cp = &vcpu->arch.guest_context;
 	struct kvm_vcpu *target_vcpu;
 	unsigned long target_vcpuid = cp->a0;
@@ -32,17 +31,7 @@ static int kvm_sbi_hsm_vcpu_start(struct kvm_vcpu *vcpu)
 		goto out;
 	}
 
-	spin_lock(&target_vcpu->arch.reset_cntx_lock);
-	reset_cntx = &target_vcpu->arch.guest_reset_context;
-	/* start address */
-	reset_cntx->sepc = cp->a1;
-	/* target vcpu id to start */
-	reset_cntx->a0 = target_vcpuid;
-	/* private data passed from kernel */
-	reset_cntx->a1 = cp->a2;
-	spin_unlock(&target_vcpu->arch.reset_cntx_lock);
-
-	kvm_make_request(KVM_REQ_VCPU_RESET, target_vcpu);
+	kvm_riscv_vcpu_sbi_request_reset(target_vcpu, cp->a1, cp->a2);
 
 	__kvm_riscv_vcpu_power_on(target_vcpu);
 
diff --git a/arch/riscv/kvm/vcpu_sbi_system.c b/arch/riscv/kvm/vcpu_sbi_system.c
index bc0ebba89003..359be90b0fc5 100644
--- a/arch/riscv/kvm/vcpu_sbi_system.c
+++ b/arch/riscv/kvm/vcpu_sbi_system.c
@@ -13,7 +13,6 @@ static int kvm_sbi_ext_susp_handler(struct kvm_vcpu *vcpu, struct kvm_run *run,
 				    struct kvm_vcpu_sbi_return *retdata)
 {
 	struct kvm_cpu_context *cp = &vcpu->arch.guest_context;
-	struct kvm_cpu_context *reset_cntx;
 	unsigned long funcid = cp->a6;
 	unsigned long hva, i;
 	struct kvm_vcpu *tmp;
@@ -45,14 +44,7 @@ static int kvm_sbi_ext_susp_handler(struct kvm_vcpu *vcpu, struct kvm_run *run,
 			}
 		}
 
-		spin_lock(&vcpu->arch.reset_cntx_lock);
-		reset_cntx = &vcpu->arch.guest_reset_context;
-		reset_cntx->sepc = cp->a1;
-		reset_cntx->a0 = vcpu->vcpu_id;
-		reset_cntx->a1 = cp->a2;
-		spin_unlock(&vcpu->arch.reset_cntx_lock);
-
-		kvm_make_request(KVM_REQ_VCPU_RESET, vcpu);
+		kvm_riscv_vcpu_sbi_request_reset(vcpu, cp->a1, cp->a2);
 
 		/* userspace provides the suspend implementation */
 		kvm_riscv_vcpu_sbi_forward(vcpu, run);
-- 
2.48.1


_______________________________________________
linux-riscv mailing list
linux-riscv@lists.infradead.org
http://lists.infradead.org/mailman/listinfo/linux-riscv

[PATCH 3/5] KVM: RISC-V: remove unnecessary SBI reset state

[Radim Krčmář]

The SBI reset state has only two variables -- pc and a1.
The rest is known, so keep only the necessary information.

The reset structures make sense if we want userspace to control the
reset state (which we do), but I'd still remove them now and reintroduce
with the userspace interface later -- we could probably have just a
single reset state per VM, instead of a reset state for each VCPU.

Signed-off-by: Radim Krčmář <rkrcmar@ventanamicro.com>
---
 arch/riscv/include/asm/kvm_aia.h  |  3 --
 arch/riscv/include/asm/kvm_host.h | 12 ++++---
 arch/riscv/kvm/aia_device.c       |  4 +--
 arch/riscv/kvm/vcpu.c             | 58 +++++++++++++++++--------------
 arch/riscv/kvm/vcpu_sbi.c         |  9 +++--
 5 files changed, 44 insertions(+), 42 deletions(-)

diff --git a/arch/riscv/include/asm/kvm_aia.h b/arch/riscv/include/asm/kvm_aia.h
index 1f37b600ca47..3b643b9efc07 100644
--- a/arch/riscv/include/asm/kvm_aia.h
+++ b/arch/riscv/include/asm/kvm_aia.h
@@ -63,9 +63,6 @@ struct kvm_vcpu_aia {
 	/* CPU AIA CSR context of Guest VCPU */
 	struct kvm_vcpu_aia_csr guest_csr;
 
-	/* CPU AIA CSR context upon Guest VCPU reset */
-	struct kvm_vcpu_aia_csr guest_reset_csr;
-
 	/* Guest physical address of IMSIC for this VCPU */
 	gpa_t		imsic_addr;
 
diff --git a/arch/riscv/include/asm/kvm_host.h b/arch/riscv/include/asm/kvm_host.h
index 0e9c2fab6378..0c8c9c05af91 100644
--- a/arch/riscv/include/asm/kvm_host.h
+++ b/arch/riscv/include/asm/kvm_host.h
@@ -193,6 +193,12 @@ struct kvm_vcpu_smstateen_csr {
 	unsigned long sstateen0;
 };
 
+struct kvm_vcpu_reset_state {
+	spinlock_t lock;
+	unsigned long pc;
+	unsigned long a1;
+};
+
 struct kvm_vcpu_arch {
 	/* VCPU ran at least once */
 	bool ran_atleast_once;
@@ -227,12 +233,8 @@ struct kvm_vcpu_arch {
 	/* CPU Smstateen CSR context of Guest VCPU */
 	struct kvm_vcpu_smstateen_csr smstateen_csr;
 
-	/* CPU context upon Guest VCPU reset */
-	struct kvm_cpu_context guest_reset_context;
-	spinlock_t reset_cntx_lock;
+	struct kvm_vcpu_reset_state reset_state;
 
-	/* CPU CSR context upon Guest VCPU reset */
-	struct kvm_vcpu_csr guest_reset_csr;
 
 	/*
 	 * VCPU interrupts
diff --git a/arch/riscv/kvm/aia_device.c b/arch/riscv/kvm/aia_device.c
index 39cd26af5a69..43e472ff3e1a 100644
--- a/arch/riscv/kvm/aia_device.c
+++ b/arch/riscv/kvm/aia_device.c
@@ -526,12 +526,10 @@ int kvm_riscv_vcpu_aia_update(struct kvm_vcpu *vcpu)
 void kvm_riscv_vcpu_aia_reset(struct kvm_vcpu *vcpu)
 {
 	struct kvm_vcpu_aia_csr *csr = &vcpu->arch.aia_context.guest_csr;
-	struct kvm_vcpu_aia_csr *reset_csr =
-				&vcpu->arch.aia_context.guest_reset_csr;
 
 	if (!kvm_riscv_aia_available())
 		return;
-	memcpy(csr, reset_csr, sizeof(*csr));
+	memset(csr, 0, sizeof(*csr));
 
 	/* Proceed only if AIA was initialized successfully */
 	if (!kvm_riscv_aia_initialized(vcpu->kvm))
diff --git a/arch/riscv/kvm/vcpu.c b/arch/riscv/kvm/vcpu.c
index 2fb75288ecfe..b8485c1c1ce4 100644
--- a/arch/riscv/kvm/vcpu.c
+++ b/arch/riscv/kvm/vcpu.c
@@ -51,13 +51,40 @@ const struct kvm_stats_header kvm_vcpu_stats_header = {
 		       sizeof(kvm_vcpu_stats_desc),
 };
 
-static void kvm_riscv_reset_vcpu(struct kvm_vcpu *vcpu)
+static void kvm_riscv_vcpu_context_reset(struct kvm_vcpu *vcpu)
 {
 	struct kvm_vcpu_csr *csr = &vcpu->arch.guest_csr;
-	struct kvm_vcpu_csr *reset_csr = &vcpu->arch.guest_reset_csr;
 	struct kvm_cpu_context *cntx = &vcpu->arch.guest_context;
-	struct kvm_cpu_context *reset_cntx = &vcpu->arch.guest_reset_context;
+	struct kvm_vcpu_reset_state *reset_state = &vcpu->arch.reset_state;
 	void *vector_datap = cntx->vector.datap;
+
+	memset(cntx, 0, sizeof(*cntx));
+	memset(csr, 0, sizeof(*csr));
+
+	/* Restore datap as it's not a part of the guest context. */
+	cntx->vector.datap = vector_datap;
+
+	/* Load SBI reset values */
+	cntx->a0 = vcpu->vcpu_id;
+
+	spin_lock(&reset_state->lock);
+	cntx->sepc = reset_state->pc;
+	cntx->a1 = reset_state->a1;
+	spin_unlock(&reset_state->lock);
+
+	/* Setup reset state of shadow SSTATUS and HSTATUS CSRs */
+	cntx->sstatus = SR_SPP | SR_SPIE;
+
+	cntx->hstatus |= HSTATUS_VTW;
+	cntx->hstatus |= HSTATUS_SPVP;
+	cntx->hstatus |= HSTATUS_SPV;
+
+	/* By default, make CY, TM, and IR counters accessible in VU mode */
+	csr->scounteren = 0x7;
+}
+
+static void kvm_riscv_reset_vcpu(struct kvm_vcpu *vcpu)
+{
 	bool loaded;
 
 	/**
@@ -72,16 +99,10 @@ static void kvm_riscv_reset_vcpu(struct kvm_vcpu *vcpu)
 
 	vcpu->arch.last_exit_cpu = -1;
 
-	memcpy(csr, reset_csr, sizeof(*csr));
-
-	spin_lock(&vcpu->arch.reset_cntx_lock);
-	memcpy(cntx, reset_cntx, sizeof(*cntx));
-	spin_unlock(&vcpu->arch.reset_cntx_lock);
+	kvm_riscv_vcpu_context_reset(vcpu);
 
 	kvm_riscv_vcpu_fp_reset(vcpu);
 
-	/* Restore datap as it's not a part of the guest context. */
-	cntx->vector.datap = vector_datap;
 	kvm_riscv_vcpu_vector_reset(vcpu);
 
 	kvm_riscv_vcpu_timer_reset(vcpu);
@@ -113,8 +134,6 @@ int kvm_arch_vcpu_precreate(struct kvm *kvm, unsigned int id)
 int kvm_arch_vcpu_create(struct kvm_vcpu *vcpu)
 {
 	int rc;
-	struct kvm_cpu_context *cntx;
-	struct kvm_vcpu_csr *reset_csr = &vcpu->arch.guest_reset_csr;
 
 	spin_lock_init(&vcpu->arch.mp_state_lock);
 
@@ -134,24 +153,11 @@ int kvm_arch_vcpu_create(struct kvm_vcpu *vcpu)
 	/* Setup VCPU hfence queue */
 	spin_lock_init(&vcpu->arch.hfence_lock);
 
-	/* Setup reset state of shadow SSTATUS and HSTATUS CSRs */
-	spin_lock_init(&vcpu->arch.reset_cntx_lock);
-
-	spin_lock(&vcpu->arch.reset_cntx_lock);
-	cntx = &vcpu->arch.guest_reset_context;
-	cntx->sstatus = SR_SPP | SR_SPIE;
-	cntx->hstatus = 0;
-	cntx->hstatus |= HSTATUS_VTW;
-	cntx->hstatus |= HSTATUS_SPVP;
-	cntx->hstatus |= HSTATUS_SPV;
-	spin_unlock(&vcpu->arch.reset_cntx_lock);
+	spin_lock_init(&vcpu->arch.reset_state.lock);
 
 	if (kvm_riscv_vcpu_alloc_vector_context(vcpu))
 		return -ENOMEM;
 
-	/* By default, make CY, TM, and IR counters accessible in VU mode */
-	reset_csr->scounteren = 0x7;
-
 	/* Setup VCPU timer */
 	kvm_riscv_vcpu_timer_init(vcpu);
 
diff --git a/arch/riscv/kvm/vcpu_sbi.c b/arch/riscv/kvm/vcpu_sbi.c
index f58368f7df1d..3d7955e05cc3 100644
--- a/arch/riscv/kvm/vcpu_sbi.c
+++ b/arch/riscv/kvm/vcpu_sbi.c
@@ -159,11 +159,10 @@ void kvm_riscv_vcpu_sbi_system_reset(struct kvm_vcpu *vcpu,
 void kvm_riscv_vcpu_sbi_request_reset(struct kvm_vcpu *vcpu,
                                       unsigned long pc, unsigned long a1)
 {
-	spin_lock(&vcpu->arch.reset_cntx_lock);
-	vcpu->arch.guest_reset_context.sepc = pc;
-	vcpu->arch.guest_reset_context.a0 = vcpu->vcpu_id;
-	vcpu->arch.guest_reset_context.a1 = a1;
-	spin_unlock(&vcpu->arch.reset_cntx_lock);
+	spin_lock(&vcpu->arch.reset_state.lock);
+	vcpu->arch.reset_state.pc = pc;
+	vcpu->arch.reset_state.a1 = a1;
+	spin_unlock(&vcpu->arch.reset_state.lock);
 
 	kvm_make_request(KVM_REQ_VCPU_RESET, vcpu);
 }
-- 
2.48.1


_______________________________________________
linux-riscv mailing list
linux-riscv@lists.infradead.org
http://lists.infradead.org/mailman/listinfo/linux-riscv

[PATCH 4/5] KVM: RISC-V: reset VCPU state when becoming runnable

[Radim Krčmář]

Beware, this patch is "breaking" the userspace interface, because it
fixes a KVM/QEMU bug where the boot VCPU is not being reset by KVM.

The VCPU reset paths are inconsistent right now.  KVM resets VCPUs that
are brought up by KVM-accelerated SBI calls, but does nothing for VCPUs
brought up through ioctls.

We need to perform a KVM reset even when the VCPU is started through an
ioctl.  This patch is one of the ways we can achieve it.

Assume that userspace has no business setting the post-reset state.
KVM is de-facto the SBI implementation, as the SBI HSM acceleration
cannot be disabled and userspace cannot control the reset state, so KVM
should be in full control of the post-reset state.

Do not reset the pc and a1 registers, because SBI reset is expected to
provide them and KVM has no idea what these registers should be -- only
the userspace knows where it put the data.

An important consideration is resume.  Userspace might want to start
with non-reset state.  Check ran_atleast_once to allow this, because
KVM-SBI HSM creates some VCPUs as STOPPED.

The drawback is that userspace can still start the boot VCPU with an
incorrect reset state, because there is no way to distinguish a freshly
reset new VCPU on the KVM side (userspace might set some values by
mistake) from a restored VCPU (userspace must set all values).

The advantage of this solution is that it fixes current QEMU and makes
some sense with the assumption that KVM implements SBI HSM.
I do not like it too much, so I'd be in favor of a different solution if
we can still afford to drop support for current userspaces.

For a cleaner solution, we should add interfaces to perform the KVM-SBI
reset request on userspace demand.  I think it would also be much better
if userspace was in control of the post-reset state.

Signed-off-by: Radim Krčmář <rkrcmar@ventanamicro.com>
---
 arch/riscv/include/asm/kvm_host.h     |  1 +
 arch/riscv/include/asm/kvm_vcpu_sbi.h |  3 +++
 arch/riscv/kvm/vcpu.c                 |  9 +++++++++
 arch/riscv/kvm/vcpu_sbi.c             | 21 +++++++++++++++++++--
 4 files changed, 32 insertions(+), 2 deletions(-)

diff --git a/arch/riscv/include/asm/kvm_host.h b/arch/riscv/include/asm/kvm_host.h
index 0c8c9c05af91..9bbf8c4a286b 100644
--- a/arch/riscv/include/asm/kvm_host.h
+++ b/arch/riscv/include/asm/kvm_host.h
@@ -195,6 +195,7 @@ struct kvm_vcpu_smstateen_csr {
 
 struct kvm_vcpu_reset_state {
 	spinlock_t lock;
+	bool active;
 	unsigned long pc;
 	unsigned long a1;
 };
diff --git a/arch/riscv/include/asm/kvm_vcpu_sbi.h b/arch/riscv/include/asm/kvm_vcpu_sbi.h
index aaaa81355276..2c334a87e02a 100644
--- a/arch/riscv/include/asm/kvm_vcpu_sbi.h
+++ b/arch/riscv/include/asm/kvm_vcpu_sbi.h
@@ -57,6 +57,9 @@ void kvm_riscv_vcpu_sbi_system_reset(struct kvm_vcpu *vcpu,
 				     u32 type, u64 flags);
 void kvm_riscv_vcpu_sbi_request_reset(struct kvm_vcpu *vcpu,
                                       unsigned long pc, unsigned long a1);
+void __kvm_riscv_vcpu_set_reset_state(struct kvm_vcpu *vcpu,
+                                      unsigned long pc, unsigned long a1);
+void kvm_riscv_vcpu_sbi_request_reset_from_userspace(struct kvm_vcpu *vcpu);
 int kvm_riscv_vcpu_sbi_return(struct kvm_vcpu *vcpu, struct kvm_run *run);
 int kvm_riscv_vcpu_set_reg_sbi_ext(struct kvm_vcpu *vcpu,
 				   const struct kvm_one_reg *reg);
diff --git a/arch/riscv/kvm/vcpu.c b/arch/riscv/kvm/vcpu.c
index b8485c1c1ce4..4578863a39e3 100644
--- a/arch/riscv/kvm/vcpu.c
+++ b/arch/riscv/kvm/vcpu.c
@@ -58,6 +58,11 @@ static void kvm_riscv_vcpu_context_reset(struct kvm_vcpu *vcpu)
 	struct kvm_vcpu_reset_state *reset_state = &vcpu->arch.reset_state;
 	void *vector_datap = cntx->vector.datap;
 
+	spin_lock(&reset_state->lock);
+	if (!reset_state->active)
+		__kvm_riscv_vcpu_set_reset_state(vcpu, cntx->sepc, cntx->a1);
+	spin_unlock(&reset_state->lock);
+
 	memset(cntx, 0, sizeof(*cntx));
 	memset(csr, 0, sizeof(*csr));
 
@@ -520,6 +525,10 @@ int kvm_arch_vcpu_ioctl_set_mpstate(struct kvm_vcpu *vcpu,
 
 	switch (mp_state->mp_state) {
 	case KVM_MP_STATE_RUNNABLE:
+		if (riscv_vcpu_supports_sbi_ext(vcpu, KVM_RISCV_SBI_EXT_HSM) &&
+				vcpu->arch.ran_atleast_once &&
+				kvm_riscv_vcpu_stopped(vcpu))
+			kvm_riscv_vcpu_sbi_request_reset_from_userspace(vcpu);
 		WRITE_ONCE(vcpu->arch.mp_state, *mp_state);
 		break;
 	case KVM_MP_STATE_STOPPED:
diff --git a/arch/riscv/kvm/vcpu_sbi.c b/arch/riscv/kvm/vcpu_sbi.c
index 3d7955e05cc3..77f9f0bd3842 100644
--- a/arch/riscv/kvm/vcpu_sbi.c
+++ b/arch/riscv/kvm/vcpu_sbi.c
@@ -156,12 +156,29 @@ void kvm_riscv_vcpu_sbi_system_reset(struct kvm_vcpu *vcpu,
 	run->exit_reason = KVM_EXIT_SYSTEM_EVENT;
 }
 
+/* must be called with held vcpu->arch.reset_state.lock */
+void __kvm_riscv_vcpu_set_reset_state(struct kvm_vcpu *vcpu,
+                                      unsigned long pc, unsigned long a1)
+{
+	vcpu->arch.reset_state.active = true;
+	vcpu->arch.reset_state.pc = pc;
+	vcpu->arch.reset_state.a1 = a1;
+}
+
 void kvm_riscv_vcpu_sbi_request_reset(struct kvm_vcpu *vcpu,
                                       unsigned long pc, unsigned long a1)
 {
 	spin_lock(&vcpu->arch.reset_state.lock);
-	vcpu->arch.reset_state.pc = pc;
-	vcpu->arch.reset_state.a1 = a1;
+	__kvm_riscv_vcpu_set_reset_state(vcpu, pc, a1);
+	spin_unlock(&vcpu->arch.reset_state.lock);
+
+	kvm_make_request(KVM_REQ_VCPU_RESET, vcpu);
+}
+
+void kvm_riscv_vcpu_sbi_request_reset_from_userspace(struct kvm_vcpu *vcpu)
+{
+	spin_lock(&vcpu->arch.reset_state.lock);
+	vcpu->arch.reset_state.active = false;
 	spin_unlock(&vcpu->arch.reset_state.lock);
 
 	kvm_make_request(KVM_REQ_VCPU_RESET, vcpu);
-- 
2.48.1


_______________________________________________
linux-riscv mailing list
linux-riscv@lists.infradead.org
http://lists.infradead.org/mailman/listinfo/linux-riscv

[PATCH 5/5] KVM: RISC-V: reset smstateen CSRs

[Radim Krčmář]

Not resetting smstateen is a potential security hole, because VU might
be able to access state that VS does not properly context-switch.

Fixes: 81f0f314fec9 ("RISCV: KVM: Add sstateen0 context save/restore")
Signed-off-by: Radim Krčmář <rkrcmar@ventanamicro.com>
---
 arch/riscv/kvm/vcpu.c | 1 +
 1 file changed, 1 insertion(+)

diff --git a/arch/riscv/kvm/vcpu.c b/arch/riscv/kvm/vcpu.c
index 4578863a39e3..ac0fa50bc489 100644
--- a/arch/riscv/kvm/vcpu.c
+++ b/arch/riscv/kvm/vcpu.c
@@ -65,6 +65,7 @@ static void kvm_riscv_vcpu_context_reset(struct kvm_vcpu *vcpu)
 
 	memset(cntx, 0, sizeof(*cntx));
 	memset(csr, 0, sizeof(*csr));
+	memset(&vcpu->arch.smstateen_csr, 0, sizeof(vcpu->arch.smstateen_csr));
 
 	/* Restore datap as it's not a part of the guest context. */
 	cntx->vector.datap = vector_datap;
-- 
2.48.1


_______________________________________________
linux-riscv mailing list
linux-riscv@lists.infradead.org
http://lists.infradead.org/mailman/listinfo/linux-riscv

Re: [PATCH 5/5] KVM: RISC-V: reset smstateen CSRs

[Anup Patel]

On Thu, Apr 3, 2025 at 5:02 PM Radim Krčmář <rkrcmar@ventanamicro.com> wrote:
>
> Not resetting smstateen is a potential security hole, because VU might
> be able to access state that VS does not properly context-switch.
>
> Fixes: 81f0f314fec9 ("RISCV: KVM: Add sstateen0 context save/restore")
> Signed-off-by: Radim Krčmář <rkrcmar@ventanamicro.com>

This should be the first patch of the series which can
be applied independently.

I have rebased and queued it as a fix for Linux-6.15.

Regards,
Anup

> ---
>  arch/riscv/kvm/vcpu.c | 1 +
>  1 file changed, 1 insertion(+)
>
> diff --git a/arch/riscv/kvm/vcpu.c b/arch/riscv/kvm/vcpu.c
> index 4578863a39e3..ac0fa50bc489 100644
> --- a/arch/riscv/kvm/vcpu.c
> +++ b/arch/riscv/kvm/vcpu.c
> @@ -65,6 +65,7 @@ static void kvm_riscv_vcpu_context_reset(struct kvm_vcpu *vcpu)
>
>         memset(cntx, 0, sizeof(*cntx));
>         memset(csr, 0, sizeof(*csr));
> +       memset(&vcpu->arch.smstateen_csr, 0, sizeof(vcpu->arch.smstateen_csr));
>
>         /* Restore datap as it's not a part of the guest context. */
>         cntx->vector.datap = vector_datap;
> --
> 2.48.1
>

_______________________________________________
linux-riscv mailing list
linux-riscv@lists.infradead.org
http://lists.infradead.org/mailman/listinfo/linux-riscv

Re: [PATCH 1/5] KVM: RISC-V: refactor vector state reset

[Andrew Jones]

On Thu, Apr 03, 2025 at 01:25:20PM +0200, Radim Krčmář wrote:
> Do not depend on the reset structures.
> 
> vector.datap is a kernel memory pointer that needs to be preserved as it
> is not a part of the guest vector data.
> 
> Signed-off-by: Radim Krčmář <rkrcmar@ventanamicro.com>
> ---
>  arch/riscv/include/asm/kvm_vcpu_vector.h |  6 ++----
>  arch/riscv/kvm/vcpu.c                    |  5 ++++-
>  arch/riscv/kvm/vcpu_vector.c             | 13 +++++++------
>  3 files changed, 13 insertions(+), 11 deletions(-)
>

Reviewed-by: Andrew Jones <ajones@ventanamicro.com>

_______________________________________________
linux-riscv mailing list
linux-riscv@lists.infradead.org
http://lists.infradead.org/mailman/listinfo/linux-riscv

Re: [PATCH 2/5] KVM: RISC-V: refactor sbi reset request

[Andrew Jones]

On Thu, Apr 03, 2025 at 01:25:21PM +0200, Radim Krčmář wrote:
> The same code is used twice and SBI reset sets only two variables.
> 
> Signed-off-by: Radim Krčmář <rkrcmar@ventanamicro.com>
> ---
>  arch/riscv/include/asm/kvm_vcpu_sbi.h |  2 ++
>  arch/riscv/kvm/vcpu_sbi.c             | 12 ++++++++++++
>  arch/riscv/kvm/vcpu_sbi_hsm.c         | 13 +------------
>  arch/riscv/kvm/vcpu_sbi_system.c      | 10 +---------
>  4 files changed, 16 insertions(+), 21 deletions(-)
>

Reviewed-by: Andrew Jones <ajones@ventanamicro.com>

_______________________________________________
linux-riscv mailing list
linux-riscv@lists.infradead.org
http://lists.infradead.org/mailman/listinfo/linux-riscv

Re: [PATCH 3/5] KVM: RISC-V: remove unnecessary SBI reset state

[Andrew Jones]

On Thu, Apr 03, 2025 at 01:25:22PM +0200, Radim Krčmář wrote:
> The SBI reset state has only two variables -- pc and a1.
> The rest is known, so keep only the necessary information.
> 
> The reset structures make sense if we want userspace to control the
> reset state (which we do), but I'd still remove them now and reintroduce
> with the userspace interface later -- we could probably have just a
> single reset state per VM, instead of a reset state for each VCPU.
> 
> Signed-off-by: Radim Krčmář <rkrcmar@ventanamicro.com>
> ---
>  arch/riscv/include/asm/kvm_aia.h  |  3 --
>  arch/riscv/include/asm/kvm_host.h | 12 ++++---
>  arch/riscv/kvm/aia_device.c       |  4 +--
>  arch/riscv/kvm/vcpu.c             | 58 +++++++++++++++++--------------
>  arch/riscv/kvm/vcpu_sbi.c         |  9 +++--
>  5 files changed, 44 insertions(+), 42 deletions(-)
>

Reviewed-by: Andrew Jones <ajones@ventanamicro.com>

_______________________________________________
linux-riscv mailing list
linux-riscv@lists.infradead.org
http://lists.infradead.org/mailman/listinfo/linux-riscv

Re: [PATCH 4/5] KVM: RISC-V: reset VCPU state when becoming runnable

[Andrew Jones]

On Thu, Apr 03, 2025 at 01:25:23PM +0200, Radim Krčmář wrote:
> Beware, this patch is "breaking" the userspace interface, because it
> fixes a KVM/QEMU bug where the boot VCPU is not being reset by KVM.
> 
> The VCPU reset paths are inconsistent right now.  KVM resets VCPUs that
> are brought up by KVM-accelerated SBI calls, but does nothing for VCPUs
> brought up through ioctls.

I guess we currently expect userspace to make a series of set-one-reg
ioctls in order to prepare ("reset") newly created vcpus, and I guess
the problem is that KVM isn't capturing the resulting configuration
in order to replay it when SBI HSM reset is invoked by the guest. But,
instead of capture-replay we could just exit to userspace on an SBI
HSM reset call and let userspace repeat what it did at vcpu-create
time.

> 
> We need to perform a KVM reset even when the VCPU is started through an
> ioctl.  This patch is one of the ways we can achieve it.
> 
> Assume that userspace has no business setting the post-reset state.
> KVM is de-facto the SBI implementation, as the SBI HSM acceleration
> cannot be disabled and userspace cannot control the reset state, so KVM
> should be in full control of the post-reset state.
> 
> Do not reset the pc and a1 registers, because SBI reset is expected to
> provide them and KVM has no idea what these registers should be -- only
> the userspace knows where it put the data.

s/userspace/guest/

> 
> An important consideration is resume.  Userspace might want to start
> with non-reset state.  Check ran_atleast_once to allow this, because
> KVM-SBI HSM creates some VCPUs as STOPPED.
> 
> The drawback is that userspace can still start the boot VCPU with an
> incorrect reset state, because there is no way to distinguish a freshly
> reset new VCPU on the KVM side (userspace might set some values by
> mistake) from a restored VCPU (userspace must set all values).

If there's a correct vs. incorrect reset state that KVM needs to enforce,
then we'll need a different API than just a bunch of set-one-reg calls,
or set/get-one-reg should be WARL for userpace.

> 
> The advantage of this solution is that it fixes current QEMU and makes
> some sense with the assumption that KVM implements SBI HSM.
> I do not like it too much, so I'd be in favor of a different solution if
> we can still afford to drop support for current userspaces.
> 
> For a cleaner solution, we should add interfaces to perform the KVM-SBI
> reset request on userspace demand.

That's what the change to kvm_arch_vcpu_ioctl_set_mpstate() in this
patch is providing, right?

> I think it would also be much better
> if userspace was in control of the post-reset state.

Agreed. Can we just exit to userspace on SBI HSM reset?

Thanks,
drew

> 
> Signed-off-by: Radim Krčmář <rkrcmar@ventanamicro.com>
> ---
>  arch/riscv/include/asm/kvm_host.h     |  1 +
>  arch/riscv/include/asm/kvm_vcpu_sbi.h |  3 +++
>  arch/riscv/kvm/vcpu.c                 |  9 +++++++++
>  arch/riscv/kvm/vcpu_sbi.c             | 21 +++++++++++++++++++--
>  4 files changed, 32 insertions(+), 2 deletions(-)
> 
> diff --git a/arch/riscv/include/asm/kvm_host.h b/arch/riscv/include/asm/kvm_host.h
> index 0c8c9c05af91..9bbf8c4a286b 100644
> --- a/arch/riscv/include/asm/kvm_host.h
> +++ b/arch/riscv/include/asm/kvm_host.h
> @@ -195,6 +195,7 @@ struct kvm_vcpu_smstateen_csr {
>  
>  struct kvm_vcpu_reset_state {
>  	spinlock_t lock;
> +	bool active;
>  	unsigned long pc;
>  	unsigned long a1;
>  };
> diff --git a/arch/riscv/include/asm/kvm_vcpu_sbi.h b/arch/riscv/include/asm/kvm_vcpu_sbi.h
> index aaaa81355276..2c334a87e02a 100644
> --- a/arch/riscv/include/asm/kvm_vcpu_sbi.h
> +++ b/arch/riscv/include/asm/kvm_vcpu_sbi.h
> @@ -57,6 +57,9 @@ void kvm_riscv_vcpu_sbi_system_reset(struct kvm_vcpu *vcpu,
>  				     u32 type, u64 flags);
>  void kvm_riscv_vcpu_sbi_request_reset(struct kvm_vcpu *vcpu,
>                                        unsigned long pc, unsigned long a1);
> +void __kvm_riscv_vcpu_set_reset_state(struct kvm_vcpu *vcpu,
> +                                      unsigned long pc, unsigned long a1);
> +void kvm_riscv_vcpu_sbi_request_reset_from_userspace(struct kvm_vcpu *vcpu);
>  int kvm_riscv_vcpu_sbi_return(struct kvm_vcpu *vcpu, struct kvm_run *run);
>  int kvm_riscv_vcpu_set_reg_sbi_ext(struct kvm_vcpu *vcpu,
>  				   const struct kvm_one_reg *reg);
> diff --git a/arch/riscv/kvm/vcpu.c b/arch/riscv/kvm/vcpu.c
> index b8485c1c1ce4..4578863a39e3 100644
> --- a/arch/riscv/kvm/vcpu.c
> +++ b/arch/riscv/kvm/vcpu.c
> @@ -58,6 +58,11 @@ static void kvm_riscv_vcpu_context_reset(struct kvm_vcpu *vcpu)
>  	struct kvm_vcpu_reset_state *reset_state = &vcpu->arch.reset_state;
>  	void *vector_datap = cntx->vector.datap;
>  
> +	spin_lock(&reset_state->lock);
> +	if (!reset_state->active)
> +		__kvm_riscv_vcpu_set_reset_state(vcpu, cntx->sepc, cntx->a1);
> +	spin_unlock(&reset_state->lock);
> +
>  	memset(cntx, 0, sizeof(*cntx));
>  	memset(csr, 0, sizeof(*csr));
>  
> @@ -520,6 +525,10 @@ int kvm_arch_vcpu_ioctl_set_mpstate(struct kvm_vcpu *vcpu,
>  
>  	switch (mp_state->mp_state) {
>  	case KVM_MP_STATE_RUNNABLE:
> +		if (riscv_vcpu_supports_sbi_ext(vcpu, KVM_RISCV_SBI_EXT_HSM) &&
> +				vcpu->arch.ran_atleast_once &&
> +				kvm_riscv_vcpu_stopped(vcpu))
> +			kvm_riscv_vcpu_sbi_request_reset_from_userspace(vcpu);
>  		WRITE_ONCE(vcpu->arch.mp_state, *mp_state);
>  		break;
>  	case KVM_MP_STATE_STOPPED:
> diff --git a/arch/riscv/kvm/vcpu_sbi.c b/arch/riscv/kvm/vcpu_sbi.c
> index 3d7955e05cc3..77f9f0bd3842 100644
> --- a/arch/riscv/kvm/vcpu_sbi.c
> +++ b/arch/riscv/kvm/vcpu_sbi.c
> @@ -156,12 +156,29 @@ void kvm_riscv_vcpu_sbi_system_reset(struct kvm_vcpu *vcpu,
>  	run->exit_reason = KVM_EXIT_SYSTEM_EVENT;
>  }
>  
> +/* must be called with held vcpu->arch.reset_state.lock */
> +void __kvm_riscv_vcpu_set_reset_state(struct kvm_vcpu *vcpu,
> +                                      unsigned long pc, unsigned long a1)
> +{
> +	vcpu->arch.reset_state.active = true;
> +	vcpu->arch.reset_state.pc = pc;
> +	vcpu->arch.reset_state.a1 = a1;
> +}
> +
>  void kvm_riscv_vcpu_sbi_request_reset(struct kvm_vcpu *vcpu,
>                                        unsigned long pc, unsigned long a1)
>  {
>  	spin_lock(&vcpu->arch.reset_state.lock);
> -	vcpu->arch.reset_state.pc = pc;
> -	vcpu->arch.reset_state.a1 = a1;
> +	__kvm_riscv_vcpu_set_reset_state(vcpu, pc, a1);
> +	spin_unlock(&vcpu->arch.reset_state.lock);
> +
> +	kvm_make_request(KVM_REQ_VCPU_RESET, vcpu);
> +}
> +
> +void kvm_riscv_vcpu_sbi_request_reset_from_userspace(struct kvm_vcpu *vcpu)
> +{
> +	spin_lock(&vcpu->arch.reset_state.lock);
> +	vcpu->arch.reset_state.active = false;
>  	spin_unlock(&vcpu->arch.reset_state.lock);
>  
>  	kvm_make_request(KVM_REQ_VCPU_RESET, vcpu);
> -- 
> 2.48.1
> 

_______________________________________________
linux-riscv mailing list
linux-riscv@lists.infradead.org
http://lists.infradead.org/mailman/listinfo/linux-riscv

Re: [PATCH 4/5] KVM: RISC-V: reset VCPU state when becoming runnable

[Radim Krčmář]

2025-04-25T15:26:08+02:00, Andrew Jones <ajones@ventanamicro.com>:
> On Thu, Apr 03, 2025 at 01:25:23PM +0200, Radim Krčmář wrote:
>> Beware, this patch is "breaking" the userspace interface, because it
>> fixes a KVM/QEMU bug where the boot VCPU is not being reset by KVM.
>> 
>> The VCPU reset paths are inconsistent right now.  KVM resets VCPUs that
>> are brought up by KVM-accelerated SBI calls, but does nothing for VCPUs
>> brought up through ioctls.
>
> I guess we currently expect userspace to make a series of set-one-reg
> ioctls in order to prepare ("reset") newly created vcpus,

Userspace should currently get-one-reg a freshly reset VCPU to know what
KVM SBI decides is the correct reset.  Userspace shouldn't set-one-reg
anything other than what KVM decides, hence we can currently just let
KVM do it.

>                                                           and I guess
> the problem is that KVM isn't capturing the resulting configuration
> in order to replay it when SBI HSM reset is invoked by the guest.

That can also be a solution, but it's not possible to capture the
desired reset state with current IOCTLs, because the first run of a VCPU
can just as well be a resume from a mid-execution.

>                                                                   But,
> instead of capture-replay we could just exit to userspace on an SBI
> HSM reset call and let userspace repeat what it did at vcpu-create
> time.

Right, I like the idea.  (It doesn't fix current userspaces, though.)

>> We need to perform a KVM reset even when the VCPU is started through an
>> ioctl.  This patch is one of the ways we can achieve it.
>> 
>> Assume that userspace has no business setting the post-reset state.
>> KVM is de-facto the SBI implementation, as the SBI HSM acceleration
>> cannot be disabled and userspace cannot control the reset state, so KVM
>> should be in full control of the post-reset state.
>> 
>> Do not reset the pc and a1 registers, because SBI reset is expected to
>> provide them and KVM has no idea what these registers should be -- only
>> the userspace knows where it put the data.
>
> s/userspace/guest/

Both are correct...  I should have made the context clearer here.
I meant the initial hart boot, where userspace loads code/dt and sets
pc/a1 to them.

>> An important consideration is resume.  Userspace might want to start
>> with non-reset state.  Check ran_atleast_once to allow this, because
>> KVM-SBI HSM creates some VCPUs as STOPPED.
>> 
>> The drawback is that userspace can still start the boot VCPU with an
>> incorrect reset state, because there is no way to distinguish a freshly
>> reset new VCPU on the KVM side (userspace might set some values by
>> mistake) from a restored VCPU (userspace must set all values).
>
> If there's a correct vs. incorrect reset state that KVM needs to enforce,
> then we'll need a different API than just a bunch of set-one-reg calls,
> or set/get-one-reg should be WARL for userpace.

Incorrect might have been too strong word... while the SBI reset state
is technically UNSPECIFIED, I think it's just asking for bugs if the
harts have different initial states based on their reset method.

>    set/get-one-reg should be WARL for userpace.

WAAAA! :)

>> The advantage of this solution is that it fixes current QEMU and makes
>> some sense with the assumption that KVM implements SBI HSM.
>> I do not like it too much, so I'd be in favor of a different solution if
>> we can still afford to drop support for current userspaces.
>> 
>> For a cleaner solution, we should add interfaces to perform the KVM-SBI
>> reset request on userspace demand.
>
> That's what the change to kvm_arch_vcpu_ioctl_set_mpstate() in this
> patch is providing, right?

It does.  With conditions to be as compatible as possible.

>> I think it would also be much better
>> if userspace was in control of the post-reset state.
>
> Agreed. Can we just exit to userspace on SBI HSM reset?

Yes.  (It needs an userspace toggle if we care about
backward-compatiblity, though.)

How much do we want to fix/break current userspaces?

Thanks.

_______________________________________________
linux-riscv mailing list
linux-riscv@lists.infradead.org
http://lists.infradead.org/mailman/listinfo/linux-riscv

Re: [PATCH 3/5] KVM: RISC-V: remove unnecessary SBI reset state

[Anup Patel]

On Thu, Apr 3, 2025 at 5:02 PM Radim Krčmář <rkrcmar@ventanamicro.com> wrote:
>
> The SBI reset state has only two variables -- pc and a1.
> The rest is known, so keep only the necessary information.
>
> The reset structures make sense if we want userspace to control the
> reset state (which we do), but I'd still remove them now and reintroduce
> with the userspace interface later -- we could probably have just a
> single reset state per VM, instead of a reset state for each VCPU.

The SBI spec does not define the reset state of CPUs. The SBI
implementations (aka KVM RISC-V or OpenSBI) or platform
firmwares are free to clear additional registers as part system
reset or CPU.

As part of resetting the VCPU, the in-kernel KVM clears all
the registers.

The setting of PC, A0, and A1 is only an entry condition defined
for CPUs brought-up using SBI HSM start or SBI System suspend.

We should not go ahead with this patch.

Regards,
Anup

>
> Signed-off-by: Radim Krčmář <rkrcmar@ventanamicro.com>
> ---
>  arch/riscv/include/asm/kvm_aia.h  |  3 --
>  arch/riscv/include/asm/kvm_host.h | 12 ++++---
>  arch/riscv/kvm/aia_device.c       |  4 +--
>  arch/riscv/kvm/vcpu.c             | 58 +++++++++++++++++--------------
>  arch/riscv/kvm/vcpu_sbi.c         |  9 +++--
>  5 files changed, 44 insertions(+), 42 deletions(-)
>
> diff --git a/arch/riscv/include/asm/kvm_aia.h b/arch/riscv/include/asm/kvm_aia.h
> index 1f37b600ca47..3b643b9efc07 100644
> --- a/arch/riscv/include/asm/kvm_aia.h
> +++ b/arch/riscv/include/asm/kvm_aia.h
> @@ -63,9 +63,6 @@ struct kvm_vcpu_aia {
>         /* CPU AIA CSR context of Guest VCPU */
>         struct kvm_vcpu_aia_csr guest_csr;
>
> -       /* CPU AIA CSR context upon Guest VCPU reset */
> -       struct kvm_vcpu_aia_csr guest_reset_csr;
> -
>         /* Guest physical address of IMSIC for this VCPU */
>         gpa_t           imsic_addr;
>
> diff --git a/arch/riscv/include/asm/kvm_host.h b/arch/riscv/include/asm/kvm_host.h
> index 0e9c2fab6378..0c8c9c05af91 100644
> --- a/arch/riscv/include/asm/kvm_host.h
> +++ b/arch/riscv/include/asm/kvm_host.h
> @@ -193,6 +193,12 @@ struct kvm_vcpu_smstateen_csr {
>         unsigned long sstateen0;
>  };
>
> +struct kvm_vcpu_reset_state {
> +       spinlock_t lock;
> +       unsigned long pc;
> +       unsigned long a1;
> +};
> +
>  struct kvm_vcpu_arch {
>         /* VCPU ran at least once */
>         bool ran_atleast_once;
> @@ -227,12 +233,8 @@ struct kvm_vcpu_arch {
>         /* CPU Smstateen CSR context of Guest VCPU */
>         struct kvm_vcpu_smstateen_csr smstateen_csr;
>
> -       /* CPU context upon Guest VCPU reset */
> -       struct kvm_cpu_context guest_reset_context;
> -       spinlock_t reset_cntx_lock;
> +       struct kvm_vcpu_reset_state reset_state;
>
> -       /* CPU CSR context upon Guest VCPU reset */
> -       struct kvm_vcpu_csr guest_reset_csr;
>
>         /*
>          * VCPU interrupts
> diff --git a/arch/riscv/kvm/aia_device.c b/arch/riscv/kvm/aia_device.c
> index 39cd26af5a69..43e472ff3e1a 100644
> --- a/arch/riscv/kvm/aia_device.c
> +++ b/arch/riscv/kvm/aia_device.c
> @@ -526,12 +526,10 @@ int kvm_riscv_vcpu_aia_update(struct kvm_vcpu *vcpu)
>  void kvm_riscv_vcpu_aia_reset(struct kvm_vcpu *vcpu)
>  {
>         struct kvm_vcpu_aia_csr *csr = &vcpu->arch.aia_context.guest_csr;
> -       struct kvm_vcpu_aia_csr *reset_csr =
> -                               &vcpu->arch.aia_context.guest_reset_csr;
>
>         if (!kvm_riscv_aia_available())
>                 return;
> -       memcpy(csr, reset_csr, sizeof(*csr));
> +       memset(csr, 0, sizeof(*csr));
>
>         /* Proceed only if AIA was initialized successfully */
>         if (!kvm_riscv_aia_initialized(vcpu->kvm))
> diff --git a/arch/riscv/kvm/vcpu.c b/arch/riscv/kvm/vcpu.c
> index 2fb75288ecfe..b8485c1c1ce4 100644
> --- a/arch/riscv/kvm/vcpu.c
> +++ b/arch/riscv/kvm/vcpu.c
> @@ -51,13 +51,40 @@ const struct kvm_stats_header kvm_vcpu_stats_header = {
>                        sizeof(kvm_vcpu_stats_desc),
>  };
>
> -static void kvm_riscv_reset_vcpu(struct kvm_vcpu *vcpu)
> +static void kvm_riscv_vcpu_context_reset(struct kvm_vcpu *vcpu)
>  {
>         struct kvm_vcpu_csr *csr = &vcpu->arch.guest_csr;
> -       struct kvm_vcpu_csr *reset_csr = &vcpu->arch.guest_reset_csr;
>         struct kvm_cpu_context *cntx = &vcpu->arch.guest_context;
> -       struct kvm_cpu_context *reset_cntx = &vcpu->arch.guest_reset_context;
> +       struct kvm_vcpu_reset_state *reset_state = &vcpu->arch.reset_state;
>         void *vector_datap = cntx->vector.datap;
> +
> +       memset(cntx, 0, sizeof(*cntx));
> +       memset(csr, 0, sizeof(*csr));
> +
> +       /* Restore datap as it's not a part of the guest context. */
> +       cntx->vector.datap = vector_datap;
> +
> +       /* Load SBI reset values */
> +       cntx->a0 = vcpu->vcpu_id;
> +
> +       spin_lock(&reset_state->lock);
> +       cntx->sepc = reset_state->pc;
> +       cntx->a1 = reset_state->a1;
> +       spin_unlock(&reset_state->lock);
> +
> +       /* Setup reset state of shadow SSTATUS and HSTATUS CSRs */
> +       cntx->sstatus = SR_SPP | SR_SPIE;
> +
> +       cntx->hstatus |= HSTATUS_VTW;
> +       cntx->hstatus |= HSTATUS_SPVP;
> +       cntx->hstatus |= HSTATUS_SPV;
> +
> +       /* By default, make CY, TM, and IR counters accessible in VU mode */
> +       csr->scounteren = 0x7;
> +}
> +
> +static void kvm_riscv_reset_vcpu(struct kvm_vcpu *vcpu)
> +{
>         bool loaded;
>
>         /**
> @@ -72,16 +99,10 @@ static void kvm_riscv_reset_vcpu(struct kvm_vcpu *vcpu)
>
>         vcpu->arch.last_exit_cpu = -1;
>
> -       memcpy(csr, reset_csr, sizeof(*csr));
> -
> -       spin_lock(&vcpu->arch.reset_cntx_lock);
> -       memcpy(cntx, reset_cntx, sizeof(*cntx));
> -       spin_unlock(&vcpu->arch.reset_cntx_lock);
> +       kvm_riscv_vcpu_context_reset(vcpu);
>
>         kvm_riscv_vcpu_fp_reset(vcpu);
>
> -       /* Restore datap as it's not a part of the guest context. */
> -       cntx->vector.datap = vector_datap;
>         kvm_riscv_vcpu_vector_reset(vcpu);
>
>         kvm_riscv_vcpu_timer_reset(vcpu);
> @@ -113,8 +134,6 @@ int kvm_arch_vcpu_precreate(struct kvm *kvm, unsigned int id)
>  int kvm_arch_vcpu_create(struct kvm_vcpu *vcpu)
>  {
>         int rc;
> -       struct kvm_cpu_context *cntx;
> -       struct kvm_vcpu_csr *reset_csr = &vcpu->arch.guest_reset_csr;
>
>         spin_lock_init(&vcpu->arch.mp_state_lock);
>
> @@ -134,24 +153,11 @@ int kvm_arch_vcpu_create(struct kvm_vcpu *vcpu)
>         /* Setup VCPU hfence queue */
>         spin_lock_init(&vcpu->arch.hfence_lock);
>
> -       /* Setup reset state of shadow SSTATUS and HSTATUS CSRs */
> -       spin_lock_init(&vcpu->arch.reset_cntx_lock);
> -
> -       spin_lock(&vcpu->arch.reset_cntx_lock);
> -       cntx = &vcpu->arch.guest_reset_context;
> -       cntx->sstatus = SR_SPP | SR_SPIE;
> -       cntx->hstatus = 0;
> -       cntx->hstatus |= HSTATUS_VTW;
> -       cntx->hstatus |= HSTATUS_SPVP;
> -       cntx->hstatus |= HSTATUS_SPV;
> -       spin_unlock(&vcpu->arch.reset_cntx_lock);
> +       spin_lock_init(&vcpu->arch.reset_state.lock);
>
>         if (kvm_riscv_vcpu_alloc_vector_context(vcpu))
>                 return -ENOMEM;
>
> -       /* By default, make CY, TM, and IR counters accessible in VU mode */
> -       reset_csr->scounteren = 0x7;
> -
>         /* Setup VCPU timer */
>         kvm_riscv_vcpu_timer_init(vcpu);
>
> diff --git a/arch/riscv/kvm/vcpu_sbi.c b/arch/riscv/kvm/vcpu_sbi.c
> index f58368f7df1d..3d7955e05cc3 100644
> --- a/arch/riscv/kvm/vcpu_sbi.c
> +++ b/arch/riscv/kvm/vcpu_sbi.c
> @@ -159,11 +159,10 @@ void kvm_riscv_vcpu_sbi_system_reset(struct kvm_vcpu *vcpu,
>  void kvm_riscv_vcpu_sbi_request_reset(struct kvm_vcpu *vcpu,
>                                        unsigned long pc, unsigned long a1)
>  {
> -       spin_lock(&vcpu->arch.reset_cntx_lock);
> -       vcpu->arch.guest_reset_context.sepc = pc;
> -       vcpu->arch.guest_reset_context.a0 = vcpu->vcpu_id;
> -       vcpu->arch.guest_reset_context.a1 = a1;
> -       spin_unlock(&vcpu->arch.reset_cntx_lock);
> +       spin_lock(&vcpu->arch.reset_state.lock);
> +       vcpu->arch.reset_state.pc = pc;
> +       vcpu->arch.reset_state.a1 = a1;
> +       spin_unlock(&vcpu->arch.reset_state.lock);
>
>         kvm_make_request(KVM_REQ_VCPU_RESET, vcpu);
>  }
> --
> 2.48.1
>

_______________________________________________
linux-riscv mailing list
linux-riscv@lists.infradead.org
http://lists.infradead.org/mailman/listinfo/linux-riscv

Re: [PATCH 4/5] KVM: RISC-V: reset VCPU state when becoming runnable

[Anup Patel]

On Thu, Apr 3, 2025 at 5:02 PM Radim Krčmář <rkrcmar@ventanamicro.com> wrote:
>
> Beware, this patch is "breaking" the userspace interface, because it
> fixes a KVM/QEMU bug where the boot VCPU is not being reset by KVM.
>
> The VCPU reset paths are inconsistent right now.  KVM resets VCPUs that
> are brought up by KVM-accelerated SBI calls, but does nothing for VCPUs
> brought up through ioctls.
>
> We need to perform a KVM reset even when the VCPU is started through an
> ioctl.  This patch is one of the ways we can achieve it.
>
> Assume that userspace has no business setting the post-reset state.
> KVM is de-facto the SBI implementation, as the SBI HSM acceleration
> cannot be disabled and userspace cannot control the reset state, so KVM
> should be in full control of the post-reset state.
>
> Do not reset the pc and a1 registers, because SBI reset is expected to
> provide them and KVM has no idea what these registers should be -- only
> the userspace knows where it put the data.
>
> An important consideration is resume.  Userspace might want to start
> with non-reset state.  Check ran_atleast_once to allow this, because
> KVM-SBI HSM creates some VCPUs as STOPPED.
>
> The drawback is that userspace can still start the boot VCPU with an
> incorrect reset state, because there is no way to distinguish a freshly
> reset new VCPU on the KVM side (userspace might set some values by
> mistake) from a restored VCPU (userspace must set all values).
>
> The advantage of this solution is that it fixes current QEMU and makes
> some sense with the assumption that KVM implements SBI HSM.
> I do not like it too much, so I'd be in favor of a different solution if
> we can still afford to drop support for current userspaces.
>
> For a cleaner solution, we should add interfaces to perform the KVM-SBI
> reset request on userspace demand.  I think it would also be much better
> if userspace was in control of the post-reset state.

Apart from breaking KVM user-space, this patch is incorrect and
does not align with the:
1) SBI spec
2) OS boot protocol.

The SBI spec only defines the entry state of certain CPU registers
(namely, PC, A0, and A1) when CPU enters S-mode:
1) Upon SBI HSM start call from some other CPU
2) Upon resuming from non-retentive SBI HSM suspend or
    SBI system suspend

The S-mode entry state of the boot CPU is defined by the
OS boot protocol and not by the SBI spec. Due to this, reason
KVM RISC-V expects user-space to set up the S-mode entry
state of the boot CPU upon system reset.

Due to above, reasons we should not go ahead with this patch.

Regards,
Anup

>
> Signed-off-by: Radim Krčmář <rkrcmar@ventanamicro.com>
> ---
>  arch/riscv/include/asm/kvm_host.h     |  1 +
>  arch/riscv/include/asm/kvm_vcpu_sbi.h |  3 +++
>  arch/riscv/kvm/vcpu.c                 |  9 +++++++++
>  arch/riscv/kvm/vcpu_sbi.c             | 21 +++++++++++++++++++--
>  4 files changed, 32 insertions(+), 2 deletions(-)
>
> diff --git a/arch/riscv/include/asm/kvm_host.h b/arch/riscv/include/asm/kvm_host.h
> index 0c8c9c05af91..9bbf8c4a286b 100644
> --- a/arch/riscv/include/asm/kvm_host.h
> +++ b/arch/riscv/include/asm/kvm_host.h
> @@ -195,6 +195,7 @@ struct kvm_vcpu_smstateen_csr {
>
>  struct kvm_vcpu_reset_state {
>         spinlock_t lock;
> +       bool active;
>         unsigned long pc;
>         unsigned long a1;
>  };
> diff --git a/arch/riscv/include/asm/kvm_vcpu_sbi.h b/arch/riscv/include/asm/kvm_vcpu_sbi.h
> index aaaa81355276..2c334a87e02a 100644
> --- a/arch/riscv/include/asm/kvm_vcpu_sbi.h
> +++ b/arch/riscv/include/asm/kvm_vcpu_sbi.h
> @@ -57,6 +57,9 @@ void kvm_riscv_vcpu_sbi_system_reset(struct kvm_vcpu *vcpu,
>                                      u32 type, u64 flags);
>  void kvm_riscv_vcpu_sbi_request_reset(struct kvm_vcpu *vcpu,
>                                        unsigned long pc, unsigned long a1);
> +void __kvm_riscv_vcpu_set_reset_state(struct kvm_vcpu *vcpu,
> +                                      unsigned long pc, unsigned long a1);
> +void kvm_riscv_vcpu_sbi_request_reset_from_userspace(struct kvm_vcpu *vcpu);
>  int kvm_riscv_vcpu_sbi_return(struct kvm_vcpu *vcpu, struct kvm_run *run);
>  int kvm_riscv_vcpu_set_reg_sbi_ext(struct kvm_vcpu *vcpu,
>                                    const struct kvm_one_reg *reg);
> diff --git a/arch/riscv/kvm/vcpu.c b/arch/riscv/kvm/vcpu.c
> index b8485c1c1ce4..4578863a39e3 100644
> --- a/arch/riscv/kvm/vcpu.c
> +++ b/arch/riscv/kvm/vcpu.c
> @@ -58,6 +58,11 @@ static void kvm_riscv_vcpu_context_reset(struct kvm_vcpu *vcpu)
>         struct kvm_vcpu_reset_state *reset_state = &vcpu->arch.reset_state;
>         void *vector_datap = cntx->vector.datap;
>
> +       spin_lock(&reset_state->lock);
> +       if (!reset_state->active)
> +               __kvm_riscv_vcpu_set_reset_state(vcpu, cntx->sepc, cntx->a1);
> +       spin_unlock(&reset_state->lock);
> +
>         memset(cntx, 0, sizeof(*cntx));
>         memset(csr, 0, sizeof(*csr));
>
> @@ -520,6 +525,10 @@ int kvm_arch_vcpu_ioctl_set_mpstate(struct kvm_vcpu *vcpu,
>
>         switch (mp_state->mp_state) {
>         case KVM_MP_STATE_RUNNABLE:
> +               if (riscv_vcpu_supports_sbi_ext(vcpu, KVM_RISCV_SBI_EXT_HSM) &&
> +                               vcpu->arch.ran_atleast_once &&
> +                               kvm_riscv_vcpu_stopped(vcpu))
> +                       kvm_riscv_vcpu_sbi_request_reset_from_userspace(vcpu);
>                 WRITE_ONCE(vcpu->arch.mp_state, *mp_state);
>                 break;
>         case KVM_MP_STATE_STOPPED:
> diff --git a/arch/riscv/kvm/vcpu_sbi.c b/arch/riscv/kvm/vcpu_sbi.c
> index 3d7955e05cc3..77f9f0bd3842 100644
> --- a/arch/riscv/kvm/vcpu_sbi.c
> +++ b/arch/riscv/kvm/vcpu_sbi.c
> @@ -156,12 +156,29 @@ void kvm_riscv_vcpu_sbi_system_reset(struct kvm_vcpu *vcpu,
>         run->exit_reason = KVM_EXIT_SYSTEM_EVENT;
>  }
>
> +/* must be called with held vcpu->arch.reset_state.lock */
> +void __kvm_riscv_vcpu_set_reset_state(struct kvm_vcpu *vcpu,
> +                                      unsigned long pc, unsigned long a1)
> +{
> +       vcpu->arch.reset_state.active = true;
> +       vcpu->arch.reset_state.pc = pc;
> +       vcpu->arch.reset_state.a1 = a1;
> +}
> +
>  void kvm_riscv_vcpu_sbi_request_reset(struct kvm_vcpu *vcpu,
>                                        unsigned long pc, unsigned long a1)
>  {
>         spin_lock(&vcpu->arch.reset_state.lock);
> -       vcpu->arch.reset_state.pc = pc;
> -       vcpu->arch.reset_state.a1 = a1;
> +       __kvm_riscv_vcpu_set_reset_state(vcpu, pc, a1);
> +       spin_unlock(&vcpu->arch.reset_state.lock);
> +
> +       kvm_make_request(KVM_REQ_VCPU_RESET, vcpu);
> +}
> +
> +void kvm_riscv_vcpu_sbi_request_reset_from_userspace(struct kvm_vcpu *vcpu)
> +{
> +       spin_lock(&vcpu->arch.reset_state.lock);
> +       vcpu->arch.reset_state.active = false;
>         spin_unlock(&vcpu->arch.reset_state.lock);
>
>         kvm_make_request(KVM_REQ_VCPU_RESET, vcpu);
> --
> 2.48.1
>

_______________________________________________
linux-riscv mailing list
linux-riscv@lists.infradead.org
http://lists.infradead.org/mailman/listinfo/linux-riscv

Re: [PATCH 4/5] KVM: RISC-V: reset VCPU state when becoming runnable

[Radim Krčmář]

2025-04-28T17:52:25+05:30, Anup Patel <anup@brainfault.org>:
> On Thu, Apr 3, 2025 at 5:02 PM Radim Krčmář <rkrcmar@ventanamicro.com> wrote:
>> For a cleaner solution, we should add interfaces to perform the KVM-SBI
>> reset request on userspace demand.  I think it would also be much better
>> if userspace was in control of the post-reset state.
>
> Apart from breaking KVM user-space, this patch is incorrect and
> does not align with the:
> 1) SBI spec
> 2) OS boot protocol.
>
> The SBI spec only defines the entry state of certain CPU registers
> (namely, PC, A0, and A1) when CPU enters S-mode:
> 1) Upon SBI HSM start call from some other CPU
> 2) Upon resuming from non-retentive SBI HSM suspend or
>     SBI system suspend
>
> The S-mode entry state of the boot CPU is defined by the
> OS boot protocol and not by the SBI spec. Due to this, reason
> KVM RISC-V expects user-space to set up the S-mode entry
> state of the boot CPU upon system reset.

We can handle the initial state consistency in other patches.
What needs addressing is a way to trigger the KVM reset from userspace,
even if only to clear the internal KVM state.

I think mp_state is currently the best signalization that KVM should
reset, so I added it there.

What would be your preferred interface for that?

Thanks.


_______________________________________________
linux-riscv mailing list
linux-riscv@lists.infradead.org
http://lists.infradead.org/mailman/listinfo/linux-riscv

Re: [PATCH 3/5] KVM: RISC-V: remove unnecessary SBI reset state

[Radim Krčmář]

2025-04-28T17:46:01+05:30, Anup Patel <anup@brainfault.org>:
> On Thu, Apr 3, 2025 at 5:02 PM Radim Krčmář <rkrcmar@ventanamicro.com> wrote:
>>
>> The SBI reset state has only two variables -- pc and a1.
>> The rest is known, so keep only the necessary information.
>>
>> The reset structures make sense if we want userspace to control the
>> reset state (which we do), but I'd still remove them now and reintroduce
>> with the userspace interface later -- we could probably have just a
>> single reset state per VM, instead of a reset state for each VCPU.
>
> The SBI spec does not define the reset state of CPUs. The SBI
> implementations (aka KVM RISC-V or OpenSBI) or platform
> firmwares are free to clear additional registers as part system
> reset or CPU.
>
> As part of resetting the VCPU, the in-kernel KVM clears all
> the registers.

Yes, but instead of doing a simple memset(0), KVM carriers around a lot
of data with minimal information value.  Reset is not really a fast
path, so I think it would be good to have the code there as simple as
possible.

> The setting of PC, A0, and A1 is only an entry condition defined
> for CPUs brought-up using SBI HSM start or SBI System suspend.

That is why this patch has to add kvm_vcpu_reset_state, to remember the
state of pc and a1.  (a0 is hart id and can be figured out.)

> We should not go ahead with this patch.

This patch only does refactoring.  Do you think the current reset
structures are better?

Thanks.

_______________________________________________
linux-riscv mailing list
linux-riscv@lists.infradead.org
http://lists.infradead.org/mailman/listinfo/linux-riscv

Re: [PATCH 3/5] KVM: RISC-V: remove unnecessary SBI reset state

[Anup Patel]

On Mon, Apr 28, 2025 at 11:31 PM Radim Krčmář <rkrcmar@ventanamicro.com> wrote:
>
> 2025-04-28T17:46:01+05:30, Anup Patel <anup@brainfault.org>:
> > On Thu, Apr 3, 2025 at 5:02 PM Radim Krčmář <rkrcmar@ventanamicro.com> wrote:
> >>
> >> The SBI reset state has only two variables -- pc and a1.
> >> The rest is known, so keep only the necessary information.
> >>
> >> The reset structures make sense if we want userspace to control the
> >> reset state (which we do), but I'd still remove them now and reintroduce
> >> with the userspace interface later -- we could probably have just a
> >> single reset state per VM, instead of a reset state for each VCPU.
> >
> > The SBI spec does not define the reset state of CPUs. The SBI
> > implementations (aka KVM RISC-V or OpenSBI) or platform
> > firmwares are free to clear additional registers as part system
> > reset or CPU.
> >
> > As part of resetting the VCPU, the in-kernel KVM clears all
> > the registers.
>
> Yes, but instead of doing a simple memset(0), KVM carriers around a lot
> of data with minimal information value.  Reset is not really a fast
> path, so I think it would be good to have the code there as simple as
> possible.
>
> > The setting of PC, A0, and A1 is only an entry condition defined
> > for CPUs brought-up using SBI HSM start or SBI System suspend.
>
> That is why this patch has to add kvm_vcpu_reset_state, to remember the
> state of pc and a1.  (a0 is hart id and can be figured out.)
>
> > We should not go ahead with this patch.
>
> This patch only does refactoring.  Do you think the current reset
> structures are better?
>

I am fine getting rid of reset structures as long as we have
memset(0) in-place to achieve the same thing.

Regards,
Anup

_______________________________________________
linux-riscv mailing list
linux-riscv@lists.infradead.org
http://lists.infradead.org/mailman/listinfo/linux-riscv

Re: [PATCH 4/5] KVM: RISC-V: reset VCPU state when becoming runnable

[Anup Patel]

On Mon, Apr 28, 2025 at 11:15 PM Radim Krčmář <rkrcmar@ventanamicro.com> wrote:
>
> 2025-04-28T17:52:25+05:30, Anup Patel <anup@brainfault.org>:
> > On Thu, Apr 3, 2025 at 5:02 PM Radim Krčmář <rkrcmar@ventanamicro.com> wrote:
> >> For a cleaner solution, we should add interfaces to perform the KVM-SBI
> >> reset request on userspace demand.  I think it would also be much better
> >> if userspace was in control of the post-reset state.
> >
> > Apart from breaking KVM user-space, this patch is incorrect and
> > does not align with the:
> > 1) SBI spec
> > 2) OS boot protocol.
> >
> > The SBI spec only defines the entry state of certain CPU registers
> > (namely, PC, A0, and A1) when CPU enters S-mode:
> > 1) Upon SBI HSM start call from some other CPU
> > 2) Upon resuming from non-retentive SBI HSM suspend or
> >     SBI system suspend
> >
> > The S-mode entry state of the boot CPU is defined by the
> > OS boot protocol and not by the SBI spec. Due to this, reason
> > KVM RISC-V expects user-space to set up the S-mode entry
> > state of the boot CPU upon system reset.
>
> We can handle the initial state consistency in other patches.
> What needs addressing is a way to trigger the KVM reset from userspace,
> even if only to clear the internal KVM state.
>
> I think mp_state is currently the best signalization that KVM should
> reset, so I added it there.
>
> What would be your preferred interface for that?
>

Instead of creating a new interface, I would prefer that VCPU
which initiates SBI System Reset should be resetted immediately
in-kernel space before forwarding the system reset request to
user space. This way we also force KVM user-space to explicitly
set the PC, A0, and A1 before running the VCPU again after
system reset.

Regards,
Anup

_______________________________________________
linux-riscv mailing list
linux-riscv@lists.infradead.org
http://lists.infradead.org/mailman/listinfo/linux-riscv

Re: [PATCH 4/5] KVM: RISC-V: reset VCPU state when becoming runnable

[Radim Krčmář]

2025-04-29T11:25:35+05:30, Anup Patel <apatel@ventanamicro.com>:
> On Mon, Apr 28, 2025 at 11:15 PM Radim Krčmář <rkrcmar@ventanamicro.com> wrote:
>>
>> 2025-04-28T17:52:25+05:30, Anup Patel <anup@brainfault.org>:
>> > On Thu, Apr 3, 2025 at 5:02 PM Radim Krčmář <rkrcmar@ventanamicro.com> wrote:
>> >> For a cleaner solution, we should add interfaces to perform the KVM-SBI
>> >> reset request on userspace demand.  I think it would also be much better
>> >> if userspace was in control of the post-reset state.
>> >
>> > Apart from breaking KVM user-space, this patch is incorrect and
>> > does not align with the:
>> > 1) SBI spec
>> > 2) OS boot protocol.
>> >
>> > The SBI spec only defines the entry state of certain CPU registers
>> > (namely, PC, A0, and A1) when CPU enters S-mode:
>> > 1) Upon SBI HSM start call from some other CPU
>> > 2) Upon resuming from non-retentive SBI HSM suspend or
>> >     SBI system suspend
>> >
>> > The S-mode entry state of the boot CPU is defined by the
>> > OS boot protocol and not by the SBI spec. Due to this, reason
>> > KVM RISC-V expects user-space to set up the S-mode entry
>> > state of the boot CPU upon system reset.
>>
>> We can handle the initial state consistency in other patches.
>> What needs addressing is a way to trigger the KVM reset from userspace,
>> even if only to clear the internal KVM state.
>>
>> I think mp_state is currently the best signalization that KVM should
>> reset, so I added it there.
>>
>> What would be your preferred interface for that?
>>
>
> Instead of creating a new interface, I would prefer that VCPU
> which initiates SBI System Reset should be resetted immediately
> in-kernel space before forwarding the system reset request to
> user space.

The initiating VCPU might not be the boot VCPU.
It would be safer to reset all of them.

You also previously mentioned that we need to preserve the pre-reset
state for userspace, which I completely agree with and it is why the
reset happens later.

>             This way we also force KVM user-space to explicitly
> set the PC, A0, and A1 before running the VCPU again after
> system reset.

We also want to consider reset from emulation outside of KVM.

There is a "simple" solution that covers everything (except speed) --
the userspace can tear down the whole VM and re-create it.
Do we want to do this instead and drop all resets from KVM?

_______________________________________________
linux-riscv mailing list
linux-riscv@lists.infradead.org
http://lists.infradead.org/mailman/listinfo/linux-riscv

Re: [PATCH 4/5] KVM: RISC-V: reset VCPU state when becoming runnable

[Anup Patel]

On Tue, Apr 29, 2025 at 3:55 PM Radim Krčmář <rkrcmar@ventanamicro.com> wrote:
>
> 2025-04-29T11:25:35+05:30, Anup Patel <apatel@ventanamicro.com>:
> > On Mon, Apr 28, 2025 at 11:15 PM Radim Krčmář <rkrcmar@ventanamicro.com> wrote:
> >>
> >> 2025-04-28T17:52:25+05:30, Anup Patel <anup@brainfault.org>:
> >> > On Thu, Apr 3, 2025 at 5:02 PM Radim Krčmář <rkrcmar@ventanamicro.com> wrote:
> >> >> For a cleaner solution, we should add interfaces to perform the KVM-SBI
> >> >> reset request on userspace demand.  I think it would also be much better
> >> >> if userspace was in control of the post-reset state.
> >> >
> >> > Apart from breaking KVM user-space, this patch is incorrect and
> >> > does not align with the:
> >> > 1) SBI spec
> >> > 2) OS boot protocol.
> >> >
> >> > The SBI spec only defines the entry state of certain CPU registers
> >> > (namely, PC, A0, and A1) when CPU enters S-mode:
> >> > 1) Upon SBI HSM start call from some other CPU
> >> > 2) Upon resuming from non-retentive SBI HSM suspend or
> >> >     SBI system suspend
> >> >
> >> > The S-mode entry state of the boot CPU is defined by the
> >> > OS boot protocol and not by the SBI spec. Due to this, reason
> >> > KVM RISC-V expects user-space to set up the S-mode entry
> >> > state of the boot CPU upon system reset.
> >>
> >> We can handle the initial state consistency in other patches.
> >> What needs addressing is a way to trigger the KVM reset from userspace,
> >> even if only to clear the internal KVM state.
> >>
> >> I think mp_state is currently the best signalization that KVM should
> >> reset, so I added it there.
> >>
> >> What would be your preferred interface for that?
> >>
> >
> > Instead of creating a new interface, I would prefer that VCPU
> > which initiates SBI System Reset should be resetted immediately
> > in-kernel space before forwarding the system reset request to
> > user space.
>
> The initiating VCPU might not be the boot VCPU.
> It would be safer to reset all of them.

I meant initiating VCPU and not the boot VCPU. Currently, the
non-initiating VCPUs are already resetted by VCPU requests
so nothing special needs to be done.

>
> You also previously mentioned that we need to preserve the pre-reset
> state for userspace, which I completely agree with and it is why the
> reset happens later.

Yes, that was only for debug purposes from user space. At the
moment, there is no one using this for debug purposes so we
can sacrifice that.

>
> >             This way we also force KVM user-space to explicitly
> > set the PC, A0, and A1 before running the VCPU again after
> > system reset.
>
> We also want to consider reset from emulation outside of KVM.
>
> There is a "simple" solution that covers everything (except speed) --
> the userspace can tear down the whole VM and re-create it.
> Do we want to do this instead and drop all resets from KVM?

I think we should keep the VCPU resets in KVM so that handling
of system reset handling in user space remains simple. The user
space can also re-create the VM upon system reset but that is
user space choice.

Regards,
Anup

_______________________________________________
linux-riscv mailing list
linux-riscv@lists.infradead.org
http://lists.infradead.org/mailman/listinfo/linux-riscv

Re: [PATCH 4/5] KVM: RISC-V: reset VCPU state when becoming runnable

[Radim Krčmář]

2025-04-29T20:31:18+05:30, Anup Patel <anup@brainfault.org>:
> On Tue, Apr 29, 2025 at 3:55 PM Radim Krčmář <rkrcmar@ventanamicro.com> wrote:
>>
>> 2025-04-29T11:25:35+05:30, Anup Patel <apatel@ventanamicro.com>:
>> > On Mon, Apr 28, 2025 at 11:15 PM Radim Krčmář <rkrcmar@ventanamicro.com> wrote:
>> >>
>> >> 2025-04-28T17:52:25+05:30, Anup Patel <anup@brainfault.org>:
>> >> > On Thu, Apr 3, 2025 at 5:02 PM Radim Krčmář <rkrcmar@ventanamicro.com> wrote:
>> >> >> For a cleaner solution, we should add interfaces to perform the KVM-SBI
>> >> >> reset request on userspace demand.  I think it would also be much better
>> >> >> if userspace was in control of the post-reset state.
>> >> >
>> >> > Apart from breaking KVM user-space, this patch is incorrect and
>> >> > does not align with the:
>> >> > 1) SBI spec
>> >> > 2) OS boot protocol.
>> >> >
>> >> > The SBI spec only defines the entry state of certain CPU registers
>> >> > (namely, PC, A0, and A1) when CPU enters S-mode:
>> >> > 1) Upon SBI HSM start call from some other CPU
>> >> > 2) Upon resuming from non-retentive SBI HSM suspend or
>> >> >     SBI system suspend
>> >> >
>> >> > The S-mode entry state of the boot CPU is defined by the
>> >> > OS boot protocol and not by the SBI spec. Due to this, reason
>> >> > KVM RISC-V expects user-space to set up the S-mode entry
>> >> > state of the boot CPU upon system reset.
>> >>
>> >> We can handle the initial state consistency in other patches.
>> >> What needs addressing is a way to trigger the KVM reset from userspace,
>> >> even if only to clear the internal KVM state.
>> >>
>> >> I think mp_state is currently the best signalization that KVM should
>> >> reset, so I added it there.
>> >>
>> >> What would be your preferred interface for that?
>> >>
>> >
>> > Instead of creating a new interface, I would prefer that VCPU
>> > which initiates SBI System Reset should be resetted immediately
>> > in-kernel space before forwarding the system reset request to
>> > user space.
>>
>> The initiating VCPU might not be the boot VCPU.
>> It would be safer to reset all of them.
>
> I meant initiating VCPU and not the boot VCPU. Currently, the
> non-initiating VCPUs are already resetted by VCPU requests
> so nothing special needs to be done.

Currently, we make the request only for VCPUs brought up by HSM -- the
non-boot VCPUs.  There is a single VCPU not being reset and resetting
the reset initiating VCPU changes nothing. e.g.

  1) VCPU 1 initiates the reset through an ecall.
  2) All VCPUs are stopped and return to userspace.
  3) Userspace prepares VCPU 0 as the boot VCPU.
  4) VCPU 0 executes without going through KVM reset paths.

The point of this patch is to reset the boot VCPU, so we reset the VCPU
that is made runnable by the KVM_SET_MP_STATE IOCTL.

For design alternatives, it is also possible to reset immediately in an
IOCTL instead of making the reset request.

>> You also previously mentioned that we need to preserve the pre-reset
>> state for userspace, which I completely agree with and it is why the
>> reset happens later.
>
> Yes, that was only for debug purposes from user space. At the
> moment, there is no one using this for debug purposes so we
> can sacrifice that.

We still can't immediately reset the boot VCPU, because it might already
be in userspace.  We don't really benefit from immediately resetting the
initiating VCPU.
Also, making the reset request for all VCPUs from the initiating VCPU
has some undesirable race conditions we would have to prevent, so I do
prefer we go the IOCTL reset way.

>> >             This way we also force KVM user-space to explicitly
>> > set the PC, A0, and A1 before running the VCPU again after
>> > system reset.
>>
>> We also want to consider reset from emulation outside of KVM.
>>
>> There is a "simple" solution that covers everything (except speed) --
>> the userspace can tear down the whole VM and re-create it.
>> Do we want to do this instead and drop all resets from KVM?
>
> I think we should keep the VCPU resets in KVM so that handling
> of system reset handling in user space remains simple. The user
> space can also re-create the VM upon system reset but that is
> user space choice.

Ok.

_______________________________________________
linux-riscv mailing list
linux-riscv@lists.infradead.org
http://lists.infradead.org/mailman/listinfo/linux-riscv

Re: [PATCH 4/5] KVM: RISC-V: reset VCPU state when becoming runnable

[Anup Patel]

On Tue, Apr 29, 2025 at 9:51 PM Radim Krčmář <rkrcmar@ventanamicro.com> wrote:
>
> 2025-04-29T20:31:18+05:30, Anup Patel <anup@brainfault.org>:
> > On Tue, Apr 29, 2025 at 3:55 PM Radim Krčmář <rkrcmar@ventanamicro.com> wrote:
> >>
> >> 2025-04-29T11:25:35+05:30, Anup Patel <apatel@ventanamicro.com>:
> >> > On Mon, Apr 28, 2025 at 11:15 PM Radim Krčmář <rkrcmar@ventanamicro.com> wrote:
> >> >>
> >> >> 2025-04-28T17:52:25+05:30, Anup Patel <anup@brainfault.org>:
> >> >> > On Thu, Apr 3, 2025 at 5:02 PM Radim Krčmář <rkrcmar@ventanamicro.com> wrote:
> >> >> >> For a cleaner solution, we should add interfaces to perform the KVM-SBI
> >> >> >> reset request on userspace demand.  I think it would also be much better
> >> >> >> if userspace was in control of the post-reset state.
> >> >> >
> >> >> > Apart from breaking KVM user-space, this patch is incorrect and
> >> >> > does not align with the:
> >> >> > 1) SBI spec
> >> >> > 2) OS boot protocol.
> >> >> >
> >> >> > The SBI spec only defines the entry state of certain CPU registers
> >> >> > (namely, PC, A0, and A1) when CPU enters S-mode:
> >> >> > 1) Upon SBI HSM start call from some other CPU
> >> >> > 2) Upon resuming from non-retentive SBI HSM suspend or
> >> >> >     SBI system suspend
> >> >> >
> >> >> > The S-mode entry state of the boot CPU is defined by the
> >> >> > OS boot protocol and not by the SBI spec. Due to this, reason
> >> >> > KVM RISC-V expects user-space to set up the S-mode entry
> >> >> > state of the boot CPU upon system reset.
> >> >>
> >> >> We can handle the initial state consistency in other patches.
> >> >> What needs addressing is a way to trigger the KVM reset from userspace,
> >> >> even if only to clear the internal KVM state.
> >> >>
> >> >> I think mp_state is currently the best signalization that KVM should
> >> >> reset, so I added it there.
> >> >>
> >> >> What would be your preferred interface for that?
> >> >>
> >> >
> >> > Instead of creating a new interface, I would prefer that VCPU
> >> > which initiates SBI System Reset should be resetted immediately
> >> > in-kernel space before forwarding the system reset request to
> >> > user space.
> >>
> >> The initiating VCPU might not be the boot VCPU.
> >> It would be safer to reset all of them.
> >
> > I meant initiating VCPU and not the boot VCPU. Currently, the
> > non-initiating VCPUs are already resetted by VCPU requests
> > so nothing special needs to be done.

There is no designated boot VCPU for KVM so let us only use the
term "initiating" or "non-initiating" VCPUs in context of system reset.

>
> Currently, we make the request only for VCPUs brought up by HSM -- the
> non-boot VCPUs.  There is a single VCPU not being reset and resetting
> the reset initiating VCPU changes nothing. e.g.
>
>   1) VCPU 1 initiates the reset through an ecall.
>   2) All VCPUs are stopped and return to userspace.

When all VCPUs are stopped, all VCPUs except VCPU1
(in this example) will SLEEP because we do
"kvm_make_all_cpus_request(vcpu->kvm, KVM_REQ_SLEEP)"
so none of the VCPUs except VCPU1 (in this case) will
return to userspace.

>   3) Userspace prepares VCPU 0 as the boot VCPU.
>   4) VCPU 0 executes without going through KVM reset paths.

Userspace will see a system reset event exit for the
initiating VCPU by that time all other VCPUs are already
sleeping with mp_state == KVM_MP_STATE_STOPPED.

>
> The point of this patch is to reset the boot VCPU, so we reset the VCPU
> that is made runnable by the KVM_SET_MP_STATE IOCTL.

Like I said before, we don't need to do this. The initiating VCPU
can be resetted just before exiting to user space for system reset
event exit.

>
> For design alternatives, it is also possible to reset immediately in an
> IOCTL instead of making the reset request.
>
> >> You also previously mentioned that we need to preserve the pre-reset
> >> state for userspace, which I completely agree with and it is why the
> >> reset happens later.
> >
> > Yes, that was only for debug purposes from user space. At the
> > moment, there is no one using this for debug purposes so we
> > can sacrifice that.
>
> We still can't immediately reset the boot VCPU, because it might already
> be in userspace.  We don't really benefit from immediately resetting the
> initiating VCPU.
> Also, making the reset request for all VCPUs from the initiating VCPU
> has some undesirable race conditions we would have to prevent, so I do
> prefer we go the IOCTL reset way.

All VCPUs are sleeping with mp_state == KVM_MP_STATE_STOPPED
when userspace sees system reset exit on the initiating VCPU so I don't
see any race condition if we also reset the initiating VCPU before exiting
to userspace.

>
> >> >             This way we also force KVM user-space to explicitly
> >> > set the PC, A0, and A1 before running the VCPU again after
> >> > system reset.
> >>
> >> We also want to consider reset from emulation outside of KVM.
> >>
> >> There is a "simple" solution that covers everything (except speed) --
> >> the userspace can tear down the whole VM and re-create it.
> >> Do we want to do this instead and drop all resets from KVM?
> >
> > I think we should keep the VCPU resets in KVM so that handling
> > of system reset handling in user space remains simple. The user
> > space can also re-create the VM upon system reset but that is
> > user space choice.
>
> Ok.

Regards,
Anup

_______________________________________________
linux-riscv mailing list
linux-riscv@lists.infradead.org
http://lists.infradead.org/mailman/listinfo/linux-riscv

Re: [PATCH 4/5] KVM: RISC-V: reset VCPU state when becoming runnable

[Anup Patel]

On Wed, Apr 30, 2025 at 9:52 AM Anup Patel <anup@brainfault.org> wrote:
>
> On Tue, Apr 29, 2025 at 9:51 PM Radim Krčmář <rkrcmar@ventanamicro.com> wrote:
> >
> > 2025-04-29T20:31:18+05:30, Anup Patel <anup@brainfault.org>:
> > > On Tue, Apr 29, 2025 at 3:55 PM Radim Krčmář <rkrcmar@ventanamicro.com> wrote:
> > >>
> > >> 2025-04-29T11:25:35+05:30, Anup Patel <apatel@ventanamicro.com>:
> > >> > On Mon, Apr 28, 2025 at 11:15 PM Radim Krčmář <rkrcmar@ventanamicro.com> wrote:
> > >> >>
> > >> >> 2025-04-28T17:52:25+05:30, Anup Patel <anup@brainfault.org>:
> > >> >> > On Thu, Apr 3, 2025 at 5:02 PM Radim Krčmář <rkrcmar@ventanamicro.com> wrote:
> > >> >> >> For a cleaner solution, we should add interfaces to perform the KVM-SBI
> > >> >> >> reset request on userspace demand.  I think it would also be much better
> > >> >> >> if userspace was in control of the post-reset state.
> > >> >> >
> > >> >> > Apart from breaking KVM user-space, this patch is incorrect and
> > >> >> > does not align with the:
> > >> >> > 1) SBI spec
> > >> >> > 2) OS boot protocol.
> > >> >> >
> > >> >> > The SBI spec only defines the entry state of certain CPU registers
> > >> >> > (namely, PC, A0, and A1) when CPU enters S-mode:
> > >> >> > 1) Upon SBI HSM start call from some other CPU
> > >> >> > 2) Upon resuming from non-retentive SBI HSM suspend or
> > >> >> >     SBI system suspend
> > >> >> >
> > >> >> > The S-mode entry state of the boot CPU is defined by the
> > >> >> > OS boot protocol and not by the SBI spec. Due to this, reason
> > >> >> > KVM RISC-V expects user-space to set up the S-mode entry
> > >> >> > state of the boot CPU upon system reset.
> > >> >>
> > >> >> We can handle the initial state consistency in other patches.
> > >> >> What needs addressing is a way to trigger the KVM reset from userspace,
> > >> >> even if only to clear the internal KVM state.
> > >> >>
> > >> >> I think mp_state is currently the best signalization that KVM should
> > >> >> reset, so I added it there.
> > >> >>
> > >> >> What would be your preferred interface for that?
> > >> >>
> > >> >
> > >> > Instead of creating a new interface, I would prefer that VCPU
> > >> > which initiates SBI System Reset should be resetted immediately
> > >> > in-kernel space before forwarding the system reset request to
> > >> > user space.
> > >>
> > >> The initiating VCPU might not be the boot VCPU.
> > >> It would be safer to reset all of them.
> > >
> > > I meant initiating VCPU and not the boot VCPU. Currently, the
> > > non-initiating VCPUs are already resetted by VCPU requests
> > > so nothing special needs to be done.
>
> There is no designated boot VCPU for KVM so let us only use the
> term "initiating" or "non-initiating" VCPUs in context of system reset.
>
> >
> > Currently, we make the request only for VCPUs brought up by HSM -- the
> > non-boot VCPUs.  There is a single VCPU not being reset and resetting
> > the reset initiating VCPU changes nothing. e.g.
> >
> >   1) VCPU 1 initiates the reset through an ecall.
> >   2) All VCPUs are stopped and return to userspace.
>
> When all VCPUs are stopped, all VCPUs except VCPU1
> (in this example) will SLEEP because we do
> "kvm_make_all_cpus_request(vcpu->kvm, KVM_REQ_SLEEP)"
> so none of the VCPUs except VCPU1 (in this case) will
> return to userspace.
>
> >   3) Userspace prepares VCPU 0 as the boot VCPU.
> >   4) VCPU 0 executes without going through KVM reset paths.
>
> Userspace will see a system reset event exit for the
> initiating VCPU by that time all other VCPUs are already
> sleeping with mp_state == KVM_MP_STATE_STOPPED.
>
> >
> > The point of this patch is to reset the boot VCPU, so we reset the VCPU
> > that is made runnable by the KVM_SET_MP_STATE IOCTL.
>
> Like I said before, we don't need to do this. The initiating VCPU
> can be resetted just before exiting to user space for system reset
> event exit.
>

Below is what I am suggesting. This change completely removes
dependency of kvm_sbi_hsm_vcpu_start() on "reset" structures.

diff --git a/arch/riscv/include/asm/kvm_host.h
b/arch/riscv/include/asm/kvm_host.h
index 0e9c2fab6378..6bd12469852d 100644
--- a/arch/riscv/include/asm/kvm_host.h
+++ b/arch/riscv/include/asm/kvm_host.h
@@ -396,6 +396,7 @@ int kvm_riscv_vcpu_get_reg(struct kvm_vcpu *vcpu,
 int kvm_riscv_vcpu_set_reg(struct kvm_vcpu *vcpu,
                const struct kvm_one_reg *reg);

+void kvm_riscv_reset_vcpu(struct kvm_vcpu *vcpu);
 int kvm_riscv_vcpu_set_interrupt(struct kvm_vcpu *vcpu, unsigned int irq);
 int kvm_riscv_vcpu_unset_interrupt(struct kvm_vcpu *vcpu, unsigned int irq);
 void kvm_riscv_vcpu_flush_interrupts(struct kvm_vcpu *vcpu);
diff --git a/arch/riscv/kvm/vcpu.c b/arch/riscv/kvm/vcpu.c
index 02635bac91f1..801c6a1a1aef 100644
--- a/arch/riscv/kvm/vcpu.c
+++ b/arch/riscv/kvm/vcpu.c
@@ -51,7 +51,7 @@ const struct kvm_stats_header kvm_vcpu_stats_header = {
                sizeof(kvm_vcpu_stats_desc),
 };

-static void kvm_riscv_reset_vcpu(struct kvm_vcpu *vcpu)
+void kvm_riscv_reset_vcpu(struct kvm_vcpu *vcpu)
 {
     struct kvm_vcpu_csr *csr = &vcpu->arch.guest_csr;
     struct kvm_vcpu_csr *reset_csr = &vcpu->arch.guest_reset_csr;
@@ -689,6 +689,9 @@ static void kvm_riscv_check_vcpu_requests(struct
kvm_vcpu *vcpu)
     struct rcuwait *wait = kvm_arch_vcpu_get_wait(vcpu);

     if (kvm_request_pending(vcpu)) {
+        if (kvm_check_request(KVM_REQ_VCPU_RESET, vcpu))
+            kvm_riscv_reset_vcpu(vcpu);
+
         if (kvm_check_request(KVM_REQ_SLEEP, vcpu)) {
             kvm_vcpu_srcu_read_unlock(vcpu);
             rcuwait_wait_event(wait,
@@ -705,9 +708,6 @@ static void kvm_riscv_check_vcpu_requests(struct
kvm_vcpu *vcpu)
             }
         }

-        if (kvm_check_request(KVM_REQ_VCPU_RESET, vcpu))
-            kvm_riscv_reset_vcpu(vcpu);
-
         if (kvm_check_request(KVM_REQ_UPDATE_HGATP, vcpu))
             kvm_riscv_gstage_update_hgatp(vcpu);

diff --git a/arch/riscv/kvm/vcpu_sbi.c b/arch/riscv/kvm/vcpu_sbi.c
index d1c83a77735e..79477e7f240a 100644
--- a/arch/riscv/kvm/vcpu_sbi.c
+++ b/arch/riscv/kvm/vcpu_sbi.c
@@ -146,9 +146,15 @@ void kvm_riscv_vcpu_sbi_system_reset(struct kvm_vcpu *vcpu,
         spin_lock(&vcpu->arch.mp_state_lock);
         WRITE_ONCE(tmp->arch.mp_state.mp_state, KVM_MP_STATE_STOPPED);
         spin_unlock(&vcpu->arch.mp_state_lock);
+        if (tmp != vcpu) {
+            kvm_make_request(KVM_REQ_SLEEP | KVM_REQ_VCPU_RESET, vcpu);
+            kvm_vcpu_kick(vcpu);
+        } else {
+            kvm_make_request(KVM_REQ_SLEEP, vcpu);
+        }
     }
-    kvm_make_all_cpus_request(vcpu->kvm, KVM_REQ_SLEEP);

+    kvm_riscv_reset_vcpu(vcpu);
     memset(&run->system_event, 0, sizeof(run->system_event));
     run->system_event.type = type;
     run->system_event.ndata = 1;
diff --git a/arch/riscv/kvm/vcpu_sbi_hsm.c b/arch/riscv/kvm/vcpu_sbi_hsm.c
index 3070bb31745d..30d7d59db5a5 100644
--- a/arch/riscv/kvm/vcpu_sbi_hsm.c
+++ b/arch/riscv/kvm/vcpu_sbi_hsm.c
@@ -15,15 +15,15 @@

 static int kvm_sbi_hsm_vcpu_start(struct kvm_vcpu *vcpu)
 {
-    struct kvm_cpu_context *reset_cntx;
-    struct kvm_cpu_context *cp = &vcpu->arch.guest_context;
-    struct kvm_vcpu *target_vcpu;
+    struct kvm_cpu_context *target_cp, *cp = &vcpu->arch.guest_context;
     unsigned long target_vcpuid = cp->a0;
+    struct kvm_vcpu *target_vcpu;
     int ret = 0;

     target_vcpu = kvm_get_vcpu_by_id(vcpu->kvm, target_vcpuid);
     if (!target_vcpu)
         return SBI_ERR_INVALID_PARAM;
+    target_cp = &target_vcpu->arch.guest_context;

     spin_lock(&target_vcpu->arch.mp_state_lock);

@@ -32,17 +32,12 @@ static int kvm_sbi_hsm_vcpu_start(struct kvm_vcpu *vcpu)
         goto out;
     }

-    spin_lock(&target_vcpu->arch.reset_cntx_lock);
-    reset_cntx = &target_vcpu->arch.guest_reset_context;
     /* start address */
-    reset_cntx->sepc = cp->a1;
+    target_cp->sepc = cp->a1;
     /* target vcpu id to start */
-    reset_cntx->a0 = target_vcpuid;
+    target_cp->a0 = target_vcpuid;
     /* private data passed from kernel */
-    reset_cntx->a1 = cp->a2;
-    spin_unlock(&target_vcpu->arch.reset_cntx_lock);
-
-    kvm_make_request(KVM_REQ_VCPU_RESET, target_vcpu);
+    target_cp->a1 = cp->a2;

     __kvm_riscv_vcpu_power_on(target_vcpu);

@@ -63,6 +58,7 @@ static int kvm_sbi_hsm_vcpu_stop(struct kvm_vcpu *vcpu)
         goto out;
     }

+    kvm_make_request(KVM_REQ_VCPU_RESET, vcpu);
     __kvm_riscv_vcpu_power_off(vcpu);

 out:

Regards,
Anup

_______________________________________________
linux-riscv mailing list
linux-riscv@lists.infradead.org
http://lists.infradead.org/mailman/listinfo/linux-riscv

Re: [PATCH 4/5] KVM: RISC-V: reset VCPU state when becoming runnable

[Radim Krčmář]

2025-04-30T10:56:35+05:30, Anup Patel <anup@brainfault.org>:
> On Wed, Apr 30, 2025 at 9:52 AM Anup Patel <anup@brainfault.org> wrote:
>>
>> On Tue, Apr 29, 2025 at 9:51 PM Radim Krčmář <rkrcmar@ventanamicro.com> wrote:
>> >
>> > 2025-04-29T20:31:18+05:30, Anup Patel <anup@brainfault.org>:
>> > > On Tue, Apr 29, 2025 at 3:55 PM Radim Krčmář <rkrcmar@ventanamicro.com> wrote:
>> > >>
>> > >> 2025-04-29T11:25:35+05:30, Anup Patel <apatel@ventanamicro.com>:
>> > >> > On Mon, Apr 28, 2025 at 11:15 PM Radim Krčmář <rkrcmar@ventanamicro.com> wrote:
>> > >> >>
>> > >> >> 2025-04-28T17:52:25+05:30, Anup Patel <anup@brainfault.org>:
>> > >> >> > On Thu, Apr 3, 2025 at 5:02 PM Radim Krčmář <rkrcmar@ventanamicro.com> wrote:
>> > >> >> >> For a cleaner solution, we should add interfaces to perform the KVM-SBI
>> > >> >> >> reset request on userspace demand.  I think it would also be much better
>> > >> >> >> if userspace was in control of the post-reset state.
>> > >> >> >
>> > >> >> > Apart from breaking KVM user-space, this patch is incorrect and
>> > >> >> > does not align with the:
>> > >> >> > 1) SBI spec
>> > >> >> > 2) OS boot protocol.
>> > >> >> >
>> > >> >> > The SBI spec only defines the entry state of certain CPU registers
>> > >> >> > (namely, PC, A0, and A1) when CPU enters S-mode:
>> > >> >> > 1) Upon SBI HSM start call from some other CPU
>> > >> >> > 2) Upon resuming from non-retentive SBI HSM suspend or
>> > >> >> >     SBI system suspend
>> > >> >> >
>> > >> >> > The S-mode entry state of the boot CPU is defined by the
>> > >> >> > OS boot protocol and not by the SBI spec. Due to this, reason
>> > >> >> > KVM RISC-V expects user-space to set up the S-mode entry
>> > >> >> > state of the boot CPU upon system reset.
>> > >> >>
>> > >> >> We can handle the initial state consistency in other patches.
>> > >> >> What needs addressing is a way to trigger the KVM reset from userspace,
>> > >> >> even if only to clear the internal KVM state.
>> > >> >>
>> > >> >> I think mp_state is currently the best signalization that KVM should
>> > >> >> reset, so I added it there.
>> > >> >>
>> > >> >> What would be your preferred interface for that?
>> > >> >>
>> > >> >
>> > >> > Instead of creating a new interface, I would prefer that VCPU
>> > >> > which initiates SBI System Reset should be resetted immediately
>> > >> > in-kernel space before forwarding the system reset request to
>> > >> > user space.
>> > >>
>> > >> The initiating VCPU might not be the boot VCPU.
>> > >> It would be safer to reset all of them.
>> > >
>> > > I meant initiating VCPU and not the boot VCPU. Currently, the
>> > > non-initiating VCPUs are already resetted by VCPU requests
>> > > so nothing special needs to be done.
>>
>> There is no designated boot VCPU for KVM so let us only use the
>> term "initiating" or "non-initiating" VCPUs in context of system reset.

That is exactly how I use it.  Some VCPU will be the boot VCPU (the VCPU
made runnable by KVM_SET_MP_STATE) and loaded with state from userspace.

RISC-V doesn't guarantee that the boot VCPU is the reset initiating
VCPU, so I think KVM should allow it.

>> > Currently, we make the request only for VCPUs brought up by HSM -- the
>> > non-boot VCPUs.  There is a single VCPU not being reset and resetting
>> > the reset initiating VCPU changes nothing. e.g.
>> >
>> >   1) VCPU 1 initiates the reset through an ecall.
>> >   2) All VCPUs are stopped and return to userspace.
>>
>> When all VCPUs are stopped, all VCPUs except VCPU1
>> (in this example) will SLEEP because we do
>> "kvm_make_all_cpus_request(vcpu->kvm, KVM_REQ_SLEEP)"
>> so none of the VCPUs except VCPU1 (in this case) will
>> return to userspace.

Userspace should be able to do whatever it likes -- in my example, all
the VCPUs are brought to userspace and a different boot VCPU is
selected.

(Perhaps userspace wanted to record their reset pre-reset state, or
 maybe it really wants to boot with a designated VCPU.)

>> >   3) Userspace prepares VCPU 0 as the boot VCPU.
>> >   4) VCPU 0 executes without going through KVM reset paths.
>>
>> Userspace will see a system reset event exit for the
>> initiating VCPU by that time all other VCPUs are already
>> sleeping with mp_state == KVM_MP_STATE_STOPPED.
>>
>> >
>> > The point of this patch is to reset the boot VCPU, so we reset the VCPU
>> > that is made runnable by the KVM_SET_MP_STATE IOCTL.
>>
>> Like I said before, we don't need to do this. The initiating VCPU
>> can be resetted just before exiting to user space for system reset
>> event exit.

You assume initiating VCPU == boot VCPU.

We should prevent KVM_SET_MP_STATE IOCTL for all non-initiating VCPUs if
we decide to accept the assumption.

I'd rather choose a different design, though.

How about a new userspace interface for IOCTL reset?
(Can be capability toggle for KVM_SET_MP_STATE or a straight new IOCTL.)

That wouldn't "fix" current userspaces, but would significantly improve
the sanity of the KVM interface.

> Below is what I am suggesting. This change completely removes
> dependency of kvm_sbi_hsm_vcpu_start() on "reset" structures.

I'd keep the reset structure in this series -- it's small enough and
locklessly accessing the state of another VCPU needs a lot of
consideration to prevent all possible race conditions.

Thanks.

_______________________________________________
linux-riscv mailing list
linux-riscv@lists.infradead.org
http://lists.infradead.org/mailman/listinfo/linux-riscv

Re: [PATCH 4/5] KVM: RISC-V: reset VCPU state when becoming runnable

[Anup Patel]

On Wed, Apr 30, 2025 at 1:59 PM Radim Krčmář <rkrcmar@ventanamicro.com> wrote:
>
> 2025-04-30T10:56:35+05:30, Anup Patel <anup@brainfault.org>:
> > On Wed, Apr 30, 2025 at 9:52 AM Anup Patel <anup@brainfault.org> wrote:
> >>
> >> On Tue, Apr 29, 2025 at 9:51 PM Radim Krčmář <rkrcmar@ventanamicro.com> wrote:
> >> >
> >> > 2025-04-29T20:31:18+05:30, Anup Patel <anup@brainfault.org>:
> >> > > On Tue, Apr 29, 2025 at 3:55 PM Radim Krčmář <rkrcmar@ventanamicro.com> wrote:
> >> > >>
> >> > >> 2025-04-29T11:25:35+05:30, Anup Patel <apatel@ventanamicro.com>:
> >> > >> > On Mon, Apr 28, 2025 at 11:15 PM Radim Krčmář <rkrcmar@ventanamicro.com> wrote:
> >> > >> >>
> >> > >> >> 2025-04-28T17:52:25+05:30, Anup Patel <anup@brainfault.org>:
> >> > >> >> > On Thu, Apr 3, 2025 at 5:02 PM Radim Krčmář <rkrcmar@ventanamicro.com> wrote:
> >> > >> >> >> For a cleaner solution, we should add interfaces to perform the KVM-SBI
> >> > >> >> >> reset request on userspace demand.  I think it would also be much better
> >> > >> >> >> if userspace was in control of the post-reset state.
> >> > >> >> >
> >> > >> >> > Apart from breaking KVM user-space, this patch is incorrect and
> >> > >> >> > does not align with the:
> >> > >> >> > 1) SBI spec
> >> > >> >> > 2) OS boot protocol.
> >> > >> >> >
> >> > >> >> > The SBI spec only defines the entry state of certain CPU registers
> >> > >> >> > (namely, PC, A0, and A1) when CPU enters S-mode:
> >> > >> >> > 1) Upon SBI HSM start call from some other CPU
> >> > >> >> > 2) Upon resuming from non-retentive SBI HSM suspend or
> >> > >> >> >     SBI system suspend
> >> > >> >> >
> >> > >> >> > The S-mode entry state of the boot CPU is defined by the
> >> > >> >> > OS boot protocol and not by the SBI spec. Due to this, reason
> >> > >> >> > KVM RISC-V expects user-space to set up the S-mode entry
> >> > >> >> > state of the boot CPU upon system reset.
> >> > >> >>
> >> > >> >> We can handle the initial state consistency in other patches.
> >> > >> >> What needs addressing is a way to trigger the KVM reset from userspace,
> >> > >> >> even if only to clear the internal KVM state.
> >> > >> >>
> >> > >> >> I think mp_state is currently the best signalization that KVM should
> >> > >> >> reset, so I added it there.
> >> > >> >>
> >> > >> >> What would be your preferred interface for that?
> >> > >> >>
> >> > >> >
> >> > >> > Instead of creating a new interface, I would prefer that VCPU
> >> > >> > which initiates SBI System Reset should be resetted immediately
> >> > >> > in-kernel space before forwarding the system reset request to
> >> > >> > user space.
> >> > >>
> >> > >> The initiating VCPU might not be the boot VCPU.
> >> > >> It would be safer to reset all of them.
> >> > >
> >> > > I meant initiating VCPU and not the boot VCPU. Currently, the
> >> > > non-initiating VCPUs are already resetted by VCPU requests
> >> > > so nothing special needs to be done.
> >>
> >> There is no designated boot VCPU for KVM so let us only use the
> >> term "initiating" or "non-initiating" VCPUs in context of system reset.
>
> That is exactly how I use it.  Some VCPU will be the boot VCPU (the VCPU
> made runnable by KVM_SET_MP_STATE) and loaded with state from userspace.
>
> RISC-V doesn't guarantee that the boot VCPU is the reset initiating
> VCPU, so I think KVM should allow it.

We do allow any VCPU to be the boot VCPU. I am not sure why you
think otherwise.

>
> >> > Currently, we make the request only for VCPUs brought up by HSM -- the
> >> > non-boot VCPUs.  There is a single VCPU not being reset and resetting
> >> > the reset initiating VCPU changes nothing. e.g.
> >> >
> >> >   1) VCPU 1 initiates the reset through an ecall.
> >> >   2) All VCPUs are stopped and return to userspace.
> >>
> >> When all VCPUs are stopped, all VCPUs except VCPU1
> >> (in this example) will SLEEP because we do
> >> "kvm_make_all_cpus_request(vcpu->kvm, KVM_REQ_SLEEP)"
> >> so none of the VCPUs except VCPU1 (in this case) will
> >> return to userspace.
>
> Userspace should be able to do whatever it likes -- in my example, all
> the VCPUs are brought to userspace and a different boot VCPU is
> selected.

In your example, the VCPU1 (initiating VCPU) need not be the
boot VCPU after system reset. The user space can setup some
other VCPU as boot VCPU (by setting its MPSTATE, PC, A0,
and A1) and simply do ioctl_run() for the initiating VCPU without
changing its MPSTATE.

>
> (Perhaps userspace wanted to record their reset pre-reset state, or
>  maybe it really wants to boot with a designated VCPU.)
>
> >> >   3) Userspace prepares VCPU 0 as the boot VCPU.
> >> >   4) VCPU 0 executes without going through KVM reset paths.
> >>
> >> Userspace will see a system reset event exit for the
> >> initiating VCPU by that time all other VCPUs are already
> >> sleeping with mp_state == KVM_MP_STATE_STOPPED.
> >>
> >> >
> >> > The point of this patch is to reset the boot VCPU, so we reset the VCPU
> >> > that is made runnable by the KVM_SET_MP_STATE IOCTL.
> >>
> >> Like I said before, we don't need to do this. The initiating VCPU
> >> can be resetted just before exiting to user space for system reset
> >> event exit.
>
> You assume initiating VCPU == boot VCPU.
>
> We should prevent KVM_SET_MP_STATE IOCTL for all non-initiating VCPUs if
> we decide to accept the assumption.

There is no such assumption.

>
> I'd rather choose a different design, though.
>
> How about a new userspace interface for IOCTL reset?
> (Can be capability toggle for KVM_SET_MP_STATE or a straight new IOCTL.)
>
> That wouldn't "fix" current userspaces, but would significantly improve
> the sanity of the KVM interface.

I believe the current implementation needs a few improvements
that's all. We certainly don't need to introduce any new IOCTL.

Also, keep in mind that so far we have avoided any RISC-V
specific KVM IOCTLs and we should try to keep it that way
as long as we can.

Regards,
Anup

_______________________________________________
linux-riscv mailing list
linux-riscv@lists.infradead.org
http://lists.infradead.org/mailman/listinfo/linux-riscv

Re: [PATCH 4/5] KVM: RISC-V: reset VCPU state when becoming runnable

[Radim Krčmář]

2025-04-30T15:47:13+05:30, Anup Patel <anup@brainfault.org>:
> On Wed, Apr 30, 2025 at 1:59 PM Radim Krčmář <rkrcmar@ventanamicro.com> wrote:
>> 2025-04-30T10:56:35+05:30, Anup Patel <anup@brainfault.org>:
>> > On Wed, Apr 30, 2025 at 9:52 AM Anup Patel <anup@brainfault.org> wrote:
>> >> On Tue, Apr 29, 2025 at 9:51 PM Radim Krčmář <rkrcmar@ventanamicro.com> wrote:
>> >> > The point of this patch is to reset the boot VCPU, so we reset the VCPU
>> >> > that is made runnable by the KVM_SET_MP_STATE IOCTL.
>> >>
>> >> Like I said before, we don't need to do this. The initiating VCPU
>> >> can be resetted just before exiting to user space for system reset
>> >> event exit.
>>
>> You assume initiating VCPU == boot VCPU.
>>
>> We should prevent KVM_SET_MP_STATE IOCTL for all non-initiating VCPUs if
>> we decide to accept the assumption.
>
> There is no such assumption.

You probably haven't intended it:

  1) VCPU 0 is "chilling" in userspace.
  2) VCPU 1 initiates SBI reset.
  3) VCPU 1 makes a reset request to VCPU 0.
  4) VCPU 1 returns to userspace.
  5) Userspace knows it should reset the VM.
  6) VCPU 0 still hasn't entered KVM.
  7) Userspace sets the initial state of VCPU 0 and enters KVM.
  8) VCPU 0 is reset in KVM, because of the pending request.
  9) The initial boot state from userspace is lost.

>> I'd rather choose a different design, though.
>>
>> How about a new userspace interface for IOCTL reset?
>> (Can be capability toggle for KVM_SET_MP_STATE or a straight new IOCTL.)
>>
>> That wouldn't "fix" current userspaces, but would significantly improve
>> the sanity of the KVM interface.
>
> I believe the current implementation needs a few improvements
> that's all. We certainly don't need to introduce any new IOCTL.

I do too.  The whole patch could have been a single line:

diff --git a/arch/riscv/kvm/vcpu.c b/arch/riscv/kvm/vcpu.c
index d3d957a9e5c4..b3e6ad87e1cd 100644
--- a/arch/riscv/kvm/vcpu.c
+++ b/arch/riscv/kvm/vcpu.c
@@ -511,6 +511,7 @@ int kvm_arch_vcpu_ioctl_set_mpstate(struct kvm_vcpu *vcpu,
 
 	switch (mp_state->mp_state) {
 	case KVM_MP_STATE_RUNNABLE:
+		kvm_riscv_reset_vcpu(vcpu);
 		WRITE_ONCE(vcpu->arch.mp_state, *mp_state);
 		break;
 	case KVM_MP_STATE_STOPPED:

It is the backward compatibility and trying to fix current userspaces
that's making it ugly.  I already gave up on the latter, so we can have
a decently clean solution with the former.

> Also, keep in mind that so far we have avoided any RISC-V
> specific KVM IOCTLs and we should try to keep it that way
> as long as we can.

We can re-use KVM_SET_MP_STATE and add a KVM capability.
Userspace will opt-in to reset the VCPU through the existing IOCTL.

This design will also allow userspace to trigger a VCPU reset without
tearing down the whole VM.

_______________________________________________
linux-riscv mailing list
linux-riscv@lists.infradead.org
http://lists.infradead.org/mailman/listinfo/linux-riscv

Re: [PATCH 4/5] KVM: RISC-V: reset VCPU state when becoming runnable

[Anup Patel]

On Wed, Apr 30, 2025 at 5:15 PM Radim Krčmář <rkrcmar@ventanamicro.com> wrote:
>
> 2025-04-30T15:47:13+05:30, Anup Patel <anup@brainfault.org>:
> > On Wed, Apr 30, 2025 at 1:59 PM Radim Krčmář <rkrcmar@ventanamicro.com> wrote:
> >> 2025-04-30T10:56:35+05:30, Anup Patel <anup@brainfault.org>:
> >> > On Wed, Apr 30, 2025 at 9:52 AM Anup Patel <anup@brainfault.org> wrote:
> >> >> On Tue, Apr 29, 2025 at 9:51 PM Radim Krčmář <rkrcmar@ventanamicro.com> wrote:
> >> >> > The point of this patch is to reset the boot VCPU, so we reset the VCPU
> >> >> > that is made runnable by the KVM_SET_MP_STATE IOCTL.
> >> >>
> >> >> Like I said before, we don't need to do this. The initiating VCPU
> >> >> can be resetted just before exiting to user space for system reset
> >> >> event exit.
> >>
> >> You assume initiating VCPU == boot VCPU.
> >>
> >> We should prevent KVM_SET_MP_STATE IOCTL for all non-initiating VCPUs if
> >> we decide to accept the assumption.
> >
> > There is no such assumption.
>
> You probably haven't intended it:
>
>   1) VCPU 0 is "chilling" in userspace.
>   2) VCPU 1 initiates SBI reset.
>   3) VCPU 1 makes a reset request to VCPU 0.
>   4) VCPU 1 returns to userspace.
>   5) Userspace knows it should reset the VM.
>   6) VCPU 0 still hasn't entered KVM.
>   7) Userspace sets the initial state of VCPU 0 and enters KVM.
>   8) VCPU 0 is reset in KVM, because of the pending request.
>   9) The initial boot state from userspace is lost.
>
> >> I'd rather choose a different design, though.
> >>
> >> How about a new userspace interface for IOCTL reset?
> >> (Can be capability toggle for KVM_SET_MP_STATE or a straight new IOCTL.)
> >>
> >> That wouldn't "fix" current userspaces, but would significantly improve
> >> the sanity of the KVM interface.
> >
> > I believe the current implementation needs a few improvements
> > that's all. We certainly don't need to introduce any new IOCTL.
>
> I do too.  The whole patch could have been a single line:
>
> diff --git a/arch/riscv/kvm/vcpu.c b/arch/riscv/kvm/vcpu.c
> index d3d957a9e5c4..b3e6ad87e1cd 100644
> --- a/arch/riscv/kvm/vcpu.c
> +++ b/arch/riscv/kvm/vcpu.c
> @@ -511,6 +511,7 @@ int kvm_arch_vcpu_ioctl_set_mpstate(struct kvm_vcpu *vcpu,
>
>         switch (mp_state->mp_state) {
>         case KVM_MP_STATE_RUNNABLE:
> +               kvm_riscv_reset_vcpu(vcpu);
>                 WRITE_ONCE(vcpu->arch.mp_state, *mp_state);
>                 break;
>         case KVM_MP_STATE_STOPPED:
>
> It is the backward compatibility and trying to fix current userspaces
> that's making it ugly.  I already gave up on the latter, so we can have
> a decently clean solution with the former.
>
> > Also, keep in mind that so far we have avoided any RISC-V
> > specific KVM IOCTLs and we should try to keep it that way
> > as long as we can.
>
> We can re-use KVM_SET_MP_STATE and add a KVM capability.
> Userspace will opt-in to reset the VCPU through the existing IOCTL.
>
> This design will also allow userspace to trigger a VCPU reset without
> tearing down the whole VM.

Okay, lets go ahead with a KVM capability which user space can opt-in
for KVM_SET_MP_STATE ioctl().

Keep in mind that at runtime Guest can still do CPU hotplug using SBI
HSM start/stop and do system suspend using SBI SUSP so we should
continue to have VCPU reset requests for both these SBI extensions.

Regards,
Anup

_______________________________________________
linux-riscv mailing list
linux-riscv@lists.infradead.org
http://lists.infradead.org/mailman/listinfo/linux-riscv

Re: [PATCH 4/5] KVM: RISC-V: reset VCPU state when becoming runnable

[Radim Krčmář]

2025-04-30T18:32:31+05:30, Anup Patel <anup@brainfault.org>:
> On Wed, Apr 30, 2025 at 5:15 PM Radim Krčmář <rkrcmar@ventanamicro.com> wrote:
>> We can re-use KVM_SET_MP_STATE and add a KVM capability.
>> Userspace will opt-in to reset the VCPU through the existing IOCTL.
>>
>> This design will also allow userspace to trigger a VCPU reset without
>> tearing down the whole VM.
>
> Okay, lets go ahead with a KVM capability which user space can opt-in
> for KVM_SET_MP_STATE ioctl().
>
> Keep in mind that at runtime Guest can still do CPU hotplug using SBI
> HSM start/stop and do system suspend using SBI SUSP so we should
> continue to have VCPU reset requests for both these SBI extensions.

Will do, thanks.

_______________________________________________
linux-riscv mailing list
linux-riscv@lists.infradead.org
http://lists.infradead.org/mailman/listinfo/linux-riscv

Re: [PATCH 1/5] KVM: RISC-V: refactor vector state reset

[Anup Patel]

On Thu, Apr 3, 2025 at 5:02 PM Radim Krčmář <rkrcmar@ventanamicro.com> wrote:
>
> Do not depend on the reset structures.
>
> vector.datap is a kernel memory pointer that needs to be preserved as it
> is not a part of the guest vector data.
>
> Signed-off-by: Radim Krčmář <rkrcmar@ventanamicro.com>

Queued this patch for Linux-6.16

Thanks,
Anup

> ---
>  arch/riscv/include/asm/kvm_vcpu_vector.h |  6 ++----
>  arch/riscv/kvm/vcpu.c                    |  5 ++++-
>  arch/riscv/kvm/vcpu_vector.c             | 13 +++++++------
>  3 files changed, 13 insertions(+), 11 deletions(-)
>
> diff --git a/arch/riscv/include/asm/kvm_vcpu_vector.h b/arch/riscv/include/asm/kvm_vcpu_vector.h
> index 27f5bccdd8b0..57a798a4cb0d 100644
> --- a/arch/riscv/include/asm/kvm_vcpu_vector.h
> +++ b/arch/riscv/include/asm/kvm_vcpu_vector.h
> @@ -33,8 +33,7 @@ void kvm_riscv_vcpu_guest_vector_restore(struct kvm_cpu_context *cntx,
>                                          unsigned long *isa);
>  void kvm_riscv_vcpu_host_vector_save(struct kvm_cpu_context *cntx);
>  void kvm_riscv_vcpu_host_vector_restore(struct kvm_cpu_context *cntx);
> -int kvm_riscv_vcpu_alloc_vector_context(struct kvm_vcpu *vcpu,
> -                                       struct kvm_cpu_context *cntx);
> +int kvm_riscv_vcpu_alloc_vector_context(struct kvm_vcpu *vcpu);
>  void kvm_riscv_vcpu_free_vector_context(struct kvm_vcpu *vcpu);
>  #else
>
> @@ -62,8 +61,7 @@ static inline void kvm_riscv_vcpu_host_vector_restore(struct kvm_cpu_context *cn
>  {
>  }
>
> -static inline int kvm_riscv_vcpu_alloc_vector_context(struct kvm_vcpu *vcpu,
> -                                                     struct kvm_cpu_context *cntx)
> +static inline int kvm_riscv_vcpu_alloc_vector_context(struct kvm_vcpu *vcpu)
>  {
>         return 0;
>  }
> diff --git a/arch/riscv/kvm/vcpu.c b/arch/riscv/kvm/vcpu.c
> index 60d684c76c58..2fb75288ecfe 100644
> --- a/arch/riscv/kvm/vcpu.c
> +++ b/arch/riscv/kvm/vcpu.c
> @@ -57,6 +57,7 @@ static void kvm_riscv_reset_vcpu(struct kvm_vcpu *vcpu)
>         struct kvm_vcpu_csr *reset_csr = &vcpu->arch.guest_reset_csr;
>         struct kvm_cpu_context *cntx = &vcpu->arch.guest_context;
>         struct kvm_cpu_context *reset_cntx = &vcpu->arch.guest_reset_context;
> +       void *vector_datap = cntx->vector.datap;
>         bool loaded;
>
>         /**
> @@ -79,6 +80,8 @@ static void kvm_riscv_reset_vcpu(struct kvm_vcpu *vcpu)
>
>         kvm_riscv_vcpu_fp_reset(vcpu);
>
> +       /* Restore datap as it's not a part of the guest context. */
> +       cntx->vector.datap = vector_datap;
>         kvm_riscv_vcpu_vector_reset(vcpu);
>
>         kvm_riscv_vcpu_timer_reset(vcpu);
> @@ -143,7 +146,7 @@ int kvm_arch_vcpu_create(struct kvm_vcpu *vcpu)
>         cntx->hstatus |= HSTATUS_SPV;
>         spin_unlock(&vcpu->arch.reset_cntx_lock);
>
> -       if (kvm_riscv_vcpu_alloc_vector_context(vcpu, cntx))
> +       if (kvm_riscv_vcpu_alloc_vector_context(vcpu))
>                 return -ENOMEM;
>
>         /* By default, make CY, TM, and IR counters accessible in VU mode */
> diff --git a/arch/riscv/kvm/vcpu_vector.c b/arch/riscv/kvm/vcpu_vector.c
> index d92d1348045c..a5f88cb717f3 100644
> --- a/arch/riscv/kvm/vcpu_vector.c
> +++ b/arch/riscv/kvm/vcpu_vector.c
> @@ -22,6 +22,9 @@ void kvm_riscv_vcpu_vector_reset(struct kvm_vcpu *vcpu)
>         struct kvm_cpu_context *cntx = &vcpu->arch.guest_context;
>
>         cntx->sstatus &= ~SR_VS;
> +
> +       cntx->vector.vlenb = riscv_v_vsize / 32;
> +
>         if (riscv_isa_extension_available(isa, v)) {
>                 cntx->sstatus |= SR_VS_INITIAL;
>                 WARN_ON(!cntx->vector.datap);
> @@ -70,13 +73,11 @@ void kvm_riscv_vcpu_host_vector_restore(struct kvm_cpu_context *cntx)
>                 __kvm_riscv_vector_restore(cntx);
>  }
>
> -int kvm_riscv_vcpu_alloc_vector_context(struct kvm_vcpu *vcpu,
> -                                       struct kvm_cpu_context *cntx)
> +int kvm_riscv_vcpu_alloc_vector_context(struct kvm_vcpu *vcpu)
>  {
> -       cntx->vector.datap = kmalloc(riscv_v_vsize, GFP_KERNEL);
> -       if (!cntx->vector.datap)
> +       vcpu->arch.guest_context.vector.datap = kzalloc(riscv_v_vsize, GFP_KERNEL);
> +       if (!vcpu->arch.guest_context.vector.datap)
>                 return -ENOMEM;
> -       cntx->vector.vlenb = riscv_v_vsize / 32;
>
>         vcpu->arch.host_context.vector.datap = kzalloc(riscv_v_vsize, GFP_KERNEL);
>         if (!vcpu->arch.host_context.vector.datap)
> @@ -87,7 +88,7 @@ int kvm_riscv_vcpu_alloc_vector_context(struct kvm_vcpu *vcpu,
>
>  void kvm_riscv_vcpu_free_vector_context(struct kvm_vcpu *vcpu)
>  {
> -       kfree(vcpu->arch.guest_reset_context.vector.datap);
> +       kfree(vcpu->arch.guest_context.vector.datap);
>         kfree(vcpu->arch.host_context.vector.datap);
>  }
>  #endif
> --
> 2.48.1
>

_______________________________________________
linux-riscv mailing list
linux-riscv@lists.infradead.org
http://lists.infradead.org/mailman/listinfo/linux-riscv

Re: [PATCH 2/5] KVM: RISC-V: refactor sbi reset request

[Anup Patel]

On Thu, Apr 3, 2025 at 5:02 PM Radim Krčmář <rkrcmar@ventanamicro.com> wrote:
>
> The same code is used twice and SBI reset sets only two variables.
>
> Signed-off-by: Radim Krčmář <rkrcmar@ventanamicro.com>
> ---
>  arch/riscv/include/asm/kvm_vcpu_sbi.h |  2 ++
>  arch/riscv/kvm/vcpu_sbi.c             | 12 ++++++++++++
>  arch/riscv/kvm/vcpu_sbi_hsm.c         | 13 +------------
>  arch/riscv/kvm/vcpu_sbi_system.c      | 10 +---------
>  4 files changed, 16 insertions(+), 21 deletions(-)
>
> diff --git a/arch/riscv/include/asm/kvm_vcpu_sbi.h b/arch/riscv/include/asm/kvm_vcpu_sbi.h
> index 4ed6203cdd30..aaaa81355276 100644
> --- a/arch/riscv/include/asm/kvm_vcpu_sbi.h
> +++ b/arch/riscv/include/asm/kvm_vcpu_sbi.h
> @@ -55,6 +55,8 @@ void kvm_riscv_vcpu_sbi_forward(struct kvm_vcpu *vcpu, struct kvm_run *run);
>  void kvm_riscv_vcpu_sbi_system_reset(struct kvm_vcpu *vcpu,
>                                      struct kvm_run *run,
>                                      u32 type, u64 flags);
> +void kvm_riscv_vcpu_sbi_request_reset(struct kvm_vcpu *vcpu,
> +                                      unsigned long pc, unsigned long a1);

Use tabs for alignment instead of spaces.

>  int kvm_riscv_vcpu_sbi_return(struct kvm_vcpu *vcpu, struct kvm_run *run);
>  int kvm_riscv_vcpu_set_reg_sbi_ext(struct kvm_vcpu *vcpu,
>                                    const struct kvm_one_reg *reg);
> diff --git a/arch/riscv/kvm/vcpu_sbi.c b/arch/riscv/kvm/vcpu_sbi.c
> index d1c83a77735e..f58368f7df1d 100644
> --- a/arch/riscv/kvm/vcpu_sbi.c
> +++ b/arch/riscv/kvm/vcpu_sbi.c
> @@ -156,6 +156,18 @@ void kvm_riscv_vcpu_sbi_system_reset(struct kvm_vcpu *vcpu,
>         run->exit_reason = KVM_EXIT_SYSTEM_EVENT;
>  }
>
> +void kvm_riscv_vcpu_sbi_request_reset(struct kvm_vcpu *vcpu,
> +                                      unsigned long pc, unsigned long a1)
> +{
> +       spin_lock(&vcpu->arch.reset_cntx_lock);
> +       vcpu->arch.guest_reset_context.sepc = pc;
> +       vcpu->arch.guest_reset_context.a0 = vcpu->vcpu_id;
> +       vcpu->arch.guest_reset_context.a1 = a1;
> +       spin_unlock(&vcpu->arch.reset_cntx_lock);
> +
> +       kvm_make_request(KVM_REQ_VCPU_RESET, vcpu);
> +}
> +
>  int kvm_riscv_vcpu_sbi_return(struct kvm_vcpu *vcpu, struct kvm_run *run)
>  {
>         struct kvm_cpu_context *cp = &vcpu->arch.guest_context;
> diff --git a/arch/riscv/kvm/vcpu_sbi_hsm.c b/arch/riscv/kvm/vcpu_sbi_hsm.c
> index 3070bb31745d..f26207f84bab 100644
> --- a/arch/riscv/kvm/vcpu_sbi_hsm.c
> +++ b/arch/riscv/kvm/vcpu_sbi_hsm.c
> @@ -15,7 +15,6 @@
>
>  static int kvm_sbi_hsm_vcpu_start(struct kvm_vcpu *vcpu)
>  {
> -       struct kvm_cpu_context *reset_cntx;
>         struct kvm_cpu_context *cp = &vcpu->arch.guest_context;
>         struct kvm_vcpu *target_vcpu;
>         unsigned long target_vcpuid = cp->a0;
> @@ -32,17 +31,7 @@ static int kvm_sbi_hsm_vcpu_start(struct kvm_vcpu *vcpu)
>                 goto out;
>         }
>
> -       spin_lock(&target_vcpu->arch.reset_cntx_lock);
> -       reset_cntx = &target_vcpu->arch.guest_reset_context;
> -       /* start address */
> -       reset_cntx->sepc = cp->a1;
> -       /* target vcpu id to start */
> -       reset_cntx->a0 = target_vcpuid;
> -       /* private data passed from kernel */
> -       reset_cntx->a1 = cp->a2;
> -       spin_unlock(&target_vcpu->arch.reset_cntx_lock);
> -
> -       kvm_make_request(KVM_REQ_VCPU_RESET, target_vcpu);
> +       kvm_riscv_vcpu_sbi_request_reset(target_vcpu, cp->a1, cp->a2);
>
>         __kvm_riscv_vcpu_power_on(target_vcpu);
>
> diff --git a/arch/riscv/kvm/vcpu_sbi_system.c b/arch/riscv/kvm/vcpu_sbi_system.c
> index bc0ebba89003..359be90b0fc5 100644
> --- a/arch/riscv/kvm/vcpu_sbi_system.c
> +++ b/arch/riscv/kvm/vcpu_sbi_system.c
> @@ -13,7 +13,6 @@ static int kvm_sbi_ext_susp_handler(struct kvm_vcpu *vcpu, struct kvm_run *run,
>                                     struct kvm_vcpu_sbi_return *retdata)
>  {
>         struct kvm_cpu_context *cp = &vcpu->arch.guest_context;
> -       struct kvm_cpu_context *reset_cntx;
>         unsigned long funcid = cp->a6;
>         unsigned long hva, i;
>         struct kvm_vcpu *tmp;
> @@ -45,14 +44,7 @@ static int kvm_sbi_ext_susp_handler(struct kvm_vcpu *vcpu, struct kvm_run *run,
>                         }
>                 }
>
> -               spin_lock(&vcpu->arch.reset_cntx_lock);
> -               reset_cntx = &vcpu->arch.guest_reset_context;
> -               reset_cntx->sepc = cp->a1;
> -               reset_cntx->a0 = vcpu->vcpu_id;
> -               reset_cntx->a1 = cp->a2;
> -               spin_unlock(&vcpu->arch.reset_cntx_lock);
> -
> -               kvm_make_request(KVM_REQ_VCPU_RESET, vcpu);
> +               kvm_riscv_vcpu_sbi_request_reset(vcpu, cp->a1, cp->a2);
>
>                 /* userspace provides the suspend implementation */
>                 kvm_riscv_vcpu_sbi_forward(vcpu, run);
> --
> 2.48.1
>

Otherwise, it looks good to me.
I have taken care of the above comment at the time
of merging this patch.

Queued this patch for Linux-6.16

Thanks,
Anup

_______________________________________________
linux-riscv mailing list
linux-riscv@lists.infradead.org
http://lists.infradead.org/mailman/listinfo/linux-riscv

Re: [PATCH 2/5] KVM: RISC-V: refactor sbi reset request

[Radim Krčmář]

2025-05-07T17:31:33+05:30, Anup Patel <anup@brainfault.org>:
> On Thu, Apr 3, 2025 at 5:02 PM Radim Krčmář <rkrcmar@ventanamicro.com> wrote:
>> diff --git a/arch/riscv/include/asm/kvm_vcpu_sbi.h b/arch/riscv/include/asm/kvm_vcpu_sbi.h
>> @@ -55,6 +55,8 @@ void kvm_riscv_vcpu_sbi_forward(struct kvm_vcpu *vcpu, struct kvm_run *run);
>>  void kvm_riscv_vcpu_sbi_system_reset(struct kvm_vcpu *vcpu,
>>                                      struct kvm_run *run,
>>                                      u32 type, u64 flags);
>> +void kvm_riscv_vcpu_sbi_request_reset(struct kvm_vcpu *vcpu,
>> +                                      unsigned long pc, unsigned long a1);
>
> Use tabs for alignment instead of spaces.

Oops, I totally forgot that linux uses tabs even for alignment.

> Otherwise, it looks good to me.
> I have taken care of the above comment at the time
> of merging this patch.

Thanks, I'll post v2 without the three patches.

_______________________________________________
linux-riscv mailing list
linux-riscv@lists.infradead.org
http://lists.infradead.org/mailman/listinfo/linux-riscv

Re: [PATCH 2/5] KVM: RISC-V: refactor sbi reset request

[Anup Patel]

On Wed, May 7, 2025 at 10:58 PM Radim Krčmář <rkrcmar@ventanamicro.com> wrote:
>
> 2025-05-07T17:31:33+05:30, Anup Patel <anup@brainfault.org>:
> > On Thu, Apr 3, 2025 at 5:02 PM Radim Krčmář <rkrcmar@ventanamicro.com> wrote:
> >> diff --git a/arch/riscv/include/asm/kvm_vcpu_sbi.h b/arch/riscv/include/asm/kvm_vcpu_sbi.h
> >> @@ -55,6 +55,8 @@ void kvm_riscv_vcpu_sbi_forward(struct kvm_vcpu *vcpu, struct kvm_run *run);
> >>  void kvm_riscv_vcpu_sbi_system_reset(struct kvm_vcpu *vcpu,
> >>                                      struct kvm_run *run,
> >>                                      u32 type, u64 flags);
> >> +void kvm_riscv_vcpu_sbi_request_reset(struct kvm_vcpu *vcpu,
> >> +                                      unsigned long pc, unsigned long a1);
> >
> > Use tabs for alignment instead of spaces.
>
> Oops, I totally forgot that linux uses tabs even for alignment.
>
> > Otherwise, it looks good to me.
> > I have taken care of the above comment at the time
> > of merging this patch.
>
> Thanks, I'll post v2 without the three patches.

Base your v2 upon riscv_kvm_queue branch at
https://github.com/kvm-riscv/linux.git

Regards,
Anup

_______________________________________________
linux-riscv mailing list
linux-riscv@lists.infradead.org
http://lists.infradead.org/mailman/listinfo/linux-riscv

Re: [PATCH 3/5] KVM: RISC-V: remove unnecessary SBI reset state

[Anup Patel]

On Thu, Apr 3, 2025 at 5:02 PM Radim Krčmář <rkrcmar@ventanamicro.com> wrote:
>
> The SBI reset state has only two variables -- pc and a1.
> The rest is known, so keep only the necessary information.
>
> The reset structures make sense if we want userspace to control the
> reset state (which we do), but I'd still remove them now and reintroduce
> with the userspace interface later -- we could probably have just a
> single reset state per VM, instead of a reset state for each VCPU.
>
> Signed-off-by: Radim Krčmář <rkrcmar@ventanamicro.com>

Queued this patch for Linux-6.16

Thanks,
Anup

> ---
>  arch/riscv/include/asm/kvm_aia.h  |  3 --
>  arch/riscv/include/asm/kvm_host.h | 12 ++++---
>  arch/riscv/kvm/aia_device.c       |  4 +--
>  arch/riscv/kvm/vcpu.c             | 58 +++++++++++++++++--------------
>  arch/riscv/kvm/vcpu_sbi.c         |  9 +++--
>  5 files changed, 44 insertions(+), 42 deletions(-)
>
> diff --git a/arch/riscv/include/asm/kvm_aia.h b/arch/riscv/include/asm/kvm_aia.h
> index 1f37b600ca47..3b643b9efc07 100644
> --- a/arch/riscv/include/asm/kvm_aia.h
> +++ b/arch/riscv/include/asm/kvm_aia.h
> @@ -63,9 +63,6 @@ struct kvm_vcpu_aia {
>         /* CPU AIA CSR context of Guest VCPU */
>         struct kvm_vcpu_aia_csr guest_csr;
>
> -       /* CPU AIA CSR context upon Guest VCPU reset */
> -       struct kvm_vcpu_aia_csr guest_reset_csr;
> -
>         /* Guest physical address of IMSIC for this VCPU */
>         gpa_t           imsic_addr;
>
> diff --git a/arch/riscv/include/asm/kvm_host.h b/arch/riscv/include/asm/kvm_host.h
> index 0e9c2fab6378..0c8c9c05af91 100644
> --- a/arch/riscv/include/asm/kvm_host.h
> +++ b/arch/riscv/include/asm/kvm_host.h
> @@ -193,6 +193,12 @@ struct kvm_vcpu_smstateen_csr {
>         unsigned long sstateen0;
>  };
>
> +struct kvm_vcpu_reset_state {
> +       spinlock_t lock;
> +       unsigned long pc;
> +       unsigned long a1;
> +};
> +
>  struct kvm_vcpu_arch {
>         /* VCPU ran at least once */
>         bool ran_atleast_once;
> @@ -227,12 +233,8 @@ struct kvm_vcpu_arch {
>         /* CPU Smstateen CSR context of Guest VCPU */
>         struct kvm_vcpu_smstateen_csr smstateen_csr;
>
> -       /* CPU context upon Guest VCPU reset */
> -       struct kvm_cpu_context guest_reset_context;
> -       spinlock_t reset_cntx_lock;
> +       struct kvm_vcpu_reset_state reset_state;
>
> -       /* CPU CSR context upon Guest VCPU reset */
> -       struct kvm_vcpu_csr guest_reset_csr;
>
>         /*
>          * VCPU interrupts
> diff --git a/arch/riscv/kvm/aia_device.c b/arch/riscv/kvm/aia_device.c
> index 39cd26af5a69..43e472ff3e1a 100644
> --- a/arch/riscv/kvm/aia_device.c
> +++ b/arch/riscv/kvm/aia_device.c
> @@ -526,12 +526,10 @@ int kvm_riscv_vcpu_aia_update(struct kvm_vcpu *vcpu)
>  void kvm_riscv_vcpu_aia_reset(struct kvm_vcpu *vcpu)
>  {
>         struct kvm_vcpu_aia_csr *csr = &vcpu->arch.aia_context.guest_csr;
> -       struct kvm_vcpu_aia_csr *reset_csr =
> -                               &vcpu->arch.aia_context.guest_reset_csr;
>
>         if (!kvm_riscv_aia_available())
>                 return;
> -       memcpy(csr, reset_csr, sizeof(*csr));
> +       memset(csr, 0, sizeof(*csr));
>
>         /* Proceed only if AIA was initialized successfully */
>         if (!kvm_riscv_aia_initialized(vcpu->kvm))
> diff --git a/arch/riscv/kvm/vcpu.c b/arch/riscv/kvm/vcpu.c
> index 2fb75288ecfe..b8485c1c1ce4 100644
> --- a/arch/riscv/kvm/vcpu.c
> +++ b/arch/riscv/kvm/vcpu.c
> @@ -51,13 +51,40 @@ const struct kvm_stats_header kvm_vcpu_stats_header = {
>                        sizeof(kvm_vcpu_stats_desc),
>  };
>
> -static void kvm_riscv_reset_vcpu(struct kvm_vcpu *vcpu)
> +static void kvm_riscv_vcpu_context_reset(struct kvm_vcpu *vcpu)
>  {
>         struct kvm_vcpu_csr *csr = &vcpu->arch.guest_csr;
> -       struct kvm_vcpu_csr *reset_csr = &vcpu->arch.guest_reset_csr;
>         struct kvm_cpu_context *cntx = &vcpu->arch.guest_context;
> -       struct kvm_cpu_context *reset_cntx = &vcpu->arch.guest_reset_context;
> +       struct kvm_vcpu_reset_state *reset_state = &vcpu->arch.reset_state;
>         void *vector_datap = cntx->vector.datap;
> +
> +       memset(cntx, 0, sizeof(*cntx));
> +       memset(csr, 0, sizeof(*csr));
> +
> +       /* Restore datap as it's not a part of the guest context. */
> +       cntx->vector.datap = vector_datap;
> +
> +       /* Load SBI reset values */
> +       cntx->a0 = vcpu->vcpu_id;
> +
> +       spin_lock(&reset_state->lock);
> +       cntx->sepc = reset_state->pc;
> +       cntx->a1 = reset_state->a1;
> +       spin_unlock(&reset_state->lock);
> +
> +       /* Setup reset state of shadow SSTATUS and HSTATUS CSRs */
> +       cntx->sstatus = SR_SPP | SR_SPIE;
> +
> +       cntx->hstatus |= HSTATUS_VTW;
> +       cntx->hstatus |= HSTATUS_SPVP;
> +       cntx->hstatus |= HSTATUS_SPV;
> +
> +       /* By default, make CY, TM, and IR counters accessible in VU mode */
> +       csr->scounteren = 0x7;
> +}
> +
> +static void kvm_riscv_reset_vcpu(struct kvm_vcpu *vcpu)
> +{
>         bool loaded;
>
>         /**
> @@ -72,16 +99,10 @@ static void kvm_riscv_reset_vcpu(struct kvm_vcpu *vcpu)
>
>         vcpu->arch.last_exit_cpu = -1;
>
> -       memcpy(csr, reset_csr, sizeof(*csr));
> -
> -       spin_lock(&vcpu->arch.reset_cntx_lock);
> -       memcpy(cntx, reset_cntx, sizeof(*cntx));
> -       spin_unlock(&vcpu->arch.reset_cntx_lock);
> +       kvm_riscv_vcpu_context_reset(vcpu);
>
>         kvm_riscv_vcpu_fp_reset(vcpu);
>
> -       /* Restore datap as it's not a part of the guest context. */
> -       cntx->vector.datap = vector_datap;
>         kvm_riscv_vcpu_vector_reset(vcpu);
>
>         kvm_riscv_vcpu_timer_reset(vcpu);
> @@ -113,8 +134,6 @@ int kvm_arch_vcpu_precreate(struct kvm *kvm, unsigned int id)
>  int kvm_arch_vcpu_create(struct kvm_vcpu *vcpu)
>  {
>         int rc;
> -       struct kvm_cpu_context *cntx;
> -       struct kvm_vcpu_csr *reset_csr = &vcpu->arch.guest_reset_csr;
>
>         spin_lock_init(&vcpu->arch.mp_state_lock);
>
> @@ -134,24 +153,11 @@ int kvm_arch_vcpu_create(struct kvm_vcpu *vcpu)
>         /* Setup VCPU hfence queue */
>         spin_lock_init(&vcpu->arch.hfence_lock);
>
> -       /* Setup reset state of shadow SSTATUS and HSTATUS CSRs */
> -       spin_lock_init(&vcpu->arch.reset_cntx_lock);
> -
> -       spin_lock(&vcpu->arch.reset_cntx_lock);
> -       cntx = &vcpu->arch.guest_reset_context;
> -       cntx->sstatus = SR_SPP | SR_SPIE;
> -       cntx->hstatus = 0;
> -       cntx->hstatus |= HSTATUS_VTW;
> -       cntx->hstatus |= HSTATUS_SPVP;
> -       cntx->hstatus |= HSTATUS_SPV;
> -       spin_unlock(&vcpu->arch.reset_cntx_lock);
> +       spin_lock_init(&vcpu->arch.reset_state.lock);
>
>         if (kvm_riscv_vcpu_alloc_vector_context(vcpu))
>                 return -ENOMEM;
>
> -       /* By default, make CY, TM, and IR counters accessible in VU mode */
> -       reset_csr->scounteren = 0x7;
> -
>         /* Setup VCPU timer */
>         kvm_riscv_vcpu_timer_init(vcpu);
>
> diff --git a/arch/riscv/kvm/vcpu_sbi.c b/arch/riscv/kvm/vcpu_sbi.c
> index f58368f7df1d..3d7955e05cc3 100644
> --- a/arch/riscv/kvm/vcpu_sbi.c
> +++ b/arch/riscv/kvm/vcpu_sbi.c
> @@ -159,11 +159,10 @@ void kvm_riscv_vcpu_sbi_system_reset(struct kvm_vcpu *vcpu,
>  void kvm_riscv_vcpu_sbi_request_reset(struct kvm_vcpu *vcpu,
>                                        unsigned long pc, unsigned long a1)
>  {
> -       spin_lock(&vcpu->arch.reset_cntx_lock);
> -       vcpu->arch.guest_reset_context.sepc = pc;
> -       vcpu->arch.guest_reset_context.a0 = vcpu->vcpu_id;
> -       vcpu->arch.guest_reset_context.a1 = a1;
> -       spin_unlock(&vcpu->arch.reset_cntx_lock);
> +       spin_lock(&vcpu->arch.reset_state.lock);
> +       vcpu->arch.reset_state.pc = pc;
> +       vcpu->arch.reset_state.a1 = a1;
> +       spin_unlock(&vcpu->arch.reset_state.lock);
>
>         kvm_make_request(KVM_REQ_VCPU_RESET, vcpu);
>  }
> --
> 2.48.1
>

_______________________________________________
linux-riscv mailing list
linux-riscv@lists.infradead.org
http://lists.infradead.org/mailman/listinfo/linux-riscv

Re: [PATCH 3/5] KVM: RISC-V: remove unnecessary SBI reset state

[Radim Krčmář]

2025-05-08T11:48:00+05:30, Anup Patel <anup@brainfault.org>:
> On Thu, Apr 3, 2025 at 5:02 PM Radim Krčmář <rkrcmar@ventanamicro.com> wrote:
>>
>> The SBI reset state has only two variables -- pc and a1.
>> The rest is known, so keep only the necessary information.
>>
>> The reset structures make sense if we want userspace to control the
>> reset state (which we do), but I'd still remove them now and reintroduce
>> with the userspace interface later -- we could probably have just a
>> single reset state per VM, instead of a reset state for each VCPU.
>>
>> Signed-off-by: Radim Krčmář <rkrcmar@ventanamicro.com>
>
> Queued this patch for Linux-6.16

[5/5] was already applied, which means that [3/5] would be nicer with

  memset(&vcpu->arch.smstateen_csr, 0, sizeof(vcpu->arch.smstateen_csr));

in the new function (kvm_riscv_vcpu_context_reset) where we memset(0)
the other csr context.

Should I add a patch to do that in v2?

Thanks.

_______________________________________________
linux-riscv mailing list
linux-riscv@lists.infradead.org
http://lists.infradead.org/mailman/listinfo/linux-riscv

Re: [PATCH 3/5] KVM: RISC-V: remove unnecessary SBI reset state

[Anup Patel]

On Thu, May 8, 2025 at 3:32 PM Radim Krčmář <rkrcmar@ventanamicro.com> wrote:
>
> 2025-05-08T11:48:00+05:30, Anup Patel <anup@brainfault.org>:
> > On Thu, Apr 3, 2025 at 5:02 PM Radim Krčmář <rkrcmar@ventanamicro.com> wrote:
> >>
> >> The SBI reset state has only two variables -- pc and a1.
> >> The rest is known, so keep only the necessary information.
> >>
> >> The reset structures make sense if we want userspace to control the
> >> reset state (which we do), but I'd still remove them now and reintroduce
> >> with the userspace interface later -- we could probably have just a
> >> single reset state per VM, instead of a reset state for each VCPU.
> >>
> >> Signed-off-by: Radim Krčmář <rkrcmar@ventanamicro.com>
> >
> > Queued this patch for Linux-6.16
>
> [5/5] was already applied, which means that [3/5] would be nicer with
>
>   memset(&vcpu->arch.smstateen_csr, 0, sizeof(vcpu->arch.smstateen_csr));
>
> in the new function (kvm_riscv_vcpu_context_reset) where we memset(0)
> the other csr context.
>
> Should I add a patch to do that in v2?

Yes, please add it to your v2. I will update my queue accordingly.

Regards,
Anup

_______________________________________________
linux-riscv mailing list
linux-riscv@lists.infradead.org
http://lists.infradead.org/mailman/listinfo/linux-riscv