Re: [Patch V4 08/21] UefiCpuPkg/CpuPageTableLib:Add check for Mask and Attr

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

Thanks for the comments Ray. I will update the patches in next version patch set.

-----Why break out of the loop when meeting a non-present entry?
Because I think the parameter check is only needed once.

Thanks,
Dun

-----Original Message-----
From: Ni, Ray <ray.ni@intel.com> 
Sent: Thursday, March 23, 2023 8:53 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 V4 08/21] UefiCpuPkg/CpuPageTableLib:Add check for Mask and Attr

The commit message is very detailed.

But the function header comments are not.
Can you please update the function header comments to add 4 lines of @retval RETURN_INVALID_PARAMETER, each line describes one invalid case.

It's ok to have multiple lines starting with @retval RETURN_INVALID_PARAMETER>

> +    // If (LinearAddress + Length - 1) is not in the same 
> + ParentPagingEntry
> with (LinearAddress + Offset), then the remaining child PagingEntry
> +    // starting from PagingEntryIndex of ParentPagingEntry is all 
> + covered by
> [LinearAddress + Offset, LinearAddress + Length - 1].
> +    //
> +    PagingEntryIndexEnd = (BitFieldRead64 (LinearAddress + Length - 
> + 1,
> BitStart + 9, 63) != BitFieldRead64 (LinearAddress + Offset, BitStart + 9, 63)) ?
> 511 :
> +                          (UINTN)BitFieldRead64 (LinearAddress + 
> + Length - 1, BitStart,
> BitStart + 9 - 1);
> +    PagingEntry = (IA32_PAGING_ENTRY
> *)(UINTN)IA32_PNLE_PAGE_TABLE_BASE_ADDRESS (&ParentPagingEntry-
> >Pnle);
> +    for (Index = PagingEntryIndex; Index <= PagingEntryIndexEnd; Index++) {
> +      if (PagingEntry[Index].Pce.Present == 0) {
> +        //
> +        // [LinearAddress, LinearAddress + Length] contains non-present range.
> +        //
> +        Status = IsAttributesAndMaskValidForNonPresentEntry 
> + (Attribute,
> Mask);
> +        if (RETURN_ERROR (Status)) {
> +          return Status;
> +        }
> +
> +        break;

Why break out of the loop when meeting a non-present entry?