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

[Frediano Ziglio <frediano.ziglio@xxxxxxxxx>]

On Mon, Nov 11, 2024 at 11:05 AM Andrew Cooper
<andrew.cooper3@xxxxxxxxxx> wrote:
>
> 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.
>

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@xxxxxxxxx>
>
> Thanks.
>
> ~Andrew

Frediano