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.68040.1673259313266077363 for ; Mon, 09 Jan 2023 02:15:13 -0800 Authentication-Results: mx.groups.io; dkim=pass header.i=@redhat.com header.s=mimecast20190719 header.b=Na4d9/55; spf=pass (domain: redhat.com, ip: 170.10.129.124, mailfrom: kraxel@redhat.com) DKIM-Signature: v=1; a=rsa-sha256; c=relaxed/relaxed; d=redhat.com; s=mimecast20190719; t=1673259311; 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: in-reply-to:in-reply-to:references:references; bh=PAg8mufBXfbLGfW7kwo46roXNH9oOnzdJKWKaYrY9jk=; b=Na4d9/55FabYyBj1CC9C+isgRz1HO4kMXioa6A+x0mNQZBNvym+5vsIGMabjj128izgsnf eQsVJXQOpszYavIbCVwQ3335hVtlYaQ/v+Dd1pE19f/li+a4wvij58GFiME9BxLA8OGi/Q 3uud0N+nzjzfKscVp2ca/w9CCCzUz6A= 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-351-aNM34Lb1N7eeswjXDcw0jQ-1; Mon, 09 Jan 2023 05:15:08 -0500 X-MC-Unique: aNM34Lb1N7eeswjXDcw0jQ-1 Received: from smtp.corp.redhat.com (int-mx02.intmail.prod.int.rdu2.redhat.com [10.11.54.2]) (using TLSv1.2 with cipher AECDH-AES256-SHA (256/256 bits)) (No client certificate requested) by mimecast-mx02.redhat.com (Postfix) with ESMTPS id 6761385C6E3; Mon, 9 Jan 2023 10:15:08 +0000 (UTC) Received: from sirius.home.kraxel.org (unknown [10.39.192.238]) by smtp.corp.redhat.com (Postfix) with ESMTPS id 148394078904; Mon, 9 Jan 2023 10:15:08 +0000 (UTC) Received: by sirius.home.kraxel.org (Postfix, from userid 1000) id 442211800624; Mon, 9 Jan 2023 11:15:05 +0100 (CET) Date: Mon, 9 Jan 2023 11:15:05 +0100 From: "Gerd Hoffmann" To: Laszlo Ersek Cc: devel@edk2.groups.io, Ard Biesheuvel , Jordan Justen , Pawel Polawski , Oliver Steffen , Jiewen Yao Subject: Re: [edk2-devel] [PATCH 1/2] OvmfPkg/PlatformInitLib: update PlatformScanOrAdd64BitE820Ram documentation Message-ID: <20230109101505.ehipkirgssqyd3rr@sirius.home.kraxel.org> References: <20230106140403.2889131-1-kraxel@redhat.com> <20230106140403.2889131-2-kraxel@redhat.com> MIME-Version: 1.0 In-Reply-To: X-Scanned-By: MIMEDefang 3.1 on 10.11.54.2 X-Mimecast-Spam-Score: 0 X-Mimecast-Originator: redhat.com Content-Type: text/plain; charset=us-ascii Content-Disposition: inline > > + @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. Well, it's not *that* different from the original. The implicit add-hob request (via MaxAddress == NULL) has been replaced by an explicit bool. MaxAddress works the same way it used to when non-NULL. LowMemory has the same behavior (set to non-NULL to have the value returned). Handling reservation hobs and reservation conflicts too doesn't fit in that well indeed. > 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. The code need to also work in SEC now. Using a HOB should work too, and given that the number of e820 entries is rather small (2-4 entries with 20 bytes each) it might not be that much of a problem to have that permanently allocated. I think a page allocator is not available in SEC. > (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. Looks like the better option to me. > (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. Was thinking about that one, but I don't like passing around VOID pointers and the overhead coming from the 4 callback functions. Maybe I can pass around EFI_HOB_PLATFORM_INFO pointers instead. take care, Gerd