Re: [Xen-devel] [for-4.7 1/2] xen/bitops: Introduce macros to generate mask

[Julien Grall <julien.grall@xxxxxxx>]

Hi Andrew,

On 13/04/2016 19:14, Andrew Cooper wrote:
> On 13/04/16 16:55, Julien Grall wrote:
> > The code has been imported from the header include/linux/bitops.h in
> > Linux v4.6-rc3.
> >
> > Signed-off-by: Julien Grall <julien.grall@xxxxxxx>
> >
> > ---
> > Cc: Ian Jackson <ian.jackson@xxxxxxxxxxxxx>
> > Cc: Jan Beulich <jbeulich@xxxxxxxx>
> > Cc: Keir Fraser <keir@xxxxxxx>
> > Cc: Tim Deegan <tim@xxxxxxx>
> > ---
> >   xen/include/xen/bitops.h | 11 +++++++++++
> >   1 file changed, 11 insertions(+)
> >
> > diff --git a/xen/include/xen/bitops.h b/xen/include/xen/bitops.h
> > index cb56f24..e1a4d93 100644
> > --- a/xen/include/xen/bitops.h
> > +++ b/xen/include/xen/bitops.h
> > @@ -3,6 +3,17 @@
> >   #include <asm/types.h>
> >
> >   /*
> > + * Create a contiguous bitmask starting at bit position @l and ending at
> > + * position @h. For example
> > + * GENMASK_ULL(39, 21) gives us the 64bit vector 0x000000ffffe00000.
> > + */
> > +#define GENMASK(h, l) \
> > +       (((~0UL) << (l)) & (~0UL >> (BITS_PER_LONG - 1 - (h))))
> > +
> > +#define GENMASK_ULL(h, l) \
> > +       (((~0ULL) << (l)) & (~0ULL >> (BITS_PER_LONG_LONG - 1 - (h))))
> You should have just a single GENMASK() which works in terms of LL.
>
> Masks must be signed to work correctly when implicitly extended.
May I ask you why? AFAICT, if the mask is signed, it will result to 
duplicate the sign bit to the new bits. This is not the expected 
behavior of this macro.
For instance, the following patch is using this macro to mask RES0 bits 
in the HPFAR_EL2 register. For ARM, RES0 means the bit is currently read 
as zero but the software should not rely on it to preserve forward 
compatibility.
Regards,

--
Julien Grall

_______________________________________________
Xen-devel mailing list
Xen-devel@xxxxxxxxxxxxx
http://lists.xen.org/xen-devel