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...@intel.com> Sent: Wednesday, March 15, 2023 1:49 PM To: Tan, Dun <dun....@intel.com>; devel@edk2.groups.io Cc: Dong, Eric <eric.d...@intel.com>; Kumar, Rahul R <rahul.r.ku...@intel.com>; Gerd Hoffmann <kra...@redhat.com>; Liu, Zhiguang <zhiguang....@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 -=-=-=-=-=-=-=-=-=-=-=- Groups.io Links: You receive all messages sent to this group. View/Reply Online (#101223): https://edk2.groups.io/g/devel/message/101223 Mute This Topic: https://groups.io/mt/97469479/21656 Group Owner: devel+ow...@edk2.groups.io Unsubscribe: https://edk2.groups.io/g/devel/unsub [arch...@mail-archive.com] -=-=-=-=-=-=-=-=-=-=-=-