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(-)
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) {
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
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
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 */
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
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
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,
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
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
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
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
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
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)
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
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
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
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
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
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
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
linux-kselftest-mirror@lists.linaro.org
-
Andrew Jones -
Anup Patel -
Atish Patra