From mboxrd@z Thu Jan 1 00:00:00 1970 Received: from us-smtp-delivery-1.mimecast.com (us-smtp-delivery-1.mimecast.com [205.139.110.61]) by mx.groups.io with SMTP id smtpd.web12.13951.1593006372248698246 for ; Wed, 24 Jun 2020 06:46:12 -0700 Authentication-Results: mx.groups.io; dkim=pass header.i=@redhat.com header.s=mimecast20190719 header.b=d3+Huyif; spf=pass (domain: redhat.com, ip: 205.139.110.61, mailfrom: philmd@redhat.com) DKIM-Signature: v=1; a=rsa-sha256; c=relaxed/relaxed; d=redhat.com; s=mimecast20190719; t=1593006371; h=from:from:reply-to:subject:subject:date:date:message-id:message-id: to:to:cc:cc:mime-version:mime-version:content-type:content-type: content-transfer-encoding:content-transfer-encoding: in-reply-to:in-reply-to:references:references; bh=By8J7GtbgW8TwKWG9eGs2VRSXwOOpWtWfsbI70cEjrw=; b=d3+HuyifLipxtgFw1P+pHGKkx8NG0Z82MYaXuNRhbgddWZvs8c/pKYcs7U95U5DnLB75fJ GUPgVTWnWFFBu0TDOKjKamT3Alph8QW7Ac2aYStvD+o5jISny8B08IGLu5CzXsSaJEkYUf vBGuPIdtcAfv5PK7wdtCdscrtNDX+/U= Received: from mail-ot1-f71.google.com (mail-ot1-f71.google.com [209.85.210.71]) (Using TLS) by relay.mimecast.com with ESMTP id us-mta-426-J89wNOKkP7eIaJZWq5MpKQ-1; Wed, 24 Jun 2020 09:46:09 -0400 X-MC-Unique: J89wNOKkP7eIaJZWq5MpKQ-1 Received: by mail-ot1-f71.google.com with SMTP id y8so1436491ote.3 for ; Wed, 24 Jun 2020 06:46:09 -0700 (PDT) X-Google-DKIM-Signature: v=1; a=rsa-sha256; c=relaxed/relaxed; d=1e100.net; s=20161025; h=x-gm-message-state:mime-version:references:in-reply-to:from:date :message-id:subject:to:cc:content-transfer-encoding; bh=By8J7GtbgW8TwKWG9eGs2VRSXwOOpWtWfsbI70cEjrw=; b=R1HPewWrcDmT6/QAtywojrM1O8hYhDZBEwDZaQQJeVCG5Mw6DfB5HUxN/U42hrq70n s3YRWN3jF5vmTkK0KlJRPKPpcGTt3/huPebTWPIw+SmmlJj+H9sJwiJk5NJvcX0ZFLwR jx5I6l6GuSS8iUcuv0Ooc8f/+tny9xF6bDwv5tKuz+pXiGw57VII2b9MtbVF195pkb/N 858bBjZXYQStXugLxivzq6fD9vStq2fqjit1k1tU3EYayVqs6MK3Gr7Ts8LFHLyBm8g6 pIIx2d+L5bcfR2BlvLjetuTPqWo3hiuL0IPTYFm9vF8rJiVRT1rDM6nrVZ8zlqkM2Dmj t8Aw== X-Gm-Message-State: AOAM532H0PgNLQsNSFn886/BhTzOkD9PLzQG5b+hHKmQATb54llBUqF6 ghboyjQnYXMZGE3imikTlC4+BHanyMSeRKe4IGttrE/xu/TMi9/N/Zqll6B4GBrzmL+K0DR1A3L cC/OJwGfg65xvuNI6U/D2+LEawbQJJQ== X-Received: by 2002:a9d:12f7:: with SMTP id g110mr23400741otg.79.1593006368744; Wed, 24 Jun 2020 06:46:08 -0700 (PDT) X-Google-Smtp-Source: ABdhPJwlEJcjlcEQ7fNcJkoL8e4yBamN3pVRaJ5JRrO8QLl9eTBUYg+mUsruA09A6BvZrtiKHAkvzP3200IDJmjat9M= X-Received: by 2002:a9d:12f7:: with SMTP id g110mr23400713otg.79.1593006368420; Wed, 24 Jun 2020 06:46:08 -0700 (PDT) MIME-Version: 1.0 References: <20200623175700.1564281-1-ard.biesheuvel@arm.com> <85a63fc4-f3d1-1e17-bf1d-dace59bb02a8@arm.com> <2a43b904-5172-0fb3-6d40-e1fd3b652fe7@redhat.com> <20200624134154.isbcqvscs7eidbd4@kamzik.brq.redhat.com> In-Reply-To: <20200624134154.isbcqvscs7eidbd4@kamzik.brq.redhat.com> From: =?UTF-8?B?UGhpbGlwcGUgTWF0aGlldS1EYXVkw6k=?= Date: Wed, 24 Jun 2020 15:45:57 +0200 Message-ID: Subject: Re: [edk2-devel] [PATCH] ArmVirtPkg/NorFlashQemuLib: disable NOR flash DT nodes upon discovery To: Andrew Jones Cc: Ard Biesheuvel , edk2-devel-groups-io , Laszlo Ersek , Eric Auger X-Mimecast-Spam-Score: 0 X-Mimecast-Originator: redhat.com Content-Type: text/plain; charset="UTF-8" Content-Transfer-Encoding: quoted-printable On Wed, Jun 24, 2020 at 3:42 PM Andrew Jones wrote: > On Wed, Jun 24, 2020 at 03:02:49PM +0200, Philippe Mathieu-Daud=C3=A9 wro= te: > > 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 t= he > > >>> 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 whet= her > > >>> it was always like that (for multi_v7_defconfig) but there is no go= od > > >>> 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 pfl= ash > > >> 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 =3D 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 =3D MACHINE(vms); > > >>> const MemMapEntry *memmap =3D vms->memmap; > > >>> const int *irqmap =3D vms->irqmap; > > >>> > > >>> dsdt =3D 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 =3D 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 comm= it > > >> 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 syste= m > > >> config table), so in this function, the presence of UEFI can be assu= med > > >> "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 y= ou > > > 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 top= ic :)