Re: [edk2-devel] [Patch] StandaloneMmPkg: Fixed communicating from TF-A failed issue

["Ming Huang" <huangming@linux.alibaba.com>]

On 6/16/21 10:10 PM, Ard Biesheuvel wrote:
> On Wed, 16 Jun 2021 at 07:30, Omkar Kulkarni <Omkar.Kulkarni@arm.com> wrote:
>>
>>
>> On 6/10/21 6:44 AM, Ming Huang via groups.io wrote:
>>> On 6/9/21 3:10 PM, Ard Biesheuvel wrote:
>>>> On Tue, 8 Jun 2021 at 16:21, Ming Huang <huangming@linux.alibaba.com>
>>> wrote:
>>>>>
>>>>> TF-A: TrustedFirmware-a
>>>>> SPM: Secure Partition Manager(MM)
>>>>>
>>>>> For AArch64, when SPM enable in TF-A, TF-A may communicate to MM
>>> with
>>>>> buffer address (PLAT_SPM_BUF_BASE). The address is different from
>>>>> PcdMmBufferBase which use in edk2.
>>>>
>>>> Then why do we have PcdMmBufferBase?
>>>
>>> ArmPkg use this Pcd for the base address of non-secure communication
>>> buffer.
>>>
>>>>
>>>> Is it possible to set PcdMmBufferBase to the correct value?
>>>
>>> The secure communication may interrupt the non-secure communication. if
>>> we use the same address (PcdMmBufferBase and PLAT_SPM_BUF_BASE), the
>>> date in communication buffer may be corrupted.
>>>
>>> Best Regards,
>>> Ming
>>
>> In case where an interrupt handler executing from EL3 makes a call into StandaloneMM, the handler in EL3 makes an spm call into StandaloneMM using PLAT_SPM_BUF_BASE buffer base address. This PLAT_SPM_BUF_BASE is a shared buffer between EL3 and S-EL0. This is where the following check fails and leads to spm call failure. So this change would help resolve this issue.
>>
> 
> But is it the right fix? Why would EDK2 even be aware of how EL3 and
> S-EL0 communicate with each other, and where the buffer is located?

The root cause for this problem is that the StandaloneMmPkg and TF-A
are not full cooperative.
PLAT_SPM_BUF_BASE is not dynamic buffer just like PcdMmBufferBase.

Please refer PcdMmBufferBase comments in edk2-platforms/Platform/ARM/SgiPkg/SgiPlatform.dsc.inc: 
  #
  # Set the base address and size of the buffer used
  # for communication between the Normal world edk2
  # with StandaloneMm image at S-EL0 through MM_COMMUNICATE.
  # This buffer gets allocated in ATF and since we do not have
  # a mechanism currently to communicate the base address and
  # size of this buffer from ATF, hard-code it here
  #
  ## MM Communicate
  gArmTokenSpaceGuid.PcdMmBufferBase|0xFF600000
  gArmTokenSpaceGuid.PcdMmBufferSize|0x10000


Best Regards,
Ming

> 
> 
>>>
>>>>
>>>>> Checking address will let TF-A communicate failed to MM. So remove
>>>>> below checking code:
>>>>> if (NsCommBufferAddr < mNsCommBuffer.PhysicalStart) {
>>>>>   return EFI_ACCESS_DENIED;
>>>>> }
>>>>>
>>>>> Signed-off-by: Ming Huang <huangming@linux.alibaba.com>
>>>>> ---
>>>>>  StandaloneMmPkg/Drivers/StandaloneMmCpu/AArch64/EventHandle.c |
>>> 4
>>>>> ----
>>>>>  1 file changed, 4 deletions(-)
>>>>>
>>>>> diff --git
>>>>> a/StandaloneMmPkg/Drivers/StandaloneMmCpu/AArch64/EventHandle.c
>>>>> b/StandaloneMmPkg/Drivers/StandaloneMmCpu/AArch64/EventHandle.c
>>>>> index 63fbe26642..fe98d3181d 100644
>>>>> ---
>>> a/StandaloneMmPkg/Drivers/StandaloneMmCpu/AArch64/EventHandle.c
>>>>> +++
>>> b/StandaloneMmPkg/Drivers/StandaloneMmCpu/AArch64/EventHandle.c
>>>>> @@ -103,10 +103,6 @@ PiMmStandaloneArmTfCpuDriverEntry (
>>>>>      return EFI_INVALID_PARAMETER;
>>>>>    }
>>>>>
>>>>> -  if (NsCommBufferAddr < mNsCommBuffer.PhysicalStart) {
>>>>> -    return EFI_ACCESS_DENIED;
>>>>> -  }
>>>>> -
>>>>>    if ((NsCommBufferAddr + sizeof (EFI_MM_COMMUNICATE_HEADER)) >=
>>>>>        (mNsCommBuffer.PhysicalStart + mNsCommBuffer.PhysicalSize)) {
>>>>>      return EFI_INVALID_PARAMETER;
>>>>> --
>>>>> 2.17.1
>>>>>
>>>
>>>
>>> 
>>>
>>