* [Patch] StandaloneMmPkg: Fixed communicating from TF-A failed issue
@ 2021-06-08 14:21 Ming Huang
2021-06-09 7:10 ` Ard Biesheuvel
0 siblings, 1 reply; 6+ messages in thread
From: Ming Huang @ 2021-06-08 14:21 UTC (permalink / raw)
To: devel, ardb+tianocore, sami.mujawar, jiewen.yao,
supreeth.venkatesh
Cc: guoheyi, Ming Huang
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.
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
^ permalink raw reply related [flat|nested] 6+ messages in thread
* Re: [Patch] StandaloneMmPkg: Fixed communicating from TF-A failed issue
2021-06-08 14:21 [Patch] StandaloneMmPkg: Fixed communicating from TF-A failed issue Ming Huang
@ 2021-06-09 7:10 ` Ard Biesheuvel
2021-06-10 1:14 ` Ming Huang
0 siblings, 1 reply; 6+ messages in thread
From: Ard Biesheuvel @ 2021-06-09 7:10 UTC (permalink / raw)
To: Ming Huang
Cc: edk2-devel-groups-io, Ard Biesheuvel, Sami Mujawar, Jiewen Yao,
Supreeth Venkatesh, guoheyi
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?
Is it possible to set PcdMmBufferBase to the correct value?
> 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
>
^ permalink raw reply [flat|nested] 6+ messages in thread
* Re: [Patch] StandaloneMmPkg: Fixed communicating from TF-A failed issue
2021-06-09 7:10 ` Ard Biesheuvel
@ 2021-06-10 1:14 ` Ming Huang
2021-06-16 5:29 ` [edk2-devel] " Omkar Anand Kulkarni
0 siblings, 1 reply; 6+ messages in thread
From: Ming Huang @ 2021-06-10 1:14 UTC (permalink / raw)
To: Ard Biesheuvel
Cc: edk2-devel-groups-io, Ard Biesheuvel, Sami Mujawar, Jiewen Yao,
Supreeth Venkatesh, guoheyi
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
>
>> 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
>>
^ permalink raw reply [flat|nested] 6+ messages in thread
* Re: [edk2-devel] [Patch] StandaloneMmPkg: Fixed communicating from TF-A failed issue
2021-06-10 1:14 ` Ming Huang
@ 2021-06-16 5:29 ` Omkar Anand Kulkarni
2021-06-16 14:10 ` Ard Biesheuvel
0 siblings, 1 reply; 6+ messages in thread
From: Omkar Anand Kulkarni @ 2021-06-16 5:29 UTC (permalink / raw)
To: devel@edk2.groups.io, huangming@linux.alibaba.com, Ard Biesheuvel
Cc: Ard Biesheuvel, Sami Mujawar, Jiewen Yao, Supreeth Venkatesh,
guoheyi@linux.alibaba.com, nd
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.
- Omkar
>
> >
> >> 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
> >>
>
>
>
>
^ permalink raw reply [flat|nested] 6+ messages in thread
* Re: [edk2-devel] [Patch] StandaloneMmPkg: Fixed communicating from TF-A failed issue
2021-06-16 5:29 ` [edk2-devel] " Omkar Anand Kulkarni
@ 2021-06-16 14:10 ` Ard Biesheuvel
2021-06-18 3:42 ` Ming Huang
0 siblings, 1 reply; 6+ messages in thread
From: Ard Biesheuvel @ 2021-06-16 14:10 UTC (permalink / raw)
To: Omkar Kulkarni
Cc: devel@edk2.groups.io, huangming@linux.alibaba.com, Ard Biesheuvel,
Sami Mujawar, Jiewen Yao, Supreeth Venkatesh,
guoheyi@linux.alibaba.com, nd
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?
> >
> > >
> > >> 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
> > >>
> >
> >
> >
> >
>
^ permalink raw reply [flat|nested] 6+ messages in thread
* Re: [edk2-devel] [Patch] StandaloneMmPkg: Fixed communicating from TF-A failed issue
2021-06-16 14:10 ` Ard Biesheuvel
@ 2021-06-18 3:42 ` Ming Huang
0 siblings, 0 replies; 6+ messages in thread
From: Ming Huang @ 2021-06-18 3:42 UTC (permalink / raw)
To: Ard Biesheuvel, Omkar Kulkarni
Cc: devel@edk2.groups.io, Ard Biesheuvel, Sami Mujawar, Jiewen Yao,
Supreeth Venkatesh, guoheyi@linux.alibaba.com, nd
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
>>>>>
>>>
>>>
>>>
>>>
>>
^ permalink raw reply [flat|nested] 6+ messages in thread
end of thread, other threads:[~2021-06-18 3:42 UTC | newest]
Thread overview: 6+ messages (download: mbox.gz follow: Atom feed
-- links below jump to the message on this page --
2021-06-08 14:21 [Patch] StandaloneMmPkg: Fixed communicating from TF-A failed issue Ming Huang
2021-06-09 7:10 ` Ard Biesheuvel
2021-06-10 1:14 ` Ming Huang
2021-06-16 5:29 ` [edk2-devel] " Omkar Anand Kulkarni
2021-06-16 14:10 ` Ard Biesheuvel
2021-06-18 3:42 ` Ming Huang
This is a public inbox, see mirroring instructions
for how to clone and mirror all data and code used for this inbox