From: "Ni, Ray" <ray.ni@intel.com>
To: Sheng Wei <w.sheng@intel.com>,devel@edk2.groups.io
Subject: Re: [edk2-devel] [PATCH] UefiCpuPkg/PiSmmCpuDxeSmm: Adjust level paging type for Internal CR3
Date: Mon, 26 Oct 2020 18:27:17 -0700 [thread overview]
Message-ID: <12075.1603762037777912272@groups.io> (raw)
In-Reply-To: <20201023033153.21360-1-w.sheng@intel.com>
[-- Attachment #1: Type: text/plain, Size: 1044 bytes --]
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 directly checking CR4 doesn't work sometimes? And when (For example, when the 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 is set when mInternalCr3 is generated." This commit message isn'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 is set to mInternalCr3, gSmmFeatureEnable5LevelPaging reflects the page table type pointed by mInternalCr3.
4. I cannot find below variable. Did you miss some code change in the patch?
//
// Variables from Protected Mode SMI Entry Template
//
extern BOOLEAN gSmmFeatureEnable5LevelPaging;
[-- Attachment #2: Type: text/html, Size: 1133 bytes --]
next parent reply other threads:[~2020-10-27 1:27 UTC|newest]
Thread overview: 4+ messages / expand[flat|nested] mbox.gz Atom feed top
[not found] <20201023033153.21360-1-w.sheng@intel.com>
2020-10-27 1:27 ` Ni, Ray [this message]
[not found] ` <CY4PR11MB192888F8A3E14DFC6B952B2AE1160@CY4PR11MB1928.namprd11.prod.outlook.com>
2020-10-27 9:43 ` [edk2-devel] [PATCH] UefiCpuPkg/PiSmmCpuDxeSmm: Adjust level paging type for Internal CR3 Ni, Ray
2020-10-27 10:00 ` Yao, Jiewen
2020-10-27 14:32 ` Laszlo Ersek
Reply instructions:
You may reply publicly to this message via plain-text email
using any one of the following methods:
* Save the following mbox file, import it into your mail client,
and reply-to-list from there: mbox
Avoid top-posting and favor interleaved quoting:
https://en.wikipedia.org/wiki/Posting_style#Interleaved_style
* Reply using the --to, --cc, and --in-reply-to
switches of git-send-email(1):
git send-email \
--in-reply-to=12075.1603762037777912272@groups.io \
--to=devel@edk2.groups.io \
/path/to/YOUR_REPLY
https://kernel.org/pub/software/scm/git/docs/git-send-email.html
* If your mail client supports setting the In-Reply-To header
via mailto: links, try the mailto: link
Be sure your reply has a Subject: header at the top and a blank line
before the message body.
This is a public inbox, see mirroring instructions
for how to clone and mirror all data and code used for this inbox