public inbox for devel@edk2.groups.io
 help / color / mirror / Atom feed
From: "Laszlo Ersek" <lersek@redhat.com>
To: Ard Biesheuvel <ard.biesheuvel@arm.com>, devel@edk2.groups.io
Cc: philmd@redhat.com
Subject: Re: [PATCH v2] ArmVirtPkg/NorFlashQemuLib: disable NOR flash DT nodes upon discovery
Date: Wed, 24 Jun 2020 20:47:41 +0200	[thread overview]
Message-ID: <eeae7015-68bb-8531-bcc9-52720b64cc1e@redhat.com> (raw)
In-Reply-To: <c2ac0cd4-2af0-71e1-b23b-e3c4c56841f0@arm.com>

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 <lersek@redhat.com>
>>> Reviewed-by: Philippe Mathieu-Daude <philmd@redhat.com>
>>> Signed-off-by: Ard Biesheuvel <ard.biesheuvel@arm.com>
>>> ---
>>>   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


      parent reply	other threads:[~2020-06-24 18:47 UTC|newest]

Thread overview: 6+ messages / expand[flat|nested]  mbox.gz  Atom feed  top
2020-06-24 11:15 [PATCH v2] ArmVirtPkg/NorFlashQemuLib: disable NOR flash DT nodes upon discovery Ard Biesheuvel
2020-06-24 11:48 ` Laszlo Ersek
2020-06-24 12:19   ` Ard Biesheuvel
2020-06-24 13:15     ` Philippe Mathieu-Daudé
2020-06-24 15:14       ` Ard Biesheuvel
2020-06-24 18:47     ` Laszlo Ersek [this message]

Reply instructions:

You may reply publicly to this message via plain-text email
using any one of the following methods:

* Save the following mbox file, import it into your mail client,
  and reply-to-list from there: mbox

  Avoid top-posting and favor interleaved quoting:
  https://en.wikipedia.org/wiki/Posting_style#Interleaved_style

* Reply using the --to, --cc, and --in-reply-to
  switches of git-send-email(1):

  git send-email \
    --in-reply-to=eeae7015-68bb-8531-bcc9-52720b64cc1e@redhat.com \
    --to=devel@edk2.groups.io \
    /path/to/YOUR_REPLY

  https://kernel.org/pub/software/scm/git/docs/git-send-email.html

* If your mail client supports setting the In-Reply-To header
  via mailto: links, try the mailto: link
Be sure your reply has a Subject: header at the top and a blank line before the message body.
This is a public inbox, see mirroring instructions
for how to clone and mirror all data and code used for this inbox