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

[Marc-André Lureau]

Hi

On Fri, Jun 29, 2018 at 4:09 PM, Igor Mammedov <address@hidden> wrote:
> On Thu, 28 Jun 2018 17:24:47 +0200
> Marc-André Lureau <address@hidden> wrote:
>
>> Hi
>>
>> On Wed, Jun 27, 2018 at 3:19 PM, Igor Mammedov <address@hidden> 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???
>> >
>> > I'd do it like this:
>> >    tmp = tpm_find()
>> >    if (TPM_IS_TIS(tpm) {
>> >       ...
>> >       if (object_property_get_bool(tpm), "ppi", &error_abort)) {
>> >           build_tpm_ppi(tpm, dev)
>> >       }
>> >       ...
>> >    }
>> >
>> >    if (TPM_IS_CRB(tpm))
>> >    {
>> >       ....
>> >    }
>> >
>> > a bit more verbose but then the reader won't have to jump inside of
>> > build_tpm_ppi() if he/she is not interested in it.
>>
>> ok, I pass the tpm to build_tpm_ppi() for the ppi property check, and
>> avoid extra lookup.
>>
>> >
>> >> +        return;
>> >> +    }
>> >> +    /*
>> >> +     * 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);
>> >> +    }
>> >> +    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);
>> >> +
>> >> +    /*
>> >> +     * 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));
>> >> +    name = aml_name_decl("TPM2", pak);
>> >> +    aml_append(dev, name);
>> >> +
>> >> +    pak = aml_package(3);
>> >> +    aml_append(pak, aml_int(0));
>> >> +    aml_append(pak, aml_int(0));
>> >> +    aml_append(pak, aml_int(0));
>> >> +    name = aml_name_decl("TPM3", pak);
>> > Aml one = aml_int(1)
>> > Aml tpm3 = local(x)
>> > aml_store(pak, tpm3)
>> >
>> > ... and then later ...
>> >
>> >             {
>> >              /* TPM2[1] = PPRQ */
>> >              aml_append(ifctx3, aml_store(aml_name("PPRQ"), 
>> > aml_index(tpm2, one)));
>> >              aml_append(ifctx3, aml_return(tpm2));
>> >
>> > which would make code a bit more readable
>> >
>>
>> But you would have to create aml_int(), aml_local() for every usage. I
>> don't think we gain in readability.
> Aml* variables can be reused as aml_append/aml_foo copies input arguments.
> Take look at build_cpus_aml() how it makes code more readable.
>
>> >> +    aml_append(dev, name);
>> > both packages could be local vars as return is done by value
>> >
>> >> +
>> >> +    method = aml_method("_DSM", 4, AML_SERIALIZED);
>> >> +    {
>> >> +        uint8_t zerobyte[1] = { 0 };
>> >> +        Aml *func_idx, *rev, *op, *op_arg, *op_flags;
>> >> +
>> >> +        ifctx = aml_if(
>> >> +            aml_equal(aml_arg(0),
>> >> +                      
>> >> aml_touuid("3DDDFAA6-361B-4EB4-A424-8D10089D1653")));
>> > aml_equal(uuid, aml_touuid("3DDDFAA6-361B-4EB4-A424-8D10089D1653"))
>>
>> Ok
>>
>> >
>> >> +        {
>> >> +            func_idx = aml_local(0);
>> >> +            aml_append(ifctx,
>> >> +                       aml_store(aml_to_integer(aml_arg(2)), func_idx));
>> > why do you need aml_to_integer() cast here?
>> >
>> > Wouldn't following just work:
>> >
>> >    Aml function = aml_arg(2)
>> >
>> > I'd suggest to use "function" for consistency with nvdimm _DSM
>>
>> ok
>>
>> >
>> > you can use the same naming approach for other arguments i.e.
>> > arguments = aml_arg(3)
>> >
>>
>> ok
>>
>> >> +
>> >> +            /* standard DSM query function */
>> >> +            func_idx = aml_local(0);
>> > I don't think you need to init it twice.
>> >
>>
>> That looks right, and it's the way nvdimm ACPI code is written, but is
>> not following the usual convention to give up the ownership when given
>> as argument to other AML building function.
>
> That's misunderstanding of how it works, there isn't such convention
> input argument is copied into output AML element.
>
>
>> Sad we don't stick to a single usage pattern (giving up ownership).
>> Some day, someone will look at fixing the leaks, and that may make its
>> work easier if the code was consistent...
> Aml* pointers aren't leaked, see aml_alloc() which stores all allocations
> into a list which is freed later by free_aml_allocator() when table
> is build.
>

oh ok, GC magic :)

>
>> But I'll follow your advise for now.
>>
>> >> +            ifctx2 = aml_if(aml_equal(func_idx, aml_int(0)));
>> >> +            {
>> >> +                uint8_t byte_list[2] = { 0xff, 0x01 };
>> >> +                aml_append(ifctx2, aml_return(aml_buffer(2, byte_list)));
>> >> +            }
>> >> +            aml_append(ifctx, ifctx2);
>> >> +
>> >> +            /*
>> >> +             * 8.1.1 Get Physical Presence Interface Version
>> > like Michael noted use version/chapter pair to as reference
>> >
>> > ex: /* ACPI 1.0b: 16.2.5.2 Named Objects Encoding: DefOpRegion */
>>
>> ok
>>
>> >
>> >> +             *
>> >> +             * Arg 2 (Integer): Function Index = 1
>> >> +             * Arg 3 (Package): Arguments = Empty Package
>> >> +             * Returns: Type: String
>> >> +             */
>> >> +            func_idx = aml_local(0);
>> > not needed
>> >
>> >> +            ifctx2 = aml_if(aml_equal(func_idx, aml_int(1)));
>> >> +            {
>> >> +                aml_append(ifctx2, aml_return(aml_string("1.3")));
>> >> +            }
>> >> +            aml_append(ifctx, ifctx2);
>> >> +
>> >> +            /*
>> >> +             * 8.1.2 Submit TPM Operation Request to Pre-OS Environment
>> >> +             *
>> >> +             * Arg 2 (Integer): Function Index = 2
>> >> +             * Arg 3 (Package): Arguments = Package: Type: Integer
>> >> +             *                              Operation Value of the 
>> >> Request
>> >> +             * Returns: Type: Integer
>> >> +             *          0: Success
>> >> +             *          1: Operation Value of the Request Not Supported
>> >> +             *          2: General Failure
>> >> +             */
>> >> +            func_idx = aml_local(0);
>> > ditto and the same in other places
>> >
>> >> +            ifctx2 = aml_if(aml_equal(func_idx, aml_int(2)));
>> >> +            {
>> >> +                /* get opcode */
>> >> +                op = aml_derefof(aml_index(aml_arg(3), aml_int(0)));
>> >> +                aml_append(ifctx2, aml_store(op, aml_local(0)));
>> >                                                     ^^^ what this local(0) 
>> > is? suggest to use named var here (in C sense)
>> >> +
>> >> +                /* get opcode flags */
>> >> +                op = aml_local(0);
>> > use other local to make ASL less confusion (i.e. try not to reuse local 
>> > vars for something else)
>>
>> I must say I get a bit confused by the code style change you request.
>> The original code from Stefan was more straightforward to me. Now it
>> seems C variables adds another layer of complexity. I think it's
>> easier if you come up with a follow-up patch for further cleanup
>> iterations, as I find it hard to match your wanted style.
>
> Whole point of review is to merge code that doesn't need to be
> rewritten later on and to show preferred style for ACPI code
> so I wouldn't be the only one who knows how it is supposed to work.
>
> Now about my original comment.
> What we have in this patch:
>   func_idx = aml_local(0);
>   ...
>   op = aml_local(0);
> so resulting AML decompiled in to ASL would (re)use Local0 with different 
> meanings
> what I've suggested is:
>   Aml *func_idx = aml_local(0);
>   Aml *op = aml_local(1);
>   Aml *foo = aml_local(X);
>
> where ASL would retain LocalX meaning throughout method, making it a bit 
> readable.
>

Ok, hopefully last iteration is closer to that goal.

>
>> >> +                aml_append(ifctx2,
>> >> +                           aml_store(aml_call1("TPFN", op), 
>> >> aml_local(1)));
>> >                                                                ^^^ what 
>> > this local(1) is?
>> >
>> >> +
>> >> +                op_flags = aml_local(1);
>> > I'd init all named vars at the top of function where it's easy to see what 
>> > is what
>> > and use them later. ex: nvdimm_build_common_dsm()
>>
>> But the arguments depend on the function.
> some maybe, but
> function index is common, arguments also, so one could do following:
>
> Aml *func = aml_arg(2);
> Aml *args = aml_arg(3);
>
> without storing them in locals, and then use them within the method as 
> necessary

I did that for aml_arg(0..2). just now reading ACPI DSM spec, I
realize aml_arg(3) is always supposed to be optional arguments
package. So I agree, and my last iteration can be further improved in
that direction.

thanks


-- 
Marc-André Lureau