[Qemu-devel] [PATCH v5 0/4] Add support for TPM Physical Presence interface

[Qemu-devel] [PATCH v5 0/4] Add support for TPM Physical Presence interface

[Marc-André Lureau]

Hi,

The following patches implement the TPM Physical Presence Interface
that allows a user to set a command via ACPI (sysfs entry in Linux)
that, upon the next reboot, the firmware looks for and acts upon by
sending sequences of commands to the TPM.

A dedicated memory region is added to the TPM CRB & TIS devices, at
address/size 0xFED45000/0x400. A new "etc/tpm/config" fw_cfg entry
holds the location for that PPI region and some version details, to
allow for future flexibility.

With the associated edk2/ovmf firmware, the Windows HLK "PPI 1.3" test
now runs successfully.

It is based on previous work from Stefan Berger ("[PATCH v2 0/4]
Implement Physical Presence interface for TPM 1.2 and 2")

The edk2 support is merged upstream.

v5:
 - 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)
 - remove the dummy ACPI memory clear interface patch

v4:
 - add a "ppi" property, default to true, unless machine <= 2.12
 - pass PPI address to tpm_ppi_init_io()
 - renamed tpm_ppi struct name

Marc-André Lureau (1):
  tpm: add a "ppi" boolean property

Stefan Berger (3):
  tpm: implement virtual memory device for TPM PPI
  acpi: add fw_cfg file for TPM and PPI virtual memory device
  acpi: build TPM Physical Presence interface

 hw/tpm/tpm_ppi.h      |  27 +++
 include/hw/acpi/tpm.h |  17 ++
 include/hw/compat.h   |  10 +
 hw/i386/acpi-build.c  | 440 ++++++++++++++++++++++++++++++++++++++++++
 hw/tpm/tpm_crb.c      |  10 +
 hw/tpm/tpm_ppi.c      |  57 ++++++
 hw/tpm/tpm_tis.c      |  10 +
 docs/specs/tpm.txt    |  99 ++++++++++
 hw/tpm/Makefile.objs  |   2 +-
 hw/tpm/trace-events   |   4 +
 10 files changed, 675 insertions(+), 1 deletion(-)
 create mode 100644 hw/tpm/tpm_ppi.h
 create mode 100644 hw/tpm/tpm_ppi.c

-- 
2.18.0.rc1

[Qemu-devel] [PATCH v5 1/4] tpm: add a "ppi" boolean property

[Marc-André Lureau]

The following patches implement the TPM Physical Presence Interface,
make use of a new memory region and a fw_cfg entry. Enable PPI by
default with >2.12 machine type, to avoid migration issues.

Signed-off-by: Marc-André Lureau <marcandre.lureau@redhat.com>
---
 include/hw/compat.h | 10 ++++++++++
 hw/tpm/tpm_crb.c    |  3 +++
 hw/tpm/tpm_tis.c    |  3 +++
 3 files changed, 16 insertions(+)

diff --git a/include/hw/compat.h b/include/hw/compat.h
index 44d5964060..01758991a0 100644
--- a/include/hw/compat.h
+++ b/include/hw/compat.h
@@ -2,6 +2,16 @@
 #define HW_COMPAT_H
 
 #define HW_COMPAT_2_12 \
+    {\
+        .driver   = "tpm-crb",\
+        .property = "ppi",\
+        .value    = "false",\
+    },\
+    {\
+        .driver   = "tpm-tis",\
+        .property = "ppi",\
+        .value    = "false",\
+    },\
     {\
         .driver   = "migration",\
         .property = "decompress-error-check",\
diff --git a/hw/tpm/tpm_crb.c b/hw/tpm/tpm_crb.c
index a92dd50437..d5b0ac5920 100644
--- a/hw/tpm/tpm_crb.c
+++ b/hw/tpm/tpm_crb.c
@@ -41,6 +41,8 @@ typedef struct CRBState {
     MemoryRegion cmdmem;
 
     size_t be_buffer_size;
+
+    bool ppi_enabled;
 } CRBState;
 
 #define CRB(obj) OBJECT_CHECK(CRBState, (obj), TYPE_TPM_CRB)
@@ -221,6 +223,7 @@ static const VMStateDescription vmstate_tpm_crb = {
 
 static Property tpm_crb_properties[] = {
     DEFINE_PROP_TPMBE("tpmdev", CRBState, tpmbe),
+    DEFINE_PROP_BOOL("ppi", CRBState, ppi_enabled, true),
     DEFINE_PROP_END_OF_LIST(),
 };
 
diff --git a/hw/tpm/tpm_tis.c b/hw/tpm/tpm_tis.c
index 12f5c9a759..d9ddf9b723 100644
--- a/hw/tpm/tpm_tis.c
+++ b/hw/tpm/tpm_tis.c
@@ -81,6 +81,8 @@ typedef struct TPMState {
     TPMVersion be_tpm_version;
 
     size_t be_buffer_size;
+
+    bool ppi_enabled;
 } TPMState;
 
 #define TPM(obj) OBJECT_CHECK(TPMState, (obj), TYPE_TPM_TIS)
@@ -950,6 +952,7 @@ static const VMStateDescription vmstate_tpm_tis = {
 static Property tpm_tis_properties[] = {
     DEFINE_PROP_UINT32("irq", TPMState, irq_num, TPM_TIS_IRQ),
     DEFINE_PROP_TPMBE("tpmdev", TPMState, be_driver),
+    DEFINE_PROP_BOOL("ppi", TPMState, ppi_enabled, true),
     DEFINE_PROP_END_OF_LIST(),
 };
 
-- 
2.18.0.rc1

Re: [Qemu-devel] [PATCH v5 1/4] tpm: add a "ppi" boolean property

[Igor Mammedov]

On Tue, 26 Jun 2018 14:23:40 +0200
Marc-André Lureau <marcandre.lureau@redhat.com> wrote:

> The following patches implement the TPM Physical Presence Interface,
> make use of a new memory region and a fw_cfg entry. Enable PPI by
> default with >2.12 machine type, to avoid migration issues.
> 
> Signed-off-by: Marc-André Lureau <marcandre.lureau@redhat.com>
Reviewed-by: Igor Mammedov <imammedo@redhat.com>

> ---
>  include/hw/compat.h | 10 ++++++++++
>  hw/tpm/tpm_crb.c    |  3 +++
>  hw/tpm/tpm_tis.c    |  3 +++
>  3 files changed, 16 insertions(+)
> 
> diff --git a/include/hw/compat.h b/include/hw/compat.h
> index 44d5964060..01758991a0 100644
> --- a/include/hw/compat.h
> +++ b/include/hw/compat.h
> @@ -2,6 +2,16 @@
>  #define HW_COMPAT_H
>  
>  #define HW_COMPAT_2_12 \
> +    {\
> +        .driver   = "tpm-crb",\
> +        .property = "ppi",\
> +        .value    = "false",\
> +    },\
> +    {\
> +        .driver   = "tpm-tis",\
> +        .property = "ppi",\
> +        .value    = "false",\
> +    },\
>      {\
>          .driver   = "migration",\
>          .property = "decompress-error-check",\
> diff --git a/hw/tpm/tpm_crb.c b/hw/tpm/tpm_crb.c
> index a92dd50437..d5b0ac5920 100644
> --- a/hw/tpm/tpm_crb.c
> +++ b/hw/tpm/tpm_crb.c
> @@ -41,6 +41,8 @@ typedef struct CRBState {
>      MemoryRegion cmdmem;
>  
>      size_t be_buffer_size;
> +
> +    bool ppi_enabled;
>  } CRBState;
>  
>  #define CRB(obj) OBJECT_CHECK(CRBState, (obj), TYPE_TPM_CRB)
> @@ -221,6 +223,7 @@ static const VMStateDescription vmstate_tpm_crb = {
>  
>  static Property tpm_crb_properties[] = {
>      DEFINE_PROP_TPMBE("tpmdev", CRBState, tpmbe),
> +    DEFINE_PROP_BOOL("ppi", CRBState, ppi_enabled, true),
>      DEFINE_PROP_END_OF_LIST(),
>  };
>  
> diff --git a/hw/tpm/tpm_tis.c b/hw/tpm/tpm_tis.c
> index 12f5c9a759..d9ddf9b723 100644
> --- a/hw/tpm/tpm_tis.c
> +++ b/hw/tpm/tpm_tis.c
> @@ -81,6 +81,8 @@ typedef struct TPMState {
>      TPMVersion be_tpm_version;
>  
>      size_t be_buffer_size;
> +
> +    bool ppi_enabled;
>  } TPMState;
>  
>  #define TPM(obj) OBJECT_CHECK(TPMState, (obj), TYPE_TPM_TIS)
> @@ -950,6 +952,7 @@ static const VMStateDescription vmstate_tpm_tis = {
>  static Property tpm_tis_properties[] = {
>      DEFINE_PROP_UINT32("irq", TPMState, irq_num, TPM_TIS_IRQ),
>      DEFINE_PROP_TPMBE("tpmdev", TPMState, be_driver),
> +    DEFINE_PROP_BOOL("ppi", TPMState, ppi_enabled, true),
>      DEFINE_PROP_END_OF_LIST(),
>  };
>  

[Qemu-devel] [PATCH v5 2/4] tpm: implement virtual memory device for TPM PPI

[Marc-André Lureau]

From: Stefan Berger <stefanb@linux.vnet.ibm.com>

Implement a virtual memory device for the TPM Physical Presence interface.
The memory is located at 0xFED45000 and used by ACPI to send messages to the
firmware (BIOS) and by the firmware to provide parameters for each one of
the supported codes.

This device should be used by all TPM interfaces on x86 and can be added
by calling tpm_ppi_init_io().

Signed-off-by: Stefan Berger <stefanb@linux.vnet.ibm.com>
Signed-off-by: Marc-André Lureau <marcandre.lureau@redhat.com>

---

v4 (Marc-André):
 - pass TPM_PPI_ADDR_BASE as argument to tpm_ppi_init_io()
 - only enable PPI if property is set

v3 (Marc-André):
 - merge CRB support
 - use trace events instead of DEBUG printf
 - headers inclusion simplification

v2:
 - moved to byte access since an infrequently used device;
   this simplifies code
 - increase size of device to 0x400
 - move device to 0xfffef000 since SeaBIOS has some code at 0xffff0000:
   'On the emulators, the bios at 0xf0000 is also at 0xffff0000'
---
 hw/tpm/tpm_ppi.h      | 27 ++++++++++++++++++++
 include/hw/acpi/tpm.h |  6 +++++
 hw/tpm/tpm_crb.c      |  7 ++++++
 hw/tpm/tpm_ppi.c      | 57 +++++++++++++++++++++++++++++++++++++++++++
 hw/tpm/tpm_tis.c      |  7 ++++++
 hw/tpm/Makefile.objs  |  2 +-
 hw/tpm/trace-events   |  4 +++
 7 files changed, 109 insertions(+), 1 deletion(-)
 create mode 100644 hw/tpm/tpm_ppi.h
 create mode 100644 hw/tpm/tpm_ppi.c

diff --git a/hw/tpm/tpm_ppi.h b/hw/tpm/tpm_ppi.h
new file mode 100644
index 0000000000..ac7ad47238
--- /dev/null
+++ b/hw/tpm/tpm_ppi.h
@@ -0,0 +1,27 @@
+/*
+ * TPM Physical Presence Interface
+ *
+ * Copyright (C) 2018 IBM Corporation
+ *
+ * Authors:
+ *  Stefan Berger    <stefanb@us.ibm.com>
+ *
+ * This work is licensed under the terms of the GNU GPL, version 2 or later.
+ * See the COPYING file in the top-level directory.
+ */
+#ifndef TPM_TPM_PPI_H
+#define TPM_TPM_PPI_H
+
+#include "hw/acpi/tpm.h"
+#include "exec/address-spaces.h"
+
+typedef struct TPMPPI {
+    MemoryRegion mmio;
+
+    uint8_t ram[TPM_PPI_ADDR_SIZE];
+} TPMPPI;
+
+void tpm_ppi_init_io(TPMPPI *tpmppi, struct MemoryRegion *m,
+                     hwaddr addr, Object *obj);
+
+#endif /* TPM_TPM_PPI_H */
diff --git a/include/hw/acpi/tpm.h b/include/hw/acpi/tpm.h
index 46ac4dc581..c082df7d1d 100644
--- a/include/hw/acpi/tpm.h
+++ b/include/hw/acpi/tpm.h
@@ -187,4 +187,10 @@ REG32(CRB_DATA_BUFFER, 0x80)
 #define TPM2_START_METHOD_MMIO      6
 #define TPM2_START_METHOD_CRB       7
 
+/*
+ * Physical Presence Interface
+ */
+#define TPM_PPI_ADDR_SIZE           0x400
+#define TPM_PPI_ADDR_BASE           0xFED45000
+
 #endif /* HW_ACPI_TPM_H */
diff --git a/hw/tpm/tpm_crb.c b/hw/tpm/tpm_crb.c
index d5b0ac5920..37c095f00f 100644
--- a/hw/tpm/tpm_crb.c
+++ b/hw/tpm/tpm_crb.c
@@ -29,6 +29,7 @@
 #include "sysemu/reset.h"
 #include "tpm_int.h"
 #include "tpm_util.h"
+#include "tpm_ppi.h"
 #include "trace.h"
 
 typedef struct CRBState {
@@ -43,6 +44,7 @@ typedef struct CRBState {
     size_t be_buffer_size;
 
     bool ppi_enabled;
+    TPMPPI ppi;
 } CRBState;
 
 #define CRB(obj) OBJECT_CHECK(CRBState, (obj), TYPE_TPM_CRB)
@@ -294,6 +296,11 @@ static void tpm_crb_realize(DeviceState *dev, Error **errp)
     memory_region_add_subregion(get_system_memory(),
         TPM_CRB_ADDR_BASE + sizeof(s->regs), &s->cmdmem);
 
+    if (s->ppi_enabled) {
+        tpm_ppi_init_io(&s->ppi, get_system_memory(),
+                        TPM_PPI_ADDR_BASE, OBJECT(s));
+    }
+
     qemu_register_reset(tpm_crb_reset, dev);
 }
 
diff --git a/hw/tpm/tpm_ppi.c b/hw/tpm/tpm_ppi.c
new file mode 100644
index 0000000000..79bec186c7
--- /dev/null
+++ b/hw/tpm/tpm_ppi.c
@@ -0,0 +1,57 @@
+/*
+ * tpm_ppi.c - TPM Physical Presence Interface
+ *
+ * Copyright (C) 2018 IBM Corporation
+ *
+ * Authors:
+ *  Stefan Berger <stefanb@us.ibm.com>
+ *
+ * This work is licensed under the terms of the GNU GPL, version 2 or later.
+ * See the COPYING file in the top-level directory.
+ *
+ */
+
+#include "qemu/osdep.h"
+
+#include "tpm_ppi.h"
+#include "trace.h"
+
+static uint64_t tpm_ppi_mmio_read(void *opaque, hwaddr addr,
+                                  unsigned size)
+{
+    TPMPPI *s = opaque;
+
+    trace_tpm_ppi_mmio_read(addr, size, s->ram[addr]);
+
+    return s->ram[addr];
+}
+
+static void tpm_ppi_mmio_write(void *opaque, hwaddr addr,
+                               uint64_t val, unsigned size)
+{
+    TPMPPI *s = opaque;
+
+    trace_tpm_ppi_mmio_write(addr, size, val);
+
+    s->ram[addr] = val;
+}
+
+static const MemoryRegionOps tpm_ppi_memory_ops = {
+    .read = tpm_ppi_mmio_read,
+    .write = tpm_ppi_mmio_write,
+    .endianness = DEVICE_NATIVE_ENDIAN,
+    .valid = {
+        .min_access_size = 1,
+        .max_access_size = 1,
+    },
+};
+
+void tpm_ppi_init_io(TPMPPI *tpmppi, struct MemoryRegion *m,
+                     hwaddr addr, Object *obj)
+{
+    memory_region_init_io(&tpmppi->mmio, obj, &tpm_ppi_memory_ops,
+                          tpmppi, "tpm-ppi-mmio",
+                          TPM_PPI_ADDR_SIZE);
+
+    memory_region_add_subregion(m, addr, &tpmppi->mmio);
+}
diff --git a/hw/tpm/tpm_tis.c b/hw/tpm/tpm_tis.c
index d9ddf9b723..e7f45a05b9 100644
--- a/hw/tpm/tpm_tis.c
+++ b/hw/tpm/tpm_tis.c
@@ -31,6 +31,7 @@
 #include "sysemu/tpm_backend.h"
 #include "tpm_int.h"
 #include "tpm_util.h"
+#include "tpm_ppi.h"
 #include "trace.h"
 
 #define TPM_TIS_NUM_LOCALITIES      5     /* per spec */
@@ -83,6 +84,7 @@ typedef struct TPMState {
     size_t be_buffer_size;
 
     bool ppi_enabled;
+    TPMPPI ppi;
 } TPMState;
 
 #define TPM(obj) OBJECT_CHECK(TPMState, (obj), TYPE_TPM_TIS)
@@ -979,6 +981,11 @@ static void tpm_tis_realizefn(DeviceState *dev, Error **errp)
 
     memory_region_add_subregion(isa_address_space(ISA_DEVICE(dev)),
                                 TPM_TIS_ADDR_BASE, &s->mmio);
+
+    if (s->ppi_enabled) {
+        tpm_ppi_init_io(&s->ppi, isa_address_space(ISA_DEVICE(dev)),
+                        TPM_PPI_ADDR_BASE, OBJECT(s));
+    }
 }
 
 static void tpm_tis_initfn(Object *obj)
diff --git a/hw/tpm/Makefile.objs b/hw/tpm/Makefile.objs
index 1dc9f8bf2c..eedd8b6858 100644
--- a/hw/tpm/Makefile.objs
+++ b/hw/tpm/Makefile.objs
@@ -1,4 +1,4 @@
-common-obj-y += tpm_util.o
+common-obj-y += tpm_util.o tpm_ppi.o
 common-obj-$(CONFIG_TPM_TIS) += tpm_tis.o
 common-obj-$(CONFIG_TPM_CRB) += tpm_crb.o
 common-obj-$(CONFIG_TPM_PASSTHROUGH) += tpm_passthrough.o
diff --git a/hw/tpm/trace-events b/hw/tpm/trace-events
index 25bee0cecf..81f9923401 100644
--- a/hw/tpm/trace-events
+++ b/hw/tpm/trace-events
@@ -8,6 +8,10 @@ tpm_crb_mmio_write(uint64_t addr, unsigned size, uint32_t val) "CRB write 0x" TA
 tpm_passthrough_handle_request(void *cmd) "processing command %p"
 tpm_passthrough_reset(void) "reset"
 
+# hw/tpm/tpm_ppi.c
+tpm_ppi_mmio_read(uint64_t addr, unsigned size, uint32_t val) "PPI read 0x" TARGET_FMT_plx " len:%u val: 0x%" PRIx32
+tpm_ppi_mmio_write(uint64_t addr, unsigned size, uint32_t val) "PPI write 0x" TARGET_FMT_plx " len:%u val: 0x%" PRIx32
+
 # hw/tpm/tpm_util.c
 tpm_util_get_buffer_size_hdr_len(uint32_t len, size_t expected) "tpm_resp->hdr.len = %u, expected = %zu"
 tpm_util_get_buffer_size_len(uint32_t len, size_t expected) "tpm_resp->len = %u, expected = %zu"
-- 
2.18.0.rc1

Re: [Qemu-devel] [PATCH v5 2/4] tpm: implement virtual memory device for TPM PPI

[Igor Mammedov]

On Tue, 26 Jun 2018 14:23:41 +0200
Marc-André Lureau <marcandre.lureau@redhat.com> wrote:

> From: Stefan Berger <stefanb@linux.vnet.ibm.com>
> 
> Implement a virtual memory device for the TPM Physical Presence interface.
> The memory is located at 0xFED45000 and used by ACPI to send messages to the
> firmware (BIOS) and by the firmware to provide parameters for each one of
> the supported codes.
> 
> This device should be used by all TPM interfaces on x86 and can be added
> by calling tpm_ppi_init_io().
> 
> Signed-off-by: Stefan Berger <stefanb@linux.vnet.ibm.com>
> Signed-off-by: Marc-André Lureau <marcandre.lureau@redhat.com>
> 
> ---
> 
> v4 (Marc-André):
>  - pass TPM_PPI_ADDR_BASE as argument to tpm_ppi_init_io()
>  - only enable PPI if property is set
> 
> v3 (Marc-André):
>  - merge CRB support
>  - use trace events instead of DEBUG printf
>  - headers inclusion simplification
> 
> v2:
>  - moved to byte access since an infrequently used device;
>    this simplifies code
>  - increase size of device to 0x400
>  - move device to 0xfffef000 since SeaBIOS has some code at 0xffff0000:
>    'On the emulators, the bios at 0xf0000 is also at 0xffff0000'
> ---
>  hw/tpm/tpm_ppi.h      | 27 ++++++++++++++++++++
>  include/hw/acpi/tpm.h |  6 +++++
>  hw/tpm/tpm_crb.c      |  7 ++++++
>  hw/tpm/tpm_ppi.c      | 57 +++++++++++++++++++++++++++++++++++++++++++
>  hw/tpm/tpm_tis.c      |  7 ++++++
>  hw/tpm/Makefile.objs  |  2 +-
>  hw/tpm/trace-events   |  4 +++
>  7 files changed, 109 insertions(+), 1 deletion(-)
>  create mode 100644 hw/tpm/tpm_ppi.h
>  create mode 100644 hw/tpm/tpm_ppi.c
> 
> diff --git a/hw/tpm/tpm_ppi.h b/hw/tpm/tpm_ppi.h
> new file mode 100644
> index 0000000000..ac7ad47238
> --- /dev/null
> +++ b/hw/tpm/tpm_ppi.h
> @@ -0,0 +1,27 @@
> +/*
> + * TPM Physical Presence Interface
> + *
> + * Copyright (C) 2018 IBM Corporation
> + *
> + * Authors:
> + *  Stefan Berger    <stefanb@us.ibm.com>
> + *
> + * This work is licensed under the terms of the GNU GPL, version 2 or later.
> + * See the COPYING file in the top-level directory.
> + */
> +#ifndef TPM_TPM_PPI_H
> +#define TPM_TPM_PPI_H
> +
> +#include "hw/acpi/tpm.h"
> +#include "exec/address-spaces.h"
> +
> +typedef struct TPMPPI {
> +    MemoryRegion mmio;
> +
> +    uint8_t ram[TPM_PPI_ADDR_SIZE];
> +} TPMPPI;
I probably miss something obvious here,
1st:
commit message says that memory reion is supposed to be interface
between FW and OSPM (i.e. totally guest internal thingy).
So question is:
  why do we register memory region at all?
it should be dropped or explained in commit message.

2nd:
If region is really necessary, then where is subsection to migrate
data stored there?


> +
> +void tpm_ppi_init_io(TPMPPI *tpmppi, struct MemoryRegion *m,
> +                     hwaddr addr, Object *obj);
> +
> +#endif /* TPM_TPM_PPI_H */
> diff --git a/include/hw/acpi/tpm.h b/include/hw/acpi/tpm.h
> index 46ac4dc581..c082df7d1d 100644
> --- a/include/hw/acpi/tpm.h
> +++ b/include/hw/acpi/tpm.h
> @@ -187,4 +187,10 @@ REG32(CRB_DATA_BUFFER, 0x80)
>  #define TPM2_START_METHOD_MMIO      6
>  #define TPM2_START_METHOD_CRB       7
>  
> +/*
> + * Physical Presence Interface
> + */
> +#define TPM_PPI_ADDR_SIZE           0x400
> +#define TPM_PPI_ADDR_BASE           0xFED45000
> +
>  #endif /* HW_ACPI_TPM_H */
> diff --git a/hw/tpm/tpm_crb.c b/hw/tpm/tpm_crb.c
> index d5b0ac5920..37c095f00f 100644
> --- a/hw/tpm/tpm_crb.c
> +++ b/hw/tpm/tpm_crb.c
> @@ -29,6 +29,7 @@
>  #include "sysemu/reset.h"
>  #include "tpm_int.h"
>  #include "tpm_util.h"
> +#include "tpm_ppi.h"
>  #include "trace.h"
>  
>  typedef struct CRBState {
> @@ -43,6 +44,7 @@ typedef struct CRBState {
>      size_t be_buffer_size;
>  
>      bool ppi_enabled;
> +    TPMPPI ppi;
>  } CRBState;
>  
>  #define CRB(obj) OBJECT_CHECK(CRBState, (obj), TYPE_TPM_CRB)
> @@ -294,6 +296,11 @@ static void tpm_crb_realize(DeviceState *dev, Error **errp)
>      memory_region_add_subregion(get_system_memory(),
>          TPM_CRB_ADDR_BASE + sizeof(s->regs), &s->cmdmem);
>  
> +    if (s->ppi_enabled) {
> +        tpm_ppi_init_io(&s->ppi, get_system_memory(),
> +                        TPM_PPI_ADDR_BASE, OBJECT(s));
> +    }
> +
>      qemu_register_reset(tpm_crb_reset, dev);
>  }
>  
> diff --git a/hw/tpm/tpm_ppi.c b/hw/tpm/tpm_ppi.c
> new file mode 100644
> index 0000000000..79bec186c7
> --- /dev/null
> +++ b/hw/tpm/tpm_ppi.c
> @@ -0,0 +1,57 @@
> +/*
> + * tpm_ppi.c - TPM Physical Presence Interface
> + *
> + * Copyright (C) 2018 IBM Corporation
> + *
> + * Authors:
> + *  Stefan Berger <stefanb@us.ibm.com>
> + *
> + * This work is licensed under the terms of the GNU GPL, version 2 or later.
> + * See the COPYING file in the top-level directory.
> + *
> + */
> +
> +#include "qemu/osdep.h"
> +
> +#include "tpm_ppi.h"
> +#include "trace.h"
> +
> +static uint64_t tpm_ppi_mmio_read(void *opaque, hwaddr addr,
> +                                  unsigned size)
> +{
> +    TPMPPI *s = opaque;
> +
> +    trace_tpm_ppi_mmio_read(addr, size, s->ram[addr]);
> +
> +    return s->ram[addr];
> +}
> +
> +static void tpm_ppi_mmio_write(void *opaque, hwaddr addr,
> +                               uint64_t val, unsigned size)
> +{
> +    TPMPPI *s = opaque;
> +
> +    trace_tpm_ppi_mmio_write(addr, size, val);
> +
> +    s->ram[addr] = val;
> +}
> +
> +static const MemoryRegionOps tpm_ppi_memory_ops = {
> +    .read = tpm_ppi_mmio_read,
> +    .write = tpm_ppi_mmio_write,
> +    .endianness = DEVICE_NATIVE_ENDIAN,
> +    .valid = {
> +        .min_access_size = 1,
> +        .max_access_size = 1,
> +    },
> +};
> +
> +void tpm_ppi_init_io(TPMPPI *tpmppi, struct MemoryRegion *m,
> +                     hwaddr addr, Object *obj)
> +{
> +    memory_region_init_io(&tpmppi->mmio, obj, &tpm_ppi_memory_ops,
> +                          tpmppi, "tpm-ppi-mmio",
> +                          TPM_PPI_ADDR_SIZE);
> +
> +    memory_region_add_subregion(m, addr, &tpmppi->mmio);
> +}
> diff --git a/hw/tpm/tpm_tis.c b/hw/tpm/tpm_tis.c
> index d9ddf9b723..e7f45a05b9 100644
> --- a/hw/tpm/tpm_tis.c
> +++ b/hw/tpm/tpm_tis.c
> @@ -31,6 +31,7 @@
>  #include "sysemu/tpm_backend.h"
>  #include "tpm_int.h"
>  #include "tpm_util.h"
> +#include "tpm_ppi.h"
>  #include "trace.h"
>  
>  #define TPM_TIS_NUM_LOCALITIES      5     /* per spec */
> @@ -83,6 +84,7 @@ typedef struct TPMState {
>      size_t be_buffer_size;
>  
>      bool ppi_enabled;
> +    TPMPPI ppi;
>  } TPMState;
>  
>  #define TPM(obj) OBJECT_CHECK(TPMState, (obj), TYPE_TPM_TIS)
> @@ -979,6 +981,11 @@ static void tpm_tis_realizefn(DeviceState *dev, Error **errp)
>  
>      memory_region_add_subregion(isa_address_space(ISA_DEVICE(dev)),
>                                  TPM_TIS_ADDR_BASE, &s->mmio);
> +
> +    if (s->ppi_enabled) {
> +        tpm_ppi_init_io(&s->ppi, isa_address_space(ISA_DEVICE(dev)),
> +                        TPM_PPI_ADDR_BASE, OBJECT(s));
> +    }
>  }
>  
>  static void tpm_tis_initfn(Object *obj)
> diff --git a/hw/tpm/Makefile.objs b/hw/tpm/Makefile.objs
> index 1dc9f8bf2c..eedd8b6858 100644
> --- a/hw/tpm/Makefile.objs
> +++ b/hw/tpm/Makefile.objs
> @@ -1,4 +1,4 @@
> -common-obj-y += tpm_util.o
> +common-obj-y += tpm_util.o tpm_ppi.o
>  common-obj-$(CONFIG_TPM_TIS) += tpm_tis.o
>  common-obj-$(CONFIG_TPM_CRB) += tpm_crb.o
>  common-obj-$(CONFIG_TPM_PASSTHROUGH) += tpm_passthrough.o
> diff --git a/hw/tpm/trace-events b/hw/tpm/trace-events
> index 25bee0cecf..81f9923401 100644
> --- a/hw/tpm/trace-events
> +++ b/hw/tpm/trace-events
> @@ -8,6 +8,10 @@ tpm_crb_mmio_write(uint64_t addr, unsigned size, uint32_t val) "CRB write 0x" TA
>  tpm_passthrough_handle_request(void *cmd) "processing command %p"
>  tpm_passthrough_reset(void) "reset"
>  
> +# hw/tpm/tpm_ppi.c
> +tpm_ppi_mmio_read(uint64_t addr, unsigned size, uint32_t val) "PPI read 0x" TARGET_FMT_plx " len:%u val: 0x%" PRIx32
> +tpm_ppi_mmio_write(uint64_t addr, unsigned size, uint32_t val) "PPI write 0x" TARGET_FMT_plx " len:%u val: 0x%" PRIx32
> +
>  # hw/tpm/tpm_util.c
>  tpm_util_get_buffer_size_hdr_len(uint32_t len, size_t expected) "tpm_resp->hdr.len = %u, expected = %zu"
>  tpm_util_get_buffer_size_len(uint32_t len, size_t expected) "tpm_resp->len = %u, expected = %zu"

Re: [Qemu-devel] [PATCH v5 2/4] tpm: implement virtual memory device for TPM PPI

[Stefan Berger]

On 06/27/2018 07:44 AM, Igor Mammedov wrote:
> On Tue, 26 Jun 2018 14:23:41 +0200
> Marc-André Lureau <marcandre.lureau@redhat.com> wrote:
>
>> From: Stefan Berger <stefanb@linux.vnet.ibm.com>
>>
>> Implement a virtual memory device for the TPM Physical Presence interface.
>> The memory is located at 0xFED45000 and used by ACPI to send messages to the
>> firmware (BIOS) and by the firmware to provide parameters for each one of
>> the supported codes.
>>
>> This device should be used by all TPM interfaces on x86 and can be added
>> by calling tpm_ppi_init_io().
>>
>> Signed-off-by: Stefan Berger <stefanb@linux.vnet.ibm.com>
>> Signed-off-by: Marc-André Lureau <marcandre.lureau@redhat.com>
>>
>> ---
>>
>> v4 (Marc-André):
>>   - pass TPM_PPI_ADDR_BASE as argument to tpm_ppi_init_io()
>>   - only enable PPI if property is set
>>
>> v3 (Marc-André):
>>   - merge CRB support
>>   - use trace events instead of DEBUG printf
>>   - headers inclusion simplification
>>
>> v2:
>>   - moved to byte access since an infrequently used device;
>>     this simplifies code
>>   - increase size of device to 0x400
>>   - move device to 0xfffef000 since SeaBIOS has some code at 0xffff0000:
>>     'On the emulators, the bios at 0xf0000 is also at 0xffff0000'
>> ---
>>   hw/tpm/tpm_ppi.h      | 27 ++++++++++++++++++++
>>   include/hw/acpi/tpm.h |  6 +++++
>>   hw/tpm/tpm_crb.c      |  7 ++++++
>>   hw/tpm/tpm_ppi.c      | 57 +++++++++++++++++++++++++++++++++++++++++++
>>   hw/tpm/tpm_tis.c      |  7 ++++++
>>   hw/tpm/Makefile.objs  |  2 +-
>>   hw/tpm/trace-events   |  4 +++
>>   7 files changed, 109 insertions(+), 1 deletion(-)
>>   create mode 100644 hw/tpm/tpm_ppi.h
>>   create mode 100644 hw/tpm/tpm_ppi.c
>>
>> diff --git a/hw/tpm/tpm_ppi.h b/hw/tpm/tpm_ppi.h
>> new file mode 100644
>> index 0000000000..ac7ad47238
>> --- /dev/null
>> +++ b/hw/tpm/tpm_ppi.h
>> @@ -0,0 +1,27 @@
>> +/*
>> + * TPM Physical Presence Interface
>> + *
>> + * Copyright (C) 2018 IBM Corporation
>> + *
>> + * Authors:
>> + *  Stefan Berger    <stefanb@us.ibm.com>
>> + *
>> + * This work is licensed under the terms of the GNU GPL, version 2 or later.
>> + * See the COPYING file in the top-level directory.
>> + */
>> +#ifndef TPM_TPM_PPI_H
>> +#define TPM_TPM_PPI_H
>> +
>> +#include "hw/acpi/tpm.h"
>> +#include "exec/address-spaces.h"
>> +
>> +typedef struct TPMPPI {
>> +    MemoryRegion mmio;
>> +
>> +    uint8_t ram[TPM_PPI_ADDR_SIZE];
>> +} TPMPPI;
> I probably miss something obvious here,
> 1st:
> commit message says that memory reion is supposed to be interface
> between FW and OSPM (i.e. totally guest internal thingy).
> So question is:
>    why do we register memory region at all?

One reason for the device itself was being able to debug the interaction 
of the guest with ACPI though I had additional instrumentation for that 
showing register contents.
We need it to have some memory in the region where we place it. I 
suppose a memory_region_init_ram() would provide migration support 
automatically but cannot be used on memory where we have 
MemoryRegionOps. So we could drop most parts of the device and only run 
memory_region_init_ram() ?


    Stefan

Re: [Qemu-devel] [PATCH v5 2/4] tpm: implement virtual memory device for TPM PPI

[Igor Mammedov]

On Wed, 27 Jun 2018 08:53:28 -0400
Stefan Berger <stefanb@linux.vnet.ibm.com> wrote:

> On 06/27/2018 07:44 AM, Igor Mammedov wrote:
> > On Tue, 26 Jun 2018 14:23:41 +0200
> > Marc-André Lureau <marcandre.lureau@redhat.com> wrote:
> >  
> >> From: Stefan Berger <stefanb@linux.vnet.ibm.com>
> >>
> >> Implement a virtual memory device for the TPM Physical Presence interface.
> >> The memory is located at 0xFED45000 and used by ACPI to send messages to the
> >> firmware (BIOS) and by the firmware to provide parameters for each one of
> >> the supported codes.
> >>
> >> This device should be used by all TPM interfaces on x86 and can be added
> >> by calling tpm_ppi_init_io().
> >>
> >> Signed-off-by: Stefan Berger <stefanb@linux.vnet.ibm.com>
> >> Signed-off-by: Marc-André Lureau <marcandre.lureau@redhat.com>
> >>
> >> ---
> >>
> >> v4 (Marc-André):
> >>   - pass TPM_PPI_ADDR_BASE as argument to tpm_ppi_init_io()
> >>   - only enable PPI if property is set
> >>
> >> v3 (Marc-André):
> >>   - merge CRB support
> >>   - use trace events instead of DEBUG printf
> >>   - headers inclusion simplification
> >>
> >> v2:
> >>   - moved to byte access since an infrequently used device;
> >>     this simplifies code
> >>   - increase size of device to 0x400
> >>   - move device to 0xfffef000 since SeaBIOS has some code at 0xffff0000:
> >>     'On the emulators, the bios at 0xf0000 is also at 0xffff0000'
> >> ---
> >>   hw/tpm/tpm_ppi.h      | 27 ++++++++++++++++++++
> >>   include/hw/acpi/tpm.h |  6 +++++
> >>   hw/tpm/tpm_crb.c      |  7 ++++++
> >>   hw/tpm/tpm_ppi.c      | 57 +++++++++++++++++++++++++++++++++++++++++++
> >>   hw/tpm/tpm_tis.c      |  7 ++++++
> >>   hw/tpm/Makefile.objs  |  2 +-
> >>   hw/tpm/trace-events   |  4 +++
> >>   7 files changed, 109 insertions(+), 1 deletion(-)
> >>   create mode 100644 hw/tpm/tpm_ppi.h
> >>   create mode 100644 hw/tpm/tpm_ppi.c
> >>
> >> diff --git a/hw/tpm/tpm_ppi.h b/hw/tpm/tpm_ppi.h
> >> new file mode 100644
> >> index 0000000000..ac7ad47238
> >> --- /dev/null
> >> +++ b/hw/tpm/tpm_ppi.h
> >> @@ -0,0 +1,27 @@
> >> +/*
> >> + * TPM Physical Presence Interface
> >> + *
> >> + * Copyright (C) 2018 IBM Corporation
> >> + *
> >> + * Authors:
> >> + *  Stefan Berger    <stefanb@us.ibm.com>
> >> + *
> >> + * This work is licensed under the terms of the GNU GPL, version 2 or later.
> >> + * See the COPYING file in the top-level directory.
> >> + */
> >> +#ifndef TPM_TPM_PPI_H
> >> +#define TPM_TPM_PPI_H
> >> +
> >> +#include "hw/acpi/tpm.h"
> >> +#include "exec/address-spaces.h"
> >> +
> >> +typedef struct TPMPPI {
> >> +    MemoryRegion mmio;
> >> +
> >> +    uint8_t ram[TPM_PPI_ADDR_SIZE];
> >> +} TPMPPI;  
> > I probably miss something obvious here,
> > 1st:
> > commit message says that memory reion is supposed to be interface
> > between FW and OSPM (i.e. totally guest internal thingy).
> > So question is:
> >    why do we register memory region at all?  
> 
> One reason for the device itself was being able to debug the interaction 
> of the guest with ACPI though I had additional instrumentation for that 
> showing register contents.
> We need it to have some memory in the region where we place it. I 
> suppose a memory_region_init_ram() would provide migration support 
> automatically but cannot be used on memory where we have 
> MemoryRegionOps. So we could drop most parts of the device and only run 
> memory_region_init_ram() ?
if QEMU doesn't need to touch it ever, you could do even better.
Use bios_linker to make FW allocate a reserved portion if guest RAM and
patch TPM aml code with allocated address. Then you won't have to worry
about migration as reserved area is already migrated as part of RAM.

Huge benefit here is that QEMU doesn't dictate address so no chance
of conflicts and related compat hacks in case we have to move it. 

To do it easily, I'd suggest to extract TPM from DSDT into a dedicated
SSDT table (see vmgenid_build_acpi() as example /you don't need
bios_linker_loader_write_pointer() part, later we can use it if
it would be necessary for QEMU to know address/)
 
>     Stefan
> 

Re: [Qemu-devel] [PATCH v5 2/4] tpm: implement virtual memory device for TPM PPI

[Marc-André Lureau]

On Wed, Jun 27, 2018 at 4:19 PM, Igor Mammedov <imammedo@redhat.com> wrote:
> On Wed, 27 Jun 2018 08:53:28 -0400
> Stefan Berger <stefanb@linux.vnet.ibm.com> wrote:
>
>> On 06/27/2018 07:44 AM, Igor Mammedov wrote:
>> > On Tue, 26 Jun 2018 14:23:41 +0200
>> > Marc-André Lureau <marcandre.lureau@redhat.com> wrote:
>> >
>> >> From: Stefan Berger <stefanb@linux.vnet.ibm.com>
>> >>
>> >> Implement a virtual memory device for the TPM Physical Presence interface.
>> >> The memory is located at 0xFED45000 and used by ACPI to send messages to the
>> >> firmware (BIOS) and by the firmware to provide parameters for each one of
>> >> the supported codes.
>> >>
>> >> This device should be used by all TPM interfaces on x86 and can be added
>> >> by calling tpm_ppi_init_io().
>> >>
>> >> Signed-off-by: Stefan Berger <stefanb@linux.vnet.ibm.com>
>> >> Signed-off-by: Marc-André Lureau <marcandre.lureau@redhat.com>
>> >>
>> >> ---
>> >>
>> >> v4 (Marc-André):
>> >>   - pass TPM_PPI_ADDR_BASE as argument to tpm_ppi_init_io()
>> >>   - only enable PPI if property is set
>> >>
>> >> v3 (Marc-André):
>> >>   - merge CRB support
>> >>   - use trace events instead of DEBUG printf
>> >>   - headers inclusion simplification
>> >>
>> >> v2:
>> >>   - moved to byte access since an infrequently used device;
>> >>     this simplifies code
>> >>   - increase size of device to 0x400
>> >>   - move device to 0xfffef000 since SeaBIOS has some code at 0xffff0000:
>> >>     'On the emulators, the bios at 0xf0000 is also at 0xffff0000'
>> >> ---
>> >>   hw/tpm/tpm_ppi.h      | 27 ++++++++++++++++++++
>> >>   include/hw/acpi/tpm.h |  6 +++++
>> >>   hw/tpm/tpm_crb.c      |  7 ++++++
>> >>   hw/tpm/tpm_ppi.c      | 57 +++++++++++++++++++++++++++++++++++++++++++
>> >>   hw/tpm/tpm_tis.c      |  7 ++++++
>> >>   hw/tpm/Makefile.objs  |  2 +-
>> >>   hw/tpm/trace-events   |  4 +++
>> >>   7 files changed, 109 insertions(+), 1 deletion(-)
>> >>   create mode 100644 hw/tpm/tpm_ppi.h
>> >>   create mode 100644 hw/tpm/tpm_ppi.c
>> >>
>> >> diff --git a/hw/tpm/tpm_ppi.h b/hw/tpm/tpm_ppi.h
>> >> new file mode 100644
>> >> index 0000000000..ac7ad47238
>> >> --- /dev/null
>> >> +++ b/hw/tpm/tpm_ppi.h
>> >> @@ -0,0 +1,27 @@
>> >> +/*
>> >> + * TPM Physical Presence Interface
>> >> + *
>> >> + * Copyright (C) 2018 IBM Corporation
>> >> + *
>> >> + * Authors:
>> >> + *  Stefan Berger    <stefanb@us.ibm.com>
>> >> + *
>> >> + * This work is licensed under the terms of the GNU GPL, version 2 or later.
>> >> + * See the COPYING file in the top-level directory.
>> >> + */
>> >> +#ifndef TPM_TPM_PPI_H
>> >> +#define TPM_TPM_PPI_H
>> >> +
>> >> +#include "hw/acpi/tpm.h"
>> >> +#include "exec/address-spaces.h"
>> >> +
>> >> +typedef struct TPMPPI {
>> >> +    MemoryRegion mmio;
>> >> +
>> >> +    uint8_t ram[TPM_PPI_ADDR_SIZE];
>> >> +} TPMPPI;
>> > I probably miss something obvious here,
>> > 1st:
>> > commit message says that memory reion is supposed to be interface
>> > between FW and OSPM (i.e. totally guest internal thingy).
>> > So question is:
>> >    why do we register memory region at all?
>>
>> One reason for the device itself was being able to debug the interaction
>> of the guest with ACPI though I had additional instrumentation for that
>> showing register contents.
>> We need it to have some memory in the region where we place it. I
>> suppose a memory_region_init_ram() would provide migration support
>> automatically but cannot be used on memory where we have
>> MemoryRegionOps. So we could drop most parts of the device and only run
>> memory_region_init_ram() ?
> if QEMU doesn't need to touch it ever, you could do even better.
> Use bios_linker to make FW allocate a reserved portion if guest RAM and
> patch TPM aml code with allocated address. Then you won't have to worry
> about migration as reserved area is already migrated as part of RAM.

The location needs to be fixed across reboots (or content moved
somehow..) And timing-wise, I am not sure the ACPI tables are
installed and processed by firmware before the TPM PPI operation must
be handled.

>
> Huge benefit here is that QEMU doesn't dictate address so no chance
> of conflicts and related compat hacks in case we have to move it.
>
> To do it easily, I'd suggest to extract TPM from DSDT into a dedicated
> SSDT table (see vmgenid_build_acpi() as example /you don't need
> bios_linker_loader_write_pointer() part, later we can use it if
> it would be necessary for QEMU to know address/)
>
>>     Stefan
>>
>
>



-- 
Marc-André Lureau

Re: [Qemu-devel] [PATCH v5 2/4] tpm: implement virtual memory device for TPM PPI

[Igor Mammedov]

On Wed, 27 Jun 2018 16:29:51 +0200
Marc-André Lureau <marcandre.lureau@gmail.com> wrote:

> On Wed, Jun 27, 2018 at 4:19 PM, Igor Mammedov <imammedo@redhat.com> wrote:
> > On Wed, 27 Jun 2018 08:53:28 -0400
> > Stefan Berger <stefanb@linux.vnet.ibm.com> wrote:
> >  
> >> On 06/27/2018 07:44 AM, Igor Mammedov wrote:  
> >> > On Tue, 26 Jun 2018 14:23:41 +0200
> >> > Marc-André Lureau <marcandre.lureau@redhat.com> wrote:
> >> >  
> >> >> From: Stefan Berger <stefanb@linux.vnet.ibm.com>
> >> >>
> >> >> Implement a virtual memory device for the TPM Physical Presence interface.
> >> >> The memory is located at 0xFED45000 and used by ACPI to send messages to the
> >> >> firmware (BIOS) and by the firmware to provide parameters for each one of
> >> >> the supported codes.
> >> >>
> >> >> This device should be used by all TPM interfaces on x86 and can be added
> >> >> by calling tpm_ppi_init_io().
> >> >>
> >> >> Signed-off-by: Stefan Berger <stefanb@linux.vnet.ibm.com>
> >> >> Signed-off-by: Marc-André Lureau <marcandre.lureau@redhat.com>
> >> >>
> >> >> ---
> >> >>
> >> >> v4 (Marc-André):
> >> >>   - pass TPM_PPI_ADDR_BASE as argument to tpm_ppi_init_io()
> >> >>   - only enable PPI if property is set
> >> >>
> >> >> v3 (Marc-André):
> >> >>   - merge CRB support
> >> >>   - use trace events instead of DEBUG printf
> >> >>   - headers inclusion simplification
> >> >>
> >> >> v2:
> >> >>   - moved to byte access since an infrequently used device;
> >> >>     this simplifies code
> >> >>   - increase size of device to 0x400
> >> >>   - move device to 0xfffef000 since SeaBIOS has some code at 0xffff0000:
> >> >>     'On the emulators, the bios at 0xf0000 is also at 0xffff0000'
> >> >> ---
> >> >>   hw/tpm/tpm_ppi.h      | 27 ++++++++++++++++++++
> >> >>   include/hw/acpi/tpm.h |  6 +++++
> >> >>   hw/tpm/tpm_crb.c      |  7 ++++++
> >> >>   hw/tpm/tpm_ppi.c      | 57 +++++++++++++++++++++++++++++++++++++++++++
> >> >>   hw/tpm/tpm_tis.c      |  7 ++++++
> >> >>   hw/tpm/Makefile.objs  |  2 +-
> >> >>   hw/tpm/trace-events   |  4 +++
> >> >>   7 files changed, 109 insertions(+), 1 deletion(-)
> >> >>   create mode 100644 hw/tpm/tpm_ppi.h
> >> >>   create mode 100644 hw/tpm/tpm_ppi.c
> >> >>
> >> >> diff --git a/hw/tpm/tpm_ppi.h b/hw/tpm/tpm_ppi.h
> >> >> new file mode 100644
> >> >> index 0000000000..ac7ad47238
> >> >> --- /dev/null
> >> >> +++ b/hw/tpm/tpm_ppi.h
> >> >> @@ -0,0 +1,27 @@
> >> >> +/*
> >> >> + * TPM Physical Presence Interface
> >> >> + *
> >> >> + * Copyright (C) 2018 IBM Corporation
> >> >> + *
> >> >> + * Authors:
> >> >> + *  Stefan Berger    <stefanb@us.ibm.com>
> >> >> + *
> >> >> + * This work is licensed under the terms of the GNU GPL, version 2 or later.
> >> >> + * See the COPYING file in the top-level directory.
> >> >> + */
> >> >> +#ifndef TPM_TPM_PPI_H
> >> >> +#define TPM_TPM_PPI_H
> >> >> +
> >> >> +#include "hw/acpi/tpm.h"
> >> >> +#include "exec/address-spaces.h"
> >> >> +
> >> >> +typedef struct TPMPPI {
> >> >> +    MemoryRegion mmio;
> >> >> +
> >> >> +    uint8_t ram[TPM_PPI_ADDR_SIZE];
> >> >> +} TPMPPI;  
> >> > I probably miss something obvious here,
> >> > 1st:
> >> > commit message says that memory reion is supposed to be interface
> >> > between FW and OSPM (i.e. totally guest internal thingy).
> >> > So question is:
> >> >    why do we register memory region at all?  
> >>
> >> One reason for the device itself was being able to debug the interaction
> >> of the guest with ACPI though I had additional instrumentation for that
> >> showing register contents.
> >> We need it to have some memory in the region where we place it. I
> >> suppose a memory_region_init_ram() would provide migration support
> >> automatically but cannot be used on memory where we have
> >> MemoryRegionOps. So we could drop most parts of the device and only run
> >> memory_region_init_ram() ?  
> > if QEMU doesn't need to touch it ever, you could do even better.
> > Use bios_linker to make FW allocate a reserved portion if guest RAM and
> > patch TPM aml code with allocated address. Then you won't have to worry
> > about migration as reserved area is already migrated as part of RAM.  
> 
> The location needs to be fixed across reboots (or content moved
> somehow..) And timing-wise, I am not sure the ACPI tables are
> installed and processed by firmware before the TPM PPI operation must
> be handled.
true,
maybe add a note/reason to commit message why bios_linker couldn't be used,
to prevent the same question being raised again or wasting cycles on
'optimizing' code later with bios_linker when nobody recalls why it can't be used.

> 
> >
> > Huge benefit here is that QEMU doesn't dictate address so no chance
> > of conflicts and related compat hacks in case we have to move it.
> >
> > To do it easily, I'd suggest to extract TPM from DSDT into a dedicated
> > SSDT table (see vmgenid_build_acpi() as example /you don't need
> > bios_linker_loader_write_pointer() part, later we can use it if
> > it would be necessary for QEMU to know address/)
> >  
> >>     Stefan
> >>  
> >
> >  
> 
> 
> 

Re: [Qemu-devel] [PATCH v5 2/4] tpm: implement virtual memory device for TPM PPI

[Stefan Berger]

On 06/27/2018 10:19 AM, Igor Mammedov wrote:
> On Wed, 27 Jun 2018 08:53:28 -0400
> Stefan Berger <stefanb@linux.vnet.ibm.com> wrote:
>
>> On 06/27/2018 07:44 AM, Igor Mammedov wrote:
>>> On Tue, 26 Jun 2018 14:23:41 +0200
>>> Marc-André Lureau <marcandre.lureau@redhat.com> wrote:
>>>   
>>>> From: Stefan Berger <stefanb@linux.vnet.ibm.com>
>>>>
>>>> Implement a virtual memory device for the TPM Physical Presence interface.
>>>> The memory is located at 0xFED45000 and used by ACPI to send messages to the
>>>> firmware (BIOS) and by the firmware to provide parameters for each one of
>>>> the supported codes.
>>>>
>>>> This device should be used by all TPM interfaces on x86 and can be added
>>>> by calling tpm_ppi_init_io().
>>>>
>>>> Signed-off-by: Stefan Berger <stefanb@linux.vnet.ibm.com>
>>>> Signed-off-by: Marc-André Lureau <marcandre.lureau@redhat.com>
>>>>
>>>> ---
>>>>
>>>> v4 (Marc-André):
>>>>    - pass TPM_PPI_ADDR_BASE as argument to tpm_ppi_init_io()
>>>>    - only enable PPI if property is set
>>>>
>>>> v3 (Marc-André):
>>>>    - merge CRB support
>>>>    - use trace events instead of DEBUG printf
>>>>    - headers inclusion simplification
>>>>
>>>> v2:
>>>>    - moved to byte access since an infrequently used device;
>>>>      this simplifies code
>>>>    - increase size of device to 0x400
>>>>    - move device to 0xfffef000 since SeaBIOS has some code at 0xffff0000:
>>>>      'On the emulators, the bios at 0xf0000 is also at 0xffff0000'
>>>> ---
>>>>    hw/tpm/tpm_ppi.h      | 27 ++++++++++++++++++++
>>>>    include/hw/acpi/tpm.h |  6 +++++
>>>>    hw/tpm/tpm_crb.c      |  7 ++++++
>>>>    hw/tpm/tpm_ppi.c      | 57 +++++++++++++++++++++++++++++++++++++++++++
>>>>    hw/tpm/tpm_tis.c      |  7 ++++++
>>>>    hw/tpm/Makefile.objs  |  2 +-
>>>>    hw/tpm/trace-events   |  4 +++
>>>>    7 files changed, 109 insertions(+), 1 deletion(-)
>>>>    create mode 100644 hw/tpm/tpm_ppi.h
>>>>    create mode 100644 hw/tpm/tpm_ppi.c
>>>>
>>>> diff --git a/hw/tpm/tpm_ppi.h b/hw/tpm/tpm_ppi.h
>>>> new file mode 100644
>>>> index 0000000000..ac7ad47238
>>>> --- /dev/null
>>>> +++ b/hw/tpm/tpm_ppi.h
>>>> @@ -0,0 +1,27 @@
>>>> +/*
>>>> + * TPM Physical Presence Interface
>>>> + *
>>>> + * Copyright (C) 2018 IBM Corporation
>>>> + *
>>>> + * Authors:
>>>> + *  Stefan Berger    <stefanb@us.ibm.com>
>>>> + *
>>>> + * This work is licensed under the terms of the GNU GPL, version 2 or later.
>>>> + * See the COPYING file in the top-level directory.
>>>> + */
>>>> +#ifndef TPM_TPM_PPI_H
>>>> +#define TPM_TPM_PPI_H
>>>> +
>>>> +#include "hw/acpi/tpm.h"
>>>> +#include "exec/address-spaces.h"
>>>> +
>>>> +typedef struct TPMPPI {
>>>> +    MemoryRegion mmio;
>>>> +
>>>> +    uint8_t ram[TPM_PPI_ADDR_SIZE];
>>>> +} TPMPPI;
>>> I probably miss something obvious here,
>>> 1st:
>>> commit message says that memory reion is supposed to be interface
>>> between FW and OSPM (i.e. totally guest internal thingy).
>>> So question is:
>>>     why do we register memory region at all?
>> One reason for the device itself was being able to debug the interaction
>> of the guest with ACPI though I had additional instrumentation for that
>> showing register contents.
>> We need it to have some memory in the region where we place it. I
>> suppose a memory_region_init_ram() would provide migration support
>> automatically but cannot be used on memory where we have
>> MemoryRegionOps. So we could drop most parts of the device and only run
>> memory_region_init_ram() ?
> if QEMU doesn't need to touch it ever, you could do even better.

QEMU does indirectly touch it in 4/4 where we define the 
OperationRegion()s and need to know where they are located in memory. We 
could read the base address that is now TPM_PPI_ADDR_BASE from a hard 
coded memory location and pass it into OperationRegion(), but I doubt we 
would want that.

+    aml_append(dev,
+               aml_operation_region("TPP1", AML_SYSTEM_MEMORY,
+                                    aml_int(TPM_PPI_ADDR_BASE), 0x100));


+    aml_append(dev,
+               aml_operation_region("TPP2", AML_SYSTEM_MEMORY,
+                                    aml_int(TPM_PPI_ADDR_BASE + 0x100),
+                                    0x5A));



> Use bios_linker to make FW allocate a reserved portion if guest RAM and
> patch TPM aml code with allocated address. Then you won't have to worry
> about migration as reserved area is already migrated as part of RAM.
>
> Huge benefit here is that QEMU doesn't dictate address so no chance
> of conflicts and related compat hacks in case we have to move it.
> To do it easily, I'd suggest to extract TPM from DSDT into a dedicated
> SSDT table (see vmgenid_build_acpi() as example /you don't need
> bios_linker_loader_write_pointer() part, later we can use it if
> it would be necessary for QEMU to know address/)
>   
>>      Stefan
>>

Re: [Qemu-devel] [PATCH v5 2/4] tpm: implement virtual memory device for TPM PPI

[Igor Mammedov]

On Wed, 27 Jun 2018 10:36:52 -0400
Stefan Berger <stefanb@linux.vnet.ibm.com> wrote:

> On 06/27/2018 10:19 AM, Igor Mammedov wrote:
> > On Wed, 27 Jun 2018 08:53:28 -0400
> > Stefan Berger <stefanb@linux.vnet.ibm.com> wrote:
> >  
> >> On 06/27/2018 07:44 AM, Igor Mammedov wrote:  
> >>> On Tue, 26 Jun 2018 14:23:41 +0200
> >>> Marc-André Lureau <marcandre.lureau@redhat.com> wrote:
> >>>     
> >>>> From: Stefan Berger <stefanb@linux.vnet.ibm.com>
> >>>>
> >>>> Implement a virtual memory device for the TPM Physical Presence interface.
> >>>> The memory is located at 0xFED45000 and used by ACPI to send messages to the
> >>>> firmware (BIOS) and by the firmware to provide parameters for each one of
> >>>> the supported codes.
> >>>>
> >>>> This device should be used by all TPM interfaces on x86 and can be added
> >>>> by calling tpm_ppi_init_io().
> >>>>
> >>>> Signed-off-by: Stefan Berger <stefanb@linux.vnet.ibm.com>
> >>>> Signed-off-by: Marc-André Lureau <marcandre.lureau@redhat.com>
> >>>>
> >>>> ---
> >>>>
> >>>> v4 (Marc-André):
> >>>>    - pass TPM_PPI_ADDR_BASE as argument to tpm_ppi_init_io()
> >>>>    - only enable PPI if property is set
> >>>>
> >>>> v3 (Marc-André):
> >>>>    - merge CRB support
> >>>>    - use trace events instead of DEBUG printf
> >>>>    - headers inclusion simplification
> >>>>
> >>>> v2:
> >>>>    - moved to byte access since an infrequently used device;
> >>>>      this simplifies code
> >>>>    - increase size of device to 0x400
> >>>>    - move device to 0xfffef000 since SeaBIOS has some code at 0xffff0000:
> >>>>      'On the emulators, the bios at 0xf0000 is also at 0xffff0000'
> >>>> ---
> >>>>    hw/tpm/tpm_ppi.h      | 27 ++++++++++++++++++++
> >>>>    include/hw/acpi/tpm.h |  6 +++++
> >>>>    hw/tpm/tpm_crb.c      |  7 ++++++
> >>>>    hw/tpm/tpm_ppi.c      | 57 +++++++++++++++++++++++++++++++++++++++++++
> >>>>    hw/tpm/tpm_tis.c      |  7 ++++++
> >>>>    hw/tpm/Makefile.objs  |  2 +-
> >>>>    hw/tpm/trace-events   |  4 +++
> >>>>    7 files changed, 109 insertions(+), 1 deletion(-)
> >>>>    create mode 100644 hw/tpm/tpm_ppi.h
> >>>>    create mode 100644 hw/tpm/tpm_ppi.c
> >>>>
> >>>> diff --git a/hw/tpm/tpm_ppi.h b/hw/tpm/tpm_ppi.h
> >>>> new file mode 100644
> >>>> index 0000000000..ac7ad47238
> >>>> --- /dev/null
> >>>> +++ b/hw/tpm/tpm_ppi.h
> >>>> @@ -0,0 +1,27 @@
> >>>> +/*
> >>>> + * TPM Physical Presence Interface
> >>>> + *
> >>>> + * Copyright (C) 2018 IBM Corporation
> >>>> + *
> >>>> + * Authors:
> >>>> + *  Stefan Berger    <stefanb@us.ibm.com>
> >>>> + *
> >>>> + * This work is licensed under the terms of the GNU GPL, version 2 or later.
> >>>> + * See the COPYING file in the top-level directory.
> >>>> + */
> >>>> +#ifndef TPM_TPM_PPI_H
> >>>> +#define TPM_TPM_PPI_H
> >>>> +
> >>>> +#include "hw/acpi/tpm.h"
> >>>> +#include "exec/address-spaces.h"
> >>>> +
> >>>> +typedef struct TPMPPI {
> >>>> +    MemoryRegion mmio;
> >>>> +
> >>>> +    uint8_t ram[TPM_PPI_ADDR_SIZE];
> >>>> +} TPMPPI;  
> >>> I probably miss something obvious here,
> >>> 1st:
> >>> commit message says that memory reion is supposed to be interface
> >>> between FW and OSPM (i.e. totally guest internal thingy).
> >>> So question is:
> >>>     why do we register memory region at all?  
> >> One reason for the device itself was being able to debug the interaction
> >> of the guest with ACPI though I had additional instrumentation for that
> >> showing register contents.
> >> We need it to have some memory in the region where we place it. I
> >> suppose a memory_region_init_ram() would provide migration support
> >> automatically but cannot be used on memory where we have
> >> MemoryRegionOps. So we could drop most parts of the device and only run
> >> memory_region_init_ram() ?  
> > if QEMU doesn't need to touch it ever, you could do even better.  
> 
> QEMU does indirectly touch it in 4/4 where we define the 
> OperationRegion()s and need to know where they are located in memory. We 
> could read the base address that is now TPM_PPI_ADDR_BASE from a hard 
> coded memory location
that's done for you by bios_linker_loader_add_pointer() when
ACPI tables are installed by FW.

> and pass it into OperationRegion(), but I doubt we 
> would want that.
> 
> +    aml_append(dev,
> +               aml_operation_region("TPP1", AML_SYSTEM_MEMORY,
> +                                    aml_int(TPM_PPI_ADDR_BASE), 0x100));
> 
> 
> +    aml_append(dev,
> +               aml_operation_region("TPP2", AML_SYSTEM_MEMORY,
> +                                    aml_int(TPM_PPI_ADDR_BASE + 0x100),
> +                                    0x5A));
that's possible, usually it works as dynamic memory region where region
lives within scope of a method.

but scratch it.
As Andre pointed out reserved memory should stay at the same place
across reboots and might be needed before ACPI tables are installed,
which probably is impossible.

CCing Laszlo just in case if I'm wrong.


> > Use bios_linker to make FW allocate a reserved portion if guest RAM and
> > patch TPM aml code with allocated address. Then you won't have to worry
> > about migration as reserved area is already migrated as part of RAM.
> >
> > Huge benefit here is that QEMU doesn't dictate address so no chance
> > of conflicts and related compat hacks in case we have to move it.
> > To do it easily, I'd suggest to extract TPM from DSDT into a dedicated
> > SSDT table (see vmgenid_build_acpi() as example /you don't need
> > bios_linker_loader_write_pointer() part, later we can use it if
> > it would be necessary for QEMU to know address/)
> >     
> >>      Stefan
> >>  
> 

Re: [Qemu-devel] [PATCH v5 2/4] tpm: implement virtual memory device for TPM PPI

[Laszlo Ersek]

On 06/27/18 17:05, Igor Mammedov wrote:
> On Wed, 27 Jun 2018 10:36:52 -0400
> Stefan Berger <stefanb@linux.vnet.ibm.com> wrote:
> 
>> On 06/27/2018 10:19 AM, Igor Mammedov wrote:
>>> On Wed, 27 Jun 2018 08:53:28 -0400
>>> Stefan Berger <stefanb@linux.vnet.ibm.com> wrote:
>>>  
>>>> On 06/27/2018 07:44 AM, Igor Mammedov wrote:  
>>>>> On Tue, 26 Jun 2018 14:23:41 +0200
>>>>> Marc-André Lureau <marcandre.lureau@redhat.com> wrote:
>>>>>     
>>>>>> From: Stefan Berger <stefanb@linux.vnet.ibm.com>
>>>>>>
>>>>>> Implement a virtual memory device for the TPM Physical Presence interface.
>>>>>> The memory is located at 0xFED45000 and used by ACPI to send messages to the
>>>>>> firmware (BIOS) and by the firmware to provide parameters for each one of
>>>>>> the supported codes.
>>>>>>
>>>>>> This device should be used by all TPM interfaces on x86 and can be added
>>>>>> by calling tpm_ppi_init_io().
>>>>>>
>>>>>> Signed-off-by: Stefan Berger <stefanb@linux.vnet.ibm.com>
>>>>>> Signed-off-by: Marc-André Lureau <marcandre.lureau@redhat.com>
>>>>>>
>>>>>> ---
>>>>>>
>>>>>> v4 (Marc-André):
>>>>>>    - pass TPM_PPI_ADDR_BASE as argument to tpm_ppi_init_io()
>>>>>>    - only enable PPI if property is set
>>>>>>
>>>>>> v3 (Marc-André):
>>>>>>    - merge CRB support
>>>>>>    - use trace events instead of DEBUG printf
>>>>>>    - headers inclusion simplification
>>>>>>
>>>>>> v2:
>>>>>>    - moved to byte access since an infrequently used device;
>>>>>>      this simplifies code
>>>>>>    - increase size of device to 0x400
>>>>>>    - move device to 0xfffef000 since SeaBIOS has some code at 0xffff0000:
>>>>>>      'On the emulators, the bios at 0xf0000 is also at 0xffff0000'
>>>>>> ---
>>>>>>    hw/tpm/tpm_ppi.h      | 27 ++++++++++++++++++++
>>>>>>    include/hw/acpi/tpm.h |  6 +++++
>>>>>>    hw/tpm/tpm_crb.c      |  7 ++++++
>>>>>>    hw/tpm/tpm_ppi.c      | 57 +++++++++++++++++++++++++++++++++++++++++++
>>>>>>    hw/tpm/tpm_tis.c      |  7 ++++++
>>>>>>    hw/tpm/Makefile.objs  |  2 +-
>>>>>>    hw/tpm/trace-events   |  4 +++
>>>>>>    7 files changed, 109 insertions(+), 1 deletion(-)
>>>>>>    create mode 100644 hw/tpm/tpm_ppi.h
>>>>>>    create mode 100644 hw/tpm/tpm_ppi.c
>>>>>>
>>>>>> diff --git a/hw/tpm/tpm_ppi.h b/hw/tpm/tpm_ppi.h
>>>>>> new file mode 100644
>>>>>> index 0000000000..ac7ad47238
>>>>>> --- /dev/null
>>>>>> +++ b/hw/tpm/tpm_ppi.h
>>>>>> @@ -0,0 +1,27 @@
>>>>>> +/*
>>>>>> + * TPM Physical Presence Interface
>>>>>> + *
>>>>>> + * Copyright (C) 2018 IBM Corporation
>>>>>> + *
>>>>>> + * Authors:
>>>>>> + *  Stefan Berger    <stefanb@us.ibm.com>
>>>>>> + *
>>>>>> + * This work is licensed under the terms of the GNU GPL, version 2 or later.
>>>>>> + * See the COPYING file in the top-level directory.
>>>>>> + */
>>>>>> +#ifndef TPM_TPM_PPI_H
>>>>>> +#define TPM_TPM_PPI_H
>>>>>> +
>>>>>> +#include "hw/acpi/tpm.h"
>>>>>> +#include "exec/address-spaces.h"
>>>>>> +
>>>>>> +typedef struct TPMPPI {
>>>>>> +    MemoryRegion mmio;
>>>>>> +
>>>>>> +    uint8_t ram[TPM_PPI_ADDR_SIZE];
>>>>>> +} TPMPPI;  
>>>>> I probably miss something obvious here,
>>>>> 1st:
>>>>> commit message says that memory reion is supposed to be interface
>>>>> between FW and OSPM (i.e. totally guest internal thingy).
>>>>> So question is:
>>>>>     why do we register memory region at all?  
>>>> One reason for the device itself was being able to debug the interaction
>>>> of the guest with ACPI though I had additional instrumentation for that
>>>> showing register contents.
>>>> We need it to have some memory in the region where we place it. I
>>>> suppose a memory_region_init_ram() would provide migration support
>>>> automatically but cannot be used on memory where we have
>>>> MemoryRegionOps. So we could drop most parts of the device and only run
>>>> memory_region_init_ram() ?  
>>> if QEMU doesn't need to touch it ever, you could do even better.  
>>
>> QEMU does indirectly touch it in 4/4 where we define the 
>> OperationRegion()s and need to know where they are located in memory. We 
>> could read the base address that is now TPM_PPI_ADDR_BASE from a hard 
>> coded memory location
> that's done for you by bios_linker_loader_add_pointer() when
> ACPI tables are installed by FW.
> 
>> and pass it into OperationRegion(), but I doubt we 
>> would want that.
>>
>> +    aml_append(dev,
>> +               aml_operation_region("TPP1", AML_SYSTEM_MEMORY,
>> +                                    aml_int(TPM_PPI_ADDR_BASE), 0x100));
>>
>>
>> +    aml_append(dev,
>> +               aml_operation_region("TPP2", AML_SYSTEM_MEMORY,
>> +                                    aml_int(TPM_PPI_ADDR_BASE + 0x100),
>> +                                    0x5A));
> that's possible, usually it works as dynamic memory region where region
> lives within scope of a method.
> 
> but scratch it.
> As Andre pointed out reserved memory should stay at the same place
> across reboots and might be needed before ACPI tables are installed,
> which probably is impossible.
> 
> CCing Laszlo just in case if I'm wrong.

Stability of reserved memory areas is only guaranteed across S3 resume.
Through a normal reboot, all DRAM is considered invalidated.

Thanks
Laszlo

[Qemu-devel] [PATCH v5 3/4] acpi: add fw_cfg file for TPM and PPI virtual memory device

[Marc-André Lureau]

From: Stefan Berger <stefanb@linux.vnet.ibm.com>

To avoid having to hard code the base address of the PPI virtual
memory device we introduce a fw_cfg file etc/tpm/config that holds the
base address of the PPI device, the version of the PPI interface and
the version of the attached TPM.

Signed-off-by: Stefan Berger <stefanb@linux.vnet.ibm.com>
[ Marc-André: renamed to etc/tpm/config, made it static, document it ]
Signed-off-by: Marc-André Lureau <marcandre.lureau@redhat.com>

---

v4:
 - add ACPI only if PPI is enabled
v3:
 - renamed to etc/tpm/config, made it static, document it
---
 include/hw/acpi/tpm.h |  3 +++
 hw/i386/acpi-build.c  | 19 +++++++++++++++++++
 docs/specs/tpm.txt    | 20 ++++++++++++++++++++
 3 files changed, 42 insertions(+)

diff --git a/include/hw/acpi/tpm.h b/include/hw/acpi/tpm.h
index c082df7d1d..f79d68a77a 100644
--- a/include/hw/acpi/tpm.h
+++ b/include/hw/acpi/tpm.h
@@ -193,4 +193,7 @@ REG32(CRB_DATA_BUFFER, 0x80)
 #define TPM_PPI_ADDR_SIZE           0x400
 #define TPM_PPI_ADDR_BASE           0xFED45000
 
+#define TPM_PPI_VERSION_NONE        0
+#define TPM_PPI_VERSION_1_30        1
+
 #endif /* HW_ACPI_TPM_H */
diff --git a/hw/i386/acpi-build.c b/hw/i386/acpi-build.c
index 9bc6d97ea1..d9320845ed 100644
--- a/hw/i386/acpi-build.c
+++ b/hw/i386/acpi-build.c
@@ -119,6 +119,12 @@ typedef struct AcpiBuildPciBusHotplugState {
     bool pcihp_bridge_en;
 } AcpiBuildPciBusHotplugState;
 
+typedef struct FWCfgTPMConfig {
+    uint32_t tpmppi_address;
+    uint8_t tpm_version;
+    uint8_t tpmppi_version;
+} QEMU_PACKED FWCfgTPMConfig;
+
 static void init_common_fadt_data(Object *o, AcpiFadtData *data)
 {
     uint32_t io = object_property_get_uint(o, ACPI_PM_PROP_PM_IO_BASE, NULL);
@@ -2873,6 +2879,8 @@ void acpi_setup(void)
     AcpiBuildTables tables;
     AcpiBuildState *build_state;
     Object *vmgenid_dev;
+    TPMIf *tpm;
+    static FWCfgTPMConfig tpm_config;
 
     if (!pcms->fw_cfg) {
         ACPI_BUILD_DPRINTF("No fw cfg. Bailing out.\n");
@@ -2907,6 +2915,17 @@ void acpi_setup(void)
     fw_cfg_add_file(pcms->fw_cfg, ACPI_BUILD_TPMLOG_FILE,
                     tables.tcpalog->data, acpi_data_len(tables.tcpalog));
 
+    tpm = tpm_find();
+    if (tpm && object_property_get_bool(OBJECT(tpm), "ppi", &error_abort)) {
+        tpm_config = (FWCfgTPMConfig) {
+            .tpmppi_address = cpu_to_le32(TPM_PPI_ADDR_BASE),
+            .tpm_version = cpu_to_le32(tpm_get_version(tpm_find())),
+            .tpmppi_version = cpu_to_le32(TPM_PPI_VERSION_NONE)
+        };
+        fw_cfg_add_file(pcms->fw_cfg, "etc/tpm/config",
+                        &tpm_config, sizeof tpm_config);
+    }
+
     vmgenid_dev = find_vmgenid_dev();
     if (vmgenid_dev) {
         vmgenid_add_fw_cfg(VMGENID(vmgenid_dev), pcms->fw_cfg,
diff --git a/docs/specs/tpm.txt b/docs/specs/tpm.txt
index c230c4c93e..2ddb768084 100644
--- a/docs/specs/tpm.txt
+++ b/docs/specs/tpm.txt
@@ -20,6 +20,26 @@ QEMU files related to TPM TIS interface:
  - hw/tpm/tpm_tis.h
 
 
+= fw_cfg interface =
+
+The bios/firmware may use the "etc/tpm/config" fw_cfg entry for
+configuring the guest appropriately.
+
+The entry of 6 bytes has the following content, in little-endian:
+
+    #define TPM_VERSION_UNSPEC          0
+    #define TPM_VERSION_1_2             1
+    #define TPM_VERSION_2_0             2
+
+    #define TPM_PPI_VERSION_NONE        0
+    #define TPM_PPI_VERSION_1_30        1
+
+    struct FWCfgTPMConfig {
+        uint32_t tpmppi_address;         /* PPI memory location */
+        uint8_t tpm_version;             /* TPM version */
+        uint8_t tpmppi_version;          /* PPI version */
+    };
+
 = ACPI Interface =
 
 The TPM device is defined with ACPI ID "PNP0C31". QEMU builds a SSDT and passes
-- 
2.18.0.rc1

Re: [Qemu-devel] [PATCH v5 3/4] acpi: add fw_cfg file for TPM and PPI virtual memory device

[Igor Mammedov]

On Tue, 26 Jun 2018 14:23:42 +0200
Marc-André Lureau <marcandre.lureau@redhat.com> wrote:

> From: Stefan Berger <stefanb@linux.vnet.ibm.com>
> 
> To avoid having to hard code the base address of the PPI virtual
> memory device we introduce a fw_cfg file etc/tpm/config that holds the
> base address of the PPI device, the version of the PPI interface and
> the version of the attached TPM.
> 
> Signed-off-by: Stefan Berger <stefanb@linux.vnet.ibm.com>
> [ Marc-André: renamed to etc/tpm/config, made it static, document it ]
> Signed-off-by: Marc-André Lureau <marcandre.lureau@redhat.com>
> 
> ---
> 
> v4:
>  - add ACPI only if PPI is enabled
> v3:
>  - renamed to etc/tpm/config, made it static, document it
> ---
>  include/hw/acpi/tpm.h |  3 +++
>  hw/i386/acpi-build.c  | 19 +++++++++++++++++++
>  docs/specs/tpm.txt    | 20 ++++++++++++++++++++
>  3 files changed, 42 insertions(+)
> 
> diff --git a/include/hw/acpi/tpm.h b/include/hw/acpi/tpm.h
> index c082df7d1d..f79d68a77a 100644
> --- a/include/hw/acpi/tpm.h
> +++ b/include/hw/acpi/tpm.h
> @@ -193,4 +193,7 @@ REG32(CRB_DATA_BUFFER, 0x80)
>  #define TPM_PPI_ADDR_SIZE           0x400
>  #define TPM_PPI_ADDR_BASE           0xFED45000
>  
> +#define TPM_PPI_VERSION_NONE        0
> +#define TPM_PPI_VERSION_1_30        1
> +
>  #endif /* HW_ACPI_TPM_H */
> diff --git a/hw/i386/acpi-build.c b/hw/i386/acpi-build.c
> index 9bc6d97ea1..d9320845ed 100644
> --- a/hw/i386/acpi-build.c
> +++ b/hw/i386/acpi-build.c
> @@ -119,6 +119,12 @@ typedef struct AcpiBuildPciBusHotplugState {
>      bool pcihp_bridge_en;
>  } AcpiBuildPciBusHotplugState;
>  
> +typedef struct FWCfgTPMConfig {
> +    uint32_t tpmppi_address;
is 32bit enough (what if on ARM or somewhere else area would be above 4Gb)?
to be future proof I'd make it 64bit field so we won't have to change ABI
later on.

> +    uint8_t tpm_version;
> +    uint8_t tpmppi_version;
> +} QEMU_PACKED FWCfgTPMConfig;
> +
>  static void init_common_fadt_data(Object *o, AcpiFadtData *data)
>  {
>      uint32_t io = object_property_get_uint(o, ACPI_PM_PROP_PM_IO_BASE, NULL);
> @@ -2873,6 +2879,8 @@ void acpi_setup(void)
>      AcpiBuildTables tables;
>      AcpiBuildState *build_state;
>      Object *vmgenid_dev;
> +    TPMIf *tpm;
> +    static FWCfgTPMConfig tpm_config;
>  
>      if (!pcms->fw_cfg) {
>          ACPI_BUILD_DPRINTF("No fw cfg. Bailing out.\n");
> @@ -2907,6 +2915,17 @@ void acpi_setup(void)
what about vart-arm machine?
(it also has some TPM stuff but it doesn't use acpi_setup())
maybe add common helper and reuse it for both?


>      fw_cfg_add_file(pcms->fw_cfg, ACPI_BUILD_TPMLOG_FILE,
>                      tables.tcpalog->data, acpi_data_len(tables.tcpalog));
>  
> +    tpm = tpm_find();
> +    if (tpm && object_property_get_bool(OBJECT(tpm), "ppi", &error_abort)) {
> +        tpm_config = (FWCfgTPMConfig) {
> +            .tpmppi_address = cpu_to_le32(TPM_PPI_ADDR_BASE),
> +            .tpm_version = cpu_to_le32(tpm_get_version(tpm_find())),
Have it actually been tested on big endian host?

> +            .tpmppi_version = cpu_to_le32(TPM_PPI_VERSION_NONE)
ditto

(that's why I don't welcome packed structures in random places)

how about adding unit-tests to series to make sure that it works
across different hosts that travis builds on and that thing won't
fall apart in future?

> +        };
> +        fw_cfg_add_file(pcms->fw_cfg, "etc/tpm/config",
> +                        &tpm_config, sizeof tpm_config);
> +    }
> +
>      vmgenid_dev = find_vmgenid_dev();
>      if (vmgenid_dev) {
>          vmgenid_add_fw_cfg(VMGENID(vmgenid_dev), pcms->fw_cfg,
> diff --git a/docs/specs/tpm.txt b/docs/specs/tpm.txt
> index c230c4c93e..2ddb768084 100644
> --- a/docs/specs/tpm.txt
> +++ b/docs/specs/tpm.txt
> @@ -20,6 +20,26 @@ QEMU files related to TPM TIS interface:
>   - hw/tpm/tpm_tis.h
>  
>  
> += fw_cfg interface =
> +
> +The bios/firmware may use the "etc/tpm/config" fw_cfg entry for
> +configuring the guest appropriately.
> +
> +The entry of 6 bytes has the following content, in little-endian:
> +
> +    #define TPM_VERSION_UNSPEC          0
> +    #define TPM_VERSION_1_2             1
> +    #define TPM_VERSION_2_0             2
> +
> +    #define TPM_PPI_VERSION_NONE        0
> +    #define TPM_PPI_VERSION_1_30        1
> +
> +    struct FWCfgTPMConfig {
> +        uint32_t tpmppi_address;         /* PPI memory location */
> +        uint8_t tpm_version;             /* TPM version */
> +        uint8_t tpmppi_version;          /* PPI version */
> +    };
> +
>  = ACPI Interface =
>  
>  The TPM device is defined with ACPI ID "PNP0C31". QEMU builds a SSDT and passes

Re: [Qemu-devel] [PATCH v5 3/4] acpi: add fw_cfg file for TPM and PPI virtual memory device

[Stefan Berger]

On 06/27/2018 08:01 AM, Igor Mammedov wrote:
> On Tue, 26 Jun 2018 14:23:42 +0200
> Marc-André Lureau <marcandre.lureau@redhat.com> wrote:
>
>> From: Stefan Berger <stefanb@linux.vnet.ibm.com>
>>
>> To avoid having to hard code the base address of the PPI virtual
>> memory device we introduce a fw_cfg file etc/tpm/config that holds the
>> base address of the PPI device, the version of the PPI interface and
>> the version of the attached TPM.
>>
>> Signed-off-by: Stefan Berger <stefanb@linux.vnet.ibm.com>
>> [ Marc-André: renamed to etc/tpm/config, made it static, document it ]
>> Signed-off-by: Marc-André Lureau <marcandre.lureau@redhat.com>
>>
>> ---
>>
>> v4:
>>   - add ACPI only if PPI is enabled
>> v3:
>>   - renamed to etc/tpm/config, made it static, document it
>> ---
>>   include/hw/acpi/tpm.h |  3 +++
>>   hw/i386/acpi-build.c  | 19 +++++++++++++++++++
>>   docs/specs/tpm.txt    | 20 ++++++++++++++++++++
>>   3 files changed, 42 insertions(+)
>>
>> diff --git a/include/hw/acpi/tpm.h b/include/hw/acpi/tpm.h
>> index c082df7d1d..f79d68a77a 100644
>> --- a/include/hw/acpi/tpm.h
>> +++ b/include/hw/acpi/tpm.h
>> @@ -193,4 +193,7 @@ REG32(CRB_DATA_BUFFER, 0x80)
>>   #define TPM_PPI_ADDR_SIZE           0x400
>>   #define TPM_PPI_ADDR_BASE           0xFED45000
>>   
>> +#define TPM_PPI_VERSION_NONE        0
>> +#define TPM_PPI_VERSION_1_30        1
>> +
>>   #endif /* HW_ACPI_TPM_H */
>> diff --git a/hw/i386/acpi-build.c b/hw/i386/acpi-build.c
>> index 9bc6d97ea1..d9320845ed 100644
>> --- a/hw/i386/acpi-build.c
>> +++ b/hw/i386/acpi-build.c
>> @@ -119,6 +119,12 @@ typedef struct AcpiBuildPciBusHotplugState {
>>       bool pcihp_bridge_en;
>>   } AcpiBuildPciBusHotplugState;
>>   
>> +typedef struct FWCfgTPMConfig {
>> +    uint32_t tpmppi_address;
> is 32bit enough (what if on ARM or somewhere else area would be above 4Gb)?
> to be future proof I'd make it 64bit field so we won't have to change ABI
> later on.
>
>> +    uint8_t tpm_version;
>> +    uint8_t tpmppi_version;
>> +} QEMU_PACKED FWCfgTPMConfig;
>> +
>>   static void init_common_fadt_data(Object *o, AcpiFadtData *data)
>>   {
>>       uint32_t io = object_property_get_uint(o, ACPI_PM_PROP_PM_IO_BASE, NULL);
>> @@ -2873,6 +2879,8 @@ void acpi_setup(void)
>>       AcpiBuildTables tables;
>>       AcpiBuildState *build_state;
>>       Object *vmgenid_dev;
>> +    TPMIf *tpm;
>> +    static FWCfgTPMConfig tpm_config;
>>   
>>       if (!pcms->fw_cfg) {
>>           ACPI_BUILD_DPRINTF("No fw cfg. Bailing out.\n");
>> @@ -2907,6 +2915,17 @@ void acpi_setup(void)
> what about vart-arm machine?
> (it also has some TPM stuff but it doesn't use acpi_setup())
> maybe add common helper and reuse it for both?

I have never used ARM with it. I am not sure whether the firmware on ARM 
is instrumented to have support for PPI. If someone wanted to enable 
that, I would leave these parts up to them.

>
>
>>       fw_cfg_add_file(pcms->fw_cfg, ACPI_BUILD_TPMLOG_FILE,
>>                       tables.tcpalog->data, acpi_data_len(tables.tcpalog));
>>   
>> +    tpm = tpm_find();
>> +    if (tpm && object_property_get_bool(OBJECT(tpm), "ppi", &error_abort)) {
>> +        tpm_config = (FWCfgTPMConfig) {
>> +            .tpmppi_address = cpu_to_le32(TPM_PPI_ADDR_BASE),
>> +            .tpm_version = cpu_to_le32(tpm_get_version(tpm_find())),
> Have it actually been tested on big endian host?

At some point I did, yes.

>
>> +            .tpmppi_version = cpu_to_le32(TPM_PPI_VERSION_NONE)
> ditto
>
> (that's why I don't welcome packed structures in random places)

I thought a packed structure at least ensures that the offsets of the 
fields are agreed upon by 32 and 64bit, no ? So the firmware can use 64 
bit or 32bit and the offsets would be ensured.

>
> how about adding unit-tests to series to make sure that it works
> across different hosts that travis builds on and that thing won't
> fall apart in future?

>> +        };
>> +        fw_cfg_add_file(pcms->fw_cfg, "etc/tpm/config",
>> +                        &tpm_config, sizeof tpm_config);
>> +    }
>> +
>>       vmgenid_dev = find_vmgenid_dev();
>>       if (vmgenid_dev) {
>>           vmgenid_add_fw_cfg(VMGENID(vmgenid_dev), pcms->fw_cfg,
>> diff --git a/docs/specs/tpm.txt b/docs/specs/tpm.txt
>> index c230c4c93e..2ddb768084 100644
>> --- a/docs/specs/tpm.txt
>> +++ b/docs/specs/tpm.txt
>> @@ -20,6 +20,26 @@ QEMU files related to TPM TIS interface:
>>    - hw/tpm/tpm_tis.h
>>   
>>   
>> += fw_cfg interface =
>> +
>> +The bios/firmware may use the "etc/tpm/config" fw_cfg entry for
>> +configuring the guest appropriately.
>> +
>> +The entry of 6 bytes has the following content, in little-endian:
>> +
>> +    #define TPM_VERSION_UNSPEC          0
>> +    #define TPM_VERSION_1_2             1
>> +    #define TPM_VERSION_2_0             2
>> +
>> +    #define TPM_PPI_VERSION_NONE        0
>> +    #define TPM_PPI_VERSION_1_30        1
>> +
>> +    struct FWCfgTPMConfig {
>> +        uint32_t tpmppi_address;         /* PPI memory location */
>> +        uint8_t tpm_version;             /* TPM version */
>> +        uint8_t tpmppi_version;          /* PPI version */
>> +    };
>> +
>>   = ACPI Interface =
>>   
>>   The TPM device is defined with ACPI ID "PNP0C31". QEMU builds a SSDT and passes

Re: [Qemu-devel] [PATCH v5 3/4] acpi: add fw_cfg file for TPM and PPI virtual memory device

[Igor Mammedov]

On Wed, 27 Jun 2018 08:59:55 -0400
Stefan Berger <stefanb@linux.vnet.ibm.com> wrote:

> On 06/27/2018 08:01 AM, Igor Mammedov wrote:
> > On Tue, 26 Jun 2018 14:23:42 +0200
> > Marc-André Lureau <marcandre.lureau@redhat.com> wrote:
> >  
> >> From: Stefan Berger <stefanb@linux.vnet.ibm.com>
> >>
> >> To avoid having to hard code the base address of the PPI virtual
> >> memory device we introduce a fw_cfg file etc/tpm/config that holds the
> >> base address of the PPI device, the version of the PPI interface and
> >> the version of the attached TPM.
> >>
> >> Signed-off-by: Stefan Berger <stefanb@linux.vnet.ibm.com>
> >> [ Marc-André: renamed to etc/tpm/config, made it static, document it ]
> >> Signed-off-by: Marc-André Lureau <marcandre.lureau@redhat.com>
> >>
> >> ---
> >>
> >> v4:
> >>   - add ACPI only if PPI is enabled
> >> v3:
> >>   - renamed to etc/tpm/config, made it static, document it
> >> ---
> >>   include/hw/acpi/tpm.h |  3 +++
> >>   hw/i386/acpi-build.c  | 19 +++++++++++++++++++
> >>   docs/specs/tpm.txt    | 20 ++++++++++++++++++++
> >>   3 files changed, 42 insertions(+)
> >>
> >> diff --git a/include/hw/acpi/tpm.h b/include/hw/acpi/tpm.h
> >> index c082df7d1d..f79d68a77a 100644
> >> --- a/include/hw/acpi/tpm.h
> >> +++ b/include/hw/acpi/tpm.h
> >> @@ -193,4 +193,7 @@ REG32(CRB_DATA_BUFFER, 0x80)
> >>   #define TPM_PPI_ADDR_SIZE           0x400
> >>   #define TPM_PPI_ADDR_BASE           0xFED45000
> >>   
> >> +#define TPM_PPI_VERSION_NONE        0
> >> +#define TPM_PPI_VERSION_1_30        1
> >> +
> >>   #endif /* HW_ACPI_TPM_H */
> >> diff --git a/hw/i386/acpi-build.c b/hw/i386/acpi-build.c
> >> index 9bc6d97ea1..d9320845ed 100644
> >> --- a/hw/i386/acpi-build.c
> >> +++ b/hw/i386/acpi-build.c
> >> @@ -119,6 +119,12 @@ typedef struct AcpiBuildPciBusHotplugState {
> >>       bool pcihp_bridge_en;
> >>   } AcpiBuildPciBusHotplugState;
> >>   
> >> +typedef struct FWCfgTPMConfig {
> >> +    uint32_t tpmppi_address;  
> > is 32bit enough (what if on ARM or somewhere else area would be above 4Gb)?
> > to be future proof I'd make it 64bit field so we won't have to change ABI
> > later on.
> >  
> >> +    uint8_t tpm_version;
> >> +    uint8_t tpmppi_version;
> >> +} QEMU_PACKED FWCfgTPMConfig;
> >> +
> >>   static void init_common_fadt_data(Object *o, AcpiFadtData *data)
> >>   {
> >>       uint32_t io = object_property_get_uint(o, ACPI_PM_PROP_PM_IO_BASE, NULL);
> >> @@ -2873,6 +2879,8 @@ void acpi_setup(void)
> >>       AcpiBuildTables tables;
> >>       AcpiBuildState *build_state;
> >>       Object *vmgenid_dev;
> >> +    TPMIf *tpm;
> >> +    static FWCfgTPMConfig tpm_config;
> >>   
> >>       if (!pcms->fw_cfg) {
> >>           ACPI_BUILD_DPRINTF("No fw cfg. Bailing out.\n");
> >> @@ -2907,6 +2915,17 @@ void acpi_setup(void)  
> > what about vart-arm machine?
> > (it also has some TPM stuff but it doesn't use acpi_setup())
> > maybe add common helper and reuse it for both?  
> 
> I have never used ARM with it. I am not sure whether the firmware on ARM 
> is instrumented to have support for PPI. If someone wanted to enable 
> that, I would leave these parts up to them.
> 
> >
> >  
> >>       fw_cfg_add_file(pcms->fw_cfg, ACPI_BUILD_TPMLOG_FILE,
> >>                       tables.tcpalog->data, acpi_data_len(tables.tcpalog));
> >>   
> >> +    tpm = tpm_find();
> >> +    if (tpm && object_property_get_bool(OBJECT(tpm), "ppi", &error_abort)) {
> >> +        tpm_config = (FWCfgTPMConfig) {
> >> +            .tpmppi_address = cpu_to_le32(TPM_PPI_ADDR_BASE),
> >> +            .tpm_version = cpu_to_le32(tpm_get_version(tpm_find())),  
> > Have it actually been tested on big endian host?  
> 
> At some point I did, yes.
> 
> >  
> >> +            .tpmppi_version = cpu_to_le32(TPM_PPI_VERSION_NONE)  
> > ditto
> >
> > (that's why I don't welcome packed structures in random places)  
> 
> I thought a packed structure at least ensures that the offsets of the 
> fields are agreed upon by 32 and 64bit, no ? So the firmware can use 64 
> bit or 32bit and the offsets would be ensured.

I think layout for this structure is:
  0-3 : tpmppi_address
    4 : tpm_version
    5 : tpmppi_version

but that's not a problem,

  unit8_t foo = cpu_to_le32(bar)

wouldn't it produce different results depending on host endianness?

> 
> >
> > how about adding unit-tests to series to make sure that it works
> > across different hosts that travis builds on and that thing won't
> > fall apart in future?  
> 
> >> +        };
> >> +        fw_cfg_add_file(pcms->fw_cfg, "etc/tpm/config",
> >> +                        &tpm_config, sizeof tpm_config);
> >> +    }
> >> +
> >>       vmgenid_dev = find_vmgenid_dev();
> >>       if (vmgenid_dev) {
> >>           vmgenid_add_fw_cfg(VMGENID(vmgenid_dev), pcms->fw_cfg,
> >> diff --git a/docs/specs/tpm.txt b/docs/specs/tpm.txt
> >> index c230c4c93e..2ddb768084 100644
> >> --- a/docs/specs/tpm.txt
> >> +++ b/docs/specs/tpm.txt
> >> @@ -20,6 +20,26 @@ QEMU files related to TPM TIS interface:
> >>    - hw/tpm/tpm_tis.h
> >>   
> >>   
> >> += fw_cfg interface =
> >> +
> >> +The bios/firmware may use the "etc/tpm/config" fw_cfg entry for
> >> +configuring the guest appropriately.
> >> +
> >> +The entry of 6 bytes has the following content, in little-endian:
> >> +
> >> +    #define TPM_VERSION_UNSPEC          0
> >> +    #define TPM_VERSION_1_2             1
> >> +    #define TPM_VERSION_2_0             2
> >> +
> >> +    #define TPM_PPI_VERSION_NONE        0
> >> +    #define TPM_PPI_VERSION_1_30        1
> >> +
> >> +    struct FWCfgTPMConfig {
> >> +        uint32_t tpmppi_address;         /* PPI memory location */
> >> +        uint8_t tpm_version;             /* TPM version */
> >> +        uint8_t tpmppi_version;          /* PPI version */
> >> +    };
> >> +
> >>   = ACPI Interface =
> >>   
> >>   The TPM device is defined with ACPI ID "PNP0C31". QEMU builds a SSDT and passes  
> 
> 

Re: [Qemu-devel] [PATCH v5 3/4] acpi: add fw_cfg file for TPM and PPI virtual memory device

[Marc-André Lureau]

Hi

On Wed, Jun 27, 2018 at 4:26 PM, Igor Mammedov <imammedo@redhat.com> wrote:
> On Wed, 27 Jun 2018 08:59:55 -0400
> Stefan Berger <stefanb@linux.vnet.ibm.com> wrote:
>
>> On 06/27/2018 08:01 AM, Igor Mammedov wrote:
>> > On Tue, 26 Jun 2018 14:23:42 +0200
>> > Marc-André Lureau <marcandre.lureau@redhat.com> wrote:
>> >
>> >> From: Stefan Berger <stefanb@linux.vnet.ibm.com>
>> >>
>> >> To avoid having to hard code the base address of the PPI virtual
>> >> memory device we introduce a fw_cfg file etc/tpm/config that holds the
>> >> base address of the PPI device, the version of the PPI interface and
>> >> the version of the attached TPM.
>> >>
>> >> Signed-off-by: Stefan Berger <stefanb@linux.vnet.ibm.com>
>> >> [ Marc-André: renamed to etc/tpm/config, made it static, document it ]
>> >> Signed-off-by: Marc-André Lureau <marcandre.lureau@redhat.com>
>> >>
>> >> ---
>> >>
>> >> v4:
>> >>   - add ACPI only if PPI is enabled
>> >> v3:
>> >>   - renamed to etc/tpm/config, made it static, document it
>> >> ---
>> >>   include/hw/acpi/tpm.h |  3 +++
>> >>   hw/i386/acpi-build.c  | 19 +++++++++++++++++++
>> >>   docs/specs/tpm.txt    | 20 ++++++++++++++++++++
>> >>   3 files changed, 42 insertions(+)
>> >>
>> >> diff --git a/include/hw/acpi/tpm.h b/include/hw/acpi/tpm.h
>> >> index c082df7d1d..f79d68a77a 100644
>> >> --- a/include/hw/acpi/tpm.h
>> >> +++ b/include/hw/acpi/tpm.h
>> >> @@ -193,4 +193,7 @@ REG32(CRB_DATA_BUFFER, 0x80)
>> >>   #define TPM_PPI_ADDR_SIZE           0x400
>> >>   #define TPM_PPI_ADDR_BASE           0xFED45000
>> >>
>> >> +#define TPM_PPI_VERSION_NONE        0
>> >> +#define TPM_PPI_VERSION_1_30        1
>> >> +
>> >>   #endif /* HW_ACPI_TPM_H */
>> >> diff --git a/hw/i386/acpi-build.c b/hw/i386/acpi-build.c
>> >> index 9bc6d97ea1..d9320845ed 100644
>> >> --- a/hw/i386/acpi-build.c
>> >> +++ b/hw/i386/acpi-build.c
>> >> @@ -119,6 +119,12 @@ typedef struct AcpiBuildPciBusHotplugState {
>> >>       bool pcihp_bridge_en;
>> >>   } AcpiBuildPciBusHotplugState;
>> >>
>> >> +typedef struct FWCfgTPMConfig {
>> >> +    uint32_t tpmppi_address;
>> > is 32bit enough (what if on ARM or somewhere else area would be above 4Gb)?
>> > to be future proof I'd make it 64bit field so we won't have to change ABI
>> > later on.
>> >
>> >> +    uint8_t tpm_version;
>> >> +    uint8_t tpmppi_version;
>> >> +} QEMU_PACKED FWCfgTPMConfig;
>> >> +
>> >>   static void init_common_fadt_data(Object *o, AcpiFadtData *data)
>> >>   {
>> >>       uint32_t io = object_property_get_uint(o, ACPI_PM_PROP_PM_IO_BASE, NULL);
>> >> @@ -2873,6 +2879,8 @@ void acpi_setup(void)
>> >>       AcpiBuildTables tables;
>> >>       AcpiBuildState *build_state;
>> >>       Object *vmgenid_dev;
>> >> +    TPMIf *tpm;
>> >> +    static FWCfgTPMConfig tpm_config;
>> >>
>> >>       if (!pcms->fw_cfg) {
>> >>           ACPI_BUILD_DPRINTF("No fw cfg. Bailing out.\n");
>> >> @@ -2907,6 +2915,17 @@ void acpi_setup(void)
>> > what about vart-arm machine?
>> > (it also has some TPM stuff but it doesn't use acpi_setup())
>> > maybe add common helper and reuse it for both?
>>
>> I have never used ARM with it. I am not sure whether the firmware on ARM
>> is instrumented to have support for PPI. If someone wanted to enable
>> that, I would leave these parts up to them.
>>
>> >
>> >
>> >>       fw_cfg_add_file(pcms->fw_cfg, ACPI_BUILD_TPMLOG_FILE,
>> >>                       tables.tcpalog->data, acpi_data_len(tables.tcpalog));
>> >>
>> >> +    tpm = tpm_find();
>> >> +    if (tpm && object_property_get_bool(OBJECT(tpm), "ppi", &error_abort)) {
>> >> +        tpm_config = (FWCfgTPMConfig) {
>> >> +            .tpmppi_address = cpu_to_le32(TPM_PPI_ADDR_BASE),
>> >> +            .tpm_version = cpu_to_le32(tpm_get_version(tpm_find())),
>> > Have it actually been tested on big endian host?
>>
>> At some point I did, yes.
>>
>> >
>> >> +            .tpmppi_version = cpu_to_le32(TPM_PPI_VERSION_NONE)
>> > ditto
>> >
>> > (that's why I don't welcome packed structures in random places)
>>
>> I thought a packed structure at least ensures that the offsets of the
>> fields are agreed upon by 32 and 64bit, no ? So the firmware can use 64
>> bit or 32bit and the offsets would be ensured.
>
> I think layout for this structure is:
>   0-3 : tpmppi_address
>     4 : tpm_version
>     5 : tpmppi_version
>
> but that's not a problem,
>
>   unit8_t foo = cpu_to_le32(bar)
>
> wouldn't it produce different results depending on host endianness?

My understanding of C conversion from longer int to shorter ones is
that excessive higher order bits are dropped.

I think we could just remove the cpu_to_le32() call.

Re: [Qemu-devel] [PATCH v5 3/4] acpi: add fw_cfg file for TPM and PPI virtual memory device

[Igor Mammedov]

On Thu, 28 Jun 2018 14:42:34 +0200
Marc-André Lureau <marcandre.lureau@gmail.com> wrote:

> Hi
> 
> On Wed, Jun 27, 2018 at 4:26 PM, Igor Mammedov <imammedo@redhat.com> wrote:
> > On Wed, 27 Jun 2018 08:59:55 -0400
> > Stefan Berger <stefanb@linux.vnet.ibm.com> wrote:
> >  
> >> On 06/27/2018 08:01 AM, Igor Mammedov wrote:  
> >> > On Tue, 26 Jun 2018 14:23:42 +0200
> >> > Marc-André Lureau <marcandre.lureau@redhat.com> wrote:
> >> >  
> >> >> From: Stefan Berger <stefanb@linux.vnet.ibm.com>
> >> >>
> >> >> To avoid having to hard code the base address of the PPI virtual
> >> >> memory device we introduce a fw_cfg file etc/tpm/config that holds the
> >> >> base address of the PPI device, the version of the PPI interface and
> >> >> the version of the attached TPM.
> >> >>
> >> >> Signed-off-by: Stefan Berger <stefanb@linux.vnet.ibm.com>
> >> >> [ Marc-André: renamed to etc/tpm/config, made it static, document it ]
> >> >> Signed-off-by: Marc-André Lureau <marcandre.lureau@redhat.com>
> >> >>
> >> >> ---
> >> >>
> >> >> v4:
> >> >>   - add ACPI only if PPI is enabled
> >> >> v3:
> >> >>   - renamed to etc/tpm/config, made it static, document it
> >> >> ---
> >> >>   include/hw/acpi/tpm.h |  3 +++
> >> >>   hw/i386/acpi-build.c  | 19 +++++++++++++++++++
> >> >>   docs/specs/tpm.txt    | 20 ++++++++++++++++++++
> >> >>   3 files changed, 42 insertions(+)
> >> >>
> >> >> diff --git a/include/hw/acpi/tpm.h b/include/hw/acpi/tpm.h
> >> >> index c082df7d1d..f79d68a77a 100644
> >> >> --- a/include/hw/acpi/tpm.h
> >> >> +++ b/include/hw/acpi/tpm.h
> >> >> @@ -193,4 +193,7 @@ REG32(CRB_DATA_BUFFER, 0x80)
> >> >>   #define TPM_PPI_ADDR_SIZE           0x400
> >> >>   #define TPM_PPI_ADDR_BASE           0xFED45000
> >> >>
> >> >> +#define TPM_PPI_VERSION_NONE        0
> >> >> +#define TPM_PPI_VERSION_1_30        1
> >> >> +
> >> >>   #endif /* HW_ACPI_TPM_H */
> >> >> diff --git a/hw/i386/acpi-build.c b/hw/i386/acpi-build.c
> >> >> index 9bc6d97ea1..d9320845ed 100644
> >> >> --- a/hw/i386/acpi-build.c
> >> >> +++ b/hw/i386/acpi-build.c
> >> >> @@ -119,6 +119,12 @@ typedef struct AcpiBuildPciBusHotplugState {
> >> >>       bool pcihp_bridge_en;
> >> >>   } AcpiBuildPciBusHotplugState;
> >> >>
> >> >> +typedef struct FWCfgTPMConfig {
> >> >> +    uint32_t tpmppi_address;  
> >> > is 32bit enough (what if on ARM or somewhere else area would be above 4Gb)?
> >> > to be future proof I'd make it 64bit field so we won't have to change ABI
> >> > later on.
> >> >  
> >> >> +    uint8_t tpm_version;
> >> >> +    uint8_t tpmppi_version;
> >> >> +} QEMU_PACKED FWCfgTPMConfig;
> >> >> +
> >> >>   static void init_common_fadt_data(Object *o, AcpiFadtData *data)
> >> >>   {
> >> >>       uint32_t io = object_property_get_uint(o, ACPI_PM_PROP_PM_IO_BASE, NULL);
> >> >> @@ -2873,6 +2879,8 @@ void acpi_setup(void)
> >> >>       AcpiBuildTables tables;
> >> >>       AcpiBuildState *build_state;
> >> >>       Object *vmgenid_dev;
> >> >> +    TPMIf *tpm;
> >> >> +    static FWCfgTPMConfig tpm_config;
> >> >>
> >> >>       if (!pcms->fw_cfg) {
> >> >>           ACPI_BUILD_DPRINTF("No fw cfg. Bailing out.\n");
> >> >> @@ -2907,6 +2915,17 @@ void acpi_setup(void)  
> >> > what about vart-arm machine?
> >> > (it also has some TPM stuff but it doesn't use acpi_setup())
> >> > maybe add common helper and reuse it for both?  
> >>
> >> I have never used ARM with it. I am not sure whether the firmware on ARM
> >> is instrumented to have support for PPI. If someone wanted to enable
> >> that, I would leave these parts up to them.
> >>  
> >> >
> >> >  
> >> >>       fw_cfg_add_file(pcms->fw_cfg, ACPI_BUILD_TPMLOG_FILE,
> >> >>                       tables.tcpalog->data, acpi_data_len(tables.tcpalog));
> >> >>
> >> >> +    tpm = tpm_find();
> >> >> +    if (tpm && object_property_get_bool(OBJECT(tpm), "ppi", &error_abort)) {
> >> >> +        tpm_config = (FWCfgTPMConfig) {
> >> >> +            .tpmppi_address = cpu_to_le32(TPM_PPI_ADDR_BASE),
> >> >> +            .tpm_version = cpu_to_le32(tpm_get_version(tpm_find())),  
> >> > Have it actually been tested on big endian host?  
> >>
> >> At some point I did, yes.
> >>  
> >> >  
> >> >> +            .tpmppi_version = cpu_to_le32(TPM_PPI_VERSION_NONE)  
> >> > ditto
> >> >
> >> > (that's why I don't welcome packed structures in random places)  
> >>
> >> I thought a packed structure at least ensures that the offsets of the
> >> fields are agreed upon by 32 and 64bit, no ? So the firmware can use 64
> >> bit or 32bit and the offsets would be ensured.  
> >
> > I think layout for this structure is:
> >   0-3 : tpmppi_address
> >     4 : tpm_version
> >     5 : tpmppi_version
> >
> > but that's not a problem,
> >
> >   unit8_t foo = cpu_to_le32(bar)
> >
> > wouldn't it produce different results depending on host endianness?  
> 
> My understanding of C conversion from longer int to shorter ones is
> that excessive higher order bits are dropped.
> 
> I think we could just remove the cpu_to_le32() call.
Yep, that should fix the bug

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

[Marc-André Lureau]

From: Stefan Berger <stefanb@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 <stefanb@linux.vnet.ibm.com>

---

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)) {
+        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_append(dev, name);
+
+    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")));
+        {
+            func_idx = aml_local(0);
+            aml_append(ifctx,
+                       aml_store(aml_to_integer(aml_arg(2)), func_idx));
+
+            /* standard DSM query function */
+            func_idx = aml_local(0);
+            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
+             *
+             * Arg 2 (Integer): Function Index = 1
+             * Arg 3 (Package): Arguments = Empty Package
+             * Returns: Type: String
+             */
+            func_idx = aml_local(0);
+            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);
+            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)));
+
+                /* get opcode flags */
+                op = aml_local(0);
+                aml_append(ifctx2,
+                           aml_store(aml_call1("TPFN", op), aml_local(1)));
+
+                op_flags = aml_local(1);
+                /* if func[opcode] & TPM_PPI_FUNC_NOT_IMPLEMENTED */
+                ifctx3 = aml_if(
+                    aml_equal(
+                        aml_and(op_flags, aml_int(TPM_PPI_FUNC_MASK), NULL),
+                        aml_int(TPM_PPI_FUNC_NOT_IMPLEMENTED)));
+                {
+                    /* 1: not implemented */
+                    aml_append(ifctx3, aml_return(aml_int(1)));
+                }
+                aml_append(ifctx2, ifctx3);
+
+                op = aml_local(0);
+                aml_append(ifctx2, aml_store(op, aml_name("PPRQ")));
+                aml_append(ifctx2, aml_store(aml_int(0), aml_name("PPRM")));
+                /* 0: success */
+                aml_append(ifctx2, aml_return(aml_int(0)));
+            }
+            aml_append(ifctx, ifctx2);
+
+            /*
+             * 8.1.3 Get Pending TPM Operation Requested By the OS
+             *
+             * Arg 2 (Integer): Function Index = 3
+             * Arg 3 (Package): Arguments = Empty Package
+             * Returns: Type: Package of Integers
+             *          Integer 1: Function Return code
+             *                     0: Success
+             *                     1: General Failure
+             *          Integer 2: Pending operation requested by the OS
+             *                     0: None
+             *                    >0: Operation Value of the Pending Request
+             *          Integer 3: Optional argument to pending operation
+             *                     requested by the OS
+             *                     0: None
+             *                    >0: Argument Value of the Pending Request
+             */
+            func_idx = aml_local(0);
+            ifctx2 = aml_if(aml_equal(func_idx, aml_int(3)));
+            {
+                /* revision to integer */
+                aml_append(ifctx2,
+                           aml_store(
+                               aml_to_integer(aml_arg(1)),
+                               aml_local(1)));
+                /*
+                 * Revision ID of 1, no integer parameter beyond
+                 * parameter two are expected
+                 */
+                rev = aml_local(1);
+                ifctx3 = aml_if(aml_equal(rev, aml_int(1)));
+                {
+                    /* TPM2[1] = PPRQ */
+                    aml_append(ifctx3,
+                               aml_store(
+                                   aml_name("PPRQ"),
+                                   aml_index(aml_name("TPM2"), aml_int(1))));
+                    aml_append(ifctx3, aml_return(aml_name("TPM2")));
+                }
+                aml_append(ifctx2, ifctx3);
+
+                /*
+                 * A return value of {0, 23, 1} indicates that
+                 * operation 23 with argument 1 is pending.
+                 */
+                rev = aml_local(1);
+                ifctx3 = aml_if(aml_equal(rev, aml_int(2)));
+                {
+                    /* TPM3[1] = PPRQ */
+                    aml_append(ifctx3,
+                               aml_store(
+                                   aml_name("PPRQ"),
+                                   aml_index(aml_name("TPM3"), aml_int(1))));
+                    /* TPM3[2] = PPRM */
+                    aml_append(ifctx3,
+                               aml_store(
+                                   aml_name("PPRM"),
+                                   aml_index(aml_name("TPM3"), aml_int(2))));
+                    aml_append(ifctx3, aml_return(aml_name("TPM3")));
+                }
+                aml_append(ifctx2, ifctx3);
+            }
+            aml_append(ifctx, ifctx2);
+
+            /*
+             * 8.1.4 Get Platform-Specific Action to Transition to
+             *       Pre-OS Environment
+             *
+             * Arg 2 (Integer): Function Index = 4
+             * Arg 3 (Package): Arguments = Empty Package
+             * Returns: Type: Integer
+             *          0: None
+             *          1: Shutdown
+             *          2: Reboot
+             *          3: OS Vendor-specific
+             */
+            func_idx = aml_local(0);
+            ifctx2 = aml_if(aml_equal(func_idx, aml_int(4)));
+            {
+                /* reboot */
+                aml_append(ifctx2, aml_return(aml_int(2)));
+            }
+            aml_append(ifctx, ifctx2);
+
+            /*
+             * 8.1.5 Return TPM Operation Response to OS Environment
+             *
+             * Arg 2 (Integer): Function Index = 5
+             * Arg 3 (Package): Arguments = Empty Package
+             * Returns: Type: Package of Integer
+             *          Integer 1: Function Return code
+             *                     0: Success
+             *                     1: General Failure
+             *          Integer 2: Most recent operation request
+             *                     0: None
+             *                    >0: Operation Value of the most recent request
+             *          Integer 3: Response to the most recent operation request
+             *                     0: Success
+             *                     0x00000001..0x00000FFF: Corresponding TPM
+             *                                             error code
+             *                     0xFFFFFFF0: User Abort or timeout of dialog
+             *                     0xFFFFFFF1: firmware Failure
+             */
+            func_idx = aml_local(0);
+            ifctx2 = aml_if(aml_equal(func_idx, aml_int(5)));
+            {
+                /* TPM3[1] = LPPR */
+                aml_append(ifctx2,
+                           aml_store(
+                               aml_name("LPPR"),
+                               aml_index(aml_name("TPM3"), aml_int(1))));
+                /* TPM3[2] = PPRP */
+                aml_append(ifctx2,
+                           aml_store(
+                               aml_name("PPRP"),
+                               aml_index(aml_name("TPM3"), aml_int(2))));
+                aml_append(ifctx2, aml_return(aml_name("TPM3")));
+            }
+            aml_append(ifctx, ifctx2);
+
+            /*
+             * 8.1.6 Submit preferred user language
+             *
+             * Arg 2 (Integer): Function Index = 6
+             * Arg 3 (Package): Arguments = String Package
+             *                  Preferred language code
+             * Returns: Type: Integer
+             * Function Return Code
+             *          3: Not implemented
+             */
+            func_idx = aml_local(0);
+            ifctx2 = aml_if(aml_equal(func_idx, aml_int(6)));
+            {
+                /* 3 = not implemented */
+                aml_append(ifctx2, aml_return(aml_int(3)));
+            }
+            aml_append(ifctx, ifctx2);
+
+            /*
+             * 8.1.7 Submit TPM Operation Request to Pre-OS Environment 2
+             *
+             * Arg 2 (Integer): Function Index = 7
+             * Arg 3 (Package): Arguments = Package: Type: Integer
+             *                  Integer 1: Operation Value of the Request
+             *                  Integer 2: Argument for Operation (optional)
+             * Returns: Type: Integer
+             *          0: Success
+             *          1: Not Implemented
+             *          2: General Failure
+             *          3: Operation blocked by current firmware settings
+             */
+            func_idx = aml_local(0);
+            ifctx2 = aml_if(aml_equal(func_idx, aml_int(7)));
+            {
+                /* get opcode */
+                op = aml_derefof(aml_index(aml_arg(3), aml_int(0)));
+                aml_append(ifctx2, aml_store(op, aml_local(0)));
+
+                /* get opcode flags */
+                op = aml_local(0);
+                aml_append(ifctx2, aml_store(aml_call1("TPFN", op),
+                                             aml_local(1)));
+                /* if func[opcode] & TPM_PPI_FUNC_NOT_IMPLEMENTED */
+                op_flags = aml_local(1);
+                ifctx3 = aml_if(
+                    aml_equal(
+                        aml_and(op_flags, aml_int(TPM_PPI_FUNC_MASK), NULL),
+                        aml_int(TPM_PPI_FUNC_NOT_IMPLEMENTED)));
+                {
+                    /* 1: not implemented */
+                    aml_append(ifctx3, aml_return(aml_int(1)));
+                }
+                aml_append(ifctx2, ifctx3);
+
+                /* if func[opcode] & TPM_PPI_FUNC_BLOCKED */
+                op_flags = aml_local(1);
+                ifctx3 = aml_if(
+                    aml_equal(
+                        aml_and(op_flags, aml_int(TPM_PPI_FUNC_MASK), NULL),
+                        aml_int(TPM_PPI_FUNC_BLOCKED)));
+                {
+                    /* 3: blocked by firmware */
+                    aml_append(ifctx3, aml_return(aml_int(3)));
+                }
+                aml_append(ifctx2, ifctx3);
+
+                /* revision to integer */
+                aml_append(ifctx2,
+                           aml_store(
+                               aml_to_integer(aml_arg(1)),
+                               aml_local(1)));
+
+                rev = aml_local(1);
+                ifctx3 = aml_if(aml_equal(rev, aml_int(1)));
+                {
+                    /* revision 1 */
+                    /* PPRQ = op */
+                    op = aml_local(0);
+                    aml_append(ifctx3, aml_store(op, aml_name("PPRQ")));
+                    /* no argument, PPRM = 0 */
+                    aml_append(ifctx3, aml_store(aml_int(0),
+                                                 aml_name("PPRM")));
+                }
+                aml_append(ifctx2, ifctx3);
+
+                rev = aml_local(1);
+                ifctx3 = aml_if(aml_equal(rev, aml_int(2)));
+                {
+                    /* revision 2 */
+                    /* PPRQ = op */
+                    op = aml_local(0);
+                    op_arg = aml_derefof(aml_index(aml_arg(3), aml_int(1)));
+                    aml_append(ifctx3, aml_store(op, aml_name("PPRQ")));
+                    /* PPRM = arg3[1] */
+                    aml_append(ifctx3, aml_store(op_arg, aml_name("PPRM")));
+                }
+                aml_append(ifctx2, ifctx3);
+                /* 0: success */
+                aml_append(ifctx2, aml_return(aml_int(0)));
+            }
+            aml_append(ifctx, ifctx2);
+
+            /*
+             * 8.1.8 Get User Confirmation Status for Operation
+             *
+             * Arg 2 (Integer): Function Index = 8
+             * Arg 3 (Package): Arguments = Package: Type: Integer
+             *                  Operation Value that may need user confirmation
+             * Returns: Type: Integer
+             *          0: Not implemented
+             *          1: Firmware only
+             *          2: Blocked for OS by firmware configuration
+             *          3: Allowed and physically present user required
+             *          4: Allowed and physically present user not required
+             */
+            func_idx = aml_local(0);
+            ifctx2 = aml_if(aml_equal(func_idx, aml_int(8)));
+            {
+                /* get opcode */
+                op = aml_derefof(aml_index(aml_arg(3), aml_int(0)));
+                aml_append(ifctx2, aml_store(op, aml_local(0)));
+
+                /* get opcode flags */
+                op = aml_local(0);
+                aml_append(ifctx2, aml_store(aml_call1("TPFN", op),
+                                             aml_local(1)));
+                op_flags = aml_local(1);
+                /* return confirmation status code */
+                aml_append(ifctx2,
+                           aml_return(
+                               aml_and(op_flags,
+                                       aml_int(TPM_PPI_FUNC_MASK), NULL)));
+            }
+            aml_append(ifctx, ifctx2);
+
+            aml_append(ifctx, aml_return(aml_buffer(1, zerobyte)));
+        }
+        aml_append(method, ifctx);
+    }
+    aml_append(dev, method);
+}
+
 static void
 build_dsdt(GArray *table_data, BIOSLinker *linker,
            AcpiPmInfo *pm, AcpiMiscInfo *misc,
@@ -2153,6 +2569,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(dev);
+
                 aml_append(scope, dev);
             }
 
@@ -2172,6 +2591,8 @@ build_dsdt(GArray *table_data, BIOSLinker *linker,
         aml_append(method, aml_return(aml_int(0x0f)));
         aml_append(dev, method);
 
+        build_tpm_ppi(dev);
+
         aml_append(sb_scope, dev);
     }
 
@@ -2920,7 +3341,7 @@ void acpi_setup(void)
         tpm_config = (FWCfgTPMConfig) {
             .tpmppi_address = cpu_to_le32(TPM_PPI_ADDR_BASE),
             .tpm_version = cpu_to_le32(tpm_get_version(tpm_find())),
-            .tpmppi_version = cpu_to_le32(TPM_PPI_VERSION_NONE)
+            .tpmppi_version = cpu_to_le32(TPM_PPI_VERSION_1_30)
         };
         fw_cfg_add_file(pcms->fw_cfg, "etc/tpm/config",
                         &tpm_config, sizeof tpm_config);
diff --git a/docs/specs/tpm.txt b/docs/specs/tpm.txt
index 2ddb768084..c27762c723 100644
--- a/docs/specs/tpm.txt
+++ b/docs/specs/tpm.txt
@@ -62,6 +62,85 @@ URL:
 
 https://trustedcomputinggroup.org/tcg-acpi-specification/
 
+== ACPI PPI Interface ==
+
+QEMU supports the Physical Presence Interface (PPI) for TPM 1.2 and TPM 2. This
+interface requires ACPI and firmware support. The specification can be found at
+the following URL:
+
+https://trustedcomputinggroup.org/resource/tcg-physical-presence-interface-specification/
+
+PPI enables a system administrator (root) to request a modification to the
+TPM upon reboot. The PPI specification defines the operation requests and the
+actions the firmware has to take. The system administrator passes the operation
+request number to the firmware through an ACPI interface which writes this
+number to a memory location that the firmware knows. Upon reboot, the firmware
+finds the number and sends commands to the the TPM. The firmware writes the TPM
+result code and the operation request number to a memory location that ACPI can
+read from and pass the result on to the administrator.
+
+The PPI specification defines a set of mandatory and optional operations for
+the firmware to implement. The ACPI interface also allows an administrator to
+list the supported operations. In QEMU the ACPI code is generated by QEMU, yet
+the firmware needs to implement support on a per-operations basis, and
+different firmwares may support a different subset. Therefore, QEMU introduces
+the virtual memory device for PPI where the firmware can indicate which
+operations it supports and ACPI can enable the ones that are supported and
+disable all others. This interface lies in main memory and has the following
+layout:
+
+ +----------+--------+--------+-------------------------------------------+
+ |  Field   | Length | Offset | Description                               |
+ +----------+--------+--------+-------------------------------------------+
+ | func     |  0x100 |  0x000 | Firmware sets values for each supported   |
+ |          |        |        | operation. See defined values below.      |
+ +----------+--------+--------+-------------------------------------------+
+ | ppin     |   0x1  |  0x100 | SMI interrupt to use. Set by firmware.    |
+ |          |        |        | Not supported.                            |
+ +----------+--------+--------+-------------------------------------------+
+ | ppip     |   0x4  |  0x101 | ACPI function index to pass to SMM code.  |
+ |          |        |        | Set by ACPI. Not supported.               |
+ +----------+--------+--------+-------------------------------------------+
+ | pprp     |   0x4  |  0x105 | Result of last executed operation. Set by |
+ |          |        |        | firmware. See function index 5 for values.|
+ +----------+--------+--------+-------------------------------------------+
+ | pprq     |   0x4  |  0x109 | Operation request number to execute. See  |
+ |          |        |        | 'Physical Presence Interface Operation    |
+ |          |        |        | Summary' tables in specs. Set by ACPI.    |
+ +----------+--------+--------+-------------------------------------------+
+ | pprm     |   0x4  |  0x10d | Operation request optional parameter.     |
+ |          |        |        | Values depend on operation. Set by ACPI.  |
+ +----------+--------+--------+-------------------------------------------+
+ | lppr     |   0x4  |  0x111 | Last executed operation request number.   |
+ |          |        |        | Copied from pprq field by firmware.       |
+ +----------+--------+--------+-------------------------------------------+
+ | fret     |   0x4  |  0x115 | Result code from SMM function.            |
+ |          |        |        | Not supported.                            |
+ +----------+--------+--------+-------------------------------------------+
+ | res1     |  0x40  |  0x119 | Reserved for future use                   |
+ +----------+--------+--------+-------------------------------------------+
+ | next_step|   0x1  |  0x159 | Operation to execute after reboot by      |
+ |          |        |        | firmware. Used by firmware.               |
+ +----------+--------+--------+-------------------------------------------+
+
+   The following values are supported for the 'func' field. They correspond
+   to the values used by ACPI function index 8.
+
+ +----------+-------------------------------------------------------------+
+ | value    | Description                                                 |
+ +----------+-------------------------------------------------------------+
+ | 0        | Operation is not implemented.                               |
+ +----------+-------------------------------------------------------------+
+ | 1        | Operation is only accessible through firmware.              |
+ +----------+-------------------------------------------------------------+
+ | 2        | Operation is blocked for OS by firmware configuration.      |
+ +----------+-------------------------------------------------------------+
+ | 3        | Operation is allowed and physically present user required.  |
+ +----------+-------------------------------------------------------------+
+ | 4        | Operation is allowed and physically present user is not     |
+ |          | required.                                                   |
+ +----------+-------------------------------------------------------------+
+
 
 QEMU files related to TPM ACPI tables:
  - hw/i386/acpi-build.c
-- 
2.18.0.rc1

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

[Michael S. Tsirkin]

On Tue, Jun 26, 2018 at 02:23:43PM +0200, Marc-André Lureau wrote:
> From: Stefan Berger <stefanb@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 <stefanb@linux.vnet.ibm.com>
> 
> ---
> 
> 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)) {
> +        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_append(dev, name);
> +
> +    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")));
> +        {
> +            func_idx = aml_local(0);
> +            aml_append(ifctx,
> +                       aml_store(aml_to_integer(aml_arg(2)), func_idx));
> +
> +            /* standard DSM query function */
> +            func_idx = aml_local(0);
> +            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

For this and other instances please include the
1st version of spec that includes the specific interface,
and refer to chapter #s from that spec.
E.g. I see this in version 1 of spec but it's a
different chapter there.

Listing earliest version is important as it gives you
an idea which guests will support it.


> +             *
> +             * Arg 2 (Integer): Function Index = 1
> +             * Arg 3 (Package): Arguments = Empty Package
> +             * Returns: Type: String
> +             */
> +            func_idx = aml_local(0);
> +            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);
> +            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)));
> +
> +                /* get opcode flags */
> +                op = aml_local(0);
> +                aml_append(ifctx2,
> +                           aml_store(aml_call1("TPFN", op), aml_local(1)));
> +
> +                op_flags = aml_local(1);
> +                /* if func[opcode] & TPM_PPI_FUNC_NOT_IMPLEMENTED */
> +                ifctx3 = aml_if(
> +                    aml_equal(
> +                        aml_and(op_flags, aml_int(TPM_PPI_FUNC_MASK), NULL),
> +                        aml_int(TPM_PPI_FUNC_NOT_IMPLEMENTED)));
> +                {
> +                    /* 1: not implemented */
> +                    aml_append(ifctx3, aml_return(aml_int(1)));
> +                }
> +                aml_append(ifctx2, ifctx3);
> +
> +                op = aml_local(0);
> +                aml_append(ifctx2, aml_store(op, aml_name("PPRQ")));
> +                aml_append(ifctx2, aml_store(aml_int(0), aml_name("PPRM")));
> +                /* 0: success */
> +                aml_append(ifctx2, aml_return(aml_int(0)));
> +            }
> +            aml_append(ifctx, ifctx2);
> +
> +            /*
> +             * 8.1.3 Get Pending TPM Operation Requested By the OS
> +             *
> +             * Arg 2 (Integer): Function Index = 3
> +             * Arg 3 (Package): Arguments = Empty Package
> +             * Returns: Type: Package of Integers
> +             *          Integer 1: Function Return code
> +             *                     0: Success
> +             *                     1: General Failure
> +             *          Integer 2: Pending operation requested by the OS
> +             *                     0: None
> +             *                    >0: Operation Value of the Pending Request
> +             *          Integer 3: Optional argument to pending operation
> +             *                     requested by the OS
> +             *                     0: None
> +             *                    >0: Argument Value of the Pending Request
> +             */
> +            func_idx = aml_local(0);
> +            ifctx2 = aml_if(aml_equal(func_idx, aml_int(3)));
> +            {
> +                /* revision to integer */
> +                aml_append(ifctx2,
> +                           aml_store(
> +                               aml_to_integer(aml_arg(1)),
> +                               aml_local(1)));
> +                /*
> +                 * Revision ID of 1, no integer parameter beyond
> +                 * parameter two are expected
> +                 */
> +                rev = aml_local(1);
> +                ifctx3 = aml_if(aml_equal(rev, aml_int(1)));
> +                {
> +                    /* TPM2[1] = PPRQ */
> +                    aml_append(ifctx3,
> +                               aml_store(
> +                                   aml_name("PPRQ"),
> +                                   aml_index(aml_name("TPM2"), aml_int(1))));
> +                    aml_append(ifctx3, aml_return(aml_name("TPM2")));
> +                }
> +                aml_append(ifctx2, ifctx3);
> +
> +                /*
> +                 * A return value of {0, 23, 1} indicates that
> +                 * operation 23 with argument 1 is pending.
> +                 */
> +                rev = aml_local(1);
> +                ifctx3 = aml_if(aml_equal(rev, aml_int(2)));
> +                {
> +                    /* TPM3[1] = PPRQ */
> +                    aml_append(ifctx3,
> +                               aml_store(
> +                                   aml_name("PPRQ"),
> +                                   aml_index(aml_name("TPM3"), aml_int(1))));
> +                    /* TPM3[2] = PPRM */
> +                    aml_append(ifctx3,
> +                               aml_store(
> +                                   aml_name("PPRM"),
> +                                   aml_index(aml_name("TPM3"), aml_int(2))));
> +                    aml_append(ifctx3, aml_return(aml_name("TPM3")));
> +                }
> +                aml_append(ifctx2, ifctx3);
> +            }
> +            aml_append(ifctx, ifctx2);
> +
> +            /*
> +             * 8.1.4 Get Platform-Specific Action to Transition to
> +             *       Pre-OS Environment
> +             *
> +             * Arg 2 (Integer): Function Index = 4
> +             * Arg 3 (Package): Arguments = Empty Package
> +             * Returns: Type: Integer
> +             *          0: None
> +             *          1: Shutdown
> +             *          2: Reboot
> +             *          3: OS Vendor-specific
> +             */
> +            func_idx = aml_local(0);
> +            ifctx2 = aml_if(aml_equal(func_idx, aml_int(4)));
> +            {
> +                /* reboot */
> +                aml_append(ifctx2, aml_return(aml_int(2)));
> +            }
> +            aml_append(ifctx, ifctx2);
> +
> +            /*
> +             * 8.1.5 Return TPM Operation Response to OS Environment
> +             *
> +             * Arg 2 (Integer): Function Index = 5
> +             * Arg 3 (Package): Arguments = Empty Package
> +             * Returns: Type: Package of Integer
> +             *          Integer 1: Function Return code
> +             *                     0: Success
> +             *                     1: General Failure
> +             *          Integer 2: Most recent operation request
> +             *                     0: None
> +             *                    >0: Operation Value of the most recent request
> +             *          Integer 3: Response to the most recent operation request
> +             *                     0: Success
> +             *                     0x00000001..0x00000FFF: Corresponding TPM
> +             *                                             error code
> +             *                     0xFFFFFFF0: User Abort or timeout of dialog
> +             *                     0xFFFFFFF1: firmware Failure
> +             */
> +            func_idx = aml_local(0);
> +            ifctx2 = aml_if(aml_equal(func_idx, aml_int(5)));
> +            {
> +                /* TPM3[1] = LPPR */
> +                aml_append(ifctx2,
> +                           aml_store(
> +                               aml_name("LPPR"),
> +                               aml_index(aml_name("TPM3"), aml_int(1))));
> +                /* TPM3[2] = PPRP */
> +                aml_append(ifctx2,
> +                           aml_store(
> +                               aml_name("PPRP"),
> +                               aml_index(aml_name("TPM3"), aml_int(2))));
> +                aml_append(ifctx2, aml_return(aml_name("TPM3")));
> +            }
> +            aml_append(ifctx, ifctx2);
> +
> +            /*
> +             * 8.1.6 Submit preferred user language
> +             *
> +             * Arg 2 (Integer): Function Index = 6
> +             * Arg 3 (Package): Arguments = String Package
> +             *                  Preferred language code
> +             * Returns: Type: Integer
> +             * Function Return Code
> +             *          3: Not implemented
> +             */
> +            func_idx = aml_local(0);
> +            ifctx2 = aml_if(aml_equal(func_idx, aml_int(6)));
> +            {
> +                /* 3 = not implemented */
> +                aml_append(ifctx2, aml_return(aml_int(3)));
> +            }
> +            aml_append(ifctx, ifctx2);
> +
> +            /*
> +             * 8.1.7 Submit TPM Operation Request to Pre-OS Environment 2
> +             *
> +             * Arg 2 (Integer): Function Index = 7
> +             * Arg 3 (Package): Arguments = Package: Type: Integer
> +             *                  Integer 1: Operation Value of the Request
> +             *                  Integer 2: Argument for Operation (optional)
> +             * Returns: Type: Integer
> +             *          0: Success
> +             *          1: Not Implemented
> +             *          2: General Failure
> +             *          3: Operation blocked by current firmware settings
> +             */
> +            func_idx = aml_local(0);
> +            ifctx2 = aml_if(aml_equal(func_idx, aml_int(7)));
> +            {
> +                /* get opcode */
> +                op = aml_derefof(aml_index(aml_arg(3), aml_int(0)));
> +                aml_append(ifctx2, aml_store(op, aml_local(0)));
> +
> +                /* get opcode flags */
> +                op = aml_local(0);
> +                aml_append(ifctx2, aml_store(aml_call1("TPFN", op),
> +                                             aml_local(1)));
> +                /* if func[opcode] & TPM_PPI_FUNC_NOT_IMPLEMENTED */
> +                op_flags = aml_local(1);
> +                ifctx3 = aml_if(
> +                    aml_equal(
> +                        aml_and(op_flags, aml_int(TPM_PPI_FUNC_MASK), NULL),
> +                        aml_int(TPM_PPI_FUNC_NOT_IMPLEMENTED)));
> +                {
> +                    /* 1: not implemented */
> +                    aml_append(ifctx3, aml_return(aml_int(1)));
> +                }
> +                aml_append(ifctx2, ifctx3);
> +
> +                /* if func[opcode] & TPM_PPI_FUNC_BLOCKED */
> +                op_flags = aml_local(1);
> +                ifctx3 = aml_if(
> +                    aml_equal(
> +                        aml_and(op_flags, aml_int(TPM_PPI_FUNC_MASK), NULL),
> +                        aml_int(TPM_PPI_FUNC_BLOCKED)));
> +                {
> +                    /* 3: blocked by firmware */
> +                    aml_append(ifctx3, aml_return(aml_int(3)));
> +                }
> +                aml_append(ifctx2, ifctx3);
> +
> +                /* revision to integer */
> +                aml_append(ifctx2,
> +                           aml_store(
> +                               aml_to_integer(aml_arg(1)),
> +                               aml_local(1)));
> +
> +                rev = aml_local(1);
> +                ifctx3 = aml_if(aml_equal(rev, aml_int(1)));
> +                {
> +                    /* revision 1 */
> +                    /* PPRQ = op */
> +                    op = aml_local(0);
> +                    aml_append(ifctx3, aml_store(op, aml_name("PPRQ")));
> +                    /* no argument, PPRM = 0 */
> +                    aml_append(ifctx3, aml_store(aml_int(0),
> +                                                 aml_name("PPRM")));
> +                }
> +                aml_append(ifctx2, ifctx3);
> +
> +                rev = aml_local(1);
> +                ifctx3 = aml_if(aml_equal(rev, aml_int(2)));
> +                {
> +                    /* revision 2 */
> +                    /* PPRQ = op */
> +                    op = aml_local(0);
> +                    op_arg = aml_derefof(aml_index(aml_arg(3), aml_int(1)));
> +                    aml_append(ifctx3, aml_store(op, aml_name("PPRQ")));
> +                    /* PPRM = arg3[1] */
> +                    aml_append(ifctx3, aml_store(op_arg, aml_name("PPRM")));
> +                }
> +                aml_append(ifctx2, ifctx3);
> +                /* 0: success */
> +                aml_append(ifctx2, aml_return(aml_int(0)));
> +            }
> +            aml_append(ifctx, ifctx2);
> +
> +            /*
> +             * 8.1.8 Get User Confirmation Status for Operation
> +             *
> +             * Arg 2 (Integer): Function Index = 8
> +             * Arg 3 (Package): Arguments = Package: Type: Integer
> +             *                  Operation Value that may need user confirmation
> +             * Returns: Type: Integer
> +             *          0: Not implemented
> +             *          1: Firmware only
> +             *          2: Blocked for OS by firmware configuration
> +             *          3: Allowed and physically present user required
> +             *          4: Allowed and physically present user not required
> +             */
> +            func_idx = aml_local(0);
> +            ifctx2 = aml_if(aml_equal(func_idx, aml_int(8)));
> +            {
> +                /* get opcode */
> +                op = aml_derefof(aml_index(aml_arg(3), aml_int(0)));
> +                aml_append(ifctx2, aml_store(op, aml_local(0)));
> +
> +                /* get opcode flags */
> +                op = aml_local(0);
> +                aml_append(ifctx2, aml_store(aml_call1("TPFN", op),
> +                                             aml_local(1)));
> +                op_flags = aml_local(1);
> +                /* return confirmation status code */
> +                aml_append(ifctx2,
> +                           aml_return(
> +                               aml_and(op_flags,
> +                                       aml_int(TPM_PPI_FUNC_MASK), NULL)));
> +            }
> +            aml_append(ifctx, ifctx2);
> +
> +            aml_append(ifctx, aml_return(aml_buffer(1, zerobyte)));
> +        }
> +        aml_append(method, ifctx);
> +    }
> +    aml_append(dev, method);
> +}
> +
>  static void
>  build_dsdt(GArray *table_data, BIOSLinker *linker,
>             AcpiPmInfo *pm, AcpiMiscInfo *misc,
> @@ -2153,6 +2569,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(dev);
> +
>                  aml_append(scope, dev);
>              }
>  
> @@ -2172,6 +2591,8 @@ build_dsdt(GArray *table_data, BIOSLinker *linker,
>          aml_append(method, aml_return(aml_int(0x0f)));
>          aml_append(dev, method);
>  
> +        build_tpm_ppi(dev);
> +
>          aml_append(sb_scope, dev);
>      }
>  
> @@ -2920,7 +3341,7 @@ void acpi_setup(void)
>          tpm_config = (FWCfgTPMConfig) {
>              .tpmppi_address = cpu_to_le32(TPM_PPI_ADDR_BASE),
>              .tpm_version = cpu_to_le32(tpm_get_version(tpm_find())),
> -            .tpmppi_version = cpu_to_le32(TPM_PPI_VERSION_NONE)
> +            .tpmppi_version = cpu_to_le32(TPM_PPI_VERSION_1_30)
>          };
>          fw_cfg_add_file(pcms->fw_cfg, "etc/tpm/config",
>                          &tpm_config, sizeof tpm_config);
> diff --git a/docs/specs/tpm.txt b/docs/specs/tpm.txt
> index 2ddb768084..c27762c723 100644
> --- a/docs/specs/tpm.txt
> +++ b/docs/specs/tpm.txt
> @@ -62,6 +62,85 @@ URL:
>  
>  https://trustedcomputinggroup.org/tcg-acpi-specification/
>  
> +== ACPI PPI Interface ==
> +
> +QEMU supports the Physical Presence Interface (PPI) for TPM 1.2 and TPM 2. This
> +interface requires ACPI and firmware support. The specification can be found at
> +the following URL:
> +
> +https://trustedcomputinggroup.org/resource/tcg-physical-presence-interface-specification/
> +
> +PPI enables a system administrator (root) to request a modification to the
> +TPM upon reboot. The PPI specification defines the operation requests and the
> +actions the firmware has to take. The system administrator passes the operation
> +request number to the firmware through an ACPI interface which writes this
> +number to a memory location that the firmware knows. Upon reboot, the firmware
> +finds the number and sends commands to the the TPM. The firmware writes the TPM
> +result code and the operation request number to a memory location that ACPI can
> +read from and pass the result on to the administrator.
> +
> +The PPI specification defines a set of mandatory and optional operations for
> +the firmware to implement. The ACPI interface also allows an administrator to
> +list the supported operations. In QEMU the ACPI code is generated by QEMU, yet
> +the firmware needs to implement support on a per-operations basis, and
> +different firmwares may support a different subset. Therefore, QEMU introduces
> +the virtual memory device for PPI where the firmware can indicate which
> +operations it supports and ACPI can enable the ones that are supported and
> +disable all others. This interface lies in main memory and has the following
> +layout:
> +
> + +----------+--------+--------+-------------------------------------------+
> + |  Field   | Length | Offset | Description                               |
> + +----------+--------+--------+-------------------------------------------+
> + | func     |  0x100 |  0x000 | Firmware sets values for each supported   |
> + |          |        |        | operation. See defined values below.      |
> + +----------+--------+--------+-------------------------------------------+
> + | ppin     |   0x1  |  0x100 | SMI interrupt to use. Set by firmware.    |
> + |          |        |        | Not supported.                            |
> + +----------+--------+--------+-------------------------------------------+
> + | ppip     |   0x4  |  0x101 | ACPI function index to pass to SMM code.  |
> + |          |        |        | Set by ACPI. Not supported.               |
> + +----------+--------+--------+-------------------------------------------+
> + | pprp     |   0x4  |  0x105 | Result of last executed operation. Set by |
> + |          |        |        | firmware. See function index 5 for values.|
> + +----------+--------+--------+-------------------------------------------+
> + | pprq     |   0x4  |  0x109 | Operation request number to execute. See  |
> + |          |        |        | 'Physical Presence Interface Operation    |
> + |          |        |        | Summary' tables in specs. Set by ACPI.    |
> + +----------+--------+--------+-------------------------------------------+
> + | pprm     |   0x4  |  0x10d | Operation request optional parameter.     |
> + |          |        |        | Values depend on operation. Set by ACPI.  |
> + +----------+--------+--------+-------------------------------------------+
> + | lppr     |   0x4  |  0x111 | Last executed operation request number.   |
> + |          |        |        | Copied from pprq field by firmware.       |
> + +----------+--------+--------+-------------------------------------------+
> + | fret     |   0x4  |  0x115 | Result code from SMM function.            |
> + |          |        |        | Not supported.                            |
> + +----------+--------+--------+-------------------------------------------+
> + | res1     |  0x40  |  0x119 | Reserved for future use                   |
> + +----------+--------+--------+-------------------------------------------+
> + | next_step|   0x1  |  0x159 | Operation to execute after reboot by      |
> + |          |        |        | firmware. Used by firmware.               |
> + +----------+--------+--------+-------------------------------------------+
> +
> +   The following values are supported for the 'func' field. They correspond
> +   to the values used by ACPI function index 8.
> +
> + +----------+-------------------------------------------------------------+
> + | value    | Description                                                 |
> + +----------+-------------------------------------------------------------+
> + | 0        | Operation is not implemented.                               |
> + +----------+-------------------------------------------------------------+
> + | 1        | Operation is only accessible through firmware.              |
> + +----------+-------------------------------------------------------------+
> + | 2        | Operation is blocked for OS by firmware configuration.      |
> + +----------+-------------------------------------------------------------+
> + | 3        | Operation is allowed and physically present user required.  |
> + +----------+-------------------------------------------------------------+
> + | 4        | Operation is allowed and physically present user is not     |
> + |          | required.                                                   |
> + +----------+-------------------------------------------------------------+
> +
>  
>  QEMU files related to TPM ACPI tables:
>   - hw/i386/acpi-build.c
> -- 
> 2.18.0.rc1

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

[Igor Mammedov]

On Tue, 26 Jun 2018 14:23:43 +0200
Marc-André Lureau <marcandre.lureau@redhat.com> wrote:

> From: Stefan Berger <stefanb@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.

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 <stefanb@linux.vnet.ibm.com>
> 
> ---
> 
> 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.

> +        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

> +    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"))

> +        {
> +            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

you can use the same naming approach for other arguments i.e.
arguments = aml_arg(3)

> +
> +            /* standard DSM query function */
> +            func_idx = aml_local(0);
I don't think you need to init it twice.

> +            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 */

> +             *
> +             * 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)

> +                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()


> +                /* if func[opcode] & TPM_PPI_FUNC_NOT_IMPLEMENTED */
> +                ifctx3 = aml_if(
> +                    aml_equal(
> +                        aml_and(op_flags, aml_int(TPM_PPI_FUNC_MASK), NULL),
> +                        aml_int(TPM_PPI_FUNC_NOT_IMPLEMENTED)));
> +                {
> +                    /* 1: not implemented */
may be the same language as in above comment
  /* 1: Operation Value of the Request Not Supported */
same applies to other return values 

> +                    aml_append(ifctx3, aml_return(aml_int(1)));
> +                }
> +                aml_append(ifctx2, ifctx3);
> +
> +                op = aml_local(0);
> +                aml_append(ifctx2, aml_store(op, aml_name("PPRQ")));
> +                aml_append(ifctx2, aml_store(aml_int(0), aml_name("PPRM")));
> +                /* 0: success */
> +                aml_append(ifctx2, aml_return(aml_int(0)));
> +            }
> +            aml_append(ifctx, ifctx2);
> +
> +            /*
> +             * 8.1.3 Get Pending TPM Operation Requested By the OS
> +             *
> +             * Arg 2 (Integer): Function Index = 3
> +             * Arg 3 (Package): Arguments = Empty Package
> +             * Returns: Type: Package of Integers
> +             *          Integer 1: Function Return code
> +             *                     0: Success
> +             *                     1: General Failure
> +             *          Integer 2: Pending operation requested by the OS
> +             *                     0: None
> +             *                    >0: Operation Value of the Pending Request
> +             *          Integer 3: Optional argument to pending operation
> +             *                     requested by the OS
> +             *                     0: None
> +             *                    >0: Argument Value of the Pending Request
> +             */
> +            func_idx = aml_local(0);
> +            ifctx2 = aml_if(aml_equal(func_idx, aml_int(3)));
> +            {
> +                /* revision to integer */
> +                aml_append(ifctx2,
> +                           aml_store(
> +                               aml_to_integer(aml_arg(1)),
same do you need cast here, by spec it's 'int' already?

> +                               aml_local(1)));
why store into local var, couldn't it be used directly ?

> +                /*
> +                 * Revision ID of 1, no integer parameter beyond
> +                 * parameter two are expected
> +                 */
> +                rev = aml_local(1);
> +                ifctx3 = aml_if(aml_equal(rev, aml_int(1)));
> +                {
> +                    /* TPM2[1] = PPRQ */
> +                    aml_append(ifctx3,
> +                               aml_store(
> +                                   aml_name("PPRQ"),
> +                                   aml_index(aml_name("TPM2"), aml_int(1))));
> +                    aml_append(ifctx3, aml_return(aml_name("TPM2")));
> +                }
> +                aml_append(ifctx2, ifctx3);
> +
> +                /*
> +                 * A return value of {0, 23, 1} indicates that
> +                 * operation 23 with argument 1 is pending.
> +                 */
> +                rev = aml_local(1);
> +                ifctx3 = aml_if(aml_equal(rev, aml_int(2)));
> +                {
> +                    /* TPM3[1] = PPRQ */
> +                    aml_append(ifctx3,
> +                               aml_store(
> +                                   aml_name("PPRQ"),
> +                                   aml_index(aml_name("TPM3"), aml_int(1))));
> +                    /* TPM3[2] = PPRM */
> +                    aml_append(ifctx3,
> +                               aml_store(
> +                                   aml_name("PPRM"),
> +                                   aml_index(aml_name("TPM3"), aml_int(2))));
> +                    aml_append(ifctx3, aml_return(aml_name("TPM3")));
> +                }
> +                aml_append(ifctx2, ifctx3);
> +            }
> +            aml_append(ifctx, ifctx2);
> +
> +            /*
> +             * 8.1.4 Get Platform-Specific Action to Transition to
> +             *       Pre-OS Environment
> +             *
> +             * Arg 2 (Integer): Function Index = 4
> +             * Arg 3 (Package): Arguments = Empty Package
> +             * Returns: Type: Integer
> +             *          0: None
> +             *          1: Shutdown
> +             *          2: Reboot
> +             *          3: OS Vendor-specific
> +             */
> +            func_idx = aml_local(0);
> +            ifctx2 = aml_if(aml_equal(func_idx, aml_int(4)));
> +            {
> +                /* reboot */
> +                aml_append(ifctx2, aml_return(aml_int(2)));
> +            }
> +            aml_append(ifctx, ifctx2);
> +
> +            /*
> +             * 8.1.5 Return TPM Operation Response to OS Environment
> +             *
> +             * Arg 2 (Integer): Function Index = 5
> +             * Arg 3 (Package): Arguments = Empty Package
> +             * Returns: Type: Package of Integer
> +             *          Integer 1: Function Return code
> +             *                     0: Success
> +             *                     1: General Failure
> +             *          Integer 2: Most recent operation request
> +             *                     0: None
> +             *                    >0: Operation Value of the most recent request
> +             *          Integer 3: Response to the most recent operation request
> +             *                     0: Success
> +             *                     0x00000001..0x00000FFF: Corresponding TPM
> +             *                                             error code
> +             *                     0xFFFFFFF0: User Abort or timeout of dialog
> +             *                     0xFFFFFFF1: firmware Failure
> +             */
> +            func_idx = aml_local(0);
> +            ifctx2 = aml_if(aml_equal(func_idx, aml_int(5)));
> +            {
> +                /* TPM3[1] = LPPR */
> +                aml_append(ifctx2,
> +                           aml_store(
> +                               aml_name("LPPR"),
> +                               aml_index(aml_name("TPM3"), aml_int(1))));
> +                /* TPM3[2] = PPRP */
> +                aml_append(ifctx2,
> +                           aml_store(
> +                               aml_name("PPRP"),
> +                               aml_index(aml_name("TPM3"), aml_int(2))));
> +                aml_append(ifctx2, aml_return(aml_name("TPM3")));
> +            }
> +            aml_append(ifctx, ifctx2);
> +
> +            /*
> +             * 8.1.6 Submit preferred user language
> +             *
> +             * Arg 2 (Integer): Function Index = 6
> +             * Arg 3 (Package): Arguments = String Package
> +             *                  Preferred language code
> +             * Returns: Type: Integer
> +             * Function Return Code
> +             *          3: Not implemented
> +             */
> +            func_idx = aml_local(0);
> +            ifctx2 = aml_if(aml_equal(func_idx, aml_int(6)));
> +            {
> +                /* 3 = not implemented */
> +                aml_append(ifctx2, aml_return(aml_int(3)));
> +            }
> +            aml_append(ifctx, ifctx2);
> +
> +            /*
> +             * 8.1.7 Submit TPM Operation Request to Pre-OS Environment 2
> +             *
> +             * Arg 2 (Integer): Function Index = 7
> +             * Arg 3 (Package): Arguments = Package: Type: Integer
> +             *                  Integer 1: Operation Value of the Request
> +             *                  Integer 2: Argument for Operation (optional)
> +             * Returns: Type: Integer
> +             *          0: Success
> +             *          1: Not Implemented
> +             *          2: General Failure
> +             *          3: Operation blocked by current firmware settings
> +             */
> +            func_idx = aml_local(0);
> +            ifctx2 = aml_if(aml_equal(func_idx, aml_int(7)));
> +            {
> +                /* get opcode */
> +                op = aml_derefof(aml_index(aml_arg(3), aml_int(0)));
> +                aml_append(ifctx2, aml_store(op, aml_local(0)));
> +
> +                /* get opcode flags */
> +                op = aml_local(0);
> +                aml_append(ifctx2, aml_store(aml_call1("TPFN", op),
> +                                             aml_local(1)));
> +                /* if func[opcode] & TPM_PPI_FUNC_NOT_IMPLEMENTED */
> +                op_flags = aml_local(1);
> +                ifctx3 = aml_if(
> +                    aml_equal(
> +                        aml_and(op_flags, aml_int(TPM_PPI_FUNC_MASK), NULL),
> +                        aml_int(TPM_PPI_FUNC_NOT_IMPLEMENTED)));
> +                {
> +                    /* 1: not implemented */
> +                    aml_append(ifctx3, aml_return(aml_int(1)));
> +                }
> +                aml_append(ifctx2, ifctx3);
> +
> +                /* if func[opcode] & TPM_PPI_FUNC_BLOCKED */
> +                op_flags = aml_local(1);
> +                ifctx3 = aml_if(
> +                    aml_equal(
> +                        aml_and(op_flags, aml_int(TPM_PPI_FUNC_MASK), NULL),
> +                        aml_int(TPM_PPI_FUNC_BLOCKED)));
> +                {
> +                    /* 3: blocked by firmware */
> +                    aml_append(ifctx3, aml_return(aml_int(3)));
> +                }
> +                aml_append(ifctx2, ifctx3);
> +
> +                /* revision to integer */
> +                aml_append(ifctx2,
> +                           aml_store(
> +                               aml_to_integer(aml_arg(1)),
> +                               aml_local(1)));
> +
> +                rev = aml_local(1);
> +                ifctx3 = aml_if(aml_equal(rev, aml_int(1)));
> +                {
> +                    /* revision 1 */
> +                    /* PPRQ = op */
> +                    op = aml_local(0);
> +                    aml_append(ifctx3, aml_store(op, aml_name("PPRQ")));
> +                    /* no argument, PPRM = 0 */
> +                    aml_append(ifctx3, aml_store(aml_int(0),
> +                                                 aml_name("PPRM")));
> +                }
> +                aml_append(ifctx2, ifctx3);
> +
> +                rev = aml_local(1);
> +                ifctx3 = aml_if(aml_equal(rev, aml_int(2)));
> +                {
> +                    /* revision 2 */
> +                    /* PPRQ = op */
> +                    op = aml_local(0);
> +                    op_arg = aml_derefof(aml_index(aml_arg(3), aml_int(1)));
> +                    aml_append(ifctx3, aml_store(op, aml_name("PPRQ")));
> +                    /* PPRM = arg3[1] */
> +                    aml_append(ifctx3, aml_store(op_arg, aml_name("PPRM")));
> +                }
> +                aml_append(ifctx2, ifctx3);
> +                /* 0: success */
> +                aml_append(ifctx2, aml_return(aml_int(0)));
> +            }
> +            aml_append(ifctx, ifctx2);
> +
> +            /*
> +             * 8.1.8 Get User Confirmation Status for Operation
> +             *
> +             * Arg 2 (Integer): Function Index = 8
> +             * Arg 3 (Package): Arguments = Package: Type: Integer
> +             *                  Operation Value that may need user confirmation
> +             * Returns: Type: Integer
> +             *          0: Not implemented
> +             *          1: Firmware only
> +             *          2: Blocked for OS by firmware configuration
> +             *          3: Allowed and physically present user required
> +             *          4: Allowed and physically present user not required
> +             */
> +            func_idx = aml_local(0);
> +            ifctx2 = aml_if(aml_equal(func_idx, aml_int(8)));
> +            {
> +                /* get opcode */
> +                op = aml_derefof(aml_index(aml_arg(3), aml_int(0)));
> +                aml_append(ifctx2, aml_store(op, aml_local(0)));
> +
> +                /* get opcode flags */
> +                op = aml_local(0);
> +                aml_append(ifctx2, aml_store(aml_call1("TPFN", op),
> +                                             aml_local(1)));
> +                op_flags = aml_local(1);
> +                /* return confirmation status code */
> +                aml_append(ifctx2,
> +                           aml_return(
> +                               aml_and(op_flags,
> +                                       aml_int(TPM_PPI_FUNC_MASK), NULL)));
> +            }
> +            aml_append(ifctx, ifctx2);
> +
> +            aml_append(ifctx, aml_return(aml_buffer(1, zerobyte)));
> +        }
> +        aml_append(method, ifctx);
> +    }
> +    aml_append(dev, method);
> +}
> +
>  static void
>  build_dsdt(GArray *table_data, BIOSLinker *linker,
>             AcpiPmInfo *pm, AcpiMiscInfo *misc,
> @@ -2153,6 +2569,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(dev);
> +
>                  aml_append(scope, dev);
>              }
>  
> @@ -2172,6 +2591,8 @@ build_dsdt(GArray *table_data, BIOSLinker *linker,
>          aml_append(method, aml_return(aml_int(0x0f)));
>          aml_append(dev, method);
>  
> +        build_tpm_ppi(dev);
> +
>          aml_append(sb_scope, dev);
>      }
>  
> @@ -2920,7 +3341,7 @@ void acpi_setup(void)
>          tpm_config = (FWCfgTPMConfig) {
>              .tpmppi_address = cpu_to_le32(TPM_PPI_ADDR_BASE),
>              .tpm_version = cpu_to_le32(tpm_get_version(tpm_find())),
> -            .tpmppi_version = cpu_to_le32(TPM_PPI_VERSION_NONE)
> +            .tpmppi_version = cpu_to_le32(TPM_PPI_VERSION_1_30)
s/cpu_to_le32//

>          };
>          fw_cfg_add_file(pcms->fw_cfg, "etc/tpm/config",
>                          &tpm_config, sizeof tpm_config);
> diff --git a/docs/specs/tpm.txt b/docs/specs/tpm.txt
> index 2ddb768084..c27762c723 100644
> --- a/docs/specs/tpm.txt
> +++ b/docs/specs/tpm.txt
> @@ -62,6 +62,85 @@ URL:
>  
>  https://trustedcomputinggroup.org/tcg-acpi-specification/
>  
> +== ACPI PPI Interface ==
> +
> +QEMU supports the Physical Presence Interface (PPI) for TPM 1.2 and TPM 2. This
> +interface requires ACPI and firmware support. The specification can be found at
> +the following URL:
> +
> +https://trustedcomputinggroup.org/resource/tcg-physical-presence-interface-specification/
> +
> +PPI enables a system administrator (root) to request a modification to the
> +TPM upon reboot. The PPI specification defines the operation requests and the
> +actions the firmware has to take. The system administrator passes the operation
> +request number to the firmware through an ACPI interface which writes this
> +number to a memory location that the firmware knows. Upon reboot, the firmware
> +finds the number and sends commands to the the TPM. The firmware writes the TPM
> +result code and the operation request number to a memory location that ACPI can
> +read from and pass the result on to the administrator.
> +
> +The PPI specification defines a set of mandatory and optional operations for
> +the firmware to implement. The ACPI interface also allows an administrator to
> +list the supported operations. In QEMU the ACPI code is generated by QEMU, yet
> +the firmware needs to implement support on a per-operations basis, and
> +different firmwares may support a different subset. Therefore, QEMU introduces
> +the virtual memory device for PPI where the firmware can indicate which
> +operations it supports and ACPI can enable the ones that are supported and
> +disable all others. This interface lies in main memory and has the following
> +layout:
> +
> + +----------+--------+--------+-------------------------------------------+
> + |  Field   | Length | Offset | Description                               |
> + +----------+--------+--------+-------------------------------------------+
> + | func     |  0x100 |  0x000 | Firmware sets values for each supported   |
> + |          |        |        | operation. See defined values below.      |
> + +----------+--------+--------+-------------------------------------------+
> + | ppin     |   0x1  |  0x100 | SMI interrupt to use. Set by firmware.    |
> + |          |        |        | Not supported.                            |
> + +----------+--------+--------+-------------------------------------------+
> + | ppip     |   0x4  |  0x101 | ACPI function index to pass to SMM code.  |
> + |          |        |        | Set by ACPI. Not supported.               |
> + +----------+--------+--------+-------------------------------------------+
> + | pprp     |   0x4  |  0x105 | Result of last executed operation. Set by |
> + |          |        |        | firmware. See function index 5 for values.|
> + +----------+--------+--------+-------------------------------------------+
> + | pprq     |   0x4  |  0x109 | Operation request number to execute. See  |
> + |          |        |        | 'Physical Presence Interface Operation    |
> + |          |        |        | Summary' tables in specs. Set by ACPI.    |
> + +----------+--------+--------+-------------------------------------------+
> + | pprm     |   0x4  |  0x10d | Operation request optional parameter.     |
> + |          |        |        | Values depend on operation. Set by ACPI.  |
> + +----------+--------+--------+-------------------------------------------+
> + | lppr     |   0x4  |  0x111 | Last executed operation request number.   |
> + |          |        |        | Copied from pprq field by firmware.       |
> + +----------+--------+--------+-------------------------------------------+
> + | fret     |   0x4  |  0x115 | Result code from SMM function.            |
> + |          |        |        | Not supported.                            |
> + +----------+--------+--------+-------------------------------------------+
> + | res1     |  0x40  |  0x119 | Reserved for future use                   |
> + +----------+--------+--------+-------------------------------------------+
> + | next_step|   0x1  |  0x159 | Operation to execute after reboot by      |
> + |          |        |        | firmware. Used by firmware.               |
> + +----------+--------+--------+-------------------------------------------+
> +
> +   The following values are supported for the 'func' field. They correspond
> +   to the values used by ACPI function index 8.
> +
> + +----------+-------------------------------------------------------------+
> + | value    | Description                                                 |
> + +----------+-------------------------------------------------------------+
> + | 0        | Operation is not implemented.                               |
> + +----------+-------------------------------------------------------------+
> + | 1        | Operation is only accessible through firmware.              |
> + +----------+-------------------------------------------------------------+
> + | 2        | Operation is blocked for OS by firmware configuration.      |
> + +----------+-------------------------------------------------------------+
> + | 3        | Operation is allowed and physically present user required.  |
> + +----------+-------------------------------------------------------------+
> + | 4        | Operation is allowed and physically present user is not     |
> + |          | required.                                                   |
> + +----------+-------------------------------------------------------------+
> +
>  
>  QEMU files related to TPM ACPI tables:
>   - hw/i386/acpi-build.c

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 <marcandre.lureau@redhat.com> wrote:
>
>> From: Stefan Berger <stefanb@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.
> 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 <stefanb@linux.vnet.ibm.com>
>>
>> ---
>>
>> 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.

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

[Igor Mammedov]

On Wed, 27 Jun 2018 10:06:18 -0400
Stefan Berger <stefanb@linux.vnet.ibm.com> wrote:

> On 06/27/2018 09:19 AM, Igor Mammedov wrote:
> > On Tue, 26 Jun 2018 14:23:43 +0200
> > Marc-André Lureau <marcandre.lureau@redhat.com> wrote:
> >  
> >> From: Stefan Berger <stefanb@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.  
> > 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 <stefanb@linux.vnet.ibm.com>
> >>
> >> ---
> >>
> >> 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.
it happens not to be called now,
I'd suggest do look up once (QOM tree walk expensive)
and pass found value as an argument down call chain.

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

[Marc-André Lureau]

Hi

On Wed, Jun 27, 2018 at 3:19 PM, Igor Mammedov <imammedo@redhat.com> wrote:
> On Tue, 26 Jun 2018 14:23:43 +0200
> Marc-André Lureau <marcandre.lureau@redhat.com> wrote:
>
>> From: Stefan Berger <stefanb@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.
>
> 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 <stefanb@linux.vnet.ibm.com>
>>
>> ---
>>
>> 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_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.

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...

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.

>> +                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.

>
>> +                /* if func[opcode] & TPM_PPI_FUNC_NOT_IMPLEMENTED */
>> +                ifctx3 = aml_if(
>> +                    aml_equal(
>> +                        aml_and(op_flags, aml_int(TPM_PPI_FUNC_MASK), NULL),
>> +                        aml_int(TPM_PPI_FUNC_NOT_IMPLEMENTED)));
>> +                {
>> +                    /* 1: not implemented */
> may be the same language as in above comment
>   /* 1: Operation Value of the Request Not Supported */
> same applies to other return values

ok

>
>> +                    aml_append(ifctx3, aml_return(aml_int(1)));
>> +                }
>> +                aml_append(ifctx2, ifctx3);
>> +
>> +                op = aml_local(0);
>> +                aml_append(ifctx2, aml_store(op, aml_name("PPRQ")));
>> +                aml_append(ifctx2, aml_store(aml_int(0), aml_name("PPRM")));
>> +                /* 0: success */
>> +                aml_append(ifctx2, aml_return(aml_int(0)));
>> +            }
>> +            aml_append(ifctx, ifctx2);
>> +
>> +            /*
>> +             * 8.1.3 Get Pending TPM Operation Requested By the OS
>> +             *
>> +             * Arg 2 (Integer): Function Index = 3
>> +             * Arg 3 (Package): Arguments = Empty Package
>> +             * Returns: Type: Package of Integers
>> +             *          Integer 1: Function Return code
>> +             *                     0: Success
>> +             *                     1: General Failure
>> +             *          Integer 2: Pending operation requested by the OS
>> +             *                     0: None
>> +             *                    >0: Operation Value of the Pending Request
>> +             *          Integer 3: Optional argument to pending operation
>> +             *                     requested by the OS
>> +             *                     0: None
>> +             *                    >0: Argument Value of the Pending Request
>> +             */
>> +            func_idx = aml_local(0);
>> +            ifctx2 = aml_if(aml_equal(func_idx, aml_int(3)));
>> +            {
>> +                /* revision to integer */
>> +                aml_append(ifctx2,
>> +                           aml_store(
>> +                               aml_to_integer(aml_arg(1)),
> same do you need cast here, by spec it's 'int' already?
>

Yeah, looks like it's unnecessary. Stefan?

>> +                               aml_local(1)));
> why store into local var, couldn't it be used directly ?

removed

>
>> +                /*
>> +                 * Revision ID of 1, no integer parameter beyond
>> +                 * parameter two are expected
>> +                 */
>> +                rev = aml_local(1);
>> +                ifctx3 = aml_if(aml_equal(rev, aml_int(1)));
>> +                {
>> +                    /* TPM2[1] = PPRQ */
>> +                    aml_append(ifctx3,
>> +                               aml_store(
>> +                                   aml_name("PPRQ"),
>> +                                   aml_index(aml_name("TPM2"), aml_int(1))));
>> +                    aml_append(ifctx3, aml_return(aml_name("TPM2")));
>> +                }
>> +                aml_append(ifctx2, ifctx3);
>> +
>> +                /*
>> +                 * A return value of {0, 23, 1} indicates that
>> +                 * operation 23 with argument 1 is pending.
>> +                 */
>> +                rev = aml_local(1);
>> +                ifctx3 = aml_if(aml_equal(rev, aml_int(2)));
>> +                {
>> +                    /* TPM3[1] = PPRQ */
>> +                    aml_append(ifctx3,
>> +                               aml_store(
>> +                                   aml_name("PPRQ"),
>> +                                   aml_index(aml_name("TPM3"), aml_int(1))));
>> +                    /* TPM3[2] = PPRM */
>> +                    aml_append(ifctx3,
>> +                               aml_store(
>> +                                   aml_name("PPRM"),
>> +                                   aml_index(aml_name("TPM3"), aml_int(2))));
>> +                    aml_append(ifctx3, aml_return(aml_name("TPM3")));
>> +                }
>> +                aml_append(ifctx2, ifctx3);
>> +            }
>> +            aml_append(ifctx, ifctx2);
>> +
>> +            /*
>> +             * 8.1.4 Get Platform-Specific Action to Transition to
>> +             *       Pre-OS Environment
>> +             *
>> +             * Arg 2 (Integer): Function Index = 4
>> +             * Arg 3 (Package): Arguments = Empty Package
>> +             * Returns: Type: Integer
>> +             *          0: None
>> +             *          1: Shutdown
>> +             *          2: Reboot
>> +             *          3: OS Vendor-specific
>> +             */
>> +            func_idx = aml_local(0);
>> +            ifctx2 = aml_if(aml_equal(func_idx, aml_int(4)));
>> +            {
>> +                /* reboot */
>> +                aml_append(ifctx2, aml_return(aml_int(2)));
>> +            }
>> +            aml_append(ifctx, ifctx2);
>> +
>> +            /*
>> +             * 8.1.5 Return TPM Operation Response to OS Environment
>> +             *
>> +             * Arg 2 (Integer): Function Index = 5
>> +             * Arg 3 (Package): Arguments = Empty Package
>> +             * Returns: Type: Package of Integer
>> +             *          Integer 1: Function Return code
>> +             *                     0: Success
>> +             *                     1: General Failure
>> +             *          Integer 2: Most recent operation request
>> +             *                     0: None
>> +             *                    >0: Operation Value of the most recent request
>> +             *          Integer 3: Response to the most recent operation request
>> +             *                     0: Success
>> +             *                     0x00000001..0x00000FFF: Corresponding TPM
>> +             *                                             error code
>> +             *                     0xFFFFFFF0: User Abort or timeout of dialog
>> +             *                     0xFFFFFFF1: firmware Failure
>> +             */
>> +            func_idx = aml_local(0);
>> +            ifctx2 = aml_if(aml_equal(func_idx, aml_int(5)));
>> +            {
>> +                /* TPM3[1] = LPPR */
>> +                aml_append(ifctx2,
>> +                           aml_store(
>> +                               aml_name("LPPR"),
>> +                               aml_index(aml_name("TPM3"), aml_int(1))));
>> +                /* TPM3[2] = PPRP */
>> +                aml_append(ifctx2,
>> +                           aml_store(
>> +                               aml_name("PPRP"),
>> +                               aml_index(aml_name("TPM3"), aml_int(2))));
>> +                aml_append(ifctx2, aml_return(aml_name("TPM3")));
>> +            }
>> +            aml_append(ifctx, ifctx2);
>> +
>> +            /*
>> +             * 8.1.6 Submit preferred user language
>> +             *
>> +             * Arg 2 (Integer): Function Index = 6
>> +             * Arg 3 (Package): Arguments = String Package
>> +             *                  Preferred language code
>> +             * Returns: Type: Integer
>> +             * Function Return Code
>> +             *          3: Not implemented
>> +             */
>> +            func_idx = aml_local(0);
>> +            ifctx2 = aml_if(aml_equal(func_idx, aml_int(6)));
>> +            {
>> +                /* 3 = not implemented */
>> +                aml_append(ifctx2, aml_return(aml_int(3)));
>> +            }
>> +            aml_append(ifctx, ifctx2);
>> +
>> +            /*
>> +             * 8.1.7 Submit TPM Operation Request to Pre-OS Environment 2
>> +             *
>> +             * Arg 2 (Integer): Function Index = 7
>> +             * Arg 3 (Package): Arguments = Package: Type: Integer
>> +             *                  Integer 1: Operation Value of the Request
>> +             *                  Integer 2: Argument for Operation (optional)
>> +             * Returns: Type: Integer
>> +             *          0: Success
>> +             *          1: Not Implemented
>> +             *          2: General Failure
>> +             *          3: Operation blocked by current firmware settings
>> +             */
>> +            func_idx = aml_local(0);
>> +            ifctx2 = aml_if(aml_equal(func_idx, aml_int(7)));
>> +            {
>> +                /* get opcode */
>> +                op = aml_derefof(aml_index(aml_arg(3), aml_int(0)));
>> +                aml_append(ifctx2, aml_store(op, aml_local(0)));
>> +
>> +                /* get opcode flags */
>> +                op = aml_local(0);
>> +                aml_append(ifctx2, aml_store(aml_call1("TPFN", op),
>> +                                             aml_local(1)));
>> +                /* if func[opcode] & TPM_PPI_FUNC_NOT_IMPLEMENTED */
>> +                op_flags = aml_local(1);
>> +                ifctx3 = aml_if(
>> +                    aml_equal(
>> +                        aml_and(op_flags, aml_int(TPM_PPI_FUNC_MASK), NULL),
>> +                        aml_int(TPM_PPI_FUNC_NOT_IMPLEMENTED)));
>> +                {
>> +                    /* 1: not implemented */
>> +                    aml_append(ifctx3, aml_return(aml_int(1)));
>> +                }
>> +                aml_append(ifctx2, ifctx3);
>> +
>> +                /* if func[opcode] & TPM_PPI_FUNC_BLOCKED */
>> +                op_flags = aml_local(1);
>> +                ifctx3 = aml_if(
>> +                    aml_equal(
>> +                        aml_and(op_flags, aml_int(TPM_PPI_FUNC_MASK), NULL),
>> +                        aml_int(TPM_PPI_FUNC_BLOCKED)));
>> +                {
>> +                    /* 3: blocked by firmware */
>> +                    aml_append(ifctx3, aml_return(aml_int(3)));
>> +                }
>> +                aml_append(ifctx2, ifctx3);
>> +
>> +                /* revision to integer */
>> +                aml_append(ifctx2,
>> +                           aml_store(
>> +                               aml_to_integer(aml_arg(1)),
>> +                               aml_local(1)));
>> +
>> +                rev = aml_local(1);
>> +                ifctx3 = aml_if(aml_equal(rev, aml_int(1)));
>> +                {
>> +                    /* revision 1 */
>> +                    /* PPRQ = op */
>> +                    op = aml_local(0);
>> +                    aml_append(ifctx3, aml_store(op, aml_name("PPRQ")));
>> +                    /* no argument, PPRM = 0 */
>> +                    aml_append(ifctx3, aml_store(aml_int(0),
>> +                                                 aml_name("PPRM")));
>> +                }
>> +                aml_append(ifctx2, ifctx3);
>> +
>> +                rev = aml_local(1);
>> +                ifctx3 = aml_if(aml_equal(rev, aml_int(2)));
>> +                {
>> +                    /* revision 2 */
>> +                    /* PPRQ = op */
>> +                    op = aml_local(0);
>> +                    op_arg = aml_derefof(aml_index(aml_arg(3), aml_int(1)));
>> +                    aml_append(ifctx3, aml_store(op, aml_name("PPRQ")));
>> +                    /* PPRM = arg3[1] */
>> +                    aml_append(ifctx3, aml_store(op_arg, aml_name("PPRM")));
>> +                }
>> +                aml_append(ifctx2, ifctx3);
>> +                /* 0: success */
>> +                aml_append(ifctx2, aml_return(aml_int(0)));
>> +            }
>> +            aml_append(ifctx, ifctx2);
>> +
>> +            /*
>> +             * 8.1.8 Get User Confirmation Status for Operation
>> +             *
>> +             * Arg 2 (Integer): Function Index = 8
>> +             * Arg 3 (Package): Arguments = Package: Type: Integer
>> +             *                  Operation Value that may need user confirmation
>> +             * Returns: Type: Integer
>> +             *          0: Not implemented
>> +             *          1: Firmware only
>> +             *          2: Blocked for OS by firmware configuration
>> +             *          3: Allowed and physically present user required
>> +             *          4: Allowed and physically present user not required
>> +             */
>> +            func_idx = aml_local(0);
>> +            ifctx2 = aml_if(aml_equal(func_idx, aml_int(8)));
>> +            {
>> +                /* get opcode */
>> +                op = aml_derefof(aml_index(aml_arg(3), aml_int(0)));
>> +                aml_append(ifctx2, aml_store(op, aml_local(0)));
>> +
>> +                /* get opcode flags */
>> +                op = aml_local(0);
>> +                aml_append(ifctx2, aml_store(aml_call1("TPFN", op),
>> +                                             aml_local(1)));
>> +                op_flags = aml_local(1);
>> +                /* return confirmation status code */
>> +                aml_append(ifctx2,
>> +                           aml_return(
>> +                               aml_and(op_flags,
>> +                                       aml_int(TPM_PPI_FUNC_MASK), NULL)));
>> +            }
>> +            aml_append(ifctx, ifctx2);
>> +
>> +            aml_append(ifctx, aml_return(aml_buffer(1, zerobyte)));
>> +        }
>> +        aml_append(method, ifctx);
>> +    }
>> +    aml_append(dev, method);
>> +}
>> +
>>  static void
>>  build_dsdt(GArray *table_data, BIOSLinker *linker,
>>             AcpiPmInfo *pm, AcpiMiscInfo *misc,
>> @@ -2153,6 +2569,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(dev);
>> +
>>                  aml_append(scope, dev);
>>              }
>>
>> @@ -2172,6 +2591,8 @@ build_dsdt(GArray *table_data, BIOSLinker *linker,
>>          aml_append(method, aml_return(aml_int(0x0f)));
>>          aml_append(dev, method);
>>
>> +        build_tpm_ppi(dev);
>> +
>>          aml_append(sb_scope, dev);
>>      }
>>
>> @@ -2920,7 +3341,7 @@ void acpi_setup(void)
>>          tpm_config = (FWCfgTPMConfig) {
>>              .tpmppi_address = cpu_to_le32(TPM_PPI_ADDR_BASE),
>>              .tpm_version = cpu_to_le32(tpm_get_version(tpm_find())),
>> -            .tpmppi_version = cpu_to_le32(TPM_PPI_VERSION_NONE)
>> +            .tpmppi_version = cpu_to_le32(TPM_PPI_VERSION_1_30)
> s/cpu_to_le32//

ok

>
>>          };
>>          fw_cfg_add_file(pcms->fw_cfg, "etc/tpm/config",
>>                          &tpm_config, sizeof tpm_config);
>> diff --git a/docs/specs/tpm.txt b/docs/specs/tpm.txt
>> index 2ddb768084..c27762c723 100644
>> --- a/docs/specs/tpm.txt
>> +++ b/docs/specs/tpm.txt
>> @@ -62,6 +62,85 @@ URL:
>>
>>  https://trustedcomputinggroup.org/tcg-acpi-specification/
>>
>> +== ACPI PPI Interface ==
>> +
>> +QEMU supports the Physical Presence Interface (PPI) for TPM 1.2 and TPM 2. This
>> +interface requires ACPI and firmware support. The specification can be found at
>> +the following URL:
>> +
>> +https://trustedcomputinggroup.org/resource/tcg-physical-presence-interface-specification/
>> +
>> +PPI enables a system administrator (root) to request a modification to the
>> +TPM upon reboot. The PPI specification defines the operation requests and the
>> +actions the firmware has to take. The system administrator passes the operation
>> +request number to the firmware through an ACPI interface which writes this
>> +number to a memory location that the firmware knows. Upon reboot, the firmware
>> +finds the number and sends commands to the the TPM. The firmware writes the TPM
>> +result code and the operation request number to a memory location that ACPI can
>> +read from and pass the result on to the administrator.
>> +
>> +The PPI specification defines a set of mandatory and optional operations for
>> +the firmware to implement. The ACPI interface also allows an administrator to
>> +list the supported operations. In QEMU the ACPI code is generated by QEMU, yet
>> +the firmware needs to implement support on a per-operations basis, and
>> +different firmwares may support a different subset. Therefore, QEMU introduces
>> +the virtual memory device for PPI where the firmware can indicate which
>> +operations it supports and ACPI can enable the ones that are supported and
>> +disable all others. This interface lies in main memory and has the following
>> +layout:
>> +
>> + +----------+--------+--------+-------------------------------------------+
>> + |  Field   | Length | Offset | Description                               |
>> + +----------+--------+--------+-------------------------------------------+
>> + | func     |  0x100 |  0x000 | Firmware sets values for each supported   |
>> + |          |        |        | operation. See defined values below.      |
>> + +----------+--------+--------+-------------------------------------------+
>> + | ppin     |   0x1  |  0x100 | SMI interrupt to use. Set by firmware.    |
>> + |          |        |        | Not supported.                            |
>> + +----------+--------+--------+-------------------------------------------+
>> + | ppip     |   0x4  |  0x101 | ACPI function index to pass to SMM code.  |
>> + |          |        |        | Set by ACPI. Not supported.               |
>> + +----------+--------+--------+-------------------------------------------+
>> + | pprp     |   0x4  |  0x105 | Result of last executed operation. Set by |
>> + |          |        |        | firmware. See function index 5 for values.|
>> + +----------+--------+--------+-------------------------------------------+
>> + | pprq     |   0x4  |  0x109 | Operation request number to execute. See  |
>> + |          |        |        | 'Physical Presence Interface Operation    |
>> + |          |        |        | Summary' tables in specs. Set by ACPI.    |
>> + +----------+--------+--------+-------------------------------------------+
>> + | pprm     |   0x4  |  0x10d | Operation request optional parameter.     |
>> + |          |        |        | Values depend on operation. Set by ACPI.  |
>> + +----------+--------+--------+-------------------------------------------+
>> + | lppr     |   0x4  |  0x111 | Last executed operation request number.   |
>> + |          |        |        | Copied from pprq field by firmware.       |
>> + +----------+--------+--------+-------------------------------------------+
>> + | fret     |   0x4  |  0x115 | Result code from SMM function.            |
>> + |          |        |        | Not supported.                            |
>> + +----------+--------+--------+-------------------------------------------+
>> + | res1     |  0x40  |  0x119 | Reserved for future use                   |
>> + +----------+--------+--------+-------------------------------------------+
>> + | next_step|   0x1  |  0x159 | Operation to execute after reboot by      |
>> + |          |        |        | firmware. Used by firmware.               |
>> + +----------+--------+--------+-------------------------------------------+
>> +
>> +   The following values are supported for the 'func' field. They correspond
>> +   to the values used by ACPI function index 8.
>> +
>> + +----------+-------------------------------------------------------------+
>> + | value    | Description                                                 |
>> + +----------+-------------------------------------------------------------+
>> + | 0        | Operation is not implemented.                               |
>> + +----------+-------------------------------------------------------------+
>> + | 1        | Operation is only accessible through firmware.              |
>> + +----------+-------------------------------------------------------------+
>> + | 2        | Operation is blocked for OS by firmware configuration.      |
>> + +----------+-------------------------------------------------------------+
>> + | 3        | Operation is allowed and physically present user required.  |
>> + +----------+-------------------------------------------------------------+
>> + | 4        | Operation is allowed and physically present user is not     |
>> + |          | required.                                                   |
>> + +----------+-------------------------------------------------------------+
>> +
>>
>>  QEMU files related to TPM ACPI tables:
>>   - hw/i386/acpi-build.c
>
>



-- 
Marc-André Lureau

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

[Stefan Berger]

On 06/28/2018 11:24 AM, Marc-André Lureau wrote:
>
>>> +            ifctx2 = aml_if(aml_equal(func_idx, aml_int(3)));
>>> +            {
>>> +                /* revision to integer */
>>> +                aml_append(ifctx2,
>>> +                           aml_store(
>>> +                               aml_to_integer(aml_arg(1)),
>> same do you need cast here, by spec it's 'int' already?
>>
> Yeah, looks like it's unnecessary. Stefan?

Iirc some things were not working at the beginning when I wrote this 
ACPI thing and one problem went away by doing this cast, though it may 
have overlapped with other issues. I cannot say for sure.

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

[Igor Mammedov]

On Thu, 28 Jun 2018 17:24:47 +0200
Marc-André Lureau <marcandre.lureau@gmail.com> wrote:

> Hi
> 
> On Wed, Jun 27, 2018 at 3:19 PM, Igor Mammedov <imammedo@redhat.com> wrote:
> > On Tue, 26 Jun 2018 14:23:43 +0200
> > Marc-André Lureau <marcandre.lureau@redhat.com> wrote:
> >  
> >> From: Stefan Berger <stefanb@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.  
> >
> > 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 <stefanb@linux.vnet.ibm.com>
> >>
> >> ---
> >>
> >> 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.


> 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.


> >> +                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

> >  
> >> +                /* if func[opcode] & TPM_PPI_FUNC_NOT_IMPLEMENTED */
> >> +                ifctx3 = aml_if(
> >> +                    aml_equal(
> >> +                        aml_and(op_flags, aml_int(TPM_PPI_FUNC_MASK), NULL),
> >> +                        aml_int(TPM_PPI_FUNC_NOT_IMPLEMENTED)));
> >> +                {
> >> +                    /* 1: not implemented */  
> > may be the same language as in above comment
> >   /* 1: Operation Value of the Request Not Supported */
> > same applies to other return values  
> 
> ok
> 
> >  
> >> +                    aml_append(ifctx3, aml_return(aml_int(1)));
> >> +                }
> >> +                aml_append(ifctx2, ifctx3);
> >> +
> >> +                op = aml_local(0);
> >> +                aml_append(ifctx2, aml_store(op, aml_name("PPRQ")));
> >> +                aml_append(ifctx2, aml_store(aml_int(0), aml_name("PPRM")));
> >> +                /* 0: success */
> >> +                aml_append(ifctx2, aml_return(aml_int(0)));
> >> +            }
> >> +            aml_append(ifctx, ifctx2);
> >> +
> >> +            /*
> >> +             * 8.1.3 Get Pending TPM Operation Requested By the OS
> >> +             *
> >> +             * Arg 2 (Integer): Function Index = 3
> >> +             * Arg 3 (Package): Arguments = Empty Package
> >> +             * Returns: Type: Package of Integers
> >> +             *          Integer 1: Function Return code
> >> +             *                     0: Success
> >> +             *                     1: General Failure
> >> +             *          Integer 2: Pending operation requested by the OS
> >> +             *                     0: None
> >> +             *                    >0: Operation Value of the Pending Request
> >> +             *          Integer 3: Optional argument to pending operation
> >> +             *                     requested by the OS
> >> +             *                     0: None
> >> +             *                    >0: Argument Value of the Pending Request
> >> +             */
> >> +            func_idx = aml_local(0);
> >> +            ifctx2 = aml_if(aml_equal(func_idx, aml_int(3)));
> >> +            {
> >> +                /* revision to integer */
> >> +                aml_append(ifctx2,
> >> +                           aml_store(
> >> +                               aml_to_integer(aml_arg(1)),  
> > same do you need cast here, by spec it's 'int' already?
> >  
> 
> Yeah, looks like it's unnecessary. Stefan?
> 
> >> +                               aml_local(1)));  
> > why store into local var, couldn't it be used directly ?  
> 
> removed
> 
> >  
> >> +                /*
> >> +                 * Revision ID of 1, no integer parameter beyond
> >> +                 * parameter two are expected
> >> +                 */
> >> +                rev = aml_local(1);
> >> +                ifctx3 = aml_if(aml_equal(rev, aml_int(1)));
> >> +                {
> >> +                    /* TPM2[1] = PPRQ */
> >> +                    aml_append(ifctx3,
> >> +                               aml_store(
> >> +                                   aml_name("PPRQ"),
> >> +                                   aml_index(aml_name("TPM2"), aml_int(1))));
> >> +                    aml_append(ifctx3, aml_return(aml_name("TPM2")));
> >> +                }
> >> +                aml_append(ifctx2, ifctx3);
> >> +
> >> +                /*
> >> +                 * A return value of {0, 23, 1} indicates that
> >> +                 * operation 23 with argument 1 is pending.
> >> +                 */
> >> +                rev = aml_local(1);
> >> +                ifctx3 = aml_if(aml_equal(rev, aml_int(2)));
> >> +                {
> >> +                    /* TPM3[1] = PPRQ */
> >> +                    aml_append(ifctx3,
> >> +                               aml_store(
> >> +                                   aml_name("PPRQ"),
> >> +                                   aml_index(aml_name("TPM3"), aml_int(1))));
> >> +                    /* TPM3[2] = PPRM */
> >> +                    aml_append(ifctx3,
> >> +                               aml_store(
> >> +                                   aml_name("PPRM"),
> >> +                                   aml_index(aml_name("TPM3"), aml_int(2))));
> >> +                    aml_append(ifctx3, aml_return(aml_name("TPM3")));
> >> +                }
> >> +                aml_append(ifctx2, ifctx3);
> >> +            }
> >> +            aml_append(ifctx, ifctx2);
> >> +
> >> +            /*
> >> +             * 8.1.4 Get Platform-Specific Action to Transition to
> >> +             *       Pre-OS Environment
> >> +             *
> >> +             * Arg 2 (Integer): Function Index = 4
> >> +             * Arg 3 (Package): Arguments = Empty Package
> >> +             * Returns: Type: Integer
> >> +             *          0: None
> >> +             *          1: Shutdown
> >> +             *          2: Reboot
> >> +             *          3: OS Vendor-specific
> >> +             */
> >> +            func_idx = aml_local(0);
> >> +            ifctx2 = aml_if(aml_equal(func_idx, aml_int(4)));
> >> +            {
> >> +                /* reboot */
> >> +                aml_append(ifctx2, aml_return(aml_int(2)));
> >> +            }
> >> +            aml_append(ifctx, ifctx2);
> >> +
> >> +            /*
> >> +             * 8.1.5 Return TPM Operation Response to OS Environment
> >> +             *
> >> +             * Arg 2 (Integer): Function Index = 5
> >> +             * Arg 3 (Package): Arguments = Empty Package
> >> +             * Returns: Type: Package of Integer
> >> +             *          Integer 1: Function Return code
> >> +             *                     0: Success
> >> +             *                     1: General Failure
> >> +             *          Integer 2: Most recent operation request
> >> +             *                     0: None
> >> +             *                    >0: Operation Value of the most recent request
> >> +             *          Integer 3: Response to the most recent operation request
> >> +             *                     0: Success
> >> +             *                     0x00000001..0x00000FFF: Corresponding TPM
> >> +             *                                             error code
> >> +             *                     0xFFFFFFF0: User Abort or timeout of dialog
> >> +             *                     0xFFFFFFF1: firmware Failure
> >> +             */
> >> +            func_idx = aml_local(0);
> >> +            ifctx2 = aml_if(aml_equal(func_idx, aml_int(5)));
> >> +            {
> >> +                /* TPM3[1] = LPPR */
> >> +                aml_append(ifctx2,
> >> +                           aml_store(
> >> +                               aml_name("LPPR"),
> >> +                               aml_index(aml_name("TPM3"), aml_int(1))));
> >> +                /* TPM3[2] = PPRP */
> >> +                aml_append(ifctx2,
> >> +                           aml_store(
> >> +                               aml_name("PPRP"),
> >> +                               aml_index(aml_name("TPM3"), aml_int(2))));
> >> +                aml_append(ifctx2, aml_return(aml_name("TPM3")));
> >> +            }
> >> +            aml_append(ifctx, ifctx2);
> >> +
> >> +            /*
> >> +             * 8.1.6 Submit preferred user language
> >> +             *
> >> +             * Arg 2 (Integer): Function Index = 6
> >> +             * Arg 3 (Package): Arguments = String Package
> >> +             *                  Preferred language code
> >> +             * Returns: Type: Integer
> >> +             * Function Return Code
> >> +             *          3: Not implemented
> >> +             */
> >> +            func_idx = aml_local(0);
> >> +            ifctx2 = aml_if(aml_equal(func_idx, aml_int(6)));
> >> +            {
> >> +                /* 3 = not implemented */
> >> +                aml_append(ifctx2, aml_return(aml_int(3)));
> >> +            }
> >> +            aml_append(ifctx, ifctx2);
> >> +
> >> +            /*
> >> +             * 8.1.7 Submit TPM Operation Request to Pre-OS Environment 2
> >> +             *
> >> +             * Arg 2 (Integer): Function Index = 7
> >> +             * Arg 3 (Package): Arguments = Package: Type: Integer
> >> +             *                  Integer 1: Operation Value of the Request
> >> +             *                  Integer 2: Argument for Operation (optional)
> >> +             * Returns: Type: Integer
> >> +             *          0: Success
> >> +             *          1: Not Implemented
> >> +             *          2: General Failure
> >> +             *          3: Operation blocked by current firmware settings
> >> +             */
> >> +            func_idx = aml_local(0);
> >> +            ifctx2 = aml_if(aml_equal(func_idx, aml_int(7)));
> >> +            {
> >> +                /* get opcode */
> >> +                op = aml_derefof(aml_index(aml_arg(3), aml_int(0)));
> >> +                aml_append(ifctx2, aml_store(op, aml_local(0)));
> >> +
> >> +                /* get opcode flags */
> >> +                op = aml_local(0);
> >> +                aml_append(ifctx2, aml_store(aml_call1("TPFN", op),
> >> +                                             aml_local(1)));
> >> +                /* if func[opcode] & TPM_PPI_FUNC_NOT_IMPLEMENTED */
> >> +                op_flags = aml_local(1);
> >> +                ifctx3 = aml_if(
> >> +                    aml_equal(
> >> +                        aml_and(op_flags, aml_int(TPM_PPI_FUNC_MASK), NULL),
> >> +                        aml_int(TPM_PPI_FUNC_NOT_IMPLEMENTED)));
> >> +                {
> >> +                    /* 1: not implemented */
> >> +                    aml_append(ifctx3, aml_return(aml_int(1)));
> >> +                }
> >> +                aml_append(ifctx2, ifctx3);
> >> +
> >> +                /* if func[opcode] & TPM_PPI_FUNC_BLOCKED */
> >> +                op_flags = aml_local(1);
> >> +                ifctx3 = aml_if(
> >> +                    aml_equal(
> >> +                        aml_and(op_flags, aml_int(TPM_PPI_FUNC_MASK), NULL),
> >> +                        aml_int(TPM_PPI_FUNC_BLOCKED)));
> >> +                {
> >> +                    /* 3: blocked by firmware */
> >> +                    aml_append(ifctx3, aml_return(aml_int(3)));
> >> +                }
> >> +                aml_append(ifctx2, ifctx3);
> >> +
> >> +                /* revision to integer */
> >> +                aml_append(ifctx2,
> >> +                           aml_store(
> >> +                               aml_to_integer(aml_arg(1)),
> >> +                               aml_local(1)));
> >> +
> >> +                rev = aml_local(1);
> >> +                ifctx3 = aml_if(aml_equal(rev, aml_int(1)));
> >> +                {
> >> +                    /* revision 1 */
> >> +                    /* PPRQ = op */
> >> +                    op = aml_local(0);
> >> +                    aml_append(ifctx3, aml_store(op, aml_name("PPRQ")));
> >> +                    /* no argument, PPRM = 0 */
> >> +                    aml_append(ifctx3, aml_store(aml_int(0),
> >> +                                                 aml_name("PPRM")));
> >> +                }
> >> +                aml_append(ifctx2, ifctx3);
> >> +
> >> +                rev = aml_local(1);
> >> +                ifctx3 = aml_if(aml_equal(rev, aml_int(2)));
> >> +                {
> >> +                    /* revision 2 */
> >> +                    /* PPRQ = op */
> >> +                    op = aml_local(0);
> >> +                    op_arg = aml_derefof(aml_index(aml_arg(3), aml_int(1)));
> >> +                    aml_append(ifctx3, aml_store(op, aml_name("PPRQ")));
> >> +                    /* PPRM = arg3[1] */
> >> +                    aml_append(ifctx3, aml_store(op_arg, aml_name("PPRM")));
> >> +                }
> >> +                aml_append(ifctx2, ifctx3);
> >> +                /* 0: success */
> >> +                aml_append(ifctx2, aml_return(aml_int(0)));
> >> +            }
> >> +            aml_append(ifctx, ifctx2);
> >> +
> >> +            /*
> >> +             * 8.1.8 Get User Confirmation Status for Operation
> >> +             *
> >> +             * Arg 2 (Integer): Function Index = 8
> >> +             * Arg 3 (Package): Arguments = Package: Type: Integer
> >> +             *                  Operation Value that may need user confirmation
> >> +             * Returns: Type: Integer
> >> +             *          0: Not implemented
> >> +             *          1: Firmware only
> >> +             *          2: Blocked for OS by firmware configuration
> >> +             *          3: Allowed and physically present user required
> >> +             *          4: Allowed and physically present user not required
> >> +             */
> >> +            func_idx = aml_local(0);
> >> +            ifctx2 = aml_if(aml_equal(func_idx, aml_int(8)));
> >> +            {
> >> +                /* get opcode */
> >> +                op = aml_derefof(aml_index(aml_arg(3), aml_int(0)));
> >> +                aml_append(ifctx2, aml_store(op, aml_local(0)));
> >> +
> >> +                /* get opcode flags */
> >> +                op = aml_local(0);
> >> +                aml_append(ifctx2, aml_store(aml_call1("TPFN", op),
> >> +                                             aml_local(1)));
> >> +                op_flags = aml_local(1);
> >> +                /* return confirmation status code */
> >> +                aml_append(ifctx2,
> >> +                           aml_return(
> >> +                               aml_and(op_flags,
> >> +                                       aml_int(TPM_PPI_FUNC_MASK), NULL)));
> >> +            }
> >> +            aml_append(ifctx, ifctx2);
> >> +
> >> +            aml_append(ifctx, aml_return(aml_buffer(1, zerobyte)));
> >> +        }
> >> +        aml_append(method, ifctx);
> >> +    }
> >> +    aml_append(dev, method);
> >> +}
> >> +
> >>  static void
> >>  build_dsdt(GArray *table_data, BIOSLinker *linker,
> >>             AcpiPmInfo *pm, AcpiMiscInfo *misc,
> >> @@ -2153,6 +2569,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(dev);
> >> +
> >>                  aml_append(scope, dev);
> >>              }
> >>
> >> @@ -2172,6 +2591,8 @@ build_dsdt(GArray *table_data, BIOSLinker *linker,
> >>          aml_append(method, aml_return(aml_int(0x0f)));
> >>          aml_append(dev, method);
> >>
> >> +        build_tpm_ppi(dev);
> >> +
> >>          aml_append(sb_scope, dev);
> >>      }
> >>
> >> @@ -2920,7 +3341,7 @@ void acpi_setup(void)
> >>          tpm_config = (FWCfgTPMConfig) {
> >>              .tpmppi_address = cpu_to_le32(TPM_PPI_ADDR_BASE),
> >>              .tpm_version = cpu_to_le32(tpm_get_version(tpm_find())),
> >> -            .tpmppi_version = cpu_to_le32(TPM_PPI_VERSION_NONE)
> >> +            .tpmppi_version = cpu_to_le32(TPM_PPI_VERSION_1_30)  
> > s/cpu_to_le32//  
> 
> ok
> 
> >  
> >>          };
> >>          fw_cfg_add_file(pcms->fw_cfg, "etc/tpm/config",
> >>                          &tpm_config, sizeof tpm_config);
> >> diff --git a/docs/specs/tpm.txt b/docs/specs/tpm.txt
> >> index 2ddb768084..c27762c723 100644
> >> --- a/docs/specs/tpm.txt
> >> +++ b/docs/specs/tpm.txt
> >> @@ -62,6 +62,85 @@ URL:
> >>
> >>  https://trustedcomputinggroup.org/tcg-acpi-specification/
> >>
> >> +== ACPI PPI Interface ==
> >> +
> >> +QEMU supports the Physical Presence Interface (PPI) for TPM 1.2 and TPM 2. This
> >> +interface requires ACPI and firmware support. The specification can be found at
> >> +the following URL:
> >> +
> >> +https://trustedcomputinggroup.org/resource/tcg-physical-presence-interface-specification/
> >> +
> >> +PPI enables a system administrator (root) to request a modification to the
> >> +TPM upon reboot. The PPI specification defines the operation requests and the
> >> +actions the firmware has to take. The system administrator passes the operation
> >> +request number to the firmware through an ACPI interface which writes this
> >> +number to a memory location that the firmware knows. Upon reboot, the firmware
> >> +finds the number and sends commands to the the TPM. The firmware writes the TPM
> >> +result code and the operation request number to a memory location that ACPI can
> >> +read from and pass the result on to the administrator.
> >> +
> >> +The PPI specification defines a set of mandatory and optional operations for
> >> +the firmware to implement. The ACPI interface also allows an administrator to
> >> +list the supported operations. In QEMU the ACPI code is generated by QEMU, yet
> >> +the firmware needs to implement support on a per-operations basis, and
> >> +different firmwares may support a different subset. Therefore, QEMU introduces
> >> +the virtual memory device for PPI where the firmware can indicate which
> >> +operations it supports and ACPI can enable the ones that are supported and
> >> +disable all others. This interface lies in main memory and has the following
> >> +layout:
> >> +
> >> + +----------+--------+--------+-------------------------------------------+
> >> + |  Field   | Length | Offset | Description                               |
> >> + +----------+--------+--------+-------------------------------------------+
> >> + | func     |  0x100 |  0x000 | Firmware sets values for each supported   |
> >> + |          |        |        | operation. See defined values below.      |
> >> + +----------+--------+--------+-------------------------------------------+
> >> + | ppin     |   0x1  |  0x100 | SMI interrupt to use. Set by firmware.    |
> >> + |          |        |        | Not supported.                            |
> >> + +----------+--------+--------+-------------------------------------------+
> >> + | ppip     |   0x4  |  0x101 | ACPI function index to pass to SMM code.  |
> >> + |          |        |        | Set by ACPI. Not supported.               |
> >> + +----------+--------+--------+-------------------------------------------+
> >> + | pprp     |   0x4  |  0x105 | Result of last executed operation. Set by |
> >> + |          |        |        | firmware. See function index 5 for values.|
> >> + +----------+--------+--------+-------------------------------------------+
> >> + | pprq     |   0x4  |  0x109 | Operation request number to execute. See  |
> >> + |          |        |        | 'Physical Presence Interface Operation    |
> >> + |          |        |        | Summary' tables in specs. Set by ACPI.    |
> >> + +----------+--------+--------+-------------------------------------------+
> >> + | pprm     |   0x4  |  0x10d | Operation request optional parameter.     |
> >> + |          |        |        | Values depend on operation. Set by ACPI.  |
> >> + +----------+--------+--------+-------------------------------------------+
> >> + | lppr     |   0x4  |  0x111 | Last executed operation request number.   |
> >> + |          |        |        | Copied from pprq field by firmware.       |
> >> + +----------+--------+--------+-------------------------------------------+
> >> + | fret     |   0x4  |  0x115 | Result code from SMM function.            |
> >> + |          |        |        | Not supported.                            |
> >> + +----------+--------+--------+-------------------------------------------+
> >> + | res1     |  0x40  |  0x119 | Reserved for future use                   |
> >> + +----------+--------+--------+-------------------------------------------+
> >> + | next_step|   0x1  |  0x159 | Operation to execute after reboot by      |
> >> + |          |        |        | firmware. Used by firmware.               |
> >> + +----------+--------+--------+-------------------------------------------+
> >> +
> >> +   The following values are supported for the 'func' field. They correspond
> >> +   to the values used by ACPI function index 8.
> >> +
> >> + +----------+-------------------------------------------------------------+
> >> + | value    | Description                                                 |
> >> + +----------+-------------------------------------------------------------+
> >> + | 0        | Operation is not implemented.                               |
> >> + +----------+-------------------------------------------------------------+
> >> + | 1        | Operation is only accessible through firmware.              |
> >> + +----------+-------------------------------------------------------------+
> >> + | 2        | Operation is blocked for OS by firmware configuration.      |
> >> + +----------+-------------------------------------------------------------+
> >> + | 3        | Operation is allowed and physically present user required.  |
> >> + +----------+-------------------------------------------------------------+
> >> + | 4        | Operation is allowed and physically present user is not     |
> >> + |          | required.                                                   |
> >> + +----------+-------------------------------------------------------------+
> >> +
> >>
> >>  QEMU files related to TPM ACPI tables:
> >>   - hw/i386/acpi-build.c  
> >
> >  
> 
> 
> 

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 <imammedo@redhat.com> wrote:
> On Thu, 28 Jun 2018 17:24:47 +0200
> Marc-André Lureau <marcandre.lureau@gmail.com> wrote:
>
>> Hi
>>
>> On Wed, Jun 27, 2018 at 3:19 PM, Igor Mammedov <imammedo@redhat.com> wrote:
>> > On Tue, 26 Jun 2018 14:23:43 +0200
>> > Marc-André Lureau <marcandre.lureau@redhat.com> wrote:
>> >
>> >> From: Stefan Berger <stefanb@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.
>> >
>> > 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 <stefanb@linux.vnet.ibm.com>
>> >>
>> >> ---
>> >>
>> >> 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