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

[Pankaj Bansal <pankaj.bansal@nxp.com>]

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 ?

Thanks & Regards,
Pankaj Bansal

> -----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