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.12275.1593001211087435436 for ; Wed, 24 Jun 2020 05:20:11 -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 997FB1F1; Wed, 24 Jun 2020 05:20:09 -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 CDCCF3F6CF; Wed, 24 Jun 2020 05:20:08 -0700 (PDT) Subject: Re: [PATCH v2] ArmVirtPkg/NorFlashQemuLib: disable NOR flash DT nodes upon discovery To: Laszlo Ersek , devel@edk2.groups.io Cc: philmd@redhat.com References: <20200624111524.1588197-1-ard.biesheuvel@arm.com> <18682fea-78cd-ab6f-c2f5-4d9b9fbdaac0@redhat.com> From: "Ard Biesheuvel" Message-ID: Date: Wed, 24 Jun 2020 14:19:59 +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: <18682fea-78cd-ab6f-c2f5-4d9b9fbdaac0@redhat.com> Content-Type: text/plain; charset=utf-8; format=flowed Content-Language: en-US Content-Transfer-Encoding: quoted-printable 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 runtim= e >> 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 fla= sh >> 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", 202= 0-03-16). >> >> 5f068190cc10 >> ("ARM: multi_v7_defconfig: Enable support for CFI NOR FLASH", 2019-04-= 03) >> >> Reviewed-by: Laszlo Ersek >> Reviewed-by: Philippe Mathieu-Daude >> Signed-off-by: Ard Biesheuvel >> --- >> ArmVirtPkg/Library/NorFlashQemuLib/NorFlashQemuLib.c | 16 ++++++++++= ++++++ >> 1 file changed, 16 insertions(+) >> >> diff --git a/ArmVirtPkg/Library/NorFlashQemuLib/NorFlashQemuLib.c b/Ar= mVirtPkg/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 =3D QEMU_NOR_BLOCK_SIZ= E; >> Num++; >> } >> + >> + // >> + // UEFI takes ownership of the NOR flash, and exposes its functio= nality >> + // through the UEFI Runtime Services GetVariable, SetVariable, et= c. 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 othe= r flash >> + // bank we expect to encounter is the one that carries the UEFI e= xecutable >> + // code, which is not intended to be guest updatable, and is usua= lly backed >> + // in a readonly manner by QEMU anyway. >> + // >> + Status =3D FdtClient->SetNodeProperty (FdtClient, Node, "status", >> + "disabled", sizeof ("disabled")); >> + if (EFI_ERROR (Status)) { >> + DEBUG ((DEBUG_WARN, "Failed to set NOR flash status to 'disab= led'\n")); >> + } >> } >> =20 >> *NorFlashDescriptions =3D mNorFlashDevices; >> Just noticed that both flash banks are actually emitted as a single DT=20 node, i.e. flash@0 { compatible =3D "cfi-flash"; reg =3D < 0x00 0x00 0x00 0x4000000 0x00 0x4000000 0x00 0x4000000 >;=E2= =94=82 bank-width =3D < 0x04 >; } so in our case, the only straight-forward way to disable one of them is=20 to disable all of them (unless we want to poke around in the 'reg'=20 property). This doesn't really make a difference for the reasoning above, so I=20 propose to apply the patch as is.