To prevent the initial MMU->GCD memory space map synchronization from
stripping permissions attributes [which we cannot use in the GCD memory
space map, unfortunately], implement the same approach as x86, and ignore
SetMemoryAttributes() calls during the time SyncCacheConfig() is in
progress. This is a horrible hack, but is currently the only way we can
implement strict permissions on arbitrary memory regions [as opposed to
PE/COFF text/data sections only]

Contributed-under: TianoCore Contribution Agreement 1.0
Signed-off-by: Ard Biesheuvel <ard.biesheuvel@linaro.org>
Reviewed-by: Jiewen Yao <jiewen.yao@intel.com>
---
 ArmPkg/Drivers/CpuDxe/CpuDxe.c       | 3 +++
 ArmPkg/Drivers/CpuDxe/CpuDxe.h       | 1 +
 ArmPkg/Drivers/CpuDxe/CpuMmuCommon.c | 4 ++++
 3 files changed, 8 insertions(+)

diff --git a/ArmPkg/Drivers/CpuDxe/CpuDxe.c b/ArmPkg/Drivers/CpuDxe/CpuDxe.c
index 5aa5b874144a..1955d1dece03 100644
--- a/ArmPkg/Drivers/CpuDxe/CpuDxe.c
+++ b/ArmPkg/Drivers/CpuDxe/CpuDxe.c
@@ -17,6 +17,7 @@
 
 #include <Guid/IdleLoopEvent.h>
 
+BOOLEAN                   gIsFlushingGCD;
 
 /**
   This function flushes the range of addresses from Start to Start+Length
@@ -261,7 +262,9 @@ CpuDxeInitialize (
   // and that calls EFI_CPU_ARCH_PROTOCOL.SetMemoryAttributes, so this code needs to go
   // after the protocol is installed
   //
+  gIsFlushingGCD = TRUE;
   SyncCacheConfig (&mCpu);
+  gIsFlushingGCD = FALSE;
 
   // If the platform is a MPCore system then install the Configuration Table describing the
   // secondary core states
diff --git a/ArmPkg/Drivers/CpuDxe/CpuDxe.h b/ArmPkg/Drivers/CpuDxe/CpuDxe.h
index a00fc3064362..085e4cab2921 100644
--- a/ArmPkg/Drivers/CpuDxe/CpuDxe.h
+++ b/ArmPkg/Drivers/CpuDxe/CpuDxe.h
@@ -37,6 +37,7 @@
 #include <Protocol/DebugSupportPeriodicCallback.h>
 #include <Protocol/LoadedImage.h>
 
+extern BOOLEAN gIsFlushingGCD;
 
 /**
   This function registers and enables the handler specified by InterruptHandler for a processor
diff --git a/ArmPkg/Drivers/CpuDxe/CpuMmuCommon.c b/ArmPkg/Drivers/CpuDxe/CpuMmuCommon.c
index ebe593d1c325..6dfec7e55888 100644
--- a/ArmPkg/Drivers/CpuDxe/CpuMmuCommon.c
+++ b/ArmPkg/Drivers/CpuDxe/CpuMmuCommon.c
@@ -188,6 +188,10 @@ CpuSetMemoryAttributes (
   UINTN       RegionLength;
   UINTN       RegionArmAttributes;
 
+  if (gIsFlushingGCD) {
+    return EFI_SUCCESS;
+  }
+
   if ((BaseAddress & (SIZE_4KB - 1)) != 0) {
     // Minimum granularity is SIZE_4KB (4KB on ARM)
     DEBUG ((EFI_D_PAGE, "CpuSetMemoryAttributes(%lx, %lx, %lx): Minimum ganularity is SIZE_4KB\n", BaseAddress, Length, EfiAttributes));
-- 
2.7.4

_______________________________________________
edk2-devel mailing list
edk2-devel@lists.01.org
https://lists.01.org/mailman/listinfo/edk2-devel

On Mon, Feb 27, 2017 at 02:38:05PM +0000, Ard Biesheuvel wrote:
> To prevent the initial MMU->GCD memory space map synchronization from
> stripping permissions attributes [which we cannot use in the GCD memory
> space map, unfortunately], implement the same approach as x86, and ignore
> SetMemoryAttributes() calls during the time SyncCacheConfig() is in
> progress. This is a horrible hack, but is currently the only way we can
> implement strict permissions on arbitrary memory regions [as opposed to
> PE/COFF text/data sections only]

Sounds like another excellent argument for why this CpuDxe should be
cosying up with the UefiCpuPkg one longer-term.

> Contributed-under: TianoCore Contribution Agreement 1.0
> Signed-off-by: Ard Biesheuvel <ard.biesheuvel@linaro.org>
> Reviewed-by: Jiewen Yao <jiewen.yao@intel.com>
> ---
>  ArmPkg/Drivers/CpuDxe/CpuDxe.c       | 3 +++
>  ArmPkg/Drivers/CpuDxe/CpuDxe.h       | 1 +
>  ArmPkg/Drivers/CpuDxe/CpuMmuCommon.c | 4 ++++
>  3 files changed, 8 insertions(+)
> 
> diff --git a/ArmPkg/Drivers/CpuDxe/CpuDxe.c b/ArmPkg/Drivers/CpuDxe/CpuDxe.c
> index 5aa5b874144a..1955d1dece03 100644
> --- a/ArmPkg/Drivers/CpuDxe/CpuDxe.c
> +++ b/ArmPkg/Drivers/CpuDxe/CpuDxe.c
> @@ -17,6 +17,7 @@
>  
>  #include <Guid/IdleLoopEvent.h>
>  
> +BOOLEAN                   gIsFlushingGCD;

OK, I am unable to not bikeshed this:
The behaviour you're copying is implemented via a variable called
mIsFlushingGCD. Why change the prefix? Surely we're not looking to
export this variable any further in ARM?

>  
>  /**
>    This function flushes the range of addresses from Start to Start+Length
> @@ -261,7 +262,9 @@ CpuDxeInitialize (
>    // and that calls EFI_CPU_ARCH_PROTOCOL.SetMemoryAttributes, so this code needs to go
>    // after the protocol is installed
>    //
> +  gIsFlushingGCD = TRUE;
>    SyncCacheConfig (&mCpu);
> +  gIsFlushingGCD = FALSE;
>  
>    // If the platform is a MPCore system then install the Configuration Table describing the
>    // secondary core states
> diff --git a/ArmPkg/Drivers/CpuDxe/CpuDxe.h b/ArmPkg/Drivers/CpuDxe/CpuDxe.h
> index a00fc3064362..085e4cab2921 100644
> --- a/ArmPkg/Drivers/CpuDxe/CpuDxe.h
> +++ b/ArmPkg/Drivers/CpuDxe/CpuDxe.h
> @@ -37,6 +37,7 @@
>  #include <Protocol/DebugSupportPeriodicCallback.h>
>  #include <Protocol/LoadedImage.h>
>  
> +extern BOOLEAN gIsFlushingGCD;

Eew ... this suggest we are.

/
    Leif

>  
>  /**
>    This function registers and enables the handler specified by InterruptHandler for a processor
> diff --git a/ArmPkg/Drivers/CpuDxe/CpuMmuCommon.c b/ArmPkg/Drivers/CpuDxe/CpuMmuCommon.c
> index ebe593d1c325..6dfec7e55888 100644
> --- a/ArmPkg/Drivers/CpuDxe/CpuMmuCommon.c
> +++ b/ArmPkg/Drivers/CpuDxe/CpuMmuCommon.c
> @@ -188,6 +188,10 @@ CpuSetMemoryAttributes (
>    UINTN       RegionLength;
>    UINTN       RegionArmAttributes;
>  
> +  if (gIsFlushingGCD) {
> +    return EFI_SUCCESS;
> +  }
> +
>    if ((BaseAddress & (SIZE_4KB - 1)) != 0) {
>      // Minimum granularity is SIZE_4KB (4KB on ARM)
>      DEBUG ((EFI_D_PAGE, "CpuSetMemoryAttributes(%lx, %lx, %lx): Minimum ganularity is SIZE_4KB\n", BaseAddress, Length, EfiAttributes));
> -- 
> 2.7.4
> 
_______________________________________________
edk2-devel mailing list
edk2-devel@lists.01.org
https://lists.01.org/mailman/listinfo/edk2-devel

On 27 February 2017 at 15:32, Leif Lindholm <leif.lindholm@linaro.org> wrote:
> On Mon, Feb 27, 2017 at 02:38:05PM +0000, Ard Biesheuvel wrote:
>> To prevent the initial MMU->GCD memory space map synchronization from
>> stripping permissions attributes [which we cannot use in the GCD memory
>> space map, unfortunately], implement the same approach as x86, and ignore
>> SetMemoryAttributes() calls during the time SyncCacheConfig() is in
>> progress. This is a horrible hack, but is currently the only way we can
>> implement strict permissions on arbitrary memory regions [as opposed to
>> PE/COFF text/data sections only]
>
> Sounds like another excellent argument for why this CpuDxe should be
> cosying up with the UefiCpuPkg one longer-term.
>

I suppose so, yes.

>> Contributed-under: TianoCore Contribution Agreement 1.0
>> Signed-off-by: Ard Biesheuvel <ard.biesheuvel@linaro.org>
>> Reviewed-by: Jiewen Yao <jiewen.yao@intel.com>
>> ---
>>  ArmPkg/Drivers/CpuDxe/CpuDxe.c       | 3 +++
>>  ArmPkg/Drivers/CpuDxe/CpuDxe.h       | 1 +
>>  ArmPkg/Drivers/CpuDxe/CpuMmuCommon.c | 4 ++++
>>  3 files changed, 8 insertions(+)
>>
>> diff --git a/ArmPkg/Drivers/CpuDxe/CpuDxe.c b/ArmPkg/Drivers/CpuDxe/CpuDxe.c
>> index 5aa5b874144a..1955d1dece03 100644
>> --- a/ArmPkg/Drivers/CpuDxe/CpuDxe.c
>> +++ b/ArmPkg/Drivers/CpuDxe/CpuDxe.c
>> @@ -17,6 +17,7 @@
>>
>>  #include <Guid/IdleLoopEvent.h>
>>
>> +BOOLEAN                   gIsFlushingGCD;
>
> OK, I am unable to not bikeshed this:
> The behaviour you're copying is implemented via a variable called
> mIsFlushingGCD. Why change the prefix? Surely we're not looking to
> export this variable any further in ARM?
>

Because it is not local the one compilation unit, that's all. It is
not in a library, so in that sense, it is guaranteed to remain local
to this module, if that is any consolation.

>>
>>  /**
>>    This function flushes the range of addresses from Start to Start+Length
>> @@ -261,7 +262,9 @@ CpuDxeInitialize (
>>    // and that calls EFI_CPU_ARCH_PROTOCOL.SetMemoryAttributes, so this code needs to go
>>    // after the protocol is installed
>>    //
>> +  gIsFlushingGCD = TRUE;
>>    SyncCacheConfig (&mCpu);
>> +  gIsFlushingGCD = FALSE;
>>
>>    // If the platform is a MPCore system then install the Configuration Table describing the
>>    // secondary core states
>> diff --git a/ArmPkg/Drivers/CpuDxe/CpuDxe.h b/ArmPkg/Drivers/CpuDxe/CpuDxe.h
>> index a00fc3064362..085e4cab2921 100644
>> --- a/ArmPkg/Drivers/CpuDxe/CpuDxe.h
>> +++ b/ArmPkg/Drivers/CpuDxe/CpuDxe.h
>> @@ -37,6 +37,7 @@
>>  #include <Protocol/DebugSupportPeriodicCallback.h>
>>  #include <Protocol/LoadedImage.h>
>>
>> +extern BOOLEAN gIsFlushingGCD;
>
> Eew ... this suggest we are.
>
> /
>     Leif
>
>>
>>  /**
>>    This function registers and enables the handler specified by InterruptHandler for a processor
>> diff --git a/ArmPkg/Drivers/CpuDxe/CpuMmuCommon.c b/ArmPkg/Drivers/CpuDxe/CpuMmuCommon.c
>> index ebe593d1c325..6dfec7e55888 100644
>> --- a/ArmPkg/Drivers/CpuDxe/CpuMmuCommon.c
>> +++ b/ArmPkg/Drivers/CpuDxe/CpuMmuCommon.c
>> @@ -188,6 +188,10 @@ CpuSetMemoryAttributes (
>>    UINTN       RegionLength;
>>    UINTN       RegionArmAttributes;
>>
>> +  if (gIsFlushingGCD) {
>> +    return EFI_SUCCESS;
>> +  }
>> +
>>    if ((BaseAddress & (SIZE_4KB - 1)) != 0) {
>>      // Minimum granularity is SIZE_4KB (4KB on ARM)
>>      DEBUG ((EFI_D_PAGE, "CpuSetMemoryAttributes(%lx, %lx, %lx): Minimum ganularity is SIZE_4KB\n", BaseAddress, Length, EfiAttributes));
>> --
>> 2.7.4
>>
_______________________________________________
edk2-devel mailing list
edk2-devel@lists.01.org
https://lists.01.org/mailman/listinfo/edk2-devel

On Mon, Feb 27, 2017 at 03:33:56PM +0000, Ard Biesheuvel wrote:
> On 27 February 2017 at 15:32, Leif Lindholm <leif.lindholm@linaro.org> wrote:
> > On Mon, Feb 27, 2017 at 02:38:05PM +0000, Ard Biesheuvel wrote:
> >> To prevent the initial MMU->GCD memory space map synchronization from
> >> stripping permissions attributes [which we cannot use in the GCD memory
> >> space map, unfortunately], implement the same approach as x86, and ignore
> >> SetMemoryAttributes() calls during the time SyncCacheConfig() is in
> >> progress. This is a horrible hack, but is currently the only way we can
> >> implement strict permissions on arbitrary memory regions [as opposed to
> >> PE/COFF text/data sections only]
> >
> > Sounds like another excellent argument for why this CpuDxe should be
> > cosying up with the UefiCpuPkg one longer-term.
> >
> 
> I suppose so, yes.
> 
> >> Contributed-under: TianoCore Contribution Agreement 1.0
> >> Signed-off-by: Ard Biesheuvel <ard.biesheuvel@linaro.org>
> >> Reviewed-by: Jiewen Yao <jiewen.yao@intel.com>
> >> ---
> >>  ArmPkg/Drivers/CpuDxe/CpuDxe.c       | 3 +++
> >>  ArmPkg/Drivers/CpuDxe/CpuDxe.h       | 1 +
> >>  ArmPkg/Drivers/CpuDxe/CpuMmuCommon.c | 4 ++++
> >>  3 files changed, 8 insertions(+)
> >>
> >> diff --git a/ArmPkg/Drivers/CpuDxe/CpuDxe.c b/ArmPkg/Drivers/CpuDxe/CpuDxe.c
> >> index 5aa5b874144a..1955d1dece03 100644
> >> --- a/ArmPkg/Drivers/CpuDxe/CpuDxe.c
> >> +++ b/ArmPkg/Drivers/CpuDxe/CpuDxe.c
> >> @@ -17,6 +17,7 @@
> >>
> >>  #include <Guid/IdleLoopEvent.h>
> >>
> >> +BOOLEAN                   gIsFlushingGCD;
> >
> > OK, I am unable to not bikeshed this:
> > The behaviour you're copying is implemented via a variable called
> > mIsFlushingGCD. Why change the prefix? Surely we're not looking to
> > export this variable any further in ARM?
> >
> 
> Because it is not local the one compilation unit, that's all. It is
> not in a library, so in that sense, it is guaranteed to remain local
> to this module, if that is any consolation.

Ah, excellent.

So, from how I read the coding standards (section 4.4), 'm' would
still be the appropriate prefix:
"A module variable is intended to only be accessed across a small set of
related routines that have strict rules for accessing the data; in
effect, constrained to the set of files described within a single .inf
file."

Regards,

Leif

> >>
> >>  /**
> >>    This function flushes the range of addresses from Start to Start+Length
> >> @@ -261,7 +262,9 @@ CpuDxeInitialize (
> >>    // and that calls EFI_CPU_ARCH_PROTOCOL.SetMemoryAttributes, so this code needs to go
> >>    // after the protocol is installed
> >>    //
> >> +  gIsFlushingGCD = TRUE;
> >>    SyncCacheConfig (&mCpu);
> >> +  gIsFlushingGCD = FALSE;
> >>
> >>    // If the platform is a MPCore system then install the Configuration Table describing the
> >>    // secondary core states
> >> diff --git a/ArmPkg/Drivers/CpuDxe/CpuDxe.h b/ArmPkg/Drivers/CpuDxe/CpuDxe.h
> >> index a00fc3064362..085e4cab2921 100644
> >> --- a/ArmPkg/Drivers/CpuDxe/CpuDxe.h
> >> +++ b/ArmPkg/Drivers/CpuDxe/CpuDxe.h
> >> @@ -37,6 +37,7 @@
> >>  #include <Protocol/DebugSupportPeriodicCallback.h>
> >>  #include <Protocol/LoadedImage.h>
> >>
> >> +extern BOOLEAN gIsFlushingGCD;
> >
> > Eew ... this suggest we are.
> >
> > /
> >     Leif
> >
> >>
> >>  /**
> >>    This function registers and enables the handler specified by InterruptHandler for a processor
> >> diff --git a/ArmPkg/Drivers/CpuDxe/CpuMmuCommon.c b/ArmPkg/Drivers/CpuDxe/CpuMmuCommon.c
> >> index ebe593d1c325..6dfec7e55888 100644
> >> --- a/ArmPkg/Drivers/CpuDxe/CpuMmuCommon.c
> >> +++ b/ArmPkg/Drivers/CpuDxe/CpuMmuCommon.c
> >> @@ -188,6 +188,10 @@ CpuSetMemoryAttributes (
> >>    UINTN       RegionLength;
> >>    UINTN       RegionArmAttributes;
> >>
> >> +  if (gIsFlushingGCD) {
> >> +    return EFI_SUCCESS;
> >> +  }
> >> +
> >>    if ((BaseAddress & (SIZE_4KB - 1)) != 0) {
> >>      // Minimum granularity is SIZE_4KB (4KB on ARM)
> >>      DEBUG ((EFI_D_PAGE, "CpuSetMemoryAttributes(%lx, %lx, %lx): Minimum ganularity is SIZE_4KB\n", BaseAddress, Length, EfiAttributes));
> >> --
> >> 2.7.4
> >>
_______________________________________________
edk2-devel mailing list
edk2-devel@lists.01.org
https://lists.01.org/mailman/listinfo/edk2-devel

On 27 February 2017 at 15:38, Leif Lindholm <leif.lindholm@linaro.org> wrote:
> On Mon, Feb 27, 2017 at 03:33:56PM +0000, Ard Biesheuvel wrote:
>> On 27 February 2017 at 15:32, Leif Lindholm <leif.lindholm@linaro.org> wrote:
>> > On Mon, Feb 27, 2017 at 02:38:05PM +0000, Ard Biesheuvel wrote:
>> >> To prevent the initial MMU->GCD memory space map synchronization from
>> >> stripping permissions attributes [which we cannot use in the GCD memory
>> >> space map, unfortunately], implement the same approach as x86, and ignore
>> >> SetMemoryAttributes() calls during the time SyncCacheConfig() is in
>> >> progress. This is a horrible hack, but is currently the only way we can
>> >> implement strict permissions on arbitrary memory regions [as opposed to
>> >> PE/COFF text/data sections only]
>> >
>> > Sounds like another excellent argument for why this CpuDxe should be
>> > cosying up with the UefiCpuPkg one longer-term.
>> >
>>
>> I suppose so, yes.
>>
>> >> Contributed-under: TianoCore Contribution Agreement 1.0
>> >> Signed-off-by: Ard Biesheuvel <ard.biesheuvel@linaro.org>
>> >> Reviewed-by: Jiewen Yao <jiewen.yao@intel.com>
>> >> ---
>> >>  ArmPkg/Drivers/CpuDxe/CpuDxe.c       | 3 +++
>> >>  ArmPkg/Drivers/CpuDxe/CpuDxe.h       | 1 +
>> >>  ArmPkg/Drivers/CpuDxe/CpuMmuCommon.c | 4 ++++
>> >>  3 files changed, 8 insertions(+)
>> >>
>> >> diff --git a/ArmPkg/Drivers/CpuDxe/CpuDxe.c b/ArmPkg/Drivers/CpuDxe/CpuDxe.c
>> >> index 5aa5b874144a..1955d1dece03 100644
>> >> --- a/ArmPkg/Drivers/CpuDxe/CpuDxe.c
>> >> +++ b/ArmPkg/Drivers/CpuDxe/CpuDxe.c
>> >> @@ -17,6 +17,7 @@
>> >>
>> >>  #include <Guid/IdleLoopEvent.h>
>> >>
>> >> +BOOLEAN                   gIsFlushingGCD;
>> >
>> > OK, I am unable to not bikeshed this:
>> > The behaviour you're copying is implemented via a variable called
>> > mIsFlushingGCD. Why change the prefix? Surely we're not looking to
>> > export this variable any further in ARM?
>> >
>>
>> Because it is not local the one compilation unit, that's all. It is
>> not in a library, so in that sense, it is guaranteed to remain local
>> to this module, if that is any consolation.
>
> Ah, excellent.
>
> So, from how I read the coding standards (section 4.4), 'm' would
> still be the appropriate prefix:
> "A module variable is intended to only be accessed across a small set of
> related routines that have strict rules for accessing the data; in
> effect, constrained to the set of files described within a single .inf
> file."
>

Ah, nice, I wasn't aware of that. I will use the m prefix instead, then.

Thanks,
Ard.


>> >>
>> >>  /**
>> >>    This function flushes the range of addresses from Start to Start+Length
>> >> @@ -261,7 +262,9 @@ CpuDxeInitialize (
>> >>    // and that calls EFI_CPU_ARCH_PROTOCOL.SetMemoryAttributes, so this code needs to go
>> >>    // after the protocol is installed
>> >>    //
>> >> +  gIsFlushingGCD = TRUE;
>> >>    SyncCacheConfig (&mCpu);
>> >> +  gIsFlushingGCD = FALSE;
>> >>
>> >>    // If the platform is a MPCore system then install the Configuration Table describing the
>> >>    // secondary core states
>> >> diff --git a/ArmPkg/Drivers/CpuDxe/CpuDxe.h b/ArmPkg/Drivers/CpuDxe/CpuDxe.h
>> >> index a00fc3064362..085e4cab2921 100644
>> >> --- a/ArmPkg/Drivers/CpuDxe/CpuDxe.h
>> >> +++ b/ArmPkg/Drivers/CpuDxe/CpuDxe.h
>> >> @@ -37,6 +37,7 @@
>> >>  #include <Protocol/DebugSupportPeriodicCallback.h>
>> >>  #include <Protocol/LoadedImage.h>
>> >>
>> >> +extern BOOLEAN gIsFlushingGCD;
>> >
>> > Eew ... this suggest we are.
>> >
>> > /
>> >     Leif
>> >
>> >>
>> >>  /**
>> >>    This function registers and enables the handler specified by InterruptHandler for a processor
>> >> diff --git a/ArmPkg/Drivers/CpuDxe/CpuMmuCommon.c b/ArmPkg/Drivers/CpuDxe/CpuMmuCommon.c
>> >> index ebe593d1c325..6dfec7e55888 100644
>> >> --- a/ArmPkg/Drivers/CpuDxe/CpuMmuCommon.c
>> >> +++ b/ArmPkg/Drivers/CpuDxe/CpuMmuCommon.c
>> >> @@ -188,6 +188,10 @@ CpuSetMemoryAttributes (
>> >>    UINTN       RegionLength;
>> >>    UINTN       RegionArmAttributes;
>> >>
>> >> +  if (gIsFlushingGCD) {
>> >> +    return EFI_SUCCESS;
>> >> +  }
>> >> +
>> >>    if ((BaseAddress & (SIZE_4KB - 1)) != 0) {
>> >>      // Minimum granularity is SIZE_4KB (4KB on ARM)
>> >>      DEBUG ((EFI_D_PAGE, "CpuSetMemoryAttributes(%lx, %lx, %lx): Minimum ganularity is SIZE_4KB\n", BaseAddress, Length, EfiAttributes));
>> >> --
>> >> 2.7.4
>> >>
_______________________________________________
edk2-devel mailing list
edk2-devel@lists.01.org
https://lists.01.org/mailman/listinfo/edk2-devel

On Mon, Feb 27, 2017 at 03:39:54PM +0000, Ard Biesheuvel wrote:
> On 27 February 2017 at 15:38, Leif Lindholm <leif.lindholm@linaro.org> wrote:
> > On Mon, Feb 27, 2017 at 03:33:56PM +0000, Ard Biesheuvel wrote:
> >> On 27 February 2017 at 15:32, Leif Lindholm <leif.lindholm@linaro.org> wrote:
> >> > On Mon, Feb 27, 2017 at 02:38:05PM +0000, Ard Biesheuvel wrote:
> >> >> To prevent the initial MMU->GCD memory space map synchronization from
> >> >> stripping permissions attributes [which we cannot use in the GCD memory
> >> >> space map, unfortunately], implement the same approach as x86, and ignore
> >> >> SetMemoryAttributes() calls during the time SyncCacheConfig() is in
> >> >> progress. This is a horrible hack, but is currently the only way we can
> >> >> implement strict permissions on arbitrary memory regions [as opposed to
> >> >> PE/COFF text/data sections only]
> >> >
> >> > Sounds like another excellent argument for why this CpuDxe should be
> >> > cosying up with the UefiCpuPkg one longer-term.
> >> >
> >>
> >> I suppose so, yes.
> >>
> >> >> Contributed-under: TianoCore Contribution Agreement 1.0
> >> >> Signed-off-by: Ard Biesheuvel <ard.biesheuvel@linaro.org>
> >> >> Reviewed-by: Jiewen Yao <jiewen.yao@intel.com>
> >> >> ---
> >> >>  ArmPkg/Drivers/CpuDxe/CpuDxe.c       | 3 +++
> >> >>  ArmPkg/Drivers/CpuDxe/CpuDxe.h       | 1 +
> >> >>  ArmPkg/Drivers/CpuDxe/CpuMmuCommon.c | 4 ++++
> >> >>  3 files changed, 8 insertions(+)
> >> >>
> >> >> diff --git a/ArmPkg/Drivers/CpuDxe/CpuDxe.c b/ArmPkg/Drivers/CpuDxe/CpuDxe.c
> >> >> index 5aa5b874144a..1955d1dece03 100644
> >> >> --- a/ArmPkg/Drivers/CpuDxe/CpuDxe.c
> >> >> +++ b/ArmPkg/Drivers/CpuDxe/CpuDxe.c
> >> >> @@ -17,6 +17,7 @@
> >> >>
> >> >>  #include <Guid/IdleLoopEvent.h>
> >> >>
> >> >> +BOOLEAN                   gIsFlushingGCD;
> >> >
> >> > OK, I am unable to not bikeshed this:
> >> > The behaviour you're copying is implemented via a variable called
> >> > mIsFlushingGCD. Why change the prefix? Surely we're not looking to
> >> > export this variable any further in ARM?
> >> >
> >>
> >> Because it is not local the one compilation unit, that's all. It is
> >> not in a library, so in that sense, it is guaranteed to remain local
> >> to this module, if that is any consolation.
> >
> > Ah, excellent.
> >
> > So, from how I read the coding standards (section 4.4), 'm' would
> > still be the appropriate prefix:
> > "A module variable is intended to only be accessed across a small set of
> > related routines that have strict rules for accessing the data; in
> > effect, constrained to the set of files described within a single .inf
> > file."
> >
> 
> Ah, nice, I wasn't aware of that. I will use the m prefix instead, then.

In that case,
Reviewed-by: Leif Lindholm <leif.lindholm@linaro.org>

> Thanks,
> Ard.
> 
> 
> >> >>
> >> >>  /**
> >> >>    This function flushes the range of addresses from Start to Start+Length
> >> >> @@ -261,7 +262,9 @@ CpuDxeInitialize (
> >> >>    // and that calls EFI_CPU_ARCH_PROTOCOL.SetMemoryAttributes, so this code needs to go
> >> >>    // after the protocol is installed
> >> >>    //
> >> >> +  gIsFlushingGCD = TRUE;
> >> >>    SyncCacheConfig (&mCpu);
> >> >> +  gIsFlushingGCD = FALSE;
> >> >>
> >> >>    // If the platform is a MPCore system then install the Configuration Table describing the
> >> >>    // secondary core states
> >> >> diff --git a/ArmPkg/Drivers/CpuDxe/CpuDxe.h b/ArmPkg/Drivers/CpuDxe/CpuDxe.h
> >> >> index a00fc3064362..085e4cab2921 100644
> >> >> --- a/ArmPkg/Drivers/CpuDxe/CpuDxe.h
> >> >> +++ b/ArmPkg/Drivers/CpuDxe/CpuDxe.h
> >> >> @@ -37,6 +37,7 @@
> >> >>  #include <Protocol/DebugSupportPeriodicCallback.h>
> >> >>  #include <Protocol/LoadedImage.h>
> >> >>
> >> >> +extern BOOLEAN gIsFlushingGCD;
> >> >
> >> > Eew ... this suggest we are.
> >> >
> >> > /
> >> >     Leif
> >> >
> >> >>
> >> >>  /**
> >> >>    This function registers and enables the handler specified by InterruptHandler for a processor
> >> >> diff --git a/ArmPkg/Drivers/CpuDxe/CpuMmuCommon.c b/ArmPkg/Drivers/CpuDxe/CpuMmuCommon.c
> >> >> index ebe593d1c325..6dfec7e55888 100644
> >> >> --- a/ArmPkg/Drivers/CpuDxe/CpuMmuCommon.c
> >> >> +++ b/ArmPkg/Drivers/CpuDxe/CpuMmuCommon.c
> >> >> @@ -188,6 +188,10 @@ CpuSetMemoryAttributes (
> >> >>    UINTN       RegionLength;
> >> >>    UINTN       RegionArmAttributes;
> >> >>
> >> >> +  if (gIsFlushingGCD) {
> >> >> +    return EFI_SUCCESS;
> >> >> +  }
> >> >> +
> >> >>    if ((BaseAddress & (SIZE_4KB - 1)) != 0) {
> >> >>      // Minimum granularity is SIZE_4KB (4KB on ARM)
> >> >>      DEBUG ((EFI_D_PAGE, "CpuSetMemoryAttributes(%lx, %lx, %lx): Minimum ganularity is SIZE_4KB\n", BaseAddress, Length, EfiAttributes));
> >> >> --
> >> >> 2.7.4
> >> >>
_______________________________________________
edk2-devel mailing list
edk2-devel@lists.01.org
https://lists.01.org/mailman/listinfo/edk2-devel