On 02/23/18 12:16, Pankaj Bansal wrote: > Hi Liming Gao, > > Thanks for your comments. > I am not able to understand the full purpose of the BitField functions. > Can you please help me to understand. > > As per my understanding : > BitFieldAnd32 (Operand, StartBit, EndBit, AndData) : > > We want to calculate the bitwise and (&) between Operand's bits (StartBit to > EndBit, both inclusive) and AndData's bits (Startbit to Endbit, both > inclusive). > If this is the case, then why is there a restriction on AndData ? (If AndData > is larger than the bitmask value range specified by StartBit and EndBit, then > ASSERT().) > > How these functions are intended to be used ?
StartBit and EndBit designate a bit-field within Operand. Operand is the "container" of the bit-field. "AndData" is meant for the bit-field itself, not the container. AndData is applied relative to StartBit, and (relative to StartBit) it must not exceed EndBit. So BitFieldAnd32() applies the AndData mask to the bit-field within the 32-bit container word. Bits outside of the bit-field are unchanged. Laszlo >> -----Original Message----- >> From: Gao, Liming [mailto:[email protected]] >> Sent: Friday, February 23, 2018 4:33 PM >> To: Pankaj Bansal <[email protected]>; [email protected] >> Cc: Kinney, Michael D <[email protected]> >> Subject: RE: [RFC] MdePkg/BaseLib: Change BitField functions. >> >> Pankaj: >> OrData, AndData and Value are the bit field value specified by StartBit and >> EndBit. They are not the full value. They must be adjusted to the full value, >> then calculated with Operand. >> >> And, we use LShit() or RShit() function for 64bit operand, because we find >> VS compiler may generate the intrinsic function when >> or << is used 64bit >> operand on 32bit arch. >> >> Thanks >> Liming >>> -----Original Message----- >>> From: Pankaj Bansal [mailto:[email protected]] >>> Sent: Tuesday, February 20, 2018 12:50 PM >>> To: [email protected] >>> Cc: Gao, Liming <[email protected]>; Kinney, Michael D >>> <[email protected]> >>> Subject: RE: [RFC] MdePkg/BaseLib: Change BitField functions. >>> >>> Hi everybody, >>> >>> Any comments on this change ? >>> >>>> -----Original Message----- >>>> From: Pankaj Bansal >>>> Sent: Friday, February 09, 2018 4:30 PM >>>> To: [email protected] >>>> Cc: Pankaj Bansal <[email protected]>; Michael D Kinney >>>> <[email protected]>; Liming Gao <[email protected]> >>>> Subject: [RFC] MdePkg/BaseLib: Change BitField functions. >>>> >>>> The bit field functions in MdePkg are not working as expected. >>>> The restrictions on Value parameter such that Value should not be >>>> greater than the bitmask value range specified by StartBit and EndBit >>>> doesn't seem >>> to >>>> make sense. >>>> >>>> Also the restriction on End bit to not be equal to start bit >>>> prohibits single bit change. >>>> >>>> This is an attempt to correct these limitations. >>>> >>>> Cc: Michael D Kinney <[email protected]> >>>> Cc: Liming Gao <[email protected]> >>>> Contributed-under: TianoCore Contribution Agreement 1.1 >>>> Signed-off-by: Pankaj Bansal <[email protected]> >>>> --- >>>> >>>> Notes: >>>> This is a RFC patch. >>>> >>>> MdePkg/Library/BaseLib/BitField.c | 179 ++++++++++++++-------------- >>>> 1 file changed, 89 insertions(+), 90 deletions(-) >>>> >>>> diff --git a/MdePkg/Library/BaseLib/BitField.c >>>> b/MdePkg/Library/BaseLib/BitField.c >>>> index eb9e276..2b5be52 100644 >>>> --- a/MdePkg/Library/BaseLib/BitField.c >>>> +++ b/MdePkg/Library/BaseLib/BitField.c >>>> @@ -2,6 +2,8 @@ >>>> Bit field functions of BaseLib. >>>> >>>> Copyright (c) 2006 - 2013, Intel Corporation. All rights >>>> reserved.<BR> >>>> + Copyright 2018 NXP >>>> + >>>> This program and the accompanying materials >>>> are licensed and made available under the terms and conditions of >>>> the BSD License >>>> which accompanies this distribution. The full text of the license >>>> may be found at @@ -14,6 +16,12 @@ >>>> >>>> #include "BaseLibInternals.h" >>>> >>>> +#define BITMASK_UNITN(StartBit, EndBit) \ >>>> + (((MAX_UINTN) << (StartBit)) & (MAX_UINTN >> ((sizeof (UINTN) * >>>> +8) >>>> +- 1 - (EndBit)))) >>>> + >>>> +#define BITMASK_UNIT64(StartBit, EndBit) \ >>>> + (((MAX_UINT64) << (StartBit)) & (MAX_UINT64 >> ((sizeof (UINT64) >>>> +* >>>> +8) - 1 - (EndBit)))) >>>> + >>>> /** >>>> Worker function that returns a bit field from Operand. >>>> >>>> @@ -34,11 +42,9 @@ InternalBaseLibBitFieldReadUint ( >>>> IN UINTN EndBit >>>> ) >>>> { >>>> - // >>>> - // ~((UINTN)-2 << EndBit) is a mask in which bit[0] thru >>>> bit[EndBit] >>>> - // are 1's while bit[EndBit + 1] thru the most significant bit are 0's. >>>> - // >>>> - return (Operand & ~((UINTN)-2 << EndBit)) >> StartBit; >>>> + UINTN Mask = BITMASK_UNITN (StartBit, EndBit); >>>> + >>>> + return ( (Operand & Mask) >> StartBit); >>>> } >>>> >>>> /** >>>> @@ -68,19 +74,9 @@ InternalBaseLibBitFieldOrUint ( >>>> IN UINTN OrData >>>> ) >>>> { >>>> - // >>>> - // Higher bits in OrData those are not used must be zero. >>>> - // >>>> - // EndBit - StartBit + 1 might be 32 while the result right >>>> shifting 32 on a 32bit integer is undefined, >>>> - // So the logic is updated to right shift (EndBit - StartBit) bits >>>> and compare the last bit directly. >>>> - // >>>> - ASSERT ((OrData >> (EndBit - StartBit)) == ((OrData >> (EndBit - >>>> StartBit)) & 1)); >>>> - >>>> - // >>>> - // ~((UINTN)-2 << EndBit) is a mask in which bit[0] thru >>>> bit[EndBit] >>>> - // are 1's while bit[EndBit + 1] thru the most significant bit are 0's. >>>> - // >>>> - return Operand | ((OrData << StartBit) & ~((UINTN) -2 << EndBit)); >>>> + UINTN Mask = BITMASK_UNITN (StartBit, EndBit); >>>> + >>>> + return ( ( (Operand | OrData) & Mask) | (Operand & ~Mask)); >>>> } >>>> >>>> /** >>>> @@ -110,19 +106,38 @@ InternalBaseLibBitFieldAndUint ( >>>> IN UINTN AndData >>>> ) >>>> { >>>> - // >>>> - // Higher bits in AndData those are not used must be zero. >>>> - // >>>> - // EndBit - StartBit + 1 might be 32 while the result right >>>> shifting 32 on a 32bit integer is undefined, >>>> - // So the logic is updated to right shift (EndBit - StartBit) bits >>>> and compare the last bit directly. >>>> - // >>>> - ASSERT ((AndData >> (EndBit - StartBit)) == ((AndData >> (EndBit - >>> StartBit)) >>>> & 1)); >>>> - >>>> - // >>>> - // ~((UINTN)-2 << EndBit) is a mask in which bit[0] thru >>>> bit[EndBit] >>>> - // are 1's while bit[EndBit + 1] thru the most significant bit are 0's. >>>> - // >>>> - return Operand & ~((~AndData << StartBit) & ~((UINTN)-2 << >>>> EndBit)); >>>> + UINTN Mask = BITMASK_UNITN (StartBit, EndBit); >>>> + >>>> + return ( ( (Operand & AndData) & Mask) | (Operand & ~Mask)); } >>>> + >>>> +/** >>>> + Worker function that writes a bit field to an value, and returns the >> result. >>>> + >>>> + Writes Value to the bit field specified by the StartBit and the >>>> + EndBit in Operand. All other bits in Operand are preserved. The >>>> + new 8-bit value is returned. >>>> + >>>> + @param Operand Operand on which to perform the bitfield >> operation. >>>> + @param StartBit The ordinal of the least significant bit in the bit >>>> field. >>>> + @param EndBit The ordinal of the most significant bit in the bit >>>> field. >>>> + @param Value The new value of the bit field. >>>> + >>>> + @return The new value. >>>> + >>>> +**/ >>>> +UINTN >>>> +EFIAPI >>>> +InternalBaseLibBitFieldWriteUint ( >>>> + IN UINTN Operand, >>>> + IN UINTN StartBit, >>>> + IN UINTN EndBit, >>>> + IN UINTN Value >>>> + ) >>>> +{ >>>> + UINTN Mask = BITMASK_UNITN (StartBit, EndBit); >>>> + >>>> + return ( (Value & Mask) | (Operand & ~Mask)); >>>> } >>>> >>>> /** >>>> @@ -153,7 +168,7 @@ BitFieldRead8 ( >>>> ) >>>> { >>>> ASSERT (EndBit < 8); >>>> - ASSERT (StartBit <= EndBit); >>>> + ASSERT (StartBit < EndBit); >>>> return (UINT8)InternalBaseLibBitFieldReadUint (Operand, StartBit, >>>> EndBit); } >>>> >>>> @@ -190,8 +205,8 @@ BitFieldWrite8 ( >>>> ) >>>> { >>>> ASSERT (EndBit < 8); >>>> - ASSERT (StartBit <= EndBit); >>>> - return BitFieldAndThenOr8 (Operand, StartBit, EndBit, 0, Value); >>>> + ASSERT (StartBit < EndBit); >>>> + return (UINT8)InternalBaseLibBitFieldWriteUint (Operand, StartBit, >>>> + EndBit, >>>> Value); >>>> } >>>> >>>> /** >>>> @@ -228,7 +243,7 @@ BitFieldOr8 ( >>>> ) >>>> { >>>> ASSERT (EndBit < 8); >>>> - ASSERT (StartBit <= EndBit); >>>> + ASSERT (StartBit < EndBit); >>>> return (UINT8)InternalBaseLibBitFieldOrUint (Operand, StartBit, >>>> EndBit, OrData); } >>>> >>>> @@ -266,7 +281,7 @@ BitFieldAnd8 ( >>>> ) >>>> { >>>> ASSERT (EndBit < 8); >>>> - ASSERT (StartBit <= EndBit); >>>> + ASSERT (StartBit < EndBit); >>>> return (UINT8)InternalBaseLibBitFieldAndUint (Operand, StartBit, >>>> EndBit, AndData); } >>>> >>>> @@ -275,7 +290,7 @@ BitFieldAnd8 ( >>>> bitwise OR, and returns the result. >>>> >>>> Performs a bitwise AND between the bit field specified by StartBit >>>> and EndBit >>>> - in Operand and the value specified by AndData, followed by a >>>> bitwise >>>> + in Operand and the value specified by AndData, followed by a >>>> + bitwise >>>> OR with value specified by OrData. All other bits in Operand are >>>> preserved. The new 8-bit value is returned. >>>> >>>> @@ -308,7 +323,7 @@ BitFieldAndThenOr8 ( >>>> ) >>>> { >>>> ASSERT (EndBit < 8); >>>> - ASSERT (StartBit <= EndBit); >>>> + ASSERT (StartBit < EndBit); >>>> return BitFieldOr8 ( >>>> BitFieldAnd8 (Operand, StartBit, EndBit, AndData), >>>> StartBit, >>>> @@ -345,7 +360,7 @@ BitFieldRead16 ( >>>> ) >>>> { >>>> ASSERT (EndBit < 16); >>>> - ASSERT (StartBit <= EndBit); >>>> + ASSERT (StartBit < EndBit); >>>> return (UINT16)InternalBaseLibBitFieldReadUint (Operand, StartBit, >>> EndBit); >>>> } >>>> >>>> @@ -382,8 +397,8 @@ BitFieldWrite16 ( >>>> ) >>>> { >>>> ASSERT (EndBit < 16); >>>> - ASSERT (StartBit <= EndBit); >>>> - return BitFieldAndThenOr16 (Operand, StartBit, EndBit, 0, Value); >>>> + ASSERT (StartBit < EndBit); >>>> + return (UINT16)InternalBaseLibBitFieldWriteUint (Operand, >>>> + StartBit, >>>> EndBit, Value); >>>> } >>>> >>>> /** >>>> @@ -420,7 +435,7 @@ BitFieldOr16 ( >>>> ) >>>> { >>>> ASSERT (EndBit < 16); >>>> - ASSERT (StartBit <= EndBit); >>>> + ASSERT (StartBit < EndBit); >>>> return (UINT16)InternalBaseLibBitFieldOrUint (Operand, StartBit, >>>> EndBit, OrData); } >>>> >>>> @@ -458,7 +473,7 @@ BitFieldAnd16 ( >>>> ) >>>> { >>>> ASSERT (EndBit < 16); >>>> - ASSERT (StartBit <= EndBit); >>>> + ASSERT (StartBit < EndBit); >>>> return (UINT16)InternalBaseLibBitFieldAndUint (Operand, StartBit, >>>> EndBit, AndData); } >>>> >>>> @@ -467,7 +482,7 @@ BitFieldAnd16 ( >>>> bitwise OR, and returns the result. >>>> >>>> Performs a bitwise AND between the bit field specified by StartBit >>>> and EndBit >>>> - in Operand and the value specified by AndData, followed by a >>>> bitwise >>>> + in Operand and the value specified by AndData, followed by a >>>> + bitwise >>>> OR with value specified by OrData. All other bits in Operand are >>>> preserved. The new 16-bit value is returned. >>>> >>>> @@ -500,7 +515,7 @@ BitFieldAndThenOr16 ( >>>> ) >>>> { >>>> ASSERT (EndBit < 16); >>>> - ASSERT (StartBit <= EndBit); >>>> + ASSERT (StartBit < EndBit); >>>> return BitFieldOr16 ( >>>> BitFieldAnd16 (Operand, StartBit, EndBit, AndData), >>>> StartBit, >>>> @@ -537,7 +552,7 @@ BitFieldRead32 ( >>>> ) >>>> { >>>> ASSERT (EndBit < 32); >>>> - ASSERT (StartBit <= EndBit); >>>> + ASSERT (StartBit < EndBit); >>>> return (UINT32)InternalBaseLibBitFieldReadUint (Operand, StartBit, >>> EndBit); >>>> } >>>> >>>> @@ -574,8 +589,8 @@ BitFieldWrite32 ( >>>> ) >>>> { >>>> ASSERT (EndBit < 32); >>>> - ASSERT (StartBit <= EndBit); >>>> - return BitFieldAndThenOr32 (Operand, StartBit, EndBit, 0, Value); >>>> + ASSERT (StartBit < EndBit); >>>> + return (UINT32)InternalBaseLibBitFieldWriteUint (Operand, >>>> + StartBit, >>>> EndBit, Value); >>>> } >>>> >>>> /** >>>> @@ -612,7 +627,7 @@ BitFieldOr32 ( >>>> ) >>>> { >>>> ASSERT (EndBit < 32); >>>> - ASSERT (StartBit <= EndBit); >>>> + ASSERT (StartBit < EndBit); >>>> return (UINT32)InternalBaseLibBitFieldOrUint (Operand, StartBit, >>>> EndBit, OrData); } >>>> >>>> @@ -650,7 +665,7 @@ BitFieldAnd32 ( >>>> ) >>>> { >>>> ASSERT (EndBit < 32); >>>> - ASSERT (StartBit <= EndBit); >>>> + ASSERT (StartBit < EndBit); >>>> return (UINT32)InternalBaseLibBitFieldAndUint (Operand, StartBit, >>>> EndBit, AndData); } >>>> >>>> @@ -659,7 +674,7 @@ BitFieldAnd32 ( >>>> bitwise OR, and returns the result. >>>> >>>> Performs a bitwise AND between the bit field specified by StartBit >>>> and EndBit >>>> - in Operand and the value specified by AndData, followed by a >>>> bitwise >>>> + in Operand and the value specified by AndData, followed by a >>>> + bitwise >>>> OR with value specified by OrData. All other bits in Operand are >>>> preserved. The new 32-bit value is returned. >>>> >>>> @@ -692,7 +707,7 @@ BitFieldAndThenOr32 ( >>>> ) >>>> { >>>> ASSERT (EndBit < 32); >>>> - ASSERT (StartBit <= EndBit); >>>> + ASSERT (StartBit < EndBit); >>>> return BitFieldOr32 ( >>>> BitFieldAnd32 (Operand, StartBit, EndBit, AndData), >>>> StartBit, >>>> @@ -729,8 +744,11 @@ BitFieldRead64 ( >>>> ) >>>> { >>>> ASSERT (EndBit < 64); >>>> - ASSERT (StartBit <= EndBit); >>>> - return RShiftU64 (Operand & ~LShiftU64 ((UINT64)-2, EndBit), >>>> StartBit); >>>> + ASSERT (StartBit < EndBit); >>>> + >>>> + UINT64 Mask = BITMASK_UNIT64 (StartBit, EndBit); >>>> + >>>> + return ( (Operand & Mask) >> StartBit); >>>> } >>>> >>>> /** >>>> @@ -766,8 +784,11 @@ BitFieldWrite64 ( >>>> ) >>>> { >>>> ASSERT (EndBit < 64); >>>> - ASSERT (StartBit <= EndBit); >>>> - return BitFieldAndThenOr64 (Operand, StartBit, EndBit, 0, Value); >>>> + ASSERT (StartBit < EndBit); >>>> + >>>> + UINT64 Mask = BITMASK_UNIT64 (StartBit, EndBit); >>>> + >>>> + return ( (Value & Mask) | (Operand & ~Mask)); >>>> } >>>> >>>> /** >>>> @@ -803,23 +824,12 @@ BitFieldOr64 ( >>>> IN UINT64 OrData >>>> ) >>>> { >>>> - UINT64 Value1; >>>> - UINT64 Value2; >>>> - >>>> ASSERT (EndBit < 64); >>>> - ASSERT (StartBit <= EndBit); >>>> - // >>>> - // Higher bits in OrData those are not used must be zero. >>>> - // >>>> - // EndBit - StartBit + 1 might be 64 while the result right >>>> shifting 64 on >>>> RShiftU64() API is invalid, >>>> - // So the logic is updated to right shift (EndBit - StartBit) bits >>>> and compare the last bit directly. >>>> - // >>>> - ASSERT (RShiftU64 (OrData, EndBit - StartBit) == (RShiftU64 >>>> (OrData, >>> EndBit >>>> - StartBit) & 1)); >>>> - >>>> - Value1 = LShiftU64 (OrData, StartBit); >>>> - Value2 = LShiftU64 ((UINT64) - 2, EndBit); >>>> - >>>> - return Operand | (Value1 & ~Value2); >>>> + ASSERT (StartBit < EndBit); >>>> + >>>> + UINT64 Mask = BITMASK_UNIT64 (StartBit, EndBit); >>>> + >>>> + return ( ( (Operand | OrData) & Mask) | (Operand & ~Mask)); >>>> } >>>> >>>> /** >>>> @@ -855,23 +865,12 @@ BitFieldAnd64 ( >>>> IN UINT64 AndData >>>> ) >>>> { >>>> - UINT64 Value1; >>>> - UINT64 Value2; >>>> - >>>> ASSERT (EndBit < 64); >>>> - ASSERT (StartBit <= EndBit); >>>> - // >>>> - // Higher bits in AndData those are not used must be zero. >>>> - // >>>> - // EndBit - StartBit + 1 might be 64 while the right shifting 64 >>>> on RShiftU64() API is invalid, >>>> - // So the logic is updated to right shift (EndBit - StartBit) bits >>>> and compare the last bit directly. >>>> - // >>>> - ASSERT (RShiftU64 (AndData, EndBit - StartBit) == (RShiftU64 >>>> (AndData, EndBit - StartBit) & 1)); >>>> - >>>> - Value1 = LShiftU64 (~AndData, StartBit); >>>> - Value2 = LShiftU64 ((UINT64)-2, EndBit); >>>> - >>>> - return Operand & ~(Value1 & ~Value2); >>>> + ASSERT (StartBit < EndBit); >>>> + >>>> + UINT64 Mask = BITMASK_UNIT64 (StartBit, EndBit); >>>> + >>>> + return ( ( (Operand & AndData) & Mask) | (Operand & ~Mask)); >>>> } >>>> >>>> /** >>>> @@ -879,7 +878,7 @@ BitFieldAnd64 ( >>>> bitwise OR, and returns the result. >>>> >>>> Performs a bitwise AND between the bit field specified by StartBit >>>> and EndBit >>>> - in Operand and the value specified by AndData, followed by a >>>> bitwise >>>> + in Operand and the value specified by AndData, followed by a >>>> + bitwise >>>> OR with value specified by OrData. All other bits in Operand are >>>> preserved. The new 64-bit value is returned. >>>> >>>> @@ -912,7 +911,7 @@ BitFieldAndThenOr64 ( >>>> ) >>>> { >>>> ASSERT (EndBit < 64); >>>> - ASSERT (StartBit <= EndBit); >>>> + ASSERT (StartBit < EndBit); >>>> return BitFieldOr64 ( >>>> BitFieldAnd64 (Operand, StartBit, EndBit, AndData), >>>> StartBit, >>>> -- >>>> 2.7.4 > > _______________________________________________ > edk2-devel mailing list > [email protected] > https://lists.01.org/mailman/listinfo/edk2-devel > _______________________________________________ edk2-devel mailing list [email protected] https://lists.01.org/mailman/listinfo/edk2-devel
