Re: [PATCH v7 6/9] scsi: ufs: Add inline encryption support to UFS

[Stanley Chu <stanley.chu@mediatek.com>]

Hi Eric,

On Tue, 2020-02-25 at 17:12 -0800, Eric Biggers wrote:
> On Tue, Feb 25, 2020 at 03:21:25PM +0800, Stanley Chu wrote:
> > Hi Christoph,
> > 
> > On Mon, 2020-02-24 at 15:37 -0800, Christoph Hellwig wrote:
> > > On Sun, Feb 23, 2020 at 09:47:36PM +0800, Stanley Chu wrote:
> > > > Yes, MediaTek is keeping work closely with inline encryption patch sets.
> > > > Currently the v6 version can work well (without
> > > > UFSHCD_QUIRK_BROKEN_CRYPTO quirk) at least in our MT6779 SoC platform
> > > > which basic SoC support and some other peripheral drivers are under
> > > > upstreaming as below link,
> > > > 
> > > > https://patchwork.kernel.org/project/linux-mediatek/list/?state=%
> > > > 2A&q=6779&series=&submitter=&delegate=&archive=both
> > > > 
> > > > The integration with inline encryption patch set needs to patch
> > > > ufs-mediatek and patches are ready in downstream. We plan to upstream
> > > > them soon after inline encryption patch sets get merged.
> > > 
> > > What amount of support do you need in ufs-mediatek?  It seems like
> > > pretty much every ufs low-level driver needs some kind of specific
> > > support now, right?  I wonder if we should instead opt into the support
> > > instead of all the quirking here.
> > 
> > The patch in ufs-mediatek is aimed to issue vendor-specific SMC calls
> > for host initialization and configuration. This is because MediaTek UFS
> > host has some "secure-protected" registers/features which need to be
> > accessed/switched in secure world. 
> > 
> > Such protection is not mentioned by UFSHCI specifications thus inline
> > encryption patch set assumes that every registers in UFSHCI can be
> > accessed normally in kernel. This makes sense and surely the patchset
> > can work fine in any "standard" UFS host. However if host has special
> > design then it can work normally only if some vendor-specific treatment
> > is applied.
> > 
> > I think one of the reason to apply UFSHCD_QUIRK_BROKEN_CRYPTO quirk in
> > ufs-qcom host is similar to above case.
> 
> So, I had originally assumed that most kernel developers would prefer to make
> the UFS crypto support opt-out rather than opt-in, since that matches the normal
> Linux way of doing things.  I.e. normally the kernel's default assumption is
> that devices implement the relevant standard, and only when a device is known to
> deviate from the standard does the driver apply quirks.
> 
> But indeed, as we've investigated more vendors' UFS hardware, it seems that
> everyone has some quirk that needs to be handled in the platform driver:
> 
>   - ufs-qcom (tested on DragonBoard 845c with upstream kernel) needs
>     vendor-specific crypto initialization logic and SMC calls to set keys
> 
>   - ufs-mediatek needs the quirks that Stanley mentioned above
> 
>   - ufs-hisi (tested on Hikey960 with upstream kernel) needs to write a
>     vendor-specific register to use high keyslots, but even then I still
>     couldn't get the crypto support working correctly.
> 
> I'm not sure about the UFS controllers from Synopsys, Cadence, or Samsung, all
> of which apparently have implemented some form of the crypto support too.  But I
> wouldn't get my hopes up that everyone followed the UFS standard precisely.
> 
> So if there are no objections, IMO we should make the crypto support opt-in.
> 
> That makes it even more important to upstream the crypto support for specific
> hardware like ufs-qcom and ufs-mediatek, since otherwise the ufshcd-crypto code
> would be unusable even theoretically.  I'm volunteering to handle ufs-qcom with
> https://lkml.kernel.org/linux-block/20200110061634.46742-1-ebiggers@kernel.org/.
> Stanley, could you send out ufs-mediatek support as an RFC so people can see
> better what it involves?

Sure, I will send out our RFC patches. Please allow me some time for
submission.

Thanks,
Stanley Chu
> - Eric