From mboxrd@z Thu Jan 1 00:00:00 1970 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.web10.106828.1673365302330735948 for ; Tue, 10 Jan 2023 07:41:42 -0800 Authentication-Results: mx.groups.io; dkim=pass header.i=@redhat.com header.s=mimecast20190719 header.b=MqUntv7G; spf=pass (domain: redhat.com, ip: 170.10.129.124, mailfrom: lersek@redhat.com) DKIM-Signature: v=1; a=rsa-sha256; c=relaxed/relaxed; d=redhat.com; s=mimecast20190719; t=1673365301; h=from:from:reply-to:subject:subject:date:date:message-id:message-id: to:to:cc:cc:mime-version:mime-version:content-type:content-type: content-transfer-encoding:content-transfer-encoding: in-reply-to:in-reply-to:references:references; bh=2zVYUSr4XNkxChseQRjVFO0N+bnFqXJO+Pn/V01+Av4=; b=MqUntv7GhzoEXNGyq0S077AN1FE/84zZbLJhN+pi+y8MrmdSdoCWAexU065/Zwz1ymt3qm 6SSoZj4kf6n2a9afyI/0yj5Tl9biJHYCjenYdbO9zjsImRNf9maPUvNth7O4k/i4BR79w2 1lI8UflFpWYHz+Rz7nYOradvN5HyZNk= Received: from mimecast-mx02.redhat.com (mimecast-mx02.redhat.com [66.187.233.88]) by relay.mimecast.com with ESMTP with STARTTLS (version=TLSv1.2, cipher=TLS_ECDHE_RSA_WITH_AES_256_GCM_SHA384) id us-mta-191-RrOQIpAlP-ySgo0SxFh9NA-1; Tue, 10 Jan 2023 10:41:37 -0500 X-MC-Unique: RrOQIpAlP-ySgo0SxFh9NA-1 Received: from smtp.corp.redhat.com (int-mx09.intmail.prod.int.rdu2.redhat.com [10.11.54.9]) (using TLSv1.2 with cipher AECDH-AES256-SHA (256/256 bits)) (No client certificate requested) by mimecast-mx02.redhat.com (Postfix) with ESMTPS id 6C16B8030A0; Tue, 10 Jan 2023 15:41:37 +0000 (UTC) Received: from [10.39.192.22] (unknown [10.39.192.22]) by smtp.corp.redhat.com (Postfix) with ESMTPS id 0F8C3492C18; Tue, 10 Jan 2023 15:41:35 +0000 (UTC) Message-ID: Date: Tue, 10 Jan 2023 16:41:34 +0100 MIME-Version: 1.0 Subject: Re: [PATCH v2 1/4] OvmfPkg/PlatformInitLib: Add PlatformScanE820 and GetFirstNonAddressCB To: Gerd Hoffmann , devel@edk2.groups.io Cc: Pawel Polawski , Jiewen Yao , Oliver Steffen , Jordan Justen , Ard Biesheuvel References: <20230110082123.159521-1-kraxel@redhat.com> <20230110082123.159521-2-kraxel@redhat.com> From: "Laszlo Ersek" In-Reply-To: <20230110082123.159521-2-kraxel@redhat.com> X-Scanned-By: MIMEDefang 3.1 on 10.11.54.9 X-Mimecast-Spam-Score: 0 X-Mimecast-Originator: redhat.com Content-Language: en-US Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 7bit On 1/10/23 09:21, 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. > > Also drop local FirstNonAddress variables and use > PlatformInfoHob->FirstNonAddress instead everywhere. > > Signed-off-by: Gerd Hoffmann > --- > OvmfPkg/Library/PlatformInitLib/MemDetect.c | 114 ++++++++++++++++---- > 1 file changed, 91 insertions(+), 23 deletions(-) > > diff --git a/OvmfPkg/Library/PlatformInitLib/MemDetect.c b/OvmfPkg/Library/PlatformInitLib/MemDetect.c > index 0c4956852689..a2a4dc043f2e 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 RAM 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. > +**/ (1) I suggest removing the "RAM" references from the above, now that the type checking is being moved to the callbacks. I think we can just say "entries" and "entry", without "RAM". Not a huge deal though. > +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,9 @@ 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); > + Status = PlatformScanE820 (PlatformGetFirstNonAddressCB, PlatformInfoHob); (2) There is one omission in this patch. Namely, we're performing a conditional maximum search, and because it is conditional, it is possible that we find zero candidates. In that case, we still need to have "PlatformInfoHob->FirstNonAddress" set to a sane value. The original code handles this by initializing FirstNonAddress to 4GB: if (MaxAddress != NULL) { *MaxAddress = BASE_4GB; } Which is consistent with the comments that *remain* in the code now. So my request is that, right before this call to PlatformScanE820(), please set PlatformInfoHob->FirstNonAddress = BASE_4GB; With (2) updated, and (1) updated or not: Reviewed-by: Laszlo Ersek Thanks, Laszlo > if (EFI_ERROR (Status)) { > - FirstNonAddress = BASE_4GB + PlatformGetSystemMemorySizeAbove4gb (); > + PlatformInfoHob->FirstNonAddress = BASE_4GB + PlatformGetSystemMemorySizeAbove4gb (); > } > > // > @@ -420,7 +491,7 @@ PlatformGetFirstNonAddress ( > // > #ifdef MDE_CPU_IA32 > if (!FeaturePcdGet (PcdDxeIplSwitchToLongMode)) { > - return FirstNonAddress; > + return; > } > > #endif > @@ -473,7 +544,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 +568,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 +590,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 +852,6 @@ PlatformAddressWidthInitialization ( > IN OUT EFI_HOB_PLATFORM_INFO *PlatformInfoHob > ) > { > - UINT64 FirstNonAddress; > UINT8 PhysMemAddressWidth; > EFI_STATUS Status; > > @@ -794,7 +864,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 +876,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 +892,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 +926,6 @@ PlatformAddressWidthInitialization ( > ASSERT (PhysMemAddressWidth <= 48); > #endif > > - PlatformInfoHob->FirstNonAddress = FirstNonAddress; > PlatformInfoHob->PhysMemAddressWidth = PhysMemAddressWidth; > } >