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

[Stefan Berger <stefanb@linux.vnet.ibm.com>]

On 06/22/2018 09:56 AM, Igor Mammedov wrote:
> 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))

The use case for PPI is 'administrator wants to reconfigure the TPM on 
next reboot.' I am not sure what else to add?!

>
>> +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 ACPI spec does not go to the level of the implementation let alone 
the necessary fields. The names of the fields stem from edk2.


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