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 <ard.biesheu...@arm.com>
>>>> ---
>>>> 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 <ler...@redhat.com>
>>>
>>
>> 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 <ler...@redhat.com>
>>>
>>>
>>> (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 <phi...@redhat.com>
>
> 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
>
-=-=-=-=-=-=-=-=-=-=-=-
Groups.io Links: You receive all messages sent to this group.
View/Reply Online (#61646): https://edk2.groups.io/g/devel/message/61646
Mute This Topic: https://groups.io/mt/75065345/21656
Group Owner: devel+ow...@edk2.groups.io
Unsubscribe: https://edk2.groups.io/g/devel/unsub [arch...@mail-archive.com]
-=-=-=-=-=-=-=-=-=-=-=-
