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

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

[Adam Dunlap]

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(-)

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

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

[Dave Hansen]

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.

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

[Ard Biesheuvel]

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.

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

[Adam Dunlap]

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

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

[Kevin Loughlin]

On Fri, Feb 9, 2024 at 10:20 AM Adam Dunlap <acdunlap@google.com> wrote:
>
> On Fri, Feb 9, 2024 at 7:22 AM Ard Biesheuvel <ardb@kernel.org> wrote:
> >
> > 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.
>
> [1] https://git.kernel.org/pub/scm/linux/kernel/git/tip/tip.git/commit/?id=1c811d403afd73f04bde82b83b24c754011bd0e8

Agreed; clang builds of SEV-SNP guests will need both this patch and
[1]. If an additional patch [2] also gets merged, we may want to do
the same for [2] as well.

[2] https://lore.kernel.org/lkml/20240313121546.2964854-1-kevinloughlin@google.com/

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

[Thomas Gleixner]

On Tue, Feb 06 2024 at 14:36, Adam Dunlap wrote:

Can you please use foo() as notation for functions all over the place
including the subject line, which also wants s/mov/the MOV instruction/
and use MOV instead of 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

s/It would/It's/

Either it's a fact or not.

> 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.

How is this paragraph relevant?

> [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>

Other than the above nit picks:

Reviewed-by: Thomas Gleixner <tglx@linutronix.de>

[PATCH v4] x86/asm: Force native_apic_mem_read() to use the MOV instruction

[Adam Dunlap]

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
instruction 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 apic_mem_wait_icr_idle(). The emulator comes from insn_decode_mmio
in arch/x86/lib/insn-eval.c. It's not 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.

[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>
Reviewed-by: Thomas Gleixner <tglx@linutronix.de>
Cc: stable@vger.kernel.org

---

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.

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
V3 -> V4: Fixed nits in commit message

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

Re: [PATCH v4] x86/asm: Force native_apic_mem_read() to use the MOV instruction

[Ard Biesheuvel]

On Tue, 19 Mar 2024 at 00:10, Adam Dunlap <acdunlap@google.com> wrote:
>
> 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
> instruction 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 apic_mem_wait_icr_idle(). The emulator comes from insn_decode_mmio
> in arch/x86/lib/insn-eval.c. It's not 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.
>
> [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>
> Reviewed-by: Thomas Gleixner <tglx@linutronix.de>
> Cc: stable@vger.kernel.org
>

Reviewed-by: Ard Biesheuvel <ardb@kernel.org>

> ---
>
> 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.
>
> 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
> V3 -> V4: Fixed nits in commit message
>
> 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
>

[tip: x86/urgent] x86/apic: Force native_apic_mem_read() to use the MOV instruction

[tip-bot2 for Adam Dunlap]

The following commit has been merged into the x86/urgent branch of tip:

Commit-ID:     5ce344beaca688f4cdea07045e0b8f03dc537e74
Gitweb:        https://git.kernel.org/tip/5ce344beaca688f4cdea07045e0b8f03dc537e74
Author:        Adam Dunlap <acdunlap@google.com>
AuthorDate:    Mon, 18 Mar 2024 16:09:27 -07:00
Committer:     Borislav Petkov (AMD) <bp@alien8.de>
CommitterDate: Mon, 08 Apr 2024 15:37:57 +02:00

x86/apic: Force native_apic_mem_read() to use the MOV instruction

When done from a virtual machine, instructions that touch APIC memory
must be emulated. By convention, MMIO accesses 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
instruction generated by readl(). In particular, when the kernel is
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 as it is the most
generic and is what is usually emitted currently.

The TESTL instruction is emitted when native_apic_mem_read() is inlined
into apic_mem_wait_icr_idle(). The emulator comes from
insn_decode_mmio() in arch/x86/lib/insn-eval.c. It's not 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.

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

  [ bp: Massage commit message, fix typos. ]

Signed-off-by: Adam Dunlap <acdunlap@google.com>
Signed-off-by: Borislav Petkov (AMD) <bp@alien8.de>
Reviewed-by: Thomas Gleixner <tglx@linutronix.de>
Reviewed-by: Ard Biesheuvel <ardb@kernel.org>
Tested-by: Kevin Loughlin <kevinloughlin@google.com>
Cc: <stable@vger.kernel.org>
Link: https://lore.kernel.org/r/20240318230927.2191933-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 94ce0f7..e6ab0cf 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
 
@@ -98,7 +99,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)