Currently, the every vcpu only stores the ISA extensions in a unsigned long
which is not scalable as number of extensions will continue to grow.
Using a bitmap allows the ISA extension to support any number of
extensions. The CONFIG one reg interface implementation is modified to
support the bitmap as well. But it is meant only for base extensions.
Thus, the first element of the bitmap array is sufficient for that
interface.
In the future, all the new multi-letter extensions must use the
ISA_EXT one reg interface that allows enabling/disabling any extension
now.
Signed-off-by: Atish Patra <[email protected]>
---
arch/riscv/include/asm/kvm_host.h | 3 +-
arch/riscv/include/asm/kvm_vcpu_fp.h | 8 +--
arch/riscv/kvm/vcpu.c | 81 ++++++++++++++--------------
arch/riscv/kvm/vcpu_fp.c | 27 +++++-----
4 files changed, 59 insertions(+), 60 deletions(-)
diff --git a/arch/riscv/include/asm/kvm_host.h b/arch/riscv/include/asm/kvm_host.h
index 319c8aeb42af..c749cdacbd63 100644
--- a/arch/riscv/include/asm/kvm_host.h
+++ b/arch/riscv/include/asm/kvm_host.h
@@ -14,6 +14,7 @@
#include <linux/kvm_types.h>
#include <linux/spinlock.h>
#include <asm/csr.h>
+#include <asm/hwcap.h>
#include <asm/kvm_vcpu_fp.h>
#include <asm/kvm_vcpu_timer.h>
@@ -170,7 +171,7 @@ struct kvm_vcpu_arch {
int last_exit_cpu;
/* ISA feature bits (similar to MISA) */
- unsigned long isa;
+ DECLARE_BITMAP(isa, RISCV_ISA_EXT_MAX);
/* SSCRATCH, STVEC, and SCOUNTEREN of Host */
unsigned long host_sscratch;
diff --git a/arch/riscv/include/asm/kvm_vcpu_fp.h b/arch/riscv/include/asm/kvm_vcpu_fp.h
index 4da9b8e0f050..e86bb67f2a8a 100644
--- a/arch/riscv/include/asm/kvm_vcpu_fp.h
+++ b/arch/riscv/include/asm/kvm_vcpu_fp.h
@@ -22,9 +22,9 @@ void __kvm_riscv_fp_d_restore(struct kvm_cpu_context *context);
void kvm_riscv_vcpu_fp_reset(struct kvm_vcpu *vcpu);
void kvm_riscv_vcpu_guest_fp_save(struct kvm_cpu_context *cntx,
- unsigned long isa);
+ unsigned long *isa);
void kvm_riscv_vcpu_guest_fp_restore(struct kvm_cpu_context *cntx,
- unsigned long isa);
+ unsigned long *isa);
void kvm_riscv_vcpu_host_fp_save(struct kvm_cpu_context *cntx);
void kvm_riscv_vcpu_host_fp_restore(struct kvm_cpu_context *cntx);
#else
@@ -32,12 +32,12 @@ static inline void kvm_riscv_vcpu_fp_reset(struct kvm_vcpu *vcpu)
{
}
static inline void kvm_riscv_vcpu_guest_fp_save(struct kvm_cpu_context *cntx,
- unsigned long isa)
+ unsigned long *isa)
{
}
static inline void kvm_riscv_vcpu_guest_fp_restore(
struct kvm_cpu_context *cntx,
- unsigned long isa)
+ unsigned long *isa)
{
}
static inline void kvm_riscv_vcpu_host_fp_save(struct kvm_cpu_context *cntx)
diff --git a/arch/riscv/kvm/vcpu.c b/arch/riscv/kvm/vcpu.c
index 7f4ad5e4373a..cb2a65b5d563 100644
--- a/arch/riscv/kvm/vcpu.c
+++ b/arch/riscv/kvm/vcpu.c
@@ -46,8 +46,19 @@ const struct kvm_stats_header kvm_vcpu_stats_header = {
riscv_isa_extension_mask(i) | \
riscv_isa_extension_mask(m))
-#define KVM_RISCV_ISA_ALLOWED (KVM_RISCV_ISA_DISABLE_ALLOWED | \
- KVM_RISCV_ISA_DISABLE_NOT_ALLOWED)
+#define KVM_RISCV_ISA_MASK GENMASK(25, 0)
+
+/* Mapping between KVM ISA Extension ID & Host ISA extension ID */
+static unsigned long kvm_isa_ext_arr[] = {
+ RISCV_ISA_EXT_a,
+ RISCV_ISA_EXT_c,
+ RISCV_ISA_EXT_d,
+ RISCV_ISA_EXT_f,
+ RISCV_ISA_EXT_h,
+ RISCV_ISA_EXT_i,
+ RISCV_ISA_EXT_m,
+ RISCV_ISA_EXT_SSCOFPMF,
+};
static void kvm_riscv_reset_vcpu(struct kvm_vcpu *vcpu)
{
@@ -99,13 +110,20 @@ int kvm_arch_vcpu_create(struct kvm_vcpu *vcpu)
{
struct kvm_cpu_context *cntx;
struct kvm_vcpu_csr *reset_csr = &vcpu->arch.guest_reset_csr;
+ unsigned long host_isa, i;
/* Mark this VCPU never ran */
vcpu->arch.ran_atleast_once = false;
vcpu->arch.mmu_page_cache.gfp_zero = __GFP_ZERO;
+ bitmap_zero(vcpu->arch.isa, RISCV_ISA_EXT_MAX);
/* Setup ISA features available to VCPU */
- vcpu->arch.isa = riscv_isa_extension_base(NULL) & KVM_RISCV_ISA_ALLOWED;
+ for (i = 0; i < ARRAY_SIZE(kvm_isa_ext_arr); i++) {
+ host_isa = kvm_isa_ext_arr[i];
+ if (__riscv_isa_extension_available(NULL, host_isa) &&
+ host_isa != RISCV_ISA_EXT_h)
+ set_bit(host_isa, vcpu->arch.isa);
+ }
/* Setup VCPU hfence queue */
spin_lock_init(&vcpu->arch.hfence_lock);
@@ -199,7 +217,7 @@ static int kvm_riscv_vcpu_get_reg_config(struct kvm_vcpu *vcpu,
switch (reg_num) {
case KVM_REG_RISCV_CONFIG_REG(isa):
- reg_val = vcpu->arch.isa;
+ reg_val = vcpu->arch.isa[0] & KVM_RISCV_ISA_MASK;
break;
default:
return -EINVAL;
@@ -220,6 +238,7 @@ static int kvm_riscv_vcpu_set_reg_config(struct kvm_vcpu *vcpu,
KVM_REG_SIZE_MASK |
KVM_REG_RISCV_CONFIG);
unsigned long reg_val;
+ unsigned long isa_mask;
if (KVM_REG_SIZE(reg->id) != sizeof(unsigned long))
return -EINVAL;
@@ -227,13 +246,19 @@ static int kvm_riscv_vcpu_set_reg_config(struct kvm_vcpu *vcpu,
if (copy_from_user(®_val, uaddr, KVM_REG_SIZE(reg->id)))
return -EFAULT;
+ /* This ONE REG interface is only defined for single letter extensions */
+ if (fls(reg_val) >= RISCV_ISA_EXT_BASE)
+ return -EINVAL;
+
switch (reg_num) {
case KVM_REG_RISCV_CONFIG_REG(isa):
if (!vcpu->arch.ran_atleast_once) {
/* Ignore the disable request for these extensions */
- vcpu->arch.isa = reg_val | KVM_RISCV_ISA_DISABLE_NOT_ALLOWED;
- vcpu->arch.isa &= riscv_isa_extension_base(NULL);
- vcpu->arch.isa &= KVM_RISCV_ISA_ALLOWED;
+ isa_mask = (reg_val | KVM_RISCV_ISA_DISABLE_NOT_ALLOWED);
+ isa_mask &= riscv_isa_extension_base(NULL);
+ /* Do not modify anything beyond single letter extensions */
+ isa_mask |= (~KVM_RISCV_ISA_MASK);
+ bitmap_and(vcpu->arch.isa, vcpu->arch.isa, &isa_mask, RISCV_ISA_EXT_MAX);
kvm_riscv_vcpu_fp_reset(vcpu);
} else {
return -EOPNOTSUPP;
@@ -374,17 +399,6 @@ static int kvm_riscv_vcpu_set_reg_csr(struct kvm_vcpu *vcpu,
return 0;
}
-/* Mapping between KVM ISA Extension ID & Host ISA extension ID */
-static unsigned long kvm_isa_ext_arr[] = {
- RISCV_ISA_EXT_a,
- RISCV_ISA_EXT_c,
- RISCV_ISA_EXT_d,
- RISCV_ISA_EXT_f,
- RISCV_ISA_EXT_h,
- RISCV_ISA_EXT_i,
- RISCV_ISA_EXT_m,
-};
-
static int kvm_riscv_vcpu_get_reg_isa_ext(struct kvm_vcpu *vcpu,
const struct kvm_one_reg *reg)
{
@@ -403,7 +417,7 @@ static int kvm_riscv_vcpu_get_reg_isa_ext(struct kvm_vcpu *vcpu,
return -EINVAL;
host_isa_ext = kvm_isa_ext_arr[reg_num];
- if (__riscv_isa_extension_available(&vcpu->arch.isa, host_isa_ext))
+ if (__riscv_isa_extension_available(vcpu->arch.isa, host_isa_ext))
reg_val = 1; /* Mark the given extension as available */
if (copy_to_user(uaddr, ®_val, KVM_REG_SIZE(reg->id)))
@@ -437,30 +451,17 @@ static int kvm_riscv_vcpu_set_reg_isa_ext(struct kvm_vcpu *vcpu,
if (!__riscv_isa_extension_available(NULL, host_isa_ext))
return -EOPNOTSUPP;
- if (host_isa_ext >= RISCV_ISA_EXT_BASE &&
- host_isa_ext < RISCV_ISA_EXT_MAX) {
- /*
- * Multi-letter ISA extension. Currently there is no provision
- * to enable/disable the multi-letter ISA extensions for guests.
- * Return success if the request is to enable any ISA extension
- * that is available in the hardware.
- * Return -EOPNOTSUPP otherwise.
- */
- if (!reg_val)
- return -EOPNOTSUPP;
- else
- return 0;
- }
-
- /* Single letter base ISA extension */
if (!vcpu->arch.ran_atleast_once) {
+ /* All multi-letter extension and a few single letter extension can be disabled */
host_isa_ext_mask = BIT_MASK(host_isa_ext);
- if (!reg_val && (host_isa_ext_mask & KVM_RISCV_ISA_DISABLE_ALLOWED))
- vcpu->arch.isa &= ~host_isa_ext_mask;
+ if (!reg_val &&
+ ((host_isa_ext_mask & KVM_RISCV_ISA_DISABLE_ALLOWED) ||
+ ((host_isa_ext >= RISCV_ISA_EXT_BASE) && (host_isa_ext < RISCV_ISA_EXT_MAX))))
+ clear_bit(host_isa_ext, vcpu->arch.isa);
+ else if (reg_val == 1 && (host_isa_ext != RISCV_ISA_EXT_h))
+ set_bit(host_isa_ext, vcpu->arch.isa);
else
- vcpu->arch.isa |= host_isa_ext_mask;
- vcpu->arch.isa &= riscv_isa_extension_base(NULL);
- vcpu->arch.isa &= KVM_RISCV_ISA_ALLOWED;
+ return -EINVAL;
kvm_riscv_vcpu_fp_reset(vcpu);
} else {
return -EOPNOTSUPP;
diff --git a/arch/riscv/kvm/vcpu_fp.c b/arch/riscv/kvm/vcpu_fp.c
index d4308c512007..748a8f6a9b5d 100644
--- a/arch/riscv/kvm/vcpu_fp.c
+++ b/arch/riscv/kvm/vcpu_fp.c
@@ -16,12 +16,11 @@
#ifdef CONFIG_FPU
void kvm_riscv_vcpu_fp_reset(struct kvm_vcpu *vcpu)
{
- unsigned long isa = vcpu->arch.isa;
struct kvm_cpu_context *cntx = &vcpu->arch.guest_context;
cntx->sstatus &= ~SR_FS;
- if (riscv_isa_extension_available(&isa, f) ||
- riscv_isa_extension_available(&isa, d))
+ if (riscv_isa_extension_available(vcpu->arch.isa, f) ||
+ riscv_isa_extension_available(vcpu->arch.isa, d))
cntx->sstatus |= SR_FS_INITIAL;
else
cntx->sstatus |= SR_FS_OFF;
@@ -34,24 +33,24 @@ static void kvm_riscv_vcpu_fp_clean(struct kvm_cpu_context *cntx)
}
void kvm_riscv_vcpu_guest_fp_save(struct kvm_cpu_context *cntx,
- unsigned long isa)
+ unsigned long *isa)
{
if ((cntx->sstatus & SR_FS) == SR_FS_DIRTY) {
- if (riscv_isa_extension_available(&isa, d))
+ if (riscv_isa_extension_available(isa, d))
__kvm_riscv_fp_d_save(cntx);
- else if (riscv_isa_extension_available(&isa, f))
+ else if (riscv_isa_extension_available(isa, f))
__kvm_riscv_fp_f_save(cntx);
kvm_riscv_vcpu_fp_clean(cntx);
}
}
void kvm_riscv_vcpu_guest_fp_restore(struct kvm_cpu_context *cntx,
- unsigned long isa)
+ unsigned long *isa)
{
if ((cntx->sstatus & SR_FS) != SR_FS_OFF) {
- if (riscv_isa_extension_available(&isa, d))
+ if (riscv_isa_extension_available(isa, d))
__kvm_riscv_fp_d_restore(cntx);
- else if (riscv_isa_extension_available(&isa, f))
+ else if (riscv_isa_extension_available(isa, f))
__kvm_riscv_fp_f_restore(cntx);
kvm_riscv_vcpu_fp_clean(cntx);
}
@@ -80,7 +79,6 @@ int kvm_riscv_vcpu_get_reg_fp(struct kvm_vcpu *vcpu,
unsigned long rtype)
{
struct kvm_cpu_context *cntx = &vcpu->arch.guest_context;
- unsigned long isa = vcpu->arch.isa;
unsigned long __user *uaddr =
(unsigned long __user *)(unsigned long)reg->addr;
unsigned long reg_num = reg->id & ~(KVM_REG_ARCH_MASK |
@@ -89,7 +87,7 @@ int kvm_riscv_vcpu_get_reg_fp(struct kvm_vcpu *vcpu,
void *reg_val;
if ((rtype == KVM_REG_RISCV_FP_F) &&
- riscv_isa_extension_available(&isa, f)) {
+ riscv_isa_extension_available(vcpu->arch.isa, f)) {
if (KVM_REG_SIZE(reg->id) != sizeof(u32))
return -EINVAL;
if (reg_num == KVM_REG_RISCV_FP_F_REG(fcsr))
@@ -100,7 +98,7 @@ int kvm_riscv_vcpu_get_reg_fp(struct kvm_vcpu *vcpu,
else
return -EINVAL;
} else if ((rtype == KVM_REG_RISCV_FP_D) &&
- riscv_isa_extension_available(&isa, d)) {
+ riscv_isa_extension_available(vcpu->arch.isa, d)) {
if (reg_num == KVM_REG_RISCV_FP_D_REG(fcsr)) {
if (KVM_REG_SIZE(reg->id) != sizeof(u32))
return -EINVAL;
@@ -126,7 +124,6 @@ int kvm_riscv_vcpu_set_reg_fp(struct kvm_vcpu *vcpu,
unsigned long rtype)
{
struct kvm_cpu_context *cntx = &vcpu->arch.guest_context;
- unsigned long isa = vcpu->arch.isa;
unsigned long __user *uaddr =
(unsigned long __user *)(unsigned long)reg->addr;
unsigned long reg_num = reg->id & ~(KVM_REG_ARCH_MASK |
@@ -135,7 +132,7 @@ int kvm_riscv_vcpu_set_reg_fp(struct kvm_vcpu *vcpu,
void *reg_val;
if ((rtype == KVM_REG_RISCV_FP_F) &&
- riscv_isa_extension_available(&isa, f)) {
+ riscv_isa_extension_available(vcpu->arch.isa, f)) {
if (KVM_REG_SIZE(reg->id) != sizeof(u32))
return -EINVAL;
if (reg_num == KVM_REG_RISCV_FP_F_REG(fcsr))
@@ -146,7 +143,7 @@ int kvm_riscv_vcpu_set_reg_fp(struct kvm_vcpu *vcpu,
else
return -EINVAL;
} else if ((rtype == KVM_REG_RISCV_FP_D) &&
- riscv_isa_extension_available(&isa, d)) {
+ riscv_isa_extension_available(vcpu->arch.isa, d)) {
if (reg_num == KVM_REG_RISCV_FP_D_REG(fcsr)) {
if (KVM_REG_SIZE(reg->id) != sizeof(u32))
return -EINVAL;
--
2.25.1
On Tue, Jun 21, 2022 at 5:13 AM Atish Patra <[email protected]> wrote:
>
> Currently, the every vcpu only stores the ISA extensions in a unsigned long
> which is not scalable as number of extensions will continue to grow.
> Using a bitmap allows the ISA extension to support any number of
> extensions. The CONFIG one reg interface implementation is modified to
> support the bitmap as well. But it is meant only for base extensions.
> Thus, the first element of the bitmap array is sufficient for that
> interface.
>
> In the future, all the new multi-letter extensions must use the
> ISA_EXT one reg interface that allows enabling/disabling any extension
> now.
>
> Signed-off-by: Atish Patra <[email protected]>
> ---
> arch/riscv/include/asm/kvm_host.h | 3 +-
> arch/riscv/include/asm/kvm_vcpu_fp.h | 8 +--
> arch/riscv/kvm/vcpu.c | 81 ++++++++++++++--------------
> arch/riscv/kvm/vcpu_fp.c | 27 +++++-----
> 4 files changed, 59 insertions(+), 60 deletions(-)
>
> diff --git a/arch/riscv/include/asm/kvm_host.h b/arch/riscv/include/asm/kvm_host.h
> index 319c8aeb42af..c749cdacbd63 100644
> --- a/arch/riscv/include/asm/kvm_host.h
> +++ b/arch/riscv/include/asm/kvm_host.h
> @@ -14,6 +14,7 @@
> #include <linux/kvm_types.h>
> #include <linux/spinlock.h>
> #include <asm/csr.h>
> +#include <asm/hwcap.h>
> #include <asm/kvm_vcpu_fp.h>
> #include <asm/kvm_vcpu_timer.h>
>
> @@ -170,7 +171,7 @@ struct kvm_vcpu_arch {
> int last_exit_cpu;
>
> /* ISA feature bits (similar to MISA) */
> - unsigned long isa;
> + DECLARE_BITMAP(isa, RISCV_ISA_EXT_MAX);
>
> /* SSCRATCH, STVEC, and SCOUNTEREN of Host */
> unsigned long host_sscratch;
> diff --git a/arch/riscv/include/asm/kvm_vcpu_fp.h b/arch/riscv/include/asm/kvm_vcpu_fp.h
> index 4da9b8e0f050..e86bb67f2a8a 100644
> --- a/arch/riscv/include/asm/kvm_vcpu_fp.h
> +++ b/arch/riscv/include/asm/kvm_vcpu_fp.h
> @@ -22,9 +22,9 @@ void __kvm_riscv_fp_d_restore(struct kvm_cpu_context *context);
>
> void kvm_riscv_vcpu_fp_reset(struct kvm_vcpu *vcpu);
> void kvm_riscv_vcpu_guest_fp_save(struct kvm_cpu_context *cntx,
> - unsigned long isa);
> + unsigned long *isa);
Better to use "const unsigned long *"
> void kvm_riscv_vcpu_guest_fp_restore(struct kvm_cpu_context *cntx,
> - unsigned long isa);
> + unsigned long *isa);
Same as above.
> void kvm_riscv_vcpu_host_fp_save(struct kvm_cpu_context *cntx);
> void kvm_riscv_vcpu_host_fp_restore(struct kvm_cpu_context *cntx);
> #else
> @@ -32,12 +32,12 @@ static inline void kvm_riscv_vcpu_fp_reset(struct kvm_vcpu *vcpu)
> {
> }
> static inline void kvm_riscv_vcpu_guest_fp_save(struct kvm_cpu_context *cntx,
> - unsigned long isa)
> + unsigned long *isa)
> {
> }
> static inline void kvm_riscv_vcpu_guest_fp_restore(
> struct kvm_cpu_context *cntx,
> - unsigned long isa)
> + unsigned long *isa)
> {
> }
> static inline void kvm_riscv_vcpu_host_fp_save(struct kvm_cpu_context *cntx)
> diff --git a/arch/riscv/kvm/vcpu.c b/arch/riscv/kvm/vcpu.c
> index 7f4ad5e4373a..cb2a65b5d563 100644
> --- a/arch/riscv/kvm/vcpu.c
> +++ b/arch/riscv/kvm/vcpu.c
> @@ -46,8 +46,19 @@ const struct kvm_stats_header kvm_vcpu_stats_header = {
> riscv_isa_extension_mask(i) | \
> riscv_isa_extension_mask(m))
>
> -#define KVM_RISCV_ISA_ALLOWED (KVM_RISCV_ISA_DISABLE_ALLOWED | \
> - KVM_RISCV_ISA_DISABLE_NOT_ALLOWED)
> +#define KVM_RISCV_ISA_MASK GENMASK(25, 0)
> +
> +/* Mapping between KVM ISA Extension ID & Host ISA extension ID */
> +static unsigned long kvm_isa_ext_arr[] = {
> + RISCV_ISA_EXT_a,
> + RISCV_ISA_EXT_c,
> + RISCV_ISA_EXT_d,
> + RISCV_ISA_EXT_f,
> + RISCV_ISA_EXT_h,
> + RISCV_ISA_EXT_i,
> + RISCV_ISA_EXT_m,
> + RISCV_ISA_EXT_SSCOFPMF,
The RISCV_ISA_EXT_SSCOFPMF should be added only after we have
SBI PMU support in KVM RISC-V. Please drop it.
> +};
>
> static void kvm_riscv_reset_vcpu(struct kvm_vcpu *vcpu)
> {
> @@ -99,13 +110,20 @@ int kvm_arch_vcpu_create(struct kvm_vcpu *vcpu)
> {
> struct kvm_cpu_context *cntx;
> struct kvm_vcpu_csr *reset_csr = &vcpu->arch.guest_reset_csr;
> + unsigned long host_isa, i;
>
> /* Mark this VCPU never ran */
> vcpu->arch.ran_atleast_once = false;
> vcpu->arch.mmu_page_cache.gfp_zero = __GFP_ZERO;
> + bitmap_zero(vcpu->arch.isa, RISCV_ISA_EXT_MAX);
>
> /* Setup ISA features available to VCPU */
> - vcpu->arch.isa = riscv_isa_extension_base(NULL) & KVM_RISCV_ISA_ALLOWED;
> + for (i = 0; i < ARRAY_SIZE(kvm_isa_ext_arr); i++) {
> + host_isa = kvm_isa_ext_arr[i];
> + if (__riscv_isa_extension_available(NULL, host_isa) &&
> + host_isa != RISCV_ISA_EXT_h)
> + set_bit(host_isa, vcpu->arch.isa);
> + }
>
> /* Setup VCPU hfence queue */
> spin_lock_init(&vcpu->arch.hfence_lock);
> @@ -199,7 +217,7 @@ static int kvm_riscv_vcpu_get_reg_config(struct kvm_vcpu *vcpu,
>
> switch (reg_num) {
> case KVM_REG_RISCV_CONFIG_REG(isa):
> - reg_val = vcpu->arch.isa;
> + reg_val = vcpu->arch.isa[0] & KVM_RISCV_ISA_MASK;
> break;
> default:
> return -EINVAL;
> @@ -220,6 +238,7 @@ static int kvm_riscv_vcpu_set_reg_config(struct kvm_vcpu *vcpu,
> KVM_REG_SIZE_MASK |
> KVM_REG_RISCV_CONFIG);
> unsigned long reg_val;
> + unsigned long isa_mask;
>
> if (KVM_REG_SIZE(reg->id) != sizeof(unsigned long))
> return -EINVAL;
> @@ -227,13 +246,19 @@ static int kvm_riscv_vcpu_set_reg_config(struct kvm_vcpu *vcpu,
> if (copy_from_user(®_val, uaddr, KVM_REG_SIZE(reg->id)))
> return -EFAULT;
>
> + /* This ONE REG interface is only defined for single letter extensions */
> + if (fls(reg_val) >= RISCV_ISA_EXT_BASE)
> + return -EINVAL;
> +
> switch (reg_num) {
> case KVM_REG_RISCV_CONFIG_REG(isa):
> if (!vcpu->arch.ran_atleast_once) {
> /* Ignore the disable request for these extensions */
> - vcpu->arch.isa = reg_val | KVM_RISCV_ISA_DISABLE_NOT_ALLOWED;
> - vcpu->arch.isa &= riscv_isa_extension_base(NULL);
> - vcpu->arch.isa &= KVM_RISCV_ISA_ALLOWED;
> + isa_mask = (reg_val | KVM_RISCV_ISA_DISABLE_NOT_ALLOWED);
> + isa_mask &= riscv_isa_extension_base(NULL);
> + /* Do not modify anything beyond single letter extensions */
> + isa_mask |= (~KVM_RISCV_ISA_MASK);
> + bitmap_and(vcpu->arch.isa, vcpu->arch.isa, &isa_mask, RISCV_ISA_EXT_MAX);
A little more readable version of above sequence can be:
/* Ignore the disable request for these extensions */
reg_val |= KVM_RISCV_ISA_DISABLE_NOT_ALLOWED;
reg_val &= riscv_isa_extension_base(NULL);
/* Do not modify anything beyond single letter extensions */
reg_val = (vcpu->arch.isa[0] & ~KVM_RISCV_ISA_MASK) |
(reg_val & KVM_RISCV_ISA_MASK);
vcpu->arch.isa[0] = reg_val;
kvm_riscv_vcpu_fp_reset(vcpu);
> kvm_riscv_vcpu_fp_reset(vcpu);
> } else {
> return -EOPNOTSUPP;
> @@ -374,17 +399,6 @@ static int kvm_riscv_vcpu_set_reg_csr(struct kvm_vcpu *vcpu,
> return 0;
> }
>
> -/* Mapping between KVM ISA Extension ID & Host ISA extension ID */
> -static unsigned long kvm_isa_ext_arr[] = {
> - RISCV_ISA_EXT_a,
> - RISCV_ISA_EXT_c,
> - RISCV_ISA_EXT_d,
> - RISCV_ISA_EXT_f,
> - RISCV_ISA_EXT_h,
> - RISCV_ISA_EXT_i,
> - RISCV_ISA_EXT_m,
> -};
> -
> static int kvm_riscv_vcpu_get_reg_isa_ext(struct kvm_vcpu *vcpu,
> const struct kvm_one_reg *reg)
> {
> @@ -403,7 +417,7 @@ static int kvm_riscv_vcpu_get_reg_isa_ext(struct kvm_vcpu *vcpu,
> return -EINVAL;
>
> host_isa_ext = kvm_isa_ext_arr[reg_num];
> - if (__riscv_isa_extension_available(&vcpu->arch.isa, host_isa_ext))
> + if (__riscv_isa_extension_available(vcpu->arch.isa, host_isa_ext))
> reg_val = 1; /* Mark the given extension as available */
>
> if (copy_to_user(uaddr, ®_val, KVM_REG_SIZE(reg->id)))
> @@ -437,30 +451,17 @@ static int kvm_riscv_vcpu_set_reg_isa_ext(struct kvm_vcpu *vcpu,
> if (!__riscv_isa_extension_available(NULL, host_isa_ext))
> return -EOPNOTSUPP;
>
> - if (host_isa_ext >= RISCV_ISA_EXT_BASE &&
> - host_isa_ext < RISCV_ISA_EXT_MAX) {
> - /*
> - * Multi-letter ISA extension. Currently there is no provision
> - * to enable/disable the multi-letter ISA extensions for guests.
> - * Return success if the request is to enable any ISA extension
> - * that is available in the hardware.
> - * Return -EOPNOTSUPP otherwise.
> - */
> - if (!reg_val)
> - return -EOPNOTSUPP;
> - else
> - return 0;
> - }
> -
> - /* Single letter base ISA extension */
> if (!vcpu->arch.ran_atleast_once) {
> + /* All multi-letter extension and a few single letter extension can be disabled */
> host_isa_ext_mask = BIT_MASK(host_isa_ext);
> - if (!reg_val && (host_isa_ext_mask & KVM_RISCV_ISA_DISABLE_ALLOWED))
> - vcpu->arch.isa &= ~host_isa_ext_mask;
> + if (!reg_val &&
> + ((host_isa_ext_mask & KVM_RISCV_ISA_DISABLE_ALLOWED) ||
> + ((host_isa_ext >= RISCV_ISA_EXT_BASE) && (host_isa_ext < RISCV_ISA_EXT_MAX))))
> + clear_bit(host_isa_ext, vcpu->arch.isa);
> + else if (reg_val == 1 && (host_isa_ext != RISCV_ISA_EXT_h))
> + set_bit(host_isa_ext, vcpu->arch.isa);
> else
> - vcpu->arch.isa |= host_isa_ext_mask;
> - vcpu->arch.isa &= riscv_isa_extension_base(NULL);
> - vcpu->arch.isa &= KVM_RISCV_ISA_ALLOWED;
> + return -EINVAL;
A slightly more readable version of above sequence can be:
/* All multi-letter extension and a few single letter
extension can be disabled */
if (host_isa_ext >= RISCV_ISA_EXT_MAX)
return -EINVAL;
disable_allow_mask = KVM_RISCV_ISA_DISABLE_ALLOWED;
if (reg_val == 1)
set_bit(host_isa_ext, vcpu->arch.isa);
else if (!reg_val && test_bit(host_isa_ext, &disable_allow_mask))
clear_bit(host_isa_ext, vcpu->arch.isa);
else
return -EINVAL;
> kvm_riscv_vcpu_fp_reset(vcpu);
> } else {
> return -EOPNOTSUPP;
> diff --git a/arch/riscv/kvm/vcpu_fp.c b/arch/riscv/kvm/vcpu_fp.c
> index d4308c512007..748a8f6a9b5d 100644
> --- a/arch/riscv/kvm/vcpu_fp.c
> +++ b/arch/riscv/kvm/vcpu_fp.c
> @@ -16,12 +16,11 @@
> #ifdef CONFIG_FPU
> void kvm_riscv_vcpu_fp_reset(struct kvm_vcpu *vcpu)
> {
> - unsigned long isa = vcpu->arch.isa;
> struct kvm_cpu_context *cntx = &vcpu->arch.guest_context;
>
> cntx->sstatus &= ~SR_FS;
> - if (riscv_isa_extension_available(&isa, f) ||
> - riscv_isa_extension_available(&isa, d))
> + if (riscv_isa_extension_available(vcpu->arch.isa, f) ||
> + riscv_isa_extension_available(vcpu->arch.isa, d))
> cntx->sstatus |= SR_FS_INITIAL;
> else
> cntx->sstatus |= SR_FS_OFF;
> @@ -34,24 +33,24 @@ static void kvm_riscv_vcpu_fp_clean(struct kvm_cpu_context *cntx)
> }
>
> void kvm_riscv_vcpu_guest_fp_save(struct kvm_cpu_context *cntx,
> - unsigned long isa)
> + unsigned long *isa)
> {
> if ((cntx->sstatus & SR_FS) == SR_FS_DIRTY) {
> - if (riscv_isa_extension_available(&isa, d))
> + if (riscv_isa_extension_available(isa, d))
> __kvm_riscv_fp_d_save(cntx);
> - else if (riscv_isa_extension_available(&isa, f))
> + else if (riscv_isa_extension_available(isa, f))
> __kvm_riscv_fp_f_save(cntx);
> kvm_riscv_vcpu_fp_clean(cntx);
> }
> }
>
> void kvm_riscv_vcpu_guest_fp_restore(struct kvm_cpu_context *cntx,
> - unsigned long isa)
> + unsigned long *isa)
> {
> if ((cntx->sstatus & SR_FS) != SR_FS_OFF) {
> - if (riscv_isa_extension_available(&isa, d))
> + if (riscv_isa_extension_available(isa, d))
> __kvm_riscv_fp_d_restore(cntx);
> - else if (riscv_isa_extension_available(&isa, f))
> + else if (riscv_isa_extension_available(isa, f))
> __kvm_riscv_fp_f_restore(cntx);
> kvm_riscv_vcpu_fp_clean(cntx);
> }
> @@ -80,7 +79,6 @@ int kvm_riscv_vcpu_get_reg_fp(struct kvm_vcpu *vcpu,
> unsigned long rtype)
> {
> struct kvm_cpu_context *cntx = &vcpu->arch.guest_context;
> - unsigned long isa = vcpu->arch.isa;
> unsigned long __user *uaddr =
> (unsigned long __user *)(unsigned long)reg->addr;
> unsigned long reg_num = reg->id & ~(KVM_REG_ARCH_MASK |
> @@ -89,7 +87,7 @@ int kvm_riscv_vcpu_get_reg_fp(struct kvm_vcpu *vcpu,
> void *reg_val;
>
> if ((rtype == KVM_REG_RISCV_FP_F) &&
> - riscv_isa_extension_available(&isa, f)) {
> + riscv_isa_extension_available(vcpu->arch.isa, f)) {
> if (KVM_REG_SIZE(reg->id) != sizeof(u32))
> return -EINVAL;
> if (reg_num == KVM_REG_RISCV_FP_F_REG(fcsr))
> @@ -100,7 +98,7 @@ int kvm_riscv_vcpu_get_reg_fp(struct kvm_vcpu *vcpu,
> else
> return -EINVAL;
> } else if ((rtype == KVM_REG_RISCV_FP_D) &&
> - riscv_isa_extension_available(&isa, d)) {
> + riscv_isa_extension_available(vcpu->arch.isa, d)) {
> if (reg_num == KVM_REG_RISCV_FP_D_REG(fcsr)) {
> if (KVM_REG_SIZE(reg->id) != sizeof(u32))
> return -EINVAL;
> @@ -126,7 +124,6 @@ int kvm_riscv_vcpu_set_reg_fp(struct kvm_vcpu *vcpu,
> unsigned long rtype)
> {
> struct kvm_cpu_context *cntx = &vcpu->arch.guest_context;
> - unsigned long isa = vcpu->arch.isa;
> unsigned long __user *uaddr =
> (unsigned long __user *)(unsigned long)reg->addr;
> unsigned long reg_num = reg->id & ~(KVM_REG_ARCH_MASK |
> @@ -135,7 +132,7 @@ int kvm_riscv_vcpu_set_reg_fp(struct kvm_vcpu *vcpu,
> void *reg_val;
>
> if ((rtype == KVM_REG_RISCV_FP_F) &&
> - riscv_isa_extension_available(&isa, f)) {
> + riscv_isa_extension_available(vcpu->arch.isa, f)) {
> if (KVM_REG_SIZE(reg->id) != sizeof(u32))
> return -EINVAL;
> if (reg_num == KVM_REG_RISCV_FP_F_REG(fcsr))
> @@ -146,7 +143,7 @@ int kvm_riscv_vcpu_set_reg_fp(struct kvm_vcpu *vcpu,
> else
> return -EINVAL;
> } else if ((rtype == KVM_REG_RISCV_FP_D) &&
> - riscv_isa_extension_available(&isa, d)) {
> + riscv_isa_extension_available(vcpu->arch.isa, d)) {
> if (reg_num == KVM_REG_RISCV_FP_D_REG(fcsr)) {
> if (KVM_REG_SIZE(reg->id) != sizeof(u32))
> return -EINVAL;
> --
> 2.25.1
>
Apart from minor comments above, this looks good to me.
I have taken care of the above comments and queued it for 5.20. I
can certainly modify my queue if you want further changes in this patch.
Thanks,
Anup
On Mon, Jul 4, 2022 at 1:47 AM Anup Patel <[email protected]> wrote:
>
> On Tue, Jun 21, 2022 at 5:13 AM Atish Patra <[email protected]> wrote:
> >
> > Currently, the every vcpu only stores the ISA extensions in a unsigned long
> > which is not scalable as number of extensions will continue to grow.
> > Using a bitmap allows the ISA extension to support any number of
> > extensions. The CONFIG one reg interface implementation is modified to
> > support the bitmap as well. But it is meant only for base extensions.
> > Thus, the first element of the bitmap array is sufficient for that
> > interface.
> >
> > In the future, all the new multi-letter extensions must use the
> > ISA_EXT one reg interface that allows enabling/disabling any extension
> > now.
> >
> > Signed-off-by: Atish Patra <[email protected]>
> > ---
> > arch/riscv/include/asm/kvm_host.h | 3 +-
> > arch/riscv/include/asm/kvm_vcpu_fp.h | 8 +--
> > arch/riscv/kvm/vcpu.c | 81 ++++++++++++++--------------
> > arch/riscv/kvm/vcpu_fp.c | 27 +++++-----
> > 4 files changed, 59 insertions(+), 60 deletions(-)
> >
> > diff --git a/arch/riscv/include/asm/kvm_host.h b/arch/riscv/include/asm/kvm_host.h
> > index 319c8aeb42af..c749cdacbd63 100644
> > --- a/arch/riscv/include/asm/kvm_host.h
> > +++ b/arch/riscv/include/asm/kvm_host.h
> > @@ -14,6 +14,7 @@
> > #include <linux/kvm_types.h>
> > #include <linux/spinlock.h>
> > #include <asm/csr.h>
> > +#include <asm/hwcap.h>
> > #include <asm/kvm_vcpu_fp.h>
> > #include <asm/kvm_vcpu_timer.h>
> >
> > @@ -170,7 +171,7 @@ struct kvm_vcpu_arch {
> > int last_exit_cpu;
> >
> > /* ISA feature bits (similar to MISA) */
> > - unsigned long isa;
> > + DECLARE_BITMAP(isa, RISCV_ISA_EXT_MAX);
> >
> > /* SSCRATCH, STVEC, and SCOUNTEREN of Host */
> > unsigned long host_sscratch;
> > diff --git a/arch/riscv/include/asm/kvm_vcpu_fp.h b/arch/riscv/include/asm/kvm_vcpu_fp.h
> > index 4da9b8e0f050..e86bb67f2a8a 100644
> > --- a/arch/riscv/include/asm/kvm_vcpu_fp.h
> > +++ b/arch/riscv/include/asm/kvm_vcpu_fp.h
> > @@ -22,9 +22,9 @@ void __kvm_riscv_fp_d_restore(struct kvm_cpu_context *context);
> >
> > void kvm_riscv_vcpu_fp_reset(struct kvm_vcpu *vcpu);
> > void kvm_riscv_vcpu_guest_fp_save(struct kvm_cpu_context *cntx,
> > - unsigned long isa);
> > + unsigned long *isa);
>
> Better to use "const unsigned long *"
>
> > void kvm_riscv_vcpu_guest_fp_restore(struct kvm_cpu_context *cntx,
> > - unsigned long isa);
> > + unsigned long *isa);
>
> Same as above.
>
Yes. Thanks.
> > void kvm_riscv_vcpu_host_fp_save(struct kvm_cpu_context *cntx);
> > void kvm_riscv_vcpu_host_fp_restore(struct kvm_cpu_context *cntx);
> > #else
> > @@ -32,12 +32,12 @@ static inline void kvm_riscv_vcpu_fp_reset(struct kvm_vcpu *vcpu)
> > {
> > }
> > static inline void kvm_riscv_vcpu_guest_fp_save(struct kvm_cpu_context *cntx,
> > - unsigned long isa)
> > + unsigned long *isa)
> > {
> > }
> > static inline void kvm_riscv_vcpu_guest_fp_restore(
> > struct kvm_cpu_context *cntx,
> > - unsigned long isa)
> > + unsigned long *isa)
> > {
> > }
> > static inline void kvm_riscv_vcpu_host_fp_save(struct kvm_cpu_context *cntx)
> > diff --git a/arch/riscv/kvm/vcpu.c b/arch/riscv/kvm/vcpu.c
> > index 7f4ad5e4373a..cb2a65b5d563 100644
> > --- a/arch/riscv/kvm/vcpu.c
> > +++ b/arch/riscv/kvm/vcpu.c
> > @@ -46,8 +46,19 @@ const struct kvm_stats_header kvm_vcpu_stats_header = {
> > riscv_isa_extension_mask(i) | \
> > riscv_isa_extension_mask(m))
> >
> > -#define KVM_RISCV_ISA_ALLOWED (KVM_RISCV_ISA_DISABLE_ALLOWED | \
> > - KVM_RISCV_ISA_DISABLE_NOT_ALLOWED)
> > +#define KVM_RISCV_ISA_MASK GENMASK(25, 0)
> > +
> > +/* Mapping between KVM ISA Extension ID & Host ISA extension ID */
> > +static unsigned long kvm_isa_ext_arr[] = {
> > + RISCV_ISA_EXT_a,
> > + RISCV_ISA_EXT_c,
> > + RISCV_ISA_EXT_d,
> > + RISCV_ISA_EXT_f,
> > + RISCV_ISA_EXT_h,
> > + RISCV_ISA_EXT_i,
> > + RISCV_ISA_EXT_m,
> > + RISCV_ISA_EXT_SSCOFPMF,
>
> The RISCV_ISA_EXT_SSCOFPMF should be added only after we have
> SBI PMU support in KVM RISC-V. Please drop it.
>
Ahh. Sorry. I forgot to remove it while rebasing.
> > +};
> >
> > static void kvm_riscv_reset_vcpu(struct kvm_vcpu *vcpu)
> > {
> > @@ -99,13 +110,20 @@ int kvm_arch_vcpu_create(struct kvm_vcpu *vcpu)
> > {
> > struct kvm_cpu_context *cntx;
> > struct kvm_vcpu_csr *reset_csr = &vcpu->arch.guest_reset_csr;
> > + unsigned long host_isa, i;
> >
> > /* Mark this VCPU never ran */
> > vcpu->arch.ran_atleast_once = false;
> > vcpu->arch.mmu_page_cache.gfp_zero = __GFP_ZERO;
> > + bitmap_zero(vcpu->arch.isa, RISCV_ISA_EXT_MAX);
> >
> > /* Setup ISA features available to VCPU */
> > - vcpu->arch.isa = riscv_isa_extension_base(NULL) & KVM_RISCV_ISA_ALLOWED;
> > + for (i = 0; i < ARRAY_SIZE(kvm_isa_ext_arr); i++) {
> > + host_isa = kvm_isa_ext_arr[i];
> > + if (__riscv_isa_extension_available(NULL, host_isa) &&
> > + host_isa != RISCV_ISA_EXT_h)
> > + set_bit(host_isa, vcpu->arch.isa);
> > + }
> >
> > /* Setup VCPU hfence queue */
> > spin_lock_init(&vcpu->arch.hfence_lock);
> > @@ -199,7 +217,7 @@ static int kvm_riscv_vcpu_get_reg_config(struct kvm_vcpu *vcpu,
> >
> > switch (reg_num) {
> > case KVM_REG_RISCV_CONFIG_REG(isa):
> > - reg_val = vcpu->arch.isa;
> > + reg_val = vcpu->arch.isa[0] & KVM_RISCV_ISA_MASK;
> > break;
> > default:
> > return -EINVAL;
> > @@ -220,6 +238,7 @@ static int kvm_riscv_vcpu_set_reg_config(struct kvm_vcpu *vcpu,
> > KVM_REG_SIZE_MASK |
> > KVM_REG_RISCV_CONFIG);
> > unsigned long reg_val;
> > + unsigned long isa_mask;
> >
> > if (KVM_REG_SIZE(reg->id) != sizeof(unsigned long))
> > return -EINVAL;
> > @@ -227,13 +246,19 @@ static int kvm_riscv_vcpu_set_reg_config(struct kvm_vcpu *vcpu,
> > if (copy_from_user(®_val, uaddr, KVM_REG_SIZE(reg->id)))
> > return -EFAULT;
> >
> > + /* This ONE REG interface is only defined for single letter extensions */
> > + if (fls(reg_val) >= RISCV_ISA_EXT_BASE)
> > + return -EINVAL;
> > +
> > switch (reg_num) {
> > case KVM_REG_RISCV_CONFIG_REG(isa):
> > if (!vcpu->arch.ran_atleast_once) {
> > /* Ignore the disable request for these extensions */
> > - vcpu->arch.isa = reg_val | KVM_RISCV_ISA_DISABLE_NOT_ALLOWED;
> > - vcpu->arch.isa &= riscv_isa_extension_base(NULL);
> > - vcpu->arch.isa &= KVM_RISCV_ISA_ALLOWED;
> > + isa_mask = (reg_val | KVM_RISCV_ISA_DISABLE_NOT_ALLOWED);
> > + isa_mask &= riscv_isa_extension_base(NULL);
> > + /* Do not modify anything beyond single letter extensions */
> > + isa_mask |= (~KVM_RISCV_ISA_MASK);
> > + bitmap_and(vcpu->arch.isa, vcpu->arch.isa, &isa_mask, RISCV_ISA_EXT_MAX);
>
> A little more readable version of above sequence can be:
>
> /* Ignore the disable request for these extensions */
> reg_val |= KVM_RISCV_ISA_DISABLE_NOT_ALLOWED;
> reg_val &= riscv_isa_extension_base(NULL);
> /* Do not modify anything beyond single letter extensions */
> reg_val = (vcpu->arch.isa[0] & ~KVM_RISCV_ISA_MASK) |
> (reg_val & KVM_RISCV_ISA_MASK);
> vcpu->arch.isa[0] = reg_val;
> kvm_riscv_vcpu_fp_reset(vcpu);
>
>
> > kvm_riscv_vcpu_fp_reset(vcpu);
> > } else {
> > return -EOPNOTSUPP;
> > @@ -374,17 +399,6 @@ static int kvm_riscv_vcpu_set_reg_csr(struct kvm_vcpu *vcpu,
> > return 0;
> > }
> >
> > -/* Mapping between KVM ISA Extension ID & Host ISA extension ID */
> > -static unsigned long kvm_isa_ext_arr[] = {
> > - RISCV_ISA_EXT_a,
> > - RISCV_ISA_EXT_c,
> > - RISCV_ISA_EXT_d,
> > - RISCV_ISA_EXT_f,
> > - RISCV_ISA_EXT_h,
> > - RISCV_ISA_EXT_i,
> > - RISCV_ISA_EXT_m,
> > -};
> > -
> > static int kvm_riscv_vcpu_get_reg_isa_ext(struct kvm_vcpu *vcpu,
> > const struct kvm_one_reg *reg)
> > {
> > @@ -403,7 +417,7 @@ static int kvm_riscv_vcpu_get_reg_isa_ext(struct kvm_vcpu *vcpu,
> > return -EINVAL;
> >
> > host_isa_ext = kvm_isa_ext_arr[reg_num];
> > - if (__riscv_isa_extension_available(&vcpu->arch.isa, host_isa_ext))
> > + if (__riscv_isa_extension_available(vcpu->arch.isa, host_isa_ext))
> > reg_val = 1; /* Mark the given extension as available */
> >
> > if (copy_to_user(uaddr, ®_val, KVM_REG_SIZE(reg->id)))
> > @@ -437,30 +451,17 @@ static int kvm_riscv_vcpu_set_reg_isa_ext(struct kvm_vcpu *vcpu,
> > if (!__riscv_isa_extension_available(NULL, host_isa_ext))
> > return -EOPNOTSUPP;
> >
> > - if (host_isa_ext >= RISCV_ISA_EXT_BASE &&
> > - host_isa_ext < RISCV_ISA_EXT_MAX) {
> > - /*
> > - * Multi-letter ISA extension. Currently there is no provision
> > - * to enable/disable the multi-letter ISA extensions for guests.
> > - * Return success if the request is to enable any ISA extension
> > - * that is available in the hardware.
> > - * Return -EOPNOTSUPP otherwise.
> > - */
> > - if (!reg_val)
> > - return -EOPNOTSUPP;
> > - else
> > - return 0;
> > - }
> > -
> > - /* Single letter base ISA extension */
> > if (!vcpu->arch.ran_atleast_once) {
> > + /* All multi-letter extension and a few single letter extension can be disabled */
> > host_isa_ext_mask = BIT_MASK(host_isa_ext);
> > - if (!reg_val && (host_isa_ext_mask & KVM_RISCV_ISA_DISABLE_ALLOWED))
> > - vcpu->arch.isa &= ~host_isa_ext_mask;
> > + if (!reg_val &&
> > + ((host_isa_ext_mask & KVM_RISCV_ISA_DISABLE_ALLOWED) ||
> > + ((host_isa_ext >= RISCV_ISA_EXT_BASE) && (host_isa_ext < RISCV_ISA_EXT_MAX))))
> > + clear_bit(host_isa_ext, vcpu->arch.isa);
> > + else if (reg_val == 1 && (host_isa_ext != RISCV_ISA_EXT_h))
> > + set_bit(host_isa_ext, vcpu->arch.isa);
> > else
> > - vcpu->arch.isa |= host_isa_ext_mask;
> > - vcpu->arch.isa &= riscv_isa_extension_base(NULL);
> > - vcpu->arch.isa &= KVM_RISCV_ISA_ALLOWED;
> > + return -EINVAL;
>
> A slightly more readable version of above sequence can be:
>
> /* All multi-letter extension and a few single letter
> extension can be disabled */
> if (host_isa_ext >= RISCV_ISA_EXT_MAX)
> return -EINVAL;
> disable_allow_mask = KVM_RISCV_ISA_DISABLE_ALLOWED;
> if (reg_val == 1)
> set_bit(host_isa_ext, vcpu->arch.isa);
Shouldn't we ensure that (host_isa_ext != RISCV_ISA_EXT_h) here ?
> else if (!reg_val && test_bit(host_isa_ext, &disable_allow_mask))
> clear_bit(host_isa_ext, vcpu->arch.isa);
> else
> return -EINVAL;
>
>
> > kvm_riscv_vcpu_fp_reset(vcpu);
> > } else {
> > return -EOPNOTSUPP;
> > diff --git a/arch/riscv/kvm/vcpu_fp.c b/arch/riscv/kvm/vcpu_fp.c
> > index d4308c512007..748a8f6a9b5d 100644
> > --- a/arch/riscv/kvm/vcpu_fp.c
> > +++ b/arch/riscv/kvm/vcpu_fp.c
> > @@ -16,12 +16,11 @@
> > #ifdef CONFIG_FPU
> > void kvm_riscv_vcpu_fp_reset(struct kvm_vcpu *vcpu)
> > {
> > - unsigned long isa = vcpu->arch.isa;
> > struct kvm_cpu_context *cntx = &vcpu->arch.guest_context;
> >
> > cntx->sstatus &= ~SR_FS;
> > - if (riscv_isa_extension_available(&isa, f) ||
> > - riscv_isa_extension_available(&isa, d))
> > + if (riscv_isa_extension_available(vcpu->arch.isa, f) ||
> > + riscv_isa_extension_available(vcpu->arch.isa, d))
> > cntx->sstatus |= SR_FS_INITIAL;
> > else
> > cntx->sstatus |= SR_FS_OFF;
> > @@ -34,24 +33,24 @@ static void kvm_riscv_vcpu_fp_clean(struct kvm_cpu_context *cntx)
> > }
> >
> > void kvm_riscv_vcpu_guest_fp_save(struct kvm_cpu_context *cntx,
> > - unsigned long isa)
> > + unsigned long *isa)
> > {
> > if ((cntx->sstatus & SR_FS) == SR_FS_DIRTY) {
> > - if (riscv_isa_extension_available(&isa, d))
> > + if (riscv_isa_extension_available(isa, d))
> > __kvm_riscv_fp_d_save(cntx);
> > - else if (riscv_isa_extension_available(&isa, f))
> > + else if (riscv_isa_extension_available(isa, f))
> > __kvm_riscv_fp_f_save(cntx);
> > kvm_riscv_vcpu_fp_clean(cntx);
> > }
> > }
> >
> > void kvm_riscv_vcpu_guest_fp_restore(struct kvm_cpu_context *cntx,
> > - unsigned long isa)
> > + unsigned long *isa)
> > {
> > if ((cntx->sstatus & SR_FS) != SR_FS_OFF) {
> > - if (riscv_isa_extension_available(&isa, d))
> > + if (riscv_isa_extension_available(isa, d))
> > __kvm_riscv_fp_d_restore(cntx);
> > - else if (riscv_isa_extension_available(&isa, f))
> > + else if (riscv_isa_extension_available(isa, f))
> > __kvm_riscv_fp_f_restore(cntx);
> > kvm_riscv_vcpu_fp_clean(cntx);
> > }
> > @@ -80,7 +79,6 @@ int kvm_riscv_vcpu_get_reg_fp(struct kvm_vcpu *vcpu,
> > unsigned long rtype)
> > {
> > struct kvm_cpu_context *cntx = &vcpu->arch.guest_context;
> > - unsigned long isa = vcpu->arch.isa;
> > unsigned long __user *uaddr =
> > (unsigned long __user *)(unsigned long)reg->addr;
> > unsigned long reg_num = reg->id & ~(KVM_REG_ARCH_MASK |
> > @@ -89,7 +87,7 @@ int kvm_riscv_vcpu_get_reg_fp(struct kvm_vcpu *vcpu,
> > void *reg_val;
> >
> > if ((rtype == KVM_REG_RISCV_FP_F) &&
> > - riscv_isa_extension_available(&isa, f)) {
> > + riscv_isa_extension_available(vcpu->arch.isa, f)) {
> > if (KVM_REG_SIZE(reg->id) != sizeof(u32))
> > return -EINVAL;
> > if (reg_num == KVM_REG_RISCV_FP_F_REG(fcsr))
> > @@ -100,7 +98,7 @@ int kvm_riscv_vcpu_get_reg_fp(struct kvm_vcpu *vcpu,
> > else
> > return -EINVAL;
> > } else if ((rtype == KVM_REG_RISCV_FP_D) &&
> > - riscv_isa_extension_available(&isa, d)) {
> > + riscv_isa_extension_available(vcpu->arch.isa, d)) {
> > if (reg_num == KVM_REG_RISCV_FP_D_REG(fcsr)) {
> > if (KVM_REG_SIZE(reg->id) != sizeof(u32))
> > return -EINVAL;
> > @@ -126,7 +124,6 @@ int kvm_riscv_vcpu_set_reg_fp(struct kvm_vcpu *vcpu,
> > unsigned long rtype)
> > {
> > struct kvm_cpu_context *cntx = &vcpu->arch.guest_context;
> > - unsigned long isa = vcpu->arch.isa;
> > unsigned long __user *uaddr =
> > (unsigned long __user *)(unsigned long)reg->addr;
> > unsigned long reg_num = reg->id & ~(KVM_REG_ARCH_MASK |
> > @@ -135,7 +132,7 @@ int kvm_riscv_vcpu_set_reg_fp(struct kvm_vcpu *vcpu,
> > void *reg_val;
> >
> > if ((rtype == KVM_REG_RISCV_FP_F) &&
> > - riscv_isa_extension_available(&isa, f)) {
> > + riscv_isa_extension_available(vcpu->arch.isa, f)) {
> > if (KVM_REG_SIZE(reg->id) != sizeof(u32))
> > return -EINVAL;
> > if (reg_num == KVM_REG_RISCV_FP_F_REG(fcsr))
> > @@ -146,7 +143,7 @@ int kvm_riscv_vcpu_set_reg_fp(struct kvm_vcpu *vcpu,
> > else
> > return -EINVAL;
> > } else if ((rtype == KVM_REG_RISCV_FP_D) &&
> > - riscv_isa_extension_available(&isa, d)) {
> > + riscv_isa_extension_available(vcpu->arch.isa, d)) {
> > if (reg_num == KVM_REG_RISCV_FP_D_REG(fcsr)) {
> > if (KVM_REG_SIZE(reg->id) != sizeof(u32))
> > return -EINVAL;
> > --
> > 2.25.1
> >
>
> Apart from minor comments above, this looks good to me.
>
Thanks.
> I have taken care of the above comments and queued it for 5.20. I
> can certainly modify my queue if you want further changes in this patch.
>
> Thanks,
> Anup
On Tue, Jul 5, 2022 at 12:53 PM Atish Kumar Patra <[email protected]> wrote:
>
> On Mon, Jul 4, 2022 at 1:47 AM Anup Patel <[email protected]> wrote:
> >
> > On Tue, Jun 21, 2022 at 5:13 AM Atish Patra <[email protected]> wrote:
> > >
> > > Currently, the every vcpu only stores the ISA extensions in a unsigned long
> > > which is not scalable as number of extensions will continue to grow.
> > > Using a bitmap allows the ISA extension to support any number of
> > > extensions. The CONFIG one reg interface implementation is modified to
> > > support the bitmap as well. But it is meant only for base extensions.
> > > Thus, the first element of the bitmap array is sufficient for that
> > > interface.
> > >
> > > In the future, all the new multi-letter extensions must use the
> > > ISA_EXT one reg interface that allows enabling/disabling any extension
> > > now.
> > >
> > > Signed-off-by: Atish Patra <[email protected]>
> > > ---
> > > arch/riscv/include/asm/kvm_host.h | 3 +-
> > > arch/riscv/include/asm/kvm_vcpu_fp.h | 8 +--
> > > arch/riscv/kvm/vcpu.c | 81 ++++++++++++++--------------
> > > arch/riscv/kvm/vcpu_fp.c | 27 +++++-----
> > > 4 files changed, 59 insertions(+), 60 deletions(-)
> > >
> > > diff --git a/arch/riscv/include/asm/kvm_host.h b/arch/riscv/include/asm/kvm_host.h
> > > index 319c8aeb42af..c749cdacbd63 100644
> > > --- a/arch/riscv/include/asm/kvm_host.h
> > > +++ b/arch/riscv/include/asm/kvm_host.h
> > > @@ -14,6 +14,7 @@
> > > #include <linux/kvm_types.h>
> > > #include <linux/spinlock.h>
> > > #include <asm/csr.h>
> > > +#include <asm/hwcap.h>
> > > #include <asm/kvm_vcpu_fp.h>
> > > #include <asm/kvm_vcpu_timer.h>
> > >
> > > @@ -170,7 +171,7 @@ struct kvm_vcpu_arch {
> > > int last_exit_cpu;
> > >
> > > /* ISA feature bits (similar to MISA) */
> > > - unsigned long isa;
> > > + DECLARE_BITMAP(isa, RISCV_ISA_EXT_MAX);
> > >
> > > /* SSCRATCH, STVEC, and SCOUNTEREN of Host */
> > > unsigned long host_sscratch;
> > > diff --git a/arch/riscv/include/asm/kvm_vcpu_fp.h b/arch/riscv/include/asm/kvm_vcpu_fp.h
> > > index 4da9b8e0f050..e86bb67f2a8a 100644
> > > --- a/arch/riscv/include/asm/kvm_vcpu_fp.h
> > > +++ b/arch/riscv/include/asm/kvm_vcpu_fp.h
> > > @@ -22,9 +22,9 @@ void __kvm_riscv_fp_d_restore(struct kvm_cpu_context *context);
> > >
> > > void kvm_riscv_vcpu_fp_reset(struct kvm_vcpu *vcpu);
> > > void kvm_riscv_vcpu_guest_fp_save(struct kvm_cpu_context *cntx,
> > > - unsigned long isa);
> > > + unsigned long *isa);
> >
> > Better to use "const unsigned long *"
> >
> > > void kvm_riscv_vcpu_guest_fp_restore(struct kvm_cpu_context *cntx,
> > > - unsigned long isa);
> > > + unsigned long *isa);
> >
> > Same as above.
> >
>
> Yes. Thanks.
>
> > > void kvm_riscv_vcpu_host_fp_save(struct kvm_cpu_context *cntx);
> > > void kvm_riscv_vcpu_host_fp_restore(struct kvm_cpu_context *cntx);
> > > #else
> > > @@ -32,12 +32,12 @@ static inline void kvm_riscv_vcpu_fp_reset(struct kvm_vcpu *vcpu)
> > > {
> > > }
> > > static inline void kvm_riscv_vcpu_guest_fp_save(struct kvm_cpu_context *cntx,
> > > - unsigned long isa)
> > > + unsigned long *isa)
> > > {
> > > }
> > > static inline void kvm_riscv_vcpu_guest_fp_restore(
> > > struct kvm_cpu_context *cntx,
> > > - unsigned long isa)
> > > + unsigned long *isa)
> > > {
> > > }
> > > static inline void kvm_riscv_vcpu_host_fp_save(struct kvm_cpu_context *cntx)
> > > diff --git a/arch/riscv/kvm/vcpu.c b/arch/riscv/kvm/vcpu.c
> > > index 7f4ad5e4373a..cb2a65b5d563 100644
> > > --- a/arch/riscv/kvm/vcpu.c
> > > +++ b/arch/riscv/kvm/vcpu.c
> > > @@ -46,8 +46,19 @@ const struct kvm_stats_header kvm_vcpu_stats_header = {
> > > riscv_isa_extension_mask(i) | \
> > > riscv_isa_extension_mask(m))
> > >
> > > -#define KVM_RISCV_ISA_ALLOWED (KVM_RISCV_ISA_DISABLE_ALLOWED | \
> > > - KVM_RISCV_ISA_DISABLE_NOT_ALLOWED)
> > > +#define KVM_RISCV_ISA_MASK GENMASK(25, 0)
> > > +
> > > +/* Mapping between KVM ISA Extension ID & Host ISA extension ID */
> > > +static unsigned long kvm_isa_ext_arr[] = {
> > > + RISCV_ISA_EXT_a,
> > > + RISCV_ISA_EXT_c,
> > > + RISCV_ISA_EXT_d,
> > > + RISCV_ISA_EXT_f,
> > > + RISCV_ISA_EXT_h,
> > > + RISCV_ISA_EXT_i,
> > > + RISCV_ISA_EXT_m,
> > > + RISCV_ISA_EXT_SSCOFPMF,
> >
> > The RISCV_ISA_EXT_SSCOFPMF should be added only after we have
> > SBI PMU support in KVM RISC-V. Please drop it.
> >
>
> Ahh. Sorry. I forgot to remove it while rebasing.
>
> > > +};
> > >
> > > static void kvm_riscv_reset_vcpu(struct kvm_vcpu *vcpu)
> > > {
> > > @@ -99,13 +110,20 @@ int kvm_arch_vcpu_create(struct kvm_vcpu *vcpu)
> > > {
> > > struct kvm_cpu_context *cntx;
> > > struct kvm_vcpu_csr *reset_csr = &vcpu->arch.guest_reset_csr;
> > > + unsigned long host_isa, i;
> > >
> > > /* Mark this VCPU never ran */
> > > vcpu->arch.ran_atleast_once = false;
> > > vcpu->arch.mmu_page_cache.gfp_zero = __GFP_ZERO;
> > > + bitmap_zero(vcpu->arch.isa, RISCV_ISA_EXT_MAX);
> > >
> > > /* Setup ISA features available to VCPU */
> > > - vcpu->arch.isa = riscv_isa_extension_base(NULL) & KVM_RISCV_ISA_ALLOWED;
> > > + for (i = 0; i < ARRAY_SIZE(kvm_isa_ext_arr); i++) {
> > > + host_isa = kvm_isa_ext_arr[i];
> > > + if (__riscv_isa_extension_available(NULL, host_isa) &&
> > > + host_isa != RISCV_ISA_EXT_h)
> > > + set_bit(host_isa, vcpu->arch.isa);
> > > + }
> > >
> > > /* Setup VCPU hfence queue */
> > > spin_lock_init(&vcpu->arch.hfence_lock);
> > > @@ -199,7 +217,7 @@ static int kvm_riscv_vcpu_get_reg_config(struct kvm_vcpu *vcpu,
> > >
> > > switch (reg_num) {
> > > case KVM_REG_RISCV_CONFIG_REG(isa):
> > > - reg_val = vcpu->arch.isa;
> > > + reg_val = vcpu->arch.isa[0] & KVM_RISCV_ISA_MASK;
> > > break;
> > > default:
> > > return -EINVAL;
> > > @@ -220,6 +238,7 @@ static int kvm_riscv_vcpu_set_reg_config(struct kvm_vcpu *vcpu,
> > > KVM_REG_SIZE_MASK |
> > > KVM_REG_RISCV_CONFIG);
> > > unsigned long reg_val;
> > > + unsigned long isa_mask;
> > >
> > > if (KVM_REG_SIZE(reg->id) != sizeof(unsigned long))
> > > return -EINVAL;
> > > @@ -227,13 +246,19 @@ static int kvm_riscv_vcpu_set_reg_config(struct kvm_vcpu *vcpu,
> > > if (copy_from_user(®_val, uaddr, KVM_REG_SIZE(reg->id)))
> > > return -EFAULT;
> > >
> > > + /* This ONE REG interface is only defined for single letter extensions */
> > > + if (fls(reg_val) >= RISCV_ISA_EXT_BASE)
> > > + return -EINVAL;
> > > +
> > > switch (reg_num) {
> > > case KVM_REG_RISCV_CONFIG_REG(isa):
> > > if (!vcpu->arch.ran_atleast_once) {
> > > /* Ignore the disable request for these extensions */
> > > - vcpu->arch.isa = reg_val | KVM_RISCV_ISA_DISABLE_NOT_ALLOWED;
> > > - vcpu->arch.isa &= riscv_isa_extension_base(NULL);
> > > - vcpu->arch.isa &= KVM_RISCV_ISA_ALLOWED;
> > > + isa_mask = (reg_val | KVM_RISCV_ISA_DISABLE_NOT_ALLOWED);
> > > + isa_mask &= riscv_isa_extension_base(NULL);
> > > + /* Do not modify anything beyond single letter extensions */
> > > + isa_mask |= (~KVM_RISCV_ISA_MASK);
> > > + bitmap_and(vcpu->arch.isa, vcpu->arch.isa, &isa_mask, RISCV_ISA_EXT_MAX);
> >
> > A little more readable version of above sequence can be:
> >
> > /* Ignore the disable request for these extensions */
> > reg_val |= KVM_RISCV_ISA_DISABLE_NOT_ALLOWED;
> > reg_val &= riscv_isa_extension_base(NULL);
> > /* Do not modify anything beyond single letter extensions */
> > reg_val = (vcpu->arch.isa[0] & ~KVM_RISCV_ISA_MASK) |
> > (reg_val & KVM_RISCV_ISA_MASK);
> > vcpu->arch.isa[0] = reg_val;
> > kvm_riscv_vcpu_fp_reset(vcpu);
> >
> >
> > > kvm_riscv_vcpu_fp_reset(vcpu);
> > > } else {
> > > return -EOPNOTSUPP;
> > > @@ -374,17 +399,6 @@ static int kvm_riscv_vcpu_set_reg_csr(struct kvm_vcpu *vcpu,
> > > return 0;
> > > }
> > >
> > > -/* Mapping between KVM ISA Extension ID & Host ISA extension ID */
> > > -static unsigned long kvm_isa_ext_arr[] = {
> > > - RISCV_ISA_EXT_a,
> > > - RISCV_ISA_EXT_c,
> > > - RISCV_ISA_EXT_d,
> > > - RISCV_ISA_EXT_f,
> > > - RISCV_ISA_EXT_h,
> > > - RISCV_ISA_EXT_i,
> > > - RISCV_ISA_EXT_m,
> > > -};
> > > -
> > > static int kvm_riscv_vcpu_get_reg_isa_ext(struct kvm_vcpu *vcpu,
> > > const struct kvm_one_reg *reg)
> > > {
> > > @@ -403,7 +417,7 @@ static int kvm_riscv_vcpu_get_reg_isa_ext(struct kvm_vcpu *vcpu,
> > > return -EINVAL;
> > >
> > > host_isa_ext = kvm_isa_ext_arr[reg_num];
> > > - if (__riscv_isa_extension_available(&vcpu->arch.isa, host_isa_ext))
> > > + if (__riscv_isa_extension_available(vcpu->arch.isa, host_isa_ext))
> > > reg_val = 1; /* Mark the given extension as available */
> > >
> > > if (copy_to_user(uaddr, ®_val, KVM_REG_SIZE(reg->id)))
> > > @@ -437,30 +451,17 @@ static int kvm_riscv_vcpu_set_reg_isa_ext(struct kvm_vcpu *vcpu,
> > > if (!__riscv_isa_extension_available(NULL, host_isa_ext))
> > > return -EOPNOTSUPP;
> > >
> > > - if (host_isa_ext >= RISCV_ISA_EXT_BASE &&
> > > - host_isa_ext < RISCV_ISA_EXT_MAX) {
> > > - /*
> > > - * Multi-letter ISA extension. Currently there is no provision
> > > - * to enable/disable the multi-letter ISA extensions for guests.
> > > - * Return success if the request is to enable any ISA extension
> > > - * that is available in the hardware.
> > > - * Return -EOPNOTSUPP otherwise.
> > > - */
> > > - if (!reg_val)
> > > - return -EOPNOTSUPP;
> > > - else
> > > - return 0;
> > > - }
> > > -
> > > - /* Single letter base ISA extension */
> > > if (!vcpu->arch.ran_atleast_once) {
> > > + /* All multi-letter extension and a few single letter extension can be disabled */
> > > host_isa_ext_mask = BIT_MASK(host_isa_ext);
> > > - if (!reg_val && (host_isa_ext_mask & KVM_RISCV_ISA_DISABLE_ALLOWED))
> > > - vcpu->arch.isa &= ~host_isa_ext_mask;
> > > + if (!reg_val &&
> > > + ((host_isa_ext_mask & KVM_RISCV_ISA_DISABLE_ALLOWED) ||
> > > + ((host_isa_ext >= RISCV_ISA_EXT_BASE) && (host_isa_ext < RISCV_ISA_EXT_MAX))))
> > > + clear_bit(host_isa_ext, vcpu->arch.isa);
> > > + else if (reg_val == 1 && (host_isa_ext != RISCV_ISA_EXT_h))
> > > + set_bit(host_isa_ext, vcpu->arch.isa);
> > > else
> > > - vcpu->arch.isa |= host_isa_ext_mask;
> > > - vcpu->arch.isa &= riscv_isa_extension_base(NULL);
> > > - vcpu->arch.isa &= KVM_RISCV_ISA_ALLOWED;
> > > + return -EINVAL;
> >
> > A slightly more readable version of above sequence can be:
> >
> > /* All multi-letter extension and a few single letter
> > extension can be disabled */
> > if (host_isa_ext >= RISCV_ISA_EXT_MAX)
> > return -EINVAL;
> > disable_allow_mask = KVM_RISCV_ISA_DISABLE_ALLOWED;
> > if (reg_val == 1)
> > set_bit(host_isa_ext, vcpu->arch.isa);
>
> Shouldn't we ensure that (host_isa_ext != RISCV_ISA_EXT_h) here ?
Ahh, yes. I have updated the KVM queue.
Thanks,
Anup
>
> > else if (!reg_val && test_bit(host_isa_ext, &disable_allow_mask))
> > clear_bit(host_isa_ext, vcpu->arch.isa);
> > else
> > return -EINVAL;
> >
> >
> > > kvm_riscv_vcpu_fp_reset(vcpu);
> > > } else {
> > > return -EOPNOTSUPP;
> > > diff --git a/arch/riscv/kvm/vcpu_fp.c b/arch/riscv/kvm/vcpu_fp.c
> > > index d4308c512007..748a8f6a9b5d 100644
> > > --- a/arch/riscv/kvm/vcpu_fp.c
> > > +++ b/arch/riscv/kvm/vcpu_fp.c
> > > @@ -16,12 +16,11 @@
> > > #ifdef CONFIG_FPU
> > > void kvm_riscv_vcpu_fp_reset(struct kvm_vcpu *vcpu)
> > > {
> > > - unsigned long isa = vcpu->arch.isa;
> > > struct kvm_cpu_context *cntx = &vcpu->arch.guest_context;
> > >
> > > cntx->sstatus &= ~SR_FS;
> > > - if (riscv_isa_extension_available(&isa, f) ||
> > > - riscv_isa_extension_available(&isa, d))
> > > + if (riscv_isa_extension_available(vcpu->arch.isa, f) ||
> > > + riscv_isa_extension_available(vcpu->arch.isa, d))
> > > cntx->sstatus |= SR_FS_INITIAL;
> > > else
> > > cntx->sstatus |= SR_FS_OFF;
> > > @@ -34,24 +33,24 @@ static void kvm_riscv_vcpu_fp_clean(struct kvm_cpu_context *cntx)
> > > }
> > >
> > > void kvm_riscv_vcpu_guest_fp_save(struct kvm_cpu_context *cntx,
> > > - unsigned long isa)
> > > + unsigned long *isa)
> > > {
> > > if ((cntx->sstatus & SR_FS) == SR_FS_DIRTY) {
> > > - if (riscv_isa_extension_available(&isa, d))
> > > + if (riscv_isa_extension_available(isa, d))
> > > __kvm_riscv_fp_d_save(cntx);
> > > - else if (riscv_isa_extension_available(&isa, f))
> > > + else if (riscv_isa_extension_available(isa, f))
> > > __kvm_riscv_fp_f_save(cntx);
> > > kvm_riscv_vcpu_fp_clean(cntx);
> > > }
> > > }
> > >
> > > void kvm_riscv_vcpu_guest_fp_restore(struct kvm_cpu_context *cntx,
> > > - unsigned long isa)
> > > + unsigned long *isa)
> > > {
> > > if ((cntx->sstatus & SR_FS) != SR_FS_OFF) {
> > > - if (riscv_isa_extension_available(&isa, d))
> > > + if (riscv_isa_extension_available(isa, d))
> > > __kvm_riscv_fp_d_restore(cntx);
> > > - else if (riscv_isa_extension_available(&isa, f))
> > > + else if (riscv_isa_extension_available(isa, f))
> > > __kvm_riscv_fp_f_restore(cntx);
> > > kvm_riscv_vcpu_fp_clean(cntx);
> > > }
> > > @@ -80,7 +79,6 @@ int kvm_riscv_vcpu_get_reg_fp(struct kvm_vcpu *vcpu,
> > > unsigned long rtype)
> > > {
> > > struct kvm_cpu_context *cntx = &vcpu->arch.guest_context;
> > > - unsigned long isa = vcpu->arch.isa;
> > > unsigned long __user *uaddr =
> > > (unsigned long __user *)(unsigned long)reg->addr;
> > > unsigned long reg_num = reg->id & ~(KVM_REG_ARCH_MASK |
> > > @@ -89,7 +87,7 @@ int kvm_riscv_vcpu_get_reg_fp(struct kvm_vcpu *vcpu,
> > > void *reg_val;
> > >
> > > if ((rtype == KVM_REG_RISCV_FP_F) &&
> > > - riscv_isa_extension_available(&isa, f)) {
> > > + riscv_isa_extension_available(vcpu->arch.isa, f)) {
> > > if (KVM_REG_SIZE(reg->id) != sizeof(u32))
> > > return -EINVAL;
> > > if (reg_num == KVM_REG_RISCV_FP_F_REG(fcsr))
> > > @@ -100,7 +98,7 @@ int kvm_riscv_vcpu_get_reg_fp(struct kvm_vcpu *vcpu,
> > > else
> > > return -EINVAL;
> > > } else if ((rtype == KVM_REG_RISCV_FP_D) &&
> > > - riscv_isa_extension_available(&isa, d)) {
> > > + riscv_isa_extension_available(vcpu->arch.isa, d)) {
> > > if (reg_num == KVM_REG_RISCV_FP_D_REG(fcsr)) {
> > > if (KVM_REG_SIZE(reg->id) != sizeof(u32))
> > > return -EINVAL;
> > > @@ -126,7 +124,6 @@ int kvm_riscv_vcpu_set_reg_fp(struct kvm_vcpu *vcpu,
> > > unsigned long rtype)
> > > {
> > > struct kvm_cpu_context *cntx = &vcpu->arch.guest_context;
> > > - unsigned long isa = vcpu->arch.isa;
> > > unsigned long __user *uaddr =
> > > (unsigned long __user *)(unsigned long)reg->addr;
> > > unsigned long reg_num = reg->id & ~(KVM_REG_ARCH_MASK |
> > > @@ -135,7 +132,7 @@ int kvm_riscv_vcpu_set_reg_fp(struct kvm_vcpu *vcpu,
> > > void *reg_val;
> > >
> > > if ((rtype == KVM_REG_RISCV_FP_F) &&
> > > - riscv_isa_extension_available(&isa, f)) {
> > > + riscv_isa_extension_available(vcpu->arch.isa, f)) {
> > > if (KVM_REG_SIZE(reg->id) != sizeof(u32))
> > > return -EINVAL;
> > > if (reg_num == KVM_REG_RISCV_FP_F_REG(fcsr))
> > > @@ -146,7 +143,7 @@ int kvm_riscv_vcpu_set_reg_fp(struct kvm_vcpu *vcpu,
> > > else
> > > return -EINVAL;
> > > } else if ((rtype == KVM_REG_RISCV_FP_D) &&
> > > - riscv_isa_extension_available(&isa, d)) {
> > > + riscv_isa_extension_available(vcpu->arch.isa, d)) {
> > > if (reg_num == KVM_REG_RISCV_FP_D_REG(fcsr)) {
> > > if (KVM_REG_SIZE(reg->id) != sizeof(u32))
> > > return -EINVAL;
> > > --
> > > 2.25.1
> > >
> >
> > Apart from minor comments above, this looks good to me.
> >
> Thanks.
>
> > I have taken care of the above comments and queued it for 5.20. I
> > can certainly modify my queue if you want further changes in this patch.
> >
> > Thanks,
> > Anup