* [PATCH v2] UefiCpuPkg/CpuDxe: Fix multiple entries of RT_CODE in memory map
@ 2017-10-25 8:12 Jian J Wang
0 siblings, 0 replies; 17+ messages in thread
From: Jian J Wang @ 2017-10-25 8:12 UTC (permalink / raw)
To: edk2-devel; +Cc: Eric Dong, Jiewen Yao
> v2 just updated the commit message.
Multiple RT_CODE in memory map was introduced by previous commit
c1cab54ce57c2608b8b3ea051c7041f036f21153
This commit changed memory capability only based on page type of memory
attributes. EDK2's report of memory map to OS is grouped by memory
capabilites. This will cause more than on RT_CODE entries in memory map
if UEFI image protection is enabled, which will mark memory of code
segments of some modules to be read-only.
More than one entry of RT_CODE memory will cause boot problem for specific
old version of Linux kernel, because it will misplace the code segment and
data segment in this situation. The recent major Linux distro have no such
problem in boot. This patch will fix this issue to keep OS compatibility
as much as possible.
>From memory paging point of view, all memory block should have the capabilites
to change paging related memory attributes, if the memory paging has been
enabled and well setup, not just those memory blocks having some paging related
attributes set. So this patch will simply add all paging related memory
attribtes to the capability of all existing memory blocks if paging is enabled.
As a side effect, it will prevent EDK2 from reporting multple RT_CODE to OS.
Cc: Eric Dong <eric.dong@intel.com>
Cc: Jiewen Yao <jiewen.yao@intel.com>
Contributed-under: TianoCore Contribution Agreement 1.1
Signed-off-by: Jian J Wang <jian.j.wang@intel.com>
---
UefiCpuPkg/CpuDxe/CpuPageTable.c | 14 +++++++++++---
1 file changed, 11 insertions(+), 3 deletions(-)
diff --git a/UefiCpuPkg/CpuDxe/CpuPageTable.c b/UefiCpuPkg/CpuDxe/CpuPageTable.c
index d312eb66f8..0802464b9d 100644
--- a/UefiCpuPkg/CpuDxe/CpuPageTable.c
+++ b/UefiCpuPkg/CpuDxe/CpuPageTable.c
@@ -829,6 +829,15 @@ RefreshGcdMemoryAttributesFromPaging (
// Sync real page attributes to GCD
BaseAddress = MemorySpaceMap[Index].BaseAddress;
MemorySpaceLength = MemorySpaceMap[Index].Length;
+ Capabilities = MemorySpaceMap[Index].Capabilities |
+ EFI_MEMORY_PAGETYPE_MASK;
+ Status = gDS->SetMemorySpaceCapabilities (
+ BaseAddress,
+ MemorySpaceLength,
+ Capabilities
+ );
+ ASSERT_EFI_ERROR (Status);
+
while (MemorySpaceLength > 0) {
if (PageLength == 0) {
PageEntry = GetPageTableEntry (&PagingContext, BaseAddress, &PageAttribute);
@@ -846,7 +855,6 @@ RefreshGcdMemoryAttributesFromPaging (
if (Attributes != (MemorySpaceMap[Index].Attributes & EFI_MEMORY_PAGETYPE_MASK)) {
DoUpdate = TRUE;
Attributes |= (MemorySpaceMap[Index].Attributes & ~EFI_MEMORY_PAGETYPE_MASK);
- Capabilities = Attributes | MemorySpaceMap[Index].Capabilities;
} else {
DoUpdate = FALSE;
}
@@ -854,8 +862,8 @@ RefreshGcdMemoryAttributesFromPaging (
Length = MIN (PageLength, MemorySpaceLength);
if (DoUpdate) {
- gDS->SetMemorySpaceCapabilities (BaseAddress, Length, Capabilities);
- gDS->SetMemorySpaceAttributes (BaseAddress, Length, Attributes);
+ Status = gDS->SetMemorySpaceAttributes (BaseAddress, Length, Attributes);
+ ASSERT_EFI_ERROR (Status);
DEBUG ((DEBUG_INFO, "Update memory space attribute: [%02d] %016lx - %016lx (%08lx -> %08lx)\r\n",
Index, BaseAddress, BaseAddress + Length - 1,
MemorySpaceMap[Index].Attributes, Attributes));
--
2.14.1.windows.1
^ permalink raw reply related [flat|nested] 17+ messages in thread
* [PATCH v2] UefiCpuPkg/CpuDxe: Fix multiple entries of RT_CODE in memory map
@ 2017-11-03 0:57 Jian J Wang
2017-11-06 9:15 ` Zeng, Star
` (2 more replies)
0 siblings, 3 replies; 17+ messages in thread
From: Jian J Wang @ 2017-11-03 0:57 UTC (permalink / raw)
To: edk2-devel; +Cc: Eric Dong, Jiewen Yao, Laszlo Ersek
> v2
> a. Fix an issue which will cause setting capability failure if size is smaller
> than a page.
More than one entry of RT_CODE memory might cause boot problem for some
old OSs. This patch will fix this issue to keep OS compatibility as much
as possible.
More detailed information, please refer to
https://bugzilla.tianocore.org/show_bug.cgi?id=753
Cc: Eric Dong <eric.dong@intel.com>
Cc: Jiewen Yao <jiewen.yao@intel.com>
Cc: Laszlo Ersek <lersek@redhat.com>
Contributed-under: TianoCore Contribution Agreement 1.1
Signed-off-by: Jian J Wang <jian.j.wang@intel.com>
---
UefiCpuPkg/CpuDxe/CpuPageTable.c | 18 ++++++++++++++----
1 file changed, 14 insertions(+), 4 deletions(-)
diff --git a/UefiCpuPkg/CpuDxe/CpuPageTable.c b/UefiCpuPkg/CpuDxe/CpuPageTable.c
index d312eb66f8..4a7827ebc9 100644
--- a/UefiCpuPkg/CpuDxe/CpuPageTable.c
+++ b/UefiCpuPkg/CpuDxe/CpuPageTable.c
@@ -809,7 +809,9 @@ RefreshGcdMemoryAttributesFromPaging (
PageLength = 0;
for (Index = 0; Index < NumberOfDescriptors; Index++) {
- if (MemorySpaceMap[Index].GcdMemoryType == EfiGcdMemoryTypeNonExistent) {
+ if (MemorySpaceMap[Index].GcdMemoryType == EfiGcdMemoryTypeNonExistent
+ || (MemorySpaceMap[Index].BaseAddress & EFI_PAGE_MASK) != 0
+ || (MemorySpaceMap[Index].Length & EFI_PAGE_MASK) != 0) {
continue;
}
@@ -829,6 +831,15 @@ RefreshGcdMemoryAttributesFromPaging (
// Sync real page attributes to GCD
BaseAddress = MemorySpaceMap[Index].BaseAddress;
MemorySpaceLength = MemorySpaceMap[Index].Length;
+ Capabilities = MemorySpaceMap[Index].Capabilities |
+ EFI_MEMORY_PAGETYPE_MASK;
+ Status = gDS->SetMemorySpaceCapabilities (
+ BaseAddress,
+ MemorySpaceLength,
+ Capabilities
+ );
+ ASSERT_EFI_ERROR (Status);
+
while (MemorySpaceLength > 0) {
if (PageLength == 0) {
PageEntry = GetPageTableEntry (&PagingContext, BaseAddress, &PageAttribute);
@@ -846,7 +857,6 @@ RefreshGcdMemoryAttributesFromPaging (
if (Attributes != (MemorySpaceMap[Index].Attributes & EFI_MEMORY_PAGETYPE_MASK)) {
DoUpdate = TRUE;
Attributes |= (MemorySpaceMap[Index].Attributes & ~EFI_MEMORY_PAGETYPE_MASK);
- Capabilities = Attributes | MemorySpaceMap[Index].Capabilities;
} else {
DoUpdate = FALSE;
}
@@ -854,8 +864,8 @@ RefreshGcdMemoryAttributesFromPaging (
Length = MIN (PageLength, MemorySpaceLength);
if (DoUpdate) {
- gDS->SetMemorySpaceCapabilities (BaseAddress, Length, Capabilities);
- gDS->SetMemorySpaceAttributes (BaseAddress, Length, Attributes);
+ Status = gDS->SetMemorySpaceAttributes (BaseAddress, Length, Attributes);
+ ASSERT_EFI_ERROR (Status);
DEBUG ((DEBUG_INFO, "Update memory space attribute: [%02d] %016lx - %016lx (%08lx -> %08lx)\r\n",
Index, BaseAddress, BaseAddress + Length - 1,
MemorySpaceMap[Index].Attributes, Attributes));
--
2.14.1.windows.1
^ permalink raw reply related [flat|nested] 17+ messages in thread
* Re: [PATCH v2] UefiCpuPkg/CpuDxe: Fix multiple entries of RT_CODE in memory map
2017-11-03 0:57 Jian J Wang
@ 2017-11-06 9:15 ` Zeng, Star
2017-11-07 0:55 ` Wang, Jian J
2017-11-07 17:13 ` Laszlo Ersek
2017-11-08 4:41 ` Ni, Ruiyu
2 siblings, 1 reply; 17+ messages in thread
From: Zeng, Star @ 2017-11-06 9:15 UTC (permalink / raw)
To: Wang, Jian J, edk2-devel@lists.01.org
Cc: Laszlo Ersek, Yao, Jiewen, Dong, Eric, Zeng, Star
I am ok to this code approach.
Acknowledged-by: Star Zeng <star.zeng@intel.com>
Besides, I think except GCD services, DxeCore should not call gCpu->SetMemoryAttributes() directly, for example ApplyMemoryProtectionPolicy(), it should have no assumption of the CPU code (to set capabilities), and call GCD setcapabilities first, and then call GCD setattributes since 14dde9e903bb9a719ebb8f3381da72b19509bc36 "MdeModulePkg/Core: Fix out-of-sync issue in GCD", otherwise there may be mismatch of page attributes between GCD and gCPU after RefreshGcdMemoryAttributesFromPaging() by the calling gCpu->SetMemoryAttributes() in ApplyMemoryProtectionPolicy().
Anyway, that could be in a separated patch. :)
Thanks,
Star
-----Original Message-----
From: edk2-devel [mailto:edk2-devel-bounces@lists.01.org] On Behalf Of Jian J Wang
Sent: Friday, November 3, 2017 8:57 AM
To: edk2-devel@lists.01.org
Cc: Laszlo Ersek <lersek@redhat.com>; Yao, Jiewen <jiewen.yao@intel.com>; Dong, Eric <eric.dong@intel.com>
Subject: [edk2] [PATCH v2] UefiCpuPkg/CpuDxe: Fix multiple entries of RT_CODE in memory map
> v2
> a. Fix an issue which will cause setting capability failure if size is smaller
> than a page.
More than one entry of RT_CODE memory might cause boot problem for some old OSs. This patch will fix this issue to keep OS compatibility as much as possible.
More detailed information, please refer to
https://bugzilla.tianocore.org/show_bug.cgi?id=753
Cc: Eric Dong <eric.dong@intel.com>
Cc: Jiewen Yao <jiewen.yao@intel.com>
Cc: Laszlo Ersek <lersek@redhat.com>
Contributed-under: TianoCore Contribution Agreement 1.1
Signed-off-by: Jian J Wang <jian.j.wang@intel.com>
---
UefiCpuPkg/CpuDxe/CpuPageTable.c | 18 ++++++++++++++----
1 file changed, 14 insertions(+), 4 deletions(-)
diff --git a/UefiCpuPkg/CpuDxe/CpuPageTable.c b/UefiCpuPkg/CpuDxe/CpuPageTable.c
index d312eb66f8..4a7827ebc9 100644
--- a/UefiCpuPkg/CpuDxe/CpuPageTable.c
+++ b/UefiCpuPkg/CpuDxe/CpuPageTable.c
@@ -809,7 +809,9 @@ RefreshGcdMemoryAttributesFromPaging (
PageLength = 0;
for (Index = 0; Index < NumberOfDescriptors; Index++) {
- if (MemorySpaceMap[Index].GcdMemoryType == EfiGcdMemoryTypeNonExistent) {
+ if (MemorySpaceMap[Index].GcdMemoryType == EfiGcdMemoryTypeNonExistent
+ || (MemorySpaceMap[Index].BaseAddress & EFI_PAGE_MASK) != 0
+ || (MemorySpaceMap[Index].Length & EFI_PAGE_MASK) != 0) {
continue;
}
@@ -829,6 +831,15 @@ RefreshGcdMemoryAttributesFromPaging (
// Sync real page attributes to GCD
BaseAddress = MemorySpaceMap[Index].BaseAddress;
MemorySpaceLength = MemorySpaceMap[Index].Length;
+ Capabilities = MemorySpaceMap[Index].Capabilities |
+ EFI_MEMORY_PAGETYPE_MASK;
+ Status = gDS->SetMemorySpaceCapabilities (
+ BaseAddress,
+ MemorySpaceLength,
+ Capabilities
+ );
+ ASSERT_EFI_ERROR (Status);
+
while (MemorySpaceLength > 0) {
if (PageLength == 0) {
PageEntry = GetPageTableEntry (&PagingContext, BaseAddress, &PageAttribute); @@ -846,7 +857,6 @@ RefreshGcdMemoryAttributesFromPaging (
if (Attributes != (MemorySpaceMap[Index].Attributes & EFI_MEMORY_PAGETYPE_MASK)) {
DoUpdate = TRUE;
Attributes |= (MemorySpaceMap[Index].Attributes & ~EFI_MEMORY_PAGETYPE_MASK);
- Capabilities = Attributes | MemorySpaceMap[Index].Capabilities;
} else {
DoUpdate = FALSE;
}
@@ -854,8 +864,8 @@ RefreshGcdMemoryAttributesFromPaging (
Length = MIN (PageLength, MemorySpaceLength);
if (DoUpdate) {
- gDS->SetMemorySpaceCapabilities (BaseAddress, Length, Capabilities);
- gDS->SetMemorySpaceAttributes (BaseAddress, Length, Attributes);
+ Status = gDS->SetMemorySpaceAttributes (BaseAddress, Length, Attributes);
+ ASSERT_EFI_ERROR (Status);
DEBUG ((DEBUG_INFO, "Update memory space attribute: [%02d] %016lx - %016lx (%08lx -> %08lx)\r\n",
Index, BaseAddress, BaseAddress + Length - 1,
MemorySpaceMap[Index].Attributes, Attributes));
--
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 related [flat|nested] 17+ messages in thread
* Re: [PATCH v2] UefiCpuPkg/CpuDxe: Fix multiple entries of RT_CODE in memory map
2017-11-06 9:15 ` Zeng, Star
@ 2017-11-07 0:55 ` Wang, Jian J
2017-11-07 1:12 ` Zeng, Star
2017-11-08 3:13 ` Zeng, Star
0 siblings, 2 replies; 17+ messages in thread
From: Wang, Jian J @ 2017-11-07 0:55 UTC (permalink / raw)
To: Zeng, Star, edk2-devel@lists.01.org; +Cc: Laszlo Ersek, Yao, Jiewen, Dong, Eric
Thanks for the review. And I agree that GCD.SetMemoryAttributes should be
used all the time in DxeCore. Let's fix it in another patch.
> -----Original Message-----
> From: Zeng, Star
> Sent: Monday, November 06, 2017 5:16 PM
> To: Wang, Jian J <jian.j.wang@intel.com>; edk2-devel@lists.01.org
> Cc: Laszlo Ersek <lersek@redhat.com>; Yao, Jiewen <jiewen.yao@intel.com>;
> Dong, Eric <eric.dong@intel.com>; Zeng, Star <star.zeng@intel.com>
> Subject: RE: [edk2] [PATCH v2] UefiCpuPkg/CpuDxe: Fix multiple entries of
> RT_CODE in memory map
>
> I am ok to this code approach.
>
> Acknowledged-by: Star Zeng <star.zeng@intel.com>
>
> Besides, I think except GCD services, DxeCore should not call gCpu-
> >SetMemoryAttributes() directly, for example ApplyMemoryProtectionPolicy(), it
> should have no assumption of the CPU code (to set capabilities), and call GCD
> setcapabilities first, and then call GCD setattributes since
> 14dde9e903bb9a719ebb8f3381da72b19509bc36 "MdeModulePkg/Core: Fix out-
> of-sync issue in GCD", otherwise there may be mismatch of page attributes
> between GCD and gCPU after RefreshGcdMemoryAttributesFromPaging() by the
> calling gCpu->SetMemoryAttributes() in ApplyMemoryProtectionPolicy().
>
> Anyway, that could be in a separated patch. :)
>
> Thanks,
> Star
> -----Original Message-----
> From: edk2-devel [mailto:edk2-devel-bounces@lists.01.org] On Behalf Of Jian J
> Wang
> Sent: Friday, November 3, 2017 8:57 AM
> To: edk2-devel@lists.01.org
> Cc: Laszlo Ersek <lersek@redhat.com>; Yao, Jiewen <jiewen.yao@intel.com>;
> Dong, Eric <eric.dong@intel.com>
> Subject: [edk2] [PATCH v2] UefiCpuPkg/CpuDxe: Fix multiple entries of RT_CODE
> in memory map
>
> > v2
> > a. Fix an issue which will cause setting capability failure if size is smaller
> > than a page.
>
> More than one entry of RT_CODE memory might cause boot problem for some
> old OSs. This patch will fix this issue to keep OS compatibility as much as possible.
>
> More detailed information, please refer to
> https://bugzilla.tianocore.org/show_bug.cgi?id=753
>
> Cc: Eric Dong <eric.dong@intel.com>
> Cc: Jiewen Yao <jiewen.yao@intel.com>
> Cc: Laszlo Ersek <lersek@redhat.com>
> Contributed-under: TianoCore Contribution Agreement 1.1
> Signed-off-by: Jian J Wang <jian.j.wang@intel.com>
> ---
> UefiCpuPkg/CpuDxe/CpuPageTable.c | 18 ++++++++++++++----
> 1 file changed, 14 insertions(+), 4 deletions(-)
>
> diff --git a/UefiCpuPkg/CpuDxe/CpuPageTable.c
> b/UefiCpuPkg/CpuDxe/CpuPageTable.c
> index d312eb66f8..4a7827ebc9 100644
> --- a/UefiCpuPkg/CpuDxe/CpuPageTable.c
> +++ b/UefiCpuPkg/CpuDxe/CpuPageTable.c
> @@ -809,7 +809,9 @@ RefreshGcdMemoryAttributesFromPaging (
> PageLength = 0;
>
> for (Index = 0; Index < NumberOfDescriptors; Index++) {
> - if (MemorySpaceMap[Index].GcdMemoryType ==
> EfiGcdMemoryTypeNonExistent) {
> + if (MemorySpaceMap[Index].GcdMemoryType ==
> EfiGcdMemoryTypeNonExistent
> + || (MemorySpaceMap[Index].BaseAddress & EFI_PAGE_MASK) != 0
> + || (MemorySpaceMap[Index].Length & EFI_PAGE_MASK) != 0) {
> continue;
> }
>
> @@ -829,6 +831,15 @@ RefreshGcdMemoryAttributesFromPaging (
> // Sync real page attributes to GCD
> BaseAddress = MemorySpaceMap[Index].BaseAddress;
> MemorySpaceLength = MemorySpaceMap[Index].Length;
> + Capabilities = MemorySpaceMap[Index].Capabilities |
> + EFI_MEMORY_PAGETYPE_MASK;
> + Status = gDS->SetMemorySpaceCapabilities (
> + BaseAddress,
> + MemorySpaceLength,
> + Capabilities
> + );
> + ASSERT_EFI_ERROR (Status);
> +
> while (MemorySpaceLength > 0) {
> if (PageLength == 0) {
> PageEntry = GetPageTableEntry (&PagingContext, BaseAddress,
> &PageAttribute); @@ -846,7 +857,6 @@
> RefreshGcdMemoryAttributesFromPaging (
> if (Attributes != (MemorySpaceMap[Index].Attributes &
> EFI_MEMORY_PAGETYPE_MASK)) {
> DoUpdate = TRUE;
> Attributes |= (MemorySpaceMap[Index].Attributes &
> ~EFI_MEMORY_PAGETYPE_MASK);
> - Capabilities = Attributes | MemorySpaceMap[Index].Capabilities;
> } else {
> DoUpdate = FALSE;
> }
> @@ -854,8 +864,8 @@ RefreshGcdMemoryAttributesFromPaging (
>
> Length = MIN (PageLength, MemorySpaceLength);
> if (DoUpdate) {
> - gDS->SetMemorySpaceCapabilities (BaseAddress, Length, Capabilities);
> - gDS->SetMemorySpaceAttributes (BaseAddress, Length, Attributes);
> + Status = gDS->SetMemorySpaceAttributes (BaseAddress, Length,
> Attributes);
> + ASSERT_EFI_ERROR (Status);
> DEBUG ((DEBUG_INFO, "Update memory space attribute: [%02d] %016lx -
> %016lx (%08lx -> %08lx)\r\n",
> Index, BaseAddress, BaseAddress + Length - 1,
> MemorySpaceMap[Index].Attributes, Attributes));
> --
> 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] 17+ messages in thread
* Re: [PATCH v2] UefiCpuPkg/CpuDxe: Fix multiple entries of RT_CODE in memory map
2017-11-07 0:55 ` Wang, Jian J
@ 2017-11-07 1:12 ` Zeng, Star
2017-11-08 3:13 ` Zeng, Star
1 sibling, 0 replies; 17+ messages in thread
From: Zeng, Star @ 2017-11-07 1:12 UTC (permalink / raw)
To: Laszlo Ersek, Yao, Jiewen, Dong, Eric; +Cc: edk2-devel@lists.01.org, Zeng, Star
Hi,
Do you have further concern to this patch and have a RB?
If no, I am happy to help push this patch with my AB.
Thanks,
Star
-----Original Message-----
From: Wang, Jian J
Sent: Tuesday, November 7, 2017 8:55 AM
To: Zeng, Star <star.zeng@intel.com>; edk2-devel@lists.01.org
Cc: Laszlo Ersek <lersek@redhat.com>; Yao, Jiewen <jiewen.yao@intel.com>; Dong, Eric <eric.dong@intel.com>
Subject: RE: [edk2] [PATCH v2] UefiCpuPkg/CpuDxe: Fix multiple entries of RT_CODE in memory map
Thanks for the review. And I agree that GCD.SetMemoryAttributes should be used all the time in DxeCore. Let's fix it in another patch.
> -----Original Message-----
> From: Zeng, Star
> Sent: Monday, November 06, 2017 5:16 PM
> To: Wang, Jian J <jian.j.wang@intel.com>; edk2-devel@lists.01.org
> Cc: Laszlo Ersek <lersek@redhat.com>; Yao, Jiewen
> <jiewen.yao@intel.com>; Dong, Eric <eric.dong@intel.com>; Zeng, Star
> <star.zeng@intel.com>
> Subject: RE: [edk2] [PATCH v2] UefiCpuPkg/CpuDxe: Fix multiple entries
> of RT_CODE in memory map
>
> I am ok to this code approach.
>
> Acknowledged-by: Star Zeng <star.zeng@intel.com>
>
> Besides, I think except GCD services, DxeCore should not call gCpu-
> >SetMemoryAttributes() directly, for example
> >ApplyMemoryProtectionPolicy(), it
> should have no assumption of the CPU code (to set capabilities), and
> call GCD setcapabilities first, and then call GCD setattributes since
> 14dde9e903bb9a719ebb8f3381da72b19509bc36 "MdeModulePkg/Core: Fix out-
> of-sync issue in GCD", otherwise there may be mismatch of page
> attributes between GCD and gCPU after
> RefreshGcdMemoryAttributesFromPaging() by the calling gCpu->SetMemoryAttributes() in ApplyMemoryProtectionPolicy().
>
> Anyway, that could be in a separated patch. :)
>
> Thanks,
> Star
> -----Original Message-----
> From: edk2-devel [mailto:edk2-devel-bounces@lists.01.org] On Behalf Of
> Jian J Wang
> Sent: Friday, November 3, 2017 8:57 AM
> To: edk2-devel@lists.01.org
> Cc: Laszlo Ersek <lersek@redhat.com>; Yao, Jiewen
> <jiewen.yao@intel.com>; Dong, Eric <eric.dong@intel.com>
> Subject: [edk2] [PATCH v2] UefiCpuPkg/CpuDxe: Fix multiple entries of
> RT_CODE in memory map
>
> > v2
> > a. Fix an issue which will cause setting capability failure if size is smaller
> > than a page.
>
> More than one entry of RT_CODE memory might cause boot problem for
> some old OSs. This patch will fix this issue to keep OS compatibility as much as possible.
>
> More detailed information, please refer to
> https://bugzilla.tianocore.org/show_bug.cgi?id=753
>
> Cc: Eric Dong <eric.dong@intel.com>
> Cc: Jiewen Yao <jiewen.yao@intel.com>
> Cc: Laszlo Ersek <lersek@redhat.com>
> Contributed-under: TianoCore Contribution Agreement 1.1
> Signed-off-by: Jian J Wang <jian.j.wang@intel.com>
> ---
> UefiCpuPkg/CpuDxe/CpuPageTable.c | 18 ++++++++++++++----
> 1 file changed, 14 insertions(+), 4 deletions(-)
>
> diff --git a/UefiCpuPkg/CpuDxe/CpuPageTable.c
> b/UefiCpuPkg/CpuDxe/CpuPageTable.c
> index d312eb66f8..4a7827ebc9 100644
> --- a/UefiCpuPkg/CpuDxe/CpuPageTable.c
> +++ b/UefiCpuPkg/CpuDxe/CpuPageTable.c
> @@ -809,7 +809,9 @@ RefreshGcdMemoryAttributesFromPaging (
> PageLength = 0;
>
> for (Index = 0; Index < NumberOfDescriptors; Index++) {
> - if (MemorySpaceMap[Index].GcdMemoryType ==
> EfiGcdMemoryTypeNonExistent) {
> + if (MemorySpaceMap[Index].GcdMemoryType ==
> EfiGcdMemoryTypeNonExistent
> + || (MemorySpaceMap[Index].BaseAddress & EFI_PAGE_MASK) != 0
> + || (MemorySpaceMap[Index].Length & EFI_PAGE_MASK) != 0) {
> continue;
> }
>
> @@ -829,6 +831,15 @@ RefreshGcdMemoryAttributesFromPaging (
> // Sync real page attributes to GCD
> BaseAddress = MemorySpaceMap[Index].BaseAddress;
> MemorySpaceLength = MemorySpaceMap[Index].Length;
> + Capabilities = MemorySpaceMap[Index].Capabilities |
> + EFI_MEMORY_PAGETYPE_MASK;
> + Status = gDS->SetMemorySpaceCapabilities (
> + BaseAddress,
> + MemorySpaceLength,
> + Capabilities
> + );
> + ASSERT_EFI_ERROR (Status);
> +
> while (MemorySpaceLength > 0) {
> if (PageLength == 0) {
> PageEntry = GetPageTableEntry (&PagingContext, BaseAddress,
> &PageAttribute); @@ -846,7 +857,6 @@
> RefreshGcdMemoryAttributesFromPaging (
> if (Attributes != (MemorySpaceMap[Index].Attributes &
> EFI_MEMORY_PAGETYPE_MASK)) {
> DoUpdate = TRUE;
> Attributes |= (MemorySpaceMap[Index].Attributes &
> ~EFI_MEMORY_PAGETYPE_MASK);
> - Capabilities = Attributes | MemorySpaceMap[Index].Capabilities;
> } else {
> DoUpdate = FALSE;
> }
> @@ -854,8 +864,8 @@ RefreshGcdMemoryAttributesFromPaging (
>
> Length = MIN (PageLength, MemorySpaceLength);
> if (DoUpdate) {
> - gDS->SetMemorySpaceCapabilities (BaseAddress, Length, Capabilities);
> - gDS->SetMemorySpaceAttributes (BaseAddress, Length, Attributes);
> + Status = gDS->SetMemorySpaceAttributes (BaseAddress, Length,
> Attributes);
> + ASSERT_EFI_ERROR (Status);
> DEBUG ((DEBUG_INFO, "Update memory space attribute: [%02d]
> %016lx - %016lx (%08lx -> %08lx)\r\n",
> Index, BaseAddress, BaseAddress + Length - 1,
> MemorySpaceMap[Index].Attributes,
> Attributes));
> --
> 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] 17+ messages in thread
* Re: [PATCH v2] UefiCpuPkg/CpuDxe: Fix multiple entries of RT_CODE in memory map
2017-11-07 0:55 ` Wang, Jian J
2017-11-07 1:12 ` Zeng, Star
@ 2017-11-08 3:13 ` Zeng, Star
2017-11-08 13:25 ` Laszlo Ersek
1 sibling, 1 reply; 17+ messages in thread
From: Zeng, Star @ 2017-11-08 3:13 UTC (permalink / raw)
To: Wang, Jian J, edk2-devel@lists.01.org
Cc: Laszlo Ersek, Yao, Jiewen, Dong, Eric, Zeng, Star
https://bugzilla.tianocore.org/show_bug.cgi?id=763 is submitted to track this.
Thanks,
Star
-----Original Message-----
From: Wang, Jian J
Sent: Tuesday, November 7, 2017 8:55 AM
To: Zeng, Star <star.zeng@intel.com>; edk2-devel@lists.01.org
Cc: Laszlo Ersek <lersek@redhat.com>; Yao, Jiewen <jiewen.yao@intel.com>; Dong, Eric <eric.dong@intel.com>
Subject: RE: [edk2] [PATCH v2] UefiCpuPkg/CpuDxe: Fix multiple entries of RT_CODE in memory map
Thanks for the review. And I agree that GCD.SetMemoryAttributes should be used all the time in DxeCore. Let's fix it in another patch.
> -----Original Message-----
> From: Zeng, Star
> Sent: Monday, November 06, 2017 5:16 PM
> To: Wang, Jian J <jian.j.wang@intel.com>; edk2-devel@lists.01.org
> Cc: Laszlo Ersek <lersek@redhat.com>; Yao, Jiewen
> <jiewen.yao@intel.com>; Dong, Eric <eric.dong@intel.com>; Zeng, Star
> <star.zeng@intel.com>
> Subject: RE: [edk2] [PATCH v2] UefiCpuPkg/CpuDxe: Fix multiple entries
> of RT_CODE in memory map
>
> I am ok to this code approach.
>
> Acknowledged-by: Star Zeng <star.zeng@intel.com>
>
> Besides, I think except GCD services, DxeCore should not call gCpu-
> >SetMemoryAttributes() directly, for example
> >ApplyMemoryProtectionPolicy(), it
> should have no assumption of the CPU code (to set capabilities), and
> call GCD setcapabilities first, and then call GCD setattributes since
> 14dde9e903bb9a719ebb8f3381da72b19509bc36 "MdeModulePkg/Core: Fix out-
> of-sync issue in GCD", otherwise there may be mismatch of page
> attributes between GCD and gCPU after
> RefreshGcdMemoryAttributesFromPaging() by the calling gCpu->SetMemoryAttributes() in ApplyMemoryProtectionPolicy().
>
> Anyway, that could be in a separated patch. :)
>
> Thanks,
> Star
> -----Original Message-----
> From: edk2-devel [mailto:edk2-devel-bounces@lists.01.org] On Behalf Of
> Jian J Wang
> Sent: Friday, November 3, 2017 8:57 AM
> To: edk2-devel@lists.01.org
> Cc: Laszlo Ersek <lersek@redhat.com>; Yao, Jiewen
> <jiewen.yao@intel.com>; Dong, Eric <eric.dong@intel.com>
> Subject: [edk2] [PATCH v2] UefiCpuPkg/CpuDxe: Fix multiple entries of
> RT_CODE in memory map
>
> > v2
> > a. Fix an issue which will cause setting capability failure if size is smaller
> > than a page.
>
> More than one entry of RT_CODE memory might cause boot problem for
> some old OSs. This patch will fix this issue to keep OS compatibility as much as possible.
>
> More detailed information, please refer to
> https://bugzilla.tianocore.org/show_bug.cgi?id=753
>
> Cc: Eric Dong <eric.dong@intel.com>
> Cc: Jiewen Yao <jiewen.yao@intel.com>
> Cc: Laszlo Ersek <lersek@redhat.com>
> Contributed-under: TianoCore Contribution Agreement 1.1
> Signed-off-by: Jian J Wang <jian.j.wang@intel.com>
> ---
> UefiCpuPkg/CpuDxe/CpuPageTable.c | 18 ++++++++++++++----
> 1 file changed, 14 insertions(+), 4 deletions(-)
>
> diff --git a/UefiCpuPkg/CpuDxe/CpuPageTable.c
> b/UefiCpuPkg/CpuDxe/CpuPageTable.c
> index d312eb66f8..4a7827ebc9 100644
> --- a/UefiCpuPkg/CpuDxe/CpuPageTable.c
> +++ b/UefiCpuPkg/CpuDxe/CpuPageTable.c
> @@ -809,7 +809,9 @@ RefreshGcdMemoryAttributesFromPaging (
> PageLength = 0;
>
> for (Index = 0; Index < NumberOfDescriptors; Index++) {
> - if (MemorySpaceMap[Index].GcdMemoryType ==
> EfiGcdMemoryTypeNonExistent) {
> + if (MemorySpaceMap[Index].GcdMemoryType ==
> EfiGcdMemoryTypeNonExistent
> + || (MemorySpaceMap[Index].BaseAddress & EFI_PAGE_MASK) != 0
> + || (MemorySpaceMap[Index].Length & EFI_PAGE_MASK) != 0) {
> continue;
> }
>
> @@ -829,6 +831,15 @@ RefreshGcdMemoryAttributesFromPaging (
> // Sync real page attributes to GCD
> BaseAddress = MemorySpaceMap[Index].BaseAddress;
> MemorySpaceLength = MemorySpaceMap[Index].Length;
> + Capabilities = MemorySpaceMap[Index].Capabilities |
> + EFI_MEMORY_PAGETYPE_MASK;
> + Status = gDS->SetMemorySpaceCapabilities (
> + BaseAddress,
> + MemorySpaceLength,
> + Capabilities
> + );
> + ASSERT_EFI_ERROR (Status);
> +
> while (MemorySpaceLength > 0) {
> if (PageLength == 0) {
> PageEntry = GetPageTableEntry (&PagingContext, BaseAddress,
> &PageAttribute); @@ -846,7 +857,6 @@
> RefreshGcdMemoryAttributesFromPaging (
> if (Attributes != (MemorySpaceMap[Index].Attributes &
> EFI_MEMORY_PAGETYPE_MASK)) {
> DoUpdate = TRUE;
> Attributes |= (MemorySpaceMap[Index].Attributes &
> ~EFI_MEMORY_PAGETYPE_MASK);
> - Capabilities = Attributes | MemorySpaceMap[Index].Capabilities;
> } else {
> DoUpdate = FALSE;
> }
> @@ -854,8 +864,8 @@ RefreshGcdMemoryAttributesFromPaging (
>
> Length = MIN (PageLength, MemorySpaceLength);
> if (DoUpdate) {
> - gDS->SetMemorySpaceCapabilities (BaseAddress, Length, Capabilities);
> - gDS->SetMemorySpaceAttributes (BaseAddress, Length, Attributes);
> + Status = gDS->SetMemorySpaceAttributes (BaseAddress, Length,
> Attributes);
> + ASSERT_EFI_ERROR (Status);
> DEBUG ((DEBUG_INFO, "Update memory space attribute: [%02d]
> %016lx - %016lx (%08lx -> %08lx)\r\n",
> Index, BaseAddress, BaseAddress + Length - 1,
> MemorySpaceMap[Index].Attributes,
> Attributes));
> --
> 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] 17+ messages in thread
* Re: [PATCH v2] UefiCpuPkg/CpuDxe: Fix multiple entries of RT_CODE in memory map
2017-11-08 3:13 ` Zeng, Star
@ 2017-11-08 13:25 ` Laszlo Ersek
0 siblings, 0 replies; 17+ messages in thread
From: Laszlo Ersek @ 2017-11-08 13:25 UTC (permalink / raw)
To: Zeng, Star, Wang, Jian J, edk2-devel@lists.01.org; +Cc: Yao, Jiewen, Dong, Eric
On 11/08/17 04:13, Zeng, Star wrote:
> https://bugzilla.tianocore.org/show_bug.cgi?id=763 is submitted to track this.
I'd like to ask a question about the impact of BZ#763:
Regarding BZ#753 ("UEFI memory map regression (runtime code entry
splitting) introduced by c1cab54ce57c"), we seem to have agreed that any
firmware that has commit c1cab54ce57c will need the fix for BZ#753.
Otherwise the OS may get an invalid UEFI memory map, and if the OS is
not specifically prepared for that, it can crash.
My question is if the issue described in BZ#763 is similarly serious;
i.e., whether any firmware that has commit 14dde9e903bb
("MdeModulePkg/Core: Fix out-of-sync issue in GCD", 2017-09-19) should
urgently get the fix for BZ#763.
In other words, does BZ#763 have a direct OS-level impact that needs to
be fixed quickly?
Thanks!
Laszlo
> -----Original Message-----
> From: Wang, Jian J
> Sent: Tuesday, November 7, 2017 8:55 AM
> To: Zeng, Star <star.zeng@intel.com>; edk2-devel@lists.01.org
> Cc: Laszlo Ersek <lersek@redhat.com>; Yao, Jiewen <jiewen.yao@intel.com>; Dong, Eric <eric.dong@intel.com>
> Subject: RE: [edk2] [PATCH v2] UefiCpuPkg/CpuDxe: Fix multiple entries of RT_CODE in memory map
>
> Thanks for the review. And I agree that GCD.SetMemoryAttributes should be used all the time in DxeCore. Let's fix it in another patch.
>
>> -----Original Message-----
>> From: Zeng, Star
>> Sent: Monday, November 06, 2017 5:16 PM
>> To: Wang, Jian J <jian.j.wang@intel.com>; edk2-devel@lists.01.org
>> Cc: Laszlo Ersek <lersek@redhat.com>; Yao, Jiewen
>> <jiewen.yao@intel.com>; Dong, Eric <eric.dong@intel.com>; Zeng, Star
>> <star.zeng@intel.com>
>> Subject: RE: [edk2] [PATCH v2] UefiCpuPkg/CpuDxe: Fix multiple entries
>> of RT_CODE in memory map
>>
>> I am ok to this code approach.
>>
>> Acknowledged-by: Star Zeng <star.zeng@intel.com>
>>
>> Besides, I think except GCD services, DxeCore should not call gCpu-
>>> SetMemoryAttributes() directly, for example
>>> ApplyMemoryProtectionPolicy(), it
>> should have no assumption of the CPU code (to set capabilities), and
>> call GCD setcapabilities first, and then call GCD setattributes since
>> 14dde9e903bb9a719ebb8f3381da72b19509bc36 "MdeModulePkg/Core: Fix out-
>> of-sync issue in GCD", otherwise there may be mismatch of page
>> attributes between GCD and gCPU after
>> RefreshGcdMemoryAttributesFromPaging() by the calling gCpu->SetMemoryAttributes() in ApplyMemoryProtectionPolicy().
>>
>> Anyway, that could be in a separated patch. :)
>>
>> Thanks,
>> Star
>> -----Original Message-----
>> From: edk2-devel [mailto:edk2-devel-bounces@lists.01.org] On Behalf Of
>> Jian J Wang
>> Sent: Friday, November 3, 2017 8:57 AM
>> To: edk2-devel@lists.01.org
>> Cc: Laszlo Ersek <lersek@redhat.com>; Yao, Jiewen
>> <jiewen.yao@intel.com>; Dong, Eric <eric.dong@intel.com>
>> Subject: [edk2] [PATCH v2] UefiCpuPkg/CpuDxe: Fix multiple entries of
>> RT_CODE in memory map
>>
>>> v2
>>> a. Fix an issue which will cause setting capability failure if size is smaller
>>> than a page.
>>
>> More than one entry of RT_CODE memory might cause boot problem for
>> some old OSs. This patch will fix this issue to keep OS compatibility as much as possible.
>>
>> More detailed information, please refer to
>> https://bugzilla.tianocore.org/show_bug.cgi?id=753
>>
>> Cc: Eric Dong <eric.dong@intel.com>
>> Cc: Jiewen Yao <jiewen.yao@intel.com>
>> Cc: Laszlo Ersek <lersek@redhat.com>
>> Contributed-under: TianoCore Contribution Agreement 1.1
>> Signed-off-by: Jian J Wang <jian.j.wang@intel.com>
>> ---
>> UefiCpuPkg/CpuDxe/CpuPageTable.c | 18 ++++++++++++++----
>> 1 file changed, 14 insertions(+), 4 deletions(-)
>>
>> diff --git a/UefiCpuPkg/CpuDxe/CpuPageTable.c
>> b/UefiCpuPkg/CpuDxe/CpuPageTable.c
>> index d312eb66f8..4a7827ebc9 100644
>> --- a/UefiCpuPkg/CpuDxe/CpuPageTable.c
>> +++ b/UefiCpuPkg/CpuDxe/CpuPageTable.c
>> @@ -809,7 +809,9 @@ RefreshGcdMemoryAttributesFromPaging (
>> PageLength = 0;
>>
>> for (Index = 0; Index < NumberOfDescriptors; Index++) {
>> - if (MemorySpaceMap[Index].GcdMemoryType ==
>> EfiGcdMemoryTypeNonExistent) {
>> + if (MemorySpaceMap[Index].GcdMemoryType ==
>> EfiGcdMemoryTypeNonExistent
>> + || (MemorySpaceMap[Index].BaseAddress & EFI_PAGE_MASK) != 0
>> + || (MemorySpaceMap[Index].Length & EFI_PAGE_MASK) != 0) {
>> continue;
>> }
>>
>> @@ -829,6 +831,15 @@ RefreshGcdMemoryAttributesFromPaging (
>> // Sync real page attributes to GCD
>> BaseAddress = MemorySpaceMap[Index].BaseAddress;
>> MemorySpaceLength = MemorySpaceMap[Index].Length;
>> + Capabilities = MemorySpaceMap[Index].Capabilities |
>> + EFI_MEMORY_PAGETYPE_MASK;
>> + Status = gDS->SetMemorySpaceCapabilities (
>> + BaseAddress,
>> + MemorySpaceLength,
>> + Capabilities
>> + );
>> + ASSERT_EFI_ERROR (Status);
>> +
>> while (MemorySpaceLength > 0) {
>> if (PageLength == 0) {
>> PageEntry = GetPageTableEntry (&PagingContext, BaseAddress,
>> &PageAttribute); @@ -846,7 +857,6 @@
>> RefreshGcdMemoryAttributesFromPaging (
>> if (Attributes != (MemorySpaceMap[Index].Attributes &
>> EFI_MEMORY_PAGETYPE_MASK)) {
>> DoUpdate = TRUE;
>> Attributes |= (MemorySpaceMap[Index].Attributes &
>> ~EFI_MEMORY_PAGETYPE_MASK);
>> - Capabilities = Attributes | MemorySpaceMap[Index].Capabilities;
>> } else {
>> DoUpdate = FALSE;
>> }
>> @@ -854,8 +864,8 @@ RefreshGcdMemoryAttributesFromPaging (
>>
>> Length = MIN (PageLength, MemorySpaceLength);
>> if (DoUpdate) {
>> - gDS->SetMemorySpaceCapabilities (BaseAddress, Length, Capabilities);
>> - gDS->SetMemorySpaceAttributes (BaseAddress, Length, Attributes);
>> + Status = gDS->SetMemorySpaceAttributes (BaseAddress, Length,
>> Attributes);
>> + ASSERT_EFI_ERROR (Status);
>> DEBUG ((DEBUG_INFO, "Update memory space attribute: [%02d]
>> %016lx - %016lx (%08lx -> %08lx)\r\n",
>> Index, BaseAddress, BaseAddress + Length - 1,
>> MemorySpaceMap[Index].Attributes,
>> Attributes));
>> --
>> 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] 17+ messages in thread
* Re: [PATCH v2] UefiCpuPkg/CpuDxe: Fix multiple entries of RT_CODE in memory map
2017-11-03 0:57 Jian J Wang
2017-11-06 9:15 ` Zeng, Star
@ 2017-11-07 17:13 ` Laszlo Ersek
2017-11-08 0:10 ` Wang, Jian J
2017-11-08 4:41 ` Ni, Ruiyu
2 siblings, 1 reply; 17+ messages in thread
From: Laszlo Ersek @ 2017-11-07 17:13 UTC (permalink / raw)
To: Jian J Wang, edk2-devel; +Cc: Jiewen Yao, Eric Dong
sorry about the late response
On 11/03/17 01:57, Jian J Wang wrote:
>> v2
>> a. Fix an issue which will cause setting capability failure if size is smaller
>> than a page.
>
> More than one entry of RT_CODE memory might cause boot problem for some
> old OSs. This patch will fix this issue to keep OS compatibility as much
> as possible.
>
> More detailed information, please refer to
> https://bugzilla.tianocore.org/show_bug.cgi?id=753
>
> Cc: Eric Dong <eric.dong@intel.com>
> Cc: Jiewen Yao <jiewen.yao@intel.com>
> Cc: Laszlo Ersek <lersek@redhat.com>
> Contributed-under: TianoCore Contribution Agreement 1.1
> Signed-off-by: Jian J Wang <jian.j.wang@intel.com>
> ---
> UefiCpuPkg/CpuDxe/CpuPageTable.c | 18 ++++++++++++++----
> 1 file changed, 14 insertions(+), 4 deletions(-)
>
> diff --git a/UefiCpuPkg/CpuDxe/CpuPageTable.c b/UefiCpuPkg/CpuDxe/CpuPageTable.c
> index d312eb66f8..4a7827ebc9 100644
> --- a/UefiCpuPkg/CpuDxe/CpuPageTable.c
> +++ b/UefiCpuPkg/CpuDxe/CpuPageTable.c
> @@ -809,7 +809,9 @@ RefreshGcdMemoryAttributesFromPaging (
> PageLength = 0;
>
> for (Index = 0; Index < NumberOfDescriptors; Index++) {
> - if (MemorySpaceMap[Index].GcdMemoryType == EfiGcdMemoryTypeNonExistent) {
> + if (MemorySpaceMap[Index].GcdMemoryType == EfiGcdMemoryTypeNonExistent
> + || (MemorySpaceMap[Index].BaseAddress & EFI_PAGE_MASK) != 0
> + || (MemorySpaceMap[Index].Length & EFI_PAGE_MASK) != 0) {
> continue;
> }
When exactly do the new conditions match?
I thought the base addresses and the lengths in the GCD memory space map
are all page aligned. Is that not the case?
If these conditions are just a sanity check (i.e. we never expect them
to fire), then should we perpahs turn them into ASSERT()s?
>
> @@ -829,6 +831,15 @@ RefreshGcdMemoryAttributesFromPaging (
> // Sync real page attributes to GCD
> BaseAddress = MemorySpaceMap[Index].BaseAddress;
> MemorySpaceLength = MemorySpaceMap[Index].Length;
> + Capabilities = MemorySpaceMap[Index].Capabilities |
> + EFI_MEMORY_PAGETYPE_MASK;
> + Status = gDS->SetMemorySpaceCapabilities (
> + BaseAddress,
> + MemorySpaceLength,
> + Capabilities
> + );
> + ASSERT_EFI_ERROR (Status);
> +
OK, so I guess we simply add EFI_MEMORY_PAGETYPE_MASK to the
capabilities of all memory space map entries that have a type different
from non-existent. We discussed it before and (apparently) it is
considered safe.
> while (MemorySpaceLength > 0) {
> if (PageLength == 0) {
> PageEntry = GetPageTableEntry (&PagingContext, BaseAddress, &PageAttribute);
> @@ -846,7 +857,6 @@ RefreshGcdMemoryAttributesFromPaging (
> if (Attributes != (MemorySpaceMap[Index].Attributes & EFI_MEMORY_PAGETYPE_MASK)) {
> DoUpdate = TRUE;
> Attributes |= (MemorySpaceMap[Index].Attributes & ~EFI_MEMORY_PAGETYPE_MASK);
> - Capabilities = Attributes | MemorySpaceMap[Index].Capabilities;
> } else {
> DoUpdate = FALSE;
> }
> @@ -854,8 +864,8 @@ RefreshGcdMemoryAttributesFromPaging (
>
> Length = MIN (PageLength, MemorySpaceLength);
> if (DoUpdate) {
> - gDS->SetMemorySpaceCapabilities (BaseAddress, Length, Capabilities);
> - gDS->SetMemorySpaceAttributes (BaseAddress, Length, Attributes);
> + Status = gDS->SetMemorySpaceAttributes (BaseAddress, Length, Attributes);
> + ASSERT_EFI_ERROR (Status);
> DEBUG ((DEBUG_INFO, "Update memory space attribute: [%02d] %016lx - %016lx (%08lx -> %08lx)\r\n",
> Index, BaseAddress, BaseAddress + Length - 1,
> MemorySpaceMap[Index].Attributes, Attributes));
>
I'll let you decide about the EFI_PAGE_MASK conditions near the top.
Acked-by: Laszlo Ersek <lersek@redhat.com>
Thanks
Laszlo
^ permalink raw reply [flat|nested] 17+ messages in thread
* Re: [PATCH v2] UefiCpuPkg/CpuDxe: Fix multiple entries of RT_CODE in memory map
2017-11-07 17:13 ` Laszlo Ersek
@ 2017-11-08 0:10 ` Wang, Jian J
2017-11-08 9:10 ` Wang, Jian J
2017-11-08 14:17 ` Laszlo Ersek
0 siblings, 2 replies; 17+ messages in thread
From: Wang, Jian J @ 2017-11-08 0:10 UTC (permalink / raw)
To: Laszlo Ersek, edk2-devel@lists.01.org; +Cc: Yao, Jiewen, Dong, Eric
Hi Laszlo,
> -----Original Message-----
> From: Laszlo Ersek [mailto:lersek@redhat.com]
> Sent: Wednesday, November 08, 2017 1:14 AM
> To: Wang, Jian J <jian.j.wang@intel.com>; edk2-devel@lists.01.org
> Cc: Yao, Jiewen <jiewen.yao@intel.com>; Dong, Eric <eric.dong@intel.com>
> Subject: Re: [edk2] [PATCH v2] UefiCpuPkg/CpuDxe: Fix multiple entries of
> RT_CODE in memory map
>
> sorry about the late response
>
> On 11/03/17 01:57, Jian J Wang wrote:
> >> v2
> >> a. Fix an issue which will cause setting capability failure if size is smaller
> >> than a page.
> >
> > More than one entry of RT_CODE memory might cause boot problem for
> some
> > old OSs. This patch will fix this issue to keep OS compatibility as much
> > as possible.
> >
> > More detailed information, please refer to
> > https://bugzilla.tianocore.org/show_bug.cgi?id=753
> >
> > Cc: Eric Dong <eric.dong@intel.com>
> > Cc: Jiewen Yao <jiewen.yao@intel.com>
> > Cc: Laszlo Ersek <lersek@redhat.com>
> > Contributed-under: TianoCore Contribution Agreement 1.1
> > Signed-off-by: Jian J Wang <jian.j.wang@intel.com>
> > ---
> > UefiCpuPkg/CpuDxe/CpuPageTable.c | 18 ++++++++++++++----
> > 1 file changed, 14 insertions(+), 4 deletions(-)
> >
> > diff --git a/UefiCpuPkg/CpuDxe/CpuPageTable.c
> b/UefiCpuPkg/CpuDxe/CpuPageTable.c
> > index d312eb66f8..4a7827ebc9 100644
> > --- a/UefiCpuPkg/CpuDxe/CpuPageTable.c
> > +++ b/UefiCpuPkg/CpuDxe/CpuPageTable.c
> > @@ -809,7 +809,9 @@ RefreshGcdMemoryAttributesFromPaging (
> > PageLength = 0;
> >
> > for (Index = 0; Index < NumberOfDescriptors; Index++) {
> > - if (MemorySpaceMap[Index].GcdMemoryType ==
> EfiGcdMemoryTypeNonExistent) {
> > + if (MemorySpaceMap[Index].GcdMemoryType ==
> EfiGcdMemoryTypeNonExistent
> > + || (MemorySpaceMap[Index].BaseAddress & EFI_PAGE_MASK) != 0
> > + || (MemorySpaceMap[Index].Length & EFI_PAGE_MASK) != 0) {
> > continue;
> > }
>
> When exactly do the new conditions match?
>
> I thought the base addresses and the lengths in the GCD memory space map
> are all page aligned. Is that not the case?
>
> If these conditions are just a sanity check (i.e. we never expect them
> to fire), then should we perpahs turn them into ASSERT()s?
>
I found that there's a mmio entry in memory map on OVMF which has size
less than a page. I didn't encounter this before. Maybe some recent changes
in other part of EDKII caused this situation. So ASSERT is not enough.
> >
> > @@ -829,6 +831,15 @@ RefreshGcdMemoryAttributesFromPaging (
> > // Sync real page attributes to GCD
> > BaseAddress = MemorySpaceMap[Index].BaseAddress;
> > MemorySpaceLength = MemorySpaceMap[Index].Length;
> > + Capabilities = MemorySpaceMap[Index].Capabilities |
> > + EFI_MEMORY_PAGETYPE_MASK;
> > + Status = gDS->SetMemorySpaceCapabilities (
> > + BaseAddress,
> > + MemorySpaceLength,
> > + Capabilities
> > + );
> > + ASSERT_EFI_ERROR (Status);
> > +
>
> OK, so I guess we simply add EFI_MEMORY_PAGETYPE_MASK to the
> capabilities of all memory space map entries that have a type different
> from non-existent. We discussed it before and (apparently) it is
> considered safe.
>
Yes. I've validated different OSs boot. It's safe to stay this way.
> > while (MemorySpaceLength > 0) {
> > if (PageLength == 0) {
> > PageEntry = GetPageTableEntry (&PagingContext, BaseAddress,
> &PageAttribute);
> > @@ -846,7 +857,6 @@ RefreshGcdMemoryAttributesFromPaging (
> > if (Attributes != (MemorySpaceMap[Index].Attributes &
> EFI_MEMORY_PAGETYPE_MASK)) {
> > DoUpdate = TRUE;
> > Attributes |= (MemorySpaceMap[Index].Attributes &
> ~EFI_MEMORY_PAGETYPE_MASK);
> > - Capabilities = Attributes | MemorySpaceMap[Index].Capabilities;
> > } else {
> > DoUpdate = FALSE;
> > }
> > @@ -854,8 +864,8 @@ RefreshGcdMemoryAttributesFromPaging (
> >
> > Length = MIN (PageLength, MemorySpaceLength);
> > if (DoUpdate) {
> > - gDS->SetMemorySpaceCapabilities (BaseAddress, Length, Capabilities);
> > - gDS->SetMemorySpaceAttributes (BaseAddress, Length, Attributes);
> > + Status = gDS->SetMemorySpaceAttributes (BaseAddress, Length,
> Attributes);
> > + ASSERT_EFI_ERROR (Status);
> > DEBUG ((DEBUG_INFO, "Update memory space attribute: [%02d] %016lx
> - %016lx (%08lx -> %08lx)\r\n",
> > Index, BaseAddress, BaseAddress + Length - 1,
> > MemorySpaceMap[Index].Attributes, Attributes));
> >
>
> I'll let you decide about the EFI_PAGE_MASK conditions near the top.
>
> Acked-by: Laszlo Ersek <lersek@redhat.com>
>
Thanks.
> Thanks
> Laszlo
^ permalink raw reply [flat|nested] 17+ messages in thread
* Re: [PATCH v2] UefiCpuPkg/CpuDxe: Fix multiple entries of RT_CODE in memory map
2017-11-08 0:10 ` Wang, Jian J
@ 2017-11-08 9:10 ` Wang, Jian J
2017-11-08 14:17 ` Laszlo Ersek
1 sibling, 0 replies; 17+ messages in thread
From: Wang, Jian J @ 2017-11-08 9:10 UTC (permalink / raw)
To: Wang, Jian J, Laszlo Ersek, edk2-devel@lists.01.org
Cc: Yao, Jiewen, Dong, Eric
Some updates below
> -----Original Message-----
> From: edk2-devel [mailto:edk2-devel-bounces@lists.01.org] On Behalf Of Wang,
> Jian J
> Sent: Wednesday, November 08, 2017 8:11 AM
> To: Laszlo Ersek <lersek@redhat.com>; edk2-devel@lists.01.org
> Cc: Yao, Jiewen <jiewen.yao@intel.com>; Dong, Eric <eric.dong@intel.com>
> Subject: Re: [edk2] [PATCH v2] UefiCpuPkg/CpuDxe: Fix multiple entries of
> RT_CODE in memory map
>
> Hi Laszlo,
>
> > -----Original Message-----
> > From: Laszlo Ersek [mailto:lersek@redhat.com]
> > Sent: Wednesday, November 08, 2017 1:14 AM
> > To: Wang, Jian J <jian.j.wang@intel.com>; edk2-devel@lists.01.org
> > Cc: Yao, Jiewen <jiewen.yao@intel.com>; Dong, Eric <eric.dong@intel.com>
> > Subject: Re: [edk2] [PATCH v2] UefiCpuPkg/CpuDxe: Fix multiple entries of
> > RT_CODE in memory map
> >
> > sorry about the late response
> >
> > On 11/03/17 01:57, Jian J Wang wrote:
> > >> v2
> > >> a. Fix an issue which will cause setting capability failure if size is smaller
> > >> than a page.
> > >
> > > More than one entry of RT_CODE memory might cause boot problem for
> > some
> > > old OSs. This patch will fix this issue to keep OS compatibility as much
> > > as possible.
> > >
> > > More detailed information, please refer to
> > > https://bugzilla.tianocore.org/show_bug.cgi?id=753
> > >
> > > Cc: Eric Dong <eric.dong@intel.com>
> > > Cc: Jiewen Yao <jiewen.yao@intel.com>
> > > Cc: Laszlo Ersek <lersek@redhat.com>
> > > Contributed-under: TianoCore Contribution Agreement 1.1
> > > Signed-off-by: Jian J Wang <jian.j.wang@intel.com>
> > > ---
> > > UefiCpuPkg/CpuDxe/CpuPageTable.c | 18 ++++++++++++++----
> > > 1 file changed, 14 insertions(+), 4 deletions(-)
> > >
> > > diff --git a/UefiCpuPkg/CpuDxe/CpuPageTable.c
> > b/UefiCpuPkg/CpuDxe/CpuPageTable.c
> > > index d312eb66f8..4a7827ebc9 100644
> > > --- a/UefiCpuPkg/CpuDxe/CpuPageTable.c
> > > +++ b/UefiCpuPkg/CpuDxe/CpuPageTable.c
> > > @@ -809,7 +809,9 @@ RefreshGcdMemoryAttributesFromPaging (
> > > PageLength = 0;
> > >
> > > for (Index = 0; Index < NumberOfDescriptors; Index++) {
> > > - if (MemorySpaceMap[Index].GcdMemoryType ==
> > EfiGcdMemoryTypeNonExistent) {
> > > + if (MemorySpaceMap[Index].GcdMemoryType ==
> > EfiGcdMemoryTypeNonExistent
> > > + || (MemorySpaceMap[Index].BaseAddress & EFI_PAGE_MASK) != 0
> > > + || (MemorySpaceMap[Index].Length & EFI_PAGE_MASK) != 0) {
> > > continue;
> > > }
> >
> > When exactly do the new conditions match?
> >
> > I thought the base addresses and the lengths in the GCD memory space map
> > are all page aligned. Is that not the case?
> >
> > If these conditions are just a sanity check (i.e. we never expect them
> > to fire), then should we perpahs turn them into ASSERT()s?
> >
>
> I found that there's a mmio entry in memory map on OVMF which has size
> less than a page. I didn't encounter this before. Maybe some recent changes
> in other part of EDKII caused this situation. So ASSERT is not enough.
>
I changed my original fix in v2 to not check the Address and Size. Instead,
I'll use the Status of gDS->SetMemorySpaceCapabilities() to skip those memory
block which cannot be updated with new capabilities. This can avoid the
assumption that only the address and size will cause the calling failure. And I
found a logic hole in code. You'll find new changes in v3 patch.
> > >
> > > @@ -829,6 +831,15 @@ RefreshGcdMemoryAttributesFromPaging (
> > > // Sync real page attributes to GCD
> > > BaseAddress = MemorySpaceMap[Index].BaseAddress;
> > > MemorySpaceLength = MemorySpaceMap[Index].Length;
> > > + Capabilities = MemorySpaceMap[Index].Capabilities |
> > > + EFI_MEMORY_PAGETYPE_MASK;
> > > + Status = gDS->SetMemorySpaceCapabilities (
> > > + BaseAddress,
> > > + MemorySpaceLength,
> > > + Capabilities
> > > + );
> > > + ASSERT_EFI_ERROR (Status);
> > > +
> >
> > OK, so I guess we simply add EFI_MEMORY_PAGETYPE_MASK to the
> > capabilities of all memory space map entries that have a type different
> > from non-existent. We discussed it before and (apparently) it is
> > considered safe.
> >
>
> Yes. I've validated different OSs boot. It's safe to stay this way.
>
> > > while (MemorySpaceLength > 0) {
> > > if (PageLength == 0) {
> > > PageEntry = GetPageTableEntry (&PagingContext, BaseAddress,
> > &PageAttribute);
> > > @@ -846,7 +857,6 @@ RefreshGcdMemoryAttributesFromPaging (
> > > if (Attributes != (MemorySpaceMap[Index].Attributes &
> > EFI_MEMORY_PAGETYPE_MASK)) {
> > > DoUpdate = TRUE;
> > > Attributes |= (MemorySpaceMap[Index].Attributes &
> > ~EFI_MEMORY_PAGETYPE_MASK);
> > > - Capabilities = Attributes | MemorySpaceMap[Index].Capabilities;
> > > } else {
> > > DoUpdate = FALSE;
> > > }
> > > @@ -854,8 +864,8 @@ RefreshGcdMemoryAttributesFromPaging (
> > >
> > > Length = MIN (PageLength, MemorySpaceLength);
> > > if (DoUpdate) {
> > > - gDS->SetMemorySpaceCapabilities (BaseAddress, Length, Capabilities);
> > > - gDS->SetMemorySpaceAttributes (BaseAddress, Length, Attributes);
> > > + Status = gDS->SetMemorySpaceAttributes (BaseAddress, Length,
> > Attributes);
> > > + ASSERT_EFI_ERROR (Status);
> > > DEBUG ((DEBUG_INFO, "Update memory space attribute:
> [%02d] %016lx
> > - %016lx (%08lx -> %08lx)\r\n",
> > > Index, BaseAddress, BaseAddress + Length - 1,
> > > MemorySpaceMap[Index].Attributes, Attributes));
> > >
> >
> > I'll let you decide about the EFI_PAGE_MASK conditions near the top.
> >
> > Acked-by: Laszlo Ersek <lersek@redhat.com>
> >
>
> Thanks.
>
> > Thanks
> > Laszlo
> _______________________________________________
> edk2-devel mailing list
> edk2-devel@lists.01.org
> https://lists.01.org/mailman/listinfo/edk2-devel
^ permalink raw reply [flat|nested] 17+ messages in thread
* Re: [PATCH v2] UefiCpuPkg/CpuDxe: Fix multiple entries of RT_CODE in memory map
2017-11-08 0:10 ` Wang, Jian J
2017-11-08 9:10 ` Wang, Jian J
@ 2017-11-08 14:17 ` Laszlo Ersek
2017-11-09 0:41 ` Wang, Jian J
1 sibling, 1 reply; 17+ messages in thread
From: Laszlo Ersek @ 2017-11-08 14:17 UTC (permalink / raw)
To: Wang, Jian J, edk2-devel@lists.01.org; +Cc: Yao, Jiewen, Dong, Eric
On 11/08/17 01:10, Wang, Jian J wrote:
> Hi Laszlo,
>
>> -----Original Message-----
>> From: Laszlo Ersek [mailto:lersek@redhat.com]
>> Sent: Wednesday, November 08, 2017 1:14 AM
>> To: Wang, Jian J <jian.j.wang@intel.com>; edk2-devel@lists.01.org
>> Cc: Yao, Jiewen <jiewen.yao@intel.com>; Dong, Eric <eric.dong@intel.com>
>> Subject: Re: [edk2] [PATCH v2] UefiCpuPkg/CpuDxe: Fix multiple entries of
>> RT_CODE in memory map
>>
>> sorry about the late response
>>
>> On 11/03/17 01:57, Jian J Wang wrote:
>>>> v2
>>>> a. Fix an issue which will cause setting capability failure if size is smaller
>>>> than a page.
>>>
>>> More than one entry of RT_CODE memory might cause boot problem for
>> some
>>> old OSs. This patch will fix this issue to keep OS compatibility as much
>>> as possible.
>>>
>>> More detailed information, please refer to
>>> https://bugzilla.tianocore.org/show_bug.cgi?id=753
>>>
>>> Cc: Eric Dong <eric.dong@intel.com>
>>> Cc: Jiewen Yao <jiewen.yao@intel.com>
>>> Cc: Laszlo Ersek <lersek@redhat.com>
>>> Contributed-under: TianoCore Contribution Agreement 1.1
>>> Signed-off-by: Jian J Wang <jian.j.wang@intel.com>
>>> ---
>>> UefiCpuPkg/CpuDxe/CpuPageTable.c | 18 ++++++++++++++----
>>> 1 file changed, 14 insertions(+), 4 deletions(-)
>>>
>>> diff --git a/UefiCpuPkg/CpuDxe/CpuPageTable.c
>> b/UefiCpuPkg/CpuDxe/CpuPageTable.c
>>> index d312eb66f8..4a7827ebc9 100644
>>> --- a/UefiCpuPkg/CpuDxe/CpuPageTable.c
>>> +++ b/UefiCpuPkg/CpuDxe/CpuPageTable.c
>>> @@ -809,7 +809,9 @@ RefreshGcdMemoryAttributesFromPaging (
>>> PageLength = 0;
>>>
>>> for (Index = 0; Index < NumberOfDescriptors; Index++) {
>>> - if (MemorySpaceMap[Index].GcdMemoryType ==
>> EfiGcdMemoryTypeNonExistent) {
>>> + if (MemorySpaceMap[Index].GcdMemoryType ==
>> EfiGcdMemoryTypeNonExistent
>>> + || (MemorySpaceMap[Index].BaseAddress & EFI_PAGE_MASK) != 0
>>> + || (MemorySpaceMap[Index].Length & EFI_PAGE_MASK) != 0) {
>>> continue;
>>> }
>>
>> When exactly do the new conditions match?
>>
>> I thought the base addresses and the lengths in the GCD memory space map
>> are all page aligned. Is that not the case?
>>
>> If these conditions are just a sanity check (i.e. we never expect them
>> to fire), then should we perpahs turn them into ASSERT()s?
>>
>
> I found that there's a mmio entry in memory map on OVMF which has size
> less than a page. I didn't encounter this before. Maybe some recent changes
> in other part of EDKII caused this situation. So ASSERT is not enough.
Can you describe the base address and size of this MMIO entry?
I don't see how it is possible to add such an entry in the first place
-- if you try to add it in PEI, via a HOB, then IIRC HobLib will
ASSERT(). If you try to add it with gDS->AddMemorySpace() in DXE, then
the call should fail.
I believe it might be useful to investigate this entry more closely.
Knowing the base address could help us.
Thanks!
Laszlo
^ permalink raw reply [flat|nested] 17+ messages in thread
* Re: [PATCH v2] UefiCpuPkg/CpuDxe: Fix multiple entries of RT_CODE in memory map
2017-11-08 14:17 ` Laszlo Ersek
@ 2017-11-09 0:41 ` Wang, Jian J
2017-11-09 1:48 ` Yao, Jiewen
0 siblings, 1 reply; 17+ messages in thread
From: Wang, Jian J @ 2017-11-09 0:41 UTC (permalink / raw)
To: Laszlo Ersek, edk2-devel@lists.01.org; +Cc: Yao, Jiewen, Dong, Eric
Hi Laszlo,
The memory range is 00000000FED00000 - 00000000FED003FF. Actually I don't
know if it's for MMIO or not. I might be mess it with other things.
Thanks,
Jian
> -----Original Message-----
> From: Laszlo Ersek [mailto:lersek@redhat.com]
> Sent: Wednesday, November 08, 2017 10:17 PM
> To: Wang, Jian J <jian.j.wang@intel.com>; edk2-devel@lists.01.org
> Cc: Yao, Jiewen <jiewen.yao@intel.com>; Dong, Eric <eric.dong@intel.com>
> Subject: Re: [edk2] [PATCH v2] UefiCpuPkg/CpuDxe: Fix multiple entries of
> RT_CODE in memory map
>
> On 11/08/17 01:10, Wang, Jian J wrote:
> > Hi Laszlo,
> >
> >> -----Original Message-----
> >> From: Laszlo Ersek [mailto:lersek@redhat.com]
> >> Sent: Wednesday, November 08, 2017 1:14 AM
> >> To: Wang, Jian J <jian.j.wang@intel.com>; edk2-devel@lists.01.org
> >> Cc: Yao, Jiewen <jiewen.yao@intel.com>; Dong, Eric <eric.dong@intel.com>
> >> Subject: Re: [edk2] [PATCH v2] UefiCpuPkg/CpuDxe: Fix multiple entries of
> >> RT_CODE in memory map
> >>
> >> sorry about the late response
> >>
> >> On 11/03/17 01:57, Jian J Wang wrote:
> >>>> v2
> >>>> a. Fix an issue which will cause setting capability failure if size is smaller
> >>>> than a page.
> >>>
> >>> More than one entry of RT_CODE memory might cause boot problem for
> >> some
> >>> old OSs. This patch will fix this issue to keep OS compatibility as much
> >>> as possible.
> >>>
> >>> More detailed information, please refer to
> >>> https://bugzilla.tianocore.org/show_bug.cgi?id=753
> >>>
> >>> Cc: Eric Dong <eric.dong@intel.com>
> >>> Cc: Jiewen Yao <jiewen.yao@intel.com>
> >>> Cc: Laszlo Ersek <lersek@redhat.com>
> >>> Contributed-under: TianoCore Contribution Agreement 1.1
> >>> Signed-off-by: Jian J Wang <jian.j.wang@intel.com>
> >>> ---
> >>> UefiCpuPkg/CpuDxe/CpuPageTable.c | 18 ++++++++++++++----
> >>> 1 file changed, 14 insertions(+), 4 deletions(-)
> >>>
> >>> diff --git a/UefiCpuPkg/CpuDxe/CpuPageTable.c
> >> b/UefiCpuPkg/CpuDxe/CpuPageTable.c
> >>> index d312eb66f8..4a7827ebc9 100644
> >>> --- a/UefiCpuPkg/CpuDxe/CpuPageTable.c
> >>> +++ b/UefiCpuPkg/CpuDxe/CpuPageTable.c
> >>> @@ -809,7 +809,9 @@ RefreshGcdMemoryAttributesFromPaging (
> >>> PageLength = 0;
> >>>
> >>> for (Index = 0; Index < NumberOfDescriptors; Index++) {
> >>> - if (MemorySpaceMap[Index].GcdMemoryType ==
> >> EfiGcdMemoryTypeNonExistent) {
> >>> + if (MemorySpaceMap[Index].GcdMemoryType ==
> >> EfiGcdMemoryTypeNonExistent
> >>> + || (MemorySpaceMap[Index].BaseAddress & EFI_PAGE_MASK) != 0
> >>> + || (MemorySpaceMap[Index].Length & EFI_PAGE_MASK) != 0) {
> >>> continue;
> >>> }
> >>
> >> When exactly do the new conditions match?
> >>
> >> I thought the base addresses and the lengths in the GCD memory space map
> >> are all page aligned. Is that not the case?
> >>
> >> If these conditions are just a sanity check (i.e. we never expect them
> >> to fire), then should we perpahs turn them into ASSERT()s?
> >>
> >
> > I found that there's a mmio entry in memory map on OVMF which has size
> > less than a page. I didn't encounter this before. Maybe some recent changes
> > in other part of EDKII caused this situation. So ASSERT is not enough.
>
> Can you describe the base address and size of this MMIO entry?
>
> I don't see how it is possible to add such an entry in the first place
> -- if you try to add it in PEI, via a HOB, then IIRC HobLib will
> ASSERT(). If you try to add it with gDS->AddMemorySpace() in DXE, then
> the call should fail.
>
> I believe it might be useful to investigate this entry more closely.
> Knowing the base address could help us.
>
> Thanks!
> Laszlo
^ permalink raw reply [flat|nested] 17+ messages in thread
* Re: [PATCH v2] UefiCpuPkg/CpuDxe: Fix multiple entries of RT_CODE in memory map
2017-11-09 0:41 ` Wang, Jian J
@ 2017-11-09 1:48 ` Yao, Jiewen
2017-11-09 1:51 ` Wang, Jian J
2017-11-09 12:19 ` Laszlo Ersek
0 siblings, 2 replies; 17+ messages in thread
From: Yao, Jiewen @ 2017-11-09 1:48 UTC (permalink / raw)
To: Wang, Jian J, Laszlo Ersek, edk2-devel@lists.01.org; +Cc: Dong, Eric
FED00000 is HPET region. It is MMIO.
Thank you
Yao Jiewen
> -----Original Message-----
> From: Wang, Jian J
> Sent: Thursday, November 9, 2017 8:42 AM
> To: Laszlo Ersek <lersek@redhat.com>; edk2-devel@lists.01.org
> Cc: Yao, Jiewen <jiewen.yao@intel.com>; Dong, Eric <eric.dong@intel.com>
> Subject: RE: [edk2] [PATCH v2] UefiCpuPkg/CpuDxe: Fix multiple entries of
> RT_CODE in memory map
>
> Hi Laszlo,
>
> The memory range is 00000000FED00000 - 00000000FED003FF. Actually I don't
> know if it's for MMIO or not. I might be mess it with other things.
>
> Thanks,
> Jian
> > -----Original Message-----
> > From: Laszlo Ersek [mailto:lersek@redhat.com]
> > Sent: Wednesday, November 08, 2017 10:17 PM
> > To: Wang, Jian J <jian.j.wang@intel.com>; edk2-devel@lists.01.org
> > Cc: Yao, Jiewen <jiewen.yao@intel.com>; Dong, Eric <eric.dong@intel.com>
> > Subject: Re: [edk2] [PATCH v2] UefiCpuPkg/CpuDxe: Fix multiple entries of
> > RT_CODE in memory map
> >
> > On 11/08/17 01:10, Wang, Jian J wrote:
> > > Hi Laszlo,
> > >
> > >> -----Original Message-----
> > >> From: Laszlo Ersek [mailto:lersek@redhat.com]
> > >> Sent: Wednesday, November 08, 2017 1:14 AM
> > >> To: Wang, Jian J <jian.j.wang@intel.com>; edk2-devel@lists.01.org
> > >> Cc: Yao, Jiewen <jiewen.yao@intel.com>; Dong, Eric <eric.dong@intel.com>
> > >> Subject: Re: [edk2] [PATCH v2] UefiCpuPkg/CpuDxe: Fix multiple entries of
> > >> RT_CODE in memory map
> > >>
> > >> sorry about the late response
> > >>
> > >> On 11/03/17 01:57, Jian J Wang wrote:
> > >>>> v2
> > >>>> a. Fix an issue which will cause setting capability failure if size is smaller
> > >>>> than a page.
> > >>>
> > >>> More than one entry of RT_CODE memory might cause boot problem for
> > >> some
> > >>> old OSs. This patch will fix this issue to keep OS compatibility as much
> > >>> as possible.
> > >>>
> > >>> More detailed information, please refer to
> > >>> https://bugzilla.tianocore.org/show_bug.cgi?id=753
> > >>>
> > >>> Cc: Eric Dong <eric.dong@intel.com>
> > >>> Cc: Jiewen Yao <jiewen.yao@intel.com>
> > >>> Cc: Laszlo Ersek <lersek@redhat.com>
> > >>> Contributed-under: TianoCore Contribution Agreement 1.1
> > >>> Signed-off-by: Jian J Wang <jian.j.wang@intel.com>
> > >>> ---
> > >>> UefiCpuPkg/CpuDxe/CpuPageTable.c | 18 ++++++++++++++----
> > >>> 1 file changed, 14 insertions(+), 4 deletions(-)
> > >>>
> > >>> diff --git a/UefiCpuPkg/CpuDxe/CpuPageTable.c
> > >> b/UefiCpuPkg/CpuDxe/CpuPageTable.c
> > >>> index d312eb66f8..4a7827ebc9 100644
> > >>> --- a/UefiCpuPkg/CpuDxe/CpuPageTable.c
> > >>> +++ b/UefiCpuPkg/CpuDxe/CpuPageTable.c
> > >>> @@ -809,7 +809,9 @@ RefreshGcdMemoryAttributesFromPaging (
> > >>> PageLength = 0;
> > >>>
> > >>> for (Index = 0; Index < NumberOfDescriptors; Index++) {
> > >>> - if (MemorySpaceMap[Index].GcdMemoryType ==
> > >> EfiGcdMemoryTypeNonExistent) {
> > >>> + if (MemorySpaceMap[Index].GcdMemoryType ==
> > >> EfiGcdMemoryTypeNonExistent
> > >>> + || (MemorySpaceMap[Index].BaseAddress &
> EFI_PAGE_MASK) != 0
> > >>> + || (MemorySpaceMap[Index].Length & EFI_PAGE_MASK) != 0)
> {
> > >>> continue;
> > >>> }
> > >>
> > >> When exactly do the new conditions match?
> > >>
> > >> I thought the base addresses and the lengths in the GCD memory space
> map
> > >> are all page aligned. Is that not the case?
> > >>
> > >> If these conditions are just a sanity check (i.e. we never expect them
> > >> to fire), then should we perpahs turn them into ASSERT()s?
> > >>
> > >
> > > I found that there's a mmio entry in memory map on OVMF which has size
> > > less than a page. I didn't encounter this before. Maybe some recent changes
> > > in other part of EDKII caused this situation. So ASSERT is not enough.
> >
> > Can you describe the base address and size of this MMIO entry?
> >
> > I don't see how it is possible to add such an entry in the first place
> > -- if you try to add it in PEI, via a HOB, then IIRC HobLib will
> > ASSERT(). If you try to add it with gDS->AddMemorySpace() in DXE, then
> > the call should fail.
> >
> > I believe it might be useful to investigate this entry more closely.
> > Knowing the base address could help us.
> >
> > Thanks!
> > Laszlo
^ permalink raw reply [flat|nested] 17+ messages in thread
* Re: [PATCH v2] UefiCpuPkg/CpuDxe: Fix multiple entries of RT_CODE in memory map
2017-11-09 1:48 ` Yao, Jiewen
@ 2017-11-09 1:51 ` Wang, Jian J
2017-11-09 12:19 ` Laszlo Ersek
1 sibling, 0 replies; 17+ messages in thread
From: Wang, Jian J @ 2017-11-09 1:51 UTC (permalink / raw)
To: Yao, Jiewen, Laszlo Ersek, edk2-devel@lists.01.org; +Cc: Dong, Eric
Great. Glad to know my memory is still good enough.
> -----Original Message-----
> From: Yao, Jiewen
> Sent: Thursday, November 09, 2017 9:49 AM
> To: Wang, Jian J <jian.j.wang@intel.com>; Laszlo Ersek <lersek@redhat.com>;
> edk2-devel@lists.01.org
> Cc: Dong, Eric <eric.dong@intel.com>
> Subject: RE: [edk2] [PATCH v2] UefiCpuPkg/CpuDxe: Fix multiple entries of
> RT_CODE in memory map
>
> FED00000 is HPET region. It is MMIO.
>
> Thank you
> Yao Jiewen
>
> > -----Original Message-----
> > From: Wang, Jian J
> > Sent: Thursday, November 9, 2017 8:42 AM
> > To: Laszlo Ersek <lersek@redhat.com>; edk2-devel@lists.01.org
> > Cc: Yao, Jiewen <jiewen.yao@intel.com>; Dong, Eric <eric.dong@intel.com>
> > Subject: RE: [edk2] [PATCH v2] UefiCpuPkg/CpuDxe: Fix multiple entries of
> > RT_CODE in memory map
> >
> > Hi Laszlo,
> >
> > The memory range is 00000000FED00000 - 00000000FED003FF. Actually I
> don't
> > know if it's for MMIO or not. I might be mess it with other things.
> >
> > Thanks,
> > Jian
> > > -----Original Message-----
> > > From: Laszlo Ersek [mailto:lersek@redhat.com]
> > > Sent: Wednesday, November 08, 2017 10:17 PM
> > > To: Wang, Jian J <jian.j.wang@intel.com>; edk2-devel@lists.01.org
> > > Cc: Yao, Jiewen <jiewen.yao@intel.com>; Dong, Eric <eric.dong@intel.com>
> > > Subject: Re: [edk2] [PATCH v2] UefiCpuPkg/CpuDxe: Fix multiple entries of
> > > RT_CODE in memory map
> > >
> > > On 11/08/17 01:10, Wang, Jian J wrote:
> > > > Hi Laszlo,
> > > >
> > > >> -----Original Message-----
> > > >> From: Laszlo Ersek [mailto:lersek@redhat.com]
> > > >> Sent: Wednesday, November 08, 2017 1:14 AM
> > > >> To: Wang, Jian J <jian.j.wang@intel.com>; edk2-devel@lists.01.org
> > > >> Cc: Yao, Jiewen <jiewen.yao@intel.com>; Dong, Eric
> <eric.dong@intel.com>
> > > >> Subject: Re: [edk2] [PATCH v2] UefiCpuPkg/CpuDxe: Fix multiple entries
> of
> > > >> RT_CODE in memory map
> > > >>
> > > >> sorry about the late response
> > > >>
> > > >> On 11/03/17 01:57, Jian J Wang wrote:
> > > >>>> v2
> > > >>>> a. Fix an issue which will cause setting capability failure if size is smaller
> > > >>>> than a page.
> > > >>>
> > > >>> More than one entry of RT_CODE memory might cause boot problem
> for
> > > >> some
> > > >>> old OSs. This patch will fix this issue to keep OS compatibility as much
> > > >>> as possible.
> > > >>>
> > > >>> More detailed information, please refer to
> > > >>> https://bugzilla.tianocore.org/show_bug.cgi?id=753
> > > >>>
> > > >>> Cc: Eric Dong <eric.dong@intel.com>
> > > >>> Cc: Jiewen Yao <jiewen.yao@intel.com>
> > > >>> Cc: Laszlo Ersek <lersek@redhat.com>
> > > >>> Contributed-under: TianoCore Contribution Agreement 1.1
> > > >>> Signed-off-by: Jian J Wang <jian.j.wang@intel.com>
> > > >>> ---
> > > >>> UefiCpuPkg/CpuDxe/CpuPageTable.c | 18 ++++++++++++++----
> > > >>> 1 file changed, 14 insertions(+), 4 deletions(-)
> > > >>>
> > > >>> diff --git a/UefiCpuPkg/CpuDxe/CpuPageTable.c
> > > >> b/UefiCpuPkg/CpuDxe/CpuPageTable.c
> > > >>> index d312eb66f8..4a7827ebc9 100644
> > > >>> --- a/UefiCpuPkg/CpuDxe/CpuPageTable.c
> > > >>> +++ b/UefiCpuPkg/CpuDxe/CpuPageTable.c
> > > >>> @@ -809,7 +809,9 @@ RefreshGcdMemoryAttributesFromPaging (
> > > >>> PageLength = 0;
> > > >>>
> > > >>> for (Index = 0; Index < NumberOfDescriptors; Index++) {
> > > >>> - if (MemorySpaceMap[Index].GcdMemoryType ==
> > > >> EfiGcdMemoryTypeNonExistent) {
> > > >>> + if (MemorySpaceMap[Index].GcdMemoryType ==
> > > >> EfiGcdMemoryTypeNonExistent
> > > >>> + || (MemorySpaceMap[Index].BaseAddress &
> > EFI_PAGE_MASK) != 0
> > > >>> + || (MemorySpaceMap[Index].Length & EFI_PAGE_MASK) != 0)
> > {
> > > >>> continue;
> > > >>> }
> > > >>
> > > >> When exactly do the new conditions match?
> > > >>
> > > >> I thought the base addresses and the lengths in the GCD memory space
> > map
> > > >> are all page aligned. Is that not the case?
> > > >>
> > > >> If these conditions are just a sanity check (i.e. we never expect them
> > > >> to fire), then should we perpahs turn them into ASSERT()s?
> > > >>
> > > >
> > > > I found that there's a mmio entry in memory map on OVMF which has size
> > > > less than a page. I didn't encounter this before. Maybe some recent
> changes
> > > > in other part of EDKII caused this situation. So ASSERT is not enough.
> > >
> > > Can you describe the base address and size of this MMIO entry?
> > >
> > > I don't see how it is possible to add such an entry in the first place
> > > -- if you try to add it in PEI, via a HOB, then IIRC HobLib will
> > > ASSERT(). If you try to add it with gDS->AddMemorySpace() in DXE, then
> > > the call should fail.
> > >
> > > I believe it might be useful to investigate this entry more closely.
> > > Knowing the base address could help us.
> > >
> > > Thanks!
> > > Laszlo
^ permalink raw reply [flat|nested] 17+ messages in thread
* Re: [PATCH v2] UefiCpuPkg/CpuDxe: Fix multiple entries of RT_CODE in memory map
2017-11-09 1:48 ` Yao, Jiewen
2017-11-09 1:51 ` Wang, Jian J
@ 2017-11-09 12:19 ` Laszlo Ersek
1 sibling, 0 replies; 17+ messages in thread
From: Laszlo Ersek @ 2017-11-09 12:19 UTC (permalink / raw)
To: Yao, Jiewen, Wang, Jian J, edk2-devel@lists.01.org; +Cc: Dong, Eric
On 11/09/17 02:48, Yao, Jiewen wrote:
> FED00000 is HPET region. It is MMIO.
Right, we add it in OvmfPkg/PlatformPei/Platform.c:
//
// address purpose size
// ------------ -------- -------------------------
...
// 0xFED00000 HPET 1 KB
...
AddIoMemoryBaseSizeHob (0xFED00000, SIZE_1KB);
Because this is MMIO and not system memory, I think the 1KB size is
valid, as far as a resource descriptor HOB is concerned.
This goes on to support my earlier suggestion to check the GCD memory
space entry more strictly, for its type.
Anyway, the latest v3 (v4?) approach can handle the entry just as well,
so I don't insist on modifying the entry type check. I was mainly
curious if we did something wrong in OVMF. I believe the above HOB is
valid though.
Thanks
Laszlo
>> -----Original Message-----
>> From: Wang, Jian J
>> Sent: Thursday, November 9, 2017 8:42 AM
>> To: Laszlo Ersek <lersek@redhat.com>; edk2-devel@lists.01.org
>> Cc: Yao, Jiewen <jiewen.yao@intel.com>; Dong, Eric <eric.dong@intel.com>
>> Subject: RE: [edk2] [PATCH v2] UefiCpuPkg/CpuDxe: Fix multiple entries of
>> RT_CODE in memory map
>>
>> Hi Laszlo,
>>
>> The memory range is 00000000FED00000 - 00000000FED003FF. Actually I don't
>> know if it's for MMIO or not. I might be mess it with other things.
>>
>> Thanks,
>> Jian
>>> -----Original Message-----
>>> From: Laszlo Ersek [mailto:lersek@redhat.com]
>>> Sent: Wednesday, November 08, 2017 10:17 PM
>>> To: Wang, Jian J <jian.j.wang@intel.com>; edk2-devel@lists.01.org
>>> Cc: Yao, Jiewen <jiewen.yao@intel.com>; Dong, Eric <eric.dong@intel.com>
>>> Subject: Re: [edk2] [PATCH v2] UefiCpuPkg/CpuDxe: Fix multiple entries of
>>> RT_CODE in memory map
>>>
>>> On 11/08/17 01:10, Wang, Jian J wrote:
>>>> Hi Laszlo,
>>>>
>>>>> -----Original Message-----
>>>>> From: Laszlo Ersek [mailto:lersek@redhat.com]
>>>>> Sent: Wednesday, November 08, 2017 1:14 AM
>>>>> To: Wang, Jian J <jian.j.wang@intel.com>; edk2-devel@lists.01.org
>>>>> Cc: Yao, Jiewen <jiewen.yao@intel.com>; Dong, Eric <eric.dong@intel.com>
>>>>> Subject: Re: [edk2] [PATCH v2] UefiCpuPkg/CpuDxe: Fix multiple entries of
>>>>> RT_CODE in memory map
>>>>>
>>>>> sorry about the late response
>>>>>
>>>>> On 11/03/17 01:57, Jian J Wang wrote:
>>>>>>> v2
>>>>>>> a. Fix an issue which will cause setting capability failure if size is smaller
>>>>>>> than a page.
>>>>>>
>>>>>> More than one entry of RT_CODE memory might cause boot problem for
>>>>> some
>>>>>> old OSs. This patch will fix this issue to keep OS compatibility as much
>>>>>> as possible.
>>>>>>
>>>>>> More detailed information, please refer to
>>>>>> https://bugzilla.tianocore.org/show_bug.cgi?id=753
>>>>>>
>>>>>> Cc: Eric Dong <eric.dong@intel.com>
>>>>>> Cc: Jiewen Yao <jiewen.yao@intel.com>
>>>>>> Cc: Laszlo Ersek <lersek@redhat.com>
>>>>>> Contributed-under: TianoCore Contribution Agreement 1.1
>>>>>> Signed-off-by: Jian J Wang <jian.j.wang@intel.com>
>>>>>> ---
>>>>>> UefiCpuPkg/CpuDxe/CpuPageTable.c | 18 ++++++++++++++----
>>>>>> 1 file changed, 14 insertions(+), 4 deletions(-)
>>>>>>
>>>>>> diff --git a/UefiCpuPkg/CpuDxe/CpuPageTable.c
>>>>> b/UefiCpuPkg/CpuDxe/CpuPageTable.c
>>>>>> index d312eb66f8..4a7827ebc9 100644
>>>>>> --- a/UefiCpuPkg/CpuDxe/CpuPageTable.c
>>>>>> +++ b/UefiCpuPkg/CpuDxe/CpuPageTable.c
>>>>>> @@ -809,7 +809,9 @@ RefreshGcdMemoryAttributesFromPaging (
>>>>>> PageLength = 0;
>>>>>>
>>>>>> for (Index = 0; Index < NumberOfDescriptors; Index++) {
>>>>>> - if (MemorySpaceMap[Index].GcdMemoryType ==
>>>>> EfiGcdMemoryTypeNonExistent) {
>>>>>> + if (MemorySpaceMap[Index].GcdMemoryType ==
>>>>> EfiGcdMemoryTypeNonExistent
>>>>>> + || (MemorySpaceMap[Index].BaseAddress &
>> EFI_PAGE_MASK) != 0
>>>>>> + || (MemorySpaceMap[Index].Length & EFI_PAGE_MASK) != 0)
>> {
>>>>>> continue;
>>>>>> }
>>>>>
>>>>> When exactly do the new conditions match?
>>>>>
>>>>> I thought the base addresses and the lengths in the GCD memory space
>> map
>>>>> are all page aligned. Is that not the case?
>>>>>
>>>>> If these conditions are just a sanity check (i.e. we never expect them
>>>>> to fire), then should we perpahs turn them into ASSERT()s?
>>>>>
>>>>
>>>> I found that there's a mmio entry in memory map on OVMF which has size
>>>> less than a page. I didn't encounter this before. Maybe some recent changes
>>>> in other part of EDKII caused this situation. So ASSERT is not enough.
>>>
>>> Can you describe the base address and size of this MMIO entry?
>>>
>>> I don't see how it is possible to add such an entry in the first place
>>> -- if you try to add it in PEI, via a HOB, then IIRC HobLib will
>>> ASSERT(). If you try to add it with gDS->AddMemorySpace() in DXE, then
>>> the call should fail.
>>>
>>> I believe it might be useful to investigate this entry more closely.
>>> Knowing the base address could help us.
>>>
>>> Thanks!
>>> Laszlo
^ permalink raw reply [flat|nested] 17+ messages in thread
* Re: [PATCH v2] UefiCpuPkg/CpuDxe: Fix multiple entries of RT_CODE in memory map
2017-11-03 0:57 Jian J Wang
2017-11-06 9:15 ` Zeng, Star
2017-11-07 17:13 ` Laszlo Ersek
@ 2017-11-08 4:41 ` Ni, Ruiyu
2017-11-08 4:46 ` Wang, Jian J
2 siblings, 1 reply; 17+ messages in thread
From: Ni, Ruiyu @ 2017-11-08 4:41 UTC (permalink / raw)
To: Wang, Jian J, edk2-devel@lists.01.org
Cc: Laszlo Ersek, Yao, Jiewen, Dong, Eric
Jian,
Can you add more comments to explain why changing the capabilities of GCD entry?
The background is clear by checking the Bugzilla. But it would be great to know the issue
by just reading the code.
Thanks/Ray
> -----Original Message-----
> From: edk2-devel [mailto:edk2-devel-bounces@lists.01.org] On Behalf Of
> Jian J Wang
> Sent: Friday, November 3, 2017 8:57 AM
> To: edk2-devel@lists.01.org
> Cc: Laszlo Ersek <lersek@redhat.com>; Yao, Jiewen <jiewen.yao@intel.com>;
> Dong, Eric <eric.dong@intel.com>
> Subject: [edk2] [PATCH v2] UefiCpuPkg/CpuDxe: Fix multiple entries of
> RT_CODE in memory map
>
> > v2
> > a. Fix an issue which will cause setting capability failure if size is smaller
> > than a page.
>
> More than one entry of RT_CODE memory might cause boot problem for
> some old OSs. This patch will fix this issue to keep OS compatibility as much as
> possible.
>
> More detailed information, please refer to
> https://bugzilla.tianocore.org/show_bug.cgi?id=753
>
> Cc: Eric Dong <eric.dong@intel.com>
> Cc: Jiewen Yao <jiewen.yao@intel.com>
> Cc: Laszlo Ersek <lersek@redhat.com>
> Contributed-under: TianoCore Contribution Agreement 1.1
> Signed-off-by: Jian J Wang <jian.j.wang@intel.com>
> ---
> UefiCpuPkg/CpuDxe/CpuPageTable.c | 18 ++++++++++++++----
> 1 file changed, 14 insertions(+), 4 deletions(-)
>
> diff --git a/UefiCpuPkg/CpuDxe/CpuPageTable.c
> b/UefiCpuPkg/CpuDxe/CpuPageTable.c
> index d312eb66f8..4a7827ebc9 100644
> --- a/UefiCpuPkg/CpuDxe/CpuPageTable.c
> +++ b/UefiCpuPkg/CpuDxe/CpuPageTable.c
> @@ -809,7 +809,9 @@ RefreshGcdMemoryAttributesFromPaging (
> PageLength = 0;
>
> for (Index = 0; Index < NumberOfDescriptors; Index++) {
> - if (MemorySpaceMap[Index].GcdMemoryType ==
> EfiGcdMemoryTypeNonExistent) {
> + if (MemorySpaceMap[Index].GcdMemoryType ==
> EfiGcdMemoryTypeNonExistent
> + || (MemorySpaceMap[Index].BaseAddress & EFI_PAGE_MASK) != 0
> + || (MemorySpaceMap[Index].Length & EFI_PAGE_MASK) != 0) {
> continue;
> }
>
> @@ -829,6 +831,15 @@ RefreshGcdMemoryAttributesFromPaging (
> // Sync real page attributes to GCD
> BaseAddress = MemorySpaceMap[Index].BaseAddress;
> MemorySpaceLength = MemorySpaceMap[Index].Length;
> + Capabilities = MemorySpaceMap[Index].Capabilities |
> + EFI_MEMORY_PAGETYPE_MASK;
> + Status = gDS->SetMemorySpaceCapabilities (
> + BaseAddress,
> + MemorySpaceLength,
> + Capabilities
> + );
> + ASSERT_EFI_ERROR (Status);
> +
> while (MemorySpaceLength > 0) {
> if (PageLength == 0) {
> PageEntry = GetPageTableEntry (&PagingContext, BaseAddress,
> &PageAttribute); @@ -846,7 +857,6 @@
> RefreshGcdMemoryAttributesFromPaging (
> if (Attributes != (MemorySpaceMap[Index].Attributes &
> EFI_MEMORY_PAGETYPE_MASK)) {
> DoUpdate = TRUE;
> Attributes |= (MemorySpaceMap[Index].Attributes &
> ~EFI_MEMORY_PAGETYPE_MASK);
> - Capabilities = Attributes | MemorySpaceMap[Index].Capabilities;
> } else {
> DoUpdate = FALSE;
> }
> @@ -854,8 +864,8 @@ RefreshGcdMemoryAttributesFromPaging (
>
> Length = MIN (PageLength, MemorySpaceLength);
> if (DoUpdate) {
> - gDS->SetMemorySpaceCapabilities (BaseAddress, Length, Capabilities);
> - gDS->SetMemorySpaceAttributes (BaseAddress, Length, Attributes);
> + Status = gDS->SetMemorySpaceAttributes (BaseAddress, Length,
> Attributes);
> + ASSERT_EFI_ERROR (Status);
> DEBUG ((DEBUG_INFO, "Update memory space attribute:
> [%02d] %016lx - %016lx (%08lx -> %08lx)\r\n",
> Index, BaseAddress, BaseAddress + Length - 1,
> MemorySpaceMap[Index].Attributes, Attributes));
> --
> 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] 17+ messages in thread
* Re: [PATCH v2] UefiCpuPkg/CpuDxe: Fix multiple entries of RT_CODE in memory map
2017-11-08 4:41 ` Ni, Ruiyu
@ 2017-11-08 4:46 ` Wang, Jian J
0 siblings, 0 replies; 17+ messages in thread
From: Wang, Jian J @ 2017-11-08 4:46 UTC (permalink / raw)
To: Ni, Ruiyu, edk2-devel@lists.01.org; +Cc: Laszlo Ersek, Yao, Jiewen, Dong, Eric
Make sense. Thanks for the comment.
> -----Original Message-----
> From: Ni, Ruiyu
> Sent: Wednesday, November 08, 2017 12:42 PM
> To: Wang, Jian J <jian.j.wang@intel.com>; edk2-devel@lists.01.org
> Cc: Laszlo Ersek <lersek@redhat.com>; Yao, Jiewen <jiewen.yao@intel.com>;
> Dong, Eric <eric.dong@intel.com>
> Subject: RE: [edk2] [PATCH v2] UefiCpuPkg/CpuDxe: Fix multiple entries of
> RT_CODE in memory map
>
> Jian,
> Can you add more comments to explain why changing the capabilities of GCD
> entry?
>
> The background is clear by checking the Bugzilla. But it would be great to know
> the issue
> by just reading the code.
>
> Thanks/Ray
>
> > -----Original Message-----
> > From: edk2-devel [mailto:edk2-devel-bounces@lists.01.org] On Behalf Of
> > Jian J Wang
> > Sent: Friday, November 3, 2017 8:57 AM
> > To: edk2-devel@lists.01.org
> > Cc: Laszlo Ersek <lersek@redhat.com>; Yao, Jiewen <jiewen.yao@intel.com>;
> > Dong, Eric <eric.dong@intel.com>
> > Subject: [edk2] [PATCH v2] UefiCpuPkg/CpuDxe: Fix multiple entries of
> > RT_CODE in memory map
> >
> > > v2
> > > a. Fix an issue which will cause setting capability failure if size is smaller
> > > than a page.
> >
> > More than one entry of RT_CODE memory might cause boot problem for
> > some old OSs. This patch will fix this issue to keep OS compatibility as much as
> > possible.
> >
> > More detailed information, please refer to
> > https://bugzilla.tianocore.org/show_bug.cgi?id=753
> >
> > Cc: Eric Dong <eric.dong@intel.com>
> > Cc: Jiewen Yao <jiewen.yao@intel.com>
> > Cc: Laszlo Ersek <lersek@redhat.com>
> > Contributed-under: TianoCore Contribution Agreement 1.1
> > Signed-off-by: Jian J Wang <jian.j.wang@intel.com>
> > ---
> > UefiCpuPkg/CpuDxe/CpuPageTable.c | 18 ++++++++++++++----
> > 1 file changed, 14 insertions(+), 4 deletions(-)
> >
> > diff --git a/UefiCpuPkg/CpuDxe/CpuPageTable.c
> > b/UefiCpuPkg/CpuDxe/CpuPageTable.c
> > index d312eb66f8..4a7827ebc9 100644
> > --- a/UefiCpuPkg/CpuDxe/CpuPageTable.c
> > +++ b/UefiCpuPkg/CpuDxe/CpuPageTable.c
> > @@ -809,7 +809,9 @@ RefreshGcdMemoryAttributesFromPaging (
> > PageLength = 0;
> >
> > for (Index = 0; Index < NumberOfDescriptors; Index++) {
> > - if (MemorySpaceMap[Index].GcdMemoryType ==
> > EfiGcdMemoryTypeNonExistent) {
> > + if (MemorySpaceMap[Index].GcdMemoryType ==
> > EfiGcdMemoryTypeNonExistent
> > + || (MemorySpaceMap[Index].BaseAddress & EFI_PAGE_MASK) != 0
> > + || (MemorySpaceMap[Index].Length & EFI_PAGE_MASK) != 0) {
> > continue;
> > }
> >
> > @@ -829,6 +831,15 @@ RefreshGcdMemoryAttributesFromPaging (
> > // Sync real page attributes to GCD
> > BaseAddress = MemorySpaceMap[Index].BaseAddress;
> > MemorySpaceLength = MemorySpaceMap[Index].Length;
> > + Capabilities = MemorySpaceMap[Index].Capabilities |
> > + EFI_MEMORY_PAGETYPE_MASK;
>
>
> > + Status = gDS->SetMemorySpaceCapabilities (
> > + BaseAddress,
> > + MemorySpaceLength,
> > + Capabilities
> > + );
> > + ASSERT_EFI_ERROR (Status);
> > +
> > while (MemorySpaceLength > 0) {
> > if (PageLength == 0) {
> > PageEntry = GetPageTableEntry (&PagingContext, BaseAddress,
> > &PageAttribute); @@ -846,7 +857,6 @@
> > RefreshGcdMemoryAttributesFromPaging (
> > if (Attributes != (MemorySpaceMap[Index].Attributes &
> > EFI_MEMORY_PAGETYPE_MASK)) {
> > DoUpdate = TRUE;
> > Attributes |= (MemorySpaceMap[Index].Attributes &
> > ~EFI_MEMORY_PAGETYPE_MASK);
> > - Capabilities = Attributes | MemorySpaceMap[Index].Capabilities;
> > } else {
> > DoUpdate = FALSE;
> > }
> > @@ -854,8 +864,8 @@ RefreshGcdMemoryAttributesFromPaging (
> >
> > Length = MIN (PageLength, MemorySpaceLength);
> > if (DoUpdate) {
> > - gDS->SetMemorySpaceCapabilities (BaseAddress, Length, Capabilities);
> > - gDS->SetMemorySpaceAttributes (BaseAddress, Length, Attributes);
> > + Status = gDS->SetMemorySpaceAttributes (BaseAddress, Length,
> > Attributes);
> > + ASSERT_EFI_ERROR (Status);
> > DEBUG ((DEBUG_INFO, "Update memory space attribute:
> > [%02d] %016lx - %016lx (%08lx -> %08lx)\r\n",
> > Index, BaseAddress, BaseAddress + Length - 1,
> > MemorySpaceMap[Index].Attributes, Attributes));
> > --
> > 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] 17+ messages in thread
end of thread, other threads:[~2017-11-09 12:15 UTC | newest]
Thread overview: 17+ messages (download: mbox.gz follow: Atom feed
-- links below jump to the message on this page --
2017-10-25 8:12 [PATCH v2] UefiCpuPkg/CpuDxe: Fix multiple entries of RT_CODE in memory map Jian J Wang
-- strict thread matches above, loose matches on Subject: below --
2017-11-03 0:57 Jian J Wang
2017-11-06 9:15 ` Zeng, Star
2017-11-07 0:55 ` Wang, Jian J
2017-11-07 1:12 ` Zeng, Star
2017-11-08 3:13 ` Zeng, Star
2017-11-08 13:25 ` Laszlo Ersek
2017-11-07 17:13 ` Laszlo Ersek
2017-11-08 0:10 ` Wang, Jian J
2017-11-08 9:10 ` Wang, Jian J
2017-11-08 14:17 ` Laszlo Ersek
2017-11-09 0:41 ` Wang, Jian J
2017-11-09 1:48 ` Yao, Jiewen
2017-11-09 1:51 ` Wang, Jian J
2017-11-09 12:19 ` Laszlo Ersek
2017-11-08 4:41 ` Ni, Ruiyu
2017-11-08 4:46 ` 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