| Message ID | b6ef1b031e3a581f481cf19a26623388163444b4.1693659382.git.haibo1.xu@intel.com (mailing list archive) |
|---|---|
| State | Handled Elsewhere, archived |
| Headers | show |
| Series | RISCV: Add kvm Sstc timer selftest | expand |
| Context | Check | Description |
|---|---|---|
| conchuod/tree_selection | fail | Failed to apply to next/pending-fixes, riscv/for-next or riscv/master |
On Sat, Sep 02, 2023 at 08:59:29PM +0800, Haibo Xu wrote: > Move vcpu_has_ext to the processor.c so that other test cases > can use it for vCPU extension check. > > Signed-off-by: Haibo Xu <haibo1.xu@intel.com> > --- > .../selftests/kvm/include/riscv/processor.h | 2 ++ > tools/testing/selftests/kvm/lib/riscv/processor.c | 9 +++++++++ > tools/testing/selftests/kvm/riscv/get-reg-list.c | 14 -------------- > 3 files changed, 11 insertions(+), 14 deletions(-) > > diff --git a/tools/testing/selftests/kvm/include/riscv/processor.h b/tools/testing/selftests/kvm/include/riscv/processor.h > index d1e5d9f7ad45..6087c8fc263a 100644 > --- a/tools/testing/selftests/kvm/include/riscv/processor.h > +++ b/tools/testing/selftests/kvm/include/riscv/processor.h > @@ -42,6 +42,8 @@ static inline uint64_t __kvm_reg_id(uint64_t type, uint64_t idx, > #define RISCV_ISA_EXT_REG(idx) __kvm_reg_id(KVM_REG_RISCV_ISA_EXT, \ > idx, KVM_REG_SIZE_ULONG) > > +bool vcpu_has_ext(struct kvm_vcpu *vcpu, int ext); > + > struct ex_regs { > unsigned long ra; > unsigned long sp; > diff --git a/tools/testing/selftests/kvm/lib/riscv/processor.c b/tools/testing/selftests/kvm/lib/riscv/processor.c > index 39a1e9902dec..5ececa566f24 100644 > --- a/tools/testing/selftests/kvm/lib/riscv/processor.c > +++ b/tools/testing/selftests/kvm/lib/riscv/processor.c > @@ -15,6 +15,15 @@ > > static vm_vaddr_t exception_handlers; > > +bool vcpu_has_ext(struct kvm_vcpu *vcpu, int ext) > +{ > + unsigned long value = 0; > + > + vcpu_get_reg(vcpu, RISCV_ISA_EXT_REG(ext), &value); > + > + return !!value; > +} > + > static uint64_t page_align(struct kvm_vm *vm, uint64_t v) > { > return (v + vm->page_size) & ~(vm->page_size - 1); > diff --git a/tools/testing/selftests/kvm/riscv/get-reg-list.c b/tools/testing/selftests/kvm/riscv/get-reg-list.c > index d8ecacd03ecf..c4028bf32e3f 100644 > --- a/tools/testing/selftests/kvm/riscv/get-reg-list.c > +++ b/tools/testing/selftests/kvm/riscv/get-reg-list.c > @@ -44,20 +44,6 @@ bool check_reject_set(int err) > return err == EINVAL; > } > > -static inline bool vcpu_has_ext(struct kvm_vcpu *vcpu, int ext) > -{ > - int ret; > - 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; get-reg-list will now assert on get-reg when an extension isn't present, rather than failing the __TEST_REQUIRE(), which would do a skip instead. We need both the return false version and the assert version. > -} > - > void finalize_vcpu(struct kvm_vcpu *vcpu, struct vcpu_reg_list *c) > { > struct vcpu_reg_sublist *s; > -- > 2.34.1 > Thanks, drew
On Mon, Sep 4, 2023 at 10:04 PM Andrew Jones <ajones@ventanamicro.com> wrote: > > On Sat, Sep 02, 2023 at 08:59:29PM +0800, Haibo Xu wrote: > > diff --git a/tools/testing/selftests/kvm/riscv/get-reg-list.c b/tools/testing/selftests/kvm/riscv/get-reg-list.c > > index d8ecacd03ecf..c4028bf32e3f 100644 > > --- a/tools/testing/selftests/kvm/riscv/get-reg-list.c > > +++ b/tools/testing/selftests/kvm/riscv/get-reg-list.c > > @@ -44,20 +44,6 @@ bool check_reject_set(int err) > > return err == EINVAL; > > } > > > > -static inline bool vcpu_has_ext(struct kvm_vcpu *vcpu, int ext) > > -{ > > - int ret; > > - 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; > > get-reg-list will now assert on get-reg when an extension isn't present, > rather than failing the __TEST_REQUIRE(), which would do a skip instead. > We need both the return false version and the assert version. > Ok, Will keep this one for get-reg-list and add another one for arch-timer specific usage. > > -} > > - > > void finalize_vcpu(struct kvm_vcpu *vcpu, struct vcpu_reg_list *c) > > { > > struct vcpu_reg_sublist *s; > > -- > > 2.34.1 > > > > Thanks, > drew
On Wed, Sep 6, 2023 at 6:10 PM Haibo Xu <xiaobo55x@gmail.com> wrote: > > On Mon, Sep 4, 2023 at 10:04 PM Andrew Jones <ajones@ventanamicro.com> wrote: > > > > On Sat, Sep 02, 2023 at 08:59:29PM +0800, Haibo Xu wrote: > > > diff --git a/tools/testing/selftests/kvm/riscv/get-reg-list.c b/tools/testing/selftests/kvm/riscv/get-reg-list.c > > > index d8ecacd03ecf..c4028bf32e3f 100644 > > > --- a/tools/testing/selftests/kvm/riscv/get-reg-list.c > > > +++ b/tools/testing/selftests/kvm/riscv/get-reg-list.c > > > @@ -44,20 +44,6 @@ bool check_reject_set(int err) > > > return err == EINVAL; > > > } > > > > > > -static inline bool vcpu_has_ext(struct kvm_vcpu *vcpu, int ext) > > > -{ > > > - int ret; > > > - 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; > > > > get-reg-list will now assert on get-reg when an extension isn't present, > > rather than failing the __TEST_REQUIRE(), which would do a skip instead. > > We need both the return false version and the assert version. > > > > Ok, Will keep this one for get-reg-list and add another one for > arch-timer specific usage. > Just thought about it again, maybe we only need the "return false" version for both get-reg-list and arch-timer tests since if an extension was not available, the test can be skipped with a message. bool vcpu_has_ext(struct kvm_vcpu *vcpu, int ext) { unsigned long value = 0; __vcpu_get_reg(vcpu, RISCV_ISA_EXT_REG(ext), &value); return !!value; } > > > -} > > > - > > > void finalize_vcpu(struct kvm_vcpu *vcpu, struct vcpu_reg_list *c) > > > { > > > struct vcpu_reg_sublist *s; > > > -- > > > 2.34.1 > > > > > > > Thanks, > > drew
On Thu, Sep 07, 2023 at 11:57:00AM +0800, Haibo Xu wrote: > On Wed, Sep 6, 2023 at 6:10 PM Haibo Xu <xiaobo55x@gmail.com> wrote: > > > > On Mon, Sep 4, 2023 at 10:04 PM Andrew Jones <ajones@ventanamicro.com> wrote: > > > > > > On Sat, Sep 02, 2023 at 08:59:29PM +0800, Haibo Xu wrote: > > > > diff --git a/tools/testing/selftests/kvm/riscv/get-reg-list.c b/tools/testing/selftests/kvm/riscv/get-reg-list.c > > > > index d8ecacd03ecf..c4028bf32e3f 100644 > > > > --- a/tools/testing/selftests/kvm/riscv/get-reg-list.c > > > > +++ b/tools/testing/selftests/kvm/riscv/get-reg-list.c > > > > @@ -44,20 +44,6 @@ bool check_reject_set(int err) > > > > return err == EINVAL; > > > > } > > > > > > > > -static inline bool vcpu_has_ext(struct kvm_vcpu *vcpu, int ext) > > > > -{ > > > > - int ret; > > > > - 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; > > > > > > get-reg-list will now assert on get-reg when an extension isn't present, > > > rather than failing the __TEST_REQUIRE(), which would do a skip instead. > > > We need both the return false version and the assert version. > > > > > > > Ok, Will keep this one for get-reg-list and add another one for > > arch-timer specific usage. > > > > Just thought about it again, maybe we only need the "return false" > version for both get-reg-list > and arch-timer tests since if an extension was not available, the test > can be skipped with a message. > > bool vcpu_has_ext(struct kvm_vcpu *vcpu, int ext) > { > unsigned long value = 0; > > __vcpu_get_reg(vcpu, RISCV_ISA_EXT_REG(ext), &value); > > return !!value; > } Yup, I had actually seen that when reviewing a later patch in this series, but I wasn't concerned if we added the assert type anyway, since we frequently end up with the two function types for KVM queries. If we don't have a need for an assert type yet, then we don't need to introduce it. However, we should introduce the non-assert type as __vcpu_has_ext(), reserving the vcpu_has_ext() name for the assert type, per the kvm selftests naming convention. Thanks, drew
On Thu, Sep 7, 2023 at 5:01 PM Andrew Jones <ajones@ventanamicro.com> wrote: > > On Thu, Sep 07, 2023 at 11:57:00AM +0800, Haibo Xu wrote: > > On Wed, Sep 6, 2023 at 6:10 PM Haibo Xu <xiaobo55x@gmail.com> wrote: > > > > > > On Mon, Sep 4, 2023 at 10:04 PM Andrew Jones <ajones@ventanamicro.com> wrote: > > > > > > > > On Sat, Sep 02, 2023 at 08:59:29PM +0800, Haibo Xu wrote: > > > > > diff --git a/tools/testing/selftests/kvm/riscv/get-reg-list.c b/tools/testing/selftests/kvm/riscv/get-reg-list.c > > > > > index d8ecacd03ecf..c4028bf32e3f 100644 > > > > > --- a/tools/testing/selftests/kvm/riscv/get-reg-list.c > > > > > +++ b/tools/testing/selftests/kvm/riscv/get-reg-list.c > > > > > @@ -44,20 +44,6 @@ bool check_reject_set(int err) > > > > > return err == EINVAL; > > > > > } > > > > > > > > > > -static inline bool vcpu_has_ext(struct kvm_vcpu *vcpu, int ext) > > > > > -{ > > > > > - int ret; > > > > > - 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; > > > > > > > > get-reg-list will now assert on get-reg when an extension isn't present, > > > > rather than failing the __TEST_REQUIRE(), which would do a skip instead. > > > > We need both the return false version and the assert version. > > > > > > > > > > Ok, Will keep this one for get-reg-list and add another one for > > > arch-timer specific usage. > > > > > > > Just thought about it again, maybe we only need the "return false" > > version for both get-reg-list > > and arch-timer tests since if an extension was not available, the test > > can be skipped with a message. > > > > bool vcpu_has_ext(struct kvm_vcpu *vcpu, int ext) > > { > > unsigned long value = 0; > > > > __vcpu_get_reg(vcpu, RISCV_ISA_EXT_REG(ext), &value); > > > > return !!value; > > } > > Yup, I had actually seen that when reviewing a later patch in this series, > but I wasn't concerned if we added the assert type anyway, since we > frequently end up with the two function types for KVM queries. If we don't > have a need for an assert type yet, then we don't need to introduce it. > However, we should introduce the non-assert type as __vcpu_has_ext(), > reserving the vcpu_has_ext() name for the assert type, per the kvm > selftests naming convention. > Sure, thanks! > Thanks, > drew
diff --git a/tools/testing/selftests/kvm/include/riscv/processor.h b/tools/testing/selftests/kvm/include/riscv/processor.h index d1e5d9f7ad45..6087c8fc263a 100644 --- a/tools/testing/selftests/kvm/include/riscv/processor.h +++ b/tools/testing/selftests/kvm/include/riscv/processor.h @@ -42,6 +42,8 @@ static inline uint64_t __kvm_reg_id(uint64_t type, uint64_t idx, #define RISCV_ISA_EXT_REG(idx) __kvm_reg_id(KVM_REG_RISCV_ISA_EXT, \ idx, KVM_REG_SIZE_ULONG) +bool vcpu_has_ext(struct kvm_vcpu *vcpu, int ext); + struct ex_regs { unsigned long ra; unsigned long sp; diff --git a/tools/testing/selftests/kvm/lib/riscv/processor.c b/tools/testing/selftests/kvm/lib/riscv/processor.c index 39a1e9902dec..5ececa566f24 100644 --- a/tools/testing/selftests/kvm/lib/riscv/processor.c +++ b/tools/testing/selftests/kvm/lib/riscv/processor.c @@ -15,6 +15,15 @@ static vm_vaddr_t exception_handlers; +bool vcpu_has_ext(struct kvm_vcpu *vcpu, int ext) +{ + unsigned long value = 0; + + vcpu_get_reg(vcpu, RISCV_ISA_EXT_REG(ext), &value); + + return !!value; +} + static uint64_t page_align(struct kvm_vm *vm, uint64_t v) { return (v + vm->page_size) & ~(vm->page_size - 1); diff --git a/tools/testing/selftests/kvm/riscv/get-reg-list.c b/tools/testing/selftests/kvm/riscv/get-reg-list.c index d8ecacd03ecf..c4028bf32e3f 100644 --- a/tools/testing/selftests/kvm/riscv/get-reg-list.c +++ b/tools/testing/selftests/kvm/riscv/get-reg-list.c @@ -44,20 +44,6 @@ bool check_reject_set(int err) return err == EINVAL; } -static inline bool vcpu_has_ext(struct kvm_vcpu *vcpu, int ext) -{ - int ret; - 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; -} - void finalize_vcpu(struct kvm_vcpu *vcpu, struct vcpu_reg_list *c) { struct vcpu_reg_sublist *s;
Move vcpu_has_ext to the processor.c so that other test cases can use it for vCPU extension check. Signed-off-by: Haibo Xu <haibo1.xu@intel.com> --- .../selftests/kvm/include/riscv/processor.h | 2 ++ tools/testing/selftests/kvm/lib/riscv/processor.c | 9 +++++++++ tools/testing/selftests/kvm/riscv/get-reg-list.c | 14 -------------- 3 files changed, 11 insertions(+), 14 deletions(-)