ArmVirtPkg/Library/NorFlashQemuLib/NorFlashQemuLib.c | 12 ++++++++++++ 1 file changed, 12 insertions(+)
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
-=-=-=-=-=-=-=-=-=-=-=-
Groups.io Links: You receive all messages sent to this group.
View/Reply Online (#61614): https://edk2.groups.io/g/devel/message/61614
Mute This Topic: https://groups.io/mt/75065345/1787277
Group Owner: devel+owner@edk2.groups.io
Unsubscribe: https://edk2.groups.io/g/devel/unsub [importer@patchew.org]
-=-=-=-=-=-=-=-=-=-=-=-
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
-=-=-=-=-=-=-=-=-=-=-=-
Groups.io Links: You receive all messages sent to this group.
View/Reply Online (#61618): https://edk2.groups.io/g/devel/message/61618
Mute This Topic: https://groups.io/mt/75065345/1787277
Group Owner: devel+owner@edk2.groups.io
Unsubscribe: https://edk2.groups.io/g/devel/unsub [importer@patchew.org]
-=-=-=-=-=-=-=-=-=-=-=-
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.
-=-=-=-=-=-=-=-=-=-=-=-
Groups.io Links: You receive all messages sent to this group.
View/Reply Online (#61635): https://edk2.groups.io/g/devel/message/61635
Mute This Topic: https://groups.io/mt/75065345/1787277
Group Owner: devel+owner@edk2.groups.io
Unsubscribe: https://edk2.groups.io/g/devel/unsub [importer@patchew.org]
-=-=-=-=-=-=-=-=-=-=-=-
(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
-=-=-=-=-=-=-=-=-=-=-=-
Groups.io Links: You receive all messages sent to this group.
View/Reply Online (#61649): https://edk2.groups.io/g/devel/message/61649
Mute This Topic: https://groups.io/mt/75065345/1787277
Group Owner: devel+owner@edk2.groups.io
Unsubscribe: https://edk2.groups.io/g/devel/unsub [importer@patchew.org]
-=-=-=-=-=-=-=-=-=-=-=-
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.
-=-=-=-=-=-=-=-=-=-=-=-
Groups.io Links: You receive all messages sent to this group.
View/Reply Online (#61650): https://edk2.groups.io/g/devel/message/61650
Mute This Topic: https://groups.io/mt/75065345/1787277
Group Owner: devel+owner@edk2.groups.io
Unsubscribe: https://edk2.groups.io/g/devel/unsub [importer@patchew.org]
-=-=-=-=-=-=-=-=-=-=-=-
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.
>
>
>
-=-=-=-=-=-=-=-=-=-=-=-
Groups.io Links: You receive all messages sent to this group.
View/Reply Online (#61659): https://edk2.groups.io/g/devel/message/61659
Mute This Topic: https://groups.io/mt/75065345/1787277
Group Owner: devel+owner@edk2.groups.io
Unsubscribe: https://edk2.groups.io/g/devel/unsub [importer@patchew.org]
-=-=-=-=-=-=-=-=-=-=-=-
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
-=-=-=-=-=-=-=-=-=-=-=-
Groups.io Links: You receive all messages sent to this group.
View/Reply Online (#61695): https://edk2.groups.io/g/devel/message/61695
Mute This Topic: https://groups.io/mt/75065345/1787277
Group Owner: devel+owner@edk2.groups.io
Unsubscribe: https://edk2.groups.io/g/devel/unsub [importer@patchew.org]
-=-=-=-=-=-=-=-=-=-=-=-
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 :)
-=-=-=-=-=-=-=-=-=-=-=-
Groups.io Links: You receive all messages sent to this group.
View/Reply Online (#61677): https://edk2.groups.io/g/devel/message/61677
Mute This Topic: https://groups.io/mt/75065345/1787277
Group Owner: devel+owner@edk2.groups.io
Unsubscribe: https://edk2.groups.io/g/devel/unsub [importer@patchew.org]
-=-=-=-=-=-=-=-=-=-=-=-
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. -=-=-=-=-=-=-=-=-=-=-=- Groups.io Links: You receive all messages sent to this group. View/Reply Online (#61678): https://edk2.groups.io/g/devel/message/61678 Mute This Topic: https://groups.io/mt/75065345/1787277 Group Owner: devel+owner@edk2.groups.io Unsubscribe: https://edk2.groups.io/g/devel/unsub [importer@patchew.org] -=-=-=-=-=-=-=-=-=-=-=-
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 -=-=-=-=-=-=-=-=-=-=-=- Groups.io Links: You receive all messages sent to this group. View/Reply Online (#61696): https://edk2.groups.io/g/devel/message/61696 Mute This Topic: https://groups.io/mt/75065345/1787277 Group Owner: devel+owner@edk2.groups.io Unsubscribe: https://edk2.groups.io/g/devel/unsub [importer@patchew.org] -=-=-=-=-=-=-=-=-=-=-=-
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 -=-=-=-=-=-=-=-=-=-=-=- Groups.io Links: You receive all messages sent to this group. View/Reply Online (#61692): https://edk2.groups.io/g/devel/message/61692 Mute This Topic: https://groups.io/mt/75065345/1787277 Group Owner: devel+owner@edk2.groups.io Unsubscribe: https://edk2.groups.io/g/devel/unsub [importer@patchew.org] -=-=-=-=-=-=-=-=-=-=-=-
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 -=-=-=-=-=-=-=-=-=-=-=- Groups.io Links: You receive all messages sent to this group. View/Reply Online (#61693): https://edk2.groups.io/g/devel/message/61693 Mute This Topic: https://groups.io/mt/75065345/1787277 Group Owner: devel+owner@edk2.groups.io Unsubscribe: https://edk2.groups.io/g/devel/unsub [importer@patchew.org] -=-=-=-=-=-=-=-=-=-=-=-
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
-=-=-=-=-=-=-=-=-=-=-=-
Groups.io Links: You receive all messages sent to this group.
View/Reply Online (#61638): https://edk2.groups.io/g/devel/message/61638
Mute This Topic: https://groups.io/mt/75065345/1787277
Group Owner: devel+owner@edk2.groups.io
Unsubscribe: https://edk2.groups.io/g/devel/unsub [importer@patchew.org]
-=-=-=-=-=-=-=-=-=-=-=-
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
>
-=-=-=-=-=-=-=-=-=-=-=-
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/1787277
Group Owner: devel+owner@edk2.groups.io
Unsubscribe: https://edk2.groups.io/g/devel/unsub [importer@patchew.org]
-=-=-=-=-=-=-=-=-=-=-=-
© 2016 - 2024 Red Hat, Inc.