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

[Stefan Berger]

On 06/27/2018 09:19 AM, Igor Mammedov wrote:
> On Tue, 26 Jun 2018 14:23:43 +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.
> I've made several suggestions below how to improve aml part of patch a bit,
> will review v6 once it's done
> /hopefully it would be more readable, considering that ASM language is horrible 
> to begin with/
>
> > Signed-off-by: Stefan Berger <address@hidden>
> >
> > ---
> >
> > v6:
> >   - more code documentation (Marc-André)
> >   - use some explicit named variables to ease reading (Marc-André)
> >   - use fixed size fields/memory regions, remove PPI struct (Marc-André)
> >   - only add PPI ACPI methods if PPI is enabled (Marc-André)
> >   - document the qemu/firmware ACPI memory region (Stefan)
> >
> > v5 (Marc-André):
> >   - /struct tpm_ppi/struct TPMPPIData
> >
> > 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 |   8 +
> >   hw/i386/acpi-build.c  | 423 +++++++++++++++++++++++++++++++++++++++++-
> >   docs/specs/tpm.txt    |  79 ++++++++
> >   3 files changed, 509 insertions(+), 1 deletion(-)
> >
> > diff --git a/include/hw/acpi/tpm.h b/include/hw/acpi/tpm.h
> > index f79d68a77a..e0bd07862e 100644
> > --- a/include/hw/acpi/tpm.h
> > +++ b/include/hw/acpi/tpm.h
> > @@ -196,4 +196,12 @@ REG32(CRB_DATA_BUFFER, 0x80)
> >   #define TPM_PPI_VERSION_NONE        0
> >   #define TPM_PPI_VERSION_1_30        1
> >   
> > +/* 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)
> > +
> >   #endif /* HW_ACPI_TPM_H */
> > diff --git a/hw/i386/acpi-build.c b/hw/i386/acpi-build.c
> > index d9320845ed..d815af4eef 100644
> > --- a/hw/i386/acpi-build.c
> > +++ b/hw/i386/acpi-build.c
> > @@ -43,6 +43,7 @@
> >   #include "hw/acpi/memory_hotplug.h"
> >   #include "sysemu/tpm.h"
> >   #include "hw/acpi/tpm.h"
> > +#include "hw/tpm/tpm_ppi.h"
> >   #include "hw/acpi/vmgenid.h"
> >   #include "sysemu/tpm_backend.h"
> >   #include "hw/timer/mc146818rtc_regs.h"
> > @@ -1789,6 +1790,421 @@ static Aml *build_q35_osc_method(void)
> >       return method;
> >   }
> >   
> > +static void
> > +build_tpm_ppi(Aml *dev)
> > +{
> > +    Aml *method, *name, *field, *ifctx, *ifctx2, *ifctx3, *pak;
> > +    int i;
> > +
> > +    if (!object_property_get_bool(OBJECT(tpm_find()), "ppi", &error_abort)) {
> if tpm_find() == NULL -> BAAM???

This function wouldn't be called if there's no TPM.