[edk2-devel] [PATCH edk2-platforms 0/1] RFC: SbsaQemu use FEAT_RNG for EFI_RNG_PROTOCOL

[edk2-devel] [PATCH edk2-platforms 0/1] RFC: SbsaQemu use FEAT_RNG for EFI_RNG_PROTOCOL

[Marcin Juszkiewicz]

SBSA Reference Platform in QEMU uses Neoverse-N2 cpu by default now.
This core supports FEAT_RNG feature so I thought that it should be
possible to use it for EFI_RNG_PROTOCOL.

Checked history and found that commit
5de5e230a80bed083360da95ba16a2c4a001620d (in EDK2) enabled that for
ArmVirt platform.

When I boot with Neoverse-N2 or 'max' cpu then EFI_RNG_PROTOCOL gets
reported by 'efi-stub' on Linux boot.

Other cpu models do not have FEAT_RNG so RngDxe driver is not running
and we have situation similar to previous one - no EFI_RNG_PROTOCOL
reported.

I left ArmTrngLib enabled because RngDxe does not start without it.
Probably there is a better way to handle it.

Now the question is: do we want to go this way?

And what to do with older cores? BaseRngLibTimerLib is not(?) used now
so are they left with nothing? I would prefer to not add
'virtio-rng-pci' device in QEMU to have some TRNG emulated.

Signed-off-by: Marcin Juszkiewicz <marcin.juszkiewicz@linaro.org>


Marcin Juszkiewicz (1):
  SbsaQemu: use FEAT_RNG for EFI_RNG_PROTOCOL

 Platform/Qemu/SbsaQemu/SbsaQemu.dsc | 6 +++++-
 Platform/Qemu/SbsaQemu/SbsaQemu.fdf | 1 +
 2 files changed, 6 insertions(+), 1 deletion(-)

-- 
2.45.2



-=-=-=-=-=-=-=-=-=-=-=-
Groups.io Links: You receive all messages sent to this group.
View/Reply Online (#119723): https://edk2.groups.io/g/devel/message/119723
Mute This Topic: https://groups.io/mt/106909458/7686176
Group Owner: devel+owner@edk2.groups.io
Unsubscribe: https://edk2.groups.io/g/devel/unsub [rebecca@openfw.io]
-=-=-=-=-=-=-=-=-=-=-=-

[edk2-devel] [PATCH edk2-platforms 1/1] SbsaQemu: use FEAT_RNG for EFI_RNG_PROTOCOL

[Marcin Juszkiewicz]

By default we have Neoverse-N2 cpu which supports FEAT_RNG feature.

Commit 5de5e230a80bed083360da95ba16a2c4a001620d (in EDK2) enabled that for
ArmVirt platform.

RNDR is implemented by both Neoverse-N2 and 'max' cpu implemented by QEMU.
Other cpu models lack it which prevents the RngDxe driver from running,
resulting in the same situation as before.

TRNG is not implemented in TCG mode but is required by RngDxe to run.

Signed-off-by: Marcin Juszkiewicz <marcin.juszkiewicz@linaro.org>
---
 Platform/Qemu/SbsaQemu/SbsaQemu.dsc | 6 +++++-
 Platform/Qemu/SbsaQemu/SbsaQemu.fdf | 1 +
 2 files changed, 6 insertions(+), 1 deletion(-)

diff --git a/Platform/Qemu/SbsaQemu/SbsaQemu.dsc b/Platform/Qemu/SbsaQemu/SbsaQemu.dsc
index 9306986bf7c0..3463e5c7a635 100644
--- a/Platform/Qemu/SbsaQemu/SbsaQemu.dsc
+++ b/Platform/Qemu/SbsaQemu/SbsaQemu.dsc
@@ -148,7 +148,9 @@ [LibraryClasses.common]
   #
   IntrinsicLib|CryptoPkg/Library/IntrinsicLib/IntrinsicLib.inf
   OpensslLib|CryptoPkg/Library/OpensslLib/OpensslLib.inf
-  RngLib|MdeModulePkg/Library/BaseRngLibTimerLib/BaseRngLibTimerLib.inf
+  RngLib|MdePkg/Library/BaseRngLib/BaseRngLib.inf
+  ArmTrngLib|ArmPkg/Library/ArmTrngLib/ArmTrngLib.inf
+  ArmMonitorLib|ArmPkg/Library/ArmMonitorLib/ArmMonitorLib.inf
   BaseCryptLib|CryptoPkg/Library/BaseCryptLib/BaseCryptLib.inf
 
   #
@@ -660,6 +662,8 @@ [Components.common]
   OvmfPkg/VirtNorFlashDxe/VirtNorFlashDxe.inf
   MdeModulePkg/Universal/WatchdogTimerDxe/WatchdogTimer.inf
   Silicon/Qemu/SbsaQemu/Drivers/SbsaQemuHighMemDxe/SbsaQemuHighMemDxe.inf
+  SecurityPkg/RandomNumberGenerator/RngDxe/RngDxe.inf
+
 
   #
   # FAT filesystem + GPT/MBR partitioning
diff --git a/Platform/Qemu/SbsaQemu/SbsaQemu.fdf b/Platform/Qemu/SbsaQemu/SbsaQemu.fdf
index b35f42e11aa4..51a1ef8519f9 100644
--- a/Platform/Qemu/SbsaQemu/SbsaQemu.fdf
+++ b/Platform/Qemu/SbsaQemu/SbsaQemu.fdf
@@ -192,6 +192,7 @@ [FV.FvMain]
   INF ArmPkg/Drivers/TimerDxe/TimerDxe.inf
   INF OvmfPkg/VirtNorFlashDxe/VirtNorFlashDxe.inf
   INF MdeModulePkg/Universal/WatchdogTimerDxe/WatchdogTimer.inf
+  INF SecurityPkg/RandomNumberGenerator/RngDxe/RngDxe.inf
 
   #
   # FAT filesystem + GPT/MBR partitioning + UDF filesystem
-- 
2.45.2



-=-=-=-=-=-=-=-=-=-=-=-
Groups.io Links: You receive all messages sent to this group.
View/Reply Online (#119724): https://edk2.groups.io/g/devel/message/119724
Mute This Topic: https://groups.io/mt/106909459/7686176
Group Owner: devel+owner@edk2.groups.io
Unsubscribe: https://edk2.groups.io/g/devel/unsub [rebecca@openfw.io]
-=-=-=-=-=-=-=-=-=-=-=-

Re: [edk2-devel] [PATCH edk2-platforms 1/1] SbsaQemu: use FEAT_RNG for EFI_RNG_PROTOCOL

[Ard Biesheuvel]

On Thu, 27 Jun 2024 at 16:22, Marcin Juszkiewicz
<marcin.juszkiewicz@linaro.org> wrote:
>
> By default we have Neoverse-N2 cpu which supports FEAT_RNG feature.
>
> Commit 5de5e230a80bed083360da95ba16a2c4a001620d (in EDK2) enabled that for
> ArmVirt platform.
>
> RNDR is implemented by both Neoverse-N2 and 'max' cpu implemented by QEMU.
> Other cpu models lack it which prevents the RngDxe driver from running,
> resulting in the same situation as before.
>
> TRNG is not implemented in TCG mode but is required by RngDxe to run.
>
> Signed-off-by: Marcin Juszkiewicz <marcin.juszkiewicz@linaro.org>

Reviewed-by: Ard Biesheuvel <ardb@kernel.org>

> ---
>  Platform/Qemu/SbsaQemu/SbsaQemu.dsc | 6 +++++-
>  Platform/Qemu/SbsaQemu/SbsaQemu.fdf | 1 +
>  2 files changed, 6 insertions(+), 1 deletion(-)
>
> diff --git a/Platform/Qemu/SbsaQemu/SbsaQemu.dsc b/Platform/Qemu/SbsaQemu/SbsaQemu.dsc
> index 9306986bf7c0..3463e5c7a635 100644
> --- a/Platform/Qemu/SbsaQemu/SbsaQemu.dsc
> +++ b/Platform/Qemu/SbsaQemu/SbsaQemu.dsc
> @@ -148,7 +148,9 @@ [LibraryClasses.common]
>    #
>    IntrinsicLib|CryptoPkg/Library/IntrinsicLib/IntrinsicLib.inf
>    OpensslLib|CryptoPkg/Library/OpensslLib/OpensslLib.inf
> -  RngLib|MdeModulePkg/Library/BaseRngLibTimerLib/BaseRngLibTimerLib.inf
> +  RngLib|MdePkg/Library/BaseRngLib/BaseRngLib.inf
> +  ArmTrngLib|ArmPkg/Library/ArmTrngLib/ArmTrngLib.inf
> +  ArmMonitorLib|ArmPkg/Library/ArmMonitorLib/ArmMonitorLib.inf
>    BaseCryptLib|CryptoPkg/Library/BaseCryptLib/BaseCryptLib.inf
>
>    #
> @@ -660,6 +662,8 @@ [Components.common]
>    OvmfPkg/VirtNorFlashDxe/VirtNorFlashDxe.inf
>    MdeModulePkg/Universal/WatchdogTimerDxe/WatchdogTimer.inf
>    Silicon/Qemu/SbsaQemu/Drivers/SbsaQemuHighMemDxe/SbsaQemuHighMemDxe.inf
> +  SecurityPkg/RandomNumberGenerator/RngDxe/RngDxe.inf
> +
>
>    #
>    # FAT filesystem + GPT/MBR partitioning
> diff --git a/Platform/Qemu/SbsaQemu/SbsaQemu.fdf b/Platform/Qemu/SbsaQemu/SbsaQemu.fdf
> index b35f42e11aa4..51a1ef8519f9 100644
> --- a/Platform/Qemu/SbsaQemu/SbsaQemu.fdf
> +++ b/Platform/Qemu/SbsaQemu/SbsaQemu.fdf
> @@ -192,6 +192,7 @@ [FV.FvMain]
>    INF ArmPkg/Drivers/TimerDxe/TimerDxe.inf
>    INF OvmfPkg/VirtNorFlashDxe/VirtNorFlashDxe.inf
>    INF MdeModulePkg/Universal/WatchdogTimerDxe/WatchdogTimer.inf
> +  INF SecurityPkg/RandomNumberGenerator/RngDxe/RngDxe.inf
>
>    #
>    # FAT filesystem + GPT/MBR partitioning + UDF filesystem
> --
> 2.45.2
>
>
>
> ------------
> Groups.io Links: You receive all messages sent to this group.
> View/Reply Online (#119724): https://edk2.groups.io/g/devel/message/119724
> Mute This Topic: https://groups.io/mt/106909459/1131722
> Group Owner: devel+owner@edk2.groups.io
> Unsubscribe: https://edk2.groups.io/g/devel/unsub [ardb@kernel.org]
> ------------
>
>


-=-=-=-=-=-=-=-=-=-=-=-
Groups.io Links: You receive all messages sent to this group.
View/Reply Online (#119731): https://edk2.groups.io/g/devel/message/119731
Mute This Topic: https://groups.io/mt/106909459/7686176
Group Owner: devel+owner@edk2.groups.io
Unsubscribe: https://edk2.groups.io/g/devel/unsub [rebecca@openfw.io]
-=-=-=-=-=-=-=-=-=-=-=-

Re: [edk2-devel] [PATCH edk2-platforms 1/1] SbsaQemu: use FEAT_RNG for EFI_RNG_PROTOCOL

[Leif Lindholm]

On 2024-06-27 15:22, Marcin Juszkiewicz wrote:
> By default we have Neoverse-N2 cpu which supports FEAT_RNG feature.
> 
> Commit 5de5e230a80bed083360da95ba16a2c4a001620d (in EDK2) enabled that for
> ArmVirt platform.
> 
> RNDR is implemented by both Neoverse-N2 and 'max' cpu implemented by QEMU.
> Other cpu models lack it which prevents the RngDxe driver from running,
> resulting in the same situation as before.
> 
> TRNG is not implemented in TCG mode but is required by RngDxe to run.

This commit also adds RngDxe for this platform, which neither the short 
nor the long description mentions.

> Signed-off-by: Marcin Juszkiewicz <marcin.juszkiewicz@linaro.org>
> ---
>   Platform/Qemu/SbsaQemu/SbsaQemu.dsc | 6 +++++-
>   Platform/Qemu/SbsaQemu/SbsaQemu.fdf | 1 +
>   2 files changed, 6 insertions(+), 1 deletion(-)
> 
> diff --git a/Platform/Qemu/SbsaQemu/SbsaQemu.dsc b/Platform/Qemu/SbsaQemu/SbsaQemu.dsc
> index 9306986bf7c0..3463e5c7a635 100644
> --- a/Platform/Qemu/SbsaQemu/SbsaQemu.dsc
> +++ b/Platform/Qemu/SbsaQemu/SbsaQemu.dsc
> @@ -148,7 +148,9 @@ [LibraryClasses.common]
>     #

Since sbsa-ref still supports processors without FEAT_RNG, this may 
cause unexpected breakages for some users.

Could we first of all conditionalise this change:

[Defines]
...
   DEFINE_DEBUG_PRINT_ERROR_LEVEL = ...
   DEFINE FEATRNG_ENABLE		 = TRUE

so that someone who still wishes to run tests against older cpus can 
still do so through a rebuild with -D FEATRNG_ENABLE=FALSE

>     IntrinsicLib|CryptoPkg/Library/IntrinsicLib/IntrinsicLib.inf
>     OpensslLib|CryptoPkg/Library/OpensslLib/OpensslLib.inf

!if $(FEATRNG_ENABLE) == TRUE
   RngLib|MdePkg/Library/BaseRngLib/BaseRngLib.inf
!else
   RngLib|MdeModulePkg/Library/BaseRngLibTimerLib/BaseRngLibTimerLib.inf
!endif
   ArmTrngLib|ArmPkg/Library/ArmTrngLib/ArmTrngLib.inf
   ArmMonitorLib|ArmPkg/Library/ArmMonitorLib/ArmMonitorLib.inf

>     BaseCryptLib|CryptoPkg/Library/BaseCryptLib/BaseCryptLib.inf
>   
>     #
> @@ -660,6 +662,8 @@ [Components.common]
>     OvmfPkg/VirtNorFlashDxe/VirtNorFlashDxe.inf
>     MdeModulePkg/Universal/WatchdogTimerDxe/WatchdogTimer.inf
>     Silicon/Qemu/SbsaQemu/Drivers/SbsaQemuHighMemDxe/SbsaQemuHighMemDxe.inf
> +  SecurityPkg/RandomNumberGenerator/RngDxe/RngDxe.inf
> +

Spurious added newline.

>   
>     #
>     # FAT filesystem + GPT/MBR partitioning
> diff --git a/Platform/Qemu/SbsaQemu/SbsaQemu.fdf b/Platform/Qemu/SbsaQemu/SbsaQemu.fdf
> index b35f42e11aa4..51a1ef8519f9 100644
> --- a/Platform/Qemu/SbsaQemu/SbsaQemu.fdf
> +++ b/Platform/Qemu/SbsaQemu/SbsaQemu.fdf
> @@ -192,6 +192,7 @@ [FV.FvMain]
>     INF ArmPkg/Drivers/TimerDxe/TimerDxe.inf
>     INF OvmfPkg/VirtNorFlashDxe/VirtNorFlashDxe.inf
>     INF MdeModulePkg/Universal/WatchdogTimerDxe/WatchdogTimer.inf
> +  INF SecurityPkg/RandomNumberGenerator/RngDxe/RngDxe.inf

Second:
What is the failure mode of running the BaseRngLib flavour on cpus that 
don't support FEAT_RNG? RngDxe itself seems to do the right thing, but 
do we get any warning messages or will certain operations now fail silently?

/
     Leif

>   
>     #
>     # FAT filesystem + GPT/MBR partitioning + UDF filesystem



-=-=-=-=-=-=-=-=-=-=-=-
Groups.io Links: You receive all messages sent to this group.
View/Reply Online (#119735): https://edk2.groups.io/g/devel/message/119735
Mute This Topic: https://groups.io/mt/106909459/7686176
Group Owner: devel+owner@edk2.groups.io
Unsubscribe: https://edk2.groups.io/g/devel/unsub [rebecca@openfw.io]
-=-=-=-=-=-=-=-=-=-=-=-

Re: [edk2-devel] [PATCH edk2-platforms 1/1] SbsaQemu: use FEAT_RNG for EFI_RNG_PROTOCOL

[Marcin Juszkiewicz]

W dniu 1.07.2024 o 13:08, Leif Lindholm pisze:
> On 2024-06-27 15:22, Marcin Juszkiewicz wrote:
>> By default we have Neoverse-N2 cpu which supports FEAT_RNG feature.
>>
>> Commit 5de5e230a80bed083360da95ba16a2c4a001620d (in EDK2) enabled that 
>> for
>> ArmVirt platform.
>>
>> RNDR is implemented by both Neoverse-N2 and 'max' cpu implemented by 
>> QEMU.
>> Other cpu models lack it which prevents the RngDxe driver from running,
>> resulting in the same situation as before.
>>
>> TRNG is not implemented in TCG mode but is required by RngDxe to run.
> 
> This commit also adds RngDxe for this platform, which neither the short 
> nor the long description mentions.
> 
>> Signed-off-by: Marcin Juszkiewicz <marcin.juszkiewicz@linaro.org>
>> ---
>>   Platform/Qemu/SbsaQemu/SbsaQemu.dsc | 6 +++++-
>>   Platform/Qemu/SbsaQemu/SbsaQemu.fdf | 1 +
>>   2 files changed, 6 insertions(+), 1 deletion(-)
>>
>> diff --git a/Platform/Qemu/SbsaQemu/SbsaQemu.dsc 
>> b/Platform/Qemu/SbsaQemu/SbsaQemu.dsc
>> index 9306986bf7c0..3463e5c7a635 100644
>> --- a/Platform/Qemu/SbsaQemu/SbsaQemu.dsc
>> +++ b/Platform/Qemu/SbsaQemu/SbsaQemu.dsc
>> @@ -148,7 +148,9 @@ [LibraryClasses.common]
>>     #
> 
> Since sbsa-ref still supports processors without FEAT_RNG, this may 
> cause unexpected breakages for some users.

That's why I sent it as more of RFC than changes for merging.

> Could we first of all conditionalise this change:
> 
> [Defines]
> ...
>    DEFINE_DEBUG_PRINT_ERROR_LEVEL = ...
>    DEFINE FEATRNG_ENABLE         = TRUE
> 
> so that someone who still wishes to run tests against older cpus can 
> still do so through a rebuild with -D FEATRNG_ENABLE=FALSE

Is there a way to load both BaseRngLib and BaseRngLibTimerLib and switch
between them depending on availability of FEAT_RNG?
  
>>     IntrinsicLib|CryptoPkg/Library/IntrinsicLib/IntrinsicLib.inf
>>     OpensslLib|CryptoPkg/Library/OpensslLib/OpensslLib.inf
> 
> !if $(FEATRNG_ENABLE) == TRUE
>    RngLib|MdePkg/Library/BaseRngLib/BaseRngLib.inf
> !else
>    RngLib|MdeModulePkg/Library/BaseRngLibTimerLib/BaseRngLibTimerLib.inf
> !endif
>    ArmTrngLib|ArmPkg/Library/ArmTrngLib/ArmTrngLib.inf
>    ArmMonitorLib|ArmPkg/Library/ArmMonitorLib/ArmMonitorLib.inf
> 
>>     BaseCryptLib|CryptoPkg/Library/BaseCryptLib/BaseCryptLib.inf
>>     #
>> @@ -660,6 +662,8 @@ [Components.common]
>>     OvmfPkg/VirtNorFlashDxe/VirtNorFlashDxe.inf
>>     MdeModulePkg/Universal/WatchdogTimerDxe/WatchdogTimer.inf
>>     
>> Silicon/Qemu/SbsaQemu/Drivers/SbsaQemuHighMemDxe/SbsaQemuHighMemDxe.inf
>> +  SecurityPkg/RandomNumberGenerator/RngDxe/RngDxe.inf
>> +
> 
> Spurious added newline.
> 
>>     #
>>     # FAT filesystem + GPT/MBR partitioning
>> diff --git a/Platform/Qemu/SbsaQemu/SbsaQemu.fdf 
>> b/Platform/Qemu/SbsaQemu/SbsaQemu.fdf
>> index b35f42e11aa4..51a1ef8519f9 100644
>> --- a/Platform/Qemu/SbsaQemu/SbsaQemu.fdf
>> +++ b/Platform/Qemu/SbsaQemu/SbsaQemu.fdf
>> @@ -192,6 +192,7 @@ [FV.FvMain]
>>     INF ArmPkg/Drivers/TimerDxe/TimerDxe.inf
>>     INF OvmfPkg/VirtNorFlashDxe/VirtNorFlashDxe.inf
>>     INF MdeModulePkg/Universal/WatchdogTimerDxe/WatchdogTimer.inf
>> +  INF SecurityPkg/RandomNumberGenerator/RngDxe/RngDxe.inf
> 
> Second:
> What is the failure mode of running the BaseRngLib flavour on cpus that 
> don't support FEAT_RNG? RngDxe itself seems to do the right thing, but 
> do we get any warning messages or will certain operations now fail 
> silently?

On FEAT_RNG cores we get:

InstallProtocolInterface: BC62157E-3E33-4FEC-9920-2D3B36D750DF 101FAD68798
ProtectUefiImageCommon - 0xFAD683C0
   - 0x00000101FBBDB000 - 0x0000000000007000
ArmTrngLib could not be correctly initialized.
InstallProtocolInterface: 3152BCA5-EADE-433D-862E-C01CDC291F44 101FBBE0020
Loading driver B601F8C4-43B7-4784-95B1-F4226CB40CEE


On core without FEAT_RNG:

InstallProtocolInterface: BC62157E-3E33-4FEC-9920-2D3B36D750DF 101FAD68798
ProtectUefiImageCommon - 0xFAD683C0
   - 0x00000101FBBDB000 - 0x0000000000007000
ArmTrngLib could not be correctly initialized.
Error: Image at 101FBBDB000 start failed: 00000001
remove-symbol-file /home/marcin/devel/linaro/sbsa-qemu/code/Build/SbsaQemu/DEBUG_GCC/AARCH64/SecurityPkg/RandomNumberGenerator/RngDxe/RngDxe/DEBUG/RngDxe.dll 0xFBBDC000
Loading driver B601F8C4-43B7-4784-95B1-F4226CB40CEE


So there is some kind of information but you need to know what
to look for ;(



-=-=-=-=-=-=-=-=-=-=-=-
Groups.io Links: You receive all messages sent to this group.
View/Reply Online (#119736): https://edk2.groups.io/g/devel/message/119736
Mute This Topic: https://groups.io/mt/106909459/7686176
Group Owner: devel+owner@edk2.groups.io
Unsubscribe: https://edk2.groups.io/g/devel/unsub [rebecca@openfw.io]
-=-=-=-=-=-=-=-=-=-=-=-

Re: [edk2-devel] [PATCH edk2-platforms 1/1] SbsaQemu: use FEAT_RNG for EFI_RNG_PROTOCOL

[Leif Lindholm]

On 2024-07-01 13:58, Marcin Juszkiewicz wrote:

>>> diff --git a/Platform/Qemu/SbsaQemu/SbsaQemu.dsc 
>>> b/Platform/Qemu/SbsaQemu/SbsaQemu.dsc
>>> index 9306986bf7c0..3463e5c7a635 100644
>>> --- a/Platform/Qemu/SbsaQemu/SbsaQemu.dsc
>>> +++ b/Platform/Qemu/SbsaQemu/SbsaQemu.dsc
>>> @@ -148,7 +148,9 @@ [LibraryClasses.common]
>>>     #
>>
>> Since sbsa-ref still supports processors without FEAT_RNG, this may 
>> cause unexpected breakages for some users.
> 
> That's why I sent it as more of RFC than changes for merging.
> 
>> Could we first of all conditionalise this change:
>>
>> [Defines]
>> ...
>>    DEFINE_DEBUG_PRINT_ERROR_LEVEL = ...
>>    DEFINE FEATRNG_ENABLE         = TRUE
>>
>> so that someone who still wishes to run tests against older cpus can 
>> still do so through a rebuild with -D FEATRNG_ENABLE=FALSE
> 
> Is there a way to load both BaseRngLib and BaseRngLibTimerLib and switch
> between them depending on availability of FEAT_RNG?

Not without severe hackery. The library is statically linked into RngDxe.

>>> diff --git a/Platform/Qemu/SbsaQemu/SbsaQemu.fdf 
>>> b/Platform/Qemu/SbsaQemu/SbsaQemu.fdf
>>> index b35f42e11aa4..51a1ef8519f9 100644
>>> --- a/Platform/Qemu/SbsaQemu/SbsaQemu.fdf
>>> +++ b/Platform/Qemu/SbsaQemu/SbsaQemu.fdf
>>> @@ -192,6 +192,7 @@ [FV.FvMain]
>>>     INF ArmPkg/Drivers/TimerDxe/TimerDxe.inf
>>>     INF OvmfPkg/VirtNorFlashDxe/VirtNorFlashDxe.inf
>>>     INF MdeModulePkg/Universal/WatchdogTimerDxe/WatchdogTimer.inf
>>> +  INF SecurityPkg/RandomNumberGenerator/RngDxe/RngDxe.inf
>>
>> Second:
>> What is the failure mode of running the BaseRngLib flavour on cpus 
>> that don't support FEAT_RNG? RngDxe itself seems to do the right 
>> thing, but do we get any warning messages or will certain operations 
>> now fail silently?
> 
> On FEAT_RNG cores we get:
> 
> InstallProtocolInterface: BC62157E-3E33-4FEC-9920-2D3B36D750DF 101FAD68798
> ProtectUefiImageCommon - 0xFAD683C0
>    - 0x00000101FBBDB000 - 0x0000000000007000
> ArmTrngLib could not be correctly initialized.
> InstallProtocolInterface: 3152BCA5-EADE-433D-862E-C01CDC291F44 101FBBE0020
> Loading driver B601F8C4-43B7-4784-95B1-F4226CB40CEE
> 
> 
> On core without FEAT_RNG:
> 
> InstallProtocolInterface: BC62157E-3E33-4FEC-9920-2D3B36D750DF 101FAD68798
> ProtectUefiImageCommon - 0xFAD683C0
>    - 0x00000101FBBDB000 - 0x0000000000007000
> ArmTrngLib could not be correctly initialized.
> Error: Image at 101FBBDB000 start failed: 00000001
> Loading driver B601F8C4-43B7-4784-95B1-F4226CB40CEE

But it still keeps booting correctly after that? With some missing 
functionality?

> So there is some kind of information but you need to know what
> to look for ;(

Ard: would you be opposed to putting a DEBUG print and/or an ASSERT in 
BaseRngLibContructor if mRndrSupported == 0?

An alternative would be to place a test and noisy warning inside 
SbsaQemuPlatformDxe.

/
     Leif



-=-=-=-=-=-=-=-=-=-=-=-
Groups.io Links: You receive all messages sent to this group.
View/Reply Online (#119737): https://edk2.groups.io/g/devel/message/119737
Mute This Topic: https://groups.io/mt/106909459/7686176
Group Owner: devel+owner@edk2.groups.io
Unsubscribe: https://edk2.groups.io/g/devel/unsub [rebecca@openfw.io]
-=-=-=-=-=-=-=-=-=-=-=-

Re: [edk2-devel] [PATCH edk2-platforms 1/1] SbsaQemu: use FEAT_RNG for EFI_RNG_PROTOCOL

[Ard Biesheuvel]

On Mon, 1 Jul 2024 at 15:28, Leif Lindholm <quic_llindhol@quicinc.com> wrote:
>
> On 2024-07-01 13:58, Marcin Juszkiewicz wrote:
>
> >>> diff --git a/Platform/Qemu/SbsaQemu/SbsaQemu.dsc
> >>> b/Platform/Qemu/SbsaQemu/SbsaQemu.dsc
> >>> index 9306986bf7c0..3463e5c7a635 100644
> >>> --- a/Platform/Qemu/SbsaQemu/SbsaQemu.dsc
> >>> +++ b/Platform/Qemu/SbsaQemu/SbsaQemu.dsc
> >>> @@ -148,7 +148,9 @@ [LibraryClasses.common]
> >>>     #
> >>
> >> Since sbsa-ref still supports processors without FEAT_RNG, this may
> >> cause unexpected breakages for some users.
> >
> > That's why I sent it as more of RFC than changes for merging.
> >
> >> Could we first of all conditionalise this change:
> >>
> >> [Defines]
> >> ...
> >>    DEFINE_DEBUG_PRINT_ERROR_LEVEL = ...
> >>    DEFINE FEATRNG_ENABLE         = TRUE
> >>
> >> so that someone who still wishes to run tests against older cpus can
> >> still do so through a rebuild with -D FEATRNG_ENABLE=FALSE
> >
> > Is there a way to load both BaseRngLib and BaseRngLibTimerLib and switch
> > between them depending on availability of FEAT_RNG?
>
> Not without severe hackery. The library is statically linked into RngDxe.
>
> >>> diff --git a/Platform/Qemu/SbsaQemu/SbsaQemu.fdf
> >>> b/Platform/Qemu/SbsaQemu/SbsaQemu.fdf
> >>> index b35f42e11aa4..51a1ef8519f9 100644
> >>> --- a/Platform/Qemu/SbsaQemu/SbsaQemu.fdf
> >>> +++ b/Platform/Qemu/SbsaQemu/SbsaQemu.fdf
> >>> @@ -192,6 +192,7 @@ [FV.FvMain]
> >>>     INF ArmPkg/Drivers/TimerDxe/TimerDxe.inf
> >>>     INF OvmfPkg/VirtNorFlashDxe/VirtNorFlashDxe.inf
> >>>     INF MdeModulePkg/Universal/WatchdogTimerDxe/WatchdogTimer.inf
> >>> +  INF SecurityPkg/RandomNumberGenerator/RngDxe/RngDxe.inf
> >>
> >> Second:
> >> What is the failure mode of running the BaseRngLib flavour on cpus
> >> that don't support FEAT_RNG? RngDxe itself seems to do the right
> >> thing, but do we get any warning messages or will certain operations
> >> now fail silently?
> >
> > On FEAT_RNG cores we get:
> >
> > InstallProtocolInterface: BC62157E-3E33-4FEC-9920-2D3B36D750DF 101FAD68798
> > ProtectUefiImageCommon - 0xFAD683C0
> >    - 0x00000101FBBDB000 - 0x0000000000007000
> > ArmTrngLib could not be correctly initialized.
> > InstallProtocolInterface: 3152BCA5-EADE-433D-862E-C01CDC291F44 101FBBE0020
> > Loading driver B601F8C4-43B7-4784-95B1-F4226CB40CEE
> >
> >
> > On core without FEAT_RNG:
> >
> > InstallProtocolInterface: BC62157E-3E33-4FEC-9920-2D3B36D750DF 101FAD68798
> > ProtectUefiImageCommon - 0xFAD683C0
> >    - 0x00000101FBBDB000 - 0x0000000000007000
> > ArmTrngLib could not be correctly initialized.
> > Error: Image at 101FBBDB000 start failed: 00000001
> > Loading driver B601F8C4-43B7-4784-95B1-F4226CB40CEE
>
> But it still keeps booting correctly after that? With some missing
> functionality?
>
> > So there is some kind of information but you need to know what
> > to look for ;(
>
> Ard: would you be opposed to putting a DEBUG print and/or an ASSERT in
> BaseRngLibContructor if mRndrSupported == 0?
>
> An alternative would be to place a test and noisy warning inside
> SbsaQemuPlatformDxe.
>

I'm not sure I follow what we are trying to fix here. RngDxe will
simply not load if BaseRngLib does not find FEAT_RNG support.

If the issue is other, existing users of RngLib that are currently
served by BaseRngTimerLib, we could make the library class resolutions
introduced here local the RngDxe driver, using

SecurityPkg/RandomNumberGenerator/RngDxe/RngDxe.inf {
  <LibraryClasses>
    RngLib|MdePkg/Library/BaseRngLib/BaseRngLib.inf
    ArmTrngLib|ArmPkg/Library/ArmTrngLib/ArmTrngLib.inf
    ArmMonitorLib|ArmPkg/Library/ArmMonitorLib/ArmMonitorLib.inf
}


-=-=-=-=-=-=-=-=-=-=-=-
Groups.io Links: You receive all messages sent to this group.
View/Reply Online (#119738): https://edk2.groups.io/g/devel/message/119738
Mute This Topic: https://groups.io/mt/106909459/7686176
Group Owner: devel+owner@edk2.groups.io
Unsubscribe: https://edk2.groups.io/g/devel/unsub [rebecca@openfw.io]
-=-=-=-=-=-=-=-=-=-=-=-

Re: [edk2-devel] [PATCH edk2-platforms 1/1] SbsaQemu: use FEAT_RNG for EFI_RNG_PROTOCOL

[Leif Lindholm]

On 2024-07-01 14:35, Ard Biesheuvel wrote:
>> Ard: would you be opposed to putting a DEBUG print and/or an ASSERT in
>> BaseRngLibContructor if mRndrSupported == 0?
>>
>> An alternative would be to place a test and noisy warning inside
>> SbsaQemuPlatformDxe.
> 
> I'm not sure I follow what we are trying to fix here. RngDxe will
> simply not load if BaseRngLib does not find FEAT_RNG support.
>
> If the issue is other, existing users of RngLib that are currently
> served by BaseRngTimerLib, we could make the library class resolutions
> introduced here local the RngDxe driver, using
> 
> SecurityPkg/RandomNumberGenerator/RngDxe/RngDxe.inf {
>    <LibraryClasses>
>      RngLib|MdePkg/Library/BaseRngLib/BaseRngLib.inf
>      ArmTrngLib|ArmPkg/Library/ArmTrngLib/ArmTrngLib.inf
>      ArmMonitorLib|ArmPkg/Library/ArmMonitorLib/ArmMonitorLib.inf
> }

Good point.

I think Ard's example solves the original purpose (providing 
EFI_RNG_PROTOCOL to linux efi stub) while not breaking any potential 
usage before this point.

I'd still quite like to nudge existing users onto a more recent cpu,
with less cryptic console output. But if we do the above we can drop the 
conditional part and just put a test and a message in the PlatformDxe.

(And have the commit message be clear on RngDxe being added.)

/
     Leif



-=-=-=-=-=-=-=-=-=-=-=-
Groups.io Links: You receive all messages sent to this group.
View/Reply Online (#119741): https://edk2.groups.io/g/devel/message/119741
Mute This Topic: https://groups.io/mt/106909459/7686176
Group Owner: devel+owner@edk2.groups.io
Unsubscribe: https://edk2.groups.io/g/devel/unsub [rebecca@openfw.io]
-=-=-=-=-=-=-=-=-=-=-=-