Re: [Qemu-devel] [PATCH v4 4/5] acpi: build TPM Physical Presence interface

[Igor Mammedov <imammedo@redhat.com>]

On Fri, 22 Jun 2018 09:26:45 -0400
Stefan Berger <stefanb@linux.vnet.ibm.com> wrote:

> On 06/22/2018 05:03 AM, Igor Mammedov wrote:
> > I'd prefer a table format to describe layout, like in
> > docs/specs/acpi_nvdimm.txt or docs/specs/acpi_mem_hotplug.txt  
> 
> diff --git a/docs/specs/tpm.txt b/docs/specs/tpm.txt
> index c230c4c93e..5bf4e892a0 100644
> --- a/docs/specs/tpm.txt
> +++ b/docs/specs/tpm.txt
> @@ -42,6 +42,76 @@ URL:
> 
>   https://trustedcomputinggroup.org/tcg-acpi-specification/
> 
> +== ACPI PPI Interface ==
> +
> +QEMU supports the Physical Presence Interface (PPI) for TPM 1.2 and TPM 
> 2. This
> +interface requires ACPI and firmware support. The specification can be 
> found at
> +the following URL:
> +
> +https://trustedcomputinggroup.org/resource/tcg-physical-presence-interface-specification/
> +
> +PPI enables a system administrator (root) to request a modification to the
> +TPM upon reboot. The PPI specification defines the operation requests 
> and the
> +actions the firmware has to take. The system administrator passes the 
> operation
> +request number to the firmware through an ACPI interface which writes this
> +number to a memory location that the firmware knows. Upon reboot, the 
> firmware
> +finds the number and sends commands to the the TPM. The firmware writes 
> the TPM
> +result code and the operation request number to a memory location that 
> ACPI can
> +read from and pass the result on to the administrator.
> +
> +The PPI specification defines a set of mandatory and optional 
> operations for
> +the firmware to implement. The ACPI interface also allows an 
> administrator to
> +list the supported operations. In QEMU the ACPI code is generated by 
> QEMU, yet
> +the firmware needs to implement support on a per-operations basis, and
> +different firmwares may support a different subset. Therefore, QEMU 
> introduces
> +the virtual memory device for PPI where the firmware can indicate which
> +operations it supports and ACPI can enable the ones that are supported and
> +disable all others. This interface lies in main memory and has the 
> following
what about usecase when it's mapped in isa address space?
  tpm_tis_realizefn() ->
       tpm_ppi_init_io(&s->ppi, isa_address_space(ISA_DEVICE(dev))

> +layout:
> +
> + +----------+--------+--------+-------------------------------------------+
> +   |  Field   | Length | Offset | Description               |
> + +----------+--------+--------+-------------------------------------------+
> +   | func     |  0x100 |  0x000 | Firmware sets values for each 
> supported   |
> +   |          |        |        | operation. See defined values 
> below.      |
> + +----------+--------+--------+-------------------------------------------+
> +   | ppin     |   0x1  |  0x100 | SMI interrupt to use. Set by 
> firmware.    |
> +   |          |        |        | Not 
> supported.                            |
> + +----------+--------+--------+-------------------------------------------+
> +   | ppip     |   0x4  |  0x101 | ACPI function index to pass to SMM 
> code.  |
> +   |          |        |        | Set by ACPI. Not 
> supported.               |
> + +----------+--------+--------+-------------------------------------------+
> +   | pprp     |   0x4  |  0x105 | Result of last executed operation. 
> Set by |
> +   |          |        |        | 
> firmware.                                 |
> + +----------+--------+--------+-------------------------------------------+
> +   | pprq     |   0x4  |  0x109 | Operation request number to execute. 
> Set  |
> +   |          |        |        | by 
> ACPI.                                  |
> + +----------+--------+--------+-------------------------------------------+
> +   | pprm     |   0x4  |  0x10d | Operation request optional parameter. 
> Set |
> +   |          |        |        | by 
> ACPI.                                  |
> + +----------+--------+--------+-------------------------------------------+
> +   | lppr     |   0x4  |  0x111 | Last execute request number. Set 
> by       |
> +   |          |        |        | 
> firmware.                                 |
> + +----------+--------+--------+-------------------------------------------+
> +   | fret     |   0x4  |  0x115 | Result code from SMM 
> function.            |
> +   |          |        |        | Not 
> supported.                            |
> + +----------+--------+--------+-------------------------------------------+
> +   | res1     |  0x40  |  0x119 | Reserved for future 
> use                   |
> + +----------+--------+--------+-------------------------------------------+
> +   | next_step|   0x1  |  0x159 | Operation to execute after reboot 
> by      |
> +   |          |        |        | firmware. Used by 
> firmware.               |
> + +----------+--------+--------+-------------------------------------------+
if there is relevant match/description in PPI spec for above fields
it would be better to put reference to it in description fields and
as field name that could be easily grepped for in PPI spec,
like we we do with ACPI primitives:

  "ACPI 1.0b: 16.2.5.1 Namespace Modifier Objects Encoding: DefScope"

so reader could easily match spec with parts he/she is interested in.

> +
> +   The following values are supported for the 'func' field. They correspond
> +   to the values used by ACPI function index 8.
> +
> +    #define TPM_PPI_FUNC_NOT_IMPLEMENTED     (0 << 0)
> +    #define TPM_PPI_FUNC_BIOS_ONLY           (1 << 0)
> +    #define TPM_PPI_FUNC_BLOCKED             (2 << 0)
> +    #define TPM_PPI_FUNC_ALLOWED_USR_REQ     (3 << 0)
> +    #define TPM_PPI_FUNC_ALLOWED_USR_NOT_REQ (4 << 0)
> +    #define TPM_PPI_FUNC_MASK                (7 << 0)
> +

looks good to me
PS:
maybe drop #define part and make a table from it too so reader
won't have to do shift on his own, like

  0x0  | function is not implemented
  0x1  | ...
  ...


>   QEMU files related to TPM ACPI tables:
>    - hw/i386/acpi-build.c
>