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

[Igor Mammedov]

On Fri, 22 Jun 2018 10:23:05 -0400
Stefan Berger <address@hidden> wrote:

> On 06/22/2018 09:56 AM, Igor Mammedov wrote:
> > On Fri, 22 Jun 2018 09:26:45 -0400
> > Stefan Berger <address@hidden> 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)
 
> 
> >  
> >> +
> >> +   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
> >>  
>