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

[Daniel Kiper <daniel.kiper@xxxxxxxxxx>]

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!

[...]

> >> > --- a/xen/arch/x86/boot/trampoline.S
> >> > +++ b/xen/arch/x86/boot/trampoline.S
> >> > @@ -54,12 +54,20 @@ trampoline_gdt:
> >> >          /* 0x0028: real-mode data @ BOOT_TRAMPOLINE */
> >> >          .long   0x0000ffff
> >> >          .long   0x00009200
> >> > +        /*
> >> > +         * 0x0030: ring 0 Xen data, 16 MiB size, base
> >> > +         * address is computed during runtime.
> >> > +         */
> >> > +        .quad   0x00c0920000001000
> >>
> >> This does not look like it covers 1Mb, it's more like 1Mb+4k-1.
> >
> > I have checked it once again. It covers 16 MiB as required:
> >   4 KiB * 0x1000 = 0x1000000 (no taking into account relocation).
>
> I'm sorry, but no. The raw limit value taken from that descriptor
> is 0x1000, and the G bit is set. That makes the actual limit 0x1000fff.

I was not sure why but this explains it: "When the granularity flag is
set, the twelve least significant bits of an offset are not tested when
checking the offset against the segment limit. For example, when the
granularity flag is set, a limit of 0 results in valid offsets from 0 to 4095."
(Intel(R) 64 and IA-32 Architectures Software Developer’s Manual Volume 3
(3A, 3B & 3C): System Programming Guide). So, it means that this should be:

  .quad 0x00c0920000000fff

[...]

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

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

(XEN) Detected 2194.733 MHz processor.
(XEN) Assertion 'l2e_get_flags(l2e) & _PAGE_PRESENT' failed at x86_64/mm.c:902
(XEN) ----[ Xen-4.8-unstable  x86_64  debug=y  Tainted:  C   ]----
(XEN) CPU:    0
(XEN) RIP:    e008:[<ffff82d0803c55d4>] subarch_init_memory+0x53e/0x5ea
(XEN) RFLAGS: 0000000000010046   CONTEXT: hypervisor
(XEN) rax: ffff830000200000   rbx: ffffffffffffffff   rcx: 0000000000000000
(XEN) rdx: 0000000000000000   rsi: 000000007ea04000   rdi: 8000000000000000
(XEN) rbp: ffff82d080417d78   rsp: ffff82d080417d38   r8:  ffff830000000000
(XEN) r9:  00000007c7ffffff   r10: ffffffffffffffff   r11: 0000000000000000
(XEN) r12: ffff83007ea04008   r13: 0000000000000000   r14: 0000000000000000
(XEN) r15: 0000000000000000   cr0: 000000008005003b   cr4: 00000000000006e0
(XEN) cr3: 000000007ea0c000   cr2: 0000000000000000
(XEN) ds: 0000   es: 0000   fs: 0000   gs: 0000   ss: 0000   cs: e008
(XEN) Xen code around <ffff82d0803c55d4> (subarch_init_memory+0x53e/0x5ea):
(XEN)  09 fe 41 f6 c6 01 75 02 <0f> 0b 41 f6 c6 80 74 0f 48 09 fa 49 89 14 24 48
(XEN) Xen stack trace from rsp=ffff82d080417d38:
(XEN)    ffff82d0802324c3 000000000007ea04 ffff82d080417d78 0000000000180000
(XEN)    0000000000000009 0000000000180000 ffff82e000000000 0000000000180000
(XEN)    ffff82d080417db8 ffff82d0803c42af ffff82d080417da8 ffff82d080417fff
(XEN)    ffff83017f1e0670 ffff82d08054cad0 ffff82d08054cad0 0000000000000003
(XEN)    ffff82d080417f08 ffff82d0803ca8d5 0000000000000000 0000000000000001
(XEN)    0000024d00000000 000000000034cc00 000000000000014d 00000000000001f5
(XEN)    00000000000001f5 000000000000019f 000000000000019f 000000000000014e
(XEN)    000000000000014e 0000000000000002 0000000000000001 0000000000000001
(XEN)    0000000000000001 0000000000000001 0000000000000001 0000000000000001
(XEN)    0000000000b42000 ffff83000008eee0 0000000000000000 000000000000000e
(XEN)    0000000000180000 ffff83000008efa0 000000017f1f2000 fffffffffffff001
(XEN)    ffff83000008efb0 ffff82d0803ef99c 0000000000000000 0000000000000000
(XEN)    0000000800000000 000000010000006e 0000000000000003 00000000000002f8
(XEN)    0000000000000000 0000000000000000 0000000000000000 0000000000000000
(XEN)    0000000000000000 0000000000000000 0000000000000000 ffff82d0802000f3
(XEN)    0000000000000000 0000000000000000 0000000000000000 0000000000000000
(XEN)    0000000000000000 0000000000000000 0000000000000000 0000000000000000
(XEN)    0000000000000000 0000000000000000 0000000000000000 0000000000000000
(XEN)    0000000000000000 0000000000000000 0000000000000000 0000000000000000
(XEN)    0000000000000000 0000000000000000 0000000000000000 0000000000000000
(XEN) Xen call trace:
(XEN)    [<ffff82d0803c55d4>] subarch_init_memory+0x53e/0x5ea
(XEN)    [<ffff82d0803c42af>] arch_init_memory+0x2f3/0x5d3
(XEN)    [<ffff82d0803ca8d5>] __start_xen+0x242f/0x2965
(XEN)    [<ffff82d0802000f3>] __high_start+0x53/0x60
(XEN)
(XEN)
(XEN) ****************************************
(XEN) Panic on CPU 0:
(XEN) Assertion 'l2e_get_flags(l2e) & _PAGE_PRESENT' failed at x86_64/mm.c:902
(XEN) ****************************************

Is it sufficient? If not please drop me a line what else do you need.

Daniel

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