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

Ard Biesheuvel posted 1 patch 3 years, 9 months ago
Patches applied successfully (tree, apply log)
git fetch https://github.com/patchew-project/edk2 tags/patchew/20200624111524.1588197-1-ard.biesheuvel@arm.com
ArmVirtPkg/Library/NorFlashQemuLib/NorFlashQemuLib.c | 16 ++++++++++++++++
1 file changed, 16 insertions(+)
[edk2-devel] [PATCH v2] ArmVirtPkg/NorFlashQemuLib: disable NOR flash DT nodes upon discovery
Posted by Ard Biesheuvel 3 years, 9 months ago
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;
-- 
2.27.0


-=-=-=-=-=-=-=-=-=-=-=-
Groups.io Links: You receive all messages sent to this group.

View/Reply Online (#61648): https://edk2.groups.io/g/devel/message/61648
Mute This Topic: https://groups.io/mt/75079347/1787277
Group Owner: devel+owner@edk2.groups.io
Unsubscribe: https://edk2.groups.io/g/devel/unsub  [importer@patchew.org]
-=-=-=-=-=-=-=-=-=-=-=-

Re: [edk2-devel] [PATCH v2] ArmVirtPkg/NorFlashQemuLib: disable NOR flash DT nodes upon discovery
Posted by Laszlo Ersek 3 years, 9 months ago
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;
> 

Thank you!
Laszlo


-=-=-=-=-=-=-=-=-=-=-=-
Groups.io Links: You receive all messages sent to this group.

View/Reply Online (#61651): https://edk2.groups.io/g/devel/message/61651
Mute This Topic: https://groups.io/mt/75079347/1787277
Group Owner: devel+owner@edk2.groups.io
Unsubscribe: https://edk2.groups.io/g/devel/unsub  [importer@patchew.org]
-=-=-=-=-=-=-=-=-=-=-=-

Re: [edk2-devel] [PATCH v2] ArmVirtPkg/NorFlashQemuLib: disable NOR flash DT nodes upon discovery
Posted by Ard Biesheuvel 3 years, 9 months ago
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.



-=-=-=-=-=-=-=-=-=-=-=-
Groups.io Links: You receive all messages sent to this group.

View/Reply Online (#61658): https://edk2.groups.io/g/devel/message/61658
Mute This Topic: https://groups.io/mt/75079347/1787277
Group Owner: devel+owner@edk2.groups.io
Unsubscribe: https://edk2.groups.io/g/devel/unsub  [importer@patchew.org]
-=-=-=-=-=-=-=-=-=-=-=-

Re: [edk2-devel] [PATCH v2] ArmVirtPkg/NorFlashQemuLib: disable NOR flash DT nodes upon discovery
Posted by Laszlo Ersek 3 years, 9 months ago
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


-=-=-=-=-=-=-=-=-=-=-=-
Groups.io Links: You receive all messages sent to this group.

View/Reply Online (#61694): https://edk2.groups.io/g/devel/message/61694
Mute This Topic: https://groups.io/mt/75079347/1787277
Group Owner: devel+owner@edk2.groups.io
Unsubscribe: https://edk2.groups.io/g/devel/unsub  [importer@patchew.org]
-=-=-=-=-=-=-=-=-=-=-=-

Re: [edk2-devel] [PATCH v2] ArmVirtPkg/NorFlashQemuLib: disable NOR flash DT nodes upon discovery
Posted by Philippe Mathieu-Daudé 3 years, 9 months ago
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 <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 >;
> }

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.
> 
> 


-=-=-=-=-=-=-=-=-=-=-=-
Groups.io Links: You receive all messages sent to this group.

View/Reply Online (#61660): https://edk2.groups.io/g/devel/message/61660
Mute This Topic: https://groups.io/mt/75079347/1787277
Group Owner: devel+owner@edk2.groups.io
Unsubscribe: https://edk2.groups.io/g/devel/unsub  [importer@patchew.org]
-=-=-=-=-=-=-=-=-=-=-=-

Re: [edk2-devel] [PATCH v2] ArmVirtPkg/NorFlashQemuLib: disable NOR flash DT nodes upon discovery
Posted by Ard Biesheuvel 3 years, 9 months ago
On 6/24/20 3:15 PM, Philippe Mathieu-Daudé wrote:
> 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 <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 >;
>> }
> 
> 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...
> 

Is it? The reg property contains two (base, size) tuples, and the driver 
seems to treat them as two individual flash chips.


-=-=-=-=-=-=-=-=-=-=-=-
Groups.io Links: You receive all messages sent to this group.

View/Reply Online (#61686): https://edk2.groups.io/g/devel/message/61686
Mute This Topic: https://groups.io/mt/75079347/1787277
Group Owner: devel+owner@edk2.groups.io
Unsubscribe: https://edk2.groups.io/g/devel/unsub  [importer@patchew.org]
-=-=-=-=-=-=-=-=-=-=-=-