From mboxrd@z Thu Jan 1 00:00:00 1970 Authentication-Results: mx.groups.io; dkim=missing; spf=pass (domain: redhat.com, ip: 209.132.183.28, mailfrom: lersek@redhat.com) Received: from mx1.redhat.com (mx1.redhat.com [209.132.183.28]) by groups.io with SMTP; Thu, 15 Aug 2019 02:26:57 -0700 Received: from smtp.corp.redhat.com (int-mx05.intmail.prod.int.phx2.redhat.com [10.5.11.15]) (using TLSv1.2 with cipher AECDH-AES256-SHA (256/256 bits)) (No client certificate requested) by mx1.redhat.com (Postfix) with ESMTPS id 937F183F42; Thu, 15 Aug 2019 09:26:56 +0000 (UTC) Received: from lacos-laptop-7.usersys.redhat.com (ovpn-117-57.ams2.redhat.com [10.36.117.57]) by smtp.corp.redhat.com (Postfix) with ESMTP id 0A31F9805; Thu, 15 Aug 2019 09:26:54 +0000 (UTC) Subject: Re: [edk2-devel] [PATCH v5 23/35] OvmfPkg/XenPlatformPei: Rework memory detection To: devel@edk2.groups.io, anthony.perard@citrix.com Cc: Jordan Justen , Julien Grall , xen-devel@lists.xenproject.org, Ard Biesheuvel References: <20190813113119.14804-1-anthony.perard@citrix.com> <20190813113119.14804-24-anthony.perard@citrix.com> From: "Laszlo Ersek" Message-ID: Date: Thu, 15 Aug 2019 11:26:54 +0200 User-Agent: Mozilla/5.0 (X11; Linux x86_64; rv:52.0) Gecko/20100101 Thunderbird/52.9.1 MIME-Version: 1.0 In-Reply-To: <20190813113119.14804-24-anthony.perard@citrix.com> X-Scanned-By: MIMEDefang 2.79 on 10.5.11.15 X-Greylist: Sender IP whitelisted, not delayed by milter-greylist-4.5.16 (mx1.redhat.com [10.5.110.27]); Thu, 15 Aug 2019 09:26:56 +0000 (UTC) Content-Type: text/plain; charset=utf-8 Content-Language: en-US Content-Transfer-Encoding: 7bit On 08/13/19 13:31, Anthony PERARD wrote: > When running as a Xen PVH guest, there is no CMOS to read the memory > size from. Rework GetSystemMemorySize(Below|Above)4gb() so they can > work without CMOS by reading the e820 table. > > Rework XenPublishRamRegions to also care for the reserved and ACPI > entry in the e820 table. The region that was added by InitializeXen() > isn't needed as that same entry is in the e820 table provided by > hvmloader. > > MTRR settings aren't modified anymore, on HVM it's already done by > hvmloader, on PVH it is supposed to have sane default. MTRR will need > to be done properly but keeping what's already been done by programs > that have run before OVMF will do for now. > > Ref: https://bugzilla.tianocore.org/show_bug.cgi?id=1689 > Signed-off-by: Anthony PERARD > Acked-by: Laszlo Ersek > --- > > Notes: > v5: > - fix coding style > - fix typo in commit message > - Handle all possible cases of a E820 reserved range overlapping with the > LAPIC range. > > v4: > - some coding style > - Added AddReservedMemoryRangeHob, and using it. > - this patch now replace "OvmfPkg/XenPlatformPei: Reserve hvmloader's memory only when it has run" > from v3. hvmloader have added an entry in the e820 table, there is no > need for a special case. > - now, everything that is in the e820 table is added to OVMF's memory > map, no more skipping ACPI entries or hvmloader's reserved entries. > Instead, we look for the local APIC region and avoid it if it is > present in the e820. > - rework commit message > > OvmfPkg/XenPlatformPei/Platform.h | 13 ++++++ > OvmfPkg/XenPlatformPei/MemDetect.c | 69 +++++++++++++++++++++++++++ > OvmfPkg/XenPlatformPei/Platform.c | 11 +++++ > OvmfPkg/XenPlatformPei/Xen.c | 75 +++++++++++++++++++++--------- > 4 files changed, 147 insertions(+), 21 deletions(-) > > diff --git a/OvmfPkg/XenPlatformPei/Platform.h b/OvmfPkg/XenPlatformPei/Platform.h > index db9a62572f..7661f4a8de 100644 > --- a/OvmfPkg/XenPlatformPei/Platform.h > +++ b/OvmfPkg/XenPlatformPei/Platform.h > @@ -44,6 +44,13 @@ AddReservedMemoryBaseSizeHob ( > BOOLEAN Cacheable > ); > > +VOID > +AddReservedMemoryRangeHob ( > + EFI_PHYSICAL_ADDRESS MemoryBase, > + EFI_PHYSICAL_ADDRESS MemoryLimit, > + BOOLEAN Cacheable > + ); > + > VOID > AddressWidthInitialization ( > VOID > @@ -114,6 +121,12 @@ XenPublishRamRegions ( > VOID > ); > > +EFI_STATUS > +XenGetE820Map ( > + EFI_E820_ENTRY64 **Entries, > + UINT32 *Count > + ); > + > extern EFI_BOOT_MODE mBootMode; > > extern UINT8 mPhysMemAddressWidth; > diff --git a/OvmfPkg/XenPlatformPei/MemDetect.c b/OvmfPkg/XenPlatformPei/MemDetect.c > index cf95f9c474..1f81eee407 100644 > --- a/OvmfPkg/XenPlatformPei/MemDetect.c > +++ b/OvmfPkg/XenPlatformPei/MemDetect.c > @@ -96,6 +96,45 @@ Q35TsegMbytesInitialization ( > mQ35TsegMbytes = ExtendedTsegMbytes; > } > > +STATIC > +UINT64 > +GetHighestSystemMemoryAddress ( > + BOOLEAN Below4gb > + ) > +{ > + EFI_E820_ENTRY64 *E820Map; > + UINT32 E820EntriesCount; > + EFI_E820_ENTRY64 *Entry; > + EFI_STATUS Status; > + UINT32 Loop; > + UINT64 HighestAddress; > + UINT64 EntryEnd; > + > + HighestAddress = 0; > + > + Status = XenGetE820Map (&E820Map, &E820EntriesCount); > + ASSERT_EFI_ERROR (Status); > + > + for (Loop = 0; Loop < E820EntriesCount; Loop++) { > + Entry = E820Map + Loop; > + EntryEnd = Entry->BaseAddr + Entry->Length; > + > + if (Entry->Type == EfiAcpiAddressRangeMemory && > + EntryEnd > HighestAddress) { > + > + if (Below4gb && (EntryEnd <= BASE_4GB)) { > + HighestAddress = EntryEnd; > + } else if (!Below4gb && (EntryEnd >= BASE_4GB)) { > + HighestAddress = EntryEnd; > + } > + } > + } > + > + // > + // Round down the end address. > + // > + return HighestAddress & ~(UINT64)EFI_PAGE_MASK; > +} > > UINT32 > GetSystemMemorySizeBelow4gb ( > @@ -105,6 +144,19 @@ GetSystemMemorySizeBelow4gb ( > UINT8 Cmos0x34; > UINT8 Cmos0x35; > > + // > + // In PVH case, there is no CMOS, we have to calculate the memory size > + // from parsing the E820 > + // > + if (XenPvhDetected ()) { > + UINT64 HighestAddress; > + > + HighestAddress = GetHighestSystemMemoryAddress (TRUE); > + ASSERT (HighestAddress > 0 && HighestAddress <= BASE_4GB); > + > + return HighestAddress; > + } > + > // > // CMOS 0x34/0x35 specifies the system memory above 16 MB. > // * CMOS(0x35) is the high byte > @@ -129,6 +181,23 @@ GetSystemMemorySizeAbove4gb ( > UINT32 Size; > UINTN CmosIndex; > > + // > + // In PVH case, there is no CMOS, we have to calculate the memory size > + // from parsing the E820 > + // > + if (XenPvhDetected ()) { > + UINT64 HighestAddress; > + > + HighestAddress = GetHighestSystemMemoryAddress (FALSE); > + ASSERT (HighestAddress == 0 || HighestAddress >= BASE_4GB); > + > + if (HighestAddress >= BASE_4GB) { > + HighestAddress -= BASE_4GB; > + } > + > + return HighestAddress; > + } > + > // > // CMOS 0x5b-0x5d specifies the system memory above 4GB MB. > // * CMOS(0x5d) is the most significant size byte > diff --git a/OvmfPkg/XenPlatformPei/Platform.c b/OvmfPkg/XenPlatformPei/Platform.c > index 6aaafc3ee9..2f42ca6ccd 100644 > --- a/OvmfPkg/XenPlatformPei/Platform.c > +++ b/OvmfPkg/XenPlatformPei/Platform.c > @@ -102,6 +102,17 @@ AddReservedMemoryBaseSizeHob ( > ); > } > > +VOID > +AddReservedMemoryRangeHob ( > + EFI_PHYSICAL_ADDRESS MemoryBase, > + EFI_PHYSICAL_ADDRESS MemoryLimit, > + BOOLEAN Cacheable > + ) > +{ > + AddReservedMemoryBaseSizeHob (MemoryBase, > + (UINT64)(MemoryLimit - MemoryBase), Cacheable); > +} > + > VOID > AddIoMemoryRangeHob ( > EFI_PHYSICAL_ADDRESS MemoryBase, > diff --git a/OvmfPkg/XenPlatformPei/Xen.c b/OvmfPkg/XenPlatformPei/Xen.c > index 72f6f37b46..c4506def9a 100644 > --- a/OvmfPkg/XenPlatformPei/Xen.c > +++ b/OvmfPkg/XenPlatformPei/Xen.c > @@ -276,9 +276,14 @@ XenPublishRamRegions ( > VOID > ) > { > - EFI_E820_ENTRY64 *E820Map; > - UINT32 E820EntriesCount; > - EFI_STATUS Status; > + EFI_E820_ENTRY64 *E820Map; > + UINT32 E820EntriesCount; > + EFI_STATUS Status; > + EFI_E820_ENTRY64 *Entry; > + UINTN Index; > + UINT64 LapicBase; > + UINT64 LapicEnd; > + > The extra newline is not really justified, but it's also harmless and not too annoying. Otherwise, the update looks good to me, addressing: * http://mid.mail-archive.com/7b8cb5ef-2163-9e22-350f-877be6951b34@redhat.com https://edk2.groups.io/g/devel/message/44615 * http://mid.mail-archive.com/20190807152852.e47kogsxubbjkue5@Air-de-Roger https://edk2.groups.io/g/devel/message/45012 So my ACK stands. Thanks Laszlo > DEBUG ((DEBUG_INFO, "Using memory map provided by Xen\n")); > > @@ -287,26 +292,60 @@ XenPublishRamRegions ( > // > E820EntriesCount = 0; > Status = XenGetE820Map (&E820Map, &E820EntriesCount); > - > ASSERT_EFI_ERROR (Status); > > - if (E820EntriesCount > 0) { > - EFI_E820_ENTRY64 *Entry; > - UINT32 Loop; > + LapicBase = PcdGet32 (PcdCpuLocalApicBaseAddress); > + LapicEnd = LapicBase + SIZE_1MB; > + AddIoMemoryRangeHob (LapicBase, LapicEnd); > > - for (Loop = 0; Loop < E820EntriesCount; Loop++) { > - Entry = E820Map + Loop; > + for (Index = 0; Index < E820EntriesCount; Index++) { > + UINT64 Base; > + UINT64 End; > + UINT64 ReservedBase; > + UINT64 ReservedEnd; > > + Entry = &E820Map[Index]; > + > + // > + // Round up the start address, and round down the end address. > + // > + Base = ALIGN_VALUE (Entry->BaseAddr, (UINT64)EFI_PAGE_SIZE); > + End = (Entry->BaseAddr + Entry->Length) & ~(UINT64)EFI_PAGE_MASK; > + > + switch (Entry->Type) { > + case EfiAcpiAddressRangeMemory: > + AddMemoryRangeHob (Base, End); > + break; > + case EfiAcpiAddressRangeACPI: > + AddReservedMemoryRangeHob (Base, End, FALSE); > + break; > + case EfiAcpiAddressRangeReserved: > // > - // Only care about RAM > + // hvmloader marks a range that overlaps with the local APIC memory > + // mapped region as reserved, but CpuDxe wants it as mapped IO. We > + // have already added it as mapped IO, so skip it here. > // > - if (Entry->Type != EfiAcpiAddressRangeMemory) { > - continue; > - } > > - AddMemoryBaseSizeHob (Entry->BaseAddr, Entry->Length); > + // > + // add LAPIC predecessor range, if any > + // > + ReservedBase = Base; > + ReservedEnd = MIN (End, LapicBase); > + if (ReservedBase < ReservedEnd) { > + AddReservedMemoryRangeHob (ReservedBase, ReservedEnd, FALSE); > + } > > - MtrrSetMemoryAttribute (Entry->BaseAddr, Entry->Length, CacheWriteBack); > + // > + // add LAPIC successor range, if any > + // > + ReservedBase = MAX (Base, LapicEnd); > + ReservedEnd = End; > + if (ReservedBase < ReservedEnd) { > + AddReservedMemoryRangeHob (ReservedBase, ReservedEnd, FALSE); > + } > + break; > + default: > + break; > } > } > } > @@ -326,12 +365,6 @@ InitializeXen ( > { > RETURN_STATUS PcdStatus; > > - // > - // Reserve away HVMLOADER reserved memory [0xFC000000,0xFD000000). > - // This needs to match HVMLOADER RESERVED_MEMBASE/RESERVED_MEMSIZE. > - // > - AddReservedMemoryBaseSizeHob (0xFC000000, 0x1000000, FALSE); > - > PcdStatus = PcdSetBoolS (PcdPciDisableBusEnumeration, TRUE); > ASSERT_RETURN_ERROR (PcdStatus); > >