Re: [Xen-devel] [PATCH for-4.9] x86/boot: Fix the boot time relocation calculations

[Daniel Kiper <daniel.kiper@xxxxxxxxxx>]

On Fri, Jun 02, 2017 at 05:57:46PM +0100, Andrew Cooper wrote:
> c/s b28044226e1 "x86: make Xen early boot code relocatable" introduces
>
>     mov $sym_offs(__image_base__),%esi
>
> to the legacy boot path.  However, this is by definition 0, which means the
> boot code only functions correctly when Xen is loaded at its preferred
> physical address (2M at the time of writing).
>
> Xen does cope if loaded at an alternative physical address, if the
> MULTIBOOT2_TAG_TYPE_LOAD_BASE_ADDR tag is filled in properly.  While recent
> versions of Grub do fill this in appropriately, tboot does not.  (In fact,
> tboot loads Xen at the preferred address, but claims a load address of 8M.)

Does it use Multiboot or Multiboot2 protocol? Anyway, I think that this
should be fixed in tboot.

> Both multiboot 1 and 2 specify the execution environment as being flat.  As a
> result, Xen needs no help calculating the proper load address.
>
> Calculate the load address from %eip alone, and ignore
> MULTIBOOT2_TAG_TYPE_LOAD_BASE_ADDR entirely.  This fixes legacy boot under
> various versions of tboot.
>
> Signed-off-by: Andrew Cooper <andrew.cooper3@xxxxxxxxxx>
> Tested-by: Sergey Dyasli <sergey.dyasli@xxxxxxxxxx>
> ---
> CC: Jan Beulich <JBeulich@xxxxxxxx>
> CC: Julien Grall <julien.grall@xxxxxxx>
> CC: Daniel Kiper <daniel.kiper@xxxxxxxxxx>
> CC: Doug Goldstein <cardoe@xxxxxxxxxx>
> CC: Sergey Dyasli <sergey.dyasli@xxxxxxxxxx>
>
> This is a regression introduced in Xen 4.9, and should therefore be fixed.
> ---
>  xen/arch/x86/boot/head.S | 15 ++++-----------
>  1 file changed, 4 insertions(+), 11 deletions(-)
>
> diff --git a/xen/arch/x86/boot/head.S b/xen/arch/x86/boot/head.S
> index 5e84e42..df28b09 100644
> --- a/xen/arch/x86/boot/head.S
> +++ b/xen/arch/x86/boot/head.S
> @@ -377,8 +377,10 @@ __start:
>          cld
>          cli
>
> -        /* Load default Xen image load base address. */
> -        mov     $sym_offs(__image_base__),%esi
> +        /* Calculate the load base address. */
> +        call    1f
> +1:      pop     %esi
> +        sub     $sym_offs(1b), %esi

I have a problem with this. I know that this should be done in that way. Even
I considered that during my work on Multiboot2 stuff. However Multiboot and
Multiboot2 specs clearly say: The OS image must create its own stack as soon
as it needs one. So, we cannot assume here that %ss:%esp points to valid stack
and do such things. Even if it works with this and that right now. So, I would
prefer to fix it in more reliable way right now even if it means more code here
and there.

Daniel

_______________________________________________
Xen-devel mailing list
Xen-devel@xxxxxxxxxxxxx
https://lists.xen.org/xen-devel