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" >> >> 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 >> Signed-off-by: Kuppuswamy Sathyanarayanan >> Signed-off-by: Kirill A. Shutemov >> Reviewed-by: Andi Kleen >> Reviewed-by: Tony Luck >> --- > > You need to do this before sending your patches: > > ./scripts/get_maintainer.pl /tmp/tdx.01 > Thomas Gleixner (maintainer:X86 ARCHITECTURE (32-BIT AND 64-BIT),commit_signer:1/6=17%) > Ingo Molnar (maintainer:X86 ARCHITECTURE (32-BIT AND 64-BIT)) > Borislav Petkov (maintainer:X86 ARCHITECTURE (32-BIT AND 64-BIT),commit_signer:6/6=100%) > x86@kernel.org (maintainer:X86 ARCHITECTURE (32-BIT AND 64-BIT)) > "H. Peter Anvin" (reviewer:X86 ARCHITECTURE (32-BIT AND 64-BIT)) > Juergen Gross (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 (supporter:PARAVIRT_OPS INTERFACE) > "VMware, Inc." (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 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