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

[Jeremy Fitzhardinge <jeremy@xxxxxxxx>]

 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.

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

    J

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