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