| Message ID | 20201112194011.103774-1-ebiggers@kernel.org (mailing list archive) |
|---|---|
| Headers | show |
| Series | eMMC inline encryption support | expand |
On Thu, Nov 12, 2020 at 11:40:03AM -0800, Eric Biggers wrote: > Hello, > > This patchset adds support for eMMC inline encryption, as specified by > the upcoming version of the eMMC specification and as already > implemented and used on many devices. Building on that, it then adds > Qualcomm ICE support and wires it up for the Snapdragon 630 SoC. > > Inline encryption hardware improves the performance of storage > encryption and reduces power usage. See > Documentation/block/inline-encryption.rst for more information about > inline encryption and the blk-crypto framework (upstreamed in v5.8) > which supports it. Most mobile devices already use UFS or eMMC inline > encryption hardware; UFS support was already upstreamed in v5.9. > > Patches 1-3 add support for the standard eMMC inline encryption. > > However, as with UFS, host controller-specific patches are needed on top > of the standard support. Therefore, patches 4-8 add Qualcomm ICE > (Inline Crypto Engine) support and wire it up on the Snapdragon 630 SoC. > > To test this I took advantage of the recently upstreamed support for the > Snapdragon 630 SoC, plus work-in-progress patches from the SoMainline > project (https://github.com/SoMainline/linux/tree/konrad/v5.10-rc3). In > particular, I was able to run the fscrypt xfstests for ext4 and f2fs in > a Debian chroot. Among other things, these tests verified that the > correct ciphertext is written to disk (the same as software encryption). > > It will also be possible to add support for Mediatek eMMC inline > encryption hardware in mtk-sd, and it should be easier than the Qualcomm > hardware since the Mediatek hardware follows the standard more closely. > I.e., patches 1-3 should be almost enough for the Mediatek hardware. > However, I don't have the hardware to do this yet. > > This patchset is based on v5.10-rc3, and it can also be retrieved from > tag "mmc-crypto-v1" of > https://git.kernel.org/pub/scm/linux/kernel/git/ebiggers/linux.git > > Note: the fscrypt inline encryption support is partially broken in > v5.10-rc3, so for testing a fscrypt fix needs to be applied too: > https://lkml.kernel.org/r/20201111015224.303073-1-ebiggers@kernel.org > > Eric Biggers (8): > mmc: add basic support for inline encryption > mmc: cqhci: rename cqhci.c to cqhci-core.c > mmc: cqhci: add support for inline encryption > mmc: cqhci: add cqhci_host_ops::program_key > firmware: qcom_scm: update comment for ICE-related functions > dt-bindings: mmc: sdhci-msm: add ICE registers and clock > arm64: dts: qcom: sdm630: add ICE registers and clocks > mmc: sdhci-msm: add Inline Crypto Engine support Any comments on this patchset? - Eric
On 20/11/20 8:54 pm, Eric Biggers wrote: > On Thu, Nov 12, 2020 at 11:40:03AM -0800, Eric Biggers wrote: >> Hello, >> >> This patchset adds support for eMMC inline encryption, as specified by >> the upcoming version of the eMMC specification and as already >> implemented and used on many devices. Building on that, it then adds >> Qualcomm ICE support and wires it up for the Snapdragon 630 SoC. >> >> Inline encryption hardware improves the performance of storage >> encryption and reduces power usage. See >> Documentation/block/inline-encryption.rst for more information about >> inline encryption and the blk-crypto framework (upstreamed in v5.8) >> which supports it. Most mobile devices already use UFS or eMMC inline >> encryption hardware; UFS support was already upstreamed in v5.9. >> >> Patches 1-3 add support for the standard eMMC inline encryption. >> >> However, as with UFS, host controller-specific patches are needed on top >> of the standard support. Therefore, patches 4-8 add Qualcomm ICE >> (Inline Crypto Engine) support and wire it up on the Snapdragon 630 SoC. >> >> To test this I took advantage of the recently upstreamed support for the >> Snapdragon 630 SoC, plus work-in-progress patches from the SoMainline >> project (https://github.com/SoMainline/linux/tree/konrad/v5.10-rc3). In >> particular, I was able to run the fscrypt xfstests for ext4 and f2fs in >> a Debian chroot. Among other things, these tests verified that the >> correct ciphertext is written to disk (the same as software encryption). >> >> It will also be possible to add support for Mediatek eMMC inline >> encryption hardware in mtk-sd, and it should be easier than the Qualcomm >> hardware since the Mediatek hardware follows the standard more closely. >> I.e., patches 1-3 should be almost enough for the Mediatek hardware. >> However, I don't have the hardware to do this yet. >> >> This patchset is based on v5.10-rc3, and it can also be retrieved from >> tag "mmc-crypto-v1" of >> https://git.kernel.org/pub/scm/linux/kernel/git/ebiggers/linux.git >> >> Note: the fscrypt inline encryption support is partially broken in >> v5.10-rc3, so for testing a fscrypt fix needs to be applied too: >> https://lkml.kernel.org/r/20201111015224.303073-1-ebiggers@kernel.org >> >> Eric Biggers (8): >> mmc: add basic support for inline encryption >> mmc: cqhci: rename cqhci.c to cqhci-core.c >> mmc: cqhci: add support for inline encryption >> mmc: cqhci: add cqhci_host_ops::program_key >> firmware: qcom_scm: update comment for ICE-related functions >> dt-bindings: mmc: sdhci-msm: add ICE registers and clock >> arm64: dts: qcom: sdm630: add ICE registers and clocks >> mmc: sdhci-msm: add Inline Crypto Engine support > > Any comments on this patchset? 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? 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?
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(). > 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.) - Eric
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?
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);
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);
On Fri, 20 Nov 2020 at 19:54, Eric Biggers <ebiggers@kernel.org> wrote: > > On Thu, Nov 12, 2020 at 11:40:03AM -0800, Eric Biggers wrote: > > Hello, > > > > This patchset adds support for eMMC inline encryption, as specified by > > the upcoming version of the eMMC specification and as already > > implemented and used on many devices. Building on that, it then adds > > Qualcomm ICE support and wires it up for the Snapdragon 630 SoC. > > > > Inline encryption hardware improves the performance of storage > > encryption and reduces power usage. See > > Documentation/block/inline-encryption.rst for more information about > > inline encryption and the blk-crypto framework (upstreamed in v5.8) > > which supports it. Most mobile devices already use UFS or eMMC inline > > encryption hardware; UFS support was already upstreamed in v5.9. > > > > Patches 1-3 add support for the standard eMMC inline encryption. > > > > However, as with UFS, host controller-specific patches are needed on top > > of the standard support. Therefore, patches 4-8 add Qualcomm ICE > > (Inline Crypto Engine) support and wire it up on the Snapdragon 630 SoC. > > > > To test this I took advantage of the recently upstreamed support for the > > Snapdragon 630 SoC, plus work-in-progress patches from the SoMainline > > project (https://github.com/SoMainline/linux/tree/konrad/v5.10-rc3). In > > particular, I was able to run the fscrypt xfstests for ext4 and f2fs in > > a Debian chroot. Among other things, these tests verified that the > > correct ciphertext is written to disk (the same as software encryption). > > > > It will also be possible to add support for Mediatek eMMC inline > > encryption hardware in mtk-sd, and it should be easier than the Qualcomm > > hardware since the Mediatek hardware follows the standard more closely. > > I.e., patches 1-3 should be almost enough for the Mediatek hardware. > > However, I don't have the hardware to do this yet. > > > > This patchset is based on v5.10-rc3, and it can also be retrieved from > > tag "mmc-crypto-v1" of > > https://git.kernel.org/pub/scm/linux/kernel/git/ebiggers/linux.git > > > > Note: the fscrypt inline encryption support is partially broken in > > v5.10-rc3, so for testing a fscrypt fix needs to be applied too: > > https://lkml.kernel.org/r/20201111015224.303073-1-ebiggers@kernel.org > > > > Eric Biggers (8): > > mmc: add basic support for inline encryption > > mmc: cqhci: rename cqhci.c to cqhci-core.c > > mmc: cqhci: add support for inline encryption > > mmc: cqhci: add cqhci_host_ops::program_key > > firmware: qcom_scm: update comment for ICE-related functions > > dt-bindings: mmc: sdhci-msm: add ICE registers and clock > > arm64: dts: qcom: sdm630: add ICE registers and clocks > > mmc: sdhci-msm: add Inline Crypto Engine support > > Any comments on this patchset? I have been busy, but just wanted to let you know that I am moving to start reviewing this series shortly. I also need to catch up on the eMMC spec a bit, before I can provide you with comments. Kind regards Uffe
On Thu, Dec 17, 2020 at 05:21:32PM +0800, Peng.Zhou wrote: > > On Wed, 2020-11-25 at 17:03 +0800, Stanley Chu wrote: > > 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); > > > > > > Hi Eric, > > I also have a question about reprogramming keys scenarios, if some SoC > vensors' eMMC host will power down or something else like that keys will > be lost after runtime suspend, that means we must do reprogramming keys > in runtime resume, right? Do you think that we should add it in > cqhci-core layer(such as __cqhci_enable()) or every SoC vendor's host > driver resume path? > The keys should only be lost on reset, not on runtime suspend. So I believe the code I've proposed is sufficient. - Eric
On Fri, Dec 18, 2020 at 07:40:41AM +0800, Peng.Zhou wrote: > > > Hi Eric, > > > > > > I also have a question about reprogramming keys scenarios, if some SoC > > > vensors' eMMC host will power down or something else like that keys will > > > be lost after runtime suspend, that means we must do reprogramming keys > > > in runtime resume, right? Do you think that we should add it in > > > cqhci-core layer(such as __cqhci_enable()) or every SoC vendor's host > > > driver resume path? > > > > > > > The keys should only be lost on reset, not on runtime suspend. So I believe the > > code I've proposed is sufficient. > > > > - Eric > > That's a little too absolute for me...anyway that's my concern for much > more SoC vendors who want to be compatible with your framework in > future.Thank you for explanation. But the current approach works on all the hardware that's been tested so far, right? And programming the keys can take a long time, so it shouldn't be done unnecessarily. (I've heard it's fairly fast on Mediatek SoCs. However, with Qualcomm ICE it takes longer.) It seems that host controller configuration typically doesn't get lost during runtime suspend, and the keyslots are no exception to that. And if (hypothetically) a host controller that adds crypto support in the future actually does need to restore the keys during runtime resume, it can just do it in its ->runtime_resume() method. So from what I can see, there isn't anything else we should do for now. - Eric
On Wed, Nov 25, 2020 at 10:56:42AM +0100, Ulf Hansson wrote: > On Fri, 20 Nov 2020 at 19:54, Eric Biggers <ebiggers@kernel.org> wrote: > > > > On Thu, Nov 12, 2020 at 11:40:03AM -0800, Eric Biggers wrote: > > > Hello, > > > > > > This patchset adds support for eMMC inline encryption, as specified by > > > the upcoming version of the eMMC specification and as already > > > implemented and used on many devices. Building on that, it then adds > > > Qualcomm ICE support and wires it up for the Snapdragon 630 SoC. > > > > > > Inline encryption hardware improves the performance of storage > > > encryption and reduces power usage. See > > > Documentation/block/inline-encryption.rst for more information about > > > inline encryption and the blk-crypto framework (upstreamed in v5.8) > > > which supports it. Most mobile devices already use UFS or eMMC inline > > > encryption hardware; UFS support was already upstreamed in v5.9. > > > > > > Patches 1-3 add support for the standard eMMC inline encryption. > > > > > > However, as with UFS, host controller-specific patches are needed on top > > > of the standard support. Therefore, patches 4-8 add Qualcomm ICE > > > (Inline Crypto Engine) support and wire it up on the Snapdragon 630 SoC. > > > > > > To test this I took advantage of the recently upstreamed support for the > > > Snapdragon 630 SoC, plus work-in-progress patches from the SoMainline > > > project (https://github.com/SoMainline/linux/tree/konrad/v5.10-rc3). In > > > particular, I was able to run the fscrypt xfstests for ext4 and f2fs in > > > a Debian chroot. Among other things, these tests verified that the > > > correct ciphertext is written to disk (the same as software encryption). > > > > > > It will also be possible to add support for Mediatek eMMC inline > > > encryption hardware in mtk-sd, and it should be easier than the Qualcomm > > > hardware since the Mediatek hardware follows the standard more closely. > > > I.e., patches 1-3 should be almost enough for the Mediatek hardware. > > > However, I don't have the hardware to do this yet. > > > > > > This patchset is based on v5.10-rc3, and it can also be retrieved from > > > tag "mmc-crypto-v1" of > > > https://git.kernel.org/pub/scm/linux/kernel/git/ebiggers/linux.git > > > > > > Note: the fscrypt inline encryption support is partially broken in > > > v5.10-rc3, so for testing a fscrypt fix needs to be applied too: > > > https://lkml.kernel.org/r/20201111015224.303073-1-ebiggers@kernel.org > > > > > > Eric Biggers (8): > > > mmc: add basic support for inline encryption > > > mmc: cqhci: rename cqhci.c to cqhci-core.c > > > mmc: cqhci: add support for inline encryption > > > mmc: cqhci: add cqhci_host_ops::program_key > > > firmware: qcom_scm: update comment for ICE-related functions > > > dt-bindings: mmc: sdhci-msm: add ICE registers and clock > > > arm64: dts: qcom: sdm630: add ICE registers and clocks > > > mmc: sdhci-msm: add Inline Crypto Engine support > > > > Any comments on this patchset? > > I have been busy, but just wanted to let you know that I am moving to > start reviewing this series shortly. > > I also need to catch up on the eMMC spec a bit, before I can provide > you with comments. > > Kind regards > Uffe Ulf, are you still planning to review this patchset? I just sent out v4 of this patchset based on v5.11-rc2, but not a lot has changed from previous versions, since people have generally seemed happy with it. Any chance that you will apply it for 5.12? Thanks! - Eric
On Mon, 4 Jan 2021 at 21:46, Eric Biggers <ebiggers@kernel.org> wrote: > > On Wed, Nov 25, 2020 at 10:56:42AM +0100, Ulf Hansson wrote: > > On Fri, 20 Nov 2020 at 19:54, Eric Biggers <ebiggers@kernel.org> wrote: > > > > > > On Thu, Nov 12, 2020 at 11:40:03AM -0800, Eric Biggers wrote: > > > > Hello, > > > > > > > > This patchset adds support for eMMC inline encryption, as specified by > > > > the upcoming version of the eMMC specification and as already > > > > implemented and used on many devices. Building on that, it then adds > > > > Qualcomm ICE support and wires it up for the Snapdragon 630 SoC. > > > > > > > > Inline encryption hardware improves the performance of storage > > > > encryption and reduces power usage. See > > > > Documentation/block/inline-encryption.rst for more information about > > > > inline encryption and the blk-crypto framework (upstreamed in v5.8) > > > > which supports it. Most mobile devices already use UFS or eMMC inline > > > > encryption hardware; UFS support was already upstreamed in v5.9. > > > > > > > > Patches 1-3 add support for the standard eMMC inline encryption. > > > > > > > > However, as with UFS, host controller-specific patches are needed on top > > > > of the standard support. Therefore, patches 4-8 add Qualcomm ICE > > > > (Inline Crypto Engine) support and wire it up on the Snapdragon 630 SoC. > > > > > > > > To test this I took advantage of the recently upstreamed support for the > > > > Snapdragon 630 SoC, plus work-in-progress patches from the SoMainline > > > > project (https://github.com/SoMainline/linux/tree/konrad/v5.10-rc3). In > > > > particular, I was able to run the fscrypt xfstests for ext4 and f2fs in > > > > a Debian chroot. Among other things, these tests verified that the > > > > correct ciphertext is written to disk (the same as software encryption). > > > > > > > > It will also be possible to add support for Mediatek eMMC inline > > > > encryption hardware in mtk-sd, and it should be easier than the Qualcomm > > > > hardware since the Mediatek hardware follows the standard more closely. > > > > I.e., patches 1-3 should be almost enough for the Mediatek hardware. > > > > However, I don't have the hardware to do this yet. > > > > > > > > This patchset is based on v5.10-rc3, and it can also be retrieved from > > > > tag "mmc-crypto-v1" of > > > > https://git.kernel.org/pub/scm/linux/kernel/git/ebiggers/linux.git > > > > > > > > Note: the fscrypt inline encryption support is partially broken in > > > > v5.10-rc3, so for testing a fscrypt fix needs to be applied too: > > > > https://lkml.kernel.org/r/20201111015224.303073-1-ebiggers@kernel.org > > > > > > > > Eric Biggers (8): > > > > mmc: add basic support for inline encryption > > > > mmc: cqhci: rename cqhci.c to cqhci-core.c > > > > mmc: cqhci: add support for inline encryption > > > > mmc: cqhci: add cqhci_host_ops::program_key > > > > firmware: qcom_scm: update comment for ICE-related functions > > > > dt-bindings: mmc: sdhci-msm: add ICE registers and clock > > > > arm64: dts: qcom: sdm630: add ICE registers and clocks > > > > mmc: sdhci-msm: add Inline Crypto Engine support > > > > > > Any comments on this patchset? > > > > I have been busy, but just wanted to let you know that I am moving to > > start reviewing this series shortly. > > > > I also need to catch up on the eMMC spec a bit, before I can provide > > you with comments. > > > > Kind regards > > Uffe > > Ulf, are you still planning to review this patchset? I just sent out v4 of this > patchset based on v5.11-rc2, but not a lot has changed from previous versions, > since people have generally seemed happy with it. Any chance that you will > apply it for 5.12? Thanks! My apologies for the delay. I certainly appreciate the review that's been done by people and I intend to have a look myself within the coming week. I definitely think it should be possible to get this queued for v5.12, unless I find some very weird things, which I doubt. Kind regards Uffe
Hello, This patchset adds support for eMMC inline encryption, as specified by the upcoming version of the eMMC specification and as already implemented and used on many devices. Building on that, it then adds Qualcomm ICE support and wires it up for the Snapdragon 630 SoC. Inline encryption hardware improves the performance of storage encryption and reduces power usage. See Documentation/block/inline-encryption.rst for more information about inline encryption and the blk-crypto framework (upstreamed in v5.8) which supports it. Most mobile devices already use UFS or eMMC inline encryption hardware; UFS support was already upstreamed in v5.9. Patches 1-3 add support for the standard eMMC inline encryption. However, as with UFS, host controller-specific patches are needed on top of the standard support. Therefore, patches 4-8 add Qualcomm ICE (Inline Crypto Engine) support and wire it up on the Snapdragon 630 SoC. To test this I took advantage of the recently upstreamed support for the Snapdragon 630 SoC, plus work-in-progress patches from the SoMainline project (https://github.com/SoMainline/linux/tree/konrad/v5.10-rc3). In particular, I was able to run the fscrypt xfstests for ext4 and f2fs in a Debian chroot. Among other things, these tests verified that the correct ciphertext is written to disk (the same as software encryption). It will also be possible to add support for Mediatek eMMC inline encryption hardware in mtk-sd, and it should be easier than the Qualcomm hardware since the Mediatek hardware follows the standard more closely. I.e., patches 1-3 should be almost enough for the Mediatek hardware. However, I don't have the hardware to do this yet. This patchset is based on v5.10-rc3, and it can also be retrieved from tag "mmc-crypto-v1" of https://git.kernel.org/pub/scm/linux/kernel/git/ebiggers/linux.git Note: the fscrypt inline encryption support is partially broken in v5.10-rc3, so for testing a fscrypt fix needs to be applied too: https://lkml.kernel.org/r/20201111015224.303073-1-ebiggers@kernel.org Eric Biggers (8): mmc: add basic support for inline encryption mmc: cqhci: rename cqhci.c to cqhci-core.c mmc: cqhci: add support for inline encryption mmc: cqhci: add cqhci_host_ops::program_key firmware: qcom_scm: update comment for ICE-related functions dt-bindings: mmc: sdhci-msm: add ICE registers and clock arm64: dts: qcom: sdm630: add ICE registers and clocks mmc: sdhci-msm: add Inline Crypto Engine support .../devicetree/bindings/mmc/sdhci-msm.txt | 3 + arch/arm64/boot/dts/qcom/sdm630.dtsi | 10 +- drivers/firmware/qcom_scm.c | 16 +- drivers/mmc/core/Kconfig | 8 + drivers/mmc/core/Makefile | 1 + drivers/mmc/core/block.c | 3 + drivers/mmc/core/core.c | 3 + drivers/mmc/core/crypto.c | 54 ++++ drivers/mmc/core/crypto.h | 46 +++ drivers/mmc/core/host.c | 2 + drivers/mmc/core/queue.c | 3 + drivers/mmc/host/Kconfig | 1 + drivers/mmc/host/Makefile | 2 + drivers/mmc/host/{cqhci.c => cqhci-core.c} | 66 ++++- drivers/mmc/host/cqhci-crypto.c | 237 +++++++++++++++ drivers/mmc/host/cqhci-crypto.h | 47 +++ drivers/mmc/host/cqhci.h | 85 +++++- drivers/mmc/host/sdhci-msm.c | 270 +++++++++++++++++- include/linux/mmc/core.h | 6 + include/linux/mmc/host.h | 7 + 20 files changed, 845 insertions(+), 25 deletions(-) create mode 100644 drivers/mmc/core/crypto.c create mode 100644 drivers/mmc/core/crypto.h rename drivers/mmc/host/{cqhci.c => cqhci-core.c} (94%) create mode 100644 drivers/mmc/host/cqhci-crypto.c create mode 100644 drivers/mmc/host/cqhci-crypto.h base-commit: f8394f232b1eab649ce2df5c5f15b0e528c92091