[PATCH] ArmVirtPkg/NorFlashQemuLib: disable NOR flash DT nodes upon discovery

[PATCH] ArmVirtPkg/NorFlashQemuLib: disable NOR flash DT nodes upon discovery

[Ard Biesheuvel]

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.biesheuvel@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;
-- 
2.27.0

Re: [PATCH] ArmVirtPkg/NorFlashQemuLib: disable NOR flash DT nodes upon discovery

[Laszlo Ersek]

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.biesheuvel@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?

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 <lersek@redhat.com>

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 <lersek@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?

Thanks!
Laszlo

Re: [PATCH] ArmVirtPkg/NorFlashQemuLib: disable NOR flash DT nodes upon discovery

[Ard Biesheuvel]

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.biesheuvel@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 <lersek@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 <lersek@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. 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) but there is no good 
reason to keep supporting this.

Re: [PATCH] ArmVirtPkg/NorFlashQemuLib: disable NOR flash DT nodes upon discovery

[Laszlo Ersek]

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.biesheuvel@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 <lersek@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 <lersek@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.

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

Re: [PATCH] ArmVirtPkg/NorFlashQemuLib: disable NOR flash DT nodes upon discovery

[Philippe Mathieu-Daudé]

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.biesheuvel@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 <lersek@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 <lersek@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 <philmd@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
> 

Re: [edk2-devel] [PATCH] ArmVirtPkg/NorFlashQemuLib: disable NOR flash DT nodes upon discovery

[Laszlo Ersek]

(CC Drew, Eric)

On 06/24/20 09:19, Ard Biesheuvel wrote:

> The arm64 defconfig was recently updated with MTD support, and so the
> flash banks are now claimed by the CFI flash driver. 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) but there is no good
> reason to keep supporting this.

Can the same (problematic) kernel driver binding occur via the ACPI
DSDT?

In this fw patch we hide the flash chip(s) from the guest kernel via
Device Tree only.

There isn't much I guess we can (or should) do about ACPI in the
firmware, but it would still be a conflict between the fw driver and the
kernel driver -- we might have to address that in QEMU (hide the pflash
in the ACPI generator when UEFI is used as guest firmware).

IIRC, in QEMU, we use "arm_boot_info.firmware_loaded", from
<include/hw/arm/boot.h>, to represent UEFI:

>     /* Boot firmware has been loaded, typically at address 0, with -bios or
>      * -pflash. It also implies that fw_cfg_find() will succeed.
>      */
>     bool firmware_loaded;

And we already seem to have this exact kind of distinction in QEMU; see
for example "hw/arm/virt.c":

>     if (has_ged && aarch64 && firmware_loaded && virt_is_acpi_enabled(vms)) {
>         vms->acpi_dev = create_acpi_ged(vms);
>     } else {
>         create_gpio(vms);
>     }

coming from commit cff51ac978c4 ("hw/arm/virt: Enable device memory
cold/hot plug with ACPI boot", 2019-10-05).

And (same file):

>         if (!vms->bootinfo.firmware_loaded || !virt_is_acpi_enabled(vms)) {
>             return HOTPLUG_HANDLER(machine);
>         }

coming from commit 70e89132c9e6 ("hw/arm/virt: Add the virtio-iommu
device tree mappings", 2020-02-27).

... Ah, I think I've found it [hw/arm/virt-acpi-build.c]:

> /* DSDT */
> static void
> build_dsdt(GArray *table_data, BIOSLinker *linker, VirtMachineState *vms)
> {
>     Aml *scope, *dsdt;
>     MachineState *ms = MACHINE(vms);
>     const MemMapEntry *memmap = vms->memmap;
>     const int *irqmap = vms->irqmap;
>
>     dsdt = init_aml_allocator();
>     /* Reserve space for header */
>     acpi_data_push(dsdt->buf, sizeof(AcpiTableHeader));
>
>     /* When booting the VM with UEFI, UEFI takes ownership of the RTC hardware.
>      * While UEFI can use libfdt to disable the RTC device node in the DTB that
>      * it passes to the OS, it cannot modify AML. Therefore, we won't generate
>      * the RTC ACPI device at all when using UEFI.
>      */
>     scope = aml_scope("\\_SB");
>     acpi_dsdt_add_cpus(scope, vms->smp_cpus);
>     acpi_dsdt_add_uart(scope, &memmap[VIRT_UART],
>                        (irqmap[VIRT_UART] + ARM_SPI_BASE));
>     acpi_dsdt_add_flash(scope, &memmap[VIRT_FLASH]);

The ACPI generator already takes the RTC into account; see QEMU commit
67736a25f865 ("ARM: virt: Don't generate RTC ACPI device when using
UEFI", 2016-01-15).

Should we do the same for the acpi_dsdt_add_flash() call, now?

(This also suggests that my consideration of "firmware_loaded" above is
irrelevant, if we end up modifying the build_dsdt() function -- on
AARCH64, ACPI is only defined in UEFI terms (namely, as a UEFI system
config table), so in this function, the presence of UEFI can be assumed
"yes".)

Thanks!
Laszlo

Re: [edk2-devel] [PATCH] ArmVirtPkg/NorFlashQemuLib: disable NOR flash DT nodes upon discovery

[Ard Biesheuvel]

On 6/24/20 1:20 PM, Laszlo Ersek via groups.io wrote:
> (CC Drew, Eric)
> 
> On 06/24/20 09:19, Ard Biesheuvel wrote:
> 
>> The arm64 defconfig was recently updated with MTD support, and so the
>> flash banks are now claimed by the CFI flash driver. 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) but there is no good
>> reason to keep supporting this.
> 
> Can the same (problematic) kernel driver binding occur via the ACPI
> DSDT?
> 
> In this fw patch we hide the flash chip(s) from the guest kernel via
> Device Tree only.
> 
> There isn't much I guess we can (or should) do about ACPI in the
> firmware, but it would still be a conflict between the fw driver and the
> kernel driver -- we might have to address that in QEMU (hide the pflash
> in the ACPI generator when UEFI is used as guest firmware).
> 
> IIRC, in QEMU, we use "arm_boot_info.firmware_loaded", from
> <include/hw/arm/boot.h>, to represent UEFI:
> 
>>      /* Boot firmware has been loaded, typically at address 0, with -bios or
>>       * -pflash. It also implies that fw_cfg_find() will succeed.
>>       */
>>      bool firmware_loaded;
> 
> And we already seem to have this exact kind of distinction in QEMU; see
> for example "hw/arm/virt.c":
> 
>>      if (has_ged && aarch64 && firmware_loaded && virt_is_acpi_enabled(vms)) {
>>          vms->acpi_dev = create_acpi_ged(vms);
>>      } else {
>>          create_gpio(vms);
>>      }
> 
> coming from commit cff51ac978c4 ("hw/arm/virt: Enable device memory
> cold/hot plug with ACPI boot", 2019-10-05).
> 
> And (same file):
> 
>>          if (!vms->bootinfo.firmware_loaded || !virt_is_acpi_enabled(vms)) {
>>              return HOTPLUG_HANDLER(machine);
>>          }
> 
> coming from commit 70e89132c9e6 ("hw/arm/virt: Add the virtio-iommu
> device tree mappings", 2020-02-27).
> 
> ... Ah, I think I've found it [hw/arm/virt-acpi-build.c]:
> 
>> /* DSDT */
>> static void
>> build_dsdt(GArray *table_data, BIOSLinker *linker, VirtMachineState *vms)
>> {
>>      Aml *scope, *dsdt;
>>      MachineState *ms = MACHINE(vms);
>>      const MemMapEntry *memmap = vms->memmap;
>>      const int *irqmap = vms->irqmap;
>>
>>      dsdt = init_aml_allocator();
>>      /* Reserve space for header */
>>      acpi_data_push(dsdt->buf, sizeof(AcpiTableHeader));
>>
>>      /* When booting the VM with UEFI, UEFI takes ownership of the RTC hardware.
>>       * While UEFI can use libfdt to disable the RTC device node in the DTB that
>>       * it passes to the OS, it cannot modify AML. Therefore, we won't generate
>>       * the RTC ACPI device at all when using UEFI.
>>       */
>>      scope = aml_scope("\\_SB");
>>      acpi_dsdt_add_cpus(scope, vms->smp_cpus);
>>      acpi_dsdt_add_uart(scope, &memmap[VIRT_UART],
>>                         (irqmap[VIRT_UART] + ARM_SPI_BASE));
>>      acpi_dsdt_add_flash(scope, &memmap[VIRT_FLASH]);
> 
> The ACPI generator already takes the RTC into account; see QEMU commit
> 67736a25f865 ("ARM: virt: Don't generate RTC ACPI device when using
> UEFI", 2016-01-15).
> 
> Should we do the same for the acpi_dsdt_add_flash() call, now?
> 
> (This also suggests that my consideration of "firmware_loaded" above is
> irrelevant, if we end up modifying the build_dsdt() function -- on
> AARCH64, ACPI is only defined in UEFI terms (namely, as a UEFI system
> config table), so in this function, the presence of UEFI can be assumed
> "yes".)
> 

I wasn't aware that we even expose the flash in the DSDT. In any case, 
no driver exists in Linux today that claims the LNRO0015 _HID, and so I 
agree we should simply remove it entirely.

However, I am no longer able to contribute to QEMU, so I was hoping you 
or Phil could take the action?

Thanks,
Ard.

Re: [edk2-devel] [PATCH] ArmVirtPkg/NorFlashQemuLib: disable NOR flash DT nodes upon discovery

[Philippe Mathieu-Daudé]

On 6/24/20 1:43 PM, Ard Biesheuvel wrote:
> On 6/24/20 1:20 PM, Laszlo Ersek via groups.io wrote:
>> (CC Drew, Eric)
>>
>> On 06/24/20 09:19, Ard Biesheuvel wrote:
>>
>>> The arm64 defconfig was recently updated with MTD support, and so the
>>> flash banks are now claimed by the CFI flash driver. 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) but there is no good
>>> reason to keep supporting this.
>>
>> Can the same (problematic) kernel driver binding occur via the ACPI
>> DSDT?
>>
>> In this fw patch we hide the flash chip(s) from the guest kernel via
>> Device Tree only.
>>
>> There isn't much I guess we can (or should) do about ACPI in the
>> firmware, but it would still be a conflict between the fw driver and the
>> kernel driver -- we might have to address that in QEMU (hide the pflash
>> in the ACPI generator when UEFI is used as guest firmware).
>>
>> IIRC, in QEMU, we use "arm_boot_info.firmware_loaded", from
>> <include/hw/arm/boot.h>, to represent UEFI:
>>
>>>      /* Boot firmware has been loaded, typically at address 0, with
>>> -bios or
>>>       * -pflash. It also implies that fw_cfg_find() will succeed.
>>>       */
>>>      bool firmware_loaded;
>>
>> And we already seem to have this exact kind of distinction in QEMU; see
>> for example "hw/arm/virt.c":
>>
>>>      if (has_ged && aarch64 && firmware_loaded &&
>>> virt_is_acpi_enabled(vms)) {
>>>          vms->acpi_dev = create_acpi_ged(vms);
>>>      } else {
>>>          create_gpio(vms);
>>>      }
>>
>> coming from commit cff51ac978c4 ("hw/arm/virt: Enable device memory
>> cold/hot plug with ACPI boot", 2019-10-05).
>>
>> And (same file):
>>
>>>          if (!vms->bootinfo.firmware_loaded ||
>>> !virt_is_acpi_enabled(vms)) {
>>>              return HOTPLUG_HANDLER(machine);
>>>          }
>>
>> coming from commit 70e89132c9e6 ("hw/arm/virt: Add the virtio-iommu
>> device tree mappings", 2020-02-27).
>>
>> ... Ah, I think I've found it [hw/arm/virt-acpi-build.c]:
>>
>>> /* DSDT */
>>> static void
>>> build_dsdt(GArray *table_data, BIOSLinker *linker, VirtMachineState
>>> *vms)
>>> {
>>>      Aml *scope, *dsdt;
>>>      MachineState *ms = MACHINE(vms);
>>>      const MemMapEntry *memmap = vms->memmap;
>>>      const int *irqmap = vms->irqmap;
>>>
>>>      dsdt = init_aml_allocator();
>>>      /* Reserve space for header */
>>>      acpi_data_push(dsdt->buf, sizeof(AcpiTableHeader));
>>>
>>>      /* When booting the VM with UEFI, UEFI takes ownership of the
>>> RTC hardware.
>>>       * While UEFI can use libfdt to disable the RTC device node in
>>> the DTB that
>>>       * it passes to the OS, it cannot modify AML. Therefore, we
>>> won't generate
>>>       * the RTC ACPI device at all when using UEFI.
>>>       */
>>>      scope = aml_scope("\\_SB");
>>>      acpi_dsdt_add_cpus(scope, vms->smp_cpus);
>>>      acpi_dsdt_add_uart(scope, &memmap[VIRT_UART],
>>>                         (irqmap[VIRT_UART] + ARM_SPI_BASE));
>>>      acpi_dsdt_add_flash(scope, &memmap[VIRT_FLASH]);
>>
>> The ACPI generator already takes the RTC into account; see QEMU commit
>> 67736a25f865 ("ARM: virt: Don't generate RTC ACPI device when using
>> UEFI", 2016-01-15).
>>
>> Should we do the same for the acpi_dsdt_add_flash() call, now?
>>
>> (This also suggests that my consideration of "firmware_loaded" above is
>> irrelevant, if we end up modifying the build_dsdt() function -- on
>> AARCH64, ACPI is only defined in UEFI terms (namely, as a UEFI system
>> config table), so in this function, the presence of UEFI can be assumed
>> "yes".)
>>
> 
> I wasn't aware that we even expose the flash in the DSDT. In any case,
> no driver exists in Linux today that claims the LNRO0015 _HID, and so I
> agree we should simply remove it entirely.
> 
> However, I am no longer able to contribute to QEMU, so I was hoping you
> or Phil could take the action?

I try to stay as far as possible from ACPI, but here it seems
fair I assign myself to this (except if Drew/Eric prefer to
do it, of course!).

> 
> Thanks,
> Ard.
> 
> 
> 

Re: [edk2-devel] [PATCH] ArmVirtPkg/NorFlashQemuLib: disable NOR flash DT nodes upon discovery

[Andrew Jones]

On Wed, Jun 24, 2020 at 03:02:49PM +0200, Philippe Mathieu-Daudé wrote:
> On 6/24/20 1:43 PM, Ard Biesheuvel wrote:
> > On 6/24/20 1:20 PM, Laszlo Ersek via groups.io wrote:
> >> (CC Drew, Eric)
> >>
> >> On 06/24/20 09:19, Ard Biesheuvel wrote:
> >>
> >>> The arm64 defconfig was recently updated with MTD support, and so the
> >>> flash banks are now claimed by the CFI flash driver. 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) but there is no good
> >>> reason to keep supporting this.
> >>
> >> Can the same (problematic) kernel driver binding occur via the ACPI
> >> DSDT?
> >>
> >> In this fw patch we hide the flash chip(s) from the guest kernel via
> >> Device Tree only.
> >>
> >> There isn't much I guess we can (or should) do about ACPI in the
> >> firmware, but it would still be a conflict between the fw driver and the
> >> kernel driver -- we might have to address that in QEMU (hide the pflash
> >> in the ACPI generator when UEFI is used as guest firmware).
> >>
> >> IIRC, in QEMU, we use "arm_boot_info.firmware_loaded", from
> >> <include/hw/arm/boot.h>, to represent UEFI:
> >>
> >>>      /* Boot firmware has been loaded, typically at address 0, with
> >>> -bios or
> >>>       * -pflash. It also implies that fw_cfg_find() will succeed.
> >>>       */
> >>>      bool firmware_loaded;
> >>
> >> And we already seem to have this exact kind of distinction in QEMU; see
> >> for example "hw/arm/virt.c":
> >>
> >>>      if (has_ged && aarch64 && firmware_loaded &&
> >>> virt_is_acpi_enabled(vms)) {
> >>>          vms->acpi_dev = create_acpi_ged(vms);
> >>>      } else {
> >>>          create_gpio(vms);
> >>>      }
> >>
> >> coming from commit cff51ac978c4 ("hw/arm/virt: Enable device memory
> >> cold/hot plug with ACPI boot", 2019-10-05).
> >>
> >> And (same file):
> >>
> >>>          if (!vms->bootinfo.firmware_loaded ||
> >>> !virt_is_acpi_enabled(vms)) {
> >>>              return HOTPLUG_HANDLER(machine);
> >>>          }
> >>
> >> coming from commit 70e89132c9e6 ("hw/arm/virt: Add the virtio-iommu
> >> device tree mappings", 2020-02-27).
> >>
> >> ... Ah, I think I've found it [hw/arm/virt-acpi-build.c]:
> >>
> >>> /* DSDT */
> >>> static void
> >>> build_dsdt(GArray *table_data, BIOSLinker *linker, VirtMachineState
> >>> *vms)
> >>> {
> >>>      Aml *scope, *dsdt;
> >>>      MachineState *ms = MACHINE(vms);
> >>>      const MemMapEntry *memmap = vms->memmap;
> >>>      const int *irqmap = vms->irqmap;
> >>>
> >>>      dsdt = init_aml_allocator();
> >>>      /* Reserve space for header */
> >>>      acpi_data_push(dsdt->buf, sizeof(AcpiTableHeader));
> >>>
> >>>      /* When booting the VM with UEFI, UEFI takes ownership of the
> >>> RTC hardware.
> >>>       * While UEFI can use libfdt to disable the RTC device node in
> >>> the DTB that
> >>>       * it passes to the OS, it cannot modify AML. Therefore, we
> >>> won't generate
> >>>       * the RTC ACPI device at all when using UEFI.
> >>>       */
> >>>      scope = aml_scope("\\_SB");
> >>>      acpi_dsdt_add_cpus(scope, vms->smp_cpus);
> >>>      acpi_dsdt_add_uart(scope, &memmap[VIRT_UART],
> >>>                         (irqmap[VIRT_UART] + ARM_SPI_BASE));
> >>>      acpi_dsdt_add_flash(scope, &memmap[VIRT_FLASH]);
> >>
> >> The ACPI generator already takes the RTC into account; see QEMU commit
> >> 67736a25f865 ("ARM: virt: Don't generate RTC ACPI device when using
> >> UEFI", 2016-01-15).
> >>
> >> Should we do the same for the acpi_dsdt_add_flash() call, now?
> >>
> >> (This also suggests that my consideration of "firmware_loaded" above is
> >> irrelevant, if we end up modifying the build_dsdt() function -- on
> >> AARCH64, ACPI is only defined in UEFI terms (namely, as a UEFI system
> >> config table), so in this function, the presence of UEFI can be assumed
> >> "yes".)
> >>
> > 
> > I wasn't aware that we even expose the flash in the DSDT. In any case,
> > no driver exists in Linux today that claims the LNRO0015 _HID, and so I
> > agree we should simply remove it entirely.
> > 
> > However, I am no longer able to contribute to QEMU, so I was hoping you
> > or Phil could take the action?
> 
> I try to stay as far as possible from ACPI, but here it seems
> fair I assign myself to this (except if Drew/Eric prefer to
> do it, of course!).

I don't mind doing it. IIUC, all we need to do is remove the flash device
from the DSDT to "hide" it from the guest. Of course we'll need some
compat code too in order to only do this for machine types 5.1 and later,
and that means that running guest kernels which want to bind the flash on
5.0 and older machine types will still have the problem.

Also, it seems a bit odd to hide hardware from the guest OS. Wouldn't it
be better to somehow flag that the hardare may be in use by firmware,
and therefore is only safe to use if runtime services are not used? I'm
not sure ACPI supports that for table entries like these, but maybe some
_STA value indicates something like it. I'll take a look at the spec.

Thanks,
drew

Re: [edk2-devel] [PATCH] ArmVirtPkg/NorFlashQemuLib: disable NOR flash DT nodes upon discovery

[Philippe Mathieu-Daudé]

On Wed, Jun 24, 2020 at 3:42 PM Andrew Jones <drjones@redhat.com> wrote:
> On Wed, Jun 24, 2020 at 03:02:49PM +0200, Philippe Mathieu-Daudé wrote:
> > On 6/24/20 1:43 PM, Ard Biesheuvel wrote:
> > > On 6/24/20 1:20 PM, Laszlo Ersek via groups.io wrote:
> > >> (CC Drew, Eric)
> > >>
> > >> On 06/24/20 09:19, Ard Biesheuvel wrote:
> > >>
> > >>> The arm64 defconfig was recently updated with MTD support, and so the
> > >>> flash banks are now claimed by the CFI flash driver. 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) but there is no good
> > >>> reason to keep supporting this.
> > >>
> > >> Can the same (problematic) kernel driver binding occur via the ACPI
> > >> DSDT?
> > >>
> > >> In this fw patch we hide the flash chip(s) from the guest kernel via
> > >> Device Tree only.
> > >>
> > >> There isn't much I guess we can (or should) do about ACPI in the
> > >> firmware, but it would still be a conflict between the fw driver and the
> > >> kernel driver -- we might have to address that in QEMU (hide the pflash
> > >> in the ACPI generator when UEFI is used as guest firmware).
> > >>
> > >> IIRC, in QEMU, we use "arm_boot_info.firmware_loaded", from
> > >> <include/hw/arm/boot.h>, to represent UEFI:
> > >>
> > >>>      /* Boot firmware has been loaded, typically at address 0, with
> > >>> -bios or
> > >>>       * -pflash. It also implies that fw_cfg_find() will succeed.
> > >>>       */
> > >>>      bool firmware_loaded;
> > >>
> > >> And we already seem to have this exact kind of distinction in QEMU; see
> > >> for example "hw/arm/virt.c":
> > >>
> > >>>      if (has_ged && aarch64 && firmware_loaded &&
> > >>> virt_is_acpi_enabled(vms)) {
> > >>>          vms->acpi_dev = create_acpi_ged(vms);
> > >>>      } else {
> > >>>          create_gpio(vms);
> > >>>      }
> > >>
> > >> coming from commit cff51ac978c4 ("hw/arm/virt: Enable device memory
> > >> cold/hot plug with ACPI boot", 2019-10-05).
> > >>
> > >> And (same file):
> > >>
> > >>>          if (!vms->bootinfo.firmware_loaded ||
> > >>> !virt_is_acpi_enabled(vms)) {
> > >>>              return HOTPLUG_HANDLER(machine);
> > >>>          }
> > >>
> > >> coming from commit 70e89132c9e6 ("hw/arm/virt: Add the virtio-iommu
> > >> device tree mappings", 2020-02-27).
> > >>
> > >> ... Ah, I think I've found it [hw/arm/virt-acpi-build.c]:
> > >>
> > >>> /* DSDT */
> > >>> static void
> > >>> build_dsdt(GArray *table_data, BIOSLinker *linker, VirtMachineState
> > >>> *vms)
> > >>> {
> > >>>      Aml *scope, *dsdt;
> > >>>      MachineState *ms = MACHINE(vms);
> > >>>      const MemMapEntry *memmap = vms->memmap;
> > >>>      const int *irqmap = vms->irqmap;
> > >>>
> > >>>      dsdt = init_aml_allocator();
> > >>>      /* Reserve space for header */
> > >>>      acpi_data_push(dsdt->buf, sizeof(AcpiTableHeader));
> > >>>
> > >>>      /* When booting the VM with UEFI, UEFI takes ownership of the
> > >>> RTC hardware.
> > >>>       * While UEFI can use libfdt to disable the RTC device node in
> > >>> the DTB that
> > >>>       * it passes to the OS, it cannot modify AML. Therefore, we
> > >>> won't generate
> > >>>       * the RTC ACPI device at all when using UEFI.
> > >>>       */
> > >>>      scope = aml_scope("\\_SB");
> > >>>      acpi_dsdt_add_cpus(scope, vms->smp_cpus);
> > >>>      acpi_dsdt_add_uart(scope, &memmap[VIRT_UART],
> > >>>                         (irqmap[VIRT_UART] + ARM_SPI_BASE));
> > >>>      acpi_dsdt_add_flash(scope, &memmap[VIRT_FLASH]);
> > >>
> > >> The ACPI generator already takes the RTC into account; see QEMU commit
> > >> 67736a25f865 ("ARM: virt: Don't generate RTC ACPI device when using
> > >> UEFI", 2016-01-15).
> > >>
> > >> Should we do the same for the acpi_dsdt_add_flash() call, now?
> > >>
> > >> (This also suggests that my consideration of "firmware_loaded" above is
> > >> irrelevant, if we end up modifying the build_dsdt() function -- on
> > >> AARCH64, ACPI is only defined in UEFI terms (namely, as a UEFI system
> > >> config table), so in this function, the presence of UEFI can be assumed
> > >> "yes".)
> > >>
> > >
> > > I wasn't aware that we even expose the flash in the DSDT. In any case,
> > > no driver exists in Linux today that claims the LNRO0015 _HID, and so I
> > > agree we should simply remove it entirely.
> > >
> > > However, I am no longer able to contribute to QEMU, so I was hoping you
> > > or Phil could take the action?
> >
> > I try to stay as far as possible from ACPI, but here it seems
> > fair I assign myself to this (except if Drew/Eric prefer to
> > do it, of course!).
>
> I don't mind doing it. IIUC, all we need to do is remove the flash device
> from the DSDT to "hide" it from the guest. Of course we'll need some
> compat code too in order to only do this for machine types 5.1 and later,
> and that means that running guest kernels which want to bind the flash on
> 5.0 and older machine types will still have the problem.
>
> Also, it seems a bit odd to hide hardware from the guest OS. Wouldn't it
> be better to somehow flag that the hardare may be in use by firmware,
> and therefore is only safe to use if runtime services are not used? I'm
> not sure ACPI supports that for table entries like these, but maybe some
> _STA value indicates something like it. I'll take a look at the spec.

Thank you! You'll certainly send a clearer/better patch than me on this topic :)

Re: [edk2-devel] [PATCH] ArmVirtPkg/NorFlashQemuLib: disable NOR flash DT nodes upon discovery

[Ard Biesheuvel]

On 6/24/20 3:41 PM, Andrew Jones wrote:
> On Wed, Jun 24, 2020 at 03:02:49PM +0200, Philippe Mathieu-Daudé wrote:
>> On 6/24/20 1:43 PM, Ard Biesheuvel wrote:
...
>>> I wasn't aware that we even expose the flash in the DSDT. In any case,
>>> no driver exists in Linux today that claims the LNRO0015 _HID, and so I
>>> agree we should simply remove it entirely.
>>>
>>> However, I am no longer able to contribute to QEMU, so I was hoping you
>>> or Phil could take the action?
>>
>> I try to stay as far as possible from ACPI, but here it seems
>> fair I assign myself to this (except if Drew/Eric prefer to
>> do it, of course!).
> 
> I don't mind doing it. IIUC, all we need to do is remove the flash device
> from the DSDT to "hide" it from the guest. Of course we'll need some
> compat code too in order to only do this for machine types 5.1 and later,
> and that means that running guest kernels which want to bind the flash on
> 5.0 and older machine types will still have the problem.
> 

Do you think that is really necessary? LNRO0015 never had a driver in 
Linux to begin with, and I doubt other ACPI capable arm64 OSes would be 
any different.

> Also, it seems a bit odd to hide hardware from the guest OS. Wouldn't it
> be better to somehow flag that the hardare may be in use by firmware,
> and therefore is only safe to use if runtime services are not used? I'm
> not sure ACPI supports that for table entries like these, but maybe some
> _STA value indicates something like it. I'll take a look at the spec.
> 

We could do either, but a _STA indicating that the device is not present 
or not ready amounts to the same afaik. Ultimately, the OS could still 
access the physical range if it wanted to (e.g., via /dev/mem), so not 
exposing it in the first place seems more robust to me.

Re: [edk2-devel] [PATCH] ArmVirtPkg/NorFlashQemuLib: disable NOR flash DT nodes upon discovery

[Andrew Jones]

On Wed, Jun 24, 2020 at 03:48:46PM +0200, Ard Biesheuvel wrote:
> On 6/24/20 3:41 PM, Andrew Jones wrote:
> > On Wed, Jun 24, 2020 at 03:02:49PM +0200, Philippe Mathieu-Daudé wrote:
> > > On 6/24/20 1:43 PM, Ard Biesheuvel wrote:
> ...
> > > > I wasn't aware that we even expose the flash in the DSDT. In any case,
> > > > no driver exists in Linux today that claims the LNRO0015 _HID, and so I
> > > > agree we should simply remove it entirely.
> > > > 
> > > > However, I am no longer able to contribute to QEMU, so I was hoping you
> > > > or Phil could take the action?
> > > 
> > > I try to stay as far as possible from ACPI, but here it seems
> > > fair I assign myself to this (except if Drew/Eric prefer to
> > > do it, of course!).
> > 
> > I don't mind doing it. IIUC, all we need to do is remove the flash device
> > from the DSDT to "hide" it from the guest. Of course we'll need some
> > compat code too in order to only do this for machine types 5.1 and later,
> > and that means that running guest kernels which want to bind the flash on
> > 5.0 and older machine types will still have the problem.
> > 
> 
> Do you think that is really necessary? LNRO0015 never had a driver in Linux
> to begin with, and I doubt other ACPI capable arm64 OSes would be any
> different.

I'd rather not add/remove hardware in older machine types. While it's
unlikely anybody would notice, we can't be sure that there's nothing
out there which would break.

> 
> > Also, it seems a bit odd to hide hardware from the guest OS. Wouldn't it
> > be better to somehow flag that the hardare may be in use by firmware,
> > and therefore is only safe to use if runtime services are not used? I'm
> > not sure ACPI supports that for table entries like these, but maybe some
> > _STA value indicates something like it. I'll take a look at the spec.
> > 
> 
> We could do either, but a _STA indicating that the device is not present or
> not ready amounts to the same afaik. Ultimately, the OS could still access
> the physical range if it wanted to (e.g., via /dev/mem), so not exposing it
> in the first place seems more robust to me.
> 

If there's no _STA state that says the device is here and works, but it's
not available, then I agree removing it is the same. And, thinking about
it some more, this flash device is really only for our host-controlled
firmware. Since we don't give the guest any control over the firmware,
then the device the firmware lives on should probably not even exist as
far as the guest is concerned.

Thanks,
drew

Re: [edk2-devel] [PATCH] ArmVirtPkg/NorFlashQemuLib: disable NOR flash DT nodes upon discovery

[Laszlo Ersek]

On 06/24/20 16:37, Andrew Jones wrote:
> On Wed, Jun 24, 2020 at 03:48:46PM +0200, Ard Biesheuvel wrote:
>> On 6/24/20 3:41 PM, Andrew Jones wrote:
>>> On Wed, Jun 24, 2020 at 03:02:49PM +0200, Philippe Mathieu-Daudé wrote:
>>>> On 6/24/20 1:43 PM, Ard Biesheuvel wrote:
>> ...
>>>>> I wasn't aware that we even expose the flash in the DSDT. In any case,
>>>>> no driver exists in Linux today that claims the LNRO0015 _HID, and so I
>>>>> agree we should simply remove it entirely.
>>>>>
>>>>> However, I am no longer able to contribute to QEMU, so I was hoping you
>>>>> or Phil could take the action?
>>>>
>>>> I try to stay as far as possible from ACPI, but here it seems
>>>> fair I assign myself to this (except if Drew/Eric prefer to
>>>> do it, of course!).
>>>
>>> I don't mind doing it. IIUC, all we need to do is remove the flash device
>>> from the DSDT to "hide" it from the guest. Of course we'll need some
>>> compat code too in order to only do this for machine types 5.1 and later,
>>> and that means that running guest kernels which want to bind the flash on
>>> 5.0 and older machine types will still have the problem.
>>>
>>
>> Do you think that is really necessary? LNRO0015 never had a driver in Linux
>> to begin with, and I doubt other ACPI capable arm64 OSes would be any
>> different.
> 
> I'd rather not add/remove hardware in older machine types. While it's
> unlikely anybody would notice, we can't be sure that there's nothing
> out there which would break.

I agree.

>>
>>> Also, it seems a bit odd to hide hardware from the guest OS. Wouldn't it
>>> be better to somehow flag that the hardare may be in use by firmware,
>>> and therefore is only safe to use if runtime services are not used? I'm
>>> not sure ACPI supports that for table entries like these, but maybe some
>>> _STA value indicates something like it. I'll take a look at the spec.
>>>
>>
>> We could do either, but a _STA indicating that the device is not present or
>> not ready amounts to the same afaik. Ultimately, the OS could still access
>> the physical range if it wanted to (e.g., via /dev/mem), so not exposing it
>> in the first place seems more robust to me.
>>
> 
> If there's no _STA state that says the device is here and works, but it's
> not available, then I agree removing it is the same. And, thinking about
> it some more, this flash device is really only for our host-controlled
> firmware. Since we don't give the guest any control over the firmware,
> then the device the firmware lives on should probably not even exist as
> far as the guest is concerned.

I think it's safest to remove the object from the DSDT; at least x86
Windows used to be really picky about _STA in Device Manager. Best to
avoid yellow triangles (or whatever) there (assuming Device Manager is a
thing on aarch64 Windows -- I don't know).

Drew, when you remove the flash addition function call, please replace
it with a comment that's similar to the one we have about the RTC. That
way, we can run "git blame" on the comment. (Pure deletions tend to
impede "git blame", as no code lines remain on which git-blame could
report a "latest commit".)

Thank you very much!
Laszlo

Re: [edk2-devel] [PATCH] ArmVirtPkg/NorFlashQemuLib: disable NOR flash DT nodes upon discovery

[Laszlo Ersek]

On 06/24/20 20:43, Laszlo Ersek wrote:
> On 06/24/20 16:37, Andrew Jones wrote:
>> On Wed, Jun 24, 2020 at 03:48:46PM +0200, Ard Biesheuvel wrote:
>>> On 6/24/20 3:41 PM, Andrew Jones wrote:
>>>> On Wed, Jun 24, 2020 at 03:02:49PM +0200, Philippe Mathieu-Daudé wrote:
>>>>> On 6/24/20 1:43 PM, Ard Biesheuvel wrote:
>>> ...
>>>>>> I wasn't aware that we even expose the flash in the DSDT. In any case,
>>>>>> no driver exists in Linux today that claims the LNRO0015 _HID, and so I
>>>>>> agree we should simply remove it entirely.
>>>>>>
>>>>>> However, I am no longer able to contribute to QEMU, so I was hoping you
>>>>>> or Phil could take the action?
>>>>>
>>>>> I try to stay as far as possible from ACPI, but here it seems
>>>>> fair I assign myself to this (except if Drew/Eric prefer to
>>>>> do it, of course!).
>>>>
>>>> I don't mind doing it. IIUC, all we need to do is remove the flash device
>>>> from the DSDT to "hide" it from the guest. Of course we'll need some
>>>> compat code too in order to only do this for machine types 5.1 and later,
>>>> and that means that running guest kernels which want to bind the flash on
>>>> 5.0 and older machine types will still have the problem.
>>>>
>>>
>>> Do you think that is really necessary? LNRO0015 never had a driver in Linux
>>> to begin with, and I doubt other ACPI capable arm64 OSes would be any
>>> different.
>>
>> I'd rather not add/remove hardware in older machine types. While it's
>> unlikely anybody would notice, we can't be sure that there's nothing
>> out there which would break.
> 
> I agree.
> 
>>>
>>>> Also, it seems a bit odd to hide hardware from the guest OS. Wouldn't it
>>>> be better to somehow flag that the hardare may be in use by firmware,
>>>> and therefore is only safe to use if runtime services are not used? I'm
>>>> not sure ACPI supports that for table entries like these, but maybe some
>>>> _STA value indicates something like it. I'll take a look at the spec.
>>>>
>>>
>>> We could do either, but a _STA indicating that the device is not present or
>>> not ready amounts to the same afaik. Ultimately, the OS could still access
>>> the physical range if it wanted to (e.g., via /dev/mem), so not exposing it
>>> in the first place seems more robust to me.
>>>
>>
>> If there's no _STA state that says the device is here and works, but it's
>> not available, then I agree removing it is the same. And, thinking about
>> it some more, this flash device is really only for our host-controlled
>> firmware. Since we don't give the guest any control over the firmware,
>> then the device the firmware lives on should probably not even exist as
>> far as the guest is concerned.
> 
> I think it's safest to remove the object from the DSDT; at least x86
> Windows used to be really picky about _STA in Device Manager. Best to
> avoid yellow triangles (or whatever) there (assuming Device Manager is a
> thing on aarch64 Windows -- I don't know).
> 
> Drew, when you remove the flash addition function call, please replace
> it with a comment that's similar to the one we have about the RTC. That
> way, we can run "git blame" on the comment. (Pure deletions tend to
> impede "git blame", as no code lines remain on which git-blame could
> report a "latest commit".)

oops, sorry, dumb request -- this isn't going to be a pure removal; the
flash addition will remain, it will only be made conditional on a new
machine type property. So there's going to be *something* to run
git-blame on. Apologies for my mistake.

Laszlo