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

[Stanley Chu <stanley.chu@mediatek.com>]

Hi Eric,

On Mon, 2020-11-23 at 18:01 -0800, Eric Biggers wrote:
> 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.

MediaTek CQHCI also works in this way.

Complete test is on-going now and we will update the results as soon as
possible.

Thanks,
Stanley Chu

> 
> 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);