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

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

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