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

["Masahisa Kojima" <masahisa.kojima@linaro.org>]

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.


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
> >