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(-)
From: Pierre Gondois <pierre.gondois@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() SecurityPkg/RngDxe: Correctly update mAvailableAlgoArrayCount SecurityPkg/RngDxe: Conditionally install EFI_RNG_PROTOCOL SecurityPkg/RngDxe: Fix Rng algo selection for Arm 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 (#96617): https://edk2.groups.io/g/devel/message/96617 Mute This Topic: https://groups.io/mt/95240503/1787277 Group Owner: devel+owner@edk2.groups.io Unsubscribe: https://edk2.groups.io/g/devel/unsub [importer@patchew.org] -=-=-=-=-=-=-=-=-=-=-=-
On Thu, 24 Nov 2022 at 17:18, <Pierre.Gondois@arm.com> wrote: > > From: Pierre Gondois <pierre.gondois@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 <ardb@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? > 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 (#96632): https://edk2.groups.io/g/devel/message/96632 Mute This Topic: https://groups.io/mt/95240503/1787277 Group Owner: devel+owner@edk2.groups.io Unsubscribe: https://edk2.groups.io/g/devel/unsub [importer@patchew.org] -=-=-=-=-=-=-=-=-=-=-=-
Hi Ard, On Sat, Nov 26, 2022 at 06:34 AM, Ard Biesheuvel wrote: > > 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? The problem with returning an error code from a library constructor is that it generates an ASSERT for debug builds. The reason is that the generated code in AutoGen.c looks as shown below: VOID EFIAPI ProcessLibraryConstructorList ( IN EFI_HANDLE ImageHandle, IN EFI_SYSTEM_TABLE *SystemTable ) { EFI_STATUS Status; Status = HobLibConstructor (ImageHandle, SystemTable); ASSERT_EFI_ERROR (Status); ... Status = ArmTrngLibConstructor (); ASSERT_RETURN_ERROR (Status); } -=-=-=-=-=-=-=-=-=-=-=- Groups.io Links: You receive all messages sent to this group. View/Reply Online (#97070): https://edk2.groups.io/g/devel/message/97070 Mute This Topic: https://groups.io/mt/95240503/1787277 Group Owner: devel+owner@edk2.groups.io Unsubscribe: https://edk2.groups.io/g/devel/unsub [importer@patchew.org] -=-=-=-=-=-=-=-=-=-=-=-
Apologies, I accidentally sent the message before I finished. The remaining part of my reply is as below. I think having the asserts in ProcessLibraryConstructorList() creates more problem. It is very hard to debug issues if the serial port initialisation fails. Regards, Sami Mujawar -=-=-=-=-=-=-=-=-=-=-=- Groups.io Links: You receive all messages sent to this group. View/Reply Online (#97071): https://edk2.groups.io/g/devel/message/97071 Mute This Topic: https://groups.io/mt/95240503/1787277 Group Owner: devel+owner@edk2.groups.io Unsubscribe: https://edk2.groups.io/g/devel/unsub [importer@patchew.org] -=-=-=-=-=-=-=-=-=-=-=-
Hello Ard, On 11/26/22 15:33, Ard Biesheuvel wrote: > On Thu, 24 Nov 2022 at 17:18, <Pierre.Gondois@arm.com> wrote: >> >> From: Pierre Gondois <pierre.gondois@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 <ardb@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 (#96652): https://edk2.groups.io/g/devel/message/96652 Mute This Topic: https://groups.io/mt/95240503/1787277 Group Owner: devel+owner@edk2.groups.io Unsubscribe: https://edk2.groups.io/g/devel/unsub [importer@patchew.org] -=-=-=-=-=-=-=-=-=-=-=-
© 2016 - 2024 Red Hat, Inc.