From mboxrd@z Thu Jan 1 00:00:00 1970 Return-Path: Received: from mx1.redhat.com (mx1.redhat.com [209.132.183.28]) (using TLSv1.2 with cipher ECDHE-RSA-AES256-GCM-SHA384 (256/256 bits)) (No client certificate requested) by ml01.01.org (Postfix) with ESMTPS id 6B136817DE for ; Tue, 3 Jan 2017 06:46:11 -0800 (PST) Received: from int-mx11.intmail.prod.int.phx2.redhat.com (int-mx11.intmail.prod.int.phx2.redhat.com [10.5.11.24]) (using TLSv1.2 with cipher ECDHE-RSA-AES256-GCM-SHA384 (256/256 bits)) (No client certificate requested) by mx1.redhat.com (Postfix) with ESMTPS id DBB964F639; Tue, 3 Jan 2017 14:46:11 +0000 (UTC) Received: from lacos-laptop-7.usersys.redhat.com (ovpn-116-81.phx2.redhat.com [10.3.116.81]) by int-mx11.intmail.prod.int.phx2.redhat.com (8.14.4/8.14.4) with ESMTP id v03EkAoT004763; Tue, 3 Jan 2017 09:46:10 -0500 To: Bhupesh Sharma References: <1482729935-25823-1-git-send-email-bhsharma@redhat.com> Cc: edk2-devel@ml01.01.org, "Jordan Justen (Intel address)" , Ard Biesheuvel From: Laszlo Ersek Message-ID: <152b7f31-f4ed-3494-81d0-f57fe4beeced@redhat.com> Date: Tue, 3 Jan 2017 15:46:08 +0100 User-Agent: Mozilla/5.0 (X11; Linux x86_64; rv:45.0) Gecko/20100101 Thunderbird/45.6.0 MIME-Version: 1.0 In-Reply-To: <1482729935-25823-1-git-send-email-bhsharma@redhat.com> X-Scanned-By: MIMEDefang 2.68 on 10.5.11.24 X-Greylist: Sender IP whitelisted, not delayed by milter-greylist-4.5.16 (mx1.redhat.com [10.5.110.26]); Tue, 03 Jan 2017 14:46:11 +0000 (UTC) Subject: Re: [PATCH 1/1] Add BGRT ACPI table inclusion support in OVMF 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: Tue, 03 Jan 2017 14:46:11 -0000 Content-Type: text/plain; charset=windows-1252 Content-Transfer-Encoding: 7bit On 12/26/16 06:25, Bhupesh Sharma wrote: > During debugging Linux kernel for ACPI BGRT support > (especially on VMs), it is sometimes very useful to have > the EFI firmware (OVMF in most cases which use Tianocore) > to export the ACPI BGRT table. > > This patch tries to add this support. > > I have tested this patch on a RHEL7.3 and Fedora-25 KVM > and ensured that the BGRT logo is properly prepared and > can be viewed with user-space tools (like 'Gwenview' on KDE, > for example): > > $ file /sys/firmware/acpi/bgrt/image > /sys/firmware/acpi/bgrt/image: PC bitmap, Windows 3.x format, 193 x 58 x > 24 > > Cc: Laszlo Ersek > Signed-off-by: Bhupesh Sharma > Contributed-under: TianoCore Contribution Agreement 1.0 > --- > OvmfPkg/OvmfPkgIa32.dsc | 1 + > OvmfPkg/OvmfPkgIa32.fdf | 1 + > OvmfPkg/OvmfPkgIa32X64.dsc | 1 + > OvmfPkg/OvmfPkgIa32X64.fdf | 1 + > OvmfPkg/OvmfPkgX64.dsc | 1 + > OvmfPkg/OvmfPkgX64.fdf | 1 + > 6 files changed, 6 insertions(+) > > diff --git a/OvmfPkg/OvmfPkgIa32.dsc b/OvmfPkg/OvmfPkgIa32.dsc > index 81f7521..e97f7f0 100644 > --- a/OvmfPkg/OvmfPkgIa32.dsc > +++ b/OvmfPkg/OvmfPkgIa32.dsc > @@ -679,6 +679,7 @@ > OvmfPkg/AcpiTables/AcpiTables.inf > MdeModulePkg/Universal/Acpi/S3SaveStateDxe/S3SaveStateDxe.inf > MdeModulePkg/Universal/Acpi/BootScriptExecutorDxe/BootScriptExecutorDxe.inf > + MdeModulePkg/Universal/Acpi/BootGraphicsResourceTableDxe/BootGraphicsResourceTableDxe.inf > > # > # Network Support > diff --git a/OvmfPkg/OvmfPkgIa32.fdf b/OvmfPkg/OvmfPkgIa32.fdf > index 2a5b211..34d57a6 100644 > --- a/OvmfPkg/OvmfPkgIa32.fdf > +++ b/OvmfPkg/OvmfPkgIa32.fdf > @@ -278,6 +278,7 @@ INF OvmfPkg/AcpiPlatformDxe/AcpiPlatformDxe.inf > INF RuleOverride=ACPITABLE OvmfPkg/AcpiTables/AcpiTables.inf > INF MdeModulePkg/Universal/Acpi/S3SaveStateDxe/S3SaveStateDxe.inf > INF MdeModulePkg/Universal/Acpi/BootScriptExecutorDxe/BootScriptExecutorDxe.inf > +INF MdeModulePkg/Universal/Acpi/BootGraphicsResourceTableDxe/BootGraphicsResourceTableDxe.inf > > INF FatPkg/EnhancedFatDxe/Fat.inf > > diff --git a/OvmfPkg/OvmfPkgIa32X64.dsc b/OvmfPkg/OvmfPkgIa32X64.dsc > index f7855b6..8e3e04c 100644 > --- a/OvmfPkg/OvmfPkgIa32X64.dsc > +++ b/OvmfPkg/OvmfPkgIa32X64.dsc > @@ -688,6 +688,7 @@ > OvmfPkg/AcpiTables/AcpiTables.inf > MdeModulePkg/Universal/Acpi/S3SaveStateDxe/S3SaveStateDxe.inf > MdeModulePkg/Universal/Acpi/BootScriptExecutorDxe/BootScriptExecutorDxe.inf > + MdeModulePkg/Universal/Acpi/BootGraphicsResourceTableDxe/BootGraphicsResourceTableDxe.inf > > # > # Network Support > diff --git a/OvmfPkg/OvmfPkgIa32X64.fdf b/OvmfPkg/OvmfPkgIa32X64.fdf > index 1c7df21..df55c2b 100644 > --- a/OvmfPkg/OvmfPkgIa32X64.fdf > +++ b/OvmfPkg/OvmfPkgIa32X64.fdf > @@ -278,6 +278,7 @@ INF OvmfPkg/AcpiPlatformDxe/AcpiPlatformDxe.inf > INF RuleOverride=ACPITABLE OvmfPkg/AcpiTables/AcpiTables.inf > INF MdeModulePkg/Universal/Acpi/S3SaveStateDxe/S3SaveStateDxe.inf > INF MdeModulePkg/Universal/Acpi/BootScriptExecutorDxe/BootScriptExecutorDxe.inf > +INF MdeModulePkg/Universal/Acpi/BootGraphicsResourceTableDxe/BootGraphicsResourceTableDxe.inf > > INF FatPkg/EnhancedFatDxe/Fat.inf > > diff --git a/OvmfPkg/OvmfPkgX64.dsc b/OvmfPkg/OvmfPkgX64.dsc > index e933a41..6ec3fe0 100644 > --- a/OvmfPkg/OvmfPkgX64.dsc > +++ b/OvmfPkg/OvmfPkgX64.dsc > @@ -686,6 +686,7 @@ > OvmfPkg/AcpiTables/AcpiTables.inf > MdeModulePkg/Universal/Acpi/S3SaveStateDxe/S3SaveStateDxe.inf > MdeModulePkg/Universal/Acpi/BootScriptExecutorDxe/BootScriptExecutorDxe.inf > + MdeModulePkg/Universal/Acpi/BootGraphicsResourceTableDxe/BootGraphicsResourceTableDxe.inf > > # > # Network Support > diff --git a/OvmfPkg/OvmfPkgX64.fdf b/OvmfPkg/OvmfPkgX64.fdf > index 3bb11cb..5e2e1df 100644 > --- a/OvmfPkg/OvmfPkgX64.fdf > +++ b/OvmfPkg/OvmfPkgX64.fdf > @@ -278,6 +278,7 @@ INF OvmfPkg/AcpiPlatformDxe/AcpiPlatformDxe.inf > INF RuleOverride=ACPITABLE OvmfPkg/AcpiTables/AcpiTables.inf > INF MdeModulePkg/Universal/Acpi/S3SaveStateDxe/S3SaveStateDxe.inf > INF MdeModulePkg/Universal/Acpi/BootScriptExecutorDxe/BootScriptExecutorDxe.inf > +INF MdeModulePkg/Universal/Acpi/BootGraphicsResourceTableDxe/BootGraphicsResourceTableDxe.inf > > INF FatPkg/EnhancedFatDxe/Fat.inf > > (1) This is a good idea. I superficially checked the logic in "MdeModulePkg/Universal/Acpi/BootGraphicsResourceTableDxe/BootGraphicsResourceTableDxe.c", plus the code around the tree that connects with it in one way or another, and it seems sane. (2) Also, the fw binary footprint of the new module doesn't seem large, which is good. (3) Please change the subject line to: OvmfPkg: install BGRT ACPI table (4) Please also CC Jordan on the OvmfPkg patch, not just me. Consult Maintainers.txt. (5) The same feature would be beneficial to ArmVirtPkg as well. Please add a separate patch to the series that does the same for ArmVirtPkg. The files you should extend are: - ArmVirtPkg/ArmVirtQemu.dsc (under comment "ACPI Support") - ArmVirtPkg/ArmVirtQemuKernel.dsc (under comment "ACPI Support") - ArmVirtPkg/ArmVirtQemuFvMain.fdf.inc (under comment "ACPI Support") The subject line should be ArmVirtPkg/ArmVirtQemu: install BGRT ACPI table please CC Ard and me on the patch (see Maintainers.txt). (6) I would appreciate if you could repeat your BGRT image test in an aarch64 guest as well (for patch #2). We can discuss the necessary setup / guest installation steps separately. (It makes sense to keep at least one long-term aarch64 guest, TCG or KVM.) (7) Any chance you could regression-test the OvmfPkg change with some x86-64 Windows guests? Ideally, Windows Server 2008 R2, Windows 8[.1] and Windows 10 should be covered. Windows has a proprietary (non-ACPICA) ACPI interpreter, and such changes are best regression-tested against it as well. (8) Last but not least, welcome to Red Hat, Bhupesh -- I just noticed :) Laszlo