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

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

On 06/25/2018 05:31 AM, Igor Mammedov wrote:
> On Fri, 22 Jun 2018 10:23:05 -0400
> Stefan Berger <stefanb@linux.vnet.ibm.com> wrote:
>
>> 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.
> my understanding was that values returned in above fields (or some of them)
> are standardized in spec since they are an interface defined by functions
> implemented in _DSM method.
>
> It would be excessive to describe them here, but sending reader to
> a relevant part/term of TPM spec would be helpful.
> (that's what we do with ACPI code, so it would be nice to it in this case
> as well if possible)

Will revise.
>   
>>>   
>>>> +
>>>> +   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
>>>>