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

["Laszlo Ersek" <lersek@redhat.com>]

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?

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>

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?

Thanks!
Laszlo