Re: [Xen-devel] dom0 pvops boot crash on very ordinary Dell R310

[Ian Campbell <Ian.Campbell@xxxxxxxxxxxxx>]

On Wed, 2010-10-27 at 18:20 +0100, Jeremy Fitzhardinge wrote:
> On 10/27/2010 09:04 AM, Ian Campbell wrote:
> > On Wed, 2010-10-27 at 14:18 +0100, Ian Campbell wrote:
> >>
> >> IOW we are faulting on precisely one of the PFNs which we earlier
> >> released. We released these because of a hole in the e820 between
> >> 0xa0000-0x100000 which dom0 apparently manufactured. 
> >>
> >> IIRC in a PV domU we reserve some of the legacy address space to stop
> >> old ISA drivers etc from getting at it. I suspect this is wrong for a
> >> dom0 where we want the e820 to be more or less unmolested. I'll track
> >> down the code in question and see if removing it for dom0 helps.
> > More than the issue with unnecessarily reserving memory we simply can't
> > trust the e820 to cover all the firmware tables here. I think it is
> > better to simply not give any memory under 1M back to Xen in the domain
> > 0 case.
> >
> > 8<-----------
> >
> > >From e25932748e354f5da3fbb5719fb17f9ca2e35f09 Mon Sep 17 00:00:00 2001
> > From: Ian Campbell <ian.campbell@xxxxxxxxxx>
> > Date: Wed, 27 Oct 2010 17:02:25 +0100
> > Subject: [PATCH] xen: do not release any memory under 1M in domain 0
> >
> > We already deliberately setup a 1-1 P2M for the region up to 1M in
> > order to allow code which assumes this region is already mapped to
> > work without having to convert everything to ioremap.
> >
> 
> Looks good, but:
> 
> > Domain 0 should not return any apparently unused memory regions
> > (reserved or otherwise) in this region to Xen since the e820 may not
> > accurately reflect what the BIOS has stashed in this region.
> >
> > Similarly do not reserve the ISA range if we are domain 0 since it is
> > not necessarily normal, usable memory in that case.
> >
> > Since we now assume that we have a (reasonably) sensible e820 map in
> > domain 0 make a failure to obtain a machine memory map from the
> > hypervisor fatal rather than struggling on with a made up map and
> > suffering all the potential fallout.
> >
> > Signed-off-by: Ian Campbell <ian.campbell@xxxxxxxxxx>
> > ---
> >  arch/x86/xen/setup.c |   32 +++++++++++++++++++++++++-------
> >  1 files changed, 25 insertions(+), 7 deletions(-)
> >
> > diff --git a/arch/x86/xen/setup.c b/arch/x86/xen/setup.c
> > index 915b0c3..baad88c 100644
> > --- a/arch/x86/xen/setup.c
> > +++ b/arch/x86/xen/setup.c
> > @@ -84,6 +84,22 @@ static unsigned long __init
> > xen_release_chunk(phys_addr_t start_addr,
> >     start = PFN_UP(start_addr);
> >     end = PFN_DOWN(end_addr);
> >  
> > +   /*
> > +    * Domain 0 maintains a 1-1 P2M mapping for the first megabyte
> > +    * so do not return such memory to the hypervisor.
> > +    *
> > +    * This region can contain various firmware tables and the
> > +    * like which are often assumed to be always mapped and
> > +    * available via phys_to_virt.
> > +    */
> > +   if (xen_initial_domain()) {
> > +           if (end < PFN_DOWN(0x100000UL))
> > +                   return 0;
> > +
> > +           if (start < PFN_DOWN(0x100000UL))
> > +                   start = PFN_DOWN(0x100000UL);
> 
> Use ISA_END_ADDRESS I think.

Yes.

I notice that the 1-1 P2M only seems to extend from ISA_START_ADDRESS
(0xa0000) but the range missing from the e820 here starts at 0x9e000.
Should we be doing the 1-1 P2M over the whole lower 1M?

> 
> > +   }
> > +
> >     if (end <= start)
> >             return 0;
> >  
> > @@ -163,6 +179,7 @@ char * __init xen_memory_setup(void)
> >             XENMEM_memory_map;
> >     rc = HYPERVISOR_memory_op(op, &memmap);
> >     if (rc == -ENOSYS) {
> > +           BUG_ON(xen_initial_domain());
> >             memmap.nr_entries = 1;
> >             map[0].addr = 0ULL;
> >             map[0].size = mem_end;
> > @@ -200,15 +217,16 @@ char * __init xen_memory_setup(void)
> >     }
> >  
> >     /*
> > -    * Even though this is normal, usable memory under Xen, reserve
> > -    * ISA memory anyway because too many things think they can poke
> > -    * about in there.
> > +    * Even though this is normal, usable memory in a Xen domU,
> > +    * reserve ISA memory anyway because too many things think
> > +    * they can poke about in there.
> >      *
> > -    * In a dom0 kernel, this region is identity mapped with the
> > -    * hardware ISA area, so it really is out of bounds.
> > +    * In dom0 we use the host e820 and therefore do not need to
> > +    * specially reserved anything.
> >      */
> > -   e820_add_region(ISA_START_ADDRESS, ISA_END_ADDRESS -
> > ISA_START_ADDRESS,
> > -                   E820_RESERVED);
> > +   if (!xen_initial_domain())
> > +           e820_add_region(ISA_START_ADDRESS, ISA_END_ADDRESS -
> > ISA_START_ADDRESS,
> > +                           E820_RESERVED);
> 
> What's the problem with making it unconditionally reserved even if the
> host E820 doesn't?

Perhaps nothing, but if it was necessary wouldn't it be needed on native
too?

Ian.



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