Re: [PATCH v3 24/35] OvmfPkg/XenPlatformPei: Rework memory detection

["Roger Pau Monné" <roger.pau@citrix.com>]

On Wed, Jul 24, 2019 at 05:17:59PM +0100, Anthony PERARD wrote:
> On Tue, Jul 23, 2019 at 11:42:07AM +0200, Roger Pau Monné wrote:
> > On Mon, Jul 22, 2019 at 03:53:19PM +0100, Anthony PERARD wrote:
> > > On Mon, Jul 15, 2019 at 04:15:21PM +0200, Roger Pau Monné wrote:
> > > > On Thu, Jul 04, 2019 at 03:42:22PM +0100, Anthony PERARD wrote:
> > > > > +      // error message: CpuDxe: IntersectMemoryDescriptor:
> > > > > +      //        desc [FC000000, 100000000) type 1 cap 8700000000026001
> > > > > +      //        conflicts with aperture [FEE00000, FEE01000) cap 1
> > > > >        //
> > > > > -      if (Entry->Type != EfiAcpiAddressRangeMemory) {
> > > > > -        continue;
> > > > > +      if (!XenHvmloaderDetected ()) {
> > > > > +        AddReservedMemoryBaseSizeHob (Base, End - Base, FALSE);
> > > > 
> > > > This special casing for PVH looks weird, ideally we would like to use
> > > > the same code path, or else it should be explicitly mentioned why PVH
> > > > has diverging behaviour.
> > > 
> > > I think hvmloader is the issue rather than PVH. Here is part of the
> > > "memory map" as found in hvmloader/config.h:
> > > 
> > >   /* Special BIOS mappings, etc. are allocated from here upwards... */
> > >   #define RESERVED_MEMBASE              0xFC000000
> > >   /* NB. ACPI_INFO_PHYSICAL_ADDRESS *MUST* match definition in acpi/dsdt.asl! */
> > >   #define ACPI_INFO_PHYSICAL_ADDRESS    0xFC000000
> > >   #define RESERVED_MEMORY_DYNAMIC_START 0xFC001000
> > >   #define RESERVED_MEMORY_DYNAMIC_END   0xFE000000
> > > 
> > > and hvmloader simply creates a single e820 reserved entry, from
> > > RESERVED_MEMBASE to the top of 4GB. It's probably too much.
> > 
> > But isn't this kind of dangerous? How can you assure future versions
> > of hvmloader won't use this space?
> > 
> > > If hvmloader only reserved
> > > ACPI_INFO_PHYSICAL_ADDRESS-RESERVED_MEMORY_DYNAMIC_END, I might not have
> > > to special case hvmloader.
> > 
> > Could we look into getting this fixed in hvmloader then?
> > 
> > I think it's dangerous for OVMF to play such tricks with the memory
> > map.
> > 
> > > As far as I know 0xfee00000 isn't a special
> > > bios mapping, but something the hardware provides.
> > 
> > Yes, that's used by the lapic, so it's not specific to hvmloader.
> 
> Right, I've got a closer look at that CpuDxe module, it wants the local
> APIC memory mapped space to be "mapped IO", and that different than
> "reserved".
> 
> So while parsing the e820 from hvmloader, instead of ignoring all
> reserved region, I'm going to avoid adding the local apic memory mapped
> space.
> 
> something like:
>   if (hvmloaderDetected())

I don't think you need to gate this on hvmloader being used, while
it's true that PVH memory map doesn't contain such reserved memory
region ATM I don't see any harm in doing this for PVH also.

>     Base = $(start of the e820 entry);
>     End = $(start of the e820 entry + size);
>     LocalApic = 0xfee00000;
>     if (Base < LocalApic && LocalApic < End) {
>       AddReservedMemoryRangeHob (Base, LocalApic, FALSE);
>       if (End > (LocalApic + SIZE_4KB)) {

The range is actually from 0xfee00000 to 0xfeefffff (2MB), so you
likely want to make sure non of this is added as reserved?

Thanks, Roger.