Re: [PATCH] x86/wakeup: Fix code generation for bogus_saved_magic

[Andrew Cooper <andrew.cooper3@xxxxxxxxxx>]

On 09/11/2024 5:29 pm, Frediano Ziglio wrote:
> On Sat, Nov 9, 2024 at 12:37 AM Andrew Cooper <andrew.cooper3@xxxxxxxxxx> 
> wrote:
>> bogus_saved_magic() is in a .code64 section but invokved in 32bit mode.  This
> Typo: invoked
>
>> causes a real encoding difference.
>>
>> Before:
>>   66 c7 04 25 14 80 0b 00 53 0e    movw   $0xe53,0xb8014(,%eiz,1)
>>
>> After:
>>   66 c7 05 14 80 0b 00 53 0e       movw   $0xe53,0xb8014
>>
>> The differnce happens to be benign, but move the logic back into a .code32 
>> for
> Typo: difference

Thanks.  I'd noticed and fixed up locally.

>
>> sanity sake.  Annotate it with ELF metadata while doing so.
>>
>> Fixes: d8c8fef09054 ("Provide basic Xen PM infrastructure")
>> Signed-off-by: Andrew Cooper <andrew.cooper3@xxxxxxxxxx>
>> ---
>> CC: Jan Beulich <JBeulich@xxxxxxxx>
>> CC: Roger Pau Monné <roger.pau@xxxxxxxxxx>
>>
>> This issue dates back to the very introduction of S3 support in Xen, in 2007.
>> ---
>>  xen/arch/x86/boot/wakeup.S | 9 +++++----
>>  1 file changed, 5 insertions(+), 4 deletions(-)
>>
>> diff --git a/xen/arch/x86/boot/wakeup.S b/xen/arch/x86/boot/wakeup.S
>> index 08447e193496..c929fe921823 100644
>> --- a/xen/arch/x86/boot/wakeup.S
>> +++ b/xen/arch/x86/boot/wakeup.S
>> @@ -153,15 +153,16 @@ wakeup_32:
>>          /* Now in compatibility mode. Long-jump to 64-bit mode */
>>          ljmp    $BOOT_CS64, $bootsym_rel(wakeup_64,6)
>>
>> +FUNC_LOCAL(bogus_saved_magic, 0)
>> +        movw    $0x0e00 + 'S', 0xb8014
>> +        jmp     bogus_saved_magic
>> +END(bogus_saved_magic)
>> +
>>          .code64
>>  wakeup_64:
>>          /* Jump to high mappings and the higher-level wakeup code. */
>>          movabs  $s3_resume, %rbx
>>          jmp     *%rbx
>>
>> -bogus_saved_magic:
>> -        movw    $0x0e00 + 'S', 0xb8014
>> -        jmp     bogus_saved_magic
>> -
>>  /* Stack for wakeup: rest of first trampoline page. */
>>  ENTRY(wakeup_stack_start)
> Hi,
>    I agree with the code move, it's supposed to be 32 bit so it should
> be in the 32 bit section.
> Does the ELF annotation help with debug information? Maybe worth
> adding to the comment.

As said in the commit message, it's simply ELF metadata (symbol type and
size).

It doesn't interact with debug symbols, so far as I'm aware.

It's mainly for livepatching (the ELF metadata is how changes are
identified), but we're applying it uniformly to all assembly as a
cleanup activity.

> OT: If I understood correctly, the code is writing a character on
> screen in a tight loop. Maybe an hlt instruction could be helpful?

Yeah, it's not exactly great code, but it needs more adjustments than
just a hlt.

It ought to know about CONFIG_VIDEO, and ideally video=none.

> Reviewed-by: Frediano Ziglio <frediano.ziglio@xxxxxxxxx>

Thanks.

~Andrew