From mboxrd@z Thu Jan 1 00:00:00 1970 Received: from foss.arm.com (foss.arm.com [217.140.110.172]) by mx.groups.io with SMTP id smtpd.web11.11535.1592998996671070495 for ; Wed, 24 Jun 2020 04:43:16 -0700 Authentication-Results: mx.groups.io; dkim=missing; spf=pass (domain: arm.com, ip: 217.140.110.172, mailfrom: ard.biesheuvel@arm.com) Received: from usa-sjc-imap-foss1.foss.arm.com (unknown [10.121.207.14]) by usa-sjc-mx-foss1.foss.arm.com (Postfix) with ESMTP id 5A4581F1; Wed, 24 Jun 2020 04:43:16 -0700 (PDT) Received: from [192.168.1.69] (unknown [10.37.8.29]) by usa-sjc-imap-foss1.foss.arm.com (Postfix) with ESMTPSA id 232C53F6CF; Wed, 24 Jun 2020 04:43:14 -0700 (PDT) Subject: Re: [edk2-devel] [PATCH] ArmVirtPkg/NorFlashQemuLib: disable NOR flash DT nodes upon discovery To: devel@edk2.groups.io, lersek@redhat.com Cc: =?UTF-8?Q?Philippe_Mathieu-Daud=c3=a9?= , Drew Jones , Eric Auger References: <20200623175700.1564281-1-ard.biesheuvel@arm.com> <85a63fc4-f3d1-1e17-bf1d-dace59bb02a8@arm.com> <2a43b904-5172-0fb3-6d40-e1fd3b652fe7@redhat.com> From: "Ard Biesheuvel" Message-ID: Date: Wed, 24 Jun 2020 13:43:07 +0200 User-Agent: Mozilla/5.0 (X11; Linux aarch64; rv:68.0) Gecko/20100101 Thunderbird/68.9.0 MIME-Version: 1.0 In-Reply-To: <2a43b904-5172-0fb3-6d40-e1fd3b652fe7@redhat.com> Content-Type: text/plain; charset=utf-8; format=flowed Content-Language: en-US Content-Transfer-Encoding: 7bit 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 > , 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.