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...
>
> so in our case, the only straight-forward way to disable one of them is
> to disable all of them (unless we want to poke around in the 'reg'
> property).
>
> This doesn't really make a difference for the reasoning above, so I
> propose to apply the patch as is.
>
>
-=-=-=-=-=-=-=-=-=-=-=-
Groups.io Links: You receive all messages sent to this group.
View/Reply Online (#61660): https://edk2.groups.io/g/devel/message/61660
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]
-=-=-=-=-=-=-=-=-=-=-=-
