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.web11.15825.1593011680673779082 for ; Wed, 24 Jun 2020 08:14: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 9B6B91FB; Wed, 24 Jun 2020 08:14: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 D957C3F73C; Wed, 24 Jun 2020 08:14:38 -0700 (PDT) Subject: Re: [PATCH v2] ArmVirtPkg/NorFlashQemuLib: disable NOR flash DT nodes upon discovery To: =?UTF-8?Q?Philippe_Mathieu-Daud=c3=a9?= , Laszlo Ersek , devel@edk2.groups.io References: <20200624111524.1588197-1-ard.biesheuvel@arm.com> <18682fea-78cd-ab6f-c2f5-4d9b9fbdaac0@redhat.com> From: "Ard Biesheuvel" Message-ID: <9bd093b9-5e39-923d-5a4f-3f5593388192@arm.com> Date: Wed, 24 Jun 2020 17:14:36 +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: quoted-printable On 6/24/20 3:15 PM, Philippe Mathieu-Daud=C3=A9 wrote: > On 6/24/20 2:19 PM, Ard Biesheuvel wrote: >> 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 expe= ct >>>> 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 runt= ime >>>> 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 insid= e >>>> 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 f= lash >>>> bank is often issued with the 'readonly' modifier, in order to preve= nt >>>> any changes whatsoever to be made to the executable firmware image b= y >>>> 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", >>>> 2020-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 >>>> --- >>>> =C2=A0 ArmVirtPkg/Library/NorFlashQemuLib/NorFlashQemuLib.c | 16 >>>> ++++++++++++++++ >>>> =C2=A0 1 file changed, 16 insertions(+) >>>> >>>> diff --git a/ArmVirtPkg/Library/NorFlashQemuLib/NorFlashQemuLib.c >>>> b/ArmVirtPkg/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 ( >>>> =C2=A0=C2=A0=C2=A0=C2=A0=C2=A0=C2=A0=C2=A0 mNorFlashDevices[Num].Bl= ockSize=C2=A0=C2=A0=C2=A0=C2=A0=C2=A0=C2=A0=C2=A0=C2=A0 =3D QEMU_NOR_BLOC= K_SIZE; >>>> =C2=A0=C2=A0=C2=A0=C2=A0=C2=A0=C2=A0=C2=A0 Num++; >>>> =C2=A0=C2=A0=C2=A0=C2=A0=C2=A0 } >>>> + >>>> +=C2=A0=C2=A0=C2=A0 // >>>> +=C2=A0=C2=A0=C2=A0 // UEFI takes ownership of the NOR flash, and ex= poses its >>>> functionality >>>> +=C2=A0=C2=A0=C2=A0 // through the UEFI Runtime Services GetVariable= , SetVariable, >>>> etc. This >>>> +=C2=A0=C2=A0=C2=A0 // means we need to disable it in the device tre= e to prevent the >>>> OS from >>>> +=C2=A0=C2=A0=C2=A0 // attaching its device driver as well. >>>> +=C2=A0=C2=A0=C2=A0 // Note that this also hides other flash banks, = but the only >>>> other flash >>>> +=C2=A0=C2=A0=C2=A0 // bank we expect to encounter is the one that c= arries the UEFI >>>> executable >>>> +=C2=A0=C2=A0=C2=A0 // code, which is not intended to be guest updat= able, and is >>>> usually backed >>>> +=C2=A0=C2=A0=C2=A0 // in a readonly manner by QEMU anyway. >>>> +=C2=A0=C2=A0=C2=A0 // >>>> +=C2=A0=C2=A0=C2=A0 Status =3D FdtClient->SetNodeProperty (FdtClient= , Node, "status", >>>> +=C2=A0=C2=A0=C2=A0=C2=A0=C2=A0=C2=A0=C2=A0=C2=A0=C2=A0=C2=A0=C2=A0=C2= =A0=C2=A0=C2=A0=C2=A0=C2=A0=C2=A0=C2=A0=C2=A0=C2=A0=C2=A0=C2=A0=C2=A0=C2=A0= =C2=A0 "disabled", sizeof ("disabled")); >>>> +=C2=A0=C2=A0=C2=A0 if (EFI_ERROR (Status)) { >>>> +=C2=A0=C2=A0=C2=A0=C2=A0=C2=A0=C2=A0=C2=A0 DEBUG ((DEBUG_WARN, "Fai= led to set NOR flash status to >>>> 'disabled'\n")); >>>> +=C2=A0=C2=A0=C2=A0 } >>>> =C2=A0=C2=A0=C2=A0 } >>>> =C2=A0 =C2=A0=C2=A0=C2=A0 *NorFlashDescriptions =3D mNorFlashDevice= s; >>>> >> >> Just noticed that both flash banks are actually emitted as a single DT >> node, i.e. >> >> flash@0 { >> =C2=A0=C2=A0=C2=A0 compatible =3D "cfi-flash"; >> =C2=A0=C2=A0=C2=A0 reg =3D < 0x00 0x00 0x00 0x4000000 0x00 0x4000000 = 0x00 0x4000000 >;=E2=94=82 >> =C2=A0=C2=A0=C2=A0 bank-width =3D < 0x04 >; >> } >=20 > IIRC the idea was to use protected banks, but QEMU doesn't model them, > so it was easier to use 1 ROM and 1 FLASH, but we accidentally ended > using 2 FLASH (the CODE one being 'read-only'). >=20 > In one of the first Tianocore call I participated, someone from Intel > said they like the idea of having it FLASH and not ROM so they could > test the Capsule Update feature when QEMU support multiple banks & > locking. > The QEMU community is reluctant to change the pflash device as "it > just works for our needs". >=20 > I wonder how this DT node is consumed on the kernel side. > Ah, I suppose it trust the firmware, and doesn't CFI-query the flash > to verify its size. This is certainly fragile... >=20 Is it? The reg property contains two (base, size) tuples, and the driver=20 seems to treat them as two individual flash chips.