From mboxrd@z Thu Jan 1 00:00:00 1970 Received: from mail-pg1-f171.google.com (mail-pg1-f171.google.com [209.85.215.171]) by mx.groups.io with SMTP id smtpd.web11.16842.1618421624597219996 for ; Wed, 14 Apr 2021 10:33:44 -0700 Authentication-Results: mx.groups.io; dkim=pass header.i=@gmail.com header.s=20161025 header.b=K9f8u/m8; spf=pass (domain: gmail.com, ip: 209.85.215.171, mailfrom: kuqin12@gmail.com) Received: by mail-pg1-f171.google.com with SMTP id p12so14921592pgj.10 for ; Wed, 14 Apr 2021 10:33:44 -0700 (PDT) DKIM-Signature: v=1; a=rsa-sha256; c=relaxed/relaxed; d=gmail.com; s=20161025; h=subject:to:cc:references:from:message-id:date:user-agent :mime-version:in-reply-to:content-language:content-transfer-encoding; bh=QA2nce9kpzPog8baU0lDYxwt9yW+qhH/JK3b48JPTSM=; b=K9f8u/m8Uw87+I1h+SuHkNtP6UDIGJ7WJABF3WJkghmLE+SD3S5XGeUQFA/h7UjeR7 NXmHbBIA8mcCzWVJzBmJQ8wVNNfUUGI2Tfu8A6QGGtC/bQHrGZRvHO9tWo8SO+NEwi0t B256sgGc973ISDQuAI7IkrQsKfjlt10SsMAuPYE6Lbfl7VuP44P7y9X2uyw2gXW8Ezy6 yCCp5Gukbc5y3emRJcLWv4urGK9IUs3wjEEHgmfCgIV10BLaue1gEimTlpSsJChwjnV/ Po3iV7+aI1YbYtPa0lOMk6BgNUNrEU0rCpuJbr9fv/87+/wh2ZRFhMqdqi4pVKLjFXZP /pLg== X-Google-DKIM-Signature: v=1; a=rsa-sha256; c=relaxed/relaxed; d=1e100.net; s=20161025; h=x-gm-message-state:subject:to:cc:references:from:message-id:date :user-agent:mime-version:in-reply-to:content-language :content-transfer-encoding; bh=QA2nce9kpzPog8baU0lDYxwt9yW+qhH/JK3b48JPTSM=; b=SJzYwSemrS9cEjmLy33wuYMIdPRSWKcchGambz4/Zhoqty5P7XgJ612QQvvjTH1tUe 6N7Hxqqt4FoPpPLTtEWuFBs/AenR3WYQUzwix/Uo4Pj6fba75A8FVwUGIhM2vfVEDefu Az0ftxkAH7LaUzVyJv5HPIlGNOMYAYKF8CHnCi0fBa3vzruBrJ7a0zVlzu6Z9TrcBYUY Tx7NXb6fACxFi+TMJZyrskAJ7FAE894VkiAc0sKFveSEEUAeW5Wxd+m0zzIuqGa+0pGc +hSwUd45z+0dvPVXZEyJ9AiViLvCHfLXXS4sH35GhQDub/XixS6SO2dnGIb40VIYZAVS 9weQ== X-Gm-Message-State: AOAM530IbG+02sfgGETNNXheJEVEvH58+MhOUd3eNDUZrVt8IpxKQULc r4JxUhaip/NL24I2dj9XdDU= X-Google-Smtp-Source: ABdhPJwDBDR0GQOpmuTxTC+hbvfY+/+ljcN6d4ufkYyMIjV6rJ8m4toxt62phGmLepaxk6bcaoAQGA== X-Received: by 2002:a63:1a11:: with SMTP id a17mr37918598pga.371.1618421624151; Wed, 14 Apr 2021 10:33:44 -0700 (PDT) Return-Path: Received: from [192.168.50.18] ([50.35.88.161]) by smtp.gmail.com with ESMTPSA id 14sm80885pfl.1.2021.04.14.10.33.43 (version=TLS1_3 cipher=TLS_AES_128_GCM_SHA256 bits=128/128); Wed, 14 Apr 2021 10:33:43 -0700 (PDT) Subject: Re: [PATCH v1 1/1] UefiCpuPkg: PiSmmCpuDxeSmm: Not to Change Bitwidth During Static Paging To: Laszlo Ersek , devel@edk2.groups.io Cc: Eric Dong , Ray Ni , Rahul Kumar References: <20210414025922.850-1-kuqin12@gmail.com> <20210414025922.850-2-kuqin12@gmail.com> <10167fcb-675b-f514-ff03-f40b6e071d1e@redhat.com> From: "Kun Qin" Message-ID: <2072f1f3-c1d4-96c6-a4f7-360aa3fabd9a@gmail.com> Date: Wed, 14 Apr 2021 10:33:43 -0700 User-Agent: Mozilla/5.0 (Windows NT 10.0; Win64; x64; rv:78.0) Gecko/20100101 Thunderbird/78.9.1 MIME-Version: 1.0 In-Reply-To: <10167fcb-675b-f514-ff03-f40b6e071d1e@redhat.com> Content-Type: text/plain; charset=utf-8; format=flowed Content-Language: en-US Content-Transfer-Encoding: 7bit Hi Laszlo, Thanks for the feedback. I will update commit message for (2) and variable type for (3) in v2. As per suggestion by Ray in another thread, we are moving "PhysicalAddressBits" from local variable to input parameter. So I will update the term "stack based variable" for (1) accordingly. Regards, Kun On 04/14/2021 02:15, Laszlo Ersek wrote: > 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. >> >