From mboxrd@z Thu Jan 1 00:00:00 1970 Return-Path: Received-SPF: Pass (sender SPF authorized) identity=mailfrom; client-ip=134.134.136.100; helo=mga07.intel.com; envelope-from=liming.gao@intel.com; receiver=edk2-devel@lists.01.org Received: from mga07.intel.com (mga07.intel.com [134.134.136.100]) (using TLSv1.2 with cipher ECDHE-RSA-AES256-GCM-SHA384 (256/256 bits)) (No client certificate requested) by ml01.01.org (Postfix) with ESMTPS id 8E0E622436937 for ; Fri, 23 Feb 2018 02:56:55 -0800 (PST) X-Amp-Result: SKIPPED(no attachment in message) X-Amp-File-Uploaded: False Received: from orsmga006.jf.intel.com ([10.7.209.51]) by orsmga105.jf.intel.com with ESMTP/TLS/DHE-RSA-AES256-GCM-SHA384; 23 Feb 2018 03:02:56 -0800 X-ExtLoop1: 1 X-IronPort-AV: E=Sophos;i="5.47,383,1515484800"; d="scan'208";a="20507723" Received: from fmsmsx106.amr.corp.intel.com ([10.18.124.204]) by orsmga006.jf.intel.com with ESMTP; 23 Feb 2018 03:02:56 -0800 Received: from fmsmsx155.amr.corp.intel.com (10.18.116.71) by FMSMSX106.amr.corp.intel.com (10.18.124.204) with Microsoft SMTP Server (TLS) id 14.3.319.2; Fri, 23 Feb 2018 03:02:56 -0800 Received: from shsmsx152.ccr.corp.intel.com (10.239.6.52) by FMSMSX155.amr.corp.intel.com (10.18.116.71) with Microsoft SMTP Server (TLS) id 14.3.319.2; Fri, 23 Feb 2018 03:02:55 -0800 Received: from shsmsx104.ccr.corp.intel.com ([169.254.5.125]) by SHSMSX152.ccr.corp.intel.com ([169.254.6.130]) with mapi id 14.03.0319.002; Fri, 23 Feb 2018 19:02:53 +0800 From: "Gao, Liming" To: Pankaj Bansal , "edk2-devel@lists.01.org" CC: "Kinney, Michael D" Thread-Topic: [RFC] MdePkg/BaseLib: Change BitField functions. Thread-Index: AQHToZVRshsJmy+6ekq8srRNWTe1b6OsQzmAgAWhLNA= Date: Fri, 23 Feb 2018 11:02:53 +0000 Message-ID: <4A89E2EF3DFEDB4C8BFDE51014F606A14E1CEF81@SHSMSX104.ccr.corp.intel.com> References: <1518173996-26906-1-git-send-email-pankaj.bansal@nxp.com> In-Reply-To: Accept-Language: en-US X-MS-Has-Attach: X-MS-TNEF-Correlator: x-originating-ip: [10.239.127.40] MIME-Version: 1.0 Subject: Re: [RFC] MdePkg/BaseLib: Change BitField functions. X-BeenThere: edk2-devel@lists.01.org X-Mailman-Version: 2.1.23 Precedence: list List-Id: EDK II Development List-Unsubscribe: , List-Archive: List-Post: List-Help: List-Subscribe: , X-List-Received-Date: Fri, 23 Feb 2018 10:56:56 -0000 Content-Language: en-US Content-Type: text/plain; charset="us-ascii" Content-Transfer-Encoding: quoted-printable Pankaj: OrData, AndData and Value are the bit field value specified by StartBit a= nd EndBit. They are not the full value. They must be adjusted to the full v= alue, then calculated with Operand. And, we use LShit() or RShit() function for 64bit operand, because we fin= d VS compiler may generate the intrinsic function when >> or << is used 64b= it operand on 32bit arch.=20 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 ; Kinney, Michael D > >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 ; Michael D Kinney >> ; Liming Gao >> 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 greate= r >> than the bitmask value range specified by StartBit and EndBit doesn't se= em >to >> make sense. >> >> Also the restriction on End bit to not be equal to start bit prohibits s= ingle bit >> change. >> >> This is an attempt to correct these limitations. >> >> Cc: Michael D Kinney >> Cc: Liming Gao >> Contributed-under: TianoCore Contribution Agreement 1.1 >> Signed-off-by: Pankaj Bansal >> --- >> >> 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. >> + 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 ma= y 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 =3D 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 an= d compare >> the last bit directly. >> - // >> - ASSERT ((OrData >> (EndBit - StartBit)) =3D=3D ((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 =3D 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 an= d compare >> the last bit directly. >> - // >> - ASSERT ((AndData >> (EndBit - StartBit)) =3D=3D ((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 =3D 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 EndBi= t 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 =3D BITMASK_UNITN (StartBit, EndBit); >> + >> + return ( (Value & Mask) | (Operand & ~Mask)); >> } >> >> /** >> @@ -153,7 +168,7 @@ BitFieldRead8 ( >> ) >> { >> ASSERT (EndBit < 8); >> - ASSERT (StartBit <=3D EndBit); >> + ASSERT (StartBit < EndBit); >> return (UINT8)InternalBaseLibBitFieldReadUint (Operand, StartBit, End= Bit); >> } >> >> @@ -190,8 +205,8 @@ BitFieldWrite8 ( >> ) >> { >> ASSERT (EndBit < 8); >> - ASSERT (StartBit <=3D EndBit); >> - return BitFieldAndThenOr8 (Operand, StartBit, EndBit, 0, Value); >> + ASSERT (StartBit < EndBit); >> + return (UINT8)InternalBaseLibBitFieldWriteUint (Operand, StartBit, En= dBit, >> Value); >> } >> >> /** >> @@ -228,7 +243,7 @@ BitFieldOr8 ( >> ) >> { >> ASSERT (EndBit < 8); >> - ASSERT (StartBit <=3D EndBit); >> + ASSERT (StartBit < EndBit); >> return (UINT8)InternalBaseLibBitFieldOrUint (Operand, StartBit, EndBi= t, >> OrData); >> } >> >> @@ -266,7 +281,7 @@ BitFieldAnd8 ( >> ) >> { >> ASSERT (EndBit < 8); >> - ASSERT (StartBit <=3D EndBit); >> + ASSERT (StartBit < EndBit); >> return (UINT8)InternalBaseLibBitFieldAndUint (Operand, StartBit, EndB= it, >> AndData); >> } >> >> @@ -275,7 +290,7 @@ BitFieldAnd8 ( >> bitwise OR, and returns the result. >> >> Performs a bitwise AND between the bit field specified by StartBit an= d >> 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 <=3D EndBit); >> + ASSERT (StartBit < EndBit); >> return BitFieldOr8 ( >> BitFieldAnd8 (Operand, StartBit, EndBit, AndData), >> StartBit, >> @@ -345,7 +360,7 @@ BitFieldRead16 ( >> ) >> { >> ASSERT (EndBit < 16); >> - ASSERT (StartBit <=3D EndBit); >> + ASSERT (StartBit < EndBit); >> return (UINT16)InternalBaseLibBitFieldReadUint (Operand, StartBit, >EndBit); >> } >> >> @@ -382,8 +397,8 @@ BitFieldWrite16 ( >> ) >> { >> ASSERT (EndBit < 16); >> - ASSERT (StartBit <=3D 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 <=3D EndBit); >> + ASSERT (StartBit < EndBit); >> return (UINT16)InternalBaseLibBitFieldOrUint (Operand, StartBit, EndB= it, >> OrData); >> } >> >> @@ -458,7 +473,7 @@ BitFieldAnd16 ( >> ) >> { >> ASSERT (EndBit < 16); >> - ASSERT (StartBit <=3D EndBit); >> + ASSERT (StartBit < EndBit); >> return (UINT16)InternalBaseLibBitFieldAndUint (Operand, StartBit, End= Bit, >> AndData); >> } >> >> @@ -467,7 +482,7 @@ BitFieldAnd16 ( >> bitwise OR, and returns the result. >> >> Performs a bitwise AND between the bit field specified by StartBit an= d >> 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 <=3D EndBit); >> + ASSERT (StartBit < EndBit); >> return BitFieldOr16 ( >> BitFieldAnd16 (Operand, StartBit, EndBit, AndData), >> StartBit, >> @@ -537,7 +552,7 @@ BitFieldRead32 ( >> ) >> { >> ASSERT (EndBit < 32); >> - ASSERT (StartBit <=3D EndBit); >> + ASSERT (StartBit < EndBit); >> return (UINT32)InternalBaseLibBitFieldReadUint (Operand, StartBit, >EndBit); >> } >> >> @@ -574,8 +589,8 @@ BitFieldWrite32 ( >> ) >> { >> ASSERT (EndBit < 32); >> - ASSERT (StartBit <=3D 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 <=3D EndBit); >> + ASSERT (StartBit < EndBit); >> return (UINT32)InternalBaseLibBitFieldOrUint (Operand, StartBit, EndB= it, >> OrData); >> } >> >> @@ -650,7 +665,7 @@ BitFieldAnd32 ( >> ) >> { >> ASSERT (EndBit < 32); >> - ASSERT (StartBit <=3D EndBit); >> + ASSERT (StartBit < EndBit); >> return (UINT32)InternalBaseLibBitFieldAndUint (Operand, StartBit, End= Bit, >> AndData); >> } >> >> @@ -659,7 +674,7 @@ BitFieldAnd32 ( >> bitwise OR, and returns the result. >> >> Performs a bitwise AND between the bit field specified by StartBit an= d >> 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 <=3D EndBit); >> + ASSERT (StartBit < EndBit); >> return BitFieldOr32 ( >> BitFieldAnd32 (Operand, StartBit, EndBit, AndData), >> StartBit, >> @@ -729,8 +744,11 @@ BitFieldRead64 ( >> ) >> { >> ASSERT (EndBit < 64); >> - ASSERT (StartBit <=3D EndBit); >> - return RShiftU64 (Operand & ~LShiftU64 ((UINT64)-2, EndBit), StartBit= ); >> + ASSERT (StartBit < EndBit); >> + >> + UINT64 Mask =3D BITMASK_UNIT64 (StartBit, EndBit); >> + >> + return ( (Operand & Mask) >> StartBit); >> } >> >> /** >> @@ -766,8 +784,11 @@ BitFieldWrite64 ( >> ) >> { >> ASSERT (EndBit < 64); >> - ASSERT (StartBit <=3D EndBit); >> - return BitFieldAndThenOr64 (Operand, StartBit, EndBit, 0, Value); >> + ASSERT (StartBit < EndBit); >> + >> + UINT64 Mask =3D 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 <=3D 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 an= d compare >> the last bit directly. >> - // >> - ASSERT (RShiftU64 (OrData, EndBit - StartBit) =3D=3D (RShiftU64 (OrDa= ta, >EndBit >> - StartBit) & 1)); >> - >> - Value1 =3D LShiftU64 (OrData, StartBit); >> - Value2 =3D LShiftU64 ((UINT64) - 2, EndBit); >> - >> - return Operand | (Value1 & ~Value2); >> + ASSERT (StartBit < EndBit); >> + >> + UINT64 Mask =3D 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 <=3D EndBit); >> - // >> - // Higher bits in AndData those are not used must be zero. >> - // >> - // EndBit - StartBit + 1 might be 64 while the right shifting 64 on R= ShiftU64() >> API is invalid, >> - // So the logic is updated to right shift (EndBit - StartBit) bits an= d compare >> the last bit directly. >> - // >> - ASSERT (RShiftU64 (AndData, EndBit - StartBit) =3D=3D (RShiftU64 (And= Data, >> EndBit - StartBit) & 1)); >> - >> - Value1 =3D LShiftU64 (~AndData, StartBit); >> - Value2 =3D LShiftU64 ((UINT64)-2, EndBit); >> - >> - return Operand & ~(Value1 & ~Value2); >> + ASSERT (StartBit < EndBit); >> + >> + UINT64 Mask =3D 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 an= d >> 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 <=3D EndBit); >> + ASSERT (StartBit < EndBit); >> return BitFieldOr64 ( >> BitFieldAnd64 (Operand, StartBit, EndBit, AndData), >> StartBit, >> -- >> 2.7.4