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

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

[Adam Dunlap]

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

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

[Sean Christopherson]

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

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

[H. Peter Anvin]

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.

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

[Sean Christopherson]

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

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

[H. Peter Anvin]

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.

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

[Adam Dunlap]

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.

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

[Adam Dunlap]

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

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

[Adam Dunlap]

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

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

[Peter Gonda]

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.

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

[Marc Orr]

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.

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

[Marc Orr]

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

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

[Tom Lendacky]

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

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

[Dave Hansen]

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.

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

[Sean Christopherson]

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.

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

[Peter Gonda]

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.

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

[Adam Dunlap]

[resent with plain text]

Thanks for all the responses. Is the consensus that we should use the
readl function here or instead use inline assembly directly as in the patch
I originally sent out:

asm_inline("movl %1, %0" : "=r"(out) : "m"(*addr));

? The readl function has this exact same code, I'm just not sure
which version fits better stylistically.


On Mon, Oct 3, 2022 at 4:01 PM Adam Dunlap <acdunlap@google.com> wrote:
>
> Thanks for all the responses. Is the consensus that we should use the
> readl function here or instead use inline assembly directly as in the patch
> I originally sent out:
>
> asm_inline("movl %1, %0" : "=r"(out) : "m"(*addr));
>
> ? The readl function has this exact same code, I'm just not sure
> which version fits better stylistically.

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

[Tom Lendacky]

On 10/3/22 18:11, H. Peter Anvin wrote:
> On October 3, 2022 4:01:01 PM PDT, Adam Dunlap <acdunlap@google.com> wrote:
>> Thanks for all the responses. Is the consensus that we should use the
>> readl function here or instead use inline assembly directly as in the patch
>> I originally sent out:
>>
>> asm_inline("movl %1, %0" : "=r"(out) : "m"(*addr));
>>
>> ? The readl function has this exact same code, I'm just not sure
>> which version fits better stylistically.
> 
> Is mov with an arbitrary addressing mode still acceptable for whatever is causing this problem?

The acceptable forms of MOV are covered by insn_decode_mmio() in 
arch/x86/lib/insn-eval.c.

Thanks,
Tom

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

[Marc Orr]

On Wed, Oct 5, 2022 at 6:29 AM Tom Lendacky <thomas.lendacky@amd.com> wrote:
>
> On 10/3/22 18:11, H. Peter Anvin wrote:
> > On October 3, 2022 4:01:01 PM PDT, Adam Dunlap <acdunlap@google.com> wrote:
> >> Thanks for all the responses. Is the consensus that we should use the
> >> readl function here or instead use inline assembly directly as in the patch
> >> I originally sent out:
> >>
> >> asm_inline("movl %1, %0" : "=r"(out) : "m"(*addr));
> >>
> >> ? The readl function has this exact same code, I'm just not sure
> >> which version fits better stylistically.
> >
> > Is mov with an arbitrary addressing mode still acceptable for whatever is causing this problem?
>
> The acceptable forms of MOV are covered by insn_decode_mmio() in
> arch/x86/lib/insn-eval.c.

Is this blocked on an item? There seems to be consensus that this
patch fixes a bug and is taking the right high-level approach (i.e.,
change the guest code to avoid triggering a sequence that isn't
supported under CVM exception-based emulation). Without something like
this, we weren't able to build the kernel w/ CLANG when it is
configured to run under SEV-ES.

We sent out two versions of the patch. One that does the mov directly
[1] and a second that calls readl [2]. Is one of these two patches
acceptable? Or do we need to follow up on something?

[1] https://lore.kernel.org/lkml/0D6A1E49-F21B-42AA-BBBF-13BFC308BB1E@zytor.com/T/
[2] https://lore.kernel.org/all/20220812183501.3555820-1-acdunlap@google.com/

Thanks,
Marc

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

[Sidharth Telang]

> Is this blocked on an item? There seems to be consensus that this
> patch fixes a bug and is taking the right high-level approach (i.e.,
> change the guest code to avoid triggering a sequence that isn't
> supported under CVM exception-based emulation). Without something like
> this, we weren't able to build the kernel w/ CLANG when it is
> configured to run under SEV-ES.

> We sent out two versions of the patch. One that does the mov directly
> [1] and a second that calls readl [2]. Is one of these two patches
> acceptable? Or do we need to follow up on something?
> 
> [1] https://lore.kernel.org/lkml/0D6A1E49-F21B-42AA-BBBF-13BFC308BB1E@zytor.com/T/
> [2] https://lore.kernel.org/all/20220812183501.3555820-1-acdunlap@google.com/

Signal-boosting this thread: is this blocked on any item?

We are still running into this issue (SEV-ES guest unexpectedly requests
termination minutes after booting) and applying this patch seems to fix it.

Thanks,
Sid

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

[Dave Hansen]

On 11/17/23 10:14, Sidharth Telang wrote:
>> Is this blocked on an item? There seems to be consensus that this
>> patch fixes a bug and is taking the right high-level approach (i.e.,
>> change the guest code to avoid triggering a sequence that isn't
>> supported under CVM exception-based emulation). Without something like
>> this, we weren't able to build the kernel w/ CLANG when it is
>> configured to run under SEV-ES.
> 
>> We sent out two versions of the patch. One that does the mov directly
>> [1] and a second that calls readl [2]. Is one of these two patches
>> acceptable? Or do we need to follow up on something?
>>
>> [1] https://lore.kernel.org/lkml/0D6A1E49-F21B-42AA-BBBF-13BFC308BB1E@zytor.com/T/
>> [2] https://lore.kernel.org/all/20220812183501.3555820-1-acdunlap@google.com/
> 
> Signal-boosting this thread: is this blocked on any item?

Yes, it's blocked on you sending a well-described patch.

You sent out two patches which both received a lot of discussion and
induced a lot of confusion.  Can you please take all the knowledge from
this thread and send a third patch that has a proper changelog
incorporating all that knowledge?

Which approach should that patch have?  Whatever one is as close to what
native_apic_mem_write() does as possible.

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

[H. Peter Anvin]

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