From: "Laszlo Ersek" <lersek@redhat.com>
To: Gerd Hoffmann <kraxel@redhat.com>, devel@edk2.groups.io
Cc: Jiewen Yao <jiewen.yao@intel.com>,
Oliver Steffen <osteffen@redhat.com>,
Ard Biesheuvel <ardb+tianocore@kernel.org>,
Pawel Polawski <ppolawsk@redhat.com>,
Jordan Justen <jordan.l.justen@intel.com>
Subject: Re: [PATCH v4 1/5] OvmfPkg/PlatformInitLib: Add PlatformScanE820 and GetFirstNonAddressCB
Date: Tue, 17 Jan 2023 15:30:42 +0100 [thread overview]
Message-ID: <dd67f128-5461-dbd4-e352-818ba2a84496@redhat.com> (raw)
In-Reply-To: <20230117121629.2149112-2-kraxel@redhat.com>
On 1/17/23 13:16, Gerd Hoffmann wrote:
> First step replacing the PlatformScanOrAdd64BitE820Ram() function.
>
> Add a PlatformScanE820() function which loops over the e280 entries
> from FwCfg and calls a callback for each of them.
>
> Add a GetFirstNonAddressCB() function which will store the first free
> address (right after the last RAM block) in
> PlatformInfoHob->FirstNonAddress. This replaces calls to
> PlatformScanOrAdd64BitE820Ram() with non-NULL MaxAddress.
>
> Write any actions done (setting FirstNonAddress) to the firmware log
> with INFO loglevel.
>
> Also drop local FirstNonAddress variables and use
> PlatformInfoHob->FirstNonAddress instead everywhere.
>
> Signed-off-by: Gerd Hoffmann <kraxel@redhat.com>
> ---
> OvmfPkg/Library/PlatformInitLib/MemDetect.c | 115 ++++++++++++++++----
> 1 file changed, 92 insertions(+), 23 deletions(-)
>
> diff --git a/OvmfPkg/Library/PlatformInitLib/MemDetect.c b/OvmfPkg/Library/PlatformInitLib/MemDetect.c
> index 882805269b3e..1ea91f78cefd 100644
> --- a/OvmfPkg/Library/PlatformInitLib/MemDetect.c
> +++ b/OvmfPkg/Library/PlatformInitLib/MemDetect.c
> @@ -251,6 +251,83 @@ PlatformScanOrAdd64BitE820Ram (
> return EFI_SUCCESS;
> }
>
> +typedef VOID (*E820_SCAN_CALLBACK) (
> + EFI_E820_ENTRY64 *E820Entry,
> + EFI_HOB_PLATFORM_INFO *PlatformInfoHob
> + );
> +
> +/**
> + Store first address not used by e820 RAM entries in
> + PlatformInfoHob->FirstNonAddress
> +**/
> +VOID
> +PlatformGetFirstNonAddressCB (
> + IN EFI_E820_ENTRY64 *E820Entry,
> + IN OUT EFI_HOB_PLATFORM_INFO *PlatformInfoHob
> + )
> +{
> + UINT64 Candidate;
> +
> + if (E820Entry->Type != EfiAcpiAddressRangeMemory) {
> + return;
> + }
> +
> + Candidate = E820Entry->BaseAddr + E820Entry->Length;
> + if (PlatformInfoHob->FirstNonAddress < Candidate) {
> + DEBUG ((DEBUG_INFO, "%a: FirstNonAddress=0x%Lx\n", __FUNCTION__, Candidate));
> + PlatformInfoHob->FirstNonAddress = Candidate;
> + }
> +}
> +
> +/**
> + Iterate over the entries in QEMU's fw_cfg E820 RAM map, call the
> + passed callback for each entry.
> +
> + @param[in] Callback The callback function to be called.
> +
> + @param[in out] PlatformInfoHob PlatformInfo struct which is passed
> + through to the callback.
> +
> + @retval EFI_SUCCESS The fw_cfg E820 RAM map was found and processed.
> +
> + @retval EFI_PROTOCOL_ERROR The RAM map was found, but its size wasn't a
> + whole multiple of sizeof(EFI_E820_ENTRY64). No
> + RAM entry was processed.
> +
> + @return Error codes from QemuFwCfgFindFile(). No RAM
> + entry was processed.
> +**/
> +STATIC
> +EFI_STATUS
> +PlatformScanE820 (
> + IN E820_SCAN_CALLBACK Callback,
> + IN OUT EFI_HOB_PLATFORM_INFO *PlatformInfoHob
> + )
> +{
> + EFI_STATUS Status;
> + FIRMWARE_CONFIG_ITEM FwCfgItem;
> + UINTN FwCfgSize;
> + EFI_E820_ENTRY64 E820Entry;
> + UINTN Processed;
> +
> + Status = QemuFwCfgFindFile ("etc/e820", &FwCfgItem, &FwCfgSize);
> + if (EFI_ERROR (Status)) {
> + return Status;
> + }
> +
> + if (FwCfgSize % sizeof E820Entry != 0) {
> + return EFI_PROTOCOL_ERROR;
> + }
> +
> + QemuFwCfgSelectItem (FwCfgItem);
> + for (Processed = 0; Processed < FwCfgSize; Processed += sizeof E820Entry) {
> + QemuFwCfgReadBytes (sizeof E820Entry, &E820Entry);
> + Callback (&E820Entry, PlatformInfoHob);
> + }
> +
> + return EFI_SUCCESS;
> +}
> +
> /**
> Returns PVH memmap
>
> @@ -384,23 +461,17 @@ PlatformGetSystemMemorySizeAbove4gb (
> Return the highest address that DXE could possibly use, plus one.
> **/
> STATIC
> -UINT64
> +VOID
> PlatformGetFirstNonAddress (
> IN OUT EFI_HOB_PLATFORM_INFO *PlatformInfoHob
> )
> {
> - UINT64 FirstNonAddress;
> UINT32 FwCfgPciMmio64Mb;
> EFI_STATUS Status;
> FIRMWARE_CONFIG_ITEM FwCfgItem;
> UINTN FwCfgSize;
> UINT64 HotPlugMemoryEnd;
>
> - //
> - // set FirstNonAddress to suppress incorrect compiler/analyzer warnings
> - //
> - FirstNonAddress = 0;
> -
> //
> // If QEMU presents an E820 map, then get the highest exclusive >=4GB RAM
> // address from it. This can express an address >= 4GB+1TB.
> @@ -408,9 +479,10 @@ PlatformGetFirstNonAddress (
> // Otherwise, get the flat size of the memory above 4GB from the CMOS (which
> // can only express a size smaller than 1TB), and add it to 4GB.
> //
> - Status = PlatformScanOrAdd64BitE820Ram (FALSE, NULL, &FirstNonAddress);
> + PlatformInfoHob->FirstNonAddress = BASE_4GB;
> + Status = PlatformScanE820 (PlatformGetFirstNonAddressCB, PlatformInfoHob);
> if (EFI_ERROR (Status)) {
> - FirstNonAddress = BASE_4GB + PlatformGetSystemMemorySizeAbove4gb ();
> + PlatformInfoHob->FirstNonAddress = BASE_4GB + PlatformGetSystemMemorySizeAbove4gb ();
> }
>
> //
> @@ -420,7 +492,7 @@ PlatformGetFirstNonAddress (
> //
> #ifdef MDE_CPU_IA32
> if (!FeaturePcdGet (PcdDxeIplSwitchToLongMode)) {
> - return FirstNonAddress;
> + return;
> }
>
> #endif
> @@ -473,7 +545,7 @@ PlatformGetFirstNonAddress (
> // determines the highest address plus one. The memory hotplug area (see
> // below) plays no role for the firmware in this case.
> //
> - return FirstNonAddress;
> + return;
> }
>
> //
> @@ -497,15 +569,15 @@ PlatformGetFirstNonAddress (
> HotPlugMemoryEnd
> ));
>
> - ASSERT (HotPlugMemoryEnd >= FirstNonAddress);
> - FirstNonAddress = HotPlugMemoryEnd;
> + ASSERT (HotPlugMemoryEnd >= PlatformInfoHob->FirstNonAddress);
> + PlatformInfoHob->FirstNonAddress = HotPlugMemoryEnd;
> }
>
> //
> // SeaBIOS aligns both boundaries of the 64-bit PCI host aperture to 1GB, so
> // that the host can map it with 1GB hugepages. Follow suit.
> //
> - PlatformInfoHob->PcdPciMmio64Base = ALIGN_VALUE (FirstNonAddress, (UINT64)SIZE_1GB);
> + PlatformInfoHob->PcdPciMmio64Base = ALIGN_VALUE (PlatformInfoHob->FirstNonAddress, (UINT64)SIZE_1GB);
> PlatformInfoHob->PcdPciMmio64Size = ALIGN_VALUE (PlatformInfoHob->PcdPciMmio64Size, (UINT64)SIZE_1GB);
>
> //
> @@ -519,8 +591,8 @@ PlatformGetFirstNonAddress (
> //
> // The useful address space ends with the 64-bit PCI host aperture.
> //
> - FirstNonAddress = PlatformInfoHob->PcdPciMmio64Base + PlatformInfoHob->PcdPciMmio64Size;
> - return FirstNonAddress;
> + PlatformInfoHob->FirstNonAddress = PlatformInfoHob->PcdPciMmio64Base + PlatformInfoHob->PcdPciMmio64Size;
> + return;
> }
>
> /*
> @@ -781,7 +853,6 @@ PlatformAddressWidthInitialization (
> IN OUT EFI_HOB_PLATFORM_INFO *PlatformInfoHob
> )
> {
> - UINT64 FirstNonAddress;
> UINT8 PhysMemAddressWidth;
> EFI_STATUS Status;
>
> @@ -794,7 +865,7 @@ PlatformAddressWidthInitialization (
> // First scan host-provided hardware information to assess if the address
> // space is already known. If so, guest must use those values.
> //
> - Status = PlatformScanHostProvided64BitPciMmioEnd (&FirstNonAddress);
> + Status = PlatformScanHostProvided64BitPciMmioEnd (&PlatformInfoHob->FirstNonAddress);
>
> if (EFI_ERROR (Status)) {
> //
> @@ -806,13 +877,12 @@ PlatformAddressWidthInitialization (
> // The DXL IPL keys off of the physical address bits advertized in the CPU
> // HOB. To conserve memory, we calculate the minimum address width here.
> //
> - FirstNonAddress = PlatformGetFirstNonAddress (PlatformInfoHob);
> + PlatformGetFirstNonAddress (PlatformInfoHob);
> }
>
> PlatformAddressWidthFromCpuid (PlatformInfoHob, TRUE);
> if (PlatformInfoHob->PhysMemAddressWidth != 0) {
> // physical address width is known
> - PlatformInfoHob->FirstNonAddress = FirstNonAddress;
> PlatformDynamicMmioWindow (PlatformInfoHob);
> return;
> }
> @@ -823,13 +893,13 @@ PlatformAddressWidthInitialization (
> // -> try be conservstibe to stay below the guaranteed minimum of
> // 36 phys bits (aka 64 GB).
> //
> - PhysMemAddressWidth = (UINT8)HighBitSet64 (FirstNonAddress);
> + PhysMemAddressWidth = (UINT8)HighBitSet64 (PlatformInfoHob->FirstNonAddress);
>
> //
> // If FirstNonAddress is not an integral power of two, then we need an
> // additional bit.
> //
> - if ((FirstNonAddress & (FirstNonAddress - 1)) != 0) {
> + if ((PlatformInfoHob->FirstNonAddress & (PlatformInfoHob->FirstNonAddress - 1)) != 0) {
> ++PhysMemAddressWidth;
> }
>
> @@ -857,7 +927,6 @@ PlatformAddressWidthInitialization (
> ASSERT (PhysMemAddressWidth <= 48);
> #endif
>
> - PlatformInfoHob->FirstNonAddress = FirstNonAddress;
> PlatformInfoHob->PhysMemAddressWidth = PhysMemAddressWidth;
> }
>
(I'm no longer subscribed to the list, but I got a direct copy of this,
and I decided to look at it.)
Reviewed-by: Laszlo Ersek <lersek@redhat.com>
next prev parent reply other threads:[~2023-01-17 14:31 UTC|newest]
Thread overview: 22+ messages / expand[flat|nested] mbox.gz Atom feed top
2023-01-17 12:16 [PATCH v4 0/5] OvmfPkg: check 64bit mmio window for resource conflicts Gerd Hoffmann
2023-01-17 12:16 ` [PATCH v4 1/5] OvmfPkg/PlatformInitLib: Add PlatformScanE820 and GetFirstNonAddressCB Gerd Hoffmann
2023-01-17 14:30 ` Laszlo Ersek [this message]
2023-01-17 14:46 ` Laszlo Ersek
2023-01-17 14:48 ` [edk2-devel] " Ard Biesheuvel
2023-01-17 12:16 ` [PATCH v4 2/5] OvmfPkg/PlatformInitLib: Add PlatformGetLowMemoryCB Gerd Hoffmann
2023-01-17 14:45 ` Laszlo Ersek
2023-01-17 12:16 ` [PATCH v4 3/5] OvmfPkg/PlatformInitLib: Add PlatformAddHobCB Gerd Hoffmann
2023-01-17 15:00 ` Laszlo Ersek
2023-01-17 15:06 ` Ard Biesheuvel
2023-01-17 16:04 ` Ard Biesheuvel
2023-01-24 22:33 ` [edk2-devel] " Lendacky, Thomas
2023-01-25 9:11 ` Gerd Hoffmann
2023-01-25 15:35 ` Lendacky, Thomas
2023-01-25 16:32 ` Lendacky, Thomas
2023-01-30 15:23 ` Laszlo Ersek
2023-01-30 15:24 ` Laszlo Ersek
2023-01-17 12:16 ` [PATCH v4 4/5] OvmfPkg/PlatformInitLib: Add PlatformReservationConflictCB Gerd Hoffmann
2023-01-17 15:05 ` Laszlo Ersek
2023-01-17 12:16 ` [PATCH v4 5/5] OvmfPkg/PlatformInitLib: reorder PlatformQemuUc32BaseInitialization Gerd Hoffmann
2023-01-17 14:49 ` Laszlo Ersek
2023-01-17 16:38 ` [edk2-devel] [PATCH v4 0/5] OvmfPkg: check 64bit mmio window for resource conflicts Ard Biesheuvel
Reply instructions:
You may reply publicly to this message via plain-text email
using any one of the following methods:
* Save the following mbox file, import it into your mail client,
and reply-to-list from there: mbox
Avoid top-posting and favor interleaved quoting:
https://en.wikipedia.org/wiki/Posting_style#Interleaved_style
* Reply using the --to, --cc, and --in-reply-to
switches of git-send-email(1):
git send-email \
--in-reply-to=dd67f128-5461-dbd4-e352-818ba2a84496@redhat.com \
--to=devel@edk2.groups.io \
/path/to/YOUR_REPLY
https://kernel.org/pub/software/scm/git/docs/git-send-email.html
* If your mail client supports setting the In-Reply-To header
via mailto: links, try the mailto: link
Be sure your reply has a Subject: header at the top and a blank line
before the message body.
This is a public inbox, see mirroring instructions
for how to clone and mirror all data and code used for this inbox