| Message ID | 20230918180646.1398384-4-apatel@ventanamicro.com (mailing list archive) |
|---|---|
| State | Accepted |
| Commit | ba1af6e2e0f0e814c0f6be6ef64917c212f9fa96 |
| Headers | show |
| Series | KVM RISC-V fixes for ONE_REG interface | expand |
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 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
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
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,
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(-)