Re: [PATCH v6 1/5] x86/kvm: Add AMD SEV specific Hypercall3

From: Borislav Petkov <bp@alien8.de>
To: Sean Christopherson <seanjc@google.com>
Cc: Ashish Kalra <ashish.kalra@amd.com>,
	Steve Rutherford <srutherford@google.com>,
	pbonzini@redhat.com, tglx@linutronix.de, mingo@redhat.com,
	hpa@zytor.com, joro@8bytes.org, thomas.lendacky@amd.com,
	x86@kernel.org, kvm@vger.kernel.org,
	linux-kernel@vger.kernel.org, brijesh.singh@amd.com,
	dovmurik@linux.ibm.com, tobin@linux.ibm.com, jejb@linux.ibm.com,
	dgilbert@redhat.com
Subject: Re: [PATCH v6 1/5] x86/kvm: Add AMD SEV specific Hypercall3
Date: Wed, 22 Sep 2021 11:38:08 +0200	[thread overview]
Message-ID: <YUr5gCgNe7tT0U/+@zn.tnic> (raw)
In-Reply-To: <YUoDJxfNZgNjY8zh@google.com>

On Tue, Sep 21, 2021 at 04:07:03PM +0000, Sean Christopherson wrote:
> init_hypervisor_platform(), after x86_init.hyper.init_platform() so that the
> PV support can set the desired feature flags.  Since kvm_hypercall*() is only
> used by KVM guests, set_cpu_cap(c, X86_FEATURE_VMMCALL) can be moved out of
> early_init_amd/hygon() and into kvm_init_platform().

See below.

> Another option would be to refactor apply_alternatives() to allow
> the caller to provide a different feature check mechanism than
> boot_cpu_has(), which I think would let us drop X86_FEATURE_VMMCALL,
> X86_FEATURE_VMCALL, and X86_FEATURE_VMW_VMMCALL from cpufeatures. That
> might get more than a bit gross though.

Uuuf.

So here's what I'm seeing (line numbers given to show when stuff
happens):

start_kernel
|-> 953: setup_arch
    |-> 794: early_cpu_init
    |-> 936: init_hypervisor_platform
|
|-> 1134: check_bugs
	  |-> alternative_instructions

at line 794 setup_arch() calls early_cpu_init() which would end up
setting X86_FEATURE_VMMCALL on an AMD guest, based on CPUID information.

init_hypervisor_platform() happens after that.

The alternatives patching happens in check_bugs() at line 1134. Which
means, if one would consider moving the patching up, one would have
to audit all the code between line 953 and 1134, whether it does
set_cpu_cap() or some of the other helpers to set or clear bits in
boot_cpu_data which controls the patching.

So for that I have only one thing to say: can'o'worms. We tried to move
the memblock allocations placement in the boot process and generated at
least 4 regressions. I'm still testing the fix for the fix for the 4th
regression.

So moving stuff in the fragile boot process makes my hair stand up.

Refactoring apply_alternatives() to patch only for X86_FEATURE_VMMCALL
and then patch again, I dunno, this stuff is fragile and it might cause
some other similarly nasty fallout. And those are hard to debug because
one does not see immediately when boot_cpu_data features are missing and
functionality is behaving differently because of that.

So what's wrong with:

kvm_hypercall3:

	if (cpu_feature_enabled(X86_FEATURE_VMMCALL))
		return kvm_sev_hypercall3(...);

	/* rest of code */

?

Dunno we probably had that already in those old versions and maybe that
was shot down for another reason but it should get you what you want
without having to test the world and more for regressions possibly
happening from disturbing the house of cards called x86 boot order.

IMHO, I'd say.

-- 
Regards/Gruss,
    Boris.

https://people.kernel.org/tglx/notes-about-netiquette