diff mbox series

[v3] x86/asm: Force native_apic_mem_read to use mov

Message ID 20240206223620.1833276-1-acdunlap@google.com
State New
Headers
Series [v3] x86/asm: Force native_apic_mem_read to use mov |

Commit Message

Adam Dunlap Feb. 6, 2024, 10:36 p.m. UTC 
  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@google.com>
Tested-by: Kevin Loughlin <kevinloughlin@google.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(-)

Comments

Dave Hansen Feb. 8, 2024, 4:47 p.m. UTC | #1 
On 2/6/24 14:36, Adam Dunlap wrote:
..
> 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

What changed?  Why is this a bug that we're only noticing now?  The line
of code that's modified here is from 2008.

I assume that it's something new in clang, but it'd be great to know
that for sure.

Also, considering the age of the last commit to touch that line:

Fixes: 67c5fc5c330f ("x86: merge apic_32/64.h")

this seems like the kind of thing we'll want in -stable in case folks
are compiling stable kernels with new clangs.
Ard Biesheuvel Feb. 9, 2024, 3:22 p.m. UTC | #2 
On Thu, 8 Feb 2024 at 16:48, Dave Hansen <dave.hansen@intel.com> wrote:
>
> On 2/6/24 14:36, Adam Dunlap wrote:
> ...
> > 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
>
> What changed?  Why is this a bug that we're only noticing now?  The line
> of code that's modified here is from 2008.
>
> I assume that it's something new in clang, but it'd be great to know
> that for sure.
>

Might be the use of LTO in the Google prod[uction]kernel. Adam, can you confirm?

> Also, considering the age of the last commit to touch that line:
>
> Fixes: 67c5fc5c330f ("x86: merge apic_32/64.h")
>
> this seems like the kind of thing we'll want in -stable in case folks
> are compiling stable kernels with new clangs.

LTO support was introduced in v5.12 afaict.
Adam Dunlap Feb. 9, 2024, 6:20 p.m. UTC | #3 
On Fri, Feb 9, 2024 at 7:22 AM Ard Biesheuvel <ardb@kernel.org> wrote:
>
> On Thu, 8 Feb 2024 at 16:48, Dave Hansen <dave.hansen@intel.com> wrote:
> >
> > On 2/6/24 14:36, Adam Dunlap wrote:
> > ...
> > > 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
> >
> > What changed?  Why is this a bug that we're only noticing now?  The line
> > of code that's modified here is from 2008.
> >
> > I assume that it's something new in clang, but it'd be great to know
> > that for sure.
> >
>
> Might be the use of LTO in the Google prod[uction]kernel. Adam, can you confirm?

It doesn't look like it's LTO. I disabled the LTO options in .config
and you can see the
problem just in a single object file:

With gcc:

% gdb -batch -ex 'file arch/x86/kernel/apic/ipi.o' -ex 'disassemble
apic_mem_wait_icr_idle'
Dump of assembler code for function apic_mem_wait_icr_idle:
   0x0000000000000260 <+0>:     endbr64
   0x0000000000000264 <+4>:     jmp    0x268 <apic_mem_wait_icr_idle+8>
   0x0000000000000266 <+6>:     pause
   0x0000000000000268 <+8>:     mov    0xffffffffff5fc300,%eax
   0x000000000000026f <+15>:    test   $0x10,%ah
   0x0000000000000272 <+18>:    jne    0x266 <apic_mem_wait_icr_idle+6>
   0x0000000000000274 <+20>:    jmpq   0x279

With clang:

% gdb -batch -ex 'file arch/x86/kernel/apic/ipi.o' -ex 'disassemble
apic_mem_wait_icr_idle'
Dump of assembler code for function apic_mem_wait_icr_idle:
   0x0000000000000250 <+0>:     endbr64
   0x0000000000000254 <+4>:     testl  $0x1000,0xffffffffff5fc300
   0x000000000000025f <+15>:    je     0x270 <apic_mem_wait_icr_idle+32>
   0x0000000000000261 <+17>:    pause
   0x0000000000000263 <+19>:    testl  $0x1000,0xffffffffff5fc300
   0x000000000000026e <+30>:    jne    0x261 <apic_mem_wait_icr_idle+17>
   0x0000000000000270 <+32>:    cs jmpq 0x276

This shows how gcc uses mov to load the apic memory and then testl to
test it, while clang
combines those instructions.

I plugged in the relevant subsection into godbolt [0] and it appears
the assembly changed in
clang 8 (released 2019). I'm not set up to do full compilations with
old clang versions, but
this is the most likely change point.

> this seems like the kind of thing we'll want in -stable in case folks
> are compiling stable kernels with new clangs.

That makes sense. Note that there was another patch accepted recently
that fixed another
clang-with-SEV problem [1], so they should probably be backported to
the same stable
branches since neither is that useful without the other.

[0] https://godbolt.org/z/nq9M9e8ex
[1] https://git.kernel.org/pub/scm/linux/kernel/git/tip/tip.git/commit/?id=1c811d403afd73f04bde82b83b24c754011bd0e8
diff mbox series

Patch

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)