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>
Reviewed-by: Marc Orr <marcorr@google.com>
Reviewed-by: Jacob Xu <jacobhxu@google.com>
---
 arch/x86/include/asm/apic.h | 13 ++++++++++++-
 1 file changed, 12 insertions(+), 1 deletion(-)

diff --git a/arch/x86/include/asm/apic.h b/arch/x86/include/asm/apic.h
index 3415321c8240..281db79e76a9 100644
--- a/arch/x86/include/asm/apic.h
+++ b/arch/x86/include/asm/apic.h
@@ -109,7 +109,18 @@ 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));
+	volatile u32 *addr = (volatile u32 *)(APIC_BASE + reg);
+	u32 out;
+
+	/*
+	 * Functionally, what we want to do is simply return *addr. However,
+	 * this accesses an MMIO which may need to be emulated in some cases.
+	 * The emulator doesn't necessarily support all instructions, so we
+	 * force the read from addr to use a mov instruction.
+	 */
+	asm_inline("movl %1, %0" : "=r"(out) : "m"(*addr));
+
+	return out;
 }
 
 extern void native_apic_wait_icr_idle(void);
-- 
2.37.1.559.g78731f0fdb-goog

On August 11, 2022 11:00:10 AM PDT, 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.
>
>Signed-off-by: Adam Dunlap <acdunlap@google.com>
>Reviewed-by: Marc Orr <marcorr@google.com>
>Reviewed-by: Jacob Xu <jacobhxu@google.com>
>---
> arch/x86/include/asm/apic.h | 13 ++++++++++++-
> 1 file changed, 12 insertions(+), 1 deletion(-)
>
>diff --git a/arch/x86/include/asm/apic.h b/arch/x86/include/asm/apic.h
>index 3415321c8240..281db79e76a9 100644
>--- a/arch/x86/include/asm/apic.h
>+++ b/arch/x86/include/asm/apic.h
>@@ -109,7 +109,18 @@ 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));
>+	volatile u32 *addr = (volatile u32 *)(APIC_BASE + reg);
>+	u32 out;
>+
>+	/*
>+	 * Functionally, what we want to do is simply return *addr. However,
>+	 * this accesses an MMIO which may need to be emulated in some cases.
>+	 * The emulator doesn't necessarily support all instructions, so we
>+	 * force the read from addr to use a mov instruction.
>+	 */
>+	asm_inline("movl %1, %0" : "=r"(out) : "m"(*addr));
>+
>+	return out;
> }
> 
> extern void native_apic_wait_icr_idle(void);

As I recall, there are some variants which only support using the ax register, so if you are going to do this might as well go all the way and force a specific instruction with specific registers, like mov (%rdx),%rax (by analogy with the I/O registers.)

On Thu, Aug 11, 2022, Adam Dunlap 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.
> 
> Signed-off-by: Adam Dunlap <acdunlap@google.com>
> Reviewed-by: Marc Orr <marcorr@google.com>
> Reviewed-by: Jacob Xu <jacobhxu@google.com>
> ---
>  arch/x86/include/asm/apic.h | 13 ++++++++++++-
>  1 file changed, 12 insertions(+), 1 deletion(-)
> 
> diff --git a/arch/x86/include/asm/apic.h b/arch/x86/include/asm/apic.h
> index 3415321c8240..281db79e76a9 100644
> --- a/arch/x86/include/asm/apic.h
> +++ b/arch/x86/include/asm/apic.h
> @@ -109,7 +109,18 @@ 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));
> +	volatile u32 *addr = (volatile u32 *)(APIC_BASE + reg);
> +	u32 out;
> +
> +	/*
> +	 * Functionally, what we want to do is simply return *addr. However,
> +	 * this accesses an MMIO which may need to be emulated in some cases.
> +	 * The emulator doesn't necessarily support all instructions, so we
> +	 * force the read from addr to use a mov instruction.
> +	 */
> +	asm_inline("movl %1, %0" : "=r"(out) : "m"(*addr));
> +
> +	return out;

Can't this just be:

	return readl((void __iomem *)(APIC_BASE + reg));

On August 11, 2022 12:27:10 PM PDT, Sean Christopherson <seanjc@google.com> wrote:
>On Thu, Aug 11, 2022, Adam Dunlap 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.
>> 
>> Signed-off-by: Adam Dunlap <acdunlap@google.com>
>> Reviewed-by: Marc Orr <marcorr@google.com>
>> Reviewed-by: Jacob Xu <jacobhxu@google.com>
>> ---
>>  arch/x86/include/asm/apic.h | 13 ++++++++++++-
>>  1 file changed, 12 insertions(+), 1 deletion(-)
>> 
>> diff --git a/arch/x86/include/asm/apic.h b/arch/x86/include/asm/apic.h
>> index 3415321c8240..281db79e76a9 100644
>> --- a/arch/x86/include/asm/apic.h
>> +++ b/arch/x86/include/asm/apic.h
>> @@ -109,7 +109,18 @@ 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));
>> +	volatile u32 *addr = (volatile u32 *)(APIC_BASE + reg);
>> +	u32 out;
>> +
>> +	/*
>> +	 * Functionally, what we want to do is simply return *addr. However,
>> +	 * this accesses an MMIO which may need to be emulated in some cases.
>> +	 * The emulator doesn't necessarily support all instructions, so we
>> +	 * force the read from addr to use a mov instruction.
>> +	 */
>> +	asm_inline("movl %1, %0" : "=r"(out) : "m"(*addr));
>> +
>> +	return out;
>
>Can't this just be:
>
>	return readl((void __iomem *)(APIC_BASE + reg));

The very point of the patch is to force a specific instruction sequence.

On Thu, Aug 11, 2022, H. Peter Anvin wrote:
> On August 11, 2022 12:27:10 PM PDT, Sean Christopherson <seanjc@google.com> wrote:
> >On Thu, Aug 11, 2022, Adam Dunlap 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.
> >> 
> >> Signed-off-by: Adam Dunlap <acdunlap@google.com>
> >> Reviewed-by: Marc Orr <marcorr@google.com>
> >> Reviewed-by: Jacob Xu <jacobhxu@google.com>
> >> ---
> >>  arch/x86/include/asm/apic.h | 13 ++++++++++++-
> >>  1 file changed, 12 insertions(+), 1 deletion(-)
> >> 
> >> diff --git a/arch/x86/include/asm/apic.h b/arch/x86/include/asm/apic.h
> >> index 3415321c8240..281db79e76a9 100644
> >> --- a/arch/x86/include/asm/apic.h
> >> +++ b/arch/x86/include/asm/apic.h
> >> @@ -109,7 +109,18 @@ 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));
> >> +	volatile u32 *addr = (volatile u32 *)(APIC_BASE + reg);
> >> +	u32 out;
> >> +
> >> +	/*
> >> +	 * Functionally, what we want to do is simply return *addr. However,
> >> +	 * this accesses an MMIO which may need to be emulated in some cases.
> >> +	 * The emulator doesn't necessarily support all instructions, so we
> >> +	 * force the read from addr to use a mov instruction.
> >> +	 */
> >> +	asm_inline("movl %1, %0" : "=r"(out) : "m"(*addr));
> >> +
> >> +	return out;
> >
> >Can't this just be:
> >
> >	return readl((void __iomem *)(APIC_BASE + reg));
> 
> The very point of the patch is to force a specific instruction sequence.

Yes, and that specific emulator-friendly instruction also has to be forced for all
of the core x86 read/write MMIO helpers.  And it's also possible for MMIO read/write
to be enlightened to skip the MOV and go straight to #VMGEXIT, i.e. the xAPIC code
shouldn't assume MOV is the best/only option (ignoring the handling of the P54C
erratum in the write path).

On August 11, 2022 1:03:11 PM PDT, Sean Christopherson <seanjc@google.com> wrote:
>On Thu, Aug 11, 2022, H. Peter Anvin wrote:
>> On August 11, 2022 12:27:10 PM PDT, Sean Christopherson <seanjc@google.com> wrote:
>> >On Thu, Aug 11, 2022, Adam Dunlap 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.
>> >> 
>> >> Signed-off-by: Adam Dunlap <acdunlap@google.com>
>> >> Reviewed-by: Marc Orr <marcorr@google.com>
>> >> Reviewed-by: Jacob Xu <jacobhxu@google.com>
>> >> ---
>> >>  arch/x86/include/asm/apic.h | 13 ++++++++++++-
>> >>  1 file changed, 12 insertions(+), 1 deletion(-)
>> >> 
>> >> diff --git a/arch/x86/include/asm/apic.h b/arch/x86/include/asm/apic.h
>> >> index 3415321c8240..281db79e76a9 100644
>> >> --- a/arch/x86/include/asm/apic.h
>> >> +++ b/arch/x86/include/asm/apic.h
>> >> @@ -109,7 +109,18 @@ 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));
>> >> +	volatile u32 *addr = (volatile u32 *)(APIC_BASE + reg);
>> >> +	u32 out;
>> >> +
>> >> +	/*
>> >> +	 * Functionally, what we want to do is simply return *addr. However,
>> >> +	 * this accesses an MMIO which may need to be emulated in some cases.
>> >> +	 * The emulator doesn't necessarily support all instructions, so we
>> >> +	 * force the read from addr to use a mov instruction.
>> >> +	 */
>> >> +	asm_inline("movl %1, %0" : "=r"(out) : "m"(*addr));
>> >> +
>> >> +	return out;
>> >
>> >Can't this just be:
>> >
>> >	return readl((void __iomem *)(APIC_BASE + reg));
>> 
>> The very point of the patch is to force a specific instruction sequence.
>
>Yes, and that specific emulator-friendly instruction also has to be forced for all
>of the core x86 read/write MMIO helpers.  And it's also possible for MMIO read/write
>to be enlightened to skip the MOV and go straight to #VMGEXIT, i.e. the xAPIC code
>shouldn't assume MOV is the best/only option (ignoring the handling of the P54C
>erratum in the write path).

That's not reasonable... but xAPIC is "special" enough.

On Thu, Aug 11, 2022 at 9:40 PM H. Peter Anvin <hpa@zytor.com> wrote:
>
> On August 11, 2022 1:03:11 PM PDT, Sean Christopherson <seanjc@google.com> wrote:
> >On Thu, Aug 11, 2022, H. Peter Anvin wrote:
> >> On August 11, 2022 12:27:10 PM PDT, Sean Christopherson <seanjc@google.com> wrote:
> >> >On Thu, Aug 11, 2022, Adam Dunlap 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.
> >> >>
> >> >> Signed-off-by: Adam Dunlap <acdunlap@google.com>
> >> >> Reviewed-by: Marc Orr <marcorr@google.com>
> >> >> Reviewed-by: Jacob Xu <jacobhxu@google.com>
> >> >> ---
> >> >>  arch/x86/include/asm/apic.h | 13 ++++++++++++-
> >> >>  1 file changed, 12 insertions(+), 1 deletion(-)
> >> >>
> >> >> diff --git a/arch/x86/include/asm/apic.h b/arch/x86/include/asm/apic.h
> >> >> index 3415321c8240..281db79e76a9 100644
> >> >> --- a/arch/x86/include/asm/apic.h
> >> >> +++ b/arch/x86/include/asm/apic.h
> >> >> @@ -109,7 +109,18 @@ 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));
> >> >> + volatile u32 *addr = (volatile u32 *)(APIC_BASE + reg);
> >> >> + u32 out;
> >> >> +
> >> >> + /*
> >> >> +  * Functionally, what we want to do is simply return *addr. However,
> >> >> +  * this accesses an MMIO which may need to be emulated in some cases.
> >> >> +  * The emulator doesn't necessarily support all instructions, so we
> >> >> +  * force the read from addr to use a mov instruction.
> >> >> +  */
> >> >> + asm_inline("movl %1, %0" : "=r"(out) : "m"(*addr));
> >> >> +
> >> >> + return out;
> >> >
> >> >Can't this just be:
> >> >
> >> >    return readl((void __iomem *)(APIC_BASE + reg));
> >>
> >> The very point of the patch is to force a specific instruction sequence.
> >
> >Yes, and that specific emulator-friendly instruction also has to be forced for all
> >of the core x86 read/write MMIO helpers.  And it's also possible for MMIO read/write
> >to be enlightened to skip the MOV and go straight to #VMGEXIT, i.e. the xAPIC code
> >shouldn't assume MOV is the best/only option (ignoring the handling of the P54C
> >erratum in the write path).
>
> That's not reasonable... but xAPIC is "special" enough.

Thanks for your responses. I think for now it makes sense to use the
readl function because
I haven't seen it require the ax register so can't verify the result.
I will send out a modified
patch using readl shortly.

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

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.