Re: [PATCHv9 05/17] x86/kexec: Keep CR4.MCE set during kexec for TDX guest

[David Woodhouse <dwmw2@infradead.org>]

[-- Attachment #1: Type: text/plain, Size: 3976 bytes --]

On Thu, 2024-04-04 at 12:32 +0300, Kirill A. Shutemov wrote:
> On Thu, Apr 04, 2024 at 10:40:34AM +1300, Huang, Kai wrote:
> > 
> > 
> > On 3/04/2024 4:42 am, Kirill A. Shutemov wrote:
> > > On Fri, Mar 29, 2024 at 06:48:21PM +0200, Kirill A. Shutemov wrote:
> > > > On Fri, Mar 29, 2024 at 11:21:32PM +0800, Xiaoyao Li wrote:
> > > > > On 3/25/2024 6:38 PM, Kirill A. Shutemov wrote:
> > > > > > TDX guests are not allowed to clear CR4.MCE. Attempt to clear it leads
> > > > > > to #VE.
> > > > > 
> > > > > Will we consider making it more safe and compatible for future to guard
> > > > > against X86_FEATURE_MCE as well?
> > > > > 
> > > > > If in the future, MCE becomes configurable for TD guest, then CR4.MCE might
> > > > > not be fixed1.
> > > > 
> > > > Good point.
> > > > 
> > > > I guess we can leave it clear if it was clear. This should be easy
> > > > enough. But we might want to clear even if was set if clearing is allowed.
> > > > 
> > > > It would require some kind of indication that clearing MCE is fine. We
> > > > don't have such indication yet. Not sure we can reasonably future-proof
> > > > the code at this point.
> > > > 
> > > > But let me think more.
> > > 
> > > I think I will go with the variant below.
> > > 
> > > diff --git a/arch/x86/kernel/relocate_kernel_64.S b/arch/x86/kernel/relocate_kernel_64.S
> > > index 56cab1bb25f5..8e2037d78a1f 100644
> > > --- a/arch/x86/kernel/relocate_kernel_64.S
> > > +++ b/arch/x86/kernel/relocate_kernel_64.S
> > > @@ -5,6 +5,8 @@
> > >    */
> > >   #include <linux/linkage.h>
> > > +#include <linux/stringify.h>
> > > +#include <asm/alternative.h>
> > >   #include <asm/page_types.h>
> > >   #include <asm/kexec.h>
> > >   #include <asm/processor-flags.h>
> > > @@ -145,11 +147,17 @@ SYM_CODE_START_LOCAL_NOALIGN(identity_mapped)
> > >   	 * Set cr4 to a known state:
> > >   	 *  - physical address extension enabled
> > >   	 *  - 5-level paging, if it was enabled before
> > > +	 *  - Machine check exception on TDX guest, if it was enabled before.
> > > +	 *    Clearing MCE might not allowed in TDX guests, depending on setup.
> > 
> > Nit:  Perhaps we can just call out:
> > 
> > 	Clearing MCE is not allowed if it _was_ enabled before.
> > 
> > Which is always true I suppose.
> 
> It is true now. Future TDX will allow to clear CR4.MCE and we don't want
> to flip it back on in this case.

And yet v12 of the patch which became commit de60613173df does
precisely that.

It uses the original contents of CR4 which are stored in %r13 (instead
of building a completely new set of bits for CR4 as before). So it
would never have *cleared* the CR4.MCE bit now anyway... what it does
is explicitly *set* the bit even if it wasn't set before?

This is what got committed, and I think we can just drop the
ALTERNATIVE line completely because it's redundant in the case that
CR4.MCE was already set, and *wrong* in the case that it wasn't already
set?

@@ -145,14 +147,15 @@ SYM_CODE_START_LOCAL_NOALIGN(identity_mapped)
         * Set cr4 to a known state:
         *  - physical address extension enabled
         *  - 5-level paging, if it was enabled before
+        *  - Machine check exception on TDX guest, if it was enabled before.
+        *    Clearing MCE might not be allowed in TDX guests, depending on setup.
+        *
+        * Use R13 that contains the original CR4 value, read in relocate_kernel().
+        * PAE is always set in the original CR4.
         */
-       movl    $X86_CR4_PAE, %eax
-       testq   $X86_CR4_LA57, %r13
-       jz      .Lno_la57
-       orl     $X86_CR4_LA57, %eax
-.Lno_la57:
-
-       movq    %rax, %cr4
+       andl    $(X86_CR4_PAE | X86_CR4_LA57), %r13d
+       ALTERNATIVE "", __stringify(orl $X86_CR4_MCE, %r13d), X86_FEATURE_TDX_GUEST
+       movq    %r13, %cr4
 
        /* Flush the TLB (needed?) */
        movq    %r9, %cr3



[-- Attachment #2: smime.p7s --]
[-- Type: application/pkcs7-signature, Size: 5069 bytes --]