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.a...@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
