| 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
| ||
|
Message-ID:
<SN6PR02MB4157DF6CA5F5597A0A4F78F8D4802@SN6PR02MB4157.namprd02.prod.outlook.com>
Date: Tue, 29 Apr 2025 20:36:03 +0000
From: Michael Kelley <mhklinux@...look.com>
To: Peter Zijlstra <peterz@...radead.org>
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()
From: Peter Zijlstra <peterz@...radead.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
Powered by blists - more mailing lists