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

["Jan Beulich" <JBeulich@xxxxxxxx>]

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

Jan


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