On Tue, 16 Mar 2021 at 12:22, Peng.Zhou <peng.zhou@xxxxxxxxxxxx> 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@xxxxxxxxxxxx> 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@xxxxxxxxxx> 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@xxxxxxxxxxxx> wrote: > > > > > > > > > > > > > Use SMC call enable hardware crypto engine > > > > > > > due to it only be changed in ATF(EL3). > > > > > > > > > > > > > > Signed-off-by: Peng Zhou <peng.zhou@xxxxxxxxxxxx> > > > > > > > > > > > > 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
