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 <[email protected]>
Reviewed-by: Marc Orr <[email protected]>
Reviewed-by: Jacob Xu <[email protected]>
---
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 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 <[email protected]>
> Reviewed-by: Marc Orr <[email protected]>
> Reviewed-by: Jacob Xu <[email protected]>
> ---
> 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 Thu, Aug 11, 2022, H. Peter Anvin wrote:
> On August 11, 2022 12:27:10 PM PDT, Sean Christopherson <[email protected]> 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 <[email protected]>
> >> Reviewed-by: Marc Orr <[email protected]>
> >> Reviewed-by: Jacob Xu <[email protected]>
> >> ---
> >> 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 11:00:10 AM PDT, Adam Dunlap <[email protected]> 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 <[email protected]>
>Reviewed-by: Marc Orr <[email protected]>
>Reviewed-by: Jacob Xu <[email protected]>
>---
> 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 August 11, 2022 12:27:10 PM PDT, Sean Christopherson <[email protected]> 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 <[email protected]>
>> Reviewed-by: Marc Orr <[email protected]>
>> Reviewed-by: Jacob Xu <[email protected]>
>> ---
>> 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 August 11, 2022 1:03:11 PM PDT, Sean Christopherson <[email protected]> wrote:
>On Thu, Aug 11, 2022, H. Peter Anvin wrote:
>> On August 11, 2022 12:27:10 PM PDT, Sean Christopherson <[email protected]> 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 <[email protected]>
>> >> Reviewed-by: Marc Orr <[email protected]>
>> >> Reviewed-by: Jacob Xu <[email protected]>
>> >> ---
>> >> 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 <[email protected]> wrote:
>
> On August 11, 2022 1:03:11 PM PDT, Sean Christopherson <[email protected]> wrote:
> >On Thu, Aug 11, 2022, H. Peter Anvin wrote:
> >> On August 11, 2022 12:27:10 PM PDT, Sean Christopherson <[email protected]> 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 <[email protected]>
> >> >> Reviewed-by: Marc Orr <[email protected]>
> >> >> Reviewed-by: Jacob Xu <[email protected]>
> >> >> ---
> >> >> 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 <[email protected]>
---
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 <[email protected]>
---
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 <[email protected]> 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 <[email protected]> wrote:
>
> On Thu, Sep 8, 2022 at 6:05 PM Adam Dunlap <[email protected]> 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 <[email protected]> wrote:
>
> On Wed, Sep 14, 2022 at 12:13 PM Peter Gonda <[email protected]> wrote:
> >
> > On Thu, Sep 8, 2022 at 6:05 PM Adam Dunlap <[email protected]> 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 04:13, Peter Gonda wrote:
> On Thu, Sep 8, 2022 at 6:05 PM Adam Dunlap <[email protected]> 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 <[email protected]> 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 9/14/22 06:59, Marc Orr wrote:
> On Wed, Sep 14, 2022 at 12:59 PM Marc Orr <[email protected]> wrote:
>>
>> On Wed, Sep 14, 2022 at 12:13 PM Peter Gonda <[email protected]> wrote:
>>>
>>> On Thu, Sep 8, 2022 at 6:05 PM Adam Dunlap <[email protected]> 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 Wed, Sep 14, 2022 at 5:22 PM Sean Christopherson <[email protected]> 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 <[email protected]> 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.
[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 <[email protected]> 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.
On 10/3/22 18:11, H. Peter Anvin wrote:
> On October 3, 2022 4:01:01 PM PDT, Adam Dunlap <[email protected]> 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
On Wed, Oct 5, 2022 at 6:29 AM Tom Lendacky <[email protected]> wrote:
>
> On 10/3/22 18:11, H. Peter Anvin wrote:
> > On October 3, 2022 4:01:01 PM PDT, Adam Dunlap <[email protected]> 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/[email protected]/T/
[2] https://lore.kernel.org/all/[email protected]/
Thanks,
Marc
> 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/[email protected]/T/
> [2] https://lore.kernel.org/all/[email protected]/
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
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/[email protected]/T/
>> [2] https://lore.kernel.org/all/[email protected]/
>
> 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.