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; Tue, 30 Jul 2019 04:45:11 -0700 Received: from smtp.corp.redhat.com (int-mx01.intmail.prod.int.phx2.redhat.com [10.5.11.11]) (using TLSv1.2 with cipher AECDH-AES256-SHA (256/256 bits)) (No client certificate requested) by mx1.redhat.com (Postfix) with ESMTPS id 72E6330860C9; Tue, 30 Jul 2019 11:45:10 +0000 (UTC) Received: from lacos-laptop-7.usersys.redhat.com (unknown [10.36.118.39]) by smtp.corp.redhat.com (Postfix) with ESMTP id 3F4186012C; Tue, 30 Jul 2019 11:45:08 +0000 (UTC) Subject: Re: [edk2-devel] [PATCH v4 23/35] OvmfPkg/XenPlatformPei: Rework memory detection To: devel@edk2.groups.io, anthony.perard@citrix.com Cc: Julien Grall , xen-devel@lists.xenproject.org, Jordan Justen , Ard Biesheuvel , =?UTF-8?Q?Roger_Pau_Monn=c3=a9?= , Andrew Cooper References: <20190729153944.24239-1-anthony.perard@citrix.com> <20190729153944.24239-24-anthony.perard@citrix.com> From: "Laszlo Ersek" Message-ID: <7b8cb5ef-2163-9e22-350f-877be6951b34@redhat.com> Date: Tue, 30 Jul 2019 13:45:07 +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: <20190729153944.24239-24-anthony.perard@citrix.com> X-Scanned-By: MIMEDefang 2.79 on 10.5.11.11 X-Greylist: Sender IP whitelisted, not delayed by milter-greylist-4.5.16 (mx1.redhat.com [10.5.110.44]); Tue, 30 Jul 2019 11:45:10 +0000 (UTC) Content-Type: text/plain; charset=utf-8 Content-Language: en-US Content-Transfer-Encoding: 7bit On 07/29/19 17:39, 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 programmes > that has runned before OVMF will do for now. (1) s/programmes that has runned/programs that have run/ > > Ref: https://bugzilla.tianocore.org/show_bug.cgi?id=1689 > Signed-off-by: Anthony PERARD > Acked-by: Laszlo Ersek > --- > > Notes: > 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 | 70 +++++++++++++++++++----------- > 4 files changed, 137 insertions(+), 26 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 a21d657357..182e96cc5b 100644 > --- a/OvmfPkg/XenPlatformPei/Xen.c > +++ b/OvmfPkg/XenPlatformPei/Xen.c > @@ -276,9 +276,12 @@ 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; > + EFI_PHYSICAL_ADDRESS LocalApic; > > DEBUG ((DEBUG_INFO, "Using memory map provided by Xen\n")); > > @@ -287,26 +290,47 @@ XenPublishRamRegions ( > // > E820EntriesCount = 0; > Status = XenGetE820Map (&E820Map, &E820EntriesCount); > - > ASSERT_EFI_ERROR (Status); > > - if (E820EntriesCount > 0) { > - EFI_E820_ENTRY64 *Entry; > - UINT32 Loop; > - > - for (Loop = 0; Loop < E820EntriesCount; Loop++) { > - Entry = E820Map + Loop; > - > - // > - // Only care about RAM > - // > - if (Entry->Type != EfiAcpiAddressRangeMemory) { > - continue; > + LocalApic = PcdGet32(PcdCpuLocalApicBaseAddress); (2) missing space before "(" -- but, actually, please see below > + AddIoMemoryBaseSizeHob (LocalApic, SIZE_1MB); > + > + for (Index = 0; Index < E820EntriesCount; Index++) { > + UINT64 Base; > + UINT64 End; > + > + 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: > + if (Base < LocalApic && LocalApic < End) { > + // > + // 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. > + // > + AddReservedMemoryRangeHob (Base, LocalApic, FALSE); > + if (End > (LocalApic + SIZE_1MB)) { > + AddReservedMemoryRangeHob (LocalApic + SIZE_1MB, End, FALSE); > + } > + } else { > + AddReservedMemoryRangeHob (Base, End, FALSE); > } (3) Not sure how general you want to be here, but I don't think this is fully general. How about: (3a) before the loop, drop "LocalApic", and add: UINT64 LapicBase; UINT64 LapicEnd; LapicBase = PcdGet32 (PcdCpuLocalApicBaseAddress); LapicEnd = LapicBase + SIZE_1MB; (3b) inside the loop body, near the top of the block, add: UINT64 ReservedBase; UINT64 ReservedEnd; (3c) under the EfiAcpiAddressRangeReserved case label: // // add LAPIC predecessor range, if any // ReservedBase = Base; ReservedEnd = MIN (End, LapicBase); if (ReservedBase < ReservedEnd) { AddReservedMemoryRangeHob (ReservedBase, ReservedEnd, FALSE); } // // add LAPIC successor range, if any // ReservedBase = MAX (Base, LapicEnd); ReservedEnd = End; if (ReservedBase < ReservedEnd) { AddReservedMemoryRangeHob (ReservedBase, ReservedEnd, FALSE); } This will cover all possible constellations between the [LapicBase, LapicEnd) and [Base, End) intervals. --*-- Regarding the approach itself, I'll defer to Roger. Thanks Laszlo > - > - AddMemoryBaseSizeHob (Entry->BaseAddr, Entry->Length); > - > - MtrrSetMemoryAttribute (Entry->BaseAddr, Entry->Length, CacheWriteBack); > + break; > + default: > + break; > } > } > } > @@ -326,12 +350,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); > >