Re: [PATCH 4/6] x86,hyperv: Clean up hv_do_hypercall()

[Peter Zijlstra]

On Mon, Apr 21, 2025 at 06:27:57PM +0000, Michael Kelley wrote:
> From: Peter Zijlstra <peterz@xxxxxxxxxxxxx> 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.