From mboxrd@z Thu Jan 1 00:00:00 1970 Received: from us-smtp-delivery-124.mimecast.com (us-smtp-delivery-124.mimecast.com [216.145.221.124]) by mx.groups.io with SMTP id smtpd.web11.66381.1673251651125363795 for ; Mon, 09 Jan 2023 00:07:32 -0800 Authentication-Results: mx.groups.io; dkim=pass header.i=@redhat.com header.s=mimecast20190719 header.b=V8DChM06; spf=permerror, err=parse error for token &{10 18 spf1.redhat.com}: parse error for token &{10 18 spf2.redhat.com}: parse error for token &{10 18 spf3.redhat.com}: parse error for token &{10 18 spf4.redhat.com}: parse error for token &{10 18 spf5.redhat.com}: parse error for token &{10 18 spf6.redhat.com}: parse error for token &{10 18 spf7.redhat.com}: parse error for token &{10 18 service-now.com}: limit exceeded (domain: redhat.com, ip: 216.145.221.124, mailfrom: lersek@redhat.com) DKIM-Signature: v=1; a=rsa-sha256; c=relaxed/relaxed; d=redhat.com; s=mimecast20190719; t=1673251648; 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=LknUUYZM7fk7rNZvqBDgwc8MHKMATeXYUX1eHm5TXTw=; b=V8DChM06pM4E8Q74gInpeQV9tSBiDclOj2hxGcDpPMYs6dJQOFzwaPRiXgXXfl9ixLazuC 18QE/YZAStI85eN25ei63+vfOSX/Baq/Y4SD/TyO1Db5GL0o8jnL1NocsuHY5S4Gl6zEob TBZ2NbE1KvqboYlt0oC80xVl2hGEmTk= 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-2-WXl6T1iOMLOWcGV2xOjzQg-1; Mon, 09 Jan 2023 03:07:25 -0500 X-MC-Unique: WXl6T1iOMLOWcGV2xOjzQg-1 Received: from smtp.corp.redhat.com (int-mx07.intmail.prod.int.rdu2.redhat.com [10.11.54.7]) (using TLSv1.2 with cipher AECDH-AES256-SHA (256/256 bits)) (No client certificate requested) by mimecast-mx02.redhat.com (Postfix) with ESMTPS id BE3BE2A59567; Mon, 9 Jan 2023 08:07:24 +0000 (UTC) Received: from [10.39.192.220] (unknown [10.39.192.220]) by smtp.corp.redhat.com (Postfix) with ESMTPS id 3FF23140EBF4; Mon, 9 Jan 2023 08:07:23 +0000 (UTC) Message-ID: Date: Mon, 9 Jan 2023 09:07:21 +0100 MIME-Version: 1.0 Subject: Re: [edk2-devel] [PATCH 1/2] OvmfPkg/PlatformInitLib: update PlatformScanOrAdd64BitE820Ram documentation To: devel@edk2.groups.io, kraxel@redhat.com Cc: Ard Biesheuvel , Jordan Justen , Pawel Polawski , Oliver Steffen , Jiewen Yao References: <20230106140403.2889131-1-kraxel@redhat.com> <20230106140403.2889131-2-kraxel@redhat.com> From: "Laszlo Ersek" In-Reply-To: <20230106140403.2889131-2-kraxel@redhat.com> X-Scanned-By: MIMEDefang 3.1 on 10.11.54.7 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/6/23 15:04, Gerd Hoffmann wrote: > Documentation of PlatformScanOrAdd64BitE820Ram() ran out of sync > with the implementation. Fix that. > > Signed-off-by: Gerd Hoffmann > --- > OvmfPkg/Library/PlatformInitLib/MemDetect.c | 16 ++++++++++------ > 1 file changed, 10 insertions(+), 6 deletions(-) > > diff --git a/OvmfPkg/Library/PlatformInitLib/MemDetect.c b/OvmfPkg/Library/PlatformInitLib/MemDetect.c > index 0c4956852689..1255d6300fdd 100644 > --- a/OvmfPkg/Library/PlatformInitLib/MemDetect.c > +++ b/OvmfPkg/Library/PlatformInitLib/MemDetect.c > @@ -121,15 +121,19 @@ PlatformQemuUc32BaseInitialization ( > 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() > + @param[in] AddHighHob If True then PlatformScanOrAdd64BitE820Ram() > produces memory resource descriptor HOBs for RAM > entries that start at or above 4GB. > + It also produces HOBs for reserved entries. > > - 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. > + @param[out] LowMemory If Lowmemory is not NULL, then Lowmemory MaxAddress > + holds the amout of emory below 4G on output. > + (1) The specification in the right hand side column is not aligned with the other specs in the same column. (2) Typo in "emory" (3) Typo in "Lowmemory" (twice) (4) "Lowmemory MaxAddress holds the amount ..." is probably another typo, I don't understand it. > + @param[out] MaxAddress If MaxAddress is not NULL, then 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. > I've tried to review the function in its current state, but I don't understand the code either. Originally this function had two behaviors, reflected by its name as well ("Scan Or Add 64 Bit E820 Ram"), and its sole (NULL-able) parameter MaxAddress would switch between those two behaviors. The single "if" in the loop body, and the loop body in general was trivial -- and AddMemoryRangeHob() call on one branch, and a maximum search/comparison step on the other. Entry types other than EfiAcpiAddressRangeMemory were summarily ignored. Now the function does many more things, especially at the end of this series. It does things for EfiAcpiAddressRangeReserved, but only when AddHighHob is TRUE. It implements a maximum search for LowMemory as well. The function name "Scan Or Add 64 Bit E820 Ram" has become a misnomer. It's not just the function comment block that is out of date, but the function's name too. The function's initially simple structure can clealy not carry all its new tasks; I'm struggling to read the function definition. This is best shown by the multiple calls to the function in the code base, where we have a plethora of NULLs and TRUE/FALSE arguments, much obscuring the intended purpose of those calls. The reason I originally wrote the function the way I did is that it would run in PEI. Small memory allocations go into HOBs in PEI, and cannot be freed (see FreePool() in "MdePkg/Library/PeiMemoryAllocationLib/MemoryAllocationLib.c"). Page allocations work, but I deemed that overkill, and there would be only two calls to this function anyway. Therefore, looping through the fw_cfg file twice, even using Port IO (for example in a SEV guest) would not be a big deal, not to mention when DMA would be available (the common case). But that no longer holds. We have a bunch of calls now. So, I request that we please split this function up. There are two ways to do that I guess: (1) Perform an initial set of checks, for the existence & proper size, of the fw_cfg file. Allocate the necessary number of pages, download the file, before the first scan. Implement all the scans based on the downloaded file, with separate, open-coded loops at every current call site. After the last use, release the pages. (2) Alternatively, keep the current, outermost, checking and looping logic in the function, so that we not need dynamic memory just like before. However, the internals should be broken out, by taking a callback function pointer as parameter. The callback function would have two parameters: the E820 Entry just found, and a "VOID *Context" pointer. typedef VOID (* ACCUMULATE_E820_ENTRY) ( IN CONST EFI_E820_ENTRY64 *E820Entry, IN OUT VOID *Context ); [The above need not be EFIAPI; so that omission is not an oversight.] STATIC EFI_STATUS PlatformScanE820 ( IN ACCUMULATE_E820_ENTRY Accumulate, IN OUT VOID *Context ) { ... for (...) { ... Accumulate (&E820Entry, Context); ... } ... } Then the various call sites would pass in their own context pointers (pointing to each's own pre-initialized context structure or even just scalar variable), and compatible accumulator functions. Quite a bit more code, but much cleaner, IMO. The documentation would also be much-much simpler to get right. (Right now, I'm proposing that the callback function return VOID. Later, if it becomes necessary, the return type can be changed to EFI_STATUS, and then a failure from the callback could -- if necessary -- abort the outer loop, and make PlatformScanE820 return with a failure code early, as well. But right now that's not needed.) Thanks, Laszlo