From mboxrd@z Thu Jan 1 00:00:00 1970 Return-Path: Received: from mail-qt0-x244.google.com (mail-qt0-x244.google.com [IPv6:2607:f8b0:400d:c0d::244]) (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 C940281944 for ; Wed, 4 Jan 2017 11:34:48 -0800 (PST) Received: by mail-qt0-x244.google.com with SMTP id 3so40905597qtr.2 for ; Wed, 04 Jan 2017 11:34:48 -0800 (PST) DKIM-Signature: v=1; a=rsa-sha256; c=relaxed/relaxed; d=gmail.com; s=20161025; h=mime-version:in-reply-to:references:from:date:message-id:subject:to :cc; bh=DfpOXdCMYORr9/0wRHa4VjC5r0tayONbv9HfhtPOx68=; b=fO+bijjNYi1tgKHYoUBbJ4m8ZD5GPX8xipkhdOFb4/gxaqRLiUowvv54cLHb2c06G6 rW5ArXv/Yei3bzZsHEWEKCxE+syYOeMH3IQqiNmzkaX+LS719u/YEL6EUlPnqcwLJ0fB oh/G49W7fbCfOk+jz5DDjdXsqEAoI7vGCSahQ8Jlkd7DYvmeCcw0/RDdMXpLpZ+B9U+V u7phlJAtNO5bRtskAxe8KaJK+D+vOINqMDJ8FWn/aDCCRW52jwOAzXDYFw3+ArdvdjVA 0S0UmhofWVLsUBoqFjsxUt/JJNfBYKUxAREduGNeUnAOO3NWcIwiSc3KZFEl4yzyT6mo kxGQ== 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; bh=DfpOXdCMYORr9/0wRHa4VjC5r0tayONbv9HfhtPOx68=; b=TyBybsAl/46e+keEjYjS3dHP3EBhL1U7tx0mqJF+mHgStE5vgok7zDZ3Qln82tbWlB bq5eMP+IBeV7zYAt2jsEQQeAN7UrAhKGmtJC9Hpc+llBAvmh7csyTt+LCO4vxveoLTyl 498fSyNRtxIsD8jKa24RG2spM+lBtiaGVWV0tbKb66Fs6Q9F3foeREtunC8PXCdfD0IV 9nAa2QwPDPFepuZ5686+lwqc1j5RpDhAQmFHd71/h/Fg2qnSKCF/DZjByKCWgf8JnYHe w4CupJh8Rkh4XLu9wj2vnIz4sGdbv0dyM+8DaMv77Jw7lsnKDOQEG+j5lUVfmxieimI8 y8yw== X-Gm-Message-State: AIkVDXKp4ezdGBsR0cvSlNe1S6aI8FnOFA8d4YPpZIJZs/TiQJxG6HhNk0D4ieNtAVYD6IphJZWMqK4E7AF9Rw== X-Received: by 10.200.40.179 with SMTP id i48mr70100056qti.42.1483558487662; Wed, 04 Jan 2017 11:34:47 -0800 (PST) MIME-Version: 1.0 Received: by 10.140.101.10 with HTTP; Wed, 4 Jan 2017 11:34:47 -0800 (PST) In-Reply-To: <152b7f31-f4ed-3494-81d0-f57fe4beeced@redhat.com> References: <1482729935-25823-1-git-send-email-bhsharma@redhat.com> <152b7f31-f4ed-3494-81d0-f57fe4beeced@redhat.com> From: Bhupesh SHARMA Date: Thu, 5 Jan 2017 01:04:47 +0530 Message-ID: To: Laszlo Ersek Cc: Bhupesh Sharma , "Jordan Justen (Intel address)" , edk2-devel@ml01.01.org, Ard Biesheuvel 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: Wed, 04 Jan 2017 19:34:49 -0000 Content-Type: text/plain; charset=UTF-8 Hi Laszlo, Many thanks for taking out the time to review this. On Tue, Jan 3, 2017 at 8:16 PM, Laszlo Ersek wrote: > 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 Sure. > (4) Please also CC Jordan on the OvmfPkg patch, not just me. Consult > Maintainers.txt. Oops. Sorry for missing out Jordan. > (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). Sure, will spin that as a separate patch. > (6) I would appreciate if you could repeat your BGRT image test in an > aarch64 guest as well (for patch #2). Sure I am trying to get hold of a AARCH64 board - will test the 2nd patch on the same and confirm the test results before sending out the 2nd patch. > 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. I am also trying to install a Windows 10 guest at my end. Will try to check the OvmfPkg change on the same before sending a v2. > 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 :) Many thanks Laszlo :) > > Laszlo > _______________________________________________ > edk2-devel mailing list > edk2-devel@lists.01.org > https://lists.01.org/mailman/listinfo/edk2-devel Regards, Bhupesh