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

[Daniel Kiper <daniel.kiper@xxxxxxxxxx>]

On Fri, Sep 02, 2016 at 12:58:14AM -0600, Jan Beulich wrote:
> >>> On 01.09.16 at 23:19, <daniel.kiper@xxxxxxxxxx> wrote:
> > 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:
> >> >> >> >          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.
>
> Ah, I see now. But instead of altering the setup.c logic even for the
> not relocated case, perhaps you'd better establish the here expected
> mapping in the relocated case? I'm rather hesitant to see that setup.c
> logic change for pre-existing cases; it may alternatively be acceptable
> to make the line you delete conditional.

...or map 2 MiB - 16 MiB region in xen/arch/x86/boot/head.S. It could
be done in similar way to Xen image mapping. However, this should be
tested...

Daniel

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