From: "Laszlo Ersek" <lersek@redhat.com>
To: devel@edk2.groups.io, ray.ni@intel.com
Subject: Re: [edk2-devel] [PATCH 1/3] UefiCpuPkg/PiSmmCpu: Add Internal function IsStaticPageTableEnabled
Date: Mon, 29 Jul 2019 13:33:16 +0200 [thread overview]
Message-ID: <1812e3c6-f388-6faa-71d9-8e878b8e9b6c@redhat.com> (raw)
In-Reply-To: <20190727032850.337840-2-ray.ni@intel.com>
Hi Ray,
On 07/27/19 05:28, Ni, Ray wrote:
> The internal function reflects the status whether static page table
> is enabled.
>
> Signed-off-by: Ray Ni <ray.ni@intel.com>
> Cc: Laszlo Ersek <lersek@redhat.com>
> Cc: Eric Dong <eric.dong@intel.com.
> ---
> UefiCpuPkg/PiSmmCpuDxeSmm/Ia32/PageTbl.c | 16 ++++++++++++++++
> UefiCpuPkg/PiSmmCpuDxeSmm/PiSmmCpuDxeSmm.h | 15 +++++++++++++++
> UefiCpuPkg/PiSmmCpuDxeSmm/X64/PageTbl.c | 16 ++++++++++++++++
> 3 files changed, 47 insertions(+)
>
> diff --git a/UefiCpuPkg/PiSmmCpuDxeSmm/Ia32/PageTbl.c b/UefiCpuPkg/PiSmmCpuDxeSmm/Ia32/PageTbl.c
> index 05fb455936..2a9af4b77d 100644
> --- a/UefiCpuPkg/PiSmmCpuDxeSmm/Ia32/PageTbl.c
> +++ b/UefiCpuPkg/PiSmmCpuDxeSmm/Ia32/PageTbl.c
> @@ -28,6 +28,22 @@ EnableCet (
> VOID
> );
>
> +/**
> + Return whether Static Page Table is enabled.
> +
> + Note: Static Page Table is always disabled for IA32 build.
> +
> + @retval TRUE Static Page Table is enabled.
> + @retval FALSE Static Page Table is disabled.
> +**/
> +BOOLEAN
> +IsStaticPageTableEnabled (
> + VOID
> + )
> +{
> + return FALSE;
> +}
> +
> /**
> Create PageTable for SMM use.
>
> diff --git a/UefiCpuPkg/PiSmmCpuDxeSmm/PiSmmCpuDxeSmm.h b/UefiCpuPkg/PiSmmCpuDxeSmm/PiSmmCpuDxeSmm.h
> index 186809f431..14b7676c16 100644
> --- a/UefiCpuPkg/PiSmmCpuDxeSmm/PiSmmCpuDxeSmm.h
> +++ b/UefiCpuPkg/PiSmmCpuDxeSmm/PiSmmCpuDxeSmm.h
> @@ -1267,6 +1267,21 @@ EFIAPI
> PiSmmCpuSmiEntryFixupAddress (
> );
>
> +
> +/**
> + Return whether Static Page Table is enabled.
> +
> + Note: Static Page Table is always disabled for IA32 build.
> +
> + @retval TRUE Static Page Table is enabled.
> + @retval FALSE Static Page Table is disabled.
> +**/
> +BOOLEAN
> +IsStaticPageTableEnabled (
> + VOID
> + )
> +;
> +
> /**
> This function reads CR2 register when on-demand paging is enabled
> for 64 bit and no action for 32 bit.
> diff --git a/UefiCpuPkg/PiSmmCpuDxeSmm/X64/PageTbl.c b/UefiCpuPkg/PiSmmCpuDxeSmm/X64/PageTbl.c
> index a3b62f7787..18e3f9e08d 100644
> --- a/UefiCpuPkg/PiSmmCpuDxeSmm/X64/PageTbl.c
> +++ b/UefiCpuPkg/PiSmmCpuDxeSmm/X64/PageTbl.c
> @@ -37,6 +37,22 @@ EnableCet (
> VOID
> );
>
> +/**
> + Return whether Static Page Table is enabled.
> +
> + Note: Static Page Table is always disabled for IA32 build.
> +
> + @retval TRUE Static Page Table is enabled.
> + @retval FALSE Static Page Table is disabled.
> +**/
> +BOOLEAN
> +IsStaticPageTableEnabled (
> + VOID
> + )
> +{
> + return mCpuSmmStaticPageTable;
> +}
> +
> /**
> Check if 1-GByte pages is supported by processor or not.
>
>
I like the approach in this patch; however I think the IA32
implementation (and comments) are wrong. The function should return
constant TRUE on IA32.
"static paging" means that all page tables used in SMM are built in
advance. When "static paging" is off (= "dynamic paging" is enabled),
then page tables are built in SMM on-demand, based on individual page
faults. (This is my understanding anyway.)
And in the IA32 build, the page tables are always built in advance, to
my understanding. Hence "static paging" should be reported as "on".
The mistake in this patch (patch#1) is apparent in patch#2. Patch#2
seems good (I'll comment on it separately), but because of the incorrect
IA32 implementation in patch#1, patch#2 ends up changing the behavior on
IA32.
Thanks
Laszlo
next prev parent reply other threads:[~2019-07-29 11:33 UTC|newest]
Thread overview: 13+ messages / expand[flat|nested] mbox.gz Atom feed top
2019-07-27 3:28 [PATCH 0/3] Allow SMM access-out when static paging is OFF Ni, Ray
2019-07-27 3:28 ` [PATCH 1/3] UefiCpuPkg/PiSmmCpu: Add Internal function IsStaticPageTableEnabled Ni, Ray
2019-07-29 11:33 ` [edk2-devel] " Dong, Eric
2019-07-29 11:33 ` Laszlo Ersek [this message]
2019-07-30 15:20 ` Ni, Ray
2019-07-31 9:57 ` Laszlo Ersek
2019-07-31 16:40 ` Ni, Ray
2019-07-27 3:28 ` [PATCH 2/3] UefiCpuPkg/PiSmmCpu: Refine CR2 save/restore logic Ni, Ray
2019-07-29 11:33 ` Dong, Eric
2019-07-29 11:42 ` [edk2-devel] " Laszlo Ersek
2019-07-27 3:28 ` [PATCH 3/3] UefiCpuPkg/PiSmmCpu: Allow SMM access-out when static paging is OFF Ni, Ray
2019-07-29 11:33 ` Dong, Eric
2019-07-29 11:49 ` [edk2-devel] " 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=1812e3c6-f388-6faa-71d9-8e878b8e9b6c@redhat.com \
--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