Re: [Xen-devel] Re: xen: memory initialization/balloon fixes (#3)

[Konrad Rzeszutek Wilk <konrad.wilk@xxxxxxxxxx>]

On Wed, Sep 28, 2011 at 11:45:02AM +0100, David Vrabel wrote:
> On 28/09/11 00:10, Konrad Rzeszutek Wilk wrote:
> >>> (XEN) Xen-e820 RAM map:
> >>> (XEN)  0000000000000000 - 000000000009d800 (usable)
> >>
> >> It's because it's not correctly handling the half-page of RAM at the end
> >> of this region.
> >>
> >> I don't have access to any test boxes with a dodgy BIOS like this so can
> >> you test this patch?  If it works I'll fold it in and post an updated
> >> series.
> > 
> > It works. Albeit I think we are going to hit a problem with dmidecode
> > if the DMI data is right in the reserved region
> > 
> > (http://lists.xensource.com/archives/html/xen-devel/2011-09/msg01299.html)
> > 
> > As in, if it starts in 9D800 - we consider 0->9d as RAM PFN, and 9e->100 as 
> > 1-1
> > mapping.
> > 
> > I am thinking that perhaps the call to xen_set_phys_identity, where
> > we call PFN_UP(x) should be replaced with PFN_DOWN(x). That way
> > we would consider 0>9c as RAM PFN and 9D->100 as 1-1 mapping.
> 
> I almost did an equivalent change (see below) but discarded it as it
> would have resulting in overlapping regions and attempting to
> release/map some pages twice.
> 
> I think we will have to move the release/map until after the final e820
> map has been sanitized so there are no overlapping regions.

<nods>Fortunatly for us, the overlap does not happen - they are just
next to each other. BTW, I think Xen hypervisor does the E820 sanitisation
so there shouldn't be any funny entries.

> 
> I'll prepare another patch for this.

OK.
> 
> > That would imply a new patch to your series naturally.
> >>
> >> Can you remember why this page alignment was required?  I'd like to
> > 
> > The e820_* calls define how the memory subsystem will use it.
> > It ended at some point assuming that the full page is RAM even thought
> > it was only half-RAM and tried to use it and blew the machine up.
> > 
> > The fix was to make the calls to the e820_* with size and regions
> > that were page-aligned.
> > 
> > Anyhow, here is what the bootup looks now:
> > 
> > [    0.000000] Freeing  9e-a0 pfn range: 2 pages freed
> > [    0.000000] 1-1 mapping on 9e->a0
> > [    0.000000] Freeing  a0-100 pfn range: 96 pages freed
> > [    0.000000] 1-1 mapping on a0->100
> > [    0.000000] Freeing  7fff0-80000 pfn range: 16 pages freed
> > [    0.000000] 1-1 mapping on 7fff0->80000
> > [    0.000000] Freeing  cfef0-cfef5 pfn range: 5 pages freed
> > [    0.000000] 1-1 mapping on cfef0->cfef5
> > [    0.000000] Freeing  cfef5-cff7f pfn range: 138 pages freed
> > [    0.000000] 1-1 mapping on cfef5->cff7f
> > [    0.000000] Freeing  cff7f-d0000 pfn range: 129 pages freed
> > [    0.000000] 1-1 mapping on cff7f->d0000
> > [    0.000000] Freeing  d0000-f0000 pfn range: 131072 pages freed
> > [    0.000000] 1-1 mapping on d0000->f0000
> > [    0.000000] Freeing  f0000-f4b58 pfn range: 19288 pages freed
> > [    0.000000] 1-1 mapping on f0000->fec10
> > [    0.000000] 1-1 mapping on fec10->fee01
> > [    0.000000] 1-1 mapping on fee01->100000
> > [    0.000000] Released 150746 pages of unused memory
> > [    0.000000] Set 196994 page(s) to 1-1 mapping
> > [    0.000000] BIOS-provided physical RAM map:
> > [    0.000000]  Xen: 0000000000000000 - 000000000009d000 (usable)
> > [    0.000000]  Xen: 000000000009d800 - 0000000000100000 (reserved)
> > [    0.000000]  Xen: 0000000000100000 - 000000007fff0000 (usable)
> > [    0.000000]  Xen: 000000007fff0000 - 0000000080000000 (reserved)
> > 
> > 
> >> update the comment with the reason because the bare-metal x86 memory
> >> init code doesn't appear to fixup the memory map in this way.
> >>
> >> diff --git a/arch/x86/xen/setup.c b/arch/x86/xen/setup.c
> >> index 986661b..e473c4c 100644
> >> --- a/arch/x86/xen/setup.c
> >> +++ b/arch/x86/xen/setup.c
> >> @@ -178,6 +178,19 @@ static unsigned long __init xen_get_max_pages(void)
> >>    return min(max_pages, MAX_DOMAIN_PAGES);
> >>  }
> >>
> >> +static void xen_e820_add_region(u64 start, u64 size, int type)

Might as well call this function "xen_align_and_add_e820_region"

> >> +{
> >> +  u64 end = start + size;
> >> +
> >> +  /* Align RAM regions to page boundaries. */
> >> +  if (type == E820_RAM || type == E820_UNUSABLE) {
> > 
> > Hm, do we care about E820_UNUSABLE to be page aligned?
> > If so, please comment why.
> 
> Er. We don't really but I think this if needs to be:
> 
>     /*
>      * Page align regions.
>      *
>      * Reduce RAM regions and expand other (reserved) regions.
>      */
>     if (type == E820_RAM || type == E820_UNUSABLE) {
>       start = PAGE_ALIGN(start);
>         end  &= ~((u64)PAGE_SIZE - 1);
>     } else {
>         start &= ~((u64)PAGE_SIZE - 1);
>         end = PAGE_ALIGN(start);
>     }
> 
> So reserved regions also become page aligned (which is part of the fix
> for the dmidecode bug).

<nods> That should be part of a seperate patch (well, the dmidecde
patch). Instead of the "infinite loop, won't boot on Konrad's
machines with non-standard E820".


> 
> >> +          start = PAGE_ALIGN(start);
> > 
> > Is that actually safe? Say it starts a 9ffff? We would
> > end up using 9f000 which is not right.
> 
> PAGE_ALIGN() (and ALIGN()) round upwards.

<smacks his head> Right.

_______________________________________________
Xen-devel mailing list
Xen-devel@xxxxxxxxxxxxxxxxxxx
http://lists.xensource.com/xen-devel