Re: [Qemu-devel] [edk2 PATCH 1/1] OvmfPkg/AcpiPlatformDxe: download ACPI tables from QEMU
On Fri, Nov 22, 2013 at 10:49 AM, Laszlo Ersek wrote: > On 11/22/13 19:10, Jordan Justen wrote: >> Do we need to print all those fields? >> >> Anyway, maybe a better centralized place for this would be: >> MdeModulePkg/Universal/Acpi/AcpiTableDxe/AcpiTableProtocol.c >> >> Also, I think we try to keep debug messages under 80 characters. > > We seem to think completely differently about debug messages :) Maybe... ? I think you can have too much debug if: * it makes it debug builds excessively slow or large * it makes it challenging to find items in the debug log * it makes the code significantly more difficult to read * it is in a code area that is just not valuable to log > I can cut most of the fields though, and simply keep the signature (and > remove the format string macro too, because for the signature I'd only > use it once). Would that work for you? Sure. >> We do have a local InstallAcpiTable function. (Just remove "AcpiProtocol->") > > Yes, I saw that. But, we don't have a wrapper for the rollback on the > error path (where we uninstall the tables). Since I used AcpiProtocol-> > in the rollback part, I wanted to be consistent here. Good point. -Jordan
Re: [Qemu-devel] [edk2 PATCH 1/1] OvmfPkg/AcpiPlatformDxe: download ACPI tables from QEMU
On 11/22/13 19:10, Jordan Justen wrote:
> On Tue, Nov 12, 2013 at 7:11 AM, Laszlo Ersek wrote:
>> Qemu v1.7.0-rc0 features an ACPI linker/loader interface, available over
>> fw_cfg, written by Michael Tsirkin.
>>
>> Qemu composes all ACPI tables on the host side, according to the target
>> hardware configuration, and makes the tables available to any guest
>> firmware over fw_cfg.
>>
>> The feature moves the burden of keeping ACPI tables up-to-date from boot
>> firmware to qemu (which is the source of hardware configuration anyway).
>>
>> This patch adds client code for this feature.
>>
>> Contributed-under: TianoCore Contribution Agreement 1.0
>> Signed-off-by: Laszlo Ersek
>> ---
>> OvmfPkg/AcpiPlatformDxe/AcpiPlatform.h | 7 +-
>> OvmfPkg/AcpiPlatformDxe/AcpiPlatform.c | 12 +-
>> OvmfPkg/AcpiPlatformDxe/Qemu.c | 204
>> +
>> 3 files changed, 215 insertions(+), 8 deletions(-)
>>
>> diff --git a/OvmfPkg/AcpiPlatformDxe/AcpiPlatform.h
>> b/OvmfPkg/AcpiPlatformDxe/AcpiPlatform.h
>> index 21107cd..c643fa1 100644
>> --- a/OvmfPkg/AcpiPlatformDxe/AcpiPlatform.h
>> +++ b/OvmfPkg/AcpiPlatformDxe/AcpiPlatform.h
>> @@ -10,7 +10,7 @@
>>THE PROGRAM IS DISTRIBUTED UNDER THE BSD LICENSE ON AN "AS IS" BASIS,
>>WITHOUT WARRANTIES OR REPRESENTATIONS OF ANY KIND, EITHER EXPRESS OR
>> IMPLIED.
>>
>> -**/
>> +**/
>>
>> #ifndef _ACPI_PLATFORM_H_INCLUDED_
>> #define _ACPI_PLATFORM_H_INCLUDED_
>> @@ -61,5 +61,10 @@ InstallXenTables (
>>IN EFI_ACPI_TABLE_PROTOCOL *AcpiProtocol
>>);
>>
>> +EFI_STATUS
>> +EFIAPI
>> +InstallQemuLinkedTables (
>> + IN EFI_ACPI_TABLE_PROTOCOL *AcpiProtocol
>> + );
>> #endif
>>
>> diff --git a/OvmfPkg/AcpiPlatformDxe/AcpiPlatform.c
>> b/OvmfPkg/AcpiPlatformDxe/AcpiPlatform.c
>> index 6e0b610..084c393 100644
>> --- a/OvmfPkg/AcpiPlatformDxe/AcpiPlatform.c
>> +++ b/OvmfPkg/AcpiPlatformDxe/AcpiPlatform.c
>> @@ -256,16 +256,14 @@ AcpiPlatformEntryPoint (
>>
>>if (XenDetected ()) {
>> Status = InstallXenTables (AcpiTable);
>> -if (EFI_ERROR (Status)) {
>> - Status = FindAcpiTablesInFv (AcpiTable);
>> -}
>>} else {
>> +Status = InstallQemuLinkedTables (AcpiTable);
>> + }
>> +
>> + if (EFI_ERROR (Status)) {
>> Status = FindAcpiTablesInFv (AcpiTable);
>>}
>> - if (EFI_ERROR (Status)) {
>> -return Status;
>> - }
>>
>> - return EFI_SUCCESS;
>> + return Status;
>> }
>>
>> diff --git a/OvmfPkg/AcpiPlatformDxe/Qemu.c b/OvmfPkg/AcpiPlatformDxe/Qemu.c
>> index 06bd463..c572f8a 100644
>> --- a/OvmfPkg/AcpiPlatformDxe/Qemu.c
>> +++ b/OvmfPkg/AcpiPlatformDxe/Qemu.c
>> @@ -515,3 +515,207 @@ QemuInstallAcpiTable (
>> );
>> }
>>
>> +
>> +/**
>> + Download the ACPI table data file from QEMU and interpret it.
>> +
>> + Starting with v1.7.0-rc0, QEMU provides the following three files for 1.7+
>> + machine types:
>> + - etc/acpi/rsdp
>> + - etc/acpi/tables
>> + - etc/table-loader
>> +
>> + "etc/acpi/rsdp" and "etc/acpi/tables" are similar, they are only kept
>> + separate because they have different allocation requirements in SeaBIOS.
>> +
>> + Both of these fw_cfg files contain preformatted ACPI payload.
>> "etc/acpi/rsdp"
>> + contains only the RSDP table, while "etc/acpi/tables" contains all other
>> + tables, concatenated.
>> +
>> + The tables in these two files have been filled in by qemu, but two kinds
>> of
>> + fields are incomplete in each table: pointers to other tables, and
>> checksums
>> + (which depend on the pointers). The pointers are pre-initialized with
>> + *relative* offsets, but their final values depend on where the actual
>> files
>> + will be placed in memory by the guest. That is, the pointer fields need
>> to be
>> + "relocated" (incremented) by the base addresses of where "/etc/acpi/rsdp"
>> and
>> + "/etc/acpi/tables" will be placed in guest memory.
>> +
>> + This is where the third file, "/etc/table-loader" comes into the picture.
>> It
>> + is a linker/loader script that has several command types:
>> +
>> +One command type instructs the guest to download the other two files.
>> +
>> +Another command type instructs the guest to increment ("absolutize") a
>> +pointer field (having a relative initial value) in some file, with the
>> +dynamic base address of the same (or another) file.
>> +
>> +The third command type instructs the guest to compute checksums over
>> +ranges and to store them.
>> +
>> + In edk2, EFI_ACPI_TABLE_PROTOCOL knows about table relationships -- it
>> + handles linkage automatically when a table is installed. The protocol
>> takes
>> + care of checksumming too. RSDP is installed automatically. Hence we only
>> need
>> + to care about the "etc/acpi/tables" file, determining the boundaries of
>> each
>> + table and installing it.
>
> I'm wondering if some of the information in this comment might have a
> better home in the commit message. Will it help in maintaining the
Re: [Qemu-devel] [edk2 PATCH 1/1] OvmfPkg/AcpiPlatformDxe: download ACPI tables from QEMU
On Tue, Nov 12, 2013 at 7:11 AM, Laszlo Ersek wrote:
> Qemu v1.7.0-rc0 features an ACPI linker/loader interface, available over
> fw_cfg, written by Michael Tsirkin.
>
> Qemu composes all ACPI tables on the host side, according to the target
> hardware configuration, and makes the tables available to any guest
> firmware over fw_cfg.
>
> The feature moves the burden of keeping ACPI tables up-to-date from boot
> firmware to qemu (which is the source of hardware configuration anyway).
>
> This patch adds client code for this feature.
>
> Contributed-under: TianoCore Contribution Agreement 1.0
> Signed-off-by: Laszlo Ersek
> ---
> OvmfPkg/AcpiPlatformDxe/AcpiPlatform.h | 7 +-
> OvmfPkg/AcpiPlatformDxe/AcpiPlatform.c | 12 +-
> OvmfPkg/AcpiPlatformDxe/Qemu.c | 204
> +
> 3 files changed, 215 insertions(+), 8 deletions(-)
>
> diff --git a/OvmfPkg/AcpiPlatformDxe/AcpiPlatform.h
> b/OvmfPkg/AcpiPlatformDxe/AcpiPlatform.h
> index 21107cd..c643fa1 100644
> --- a/OvmfPkg/AcpiPlatformDxe/AcpiPlatform.h
> +++ b/OvmfPkg/AcpiPlatformDxe/AcpiPlatform.h
> @@ -10,7 +10,7 @@
>THE PROGRAM IS DISTRIBUTED UNDER THE BSD LICENSE ON AN "AS IS" BASIS,
>WITHOUT WARRANTIES OR REPRESENTATIONS OF ANY KIND, EITHER EXPRESS OR
> IMPLIED.
>
> -**/
> +**/
>
> #ifndef _ACPI_PLATFORM_H_INCLUDED_
> #define _ACPI_PLATFORM_H_INCLUDED_
> @@ -61,5 +61,10 @@ InstallXenTables (
>IN EFI_ACPI_TABLE_PROTOCOL *AcpiProtocol
>);
>
> +EFI_STATUS
> +EFIAPI
> +InstallQemuLinkedTables (
> + IN EFI_ACPI_TABLE_PROTOCOL *AcpiProtocol
> + );
> #endif
>
> diff --git a/OvmfPkg/AcpiPlatformDxe/AcpiPlatform.c
> b/OvmfPkg/AcpiPlatformDxe/AcpiPlatform.c
> index 6e0b610..084c393 100644
> --- a/OvmfPkg/AcpiPlatformDxe/AcpiPlatform.c
> +++ b/OvmfPkg/AcpiPlatformDxe/AcpiPlatform.c
> @@ -256,16 +256,14 @@ AcpiPlatformEntryPoint (
>
>if (XenDetected ()) {
> Status = InstallXenTables (AcpiTable);
> -if (EFI_ERROR (Status)) {
> - Status = FindAcpiTablesInFv (AcpiTable);
> -}
>} else {
> +Status = InstallQemuLinkedTables (AcpiTable);
> + }
> +
> + if (EFI_ERROR (Status)) {
> Status = FindAcpiTablesInFv (AcpiTable);
>}
> - if (EFI_ERROR (Status)) {
> -return Status;
> - }
>
> - return EFI_SUCCESS;
> + return Status;
> }
>
> diff --git a/OvmfPkg/AcpiPlatformDxe/Qemu.c b/OvmfPkg/AcpiPlatformDxe/Qemu.c
> index 06bd463..c572f8a 100644
> --- a/OvmfPkg/AcpiPlatformDxe/Qemu.c
> +++ b/OvmfPkg/AcpiPlatformDxe/Qemu.c
> @@ -515,3 +515,207 @@ QemuInstallAcpiTable (
> );
> }
>
> +
> +/**
> + Download the ACPI table data file from QEMU and interpret it.
> +
> + Starting with v1.7.0-rc0, QEMU provides the following three files for 1.7+
> + machine types:
> + - etc/acpi/rsdp
> + - etc/acpi/tables
> + - etc/table-loader
> +
> + "etc/acpi/rsdp" and "etc/acpi/tables" are similar, they are only kept
> + separate because they have different allocation requirements in SeaBIOS.
> +
> + Both of these fw_cfg files contain preformatted ACPI payload.
> "etc/acpi/rsdp"
> + contains only the RSDP table, while "etc/acpi/tables" contains all other
> + tables, concatenated.
> +
> + The tables in these two files have been filled in by qemu, but two kinds of
> + fields are incomplete in each table: pointers to other tables, and
> checksums
> + (which depend on the pointers). The pointers are pre-initialized with
> + *relative* offsets, but their final values depend on where the actual files
> + will be placed in memory by the guest. That is, the pointer fields need to
> be
> + "relocated" (incremented) by the base addresses of where "/etc/acpi/rsdp"
> and
> + "/etc/acpi/tables" will be placed in guest memory.
> +
> + This is where the third file, "/etc/table-loader" comes into the picture.
> It
> + is a linker/loader script that has several command types:
> +
> +One command type instructs the guest to download the other two files.
> +
> +Another command type instructs the guest to increment ("absolutize") a
> +pointer field (having a relative initial value) in some file, with the
> +dynamic base address of the same (or another) file.
> +
> +The third command type instructs the guest to compute checksums over
> +ranges and to store them.
> +
> + In edk2, EFI_ACPI_TABLE_PROTOCOL knows about table relationships -- it
> + handles linkage automatically when a table is installed. The protocol takes
> + care of checksumming too. RSDP is installed automatically. Hence we only
> need
> + to care about the "etc/acpi/tables" file, determining the boundaries of
> each
> + table and installing it.
I'm wondering if some of the information in this comment might have a
better home in the commit message. Will it help in maintaining the
code to have it here? Or, maybe a more terse version can live here?
Of course, I rarely comment my code enough, which is much worse. So,
feel free to ignore this feedback. :)
> + @param[in
