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.web10.13165.1593004554356822158 for ; Wed, 24 Jun 2020 06:15:54 -0700 Authentication-Results: mx.groups.io; dkim=pass header.i=@redhat.com header.s=mimecast20190719 header.b=MIBbA3tQ; spf=pass (domain: redhat.com, ip: 205.139.110.61, mailfrom: philmd@redhat.com) DKIM-Signature: v=1; a=rsa-sha256; c=relaxed/relaxed; d=redhat.com; s=mimecast20190719; t=1593004553; h=from:from:reply-to:subject:subject:date:date:message-id:message-id: to:to:cc:mime-version:mime-version:content-type:content-type: content-transfer-encoding:content-transfer-encoding: in-reply-to:in-reply-to:references:references:autocrypt:autocrypt; bh=7oBkdchRO8S+TG0SdV7obWRvQ4H4ZUW9GVrsi5Flg8U=; b=MIBbA3tQDd0qtTN9raDpch8rk2lnmaXJhZdakwYUkpX0qA6u6F1mfES/SQoF4FLrpqDwvI 1gjO+MW9NqxPFU7nk4k/9bnYFNg5mCLjENqgK0mvhL0sfZUR3ZCIZt/cIQYTwy5GNOZhRD G0EkHGiYZc0UpMTz6C6vsTfypW27BEI= Received: from mail-wm1-f69.google.com (mail-wm1-f69.google.com [209.85.128.69]) (Using TLS) by relay.mimecast.com with ESMTP id us-mta-370-umi1F2ACPiOFd-uW__JO4g-1; Wed, 24 Jun 2020 09:15:51 -0400 X-MC-Unique: umi1F2ACPiOFd-uW__JO4g-1 Received: by mail-wm1-f69.google.com with SMTP id t18so2730912wmj.5 for ; Wed, 24 Jun 2020 06:15:51 -0700 (PDT) X-Google-DKIM-Signature: v=1; a=rsa-sha256; c=relaxed/relaxed; d=1e100.net; s=20161025; h=x-gm-message-state:subject:to:references:from:autocrypt:message-id :date:user-agent:mime-version:in-reply-to:content-language :content-transfer-encoding; bh=7oBkdchRO8S+TG0SdV7obWRvQ4H4ZUW9GVrsi5Flg8U=; b=ONrqit1SBINBU1JlG0Qt1TjZlMfCm7KlUEC9NdpKujyawcuaRaABj/QRLp9sHZyisU ZZgau3VEY0O/B/nlmGK/BOrxuqoXB76Az1AgDIvmT9i4RQ1ZOzvaGhGhdVcD3kWebZQR cmJCg3KshRirFoR0y9BkM2ciwuWrVs9SaAPpZiykr+c62/HgcKy2QZvb0M4kdmjKVJm4 kH3YohqwONfJyUt80v3awpYe2kikN+Rz2t/KOktBkPqSZf70H3hyr74QS87z/wM0hM7+ GGI/xbPi7BXkVID1OTReFIoDySh21rMjLO/iLe22J10lU4vRk30gZ+eXyvmfvZK9YosE uLkw== X-Gm-Message-State: AOAM531KsUk/OLsHLmwCO8FrJLMVl5B1vTaSTbA4jRZrFTTo1W+dxnM3 b2rmaVjsKBxP9lkI27QoRN3JSMuNdVG8xu2oaAFow/Kq1eQoQmpfvtVp+tHO9Sloe/RZ2eRCuwF WvXSd9mipRoW8fQ== X-Received: by 2002:adf:ea06:: with SMTP id q6mr25683060wrm.69.1593004550167; Wed, 24 Jun 2020 06:15:50 -0700 (PDT) X-Google-Smtp-Source: ABdhPJyYMvoUbC5VDdBUHO51H85w3tHv0pYNCO8DEr8XMuE6x4dNgbmePuTgSsGJR9vx4BU+FVA1Ew== X-Received: by 2002:adf:ea06:: with SMTP id q6mr25683036wrm.69.1593004549826; Wed, 24 Jun 2020 06:15:49 -0700 (PDT) Return-Path: Received: from [192.168.1.40] (1.red-83-51-162.dynamicip.rima-tde.net. [83.51.162.1]) by smtp.gmail.com with ESMTPSA id w2sm15992088wrs.77.2020.06.24.06.15.48 (version=TLS1_3 cipher=TLS_AES_128_GCM_SHA256 bits=128/128); Wed, 24 Jun 2020 06:15:49 -0700 (PDT) Subject: Re: [PATCH v2] ArmVirtPkg/NorFlashQemuLib: disable NOR flash DT nodes upon discovery To: Ard Biesheuvel , Laszlo Ersek , devel@edk2.groups.io References: <20200624111524.1588197-1-ard.biesheuvel@arm.com> <18682fea-78cd-ab6f-c2f5-4d9b9fbdaac0@redhat.com> From: =?UTF-8?B?UGhpbGlwcGUgTWF0aGlldS1EYXVkw6k=?= Autocrypt: addr=philmd@redhat.com; keydata= mQINBDXML8YBEADXCtUkDBKQvNsQA7sDpw6YLE/1tKHwm24A1au9Hfy/OFmkpzo+MD+dYc+7 bvnqWAeGweq2SDq8zbzFZ1gJBd6+e5v1a/UrTxvwBk51yEkadrpRbi+r2bDpTJwXc/uEtYAB GvsTZMtiQVA4kRID1KCdgLa3zztPLCj5H1VZhqZsiGvXa/nMIlhvacRXdbgllPPJ72cLUkXf z1Zu4AkEKpccZaJspmLWGSzGu6UTZ7UfVeR2Hcc2KI9oZB1qthmZ1+PZyGZ/Dy+z+zklC0xl XIpQPmnfy9+/1hj1LzJ+pe3HzEodtlVA+rdttSvA6nmHKIt8Ul6b/h1DFTmUT1lN1WbAGxmg CH1O26cz5nTrzdjoqC/b8PpZiT0kO5MKKgiu5S4PRIxW2+RA4H9nq7nztNZ1Y39bDpzwE5Sp bDHzd5owmLxMLZAINtCtQuRbSOcMjZlg4zohA9TQP9krGIk+qTR+H4CV22sWldSkVtsoTaA2 qNeSJhfHQY0TyQvFbqRsSNIe2gTDzzEQ8itsmdHHE/yzhcCVvlUzXhAT6pIN0OT+cdsTTfif MIcDboys92auTuJ7U+4jWF1+WUaJ8gDL69ThAsu7mGDBbm80P3vvUZ4fQM14NkxOnuGRrJxO qjWNJ2ZUxgyHAh5TCxMLKWZoL5hpnvx3dF3Ti9HW2dsUUWICSQARAQABtDJQaGlsaXBwZSBN YXRoaWV1LURhdWTDqSAoUGhpbCkgPHBoaWxtZEByZWRoYXQuY29tPokCVQQTAQgAPwIbDwYL CQgHAwIGFQgCCQoLBBYCAwECHgECF4AWIQSJweePYB7obIZ0lcuio/1u3q3A3gUCXsfWwAUJ KtymWgAKCRCio/1u3q3A3ircD/9Vjh3aFNJ3uF3hddeoFg1H038wZr/xi8/rX27M1Vj2j9VH 0B8Olp4KUQw/hyO6kUxqkoojmzRpmzvlpZ0cUiZJo2bQIWnvScyHxFCv33kHe+YEIqoJlaQc JfKYlbCoubz+02E2A6bFD9+BvCY0LBbEj5POwyKGiDMjHKCGuzSuDRbCn0Mz4kCa7nFMF5Jv piC+JemRdiBd6102ThqgIsyGEBXuf1sy0QIVyXgaqr9O2b/0VoXpQId7yY7OJuYYxs7kQoXI 6WzSMpmuXGkmfxOgbc/L6YbzB0JOriX0iRClxu4dEUg8Bs2pNnr6huY2Ft+qb41RzCJvvMyu gS32LfN0bTZ6Qm2A8ayMtUQgnwZDSO23OKgQWZVglGliY3ezHZ6lVwC24Vjkmq/2yBSLakZE 6DZUjZzCW1nvtRK05ebyK6tofRsx8xB8pL/kcBb9nCuh70aLR+5cmE41X4O+MVJbwfP5s/RW 9BFSL3qgXuXso/3XuWTQjJJGgKhB6xXjMmb1J4q/h5IuVV4juv1Fem9sfmyrh+Wi5V1IzKI7 RPJ3KVb937eBgSENk53P0gUorwzUcO+ASEo3Z1cBKkJSPigDbeEjVfXQMzNt0oDRzpQqH2vp apo2jHnidWt8BsckuWZpxcZ9+/9obQ55DyVQHGiTN39hkETy3Emdnz1JVHTU0Q== Message-ID: Date: Wed, 24 Jun 2020 15:15:48 +0200 User-Agent: Mozilla/5.0 (X11; Linux x86_64; rv:68.0) Gecko/20100101 Thunderbird/68.5.0 MIME-Version: 1.0 In-Reply-To: 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 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 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 >; > } 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'). 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". 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... > > 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. > >