diff mbox series

[v3,1/3] bits: introduce fixed-type genmasks

Message ID	20240208074521.577076-2-lucas.demarchi@intel.com
State	New
Headers	Received-SPF: pass (google.com: domain of linux-kernel+bounces-57568-ouuuleilei=gmail.com@vger.kernel.org designates 147.75.48.161 as permitted sender) client-ip=147.75.48.161; From: Lucas De Marchi <lucas.demarchi@intel.com> To: Yury Norov <yury.norov@gmail.com> Cc: linux-kernel@vger.kernel.org, dri-devel@lists.freedesktop.org, Andy Shevchenko <andriy.shevchenko@linux.intel.com>, Jani Nikula <jani.nikula@linux.intel.com>, intel-xe@lists.freedesktop.org, intel-gfx@lists.freedesktop.org, Lucas De Marchi <lucas.demarchi@intel.com>, Jani Nikula <jani.nikula@intel.com> Subject: [PATCH v3 1/3] bits: introduce fixed-type genmasks Date: Wed, 7 Feb 2024 23:45:19 -0800 Message-ID: <20240208074521.577076-2-lucas.demarchi@intel.com> In-Reply-To: <20240208074521.577076-1-lucas.demarchi@intel.com> References: <20240208074521.577076-1-lucas.demarchi@intel.com> Precedence: bulk MIME-Version: 1.0 Content-Transfer-Encoding: 8bit X-getmail-retrieved-from-mailbox: INBOX
Series	Fixed-type GENMASK/BIT \| [v3,0/3] Fixed-type GENMASK/BIT [v3,1/3] bits: introduce fixed-type genmasks [v3,2/3] bits: Introduce fixed-type BIT [v3,3/3] drm/i915: Convert REG_GENMASK* to fixed-width GENMASK_*

Commit Message

Lucas De Marchi Feb. 8, 2024, 7:45 a.m. UTC

  From: Yury Norov <yury.norov@gmail.com>

Generalize __GENMASK() to support different types, and implement
fixed-types versions of GENMASK() based on it. The fixed-type version
allows more strict checks to the min/max values accepted, which is
useful for defining registers like implemented by i915 and xe drivers
with their REG_GENMASK*() macros.

The strict checks rely on shift-count-overflow compiler check to
fail the build if a number outside of the range allowed is passed.
Example:

	#define FOO_MASK GENMASK_U32(33, 4)

will generate a warning like:

	../include/linux/bits.h:41:31: error: left shift count >= width of type [-Werror=shift-count-overflow]
	   41 |          (((t)~0ULL - ((t)(1) << (l)) + 1) & \
	      |                               ^~

Signed-off-by: Yury Norov <yury.norov@gmail.com>
Signed-off-by: Lucas De Marchi <lucas.demarchi@intel.com>
Acked-by: Jani Nikula <jani.nikula@intel.com>
---
 include/linux/bitops.h |  1 -
 include/linux/bits.h   | 32 ++++++++++++++++++++++----------
 2 files changed, 22 insertions(+), 11 deletions(-)

Comments

Andi Shyti Feb. 8, 2024, 7:48 p.m. UTC | #1

Hi Lucas and Yury,

On Wed, Feb 07, 2024 at 11:45:19PM -0800, Lucas De Marchi wrote:
> From: Yury Norov <yury.norov@gmail.com>
> 
> Generalize __GENMASK() to support different types, and implement
> fixed-types versions of GENMASK() based on it. The fixed-type version
> allows more strict checks to the min/max values accepted, which is
> useful for defining registers like implemented by i915 and xe drivers
> with their REG_GENMASK*() macros.
> 
> The strict checks rely on shift-count-overflow compiler check to
> fail the build if a number outside of the range allowed is passed.
> Example:
> 
> 	#define FOO_MASK GENMASK_U32(33, 4)
> 
> will generate a warning like:
> 
> 	../include/linux/bits.h:41:31: error: left shift count >= width of type [-Werror=shift-count-overflow]
> 	   41 |          (((t)~0ULL - ((t)(1) << (l)) + 1) & \
> 	      |                               ^~
> 
> Signed-off-by: Yury Norov <yury.norov@gmail.com>
> Signed-off-by: Lucas De Marchi <lucas.demarchi@intel.com>
> Acked-by: Jani Nikula <jani.nikula@intel.com>

Lucas' SoB should be at the bottom here. In any case, nice patch:

Reviewed-by: Andi Shyti <andi.shyti@linux.intel.com>

Thanks,
Andi

Andy Shevchenko Feb. 9, 2024, 2:04 p.m. UTC | #2

On Thu, Feb 08, 2024 at 08:48:59PM +0100, Andi Shyti wrote:
> On Wed, Feb 07, 2024 at 11:45:19PM -0800, Lucas De Marchi wrote:

..

> > Signed-off-by: Yury Norov <yury.norov@gmail.com>
> > Signed-off-by: Lucas De Marchi <lucas.demarchi@intel.com>
> > Acked-by: Jani Nikula <jani.nikula@intel.com>
> 
> Lucas' SoB should be at the bottom here. In any case, nice patch:

And it's at the bottom (among SoB lines), there is no violation with Submitting
Patches.

Dmitry Baryshkov Feb. 21, 2024, 8:30 p.m. UTC | #3

On Thu, 8 Feb 2024 at 09:45, Lucas De Marchi <lucas.demarchi@intel.com> wrote:
>
> From: Yury Norov <yury.norov@gmail.com>
>
> Generalize __GENMASK() to support different types, and implement
> fixed-types versions of GENMASK() based on it. The fixed-type version
> allows more strict checks to the min/max values accepted, which is
> useful for defining registers like implemented by i915 and xe drivers
> with their REG_GENMASK*() macros.
>
> The strict checks rely on shift-count-overflow compiler check to
> fail the build if a number outside of the range allowed is passed.
> Example:
>
>         #define FOO_MASK GENMASK_U32(33, 4)
>
> will generate a warning like:
>
>         ../include/linux/bits.h:41:31: error: left shift count >= width of type [-Werror=shift-count-overflow]
>            41 |          (((t)~0ULL - ((t)(1) << (l)) + 1) & \
>               |                               ^~
>
> Signed-off-by: Yury Norov <yury.norov@gmail.com>
> Signed-off-by: Lucas De Marchi <lucas.demarchi@intel.com>
> Acked-by: Jani Nikula <jani.nikula@intel.com>
> ---
>  include/linux/bitops.h |  1 -
>  include/linux/bits.h   | 32 ++++++++++++++++++++++----------
>  2 files changed, 22 insertions(+), 11 deletions(-)
>
> diff --git a/include/linux/bitops.h b/include/linux/bitops.h
> index 2ba557e067fe..1db50c69cfdb 100644
> --- a/include/linux/bitops.h
> +++ b/include/linux/bitops.h
> @@ -15,7 +15,6 @@
>  #  define aligned_byte_mask(n) (~0xffUL << (BITS_PER_LONG - 8 - 8*(n)))
>  #endif
>
> -#define BITS_PER_TYPE(type)    (sizeof(type) * BITS_PER_BYTE)
>  #define BITS_TO_LONGS(nr)      __KERNEL_DIV_ROUND_UP(nr, BITS_PER_TYPE(long))
>  #define BITS_TO_U64(nr)                __KERNEL_DIV_ROUND_UP(nr, BITS_PER_TYPE(u64))
>  #define BITS_TO_U32(nr)                __KERNEL_DIV_ROUND_UP(nr, BITS_PER_TYPE(u32))
> diff --git a/include/linux/bits.h b/include/linux/bits.h
> index 7c0cf5031abe..bd56f32de44e 100644
> --- a/include/linux/bits.h
> +++ b/include/linux/bits.h
> @@ -6,6 +6,8 @@
>  #include <vdso/bits.h>
>  #include <asm/bitsperlong.h>
>
> +#define BITS_PER_TYPE(type)    (sizeof(type) * BITS_PER_BYTE)
> +
>  #define BIT_MASK(nr)           (UL(1) << ((nr) % BITS_PER_LONG))
>  #define BIT_WORD(nr)           ((nr) / BITS_PER_LONG)
>  #define BIT_ULL_MASK(nr)       (ULL(1) << ((nr) % BITS_PER_LONG_LONG))
> @@ -30,16 +32,26 @@
>  #define GENMASK_INPUT_CHECK(h, l) 0
>  #endif
>
> -#define __GENMASK(h, l) \
> -       (((~UL(0)) - (UL(1) << (l)) + 1) & \
> -        (~UL(0) >> (BITS_PER_LONG - 1 - (h))))
> -#define GENMASK(h, l) \
> -       (GENMASK_INPUT_CHECK(h, l) + __GENMASK(h, l))
> +/*
> + * Generate a mask for the specified type @t. Additional checks are made to
> + * guarantee the value returned fits in that type, relying on
> + * shift-count-overflow compiler check to detect incompatible arguments.
> + * For example, all these create build errors or warnings:
> + *
> + * - GENMASK(15, 20): wrong argument order
> + * - GENMASK(72, 15): doesn't fit unsigned long
> + * - GENMASK_U32(33, 15): doesn't fit in a u32
> + */
> +#define __GENMASK(t, h, l) \
> +       (GENMASK_INPUT_CHECK(h, l) + \
> +        (((t)~0ULL - ((t)(1) << (l)) + 1) & \
> +        ((t)~0ULL >> (BITS_PER_TYPE(t) - 1 - (h)))))
>
> -#define __GENMASK_ULL(h, l) \
> -       (((~ULL(0)) - (ULL(1) << (l)) + 1) & \
> -        (~ULL(0) >> (BITS_PER_LONG_LONG - 1 - (h))))
> -#define GENMASK_ULL(h, l) \
> -       (GENMASK_INPUT_CHECK(h, l) + __GENMASK_ULL(h, l))
> +#define GENMASK(h, l)          __GENMASK(unsigned long,  h, l)
> +#define GENMASK_ULL(h, l)      __GENMASK(unsigned long long, h, l)
> +#define GENMASK_U8(h, l)       __GENMASK(u8,  h, l)
> +#define GENMASK_U16(h, l)      __GENMASK(u16, h, l)
> +#define GENMASK_U32(h, l)      __GENMASK(u32, h, l)
> +#define GENMASK_U64(h, l)      __GENMASK(u64, h, l)

This breaks drm-tip on arm64 architecture:

arch/arm64/kernel/entry-fpsimd.S: Assembler messages:
465arch/arm64/kernel/entry-fpsimd.S:271: Error: found 'l', expected: ')'
466arch/arm64/kernel/entry-fpsimd.S:66: Info: macro invoked from here
467arch/arm64/kernel/entry-fpsimd.S:271: Error: found 'l', expected: ')'
468arch/arm64/kernel/entry-fpsimd.S:66: Info: macro invoked from here
469arch/arm64/kernel/entry-fpsimd.S:271: Error: found 'l', expected: ')'
470arch/arm64/kernel/entry-fpsimd.S:66: Info: macro invoked from here
471arch/arm64/kernel/entry-fpsimd.S:271: Error: found 'l', expected: ')'
472arch/arm64/kernel/entry-fpsimd.S:66: Info: macro invoked from here
473arch/arm64/kernel/entry-fpsimd.S:271: Error: unexpected characters
following instruction at operand 3 -- `bic x2,x1,(0+(((unsigned
long)~0ULL-((unsigned long)(1)<<(0))+1)&((unsigned
long)~0ULL>>((sizeof(unsigned long)*8)-1-(3)))))'
474arch/arm64/kernel/entry-fpsimd.S:66: Info: macro invoked from here
475arch/arm64/kernel/entry-fpsimd.S:282: Error: found 'l', expected: ')'
476arch/arm64/kernel/entry-fpsimd.S:98: Info: macro invoked from here
477arch/arm64/kernel/entry-fpsimd.S:282: Error: found 'l', expected: ')'
478arch/arm64/kernel/entry-fpsimd.S:98: Info: macro invoked from here
479arch/arm64/kernel/entry-fpsimd.S:282: Error: found 'l', expected: ')'
480arch/arm64/kernel/entry-fpsimd.S:98: Info: macro invoked from here
481arch/arm64/kernel/entry-fpsimd.S:282: Error: found 'l', expected: ')'
482arch/arm64/kernel/entry-fpsimd.S:98: Info: macro invoked from here
483arch/arm64/kernel/entry-fpsimd.S:282: Error: unexpected characters
following instruction at operand 3 -- `bic x2,x1,(0+(((unsigned
long)~0ULL-((unsigned long)(1)<<(0))+1)&((unsigned
long)~0ULL>>((sizeof(unsigned long)*8)-1-(3)))))'
484arch/arm64/kernel/entry-fpsimd.S:98: Info: macro invoked from here

>
>  #endif /* __LINUX_BITS_H */
> --
> 2.43.0
>

Dmitry Baryshkov Feb. 21, 2024, 8:37 p.m. UTC | #4

On Wed, 21 Feb 2024 at 22:30, Dmitry Baryshkov
<dmitry.baryshkov@linaro.org> wrote:
>
> On Thu, 8 Feb 2024 at 09:45, Lucas De Marchi <lucas.demarchi@intel.com> wrote:
> >
> > From: Yury Norov <yury.norov@gmail.com>
> >
> > Generalize __GENMASK() to support different types, and implement
> > fixed-types versions of GENMASK() based on it. The fixed-type version
> > allows more strict checks to the min/max values accepted, which is
> > useful for defining registers like implemented by i915 and xe drivers
> > with their REG_GENMASK*() macros.
> >
> > The strict checks rely on shift-count-overflow compiler check to
> > fail the build if a number outside of the range allowed is passed.
> > Example:
> >
> >         #define FOO_MASK GENMASK_U32(33, 4)
> >
> > will generate a warning like:
> >
> >         ../include/linux/bits.h:41:31: error: left shift count >= width of type [-Werror=shift-count-overflow]
> >            41 |          (((t)~0ULL - ((t)(1) << (l)) + 1) & \
> >               |                               ^~
> >
> > Signed-off-by: Yury Norov <yury.norov@gmail.com>
> > Signed-off-by: Lucas De Marchi <lucas.demarchi@intel.com>
> > Acked-by: Jani Nikula <jani.nikula@intel.com>
> > ---
> >  include/linux/bitops.h |  1 -
> >  include/linux/bits.h   | 32 ++++++++++++++++++++++----------
> >  2 files changed, 22 insertions(+), 11 deletions(-)
> >
> > diff --git a/include/linux/bitops.h b/include/linux/bitops.h
> > index 2ba557e067fe..1db50c69cfdb 100644
> > --- a/include/linux/bitops.h
> > +++ b/include/linux/bitops.h
> > @@ -15,7 +15,6 @@
> >  #  define aligned_byte_mask(n) (~0xffUL << (BITS_PER_LONG - 8 - 8*(n)))
> >  #endif
> >
> > -#define BITS_PER_TYPE(type)    (sizeof(type) * BITS_PER_BYTE)
> >  #define BITS_TO_LONGS(nr)      __KERNEL_DIV_ROUND_UP(nr, BITS_PER_TYPE(long))
> >  #define BITS_TO_U64(nr)                __KERNEL_DIV_ROUND_UP(nr, BITS_PER_TYPE(u64))
> >  #define BITS_TO_U32(nr)                __KERNEL_DIV_ROUND_UP(nr, BITS_PER_TYPE(u32))
> > diff --git a/include/linux/bits.h b/include/linux/bits.h
> > index 7c0cf5031abe..bd56f32de44e 100644
> > --- a/include/linux/bits.h
> > +++ b/include/linux/bits.h
> > @@ -6,6 +6,8 @@
> >  #include <vdso/bits.h>
> >  #include <asm/bitsperlong.h>
> >
> > +#define BITS_PER_TYPE(type)    (sizeof(type) * BITS_PER_BYTE)
> > +
> >  #define BIT_MASK(nr)           (UL(1) << ((nr) % BITS_PER_LONG))
> >  #define BIT_WORD(nr)           ((nr) / BITS_PER_LONG)
> >  #define BIT_ULL_MASK(nr)       (ULL(1) << ((nr) % BITS_PER_LONG_LONG))
> > @@ -30,16 +32,26 @@
> >  #define GENMASK_INPUT_CHECK(h, l) 0
> >  #endif
> >
> > -#define __GENMASK(h, l) \
> > -       (((~UL(0)) - (UL(1) << (l)) + 1) & \
> > -        (~UL(0) >> (BITS_PER_LONG - 1 - (h))))
> > -#define GENMASK(h, l) \
> > -       (GENMASK_INPUT_CHECK(h, l) + __GENMASK(h, l))
> > +/*
> > + * Generate a mask for the specified type @t. Additional checks are made to
> > + * guarantee the value returned fits in that type, relying on
> > + * shift-count-overflow compiler check to detect incompatible arguments.
> > + * For example, all these create build errors or warnings:
> > + *
> > + * - GENMASK(15, 20): wrong argument order
> > + * - GENMASK(72, 15): doesn't fit unsigned long
> > + * - GENMASK_U32(33, 15): doesn't fit in a u32
> > + */
> > +#define __GENMASK(t, h, l) \
> > +       (GENMASK_INPUT_CHECK(h, l) + \
> > +        (((t)~0ULL - ((t)(1) << (l)) + 1) & \
> > +        ((t)~0ULL >> (BITS_PER_TYPE(t) - 1 - (h)))))
> >
> > -#define __GENMASK_ULL(h, l) \
> > -       (((~ULL(0)) - (ULL(1) << (l)) + 1) & \
> > -        (~ULL(0) >> (BITS_PER_LONG_LONG - 1 - (h))))
> > -#define GENMASK_ULL(h, l) \
> > -       (GENMASK_INPUT_CHECK(h, l) + __GENMASK_ULL(h, l))
> > +#define GENMASK(h, l)          __GENMASK(unsigned long,  h, l)
> > +#define GENMASK_ULL(h, l)      __GENMASK(unsigned long long, h, l)
> > +#define GENMASK_U8(h, l)       __GENMASK(u8,  h, l)
> > +#define GENMASK_U16(h, l)      __GENMASK(u16, h, l)
> > +#define GENMASK_U32(h, l)      __GENMASK(u32, h, l)
> > +#define GENMASK_U64(h, l)      __GENMASK(u64, h, l)
>
> This breaks drm-tip on arm64 architecture:
>

Excuse me, it seems a c&p from gitlab didn't work as expected.

arch/arm64/kernel/entry-fpsimd.S: Assembler messages:
arch/arm64/kernel/entry-fpsimd.S:271: Error: found 'l', expected: ')'
arch/arm64/kernel/entry-fpsimd.S:66:  Info: macro invoked from here
arch/arm64/kernel/entry-fpsimd.S:271: Error: found 'l', expected: ')'
arch/arm64/kernel/entry-fpsimd.S:66:  Info: macro invoked from here
arch/arm64/kernel/entry-fpsimd.S:271: Error: found 'l', expected: ')'
arch/arm64/kernel/entry-fpsimd.S:66:  Info: macro invoked from here
arch/arm64/kernel/entry-fpsimd.S:271: Error: found 'l', expected: ')'
arch/arm64/kernel/entry-fpsimd.S:66:  Info: macro invoked from here
arch/arm64/kernel/entry-fpsimd.S:271: Error: unexpected characters
following instruction at operand 3 -- `bic x2,x1,(0+(((unsigned
long)~0ULL-((unsigned long)(1)<<(0))+1)&((unsigned
long)~0ULL>>((sizeof(unsigned long)*8)-1-(3)))))'
arch/arm64/kernel/entry-fpsimd.S:66:  Info: macro invoked from here
arch/arm64/kernel/entry-fpsimd.S:282: Error: found 'l', expected: ')'
arch/arm64/kernel/entry-fpsimd.S:98:  Info: macro invoked from here
arch/arm64/kernel/entry-fpsimd.S:282: Error: found 'l', expected: ')'
arch/arm64/kernel/entry-fpsimd.S:98:  Info: macro invoked from here
arch/arm64/kernel/entry-fpsimd.S:282: Error: found 'l', expected: ')'
arch/arm64/kernel/entry-fpsimd.S:98:  Info: macro invoked from here
arch/arm64/kernel/entry-fpsimd.S:282: Error: found 'l', expected: ')'
arch/arm64/kernel/entry-fpsimd.S:98:  Info: macro invoked from here
arch/arm64/kernel/entry-fpsimd.S:282: Error: unexpected characters
following instruction at operand 3 -- `bic x2,x1,(0+(((unsigned
long)~0ULL-((unsigned long)(1)<<(0))+1)&((unsigned
long)~0ULL>>((sizeof(unsigned long)*8)-1-(3)))))'
arch/arm64/kernel/entry-fpsimd.S:98:  Info: macro invoked from here
make[4]: *** [scripts/Makefile.build:361:
arch/arm64/kernel/entry-fpsimd.o] Error 1
make[3]: *** [scripts/Makefile.build:481: arch/arm64/kernel] Error 2
make[2]: *** [scripts/Makefile.build:481: arch/arm64] Error 2
make[2]: *** Waiting for unfinished jobs...

Andy Shevchenko Feb. 21, 2024, 9:04 p.m. UTC | #5

On Wed, Feb 21, 2024 at 10:30:02PM +0200, Dmitry Baryshkov wrote:
> On Thu, 8 Feb 2024 at 09:45, Lucas De Marchi <lucas.demarchi@intel.com> wrote:

..

> > +#define BITS_PER_TYPE(type)    (sizeof(type) * BITS_PER_BYTE)

Can sizeof() be used in assembly?

..

> > -#define __GENMASK(h, l) \
> > -       (((~UL(0)) - (UL(1) << (l)) + 1) & \
> > -        (~UL(0) >> (BITS_PER_LONG - 1 - (h))))
> > -#define GENMASK(h, l) \
> > -       (GENMASK_INPUT_CHECK(h, l) + __GENMASK(h, l))

> > +#define __GENMASK(t, h, l) \
> > +       (GENMASK_INPUT_CHECK(h, l) + \
> > +        (((t)~0ULL - ((t)(1) << (l)) + 1) & \
> > +        ((t)~0ULL >> (BITS_PER_TYPE(t) - 1 - (h)))))

Nevertheless, the use ~0ULL is not proper assembly, this broke initial
implementation using UL() / ULL().


> > -#define __GENMASK_ULL(h, l) \
> > -       (((~ULL(0)) - (ULL(1) << (l)) + 1) & \
> > -        (~ULL(0) >> (BITS_PER_LONG_LONG - 1 - (h))))
> > -#define GENMASK_ULL(h, l) \
> > -       (GENMASK_INPUT_CHECK(h, l) + __GENMASK_ULL(h, l))

Ditto.

> > +#define GENMASK(h, l)          __GENMASK(unsigned long,  h, l)
> > +#define GENMASK_ULL(h, l)      __GENMASK(unsigned long long, h, l)
> > +#define GENMASK_U8(h, l)       __GENMASK(u8,  h, l)
> > +#define GENMASK_U16(h, l)      __GENMASK(u16, h, l)
> > +#define GENMASK_U32(h, l)      __GENMASK(u32, h, l)
> > +#define GENMASK_U64(h, l)      __GENMASK(u64, h, l)
> 
> This breaks drm-tip on arm64 architecture:
> 
> arch/arm64/kernel/entry-fpsimd.S: Assembler messages:
> 465arch/arm64/kernel/entry-fpsimd.S:271: Error: found 'l', expected: ')'
> 466arch/arm64/kernel/entry-fpsimd.S:66: Info: macro invoked from here
> 467arch/arm64/kernel/entry-fpsimd.S:271: Error: found 'l', expected: ')'
> 468arch/arm64/kernel/entry-fpsimd.S:66: Info: macro invoked from here
> 469arch/arm64/kernel/entry-fpsimd.S:271: Error: found 'l', expected: ')'
> 470arch/arm64/kernel/entry-fpsimd.S:66: Info: macro invoked from here
> 471arch/arm64/kernel/entry-fpsimd.S:271: Error: found 'l', expected: ')'
> 472arch/arm64/kernel/entry-fpsimd.S:66: Info: macro invoked from here
> 473arch/arm64/kernel/entry-fpsimd.S:271: Error: unexpected characters
> following instruction at operand 3 -- `bic x2,x1,(0+(((unsigned
> long)~0ULL-((unsigned long)(1)<<(0))+1)&((unsigned
> long)~0ULL>>((sizeof(unsigned long)*8)-1-(3)))))'
> 474arch/arm64/kernel/entry-fpsimd.S:66: Info: macro invoked from here
> 475arch/arm64/kernel/entry-fpsimd.S:282: Error: found 'l', expected: ')'
> 476arch/arm64/kernel/entry-fpsimd.S:98: Info: macro invoked from here
> 477arch/arm64/kernel/entry-fpsimd.S:282: Error: found 'l', expected: ')'
> 478arch/arm64/kernel/entry-fpsimd.S:98: Info: macro invoked from here
> 479arch/arm64/kernel/entry-fpsimd.S:282: Error: found 'l', expected: ')'
> 480arch/arm64/kernel/entry-fpsimd.S:98: Info: macro invoked from here
> 481arch/arm64/kernel/entry-fpsimd.S:282: Error: found 'l', expected: ')'
> 482arch/arm64/kernel/entry-fpsimd.S:98: Info: macro invoked from here
> 483arch/arm64/kernel/entry-fpsimd.S:282: Error: unexpected characters
> following instruction at operand 3 -- `bic x2,x1,(0+(((unsigned
> long)~0ULL-((unsigned long)(1)<<(0))+1)&((unsigned
> long)~0ULL>>((sizeof(unsigned long)*8)-1-(3)))))'
> 484arch/arm64/kernel/entry-fpsimd.S:98: Info: macro invoked from here

Andy Shevchenko Feb. 21, 2024, 9:06 p.m. UTC | #6

On Wed, Feb 21, 2024 at 10:37:30PM +0200, Dmitry Baryshkov wrote:
> On Wed, 21 Feb 2024 at 22:30, Dmitry Baryshkov
> <dmitry.baryshkov@linaro.org> wrote:

..

> Excuse me, it seems a c&p from gitlab didn't work as expected.

No problem, it's clear that the patch has not been properly tested and
obviously wrong. Has to be dropped. If somebody wants that, it will
be material for v6.10 cycle.

Andy Shevchenko Feb. 21, 2024, 9:07 p.m. UTC | #7

On Wed, Feb 21, 2024 at 11:06:10PM +0200, Andy Shevchenko wrote:
> On Wed, Feb 21, 2024 at 10:37:30PM +0200, Dmitry Baryshkov wrote:
> > On Wed, 21 Feb 2024 at 22:30, Dmitry Baryshkov
> > <dmitry.baryshkov@linaro.org> wrote:

..

> > Excuse me, it seems a c&p from gitlab didn't work as expected.
> 
> No problem, it's clear that the patch has not been properly tested and
> obviously wrong. Has to be dropped. If somebody wants that, it will
> be material for v6.10 cycle.

..which makes me think that bitmap(bitops) lack of assembly test case(s).

Lucas De Marchi Feb. 21, 2024, 9:59 p.m. UTC | #8

On Wed, Feb 21, 2024 at 11:04:22PM +0200, Andy Shevchenko wrote:
>On Wed, Feb 21, 2024 at 10:30:02PM +0200, Dmitry Baryshkov wrote:
>> On Thu, 8 Feb 2024 at 09:45, Lucas De Marchi <lucas.demarchi@intel.com> wrote:
>
>...
>
>> > +#define BITS_PER_TYPE(type)    (sizeof(type) * BITS_PER_BYTE)
>
>Can sizeof() be used in assembly?
>
>...
>
>> > -#define __GENMASK(h, l) \
>> > -       (((~UL(0)) - (UL(1) << (l)) + 1) & \
>> > -        (~UL(0) >> (BITS_PER_LONG - 1 - (h))))
>> > -#define GENMASK(h, l) \
>> > -       (GENMASK_INPUT_CHECK(h, l) + __GENMASK(h, l))
>
>> > +#define __GENMASK(t, h, l) \
>> > +       (GENMASK_INPUT_CHECK(h, l) + \
>> > +        (((t)~0ULL - ((t)(1) << (l)) + 1) & \
>> > +        ((t)~0ULL >> (BITS_PER_TYPE(t) - 1 - (h)))))
>
>Nevertheless, the use ~0ULL is not proper assembly, this broke initial
>implementation using UL() / ULL().

indeed.

>
>
>> > -#define __GENMASK_ULL(h, l) \
>> > -       (((~ULL(0)) - (ULL(1) << (l)) + 1) & \
>> > -        (~ULL(0) >> (BITS_PER_LONG_LONG - 1 - (h))))
>> > -#define GENMASK_ULL(h, l) \
>> > -       (GENMASK_INPUT_CHECK(h, l) + __GENMASK_ULL(h, l))
>
>Ditto.

problem here seems actually because of the cast to the final type. My
previous impl was avoiding that, but was too verbose compared to this.

I will look at reverting this.

Lucas De Marchi

>
>> > +#define GENMASK(h, l)          __GENMASK(unsigned long,  h, l)
>> > +#define GENMASK_ULL(h, l)      __GENMASK(unsigned long long, h, l)
>> > +#define GENMASK_U8(h, l)       __GENMASK(u8,  h, l)
>> > +#define GENMASK_U16(h, l)      __GENMASK(u16, h, l)
>> > +#define GENMASK_U32(h, l)      __GENMASK(u32, h, l)
>> > +#define GENMASK_U64(h, l)      __GENMASK(u64, h, l)
>>
>> This breaks drm-tip on arm64 architecture:
>>
>> arch/arm64/kernel/entry-fpsimd.S: Assembler messages:
>> 465arch/arm64/kernel/entry-fpsimd.S:271: Error: found 'l', expected: ')'
>> 466arch/arm64/kernel/entry-fpsimd.S:66: Info: macro invoked from here
>> 467arch/arm64/kernel/entry-fpsimd.S:271: Error: found 'l', expected: ')'
>> 468arch/arm64/kernel/entry-fpsimd.S:66: Info: macro invoked from here
>> 469arch/arm64/kernel/entry-fpsimd.S:271: Error: found 'l', expected: ')'
>> 470arch/arm64/kernel/entry-fpsimd.S:66: Info: macro invoked from here
>> 471arch/arm64/kernel/entry-fpsimd.S:271: Error: found 'l', expected: ')'
>> 472arch/arm64/kernel/entry-fpsimd.S:66: Info: macro invoked from here
>> 473arch/arm64/kernel/entry-fpsimd.S:271: Error: unexpected characters
>> following instruction at operand 3 -- `bic x2,x1,(0+(((unsigned
>> long)~0ULL-((unsigned long)(1)<<(0))+1)&((unsigned
>> long)~0ULL>>((sizeof(unsigned long)*8)-1-(3)))))'
>> 474arch/arm64/kernel/entry-fpsimd.S:66: Info: macro invoked from here
>> 475arch/arm64/kernel/entry-fpsimd.S:282: Error: found 'l', expected: ')'
>> 476arch/arm64/kernel/entry-fpsimd.S:98: Info: macro invoked from here
>> 477arch/arm64/kernel/entry-fpsimd.S:282: Error: found 'l', expected: ')'
>> 478arch/arm64/kernel/entry-fpsimd.S:98: Info: macro invoked from here
>> 479arch/arm64/kernel/entry-fpsimd.S:282: Error: found 'l', expected: ')'
>> 480arch/arm64/kernel/entry-fpsimd.S:98: Info: macro invoked from here
>> 481arch/arm64/kernel/entry-fpsimd.S:282: Error: found 'l', expected: ')'
>> 482arch/arm64/kernel/entry-fpsimd.S:98: Info: macro invoked from here
>> 483arch/arm64/kernel/entry-fpsimd.S:282: Error: unexpected characters
>> following instruction at operand 3 -- `bic x2,x1,(0+(((unsigned
>> long)~0ULL-((unsigned long)(1)<<(0))+1)&((unsigned
>> long)~0ULL>>((sizeof(unsigned long)*8)-1-(3)))))'
>> 484arch/arm64/kernel/entry-fpsimd.S:98: Info: macro invoked from here
>
>-- 
>With Best Regards,
>Andy Shevchenko
>
>

Yury Norov Feb. 22, 2024, 2:49 p.m. UTC | #9

On Wed, Feb 21, 2024 at 03:59:06PM -0600, Lucas De Marchi wrote:
> On Wed, Feb 21, 2024 at 11:04:22PM +0200, Andy Shevchenko wrote:
> > On Wed, Feb 21, 2024 at 10:30:02PM +0200, Dmitry Baryshkov wrote:
> > > On Thu, 8 Feb 2024 at 09:45, Lucas De Marchi <lucas.demarchi@intel.com> wrote:
> > 
> > ...
> > 
> > > > +#define BITS_PER_TYPE(type)    (sizeof(type) * BITS_PER_BYTE)
> > 
> > Can sizeof() be used in assembly?
> > 
> > ...
> > 
> > > > -#define __GENMASK(h, l) \
> > > > -       (((~UL(0)) - (UL(1) << (l)) + 1) & \
> > > > -        (~UL(0) >> (BITS_PER_LONG - 1 - (h))))
> > > > -#define GENMASK(h, l) \
> > > > -       (GENMASK_INPUT_CHECK(h, l) + __GENMASK(h, l))
> > 
> > > > +#define __GENMASK(t, h, l) \
> > > > +       (GENMASK_INPUT_CHECK(h, l) + \
> > > > +        (((t)~0ULL - ((t)(1) << (l)) + 1) & \
> > > > +        ((t)~0ULL >> (BITS_PER_TYPE(t) - 1 - (h)))))
> > 
> > Nevertheless, the use ~0ULL is not proper assembly, this broke initial
> > implementation using UL() / ULL().
> 
> indeed.
> 
> > 
> > 
> > > > -#define __GENMASK_ULL(h, l) \
> > > > -       (((~ULL(0)) - (ULL(1) << (l)) + 1) & \
> > > > -        (~ULL(0) >> (BITS_PER_LONG_LONG - 1 - (h))))
> > > > -#define GENMASK_ULL(h, l) \
> > > > -       (GENMASK_INPUT_CHECK(h, l) + __GENMASK_ULL(h, l))
> > 
> > Ditto.
> 
> problem here seems actually because of the cast to the final type. My
> previous impl was avoiding that, but was too verbose compared to this.
> 
> I will look at reverting this.
> 
> Lucas De Marchi

The fix is quite straightforward. Can you consider the following
patch? I tested it for C and x86_64 asm parts, and it compiles well.

Thanks,
Yury

From 78b2887eea26f208aac50ae283ba9a4d062bb997 Mon Sep 17 00:00:00 2001
From: Yury Norov <yury.norov@gmail.com>
Date: Wed, 7 Feb 2024 23:45:19 -0800
Subject: [PATCH v2] bits: introduce fixed-type GENMASKs

Generalize __GENMASK() to support different types, and implement
fixed-types versions of GENMASK() based on it. The fixed-type version
allows more strict checks to the min/max values accepted, which is
useful for defining registers like implemented by i915 and xe drivers
with their REG_GENMASK*() macros.

The strict checks rely on shift-count-overflow compiler check to
fail the build if a number outside of the range allowed is passed.
Example:

	#define FOO_MASK GENMASK_U32(33, 4)

will generate a warning like:

	../include/linux/bits.h:41:31: error: left shift count >= width of type [-Werror=shift-count-overflow]
	   41 |          (((t)~0ULL - ((t)(1) << (l)) + 1) & \
	      |                               ^~

CC: Dmitry Baryshkov <dmitry.baryshkov@linaro.org>	      
Signed-off-by: Yury Norov <yury.norov@gmail.com>
Acked-by: Jani Nikula <jani.nikula@intel.com>
Reviewed-by: Andi Shyti <andi.shyti@linux.intel.com>

---
 include/linux/bitops.h |  1 -
 include/linux/bits.h   | 41 ++++++++++++++++++++++++++++-------------
 2 files changed, 28 insertions(+), 14 deletions(-)

diff --git a/include/linux/bitops.h b/include/linux/bitops.h
index 2ba557e067fe..1db50c69cfdb 100644
--- a/include/linux/bitops.h
+++ b/include/linux/bitops.h
@@ -15,7 +15,6 @@
 #  define aligned_byte_mask(n) (~0xffUL << (BITS_PER_LONG - 8 - 8*(n)))
 #endif
 
-#define BITS_PER_TYPE(type)	(sizeof(type) * BITS_PER_BYTE)
 #define BITS_TO_LONGS(nr)	__KERNEL_DIV_ROUND_UP(nr, BITS_PER_TYPE(long))
 #define BITS_TO_U64(nr)		__KERNEL_DIV_ROUND_UP(nr, BITS_PER_TYPE(u64))
 #define BITS_TO_U32(nr)		__KERNEL_DIV_ROUND_UP(nr, BITS_PER_TYPE(u32))
diff --git a/include/linux/bits.h b/include/linux/bits.h
index 7c0cf5031abe..f3cf8d5f2b55 100644
--- a/include/linux/bits.h
+++ b/include/linux/bits.h
@@ -6,6 +6,8 @@
 #include <vdso/bits.h>
 #include <asm/bitsperlong.h>
 
+#define BITS_PER_TYPE(type)	(sizeof(type) * BITS_PER_BYTE)
+
 #define BIT_MASK(nr)		(UL(1) << ((nr) % BITS_PER_LONG))
 #define BIT_WORD(nr)		((nr) / BITS_PER_LONG)
 #define BIT_ULL_MASK(nr)	(ULL(1) << ((nr) % BITS_PER_LONG_LONG))
@@ -22,24 +24,37 @@
 #define GENMASK_INPUT_CHECK(h, l) \
 	(BUILD_BUG_ON_ZERO(__builtin_choose_expr( \
 		__is_constexpr((l) > (h)), (l) > (h), 0)))
+#define __GENMASK(t, h, l) \
+	(GENMASK_INPUT_CHECK(h, l) + \
+	 (((t)~0ULL - ((t)(1) << (l)) + 1) & \
+	 ((t)~0ULL >> (BITS_PER_TYPE(t) - 1 - (h)))))
 #else
 /*
- * BUILD_BUG_ON_ZERO is not available in h files included from asm files,
- * disable the input check if that is the case.
+ * BUILD_BUG_ON_ZERO is not available in h files included from asm files.
+ * Similarly, assembler lacks for C types. So no parameters check in asm.
+ * It's users' responsibility to provide bitranges within a machine word
+ * boundaries.
  */
 #define GENMASK_INPUT_CHECK(h, l) 0
+#define __GENMASK(t, h, l) \
+	((~0 - (1 << (l)) + 1) & (~0 >> (BITS_PER_LONG - 1 - (h))))
 #endif
 
-#define __GENMASK(h, l) \
-	(((~UL(0)) - (UL(1) << (l)) + 1) & \
-	 (~UL(0) >> (BITS_PER_LONG - 1 - (h))))
-#define GENMASK(h, l) \
-	(GENMASK_INPUT_CHECK(h, l) + __GENMASK(h, l))
-
-#define __GENMASK_ULL(h, l) \
-	(((~ULL(0)) - (ULL(1) << (l)) + 1) & \
-	 (~ULL(0) >> (BITS_PER_LONG_LONG - 1 - (h))))
-#define GENMASK_ULL(h, l) \
-	(GENMASK_INPUT_CHECK(h, l) + __GENMASK_ULL(h, l))
+/*
+ * Generate a mask for the specified type @t. Additional checks are made to
+ * guarantee the value returned fits in that type, relying on
+ * shift-count-overflow compiler check to detect incompatible arguments.
+ * For example, all these create build errors or warnings:
+ *
+ * - GENMASK(15, 20): wrong argument order
+ * - GENMASK(72, 15): doesn't fit unsigned long
+ * - GENMASK_U32(33, 15): doesn't fit in a u32
+ */
+#define GENMASK(h, l)		__GENMASK(unsigned long,  h, l)
+#define GENMASK_ULL(h, l)	__GENMASK(unsigned long long, h, l)
+#define GENMASK_U8(h, l)	__GENMASK(u8,  h, l)
+#define GENMASK_U16(h, l)	__GENMASK(u16, h, l)
+#define GENMASK_U32(h, l)	__GENMASK(u32, h, l)
+#define GENMASK_U64(h, l)	__GENMASK(u64, h, l)
 
 #endif	/* __LINUX_BITS_H */

Andy Shevchenko Feb. 22, 2024, 3:04 p.m. UTC | #10

On Thu, Feb 22, 2024 at 06:49:59AM -0800, Yury Norov wrote:
> On Wed, Feb 21, 2024 at 03:59:06PM -0600, Lucas De Marchi wrote:

..

> +#define __GENMASK(t, h, l) \
> +	((~0 - (1 << (l)) + 1) & (~0 >> (BITS_PER_LONG - 1 - (h))))

What's wrong on using the UL/ULL() macros?

Also it would be really good to avoid bifurcation of the implementations of
__GENMASK() for both cases.

..

> -#define __GENMASK(h, l) \
> -	(((~UL(0)) - (UL(1) << (l)) + 1) & \
> -	 (~UL(0) >> (BITS_PER_LONG - 1 - (h))))

This at bare minimum can be left untouched for asm case, no?

Yury Norov Feb. 22, 2024, 9:31 p.m. UTC | #11

On Thu, Feb 22, 2024 at 05:04:10PM +0200, Andy Shevchenko wrote:
> On Thu, Feb 22, 2024 at 06:49:59AM -0800, Yury Norov wrote:
> > On Wed, Feb 21, 2024 at 03:59:06PM -0600, Lucas De Marchi wrote:
> 
> ...
> 
> > +#define __GENMASK(t, h, l) \
> > +	((~0 - (1 << (l)) + 1) & (~0 >> (BITS_PER_LONG - 1 - (h))))
> 
> What's wrong on using the UL/ULL() macros?

Nothing wrong except that in the !__ASSEMBLY section they all are
useless. And having that in mind, useless macros may hurt readability.

> Also it would be really good to avoid bifurcation of the implementations of
> __GENMASK() for both cases.

Not exactly. It would be really good if GENMASK_XX() will share the
implementation (and they do). The underscored helper macro is not
intended to be used directly, and I think nobody cares.

> ...
> 
> > -#define __GENMASK(h, l) \
> > -	(((~UL(0)) - (UL(1) << (l)) + 1) & \
> > -	 (~UL(0) >> (BITS_PER_LONG - 1 - (h))))
> 
> This at bare minimum can be left untouched for asm case, no?

As soon as we have to have different versions for the macro depending
on __ASSEMBLY__, I would prefer to remove all compatibility black
magic. After all, the <linux/bits.h> machinery to me is about the same
level of abstraction as the stuff in <linux/const.h>, and in future we
can try to remove dependency on it.

This all is not a big deal to me. I can keep the old implementation
for the asm, if you think it's really important.

What are you thinking guys?

Thanks,
Yury

Lucas De Marchi Feb. 28, 2024, 11:39 p.m. UTC | #12

On Thu, Feb 22, 2024 at 06:49:59AM -0800, Yury Norov wrote:
>On Wed, Feb 21, 2024 at 03:59:06PM -0600, Lucas De Marchi wrote:
>> On Wed, Feb 21, 2024 at 11:04:22PM +0200, Andy Shevchenko wrote:
>> > On Wed, Feb 21, 2024 at 10:30:02PM +0200, Dmitry Baryshkov wrote:
>> > > On Thu, 8 Feb 2024 at 09:45, Lucas De Marchi <lucas.demarchi@intel.com> wrote:
>> >
>> > ...
>> >
>> > > > +#define BITS_PER_TYPE(type)    (sizeof(type) * BITS_PER_BYTE)
>> >
>> > Can sizeof() be used in assembly?
>> >
>> > ...
>> >
>> > > > -#define __GENMASK(h, l) \
>> > > > -       (((~UL(0)) - (UL(1) << (l)) + 1) & \
>> > > > -        (~UL(0) >> (BITS_PER_LONG - 1 - (h))))
>> > > > -#define GENMASK(h, l) \
>> > > > -       (GENMASK_INPUT_CHECK(h, l) + __GENMASK(h, l))
>> >
>> > > > +#define __GENMASK(t, h, l) \
>> > > > +       (GENMASK_INPUT_CHECK(h, l) + \
>> > > > +        (((t)~0ULL - ((t)(1) << (l)) + 1) & \
>> > > > +        ((t)~0ULL >> (BITS_PER_TYPE(t) - 1 - (h)))))
>> >
>> > Nevertheless, the use ~0ULL is not proper assembly, this broke initial
>> > implementation using UL() / ULL().
>>
>> indeed.
>>
>> >
>> >
>> > > > -#define __GENMASK_ULL(h, l) \
>> > > > -       (((~ULL(0)) - (ULL(1) << (l)) + 1) & \
>> > > > -        (~ULL(0) >> (BITS_PER_LONG_LONG - 1 - (h))))
>> > > > -#define GENMASK_ULL(h, l) \
>> > > > -       (GENMASK_INPUT_CHECK(h, l) + __GENMASK_ULL(h, l))
>> >
>> > Ditto.
>>
>> problem here seems actually because of the cast to the final type. My
>> previous impl was avoiding that, but was too verbose compared to this.
>>
>> I will look at reverting this.
>>
>> Lucas De Marchi
>
>The fix is quite straightforward. Can you consider the following
>patch? I tested it for C and x86_64 asm parts, and it compiles well.
>
>Thanks,
>Yury
>
>From 78b2887eea26f208aac50ae283ba9a4d062bb997 Mon Sep 17 00:00:00 2001
>From: Yury Norov <yury.norov@gmail.com>
>Date: Wed, 7 Feb 2024 23:45:19 -0800
>Subject: [PATCH v2] bits: introduce fixed-type GENMASKs
>
>Generalize __GENMASK() to support different types, and implement
>fixed-types versions of GENMASK() based on it. The fixed-type version
>allows more strict checks to the min/max values accepted, which is
>useful for defining registers like implemented by i915 and xe drivers
>with their REG_GENMASK*() macros.
>
>The strict checks rely on shift-count-overflow compiler check to
>fail the build if a number outside of the range allowed is passed.
>Example:
>
>	#define FOO_MASK GENMASK_U32(33, 4)
>
>will generate a warning like:
>
>	../include/linux/bits.h:41:31: error: left shift count >= width of type [-Werror=shift-count-overflow]
>	   41 |          (((t)~0ULL - ((t)(1) << (l)) + 1) & \
>	      |                               ^~
>
>CC: Dmitry Baryshkov <dmitry.baryshkov@linaro.org>	
>Signed-off-by: Yury Norov <yury.norov@gmail.com>
>Acked-by: Jani Nikula <jani.nikula@intel.com>
>Reviewed-by: Andi Shyti <andi.shyti@linux.intel.com>

I build-tested this in x86-64, x86-32 and arm64. I didn't like much the
need to fork the __GENMASK() implementation on the 2 sides of the ifdef
since I think the GENMASK_INPUT_CHECK() should be the one covering the
input checks. However to make it common we'd need to solve 2 problems:
the casts and the sizeof. The sizeof can be passed as arg to
__GENMASK(), however the casts I think would need a __CAST_U8(x)
or the like and sprinkle it everywhere, which would hurt readability.
Not pretty. Or go back to the original submission and make it less
horrible :-/

>
>---
> include/linux/bitops.h |  1 -
> include/linux/bits.h   | 41 ++++++++++++++++++++++++++++-------------
> 2 files changed, 28 insertions(+), 14 deletions(-)
>
>diff --git a/include/linux/bitops.h b/include/linux/bitops.h
>index 2ba557e067fe..1db50c69cfdb 100644
>--- a/include/linux/bitops.h
>+++ b/include/linux/bitops.h
>@@ -15,7 +15,6 @@
> #  define aligned_byte_mask(n) (~0xffUL << (BITS_PER_LONG - 8 - 8*(n)))
> #endif
>
>-#define BITS_PER_TYPE(type)	(sizeof(type) * BITS_PER_BYTE)
> #define BITS_TO_LONGS(nr)	__KERNEL_DIV_ROUND_UP(nr, BITS_PER_TYPE(long))
> #define BITS_TO_U64(nr)		__KERNEL_DIV_ROUND_UP(nr, BITS_PER_TYPE(u64))
> #define BITS_TO_U32(nr)		__KERNEL_DIV_ROUND_UP(nr, BITS_PER_TYPE(u32))
>diff --git a/include/linux/bits.h b/include/linux/bits.h
>index 7c0cf5031abe..f3cf8d5f2b55 100644
>--- a/include/linux/bits.h
>+++ b/include/linux/bits.h
>@@ -6,6 +6,8 @@
> #include <vdso/bits.h>
> #include <asm/bitsperlong.h>
>
>+#define BITS_PER_TYPE(type)	(sizeof(type) * BITS_PER_BYTE)
>+
> #define BIT_MASK(nr)		(UL(1) << ((nr) % BITS_PER_LONG))
> #define BIT_WORD(nr)		((nr) / BITS_PER_LONG)
> #define BIT_ULL_MASK(nr)	(ULL(1) << ((nr) % BITS_PER_LONG_LONG))
>@@ -22,24 +24,37 @@
> #define GENMASK_INPUT_CHECK(h, l) \
> 	(BUILD_BUG_ON_ZERO(__builtin_choose_expr( \
> 		__is_constexpr((l) > (h)), (l) > (h), 0)))
>+#define __GENMASK(t, h, l) \
>+	(GENMASK_INPUT_CHECK(h, l) + \
>+	 (((t)~0ULL - ((t)(1) << (l)) + 1) & \
>+	 ((t)~0ULL >> (BITS_PER_TYPE(t) - 1 - (h)))))
> #else
> /*
>- * BUILD_BUG_ON_ZERO is not available in h files included from asm files,
>- * disable the input check if that is the case.
>+ * BUILD_BUG_ON_ZERO is not available in h files included from asm files.
>+ * Similarly, assembler lacks for C types. So no parameters check in asm.
>+ * It's users' responsibility to provide bitranges within a machine word
>+ * boundaries.
>  */
> #define GENMASK_INPUT_CHECK(h, l) 0
>+#define __GENMASK(t, h, l) \
>+	((~0 - (1 << (l)) + 1) & (~0 >> (BITS_PER_LONG - 1 - (h))))

humn... this builds, but does it work if GENMASK_ULL() is used in
assembly? That BITS_PER_LONG does not match the type width.

Lucas De Marchi

> #endif
>
>-#define __GENMASK(h, l) \
>-	(((~UL(0)) - (UL(1) << (l)) + 1) & \
>-	 (~UL(0) >> (BITS_PER_LONG - 1 - (h))))
>-#define GENMASK(h, l) \
>-	(GENMASK_INPUT_CHECK(h, l) + __GENMASK(h, l))
>-
>-#define __GENMASK_ULL(h, l) \
>-	(((~ULL(0)) - (ULL(1) << (l)) + 1) & \
>-	 (~ULL(0) >> (BITS_PER_LONG_LONG - 1 - (h))))
>-#define GENMASK_ULL(h, l) \
>-	(GENMASK_INPUT_CHECK(h, l) + __GENMASK_ULL(h, l))
>+/*
>+ * Generate a mask for the specified type @t. Additional checks are made to
>+ * guarantee the value returned fits in that type, relying on
>+ * shift-count-overflow compiler check to detect incompatible arguments.
>+ * For example, all these create build errors or warnings:
>+ *
>+ * - GENMASK(15, 20): wrong argument order
>+ * - GENMASK(72, 15): doesn't fit unsigned long
>+ * - GENMASK_U32(33, 15): doesn't fit in a u32
>+ */
>+#define GENMASK(h, l)		__GENMASK(unsigned long,  h, l)
>+#define GENMASK_ULL(h, l)	__GENMASK(unsigned long long, h, l)
>+#define GENMASK_U8(h, l)	__GENMASK(u8,  h, l)
>+#define GENMASK_U16(h, l)	__GENMASK(u16, h, l)
>+#define GENMASK_U32(h, l)	__GENMASK(u32, h, l)
>+#define GENMASK_U64(h, l)	__GENMASK(u64, h, l)
>
> #endif	/* __LINUX_BITS_H */
>-- 
>2.40.1
>

Andy Shevchenko Feb. 29, 2024, 10:49 a.m. UTC | #13

On Wed, Feb 28, 2024 at 05:39:21PM -0600, Lucas De Marchi wrote:
> On Thu, Feb 22, 2024 at 06:49:59AM -0800, Yury Norov wrote:
> > On Wed, Feb 21, 2024 at 03:59:06PM -0600, Lucas De Marchi wrote:
> > > On Wed, Feb 21, 2024 at 11:04:22PM +0200, Andy Shevchenko wrote:
> > > > On Wed, Feb 21, 2024 at 10:30:02PM +0200, Dmitry Baryshkov wrote:
> > > > > On Thu, 8 Feb 2024 at 09:45, Lucas De Marchi <lucas.demarchi@intel.com> wrote:

..

> I build-tested this in x86-64, x86-32 and arm64. I didn't like much the
> need to fork the __GENMASK() implementation on the 2 sides of the ifdef
> since I think the GENMASK_INPUT_CHECK() should be the one covering the
> input checks. However to make it common we'd need to solve 2 problems:
> the casts and the sizeof. The sizeof can be passed as arg to
> __GENMASK(), however the casts I think would need a __CAST_U8(x)
> or the like and sprinkle it everywhere, which would hurt readability.
> Not pretty. Or go back to the original submission and make it less
> horrible :-/

I'm wondering if we can use _Generic() approach here.

..

> > #define GENMASK_INPUT_CHECK(h, l) 0
> > +#define __GENMASK(t, h, l) \
> > +	((~0 - (1 << (l)) + 1) & (~0 >> (BITS_PER_LONG - 1 - (h))))
> 
> humn... this builds, but does it work if GENMASK_ULL() is used in
> assembly? That BITS_PER_LONG does not match the type width.

UL()/ULL() macros are not just for fun.

Lucas De Marchi Feb. 29, 2024, 6:21 p.m. UTC | #14

On Thu, Feb 29, 2024 at 12:49:57PM +0200, Andy Shevchenko wrote:
>On Wed, Feb 28, 2024 at 05:39:21PM -0600, Lucas De Marchi wrote:
>> On Thu, Feb 22, 2024 at 06:49:59AM -0800, Yury Norov wrote:
>> > On Wed, Feb 21, 2024 at 03:59:06PM -0600, Lucas De Marchi wrote:
>> > > On Wed, Feb 21, 2024 at 11:04:22PM +0200, Andy Shevchenko wrote:
>> > > > On Wed, Feb 21, 2024 at 10:30:02PM +0200, Dmitry Baryshkov wrote:
>> > > > > On Thu, 8 Feb 2024 at 09:45, Lucas De Marchi <lucas.demarchi@intel.com> wrote:
>
>...
>
>> I build-tested this in x86-64, x86-32 and arm64. I didn't like much the
>> need to fork the __GENMASK() implementation on the 2 sides of the ifdef
>> since I think the GENMASK_INPUT_CHECK() should be the one covering the
>> input checks. However to make it common we'd need to solve 2 problems:
>> the casts and the sizeof. The sizeof can be passed as arg to
>> __GENMASK(), however the casts I think would need a __CAST_U8(x)
>> or the like and sprinkle it everywhere, which would hurt readability.
>> Not pretty. Or go back to the original submission and make it less
>> horrible :-/
>
>I'm wondering if we can use _Generic() approach here.

in assembly?

>
>...
>
>> > #define GENMASK_INPUT_CHECK(h, l) 0
>> > +#define __GENMASK(t, h, l) \
>> > +	((~0 - (1 << (l)) + 1) & (~0 >> (BITS_PER_LONG - 1 - (h))))
>>
>> humn... this builds, but does it work if GENMASK_ULL() is used in
>> assembly? That BITS_PER_LONG does not match the type width.
>
>UL()/ULL() macros are not just for fun.

they are not for fun, but they expand to a nop in assembly. And it's up
to the instruction used to be the right one. Since this branch is for
assembly only, having them wouldn't really change the current behavior.
I'm talking about BITS_PER_LONG vs BITS_PER_LONG_LONG. That introduces
a bug here.

Lucas De Marchi

>
>-- 
>With Best Regards,
>Andy Shevchenko
>
>

Andy Shevchenko Feb. 29, 2024, 6:27 p.m. UTC | #15

On Thu, Feb 29, 2024 at 12:21:34PM -0600, Lucas De Marchi wrote:
> On Thu, Feb 29, 2024 at 12:49:57PM +0200, Andy Shevchenko wrote:
> > On Wed, Feb 28, 2024 at 05:39:21PM -0600, Lucas De Marchi wrote:
> > > On Thu, Feb 22, 2024 at 06:49:59AM -0800, Yury Norov wrote:

..

> > > I build-tested this in x86-64, x86-32 and arm64. I didn't like much the
> > > need to fork the __GENMASK() implementation on the 2 sides of the ifdef
> > > since I think the GENMASK_INPUT_CHECK() should be the one covering the
> > > input checks. However to make it common we'd need to solve 2 problems:
> > > the casts and the sizeof. The sizeof can be passed as arg to
> > > __GENMASK(), however the casts I think would need a __CAST_U8(x)
> > > or the like and sprinkle it everywhere, which would hurt readability.
> > > Not pretty. Or go back to the original submission and make it less
> > > horrible :-/
> > 
> > I'm wondering if we can use _Generic() approach here.
> 
> in assembly?

Yes.

Lucas De Marchi March 1, 2024, 4:06 a.m. UTC | #16

On Thu, Feb 29, 2024 at 08:27:30PM +0200, Andy Shevchenko wrote:
>On Thu, Feb 29, 2024 at 12:21:34PM -0600, Lucas De Marchi wrote:
>> On Thu, Feb 29, 2024 at 12:49:57PM +0200, Andy Shevchenko wrote:
>> > On Wed, Feb 28, 2024 at 05:39:21PM -0600, Lucas De Marchi wrote:
>> > > On Thu, Feb 22, 2024 at 06:49:59AM -0800, Yury Norov wrote:
>
>...
>
>> > > I build-tested this in x86-64, x86-32 and arm64. I didn't like much the
>> > > need to fork the __GENMASK() implementation on the 2 sides of the ifdef
>> > > since I think the GENMASK_INPUT_CHECK() should be the one covering the
>> > > input checks. However to make it common we'd need to solve 2 problems:
>> > > the casts and the sizeof. The sizeof can be passed as arg to
>> > > __GENMASK(), however the casts I think would need a __CAST_U8(x)
>> > > or the like and sprinkle it everywhere, which would hurt readability.
>> > > Not pretty. Or go back to the original submission and make it less
>> > > horrible :-/
>> >
>> > I'm wondering if we can use _Generic() approach here.
>>
>> in assembly?
>
>Yes.

I added a _Generic() in a random .S and to my surprise the build didn't
break. Then I went to implement, and couldn't find where the _Generic()
would actually be useful. What I came up with builds for me with gcc on
x86-64, x86-32 and arm64.

I'm also adding some additional tests in lib/test_bits.c to cover the
expected values and types. Thoughts?

--------8<------------
Subject: [PATCH] bits: introduce fixed-type genmasks

Generalize __GENMASK() to support different types, and implement
fixed-types versions of GENMASK() based on it. The fixed-type version
allows more strict checks to the min/max values accepted, which is
useful for defining registers like implemented by i915 and xe drivers
with their REG_GENMASK*() macros.

The strict checks rely on shift-count-overflow compiler check to
fail the build if a number outside of the range allowed is passed.
Example:

	#define FOO_MASK GENMASK_U32(33, 4)

will generate a warning like:

	../include/linux/bits.h:48:23: warning: right shift count is negative [-Wshift-count-negative]
	   48 |          (~literal(0) >> ((w) - 1 - (h)))))
	      |                       ^~

Some additional tests in lib/test_bits.c are added to cover the
expected/non-expected values and also that the result value matches the
expected type. Since those are known at build time, use static_assert()
instead of normal kunit tests.

Signed-off-by: Lucas De Marchi <lucas.demarchi@intel.com>
---
  include/linux/bits.h | 33 +++++++++++++++++++++++----------
  lib/test_bits.c      | 21 +++++++++++++++++++--
  2 files changed, 42 insertions(+), 12 deletions(-)

diff --git a/include/linux/bits.h b/include/linux/bits.h
index 7c0cf5031abe8..6f089e71a195c 100644
--- a/include/linux/bits.h
+++ b/include/linux/bits.h
@@ -22,24 +22,37 @@
  #define GENMASK_INPUT_CHECK(h, l) \
  	(BUILD_BUG_ON_ZERO(__builtin_choose_expr( \
  		__is_constexpr((l) > (h)), (l) > (h), 0)))
+#define __CAST_T(t, v) ((t)v)
  #else
  /*
   * BUILD_BUG_ON_ZERO is not available in h files included from asm files,
   * disable the input check if that is the case.
   */
  #define GENMASK_INPUT_CHECK(h, l) 0
+#define __CAST_T(t, v) v
  #endif
  
-#define __GENMASK(h, l) \
-	(((~UL(0)) - (UL(1) << (l)) + 1) & \
-	 (~UL(0) >> (BITS_PER_LONG - 1 - (h))))
-#define GENMASK(h, l) \
-	(GENMASK_INPUT_CHECK(h, l) + __GENMASK(h, l))
+/*
+ * Generate a mask for a specific type. @literal is the suffix to be used for
+ * an integer constant of that type and @width is the bits-per-type. Additional
+ * checks are made to guarantee the value returned fits in that type, relying
+ * on shift-count-overflow compiler check to detect incompatible arguments.
+ * For example, all these create build errors or warnings:
+ *
+ * - GENMASK(15, 20): wrong argument order
+ * - GENMASK(72, 15): doesn't fit unsigned long
+ * - GENMASK_U32(33, 15): doesn't fit in a u32
+ */
+#define __GENMASK(literal, w, h, l) \
+	(GENMASK_INPUT_CHECK(h, l) + \
+	 ((~literal(0) - (literal(1) << (l)) + 1) & \
+	 (~literal(0) >> ((w) - 1 - (h)))))
  
-#define __GENMASK_ULL(h, l) \
-	(((~ULL(0)) - (ULL(1) << (l)) + 1) & \
-	 (~ULL(0) >> (BITS_PER_LONG_LONG - 1 - (h))))
-#define GENMASK_ULL(h, l) \
-	(GENMASK_INPUT_CHECK(h, l) + __GENMASK_ULL(h, l))
+#define GENMASK(h, l)		__GENMASK(UL, BITS_PER_LONG, h, l)
+#define GENMASK_ULL(h, l)	__GENMASK(ULL, BITS_PER_LONG_LONG, h, l)
+#define GENMASK_U8(h, l)	__CAST_T(u8, __GENMASK(UL, 8, h, l))
+#define GENMASK_U16(h, l)	__CAST_T(u16, __GENMASK(UL, 16, h, l))
+#define GENMASK_U32(h, l)	__CAST_T(u32, __GENMASK(UL, 32, h, l))
+#define GENMASK_U64(h, l)	__CAST_T(u64, __GENMASK(ULL, 64, h, l))
  
  #endif	/* __LINUX_BITS_H */
diff --git a/lib/test_bits.c b/lib/test_bits.c
index c9368a2314e7c..e2fc1a1d38702 100644
--- a/lib/test_bits.c
+++ b/lib/test_bits.c
@@ -5,7 +5,16 @@
  
  #include <kunit/test.h>
  #include <linux/bits.h>
+#include <linux/types.h>
  
+#define assert_type(t, x) _Generic(x, t: x, default: 0)
+
+static_assert(assert_type(unsigned long, GENMASK(31, 0)) == U32_MAX);
+static_assert(assert_type(unsigned long long, GENMASK_ULL(63, 0)) == U64_MAX);
+static_assert(assert_type(u64, GENMASK_U64(63, 0)) == U64_MAX);
+static_assert(assert_type(u32, GENMASK_U32(31, 0)) == U32_MAX);
+static_assert(assert_type(u16, GENMASK_U16(15, 0)) == U16_MAX);
+static_assert(assert_type(u8,  GENMASK_U8(7, 0))   == U8_MAX);
  
  static void genmask_test(struct kunit *test)
  {
@@ -14,14 +23,22 @@ static void genmask_test(struct kunit *test)
  	KUNIT_EXPECT_EQ(test, 6ul, GENMASK(2, 1));
  	KUNIT_EXPECT_EQ(test, 0xFFFFFFFFul, GENMASK(31, 0));
  
+	KUNIT_EXPECT_EQ(test, 1u, GENMASK_U8(0, 0));
+	KUNIT_EXPECT_EQ(test, 3u, GENMASK_U16(1, 0));
+	KUNIT_EXPECT_EQ(test, 0x10000, GENMASK_U32(16, 16));
+
  #ifdef TEST_GENMASK_FAILURES
  	/* these should fail compilation */
  	GENMASK(0, 1);
  	GENMASK(0, 10);
  	GENMASK(9, 10);
-#endif
-
  
+	GENMASK_U32(0, 31);
+	GENMASK_U64(64, 0);
+	GENMASK_U32(32, 0);
+	GENMASK_U16(16, 0);
+	GENMASK_U8(8, 0);
+#endif
  }
  
  static void genmask_ull_test(struct kunit *test)

diff mbox series

Patch

diff --git a/include/linux/bitops.h b/include/linux/bitops.h
index 2ba557e067fe..1db50c69cfdb 100644
--- a/include/linux/bitops.h
+++ b/include/linux/bitops.h
@@ -15,7 +15,6 @@ 
 #  define aligned_byte_mask(n) (~0xffUL << (BITS_PER_LONG - 8 - 8*(n)))
 #endif
 
-#define BITS_PER_TYPE(type)	(sizeof(type) * BITS_PER_BYTE)
 #define BITS_TO_LONGS(nr)	__KERNEL_DIV_ROUND_UP(nr, BITS_PER_TYPE(long))
 #define BITS_TO_U64(nr)		__KERNEL_DIV_ROUND_UP(nr, BITS_PER_TYPE(u64))
 #define BITS_TO_U32(nr)		__KERNEL_DIV_ROUND_UP(nr, BITS_PER_TYPE(u32))
diff --git a/include/linux/bits.h b/include/linux/bits.h
index 7c0cf5031abe..bd56f32de44e 100644
--- a/include/linux/bits.h
+++ b/include/linux/bits.h
@@ -6,6 +6,8 @@ 
 #include <vdso/bits.h>
 #include <asm/bitsperlong.h>
 
+#define BITS_PER_TYPE(type)	(sizeof(type) * BITS_PER_BYTE)
+
 #define BIT_MASK(nr)		(UL(1) << ((nr) % BITS_PER_LONG))
 #define BIT_WORD(nr)		((nr) / BITS_PER_LONG)
 #define BIT_ULL_MASK(nr)	(ULL(1) << ((nr) % BITS_PER_LONG_LONG))
@@ -30,16 +32,26 @@ 
 #define GENMASK_INPUT_CHECK(h, l) 0
 #endif
 
-#define __GENMASK(h, l) \
-	(((~UL(0)) - (UL(1) << (l)) + 1) & \
-	 (~UL(0) >> (BITS_PER_LONG - 1 - (h))))
-#define GENMASK(h, l) \
-	(GENMASK_INPUT_CHECK(h, l) + __GENMASK(h, l))
+/*
+ * Generate a mask for the specified type @t. Additional checks are made to
+ * guarantee the value returned fits in that type, relying on
+ * shift-count-overflow compiler check to detect incompatible arguments.
+ * For example, all these create build errors or warnings:
+ *
+ * - GENMASK(15, 20): wrong argument order
+ * - GENMASK(72, 15): doesn't fit unsigned long
+ * - GENMASK_U32(33, 15): doesn't fit in a u32
+ */
+#define __GENMASK(t, h, l) \
+	(GENMASK_INPUT_CHECK(h, l) + \
+	 (((t)~0ULL - ((t)(1) << (l)) + 1) & \
+	 ((t)~0ULL >> (BITS_PER_TYPE(t) - 1 - (h)))))
 
-#define __GENMASK_ULL(h, l) \
-	(((~ULL(0)) - (ULL(1) << (l)) + 1) & \
-	 (~ULL(0) >> (BITS_PER_LONG_LONG - 1 - (h))))
-#define GENMASK_ULL(h, l) \
-	(GENMASK_INPUT_CHECK(h, l) + __GENMASK_ULL(h, l))
+#define GENMASK(h, l)		__GENMASK(unsigned long,  h, l)
+#define GENMASK_ULL(h, l)	__GENMASK(unsigned long long, h, l)
+#define GENMASK_U8(h, l)	__GENMASK(u8,  h, l)
+#define GENMASK_U16(h, l)	__GENMASK(u16, h, l)
+#define GENMASK_U32(h, l)	__GENMASK(u32, h, l)
+#define GENMASK_U64(h, l)	__GENMASK(u64, h, l)
 
 #endif	/* __LINUX_BITS_H */