From mboxrd@z Thu Jan 1 00:00:00 1970 Received: from us-smtp-delivery-1.mimecast.com (us-smtp-delivery-1.mimecast.com [207.211.31.81]) by mx.groups.io with SMTP id smtpd.web10.9535.1592989237913229345 for ; Wed, 24 Jun 2020 02:00:38 -0700 Authentication-Results: mx.groups.io; dkim=pass header.i=@redhat.com header.s=mimecast20190719 header.b=A3KqRHC3; spf=pass (domain: redhat.com, ip: 207.211.31.81, mailfrom: lersek@redhat.com) DKIM-Signature: v=1; a=rsa-sha256; c=relaxed/relaxed; d=redhat.com; s=mimecast20190719; t=1592989237; h=from:from:reply-to:subject:subject:date:date:message-id:message-id: to:to:cc:cc:mime-version:mime-version:content-type:content-type: content-transfer-encoding:content-transfer-encoding: in-reply-to:in-reply-to:references:references; bh=R2QHW4nmPJkE4KEwAk0Hp/b12m90Oz+0l0Ojax9bLic=; b=A3KqRHC3eTT8e4mEAhhWiGQfZ5gMrrqWD+b88Lzh03gUdkB/TrpT/rvz+jCYdFnBAC/tFV fw5DTFoKJ0+oX3uFRScxVZWOQY/aZbFiO7LEhDH75ziVNbQOxHJvk+zLkKoLKCqPPOLEye HQfs1Y+T726bQFy4rNI/8fTvT+5EUOc= Received: from mimecast-mx01.redhat.com (mimecast-mx01.redhat.com [209.132.183.4]) (Using TLS) by relay.mimecast.com with ESMTP id us-mta-197-Q2eWgUDEPmO6AriRfTgn7w-1; Wed, 24 Jun 2020 05:00:35 -0400 X-MC-Unique: Q2eWgUDEPmO6AriRfTgn7w-1 Received: from smtp.corp.redhat.com (int-mx02.intmail.prod.int.phx2.redhat.com [10.5.11.12]) (using TLSv1.2 with cipher AECDH-AES256-SHA (256/256 bits)) (No client certificate requested) by mimecast-mx01.redhat.com (Postfix) with ESMTPS id 2550C805EF6; Wed, 24 Jun 2020 09:00:34 +0000 (UTC) Received: from lacos-laptop-7.usersys.redhat.com (ovpn-112-56.ams2.redhat.com [10.36.112.56]) by smtp.corp.redhat.com (Postfix) with ESMTP id 295DF61169; Wed, 24 Jun 2020 09:00:32 +0000 (UTC) Subject: Re: [PATCH] ArmVirtPkg/NorFlashQemuLib: disable NOR flash DT nodes upon discovery To: Ard Biesheuvel , devel@edk2.groups.io Cc: philmd@redhat.com References: <20200623175700.1564281-1-ard.biesheuvel@arm.com> <85a63fc4-f3d1-1e17-bf1d-dace59bb02a8@arm.com> From: "Laszlo Ersek" Message-ID: <6cc28940-abb6-fce8-b24b-92a1bf78f14d@redhat.com> Date: Wed, 24 Jun 2020 11:00:32 +0200 User-Agent: Mozilla/5.0 (X11; Linux x86_64; rv:52.0) Gecko/20100101 Firefox/52.0 Thunderbird/52.9.1 MIME-Version: 1.0 In-Reply-To: <85a63fc4-f3d1-1e17-bf1d-dace59bb02a8@arm.com> X-Scanned-By: MIMEDefang 2.79 on 10.5.11.12 X-Mimecast-Spam-Score: 0 X-Mimecast-Originator: redhat.com Content-Language: en-US Content-Type: text/plain; charset=utf-8 Content-Transfer-Encoding: 8bit On 06/24/20 09:19, Ard Biesheuvel wrote: > 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. That seems to suggest commit ce693fc2a877 ("arm64: defconfig: Enable flash device drivers for QorIQ boards", 2020-03-16). > 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) Seems to come from commit 5f068190cc10 ("ARM: multi_v7_defconfig: Enable support for CFI NOR FLASH", 2019-04-03) -- also quite recent. > but there is no good reason to keep supporting this. I agree -- I'm asking because backporting your edk2 patch to downstreams could depend on the presence of these kernel commits in the guests those downstreams support. So mentioning the kernel commits can help downstreams evaluate the edk2 backport. Thanks! Laszlo