* Re: [PATCH v2 2/4] mmc: Mediatek: enable crypto hardware engine [not found] <20210309015750.6283-1-peng.zhou@mediatek.com> @ 2021-03-11 13:48 ` Linus Walleij 2021-03-11 19:08 ` Eric Biggers 0 siblings, 1 reply; 10+ messages in thread From: Linus Walleij @ 2021-03-11 13:48 UTC (permalink / raw) To: Peng Zhou, linux-block, Eric Biggers Cc: Ulf Hansson, Chaotian Jing, linux-mmc, open list:OPEN FIRMWARE AND FLATTENED DEVICE TREE BINDINGS, Adrian Hunter, Satya Tangirala, Wulin Li Hi Peng, thanks for your patch! On Tue, Mar 9, 2021 at 3:06 AM Peng Zhou <peng.zhou@mediatek.com> wrote: > Use SMC call enable hardware crypto engine > due to it only be changed in ATF(EL3). > > Signed-off-by: Peng Zhou <peng.zhou@mediatek.com> Unfortunately this commit message is way to short to understand what is going on, and has a lot of assumed previous knowledge. Can you expand the commit message so that anyone who just know MMC and some SoC basics can understand what an SMC call and and what ATF(EL3) means? I assume this some kind of inline encryption? I think maybe linux-block mailing list need to be involved because there is certain a Linux standard way of setting up inline encryption for the block layer. For example: how is the key to be used derived? How is the device unlocked in the first place? If I insert a LUKS encrypted harddrive in a Linux machine the whole system is pretty much aware of how this should be handled and everything "just works", I enter a pass phrase and off it goes. I can use symmetric keys as well. How is this stuff done for this hardware? > + /* > + * 1: MSDC_AES_CTL_INIT > + * 4: cap_id, no-meaning now > + * 1: cfg_id, we choose the second cfg group > + */ > + if (mmc->caps2 & MMC_CAP2_CRYPTO) > + arm_smccc_smc(MTK_SIP_MMC_CONTROL, > + 1, 4, 1, 0, 0, 0, 0, &smccc_res); The same as above: these comments assume that everyone already knows what is going on. AES encryption requires a key and I don't see the driver setting up any key. How is the code in this file: drivers/mmc/core/crypto.c interacting with your driver? drivers/mmc/host/cqhci-crypto.c is used by SDHCI and is quite readable and I see what is going on. For example it contains functions like: cqhci_crypto_program_key() cqhci_crypto_keyslot_program() cqhci_crypto_clear_keyslot() cqhci_crypto_keyslot_evict() cqhci_find_blk_crypto_mode() MMC_CAP2_CRYPTO is used as a sign that the driver can do inline encryption, then devm_blk_ksm_init() is called to initialize a block encryption abstraction with the block layer. Ops are registered using struct blk_ksm_ll_ops cqhci_ksm_ops. This is very straight forward. But where does all the above happen for this driver? I get the feeling that some magic is happening in outoftree patches or in the secure world, and that is not how we do these things, you have to use the frameworks. Yours, Linus Walleij ^ permalink raw reply [flat|nested] 10+ messages in thread
* Re: [PATCH v2 2/4] mmc: Mediatek: enable crypto hardware engine 2021-03-11 13:48 ` [PATCH v2 2/4] mmc: Mediatek: enable crypto hardware engine Linus Walleij @ 2021-03-11 19:08 ` Eric Biggers 2021-03-12 9:05 ` Ulf Hansson 2021-03-15 13:41 ` Linus Walleij 0 siblings, 2 replies; 10+ messages in thread From: Eric Biggers @ 2021-03-11 19:08 UTC (permalink / raw) To: Linus Walleij Cc: Peng Zhou, linux-block, Ulf Hansson, Chaotian Jing, linux-mmc, open list:OPEN FIRMWARE AND FLATTENED DEVICE TREE BINDINGS, Adrian Hunter, Satya Tangirala, Wulin Li On Thu, Mar 11, 2021 at 02:48:23PM +0100, Linus Walleij wrote: > Hi Peng, > > thanks for your patch! > > On Tue, Mar 9, 2021 at 3:06 AM Peng Zhou <peng.zhou@mediatek.com> wrote: > > > Use SMC call enable hardware crypto engine > > due to it only be changed in ATF(EL3). > > > > Signed-off-by: Peng Zhou <peng.zhou@mediatek.com> > > Unfortunately this commit message is way to short to > understand what is going on, and has a lot of assumed > previous knowledge. > > Can you expand the commit message so that anyone > who just know MMC and some SoC basics can understand > what an SMC call and and what ATF(EL3) means? > > I assume this some kind of inline encryption? > > I think maybe linux-block mailing list need to be involved > because there is certain a Linux standard way of setting > up inline encryption for the block layer. > > For example: how is the key to be used derived? > How is the device unlocked in the first place? > > If I insert a LUKS encrypted harddrive in a Linux machine > the whole system is pretty much aware of how this should > be handled and everything "just works", I enter a pass > phrase and off it goes. I can use symmetric keys as well. > How is this stuff done for this hardware? > > > + /* > > + * 1: MSDC_AES_CTL_INIT > > + * 4: cap_id, no-meaning now > > + * 1: cfg_id, we choose the second cfg group > > + */ > > + if (mmc->caps2 & MMC_CAP2_CRYPTO) > > + arm_smccc_smc(MTK_SIP_MMC_CONTROL, > > + 1, 4, 1, 0, 0, 0, 0, &smccc_res); > > The same as above: these comments assume that everyone > already knows what is going on. > > AES encryption requires a key and I don't see the driver > setting up any key. How is the code in this file: > drivers/mmc/core/crypto.c > interacting with your driver? > drivers/mmc/host/cqhci-crypto.c > is used by SDHCI and is quite readable and I see what is going on. > For example it contains functions like: > cqhci_crypto_program_key() > cqhci_crypto_keyslot_program() > cqhci_crypto_clear_keyslot() > cqhci_crypto_keyslot_evict() > cqhci_find_blk_crypto_mode() > > MMC_CAP2_CRYPTO is used as a sign that the driver > can do inline encryption, then devm_blk_ksm_init() is called > to initialize a block encryption abstraction with the block layer. > Ops are registered using > struct blk_ksm_ll_ops cqhci_ksm_ops. > > This is very straight forward. > > But where does all the above happen for this driver? > It happens in the same place, cqhci-crypto.c. Mediatek's eMMC inline encryption hardware follows the eMMC standard fairly closely, so Peng's patch series just sets MMC_CAP2_CRYPTO to make it use the standard cqhci crypto code, and does a couple extra things to actually enable the hardware's crypto support on Mediatek platforms since it isn't enabled by default. (*Why* it requires an SMC call to enable instead of just working as expected, I don't know though.) The way all this gets used is via the blk-crypto framework (Documentation/block/inline-encryption.rst), which is used by fscrypt (ext4 and f2fs encryption). Peng, if you could explain all this properly in the cover letter, commit messages, and code comments (where it makes sense), that would be really helpful for people. Also please make sure your patch series is in a thread so that people see it together. - Eric ^ permalink raw reply [flat|nested] 10+ messages in thread
* Re: [PATCH v2 2/4] mmc: Mediatek: enable crypto hardware engine 2021-03-11 19:08 ` Eric Biggers @ 2021-03-12 9:05 ` Ulf Hansson 2021-03-12 10:47 ` Arnd Bergmann [not found] ` <1615884533.21508.118.camel@mbjsdccf07> 2021-03-15 13:41 ` Linus Walleij 1 sibling, 2 replies; 10+ messages in thread From: Ulf Hansson @ 2021-03-12 9:05 UTC (permalink / raw) To: Eric Biggers, Peng Zhou Cc: Linus Walleij, linux-block, Chaotian Jing, linux-mmc, open list:OPEN FIRMWARE AND FLATTENED DEVICE TREE BINDINGS, Adrian Hunter, Satya Tangirala, Wulin Li, Arnd Bergmann, Sudeep Holla + Arnd, Sudeep On Thu, 11 Mar 2021 at 20:08, Eric Biggers <ebiggers@kernel.org> wrote: > > On Thu, Mar 11, 2021 at 02:48:23PM +0100, Linus Walleij wrote: > > Hi Peng, > > > > thanks for your patch! > > > > On Tue, Mar 9, 2021 at 3:06 AM Peng Zhou <peng.zhou@mediatek.com> wrote: > > > > > Use SMC call enable hardware crypto engine > > > due to it only be changed in ATF(EL3). > > > > > > Signed-off-by: Peng Zhou <peng.zhou@mediatek.com> > > > > Unfortunately this commit message is way to short to > > understand what is going on, and has a lot of assumed > > previous knowledge. > > > > Can you expand the commit message so that anyone > > who just know MMC and some SoC basics can understand > > what an SMC call and and what ATF(EL3) means? > > > > I assume this some kind of inline encryption? > > > > I think maybe linux-block mailing list need to be involved > > because there is certain a Linux standard way of setting > > up inline encryption for the block layer. > > > > For example: how is the key to be used derived? > > How is the device unlocked in the first place? > > > > If I insert a LUKS encrypted harddrive in a Linux machine > > the whole system is pretty much aware of how this should > > be handled and everything "just works", I enter a pass > > phrase and off it goes. I can use symmetric keys as well. > > How is this stuff done for this hardware? > > > > > + /* > > > + * 1: MSDC_AES_CTL_INIT > > > + * 4: cap_id, no-meaning now > > > + * 1: cfg_id, we choose the second cfg group > > > + */ > > > + if (mmc->caps2 & MMC_CAP2_CRYPTO) > > > + arm_smccc_smc(MTK_SIP_MMC_CONTROL, > > > + 1, 4, 1, 0, 0, 0, 0, &smccc_res); > > > > The same as above: these comments assume that everyone > > already knows what is going on. > > > > AES encryption requires a key and I don't see the driver > > setting up any key. How is the code in this file: > > drivers/mmc/core/crypto.c > > interacting with your driver? > > drivers/mmc/host/cqhci-crypto.c > > is used by SDHCI and is quite readable and I see what is going on. > > For example it contains functions like: > > cqhci_crypto_program_key() > > cqhci_crypto_keyslot_program() > > cqhci_crypto_clear_keyslot() > > cqhci_crypto_keyslot_evict() > > cqhci_find_blk_crypto_mode() > > > > MMC_CAP2_CRYPTO is used as a sign that the driver > > can do inline encryption, then devm_blk_ksm_init() is called > > to initialize a block encryption abstraction with the block layer. > > Ops are registered using > > struct blk_ksm_ll_ops cqhci_ksm_ops. > > > > This is very straight forward. > > > > But where does all the above happen for this driver? > > > > It happens in the same place, cqhci-crypto.c. Mediatek's eMMC inline encryption > hardware follows the eMMC standard fairly closely, so Peng's patch series just > sets MMC_CAP2_CRYPTO to make it use the standard cqhci crypto code, and does a > couple extra things to actually enable the hardware's crypto support on Mediatek > platforms since it isn't enabled by default. (*Why* it requires an SMC call to > enable instead of just working as expected, I don't know though.) As I have probably indicated earlier, I am starting to become more and more annoyed with these arm_smccc_smc() calls in generic drivers. As a matter of fact, I think the situation is about to explode. Just do a "git grep arm_smccc_smc" and you will find that it's not only SoC specific drivers that call them. In general we want to keep drivers portable and this is clearly moving in the wrong direction. Or maybe it's just me being grumpy and having a bad day. :-) In the Qcom mmc case (drivers/mmc/host/sdhci-msm.c) for eMMC inline encryption, the arm_smccc_smc() call is slightly better handled as it's abstracted behind a Qcom specific firmware API. So, sdhci-msm.c calls qcom_scm_ice_set_key() (implemented in drivers/firmware/qcom_scm.c) to program a key. I guess we don't have an abstraction layer that would fit for this case, right? My point is, when there is no proper abstraction layer to use for the relevant arm_smccc_smc() call, the Qcom way is fine to me. In this Mediatek case, it looks slightly different. To me it looks more like a resource that needs to be turned on/off to enable/disable the "inline encryption engine". Could it be modeled as phy, power-rail, clock, pinctrl or perhaps behind a PM domain (where SoC specific calls makes perfect sense). Peng can you please elaborate on what goes on behind the arm_smccc_smc() call, as that would help us to understand what abstraction layer to pick? [...] Kind regards Uffe ^ permalink raw reply [flat|nested] 10+ messages in thread
* Re: [PATCH v2 2/4] mmc: Mediatek: enable crypto hardware engine 2021-03-12 9:05 ` Ulf Hansson @ 2021-03-12 10:47 ` Arnd Bergmann [not found] ` <1615884533.21508.118.camel@mbjsdccf07> 1 sibling, 0 replies; 10+ messages in thread From: Arnd Bergmann @ 2021-03-12 10:47 UTC (permalink / raw) To: Ulf Hansson Cc: Eric Biggers, Peng Zhou, Linus Walleij, linux-block, Chaotian Jing, linux-mmc, open list:OPEN FIRMWARE AND FLATTENED DEVICE TREE BINDINGS, Adrian Hunter, Satya Tangirala, Wulin Li, Sudeep Holla On Fri, Mar 12, 2021 at 10:05 AM Ulf Hansson <ulf.hansson@linaro.org> wrote: > On Thu, 11 Mar 2021 at 20:08, Eric Biggers <ebiggers@kernel.org> wrote: > > On Thu, Mar 11, 2021 at 02:48:23PM +0100, Linus Walleij wrote: > > > > It happens in the same place, cqhci-crypto.c. Mediatek's eMMC inline encryption > > hardware follows the eMMC standard fairly closely, so Peng's patch series just > > sets MMC_CAP2_CRYPTO to make it use the standard cqhci crypto code, and does a > > couple extra things to actually enable the hardware's crypto support on Mediatek > > platforms since it isn't enabled by default. (*Why* it requires an SMC call to > > enable instead of just working as expected, I don't know though.) > > As I have probably indicated earlier, I am starting to become more and > more annoyed with these arm_smccc_smc() calls in generic drivers. > > As a matter of fact, I think the situation is about to explode. Just > do a "git grep arm_smccc_smc" and you will find that it's not only SoC > specific drivers that call them. In general we want to keep drivers > portable and this is clearly moving in the wrong direction. Or maybe > it's just me being grumpy and having a bad day. :-) I agree, this really does feel underspecified, as there is no way to actually know which smc interfaces are available, or which ones belong to a particular driver. Simply calling them is not that different from using an ioremap() on a hardwired physical address just because you know where a device is on a given SoC. Or another way of looking at it is that these are function calls that are arbitrarily moved out from the kernel into a piece of (usually) closed source software running on the same chip. A first step toward managing this better might be to enforce namespaces of the smc calls, if we can find a way to limit arm_smccc_smc() calls those that have the first argument defined in a central header file. Arnd ^ permalink raw reply [flat|nested] 10+ messages in thread
[parent not found: <1615884533.21508.118.camel@mbjsdccf07>]
* Re: [PATCH v2 2/4] mmc: Mediatek: enable crypto hardware engine [not found] ` <1615884533.21508.118.camel@mbjsdccf07> @ 2021-03-16 10:09 ` Ulf Hansson [not found] ` <1615893329.21508.128.camel@mbjsdccf07> 0 siblings, 1 reply; 10+ messages in thread From: Ulf Hansson @ 2021-03-16 10:09 UTC (permalink / raw) To: Peng.Zhou Cc: Eric Biggers, Linus Walleij, linux-block, Chaotian Jing, linux-mmc, open list:OPEN FIRMWARE AND FLATTENED DEVICE TREE BINDINGS, Adrian Hunter, Satya Tangirala, Wulin Li, Arnd Bergmann, Sudeep Holla On Tue, 16 Mar 2021 at 09:55, Peng.Zhou <peng.zhou@mediatek.com> wrote: > > On Fri, 2021-03-12 at 10:05 +0100, Ulf Hansson wrote: > > + Arnd, Sudeep > > > > On Thu, 11 Mar 2021 at 20:08, Eric Biggers <ebiggers@kernel.org> wrote: > > > > > > On Thu, Mar 11, 2021 at 02:48:23PM +0100, Linus Walleij wrote: > > > > Hi Peng, > > > > > > > > thanks for your patch! > > > > > > > > On Tue, Mar 9, 2021 at 3:06 AM Peng Zhou <peng.zhou@mediatek.com> wrote: > > > > > > > > > Use SMC call enable hardware crypto engine > > > > > due to it only be changed in ATF(EL3). > > > > > > > > > > Signed-off-by: Peng Zhou <peng.zhou@mediatek.com> > > > > > > > > Unfortunately this commit message is way to short to > > > > understand what is going on, and has a lot of assumed > > > > previous knowledge. > > > > > > > > Can you expand the commit message so that anyone > > > > who just know MMC and some SoC basics can understand > > > > what an SMC call and and what ATF(EL3) means? > > > > > > > > I assume this some kind of inline encryption? > > > > > > > > I think maybe linux-block mailing list need to be involved > > > > because there is certain a Linux standard way of setting > > > > up inline encryption for the block layer. > > > > > > > > For example: how is the key to be used derived? > > > > How is the device unlocked in the first place? > > > > > > > > If I insert a LUKS encrypted harddrive in a Linux machine > > > > the whole system is pretty much aware of how this should > > > > be handled and everything "just works", I enter a pass > > > > phrase and off it goes. I can use symmetric keys as well. > > > > How is this stuff done for this hardware? > > > > > > > > > + /* > > > > > + * 1: MSDC_AES_CTL_INIT > > > > > + * 4: cap_id, no-meaning now > > > > > + * 1: cfg_id, we choose the second cfg group > > > > > + */ > > > > > + if (mmc->caps2 & MMC_CAP2_CRYPTO) > > > > > + arm_smccc_smc(MTK_SIP_MMC_CONTROL, > > > > > + 1, 4, 1, 0, 0, 0, 0, &smccc_res); > > > > > > > > The same as above: these comments assume that everyone > > > > already knows what is going on. > > > > > > > > AES encryption requires a key and I don't see the driver > > > > setting up any key. How is the code in this file: > > > > drivers/mmc/core/crypto.c > > > > interacting with your driver? > > > > drivers/mmc/host/cqhci-crypto.c > > > > is used by SDHCI and is quite readable and I see what is going on. > > > > For example it contains functions like: > > > > cqhci_crypto_program_key() > > > > cqhci_crypto_keyslot_program() > > > > cqhci_crypto_clear_keyslot() > > > > cqhci_crypto_keyslot_evict() > > > > cqhci_find_blk_crypto_mode() > > > > > > > > MMC_CAP2_CRYPTO is used as a sign that the driver > > > > can do inline encryption, then devm_blk_ksm_init() is called > > > > to initialize a block encryption abstraction with the block layer. > > > > Ops are registered using > > > > struct blk_ksm_ll_ops cqhci_ksm_ops. > > > > > > > > This is very straight forward. > > > > > > > > But where does all the above happen for this driver? > > > > > > > > > > It happens in the same place, cqhci-crypto.c. Mediatek's eMMC inline encryption > > > hardware follows the eMMC standard fairly closely, so Peng's patch series just > > > sets MMC_CAP2_CRYPTO to make it use the standard cqhci crypto code, and does a > > > couple extra things to actually enable the hardware's crypto support on Mediatek > > > platforms since it isn't enabled by default. (*Why* it requires an SMC call to > > > enable instead of just working as expected, I don't know though.) > > > > As I have probably indicated earlier, I am starting to become more and > > more annoyed with these arm_smccc_smc() calls in generic drivers. > > > > As a matter of fact, I think the situation is about to explode. Just > > do a "git grep arm_smccc_smc" and you will find that it's not only SoC > > specific drivers that call them. In general we want to keep drivers > > portable and this is clearly moving in the wrong direction. Or maybe > > it's just me being grumpy and having a bad day. :-) > > > > In the Qcom mmc case (drivers/mmc/host/sdhci-msm.c) for eMMC inline > > encryption, the arm_smccc_smc() call is slightly better handled as > > it's abstracted behind a Qcom specific firmware API. So, sdhci-msm.c > > calls qcom_scm_ice_set_key() (implemented in > > drivers/firmware/qcom_scm.c) to program a key. I guess we don't have > > an abstraction layer that would fit for this case, right? > > > > My point is, when there is no proper abstraction layer to use for the > > relevant arm_smccc_smc() call, the Qcom way is fine to me. > > > > In this Mediatek case, it looks slightly different. To me it looks > > more like a resource that needs to be turned on/off to enable/disable > > the "inline encryption engine". Could it be modeled as phy, > > power-rail, clock, pinctrl or perhaps behind a PM domain (where SoC > > specific calls makes perfect sense). > > > > Peng can you please elaborate on what goes on behind the > > arm_smccc_smc() call, as that would help us to understand what > > abstraction layer to pick? > > > > [...] > > > > Kind regards > > Uffe > > Hi All, > > First of all, I appreciated that you are interested in my patch series > and give me so much significant suggestions! Then, please let me summary > the detail information about MediaTek eMMC hardware crypto IP. > > Before that, as you know, due to I work for MediaTek.inc that means I'm > as an employee in this mail thread, so I don't give any comment about > other SoC manufacturers.I will only focus on ours. > > > [Background] Why I upstream this patch series? > Obiviously, we want to support hardware level file base encryption, file > encryption had been a mandatory feature in mobile device such as Android > environment. > > A few years ago, we only support software level file encryption, it > based on the reality of that time: > - There is no official encryption specification announced by JEDEC or > any device manufacturers > - File based encryption is not a mandatory feature for mobile devices > - Security is not the highest priority thing for our most of Customers > > Time can fly and Market requirement is also, hardware level encryption > functions had been add in our SoCs in soon, because that: > - An encryption specification which is widely recognized by device > manufacturers and SoC manufacturers had been announced. Although it > doesn't been accepted by JEDEC until now, most of eMMC device > manufacturers had support it. > - Performance, special in low end mobile device, to some extent, > hardware encryption could reduce some CPU loading, > - Almost overnight, Security has became the super star, everyone want > it, consider the performance (comparing with full disk encryption) and > flexibility, file based encryption is indispensable. > > One more thing, there is no common framework in kernel when our SoCs had > crypto IP in that time, so we design a special framework in kernel to > support it. In fact, we had support hardware encryption for several > years in a special and non-public way. > > You'll know the rest, Eric design a common framework that lets SoC > manufacturers support hardware encryption easier now. That' why we give > up our own special private way and try to support it. > > In fact, at this point in time, we have used this framework(include my > patch series) in our mobile products with newest Android version for > almost one year. > > > [Your question] Why we need use SMC call in our driver? or Why our > crypto hardware IP is not default on? > > Yes, MediaTek eMMC crypto hardware IP is default off in current design > and most important is we only turn it on in ARM exception level 3 > (EL3,the highest security level), that means we can only control it > under ARM trust firmware (ATF) environment, but kernel space (it's EL2 > in our platform). > > I can get your bewilderment: why it's default off and why put it in high > security level control? Actually, I don't have an issue with this, at all. Instead, my worries are about keeping generic drivers portable, which means resources need to be modelled through proper abstraction layers. SoC specific drivers are different, they don't necessarily need to cope with this requirement. Additionally, to me, it makes perfect sense to allow the crypto IP block to be powered off, as you would likely waste energy to have it always powered on, especially when it's not being in use. So, this boils down to understand what "turn on" crypto hardware IP actually means? Is it a clock, a phy, a power-rail or perhaps a combination of things that is turned on for the IP to work? What happens behind the SMC call? The answer to this question will help us understand what abstraction layer we should pick. [...] > > [Your suggestion] > In general, I agree it, and I will check qcom's solution then try to do > a firmware layer for our eMMC driver to call. According to what you have described, I don't think the Qcom solution is feasible for this case. In your case it's about turning on a resource and not about programming a key. I am sure we can find an existing abstraction layer to use, we just need to agree on which one that makes best sense. Kind regards Uffe ^ permalink raw reply [flat|nested] 10+ messages in thread
[parent not found: <1615893329.21508.128.camel@mbjsdccf07>]
* Re: [PATCH v2 2/4] mmc: Mediatek: enable crypto hardware engine [not found] ` <1615893329.21508.128.camel@mbjsdccf07> @ 2021-03-16 13:55 ` Ulf Hansson 2021-03-22 13:45 ` Linus Walleij 0 siblings, 1 reply; 10+ messages in thread From: Ulf Hansson @ 2021-03-16 13:55 UTC (permalink / raw) To: Peng.Zhou Cc: Eric Biggers, Linus Walleij, linux-block, Chaotian Jing, linux-mmc, open list:OPEN FIRMWARE AND FLATTENED DEVICE TREE BINDINGS, Adrian Hunter, Satya Tangirala, Wulin Li, Arnd Bergmann, Sudeep Holla On Tue, 16 Mar 2021 at 12:22, Peng.Zhou <peng.zhou@mediatek.com> wrote: > > On Tue, 2021-03-16 at 11:09 +0100, Ulf Hansson wrote: > > On Tue, 16 Mar 2021 at 09:55, Peng.Zhou <peng.zhou@mediatek.com> wrote: > > > > > > On Fri, 2021-03-12 at 10:05 +0100, Ulf Hansson wrote: > > > > + Arnd, Sudeep > > > > > > > > On Thu, 11 Mar 2021 at 20:08, Eric Biggers <ebiggers@kernel.org> wrote: > > > > > > > > > > On Thu, Mar 11, 2021 at 02:48:23PM +0100, Linus Walleij wrote: > > > > > > Hi Peng, > > > > > > > > > > > > thanks for your patch! > > > > > > > > > > > > On Tue, Mar 9, 2021 at 3:06 AM Peng Zhou <peng.zhou@mediatek.com> wrote: > > > > > > > > > > > > > Use SMC call enable hardware crypto engine > > > > > > > due to it only be changed in ATF(EL3). > > > > > > > > > > > > > > Signed-off-by: Peng Zhou <peng.zhou@mediatek.com> > > > > > > > > > > > > Unfortunately this commit message is way to short to > > > > > > understand what is going on, and has a lot of assumed > > > > > > previous knowledge. > > > > > > > > > > > > Can you expand the commit message so that anyone > > > > > > who just know MMC and some SoC basics can understand > > > > > > what an SMC call and and what ATF(EL3) means? > > > > > > > > > > > > I assume this some kind of inline encryption? > > > > > > > > > > > > I think maybe linux-block mailing list need to be involved > > > > > > because there is certain a Linux standard way of setting > > > > > > up inline encryption for the block layer. > > > > > > > > > > > > For example: how is the key to be used derived? > > > > > > How is the device unlocked in the first place? > > > > > > > > > > > > If I insert a LUKS encrypted harddrive in a Linux machine > > > > > > the whole system is pretty much aware of how this should > > > > > > be handled and everything "just works", I enter a pass > > > > > > phrase and off it goes. I can use symmetric keys as well. > > > > > > How is this stuff done for this hardware? > > > > > > > > > > > > > + /* > > > > > > > + * 1: MSDC_AES_CTL_INIT > > > > > > > + * 4: cap_id, no-meaning now > > > > > > > + * 1: cfg_id, we choose the second cfg group > > > > > > > + */ > > > > > > > + if (mmc->caps2 & MMC_CAP2_CRYPTO) > > > > > > > + arm_smccc_smc(MTK_SIP_MMC_CONTROL, > > > > > > > + 1, 4, 1, 0, 0, 0, 0, &smccc_res); > > > > > > > > > > > > The same as above: these comments assume that everyone > > > > > > already knows what is going on. > > > > > > > > > > > > AES encryption requires a key and I don't see the driver > > > > > > setting up any key. How is the code in this file: > > > > > > drivers/mmc/core/crypto.c > > > > > > interacting with your driver? > > > > > > drivers/mmc/host/cqhci-crypto.c > > > > > > is used by SDHCI and is quite readable and I see what is going on. > > > > > > For example it contains functions like: > > > > > > cqhci_crypto_program_key() > > > > > > cqhci_crypto_keyslot_program() > > > > > > cqhci_crypto_clear_keyslot() > > > > > > cqhci_crypto_keyslot_evict() > > > > > > cqhci_find_blk_crypto_mode() > > > > > > > > > > > > MMC_CAP2_CRYPTO is used as a sign that the driver > > > > > > can do inline encryption, then devm_blk_ksm_init() is called > > > > > > to initialize a block encryption abstraction with the block layer. > > > > > > Ops are registered using > > > > > > struct blk_ksm_ll_ops cqhci_ksm_ops. > > > > > > > > > > > > This is very straight forward. > > > > > > > > > > > > But where does all the above happen for this driver? > > > > > > > > > > > > > > > > It happens in the same place, cqhci-crypto.c. Mediatek's eMMC inline encryption > > > > > hardware follows the eMMC standard fairly closely, so Peng's patch series just > > > > > sets MMC_CAP2_CRYPTO to make it use the standard cqhci crypto code, and does a > > > > > couple extra things to actually enable the hardware's crypto support on Mediatek > > > > > platforms since it isn't enabled by default. (*Why* it requires an SMC call to > > > > > enable instead of just working as expected, I don't know though.) > > > > > > > > As I have probably indicated earlier, I am starting to become more and > > > > more annoyed with these arm_smccc_smc() calls in generic drivers. > > > > > > > > As a matter of fact, I think the situation is about to explode. Just > > > > do a "git grep arm_smccc_smc" and you will find that it's not only SoC > > > > specific drivers that call them. In general we want to keep drivers > > > > portable and this is clearly moving in the wrong direction. Or maybe > > > > it's just me being grumpy and having a bad day. :-) > > > > > > > > In the Qcom mmc case (drivers/mmc/host/sdhci-msm.c) for eMMC inline > > > > encryption, the arm_smccc_smc() call is slightly better handled as > > > > it's abstracted behind a Qcom specific firmware API. So, sdhci-msm.c > > > > calls qcom_scm_ice_set_key() (implemented in > > > > drivers/firmware/qcom_scm.c) to program a key. I guess we don't have > > > > an abstraction layer that would fit for this case, right? > > > > > > > > My point is, when there is no proper abstraction layer to use for the > > > > relevant arm_smccc_smc() call, the Qcom way is fine to me. > > > > > > > > In this Mediatek case, it looks slightly different. To me it looks > > > > more like a resource that needs to be turned on/off to enable/disable > > > > the "inline encryption engine". Could it be modeled as phy, > > > > power-rail, clock, pinctrl or perhaps behind a PM domain (where SoC > > > > specific calls makes perfect sense). > > > > > > > > Peng can you please elaborate on what goes on behind the > > > > arm_smccc_smc() call, as that would help us to understand what > > > > abstraction layer to pick? > > > > > > > > [...] > > > > > > > > Kind regards > > > > Uffe > > > > > > Hi All, > > > > > > First of all, I appreciated that you are interested in my patch series > > > and give me so much significant suggestions! Then, please let me summary > > > the detail information about MediaTek eMMC hardware crypto IP. > > > > > > Before that, as you know, due to I work for MediaTek.inc that means I'm > > > as an employee in this mail thread, so I don't give any comment about > > > other SoC manufacturers.I will only focus on ours. > > > > > > > > > [Background] Why I upstream this patch series? > > > Obiviously, we want to support hardware level file base encryption, file > > > encryption had been a mandatory feature in mobile device such as Android > > > environment. > > > > > > A few years ago, we only support software level file encryption, it > > > based on the reality of that time: > > > - There is no official encryption specification announced by JEDEC or > > > any device manufacturers > > > - File based encryption is not a mandatory feature for mobile devices > > > - Security is not the highest priority thing for our most of Customers > > > > > > Time can fly and Market requirement is also, hardware level encryption > > > functions had been add in our SoCs in soon, because that: > > > - An encryption specification which is widely recognized by device > > > manufacturers and SoC manufacturers had been announced. Although it > > > doesn't been accepted by JEDEC until now, most of eMMC device > > > manufacturers had support it. > > > - Performance, special in low end mobile device, to some extent, > > > hardware encryption could reduce some CPU loading, > > > - Almost overnight, Security has became the super star, everyone want > > > it, consider the performance (comparing with full disk encryption) and > > > flexibility, file based encryption is indispensable. > > > > > > One more thing, there is no common framework in kernel when our SoCs had > > > crypto IP in that time, so we design a special framework in kernel to > > > support it. In fact, we had support hardware encryption for several > > > years in a special and non-public way. > > > > > > You'll know the rest, Eric design a common framework that lets SoC > > > manufacturers support hardware encryption easier now. That' why we give > > > up our own special private way and try to support it. > > > > > > In fact, at this point in time, we have used this framework(include my > > > patch series) in our mobile products with newest Android version for > > > almost one year. > > > > > > > > > [Your question] Why we need use SMC call in our driver? or Why our > > > crypto hardware IP is not default on? > > > > > > Yes, MediaTek eMMC crypto hardware IP is default off in current design > > > and most important is we only turn it on in ARM exception level 3 > > > (EL3,the highest security level), that means we can only control it > > > under ARM trust firmware (ATF) environment, but kernel space (it's EL2 > > > in our platform). > > > > > > I can get your bewilderment: why it's default off and why put it in high > > > security level control? > > > > Actually, I don't have an issue with this, at all. Instead, my worries > > are about keeping generic drivers portable, which means resources need > > to be modelled through proper abstraction layers. SoC specific drivers > > are different, they don't necessarily need to cope with this > > requirement. > > > > Additionally, to me, it makes perfect sense to allow the crypto IP > > block to be powered off, as you would likely waste energy to have it > > always powered on, especially when it's not being in use. > > > > So, this boils down to understand what "turn on" crypto hardware IP > > actually means? Is it a clock, a phy, a power-rail or perhaps a > > combination of things that is turned on for the IP to work? What > > happens behind the SMC call? > > > > The answer to this question will help us understand what abstraction > > layer we should pick. > > Hi, > > "turn on" crypto hardware IP has no related about clock or power, it > means: > > On: eMMC host encrypt/dencrpt data works normally. > Off: eMMC host encrypt/dencrpt data works error, although clock and > power had been enabled. Error is command timeout or bus hang in our > platforms. > > SMC call means write a register of our SoC specific, a bit of a special > register actually, such as 0 means disable and 1 means enable. Okay, thanks for clarifying. It looks like we have a couple of options to support this. I suggest we consider the two below, but perhaps others (Arnd/Linus?) have better ideas? 1) Model the power on/off of the silicon around the crypto HW through a phy provider driver. The phy provider should then manage the "ice" clock and the SMC call, through the ->power_on|off() callbacks, while the mmc driver would act as the consumer of the phy. This would be rather straightforward to implement, but I guess it can be debated if this fits as a phy or not. 2) Another slightly more complicated solution, would be to manage the SMC call and the "ice" clock through a PM domain (aka genpd provider). As a matter of fact, we already have a couple of references that use the genpd infracture like this, as it allows devices to be turned on/off from SoC specific code, through runtime PM. I wouldn't mind giving you more pointers to examples for inspirations, if this is the option we decide to go for. Kind regards Uffe ^ permalink raw reply [flat|nested] 10+ messages in thread
* Re: [PATCH v2 2/4] mmc: Mediatek: enable crypto hardware engine 2021-03-16 13:55 ` Ulf Hansson @ 2021-03-22 13:45 ` Linus Walleij 2021-03-23 13:37 ` Ulf Hansson 0 siblings, 1 reply; 10+ messages in thread From: Linus Walleij @ 2021-03-22 13:45 UTC (permalink / raw) To: Ulf Hansson Cc: Peng.Zhou, Eric Biggers, linux-block, Chaotian Jing, linux-mmc, open list:OPEN FIRMWARE AND FLATTENED DEVICE TREE BINDINGS, Adrian Hunter, Satya Tangirala, Wulin Li, Arnd Bergmann, Sudeep Holla On Tue, Mar 16, 2021 at 2:56 PM Ulf Hansson <ulf.hansson@linaro.org> wrote: > It looks like we have a couple of options to support this. I suggest > we consider the two below, but perhaps others (Arnd/Linus?) have > better ideas? Admittedly it's a bit hard to shoehorn this in as it is not a standard resource (clk, regulator, genpd, reset, gpio...) There is drivers/soc and then you end up with the same custom abstraction that qcom is using. The upside to using that is that we can #ifdef it to static stubs in the .h file if this SoC is not used, so I would go for that. See for example qcom_scm_ice_invalidate_key() used from drivers/firmware/qcom_scm.c, header is at include/linux/qcom_scm.h and here you find: #if IS_ENABLED(CONFIG_QCOM_SCM) and if not, there are some stubs. Yours, Linus Walleij ^ permalink raw reply [flat|nested] 10+ messages in thread
* Re: [PATCH v2 2/4] mmc: Mediatek: enable crypto hardware engine 2021-03-22 13:45 ` Linus Walleij @ 2021-03-23 13:37 ` Ulf Hansson 0 siblings, 0 replies; 10+ messages in thread From: Ulf Hansson @ 2021-03-23 13:37 UTC (permalink / raw) To: Linus Walleij Cc: Peng.Zhou, Eric Biggers, linux-block, Chaotian Jing, linux-mmc, open list:OPEN FIRMWARE AND FLATTENED DEVICE TREE BINDINGS, Adrian Hunter, Satya Tangirala, Wulin Li, Arnd Bergmann, Sudeep Holla On Mon, 22 Mar 2021 at 14:45, Linus Walleij <linus.walleij@linaro.org> wrote: > > On Tue, Mar 16, 2021 at 2:56 PM Ulf Hansson <ulf.hansson@linaro.org> wrote: > > > It looks like we have a couple of options to support this. I suggest > > we consider the two below, but perhaps others (Arnd/Linus?) have > > better ideas? > > Admittedly it's a bit hard to shoehorn this in as it is not a standard > resource (clk, regulator, genpd, reset, gpio...) In my opinion, I wouldn't object if we would model this as phy, simply because I think it would be the easiest way. Although, I agree, it's not a perfect fit. > > There is drivers/soc and then you end up with the same custom > abstraction that qcom is using. The upside to using that > is that we can #ifdef it to static stubs in the .h file if this SoC > is not used, so I would go for that. > > See for example qcom_scm_ice_invalidate_key() used from > drivers/firmware/qcom_scm.c, header is at > include/linux/qcom_scm.h and here you find: > #if IS_ENABLED(CONFIG_QCOM_SCM) > and if not, there are some stubs. Please, no. As discussed and also pointed out by Arnd in another thread, generic drivers must remain portable and must not get sprinkled with SoC specific code. If not, we would be moving backwards and increasing the fragmentation of the kernel. The qcom case is about programming a crypto key, which seems rather specific to me. I can't figure out another generic way to support this, but using the SoC specific calls. The Mediatek case is about turning on/off a resource to activate the device. If the phy framework doesn't work for us (or another), then at least we should fall back to use runtime PM + PM domain provider (genpd), because this would solve the problem. SoC specific code, like the SMC call can then be called from the genpd provider driver and abstracted from generic drivers. Additionally, in this case the mmc driver has already runtime PM support deployed, which means some of the work has already been completed. > > Yours, > Linus Walleij Kind regards Uffe ^ permalink raw reply [flat|nested] 10+ messages in thread
* Re: [PATCH v2 2/4] mmc: Mediatek: enable crypto hardware engine 2021-03-11 19:08 ` Eric Biggers 2021-03-12 9:05 ` Ulf Hansson @ 2021-03-15 13:41 ` Linus Walleij 2021-03-15 23:02 ` Eric Biggers 1 sibling, 1 reply; 10+ messages in thread From: Linus Walleij @ 2021-03-15 13:41 UTC (permalink / raw) To: Eric Biggers Cc: Peng Zhou, linux-block, Ulf Hansson, Chaotian Jing, linux-mmc, open list:OPEN FIRMWARE AND FLATTENED DEVICE TREE BINDINGS, Adrian Hunter, Satya Tangirala, Wulin Li Hi Eric, thanks for stepping in and clarifying! I get it better now, I though this was some other encryption scheme "on the side". There is one worrying thing in the patch still: On Thu, Mar 11, 2021 at 8:08 PM Eric Biggers <ebiggers@kernel.org> wrote: > On Thu, Mar 11, 2021 at 02:48:23PM +0100, Linus Walleij wrote: > > On Tue, Mar 9, 2021 at 3:06 AM Peng Zhou <peng.zhou@mediatek.com> wrote: > > > + /* > > > + * 1: MSDC_AES_CTL_INIT > > > + * 4: cap_id, no-meaning now > > > + * 1: cfg_id, we choose the second cfg group > > > + */ > > > + if (mmc->caps2 & MMC_CAP2_CRYPTO) > > > + arm_smccc_smc(MTK_SIP_MMC_CONTROL, > > > + 1, 4, 1, 0, 0, 0, 0, &smccc_res); So MSDC_AES_CTL_INIT. Assumes we are using AES and AES only I suppose? > It happens in the same place, cqhci-crypto.c. Mediatek's eMMC inline encryption > hardware follows the eMMC standard fairly closely, so Peng's patch series just > sets MMC_CAP2_CRYPTO to make it use the standard cqhci crypto code, and does a > couple extra things to actually enable the hardware's crypto support on Mediatek > platforms since it isn't enabled by default. (*Why* it requires an SMC call to > enable instead of just working as expected, I don't know though.) Now I don't know the limitations of cqhci crypto. Clearly it only supports AES today. However would the cqhci crypto grow support for any other crypto like 2Fish or DES and the user request this, then I suppose there is no way for the MTK driver to announce "uh no I don't do that"? Or will this cqhci hardware only ever support AES? Yours, Linus Walleij ^ permalink raw reply [flat|nested] 10+ messages in thread
* Re: [PATCH v2 2/4] mmc: Mediatek: enable crypto hardware engine 2021-03-15 13:41 ` Linus Walleij @ 2021-03-15 23:02 ` Eric Biggers 0 siblings, 0 replies; 10+ messages in thread From: Eric Biggers @ 2021-03-15 23:02 UTC (permalink / raw) To: Linus Walleij Cc: Peng Zhou, linux-block, Ulf Hansson, Chaotian Jing, linux-mmc, open list:OPEN FIRMWARE AND FLATTENED DEVICE TREE BINDINGS, Adrian Hunter, Satya Tangirala, Wulin Li On Mon, Mar 15, 2021 at 02:41:58PM +0100, Linus Walleij wrote: > Hi Eric, > > thanks for stepping in and clarifying! I get it better now, I though > this was some other encryption scheme "on the side". > > There is one worrying thing in the patch still: > > On Thu, Mar 11, 2021 at 8:08 PM Eric Biggers <ebiggers@kernel.org> wrote: > > On Thu, Mar 11, 2021 at 02:48:23PM +0100, Linus Walleij wrote: > > > On Tue, Mar 9, 2021 at 3:06 AM Peng Zhou <peng.zhou@mediatek.com> wrote: > > > > > + /* > > > > + * 1: MSDC_AES_CTL_INIT > > > > + * 4: cap_id, no-meaning now > > > > + * 1: cfg_id, we choose the second cfg group > > > > + */ > > > > + if (mmc->caps2 & MMC_CAP2_CRYPTO) > > > > + arm_smccc_smc(MTK_SIP_MMC_CONTROL, > > > > + 1, 4, 1, 0, 0, 0, 0, &smccc_res); > > So MSDC_AES_CTL_INIT. Assumes we are using AES and AES > only I suppose? > > > It happens in the same place, cqhci-crypto.c. Mediatek's eMMC inline encryption > > hardware follows the eMMC standard fairly closely, so Peng's patch series just > > sets MMC_CAP2_CRYPTO to make it use the standard cqhci crypto code, and does a > > couple extra things to actually enable the hardware's crypto support on Mediatek > > platforms since it isn't enabled by default. (*Why* it requires an SMC call to > > enable instead of just working as expected, I don't know though.) > > Now I don't know the limitations of cqhci crypto. Clearly it only supports > AES today. > > However would the cqhci crypto grow support for any other crypto > like 2Fish or DES and the user request this, then I suppose there is > no way for the MTK driver to announce "uh no I don't do that"? > > Or will this cqhci hardware only ever support AES? The standard specifies the encryption algorithms that may be supported, and it specifies that host controllers have a set of crypto capability registers that list the subset of those algorithms that the hardware actually supports. See cqhci_crypto_init() which reads these registers. If new algorithms get added, the hardware won't declare support for them. So what you describe won't be a problem. If, nevertheless, there is broken hardware that declares support for algorithms it doesn't support, we could work around it using a method in cqhci_host_ops. That isn't necessary now though. - Eric ^ permalink raw reply [flat|nested] 10+ messages in thread
end of thread, other threads:[~2021-03-23 13:39 UTC | newest] Thread overview: 10+ messages (download: mbox.gz / follow: Atom feed) -- links below jump to the message on this page -- [not found] <20210309015750.6283-1-peng.zhou@mediatek.com> 2021-03-11 13:48 ` [PATCH v2 2/4] mmc: Mediatek: enable crypto hardware engine Linus Walleij 2021-03-11 19:08 ` Eric Biggers 2021-03-12 9:05 ` Ulf Hansson 2021-03-12 10:47 ` Arnd Bergmann [not found] ` <1615884533.21508.118.camel@mbjsdccf07> 2021-03-16 10:09 ` Ulf Hansson [not found] ` <1615893329.21508.128.camel@mbjsdccf07> 2021-03-16 13:55 ` Ulf Hansson 2021-03-22 13:45 ` Linus Walleij 2021-03-23 13:37 ` Ulf Hansson 2021-03-15 13:41 ` Linus Walleij 2021-03-15 23:02 ` Eric Biggers
This is a public inbox, see mirroring instructions for how to clone and mirror all data and code used for this inbox; as well as URLs for NNTP newsgroup(s).