From: "Kirill A. Shutemov" kirill.shutemov@linux.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 CONFIG_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.
Cc: stable@vger.kernel.org Fixes: bfe6ed0c6727 ("x86/tdx: Add HLT support for TDX guests") Co-developed-by: Kuppuswamy Sathyanarayanan sathyanarayanan.kuppuswamy@linux.intel.com Signed-off-by: Kuppuswamy Sathyanarayanan sathyanarayanan.kuppuswamy@linux.intel.com Signed-off-by: Kirill A. Shutemov kirill.shutemov@linux.intel.com Reviewed-by: Andi Kleen ak@linux.intel.com Reviewed-by: Tony Luck tony.luck@intel.com Signed-off-by: Vishal Annapurve vannapurve@google.com --- 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 | 14 ++++++---- 4 files changed, 41 insertions(+), 36 deletions(-)
diff --git a/arch/x86/include/asm/irqflags.h b/arch/x86/include/asm/irqflags.h index cf7fc2b8e3ce..1c2db11a2c3c 100644 --- a/arch/x86/include/asm/irqflags.h +++ b/arch/x86/include/asm/irqflags.h @@ -76,6 +76,28 @@ static __always_inline void native_local_irq_restore(unsigned long flags)
#endif
+#ifndef CONFIG_PARAVIRT +#ifndef __ASSEMBLY__ +/* + * Used in the idle loop; sti takes one instruction cycle + * to complete: + */ +static __always_inline void arch_safe_halt(void) +{ + native_safe_halt(); +} + +/* + * Used when interrupts are already enabled or to + * shutdown the processor: + */ +static __always_inline void halt(void) +{ + native_halt(); +} +#endif /* __ASSEMBLY__ */ +#endif /* CONFIG_PARAVIRT */ + #ifdef CONFIG_PARAVIRT_XXL #include <asm/paravirt.h> #else @@ -97,24 +119,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 __always_inline void arch_safe_halt(void) -{ - native_safe_halt(); -} - -/* - * Used when interrupts are already enabled or to - * shutdown the processor: - */ -static __always_inline 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 041aff51eb50..29e7331a0c98 100644 --- a/arch/x86/include/asm/paravirt.h +++ b/arch/x86/include/asm/paravirt.h @@ -107,6 +107,16 @@ static inline void notify_page_enc_status_changed(unsigned long pfn, PVOP_VCALL3(mmu.notify_page_enc_status_changed, pfn, npages, enc); }
+static __always_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) { @@ -170,16 +180,6 @@ static inline void __write_cr4(unsigned long x) PVOP_VCALL1(cpu.write_cr4, x); }
-static __always_inline void arch_safe_halt(void) -{ - PVOP_VCALL0(irq.safe_halt); -} - -static inline void halt(void) -{ - PVOP_VCALL0(irq.halt); -} - static inline u64 paravirt_read_msr(unsigned msr) { return PVOP_CALL1(u64, cpu.read_msr, msr); diff --git a/arch/x86/include/asm/paravirt_types.h b/arch/x86/include/asm/paravirt_types.h index fea56b04f436..abccfccc2e3f 100644 --- a/arch/x86/include/asm/paravirt_types.h +++ b/arch/x86/include/asm/paravirt_types.h @@ -120,10 +120,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 1ccaa3397a67..c5bb980b8a67 100644 --- a/arch/x86/kernel/paravirt.c +++ b/arch/x86/kernel/paravirt.c @@ -110,6 +110,11 @@ int paravirt_disable_iospace(void) return request_resource(&ioport_resource, &reserve_ioports); }
+static noinstr void pv_native_safe_halt(void) +{ + native_safe_halt(); +} + #ifdef CONFIG_PARAVIRT_XXL static noinstr void pv_native_write_cr2(unsigned long val) { @@ -125,11 +130,6 @@ static noinstr void pv_native_set_debugreg(int regno, unsigned long val) { native_set_debugreg(regno, val); } - -static noinstr void pv_native_safe_halt(void) -{ - native_safe_halt(); -} #endif
struct pv_info pv_info = { @@ -186,9 +186,11 @@ struct paravirt_patch_template pv_ops = { .irq.save_fl = __PV_IS_CALLEE_SAVE(pv_native_save_fl), .irq.irq_disable = __PV_IS_CALLEE_SAVE(pv_native_irq_disable), .irq.irq_enable = __PV_IS_CALLEE_SAVE(pv_native_irq_enable), +#endif /* CONFIG_PARAVIRT_XXL */ + + /* Irq HLT ops. */ .irq.safe_halt = pv_native_safe_halt, .irq.halt = native_halt, -#endif /* CONFIG_PARAVIRT_XXL */
/* Mmu ops. */ .mmu.flush_tlb_user = native_flush_tlb_local,
On 2/20/25 13:16, Vishal Annapurve wrote:
Since enabling CONFIG_PARAVIRT_XXL is too bloated for TDX guest like platforms, move HLT and SAFE_HLT paravirt calls under CONFIG_PARAVIRT.
I guess it's just one patch, but doesn't this expose CONFIG_PARAVIRT=y users to what _was_ specific to CONFIG_PARAVIRT_XXL=y? According to the changelog, TDX users shouldn't have to use use PARAVIRT_XXL, so PARAVIRT=y and PARAVIRT_XXL=n must be an *IMPORTANT* configuration for TDX users.
Before this patch, those users would have no way to hit the unsafe-for-TDX pv_native_safe_halt(). After this patch, they will hit it.
So, there are two possibilities:
1. This patch breaks bisection for an important TDX configuration 2. This patch's conjecture that PARAVIRT_XXL=n is important for TDX is wrong and it is not necessary in the first place.
What am I missing?
On Thu, Feb 20, 2025 at 1:47 PM Dave Hansen dave.hansen@intel.com wrote:
On 2/20/25 13:16, Vishal Annapurve wrote:
Since enabling CONFIG_PARAVIRT_XXL is too bloated for TDX guest like platforms, move HLT and SAFE_HLT paravirt calls under CONFIG_PARAVIRT.
I guess it's just one patch, but doesn't this expose CONFIG_PARAVIRT=y users to what _was_ specific to CONFIG_PARAVIRT_XXL=y? According to the changelog, TDX users shouldn't have to use use PARAVIRT_XXL, so PARAVIRT=y and PARAVIRT_XXL=n must be an *IMPORTANT* configuration for TDX users.
Before this patch, those users would have no way to hit the unsafe-for-TDX pv_native_safe_halt(). After this patch, they will hit it.
Before this patch, those users had access to arch_safe_halt() -> native_safe_halt() path. With this patch, such users can execute arch_safe_halt -> pv_native_safe_halt() -> native_safe_halt(), so this patch doesn't cause any additional regression.
So, there are two possibilities:
- This patch breaks bisection for an important TDX configuration
- This patch's conjecture that PARAVIRT_XXL=n is important for TDX is wrong and it is not necessary in the first place.
What am I missing?
On Thu, Feb 20, 2025 at 09:16:25PM +0000, Vishal Annapurve wrote:
From: "Kirill A. Shutemov" kirill.shutemov@linux.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 CONFIG_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.
Could you use the bloat-o-meter to give an idea of the savings?
Also .. aren't most distros building with Xen support so they will always have the full paravirt support?
Moving HLT and SAFE_HLT paravirt calls are not fatal and should not break any functionality for current users of CONFIG_PARAVIRT.
Cc: stable@vger.kernel.org Fixes: bfe6ed0c6727 ("x86/tdx: Add HLT support for TDX guests") Co-developed-by: Kuppuswamy Sathyanarayanan sathyanarayanan.kuppuswamy@linux.intel.com Signed-off-by: Kuppuswamy Sathyanarayanan sathyanarayanan.kuppuswamy@linux.intel.com Signed-off-by: Kirill A. Shutemov kirill.shutemov@linux.intel.com Reviewed-by: Andi Kleen ak@linux.intel.com Reviewed-by: Tony Luck tony.luck@intel.com Signed-off-by: Vishal Annapurve vannapurve@google.com
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 | 14 ++++++---- 4 files changed, 41 insertions(+), 36 deletions(-)
diff --git a/arch/x86/include/asm/irqflags.h b/arch/x86/include/asm/irqflags.h index cf7fc2b8e3ce..1c2db11a2c3c 100644 --- a/arch/x86/include/asm/irqflags.h +++ b/arch/x86/include/asm/irqflags.h @@ -76,6 +76,28 @@ static __always_inline void native_local_irq_restore(unsigned long flags) #endif +#ifndef CONFIG_PARAVIRT +#ifndef __ASSEMBLY__ +/*
- Used in the idle loop; sti takes one instruction cycle
- to complete:
- */
+static __always_inline void arch_safe_halt(void) +{
- native_safe_halt();
+}
+/*
- Used when interrupts are already enabled or to
- shutdown the processor:
- */
+static __always_inline void halt(void) +{
- native_halt();
+} +#endif /* __ASSEMBLY__ */ +#endif /* CONFIG_PARAVIRT */
#ifdef CONFIG_PARAVIRT_XXL #include <asm/paravirt.h> #else @@ -97,24 +119,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 __always_inline void arch_safe_halt(void) -{
- native_safe_halt();
-}
-/*
- Used when interrupts are already enabled or to
- shutdown the processor:
- */
-static __always_inline 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 041aff51eb50..29e7331a0c98 100644 --- a/arch/x86/include/asm/paravirt.h +++ b/arch/x86/include/asm/paravirt.h @@ -107,6 +107,16 @@ static inline void notify_page_enc_status_changed(unsigned long pfn, PVOP_VCALL3(mmu.notify_page_enc_status_changed, pfn, npages, enc); } +static __always_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) { @@ -170,16 +180,6 @@ static inline void __write_cr4(unsigned long x) PVOP_VCALL1(cpu.write_cr4, x); } -static __always_inline void arch_safe_halt(void) -{
- PVOP_VCALL0(irq.safe_halt);
-}
-static inline void halt(void) -{
- PVOP_VCALL0(irq.halt);
-}
static inline u64 paravirt_read_msr(unsigned msr) { return PVOP_CALL1(u64, cpu.read_msr, msr); diff --git a/arch/x86/include/asm/paravirt_types.h b/arch/x86/include/asm/paravirt_types.h index fea56b04f436..abccfccc2e3f 100644 --- a/arch/x86/include/asm/paravirt_types.h +++ b/arch/x86/include/asm/paravirt_types.h @@ -120,10 +120,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 1ccaa3397a67..c5bb980b8a67 100644 --- a/arch/x86/kernel/paravirt.c +++ b/arch/x86/kernel/paravirt.c @@ -110,6 +110,11 @@ int paravirt_disable_iospace(void) return request_resource(&ioport_resource, &reserve_ioports); } +static noinstr void pv_native_safe_halt(void) +{
- native_safe_halt();
+}
#ifdef CONFIG_PARAVIRT_XXL static noinstr void pv_native_write_cr2(unsigned long val) { @@ -125,11 +130,6 @@ static noinstr void pv_native_set_debugreg(int regno, unsigned long val) { native_set_debugreg(regno, val); }
-static noinstr void pv_native_safe_halt(void) -{
- native_safe_halt();
-} #endif struct pv_info pv_info = { @@ -186,9 +186,11 @@ struct paravirt_patch_template pv_ops = { .irq.save_fl = __PV_IS_CALLEE_SAVE(pv_native_save_fl), .irq.irq_disable = __PV_IS_CALLEE_SAVE(pv_native_irq_disable), .irq.irq_enable = __PV_IS_CALLEE_SAVE(pv_native_irq_enable), +#endif /* CONFIG_PARAVIRT_XXL */
- /* Irq HLT ops. */ .irq.safe_halt = pv_native_safe_halt, .irq.halt = native_halt,
-#endif /* CONFIG_PARAVIRT_XXL */ /* Mmu ops. */ .mmu.flush_tlb_user = native_flush_tlb_local, -- 2.48.1.601.g30ceb7b040-goog
On 28.02.25 22:47, Konrad Rzeszutek Wilk wrote:
On Thu, Feb 20, 2025 at 09:16:25PM +0000, Vishal Annapurve wrote:
From: "Kirill A. Shutemov" kirill.shutemov@linux.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 CONFIG_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.
Could you use the bloat-o-meter to give an idea of the savings?
Also .. aren't most distros building with Xen support so they will always have the full paravirt support?
Adding PARAVIRT_XXL users should be avoided if possible.
Main reason is that the work to make PVH dom0 fully functional compared to PV dom0 will make it possible to deprecate PV mode in the long run.
Juergen
linux-stable-mirror@lists.linaro.org
-
Dave Hansen -
Jürgen Groß -
Konrad Rzeszutek Wilk -
Vishal Annapurve