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


Ming Huang
 

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


Ard Biesheuvel
 

(+ 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?


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


Masahisa Kojima
 

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


Jeff Fan
 

You may try to enlarger stmm_ns_comm_buf_size in Optee to check if it could solve ASSERT issue.


fanjianfeng@...

 
Date: 2021-12-16 16:07
Subject: Re: [edk2-devel] [PATCH edk2 v1 1/3] StandaloneMmPkg: Fix issue about SpPcpuSharedBufSize field
Hi Ard,
 
On Wed, 15 Dec 2021 at 17:45, Ard Biesheuvel <ardb@...> wrote:
>
> (+ Masahisa, Masami)
>
>
> On Fri, 15 Oct 2021 at 11:07, Ming Huang <huangming@...> 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@...>
>
> 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
> >
 
 
 
 
 


Ard Biesheuvel
 

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