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; Fri, 12 Apr 2019 04:15:52 -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 C1C3286663; Fri, 12 Apr 2019 11:15:51 +0000 (UTC) Received: from lacos-laptop-7.usersys.redhat.com (ovpn-120-65.rdu2.redhat.com [10.10.120.65]) by smtp.corp.redhat.com (Postfix) with ESMTP id E7393600C5; Fri, 12 Apr 2019 11:15:49 +0000 (UTC) Subject: Re: [edk2-devel] [PATCH v2 22/31] OvmfPkg/XenPlatformPei: Rework memory detection To: devel@edk2.groups.io, anthony.perard@citrix.com Cc: Jordan Justen , Ard Biesheuvel , Julien Grall , xen-devel@lists.xenproject.org References: <20190409110844.14746-1-anthony.perard@citrix.com> <20190409110844.14746-23-anthony.perard@citrix.com> From: "Laszlo Ersek" Message-ID: Date: Fri, 12 Apr 2019 13:15:48 +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: <20190409110844.14746-23-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.26]); Fri, 12 Apr 2019 11:15:52 +0000 (UTC) Content-Type: text/plain; charset=utf-8 Content-Language: en-US Content-Transfer-Encoding: 7bit On 04/09/19 13:08, 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 > works without CMOS by reading the e820 table. > > Rework XenPublishRamRegions for PVH, handle the Reserve type and explain > about the ACPI type. MTRR settings aren't modified anymore, on HVM, it's > already done by hvmloader, on PVH it is supposed to have sane default. > > Contributed-under: TianoCore Contribution Agreement 1.1 > Signed-off-by: Anthony PERARD > --- For this patch: Acked-by: Laszlo Ersek Another comment below: > > Notes: > About MTRR, should we redo the setting in OVMF? Even if in both case of > PVH and HVM, something would have setup the default type to write back > and handle a few other ranges like PCI hole, hvmloader for HVM or and > libxc I think for PVH. This patch is *exactly* the kind of change that I want to keep as far as possible away from code that runs (even if in part) on QEMU. Every time I need to touch OvmfPkg/PlatformPei/MemDetect.c, and in particular go near the MTRR setup or the physical memory layout (resource descriptor HOBs, CPU address width, etc), I start convulsing. If, under "OvmfPkg/PlatformPei/", you could limit your changes to "Xen.c", I'd be OK with that. Otherwise, please don't go near that code. Again, this is *the* kind of change why we have the platform split / duplication. Thanks Laszlo > > (For PVH, it's in the spec as well > https://xenbits.xenproject.org/docs/unstable/misc/pvh.html#mtrr ) > > OvmfPkg/XenPlatformPei/Platform.h | 6 ++ > OvmfPkg/XenPlatformPei/MemDetect.c | 71 ++++++++++++++++++++ > OvmfPkg/XenPlatformPei/Xen.c | 47 +++++++++---- > 3 files changed, 111 insertions(+), 13 deletions(-) > > diff --git a/OvmfPkg/XenPlatformPei/Platform.h b/OvmfPkg/XenPlatformPei/Platform.h > index c5a139f016..d6d93eab2d 100644 > --- a/OvmfPkg/XenPlatformPei/Platform.h > +++ b/OvmfPkg/XenPlatformPei/Platform.h > @@ -120,6 +120,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 db3d387d1c..0c066d518b 100644 > --- a/OvmfPkg/XenPlatformPei/MemDetect.c > +++ b/OvmfPkg/XenPlatformPei/MemDetect.c > @@ -102,6 +102,47 @@ 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. > + // > + HighestAddress &= ~(UINT64)EFI_PAGE_MASK; > + > + return HighestAddress; > +} > > UINT32 > GetSystemMemorySizeBelow4gb ( > @@ -111,6 +152,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 > @@ -135,6 +189,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/Xen.c b/OvmfPkg/XenPlatformPei/Xen.c > index f8cee671d8..7b503f2c4e 100644 > --- a/OvmfPkg/XenPlatformPei/Xen.c > +++ b/OvmfPkg/XenPlatformPei/Xen.c > @@ -282,6 +282,8 @@ XenPublishRamRegions ( > EFI_E820_ENTRY64 *E820Map; > UINT32 E820EntriesCount; > EFI_STATUS Status; > + EFI_E820_ENTRY64 *Entry; > + UINTN Index; > > DEBUG ((EFI_D_INFO, "Using memory map provided by Xen\n")); > > @@ -290,26 +292,45 @@ XenPublishRamRegions ( > // > E820EntriesCount = 0; > Status = XenGetE820Map (&E820Map, &E820EntriesCount); > - > ASSERT_EFI_ERROR (Status); > > - if (E820EntriesCount > 0) { > - EFI_E820_ENTRY64 *Entry; > - UINT32 Loop; > + for (Index = 0; Index < E820EntriesCount; Index++) { > + UINT64 Base; > + UINT64 End; > > - for (Loop = 0; Loop < E820EntriesCount; Loop++) { > - Entry = E820Map + Loop; > + 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: > + // > + // Ignore, OVMF should read the ACPI tables and provide them to linux > + // from a different location. > + // > + break; > + case EfiAcpiAddressRangeReserved: > // > - // Only care about RAM > + // Avoid ranges marked as reserved in the e820 table provided by > + // hvmloader as it conflicts with an other aperture. > + // 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); > } > - > - AddMemoryBaseSizeHob (Entry->BaseAddr, Entry->Length); > - > - MtrrSetMemoryAttribute (Entry->BaseAddr, Entry->Length, CacheWriteBack); > + break; > + default: > + break; > } > } > } >