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

["Michael S. Tsirkin" <mst@redhat.com>]

On Thu, Mar 12, 2020 at 07:28:31PM +0200, Liran Alon wrote:
> 
> 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
> 

I'm inclined to agree, but no biggie if Igor disagrees let's go along
with his opinion.

-- 
MST