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

["Laszlo Ersek" <lersek@redhat.com>]

(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