From mboxrd@z Thu Jan 1 00:00:00 1970 Received: from alexa-out-sd-02.qualcomm.com (alexa-out-sd-02.qualcomm.com [199.106.114.39]) by mx.groups.io with SMTP id smtpd.web11.6.1664471355250405412 for ; Thu, 29 Sep 2022 10:09:15 -0700 Authentication-Results: mx.groups.io; dkim=pass header.i=@quicinc.com header.s=qcdkim header.b=K3D3MmpS; spf=permerror, err=parse error for token &{10 18 %{ir}.%{v}.%{d}.spf.has.pphosted.com}: invalid domain name (domain: quicinc.com, ip: 199.106.114.39, mailfrom: quic_llindhol@quicinc.com) DKIM-Signature: v=1; a=rsa-sha256; c=relaxed/relaxed; d=quicinc.com; i=@quicinc.com; q=dns/txt; s=qcdkim; t=1664471355; x=1696007355; h=message-id:date:mime-version:subject:to:cc:references: from:in-reply-to:content-transfer-encoding; bh=25ycnoRMwYbDOlhsv6+z9QtHazBg+Z8iPSOB+ZwYGXI=; b=K3D3MmpSBPQ761SmPlbW71G7/xOX3obO0DrhnneJgNu2dAioqfy4Gem5 Ul6GWOY4wsQUrRzGprHmDe0KLhHa0sbV6Ir+gsQ4RNdCrvn/+9M6dW7Pj S3T69WVIawsnlFfbIZgT6vrdBO7EAbuyrnBUnyXQ6wKyiWJH+cXrFuWVC E=; Received: from unknown (HELO ironmsg04-sd.qualcomm.com) ([10.53.140.144]) by alexa-out-sd-02.qualcomm.com with ESMTP; 29 Sep 2022 10:09:14 -0700 X-QCInternal: smtphost Received: from nasanex01c.na.qualcomm.com ([10.45.79.139]) by ironmsg04-sd.qualcomm.com with ESMTP/TLS/ECDHE-RSA-AES256-GCM-SHA384; 29 Sep 2022 10:09:14 -0700 Received: from [10.110.93.70] (10.80.80.8) by nasanex01c.na.qualcomm.com (10.45.79.139) with Microsoft SMTP Server (version=TLS1_2, cipher=TLS_ECDHE_RSA_WITH_AES_256_GCM_SHA384) id 15.2.986.29; Thu, 29 Sep 2022 10:09:13 -0700 Message-ID: <510bde4f-e405-5ef6-1d3c-29d30e3d1654@quicinc.com> Date: Thu, 29 Sep 2022 10:09:12 -0700 MIME-Version: 1.0 User-Agent: Mozilla/5.0 (Windows NT 10.0; Win64; x64; rv:91.0) Gecko/20100101 Thunderbird/91.13.0 Subject: Re: [PATCH v5 09/21] MdePkg/BaseRngLib: Rename ArmReadIdIsar0() to ArmGetFeatRng() To: Pierre Gondois , 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> <77a95499-dc44-6d16-dc07-e4feeceed1ba@arm.com> From: "Leif Lindholm" In-Reply-To: <77a95499-dc44-6d16-dc07-e4feeceed1ba@arm.com> Return-Path: quic_llindhol@quicinc.com X-Originating-IP: [10.80.80.8] X-ClientProxiedBy: nasanex01b.na.qualcomm.com (10.46.141.250) To nasanex01c.na.qualcomm.com (10.45.79.139) Content-Type: text/plain; charset="UTF-8"; format=flowed Content-Transfer-Encoding: 8bit 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 >>> >>> 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 >>> --- >>>    .../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] >>