Re: [RFC] MdePkg/BaseLib: Change BitField functions.

[Laszlo Ersek <lersek@redhat.com>]

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.gao@intel.com]
>> Sent: Friday, February 23, 2018 4:33 PM
>> To: Pankaj Bansal <pankaj.bansal@nxp.com>; edk2-devel@lists.01.org
>> Cc: Kinney, Michael D <michael.d.kinney@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.bansal@nxp.com]
>>> Sent: Tuesday, February 20, 2018 12:50 PM
>>> To: edk2-devel@lists.01.org
>>> Cc: Gao, Liming <liming.gao@intel.com>; Kinney, Michael D
>>> <michael.d.kinney@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.bansal@nxp.com>; Michael D Kinney
>>>> <michael.d.kinney@intel.com>; Liming Gao <liming.gao@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.kinney@intel.com>
>>>> Cc: Liming Gao <liming.gao@intel.com>
>>>> Contributed-under: TianoCore Contribution Agreement 1.1
>>>> Signed-off-by: Pankaj Bansal <pankaj.bansal@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://lists.01.org/mailman/listinfo/edk2-devel
>