UefiCpuPkg/CpuDxe/CpuPageTable.c | 18 ++++++++++++++---- 1 file changed, 14 insertions(+), 4 deletions(-)
> 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
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
_______________________________________________
edk2-devel mailing list
edk2-devel@lists.01.org
https://lists.01.org/mailman/listinfo/edk2-devel
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
_______________________________________________
edk2-devel mailing list
edk2-devel@lists.01.org
https://lists.01.org/mailman/listinfo/edk2-devel
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
_______________________________________________
edk2-devel mailing list
edk2-devel@lists.01.org
https://lists.01.org/mailman/listinfo/edk2-devel
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
_______________________________________________
edk2-devel mailing list
edk2-devel@lists.01.org
https://lists.01.org/mailman/listinfo/edk2-devel
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
_______________________________________________
edk2-devel mailing list
edk2-devel@lists.01.org
https://lists.01.org/mailman/listinfo/edk2-devel
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
_______________________________________________
edk2-devel mailing list
edk2-devel@lists.01.org
https://lists.01.org/mailman/listinfo/edk2-devel
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
_______________________________________________
edk2-devel mailing list
edk2-devel@lists.01.org
https://lists.01.org/mailman/listinfo/edk2-devel
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
_______________________________________________
edk2-devel mailing list
edk2-devel@lists.01.org
https://lists.01.org/mailman/listinfo/edk2-devel
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
_______________________________________________
edk2-devel mailing list
edk2-devel@lists.01.org
https://lists.01.org/mailman/listinfo/edk2-devel
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
_______________________________________________
edk2-devel mailing list
edk2-devel@lists.01.org
https://lists.01.org/mailman/listinfo/edk2-devel
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
_______________________________________________
edk2-devel mailing list
edk2-devel@lists.01.org
https://lists.01.org/mailman/listinfo/edk2-devel
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
_______________________________________________
edk2-devel mailing list
edk2-devel@lists.01.org
https://lists.01.org/mailman/listinfo/edk2-devel
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
_______________________________________________
edk2-devel mailing list
edk2-devel@lists.01.org
https://lists.01.org/mailman/listinfo/edk2-devel
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
_______________________________________________
edk2-devel mailing list
edk2-devel@lists.01.org
https://lists.01.org/mailman/listinfo/edk2-devel
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
_______________________________________________
edk2-devel mailing list
edk2-devel@lists.01.org
https://lists.01.org/mailman/listinfo/edk2-devel
© 2016 - 2024 Red Hat, Inc.