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

[Igor Mammedov]

On Thu, 21 Jun 2018 19:10:38 +0200
Marc-André Lureau <address@hidden> wrote:

> Hi
> 
> On Thu, Jun 21, 2018 at 4:23 PM, Igor Mammedov <address@hidden> wrote:
> > On Thu, 21 Jun 2018 15:21:02 +0200
> > Marc-André Lureau <address@hidden> wrote:
> >  
> >> Hi
> >>
> >> On Thu, Jun 21, 2018 at 2:54 PM, Igor Mammedov <address@hidden> wrote:  
> >> > On Tue, 15 May 2018 14:14:32 +0200
> >> > Marc-André Lureau <address@hidden> wrote:
> >> >  
> >> >> From: Stefan Berger <address@hidden>
> >> >>
> >> >> The TPM Physical Presence interface consists of an ACPI part, a shared
> >> >> memory part, and code in the firmware. Users can send messages to the
> >> >> firmware by writing a code into the shared memory through invoking the
> >> >> ACPI code. When a reboot happens, the firmware looks for the code and
> >> >> acts on it by sending sequences of commands to the TPM.
> >> >>
> >> >> This patch adds the ACPI code. It is similar to the one in EDK2 but 
> >> >> doesn't
> >> >> assume that SMIs are necessary to use. It uses a similar datastructure 
> >> >> for
> >> >> the shared memory as EDK2 does so that EDK2 and SeaBIOS could both make 
> >> >> use
> >> >> of it. I extended the shared memory data structure with an array of 256
> >> >> bytes, one for each code that could be implemented. The array contains
> >> >> flags describing the individual codes. This decouples the ACPI 
> >> >> implementation
> >> >> from the firmware implementation.
> >> >>
> >> >> The underlying TCG specification is accessible from the following page.
> >> >>
> >> >> https://trustedcomputinggroup.org/tcg-physical-presence-interface-specification/
> >> >>
> >> >> This patch implements version 1.30.
> >> >>
> >> >> Signed-off-by: Stefan Berger <address@hidden>
> >> >>
> >> >> ---
> >> >>
> >> >> v4 (Marc-André):
> >> >>  - replace 'DerefOf (FUNC [N])' with a function, to fix Windows ACPI
> >> >>     handling.
> >> >>  - replace 'return Package (..) {} ' with scoped variables, to fix
> >> >>    Windows ACPI handling.
> >> >>
> >> >> v3:
> >> >>  - add support for PPI to CRB
> >> >>  - split up OperationRegion TPPI into two parts, one containing
> >> >>    the registers (TPP1) and the other one the flags (TPP2); switched
> >> >>    the order of the flags versus registers in the code
> >> >>  - adapted ACPI code to small changes to the array of flags where
> >> >>    previous flag 0 was removed and now shifting right wasn't always
> >> >>    necessary anymore
> >> >>
> >> >> v2:
> >> >>  - get rid of FAIL variable; function 5 was using it and always
> >> >>    returns 0; the value is related to the ACPI function call not
> >> >>    a possible failure of the TPM function call.
> >> >>  - extend shared memory data structure with per-opcode entries
> >> >>    holding flags and use those flags to determine what to return
> >> >>    to caller
> >> >>  - implement interface version 1.3
> >> >> ---
> >> >>  include/hw/acpi/tpm.h |  21 +++
> >> >>  hw/i386/acpi-build.c  | 294 +++++++++++++++++++++++++++++++++++++++++-
> >> >>  2 files changed, 314 insertions(+), 1 deletion(-)
> >> >>
> >> >> diff --git a/include/hw/acpi/tpm.h b/include/hw/acpi/tpm.h
> >> >> index f79d68a77a..fc53f08827 100644
> >> >> --- a/include/hw/acpi/tpm.h
> >> >> +++ b/include/hw/acpi/tpm.h
> >> >> @@ -196,4 +196,25 @@ REG32(CRB_DATA_BUFFER, 0x80)
> >> >>  #define TPM_PPI_VERSION_NONE        0
> >> >>  #define TPM_PPI_VERSION_1_30        1
> >> >>
> >> >> +struct tpm_ppi {
> >> >> +    uint8_t  func[256];      /* 0x000: per TPM function implementation 
> >> >> flags;
> >> >> +                                       set by BIOS */
> >> >> +/* whether function is blocked by BIOS settings; bits 0, 1, 2 */
> >> >> +#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)
> >> >> +    uint8_t ppin;            /* 0x100 : set by BIOS */
> >> >> +    uint32_t ppip;           /* 0x101 : set by ACPI; not used */
> >> >> +    uint32_t pprp;           /* 0x105 : response from TPM; set by BIOS 
> >> >> */
> >> >> +    uint32_t pprq;           /* 0x109 : opcode; set by ACPI */
> >> >> +    uint32_t pprm;           /* 0x10d : parameter for opcode; set by 
> >> >> ACPI */
> >> >> +    uint32_t lppr;           /* 0x111 : last opcode; set by BIOS */
> >> >> +    uint32_t fret;           /* 0x115 : set by ACPI; not used */
> >> >> +    uint8_t res1[0x40];      /* 0x119 : reserved for future use */
> >> >> +    uint8_t next_step;       /* 0x159 : next step after reboot; set by 
> >> >> BIOS */
> >> >> +} QEMU_PACKED;  
> >> > is this structure plant to be used for accessing data from within QEMU
> >> > or it is just used for convenience of calculating offsets/sizes for AML?
> >> >
> >> >  
> >>
> >> For convenience (and it can be useful for debugging).  
> > we are gradually getting rid of usage "struct foo" in ACPI code
> > (i.e. when I have time to convert old struct based approach
> > to build_append_int() table based format).
> > and for new code I usually require ACPI parts be struct less
> > (if there is no previous struct baggage to back it up).
> >  
> 
> The tpm_ppi struct is not an ACPI table, it's a fixed memory region /
> layout to exchange between guest/os and firmware.
it's interface between ACPI and firmware and it's used in ACPI
parts of code (mainly as means to document layout). I wouldn't care
much if it were buried inside of TPM code, but it's used mainly as
dummy struct for documenting and by ACPI parts of code for an implicit
sizing math, for that I'd like you to use spec/constants like any
other new ACPI code does.
So lets keep things consistent here at least for ACPI parts that
I maintain.

 
> > Hence my request to drop dummy struct and document layout
> > properly in related spec doc (that's where FW should look for
> > documentation and not into struct definition somewhere in
> > the header). So I'd strongly prefer it be done this way.  
> 
> There is not much more than the field description for me ;). Stefan,
> would you like to develop the structure usage?
You should use table format to document ACPI interface in spec
(ex: docs/specs/acpi_mem_hotplug.txt) at least for consistence sake.
I don't care that firmware side uses C code as documentation as
it's up whatever maintainers prefer over there.
 
[...]