Message-ID: <Z8ipvnURG_iejzSX@smile.fi.intel.com>
Date: Wed, 5 Mar 2025 21:45:02 +0200
From: Andy Shevchenko <andriy.shevchenko@...ux.intel.com>
To: Vincent Mailhol <mailhol.vincent@...adoo.fr>
Cc: Yury Norov <yury.norov@...il.com>,
	Lucas De Marchi <lucas.demarchi@...el.com>,
	Rasmus Villemoes <linux@...musvillemoes.dk>,
	Jani Nikula <jani.nikula@...ux.intel.com>,
	Joonas Lahtinen <joonas.lahtinen@...ux.intel.com>,
	Rodrigo Vivi <rodrigo.vivi@...el.com>,
	Tvrtko Ursulin <tursulin@...ulin.net>,
	David Airlie <airlied@...il.com>, Simona Vetter <simona@...ll.ch>,
	Andrew Morton <akpm@...ux-foundation.org>,
	linux-kernel@...r.kernel.org, intel-gfx@...ts.freedesktop.org,
	dri-devel@...ts.freedesktop.org,
	Andi Shyti <andi.shyti@...ux.intel.com>,
	David Laight <David.Laight@...lab.com>,
	Dmitry Baryshkov <dmitry.baryshkov@...aro.org>,
	Jani Nikula <jani.nikula@...el.com>
Subject: Re: [PATCH v4 3/8] bits: introduce fixed-type genmasks

On Thu, Mar 06, 2025 at 01:48:49AM +0900, Vincent Mailhol wrote:
> On 06/03/2025 at 00:47, Yury Norov wrote:
> > On Wed, Mar 05, 2025 at 10:00:15PM +0900, Vincent Mailhol via B4 Relay wrote:

...

> > Having, in fact, different implementations of the same macro for kernel
> > and userspace is a source of problems. Can we move GENMASK_TYPE() to uapi,
> > and implement __GENMASK() on top of them? If not, I'd prefer to keep
> > GENMASK and GENMASK_ULL untouched.
> 
> This is something which I tried to explain in the cover letter. I am not
> confident to declare GENMASK_TYPE() in the uapi and expose it to the
> userland. If we do so, any future change in the parameters would be a
> user breaking change. __GENMASK_U128() looks already too much to me for
> the uapi, I am not keen to bloat it even more with GENMASK_TYPE().
> 
> This plus the fact that if we use GENMASK_TYPE() to generate the asm
> variant, then we can not rely on sizeof() in the definition which makes
> everything over complicated.

I am with you here. The less we done in uAPI the better.
uAPI is something carved in stone, once done it's impossible to change.

> I acknowledge that not having a common denominator is not best, but I
> see this as an acceptable tradeoff.

...

> >> +#define GENMASK_U8(h, l) ((unsigned int)GENMASK_t(u8,  h, l))
> >> +#define GENMASK_U16(h, l) ((unsigned int)GENMASK_t(u16, h, l))
> > 
> > Typecast to the type that user provides explicitly?  And maybe do
> > in GENMASK_TYPE()
> 
> I have a slight preference for the cast to unsigned int for the reason
> explained above. But that is not a deal breaker. If you believe that the
> u8/u16 casts are better, let me know, I will be happy to change it :)

At least can you provide an existing use case (or use cases) that need
this castings? Also still a big question what will happen with it on asm.
Can it cope with 0x000000f0 passed as imm8, for example?

> >> +#define GENMASK_U32(h, l) GENMASK_t(u32, h, l)
> >> +#define GENMASK_U64(h, l) GENMASK_t(u64, h, l)

...

> > But GENMASK_U128() becomes a special case now.
> > The 128-bit GENMASK is unsued, but it's exported in uapi. Is there any
> > simple way to end up with a common implementation for all fixed-type
> > GENMASKs?
> 
> What bothers me is that the 128 bit types are not something available on
> all architectures, c.f. the CONFIG_ARCH_SUPPORTS_INT128. So, I would
> need a U128() equivalent to the ULL() but which does not break on
> architectures which do not support 128 bits integers.
> 
> This is where I am stuck. If someone can guide me on how to write a
> robust U128() macro, then I think the common implementation could be
> feasible.

I think we may leave that U128 stuff alone for now.

-- 
With Best Regards,
Andy Shevchenko

Powered by blists - more mailing lists