Add LPIT parsing to acpidump

[bwidawsk]

Summary

acpidump(8): Add NFIT to manpage missed in r321298
acpidump(8): Add ACPI LPIT (Low Power Idle Table)

The LPIT is the part of the "standardized" way that one can enumerate
various power state information on Intel platforms.

The documentation for this change can be found here:
http://www.uefi.org/sites/default/files/resources/Intel_ACPI_Low_Power_S0_Idle.pdf

Here is a sample dump from my system:
/*

LPIT: Length=148, Revision=1, Checksum=32,
      OEMID=INTEL, OEM Table ID=SKL, OEM Revision=0x0,
      Creator ID=MSFT, Creator Revision=0x5f

      Type=ACPI_LPIT_TYPE_NATIVE_CSTATE
      Length=56
      UniqueId=0x0000
      Flags=
      EntryTrigger=0x0000000000000060 (?)     Residency=30000
      Latency=3000
      ResidencyCounter=0x0000000000000632 (?) CounterFrequency=TSC

      Type=ACPI_LPIT_TYPE_NATIVE_CSTATE
      Length=56
      UniqueId=0x0001
      Flags=
      EntryTrigger=0x0000000000000060 (?)     Residency=30000
      Latency=3000
      ResidencyCounter=0x0000000000000632 (?) CounterFrequency=TSC

*/

[bwidawsk]

I probably should alphabetize the man page entries...

[bwidawsk]

• acpidump(8): Add NFIT to manpage missed in r321298
• acpidump(8): Add ACPI LPIT (Low Power Idle Table)

1. Updating D15931: acpidump(8): Add NFIT to manpage missed in r321298 #
2. Enter a brief description of the changes included in this update.
3. The first line is used as subject, next lines as comment. #
4. If you intended to create a new revision, use:
5. $ arc diff --create

v2: Alphabetize manpage entries.

[jhb]

Are the question marks after the GAS output an issue with acpi_print_gas() needing to understand newer GAS types?

[jhb]

In general this looks fine. I would probably do the NFIT fixup as a separate commit first. Also, changes to manpages generally require updating Dd to the commit date, though it's ok to leave it out of the review and just change that prior to committing.

[bwidawsk]

In D15931#337133, @jhb wrote:

I am far from an expert on ACPI...

If the LPI residency counter is a Model Specific Register (MSR), it cannot be natively
described in an ACPI Generic Address Structure (GAS). Therefore it must be
described as Fixed Functional Hardware (FFH) with the GAS field requirements:

My understanding is that the FFH addresses have specific parsing based on the hardware and so the existing acpidump code doesn't bother trying to translate it. I looked into fixing that, but I wasn't sure it made sense. I can manually parse this one if you prefer.

[bwidawsk]

In D15931#337134, @jhb wrote:

In general this looks fine. I would probably do the NFIT fixup as a separate commit first. Also, changes to manpages generally require updating Dd to the commit date, though it's ok to leave it out of the review and just change that prior to committing.

I'm still getting the handle of the process here. In my local repo, these are two separate git commits, are you saying the correct way to do this would have been two separate arc diff commands for each of my git commits? Happy to do that.

[emaste]

I took care of the NFIT man page addition in rS335459 - it was my fault :)

[emaste]

I'm still getting the handle of the process here. In my local repo, these are two separate git commits, are you saying the correct way to do this would have been two separate arc diff commands for each of my git commits?

Once you have a commit bit the NFIT fix could just be committed directly to svn. Until then the pedantic approach would be to create two separate reviews, but as the NFIT part is trivial including it here's fine.

[jhb]

In D15931#337135, @ben.widawsky_intel.com wrote:

In D15931#337133, @jhb wrote:

I am far from an expert on ACPI...

If the LPI residency counter is a Model Specific Register (MSR), it cannot be natively
described in an ACPI Generic Address Structure (GAS). Therefore it must be
described as Fixed Functional Hardware (FFH) with the GAS field requirements:

My understanding is that the FFH addresses have specific parsing based on the hardware and so the existing acpidump code doesn't bother trying to translate it. I looked into fixing that, but I wasn't sure it made sense. I can manually parse this one if you prefer.

Hmmm, I was more wondering if acpi_print_gas() simply needed updating in general for newer GAS parsing in newer ACPI revisions. If instead this is a specific hack just for this table then I don't think acpi_print_gas() needs adjusting. It might be nice to recognize the special case and print it as "MSR=" or some such if that isn't a pain to do. However, if that involves a lot of mess and manual GAS parsing code then the current approach is probably fine.

[jhb]

In D15931#337176, @emaste wrote:

I'm still getting the handle of the process here. In my local repo, these are two separate git commits, are you saying the correct way to do this would have been two separate arc diff commands for each of my git commits?

Once you have a commit bit the NFIT fix could just be committed directly to svn. Until then the pedantic approach would be to create two separate reviews, but as the NFIT part is trivial including it here's fine.

Eh, even for reviews I think it is fine to squash related things together so they are reviewed in one batch (I do this sometimes, kib@ does as well). My comment was more trying to give some guidance on the process of landing things in svn directly.

[bwidawsk]

Hmmm, I was more wondering if acpi_print_gas() simply needed updating in general for newer GAS parsing in newer ACPI revisions. If instead this is a specific hack just for this table then I don't think acpi_print_gas() needs adjusting. It might be nice to recognize the special case and print it as "MSR=" or some such if that isn't a pain to do. However, if that involves a lot of mess and manual GAS parsing code then the current approach is probably fine.

The problem is, I don't know. I'd venture to guess that something somewhere uses 0x7f for address space id, and 64 for bit width, but it doesn't mean it's an MSR. If it *does* then that would be a great addition that I'm happy to add.

[jhb]

In D15931#337328, @ben.widawsky_intel.com wrote:

Hmmm, I was more wondering if acpi_print_gas() simply needed updating in general for newer GAS parsing in newer ACPI revisions. If instead this is a specific hack just for this table then I don't think acpi_print_gas() needs adjusting. It might be nice to recognize the special case and print it as "MSR=" or some such if that isn't a pain to do. However, if that involves a lot of mess and manual GAS parsing code then the current approach is probably fine.

The problem is, I don't know. I'd venture to guess that something somewhere uses 0x7f for address space id, and 64 for bit width, but it doesn't mean it's an MSR. If it *does* then that would be a great addition that I'm happy to add.

Looking in the 6.2 spec, 0x7f is "fixed functional hardware" which acpi_print_gas() maps (ACPI_GAS_FIXED) to the default case. I think we can just not worry about acpi_print_gas() for now.

[jhb]

Don't forget to update the date (Dd) in the manpage before committing. I would probably not include the sample output in the commit message but instead send a reply e-mail to the commit mail that includes the sample output. For future reference, jkim@ is probably a good person to add on any ACPI-related reviews.