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

[Jeremy Fitzhardinge <jeremy@xxxxxxxx>]

 On 10/27/2010 10:39 AM, Ian Campbell wrote:
> 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?

I suppose.  The lower 640k really should be RAM so you can boot DOS, but
I think Xen (Win7 too, I gather) just leaves it completely unused just
in case BIOS uses/trashes it.

>>> +   }
>>> +
>>>     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?

I don't think its *necessary*  (BIOS would have to be desperately broken
to mark anything in that range as E820_RAM), but I'd prefer to avoid
unneeded conditionals just to try and make testing simpler.

    J

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