Hi Barani, On Wed, Mar 04, 2020 at 12:18:20PM -0800, bmuthuku@xxxxxxxxxxxxxx wrote: > Eric, I strongly recommend not to support the old mechanism of calling into > TEE to set keys as this has been deprecated and will not work with newer > hardware. Actually, I've also verified that this driver works nearly as-is on Snapdragon 865 and Snapdragon 765. (Those SoCs currently lack upstream support, but I tested with downstream kernels.) IIUC, this is the latest Qualcomm hardware in the real world. Lots of phones will be shipping with those SoCS this year. So your position is that crypto support for current Qualcomm hardware shouldn't be added, and instead we should wait years for new hardware that hasn't even been announced/released yet and has no upstream kernel support at all yet? Snapdragon 845 (via DragonBoard 845c) already has upstream support. We can easily support multiple hardware versions in the driver, so I don't see why this is an issue. In particular, if Qualcomm releases new hardware that just complies with the UFS standard and doesn't require these weird vendor-specific SCM calls, we can just conditionally skip the quirks when standard hardware is detected. > There are few issues with this patch, it adds all the code within > UFS and we would have to reimplement all the common ICE code for eMMC as > well. The downstream solution of having a separate ICE "device" and "driver" is really horrendous and results in lots of layering violations and unnecessary code, since as far as I can tell ICE is actually part of the UFS and eMMC controllers and not a separate device. So I think starting with the ICE logic in ufs-qcom is the right approach. If it turns out that sdhci-msm needs similar ICE management code, we can put it in a library. Though, note that after I removed all the unneeded stuff, we're only talking about ~200 lines of code, so it may not even be worthwhile. However, an issue with eMMC is that the eMMC crypto standard hasn't actually been published yet. And I haven't heard of any vendors implementing it besides Qualcomm, nor do I know of any upstream SoC we can use to develop it. So if you want eMMC crypto support, you're going to have to help with it. > For clearing a key, the patch uses program_key to set zeroes to the > keyslot, without going into details (since newer hardware is not yet public) > this will not work. That's incorrect; this patch uses the SCM call QCOM_SCM_ES_INVALIDATE_ICE_KEY to evict keys. That's exactly what the downstream driver does. Of course, when we add support for newer hardware we can use whatever keyslot management method works on that hardware. Hopefully, it's the standard UFS method and not another weird vendor-specific method. > We have a plan to upstream ICE support with the new hardware along with the > framework to support wrapped keys and add sdhci/cqhci-crypto support. Great, what's the timeline on this? Are there patches you can point to anywhere? How would one get ahold of this hardware, and does it / will it have upstream kernel support like sda845 does? - Eric
