MdePkg/BaseLib: Change BitField functions.

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

Posted by Pankaj Bansal 6 years, 2 months ago

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

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

Posted by Pankaj Bansal 6 years, 1 month ago

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

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

Posted by Gao, Liming 6 years, 1 month ago

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

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

Posted by Pankaj Bansal 6 years, 1 month ago

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

_______________________________________________
edk2-devel mailing list
edk2-devel@lists.01.org
https://lists.01.org/mailman/listinfo/edk2-devel

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

Posted by Laszlo Ersek 6 years, 1 month ago

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
> 

_______________________________________________
edk2-devel mailing list
edk2-devel@lists.01.org
https://lists.01.org/mailman/listinfo/edk2-devel

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

Posted by Pankaj Bansal 6 years, 1 month ago

Hello Laszlo,

Thanks for your explanation.
Now I understand the usage of bit field functions.

Regards,
Pankaj Bansal

> -----Original Message-----
> From: Laszlo Ersek [mailto:lersek@redhat.com]
> Sent: Friday, February 23, 2018 5:43 PM
> To: Pankaj Bansal <pankaj.bansal@nxp.com>; Gao, Liming
> <liming.gao@intel.com>; edk2-devel@lists.01.org
> Cc: Kinney, Michael D <michael.d.kinney@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.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://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