From mboxrd@z Thu Jan 1 00:00:00 1970 Return-Path: Received-SPF: Pass (sender SPF authorized) identity=mailfrom; client-ip=66.187.233.73; helo=mx1.redhat.com; envelope-from=lersek@redhat.com; receiver=edk2-devel@lists.01.org Received: from mx1.redhat.com (mx3-rdu2.redhat.com [66.187.233.73]) (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 A206822436936 for ; Fri, 23 Feb 2018 04:06:55 -0800 (PST) Received: from smtp.corp.redhat.com (int-mx03.intmail.prod.int.rdu2.redhat.com [10.11.54.3]) (using TLSv1.2 with cipher AECDH-AES256-SHA (256/256 bits)) (No client certificate requested) by mx1.redhat.com (Postfix) with ESMTPS id 91540814DF58; Fri, 23 Feb 2018 12:12:56 +0000 (UTC) Received: from lacos-laptop-7.usersys.redhat.com (ovpn-120-112.rdu2.redhat.com [10.10.120.112]) by smtp.corp.redhat.com (Postfix) with ESMTP id 1B9241049475; Fri, 23 Feb 2018 12:12:54 +0000 (UTC) To: Pankaj Bansal , "Gao, Liming" , "edk2-devel@lists.01.org" Cc: "Kinney, Michael D" References: <1518173996-26906-1-git-send-email-pankaj.bansal@nxp.com> <4A89E2EF3DFEDB4C8BFDE51014F606A14E1CEF81@SHSMSX104.ccr.corp.intel.com> From: Laszlo Ersek Message-ID: <48d7d1a9-503f-2a72-803d-f63a8bc67e9c@redhat.com> Date: Fri, 23 Feb 2018 13:12:54 +0100 User-Agent: Mozilla/5.0 (X11; Linux x86_64; rv:52.0) Gecko/20100101 Thunderbird/52.6.0 MIME-Version: 1.0 In-Reply-To: X-Scanned-By: MIMEDefang 2.78 on 10.11.54.3 X-Greylist: Sender IP whitelisted, not delayed by milter-greylist-4.5.16 (mx1.redhat.com [10.11.55.8]); Fri, 23 Feb 2018 12:12:56 +0000 (UTC) X-Greylist: inspected by milter-greylist-4.5.16 (mx1.redhat.com [10.11.55.8]); Fri, 23 Feb 2018 12:12:56 +0000 (UTC) for IP:'10.11.54.3' DOMAIN:'int-mx03.intmail.prod.int.rdu2.redhat.com' HELO:'smtp.corp.redhat.com' FROM:'lersek@redhat.com' RCPT:'' 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 12:06:56 -0000 Content-Type: text/plain; charset=utf-8 Content-Language: en-US Content-Transfer-Encoding: 7bit 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 ; edk2-devel@lists.01.org >> Cc: Kinney, Michael D >> 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 ; 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 >>>> 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 >>>> 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 >>>> 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 >