From mboxrd@z Thu Jan 1 00:00:00 1970 Received: from foss.arm.com (foss.arm.com [217.140.110.172]) by mx.groups.io with SMTP id smtpd.web09.12704.1664468493595872290 for ; Thu, 29 Sep 2022 09:21:33 -0700 Authentication-Results: mx.groups.io; dkim=missing; spf=pass (domain: arm.com, ip: 217.140.110.172, mailfrom: pierre.gondois@arm.com) Received: from usa-sjc-imap-foss1.foss.arm.com (unknown [10.121.207.14]) by usa-sjc-mx-foss1.foss.arm.com (Postfix) with ESMTP id 3F9001650; Thu, 29 Sep 2022 09:21:39 -0700 (PDT) Received: from [10.57.65.135] (unknown [10.57.65.135]) by usa-sjc-imap-foss1.foss.arm.com (Postfix) with ESMTPSA id 969F33F792; Thu, 29 Sep 2022 09:21:30 -0700 (PDT) Message-ID: <77a95499-dc44-6d16-dc07-e4feeceed1ba@arm.com> Date: Thu, 29 Sep 2022 18:21:24 +0200 MIME-Version: 1.0 User-Agent: Mozilla/5.0 (X11; Linux x86_64; rv:91.0) Gecko/20100101 Thunderbird/91.11.0 Subject: Re: [PATCH v5 09/21] MdePkg/BaseRngLib: Rename ArmReadIdIsar0() to ArmGetFeatRng() To: Leif Lindholm , devel@edk2.groups.io Cc: Sami Mujawar , Ard Biesheuvel , Rebecca Cran , Michael D Kinney , Liming Gao , Jiewen Yao , Jian J Wang References: <20220919192207.637786-1-Pierre.Gondois@arm.com> <20220919192207.637786-10-Pierre.Gondois@arm.com> <07de359d-9aff-4c09-629a-ba47e6a23964@quicinc.com> From: "PierreGondois" In-Reply-To: <07de359d-9aff-4c09-629a-ba47e6a23964@quicinc.com> Content-Language: en-US Content-Type: text/plain; charset=UTF-8; format=flowed Content-Transfer-Encoding: 7bit On 9/28/22 19:10, Leif Lindholm wrote: > On 2022-09-19 12:21, Pierre.Gondois@arm.com wrote: >> From: Pierre Gondois >> >> 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 >> --- >> .../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.
>> # >> @@ -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.
>> ; >> @@ -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] >