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.
 
-- 
2.21.0.windows.1


-=-=-=-=-=-=-=-=-=-=-=-
Groups.io Links: You receive all messages sent to this group.

View/Reply Online (#44468): https://edk2.groups.io/g/devel/message/44468
Mute This Topic: https://groups.io/mt/32616001/1787277
Group Owner: devel+owner@edk2.groups.io
Unsubscribe: https://edk2.groups.io/g/devel/unsub  [importer@patchew.org]
-=-=-=-=-=-=-=-=-=-=-=-

Reviewed-by: Eric Dong <eric.dong@intel.com>

> -----Original Message-----
> From: devel@edk2.groups.io [mailto:devel@edk2.groups.io] On Behalf Of Ni,
> Ray
> Sent: Saturday, July 27, 2019 11:29 AM
> To: devel@edk2.groups.io
> Cc: Laszlo Ersek <lersek@redhat.com>
> Subject: [edk2-devel] [PATCH 1/3] UefiCpuPkg/PiSmmCpu: Add Internal
> function IsStaticPageTableEnabled
> 
> 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.
> 
> --
> 2.21.0.windows.1
> 
> 
> 


-=-=-=-=-=-=-=-=-=-=-=-
Groups.io Links: You receive all messages sent to this group.

View/Reply Online (#44513): https://edk2.groups.io/g/devel/message/44513
Mute This Topic: https://groups.io/mt/32616001/1787277
Group Owner: devel+owner@edk2.groups.io
Unsubscribe: https://edk2.groups.io/g/devel/unsub  [importer@patchew.org]
-=-=-=-=-=-=-=-=-=-=-=-

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

-=-=-=-=-=-=-=-=-=-=-=-
Groups.io Links: You receive all messages sent to this group.

View/Reply Online (#44514): https://edk2.groups.io/g/devel/message/44514
Mute This Topic: https://groups.io/mt/32616001/1787277
Group Owner: devel+owner@edk2.groups.io
Unsubscribe: https://edk2.groups.io/g/devel/unsub  [importer@patchew.org]
-=-=-=-=-=-=-=-=-=-=-=-

> -----Original Message-----
> From: devel@edk2.groups.io <devel@edk2.groups.io> On Behalf Of Laszlo Ersek
> Sent: Monday, July 29, 2019 7:33 PM
> To: devel@edk2.groups.io; Ni, Ray <ray.ni@intel.com>
> Subject: Re: [edk2-devel] [PATCH 1/3] UefiCpuPkg/PiSmmCpu: Add Internal function IsStaticPageTableEnabled
> 
> 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".

Your understanding to the meaning of "static paging" is correct.
But later commit c60d36b4d1ee1f69b7cca897d3621dfa951895c2
* UefiCpuPkg/SmmCpu: Block access-out only when static paging is used
alters the meaning of "static paging" to a platform policy that:
  When it's true, SMM code cannot access out after EndOfDxe;
  When it's false, SMM code can access out after EndOfDxe.

Which means "static paging" can be either TRUE or FALSE in IA32 build.
Which means the PCD description in UefiCpuPkg.dec is wrong after
commit c60d36b4d1ee1f69b7cca897d3621dfa951895c2.
Right now it says:
  ## Indicates if SMM uses static page table.
  #  If enabled, SMM will not use on-demand paging. SMM will build static page table for all memory.
  #  This flag only impacts X64 build, because SMM always builds static page table for IA32.

I was confused when changing the code.

In the V2 patch, I will:
1. update UefiCpuPkg.dec to indicate it's new usage of this PCD. Name of PCD is unchanged but it's also applicable
to IA32 now;
2. No touch to the CR2 save/restore logic because that code depends on whether page table is "built in advance".
3. Add Ia32 version of mCpuSmmStaticPageTable and cache the PCD value to it.
4. Check mCpuSmmStaticPageTable and only protect non-SMRAM and page table when it's TRUE.

Do you agree?



-=-=-=-=-=-=-=-=-=-=-=-
Groups.io Links: You receive all messages sent to this group.

View/Reply Online (#44634): https://edk2.groups.io/g/devel/message/44634
Mute This Topic: https://groups.io/mt/32616001/1787277
Group Owner: devel+owner@edk2.groups.io
Unsubscribe: https://edk2.groups.io/g/devel/unsub  [importer@patchew.org]
-=-=-=-=-=-=-=-=-=-=-=-

On 07/30/19 17:20, Ni, Ray wrote:
> 
> 
>> -----Original Message-----
>> From: devel@edk2.groups.io <devel@edk2.groups.io> On Behalf Of Laszlo Ersek
>> Sent: Monday, July 29, 2019 7:33 PM
>> To: devel@edk2.groups.io; Ni, Ray <ray.ni@intel.com>
>> Subject: Re: [edk2-devel] [PATCH 1/3] UefiCpuPkg/PiSmmCpu: Add Internal function IsStaticPageTableEnabled
>>
>> 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".
> 
> Your understanding to the meaning of "static paging" is correct.
> But later commit c60d36b4d1ee1f69b7cca897d3621dfa951895c2
> * UefiCpuPkg/SmmCpu: Block access-out only when static paging is used
> alters the meaning of "static paging" to a platform policy that:
>   When it's true, SMM code cannot access out after EndOfDxe;
>   When it's false, SMM code can access out after EndOfDxe.
> 
> Which means "static paging" can be either TRUE or FALSE in IA32 build.
> Which means the PCD description in UefiCpuPkg.dec is wrong after
> commit c60d36b4d1ee1f69b7cca897d3621dfa951895c2.
> Right now it says:
>   ## Indicates if SMM uses static page table.
>   #  If enabled, SMM will not use on-demand paging. SMM will build static page table for all memory.
>   #  This flag only impacts X64 build, because SMM always builds static page table for IA32.
> 
> I was confused when changing the code.
> 
> In the V2 patch, I will:
> 1. update UefiCpuPkg.dec to indicate it's new usage of this PCD. Name of PCD is unchanged but it's also applicable
> to IA32 now;
> 2. No touch to the CR2 save/restore logic because that code depends on whether page table is "built in advance".
> 3. Add Ia32 version of mCpuSmmStaticPageTable and cache the PCD value to it.
> 4. Check mCpuSmmStaticPageTable and only protect non-SMRAM and page table when it's TRUE.
> 
> Do you agree?

I find it extremely confusing to have a PCD which says "StaticPageTable"
in the name, but controls something else entirely in the IA32 build of
PiSmmCpuDxeSmm.

Because, IA32 continues to use statically built page tables. Setting a
PCD that says "StaticPageTable" in the name to FALSE, but expecting the
page fault handler to *only* change access-out behavior, is wrong. It
makes "PcdCpuSmmStaticPageTable" a misnomer.

I'm starting to doubt whether the "simplification" in commit
c60d36b4d1ee was a good idea. It looks like that commit repurposed
PcdCpuSmmStaticPageTable for something that the PCD was never meant for.

In my opinion, there are two alternatives:

(1) Restore the original meaning of PcdCpuSmmStaticPageTable, and
introduce a *new* feature or boolean PCD for controlling access-out. The
X64 code would use both PCDs (for different purposes, of course). The
IA32 code would only use the new (access-out) PCD. If necessary, catch
the invalid combinations of both PCDs (static page table, versus
access-out) in the entry point function of PiSmmCpuDxeSmm, with
ASSERT()s. I think it possible that out of the 2*2=4 variations only 3
are valid.

(2) Alternatively, if it turns out that the original purpose of
PcdCpuSmmStaticPageTable is actually a one-to-one match with the new
access-out PCD -- in other words, if it turns out that the access-out
control *inherently* maps to static paging, and *not* just as a
simplification --, then we should not introduce a new PCD. However, in
that case, both "mCpuSmmStaticPageTable" and "PcdCpuSmmStaticPageTable"
should be renamed to explain this dual purpose.

Basically, my alternative (2) is what you are proposing, except I'd also
like to see the PCD and the global variable renamed, in that case.

However, what's more important is to first re-evaluate whether the
"simplification" in commit c60d36b4d1ee was valid. I'm quite unconvinced
that using PcdCpuSmmStaticPageTable *additionally* for controlling
access outside of SMRAM is inherently necessary.


As I understand it, "access-out" is a characteristic that both IA32 and
X64 platforms may reasonably want to control. "Static page tables" is a
different characteristic that only X64 platforms may reasonably want to
control. I think we need separate PCDs.

If we can, we should even make "PcdCpuSmmStaticPageTable" an X64-only
PCD -- code built for IA32 that references the PCD shouldn't even
compile. I think the DEC file syntax makes such restrictions possible.

Thanks
Laszlo

-=-=-=-=-=-=-=-=-=-=-=-
Groups.io Links: You receive all messages sent to this group.

View/Reply Online (#44674): https://edk2.groups.io/g/devel/message/44674
Mute This Topic: https://groups.io/mt/32616001/1787277
Group Owner: devel+owner@edk2.groups.io
Unsubscribe: https://edk2.groups.io/g/devel/unsub  [importer@patchew.org]
-=-=-=-=-=-=-=-=-=-=-=-

Hi Laszlo,
I agree with your points of creating a new PCD.
It also reduces confusions. I just posted V2 patch. Please help to review.

Thanks,
Ray


-=-=-=-=-=-=-=-=-=-=-=-
Groups.io Links: You receive all messages sent to this group.

View/Reply Online (#44689): https://edk2.groups.io/g/devel/message/44689
Mute This Topic: https://groups.io/mt/32616001/1787277
Group Owner: devel+owner@edk2.groups.io
Unsubscribe: https://edk2.groups.io/g/devel/unsub  [importer@patchew.org]
-=-=-=-=-=-=-=-=-=-=-=-