Date: Tue,  6 Feb 2024 14:36:20 -0800
From: Adam Dunlap <acdunlap@...gle.com>
To: Thomas Gleixner <tglx@...utronix.de>, Ingo Molnar <mingo@...hat.com>, Borislav Petkov <bp@...en8.de>, 
	Dave Hansen <dave.hansen@...ux.intel.com>, x86@...nel.org, 
	"H. Peter Anvin" <hpa@...or.com>, Nathan Chancellor <nathan@...nel.org>, 
	Nick Desaulniers <ndesaulniers@...gle.com>, Bill Wendling <morbo@...gle.com>, 
	Justin Stitt <justinstitt@...gle.com>, "Peter Zijlstra (Intel)" <peterz@...radead.org>, 
	Arjan van de Ven <arjan@...ux.intel.com>, Wei Liu <wei.liu@...nel.org>, linux-kernel@...r.kernel.org, 
	llvm@...ts.linux.dev, Jacob Xu <jacobhxu@...gle.com>, Alper Gun <alpergun@...gle.com>, 
	Kevin Loughlin <kevinloughlin@...gle.com>, Peter Gonda <pgonda@...gle.com>, 
	Ard Biesheuvel <ardb@...nel.org>
Cc: Adam Dunlap <acdunlap@...gle.com>
Subject: [PATCH v3] x86/asm: Force native_apic_mem_read to use mov

When done from a virtual machine, instructions that touch APIC memory
must be emulated. By convention, MMIO access are typically performed via
io.h helpers such as 'readl()' or 'writeq()' to simplify instruction
emulation/decoding (ex: in KVM hosts and SEV guests) [0].

Currently, native_apic_mem_read does not follow this convention,
allowing the compiler to emit instructions other than the mov generated
by readl(). In particular, when compiled with clang and run as a SEV-ES
or SEV-SNP guest, the compiler would emit a testl instruction which is
not supported by the SEV-ES emulator, causing a boot failure in that
environment. It is likely the same problem would happen in a TDX guest
as that uses the same instruction emulator as SEV-ES.

To make sure all emulators can emulate APIC memory reads via mov, use
the readl function in native_apic_mem_read. It is expected that any
emulator would support mov in any addressing mode it is the most generic
and is what is ususally emitted currently.

The testl instruction is emitted when native_apic_mem_read
is inlined into __xapic_wait_icr_idle. The emulator comes from
insn_decode_mmio in arch/x86/lib/insn-eval.c. It would not be worth it
to extend insn_decode_mmio to support more instructions since, in
theory, the compiler could choose to output nearly any instruction for
such reads which would bloat the emulator beyond reason.

An alterative to this approach would be to use inline assembly instead
of the readl helper, as that is what native_apic_mem_write does. I
consider using readl to be cleaner since it is documented to be a simple
wrapper and inline assembly is less readable. native_apic_mem_write
cannot be trivially updated to use writel since it appears to use custom
asm to workaround for a processor-specific bug.

[0] https://lore.kernel.org/all/20220405232939.73860-12-kirill.shutemov@linux.intel.com/

Signed-off-by: Adam Dunlap <acdunlap@...gle.com>
Tested-by: Kevin Loughlin <kevinloughlin@...gle.com>
---

Patch changelog:
V1 -> V2: Replaced asm with readl function which does the same thing
V2 -> V3: Updated commit message to show more motivation and
justification

Link to v2 discussion: https://lore.kernel.org/all/20220908170456.3177635-1-acdunlap@google.com/

 arch/x86/include/asm/apic.h | 3 ++-
 1 file changed, 2 insertions(+), 1 deletion(-)

diff --git a/arch/x86/include/asm/apic.h b/arch/x86/include/asm/apic.h
index 9d159b771dc8..dddd3fc195ef 100644
--- a/arch/x86/include/asm/apic.h
+++ b/arch/x86/include/asm/apic.h
@@ -13,6 +13,7 @@
 #include <asm/mpspec.h>
 #include <asm/msr.h>
 #include <asm/hardirq.h>
+#include <asm/io.h>
 
 #define ARCH_APICTIMER_STOPS_ON_C3	1
 
@@ -96,7 +97,7 @@ static inline void native_apic_mem_write(u32 reg, u32 v)
 
 static inline u32 native_apic_mem_read(u32 reg)
 {
-	return *((volatile u32 *)(APIC_BASE + reg));
+	return readl((void __iomem *)(APIC_BASE + reg));
 }
 
 static inline void native_apic_mem_eoi(void)
-- 
2.43.0.594.gd9cf4e227d-goog

Powered by blists - more mailing lists