Previously, when compiled with clang, native_apic_mem_read gets inlined
into __xapic_wait_icr_idle and optimized to a testl instruction. When
run in a VM with SEV-ES enabled, it attempts to emulate this
instruction, but the emulator does not support it. Instead, use inline
assembly to force native_apic_mem_read to use the mov instruction which
is supported by the emulator.

Signed-off-by: Adam Dunlap <acdunlap@google.com>
---
V1 -> V2: Replaced asm with readl function which does the same thing

 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 3415321c8240..b4c9034aa073 100644
--- a/arch/x86/include/asm/apic.h
+++ b/arch/x86/include/asm/apic.h
@@ -12,6 +12,7 @@
 #include <asm/mpspec.h>
 #include <asm/msr.h>
 #include <asm/hardirq.h>
+#include <asm/io.h>
 
 #define ARCH_APICTIMER_STOPS_ON_C3	1
 
@@ -109,7 +110,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));
 }
 
 extern void native_apic_wait_icr_idle(void);
-- 
2.37.1.559.g78731f0fdb-goog

On Thu, Sep 8, 2022 at 6:05 PM Adam Dunlap <acdunlap@google.com> wrote:
>
> Previously, when compiled with clang, native_apic_mem_read gets inlined
> into __xapic_wait_icr_idle and optimized to a testl instruction. When
> run in a VM with SEV-ES enabled, it attempts to emulate this
> instruction, but the emulator does not support it. Instead, use inline
> assembly to force native_apic_mem_read to use the mov instruction which
> is supported by the emulator.

This seems to be an issue with the SEV-ES in guest #VC handler's
"emulator" right?

If that's the case I think this should be fixed in the #VC handler
instead of fixing the code that is failing to be emulated. What if
there are other places where a testl is used and our tests have not
caught them.

On Wed, Sep 14, 2022 at 12:13 PM Peter Gonda <pgonda@google.com> wrote:
>
> On Thu, Sep 8, 2022 at 6:05 PM Adam Dunlap <acdunlap@google.com> wrote:
> >
> > Previously, when compiled with clang, native_apic_mem_read gets inlined
> > into __xapic_wait_icr_idle and optimized to a testl instruction. When
> > run in a VM with SEV-ES enabled, it attempts to emulate this
> > instruction, but the emulator does not support it. Instead, use inline
> > assembly to force native_apic_mem_read to use the mov instruction which
> > is supported by the emulator.
>
> This seems to be an issue with the SEV-ES in guest #VC handler's
> "emulator" right?
>
> If that's the case I think this should be fixed in the #VC handler
> instead of fixing the code that is failing to be emulated. What if
> there are other places where a testl is used and our tests have not
> caught them.

That was my initial reaction too. But we spoke w/ Tom offline (before
sending this) and my understanding was that we really should be using
MOV for MMIO. I've cc'd Tom so he can speak to this directly though.

On Wed, Sep 14, 2022 at 12:59 PM Marc Orr <marcorr@google.com> wrote:
>
> On Wed, Sep 14, 2022 at 12:13 PM Peter Gonda <pgonda@google.com> wrote:
> >
> > On Thu, Sep 8, 2022 at 6:05 PM Adam Dunlap <acdunlap@google.com> wrote:
> > >
> > > Previously, when compiled with clang, native_apic_mem_read gets inlined
> > > into __xapic_wait_icr_idle and optimized to a testl instruction. When
> > > run in a VM with SEV-ES enabled, it attempts to emulate this
> > > instruction, but the emulator does not support it. Instead, use inline
> > > assembly to force native_apic_mem_read to use the mov instruction which
> > > is supported by the emulator.
> >
> > This seems to be an issue with the SEV-ES in guest #VC handler's
> > "emulator" right?
> >
> > If that's the case I think this should be fixed in the #VC handler
> > instead of fixing the code that is failing to be emulated. What if
> > there are other places where a testl is used and our tests have not
> > caught them.
>
> That was my initial reaction too. But we spoke w/ Tom offline (before
> sending this) and my understanding was that we really should be using
> MOV for MMIO. I've cc'd Tom so he can speak to this directly though.

Actually cc'ing Tom :-).

On 9/14/22 06:59, Marc Orr wrote:
> On Wed, Sep 14, 2022 at 12:59 PM Marc Orr <marcorr@google.com> wrote:
>>
>> On Wed, Sep 14, 2022 at 12:13 PM Peter Gonda <pgonda@google.com> wrote:
>>>
>>> On Thu, Sep 8, 2022 at 6:05 PM Adam Dunlap <acdunlap@google.com> wrote:
>>>>
>>>> Previously, when compiled with clang, native_apic_mem_read gets inlined
>>>> into __xapic_wait_icr_idle and optimized to a testl instruction. When
>>>> run in a VM with SEV-ES enabled, it attempts to emulate this
>>>> instruction, but the emulator does not support it. Instead, use inline
>>>> assembly to force native_apic_mem_read to use the mov instruction which
>>>> is supported by the emulator.
>>>
>>> This seems to be an issue with the SEV-ES in guest #VC handler's
>>> "emulator" right?
>>>
>>> If that's the case I think this should be fixed in the #VC handler
>>> instead of fixing the code that is failing to be emulated. What if
>>> there are other places where a testl is used and our tests have not
>>> caught them.
>>
>> That was my initial reaction too. But we spoke w/ Tom offline (before
>> sending this) and my understanding was that we really should be using
>> MOV for MMIO. I've cc'd Tom so he can speak to this directly though.

I did finally find the section in our PPR that references using the MOV 
instruction, but it was for MMIO configuration space, not general MMIO 
operations.

So, yes, the #VC handler could be extended to handle a TEST instruction to 
fix this. My worry would be if the compiler decided to use a different 
instruction in the future. I see that the native_apic_mem_write() is using 
assembler to perform its operation, it just seemed right that the 
native_apic_mem_read() could do the same.

Thanks,
Tom

> 
> Actually cc'ing Tom :-).

On 9/14/22 04:13, Peter Gonda wrote:
> On Thu, Sep 8, 2022 at 6:05 PM Adam Dunlap <acdunlap@google.com> wrote:
>> Previously, when compiled with clang, native_apic_mem_read gets inlined
>> into __xapic_wait_icr_idle and optimized to a testl instruction. When
>> run in a VM with SEV-ES enabled, it attempts to emulate this
>> instruction, but the emulator does not support it. Instead, use inline
>> assembly to force native_apic_mem_read to use the mov instruction which
>> is supported by the emulator.
> This seems to be an issue with the SEV-ES in guest #VC handler's
> "emulator" right?

No.

It's not just an SEV-ES thing.  It's a problem for TDX and _probably_ a
problem for normal virtualization where it's a host-side issue.  Kirill
wrote a lot of great background information in here:

> https://lore.kernel.org/all/164946765464.4207.3715751176055921036.tip-bot2@tip-bot2/

So, the question is not "should we extend the MMIO instruction decoders
to handle one more instruction?".  It is "should we extend the MMIO
decoders to handle *ALL* memory read instructions?"

That's an even more emphatic "NO".

readl() seems to be the right thing to do.  Also, Dear TDX, SEV and virt
folks: please look for more of these.  They're going to bite you sooner
or later.  You should have caught this one before now.

On Wed, Sep 14, 2022, Dave Hansen wrote:
> On 9/14/22 04:13, Peter Gonda wrote:
> > On Thu, Sep 8, 2022 at 6:05 PM Adam Dunlap <acdunlap@google.com> wrote:
> >> Previously, when compiled with clang, native_apic_mem_read gets inlined
> >> into __xapic_wait_icr_idle and optimized to a testl instruction. When
> >> run in a VM with SEV-ES enabled, it attempts to emulate this
> >> instruction, but the emulator does not support it. Instead, use inline
> >> assembly to force native_apic_mem_read to use the mov instruction which
> >> is supported by the emulator.
> > This seems to be an issue with the SEV-ES in guest #VC handler's
> > "emulator" right?
> 
> No.
> 
> It's not just an SEV-ES thing.  It's a problem for TDX and _probably_ a
> problem for normal virtualization where it's a host-side issue.  Kirill
> wrote a lot of great background information in here:
> 
> > https://lore.kernel.org/all/164946765464.4207.3715751176055921036.tip-bot2@tip-bot2/
> 
> So, the question is not "should we extend the MMIO instruction decoders
> to handle one more instruction?".  It is "should we extend the MMIO
> decoders to handle *ALL* memory read instructions?"
> 
> That's an even more emphatic "NO".

+1, keep the guest-side decoding as simple as possible.

> readl() seems to be the right thing to do.  Also, Dear TDX, SEV and virt
> folks: please look for more of these.  They're going to bite you sooner
> or later.  You should have caught this one before now.

On Wed, Sep 14, 2022 at 5:22 PM Sean Christopherson <seanjc@google.com> wrote:
>
> On Wed, Sep 14, 2022, Dave Hansen wrote:
> > On 9/14/22 04:13, Peter Gonda wrote:
> > > On Thu, Sep 8, 2022 at 6:05 PM Adam Dunlap <acdunlap@google.com> wrote:
> > >> Previously, when compiled with clang, native_apic_mem_read gets inlined
> > >> into __xapic_wait_icr_idle and optimized to a testl instruction. When
> > >> run in a VM with SEV-ES enabled, it attempts to emulate this
> > >> instruction, but the emulator does not support it. Instead, use inline
> > >> assembly to force native_apic_mem_read to use the mov instruction which
> > >> is supported by the emulator.
> > > This seems to be an issue with the SEV-ES in guest #VC handler's
> > > "emulator" right?
> >
> > No.
> >
> > It's not just an SEV-ES thing.  It's a problem for TDX and _probably_ a
> > problem for normal virtualization where it's a host-side issue.  Kirill
> > wrote a lot of great background information in here:
> >
> > > https://lore.kernel.org/all/164946765464.4207.3715751176055921036.tip-bot2@tip-bot2/
> >
> > So, the question is not "should we extend the MMIO instruction decoders
> > to handle one more instruction?".  It is "should we extend the MMIO
> > decoders to handle *ALL* memory read instructions?"
> >
> > That's an even more emphatic "NO".
>
> +1, keep the guest-side decoding as simple as possible.
>
> > readl() seems to be the right thing to do.  Also, Dear TDX, SEV and virt
> > folks: please look for more of these.  They're going to bite you sooner
> > or later.  You should have caught this one before now.

Thanks for the context here Dave. Fixing the offending MMIO
instruction here makes sense.