Re: [PATCH 2/3] x86/boot: Refactor BIOS/PVH start

[Frediano Ziglio <frediano.ziglio@xxxxxxxxx>]

On Sun, Sep 15, 2024 at 7:16 AM Jan Beulich <jbeulich@xxxxxxxx> wrote:
>
> On 10.09.2024 18:16, Frediano Ziglio wrote:
> > --- a/xen/arch/x86/boot/head.S
> > +++ b/xen/arch/x86/boot/head.S
> > @@ -25,6 +25,9 @@
> >  #define MB2_HT(name)      (MULTIBOOT2_HEADER_TAG_##name)
> >  #define MB2_TT(name)      (MULTIBOOT2_TAG_TYPE_##name)
> >
> > +#define BOOT_TYPE_BIOS 1
> > +#define BOOT_TYPE_PVH 2
>
> Did you consider using 0 and 1, to be able to use XOR on the BIOS
> path and TEST for checking?
>

Not clear what you are trying to achieve. Fewer bytes using the XOR? I
think the TEST in this case is only reducing readability, it's an
enumeration.
If you are concerns about code size I would use an 8 bit register (I
would say DL) and use EBP register to temporary save EAX, 8 bit
registers have usually tiny instructions, MOV has same size as XOR you
mentioned not loosing any readability or forcing to change values.

> > @@ -409,13 +412,28 @@ cs32_switch:
> >  ELFNOTE(Xen, XEN_ELFNOTE_PHYS32_ENTRY, .long sym_offs(__pvh_start))
> >
> >  __pvh_start:
> > -        cld
> >          cli
> > +        mov     $BOOT_TYPE_PVH, %ebp
> > +        jmp     common_bios_pvh
> > +#endif /* CONFIG_PVH_GUEST */
> > +
> > +__start:
> > +        cli
> > +        mov     $BOOT_TYPE_BIOS, %ebp
> > +
> > +common_bios_pvh:
>
> I think labels like this one don't need to show up in the symbol
> table, and hence would better start with .L.
>

Done

> > +        cld
>
> So you fold the STDs but not the STIs, despite that not even having
> been first on the PVH path. This decision wants explaining in the
> description, even if just briefly.
>

Just in case, I disable interrupts ASAP. Not that this should change
much the result.
Would you prefer to fold it too?
By "description" do you mean having an additional comment in the code
or something in the commit message?

> > @@ -433,14 +451,9 @@ __pvh_start:
> >          /* Set up stack. */
> >          lea     STACK_SIZE - CPUINFO_sizeof + sym_esi(cpu0_stack), %esp
> >
> > -        call    initialise_bss
>
> I'm little puzzled: The previous patch moved it "as early as possible"
> just for it to be moved to a later point again here?
>

The rationale is being able to use C, for that you need a stack,
correct segments and BSS.
Are you suggesting any change?

> > -        mov     %ebx, sym_esi(pvh_start_info_pa)
> > -
> > -        /* Force xen console.  Will revert to user choice in init code. */
> > -        movb    $-1, sym_esi(opt_console_xen)
> > -
> > -        /* Prepare gdt and segments */
> > +        /*
> > +         * Initialize GDTR and basic data segments.
> > +         */
> >          add     %esi, sym_esi(gdt_boot_base)
> >          lgdt    sym_esi(gdt_boot_descr)
>
> No real need to change the comment? In any even it wants to remain a
> single-line one.
>

It's not changed, it's the exact comment from the second copy of this
code (the BIOS path).
I think this comment is more clear than the first, I'll change to be one line.

> > @@ -449,67 +462,44 @@ __pvh_start:
> >          mov     %ecx, %es
> >          mov     %ecx, %ss
> >
> > -        /* Skip bootloader setup and bios setup, go straight to trampoline 
> > */
> > -        movb    $1, sym_esi(pvh_boot)
> > -        movb    $1, sym_esi(skip_realmode)
> > -
> > -        /* Set trampoline_phys to use mfn 1 to avoid having a mapping at 
> > VA 0 */
> > -        movw    $0x1000, sym_esi(trampoline_phys)
> > -        mov     (%ebx), %eax /* mov $XEN_HVM_START_MAGIC_VALUE, %eax */
> > -        jmp     trampoline_setup
> > -
> > -#endif /* CONFIG_PVH_GUEST */
> > +        /* Load null descriptor to unused segment registers. */
> > +        xor     %ecx, %ecx
> > +        mov     %ecx, %fs
> > +        mov     %ecx, %gs
>
> The comment said "descriptor", yes, but it's a null selector that
> we load here. Perhaps worth adjusting as the comment is moved.
>

I'll do.

> > -initialise_bss:
> >          /*
> >           * Initialise the BSS.
> >           *
> >           * !!! WARNING - also zeroes the current stack !!!
> >           */
> > -        pop     %ebp
> >          mov     %eax, %edx
> > -
> >          lea     sym_esi(__bss_start), %edi
> >          lea     sym_esi(__bss_end), %ecx
> >          sub     %edi, %ecx
> >          xor     %eax, %eax
> >          shr     $2, %ecx
> >          rep stosl
> > -
> >          mov     %edx, %eax
> > -        jmp     *%ebp
> > -
> > -__start:
> > -        cld
> > -        cli
> >
> > -        /*
> > -         * Multiboot (both 1 and 2) specify the stack pointer as undefined
> > -         * when entering in BIOS circumstances.  This is unhelpful for
> > -         * relocatable images, where one call (i.e. push) is required to
> > -         * calculate the image's load address.
> > -         *
> > -         * This early in boot, there is one area of memory we know about 
> > with
> > -         * reasonable confidence that it isn't overlapped by Xen, and 
> > that's
> > -         * the Multiboot info structure in %ebx.  Use it as a temporary 
> > stack.
> > -         */
> > -
> > -        /* Preserve the field we're about to clobber. */
> > -        mov     (%ebx), %edx
> > -        lea     4(%ebx), %esp
> > +#ifdef CONFIG_PVH_GUEST
> > +        cmp     $BOOT_TYPE_PVH, %ebp
> > +        jne     1f
> >
> > -        /* Calculate the load base address. */
> > -        call    1f
> > -1:      pop     %esi
> > -        sub     $sym_offs(1b), %esi
> > +        mov     %ebx, sym_esi(pvh_start_info_pa)
> >
> > -        /* Restore the clobbered field. */
> > -        mov     %edx, (%ebx)
> > +        /* Force xen console.  Will revert to user choice in init code. */
> > +        movb    $-1, sym_esi(opt_console_xen)
> >
> > -        /* Set up stack. */
> > -        lea     STACK_SIZE - CPUINFO_sizeof + sym_esi(cpu0_stack), %esp
> > +        /* Skip bootloader setup and bios setup, go straight to trampoline 
> > */
> > +        movb    $1, sym_esi(pvh_boot)
> > +        movb    $1, sym_esi(skip_realmode)
> >
> > -        call    initialise_bss
> > +        /* Set trampoline_phys to use mfn 1 to avoid having a mapping at 
> > VA 0 */
> > +        movw    $0x1000, sym_esi(trampoline_phys)
>
> I realize you merely move this, yet I question the use of MOVW here.
> You use 32-bit operations e.g. to set %ebp; perhaps the same should
> be done here, even if that means a 1 byte code size increase. In any
> even this would preferably use PAGE_SIZE imo.
>

So movl instead of movw and PAGE_SIZE instead of 0x1000. I'll do.

> Jan

Frediano