Hello Laszlo, Thanks for your explanation. Now I understand the usage of bit field functions.
Regards, Pankaj Bansal > -----Original Message----- > From: Laszlo Ersek [mailto:ler...@redhat.com] > Sent: Friday, February 23, 2018 5:43 PM > To: Pankaj Bansal <pankaj.ban...@nxp.com>; Gao, Liming > <liming....@intel.com>; edk2-devel@lists.01.org > Cc: Kinney, Michael D <michael.d.kin...@intel.com> > Subject: Re: [edk2] [RFC] MdePkg/BaseLib: Change BitField functions. > > 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:liming....@intel.com] > >> Sent: Friday, February 23, 2018 4:33 PM > >> To: Pankaj Bansal <pankaj.ban...@nxp.com>; edk2-devel@lists.01.org > >> Cc: Kinney, Michael D <michael.d.kin...@intel.com> > >> 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:pankaj.ban...@nxp.com] > >>> Sent: Tuesday, February 20, 2018 12:50 PM > >>> To: edk2-devel@lists.01.org > >>> Cc: Gao, Liming <liming....@intel.com>; Kinney, Michael D > >>> <michael.d.kin...@intel.com> > >>> 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: edk2-devel@lists.01.org > >>>> Cc: Pankaj Bansal <pankaj.ban...@nxp.com>; Michael D Kinney > >>>> <michael.d.kin...@intel.com>; Liming Gao <liming....@intel.com> > >>>> 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 <michael.d.kin...@intel.com> > >>>> Cc: Liming Gao <liming....@intel.com> > >>>> Contributed-under: TianoCore Contribution Agreement 1.1 > >>>> Signed-off-by: Pankaj Bansal <pankaj.ban...@nxp.com> > >>>> --- > >>>> > >>>> 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 > > edk2-devel@lists.01.org > > https://emea01.safelinks.protection.outlook.com/?url=https%3A%2F%2Flis > > ts.01.org%2Fmailman%2Flistinfo%2Fedk2- > devel&data=02%7C01%7Cpankaj.bans > > > al%40nxp.com%7Ced613e79935445554a2608d57ab6c6cd%7C686ea1d3bc2b > 4c6fa92c > > > d99c5c301635%7C0%7C0%7C636549847796367463&sdata=RyFpJ4kcJnCDCOS > D103AWe > > 8cOycTiDBRBYlCFsF9cCE%3D&reserved=0 > > _______________________________________________ edk2-devel mailing list edk2-devel@lists.01.org https://lists.01.org/mailman/listinfo/edk2-devel
