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

[Andrew Jones <drjones@redhat.com>]

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