From mboxrd@z Thu Jan 1 00:00:00 1970 Received: from linux.microsoft.com (linux.microsoft.com [13.77.154.182]) by mx.groups.io with SMTP id smtpd.web10.439.1687272816170789897 for ; Tue, 20 Jun 2023 07:53:36 -0700 Authentication-Results: mx.groups.io; dkim=fail reason="body hash did not verify" header.i=@linux.microsoft.com header.s=default header.b=rXwX5GHJ; spf=pass (domain: linux.microsoft.com, ip: 13.77.154.182, mailfrom: mikuback@linux.microsoft.com) Received: from [192.168.4.22] (unknown [47.201.8.94]) by linux.microsoft.com (Postfix) with ESMTPSA id D42E421C1E69; Tue, 20 Jun 2023 07:53:33 -0700 (PDT) DKIM-Filter: OpenDKIM Filter v2.11.0 linux.microsoft.com D42E421C1E69 DKIM-Signature: v=1; a=rsa-sha256; c=relaxed/relaxed; d=linux.microsoft.com; s=default; t=1687272815; bh=Ptuf9oqkjb9AySNNAXhWchUSkVeEaLULlnRdE22rT9I=; h=Date:Subject:To:Cc:References:From:In-Reply-To:From; b=rXwX5GHJ7HnnfpGK3n2Sr/8NQ59BhXkzTtG/BEIH1uHgZLi4s6oeifPm391dm+1ir Jo3vAvetair8hcGq9QMwos+DkeUEd0Ib4/rCEMGBwzoP/Tw7HOtX519r+4dqAhHVD6 tO+9pS4hVDPIshRikUImGAiaCp/f7Lr5uJDlC5N4= Message-ID: <02ea7825-8b4e-2d0c-4658-aab179c2fd5b@linux.microsoft.com> Date: Tue, 20 Jun 2023 10:53:32 -0400 MIME-Version: 1.0 User-Agent: Mozilla/5.0 (Windows NT 10.0; Win64; x64; rv:102.0) Gecko/20100101 Thunderbird/102.12.0 Subject: Re: [edk2-devel] [PATCH 1/1] ArmPkg: Add Pcd to disable EFI_MEMORY_ATTRIBUTE_PROTOCOL To: devel@edk2.groups.io, ardb@kernel.org, Gerd Hoffmann Cc: Oliver Steffen , Ard Biesheuvel , Daniel Schaefer , Eric Dong , Leif Lindholm , Liming Gao , Michael D Kinney , Rahul Kumar , Ray Ni , Sami Mujawar , Sunil V L , Zhiguang Liu , Taylor Beebe , Oliver Smith-Denny References: <20230619203244.228933-1-osteffen@redhat.com> From: "Michael Kubacki" In-Reply-To: Content-Language: en-US Content-Type: text/plain; charset=UTF-8; format=flowed Content-Transfer-Encoding: quoted-printable 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-1= 593550704 On 6/20/2023 9:16 AM, Ard Biesheuvel wrote: >=20 >=20 > On Tue, Jun 20, 2023, 12:33 Gerd Hoffmann > wrote: >=20 > 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.=C2=A0 T= o allow > > existing installations to boot, provide a workaround in form of a = Pcd > > that allows tuning it off at build time (defaults to 'enabled'). >=20 > Background:=C2=A0 We have untested + broken code for > EFI_MEMORY_ATTRIBUTE_PROTOCOL support in the listed shim releases. >=20 > Now that firmware starts to actually provide that protocol the > time bomb explodes. >=20 >=20 >=20 > Fantastic. >=20 > This is kind of a big deal, really, and just turning it off for=20 > ArmVirtQemu does not help at all with the fact that these shim builds=20 > will crash on any platform that implements the protocol. (Including x86) >=20 > Given that secure boot is kind of pointless on this particular platform= =20 > anyway, maybe this is a good opportunity to make shim optional in the=20 > boot chain? I understand that this does not fix existing builds but shim= =20 > proves to be such a problematic component that you really should not be= =20 > using it if there is no need. >=20 > As for the protocol, this has its own set of problems, and the bug in=20 > question can partly be blamed on the misdesigned api, which has separate= =20 > set and clear methods. Not only does this force the implementation to=20 > traverse the page tables twice for the common case of switching between= =20 > RO and XP or vice versa, it also means we lose any transactional=20 > properties of a RO <-> XP switch. I.e., if we could make it the=20 > implementation's responsibility to ensure that such a transformation=20 > either completes successfully, or otherwise, doesn't make any=20 > modifications at all, the risk of ending up in a limbo state is reduced= =20 > significantly. >=20 > So maybe there is still opportunity for specifying a MemoryAttributes2=20 > protocol with a single method for set and clear? We could just drop the= =20 > current one in that case. >=20 > In any case, while i can see how this patch helps make all your ci=20 > status icons turn green again, it does so by papering over the=20 > underlying issue so I'm not a fan. >=20 >=20 >=20 >=20 >=20 > > --- a/ArmPkg/ArmPkg.dec > > +++ b/ArmPkg/ArmPkg.dec > > @@ -172,6 +172,9 @@ [PcdsFixedAtBuild.common] > > =20 > gArmTokenSpaceGuid.PcdCpuVectorBaseAddress|0xffff0000|UINT64|0x000000= 04 > >=C2=A0 =C2=A0 gArmTokenSpaceGuid.PcdCpuResetAddress|0x00000000|UINT= 32|0x00000005 > > > > +=C2=A0 # Enable/Disable EFI_MEMORY_ATTRIBUTE_PROTOCOL > > +=20 > gArmTokenSpaceGuid.PcdEnableEfiMemoryAttributeProtocol|TRUE|BOOLEAN|0= x000000EE > > + > >=C2=A0 =C2=A0 # > >=C2=A0 =C2=A0 # ARM Secure Firmware PCDs > >=C2=A0 =C2=A0 # >=20 > 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). >=20 > take care, > =C2=A0 Gerd >=20 >=20