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.9912.1592991312099153317 for ; Wed, 24 Jun 2020 02:35:12 -0700 Authentication-Results: mx.groups.io; dkim=pass header.i=@redhat.com header.s=mimecast20190719 header.b=MvCXGqo+; spf=pass (domain: redhat.com, ip: 207.211.31.81, mailfrom: philmd@redhat.com) DKIM-Signature: v=1; a=rsa-sha256; c=relaxed/relaxed; d=redhat.com; s=mimecast20190719; t=1592991311; 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=8mHCX80GtF8KXUK53mL52yvHJxoKaOKRVKdK7Yh42ac=; b=MvCXGqo+/xAK5VW4/3t5dbBVSjmh4DffGzBijrwAO4JJO4KvSN6DssVR6ceXxXc7GJkfbc tMhlVttZNt/MWDUMACfsV/vWT++MINrQf12Qt+8TAv/4h+gy1XHwG9XagAY6dKeyTazwGY 9jsfyvyRK2TEIrAa5JT3TOzsjqUprRI= Received: from mail-wr1-f70.google.com (mail-wr1-f70.google.com [209.85.221.70]) (Using TLS) by relay.mimecast.com with ESMTP id us-mta-31-dmXQGLn2PVaIoIK19kxqQQ-1; Wed, 24 Jun 2020 05:35:09 -0400 X-MC-Unique: dmXQGLn2PVaIoIK19kxqQQ-1 Received: by mail-wr1-f70.google.com with SMTP id l3so2334894wrw.4 for ; Wed, 24 Jun 2020 02:35:09 -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=8mHCX80GtF8KXUK53mL52yvHJxoKaOKRVKdK7Yh42ac=; b=Cgcv7gfvniaZHHYfpelFFjkiuA6HbhZmsxh0VLqceMGtCER0S6EjtXWVwfiF+RAOor xl3+jeK6t68UgGg0d/rIkyPhvbERMcBCkU/UTdOVL4Xy8ChAAkHTcFZMmN8qTShx1iyz AyB/c4ZD9yKsq6zXt2E5VjRRiKwC1pU5H4mOsXO0RZYmq8uJAzG4apzGcSdctmslXU0d oymFnOEEg68WHuQhvKiBEj9kyVOc6t8oHYLvODsUPr/buQAt2RjUg/y9ou4r0M3D6cmz 35DGGeS9v/7NKzzdVYtTG6VcSudBWMGVsEimFPABucRS5k0XtZq90kG5WUsYPNVlM4EE jNvg== X-Gm-Message-State: AOAM530Qlh3nNJhfg9QhDBo3IPLXyF7ldA+y4zFP8gwb3w1UHLP4RqiL re4524d4z+/yQDfhtB+lDZ7hlGGGMWVVZzjBdnQfJU8xumyCEO1NcSq0FFcviacL2+qag7jTKEz EUJg9gnzrL+8+Ug== X-Received: by 2002:a5d:4c82:: with SMTP id z2mr7411737wrs.287.1592991308337; Wed, 24 Jun 2020 02:35:08 -0700 (PDT) X-Google-Smtp-Source: ABdhPJxR/4AWbY3LOswGy3cvYVpWz/Nw/600+fvi7CGawmKhhbyPoSafpkuHy7oCrxmBs07UquLhGg== X-Received: by 2002:a5d:4c82:: with SMTP id z2mr7411712wrs.287.1592991308045; Wed, 24 Jun 2020 02:35:08 -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 v24sm30817957wrd.92.2020.06.24.02.35.07 (version=TLS1_3 cipher=TLS_AES_128_GCM_SHA256 bits=128/128); Wed, 24 Jun 2020 02:35:07 -0700 (PDT) Subject: Re: [PATCH] ArmVirtPkg/NorFlashQemuLib: disable NOR flash DT nodes upon discovery To: Laszlo Ersek , Ard Biesheuvel , devel@edk2.groups.io References: <20200623175700.1564281-1-ard.biesheuvel@arm.com> <85a63fc4-f3d1-1e17-bf1d-dace59bb02a8@arm.com> <6cc28940-abb6-fce8-b24b-92a1bf78f14d@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 11:35:06 +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: <6cc28940-abb6-fce8-b24b-92a1bf78f14d@redhat.com> Authentication-Results: relay.mimecast.com; auth=pass smtp.auth=CUSA124A263 smtp.mailfrom=philmd@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 6/24/20 11:00 AM, Laszlo Ersek wrote: > 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. Reviewed-by: Philippe Mathieu-Daude > > 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 >