* [PATCH] ArmVirtPkg/PrePei: when starting in EL2 configure HCR to not trap on PAC @ 2023-01-23 13:33 Evgeny Iakovlev 2023-01-23 15:58 ` [edk2-devel] " Sami Mujawar 0 siblings, 1 reply; 3+ messages in thread From: Evgeny Iakovlev @ 2023-01-23 13:33 UTC (permalink / raw) To: devel When FEAT_PAuth is impelemented HCR_EL2.APK and HCR_EL2.API bits control whether PAC-related instructions and register accesses should be trapped 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 are ignored and system behaves as if their value was 0b1. When starting in EL2 on ArmVirtPkg get our of the way of a potential hypervisor by setting APK and API bits. Contributed-under: TianoCore Contribution Agreement 1.1 Signed-off-by: Evgeny Iakovlev <eiakovlev@linux.microsoft.com> --- 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/Chipset/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/ArmPlatformPkg/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 trapping PAC registers and instructions + orr x0, x0, #(1 << 40) // HCR_EL2.APK + orr x0, x0, #(1 << 41) // HCR_EL2.API + 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/PrePi/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 () == AARCH64_EL2) { // Trap General Exceptions. All exceptions that would be routed 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); } } -- 2.34.1 ^ permalink raw reply related [flat|nested] 3+ messages in thread
* Re: [edk2-devel] [PATCH] ArmVirtPkg/PrePei: when starting in EL2 configure HCR to not trap on PAC 2023-01-23 13:33 [PATCH] ArmVirtPkg/PrePei: when starting in EL2 configure HCR to not trap on PAC Evgeny Iakovlev @ 2023-01-23 15:58 ` Sami Mujawar 2023-01-23 21:16 ` Evgeny Iakovlev 0 siblings, 1 reply; 3+ messages in thread From: Sami Mujawar @ 2023-01-23 15:58 UTC (permalink / raw) To: devel@edk2.groups.io, eiakovlev@linux.microsoft.com, ardb+tianocore@kernel.org, ardb@kernel.org Hi Evgeny, Thank you for this patch. Please find my response inline marked [SAMI]. Regards, Sami Mujawar On 23/01/2023, 14:43, "devel@edk2.groups.io on behalf of Evgeny Iakovlev via groups.io" <devel@edk2.groups.io on behalf of eiakovlev=linux.microsoft.com@groups.io> wrote: When FEAT_PAuth is impelemented HCR_EL2.APK and HCR_EL2.API bits control whether PAC-related instructions and register accesses should be trapped 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 are ignored and system behaves as if their value was 0b1. When starting in EL2 on ArmVirtPkg get our of the way of a potential hypervisor by setting APK and API bits. [SAMI] The modules in ArmVirPkg are expected to be used by guest firmware. Therefore, I do not expect the code to be executing at EL2. Am, I missing something here? Can you explain, please? [SAMI] Contributed-under: TianoCore Contribution Agreement 1.1 [SAMI] I believe the above line is no longer required. See, https://github.com/tianocore/edk2#code-contributions Signed-off-by: Evgeny Iakovlev <eiakovlev@linux.microsoft.com> --- 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/Chipset/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/ArmPlatformPkg/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 trapping 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 UNKNOWN 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] 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/PrePi/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 () == AARCH64_EL2) { // Trap General Exceptions. All exceptions that would be routed 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] } -- 2.34.1 -=-=-=-=-=-= 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@arm.com] -=-=-=-=-=-= IMPORTANT NOTICE: The contents of this email and any attachments are confidential and may also be privileged. If you are not the intended recipient, please notify the sender immediately and do not disclose the contents to any other person, use it for any purpose, or store or copy the information in any medium. Thank you. ^ permalink raw reply [flat|nested] 3+ messages in thread
* Re: [edk2-devel] [PATCH] ArmVirtPkg/PrePei: when starting in EL2 configure HCR to not trap on PAC 2023-01-23 15:58 ` [edk2-devel] " Sami Mujawar @ 2023-01-23 21:16 ` Evgeny Iakovlev 0 siblings, 0 replies; 3+ messages in thread From: Evgeny Iakovlev @ 2023-01-23 21:16 UTC (permalink / raw) To: Sami Mujawar, devel@edk2.groups.io, ardb+tianocore@kernel.org, ardb@kernel.org 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 > > > On 23/01/2023, 14:43, "devel@edk2.groups.io on behalf of Evgeny Iakovlev via groups.io" <devel@edk2.groups.io on behalf of eiakovlev=linux.microsoft.com@groups.io> wrote: > > When FEAT_PAuth is impelemented HCR_EL2.APK and HCR_EL2.API bits control > whether PAC-related instructions and register accesses should be trapped > 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 are > ignored and system behaves as if their value was 0b1. > > When starting in EL2 on ArmVirtPkg get our of the way of a potential > hypervisor by setting APK and API bits. > [SAMI] The modules in ArmVirPkg are expected to be used by guest firmware. Therefore, I do not > expect the code to be executing at EL2. Am, I missing something here? Can you explain, please? > [SAMI] The setup i am using emulates a qemu aarch64 machine with EL3/TrustZone support. Boot starts in EL3 and executes a different firmware (ARM trusted firmware to be specific), which initializes secure mode. Then it passes control over to UEFI FV in the next least privileged EL, which is EL2, so that EDK2 can run a boot chain that ends up with a guest hypervisor. > > Contributed-under: TianoCore Contribution Agreement 1.1 > [SAMI] I believe the above line is no longer required. See, https://github.com/tianocore/edk2#code-contributions Right, i just saw it in recent git history, but good to know. > > Signed-off-by: Evgeny Iakovlev <eiakovlev@linux.microsoft.com> > --- > 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/Chipset/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/ArmPlatformPkg/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 trapping 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 UNKNOWN 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 it doesn't, then those bits are ignored and system always behaves as if 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/PrePi/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 () == AARCH64_EL2) { > > // Trap General Exceptions. All exceptions that would be routed 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 explicit check for that (if (ArmReadCurrentEL () == AARCH64_EL2)). I've described the setup that ends up with EDK2 in EL2 above. > > } > > -- > 2.34.1 > > > > -=-=-=-=-=-= > 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@arm.com] > -=-=-=-=-=-= > > > > IMPORTANT NOTICE: The contents of this email and any attachments are confidential and may also be privileged. If you are not the intended recipient, please notify the sender immediately and do not disclose the contents to any other person, use it for any purpose, or store or copy the information in any medium. Thank you. ^ permalink raw reply [flat|nested] 3+ messages in thread
end of thread, other threads:[~2023-01-23 21:16 UTC | newest] Thread overview: 3+ messages (download: mbox.gz follow: Atom feed -- links below jump to the message on this page -- 2023-01-23 13:33 [PATCH] ArmVirtPkg/PrePei: when starting in EL2 configure HCR to not trap on PAC Evgeny Iakovlev 2023-01-23 15:58 ` [edk2-devel] " Sami Mujawar 2023-01-23 21:16 ` Evgeny Iakovlev
This is a public inbox, see mirroring instructions for how to clone and mirror all data and code used for this inbox