linux-kernel - Re: [PATCH 4/6] x86,hyperv: Clean up hv_do

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: <20250425135022.GB35881@noisy.programming.kicks-ass.net>
Date: Fri, 25 Apr 2025 15:50:22 +0200
From: Peter Zijlstra <peterz@...radead.org>
To: Michael Kelley <mhklinux@...look.com>
Cc: "x86@...nel.org" <x86@...nel.org>,
	"kys@...rosoft.com" <kys@...rosoft.com>,
	"haiyangz@...rosoft.com" <haiyangz@...rosoft.com>,
	"wei.liu@...nel.org" <wei.liu@...nel.org>,
	"decui@...rosoft.com" <decui@...rosoft.com>,
	"tglx@...utronix.de" <tglx@...utronix.de>,
	"mingo@...hat.com" <mingo@...hat.com>,
	"bp@...en8.de" <bp@...en8.de>,
	"dave.hansen@...ux.intel.com" <dave.hansen@...ux.intel.com>,
	"hpa@...or.com" <hpa@...or.com>,
	"jpoimboe@...nel.org" <jpoimboe@...nel.org>,
	"pawan.kumar.gupta@...ux.intel.com" <pawan.kumar.gupta@...ux.intel.com>,
	"seanjc@...gle.com" <seanjc@...gle.com>,
	"pbonzini@...hat.com" <pbonzini@...hat.com>,
	"ardb@...nel.org" <ardb@...nel.org>,
	"kees@...nel.org" <kees@...nel.org>, Arnd Bergmann <arnd@...db.de>,
	"gregkh@...uxfoundation.org" <gregkh@...uxfoundation.org>,
	"linux-hyperv@...r.kernel.org" <linux-hyperv@...r.kernel.org>,
	"linux-kernel@...r.kernel.org" <linux-kernel@...r.kernel.org>,
	"kvm@...r.kernel.org" <kvm@...r.kernel.org>,
	"linux-efi@...r.kernel.org" <linux-efi@...r.kernel.org>,
	"samitolvanen@...gle.com" <samitolvanen@...gle.com>,
	"ojeda@...nel.org" <ojeda@...nel.org>
Subject: Re: [PATCH 4/6] x86,hyperv: Clean up hv_do_hypercall()

On Mon, Apr 21, 2025 at 06:27:57PM +0000, Michael Kelley wrote:
> From: Peter Zijlstra <peterz@...radead.org> Sent: Monday, April 14, 2025 4:12 AM
> > 
> > What used to be a simple few instructions has turned into a giant mess
> > (for x86_64). Not only does it use static_branch wrong, it mixes it
> > with dynamic branches for no apparent reason.
> > 
> > Notably it uses static_branch through an out-of-line function call,
> > which completely defeats the purpose, since instead of a simple
> > JMP/NOP site, you get a CALL+RET+TEST+Jcc sequence in return, which is
> > absolutely idiotic.
> > 
> > Add to that a dynamic test of hyperv_paravisor_present, something
> > which is set once and never changed.
> > 
> > Replace all this idiocy with a single direct function call to the
> > right hypercall variant.
> 
> This did indeed need cleaning after all the CoCo VM and paravisor
> stuff got added. Thanks for doing it.
> 
> From looking at the code changes, I believe the 32-bit hypercall paths
> are unchanged, as they weren't affected the CoCo VM and paravisor
> additions. Perhaps explicitly state that intent in the commit message.
> 
> I've tested this patch set against linux-next-20250411 on normal Hyper-V
> guests. Basic smoke tests pass, along with taking a panic, and
> suspend/resume for guest hibernation. But getting into kdump after a
> panic does not work. See comments in Patch 5 for the likely reason why.
> 
> I've also tested SNP and TDX VMs with a paravisor, and basic smoke
> tests pass. But I'm testing in the Azure cloud, and I don't have access to an
> environment where I can test without a paravisor. So my testing doesn't
> cover the SNP and TDX specific static call paths. Maybe someone at
> Microsoft can test that configuration.

Excellent, thanks!


> > +#ifdef CONFIG_X86_64
> > +u64 hv_pg_hypercall(u64 control, u64 param1, u64 param2)
> 
> Could this get a different name so we don't have the confusion of
> hv_hypercall_pg vs hv_pg_hypercall?  Some possibilities:
> 
> hv_std_hypercall
> hv_basic_hypercall
> hv_core_hypercall
> hv_normal_hypercall
> hv_simple_hypercall

Sure, I'll throw a dice an pick one ;-)


> > @@ -483,14 +484,16 @@ static void __init ms_hyperv_init_platfo
> >  			ms_hyperv.shared_gpa_boundary =
> >  				BIT_ULL(ms_hyperv.shared_gpa_boundary_bits);
> > 
> > -		hyperv_paravisor_present = !!ms_hyperv.paravisor_present;
> > -
> >  		pr_info("Hyper-V: Isolation Config: Group A 0x%x, Group B 0x%x\n",
> >  			ms_hyperv.isolation_config_a, ms_hyperv.isolation_config_b);
> > 
> > 
> >  		if (hv_get_isolation_type() == HV_ISOLATION_TYPE_SNP) {
> >  			static_branch_enable(&isolation_type_snp);
> > +#if defined(CONFIG_AMD_MEM_ENCRYPT) && defined(CONFIG_HYPERV)
> > +			if (!ms_hyperv.paravisor_present)
> > +				static_call_update(hv_hypercall, hv_snp_hypercall);
> > +#endif
> 
> This #ifdef (and one below for TDX) are really ugly. They could be avoided by adding
> stubs for hv_snp_hypercall() and hv_tdx_hypercall(), and making the hv_hypercall static
> call exist even when !CONFIG_HYPERV (and for 32-bit builds). Or is there a reason to
> not do that?

I'll try and make it so.