1. Patchew
  2. EDK2
  3. [edk2-devel] [PATCH v5 00/21] Add Raw algorithm support using Arm FW-TRNG interface
  4. View patch

[edk2-devel] [PATCH v5 09/21] MdePkg/BaseRngLib: Rename ArmReadIdIsar0() to ArmGetFeatRng()

PierreGondois posted 21 patches 3 years, 4 months ago
There is a newer version of this series
[edk2-devel] [PATCH v5 09/21] MdePkg/BaseRngLib: Rename ArmReadIdIsar0() to ArmGetFeatRng()
Posted by PierreGondois 3 years, 4 months ago 
From: Pierre Gondois <pierre.gondois@arm.com>

The MdePkg must be self contained and not have external dependencies.
ArmReadIdIsar0() is defined in MdePkg/Library/BaseRngLib and is
limited to the scope of this library.

The same function will be required to check the FEAT_AES and FEAT_RNG
extensions in other libraries. As this function is Arm specific, it
cannot be added to a library interface in MdePkg. It should be part of
ArmPkg/ArmLib.

To avoid having mutiple definitions/prototypes of ArmReadIdIsar0(),
and as BaseRngLib only requires to check the RNG capability bits,
rename the MdePkg/Library/BaseRngLib implementation to ArmGetFeatRng().

Signed-off-by: Pierre Gondois <Pierre.Gondois@arm.com>
---
 .../AArch64/{ArmReadIdIsar0.S => ArmGetFeatRng.S}         | 8 ++++----
 .../AArch64/{ArmReadIdIsar0.asm => ArmGetFeatRng.asm}     | 8 ++++----
 MdePkg/Library/BaseRngLib/AArch64/ArmRng.h                | 2 +-
 MdePkg/Library/BaseRngLib/AArch64/Rndr.c                  | 2 +-
 MdePkg/Library/BaseRngLib/BaseRngLib.inf                  | 4 ++--
 5 files changed, 12 insertions(+), 12 deletions(-)
 rename MdePkg/Library/BaseRngLib/AArch64/{ArmReadIdIsar0.S => ArmGetFeatRng.S} (78%)
 rename MdePkg/Library/BaseRngLib/AArch64/{ArmReadIdIsar0.asm => ArmGetFeatRng.asm} (81%)

diff --git a/MdePkg/Library/BaseRngLib/AArch64/ArmReadIdIsar0.S b/MdePkg/Library/BaseRngLib/AArch64/ArmGetFeatRng.S
similarity index 78%
rename from MdePkg/Library/BaseRngLib/AArch64/ArmReadIdIsar0.S
rename to MdePkg/Library/BaseRngLib/AArch64/ArmGetFeatRng.S
index 82a00d362212..c42d60513077 100644
--- a/MdePkg/Library/BaseRngLib/AArch64/ArmReadIdIsar0.S
+++ b/MdePkg/Library/BaseRngLib/AArch64/ArmGetFeatRng.S
@@ -1,6 +1,6 @@
 #------------------------------------------------------------------------------
 #
-# ArmReadIdIsar0() for AArch64
+# ArmGetFeatRng() for AArch64
 #
 # Copyright (c) 2021, NUVIA Inc. All rights reserved.<BR>
 #
@@ -10,7 +10,7 @@
 
 .text
 .p2align 2
-GCC_ASM_EXPORT(ArmReadIdIsar0)
+GCC_ASM_EXPORT(ArmGetFeatRng)
 
 #/**
 #  Reads the ID_AA64ISAR0 Register.
@@ -20,11 +20,11 @@ GCC_ASM_EXPORT(ArmReadIdIsar0)
 #**/
 #UINT64
 #EFIAPI
-#ArmReadIdIsar0 (
+#ArmGetFeatRng (
 #  VOID
 #  );
 #
-ASM_PFX(ArmReadIdIsar0):
+ASM_PFX(ArmGetFeatRng):
   mrs  x0, id_aa64isar0_el1 // Read ID_AA64ISAR0 Register
   ret
 
diff --git a/MdePkg/Library/BaseRngLib/AArch64/ArmReadIdIsar0.asm b/MdePkg/Library/BaseRngLib/AArch64/ArmGetFeatRng.asm
similarity index 81%
rename from MdePkg/Library/BaseRngLib/AArch64/ArmReadIdIsar0.asm
rename to MdePkg/Library/BaseRngLib/AArch64/ArmGetFeatRng.asm
index 1d9f9a808c0c..947adfcd2749 100644
--- a/MdePkg/Library/BaseRngLib/AArch64/ArmReadIdIsar0.asm
+++ b/MdePkg/Library/BaseRngLib/AArch64/ArmGetFeatRng.asm
@@ -1,6 +1,6 @@
 ;------------------------------------------------------------------------------
 ;
-; ArmReadIdIsar0() for AArch64
+; ArmGetFeatRng() for AArch64
 ;
 ; Copyright (c) 2021, NUVIA Inc. All rights reserved.<BR>
 ;
@@ -8,7 +8,7 @@
 ;
 ;------------------------------------------------------------------------------
 
-  EXPORT ArmReadIdIsar0
+  EXPORT ArmGetFeatRng
   AREA BaseLib_LowLevel, CODE, READONLY
 
 ;/**
@@ -19,11 +19,11 @@
 ;**/
 ;UINT64
 ;EFIAPI
-;ArmReadIdIsar0 (
+;ArmGetFeatRng (
 ;  VOID
 ;  );
 ;
-ArmReadIdIsar0
+ArmGetFeatRng
   mrs  x0, id_aa64isar0_el1 // Read ID_AA64ISAR0 Register
   ret
 
diff --git a/MdePkg/Library/BaseRngLib/AArch64/ArmRng.h b/MdePkg/Library/BaseRngLib/AArch64/ArmRng.h
index 2d6ef48ab941..b35cba3c063a 100644
--- a/MdePkg/Library/BaseRngLib/AArch64/ArmRng.h
+++ b/MdePkg/Library/BaseRngLib/AArch64/ArmRng.h
@@ -35,7 +35,7 @@ ArmRndr (
 **/
 UINT64
 EFIAPI
-ArmReadIdIsar0 (
+ArmGetFeatRng (
   VOID
   );
 
diff --git a/MdePkg/Library/BaseRngLib/AArch64/Rndr.c b/MdePkg/Library/BaseRngLib/AArch64/Rndr.c
index 20811bf3ebf3..0cfdf4c37149 100644
--- a/MdePkg/Library/BaseRngLib/AArch64/Rndr.c
+++ b/MdePkg/Library/BaseRngLib/AArch64/Rndr.c
@@ -47,7 +47,7 @@ BaseRngLibConstructor (
   // Determine RNDR support by examining bits 63:60 of the ISAR0 register returned by
   // MSR. A non-zero value indicates that the processor supports the RNDR instruction.
   //
-  Isar0 = ArmReadIdIsar0 ();
+  Isar0 = ArmGetFeatRng ();
   ASSERT ((Isar0 & RNDR_MASK) != 0);
 
   mRndrSupported = ((Isar0 & RNDR_MASK) != 0);
diff --git a/MdePkg/Library/BaseRngLib/BaseRngLib.inf b/MdePkg/Library/BaseRngLib/BaseRngLib.inf
index 1fcceb941495..d6eccb07d469 100644
--- a/MdePkg/Library/BaseRngLib/BaseRngLib.inf
+++ b/MdePkg/Library/BaseRngLib/BaseRngLib.inf
@@ -37,10 +37,10 @@ [Sources.AARCH64]
   AArch64/Rndr.c
   AArch64/ArmRng.h
 
-  AArch64/ArmReadIdIsar0.S   | GCC
+  AArch64/ArmGetFeatRng.S    | GCC
   AArch64/ArmRng.S           | GCC
 
-  AArch64/ArmReadIdIsar0.asm | MSFT
+  AArch64/ArmGetFeatRng.asm  | MSFT
   AArch64/ArmRng.asm         | MSFT
 
 [Packages]
-- 
2.25.1



-=-=-=-=-=-=-=-=-=-=-=-
Groups.io Links: You receive all messages sent to this group.
View/Reply Online (#93952): https://edk2.groups.io/g/devel/message/93952
Mute This Topic: https://groups.io/mt/93788880/1787277
Group Owner: devel+owner@edk2.groups.io
Unsubscribe: https://edk2.groups.io/g/devel/unsub [importer@patchew.org]
-=-=-=-=-=-=-=-=-=-=-=-
Re: [edk2-devel] [PATCH v5 09/21] MdePkg/BaseRngLib: Rename ArmReadIdIsar0() to ArmGetFeatRng()
Posted by Leif Lindholm 3 years, 4 months ago 
On 2022-09-19 12:21, Pierre.Gondois@arm.com wrote:
> From: Pierre Gondois <pierre.gondois@arm.com>
> 
> The MdePkg must be self contained and not have external dependencies.
> ArmReadIdIsar0() is defined in MdePkg/Library/BaseRngLib and is
> limited to the scope of this library.
 >
> The same function will be required to check the FEAT_AES and FEAT_RNG
> extensions in other libraries. As this function is Arm specific, it
> cannot be added to a library interface in MdePkg. It should be part of
> ArmPkg/ArmLib.

Ultimately, ArmPkg has no justification for existing. It is a legacy of 
ARM support being added as a prototype, bolted onto the side of the rest 
of EDK2. Over time, it needs to be integrated into MdePkg, UefiCpuPkg, 
and possibly MdeModulePkg.

It makes sense to break the ID_ISAR0 read out into a separate library, 
but we're not making the world a better place if we then hide random 
direct-asm-accesses in various libraries. There are similar things in 
other architectures - i.e. CPUID.

But I don't like this patch. I'd prefer to keep this code as-is until 
the separate library has been added - in MdePkg - and we can switch 
straight to that.

/
     Leif

> To avoid having mutiple definitions/prototypes of ArmReadIdIsar0(),
> and as BaseRngLib only requires to check the RNG capability bits,
> rename the MdePkg/Library/BaseRngLib implementation to ArmGetFeatRng().
> 
> Signed-off-by: Pierre Gondois <Pierre.Gondois@arm.com>
> ---
>   .../AArch64/{ArmReadIdIsar0.S => ArmGetFeatRng.S}         | 8 ++++----
>   .../AArch64/{ArmReadIdIsar0.asm => ArmGetFeatRng.asm}     | 8 ++++----
>   MdePkg/Library/BaseRngLib/AArch64/ArmRng.h                | 2 +-
>   MdePkg/Library/BaseRngLib/AArch64/Rndr.c                  | 2 +-
>   MdePkg/Library/BaseRngLib/BaseRngLib.inf                  | 4 ++--
>   5 files changed, 12 insertions(+), 12 deletions(-)
>   rename MdePkg/Library/BaseRngLib/AArch64/{ArmReadIdIsar0.S => ArmGetFeatRng.S} (78%)
>   rename MdePkg/Library/BaseRngLib/AArch64/{ArmReadIdIsar0.asm => ArmGetFeatRng.asm} (81%)
> 
> diff --git a/MdePkg/Library/BaseRngLib/AArch64/ArmReadIdIsar0.S b/MdePkg/Library/BaseRngLib/AArch64/ArmGetFeatRng.S
> similarity index 78%
> rename from MdePkg/Library/BaseRngLib/AArch64/ArmReadIdIsar0.S
> rename to MdePkg/Library/BaseRngLib/AArch64/ArmGetFeatRng.S
> index 82a00d362212..c42d60513077 100644
> --- a/MdePkg/Library/BaseRngLib/AArch64/ArmReadIdIsar0.S
> +++ b/MdePkg/Library/BaseRngLib/AArch64/ArmGetFeatRng.S
> @@ -1,6 +1,6 @@
>   #------------------------------------------------------------------------------
>   #
> -# ArmReadIdIsar0() for AArch64
> +# ArmGetFeatRng() for AArch64
>   #
>   # Copyright (c) 2021, NUVIA Inc. All rights reserved.<BR>
>   #
> @@ -10,7 +10,7 @@
>   
>   .text
>   .p2align 2
> -GCC_ASM_EXPORT(ArmReadIdIsar0)
> +GCC_ASM_EXPORT(ArmGetFeatRng)
>   
>   #/**
>   #  Reads the ID_AA64ISAR0 Register.
> @@ -20,11 +20,11 @@ GCC_ASM_EXPORT(ArmReadIdIsar0)
>   #**/
>   #UINT64
>   #EFIAPI
> -#ArmReadIdIsar0 (
> +#ArmGetFeatRng (
>   #  VOID
>   #  );
>   #
> -ASM_PFX(ArmReadIdIsar0):
> +ASM_PFX(ArmGetFeatRng):
>     mrs  x0, id_aa64isar0_el1 // Read ID_AA64ISAR0 Register
>     ret
>   
> diff --git a/MdePkg/Library/BaseRngLib/AArch64/ArmReadIdIsar0.asm b/MdePkg/Library/BaseRngLib/AArch64/ArmGetFeatRng.asm
> similarity index 81%
> rename from MdePkg/Library/BaseRngLib/AArch64/ArmReadIdIsar0.asm
> rename to MdePkg/Library/BaseRngLib/AArch64/ArmGetFeatRng.asm
> index 1d9f9a808c0c..947adfcd2749 100644
> --- a/MdePkg/Library/BaseRngLib/AArch64/ArmReadIdIsar0.asm
> +++ b/MdePkg/Library/BaseRngLib/AArch64/ArmGetFeatRng.asm
> @@ -1,6 +1,6 @@
>   ;------------------------------------------------------------------------------
>   ;
> -; ArmReadIdIsar0() for AArch64
> +; ArmGetFeatRng() for AArch64
>   ;
>   ; Copyright (c) 2021, NUVIA Inc. All rights reserved.<BR>
>   ;
> @@ -8,7 +8,7 @@
>   ;
>   ;------------------------------------------------------------------------------
>   
> -  EXPORT ArmReadIdIsar0
> +  EXPORT ArmGetFeatRng
>     AREA BaseLib_LowLevel, CODE, READONLY
>   
>   ;/**
> @@ -19,11 +19,11 @@
>   ;**/
>   ;UINT64
>   ;EFIAPI
> -;ArmReadIdIsar0 (
> +;ArmGetFeatRng (
>   ;  VOID
>   ;  );
>   ;
> -ArmReadIdIsar0
> +ArmGetFeatRng
>     mrs  x0, id_aa64isar0_el1 // Read ID_AA64ISAR0 Register
>     ret
>   
> diff --git a/MdePkg/Library/BaseRngLib/AArch64/ArmRng.h b/MdePkg/Library/BaseRngLib/AArch64/ArmRng.h
> index 2d6ef48ab941..b35cba3c063a 100644
> --- a/MdePkg/Library/BaseRngLib/AArch64/ArmRng.h
> +++ b/MdePkg/Library/BaseRngLib/AArch64/ArmRng.h
> @@ -35,7 +35,7 @@ ArmRndr (
>   **/
>   UINT64
>   EFIAPI
> -ArmReadIdIsar0 (
> +ArmGetFeatRng (
>     VOID
>     );
>   
> diff --git a/MdePkg/Library/BaseRngLib/AArch64/Rndr.c b/MdePkg/Library/BaseRngLib/AArch64/Rndr.c
> index 20811bf3ebf3..0cfdf4c37149 100644
> --- a/MdePkg/Library/BaseRngLib/AArch64/Rndr.c
> +++ b/MdePkg/Library/BaseRngLib/AArch64/Rndr.c
> @@ -47,7 +47,7 @@ BaseRngLibConstructor (
>     // Determine RNDR support by examining bits 63:60 of the ISAR0 register returned by
>     // MSR. A non-zero value indicates that the processor supports the RNDR instruction.
>     //
> -  Isar0 = ArmReadIdIsar0 ();
> +  Isar0 = ArmGetFeatRng ();
>     ASSERT ((Isar0 & RNDR_MASK) != 0);
>   
>     mRndrSupported = ((Isar0 & RNDR_MASK) != 0);
> diff --git a/MdePkg/Library/BaseRngLib/BaseRngLib.inf b/MdePkg/Library/BaseRngLib/BaseRngLib.inf
> index 1fcceb941495..d6eccb07d469 100644
> --- a/MdePkg/Library/BaseRngLib/BaseRngLib.inf
> +++ b/MdePkg/Library/BaseRngLib/BaseRngLib.inf
> @@ -37,10 +37,10 @@ [Sources.AARCH64]
>     AArch64/Rndr.c
>     AArch64/ArmRng.h
>   
> -  AArch64/ArmReadIdIsar0.S   | GCC
> +  AArch64/ArmGetFeatRng.S    | GCC
>     AArch64/ArmRng.S           | GCC
>   
> -  AArch64/ArmReadIdIsar0.asm | MSFT
> +  AArch64/ArmGetFeatRng.asm  | MSFT
>     AArch64/ArmRng.asm         | MSFT
>   
>   [Packages]



-=-=-=-=-=-=-=-=-=-=-=-
Groups.io Links: You receive all messages sent to this group.
View/Reply Online (#94482): https://edk2.groups.io/g/devel/message/94482
Mute This Topic: https://groups.io/mt/93788880/1787277
Group Owner: devel+owner@edk2.groups.io
Unsubscribe: https://edk2.groups.io/g/devel/unsub [importer@patchew.org]
-=-=-=-=-=-=-=-=-=-=-=-
Re: [edk2-devel] [PATCH v5 09/21] MdePkg/BaseRngLib: Rename ArmReadIdIsar0() to ArmGetFeatRng()
Posted by PierreGondois 3 years, 4 months ago 

On 9/28/22 19:10, Leif Lindholm wrote:
> On 2022-09-19 12:21, Pierre.Gondois@arm.com wrote:
>> From: Pierre Gondois <pierre.gondois@arm.com>
>>
>> The MdePkg must be self contained and not have external dependencies.
>> ArmReadIdIsar0() is defined in MdePkg/Library/BaseRngLib and is
>> limited to the scope of this library.
>   >
>> The same function will be required to check the FEAT_AES and FEAT_RNG
>> extensions in other libraries. As this function is Arm specific, it
>> cannot be added to a library interface in MdePkg. It should be part of
>> ArmPkg/ArmLib.
> 
> Ultimately, ArmPkg has no justification for existing. It is a legacy of
> ARM support being added as a prototype, bolted onto the side of the rest
> of EDK2. Over time, it needs to be integrated into MdePkg, UefiCpuPkg,
> and possibly MdeModulePkg.
> 
> It makes sense to break the ID_ISAR0 read out into a separate library,
> but we're not making the world a better place if we then hide random
> direct-asm-accesses in various libraries. There are similar things in
> other architectures - i.e. CPUID.
> 
> But I don't like this patch. I'd prefer to keep this code as-is until
> the separate library has been added - in MdePkg - and we can switch
> straight to that.

Ok, it is also possible to use RngGetBytes() to enable the PcdCpuRngSupportedAlgorithm,
so I'll do that and drop:
- ArmPkg/ArmLib: Add ArmHasRngExt()
- ArmPkg/ArmLib: Add ArmReadIdIsar0() helper

Just to understand how the ArmPkg should ideally be melded in other packages,
if I actually needed to add ArmReadIdIsar0() to library should it belong for instance
to:
   MdePkg/Include/Library/BaseLib.h
as some of the functions there allow to read intel extensions ?

Regards,
Pierre

> 
> /
>       Leif
> 
>> To avoid having mutiple definitions/prototypes of ArmReadIdIsar0(),
>> and as BaseRngLib only requires to check the RNG capability bits,
>> rename the MdePkg/Library/BaseRngLib implementation to ArmGetFeatRng().
>>
>> Signed-off-by: Pierre Gondois <Pierre.Gondois@arm.com>
>> ---
>>    .../AArch64/{ArmReadIdIsar0.S => ArmGetFeatRng.S}         | 8 ++++----
>>    .../AArch64/{ArmReadIdIsar0.asm => ArmGetFeatRng.asm}     | 8 ++++----
>>    MdePkg/Library/BaseRngLib/AArch64/ArmRng.h                | 2 +-
>>    MdePkg/Library/BaseRngLib/AArch64/Rndr.c                  | 2 +-
>>    MdePkg/Library/BaseRngLib/BaseRngLib.inf                  | 4 ++--
>>    5 files changed, 12 insertions(+), 12 deletions(-)
>>    rename MdePkg/Library/BaseRngLib/AArch64/{ArmReadIdIsar0.S => ArmGetFeatRng.S} (78%)
>>    rename MdePkg/Library/BaseRngLib/AArch64/{ArmReadIdIsar0.asm => ArmGetFeatRng.asm} (81%)
>>
>> diff --git a/MdePkg/Library/BaseRngLib/AArch64/ArmReadIdIsar0.S b/MdePkg/Library/BaseRngLib/AArch64/ArmGetFeatRng.S
>> similarity index 78%
>> rename from MdePkg/Library/BaseRngLib/AArch64/ArmReadIdIsar0.S
>> rename to MdePkg/Library/BaseRngLib/AArch64/ArmGetFeatRng.S
>> index 82a00d362212..c42d60513077 100644
>> --- a/MdePkg/Library/BaseRngLib/AArch64/ArmReadIdIsar0.S
>> +++ b/MdePkg/Library/BaseRngLib/AArch64/ArmGetFeatRng.S
>> @@ -1,6 +1,6 @@
>>    #------------------------------------------------------------------------------
>>    #
>> -# ArmReadIdIsar0() for AArch64
>> +# ArmGetFeatRng() for AArch64
>>    #
>>    # Copyright (c) 2021, NUVIA Inc. All rights reserved.<BR>
>>    #
>> @@ -10,7 +10,7 @@
>>    
>>    .text
>>    .p2align 2
>> -GCC_ASM_EXPORT(ArmReadIdIsar0)
>> +GCC_ASM_EXPORT(ArmGetFeatRng)
>>    
>>    #/**
>>    #  Reads the ID_AA64ISAR0 Register.
>> @@ -20,11 +20,11 @@ GCC_ASM_EXPORT(ArmReadIdIsar0)
>>    #**/
>>    #UINT64
>>    #EFIAPI
>> -#ArmReadIdIsar0 (
>> +#ArmGetFeatRng (
>>    #  VOID
>>    #  );
>>    #
>> -ASM_PFX(ArmReadIdIsar0):
>> +ASM_PFX(ArmGetFeatRng):
>>      mrs  x0, id_aa64isar0_el1 // Read ID_AA64ISAR0 Register
>>      ret
>>    
>> diff --git a/MdePkg/Library/BaseRngLib/AArch64/ArmReadIdIsar0.asm b/MdePkg/Library/BaseRngLib/AArch64/ArmGetFeatRng.asm
>> similarity index 81%
>> rename from MdePkg/Library/BaseRngLib/AArch64/ArmReadIdIsar0.asm
>> rename to MdePkg/Library/BaseRngLib/AArch64/ArmGetFeatRng.asm
>> index 1d9f9a808c0c..947adfcd2749 100644
>> --- a/MdePkg/Library/BaseRngLib/AArch64/ArmReadIdIsar0.asm
>> +++ b/MdePkg/Library/BaseRngLib/AArch64/ArmGetFeatRng.asm
>> @@ -1,6 +1,6 @@
>>    ;------------------------------------------------------------------------------
>>    ;
>> -; ArmReadIdIsar0() for AArch64
>> +; ArmGetFeatRng() for AArch64
>>    ;
>>    ; Copyright (c) 2021, NUVIA Inc. All rights reserved.<BR>
>>    ;
>> @@ -8,7 +8,7 @@
>>    ;
>>    ;------------------------------------------------------------------------------
>>    
>> -  EXPORT ArmReadIdIsar0
>> +  EXPORT ArmGetFeatRng
>>      AREA BaseLib_LowLevel, CODE, READONLY
>>    
>>    ;/**
>> @@ -19,11 +19,11 @@
>>    ;**/
>>    ;UINT64
>>    ;EFIAPI
>> -;ArmReadIdIsar0 (
>> +;ArmGetFeatRng (
>>    ;  VOID
>>    ;  );
>>    ;
>> -ArmReadIdIsar0
>> +ArmGetFeatRng
>>      mrs  x0, id_aa64isar0_el1 // Read ID_AA64ISAR0 Register
>>      ret
>>    
>> diff --git a/MdePkg/Library/BaseRngLib/AArch64/ArmRng.h b/MdePkg/Library/BaseRngLib/AArch64/ArmRng.h
>> index 2d6ef48ab941..b35cba3c063a 100644
>> --- a/MdePkg/Library/BaseRngLib/AArch64/ArmRng.h
>> +++ b/MdePkg/Library/BaseRngLib/AArch64/ArmRng.h
>> @@ -35,7 +35,7 @@ ArmRndr (
>>    **/
>>    UINT64
>>    EFIAPI
>> -ArmReadIdIsar0 (
>> +ArmGetFeatRng (
>>      VOID
>>      );
>>    
>> diff --git a/MdePkg/Library/BaseRngLib/AArch64/Rndr.c b/MdePkg/Library/BaseRngLib/AArch64/Rndr.c
>> index 20811bf3ebf3..0cfdf4c37149 100644
>> --- a/MdePkg/Library/BaseRngLib/AArch64/Rndr.c
>> +++ b/MdePkg/Library/BaseRngLib/AArch64/Rndr.c
>> @@ -47,7 +47,7 @@ BaseRngLibConstructor (
>>      // Determine RNDR support by examining bits 63:60 of the ISAR0 register returned by
>>      // MSR. A non-zero value indicates that the processor supports the RNDR instruction.
>>      //
>> -  Isar0 = ArmReadIdIsar0 ();
>> +  Isar0 = ArmGetFeatRng ();
>>      ASSERT ((Isar0 & RNDR_MASK) != 0);
>>    
>>      mRndrSupported = ((Isar0 & RNDR_MASK) != 0);
>> diff --git a/MdePkg/Library/BaseRngLib/BaseRngLib.inf b/MdePkg/Library/BaseRngLib/BaseRngLib.inf
>> index 1fcceb941495..d6eccb07d469 100644
>> --- a/MdePkg/Library/BaseRngLib/BaseRngLib.inf
>> +++ b/MdePkg/Library/BaseRngLib/BaseRngLib.inf
>> @@ -37,10 +37,10 @@ [Sources.AARCH64]
>>      AArch64/Rndr.c
>>      AArch64/ArmRng.h
>>    
>> -  AArch64/ArmReadIdIsar0.S   | GCC
>> +  AArch64/ArmGetFeatRng.S    | GCC
>>      AArch64/ArmRng.S           | GCC
>>    
>> -  AArch64/ArmReadIdIsar0.asm | MSFT
>> +  AArch64/ArmGetFeatRng.asm  | MSFT
>>      AArch64/ArmRng.asm         | MSFT
>>    
>>    [Packages]
> 


-=-=-=-=-=-=-=-=-=-=-=-
Groups.io Links: You receive all messages sent to this group.
View/Reply Online (#94525): https://edk2.groups.io/g/devel/message/94525
Mute This Topic: https://groups.io/mt/93788880/1787277
Group Owner: devel+owner@edk2.groups.io
Unsubscribe: https://edk2.groups.io/g/devel/unsub [importer@patchew.org]
-=-=-=-=-=-=-=-=-=-=-=-
Re: [edk2-devel] [PATCH v5 09/21] MdePkg/BaseRngLib: Rename ArmReadIdIsar0() to ArmGetFeatRng()
Posted by Leif Lindholm 3 years, 4 months ago 
On 2022-09-29 09:21, Pierre Gondois wrote:
> 
> 
> On 9/28/22 19:10, Leif Lindholm wrote:
>> On 2022-09-19 12:21, Pierre.Gondois@arm.com wrote:
>>> From: Pierre Gondois <pierre.gondois@arm.com>
>>>
>>> The MdePkg must be self contained and not have external dependencies.
>>> ArmReadIdIsar0() is defined in MdePkg/Library/BaseRngLib and is
>>> limited to the scope of this library.
>>   >
>>> The same function will be required to check the FEAT_AES and FEAT_RNG
>>> extensions in other libraries. As this function is Arm specific, it
>>> cannot be added to a library interface in MdePkg. It should be part of
>>> ArmPkg/ArmLib.
>>
>> Ultimately, ArmPkg has no justification for existing. It is a legacy of
>> ARM support being added as a prototype, bolted onto the side of the rest
>> of EDK2. Over time, it needs to be integrated into MdePkg, UefiCpuPkg,
>> and possibly MdeModulePkg.
>>
>> It makes sense to break the ID_ISAR0 read out into a separate library,
>> but we're not making the world a better place if we then hide random
>> direct-asm-accesses in various libraries. There are similar things in
>> other architectures - i.e. CPUID.
>>
>> But I don't like this patch. I'd prefer to keep this code as-is until
>> the separate library has been added - in MdePkg - and we can switch
>> straight to that.
> 
> Ok, it is also possible to use RngGetBytes() to enable the 
> PcdCpuRngSupportedAlgorithm,
> so I'll do that and drop:
> - ArmPkg/ArmLib: Add ArmHasRngExt()
> - ArmPkg/ArmLib: Add ArmReadIdIsar0() helper

That works for me - thanks!

> Just to understand how the ArmPkg should ideally be melded in other 
> packages,
> if I actually needed to add ArmReadIdIsar0() to library should it belong 
> for instance
> to:
>    MdePkg/Include/Library/BaseLib.h
> as some of the functions there allow to read intel extensions ?

I think it would make sense to create a separate ArchitectureFeatureLib 
(or somesuch) and move the x86 extension detection bits into that as 
well. But then I'm not an MdePkg maintainer. :)

Regards,

Leif

> Regards,
> Pierre
> 
>>
>> /
>>       Leif
>>
>>> To avoid having mutiple definitions/prototypes of ArmReadIdIsar0(),
>>> and as BaseRngLib only requires to check the RNG capability bits,
>>> rename the MdePkg/Library/BaseRngLib implementation to ArmGetFeatRng().
>>>
>>> Signed-off-by: Pierre Gondois <Pierre.Gondois@arm.com>
>>> ---
>>>    .../AArch64/{ArmReadIdIsar0.S => ArmGetFeatRng.S}         | 8 
>>> ++++----
>>>    .../AArch64/{ArmReadIdIsar0.asm => ArmGetFeatRng.asm}     | 8 
>>> ++++----
>>>    MdePkg/Library/BaseRngLib/AArch64/ArmRng.h                | 2 +-
>>>    MdePkg/Library/BaseRngLib/AArch64/Rndr.c                  | 2 +-
>>>    MdePkg/Library/BaseRngLib/BaseRngLib.inf                  | 4 ++--
>>>    5 files changed, 12 insertions(+), 12 deletions(-)
>>>    rename MdePkg/Library/BaseRngLib/AArch64/{ArmReadIdIsar0.S => 
>>> ArmGetFeatRng.S} (78%)
>>>    rename MdePkg/Library/BaseRngLib/AArch64/{ArmReadIdIsar0.asm => 
>>> ArmGetFeatRng.asm} (81%)
>>>
>>> diff --git a/MdePkg/Library/BaseRngLib/AArch64/ArmReadIdIsar0.S 
>>> b/MdePkg/Library/BaseRngLib/AArch64/ArmGetFeatRng.S
>>> similarity index 78%
>>> rename from MdePkg/Library/BaseRngLib/AArch64/ArmReadIdIsar0.S
>>> rename to MdePkg/Library/BaseRngLib/AArch64/ArmGetFeatRng.S
>>> index 82a00d362212..c42d60513077 100644
>>> --- a/MdePkg/Library/BaseRngLib/AArch64/ArmReadIdIsar0.S
>>> +++ b/MdePkg/Library/BaseRngLib/AArch64/ArmGetFeatRng.S
>>> @@ -1,6 +1,6 @@
>>>    
>>> #------------------------------------------------------------------------------ 
>>>
>>>    #
>>> -# ArmReadIdIsar0() for AArch64
>>> +# ArmGetFeatRng() for AArch64
>>>    #
>>>    # Copyright (c) 2021, NUVIA Inc. All rights reserved.<BR>
>>>    #
>>> @@ -10,7 +10,7 @@
>>>    .text
>>>    .p2align 2
>>> -GCC_ASM_EXPORT(ArmReadIdIsar0)
>>> +GCC_ASM_EXPORT(ArmGetFeatRng)
>>>    #/**
>>>    #  Reads the ID_AA64ISAR0 Register.
>>> @@ -20,11 +20,11 @@ GCC_ASM_EXPORT(ArmReadIdIsar0)
>>>    #**/
>>>    #UINT64
>>>    #EFIAPI
>>> -#ArmReadIdIsar0 (
>>> +#ArmGetFeatRng (
>>>    #  VOID
>>>    #  );
>>>    #
>>> -ASM_PFX(ArmReadIdIsar0):
>>> +ASM_PFX(ArmGetFeatRng):
>>>      mrs  x0, id_aa64isar0_el1 // Read ID_AA64ISAR0 Register
>>>      ret
>>> diff --git a/MdePkg/Library/BaseRngLib/AArch64/ArmReadIdIsar0.asm 
>>> b/MdePkg/Library/BaseRngLib/AArch64/ArmGetFeatRng.asm
>>> similarity index 81%
>>> rename from MdePkg/Library/BaseRngLib/AArch64/ArmReadIdIsar0.asm
>>> rename to MdePkg/Library/BaseRngLib/AArch64/ArmGetFeatRng.asm
>>> index 1d9f9a808c0c..947adfcd2749 100644
>>> --- a/MdePkg/Library/BaseRngLib/AArch64/ArmReadIdIsar0.asm
>>> +++ b/MdePkg/Library/BaseRngLib/AArch64/ArmGetFeatRng.asm
>>> @@ -1,6 +1,6 @@
>>>    
>>> ;------------------------------------------------------------------------------ 
>>>
>>>    ;
>>> -; ArmReadIdIsar0() for AArch64
>>> +; ArmGetFeatRng() for AArch64
>>>    ;
>>>    ; Copyright (c) 2021, NUVIA Inc. All rights reserved.<BR>
>>>    ;
>>> @@ -8,7 +8,7 @@
>>>    ;
>>>    
>>> ;------------------------------------------------------------------------------ 
>>>
>>> -  EXPORT ArmReadIdIsar0
>>> +  EXPORT ArmGetFeatRng
>>>      AREA BaseLib_LowLevel, CODE, READONLY
>>>    ;/**
>>> @@ -19,11 +19,11 @@
>>>    ;**/
>>>    ;UINT64
>>>    ;EFIAPI
>>> -;ArmReadIdIsar0 (
>>> +;ArmGetFeatRng (
>>>    ;  VOID
>>>    ;  );
>>>    ;
>>> -ArmReadIdIsar0
>>> +ArmGetFeatRng
>>>      mrs  x0, id_aa64isar0_el1 // Read ID_AA64ISAR0 Register
>>>      ret
>>> diff --git a/MdePkg/Library/BaseRngLib/AArch64/ArmRng.h 
>>> b/MdePkg/Library/BaseRngLib/AArch64/ArmRng.h
>>> index 2d6ef48ab941..b35cba3c063a 100644
>>> --- a/MdePkg/Library/BaseRngLib/AArch64/ArmRng.h
>>> +++ b/MdePkg/Library/BaseRngLib/AArch64/ArmRng.h
>>> @@ -35,7 +35,7 @@ ArmRndr (
>>>    **/
>>>    UINT64
>>>    EFIAPI
>>> -ArmReadIdIsar0 (
>>> +ArmGetFeatRng (
>>>      VOID
>>>      );
>>> diff --git a/MdePkg/Library/BaseRngLib/AArch64/Rndr.c 
>>> b/MdePkg/Library/BaseRngLib/AArch64/Rndr.c
>>> index 20811bf3ebf3..0cfdf4c37149 100644
>>> --- a/MdePkg/Library/BaseRngLib/AArch64/Rndr.c
>>> +++ b/MdePkg/Library/BaseRngLib/AArch64/Rndr.c
>>> @@ -47,7 +47,7 @@ BaseRngLibConstructor (
>>>      // Determine RNDR support by examining bits 63:60 of the ISAR0 
>>> register returned by
>>>      // MSR. A non-zero value indicates that the processor supports 
>>> the RNDR instruction.
>>>      //
>>> -  Isar0 = ArmReadIdIsar0 ();
>>> +  Isar0 = ArmGetFeatRng ();
>>>      ASSERT ((Isar0 & RNDR_MASK) != 0);
>>>      mRndrSupported = ((Isar0 & RNDR_MASK) != 0);
>>> diff --git a/MdePkg/Library/BaseRngLib/BaseRngLib.inf 
>>> b/MdePkg/Library/BaseRngLib/BaseRngLib.inf
>>> index 1fcceb941495..d6eccb07d469 100644
>>> --- a/MdePkg/Library/BaseRngLib/BaseRngLib.inf
>>> +++ b/MdePkg/Library/BaseRngLib/BaseRngLib.inf
>>> @@ -37,10 +37,10 @@ [Sources.AARCH64]
>>>      AArch64/Rndr.c
>>>      AArch64/ArmRng.h
>>> -  AArch64/ArmReadIdIsar0.S   | GCC
>>> +  AArch64/ArmGetFeatRng.S    | GCC
>>>      AArch64/ArmRng.S           | GCC
>>> -  AArch64/ArmReadIdIsar0.asm | MSFT
>>> +  AArch64/ArmGetFeatRng.asm  | MSFT
>>>      AArch64/ArmRng.asm         | MSFT
>>>    [Packages]
>>



-=-=-=-=-=-=-=-=-=-=-=-
Groups.io Links: You receive all messages sent to this group.
View/Reply Online (#94526): https://edk2.groups.io/g/devel/message/94526
Mute This Topic: https://groups.io/mt/93788880/1787277
Group Owner: devel+owner@edk2.groups.io
Unsubscribe: https://edk2.groups.io/g/devel/unsub [importer@patchew.org]
-=-=-=-=-=-=-=-=-=-=-=-
Re: [edk2-devel] [PATCH v5 09/21] MdePkg/BaseRngLib: Rename ArmReadIdIsar0() to ArmGetFeatRng()
Posted by Rebecca Cran 3 years, 4 months ago 
On 9/19/2022 12:21 PM, PierreGondois wrote:

>   ;/**
> @@ -19,11 +19,11 @@
>   ;**/
>   ;UINT64
>   ;EFIAPI
> -;ArmReadIdIsar0 (
> +;ArmGetFeatRng (
>   ;  VOID
>   ;  );
>   ;
> -ArmReadIdIsar0
> +ArmGetFeatRng
>     mrs  x0, id_aa64isar0_el1 // Read ID_AA64ISAR0 Register
>     ret
>   

Should we be masking the register value here instead of in 
BaseRngLibConstructor, to make it more consistent with the new name?


-- 
Rebecca Cran



-=-=-=-=-=-=-=-=-=-=-=-
Groups.io Links: You receive all messages sent to this group.
View/Reply Online (#94442): https://edk2.groups.io/g/devel/message/94442
Mute This Topic: https://groups.io/mt/93788880/1787277
Group Owner: devel+owner@edk2.groups.io
Unsubscribe: https://edk2.groups.io/g/devel/unsub [importer@patchew.org]
-=-=-=-=-=-=-=-=-=-=-=-

Patchew 2.1-devel-b67e4c2

© 2016 - 2026 Red Hat, Inc.