[PATCH 0/4] KVM RISC-V fixes for ONE

public inbox for kvm@vger.kernel.org
 help / color / mirror / Atom feed

* [PATCH 0/4] KVM RISC-V fixes for ONE_REG interface
@ 2023-09-18 18:06 Anup Patel
  2023-09-18 18:06 ` [PATCH 1/4] RISC-V: KVM: Fix KVM_GET_REG_LIST API for ISA_EXT registers Anup Patel
                   ` (3 more replies)
  0 siblings, 4 replies; 21+ messages in thread
From: Anup Patel @ 2023-09-18 18:06 UTC (permalink / raw)
  To: Paolo Bonzini, Atish Patra, Shuah Khan
  Cc: Palmer Dabbelt, Paul Walmsley, Andrew Jones, kvm, kvm-riscv,
	linux-riscv, linux-kernel, linux-kselftest, Anup Patel

This series includes few assorted fixes for KVM RISC-V ONE_REG interface
and KVM_GET_REG_LIST API.

These patches can also be found in riscv_kvm_onereg_fixes_v1 branch at:
https://github.com/avpatel/linux.git

Anup Patel (4):
  RISC-V: KVM: Fix KVM_GET_REG_LIST API for ISA_EXT registers
  RISC-V: KVM: Fix riscv_vcpu_get_isa_ext_single() for missing
    extensions
  KVM: riscv: selftests: Fix ISA_EXT register handling in get-reg-list
  KVM: riscv: selftests: Selectively filter-out AIA registers

 arch/riscv/kvm/vcpu_onereg.c                  |  7 ++-
 .../selftests/kvm/riscv/get-reg-list.c        | 58 ++++++++++++++-----
 2 files changed, 47 insertions(+), 18 deletions(-)

-- 
2.34.1


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

* [PATCH 1/4] RISC-V: KVM: Fix KVM_GET_REG_LIST API for ISA_EXT registers
  2023-09-18 18:06 [PATCH 0/4] KVM RISC-V fixes for ONE_REG interface Anup Patel
@ 2023-09-18 18:06 ` Anup Patel
  2023-09-19 18:57   ` Atish Patra
  2023-09-20  5:03   ` Andrew Jones
  2023-09-18 18:06 ` [PATCH 2/4] RISC-V: KVM: Fix riscv_vcpu_get_isa_ext_single() for missing extensions Anup Patel
                   ` (2 subsequent siblings)
  3 siblings, 2 replies; 21+ messages in thread
From: Anup Patel @ 2023-09-18 18:06 UTC (permalink / raw)
  To: Paolo Bonzini, Atish Patra, Shuah Khan
  Cc: Palmer Dabbelt, Paul Walmsley, Andrew Jones, kvm, kvm-riscv,
	linux-riscv, linux-kernel, linux-kselftest, Anup Patel

The ISA_EXT registers to enabled/disable ISA extensions for VCPU
are always available when underlying host has the corresponding
ISA extension. The copy_isa_ext_reg_indices() called by the
KVM_GET_REG_LIST API does not align with this expectation so
let's fix it.

Fixes: 031f9efafc08 ("KVM: riscv: Add KVM_GET_REG_LIST API support")
Signed-off-by: Anup Patel <apatel@ventanamicro.com>
---
 arch/riscv/kvm/vcpu_onereg.c | 2 +-
 1 file changed, 1 insertion(+), 1 deletion(-)

diff --git a/arch/riscv/kvm/vcpu_onereg.c b/arch/riscv/kvm/vcpu_onereg.c
index 1b7e9fa265cb..e7e833ced91b 100644
--- a/arch/riscv/kvm/vcpu_onereg.c
+++ b/arch/riscv/kvm/vcpu_onereg.c
@@ -842,7 +842,7 @@ static int copy_isa_ext_reg_indices(const struct kvm_vcpu *vcpu,
 		u64 reg = KVM_REG_RISCV | size | KVM_REG_RISCV_ISA_EXT | i;
 
 		isa_ext = kvm_isa_ext_arr[i];
-		if (!__riscv_isa_extension_available(vcpu->arch.isa, isa_ext))
+		if (!__riscv_isa_extension_available(NULL, isa_ext))
 			continue;
 
 		if (uindices) {
-- 
2.34.1


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

* Re: [PATCH 1/4] RISC-V: KVM: Fix KVM_GET_REG_LIST API for ISA_EXT registers
  2023-09-18 18:06 ` [PATCH 1/4] RISC-V: KVM: Fix KVM_GET_REG_LIST API for ISA_EXT registers Anup Patel
@ 2023-09-19 18:57   ` Atish Patra
  2023-09-20  5:03   ` Andrew Jones
  1 sibling, 0 replies; 21+ messages in thread
From: Atish Patra @ 2023-09-19 18:57 UTC (permalink / raw)
  To: Anup Patel
  Cc: Paolo Bonzini, Shuah Khan, Palmer Dabbelt, Paul Walmsley,
	Andrew Jones, kvm, kvm-riscv, linux-riscv, linux-kernel,
	linux-kselftest

On Mon, Sep 18, 2023 at 11:07 AM Anup Patel <apatel@ventanamicro.com> wrote:
>
> The ISA_EXT registers to enabled/disable ISA extensions for VCPU
> are always available when underlying host has the corresponding
> ISA extension. The copy_isa_ext_reg_indices() called by the
> KVM_GET_REG_LIST API does not align with this expectation so
> let's fix it.
>
> Fixes: 031f9efafc08 ("KVM: riscv: Add KVM_GET_REG_LIST API support")
> Signed-off-by: Anup Patel <apatel@ventanamicro.com>
> ---
>  arch/riscv/kvm/vcpu_onereg.c | 2 +-
>  1 file changed, 1 insertion(+), 1 deletion(-)
>
> diff --git a/arch/riscv/kvm/vcpu_onereg.c b/arch/riscv/kvm/vcpu_onereg.c
> index 1b7e9fa265cb..e7e833ced91b 100644
> --- a/arch/riscv/kvm/vcpu_onereg.c
> +++ b/arch/riscv/kvm/vcpu_onereg.c
> @@ -842,7 +842,7 @@ static int copy_isa_ext_reg_indices(const struct kvm_vcpu *vcpu,
>                 u64 reg = KVM_REG_RISCV | size | KVM_REG_RISCV_ISA_EXT | i;
>
>                 isa_ext = kvm_isa_ext_arr[i];
> -               if (!__riscv_isa_extension_available(vcpu->arch.isa, isa_ext))
> +               if (!__riscv_isa_extension_available(NULL, isa_ext))
>                         continue;
>
>                 if (uindices) {
> --
> 2.34.1
>


Reviewed-by: Atish Patra <atishp@rivosinc.com>
-- 
Regards,
Atish

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

* Re: [PATCH 1/4] RISC-V: KVM: Fix KVM_GET_REG_LIST API for ISA_EXT registers
  2023-09-18 18:06 ` [PATCH 1/4] RISC-V: KVM: Fix KVM_GET_REG_LIST API for ISA_EXT registers Anup Patel
  2023-09-19 18:57   ` Atish Patra
@ 2023-09-20  5:03   ` Andrew Jones
  1 sibling, 0 replies; 21+ messages in thread
From: Andrew Jones @ 2023-09-20  5:03 UTC (permalink / raw)
  To: Anup Patel
  Cc: Paolo Bonzini, Atish Patra, Shuah Khan, Palmer Dabbelt,
	Paul Walmsley, kvm, kvm-riscv, linux-riscv, linux-kernel,
	linux-kselftest

On Mon, Sep 18, 2023 at 11:36:43PM +0530, Anup Patel wrote:
> The ISA_EXT registers to enabled/disable ISA extensions for VCPU
> are always available when underlying host has the corresponding
> ISA extension. The copy_isa_ext_reg_indices() called by the
> KVM_GET_REG_LIST API does not align with this expectation so
> let's fix it.
> 
> Fixes: 031f9efafc08 ("KVM: riscv: Add KVM_GET_REG_LIST API support")
> Signed-off-by: Anup Patel <apatel@ventanamicro.com>
> ---
>  arch/riscv/kvm/vcpu_onereg.c | 2 +-
>  1 file changed, 1 insertion(+), 1 deletion(-)
> 
> diff --git a/arch/riscv/kvm/vcpu_onereg.c b/arch/riscv/kvm/vcpu_onereg.c
> index 1b7e9fa265cb..e7e833ced91b 100644
> --- a/arch/riscv/kvm/vcpu_onereg.c
> +++ b/arch/riscv/kvm/vcpu_onereg.c
> @@ -842,7 +842,7 @@ static int copy_isa_ext_reg_indices(const struct kvm_vcpu *vcpu,
>  		u64 reg = KVM_REG_RISCV | size | KVM_REG_RISCV_ISA_EXT | i;
>  
>  		isa_ext = kvm_isa_ext_arr[i];
> -		if (!__riscv_isa_extension_available(vcpu->arch.isa, isa_ext))
> +		if (!__riscv_isa_extension_available(NULL, isa_ext))
>  			continue;
>  
>  		if (uindices) {
> -- 
> 2.34.1
>


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

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

* [PATCH 2/4] RISC-V: KVM: Fix riscv_vcpu_get_isa_ext_single() for missing extensions
  2023-09-18 18:06 [PATCH 0/4] KVM RISC-V fixes for ONE_REG interface Anup Patel
  2023-09-18 18:06 ` [PATCH 1/4] RISC-V: KVM: Fix KVM_GET_REG_LIST API for ISA_EXT registers Anup Patel
@ 2023-09-18 18:06 ` Anup Patel
  2023-09-19 18:59   ` Atish Patra
  2023-09-20  5:04   ` Andrew Jones
  2023-09-18 18:06 ` [PATCH 3/4] KVM: riscv: selftests: Fix ISA_EXT register handling in get-reg-list Anup Patel
  2023-09-18 18:06 ` [PATCH 4/4] KVM: riscv: selftests: Selectively filter-out AIA registers Anup Patel
  3 siblings, 2 replies; 21+ messages in thread
From: Anup Patel @ 2023-09-18 18:06 UTC (permalink / raw)
  To: Paolo Bonzini, Atish Patra, Shuah Khan
  Cc: Palmer Dabbelt, Paul Walmsley, Andrew Jones, kvm, kvm-riscv,
	linux-riscv, linux-kernel, linux-kselftest, Anup Patel

The riscv_vcpu_get_isa_ext_single() should fail with -ENOENT error
when corresponding ISA extension is not available on the host.

Fixes: e98b1085be79 ("RISC-V: KVM: Factor-out ONE_REG related code to its own source file")
Signed-off-by: Anup Patel <apatel@ventanamicro.com>
---
 arch/riscv/kvm/vcpu_onereg.c | 5 ++++-
 1 file changed, 4 insertions(+), 1 deletion(-)

diff --git a/arch/riscv/kvm/vcpu_onereg.c b/arch/riscv/kvm/vcpu_onereg.c
index e7e833ced91b..b7e0e03c69b1 100644
--- a/arch/riscv/kvm/vcpu_onereg.c
+++ b/arch/riscv/kvm/vcpu_onereg.c
@@ -460,8 +460,11 @@ static int riscv_vcpu_get_isa_ext_single(struct kvm_vcpu *vcpu,
 	    reg_num >= ARRAY_SIZE(kvm_isa_ext_arr))
 		return -ENOENT;
 
-	*reg_val = 0;
 	host_isa_ext = kvm_isa_ext_arr[reg_num];
+	if (!__riscv_isa_extension_available(NULL, host_isa_ext))
+		return -ENOENT;
+
+	*reg_val = 0;
 	if (__riscv_isa_extension_available(vcpu->arch.isa, host_isa_ext))
 		*reg_val = 1; /* Mark the given extension as available */
 
-- 
2.34.1


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

* Re: [PATCH 2/4] RISC-V: KVM: Fix riscv_vcpu_get_isa_ext_single() for missing extensions
  2023-09-18 18:06 ` [PATCH 2/4] RISC-V: KVM: Fix riscv_vcpu_get_isa_ext_single() for missing extensions Anup Patel
@ 2023-09-19 18:59   ` Atish Patra
  2023-09-20  5:04   ` Andrew Jones
  1 sibling, 0 replies; 21+ messages in thread
From: Atish Patra @ 2023-09-19 18:59 UTC (permalink / raw)
  To: Anup Patel
  Cc: Paolo Bonzini, Shuah Khan, Palmer Dabbelt, Paul Walmsley,
	Andrew Jones, kvm, kvm-riscv, linux-riscv, linux-kernel,
	linux-kselftest

On Mon, Sep 18, 2023 at 11:07 AM Anup Patel <apatel@ventanamicro.com> wrote:
>
> The riscv_vcpu_get_isa_ext_single() should fail with -ENOENT error
> when corresponding ISA extension is not available on the host.
>
> Fixes: e98b1085be79 ("RISC-V: KVM: Factor-out ONE_REG related code to its own source file")
> Signed-off-by: Anup Patel <apatel@ventanamicro.com>
> ---
>  arch/riscv/kvm/vcpu_onereg.c | 5 ++++-
>  1 file changed, 4 insertions(+), 1 deletion(-)
>
> diff --git a/arch/riscv/kvm/vcpu_onereg.c b/arch/riscv/kvm/vcpu_onereg.c
> index e7e833ced91b..b7e0e03c69b1 100644
> --- a/arch/riscv/kvm/vcpu_onereg.c
> +++ b/arch/riscv/kvm/vcpu_onereg.c
> @@ -460,8 +460,11 @@ static int riscv_vcpu_get_isa_ext_single(struct kvm_vcpu *vcpu,
>             reg_num >= ARRAY_SIZE(kvm_isa_ext_arr))
>                 return -ENOENT;
>
> -       *reg_val = 0;
>         host_isa_ext = kvm_isa_ext_arr[reg_num];
> +       if (!__riscv_isa_extension_available(NULL, host_isa_ext))
> +               return -ENOENT;
> +
> +       *reg_val = 0;
>         if (__riscv_isa_extension_available(vcpu->arch.isa, host_isa_ext))
>                 *reg_val = 1; /* Mark the given extension as available */
>
> --
> 2.34.1
>

Reviewed-by: Atish Patra <atishp@rivosinc.com>

-- 
Regards,
Atish

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

* Re: [PATCH 2/4] RISC-V: KVM: Fix riscv_vcpu_get_isa_ext_single() for missing extensions
  2023-09-18 18:06 ` [PATCH 2/4] RISC-V: KVM: Fix riscv_vcpu_get_isa_ext_single() for missing extensions Anup Patel
  2023-09-19 18:59   ` Atish Patra
@ 2023-09-20  5:04   ` Andrew Jones
  1 sibling, 0 replies; 21+ messages in thread
From: Andrew Jones @ 2023-09-20  5:04 UTC (permalink / raw)
  To: Anup Patel
  Cc: Paolo Bonzini, Atish Patra, Shuah Khan, Palmer Dabbelt,
	Paul Walmsley, kvm, kvm-riscv, linux-riscv, linux-kernel,
	linux-kselftest

On Mon, Sep 18, 2023 at 11:36:44PM +0530, Anup Patel wrote:
> The riscv_vcpu_get_isa_ext_single() should fail with -ENOENT error
> when corresponding ISA extension is not available on the host.
> 
> Fixes: e98b1085be79 ("RISC-V: KVM: Factor-out ONE_REG related code to its own source file")
> Signed-off-by: Anup Patel <apatel@ventanamicro.com>
> ---
>  arch/riscv/kvm/vcpu_onereg.c | 5 ++++-
>  1 file changed, 4 insertions(+), 1 deletion(-)
> 
> diff --git a/arch/riscv/kvm/vcpu_onereg.c b/arch/riscv/kvm/vcpu_onereg.c
> index e7e833ced91b..b7e0e03c69b1 100644
> --- a/arch/riscv/kvm/vcpu_onereg.c
> +++ b/arch/riscv/kvm/vcpu_onereg.c
> @@ -460,8 +460,11 @@ static int riscv_vcpu_get_isa_ext_single(struct kvm_vcpu *vcpu,
>  	    reg_num >= ARRAY_SIZE(kvm_isa_ext_arr))
>  		return -ENOENT;
>  
> -	*reg_val = 0;
>  	host_isa_ext = kvm_isa_ext_arr[reg_num];
> +	if (!__riscv_isa_extension_available(NULL, host_isa_ext))
> +		return -ENOENT;
> +
> +	*reg_val = 0;
>  	if (__riscv_isa_extension_available(vcpu->arch.isa, host_isa_ext))
>  		*reg_val = 1; /* Mark the given extension as available */
>  
> -- 
> 2.34.1
>

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

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

* [PATCH 3/4] KVM: riscv: selftests: Fix ISA_EXT register handling in get-reg-list
  2023-09-18 18:06 [PATCH 0/4] KVM RISC-V fixes for ONE_REG interface Anup Patel
  2023-09-18 18:06 ` [PATCH 1/4] RISC-V: KVM: Fix KVM_GET_REG_LIST API for ISA_EXT registers Anup Patel
  2023-09-18 18:06 ` [PATCH 2/4] RISC-V: KVM: Fix riscv_vcpu_get_isa_ext_single() for missing extensions Anup Patel
@ 2023-09-18 18:06 ` Anup Patel
  2023-09-19 19:54   ` Atish Patra
  2023-09-20  5:13   ` Andrew Jones
  2023-09-18 18:06 ` [PATCH 4/4] KVM: riscv: selftests: Selectively filter-out AIA registers Anup Patel
  3 siblings, 2 replies; 21+ messages in thread
From: Anup Patel @ 2023-09-18 18:06 UTC (permalink / raw)
  To: Paolo Bonzini, Atish Patra, Shuah Khan
  Cc: Palmer Dabbelt, Paul Walmsley, Andrew Jones, kvm, kvm-riscv,
	linux-riscv, linux-kernel, linux-kselftest, Anup Patel

Same set of ISA_EXT registers are not present on all host because
ISA_EXT registers are visible to the KVM user space based on the
ISA extensions available on the host. Also, disabling an ISA
extension using corresponding ISA_EXT register does not affect
the visibility of the ISA_EXT register itself.

Based on the above, we should filter-out all ISA_EXT registers.

Fixes: 477069398ed6 ("KVM: riscv: selftests: Add get-reg-list test")
Signed-off-by: Anup Patel <apatel@ventanamicro.com>
---
 .../selftests/kvm/riscv/get-reg-list.c        | 35 +++++++++++--------
 1 file changed, 21 insertions(+), 14 deletions(-)

diff --git a/tools/testing/selftests/kvm/riscv/get-reg-list.c b/tools/testing/selftests/kvm/riscv/get-reg-list.c
index d8ecacd03ecf..76c0ad11e423 100644
--- a/tools/testing/selftests/kvm/riscv/get-reg-list.c
+++ b/tools/testing/selftests/kvm/riscv/get-reg-list.c
@@ -14,17 +14,33 @@
 
 bool filter_reg(__u64 reg)
 {
+	switch (reg & ~REG_MASK) {
 	/*
-	 * Some ISA extensions are optional and not present on all host,
-	 * but they can't be disabled through ISA_EXT registers when present.
-	 * So, to make life easy, just filtering out these kind of registers.
+	 * Same set of ISA_EXT registers are not present on all host because
+	 * ISA_EXT registers are visible to the KVM user space based on the
+	 * ISA extensions available on the host. Also, disabling an ISA
+	 * extension using corresponding ISA_EXT register does not affect
+	 * the visibility of the ISA_EXT register itself.
+	 *
+	 * Based on above, we should filter-out all ISA_EXT registers.
 	 */
-	switch (reg & ~REG_MASK) {
+	case KVM_REG_RISCV_ISA_EXT | KVM_RISCV_ISA_EXT_A:
+	case KVM_REG_RISCV_ISA_EXT | KVM_RISCV_ISA_EXT_C:
+	case KVM_REG_RISCV_ISA_EXT | KVM_RISCV_ISA_EXT_D:
+	case KVM_REG_RISCV_ISA_EXT | KVM_RISCV_ISA_EXT_F:
+	case KVM_REG_RISCV_ISA_EXT | KVM_RISCV_ISA_EXT_H:
+	case KVM_REG_RISCV_ISA_EXT | KVM_RISCV_ISA_EXT_I:
+	case KVM_REG_RISCV_ISA_EXT | KVM_RISCV_ISA_EXT_M:
+	case KVM_REG_RISCV_ISA_EXT | KVM_RISCV_ISA_EXT_SVPBMT:
 	case KVM_REG_RISCV_ISA_EXT | KVM_RISCV_ISA_EXT_SSTC:
 	case KVM_REG_RISCV_ISA_EXT | KVM_RISCV_ISA_EXT_SVINVAL:
 	case KVM_REG_RISCV_ISA_EXT | KVM_RISCV_ISA_EXT_ZIHINTPAUSE:
+	case KVM_REG_RISCV_ISA_EXT | KVM_RISCV_ISA_EXT_ZICBOM:
+	case KVM_REG_RISCV_ISA_EXT | KVM_RISCV_ISA_EXT_ZICBOZ:
 	case KVM_REG_RISCV_ISA_EXT | KVM_RISCV_ISA_EXT_ZBB:
 	case KVM_REG_RISCV_ISA_EXT | KVM_RISCV_ISA_EXT_SSAIA:
+	case KVM_REG_RISCV_ISA_EXT | KVM_RISCV_ISA_EXT_V:
+	case KVM_REG_RISCV_ISA_EXT | KVM_RISCV_ISA_EXT_SVNAPOT:
 	case KVM_REG_RISCV_ISA_EXT | KVM_RISCV_ISA_EXT_ZBA:
 	case KVM_REG_RISCV_ISA_EXT | KVM_RISCV_ISA_EXT_ZBS:
 	case KVM_REG_RISCV_ISA_EXT | KVM_RISCV_ISA_EXT_ZICNTR:
@@ -50,12 +66,7 @@ static inline bool vcpu_has_ext(struct kvm_vcpu *vcpu, int ext)
 	unsigned long value;
 
 	ret = __vcpu_get_reg(vcpu, RISCV_ISA_EXT_REG(ext), &value);
-	if (ret) {
-		printf("Failed to get ext %d", ext);
-		return false;
-	}
-
-	return !!value;
+	return (ret) ? false : !!value;
 }
 
 void finalize_vcpu(struct kvm_vcpu *vcpu, struct vcpu_reg_list *c)
@@ -506,10 +517,6 @@ static __u64 base_regs[] = {
 	KVM_REG_RISCV | KVM_REG_SIZE_U64 | KVM_REG_RISCV_TIMER | KVM_REG_RISCV_TIMER_REG(time),
 	KVM_REG_RISCV | KVM_REG_SIZE_U64 | KVM_REG_RISCV_TIMER | KVM_REG_RISCV_TIMER_REG(compare),
 	KVM_REG_RISCV | KVM_REG_SIZE_U64 | KVM_REG_RISCV_TIMER | KVM_REG_RISCV_TIMER_REG(state),
-	KVM_REG_RISCV | KVM_REG_SIZE_ULONG | KVM_REG_RISCV_ISA_EXT | KVM_RISCV_ISA_EXT_A,
-	KVM_REG_RISCV | KVM_REG_SIZE_ULONG | KVM_REG_RISCV_ISA_EXT | KVM_RISCV_ISA_EXT_C,
-	KVM_REG_RISCV | KVM_REG_SIZE_ULONG | KVM_REG_RISCV_ISA_EXT | KVM_RISCV_ISA_EXT_I,
-	KVM_REG_RISCV | KVM_REG_SIZE_ULONG | KVM_REG_RISCV_ISA_EXT | KVM_RISCV_ISA_EXT_M,
 	KVM_REG_RISCV | KVM_REG_SIZE_ULONG | KVM_REG_RISCV_SBI_EXT | KVM_REG_RISCV_SBI_SINGLE | KVM_RISCV_SBI_EXT_V01,
 	KVM_REG_RISCV | KVM_REG_SIZE_ULONG | KVM_REG_RISCV_SBI_EXT | KVM_REG_RISCV_SBI_SINGLE | KVM_RISCV_SBI_EXT_TIME,
 	KVM_REG_RISCV | KVM_REG_SIZE_ULONG | KVM_REG_RISCV_SBI_EXT | KVM_REG_RISCV_SBI_SINGLE | KVM_RISCV_SBI_EXT_IPI,
-- 
2.34.1


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

* Re: [PATCH 3/4] KVM: riscv: selftests: Fix ISA_EXT register handling in get-reg-list
  2023-09-18 18:06 ` [PATCH 3/4] KVM: riscv: selftests: Fix ISA_EXT register handling in get-reg-list Anup Patel
@ 2023-09-19 19:54   ` Atish Patra
  2023-09-20 13:56     ` Anup Patel
  2023-09-20  5:13   ` Andrew Jones
  1 sibling, 1 reply; 21+ messages in thread
From: Atish Patra @ 2023-09-19 19:54 UTC (permalink / raw)
  To: Anup Patel
  Cc: Paolo Bonzini, Shuah Khan, Palmer Dabbelt, Paul Walmsley,
	Andrew Jones, kvm, kvm-riscv, linux-riscv, linux-kernel,
	linux-kselftest

On Mon, Sep 18, 2023 at 11:07 AM Anup Patel <apatel@ventanamicro.com> wrote:
>
> Same set of ISA_EXT registers are not present on all host because
> ISA_EXT registers are visible to the KVM user space based on the
> ISA extensions available on the host. Also, disabling an ISA
> extension using corresponding ISA_EXT register does not affect
> the visibility of the ISA_EXT register itself.
>
> Based on the above, we should filter-out all ISA_EXT registers.
>

In that case, we don't need the switch case any more. Just a
conditional check with KVM_RISCV_ISA_EXT_MAX should be sufficient.

> Fixes: 477069398ed6 ("KVM: riscv: selftests: Add get-reg-list test")
> Signed-off-by: Anup Patel <apatel@ventanamicro.com>
> ---
>  .../selftests/kvm/riscv/get-reg-list.c        | 35 +++++++++++--------
>  1 file changed, 21 insertions(+), 14 deletions(-)
>
> diff --git a/tools/testing/selftests/kvm/riscv/get-reg-list.c b/tools/testing/selftests/kvm/riscv/get-reg-list.c
> index d8ecacd03ecf..76c0ad11e423 100644
> --- a/tools/testing/selftests/kvm/riscv/get-reg-list.c
> +++ b/tools/testing/selftests/kvm/riscv/get-reg-list.c
> @@ -14,17 +14,33 @@
>
>  bool filter_reg(__u64 reg)
>  {
> +       switch (reg & ~REG_MASK) {
>         /*
> -        * Some ISA extensions are optional and not present on all host,
> -        * but they can't be disabled through ISA_EXT registers when present.
> -        * So, to make life easy, just filtering out these kind of registers.
> +        * Same set of ISA_EXT registers are not present on all host because
> +        * ISA_EXT registers are visible to the KVM user space based on the
> +        * ISA extensions available on the host. Also, disabling an ISA
> +        * extension using corresponding ISA_EXT register does not affect
> +        * the visibility of the ISA_EXT register itself.
> +        *
> +        * Based on above, we should filter-out all ISA_EXT registers.
>          */
> -       switch (reg & ~REG_MASK) {
> +       case KVM_REG_RISCV_ISA_EXT | KVM_RISCV_ISA_EXT_A:
> +       case KVM_REG_RISCV_ISA_EXT | KVM_RISCV_ISA_EXT_C:
> +       case KVM_REG_RISCV_ISA_EXT | KVM_RISCV_ISA_EXT_D:
> +       case KVM_REG_RISCV_ISA_EXT | KVM_RISCV_ISA_EXT_F:
> +       case KVM_REG_RISCV_ISA_EXT | KVM_RISCV_ISA_EXT_H:
> +       case KVM_REG_RISCV_ISA_EXT | KVM_RISCV_ISA_EXT_I:
> +       case KVM_REG_RISCV_ISA_EXT | KVM_RISCV_ISA_EXT_M:
> +       case KVM_REG_RISCV_ISA_EXT | KVM_RISCV_ISA_EXT_SVPBMT:
>         case KVM_REG_RISCV_ISA_EXT | KVM_RISCV_ISA_EXT_SSTC:
>         case KVM_REG_RISCV_ISA_EXT | KVM_RISCV_ISA_EXT_SVINVAL:
>         case KVM_REG_RISCV_ISA_EXT | KVM_RISCV_ISA_EXT_ZIHINTPAUSE:
> +       case KVM_REG_RISCV_ISA_EXT | KVM_RISCV_ISA_EXT_ZICBOM:
> +       case KVM_REG_RISCV_ISA_EXT | KVM_RISCV_ISA_EXT_ZICBOZ:
>         case KVM_REG_RISCV_ISA_EXT | KVM_RISCV_ISA_EXT_ZBB:
>         case KVM_REG_RISCV_ISA_EXT | KVM_RISCV_ISA_EXT_SSAIA:
> +       case KVM_REG_RISCV_ISA_EXT | KVM_RISCV_ISA_EXT_V:
> +       case KVM_REG_RISCV_ISA_EXT | KVM_RISCV_ISA_EXT_SVNAPOT:
>         case KVM_REG_RISCV_ISA_EXT | KVM_RISCV_ISA_EXT_ZBA:
>         case KVM_REG_RISCV_ISA_EXT | KVM_RISCV_ISA_EXT_ZBS:
>         case KVM_REG_RISCV_ISA_EXT | KVM_RISCV_ISA_EXT_ZICNTR:
> @@ -50,12 +66,7 @@ static inline bool vcpu_has_ext(struct kvm_vcpu *vcpu, int ext)
>         unsigned long value;
>
>         ret = __vcpu_get_reg(vcpu, RISCV_ISA_EXT_REG(ext), &value);
> -       if (ret) {
> -               printf("Failed to get ext %d", ext);
> -               return false;
> -       }
> -
> -       return !!value;
> +       return (ret) ? false : !!value;
>  }
>
>  void finalize_vcpu(struct kvm_vcpu *vcpu, struct vcpu_reg_list *c)
> @@ -506,10 +517,6 @@ static __u64 base_regs[] = {
>         KVM_REG_RISCV | KVM_REG_SIZE_U64 | KVM_REG_RISCV_TIMER | KVM_REG_RISCV_TIMER_REG(time),
>         KVM_REG_RISCV | KVM_REG_SIZE_U64 | KVM_REG_RISCV_TIMER | KVM_REG_RISCV_TIMER_REG(compare),
>         KVM_REG_RISCV | KVM_REG_SIZE_U64 | KVM_REG_RISCV_TIMER | KVM_REG_RISCV_TIMER_REG(state),
> -       KVM_REG_RISCV | KVM_REG_SIZE_ULONG | KVM_REG_RISCV_ISA_EXT | KVM_RISCV_ISA_EXT_A,
> -       KVM_REG_RISCV | KVM_REG_SIZE_ULONG | KVM_REG_RISCV_ISA_EXT | KVM_RISCV_ISA_EXT_C,
> -       KVM_REG_RISCV | KVM_REG_SIZE_ULONG | KVM_REG_RISCV_ISA_EXT | KVM_RISCV_ISA_EXT_I,
> -       KVM_REG_RISCV | KVM_REG_SIZE_ULONG | KVM_REG_RISCV_ISA_EXT | KVM_RISCV_ISA_EXT_M,
>         KVM_REG_RISCV | KVM_REG_SIZE_ULONG | KVM_REG_RISCV_SBI_EXT | KVM_REG_RISCV_SBI_SINGLE | KVM_RISCV_SBI_EXT_V01,
>         KVM_REG_RISCV | KVM_REG_SIZE_ULONG | KVM_REG_RISCV_SBI_EXT | KVM_REG_RISCV_SBI_SINGLE | KVM_RISCV_SBI_EXT_TIME,
>         KVM_REG_RISCV | KVM_REG_SIZE_ULONG | KVM_REG_RISCV_SBI_EXT | KVM_REG_RISCV_SBI_SINGLE | KVM_RISCV_SBI_EXT_IPI,
> --
> 2.34.1
>


-- 
Regards,
Atish

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

* Re: [PATCH 3/4] KVM: riscv: selftests: Fix ISA_EXT register handling in get-reg-list
  2023-09-19 19:54   ` Atish Patra
@ 2023-09-20 13:56     ` Anup Patel
  2023-09-20 23:01       ` Atish Patra
  0 siblings, 1 reply; 21+ messages in thread
From: Anup Patel @ 2023-09-20 13:56 UTC (permalink / raw)
  To: Atish Patra
  Cc: Paolo Bonzini, Shuah Khan, Palmer Dabbelt, Paul Walmsley,
	Andrew Jones, kvm, kvm-riscv, linux-riscv, linux-kernel,
	linux-kselftest

On Wed, Sep 20, 2023 at 1:24 AM Atish Patra <atishp@atishpatra.org> wrote:
>
> On Mon, Sep 18, 2023 at 11:07 AM Anup Patel <apatel@ventanamicro.com> wrote:
> >
> > Same set of ISA_EXT registers are not present on all host because
> > ISA_EXT registers are visible to the KVM user space based on the
> > ISA extensions available on the host. Also, disabling an ISA
> > extension using corresponding ISA_EXT register does not affect
> > the visibility of the ISA_EXT register itself.
> >
> > Based on the above, we should filter-out all ISA_EXT registers.
> >
>
> In that case, we don't need the switch case any more. Just a
> conditional check with KVM_RISCV_ISA_EXT_MAX should be sufficient.

If we compare against KVM_RISCV_ISA_EXT_MAX then we will forget
adding test configs for newer ISA extensions.

>
> > Fixes: 477069398ed6 ("KVM: riscv: selftests: Add get-reg-list test")
> > Signed-off-by: Anup Patel <apatel@ventanamicro.com>
> > ---
> >  .../selftests/kvm/riscv/get-reg-list.c        | 35 +++++++++++--------
> >  1 file changed, 21 insertions(+), 14 deletions(-)
> >
> > diff --git a/tools/testing/selftests/kvm/riscv/get-reg-list.c b/tools/testing/selftests/kvm/riscv/get-reg-list.c
> > index d8ecacd03ecf..76c0ad11e423 100644
> > --- a/tools/testing/selftests/kvm/riscv/get-reg-list.c
> > +++ b/tools/testing/selftests/kvm/riscv/get-reg-list.c
> > @@ -14,17 +14,33 @@
> >
> >  bool filter_reg(__u64 reg)
> >  {
> > +       switch (reg & ~REG_MASK) {
> >         /*
> > -        * Some ISA extensions are optional and not present on all host,
> > -        * but they can't be disabled through ISA_EXT registers when present.
> > -        * So, to make life easy, just filtering out these kind of registers.
> > +        * Same set of ISA_EXT registers are not present on all host because
> > +        * ISA_EXT registers are visible to the KVM user space based on the
> > +        * ISA extensions available on the host. Also, disabling an ISA
> > +        * extension using corresponding ISA_EXT register does not affect
> > +        * the visibility of the ISA_EXT register itself.
> > +        *
> > +        * Based on above, we should filter-out all ISA_EXT registers.
> >          */
> > -       switch (reg & ~REG_MASK) {
> > +       case KVM_REG_RISCV_ISA_EXT | KVM_RISCV_ISA_EXT_A:
> > +       case KVM_REG_RISCV_ISA_EXT | KVM_RISCV_ISA_EXT_C:
> > +       case KVM_REG_RISCV_ISA_EXT | KVM_RISCV_ISA_EXT_D:
> > +       case KVM_REG_RISCV_ISA_EXT | KVM_RISCV_ISA_EXT_F:
> > +       case KVM_REG_RISCV_ISA_EXT | KVM_RISCV_ISA_EXT_H:
> > +       case KVM_REG_RISCV_ISA_EXT | KVM_RISCV_ISA_EXT_I:
> > +       case KVM_REG_RISCV_ISA_EXT | KVM_RISCV_ISA_EXT_M:
> > +       case KVM_REG_RISCV_ISA_EXT | KVM_RISCV_ISA_EXT_SVPBMT:
> >         case KVM_REG_RISCV_ISA_EXT | KVM_RISCV_ISA_EXT_SSTC:
> >         case KVM_REG_RISCV_ISA_EXT | KVM_RISCV_ISA_EXT_SVINVAL:
> >         case KVM_REG_RISCV_ISA_EXT | KVM_RISCV_ISA_EXT_ZIHINTPAUSE:
> > +       case KVM_REG_RISCV_ISA_EXT | KVM_RISCV_ISA_EXT_ZICBOM:
> > +       case KVM_REG_RISCV_ISA_EXT | KVM_RISCV_ISA_EXT_ZICBOZ:
> >         case KVM_REG_RISCV_ISA_EXT | KVM_RISCV_ISA_EXT_ZBB:
> >         case KVM_REG_RISCV_ISA_EXT | KVM_RISCV_ISA_EXT_SSAIA:
> > +       case KVM_REG_RISCV_ISA_EXT | KVM_RISCV_ISA_EXT_V:
> > +       case KVM_REG_RISCV_ISA_EXT | KVM_RISCV_ISA_EXT_SVNAPOT:
> >         case KVM_REG_RISCV_ISA_EXT | KVM_RISCV_ISA_EXT_ZBA:
> >         case KVM_REG_RISCV_ISA_EXT | KVM_RISCV_ISA_EXT_ZBS:
> >         case KVM_REG_RISCV_ISA_EXT | KVM_RISCV_ISA_EXT_ZICNTR:
> > @@ -50,12 +66,7 @@ static inline bool vcpu_has_ext(struct kvm_vcpu *vcpu, int ext)
> >         unsigned long value;
> >
> >         ret = __vcpu_get_reg(vcpu, RISCV_ISA_EXT_REG(ext), &value);
> > -       if (ret) {
> > -               printf("Failed to get ext %d", ext);
> > -               return false;
> > -       }
> > -
> > -       return !!value;
> > +       return (ret) ? false : !!value;
> >  }
> >
> >  void finalize_vcpu(struct kvm_vcpu *vcpu, struct vcpu_reg_list *c)
> > @@ -506,10 +517,6 @@ static __u64 base_regs[] = {
> >         KVM_REG_RISCV | KVM_REG_SIZE_U64 | KVM_REG_RISCV_TIMER | KVM_REG_RISCV_TIMER_REG(time),
> >         KVM_REG_RISCV | KVM_REG_SIZE_U64 | KVM_REG_RISCV_TIMER | KVM_REG_RISCV_TIMER_REG(compare),
> >         KVM_REG_RISCV | KVM_REG_SIZE_U64 | KVM_REG_RISCV_TIMER | KVM_REG_RISCV_TIMER_REG(state),
> > -       KVM_REG_RISCV | KVM_REG_SIZE_ULONG | KVM_REG_RISCV_ISA_EXT | KVM_RISCV_ISA_EXT_A,
> > -       KVM_REG_RISCV | KVM_REG_SIZE_ULONG | KVM_REG_RISCV_ISA_EXT | KVM_RISCV_ISA_EXT_C,
> > -       KVM_REG_RISCV | KVM_REG_SIZE_ULONG | KVM_REG_RISCV_ISA_EXT | KVM_RISCV_ISA_EXT_I,
> > -       KVM_REG_RISCV | KVM_REG_SIZE_ULONG | KVM_REG_RISCV_ISA_EXT | KVM_RISCV_ISA_EXT_M,
> >         KVM_REG_RISCV | KVM_REG_SIZE_ULONG | KVM_REG_RISCV_SBI_EXT | KVM_REG_RISCV_SBI_SINGLE | KVM_RISCV_SBI_EXT_V01,
> >         KVM_REG_RISCV | KVM_REG_SIZE_ULONG | KVM_REG_RISCV_SBI_EXT | KVM_REG_RISCV_SBI_SINGLE | KVM_RISCV_SBI_EXT_TIME,
> >         KVM_REG_RISCV | KVM_REG_SIZE_ULONG | KVM_REG_RISCV_SBI_EXT | KVM_REG_RISCV_SBI_SINGLE | KVM_RISCV_SBI_EXT_IPI,
> > --
> > 2.34.1
> >
>
>
> --
> Regards,
> Atish

Regards,
Anup

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

* Re: [PATCH 3/4] KVM: riscv: selftests: Fix ISA_EXT register handling in get-reg-list
  2023-09-20 13:56     ` Anup Patel
@ 2023-09-20 23:01       ` Atish Patra
  2023-09-21  5:12         ` Anup Patel
  0 siblings, 1 reply; 21+ messages in thread
From: Atish Patra @ 2023-09-20 23:01 UTC (permalink / raw)
  To: Anup Patel
  Cc: Paolo Bonzini, Shuah Khan, Palmer Dabbelt, Paul Walmsley,
	Andrew Jones, kvm, kvm-riscv, linux-riscv, linux-kernel,
	linux-kselftest

On Wed, Sep 20, 2023 at 6:56 AM Anup Patel <apatel@ventanamicro.com> wrote:
>
> On Wed, Sep 20, 2023 at 1:24 AM Atish Patra <atishp@atishpatra.org> wrote:
> >
> > On Mon, Sep 18, 2023 at 11:07 AM Anup Patel <apatel@ventanamicro.com> wrote:
> > >
> > > Same set of ISA_EXT registers are not present on all host because
> > > ISA_EXT registers are visible to the KVM user space based on the
> > > ISA extensions available on the host. Also, disabling an ISA
> > > extension using corresponding ISA_EXT register does not affect
> > > the visibility of the ISA_EXT register itself.
> > >
> > > Based on the above, we should filter-out all ISA_EXT registers.
> > >
> >
> > In that case, we don't need the switch case any more. Just a
> > conditional check with KVM_RISCV_ISA_EXT_MAX should be sufficient.
>
> If we compare against KVM_RISCV_ISA_EXT_MAX then we will forget
> adding test configs for newer ISA extensions.
>

I feel it just bloats the code as we may end up in hundreds of
extensions in the future
given the state of the extension scheme.

> >
> > > Fixes: 477069398ed6 ("KVM: riscv: selftests: Add get-reg-list test")
> > > Signed-off-by: Anup Patel <apatel@ventanamicro.com>
> > > ---
> > >  .../selftests/kvm/riscv/get-reg-list.c        | 35 +++++++++++--------
> > >  1 file changed, 21 insertions(+), 14 deletions(-)
> > >
> > > diff --git a/tools/testing/selftests/kvm/riscv/get-reg-list.c b/tools/testing/selftests/kvm/riscv/get-reg-list.c
> > > index d8ecacd03ecf..76c0ad11e423 100644
> > > --- a/tools/testing/selftests/kvm/riscv/get-reg-list.c
> > > +++ b/tools/testing/selftests/kvm/riscv/get-reg-list.c
> > > @@ -14,17 +14,33 @@
> > >
> > >  bool filter_reg(__u64 reg)
> > >  {
> > > +       switch (reg & ~REG_MASK) {
> > >         /*
> > > -        * Some ISA extensions are optional and not present on all host,
> > > -        * but they can't be disabled through ISA_EXT registers when present.
> > > -        * So, to make life easy, just filtering out these kind of registers.
> > > +        * Same set of ISA_EXT registers are not present on all host because
> > > +        * ISA_EXT registers are visible to the KVM user space based on the
> > > +        * ISA extensions available on the host. Also, disabling an ISA
> > > +        * extension using corresponding ISA_EXT register does not affect
> > > +        * the visibility of the ISA_EXT register itself.
> > > +        *
> > > +        * Based on above, we should filter-out all ISA_EXT registers.
> > >          */
> > > -       switch (reg & ~REG_MASK) {
> > > +       case KVM_REG_RISCV_ISA_EXT | KVM_RISCV_ISA_EXT_A:
> > > +       case KVM_REG_RISCV_ISA_EXT | KVM_RISCV_ISA_EXT_C:
> > > +       case KVM_REG_RISCV_ISA_EXT | KVM_RISCV_ISA_EXT_D:
> > > +       case KVM_REG_RISCV_ISA_EXT | KVM_RISCV_ISA_EXT_F:
> > > +       case KVM_REG_RISCV_ISA_EXT | KVM_RISCV_ISA_EXT_H:
> > > +       case KVM_REG_RISCV_ISA_EXT | KVM_RISCV_ISA_EXT_I:
> > > +       case KVM_REG_RISCV_ISA_EXT | KVM_RISCV_ISA_EXT_M:
> > > +       case KVM_REG_RISCV_ISA_EXT | KVM_RISCV_ISA_EXT_SVPBMT:
> > >         case KVM_REG_RISCV_ISA_EXT | KVM_RISCV_ISA_EXT_SSTC:
> > >         case KVM_REG_RISCV_ISA_EXT | KVM_RISCV_ISA_EXT_SVINVAL:
> > >         case KVM_REG_RISCV_ISA_EXT | KVM_RISCV_ISA_EXT_ZIHINTPAUSE:
> > > +       case KVM_REG_RISCV_ISA_EXT | KVM_RISCV_ISA_EXT_ZICBOM:
> > > +       case KVM_REG_RISCV_ISA_EXT | KVM_RISCV_ISA_EXT_ZICBOZ:
> > >         case KVM_REG_RISCV_ISA_EXT | KVM_RISCV_ISA_EXT_ZBB:
> > >         case KVM_REG_RISCV_ISA_EXT | KVM_RISCV_ISA_EXT_SSAIA:
> > > +       case KVM_REG_RISCV_ISA_EXT | KVM_RISCV_ISA_EXT_V:
> > > +       case KVM_REG_RISCV_ISA_EXT | KVM_RISCV_ISA_EXT_SVNAPOT:
> > >         case KVM_REG_RISCV_ISA_EXT | KVM_RISCV_ISA_EXT_ZBA:
> > >         case KVM_REG_RISCV_ISA_EXT | KVM_RISCV_ISA_EXT_ZBS:
> > >         case KVM_REG_RISCV_ISA_EXT | KVM_RISCV_ISA_EXT_ZICNTR:
> > > @@ -50,12 +66,7 @@ static inline bool vcpu_has_ext(struct kvm_vcpu *vcpu, int ext)
> > >         unsigned long value;
> > >
> > >         ret = __vcpu_get_reg(vcpu, RISCV_ISA_EXT_REG(ext), &value);
> > > -       if (ret) {
> > > -               printf("Failed to get ext %d", ext);
> > > -               return false;
> > > -       }
> > > -
> > > -       return !!value;
> > > +       return (ret) ? false : !!value;
> > >  }
> > >
> > >  void finalize_vcpu(struct kvm_vcpu *vcpu, struct vcpu_reg_list *c)
> > > @@ -506,10 +517,6 @@ static __u64 base_regs[] = {
> > >         KVM_REG_RISCV | KVM_REG_SIZE_U64 | KVM_REG_RISCV_TIMER | KVM_REG_RISCV_TIMER_REG(time),
> > >         KVM_REG_RISCV | KVM_REG_SIZE_U64 | KVM_REG_RISCV_TIMER | KVM_REG_RISCV_TIMER_REG(compare),
> > >         KVM_REG_RISCV | KVM_REG_SIZE_U64 | KVM_REG_RISCV_TIMER | KVM_REG_RISCV_TIMER_REG(state),
> > > -       KVM_REG_RISCV | KVM_REG_SIZE_ULONG | KVM_REG_RISCV_ISA_EXT | KVM_RISCV_ISA_EXT_A,
> > > -       KVM_REG_RISCV | KVM_REG_SIZE_ULONG | KVM_REG_RISCV_ISA_EXT | KVM_RISCV_ISA_EXT_C,
> > > -       KVM_REG_RISCV | KVM_REG_SIZE_ULONG | KVM_REG_RISCV_ISA_EXT | KVM_RISCV_ISA_EXT_I,
> > > -       KVM_REG_RISCV | KVM_REG_SIZE_ULONG | KVM_REG_RISCV_ISA_EXT | KVM_RISCV_ISA_EXT_M,
> > >         KVM_REG_RISCV | KVM_REG_SIZE_ULONG | KVM_REG_RISCV_SBI_EXT | KVM_REG_RISCV_SBI_SINGLE | KVM_RISCV_SBI_EXT_V01,
> > >         KVM_REG_RISCV | KVM_REG_SIZE_ULONG | KVM_REG_RISCV_SBI_EXT | KVM_REG_RISCV_SBI_SINGLE | KVM_RISCV_SBI_EXT_TIME,
> > >         KVM_REG_RISCV | KVM_REG_SIZE_ULONG | KVM_REG_RISCV_SBI_EXT | KVM_REG_RISCV_SBI_SINGLE | KVM_RISCV_SBI_EXT_IPI,
> > > --
> > > 2.34.1
> > >
> >
> >
> > --
> > Regards,
> > Atish
>
> Regards,
> Anup



-- 
Regards,
Atish

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

* Re: [PATCH 3/4] KVM: riscv: selftests: Fix ISA_EXT register handling in get-reg-list
  2023-09-20 23:01       ` Atish Patra
@ 2023-09-21  5:12         ` Anup Patel
  0 siblings, 0 replies; 21+ messages in thread
From: Anup Patel @ 2023-09-21  5:12 UTC (permalink / raw)
  To: Atish Patra
  Cc: Paolo Bonzini, Shuah Khan, Palmer Dabbelt, Paul Walmsley,
	Andrew Jones, kvm, kvm-riscv, linux-riscv, linux-kernel,
	linux-kselftest

On Thu, Sep 21, 2023 at 4:31 AM Atish Patra <atishp@atishpatra.org> wrote:
>
> On Wed, Sep 20, 2023 at 6:56 AM Anup Patel <apatel@ventanamicro.com> wrote:
> >
> > On Wed, Sep 20, 2023 at 1:24 AM Atish Patra <atishp@atishpatra.org> wrote:
> > >
> > > On Mon, Sep 18, 2023 at 11:07 AM Anup Patel <apatel@ventanamicro.com> wrote:
> > > >
> > > > Same set of ISA_EXT registers are not present on all host because
> > > > ISA_EXT registers are visible to the KVM user space based on the
> > > > ISA extensions available on the host. Also, disabling an ISA
> > > > extension using corresponding ISA_EXT register does not affect
> > > > the visibility of the ISA_EXT register itself.
> > > >
> > > > Based on the above, we should filter-out all ISA_EXT registers.
> > > >
> > >
> > > In that case, we don't need the switch case any more. Just a
> > > conditional check with KVM_RISCV_ISA_EXT_MAX should be sufficient.
> >
> > If we compare against KVM_RISCV_ISA_EXT_MAX then we will forget
> > adding test configs for newer ISA extensions.
> >
>
> I feel it just bloats the code as we may end up in hundreds of
> extensions in the future
> given the state of the extension scheme.

That is bound to happen so eventually we will have to revisit the
get-reg-list test.

Regards,
Anup

>
> > >
> > > > Fixes: 477069398ed6 ("KVM: riscv: selftests: Add get-reg-list test")
> > > > Signed-off-by: Anup Patel <apatel@ventanamicro.com>
> > > > ---
> > > >  .../selftests/kvm/riscv/get-reg-list.c        | 35 +++++++++++--------
> > > >  1 file changed, 21 insertions(+), 14 deletions(-)
> > > >
> > > > diff --git a/tools/testing/selftests/kvm/riscv/get-reg-list.c b/tools/testing/selftests/kvm/riscv/get-reg-list.c
> > > > index d8ecacd03ecf..76c0ad11e423 100644
> > > > --- a/tools/testing/selftests/kvm/riscv/get-reg-list.c
> > > > +++ b/tools/testing/selftests/kvm/riscv/get-reg-list.c
> > > > @@ -14,17 +14,33 @@
> > > >
> > > >  bool filter_reg(__u64 reg)
> > > >  {
> > > > +       switch (reg & ~REG_MASK) {
> > > >         /*
> > > > -        * Some ISA extensions are optional and not present on all host,
> > > > -        * but they can't be disabled through ISA_EXT registers when present.
> > > > -        * So, to make life easy, just filtering out these kind of registers.
> > > > +        * Same set of ISA_EXT registers are not present on all host because
> > > > +        * ISA_EXT registers are visible to the KVM user space based on the
> > > > +        * ISA extensions available on the host. Also, disabling an ISA
> > > > +        * extension using corresponding ISA_EXT register does not affect
> > > > +        * the visibility of the ISA_EXT register itself.
> > > > +        *
> > > > +        * Based on above, we should filter-out all ISA_EXT registers.
> > > >          */
> > > > -       switch (reg & ~REG_MASK) {
> > > > +       case KVM_REG_RISCV_ISA_EXT | KVM_RISCV_ISA_EXT_A:
> > > > +       case KVM_REG_RISCV_ISA_EXT | KVM_RISCV_ISA_EXT_C:
> > > > +       case KVM_REG_RISCV_ISA_EXT | KVM_RISCV_ISA_EXT_D:
> > > > +       case KVM_REG_RISCV_ISA_EXT | KVM_RISCV_ISA_EXT_F:
> > > > +       case KVM_REG_RISCV_ISA_EXT | KVM_RISCV_ISA_EXT_H:
> > > > +       case KVM_REG_RISCV_ISA_EXT | KVM_RISCV_ISA_EXT_I:
> > > > +       case KVM_REG_RISCV_ISA_EXT | KVM_RISCV_ISA_EXT_M:
> > > > +       case KVM_REG_RISCV_ISA_EXT | KVM_RISCV_ISA_EXT_SVPBMT:
> > > >         case KVM_REG_RISCV_ISA_EXT | KVM_RISCV_ISA_EXT_SSTC:
> > > >         case KVM_REG_RISCV_ISA_EXT | KVM_RISCV_ISA_EXT_SVINVAL:
> > > >         case KVM_REG_RISCV_ISA_EXT | KVM_RISCV_ISA_EXT_ZIHINTPAUSE:
> > > > +       case KVM_REG_RISCV_ISA_EXT | KVM_RISCV_ISA_EXT_ZICBOM:
> > > > +       case KVM_REG_RISCV_ISA_EXT | KVM_RISCV_ISA_EXT_ZICBOZ:
> > > >         case KVM_REG_RISCV_ISA_EXT | KVM_RISCV_ISA_EXT_ZBB:
> > > >         case KVM_REG_RISCV_ISA_EXT | KVM_RISCV_ISA_EXT_SSAIA:
> > > > +       case KVM_REG_RISCV_ISA_EXT | KVM_RISCV_ISA_EXT_V:
> > > > +       case KVM_REG_RISCV_ISA_EXT | KVM_RISCV_ISA_EXT_SVNAPOT:
> > > >         case KVM_REG_RISCV_ISA_EXT | KVM_RISCV_ISA_EXT_ZBA:
> > > >         case KVM_REG_RISCV_ISA_EXT | KVM_RISCV_ISA_EXT_ZBS:
> > > >         case KVM_REG_RISCV_ISA_EXT | KVM_RISCV_ISA_EXT_ZICNTR:
> > > > @@ -50,12 +66,7 @@ static inline bool vcpu_has_ext(struct kvm_vcpu *vcpu, int ext)
> > > >         unsigned long value;
> > > >
> > > >         ret = __vcpu_get_reg(vcpu, RISCV_ISA_EXT_REG(ext), &value);
> > > > -       if (ret) {
> > > > -               printf("Failed to get ext %d", ext);
> > > > -               return false;
> > > > -       }
> > > > -
> > > > -       return !!value;
> > > > +       return (ret) ? false : !!value;
> > > >  }
> > > >
> > > >  void finalize_vcpu(struct kvm_vcpu *vcpu, struct vcpu_reg_list *c)
> > > > @@ -506,10 +517,6 @@ static __u64 base_regs[] = {
> > > >         KVM_REG_RISCV | KVM_REG_SIZE_U64 | KVM_REG_RISCV_TIMER | KVM_REG_RISCV_TIMER_REG(time),
> > > >         KVM_REG_RISCV | KVM_REG_SIZE_U64 | KVM_REG_RISCV_TIMER | KVM_REG_RISCV_TIMER_REG(compare),
> > > >         KVM_REG_RISCV | KVM_REG_SIZE_U64 | KVM_REG_RISCV_TIMER | KVM_REG_RISCV_TIMER_REG(state),
> > > > -       KVM_REG_RISCV | KVM_REG_SIZE_ULONG | KVM_REG_RISCV_ISA_EXT | KVM_RISCV_ISA_EXT_A,
> > > > -       KVM_REG_RISCV | KVM_REG_SIZE_ULONG | KVM_REG_RISCV_ISA_EXT | KVM_RISCV_ISA_EXT_C,
> > > > -       KVM_REG_RISCV | KVM_REG_SIZE_ULONG | KVM_REG_RISCV_ISA_EXT | KVM_RISCV_ISA_EXT_I,
> > > > -       KVM_REG_RISCV | KVM_REG_SIZE_ULONG | KVM_REG_RISCV_ISA_EXT | KVM_RISCV_ISA_EXT_M,
> > > >         KVM_REG_RISCV | KVM_REG_SIZE_ULONG | KVM_REG_RISCV_SBI_EXT | KVM_REG_RISCV_SBI_SINGLE | KVM_RISCV_SBI_EXT_V01,
> > > >         KVM_REG_RISCV | KVM_REG_SIZE_ULONG | KVM_REG_RISCV_SBI_EXT | KVM_REG_RISCV_SBI_SINGLE | KVM_RISCV_SBI_EXT_TIME,
> > > >         KVM_REG_RISCV | KVM_REG_SIZE_ULONG | KVM_REG_RISCV_SBI_EXT | KVM_REG_RISCV_SBI_SINGLE | KVM_RISCV_SBI_EXT_IPI,
> > > > --
> > > > 2.34.1
> > > >
> > >
> > >
> > > --
> > > Regards,
> > > Atish
> >
> > Regards,
> > Anup
>
>
>
> --
> Regards,
> Atish

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

* Re: [PATCH 3/4] KVM: riscv: selftests: Fix ISA_EXT register handling in get-reg-list
  2023-09-18 18:06 ` [PATCH 3/4] KVM: riscv: selftests: Fix ISA_EXT register handling in get-reg-list Anup Patel
  2023-09-19 19:54   ` Atish Patra
@ 2023-09-20  5:13   ` Andrew Jones
  1 sibling, 0 replies; 21+ messages in thread
From: Andrew Jones @ 2023-09-20  5:13 UTC (permalink / raw)
  To: Anup Patel
  Cc: Paolo Bonzini, Atish Patra, Shuah Khan, Palmer Dabbelt,
	Paul Walmsley, kvm, kvm-riscv, linux-riscv, linux-kernel,
	linux-kselftest

On Mon, Sep 18, 2023 at 11:36:45PM +0530, Anup Patel wrote:
> Same set of ISA_EXT registers are not present on all host because
> ISA_EXT registers are visible to the KVM user space based on the
> ISA extensions available on the host. Also, disabling an ISA
> extension using corresponding ISA_EXT register does not affect
> the visibility of the ISA_EXT register itself.
> 
> Based on the above, we should filter-out all ISA_EXT registers.
> 
> Fixes: 477069398ed6 ("KVM: riscv: selftests: Add get-reg-list test")
> Signed-off-by: Anup Patel <apatel@ventanamicro.com>
> ---
>  .../selftests/kvm/riscv/get-reg-list.c        | 35 +++++++++++--------
>  1 file changed, 21 insertions(+), 14 deletions(-)
> 
> diff --git a/tools/testing/selftests/kvm/riscv/get-reg-list.c b/tools/testing/selftests/kvm/riscv/get-reg-list.c
> index d8ecacd03ecf..76c0ad11e423 100644
> --- a/tools/testing/selftests/kvm/riscv/get-reg-list.c
> +++ b/tools/testing/selftests/kvm/riscv/get-reg-list.c
> @@ -14,17 +14,33 @@
>  
>  bool filter_reg(__u64 reg)
>  {
> +	switch (reg & ~REG_MASK) {
>  	/*
> -	 * Some ISA extensions are optional and not present on all host,
> -	 * but they can't be disabled through ISA_EXT registers when present.
> -	 * So, to make life easy, just filtering out these kind of registers.
> +	 * Same set of ISA_EXT registers are not present on all host because
> +	 * ISA_EXT registers are visible to the KVM user space based on the
> +	 * ISA extensions available on the host. Also, disabling an ISA
> +	 * extension using corresponding ISA_EXT register does not affect
> +	 * the visibility of the ISA_EXT register itself.
> +	 *
> +	 * Based on above, we should filter-out all ISA_EXT registers.
>  	 */
> -	switch (reg & ~REG_MASK) {
> +	case KVM_REG_RISCV_ISA_EXT | KVM_RISCV_ISA_EXT_A:
> +	case KVM_REG_RISCV_ISA_EXT | KVM_RISCV_ISA_EXT_C:
> +	case KVM_REG_RISCV_ISA_EXT | KVM_RISCV_ISA_EXT_D:
> +	case KVM_REG_RISCV_ISA_EXT | KVM_RISCV_ISA_EXT_F:
> +	case KVM_REG_RISCV_ISA_EXT | KVM_RISCV_ISA_EXT_H:
> +	case KVM_REG_RISCV_ISA_EXT | KVM_RISCV_ISA_EXT_I:
> +	case KVM_REG_RISCV_ISA_EXT | KVM_RISCV_ISA_EXT_M:
> +	case KVM_REG_RISCV_ISA_EXT | KVM_RISCV_ISA_EXT_SVPBMT:
>  	case KVM_REG_RISCV_ISA_EXT | KVM_RISCV_ISA_EXT_SSTC:
>  	case KVM_REG_RISCV_ISA_EXT | KVM_RISCV_ISA_EXT_SVINVAL:
>  	case KVM_REG_RISCV_ISA_EXT | KVM_RISCV_ISA_EXT_ZIHINTPAUSE:
> +	case KVM_REG_RISCV_ISA_EXT | KVM_RISCV_ISA_EXT_ZICBOM:
> +	case KVM_REG_RISCV_ISA_EXT | KVM_RISCV_ISA_EXT_ZICBOZ:
>  	case KVM_REG_RISCV_ISA_EXT | KVM_RISCV_ISA_EXT_ZBB:
>  	case KVM_REG_RISCV_ISA_EXT | KVM_RISCV_ISA_EXT_SSAIA:
> +	case KVM_REG_RISCV_ISA_EXT | KVM_RISCV_ISA_EXT_V:
> +	case KVM_REG_RISCV_ISA_EXT | KVM_RISCV_ISA_EXT_SVNAPOT:
>  	case KVM_REG_RISCV_ISA_EXT | KVM_RISCV_ISA_EXT_ZBA:
>  	case KVM_REG_RISCV_ISA_EXT | KVM_RISCV_ISA_EXT_ZBS:
>  	case KVM_REG_RISCV_ISA_EXT | KVM_RISCV_ISA_EXT_ZICNTR:
> @@ -50,12 +66,7 @@ static inline bool vcpu_has_ext(struct kvm_vcpu *vcpu, int ext)
>  	unsigned long value;
>  
>  	ret = __vcpu_get_reg(vcpu, RISCV_ISA_EXT_REG(ext), &value);
> -	if (ret) {
> -		printf("Failed to get ext %d", ext);
> -		return false;
> -	}
> -
> -	return !!value;
> +	return (ret) ? false : !!value;

This is an unrelated change, but OK. It's consistent with the plan[1] we
have on the timer test series

[1] https://lore.kernel.org/all/20230914-d6645bbc5ac80999674e9685@orel/

>  }
>  
>  void finalize_vcpu(struct kvm_vcpu *vcpu, struct vcpu_reg_list *c)
> @@ -506,10 +517,6 @@ static __u64 base_regs[] = {
>  	KVM_REG_RISCV | KVM_REG_SIZE_U64 | KVM_REG_RISCV_TIMER | KVM_REG_RISCV_TIMER_REG(time),
>  	KVM_REG_RISCV | KVM_REG_SIZE_U64 | KVM_REG_RISCV_TIMER | KVM_REG_RISCV_TIMER_REG(compare),
>  	KVM_REG_RISCV | KVM_REG_SIZE_U64 | KVM_REG_RISCV_TIMER | KVM_REG_RISCV_TIMER_REG(state),
> -	KVM_REG_RISCV | KVM_REG_SIZE_ULONG | KVM_REG_RISCV_ISA_EXT | KVM_RISCV_ISA_EXT_A,
> -	KVM_REG_RISCV | KVM_REG_SIZE_ULONG | KVM_REG_RISCV_ISA_EXT | KVM_RISCV_ISA_EXT_C,
> -	KVM_REG_RISCV | KVM_REG_SIZE_ULONG | KVM_REG_RISCV_ISA_EXT | KVM_RISCV_ISA_EXT_I,
> -	KVM_REG_RISCV | KVM_REG_SIZE_ULONG | KVM_REG_RISCV_ISA_EXT | KVM_RISCV_ISA_EXT_M,
>  	KVM_REG_RISCV | KVM_REG_SIZE_ULONG | KVM_REG_RISCV_SBI_EXT | KVM_REG_RISCV_SBI_SINGLE | KVM_RISCV_SBI_EXT_V01,
>  	KVM_REG_RISCV | KVM_REG_SIZE_ULONG | KVM_REG_RISCV_SBI_EXT | KVM_REG_RISCV_SBI_SINGLE | KVM_RISCV_SBI_EXT_TIME,
>  	KVM_REG_RISCV | KVM_REG_SIZE_ULONG | KVM_REG_RISCV_SBI_EXT | KVM_REG_RISCV_SBI_SINGLE | KVM_RISCV_SBI_EXT_IPI,
> -- 
> 2.34.1
>

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

Thanks,
drew

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

* [PATCH 4/4] KVM: riscv: selftests: Selectively filter-out AIA registers
  2023-09-18 18:06 [PATCH 0/4] KVM RISC-V fixes for ONE_REG interface Anup Patel
                   ` (2 preceding siblings ...)
  2023-09-18 18:06 ` [PATCH 3/4] KVM: riscv: selftests: Fix ISA_EXT register handling in get-reg-list Anup Patel
@ 2023-09-18 18:06 ` Anup Patel
  2023-09-19 20:12   ` Atish Patra
  2023-09-20  5:24   ` Andrew Jones
  3 siblings, 2 replies; 21+ messages in thread
From: Anup Patel @ 2023-09-18 18:06 UTC (permalink / raw)
  To: Paolo Bonzini, Atish Patra, Shuah Khan
  Cc: Palmer Dabbelt, Paul Walmsley, Andrew Jones, kvm, kvm-riscv,
	linux-riscv, linux-kernel, linux-kselftest, Anup Patel

Currently the AIA ONE_REG registers are reported by get-reg-list
as new registers for various vcpu_reg_list configs whenever Ssaia
is available on the host because Ssaia extension can only be
disabled by Smstateen extension which is not always available.

To tackle this, we should filter-out AIA ONE_REG registers only
when Ssaia can't be disabled for a VCPU.

Fixes: 477069398ed6 ("KVM: riscv: selftests: Add get-reg-list test")
Signed-off-by: Anup Patel <apatel@ventanamicro.com>
---
 .../selftests/kvm/riscv/get-reg-list.c        | 23 +++++++++++++++++--
 1 file changed, 21 insertions(+), 2 deletions(-)

diff --git a/tools/testing/selftests/kvm/riscv/get-reg-list.c b/tools/testing/selftests/kvm/riscv/get-reg-list.c
index 76c0ad11e423..85907c86b835 100644
--- a/tools/testing/selftests/kvm/riscv/get-reg-list.c
+++ b/tools/testing/selftests/kvm/riscv/get-reg-list.c
@@ -12,6 +12,8 @@
 
 #define REG_MASK (KVM_REG_ARCH_MASK | KVM_REG_SIZE_MASK)
 
+static bool isa_ext_cant_disable[KVM_RISCV_ISA_EXT_MAX];
+
 bool filter_reg(__u64 reg)
 {
 	switch (reg & ~REG_MASK) {
@@ -48,6 +50,15 @@ bool filter_reg(__u64 reg)
 	case KVM_REG_RISCV_ISA_EXT | KVM_RISCV_ISA_EXT_ZIFENCEI:
 	case KVM_REG_RISCV_ISA_EXT | KVM_RISCV_ISA_EXT_ZIHPM:
 		return true;
+	/* AIA registers are always available when Ssaia can't be disabled */
+	case KVM_REG_RISCV_CSR | KVM_REG_RISCV_CSR_AIA | KVM_REG_RISCV_CSR_AIA_REG(siselect):
+	case KVM_REG_RISCV_CSR | KVM_REG_RISCV_CSR_AIA | KVM_REG_RISCV_CSR_AIA_REG(iprio1):
+	case KVM_REG_RISCV_CSR | KVM_REG_RISCV_CSR_AIA | KVM_REG_RISCV_CSR_AIA_REG(iprio2):
+	case KVM_REG_RISCV_CSR | KVM_REG_RISCV_CSR_AIA | KVM_REG_RISCV_CSR_AIA_REG(sieh):
+	case KVM_REG_RISCV_CSR | KVM_REG_RISCV_CSR_AIA | KVM_REG_RISCV_CSR_AIA_REG(siph):
+	case KVM_REG_RISCV_CSR | KVM_REG_RISCV_CSR_AIA | KVM_REG_RISCV_CSR_AIA_REG(iprio1h):
+	case KVM_REG_RISCV_CSR | KVM_REG_RISCV_CSR_AIA | KVM_REG_RISCV_CSR_AIA_REG(iprio2h):
+		return isa_ext_cant_disable[KVM_RISCV_ISA_EXT_SSAIA] ? true : false;
 	default:
 		break;
 	}
@@ -71,14 +82,22 @@ static inline bool vcpu_has_ext(struct kvm_vcpu *vcpu, int ext)
 
 void finalize_vcpu(struct kvm_vcpu *vcpu, struct vcpu_reg_list *c)
 {
+	int rc;
 	struct vcpu_reg_sublist *s;
+	unsigned long isa_ext_state[KVM_RISCV_ISA_EXT_MAX] = { 0 };
+
+	for (int i = 0; i < KVM_RISCV_ISA_EXT_MAX; i++)
+		__vcpu_get_reg(vcpu, RISCV_ISA_EXT_REG(i), &isa_ext_state[i]);
 
 	/*
 	 * Disable all extensions which were enabled by default
 	 * if they were available in the risc-v host.
 	 */
-	for (int i = 0; i < KVM_RISCV_ISA_EXT_MAX; i++)
-		__vcpu_set_reg(vcpu, RISCV_ISA_EXT_REG(i), 0);
+	for (int i = 0; i < KVM_RISCV_ISA_EXT_MAX; i++) {
+		rc = __vcpu_set_reg(vcpu, RISCV_ISA_EXT_REG(i), 0);
+		if (rc && isa_ext_state[i])
+			isa_ext_cant_disable[i] = true;
+	}
 
 	for_each_sublist(c, s) {
 		if (!s->feature)
-- 
2.34.1


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

* Re: [PATCH 4/4] KVM: riscv: selftests: Selectively filter-out AIA registers
  2023-09-18 18:06 ` [PATCH 4/4] KVM: riscv: selftests: Selectively filter-out AIA registers Anup Patel
@ 2023-09-19 20:12   ` Atish Patra
  2023-09-20  4:48     ` Andrew Jones
  2023-09-20 13:51     ` Anup Patel
  2023-09-20  5:24   ` Andrew Jones
  1 sibling, 2 replies; 21+ messages in thread
From: Atish Patra @ 2023-09-19 20:12 UTC (permalink / raw)
  To: Anup Patel
  Cc: Paolo Bonzini, Shuah Khan, Palmer Dabbelt, Paul Walmsley,
	Andrew Jones, kvm, kvm-riscv, linux-riscv, linux-kernel,
	linux-kselftest

On Mon, Sep 18, 2023 at 11:07 AM Anup Patel <apatel@ventanamicro.com> wrote:
>
> Currently the AIA ONE_REG registers are reported by get-reg-list
> as new registers for various vcpu_reg_list configs whenever Ssaia
> is available on the host because Ssaia extension can only be
> disabled by Smstateen extension which is not always available.
>
> To tackle this, we should filter-out AIA ONE_REG registers only
> when Ssaia can't be disabled for a VCPU.
>
> Fixes: 477069398ed6 ("KVM: riscv: selftests: Add get-reg-list test")
> Signed-off-by: Anup Patel <apatel@ventanamicro.com>
> ---
>  .../selftests/kvm/riscv/get-reg-list.c        | 23 +++++++++++++++++--
>  1 file changed, 21 insertions(+), 2 deletions(-)
>
> diff --git a/tools/testing/selftests/kvm/riscv/get-reg-list.c b/tools/testing/selftests/kvm/riscv/get-reg-list.c
> index 76c0ad11e423..85907c86b835 100644
> --- a/tools/testing/selftests/kvm/riscv/get-reg-list.c
> +++ b/tools/testing/selftests/kvm/riscv/get-reg-list.c
> @@ -12,6 +12,8 @@
>
>  #define REG_MASK (KVM_REG_ARCH_MASK | KVM_REG_SIZE_MASK)
>
> +static bool isa_ext_cant_disable[KVM_RISCV_ISA_EXT_MAX];
> +
>  bool filter_reg(__u64 reg)
>  {
>         switch (reg & ~REG_MASK) {
> @@ -48,6 +50,15 @@ bool filter_reg(__u64 reg)
>         case KVM_REG_RISCV_ISA_EXT | KVM_RISCV_ISA_EXT_ZIFENCEI:
>         case KVM_REG_RISCV_ISA_EXT | KVM_RISCV_ISA_EXT_ZIHPM:
>                 return true;
> +       /* AIA registers are always available when Ssaia can't be disabled */
> +       case KVM_REG_RISCV_CSR | KVM_REG_RISCV_CSR_AIA | KVM_REG_RISCV_CSR_AIA_REG(siselect):
> +       case KVM_REG_RISCV_CSR | KVM_REG_RISCV_CSR_AIA | KVM_REG_RISCV_CSR_AIA_REG(iprio1):
> +       case KVM_REG_RISCV_CSR | KVM_REG_RISCV_CSR_AIA | KVM_REG_RISCV_CSR_AIA_REG(iprio2):
> +       case KVM_REG_RISCV_CSR | KVM_REG_RISCV_CSR_AIA | KVM_REG_RISCV_CSR_AIA_REG(sieh):
> +       case KVM_REG_RISCV_CSR | KVM_REG_RISCV_CSR_AIA | KVM_REG_RISCV_CSR_AIA_REG(siph):
> +       case KVM_REG_RISCV_CSR | KVM_REG_RISCV_CSR_AIA | KVM_REG_RISCV_CSR_AIA_REG(iprio1h):
> +       case KVM_REG_RISCV_CSR | KVM_REG_RISCV_CSR_AIA | KVM_REG_RISCV_CSR_AIA_REG(iprio2h):
> +               return isa_ext_cant_disable[KVM_RISCV_ISA_EXT_SSAIA] ? true : false;

Ahh I guess. you do need the switch case for AIA CSRs but for ISA
extensions can be avoided as it is contiguous.

>         default:
>                 break;
>         }
> @@ -71,14 +82,22 @@ static inline bool vcpu_has_ext(struct kvm_vcpu *vcpu, int ext)
>
>  void finalize_vcpu(struct kvm_vcpu *vcpu, struct vcpu_reg_list *c)
>  {
> +       int rc;
>         struct vcpu_reg_sublist *s;
> +       unsigned long isa_ext_state[KVM_RISCV_ISA_EXT_MAX] = { 0 };
> +
> +       for (int i = 0; i < KVM_RISCV_ISA_EXT_MAX; i++)
> +               __vcpu_get_reg(vcpu, RISCV_ISA_EXT_REG(i), &isa_ext_state[i]);
>
>         /*
>          * Disable all extensions which were enabled by default
>          * if they were available in the risc-v host.
>          */
> -       for (int i = 0; i < KVM_RISCV_ISA_EXT_MAX; i++)
> -               __vcpu_set_reg(vcpu, RISCV_ISA_EXT_REG(i), 0);
> +       for (int i = 0; i < KVM_RISCV_ISA_EXT_MAX; i++) {
> +               rc = __vcpu_set_reg(vcpu, RISCV_ISA_EXT_REG(i), 0);
> +               if (rc && isa_ext_state[i])
> +                       isa_ext_cant_disable[i] = true;
> +       }
>
>         for_each_sublist(c, s) {
>                 if (!s->feature)
> --
> 2.34.1
>

Otherwise, LGTM.

Reviewed-by: Atish Patra <atishp@rivosinc.com>

-- 
Regards,
Atish

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

* Re: [PATCH 4/4] KVM: riscv: selftests: Selectively filter-out AIA registers
  2023-09-19 20:12   ` Atish Patra
@ 2023-09-20  4:48     ` Andrew Jones
  2023-09-20  5:26       ` Andrew Jones
  2023-09-20 13:51     ` Anup Patel
  1 sibling, 1 reply; 21+ messages in thread
From: Andrew Jones @ 2023-09-20  4:48 UTC (permalink / raw)
  To: Atish Patra
  Cc: Anup Patel, Paolo Bonzini, Shuah Khan, Palmer Dabbelt,
	Paul Walmsley, kvm, kvm-riscv, linux-riscv, linux-kernel,
	linux-kselftest

On Tue, Sep 19, 2023 at 01:12:47PM -0700, Atish Patra wrote:
> On Mon, Sep 18, 2023 at 11:07 AM Anup Patel <apatel@ventanamicro.com> wrote:
> >
> > Currently the AIA ONE_REG registers are reported by get-reg-list
> > as new registers for various vcpu_reg_list configs whenever Ssaia
> > is available on the host because Ssaia extension can only be
> > disabled by Smstateen extension which is not always available.
> >
> > To tackle this, we should filter-out AIA ONE_REG registers only
> > when Ssaia can't be disabled for a VCPU.
> >
> > Fixes: 477069398ed6 ("KVM: riscv: selftests: Add get-reg-list test")
> > Signed-off-by: Anup Patel <apatel@ventanamicro.com>
> > ---
> >  .../selftests/kvm/riscv/get-reg-list.c        | 23 +++++++++++++++++--
> >  1 file changed, 21 insertions(+), 2 deletions(-)
> >
> > diff --git a/tools/testing/selftests/kvm/riscv/get-reg-list.c b/tools/testing/selftests/kvm/riscv/get-reg-list.c
> > index 76c0ad11e423..85907c86b835 100644
> > --- a/tools/testing/selftests/kvm/riscv/get-reg-list.c
> > +++ b/tools/testing/selftests/kvm/riscv/get-reg-list.c
> > @@ -12,6 +12,8 @@
> >
> >  #define REG_MASK (KVM_REG_ARCH_MASK | KVM_REG_SIZE_MASK)
> >
> > +static bool isa_ext_cant_disable[KVM_RISCV_ISA_EXT_MAX];
> > +
> >  bool filter_reg(__u64 reg)
> >  {
> >         switch (reg & ~REG_MASK) {
> > @@ -48,6 +50,15 @@ bool filter_reg(__u64 reg)
> >         case KVM_REG_RISCV_ISA_EXT | KVM_RISCV_ISA_EXT_ZIFENCEI:
> >         case KVM_REG_RISCV_ISA_EXT | KVM_RISCV_ISA_EXT_ZIHPM:
> >                 return true;
> > +       /* AIA registers are always available when Ssaia can't be disabled */
> > +       case KVM_REG_RISCV_CSR | KVM_REG_RISCV_CSR_AIA | KVM_REG_RISCV_CSR_AIA_REG(siselect):
> > +       case KVM_REG_RISCV_CSR | KVM_REG_RISCV_CSR_AIA | KVM_REG_RISCV_CSR_AIA_REG(iprio1):
> > +       case KVM_REG_RISCV_CSR | KVM_REG_RISCV_CSR_AIA | KVM_REG_RISCV_CSR_AIA_REG(iprio2):
> > +       case KVM_REG_RISCV_CSR | KVM_REG_RISCV_CSR_AIA | KVM_REG_RISCV_CSR_AIA_REG(sieh):
> > +       case KVM_REG_RISCV_CSR | KVM_REG_RISCV_CSR_AIA | KVM_REG_RISCV_CSR_AIA_REG(siph):
> > +       case KVM_REG_RISCV_CSR | KVM_REG_RISCV_CSR_AIA | KVM_REG_RISCV_CSR_AIA_REG(iprio1h):
> > +       case KVM_REG_RISCV_CSR | KVM_REG_RISCV_CSR_AIA | KVM_REG_RISCV_CSR_AIA_REG(iprio2h):
> > +               return isa_ext_cant_disable[KVM_RISCV_ISA_EXT_SSAIA] ? true : false;
> 
> Ahh I guess. you do need the switch case for AIA CSRs but for ISA
> extensions can be avoided as it is contiguous.

I guess we could so something like

case KVM_REG_RISCV_ISA_EXT ... KVM_REG_RISCV_ISA_EXT | KVM_RISCV_ISA_EXT_MAX:

for the ISA extensions.

Thanks,
drew

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

* Re: [PATCH 4/4] KVM: riscv: selftests: Selectively filter-out AIA registers
  2023-09-20  4:48     ` Andrew Jones
@ 2023-09-20  5:26       ` Andrew Jones
  0 siblings, 0 replies; 21+ messages in thread
From: Andrew Jones @ 2023-09-20  5:26 UTC (permalink / raw)
  To: Atish Patra
  Cc: Anup Patel, Paolo Bonzini, Shuah Khan, Palmer Dabbelt,
	Paul Walmsley, kvm, kvm-riscv, linux-riscv, linux-kernel,
	linux-kselftest

On Wed, Sep 20, 2023 at 06:48:11AM +0200, Andrew Jones wrote:
> On Tue, Sep 19, 2023 at 01:12:47PM -0700, Atish Patra wrote:
> > On Mon, Sep 18, 2023 at 11:07 AM Anup Patel <apatel@ventanamicro.com> wrote:
> > >
> > > Currently the AIA ONE_REG registers are reported by get-reg-list
> > > as new registers for various vcpu_reg_list configs whenever Ssaia
> > > is available on the host because Ssaia extension can only be
> > > disabled by Smstateen extension which is not always available.
> > >
> > > To tackle this, we should filter-out AIA ONE_REG registers only
> > > when Ssaia can't be disabled for a VCPU.
> > >
> > > Fixes: 477069398ed6 ("KVM: riscv: selftests: Add get-reg-list test")
> > > Signed-off-by: Anup Patel <apatel@ventanamicro.com>
> > > ---
> > >  .../selftests/kvm/riscv/get-reg-list.c        | 23 +++++++++++++++++--
> > >  1 file changed, 21 insertions(+), 2 deletions(-)
> > >
> > > diff --git a/tools/testing/selftests/kvm/riscv/get-reg-list.c b/tools/testing/selftests/kvm/riscv/get-reg-list.c
> > > index 76c0ad11e423..85907c86b835 100644
> > > --- a/tools/testing/selftests/kvm/riscv/get-reg-list.c
> > > +++ b/tools/testing/selftests/kvm/riscv/get-reg-list.c
> > > @@ -12,6 +12,8 @@
> > >
> > >  #define REG_MASK (KVM_REG_ARCH_MASK | KVM_REG_SIZE_MASK)
> > >
> > > +static bool isa_ext_cant_disable[KVM_RISCV_ISA_EXT_MAX];
> > > +
> > >  bool filter_reg(__u64 reg)
> > >  {
> > >         switch (reg & ~REG_MASK) {
> > > @@ -48,6 +50,15 @@ bool filter_reg(__u64 reg)
> > >         case KVM_REG_RISCV_ISA_EXT | KVM_RISCV_ISA_EXT_ZIFENCEI:
> > >         case KVM_REG_RISCV_ISA_EXT | KVM_RISCV_ISA_EXT_ZIHPM:
> > >                 return true;
> > > +       /* AIA registers are always available when Ssaia can't be disabled */
> > > +       case KVM_REG_RISCV_CSR | KVM_REG_RISCV_CSR_AIA | KVM_REG_RISCV_CSR_AIA_REG(siselect):
> > > +       case KVM_REG_RISCV_CSR | KVM_REG_RISCV_CSR_AIA | KVM_REG_RISCV_CSR_AIA_REG(iprio1):
> > > +       case KVM_REG_RISCV_CSR | KVM_REG_RISCV_CSR_AIA | KVM_REG_RISCV_CSR_AIA_REG(iprio2):
> > > +       case KVM_REG_RISCV_CSR | KVM_REG_RISCV_CSR_AIA | KVM_REG_RISCV_CSR_AIA_REG(sieh):
> > > +       case KVM_REG_RISCV_CSR | KVM_REG_RISCV_CSR_AIA | KVM_REG_RISCV_CSR_AIA_REG(siph):
> > > +       case KVM_REG_RISCV_CSR | KVM_REG_RISCV_CSR_AIA | KVM_REG_RISCV_CSR_AIA_REG(iprio1h):
> > > +       case KVM_REG_RISCV_CSR | KVM_REG_RISCV_CSR_AIA | KVM_REG_RISCV_CSR_AIA_REG(iprio2h):
> > > +               return isa_ext_cant_disable[KVM_RISCV_ISA_EXT_SSAIA] ? true : false;
> > 
> > Ahh I guess. you do need the switch case for AIA CSRs but for ISA
> > extensions can be avoided as it is contiguous.
> 
> I guess we could so something like
> 
> case KVM_REG_RISCV_ISA_EXT ... KVM_REG_RISCV_ISA_EXT | KVM_RISCV_ISA_EXT_MAX:
> 
> for the ISA extensions.
>

On second thought, I think I like them each listed explicitly. When we add
a new extension it will show up in the new register list, which will not
only remind us that it needs to be filtered, but also that we should
probably also create a specific config for it.

Thanks,
drew

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

* Re: [PATCH 4/4] KVM: riscv: selftests: Selectively filter-out AIA registers
  2023-09-19 20:12   ` Atish Patra
  2023-09-20  4:48     ` Andrew Jones
@ 2023-09-20 13:51     ` Anup Patel
  1 sibling, 0 replies; 21+ messages in thread
From: Anup Patel @ 2023-09-20 13:51 UTC (permalink / raw)
  To: Atish Patra
  Cc: Paolo Bonzini, Shuah Khan, Palmer Dabbelt, Paul Walmsley,
	Andrew Jones, kvm, kvm-riscv, linux-riscv, linux-kernel,
	linux-kselftest

On Wed, Sep 20, 2023 at 1:43 AM Atish Patra <atishp@atishpatra.org> wrote:
>
> On Mon, Sep 18, 2023 at 11:07 AM Anup Patel <apatel@ventanamicro.com> wrote:
> >
> > Currently the AIA ONE_REG registers are reported by get-reg-list
> > as new registers for various vcpu_reg_list configs whenever Ssaia
> > is available on the host because Ssaia extension can only be
> > disabled by Smstateen extension which is not always available.
> >
> > To tackle this, we should filter-out AIA ONE_REG registers only
> > when Ssaia can't be disabled for a VCPU.
> >
> > Fixes: 477069398ed6 ("KVM: riscv: selftests: Add get-reg-list test")
> > Signed-off-by: Anup Patel <apatel@ventanamicro.com>
> > ---
> >  .../selftests/kvm/riscv/get-reg-list.c        | 23 +++++++++++++++++--
> >  1 file changed, 21 insertions(+), 2 deletions(-)
> >
> > diff --git a/tools/testing/selftests/kvm/riscv/get-reg-list.c b/tools/testing/selftests/kvm/riscv/get-reg-list.c
> > index 76c0ad11e423..85907c86b835 100644
> > --- a/tools/testing/selftests/kvm/riscv/get-reg-list.c
> > +++ b/tools/testing/selftests/kvm/riscv/get-reg-list.c
> > @@ -12,6 +12,8 @@
> >
> >  #define REG_MASK (KVM_REG_ARCH_MASK | KVM_REG_SIZE_MASK)
> >
> > +static bool isa_ext_cant_disable[KVM_RISCV_ISA_EXT_MAX];
> > +
> >  bool filter_reg(__u64 reg)
> >  {
> >         switch (reg & ~REG_MASK) {
> > @@ -48,6 +50,15 @@ bool filter_reg(__u64 reg)
> >         case KVM_REG_RISCV_ISA_EXT | KVM_RISCV_ISA_EXT_ZIFENCEI:
> >         case KVM_REG_RISCV_ISA_EXT | KVM_RISCV_ISA_EXT_ZIHPM:
> >                 return true;
> > +       /* AIA registers are always available when Ssaia can't be disabled */
> > +       case KVM_REG_RISCV_CSR | KVM_REG_RISCV_CSR_AIA | KVM_REG_RISCV_CSR_AIA_REG(siselect):
> > +       case KVM_REG_RISCV_CSR | KVM_REG_RISCV_CSR_AIA | KVM_REG_RISCV_CSR_AIA_REG(iprio1):
> > +       case KVM_REG_RISCV_CSR | KVM_REG_RISCV_CSR_AIA | KVM_REG_RISCV_CSR_AIA_REG(iprio2):
> > +       case KVM_REG_RISCV_CSR | KVM_REG_RISCV_CSR_AIA | KVM_REG_RISCV_CSR_AIA_REG(sieh):
> > +       case KVM_REG_RISCV_CSR | KVM_REG_RISCV_CSR_AIA | KVM_REG_RISCV_CSR_AIA_REG(siph):
> > +       case KVM_REG_RISCV_CSR | KVM_REG_RISCV_CSR_AIA | KVM_REG_RISCV_CSR_AIA_REG(iprio1h):
> > +       case KVM_REG_RISCV_CSR | KVM_REG_RISCV_CSR_AIA | KVM_REG_RISCV_CSR_AIA_REG(iprio2h):
> > +               return isa_ext_cant_disable[KVM_RISCV_ISA_EXT_SSAIA] ? true : false;
>
> Ahh I guess. you do need the switch case for AIA CSRs but for ISA
> extensions can be avoided as it is contiguous.

Fow now, let's leave it as-is because this way get-reg-list will
complain if some new ONE_REG register is missed out.

>
> >         default:
> >                 break;
> >         }
> > @@ -71,14 +82,22 @@ static inline bool vcpu_has_ext(struct kvm_vcpu *vcpu, int ext)
> >
> >  void finalize_vcpu(struct kvm_vcpu *vcpu, struct vcpu_reg_list *c)
> >  {
> > +       int rc;
> >         struct vcpu_reg_sublist *s;
> > +       unsigned long isa_ext_state[KVM_RISCV_ISA_EXT_MAX] = { 0 };
> > +
> > +       for (int i = 0; i < KVM_RISCV_ISA_EXT_MAX; i++)
> > +               __vcpu_get_reg(vcpu, RISCV_ISA_EXT_REG(i), &isa_ext_state[i]);
> >
> >         /*
> >          * Disable all extensions which were enabled by default
> >          * if they were available in the risc-v host.
> >          */
> > -       for (int i = 0; i < KVM_RISCV_ISA_EXT_MAX; i++)
> > -               __vcpu_set_reg(vcpu, RISCV_ISA_EXT_REG(i), 0);
> > +       for (int i = 0; i < KVM_RISCV_ISA_EXT_MAX; i++) {
> > +               rc = __vcpu_set_reg(vcpu, RISCV_ISA_EXT_REG(i), 0);
> > +               if (rc && isa_ext_state[i])
> > +                       isa_ext_cant_disable[i] = true;
> > +       }
> >
> >         for_each_sublist(c, s) {
> >                 if (!s->feature)
> > --
> > 2.34.1
> >
>
> Otherwise, LGTM.
>
> Reviewed-by: Atish Patra <atishp@rivosinc.com>
>
> --
> Regards,
> Atish

Regards,
Anup

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

* Re: [PATCH 4/4] KVM: riscv: selftests: Selectively filter-out AIA registers
  2023-09-18 18:06 ` [PATCH 4/4] KVM: riscv: selftests: Selectively filter-out AIA registers Anup Patel
  2023-09-19 20:12   ` Atish Patra
@ 2023-09-20  5:24   ` Andrew Jones
  2023-09-20  7:10     ` Andrew Jones
  2023-09-20 13:49     ` Anup Patel
  1 sibling, 2 replies; 21+ messages in thread
From: Andrew Jones @ 2023-09-20  5:24 UTC (permalink / raw)
  To: Anup Patel
  Cc: Paolo Bonzini, Atish Patra, Shuah Khan, Palmer Dabbelt,
	Paul Walmsley, kvm, kvm-riscv, linux-riscv, linux-kernel,
	linux-kselftest

On Mon, Sep 18, 2023 at 11:36:46PM +0530, Anup Patel wrote:
> Currently the AIA ONE_REG registers are reported by get-reg-list
> as new registers for various vcpu_reg_list configs whenever Ssaia
> is available on the host because Ssaia extension can only be
> disabled by Smstateen extension which is not always available.
> 
> To tackle this, we should filter-out AIA ONE_REG registers only
> when Ssaia can't be disabled for a VCPU.
> 
> Fixes: 477069398ed6 ("KVM: riscv: selftests: Add get-reg-list test")
> Signed-off-by: Anup Patel <apatel@ventanamicro.com>
> ---
>  .../selftests/kvm/riscv/get-reg-list.c        | 23 +++++++++++++++++--
>  1 file changed, 21 insertions(+), 2 deletions(-)
> 
> diff --git a/tools/testing/selftests/kvm/riscv/get-reg-list.c b/tools/testing/selftests/kvm/riscv/get-reg-list.c
> index 76c0ad11e423..85907c86b835 100644
> --- a/tools/testing/selftests/kvm/riscv/get-reg-list.c
> +++ b/tools/testing/selftests/kvm/riscv/get-reg-list.c
> @@ -12,6 +12,8 @@
>  
>  #define REG_MASK (KVM_REG_ARCH_MASK | KVM_REG_SIZE_MASK)
>  
> +static bool isa_ext_cant_disable[KVM_RISCV_ISA_EXT_MAX];
> +
>  bool filter_reg(__u64 reg)
>  {
>  	switch (reg & ~REG_MASK) {
> @@ -48,6 +50,15 @@ bool filter_reg(__u64 reg)
>  	case KVM_REG_RISCV_ISA_EXT | KVM_RISCV_ISA_EXT_ZIFENCEI:
>  	case KVM_REG_RISCV_ISA_EXT | KVM_RISCV_ISA_EXT_ZIHPM:
>  		return true;
> +	/* AIA registers are always available when Ssaia can't be disabled */
> +	case KVM_REG_RISCV_CSR | KVM_REG_RISCV_CSR_AIA | KVM_REG_RISCV_CSR_AIA_REG(siselect):
> +	case KVM_REG_RISCV_CSR | KVM_REG_RISCV_CSR_AIA | KVM_REG_RISCV_CSR_AIA_REG(iprio1):
> +	case KVM_REG_RISCV_CSR | KVM_REG_RISCV_CSR_AIA | KVM_REG_RISCV_CSR_AIA_REG(iprio2):
> +	case KVM_REG_RISCV_CSR | KVM_REG_RISCV_CSR_AIA | KVM_REG_RISCV_CSR_AIA_REG(sieh):
> +	case KVM_REG_RISCV_CSR | KVM_REG_RISCV_CSR_AIA | KVM_REG_RISCV_CSR_AIA_REG(siph):
> +	case KVM_REG_RISCV_CSR | KVM_REG_RISCV_CSR_AIA | KVM_REG_RISCV_CSR_AIA_REG(iprio1h):
> +	case KVM_REG_RISCV_CSR | KVM_REG_RISCV_CSR_AIA | KVM_REG_RISCV_CSR_AIA_REG(iprio2h):
> +		return isa_ext_cant_disable[KVM_RISCV_ISA_EXT_SSAIA] ? true : false;

No need for the '? true : false'

>  	default:
>  		break;
>  	}
> @@ -71,14 +82,22 @@ static inline bool vcpu_has_ext(struct kvm_vcpu *vcpu, int ext)
>  
>  void finalize_vcpu(struct kvm_vcpu *vcpu, struct vcpu_reg_list *c)
>  {
> +	int rc;
>  	struct vcpu_reg_sublist *s;
> +	unsigned long isa_ext_state[KVM_RISCV_ISA_EXT_MAX] = { 0 };

nit: I think we prefer reverse xmas tree in kselftests, but whatever.

> +
> +	for (int i = 0; i < KVM_RISCV_ISA_EXT_MAX; i++)
> +		__vcpu_get_reg(vcpu, RISCV_ISA_EXT_REG(i), &isa_ext_state[i]);
>  
>  	/*
>  	 * Disable all extensions which were enabled by default
>  	 * if they were available in the risc-v host.
>  	 */
> -	for (int i = 0; i < KVM_RISCV_ISA_EXT_MAX; i++)
> -		__vcpu_set_reg(vcpu, RISCV_ISA_EXT_REG(i), 0);
> +	for (int i = 0; i < KVM_RISCV_ISA_EXT_MAX; i++) {
> +		rc = __vcpu_set_reg(vcpu, RISCV_ISA_EXT_REG(i), 0);
> +		if (rc && isa_ext_state[i])

How helpful is it to check that isa_ext_state[i] isn't zero? The value of
the register could be zero, right? Shouldn't we instead capture the return
values from __vcpu_get_reg and if the return value is zero for a get,
but nonzero for a set, then we know we have it, but can't disable it.

> +			isa_ext_cant_disable[i] = true;
> +	}
>  
>  	for_each_sublist(c, s) {
>  		if (!s->feature)
> -- 
> 2.34.1
>

Thanks,
drew

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

* Re: [PATCH 4/4] KVM: riscv: selftests: Selectively filter-out AIA registers
  2023-09-20  5:24   ` Andrew Jones
@ 2023-09-20  7:10     ` Andrew Jones
  2023-09-20 13:49     ` Anup Patel
  1 sibling, 0 replies; 21+ messages in thread
From: Andrew Jones @ 2023-09-20  7:10 UTC (permalink / raw)
  To: Anup Patel
  Cc: Paolo Bonzini, Atish Patra, Shuah Khan, Palmer Dabbelt,
	Paul Walmsley, kvm, kvm-riscv, linux-riscv, linux-kernel,
	linux-kselftest

On Wed, Sep 20, 2023 at 07:24:16AM +0200, Andrew Jones wrote:
> On Mon, Sep 18, 2023 at 11:36:46PM +0530, Anup Patel wrote:
> > Currently the AIA ONE_REG registers are reported by get-reg-list
> > as new registers for various vcpu_reg_list configs whenever Ssaia
> > is available on the host because Ssaia extension can only be
> > disabled by Smstateen extension which is not always available.
> > 
> > To tackle this, we should filter-out AIA ONE_REG registers only
> > when Ssaia can't be disabled for a VCPU.
> > 
> > Fixes: 477069398ed6 ("KVM: riscv: selftests: Add get-reg-list test")
> > Signed-off-by: Anup Patel <apatel@ventanamicro.com>
> > ---
> >  .../selftests/kvm/riscv/get-reg-list.c        | 23 +++++++++++++++++--
> >  1 file changed, 21 insertions(+), 2 deletions(-)
> > 
> > diff --git a/tools/testing/selftests/kvm/riscv/get-reg-list.c b/tools/testing/selftests/kvm/riscv/get-reg-list.c
> > index 76c0ad11e423..85907c86b835 100644
> > --- a/tools/testing/selftests/kvm/riscv/get-reg-list.c
> > +++ b/tools/testing/selftests/kvm/riscv/get-reg-list.c
> > @@ -12,6 +12,8 @@
> >  
> >  #define REG_MASK (KVM_REG_ARCH_MASK | KVM_REG_SIZE_MASK)
> >  
> > +static bool isa_ext_cant_disable[KVM_RISCV_ISA_EXT_MAX];
> > +
> >  bool filter_reg(__u64 reg)
> >  {
> >  	switch (reg & ~REG_MASK) {
> > @@ -48,6 +50,15 @@ bool filter_reg(__u64 reg)
> >  	case KVM_REG_RISCV_ISA_EXT | KVM_RISCV_ISA_EXT_ZIFENCEI:
> >  	case KVM_REG_RISCV_ISA_EXT | KVM_RISCV_ISA_EXT_ZIHPM:
> >  		return true;
> > +	/* AIA registers are always available when Ssaia can't be disabled */
> > +	case KVM_REG_RISCV_CSR | KVM_REG_RISCV_CSR_AIA | KVM_REG_RISCV_CSR_AIA_REG(siselect):
> > +	case KVM_REG_RISCV_CSR | KVM_REG_RISCV_CSR_AIA | KVM_REG_RISCV_CSR_AIA_REG(iprio1):
> > +	case KVM_REG_RISCV_CSR | KVM_REG_RISCV_CSR_AIA | KVM_REG_RISCV_CSR_AIA_REG(iprio2):
> > +	case KVM_REG_RISCV_CSR | KVM_REG_RISCV_CSR_AIA | KVM_REG_RISCV_CSR_AIA_REG(sieh):
> > +	case KVM_REG_RISCV_CSR | KVM_REG_RISCV_CSR_AIA | KVM_REG_RISCV_CSR_AIA_REG(siph):
> > +	case KVM_REG_RISCV_CSR | KVM_REG_RISCV_CSR_AIA | KVM_REG_RISCV_CSR_AIA_REG(iprio1h):
> > +	case KVM_REG_RISCV_CSR | KVM_REG_RISCV_CSR_AIA | KVM_REG_RISCV_CSR_AIA_REG(iprio2h):
> > +		return isa_ext_cant_disable[KVM_RISCV_ISA_EXT_SSAIA] ? true : false;
> 
> No need for the '? true : false'
> 
> >  	default:
> >  		break;
> >  	}
> > @@ -71,14 +82,22 @@ static inline bool vcpu_has_ext(struct kvm_vcpu *vcpu, int ext)
> >  
> >  void finalize_vcpu(struct kvm_vcpu *vcpu, struct vcpu_reg_list *c)
> >  {
> > +	int rc;
> >  	struct vcpu_reg_sublist *s;
> > +	unsigned long isa_ext_state[KVM_RISCV_ISA_EXT_MAX] = { 0 };
> 
> nit: I think we prefer reverse xmas tree in kselftests, but whatever.
> 
> > +
> > +	for (int i = 0; i < KVM_RISCV_ISA_EXT_MAX; i++)
> > +		__vcpu_get_reg(vcpu, RISCV_ISA_EXT_REG(i), &isa_ext_state[i]);
> >  
> >  	/*
> >  	 * Disable all extensions which were enabled by default
> >  	 * if they were available in the risc-v host.
> >  	 */
> > -	for (int i = 0; i < KVM_RISCV_ISA_EXT_MAX; i++)
> > -		__vcpu_set_reg(vcpu, RISCV_ISA_EXT_REG(i), 0);
> > +	for (int i = 0; i < KVM_RISCV_ISA_EXT_MAX; i++) {
> > +		rc = __vcpu_set_reg(vcpu, RISCV_ISA_EXT_REG(i), 0);
> > +		if (rc && isa_ext_state[i])
> 
> How helpful is it to check that isa_ext_state[i] isn't zero? The value of
> the register could be zero, right? Shouldn't we instead capture the return
> values from __vcpu_get_reg and if the return value is zero for a get,
> but nonzero for a set, then we know we have it, but can't disable it.

Eh, never mind. After sending this, I felt like there was something fishy
about my interpretation of how this works, so I took a second look. The
patch is correct as is, since we're checking for when the ISA extension
is present, but we can't disable it (just like it says it's doing :-) I
was thinking too much about AIA registers and not ISA extension registers.

> 
> > +			isa_ext_cant_disable[i] = true;
> > +	}
> >  
> >  	for_each_sublist(c, s) {
> >  		if (!s->feature)
> > -- 
> > 2.34.1
> >
>

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

Thanks,
drew

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

* Re: [PATCH 4/4] KVM: riscv: selftests: Selectively filter-out AIA registers
  2023-09-20  5:24   ` Andrew Jones
  2023-09-20  7:10     ` Andrew Jones
@ 2023-09-20 13:49     ` Anup Patel
  1 sibling, 0 replies; 21+ messages in thread
From: Anup Patel @ 2023-09-20 13:49 UTC (permalink / raw)
  To: Andrew Jones
  Cc: Paolo Bonzini, Atish Patra, Shuah Khan, Palmer Dabbelt,
	Paul Walmsley, kvm, kvm-riscv, linux-riscv, linux-kernel,
	linux-kselftest

On Wed, Sep 20, 2023 at 10:54 AM Andrew Jones <ajones@ventanamicro.com> wrote:
>
> On Mon, Sep 18, 2023 at 11:36:46PM +0530, Anup Patel wrote:
> > Currently the AIA ONE_REG registers are reported by get-reg-list
> > as new registers for various vcpu_reg_list configs whenever Ssaia
> > is available on the host because Ssaia extension can only be
> > disabled by Smstateen extension which is not always available.
> >
> > To tackle this, we should filter-out AIA ONE_REG registers only
> > when Ssaia can't be disabled for a VCPU.
> >
> > Fixes: 477069398ed6 ("KVM: riscv: selftests: Add get-reg-list test")
> > Signed-off-by: Anup Patel <apatel@ventanamicro.com>
> > ---
> >  .../selftests/kvm/riscv/get-reg-list.c        | 23 +++++++++++++++++--
> >  1 file changed, 21 insertions(+), 2 deletions(-)
> >
> > diff --git a/tools/testing/selftests/kvm/riscv/get-reg-list.c b/tools/testing/selftests/kvm/riscv/get-reg-list.c
> > index 76c0ad11e423..85907c86b835 100644
> > --- a/tools/testing/selftests/kvm/riscv/get-reg-list.c
> > +++ b/tools/testing/selftests/kvm/riscv/get-reg-list.c
> > @@ -12,6 +12,8 @@
> >
> >  #define REG_MASK (KVM_REG_ARCH_MASK | KVM_REG_SIZE_MASK)
> >
> > +static bool isa_ext_cant_disable[KVM_RISCV_ISA_EXT_MAX];
> > +
> >  bool filter_reg(__u64 reg)
> >  {
> >       switch (reg & ~REG_MASK) {
> > @@ -48,6 +50,15 @@ bool filter_reg(__u64 reg)
> >       case KVM_REG_RISCV_ISA_EXT | KVM_RISCV_ISA_EXT_ZIFENCEI:
> >       case KVM_REG_RISCV_ISA_EXT | KVM_RISCV_ISA_EXT_ZIHPM:
> >               return true;
> > +     /* AIA registers are always available when Ssaia can't be disabled */
> > +     case KVM_REG_RISCV_CSR | KVM_REG_RISCV_CSR_AIA | KVM_REG_RISCV_CSR_AIA_REG(siselect):
> > +     case KVM_REG_RISCV_CSR | KVM_REG_RISCV_CSR_AIA | KVM_REG_RISCV_CSR_AIA_REG(iprio1):
> > +     case KVM_REG_RISCV_CSR | KVM_REG_RISCV_CSR_AIA | KVM_REG_RISCV_CSR_AIA_REG(iprio2):
> > +     case KVM_REG_RISCV_CSR | KVM_REG_RISCV_CSR_AIA | KVM_REG_RISCV_CSR_AIA_REG(sieh):
> > +     case KVM_REG_RISCV_CSR | KVM_REG_RISCV_CSR_AIA | KVM_REG_RISCV_CSR_AIA_REG(siph):
> > +     case KVM_REG_RISCV_CSR | KVM_REG_RISCV_CSR_AIA | KVM_REG_RISCV_CSR_AIA_REG(iprio1h):
> > +     case KVM_REG_RISCV_CSR | KVM_REG_RISCV_CSR_AIA | KVM_REG_RISCV_CSR_AIA_REG(iprio2h):
> > +             return isa_ext_cant_disable[KVM_RISCV_ISA_EXT_SSAIA] ? true : false;
>
> No need for the '? true : false'

Okay, I will update.

>
> >       default:
> >               break;
> >       }
> > @@ -71,14 +82,22 @@ static inline bool vcpu_has_ext(struct kvm_vcpu *vcpu, int ext)
> >
> >  void finalize_vcpu(struct kvm_vcpu *vcpu, struct vcpu_reg_list *c)
> >  {
> > +     int rc;
> >       struct vcpu_reg_sublist *s;
> > +     unsigned long isa_ext_state[KVM_RISCV_ISA_EXT_MAX] = { 0 };
>
> nit: I think we prefer reverse xmas tree in kselftests, but whatever.

Okay, I will update.

>
> > +
> > +     for (int i = 0; i < KVM_RISCV_ISA_EXT_MAX; i++)
> > +             __vcpu_get_reg(vcpu, RISCV_ISA_EXT_REG(i), &isa_ext_state[i]);
> >
> >       /*
> >        * Disable all extensions which were enabled by default
> >        * if they were available in the risc-v host.
> >        */
> > -     for (int i = 0; i < KVM_RISCV_ISA_EXT_MAX; i++)
> > -             __vcpu_set_reg(vcpu, RISCV_ISA_EXT_REG(i), 0);
> > +     for (int i = 0; i < KVM_RISCV_ISA_EXT_MAX; i++) {
> > +             rc = __vcpu_set_reg(vcpu, RISCV_ISA_EXT_REG(i), 0);
> > +             if (rc && isa_ext_state[i])
>
> How helpful is it to check that isa_ext_state[i] isn't zero? The value of
> the register could be zero, right? Shouldn't we instead capture the return
> values from __vcpu_get_reg and if the return value is zero for a get,
> but nonzero for a set, then we know we have it, but can't disable it.

The intent is to find-out the ISA_EXT registers which are enabled but
we are not able to disable it.

>
> > +                     isa_ext_cant_disable[i] = true;
> > +     }
> >
> >       for_each_sublist(c, s) {
> >               if (!s->feature)
> > --
> > 2.34.1
> >
>
> Thanks,
> drew

Regards,
Anup

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

end of thread, other threads:[~2023-09-21 20:22 UTC | newest]

Thread overview: 21+ messages (download: mbox.gz follow: Atom feed
-- links below jump to the message on this page --
2023-09-18 18:06 [PATCH 0/4] KVM RISC-V fixes for ONE_REG interface Anup Patel
2023-09-18 18:06 ` [PATCH 1/4] RISC-V: KVM: Fix KVM_GET_REG_LIST API for ISA_EXT registers Anup Patel
2023-09-19 18:57   ` Atish Patra
2023-09-20  5:03   ` Andrew Jones
2023-09-18 18:06 ` [PATCH 2/4] RISC-V: KVM: Fix riscv_vcpu_get_isa_ext_single() for missing extensions Anup Patel
2023-09-19 18:59   ` Atish Patra
2023-09-20  5:04   ` Andrew Jones
2023-09-18 18:06 ` [PATCH 3/4] KVM: riscv: selftests: Fix ISA_EXT register handling in get-reg-list Anup Patel
2023-09-19 19:54   ` Atish Patra
2023-09-20 13:56     ` Anup Patel
2023-09-20 23:01       ` Atish Patra
2023-09-21  5:12         ` Anup Patel
2023-09-20  5:13   ` Andrew Jones
2023-09-18 18:06 ` [PATCH 4/4] KVM: riscv: selftests: Selectively filter-out AIA registers Anup Patel
2023-09-19 20:12   ` Atish Patra
2023-09-20  4:48     ` Andrew Jones
2023-09-20  5:26       ` Andrew Jones
2023-09-20 13:51     ` Anup Patel
2023-09-20  5:24   ` Andrew Jones
2023-09-20  7:10     ` Andrew Jones
2023-09-20 13:49     ` Anup Patel

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