Re: [Xen-devel] [PATCH v5 15/16] x86: make Xen early boot code relocatable

[Daniel Kiper <daniel.kiper@xxxxxxxxxx>]

On Thu, Sep 01, 2016 at 01:46:24AM -0600, Jan Beulich wrote:
> >>> On 31.08.16 at 22:50, <daniel.kiper@xxxxxxxxxx> wrote:
> > On Wed, Aug 31, 2016 at 09:46:31AM -0600, Jan Beulich wrote:
> >> >>> On 31.08.16 at 16:59, <daniel.kiper@xxxxxxxxxx> wrote:
> >> > On Thu, Aug 25, 2016 at 08:41:39AM -0600, Jan Beulich wrote:
> >> >> >>> On 20.08.16 at 00:43, <daniel.kiper@xxxxxxxxxx> wrote:
> >> >> > --- a/xen/arch/x86/boot/head.S
> >> >> > +++ b/xen/arch/x86/boot/head.S
> >> >> > @@ -12,13 +12,16 @@
> >> >> >          .text
> >> >> >          .code32
> >> >> >
> >> >> > -#define sym_phys(sym)     ((sym) - __XEN_VIRT_START)
> >> >> > +#define sym_offset(sym)   ((sym) - __XEN_VIRT_START)
> >> >>
> >> >> In a patch already large and hence hard to review, did you really
> >> >> need to do this rename?
> >> >
> >> > I think that sym_offset() is better term here. However, if you wish
> >> > I can leave sym_phys() as is. Though, IMO, sym_phys() name suggests
> >> > that we are playing with physical addresses and it is not correct in
> >> > all contexts, e.g. loot at fs_offset() (or sym_fs() as you wish).
> >>
> >> After some more thinking about this I agree sym_phys() isn't
> >> ideal anymore after this patch. Still, to avoid unrelated changes in
> >> this quite hard to review patch, I'd like to ask that you keep the
> >> name here and do just the rename in a subsequent patch.
> >
> > Granted!
>
> Btw., to save on typing and since you will need to touch this anyway
> - can I talk you into using sym_offs() instead of sym_offset()?

Sure thing!

> >> >> >          .pushsection .trampoline_rel, "a"
> >> >> >          .long   trampoline_gdt + BOOT_PSEUDORM_CS + 2 - .
> >> >> >          .long   trampoline_gdt + BOOT_PSEUDORM_DS + 2 - .
> >> >> >          .popsection
> >> >> >
> >> >> > +GLOBAL(xen_img_load_base_addr)
> >> >> > +        .long   0
> >> >>
> >> >> I've yet to understand the purpose of this symbol, but in any case:
> >> >> If the trampoline code doesn't reference it, why does it get placed
> >> >> here?
> >> >
> >> > This symbol was used by code which unconditionally relocated Xen image
> >> > even if boot loader did work for us. I removed most of this code because
> >> > we agreed that if boot loader relocated Xen image then we do not have to
> >> > relocate it higher once again. If you are still OK with this idea I can 
> >> > go
> >> > further, as I planned, and remove all such remnants from next release.
> >> > However, it looks that then I have to store load address in 
> >> > xen_phys_start
> >> > from 32-bit assembly code. So, it will be nice if I can define it as
> >> > "unsigned int" instead of "unsigned long". Is it safe?
> >>
> >> I don't see why you shouldn't be able to store into the low 32 bits of
> >> the variable without altering the high ones (which are all zero).
> >
> > I was thinking about that too but IMO it is not nice. However, if you
> > are OK with that I can do it.
>
> Well, imo that's something which is perfectly fine in assembly code.

OK, I will do that.

> >> >> >          s = (boot_e820.map[i].addr + mask) & ~mask;
> >> >> >          e = (boot_e820.map[i].addr + boot_e820.map[i].size) & ~mask;
> >> >> > -        s = max_t(uint64_t, s, BOOTSTRAP_MAP_BASE);
> >> >> >          if ( (boot_e820.map[i].type != E820_RAM) || (s >= e) )
> >> >> >              continue;
> >> >>
> >> >> This means you now map memory between 2Mb and 16Mb here. Is
> >> >> this necessary?
> >> >
> >> > Without this change Xen relocated by boot loader crashes with:
> >> >
> >> > (XEN) Panic on CPU 0:
> >> > (XEN) Assertion 'l2e_get_flags(l2e) & _PAGE_PRESENT' failed at 
> >> > x86_64/mm.c:902
> >> >
> >> > So, it must be mapped.
> >>
> >> That's too little context to be useful for understanding the full
> >> background.
> >
> > Here it is:
> >[...]
> > Is it sufficient? If not please drop me a line what else do you need.
>
> I'm sorry, but no. I didn't mean the whole register and stack dump. I
> meant an explanation of _why_ this assertion triggers (after all it
> doesn't trigger without your patches, and hence I can't just go and
> check the relevant source files).

AIUI, if Xen image is not relocated by boot loader (e.g. GRUB2) then region
2 MiB - 16 MiB is mapped. Later Xen after relocating itself marks 1 MiB - 16 MiB
region as NX (if it is supported by hardware) in subarch_init_memory(). However,
if boot loader relocate Xen then region 2 MiB - 16 MiB is not mapped if change
under consideration is not applied. Then ASSERT(l2e_get_flags(l2e) & 
_PAGE_PRESENT)
in subarch_init_memory() is triggered and Xen crashes.

I hope that helps.

Daniel

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