From mboxrd@z Thu Jan 1 00:00:00 1970 Received: from us-smtp-1.mimecast.com (us-smtp-1.mimecast.com [205.139.110.120]) by mx.groups.io with SMTP id smtpd.web10.1835.1593024467409819858 for ; Wed, 24 Jun 2020 11:47:47 -0700 Authentication-Results: mx.groups.io; dkim=pass header.i=@redhat.com header.s=mimecast20190719 header.b=hTYGZKoi; spf=pass (domain: redhat.com, ip: 205.139.110.120, mailfrom: lersek@redhat.com) DKIM-Signature: v=1; a=rsa-sha256; c=relaxed/relaxed; d=redhat.com; s=mimecast20190719; t=1593024466; 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=flbluDbqZT+Ix9mHfyGANQx92pC1XQsWCa8MYFJD0bo=; b=hTYGZKoip112bd6UHf7PMAugT6nuSZ/tofp/apa28V2dr2dmeHRfUTqlOWdZKyevFESvM4 SDIlGT8FkNZ+u+UktfH3UiVxbBjreWPrVeXVdHpJBF28htky3zrKEjZADUtBPlJr4tGWSZ yvWbhnHZpqOsBAr6KFEPaV9xumSQlX4= 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-128-i4MaXgQDP6-bggQMXWeAqw-1; Wed, 24 Jun 2020 14:47:44 -0400 X-MC-Unique: i4MaXgQDP6-bggQMXWeAqw-1 Received: from smtp.corp.redhat.com (int-mx03.intmail.prod.int.phx2.redhat.com [10.5.11.13]) (using TLSv1.2 with cipher AECDH-AES256-SHA (256/256 bits)) (No client certificate requested) by mimecast-mx01.redhat.com (Postfix) with ESMTPS id B6A8A18585A1; Wed, 24 Jun 2020 18:47:43 +0000 (UTC) Received: from lacos-laptop-7.usersys.redhat.com (ovpn-112-115.ams2.redhat.com [10.36.112.115]) by smtp.corp.redhat.com (Postfix) with ESMTP id BC6A38929D; Wed, 24 Jun 2020 18:47:42 +0000 (UTC) Subject: Re: [PATCH v2] ArmVirtPkg/NorFlashQemuLib: disable NOR flash DT nodes upon discovery To: Ard Biesheuvel , devel@edk2.groups.io Cc: philmd@redhat.com References: <20200624111524.1588197-1-ard.biesheuvel@arm.com> <18682fea-78cd-ab6f-c2f5-4d9b9fbdaac0@redhat.com> From: "Laszlo Ersek" Message-ID: Date: Wed, 24 Jun 2020 20:47:41 +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: X-Scanned-By: MIMEDefang 2.79 on 10.5.11.13 Authentication-Results: relay.mimecast.com; auth=pass smtp.auth=CUSA124A263 smtp.mailfrom=lersek@redhat.com 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 14:19, 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 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. >>> >>> 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 flash >>> 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", >>> 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 >>> --- >>>   ArmVirtPkg/Library/NorFlashQemuLib/NorFlashQemuLib.c | 16 >>> ++++++++++++++++ >>>   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 ( >>>         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. >>> +    // Note that this also hides other flash banks, but the only >>> other flash >>> +    // bank we expect to encounter is the one that carries the UEFI >>> executable >>> +    // code, which is not intended to be guest updatable, and is >>> usually backed >>> +    // in a readonly manner by QEMU anyway. >>> +    // >>> +    Status = FdtClient->SetNodeProperty (FdtClient, Node, "status", >>> +                          "disabled", sizeof ("disabled")); >>> +    if (EFI_ERROR (Status)) { >>> +        DEBUG ((DEBUG_WARN, "Failed to set NOR flash status to >>> 'disabled'\n")); >>> +    } >>>     } >>>       *NorFlashDescriptions = mNorFlashDevices; >>> > > Just noticed that both flash banks are actually emitted as a single DT > node, i.e. > > flash@0 { >     compatible = "cfi-flash"; >     reg = < 0x00 0x00 0x00 0x4000000 0x00 0x4000000 0x00 0x4000000 >;│ >     bank-width = < 0x04 >; > } > > so in our case, the only straight-forward way to disable one of them is > to disable all of them (unless we want to poke around in the 'reg' > property). > > This doesn't really make a difference for the reasoning above, so I > propose to apply the patch as is. > > I'm OK with that! Thanks Laszlo