[edk2-devel] [PATCH v2] ArmVirtPkg/NorFlashQemuLib: disable NOR flash DT nodes upon discovery

[Philippe Mathieu-Daudé]

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 <lersek at redhat.com>
>>> Reviewed-by: Philippe Mathieu-Daude <philmd at redhat.com>
>>> Signed-off-by: Ard Biesheuvel <ard.biesheuvel at 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 at 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/1813853
Group Owner: devel+owner at edk2.groups.io
Unsubscribe: https://edk2.groups.io/g/devel/unsub  [edk2-devel-archive at redhat.com]
-=-=-=-=-=-=-=-=-=-=-=-