From: Daniel Kiper <daniel.kiper@oracle.com>
To: Jan Beulich <JBeulich@suse.com>
Cc: Juergen Gross <JGross@suse.com>,
sstabellini@kernel.org, andrew.cooper3@citrix.com,
cardoe@cardoe.com, pgnet.dev@gmail.com, ning.sun@intel.com,
david.vrabel@citrix.com, xen-devel@lists.xenproject.org,
qiaowei.ren@intel.com, gang.wei@intel.com, fu.wei@linaro.org
Subject: Re: [PATCH v5 15/16] x86: make Xen early boot code relocatable
Date: Wed, 31 Aug 2016 22:50:44 +0200 [thread overview]
Message-ID: <20160831205044.GJ5364@olila.local.net-space.pl> (raw)
In-Reply-To: <57C717F7020000780010A91A@prv-mh.provo.novell.com>
On Wed, Aug 31, 2016 at 09:46:31AM -0600, Jan Beulich wrote:
> >>> On 31.08.16 at 16:59, <daniel.kiper@oracle.com> wrote:
> > On Thu, Aug 25, 2016 at 08:41:39AM -0600, Jan Beulich wrote:
> >> >>> On 20.08.16 at 00:43, <daniel.kiper@oracle.com> 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@lists.xen.org
https://lists.xen.org/xen-devel
next prev parent reply other threads:[~2016-08-31 20:51 UTC|newest]
Thread overview: 70+ messages / expand[flat|nested] mbox.gz Atom feed top
2016-08-19 22:43 [PATCH v5 00/16] x86: multiboot2 protocol support Daniel Kiper
2016-08-19 22:43 ` [PATCH v5 01/16] x86: allow EFI reboot method neither on EFI platforms Daniel Kiper
2016-08-25 11:19 ` Jan Beulich
2016-08-19 22:43 ` [PATCH v5 02/16] x86/boot: remove multiboot1_header_end from symbol table Daniel Kiper
2016-08-25 11:21 ` Jan Beulich
2016-08-30 14:27 ` Daniel Kiper
2016-08-30 15:11 ` Jan Beulich
2016-08-19 22:43 ` [PATCH v5 03/16] x86/boot: create *.lnk files with linker script Daniel Kiper
2016-08-25 11:28 ` Jan Beulich
2016-08-19 22:43 ` [PATCH v5 04/16] x86/boot/reloc: reduce assembly usage as much as possible Daniel Kiper
2016-08-25 11:29 ` Jan Beulich
2016-08-19 22:43 ` [PATCH v5 05/16] x86/boot: call reloc() using stdcall calling convention Daniel Kiper
2016-08-19 22:43 ` [PATCH v5 06/16] x86/boot/reloc: create generic alloc and copy functions Daniel Kiper
2016-08-25 11:34 ` Jan Beulich
2016-08-30 14:32 ` Daniel Kiper
2016-08-30 15:12 ` Jan Beulich
2016-08-31 15:13 ` Daniel Kiper
2016-08-31 15:25 ` Jan Beulich
2016-08-31 19:39 ` Daniel Kiper
2016-09-01 7:35 ` Jan Beulich
2016-09-06 15:33 ` Doug Goldstein
2016-08-19 22:43 ` [PATCH v5 07/16] x86/boot: use %ecx instead of %eax Daniel Kiper
2016-08-19 22:43 ` [PATCH v5 08/16] x86/boot/reloc: rename some variables and rearrange code a bit Daniel Kiper
2016-08-19 22:43 ` [PATCH v5 09/16] x86: add multiboot2 protocol support Daniel Kiper
2016-08-25 11:50 ` Jan Beulich
2016-08-30 14:41 ` Daniel Kiper
2016-08-30 15:14 ` Jan Beulich
2016-08-31 15:21 ` Daniel Kiper
2016-08-31 20:18 ` Andrew Cooper
2016-08-31 21:01 ` Daniel Kiper
2016-08-19 22:43 ` [PATCH v5 10/16] efi: create efi_enabled() Daniel Kiper
2016-08-25 12:16 ` Jan Beulich
2016-08-30 17:19 ` Daniel Kiper
2016-08-31 12:31 ` Jan Beulich
2016-08-19 22:43 ` [PATCH v5 11/16] efi: build xen.gz with EFI code Daniel Kiper
2016-08-25 12:23 ` Jan Beulich
2016-08-19 22:43 ` [PATCH v5 12/16] x86/efi: create new early memory allocator Daniel Kiper
2016-09-05 12:33 ` Jan Beulich
2016-09-07 12:05 ` Daniel Kiper
2016-09-07 14:01 ` Jan Beulich
2016-09-08 8:29 ` Daniel Kiper
2016-09-08 9:59 ` Jan Beulich
2016-08-19 22:43 ` [PATCH v5 13/16] x86: add multiboot2 protocol support for EFI platforms Daniel Kiper
2016-08-25 12:59 ` Jan Beulich
2016-08-30 19:32 ` Daniel Kiper
2016-08-31 12:49 ` Jan Beulich
2016-08-31 17:07 ` Daniel Kiper
2016-09-01 7:40 ` Jan Beulich
2016-09-01 20:37 ` Daniel Kiper
2016-08-19 22:43 ` [PATCH v5 14/16] x86/boot: implement early command line parser in C Daniel Kiper
2016-08-25 13:27 ` Jan Beulich
2016-08-30 19:58 ` Daniel Kiper
2016-08-31 13:01 ` Jan Beulich
2016-08-31 19:31 ` Daniel Kiper
2016-09-01 7:41 ` Jan Beulich
2016-09-01 20:43 ` Daniel Kiper
2016-08-19 22:43 ` [PATCH v5 15/16] x86: make Xen early boot code relocatable Daniel Kiper
2016-08-25 14:41 ` Jan Beulich
2016-08-31 14:59 ` Daniel Kiper
2016-08-31 15:46 ` Jan Beulich
2016-08-31 20:50 ` Daniel Kiper [this message]
2016-09-01 7:46 ` Jan Beulich
2016-09-01 21:19 ` Daniel Kiper
2016-09-02 6:58 ` Jan Beulich
2016-09-02 7:28 ` Daniel Kiper
2016-08-19 22:43 ` [PATCH v5 16/16] x86: add multiboot2 protocol support for relocatable images Daniel Kiper
2016-08-22 10:10 ` [PATCH v5 00/16] x86: multiboot2 protocol support Jan Beulich
2016-08-30 14:15 ` Daniel Kiper
2016-08-30 15:09 ` Jan Beulich
2016-08-31 15:05 ` Daniel Kiper
Reply instructions:
You may reply publicly to this message via plain-text email
using any one of the following methods:
* Save the following mbox file, import it into your mail client,
and reply-to-all from there: mbox
Avoid top-posting and favor interleaved quoting:
https://en.wikipedia.org/wiki/Posting_style#Interleaved_style
* Reply using the --to, --cc, and --in-reply-to
switches of git-send-email(1):
git send-email \
--in-reply-to=20160831205044.GJ5364@olila.local.net-space.pl \
--to=daniel.kiper@oracle.com \
--cc=JBeulich@suse.com \
--cc=JGross@suse.com \
--cc=andrew.cooper3@citrix.com \
--cc=cardoe@cardoe.com \
--cc=david.vrabel@citrix.com \
--cc=fu.wei@linaro.org \
--cc=gang.wei@intel.com \
--cc=ning.sun@intel.com \
--cc=pgnet.dev@gmail.com \
--cc=qiaowei.ren@intel.com \
--cc=sstabellini@kernel.org \
--cc=xen-devel@lists.xenproject.org \
/path/to/YOUR_REPLY
https://kernel.org/pub/software/scm/git/docs/git-send-email.html
* If your mail client supports setting the In-Reply-To header
via mailto: links, try the mailto: link
Be sure your reply has a Subject: header at the top and a blank line
before the message body.
This is an external index of several public inboxes,
see mirroring instructions on how to clone and mirror
all data and code used by this external index.