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

[Gerd Hoffmann]

  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?

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

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

take care,
  Gerd