Re: [Xen-devel] dom/xen heap and boot allocator (WAS Re: [xen-unstable-smoke test] 141333: regressions - FAIL)

[Julien Grall <julien.grall@xxxxxxx>]

Hi Juergen,

On 9/17/19 2:12 PM, Juergen Gross wrote:
> On 17.09.19 14:31, Julien Grall wrote:
> > (+ Juergen)
> >
> > Hi,
> >
> > On 9/16/19 9:51 AM, Jan Beulich wrote:
> > > On 15.09.2019 19:51, Julien Grall wrote:
> > > > Hi,
> > > >
> > > > On 9/15/19 3:09 PM, osstest service owner wrote:
> > > > > flight 141333 xen-unstable-smoke real [real]
> > > > > http://logs.test-lab.xenproject.org/osstest/logs/141333/
> > > > >
> > > > > Regressions :-(
> > > > >
> > > > > Tests which did not succeed and are blocking,
> > > > > including tests which could not be run:
> > > > >    test-armhf-armhf-xl           7 xen-boot                 fail 
> > > > > REGR. vs. 141253
> > > > Osstest does not provide the stack trace as the crash happen before the
> > > > console is setup, but I managed to reproduce it locally:
> > > >
> > > > (XEN) Assertion 'is_xen_heap_mfn(maddr_to_mfn(ma))' failed at
> > > > /home/julieng/works/xen/xen/include/asm/mm.h:250
> > > > (XEN) ----[ Xen-4.13-unstable  arm32  debug=y   Not tainted ]----
> > > >
> > > > [...]
> > > >
> > > > (XEN) Xen call trace:
> > > > (XEN)    [<002992c0>] page_alloc.c#bootmem_region_add+0xf8/0x17c (PC)
> > > > (XEN)    [<002995ac>] init_boot_pages+0x8c/0x1a0 (LR)
> > > > (XEN)    [<002995ac>] init_boot_pages+0x8c/0x1a0
> > > > (XEN)    [<002acc2c>] dt_unreserved_regions+0x268/0x284
> > > > (XEN)    [<002ad8e0>] start_xen+0x5c8/0xe90
> > > > (XEN)    [<00200098>] arm32/head.o#primary_switched+0x4/0x10
> > > >
> > > > This is happening because of commit 6e3e771203 "xen/arm: setup: 
> > > > Relocate
> > > > the Device-Tree later on in the boot". Since this patch, none of 
> > > > xenheap
> > > > memory is given to the boot allocator.
> > > So this change wasn't tested on 32-bit Arm at all then before
> > > committing?
> > I forgot to test it before sending the patch on the ML :(.
> >
> > > > The boot allocator is bootstrapping itself and re-use a page from the
> > > > first added region. If this region is not a xenheap region, then it 
> > > > will
> > > > crash when calling mfn_to_virt() or later on because the virtual 
> > > > address
> > > > is not mapped in memory.
> > > >
> > > > The description of the boot allocator in page_alloc.c leads to think
> > > > that only domheap memory may be given to the boot allocator.
> > > >
> > > > Furthermore, as the boot allocator may have domheap page, it means that
> > > > calling mfn_to_virt(mfn_x(alloc_boot_pages(...)) may not work when
> > > > CONFIG_SEPARATE_XENHEAP=y.
> > > >
> > > > It feels to me that imposing to give a xenheap page to the boot
> > > > allocator is quite ugly. As the boot allocator will be used in most of
> > > > the case, statically allocating bootmem_region_list maybe the best. Any
> > > > thoughts?
> > > I've gone back to 4.2 code, where 32-bit x86 was still supported. There
> > > we had
> > >
> > > #if defined(CONFIG_X86_32)
> > >      xenheap_initial_phys_start = (PFN_UP(__pa(&_end)) + 1) << 
> > > PAGE_SHIFT;
> > >      /* Must pass a single mapped page for populating 
> > > bootmem_region_list. */
> > >      init_boot_pages(__pa(&_end), xenheap_initial_phys_start);
> > >      xenheap_phys_end = DIRECTMAP_MBYTES << 20;
> > > #else
> > I know realize we had similar comment in the arm32 code:
> >
> > "Need a single mapped page for populating bootmem_region_list"
> >
> > > i.e. it was clearly intended for the first thing given to the boot
> > > allocator to be a Xen heap page.
> > >
> > > Beyond this no x86 code that wasn't x86-64 specific depended on there
> > > only coming direct-mapped memory back out of alloc_boot_pages().
> > >
> > > But yes, a static allocation may make sense (and then arguably it may
> > > not even need to be a full page) - commit 0409e29e2b, which is what
> > > had introduced bootmem_region_list, doesn't (as was the common case
> > > back then) have any description at all, i.e. there's no way to know
> > > why it wasn't done this way in the first place.
> > It is probably too late for Xen 4.13 to move to a static list for 
> > bootmem_region_list (Juergen?), so I am thinking to fix up the arm32
> > code instead.
> Depends on the patch size and the risk to take it. But in general I'd
> prefer a simple solution, especially in order to get a push rather
> sooner than later.
I have just sent a patch to fix the breakage:

https://patchwork.kernel.org/patch/11148883/

Cheers,

--
Julien Grall

_______________________________________________
Xen-devel mailing list
Xen-devel@xxxxxxxxxxxxxxxxxxxx
https://lists.xenproject.org/mailman/listinfo/xen-devel