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

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

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. 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) but there is no good 
reason to keep supporting this.