Re: [Patch V2 07/14] UefiCpuPkg/CpuPageTableLib:Modify RandomTest to check Mask/Attr

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

Thanks Ray for the comments. Will create a new patch to split the RandomBoolean() and simplify the code loop in random test

Thanks,
Dun
-----Original Message-----
From: Ni, Ray <ray.ni@intel.com> 
Sent: Wednesday, March 15, 2023 1:49 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>; Liu, Zhiguang <zhiguang.liu@intel.com>
Subject: RE: [Patch V2 07/14] UefiCpuPkg/CpuPageTableLib:Modify RandomTest to check Mask/Attr

> 
> -/**
> -  Return a random boolean.
> -
> -  @return boolean
> -**/
> -BOOLEAN
> -RandomBoolean (
> -  VOID
> -  )
> -{
> -  BOOLEAN  Value;
> -
> -  LocalRandomBytes ((UINT8 *)&Value, sizeof (BOOLEAN));
> -  return Value%2;
> -}
> -
>  /**
>    Return a 32bit random number.
> 
> @@ -139,6 +123,21 @@ Random64 (
>    return (UINT64)(Value % (Limit - Start  + 1)) + Start;  }
> 
> +/**
> +  Returns true with the percentage of input Probability.
> +
> +  @param[in]   Probability    The percentage to return true.
> +
> +  @return boolean
> +**/
> +BOOLEAN
> +RandomBoolean (
> +  UINT8  Probability
> +  )
> +{
> +  return ((Probability > ((UINT8)Random64 (0, 100))) ? TRUE : FALSE); 
> +}
1. can you split the RandomBoolean() change in standalone patch?


2. can you simplify below code to only use for-loop?
> +  //
> +  if (MapEntrys->Maps[MapsIndex].Length > 0) {
> +    if (MapCount != 0) {
> +      UT_ASSERT_EQUAL (Status, RETURN_BUFFER_TOO_SMALL);
> +      Map = AllocatePages (EFI_SIZE_TO_PAGES (MapCount * sizeof
> (IA32_MAP_ENTRY)));
> +      ASSERT (Map != NULL);
> +      Status = PageTableParse (*PageTable, PagingMode, Map, 
> + &MapCount);
> +
> +      if (Map[MapCount - 1].LinearAddress + Map[MapCount - 1].Length 
> + <
> MapEntrys->Maps[MapsIndex].LinearAddress + MapEntrys-
> >Maps[MapsIndex].Length) {
> +        IsNotPresent = TRUE;
> +      } else {
> +        for (Index = 0; Index < MapCount; Index++) {
> +          if ((PreviousAddress < Map[Index].LinearAddress) &&
> +              (MapEntrys->Maps[MapsIndex].LinearAddress <
> Map[Index].LinearAddress) &&
> +              ((MapEntrys->Maps[MapsIndex].LinearAddress + MapEntrys-
> >Maps[MapsIndex].Length) > PreviousAddress))
> +          {
> +            //
> +            // MapEntrys->Maps[MapsIndex] contains not-present range 
> + in
> exsiting page table.
> +            //
> +            IsNotPresent = TRUE;
> +            break;
> +          }
> +
> +          PreviousAddress = Map[Index].LinearAddress + Map[Index].Length;
> +        }
> +      }
> +    } else {
> +      IsNotPresent = TRUE;
> +    }
> +  }
> 
>    PageTableBufferSize = 0;
>    Status              = PageTableMap (
> @@ -638,6 +700,25 @@ SingleMapEntryTest (
>                            &MapEntrys->Maps[MapsIndex].Attribute,
>                            &MapEntrys->Maps[MapsIndex].Mask
>                            );
> +
> +  //
> +  // Return Status should be InvalidParameter when:
> +  // 1. MapEntrys->Maps[MapsIndex] contains not-present range.
> +  // 2. MapEntrys->Maps[MapsIndex].Mask contains zero value field or
> Attribute->Bits.Present is 0.
> +  //
> +  Attribute = &MapEntrys->Maps[MapsIndex].Attribute;
> +  Mask      = &MapEntrys->Maps[MapsIndex].Mask;
> +  if (((Attribute->Bits.Present == 0) || (Mask->Bits.Present == 0) || 
> + (Mask-
> >Bits.ReadWrite == 0) ||
> +       (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)) &&
> +      IsNotPresent)
> +  {
> +    RemoveLastMapEntry (MapEntrys);
> +    UT_ASSERT_EQUAL (Status, RETURN_INVALID_PARAMETER);
> +    return UNIT_TEST_PASSED;
> +  }
> +
>    if (PageTableBufferSize != 0) {
>      UT_ASSERT_EQUAL (Status, RETURN_BUFFER_TOO_SMALL);
> 
> diff --git a/UefiCpuPkg/Library/CpuPageTableLib/UnitTest/TestHelper.c
> b/UefiCpuPkg/Library/CpuPageTableLib/UnitTest/TestHelper.c
> index 5bd70c0f65..11f7e607ca 100644
> --- a/UefiCpuPkg/Library/CpuPageTableLib/UnitTest/TestHelper.c
> +++ b/UefiCpuPkg/Library/CpuPageTableLib/UnitTest/TestHelper.c
> @@ -171,6 +171,10 @@ IsPageTableValid (
>    UNIT_TEST_STATUS   Status;
>    IA32_PAGING_ENTRY  *PagingEntry;
> 
> +  if (PageTable == 0) {
> +    return UNIT_TEST_PASSED;
> +  }
> +
>    if ((PagingMode == Paging32bit) || (PagingMode == PagingPae) || 
> (PagingMode >= PagingModeMax)) {
>      //
>      // 32bit paging is never supported.
> --
> 2.31.1.windows.1