Patchwork [v2] bitops.h: Add field32() and field64() functions to extract bitfields

login
register
mail settings
Submitter Peter Maydell
Date June 27, 2012, 10:29 a.m.
Message ID <1340792953-29804-1-git-send-email-peter.maydell@linaro.org>
Download mbox | patch
Permalink /patch/167600/
State New
Headers show

Comments

Peter Maydell - June 27, 2012, 10:29 a.m.
Add field32() and field64() functions which extract a particular
bit field from a word and return it. Based on an idea by Jia Liu.

Suggested-by: Jia Liu <proljc@gmail.com>
Signed-off-by: Peter Maydell <peter.maydell@linaro.org>
---
v1->v2: added missing brackets to field32() to bring it in to line
        with field64()
(Still using 'int' rather than 'unsigned' for bit numbers as per
rationale in previous discussion.)

 bitops.h |   28 ++++++++++++++++++++++++++++
 1 files changed, 28 insertions(+), 0 deletions(-)
Andreas Färber - June 27, 2012, 11:29 a.m.
Am 27.06.2012 12:29, schrieb Peter Maydell:
> Add field32() and field64() functions which extract a particular
> bit field from a word and return it. Based on an idea by Jia Liu.
> 
> Suggested-by: Jia Liu <proljc@gmail.com>
> Signed-off-by: Peter Maydell <peter.maydell@linaro.org>
> ---
> v1->v2: added missing brackets to field32() to bring it in to line
>         with field64()
> (Still using 'int' rather than 'unsigned' for bit numbers as per
> rationale in previous discussion.)

Reviewed-by: Andreas Färber <afaerber@suse.de>

Do you have followup patches that make use of this? Might illustrate
what variables and types are being passed in.

/-F

> 
>  bitops.h |   28 ++++++++++++++++++++++++++++
>  1 files changed, 28 insertions(+), 0 deletions(-)
> 
> diff --git a/bitops.h b/bitops.h
> index 07d1a06..ffbb387 100644
> --- a/bitops.h
> +++ b/bitops.h
> @@ -269,4 +269,32 @@ static inline unsigned long hweight_long(unsigned long w)
>      return count;
>  }
>  
> +/**
> + * field64 - return a specified bit field from a uint64_t value
> + * @value: The value to extract the bit field from
> + * @start: The lowest bit in the bit field (numbered from 0)
> + * @length: The length of the bit field
> + *
> + * Returns the value of the bit field extracted from the input value.
> + */
> +static inline uint64_t field64(uint64_t value, int start, int length)
> +{
> +    assert(start >= 0 && start <= 63 && length > 0 && start + length <= 64);
> +    return (value >> start) & (~0ULL >> (64 - length));
> +}
> +
> +/**
> + * field32 - return a specified bit field from a uint32_t value
> + * @value: The value to extract the bit field from
> + * @start: The lowest bit in the bit field (numbered from 0)
> + * @length: The length of the bit field
> + *
> + * Returns the value of the bit field extracted from the input value.
> + */
> +static inline uint32_t field32(uint32_t value, int start, int length)
> +{
> +    assert(start >= 0 && start <= 31 && length > 0 && start + length <= 32);
> +    return (value >> start) & (~0U >> (32 - length));
> +}
> +
>  #endif
Eric Blake - June 27, 2012, 11:39 a.m.
On 06/27/2012 04:29 AM, Peter Maydell wrote:
> Add field32() and field64() functions which extract a particular
> bit field from a word and return it. Based on an idea by Jia Liu.
> 

> +static inline uint64_t field64(uint64_t value, int start, int length)
> +{
> +    assert(start >= 0 && start <= 63 && length > 0 && start + length <= 64);

You're failing to account for wraparound:

field64(value, 63, MAX_INT)

gives undefined behavior in the addition, and even on (most) platforms
where it silently wraps around to a negative number, you have then
missed triggering the assert and proceed to do more unefined behavior
with a negative shift.  You can solve that, and use one less conjunct,
by using:

assert(start >= 0 && length > 0 && (unsigned) start + length <= 64);

Same comment for field32().
Peter Maydell - June 27, 2012, 1:01 p.m.
On 27 June 2012 12:29, Andreas Färber <afaerber@suse.de> wrote:
> Do you have followup patches that make use of this? Might illustrate
> what variables and types are being passed in.

Here's a random snippet from the LPAE patch I'm working on:

    uint32_t t0sz = field32(env->cp15.c2_control, 0, 3);
    uint32_t t1sz = field32(env->cp15.c2_control, 16, 3);

I expect that in almost all cases the size and length values
will just be constants.

-- PMM
Peter Maydell - June 27, 2012, 1:07 p.m.
On 27 June 2012 12:39, Eric Blake <eblake@redhat.com> wrote:
> On 06/27/2012 04:29 AM, Peter Maydell wrote:
>> +static inline uint64_t field64(uint64_t value, int start, int length)
>> +{
>> +    assert(start >= 0 && start <= 63 && length > 0 && start + length <= 64);
>
> You're failing to account for wraparound:
>
> field64(value, 63, MAX_INT)
>
> gives undefined behavior in the addition, and even on (most) platforms
> where it silently wraps around to a negative number, you have then
> missed triggering the assert and proceed to do more unefined behavior
> with a negative shift.  You can solve that, and use one less conjunct,
> by using:
>
> assert(start >= 0 && length > 0 && (unsigned) start + length <= 64);

Yes, that works (took me a minute to figure out that it relies on
two positive ints not being able to overflow an unsigned int).

-- PMM
Avi Kivity - June 27, 2012, 1:15 p.m.
On 06/27/2012 01:29 PM, Peter Maydell wrote:
> Add field32() and field64() functions which extract a particular
> bit field from a word and return it. Based on an idea by Jia Liu.
> 
>  
> +/**
> + * field64 - return a specified bit field from a uint64_t value
> + * @value: The value to extract the bit field from
> + * @start: The lowest bit in the bit field (numbered from 0)
> + * @length: The length of the bit field
> + *
> + * Returns the value of the bit field extracted from the input value.
> + */
> +static inline uint64_t field64(uint64_t value, int start, int length)
> +{
> +    assert(start >= 0 && start <= 63 && length > 0 && start + length <= 64);
> +    return (value >> start) & (~0ULL >> (64 - length));
> +}
> +

Undefined for length == 64, so better add that to the assert.

I suggest adding the analogous functions for writing.  I believe the
common naming is extract/deposit.

static inline uint64_t deposit64(uint64_t *pvalue, unsigned start,
                                 unsigned length, uint64_t fieldval)
{
    uint64_t mask = (((uint64_t)1 << length) - 1) << start;
    *pvalue = (*pvalue & ~mask) | ((fieldval << start) & mask);
}

Useful for setting a bit to a specific value.
Avi Kivity - June 27, 2012, 1:20 p.m.
On 06/27/2012 04:15 PM, Avi Kivity wrote:
> On 06/27/2012 01:29 PM, Peter Maydell wrote:
>> Add field32() and field64() functions which extract a particular
>> bit field from a word and return it. Based on an idea by Jia Liu.
>> 
>>  
>> +/**
>> + * field64 - return a specified bit field from a uint64_t value
>> + * @value: The value to extract the bit field from
>> + * @start: The lowest bit in the bit field (numbered from 0)
>> + * @length: The length of the bit field
>> + *
>> + * Returns the value of the bit field extracted from the input value.
>> + */
>> +static inline uint64_t field64(uint64_t value, int start, int length)
>> +{
>> +    assert(start >= 0 && start <= 63 && length > 0 && start + length <= 64);
>> +    return (value >> start) & (~0ULL >> (64 - length));
>> +}
>> +
> 
> Undefined for length == 64, so better add that to the assert.

Er, please ignore.  It's undefined for length == 0, but that's
uninteresting and protected by the assert.
Peter Maydell - June 27, 2012, 1:22 p.m.
On 27 June 2012 14:15, Avi Kivity <avi@redhat.com> wrote:
> I suggest adding the analogous functions for writing.  I believe the
> common naming is extract/deposit.
>
> static inline uint64_t deposit64(uint64_t *pvalue, unsigned start,
>                                 unsigned length, uint64_t fieldval)
> {
>    uint64_t mask = (((uint64_t)1 << length) - 1) << start;
>    *pvalue = (*pvalue & ~mask) | ((fieldval << start) & mask);
> }
>
> Useful for setting a bit to a specific value.

Do you have a use case in mind for this one?

-- PMM
Eric Blake - June 27, 2012, 1:28 p.m.
On 06/27/2012 07:15 AM, Avi Kivity wrote:
> On 06/27/2012 01:29 PM, Peter Maydell wrote:
>> Add field32() and field64() functions which extract a particular
>> bit field from a word and return it. Based on an idea by Jia Liu.
>>

>> +static inline uint64_t field64(uint64_t value, int start, int length)
>> +{
>> +    assert(start >= 0 && start <= 63 && length > 0 && start + length <= 64);
>> +    return (value >> start) & (~0ULL >> (64 - length));
>> +}
>> +
> 
> Undefined for length == 64, so better add that to the assert.

How so?  ~0ULL >> 0 is well-defined (a length of 64, coupled with a
start of 0, is effectively writing the entire uint64_t value).  While it
is unlikely that anyone would ever trigger this with start and length
being constants (field64(value, 0, 64) == value), it might be triggered
when start and length are computed from other code.

> 
> I suggest adding the analogous functions for writing.  I believe the
> common naming is extract/deposit.
> 
> static inline uint64_t deposit64(uint64_t *pvalue, unsigned start,
>                                  unsigned length, uint64_t fieldval)
> {
>     uint64_t mask = (((uint64_t)1 << length) - 1) << start;

Here, a length of 64 is indeed undefined, but for symmetry with allowing
a full 64-bit extraction from a start of bit 0, you could special case
the computation of that mask.

>     *pvalue = (*pvalue & ~mask) | ((fieldval << start) & mask);

As with the extraction function, it would be worth an assert that start
and length don't trigger undefined shifts.  It may be further worth
asserting that fieldval is within the range given by length, although I
could see cases of intentionally wanting to pass in -1 to set all bits
within the mask and a tight assert would prevent that usage.

You marked this function signature as returning uint64_t, but didn't
return anything.  I think the logical return is the new contents of
*pvalue.  Another logical choice would be marking the function void.

> 
> Useful for setting a bit to a specific value.

Indeed, having the pair of functions may be handy.
Avi Kivity - June 27, 2012, 1:30 p.m.
On 06/27/2012 04:22 PM, Peter Maydell wrote:
> On 27 June 2012 14:15, Avi Kivity <avi@redhat.com> wrote:
>> I suggest adding the analogous functions for writing.  I believe the
>> common naming is extract/deposit.
>>
>> static inline uint64_t deposit64(uint64_t *pvalue, unsigned start,
>>                                 unsigned length, uint64_t fieldval)
>> {
>>    uint64_t mask = (((uint64_t)1 << length) - 1) << start;
>>    *pvalue = (*pvalue & ~mask) | ((fieldval << start) & mask);
>> }
>>
>> Useful for setting a bit to a specific value.
> 
> Do you have a use case in mind for this one?

A fast grep:

hw/alpha_typhoon.c-    if (level) {
hw/alpha_typhoon.c:        drir |= 1ull << irq;
hw/alpha_typhoon.c-    } else {
hw/alpha_typhoon.c:        drir &= ~(1ull << irq);
hw/alpha_typhoon.c-    }

hw/cbus.c-    if (state)
hw/cbus.c:        s->status &= ~(1 << 5);
hw/cbus.c-    else
hw/cbus.c:        s->status |= 1 << 5;

hw/dma.c-    case 0x09:
hw/dma.c-        ichan = data & 3;
hw/dma.c-        if (data & 4) {
hw/dma.c:            d->status |= 1 << (ichan + 4);
hw/dma.c-        }
hw/dma.c-        else {
hw/dma.c:            d->status &= ~(1 << (ichan + 4));
hw/dma.c-        }
hw/dma.c:        d->status &= ~(1 << ichan);
hw/dma.c-        DMA_run();
hw/dma.c-        break;
hw/dma.c-
hw/dma.c-    case 0x0a:                  /* single mask */
hw/dma.c-        if (data & 4)
hw/dma.c:            d->mask |= 1 << (data & 3);
hw/dma.c-        else
hw/dma.c:            d->mask &= ~(1 << (data & 3));
hw/dma.c-        DMA_run();

hw/exynos4210_combiner.c-    if (level) {
hw/exynos4210_combiner.c:        s->group[group_n].src_pending |= 1 << bit_n;
hw/exynos4210_combiner.c-    } else {
hw/exynos4210_combiner.c:        s->group[group_n].src_pending &= ~(1 << bit_n);
hw/exynos4210_combiner.c-    }
hw/exynos4210_combiner.c-

I stopped here, but I'm sure there are plenty others.  Bitfields are common in hardware.
Avi Kivity - June 27, 2012, 2:01 p.m.
On 06/27/2012 04:28 PM, Eric Blake wrote:
> On 06/27/2012 07:15 AM, Avi Kivity wrote:
>> On 06/27/2012 01:29 PM, Peter Maydell wrote:
>>> Add field32() and field64() functions which extract a particular
>>> bit field from a word and return it. Based on an idea by Jia Liu.
>>>
> 
>>> +static inline uint64_t field64(uint64_t value, int start, int length)
>>> +{
>>> +    assert(start >= 0 && start <= 63 && length > 0 && start + length <= 64);
>>> +    return (value >> start) & (~0ULL >> (64 - length));
>>> +}
>>> +
>> 
>> Undefined for length == 64, so better add that to the assert.
> 
> How so?  ~0ULL >> 0 is well-defined (a length of 64, coupled with a
> start of 0, is effectively writing the entire uint64_t value).  While it
> is unlikely that anyone would ever trigger this with start and length
> being constants (field64(value, 0, 64) == value), it might be triggered
> when start and length are computed from other code.

I was confused with length == 0.

> 
>> 
>> I suggest adding the analogous functions for writing.  I believe the
>> common naming is extract/deposit.
>> 
>> static inline uint64_t deposit64(uint64_t *pvalue, unsigned start,
>>                                  unsigned length, uint64_t fieldval)
>> {
>>     uint64_t mask = (((uint64_t)1 << length) - 1) << start;
> 
> Here, a length of 64 is indeed undefined, but for symmetry with allowing
> a full 64-bit extraction from a start of bit 0, you could special case
> the computation of that mask.
> 
>>     *pvalue = (*pvalue & ~mask) | ((fieldval << start) & mask);
> 
> As with the extraction function, it would be worth an assert that start
> and length don't trigger undefined shifts.  It may be further worth
> asserting that fieldval is within the range given by length, although I
> could see cases of intentionally wanting to pass in -1 to set all bits
> within the mask and a tight assert would prevent that usage.
> 
> You marked this function signature as returning uint64_t, but didn't
> return anything.  I think the logical return is the new contents of
> *pvalue.  Another logical choice would be marking the function void.
> 

Void makes more sense here as you usually want in-place update.
Markus Armbruster - June 27, 2012, 2:24 p.m.
Avi Kivity <avi@redhat.com> writes:

> On 06/27/2012 04:28 PM, Eric Blake wrote:
[...]
>> You marked this function signature as returning uint64_t, but didn't
>> return anything.  I think the logical return is the new contents of
>> *pvalue.  Another logical choice would be marking the function void.
>> 
>
> Void makes more sense here as you usually want in-place update.

It's an assignment, so I'd follow the precedence of C's assignment
operator and return the new value of the left hand side.
Avi Kivity - June 27, 2012, 2:37 p.m.
On 06/27/2012 05:24 PM, Markus Armbruster wrote:
> Avi Kivity <avi@redhat.com> writes:
> 
>> On 06/27/2012 04:28 PM, Eric Blake wrote:
> [...]
>>> You marked this function signature as returning uint64_t, but didn't
>>> return anything.  I think the logical return is the new contents of
>>> *pvalue.  Another logical choice would be marking the function void.
>>> 
>>
>> Void makes more sense here as you usually want in-place update.
> 
> It's an assignment, so I'd follow the precedence of C's assignment
> operator and return the new value of the left hand side.

Yup.

Patch

diff --git a/bitops.h b/bitops.h
index 07d1a06..ffbb387 100644
--- a/bitops.h
+++ b/bitops.h
@@ -269,4 +269,32 @@  static inline unsigned long hweight_long(unsigned long w)
     return count;
 }
 
+/**
+ * field64 - return a specified bit field from a uint64_t value
+ * @value: The value to extract the bit field from
+ * @start: The lowest bit in the bit field (numbered from 0)
+ * @length: The length of the bit field
+ *
+ * Returns the value of the bit field extracted from the input value.
+ */
+static inline uint64_t field64(uint64_t value, int start, int length)
+{
+    assert(start >= 0 && start <= 63 && length > 0 && start + length <= 64);
+    return (value >> start) & (~0ULL >> (64 - length));
+}
+
+/**
+ * field32 - return a specified bit field from a uint32_t value
+ * @value: The value to extract the bit field from
+ * @start: The lowest bit in the bit field (numbered from 0)
+ * @length: The length of the bit field
+ *
+ * Returns the value of the bit field extracted from the input value.
+ */
+static inline uint32_t field32(uint32_t value, int start, int length)
+{
+    assert(start >= 0 && start <= 31 && length > 0 && start + length <= 32);
+    return (value >> start) & (~0U >> (32 - length));
+}
+
 #endif