Re: [PATCH edk2 v1 1/3] StandaloneMmPkg: Fix issue about SpPcpuSharedBufSize field

["Ard Biesheuvel" <ardb@kernel.org>]

On Thu, 16 Dec 2021 at 09:07, Masahisa Kojima
<masahisa.kojima@linaro.org> wrote:
>
> Hi Ard,
>
> On Wed, 15 Dec 2021 at 17:45, Ard Biesheuvel <ardb@kernel.org> wrote:
> >
> > (+ Masahisa, Masami)
> >
> >
> > On Fri, 15 Oct 2021 at 11:07, Ming Huang <huangming@linux.alibaba.com> wrote:
> > >
> > > TF-A: TrustedFirmware-A
> > > SPM: Secure Partition Manager(MM)
> > >
> > > In TF-A, the name of this field is sp_shared_buf_size. This field is
> > > the size of range for transmit data from TF-A to standaloneMM when
> > > SPM enable.
> > >
> > > SpPcpuSharedBufSize is pass from TF-A while StandaloneMM initialize.
> > > So, SpPcpuSharedBufSize should be rename to SpSharedBufSize and this field
> > > should no multiply by PayloadBootInfo->NumCpus;
> > >
> > > Signed-off-by: Ming Huang <huangming@linux.alibaba.com>
> >
> > Could someone please check how this change of interpretation affects
> > standalone MM running on SynQuacer?
>
> In conclusion, this patch does not affect the standalone MM implementation
> on SynQuacer.
>
> In my check, this buffer is used for data sharing between EL3 and Secure-EL1/0.
> Currently this buffer is only used to pass the struct spm_mm_boot_info_t
> from EL3 to StMM during Stmm initialization, I could not find other use cases.
>
> TF-A maps the buffer only 64KB, not multiplied by the number of cores.
> https://git.trustedfirmware.org/TF-A/trusted-firmware-a.git/tree/plat/socionext/synquacer/include/platform_def.h#n98
> So the following definition is something strange and leads to
> misunderstanding, but there is no problem.
>    #define PLAT_SPM_BUF_BASE (BL32_LIMIT - 32 * PLAT_SPM_BUF_SIZE)
> Arm platform statically allocates 1MB buffer in TF-A,
> so I think it is better to follow the arm platform definition.
>
>

Thanks for confirming

> This is different topic, but assertion occurs in DEBUG build if StMM
> and SECURE_BOOT_ENABLED=TRUE.
>
> ===
> Loading driver at 0x000FABB0000 EntryPoint=0x000FABC447C CapsuleRuntimeDxe.efi
> add-symbol-file
> /home/ubuntu/src/sbsa-qemu/Build/DeveloperBox/DEBUG_GCC5/AARCH64/SecurityPkg/VariableAuthenticated/SecureBootConfigDxe/SecureBootConfigDxe/DEBUG/SecureBootConfigDxe.dll
> 0xFA6BA000
> Loading driver at 0x000FA6B9000 EntryPoint=0x000FA6C9610 SecureBootConfigDxe.efi
> add-symbol-file
> /home/ubuntu/src/sbsa-qemu/Build/DeveloperBox/DEBUG_GCC5/AARCH64/MdeModulePkg/Universal/BdsDxe/BdsDxe/DEBUG/BdsDxe.dll
> 0xFA69D000
> Loading driver at 0x000FA69C000 EntryPoint=0x000FA6A7E98 BdsDxe.efi
> DxeCapsuleLibFmp: Failed to lock variable
> 39B68C46-F7FB-441B-B6EC-16B0F69821F3 CapsuleMax.  Status = Invalid
> Parameter
>
> ASSERT_EFI_ERROR (Status = Invalid Parameter)
> ASSERT [BdsDxe]
> /home/ubuntu/src/sbsa-qemu/edk2/MdeModulePkg/Library/DxeCapsuleLibFmp/DxeCapsuleReportLib.c(133):
> !(((INTN)(RETURN_STATUS)(Status)) < 0)
> ===
>
> Probably it is due to the missing support of VariablePolicy, StMM
> outputs the following log output.
> ===
> CommBuffer - 0xFC85CC90, CommBufferSize - 0x52
> !!! DEPRECATED INTERFACE !!! VariableLockRequestToLock() will go away soon!
> !!! DEPRECATED INTERFACE !!! Please move to use Variable Policy!
> !!! DEPRECATED INTERFACE !!! Variable:
> 8BE4DF61-93CA-11D2-AA0D-00E098032B8C PlatformRecovery0000
> ===
>
> That's all for now.
>
> Thanks,
> Masahisa Kojima
>
> >
> >
> > > ---
> > >  StandaloneMmPkg/Include/Library/Arm/StandaloneMmCoreEntryPoint.h                    | 2 +-
> > >  StandaloneMmPkg/Library/StandaloneMmCoreEntryPoint/Arm/CreateHobList.c              | 2 +-
> > >  StandaloneMmPkg/Library/StandaloneMmCoreEntryPoint/Arm/StandaloneMmCoreEntryPoint.c | 2 +-
> > >  3 files changed, 3 insertions(+), 3 deletions(-)
> > >
> > > diff --git a/StandaloneMmPkg/Include/Library/Arm/StandaloneMmCoreEntryPoint.h b/StandaloneMmPkg/Include/Library/Arm/StandaloneMmCoreEntryPoint.h
> > > index c44f7066c6..f1683ecb61 100644
> > > --- a/StandaloneMmPkg/Include/Library/Arm/StandaloneMmCoreEntryPoint.h
> > > +++ b/StandaloneMmPkg/Include/Library/Arm/StandaloneMmCoreEntryPoint.h
> > > @@ -41,7 +41,7 @@ typedef struct {
> > >    UINT64                        SpPcpuStackSize;
> > >    UINT64                        SpHeapSize;
> > >    UINT64                        SpNsCommBufSize;
> > > -  UINT64                        SpPcpuSharedBufSize;
> > > +  UINT64                        SpSharedBufSize;
> > >    UINT32                        NumSpMemRegions;
> > >    UINT32                        NumCpus;
> > >    EFI_SECURE_PARTITION_CPU_INFO *CpuInfo;
> > > diff --git a/StandaloneMmPkg/Library/StandaloneMmCoreEntryPoint/Arm/CreateHobList.c b/StandaloneMmPkg/Library/StandaloneMmCoreEntryPoint/Arm/CreateHobList.c
> > > index 85f8194687..93773c9fe8 100644
> > > --- a/StandaloneMmPkg/Library/StandaloneMmCoreEntryPoint/Arm/CreateHobList.c
> > > +++ b/StandaloneMmPkg/Library/StandaloneMmCoreEntryPoint/Arm/CreateHobList.c
> > > @@ -173,7 +173,7 @@ CreateHobListFromBootInfo (
> > >    // Base and size of buffer shared with privileged Secure world software
> > >    MmramRanges[1].PhysicalStart = PayloadBootInfo->SpSharedBufBase;
> > >    MmramRanges[1].CpuStart      = PayloadBootInfo->SpSharedBufBase;
> > > -  MmramRanges[1].PhysicalSize  = PayloadBootInfo->SpPcpuSharedBufSize * PayloadBootInfo->NumCpus;
> > > +  MmramRanges[1].PhysicalSize  = PayloadBootInfo->SpSharedBufSize;
> > >    MmramRanges[1].RegionState   = EFI_CACHEABLE | EFI_ALLOCATED;
> > >
> > >    // Base and size of buffer used for synchronous communication with Normal
> > > diff --git a/StandaloneMmPkg/Library/StandaloneMmCoreEntryPoint/Arm/StandaloneMmCoreEntryPoint.c b/StandaloneMmPkg/Library/StandaloneMmCoreEntryPoint/Arm/StandaloneMmCoreEntryPoint.c
> > > index 49cf51a789..5db7019dda 100644
> > > --- a/StandaloneMmPkg/Library/StandaloneMmCoreEntryPoint/Arm/StandaloneMmCoreEntryPoint.c
> > > +++ b/StandaloneMmPkg/Library/StandaloneMmCoreEntryPoint/Arm/StandaloneMmCoreEntryPoint.c
> > > @@ -87,7 +87,7 @@ GetAndPrintBootinformation (
> > >    DEBUG ((DEBUG_INFO, "SpPcpuStackSize - 0x%x\n", PayloadBootInfo->SpPcpuStackSize));
> > >    DEBUG ((DEBUG_INFO, "SpHeapSize      - 0x%x\n", PayloadBootInfo->SpHeapSize));
> > >    DEBUG ((DEBUG_INFO, "SpNsCommBufSize - 0x%x\n", PayloadBootInfo->SpNsCommBufSize));
> > > -  DEBUG ((DEBUG_INFO, "SpPcpuSharedBufSize - 0x%x\n", PayloadBootInfo->SpPcpuSharedBufSize));
> > > +  DEBUG ((DEBUG_INFO, "SpSharedBufSize - 0x%x\n", PayloadBootInfo->SpSharedBufSize));
> > >
> > >    DEBUG ((DEBUG_INFO, "NumCpus         - 0x%x\n", PayloadBootInfo->NumCpus));
> > >    DEBUG ((DEBUG_INFO, "CpuInfo         - 0x%p\n", PayloadBootInfo->CpuInfo));
> > > --
> > > 2.17.1
> > >