* [PATCH 0/3] Allow SMM access-out when static paging is OFF @ 2019-07-27 3:28 Ni, Ray 2019-07-27 3:28 ` [PATCH 1/3] UefiCpuPkg/PiSmmCpu: Add Internal function IsStaticPageTableEnabled Ni, Ray ` (2 more replies) 0 siblings, 3 replies; 13+ messages in thread From: Ni, Ray @ 2019-07-27 3:28 UTC (permalink / raw) To: devel The patch set refines the original CR2 save/restore logic by adding a new internal function IsStaticPageTableEnabled() because now there are two use cases that need to conditionally do something based on whether static page table is enabled. Based on the refine, the patch changes the policy to allow SMM access out when static paging is OFF. Ray Ni (3): UefiCpuPkg/PiSmmCpu: Add Internal function IsStaticPageTableEnabled UefiCpuPkg/PiSmmCpu: Refine CR2 save/restore logic UefiCpuPkg/PiSmmCpu: Allow SMM access-out when static paging is OFF UefiCpuPkg/PiSmmCpuDxeSmm/Ia32/PageTbl.c | 41 ++++++++------------ UefiCpuPkg/PiSmmCpuDxeSmm/MpService.c | 8 +++- UefiCpuPkg/PiSmmCpuDxeSmm/PiSmmCpuDxeSmm.c | 21 +++++++--- UefiCpuPkg/PiSmmCpuDxeSmm/PiSmmCpuDxeSmm.h | 27 +++++-------- UefiCpuPkg/PiSmmCpuDxeSmm/X64/PageTbl.c | 45 ++++++++-------------- 5 files changed, 63 insertions(+), 79 deletions(-) -- 2.21.0.windows.1 ^ permalink raw reply [flat|nested] 13+ messages in thread
* [PATCH 1/3] UefiCpuPkg/PiSmmCpu: Add Internal function IsStaticPageTableEnabled 2019-07-27 3:28 [PATCH 0/3] Allow SMM access-out when static paging is OFF Ni, Ray @ 2019-07-27 3:28 ` Ni, Ray 2019-07-29 11:33 ` [edk2-devel] " Dong, Eric 2019-07-29 11:33 ` Laszlo Ersek 2019-07-27 3:28 ` [PATCH 2/3] UefiCpuPkg/PiSmmCpu: Refine CR2 save/restore logic Ni, Ray 2019-07-27 3:28 ` [PATCH 3/3] UefiCpuPkg/PiSmmCpu: Allow SMM access-out when static paging is OFF Ni, Ray 2 siblings, 2 replies; 13+ messages in thread From: Ni, Ray @ 2019-07-27 3:28 UTC (permalink / raw) To: devel; +Cc: Laszlo Ersek 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 ^ permalink raw reply related [flat|nested] 13+ messages in thread
* Re: [edk2-devel] [PATCH 1/3] UefiCpuPkg/PiSmmCpu: Add Internal function IsStaticPageTableEnabled 2019-07-27 3:28 ` [PATCH 1/3] UefiCpuPkg/PiSmmCpu: Add Internal function IsStaticPageTableEnabled Ni, Ray @ 2019-07-29 11:33 ` Dong, Eric 2019-07-29 11:33 ` Laszlo Ersek 1 sibling, 0 replies; 13+ messages in thread From: Dong, Eric @ 2019-07-29 11:33 UTC (permalink / raw) To: devel@edk2.groups.io, Ni, Ray; +Cc: Laszlo Ersek 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 > > > ^ permalink raw reply [flat|nested] 13+ messages in thread
* Re: [edk2-devel] [PATCH 1/3] UefiCpuPkg/PiSmmCpu: Add Internal function IsStaticPageTableEnabled 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 2019-07-30 15:20 ` Ni, Ray 1 sibling, 1 reply; 13+ messages in thread From: Laszlo Ersek @ 2019-07-29 11:33 UTC (permalink / raw) To: devel, ray.ni 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 ^ permalink raw reply [flat|nested] 13+ messages in thread
* Re: [edk2-devel] [PATCH 1/3] UefiCpuPkg/PiSmmCpu: Add Internal function IsStaticPageTableEnabled 2019-07-29 11:33 ` Laszlo Ersek @ 2019-07-30 15:20 ` Ni, Ray 2019-07-31 9:57 ` Laszlo Ersek 0 siblings, 1 reply; 13+ messages in thread From: Ni, Ray @ 2019-07-30 15:20 UTC (permalink / raw) To: devel@edk2.groups.io, lersek@redhat.com > -----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? ^ permalink raw reply [flat|nested] 13+ messages in thread
* Re: [edk2-devel] [PATCH 1/3] UefiCpuPkg/PiSmmCpu: Add Internal function IsStaticPageTableEnabled 2019-07-30 15:20 ` Ni, Ray @ 2019-07-31 9:57 ` Laszlo Ersek 2019-07-31 16:40 ` Ni, Ray 0 siblings, 1 reply; 13+ messages in thread From: Laszlo Ersek @ 2019-07-31 9:57 UTC (permalink / raw) To: Ni, Ray, devel@edk2.groups.io 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 ^ permalink raw reply [flat|nested] 13+ messages in thread
* Re: [edk2-devel] [PATCH 1/3] UefiCpuPkg/PiSmmCpu: Add Internal function IsStaticPageTableEnabled 2019-07-31 9:57 ` Laszlo Ersek @ 2019-07-31 16:40 ` Ni, Ray 0 siblings, 0 replies; 13+ messages in thread From: Ni, Ray @ 2019-07-31 16:40 UTC (permalink / raw) To: devel@edk2.groups.io, lersek@redhat.com 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 ^ permalink raw reply [flat|nested] 13+ messages in thread
* [PATCH 2/3] UefiCpuPkg/PiSmmCpu: Refine CR2 save/restore logic 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-27 3:28 ` 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 2 siblings, 2 replies; 13+ messages in thread From: Ni, Ray @ 2019-07-27 3:28 UTC (permalink / raw) To: devel; +Cc: Eric Dong, Laszlo Ersek, Vanguput Narendra K Because IsStaticPageTableEnabled() is added for both IA32 and x64 build, the CR2 save/restore logic can be refined: 1. Remove arch specific SaveCr2() / RestoreCr2() implementation; 2. Conditionally save and restore CR2 in SmiRendezvous(). Signed-off-by: Ray Ni <ray.ni@intel.com> Cc: Eric Dong <eric.dong@intel.com> Cc: Laszlo Ersek <lersek@redhat.com> Cc: Vanguput Narendra K <narendra.k.vanguput@intel.com> --- UefiCpuPkg/PiSmmCpuDxeSmm/Ia32/PageTbl.c | 25 ------------------- UefiCpuPkg/PiSmmCpuDxeSmm/MpService.c | 8 ++++-- UefiCpuPkg/PiSmmCpuDxeSmm/PiSmmCpuDxeSmm.h | 22 ---------------- UefiCpuPkg/PiSmmCpuDxeSmm/X64/PageTbl.c | 29 ---------------------- 4 files changed, 6 insertions(+), 78 deletions(-) diff --git a/UefiCpuPkg/PiSmmCpuDxeSmm/Ia32/PageTbl.c b/UefiCpuPkg/PiSmmCpuDxeSmm/Ia32/PageTbl.c index 2a9af4b77d..cae23d6d1d 100644 --- a/UefiCpuPkg/PiSmmCpuDxeSmm/Ia32/PageTbl.c +++ b/UefiCpuPkg/PiSmmCpuDxeSmm/Ia32/PageTbl.c @@ -327,28 +327,3 @@ SetPageTableAttributes ( return ; } -/** - This function returns with no action for 32 bit. - - @param[out] *Cr2 Pointer to variable to hold CR2 register value. -**/ -VOID -SaveCr2 ( - OUT UINTN *Cr2 - ) -{ - return ; -} - -/** - This function returns with no action for 32 bit. - - @param[in] Cr2 Value to write into CR2 register. -**/ -VOID -RestoreCr2 ( - IN UINTN Cr2 - ) -{ - return ; -} diff --git a/UefiCpuPkg/PiSmmCpuDxeSmm/MpService.c b/UefiCpuPkg/PiSmmCpuDxeSmm/MpService.c index ef16997547..5d0124b368 100644 --- a/UefiCpuPkg/PiSmmCpuDxeSmm/MpService.c +++ b/UefiCpuPkg/PiSmmCpuDxeSmm/MpService.c @@ -1575,7 +1575,9 @@ SmiRendezvous ( // when using on-demand paging for above 4G memory. // Cr2 = 0; - SaveCr2 (&Cr2); + if (!IsStaticPageTableEnabled ()) { + Cr2 = AsmReadCr2 (); + } // // Call the user register Startup function first. @@ -1725,7 +1727,9 @@ Exit: // // Restore Cr2 // - RestoreCr2 (Cr2); + if (!IsStaticPageTableEnabled ()) { + AsmWriteCr2 (Cr2); + } } /** diff --git a/UefiCpuPkg/PiSmmCpuDxeSmm/PiSmmCpuDxeSmm.h b/UefiCpuPkg/PiSmmCpuDxeSmm/PiSmmCpuDxeSmm.h index 14b7676c16..5a97733def 100644 --- a/UefiCpuPkg/PiSmmCpuDxeSmm/PiSmmCpuDxeSmm.h +++ b/UefiCpuPkg/PiSmmCpuDxeSmm/PiSmmCpuDxeSmm.h @@ -1282,28 +1282,6 @@ IsStaticPageTableEnabled ( ) ; -/** - This function reads CR2 register when on-demand paging is enabled - for 64 bit and no action for 32 bit. - - @param[out] *Cr2 Pointer to variable to hold CR2 register value. -**/ -VOID -SaveCr2 ( - OUT UINTN *Cr2 - ); - -/** - This function writes into CR2 register when on-demand paging is enabled - for 64 bit and no action for 32 bit. - - @param[in] Cr2 Value to write into CR2 register. -**/ -VOID -RestoreCr2 ( - IN UINTN Cr2 - ); - /** Schedule a procedure to run on the specified CPU. diff --git a/UefiCpuPkg/PiSmmCpuDxeSmm/X64/PageTbl.c b/UefiCpuPkg/PiSmmCpuDxeSmm/X64/PageTbl.c index 18e3f9e08d..8259b01a95 100644 --- a/UefiCpuPkg/PiSmmCpuDxeSmm/X64/PageTbl.c +++ b/UefiCpuPkg/PiSmmCpuDxeSmm/X64/PageTbl.c @@ -1209,32 +1209,3 @@ SetPageTableAttributes ( return ; } -/** - This function reads CR2 register when on-demand paging is enabled. - - @param[out] *Cr2 Pointer to variable to hold CR2 register value. -**/ -VOID -SaveCr2 ( - OUT UINTN *Cr2 - ) -{ - if (!mCpuSmmStaticPageTable) { - *Cr2 = AsmReadCr2 (); - } -} - -/** - This function restores CR2 register when on-demand paging is enabled. - - @param[in] Cr2 Value to write into CR2 register. -**/ -VOID -RestoreCr2 ( - IN UINTN Cr2 - ) -{ - if (!mCpuSmmStaticPageTable) { - AsmWriteCr2 (Cr2); - } -} -- 2.21.0.windows.1 ^ permalink raw reply related [flat|nested] 13+ messages in thread
* Re: [PATCH 2/3] UefiCpuPkg/PiSmmCpu: Refine CR2 save/restore logic 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 1 sibling, 0 replies; 13+ messages in thread From: Dong, Eric @ 2019-07-29 11:33 UTC (permalink / raw) To: Ni, Ray, devel@edk2.groups.io; +Cc: Laszlo Ersek, Vanguput, Narendra K Reviewed-by: Eric Dong <eric.dong@intel.com> > -----Original Message----- > From: Ni, Ray > Sent: Saturday, July 27, 2019 11:29 AM > To: devel@edk2.groups.io > Cc: Dong, Eric <eric.dong@intel.com>; Laszlo Ersek <lersek@redhat.com>; > Vanguput, Narendra K <narendra.k.vanguput@intel.com> > Subject: [PATCH 2/3] UefiCpuPkg/PiSmmCpu: Refine CR2 save/restore logic > > Because IsStaticPageTableEnabled() is added for both IA32 and x64 build, the > CR2 save/restore logic can be refined: > 1. Remove arch specific SaveCr2() / RestoreCr2() implementation; 2. > Conditionally save and restore CR2 in SmiRendezvous(). > > Signed-off-by: Ray Ni <ray.ni@intel.com> > Cc: Eric Dong <eric.dong@intel.com> > Cc: Laszlo Ersek <lersek@redhat.com> > Cc: Vanguput Narendra K <narendra.k.vanguput@intel.com> > --- > UefiCpuPkg/PiSmmCpuDxeSmm/Ia32/PageTbl.c | 25 ------------------- > UefiCpuPkg/PiSmmCpuDxeSmm/MpService.c | 8 ++++-- > UefiCpuPkg/PiSmmCpuDxeSmm/PiSmmCpuDxeSmm.h | 22 ---------------- > UefiCpuPkg/PiSmmCpuDxeSmm/X64/PageTbl.c | 29 ---------------------- > 4 files changed, 6 insertions(+), 78 deletions(-) > > diff --git a/UefiCpuPkg/PiSmmCpuDxeSmm/Ia32/PageTbl.c > b/UefiCpuPkg/PiSmmCpuDxeSmm/Ia32/PageTbl.c > index 2a9af4b77d..cae23d6d1d 100644 > --- a/UefiCpuPkg/PiSmmCpuDxeSmm/Ia32/PageTbl.c > +++ b/UefiCpuPkg/PiSmmCpuDxeSmm/Ia32/PageTbl.c > @@ -327,28 +327,3 @@ SetPageTableAttributes ( > return ; > } > > -/** > - This function returns with no action for 32 bit. > - > - @param[out] *Cr2 Pointer to variable to hold CR2 register value. > -**/ > -VOID > -SaveCr2 ( > - OUT UINTN *Cr2 > - ) > -{ > - return ; > -} > - > -/** > - This function returns with no action for 32 bit. > - > - @param[in] Cr2 Value to write into CR2 register. > -**/ > -VOID > -RestoreCr2 ( > - IN UINTN Cr2 > - ) > -{ > - return ; > -} > diff --git a/UefiCpuPkg/PiSmmCpuDxeSmm/MpService.c > b/UefiCpuPkg/PiSmmCpuDxeSmm/MpService.c > index ef16997547..5d0124b368 100644 > --- a/UefiCpuPkg/PiSmmCpuDxeSmm/MpService.c > +++ b/UefiCpuPkg/PiSmmCpuDxeSmm/MpService.c > @@ -1575,7 +1575,9 @@ SmiRendezvous ( > // when using on-demand paging for above 4G memory. > // > Cr2 = 0; > - SaveCr2 (&Cr2); > + if (!IsStaticPageTableEnabled ()) { > + Cr2 = AsmReadCr2 (); > + } > > // > // Call the user register Startup function first. > @@ -1725,7 +1727,9 @@ Exit: > // > // Restore Cr2 > // > - RestoreCr2 (Cr2); > + if (!IsStaticPageTableEnabled ()) { > + AsmWriteCr2 (Cr2); > + } > } > > /** > diff --git a/UefiCpuPkg/PiSmmCpuDxeSmm/PiSmmCpuDxeSmm.h > b/UefiCpuPkg/PiSmmCpuDxeSmm/PiSmmCpuDxeSmm.h > index 14b7676c16..5a97733def 100644 > --- a/UefiCpuPkg/PiSmmCpuDxeSmm/PiSmmCpuDxeSmm.h > +++ b/UefiCpuPkg/PiSmmCpuDxeSmm/PiSmmCpuDxeSmm.h > @@ -1282,28 +1282,6 @@ IsStaticPageTableEnabled ( > ) > ; > > -/** > - This function reads CR2 register when on-demand paging is enabled > - for 64 bit and no action for 32 bit. > - > - @param[out] *Cr2 Pointer to variable to hold CR2 register value. > -**/ > -VOID > -SaveCr2 ( > - OUT UINTN *Cr2 > - ); > - > -/** > - This function writes into CR2 register when on-demand paging is enabled > - for 64 bit and no action for 32 bit. > - > - @param[in] Cr2 Value to write into CR2 register. > -**/ > -VOID > -RestoreCr2 ( > - IN UINTN Cr2 > - ); > - > /** > Schedule a procedure to run on the specified CPU. > > diff --git a/UefiCpuPkg/PiSmmCpuDxeSmm/X64/PageTbl.c > b/UefiCpuPkg/PiSmmCpuDxeSmm/X64/PageTbl.c > index 18e3f9e08d..8259b01a95 100644 > --- a/UefiCpuPkg/PiSmmCpuDxeSmm/X64/PageTbl.c > +++ b/UefiCpuPkg/PiSmmCpuDxeSmm/X64/PageTbl.c > @@ -1209,32 +1209,3 @@ SetPageTableAttributes ( > return ; > } > > -/** > - This function reads CR2 register when on-demand paging is enabled. > - > - @param[out] *Cr2 Pointer to variable to hold CR2 register value. > -**/ > -VOID > -SaveCr2 ( > - OUT UINTN *Cr2 > - ) > -{ > - if (!mCpuSmmStaticPageTable) { > - *Cr2 = AsmReadCr2 (); > - } > -} > - > -/** > - This function restores CR2 register when on-demand paging is enabled. > - > - @param[in] Cr2 Value to write into CR2 register. > -**/ > -VOID > -RestoreCr2 ( > - IN UINTN Cr2 > - ) > -{ > - if (!mCpuSmmStaticPageTable) { > - AsmWriteCr2 (Cr2); > - } > -} > -- > 2.21.0.windows.1 ^ permalink raw reply [flat|nested] 13+ messages in thread
* Re: [edk2-devel] [PATCH 2/3] UefiCpuPkg/PiSmmCpu: Refine CR2 save/restore logic 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 ` Laszlo Ersek 1 sibling, 0 replies; 13+ messages in thread From: Laszlo Ersek @ 2019-07-29 11:42 UTC (permalink / raw) To: devel, ray.ni; +Cc: Eric Dong, Vanguput Narendra K On 07/27/19 05:28, Ni, Ray wrote: > Because IsStaticPageTableEnabled() is added for both IA32 and x64 > build, the CR2 save/restore logic can be refined: > 1. Remove arch specific SaveCr2() / RestoreCr2() implementation; > 2. Conditionally save and restore CR2 in SmiRendezvous(). > > Signed-off-by: Ray Ni <ray.ni@intel.com> > Cc: Eric Dong <eric.dong@intel.com> > Cc: Laszlo Ersek <lersek@redhat.com> > Cc: Vanguput Narendra K <narendra.k.vanguput@intel.com> > --- > UefiCpuPkg/PiSmmCpuDxeSmm/Ia32/PageTbl.c | 25 ------------------- > UefiCpuPkg/PiSmmCpuDxeSmm/MpService.c | 8 ++++-- > UefiCpuPkg/PiSmmCpuDxeSmm/PiSmmCpuDxeSmm.h | 22 ---------------- > UefiCpuPkg/PiSmmCpuDxeSmm/X64/PageTbl.c | 29 ---------------------- > 4 files changed, 6 insertions(+), 78 deletions(-) > > diff --git a/UefiCpuPkg/PiSmmCpuDxeSmm/Ia32/PageTbl.c b/UefiCpuPkg/PiSmmCpuDxeSmm/Ia32/PageTbl.c > index 2a9af4b77d..cae23d6d1d 100644 > --- a/UefiCpuPkg/PiSmmCpuDxeSmm/Ia32/PageTbl.c > +++ b/UefiCpuPkg/PiSmmCpuDxeSmm/Ia32/PageTbl.c > @@ -327,28 +327,3 @@ SetPageTableAttributes ( > return ; > } > > -/** > - This function returns with no action for 32 bit. > - > - @param[out] *Cr2 Pointer to variable to hold CR2 register value. > -**/ > -VOID > -SaveCr2 ( > - OUT UINTN *Cr2 > - ) > -{ > - return ; > -} > - > -/** > - This function returns with no action for 32 bit. > - > - @param[in] Cr2 Value to write into CR2 register. > -**/ > -VOID > -RestoreCr2 ( > - IN UINTN Cr2 > - ) > -{ > - return ; > -} > diff --git a/UefiCpuPkg/PiSmmCpuDxeSmm/MpService.c b/UefiCpuPkg/PiSmmCpuDxeSmm/MpService.c > index ef16997547..5d0124b368 100644 > --- a/UefiCpuPkg/PiSmmCpuDxeSmm/MpService.c > +++ b/UefiCpuPkg/PiSmmCpuDxeSmm/MpService.c > @@ -1575,7 +1575,9 @@ SmiRendezvous ( > // when using on-demand paging for above 4G memory. > // > Cr2 = 0; > - SaveCr2 (&Cr2); > + if (!IsStaticPageTableEnabled ()) { > + Cr2 = AsmReadCr2 (); > + } > > // > // Call the user register Startup function first. So, because this patch is supposed to only refactor / simplify the code, it should not change behavior. But, because in patch#1 we return FALSE for IA32, the condition above will evaluate to TRUE. And so we will massage CR2 (= fault address), even though the IA32 build shouldn't do that (and doesn't do it, at the moment). This should be fixed by returning constant TRUE from IsStaticPageTableEnabled(), in patch#1, on IA32. (Note: in the message of commit d47b85a621ad ("Revert "UefiCpuPkg/PiSmmCpu: Allow SMM access-out when static paging is OFF"", 2019-07-26), I wrote that "The IA32 implementation should return a constant value". I didn't say either "constant TRUE" or "constant FALSE". And that's because I couldn't know the right value, without actually looking at the code. Determining the correct IA32 value was out of scope for the revert.) More below: > @@ -1725,7 +1727,9 @@ Exit: > // > // Restore Cr2 > // > - RestoreCr2 (Cr2); > + if (!IsStaticPageTableEnabled ()) { > + AsmWriteCr2 (Cr2); > + } > } > > /** > diff --git a/UefiCpuPkg/PiSmmCpuDxeSmm/PiSmmCpuDxeSmm.h b/UefiCpuPkg/PiSmmCpuDxeSmm/PiSmmCpuDxeSmm.h > index 14b7676c16..5a97733def 100644 > --- a/UefiCpuPkg/PiSmmCpuDxeSmm/PiSmmCpuDxeSmm.h > +++ b/UefiCpuPkg/PiSmmCpuDxeSmm/PiSmmCpuDxeSmm.h > @@ -1282,28 +1282,6 @@ IsStaticPageTableEnabled ( > ) > ; > > -/** > - This function reads CR2 register when on-demand paging is enabled > - for 64 bit and no action for 32 bit. > - > - @param[out] *Cr2 Pointer to variable to hold CR2 register value. > -**/ > -VOID > -SaveCr2 ( > - OUT UINTN *Cr2 > - ); > - > -/** > - This function writes into CR2 register when on-demand paging is enabled > - for 64 bit and no action for 32 bit. > - > - @param[in] Cr2 Value to write into CR2 register. > -**/ > -VOID > -RestoreCr2 ( > - IN UINTN Cr2 > - ); > - > /** > Schedule a procedure to run on the specified CPU. > > diff --git a/UefiCpuPkg/PiSmmCpuDxeSmm/X64/PageTbl.c b/UefiCpuPkg/PiSmmCpuDxeSmm/X64/PageTbl.c > index 18e3f9e08d..8259b01a95 100644 > --- a/UefiCpuPkg/PiSmmCpuDxeSmm/X64/PageTbl.c > +++ b/UefiCpuPkg/PiSmmCpuDxeSmm/X64/PageTbl.c > @@ -1209,32 +1209,3 @@ SetPageTableAttributes ( > return ; > } > > -/** > - This function reads CR2 register when on-demand paging is enabled. > - > - @param[out] *Cr2 Pointer to variable to hold CR2 register value. > -**/ > -VOID > -SaveCr2 ( > - OUT UINTN *Cr2 > - ) > -{ > - if (!mCpuSmmStaticPageTable) { > - *Cr2 = AsmReadCr2 (); > - } > -} > - > -/** > - This function restores CR2 register when on-demand paging is enabled. > - > - @param[in] Cr2 Value to write into CR2 register. > -**/ > -VOID > -RestoreCr2 ( > - IN UINTN Cr2 > - ) > -{ > - if (!mCpuSmmStaticPageTable) { > - AsmWriteCr2 (Cr2); > - } > -} > For this patch: Reviewed-by: Laszlo Ersek <lersek@redhat.com> Thanks Laszlo ^ permalink raw reply [flat|nested] 13+ messages in thread
* [PATCH 3/3] UefiCpuPkg/PiSmmCpu: Allow SMM access-out when static paging is OFF 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-27 3:28 ` [PATCH 2/3] UefiCpuPkg/PiSmmCpu: Refine CR2 save/restore logic Ni, Ray @ 2019-07-27 3:28 ` Ni, Ray 2019-07-29 11:33 ` Dong, Eric 2019-07-29 11:49 ` [edk2-devel] " Laszlo Ersek 2 siblings, 2 replies; 13+ messages in thread From: Ni, Ray @ 2019-07-27 3:28 UTC (permalink / raw) To: devel; +Cc: Eric Dong, Jiewen Yao, Jian J Wang Commit c60d36b4d1ee1f69b7cca897d3621dfa951895c2 * UefiCpuPkg/SmmCpu: Block access-out only when static paging is used updated page fault handler to treat SMM access-out as allowed address when static paging is not used. But that commit is not complete because the page table is still updated in SetUefiMemMapAttributes() for non-SMRAM memory. When SMM code accesses non-SMRAM memory, page fault is still generated. This patch skips to update page table for non-SMRAM memory and page table itself. Signed-off-by: Ray Ni <ray.ni@intel.com> Cc: Eric Dong <eric.dong@intel.com> Cc: Jiewen Yao <jiewen.yao@intel.com> Cc: Jian J Wang <jian.j.wang@intel.com> --- UefiCpuPkg/PiSmmCpuDxeSmm/PiSmmCpuDxeSmm.c | 21 +++++++++++++++------ 1 file changed, 15 insertions(+), 6 deletions(-) diff --git a/UefiCpuPkg/PiSmmCpuDxeSmm/PiSmmCpuDxeSmm.c b/UefiCpuPkg/PiSmmCpuDxeSmm/PiSmmCpuDxeSmm.c index 69a04dfb23..d7d94c8b6d 100644 --- a/UefiCpuPkg/PiSmmCpuDxeSmm/PiSmmCpuDxeSmm.c +++ b/UefiCpuPkg/PiSmmCpuDxeSmm/PiSmmCpuDxeSmm.c @@ -1121,6 +1121,9 @@ FindSmramInfo ( *SmrrBase = (UINT32)CurrentSmramRange->CpuStart; *SmrrSize = (UINT32)CurrentSmramRange->PhysicalSize; + // + // Extend *SmrrBase/*SmrrSize to include adjacent SMRAM ranges + // do { Found = FALSE; for (Index = 0; Index < mSmmCpuSmramRangeCount; Index++) { @@ -1432,14 +1435,20 @@ PerformRemainingTasks ( SetMemMapAttributes (); // - // For outside SMRAM, we only map SMM communication buffer or MMIO. + // Protect memory outside SMRAM when SMM Static Page Table is enabled. // - SetUefiMemMapAttributes (); + if (IsStaticPageTableEnabled ()) { - // - // Set page table itself to be read-only - // - SetPageTableAttributes (); + // + // For outside SMRAM, we only map SMM communication buffer or MMIO. + // + SetUefiMemMapAttributes (); + + // + // Set page table itself to be read-only + // + SetPageTableAttributes (); + } // // Configure SMM Code Access Check feature if available. -- 2.21.0.windows.1 ^ permalink raw reply related [flat|nested] 13+ messages in thread
* Re: [PATCH 3/3] UefiCpuPkg/PiSmmCpu: Allow SMM access-out when static paging is OFF 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 1 sibling, 0 replies; 13+ messages in thread From: Dong, Eric @ 2019-07-29 11:33 UTC (permalink / raw) To: Ni, Ray, devel@edk2.groups.io; +Cc: Yao, Jiewen, Wang, Jian J Reviewed-by: Eric Dong <eric.dong@intel.com> > -----Original Message----- > From: Ni, Ray > Sent: Saturday, July 27, 2019 11:29 AM > To: devel@edk2.groups.io > Cc: Dong, Eric <eric.dong@intel.com>; Yao, Jiewen <jiewen.yao@intel.com>; > Wang, Jian J <jian.j.wang@intel.com> > Subject: [PATCH 3/3] UefiCpuPkg/PiSmmCpu: Allow SMM access-out when > static paging is OFF > > Commit c60d36b4d1ee1f69b7cca897d3621dfa951895c2 > * UefiCpuPkg/SmmCpu: Block access-out only when static paging is used > > updated page fault handler to treat SMM access-out as allowed address > when static paging is not used. > > But that commit is not complete because the page table is still updated in > SetUefiMemMapAttributes() for non-SMRAM memory. When SMM code > accesses non-SMRAM memory, page fault is still generated. > > This patch skips to update page table for non-SMRAM memory and page > table itself. > > Signed-off-by: Ray Ni <ray.ni@intel.com> > Cc: Eric Dong <eric.dong@intel.com> > Cc: Jiewen Yao <jiewen.yao@intel.com> > Cc: Jian J Wang <jian.j.wang@intel.com> > --- > UefiCpuPkg/PiSmmCpuDxeSmm/PiSmmCpuDxeSmm.c | 21 > +++++++++++++++------ > 1 file changed, 15 insertions(+), 6 deletions(-) > > diff --git a/UefiCpuPkg/PiSmmCpuDxeSmm/PiSmmCpuDxeSmm.c > b/UefiCpuPkg/PiSmmCpuDxeSmm/PiSmmCpuDxeSmm.c > index 69a04dfb23..d7d94c8b6d 100644 > --- a/UefiCpuPkg/PiSmmCpuDxeSmm/PiSmmCpuDxeSmm.c > +++ b/UefiCpuPkg/PiSmmCpuDxeSmm/PiSmmCpuDxeSmm.c > @@ -1121,6 +1121,9 @@ FindSmramInfo ( > *SmrrBase = (UINT32)CurrentSmramRange->CpuStart; > *SmrrSize = (UINT32)CurrentSmramRange->PhysicalSize; > > + // > + // Extend *SmrrBase/*SmrrSize to include adjacent SMRAM ranges // > do { > Found = FALSE; > for (Index = 0; Index < mSmmCpuSmramRangeCount; Index++) { @@ - > 1432,14 +1435,20 @@ PerformRemainingTasks ( > SetMemMapAttributes (); > > // > - // For outside SMRAM, we only map SMM communication buffer or > MMIO. > + // Protect memory outside SMRAM when SMM Static Page Table is > enabled. > // > - SetUefiMemMapAttributes (); > + if (IsStaticPageTableEnabled ()) { > > - // > - // Set page table itself to be read-only > - // > - SetPageTableAttributes (); > + // > + // For outside SMRAM, we only map SMM communication buffer or > MMIO. > + // > + SetUefiMemMapAttributes (); > + > + // > + // Set page table itself to be read-only > + // > + SetPageTableAttributes (); > + } > > // > // Configure SMM Code Access Check feature if available. > -- > 2.21.0.windows.1 ^ permalink raw reply [flat|nested] 13+ messages in thread
* Re: [edk2-devel] [PATCH 3/3] UefiCpuPkg/PiSmmCpu: Allow SMM access-out when static paging is OFF 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 ` Laszlo Ersek 1 sibling, 0 replies; 13+ messages in thread From: Laszlo Ersek @ 2019-07-29 11:49 UTC (permalink / raw) To: devel, ray.ni; +Cc: Eric Dong, Jiewen Yao, Jian J Wang On 07/27/19 05:28, Ni, Ray wrote: > Commit c60d36b4d1ee1f69b7cca897d3621dfa951895c2 > * UefiCpuPkg/SmmCpu: Block access-out only when static paging is used > > updated page fault handler to treat SMM access-out as allowed > address when static paging is not used. > > But that commit is not complete because the page table is still > updated in SetUefiMemMapAttributes() for non-SMRAM memory. When SMM > code accesses non-SMRAM memory, page fault is still generated. > > This patch skips to update page table for non-SMRAM memory and > page table itself. > > Signed-off-by: Ray Ni <ray.ni@intel.com> > Cc: Eric Dong <eric.dong@intel.com> > Cc: Jiewen Yao <jiewen.yao@intel.com> > Cc: Jian J Wang <jian.j.wang@intel.com> > --- > UefiCpuPkg/PiSmmCpuDxeSmm/PiSmmCpuDxeSmm.c | 21 +++++++++++++++------ > 1 file changed, 15 insertions(+), 6 deletions(-) > > diff --git a/UefiCpuPkg/PiSmmCpuDxeSmm/PiSmmCpuDxeSmm.c b/UefiCpuPkg/PiSmmCpuDxeSmm/PiSmmCpuDxeSmm.c > index 69a04dfb23..d7d94c8b6d 100644 > --- a/UefiCpuPkg/PiSmmCpuDxeSmm/PiSmmCpuDxeSmm.c > +++ b/UefiCpuPkg/PiSmmCpuDxeSmm/PiSmmCpuDxeSmm.c > @@ -1121,6 +1121,9 @@ FindSmramInfo ( > *SmrrBase = (UINT32)CurrentSmramRange->CpuStart; > *SmrrSize = (UINT32)CurrentSmramRange->PhysicalSize; > > + // > + // Extend *SmrrBase/*SmrrSize to include adjacent SMRAM ranges > + // > do { > Found = FALSE; > for (Index = 0; Index < mSmmCpuSmramRangeCount; Index++) { > @@ -1432,14 +1435,20 @@ PerformRemainingTasks ( > SetMemMapAttributes (); > > // > - // For outside SMRAM, we only map SMM communication buffer or MMIO. > + // Protect memory outside SMRAM when SMM Static Page Table is enabled. > // > - SetUefiMemMapAttributes (); > + if (IsStaticPageTableEnabled ()) { > > - // > - // Set page table itself to be read-only > - // > - SetPageTableAttributes (); > + // > + // For outside SMRAM, we only map SMM communication buffer or MMIO. > + // > + SetUefiMemMapAttributes (); > + > + // > + // Set page table itself to be read-only > + // > + SetPageTableAttributes (); > + } > > // > // Configure SMM Code Access Check feature if available. > With the IA32 bug in patch#1 fixed, this patch will also do the right thing (as part of OVMF anyway): - on IA32, there will be no change in behavior (IsStaticPageTableEnabled() will evaluate to constant TRUE) - on X64, OVMF keeps the DEC-default TRUE value for the underlying PCD (PcdCpuSmmStaticPageTable), hence IsStaticPageTableEnabled() will return TRUE, and the behavior won't change. Because I cannot validate the PcdCpuSmmStaticPageTable==FALSE case, I won't give R-b. But I can certainly give: Acked-by: Laszlo Ersek <lersek@redhat.com> Thanks! Laszlo ^ permalink raw reply [flat|nested] 13+ messages in thread
end of thread, other threads:[~2019-07-31 16:40 UTC | newest] Thread overview: 13+ messages (download: mbox.gz follow: Atom feed -- links below jump to the message on this page -- 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 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
This is a public inbox, see mirroring instructions for how to clone and mirror all data and code used for this inbox