linux-kernel - Re: [PATCH v4] 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: Windows password security audit tool. GUI, reports in PDF.

[<prev] [next>] [<thread-prev] [thread-next>] [day] [month] [year] [list]

Message-ID: <dbea629d-5aa9-4a85-6316-25ac82a33520@quicinc.com>
Date: Thu, 13 Nov 2025 12:41:28 +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-mmc@...r.kernel.org>, <linux-arm-msm@...r.kernel.org>,
        <linux-kernel@...r.kernel.org>, <quic_varada@...cinc.com>
Subject: Re: [PATCH v4] mmc: sdhci-msm: Enable ICE support for non-cmdq eMMC
 devices

Hi,

On 11/12/2025 2:22 AM, Eric Biggers wrote:
> On Tue, Nov 11, 2025 at 04:16:04PM +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
>>
>> 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.
> 
> This really should explain that this patch actually applies only to host
> controllers that *do* support CQE.  Just they are using a card that
> doesn't support CQE or CQE was explicitly disabled.  Right?
Yes, you are absolutely correct. Thank you for pointing this out - the 
commit message should be clearer about this important detail.

This patch applies specifically to CQE-capable host controllers 
(sdhci-msm controllers that support CQHCI) when they are operating in 
non-CQE mode.
This can happen in two scenarios:

1. CQE-capable controller + non-CQE card: The host controller supports 
CQE, but the eMMC card doesn't support Command Queue Engine
2. CQE explicitly disabled: The host controller and card both support 
CQE, but CQE has been explicitly disabled (e.g., via device tree)

For the 2nd case I will post another path which will handle host side
CQE enable/disable.
> 
>> +static void sdhci_msm_non_cqe_ice_init(struct sdhci_host *host)
>> +{
>> +	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;
>> +	u32 config;
>> +	u32 ice_cap;
>> +
>> +	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);
>> +}
>> +
>> +static int sdhci_msm_ice_cfg(struct sdhci_host *host, struct mmc_request *mrq)
>> +{
>> +	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;
>> +	bool crypto_enable;
>> +	u64 dun = 0;
>> +
>> +	if (mrq->crypto_ctx) {
>> +		if (!msm_host->ice_init_done) {
>> +			sdhci_msm_non_cqe_ice_init(host);
>> +			msm_host->ice_init_done = true;
>> +		}
> 
> This means sdhci_msm_ice_enable() is called only once per host
> controller.  It looks like the existing call to sdhci_msm_ice_enable()
> happens each time after the host controller is resumed.  So there seems
> to be an inconsistency there.  Which way is correct?
Thank you for highlighting this. After revisiting the code paths, I 
believe the behavior is consistent across both CQE and non-CQE modes.
ICE is re-enabled on every resume via the common 
sdhci_msm_runtime_resume() → sdhci_msm_ice_resume() → qcom_ice_resume() 
→ sdhci_msm_ice_enable() path.
The ice_init_done flag only governs one-time initialization in 
sdhci_msm_ice_cfg() and doesn’t interfere with the resume logic.

In summary:
CQE mode: ICE enabled during sdhci_msm_cqe_enable() + every resume
Non-CQE mode: ICE enabled on first crypto request + every resume
> 
>> +	} else {
>> +		crypto_enable = false;
>> +		key_index = 0;
>> +		cqhci_writel(cq_host, crypto_params, NONCQ_CRYPTO_PARM);
> 
> The values assigned to 'crypto_enable' and 'key_index' are never used.
Ok, will remove in next revision.
> 
>> +static void sdhci_msm_request(struct mmc_host *mmc, struct mmc_request *mrq)
>> +{
> 
> Could you leave a comment here that notes this is used only for non-CQE
> requests and that crypto on CQE requests is handled elsewhere?
Thank you very much — that’s a valuable suggestion. Adding a comment 
will make the code much clearer. Will add in next revision.

Thanks,
Alam.