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

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

Posted by Adam Dunlap 1 year, 10 months ago

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

Posted by Ard Biesheuvel 1 year, 10 months ago

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

Posted by tip-bot2 for Adam Dunlap 1 year, 10 months ago

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)