From mboxrd@z Thu Jan 1 00:00:00 1970 Received: from us-smtp-delivery-1.mimecast.com (us-smtp-delivery-1.mimecast.com [205.139.110.61]) by mx.groups.io with SMTP id smtpd.web11.16674.1592944924798249620 for ; Tue, 23 Jun 2020 13:42:05 -0700 Authentication-Results: mx.groups.io; dkim=pass header.i=@redhat.com header.s=mimecast20190719 header.b=Wuj8J0jd; spf=pass (domain: redhat.com, ip: 205.139.110.61, mailfrom: lersek@redhat.com) DKIM-Signature: v=1; a=rsa-sha256; c=relaxed/relaxed; d=redhat.com; s=mimecast20190719; t=1592944923; 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=zLH9mCpWnbLrxkgTR674OFqDkhwwa1UvtIuSC6Hix/k=; b=Wuj8J0jdrVI5CGgiIbHum0B/zpT8wPl/phn1w1Dt0uSxGMNi7zCidGiwtfgqajHUxa9Bm3 Zuj3BwfRa0njJhAsP3M8znWDGNyrtR6huBq3q4gnviz6oph/wSIbh+qKgKQbnqzzYhznbE QCJxnFoTo6akJsSt5b+pViwrCHR3ZTM= 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-390-UJr_XhwGPbWUjiH8xYLddQ-1; Tue, 23 Jun 2020 16:42:02 -0400 X-MC-Unique: UJr_XhwGPbWUjiH8xYLddQ-1 Received: from smtp.corp.redhat.com (int-mx07.intmail.prod.int.phx2.redhat.com [10.5.11.22]) (using TLSv1.2 with cipher AECDH-AES256-SHA (256/256 bits)) (No client certificate requested) by mimecast-mx01.redhat.com (Postfix) with ESMTPS id 1749F83DE2D; Tue, 23 Jun 2020 20:42:01 +0000 (UTC) Received: from lacos-laptop-7.usersys.redhat.com (ovpn-112-76.ams2.redhat.com [10.36.112.76]) by smtp.corp.redhat.com (Postfix) with ESMTP id 160F710013C0; Tue, 23 Jun 2020 20:41:59 +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> From: "Laszlo Ersek" Message-ID: Date: Tue, 23 Jun 2020 22:41:58 +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: <20200623175700.1564281-1-ard.biesheuvel@arm.com> X-Scanned-By: MIMEDefang 2.84 on 10.5.11.22 X-Mimecast-Spam-Score: 0 X-Mimecast-Originator: redhat.com Content-Language: en-US Content-Type: text/plain; charset=utf-8 Content-Transfer-Encoding: 7bit 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? 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 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? Thanks! Laszlo