Re: [PATCH edk2-platforms v2 15/18] ARM/VExpressPkg: New DP500/DP550/DP650 platform library.

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

On 24 January 2018 at 11:27, Alexei Fedorov <Alexei.Fedorov@arm.com> wrote:
> Hi Ard,
>
>
> You wrote:
>
>
>> > +  Status = gDS->SetMemorySpaceAttributes (
>> > +                  *VramBaseAddress,
>> > +                  *VramSize,
>> > +                  EFI_MEMORY_WC
>>
>> Please add EFI_MEMORY_XP here
>>
>
>
> Setting EFI_MEMORY_XP causes assertion because
> gDS->SetMemorySpaceAttributes() returns Unsupported in
> edk2\MdeModulePkg\Core\Dxe\Gcd\Gcd.c  with Status set at line #834:
>
>
>       if ((Entry->Capabilities & Attributes) != Attributes) {
>
>         //***AF
>         DEBUG ((DEBUG_GCD, " @%d EFI_UNSUPPORTED: Capabilitie=0x%lX
> Attributes=0x%lX\n",
>                 __LINE__, Entry->Capabilities, Attributes));
>
>         Status = EFI_UNSUPPORTED;
>         goto Done;
>       }
>
> because Entry->Capabilities don't have EFI_MEMORY_XP set, see debug output
> below:
>

<snip>

> If  gDS->SetMemorySpaceAttributes() call is replaced with
> Cpu->SetMemoryAttributes(), attributes are set successfully with eXecute bit
> for frame buffer being cleared.
>

Ugh. The GCD map conflates capabilities of the region (i.e., this
region can be configured as read-only by the MMU) with permissions
(i.e., the contents of this region should not be modified by the
program), which is why nobody bothers to set these attributes for the
GCD region. If you do set those bits in the GCD map, all allocations
carved out of it will be read-only and non-executable (because, hey,
why would you allocate writable or executable memory from a region
that can be configured as read-only or non-executable?).

In any case, your solution is the correct one: use the arch CPU
protocol directly instead (although there is something  in the PI spec
that bans it). Having an executable frame buffer is a much bigger deal
IMHO, especially when all other allocations are being locked down, and
so my preference is to switch to the arch CPU protocol here.

Thanks,
Ard.