From mboxrd@z Thu Jan 1 00:00:00 1970 Received: from foss.arm.com (foss.arm.com [217.140.110.172]) by mx.groups.io with SMTP id smtpd.web12.8507.1592983180800731004 for ; Wed, 24 Jun 2020 00:19:41 -0700 Authentication-Results: mx.groups.io; dkim=missing; spf=pass (domain: arm.com, ip: 217.140.110.172, mailfrom: ard.biesheuvel@arm.com) Received: from usa-sjc-imap-foss1.foss.arm.com (unknown [10.121.207.14]) by usa-sjc-mx-foss1.foss.arm.com (Postfix) with ESMTP id 470A21F1; Wed, 24 Jun 2020 00:19:39 -0700 (PDT) Received: from [192.168.1.69] (unknown [10.37.8.29]) by usa-sjc-imap-foss1.foss.arm.com (Postfix) with ESMTPSA id 8486A3F73C; Wed, 24 Jun 2020 00:19:38 -0700 (PDT) Subject: Re: [PATCH] ArmVirtPkg/NorFlashQemuLib: disable NOR flash DT nodes upon discovery To: Laszlo Ersek , devel@edk2.groups.io Cc: philmd@redhat.com References: <20200623175700.1564281-1-ard.biesheuvel@arm.com> From: "Ard Biesheuvel" Message-ID: <85a63fc4-f3d1-1e17-bf1d-dace59bb02a8@arm.com> Date: Wed, 24 Jun 2020 09:19:25 +0200 User-Agent: Mozilla/5.0 (X11; Linux aarch64; rv:68.0) Gecko/20100101 Thunderbird/68.9.0 MIME-Version: 1.0 In-Reply-To: Content-Type: text/plain; charset=utf-8; format=flowed Content-Language: en-US Content-Transfer-Encoding: 7bit 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 >> --- >> 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 > 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 > > > (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.