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

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

[Andrew Cooper]

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

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@citrix.com>
Tested-by: Sergey Dyasli <sergey.dyasli@citrix.com>
---
CC: Jan Beulich <JBeulich@suse.com>
CC: Julien Grall <julien.grall@arm.com>
CC: Daniel Kiper <daniel.kiper@oracle.com>
CC: Doug Goldstein <cardoe@cardoe.com>
CC: Sergey Dyasli <sergey.dyasli@citrix.com>

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
 
         /* Bootloaders may set multiboot{1,2}.mem_lower to a nonzero value. */
         xor     %edx,%edx
@@ -410,15 +412,6 @@ __start:
         cmp     %edi,MB2_fixed_total_size(%ebx)
         jbe     trampoline_bios_setup
 
-        /* Get Xen image load base address from Multiboot2 information. */
-        cmpl    $MULTIBOOT2_TAG_TYPE_LOAD_BASE_ADDR,MB2_tag_type(%ecx)
-        jne     .Lmb2_mem_lower
-
-        mov     MB2_load_base_addr(%ecx),%esi
-        sub     $XEN_IMG_OFFSET,%esi
-        jmp     .Lmb2_next_tag
-
-.Lmb2_mem_lower:
         /* Get mem_lower from Multiboot2 information. */
         cmpl    $MULTIBOOT2_TAG_TYPE_BASIC_MEMINFO,MB2_tag_type(%ecx)
         cmove   MB2_mem_lower(%ecx),%edx
-- 
2.1.4


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

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

[Daniel Kiper]

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@citrix.com>
> Tested-by: Sergey Dyasli <sergey.dyasli@citrix.com>
> ---
> CC: Jan Beulich <JBeulich@suse.com>
> CC: Julien Grall <julien.grall@arm.com>
> CC: Daniel Kiper <daniel.kiper@oracle.com>
> CC: Doug Goldstein <cardoe@cardoe.com>
> CC: Sergey Dyasli <sergey.dyasli@citrix.com>
>
> 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@lists.xen.org
https://lists.xen.org/xen-devel

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

[Andrew Cooper]

On 03/06/2017 11:22, Daniel Kiper wrote:
> 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.

In this case, MB2.  Tboot should indeed be fixed, but that doesn't
invalidate this fix.

>
>> 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@citrix.com>
>> Tested-by: Sergey Dyasli <sergey.dyasli@citrix.com>
>> ---
>> CC: Jan Beulich <JBeulich@suse.com>
>> CC: Julien Grall <julien.grall@arm.com>
>> CC: Daniel Kiper <daniel.kiper@oracle.com>
>> CC: Doug Goldstein <cardoe@cardoe.com>
>> CC: Sergey Dyasli <sergey.dyasli@citrix.com>
>>
>> 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.

There is no possible way that %ss is bad.  It must be a writeable
segment (to be loaded in the first place) with 0 base and 4G limit (to
fulfil the flat requirement).

I see MB1 says that %esp is undefined.  Where does the MB2 spec live
these days (google doesn't seem to know)?

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

Does the EFI entry guarantee a good %rsp?  That codepath doesn't
sanitise %rsp before use.

~Andrew

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

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

[Daniel Kiper]

[-- Attachment #1: Type: text/plain, Size: 3619 bytes --]

On Sat, Jun 03, 2017 at 01:04:17PM +0100, Andrew Cooper wrote:
> On 03/06/2017 11:22, Daniel Kiper wrote:
> > 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.
>
> In this case, MB2.  Tboot should indeed be fixed, but that doesn't
> invalidate this fix.

Sure thing.

> >> 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@citrix.com>
> >> Tested-by: Sergey Dyasli <sergey.dyasli@citrix.com>
> >> ---
> >> CC: Jan Beulich <JBeulich@suse.com>
> >> CC: Julien Grall <julien.grall@arm.com>
> >> CC: Daniel Kiper <daniel.kiper@oracle.com>
> >> CC: Doug Goldstein <cardoe@cardoe.com>
> >> CC: Sergey Dyasli <sergey.dyasli@citrix.com>
> >>
> >> 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.
>
> There is no possible way that %ss is bad.  It must be a writeable
> segment (to be loaded in the first place) with 0 base and 4G limit (to
> fulfil the flat requirement).

Ahhh... Right, I forgot that it is initialized like %ds and others.

> I see MB1 says that %esp is undefined.  Where does the MB2 spec live
> these days (google doesn't seem to know)?

You can find it in multiboot2 branch in GRUB2 git tree. Though I am sending PDF
with latest spec (I should put it somewhere) for your convenience.

> > 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.
>
> Does the EFI entry guarantee a good %rsp?  That codepath doesn't
> sanitise %rsp before use.

Yep, Multiboot2 spec says about that.

Daniel

[-- Attachment #2: multiboot2_ver_2.0.pdf --]
[-- Type: application/pdf, Size: 230531 bytes --]

[-- Attachment #3: Type: text/plain, Size: 127 bytes --]

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