Re: [PATCH 0/8] eMMC inline encryption support

[Eric Biggers <ebiggers@kernel.org>]

Hi Adrian,

On Mon, Nov 23, 2020 at 09:04:12AM +0200, Adrian Hunter wrote:
> On 20/11/20 9:44 pm, Eric Biggers wrote:
> > Hi Adrian,
> > 
> > On Fri, Nov 20, 2020 at 09:29:59PM +0200, Adrian Hunter wrote:
> >> I haven't had a chance to look at it properly, but I do have a couple of
> >> dumb questions.  How do you ensure the host controller is not runtime
> >> suspended when the key is programmed?
> > 
> > This is handled by the block layer, in block/keyslot-manager.c.  It ensures that
> > the device is resumed before calling blk_ksm_ll_ops::keyslot_program() or
> > blk_ksm_ll_ops::keyslot_evict().  See blk_ksm_hw_enter().
> 
> Cool, although cqhci is doing a lazy kind of resume, so maybe not be enabled
> when a key is programmed?  Would that be a problem?
> 
> > 
> >> Are the keys lost when the host controller is reset, and then how do you know
> >> the host controller does not get reset after the key is programmed but before
> >> the I/O is submitted?
> > 
> > As with UFS, keys might be lost when the host controller is reset, so we're
> > reprogramming all the keys when that happens.  See patch 1:
> > 
> >     mmc_set_initial_state()
> >         mmc_crypto_set_initial_state()
> >             blk_ksm_reprogram_all_keys()
> > 
> > (That's the intent, at least.  For MMC, I'm not sure if resets were properly
> > covered by the testing I've done so far.  But the code looks right to me.)
> 
> After reset, cqhci will not necessarily be enabled at this point.  Is that OK?

The hardware that I have (sdm630) appears to allow programming and evicting keys
even while CQHCI_CFG.CQHCI_ENABLE is clear, i.e. even when the CQE is "off".
I tested it using the patch below.

The eMMC specification isn't clear about this point.  But I'm thinking that the
crypto configuration registers (the keyslots) are probably supposed to work like
most of the other CQHCI registers, which can be written to while CQHCI_ENABLE is
clear.  Then setting CQHCI_ENABLE just enables the ability to actually issue
requests.  Likewise, setting CQHCI_CRYPTO_GENERAL_ENABLE just allows using
crypto in requests; it isn't needed to write to the crypto configurations.

For what it's worth, UFS crypto (which has been supported by upstream since
v5.9) works similarly.  Keys can be programmed while the UFS host is powered on,
even before it's "enabled".

But maybe someone interpreted the eMMC specification differently.  Hopefully
Mediatek can give some insight into how they implemented it, and test this
patchset on their hardware too.

Here's the patch I used to verify that sdm630 allows programming and evicting
keys even while the CQE is off:

diff --git a/drivers/mmc/core/block.c b/drivers/mmc/core/block.c
index eaf2f1074326..eb2d88d0b3ba 100644
--- a/drivers/mmc/core/block.c
+++ b/drivers/mmc/core/block.c
@@ -1406,6 +1406,9 @@ static void mmc_blk_cqe_complete_rq(struct mmc_queue *mq, struct request *req)
 
 	mmc_cqe_check_busy(mq);
 
+	if (mmc_tot_in_flight(mq) == 0 && host->cqe_on)
+		host->cqe_ops->cqe_off(host);
+
 	spin_unlock_irqrestore(&mq->lock, flags);
 
 	if (!mq->cqe_busy)
diff --git a/drivers/mmc/host/sdhci-msm.c b/drivers/mmc/host/sdhci-msm.c
index 6ce21414d510..70d8dbc6515f 100644
--- a/drivers/mmc/host/sdhci-msm.c
+++ b/drivers/mmc/host/sdhci-msm.c
@@ -1971,6 +1971,12 @@ static int sdhci_msm_program_key(struct cqhci_host *cq_host,
 	int i;
 	int err;
 
+	if (!cq_host->mmc->cqe_on) {
+		pr_info("@@@ cqe is off for %s slot %d\n",
+			(cfg->config_enable & CQHCI_CRYPTO_CONFIGURATION_ENABLE) ?
+			"program" : "evict", slot);
+	}
+
 	if (!(cfg->config_enable & CQHCI_CRYPTO_CONFIGURATION_ENABLE))
 		return qcom_scm_ice_invalidate_key(slot);