linux-kernel - Re: [PATCH v2] mmc: sdhci-msm: Enable ICE support for non-cmdq eMMC devices

lists.openwall.net		lists / announce owl-users owl-dev john-users john-dev passwdqc-users yescrypt popa3d-users / oss-security kernel-hardening musl sabotage tlsify passwords / crypt-dev xvendor / Bugtraq Full-Disclosure linux-kernel linux-netdev linux-ext4 linux-hardening linux-cve-announce PHC
Open Source and information security mailing list archives
Hash Suite for Android: free password hash cracker in your pocket
[<prev] [next>] [<thread-prev] [thread-next>] [day] [month] [year] [list]
Message-ID: <abe89411-e590-29df-e9e9-b50402e08aac@quicinc.com>
Date: Wed, 22 Oct 2025 10:49:23 +0530
From: Md Sadre Alam <quic_mdalam@...cinc.com>
To: Eric Biggers <ebiggers@...nel.org>
CC: <adrian.hunter@...el.com>, <ulf.hansson@...aro.org>,
        <linux-arm-msm@...r.kernel.org>, <linux-mmc@...r.kernel.org>,
        <linux-kernel@...r.kernel.org>, <quic_varada@...cinc.com>
Subject: Re: [PATCH v2] mmc: sdhci-msm: Enable ICE support for non-cmdq eMMC
 devices

Hi,

On 10/17/2025 11:08 PM, Eric Biggers wrote:
> On Tue, Oct 14, 2025 at 03:05:03PM +0530, Md Sadre Alam wrote:
>> Enable Inline Crypto Engine (ICE) support for eMMC devices that operate
>> without Command Queue Engine (CQE).This allows hardware-accelerated
>> encryption and decryption for standard (non-CMDQ) requests.
>>
>> This patch:
>> - Adds ICE register definitions for non-CMDQ crypto configuration
>> - Implements a per-request crypto setup via sdhci_msm_ice_cfg()
>> - Hooks into the request path via mmc_host_ops.request
>> - Initializes ICE hardware during CQE setup for compatible platforms
>>
>> With this, non-CMDQ eMMC devices can benefit from inline encryption,
>> improving performance for encrypted I/O while maintaining compatibility
>> with existing CQE crypto support.
>>
>> Signed-off-by: Md Sadre Alam <quic_mdalam@...cinc.com>
> 
> How was this tested?
I tested this using fscrypt on a Phison eMMC device. However, since that 
particular eMMC does not support CMDQ, inline encryption (ICE) was 
bypassed during testing.
> 
>>   #ifdef CONFIG_MMC_CRYPTO
>>   
>> +static int sdhci_msm_ice_cfg(struct sdhci_host *host, struct mmc_request *mrq,
>> +			     u32 slot)
>> +{
>> +	struct sdhci_pltfm_host *pltfm_host = sdhci_priv(host);
>> +	struct sdhci_msm_host *msm_host = sdhci_pltfm_priv(pltfm_host);
>> +	struct mmc_host *mmc = msm_host->mmc;
>> +	struct cqhci_host *cq_host = mmc->cqe_private;
>> +	unsigned int crypto_params = 0;
>> +	int key_index = 0;
>> +	bool bypass = true;
>> +	u64 dun = 0;
>> +
>> +	if (mrq->crypto_ctx) {
>> +		dun = mrq->crypto_ctx->bc_dun[0];
>> +		bypass = false;
>> +		key_index = mrq->crypto_key_slot;
>> +	}
>> +
>> +	crypto_params = FIELD_PREP(ICE_HCI_PARAM_CE, !bypass) |
>> +			FIELD_PREP(ICE_HCI_PARAM_CCI, key_index);
>> +
>> +	cqhci_writel(cq_host, crypto_params, NONCQ_CRYPTO_PARM);
>> +
>> +	if (mrq->crypto_ctx)
>> +		cqhci_writel(cq_host, lower_32_bits(dun), NONCQ_CRYPTO_DUN);
>> +
>> +	/* Ensure crypto configuration is written before proceeding */
>> +	wmb();
>> +
>> +	return 0;
>> +}
> 
> This would probably be easier to read with separate code paths for
> crypto_ctx != NULL and crypto_ctx == NULL.  Also 'bypass' should be
> inverted and renamed to 'crypto_enable' to match the bitfield.  Or just
> prepare the bitfield directly, without an intermediate variable.
Thanks for the suggestion. I agree that separating the logic based
on crypto_ctx presence improves readability.I’ll refactor the function
to use distinct code paths for crypto_ctx != NULL and crypto_ctx == 
NULL, and rename bypass to crypto_enable to better reflect the bitfield 
semantics.I’ll also remove the intermediate variable and prepare 
crypto_params directly as recommended.
> 
>> @@ -2131,6 +2185,8 @@ static int sdhci_msm_cqe_add_host(struct sdhci_host *host,
>>   	struct cqhci_host *cq_host;
>>   	bool dma64;
>>   	u32 cqcfg;
>> +	u32 config;
>> +	u32 ice_cap;
>>   	int ret;
>>   
>>   	/*
>> @@ -2185,6 +2241,18 @@ static int sdhci_msm_cqe_add_host(struct sdhci_host *host,
>>   	if (ret)
>>   		goto cleanup;
>>   
>> +	/* Initialize ICE for non-CMDQ eMMC devices */
>> +	config = sdhci_readl(host, HC_VENDOR_SPECIFIC_FUNC4);
>> +	config &= ~DISABLE_CRYPTO;
>> +	sdhci_writel(host, config, HC_VENDOR_SPECIFIC_FUNC4);
>> +	ice_cap = cqhci_readl(cq_host, CQHCI_CAP);
>> +	if (ice_cap & ICE_HCI_SUPPORT) {
>> +		config = cqhci_readl(cq_host, CQHCI_CFG);
>> +		config |= CRYPTO_GENERAL_ENABLE;
>> +		cqhci_writel(cq_host, config, CQHCI_CFG);
>> +	}
>> +	sdhci_msm_ice_enable(msm_host);
> 
> This is after __sdhci_add_host() was called, which is probably too late.
ok,I’ll move the ICE initialization earlier in the probe flow, ideally 
before __sdhci_add_host() is called.
> 
>> +#ifdef CONFIG_MMC_CRYPTO
>> +	host->mmc_host_ops.request = sdhci_msm_request;
>> +#endif
>>   	/* Set the timeout value to max possible */
>>   	host->max_timeout_count = 0xF;
> 
> A lot of the code in this patch also seems to actually run on
> CQE-capable hosts.  Can you explain?  Why is it needed?  Is there any
> change in behavior on them?
Thanks for raising this. You're right that some parts of the patch 
interact with CQE-related structures, such as cqhci_host, even on 
CQE-capable hosts. However, the intent is to reuse existing CQE 
infrastructure (like register access helpers and memory-mapped regions) 
to configure ICE for non-CMDQ requests.

Importantly, actual CQE functionality is only enabled if the eMMC device
advertises CMDQ support. For devices without CMDQ, the CQE engine 
remains disabled, and the request path falls back to standard non-CMDQ 
flow. In this case, we're simply leveraging the CQE register space to 
program ICE parameters.

So while the code runs on CQE-capable hosts, there's no change in 
behavior for CMDQ-enabled devices — the patch does not interfere with 
CQE operation. It only enables ICE for non-CMDQ requests when supported 
by the platform.

Thanks,
Alam.