From:   "Bae, Chang Seok" <chang.seok.bae@intel.com>
To:     Eric Biggers <ebiggers@kernel.org>
CC:     Thomas Gleixner <tglx@linutronix.de>, Borislav Petkov <bp@suse.de>,
        "Dave Hansen" <dave.hansen@linux.intel.com>,
        "mingo@kernel.org" <mingo@kernel.org>,
        "Lutomirski, Andy" <luto@kernel.org>,
        "x86@kernel.org" <x86@kernel.org>,
        "herbert@gondor.apana.org.au" <herbert@gondor.apana.org.au>,
        "linux-kernel@vger.kernel.org" <linux-kernel@vger.kernel.org>,
        "linux-crypto@vger.kernel.org" <linux-crypto@vger.kernel.org>,
        "Williams, Dan J" <dan.j.williams@intel.com>,
        "Gairuboyina, Charishma1" <charishma1.gairuboyina@intel.com>,
        "Dwarakanath, Kumar N" <kumar.n.dwarakanath@intel.com>,
        "Krishnakumar, Lalithambika" <lalithambika.krishnakumar@intel.com>,
        "Shankar, Ravi V" <ravi.v.shankar@intel.com>
Subject: Re: [PATCH v3 11/15] crypto: x86/aes-kl - Support AES algorithm using
 Key Locker instructions
Thread-Topic: [PATCH v3 11/15] crypto: x86/aes-kl - Support AES algorithm
 using Key Locker instructions
Thread-Index: AQHX4W/kZoA3rQNJf0mSmN0oRVVbp6wbd4YAgAA0pIA=
Date:   Tue, 30 Nov 2021 06:57:22 +0000
Message-ID: <011B53ED-6D9E-41EB-834B-8A64485DBED5@intel.com>
References: <20211124200700.15888-1-chang.seok.bae@intel.com>
 <20211124200700.15888-12-chang.seok.bae@intel.com>
 <YaWfKB+k66MzNtIi@sol.localdomain>
In-Reply-To: <YaWfKB+k66MzNtIi@sol.localdomain>
Accept-Language: en-US
Content-Language: en-US
x-ms-exchange-antispam-messagedata-chunkcount: 1
x-ms-exchange-antispam-messagedata-0: =?us-ascii?Q?VdxbpwMfW3WweDJIrTulHHHHZ/+WSm4FDBSwiRRxv5012d7NO5Bzyw2bzPO5?=
 =?us-ascii?Q?Z5+/YPca989e5OYcJFOLgt/xTtNwpTPJYJNwjV9Z8VytZmb8+gT5tpF2x7sA?=
 =?us-ascii?Q?vL76TG6jebV2H7hsBEasYI1xJD4ZLjNyThobzhflMRjfHvEXfHOeilg6BAbY?=
 =?us-ascii?Q?yXnABZ20pqlB798c6X3VgdjFCKpkqjHdM/OGSb/AAKIyRQNdNvBtl2k/BTNg?=
 =?us-ascii?Q?4U8qKfohtM1dkLTHWzZp/b6DLpCqaBDl432hdlEYVbW7hQTVQAa28aoConwY?=
 =?us-ascii?Q?SnSB5PSkvA1UlPO9ECIgelMIK1cGQVBD9FzN5LZSAEtL6msroFeWKpMnNzeX?=
 =?us-ascii?Q?IJAJik8hmz8WfR49X4cYbRUp1QTZrpxtQ3/uJQocqY0q8pk2J15KuXEDg3+k?=
 =?us-ascii?Q?FzjMl7Noc62fGDaE4/wcRNxsxALb+SY2QhzHGgZqSSV9gGqk2U5Zg/6fg1Nj?=
 =?us-ascii?Q?If0jZxL102noc4bWEwELuiMrg3QleyXYMwUyY9kecdZgichtqFX01DpqDfNK?=
 =?us-ascii?Q?Rp5bxdM3Dx1Qumm0EWsl8cm9Iv9gLyhQu35iKXP1nFOeAMCl0OkdWikZgzt9?=
 =?us-ascii?Q?c22awOybOiEZ+H3enCxsyxzU/bGpIEAZ4PM7ruwDRDfyV9XfaXy8DooP8DZq?=
 =?us-ascii?Q?wfUtrPAWFu5VRjeiGDO/akF4KY2FWWKGe/jvOhd4JNTSsLTzK6AvTdhYNk69?=
 =?us-ascii?Q?Mvue/o3mkgGTW9aLXiL1shnvLAnTAwaWkiXsnyOQwr38xNsbecoEwMTdTFON?=
 =?us-ascii?Q?VVvkM4JyMYuEL031Bkd85M/MMH9ljVhd2cPTWe0a3knjWtEweyoXrLnIh/jp?=
 =?us-ascii?Q?62ESLdSKy6b7qDZTBIyEKQsvtLFjZxWYe0BCe9D4OfWN+OgCulFCY7jealmp?=
 =?us-ascii?Q?VCNOBL7j1xoJ56Uj9QTxwcjoQHwWkyXrU80RnyezLbxl8HC31xZw2jOoSrAe?=
 =?us-ascii?Q?l/o2OvxesM6Ka8CvUlxQtEYcTZq5B8We7Yl547Fwj32YV9tLLNpHJZcWqe8O?=
 =?us-ascii?Q?Mk2hAU/afnYd92SrhndG9b33sw+Bfv1urVpzWQ0c3JZhi48Jo+9mYprq4tbA?=
 =?us-ascii?Q?Rc1ezfr5ZqWVxT0YO1hBBFw0/Zu6xw+K5CSqOKazJwwv0ZBHCO2Lvrji58y/?=
 =?us-ascii?Q?UiBLaIppbIvq2HMXjqEIjLxKqnhdXvtN3On+0LsxzHS2RGn4AuB61f77CdA6?=
 =?us-ascii?Q?bau5K5ROJI/cKlf9s32RbOzJjEFLa1g8h+juyWl/pIWF8ph+59framANfyvD?=
 =?us-ascii?Q?lKp2x+TmoKEe//2HdPMbqL049v4VMOEOxpk5dfBwNKE3gh/+2j9yRVugjj7u?=
 =?us-ascii?Q?rHen4AkNCD1t529wo3fyGjkHlSiSm20V1Fqupyt5D2YNdB7Ob7ESwnjaS2HD?=
 =?us-ascii?Q?RqMAjBrxut1RIAuVbHeevYnSMCNC8sd3b+GDn1xMOtfiHTLqCjVZHt/Jz1gK?=
 =?us-ascii?Q?HsiCRC5beuocLgfYEG1u1JCGUsUnVzZkBew6y0rLhwUClLQfItQLLNIsWBlh?=
 =?us-ascii?Q?VwDgGOjFQh8Hywr/jz5GDav3WhAG/bEmaJDynjMY8evKE6RfmQsr6EBN3i2O?=
 =?us-ascii?Q?7LCDEyqV6dIxW579w1WLlth0I/c/m8F3ZCCP2W6pgTAiwVroUPEsSLhNen0u?=
 =?us-ascii?Q?0+ZYEV4VYNjmU2FtCGouwjaPe8Mm8fMP7HJxmAAeJvFU6qz5r49L4kvTbHyv?=
 =?us-ascii?Q?uTZBNPzAMaFOgohAFJLypNPN3FA=3D?=
Content-Type: text/plain; charset="us-ascii"
Content-ID: <C42491AEBD632742B6677015D7A2A78F@namprd11.prod.outlook.com>
Content-Transfer-Encoding: quoted-printable
MIME-Version: 1.0
X-MS-Exchange-CrossTenant-AuthAs: Internal
X-MS-Exchange-CrossTenant-AuthSource: PH0PR11MB4855.namprd11.prod.outlook.com
X-MS-Exchange-CrossTenant-Network-Message-Id: a3d55da6-60d8-42ad-84d5-08d9b3cea895
X-MS-Exchange-CrossTenant-originalarrivaltime: 30 Nov 2021 06:57:22.1038
 (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: BWEA1VFkGLLl1G/FGQPJ3Oxbg5mnk2KA/lg21abR5Cuy+NONCAi8fYq2grq/qFKH05Aym+HosKBet1Uj3pG3hkz49x9VxxrA0YgZZBv8BOI=
X-MS-Exchange-Transport-CrossTenantHeadersStamped: PH7PR11MB5817
X-OriginatorOrg: intel.com
Precedence: bulk
List-ID: <linux-crypto.vger.kernel.org>
X-Mailing-List: linux-crypto@vger.kernel.org

On Nov 29, 2021, at 19:48, Eric Biggers <ebiggers@kernel.org> wrote:
> On Wed, Nov 24, 2021 at 12:06:56PM -0800, Chang S. Bae wrote:
>> diff --git a/arch/x86/crypto/Makefile b/arch/x86/crypto/Makefile
>> index ef6c0b9f69c6..f696b037faa5 100644
>> --- a/arch/x86/crypto/Makefile
>> +++ b/arch/x86/crypto/Makefile
>> @@ -50,6 +50,9 @@ obj-$(CONFIG_CRYPTO_AES_NI_INTEL) +=3D aesni-intel.o
>> aesni-intel-y :=3D aesni-intel_asm.o aesni-intel_glue.o aes-intel_glue.o
>> aesni-intel-$(CONFIG_64BIT) +=3D aesni-intel_avx-x86_64.o aes_ctrby8_avx=
-x86_64.o
>>=20
>> +obj-$(CONFIG_CRYPTO_AES_KL) +=3D aeskl-intel.o
>> +aeskl-intel-y :=3D aeskl-intel_asm.o aesni-intel_asm.o aeskl-intel_glue=
.o aes-intel_glue.o
>=20
> This makes the object files aesni-intel_asm.o and aes-intel_glue.o each b=
e built
> into two separate kernel modules.  My understanding is that duplicating c=
ode
> like that is generally frowned upon.  These files should either be built =
into a
> separate module, which both aesni-intel.ko and aeskl-intel.ko would depen=
d on,
> or aeskl-intel.ko should depend on aesni-intel.ko.

The only reason to include the AES-NI object here is that AES-KL does not
support the 192-bit key.

Maybe the fallback can be the aes-generic driver [1] instead of AES-NI here=
.

>> diff --git a/arch/x86/crypto/aeskl-intel_asm.S b/arch/x86/crypto/aeskl-i=
ntel_asm.S
>> new file mode 100644
>> index 000000000000..d56ec8dd6644
>> --- /dev/null
>> +++ b/arch/x86/crypto/aeskl-intel_asm.S
>=20
> This file gets very long after all the modes are added (> 1100 lines).  I=
s there
> really no feasible way to share code between this and aesni-intel_asm.S, =
similar
> to how the arm64 AES implementations work?  Surely most of the logic is t=
he
> same, and it's just the actual AES instructions that differ?

No, these two instruction sets are separate. So I think no room to share th=
e
ASM code.

>> +config CRYPTO_AES_KL
>> +	tristate "AES cipher algorithms (AES-KL)"
>> +	depends on (LD_VERSION >=3D 23600) || (LLD_VERSION >=3D 120000)
>> +	depends on DM_CRYPT
>=20
> 'depends on DM_CRYPT' doesn't really make sense here, since there is no a=
ctual
> dependency on dm-crypt in the code.

I think the intention here is to build a policy that the library is availab=
le
only when there is a clear use case.

But maybe putting such restriction is too much here.

Thanks
Chang

[1] https://git.kernel.org/pub/scm/linux/kernel/git/torvalds/linux.git/tree=
/crypto/aes_generic.c