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

["Philippe Mathieu-Daudé" <philmd@redhat.com>]

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