[PATCH] KVM: selftests: Fix nested SVM tests when built with clang

[PATCH] KVM: selftests: Fix nested SVM tests when built with clang

[Jim Mattson]

Though gcc conveniently compiles a simple memset to "rep stos," clang
prefers to call the libc version of memset. If a test is dynamically
linked, the libc memset isn't available in L1 (nor is the PLT or the
GOT, for that matter). Even if the test is statically linked, the libc
memset may choose to use some CPU features, like AVX, which may not be
enabled in L1. Note that __builtin_memset doesn't solve the problem,
because (a) the compiler is free to call memset anyway, and (b)
__builtin_memset may also choose to use features like AVX, which may
not be available in L1.

To avoid a myriad of problems, use an explicit "rep stos" to clear the
VMCB in generic_svm_setup(), which is called both from L0 and L1.

Reported-by: Ricardo Koller <ricarkol@google.com>
Signed-off-by: Jim Mattson <jmattson@google.com>
Fixes: 20ba262f8631a ("selftests: KVM: AMD Nested test infrastructure")
---
 tools/testing/selftests/kvm/lib/x86_64/svm.c | 14 +++++++++++++-
 1 file changed, 13 insertions(+), 1 deletion(-)

diff --git a/tools/testing/selftests/kvm/lib/x86_64/svm.c b/tools/testing/selftests/kvm/lib/x86_64/svm.c
index 2ac98d70d02b..161eba7cd128 100644
--- a/tools/testing/selftests/kvm/lib/x86_64/svm.c
+++ b/tools/testing/selftests/kvm/lib/x86_64/svm.c
@@ -54,6 +54,18 @@ static void vmcb_set_seg(struct vmcb_seg *seg, u16 selector,
 	seg->base = base;
 }
 
+/*
+ * Avoid using memset to clear the vmcb, since libc may not be
+ * available in L1 (and, even if it is, features that libc memset may
+ * want to use, like AVX, may not be enabled).
+ */
+static void clear_vmcb(struct vmcb *vmcb)
+{
+	int n = sizeof(*vmcb) / sizeof(u32);
+
+	asm volatile ("rep stosl" : "+c"(n), "+D"(vmcb) : "a"(0) : "memory");
+}
+
 void generic_svm_setup(struct svm_test_data *svm, void *guest_rip, void *guest_rsp)
 {
 	struct vmcb *vmcb = svm->vmcb;
@@ -70,7 +82,7 @@ void generic_svm_setup(struct svm_test_data *svm, void *guest_rip, void *guest_r
 	wrmsr(MSR_EFER, efer | EFER_SVME);
 	wrmsr(MSR_VM_HSAVE_PA, svm->save_area_gpa);
 
-	memset(vmcb, 0, sizeof(*vmcb));
+	clear_vmcb(vmcb);
 	asm volatile ("vmsave %0\n\t" : : "a" (vmcb_gpa) : "memory");
 	vmcb_set_seg(&save->es, get_es(), 0, -1U, data_seg_attr);
 	vmcb_set_seg(&save->cs, get_cs(), 0, -1U, code_seg_attr);
-- 
2.33.0.685.g46640cef36-goog

Re: [PATCH] KVM: selftests: Fix nested SVM tests when built with clang

[Ricardo Koller]

On Wed, Sep 29, 2021 at 05:36:49PM -0700, Jim Mattson wrote:
> Though gcc conveniently compiles a simple memset to "rep stos," clang
> prefers to call the libc version of memset. If a test is dynamically
> linked, the libc memset isn't available in L1 (nor is the PLT or the
> GOT, for that matter). Even if the test is statically linked, the libc
> memset may choose to use some CPU features, like AVX, which may not be
> enabled in L1. Note that __builtin_memset doesn't solve the problem,
> because (a) the compiler is free to call memset anyway, and (b)
> __builtin_memset may also choose to use features like AVX, which may
> not be available in L1.
> 
> To avoid a myriad of problems, use an explicit "rep stos" to clear the
> VMCB in generic_svm_setup(), which is called both from L0 and L1.
> 
> Reported-by: Ricardo Koller <ricarkol@google.com>
> Signed-off-by: Jim Mattson <jmattson@google.com>
> Fixes: 20ba262f8631a ("selftests: KVM: AMD Nested test infrastructure")
> ---
>  tools/testing/selftests/kvm/lib/x86_64/svm.c | 14 +++++++++++++-
>  1 file changed, 13 insertions(+), 1 deletion(-)
> 
> diff --git a/tools/testing/selftests/kvm/lib/x86_64/svm.c b/tools/testing/selftests/kvm/lib/x86_64/svm.c
> index 2ac98d70d02b..161eba7cd128 100644
> --- a/tools/testing/selftests/kvm/lib/x86_64/svm.c
> +++ b/tools/testing/selftests/kvm/lib/x86_64/svm.c
> @@ -54,6 +54,18 @@ static void vmcb_set_seg(struct vmcb_seg *seg, u16 selector,
>  	seg->base = base;
>  }
>  
> +/*
> + * Avoid using memset to clear the vmcb, since libc may not be
> + * available in L1 (and, even if it is, features that libc memset may
> + * want to use, like AVX, may not be enabled).
> + */
> +static void clear_vmcb(struct vmcb *vmcb)
> +{
> +	int n = sizeof(*vmcb) / sizeof(u32);
> +
> +	asm volatile ("rep stosl" : "+c"(n), "+D"(vmcb) : "a"(0) : "memory");
> +}
> +
>  void generic_svm_setup(struct svm_test_data *svm, void *guest_rip, void *guest_rsp)
>  {
>  	struct vmcb *vmcb = svm->vmcb;
> @@ -70,7 +82,7 @@ void generic_svm_setup(struct svm_test_data *svm, void *guest_rip, void *guest_r
>  	wrmsr(MSR_EFER, efer | EFER_SVME);
>  	wrmsr(MSR_VM_HSAVE_PA, svm->save_area_gpa);
>  
> -	memset(vmcb, 0, sizeof(*vmcb));
> +	clear_vmcb(vmcb);
>  	asm volatile ("vmsave %0\n\t" : : "a" (vmcb_gpa) : "memory");
>  	vmcb_set_seg(&save->es, get_es(), 0, -1U, data_seg_attr);
>  	vmcb_set_seg(&save->cs, get_cs(), 0, -1U, code_seg_attr);
> -- 
> 2.33.0.685.g46640cef36-goog
> 

Reviewed-by: Ricardo Koller <ricarkol@google.com>

Re: [PATCH] KVM: selftests: Fix nested SVM tests when built with clang

[Jim Mattson]

On Wed, Sep 29, 2021 at 6:30 PM Ricardo Koller <ricarkol@google.com> wrote:
>
> On Wed, Sep 29, 2021 at 05:36:49PM -0700, Jim Mattson wrote:
> > Though gcc conveniently compiles a simple memset to "rep stos," clang
> > prefers to call the libc version of memset. If a test is dynamically
> > linked, the libc memset isn't available in L1 (nor is the PLT or the
> > GOT, for that matter). Even if the test is statically linked, the libc
> > memset may choose to use some CPU features, like AVX, which may not be
> > enabled in L1. Note that __builtin_memset doesn't solve the problem,
> > because (a) the compiler is free to call memset anyway, and (b)
> > __builtin_memset may also choose to use features like AVX, which may
> > not be available in L1.
> >
> > To avoid a myriad of problems, use an explicit "rep stos" to clear the
> > VMCB in generic_svm_setup(), which is called both from L0 and L1.
> >
> > Reported-by: Ricardo Koller <ricarkol@google.com>
> > Signed-off-by: Jim Mattson <jmattson@google.com>
> > Fixes: 20ba262f8631a ("selftests: KVM: AMD Nested test infrastructure")
> > ---
> >  tools/testing/selftests/kvm/lib/x86_64/svm.c | 14 +++++++++++++-
> >  1 file changed, 13 insertions(+), 1 deletion(-)
> >
> > diff --git a/tools/testing/selftests/kvm/lib/x86_64/svm.c b/tools/testing/selftests/kvm/lib/x86_64/svm.c
> > index 2ac98d70d02b..161eba7cd128 100644
> > --- a/tools/testing/selftests/kvm/lib/x86_64/svm.c
> > +++ b/tools/testing/selftests/kvm/lib/x86_64/svm.c
> > @@ -54,6 +54,18 @@ static void vmcb_set_seg(struct vmcb_seg *seg, u16 selector,
> >       seg->base = base;
> >  }
> >
> > +/*
> > + * Avoid using memset to clear the vmcb, since libc may not be
> > + * available in L1 (and, even if it is, features that libc memset may
> > + * want to use, like AVX, may not be enabled).
> > + */
> > +static void clear_vmcb(struct vmcb *vmcb)
> > +{
> > +     int n = sizeof(*vmcb) / sizeof(u32);
> > +
> > +     asm volatile ("rep stosl" : "+c"(n), "+D"(vmcb) : "a"(0) : "memory");
> > +}
> > +
> >  void generic_svm_setup(struct svm_test_data *svm, void *guest_rip, void *guest_rsp)
> >  {
> >       struct vmcb *vmcb = svm->vmcb;
> > @@ -70,7 +82,7 @@ void generic_svm_setup(struct svm_test_data *svm, void *guest_rip, void *guest_r
> >       wrmsr(MSR_EFER, efer | EFER_SVME);
> >       wrmsr(MSR_VM_HSAVE_PA, svm->save_area_gpa);
> >
> > -     memset(vmcb, 0, sizeof(*vmcb));
> > +     clear_vmcb(vmcb);
> >       asm volatile ("vmsave %0\n\t" : : "a" (vmcb_gpa) : "memory");
> >       vmcb_set_seg(&save->es, get_es(), 0, -1U, data_seg_attr);
> >       vmcb_set_seg(&save->cs, get_cs(), 0, -1U, code_seg_attr);
> > --
> > 2.33.0.685.g46640cef36-goog
> >
>
> Reviewed-by: Ricardo Koller <ricarkol@google.com>

Ping!

Re: [PATCH] KVM: selftests: Fix nested SVM tests when built with clang

[Paolo Bonzini]

On 30/09/21 02:36, Jim Mattson wrote:
> Though gcc conveniently compiles a simple memset to "rep stos," clang
> prefers to call the libc version of memset. If a test is dynamically
> linked, the libc memset isn't available in L1 (nor is the PLT or the
> GOT, for that matter). Even if the test is statically linked, the libc
> memset may choose to use some CPU features, like AVX, which may not be
> enabled in L1. Note that __builtin_memset doesn't solve the problem,
> because (a) the compiler is free to call memset anyway, and (b)
> __builtin_memset may also choose to use features like AVX, which may
> not be available in L1.
> 
> To avoid a myriad of problems, use an explicit "rep stos" to clear the
> VMCB in generic_svm_setup(), which is called both from L0 and L1.

Queued, thanks.  It would be nice if llvm at least had gcc's 
-minline-all-stringops and/or -mstringop-strategy=rep_byte, but even on 
gcc it wasn't really sure to work without those options.

Paolo