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.133.124]) by mx.groups.io with SMTP id smtpd.web11.198679.1673967833157747249 for ; Tue, 17 Jan 2023 07:03:53 -0800 Authentication-Results: mx.groups.io; dkim=pass header.i=@redhat.com header.s=mimecast20190719 header.b=aF6KNiYk; spf=pass (domain: redhat.com, ip: 170.10.133.124, mailfrom: lersek@redhat.com) DKIM-Signature: v=1; a=rsa-sha256; c=relaxed/relaxed; d=redhat.com; s=mimecast20190719; t=1673967832; 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=xDlnyVOmpDwp25XX3mlygAojpWeMMKM5N5hxLDdRELE=; b=aF6KNiYknJIdQJEhybXVPsjtD4X8OJ5SsYaVB93sHq6CpulXeQEILElCD79kLk4VeU9UQd WOaZGtb7aOHlXyHqtU6z1LUsCM9L6c8cZJRAwhGozUfU3d/dXmHbGPbCKRNZHwmvPt79mL fsVCR2UBIO2gnUBRCVMgNbMsFtfwjuk= 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-354-tb4WI3YwNlyn5rEP0SEHBQ-1; Tue, 17 Jan 2023 10:02:39 -0500 X-MC-Unique: tb4WI3YwNlyn5rEP0SEHBQ-1 Received: from smtp.corp.redhat.com (int-mx01.intmail.prod.int.rdu2.redhat.com [10.11.54.1]) (using TLSv1.2 with cipher AECDH-AES256-SHA (256/256 bits)) (No client certificate requested) by mimecast-mx02.redhat.com (Postfix) with ESMTPS id 3634D2823830; Tue, 17 Jan 2023 15:00:27 +0000 (UTC) Received: from [10.39.194.135] (unknown [10.39.194.135]) by smtp.corp.redhat.com (Postfix) with ESMTPS id 75BF540C2064; Tue, 17 Jan 2023 15:00:25 +0000 (UTC) Message-ID: <64b79c1e-dc09-75d1-3345-4620ffe63629@redhat.com> Date: Tue, 17 Jan 2023 16:00:23 +0100 MIME-Version: 1.0 Subject: Re: [PATCH v4 3/5] OvmfPkg/PlatformInitLib: Add PlatformAddHobCB To: Gerd Hoffmann , devel@edk2.groups.io Cc: Jiewen Yao , Oliver Steffen , Ard Biesheuvel , Pawel Polawski , Jordan Justen References: <20230117121629.2149112-1-kraxel@redhat.com> <20230117121629.2149112-4-kraxel@redhat.com> From: "Laszlo Ersek" In-Reply-To: <20230117121629.2149112-4-kraxel@redhat.com> X-Scanned-By: MIMEDefang 3.1 on 10.11.54.1 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/17/23 13:16, 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. > > Write any actions done (adding HOBs, skip unknown types) to the firmware > log with INFO loglevel. > > Also remove PlatformScanOrAdd64BitE820Ram() which is not used any more. > > Signed-off-by: Gerd Hoffmann > --- > OvmfPkg/Library/PlatformInitLib/MemDetect.c | 185 +++++--------------- > 1 file changed, 47 insertions(+), 138 deletions(-) > > diff --git a/OvmfPkg/Library/PlatformInitLib/MemDetect.c b/OvmfPkg/Library/PlatformInitLib/MemDetect.c > index dfaa05a1c24f..c626fc49cf6c 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,52 @@ PlatformGetLowMemoryCB ( > } > } > > +/** > + Create HOBs for reservations and RAM (except low memory). > +**/ > +STATIC VOID > +PlatformAddHobCB ( > + IN EFI_E820_ENTRY64 *E820Entry, > + IN OUT EFI_HOB_PLATFORM_INFO *PlatformInfoHob > + ) > +{ > + UINT64 Base, End; > + > + Base = E820Entry->BaseAddr; > + End = E820Entry->BaseAddr + E820Entry->Length; > + > + switch (E820Entry->Type) { > + case EfiAcpiAddressRangeMemory: > + if (Base >= BASE_4GB) { > + // > + // 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 < End) { > + DEBUG ((DEBUG_INFO, "%a: HighMemory [0x%Lx, 0x%Lx)\n", __FUNCTION__, Base, End-1)); (1) Hehe, this is a bit funny: we now have *both* the round closing parenthesies ")" in the debug message, for the interval, *and* (End-1) as the argument! :) We need exactly one of those. Either-or. :) If the argument is exclusive ("End"), then the interval should be closed with ")". If the arg is inclusive ("End-1"), then we need a bracket "]". > + PlatformAddMemoryRangeHob (Base, End); > + } > + } > + (2) Still not sure if this empty line is intentional (it may be, you didn't answer it under the v2 review AFAICT). > + break; > + case EfiAcpiAddressRangeReserved: > + BuildResourceDescriptorHob (EFI_RESOURCE_MEMORY_RESERVED, 0, Base, End); > + DEBUG ((DEBUG_INFO, "%a: Reserved [0x%Lx, 0x%Lx)\n", __FUNCTION__, Base, End-1)); (3) Same comment as (1) -- please pick either End-1 as arg, with "]", or ")" as interval closing character, with End as arg. > + break; > + default: > + DEBUG (( > + DEBUG_WARN, > + "%a: Type %u [0x%Lx, 0x%Lx) (NOT HANDLED)\n", > + __FUNCTION__, > + E820Entry->Type, > + Base, > + End-1 > + )); > + break; > + } (4) Same as (1). > +} > + > /** > Iterate over the entries in QEMU's fw_cfg E820 RAM map, call the > passed callback for each entry. > @@ -1049,7 +958,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) { Update as you see fit, either way: Reviewed-by: Laszlo Ersek