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 <kra...@redhat.com
<mailto:kra...@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
-=-=-=-=-=-=-=-=-=-=-=-
Groups.io Links: You receive all messages sent to this group.
View/Reply Online (#106219): https://edk2.groups.io/g/devel/message/106219
Mute This Topic: https://groups.io/mt/99631663/21656
Group Owner: devel+ow...@edk2.groups.io
Unsubscribe: https://edk2.groups.io/g/devel/unsub [arch...@mail-archive.com]
-=-=-=-=-=-=-=-=-=-=-=-
