Re: [PATCH] acpi: Add Windows ACPI Emulated Device Table (WAET)

[Liran Alon <liran.alon@oracle.com>]

On 12/03/2020 18:27, Igor Mammedov wrote:
> On Wed, 11 Mar 2020 19:08:26 +0200
> Liran Alon <liran.alon@oracle.com> wrote:
>> +
>> +static void
>> +build_waet(GArray *table_data, BIOSLinker *linker)
> see build_hmat_lb() for example how to doc comment for such function
> should look like. Use earliest spec version where table was introduced.

Note that WAET is a table that is not part of ACPI spec officially.
It's specified on it's own document, there is only a single version, and 
there is only a single table in that document describing that table 
structure.

Therefore, I cannot write a comment such as build_hmat_lb() have:
/*
  * ACPI 6.3: 5.2.27.4 System Locality Latency and Bandwidth Information
  * Structure: Table 5-146
*/

My best attempt to do something similar in v2 is:
/*
  * Windows ACPI Emulated Devices Table
  * (Version 1.0 - April 6, 2009)
  * Spec: 
http://download.microsoft.com/download/7/E/7/7E7662CF-CBEA-470B-A97E-CE7CE0D98DC2/WAET.docx
  *
  * Helpful to speedup Windows guests and ignored by others.
  */

If it's not sufficient. Please suggest alternative phrasing which I 
would use in v2.

>
>> +{
>> +    AcpiTableWaet *waet;
>> +
>> +    waet = acpi_data_push(table_data, sizeof(*waet));
>> +    waet->emulated_device_flags = cpu_to_le32(ACPI_WAET_PM_TIMER_GOOD);
> we don't use packed structures for building ACPI tables anymore (there is
> old code that still does but that's being converted when we touch it)
>
> pls use build_append_int_noprefix() api instead, see build_amd_iommu() as
> an example how to build binary tables using it and how to use comments
> to document fields.
> Basic idea is that api makes function building a table match table's
> description in spec (each call represents a row in spec) and comment
> belonging to a row should contain verbatim field name as used by spec
> so reader could copy/past and grep it easily.
Thanks for pointing this out.
I will make sure to update my code accordingly in v2.
>
>
>
>
>> +
>> +    build_header(linker, table_data,
>> +                 (void *)waet, "WAET", sizeof(*waet), 1, NULL, NULL);
>> +}
>> +
>>   /*
>>    *   IVRS table as specified in AMD IOMMU Specification v2.62, Section 5.2
>>    *   accessible here https://urldefense.com/v3/__http://support.amd.com/TechDocs/48882_IOMMU.pdf__;!!GqivPVa7Brio!On_WsDCS8ysOeUG17h1l3dTpWEm79AHwMHLbbUgsvagBSpgZAk5U1cXddn6ZNOU$
>> @@ -2859,6 +2872,11 @@ void acpi_build(AcpiBuildTables *tables, MachineState *machine)
>>                             machine->nvdimms_state, machine->ram_slots);
>>       }
>>   
>> +    if (!pcmc->do_not_add_waet_acpi) {
>> +        acpi_add_table(table_offsets, tables_blob);
>> +        build_waet(tables_blob, tables->linker);
>> +    }
> we typically do not version ACPI table changes (there might be exceptions
> but it should be a justified one).
> ACPI tables are considered to be a part of firmware (even though they are
> generated by QEMU) so on QEMU upgrade user gets a new firmware along with
> new ACPI tables.

Hmm... I would have expected as a QEMU user that upgrading QEMU may 
update my firmware exposed table (Such as ACPI),
but only if I don't specify I wish to run on a specific machine-type. In 
that case, I would've expect to be exposed with exact same firmware 
information.
I understood that this was one of the main reasons why ACPI/SMBIOS 
generation was moved from SeaBIOS to QEMU.

If you think this isn't the case, I can just remove this flag (Makes 
code simpler). What do you prefer?

Thanks for the review,
-Liran