From mboxrd@z Thu Jan 1 00:00:00 1970 Received: from us-smtp-delivery-124.mimecast.com (us-smtp-delivery-124.mimecast.com [216.205.24.124]) by mx.groups.io with SMTP id smtpd.web12.10380.1618391752605782999 for ; Wed, 14 Apr 2021 02:15:52 -0700 Authentication-Results: mx.groups.io; dkim=pass header.i=@redhat.com header.s=mimecast20190719 header.b=GJ6esExi; spf=pass (domain: redhat.com, ip: 216.205.24.124, mailfrom: lersek@redhat.com) DKIM-Signature: v=1; a=rsa-sha256; c=relaxed/relaxed; d=redhat.com; s=mimecast20190719; t=1618391751; 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=kmKdmp1vJwqPRmmb38YG9G/mex06vdrxsHocPbg4p1M=; b=GJ6esExis3W7AV64M3MOs/ajTAbuZlpikWirKoVEckke/TZUtVwltgxgZYv4UZwvCKsbD2 wLXgFLzAaZrEY/A9BnvBD9+9qJAlqwkR6yk+jhmhXu72Ylhwjxc7qxHx4Yby7Q6M5qBsPH u33mUbOPDvGjg6lUSnCnyPx3uhWlyqM= 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-321-4rfkUz_mPd-Cgzt_8fU0EQ-1; Wed, 14 Apr 2021 05:15:48 -0400 X-MC-Unique: 4rfkUz_mPd-Cgzt_8fU0EQ-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 CC629107ACCA; Wed, 14 Apr 2021 09:15:46 +0000 (UTC) Received: from lacos-laptop-7.usersys.redhat.com (ovpn-113-134.ams2.redhat.com [10.36.113.134]) by smtp.corp.redhat.com (Postfix) with ESMTP id 6B0F35D9CC; Wed, 14 Apr 2021 09:15:45 +0000 (UTC) Subject: Re: [PATCH v1 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: <20210414025922.850-1-kuqin12@gmail.com> <20210414025922.850-2-kuqin12@gmail.com> From: "Laszlo Ersek" Message-ID: <10167fcb-675b-f514-ff03-f40b6e071d1e@redhat.com> Date: Wed, 14 Apr 2021 11:15:44 +0200 MIME-Version: 1.0 In-Reply-To: <20210414025922.850-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 04:59, 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 replaced the changed bitwidth to a stack based variable. (1) "local variable" is a more common expression. If we wanted to be exact, we could call it "variable with automatic storage duration". Either way, "stack" is irrelevant here, IMO. > > Cc: Eric Dong > Cc: Ray Ni > Cc: Laszlo Ersek > Cc: Rahul Kumar > > Signed-off-by: Kun Qin > --- > UefiCpuPkg/PiSmmCpuDxeSmm/X64/PageTbl.c | 25 +++++++++++--------- > 1 file changed, 14 insertions(+), 11 deletions(-) (2) I suggest adding the following line to the commit message, just above your Signed-off-by: Fixes: 4eee0cc7cc0db74489b99c19eba056b53eda6358 Because the issue is a regression from that commit. > > diff --git a/UefiCpuPkg/PiSmmCpuDxeSmm/X64/PageTbl.c b/UefiCpuPkg/PiSmmCpuDxeSmm/X64/PageTbl.c > index 6902584b1fbd..0caee8a27abe 100644 > --- a/UefiCpuPkg/PiSmmCpuDxeSmm/X64/PageTbl.c > +++ b/UefiCpuPkg/PiSmmCpuDxeSmm/X64/PageTbl.c > @@ -226,6 +226,7 @@ SetStaticPageTable ( > UINTN IndexOfPml4Entries; > UINTN IndexOfPdpEntries; > UINTN IndexOfPageDirectoryEntries; > + UINT64 PhysicalAddressBits; > UINT64 *PageMapLevel5Entry; > UINT64 *PageMapLevel4Entry; > UINT64 *PageMap; (3) The "mPhysicalAddressBits" global variable has type UINT8. I don't see a reason for introducing the "PhysicalAddressBits" local variable with a different type. The rest looks good to me. Thanks Laszlo > @@ -237,26 +238,28 @@ 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; > + PhysicalAddressBits = mPhysicalAddressBits; > + > + 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. >