Re: [PATCH v3] x86/boot: Preserve the value clobbered by the load-base calculation

[Jan Beulich <jbeulich@xxxxxxxx>]

On 23.08.2024 15:10, Andrew Cooper wrote:
> From: Frediano Ziglio <frediano.ziglio@xxxxxxxxx>
> 
> Right now, Xen clobbers the value at 0xffc when performing it's load-base
> calculation.  We've got plenty of free registers at this point, so the value
> can be preserved easily.
> 
> This fixes a real bug booting under Coreboot+SeaBIOS, where 0xffc happens to
> be the cbmem pointer (e.g. Coreboot's dmesg ring, among other things).
> 
> However, there's also a better choice of memory location to use than 0xffc, as
> all our supported boot protocols have a pointer to an info structure in %ebx.
> 
> Update the documentation to match.
> 
> Fixes: 1695e53851e5 ("x86/boot: Fix the boot time relocation calculations")
> Fixes: d96bb172e8c9 ("x86/entry: Early PVH boot code")
> Signed-off-by: Frediano Ziglio <frediano.ziglio@xxxxxxxxx>
> Signed-off-by: Andrew Cooper <andrew.cooper3@xxxxxxxxxx>

Reviewed-by: Jan Beulich <jbeulich@xxxxxxxx>
with two nits:

> --- a/docs/hypervisor-guide/x86/how-xen-boots.rst
> +++ b/docs/hypervisor-guide/x86/how-xen-boots.rst
> @@ -96,6 +96,12 @@ Xen, once loaded into memory, identifies its position in 
> order to relocate
>  system structures.  For 32bit entrypoints, this necessarily requires a call
>  instruction, and therefore a stack, but none of the ABIs provide one.
>  
> -Overall, given that on a BIOS-based system, the IVT and BDA occupy the first
> -5/16ths of the first page of RAM, with the rest free to use, Xen assumes the
> -top of the page is safe to use.
> +In each supported 32bit entry protocol, ``%ebx`` is a pointer to an info
> +structure, and it is highly likely that this structure does not overlap with
> +Xen.  Therefore we use this as as a temporary stack, preserving the prior

Double "as".

> @@ -460,21 +466,26 @@ __start:
>          /*
>           * Multiboot (both 1 and 2) specify the stack pointer as undefined
>           * when entering in BIOS circumstances.  This is unhelpful for
> -         * relocatable images, where one push/pop is required to calculate
> -         * images load address.
> +         * relocatable images, where one call (i.e. push) is required to
> +         * calculate images load address.

Perhaps "the" after "calculate" and (albeit you're the native speaker) isn't
it "image's" then?

Jan