From mboxrd@z Thu Jan 1 00:00:00 1970 Return-Path: Received: from mail-io0-x235.google.com (mail-io0-x235.google.com [IPv6:2607:f8b0:4001:c06::235]) (using TLSv1.2 with cipher ECDHE-RSA-AES128-GCM-SHA256 (128/128 bits)) (No client certificate requested) by ml01.01.org (Postfix) with ESMTPS id 000DB803A5 for ; Mon, 13 Mar 2017 13:55:13 -0700 (PDT) Received: by mail-io0-x235.google.com with SMTP id g6so92401624ioj.1 for ; Mon, 13 Mar 2017 13:55:13 -0700 (PDT) DKIM-Signature: v=1; a=rsa-sha256; c=relaxed/relaxed; d=linaro.org; s=google; h=mime-version:in-reply-to:references:from:date:message-id:subject:to :cc:content-transfer-encoding; bh=yDO+8RblyIg0LDizllo1wwW8W/PZi+l90HdpFxTiZws=; b=IczFgj9L68MBbgk4N7XRg80Ov40o5XV1I8CbKQKR3bQe/AHmjSpWUR/6XUpu+Ii29U o4ZpM6r8sXv5LoqEw0VRh78x/8iDw0EoZodhs/htzlfF3U3c7I/9JBYvjVjiX6VbRwfG ft48GB57ZrfKtuvhDnysM1yGUo73H8faAWgaM= X-Google-DKIM-Signature: v=1; a=rsa-sha256; c=relaxed/relaxed; d=1e100.net; s=20161025; h=x-gm-message-state:mime-version:in-reply-to:references:from:date :message-id:subject:to:cc:content-transfer-encoding; bh=yDO+8RblyIg0LDizllo1wwW8W/PZi+l90HdpFxTiZws=; b=IIkxk32QgIqk5e7kI7Z23PNg9omiipgWQYf1gOwZks6pQYO0sDRyvqkcqS/+0TiO2s uAZISBW0wxfBQQAgU0tY0I2qd/804XgVM0j87leCgGd+WT1jIN2lBLWDilkdi3LGms5E uELCsq67ChfLM3n7V6H84ZpCvVapyGGL8ECd7/i5C3dGqqjDoNJqN0Bt6I+vGkzU6hBc WCnMVb6+ggeFFdwfF5NhuC2JRfrP4Wx9Nh84vkOsuUaz/ZNdnK+f6AoY0QhTXodRa2WZ qSKOLARYecciXMZC2rHTt0zRPSr1MD4DfiDq4Rn7nPHhR+IAEHUWNr1x/uBqCEWqngJq ipgA== X-Gm-Message-State: AMke39kMywK5N6O5WBUQFDua+HdQ2TzYjCP9CUcv7edakhJa0QsxDLPGxixOqGuzpbxnVi7iEFmZ8R0yM3pKzxdB X-Received: by 10.107.168.21 with SMTP id r21mr27358839ioe.45.1489438513024; Mon, 13 Mar 2017 13:55:13 -0700 (PDT) MIME-Version: 1.0 Received: by 10.107.10.27 with HTTP; Mon, 13 Mar 2017 13:55:12 -0700 (PDT) In-Reply-To: References: <1489075441-23745-1-git-send-email-ard.biesheuvel@linaro.org> <1489075441-23745-4-git-send-email-ard.biesheuvel@linaro.org> <19f70c3d-4220-957b-24cd-bd2d41e45909@redhat.com> <5bdc3cc1-b292-8da3-f858-83c752016941@redhat.com> <20170311102640.GK16034@bivouac.eciton.net> <7561917e-fc1c-1675-e672-e0f8b716e4c4@redhat.com> From: Ard Biesheuvel Date: Mon, 13 Mar 2017 20:55:12 +0000 Message-ID: To: Laszlo Ersek Cc: Leif Lindholm , "edk2-devel@lists.01.org" , Andrew Jones Subject: Re: [PATCH 3/3] ArmVirtPkg/FdtClientDxe: make DT table installation !ACPI dependent X-BeenThere: edk2-devel@lists.01.org X-Mailman-Version: 2.1.21 Precedence: list List-Id: EDK II Development List-Unsubscribe: , List-Archive: List-Post: List-Help: List-Subscribe: , X-List-Received-Date: Mon, 13 Mar 2017 20:55:14 -0000 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: quoted-printable On 13 March 2017 at 20:39, Laszlo Ersek wrote: > On 03/13/17 15:59, Ard Biesheuvel wrote: >> On 13 March 2017 at 14:53, Laszlo Ersek wrote: >>> On 03/13/17 11:23, Ard Biesheuvel wrote: >>>> On 11 March 2017 at 10:26, Leif Lindholm wr= ote: >>>>> On Sat, Mar 11, 2017 at 08:38:19AM +0100, Laszlo Ersek wrote: >>>>>>>> "MdeModulePkg/Universal/Acpi/S3SaveStateDxe/AcpiS3ContextSave.c". = I >>>>>>>> think the above check should be reworked to look for the FADT >>>>>>>> (EFI_ACPI_2_0_FIXED_ACPI_DESCRIPTION_TABLE_SIGNATURE) with code li= fted >>>>>>>> from these helper functions. No driver outside of >>>>>>>> QemuFwCfgAcpiPlatformDxe will install the FADT. And, the FADT will >>>>>>>> always be part of QEMU's ACPI payload, if it generates one. >>>>>>> >>>>>>> OK, that would get things working again, I suppose. But do we want >>>>>>> neutered ACPI tables to be exposed at all, even if there is a DT in >>>>>>> that case to boot from? >>>>>> >>>>>> I think the neutered ACPI tables (on -no-acpi) should be fine. The >>>>>> upstream Linux guest will prefer DT if it is present; >>>>> >>>>> Yes, but we've already determined that this situation is suboptimal, >>>>> which was what triggered this changeset to begin with. >>>>> >>>> >>>> Indeed. Having an ACPI entry point config table installed, but not >>>> having the essential tables that allow you to boot is a situation that >>>> we should try very hard to avoid IMO >>>> >>> >>> That requires dynamic disabling of AcpiTableDxe -- so that EFI_ACPI_TAB= LE_PROTOCOL is not produced, despite the driver being loaded --, and equipp= ing all clients of EFI_ACPI_TABLE_PROTOCOL to fail gracefully when the prot= ocol is not found. Alternatively, let AcpiTableDxe install the protocol, bu= t prevent all clients from calling it. >>> >> >> Not really. We only care about what the OS gets to see, not what the >> firmware ends up doing internally. So, say, at ReadyToBoot, we could >> check that the ACPI entry point points to what we need minimally to >> boot successfully, otherwise we rip it out. This will trigger the >> recently added change to install the DT if no ACPI entry point was >> found. >> >> Not pretty, I know, > > That's an understatement. > >> but in this case, the handover state when invoking >> the OS is much more important than clean handling in the firmware >> itself. > > The goal of the firmware is to boot the OS. Therefore the above argument > could be used to justify anything and everything at all in the firmware. > > Anyway, here's a technical counter-argument too (i.e., against ripping > out ACPI altogether at ReadyToBoot): the ArmVirtPkg platforms include > > MdeModulePkg/Universal/Disk/RamDiskDxe/RamDiskDxe.inf > > In this driver, in function RamDiskDxeEntryPoint() > [MdeModulePkg/Universal/Disk/RamDiskDxe/RamDiskDriver.c], we have a call > to EfiCreateEventReadyToBootEx(), which registers the RamDiskAcpiCheck() > callback function. > > The leading comment on the RamDiskAcpiCheck() function (rewrapped here > for readability) is: > > Check whether EFI_ACPI_TABLE_PROTOCOL and EFI_ACPI_SDT_PROTOCOL are > produced. If both protocols are produced, publish all the reserved > memory type RAM disks to the NVDIMM Firmware Interface Table (NFIT). > > We do have both protocols available; see commit d36447418d32 > ("ArmVirtPkg: Add Ramdisk support to ArmVirtPkg platforms", 2016-08-19). > > In turn, RamDiskPublishNfit() > [MdeModulePkg/Universal/Disk/RamDiskDxe/RamDiskProtocol.c]: > - removes the NFIT and installs an updated one, if an NFIT exists to > begin with, > - creates both an SSDT (with RamDiskPublishSsdt()) and an NFIT, otherwise= . > > If RamDiskAcpiCheck() --> RamDiskPublishNfit() run after our callback -- > in which we'd null the ACPI root pointer in the sysconfig table, IIUC > --, then further use of the ACPI protocols > - might crash, > - might transparently undo our nulling, > - or might even do what we want (that is, "nothing"). > > To me the behavior looks unpredictable. > Talk about understatements :-) > Based on RamDiskDxe's workings, perhaps it is safe to say that the edk2 > drivers equate the presence of the ACPI protocols to saying "this is an > ACPI system". If that's indeed the case, perhaps we need to make the > ACPI protocols *not* appear, dynamically. > > Here's an idea how to implement that, cleanly: > > (1) Introduce two new (empty / NULL) protocols with GUIDs > gAcpiSystemProtocolGuid and gDtSystemProtocolGuid. > > (2) Introduce a plugin library instance (=3D=3D no explicit class) that h= as > an empty constructor function (returning RETURN_SUCCESS), a DEPEX on > gAcpiSystemProtocolGuid, and nothing else. Let's call this AcpiSystemLib. > > (3) In "ArmVirt.dsc.inc", hook AcpiSystemLib into > "MdeModulePkg/Universal/Acpi/AcpiTableDxe/AcpiTableDxe.inf", via NULL > class resolution. This will imbue AcpiTableDxe with a DEPEX on > gAcpiSystemProtocolGuid. > > (4) In FdtClientDxe, register a protocol notify callback for > gDtSystemProtocolGuid. In the callback, install the DT as a sysconfig tab= le. > > (5) Introduce a new DXE driver, called AcpiDtSystemDxe, which only > depends on QemuFwCfgLib (cleanly, again), which in turn makes it depend > on gFdtClientProtocolGuid. In AcpiDtSystemDxe's entry point, look up the > "etc/table-loader" fw_cfg file. > > If the file exists (don't select or download it! just check the > existence), install gAcpiSystemProtocolGuid, and exit. That will unblock > AcpiTableDxe, and everything that depends on the ACPI protocols (via > depex or protocol notify). > > Otherwise, install gDtSystemProtocolGuid in the entry point, and exit. > That will cause FdtClientDxe to install the sysconfig table. > > > This design takes a few new modules, but: > - it operates only with valid edk2 tools and means, zero hacks, > - it depends on no ReadyToBoot callbacks, > - it needs no modifications for core drivers, > - it does not try to remove resources once published. > > Note that step (5) will turn Xen guests into constantly DT-only systems. > I don't know if that's appropriate. I guess Xen can expose another knob > (different from fw_cfg) to make this switch work. > I think the desire is to support ACPI on Xen/arm64 hosts, but it does not seem to me that the current code would do anything useful in that regard. > Alternatively, if you want to stick with the status quo for Xen (both > ACPI and DT), implement XenAcpiDtSystemDxe, which would install *both* > protocols unconditionally. Then you'll get both DT and ACPI in the Xen > guest, same as now, and XenAcpiDtSystemDxe would be subject for further > customization. > > If you want me to, I can work on this (I have a downstream RHBZ for the > feature, so I can justify spending time on this, even in the current > phase/state of RHEL development). I think I would start with reverting > patches #3 and #2 in this series. > > If you agree with the idea and prefer to work on it yourself, that's > even better. > > What do you think? > I like it! There is very little new logic or processing required at runtime, and everything else is managed by DxeCore's protocol dispatch. I will look into this tomorrow.