On Tue, 19 Mar 2024 at 00:10, Adam Dunlap <acdunlap@xxxxxxxxxx> 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@xxxxxxxxxxxxxxx/ > > Signed-off-by: Adam Dunlap <acdunlap@xxxxxxxxxx> > Tested-by: Kevin Loughlin <kevinloughlin@xxxxxxxxxx> > Reviewed-by: Thomas Gleixner <tglx@xxxxxxxxxxxxx> > Cc: stable@xxxxxxxxxxxxxxx > Reviewed-by: Ard Biesheuvel <ardb@xxxxxxxxxx> > --- > > 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@xxxxxxxxxx/ > > 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 >
