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

["Sami Mujawar" <sami.mujawar@arm.com>]

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.