Re: [PATCH 1/1] Add BGRT ACPI table inclusion support in OVMF

[Laszlo Ersek <lersek@redhat.com>]

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 <lersek@redhat.com>
> Signed-off-by: Bhupesh Sharma <bhsharma@redhat.com>
> 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