Re: [edk2-devel] [PATCH] ArmVirtPkg/PrePei: when starting in EL2 configure HCR to not trap on PAC

["Evgeny Iakovlev" <eiakovlev@linux.microsoft.com>]

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.