This series adds ONE_REG interface for SBI FWFT extension implemented by KVM RISC-V. This was missed out in accepted SBI FWFT patches for KVM RISC-V.
These patches can also be found in the riscv_kvm_fwft_one_reg_v1 branch at: https://github.com/avpatel/linux.git
Anup Patel (6): RISC-V: KVM: Set initial value of hedeleg in kvm_arch_vcpu_create() RISC-V: KVM: Introduce feature specific reset for SBI FWFT RISC-V: KVM: Introduce optional ONE_REG callbacks for SBI extensions RISC-V: KVM: Move copy_sbi_ext_reg_indices() to SBI implementation RISC-V: KVM: Implement ONE_REG interface for SBI FWFT state KVM: riscv: selftests: Add SBI FWFT to get-reg-list test
arch/riscv/include/asm/kvm_vcpu_sbi.h | 23 +- arch/riscv/include/uapi/asm/kvm.h | 14 ++ arch/riscv/kvm/vcpu.c | 3 +- arch/riscv/kvm/vcpu_onereg.c | 60 +----- arch/riscv/kvm/vcpu_sbi.c | 172 ++++++++++++--- arch/riscv/kvm/vcpu_sbi_fwft.c | 199 ++++++++++++++++-- arch/riscv/kvm/vcpu_sbi_sta.c | 64 ++++-- .../selftests/kvm/riscv/get-reg-list.c | 28 +++ 8 files changed, 436 insertions(+), 127 deletions(-)
The hedeleg may be updated by ONE_REG interface before the VCPU is run at least once hence set the initial value of hedeleg in kvm_arch_vcpu_create() instead of kvm_riscv_vcpu_setup_config().
Signed-off-by: Anup Patel apatel@ventanamicro.com --- arch/riscv/kvm/vcpu.c | 3 ++- 1 file changed, 2 insertions(+), 1 deletion(-)
diff --git a/arch/riscv/kvm/vcpu.c b/arch/riscv/kvm/vcpu.c index f001e56403f9..86025f68c374 100644 --- a/arch/riscv/kvm/vcpu.c +++ b/arch/riscv/kvm/vcpu.c @@ -133,6 +133,8 @@ int kvm_arch_vcpu_create(struct kvm_vcpu *vcpu)
/* Mark this VCPU never ran */ vcpu->arch.ran_atleast_once = false; + + vcpu->arch.cfg.hedeleg = KVM_HEDELEG_DEFAULT; vcpu->arch.mmu_page_cache.gfp_zero = __GFP_ZERO; bitmap_zero(vcpu->arch.isa, RISCV_ISA_EXT_MAX);
@@ -570,7 +572,6 @@ static void kvm_riscv_vcpu_setup_config(struct kvm_vcpu *vcpu) cfg->hstateen0 |= SMSTATEEN0_SSTATEEN0; }
- cfg->hedeleg = KVM_HEDELEG_DEFAULT; if (vcpu->guest_debug) cfg->hedeleg &= ~BIT(EXC_BREAKPOINT); }
On Thu, Aug 14, 2025 at 09:25:43PM +0530, Anup Patel wrote:
The hedeleg may be updated by ONE_REG interface before the VCPU is run at least once hence set the initial value of hedeleg in kvm_arch_vcpu_create() instead of kvm_riscv_vcpu_setup_config().
Signed-off-by: Anup Patel apatel@ventanamicro.com
arch/riscv/kvm/vcpu.c | 3 ++- 1 file changed, 2 insertions(+), 1 deletion(-)
diff --git a/arch/riscv/kvm/vcpu.c b/arch/riscv/kvm/vcpu.c index f001e56403f9..86025f68c374 100644 --- a/arch/riscv/kvm/vcpu.c +++ b/arch/riscv/kvm/vcpu.c @@ -133,6 +133,8 @@ int kvm_arch_vcpu_create(struct kvm_vcpu *vcpu) /* Mark this VCPU never ran */ vcpu->arch.ran_atleast_once = false;
- vcpu->arch.cfg.hedeleg = KVM_HEDELEG_DEFAULT; vcpu->arch.mmu_page_cache.gfp_zero = __GFP_ZERO; bitmap_zero(vcpu->arch.isa, RISCV_ISA_EXT_MAX);
@@ -570,7 +572,6 @@ static void kvm_riscv_vcpu_setup_config(struct kvm_vcpu *vcpu) cfg->hstateen0 |= SMSTATEEN0_SSTATEEN0; }
- cfg->hedeleg = KVM_HEDELEG_DEFAULT; if (vcpu->guest_debug) cfg->hedeleg &= ~BIT(EXC_BREAKPOINT);
}
2.43.0
Reviewed-by: Andrew Jones ajones@ventanamicro.com
On 8/14/2025 11:55 PM, Anup Patel wrote:
The hedeleg may be updated by ONE_REG interface before the VCPU is run at least once hence set the initial value of hedeleg in kvm_arch_vcpu_create() instead of kvm_riscv_vcpu_setup_config().
Signed-off-by: Anup Patel apatel@ventanamicro.com
arch/riscv/kvm/vcpu.c | 3 ++- 1 file changed, 2 insertions(+), 1 deletion(-)
Reviewed-by: Nutty Liu nutty.liu@hotmail.com
Thanks, Nutty
The SBI FWFT feature values must be reset upon VCPU reset so introduce feature specific reset callback for this purpose.
Signed-off-by: Anup Patel apatel@ventanamicro.com --- arch/riscv/kvm/vcpu_sbi_fwft.c | 30 ++++++++++++++++++++++++++++-- 1 file changed, 28 insertions(+), 2 deletions(-)
diff --git a/arch/riscv/kvm/vcpu_sbi_fwft.c b/arch/riscv/kvm/vcpu_sbi_fwft.c index 164a01288b0a..5a3bad0f9330 100644 --- a/arch/riscv/kvm/vcpu_sbi_fwft.c +++ b/arch/riscv/kvm/vcpu_sbi_fwft.c @@ -30,6 +30,13 @@ struct kvm_sbi_fwft_feature { */ bool (*supported)(struct kvm_vcpu *vcpu);
+ /** + * @reset: Reset the feature value irrespective whether feature is supported or not + * + * This callback is mandatory + */ + void (*reset)(struct kvm_vcpu *vcpu); + /** * @set: Set the feature value * @@ -75,6 +82,13 @@ static bool kvm_sbi_fwft_misaligned_delegation_supported(struct kvm_vcpu *vcpu) return misaligned_traps_can_delegate(); }
+static void kvm_sbi_fwft_reset_misaligned_delegation(struct kvm_vcpu *vcpu) +{ + struct kvm_vcpu_config *cfg = &vcpu->arch.cfg; + + cfg->hedeleg &= ~MIS_DELEG; +} + static long kvm_sbi_fwft_set_misaligned_delegation(struct kvm_vcpu *vcpu, struct kvm_sbi_fwft_config *conf, unsigned long value) @@ -124,6 +138,11 @@ static bool kvm_sbi_fwft_pointer_masking_pmlen_supported(struct kvm_vcpu *vcpu) return fwft->have_vs_pmlen_7 || fwft->have_vs_pmlen_16; }
+static void kvm_sbi_fwft_reset_pointer_masking_pmlen(struct kvm_vcpu *vcpu) +{ + vcpu->arch.cfg.henvcfg &= ~ENVCFG_PMM; +} + static long kvm_sbi_fwft_set_pointer_masking_pmlen(struct kvm_vcpu *vcpu, struct kvm_sbi_fwft_config *conf, unsigned long value) @@ -180,6 +199,7 @@ static const struct kvm_sbi_fwft_feature features[] = { { .id = SBI_FWFT_MISALIGNED_EXC_DELEG, .supported = kvm_sbi_fwft_misaligned_delegation_supported, + .reset = kvm_sbi_fwft_reset_misaligned_delegation, .set = kvm_sbi_fwft_set_misaligned_delegation, .get = kvm_sbi_fwft_get_misaligned_delegation, }, @@ -187,6 +207,7 @@ static const struct kvm_sbi_fwft_feature features[] = { { .id = SBI_FWFT_POINTER_MASKING_PMLEN, .supported = kvm_sbi_fwft_pointer_masking_pmlen_supported, + .reset = kvm_sbi_fwft_reset_pointer_masking_pmlen, .set = kvm_sbi_fwft_set_pointer_masking_pmlen, .get = kvm_sbi_fwft_get_pointer_masking_pmlen, }, @@ -321,11 +342,16 @@ static void kvm_sbi_ext_fwft_deinit(struct kvm_vcpu *vcpu)
static void kvm_sbi_ext_fwft_reset(struct kvm_vcpu *vcpu) { - int i; struct kvm_sbi_fwft *fwft = vcpu_to_fwft(vcpu); + const struct kvm_sbi_fwft_feature *feature; + int i;
- for (i = 0; i < ARRAY_SIZE(features); i++) + for (i = 0; i < ARRAY_SIZE(features); i++) { fwft->configs[i].flags = 0; + feature = &features[i]; + if (feature->reset) + feature->reset(vcpu); + } }
const struct kvm_vcpu_sbi_extension vcpu_sbi_ext_fwft = {
On Thu, Aug 14, 2025 at 09:25:44PM +0530, Anup Patel wrote:
The SBI FWFT feature values must be reset upon VCPU reset so introduce feature specific reset callback for this purpose.
Signed-off-by: Anup Patel apatel@ventanamicro.com
arch/riscv/kvm/vcpu_sbi_fwft.c | 30 ++++++++++++++++++++++++++++-- 1 file changed, 28 insertions(+), 2 deletions(-)
diff --git a/arch/riscv/kvm/vcpu_sbi_fwft.c b/arch/riscv/kvm/vcpu_sbi_fwft.c index 164a01288b0a..5a3bad0f9330 100644 --- a/arch/riscv/kvm/vcpu_sbi_fwft.c +++ b/arch/riscv/kvm/vcpu_sbi_fwft.c @@ -30,6 +30,13 @@ struct kvm_sbi_fwft_feature { */ bool (*supported)(struct kvm_vcpu *vcpu);
- /**
* @reset: Reset the feature value irrespective whether feature is supported or not** This callback is mandatory*/- void (*reset)(struct kvm_vcpu *vcpu);
- /**
- @set: Set the feature value
@@ -75,6 +82,13 @@ static bool kvm_sbi_fwft_misaligned_delegation_supported(struct kvm_vcpu *vcpu) return misaligned_traps_can_delegate(); } +static void kvm_sbi_fwft_reset_misaligned_delegation(struct kvm_vcpu *vcpu) +{
- struct kvm_vcpu_config *cfg = &vcpu->arch.cfg;
- cfg->hedeleg &= ~MIS_DELEG;
+}
static long kvm_sbi_fwft_set_misaligned_delegation(struct kvm_vcpu *vcpu, struct kvm_sbi_fwft_config *conf, unsigned long value) @@ -124,6 +138,11 @@ static bool kvm_sbi_fwft_pointer_masking_pmlen_supported(struct kvm_vcpu *vcpu) return fwft->have_vs_pmlen_7 || fwft->have_vs_pmlen_16; } +static void kvm_sbi_fwft_reset_pointer_masking_pmlen(struct kvm_vcpu *vcpu) +{
- vcpu->arch.cfg.henvcfg &= ~ENVCFG_PMM;
+}
static long kvm_sbi_fwft_set_pointer_masking_pmlen(struct kvm_vcpu *vcpu, struct kvm_sbi_fwft_config *conf, unsigned long value) @@ -180,6 +199,7 @@ static const struct kvm_sbi_fwft_feature features[] = { { .id = SBI_FWFT_MISALIGNED_EXC_DELEG, .supported = kvm_sbi_fwft_misaligned_delegation_supported,
.reset = kvm_sbi_fwft_reset_misaligned_delegation,@@ -187,6 +207,7 @@ static const struct kvm_sbi_fwft_feature features[] = { { .id = SBI_FWFT_POINTER_MASKING_PMLEN, .supported = kvm_sbi_fwft_pointer_masking_pmlen_supported,
.reset = kvm_sbi_fwft_reset_pointer_masking_pmlen,@@ -321,11 +342,16 @@ static void kvm_sbi_ext_fwft_deinit(struct kvm_vcpu *vcpu) static void kvm_sbi_ext_fwft_reset(struct kvm_vcpu *vcpu) {
- int i; struct kvm_sbi_fwft *fwft = vcpu_to_fwft(vcpu);
- const struct kvm_sbi_fwft_feature *feature;
- int i;
- for (i = 0; i < ARRAY_SIZE(features); i++)
- for (i = 0; i < ARRAY_SIZE(features); i++) { fwft->configs[i].flags = 0;
feature = &features[i];if (feature->reset)feature->reset(vcpu);- }
} const struct kvm_vcpu_sbi_extension vcpu_sbi_ext_fwft = { -- 2.43.0
Reviewed-by: Andrew Jones ajones@ventanamicro.com
On 8/14/2025 11:55 PM, Anup Patel wrote:
The SBI FWFT feature values must be reset upon VCPU reset so introduce feature specific reset callback for this purpose.
Signed-off-by: Anup Patel apatel@ventanamicro.com
arch/riscv/kvm/vcpu_sbi_fwft.c | 30 ++++++++++++++++++++++++++++-- 1 file changed, 28 insertions(+), 2 deletions(-)
Reviewed-by: Nutty Liu nutty.liu@hotmail.com
Thanks, Nutty
SBI extensions can have per-VCPU state which needs to be saved/restored through ONE_REG interface for Guest/VM migration. Introduce optional ONE_REG callbacks for SBI extensions so that ONE_REG implementation for an SBI extenion is part of the extension sources.
Signed-off-by: Anup Patel apatel@ventanamicro.com --- arch/riscv/include/asm/kvm_vcpu_sbi.h | 21 ++-- arch/riscv/kvm/vcpu_onereg.c | 31 +----- arch/riscv/kvm/vcpu_sbi.c | 145 ++++++++++++++++++++++---- arch/riscv/kvm/vcpu_sbi_sta.c | 64 ++++++++---- 4 files changed, 178 insertions(+), 83 deletions(-)
diff --git a/arch/riscv/include/asm/kvm_vcpu_sbi.h b/arch/riscv/include/asm/kvm_vcpu_sbi.h index 766031e80960..144c3f6d5eb9 100644 --- a/arch/riscv/include/asm/kvm_vcpu_sbi.h +++ b/arch/riscv/include/asm/kvm_vcpu_sbi.h @@ -59,6 +59,15 @@ struct kvm_vcpu_sbi_extension { void (*deinit)(struct kvm_vcpu *vcpu);
void (*reset)(struct kvm_vcpu *vcpu); + + bool have_state; + unsigned long state_reg_subtype; + unsigned long (*get_state_reg_count)(struct kvm_vcpu *vcpu); + int (*get_state_reg_id)(struct kvm_vcpu *vcpu, int index, u64 *reg_id); + int (*get_state_reg)(struct kvm_vcpu *vcpu, unsigned long reg_num, + unsigned long reg_size, void *reg_val); + int (*set_state_reg)(struct kvm_vcpu *vcpu, unsigned long reg_num, + unsigned long reg_size, const void *reg_val); };
void kvm_riscv_vcpu_sbi_forward(struct kvm_vcpu *vcpu, struct kvm_run *run); @@ -73,10 +82,9 @@ int kvm_riscv_vcpu_set_reg_sbi_ext(struct kvm_vcpu *vcpu, const struct kvm_one_reg *reg); int kvm_riscv_vcpu_get_reg_sbi_ext(struct kvm_vcpu *vcpu, const struct kvm_one_reg *reg); -int kvm_riscv_vcpu_set_reg_sbi(struct kvm_vcpu *vcpu, - const struct kvm_one_reg *reg); -int kvm_riscv_vcpu_get_reg_sbi(struct kvm_vcpu *vcpu, - const struct kvm_one_reg *reg); +int kvm_riscv_vcpu_reg_indices_sbi(struct kvm_vcpu *vcpu, u64 __user *uindices); +int kvm_riscv_vcpu_set_reg_sbi(struct kvm_vcpu *vcpu, const struct kvm_one_reg *reg); +int kvm_riscv_vcpu_get_reg_sbi(struct kvm_vcpu *vcpu, const struct kvm_one_reg *reg); const struct kvm_vcpu_sbi_extension *kvm_vcpu_sbi_find_ext( struct kvm_vcpu *vcpu, unsigned long extid); bool riscv_vcpu_supports_sbi_ext(struct kvm_vcpu *vcpu, int idx); @@ -85,11 +93,6 @@ void kvm_riscv_vcpu_sbi_init(struct kvm_vcpu *vcpu); void kvm_riscv_vcpu_sbi_deinit(struct kvm_vcpu *vcpu); void kvm_riscv_vcpu_sbi_reset(struct kvm_vcpu *vcpu);
-int kvm_riscv_vcpu_get_reg_sbi_sta(struct kvm_vcpu *vcpu, unsigned long reg_num, - unsigned long *reg_val); -int kvm_riscv_vcpu_set_reg_sbi_sta(struct kvm_vcpu *vcpu, unsigned long reg_num, - unsigned long reg_val); - #ifdef CONFIG_RISCV_SBI_V01 extern const struct kvm_vcpu_sbi_extension vcpu_sbi_ext_v01; #endif diff --git a/arch/riscv/kvm/vcpu_onereg.c b/arch/riscv/kvm/vcpu_onereg.c index b77748a56a59..5843b0519224 100644 --- a/arch/riscv/kvm/vcpu_onereg.c +++ b/arch/riscv/kvm/vcpu_onereg.c @@ -1090,36 +1090,9 @@ static unsigned long num_sbi_ext_regs(struct kvm_vcpu *vcpu) return copy_sbi_ext_reg_indices(vcpu, NULL); }
-static int copy_sbi_reg_indices(struct kvm_vcpu *vcpu, u64 __user *uindices) -{ - struct kvm_vcpu_sbi_context *scontext = &vcpu->arch.sbi_context; - int total = 0; - - if (scontext->ext_status[KVM_RISCV_SBI_EXT_STA] == KVM_RISCV_SBI_EXT_STATUS_ENABLED) { - u64 size = IS_ENABLED(CONFIG_32BIT) ? KVM_REG_SIZE_U32 : KVM_REG_SIZE_U64; - int n = sizeof(struct kvm_riscv_sbi_sta) / sizeof(unsigned long); - - for (int i = 0; i < n; i++) { - u64 reg = KVM_REG_RISCV | size | - KVM_REG_RISCV_SBI_STATE | - KVM_REG_RISCV_SBI_STA | i; - - if (uindices) { - if (put_user(reg, uindices)) - return -EFAULT; - uindices++; - } - } - - total += n; - } - - return total; -} - static inline unsigned long num_sbi_regs(struct kvm_vcpu *vcpu) { - return copy_sbi_reg_indices(vcpu, NULL); + return kvm_riscv_vcpu_reg_indices_sbi(vcpu, NULL); }
static inline unsigned long num_vector_regs(const struct kvm_vcpu *vcpu) @@ -1247,7 +1220,7 @@ int kvm_riscv_vcpu_copy_reg_indices(struct kvm_vcpu *vcpu, return ret; uindices += ret;
- ret = copy_sbi_reg_indices(vcpu, uindices); + ret = kvm_riscv_vcpu_reg_indices_sbi(vcpu, uindices); if (ret < 0) return ret; uindices += ret; diff --git a/arch/riscv/kvm/vcpu_sbi.c b/arch/riscv/kvm/vcpu_sbi.c index 01a93f4fdb16..8b3c393e0c83 100644 --- a/arch/riscv/kvm/vcpu_sbi.c +++ b/arch/riscv/kvm/vcpu_sbi.c @@ -364,64 +364,163 @@ int kvm_riscv_vcpu_get_reg_sbi_ext(struct kvm_vcpu *vcpu, return 0; }
-int kvm_riscv_vcpu_set_reg_sbi(struct kvm_vcpu *vcpu, - const struct kvm_one_reg *reg) +int kvm_riscv_vcpu_reg_indices_sbi(struct kvm_vcpu *vcpu, u64 __user *uindices) +{ + struct kvm_vcpu_sbi_context *scontext = &vcpu->arch.sbi_context; + const struct kvm_riscv_sbi_extension_entry *entry; + const struct kvm_vcpu_sbi_extension *ext; + unsigned long state_reg_count; + int i, j, rc, count = 0; + u64 reg; + + for (i = 0; i < ARRAY_SIZE(sbi_ext); i++) { + entry = &sbi_ext[i]; + ext = entry->ext_ptr; + + if (!ext->have_state || + scontext->ext_status[entry->ext_idx] != KVM_RISCV_SBI_EXT_STATUS_ENABLED) + continue; + + state_reg_count = ext->get_state_reg_count(vcpu); + if (!uindices) + goto skip_put_user; + + for (j = 0; j < state_reg_count; j++) { + if (ext->get_state_reg_id) { + rc = ext->get_state_reg_id(vcpu, j, ®); + if (rc) + return rc; + } else { + reg = KVM_REG_RISCV | + (IS_ENABLED(CONFIG_32BIT) ? + KVM_REG_SIZE_U32 : KVM_REG_SIZE_U64) | + KVM_REG_RISCV_SBI_STATE | + ext->state_reg_subtype | j; + } + + if (put_user(reg, uindices)) + return -EFAULT; + uindices++; + } + +skip_put_user: + count += state_reg_count; + } + + return count; +} + +static const struct kvm_vcpu_sbi_extension *kvm_vcpu_sbi_find_ext_withstate(struct kvm_vcpu *vcpu, + unsigned long subtype) +{ + struct kvm_vcpu_sbi_context *scontext = &vcpu->arch.sbi_context; + const struct kvm_riscv_sbi_extension_entry *entry; + const struct kvm_vcpu_sbi_extension *ext; + int i; + + for (i = 0; i < ARRAY_SIZE(sbi_ext); i++) { + entry = &sbi_ext[i]; + ext = entry->ext_ptr; + + if (ext->have_state && + ext->state_reg_subtype == subtype && + scontext->ext_status[entry->ext_idx] == KVM_RISCV_SBI_EXT_STATUS_ENABLED) + return ext; + } + + return NULL; +} + +int kvm_riscv_vcpu_set_reg_sbi(struct kvm_vcpu *vcpu, const struct kvm_one_reg *reg) { unsigned long __user *uaddr = (unsigned long __user *)(unsigned long)reg->addr; unsigned long reg_num = reg->id & ~(KVM_REG_ARCH_MASK | KVM_REG_SIZE_MASK | KVM_REG_RISCV_SBI_STATE); - unsigned long reg_subtype, reg_val; - - if (KVM_REG_SIZE(reg->id) != sizeof(unsigned long)) + const struct kvm_vcpu_sbi_extension *ext; + unsigned long reg_subtype; + void *reg_val; + u64 data64; + u32 data32; + u16 data16; + u8 data8; + + switch (KVM_REG_SIZE(reg->id)) { + case 1: + reg_val = &data8; + break; + case 2: + reg_val = &data16; + break; + case 4: + reg_val = &data32; + break; + case 8: + reg_val = &data64; + break; + default: return -EINVAL; + };
- if (copy_from_user(®_val, uaddr, KVM_REG_SIZE(reg->id))) + if (copy_from_user(reg_val, uaddr, KVM_REG_SIZE(reg->id))) return -EFAULT;
reg_subtype = reg_num & KVM_REG_RISCV_SUBTYPE_MASK; reg_num &= ~KVM_REG_RISCV_SUBTYPE_MASK;
- switch (reg_subtype) { - case KVM_REG_RISCV_SBI_STA: - return kvm_riscv_vcpu_set_reg_sbi_sta(vcpu, reg_num, reg_val); - default: + ext = kvm_vcpu_sbi_find_ext_withstate(vcpu, reg_subtype); + if (!ext || !ext->set_state_reg) return -EINVAL; - }
- return 0; + return ext->set_state_reg(vcpu, reg_num, KVM_REG_SIZE(reg->id), reg_val); }
-int kvm_riscv_vcpu_get_reg_sbi(struct kvm_vcpu *vcpu, - const struct kvm_one_reg *reg) +int kvm_riscv_vcpu_get_reg_sbi(struct kvm_vcpu *vcpu, const struct kvm_one_reg *reg) { unsigned long __user *uaddr = (unsigned long __user *)(unsigned long)reg->addr; unsigned long reg_num = reg->id & ~(KVM_REG_ARCH_MASK | KVM_REG_SIZE_MASK | KVM_REG_RISCV_SBI_STATE); - unsigned long reg_subtype, reg_val; + const struct kvm_vcpu_sbi_extension *ext; + unsigned long reg_subtype; + void *reg_val; + u64 data64; + u32 data32; + u16 data16; + u8 data8; int ret;
- if (KVM_REG_SIZE(reg->id) != sizeof(unsigned long)) + switch (KVM_REG_SIZE(reg->id)) { + case 1: + reg_val = &data8; + break; + case 2: + reg_val = &data16; + break; + case 4: + reg_val = &data32; + break; + case 8: + reg_val = &data64; + break; + default: return -EINVAL; + };
reg_subtype = reg_num & KVM_REG_RISCV_SUBTYPE_MASK; reg_num &= ~KVM_REG_RISCV_SUBTYPE_MASK;
- switch (reg_subtype) { - case KVM_REG_RISCV_SBI_STA: - ret = kvm_riscv_vcpu_get_reg_sbi_sta(vcpu, reg_num, ®_val); - break; - default: + ext = kvm_vcpu_sbi_find_ext_withstate(vcpu, reg_subtype); + if (!ext || !ext->get_state_reg) return -EINVAL; - }
+ ret = ext->get_state_reg(vcpu, reg_num, KVM_REG_SIZE(reg->id), reg_val); if (ret) return ret;
- if (copy_to_user(uaddr, ®_val, KVM_REG_SIZE(reg->id))) + if (copy_to_user(uaddr, reg_val, KVM_REG_SIZE(reg->id))) return -EFAULT;
return 0; diff --git a/arch/riscv/kvm/vcpu_sbi_sta.c b/arch/riscv/kvm/vcpu_sbi_sta.c index cc6cb7c8f0e4..d14cf6255d83 100644 --- a/arch/riscv/kvm/vcpu_sbi_sta.c +++ b/arch/riscv/kvm/vcpu_sbi_sta.c @@ -151,63 +151,83 @@ static unsigned long kvm_sbi_ext_sta_probe(struct kvm_vcpu *vcpu) return !!sched_info_on(); }
-const struct kvm_vcpu_sbi_extension vcpu_sbi_ext_sta = { - .extid_start = SBI_EXT_STA, - .extid_end = SBI_EXT_STA, - .handler = kvm_sbi_ext_sta_handler, - .probe = kvm_sbi_ext_sta_probe, - .reset = kvm_riscv_vcpu_sbi_sta_reset, -}; +static unsigned long kvm_sbi_ext_sta_get_state_reg_count(struct kvm_vcpu *vcpu) +{ + return sizeof(struct kvm_riscv_sbi_sta) / sizeof(unsigned long); +}
-int kvm_riscv_vcpu_get_reg_sbi_sta(struct kvm_vcpu *vcpu, - unsigned long reg_num, - unsigned long *reg_val) +static int kvm_sbi_ext_sta_get_reg(struct kvm_vcpu *vcpu, unsigned long reg_num, + unsigned long reg_size, void *reg_val) { + unsigned long *value; + + if (reg_size != sizeof(unsigned long)) + return -EINVAL; + value = reg_val; + switch (reg_num) { case KVM_REG_RISCV_SBI_STA_REG(shmem_lo): - *reg_val = (unsigned long)vcpu->arch.sta.shmem; + *value = (unsigned long)vcpu->arch.sta.shmem; break; case KVM_REG_RISCV_SBI_STA_REG(shmem_hi): if (IS_ENABLED(CONFIG_32BIT)) - *reg_val = upper_32_bits(vcpu->arch.sta.shmem); + *value = upper_32_bits(vcpu->arch.sta.shmem); else - *reg_val = 0; + *value = 0; break; default: - return -EINVAL; + return -ENOENT; }
return 0; }
-int kvm_riscv_vcpu_set_reg_sbi_sta(struct kvm_vcpu *vcpu, - unsigned long reg_num, - unsigned long reg_val) +static int kvm_sbi_ext_sta_set_reg(struct kvm_vcpu *vcpu, unsigned long reg_num, + unsigned long reg_size, const void *reg_val) { + unsigned long value; + + if (reg_size != sizeof(unsigned long)) + return -EINVAL; + value = *(const unsigned long *)reg_val; + switch (reg_num) { case KVM_REG_RISCV_SBI_STA_REG(shmem_lo): if (IS_ENABLED(CONFIG_32BIT)) { gpa_t hi = upper_32_bits(vcpu->arch.sta.shmem);
- vcpu->arch.sta.shmem = reg_val; + vcpu->arch.sta.shmem = value; vcpu->arch.sta.shmem |= hi << 32; } else { - vcpu->arch.sta.shmem = reg_val; + vcpu->arch.sta.shmem = value; } break; case KVM_REG_RISCV_SBI_STA_REG(shmem_hi): if (IS_ENABLED(CONFIG_32BIT)) { gpa_t lo = lower_32_bits(vcpu->arch.sta.shmem);
- vcpu->arch.sta.shmem = ((gpa_t)reg_val << 32); + vcpu->arch.sta.shmem = ((gpa_t)value << 32); vcpu->arch.sta.shmem |= lo; - } else if (reg_val != 0) { + } else if (value != 0) { return -EINVAL; } break; default: - return -EINVAL; + return -ENOENT; }
return 0; } + +const struct kvm_vcpu_sbi_extension vcpu_sbi_ext_sta = { + .extid_start = SBI_EXT_STA, + .extid_end = SBI_EXT_STA, + .handler = kvm_sbi_ext_sta_handler, + .probe = kvm_sbi_ext_sta_probe, + .reset = kvm_riscv_vcpu_sbi_sta_reset, + .have_state = true, + .state_reg_subtype = KVM_REG_RISCV_SBI_STA, + .get_state_reg_count = kvm_sbi_ext_sta_get_state_reg_count, + .get_state_reg = kvm_sbi_ext_sta_get_reg, + .set_state_reg = kvm_sbi_ext_sta_set_reg, +};
On Thu, Aug 14, 2025 at 09:25:45PM +0530, Anup Patel wrote:
SBI extensions can have per-VCPU state which needs to be saved/restored through ONE_REG interface for Guest/VM migration. Introduce optional ONE_REG callbacks for SBI extensions so that ONE_REG implementation for an SBI extenion is part of the extension sources.
Signed-off-by: Anup Patel apatel@ventanamicro.com
arch/riscv/include/asm/kvm_vcpu_sbi.h | 21 ++-- arch/riscv/kvm/vcpu_onereg.c | 31 +----- arch/riscv/kvm/vcpu_sbi.c | 145 ++++++++++++++++++++++---- arch/riscv/kvm/vcpu_sbi_sta.c | 64 ++++++++---- 4 files changed, 178 insertions(+), 83 deletions(-)
diff --git a/arch/riscv/include/asm/kvm_vcpu_sbi.h b/arch/riscv/include/asm/kvm_vcpu_sbi.h index 766031e80960..144c3f6d5eb9 100644 --- a/arch/riscv/include/asm/kvm_vcpu_sbi.h +++ b/arch/riscv/include/asm/kvm_vcpu_sbi.h @@ -59,6 +59,15 @@ struct kvm_vcpu_sbi_extension { void (*deinit)(struct kvm_vcpu *vcpu); void (*reset)(struct kvm_vcpu *vcpu);
- bool have_state;
- unsigned long state_reg_subtype;
- unsigned long (*get_state_reg_count)(struct kvm_vcpu *vcpu);
I think we can drop 'have_state'. When 'get_state_reg_count' is NULL, then the state reg count must be zero (i.e. have_state == false).
- int (*get_state_reg_id)(struct kvm_vcpu *vcpu, int index, u64 *reg_id);
- int (*get_state_reg)(struct kvm_vcpu *vcpu, unsigned long reg_num,
unsigned long reg_size, void *reg_val);- int (*set_state_reg)(struct kvm_vcpu *vcpu, unsigned long reg_num,
unsigned long reg_size, const void *reg_val);}; void kvm_riscv_vcpu_sbi_forward(struct kvm_vcpu *vcpu, struct kvm_run *run); @@ -73,10 +82,9 @@ int kvm_riscv_vcpu_set_reg_sbi_ext(struct kvm_vcpu *vcpu, const struct kvm_one_reg *reg); int kvm_riscv_vcpu_get_reg_sbi_ext(struct kvm_vcpu *vcpu, const struct kvm_one_reg *reg); -int kvm_riscv_vcpu_set_reg_sbi(struct kvm_vcpu *vcpu,
const struct kvm_one_reg *reg);-int kvm_riscv_vcpu_get_reg_sbi(struct kvm_vcpu *vcpu,
const struct kvm_one_reg *reg);+int kvm_riscv_vcpu_reg_indices_sbi(struct kvm_vcpu *vcpu, u64 __user *uindices); +int kvm_riscv_vcpu_set_reg_sbi(struct kvm_vcpu *vcpu, const struct kvm_one_reg *reg); +int kvm_riscv_vcpu_get_reg_sbi(struct kvm_vcpu *vcpu, const struct kvm_one_reg *reg); const struct kvm_vcpu_sbi_extension *kvm_vcpu_sbi_find_ext( struct kvm_vcpu *vcpu, unsigned long extid); bool riscv_vcpu_supports_sbi_ext(struct kvm_vcpu *vcpu, int idx); @@ -85,11 +93,6 @@ void kvm_riscv_vcpu_sbi_init(struct kvm_vcpu *vcpu); void kvm_riscv_vcpu_sbi_deinit(struct kvm_vcpu *vcpu); void kvm_riscv_vcpu_sbi_reset(struct kvm_vcpu *vcpu); -int kvm_riscv_vcpu_get_reg_sbi_sta(struct kvm_vcpu *vcpu, unsigned long reg_num,
unsigned long *reg_val);-int kvm_riscv_vcpu_set_reg_sbi_sta(struct kvm_vcpu *vcpu, unsigned long reg_num,
unsigned long reg_val);#ifdef CONFIG_RISCV_SBI_V01 extern const struct kvm_vcpu_sbi_extension vcpu_sbi_ext_v01; #endif diff --git a/arch/riscv/kvm/vcpu_onereg.c b/arch/riscv/kvm/vcpu_onereg.c index b77748a56a59..5843b0519224 100644 --- a/arch/riscv/kvm/vcpu_onereg.c +++ b/arch/riscv/kvm/vcpu_onereg.c @@ -1090,36 +1090,9 @@ static unsigned long num_sbi_ext_regs(struct kvm_vcpu *vcpu) return copy_sbi_ext_reg_indices(vcpu, NULL); } -static int copy_sbi_reg_indices(struct kvm_vcpu *vcpu, u64 __user *uindices) -{
- struct kvm_vcpu_sbi_context *scontext = &vcpu->arch.sbi_context;
- int total = 0;
- if (scontext->ext_status[KVM_RISCV_SBI_EXT_STA] == KVM_RISCV_SBI_EXT_STATUS_ENABLED) {
u64 size = IS_ENABLED(CONFIG_32BIT) ? KVM_REG_SIZE_U32 : KVM_REG_SIZE_U64;int n = sizeof(struct kvm_riscv_sbi_sta) / sizeof(unsigned long);for (int i = 0; i < n; i++) {u64 reg = KVM_REG_RISCV | size |KVM_REG_RISCV_SBI_STATE |KVM_REG_RISCV_SBI_STA | i;if (uindices) {if (put_user(reg, uindices))return -EFAULT;uindices++;}}total += n;- }
- return total;
-}
static inline unsigned long num_sbi_regs(struct kvm_vcpu *vcpu) {
- return copy_sbi_reg_indices(vcpu, NULL);
- return kvm_riscv_vcpu_reg_indices_sbi(vcpu, NULL);
} static inline unsigned long num_vector_regs(const struct kvm_vcpu *vcpu) @@ -1247,7 +1220,7 @@ int kvm_riscv_vcpu_copy_reg_indices(struct kvm_vcpu *vcpu, return ret; uindices += ret;
- ret = copy_sbi_reg_indices(vcpu, uindices);
- ret = kvm_riscv_vcpu_reg_indices_sbi(vcpu, uindices); if (ret < 0) return ret; uindices += ret;
diff --git a/arch/riscv/kvm/vcpu_sbi.c b/arch/riscv/kvm/vcpu_sbi.c index 01a93f4fdb16..8b3c393e0c83 100644 --- a/arch/riscv/kvm/vcpu_sbi.c +++ b/arch/riscv/kvm/vcpu_sbi.c @@ -364,64 +364,163 @@ int kvm_riscv_vcpu_get_reg_sbi_ext(struct kvm_vcpu *vcpu, return 0; } -int kvm_riscv_vcpu_set_reg_sbi(struct kvm_vcpu *vcpu,
const struct kvm_one_reg *reg)+int kvm_riscv_vcpu_reg_indices_sbi(struct kvm_vcpu *vcpu, u64 __user *uindices) +{
- struct kvm_vcpu_sbi_context *scontext = &vcpu->arch.sbi_context;
- const struct kvm_riscv_sbi_extension_entry *entry;
- const struct kvm_vcpu_sbi_extension *ext;
- unsigned long state_reg_count;
- int i, j, rc, count = 0;
- u64 reg;
- for (i = 0; i < ARRAY_SIZE(sbi_ext); i++) {
entry = &sbi_ext[i];ext = entry->ext_ptr;if (!ext->have_state ||scontext->ext_status[entry->ext_idx] != KVM_RISCV_SBI_EXT_STATUS_ENABLED)continue;state_reg_count = ext->get_state_reg_count(vcpu);if (!uindices)goto skip_put_user;for (j = 0; j < state_reg_count; j++) {if (ext->get_state_reg_id) {rc = ext->get_state_reg_id(vcpu, j, ®);if (rc)return rc;} else {reg = KVM_REG_RISCV |(IS_ENABLED(CONFIG_32BIT) ?KVM_REG_SIZE_U32 : KVM_REG_SIZE_U64) |KVM_REG_RISCV_SBI_STATE |ext->state_reg_subtype | j;}if (put_user(reg, uindices))return -EFAULT;uindices++;}+skip_put_user:
count += state_reg_count;- }
- return count;
+}
+static const struct kvm_vcpu_sbi_extension *kvm_vcpu_sbi_find_ext_withstate(struct kvm_vcpu *vcpu,
unsigned long subtype)+{
- struct kvm_vcpu_sbi_context *scontext = &vcpu->arch.sbi_context;
- const struct kvm_riscv_sbi_extension_entry *entry;
- const struct kvm_vcpu_sbi_extension *ext;
- int i;
- for (i = 0; i < ARRAY_SIZE(sbi_ext); i++) {
entry = &sbi_ext[i];ext = entry->ext_ptr;if (ext->have_state &&ext->state_reg_subtype == subtype &&scontext->ext_status[entry->ext_idx] == KVM_RISCV_SBI_EXT_STATUS_ENABLED)return ext;- }
- return NULL;
+}
+int kvm_riscv_vcpu_set_reg_sbi(struct kvm_vcpu *vcpu, const struct kvm_one_reg *reg) { unsigned long __user *uaddr = (unsigned long __user *)(unsigned long)reg->addr; unsigned long reg_num = reg->id & ~(KVM_REG_ARCH_MASK | KVM_REG_SIZE_MASK | KVM_REG_RISCV_SBI_STATE);
- unsigned long reg_subtype, reg_val;
- if (KVM_REG_SIZE(reg->id) != sizeof(unsigned long))
- const struct kvm_vcpu_sbi_extension *ext;
- unsigned long reg_subtype;
- void *reg_val;
- u64 data64;
- u32 data32;
- u16 data16;
- u8 data8;
- switch (KVM_REG_SIZE(reg->id)) {
- case 1:
reg_val = &data8;break;- case 2:
reg_val = &data16;break;- case 4:
reg_val = &data32;break;- case 8:
reg_val = &data64;break;- default: return -EINVAL;
- };
superfluous ';'
- if (copy_from_user(®_val, uaddr, KVM_REG_SIZE(reg->id)))
- if (copy_from_user(reg_val, uaddr, KVM_REG_SIZE(reg->id))) return -EFAULT;
reg_subtype = reg_num & KVM_REG_RISCV_SUBTYPE_MASK; reg_num &= ~KVM_REG_RISCV_SUBTYPE_MASK;
- switch (reg_subtype) {
- case KVM_REG_RISCV_SBI_STA:
return kvm_riscv_vcpu_set_reg_sbi_sta(vcpu, reg_num, reg_val);- default:
- ext = kvm_vcpu_sbi_find_ext_withstate(vcpu, reg_subtype);
- if (!ext || !ext->set_state_reg) return -EINVAL;
- }
- return 0;
- return ext->set_state_reg(vcpu, reg_num, KVM_REG_SIZE(reg->id), reg_val);
} -int kvm_riscv_vcpu_get_reg_sbi(struct kvm_vcpu *vcpu,
const struct kvm_one_reg *reg)+int kvm_riscv_vcpu_get_reg_sbi(struct kvm_vcpu *vcpu, const struct kvm_one_reg *reg) { unsigned long __user *uaddr = (unsigned long __user *)(unsigned long)reg->addr; unsigned long reg_num = reg->id & ~(KVM_REG_ARCH_MASK | KVM_REG_SIZE_MASK | KVM_REG_RISCV_SBI_STATE);
- unsigned long reg_subtype, reg_val;
- const struct kvm_vcpu_sbi_extension *ext;
- unsigned long reg_subtype;
- void *reg_val;
- u64 data64;
- u32 data32;
- u16 data16;
- u8 data8; int ret;
- if (KVM_REG_SIZE(reg->id) != sizeof(unsigned long))
- switch (KVM_REG_SIZE(reg->id)) {
- case 1:
reg_val = &data8;break;- case 2:
reg_val = &data16;break;- case 4:
reg_val = &data32;break;- case 8:
reg_val = &data64;break;- default: return -EINVAL;
- };
superfluous ';'
reg_subtype = reg_num & KVM_REG_RISCV_SUBTYPE_MASK; reg_num &= ~KVM_REG_RISCV_SUBTYPE_MASK;
- switch (reg_subtype) {
- case KVM_REG_RISCV_SBI_STA:
ret = kvm_riscv_vcpu_get_reg_sbi_sta(vcpu, reg_num, ®_val);break;- default:
- ext = kvm_vcpu_sbi_find_ext_withstate(vcpu, reg_subtype);
- if (!ext || !ext->get_state_reg) return -EINVAL;
- }
- ret = ext->get_state_reg(vcpu, reg_num, KVM_REG_SIZE(reg->id), reg_val); if (ret) return ret;
- if (copy_to_user(uaddr, ®_val, KVM_REG_SIZE(reg->id)))
- if (copy_to_user(uaddr, reg_val, KVM_REG_SIZE(reg->id))) return -EFAULT;
return 0; diff --git a/arch/riscv/kvm/vcpu_sbi_sta.c b/arch/riscv/kvm/vcpu_sbi_sta.c index cc6cb7c8f0e4..d14cf6255d83 100644 --- a/arch/riscv/kvm/vcpu_sbi_sta.c +++ b/arch/riscv/kvm/vcpu_sbi_sta.c @@ -151,63 +151,83 @@ static unsigned long kvm_sbi_ext_sta_probe(struct kvm_vcpu *vcpu) return !!sched_info_on(); } -const struct kvm_vcpu_sbi_extension vcpu_sbi_ext_sta = {
- .extid_start = SBI_EXT_STA,
- .extid_end = SBI_EXT_STA,
- .handler = kvm_sbi_ext_sta_handler,
- .probe = kvm_sbi_ext_sta_probe,
- .reset = kvm_riscv_vcpu_sbi_sta_reset,
-}; +static unsigned long kvm_sbi_ext_sta_get_state_reg_count(struct kvm_vcpu *vcpu) +{
- return sizeof(struct kvm_riscv_sbi_sta) / sizeof(unsigned long);
+} -int kvm_riscv_vcpu_get_reg_sbi_sta(struct kvm_vcpu *vcpu,
unsigned long reg_num,unsigned long *reg_val)+static int kvm_sbi_ext_sta_get_reg(struct kvm_vcpu *vcpu, unsigned long reg_num,
unsigned long reg_size, void *reg_val){
- unsigned long *value;
- if (reg_size != sizeof(unsigned long))
return -EINVAL;- value = reg_val;
- switch (reg_num) { case KVM_REG_RISCV_SBI_STA_REG(shmem_lo):
*reg_val = (unsigned long)vcpu->arch.sta.shmem;
*value = (unsigned long)vcpu->arch.sta.shmem;
*reg_val = upper_32_bits(vcpu->arch.sta.shmem);
*value = upper_32_bits(vcpu->arch.sta.shmem);
*reg_val = 0;
*value = 0;
return -EINVAL;
return -ENOENT;return 0; } -int kvm_riscv_vcpu_set_reg_sbi_sta(struct kvm_vcpu *vcpu,
unsigned long reg_num,unsigned long reg_val)+static int kvm_sbi_ext_sta_set_reg(struct kvm_vcpu *vcpu, unsigned long reg_num,
unsigned long reg_size, const void *reg_val){
- unsigned long value;
- if (reg_size != sizeof(unsigned long))
return -EINVAL;- value = *(const unsigned long *)reg_val;
- switch (reg_num) { case KVM_REG_RISCV_SBI_STA_REG(shmem_lo): if (IS_ENABLED(CONFIG_32BIT)) { gpa_t hi = upper_32_bits(vcpu->arch.sta.shmem);
vcpu->arch.sta.shmem = reg_val;
vcpu->arch.sta.shmem = value; vcpu->arch.sta.shmem |= hi << 32;
vcpu->arch.sta.shmem = reg_val;
vcpu->arch.sta.shmem = value;
vcpu->arch.sta.shmem = ((gpa_t)reg_val << 32);
vcpu->arch.sta.shmem = ((gpa_t)value << 32); vcpu->arch.sta.shmem |= lo;
} else if (reg_val != 0) {
} else if (value != 0) { return -EINVAL;
return -EINVAL;
return -ENOENT;return 0; }
+const struct kvm_vcpu_sbi_extension vcpu_sbi_ext_sta = {
- .extid_start = SBI_EXT_STA,
- .extid_end = SBI_EXT_STA,
- .handler = kvm_sbi_ext_sta_handler,
- .probe = kvm_sbi_ext_sta_probe,
- .reset = kvm_riscv_vcpu_sbi_sta_reset,
- .have_state = true,
- .state_reg_subtype = KVM_REG_RISCV_SBI_STA,
- .get_state_reg_count = kvm_sbi_ext_sta_get_state_reg_count,
- .get_state_reg = kvm_sbi_ext_sta_get_reg,
- .set_state_reg = kvm_sbi_ext_sta_set_reg,
+};
2.43.0
Otherwise,
Reviewed-by: Andrew Jones ajones@ventanamicro.com
On Sat, Aug 16, 2025 at 2:30 AM Andrew Jones ajones@ventanamicro.com wrote:
On Thu, Aug 14, 2025 at 09:25:45PM +0530, Anup Patel wrote:
SBI extensions can have per-VCPU state which needs to be saved/restored through ONE_REG interface for Guest/VM migration. Introduce optional ONE_REG callbacks for SBI extensions so that ONE_REG implementation for an SBI extenion is part of the extension sources.
Signed-off-by: Anup Patel apatel@ventanamicro.com
arch/riscv/include/asm/kvm_vcpu_sbi.h | 21 ++-- arch/riscv/kvm/vcpu_onereg.c | 31 +----- arch/riscv/kvm/vcpu_sbi.c | 145 ++++++++++++++++++++++---- arch/riscv/kvm/vcpu_sbi_sta.c | 64 ++++++++---- 4 files changed, 178 insertions(+), 83 deletions(-)
diff --git a/arch/riscv/include/asm/kvm_vcpu_sbi.h b/arch/riscv/include/asm/kvm_vcpu_sbi.h index 766031e80960..144c3f6d5eb9 100644 --- a/arch/riscv/include/asm/kvm_vcpu_sbi.h +++ b/arch/riscv/include/asm/kvm_vcpu_sbi.h @@ -59,6 +59,15 @@ struct kvm_vcpu_sbi_extension { void (*deinit)(struct kvm_vcpu *vcpu);
void (*reset)(struct kvm_vcpu *vcpu);
bool have_state;unsigned long state_reg_subtype;unsigned long (*get_state_reg_count)(struct kvm_vcpu *vcpu);I think we can drop 'have_state'. When 'get_state_reg_count' is NULL, then the state reg count must be zero (i.e. have_state == false).
Good suggestion. I will update in the next revision.
int (*get_state_reg_id)(struct kvm_vcpu *vcpu, int index, u64 *reg_id);int (*get_state_reg)(struct kvm_vcpu *vcpu, unsigned long reg_num,unsigned long reg_size, void *reg_val);int (*set_state_reg)(struct kvm_vcpu *vcpu, unsigned long reg_num,unsigned long reg_size, const void *reg_val);};
void kvm_riscv_vcpu_sbi_forward(struct kvm_vcpu *vcpu, struct kvm_run *run); @@ -73,10 +82,9 @@ int kvm_riscv_vcpu_set_reg_sbi_ext(struct kvm_vcpu *vcpu, const struct kvm_one_reg *reg); int kvm_riscv_vcpu_get_reg_sbi_ext(struct kvm_vcpu *vcpu, const struct kvm_one_reg *reg); -int kvm_riscv_vcpu_set_reg_sbi(struct kvm_vcpu *vcpu,
const struct kvm_one_reg *reg);-int kvm_riscv_vcpu_get_reg_sbi(struct kvm_vcpu *vcpu,
const struct kvm_one_reg *reg);+int kvm_riscv_vcpu_reg_indices_sbi(struct kvm_vcpu *vcpu, u64 __user *uindices); +int kvm_riscv_vcpu_set_reg_sbi(struct kvm_vcpu *vcpu, const struct kvm_one_reg *reg); +int kvm_riscv_vcpu_get_reg_sbi(struct kvm_vcpu *vcpu, const struct kvm_one_reg *reg); const struct kvm_vcpu_sbi_extension *kvm_vcpu_sbi_find_ext( struct kvm_vcpu *vcpu, unsigned long extid); bool riscv_vcpu_supports_sbi_ext(struct kvm_vcpu *vcpu, int idx); @@ -85,11 +93,6 @@ void kvm_riscv_vcpu_sbi_init(struct kvm_vcpu *vcpu); void kvm_riscv_vcpu_sbi_deinit(struct kvm_vcpu *vcpu); void kvm_riscv_vcpu_sbi_reset(struct kvm_vcpu *vcpu);
-int kvm_riscv_vcpu_get_reg_sbi_sta(struct kvm_vcpu *vcpu, unsigned long reg_num,
unsigned long *reg_val);-int kvm_riscv_vcpu_set_reg_sbi_sta(struct kvm_vcpu *vcpu, unsigned long reg_num,
unsigned long reg_val);#ifdef CONFIG_RISCV_SBI_V01 extern const struct kvm_vcpu_sbi_extension vcpu_sbi_ext_v01; #endif diff --git a/arch/riscv/kvm/vcpu_onereg.c b/arch/riscv/kvm/vcpu_onereg.c index b77748a56a59..5843b0519224 100644 --- a/arch/riscv/kvm/vcpu_onereg.c +++ b/arch/riscv/kvm/vcpu_onereg.c @@ -1090,36 +1090,9 @@ static unsigned long num_sbi_ext_regs(struct kvm_vcpu *vcpu) return copy_sbi_ext_reg_indices(vcpu, NULL); }
-static int copy_sbi_reg_indices(struct kvm_vcpu *vcpu, u64 __user *uindices) -{
struct kvm_vcpu_sbi_context *scontext = &vcpu->arch.sbi_context;int total = 0;if (scontext->ext_status[KVM_RISCV_SBI_EXT_STA] == KVM_RISCV_SBI_EXT_STATUS_ENABLED) {u64 size = IS_ENABLED(CONFIG_32BIT) ? KVM_REG_SIZE_U32 : KVM_REG_SIZE_U64;int n = sizeof(struct kvm_riscv_sbi_sta) / sizeof(unsigned long);for (int i = 0; i < n; i++) {u64 reg = KVM_REG_RISCV | size |KVM_REG_RISCV_SBI_STATE |KVM_REG_RISCV_SBI_STA | i;if (uindices) {if (put_user(reg, uindices))return -EFAULT;uindices++;}}total += n;}return total;-}
static inline unsigned long num_sbi_regs(struct kvm_vcpu *vcpu) {
return copy_sbi_reg_indices(vcpu, NULL);
return kvm_riscv_vcpu_reg_indices_sbi(vcpu, NULL);}
static inline unsigned long num_vector_regs(const struct kvm_vcpu *vcpu) @@ -1247,7 +1220,7 @@ int kvm_riscv_vcpu_copy_reg_indices(struct kvm_vcpu *vcpu, return ret; uindices += ret;
ret = copy_sbi_reg_indices(vcpu, uindices);
ret = kvm_riscv_vcpu_reg_indices_sbi(vcpu, uindices); if (ret < 0) return ret; uindices += ret;diff --git a/arch/riscv/kvm/vcpu_sbi.c b/arch/riscv/kvm/vcpu_sbi.c index 01a93f4fdb16..8b3c393e0c83 100644 --- a/arch/riscv/kvm/vcpu_sbi.c +++ b/arch/riscv/kvm/vcpu_sbi.c @@ -364,64 +364,163 @@ int kvm_riscv_vcpu_get_reg_sbi_ext(struct kvm_vcpu *vcpu, return 0; }
-int kvm_riscv_vcpu_set_reg_sbi(struct kvm_vcpu *vcpu,
const struct kvm_one_reg *reg)+int kvm_riscv_vcpu_reg_indices_sbi(struct kvm_vcpu *vcpu, u64 __user *uindices) +{
struct kvm_vcpu_sbi_context *scontext = &vcpu->arch.sbi_context;const struct kvm_riscv_sbi_extension_entry *entry;const struct kvm_vcpu_sbi_extension *ext;unsigned long state_reg_count;int i, j, rc, count = 0;u64 reg;for (i = 0; i < ARRAY_SIZE(sbi_ext); i++) {entry = &sbi_ext[i];ext = entry->ext_ptr;if (!ext->have_state ||scontext->ext_status[entry->ext_idx] != KVM_RISCV_SBI_EXT_STATUS_ENABLED)continue;state_reg_count = ext->get_state_reg_count(vcpu);if (!uindices)goto skip_put_user;for (j = 0; j < state_reg_count; j++) {if (ext->get_state_reg_id) {rc = ext->get_state_reg_id(vcpu, j, ®);if (rc)return rc;} else {reg = KVM_REG_RISCV |(IS_ENABLED(CONFIG_32BIT) ?KVM_REG_SIZE_U32 : KVM_REG_SIZE_U64) |KVM_REG_RISCV_SBI_STATE |ext->state_reg_subtype | j;}if (put_user(reg, uindices))return -EFAULT;uindices++;}+skip_put_user:
count += state_reg_count;}return count;+}
+static const struct kvm_vcpu_sbi_extension *kvm_vcpu_sbi_find_ext_withstate(struct kvm_vcpu *vcpu,
unsigned long subtype)+{
struct kvm_vcpu_sbi_context *scontext = &vcpu->arch.sbi_context;const struct kvm_riscv_sbi_extension_entry *entry;const struct kvm_vcpu_sbi_extension *ext;int i;for (i = 0; i < ARRAY_SIZE(sbi_ext); i++) {entry = &sbi_ext[i];ext = entry->ext_ptr;if (ext->have_state &&ext->state_reg_subtype == subtype &&scontext->ext_status[entry->ext_idx] == KVM_RISCV_SBI_EXT_STATUS_ENABLED)return ext;}return NULL;+}
+int kvm_riscv_vcpu_set_reg_sbi(struct kvm_vcpu *vcpu, const struct kvm_one_reg *reg) { unsigned long __user *uaddr = (unsigned long __user *)(unsigned long)reg->addr; unsigned long reg_num = reg->id & ~(KVM_REG_ARCH_MASK | KVM_REG_SIZE_MASK | KVM_REG_RISCV_SBI_STATE);
unsigned long reg_subtype, reg_val;if (KVM_REG_SIZE(reg->id) != sizeof(unsigned long))
const struct kvm_vcpu_sbi_extension *ext;unsigned long reg_subtype;void *reg_val;u64 data64;u32 data32;u16 data16;u8 data8;switch (KVM_REG_SIZE(reg->id)) {case 1:reg_val = &data8;break;case 2:reg_val = &data16;break;case 4:reg_val = &data32;break;case 8:reg_val = &data64;break;default: return -EINVAL;};superfluous ';'
Okay, I will update in the next revision.
if (copy_from_user(®_val, uaddr, KVM_REG_SIZE(reg->id)))
if (copy_from_user(reg_val, uaddr, KVM_REG_SIZE(reg->id))) return -EFAULT; reg_subtype = reg_num & KVM_REG_RISCV_SUBTYPE_MASK; reg_num &= ~KVM_REG_RISCV_SUBTYPE_MASK;
switch (reg_subtype) {case KVM_REG_RISCV_SBI_STA:return kvm_riscv_vcpu_set_reg_sbi_sta(vcpu, reg_num, reg_val);default:
ext = kvm_vcpu_sbi_find_ext_withstate(vcpu, reg_subtype);if (!ext || !ext->set_state_reg) return -EINVAL;
}return 0;
return ext->set_state_reg(vcpu, reg_num, KVM_REG_SIZE(reg->id), reg_val);}
-int kvm_riscv_vcpu_get_reg_sbi(struct kvm_vcpu *vcpu,
const struct kvm_one_reg *reg)+int kvm_riscv_vcpu_get_reg_sbi(struct kvm_vcpu *vcpu, const struct kvm_one_reg *reg) { unsigned long __user *uaddr = (unsigned long __user *)(unsigned long)reg->addr; unsigned long reg_num = reg->id & ~(KVM_REG_ARCH_MASK | KVM_REG_SIZE_MASK | KVM_REG_RISCV_SBI_STATE);
unsigned long reg_subtype, reg_val;
const struct kvm_vcpu_sbi_extension *ext;unsigned long reg_subtype;void *reg_val;u64 data64;u32 data32;u16 data16;u8 data8; int ret;
if (KVM_REG_SIZE(reg->id) != sizeof(unsigned long))
switch (KVM_REG_SIZE(reg->id)) {case 1:reg_val = &data8;break;case 2:reg_val = &data16;break;case 4:reg_val = &data32;break;case 8:reg_val = &data64;break;default: return -EINVAL;};superfluous ';'
Okay, I will update in the next revision.
reg_subtype = reg_num & KVM_REG_RISCV_SUBTYPE_MASK; reg_num &= ~KVM_REG_RISCV_SUBTYPE_MASK;
switch (reg_subtype) {case KVM_REG_RISCV_SBI_STA:ret = kvm_riscv_vcpu_get_reg_sbi_sta(vcpu, reg_num, ®_val);break;default:
ext = kvm_vcpu_sbi_find_ext_withstate(vcpu, reg_subtype);if (!ext || !ext->get_state_reg) return -EINVAL;
}
ret = ext->get_state_reg(vcpu, reg_num, KVM_REG_SIZE(reg->id), reg_val); if (ret) return ret;
if (copy_to_user(uaddr, ®_val, KVM_REG_SIZE(reg->id)))
if (copy_to_user(uaddr, reg_val, KVM_REG_SIZE(reg->id))) return -EFAULT; return 0;diff --git a/arch/riscv/kvm/vcpu_sbi_sta.c b/arch/riscv/kvm/vcpu_sbi_sta.c index cc6cb7c8f0e4..d14cf6255d83 100644 --- a/arch/riscv/kvm/vcpu_sbi_sta.c +++ b/arch/riscv/kvm/vcpu_sbi_sta.c @@ -151,63 +151,83 @@ static unsigned long kvm_sbi_ext_sta_probe(struct kvm_vcpu *vcpu) return !!sched_info_on(); }
-const struct kvm_vcpu_sbi_extension vcpu_sbi_ext_sta = {
.extid_start = SBI_EXT_STA,.extid_end = SBI_EXT_STA,.handler = kvm_sbi_ext_sta_handler,.probe = kvm_sbi_ext_sta_probe,.reset = kvm_riscv_vcpu_sbi_sta_reset,-}; +static unsigned long kvm_sbi_ext_sta_get_state_reg_count(struct kvm_vcpu *vcpu) +{
return sizeof(struct kvm_riscv_sbi_sta) / sizeof(unsigned long);+}
-int kvm_riscv_vcpu_get_reg_sbi_sta(struct kvm_vcpu *vcpu,
unsigned long reg_num,unsigned long *reg_val)+static int kvm_sbi_ext_sta_get_reg(struct kvm_vcpu *vcpu, unsigned long reg_num,
unsigned long reg_size, void *reg_val){
unsigned long *value;if (reg_size != sizeof(unsigned long))return -EINVAL;value = reg_val;switch (reg_num) { case KVM_REG_RISCV_SBI_STA_REG(shmem_lo):
*reg_val = (unsigned long)vcpu->arch.sta.shmem;
*value = (unsigned long)vcpu->arch.sta.shmem; break; case KVM_REG_RISCV_SBI_STA_REG(shmem_hi): if (IS_ENABLED(CONFIG_32BIT))
*reg_val = upper_32_bits(vcpu->arch.sta.shmem);
*value = upper_32_bits(vcpu->arch.sta.shmem); else
*reg_val = 0;
*value = 0; break; default:
return -EINVAL;
return -ENOENT; } return 0;}
-int kvm_riscv_vcpu_set_reg_sbi_sta(struct kvm_vcpu *vcpu,
unsigned long reg_num,unsigned long reg_val)+static int kvm_sbi_ext_sta_set_reg(struct kvm_vcpu *vcpu, unsigned long reg_num,
unsigned long reg_size, const void *reg_val){
unsigned long value;if (reg_size != sizeof(unsigned long))return -EINVAL;value = *(const unsigned long *)reg_val;switch (reg_num) { case KVM_REG_RISCV_SBI_STA_REG(shmem_lo): if (IS_ENABLED(CONFIG_32BIT)) { gpa_t hi = upper_32_bits(vcpu->arch.sta.shmem);
vcpu->arch.sta.shmem = reg_val;
vcpu->arch.sta.shmem = value; vcpu->arch.sta.shmem |= hi << 32; } else {
vcpu->arch.sta.shmem = reg_val;
vcpu->arch.sta.shmem = value; } break; case KVM_REG_RISCV_SBI_STA_REG(shmem_hi): if (IS_ENABLED(CONFIG_32BIT)) { gpa_t lo = lower_32_bits(vcpu->arch.sta.shmem);
vcpu->arch.sta.shmem = ((gpa_t)reg_val << 32);
vcpu->arch.sta.shmem = ((gpa_t)value << 32); vcpu->arch.sta.shmem |= lo;
} else if (reg_val != 0) {
} else if (value != 0) { return -EINVAL; } break; default:
return -EINVAL;
return -ENOENT; } return 0;}
+const struct kvm_vcpu_sbi_extension vcpu_sbi_ext_sta = {
.extid_start = SBI_EXT_STA,.extid_end = SBI_EXT_STA,.handler = kvm_sbi_ext_sta_handler,.probe = kvm_sbi_ext_sta_probe,.reset = kvm_riscv_vcpu_sbi_sta_reset,.have_state = true,.state_reg_subtype = KVM_REG_RISCV_SBI_STA,.get_state_reg_count = kvm_sbi_ext_sta_get_state_reg_count,.get_state_reg = kvm_sbi_ext_sta_get_reg,.set_state_reg = kvm_sbi_ext_sta_set_reg,+};
2.43.0
Otherwise,
Reviewed-by: Andrew Jones ajones@ventanamicro.com
Thanks, Anup
The ONE_REG handling of SBI extension enable/disable registers and SBI extension state registers is already under SBI implementation. On similar lines, let's move copy_sbi_ext_reg_indices() under SBI implementation.
Signed-off-by: Anup Patel apatel@ventanamicro.com --- arch/riscv/include/asm/kvm_vcpu_sbi.h | 2 +- arch/riscv/kvm/vcpu_onereg.c | 29 ++------------------------- arch/riscv/kvm/vcpu_sbi.c | 27 ++++++++++++++++++++++++- 3 files changed, 29 insertions(+), 29 deletions(-)
diff --git a/arch/riscv/include/asm/kvm_vcpu_sbi.h b/arch/riscv/include/asm/kvm_vcpu_sbi.h index 144c3f6d5eb9..212c31629bc8 100644 --- a/arch/riscv/include/asm/kvm_vcpu_sbi.h +++ b/arch/riscv/include/asm/kvm_vcpu_sbi.h @@ -78,6 +78,7 @@ void kvm_riscv_vcpu_sbi_request_reset(struct kvm_vcpu *vcpu, unsigned long pc, unsigned long a1); void kvm_riscv_vcpu_sbi_load_reset_state(struct kvm_vcpu *vcpu); int kvm_riscv_vcpu_sbi_return(struct kvm_vcpu *vcpu, struct kvm_run *run); +int kvm_riscv_vcpu_reg_indices_sbi_ext(struct kvm_vcpu *vcpu, u64 __user *uindices); int kvm_riscv_vcpu_set_reg_sbi_ext(struct kvm_vcpu *vcpu, const struct kvm_one_reg *reg); int kvm_riscv_vcpu_get_reg_sbi_ext(struct kvm_vcpu *vcpu, @@ -87,7 +88,6 @@ int kvm_riscv_vcpu_set_reg_sbi(struct kvm_vcpu *vcpu, const struct kvm_one_reg * int kvm_riscv_vcpu_get_reg_sbi(struct kvm_vcpu *vcpu, const struct kvm_one_reg *reg); const struct kvm_vcpu_sbi_extension *kvm_vcpu_sbi_find_ext( struct kvm_vcpu *vcpu, unsigned long extid); -bool riscv_vcpu_supports_sbi_ext(struct kvm_vcpu *vcpu, int idx); int kvm_riscv_vcpu_sbi_ecall(struct kvm_vcpu *vcpu, struct kvm_run *run); void kvm_riscv_vcpu_sbi_init(struct kvm_vcpu *vcpu); void kvm_riscv_vcpu_sbi_deinit(struct kvm_vcpu *vcpu); diff --git a/arch/riscv/kvm/vcpu_onereg.c b/arch/riscv/kvm/vcpu_onereg.c index 5843b0519224..0894ab517525 100644 --- a/arch/riscv/kvm/vcpu_onereg.c +++ b/arch/riscv/kvm/vcpu_onereg.c @@ -1060,34 +1060,9 @@ static inline unsigned long num_isa_ext_regs(const struct kvm_vcpu *vcpu) return copy_isa_ext_reg_indices(vcpu, NULL); }
-static int copy_sbi_ext_reg_indices(struct kvm_vcpu *vcpu, u64 __user *uindices) -{ - unsigned int n = 0; - - for (int i = 0; i < KVM_RISCV_SBI_EXT_MAX; i++) { - u64 size = IS_ENABLED(CONFIG_32BIT) ? - KVM_REG_SIZE_U32 : KVM_REG_SIZE_U64; - u64 reg = KVM_REG_RISCV | size | KVM_REG_RISCV_SBI_EXT | - KVM_REG_RISCV_SBI_SINGLE | i; - - if (!riscv_vcpu_supports_sbi_ext(vcpu, i)) - continue; - - if (uindices) { - if (put_user(reg, uindices)) - return -EFAULT; - uindices++; - } - - n++; - } - - return n; -} - static unsigned long num_sbi_ext_regs(struct kvm_vcpu *vcpu) { - return copy_sbi_ext_reg_indices(vcpu, NULL); + return kvm_riscv_vcpu_reg_indices_sbi_ext(vcpu, NULL); }
static inline unsigned long num_sbi_regs(struct kvm_vcpu *vcpu) @@ -1215,7 +1190,7 @@ int kvm_riscv_vcpu_copy_reg_indices(struct kvm_vcpu *vcpu, return ret; uindices += ret;
- ret = copy_sbi_ext_reg_indices(vcpu, uindices); + ret = kvm_riscv_vcpu_reg_indices_sbi_ext(vcpu, uindices); if (ret < 0) return ret; uindices += ret; diff --git a/arch/riscv/kvm/vcpu_sbi.c b/arch/riscv/kvm/vcpu_sbi.c index 8b3c393e0c83..19e0e3d7b598 100644 --- a/arch/riscv/kvm/vcpu_sbi.c +++ b/arch/riscv/kvm/vcpu_sbi.c @@ -110,7 +110,7 @@ riscv_vcpu_get_sbi_ext(struct kvm_vcpu *vcpu, unsigned long idx) return sext; }
-bool riscv_vcpu_supports_sbi_ext(struct kvm_vcpu *vcpu, int idx) +static bool riscv_vcpu_supports_sbi_ext(struct kvm_vcpu *vcpu, int idx) { struct kvm_vcpu_sbi_context *scontext = &vcpu->arch.sbi_context; const struct kvm_riscv_sbi_extension_entry *sext; @@ -288,6 +288,31 @@ static int riscv_vcpu_get_sbi_ext_multi(struct kvm_vcpu *vcpu, return 0; }
+int kvm_riscv_vcpu_reg_indices_sbi_ext(struct kvm_vcpu *vcpu, u64 __user *uindices) +{ + unsigned int n = 0; + + for (int i = 0; i < KVM_RISCV_SBI_EXT_MAX; i++) { + u64 size = IS_ENABLED(CONFIG_32BIT) ? + KVM_REG_SIZE_U32 : KVM_REG_SIZE_U64; + u64 reg = KVM_REG_RISCV | size | KVM_REG_RISCV_SBI_EXT | + KVM_REG_RISCV_SBI_SINGLE | i; + + if (!riscv_vcpu_supports_sbi_ext(vcpu, i)) + continue; + + if (uindices) { + if (put_user(reg, uindices)) + return -EFAULT; + uindices++; + } + + n++; + } + + return n; +} + int kvm_riscv_vcpu_set_reg_sbi_ext(struct kvm_vcpu *vcpu, const struct kvm_one_reg *reg) {
On Thu, Aug 14, 2025 at 09:25:46PM +0530, Anup Patel wrote:
The ONE_REG handling of SBI extension enable/disable registers and SBI extension state registers is already under SBI implementation. On similar lines, let's move copy_sbi_ext_reg_indices() under SBI implementation.
Signed-off-by: Anup Patel apatel@ventanamicro.com
arch/riscv/include/asm/kvm_vcpu_sbi.h | 2 +- arch/riscv/kvm/vcpu_onereg.c | 29 ++------------------------- arch/riscv/kvm/vcpu_sbi.c | 27 ++++++++++++++++++++++++- 3 files changed, 29 insertions(+), 29 deletions(-)
diff --git a/arch/riscv/include/asm/kvm_vcpu_sbi.h b/arch/riscv/include/asm/kvm_vcpu_sbi.h index 144c3f6d5eb9..212c31629bc8 100644 --- a/arch/riscv/include/asm/kvm_vcpu_sbi.h +++ b/arch/riscv/include/asm/kvm_vcpu_sbi.h @@ -78,6 +78,7 @@ void kvm_riscv_vcpu_sbi_request_reset(struct kvm_vcpu *vcpu, unsigned long pc, unsigned long a1); void kvm_riscv_vcpu_sbi_load_reset_state(struct kvm_vcpu *vcpu); int kvm_riscv_vcpu_sbi_return(struct kvm_vcpu *vcpu, struct kvm_run *run); +int kvm_riscv_vcpu_reg_indices_sbi_ext(struct kvm_vcpu *vcpu, u64 __user *uindices); int kvm_riscv_vcpu_set_reg_sbi_ext(struct kvm_vcpu *vcpu, const struct kvm_one_reg *reg); int kvm_riscv_vcpu_get_reg_sbi_ext(struct kvm_vcpu *vcpu, @@ -87,7 +88,6 @@ int kvm_riscv_vcpu_set_reg_sbi(struct kvm_vcpu *vcpu, const struct kvm_one_reg * int kvm_riscv_vcpu_get_reg_sbi(struct kvm_vcpu *vcpu, const struct kvm_one_reg *reg); const struct kvm_vcpu_sbi_extension *kvm_vcpu_sbi_find_ext( struct kvm_vcpu *vcpu, unsigned long extid); -bool riscv_vcpu_supports_sbi_ext(struct kvm_vcpu *vcpu, int idx); int kvm_riscv_vcpu_sbi_ecall(struct kvm_vcpu *vcpu, struct kvm_run *run); void kvm_riscv_vcpu_sbi_init(struct kvm_vcpu *vcpu); void kvm_riscv_vcpu_sbi_deinit(struct kvm_vcpu *vcpu); diff --git a/arch/riscv/kvm/vcpu_onereg.c b/arch/riscv/kvm/vcpu_onereg.c index 5843b0519224..0894ab517525 100644 --- a/arch/riscv/kvm/vcpu_onereg.c +++ b/arch/riscv/kvm/vcpu_onereg.c @@ -1060,34 +1060,9 @@ static inline unsigned long num_isa_ext_regs(const struct kvm_vcpu *vcpu) return copy_isa_ext_reg_indices(vcpu, NULL); } -static int copy_sbi_ext_reg_indices(struct kvm_vcpu *vcpu, u64 __user *uindices) -{
- unsigned int n = 0;
- for (int i = 0; i < KVM_RISCV_SBI_EXT_MAX; i++) {
u64 size = IS_ENABLED(CONFIG_32BIT) ?KVM_REG_SIZE_U32 : KVM_REG_SIZE_U64;u64 reg = KVM_REG_RISCV | size | KVM_REG_RISCV_SBI_EXT |KVM_REG_RISCV_SBI_SINGLE | i;if (!riscv_vcpu_supports_sbi_ext(vcpu, i))continue;if (uindices) {if (put_user(reg, uindices))return -EFAULT;uindices++;}n++;- }
- return n;
-}
static unsigned long num_sbi_ext_regs(struct kvm_vcpu *vcpu) {
- return copy_sbi_ext_reg_indices(vcpu, NULL);
- return kvm_riscv_vcpu_reg_indices_sbi_ext(vcpu, NULL);
} static inline unsigned long num_sbi_regs(struct kvm_vcpu *vcpu) @@ -1215,7 +1190,7 @@ int kvm_riscv_vcpu_copy_reg_indices(struct kvm_vcpu *vcpu, return ret; uindices += ret;
- ret = copy_sbi_ext_reg_indices(vcpu, uindices);
- ret = kvm_riscv_vcpu_reg_indices_sbi_ext(vcpu, uindices); if (ret < 0) return ret; uindices += ret;
diff --git a/arch/riscv/kvm/vcpu_sbi.c b/arch/riscv/kvm/vcpu_sbi.c index 8b3c393e0c83..19e0e3d7b598 100644 --- a/arch/riscv/kvm/vcpu_sbi.c +++ b/arch/riscv/kvm/vcpu_sbi.c @@ -110,7 +110,7 @@ riscv_vcpu_get_sbi_ext(struct kvm_vcpu *vcpu, unsigned long idx) return sext; } -bool riscv_vcpu_supports_sbi_ext(struct kvm_vcpu *vcpu, int idx) +static bool riscv_vcpu_supports_sbi_ext(struct kvm_vcpu *vcpu, int idx) { struct kvm_vcpu_sbi_context *scontext = &vcpu->arch.sbi_context; const struct kvm_riscv_sbi_extension_entry *sext; @@ -288,6 +288,31 @@ static int riscv_vcpu_get_sbi_ext_multi(struct kvm_vcpu *vcpu, return 0; } +int kvm_riscv_vcpu_reg_indices_sbi_ext(struct kvm_vcpu *vcpu, u64 __user *uindices) +{
- unsigned int n = 0;
- for (int i = 0; i < KVM_RISCV_SBI_EXT_MAX; i++) {
u64 size = IS_ENABLED(CONFIG_32BIT) ?KVM_REG_SIZE_U32 : KVM_REG_SIZE_U64;u64 reg = KVM_REG_RISCV | size | KVM_REG_RISCV_SBI_EXT |KVM_REG_RISCV_SBI_SINGLE | i;if (!riscv_vcpu_supports_sbi_ext(vcpu, i))continue;if (uindices) {if (put_user(reg, uindices))return -EFAULT;uindices++;}n++;- }
- return n;
+}
int kvm_riscv_vcpu_set_reg_sbi_ext(struct kvm_vcpu *vcpu, const struct kvm_one_reg *reg) { -- 2.43.0
Reviewed-by: Andrew Jones ajones@ventanamicro.com
The KVM user-space needs a way to save/restore the state of SBI FWFT features so implement SBI extension ONE_REG callbacks.
Signed-off-by: Anup Patel apatel@ventanamicro.com --- arch/riscv/include/uapi/asm/kvm.h | 14 +++ arch/riscv/kvm/vcpu_sbi_fwft.c | 169 +++++++++++++++++++++++++++--- 2 files changed, 171 insertions(+), 12 deletions(-)
diff --git a/arch/riscv/include/uapi/asm/kvm.h b/arch/riscv/include/uapi/asm/kvm.h index a5ca0f4ce2d3..fc5624e89c7b 100644 --- a/arch/riscv/include/uapi/asm/kvm.h +++ b/arch/riscv/include/uapi/asm/kvm.h @@ -215,6 +215,17 @@ struct kvm_riscv_sbi_sta { unsigned long shmem_hi; };
+struct kvm_riscv_sbi_fwft_feature { + unsigned long flags; + unsigned long value; +}; + +/* SBI FWFT extension registers for KVM_GET_ONE_REG and KVM_SET_ONE_REG */ +struct kvm_riscv_sbi_fwft { + struct kvm_riscv_sbi_fwft_feature misaligned_deleg; + struct kvm_riscv_sbi_fwft_feature pointer_masking; +}; + /* Possible states for kvm_riscv_timer */ #define KVM_RISCV_TIMER_STATE_OFF 0 #define KVM_RISCV_TIMER_STATE_ON 1 @@ -298,6 +309,9 @@ struct kvm_riscv_sbi_sta { #define KVM_REG_RISCV_SBI_STA (0x0 << KVM_REG_RISCV_SUBTYPE_SHIFT) #define KVM_REG_RISCV_SBI_STA_REG(name) \ (offsetof(struct kvm_riscv_sbi_sta, name) / sizeof(unsigned long)) +#define KVM_REG_RISCV_SBI_FWFT (0x1 << KVM_REG_RISCV_SUBTYPE_SHIFT) +#define KVM_REG_RISCV_SBI_FWFT_REG(name) \ + (offsetof(struct kvm_riscv_sbi_fwft, name) / sizeof(unsigned long))
/* Device Control API: RISC-V AIA */ #define KVM_DEV_RISCV_APLIC_ALIGN 0x1000 diff --git a/arch/riscv/kvm/vcpu_sbi_fwft.c b/arch/riscv/kvm/vcpu_sbi_fwft.c index 5a3bad0f9330..0d740e7c5713 100644 --- a/arch/riscv/kvm/vcpu_sbi_fwft.c +++ b/arch/riscv/kvm/vcpu_sbi_fwft.c @@ -22,6 +22,11 @@ struct kvm_sbi_fwft_feature { */ enum sbi_fwft_feature_t id;
+ /** + * @flags_reg_num: ONE_REG index of the feature flag + */ + unsigned long flags_reg_num; + /** * @supported: Check if the feature is supported on the vcpu * @@ -44,7 +49,8 @@ struct kvm_sbi_fwft_feature { * * This callback is mandatory */ - long (*set)(struct kvm_vcpu *vcpu, struct kvm_sbi_fwft_config *conf, unsigned long value); + long (*set)(struct kvm_vcpu *vcpu, struct kvm_sbi_fwft_config *conf, + bool one_reg_access, unsigned long value);
/** * @get: Get the feature current value @@ -53,7 +59,8 @@ struct kvm_sbi_fwft_feature { * * This callback is mandatory */ - long (*get)(struct kvm_vcpu *vcpu, struct kvm_sbi_fwft_config *conf, unsigned long *value); + long (*get)(struct kvm_vcpu *vcpu, struct kvm_sbi_fwft_config *conf, + bool one_reg_access, unsigned long *value); };
static const enum sbi_fwft_feature_t kvm_fwft_defined_features[] = { @@ -91,16 +98,18 @@ static void kvm_sbi_fwft_reset_misaligned_delegation(struct kvm_vcpu *vcpu)
static long kvm_sbi_fwft_set_misaligned_delegation(struct kvm_vcpu *vcpu, struct kvm_sbi_fwft_config *conf, - unsigned long value) + bool one_reg_access, unsigned long value) { struct kvm_vcpu_config *cfg = &vcpu->arch.cfg;
if (value == 1) { cfg->hedeleg |= MIS_DELEG; - csr_set(CSR_HEDELEG, MIS_DELEG); + if (!one_reg_access) + csr_set(CSR_HEDELEG, MIS_DELEG); } else if (value == 0) { cfg->hedeleg &= ~MIS_DELEG; - csr_clear(CSR_HEDELEG, MIS_DELEG); + if (!one_reg_access) + csr_clear(CSR_HEDELEG, MIS_DELEG); } else { return SBI_ERR_INVALID_PARAM; } @@ -110,10 +119,11 @@ static long kvm_sbi_fwft_set_misaligned_delegation(struct kvm_vcpu *vcpu,
static long kvm_sbi_fwft_get_misaligned_delegation(struct kvm_vcpu *vcpu, struct kvm_sbi_fwft_config *conf, - unsigned long *value) + bool one_reg_access, unsigned long *value) { - *value = (csr_read(CSR_HEDELEG) & MIS_DELEG) == MIS_DELEG; + struct kvm_vcpu_config *cfg = &vcpu->arch.cfg;
+ *value = (cfg->hedeleg & MIS_DELEG) == MIS_DELEG; return SBI_SUCCESS; }
@@ -145,7 +155,7 @@ static void kvm_sbi_fwft_reset_pointer_masking_pmlen(struct kvm_vcpu *vcpu)
static long kvm_sbi_fwft_set_pointer_masking_pmlen(struct kvm_vcpu *vcpu, struct kvm_sbi_fwft_config *conf, - unsigned long value) + bool one_reg_access, unsigned long value) { struct kvm_sbi_fwft *fwft = vcpu_to_fwft(vcpu); unsigned long pmm; @@ -167,14 +177,15 @@ static long kvm_sbi_fwft_set_pointer_masking_pmlen(struct kvm_vcpu *vcpu, * update here so that VCPU see's pointer masking mode change * immediately. */ - csr_write(CSR_HENVCFG, vcpu->arch.cfg.henvcfg); + if (!one_reg_access) + csr_write(CSR_HENVCFG, vcpu->arch.cfg.henvcfg);
return SBI_SUCCESS; }
static long kvm_sbi_fwft_get_pointer_masking_pmlen(struct kvm_vcpu *vcpu, struct kvm_sbi_fwft_config *conf, - unsigned long *value) + bool one_reg_access, unsigned long *value) { switch (vcpu->arch.cfg.henvcfg & ENVCFG_PMM) { case ENVCFG_PMM_PMLEN_0: @@ -198,6 +209,8 @@ static long kvm_sbi_fwft_get_pointer_masking_pmlen(struct kvm_vcpu *vcpu, static const struct kvm_sbi_fwft_feature features[] = { { .id = SBI_FWFT_MISALIGNED_EXC_DELEG, + .flags_reg_num = offsetof(struct kvm_riscv_sbi_fwft, misaligned_deleg.flags) / + sizeof(unsigned long), .supported = kvm_sbi_fwft_misaligned_delegation_supported, .reset = kvm_sbi_fwft_reset_misaligned_delegation, .set = kvm_sbi_fwft_set_misaligned_delegation, @@ -206,6 +219,8 @@ static const struct kvm_sbi_fwft_feature features[] = { #ifndef CONFIG_32BIT { .id = SBI_FWFT_POINTER_MASKING_PMLEN, + .flags_reg_num = offsetof(struct kvm_riscv_sbi_fwft, pointer_masking.flags) / + sizeof(unsigned long), .supported = kvm_sbi_fwft_pointer_masking_pmlen_supported, .reset = kvm_sbi_fwft_reset_pointer_masking_pmlen, .set = kvm_sbi_fwft_set_pointer_masking_pmlen, @@ -214,6 +229,21 @@ static const struct kvm_sbi_fwft_feature features[] = { #endif };
+static const struct kvm_sbi_fwft_feature *kvm_sbi_fwft_regnum_to_feature(unsigned long reg_num) +{ + const struct kvm_sbi_fwft_feature *feature; + int i; + + for (i = 0; i < ARRAY_SIZE(features); i++) { + feature = &features[i]; + if (feature->flags_reg_num == reg_num || + (feature->flags_reg_num + 1) == reg_num) + return feature; + } + + return NULL; +} + static struct kvm_sbi_fwft_config * kvm_sbi_fwft_get_config(struct kvm_vcpu *vcpu, enum sbi_fwft_feature_t feature) { @@ -267,7 +297,7 @@ static int kvm_sbi_fwft_set(struct kvm_vcpu *vcpu, u32 feature,
conf->flags = flags;
- return conf->feature->set(vcpu, conf, value); + return conf->feature->set(vcpu, conf, false, value); }
static int kvm_sbi_fwft_get(struct kvm_vcpu *vcpu, unsigned long feature, @@ -280,7 +310,7 @@ static int kvm_sbi_fwft_get(struct kvm_vcpu *vcpu, unsigned long feature, if (ret) return ret;
- return conf->feature->get(vcpu, conf, value); + return conf->feature->get(vcpu, conf, false, value); }
static int kvm_sbi_ext_fwft_handler(struct kvm_vcpu *vcpu, struct kvm_run *run, @@ -354,6 +384,115 @@ static void kvm_sbi_ext_fwft_reset(struct kvm_vcpu *vcpu) } }
+static unsigned long kvm_sbi_ext_fwft_get_reg_count(struct kvm_vcpu *vcpu) +{ + unsigned long max_reg_count = sizeof(struct kvm_riscv_sbi_fwft) / sizeof(unsigned long); + const struct kvm_sbi_fwft_feature *feature; + struct kvm_sbi_fwft_config *conf; + unsigned long reg, ret = 0; + + for (reg = 0; reg < max_reg_count; reg++) { + feature = kvm_sbi_fwft_regnum_to_feature(reg); + if (!feature) + continue; + + conf = kvm_sbi_fwft_get_config(vcpu, feature->id); + if (!conf || !conf->supported) + continue; + + ret++; + } + + return ret; +} + +static int kvm_sbi_ext_fwft_get_reg_id(struct kvm_vcpu *vcpu, int index, u64 *reg_id) +{ + int reg, max_reg_count = sizeof(struct kvm_riscv_sbi_fwft) / sizeof(unsigned long); + const struct kvm_sbi_fwft_feature *feature; + struct kvm_sbi_fwft_config *conf; + int idx = 0; + + for (reg = 0; reg < max_reg_count; reg++) { + feature = kvm_sbi_fwft_regnum_to_feature(reg); + if (!feature) + continue; + + conf = kvm_sbi_fwft_get_config(vcpu, feature->id); + if (!conf || !conf->supported) + continue; + + if (index == idx) { + *reg_id = KVM_REG_RISCV | + (IS_ENABLED(CONFIG_32BIT) ? + KVM_REG_SIZE_U32 : KVM_REG_SIZE_U64) | + KVM_REG_RISCV_SBI_STATE | + KVM_REG_RISCV_SBI_FWFT | reg; + return 0; + } + + idx++; + } + + return -ENOENT; +} + +static int kvm_sbi_ext_fwft_get_reg(struct kvm_vcpu *vcpu, unsigned long reg_num, + unsigned long reg_size, void *reg_val) +{ + const struct kvm_sbi_fwft_feature *feature; + struct kvm_sbi_fwft_config *conf; + unsigned long *value; + int ret = 0; + + if (reg_size != sizeof(unsigned long)) + return -EINVAL; + value = reg_val; + + feature = kvm_sbi_fwft_regnum_to_feature(reg_num); + if (!feature) + return -ENOENT; + + conf = kvm_sbi_fwft_get_config(vcpu, feature->id); + if (!conf || !conf->supported) + return -ENOENT; + + if (feature->flags_reg_num == reg_num) + *value = conf->flags; + else + ret = conf->feature->get(vcpu, conf, true, value); + + return sbi_err_map_linux_errno(ret); +} + +static int kvm_sbi_ext_fwft_set_reg(struct kvm_vcpu *vcpu, unsigned long reg_num, + unsigned long reg_size, const void *reg_val) +{ + const struct kvm_sbi_fwft_feature *feature; + struct kvm_sbi_fwft_config *conf; + unsigned long value; + int ret = 0; + + if (reg_size != sizeof(unsigned long)) + return -EINVAL; + value = *(const unsigned long *)reg_val; + + feature = kvm_sbi_fwft_regnum_to_feature(reg_num); + if (!feature) + return -ENOENT; + + conf = kvm_sbi_fwft_get_config(vcpu, feature->id); + if (!conf || !conf->supported) + return -ENOENT; + + if (feature->flags_reg_num == reg_num) + conf->flags = value & SBI_FWFT_SET_FLAG_LOCK; + else + ret = conf->feature->set(vcpu, conf, true, value); + + return sbi_err_map_linux_errno(ret); +} + const struct kvm_vcpu_sbi_extension vcpu_sbi_ext_fwft = { .extid_start = SBI_EXT_FWFT, .extid_end = SBI_EXT_FWFT, @@ -361,4 +500,10 @@ const struct kvm_vcpu_sbi_extension vcpu_sbi_ext_fwft = { .init = kvm_sbi_ext_fwft_init, .deinit = kvm_sbi_ext_fwft_deinit, .reset = kvm_sbi_ext_fwft_reset, + .have_state = true, + .state_reg_subtype = KVM_REG_RISCV_SBI_FWFT, + .get_state_reg_count = kvm_sbi_ext_fwft_get_reg_count, + .get_state_reg_id = kvm_sbi_ext_fwft_get_reg_id, + .get_state_reg = kvm_sbi_ext_fwft_get_reg, + .set_state_reg = kvm_sbi_ext_fwft_set_reg, };
KVM RISC-V now supports SBI FWFT, so add it to the get-reg-list test.
Signed-off-by: Anup Patel apatel@ventanamicro.com --- .../selftests/kvm/riscv/get-reg-list.c | 28 +++++++++++++++++++ 1 file changed, 28 insertions(+)
diff --git a/tools/testing/selftests/kvm/riscv/get-reg-list.c b/tools/testing/selftests/kvm/riscv/get-reg-list.c index a0b7dabb5040..1bc84f09b4ee 100644 --- a/tools/testing/selftests/kvm/riscv/get-reg-list.c +++ b/tools/testing/selftests/kvm/riscv/get-reg-list.c @@ -128,6 +128,7 @@ bool filter_reg(__u64 reg) case KVM_REG_RISCV_SBI_EXT | KVM_REG_RISCV_SBI_SINGLE | KVM_RISCV_SBI_EXT_DBCN: case KVM_REG_RISCV_SBI_EXT | KVM_REG_RISCV_SBI_SINGLE | KVM_RISCV_SBI_EXT_SUSP: case KVM_REG_RISCV_SBI_EXT | KVM_REG_RISCV_SBI_SINGLE | KVM_RISCV_SBI_EXT_STA: + case KVM_REG_RISCV_SBI_EXT | KVM_REG_RISCV_SBI_SINGLE | KVM_RISCV_SBI_EXT_FWFT: case KVM_REG_RISCV_SBI_EXT | KVM_REG_RISCV_SBI_SINGLE | KVM_RISCV_SBI_EXT_EXPERIMENTAL: case KVM_REG_RISCV_SBI_EXT | KVM_REG_RISCV_SBI_SINGLE | KVM_RISCV_SBI_EXT_VENDOR: return true; @@ -627,6 +628,7 @@ static const char *sbi_ext_single_id_to_str(__u64 reg_off) KVM_SBI_EXT_ARR(KVM_RISCV_SBI_EXT_DBCN), KVM_SBI_EXT_ARR(KVM_RISCV_SBI_EXT_SUSP), KVM_SBI_EXT_ARR(KVM_RISCV_SBI_EXT_STA), + KVM_SBI_EXT_ARR(KVM_RISCV_SBI_EXT_FWFT), KVM_SBI_EXT_ARR(KVM_RISCV_SBI_EXT_EXPERIMENTAL), KVM_SBI_EXT_ARR(KVM_RISCV_SBI_EXT_VENDOR), }; @@ -683,6 +685,17 @@ static const char *sbi_sta_id_to_str(__u64 reg_off) return strdup_printf("KVM_REG_RISCV_SBI_STA | %lld /* UNKNOWN */", reg_off); }
+static const char *sbi_fwft_id_to_str(__u64 reg_off) +{ + switch (reg_off) { + case 0: return "KVM_REG_RISCV_SBI_FWFT | KVM_REG_RISCV_SBI_FWFT_REG(misaligned_deleg.flags)"; + case 1: return "KVM_REG_RISCV_SBI_FWFT | KVM_REG_RISCV_SBI_FWFT_REG(misaligned_deleg.value)"; + case 2: return "KVM_REG_RISCV_SBI_FWFT | KVM_REG_RISCV_SBI_FWFT_REG(pointer_masking.flags)"; + case 3: return "KVM_REG_RISCV_SBI_FWFT | KVM_REG_RISCV_SBI_FWFT_REG(pointer_masking.value)"; + } + return strdup_printf("KVM_REG_RISCV_SBI_STA | %lld /* UNKNOWN */", reg_off); +} + static const char *sbi_id_to_str(const char *prefix, __u64 id) { __u64 reg_off = id & ~(REG_MASK | KVM_REG_RISCV_SBI_STATE); @@ -695,6 +708,8 @@ static const char *sbi_id_to_str(const char *prefix, __u64 id) switch (reg_subtype) { case KVM_REG_RISCV_SBI_STA: return sbi_sta_id_to_str(reg_off); + case KVM_REG_RISCV_SBI_FWFT: + return sbi_fwft_id_to_str(reg_off); }
return strdup_printf("%lld | %lld /* UNKNOWN */", reg_subtype, reg_off); @@ -859,6 +874,14 @@ static __u64 sbi_sta_regs[] = { KVM_REG_RISCV | KVM_REG_SIZE_ULONG | KVM_REG_RISCV_SBI_STATE | KVM_REG_RISCV_SBI_STA | KVM_REG_RISCV_SBI_STA_REG(shmem_hi), };
+static __u64 sbi_fwft_regs[] = { + KVM_REG_RISCV | KVM_REG_SIZE_ULONG | KVM_REG_RISCV_SBI_EXT | KVM_REG_RISCV_SBI_SINGLE | KVM_RISCV_SBI_EXT_FWFT, + KVM_REG_RISCV | KVM_REG_SIZE_ULONG | KVM_REG_RISCV_SBI_STATE | KVM_REG_RISCV_SBI_FWFT | KVM_REG_RISCV_SBI_FWFT_REG(misaligned_deleg.flags), + KVM_REG_RISCV | KVM_REG_SIZE_ULONG | KVM_REG_RISCV_SBI_STATE | KVM_REG_RISCV_SBI_FWFT | KVM_REG_RISCV_SBI_FWFT_REG(misaligned_deleg.value), + KVM_REG_RISCV | KVM_REG_SIZE_ULONG | KVM_REG_RISCV_SBI_STATE | KVM_REG_RISCV_SBI_FWFT | KVM_REG_RISCV_SBI_FWFT_REG(pointer_masking.flags), + KVM_REG_RISCV | KVM_REG_SIZE_ULONG | KVM_REG_RISCV_SBI_STATE | KVM_REG_RISCV_SBI_FWFT | KVM_REG_RISCV_SBI_FWFT_REG(pointer_masking.value), +}; + static __u64 zicbom_regs[] = { KVM_REG_RISCV | KVM_REG_SIZE_ULONG | KVM_REG_RISCV_CONFIG | KVM_REG_RISCV_CONFIG_REG(zicbom_block_size), KVM_REG_RISCV | KVM_REG_SIZE_ULONG | KVM_REG_RISCV_ISA_EXT | KVM_REG_RISCV_ISA_SINGLE | KVM_RISCV_ISA_EXT_ZICBOM, @@ -1010,6 +1033,9 @@ static __u64 vector_regs[] = { #define SUBLIST_SBI_STA \ {"sbi-sta", .feature_type = VCPU_FEATURE_SBI_EXT, .feature = KVM_RISCV_SBI_EXT_STA, \ .regs = sbi_sta_regs, .regs_n = ARRAY_SIZE(sbi_sta_regs),} +#define SUBLIST_SBI_FWFT \ + {"sbi-fwft", .feature_type = VCPU_FEATURE_SBI_EXT, .feature = KVM_RISCV_SBI_EXT_FWFT, \ + .regs = sbi_fwft_regs, .regs_n = ARRAY_SIZE(sbi_fwft_regs),} #define SUBLIST_ZICBOM \ {"zicbom", .feature = KVM_RISCV_ISA_EXT_ZICBOM, .regs = zicbom_regs, .regs_n = ARRAY_SIZE(zicbom_regs),} #define SUBLIST_ZICBOZ \ @@ -1092,6 +1118,7 @@ KVM_SBI_EXT_SUBLIST_CONFIG(sta, STA); KVM_SBI_EXT_SIMPLE_CONFIG(pmu, PMU); KVM_SBI_EXT_SIMPLE_CONFIG(dbcn, DBCN); KVM_SBI_EXT_SIMPLE_CONFIG(susp, SUSP); +KVM_SBI_EXT_SUBLIST_CONFIG(fwft, FWFT);
KVM_ISA_EXT_SUBLIST_CONFIG(aia, AIA); KVM_ISA_EXT_SUBLIST_CONFIG(fp_f, FP_F); @@ -1167,6 +1194,7 @@ struct vcpu_reg_list *vcpu_configs[] = { &config_sbi_pmu, &config_sbi_dbcn, &config_sbi_susp, + &config_sbi_fwft, &config_aia, &config_fp_f, &config_fp_d,
2025-08-14T21:25:42+05:30, Anup Patel apatel@ventanamicro.com:
This series adds ONE_REG interface for SBI FWFT extension implemented by KVM RISC-V.
I think it would be better to ONE_REG the CSRs (medeleg/menvcfg), or at least expose their CSR fields (each sensible medeleg bit, PMM, ...) through kvm_riscv_config, than to couple this with SBI/FWFT.
The controlled behavior is defined by the ISA, and userspace might want to configure the S-mode execution environment even when SBI/FWFT is not present, which is not possible with the current design.
Is there a benefit in expressing the ISA model through SBI/FWFT?
Thanks.
On Mon, Aug 18, 2025 at 3:59 PM Radim Krčmář rkrcmar@ventanamicro.com wrote:
2025-08-14T21:25:42+05:30, Anup Patel apatel@ventanamicro.com:
This series adds ONE_REG interface for SBI FWFT extension implemented by KVM RISC-V.
I think it would be better to ONE_REG the CSRs (medeleg/menvcfg), or at least expose their CSR fields (each sensible medeleg bit, PMM, ...) through kvm_riscv_config, than to couple this with SBI/FWFT.
The controlled behavior is defined by the ISA, and userspace might want to configure the S-mode execution environment even when SBI/FWFT is not present, which is not possible with the current design.
Is there a benefit in expressing the ISA model through SBI/FWFT?
Exposing medeleg/menvcfg is not the right approach because a Guest/VM does not have M-mode hence it is not appropriate to expose m<xyz> CSRs via ONE_REG interface. This also aligns with H-extension architecture which does not virtualize M-mode. We already had discussions about this in the past.
As such, we have two options. One option is to expose hedeleg/henvcfg via kvm_riscv_config and another option is to have a separate ONE_REG for each FWFT feature.
Separate ONE_REG registers for each FWFT feature is better than directly exposing hedeleg/henvcfg via ONE_REG because:
1) Once nested virtualization lands, we will be having separate hedeleg/henvcfg as part of nested virtualization state of Guest which is trap-n-emulated by KVM. The existence of hedeleg/henvcfg in kvm_riscv_config and nested virtualization state will only create more confusion.
2) Not all bits in hedeleg/henvcfg are used for FWFT since quite a few bits are programmed with fixed value based on KVM implementation choices (which may change in future). Also, things like set_debug_ioctl() change hedeleg at runtime which allow KVM user space to decide who takes breakpoint traps from Guest/VM. This means value saved/restored through hedeleg/henvcfg in kvm_riscv_config becomes specific to the kernel version and specific to host ISA features.
3) We anyway need to provide ONE_REG interface to save/restore FWFT feature flags so it's better to keep the FWFT feature value as part of the same ONE_REG interface.
4) The availability of quite a few FWFT features is dependent on corresponding ISA extensions so having separate ONE_REG registers of each FWFT feature allows get_reg_list_ioctl() to provide KVM user-space only available FWFT feature registers.
Regards, Anup
2025-08-19T12:00:43+05:30, Anup Patel apatel@ventanamicro.com:
On Mon, Aug 18, 2025 at 3:59 PM Radim Krčmář rkrcmar@ventanamicro.com wrote:
2025-08-14T21:25:42+05:30, Anup Patel apatel@ventanamicro.com:
This series adds ONE_REG interface for SBI FWFT extension implemented by KVM RISC-V.
I think it would be better to ONE_REG the CSRs (medeleg/menvcfg), or at least expose their CSR fields (each sensible medeleg bit, PMM, ...) through kvm_riscv_config, than to couple this with SBI/FWFT.
The controlled behavior is defined by the ISA, and userspace might want to configure the S-mode execution environment even when SBI/FWFT is not present, which is not possible with the current design.
Is there a benefit in expressing the ISA model through SBI/FWFT?
Exposing medeleg/menvcfg is not the right approach because a Guest/VM does not have M-mode hence it is not appropriate to expose m<xyz> CSRs via ONE_REG interface. This also aligns with H-extension architecture which does not virtualize M-mode.
We already have mvendorid, marchid, and mipid in kvm_riscv_config.
The virtualized M-mode is userspace+KVM. (KVM doesn't allow userspace to configure most things now, but I think we'll have to change that when getting ready for production.)
We already had discussions about this in the past.
As such, we have two options. One option is to expose hedeleg/henvcfg via kvm_riscv_config and another option is to have a separate ONE_REG for each FWFT feature.
Separate ONE_REG registers for each FWFT feature is better than directly exposing hedeleg/henvcfg via ONE_REG because:
- Once nested virtualization lands, we will be having separate
hedeleg/henvcfg as part of nested virtualization state of Guest which is trap-n-emulated by KVM. The existence of hedeleg/henvcfg in kvm_riscv_config and nested virtualization state will only create more confusion.
Right, the userspace registers for this can't be called h*.
- Not all bits in hedeleg/henvcfg are used for FWFT since quite
a few bits are programmed with fixed value based on KVM implementation choices (which may change in future).
Yes, we'll want to expose some to userspace.
Also,things like set_debug_ioctl() change hedeleg at runtime which allow KVM user space to decide who takes breakpoint traps from Guest/VM.
This is still doable. The clear hedeleg bit does not have to change the virtualized behavior -- if the guest is expecting to see breakpoint traps, then even if userspace+KVM configure the architecture to direct the traps to the hypervisor, they must then forward the breakpoints that were supposed to be delivered to the guest.
This means value saved/restoredthrough hedeleg/henvcfg in kvm_riscv_config becomes specific to the kernel version and specific to host ISA features.
Hedeleg/henvcfg bits do not have to be the same as userspace interface bits -- KVM always has to distinguish what the userspace wants to virtualize, and what the KVM changed for its own reasons.
- We anyway need to provide ONE_REG interface to
save/restore FWFT feature flags so it's better to keep the FWFT feature value as part of the same ONE_REG interface.
I think we want to have SBI in userspace (especially for single-shot ecalls like FWFT). The userspace implementation will want an interface to set the ISA bits, and it's very awkward with the proposed design.
Flags can to stay, in case the userpace wants to accelerate FWFT.
- The availability of quite a few FWFT features is dependent
on corresponding ISA extensions so having separate ONE_REG registers of each FWFT feature allows get_reg_list_ioctl() to provide KVM user-space only available FWFT feature registers.
Yes, but similarly the userspace would be forbidden from setting bits that cannot be expressed in henvcfg/hededeg.
There are also behaviors we want to configure that do not have a FWFT toggle. e.g. the recent patches for delegation of illegal-instruction exceptions that changed the guest behavior -- someone might want to keep incrementing the SBI PMU counter, and someone will want to forward them to be implemented in userspace (when developing a new extension, because most of the existing ISA can still be accelerated by KVM).
For general virtualization, we want to be able to configure the following behavior for each exception that would go to the virtualized M-mode: 0) delegated to the guest 1) implemented by userspace 2-N) implementations by KVM (ideally zero or one)
We can have medeleg, and another method to decide how to handle trapped exceptions, but it probably makes more sense to have a per-exception ONE_REG that sets how each exception behaves.
The FWFT value could be a part of a more general interface.
Thanks.
On Tue, Aug 19, 2025 at 5:13 PM Radim Krčmář rkrcmar@ventanamicro.com wrote:
2025-08-19T12:00:43+05:30, Anup Patel apatel@ventanamicro.com:
On Mon, Aug 18, 2025 at 3:59 PM Radim Krčmář rkrcmar@ventanamicro.com wrote:
2025-08-14T21:25:42+05:30, Anup Patel apatel@ventanamicro.com:
This series adds ONE_REG interface for SBI FWFT extension implemented by KVM RISC-V.
I think it would be better to ONE_REG the CSRs (medeleg/menvcfg), or at least expose their CSR fields (each sensible medeleg bit, PMM, ...) through kvm_riscv_config, than to couple this with SBI/FWFT.
The controlled behavior is defined by the ISA, and userspace might want to configure the S-mode execution environment even when SBI/FWFT is not present, which is not possible with the current design.
Is there a benefit in expressing the ISA model through SBI/FWFT?
Exposing medeleg/menvcfg is not the right approach because a Guest/VM does not have M-mode hence it is not appropriate to expose m<xyz> CSRs via ONE_REG interface. This also aligns with H-extension architecture which does not virtualize M-mode.
We already have mvendorid, marchid, and mipid in kvm_riscv_config.
The mvendorid, marchid, and mipid are accessible via SBI BASE extension but not any other M-mode CSRs hence these are special.
The virtualized M-mode is userspace+KVM. (KVM doesn't allow userspace to configure most things now, but I think we'll have to change that when getting ready for production.)
The RISC-V architecture is not designed to virtualize M-mode and there is no practical use-case for virtualized M-mode hence WE WON'T BE SUPPORTING IT IN KVM RISC-V.
FYI, the KVM ARM64 does not virtualize EL3 either and it is already in production so please stop making random arguments for requiring virtualized M-mode for production.
We already had discussions about this in the past.
As such, we have two options. One option is to expose hedeleg/henvcfg via kvm_riscv_config and another option is to have a separate ONE_REG for each FWFT feature.
Separate ONE_REG registers for each FWFT feature is better than directly exposing hedeleg/henvcfg via ONE_REG because:
- Once nested virtualization lands, we will be having separate
hedeleg/henvcfg as part of nested virtualization state of Guest which is trap-n-emulated by KVM. The existence of hedeleg/henvcfg in kvm_riscv_config and nested virtualization state will only create more confusion.
Right, the userspace registers for this can't be called h*.
- Not all bits in hedeleg/henvcfg are used for FWFT since quite
a few bits are programmed with fixed value based on KVM implementation choices (which may change in future).
Yes, we'll want to expose some to userspace.
Also,things like set_debug_ioctl() change hedeleg at runtime which allow KVM user space to decide who takes breakpoint traps from Guest/VM.
This is still doable. The clear hedeleg bit does not have to change the virtualized behavior -- if the guest is expecting to see breakpoint traps, then even if userspace+KVM configure the architecture to direct the traps to the hypervisor, they must then forward the breakpoints that were supposed to be delivered to the guest.
This means value saved/restoredthrough hedeleg/henvcfg in kvm_riscv_config becomes specific to the kernel version and specific to host ISA features.
Hedeleg/henvcfg bits do not have to be the same as userspace interface bits -- KVM always has to distinguish what the userspace wants to virtualize, and what the KVM changed for its own reasons.
- We anyway need to provide ONE_REG interface to
save/restore FWFT feature flags so it's better to keep the FWFT feature value as part of the same ONE_REG interface.
I think we want to have SBI in userspace (especially for single-shot ecalls like FWFT). The userspace implementation will want an interface to set the ISA bits, and it's very awkward with the proposed design.
Flags can to stay, in case the userpace wants to accelerate FWFT.
- The availability of quite a few FWFT features is dependent
on corresponding ISA extensions so having separate ONE_REG registers of each FWFT feature allows get_reg_list_ioctl() to provide KVM user-space only available FWFT feature registers.
Yes, but similarly the userspace would be forbidden from setting bits that cannot be expressed in henvcfg/hededeg.
Instead of having henvcfg/hededeg via ONE_REG with restrictions, it's much cleaner and maintainable to have a separate ONE_REG register for each FWFT feature.
There are also behaviors we want to configure that do not have a FWFT toggle. e.g. the recent patches for delegation of illegal-instruction exceptions that changed the guest behavior -- someone might want to keep incrementing the SBI PMU counter, and someone will want to forward them to be implemented in userspace (when developing a new extension, because most of the existing ISA can still be accelerated by KVM).
There are alternate approaches for supporting SBI PMU counters in user-mode where we don't need virtualized M-mode. In any case, we will support user-mode emulated perf counters only when absolutely needed.
For general virtualization, we want to be able to configure the following behavior for each exception that would go to the virtualized M-mode: 0) delegated to the guest
- implemented by userspace
2-N) implementations by KVM (ideally zero or one)
We can have medeleg, and another method to decide how to handle trapped exceptions, but it probably makes more sense to have a per-exception ONE_REG that sets how each exception behaves.
No pointing in discussing this further since we won't be supporting virtualized M-mode.
-- Anup
2025-08-19T21:22:27+05:30, Anup Patel apatel@ventanamicro.com:
On Tue, Aug 19, 2025 at 5:13 PM Radim Krčmář rkrcmar@ventanamicro.com wrote:
2025-08-19T12:00:43+05:30, Anup Patel apatel@ventanamicro.com:
On Mon, Aug 18, 2025 at 3:59 PM Radim Krčmář rkrcmar@ventanamicro.com wrote:
2025-08-14T21:25:42+05:30, Anup Patel apatel@ventanamicro.com:
This series adds ONE_REG interface for SBI FWFT extension implemented by KVM RISC-V.
I think it would be better to ONE_REG the CSRs (medeleg/menvcfg), or at least expose their CSR fields (each sensible medeleg bit, PMM, ...) through kvm_riscv_config, than to couple this with SBI/FWFT.
The controlled behavior is defined by the ISA, and userspace might want to configure the S-mode execution environment even when SBI/FWFT is not present, which is not possible with the current design.
Is there a benefit in expressing the ISA model through SBI/FWFT?
Exposing medeleg/menvcfg is not the right approach because a Guest/VM does not have M-mode hence it is not appropriate to expose m<xyz> CSRs via ONE_REG interface. This also aligns with H-extension architecture which does not virtualize M-mode.
We already have mvendorid, marchid, and mipid in kvm_riscv_config.
The mvendorid, marchid, and mipid are accessible via SBI BASE extension but not any other M-mode CSRs hence these are special.
The virtualized M-mode is userspace+KVM. (KVM doesn't allow userspace to configure most things now, but I think we'll have to change that when getting ready for production.)
The RISC-V architecture is not designed to virtualize M-mode and there is no practical use-case for virtualized M-mode hence WE WON'T BE SUPPORTING IT IN KVM RISC-V.
Oh, sorry for the misunderstanding, I'll be clearer next time and talk about implementation of the supervisor execution environment. KVM+userspace provides SEE to the VS-mode, which is to VS-mode as what M-mode is to S-mode, hence I called KVM+userspace a virtualized M-mode.
FYI, the KVM ARM64 does not virtualize EL3 either and it is already in production so please stop making random arguments for requiring virtualized M-mode for production.
Yeah, I agree that we don't need it, I just had to provide so many examples in the previous discussion that I went into quite niche cases.
The increased flexibility is similarly useful for more important cases: we can't avoid "virtualized M-mode"/SEE, but we don't have to completely implement it in HS-mode.
For general virtualization, we want to be able to configure the following behavior for each exception that would go to the virtualized M-mode: 0) delegated to the guest
- implemented by userspace
2-N) implementations by KVM (ideally zero or one)
We can have medeleg, and another method to decide how to handle trapped exceptions, but it probably makes more sense to have a per-exception ONE_REG that sets how each exception behaves.
No pointing in discussing this further since we won't be supporting virtualized M-mode.
I understand, back to the current series:
I think we need to provide means with which userspace can control which FWFT features are enabled, because KVM just exposes everything it know and hardware supports right now: 1) Migration between different systems would be hindered 2) We couldn't add more FWFT features without breaking the SEE
The (2) is similar to how we must set ".default_disabled = true" to current FWFT, because KVM can't be changing the SEE for userspace.
Do you want me to send a patch that inverts the default, to make all future SBI extension start as disabled, so we can't easily repeat the mistake in the future?
Thanks.
On Tue, Aug 19, 2025 at 11:05 PM Radim Krčmář rkrcmar@ventanamicro.com wrote:
2025-08-19T21:22:27+05:30, Anup Patel apatel@ventanamicro.com:
On Tue, Aug 19, 2025 at 5:13 PM Radim Krčmář rkrcmar@ventanamicro.com wrote:
2025-08-19T12:00:43+05:30, Anup Patel apatel@ventanamicro.com:
On Mon, Aug 18, 2025 at 3:59 PM Radim Krčmář rkrcmar@ventanamicro.com wrote:
2025-08-14T21:25:42+05:30, Anup Patel apatel@ventanamicro.com:
This series adds ONE_REG interface for SBI FWFT extension implemented by KVM RISC-V.
I think it would be better to ONE_REG the CSRs (medeleg/menvcfg), or at least expose their CSR fields (each sensible medeleg bit, PMM, ...) through kvm_riscv_config, than to couple this with SBI/FWFT.
The controlled behavior is defined by the ISA, and userspace might want to configure the S-mode execution environment even when SBI/FWFT is not present, which is not possible with the current design.
Is there a benefit in expressing the ISA model through SBI/FWFT?
Exposing medeleg/menvcfg is not the right approach because a Guest/VM does not have M-mode hence it is not appropriate to expose m<xyz> CSRs via ONE_REG interface. This also aligns with H-extension architecture which does not virtualize M-mode.
We already have mvendorid, marchid, and mipid in kvm_riscv_config.
The mvendorid, marchid, and mipid are accessible via SBI BASE extension but not any other M-mode CSRs hence these are special.
The virtualized M-mode is userspace+KVM. (KVM doesn't allow userspace to configure most things now, but I think we'll have to change that when getting ready for production.)
The RISC-V architecture is not designed to virtualize M-mode and there is no practical use-case for virtualized M-mode hence WE WON'T BE SUPPORTING IT IN KVM RISC-V.
Oh, sorry for the misunderstanding, I'll be clearer next time and talk about implementation of the supervisor execution environment. KVM+userspace provides SEE to the VS-mode, which is to VS-mode as what M-mode is to S-mode, hence I called KVM+userspace a virtualized M-mode.
FYI, the KVM ARM64 does not virtualize EL3 either and it is already in production so please stop making random arguments for requiring virtualized M-mode for production.
Yeah, I agree that we don't need it, I just had to provide so many examples in the previous discussion that I went into quite niche cases.
The increased flexibility is similarly useful for more important cases: we can't avoid "virtualized M-mode"/SEE, but we don't have to completely implement it in HS-mode.
For general virtualization, we want to be able to configure the following behavior for each exception that would go to the virtualized M-mode: 0) delegated to the guest
- implemented by userspace
2-N) implementations by KVM (ideally zero or one)
We can have medeleg, and another method to decide how to handle trapped exceptions, but it probably makes more sense to have a per-exception ONE_REG that sets how each exception behaves.
No pointing in discussing this further since we won't be supporting virtualized M-mode.
I understand, back to the current series:
I think we need to provide means with which userspace can control which FWFT features are enabled, because KVM just exposes everything it know and hardware supports right now:
- Migration between different systems would be hindered
- We couldn't add more FWFT features without breaking the SEE
The FWFT features are defined to be backward compatible by SBI spec and these features only affect after Guest/VM has configured them. This means if a Guest/VM is not aware of SBI FWFT then it won't configure FWFT features and Guest/VM should still work fine.
The (2) is similar to how we must set ".default_disabled = true" to current FWFT, because KVM can't be changing the SEE for userspace.
Not needed. We only keep an SBI extension disabled by default if it is being forwarded to KVM userspace (e.g. DBCN, SUSP) because KVM userspace needs to opt-in for such SBI extension handling. In other cases, the SBI extension is enabled by default and KVM userspace can explicitly disable it for Guest/VM using ONE_REG interface. This is true for ISA extensions as well.
Do you want me to send a patch that inverts the default, to make all future SBI extension start as disabled, so we can't easily repeat the mistake in the future?
No need to send any patch. Please focus on your current assignments.
Regards, Anup
linux-kselftest-mirror@lists.linaro.org
-
Andrew Jones -
Anup Patel -
Nutty.Liu -
Radim Krčmář