Re: [PATCH v2] ArmVirtPkg/NorFlashQemuLib: disable NOR flash DT nodes upon discovery

["Ard Biesheuvel" <ard.biesheuvel@arm.com>]

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 <lersek@redhat.com>
>>>> Reviewed-by: Philippe Mathieu-Daude <philmd@redhat.com>
>>>> Signed-off-by: Ard Biesheuvel <ard.biesheuvel@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.