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

[Michael S. Tsirkin]

On Wed, Jun 20, 2018 at 04:35:23PM +0200, Marc-André Lureau wrote:
> Hi
> 
> On Wed, Jun 20, 2018 at 4:08 PM, Michael S. Tsirkin <address@hidden> wrote:
> > On Tue, May 15, 2018 at 02:14:32PM +0200, Marc-André Lureau 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 {
> >
> > The name violate the coding style.
> 
> That's easy to change. Stefan could do it on commit if the rest of the
> patch is unchanged.
> >
> >
> >> +    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 */
> >
> > Are you sure it's right? Below ints will all end up misaligned ...
> 
> Hmm. Sadly, we didn't noticed when doing the edk2 part either. If we
> change it in qemu, we will have to change it in edk2 as well

Might be worth it, it's often very slow to access unaligned fields.

> >> +    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;
> >> +
> >>  #endif /* HW_ACPI_TPM_H */
> >
> > Igor could you pls take a quick look at the rest?
> >
> > --
> > MST
> >
> 
> thanks
> 
> 
> -- 
> Marc-André Lureau