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

[Anthony PERARD <anthony.perard@xxxxxxxxxx>]

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())
    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)) {
        AddReservedMemoryRangeHob (LocalApic + SIZE_4KB, End, FALSE);
      }
    }
  }

Also, I will always add the 0xfee00000 as mapped IO, CpuDxe will not
complain as the region will be of the expected type.

I think with that change (and the other about the ACPI entries),
everything from the e820 table will be put into OVMF's memory map.

-- 
Anthony PERARD

_______________________________________________
Xen-devel mailing list
Xen-devel@xxxxxxxxxxxxxxxxxxxx
https://lists.xenproject.org/mailman/listinfo/xen-devel