Re: [Patch V2 05/14] UefiCpuPkg/CpuPageTebleLib: Check Mask and Attr in PageTableMap

["duntan" <dun.tan@intel.com>]

Thanks for the comments. Will do the code change in next version patch set.
Thanks,
Dun

-----Original Message-----
From: Ni, Ray <ray.ni@intel.com> 
Sent: Wednesday, March 15, 2023 1:34 PM
To: Tan, Dun <dun.tan@intel.com>; devel@edk2.groups.io
Cc: Dong, Eric <eric.dong@intel.com>; Kumar, Rahul R <rahul.r.kumar@intel.com>; Gerd Hoffmann <kraxel@redhat.com>
Subject: RE: [Patch V2 05/14] UefiCpuPkg/CpuPageTebleLib: Check Mask and Attr in PageTableMap

> +**/
> +RETURN_STATUS
> +CheckMaskAndAttrForNotPresentEntry (
> +  IN     IA32_MAP_ATTRIBUTE  *Attribute,
> +  IN     IA32_MAP_ATTRIBUTE  *Mask
> +  )
> +{
> +  if ((Attribute->Bits.Present == 0) || (Mask->Bits.Present == 0) || (Mask-
> >Bits.ReadWrite == 0) ||

1. I think we can allow caller to set a not-present range as not-present.
Even though it's meaningless😊
So, I think we can remove the Attribute.Present check.


2. The function name can be more readable: how about IsAllAttributesSetForNonPresentEntry()?


> +      (Mask->Bits.UserSupervisor == 0) || (Mask->Bits.WriteThrough == 0) ||
> (Mask->Bits.CacheDisabled == 0) ||
> +      (Mask->Bits.Accessed == 0) || (Mask->Bits.Dirty == 0) || (Mask-
> >Bits.Pat == 0) || (Mask->Bits.Global == 0) ||
> +      (Mask->Bits.PageTableBaseAddress == 0) || (Mask->Bits.ProtectionKey
> == 0) || (Mask->Bits.Nx == 0))
> +  {
> +    return RETURN_INVALID_PARAMETER;
> +  }
> +
> +  return RETURN_SUCCESS;
> +}

> +    PagingEntry           = (IA32_PAGING_ENTRY
> *)(UINTN)IA32_PNLE_PAGE_TABLE_BASE_ADDRESS (&ParentPagingEntry-
> >Pnle);
> +    PagingEntryIndexLimit = (BitFieldRead64 (LinearAddress + Length - 1,
> BitStart + 9, 63) > BitFieldRead64 (LinearAddress + Offset, BitStart + 9, 63)) ?
> 511 :

3. Can you add more comments for the code here?