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

[Liran Alon]

On 12/03/2020 18:27, Igor Mammedov wrote:
> On Wed, 11 Mar 2020 19:08:26 +0200
> Liran Alon <address@hidden> 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