From mboxrd@z Thu Jan  1 00:00:00 1970
Subject: Re: [edk2-devel] [PATCH] UefiCpuPkg/PiSmmCpuDxeSmm: Adjust level paging type for Internal CR3
To: Sheng Wei <w.sheng@intel.com>,devel@edk2.groups.io
From: "Ni, Ray" <ray.ni@intel.com>
X-Originating-Location: Shanghai, CN (192.102.204.36)
X-Originating-Platform: Windows Chrome 86
User-Agent: GROUPS.IO Web Poster
MIME-Version: 1.0
Date: Mon, 26 Oct 2020 18:27:17 -0700
References: <20201023033153.21360-1-w.sheng@intel.com>
In-Reply-To: <20201023033153.21360-1-w.sheng@intel.com>
Message-ID: <12075.1603762037777912272@groups.io>
Content-Type: multipart/alternative; boundary="w972QsikrlhuxI1AxEf3"

--w972QsikrlhuxI1AxEf3
Content-Type: text/plain; charset="utf-8"; markup=markdown
Content-Transfer-Encoding: quoted-printable

The overall logic looks good to me and I agree to add an internal function =
like the patch.

Some minor comments:

1. Can you please separate the patch to two patches? One is to correct the=
 Cr3 typo, the other is to fix the bug.

2. Can you please add comments in Is5LevelPageTableBase() to explain why d=
irectly checking CR4 doesn't work sometimes? And when (For example, when th=
e function is called from entrypoint and the CR3/CR4 haven't been written)?

3. "When mInternalCr3 is used, get paging level type by a variable which i=
s set when mInternalCr3 is generated." This commit message isn't so meaning=
ful. It just translates the C logic to plain text. Better to explain the ca=
se when the functions called from entrypoint the page table is set to mInte=
rnalCr3, gSmmFeatureEnable5LevelPaging reflects the page table type pointed=
 by mInternalCr3.

4. I cannot find below variable. Did you miss some code change in the patc=
h?
//
// Variables from Protected Mode SMI Entry Template
//
extern BOOLEAN gSmmFeatureEnable5LevelPaging;

--w972QsikrlhuxI1AxEf3
Content-Type: text/html; charset="utf-8"
Content-Transfer-Encoding: quoted-printable

<p>The overall logic looks good to me and I agree to add an internal functi=
on like the patch.</p>

<p>Some minor comments:</p>

<ol>
<li><p>Can you please separate the patch to two patches? One is to correct=
 the Cr3 typo, the other is to fix the bug.</p></li>

<li><p>Can you please add comments in Is5LevelPageTableBase() to explain w=
hy directly checking CR4 doesn't work sometimes? And when (For example, whe=
n the function is called from entrypoint and the CR3/CR4 haven't been writt=
en)?</p></li>

<li><p>&quot;When mInternalCr3 is used, get paging level type by a variabl=
e which is set when mInternalCr3 is generated.&quot; This commit message is=
n't so meaningful. It just translates the C logic to plain text. Better to =
explain the case when the functions called from entrypoint the page table i=
s set to mInternalCr3, gSmmFeatureEnable5LevelPaging reflects the page tabl=
e type pointed by mInternalCr3.</p></li>

<li><p>I cannot find below variable. Did you miss some code change in the =
patch?
//
// Variables from Protected Mode SMI Entry Template
//
extern BOOLEAN gSmmFeatureEnable5LevelPaging;</p></li>
</ol>

--w972QsikrlhuxI1AxEf3--