| 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
| ||
|
Message-ID: <39230635-184e-c897-4b32-e258567d1270@quicinc.com>
Date: Tue, 14 Oct 2025 12:26:07 +0530
From: Md Sadre Alam <quic_mdalam@...cinc.com>
To: Adrian Hunter <adrian.hunter@...el.com>, <quic_asutoshd@...cinc.com>,
<ulf.hansson@...aro.org>, <linux-mmc@...r.kernel.org>,
<linux-kernel@...r.kernel.org>, <linux-arm-msm@...r.kernel.org>,
<quic_varada@...cinc.com>
Subject: Re: [PATCH] mmc: sdhci-msm: Enable ICE support for non-cmdq eMMC
devices
On 10/13/2025 4:23 PM, Adrian Hunter wrote:
> On 13/10/2025 12:09, Md Sadre Alam wrote:
>> Hi,
>>
>> On 10/9/2025 5:59 PM, Adrian Hunter wrote:
>>> On 08/10/2025 14:07, Md Sadre Alam wrote:
>>>> Enable Inline Crypto Engine (ICE) support for eMMC devices that don't
>>>> use command queuing (CQE). This allows hardware-accelerated encryption
>>>> and decryption for standard eMMC operations without command queuing.
>>>>
>>>> The changes include:
>>>> - Add non-cmdq crypto register definitions
>>>> - Implement crypto configuration callback for non-cmdq operations
>>>> - Initialize ICE hardware during host setup for non-cmdq devices
>>>> - Integrate crypto configuration into the main request path
>>>>
>>>> This enables non-cmdq eMMC devices to benefit from hardware crypto
>>>> acceleration, improving performance for encrypted storage operations
>>>> while maintaining compatibility with existing cmdq crypto support.
>>>>
>>>> Signed-off-by: Md Sadre Alam <quic_mdalam@...cinc.com>
>>>> ---
>>>> drivers/mmc/host/cqhci.h | 4 ++
>>>> drivers/mmc/host/sdhci-msm.c | 74 +++++++++++++++++++++++++++++++++++-
>>>> drivers/mmc/host/sdhci.c | 20 ++++++++++
>>>> drivers/mmc/host/sdhci.h | 2 +
>>>> 4 files changed, 99 insertions(+), 1 deletion(-)
>>>>
>>>> diff --git a/drivers/mmc/host/cqhci.h b/drivers/mmc/host/cqhci.h
>>>> index ce189a1866b9..9bf236e27675 100644
>>>> --- a/drivers/mmc/host/cqhci.h
>>>> +++ b/drivers/mmc/host/cqhci.h
>>>> @@ -119,6 +119,10 @@
>>>> /* command response argument */
>>>> #define CQHCI_CRA 0x5C
>>>> +/* non command queue crypto enable register*/
>>>> +#define NONCQ_CRYPTO_PARM 0x70
>>>> +#define NONCQ_CRYPTO_DUN 0x74
>>>
>>> Since cqhci is not using these, they might be better in sdhci-msm.c
>> Ok
>>>
>>>> +
>>>> /* crypto capabilities */
>>>> #define CQHCI_CCAP 0x100
>>>> #define CQHCI_CRYPTOCAP 0x104
>>>> diff --git a/drivers/mmc/host/sdhci-msm.c b/drivers/mmc/host/sdhci-msm.c
>>>> index 4e5edbf2fc9b..2204c6abb3fe 100644
>>>> --- a/drivers/mmc/host/sdhci-msm.c
>>>> +++ b/drivers/mmc/host/sdhci-msm.c
>>>> @@ -157,6 +157,23 @@
>>>> #define CQHCI_VENDOR_CFG1 0xA00
>>>> #define CQHCI_VENDOR_DIS_RST_ON_CQ_EN (0x3 << 13)
>>>> +#define DISABLE_CRYPTO BIT(15)
>>>> +#define CRYPTO_GENERAL_ENABLE BIT(1)
>>>> +#define HC_VENDOR_SPECIFIC_FUNC4 0x260
>>>> +#define ICE_HCI_SUPPORT BIT(28)
>>>> +
>>>> +/* SDHCI MSM ICE CTRL Info register offset */
>>>> +enum {
>>>> + OFFSET_SDHCI_MSM_ICE_HCI_PARAM_CCI = 0,
>>>> + OFFSET_SDHCI_MSM_ICE_HCI_PARAM_CE = 8,
>>>> +};
>>>> +
>>>> +/* SDHCI MSM ICE CTRL Info register masks */
>>>> +enum {
>>>> + MASK_SDHCI_MSM_ICE_HCI_PARAM_CE = 0x1,
>>>> + MASK_SDHCI_MSM_ICE_HCI_PARAM_CCI = 0xff
>>>> +};
>>>
>>> Preferably use GENMASK() and FIELD_PREP()
>> Ok
>>>
>>>> +
>>>> struct sdhci_msm_offset {
>>>> u32 core_hc_mode;
>>>> u32 core_mci_data_cnt;
>>>> @@ -1882,9 +1899,47 @@ static void sdhci_msm_set_clock(struct sdhci_host *host, unsigned int clock)
>>>> * Inline Crypto Engine (ICE) support *
>>>> * *
>>>> \*****************************************************************************/
>>>> -
>>>
>>> Unnecessary to delete this line
>> Ok
>>>
>>>> #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 || !cq_host)
>>>> + return -EINVAL;
>>>
>>> It should not be possible to get here if (!mrq || !cq_host)
>> Ok, will remove it in next revision.
>>>
>>>> +
>>>> + if (mrq->crypto_ctx) {
>>>> + dun = mrq->crypto_ctx->bc_dun[0];
>>>> + bypass = false;
>>>> + key_index = mrq->crypto_key_slot;
>>>> + }
>>>> +
>>>> + /* Configure ICE bypass mode */
>>>> + crypto_params |= ((!bypass) & MASK_SDHCI_MSM_ICE_HCI_PARAM_CE)
>>>> + << OFFSET_SDHCI_MSM_ICE_HCI_PARAM_CE;
>>>> + /* Configure Crypto Configure Index (CCI) */
>>>> + crypto_params |= (key_index & MASK_SDHCI_MSM_ICE_HCI_PARAM_CCI)
>>>> + << OFFSET_SDHCI_MSM_ICE_HCI_PARAM_CCI;
>>>> +
>>>> + 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;
>>>> +}
>>>> +
>>>> static const struct blk_crypto_ll_ops sdhci_msm_crypto_ops; /* forward decl */
>>>> static int sdhci_msm_ice_init(struct sdhci_msm_host *msm_host,
>>>> @@ -2131,6 +2186,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 +2242,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);
>>>> +
>>>> dev_info(&pdev->dev, "%s: CQE init: success\n",
>>>> mmc_hostname(host->mmc));
>>>> return ret;
>>>> @@ -2450,6 +2519,9 @@ static const struct of_device_id sdhci_msm_dt_match[] = {
>>>> MODULE_DEVICE_TABLE(of, sdhci_msm_dt_match);
>>>> static const struct sdhci_ops sdhci_msm_ops = {
>>>> +#ifdef CONFIG_MMC_CRYPTO
>>>> + .crypto_engine_cfg = sdhci_msm_ice_cfg,
>>>> +#endif
>>>> .reset = sdhci_and_cqhci_reset,
>>>> .set_clock = sdhci_msm_set_clock,
>>>> .get_min_clock = sdhci_msm_get_min_clock,
>>>> diff --git a/drivers/mmc/host/sdhci.c b/drivers/mmc/host/sdhci.c
>>>> index ac7e11f37af7..2d636a8ee452 100644
>>>> --- a/drivers/mmc/host/sdhci.c
>>>> +++ b/drivers/mmc/host/sdhci.c
>>>> @@ -2202,6 +2202,21 @@ void sdhci_set_power_and_bus_voltage(struct sdhci_host *host,
>>>> }
>>>> EXPORT_SYMBOL_GPL(sdhci_set_power_and_bus_voltage);
>>>> +static int sdhci_crypto_cfg(struct sdhci_host *host, struct mmc_request *mrq,
>>>> + u32 slot)
>>>> +{
>>>> + int err = 0;
>>>> +
>>>> + if (host->ops->crypto_engine_cfg) {
>>>> + err = host->ops->crypto_engine_cfg(host, mrq, slot);
>>>> + if (err)
>>>> + pr_err("%s: failed to configure crypto: %d\n",
>>>> + mmc_hostname(host->mmc), err);
>>>> + }
>>>> +
>>>> + return err;
>>>> +}
>>>> +
>>>> /*****************************************************************************\
>>>> * *
>>>> * MMC callbacks *
>>>> @@ -2227,6 +2242,11 @@ void sdhci_request(struct mmc_host *mmc, struct mmc_request *mrq)
>>>> cmd = sdhci_manual_cmd23(host, mrq) ? mrq->sbc : mrq->cmd;
>>>> + if (mmc->caps2 & MMC_CAP2_CRYPTO) {
>>>> + if (sdhci_crypto_cfg(host, mrq, 0))
>>>> + goto out_finish;
>>>> + }
>>>
>>> It would be preferable to hook the >request() callback e.g.
>>>
>>> host->mmc_host_ops.request = sdhci_msm_request;
>>>
>>> void sdhci_msm_request(struct mmc_host *mmc, struct mmc_request *mrq)
>>> {
>>> if (mmc->caps2 & MMC_CAP2_CRYPTO) {
>>> etc
>>> }
>>>
>>> sdhci_request(mmc, mrq);
>>> }
>> Thanks for the suggestion. I Will update the patch to override the mmc_host_ops.request callback in sdhci-msm.c via a platform-specific wrapper (sdhci_msm_request). Since mmc->ops is a const pointer, I Will clone the existing ops into a local copy
>
> Can just update the sdhci ops directly:
>
> host->mmc_host_ops.request = sdhci_msm_request;
>
> (msm_mmc_ops) and replaced only the request field. This preserves all platform-specific callbacks like enable_sdio_irq and avoids probe failures. The change in probe function.
Thanks for the suggestion. Updating host->mmc_host_ops.request directly
to sdhci_msm_request does indeed retain the platform-specific callbacks.
I’ll incorporate this change in the next revision.
Thanks,
Alam.
Powered by blists - more mailing lists