1. Patchew
  2. linux
  3. [RFC V3 PATCH 0/6] selftests: KVM: selftests for fd-based private memory
  4. View patch

[RFC V3 PATCH 4/6] selftests: kvm: x86: Execute hypercall as per the cpu

Vishal Annapurve posted 6 patches 3 years, 7 months ago
[RFC V3 PATCH 4/6] selftests: kvm: x86: Execute hypercall as per the cpu
Posted by Vishal Annapurve 3 years, 7 months ago 
Add support for executing vmmcall/vmcall instruction on amd/intel cpus.
In general kvm patches the instruction according to the cpu
implementation at runtime. While executing selftest vms from private
memory KVM will not be able to update the private memory of the guest.

Hypercall parameters are fixed to explicitly populate hypercall number
in eax. Otherwise inlined function calls to kvm_hypercall would call
vmmcall/vmcall instruction without updating eax with hypercall number.

Signed-off-by: Vishal Annapurve <vannapurve@google.com>
---
 .../testing/selftests/kvm/lib/x86_64/processor.c  | 15 +++++++++++++--
 1 file changed, 13 insertions(+), 2 deletions(-)

diff --git a/tools/testing/selftests/kvm/lib/x86_64/processor.c b/tools/testing/selftests/kvm/lib/x86_64/processor.c
index 53b115876417..09d757a0b148 100644
--- a/tools/testing/selftests/kvm/lib/x86_64/processor.c
+++ b/tools/testing/selftests/kvm/lib/x86_64/processor.c
@@ -1254,10 +1254,21 @@ uint64_t kvm_hypercall(uint64_t nr, uint64_t a0, uint64_t a1, uint64_t a2,
 		       uint64_t a3)
 {
 	uint64_t r;
+	static bool is_cpu_checked;
+	static bool is_cpu_amd;
 
-	asm volatile("vmcall"
+	if (!is_cpu_checked)
+		is_cpu_amd = is_amd_cpu();
+
+	if (is_cpu_amd) {
+		asm volatile("vmmcall"
+		     : "=a"(r)
+		     : "a"(nr), "b"(a0), "c"(a1), "d"(a2), "S"(a3));
+	} else {
+		asm volatile("vmcall"
 		     : "=a"(r)
-		     : "b"(a0), "c"(a1), "d"(a2), "S"(a3));
+		     : "a"(nr), "b"(a0), "c"(a1), "d"(a2), "S"(a3));
+	}
 	return r;
 }
 
-- 
2.37.1.595.g718a3a8f04-goog
Re: [RFC V3 PATCH 4/6] selftests: kvm: x86: Execute hypercall as per the cpu
Posted by Sean Christopherson 3 years, 7 months ago 
On Fri, Aug 19, 2022, Vishal Annapurve wrote:
> Add support for executing vmmcall/vmcall instruction on amd/intel cpus.
> In general kvm patches the instruction according to the cpu
> implementation at runtime. While executing selftest vms from private
> memory KVM will not be able to update the private memory of the guest.
> 
> Hypercall parameters are fixed to explicitly populate hypercall number
> in eax. Otherwise inlined function calls to kvm_hypercall would call
> vmmcall/vmcall instruction without updating eax with hypercall number.

Can you send a seperate non-RFC series to clean up the selftests mess?  kvm_hypercall()
isn't the only culprit.

  git grep \"vmcall | wc -l
  16

I'm pretty sure things work only because of KVM's dubious behavior of patching
VMCALL/VMMCALL by default.

Note, svm_vmcall_test.c intentionally uses the wrong instructions and shouldn't
be converted.  Actually, we can and should just delete that test, it's superseded
by the wonderfully named fix_hypercall_test.

> Signed-off-by: Vishal Annapurve <vannapurve@google.com>
> ---
>  .../testing/selftests/kvm/lib/x86_64/processor.c  | 15 +++++++++++++--
>  1 file changed, 13 insertions(+), 2 deletions(-)
> 
> diff --git a/tools/testing/selftests/kvm/lib/x86_64/processor.c b/tools/testing/selftests/kvm/lib/x86_64/processor.c
> index 53b115876417..09d757a0b148 100644
> --- a/tools/testing/selftests/kvm/lib/x86_64/processor.c
> +++ b/tools/testing/selftests/kvm/lib/x86_64/processor.c
> @@ -1254,10 +1254,21 @@ uint64_t kvm_hypercall(uint64_t nr, uint64_t a0, uint64_t a1, uint64_t a2,
>  		       uint64_t a3)
>  {
>  	uint64_t r;
> +	static bool is_cpu_checked;
> +	static bool is_cpu_amd;
>  
> -	asm volatile("vmcall"
> +	if (!is_cpu_checked)
> +		is_cpu_amd = is_amd_cpu();

This can be done using a single int, e.g.

	static bool is_cpu_amd = -1;

	if (is_cpu_amd < 0)
		is_cpu_amd = is_amd_cpu();

Although... what if we declare main() in lib/kvm_util.c (or maybe a dedicated
file?) and rename all tests to use __main()?  Then add an arch hook to do global
initialization and avoid the "did we check CPUID?!?!?" altogether.

That would allow us to dedup all of the hilarious copy paste:

	/* Tell stdout not to buffer its content */
	setbuf(stdout, NULL);

and we could turn is_amd_cpu() and is_intel_cpu() into bools.

E.g.

int main(int argc, char *argv[])
{
	/* Tell stdout not to buffer its content */
	setbuf(stdout, NULL);

	kvm_arch_main();

	return __main(argc, argv);
}

void kvm_arch_main(void)
{
	is_cpu_amd = cpu_vendor_string_is("AuthenticAMD");
	is_cpu_intel = cpu_vendor_string_is("AuthenticAMD");
}


And then we just need macro magic to emit the right VMCALL/VMMCALL instruction.
Re: [RFC V3 PATCH 4/6] selftests: kvm: x86: Execute hypercall as per the cpu
Posted by Vishal Annapurve 3 years, 7 months ago 
On Wed, Aug 24, 2022 at 5:07 PM Sean Christopherson <seanjc@google.com> wrote:
>
> On Fri, Aug 19, 2022, Vishal Annapurve wrote:
> > Add support for executing vmmcall/vmcall instruction on amd/intel cpus.
> > In general kvm patches the instruction according to the cpu
> > implementation at runtime. While executing selftest vms from private
> > memory KVM will not be able to update the private memory of the guest.
> >
> > Hypercall parameters are fixed to explicitly populate hypercall number
> > in eax. Otherwise inlined function calls to kvm_hypercall would call
> > vmmcall/vmcall instruction without updating eax with hypercall number.
>
> Can you send a seperate non-RFC series to clean up the selftests mess?  kvm_hypercall()
> isn't the only culprit.
>
>   git grep \"vmcall | wc -l
>   16
>
> I'm pretty sure things work only because of KVM's dubious behavior of patching
> VMCALL/VMMCALL by default.
>
> Note, svm_vmcall_test.c intentionally uses the wrong instructions and shouldn't
> be converted.  Actually, we can and should just delete that test, it's superseded
> by the wonderfully named fix_hypercall_test.
>
> > Signed-off-by: Vishal Annapurve <vannapurve@google.com>
> > ---
> >  .../testing/selftests/kvm/lib/x86_64/processor.c  | 15 +++++++++++++--
> >  1 file changed, 13 insertions(+), 2 deletions(-)
> >
> > diff --git a/tools/testing/selftests/kvm/lib/x86_64/processor.c b/tools/testing/selftests/kvm/lib/x86_64/processor.c
> > index 53b115876417..09d757a0b148 100644
> > --- a/tools/testing/selftests/kvm/lib/x86_64/processor.c
> > +++ b/tools/testing/selftests/kvm/lib/x86_64/processor.c
> > @@ -1254,10 +1254,21 @@ uint64_t kvm_hypercall(uint64_t nr, uint64_t a0, uint64_t a1, uint64_t a2,
> >                      uint64_t a3)
> >  {
> >       uint64_t r;
> > +     static bool is_cpu_checked;
> > +     static bool is_cpu_amd;
> >
> > -     asm volatile("vmcall"
> > +     if (!is_cpu_checked)
> > +             is_cpu_amd = is_amd_cpu();
>
> This can be done using a single int, e.g.
>
>         static bool is_cpu_amd = -1;
>
>         if (is_cpu_amd < 0)
>                 is_cpu_amd = is_amd_cpu();
>
> Although... what if we declare main() in lib/kvm_util.c (or maybe a dedicated
> file?) and rename all tests to use __main()?  Then add an arch hook to do global
> initialization and avoid the "did we check CPUID?!?!?" altogether.
>
> That would allow us to dedup all of the hilarious copy paste:
>
>         /* Tell stdout not to buffer its content */
>         setbuf(stdout, NULL);
>
> and we could turn is_amd_cpu() and is_intel_cpu() into bools.
>
> E.g.
>
> int main(int argc, char *argv[])
> {
>         /* Tell stdout not to buffer its content */
>         setbuf(stdout, NULL);
>
>         kvm_arch_main();
>
>         return __main(argc, argv);
> }
>
> void kvm_arch_main(void)
> {
>         is_cpu_amd = cpu_vendor_string_is("AuthenticAMD");
>         is_cpu_intel = cpu_vendor_string_is("AuthenticAMD");
> }
>
>
> And then we just need macro magic to emit the right VMCALL/VMMCALL instruction.

Thanks Sean for the feedback here. I have posted a separate series
addressing your comments:
https://lore.kernel.org/all/20220903012849.938069-4-vannapurve@google.com/T/

Regards,
Vishal

Patchew 2.1-devel-ea86937

© 2016 - 2026 Red Hat, Inc.