Re: [edk2-devel] [PATCH v2] UefiCpuPkg: Fix issue that IsModified is wrongly set in PageTableMap

["Laszlo Ersek" <lersek@redhat.com>]

On 1/17/24 09:09, 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 considering the hardware may also change page table,
> and document the detail in function header.
> 
> 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>
> Signed-off-by: Zhiguang Liu <zhiguang.liu@intel.com>
> ---
>  .../Library/CpuPageTableLib/CpuPageTableMap.c    | 16 ++++++++++++----
>  1 file changed, 12 insertions(+), 4 deletions(-)
> 
> diff --git a/UefiCpuPkg/Library/CpuPageTableLib/CpuPageTableMap.c b/UefiCpuPkg/Library/CpuPageTableLib/CpuPageTableMap.c
> index 36b2c4e6a3..a3076ff2f6 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.
> @@ -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.

This patch looks good to me, thanks, except for one small wart: in the
documentation section of PageTableLibMapInLevel(), you change IsModified
from "@param[out]" to "@param[in, out]", which is correct, *but* a
similar change has been omitted for the actual parameter in the
parameter list:

  OUT    BOOLEAN             *IsModified

This should also become "IN OUT".

Thanks!
Laszlo



-=-=-=-=-=-=-=-=-=-=-=-
Groups.io Links: You receive all messages sent to this group.
View/Reply Online (#113948): https://edk2.groups.io/g/devel/message/113948
Mute This Topic: https://groups.io/mt/103781942/7686176
Group Owner: devel+owner@edk2.groups.io
Unsubscribe: https://edk2.groups.io/g/devel/leave/12367111/7686176/1913456212/xyzzy [rebecca@openfw.io]
-=-=-=-=-=-=-=-=-=-=-=-