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.110635.1673372528328306380 for ; Tue, 10 Jan 2023 09:42:08 -0800 Authentication-Results: mx.groups.io; dkim=pass header.i=@redhat.com header.s=mimecast20190719 header.b=iBoVUeaO; 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=1673372526; 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=oq9MoGY6amPqsRWvUynd4Qj4gqFlC7oOJMAP7WLwylw=; b=iBoVUeaO18QuYYqAC3uzwbxBxPiEo065RTEm+eyDJW8eImbSNuxl1edQG2xUutlPP2sWPn PwtyyOP+c6HHyDRQOn+GXCid/ZkddBNBGLLQeUA1dtW3A/vDOJGF63z6ISMe+VXU/Tkkto zj6aDNYaxXgWmzTqsuCpDyVwrUfHxEs= Received: from mimecast-mx02.redhat.com (mx3-rdu2.redhat.com [66.187.233.73]) by relay.mimecast.com with ESMTP with STARTTLS (version=TLSv1.2, cipher=TLS_ECDHE_RSA_WITH_AES_256_GCM_SHA384) id us-mta-299-ofq6-cHcM0OATq7GA8cmSA-1; Tue, 10 Jan 2023 12:42:03 -0500 X-MC-Unique: ofq6-cHcM0OATq7GA8cmSA-1 Received: from smtp.corp.redhat.com (int-mx03.intmail.prod.int.rdu2.redhat.com [10.11.54.3]) (using TLSv1.2 with cipher AECDH-AES256-SHA (256/256 bits)) (No client certificate requested) by mimecast-mx02.redhat.com (Postfix) with ESMTPS id 3F7E43C02B7B; Tue, 10 Jan 2023 17:42:03 +0000 (UTC) Received: from [10.39.192.22] (unknown [10.39.192.22]) by smtp.corp.redhat.com (Postfix) with ESMTPS id CE1F51121314; Tue, 10 Jan 2023 17:42:01 +0000 (UTC) Message-ID: Date: Tue, 10 Jan 2023 18:42:00 +0100 MIME-Version: 1.0 Subject: Re: [PATCH v2 3/4] OvmfPkg/PlatformInitLib: Add PlatformAddHobCB 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-4-kraxel@redhat.com> From: "Laszlo Ersek" In-Reply-To: <20230110082123.159521-4-kraxel@redhat.com> X-Scanned-By: MIMEDefang 3.1 on 10.11.54.3 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: > Add PlatformAddHobCB() callback function for use with > PlatformScanE820(). It adds HOBs for high memory and reservations (low > memory is handled elsewhere because there are some special cases to > consider). This replaces calls to PlatformScanOrAdd64BitE820Ram() with > AddHighHobs = TRUE. > > Also remove PlatformScanOrAdd64BitE820Ram() which is not used any more. > > Signed-off-by: Gerd Hoffmann > --- > OvmfPkg/Library/PlatformInitLib/MemDetect.c | 174 ++++---------------- > 1 file changed, 36 insertions(+), 138 deletions(-) > > diff --git a/OvmfPkg/Library/PlatformInitLib/MemDetect.c b/OvmfPkg/Library/PlatformInitLib/MemDetect.c > index 63329c4e796a..83a219581a1b 100644 > --- a/OvmfPkg/Library/PlatformInitLib/MemDetect.c > +++ b/OvmfPkg/Library/PlatformInitLib/MemDetect.c > @@ -112,143 +112,6 @@ PlatformQemuUc32BaseInitialization ( > } > } > > -/** > - Iterate over the RAM entries in QEMU's fw_cfg E820 RAM map that start outside > - of the 32-bit address range. > - > - Find the highest exclusive >=4GB RAM address, or produce memory resource > - descriptor HOBs for RAM entries that start at or above 4GB. > - > - @param[out] MaxAddress If MaxAddress is NULL, then PlatformScanOrAdd64BitE820Ram() > - produces memory resource descriptor HOBs for RAM > - entries that start at or above 4GB. > - > - Otherwise, MaxAddress holds the highest exclusive > - >=4GB RAM address on output. If QEMU's fw_cfg E820 > - RAM map contains no RAM entry that starts outside of > - the 32-bit address range, then MaxAddress is exactly > - 4GB on output. > - > - @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 > -PlatformScanOrAdd64BitE820Ram ( > - IN BOOLEAN AddHighHob, > - OUT UINT64 *LowMemory OPTIONAL, > - OUT UINT64 *MaxAddress OPTIONAL > - ) > -{ > - 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; > - } > - > - if (LowMemory != NULL) { > - *LowMemory = 0; > - } > - > - if (MaxAddress != NULL) { > - *MaxAddress = BASE_4GB; > - } > - > - QemuFwCfgSelectItem (FwCfgItem); > - for (Processed = 0; Processed < FwCfgSize; Processed += sizeof E820Entry) { > - QemuFwCfgReadBytes (sizeof E820Entry, &E820Entry); > - DEBUG (( > - DEBUG_VERBOSE, > - "%a: Base=0x%Lx Length=0x%Lx Type=%u\n", > - __FUNCTION__, > - E820Entry.BaseAddr, > - E820Entry.Length, > - E820Entry.Type > - )); > - if (E820Entry.Type == EfiAcpiAddressRangeMemory) { > - if (AddHighHob && (E820Entry.BaseAddr >= BASE_4GB)) { > - UINT64 Base; > - UINT64 End; > - > - // > - // Round up the start address, and round down the end address. > - // > - Base = ALIGN_VALUE (E820Entry.BaseAddr, (UINT64)EFI_PAGE_SIZE); > - End = (E820Entry.BaseAddr + E820Entry.Length) & > - ~(UINT64)EFI_PAGE_MASK; > - if (Base < End) { > - PlatformAddMemoryRangeHob (Base, End); > - DEBUG (( > - DEBUG_VERBOSE, > - "%a: PlatformAddMemoryRangeHob [0x%Lx, 0x%Lx)\n", > - __FUNCTION__, > - Base, > - End > - )); > - } > - } > - > - if (MaxAddress || LowMemory) { > - UINT64 Candidate; > - > - Candidate = E820Entry.BaseAddr + E820Entry.Length; > - if (MaxAddress && (Candidate > *MaxAddress)) { > - *MaxAddress = Candidate; > - DEBUG (( > - DEBUG_VERBOSE, > - "%a: MaxAddress=0x%Lx\n", > - __FUNCTION__, > - *MaxAddress > - )); > - } > - > - if (LowMemory && (Candidate > *LowMemory) && (Candidate < BASE_4GB)) { > - *LowMemory = Candidate; > - DEBUG (( > - DEBUG_VERBOSE, > - "%a: LowMemory=0x%Lx\n", > - __FUNCTION__, > - *LowMemory > - )); > - } > - } > - } else if (E820Entry.Type == EfiAcpiAddressRangeReserved) { > - if (AddHighHob) { > - DEBUG (( > - DEBUG_INFO, > - "%a: Reserved: Base=0x%Lx Length=0x%Lx\n", > - __FUNCTION__, > - E820Entry.BaseAddr, > - E820Entry.Length > - )); > - BuildResourceDescriptorHob ( > - EFI_RESOURCE_MEMORY_RESERVED, > - 0, > - E820Entry.BaseAddr, > - E820Entry.Length > - ); > - } > - } > - } > - > - return EFI_SUCCESS; > -} > - > typedef VOID (*E820_SCAN_CALLBACK) ( > EFI_E820_ENTRY64 *E820Entry, > EFI_HOB_PLATFORM_INFO *PlatformInfoHob > @@ -304,6 +167,41 @@ PlatformGetLowMemoryCB ( > } > } > > +/** > + Create HOBs for reservations and RAM (except low memory). > +**/ > +VOID > +PlatformAddHobCB ( > + IN EFI_E820_ENTRY64 *E820Entry, > + IN OUT EFI_HOB_PLATFORM_INFO *PlatformInfoHob > + ) > +{ > + UINT64 Base = E820Entry->BaseAddr; > + UINT64 End = E820Entry->BaseAddr + E820Entry->Length; (1) To my understanding, the edk2 coding style forbids initialization of variables with automatic storage duration ("non-static locals"). > + > + switch (E820Entry->Type) { > + case EfiAcpiAddressRangeMemory: (Side comment: I'm sure you've run this through Uncrustify -- so that's a coding style change then. What I remember is that "switch" and "case" were supposed to start in the same column.) > + // > + // Round up the start address, and round down the end address. > + // > + Base = ALIGN_VALUE (Base, (UINT64)EFI_PAGE_SIZE); > + End = End & ~(UINT64)EFI_PAGE_MASK; > + if ((Base >= BASE_4GB) && (Base < End)) { (2) This is not faithful conversion. The original code first compares the base against 4GB, then rounds it up; this variant reverses the order of those steps. I don't think if it will ever matter, but this is a refactoring, so let's stick with the original logic (not hard to do). > + DEBUG ((DEBUG_INFO, "%a: HighMemory [0x%Lx, 0x%Lx]\n", __FUNCTION__, Base, End)); (3) I've not noticed before, but I'm noticing now: before this series, the DEBUG levels (or more precisely, "debug masks") for the messages emitted by PlatformScanOrAdd64BitE820Ram() were inconsistent. Most of them were DEBUG_VERBOSE (per original intent), but then commit bf65d7ee8842 ("OvmfPkg/PlatformInitLib: pass through reservations from qemu", 2022-12-23) introduced a new branch with DEBUG_INFO. From the perspective of this series, that's a preexistent inconsistency. Is that worth fixing up at first, separately? (Changing it to DEBUG_VERBOSE.) (4) Also relating to logging. The current patch set seems to move all DEBUG masks here to DEBUG_INFO. The uniformity is welcome, but I'm unsure if DEBUG_INFO is justified. Writes to the QEMU debug console are slow; are we sure we want these logs emitted in a defult build of OVMF? (5) Still about logging. Before this series, the PlatformScanOrAdd64BitE820Ram() function would log each E820 entry before investigating them. (And, based on the investigation, further messages may be logged.) With the whole series applied however, as far as I can tell, we'll never get a complete dump of the E820 map, because PlatformScanE820() doesn't log entries at all, and the callbacks only log entries that prove "interesting" for them. Is this intentional? I think it may make debugging harder. I didn't notice it under patch#1, but I think we should add generic (unconditional) logging there. (6) *Yet more* logging-related observations. The original log message uses a bracket "[" on the left hand side of the logged intervals, and a parenthesis ")" on the RHS, to express that the RHS limit is exclusive: "%a: PlatformAddMemoryRangeHob [0x%Lx, 0x%Lx)\n", This detail is lost in this patch (in all three DEBUG messages); both sides use brackets. (7) Sorry, I'm getting tired and my observations are starting to fall apart. Anyway -- I think all the actual callback functions should be STATIC. (8) Furthermore, *perhaps* we should put E820 in their names somewhere (I don't insist at all), instead of the "Platform" prefix -- these functions are not public PlatformInitLib APIs. > + PlatformAddMemoryRangeHob (Base, End); > + } > + > + break; (9) Empty line intentional? > + case EfiAcpiAddressRangeReserved: > + BuildResourceDescriptorHob (EFI_RESOURCE_MEMORY_RESERVED, 0, Base, End); > + DEBUG ((DEBUG_INFO, "%a: Reserved [0x%Lx, 0x%Lx]\n", __FUNCTION__, Base, End)); > + break; > + default: > + DEBUG ((DEBUG_WARN, "%a: Type %d [0x%Lx, 0x%Lx] (NOT HANDLED)\n", __FUNCTION__, E820Entry->Type, Base, End)); (10) I meant to ask earlier -- what is now the maximum line length? I notice, in ".pytool/Plugin/UncrustifyCheck/uncrustify.cfg": > # Code width / line splitting > #code_width =120 # TODO: This causes non-deterministic behaviour in some cases when code wraps > ls_code_width =false Does that mean that 120 is considered a soft limit? (I used to ask for 80 chars under OvmfPkg, but there's no need to stick with that anymore.) (11) "E820Entry->Type" is an enumeration type; per UEFI 2.10, 2.3.1 Data Types, the UEFI build environment is supposed to make the compiler pick INT32 or UINT32. (This is from Mantis ticket 913.) Now, as long as QEMU sticks with the small set of enumeration constants we define in "OvmfPkg/Include/IndustryStandard/E820.h", it's all fine, and we can indeed print "E820Entry->Type" with both %d and %u, interchangeably. I used "%u" in the original logging because I find the printing of unexpected values as positive integers rather than negative ones more tolerable. I don't mind %d as long as we're conscious of it. Back to the patch: On 1/10/23 09:21, Gerd Hoffmann wrote: > + break; > + } > +} > + > /** > Iterate over the RAM entries in QEMU's fw_cfg E820 RAM map, call the > passed callback for each entry. > @@ -1048,7 +946,7 @@ PlatformQemuInitializeRam ( > // entries. Otherwise, create a single memory HOB with the flat >=4GB > // memory size read from the CMOS. > // > - Status = PlatformScanOrAdd64BitE820Ram (TRUE, NULL, NULL); > + Status = PlatformScanE820 (PlatformAddHobCB, PlatformInfoHob); > if (EFI_ERROR (Status)) { > UpperMemorySize = PlatformGetSystemMemorySizeAbove4gb (); > if (UpperMemorySize != 0) { Looks OK. Thanks, Laszlo