[v2] RISCV: Add kvm Sstc timer selftest

[PATCH v2 7/8] KVM: riscv: selftest: Change vcpu_has_ext to a common function

Posted by Haibo Xu 2 years, 3 months ago

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;
-}
-
 void finalize_vcpu(struct kvm_vcpu *vcpu, struct vcpu_reg_list *c)
 {
 	struct vcpu_reg_sublist *s;
-- 
2.34.1

Re: [PATCH v2 7/8] KVM: riscv: selftest: Change vcpu_has_ext to a common function

Posted by Andrew Jones 2 years, 3 months ago

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

Re: [PATCH v2 7/8] KVM: riscv: selftest: Change vcpu_has_ext to a common function

Posted by Haibo Xu 2 years, 3 months ago

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

Re: [PATCH v2 7/8] KVM: riscv: selftest: Change vcpu_has_ext to a common function

Posted by Haibo Xu 2 years, 3 months ago

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

Re: [PATCH v2 7/8] KVM: riscv: selftest: Change vcpu_has_ext to a common function

Posted by Andrew Jones 2 years, 3 months ago

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

Re: [PATCH v2 7/8] KVM: riscv: selftest: Change vcpu_has_ext to a common function

Posted by Haibo Xu 2 years, 3 months ago

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