From mboxrd@z Thu Jan 1 00:00:00 1970 Return-Path: Received: from mail02.groups.io (mail02.groups.io [66.175.222.108]) by spool.mail.gandi.net (Postfix) with ESMTPS id 29A1F74004A for ; Wed, 24 Jan 2024 14:17:22 +0000 (UTC) DKIM-Signature: a=rsa-sha256; bh=sCHHydM0b3gNKdcc3/ZW5aNNlX1fdmKptOfFdynnHI0=; c=relaxed/simple; d=groups.io; h=Message-ID:Date:MIME-Version:Subject:To:Cc:References:From:In-Reply-To:Precedence:List-Subscribe:List-Help:Sender:List-Id:Mailing-List:Delivered-To:Reply-To:List-Unsubscribe-Post:List-Unsubscribe:Content-Language:Content-Type:Content-Transfer-Encoding; s=20140610; t=1706105840; v=1; b=JJh6T4jPuOPi0buJHbFiDXULoPu1EYMGg8U4MCQlnjmjV3tsyZCvJwN+Ym1gEWuO7w2A1Y7q JJAOBaJVOpOg7YVI3FHeHwJ5MyzX4fXpFK3tvdiMC0poo2bZAIX6mIeswRn0ZZ+n2KaKKFMAhqL GkPUV7rSomaaOxazEm08LdiU= X-Received: by 127.0.0.2 with SMTP id cXHIYY7687511xkv9v2YIcFL; Wed, 24 Jan 2024 06:17:20 -0800 X-Received: from us-smtp-delivery-124.mimecast.com (us-smtp-delivery-124.mimecast.com [170.10.129.124]) by mx.groups.io with SMTP id smtpd.web11.23603.1706105839830040529 for ; Wed, 24 Jan 2024 06:17:20 -0800 X-Received: from mimecast-mx02.redhat.com (mimecast-mx02.redhat.com [66.187.233.88]) by relay.mimecast.com with ESMTP with STARTTLS (version=TLSv1.3, cipher=TLS_AES_256_GCM_SHA384) id us-mta-433-fmUvkm-LPxSqz-aS6Sm6wQ-1; Wed, 24 Jan 2024 09:17:15 -0500 X-MC-Unique: fmUvkm-LPxSqz-aS6Sm6wQ-1 X-Received: from smtp.corp.redhat.com (int-mx04.intmail.prod.int.rdu2.redhat.com [10.11.54.4]) (using TLSv1.3 with cipher TLS_AES_256_GCM_SHA384 (256/256 bits) key-exchange X25519 server-signature RSA-PSS (2048 bits) server-digest SHA256) (No client certificate requested) by mimecast-mx02.redhat.com (Postfix) with ESMTPS id 4561E800074; Wed, 24 Jan 2024 14:17:15 +0000 (UTC) X-Received: from [10.39.195.127] (unknown [10.39.195.127]) by smtp.corp.redhat.com (Postfix) with ESMTPS id E28642026F95; Wed, 24 Jan 2024 14:17:13 +0000 (UTC) Message-ID: Date: Wed, 24 Jan 2024 15:17:12 +0100 MIME-Version: 1.0 Subject: Re: [edk2-devel] [Patch v2 1/1] MdeModulePkg/Core/Dxe: Set MemoryTypeInfo bin range from HOB To: devel@edk2.groups.io, michael.d.kinney@intel.com Cc: Liming Gao , Aaron Li , Liu Yun , Andrew Fish References: <20240123202424.1960-1-michael.d.kinney@intel.com> From: "Laszlo Ersek" In-Reply-To: <20240123202424.1960-1-michael.d.kinney@intel.com> X-Scanned-By: MIMEDefang 3.4.1 on 10.11.54.4 X-Mimecast-Spam-Score: 0 X-Mimecast-Originator: redhat.com Precedence: Bulk List-Subscribe: List-Help: Sender: devel@edk2.groups.io List-Id: Mailing-List: list devel@edk2.groups.io; contact devel+owner@edk2.groups.io Reply-To: devel@edk2.groups.io,lersek@redhat.com List-Unsubscribe-Post: List-Unsubscribe=One-Click List-Unsubscribe: X-Gm-Message-State: BmRiFRNEo46Gz6aMXccbgiQAx7686176AA= Content-Language: en-US Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: quoted-printable X-GND-Status: LEGIT Authentication-Results: spool.mail.gandi.net; dkim=pass header.d=groups.io header.s=20140610 header.b=JJh6T4jP; dmarc=fail reason="SPF not aligned (relaxed), DKIM not aligned (relaxed)" header.from=redhat.com (policy=none); spf=pass (spool.mail.gandi.net: domain of bounce@groups.io designates 66.175.222.108 as permitted sender) smtp.mailfrom=bounce@groups.io On 1/23/24 21:24, Michael D Kinney wrote: > Provide an optional method for PEI to declare a specific address > range to use for the Memory Type Information bins. The current > algorithm uses heuristics that tends to place the Memory Type > Information bins in the same location, but memory configuration > changes across boots or algorithm changes across a firmware > updates could potentially change the Memory Type Information bin > location. > > If the HOB List contains a Resource Descriptor HOB that > describes tested system memory and has an Owner GUID of > gEfiMemoryTypeInformationGuid, then use the address range > described by the Resource Descriptor HOB as the preferred > location of the Memory Type Information bins. If this HOB is > not detected, then the current behavior is preserved. > > The HOB with an Owner GUID of gEfiMemoryTypeInformationGuid > is ignored for the following conditions: > * The HOB with an Owner GUID of gEfiMemoryTypeInformationGuid > is smaller than the Memory Type Information bins. > * The HOB list contains more than one Resource Descriptor HOB > with an owner GUID of gEfiMemoryTypeInformationGuid. > * The Resource Descriptor HOB with an Owner GUID of > gEfiMemoryTypeInformationGuid is the same Resource Descriptor > HOB that that describes the PHIT memory range. I feel that, while this GUID usage makes sense, it overloads the already existing meanings of gEfiMemoryTypeInformationGuid. We already use this GUID for two things: (see "MdeModulePkg/Include/Guid/MemoryTypeInformation.h"): - UEFI variable name space GUID for the "MemoryTypeInformation" variable - HOB of type EFI_HOB_TYPE_GUID_EXTENSION, controlling the page counts in the various memory type bins. Now this new use would introduce a HOB of type EFI_HOB_TYPE_RESOURCE_DESCRIPTOR, where the Owner field is gEfiMemoryTypeInformationGuid, and the range described by the PhysicalStart and ResourceLength fields would accommodate the actual bins. On one hand this new HOB is certainly related to the prior two uses, but on the other hand, we'd now have to HOBs that used the "same GUID" for different purposes. What distinguishes them is EFI_HOB_TYPE_GUID_EXTENSION vs. EFI_HOB_TYPE_RESOURCE_DESCRIPTOR -- which I find a bit too obscure. (1) So I'd either suggest generating a brand new GUID, *or* extending the comments in "MdeModulePkg/Include/Guid/MemoryTypeInformation.h" with the new usage: > /** @file > This file defines: > * Memory Type Information GUID for HOB and Variable. > * Memory Type Information Variable Name. > * Memory Type Information GUID HOB data structure. > > The memory type information HOB and variable can > be used to store the information for each memory type in Variable or HO= B. > > Copyright (c) 2006 - 2018, Intel Corporation. All rights reserved.
> SPDX-License-Identifier: BSD-2-Clause-Patent > > **/ Back to the patch: On 1/23/24 21:24, Michael D Kinney wrote: > Update the DxeMain initialization order to initialize GCD > services before any runtime allocations are performed. This > is required to prevent runtime data fragmentation when the > UEFI System Table and UEFI Runtime Service Table is allocated. (2) Should this be a separate patch? (First patch in the series?) This seem= s to effect behavior even if the new HOB is not present. > > Cc: Liming Gao > Cc: Aaron Li > Cc: Liu Yun > Cc: Andrew Fish > Signed-off-by: Michael D Kinney > --- > MdeModulePkg/Core/Dxe/DxeMain.h | 6 ++ > MdeModulePkg/Core/Dxe/DxeMain/DxeMain.c | 23 +++--- > MdeModulePkg/Core/Dxe/Gcd/Gcd.c | 72 ++++++++++++++++- > MdeModulePkg/Core/Dxe/Mem/Page.c | 101 ++++++++++++++++++++++++ > 4 files changed, 190 insertions(+), 12 deletions(-) > > diff --git a/MdeModulePkg/Core/Dxe/DxeMain.h b/MdeModulePkg/Core/Dxe/DxeM= ain.h > index 86a7be2f5188..53e26703f8c7 100644 > --- a/MdeModulePkg/Core/Dxe/DxeMain.h > +++ b/MdeModulePkg/Core/Dxe/DxeMain.h > @@ -277,6 +277,12 @@ CoreInitializePool ( > VOID > ); > > +VOID > +CoreSetMemoryTypeInformationRange ( > + IN EFI_PHYSICAL_ADDRESS Start, > + IN UINT64 Length > + ); > + > /** > Called to initialize the memory map and add descriptors to > the current descriptor list. > diff --git a/MdeModulePkg/Core/Dxe/DxeMain/DxeMain.c b/MdeModulePkg/Core/= Dxe/DxeMain/DxeMain.c > index 0e0f9769b99d..17d510a287e5 100644 > --- a/MdeModulePkg/Core/Dxe/DxeMain/DxeMain.c > +++ b/MdeModulePkg/Core/Dxe/DxeMain/DxeMain.c > @@ -276,6 +276,18 @@ DxeMain ( > > MemoryProfileInit (HobStart); > > + // > + // Start the Image Services. > + // > + Status =3D CoreInitializeImageServices (HobStart); > + ASSERT_EFI_ERROR (Status); > + > + // > + // Initialize the Global Coherency Domain Services > + // > + Status =3D CoreInitializeGcdServices (&HobStart, MemoryBaseAddress, Me= moryLength); > + ASSERT_EFI_ERROR (Status); > + > // > // Allocate the EFI System Table and EFI Runtime Service Table from Ef= iRuntimeServicesData > // Use the templates to initialize the contents of the EFI System Tabl= e and EFI Runtime Services Table > @@ -289,16 +301,9 @@ DxeMain ( > gDxeCoreST->RuntimeServices =3D gDxeCoreRT; > > // > - // Start the Image Services. > + // Update DXE Core Loaded Image Protocol with allocated UEFI System Ta= ble > // > - Status =3D CoreInitializeImageServices (HobStart); > - ASSERT_EFI_ERROR (Status); > - > - // > - // Initialize the Global Coherency Domain Services > - // > - Status =3D CoreInitializeGcdServices (&HobStart, MemoryBaseAddress, Me= moryLength); > - ASSERT_EFI_ERROR (Status); > + gDxeCoreLoadedImage->SystemTable =3D gDxeCoreST; > > // > // Call constructor for all libraries (3) The separate assignment to "gDxeCoreLoadedImage->SystemTable" becomes necessary because CoreInitializeImageServices() is hoisted above gDxeCoreST =3D AllocateRuntimeCopyPool (sizeof (EFI_SYSTEM_TABLE), &mEfiS= ystemTableTemplate); and therefore the assignment Image->Info.SystemTable =3D gDxeCoreST; in CoreInitializeImageServices() becomes ineffective -- at that time, gDxeCoreST is NULL, post-patch. This double-write (overwrite) to the SystemTable field is confusing. I propose (again, in said separate, first patch) the removal of the now-ineffective assignment from CoreInitializeImageServices(). > diff --git a/MdeModulePkg/Core/Dxe/Gcd/Gcd.c b/MdeModulePkg/Core/Dxe/Gcd/= Gcd.c > index 792cd2e0af23..c450d1bf2531 100644 > --- a/MdeModulePkg/Core/Dxe/Gcd/Gcd.c > +++ b/MdeModulePkg/Core/Dxe/Gcd/Gcd.c > @@ -2238,6 +2238,8 @@ CoreInitializeMemoryServices ( > EFI_HOB_HANDOFF_INFO_TABLE *PhitHob; > EFI_HOB_RESOURCE_DESCRIPTOR *ResourceHob; > EFI_HOB_RESOURCE_DESCRIPTOR *PhitResourceHob; > + EFI_HOB_RESOURCE_DESCRIPTOR *MemoryTypeInformationResourceHob; > + UINTN Count; > EFI_PHYSICAL_ADDRESS BaseAddress; > UINT64 Length; > UINT64 Attributes; > @@ -2289,12 +2291,47 @@ CoreInitializeMemoryServices ( > // > // See if a Memory Type Information HOB is available > // > - GuidHob =3D GetFirstGuidHob (&gEfiMemoryTypeInformationGuid); > + MemoryTypeInformationResourceHob =3D NULL; > + GuidHob =3D GetFirstGuidHob (&gEfiMemoryTypeI= nformationGuid); > if (GuidHob !=3D NULL) { > EfiMemoryTypeInformation =3D GET_GUID_HOB_DATA (GuidHob); > DataSize =3D GET_GUID_HOB_DATA_SIZE (GuidHob); > if ((EfiMemoryTypeInformation !=3D NULL) && (DataSize > 0) && (DataS= ize <=3D (EfiMaxMemoryType + 1) * sizeof (EFI_MEMORY_TYPE_INFORMATION))) { > CopyMem (&gMemoryTypeInformation, EfiMemoryTypeInformation, DataSi= ze); > + > + // > + // Look for Resource Descriptor HOB with a ResourceType of System = Memory > + // and an Owner GUID of gEfiMemoryTypeInformationGuid. If more tha= n 1 is > + // found, then set MemoryTypeInformationResourceHob to NULL. > + // > + Count =3D 0; > + for (Hob.Raw =3D *HobStart; !END_OF_HOB_LIST (Hob); Hob.Raw =3D GE= T_NEXT_HOB (Hob)) { > + if (GET_HOB_TYPE (Hob) !=3D EFI_HOB_TYPE_RESOURCE_DESCRIPTOR) { > + continue; > + } > + > + ResourceHob =3D Hob.ResourceDescriptor; > + if (!CompareGuid (&ResourceHob->Owner, &gEfiMemoryTypeInformatio= nGuid)) { > + continue; > + } > + > + Count++; > + if (ResourceHob->ResourceType !=3D EFI_RESOURCE_SYSTEM_MEMORY) { > + continue; > + } > + > + if ((ResourceHob->ResourceAttribute & MEMORY_ATTRIBUTE_MASK) != =3D TESTED_MEMORY_ATTRIBUTES) { > + continue; > + } > + > + if (ResourceHob->ResourceLength >=3D CalculateTotalMemoryBinSize= Needed ()) { > + MemoryTypeInformationResourceHob =3D ResourceHob; > + } > + } > + > + if (Count > 1) { > + MemoryTypeInformationResourceHob =3D NULL; > + } > } > } > > @@ -2344,6 +2381,15 @@ CoreInitializeMemoryServices ( > PhitResourceHob =3D ResourceHob; > Found =3D TRUE; > > + // > + // If a Memory Type Information Resource HOB was found and is the sa= me > + // Resource HOB that describes the PHIT HOB, then ignore the Memory = Type > + // Information Resource HOB. > + // > + if (MemoryTypeInformationResourceHob =3D=3D PhitResourceHob) { > + MemoryTypeInformationResourceHob =3D NULL; > + } > + > // > // Compute range between PHIT EfiMemoryTop and the end of the Resour= ce Descriptor HOB > // > @@ -2387,8 +2433,9 @@ CoreInitializeMemoryServices ( > if (Length < MinimalMemorySizeNeeded) { > // > // Search all the resource descriptor HOBs from the highest possible= addresses down for a memory > - // region that is big enough to initialize the DXE core. Always ski= p the PHIT Resource HOB. > - // The max address must be within the physically addressible range f= or the processor. > + // region that is big enough to initialize the DXE core. Always ski= p the PHIT Resource HOB > + // and the Memory Type Information Resource HOB. The max address mus= t be within the physically > + // addressable range for the processor. > // > HighAddress =3D MAX_ALLOC_ADDRESS; > for (Hob.Raw =3D *HobStart; !END_OF_HOB_LIST (Hob); Hob.Raw =3D GET_= NEXT_HOB (Hob)) { > @@ -2399,6 +2446,13 @@ CoreInitializeMemoryServices ( > continue; > } > > + // > + // Skip the Resource Descriptor HOB that contains Memory Type Info= rmation bins > + // > + if (Hob.ResourceDescriptor =3D=3D MemoryTypeInformationResourceHob= ) { > + continue; > + } > + > // > // Skip all HOBs except Resource Descriptor HOBs > // > @@ -2466,6 +2520,18 @@ CoreInitializeMemoryServices ( > Capabilities =3D CoreConvertResourceDescriptorHobAttributesToCapabil= ities (EfiGcdMemoryTypeSystemMemory, Attributes); > } > > + if (MemoryTypeInformationResourceHob !=3D NULL) { > + // > + // If a Memory Type Information Resource HOB was found, then use the= address > + // range of the Memory Type Information Resource HOB as the preferr= ed > + // address range for the Memory Type Information bins. > + // > + CoreSetMemoryTypeInformationRange ( > + MemoryTypeInformationResourceHob->PhysicalStart, > + MemoryTypeInformationResourceHob->ResourceLength > + ); > + } > + > // > // Declare the very first memory region, so the EFI Memory Services ar= e available. > // Hmm, I think I have a coarse understanding of the control flow here. If we call CoreSetMemoryTypeInformationRange() here -- due to the new HOB being available --, then "mMemoryTypeInformationInitialized" will be set to TRUE. Then, the very first call to CoreAddMemoryDescriptor(), just below the context quoted above, will see "mMemoryTypeInformationInitialized" as TRUE, and avoid iterating through the memory type information array. OK. > diff --git a/MdeModulePkg/Core/Dxe/Mem/Page.c b/MdeModulePkg/Core/Dxe/Mem= /Page.c > index 6497af573353..458c62090265 100644 > --- a/MdeModulePkg/Core/Dxe/Mem/Page.c > +++ b/MdeModulePkg/Core/Dxe/Mem/Page.c > @@ -532,6 +532,107 @@ CoreLoadingFixedAddressHook ( > return; > } > > +/** > + Sets the preferred memory range to use for the Memory Type Information= bins. > + This service must be called before fist call to CoreAddMemoryDescripto= r(). > + > + If the location of the Memory Type Information bins has already been > + established or the size of the range provides is smaller than all the > + Memory Type Information bins, then the range provides is not used. > + > + @param Start The start address of the Memory Type Information range= . > + @param Length The size, in bytes, of the Memory Type Information ran= ge. > +**/ > +VOID > +CoreSetMemoryTypeInformationRange ( > + IN EFI_PHYSICAL_ADDRESS Start, > + IN UINT64 Length > + ) > +{ > + EFI_PHYSICAL_ADDRESS Top; > + EFI_MEMORY_TYPE Type; > + UINTN Index; > + UINTN Size; > + > + // > + // Return if Memory Type Information bin locations have already been s= et > + // > + if (mMemoryTypeInformationInitialized) { > + return; > + } (4) Under what circumstances can this condition evaluate to TRUE? Should we perhaps ASSERT (!mMemoryTypeInformationInitialized); instead? > + > + // > + // Return if size of the Memory Type Information bins is greater than = Length > + // > + Size =3D 0; > + for (Index =3D 0; gMemoryTypeInformation[Index].Type !=3D EfiMaxMemory= Type; Index++) { > + // > + // Make sure the memory type in the gMemoryTypeInformation[] array i= s valid > + // > + Type =3D (EFI_MEMORY_TYPE)(gMemoryTypeInformation[Index].Type); > + if ((UINT32)Type > EfiMaxMemoryType) { > + continue; > + } > + > + Size +=3D EFI_PAGES_TO_SIZE (gMemoryTypeInformation[Index].NumberOfP= ages); > + } > + > + if (Size > Length) { > + return; > + } > + > + // > + // Loop through each memory type in the order specified by the > + // gMemoryTypeInformation[] array > + // > + Top =3D Start + Length; > + for (Index =3D 0; gMemoryTypeInformation[Index].Type !=3D EfiMaxMemory= Type; Index++) { > + // > + // Make sure the memory type in the gMemoryTypeInformation[] array i= s valid > + // > + Type =3D (EFI_MEMORY_TYPE)(gMemoryTypeInformation[Index].Type); > + if ((UINT32)Type > EfiMaxMemoryType) { > + continue; > + } > + > + if (gMemoryTypeInformation[Index].NumberOfPages !=3D 0) { > + mMemoryTypeStatistics[Type].MaximumAddress =3D Top - 1; > + Top -=3D EFI_PAGES_TO_SIZE (= gMemoryTypeInformation[Index].NumberOfPages); > + mMemoryTypeStatistics[Type].BaseAddress =3D Top; > + > + // > + // If the current base address is the lowest address so far, then = update > + // the default maximum address > + // > + if (mMemoryTypeStatistics[Type].BaseAddress < mDefaultMaximumAddre= ss) { > + mDefaultMaximumAddress =3D mMemoryTypeStatistics[Type].BaseAddre= ss - 1; > + } > + > + mMemoryTypeStatistics[Type].NumberOfPages =3D gMemoryTypeInforma= tion[Index].NumberOfPages; > + gMemoryTypeInformation[Index].NumberOfPages =3D 0; > + } > + } > + > + // > + // If the number of pages reserved for a memory type is 0, then all > + // allocations for that type should be in the default range. > + // > + for (Type =3D (EFI_MEMORY_TYPE)0; Type < EfiMaxMemoryType; Type++) { > + for (Index =3D 0; gMemoryTypeInformation[Index].Type !=3D EfiMaxMemo= ryType; Index++) { > + if (Type =3D=3D (EFI_MEMORY_TYPE)gMemoryTypeInformation[Index].Typ= e) { > + mMemoryTypeStatistics[Type].InformationIndex =3D Index; > + } > + } > + > + mMemoryTypeStatistics[Type].CurrentNumberOfPages =3D 0; > + if (mMemoryTypeStatistics[Type].MaximumAddress =3D=3D MAX_ALLOC_ADDR= ESS) { > + mMemoryTypeStatistics[Type].MaximumAddress =3D mDefaultMaximumAddr= ess; > + } > + } > + > + mMemoryTypeInformationInitialized =3D TRUE; > +} > + > /** > Called to initialize the memory map and add descriptors to > the current descriptor list. This part resembles (and seems to substitute for) the tail of said very first CoreAddMemoryDescriptor() execution. There are some differences; here we don't rely on CoreAllocatePages(). I don't understand either piece of code enough for making a meaningful comparison. The general control flow separation seems sane, modulo my question (4) above. Thanks, Laszlo -=3D-=3D-=3D-=3D-=3D-=3D-=3D-=3D-=3D-=3D-=3D- Groups.io Links: You receive all messages sent to this group. View/Reply Online (#114300): https://edk2.groups.io/g/devel/message/114300 Mute This Topic: https://groups.io/mt/103918464/7686176 Group Owner: devel+owner@edk2.groups.io Unsubscribe: https://edk2.groups.io/g/devel/leave/12367111/7686176/19134562= 12/xyzzy [rebecca@openfw.io] -=3D-=3D-=3D-=3D-=3D-=3D-=3D-=3D-=3D-=3D-=3D-