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

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

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 :)