* [PATCH v3] UefiCpuPkg/CpuDxe: Fix multiple entries of RT_CODE in memory map
@ 2017-11-08 10:52 Jian J Wang
2017-11-08 14:36 ` Laszlo Ersek
0 siblings, 1 reply; 6+ messages in thread
From: Jian J Wang @ 2017-11-08 10:52 UTC (permalink / raw)
To: edk2-devel; +Cc: Eric Dong, Jiewen Yao, Laszlo Ersek
> v3:
> a. Add comment to explain more on updating memory capabilities
> b. Fix logic hole in updating attributes
> c. Instead of checking illegal memory space address and size, use return
> status of gDS->SetMemorySpaceCapabilities() to skip memory block which
> cannot be updated with new capabilities.
> 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 | 48 +++++++++++++++++++++++++++++++---------
1 file changed, 38 insertions(+), 10 deletions(-)
diff --git a/UefiCpuPkg/CpuDxe/CpuPageTable.c b/UefiCpuPkg/CpuDxe/CpuPageTable.c
index d312eb66f8..455c713dfc 100644
--- a/UefiCpuPkg/CpuDxe/CpuPageTable.c
+++ b/UefiCpuPkg/CpuDxe/CpuPageTable.c
@@ -789,7 +789,6 @@ RefreshGcdMemoryAttributesFromPaging (
UINT64 BaseAddress;
UINT64 PageStartAddress;
UINT64 Attributes;
- UINT64 Capabilities;
BOOLEAN DoUpdate;
UINTN Index;
@@ -803,7 +802,6 @@ RefreshGcdMemoryAttributesFromPaging (
GetCurrentPagingContext (&PagingContext);
DoUpdate = FALSE;
- Capabilities = 0;
Attributes = 0;
BaseAddress = 0;
PageLength = 0;
@@ -813,6 +811,27 @@ RefreshGcdMemoryAttributesFromPaging (
continue;
}
+ //
+ // Sync the actual paging related capabilities back to GCD service first.
+ // As a side effect (good one), this can also help to avoid unnecessary
+ // memory map entries due to the different capabilities of the same type
+ // memory, such as multiple RT_CODE and RT_DATA entries in memory map,
+ // which could cause boot failure of some old Linux distro (before v4.3).
+ //
+ Status = gDS->SetMemorySpaceCapabilities (
+ MemorySpaceMap[Index].BaseAddress,
+ MemorySpaceMap[Index].Length,
+ MemorySpaceMap[Index].Capabilities |
+ EFI_MEMORY_PAGETYPE_MASK
+ );
+ if (EFI_ERROR (Status)) {
+ //
+ // If we cannot udpate the capabilities, we cannot update its
+ // attributes either. So just simply skip current block of memory.
+ //
+ continue;
+ }
+
if (MemorySpaceMap[Index].BaseAddress >= (BaseAddress + PageLength)) {
//
// Current memory space starts at a new page. Resetting PageLength will
@@ -826,7 +845,9 @@ RefreshGcdMemoryAttributesFromPaging (
PageLength -= (MemorySpaceMap[Index].BaseAddress - BaseAddress);
}
- // Sync real page attributes to GCD
+ //
+ // Sync actual page attributes to GCD
+ //
BaseAddress = MemorySpaceMap[Index].BaseAddress;
MemorySpaceLength = MemorySpaceMap[Index].Length;
while (MemorySpaceLength > 0) {
@@ -845,8 +866,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,11 +873,20 @@ RefreshGcdMemoryAttributesFromPaging (
Length = MIN (PageLength, MemorySpaceLength);
if (DoUpdate) {
- gDS->SetMemorySpaceCapabilities (BaseAddress, Length, Capabilities);
- gDS->SetMemorySpaceAttributes (BaseAddress, Length, Attributes);
- DEBUG ((DEBUG_INFO, "Update memory space attribute: [%02d] %016lx - %016lx (%08lx -> %08lx)\r\n",
- Index, BaseAddress, BaseAddress + Length - 1,
- MemorySpaceMap[Index].Attributes, Attributes));
+ Status = gDS->SetMemorySpaceAttributes (
+ BaseAddress,
+ Length,
+ (MemorySpaceMap[Index].Attributes
+ & ~EFI_MEMORY_PAGETYPE_MASK) | Attributes
+ );
+ ASSERT_EFI_ERROR (Status);
+ DEBUG ((
+ DEBUG_INFO,
+ "Update memory space attribute: [%02d] %016lx - %016lx (%016lx -> %016lx)\r\n",
+ Index, BaseAddress, BaseAddress + Length - 1,
+ MemorySpaceMap[Index].Attributes,
+ (MemorySpaceMap[Index].Attributes & ~EFI_MEMORY_PAGETYPE_MASK) | Attributes
+ ));
}
PageLength -= Length;
--
2.14.1.windows.1
^ permalink raw reply related [flat|nested] 6+ messages in thread
* Re: [PATCH v3] UefiCpuPkg/CpuDxe: Fix multiple entries of RT_CODE in memory map
2017-11-08 10:52 Jian J Wang
@ 2017-11-08 14:36 ` Laszlo Ersek
2017-11-09 0:51 ` Wang, Jian J
0 siblings, 1 reply; 6+ messages in thread
From: Laszlo Ersek @ 2017-11-08 14:36 UTC (permalink / raw)
To: Jian J Wang, edk2-devel; +Cc: Eric Dong, Jiewen Yao
On 11/08/17 11:52, Jian J Wang wrote:
>> v3:
>> a. Add comment to explain more on updating memory capabilities
>> b. Fix logic hole in updating attributes
>> c. Instead of checking illegal memory space address and size, use return
>> status of gDS->SetMemorySpaceCapabilities() to skip memory block which
>> cannot be updated with new capabilities.
>
>> 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 | 48 +++++++++++++++++++++++++++++++---------
> 1 file changed, 38 insertions(+), 10 deletions(-)
>
> diff --git a/UefiCpuPkg/CpuDxe/CpuPageTable.c b/UefiCpuPkg/CpuDxe/CpuPageTable.c
> index d312eb66f8..455c713dfc 100644
> --- a/UefiCpuPkg/CpuDxe/CpuPageTable.c
> +++ b/UefiCpuPkg/CpuDxe/CpuPageTable.c
> @@ -789,7 +789,6 @@ RefreshGcdMemoryAttributesFromPaging (
> UINT64 BaseAddress;
> UINT64 PageStartAddress;
> UINT64 Attributes;
> - UINT64 Capabilities;
> BOOLEAN DoUpdate;
> UINTN Index;
>
> @@ -803,7 +802,6 @@ RefreshGcdMemoryAttributesFromPaging (
> GetCurrentPagingContext (&PagingContext);
>
> DoUpdate = FALSE;
> - Capabilities = 0;
> Attributes = 0;
> BaseAddress = 0;
> PageLength = 0;
> @@ -813,6 +811,27 @@ RefreshGcdMemoryAttributesFromPaging (
> continue;
> }
>
> + //
> + // Sync the actual paging related capabilities back to GCD service first.
> + // As a side effect (good one), this can also help to avoid unnecessary
> + // memory map entries due to the different capabilities of the same type
> + // memory, such as multiple RT_CODE and RT_DATA entries in memory map,
> + // which could cause boot failure of some old Linux distro (before v4.3).
> + //
> + Status = gDS->SetMemorySpaceCapabilities (
> + MemorySpaceMap[Index].BaseAddress,
> + MemorySpaceMap[Index].Length,
> + MemorySpaceMap[Index].Capabilities |
> + EFI_MEMORY_PAGETYPE_MASK
> + );
> + if (EFI_ERROR (Status)) {
> + //
> + // If we cannot udpate the capabilities, we cannot update its
> + // attributes either. So just simply skip current block of memory.
> + //
(1) Can you perhaps add a DEBUG_WARN here?
> + continue;
> + }
> +
> if (MemorySpaceMap[Index].BaseAddress >= (BaseAddress + PageLength)) {
> //
> // Current memory space starts at a new page. Resetting PageLength will
> @@ -826,7 +845,9 @@ RefreshGcdMemoryAttributesFromPaging (
> PageLength -= (MemorySpaceMap[Index].BaseAddress - BaseAddress);
> }
>
> - // Sync real page attributes to GCD
> + //
> + // Sync actual page attributes to GCD
> + //
> BaseAddress = MemorySpaceMap[Index].BaseAddress;
> MemorySpaceLength = MemorySpaceMap[Index].Length;
> while (MemorySpaceLength > 0) {
> @@ -845,8 +866,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;
> }
(2) To me it seems like we can remove the "DoUpdate" local variable
completely. Below, we can replace the DoUpdate check with the actual
(Attributes != (MemorySpaceMap[Index].Attributes &
EFI_MEMORY_PAGETYPE_MASK))
check.
The point is that we check the EFI_MEMORY_PAGETYPE_MASK bit-field of the
entry's attributes. If they do not match Attributes, we clear the full
bit-field, and then add Attributes back in. I.e., we set the bit-field
to the desired Attributes.
> @@ -854,11 +873,20 @@ RefreshGcdMemoryAttributesFromPaging (
>
> Length = MIN (PageLength, MemorySpaceLength);
> if (DoUpdate) {
> - gDS->SetMemorySpaceCapabilities (BaseAddress, Length, Capabilities);
> - gDS->SetMemorySpaceAttributes (BaseAddress, Length, Attributes);
> - DEBUG ((DEBUG_INFO, "Update memory space attribute: [%02d] %016lx - %016lx (%08lx -> %08lx)\r\n",
> - Index, BaseAddress, BaseAddress + Length - 1,
> - MemorySpaceMap[Index].Attributes, Attributes));
> + Status = gDS->SetMemorySpaceAttributes (
> + BaseAddress,
> + Length,
> + (MemorySpaceMap[Index].Attributes
> + & ~EFI_MEMORY_PAGETYPE_MASK) | Attributes
> + );
> + ASSERT_EFI_ERROR (Status);
> + DEBUG ((
> + DEBUG_INFO,
> + "Update memory space attribute: [%02d] %016lx - %016lx (%016lx -> %016lx)\r\n",
> + Index, BaseAddress, BaseAddress + Length - 1,
> + MemorySpaceMap[Index].Attributes,
> + (MemorySpaceMap[Index].Attributes & ~EFI_MEMORY_PAGETYPE_MASK) | Attributes
> + ));
> }
(3) I suggest introducing a new variable called
"NewMemorySpaceAttributes", and using that for both the debug message
and the SetMemorySpaceAttributes() call.
(4) Not closely related to this patch, but I'll mention it: the "%d"
format specifier is not right for printing UINTN values. The
32-bit/64-bit clean way to print UINTN is:
- cast the variable to UINT64 explicitly,
- print it with "%lu".
Thanks!
Laszlo
>
> PageLength -= Length;
>
^ permalink raw reply [flat|nested] 6+ messages in thread
* Re: [PATCH v3] UefiCpuPkg/CpuDxe: Fix multiple entries of RT_CODE in memory map
2017-11-08 14:36 ` Laszlo Ersek
@ 2017-11-09 0:51 ` Wang, Jian J
0 siblings, 0 replies; 6+ messages in thread
From: Wang, Jian J @ 2017-11-09 0:51 UTC (permalink / raw)
To: Laszlo Ersek, edk2-devel@lists.01.org; +Cc: Dong, Eric, Yao, Jiewen
Hi Laszlo,
> -----Original Message-----
> From: Laszlo Ersek [mailto:lersek@redhat.com]
> Sent: Wednesday, November 08, 2017 10:37 PM
> To: Wang, Jian J <jian.j.wang@intel.com>; edk2-devel@lists.01.org
> Cc: Dong, Eric <eric.dong@intel.com>; Yao, Jiewen <jiewen.yao@intel.com>
> Subject: Re: [PATCH v3] UefiCpuPkg/CpuDxe: Fix multiple entries of RT_CODE in
> memory map
>
> On 11/08/17 11:52, Jian J Wang wrote:
> >> v3:
> >> a. Add comment to explain more on updating memory capabilities
> >> b. Fix logic hole in updating attributes
> >> c. Instead of checking illegal memory space address and size, use return
> >> status of gDS->SetMemorySpaceCapabilities() to skip memory block which
> >> cannot be updated with new capabilities.
> >
> >> 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 | 48
> +++++++++++++++++++++++++++++++---------
> > 1 file changed, 38 insertions(+), 10 deletions(-)
> >
> > diff --git a/UefiCpuPkg/CpuDxe/CpuPageTable.c
> b/UefiCpuPkg/CpuDxe/CpuPageTable.c
> > index d312eb66f8..455c713dfc 100644
> > --- a/UefiCpuPkg/CpuDxe/CpuPageTable.c
> > +++ b/UefiCpuPkg/CpuDxe/CpuPageTable.c
> > @@ -789,7 +789,6 @@ RefreshGcdMemoryAttributesFromPaging (
> > UINT64 BaseAddress;
> > UINT64 PageStartAddress;
> > UINT64 Attributes;
> > - UINT64 Capabilities;
> > BOOLEAN DoUpdate;
> > UINTN Index;
> >
> > @@ -803,7 +802,6 @@ RefreshGcdMemoryAttributesFromPaging (
> > GetCurrentPagingContext (&PagingContext);
> >
> > DoUpdate = FALSE;
> > - Capabilities = 0;
> > Attributes = 0;
> > BaseAddress = 0;
> > PageLength = 0;
> > @@ -813,6 +811,27 @@ RefreshGcdMemoryAttributesFromPaging (
> > continue;
> > }
> >
> > + //
> > + // Sync the actual paging related capabilities back to GCD service first.
> > + // As a side effect (good one), this can also help to avoid unnecessary
> > + // memory map entries due to the different capabilities of the same type
> > + // memory, such as multiple RT_CODE and RT_DATA entries in memory
> map,
> > + // which could cause boot failure of some old Linux distro (before v4.3).
> > + //
> > + Status = gDS->SetMemorySpaceCapabilities (
> > + MemorySpaceMap[Index].BaseAddress,
> > + MemorySpaceMap[Index].Length,
> > + MemorySpaceMap[Index].Capabilities |
> > + EFI_MEMORY_PAGETYPE_MASK
> > + );
> > + if (EFI_ERROR (Status)) {
> > + //
> > + // If we cannot udpate the capabilities, we cannot update its
> > + // attributes either. So just simply skip current block of memory.
> > + //
>
> (1) Can you perhaps add a DEBUG_WARN here?
Sure.
>
> > + continue;
> > + }
> > +
> > if (MemorySpaceMap[Index].BaseAddress >= (BaseAddress + PageLength)) {
> > //
> > // Current memory space starts at a new page. Resetting PageLength will
> > @@ -826,7 +845,9 @@ RefreshGcdMemoryAttributesFromPaging (
> > PageLength -= (MemorySpaceMap[Index].BaseAddress - BaseAddress);
> > }
> >
> > - // Sync real page attributes to GCD
> > + //
> > + // Sync actual page attributes to GCD
> > + //
> > BaseAddress = MemorySpaceMap[Index].BaseAddress;
> > MemorySpaceLength = MemorySpaceMap[Index].Length;
> > while (MemorySpaceLength > 0) {
> > @@ -845,8 +866,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;
> > }
>
> (2) To me it seems like we can remove the "DoUpdate" local variable
> completely. Below, we can replace the DoUpdate check with the actual
>
> (Attributes != (MemorySpaceMap[Index].Attributes &
> EFI_MEMORY_PAGETYPE_MASK))
>
> check.
>
> The point is that we check the EFI_MEMORY_PAGETYPE_MASK bit-field of the
> entry's attributes. If they do not match Attributes, we clear the full
> bit-field, and then add Attributes back in. I.e., we set the bit-field
> to the desired Attributes.
>
You're right. Checking the attribute mismatch inside the "if (PageLength == 0)"
still leaves a logic hole there. You suggestion can fix it. Thanks for this catch.
> > @@ -854,11 +873,20 @@ RefreshGcdMemoryAttributesFromPaging (
> >
> > Length = MIN (PageLength, MemorySpaceLength);
> > if (DoUpdate) {
> > - gDS->SetMemorySpaceCapabilities (BaseAddress, Length, Capabilities);
> > - gDS->SetMemorySpaceAttributes (BaseAddress, Length, Attributes);
> > - DEBUG ((DEBUG_INFO, "Update memory space attribute: [%02d] %016lx
> - %016lx (%08lx -> %08lx)\r\n",
> > - Index, BaseAddress, BaseAddress + Length - 1,
> > - MemorySpaceMap[Index].Attributes, Attributes));
> > + Status = gDS->SetMemorySpaceAttributes (
> > + BaseAddress,
> > + Length,
> > + (MemorySpaceMap[Index].Attributes
> > + & ~EFI_MEMORY_PAGETYPE_MASK) | Attributes
> > + );
> > + ASSERT_EFI_ERROR (Status);
> > + DEBUG ((
> > + DEBUG_INFO,
> > + "Update memory space attribute: [%02d] %016lx - %016lx (%016lx -
> > %016lx)\r\n",
> > + Index, BaseAddress, BaseAddress + Length - 1,
> > + MemorySpaceMap[Index].Attributes,
> > + (MemorySpaceMap[Index].Attributes &
> ~EFI_MEMORY_PAGETYPE_MASK) | Attributes
> > + ));
> > }
>
> (3) I suggest introducing a new variable called
> "NewMemorySpaceAttributes", and using that for both the debug message
> and the SetMemorySpaceAttributes() call.
>
I agree.
> (4) Not closely related to this patch, but I'll mention it: the "%d"
> format specifier is not right for printing UINTN values. The
> 32-bit/64-bit clean way to print UINTN is:
>
> - cast the variable to UINT64 explicitly,
> - print it with "%lu".
>
Thanks for pointing this out.
> Thanks!
> Laszlo
>
> >
> > PageLength -= Length;
> >
^ permalink raw reply [flat|nested] 6+ messages in thread
* [PATCH v3] UefiCpuPkg/CpuDxe: Fix multiple entries of RT_CODE in memory map
@ 2017-11-09 1:39 Jian J Wang
2017-11-09 14:12 ` Laszlo Ersek
0 siblings, 1 reply; 6+ messages in thread
From: Jian J Wang @ 2017-11-09 1:39 UTC (permalink / raw)
To: edk2-devel; +Cc: Eric Dong, Jiewen Yao, Laszlo Ersek
> v4:
> a. Remove DoUpdate and check attributes mismatch all the time to avoid
> a logic hole
> b. Add warning message if failed to update capability
> c. Add local variable to hold new attributes to make code cleaner
> v3:
> a. Add comment to explain more on updating memory capabilities
> b. Fix logic hole in updating attributes
> c. Instead of checking illegal memory space address and size, use return
> status of gDS->SetMemorySpaceCapabilities() to skip memory block which
> cannot be updated with new capabilities.
> 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 | 69 +++++++++++++++++++++++++++++-----------
1 file changed, 50 insertions(+), 19 deletions(-)
diff --git a/UefiCpuPkg/CpuDxe/CpuPageTable.c b/UefiCpuPkg/CpuDxe/CpuPageTable.c
index d312eb66f8..a1d804caed 100644
--- a/UefiCpuPkg/CpuDxe/CpuPageTable.c
+++ b/UefiCpuPkg/CpuDxe/CpuPageTable.c
@@ -789,8 +789,7 @@ RefreshGcdMemoryAttributesFromPaging (
UINT64 BaseAddress;
UINT64 PageStartAddress;
UINT64 Attributes;
- UINT64 Capabilities;
- BOOLEAN DoUpdate;
+ UINT64 NewAttributes;
UINTN Index;
//
@@ -802,9 +801,8 @@ RefreshGcdMemoryAttributesFromPaging (
GetCurrentPagingContext (&PagingContext);
- DoUpdate = FALSE;
- Capabilities = 0;
Attributes = 0;
+ NewAttributes = 0;
BaseAddress = 0;
PageLength = 0;
@@ -813,6 +811,34 @@ RefreshGcdMemoryAttributesFromPaging (
continue;
}
+ //
+ // Sync the actual paging related capabilities back to GCD service first.
+ // As a side effect (good one), this can also help to avoid unnecessary
+ // memory map entries due to the different capabilities of the same type
+ // memory, such as multiple RT_CODE and RT_DATA entries in memory map,
+ // which could cause boot failure of some old Linux distro (before v4.3).
+ //
+ Status = gDS->SetMemorySpaceCapabilities (
+ MemorySpaceMap[Index].BaseAddress,
+ MemorySpaceMap[Index].Length,
+ MemorySpaceMap[Index].Capabilities |
+ EFI_MEMORY_PAGETYPE_MASK
+ );
+ if (EFI_ERROR (Status)) {
+ //
+ // If we cannot udpate the capabilities, we cannot update its
+ // attributes either. So just simply skip current block of memory.
+ //
+ DEBUG ((
+ DEBUG_WARN,
+ "Failed to update capability: [%d] %016lx - %016lx (%016lx -> %016lx)\r\n",
+ Index, BaseAddress, BaseAddress + Length - 1,
+ MemorySpaceMap[Index].Capabilities,
+ MemorySpaceMap[Index].Capabilities | EFI_MEMORY_PAGETYPE_MASK
+ ));
+ continue;
+ }
+
if (MemorySpaceMap[Index].BaseAddress >= (BaseAddress + PageLength)) {
//
// Current memory space starts at a new page. Resetting PageLength will
@@ -826,7 +852,9 @@ RefreshGcdMemoryAttributesFromPaging (
PageLength -= (MemorySpaceMap[Index].BaseAddress - BaseAddress);
}
- // Sync real page attributes to GCD
+ //
+ // Sync actual page attributes to GCD
+ //
BaseAddress = MemorySpaceMap[Index].BaseAddress;
MemorySpaceLength = MemorySpaceMap[Index].Length;
while (MemorySpaceLength > 0) {
@@ -842,23 +870,26 @@ RefreshGcdMemoryAttributesFromPaging (
PageStartAddress = (*PageEntry) & (UINT64)PageAttributeToMask(PageAttribute);
PageLength = PageAttributeToLength (PageAttribute) - (BaseAddress - PageStartAddress);
Attributes = GetAttributesFromPageEntry (PageEntry);
-
- 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;
- }
}
Length = MIN (PageLength, MemorySpaceLength);
- if (DoUpdate) {
- gDS->SetMemorySpaceCapabilities (BaseAddress, Length, Capabilities);
- gDS->SetMemorySpaceAttributes (BaseAddress, Length, Attributes);
- DEBUG ((DEBUG_INFO, "Update memory space attribute: [%02d] %016lx - %016lx (%08lx -> %08lx)\r\n",
- Index, BaseAddress, BaseAddress + Length - 1,
- MemorySpaceMap[Index].Attributes, Attributes));
+ if (Attributes != (MemorySpaceMap[Index].Attributes
+ & EFI_MEMORY_PAGETYPE_MASK)) {
+ NewAttributes = (MemorySpaceMap[Index].Attributes
+ & ~EFI_MEMORY_PAGETYPE_MASK) | Attributes;
+ Status = gDS->SetMemorySpaceAttributes (
+ BaseAddress,
+ Length,
+ NewAttributes
+ );
+ ASSERT_EFI_ERROR (Status);
+ DEBUG ((
+ DEBUG_INFO,
+ "Updated memory space attribute: [%d] %016lx - %016lx (%016lx -> %016lx)\r\n",
+ Index, BaseAddress, BaseAddress + Length - 1,
+ MemorySpaceMap[Index].Attributes,
+ NewAttributes
+ ));
}
PageLength -= Length;
--
2.14.1.windows.1
^ permalink raw reply related [flat|nested] 6+ messages in thread
* Re: [PATCH v3] UefiCpuPkg/CpuDxe: Fix multiple entries of RT_CODE in memory map
2017-11-09 1:39 [PATCH v3] UefiCpuPkg/CpuDxe: Fix multiple entries of RT_CODE in memory map Jian J Wang
@ 2017-11-09 14:12 ` Laszlo Ersek
2017-11-10 0:22 ` Wang, Jian J
0 siblings, 1 reply; 6+ messages in thread
From: Laszlo Ersek @ 2017-11-09 14:12 UTC (permalink / raw)
To: Jian J Wang, edk2-devel; +Cc: Jiewen Yao, Eric Dong
Hi Jian,
this is v4, but the subject says v3 :) If you post a new version, please
make sure that it says "v5" in the subject.
The logic looks OK to me; I've got some comments on style:
On 11/09/17 02:39, Jian J Wang wrote:
>> v4:
>> a. Remove DoUpdate and check attributes mismatch all the time to avoid
>> a logic hole
>> b. Add warning message if failed to update capability
>> c. Add local variable to hold new attributes to make code cleaner
>
>> v3:
>> a. Add comment to explain more on updating memory capabilities
>> b. Fix logic hole in updating attributes
>> c. Instead of checking illegal memory space address and size, use return
>> status of gDS->SetMemorySpaceCapabilities() to skip memory block which
>> cannot be updated with new capabilities.
>
>> 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 | 69 +++++++++++++++++++++++++++++-----------
> 1 file changed, 50 insertions(+), 19 deletions(-)
>
> diff --git a/UefiCpuPkg/CpuDxe/CpuPageTable.c b/UefiCpuPkg/CpuDxe/CpuPageTable.c
> index d312eb66f8..a1d804caed 100644
> --- a/UefiCpuPkg/CpuDxe/CpuPageTable.c
> +++ b/UefiCpuPkg/CpuDxe/CpuPageTable.c
> @@ -789,8 +789,7 @@ RefreshGcdMemoryAttributesFromPaging (
> UINT64 BaseAddress;
> UINT64 PageStartAddress;
> UINT64 Attributes;
> - UINT64 Capabilities;
> - BOOLEAN DoUpdate;
> + UINT64 NewAttributes;
> UINTN Index;
>
> //
> @@ -802,9 +801,8 @@ RefreshGcdMemoryAttributesFromPaging (
>
> GetCurrentPagingContext (&PagingContext);
>
> - DoUpdate = FALSE;
> - Capabilities = 0;
> Attributes = 0;
> + NewAttributes = 0;
> BaseAddress = 0;
> PageLength = 0;
>
> @@ -813,6 +811,34 @@ RefreshGcdMemoryAttributesFromPaging (
> continue;
> }
>
> + //
> + // Sync the actual paging related capabilities back to GCD service first.
> + // As a side effect (good one), this can also help to avoid unnecessary
> + // memory map entries due to the different capabilities of the same type
> + // memory, such as multiple RT_CODE and RT_DATA entries in memory map,
> + // which could cause boot failure of some old Linux distro (before v4.3).
> + //
> + Status = gDS->SetMemorySpaceCapabilities (
> + MemorySpaceMap[Index].BaseAddress,
> + MemorySpaceMap[Index].Length,
> + MemorySpaceMap[Index].Capabilities |
> + EFI_MEMORY_PAGETYPE_MASK
> + );
> + if (EFI_ERROR (Status)) {
> + //
> + // If we cannot udpate the capabilities, we cannot update its
> + // attributes either. So just simply skip current block of memory.
> + //
> + DEBUG ((
> + DEBUG_WARN,
> + "Failed to update capability: [%d] %016lx - %016lx (%016lx -> %016lx)\r\n",
> + Index, BaseAddress, BaseAddress + Length - 1,
(1) I think you forgot about my note that Index (which is of type UINTN)
should not be printed with "%d". Instead, (UINT64)Index should be
printed with "%lu".
> + MemorySpaceMap[Index].Capabilities,
> + MemorySpaceMap[Index].Capabilities | EFI_MEMORY_PAGETYPE_MASK
> + ));
> + continue;
> + }
> +
> if (MemorySpaceMap[Index].BaseAddress >= (BaseAddress + PageLength)) {
> //
> // Current memory space starts at a new page. Resetting PageLength will
> @@ -826,7 +852,9 @@ RefreshGcdMemoryAttributesFromPaging (
> PageLength -= (MemorySpaceMap[Index].BaseAddress - BaseAddress);
> }
>
> - // Sync real page attributes to GCD
> + //
> + // Sync actual page attributes to GCD
> + //
> BaseAddress = MemorySpaceMap[Index].BaseAddress;
> MemorySpaceLength = MemorySpaceMap[Index].Length;
> while (MemorySpaceLength > 0) {
> @@ -842,23 +870,26 @@ RefreshGcdMemoryAttributesFromPaging (
> PageStartAddress = (*PageEntry) & (UINT64)PageAttributeToMask(PageAttribute);
> PageLength = PageAttributeToLength (PageAttribute) - (BaseAddress - PageStartAddress);
> Attributes = GetAttributesFromPageEntry (PageEntry);
> -
> - 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;
> - }
> }
>
> Length = MIN (PageLength, MemorySpaceLength);
> - if (DoUpdate) {
> - gDS->SetMemorySpaceCapabilities (BaseAddress, Length, Capabilities);
> - gDS->SetMemorySpaceAttributes (BaseAddress, Length, Attributes);
> - DEBUG ((DEBUG_INFO, "Update memory space attribute: [%02d] %016lx - %016lx (%08lx -> %08lx)\r\n",
> - Index, BaseAddress, BaseAddress + Length - 1,
> - MemorySpaceMap[Index].Attributes, Attributes));
> + if (Attributes != (MemorySpaceMap[Index].Attributes
> + & EFI_MEMORY_PAGETYPE_MASK)) {
> + NewAttributes = (MemorySpaceMap[Index].Attributes
> + & ~EFI_MEMORY_PAGETYPE_MASK) | Attributes;
(2) To my understanding, the edk2 coding style wants us to keep the
bitwise-and operator ("&") at the end of the line.
> + Status = gDS->SetMemorySpaceAttributes (
> + BaseAddress,
> + Length,
> + NewAttributes
> + );
> + ASSERT_EFI_ERROR (Status);
> + DEBUG ((
> + DEBUG_INFO,
> + "Updated memory space attribute: [%d] %016lx - %016lx (%016lx -> %016lx)\r\n",
> + Index, BaseAddress, BaseAddress + Length - 1,
(3) Same comment as (1).
Thanks,
Laszlo
> + MemorySpaceMap[Index].Attributes,
> + NewAttributes
> + ));
> }
>
> PageLength -= Length;
>
^ permalink raw reply [flat|nested] 6+ messages in thread
* Re: [PATCH v3] UefiCpuPkg/CpuDxe: Fix multiple entries of RT_CODE in memory map
2017-11-09 14:12 ` Laszlo Ersek
@ 2017-11-10 0:22 ` Wang, Jian J
0 siblings, 0 replies; 6+ messages in thread
From: Wang, Jian J @ 2017-11-10 0:22 UTC (permalink / raw)
To: Laszlo Ersek, edk2-devel@lists.01.org; +Cc: Yao, Jiewen, Dong, Eric
Thanks for catching them. There'll be v5 today:)
> -----Original Message-----
> From: Laszlo Ersek [mailto:lersek@redhat.com]
> Sent: Thursday, November 09, 2017 10:13 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 v3] UefiCpuPkg/CpuDxe: Fix multiple entries of
> RT_CODE in memory map
>
> Hi Jian,
>
> this is v4, but the subject says v3 :) If you post a new version, please
> make sure that it says "v5" in the subject.
>
> The logic looks OK to me; I've got some comments on style:
>
> On 11/09/17 02:39, Jian J Wang wrote:
> >> v4:
> >> a. Remove DoUpdate and check attributes mismatch all the time to avoid
> >> a logic hole
> >> b. Add warning message if failed to update capability
> >> c. Add local variable to hold new attributes to make code cleaner
> >
> >> v3:
> >> a. Add comment to explain more on updating memory capabilities
> >> b. Fix logic hole in updating attributes
> >> c. Instead of checking illegal memory space address and size, use return
> >> status of gDS->SetMemorySpaceCapabilities() to skip memory block which
> >> cannot be updated with new capabilities.
> >
> >> 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 | 69
> +++++++++++++++++++++++++++++-----------
> > 1 file changed, 50 insertions(+), 19 deletions(-)
> >
> > diff --git a/UefiCpuPkg/CpuDxe/CpuPageTable.c
> b/UefiCpuPkg/CpuDxe/CpuPageTable.c
> > index d312eb66f8..a1d804caed 100644
> > --- a/UefiCpuPkg/CpuDxe/CpuPageTable.c
> > +++ b/UefiCpuPkg/CpuDxe/CpuPageTable.c
> > @@ -789,8 +789,7 @@ RefreshGcdMemoryAttributesFromPaging (
> > UINT64 BaseAddress;
> > UINT64 PageStartAddress;
> > UINT64 Attributes;
> > - UINT64 Capabilities;
> > - BOOLEAN DoUpdate;
> > + UINT64 NewAttributes;
> > UINTN Index;
> >
> > //
> > @@ -802,9 +801,8 @@ RefreshGcdMemoryAttributesFromPaging (
> >
> > GetCurrentPagingContext (&PagingContext);
> >
> > - DoUpdate = FALSE;
> > - Capabilities = 0;
> > Attributes = 0;
> > + NewAttributes = 0;
> > BaseAddress = 0;
> > PageLength = 0;
> >
> > @@ -813,6 +811,34 @@ RefreshGcdMemoryAttributesFromPaging (
> > continue;
> > }
> >
> > + //
> > + // Sync the actual paging related capabilities back to GCD service first.
> > + // As a side effect (good one), this can also help to avoid unnecessary
> > + // memory map entries due to the different capabilities of the same type
> > + // memory, such as multiple RT_CODE and RT_DATA entries in memory
> map,
> > + // which could cause boot failure of some old Linux distro (before v4.3).
> > + //
> > + Status = gDS->SetMemorySpaceCapabilities (
> > + MemorySpaceMap[Index].BaseAddress,
> > + MemorySpaceMap[Index].Length,
> > + MemorySpaceMap[Index].Capabilities |
> > + EFI_MEMORY_PAGETYPE_MASK
> > + );
> > + if (EFI_ERROR (Status)) {
> > + //
> > + // If we cannot udpate the capabilities, we cannot update its
> > + // attributes either. So just simply skip current block of memory.
> > + //
> > + DEBUG ((
> > + DEBUG_WARN,
> > + "Failed to update capability: [%d] %016lx - %016lx (%016lx -
> > %016lx)\r\n",
> > + Index, BaseAddress, BaseAddress + Length - 1,
>
> (1) I think you forgot about my note that Index (which is of type UINTN)
> should not be printed with "%d". Instead, (UINT64)Index should be
> printed with "%lu".
>
> > + MemorySpaceMap[Index].Capabilities,
> > + MemorySpaceMap[Index].Capabilities | EFI_MEMORY_PAGETYPE_MASK
> > + ));
> > + continue;
> > + }
> > +
> > if (MemorySpaceMap[Index].BaseAddress >= (BaseAddress + PageLength)) {
> > //
> > // Current memory space starts at a new page. Resetting PageLength will
> > @@ -826,7 +852,9 @@ RefreshGcdMemoryAttributesFromPaging (
> > PageLength -= (MemorySpaceMap[Index].BaseAddress - BaseAddress);
> > }
> >
> > - // Sync real page attributes to GCD
> > + //
> > + // Sync actual page attributes to GCD
> > + //
> > BaseAddress = MemorySpaceMap[Index].BaseAddress;
> > MemorySpaceLength = MemorySpaceMap[Index].Length;
> > while (MemorySpaceLength > 0) {
> > @@ -842,23 +870,26 @@ RefreshGcdMemoryAttributesFromPaging (
> > PageStartAddress = (*PageEntry) &
> (UINT64)PageAttributeToMask(PageAttribute);
> > PageLength = PageAttributeToLength (PageAttribute) - (BaseAddress -
> PageStartAddress);
> > Attributes = GetAttributesFromPageEntry (PageEntry);
> > -
> > - 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;
> > - }
> > }
> >
> > Length = MIN (PageLength, MemorySpaceLength);
> > - if (DoUpdate) {
> > - gDS->SetMemorySpaceCapabilities (BaseAddress, Length, Capabilities);
> > - gDS->SetMemorySpaceAttributes (BaseAddress, Length, Attributes);
> > - DEBUG ((DEBUG_INFO, "Update memory space attribute: [%02d] %016lx
> - %016lx (%08lx -> %08lx)\r\n",
> > - Index, BaseAddress, BaseAddress + Length - 1,
> > - MemorySpaceMap[Index].Attributes, Attributes));
> > + if (Attributes != (MemorySpaceMap[Index].Attributes
> > + & EFI_MEMORY_PAGETYPE_MASK)) {
> > + NewAttributes = (MemorySpaceMap[Index].Attributes
> > + & ~EFI_MEMORY_PAGETYPE_MASK) | Attributes;
>
> (2) To my understanding, the edk2 coding style wants us to keep the
> bitwise-and operator ("&") at the end of the line.
>
> > + Status = gDS->SetMemorySpaceAttributes (
> > + BaseAddress,
> > + Length,
> > + NewAttributes
> > + );
> > + ASSERT_EFI_ERROR (Status);
> > + DEBUG ((
> > + DEBUG_INFO,
> > + "Updated memory space attribute: [%d] %016lx - %016lx (%016lx -
> > %016lx)\r\n",
> > + Index, BaseAddress, BaseAddress + Length - 1,
>
> (3) Same comment as (1).
>
> Thanks,
> Laszlo
>
> > + MemorySpaceMap[Index].Attributes,
> > + NewAttributes
> > + ));
> > }
> >
> > PageLength -= Length;
> >
^ permalink raw reply [flat|nested] 6+ messages in thread
end of thread, other threads:[~2017-11-10 0:18 UTC | newest]
Thread overview: 6+ messages (download: mbox.gz follow: Atom feed
-- links below jump to the message on this page --
2017-11-09 1:39 [PATCH v3] UefiCpuPkg/CpuDxe: Fix multiple entries of RT_CODE in memory map Jian J Wang
2017-11-09 14:12 ` Laszlo Ersek
2017-11-10 0:22 ` Wang, Jian J
-- strict thread matches above, loose matches on Subject: below --
2017-11-08 10:52 Jian J Wang
2017-11-08 14:36 ` Laszlo Ersek
2017-11-09 0:51 ` 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