Re: [PATCH] x86/xen: do not identity map E820 memory regions that are UNUSABLE

[Konrad Rzeszutek Wilk <konrad.wilk@oracle.com>]

On Thu, Jul 11, 2013 at 12:21:42PM +0100, David Vrabel wrote:
> On 09/07/13 19:45, Konrad Rzeszutek Wilk wrote:
> > On Tue, Jul 09, 2013 at 03:44:38PM +0100, David Vrabel wrote:
> >> On 09/07/13 15:13, Konrad Rzeszutek Wilk wrote:
> >>> On Tue, Jul 09, 2013 at 02:38:53PM +0100, David Vrabel wrote:
> >>>> From: David Vrabel <david.vrabel@citrix.com>
> >>>>
> >>>> If there are UNUSABLE regions in the machine memory map, dom0 will
> >>>> attempt to map them 1:1 which is not permitted by Xen and the kernel
> >>>> will crash.
> >>>>
> >>>> There isn't anything interesting in the UNUSABLE region that the dom0
> >>>> kernel needs access to so we can avoid making the 1:1 mapping and
> >>>> leave the region as RAM.
> >>>>
> >>>> Since the obtaining the memory map for dom0 and domU are now more
> >>>> different, refactor each into separate functions.
> >>>>
> >>>> This fixes a dom0 boot failure if tboot is used (because tboot was
> >>>> marking a memory region as UNUSABLE).
> >>>
> >>> Please also include the serial log that shows the crash.
> >>
> >> It's a domain crash by Xen and it isn't useful as none of the stack is
> >> decoded.
> > 
> > Could you include the E820 at least to get a sense of where and how
> > this looks? As in - without tboot and then with tboot?
> 
> This would take time to set up the host again and I don't think
> including a specific example of an E820 map with an UNUSABLE region
> really adds anything useful to the commit log.
> 
> You can look at some of the previous threads for examples.
> 
> >>>> +static int __init xen_get_memory_map_dom0(struct e820entry *map,
> >>>> +					  unsigned *nr_entries)
> >>>> +{
> >>>> +	struct xen_memory_map memmap;
> >>>> +	unsigned i;
> >>>> +	int ret;
> >>>> +
> >>>> +	/*
> >>>> +	 * Dom0 requires access to machine addresses for BIOS data and
> >>>> +	 * MMIO (e.g. PCI) devices.  The reset of the kernel expects
> >>>> +	 * to be able to access these through a 1:1 p2m mapping.
> >>>> +	 *
> >>>> +	 * We need to take the pseudo physical memory map and set up
> >>>> +	 * 1:1 mappings corresponding to the RESERVED regions and
> >>>> +	 * holes in the /machine/ memory map, adding/expanding the RAM
> >>>> +	 * region at the end of the map for the relocated RAM.
> >>
> >> This is the key paragraph.  The apparent use of the machine memory map
> >> for dom0  is a confusing fiction.
> > 
> > OK, but I don't follow when dom0 would be using the E820_UNUSED regions.
> > Is it the xen_do_chunk that is failing on said PFNs? Or is it in this
> > code xen_set_identity_and_release_chunk:
> > 
> > "217         /*                                                                      
> > 218          * If the PFNs are currently mapped, the VA mapping also needs          
> > 219          * to be updated to be 1:1.                                             
> > 220          */                                                                     
> > 221         for (pfn = start_pfn; pfn <= max_pfn_mapped && pfn < end_pfn; pfn++)    
> > 222                 (void)HYPERVISOR_update_va_mapping(                             
> > 223                         (unsigned long)__va(pfn << PAGE_SHIFT),                 
> > 224                         mfn_pte(pfn, PAGE_KERNEL_IO), 0);                       
> > 225                                                          "
> > 
> > which updates the initial PTE's with the 1-1 PFN and the E820_UNUSABLE is
> > somehow in between two E820_RAM regions?
> 
> It's here, yes.
> 
> >>>> +	 *
> >>>> +	 * This is more easily done if we just start with the machine
> >>>> +	 * memory map.
> >>>> +	 *
> >>>> +	 * UNUSABLE regions are awkward, they are not interesting to
> >>>> +	 * dom0 and Xen won't allow them to be mapped so we want to
> >>>> +	 * leave these as RAM in the pseudo physical map.
> >>>
> >>> We just want them as such in the P2M but not do any PTE creation for it?
> >>> Why do we care about it? We are not creating any page tables for
> >>> E820_UNUSABLE regions.
> >>
> >> I don't follow what you're asking here.
> > 
> > What code maps said PFNs.
> 
> See above.

So the 'HYPERVISOR_update_va_mapping' fails b/c we include it in the
xen_set_identify_and_release_chunk. Why not make the logic that sets "gaps"
and E820_RESERVED regions to omit E820_UNUSABLE regions? That would solve
it as well - and we won't be messing with the E820.
> 
> >> In dom0, UNUSABLE regions in the machine memory map are RAM regions on
> >> the pseudo-physical memory map.  So instead of playing games and making
> >> these regions special in the pseudo-physical map we just leave them as RAM.
> > 
> > .. And then exposing them to the kernel to be used as normal RAM?
> 
> Yes.
> 
> > With your change it is. But without your change it would not map it.
> 
> Incorrect. See above.

It would map it using the hypercall. But it would not create pagetables for it
(the generic code that is it).
> 
> >>>> +	 */
> >>>> +
> >>>> +	memmap.nr_entries = *nr_entries;
> >>>> +	set_xen_guest_handle(memmap.buffer, map);
> >>>> +
> >>>> +	ret = HYPERVISOR_memory_op(XENMEM_machine_memory_map, &memmap);
> >>>> +	if (ret < 0)
> >>>> +		return ret;
> >>>> +
> >>>> +	for (i = 0; i < memmap.nr_entries; i++) {
> >>>> +		if (map[i].type == E820_UNUSABLE)
> >>>
> >>> What if the E820_UNUSABLE regions were manufactured by the BIOS? Or
> >>> somebody booted Xen with mem=3G (in which we clip the memory) on a 16GB
> >>> box.
> >>
> >> The resulting memory map should be clipped by the result of the call to
> >> xen_get_max_pages().
> > 
> > OK. What about the BIOS manufacturing it?
> 
> What about it? As a PV guest we don't care what the machine memory map
> looks like, /except/ as a means to find interesting bits of hardware
> that we want 1:1 mappings for.

Right but now you are converting it from 1:1 to a RAM region - where we
don't do 1:1.
> 
> David