| Message ID | 20220819174659.2427983-5-vannapurve@google.com (mailing list archive) |
|---|---|
| State | New, archived |
| Headers | show |
| Series | selftests: KVM: selftests for fd-based private memory | expand |
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.
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
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; }
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(-)