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.55558.1674508602574777573 for ; Mon, 23 Jan 2023 13:16:42 -0800 Authentication-Results: mx.groups.io; dkim=fail reason="body hash did not verify" header.i=@linux.microsoft.com header.s=default header.b=dmzq5i2h; spf=pass (domain: linux.microsoft.com, ip: 13.77.154.182, mailfrom: eiakovlev@linux.microsoft.com) Received: from [192.168.0.20] (unknown [77.64.253.114]) by linux.microsoft.com (Postfix) with ESMTPSA id 3825920E2D0C; Mon, 23 Jan 2023 13:16:41 -0800 (PST) DKIM-Filter: OpenDKIM Filter v2.11.0 linux.microsoft.com 3825920E2D0C DKIM-Signature: v=1; a=rsa-sha256; c=relaxed/relaxed; d=linux.microsoft.com; s=default; t=1674508602; bh=4Mb/LzaCW4lMTIS4dilNL1aLdGSyig7S39ztdGLrPx4=; h=Date:Subject:To:References:From:In-Reply-To:From; b=dmzq5i2hrb1oNgE0a0VKeorF7rE8rKOQQzZ/6BrhtW0VXp+CQrx7MZJ57GjGZH/rV 5P2AZRbqF1mifh3tWcy9R+WB5VyMdgf5MwQRvHrg+qjQlbOcrya6oNh/Z0NcPWNNgC wyAOlrccV1EtPABA51CxBEh5aFOL0s3YcLc7I/rY= Message-ID: <896a0b73-8df6-e5e3-6bf1-1f0cbd90e5fd@linux.microsoft.com> Date: Mon, 23 Jan 2023 22:16:39 +0100 MIME-Version: 1.0 User-Agent: Mozilla/5.0 (Windows NT 10.0; Win64; x64; rv:102.0) Gecko/20100101 Thunderbird/102.6.1 Subject: Re: [edk2-devel] [PATCH] ArmVirtPkg/PrePei: when starting in EL2 configure HCR to not trap on PAC To: Sami Mujawar , "devel@edk2.groups.io" , "ardb+tianocore@kernel.org" , "ardb@kernel.org" References: <20230123133317.22491-1-eiakovlev@linux.microsoft.com> From: "Evgeny Iakovlev" In-Reply-To: Content-Language: en-US Content-Type: text/plain; charset=UTF-8; format=flowed Content-Transfer-Encoding: quoted-printable On 1/23/2023 16:58, Sami Mujawar wrote: > Hi Evgeny, > > Thank you for this patch. > > Please find my response inline marked [SAMI]. Hey Sami, thanks for taking a look. Responses inlined. > > Regards, > > Sami Mujawar > > > =EF=BB=BFOn 23/01/2023, 14:43, "devel@edk2.groups.io on behalf of Evgen= y Iakovlev via groups.io" wrote: > > When FEAT_PAuth is impelemented HCR_EL2.APK and HCR_EL2.API bits c= ontrol > whether PAC-related instructions and register accesses should be t= rapped > by the EL2 hypervisor. Note that bit value 0b1 means do NOT trap. = When > FEAT_PAuth is not implemented or if EL2 is disabled, those bits ar= e > ignored and system behaves as if their value was 0b1. > > When starting in EL2 on ArmVirtPkg get our of the way of a potenti= al > hypervisor by setting APK and API bits. > [SAMI] The modules in ArmVirPkg are expected to be used by guest firmwa= re. Therefore, I do not > expect the code to be executing at EL2. Am, I missing something here? C= an you explain, please? > [SAMI] The setup i am using emulates a qemu aarch64 machine with EL3/TrustZone=20 support. Boot starts in EL3 and executes a different firmware (ARM trusted=20 firmware to be specific), which initializes secure mode. Then it passes control over to UEFI FV in the next least privileged EL,=20 which is EL2, so that EDK2 can run a boot chain that ends up with a=20 guest hypervisor. > > Contributed-under: TianoCore Contribution Agreement 1.1 > [SAMI] I believe the above line is no longer required. See, https://git= hub.com/tianocore/edk2#code-contributions Right, i just saw it in recent git history, but good to know. > > Signed-off-by: Evgeny Iakovlev > --- > ArmPkg/Include/Chipset/AArch64.h | 2 ++ > ArmPlatformPkg/PrePeiCore/AArch64/Helper.S | 5 +++++ > ArmVirtPkg/PrePi/AArch64/ArchPrePi.c | 3 ++- > 3 files changed, 9 insertions(+), 1 deletion(-) > > diff --git a/ArmPkg/Include/Chipset/AArch64.h b/ArmPkg/Include/Chi= pset/AArch64.h > index bfd2859f51..da8737236d 100644 > --- a/ArmPkg/Include/Chipset/AArch64.h > +++ b/ArmPkg/Include/Chipset/AArch64.h > @@ -57,6 +57,8 @@ > #define ARM_HCR_AMO BIT5 > > #define ARM_HCR_TSC BIT19 > > #define ARM_HCR_TGE BIT27 > > +#define ARM_HCR_APK BIT40 > > +#define ARM_HCR_API BIT41 > > > > // Exception Syndrome Register > > #define AARCH64_ESR_EC(Ecr) ((0x3F << 26) & (Ecr)) > > diff --git a/ArmPlatformPkg/PrePeiCore/AArch64/Helper.S b/ArmPlatf= ormPkg/PrePeiCore/AArch64/Helper.S > index 2a604b719b..0f413c5655 100644 > --- a/ArmPlatformPkg/PrePeiCore/AArch64/Helper.S > +++ b/ArmPlatformPkg/PrePeiCore/AArch64/Helper.S > @@ -26,6 +26,11 @@ ASM_FUNC(SetupExceptionLevel2) > orr x0, x0, #(1 << 3) // Enable EL2 FIQ > > orr x0, x0, #(1 << 4) // Enable EL2 IRQ > > orr x0, x0, #(1 << 5) // Enable EL2 SError and Abort > > + > > + // Get out of the way of a poitential EL2 hypervisor by NOT tr= apping PAC registers and instructions > > + orr x0, x0, #(1 << 40) // HCR_EL2.APK > > + orr x0, x0, #(1 << 41) // HCR_EL2.API > > + > [SAMI] I see these registers are initialised to an architecturally UNKN= OWN value on reset. So, it makes sense to > configure these. However, I think we should first check if FEAT_PAuth= is supported before setting these bits. > [/SAMI] I don't think there's any need to check if FEAT_PAuth is supported. If=20 it doesn't, then those bits are ignored and system always behaves as if=20 they were 0b1. So, there's no harm in setting them either way, i think. Do you see why that could be bad? > > msr hcr_el2, x0 // Write back our settings > > > > msr cptr_el2, xzr // Disable copro traps to EL2 > > diff --git a/ArmVirtPkg/PrePi/AArch64/ArchPrePi.c b/ArmVirtPkg/Pre= Pi/AArch64/ArchPrePi.c > index 9cab88ca08..29da9d4050 100644 > --- a/ArmVirtPkg/PrePi/AArch64/ArchPrePi.c > +++ b/ArmVirtPkg/PrePi/AArch64/ArchPrePi.c > @@ -22,6 +22,7 @@ ArchInitialize ( > > > if (ArmReadCurrentEL () =3D=3D AARCH64_EL2) { > > // Trap General Exceptions. All exceptions that would be rout= ed to EL1 are routed to EL2 > > - ArmWriteHcr (ARM_HCR_TGE); > > + // Also get out of the way of a potential EL2 hypervisor and = do NOT trap PAC registers or instructions. > > + ArmWriteHcr (ARM_HCR_TGE | ARM_HCR_APK | ARM_HCR_API); > > } > [SAMI] I think the above code section should not be there in the first = place as ArmVirtPkg/PrePi should > not be running at EL2. > [/SAMI] Code here certainly does expect to run in EL2, because there's an=20 explicit check for that (if (ArmReadCurrentEL () =3D=3D AARCH64_EL2)). I've described the setup that ends up with EDK2 in EL2 above. > > } > > -- > 2.34.1 > > > > -=3D-=3D-=3D-=3D-=3D-=3D > Groups.io Links: You receive all messages sent to this group. > View/Reply Online (#98963): https://edk2.groups.io/g/devel/message= /98963 > Mute This Topic: https://groups.io/mt/96474724/1779659 > Group Owner: devel+owner@edk2.groups.io > Unsubscribe: https://edk2.groups.io/g/devel/unsub [sami.mujawar@ar= m.com] > -=3D-=3D-=3D-=3D-=3D-=3D > > > > IMPORTANT NOTICE: The contents of this email and any attachments are co= nfidential and may also be privileged. If you are not the intended recipi= ent, please notify the sender immediately and do not disclose the content= s to any other person, use it for any purpose, or store or copy the infor= mation in any medium. Thank you.