Re: [PATCH 0/9] ArmPkg: Start cleaning up ID register handling

["Ard Biesheuvel" <ard.biesheuvel@arm.com>]

On 12/18/20 3:16 PM, Leif Lindholm wrote:
> As discussed in https://edk2.groups.io/g/devel/topic/78784065,
> the ARM and AArch64 ports were using an unfortunate overloading
> of accessor functions for the ID registers which describe the
> presence and versions of features implemented in a CPU.
> 
> This makes no sense, since the layout, and indeed the names of
> the registers for the different architectural states differ.
> 
> This set replaces the use of the current accessor functions
> with some helper functions in ArmLib. It also moves the
> accessor function prototypes into private headers so they
> can no longer be called from outside ArmLib
> 
> As a pure side effect, we're able to throw away half of
> ArmGicArchLib/ArmGicArchSecLib.
> 
> Note that some additional cleanup can also be done in the ARM
> portion of ArmMmuLib (which implements its own direct accessor
> for ID_MMFR0), and internally in ArmLib where some assembly
> code does its own feature checks, which could be moved to C
> helpers.
> 
> Furthermore, it might be useful to move the ID register field
> definitions to a private header.
> 
> Cc: Ard Biesheuvel <ard.biesheuvel@arm.com>
> 
> Leif Lindholm (9):
>   ArmPkg/ArmLib: add ArmHasGicSystemRegisters () helper function
>   ArmPkg: use ID register helper for ArmGicArch(Sec)Lib
>   ArmPkg: remove duplicated ARM/AArch64 ArmGicArchLib sources
>   ArmPkg: remove duplicated ARM/AArch64 ArmGicArchSecLib sources
>   ArmPkg: add ArmHasSecurityExtensions () helper function
>   ArmPkg: use helper to check for Security extensions in ArmArchTimerLib
>   ArmPkg/ArmLib: delete AArch64 version of ArmReadIdPfr1
>   ArmPkg/ArmLib: rename AArch64 variant of ArmReadIdPfr0
>   ArmPkg/ArmLib: move ArmReadIdPfr0/1 into private header ArmV7Lib.h
> 

Thanks for getting started on this.

For the series,

Reviewed-by: Ard Biesheuvel <ard.biesheuvel@arm.com>

>  ArmPkg/Library/ArmGicArchLib/ArmGicArchLib.inf                |  7 +--
>  ArmPkg/Library/ArmGicArchSecLib/ArmGicArchSecLib.inf          |  7 +--
>  ArmPkg/Include/Library/ArmLib.h                               | 47 +++++++++++----
>  ArmPkg/Library/ArmLib/AArch64/AArch64Lib.h                    |  6 ++
>  ArmPkg/Library/ArmLib/Arm/ArmV7Lib.h                          | 12 ++++
>  ArmPkg/Library/ArmArchTimerLib/ArmArchTimerLib.c              |  2 +-
>  ArmPkg/Library/ArmGicArchLib/Arm/ArmGicArchLib.c              | 60 --------------------
>  ArmPkg/Library/ArmGicArchLib/{AArch64 => }/ArmGicArchLib.c    |  2 +-
>  ArmPkg/Library/ArmGicArchSecLib/Arm/ArmGicArchLib.c           | 45 ---------------
>  ArmPkg/Library/ArmGicArchSecLib/{AArch64 => }/ArmGicArchLib.c |  2 +-
>  ArmPkg/Library/ArmLib/AArch64/AArch64Lib.c                    | 16 ++++++
>  ArmPkg/Library/ArmLib/Arm/ArmV7Lib.c                          | 31 ++++++++++
>  ArmPkg/Library/ArmLib/AArch64/AArch64Support.S                | 13 +----
>  13 files changed, 110 insertions(+), 140 deletions(-)
>  delete mode 100644 ArmPkg/Library/ArmGicArchLib/Arm/ArmGicArchLib.c
>  rename ArmPkg/Library/ArmGicArchLib/{AArch64 => }/ArmGicArchLib.c (94%)
>  delete mode 100644 ArmPkg/Library/ArmGicArchSecLib/Arm/ArmGicArchLib.c
>  rename ArmPkg/Library/ArmGicArchSecLib/{AArch64 => }/ArmGicArchLib.c (94%)
>