Re: [PATCH v4 1/7] ArmPkg/CpuDxe: ignore attribute changes during SyncCacheConfig()

[Ard Biesheuvel <ard.biesheuvel@linaro.org>]

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