Hi
On Wed, Jul 11, 2018 at 6:25 PM, Igor Mammedov <imamm...@redhat.com> wrote:
> On Wed, 4 Jul 2018 18:00:41 +0200
> Marc-André Lureau <marcandre.lur...@gmail.com> wrote:
>
>> HI
>>
>> On Wed, Jul 4, 2018 at 5:39 PM, Igor Mammedov <imamm...@redhat.com> wrote:
>> > On Thu, 28 Jun 2018 19:26:57 +0200
>> > Marc-André Lureau <marcandre.lur...@redhat.com> wrote:
>> >
>> >> From: Stefan Berger <stef...@linux.vnet.ibm.com>
>> >>
>> >> 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 <stef...@linux.vnet.ibm.com>
>> >> [ Marc-André - ACPI code improvements and windows fixes ]
>> >> Signed-off-by: Marc-André Lureau <marcandre.lur...@redhat.com>
>> >>
>> >> ---
>> >>
>> >> 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
