Re: [edk2-devel] [PATCH 1/1] ArmPkg: Add Pcd to disable EFI_MEMORY_ATTRIBUTE_PROTOCOL

["Michael Kubacki" <mikuback@linux.microsoft.com>]

We recently encountered this issue as well.

For reference, some details are in this issue:
https://github.com/microsoft/mu_silicon_arm_tiano/issues/124

Specifically, this comment:
https://github.com/microsoft/mu_silicon_arm_tiano/issues/124#issuecomment-1593550704

On 6/20/2023 9:16 AM, Ard Biesheuvel wrote:
> 
> 
> On Tue, Jun 20, 2023, 12:33 Gerd Hoffmann <kraxel@redhat.com 
> <mailto:kraxel@redhat.com>> wrote:
> 
>     On Mon, Jun 19, 2023 at 10:32:25PM +0200, Oliver Steffen wrote:
>      > Recent versions of shim (15.6 and 15.7) crash when the newly added
>      > EFI_MEMORY_ATTRIBUTE_PROTOCOL is provided by the firmware.  To allow
>      > existing installations to boot, provide a workaround in form of a Pcd
>      > that allows tuning it off at build time (defaults to 'enabled').
> 
>     Background:  We have untested + broken code for
>     EFI_MEMORY_ATTRIBUTE_PROTOCOL support in the listed shim releases.
> 
>     Now that firmware starts to actually provide that protocol the
>     time bomb explodes.
> 
> 
> 
> Fantastic.
> 
> This is kind of a big deal, really, and just turning it off for 
> ArmVirtQemu does not help at all with the fact that these shim builds 
> will crash on any platform that implements the protocol. (Including x86)
> 
> Given that secure boot is kind of pointless on this particular platform 
> anyway, maybe this is a good opportunity to make shim optional in the 
> boot chain? I understand that this does not fix existing builds but shim 
> proves to be such a problematic component that you really should not be 
> using it if there is no need.
> 
> As for the protocol, this has its own set of problems, and the bug in 
> question can partly be blamed on the misdesigned api, which has separate 
> set and clear methods. Not only does this force the implementation to 
> traverse the page tables twice for the common case of switching between 
> RO and XP or vice versa, it also means we lose any transactional 
> properties of a RO <-> XP switch. I.e., if we could make it the 
> implementation's responsibility to ensure that such a transformation 
> either completes successfully, or otherwise, doesn't make any 
> modifications at all, the risk of ending up in a limbo state is reduced 
> significantly.
> 
> So maybe there is still opportunity for specifying a MemoryAttributes2 
> protocol with a single method for set and clear? We could just drop the 
> current one in that case.
> 
> In any case, while i can see how this patch helps make all your ci 
> status icons turn green again, it does so by papering over the 
> underlying issue so I'm not a fan.
> 
> 
> 
> 
> 
>      > --- a/ArmPkg/ArmPkg.dec
>      > +++ b/ArmPkg/ArmPkg.dec
>      > @@ -172,6 +172,9 @@ [PcdsFixedAtBuild.common]
>      >   
>     gArmTokenSpaceGuid.PcdCpuVectorBaseAddress|0xffff0000|UINT64|0x00000004
>      >    gArmTokenSpaceGuid.PcdCpuResetAddress|0x00000000|UINT32|0x00000005
>      >
>      > +  # Enable/Disable EFI_MEMORY_ATTRIBUTE_PROTOCOL
>      > + 
>     gArmTokenSpaceGuid.PcdEnableEfiMemoryAttributeProtocol|TRUE|BOOLEAN|0x000000EE
>      > +
>      >    #
>      >    # ARM Secure Firmware PCDs
>      >    #
> 
>     Given that I expect we will run into the very same problem on x64 as
>     soon as EFI_MEMORY_ATTRIBUTE_PROTOCOL gets enabled there we should
>     probably define the PCD in MdePkg not ArmPkg (which implies splitting
>     the patch into a mini series with one MdePkg and one ArmPkg patch).
> 
>     take care,
>        Gerd
> 
>