| 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
| ||
|
Date: Tue, 17 Aug 2021 14:50:47 +0200
From: Juergen Gross <jgross@...e.com>
To: Borislav Petkov <bp@...en8.de>,
Kuppuswamy Sathyanarayanan
<sathyanarayanan.kuppuswamy@...ux.intel.com>
Cc: Thomas Gleixner <tglx@...utronix.de>,
Ingo Molnar <mingo@...hat.com>,
Peter Zijlstra <peterz@...radead.org>,
Andy Lutomirski <luto@...nel.org>,
Peter H Anvin <hpa@...or.com>,
Dave Hansen <dave.hansen@...el.com>,
Tony Luck <tony.luck@...el.com>,
Dan Williams <dan.j.williams@...el.com>,
Andi Kleen <ak@...ux.intel.com>,
Kirill Shutemov <kirill.shutemov@...ux.intel.com>,
Sean Christopherson <seanjc@...gle.com>,
Kuppuswamy Sathyanarayanan <knsathya@...nel.org>,
x86@...nel.org, linux-kernel@...r.kernel.org,
Deep Shah <sdeep@...are.com>,
"VMware, Inc." <pv-drivers@...are.com>
Subject: Re: [PATCH v5 01/12] x86/paravirt: Move halt paravirt calls under
CONFIG_PARAVIRT
On 12.08.21 09:18, Borislav Petkov wrote:
> On Wed, Aug 04, 2021 at 11:13:18AM -0700, Kuppuswamy Sathyanarayanan wrote:
>> From: "Kirill A. Shutemov" <kirill.shutemov@...ux.intel.com>
>>
>> CONFIG_PARAVIRT_XXL is mainly defined/used by XEN PV guests. For
>> other VM guest types, features supported under CONFIG_PARAVIRT
>> are self sufficient. CONFIG_PARAVIRT mainly provides support for
>> TLB flush operations and time related operations.
>>
>> For TDX guest as well, paravirt calls under CONFIG_PARVIRT meets
>> most of its requirement except the need of HLT and SAFE_HLT
>> paravirt calls, which is currently defined under
>> COFNIG_PARAVIRT_XXL.
>>
>> Since enabling CONFIG_PARAVIRT_XXL is too bloated for TDX guest
>> like platforms, move HLT and SAFE_HLT paravirt calls under
>> CONFIG_PARAVIRT.
>>
>> Moving HLT and SAFE_HLT paravirt calls are not fatal and should not
>> break any functionality for current users of CONFIG_PARAVIRT.
>>
>> Co-developed-by: Kuppuswamy Sathyanarayanan <sathyanarayanan.kuppuswamy@...ux.intel.com>
>> Signed-off-by: Kuppuswamy Sathyanarayanan <sathyanarayanan.kuppuswamy@...ux.intel.com>
>> Signed-off-by: Kirill A. Shutemov <kirill.shutemov@...ux.intel.com>
>> Reviewed-by: Andi Kleen <ak@...ux.intel.com>
>> Reviewed-by: Tony Luck <tony.luck@...el.com>
>> ---
>
> You need to do this before sending your patches:
>
> ./scripts/get_maintainer.pl /tmp/tdx.01
> Thomas Gleixner <tglx@...utronix.de> (maintainer:X86 ARCHITECTURE (32-BIT AND 64-BIT),commit_signer:1/6=17%)
> Ingo Molnar <mingo@...hat.com> (maintainer:X86 ARCHITECTURE (32-BIT AND 64-BIT))
> Borislav Petkov <bp@...en8.de> (maintainer:X86 ARCHITECTURE (32-BIT AND 64-BIT),commit_signer:6/6=100%)
> x86@...nel.org (maintainer:X86 ARCHITECTURE (32-BIT AND 64-BIT))
> "H. Peter Anvin" <hpa@...or.com> (reviewer:X86 ARCHITECTURE (32-BIT AND 64-BIT))
> Juergen Gross <jgross@...e.com> (supporter:PARAVIRT_OPS INTERFACE,commit_signer:5/6=83%,authored:5/6=83%,added_lines:15/16=94%,removed_lines:38/39=97%)
> Deep Shah <sdeep@...are.com> (supporter:PARAVIRT_OPS INTERFACE)
> "VMware, Inc." <pv-drivers@...are.com> (supporter:PARAVIRT_OPS INTERFACE)
> ...
>
> and CC also the supporters - I'm pretty sure at least Juergen would like
> to be kept up-to-date on pv changes. I'll CC him and the others now and
> leave in the whole diff but make sure you do that in the future please.
>
>> arch/x86/include/asm/irqflags.h | 40 +++++++++++++++------------
>> arch/x86/include/asm/paravirt.h | 20 +++++++-------
>> arch/x86/include/asm/paravirt_types.h | 3 +-
>> arch/x86/kernel/paravirt.c | 4 ++-
>> 4 files changed, 36 insertions(+), 31 deletions(-)
>>
>> diff --git a/arch/x86/include/asm/irqflags.h b/arch/x86/include/asm/irqflags.h
>> index c5ce9845c999..f3bb33b1715d 100644
>> --- a/arch/x86/include/asm/irqflags.h
>> +++ b/arch/x86/include/asm/irqflags.h
>> @@ -59,6 +59,28 @@ static inline __cpuidle void native_halt(void)
>>
>> #endif
>>
>> +#ifndef CONFIG_PARAVIRT
>> +#ifndef __ASSEMBLY__
>> +/*
>> + * Used in the idle loop; sti takes one instruction cycle
>> + * to complete:
>> + */
>> +static inline __cpuidle void arch_safe_halt(void)
>> +{
>> + native_safe_halt();
>> +}
>> +
>> +/*
>> + * Used when interrupts are already enabled or to
>> + * shutdown the processor:
>> + */
>> +static inline __cpuidle void halt(void)
>> +{
>> + native_halt();
>> +}
>> +#endif /* __ASSEMBLY__ */
>> +#endif /* CONFIG_PARAVIRT */
>> +
>> #ifdef CONFIG_PARAVIRT_XXL
>> #include <asm/paravirt.h>
Did you test this with CONFIG_PARAVIRT enabled and CONFIG_PARAVIRT_XXL
disabled?
I'm asking because in this case I don't see where halt() and
arch_safe_halt() would be defined in case someone is including
asm/irqflags.h and not asm/paravirt.h.
>> #else
>> @@ -80,24 +102,6 @@ static __always_inline void arch_local_irq_enable(void)
>> native_irq_enable();
>> }
>>
>> -/*
>> - * Used in the idle loop; sti takes one instruction cycle
>> - * to complete:
>> - */
>> -static inline __cpuidle void arch_safe_halt(void)
>> -{
>> - native_safe_halt();
>> -}
>> -
>> -/*
>> - * Used when interrupts are already enabled or to
>> - * shutdown the processor:
>> - */
>> -static inline __cpuidle void halt(void)
>> -{
>> - native_halt();
>> -}
>> -
>> /*
>> * For spinlocks, etc:
>> */
>> diff --git a/arch/x86/include/asm/paravirt.h b/arch/x86/include/asm/paravirt.h
>> index da3a1ac82be5..d323a626c7a8 100644
>> --- a/arch/x86/include/asm/paravirt.h
>> +++ b/arch/x86/include/asm/paravirt.h
>> @@ -97,6 +97,16 @@ static inline void paravirt_arch_exit_mmap(struct mm_struct *mm)
>> PVOP_VCALL1(mmu.exit_mmap, mm);
>> }
>>
>> +static inline void arch_safe_halt(void)
>> +{
>> + PVOP_VCALL0(irq.safe_halt);
>> +}
>> +
>> +static inline void halt(void)
>> +{
>> + PVOP_VCALL0(irq.halt);
>> +}
>> +
>> #ifdef CONFIG_PARAVIRT_XXL
>> static inline void load_sp0(unsigned long sp0)
>> {
>> @@ -162,16 +172,6 @@ static inline void __write_cr4(unsigned long x)
>> PVOP_VCALL1(cpu.write_cr4, x);
>> }
>>
>> -static inline void arch_safe_halt(void)
>> -{
>> - PVOP_VCALL0(irq.safe_halt);
>> -}
>> -
>> -static inline void halt(void)
>> -{
>> - PVOP_VCALL0(irq.halt);
>> -}
>> -
>> static inline void wbinvd(void)
>> {
>> PVOP_ALT_VCALL0(cpu.wbinvd, "wbinvd", ALT_NOT(X86_FEATURE_XENPV));
>> diff --git a/arch/x86/include/asm/paravirt_types.h b/arch/x86/include/asm/paravirt_types.h
>> index d9d6b0203ec4..40082847f314 100644
>> --- a/arch/x86/include/asm/paravirt_types.h
>> +++ b/arch/x86/include/asm/paravirt_types.h
>> @@ -150,10 +150,9 @@ struct pv_irq_ops {
>> struct paravirt_callee_save save_fl;
>> struct paravirt_callee_save irq_disable;
>> struct paravirt_callee_save irq_enable;
>> -
>> +#endif
>> void (*safe_halt)(void);
>> void (*halt)(void);
>> -#endif
>> } __no_randomize_layout;
>>
>> struct pv_mmu_ops {
>> diff --git a/arch/x86/kernel/paravirt.c b/arch/x86/kernel/paravirt.c
>> index 04cafc057bed..124e0f6c5d1c 100644
>> --- a/arch/x86/kernel/paravirt.c
>> +++ b/arch/x86/kernel/paravirt.c
>> @@ -283,9 +283,11 @@ struct paravirt_patch_template pv_ops = {
>> .irq.save_fl = __PV_IS_CALLEE_SAVE(native_save_fl),
>> .irq.irq_disable = __PV_IS_CALLEE_SAVE(native_irq_disable),
>> .irq.irq_enable = __PV_IS_CALLEE_SAVE(native_irq_enable),
>> +#endif /* CONFIG_PARAVIRT_XXL */
>> +
>> + /* Irq HLT ops. */
>
> What's that comment for?
I agree, please drop it.
Juergen
Download attachment "OpenPGP_0xB0DE9DD628BF132F.asc" of type "application/pgp-keys" (3092 bytes)
Download attachment "OpenPGP_signature" of type "application/pgp-signature" (496 bytes)
Powered by blists - more mailing lists