| Message ID | 20230918180646.1398384-5-apatel@ventanamicro.com (mailing list archive) |
|---|---|
| State | Superseded, archived |
| Headers | show |
| Series | KVM RISC-V fixes for ONE_REG interface | expand |
| Context | Check | Description |
|---|---|---|
| conchuod/cover_letter | success | Series has a cover letter |
| conchuod/tree_selection | success | Guessed tree name to be fixes at HEAD 8eb8fe67e2c8 |
| conchuod/fixes_present | success | Fixes tag present in non-next series |
| conchuod/maintainers_pattern | success | MAINTAINERS pattern errors before the patch: 5 and now 5 |
| conchuod/verify_signedoff | success | Signed-off-by tag matches author and committer |
| conchuod/kdoc | success | Errors and warnings before: 0 this patch: 0 |
| conchuod/build_rv64_clang_allmodconfig | success | Errors and warnings before: 9 this patch: 9 |
| conchuod/module_param | success | Was 0 now: 0 |
| conchuod/build_rv64_gcc_allmodconfig | success | Errors and warnings before: 9 this patch: 9 |
| conchuod/build_rv32_defconfig | success | Build OK |
| conchuod/dtb_warn_rv64 | success | Errors and warnings before: 25 this patch: 25 |
| conchuod/header_inline | success | No static functions without inline keyword in header files |
| conchuod/checkpatch | success | total: 0 errors, 0 warnings, 0 checks, 47 lines checked |
| conchuod/build_rv64_nommu_k210_defconfig | success | Build OK |
| conchuod/verify_fixes | success | Fixes tag looks correct |
| conchuod/build_rv64_nommu_virt_defconfig | success | Build OK |
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 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 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 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
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
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)
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(-)