x86/realmode: Make stack lock work in trampoline_compat()

[patch] x86/realmode: Make stack lock work in trampoline_compat()

Posted by Thomas Gleixner 10 months, 3 weeks ago

The stack locking and stack assignment macro LOAD_REALMODE_ESP fails to
work when invoked from the 64bit trampoline entry point:

trampoline_start64
  trampoline_compat
    LOAD_REALMODE_ESP <- lock

Accessing tr_lock is only possible from 16bit mode. For the compat entry
point this needs to be pa_tr_lock so that the required relocation entry is
generated. Otherwise it locks the non-relocated address which is
aside of being wrong never cleared in secondary_startup_64() causing all
but the first CPU to get stuck on the lock.

Make the macro take an argument lock_pa which defaults to 0 and rename it
to LOCK_AND_LOAD_REALMODE_ESP to make it clear what this is about.

Fixes: f6f1ae9128d2 ("x86/smpboot: Implement a bit spinlock to protect the realmode stack")
Reported-by: Kirill A. Shutemov <kirill.shutemov@linux.intel.com>
Signed-off-by: Thomas Gleixner <tglx@linutronix.de>
---
 arch/x86/realmode/rm/trampoline_64.S |   12 ++++++++----
 1 file changed, 8 insertions(+), 4 deletions(-)

--- a/arch/x86/realmode/rm/trampoline_64.S
+++ b/arch/x86/realmode/rm/trampoline_64.S
@@ -37,12 +37,16 @@
 	.text
 	.code16
 
-.macro LOAD_REALMODE_ESP
+.macro LOCK_AND_LOAD_REALMODE_ESP lock_pa=0
 	/*
 	 * Make sure only one CPU fiddles with the realmode stack
 	 */
 .Llock_rm\@:
+	.if \lock_pa
+        lock btsl       $0, pa_tr_lock
+	.else
         lock btsl       $0, tr_lock
+	.endif
         jnc             2f
         pause
         jmp             .Llock_rm\@
@@ -63,7 +67,7 @@ SYM_CODE_START(trampoline_start)
 	mov	%ax, %es
 	mov	%ax, %ss
 
-	LOAD_REALMODE_ESP
+	LOCK_AND_LOAD_REALMODE_ESP
 
 	call	verify_cpu		# Verify the cpu supports long mode
 	testl   %eax, %eax		# Check for return code
@@ -106,7 +110,7 @@ SYM_CODE_START(sev_es_trampoline_start)
 	mov	%ax, %es
 	mov	%ax, %ss
 
-	LOAD_REALMODE_ESP
+	LOCK_AND_LOAD_REALMODE_ESP
 
 	jmp	.Lswitch_to_protected
 SYM_CODE_END(sev_es_trampoline_start)
@@ -189,7 +193,7 @@ SYM_CODE_START(pa_trampoline_compat)
 	 * In compatibility mode.  Prep ESP and DX for startup_32, then disable
 	 * paging and complete the switch to legacy 32-bit mode.
 	 */
-	LOAD_REALMODE_ESP
+	LOCK_AND_LOAD_REALMODE_ESP lock_pa=1
 	movw	$__KERNEL_DS, %dx
 
 	movl	$(CR0_STATE & ~X86_CR0_PG), %eax

Re: [patch] x86/realmode: Make stack lock work in trampoline_compat()

Posted by Yunhong Jiang 10 months, 2 weeks ago

On Tue, May 30, 2023 at 12:46:22PM +0200, Thomas Gleixner wrote:
> The stack locking and stack assignment macro LOAD_REALMODE_ESP fails to
> work when invoked from the 64bit trampoline entry point:
> 
> trampoline_start64
>   trampoline_compat
>     LOAD_REALMODE_ESP <- lock
One possibly dumb question and hope get some hints. The LOAD_REALMODE_ESP is
defined under .code16 directive and will be used by 32-bit mode caller also. Is
it ok because the instructions there will be same for both 16-bit and 32-bit? I
checked
https://ftp.gnu.org/old-gnu/Manuals/gas-2.9.1/html_chapter/as_16.html#SEC205 and
don't find much information there.

Re: [patch] x86/realmode: Make stack lock work in trampoline_compat()

Posted by Andrew Cooper 10 months, 2 weeks ago

On 09/06/2023 12:34 am, Yunhong Jiang wrote:
> On Tue, May 30, 2023 at 12:46:22PM +0200, Thomas Gleixner wrote:
>> The stack locking and stack assignment macro LOAD_REALMODE_ESP fails to
>> work when invoked from the 64bit trampoline entry point:
>>
>> trampoline_start64
>>   trampoline_compat
>>     LOAD_REALMODE_ESP <- lock
> One possibly dumb question and hope get some hints.

There's a phrase.  "The only dumb question is the one not asked".

If you have this question, there's an excellent chance that someone else
reading this thread has the same question.

>  The LOAD_REALMODE_ESP is
> defined under .code16 directive and will be used by 32-bit mode caller also. Is
> it ok because the instructions there will be same for both 16-bit and 32-bit? I
> checked
> https://ftp.gnu.org/old-gnu/Manuals/gas-2.9.1/html_chapter/as_16.html#SEC205 and
> don't find much information there.

The position of the LOAD_REALMODE_ESP .macro itself doesn't matter. 
It's just some text which gets pasted elsewhere.  Imagine it just the
same as running the C preprocessor on a file before compiling it.

As you note, some expansions of the macro are in .code16, and some are
not.  This does result in different bytes being emitted.  The default
operands size flips between .code16 and .code32, so there will be some
0x66 prefixes in one mode, and not in others.

The important point is the l suffix on btsl, which forces it to be long
(32bit) irrespective of the default operand size.

So yes, it will work, but that's because gas is handling the differing
encodings automatically based on the default operand size where the
LOAD_REALMODE_ESP gets expanded.

Hope this helps,

~Andrew

RE: [patch] x86/realmode: Make stack lock work in trampoline_compat()

Posted by David Laight 10 months, 1 week ago

From: Andrew Cooper
> Sent: 09 June 2023 00:58
> 
...
> The important point is the l suffix on btsl, which forces it to be long
> (32bit) irrespective of the default operand size.

Does that matter at all?
The 'bit' opcodes (I'm sure 'bts' is 'bit test and set') take
a bit offset from the base address.
This accesses the same bit regardless of the operand size.

The one real issue is that a byte operand will only lock the one byte.
This might be problematic if non-bit locked accesses are also used.
Although it would need to be rather obscure use.
(This may be one of them...)

The only other problem is that btsl always does a locked 32bit
access. If the base address is misaligned this is a misaligned
locked access - problematic if it crosses a cache line boundary.

	David

-
Registered Address Lakeside, Bramley Road, Mount Farm, Milton Keynes, MK1 1PT, UK
Registration No: 1397386 (Wales)

Re: [patch] x86/realmode: Make stack lock work in trampoline_compat()

Posted by 'Andrew Cooper' 10 months, 1 week ago

On 10/06/2023 8:50 pm, David Laight wrote:
> From: Andrew Cooper
>> Sent: 09 June 2023 00:58
>>
> ...
>> The important point is the l suffix on btsl, which forces it to be long
>> (32bit) irrespective of the default operand size.
> Does that matter at all?

Yes, or I wouldn't have wasted everyone's time saying otherwise.

I recommend educating yourself with the instruction manual, rather than
presenting a set of statements which are all wrong.

~Andrew

Re: [patch] x86/realmode: Make stack lock work in trampoline_compat()

Posted by Yunhong Jiang 10 months, 2 weeks ago

On Fri, Jun 09, 2023 at 12:57:46AM +0100, Andrew Cooper wrote:
> On 09/06/2023 12:34 am, Yunhong Jiang wrote:
> > On Tue, May 30, 2023 at 12:46:22PM +0200, Thomas Gleixner wrote:
> >> The stack locking and stack assignment macro LOAD_REALMODE_ESP fails to
> >> work when invoked from the 64bit trampoline entry point:
> >>
> >> trampoline_start64
> >>   trampoline_compat
> >>     LOAD_REALMODE_ESP <- lock
> > One possibly dumb question and hope get some hints.
> 
> There's a phrase.  "The only dumb question is the one not asked".
> 
> If you have this question, there's an excellent chance that someone else
> reading this thread has the same question.
> 
> >  The LOAD_REALMODE_ESP is
> > defined under .code16 directive and will be used by 32-bit mode caller also. Is
> > it ok because the instructions there will be same for both 16-bit and 32-bit? I
> > checked
> > https://ftp.gnu.org/old-gnu/Manuals/gas-2.9.1/html_chapter/as_16.html#SEC205 and
> > don't find much information there.
> 
> The position of the LOAD_REALMODE_ESP .macro itself doesn't matter. 
> It's just some text which gets pasted elsewhere.  Imagine it just the
> same as running the C preprocessor on a file before compiling it.
> 
> As you note, some expansions of the macro are in .code16, and some are
> not.  This does result in different bytes being emitted.  The default
> operands size flips between .code16 and .code32, so there will be some
> 0x66 prefixes in one mode, and not in others.
> 
> The important point is the l suffix on btsl, which forces it to be long
> (32bit) irrespective of the default operand size.
> 
> So yes, it will work, but that's because gas is handling the differing
> encodings automatically based on the default operand size where the
> LOAD_REALMODE_ESP gets expanded.
> 
> Hope this helps,
Thank you for the explaination, it's quite clear now.
> 
> ~Andrew

Re: [patch] x86/realmode: Make stack lock work in trampoline_compat()

Posted by Kirill A. Shutemov 10 months, 3 weeks ago

On Tue, May 30, 2023 at 12:46:22PM +0200, Thomas Gleixner wrote:
> The stack locking and stack assignment macro LOAD_REALMODE_ESP fails to
> work when invoked from the 64bit trampoline entry point:
> 
> trampoline_start64
>   trampoline_compat
>     LOAD_REALMODE_ESP <- lock
> 
> Accessing tr_lock is only possible from 16bit mode. For the compat entry
> point this needs to be pa_tr_lock so that the required relocation entry is
> generated. Otherwise it locks the non-relocated address which is
> aside of being wrong never cleared in secondary_startup_64() causing all
> but the first CPU to get stuck on the lock.
> 
> Make the macro take an argument lock_pa which defaults to 0 and rename it
> to LOCK_AND_LOAD_REALMODE_ESP to make it clear what this is about.
> 
> Fixes: f6f1ae9128d2 ("x86/smpboot: Implement a bit spinlock to protect the realmode stack")
> Reported-by: Kirill A. Shutemov <kirill.shutemov@linux.intel.com>
> Signed-off-by: Thomas Gleixner <tglx@linutronix.de>

Tested-by: Kirill A. Shutemov <kirill.shutemov@linux.intel.com>

-- 
  Kiryl Shutsemau / Kirill A. Shutemov