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

["Philippe Mathieu-Daudé" <philmd@redhat.com>]

On 6/24/20 11:00 AM, Laszlo Ersek wrote:
> On 06/24/20 09:19, Ard Biesheuvel wrote:
>> On 6/23/20 10:41 PM, Laszlo Ersek wrote:
>>> 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.biesheuvel@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?
>>>
>>
>> It depends. In practice, we will always have two, of which one needs to
>> be protected, and the other one is often backed in readonly mode, and so
>> all the guest can do is read or execute from it.
>>
>> So we might expose the FW one, as it would not interfere with the
>> variable runtime services, but it is not that useful imo.
>>
>>> 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 <lersek@redhat.com>
>>>
>>
>> Thanks
>>
>>> 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 <lersek@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?
>>>
>>
>> The arm64 defconfig was recently updated with MTD support, and so the
>> flash banks are now claimed by the CFI flash driver.
> 
> That seems to suggest commit ce693fc2a877 ("arm64: defconfig: Enable
> flash device drivers for QorIQ boards", 2020-03-16).
> 
>> I saw the same on
>> 32-bit ARM today, and I am not sure if it is a change there or whether
>> it was always like that (for multi_v7_defconfig)
> 
> Seems to come from commit 5f068190cc10 ("ARM: multi_v7_defconfig: Enable
> support for CFI NOR FLASH", 2019-04-03) -- also quite recent.
> 
>> but there is no good reason to keep supporting this.

Reviewed-by: Philippe Mathieu-Daude <philmd@redhat.com>

> 
> I agree -- I'm asking because backporting your edk2 patch to downstreams
> could depend on the presence of these kernel commits in the guests those
> downstreams support. So mentioning the kernel commits can help
> downstreams evaluate the edk2 backport.
> 
> Thanks!
> Laszlo
>