From mboxrd@z Thu Jan 1 00:00:00 1970 Return-Path: Received: from mail02.groups.io (mail02.groups.io [66.175.222.108]) by spool.mail.gandi.net (Postfix) with ESMTPS id 9D88CAC163E for ; Wed, 10 Jan 2024 13:09:59 +0000 (UTC) DKIM-Signature: a=rsa-sha256; bh=FTIjzPRU5o+9ap/H9MklITeUJb8jAQRpNZJkQ4PoQcA=; c=relaxed/simple; d=groups.io; h=Message-ID:Date:MIME-Version:Subject:To:References:From:In-Reply-To:Precedence:List-Subscribe:List-Help:Sender:List-Id:Mailing-List:Delivered-To:Reply-To:List-Unsubscribe-Post:List-Unsubscribe:Content-Language:Content-Type:Content-Transfer-Encoding; s=20140610; t=1704892198; v=1; b=veruSAseGtLW62fuNQ5f+4999YgDaaiE/i/4C+Dk+JapfYy0hWeLuzMYmHv4Tz96HG+rsJMI cIxa1IVbv8qFKNML7Cru+G9xIi+jkjZu4BE+fkie+PoGpFpA2RKBw7LrzJuayEvzStmxgSDc+Qr p0QwXyg13DJ5ETXQIu7PTw2Y= X-Received: by 127.0.0.2 with SMTP id i3LWYY7687511xv6sBPnZM8j; Wed, 10 Jan 2024 05:09:58 -0800 X-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.11053.1704892197592900664 for ; Wed, 10 Jan 2024 05:09:57 -0800 X-Received: from mimecast-mx02.redhat.com (mimecast-mx02.redhat.com [66.187.233.88]) by relay.mimecast.com with ESMTP with STARTTLS (version=TLSv1.3, cipher=TLS_AES_256_GCM_SHA384) id us-mta-376-RZTPwdLONyOEUsxSF-D5Hg-1; Wed, 10 Jan 2024 08:09:53 -0500 X-MC-Unique: RZTPwdLONyOEUsxSF-D5Hg-1 X-Received: from smtp.corp.redhat.com (int-mx08.intmail.prod.int.rdu2.redhat.com [10.11.54.8]) (using TLSv1.3 with cipher TLS_AES_256_GCM_SHA384 (256/256 bits) key-exchange X25519 server-signature RSA-PSS (2048 bits) server-digest SHA256) (No client certificate requested) by mimecast-mx02.redhat.com (Postfix) with ESMTPS id 478E810AFB4B; Wed, 10 Jan 2024 13:09:53 +0000 (UTC) X-Received: from [10.39.192.166] (unknown [10.39.192.166]) by smtp.corp.redhat.com (Postfix) with ESMTPS id 96A71C15E6A; Wed, 10 Jan 2024 13:09:52 +0000 (UTC) Message-ID: <2ad16043-754e-3bb9-3a4a-702d9a50bf63@redhat.com> Date: Wed, 10 Jan 2024 14:09:51 +0100 MIME-Version: 1.0 Subject: Re: [edk2-devel] Memory Attribute for depex section To: devel@edk2.groups.io, nhi@os.amperecomputing.com, "ardb+tianocore@kernel.org" References: <44ca139f-4d78-4322-b5b6-8e9788bb7486@os.amperecomputing.com> From: "Laszlo Ersek" In-Reply-To: <44ca139f-4d78-4322-b5b6-8e9788bb7486@os.amperecomputing.com> X-Scanned-By: MIMEDefang 3.4.1 on 10.11.54.8 X-Mimecast-Spam-Score: 0 X-Mimecast-Originator: redhat.com Precedence: Bulk List-Subscribe: List-Help: Sender: devel@edk2.groups.io List-Id: Mailing-List: list devel@edk2.groups.io; contact devel+owner@edk2.groups.io Reply-To: devel@edk2.groups.io,lersek@redhat.com List-Unsubscribe-Post: List-Unsubscribe=One-Click List-Unsubscribe: X-Gm-Message-State: ItGjcZk44Jf8aJPvog26ISGex7686176AA= Content-Language: en-US Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: quoted-printable X-GND-Status: LEGIT Authentication-Results: spool.mail.gandi.net; dkim=pass header.d=groups.io header.s=20140610 header.b=veruSAse; dmarc=fail reason="SPF not aligned (relaxed), DKIM not aligned (relaxed)" header.from=redhat.com (policy=none); spf=pass (spool.mail.gandi.net: domain of bounce@groups.io designates 66.175.222.108 as permitted sender) smtp.mailfrom=bounce@groups.io Hi, On 1/8/24 11:11, Nhi Pham via groups.io wrote: > Hi Ard, all, > > Could you please help explain how the depex section in an image is > mapped in terms of memory attribute? > > As my observation, dispatcher locates[1] the depex section inside the > module image and write[2] an evaluated data to the depex if necessary > for scheduled boot order. The problem that the depex section is now in > RO+X memory due to a part of the module image, so a writing to depex > would cause data abort. I'm unsure whether this issue is generic in > EDK2 or not. > > I think of two approaches: > > #1 Relocate the depex section to heap memory for dependency > #evaluation? > > #2 EDK2 build tool to support granting write permission for depex > #section. > > [1] StandaloneMmPkg/Core/FwVol.c:236 > [2] StandaloneMmPkg/Core/Dependency.c:256 here's my understanding of the issue. In Platform Init 1.8, section II-10.7 describes the depex instruction set for the DXE DISPATCHER. In particular, section II-10.7.3 describes the PUSH opcode as follows: PUSH Pushes a Boolean value onto the stack. If the GUID is present in the handle database, then a TRUE is pushed onto the stack. If the GUID is not present in the handle database, then a FALSE is pushed onto the stack. The test for the GUID in the handle database may be performed with the Boot Service LocateProtocol(). This basically says that every time the dispatcher sees a PUSH opcode in a depex, it has to look up the protocol GUID in the protocol database. Now, as far as I can tell, edk2's DXE Core implementation wanted to optimize this. (And this goes back to ancient commit 28a00297189c, from 2007.) Here's the idea AIUI: - Assume the protocol is not found, i.e. we push a FALSE. This will likely mean that the driver whose DEPEX contains this PUSH opcode cannot be dispatched just yet. In other words, we're going to have to re-evaluate this PUSH at least once more, possibly multiple times. We cannot optimize anything here, because the needed protocol is not present yet, but it may become available later. When we next evaluate the DEPEX for this driver, we'll check again if the protocol has become available by then. - Assume the protocol is found, i.e. we push a TRUE. The optimization here is that we assume the protocol will not *disappear*, once available. The evaluation of the driver's *entire* DEPEX may still result in FALSE, so it's not guaranteed that the driver can be dispatched right now. However, given our assumption that the protocol we've just found will not disappear during DXE driver dispatch, we might want to "cache" the current successful protocol lookup, and when we *next* evaluate the DEPEX for this same driver (assuming we cannot dispatch it right now due to something else missing from its DEPEX), we don't want to perform the *same* protocol lookup again -- we'll just want to remember it was already there last time. And the way the DXE core seems to implement this optimization (i.e., how it "remembers success") is that it patches the DEPEX in-place: it replaces the PUSH opcode with a special (edk2-specific) opcode called EFI_DEP_REPLACE_TRUE (in addition to pushing the TRUE of course, to the eval stack). This opcode is functionally identical to the plain (and standard) TRUE opcode, so the next time the dispatcher evaluates the same depex and sees EFI_DEP_REPLACE_TRUE it will just push TRUE, the difference with the TRUE opcode is that EFI_DEP_REPLACE_TRUE also provides "room" for the -- otherwise useless -- original protocol GUID that used to be the argument of the PUSH opcode. The dispatcher ignores the protocol GUID on EFI_DEP_REPLACE_TRUE, beyond logging it. EFI_DEP_REPLACE_TRUE is documented in the code as follows ("MdeModulePkg/Core/Dxe/DxeMain.h"): /// /// EFI_DEP_REPLACE_TRUE - Used to dynamically patch the dependency express= ion /// to save time. A EFI_DEP_PUSH is evaluated one a= n /// replaced with EFI_DEP_REPLACE_TRUE. If PI spec's= Vol 2 /// Driver Execution Environment Core Interface use = 0xff /// as new DEPEX opcode. EFI_DEP_REPLACE_TRUE should= be /// defined to a new value that is not conflicting w= ith PI spec. /// #define EFI_DEP_REPLACE_TRUE 0xff This documentation is hard to understand due to English grammar errors. It means to say: "Used to dynamically patch the dependency expression to save time. An EFI_DEP_PUSH opcode is evaluated *once*, *and* replaced with EFI_DEP_REPLACE_TRUE. If PI spec's Vol 2 Driver Execution Environment Core Interface *starts using* 0xff as new DEPEX opcode *in the future*, *then* EFI_DEP_REPLACE_TRUE should be *redefined* to a new value that is not conflicting with *said new* PI spec." Over time, this optimization (hack) has made its way into the traditional PiSmmCore, and finally into the standalone SMM core, apparently. In summary, this seems like a performance optimization, and should make no functional difference. If we remove it, then driver dispatch could become a bit slower (because we'd re-evaluate such PUSH opcodes too that had previously succeeded in locating the given protocol GUID), but then we'd not try to overwrite the depex section of the driver image in memory. I think that keeping the depex section read-only is valuable, so I'd rule out #2. I'd also not start with option #1 -- copying the depex to heap memory, just so this optimization can succeed. I'd simply start by removing the optimization, and measuring how much driver dispatch slows down in practice, on various platforms. Can you try this? (I have only build-tested and "uncrustified" it.) The patch removes the EFI_DEP_REPLACE_TRUE handling altogether, plus it CONST-ifies the Iterator pointer (which points into the DEPEX section), so that the compiler catch any possible accesses at *build time* that would write to the write-protected DEPEX memory area. ----------------------------- diff --git a/StandaloneMmPkg/Core/Dependency.c b/StandaloneMmPkg/Core/Depen= dency.c index 440fe3e45238..2bcb07d34666 100644 --- a/StandaloneMmPkg/Core/Dependency.c +++ b/StandaloneMmPkg/Core/Dependency.c @@ -13,16 +13,6 @@ #include "StandaloneMmCore.h" -/// -/// EFI_DEP_REPLACE_TRUE - Used to dynamically patch the dependency expres= sion -/// to save time. A EFI_DEP_PUSH is evaluated one = an -/// replaced with EFI_DEP_REPLACE_TRUE. If PI spec'= s Vol 2 -/// Driver Execution Environment Core Interface use= 0xff -/// as new DEPEX opcode. EFI_DEP_REPLACE_TRUE shoul= d be -/// defined to a new value that is not conflicting = with PI spec. -/// -#define EFI_DEP_REPLACE_TRUE 0xff - /// /// Define the initial size of the dependency expression evaluation stack /// @@ -170,12 +160,12 @@ MmIsSchedulable ( IN EFI_MM_DRIVER_ENTRY *DriverEntry ) { - EFI_STATUS Status; - UINT8 *Iterator; - BOOLEAN Operator; - BOOLEAN Operator2; - EFI_GUID DriverGuid; - VOID *Interface; + EFI_STATUS Status; + CONST UINT8 *Iterator; + BOOLEAN Operator; + BOOLEAN Operator2; + EFI_GUID DriverGuid; + VOID *Interface; Operator =3D FALSE; Operator2 =3D FALSE; @@ -253,8 +243,7 @@ MmIsSchedulable ( Status =3D PushBool (FALSE); } else { DEBUG ((DEBUG_DISPATCH, " PUSH GUID(%g) =3D TRUE\n", &DriverGui= d)); - *Iterator =3D EFI_DEP_REPLACE_TRUE; - Status =3D PushBool (TRUE); + Status =3D PushBool (TRUE); } if (EFI_ERROR (Status)) { @@ -356,18 +345,6 @@ MmIsSchedulable ( DEBUG ((DEBUG_DISPATCH, " RESULT =3D %a\n", Operator ? "TRUE" : "= FALSE")); return Operator; - case EFI_DEP_REPLACE_TRUE: - CopyMem (&DriverGuid, Iterator + 1, sizeof (EFI_GUID)); - DEBUG ((DEBUG_DISPATCH, " PUSH GUID(%g) =3D TRUE\n", &DriverGuid)= ); - Status =3D PushBool (TRUE); - if (EFI_ERROR (Status)) { - DEBUG ((DEBUG_DISPATCH, " RESULT =3D FALSE (Unexpected error)\n= ")); - return FALSE; - } - - Iterator +=3D sizeof (EFI_GUID); - break; - default: DEBUG ((DEBUG_DISPATCH, " RESULT =3D FALSE (Unknown opcode)\n")); goto Done; ----------------------------- Thanks Laszlo -=3D-=3D-=3D-=3D-=3D-=3D-=3D-=3D-=3D-=3D-=3D- Groups.io Links: You receive all messages sent to this group. View/Reply Online (#113531): https://edk2.groups.io/g/devel/message/113531 Mute This Topic: https://groups.io/mt/103594587/7686176 Group Owner: devel+owner@edk2.groups.io Unsubscribe: https://edk2.groups.io/g/devel/leave/12367111/7686176/19134562= 12/xyzzy [rebecca@openfw.io] -=3D-=3D-=3D-=3D-=3D-=3D-=3D-=3D-=3D-=3D-=3D-