Re: [PATCH v5 08/20] microvm/acpi: add minimal acpi support

From: Igor Mammedov <imammedo@redhat.com>
To: Gerd Hoffmann <kraxel@redhat.com>
Cc: Laurent Vivier <lvivier@redhat.com>,
	Peter Maydell <peter.maydell@linaro.org>,
	Thomas Huth <thuth@redhat.com>,
	Eduardo Habkost <ehabkost@redhat.com>,
	Sergio Lopez <slp@redhat.com>,
	"Michael S. Tsirkin" <mst@redhat.com>,
	qemu-devel@nongnu.org, Shannon Zhao <shannon.zhaosl@gmail.com>,
	qemu-arm@nongnu.org, Paolo Bonzini <pbonzini@redhat.com>,
	Richard Henderson <rth@twiddle.net>
Subject: Re: [PATCH v5 08/20] microvm/acpi: add minimal acpi support
Date: Fri, 10 Jul 2020 21:30:27 +0200	[thread overview]
Message-ID: <20200710213027.7e467d81@redhat.com> (raw)
In-Reply-To: <20200709123332.dlyciqz3sq6zdgsk@sirius.home.kraxel.org>

On Thu, 9 Jul 2020 14:33:32 +0200
Gerd Hoffmann <kraxel@redhat.com> wrote:

>   Hi,
> 
> > > +    scope = aml_scope("\\");
> > > +    pkg = aml_package(4);
> > > +    aml_append(pkg, aml_int(5)); /* SLEEP_CONTROL_REG.SLP_TYP */  
> > 
> > I'm not sure what does the comment refer to here.  
> 
> It's the register field the value gets written to.
> With full acpi this is PM1a_CNT.SLP_TYP, hw-reduced uses
> SLEEP_CONTROL_REG.SLP_TYP instead.  This is cut&paste from pc/q35
> version + adapted for hw-reduced.
> 
> > Does this 5 match
> > the value IO handler tests against?  
> 
> Yes.  "5" for S5 state (aka poweroff).  Can add a #define.
> 
> > Below is from "7.3.4 System \_Sx states" right?  
> 
> "7.4.2 \_Sx (System States)" here (ACPI 6.3), guess that is the same.
> 
> > > +    AcpiFadtData pmfadt = {
> > > +        /*
> > > +         * minimum version for ACPI_FADT_F_HW_REDUCED_ACPI,
> > > +         * see acpi spec "4.1 Hardware-Reduced ACPI"  
> > 
> > Spec version - I'm guessing ACPI spec 5.0.  
> 
> 6.3
> 
> > And I think here is where you refer to
> > 	Table 5-34 Fixed ACPI Description Table (FADT) Format  
> 
> Table 5-33 FADT Format
> 
> > > +         */
> > > +        .rev = 5,
> > > +        .minor_ver = 1,  
> > 
> > So 5.1 I am guessing just copied from virt/arm?  
> 
> Yes.
> 
> > > +        .flags = ((1 << ACPI_FADT_F_HW_REDUCED_ACPI) |
> > > +                  (1 << ACPI_FADT_F_RESET_REG_SUP)),
> > > +
> > > +        /* Table 5-33 FADT Format -- SLEEP_CONTROL_REG */  
> > 
> > You need to use the earliest spec version that includes
> > a specific feature - and document which one it is.  
> 
> Phew.  Isn't it easier to just use table and field name then, so it is
> easy to find in whatever version of the spec you have at hand?  Also how
> can I figure the earliest spec version easily?
> 
> Sometimes the 6.3 spec documents which table version added specific
> fields, sometimes not ...
> 
> Is the table version synced with the acpi spec version?  Does DSDT v2
> mean the DSDT format was updated for ACPI 2.0 and hasn't changed since?

not necessarily, even if it's pain to check for earliest spec where
feature appeared, it's the best and the least ambiguos comment format to
refference spec, i.e. (do not count on chapter/table numbering/naming being
stable accross versions)

/* Spec name + ver: chapter/table where the feature is described */

something like:

/* ACPI 1.0b: 16.2.5.4 Type 2 Opcodes Encoding: DefObjectType */                 
Aml *aml_object_type(Aml *object)  

> 
> > But the main poit is AcpiFadtData actually has nothing to do with
> > FADT format. It's an abstracted API   
> 
> The FADT is generated from AcpiFadtData.  There is a 1:1 relationship
> between most AcpiFadtData fields and FADT table entries.  This isn't
> what I would call "has nothing to do with" ...
> 
> > > +    xsdt = tables_blob->len;
> > > +    build_xsdt(tables_blob, tables->linker, table_offsets, NULL, NULL);
> > > +
> > > +    /* RSDP is in FSEG memory, so allocate it separately */
> > > +    {
> > > +        AcpiRsdpData rsdp_data = {
> > > +            /* Table 5-27 RSDP Structure */  
> > 
> > RSDP is since ACPI 2.0, table number there is different.  
> 
> References to ACPI 2.0 are almost useless.  ACPI 5.0 is the oldest
> version uefi.org offers for download.
all versions are at (starting from 1.0)
https://uefi.org/acpi/specs

> Guess that underlines the point I made above that referencing specific
> versions of the spec doesn't work very well ...

so far that worked well, effect of requiring spec+version was less
questions during review as reviewer doesn't have to guess where
it's documented.

> 
> take care,
>   Gerd
> 