On 6/24/20 3:15 PM, Philippe Mathieu-Daudé wrote:
On 6/24/20 2:19 PM, Ard Biesheuvel wrote:
On 6/24/20 1:48 PM, Laszlo Ersek wrote:
On 06/24/20 13:15, 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.
Note that this also hides the NOR flash bank that carries the UEFI
executable code, but this is not intended to be updatable from inside
the guest anyway, and if it was, we should use capsule update to do so.
Also, the first -pflash argument that defines the backing for this flash
bank is often issued with the 'readonly' modifier, in order to prevent
any changes whatsoever to be made to the executable firmware image by
the guest.
This issue has become relevant due to the following Linux changes,
which enable the flash driver stack for default build configurations
targetting arm64 and 32-bit ARM.
ce693fc2a877
("arm64: defconfig: Enable flash device drivers for QorIQ boards",
2020-03-16).
5f068190cc10
("ARM: multi_v7_defconfig: Enable support for CFI NOR FLASH",
2019-04-03)
Reviewed-by: Laszlo Ersek <ler...@redhat.com>
Reviewed-by: Philippe Mathieu-Daude <phi...@redhat.com>
Signed-off-by: Ard Biesheuvel <ard.biesheu...@arm.com>
---
ArmVirtPkg/Library/NorFlashQemuLib/NorFlashQemuLib.c | 16
++++++++++++++++
1 file changed, 16 insertions(+)
diff --git a/ArmVirtPkg/Library/NorFlashQemuLib/NorFlashQemuLib.c
b/ArmVirtPkg/Library/NorFlashQemuLib/NorFlashQemuLib.c
index 9b1d1184bdd3..425e36f2d127 100644
--- a/ArmVirtPkg/Library/NorFlashQemuLib/NorFlashQemuLib.c
+++ b/ArmVirtPkg/Library/NorFlashQemuLib/NorFlashQemuLib.c
@@ -86,6 +86,22 @@ 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.
+ // Note that this also hides other flash banks, but the only
other flash
+ // bank we expect to encounter is the one that carries the UEFI
executable
+ // code, which is not intended to be guest updatable, and is
usually backed
+ // in a readonly manner by QEMU anyway.
+ //
+ Status = FdtClient->SetNodeProperty (FdtClient, Node, "status",
+ "disabled", sizeof ("disabled"));
+ if (EFI_ERROR (Status)) {
+ DEBUG ((DEBUG_WARN, "Failed to set NOR flash status to
'disabled'\n"));
+ }
}
*NorFlashDescriptions = mNorFlashDevices;
Just noticed that both flash banks are actually emitted as a single DT
node, i.e.
flash@0 {
compatible = "cfi-flash";
reg = < 0x00 0x00 0x00 0x4000000 0x00 0x4000000 0x00 0x4000000 >;│
bank-width = < 0x04 >;
}
IIRC the idea was to use protected banks, but QEMU doesn't model them,
so it was easier to use 1 ROM and 1 FLASH, but we accidentally ended
using 2 FLASH (the CODE one being 'read-only').
In one of the first Tianocore call I participated, someone from Intel
said they like the idea of having it FLASH and not ROM so they could
test the Capsule Update feature when QEMU support multiple banks &
locking.
The QEMU community is reluctant to change the pflash device as "it
just works for our needs".
I wonder how this DT node is consumed on the kernel side.
Ah, I suppose it trust the firmware, and doesn't CFI-query the flash
to verify its size. This is certainly fragile...
Is it? The reg property contains two (base, size) tuples, and the driver
seems to treat them as two individual flash chips.
-=-=-=-=-=-=-=-=-=-=-=-
Groups.io Links: You receive all messages sent to this group.
View/Reply Online (#61686): https://edk2.groups.io/g/devel/message/61686
Mute This Topic: https://groups.io/mt/75079347/21656
Group Owner: devel+ow...@edk2.groups.io
Unsubscribe: https://edk2.groups.io/g/devel/unsub [arch...@mail-archive.com]
-=-=-=-=-=-=-=-=-=-=-=-
