public inbox for kvm@vger.kernel.org
 help / color / mirror / Atom feed
* [PATCH 0/5] Enable hstateen bits lazily for the KVM RISC-V Guests
@ 2025-05-05 21:39 Atish Patra
  2025-05-05 21:39 ` [PATCH 1/5] RISC-V: KVM: Lazy enable hstateen IMSIC & ISEL bit Atish Patra
                   ` (5 more replies)
  0 siblings, 6 replies; 16+ messages in thread
From: Atish Patra @ 2025-05-05 21:39 UTC (permalink / raw)
  To: Anup Patel, Atish Patra, Paul Walmsley, Palmer Dabbelt,
	Alexandre Ghiti
  Cc: kvm, kvm-riscv, linux-riscv, linux-kernel, Atish Patra

This series adds support for enabling hstateen bits lazily at runtime
instead of statically at bootime. The boot time enabling happens for
all the guests if the required extensions are present in the host and/or
guest. That may not be necessary if the guest never exercise that
feature. We can enable the hstateen bits that controls the access lazily
upon first access. This providers KVM more granular control of which
feature is enabled in the guest at runtime.

Currently, the following hstateen bits are supported to control the access
from VS mode.

1. BIT(58): IMSIC     : STOPEI and IMSIC guest interrupt file
2. BIT(59): AIA       : SIPH/SIEH/STOPI
3. BIT(60): AIA_ISEL  : Indirect csr access via siselect/sireg
4. BIT(62): HSENVCFG  : SENVCFG access
5. BIT(63): SSTATEEN0 : SSTATEEN0 access

KVM already support trap/enabling of BIT(58) and BIT(60) in order
to support sw version of the guest interrupt file. This series extends
those to enable to correpsonding hstateen bits in PATCH1. The remaining
patches adds lazy enabling support of the other bits.

I am working on a followup series to add indirect CSR extension and move the
siselect/sireg handlers out of AIA so that other features(e.g CTR) can leverage
it.

Note: This series just updates the hstateen bit in cfg so that any update
would reflect in the correct VM state during the next vcpu load.
Alternatively, we can save the hstateen state in vcpu_put to achieve this.
However, it will incur additional cost on every VM exit while the current
approach just updates the configuration once per VM life time upon first
access.

To: Anup Patel <anup@brainfault.org>
To: Atish Patra <atishp@atishpatra.org>
To: Paul Walmsley <paul.walmsley@sifive.com>
To: Palmer Dabbelt <palmer@dabbelt.com>
To: Alexandre Ghiti <alex@ghiti.fr>
Cc: kvm@vger.kernel.org
Cc: kvm-riscv@lists.infradead.org
Cc: linux-riscv@lists.infradead.org
Cc: linux-kernel@vger.kernel.org

Signed-off-by: Atish Patra <atishp@rivosinc.com>
---
Atish Patra (5):
      RISC-V: KVM: Lazy enable hstateen IMSIC & ISEL bit
      RISC-V: KVM: Add a hstateen lazy enabler helper function
      RISC-V: KVM: Support lazy enabling of siselect and aia bits
      RISC-V: KVM: Enable envcfg and sstateen bits lazily
      RISC-V: KVM: Remove the boot time enabling of hstateen bits

 arch/riscv/include/asm/kvm_aia.h       | 14 ++++++-
 arch/riscv/include/asm/kvm_vcpu_insn.h |  4 ++
 arch/riscv/kvm/aia.c                   | 77 ++++++++++++++++++++++++++++++++++
 arch/riscv/kvm/aia_imsic.c             |  8 ++++
 arch/riscv/kvm/vcpu.c                  | 10 -----
 arch/riscv/kvm/vcpu_insn.c             | 52 +++++++++++++++++++++++
 6 files changed, 154 insertions(+), 11 deletions(-)
---
base-commit: 01f95500a162fca88cefab9ed64ceded5afabc12
change-id: 20250430-kvm_lazy_enable_stateen-33c8aa9a3071
--
Regards,
Atish patra


^ permalink raw reply	[flat|nested] 16+ messages in thread

* [PATCH 1/5] RISC-V: KVM: Lazy enable hstateen IMSIC & ISEL bit
  2025-05-05 21:39 [PATCH 0/5] Enable hstateen bits lazily for the KVM RISC-V Guests Atish Patra
@ 2025-05-05 21:39 ` Atish Patra
  2025-05-08 13:31   ` Radim Krčmář
  2025-05-05 21:39 ` [PATCH 2/5] RISC-V: KVM: Add a hstateen lazy enabler helper function Atish Patra
                   ` (4 subsequent siblings)
  5 siblings, 1 reply; 16+ messages in thread
From: Atish Patra @ 2025-05-05 21:39 UTC (permalink / raw)
  To: Anup Patel, Atish Patra, Paul Walmsley, Palmer Dabbelt,
	Alexandre Ghiti
  Cc: kvm, kvm-riscv, linux-riscv, linux-kernel, Atish Patra

Currently, we enable the smstateen bit at vcpu configure time by
only checking the presence of required ISA extensions.

These bits are not required to be enabled if the guest never uses
the corresponding architectural state. Enable the smstaeen bits
at runtime lazily upon first access.

Signed-off-by: Atish Patra <atishp@rivosinc.com>
---
 arch/riscv/include/asm/kvm_aia.h |  1 +
 arch/riscv/kvm/aia.c             | 43 ++++++++++++++++++++++++++++++++++++++++
 arch/riscv/kvm/aia_imsic.c       |  8 ++++++++
 3 files changed, 52 insertions(+)

diff --git a/arch/riscv/include/asm/kvm_aia.h b/arch/riscv/include/asm/kvm_aia.h
index 1f37b600ca47..760a1aef09f7 100644
--- a/arch/riscv/include/asm/kvm_aia.h
+++ b/arch/riscv/include/asm/kvm_aia.h
@@ -112,6 +112,7 @@ int kvm_riscv_aia_aplic_has_attr(struct kvm *kvm, unsigned long type);
 int kvm_riscv_aia_aplic_inject(struct kvm *kvm, u32 source, bool level);
 int kvm_riscv_aia_aplic_init(struct kvm *kvm);
 void kvm_riscv_aia_aplic_cleanup(struct kvm *kvm);
+bool kvm_riscv_aia_imsic_state_hw_backed(struct kvm_vcpu *vcpu);
 
 #ifdef CONFIG_32BIT
 void kvm_riscv_vcpu_aia_flush_interrupts(struct kvm_vcpu *vcpu);
diff --git a/arch/riscv/kvm/aia.c b/arch/riscv/kvm/aia.c
index 19afd1f23537..1e0d2217ade7 100644
--- a/arch/riscv/kvm/aia.c
+++ b/arch/riscv/kvm/aia.c
@@ -241,6 +241,8 @@ int kvm_riscv_vcpu_aia_rmw_topei(struct kvm_vcpu *vcpu,
 				 unsigned long new_val,
 				 unsigned long wr_mask)
 {
+	bool vsfile_present = kvm_riscv_aia_imsic_state_hw_backed(vcpu);
+
 	/* If AIA not available then redirect trap */
 	if (!kvm_riscv_aia_available())
 		return KVM_INSN_ILLEGAL_TRAP;
@@ -249,6 +251,26 @@ int kvm_riscv_vcpu_aia_rmw_topei(struct kvm_vcpu *vcpu,
 	if (!kvm_riscv_aia_initialized(vcpu->kvm))
 		return KVM_INSN_EXIT_TO_USER_SPACE;
 
+	/* Continue if smstaeen is not present */
+	if (!riscv_has_extension_unlikely(RISCV_ISA_EXT_SMSTATEEN))
+		goto skip_hstateen;
+
+	/* Enable the bit in hstateen0 lazily upon first access */
+	if (!(vcpu->arch.cfg.hstateen0 & SMSTATEEN0_AIA_IMSIC)) {
+		vcpu->arch.cfg.hstateen0 |= SMSTATEEN0_AIA_IMSIC;
+		if (IS_ENABLED(CONFIG_32BIT))
+			csr_set(CSR_HSTATEEN0H, SMSTATEEN0_AIA_IMSIC >> 32);
+		else
+			csr_set(CSR_HSTATEEN0, SMSTATEEN0_AIA_IMSIC);
+		if (vsfile_present)
+			return KVM_INSN_CONTINUE_SAME_SEPC;
+	} else if (vsfile_present) {
+		pr_err("Unexpected trap for CSR [%x] with hstateen0 enabled and valid vsfile\n",
+		       csr_num);
+		return KVM_INSN_EXIT_TO_USER_SPACE;
+	}
+
+skip_hstateen:
 	return kvm_riscv_vcpu_aia_imsic_rmw(vcpu, KVM_RISCV_AIA_IMSIC_TOPEI,
 					    val, new_val, wr_mask);
 }
@@ -400,11 +422,32 @@ int kvm_riscv_vcpu_aia_rmw_ireg(struct kvm_vcpu *vcpu, unsigned int csr_num,
 				unsigned long wr_mask)
 {
 	unsigned int isel;
+	bool vsfile_present = kvm_riscv_aia_imsic_state_hw_backed(vcpu);
 
 	/* If AIA not available then redirect trap */
 	if (!kvm_riscv_aia_available())
 		return KVM_INSN_ILLEGAL_TRAP;
 
+	/* Continue if smstaeen is not present */
+	if (!riscv_has_extension_unlikely(RISCV_ISA_EXT_SMSTATEEN))
+		goto skip_hstateen;
+
+	/* Enable the bit in hstateen0 lazily upon first access */
+	if (!(vcpu->arch.cfg.hstateen0 & SMSTATEEN0_AIA_ISEL)) {
+		vcpu->arch.cfg.hstateen0 |= SMSTATEEN0_AIA_ISEL;
+		if (IS_ENABLED(CONFIG_32BIT))
+			csr_set(CSR_HSTATEEN0H, SMSTATEEN0_AIA_ISEL >> 32);
+		else
+			csr_set(CSR_HSTATEEN0, SMSTATEEN0_AIA_ISEL);
+		if (vsfile_present)
+			return KVM_INSN_CONTINUE_SAME_SEPC;
+	} else if (vsfile_present) {
+		pr_err("Unexpected trap for CSR [%x] with hstateen0 enabled and valid vsfile\n",
+		       csr_num);
+		return KVM_INSN_EXIT_TO_USER_SPACE;
+	}
+
+skip_hstateen:
 	/* First try to emulate in kernel space */
 	isel = ncsr_read(CSR_VSISELECT) & ISELECT_MASK;
 	if (isel >= ISELECT_IPRIO0 && isel <= ISELECT_IPRIO15)
diff --git a/arch/riscv/kvm/aia_imsic.c b/arch/riscv/kvm/aia_imsic.c
index 29ef9c2133a9..d8e6f14850c0 100644
--- a/arch/riscv/kvm/aia_imsic.c
+++ b/arch/riscv/kvm/aia_imsic.c
@@ -361,6 +361,14 @@ static int imsic_mrif_rmw(struct imsic_mrif *mrif, u32 nr_eix,
 	return 0;
 }
 
+bool kvm_riscv_aia_imsic_state_hw_backed(struct kvm_vcpu *vcpu)
+{
+	struct kvm_vcpu_aia *vaia = &vcpu->arch.aia_context;
+	struct imsic *imsic = vaia->imsic_state;
+
+	return imsic && imsic->vsfile_cpu >= 0;
+}
+
 struct imsic_vsfile_read_data {
 	int hgei;
 	u32 nr_eix;

-- 
2.43.0


^ permalink raw reply related	[flat|nested] 16+ messages in thread

* [PATCH 2/5] RISC-V: KVM: Add a hstateen lazy enabler helper function
  2025-05-05 21:39 [PATCH 0/5] Enable hstateen bits lazily for the KVM RISC-V Guests Atish Patra
  2025-05-05 21:39 ` [PATCH 1/5] RISC-V: KVM: Lazy enable hstateen IMSIC & ISEL bit Atish Patra
@ 2025-05-05 21:39 ` Atish Patra
  2025-05-05 21:39 ` [PATCH 3/5] RISC-V: KVM: Support lazy enabling of siselect and aia bits Atish Patra
                   ` (3 subsequent siblings)
  5 siblings, 0 replies; 16+ messages in thread
From: Atish Patra @ 2025-05-05 21:39 UTC (permalink / raw)
  To: Anup Patel, Atish Patra, Paul Walmsley, Palmer Dabbelt,
	Alexandre Ghiti
  Cc: kvm, kvm-riscv, linux-riscv, linux-kernel, Atish Patra

Hstateen has different bits that can be enabled lazily at runtime.
Most of them have similar functionality where the hstateen bit must
be enabled if not enabled already. The correpsonding config bit in
vcpu must be enabled as well so that hstateen CSR is updated correctly
during the next vcpu load. In absesnce of Smstateen extension, exit
to the userspace in the trap because CSR access control exists
architecturally only if Smstateen extension is available.

Add a common helper function to achieve the above said objective.

Signed-off-by: Atish Patra <atishp@rivosinc.com>
---
 arch/riscv/include/asm/kvm_vcpu_insn.h |  4 ++++
 arch/riscv/kvm/vcpu_insn.c             | 21 +++++++++++++++++++++
 2 files changed, 25 insertions(+)

diff --git a/arch/riscv/include/asm/kvm_vcpu_insn.h b/arch/riscv/include/asm/kvm_vcpu_insn.h
index 350011c83581..1125f3f1c8c4 100644
--- a/arch/riscv/include/asm/kvm_vcpu_insn.h
+++ b/arch/riscv/include/asm/kvm_vcpu_insn.h
@@ -6,6 +6,8 @@
 #ifndef __KVM_VCPU_RISCV_INSN_H
 #define __KVM_VCPU_RISCV_INSN_H
 
+#include <linux/kvm_types.h>
+
 struct kvm_vcpu;
 struct kvm_run;
 struct kvm_cpu_trap;
@@ -44,5 +46,7 @@ int kvm_riscv_vcpu_mmio_store(struct kvm_vcpu *vcpu, struct kvm_run *run,
 			      unsigned long fault_addr,
 			      unsigned long htinst);
 int kvm_riscv_vcpu_mmio_return(struct kvm_vcpu *vcpu, struct kvm_run *run);
+int kvm_riscv_vcpu_hstateen_lazy_enable(struct kvm_vcpu *vcpu, unsigned int csr_num,
+					uint64_t hstateen_feature_bit_mask);
 
 #endif
diff --git a/arch/riscv/kvm/vcpu_insn.c b/arch/riscv/kvm/vcpu_insn.c
index 97dec18e6989..3bc39572b79d 100644
--- a/arch/riscv/kvm/vcpu_insn.c
+++ b/arch/riscv/kvm/vcpu_insn.c
@@ -235,6 +235,27 @@ static int seed_csr_rmw(struct kvm_vcpu *vcpu, unsigned int csr_num,
 	return KVM_INSN_EXIT_TO_USER_SPACE;
 }
 
+int kvm_riscv_vcpu_hstateen_lazy_enable(struct kvm_vcpu *vcpu, unsigned int csr_num,
+					uint64_t hstateen_feature_bit_mask)
+{
+	/* Access from VS shouldn't trap if smstaeen is not present */
+	if (!riscv_has_extension_unlikely(RISCV_ISA_EXT_SMSTATEEN))
+		return KVM_INSN_EXIT_TO_USER_SPACE;
+
+	/* Enable the bit in hstateen0 lazily upon first access */
+	if (!(vcpu->arch.cfg.hstateen0 & hstateen_feature_bit_mask)) {
+		vcpu->arch.cfg.hstateen0 |= hstateen_feature_bit_mask;
+		csr_set(CSR_HSTATEEN0, hstateen_feature_bit_mask);
+		if (IS_ENABLED(CONFIG_32BIT))
+			csr_set(CSR_HSTATEEN0H, hstateen_feature_bit_mask >> 32);
+	} else {
+		return KVM_INSN_EXIT_TO_USER_SPACE;
+	}
+
+	/* Let the guest retry the instruction read after hstateen0 is modified */
+	return KVM_INSN_CONTINUE_SAME_SEPC;
+}
+
 static const struct csr_func csr_funcs[] = {
 	KVM_RISCV_VCPU_AIA_CSR_FUNCS
 	KVM_RISCV_VCPU_HPMCOUNTER_CSR_FUNCS

-- 
2.43.0


^ permalink raw reply related	[flat|nested] 16+ messages in thread

* [PATCH 3/5] RISC-V: KVM: Support lazy enabling of siselect and aia bits
  2025-05-05 21:39 [PATCH 0/5] Enable hstateen bits lazily for the KVM RISC-V Guests Atish Patra
  2025-05-05 21:39 ` [PATCH 1/5] RISC-V: KVM: Lazy enable hstateen IMSIC & ISEL bit Atish Patra
  2025-05-05 21:39 ` [PATCH 2/5] RISC-V: KVM: Add a hstateen lazy enabler helper function Atish Patra
@ 2025-05-05 21:39 ` Atish Patra
  2025-05-05 21:39 ` [PATCH 4/5] RISC-V: KVM: Enable envcfg and sstateen bits lazily Atish Patra
                   ` (2 subsequent siblings)
  5 siblings, 0 replies; 16+ messages in thread
From: Atish Patra @ 2025-05-05 21:39 UTC (permalink / raw)
  To: Anup Patel, Atish Patra, Paul Walmsley, Palmer Dabbelt,
	Alexandre Ghiti
  Cc: kvm, kvm-riscv, linux-riscv, linux-kernel, Atish Patra

Smstateen extension controls the SISELECT and SIPH/SIEH register
through hstateen.AIA bit (58). Add lazy enabling support for those
bits.

Signed-off-by: Atish Patra <atishp@rivosinc.com>
---
 arch/riscv/include/asm/kvm_aia.h | 13 ++++++++++++-
 arch/riscv/kvm/aia.c             | 34 ++++++++++++++++++++++++++++++++++
 arch/riscv/kvm/vcpu_insn.c       |  3 +++
 3 files changed, 49 insertions(+), 1 deletion(-)

diff --git a/arch/riscv/include/asm/kvm_aia.h b/arch/riscv/include/asm/kvm_aia.h
index 760a1aef09f7..9e39b0e15169 100644
--- a/arch/riscv/include/asm/kvm_aia.h
+++ b/arch/riscv/include/asm/kvm_aia.h
@@ -142,12 +142,23 @@ int kvm_riscv_vcpu_aia_rmw_topei(struct kvm_vcpu *vcpu,
 				 unsigned long *val,
 				 unsigned long new_val,
 				 unsigned long wr_mask);
+int kvm_riscv_vcpu_aia_hstateen_enable(struct kvm_vcpu *vcpu,
+				       unsigned int csr_num, unsigned long *val,
+				       unsigned long new_val, unsigned long wr_mask);
+int kvm_riscv_vcpu_aia_rmw_isel(struct kvm_vcpu *vcpu, unsigned int csr_num, unsigned long *val,
+				unsigned long new_val, unsigned long wr_mask);
 int kvm_riscv_vcpu_aia_rmw_ireg(struct kvm_vcpu *vcpu, unsigned int csr_num,
 				unsigned long *val, unsigned long new_val,
 				unsigned long wr_mask);
 #define KVM_RISCV_VCPU_AIA_CSR_FUNCS \
 { .base = CSR_SIREG,      .count = 1, .func = kvm_riscv_vcpu_aia_rmw_ireg }, \
-{ .base = CSR_STOPEI,     .count = 1, .func = kvm_riscv_vcpu_aia_rmw_topei },
+{ .base = CSR_SISELECT,   .count = 1, .func = kvm_riscv_vcpu_aia_rmw_isel }, \
+{ .base = CSR_STOPEI,     .count = 1, .func = kvm_riscv_vcpu_aia_rmw_topei }, \
+{ .base = CSR_STOPI,      .count = 1, .func = kvm_riscv_vcpu_aia_hstateen_enable }, \
+
+#define KVM_RISCV_VCPU_AIA_CSR_32BIT_FUNCS \
+{ .base = CSR_SIPH,	  .count = 1, .func = kvm_riscv_vcpu_aia_hstateen_enable }, \
+{ .base = CSR_SIEH,	  .count = 1, .func = kvm_riscv_vcpu_aia_hstateen_enable }, \
 
 int kvm_riscv_vcpu_aia_update(struct kvm_vcpu *vcpu);
 void kvm_riscv_vcpu_aia_reset(struct kvm_vcpu *vcpu);
diff --git a/arch/riscv/kvm/aia.c b/arch/riscv/kvm/aia.c
index 1e0d2217ade7..3dfabf51a4d2 100644
--- a/arch/riscv/kvm/aia.c
+++ b/arch/riscv/kvm/aia.c
@@ -235,6 +235,40 @@ int kvm_riscv_vcpu_aia_set_csr(struct kvm_vcpu *vcpu,
 	return 0;
 }
 
+int kvm_riscv_vcpu_aia_hstateen_enable(struct kvm_vcpu *vcpu,
+				       unsigned int csr_num,
+				       unsigned long *val,
+				       unsigned long new_val,
+				       unsigned long wr_mask)
+{
+	/* If AIA not available then redirect trap */
+	if (!kvm_riscv_aia_available())
+		return KVM_INSN_ILLEGAL_TRAP;
+
+	/* If AIA not initialized then forward to user space */
+	if (!kvm_riscv_aia_initialized(vcpu->kvm))
+		return KVM_INSN_EXIT_TO_USER_SPACE;
+
+	return kvm_riscv_vcpu_hstateen_lazy_enable(vcpu, csr_num, SMSTATEEN0_AIA);
+}
+
+int kvm_riscv_vcpu_aia_rmw_isel(struct kvm_vcpu *vcpu,
+				unsigned int csr_num,
+				unsigned long *val,
+				unsigned long new_val,
+				unsigned long wr_mask)
+{
+	/* If AIA not available then redirect trap */
+	if (!kvm_riscv_aia_available())
+		return KVM_INSN_ILLEGAL_TRAP;
+
+	/* If AIA not initialized then forward to user space */
+	if (!kvm_riscv_aia_initialized(vcpu->kvm))
+		return KVM_INSN_EXIT_TO_USER_SPACE;
+
+	return kvm_riscv_vcpu_hstateen_lazy_enable(vcpu, csr_num, SMSTATEEN0_AIA_ISEL);
+}
+
 int kvm_riscv_vcpu_aia_rmw_topei(struct kvm_vcpu *vcpu,
 				 unsigned int csr_num,
 				 unsigned long *val,
diff --git a/arch/riscv/kvm/vcpu_insn.c b/arch/riscv/kvm/vcpu_insn.c
index 3bc39572b79d..c46907bfe42f 100644
--- a/arch/riscv/kvm/vcpu_insn.c
+++ b/arch/riscv/kvm/vcpu_insn.c
@@ -260,6 +260,9 @@ static const struct csr_func csr_funcs[] = {
 	KVM_RISCV_VCPU_AIA_CSR_FUNCS
 	KVM_RISCV_VCPU_HPMCOUNTER_CSR_FUNCS
 	{ .base = CSR_SEED, .count = 1, .func = seed_csr_rmw },
+#ifdef CONFIG_32BIT
+	KVM_RISCV_VCPU_AIA_CSR_32BIT_FUNCS
+#endif
 };
 
 /**

-- 
2.43.0


^ permalink raw reply related	[flat|nested] 16+ messages in thread

* [PATCH 4/5] RISC-V: KVM: Enable envcfg and sstateen bits lazily
  2025-05-05 21:39 [PATCH 0/5] Enable hstateen bits lazily for the KVM RISC-V Guests Atish Patra
                   ` (2 preceding siblings ...)
  2025-05-05 21:39 ` [PATCH 3/5] RISC-V: KVM: Support lazy enabling of siselect and aia bits Atish Patra
@ 2025-05-05 21:39 ` Atish Patra
  2025-05-08 13:32   ` Radim Krčmář
  2025-05-05 21:39 ` [PATCH 5/5] RISC-V: KVM: Remove the boot time enabling of hstateen bits Atish Patra
  2025-05-06  9:24 ` [PATCH 0/5] Enable hstateen bits lazily for the KVM RISC-V Guests Radim Krčmář
  5 siblings, 1 reply; 16+ messages in thread
From: Atish Patra @ 2025-05-05 21:39 UTC (permalink / raw)
  To: Anup Patel, Atish Patra, Paul Walmsley, Palmer Dabbelt,
	Alexandre Ghiti
  Cc: kvm, kvm-riscv, linux-riscv, linux-kernel, Atish Patra

SENVCFG and SSTATEEN CSRs are controlled by HSENVCFG(62) and
SSTATEEN0(63) bits in hstateen. Enable them lazily at runtime
instead of bootime.

Signed-off-by: Atish Patra <atishp@rivosinc.com>
---
 arch/riscv/kvm/vcpu_insn.c | 28 ++++++++++++++++++++++++++++
 1 file changed, 28 insertions(+)

diff --git a/arch/riscv/kvm/vcpu_insn.c b/arch/riscv/kvm/vcpu_insn.c
index c46907bfe42f..ed6302b1992b 100644
--- a/arch/riscv/kvm/vcpu_insn.c
+++ b/arch/riscv/kvm/vcpu_insn.c
@@ -256,9 +256,37 @@ int kvm_riscv_vcpu_hstateen_lazy_enable(struct kvm_vcpu *vcpu, unsigned int csr_
 	return KVM_INSN_CONTINUE_SAME_SEPC;
 }
 
+static int kvm_riscv_vcpu_hstateen_enable_senvcfg(struct kvm_vcpu *vcpu,
+						  unsigned int csr_num,
+						  unsigned long *val,
+						  unsigned long new_val,
+						  unsigned long wr_mask)
+{
+	return kvm_riscv_vcpu_hstateen_lazy_enable(vcpu, csr_num, SMSTATEEN0_HSENVCFG);
+}
+
+static int kvm_riscv_vcpu_hstateen_enable_stateen(struct kvm_vcpu *vcpu,
+						  unsigned int csr_num,
+						  unsigned long *val,
+						  unsigned long new_val,
+						  unsigned long wr_mask)
+{
+	const unsigned long *isa = vcpu->arch.isa;
+
+	if (riscv_isa_extension_available(isa, SMSTATEEN))
+		return kvm_riscv_vcpu_hstateen_lazy_enable(vcpu, csr_num, SMSTATEEN0_SSTATEEN0);
+	else
+		return KVM_INSN_EXIT_TO_USER_SPACE;
+}
+
+#define KVM_RISCV_VCPU_STATEEN_CSR_FUNCS \
+{ .base = CSR_SENVCFG,    .count = 1, .func = kvm_riscv_vcpu_hstateen_enable_senvcfg }, \
+{ .base = CSR_SSTATEEN0,  .count = 1, .func = kvm_riscv_vcpu_hstateen_enable_stateen },\
+
 static const struct csr_func csr_funcs[] = {
 	KVM_RISCV_VCPU_AIA_CSR_FUNCS
 	KVM_RISCV_VCPU_HPMCOUNTER_CSR_FUNCS
+	KVM_RISCV_VCPU_STATEEN_CSR_FUNCS
 	{ .base = CSR_SEED, .count = 1, .func = seed_csr_rmw },
 #ifdef CONFIG_32BIT
 	KVM_RISCV_VCPU_AIA_CSR_32BIT_FUNCS

-- 
2.43.0


^ permalink raw reply related	[flat|nested] 16+ messages in thread

* [PATCH 5/5] RISC-V: KVM: Remove the boot time enabling of hstateen bits
  2025-05-05 21:39 [PATCH 0/5] Enable hstateen bits lazily for the KVM RISC-V Guests Atish Patra
                   ` (3 preceding siblings ...)
  2025-05-05 21:39 ` [PATCH 4/5] RISC-V: KVM: Enable envcfg and sstateen bits lazily Atish Patra
@ 2025-05-05 21:39 ` Atish Patra
  2025-05-06  9:24 ` [PATCH 0/5] Enable hstateen bits lazily for the KVM RISC-V Guests Radim Krčmář
  5 siblings, 0 replies; 16+ messages in thread
From: Atish Patra @ 2025-05-05 21:39 UTC (permalink / raw)
  To: Anup Patel, Atish Patra, Paul Walmsley, Palmer Dabbelt,
	Alexandre Ghiti
  Cc: kvm, kvm-riscv, linux-riscv, linux-kernel, Atish Patra

All the existing hstateen bits can be enabled at runtime upon
first access now. Remove the default enabling at bootime now.

Signed-off-by: Atish Patra <atishp@rivosinc.com>
---
 arch/riscv/kvm/vcpu.c | 10 ----------
 1 file changed, 10 deletions(-)

diff --git a/arch/riscv/kvm/vcpu.c b/arch/riscv/kvm/vcpu.c
index 60d684c76c58..0aaa9e0e4a01 100644
--- a/arch/riscv/kvm/vcpu.c
+++ b/arch/riscv/kvm/vcpu.c
@@ -560,16 +560,6 @@ static void kvm_riscv_vcpu_setup_config(struct kvm_vcpu *vcpu)
 	    !riscv_isa_extension_available(isa, SVADE))
 		cfg->henvcfg |= ENVCFG_ADUE;
 
-	if (riscv_has_extension_unlikely(RISCV_ISA_EXT_SMSTATEEN)) {
-		cfg->hstateen0 |= SMSTATEEN0_HSENVCFG;
-		if (riscv_isa_extension_available(isa, SSAIA))
-			cfg->hstateen0 |= SMSTATEEN0_AIA_IMSIC |
-					  SMSTATEEN0_AIA |
-					  SMSTATEEN0_AIA_ISEL;
-		if (riscv_isa_extension_available(isa, SMSTATEEN))
-			cfg->hstateen0 |= SMSTATEEN0_SSTATEEN0;
-	}
-
 	cfg->hedeleg = KVM_HEDELEG_DEFAULT;
 	if (vcpu->guest_debug)
 		cfg->hedeleg &= ~BIT(EXC_BREAKPOINT);

-- 
2.43.0


^ permalink raw reply related	[flat|nested] 16+ messages in thread

* Re: [PATCH 0/5] Enable hstateen bits lazily for the KVM RISC-V Guests
  2025-05-05 21:39 [PATCH 0/5] Enable hstateen bits lazily for the KVM RISC-V Guests Atish Patra
                   ` (4 preceding siblings ...)
  2025-05-05 21:39 ` [PATCH 5/5] RISC-V: KVM: Remove the boot time enabling of hstateen bits Atish Patra
@ 2025-05-06  9:24 ` Radim Krčmář
  2025-05-06 18:24   ` Atish Patra
  5 siblings, 1 reply; 16+ messages in thread
From: Radim Krčmář @ 2025-05-06  9:24 UTC (permalink / raw)
  To: Atish Patra, Anup Patel, Atish Patra, Paul Walmsley,
	Palmer Dabbelt, Alexandre Ghiti
  Cc: kvm, kvm-riscv, linux-riscv, linux-kernel, linux-riscv

2025-05-05T14:39:25-07:00, Atish Patra <atishp@rivosinc.com>:
> This series adds support for enabling hstateen bits lazily at runtime
> instead of statically at bootime. The boot time enabling happens for
> all the guests if the required extensions are present in the host and/or
> guest. That may not be necessary if the guest never exercise that
> feature. We can enable the hstateen bits that controls the access lazily
> upon first access. This providers KVM more granular control of which
> feature is enabled in the guest at runtime.
>
> Currently, the following hstateen bits are supported to control the access
> from VS mode.
>
> 1. BIT(58): IMSIC     : STOPEI and IMSIC guest interrupt file
> 2. BIT(59): AIA       : SIPH/SIEH/STOPI
> 3. BIT(60): AIA_ISEL  : Indirect csr access via siselect/sireg
> 4. BIT(62): HSENVCFG  : SENVCFG access
> 5. BIT(63): SSTATEEN0 : SSTATEEN0 access
>
> KVM already support trap/enabling of BIT(58) and BIT(60) in order
> to support sw version of the guest interrupt file.

I don't think KVM toggles the hstateen bits at runtime, because that
would mean there is a bug even in current KVM.

>                                                    This series extends
> those to enable to correpsonding hstateen bits in PATCH1. The remaining
> patches adds lazy enabling support of the other bits.

The ISA has a peculiar design for hstateen/sstateen interaction:

  For every bit in an hstateen CSR that is zero (whether read-only zero
  or set to zero), the same bit appears as read-only zero in sstateen
  when accessed in VS-mode.

This means we must clear bit 63 in hstateen and trap on sstateen
accesses if any of the sstateen bits are not supposed to be read-only 0
to the guest while the hypervisor wants to have them as 0.

Thanks.

^ permalink raw reply	[flat|nested] 16+ messages in thread

* Re: [PATCH 0/5] Enable hstateen bits lazily for the KVM RISC-V Guests
  2025-05-06  9:24 ` [PATCH 0/5] Enable hstateen bits lazily for the KVM RISC-V Guests Radim Krčmář
@ 2025-05-06 18:24   ` Atish Patra
  2025-05-07 14:36     ` Radim Krčmář
  0 siblings, 1 reply; 16+ messages in thread
From: Atish Patra @ 2025-05-06 18:24 UTC (permalink / raw)
  To: Radim Krčmář, Anup Patel, Atish Patra,
	Paul Walmsley, Palmer Dabbelt, Alexandre Ghiti
  Cc: kvm, kvm-riscv, linux-riscv, linux-kernel, linux-riscv


On 5/6/25 2:24 AM, Radim Krčmář wrote:
> 2025-05-05T14:39:25-07:00, Atish Patra <atishp@rivosinc.com>:
>> This series adds support for enabling hstateen bits lazily at runtime
>> instead of statically at bootime. The boot time enabling happens for
>> all the guests if the required extensions are present in the host and/or
>> guest. That may not be necessary if the guest never exercise that
>> feature. We can enable the hstateen bits that controls the access lazily
>> upon first access. This providers KVM more granular control of which
>> feature is enabled in the guest at runtime.
>>
>> Currently, the following hstateen bits are supported to control the access
>> from VS mode.
>>
>> 1. BIT(58): IMSIC     : STOPEI and IMSIC guest interrupt file
>> 2. BIT(59): AIA       : SIPH/SIEH/STOPI
>> 3. BIT(60): AIA_ISEL  : Indirect csr access via siselect/sireg
>> 4. BIT(62): HSENVCFG  : SENVCFG access
>> 5. BIT(63): SSTATEEN0 : SSTATEEN0 access
>>
>> KVM already support trap/enabling of BIT(58) and BIT(60) in order
>> to support sw version of the guest interrupt file.
> I don't think KVM toggles the hstateen bits at runtime, because that
> would mean there is a bug even in current KVM.

This was a typo. I meant to say trap/emulate BIT(58) and BIT(60).
This patch series is trying to enable the toggling of the hstateen bits 
upon first access.

Sorry for the confusion.

>>                                                     This series extends
>> those to enable to correpsonding hstateen bits in PATCH1. The remaining
>> patches adds lazy enabling support of the other bits.
> The ISA has a peculiar design for hstateen/sstateen interaction:
>
>    For every bit in an hstateen CSR that is zero (whether read-only zero
>    or set to zero), the same bit appears as read-only zero in sstateen
>    when accessed in VS-mode.

Correct.

> This means we must clear bit 63 in hstateen and trap on sstateen
> accesses if any of the sstateen bits are not supposed to be read-only 0
> to the guest while the hypervisor wants to have them as 0.

Currently, there are two bits in sstateen. FCSR and ZVT which are not 
used anywhere in opensbi/Linux/KVM stack.

In case, we need to enable one of the bits in the future, does hypevisor 
need to trap every sstateen access ?
As per my understanding, it should be handled in the hardware and any 
write access to to those bits should be masked
with hstateen bit value so that it matches. That's what we do in Qemu as 
well.


> Thanks.

^ permalink raw reply	[flat|nested] 16+ messages in thread

* Re: [PATCH 0/5] Enable hstateen bits lazily for the KVM RISC-V Guests
  2025-05-06 18:24   ` Atish Patra
@ 2025-05-07 14:36     ` Radim Krčmář
  2025-05-08  0:34       ` Atish Patra
  0 siblings, 1 reply; 16+ messages in thread
From: Radim Krčmář @ 2025-05-07 14:36 UTC (permalink / raw)
  To: Atish Patra, Anup Patel, Atish Patra, Paul Walmsley,
	Palmer Dabbelt, Alexandre Ghiti
  Cc: kvm, kvm-riscv, linux-riscv, linux-kernel, linux-riscv

2025-05-06T11:24:41-07:00, Atish Patra <atish.patra@linux.dev>:
> On 5/6/25 2:24 AM, Radim Krčmář wrote:
>> 2025-05-05T14:39:25-07:00, Atish Patra <atishp@rivosinc.com>:
>>> This series adds support for enabling hstateen bits lazily at runtime
>>> instead of statically at bootime. The boot time enabling happens for
>>> all the guests if the required extensions are present in the host and/or
>>> guest. That may not be necessary if the guest never exercise that
>>> feature. We can enable the hstateen bits that controls the access lazily
>>> upon first access. This providers KVM more granular control of which
>>> feature is enabled in the guest at runtime.
>>>
>>> Currently, the following hstateen bits are supported to control the access
>>> from VS mode.
>>>
>>> 1. BIT(58): IMSIC     : STOPEI and IMSIC guest interrupt file
>>> 2. BIT(59): AIA       : SIPH/SIEH/STOPI
>>> 3. BIT(60): AIA_ISEL  : Indirect csr access via siselect/sireg
>>> 4. BIT(62): HSENVCFG  : SENVCFG access
>>> 5. BIT(63): SSTATEEN0 : SSTATEEN0 access
>>>
>>> KVM already support trap/enabling of BIT(58) and BIT(60) in order
>>> to support sw version of the guest interrupt file.
>> I don't think KVM toggles the hstateen bits at runtime, because that
>> would mean there is a bug even in current KVM.
>
> This was a typo. I meant to say trap/emulate BIT(58) and BIT(60).
> This patch series is trying to enable the toggling of the hstateen bits 
> upon first access.
>
> Sorry for the confusion.

No worries, it's my fault for misreading.
I got confused, because the code looked like generic lazy enablement,
while it's really only for the upper 32 bits and this series is not lazy
toggling any VS-mode visible bits.

>>>                                                     This series extends
>>> those to enable to correpsonding hstateen bits in PATCH1. The remaining
>>> patches adds lazy enabling support of the other bits.
>> The ISA has a peculiar design for hstateen/sstateen interaction:
>>
>>    For every bit in an hstateen CSR that is zero (whether read-only zero
>>    or set to zero), the same bit appears as read-only zero in sstateen
>>    when accessed in VS-mode.
>
> Correct.
>
>> This means we must clear bit 63 in hstateen and trap on sstateen
>> accesses if any of the sstateen bits are not supposed to be read-only 0
>> to the guest while the hypervisor wants to have them as 0.
>
> Currently, there are two bits in sstateen. FCSR and ZVT which are not 
> used anywhere in opensbi/Linux/KVM stack.

True, I guess we can just make sure the current code can't by mistake
lazily enable any of the bottom 32 hstateen bits and handle the case
properly later.

> In case, we need to enable one of the bits in the future, does hypevisor 
> need to trap every sstateen access ?

We need to trap sstateen accesses if the guest is supposed to be able to
control a bit in sstateen, but the hypervisor wants to lazily enable
that feature and sets 0 in hstateen until the first trap.

If hstateen is 1 for all features that the guest could control through
sstateen, we can and should just set the SE bit (63) to 1 as well.

> As per my understanding, it should be handled in the hardware and any 
> write access to to those bits should be masked
> with hstateen bit value so that it matches. That's what we do in Qemu as 
> well.

Right, hardware will do the job most of the time.  It's really only for
the lazy masking, beause if we don't trap the stateen accesses, they
would differ from what the guest should see.

^ permalink raw reply	[flat|nested] 16+ messages in thread

* Re: [PATCH 0/5] Enable hstateen bits lazily for the KVM RISC-V Guests
  2025-05-07 14:36     ` Radim Krčmář
@ 2025-05-08  0:34       ` Atish Patra
  2025-05-08 13:45         ` Radim Krčmář
  0 siblings, 1 reply; 16+ messages in thread
From: Atish Patra @ 2025-05-08  0:34 UTC (permalink / raw)
  To: Radim Krčmář, Anup Patel, Atish Patra,
	Paul Walmsley, Palmer Dabbelt, Alexandre Ghiti
  Cc: kvm, kvm-riscv, linux-riscv, linux-kernel, linux-riscv


On 5/7/25 7:36 AM, Radim Krčmář wrote:
> 2025-05-06T11:24:41-07:00, Atish Patra <atish.patra@linux.dev>:
>> On 5/6/25 2:24 AM, Radim Krčmář wrote:
>>> 2025-05-05T14:39:25-07:00, Atish Patra <atishp@rivosinc.com>:
>>>> This series adds support for enabling hstateen bits lazily at runtime
>>>> instead of statically at bootime. The boot time enabling happens for
>>>> all the guests if the required extensions are present in the host and/or
>>>> guest. That may not be necessary if the guest never exercise that
>>>> feature. We can enable the hstateen bits that controls the access lazily
>>>> upon first access. This providers KVM more granular control of which
>>>> feature is enabled in the guest at runtime.
>>>>
>>>> Currently, the following hstateen bits are supported to control the access
>>>> from VS mode.
>>>>
>>>> 1. BIT(58): IMSIC     : STOPEI and IMSIC guest interrupt file
>>>> 2. BIT(59): AIA       : SIPH/SIEH/STOPI
>>>> 3. BIT(60): AIA_ISEL  : Indirect csr access via siselect/sireg
>>>> 4. BIT(62): HSENVCFG  : SENVCFG access
>>>> 5. BIT(63): SSTATEEN0 : SSTATEEN0 access
>>>>
>>>> KVM already support trap/enabling of BIT(58) and BIT(60) in order
>>>> to support sw version of the guest interrupt file.
>>> I don't think KVM toggles the hstateen bits at runtime, because that
>>> would mean there is a bug even in current KVM.
>> This was a typo. I meant to say trap/emulate BIT(58) and BIT(60).
>> This patch series is trying to enable the toggling of the hstateen bits
>> upon first access.
>>
>> Sorry for the confusion.
> No worries, it's my fault for misreading.
> I got confused, because the code looked like generic lazy enablement,
> while it's really only for the upper 32 bits and this series is not lazy
> toggling any VS-mode visible bits.
>
>>>>                                                      This series extends
>>>> those to enable to correpsonding hstateen bits in PATCH1. The remaining
>>>> patches adds lazy enabling support of the other bits.
>>> The ISA has a peculiar design for hstateen/sstateen interaction:
>>>
>>>     For every bit in an hstateen CSR that is zero (whether read-only zero
>>>     or set to zero), the same bit appears as read-only zero in sstateen
>>>     when accessed in VS-mode.
>> Correct.
>>
>>> This means we must clear bit 63 in hstateen and trap on sstateen
>>> accesses if any of the sstateen bits are not supposed to be read-only 0
>>> to the guest while the hypervisor wants to have them as 0.
>> Currently, there are two bits in sstateen. FCSR and ZVT which are not
>> used anywhere in opensbi/Linux/KVM stack.
> True, I guess we can just make sure the current code can't by mistake
> lazily enable any of the bottom 32 hstateen bits and handle the case
> properly later.

I can update the cover letter and leave a comment about that.

Do you want a additional check in sstateen 
trap(kvm_riscv_vcpu_hstateen_enable_stateen)
to make sure that the new value doesn't have any bits set that is not 
permitted by the hypervisor ?

>> In case, we need to enable one of the bits in the future, does hypevisor
>> need to trap every sstateen access ?
> We need to trap sstateen accesses if the guest is supposed to be able to
> control a bit in sstateen, but the hypervisor wants to lazily enable
> that feature and sets 0 in hstateen until the first trap.
Yes. That's what PATCH 4 in this series does.
> If hstateen is 1 for all features that the guest could control through
> sstateen, we can and should just set the SE bit (63) to 1 as well.
>
>> As per my understanding, it should be handled in the hardware and any
>> write access to to those bits should be masked
>> with hstateen bit value so that it matches. That's what we do in Qemu as
>> well.
> Right, hardware will do the job most of the time.  It's really only for
> the lazy masking, beause if we don't trap the stateen accesses, they
> would differ from what the guest should see.

^ permalink raw reply	[flat|nested] 16+ messages in thread

* Re: [PATCH 1/5] RISC-V: KVM: Lazy enable hstateen IMSIC & ISEL bit
  2025-05-05 21:39 ` [PATCH 1/5] RISC-V: KVM: Lazy enable hstateen IMSIC & ISEL bit Atish Patra
@ 2025-05-08 13:31   ` Radim Krčmář
  0 siblings, 0 replies; 16+ messages in thread
From: Radim Krčmář @ 2025-05-08 13:31 UTC (permalink / raw)
  To: Atish Patra, Anup Patel, Atish Patra, Paul Walmsley,
	Palmer Dabbelt, Alexandre Ghiti
  Cc: kvm, kvm-riscv, linux-riscv, linux-kernel, linux-riscv

2025-05-05T14:39:26-07:00, Atish Patra <atishp@rivosinc.com>:
> Currently, we enable the smstateen bit at vcpu configure time by
> only checking the presence of required ISA extensions.
>
> These bits are not required to be enabled if the guest never uses
> the corresponding architectural state. Enable the smstaeen bits
> at runtime lazily upon first access.

What is the advantage of enabling them lazily?

To make the trap useful, we would have to lazily perform initialization
of the AIA.  I think it would require notable changes to AIA, though...

Thanks.

^ permalink raw reply	[flat|nested] 16+ messages in thread

* Re: [PATCH 4/5] RISC-V: KVM: Enable envcfg and sstateen bits lazily
  2025-05-05 21:39 ` [PATCH 4/5] RISC-V: KVM: Enable envcfg and sstateen bits lazily Atish Patra
@ 2025-05-08 13:32   ` Radim Krčmář
  2025-05-09 22:38     ` Atish Patra
  0 siblings, 1 reply; 16+ messages in thread
From: Radim Krčmář @ 2025-05-08 13:32 UTC (permalink / raw)
  To: Atish Patra, Anup Patel, Atish Patra, Paul Walmsley,
	Palmer Dabbelt, Alexandre Ghiti
  Cc: kvm, kvm-riscv, linux-riscv, linux-kernel, linux-riscv

2025-05-05T14:39:29-07:00, Atish Patra <atishp@rivosinc.com>:
> SENVCFG and SSTATEEN CSRs are controlled by HSENVCFG(62) and
> SSTATEEN0(63) bits in hstateen. Enable them lazily at runtime
> instead of bootime.
>
> Signed-off-by: Atish Patra <atishp@rivosinc.com>
> ---
> diff --git a/arch/riscv/kvm/vcpu_insn.c b/arch/riscv/kvm/vcpu_insn.c
> @@ -256,9 +256,37 @@ int kvm_riscv_vcpu_hstateen_lazy_enable(struct kvm_vcpu *vcpu, unsigned int csr_
>  	return KVM_INSN_CONTINUE_SAME_SEPC;
>  }
>  
> +static int kvm_riscv_vcpu_hstateen_enable_senvcfg(struct kvm_vcpu *vcpu,
> +						  unsigned int csr_num,
> +						  unsigned long *val,
> +						  unsigned long new_val,
> +						  unsigned long wr_mask)
> +{
> +	return kvm_riscv_vcpu_hstateen_lazy_enable(vcpu, csr_num, SMSTATEEN0_HSENVCFG);
> +}

Basically the same comments as for [1/5]:

Why don't we want to set the ENVCFG bit (62) unconditionally?

It would save us the trap on first access.  We don't get anything from
the trap, so it looks like a net negative to me.

> +
> +static int kvm_riscv_vcpu_hstateen_enable_stateen(struct kvm_vcpu *vcpu,
> +						  unsigned int csr_num,
> +						  unsigned long *val,
> +						  unsigned long new_val,
> +						  unsigned long wr_mask)
> +{
> +	const unsigned long *isa = vcpu->arch.isa;
> +
> +	if (riscv_isa_extension_available(isa, SMSTATEEN))
> +		return kvm_riscv_vcpu_hstateen_lazy_enable(vcpu, csr_num, SMSTATEEN0_SSTATEEN0);
> +	else
> +		return KVM_INSN_EXIT_TO_USER_SPACE;
> +}

The same argument applies to the SE0 bit (63) when the guest has the
sstateen extension.

KVM doesn't want to do anything other than stop trapping and reenter, so
it seems to me we could just not trap in the first place.

Thanks.

^ permalink raw reply	[flat|nested] 16+ messages in thread

* Re: [PATCH 0/5] Enable hstateen bits lazily for the KVM RISC-V Guests
  2025-05-08  0:34       ` Atish Patra
@ 2025-05-08 13:45         ` Radim Krčmář
  2025-05-09 22:26           ` Atish Patra
  0 siblings, 1 reply; 16+ messages in thread
From: Radim Krčmář @ 2025-05-08 13:45 UTC (permalink / raw)
  To: Atish Patra, Anup Patel, Atish Patra, Paul Walmsley,
	Palmer Dabbelt, Alexandre Ghiti
  Cc: kvm, kvm-riscv, linux-riscv, linux-kernel, linux-riscv

2025-05-07T17:34:38-07:00, Atish Patra <atish.patra@linux.dev>:
> On 5/7/25 7:36 AM, Radim Krčmář wrote:
>> 2025-05-06T11:24:41-07:00, Atish Patra <atish.patra@linux.dev>:
>>> On 5/6/25 2:24 AM, Radim Krčmář wrote:
>>>> 2025-05-05T14:39:25-07:00, Atish Patra <atishp@rivosinc.com>:
>>>>>                                                      This series extends
>>>>> those to enable to correpsonding hstateen bits in PATCH1. The remaining
>>>>> patches adds lazy enabling support of the other bits.
>>>> The ISA has a peculiar design for hstateen/sstateen interaction:
>>>>
>>>>     For every bit in an hstateen CSR that is zero (whether read-only zero
>>>>     or set to zero), the same bit appears as read-only zero in sstateen
>>>>     when accessed in VS-mode.
>>> Correct.
>>>
>>>> This means we must clear bit 63 in hstateen and trap on sstateen
>>>> accesses if any of the sstateen bits are not supposed to be read-only 0
>>>> to the guest while the hypervisor wants to have them as 0.
>>> Currently, there are two bits in sstateen. FCSR and ZVT which are not
>>> used anywhere in opensbi/Linux/KVM stack.
>> True, I guess we can just make sure the current code can't by mistake
>> lazily enable any of the bottom 32 hstateen bits and handle the case
>> properly later.
>
> I can update the cover letter and leave a comment about that.
>
> Do you want a additional check in sstateen 
> trap(kvm_riscv_vcpu_hstateen_enable_stateen)
> to make sure that the new value doesn't have any bits set that is not 
> permitted by the hypervisor ?

I wanted to prevent kvm_riscv_vcpu_hstateen_lazy_enable() from being
able to modify the bottom 32 bits, because they are guest-visible and
KVM does not handle them correctly -- it's an internal KVM error that
should be made obvious to future programmers.

>>> In case, we need to enable one of the bits in the future, does hypevisor
>>> need to trap every sstateen access ?
>> We need to trap sstateen accesses if the guest is supposed to be able to
>> control a bit in sstateen, but the hypervisor wants to lazily enable
>> that feature and sets 0 in hstateen until the first trap.
> Yes. That's what PATCH 4 in this series does.

I was thinking about the correct emulation.

e.g. guest sets sstateen bit X to 1, but KVM wants to handle the feature
X lazily, which means that hstateen bit X is 0.
hstateen bit SE0 must be 0 in that case, because KVM must trap the guest
access to bit X and properly emulate it.
When the guest accesses a feature controlled by sstateen bit X, KVM will
lazily enable the feature and then set sstateen and hstateen bit X.

^ permalink raw reply	[flat|nested] 16+ messages in thread

* Re: [PATCH 0/5] Enable hstateen bits lazily for the KVM RISC-V Guests
  2025-05-08 13:45         ` Radim Krčmář
@ 2025-05-09 22:26           ` Atish Patra
  0 siblings, 0 replies; 16+ messages in thread
From: Atish Patra @ 2025-05-09 22:26 UTC (permalink / raw)
  To: Radim Krčmář, Anup Patel, Atish Patra,
	Paul Walmsley, Palmer Dabbelt, Alexandre Ghiti
  Cc: kvm, kvm-riscv, linux-riscv, linux-kernel, linux-riscv


On 5/8/25 6:45 AM, Radim Krčmář wrote:
> 2025-05-07T17:34:38-07:00, Atish Patra <atish.patra@linux.dev>:
>> On 5/7/25 7:36 AM, Radim Krčmář wrote:
>>> 2025-05-06T11:24:41-07:00, Atish Patra <atish.patra@linux.dev>:
>>>> On 5/6/25 2:24 AM, Radim Krčmář wrote:
>>>>> 2025-05-05T14:39:25-07:00, Atish Patra <atishp@rivosinc.com>:
>>>>>>                                                       This series extends
>>>>>> those to enable to correpsonding hstateen bits in PATCH1. The remaining
>>>>>> patches adds lazy enabling support of the other bits.
>>>>> The ISA has a peculiar design for hstateen/sstateen interaction:
>>>>>
>>>>>      For every bit in an hstateen CSR that is zero (whether read-only zero
>>>>>      or set to zero), the same bit appears as read-only zero in sstateen
>>>>>      when accessed in VS-mode.
>>>> Correct.
>>>>
>>>>> This means we must clear bit 63 in hstateen and trap on sstateen
>>>>> accesses if any of the sstateen bits are not supposed to be read-only 0
>>>>> to the guest while the hypervisor wants to have them as 0.
>>>> Currently, there are two bits in sstateen. FCSR and ZVT which are not
>>>> used anywhere in opensbi/Linux/KVM stack.
>>> True, I guess we can just make sure the current code can't by mistake
>>> lazily enable any of the bottom 32 hstateen bits and handle the case
>>> properly later.
>> I can update the cover letter and leave a comment about that.
>>
>> Do you want a additional check in sstateen
>> trap(kvm_riscv_vcpu_hstateen_enable_stateen)
>> to make sure that the new value doesn't have any bits set that is not
>> permitted by the hypervisor ?
> I wanted to prevent kvm_riscv_vcpu_hstateen_lazy_enable() from being
> able to modify the bottom 32 bits, because they are guest-visible and
> KVM does not handle them correctly -- it's an internal KVM error that
> should be made obvious to future programmers.

Sure. I will add something along those lines.


>>>> In case, we need to enable one of the bits in the future, does hypevisor
>>>> need to trap every sstateen access ?
>>> We need to trap sstateen accesses if the guest is supposed to be able to
>>> control a bit in sstateen, but the hypervisor wants to lazily enable
>>> that feature and sets 0 in hstateen until the first trap.
>> Yes. That's what PATCH 4 in this series does.
> I was thinking about the correct emulation.
>
> e.g. guest sets sstateen bit X to 1, but KVM wants to handle the feature
> X lazily, which means that hstateen bit X is 0.
> hstateen bit SE0 must be 0 in that case, because KVM must trap the guest
> access to bit X and properly emulate it.
> When the guest accesses a feature controlled by sstateen bit X, KVM will
> lazily enable the feature and then set sstateen and hstateen bit X.

Yeah. That's possible. The current series is just trying to trap & 
enable rather
than trap & emulate except for few AIA related bits which trap even with 
hstateen
bit set due to sw file instead of vsfile.

Once we have such requirement any other feature bit, we can extend the 
generic
trap & enable framework to trap & emulate.


^ permalink raw reply	[flat|nested] 16+ messages in thread

* Re: [PATCH 4/5] RISC-V: KVM: Enable envcfg and sstateen bits lazily
  2025-05-08 13:32   ` Radim Krčmář
@ 2025-05-09 22:38     ` Atish Patra
  2025-05-12 10:25       ` Radim Krčmář
  0 siblings, 1 reply; 16+ messages in thread
From: Atish Patra @ 2025-05-09 22:38 UTC (permalink / raw)
  To: Radim Krčmář, Anup Patel, Atish Patra,
	Paul Walmsley, Palmer Dabbelt, Alexandre Ghiti
  Cc: kvm, kvm-riscv, linux-riscv, linux-kernel, linux-riscv


On 5/8/25 6:32 AM, Radim Krčmář wrote:
> 2025-05-05T14:39:29-07:00, Atish Patra <atishp@rivosinc.com>:
>> SENVCFG and SSTATEEN CSRs are controlled by HSENVCFG(62) and
>> SSTATEEN0(63) bits in hstateen. Enable them lazily at runtime
>> instead of bootime.
>>
>> Signed-off-by: Atish Patra <atishp@rivosinc.com>
>> ---
>> diff --git a/arch/riscv/kvm/vcpu_insn.c b/arch/riscv/kvm/vcpu_insn.c
>> @@ -256,9 +256,37 @@ int kvm_riscv_vcpu_hstateen_lazy_enable(struct kvm_vcpu *vcpu, unsigned int csr_
>>   	return KVM_INSN_CONTINUE_SAME_SEPC;
>>   }
>>   
>> +static int kvm_riscv_vcpu_hstateen_enable_senvcfg(struct kvm_vcpu *vcpu,
>> +						  unsigned int csr_num,
>> +						  unsigned long *val,
>> +						  unsigned long new_val,
>> +						  unsigned long wr_mask)
>> +{
>> +	return kvm_riscv_vcpu_hstateen_lazy_enable(vcpu, csr_num, SMSTATEEN0_HSENVCFG);
>> +}
> Basically the same comments as for [1/5]:
>
> Why don't we want to set the ENVCFG bit (62) unconditionally?
>
> It would save us the trap on first access.  We don't get anything from
> the trap, so it looks like a net negative to me.

We want to lazy enablement is to make sure that hypervisor is aware of 
the what features
guest is using. We don't want to necessarily enable the architecture 
states for the guest if guest doesn't need it.

We need lazy enablement for CTR like features anyways. This will align 
all the the features controlled
by stateen in the same manner. The cost is just a single trap at the 
boot time.

IMO, it's fair trade off.
>> +
>> +static int kvm_riscv_vcpu_hstateen_enable_stateen(struct kvm_vcpu *vcpu,
>> +						  unsigned int csr_num,
>> +						  unsigned long *val,
>> +						  unsigned long new_val,
>> +						  unsigned long wr_mask)
>> +{
>> +	const unsigned long *isa = vcpu->arch.isa;
>> +
>> +	if (riscv_isa_extension_available(isa, SMSTATEEN))
>> +		return kvm_riscv_vcpu_hstateen_lazy_enable(vcpu, csr_num, SMSTATEEN0_SSTATEEN0);
>> +	else
>> +		return KVM_INSN_EXIT_TO_USER_SPACE;
>> +}
> The same argument applies to the SE0 bit (63) when the guest has the
> sstateen extension.
>
> KVM doesn't want to do anything other than stop trapping and reenter, so
> it seems to me we could just not trap in the first place.
>
> Thanks.

^ permalink raw reply	[flat|nested] 16+ messages in thread

* Re: [PATCH 4/5] RISC-V: KVM: Enable envcfg and sstateen bits lazily
  2025-05-09 22:38     ` Atish Patra
@ 2025-05-12 10:25       ` Radim Krčmář
  0 siblings, 0 replies; 16+ messages in thread
From: Radim Krčmář @ 2025-05-12 10:25 UTC (permalink / raw)
  To: Atish Patra, Anup Patel, Atish Patra, Paul Walmsley,
	Palmer Dabbelt, Alexandre Ghiti
  Cc: kvm, kvm-riscv, linux-riscv, linux-kernel, linux-riscv

2025-05-09T15:38:55-07:00, Atish Patra <atish.patra@linux.dev>:
> On 5/8/25 6:32 AM, Radim Krčmář wrote:
>> 2025-05-05T14:39:29-07:00, Atish Patra <atishp@rivosinc.com>:
>>> SENVCFG and SSTATEEN CSRs are controlled by HSENVCFG(62) and
>>> SSTATEEN0(63) bits in hstateen. Enable them lazily at runtime
>>> instead of bootime.
>>>
>>> Signed-off-by: Atish Patra <atishp@rivosinc.com>
>>> ---
>>> diff --git a/arch/riscv/kvm/vcpu_insn.c b/arch/riscv/kvm/vcpu_insn.c
>>> @@ -256,9 +256,37 @@ int kvm_riscv_vcpu_hstateen_lazy_enable(struct kvm_vcpu *vcpu, unsigned int csr_
>>>   	return KVM_INSN_CONTINUE_SAME_SEPC;
>>>   }
>>>   
>>> +static int kvm_riscv_vcpu_hstateen_enable_senvcfg(struct kvm_vcpu *vcpu,
>>> +						  unsigned int csr_num,
>>> +						  unsigned long *val,
>>> +						  unsigned long new_val,
>>> +						  unsigned long wr_mask)
>>> +{
>>> +	return kvm_riscv_vcpu_hstateen_lazy_enable(vcpu, csr_num, SMSTATEEN0_HSENVCFG);
>>> +}
>> Basically the same comments as for [1/5]:
>>
>> Why don't we want to set the ENVCFG bit (62) unconditionally?
>>
>> It would save us the trap on first access.  We don't get anything from
>> the trap, so it looks like a net negative to me.
>
> We want to lazy enablement is to make sure that hypervisor is aware of 
> the what features
> guest is using. We don't want to necessarily enable the architecture 
> states for the guest if guest doesn't need it.
>
> We need lazy enablement for CTR like features anyways. This will align 
> all the the features controlled
> by stateen in the same manner. The cost is just a single trap at the 
> boot time.
>
> IMO, it's fair trade off.

Yeah, as long as we are doing something with the information from the
trap.

^ permalink raw reply	[flat|nested] 16+ messages in thread

end of thread, other threads:[~2025-05-12 10:25 UTC | newest]

Thread overview: 16+ messages (download: mbox.gz follow: Atom feed
-- links below jump to the message on this page --
2025-05-05 21:39 [PATCH 0/5] Enable hstateen bits lazily for the KVM RISC-V Guests Atish Patra
2025-05-05 21:39 ` [PATCH 1/5] RISC-V: KVM: Lazy enable hstateen IMSIC & ISEL bit Atish Patra
2025-05-08 13:31   ` Radim Krčmář
2025-05-05 21:39 ` [PATCH 2/5] RISC-V: KVM: Add a hstateen lazy enabler helper function Atish Patra
2025-05-05 21:39 ` [PATCH 3/5] RISC-V: KVM: Support lazy enabling of siselect and aia bits Atish Patra
2025-05-05 21:39 ` [PATCH 4/5] RISC-V: KVM: Enable envcfg and sstateen bits lazily Atish Patra
2025-05-08 13:32   ` Radim Krčmář
2025-05-09 22:38     ` Atish Patra
2025-05-12 10:25       ` Radim Krčmář
2025-05-05 21:39 ` [PATCH 5/5] RISC-V: KVM: Remove the boot time enabling of hstateen bits Atish Patra
2025-05-06  9:24 ` [PATCH 0/5] Enable hstateen bits lazily for the KVM RISC-V Guests Radim Krčmář
2025-05-06 18:24   ` Atish Patra
2025-05-07 14:36     ` Radim Krčmář
2025-05-08  0:34       ` Atish Patra
2025-05-08 13:45         ` Radim Krčmář
2025-05-09 22:26           ` Atish Patra

This is a public inbox, see mirroring instructions
for how to clone and mirror all data and code used for this inbox