From: Peter Zijlstra <pet...@infradead.org> Sent: Tuesday, April 29, 2025 8:18
AM
>
> On Mon, Apr 21, 2025 at 06:27:57PM +0000, Michael Kelley wrote:
>
> > > @@ -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?
> >
> > > } else if (hv_get_isolation_type() == HV_ISOLATION_TYPE_TDX) {
> > > static_branch_enable(&isolation_type_tdx);
> > >
> > > @@ -498,6 +501,9 @@ static void __init ms_hyperv_init_platfo
> > > ms_hyperv.hints &= ~HV_X64_APIC_ACCESS_RECOMMENDED;
> > >
> > > if (!ms_hyperv.paravisor_present) {
> > > +#if defined(CONFIG_INTEL_TDX_GUEST) && defined(CONFIG_HYPERV)
> > > + static_call_update(hv_hypercall,
> > > hv_tdx_hypercall);
> > > +#endif
> > > /*
> > > * Mark the Hyper-V TSC page feature as disabled
> > > * in a TDX VM without paravisor so that the
> > >
> > >
>
> I've ended up with the below.. I thought it a waste to make all that
> stuff available to 32bit and !HYPERV.
>
>
> --- a/arch/x86/hyperv/ivm.c
> +++ b/arch/x86/hyperv/ivm.c
> @@ -392,6 +392,7 @@ u64 hv_snp_hypercall(u64 control, u64 pa
> #else
> static inline void hv_ghcb_msr_write(u64 msr, u64 value) {}
> static inline void hv_ghcb_msr_read(u64 msr, u64 *value) {}
> +u64 hv_snp_hypercall(u64 control, u64 param1, u64 param2) {}
> #endif /* CONFIG_AMD_MEM_ENCRYPT */
>
> #ifdef CONFIG_INTEL_TDX_GUEST
> @@ -441,6 +442,7 @@ u64 hv_tdx_hypercall(u64 control, u64 pa
> #else
> static inline void hv_tdx_msr_write(u64 msr, u64 value) {}
> static inline void hv_tdx_msr_read(u64 msr, u64 *value) {}
> +u64 hv_tdx_hypercall(u64 control, u64 param1, u64 param2) {}
> #endif /* CONFIG_INTEL_TDX_GUEST */
>
> #if defined(CONFIG_AMD_MEM_ENCRYPT) || defined(CONFIG_INTEL_TDX_GUEST)
> --- a/arch/x86/include/asm/mshyperv.h
> +++ b/arch/x86/include/asm/mshyperv.h
> @@ -39,6 +39,10 @@ static inline unsigned char hv_get_nmi_r
> return 0;
> }
>
> +extern u64 hv_tdx_hypercall(u64 control, u64 param1, u64 param2);
> +extern u64 hv_snp_hypercall(u64 control, u64 param1, u64 param2);
> +extern u64 hv_std_hypercall(u64 control, u64 param1, u64 param2);
> +
> #if IS_ENABLED(CONFIG_HYPERV)
> extern void *hv_hypercall_pg;
>
> @@ -48,10 +52,6 @@ bool hv_isolation_type_snp(void);
> bool hv_isolation_type_tdx(void);
>
> #ifdef CONFIG_X86_64
> -extern u64 hv_tdx_hypercall(u64 control, u64 param1, u64 param2);
> -extern u64 hv_snp_hypercall(u64 control, u64 param1, u64 param2);
> -extern u64 hv_std_hypercall(u64 control, u64 param1, u64 param2);
> -
> DECLARE_STATIC_CALL(hv_hypercall, hv_std_hypercall);
> #endif
>
> --- a/arch/x86/kernel/cpu/mshyperv.c
> +++ b/arch/x86/kernel/cpu/mshyperv.c
> @@ -287,9 +287,14 @@ static void __init x86_setup_ops_for_tsc
> #ifdef CONFIG_X86_64
> DEFINE_STATIC_CALL(hv_hypercall, hv_std_hypercall);
> EXPORT_STATIC_CALL_TRAMP_GPL(hv_hypercall);
> +#define hypercall_update(hc) static_call_update(hv_hypercall, hc)
> #endif
> #endif /* CONFIG_HYPERV */
>
> +#ifndef hypercall_update
> +#define hypercall_update(hc) (void)hc
> +#endif
> +
> static uint32_t __init ms_hyperv_platform(void)
> {
> u32 eax;
> @@ -490,10 +495,8 @@ static void __init ms_hyperv_init_platfo
>
> 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
> + hypercall_update(hv_snp_hypercall);
> } else if (hv_get_isolation_type() == HV_ISOLATION_TYPE_TDX) {
> static_branch_enable(&isolation_type_tdx);
>
> @@ -501,9 +504,7 @@ static void __init ms_hyperv_init_platfo
> ms_hyperv.hints &= ~HV_X64_APIC_ACCESS_RECOMMENDED;
>
> if (!ms_hyperv.paravisor_present) {
> -#if defined(CONFIG_INTEL_TDX_GUEST) && defined(CONFIG_HYPERV)
> - static_call_update(hv_hypercall,
> hv_tdx_hypercall);
> -#endif
> + hypercall_update(hv_tdx_hypercall);
> /*
> * Mark the Hyper-V TSC page feature as disabled
> * in a TDX VM without paravisor so that the
Yes, that's a reasonable improvement that I can live with. This source code file
is certainly not a model for avoiding ugly #ifdef's, but your new approach
avoids
adding to the problem quite so egregiously.
Michael
