On Wed, Jul 10, 2024 at 11:22:52AM +0530, Alim Akhtar wrote: > Hello Eric, > > > -----Original Message----- > > From: Eric Biggers <ebiggers@xxxxxxxxxx> > > Sent: Tuesday, July 9, 2024 5:19 AM > > To: Peter Griffin <peter.griffin@xxxxxxxxxx> > > Cc: linux-scsi@xxxxxxxxxxxxxxx; linux-samsung-soc@xxxxxxxxxxxxxxx; linux- > > fscrypt@xxxxxxxxxxxxxxx; Alim Akhtar <alim.akhtar@xxxxxxxxxxx>; Avri > > Altman <avri.altman@xxxxxxx>; Bart Van Assche <bvanassche@xxxxxxx>; > > Martin K . Petersen <martin.petersen@xxxxxxxxxx>; André Draszik > > <andre.draszik@xxxxxxxxxx>; William McVicker <willmcvicker@xxxxxxxxxx> > > Subject: Re: [PATCH v2 6/6] scsi: ufs: exynos: Add support for Flash > Memory > > Protector (FMP) > > > > On Mon, Jul 08, 2024 at 01:26:30PM -0700, Eric Biggers wrote: > > > Hi Peter, > > > > > > On Thu, Jul 04, 2024 at 02:26:05PM +0100, Peter Griffin wrote: > > > > Do you know how these FMP registers (FMPSECURITY0 etc) relate to the > > > > UFSPR* registers set in the existing exynos_ufs_config_smu()? The > > > > UFS_LINK spec talks about UFSPR(FMP), so I had assumed the FMP > > > > support would be writing these same registers but via SMC call. > > > > > > > > I think by the looks of things > > > > > > > > #define UFSPRSECURITY 0x010 > > > > #define UFSPSBEGIN0 0x200 > > > > #define UFSPSEND0 0x204 > > > > #define UFSPSLUN0 0x208 > > > > #define UFSPSCTRL0 0x20C > > > > > > > > relates to the following registers in gs101 spec > > > > > > > > FMPSECURITY0 0x0010 > > > > FMPSBEGIN0 0x2000 > > > > FMPSEND0 0x2004 > > > > FMPSLUN0 0x2008 > > > > FMPSCTRL0 0x200C > > > > > > > > And the SMC calls your calling set those same registers as > > > > exynos_ufs_config_smu() function. Although it is hard to be certain > > > > as I don't have access to the firmware code. Certainly the comment > > > > below about FMPSECURITY0 implies that :) > > > > > > > > With that in mind I think exynos_ufs_fmp_init() function in this > > > > patch needs to be better integrated with the > > > > EXYNOS_UFS_OPT_UFSPR_SECURE flag and the existing > > > > exynos_ufs_config_smu() function that is currently just disabling > > > > decryption on platforms where it can access the UFSPR(FMP) regs via > > mmio. > > > > > > I think that is all correct. For some reason, on gs101 the FMP > > > registers are not accessible by the "normal world", and SMC calls need > to > > be used instead. > > > The sequences of SMC calls originated from Samsung's Linux driver code > > for FMP. > > > So I know they are the magic incantations that are needed, but I don't > > > have access to the source code or documentation for them. It does > > > seem clear that one of the things they must do is write the needed > values > > to the FMP registers. > > > > > > I'd hope that these same SMC calls also work on Exynos-based SoCs that > > > do make the FMP registers accessible to the "normal world", and > > > therefore they can just be used on all Exynos-based SoCs and > > > ufs-exynos won't need two different code paths. But I don't have a > > > way to confirm this myself. Until someone is able to confirm this, I > > > think we need to make the FMP support depend on > > > EXYNOS_UFS_OPT_UFSPR_SECURE so that it doesn't conflict with > > > exynos_ufs_config_smu() which runs when > > !EXYNOS_UFS_OPT_UFSPR_SECURE. > > > > > > > These same SMC calls can be found in the downstream source for other > > Exynos-based SoCs. I suspect that exynos_ufs_config_smu() should be > > removed, and exynos_ufs_fmp_init() should run regardless of > > EXYNOS_UFS_OPT_UFSPR_SECURE. > > It still would need to be tested, though, which I'm not able to do. (And > > especially as a cryptography feature, this *must* be tested...) So for > now I'm > > going to make the FMP support conditional on > > EXYNOS_UFS_OPT_UFSPR_SECURE. > > > SMU controls the security access aspect of the FMP, one can have a usecase > where one wants to enable inline encryption using FMP in a non-secure > mode/world after a secure boot of the system > and in another case, configure FMP in secure mode/world during secure boot. > I am not sure how it is designed in gs101 though. > Currently, exynos_ufs_config_smu() just allows SMU registers modification by > non-secure world. > Apparently, gs101 has two levels of access control for FMP. Access to the range configuration registers like FMPSBEGIN0 (UFSPSBEGIN0 in the upstream source) is controlled by FMPSECURITY0.NSSMU (UFSPRSECURITY.NSSMU in the upstream source). But access to FMPSECURITY0 itself is controlled by TZPC.NSFMP, which is writable only by the "secure world". In the current upstream source, exynos_ufs_config_smu() writes to FMPSECURITY0, and this crashes on gs101 if actually executed (it's currently disabled on gs101 for that reason). So the secure world software on gs101 must be setting TZPC.NSFMP = 0. I expect that this isn't specific to gs101, and some other Exynos-based SoCs use this same design. This would explain the presence of the SMC calls in the downstream source used on other Exynos-based SoCs. So it seems that for now ufs-exynos has to use the SMC calls, as this patch does. This is similar to how ufs-qcom and ufs-mediatek similarly use their SoC's respective set of SMC calls to configure inline encryption, in order to work around similar designs where the inline encryption hardware can only be configured by secure world software. (I don't know why so many SoC vendors are choosing this design, given that in practice only the normal world wants to configure inline encryption. The detour to the secure world seems unnecessary.) - Eric
