[PATCH] x86/apic: Don't disable x2APIC if locked

[PATCH] x86/apic: Don't disable x2APIC if locked

[Daniel Sneddon]

The APIC supports two modes, legacy APIC (or xAPIC), and Extended APIC
(or x2APIC).  X2APIC mode is mostly compatible with legacy APIC, but
it disables the memory-mapped APIC interface in favor of one that uses
MSRs.  The APIC mode is controlled by the EXT bit in the APIC MSR.

Introduce support for a new feature that will allow the BIOS to lock
the APIC in x2APIC mode.  If the APIC is locked in x2APIC mode and the
kernel tries to disable the APIC or revert to legacy APIC mode a GP
fault will occur.

Introduce support for a new MSR (IA32_XAPIC_DISABLE_STATUS) and handle the
new locked mode when the LEGACY_XAPIC_DISABLED bit is set.

If the LEGACY_XAPIC_DISABLED is set, prevent the kernel
from trying to disable it.

Signed-off-by: Daniel Sneddon <daniel.sneddon@linux.intel.com>
---
 arch/x86/include/asm/cpu.h       |  2 ++
 arch/x86/include/asm/msr-index.h | 13 ++++++++++
 arch/x86/kernel/apic/apic.c      | 44 +++++++++++++++++++++++++++++---
 3 files changed, 55 insertions(+), 4 deletions(-)

diff --git a/arch/x86/include/asm/cpu.h b/arch/x86/include/asm/cpu.h
index 8cbf623f0ecf..b472ef76826a 100644
--- a/arch/x86/include/asm/cpu.h
+++ b/arch/x86/include/asm/cpu.h
@@ -94,4 +94,6 @@ static inline bool intel_cpu_signatures_match(unsigned int s1, unsigned int p1,
 	return p1 & p2;
 }
 
+extern u64 x86_read_arch_cap_msr(void);
+
 #endif /* _ASM_X86_CPU_H */
diff --git a/arch/x86/include/asm/msr-index.h b/arch/x86/include/asm/msr-index.h
index 6674bdb096f3..1e086b37a307 100644
--- a/arch/x86/include/asm/msr-index.h
+++ b/arch/x86/include/asm/msr-index.h
@@ -155,6 +155,11 @@
 						 * Return Stack Buffer Predictions.
 						 */
 
+#define ARCH_CAP_XAPIC_DISABLE		BIT(21)	/*
+						 * IA32_XAPIC_DISABLE_STATUS MSR
+						 * supported
+						 */
+
 #define MSR_IA32_FLUSH_CMD		0x0000010b
 #define L1D_FLUSH			BIT(0)	/*
 						 * Writeback and invalidate the
@@ -1054,4 +1059,12 @@
 #define MSR_IA32_HW_FEEDBACK_PTR        0x17d0
 #define MSR_IA32_HW_FEEDBACK_CONFIG     0x17d1
 
+/* x2APIC locked status */
+#define MSR_IA32_XAPIC_DISABLE_STATUS	0xBD
+#define LEGACY_XAPIC_DISABLED		BIT(0) /*
+						* x2APIC mode is locked and
+						* disabling x2APIC will cause
+						* a #GP
+						*/
+
 #endif /* _ASM_X86_MSR_INDEX_H */
diff --git a/arch/x86/kernel/apic/apic.c b/arch/x86/kernel/apic/apic.c
index 6d303d1d276c..cb9aab893022 100644
--- a/arch/x86/kernel/apic/apic.c
+++ b/arch/x86/kernel/apic/apic.c
@@ -61,6 +61,7 @@
 #include <asm/cpu_device_id.h>
 #include <asm/intel-family.h>
 #include <asm/irq_regs.h>
+#include <asm/cpu.h>
 
 unsigned int num_processors;
 
@@ -1751,11 +1752,26 @@ EXPORT_SYMBOL_GPL(x2apic_mode);
 
 enum {
 	X2APIC_OFF,
-	X2APIC_ON,
 	X2APIC_DISABLED,
+	/* All states below here have X2APIC enabled */
+	X2APIC_ON,
+	X2APIC_ON_LOCKED
 };
 static int x2apic_state;
 
+static bool x2apic_hw_locked(void)
+{
+	u64 ia32_cap;
+	u64 msr;
+
+	ia32_cap = x86_read_arch_cap_msr();
+	if (ia32_cap & ARCH_CAP_XAPIC_DISABLE) {
+		rdmsrl(MSR_IA32_XAPIC_DISABLE_STATUS, msr);
+		return (msr & LEGACY_XAPIC_DISABLED);
+	}
+	return false;
+}
+
 static void __x2apic_disable(void)
 {
 	u64 msr;
@@ -1793,6 +1809,10 @@ static int __init setup_nox2apic(char *str)
 				apicid);
 			return 0;
 		}
+		if (x2apic_hw_locked()) {
+			pr_warn("APIC locked in x2apic mode, can't disable");
+			return 0;
+		}
 		pr_warn("x2apic already enabled.\n");
 		__x2apic_disable();
 	}
@@ -1807,10 +1827,18 @@ early_param("nox2apic", setup_nox2apic);
 void x2apic_setup(void)
 {
 	/*
-	 * If x2apic is not in ON state, disable it if already enabled
+	 * Try to make the AP's APIC state match that of the BSP,  but if the
+	 * BSP is unlocked and the AP is locked then there is a state mismatch.
+	 * Warn about the mismatch in case a GP fault occurs due to a locked AP
+	 * trying to be turned off.
+	 */
+	if (x2apic_state != X2APIC_ON_LOCKED && x2apic_hw_locked())
+		pr_warn("x2apic lock mismatch between BSP and AP.");
+	/*
+	 * If x2apic is not in ON or LOCKED state, disable it if already enabled
 	 * from BIOS.
 	 */
-	if (x2apic_state != X2APIC_ON) {
+	if (x2apic_state < X2APIC_ON) {
 		__x2apic_disable();
 		return;
 	}
@@ -1831,6 +1859,11 @@ static __init void x2apic_disable(void)
 	if (x2apic_id >= 255)
 		panic("Cannot disable x2apic, id: %08x\n", x2apic_id);
 
+	if (x2apic_hw_locked()) {
+		pr_warn("Cannot disable locked x2apic, id: %08x\n", x2apic_id);
+		return;
+	}
+
 	__x2apic_disable();
 	register_lapic_address(mp_lapic_addr);
 }
@@ -1889,7 +1922,10 @@ void __init check_x2apic(void)
 	if (x2apic_enabled()) {
 		pr_info("x2apic: enabled by BIOS, switching to x2apic ops\n");
 		x2apic_mode = 1;
-		x2apic_state = X2APIC_ON;
+		if (x2apic_hw_locked())
+			x2apic_state = X2APIC_ON_LOCKED;
+		else
+			x2apic_state = X2APIC_ON;
 	} else if (!boot_cpu_has(X86_FEATURE_X2APIC)) {
 		x2apic_state = X2APIC_DISABLED;
 	}
-- 
2.25.1

Re: [PATCH] x86/apic: Don't disable x2APIC if locked

[Dave Hansen]

On 8/9/22 16:40, Daniel Sneddon wrote:
> The APIC supports two modes, legacy APIC (or xAPIC), and Extended APIC
> (or x2APIC).  X2APIC mode is mostly compatible with legacy APIC, but
> it disables the memory-mapped APIC interface in favor of one that uses
> MSRs.  The APIC mode is controlled by the EXT bit in the APIC MSR.
> 
> Introduce support for a new feature that will allow the BIOS to lock
> the APIC in x2APIC mode.  If the APIC is locked in x2APIC mode and the
> kernel tries to disable the APIC or revert to legacy APIC mode a GP
> fault will occur.
> 
> Introduce support for a new MSR (IA32_XAPIC_DISABLE_STATUS) and handle the
> new locked mode when the LEGACY_XAPIC_DISABLED bit is set.
> 
> If the LEGACY_XAPIC_DISABLED is set, prevent the kernel
> from trying to disable it.

Let's also not obscure the fact that the MMIO/xAPIC interface is
troublesome and there are real-world, practical reasons a piece of
hardware might not want to implement it.  First on the list is:

	https://aepicleak.com/

Second on the list is TDX.  The TDX module spec currently just dictates
that TDX guests must use x2APIC.  If this (IA32_XAPIC_DISABLE_STATUS)
mechanism was enumerated to TDX guests, they could use this instead of
crashing in burning in whatever horrid way they are now if someone
disables x2APIC on the command line.

It would also be nice to know roughly when this feature is showing up.
If it's going to show up as a part of a microcode update for my laptop
next week or next month, this might warrant a backport.  Intel would
presumably *want* a backport to happen there, too.

If it's going to be on one server CPU that's coming out in ten years,
then we can hold off.

It might also help to link to the documentation, even if it's below a
"--" in the changelog since these URLs aren't very stable.

> https://www.intel.com/content/www/us/en/developer/articles/technical/software-security-guidance/technical-documentation/cpuid-enumeration-and-architectural-msrs.html

or at least mention what the status of the documentation is.

The code looks OK to me, but I'm far from impartial because this isn't
my first look at it.  In any case:

Acked-by: Dave Hansen <dave.hansen@linux.intel.com>

Re: [PATCH] x86/apic: Don't disable x2APIC if locked

[Daniel Sneddon]

On 8/10/22 11:01, Dave Hansen wrote:
> On 8/9/22 16:40, Daniel Sneddon wrote:
>> The APIC supports two modes, legacy APIC (or xAPIC), and Extended APIC
>> (or x2APIC).  X2APIC mode is mostly compatible with legacy APIC, but
>> it disables the memory-mapped APIC interface in favor of one that uses
>> MSRs.  The APIC mode is controlled by the EXT bit in the APIC MSR.
>>
>> Introduce support for a new feature that will allow the BIOS to lock
>> the APIC in x2APIC mode.  If the APIC is locked in x2APIC mode and the
>> kernel tries to disable the APIC or revert to legacy APIC mode a GP
>> fault will occur.
>>
>> Introduce support for a new MSR (IA32_XAPIC_DISABLE_STATUS) and handle the
>> new locked mode when the LEGACY_XAPIC_DISABLED bit is set.
>>
>> If the LEGACY_XAPIC_DISABLED is set, prevent the kernel
>> from trying to disable it.
> 
> Let's also not obscure the fact that the MMIO/xAPIC interface is
> troublesome and there are real-world, practical reasons a piece of
> hardware might not want to implement it.  First on the list is:
> 
> 	https://aepicleak.com/
> 
Great point.  Since the problem with that interface wasn't public when I wrote
this patch I didn't have anything like that to link against.  Would you like me
to add the above link to the commit message?  I can certainly add more about the
SGX leak associated with APIC.

> Second on the list is TDX.  The TDX module spec currently just dictates
> that TDX guests must use x2APIC.  If this (IA32_XAPIC_DISABLE_STATUS)
> mechanism was enumerated to TDX guests, they could use this instead of
> crashing in burning in whatever horrid way they are now if someone
> disables x2APIC on the command line.
> 
> It would also be nice to know roughly when this feature is showing up.
> If it's going to show up as a part of a microcode update for my laptop
> next week or next month, this might warrant a backport.  Intel would
> presumably *want* a backport to happen there, too.
> 
I've been told that this will only be on Sapphire Rapids and newer.

> If it's going to be on one server CPU that's coming out in ten years,
> then we can hold off.
SPR will certainly be sooner than 10 years, and with distros running on LTS
kernels, it is probably worth backporting.  Since this isn't a bug fix (not a sw
bug anyway), is this something I can just CC stable or is there a better way to
say "Yes, this is a new feature, BUT, you really want it anyway"?

> 
> It might also help to link to the documentation, even if it's below a
> "--" in the changelog since these URLs aren't very stable.
> 
>> https://www.intel.com/content/www/us/en/developer/articles/technical/software-security-guidance/technical-documentation/cpuid-enumeration-and-architectural-msrs.html
> 
> or at least mention what the status of the documentation is.
> 
> The code looks OK to me, but I'm far from impartial because this isn't
> my first look at it.  In any case:
> 
> Acked-by: Dave Hansen <dave.hansen@linux.intel.com>

Thanks!

Re: [PATCH] x86/apic: Don't disable x2APIC if locked

[Dave Hansen]

On 8/10/22 11:30, Daniel Sneddon wrote:
>> If it's going to be on one server CPU that's coming out in ten years,
>> then we can hold off.
> SPR will certainly be sooner than 10 years, and with distros running on LTS
> kernels, it is probably worth backporting.  Since this isn't a bug fix (not a sw
> bug anyway), is this something I can just CC stable or is there a better way to
> say "Yes, this is a new feature, BUT, you really want it anyway"?

It it going to be *forced* on those SPR system?  In other words, is it a
little BIOS switch that users can flip?  Is there any non-kernel
workaround if a user hits this, or is the entire burden of this going to
be foisted on the kernel?

The worst case scenario would be if a user has managed to compile a
CONFIG_X86_X2APIC=n kernel and is happily running along until they get a
microcode update, reboot and can't boot any more.

A less-bad, but still nasty situation would be someone who is booting
with nox2apic, who also suddenly loses the ability to boot until they
figure out why their kernel is #GP'ing and oopsing early in boot and
remove nox2apic.

I think we need a bit more information on how this thing will get
deployed before we really know if it needs to be in stable kernels or
not.  For instance, if Intel really views this as an always-on security
mitigation, that's a different story than if this is, say, a performance
tweak.

Re: [PATCH] x86/apic: Don't disable x2APIC if locked

[Daniel Sneddon]

On 8/10/22 11:52, Dave Hansen wrote:
> On 8/10/22 11:30, Daniel Sneddon wrote:
>>> If it's going to be on one server CPU that's coming out in ten years,
>>> then we can hold off.
>> SPR will certainly be sooner than 10 years, and with distros running on LTS
>> kernels, it is probably worth backporting.  Since this isn't a bug fix (not a sw
>> bug anyway), is this something I can just CC stable or is there a better way to
>> say "Yes, this is a new feature, BUT, you really want it anyway"?
> 
> It it going to be *forced* on those SPR system?  In other words, is it a
> little BIOS switch that users can flip?  Is there any non-kernel
> workaround if a user hits this, or is the entire burden of this going to
> be foisted on the kernel?
It won't be forced, BUT, certain features won't be available if the APIC isn't
locked.  According to the documentation SGX and TDX won't be available if you
don't lock the APIC.  So, yes, you don't have to fix it in the kernel, but you
may lose access to features if you don't.
> 
> The worst case scenario would be if a user has managed to compile a
> CONFIG_X86_X2APIC=n kernel and is happily running along until they get a
> microcode update, reboot and can't boot any more.
That _shouldn't_ happen as it is only on new hardware, so a ucode update
_shouldn't_ cause a crash.
> 
> A less-bad, but still nasty situation would be someone who is booting
> with nox2apic, who also suddenly loses the ability to boot until they
> figure out why their kernel is #GP'ing and oopsing early in boot and
> remove nox2apic.

That should only happen if they update their BIOS settings in such a way that
the APIC is locked (e.g, SGX or TDX is enabled) on an existing SPR system.
> I think we need a bit more information on how this thing will get
> deployed before we really know if it needs to be in stable kernels or
> not.  For instance, if Intel really views this as an always-on security
> mitigation, that's a different story than if this is, say, a performance
> tweak.
Well, it is certainly an always on thing if you want SGX or TDX.  If you're on
SPR or newer with either of those enabled, then the APIC will be locked.  If
you're using an image that works with nox2apic on pre-SPR hardware and put it on
SPR hardware, you'll hang trying to boot.  In that case you'll either have to
remove the nox2apic option or tweak your BIOS so you're not locked, BUT, I
suspect those BIOS options aren't going to be super clear about what to turn off
to make sure you're not locked.

You can boot an old kernel without this patch, or even without X2APIC support,
but you may have to mess with the BIOS to get there.  The good news is that it
should only affect new systems as this isn't on any existing production hardware
and isn't planned on being pushed out to existing products via ucode updates.

Re: [PATCH] x86/apic: Don't disable x2APIC if locked

[Dave Hansen]

On 8/10/22 12:40, Daniel Sneddon wrote:
>> It it going to be *forced* on those SPR system?  In other words, is it a
>> little BIOS switch that users can flip?  Is there any non-kernel
>> workaround if a user hits this, or is the entire burden of this going to
>> be foisted on the kernel?
> It won't be forced, BUT, certain features won't be available if the APIC isn't
> locked.  According to the documentation SGX and TDX won't be available if you
> don't lock the APIC.  So, yes, you don't have to fix it in the kernel, but you
> may lose access to features if you don't.

Let's get some of this in the changelog and _possibly_ Documentation/ so
that users know we're depending on the BIOS to play nice.  Let's put
ourselves in the place of our users for a moment at least and try to
figure out how we play our part to help get them from seeing "can't
disable x2apic mode" or whatever to them flipping knobs in the BIOS.

I also dearly hope that Intel has told BIOS writers that the onus is on
them *and* those nice BIOS folks listen to Intel. :)

In any case, I don't think backports are warranted here at the moment.
We can always do it in the future if the need arises.

Re: [PATCH] x86/apic: Don't disable x2APIC if locked

[Daniel Sneddon]

On 8/10/22 12:59, Dave Hansen wrote:
> On 8/10/22 12:40, Daniel Sneddon wrote:
>>> It it going to be *forced* on those SPR system?  In other words, is it a
>>> little BIOS switch that users can flip?  Is there any non-kernel
>>> workaround if a user hits this, or is the entire burden of this going to
>>> be foisted on the kernel?
>> It won't be forced, BUT, certain features won't be available if the APIC isn't
>> locked.  According to the documentation SGX and TDX won't be available if you
>> don't lock the APIC.  So, yes, you don't have to fix it in the kernel, but you
>> may lose access to features if you don't.
> 
> Let's get some of this in the changelog and _possibly_ Documentation/ so
> that users know we're depending on the BIOS to play nice.  Let's put
> ourselves in the place of our users for a moment at least and try to
> figure out how we play our part to help get them from seeing "can't
> disable x2apic mode" or whatever to them flipping knobs in the BIOS.

I will certainly add this to the changelog.  I could add a blurb to the
documentation where nox2apic is defined as a parameter as well.  If there is a
better place to document that please let me know.

> 
> I also dearly hope that Intel has told BIOS writers that the onus is on
> them *and* those nice BIOS folks listen to Intel. :)
You and me both!  I know this has gone out to our BIOS partners and they are
aware of it.  Beyond that, well, I guess we'll find out when SPR is released!

> 
> In any case, I don't think backports are warranted here at the moment.
> We can always do it in the future if the need arises.

Re: [PATCH] x86/apic: Don't disable x2APIC if locked

[Daniel Sneddon]

On 8/10/22 13:06, Daniel Sneddon wrote:
> I could add a blurb to the
> documentation where nox2apic is defined as a parameter as well.  
While I'm at it, I'm thinking of adding something where CONFIG_X86_X2APIC is
defined in kconfig.  I'll put in a blurb about selecting N on SPR systems could
prevent you from booting due to the lock.

Re: [PATCH] x86/apic: Don't disable x2APIC if locked

[Dave Hansen]

On 8/10/22 13:29, Daniel Sneddon wrote:
> On 8/10/22 13:06, Daniel Sneddon wrote:
>> I could add a blurb to the
>> documentation where nox2apic is defined as a parameter as well.  
> While I'm at it, I'm thinking of adding something where CONFIG_X86_X2APIC is
> defined in kconfig.  I'll put in a blurb about selecting N on SPR systems could
> prevent you from booting due to the lock.

Please don't make it _too_ specific.  This is, after all, new
architecture that is presumably going to be in place long after we all
forget what SPR is.

Good Kconfig text would probably be something along the lines of:

	Some ~2022 and later systems are locked into x2APIC mode and can
	not fall back to the legacy APIC modes.  They will be unable to
	boot without enabling this option.

Would anybody hate if we just removed the CONFIG_X86_X2APIC prompt entirely?

Re: [PATCH] x86/apic: Don't disable x2APIC if locked

[Thomas Gleixner]

On Wed, Aug 10 2022 at 12:40, Daniel Sneddon wrote:
> On 8/10/22 11:52, Dave Hansen wrote:
>> On 8/10/22 11:30, Daniel Sneddon wrote:
>>>> If it's going to be on one server CPU that's coming out in ten years,
>>>> then we can hold off.
>>> SPR will certainly be sooner than 10 years, and with distros running on LTS
>>> kernels, it is probably worth backporting.  Since this isn't a bug fix (not a sw
>>> bug anyway), is this something I can just CC stable or is there a better way to
>>> say "Yes, this is a new feature, BUT, you really want it anyway"?
>> 
>> It it going to be *forced* on those SPR system?  In other words, is it a
>> little BIOS switch that users can flip?  Is there any non-kernel
>> workaround if a user hits this, or is the entire burden of this going to
>> be foisted on the kernel?
> It won't be forced, BUT, certain features won't be available if the APIC isn't
> locked.  According to the documentation SGX and TDX won't be available if you
> don't lock the APIC.  So, yes, you don't have to fix it in the kernel, but you
> may lose access to features if you don't.
>> 
>> The worst case scenario would be if a user has managed to compile a
>> CONFIG_X86_X2APIC=n kernel and is happily running along until they get a
>> microcode update, reboot and can't boot any more.
> That _shouldn't_ happen as it is only on new hardware, so a ucode update
> _shouldn't_ cause a crash.

Only on new hardware? The lock mechanism has to be available on _all_
affected systems which support SGX. See

 https://www.intel.com/content/www/us/en/developer/articles/technical/software-security-guidance/resources/intel-sgx-software-and-tcb-recovery-guidance.html

That means at some point in the future the locked x2APIC will be a
prerequisite for attestating a SGX enclave independent of SPR.

So this affects already deployed systems and we have to

  - backport the x2apic lock changes

  - provide proper diagnostics

  - make SGX and TDX depend on X2APIC support

There is not much we can do about an older kernel which fails to boot or
explodes once a BIOS update locks down X2APIC.

Thanks,

        tglx

Re: [PATCH] x86/apic: Don't disable x2APIC if locked

[Daniel Sneddon]

On 8/10/22 15:06, Thomas Gleixner wrote:
> On Wed, Aug 10 2022 at 12:40, Daniel Sneddon wrote:
>> On 8/10/22 11:52, Dave Hansen wrote:
>>> On 8/10/22 11:30, Daniel Sneddon wrote:
>>>>> If it's going to be on one server CPU that's coming out in ten years,
>>>>> then we can hold off.
>>>> SPR will certainly be sooner than 10 years, and with distros running on LTS
>>>> kernels, it is probably worth backporting.  Since this isn't a bug fix (not a sw
>>>> bug anyway), is this something I can just CC stable or is there a better way to
>>>> say "Yes, this is a new feature, BUT, you really want it anyway"?
>>>
>>> It it going to be *forced* on those SPR system?  In other words, is it a
>>> little BIOS switch that users can flip?  Is there any non-kernel
>>> workaround if a user hits this, or is the entire burden of this going to
>>> be foisted on the kernel?
>> It won't be forced, BUT, certain features won't be available if the APIC isn't
>> locked.  According to the documentation SGX and TDX won't be available if you
>> don't lock the APIC.  So, yes, you don't have to fix it in the kernel, but you
>> may lose access to features if you don't.
>>>
>>> The worst case scenario would be if a user has managed to compile a
>>> CONFIG_X86_X2APIC=n kernel and is happily running along until they get a
>>> microcode update, reboot and can't boot any more.
>> That _shouldn't_ happen as it is only on new hardware, so a ucode update
>> _shouldn't_ cause a crash.
> 
> Only on new hardware? The lock mechanism has to be available on _all_
> affected systems which support SGX. See
> 
>  https://www.intel.com/content/www/us/en/developer/articles/technical/software-security-guidance/resources/intel-sgx-software-and-tcb-recovery-guidance.html

I asked the architect and security team that came up with that new MSR if it was
going to be backported via ucode updates and I was told no.

"Only SPR and newer. See the IA32_XAPIC_DISABLE_STATUS documentation at
https://www.intel.com/content/www/us/en/developer/articles/technical/software-security-guidance/technical-documentation/cpuid-enumeration-and-architectural-msrs.html."

So, it appears we have a war between which documentation do we believe!

I do have a few follow up questions I'm waiting on being answered to help
clarify all this.

Thanks,
Dan

Re: [PATCH] x86/apic: Don't disable x2APIC if locked

[Daniel Sneddon]

On 8/10/22 15:06, Thomas Gleixner wrote:
> So this affects already deployed systems and we have to
> 
>   - backport the x2apic lock changes
> 
>   - provide proper diagnostics
> 
>   - make SGX and TDX depend on X2APIC support
I'll add the comments Dave mentioned earlier, and will make SGX and TDX depend
on X2APIC since that makes sense regardless of what hw ends up with this change.
 Unless we want to get rid of CONFIG_X86_X2APIC like Dave mentioned?

Re: [PATCH] x86/apic: Don't disable x2APIC if locked

[Dave Hansen]

On 8/10/22 16:03, Daniel Sneddon wrote:
> On 8/10/22 15:06, Thomas Gleixner wrote:
>> So this affects already deployed systems and we have to
>>
>>   - backport the x2apic lock changes
>>
>>   - provide proper diagnostics
>>
>>   - make SGX and TDX depend on X2APIC support
> I'll add the comments Dave mentioned earlier, and will make SGX and TDX depend
> on X2APIC since that makes sense regardless of what hw ends up with this change.
>  Unless we want to get rid of CONFIG_X86_X2APIC like Dave mentioned?

The TDX guest support in the kernel isn't _actually_ related to this*.
It's the host-side support that matters and that isn't merged yet.  I've
cc'd Kai so he doesn't forget to do this.

I agree on the SGX side, though.

* TDX guest support already has this dependency, but it's for unrelated
  reasons:

config INTEL_TDX_GUEST
        bool "Intel TDX (Trust Domain Extensions) - Guest Support"
        depends on X86_64 && CPU_SUP_INTEL
        depends on X86_X2APIC

Re: [PATCH] x86/apic: Don't disable x2APIC if locked

[Daniel Sneddon]

On 8/10/22 16:09, Dave Hansen wrote:
> On 8/10/22 16:03, Daniel Sneddon wrote:
>> On 8/10/22 15:06, Thomas Gleixner wrote:
>>> So this affects already deployed systems and we have to
>>>
>>>   - backport the x2apic lock changes
>>>
>>>   - provide proper diagnostics
>>>
>>>   - make SGX and TDX depend on X2APIC support
>> I'll add the comments Dave mentioned earlier, and will make SGX and TDX depend
>> on X2APIC since that makes sense regardless of what hw ends up with this change.
>>  Unless we want to get rid of CONFIG_X86_X2APIC like Dave mentioned?
> 
> The TDX guest support in the kernel isn't _actually_ related to this*.
> It's the host-side support that matters and that isn't merged yet.  I've
> cc'd Kai so he doesn't forget to do this.
> 
> I agree on the SGX side, though.
> 
> * TDX guest support already has this dependency, but it's for unrelated
>   reasons:
> 
> config INTEL_TDX_GUEST
>         bool "Intel TDX (Trust Domain Extensions) - Guest Support"
>         depends on X86_64 && CPU_SUP_INTEL
>         depends on X86_X2APIC
So I got some more input.  SPR and newer will lock the APIC.  Older products
will get a ucode update, but that ucode update won't include the APIC lock.  So,
on non-SPR parts do we still want to make SGX depend on X2APIC?

Re: [PATCH] x86/apic: Don't disable x2APIC if locked

[Dave Hansen]

On 8/10/22 16:38, Daniel Sneddon wrote:
>>
>> config INTEL_TDX_GUEST
>>         bool "Intel TDX (Trust Domain Extensions) - Guest Support"
>>         depends on X86_64 && CPU_SUP_INTEL
>>         depends on X86_X2APIC
> So I got some more input.  SPR and newer will lock the APIC.  

Could you get a _little_ more clarity on this, please?  Exactly how and
when will it be locked?  What does the BIOS writer's guide say?  Will
there be an explicit x2APIC lock option?  Or, will it be implicitly
locked when SGX or TDX is enabled?

> Older products will get a ucode update, but that ucode update won't
> include the APIClock.  So, on non-SPR parts do we still want to make
> SGX depend on X2APIC?
Yes.  It's a small price to pay.

Re: [PATCH] x86/apic: Don't disable x2APIC if locked

[Daniel Sneddon]

On 8/10/22 16:44, Dave Hansen wrote:
> On 8/10/22 16:38, Daniel Sneddon wrote:
>>>
>>> config INTEL_TDX_GUEST
>>>         bool "Intel TDX (Trust Domain Extensions) - Guest Support"
>>>         depends on X86_64 && CPU_SUP_INTEL
>>>         depends on X86_X2APIC
>> So I got some more input.  SPR and newer will lock the APIC.  
> 
> Could you get a _little_ more clarity on this, please?  Exactly how and
> when will it be locked?  What does the BIOS writer's guide say?  Will
> there be an explicit x2APIC lock option?  Or, will it be implicitly
> locked when SGX or TDX is enabled?
The BIOS doesn't explicitly lock the APIC.  The APIC will be locked if X2APIC
mode is enabled when the BIOS does an MCHECK.  X2APIC mode will be enabled if
SGX or TDX are enabled.  So when exactly does the BIOS do an MCHECK?  That I'll
have to get clarification on.
> 
>> Older products will get a ucode update, but that ucode update won't
>> include the APIClock.  So, on non-SPR parts do we still want to make
>> SGX depend on X2APIC?
> Yes.  It's a small price to pay.

Re: [PATCH] x86/apic: Don't disable x2APIC if locked

[Thomas Gleixner]

On Wed, Aug 10 2022 at 17:01, Daniel Sneddon wrote:
> On 8/10/22 16:44, Dave Hansen wrote:
>> On 8/10/22 16:38, Daniel Sneddon wrote:
>>>>
>>>> config INTEL_TDX_GUEST
>>>>         bool "Intel TDX (Trust Domain Extensions) - Guest Support"
>>>>         depends on X86_64 && CPU_SUP_INTEL
>>>>         depends on X86_X2APIC
>>> So I got some more input.  SPR and newer will lock the APIC.  
>> 
>> Could you get a _little_ more clarity on this, please?  Exactly how and
>> when will it be locked?  What does the BIOS writer's guide say?  Will
>> there be an explicit x2APIC lock option?  Or, will it be implicitly
>> locked when SGX or TDX is enabled?
>
> The BIOS doesn't explicitly lock the APIC.  The APIC will be locked if X2APIC
> mode is enabled when the BIOS does an MCHECK.  X2APIC mode will be enabled if
> SGX or TDX are enabled.  So when exactly does the BIOS do an MCHECK?  That I'll
> have to get clarification on.

Sorry, this is uncomprehensible word salad and none of this makes any
sense at all to me.

Can you please explain this in comprehensible sentences understandable
by mere mortals?

Thanks,

        tglx

Re: [PATCH] x86/apic: Don't disable x2APIC if locked

[Daniel Sneddon]

On 8/10/22 17:38, Thomas Gleixner wrote:
> On Wed, Aug 10 2022 at 17:01, Daniel Sneddon wrote:
>> On 8/10/22 16:44, Dave Hansen wrote:
>>> On 8/10/22 16:38, Daniel Sneddon wrote:
>>>>>
>>>>> config INTEL_TDX_GUEST
>>>>>         bool "Intel TDX (Trust Domain Extensions) - Guest Support"
>>>>>         depends on X86_64 && CPU_SUP_INTEL
>>>>>         depends on X86_X2APIC
>>>> So I got some more input.  SPR and newer will lock the APIC.  
>>>
>>> Could you get a _little_ more clarity on this, please?  Exactly how and
>>> when will it be locked?  What does the BIOS writer's guide say?  Will
>>> there be an explicit x2APIC lock option?  Or, will it be implicitly
>>> locked when SGX or TDX is enabled?
>>
>> The BIOS doesn't explicitly lock the APIC.  The APIC will be locked if X2APIC
>> mode is enabled when the BIOS does an MCHECK.  X2APIC mode will be enabled if
>> SGX or TDX are enabled.  So when exactly does the BIOS do an MCHECK?  That I'll
>> have to get clarification on.
> 
> Sorry, this is uncomprehensible word salad and none of this makes any
> sense at all to me.
> 
> Can you please explain this in comprehensible sentences understandable
> by mere mortals?

Basically if the BIOS is configured to enable SGX or TDX, it'll program the APIC
to use x2APIC mode.  At some point it'll have to run MCHECK (which is just an
MSR write).  When exactly the BIOS does this, I'm not sure.  I've asked for
clarification on that.  At the point the BIOS does the MCHECK, if X2APIC mode is
enabled, the ucode will set the LOCK bit, and any attempt after that to disable
the APIC will result in the fault.  Now, this assumes the BIOS will DTRT, and
always enable x2APIC when SGX or TDX are enabled AND always perform the MCHECK,
AND do those things in the right order.  I'm not a BIOS guy so I have no idea
where to even look to see if/where that is documented.  I can certainly try to
track that down if needed.

I'm not sure if that's any clearer?  I guess I could try some code:

if (SGX_enabled() || TDX_enabled()
	set_x2apic_mode();

.....

MCHECK <-----At this point if x2APIC mode is on then the ucode set's the lock bit.

.....


Hope that helps.

> 
> Thanks,
> 
>         tglx

Re: [PATCH] x86/apic: Don't disable x2APIC if locked

[Huang, Kai]

On Wed, 2022-08-10 at 17:59 -0700, Daniel Sneddon wrote:
> On 8/10/22 17:38, Thomas Gleixner wrote:
> > On Wed, Aug 10 2022 at 17:01, Daniel Sneddon wrote:
> > > On 8/10/22 16:44, Dave Hansen wrote:
> > > > On 8/10/22 16:38, Daniel Sneddon wrote:
> > > > > > 
> > > > > > config INTEL_TDX_GUEST
> > > > > >         bool "Intel TDX (Trust Domain Extensions) - Guest Support"
> > > > > >         depends on X86_64 && CPU_SUP_INTEL
> > > > > >         depends on X86_X2APIC
> > > > > So I got some more input.  SPR and newer will lock the APIC.  
> > > > 
> > > > Could you get a _little_ more clarity on this, please?  Exactly how and
> > > > when will it be locked?  What does the BIOS writer's guide say?  Will
> > > > there be an explicit x2APIC lock option?  Or, will it be implicitly
> > > > locked when SGX or TDX is enabled?
> > > 
> > > The BIOS doesn't explicitly lock the APIC.  The APIC will be locked if X2APIC
> > > mode is enabled when the BIOS does an MCHECK.  X2APIC mode will be enabled if
> > > SGX or TDX are enabled.  So when exactly does the BIOS do an MCHECK?  That I'll
> > > have to get clarification on.
> > 
> > Sorry, this is uncomprehensible word salad and none of this makes any
> > sense at all to me.
> > 
> > Can you please explain this in comprehensible sentences understandable
> > by mere mortals?
> 
> Basically if the BIOS is configured to enable SGX or TDX, it'll program the APIC
> to use x2APIC mode.  At some point it'll have to run MCHECK (which is just an
> MSR write).  When exactly the BIOS does this, I'm not sure.  I've asked for
> clarification on that.  At the point the BIOS does the MCHECK, if X2APIC mode is
> enabled, the ucode will set the LOCK bit, and any attempt after that to disable
> the APIC will result in the fault.  Now, this assumes the BIOS will DTRT, and
> always enable x2APIC when SGX or TDX are enabled AND always perform the MCHECK,
> AND do those things in the right order.  I'm not a BIOS guy so I have no idea
> where to even look to see if/where that is documented.  I can certainly try to
> track that down if needed.
> 
> I'm not sure if that's any clearer?  I guess I could try some code:
> 
> if (SGX_enabled() || TDX_enabled()
> 	set_x2apic_mode();
> 
> .....
> 
> MCHECK <-----At this point if x2APIC mode is on then the ucode set's the lock bit.
> 
> .....
> 
> 
> Hope that helps.
> 
> 

Hi Daniel,

This is new to me.  Could you also include me when you are seeking for internal
clarification (and documentation publication)?

-- 
Thanks,
-Kai

Re: [PATCH] x86/apic: Don't disable x2APIC if locked

[Thomas Gleixner]

On Wed, Aug 10 2022 at 16:38, Daniel Sneddon wrote:
> On 8/10/22 16:09, Dave Hansen wrote:
>> config INTEL_TDX_GUEST
>>         bool "Intel TDX (Trust Domain Extensions) - Guest Support"
>>         depends on X86_64 && CPU_SUP_INTEL
>>         depends on X86_X2APIC
>
> So I got some more input.  SPR and newer will lock the APIC.  Older products
> will get a ucode update, but that ucode update won't include the APIC lock.  So,
> on non-SPR parts do we still want to make SGX depend on X2APIC?

What is the ucode update doing on pre SPR parts?
Just providing magic voodoo which pretends to be safe?

The public available documentation for this is a huge pile of void.

The point is that if the SGX attestation will fail when X2APIC is not
enforced on the host as of 'some magic dates in 2023' according to the
documentation I pointed to, then any pre SPR SGX capable system is going
to be disfunctional vs. SGX at one of those magic dates.

Some people inside a particular company need to get their act together
and either make this consistent or provide some coherent information why
this is not required for pre SPR parts and why SPR needs to have it.

Thanks,

        tglx

Re: [PATCH] x86/apic: Don't disable x2APIC if locked

[Daniel Sneddon]

On 8/10/22 17:17, Thomas Gleixner wrote:
> On Wed, Aug 10 2022 at 16:38, Daniel Sneddon wrote:
>> On 8/10/22 16:09, Dave Hansen wrote:
>>> config INTEL_TDX_GUEST
>>>         bool "Intel TDX (Trust Domain Extensions) - Guest Support"
>>>         depends on X86_64 && CPU_SUP_INTEL
>>>         depends on X86_X2APIC
>>
>> So I got some more input.  SPR and newer will lock the APIC.  Older products
>> will get a ucode update, but that ucode update won't include the APIC lock.  So,
>> on non-SPR parts do we still want to make SGX depend on X2APIC?
> 
> What is the ucode update doing on pre SPR parts?
> Just providing magic voodoo which pretends to be safe?
It'll be clearing the buffers so that when someone tries to read data from the
APIC it won't leak data anymore.
> 
> The public available documentation for this is a huge pile of void.
I don't disagree with that.
> 
> The point is that if the SGX attestation will fail when X2APIC is not
> enforced on the host as of 'some magic dates in 2023' according to the
> documentation I pointed to, then any pre SPR SGX capable system is going
> to be disfunctional vs. SGX at one of those magic dates.
>
> Some people inside a particular company need to get their act together
> and either make this consistent or provide some coherent information why
> this is not required for pre SPR parts and why SPR needs to have it.

I'll try to get more clarification, and more importantly, get that published
somewhere.

> 
> Thanks,
> 
>         tglx
> 
> 
Thanks for the input!

Re: [PATCH] x86/apic: Don't disable x2APIC if locked

[Huang, Kai]

On Wed, 2022-08-10 at 16:09 -0700, Dave Hansen wrote:
> On 8/10/22 16:03, Daniel Sneddon wrote:
> > On 8/10/22 15:06, Thomas Gleixner wrote:
> > > So this affects already deployed systems and we have to
> > > 
> > >   - backport the x2apic lock changes
> > > 
> > >   - provide proper diagnostics
> > > 
> > >   - make SGX and TDX depend on X2APIC support
> > I'll add the comments Dave mentioned earlier, and will make SGX and TDX depend
> > on X2APIC since that makes sense regardless of what hw ends up with this change.
> >  Unless we want to get rid of CONFIG_X86_X2APIC like Dave mentioned?
> 
> The TDX guest support in the kernel isn't _actually_ related to this*.
> It's the host-side support that matters and that isn't merged yet.  I've
> cc'd Kai so he doesn't forget to do this.
> 
> 

Hi Dave,

Thanks for the info.  I'll address this before sending out next version.

-- 
Thanks,
-Kai