From mboxrd@z Thu Jan 1 00:00:00 1970 Received: from out.smtpout.orange.fr (out-18.smtpout.orange.fr [193.252.22.18]) (using TLSv1.2 with cipher AES128-GCM-SHA256 (128/128 bits)) (No client certificate requested) by smtp.subspace.kernel.org (Postfix) with ESMTPS id 064FC227B9F for ; Mon, 10 Mar 2025 10:46:56 +0000 (UTC) Authentication-Results: smtp.subspace.kernel.org; arc=none smtp.client-ip=193.252.22.18 ARC-Seal:i=1; a=rsa-sha256; d=subspace.kernel.org; s=arc-20240116; t=1741603620; cv=none; b=E7tljAv+nAQLwFxy6DJES8jlgyWpNQTAOW019UctolFBeFScWp2lb3VPQftfITEgtfe0/LkFNkq5LUn0/UBwSnFJKUQGOMihZfny/M3CtE/vWgt2mq69e44renv9EIVkbtkavIJCkgh3rieek7oOIyy68TQWurTRr4y7fDZzDLE= ARC-Message-Signature:i=1; a=rsa-sha256; d=subspace.kernel.org; s=arc-20240116; t=1741603620; c=relaxed/simple; bh=werN9mZ2wg+YvLowvzIJiio61Fy7BVWnTM1tWOzX5Kc=; h=Message-ID:Date:MIME-Version:Subject:To:Cc:References:From: In-Reply-To:Content-Type; b=avtUdyWrrb+84bXHyHrdN7PoNvF47F9Ug+9JG7+5TGR2aisCIUCoBDO1ZKvrPhf3J+sRK9fue5lkb7A/Y5EQ0azIz8a3RqmVV3+WD4JKxti/ivA6i2Y9TjR4P/pYkPD6c1a9av4QwXX6Y2kxXuRnQYUcQwOm+pYVwWQQRA/DNmI= ARC-Authentication-Results:i=1; smtp.subspace.kernel.org; dmarc=pass (p=quarantine dis=none) header.from=wanadoo.fr; spf=pass smtp.mailfrom=wanadoo.fr; dkim=pass (2048-bit key) header.d=wanadoo.fr header.i=@wanadoo.fr header.b=MKsE1Ajc; arc=none smtp.client-ip=193.252.22.18 Authentication-Results: smtp.subspace.kernel.org; dmarc=pass (p=quarantine dis=none) header.from=wanadoo.fr Authentication-Results: smtp.subspace.kernel.org; spf=pass smtp.mailfrom=wanadoo.fr Authentication-Results: smtp.subspace.kernel.org; dkim=pass (2048-bit key) header.d=wanadoo.fr header.i=@wanadoo.fr header.b="MKsE1Ajc" Received: from [172.16.82.72] ([124.33.176.97]) by smtp.orange.fr with ESMTPA id raebt34wJACDNraeftbFlA; Mon, 10 Mar 2025 11:46:49 +0100 DKIM-Signature: v=1; a=rsa-sha256; c=relaxed/relaxed; d=wanadoo.fr; s=t20230301; t=1741603609; bh=SM+GH4W9Mgpq6UbMamTcytvpYPgAzdTWefneB42yE/0=; h=Message-ID:Date:MIME-Version:Subject:To:From; b=MKsE1Ajc+m6LGBqtCOp+fWLgINX4uVO2Wrtli0Wf+5I3+tbC5y8eUDQpVvSr+sVVq LGT5lUHG3IOlheI+5av6dg7E+H+0dAGJ9PmK1E3z4ldjAqjbTNpWRlcModsGvkIBPK McEgwDJzub0wiygpF9OLg8yJkqz4zrzyRupxB2fdmvNSwVVbeynkkDf1dMT4h/QfzF anmuTlXxHZ6Y3C0MFmb/Nmz76K5aADaLEu1rl0GYuQbLqM89s9tFFezxlxTjRqYZl6 N7de4kkml1YTIY+mUr4sBP5yGotFwTIY862lumo4VGXwgMSi0hlFMI0ujKlRCKXBst 4aDTEeuoMWKIA== X-ME-Helo: [172.16.82.72] X-ME-Auth: bWFpbGhvbC52aW5jZW50QHdhbmFkb28uZnI= X-ME-Date: Mon, 10 Mar 2025 11:46:49 +0100 X-ME-IP: 124.33.176.97 Message-ID: Date: Mon, 10 Mar 2025 19:46:36 +0900 Precedence: bulk X-Mailing-List: linux-kernel@vger.kernel.org List-Id: List-Subscribe: List-Unsubscribe: MIME-Version: 1.0 User-Agent: Mozilla Thunderbird Subject: Re: [PATCH v5 1/7] bits: split the definition of the asm and non-asm GENMASK() To: David Laight Cc: Yury Norov , Lucas De Marchi , Rasmus Villemoes , Jani Nikula , Joonas Lahtinen , Rodrigo Vivi , Tvrtko Ursulin , David Airlie , Simona Vetter , Andrew Morton , linux-kernel@vger.kernel.org, intel-gfx@lists.freedesktop.org, dri-devel@lists.freedesktop.org, Andi Shyti , David Laight , Dmitry Baryshkov , Andy Shevchenko References: <20250306-fixed-type-genmasks-v5-0-b443e9dcba63@wanadoo.fr> <20250306-fixed-type-genmasks-v5-1-b443e9dcba63@wanadoo.fr> <20250306192331.2701a029@pumpkin> <20250309015853.01412484@pumpkin> <20250309102312.4ff08576@pumpkin> Content-Language: en-US From: Vincent Mailhol Autocrypt: addr=mailhol.vincent@wanadoo.fr; keydata= xjMEZluomRYJKwYBBAHaRw8BAQdAf+/PnQvy9LCWNSJLbhc+AOUsR2cNVonvxhDk/KcW7FvN LFZpbmNlbnQgTWFpbGhvbCA8bWFpbGhvbC52aW5jZW50QHdhbmFkb28uZnI+wrIEExYKAFoC GwMFCQp/CJcFCwkIBwICIgIGFQoJCAsCBBYCAwECHgcCF4AWIQTtj3AFdOZ/IOV06OKrX+uI bbuZwgUCZx41XhgYaGtwczovL2tleXMub3BlbnBncC5vcmcACgkQq1/riG27mcIYiwEAkgKK BJ+ANKwhTAAvL1XeApQ+2NNNEwFWzipVAGvTRigA+wUeyB3UQwZrwb7jsQuBXxhk3lL45HF5 8+y4bQCUCqYGzjgEZx4y8xIKKwYBBAGXVQEFAQEHQJrbYZzu0JG5w8gxE6EtQe6LmxKMqP6E yR33sA+BR9pLAwEIB8J+BBgWCgAmFiEE7Y9wBXTmfyDldOjiq1/riG27mcIFAmceMvMCGwwF CQPCZwAACgkQq1/riG27mcJU7QEA+LmpFhfQ1aij/L8VzsZwr/S44HCzcz5+jkxnVVQ5LZ4B ANOCpYEY+CYrld5XZvM8h2EntNnzxHHuhjfDOQ3MAkEK In-Reply-To: <20250309102312.4ff08576@pumpkin> Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 7bit On 09/03/2025 at 19:23, David Laight wrote: > On Sun, 9 Mar 2025 01:58:53 +0000 > David Laight wrote: > >> On Fri, 7 Mar 2025 18:58:08 +0900 >> Vincent Mailhol wrote: >> >>> On 07/03/2025 at 04:23, David Laight wrote: >>>> On Thu, 06 Mar 2025 20:29:52 +0900 >>>> Vincent Mailhol via B4 Relay wrote: >>>> >>>>> From: Vincent Mailhol >>>>> >>>>> In an upcoming change, GENMASK() and its friends will indirectly >>>>> depend on sizeof() which is not available in asm. >>>>> >>>>> Instead of adding further complexity to __GENMASK() to make it work >>>>> for both asm and non asm, just split the definition of the two >>>>> variants. >>>> ... >>>>> +#else /* defined(__ASSEMBLY__) */ >>>>> + >>>>> +#define GENMASK(h, l) __GENMASK(h, l) >>>>> +#define GENMASK_ULL(h, l) __GENMASK_ULL(h, l) >>>> >>>> What do those actually expand to now? >>>> As I've said a few times both UL(0) and ULL(0) are just (0) for __ASSEMBLY__ >>>> so the expansions of __GENMASK() and __GENMASK_ULL() contained the >>>> same numeric constants. >>> >>> Indeed, in asm, the UL(0) and ULL(0) expands to the same thing: 0. >>> >>> But the two macros still expand to something different on 32 bits >>> architectures: >>> >>> * __GENMASK: >>> >>> (((~(0)) << (l)) & (~(0) >> (32 - 1 - (h)))) >>> >>> * __GENMASK_ULL: >>> >>> (((~(0)) << (l)) & (~(0) >> (64 - 1 - (h)))) >>> >>> On 64 bits architecture these are the same. >> >> I've just fed those into godbolt (https://www.godbolt.org/z/Ter6WE9qE) as: >> int fi(void) >> { >> int v; >> asm("mov $(((~(0)) << (8)) & (~(0) >> (32 - 1 - (15)))),%0": "=r" (v)); >> return v -(((~(0u)) << (8)) & (~(0u) >> (32 - 1 - (15)))); >> } >> >> gas warns: >> :4: Warning: 0xffffffffff00 shortened to 0xffffff00 >> >> unsigned long long fll(void) >> { >> unsigned long long v; >> asm("mov $(((~(0)) << (8)) & (~(0) >> (64 - 1 - (15)))),%0": "=r" (v)); >> return v -(((~(0ull)) << (8)) & (~(0ull) >> (64 - 1 - (15)))); >> } >> >> (for other architectures you'll need to change the opcode) >> >> For x86 and x86-32 the assembler seems to be doing 64bit maths with unsigned >> right shifts - so the second function (with the 64 in it) generates 0xff00. >> I doubt a 32bit only assembler does 64bit maths, but the '>> 48' above >> might get masked to a '>> 16' by the cpu and generate the correct result. >> >> So __GENMASK() is likely to be broken for any assembler that supports 64bits >> when generating 32bit code. >> __GENMASK_ULL() works (assuming all have unsigned >>) on 64bit assemblers >> (even when generating 32bit code). It may work on some 32bit assemblers.> > I've remembered my 'pi' has a 32bit userspace (on a 64bit kernel). > I quick test of "mov %0,#(...)" and bits 11..8 gives the correct output > for size '32' but the error message: > /tmp/ccPB7bWh.s:26: Warning: shift count out of range (56 is not between 0 and 31) > with size '64'. > > Assuming that part of the gnu assembler is consistent across architectures > you can't use either GENMASK in asm for 32bit architectures. > > Any change (probably including removing the asm support for the uapi) isn't > going to make things worse! > > David > >> >> Since most uses in the header files will be GENMASK() I doubt (hope) no >> asm code actually uses the values! After reading your message, I wanted to understand where these GENMASK*() were used in asm code. It happens that most of the architectures simply don't use it! I see these are using in aarch64, but that's about it. I couldn't find any use of neither GENMASK() nor GENMASK_ULL() in x86, arm 32 bits, m68k or powerpc asm code (I did not check the other architectures). aarch64 uses both the long and long long variants, but this being a 64 bits architecture, these are the same. So OK. So, this goes into the same direction as you: I guess that the fact that no one noticed the issue is that no one uses this on a 32 bits arch, probably for historical reasons, i.e. the asm code was written before the introduction of the GENMASK() macros. >> The headers assemble - but that is about all that can be said. >> >> Bags of worms :-) +1 (I will not open that bag) I think that the asm and non asm variant should have used a different implementation from the begining. By wanting to have a single variant that fit both the asm and the C code, we have a complex result that is hard to understand and maintain (c.f. the bug which you pointed out which have been unnoticed for ever). But now that it is in the uapi, I am not sure of what is the best approach. I sincerely hope that we can just modify it with no userland impact. Well, just to make it clear, I am not touching the asm part. I acknowledge the issue, but I do not think that I have the skill to fix it. Yours sincerely, Vincent Mailhol