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

From: Dave Hansen <dave.hansen@intel.com>
To: Daniel Sneddon <daniel.sneddon@linux.intel.com>,
	Thomas Gleixner <tglx@linutronix.de>,
	Ingo Molnar <mingo@redhat.com>, Borislav Petkov <bp@alien8.de>,
	Dave Hansen <dave.hansen@linux.intel.com>,
	x86@kernel.org, "Shutemov, Kirill" <kirill.shutemov@intel.com>
Cc: "H. Peter Anvin" <hpa@zytor.com>, linux-kernel@vger.kernel.org
Subject: Re: [PATCH] x86/apic: Don't disable x2APIC if locked
Date: Wed, 10 Aug 2022 11:01:29 -0700	[thread overview]
Message-ID: <d6ffb489-7024-ff74-bd2f-d1e06573bb82@intel.com> (raw)
In-Reply-To: <20220809234000.783284-1-daniel.sneddon@linux.intel.com>

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>