x86/wakeup: Fix code generation for bogus_saved_magic

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

Posted by Andrew Cooper 11 months, 4 weeks ago

bogus_saved_magic() is in a .code64 section but invokved in 32bit mode.  This
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
sanity sake.  Annotate it with ELF metadata while doing so.

Fixes: d8c8fef09054 ("Provide basic Xen PM infrastructure")
Signed-off-by: Andrew Cooper <andrew.cooper3@citrix.com>
---
CC: Jan Beulich <JBeulich@suse.com>
CC: Roger Pau Monné <roger.pau@citrix.com>

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)
-- 
2.39.5

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

Posted by Jan Beulich 11 months, 3 weeks ago

On 09.11.2024 01:36, Andrew Cooper wrote:
> bogus_saved_magic() is in a .code64 section but invokved in 32bit mode.  This
> 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
> sanity sake.  Annotate it with ELF metadata while doing so.
> 
> Fixes: d8c8fef09054 ("Provide basic Xen PM infrastructure")
> Signed-off-by: Andrew Cooper <andrew.cooper3@citrix.com>

Reviewed-by: Jan Beulich <jbeulich@suse.com>

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

Posted by Frediano Ziglio 11 months, 4 weeks ago

On Sat, Nov 9, 2024 at 12:37 AM Andrew Cooper <andrew.cooper3@citrix.com> 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

> sanity sake.  Annotate it with ELF metadata while doing so.
>
> Fixes: d8c8fef09054 ("Provide basic Xen PM infrastructure")
> Signed-off-by: Andrew Cooper <andrew.cooper3@citrix.com>
> ---
> CC: Jan Beulich <JBeulich@suse.com>
> CC: Roger Pau Monné <roger.pau@citrix.com>
>
> 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.

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

BTW

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

Frediano

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

Posted by Andrew Cooper 11 months, 3 weeks ago

On 09/11/2024 5:29 pm, Frediano Ziglio wrote:
> On Sat, Nov 9, 2024 at 12:37 AM Andrew Cooper <andrew.cooper3@citrix.com> 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@citrix.com>
>> ---
>> CC: Jan Beulich <JBeulich@suse.com>
>> CC: Roger Pau Monné <roger.pau@citrix.com>
>>
>> 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@cloud.com>

Thanks.

~Andrew

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

Posted by Frediano Ziglio 11 months, 3 weeks ago

On Mon, Nov 11, 2024 at 11:05 AM Andrew Cooper
<andrew.cooper3@citrix.com> wrote:
>
> On 09/11/2024 5:29 pm, Frediano Ziglio wrote:
> > On Sat, Nov 9, 2024 at 12:37 AM Andrew Cooper <andrew.cooper3@citrix.com> 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@citrix.com>
> >> ---
> >> CC: Jan Beulich <JBeulich@suse.com>
> >> CC: Roger Pau Monné <roger.pau@citrix.com>
> >>
> >> 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.
>

I don't think livepatching this code would be so easy. That code is
copied to low memory and discarded (.init section) later, so patching
the code in the current ELF code would corrupt Xen memory.

> > 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.
>

In head.S we write messages in both VGA (if possible) and serial.
Anyway... out of scope here.


> > Reviewed-by: Frediano Ziglio <frediano.ziglio@cloud.com>
>
> Thanks.
>
> ~Andrew

Frediano

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

Posted by Andrew Cooper 11 months, 3 weeks ago

On 11/11/2024 11:21 am, Frediano Ziglio wrote:
>>>> sanity sake.  Annotate it with ELF metadata while doing so.
>>>>
>>>> Fixes: d8c8fef09054 ("Provide basic Xen PM infrastructure")
>>>> Signed-off-by: Andrew Cooper <andrew.cooper3@citrix.com>
>>>> ---
>>>> CC: Jan Beulich <JBeulich@suse.com>
>>>> CC: Roger Pau Monné <roger.pau@citrix.com>
>>>>
>>>> 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.
>>
> I don't think livepatching this code would be so easy. That code is
> copied to low memory and discarded (.init section) later, so patching
> the code in the current ELF code would corrupt Xen memory.

Indeed.  This specific code is out of scope for livepatching.

But, we are (slowly) cleaning up all assembly code to use proper ELF
metadata.

~Andrew