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

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

On Fri, Jul 12, 2013 at 10:38:13PM +0100, David Vrabel wrote:
> On 12/07/2013 20:33, Konrad Rzeszutek Wilk wrote:
> > 
> > So the 'HYPERVISOR_update_va_mapping' fails b/c we include it in the
> > xen_set_identify_and_release_chunk.
> 
> That's what I understand.
> 
> > 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.
> 
> I suppose we could do what you suggest but there would have to be a
> xen_release_chunk() function added, otherwise you would waste the frames
> in that region.

That is a bit different logic - but yes that would still have to work
as before and release the frames.
> 
> I remain unconvinced that adding pointless unusable regions into the
> dom0's memory map makes any sense.

Please also keep in mind that domU with PCI passthrough can have an E820
that looks like the host. Meaning this is not just for dom0.

> 
> >>> OK. What about the BIOS manufacturing [unusable regions?
> >>
> >> 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.
> 
> No, it's leaving it as a RAM region, as setup by Xen (and as marked as
> such in the pseudo-physical map).
> 
> I guess I still haven't explained very well what the (confusing) setup
> code is trying to do.
> 
> 1. Get psuedo-physical memory map.
> 
> 2. Get machine memory map.
> 
> 3. For each "interesting" (reserved or MMIO hole) region in the machine
>    memory map:
> 
>    a. Add reserved region or hole to pseudo-physical memory map.
>    b. Release memory.
>    c. Set as 1:1 in p2m.
>    d. Update any existing mappings.
> 
> The code as-is doesn't work like that but the end result should be the same.

It does right now (thought the operations are in a different order - b, d, a, c).

But your point about the E820_UNUSED throws a wrench in here. The amount of pages
that a guest has should (after re-organizing the P2M to represent the E820)
equal the amount of E820_RAM regions. I am ignoring the balloon case here.

That is still the case with 'tboot' as 'tboot' consumes some of the E820_RAM regions
and converts them in E820_UNUSED. The amount of pages that the guest gets is the
total_pages - <pages consumed for E820_UNUSED>. With your patch, you convert the
E820_UNUSED to E820_RAM. I believe that what you end up is that the amount of
E820_RAM PFNS >= nr_pages. In this case I am ignoring the usage of 'dom0_memory_max'
parameter.

Since the only code (since v3.9) that maps E820_UNUSED is the special code you
added - the HYPERVISOR_update_va_mapping in xen_set_identity_and_release_chunk,
perhaps a better mechanism is for said code to consult the E820 and not call
said update_va_mapping for the E820_UNUSED regions. After all, it is OK
_not_ to call that for 1-1 regions. It worked before - albeit it was buggy with
special (shared) 1-1 regions.

Heck, we could make that loop consult the M2P and _only_ call the update_va_mapping
on the M2P[pfn] = 0x55555555555.

Either way I think that there are two easy solutions that are also candidates for
stable tree and also make it possible to release the frames:
 - Either add some extra logic in xen_set_identity_and_release_chunk to not do the
   hypercall for E820_UNUSED region
 - Or do a check in the M2P[pfn] and skip if M2P[pfn]==shared_mfn entry value (thought
   I have no idea how hard-coded that is in the ABI) in the xen_set_identity_and_release_chunk.