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: <Ywa9T+jKUpaHLu/l@google.com>
Date:   Thu, 25 Aug 2022 00:07:43 +0000
From:   Sean Christopherson <seanjc@...gle.com>
To:     Vishal Annapurve <vannapurve@...gle.com>
Cc:     x86@...nel.org, kvm@...r.kernel.org, linux-kernel@...r.kernel.org,
        linux-kselftest@...r.kernel.org, pbonzini@...hat.com,
        vkuznets@...hat.com, wanpengli@...cent.com, jmattson@...gle.com,
        joro@...tes.org, tglx@...utronix.de, mingo@...hat.com,
        bp@...en8.de, dave.hansen@...ux.intel.com, hpa@...or.com,
        shuah@...nel.org, yang.zhong@...el.com, drjones@...hat.com,
        ricarkol@...gle.com, aaronlewis@...gle.com, wei.w.wang@...el.com,
        kirill.shutemov@...ux.intel.com, corbet@....net, hughd@...gle.com,
        jlayton@...nel.org, bfields@...ldses.org,
        akpm@...ux-foundation.org, chao.p.peng@...ux.intel.com,
        yu.c.zhang@...ux.intel.com, jun.nakajima@...el.com,
        dave.hansen@...el.com, michael.roth@....com, qperret@...gle.com,
        steven.price@....com, ak@...ux.intel.com, david@...hat.com,
        luto@...nel.org, vbabka@...e.cz, marcorr@...gle.com,
        erdemaktas@...gle.com, pgonda@...gle.com, nikunj@....com,
        diviness@...gle.com, maz@...nel.org, dmatlack@...gle.com,
        axelrasmussen@...gle.com, maciej.szmigiero@...cle.com,
        mizhang@...gle.com, bgardon@...gle.com
Subject: Re: [RFC V3 PATCH 4/6] selftests: kvm: x86: Execute hypercall as per
 the cpu

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.