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 E7890740032 for ; Fri, 19 Jan 2024 10:40:09 +0000 (UTC) DKIM-Signature: a=rsa-sha256; bh=BytMTWZ1rgUNtM27kfAfxoAz4Lc0xjPdJJ5Pkh4e3W8=; c=relaxed/simple; d=groups.io; h=MIME-Version:References:In-Reply-To:From:Date:Message-ID:Subject:To:Cc:Precedence:List-Subscribe:List-Help:Sender:List-Id:Mailing-List:Delivered-To:Reply-To:List-Unsubscribe-Post:List-Unsubscribe:Content-Type; s=20140610; t=1705660808; v=1; b=cW4tR7Rivn2Ea5Wbdzak3VRjSWtdj8McX+ugnL3ikVmHK2Gxvm0GYw2Qoswh8kIPuReZWOes LtKlyTEzVWmoRFwmrtrP40DkWigt0fZDyUoDEF84SxNpbSt9kg3Q01AEtDPrIIAvPKiv+7UNLy7 MYIAZuagenIn4hc75Y/ARMFA= X-Received: by 127.0.0.2 with SMTP id 5omLYY7687511xcvmCoCbkau; Fri, 19 Jan 2024 02:40:08 -0800 X-Received: from dfw.source.kernel.org (dfw.source.kernel.org [139.178.84.217]) by mx.groups.io with SMTP id smtpd.web10.19231.1705660807887865643 for ; Fri, 19 Jan 2024 02:40:08 -0800 X-Received: from smtp.kernel.org (transwarp.subspace.kernel.org [100.75.92.58]) by dfw.source.kernel.org (Postfix) with ESMTP id 4AE296196B for ; Fri, 19 Jan 2024 10:40:07 +0000 (UTC) X-Received: by smtp.kernel.org (Postfix) with ESMTPSA id D8A30C433A6 for ; Fri, 19 Jan 2024 10:40:06 +0000 (UTC) X-Received: by mail-lf1-f44.google.com with SMTP id 2adb3069b0e04-50e7f58c5fbso760741e87.1 for ; Fri, 19 Jan 2024 02:40:06 -0800 (PST) X-Gm-Message-State: J1g3ahaclCL0vLyrJKRhQWaSx7686176AA= X-Google-Smtp-Source: AGHT+IFGXcplaHWEd0i+vezWGLAn1ERHw0gTpIy2cvi1nnTM4A3EJllcGqhM45jS+8oiEm81giJfh6Zh0tRbR395xNE= X-Received: by 2002:ac2:5feb:0:b0:50e:51fa:1d3b with SMTP id s11-20020ac25feb000000b0050e51fa1d3bmr513375lfg.93.1705660804939; Fri, 19 Jan 2024 02:40:04 -0800 (PST) MIME-Version: 1.0 References: <20240119045646.3896430-1-nhi@os.amperecomputing.com> In-Reply-To: <20240119045646.3896430-1-nhi@os.amperecomputing.com> From: "Ard Biesheuvel" Date: Fri, 19 Jan 2024 11:39:53 +0100 X-Gmail-Original-Message-ID: Message-ID: Subject: Re: [edk2-devel] [PATCH 1/1] StandaloneMmPkg/Core: Remove optimization for depex evaluation To: devel@edk2.groups.io, nhi@os.amperecomputing.com Cc: ardb+tianocore@kernel.org, ray.ni@intel.com, sami.mujawar@arm.com, lersek@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,ardb@kernel.org List-Unsubscribe-Post: List-Unsubscribe=One-Click List-Unsubscribe: Content-Type: text/plain; charset="UTF-8" X-GND-Status: LEGIT Authentication-Results: spool.mail.gandi.net; dkim=pass header.d=groups.io header.s=20140610 header.b=cW4tR7Ri; dmarc=fail reason="SPF not aligned (relaxed), DKIM not aligned (relaxed)" header.from=kernel.org (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 On Fri, 19 Jan 2024 at 05:58, Nhi Pham via groups.io wrote: > > From: Laszlo Ersek > > The current dependency evaluator violates the memory access permission > when patching depex grammar directly in the read-only depex memory area. > > Laszlo pointed out the optimization issue in the thread (1) "Memory > Attribute for depex section" and provided suggested patch to remove the > perf optimization. > > In my testing, removing the optimization does not make significant perf > reduction. That makes sense that StandaloneMM dispatcher only searches > in MM protocol database and does not depend on UEFI/DXE protocol > database. Also, we don't have many protocols in StandaloneMM like > UEFI/DXE. > > From Laszlo, > > "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." > > (1) https://edk2.groups.io/g/devel/message/113531 > > Signed-off-by: Nhi Pham Thanks for the patch. This looks good to me in principle, only the patch got mangled by your MTA Please resend using 8bit transfer encoding. (You can use BaseTools/Scripts/SetupGit.py to configure Git for you) > --- > StandaloneMmPkg/Core/Dependency.c | 37 ++++---------------- > 1 file changed, 7 insertions(+), 30 deletions(-) > > diff --git a/StandaloneMmPkg/Core/Dependency.c b/StandaloneMmPkg/Core/Dependency.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 expression > > -/// 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 should 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 = FALSE; > > Operator2 = FALSE; > > @@ -253,8 +243,7 @@ MmIsSchedulable ( > Status = PushBool (FALSE); > > } else { > > DEBUG ((DEBUG_DISPATCH, " PUSH GUID(%g) = TRUE\n", &DriverGuid)); > > - *Iterator = EFI_DEP_REPLACE_TRUE; > > - Status = PushBool (TRUE); > > + Status = PushBool (TRUE); > > } > > > > if (EFI_ERROR (Status)) { > > @@ -356,18 +345,6 @@ MmIsSchedulable ( > DEBUG ((DEBUG_DISPATCH, " RESULT = %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) = TRUE\n", &DriverGuid)); > > - Status = PushBool (TRUE); > > - if (EFI_ERROR (Status)) { > > - DEBUG ((DEBUG_DISPATCH, " RESULT = FALSE (Unexpected error)\n")); > > - return FALSE; > > - } > > - > > - Iterator += sizeof (EFI_GUID); > > - break; > > - > > default: > > DEBUG ((DEBUG_DISPATCH, " RESULT = FALSE (Unknown opcode)\n")); > > goto Done; > > -- > 2.25.1 > > > > > > -=-=-=-=-=-=-=-=-=-=-=- Groups.io Links: You receive all messages sent to this group. View/Reply Online (#114041): https://edk2.groups.io/g/devel/message/114041 Mute This Topic: https://groups.io/mt/103824815/7686176 Group Owner: devel+owner@edk2.groups.io Unsubscribe: https://edk2.groups.io/g/devel/unsub [rebecca@openfw.io] -=-=-=-=-=-=-=-=-=-=-=-