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 A0AC0740053 for ; Wed, 24 Jan 2024 16:37:33 +0000 (UTC) DKIM-Signature: a=rsa-sha256; bh=387MeuOGiZFgYvJ/VMSM/M1ZcdUi78Dxs4+tonNKvcs=; c=relaxed/simple; d=groups.io; h=Message-ID:Date:MIME-Version:Subject:To:Cc: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=1706114252; v=1; b=LKMdEXYDj8Ot/DywLo4UetAdv/J3fSlPteyzI0BaxzQxTEDekfqyCaKsmckuqwbBcz3/KR+o 3o1M80kUOT2e1PicJ3rwpdbiz7RWq6Em2gFJqas/E0UHdwdhgJTedc0kQzfiHY+8/IGXjwUZMni xCm1tdGWXhpAptewgYpTZNOU= X-Received: by 127.0.0.2 with SMTP id JaOnYY7687511xq24l1pRvVk; Wed, 24 Jan 2024 08:37:32 -0800 X-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.web11.27610.1706114251531688095 for ; Wed, 24 Jan 2024 08:37:31 -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-637-8tKR_GgVN4SC9R1Cj6i5cg-1; Wed, 24 Jan 2024 11:37:28 -0500 X-MC-Unique: 8tKR_GgVN4SC9R1Cj6i5cg-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 DBF25185A788; Wed, 24 Jan 2024 16:37:27 +0000 (UTC) X-Received: from [10.39.195.127] (unknown [10.39.195.127]) by smtp.corp.redhat.com (Postfix) with ESMTPS id CFDF2C37A83; Wed, 24 Jan 2024 16:37:26 +0000 (UTC) Message-ID: <640e533e-5252-32da-c155-a25e3065eb78@redhat.com> Date: Wed, 24 Jan 2024 17:37:25 +0100 MIME-Version: 1.0 Subject: Re: [edk2-devel] [PATCH 1/1] MdeModulePkg/VirtualMemory: drop 5-level paging assert To: devel@edk2.groups.io, kraxel@redhat.com Cc: Oliver Steffen , Liming Gao References: <20240124154929.2146147-1-kraxel@redhat.com> From: "Laszlo Ersek" In-Reply-To: <20240124154929.2146147-1-kraxel@redhat.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: 6k0xlAzM88cKB9PYsZXlRPRtx7686176AA= 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=LKMdEXYD; 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 On 1/24/24 16:49, Gerd Hoffmann wrote: > The ResetVector decides at runtime (depending in CPU capabilities) (1) s/depending in/dependent on/ > whenever it uses 5-level paging or not. Firmware builds with 5-level > paging enabled (PcdUse5LevelPageTable=3DTRUE) may run run in 4-level (2) typo: "run run" (3) Suggest the following wording improvement: Firmware that intends to enable 5-level paging may run in 4-level ... Justification: - the expression "firmware builds" suggests a fixed-at-build PCD, but this PCD is declared as [PcdsFixedAtBuild, PcdsPatchableInModule, PcdsDynamic, PcdsDynamicEx]. So it may be a runtime decision even to *request* 5-level paging. - "intends" rather than "enabled" because the PCD is just an expression of intent. Per DEC file: > ## Indicates if 5-Level Paging will be enabled in long mode. 5-Level Pa= ging will not be enabled > # when the PCD is TRUE but CPU doesn't support 5-Level Paging. Back to the patch: On 1/24/24 16:49, Gerd Hoffmann wrote: > paging mode. The code handles that just fine, by looking at the la57 > bit in cr4 instead of checking PcdUse5LevelPageTable. > > The ASSERT is not correct, I agree, but... > remove it. I have a better idea, I believe: > > Signed-off-by: Gerd Hoffmann > --- > MdeModulePkg/Core/DxeIplPeim/X64/VirtualMemory.c | 1 - > 1 file changed, 1 deletion(-) > > diff --git a/MdeModulePkg/Core/DxeIplPeim/X64/VirtualMemory.c b/MdeModule= Pkg/Core/DxeIplPeim/X64/VirtualMemory.c > index 980c2002d4f5..575e396eeffd 100644 > --- a/MdeModulePkg/Core/DxeIplPeim/X64/VirtualMemory.c > +++ b/MdeModulePkg/Core/DxeIplPeim/X64/VirtualMemory.c > @@ -745,7 +745,6 @@ CreateIdentityMappingPageTables ( > // > Cr4.UintN =3D AsmReadCr4 (); > Page5LevelSupport =3D (Cr4.Bits.LA57 !=3D 0); > - ASSERT (PcdGetBool (PcdUse5LevelPageTable) =3D=3D Page5LevelSupport)= ; In my opinion, what the code originally wanted to express here was not *equivalence*, but *implication* (that is, only one direction, not both directions). The idea is, if we find the LA57 bit set, then the platform must have requested 5-level paging. As a logical predicate: Page5LevelSupport =3D=3D> PcdGetBool (PcdUse5LevelPageTable) In C, we don't have an "implies" operator, but A =3D=3D> B is just !(A) || (B) (4) Therefore I suggest the following update (rather than deletion): ASSERT (!Page5LevelSupport || PcdGetBool (PcdUse5LevelPageTable)); Alternatively (equivalently): if (Page5LevelSupport) { ASSERT (PcdGetBool (PcdUse5LevelPageTable)); } This variant adds a bit of useless code to RELEASE builds, but in those builds, the empty body of the "if" should cause the "if" itself to be optimized away, too. So pick whichever you like. (If the assertion fails, then the first variant's error message in the log is more informative, FWIW!) (5) At the same time, the subject should become "*fix* 5-level paging assert". (6) "Page5LevelSupport" is a misnomer to begin with! In my opinion, the right name would be "Page5LevelEnabled"; after all, it comes from CR4 (it's active!), not from some MSR. Please consider renaming the variable in a separate patch (ahead of fixing the logic bug). > } else { > // > // If cpu runs in 32bit protected mode PEI, Page table Level in DXE = is decided by PCD and feature capability. 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 (#114318): https://edk2.groups.io/g/devel/message/114318 Mute This Topic: https://groups.io/mt/103934284/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-