Re: [PATCH v5 01/12] x86/paravirt: Move halt paravirt calls under CONFIG_PARAVIRT

From: Juergen Gross
Date: Tue Aug 17 2021 - 09:29:11 EST


On 17.08.21 15:16, Kuppuswamy, Sathyanarayanan wrote:


On 8/17/21 5:50 AM, Juergen Gross wrote:
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@xxxxxxxxxxxxxxx>

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@xxxxxxxxxxxxxxx>
Signed-off-by: Kuppuswamy Sathyanarayanan <sathyanarayanan.kuppuswamy@xxxxxxxxxxxxxxx>
Signed-off-by: Kirill A. Shutemov <kirill.shutemov@xxxxxxxxxxxxxxx>
Reviewed-by: Andi Kleen <ak@xxxxxxxxxxxxxxx>
Reviewed-by: Tony Luck <tony.luck@xxxxxxxxx>
---

You need to do this before sending your patches:

./scripts/get_maintainer.pl /tmp/tdx.01
Thomas Gleixner <tglx@xxxxxxxxxxxxx> (maintainer:X86 ARCHITECTURE (32-BIT AND 64-BIT),commit_signer:1/6=17%)
Ingo Molnar <mingo@xxxxxxxxxx> (maintainer:X86 ARCHITECTURE (32-BIT AND 64-BIT))
Borislav Petkov <bp@xxxxxxxxx> (maintainer:X86 ARCHITECTURE (32-BIT AND 64-BIT),commit_signer:6/6=100%)
x86@xxxxxxxxxx (maintainer:X86 ARCHITECTURE (32-BIT AND 64-BIT))
"H. Peter Anvin" <hpa@xxxxxxxxx> (reviewer:X86 ARCHITECTURE (32-BIT AND 64-BIT))
Juergen Gross <jgross@xxxxxxxx> (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@xxxxxxxxxx> (supporter:PARAVIRT_OPS INTERFACE)
"VMware, Inc." <pv-drivers@xxxxxxxxxx> (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.

We have tested both cases and did not hit any issues.

1. CONFIG_PARAVIRT=y and  CONFIG_PARAVIRT_XXL=y
2. CONFIG_PARAVIRT=y and  CONFIG_PARAVIRT_XXL=n

I guess you have been lucky and all users of arch_safe_halt() and halt()
are directly or indirectly including asm/paravirt.h by other means.

There might be configs where this is not true, though.

In any case I believe you should fix your patch to let asm/irqflags.h
include asm/paravirt.h for the CONFIG_PARAVIRT case.


Juergen

Attachment: OpenPGP_0xB0DE9DD628BF132F.asc
Description: OpenPGP public key

Attachment: OpenPGP_signature
Description: OpenPGP digital signature