Linux Virtualization - Aug 2020 - [PATCH v6 38/76] x86/head/64: Set CR4.FSGSBASE early

On Mon, Aug 24, 2020 at 10:54:33AM +0200, Joerg Roedel
wrote:
> From: Joerg Roedel <jroedel at suse.de>
> 
> Early exception handling will use rd/wrgsbase in paranoid_entry/exit.
> Enable the feature to avoid #UD exceptions on boot APs.
> 
> Signed-off-by: Joerg Roedel <jroedel at suse.de>
> Link: https://lore.kernel.org/r/20200724160336.5435-38-joro at 8bytes.org
> ---
>  arch/x86/kernel/head_64.S | 7 +++++++
>  1 file changed, 7 insertions(+)
> 
> diff --git a/arch/x86/kernel/head_64.S b/arch/x86/kernel/head_64.S
> index 08412f308de3..4622940134a5 100644
> --- a/arch/x86/kernel/head_64.S
> +++ b/arch/x86/kernel/head_64.S
> @@ -153,6 +153,13 @@ SYM_CODE_START(secondary_startup_64)
>  	orl	$X86_CR4_LA57, %ecx
>  1:
>  #endif
> +
> +	ALTERNATIVE "jmp .Lstartup_write_cr4", "",
X86_FEATURE_FSGSBASE
> +
> +	/* Early exception handling uses FSGSBASE on APs */
> +	orl	$X86_CR4_FSGSBASE, %ecx
How is this supposed to work?

Alternatives haven't run that early yet and that piece of code looks
like this:

ffffffff81000067:       eb 06                   jmp    ffffffff8100006f
<secondary_startup_64+0x1f>
ffffffff81000069:       81 c9 00 00 01 00       or     $0x10000,%ecx
ffffffff8100006f:       0f 22 e1                mov    %rcx,%cr4

so we'll never set X86_CR4_FSGSBASE during early boot.

Stopping a guest with gdb just before that shows the same thing:

Dump of assembler code from 0x1000069 to 0x100007b:
=> 0x0000000001000069:  eb 06   jmp    0x1000071
   0x000000000100006b:  81 c9 00 00 01 00       or     $0x10000,%ecx
   0x0000000001000071:  0f 22 e1        mov    %rcx,%cr4
   0x0000000001000074:  48 03 05 95 ff 20 01    add    0x120ff95(%rip),%rax     
# 0x2210010

the unconditional JMP is there and it hasn't been patched out yet.

If you really need to test CPUID flags, you need to do something similar
to what verify_cpu does that early. And looking at that thing:

 *      verify_cpu, returns the status of longmode and SSE in register %eax.
 *              0: Success    1: Failure

you could return the FSGSBASE CPUID bit there too and act accordingly.

Hmm.

-- 
Regards/Gruss,
    Boris.

https://people.kernel.org/tglx/notes-about-netiquette

On Sat, Aug 29, 2020 at 05:55:25PM +0200, Borislav Petkov
wrote:
> On Mon, Aug 24, 2020 at 10:54:33AM +0200, Joerg Roedel wrote:
> > From: Joerg Roedel <jroedel at suse.de>
> > 
> > Early exception handling will use rd/wrgsbase in paranoid_entry/exit.
> > Enable the feature to avoid #UD exceptions on boot APs.
> > 
> > Signed-off-by: Joerg Roedel <jroedel at suse.de>
> > Link: https://lore.kernel.org/r/20200724160336.5435-38-joro at
8bytes.org
> > ---
> >  arch/x86/kernel/head_64.S | 7 +++++++
> >  1 file changed, 7 insertions(+)
> > 
> > diff --git a/arch/x86/kernel/head_64.S b/arch/x86/kernel/head_64.S
> > index 08412f308de3..4622940134a5 100644
> > --- a/arch/x86/kernel/head_64.S
> > +++ b/arch/x86/kernel/head_64.S
> > @@ -153,6 +153,13 @@ SYM_CODE_START(secondary_startup_64)
> >  	orl	$X86_CR4_LA57, %ecx
> >  1:
> >  #endif
> > +
> > +	ALTERNATIVE "jmp .Lstartup_write_cr4", "",
X86_FEATURE_FSGSBASE
> > +
> > +	/* Early exception handling uses FSGSBASE on APs */
> > +	orl	$X86_CR4_FSGSBASE, %ecx
> 
> How is this supposed to work?
> 
> Alternatives haven't run that early yet and that piece of code looks
> like this:
> 
> ffffffff81000067:       eb 06                   jmp    ffffffff8100006f
<secondary_startup_64+0x1f>
> ffffffff81000069:       81 c9 00 00 01 00       or     $0x10000,%ecx
> ffffffff8100006f:       0f 22 e1                mov    %rcx,%cr4
> 
> so we'll never set X86_CR4_FSGSBASE during early boot.
This is not needed on the boot CPU, but only on secondary CPUs. When
those are brought up the alternatives have been patches already. The
commit message should probably be more clear about that, I will fix
that.

The CR4 bit also can't be set unconditionally here on the boot CPU,
because at that point the kernel does not know whether the CPU has
support for the fsgsbase instructions.


Regards,

	Joerg

On Mon, Aug 31, 2020 at 10:58:10AM +0200, Joerg Roedel
wrote:
> This is not needed on the boot CPU, but only on secondary CPUs. When
> those are brought up the alternatives have been patches already. The
> commit message should probably be more clear about that, I will fix
> that.
Hell yeah - you need to talk more in those commit messages sir! See,
we're not in your head... :-)))

And pls put that as a comment over the code - the commit message will
not be that easily accessible in years.

Thx.

-- 
Regards/Gruss,
    Boris.

https://people.kernel.org/tglx/notes-about-netiquette