RE: [PATCH v2 6/6] scsi: ufs: exynos: Add support for Flash Memory Protector (FMP)

["Alim Akhtar" <alim.akhtar@samsung.com>]

Hello Eric,

> -----Original Message-----
> From: Eric Biggers <ebiggers@kernel.org>
> Sent: Tuesday, July 9, 2024 5:19 AM
> To: Peter Griffin <peter.griffin@linaro.org>
> Cc: linux-scsi@vger.kernel.org; linux-samsung-soc@vger.kernel.org; linux-
> fscrypt@vger.kernel.org; Alim Akhtar <alim.akhtar@samsung.com>; Avri
> Altman <avri.altman@wdc.com>; Bart Van Assche <bvanassche@acm.org>;
> Martin K . Petersen <martin.petersen@oracle.com>; André Draszik
> <andre.draszik@linaro.org>; William McVicker <willmcvicker@google.com>
> 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.

> - Eric