On Mon, 6 Mar 2023 at 16:42, Yao, Jiewen <jiewen....@intel.com> wrote: > > Hi Pierre > I don’t have strong opinion. > > For ARM specific patch, would you please get R-B from ARM expert? > > I think we need to wait for the response from Ard to confirm. >
These patches SecurityPkg/RngDxe: Correctly update mAvailableAlgoArrayCount SecurityPkg/RngDxe: Conditionally install EFI_RNG_PROTOCOL Reviewed-by: Ard Biesheuvel <a...@kernel.org> Jiewen, if you don't mind, I will merge those right away. For the remaining patch, I am not sure I understand why the behavior regarding the zero GUID is correct. Perhaps we could revisit/resend/review that patch in isolation? Thanks, Ard. > > > -----Original Message----- > > From: Pierre Gondois <pierre.gond...@arm.com> > > Sent: Monday, March 6, 2023 11:38 PM > > To: devel@edk2.groups.io > > Cc: Leif Lindholm <l...@nuviainc.com>; Sami Mujawar > > <sami.muja...@arm.com>; Yao, Jiewen <jiewen....@intel.com>; Wang, > > Jian J <jian.j.w...@intel.com>; Ard Biesheuvel <a...@kernel.org> > > Subject: Re: [edk2-devel] [PATCH v3 0/4] ArmPkg/SecurityPkg: Fixes for > > ArmTrngLib/RngDxe > > > > Hello Jiewen, Jian, > > Do these patches in this set look ok to you ? > > - SecurityPkg/RngDxe: Correctly update mAvailableAlgoArrayCount > > - SecurityPkg/RngDxe: Conditionally install EFI_RNG_PROTOCOL > > - SecurityPkg/RngDxe: Fix Rng algo selection for Arm > > > > Regards, > > Pierre > > > > On 2/10/23 10:26, PierreGondois via groups.io wrote: > > > Hello Jiewen, Jian, > > > Just a reminder in case this was forgotten, > > > Regards, > > > Pierre > > > > > > On 1/9/23 13:26, Pierre Gondois wrote: > > >> Hello, > > >> I was wondering if I should re-arrange the patch in any way/there was any > > >> concern with the patch set. The first patch of serie was taken, but the > > other > > >> ones are pending, > > >> > > >> Regards, > > >> Pierre > > >> > > >> On 11/28/22 14:34, PierreGondois via groups.io wrote: > > >>> Hello Ard, > > >>> > > >>> On 11/26/22 15:33, Ard Biesheuvel wrote: > > >>>> On Thu, 24 Nov 2022 at 17:18, <pierre.gond...@arm.com> wrote: > > >>>>> > > >>>>> From: Pierre Gondois <pierre.gond...@arm.com> > > >>>>> > > >>>>> v1: > > >>>>> - https://edk2.groups.io/g/devel/message/96356 > > >>>>> v2: > > >>>>> - https://edk2.groups.io/g/devel/message/96434 > > >>>>> - Reformulate commit message. > > >>>>> - Do not warn if no algorithm is found as the message > > >>>>> would be printed on non-Arm platforms. > > >>>>> v3: > > >>>>> - Add the following patches: > > >>>>> 1. ArmPkg/ArmTrngLib: Remove ASSERTs in > > ArmTrngLibConstructor() > > >>>>> Requested by Ard. > > >>>>> Cf https://edk2.groups.io/g/devel/message/96495 > > >>>>> 2. SecurityPkg/RngDxe: Conditionally install EFI_RNG_PROTOCOL > > >>>>> Do not install EFI_RNG_PROTOCOL if no RNG algorithm is > > available. > > >>>>> Cf. https://edk2.groups.io/g/devel/message/96494 > > >>>>> 3. SecurityPkg/RngDxe: Fix Rng algo selection for Arm > > >>>>> Coming from v2 patch being split. > > >>>>> > > >>>>> Some issues were found by Ard/Sami on the RngDxe/ArmTrngLib after > > >>>>> recent patches were merged. This patch serie intends to fix them. > > >>>>> > > >>>>> Pierre Gondois (4): > > >>>>> ArmPkg/ArmTrngLib: Remove ASSERTs in ArmTrngLibConstructor() > > >>>> > > >>>> Thanks for the fixed > > >>>> > > >>>> Reviewed-by: Ard Biesheuvel <a...@kernel.org> > > >>>> > > >>>> I pushed this one as #3663 (pending CI verification atm) > > >>>> > > >>>>> SecurityPkg/RngDxe: Correctly update mAvailableAlgoArrayCount > > >>>>> SecurityPkg/RngDxe: Conditionally install EFI_RNG_PROTOCOL > > >>>>> SecurityPkg/RngDxe: Fix Rng algo selection for Arm > > >>>>> > > >>>> > > >>>> The remaining code still looks a bit clunky to me. Can't we just > > >>>> return an error from the library constructor of the library cannot > > >>>> initialize due to a missing prerequisite? > > >>> > > >>> In RngDriverEntry(), GetAvailableAlgorithms() probe the available > > >>> RNG algorithms. If there are none, then we return an error code. > > >>> Otherwise we install EFI_RNG_PROTOCOL. > > >>> > > >>> I assume the prerequisite you a referring to is 'checking there is > > >>> at least one available RNG algorithm that RngDxe can use', so if > > >>> ArmTrngLib's constructor fails, we should not prevent RngDxe to be > > >>> loaded (as the RngLib could be used for instance). > > >>> > > >>> Does it answer your question, or did I understand it incorrectly ? > > >>> > > >>> Regards, > > >>> Pierre > > >>> > > >>>> > > >>>> > > >>>>> ArmPkg/Library/ArmTrngLib/ArmTrngLib.c | 5 ----- > > >>>>> .../RandomNumberGenerator/RngDxe/ArmRngDxe.c | 18 +++++--- > > ---------- > > >>>>> .../RngDxe/Rand/RngDxe.c | 9 ++++++++- > > >>>>> .../RandomNumberGenerator/RngDxe/RngDxe.c | 19 > > ++++++++++++++----- > > >>>>> 4 files changed, 27 insertions(+), 24 deletions(-) > > >>>>> > > >>>>> -- > > >>>>> 2.25.1 > > >>>>> > > >>> > > >>> > > >>> > > >>> > > >>> > > > > > > > > > > > > > > > -=-=-=-=-=-=-=-=-=-=-=- Groups.io Links: You receive all messages sent to this group. View/Reply Online (#100740): https://edk2.groups.io/g/devel/message/100740 Mute This Topic: https://groups.io/mt/95240503/21656 Group Owner: devel+ow...@edk2.groups.io Unsubscribe: https://edk2.groups.io/g/devel/unsub [arch...@mail-archive.com] -=-=-=-=-=-=-=-=-=-=-=-
