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

["Ard Biesheuvel" <ard.biesheuvel@arm.com>]

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.