Message-ID: <ZjmlNSwForEVNeOv@aschofie-mobl2>
Date: Mon, 6 May 2024 20:51:17 -0700
From: Alison Schofield <alison.schofield@...el.com>
To: "Huang, Kai" <kai.huang@...el.com>
Cc: "tglx@...utronix.de" <tglx@...utronix.de>,
	"mingo@...hat.com" <mingo@...hat.com>,
	"x86@...nel.org" <x86@...nel.org>, "bp@...en8.de" <bp@...en8.de>,
	"dave.hansen@...ux.intel.com" <dave.hansen@...ux.intel.com>,
	"hpa@...or.com" <hpa@...or.com>,
	"kirill.shutemov@...ux.intel.com" <kirill.shutemov@...ux.intel.com>,
	"linux-kernel@...r.kernel.org" <linux-kernel@...r.kernel.org>,
	"kai.huang@...ux.intel.com" <kai.huang@...ux.intel.com>
Subject: Re: [PATCH 1/2] x86/cpu: Remove useless work in detect_tme_early()

On Mon, Apr 29, 2024 at 04:17:12AM -0700, Huang, Kai wrote:
> On Thu, 2024-04-25 at 21:24 -0700, alison.schofield@...el.com wrote:
> > From: Alison Schofield <alison.schofield@...el.com>
> >
> > TME (Total Memory Encryption) and MKTME (Multi-Key Total Memory
> > Encryption) BIOS detection were introduced together here [1] and
> > are loosely coupled in the Intel CPU init code.
> >
> > TME is a hardware only feature and its BIOS status is all that needs
> > to be shared with the kernel user: enabled or disabled. The TME
> > algorithm the BIOS is using and whether or not the kernel recognizes
> > that algorithm is useless to the kernel user.
> >
> > MKTME is a hardware feature that requires kernel support. MKTME
> > detection code was added in advance of broader kernel support for
> > MKTME that never followed. So, rather than continuing to emit
> > needless and confusing messages about BIOS MKTME status, remove
> > most of the MKTME pieces from detect_tme_early().
> >
> > Keep one important piece: when the BIOS is configured with MKTME
> > 'on' any BIOS defined KeyID bits do take away from the physaddr bits
> > available in the kernel. Add a pr_info_once() informing about the
> > enabled keyids so the user can address (by rebooting with MKTME off)
> > if the user needs to recover the MKTME consumed bits.
> 
> Nitpickings below:

Hi Kai,
Thanks for the review -

> 
> The original code checks the MSR value consistency between BSP and APs,
> but the new code removed that.

BSP? AP?

I understood the check you are referring to as being for pronouncing MKTME
as disabled. When MKTME is removed from consideration, as is done here, the
code doesn't need to remember and compare a 'state'. It examines tme_activate
of each CPU. If one is !_LOCKED or !_ENABLED, the 'not enabled by BIOS'
message is emitted and the cpu cap is cleared. If another CPU reports TME
as properly enabled before or after that failed CPU, it is still disabled
in the kernel view. So, we don't set a state, like was done for MKTME, but
the worst status is *the* TME status.

> 
> I think the changelog should mention why it is OK.
> 
> I guess perhaps we can say something like the machine shouldn't be able to
> boot if BIOS configures TME activate MSR inconsistently among CPUs, so
> don't bother to have the consistency check.

I basically understand what you are saying, but because I don't see
that explanation in the spec, I wouldn't write that this code relies
on it.

> 
> >
> > There is no functional change for the user, only this change in boot
> > messages:
> >
> > Before:
> > [] x86/tme: enabled by BIOS
> > [] x86/tme: Unknown policy is active: 0x2
> > [] x86/mktme: No known encryption algorithm is supported: 0x4
> > [] x86/mktme: enabled by BIOS
> > [] x86/mktme: 127 KeyIDs available
> >
> > After:
> > [] x86/tme: enabled by BIOS
> > [] x86/mktme: BIOS enabled 127 keyids
> 
> Regarding to what to print:
> 
> 1) IMHO, it's still better to print the algorithm here.  The user/admin
> may want to know what algorithm is enabled by the BIOS (especially there
> are more than one that can be selected in the BIOS).  E.g., the user may
> find the default AES-XTS-128 (0) isn't secure enough and wants the more
> secure algorithm using 256-bit key.
> 
> I understand we might not want to maintain a "value to name" table for
> this so the kernel can print the algorithm in name, but it would be still
> helpful if we just dump the raw value here like:
> 
>   x86/tme: policy activated: 0x2

The path that is cleaned up here never emitted the policies, either by
value or name. It only emitted a message if the policy was unknown. This
is a new (albeit little) feature request and seems it would be best
submitted as a separate patch.

> 
> 2) Given the kernel doesn't support MKTME, I think people may be more
> interested in the reduced physical address bits, instead of how MKTME
> keyIDs are activated.
> 
>   x86/tme: MKTME enabled, physical address bits reduced from <xx> to <xx>.

I added something like this in v2:
x86/mktme: BIOS enabled: x86_phys_bits reduced by %d

I chose to report just the number of bits reduced because it is not
clear to me that this code is, or always will be, the only path
mucking with the number of phys addr bits. For that reason I didn't
want to spew absolute values. User can look up their 'final' number
using lscpu or /proc/cpu, right?

snip

> > +
> > +     pr_info_once("x86/mktme: BIOS enabled %d keyids\n", nr_keyids);
> 
> If I read correctly, if you print
> 
>   physical address bits reduced from <xx> to <xx>.
> 

as I wrote above, didn't write it quite like that...

> instead of the number of KeyIDs, you can even get rid of the 'nr_keyids'
> variable.

Got rid of nr_keyids. 

-- Alison

Powered by blists - more mailing lists