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

[Marc-André Lureau]

Hi

On Wed, Jul 11, 2018 at 6:25 PM, Igor Mammedov <address@hidden> wrote:
> On Wed, 4 Jul 2018 18:00:41 +0200
> Marc-André Lureau <address@hidden> wrote:
>
>> HI
>>
>> On Wed, Jul 4, 2018 at 5:39 PM, Igor Mammedov <address@hidden> wrote:
>> > On Thu, 28 Jun 2018 19:26:57 +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>
>> >> [ Marc-André - ACPI code improvements and windows fixes ]
>> >> Signed-off-by: Marc-André Lureau <address@hidden>
>> >>
>> >> ---
>> >>
>> >> v7: (Marc-André)
>> >>  - use first spec version/section in code comments
>> >>  - use more variables for ASL code building
>> >>  - remove unnecessering ToInteger() calls
>> >>
>> >> 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  | 403 +++++++++++++++++++++++++++++++++++++++++-
>> >>  docs/specs/tpm.txt    |  79 +++++++++
>> >>  3 files changed, 487 insertions(+), 3 deletions(-)
>> >>
>> >> 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 ebd64c4818..263677f3e4 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,396 @@ static Aml *build_q35_osc_method(void)
>> >>      return method;
>> >>  }
>> >>
>> >> +static void
>> >> +build_tpm_ppi(TPMIf *tpm, Aml *dev)
>> >> +{
>> >> +    Aml *method, *field, *ifctx, *ifctx2, *ifctx3;
>> >> +    Aml *pak, *tpm2, *tpm3, *pprm, *pprq;
>> >> +    int i;
>> >> +
>> >> +    if (!object_property_get_bool(OBJECT(tpm), "ppi", &error_abort)) {
>> >> +        return;
>> >> +    }
>> > I'd prefer this being checked by caller, like:
>> >
>> > @@ -2194,6 +2194,7 @@ build_dsdt(GArray *table_data, BIOSLinker *linker,
>> >      int root_bus_limit = 0xFF;
>> >      PCIBus *bus = NULL;
>> >      TPMIf *tpm = tpm_find();
>> > +    bool has_ppi = object_property_get_bool(OBJECT(tpm), "ppi", 
>> > &error_abort);
>>
>> it will need an additional "tpm ? .. : false". I'd rather keep it away
>> from build_dsdt(), but your call.
> ok, keep it as it is now
>
>>
>> >      int i;
>> >
>> >      dsdt = init_aml_allocator();
>> > @@ -2545,8 +2546,9 @@ build_dsdt(GArray *table_data, BIOSLinker *linker,
>> >                   */
>> >                  /* aml_append(crs, aml_irq_no_flags(TPM_TIS_IRQ)); */
>> >                  aml_append(dev, aml_name_decl("_CRS", crs));
>> > -
>> > -                build_tpm_ppi(tpm, dev);
>> > +                if (has_ppi) {
>> > +                    build_tpm_ppi(tpm, dev);
>> > +                }
>> >
>> >                  aml_append(scope, dev);
>> >              }
>> > @@ -2567,7 +2569,9 @@ build_dsdt(GArray *table_data, BIOSLinker *linker,
>> >          aml_append(method, aml_return(aml_int(0x0f)));
>> >          aml_append(dev, method);
>> >
>> > -        build_tpm_ppi(tpm, dev);
>> > +        if (has_ppi) {
>> > +            build_tpm_ppi(tpm, dev);
>> > +        }
>> >
>> >          aml_append(sb_scope, dev);
>> >      }
>> >
>> >> +    /*
>> >> +     * TPP1 is for the flags that indicate which PPI operations
>> >> +     * are supported by the firmware. The firmware is expected to
>> >> +     * write these flags.
>> >> +     */
>> >> +    aml_append(dev,
>> >> +               aml_operation_region("TPP1", AML_SYSTEM_MEMORY,
>> >> +                                    aml_int(TPM_PPI_ADDR_BASE), 0x100));
>> >> +    field = aml_field("TPP1", AML_ANY_ACC, AML_NOLOCK, AML_PRESERVE);
>> >> +    for (i = 0; i < 0x100; i++) {
>> >> +        char *tmp = g_strdup_printf("FN%02X", i);
>> >> +        aml_append(field, aml_named_field(tmp, 8));
>> >> +        g_free(tmp);
>> >> +    }
>> > this and TPFN takes more than 4K.
>> > Have you tried to declare only one filed of size TPP1, which makes
>> > field Buffer type, which could be used with Index and would take only
>> > several bytes to in AML. See ACPI 6.2a 19.6.61.2 Index with Buffers.
>> > It also looks very portable (but we should test with win xp just to be 
>> > safe)
>>
>> You mean like Stefan originally did? Yes, problem described in
>> http://www.osronline.com/showThread.CFM?link=288617.
>
> we have cpu hotplug that uses this approach, but it has IO based OpReg.
>
> Looks like DerefOf in windows is broken wrt SYSTEM_MEMORY originated field 
> buffer
> (debugger shows a correct object returned from Index() but DerefOf reads
> junk, sometimes it returns the last byte of buffer address).
>
> but do not give up yet,
> one can use dynamic operation region inside of method for indexing
> so here goes dirty draft idea:
>     TPFN(op)
>         aml_append(method, aml_operation_region(
>           "TPP1",
>           AML_SYSTEM_MEMORY,
>           aml_add(aml_int(TPM_PPI_ADDR_BASE), op, NULL),
>           0x1));
>         field = aml_field("TPP1", AML_BYTE_ACC, AML_NOLOCK, AML_PRESERVE);
>         aml_append(field, aml_named_field("TPPF", 8));
>         aml_append(method, field);
>         aml_append(method, aml_return(aml_name("TPPF")));
>
> result of addition probably should be stored in sanely named local var,
> but idea stays the same (use address in OpRegion as index) and if 'op'
> is user defined, we probably should have bounds check as well.
>
> I don't like dynamic OpRegions but in general, but it savs us a lot of RAM
> and much faster then walking 'if..else' chain multiple times.

Awesome! that works, thanks for the trick.

>
>
>> >
>> >> +    aml_append(dev, field);
>> >> +
>> >> +    /*
>> >> +     * TPP2 is for the registers that ACPI code used to pass
>> >> +     * the PPI code and parameter (PPRQ, PPRM) to the firmware.
>> >> +     */
>> >> +    aml_append(dev,
>> >> +               aml_operation_region("TPP2", AML_SYSTEM_MEMORY,
>> >> +                                    aml_int(TPM_PPI_ADDR_BASE + 0x100),
>> >> +                                    0x5A));
>> >> +    field = aml_field("TPP2", AML_ANY_ACC, AML_NOLOCK, AML_PRESERVE);
>> >> +    aml_append(field, aml_named_field("PPIN", 8));
>> >> +    aml_append(field, aml_named_field("PPIP", 32));
>> >> +    aml_append(field, aml_named_field("PPRP", 32));
>> >> +    aml_append(field, aml_named_field("PPRQ", 32));
>> >> +    aml_append(field, aml_named_field("PPRM", 32));
>> >> +    aml_append(field, aml_named_field("LPPR", 32));
>> >> +    aml_append(dev, field);
>> >> +    pprq = aml_name("PPRQ");
>> >> +    pprm = aml_name("PPRM");
>> >> +
>> >> +    /*
>> >> +     * A function to return the value of DerefOf (FUNC [N]), by using
>> >> +     * accessing the fields individually instead. This is a workaround
>> >> +     * for what looks like a Windows ACPI bug in all versions so far
>> >> +     * (fwiw, DerefOf (FUNC [N]) works on Linux).
>> >> +     */
>> >> +    method = aml_method("TPFN", 1, AML_SERIALIZED);
>> >> +    {
>> >> +        for (i = 0; i < 0x100; i++) {
>> >> +            ifctx = aml_if(aml_equal(aml_int(i), aml_arg(0)));
>> >> +            {
>> >> +                aml_append(ifctx, aml_return(aml_name("FN%02X", i)));
>> >> +            }
>> >> +            aml_append(method, ifctx);
>> >> +        }
>> >> +        aml_append(method, aml_return(aml_int(0)));
>> >> +    }
>> >> +    aml_append(dev, method);
>> >> +
>> >> +    pak = aml_package(2);
>> >> +    aml_append(pak, aml_int(0));
>> >> +    aml_append(pak, aml_int(0));
>> >> +    aml_append(dev, aml_name_decl("TPM2", pak));
>> >> +    tpm2 = aml_name("TPM2");
>> >> +
>> >> +    pak = aml_package(3);
>> >> +    aml_append(pak, aml_int(0));
>> >> +    aml_append(pak, aml_int(0));
>> >> +    aml_append(pak, aml_int(0));
>> >> +    aml_append(dev, aml_name_decl("TPM3", pak));
>> >> +    tpm3 = aml_name("TPM3");
>> > looks like above 2 packages  are used only by _DSM,
>> > so you don't need to make them global, just use local variables within 
>> > _DSM for it
>> >
>>
>> That would also fail on Windows. Fwiw (this time I am sure) my up to
>> date t460p uses the same global variables trick, presumably to satisfy
>> Windows as well.
> ok, could you add a comment about it here so that we won't optimize it
> out in future.
>
>> btw, not only it took me a while to find the shortcomings with Windows
>> ACPI, but testing any change in the ACPI code takes ~1.5h (with manual
>> intervention ever 10 minutes or so, not a lot of fun)
> don't tell me, I just killed a bunch of time first to setup debug
> environment with broken serial for this series and then to
> find a solution that would work (~15min per ACPI change)
>
>>
>> >> +
>> >> +    method = aml_method("_DSM", 4, AML_SERIALIZED);
>> >> +    {
>> >> +        uint8_t zerobyte[1] = { 0 };
>> >> +        Aml *function, *arguments, *rev, *op, *op_arg, *op_flags, *uuid;
>> >> +
>> >> +        uuid = aml_arg(0);
>> >> +        rev = aml_arg(1);
>> >> +        function = aml_arg(2);
>> >> +        ifctx = aml_if(
>> >> +            aml_equal(uuid,
>> >> +                      
>> >> aml_touuid("3DDDFAA6-361B-4EB4-A424-8D10089D1653")));
>> >> +        {
>> > it would be better to exit immediately if UUID doesn't match (less 
>> > indentation and if contexts)
>> > btw, one should return something if uuid doesn't match method,
>> >
>> > if (LNotEqual(uuid, ToUUID(“893f00a6-660c-494e-bcfd-3043f4fb67c0”))) {
>> >     return(Buffer(){0})
>> > }
>> >
>>
>> Except that the next patch has another uuid to check (and it's not a
>> common ACPI style to return early it seems).
> ok, keep it as is
>
>>
>> >> +            /* standard DSM query function */
>> >> +            ifctx2 = aml_if(aml_equal(function, aml_int(0)));
>> > constants aml_int(0..3) are used multiple times, suggest to declare them
>> > along with rev/function/..., like:
>> >
>> >  Aml *one = aml_int(0);
>> >  ...
>> >
>> > and then use them through code.
>>
>> I don't see that as an improvement tbh, it makes the code
>> inconsistent. So you would like to have zero and one, but not others?
> it will be consistent with cpu/mem hotplug ACPI code and ASL
> since 0 and 1 have their own magic names (Zero, One) in gramar
>

Ok, changed.

thanks



-- 
Marc-André Lureau