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

lists.openwall.net		lists / announce owl-users owl-dev john-users john-dev passwdqc-users yescrypt popa3d-users / oss-security kernel-hardening musl sabotage tlsify passwords / crypt-dev xvendor / Bugtraq Full-Disclosure linux-kernel linux-netdev linux-ext4 linux-hardening linux-cve-announce PHC
Open Source and information security mailing list archives
Hash Suite: Windows password security audit tool. GUI, reports in PDF.
[<prev] [next>] [<thread-prev] [thread-next>] [day] [month] [year] [list]
Message-ID: <CAGtprH9zVDP4ur2mStDVAPU3M1MmvdgFQz7uTh6hAWjRL0Xbcw@mail.gmail.com>
Date:   Tue, 6 Sep 2022 15:48:27 -0700
From:   Vishal Annapurve <vannapurve@...gle.com>
To:     Sean Christopherson <seanjc@...gle.com>
Cc:     x86 <x86@...nel.org>, kvm list <kvm@...r.kernel.org>,
        LKML <linux-kernel@...r.kernel.org>,
        linux-kselftest@...r.kernel.org,
        Paolo Bonzini <pbonzini@...hat.com>,
        Vitaly Kuznetsov <vkuznets@...hat.com>,
        Wanpeng Li <wanpengli@...cent.com>,
        Jim Mattson <jmattson@...gle.com>,
        Joerg Roedel <joro@...tes.org>,
        Thomas Gleixner <tglx@...utronix.de>,
        Ingo Molnar <mingo@...hat.com>, Borislav Petkov <bp@...en8.de>,
        dave.hansen@...ux.intel.com, "H . Peter Anvin" <hpa@...or.com>,
        shuah <shuah@...nel.org>, yang.zhong@...el.com,
        drjones@...hat.com, Ricardo Koller <ricarkol@...gle.com>,
        Aaron Lewis <aaronlewis@...gle.com>, wei.w.wang@...el.com,
        "Kirill A . Shutemov" <kirill.shutemov@...ux.intel.com>,
        Jonathan Corbet <corbet@....net>,
        Hugh Dickins <hughd@...gle.com>,
        Jeff Layton <jlayton@...nel.org>,
        "J . Bruce Fields" <bfields@...ldses.org>,
        Andrew Morton <akpm@...ux-foundation.org>,
        Chao Peng <chao.p.peng@...ux.intel.com>,
        Yu Zhang <yu.c.zhang@...ux.intel.com>,
        Jun Nakajima <jun.nakajima@...el.com>,
        Dave Hansen <dave.hansen@...el.com>,
        Michael Roth <michael.roth@....com>,
        Quentin Perret <qperret@...gle.com>,
        Steven Price <steven.price@....com>,
        Andi Kleen <ak@...ux.intel.com>,
        David Hildenbrand <david@...hat.com>,
        Andy Lutomirski <luto@...nel.org>,
        Vlastimil Babka <vbabka@...e.cz>,
        Marc Orr <marcorr@...gle.com>,
        Erdem Aktas <erdemaktas@...gle.com>,
        Peter Gonda <pgonda@...gle.com>,
        "Nikunj A. Dadhania" <nikunj@....com>,
        Austin Diviness <diviness@...gle.com>, maz@...nel.org,
        David Matlack <dmatlack@...gle.com>,
        Axel Rasmussen <axelrasmussen@...gle.com>,
        maciej.szmigiero@...cle.com, Mingwei Zhang <mizhang@...gle.com>,
        Ben Gardon <bgardon@...gle.com>
Subject: Re: [RFC V3 PATCH 4/6] selftests: kvm: x86: Execute hypercall as per
 the cpu

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