From mboxrd@z Thu Jan 1 00:00:00 1970 Return-Path: Received: from mail-wm0-x230.google.com (mail-wm0-x230.google.com [IPv6:2a00:1450:400c:c09::230]) (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 CA761802B0 for ; Mon, 20 Mar 2017 05:13:56 -0700 (PDT) Received: by mail-wm0-x230.google.com with SMTP id t189so62632132wmt.1 for ; Mon, 20 Mar 2017 05:13:56 -0700 (PDT) DKIM-Signature: v=1; a=rsa-sha256; c=relaxed/relaxed; d=linaro.org; s=google; h=date:from:to:cc:subject:message-id:references:mime-version :content-disposition:in-reply-to:user-agent; bh=9AS9cDjiXlyYY0Vrzt7v7rDrPOX5jNZOWfob4q5Z0d0=; b=aC4O9hZyoGC8HjOfcc4QiHsQjqU/6gdJsZe/fJWLCx1UcvJWx9y40MBt0XCS46M0cE B5lIkw8HtpSMi5N2Yu36cdJKBUgMT0vztqPTleq1muYaAoZ1Z2yNuvKtVoSvj0JoQKqn H5uNZ1w/V6BYnozmcw6iTqzJfWQWHEpHmeW9A= X-Google-DKIM-Signature: v=1; a=rsa-sha256; c=relaxed/relaxed; d=1e100.net; s=20161025; h=x-gm-message-state:date:from:to:cc:subject:message-id:references :mime-version:content-disposition:in-reply-to:user-agent; bh=9AS9cDjiXlyYY0Vrzt7v7rDrPOX5jNZOWfob4q5Z0d0=; b=nWzdJg1gu4+2sZ8TC1I07C73fOozrAzbFBbTm8sWN2Os/RIMDIu8t3V6Lv4ah4cxan /TB+1vWMpSqjXqR6jdm7z4i2gnmugqAV8+8IAco+FRwm2WCpHbc/SopADH4+yhmJY6ab taiztgQeDfn/eko+2qDsNHQQh5XWP1uQ90Sre6CUWzZmBu6gE0cIbYlrDe6mr7rYlXT2 3q+sedGQkohWiKAZrPqKQVM2aXAUQGhA9P7tB1Zli4bBe5/x2oLpRpgd4J33XBOg6RnG F1/Nn+q5pJ6Bu4FNHuqVz+1kzZDC2oh9sRSObCT/oOyadsIS2yZI2ZnxLS7Yj7M7koi5 Rz4w== X-Gm-Message-State: AFeK/H2KM9Nb1+vuvEtkWSxTU67AeeveaLewcRf/qGeJ2GF3tgzHafHO88q4Kc3eo6k7byX9 X-Received: by 10.28.194.7 with SMTP id s7mr9784300wmf.34.1490012034875; Mon, 20 Mar 2017 05:13:54 -0700 (PDT) Received: from bivouac.eciton.net (bivouac.eciton.net. [2a00:1098:0:86:1000:23:0:2]) by smtp.gmail.com with ESMTPSA id p12sm20687167wrb.46.2017.03.20.05.13.53 (version=TLS1_2 cipher=ECDHE-RSA-AES128-GCM-SHA256 bits=128/128); Mon, 20 Mar 2017 05:13:54 -0700 (PDT) Date: Mon, 20 Mar 2017 12:13:52 +0000 From: Leif Lindholm To: Ard Biesheuvel Cc: edk2-devel@lists.01.org, lersek@redhat.com, star.zeng@intel.com, feng.tian@intel.com Message-ID: <20170320121352.GO16034@bivouac.eciton.net> References: <20170318205824.13326-1-ard.biesheuvel@linaro.org> MIME-Version: 1.0 In-Reply-To: <20170318205824.13326-1-ard.biesheuvel@linaro.org> User-Agent: Mutt/1.5.23 (2014-03-12) Subject: Re: [RFC PATCH] MdeModulePkg/AcpiTableDxe: inhibit table publication until we have a FADT X-BeenThere: edk2-devel@lists.01.org X-Mailman-Version: 2.1.22 Precedence: list List-Id: EDK II Development List-Unsubscribe: , List-Archive: List-Post: List-Help: List-Subscribe: , X-List-Received-Date: Mon, 20 Mar 2017 12:13:57 -0000 Content-Type: text/plain; charset=us-ascii Content-Disposition: inline On Sat, Mar 18, 2017 at 08:58:24PM +0000, Ard Biesheuvel wrote: > Since commit 78c41ff519b1 ("ArmVirtPkg/FdtClientDxe: make DT table > installation !ACPI dependent"), we inhibit the installation of the core > ACPI tables when booting in DT mode, to relieve the OS from having to > decide which hardware description to use. However, as it turns out, > in presence of the RamdiskDxe or BGRT drivers, some ACPI tables are > still registered with the protocol, and published under the ACPI entry > point configuration tables, ignoring the fact that there is no way the > OS can boot with only NFIT and BGRT tables present. > > So let's only publish the ACPI tables if we can reasonably assume that > the tables that the OS requires have been registered with the protocol, > by checking that we have either FADT1 or FADT3 table (or both) before > installing the configuration tables. > > Contributed-under: TianoCore Contribution Agreement 1.0 > Signed-off-by: Ard Biesheuvel > --- > > This is a counterproposal for Laszlo's series [0], which is technically > sound, but a bit unwieldy. Instead of making ARM the odd one out, and > tweaking universal modules via NULL library class resolution, we can > prevent AcpiTableDxe from publishing the entry points when only NFIT > or BGRT tables are present (which sounds like a strange thing to do > in any case) > > I am aware that this still leaves some loose ends when it comes to ordering > and the ReadyToBoot callback, which I agree we should solve regardless, but > I hope we can do so without putting the burden on the ARM platforms to > always carry a set of NULL class libraries to keep sane behavior. > > [0] https://lists.01.org/pipermail/edk2-devel/2017-March/008684.html > > MdeModulePkg/Universal/Acpi/AcpiTableDxe/AcpiTableProtocol.c | 15 +++++++++++++++ > 1 file changed, 15 insertions(+) > > diff --git a/MdeModulePkg/Universal/Acpi/AcpiTableDxe/AcpiTableProtocol.c b/MdeModulePkg/Universal/Acpi/AcpiTableDxe/AcpiTableProtocol.c > index 4bb848df5203..4c9921f69a8a 100644 > --- a/MdeModulePkg/Universal/Acpi/AcpiTableDxe/AcpiTableProtocol.c > +++ b/MdeModulePkg/Universal/Acpi/AcpiTableDxe/AcpiTableProtocol.c > @@ -130,6 +130,21 @@ PublishTables ( > UINT64 Buffer64; > > // > + // Don't publish anything yet if we don't have a FADT table. This table is > + // mandatory for all ACPI compatible OSes, and installing the ACPI entry > + // point configuration table without a full set of ACPI tables may confuse > + // some OSes (Linux/arm64). (This may happen when the BGRT or ramdisk drivers > + // publish their respective ACPI tables without regard for whether ACPI boot > + // is currently enabled.) > + // > + if (AcpiTableInstance->Fadt1 == NULL && AcpiTableInstance->Fadt3 == NULL) { While I realise that this argument is the weakest it can possibly be in this source file and function ... this type of late-stage content inspection feels to me like it breaks the modular design intended. I guess more importantly, permitting modules to successfully install tables and then not publish them obstructs useful/helpful handling of known-to-be-fatal errors. Or, in short, I would much rather see a solution that makes AcpiTableDxe unavailable. If there was a way to prevent installation of alternative implementations with gEfiAcpiTableProtocolGuid, that would be even better. Regards, Leif > + DEBUG ((DEBUG_INFO, > + "%a: not publishing ACPI tables [yet], no FADT table installed\n", > + __FUNCTION__)); > + return EFI_SUCCESS; > + } > + > + // > // Reorder tables as some operating systems don't seem to find the > // FADT correctly if it is not in the first few entries > // > -- > 2.9.3 >