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@xxxxxxxxxxxxxxx>

Thanks!