On Fri, 13 Mar 2020 11:26:45 -0400
"Michael S. Tsirkin" <m...@redhat.com> wrote:
> On Fri, Mar 13, 2020 at 10:36:56AM +0100, Igor Mammedov wrote:
> > On Thu, 12 Mar 2020 13:09:51 -0400
> > "Michael S. Tsirkin" <m...@redhat.com> wrote:
> >
> > > On Thu, Mar 12, 2020 at 05:27:45PM +0100, Igor Mammedov wrote:
> > > > On Wed, 11 Mar 2020 19:08:26 +0200
> > > > Liran Alon <liran.a...@oracle.com> wrote:
> > > >
> > > > > From: Elad Gabay <elad.ga...@oracle.com>
> > > > >
> > > > > Microsoft introduced this ACPI table to avoid Windows guests
> > > > > performing
> > > > > various workarounds for device erratas. As the virtual device emulated
> > > > > by VMM may not have the errata.
> > > > >
> > > > > Currently, WAET allows hypervisor to inform guest about two
> > > > > specific behaviors: One for RTC and the other for ACPI PM Timer.
> > > > >
> > > > > Support for WAET have been introduced since Windows Vista. This ACPI
> > > > > table is also exposed by other hypervisors, such as VMware, by
> > > > > default.
> > > > >
> > > > > This patch adds WAET ACPI Table to QEMU. It also makes sure to
> > > > > introduce
> > > > > the new ACPI table only for new machine-types.
> > > >
> > > > in addition to comments made by Michael ...
> > > >
> > > > >
> > > > > Signed-off-by: Elad Gabay <elad.ga...@oracle.com>
> > > > > Co-developed-by: Liran Alon <liran.a...@oracle.com>
> > > > > Signed-off-by: Liran Alon <liran.a...@oracle.com>
> > > > > ---
> > > > > hw/i386/acpi-build.c | 18 ++++++++++++++++++
> > > > > hw/i386/pc_piix.c | 2 ++
> > > > > hw/i386/pc_q35.c | 2 ++
> > > > > include/hw/acpi/acpi-defs.h | 25 +++++++++++++++++++++++++
> > > > > include/hw/i386/pc.h | 1 +
> > > > > 5 files changed, 48 insertions(+)
> > > > >
> > > > > diff --git a/hw/i386/acpi-build.c b/hw/i386/acpi-build.c
> > > > > index 9c4e46fa7466..29f70741cd96 100644
> > > > > --- a/hw/i386/acpi-build.c
> > > > > +++ b/hw/i386/acpi-build.c
> > > > > @@ -2512,6 +2512,19 @@ build_dmar_q35(GArray *table_data, BIOSLinker
> > > > > *linker)
> > > > > build_header(linker, table_data, (void *)(table_data->data +
> > > > > dmar_start),
> > > > > "DMAR", table_data->len - dmar_start, 1, NULL,
> > > > > NULL);
> > > > > }
> > > > > +
> > > > > +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.
> > > >
> > > > > +{
> > > > > + 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.
> > >
> > >
> > > BTW how about a better name for this function?
> >
> > how about [aml|acpi]_int_raw
> > [...]
>
> I'm not sure how this helps. I think the main problems are
> 1- very long name
> 2- only makes sense if you know that ACPI has a special integer prefix
> 3- easy to confuse which is the value which is the length
> 4- length is in bytes (typical documentation is in bits)
in acpi spec, they use bytes mostly (with occasional bits deviation)
>
> Your suggestion only fixes issue 1.
that's what I don't like the most about current name, it's way too long.
> Having listed it all out, I suggest the following for the purpose of
> building structures:
>
> acpi/aml/build_append_u8
> acpi/aml/build_append_u16
> acpi/aml/build_append_u32
> acpi/aml/build_append_u64
I prefer having length argument, so when I'm reviewing code, I'm basically
comparing it with value in the table.
The same applies to function name, having a bunch of different
names would be distracting, at least where it comes to composing
tables.
So I prefer keeping current list of arguments.
> and maybe
> acpi/aml/build_append_pad( length)
>
> I'm not sure what the best prefix is. I guess we can have them all
> with the slightly different arguments.
