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(-)
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
-=-=-=-=-=-=-=-=-=-=-=-
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/1787277
Group Owner: devel+owner@edk2.groups.io
Unsubscribe: https://edk2.groups.io/g/devel/unsub [importer@patchew.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.
-=-=-=-=-=-=-=-=-=-=-=-
Groups.io Links: You receive all messages sent to this group.
View/Reply Online (#98965): https://edk2.groups.io/g/devel/message/98965
Mute This Topic: https://groups.io/mt/96474724/1787277
Group Owner: devel+owner@edk2.groups.io
Unsubscribe: https://edk2.groups.io/g/devel/unsub [importer@patchew.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.
-=-=-=-=-=-=-=-=-=-=-=-
Groups.io Links: You receive all messages sent to this group.
View/Reply Online (#98970): https://edk2.groups.io/g/devel/message/98970
Mute This Topic: https://groups.io/mt/96474724/1787277
Group Owner: devel+owner@edk2.groups.io
Unsubscribe: https://edk2.groups.io/g/devel/unsub [importer@patchew.org]
-=-=-=-=-=-=-=-=-=-=-=-
© 2016 - 2024 Red Hat, Inc.