Received-SPF: pass (google.com: domain of linux-kernel-owner@vger.kernel.org designates 2620:137:e000::1:20 as permitted sender) client-ip=2620:137:e000::1:20;
Content-Type: text/plain; charset="utf-8"
Content-Transfer-Encoding: quoted-printable
In-Reply-To: <20230509051403.2748545-3-lucas.demarchi@intel.com>
References: <20230509051403.2748545-1-lucas.demarchi@intel.com> <20230509051403.2748545-3-lucas.demarchi@intel.com>
Subject: Re: [Intel-xe] [PATCH 2/3] linux/bits.h: Add fixed-width GENMASK and BIT macros
From:   Gustavo Sousa <gustavo.sousa@intel.com>
CC:     Masahiro Yamada <masahiroy@kernel.org>,
        Kevin Brodsky <kevin.brodsky@arm.com>,
        Lucas De Marchi <lucas.demarchi@intel.com>,
        <linux-kernel@vger.kernel.org>,
        Christian =?utf-8?q?K=C3=B6nig?= <christian.koenig@amd.com>,
        Alex Deucher <alexander.deucher@amd.com>,
        "Thomas Gleixner" <tglx@linutronix.de>,
        Andy Shevchenko <andriy.shevchenko@linux.intel.com>,
        Andrew Morton <akpm@linux-foundation.org>
To:     Lucas De Marchi <lucas.demarchi@intel.com>,
        <dri-devel@lists.freedesktop.org>,
        <intel-gfx@lists.freedesktop.org>, <intel-xe@lists.freedesktop.org>
Date:   Tue, 9 May 2023 11:00:36 -0300
Message-ID: <168364083689.27719.2337781022536351304@gjsousa-mobl2>
User-Agent: alot/0.10
MIME-Version: 1.0
X-MS-Exchange-AntiSpam-MessageData-ChunkCount: 1
X-MS-Exchange-AntiSpam-MessageData-0: =?utf-8?B?SjBEeW5wbWpFL1lxZDB5WTFYS1ZteE9oVi81NjV2TzBiUVZidlhvVTllT3dx?=
 =?utf-8?B?OUh5NEZiRnBVaXUvOGtqUnZOT0srTUtjMW9CVkk2aDkwbHczSWI0MmUxd0Uv?=
 =?utf-8?B?b3RYOVljNXNHeDRuTU5JWkJQRWZXVGlLV2dTbTFSZmt2VGhCY2hOandMd1RU?=
 =?utf-8?B?cmNsT3YzU1FmWU1wRjY5ZHNQTkZhQ0lCSHJ6RTBTSHhVYW1aL1M3emhCQjNr?=
 =?utf-8?B?N1puZENHSmlrSkhjU1BqbFFwcnFES1l6TkNCNC9renduQWNsZkdnTFZ1UUVo?=
 =?utf-8?B?NmJKT0RyZlVDNjZGSVJ5eEVSWnIwTTJsNmZKbU9Rd2dNOGdzTmsvWHhYRG1V?=
 =?utf-8?B?dlRkYnVwS3hRRlNiVjBGY3hoaGNyR0pMa2YrYXVlMFFhb2YrbUsrSENncUdz?=
 =?utf-8?B?T2htcHNGQXpqRmVlZTJZcnNVczRZYTdkOEhNVFBYQ0MvTktxRG50UFkxMHF6?=
 =?utf-8?B?eWN2dlBObVNSek53clg2U2czbFFweE1qQUlsMzVtOU5KQ25Nc285M0tpdjNY?=
 =?utf-8?B?aDF0djZDVEVWS25DV1lueUV5LzZpUGJ0TlZJalNocm4yaW91Ym9FczAxRnNl?=
 =?utf-8?B?Tm5oVExDZC91ay8wWkVGcFBRVUluMjBjTmljL1J5dWpkUGZkQXluVGdvUlRi?=
 =?utf-8?B?UlV0NGIxOHRobzhsMlY1MXR0MjhFN0ZlcFJtTCt5RmJVYXZHOENZV1c4dnVn?=
 =?utf-8?B?bXZmRjltUGsyRngyUFFjTDNpbVVZY1MzaVFZQXRneldOa2VBSXdicFZIVWJ6?=
 =?utf-8?B?NU9TUVcvQjVlVCszQzJxSk01QmNpbSsrZWpNNGt3WE83dkVXYWlMcm9OMEl3?=
 =?utf-8?B?WTdGMEtrTkptSWlHbFlvdnNKZ09GRm9Pckd3Ukpvejh0YUdNZ2x3VzVsektD?=
 =?utf-8?B?YlRZRTRRTzZpTkxjckY2c0tlYzNuWThTNzF4WDNRc255TC9nelFlQXlnUFNj?=
 =?utf-8?B?c2NkVWJmVGE1QUwweFN5QTI3T1hkOWt5M2syZHpJL2NweXlKa0lLN2lkbmdo?=
 =?utf-8?B?RnNNZG1QazV5ZFdiQjVaMnF4OElFUXNycjlKRTFMek1zVEtIU2R6d1R4eG5N?=
 =?utf-8?B?QlJISkVkSlFWOW9xNXdSZGlFYnhxazE4cDhUemRRNFFRZ3UwSFBIblUzTFJK?=
 =?utf-8?B?ZFcrZ3VLVURmQzVlWUd5MWJmNnBvbEhMQ3JmbjlzaklpQ3FFNFZob29TU253?=
 =?utf-8?B?SGZGSEk4eU4xWEhLaDhsanMxS05TZWo4R2s3aHpoYkxRNzJhYzJqVkFiQ1cr?=
 =?utf-8?B?TjVCRTBaYVNzVVJ6SmphRWd4eHM2NEo0UHhJVkNCSjlVNk9HMjlsU00wdno4?=
 =?utf-8?B?QlZ4bXp5U2R0UW01Y0pFQlptczY3U1VxNkFneVMyOFFabXBFcnVYQUtEM2tT?=
 =?utf-8?B?a0czcEJVQnFtOG91R3h2N1hJSzZ3SXpnSzR2TE9uYWwvSGZPNUxyVWZIcDdF?=
 =?utf-8?B?bWNyUnI1c1dzYWNFTVN3VDhZc2IzU2JWeGlQeTQ4ZndqV1NDWjBOem8yd2FB?=
 =?utf-8?B?ODB5Ym5ucndmZFVSTGZUa29vVHcwOFBDUEZzeVkrUUJLSGFid1RGblhTS01s?=
 =?utf-8?B?enBkR3FEbzBRRWhnaThFL2J0SXZrVkxJQTBCcDdoSklGMWZqYzRmZHJEWG9o?=
 =?utf-8?B?U2hreWhTMkpWR0JiaG5Sc1huWksrRWlCZXRmem1yYW5mbzM1NEQxMlVocTVR?=
 =?utf-8?B?akt6SWg4ZUswUVlRdDhFcGl6Q0tNdjBXNEQxYTVZNGlkL3ByakJzZVZFOFM3?=
 =?utf-8?B?dis3TW44YWxrclpQclNtbVlZTW9aVnZ3MTltYkhIUXZMbmFxV0hZM05KdG43?=
 =?utf-8?B?Syt2TS8xbUpvakVPZDhzYytWUE9Tb2FsTmVOYk02YzYxbGo1dWZxVXBMWHdt?=
 =?utf-8?B?QlppQkcrQ1I2bWQrNW9HZ1EvS1RpN21adVpHd0tocUgzQ1dVZEttMENmZmUv?=
 =?utf-8?B?MisvaGVQMUlzZnhGcmRBcGJYRHQyRUVXTGhmYzRWL0ZrSzZuQVMvaG4xY1Vu?=
 =?utf-8?B?bkVZcm55STZGU2dBSEhhWGc2RGlmQ0VTUUhld0Z1UUhOZnNzUXU5TVpzaHJK?=
 =?utf-8?B?WlRLTlBwOWRUbmF3cE1KdFJGNXZYUm52T1N3VUpuYVl5V2pObjRRcUNaRE1a?=
 =?utf-8?B?SnAxMjFSVmIzdy9aT3NQU2dPV2c3OTNscnMyZnJYUXZPMDMweE9Oc0JTcVpN?=
 =?utf-8?B?K3c9PQ==?=
X-MS-Exchange-CrossTenant-Network-Message-Id: fd0bd7e5-6ba8-436d-a01e-08db5095cd3e
X-MS-Exchange-CrossTenant-AuthSource: BN8PR11MB3556.namprd11.prod.outlook.com
X-MS-Exchange-CrossTenant-AuthAs: Internal
X-MS-Exchange-CrossTenant-OriginalArrivalTime: 09 May 2023 14:00:53.2491
 (UTC)
X-MS-Exchange-CrossTenant-FromEntityHeader: Hosted
X-MS-Exchange-CrossTenant-Id: 46c98d88-e344-4ed4-8496-4ed7712e255d
X-MS-Exchange-CrossTenant-MailboxType: HOSTED
X-MS-Exchange-CrossTenant-UserPrincipalName: v2ZTDXqJa4cC5S2aMcW1tkxZazUKk1nCmv4k71NnnHtnKihHoViFUX/Yw70F0uNvitg01lFxGyXg9dOzuiHCzQ==
X-MS-Exchange-Transport-CrossTenantHeadersStamped: BL3PR11MB5698
X-OriginatorOrg: intel.com
X-Spam-Status: No, score=-4.4 required=5.0 tests=BAYES_00,DKIMWL_WL_HIGH,
        DKIM_SIGNED,DKIM_VALID,DKIM_VALID_AU,DKIM_VALID_EF,RCVD_IN_DNSWL_MED,
        SPF_HELO_NONE,SPF_NONE,T_SCC_BODY_TEXT_LINE,URIBL_BLOCKED
        autolearn=ham autolearn_force=no version=3.4.6
X-Spam-Checker-Version: SpamAssassin 3.4.6 (2021-04-09) on
        lindbergh.monkeyblade.net
Precedence: bulk
List-ID: <linux-kernel.vger.kernel.org>
X-Mailing-List: linux-kernel@vger.kernel.org

Quoting Lucas De Marchi (2023-05-09 02:14:02)
>Add GENMASK_U32(), GENMASK_U16() and GENMASK_U8()  macros to create
>masks for fixed-width types and also the corresponding BIT_U32(),
>BIT_U16() and BIT_U8().
>
>All of those depend on a new "U" suffix added to the integer constant.
>Due to naming clashes it's better to call the macro U32. Since C doesn't
>have a proper suffix for short and char types, the U16 and U18 variants
>just use U32 with one additional check in the BIT_* macros to make
>sure the compiler gives an error when the those types overflow.
>The BIT_U16() and BIT_U8() need the help of GENMASK_INPUT_CHECK(),
>as otherwise they would allow an invalid bit to be passed. Hence
>implement them in include/linux/bits.h rather than together with
>the other BIT* variants.
>
>The following test file is is used to test this:
>
>        $ cat mask.c
>        #include <linux/types.h>
>        #include <linux/bits.h>
>
>        static const u32 a =3D GENMASK_U32(31, 0);
>        static const u16 b =3D GENMASK_U16(15, 0);
>        static const u8 c =3D GENMASK_U8(7, 0);
>        static const u32 x =3D BIT_U32(31);
>        static const u16 y =3D BIT_U16(15);
>        static const u8 z =3D BIT_U8(7);
>
>        #if FAIL
>        static const u32 a2 =3D GENMASK_U32(32, 0);
>        static const u16 b2 =3D GENMASK_U16(16, 0);
>        static const u8 c2 =3D GENMASK_U8(8, 0);
>        static const u32 x2 =3D BIT_U32(32);
>        static const u16 y2 =3D BIT_U16(16);
>        static const u8 z2 =3D BIT_U8(8);
>        #endif
>
>Signed-off-by: Lucas De Marchi <lucas.demarchi@intel.com>
>---
> include/linux/bits.h       | 22 ++++++++++++++++++++++
> include/uapi/linux/const.h |  2 ++
> include/vdso/const.h       |  1 +
> 3 files changed, 25 insertions(+)
>
>diff --git a/include/linux/bits.h b/include/linux/bits.h
>index 7c0cf5031abe..ff4786c99b8c 100644
>--- a/include/linux/bits.h
>+++ b/include/linux/bits.h
>@@ -42,4 +42,26 @@
> #define GENMASK_ULL(h, l) \
>        (GENMASK_INPUT_CHECK(h, l) + __GENMASK_ULL(h, l))
>=20
>+#define __GENMASK_U32(h, l) \
>+  (((~U32(0)) - (U32(1) << (l)) + 1) & \
>+   (~U32(0) >> (32 - 1 - (h))))
>+#define GENMASK_U32(h, l) \
>+  (GENMASK_INPUT_CHECK(h, l) + __GENMASK_U32(h, l))
>+
>+#define __GENMASK_U16(h, l) \
>+  ((U32(0xffff) - (U32(1) << (l)) + 1) & \
>+   (U32(0xffff) >> (16 - 1 - (h))))
>+#define GENMASK_U16(h, l) \
>+  (GENMASK_INPUT_CHECK(h, l) + __GENMASK_U16(h, l))
>+
>+#define __GENMASK_U8(h, l) \
>+  (((U32(0xff)) - (U32(1) << (l)) + 1) & \
>+   (U32(0xff) >> (8 - 1 - (h))))
>+#define GENMASK_U8(h, l) \
>+  (GENMASK_INPUT_CHECK(h, l) + __GENMASK_U8(h, l))

I wonder if we should use BIT_U* variants in the above to ensure the values=
 are
valid. If so, we get a nice boundary check and we also can use a single
definition for the mask generation:

  #define __GENMASK_U32(h, l) \
          (((~U32(0)) - (U32(1) << (l)) + 1) & \
           (~U32(0) >> (32 - 1 - (h))))
  #define GENMASK_U32(h, l) \
          (GENMASK_INPUT_CHECK(h, l) + __GENMASK_U32(BIT_U32(h), BIT_U32(l)=
))
  #define GENMASK_U16(h, l) \
          (GENMASK_INPUT_CHECK(h, l) + __GENMASK_U32(BIT_U16(h), BIT_U16(l)=
))
  #define GENMASK_U8(h, l) \
          (GENMASK_INPUT_CHECK(h, l) + __GENMASK_U32(BIT_U8(h), BIT_U8(l)))

>+
>+#define BIT_U32(nr)       _BITU32(nr)
>+#define BIT_U16(nr)       (GENMASK_INPUT_CHECK(16 - 1, nr) + (U32(1) << (=
nr)))
>+#define BIT_U8(nr)        (GENMASK_INPUT_CHECK(32 - 1, nr) + (U32(1) << (=
nr)))

Shouldn't this be GENMASK_INPUT_CHECK(8 - 1, nr)?

--
Gustavo Sousa

>+
> #endif /* __LINUX_BITS_H */
>diff --git a/include/uapi/linux/const.h b/include/uapi/linux/const.h
>index a429381e7ca5..3a4e152520f4 100644
>--- a/include/uapi/linux/const.h
>+++ b/include/uapi/linux/const.h
>@@ -22,9 +22,11 @@
> #define _AT(T,X)       ((T)(X))
> #endif
>=20
>+#define _U32(x)           (_AC(x, U))
> #define _UL(x)         (_AC(x, UL))
> #define _ULL(x)                (_AC(x, ULL))
>=20
>+#define _BITU32(x)        (_U32(1) << (x))
> #define _BITUL(x)      (_UL(1) << (x))
> #define _BITULL(x)     (_ULL(1) << (x))
>=20
>diff --git a/include/vdso/const.h b/include/vdso/const.h
>index 94b385ad438d..417384a9795b 100644
>--- a/include/vdso/const.h
>+++ b/include/vdso/const.h
>@@ -4,6 +4,7 @@
>=20
> #include <uapi/linux/const.h>
>=20
>+#define U32(x)            (_U32(x))
> #define UL(x)          (_UL(x))
> #define ULL(x)         (_ULL(x))
>=20
>--=20
>2.40.1
>