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.133.124]) by mx.groups.io with SMTP id smtpd.web10.5180.1618483286752469880 for ; Thu, 15 Apr 2021 03:41:26 -0700 Authentication-Results: mx.groups.io; dkim=pass header.i=@redhat.com header.s=mimecast20190719 header.b=WsBXBmWk; spf=pass (domain: redhat.com, ip: 170.10.133.124, mailfrom: lersek@redhat.com) DKIM-Signature: v=1; a=rsa-sha256; c=relaxed/relaxed; d=redhat.com; s=mimecast20190719; t=1618483285; 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=KEr5LR3qT/DvvMjSJ9kimFAoHM4lz8tGOy1nzK1JLoA=; b=WsBXBmWkY+I9Ru08e4jBGXnV4Nk6+oZfXdvbOPuDi9YrxTxIbTTNzXBm+/QM490ZE+QH1R 9yvAx09LiMHdogiHnz4Cxq/P0a2+1ArtatKgZyjEe4fC6wQNRTKRdcXsF/Ui84H4kn3Abd GHkweC9IMuJL5fnkGL0S63jfsTeNAp4= Received: from mimecast-mx01.redhat.com (mimecast-mx01.redhat.com [209.132.183.4]) (Using TLS) by relay.mimecast.com with ESMTP id us-mta-314-K127nmdzNGykEZ8zTSL8jw-1; Thu, 15 Apr 2021 06:41:22 -0400 X-MC-Unique: K127nmdzNGykEZ8zTSL8jw-1 Received: from smtp.corp.redhat.com (int-mx04.intmail.prod.int.phx2.redhat.com [10.5.11.14]) (using TLSv1.2 with cipher AECDH-AES256-SHA (256/256 bits)) (No client certificate requested) by mimecast-mx01.redhat.com (Postfix) with ESMTPS id 06F4F1922025; Thu, 15 Apr 2021 10:41:21 +0000 (UTC) Received: from lacos-laptop-7.usersys.redhat.com (ovpn-115-85.ams2.redhat.com [10.36.115.85]) by smtp.corp.redhat.com (Postfix) with ESMTP id A8E7329AAE; Thu, 15 Apr 2021 10:41:19 +0000 (UTC) Subject: Re: [PATCH v2 1/1] UefiCpuPkg: PiSmmCpuDxeSmm: Not to Change Bitwidth During Static Paging To: Kun Qin , devel@edk2.groups.io Cc: Eric Dong , Ray Ni , Rahul Kumar References: <20210414202547.394-1-kuqin12@gmail.com> <20210414202547.394-2-kuqin12@gmail.com> From: "Laszlo Ersek" Message-ID: <9a412dd6-e026-8741-35d7-7943eab7be63@redhat.com> Date: Thu, 15 Apr 2021 12:41:18 +0200 MIME-Version: 1.0 In-Reply-To: <20210414202547.394-2-kuqin12@gmail.com> X-Scanned-By: MIMEDefang 2.79 on 10.5.11.14 Authentication-Results: relay.mimecast.com; auth=pass smtp.auth=CUSA124A263 smtp.mailfrom=lersek@redhat.com X-Mimecast-Spam-Score: 0 X-Mimecast-Originator: redhat.com Content-Type: text/plain; charset=utf-8 Content-Language: en-US Content-Transfer-Encoding: 7bit On 04/14/21 22:25, Kun Qin wrote: > REF: https://bugzilla.tianocore.org/show_bug.cgi?id=3300 > > Current implementation of SetStaticPageTable routine in PiSmmCpuDxeSmm > driver will check a global variable mPhysicalAddressBits, and eventually > cap any value larger than 39 at 39. > > This global variable is used in ConvertMemoryPageAttributes, which backs > SmmSetMemoryAttributes and SmmClearMemoryAttributes. Thus for a processor > that supports more than 39 bits width, trying to mark page table regions > higher than 39-bit will always return EFI_UNSUPPROTED. > > This change updated the interface of SetStaticPageTable function to take > PhysicalAddressBits as an input parameter, in order to avoid changing/ > accessing the global variable. > > Cc: Eric Dong > Cc: Ray Ni > Cc: Laszlo Ersek > Cc: Rahul Kumar > > Fixes: 4eee0cc7cc0db74489b99c19eba056b53eda6358 > Signed-off-by: Kun Qin > --- > > Notes: > v2: > - SetStaticPageTable interface update [Ray] > - Commit message updates, variable type change [Laszlo] > > UefiCpuPkg/PiSmmCpuDxeSmm/X64/PageTbl.c | 30 +++++++++++--------- > 1 file changed, 16 insertions(+), 14 deletions(-) > > diff --git a/UefiCpuPkg/PiSmmCpuDxeSmm/X64/PageTbl.c b/UefiCpuPkg/PiSmmCpuDxeSmm/X64/PageTbl.c > index 6902584b1fbd..d6f8dd94d303 100644 > --- a/UefiCpuPkg/PiSmmCpuDxeSmm/X64/PageTbl.c > +++ b/UefiCpuPkg/PiSmmCpuDxeSmm/X64/PageTbl.c > @@ -211,11 +211,13 @@ CalculateMaximumSupportAddress ( > /** > Set static page table. > > - @param[in] PageTable Address of page table. > + @param[in] PageTable Address of page table. > + @param[in] PhysicalAddressBits The maximum physical address bits supported. > **/ > VOID > SetStaticPageTable ( > - IN UINTN PageTable > + IN UINTN PageTable, > + IN UINT8 PhysicalAddressBits > ) > { > UINT64 PageAddress; > @@ -237,26 +239,26 @@ SetStaticPageTable ( > // IA-32e paging translates 48-bit linear addresses to 52-bit physical addresses > // when 5-Level Paging is disabled. > // > - ASSERT (mPhysicalAddressBits <= 52); > - if (!m5LevelPagingNeeded && mPhysicalAddressBits > 48) { > - mPhysicalAddressBits = 48; > + ASSERT (PhysicalAddressBits <= 52); > + if (!m5LevelPagingNeeded && PhysicalAddressBits > 48) { > + PhysicalAddressBits = 48; > } > > NumberOfPml5EntriesNeeded = 1; > - if (mPhysicalAddressBits > 48) { > - NumberOfPml5EntriesNeeded = (UINTN) LShiftU64 (1, mPhysicalAddressBits - 48); > - mPhysicalAddressBits = 48; > + if (PhysicalAddressBits > 48) { > + NumberOfPml5EntriesNeeded = (UINTN) LShiftU64 (1, PhysicalAddressBits - 48); > + PhysicalAddressBits = 48; > } > > NumberOfPml4EntriesNeeded = 1; > - if (mPhysicalAddressBits > 39) { > - NumberOfPml4EntriesNeeded = (UINTN) LShiftU64 (1, mPhysicalAddressBits - 39); > - mPhysicalAddressBits = 39; > + if (PhysicalAddressBits > 39) { > + NumberOfPml4EntriesNeeded = (UINTN) LShiftU64 (1, PhysicalAddressBits - 39); > + PhysicalAddressBits = 39; > } > > NumberOfPdpEntriesNeeded = 1; > - ASSERT (mPhysicalAddressBits > 30); > - NumberOfPdpEntriesNeeded = (UINTN) LShiftU64 (1, mPhysicalAddressBits - 30); > + ASSERT (PhysicalAddressBits > 30); > + NumberOfPdpEntriesNeeded = (UINTN) LShiftU64 (1, PhysicalAddressBits - 30); > > // > // By architecture only one PageMapLevel4 exists - so lets allocate storage for it. > @@ -438,7 +440,7 @@ SmmInitPageTable ( > // When access to non-SMRAM memory is restricted, create page table > // that covers all memory space. > // > - SetStaticPageTable ((UINTN)PTEntry); > + SetStaticPageTable ((UINTN)PTEntry, mPhysicalAddressBits); > } else { > // > // Add pages to page pool > Last time I checked the variable renames carefully, now I'm trusting that those have not been broken/modified since v1. Reviewed-by: Laszlo Ersek