| Message ID | 1340792953-29804-1-git-send-email-peter.maydell@linaro.org |
|---|---|
| State | New |
| Headers | show |
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
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().
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
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
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.
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.
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
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.
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.
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.
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.
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.
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
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(-)