Date:	Thu, 18 Oct 2012 11:26:11 -0500
From:	Jacob Shin <jacob.shin@....com>
To:	Stefano Stabellini <stefano.stabellini@...citrix.com>
CC:	Yinghai Lu <yinghai@...nel.org>,
	Thomas Gleixner <tglx@...utronix.de>,
	Ingo Molnar <mingo@...e.hu>, "H. Peter Anvin" <hpa@...or.com>,
	Tejun Heo <tj@...nel.org>,
	"linux-kernel@...r.kernel.org" <linux-kernel@...r.kernel.org>
Subject: Re: [PATCH -v3 0/7] x86: Use BRK to pre mapping page table to make
 xen happy

On Thu, Oct 18, 2012 at 05:17:28PM +0100, Stefano Stabellini wrote:
> On Thu, 11 Oct 2012, Yinghai Lu wrote:
> > On Wed, Oct 10, 2012 at 9:40 AM, Stefano Stabellini
> > <stefano.stabellini@...citrix.com> wrote:
> > >
> > > So you are missing the Xen patches entirely in this iteration of the
> > > series?
> > 
> > please check updated for-x86-mm branch.
> > 
> > [PATCH -v4 00/15] x86: Use BRK to pre mapping page table to make xen happy
> > 
> > on top of current linus/master and tip/x86/mm2, but please zap last
> > patch in that branch.
> > 
> > 1. use brk to mapping first PMD_SIZE range.
> > 2. top down to initialize page table range by range.
> > 3. get rid of calculate page table, and find_early_page_table.
> > 4. remove early_ioremap in page table accessing.
> > 
> > v2: changes, update xen interface about pagetable_reserve, so not
> >    use pgt_buf_* in xen code directly.
> > v3: use range top-down to initialize page table, so will not use
> >    calculating/find early table anymore.
> >    also reorder the patches sequence.
> > v4: add mapping_mark_page_ro to fix xen, also move pgt_buf_* to init.c
> >     and merge alloc_low_page()
> > 
> > could be found at:
> >         git://git.kernel.org/pub/scm/linux/kernel/git/yinghai/linux-yinghai.git
> > for-x86-mm
> 
> I find that patch series are easier to review than having to checkout
> your code and read the commit messages. Please post your patch series to
> the LKML next time.
> 
> In any case, regarding "x86, xen: Add xen_mapping_mark_page_ro": please
> take Peter's feedback into account; mark_page_ro is not a good name for
> a pvops.
> Also I don't believe that this call is actually needed, see below.
> 
> Regarding "x86, mm: setup page table in top-down": if you mark the
> pagetable page RO in alloc_low_page, won't the entire thing crash as
> soon as you try to write to it? You are supposed to mark it RO after
> filling up the pagetable page and before hooking it into the live
> pagetable.
> However contrary to my expectations, I did a quick test and it seems to
> be working, that is probably due to a bug: maybe __pa or lookup_address
> don't work correctly when called so early?

Hi Yinghai, I just tried it and dom0 died during init_memory_mapping, here
is the Oops snippet, full logs are attached:

e820: last_pfn = 0x22f000 max_arch_pfn = 0x400000000
e820: last_pfn = 0xcff00 max_arch_pfn = 0x400000000
initial memory mapped: [mem 0x00000000-0x022affff]
Base memory trampoline at [ffff880000096000] 96000 size 24576
init_memory_mapping: [mem 0x00000000-0x000fffff]
 [mem 0x00000000-0x000fffff] page 4k
init_memory_mapping: [mem 0x21fe00000-0x21fe77fff]
 [mem 0x21fe00000-0x21fe77fff] page 4k
init_memory_mapping: [mem 0x21c000000-0x21fdfffff]
 [mem 0x21c000000-0x21fdfffff] page 4k
init_memory_mapping: [mem 0x200000000-0x21bffffff]
 [mem 0x200000000-0x21bffffff] page 4k
init_memory_mapping: [mem 0x00100000-0xcec6dfff]
 [mem 0x00100000-0xcec6dfff] page 4k
init_memory_mapping: [mem 0xcf5f4000-0xcf5f4fff]
 [mem 0xcf5f4000-0xcf5f4fff] page 4k
init_memory_mapping: [mem 0xcf7fb000-0xcfc19fff]
 [mem 0xcf7fb000-0xcfc19fff] page 4k
init_memory_mapping: [mem 0xcfef4000-0xcfefffff]
 [mem 0xcfef4000-0xcfefffff] page 4k
init_memory_mapping: [mem 0x100001000-0x1ffffffff]
 [mem 0x100001000-0x1ffffffff] page 4k
PGD 0 
Oops: 0003 [#1] SMP 
Modules linked in:
CPU 0 
Pid: 0, comm: swapper Not tainted 3.6.0+ #3 AMD Pike/Pike
RIP: e030:[<ffffffff81cd778a>]  [<ffffffff81cd778a>] xen_set_pte_init+0x1/0x9
RSP: e02b:ffffffff81c01ae8  EFLAGS: 00010086
RAX: 80000001913d1063 RBX: ffff88021f63b1c8 RCX: 8000000000000163
RDX: 00000000000001ff RSI: 80000001913d1063 RDI: ffff88021f63b1c8
RBP: ffffffff81c01af8 R08: 0000000000000000 R09: 0000000000000000
R10: 0000000000000000 R11: 0000000000000000 R12: 000000011b039000
R13: 000000000000003a R14: 000000011b03a000 R15: 000000011b039000
FS:  0000000000000000(0000) GS:ffffffff81cbe000(0000) knlGS:0000000000000000
CS:  e033 DS: 0000 ES: 0000 CR0: 0000000080050033
CR2: 0000000000000000 CR3: 0000000001c0b000 CR4: 0000000000000660
DR0: 0000000000000000 DR1: 0000000000000000 DR2: 0000000000000000
DR3: 0000000000000000 DR6: 0000000000000000 DR7: 0000000000000000
Process swapper (pid: 0, threadinfo ffffffff81c00000, task ffffffff81c13410)
Stack:
 ffffffff81c01af8 ffffffff810330f5 ffffffff81c01b58 ffffffff816aa9f3
 8000000000000163 ffff88021f63c000 0000000200000000 000000011b039000
 ffffffff81c01b38 0000000000000000 000000011b000000 ffff88021f7146c0
Call Trace:
 [<ffffffff810330f5>] ? set_pte+0xb/0xd
 [<ffffffff816aa9f3>] phys_pte_init+0xd4/0x106
 [<ffffffff816aabe0>] phys_pmd_init+0x1bb/0x215
 [<ffffffff816aadf3>] phys_pud_init+0x1b9/0x218
 [<ffffffff816aaeff>] kernel_physical_mapping_init+0xad/0x14a
 [<ffffffff81682a1a>] init_memory_mapping+0x275/0x303
 [<ffffffff81ce6e62>] init_range_memory_mapping+0x8b/0xc8
 [<ffffffff81ce6f91>] init_mem_mapping+0xf2/0x162
 [<ffffffff81cd9d74>] setup_arch+0x682/0xaac
 [<ffffffff816af4ab>] ? printk+0x48/0x4a
 [<ffffffff81cd3868>] start_kernel+0x8e/0x3d8
 [<ffffffff81cd32d3>] x86_64_start_reservations+0xae/0xb2
 [<ffffffff81cd6dbc>] xen_start_kernel+0x63d/0x63f
Code: 00 00 48 c7 c7 f2 a8 aa 81 e8 ee 61 36 ff c7 05 59 10 06 00 01 00 00 00 5d c3 55 48 89 f7 48 89 e5 e8 95 cf 32 ff 31 c0 5d c3 55 <48> 89 37 48 89 e5 5d c3 55 48 89 e5 41 57 41 56 45 31 f6 41 55 
 RSP <ffffffff81c01ae8>
CR2: 0000000000000000
---[ end trace c2b54da46b5614cf ]---

> 
> In any case we don't care about that because if we assume that
> alloc_low_pages will always return a page from a range that is already
> mapped, then we do not need this pvop anymore. That's because
> xen_alloc_pte_init, xen_alloc_pmd_init, etc, will mark the page RO
> before hooking it into the pagetable automatically.
> Sorry for I misled you last time.
> 
> Let me repeat it again:
> Can we assume that the page returned by alloc_low_pages is already mapped?
> 
> Yes? In that case let's get rid of mark_page_ro and everything should
> work.
> 
> It is worth stating it in clear letters in a comment on top of
> alloc_low_pages:
> 
> "This function always returns a page from a memory range already
> mapped."
> 

View attachment "dom0.txt" of type "text/plain" (5360 bytes)

View attachment "xen.txt" of type "text/plain" (11995 bytes)

Powered by blists - more mailing lists