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.
