* [PATCH] UefiCpuPkg/CpuDxe: Fix assert issue on IA32 platform
@ 2017-09-29 5:40 Jian J Wang
2017-09-29 6:04 ` Zeng, Star
2017-09-29 16:27 ` Kinney, Michael D
0 siblings, 2 replies; 4+ messages in thread
From: Jian J Wang @ 2017-09-29 5:40 UTC (permalink / raw)
To: edk2-devel; +Cc: Jiewen Yao, Michael Kinney
This patch is to fix an assert issue during booting IA32 platforms
such as OvmfIa32 or Quark. This issue is caused by trying to access
page table on a platform without page table. A check is added to
avoid the assert.
Bug tracker: https://bugzilla.tianocore.org/show_bug.cgi?id=724
c: Star Zeng <star.zeng@intel.com>
Cc: Jiewen Yao <jiewen.yao@intel.com>
Cc: Michael Kinney <michael.d.kinney@intel.com>
Contributed-under: TianoCore Contribution Agreement 1.1
Signed-off-by: Jian J Wang <jian.j.wang@intel.com>
---
UefiCpuPkg/CpuDxe/CpuDxe.c | 48 ++++++++++++++++++++++++++++++++++++----------
1 file changed, 38 insertions(+), 10 deletions(-)
diff --git a/UefiCpuPkg/CpuDxe/CpuDxe.c b/UefiCpuPkg/CpuDxe/CpuDxe.c
index 4e8fa100e0..85a520079f 100644
--- a/UefiCpuPkg/CpuDxe/CpuDxe.c
+++ b/UefiCpuPkg/CpuDxe/CpuDxe.c
@@ -683,7 +683,7 @@ SetGcdMemorySpaceAttributes (
**/
VOID
-RefreshGcdMemoryAttributes (
+RefreshMemoryAttributesFromMtrr (
VOID
)
{
@@ -704,14 +704,9 @@ RefreshGcdMemoryAttributes (
UINT32 FirmwareVariableMtrrCount;
UINT8 DefaultMemoryType;
- if (!IsMtrrSupported ()) {
- return;
- }
-
FirmwareVariableMtrrCount = GetFirmwareVariableMtrrCount ();
ASSERT (FirmwareVariableMtrrCount <= MTRR_NUMBER_OF_VARIABLE_MTRR);
- mIsFlushingGCD = TRUE;
MemorySpaceMap = NULL;
//
@@ -862,11 +857,44 @@ RefreshGcdMemoryAttributes (
if (MemorySpaceMap != NULL) {
FreePool (MemorySpaceMap);
}
+}
- //
- // Update page attributes
- //
- RefreshGcdMemoryAttributesFromPaging();
+/**
+ Check if paging is enabled or not.
+**/
+BOOLEAN
+IsPagingSupported (
+ VOID
+ )
+{
+ return (
+ (AsmReadCr0 () & BIT31) != 0
+ &&
+ (AsmReadCr4 () & BIT5) != 0
+ );
+}
+
+/**
+ Refreshes the GCD Memory Space attributes according to MTRRs and Paging.
+
+ This function refreshes the GCD Memory Space attributes according to MTRRs
+ and page tables.
+
+**/
+VOID
+RefreshGcdMemoryAttributes (
+ VOID
+ )
+{
+ mIsFlushingGCD = TRUE;
+
+ if (IsMtrrSupported ()) {
+ RefreshMemoryAttributesFromMtrr ();
+ }
+
+ if (IsPagingSupported ()) {
+ RefreshGcdMemoryAttributesFromPaging ();
+ }
mIsFlushingGCD = FALSE;
}
--
2.14.1.windows.1
^ permalink raw reply related [flat|nested] 4+ messages in thread
* Re: [PATCH] UefiCpuPkg/CpuDxe: Fix assert issue on IA32 platform
2017-09-29 5:40 [PATCH] UefiCpuPkg/CpuDxe: Fix assert issue on IA32 platform Jian J Wang
@ 2017-09-29 6:04 ` Zeng, Star
2017-09-29 16:27 ` Kinney, Michael D
1 sibling, 0 replies; 4+ messages in thread
From: Zeng, Star @ 2017-09-29 6:04 UTC (permalink / raw)
To: Wang, Jian J, edk2-devel@lists.01.org
Cc: Kinney, Michael D, Yao, Jiewen, Zeng, Star
Reviewed-by: Star Zeng <star.zeng@intel.com>
Thanks,
Star
-----Original Message-----
From: edk2-devel [mailto:edk2-devel-bounces@lists.01.org] On Behalf Of Jian J Wang
Sent: Friday, September 29, 2017 1:40 PM
To: edk2-devel@lists.01.org
Cc: Kinney, Michael D <michael.d.kinney@intel.com>; Yao, Jiewen <jiewen.yao@intel.com>
Subject: [edk2] [PATCH] UefiCpuPkg/CpuDxe: Fix assert issue on IA32 platform
This patch is to fix an assert issue during booting IA32 platforms such as OvmfIa32 or Quark. This issue is caused by trying to access page table on a platform without page table. A check is added to avoid the assert.
Bug tracker: https://bugzilla.tianocore.org/show_bug.cgi?id=724
c: Star Zeng <star.zeng@intel.com>
Cc: Jiewen Yao <jiewen.yao@intel.com>
Cc: Michael Kinney <michael.d.kinney@intel.com>
Contributed-under: TianoCore Contribution Agreement 1.1
Signed-off-by: Jian J Wang <jian.j.wang@intel.com>
---
UefiCpuPkg/CpuDxe/CpuDxe.c | 48 ++++++++++++++++++++++++++++++++++++----------
1 file changed, 38 insertions(+), 10 deletions(-)
diff --git a/UefiCpuPkg/CpuDxe/CpuDxe.c b/UefiCpuPkg/CpuDxe/CpuDxe.c index 4e8fa100e0..85a520079f 100644
--- a/UefiCpuPkg/CpuDxe/CpuDxe.c
+++ b/UefiCpuPkg/CpuDxe/CpuDxe.c
@@ -683,7 +683,7 @@ SetGcdMemorySpaceAttributes (
**/
VOID
-RefreshGcdMemoryAttributes (
+RefreshMemoryAttributesFromMtrr (
VOID
)
{
@@ -704,14 +704,9 @@ RefreshGcdMemoryAttributes (
UINT32 FirmwareVariableMtrrCount;
UINT8 DefaultMemoryType;
- if (!IsMtrrSupported ()) {
- return;
- }
-
FirmwareVariableMtrrCount = GetFirmwareVariableMtrrCount ();
ASSERT (FirmwareVariableMtrrCount <= MTRR_NUMBER_OF_VARIABLE_MTRR);
- mIsFlushingGCD = TRUE;
MemorySpaceMap = NULL;
//
@@ -862,11 +857,44 @@ RefreshGcdMemoryAttributes (
if (MemorySpaceMap != NULL) {
FreePool (MemorySpaceMap);
}
+}
- //
- // Update page attributes
- //
- RefreshGcdMemoryAttributesFromPaging();
+/**
+ Check if paging is enabled or not.
+**/
+BOOLEAN
+IsPagingSupported (
+ VOID
+ )
+{
+ return (
+ (AsmReadCr0 () & BIT31) != 0
+ &&
+ (AsmReadCr4 () & BIT5) != 0
+ );
+}
+
+/**
+ Refreshes the GCD Memory Space attributes according to MTRRs and Paging.
+
+ This function refreshes the GCD Memory Space attributes according to
+ MTRRs and page tables.
+
+**/
+VOID
+RefreshGcdMemoryAttributes (
+ VOID
+ )
+{
+ mIsFlushingGCD = TRUE;
+
+ if (IsMtrrSupported ()) {
+ RefreshMemoryAttributesFromMtrr (); }
+
+ if (IsPagingSupported ()) {
+ RefreshGcdMemoryAttributesFromPaging (); }
mIsFlushingGCD = FALSE;
}
--
2.14.1.windows.1
_______________________________________________
edk2-devel mailing list
edk2-devel@lists.01.org
https://lists.01.org/mailman/listinfo/edk2-devel
^ permalink raw reply [flat|nested] 4+ messages in thread
* Re: [PATCH] UefiCpuPkg/CpuDxe: Fix assert issue on IA32 platform
2017-09-29 5:40 [PATCH] UefiCpuPkg/CpuDxe: Fix assert issue on IA32 platform Jian J Wang
2017-09-29 6:04 ` Zeng, Star
@ 2017-09-29 16:27 ` Kinney, Michael D
2017-09-30 0:09 ` Wang, Jian J
1 sibling, 1 reply; 4+ messages in thread
From: Kinney, Michael D @ 2017-09-29 16:27 UTC (permalink / raw)
To: Wang, Jian J, edk2-devel@lists.01.org, Kinney, Michael D; +Cc: Yao, Jiewen
Jian,
The names of the functions do not look right to me.
I think the checks being made check to see if paging
is enabled. Also, the check being made is to see if both
paging is enabled and page address extensions is enabled.
If you use the IA32_CR0 and IA32_CR4 structures from
BaseLib.h, the updates will also be easier to understand
than using BITxx macros.
Maybe use the following:
BOOLEAN
IsPagingAndPageAddressExtensionsEnabled (
VOID
)
{
IA32_CR0 Cr0;
IA32_CR4 Cr4;
Cr0.UintN = AsmReadCr0 ();
Cr4.UintN = AsmReadCr4 ();
return ((Cr0.Bits.PG != 0) && (Cr4.Bits.PAE != 0));
}
Best regards,
Mike
> -----Original Message-----
> From: Wang, Jian J
> Sent: Thursday, September 28, 2017 10:40 PM
> To: edk2-devel@lists.01.org
> Cc: Yao, Jiewen <jiewen.yao@intel.com>; Kinney, Michael D
> <michael.d.kinney@intel.com>
> Subject: [PATCH] UefiCpuPkg/CpuDxe: Fix assert issue on IA32
> platform
>
> This patch is to fix an assert issue during booting IA32
> platforms
> such as OvmfIa32 or Quark. This issue is caused by trying to
> access
> page table on a platform without page table. A check is added
> to
> avoid the assert.
>
> Bug tracker: https://bugzilla.tianocore.org/show_bug.cgi?id=724
>
> c: Star Zeng <star.zeng@intel.com>
> Cc: Jiewen Yao <jiewen.yao@intel.com>
> Cc: Michael Kinney <michael.d.kinney@intel.com>
> Contributed-under: TianoCore Contribution Agreement 1.1
> Signed-off-by: Jian J Wang <jian.j.wang@intel.com>
> ---
> UefiCpuPkg/CpuDxe/CpuDxe.c | 48
> ++++++++++++++++++++++++++++++++++++----------
> 1 file changed, 38 insertions(+), 10 deletions(-)
>
> diff --git a/UefiCpuPkg/CpuDxe/CpuDxe.c
> b/UefiCpuPkg/CpuDxe/CpuDxe.c
> index 4e8fa100e0..85a520079f 100644
> --- a/UefiCpuPkg/CpuDxe/CpuDxe.c
> +++ b/UefiCpuPkg/CpuDxe/CpuDxe.c
> @@ -683,7 +683,7 @@ SetGcdMemorySpaceAttributes (
>
> **/
> VOID
> -RefreshGcdMemoryAttributes (
> +RefreshMemoryAttributesFromMtrr (
> VOID
> )
> {
> @@ -704,14 +704,9 @@ RefreshGcdMemoryAttributes (
> UINT32
> FirmwareVariableMtrrCount;
> UINT8 DefaultMemoryType;
>
> - if (!IsMtrrSupported ()) {
> - return;
> - }
> -
> FirmwareVariableMtrrCount = GetFirmwareVariableMtrrCount ();
> ASSERT (FirmwareVariableMtrrCount <=
> MTRR_NUMBER_OF_VARIABLE_MTRR);
>
> - mIsFlushingGCD = TRUE;
> MemorySpaceMap = NULL;
>
> //
> @@ -862,11 +857,44 @@ RefreshGcdMemoryAttributes (
> if (MemorySpaceMap != NULL) {
> FreePool (MemorySpaceMap);
> }
> +}
>
> - //
> - // Update page attributes
> - //
> - RefreshGcdMemoryAttributesFromPaging();
> +/**
> + Check if paging is enabled or not.
> +**/
> +BOOLEAN
> +IsPagingSupported (
> + VOID
> + )
> +{
> + return (
> + (AsmReadCr0 () & BIT31) != 0
> + &&
> + (AsmReadCr4 () & BIT5) != 0
> + );
> +}
> +
> +/**
> + Refreshes the GCD Memory Space attributes according to MTRRs
> and Paging.
> +
> + This function refreshes the GCD Memory Space attributes
> according to MTRRs
> + and page tables.
> +
> +**/
> +VOID
> +RefreshGcdMemoryAttributes (
> + VOID
> + )
> +{
> + mIsFlushingGCD = TRUE;
> +
> + if (IsMtrrSupported ()) {
> + RefreshMemoryAttributesFromMtrr ();
> + }
> +
> + if (IsPagingSupported ()) {
> + RefreshGcdMemoryAttributesFromPaging ();
> + }
>
> mIsFlushingGCD = FALSE;
> }
> --
> 2.14.1.windows.1
^ permalink raw reply [flat|nested] 4+ messages in thread
* Re: [PATCH] UefiCpuPkg/CpuDxe: Fix assert issue on IA32 platform
2017-09-29 16:27 ` Kinney, Michael D
@ 2017-09-30 0:09 ` Wang, Jian J
0 siblings, 0 replies; 4+ messages in thread
From: Wang, Jian J @ 2017-09-30 0:09 UTC (permalink / raw)
To: Kinney, Michael D, edk2-devel@lists.01.org; +Cc: Yao, Jiewen
Mike,
Thanks for the feedback. I'll update the patch as what you suggested. And if no more other comments, I'll check in the updated one today (or tonight).
Jian
> -----Original Message-----
> From: Kinney, Michael D
> Sent: Saturday, September 30, 2017 12:27 AM
> To: Wang, Jian J <jian.j.wang@intel.com>; edk2-devel@lists.01.org; Kinney,
> Michael D <michael.d.kinney@intel.com>
> Cc: Yao, Jiewen <jiewen.yao@intel.com>
> Subject: RE: [PATCH] UefiCpuPkg/CpuDxe: Fix assert issue on IA32 platform
>
> Jian,
>
> The names of the functions do not look right to me.
>
> I think the checks being made check to see if paging
> is enabled. Also, the check being made is to see if both
> paging is enabled and page address extensions is enabled.
>
> If you use the IA32_CR0 and IA32_CR4 structures from
> BaseLib.h, the updates will also be easier to understand
> than using BITxx macros.
>
> Maybe use the following:
>
>
> BOOLEAN
> IsPagingAndPageAddressExtensionsEnabled (
> VOID
> )
> {
> IA32_CR0 Cr0;
> IA32_CR4 Cr4;
>
> Cr0.UintN = AsmReadCr0 ();
> Cr4.UintN = AsmReadCr4 ();
> return ((Cr0.Bits.PG != 0) && (Cr4.Bits.PAE != 0));
> }
>
> Best regards,
>
> Mike
>
> > -----Original Message-----
> > From: Wang, Jian J
> > Sent: Thursday, September 28, 2017 10:40 PM
> > To: edk2-devel@lists.01.org
> > Cc: Yao, Jiewen <jiewen.yao@intel.com>; Kinney, Michael D
> > <michael.d.kinney@intel.com>
> > Subject: [PATCH] UefiCpuPkg/CpuDxe: Fix assert issue on IA32
> > platform
> >
> > This patch is to fix an assert issue during booting IA32
> > platforms
> > such as OvmfIa32 or Quark. This issue is caused by trying to
> > access
> > page table on a platform without page table. A check is added
> > to
> > avoid the assert.
> >
> > Bug tracker: https://bugzilla.tianocore.org/show_bug.cgi?id=724
> >
> > c: Star Zeng <star.zeng@intel.com>
> > Cc: Jiewen Yao <jiewen.yao@intel.com>
> > Cc: Michael Kinney <michael.d.kinney@intel.com>
> > Contributed-under: TianoCore Contribution Agreement 1.1
> > Signed-off-by: Jian J Wang <jian.j.wang@intel.com>
> > ---
> > UefiCpuPkg/CpuDxe/CpuDxe.c | 48
> > ++++++++++++++++++++++++++++++++++++----------
> > 1 file changed, 38 insertions(+), 10 deletions(-)
> >
> > diff --git a/UefiCpuPkg/CpuDxe/CpuDxe.c
> > b/UefiCpuPkg/CpuDxe/CpuDxe.c
> > index 4e8fa100e0..85a520079f 100644
> > --- a/UefiCpuPkg/CpuDxe/CpuDxe.c
> > +++ b/UefiCpuPkg/CpuDxe/CpuDxe.c
> > @@ -683,7 +683,7 @@ SetGcdMemorySpaceAttributes (
> >
> > **/
> > VOID
> > -RefreshGcdMemoryAttributes (
> > +RefreshMemoryAttributesFromMtrr (
> > VOID
> > )
> > {
> > @@ -704,14 +704,9 @@ RefreshGcdMemoryAttributes (
> > UINT32
> > FirmwareVariableMtrrCount;
> > UINT8 DefaultMemoryType;
> >
> > - if (!IsMtrrSupported ()) {
> > - return;
> > - }
> > -
> > FirmwareVariableMtrrCount = GetFirmwareVariableMtrrCount ();
> > ASSERT (FirmwareVariableMtrrCount <=
> > MTRR_NUMBER_OF_VARIABLE_MTRR);
> >
> > - mIsFlushingGCD = TRUE;
> > MemorySpaceMap = NULL;
> >
> > //
> > @@ -862,11 +857,44 @@ RefreshGcdMemoryAttributes (
> > if (MemorySpaceMap != NULL) {
> > FreePool (MemorySpaceMap);
> > }
> > +}
> >
> > - //
> > - // Update page attributes
> > - //
> > - RefreshGcdMemoryAttributesFromPaging();
> > +/**
> > + Check if paging is enabled or not.
> > +**/
> > +BOOLEAN
> > +IsPagingSupported (
> > + VOID
> > + )
> > +{
> > + return (
> > + (AsmReadCr0 () & BIT31) != 0
> > + &&
> > + (AsmReadCr4 () & BIT5) != 0
> > + );
> > +}
> > +
> > +/**
> > + Refreshes the GCD Memory Space attributes according to MTRRs
> > and Paging.
> > +
> > + This function refreshes the GCD Memory Space attributes
> > according to MTRRs
> > + and page tables.
> > +
> > +**/
> > +VOID
> > +RefreshGcdMemoryAttributes (
> > + VOID
> > + )
> > +{
> > + mIsFlushingGCD = TRUE;
> > +
> > + if (IsMtrrSupported ()) {
> > + RefreshMemoryAttributesFromMtrr ();
> > + }
> > +
> > + if (IsPagingSupported ()) {
> > + RefreshGcdMemoryAttributesFromPaging ();
> > + }
> >
> > mIsFlushingGCD = FALSE;
> > }
> > --
> > 2.14.1.windows.1
^ permalink raw reply [flat|nested] 4+ messages in thread
end of thread, other threads:[~2017-09-30 0:05 UTC | newest]
Thread overview: 4+ messages (download: mbox.gz follow: Atom feed
-- links below jump to the message on this page --
2017-09-29 5:40 [PATCH] UefiCpuPkg/CpuDxe: Fix assert issue on IA32 platform Jian J Wang
2017-09-29 6:04 ` Zeng, Star
2017-09-29 16:27 ` Kinney, Michael D
2017-09-30 0:09 ` Wang, Jian J
This is a public inbox, see mirroring instructions
for how to clone and mirror all data and code used for this inbox