| Message ID | 1466097133-5489-2-git-send-email-dgilbert@redhat.com |
|---|---|
| State | New |
| Headers | show |
On 16/06/2016 19:12, Dr. David Alan Gilbert (git) wrote: > From: "Dr. David Alan Gilbert" <dgilbert@redhat.com> > > e.g. BIT_RANGE(15, 0) gives 0xff00 That looks more like BIT_RANGE(15, 8). :) Paolo > Suggested by: Paolo Bonzini <pbonzini@redhat.com> > Signed-off-by: Dr. David Alan Gilbert <dgilbert@redhat.com> > --- > include/qemu/bitops.h | 3 +++ > 1 file changed, 3 insertions(+) > > diff --git a/include/qemu/bitops.h b/include/qemu/bitops.h > index 755fdd1..e411688 100644 > --- a/include/qemu/bitops.h > +++ b/include/qemu/bitops.h > @@ -23,6 +23,9 @@ > #define BIT_MASK(nr) (1UL << ((nr) % BITS_PER_LONG)) > #define BIT_WORD(nr) ((nr) / BITS_PER_LONG) > #define BITS_TO_LONGS(nr) DIV_ROUND_UP(nr, BITS_PER_BYTE * sizeof(long)) > +/* e.g. BIT_RANGE(15, 0) -> 0xff00 */ > +#define BIT_RANGE(hb, lb) ((2ull << (hb)) - (1ull << (lb))) > + >
* Paolo Bonzini (pbonzini@redhat.com) wrote: > > > On 16/06/2016 19:12, Dr. David Alan Gilbert (git) wrote: > > From: "Dr. David Alan Gilbert" <dgilbert@redhat.com> > > > > e.g. BIT_RANGE(15, 0) gives 0xff00 > > That looks more like BIT_RANGE(15, 8). :) Yes (and in the comment below) - I'll repost after waiting to see if there's any other nasties. Dave > > Paolo > > > Suggested by: Paolo Bonzini <pbonzini@redhat.com> > > Signed-off-by: Dr. David Alan Gilbert <dgilbert@redhat.com> > > --- > > include/qemu/bitops.h | 3 +++ > > 1 file changed, 3 insertions(+) > > > > diff --git a/include/qemu/bitops.h b/include/qemu/bitops.h > > index 755fdd1..e411688 100644 > > --- a/include/qemu/bitops.h > > +++ b/include/qemu/bitops.h > > @@ -23,6 +23,9 @@ > > #define BIT_MASK(nr) (1UL << ((nr) % BITS_PER_LONG)) > > #define BIT_WORD(nr) ((nr) / BITS_PER_LONG) > > #define BITS_TO_LONGS(nr) DIV_ROUND_UP(nr, BITS_PER_BYTE * sizeof(long)) > > +/* e.g. BIT_RANGE(15, 0) -> 0xff00 */ > > +#define BIT_RANGE(hb, lb) ((2ull << (hb)) - (1ull << (lb))) > > + > > -- Dr. David Alan Gilbert / dgilbert@redhat.com / Manchester, UK
On 16 June 2016 at 18:12, Dr. David Alan Gilbert (git) <dgilbert@redhat.com> wrote: > From: "Dr. David Alan Gilbert" <dgilbert@redhat.com> > > e.g. BIT_RANGE(15, 0) gives 0xff00 > > Suggested by: Paolo Bonzini <pbonzini@redhat.com> > Signed-off-by: Dr. David Alan Gilbert <dgilbert@redhat.com> > --- > include/qemu/bitops.h | 3 +++ > 1 file changed, 3 insertions(+) > > diff --git a/include/qemu/bitops.h b/include/qemu/bitops.h > index 755fdd1..e411688 100644 > --- a/include/qemu/bitops.h > +++ b/include/qemu/bitops.h > @@ -23,6 +23,9 @@ > #define BIT_MASK(nr) (1UL << ((nr) % BITS_PER_LONG)) > #define BIT_WORD(nr) ((nr) / BITS_PER_LONG) > #define BITS_TO_LONGS(nr) DIV_ROUND_UP(nr, BITS_PER_BYTE * sizeof(long)) > +/* e.g. BIT_RANGE(15, 0) -> 0xff00 */ > +#define BIT_RANGE(hb, lb) ((2ull << (hb)) - (1ull << (lb))) Isn't this undefined behaviour if the hb is 63? Also there is semantic overlap with the MAKE_64BIT_MASK macro proposed in https://lists.gnu.org/archive/html/qemu-devel/2016-05/msg02154.html (which also has ub, but see https://lists.gnu.org/archive/html/qemu-devel/2016-06/msg02614.html for the version which doesn't). I prefer a "start, length" macro to "position, position", because this matches what we already have for the deposit and extract functions in this header. thanks -- PMM
On 16/06/2016 20:01, Peter Maydell wrote: >> diff --git a/include/qemu/bitops.h b/include/qemu/bitops.h >> index 755fdd1..e411688 100644 >> --- a/include/qemu/bitops.h >> +++ b/include/qemu/bitops.h >> @@ -23,6 +23,9 @@ >> #define BIT_MASK(nr) (1UL << ((nr) % BITS_PER_LONG)) >> #define BIT_WORD(nr) ((nr) / BITS_PER_LONG) >> #define BITS_TO_LONGS(nr) DIV_ROUND_UP(nr, BITS_PER_BYTE * sizeof(long)) >> +/* e.g. BIT_RANGE(15, 0) -> 0xff00 */ >> +#define BIT_RANGE(hb, lb) ((2ull << (hb)) - (1ull << (lb))) > > Isn't this undefined behaviour if the hb is 63? No, these are unsigned values so there's no undefined behavior. > I prefer a "start, length" macro to "position, position", > because this matches what we already have for the deposit > and extract functions in this header. That's fine too, albeit in the case of this patch the code would be uglier. Also, if you want a "start, length" mask you could always deposit -1 into 0... Paolo
* Peter Maydell (peter.maydell@linaro.org) wrote: > On 16 June 2016 at 18:12, Dr. David Alan Gilbert (git) > <dgilbert@redhat.com> wrote: > > From: "Dr. David Alan Gilbert" <dgilbert@redhat.com> > > > > e.g. BIT_RANGE(15, 0) gives 0xff00 > > > > Suggested by: Paolo Bonzini <pbonzini@redhat.com> > > Signed-off-by: Dr. David Alan Gilbert <dgilbert@redhat.com> > > --- > > include/qemu/bitops.h | 3 +++ > > 1 file changed, 3 insertions(+) > > > > diff --git a/include/qemu/bitops.h b/include/qemu/bitops.h > > index 755fdd1..e411688 100644 > > --- a/include/qemu/bitops.h > > +++ b/include/qemu/bitops.h > > @@ -23,6 +23,9 @@ > > #define BIT_MASK(nr) (1UL << ((nr) % BITS_PER_LONG)) > > #define BIT_WORD(nr) ((nr) / BITS_PER_LONG) > > #define BITS_TO_LONGS(nr) DIV_ROUND_UP(nr, BITS_PER_BYTE * sizeof(long)) > > +/* e.g. BIT_RANGE(15, 0) -> 0xff00 */ > > +#define BIT_RANGE(hb, lb) ((2ull << (hb)) - (1ull << (lb))) > > Isn't this undefined behaviour if the hb is 63? I've checked the C99 spec; it explicitly defines the unsigned behaviour ('reduced modulo one more than the maximum value representable in the result type'). > Also there is semantic overlap with the MAKE_64BIT_MASK macro > proposed in https://lists.gnu.org/archive/html/qemu-devel/2016-05/msg02154.html > (which also has ub, but see > https://lists.gnu.org/archive/html/qemu-devel/2016-06/msg02614.html > for the version which doesn't). > > I prefer a "start, length" macro to "position, position", > because this matches what we already have for the deposit > and extract functions in this header. I think it depends on the use; I agree that makes sense for things like extracting an n-bit integer; in this case what we have is something which is fixed at bit 51 and another bit - we dont ever think about the difference between those two bits. Dave > > thanks > -- PMM -- Dr. David Alan Gilbert / dgilbert@redhat.com / Manchester, UK
On 20 June 2016 at 15:11, Dr. David Alan Gilbert <dgilbert@redhat.com> wrote: > * Peter Maydell (peter.maydell@linaro.org) wrote: >> I prefer a "start, length" macro to "position, position", >> because this matches what we already have for the deposit >> and extract functions in this header. > > I think it depends on the use; I agree that makes sense > for things like extracting an n-bit integer; in this case > what we have is something which is fixed at bit 51 and > another bit - we dont ever think about the difference between > those two bits. Well, sure, sometimes device descriptions define fields in registers as "from bit X to bit Y", but we don't have two versions of extract32(). We've already settled on using "start, length" for other operations in this header, so I think we should continue in that vein, not have some things using "start, length" and others using "start, end". thanks -- PMM
diff --git a/include/qemu/bitops.h b/include/qemu/bitops.h index 755fdd1..e411688 100644 --- a/include/qemu/bitops.h +++ b/include/qemu/bitops.h @@ -23,6 +23,9 @@ #define BIT_MASK(nr) (1UL << ((nr) % BITS_PER_LONG)) #define BIT_WORD(nr) ((nr) / BITS_PER_LONG) #define BITS_TO_LONGS(nr) DIV_ROUND_UP(nr, BITS_PER_BYTE * sizeof(long)) +/* e.g. BIT_RANGE(15, 0) -> 0xff00 */ +#define BIT_RANGE(hb, lb) ((2ull << (hb)) - (1ull << (lb))) + /** * set_bit - Set a bit in memory