From mboxrd@z Thu Jan 1 00:00:00 1970 Received: from ams.source.kernel.org (ams.source.kernel.org [145.40.68.75]) by mx.groups.io with SMTP id smtpd.web11.198734.1673967987881294293 for ; Tue, 17 Jan 2023 07:06:28 -0800 Authentication-Results: mx.groups.io; dkim=pass header.i=@kernel.org header.s=k20201202 header.b=tO9b1+zi; spf=pass (domain: kernel.org, ip: 145.40.68.75, mailfrom: ardb@kernel.org) Received: from smtp.kernel.org (relay.kernel.org [52.25.139.140]) (using TLSv1.2 with cipher ECDHE-RSA-AES256-GCM-SHA384 (256/256 bits)) (No client certificate requested) by ams.source.kernel.org (Postfix) with ESMTPS id 0AB01B8164E for ; Tue, 17 Jan 2023 15:06:26 +0000 (UTC) Received: by smtp.kernel.org (Postfix) with ESMTPSA id A89B0C433D2 for ; Tue, 17 Jan 2023 15:06:24 +0000 (UTC) DKIM-Signature: v=1; a=rsa-sha256; c=relaxed/simple; d=kernel.org; s=k20201202; t=1673967984; bh=ZnVo1hcMZLyIUi9KxkCDtuCEvSpaCe0e1uYSIT1JaWc=; h=References:In-Reply-To:From:Date:Subject:To:Cc:From; b=tO9b1+zitWenbSpx206eZSeuZAl3U2+COZR2xCzMlhCT0JczDiuTcJORHk8Q4CiFJ 2hGShRn7bf1QWWd1k6M7R9pryjKzo0GqN0XARwOShlYnY0usJoKn2Yg1Okq54zClQe t3eRlJSYLGRBA+vvu0MrJiqF0wsFVon+9G39vg6iQzEsUvSgjhPi9KKjOJWb5lnQtV lOUxjyrek5Osb/2LXxqc49QOvWFPkRIQTGZyNaHwAvsvuYGru9gIrIRAD4KcfCZj4U SVNvS5Scw38TtopRX2bv2vFnn15JIlr1x7wvRecK1OLmiU2VuhQdsWimI4NKXWevBm jPw9qDBB+Fnyg== Received: by mail-lj1-f181.google.com with SMTP id y18so29844675ljk.11 for ; Tue, 17 Jan 2023 07:06:24 -0800 (PST) X-Gm-Message-State: AFqh2kqpXSCpzGKUt83NsdD7dYbPWZpzTJDixoILtzUGMwNxXrrfJIR/ Mxw7mESJqR6H3V5i3dEC0CUaHRuqqH57fd0harM= X-Google-Smtp-Source: AMrXdXuI4KvIgwBcvlTiONI+sMVt0L+GwyQ3R/JJ0sQS3MSDWsWXQc0sX1OgiDeJi+wZJRnljnFCHz8nAVMcTF4IL9k= X-Received: by 2002:a05:651c:b1f:b0:28b:814d:7087 with SMTP id b31-20020a05651c0b1f00b0028b814d7087mr318506ljr.516.1673967982602; Tue, 17 Jan 2023 07:06:22 -0800 (PST) MIME-Version: 1.0 References: <20230117121629.2149112-1-kraxel@redhat.com> <20230117121629.2149112-4-kraxel@redhat.com> <64b79c1e-dc09-75d1-3345-4620ffe63629@redhat.com> In-Reply-To: <64b79c1e-dc09-75d1-3345-4620ffe63629@redhat.com> From: "Ard Biesheuvel" Date: Tue, 17 Jan 2023 16:06:11 +0100 X-Gmail-Original-Message-ID: Message-ID: Subject: Re: [PATCH v4 3/5] OvmfPkg/PlatformInitLib: Add PlatformAddHobCB To: Laszlo Ersek Cc: Gerd Hoffmann , devel@edk2.groups.io, Jiewen Yao , Oliver Steffen , Ard Biesheuvel , Pawel Polawski , Jordan Justen Content-Type: text/plain; charset="UTF-8" On Tue, 17 Jan 2023 at 16:03, Laszlo Ersek wrote: > > 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 > Thanks Laszlo - I'll fix up the above locally.