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

[Daniel Kiper <daniel.kiper@xxxxxxxxxx>]

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@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.
>
> 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: multiboot2_ver_2.0.pdf
Description: Adobe PDF document

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