On 06/23/20 19:57, Ard Biesheuvel wrote:
> Our UEFI guest firmware takes ownership of the emulated NOR flash in
> order to support the variable runtime services, and it does not expect
> the OS to interfere with the underlying storage directly. So disable
> the NOR flash DT nodes as we discover them, in a way similar to how we
> disable the PL031 RTC in the device tree when we attach our RTC runtime
> driver to it.
>
> Signed-off-by: Ard Biesheuvel <ard.biesheu...@arm.com>
> ---
> ArmVirtPkg/Library/NorFlashQemuLib/NorFlashQemuLib.c | 12 ++++++++++++
> 1 file changed, 12 insertions(+)
>
> diff --git a/ArmVirtPkg/Library/NorFlashQemuLib/NorFlashQemuLib.c
> b/ArmVirtPkg/Library/NorFlashQemuLib/NorFlashQemuLib.c
> index 9b1d1184bdd3..c676039785be 100644
> --- a/ArmVirtPkg/Library/NorFlashQemuLib/NorFlashQemuLib.c
> +++ b/ArmVirtPkg/Library/NorFlashQemuLib/NorFlashQemuLib.c
> @@ -86,6 +86,18 @@ NorFlashPlatformGetDevices (
> mNorFlashDevices[Num].BlockSize = QEMU_NOR_BLOCK_SIZE;
> Num++;
> }
> +
> + //
> + // UEFI takes ownership of the NOR flash, and exposes its functionality
> + // through the UEFI Runtime Services GetVariable, SetVariable, etc. This
> + // means we need to disable it in the device tree to prevent the OS from
> + // attaching its device driver as well.
> + //
> + Status = FdtClient->SetNodeProperty (FdtClient, Node, "status",
> + "disabled", sizeof ("disabled"));
> + if (EFI_ERROR (Status)) {
> + DEBUG ((EFI_D_WARN, "Failed to set NOR flash status to
> 'disabled'\n"));
> + }
> }
>
> *NorFlashDescriptions = mNorFlashDevices;
>
Higher up we have (in the inner loop):
> //
> // Disregard any flash devices that overlap with the primary FV.
> // The firmware is not updatable from inside the guest anyway.
> //
> if ((PcdGet64 (PcdFvBaseAddress) + PcdGet32 (PcdFvSize) > Base) &&
> (Base + Size) > PcdGet64 (PcdFvBaseAddress)) {
> continue;
> }
(1) If we never append any (Base, Size) "reg" pair to "mNorFlashDevices"
for a particular cfi-flash node, should we still "own" that node?
How about something like (on top of your patch):
> diff --git a/ArmVirtPkg/Library/NorFlashQemuLib/NorFlashQemuLib.c
> b/ArmVirtPkg/Library/NorFlashQemuLib/NorFlashQemuLib.c
> index c676039785be..d063d69580e5 100644
> --- a/ArmVirtPkg/Library/NorFlashQemuLib/NorFlashQemuLib.c
> +++ b/ArmVirtPkg/Library/NorFlashQemuLib/NorFlashQemuLib.c
> @@ -42,6 +42,7 @@ NorFlashPlatformGetDevices (
> UINT32 Num;
> UINT64 Base;
> UINT64 Size;
> + BOOLEAN FirmwareOwned;
>
> Status = gBS->LocateProtocol (&gFdtClientProtocolGuid, NULL,
> (VOID **)&FdtClient);
> @@ -64,6 +65,7 @@ NorFlashPlatformGetDevices (
>
> ASSERT ((PropSize % (4 * sizeof (UINT32))) == 0);
>
> + FirmwareOwned = FALSE;
> while (PropSize >= (4 * sizeof (UINT32)) && Num < MAX_FLASH_BANKS) {
> Base = SwapBytes64 (ReadUnaligned64 ((VOID *)&Reg[0]));
> Size = SwapBytes64 (ReadUnaligned64 ((VOID *)&Reg[2]));
> @@ -80,6 +82,7 @@ NorFlashPlatformGetDevices (
> continue;
> }
>
> + FirmwareOwned = TRUE;
> mNorFlashDevices[Num].DeviceBaseAddress = (UINTN)Base;
> mNorFlashDevices[Num].RegionBaseAddress = (UINTN)Base;
> mNorFlashDevices[Num].Size = (UINTN)Size;
> @@ -87,6 +90,10 @@ NorFlashPlatformGetDevices (
> Num++;
> }
>
> + if (!FirmwareOwned) {
> + continue;
> + }
> +
> //
> // UEFI takes ownership of the NOR flash, and exposes its functionality
> // through the UEFI Runtime Services GetVariable, SetVariable, etc. This
(2) If this makes no difference in practice, then I'm fine with the
patch as posted, too:
Reviewed-by: Laszlo Ersek <ler...@redhat.com>
Just wanted to raise the question.
(3) Hm... if we *deliberately* want to prevent the OS from attaching its
flash driver to the "code" flash chip too, then the logic is good as
posted, of course; but in that case, should the comment perhaps be more
generic than "UEFI Runtime Services GetVariable, SetVariable"? Because
then we "disable" flash nodes in the DT for two reasons: (a) varstore,
(b) fw executable.
If this is the case, then with a comment / commit message update:
Reviewed-by: Laszlo Ersek <ler...@redhat.com>
(4) Is there a particular guest kernel commit that exposes the issue? Or
maybe a CONFIG knob? Can we mention whichever applies, in the commit
message?
Thanks!
Laszlo
-=-=-=-=-=-=-=-=-=-=-=-
Groups.io Links: You receive all messages sent to this group.
View/Reply Online (#61618): https://edk2.groups.io/g/devel/message/61618
Mute This Topic: https://groups.io/mt/75065345/21656
Group Owner: devel+ow...@edk2.groups.io
Unsubscribe: https://edk2.groups.io/g/devel/unsub [arch...@mail-archive.com]
-=-=-=-=-=-=-=-=-=-=-=-
