On Wed, Aug 24, 2022 at 5:07 PM Sean Christopherson <seanjc@xxxxxxxxxx> 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@xxxxxxxxxx>
> > ---
> > .../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@xxxxxxxxxx/T/
Regards,
Vishal
