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

[Bhupesh SHARMA <bhupesh.linux@gmail.com>]

Hi Laszlo,

Many thanks for taking out the time to review this.

On Tue, Jan 3, 2017 at 8:16 PM, Laszlo Ersek <lersek@redhat.com> 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 <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

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