* [edk2-devel] [PATCH v3] UefiCpuPkg: Fix issue that IsModified is wrongly set in PageTableMap
@ 2024-01-23 7:15 Zhiguang Liu
2024-01-23 13:49 ` Laszlo Ersek
2024-01-24 12:24 ` Ni, Ray
0 siblings, 2 replies; 3+ messages in thread
From: Zhiguang Liu @ 2024-01-23 7:15 UTC (permalink / raw)
To: devel
Cc: Zhiguang Liu, Ray Ni, Laszlo Ersek, Rahul Kumar, Gerd Hoffmann,
Crystal Lee, Pedro Falcato
REF: https://bugzilla.tianocore.org/show_bug.cgi?id=4614
About the IsModified, current function doesn't consider that hardware
also may change the pagetable. The issue is that in the first call of
internal function PageTableLibMapInLevel, the function assume page
table is not changed, and add ASSERT to check. But hardware may change
the page table, which cause the ASSERT happens.
Fix the issue by adding addtional condition to only check if the page
table is changed when the software want to modify the page table.
Also, add more comment to explain this behavior.
Cc: Ray Ni <ray.ni@intel.com>
Cc: Laszlo Ersek <lersek@redhat.com>
Cc: Rahul Kumar <rahul1.kumar@intel.com>
Cc: Gerd Hoffmann <kraxel@redhat.com>
Cc: Crystal Lee <CrystalLee@ami.com.tw>
Cc: Pedro Falcato <pedro.falcato@gmail.com>
Signed-off-by: Zhiguang Liu <zhiguang.liu@intel.com>
---
.../Library/CpuPageTableLib/CpuPageTableMap.c | 18 +++++++++++++-----
1 file changed, 13 insertions(+), 5 deletions(-)
diff --git a/UefiCpuPkg/Library/CpuPageTableLib/CpuPageTableMap.c b/UefiCpuPkg/Library/CpuPageTableLib/CpuPageTableMap.c
index 36b2c4e6a3..ea6547970a 100644
--- a/UefiCpuPkg/Library/CpuPageTableLib/CpuPageTableMap.c
+++ b/UefiCpuPkg/Library/CpuPageTableLib/CpuPageTableMap.c
@@ -274,7 +274,7 @@ IsAttributesAndMaskValidForNonPresentEntry (
Page table entries that map the linear address range are reset to 0 before set to the new attribute
when a new physical base address is set.
@param[in] Mask The mask used for attribute. The corresponding field in Attribute is ignored if that in Mask is 0.
- @param[out] IsModified TRUE means page table is modified. FALSE means page table is not modified.
+ @param[in, out] IsModified Change IsModified to True if page table is modified and input parameter Modify is TRUE.
@retval RETURN_INVALID_PARAMETER For non-present range, Mask->Bits.Present is 0 but some other attributes are provided.
@retval RETURN_INVALID_PARAMETER For non-present range, Mask->Bits.Present is 1, Attribute->Bits.Present is 1 but some other attributes are not provided.
@@ -294,7 +294,7 @@ PageTableLibMapInLevel (
IN UINT64 Offset,
IN IA32_MAP_ATTRIBUTE *Attribute,
IN IA32_MAP_ATTRIBUTE *Mask,
- OUT BOOLEAN *IsModified
+ IN OUT BOOLEAN *IsModified
)
{
RETURN_STATUS Status;
@@ -567,7 +567,10 @@ PageTableLibMapInLevel (
OriginalCurrentPagingEntry.Uint64 = CurrentPagingEntry->Uint64;
PageTableLibSetPle (Level, CurrentPagingEntry, Offset, Attribute, &CurrentMask);
- if (OriginalCurrentPagingEntry.Uint64 != CurrentPagingEntry->Uint64) {
+ if (Modify && (OriginalCurrentPagingEntry.Uint64 != CurrentPagingEntry->Uint64)) {
+ //
+ // The page table entry can be changed by this function only when Modify is true.
+ //
*IsModified = TRUE;
}
}
@@ -609,7 +612,10 @@ PageTableLibMapInLevel (
// Check if ParentPagingEntry entry is modified here is enough. Except the changes happen in leaf PagingEntry during
// the while loop, if there is any other change happens in page table, the ParentPagingEntry must has been modified.
//
- if (OriginalParentPagingEntry.Uint64 != ParentPagingEntry->Uint64) {
+ if (Modify && (OriginalParentPagingEntry.Uint64 != ParentPagingEntry->Uint64)) {
+ //
+ // The page table entry can be changed by this function only when Modify is true.
+ //
*IsModified = TRUE;
}
@@ -633,7 +639,9 @@ PageTableLibMapInLevel (
Page table entries that map the linear address range are reset to 0 before set to the new attribute
when a new physical base address is set.
@param[in] Mask The mask used for attribute. The corresponding field in Attribute is ignored if that in Mask is 0.
- @param[out] IsModified TRUE means page table is modified. FALSE means page table is not modified.
+ @param[out] IsModified TRUE means page table is modified by software or hardware. FALSE means page table is not modified by software.
+ If the output IsModified is FALSE, there is possibility that the page table is changed by hardware. It is ok
+ because page table can be changed by hardware anytime, and caller don't need to Flush TLB.
@retval RETURN_UNSUPPORTED PagingMode is not supported.
@retval RETURN_INVALID_PARAMETER PageTable, BufferSize, Attribute or Mask is NULL.
--
2.31.1.windows.1
-=-=-=-=-=-=-=-=-=-=-=-
Groups.io Links: You receive all messages sent to this group.
View/Reply Online (#114185): https://edk2.groups.io/g/devel/message/114185
Mute This Topic: https://groups.io/mt/103906298/7686176
Group Owner: devel+owner@edk2.groups.io
Unsubscribe: https://edk2.groups.io/g/devel/unsub [rebecca@openfw.io]
-=-=-=-=-=-=-=-=-=-=-=-
^ permalink raw reply related [flat|nested] 3+ messages in thread
* Re: [edk2-devel] [PATCH v3] UefiCpuPkg: Fix issue that IsModified is wrongly set in PageTableMap
2024-01-23 7:15 [edk2-devel] [PATCH v3] UefiCpuPkg: Fix issue that IsModified is wrongly set in PageTableMap Zhiguang Liu
@ 2024-01-23 13:49 ` Laszlo Ersek
2024-01-24 12:24 ` Ni, Ray
1 sibling, 0 replies; 3+ messages in thread
From: Laszlo Ersek @ 2024-01-23 13:49 UTC (permalink / raw)
To: devel, zhiguang.liu
Cc: Ray Ni, Rahul Kumar, Gerd Hoffmann, Crystal Lee, Pedro Falcato
On 1/23/24 08:15, Zhiguang Liu wrote:
> REF: https://bugzilla.tianocore.org/show_bug.cgi?id=4614
>
> About the IsModified, current function doesn't consider that hardware
> also may change the pagetable. The issue is that in the first call of
> internal function PageTableLibMapInLevel, the function assume page
> table is not changed, and add ASSERT to check. But hardware may change
> the page table, which cause the ASSERT happens.
> Fix the issue by adding addtional condition to only check if the page
> table is changed when the software want to modify the page table.
> Also, add more comment to explain this behavior.
>
> Cc: Ray Ni <ray.ni@intel.com>
> Cc: Laszlo Ersek <lersek@redhat.com>
> Cc: Rahul Kumar <rahul1.kumar@intel.com>
> Cc: Gerd Hoffmann <kraxel@redhat.com>
> Cc: Crystal Lee <CrystalLee@ami.com.tw>
> Cc: Pedro Falcato <pedro.falcato@gmail.com>
> Signed-off-by: Zhiguang Liu <zhiguang.liu@intel.com>
> ---
> .../Library/CpuPageTableLib/CpuPageTableMap.c | 18 +++++++++++++-----
> 1 file changed, 13 insertions(+), 5 deletions(-)
>
> diff --git a/UefiCpuPkg/Library/CpuPageTableLib/CpuPageTableMap.c b/UefiCpuPkg/Library/CpuPageTableLib/CpuPageTableMap.c
> index 36b2c4e6a3..ea6547970a 100644
> --- a/UefiCpuPkg/Library/CpuPageTableLib/CpuPageTableMap.c
> +++ b/UefiCpuPkg/Library/CpuPageTableLib/CpuPageTableMap.c
> @@ -274,7 +274,7 @@ IsAttributesAndMaskValidForNonPresentEntry (
> Page table entries that map the linear address range are reset to 0 before set to the new attribute
> when a new physical base address is set.
> @param[in] Mask The mask used for attribute. The corresponding field in Attribute is ignored if that in Mask is 0.
> - @param[out] IsModified TRUE means page table is modified. FALSE means page table is not modified.
> + @param[in, out] IsModified Change IsModified to True if page table is modified and input parameter Modify is TRUE.
>
> @retval RETURN_INVALID_PARAMETER For non-present range, Mask->Bits.Present is 0 but some other attributes are provided.
> @retval RETURN_INVALID_PARAMETER For non-present range, Mask->Bits.Present is 1, Attribute->Bits.Present is 1 but some other attributes are not provided.
> @@ -294,7 +294,7 @@ PageTableLibMapInLevel (
> IN UINT64 Offset,
> IN IA32_MAP_ATTRIBUTE *Attribute,
> IN IA32_MAP_ATTRIBUTE *Mask,
> - OUT BOOLEAN *IsModified
> + IN OUT BOOLEAN *IsModified
> )
> {
> RETURN_STATUS Status;
> @@ -567,7 +567,10 @@ PageTableLibMapInLevel (
> OriginalCurrentPagingEntry.Uint64 = CurrentPagingEntry->Uint64;
> PageTableLibSetPle (Level, CurrentPagingEntry, Offset, Attribute, &CurrentMask);
>
> - if (OriginalCurrentPagingEntry.Uint64 != CurrentPagingEntry->Uint64) {
> + if (Modify && (OriginalCurrentPagingEntry.Uint64 != CurrentPagingEntry->Uint64)) {
> + //
> + // The page table entry can be changed by this function only when Modify is true.
> + //
> *IsModified = TRUE;
> }
> }
> @@ -609,7 +612,10 @@ PageTableLibMapInLevel (
> // Check if ParentPagingEntry entry is modified here is enough. Except the changes happen in leaf PagingEntry during
> // the while loop, if there is any other change happens in page table, the ParentPagingEntry must has been modified.
> //
> - if (OriginalParentPagingEntry.Uint64 != ParentPagingEntry->Uint64) {
> + if (Modify && (OriginalParentPagingEntry.Uint64 != ParentPagingEntry->Uint64)) {
> + //
> + // The page table entry can be changed by this function only when Modify is true.
> + //
> *IsModified = TRUE;
> }
>
> @@ -633,7 +639,9 @@ PageTableLibMapInLevel (
> Page table entries that map the linear address range are reset to 0 before set to the new attribute
> when a new physical base address is set.
> @param[in] Mask The mask used for attribute. The corresponding field in Attribute is ignored if that in Mask is 0.
> - @param[out] IsModified TRUE means page table is modified. FALSE means page table is not modified.
> + @param[out] IsModified TRUE means page table is modified by software or hardware. FALSE means page table is not modified by software.
> + If the output IsModified is FALSE, there is possibility that the page table is changed by hardware. It is ok
> + because page table can be changed by hardware anytime, and caller don't need to Flush TLB.
>
> @retval RETURN_UNSUPPORTED PagingMode is not supported.
> @retval RETURN_INVALID_PARAMETER PageTable, BufferSize, Attribute or Mask is NULL.
Reviewed-by: Laszlo Ersek <lersek@redhat.com>
-=-=-=-=-=-=-=-=-=-=-=-
Groups.io Links: You receive all messages sent to this group.
View/Reply Online (#114196): https://edk2.groups.io/g/devel/message/114196
Mute This Topic: https://groups.io/mt/103906298/7686176
Group Owner: devel+owner@edk2.groups.io
Unsubscribe: https://edk2.groups.io/g/devel/leave/12367111/7686176/1913456212/xyzzy [rebecca@openfw.io]
-=-=-=-=-=-=-=-=-=-=-=-
^ permalink raw reply [flat|nested] 3+ messages in thread
* Re: [edk2-devel] [PATCH v3] UefiCpuPkg: Fix issue that IsModified is wrongly set in PageTableMap
2024-01-23 7:15 [edk2-devel] [PATCH v3] UefiCpuPkg: Fix issue that IsModified is wrongly set in PageTableMap Zhiguang Liu
2024-01-23 13:49 ` Laszlo Ersek
@ 2024-01-24 12:24 ` Ni, Ray
1 sibling, 0 replies; 3+ messages in thread
From: Ni, Ray @ 2024-01-24 12:24 UTC (permalink / raw)
To: Liu, Zhiguang, devel@edk2.groups.io
Cc: Laszlo Ersek, Kumar, Rahul R, Gerd Hoffmann, Lee, Crystal,
Pedro Falcato
Reviewed-by: Ray Ni <ray.ni@intel.com>
Thanks,
Ray
> -----Original Message-----
> From: Liu, Zhiguang <zhiguang.liu@intel.com>
> Sent: Tuesday, January 23, 2024 3:16 PM
> To: devel@edk2.groups.io
> Cc: Liu, Zhiguang <zhiguang.liu@intel.com>; Ni, Ray <ray.ni@intel.com>; Laszlo
> Ersek <lersek@redhat.com>; Kumar, Rahul R <rahul.r.kumar@intel.com>; Gerd
> Hoffmann <kraxel@redhat.com>; Lee, Crystal <crystalLee@ami.com.tw>;
> Pedro Falcato <pedro.falcato@gmail.com>
> Subject: [PATCH v3] UefiCpuPkg: Fix issue that IsModified is wrongly set in
> PageTableMap
>
> REF: https://bugzilla.tianocore.org/show_bug.cgi?id=4614
>
> About the IsModified, current function doesn't consider that hardware
> also may change the pagetable. The issue is that in the first call of
> internal function PageTableLibMapInLevel, the function assume page
> table is not changed, and add ASSERT to check. But hardware may change
> the page table, which cause the ASSERT happens.
> Fix the issue by adding addtional condition to only check if the page
> table is changed when the software want to modify the page table.
> Also, add more comment to explain this behavior.
>
> Cc: Ray Ni <ray.ni@intel.com>
> Cc: Laszlo Ersek <lersek@redhat.com>
> Cc: Rahul Kumar <rahul1.kumar@intel.com>
> Cc: Gerd Hoffmann <kraxel@redhat.com>
> Cc: Crystal Lee <CrystalLee@ami.com.tw>
> Cc: Pedro Falcato <pedro.falcato@gmail.com>
> Signed-off-by: Zhiguang Liu <zhiguang.liu@intel.com>
> ---
> .../Library/CpuPageTableLib/CpuPageTableMap.c | 18 +++++++++++++-----
> 1 file changed, 13 insertions(+), 5 deletions(-)
>
> diff --git a/UefiCpuPkg/Library/CpuPageTableLib/CpuPageTableMap.c
> b/UefiCpuPkg/Library/CpuPageTableLib/CpuPageTableMap.c
> index 36b2c4e6a3..ea6547970a 100644
> --- a/UefiCpuPkg/Library/CpuPageTableLib/CpuPageTableMap.c
> +++ b/UefiCpuPkg/Library/CpuPageTableLib/CpuPageTableMap.c
> @@ -274,7 +274,7 @@ IsAttributesAndMaskValidForNonPresentEntry (
> Page table entries that map the linear address range are
> reset to 0 before set to the new attribute
> when a new physical base address is set.
> @param[in] Mask The mask used for attribute. The corresponding
> field in Attribute is ignored if that in Mask is 0.
> - @param[out] IsModified TRUE means page table is modified. FALSE
> means page table is not modified.
> + @param[in, out] IsModified Change IsModified to True if page table is
> modified and input parameter Modify is TRUE.
>
> @retval RETURN_INVALID_PARAMETER For non-present range, Mask-
> >Bits.Present is 0 but some other attributes are provided.
> @retval RETURN_INVALID_PARAMETER For non-present range, Mask-
> >Bits.Present is 1, Attribute->Bits.Present is 1 but some other attributes are
> not provided.
> @@ -294,7 +294,7 @@ PageTableLibMapInLevel (
> IN UINT64 Offset,
> IN IA32_MAP_ATTRIBUTE *Attribute,
> IN IA32_MAP_ATTRIBUTE *Mask,
> - OUT BOOLEAN *IsModified
> + IN OUT BOOLEAN *IsModified
> )
> {
> RETURN_STATUS Status;
> @@ -567,7 +567,10 @@ PageTableLibMapInLevel (
> OriginalCurrentPagingEntry.Uint64 = CurrentPagingEntry->Uint64;
> PageTableLibSetPle (Level, CurrentPagingEntry, Offset, Attribute,
> &CurrentMask);
>
> - if (OriginalCurrentPagingEntry.Uint64 != CurrentPagingEntry->Uint64) {
> + if (Modify && (OriginalCurrentPagingEntry.Uint64 != CurrentPagingEntry-
> >Uint64)) {
> + //
> + // The page table entry can be changed by this function only when
> Modify is true.
> + //
> *IsModified = TRUE;
> }
> }
> @@ -609,7 +612,10 @@ PageTableLibMapInLevel (
> // Check if ParentPagingEntry entry is modified here is enough. Except the
> changes happen in leaf PagingEntry during
> // the while loop, if there is any other change happens in page table, the
> ParentPagingEntry must has been modified.
> //
> - if (OriginalParentPagingEntry.Uint64 != ParentPagingEntry->Uint64) {
> + if (Modify && (OriginalParentPagingEntry.Uint64 != ParentPagingEntry-
> >Uint64)) {
> + //
> + // The page table entry can be changed by this function only when Modify
> is true.
> + //
> *IsModified = TRUE;
> }
>
> @@ -633,7 +639,9 @@ PageTableLibMapInLevel (
> Page table entries that map the linear address range are reset
> to 0 before set to the new attribute
> when a new physical base address is set.
> @param[in] Mask The mask used for attribute. The corresponding
> field in Attribute is ignored if that in Mask is 0.
> - @param[out] IsModified TRUE means page table is modified. FALSE
> means page table is not modified.
> + @param[out] IsModified TRUE means page table is modified by
> software or hardware. FALSE means page table is not modified by software.
> + If the output IsModified is FALSE, there is possibility that the
> page table is changed by hardware. It is ok
> + because page table can be changed by hardware anytime,
> and caller don't need to Flush TLB.
>
> @retval RETURN_UNSUPPORTED PagingMode is not supported.
> @retval RETURN_INVALID_PARAMETER PageTable, BufferSize, Attribute or
> Mask is NULL.
> --
> 2.31.1.windows.1
-=-=-=-=-=-=-=-=-=-=-=-
Groups.io Links: You receive all messages sent to this group.
View/Reply Online (#114281): https://edk2.groups.io/g/devel/message/114281
Mute This Topic: https://groups.io/mt/103906298/7686176
Group Owner: devel+owner@edk2.groups.io
Unsubscribe: https://edk2.groups.io/g/devel/leave/12367111/7686176/1913456212/xyzzy [rebecca@openfw.io]
-=-=-=-=-=-=-=-=-=-=-=-
^ permalink raw reply [flat|nested] 3+ messages in thread
end of thread, other threads:[~2024-01-24 12:24 UTC | newest]
Thread overview: 3+ messages (download: mbox.gz follow: Atom feed
-- links below jump to the message on this page --
2024-01-23 7:15 [edk2-devel] [PATCH v3] UefiCpuPkg: Fix issue that IsModified is wrongly set in PageTableMap Zhiguang Liu
2024-01-23 13:49 ` Laszlo Ersek
2024-01-24 12:24 ` Ni, Ray
This is a public inbox, see mirroring instructions
for how to clone and mirror all data and code used for this inbox