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

[Qemu-devel] [PATCH v3 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")

Git branches:
https://github.com/elmarco/qemu tpm-ppi
https://github.com/elmarco/edk2 tpm-ppi

Marc-André Lureau (1):
  tpm: add a fake ACPI memory clear interface

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      |  26 ++++
 include/hw/acpi/tpm.h |  30 ++++
 hw/i386/acpi-build.c  | 318 ++++++++++++++++++++++++++++++++++++++++++
 hw/tpm/tpm_crb.c      |   5 +
 hw/tpm/tpm_ppi.c      |  56 ++++++++
 hw/tpm/tpm_tis.c      |   5 +
 docs/specs/tpm.txt    |  20 +++
 hw/tpm/Makefile.objs  |   2 +-
 hw/tpm/trace-events   |   4 +
 9 files changed, 465 insertions(+), 1 deletion(-)
 create mode 100644 hw/tpm/tpm_ppi.h
 create mode 100644 hw/tpm/tpm_ppi.c

-- 
2.17.0.253.g3dd125b46d

[Qemu-devel] [PATCH v3 1/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 0xfffef000 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>

---

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      | 26 ++++++++++++++++++++
 include/hw/acpi/tpm.h |  6 +++++
 hw/tpm/tpm_crb.c      |  5 ++++
 hw/tpm/tpm_ppi.c      | 56 +++++++++++++++++++++++++++++++++++++++++++
 hw/tpm/tpm_tis.c      |  5 ++++
 hw/tpm/Makefile.objs  |  2 +-
 hw/tpm/trace-events   |  4 ++++
 7 files changed, 103 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..17030bd989
--- /dev/null
+++ b/hw/tpm/tpm_ppi.h
@@ -0,0 +1,26 @@
+/*
+ * 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, 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 a92dd50437..4f585564d9 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 {
@@ -41,6 +42,8 @@ typedef struct CRBState {
     MemoryRegion cmdmem;
 
     size_t be_buffer_size;
+
+    TPMPPI ppi;
 } CRBState;
 
 #define CRB(obj) OBJECT_CHECK(CRBState, (obj), TYPE_TPM_CRB)
@@ -291,6 +294,8 @@ 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);
 
+    tpm_ppi_init_io(&s->ppi, get_system_memory(), 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..0019c3e6fc
--- /dev/null
+++ b/hw/tpm/tpm_ppi.c
@@ -0,0 +1,56 @@
+/*
+ * 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, 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, TPM_PPI_ADDR_BASE, &tpmppi->mmio);
+}
diff --git a/hw/tpm/tpm_tis.c b/hw/tpm/tpm_tis.c
index 12f5c9a759..9a2fec455a 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 */
@@ -81,6 +82,8 @@ typedef struct TPMState {
     TPMVersion be_tpm_version;
 
     size_t be_buffer_size;
+
+    TPMPPI ppi;
 } TPMState;
 
 #define TPM(obj) OBJECT_CHECK(TPMState, (obj), TYPE_TPM_TIS)
@@ -976,6 +979,8 @@ 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);
+
+    tpm_ppi_init_io(&s->ppi, isa_address_space(ISA_DEVICE(dev)), 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.17.0.253.g3dd125b46d

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

[Stefan Berger]

On 05/15/2018 08:14 AM, Marc-André Lureau 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 0xfffef000 and used by ACPI to send messages to the

It's now located at 0xFED45000. I changed that without changing the 
commit text... Rest looks good :-)


> 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>
>
> ---
>
> 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      | 26 ++++++++++++++++++++
>   include/hw/acpi/tpm.h |  6 +++++
>   hw/tpm/tpm_crb.c      |  5 ++++
>   hw/tpm/tpm_ppi.c      | 56 +++++++++++++++++++++++++++++++++++++++++++
>   hw/tpm/tpm_tis.c      |  5 ++++
>   hw/tpm/Makefile.objs  |  2 +-
>   hw/tpm/trace-events   |  4 ++++
>   7 files changed, 103 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..17030bd989
> --- /dev/null
> +++ b/hw/tpm/tpm_ppi.h
> @@ -0,0 +1,26 @@
> +/*
> + * 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, 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 a92dd50437..4f585564d9 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 {
> @@ -41,6 +42,8 @@ typedef struct CRBState {
>       MemoryRegion cmdmem;
>
>       size_t be_buffer_size;
> +
> +    TPMPPI ppi;
>   } CRBState;
>
>   #define CRB(obj) OBJECT_CHECK(CRBState, (obj), TYPE_TPM_CRB)
> @@ -291,6 +294,8 @@ 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);
>
> +    tpm_ppi_init_io(&s->ppi, get_system_memory(), 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..0019c3e6fc
> --- /dev/null
> +++ b/hw/tpm/tpm_ppi.c
> @@ -0,0 +1,56 @@
> +/*
> + * 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, 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, TPM_PPI_ADDR_BASE, &tpmppi->mmio);
> +}
> diff --git a/hw/tpm/tpm_tis.c b/hw/tpm/tpm_tis.c
> index 12f5c9a759..9a2fec455a 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 */
> @@ -81,6 +82,8 @@ typedef struct TPMState {
>       TPMVersion be_tpm_version;
>
>       size_t be_buffer_size;
> +
> +    TPMPPI ppi;
>   } TPMState;
>
>   #define TPM(obj) OBJECT_CHECK(TPMState, (obj), TYPE_TPM_TIS)
> @@ -976,6 +979,8 @@ 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);
> +
> +    tpm_ppi_init_io(&s->ppi, isa_address_space(ISA_DEVICE(dev)), 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 v3 1/4] tpm: implement virtual memory device for TPM PPI

[Igor Mammedov]

On Tue, 15 May 2018 14:14:30 +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 0xfffef000 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>
> 
> ---
> 
> 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      | 26 ++++++++++++++++++++
>  include/hw/acpi/tpm.h |  6 +++++
>  hw/tpm/tpm_crb.c      |  5 ++++
>  hw/tpm/tpm_ppi.c      | 56 +++++++++++++++++++++++++++++++++++++++++++
>  hw/tpm/tpm_tis.c      |  5 ++++
>  hw/tpm/Makefile.objs  |  2 +-
>  hw/tpm/trace-events   |  4 ++++
>  7 files changed, 103 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..17030bd989
> --- /dev/null
> +++ b/hw/tpm/tpm_ppi.h
> @@ -0,0 +1,26 @@
> +/*
> + * 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, 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 a92dd50437..4f585564d9 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 {
> @@ -41,6 +42,8 @@ typedef struct CRBState {
>      MemoryRegion cmdmem;
>  
>      size_t be_buffer_size;
> +
> +    TPMPPI ppi;
>  } CRBState;
>  
>  #define CRB(obj) OBJECT_CHECK(CRBState, (obj), TYPE_TPM_CRB)
> @@ -291,6 +294,8 @@ 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);
>  
> +    tpm_ppi_init_io(&s->ppi, get_system_memory(), 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..0019c3e6fc
> --- /dev/null
> +++ b/hw/tpm/tpm_ppi.c
> @@ -0,0 +1,56 @@
> +/*
> + * 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, 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, TPM_PPI_ADDR_BASE, &tpmppi->mmio);
I'd push TPM_PPI_ADDR_BASE up to the stack so it would be obvious
at tpm_ppi_init_io() call site wrt which region the offset is applied

also maybe s/TPM_PPI_ADDR_BASE/TPM_PPI_ADDR_OFFSET/?

> +}
> diff --git a/hw/tpm/tpm_tis.c b/hw/tpm/tpm_tis.c
> index 12f5c9a759..9a2fec455a 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 */
> @@ -81,6 +82,8 @@ typedef struct TPMState {
>      TPMVersion be_tpm_version;
>  
>      size_t be_buffer_size;
> +
> +    TPMPPI ppi;
>  } TPMState;
>  
>  #define TPM(obj) OBJECT_CHECK(TPMState, (obj), TYPE_TPM_TIS)
> @@ -976,6 +979,8 @@ 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);
> +
> +    tpm_ppi_init_io(&s->ppi, isa_address_space(ISA_DEVICE(dev)), OBJECT(s));
what address space it would get here and  at what offset address space is mapped?

>  }
>  
>  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 v3 1/4] tpm: implement virtual memory device for TPM PPI

[Marc-André Lureau]

Hi

On Thu, Jun 21, 2018 at 11:49 AM, Igor Mammedov <imammedo@redhat.com> wrote:
> On Tue, 15 May 2018 14:14:30 +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 0xfffef000 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>
>>
>> ---
>>
>> 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      | 26 ++++++++++++++++++++
>>  include/hw/acpi/tpm.h |  6 +++++
>>  hw/tpm/tpm_crb.c      |  5 ++++
>>  hw/tpm/tpm_ppi.c      | 56 +++++++++++++++++++++++++++++++++++++++++++
>>  hw/tpm/tpm_tis.c      |  5 ++++
>>  hw/tpm/Makefile.objs  |  2 +-
>>  hw/tpm/trace-events   |  4 ++++
>>  7 files changed, 103 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..17030bd989
>> --- /dev/null
>> +++ b/hw/tpm/tpm_ppi.h
>> @@ -0,0 +1,26 @@
>> +/*
>> + * 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, 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 a92dd50437..4f585564d9 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 {
>> @@ -41,6 +42,8 @@ typedef struct CRBState {
>>      MemoryRegion cmdmem;
>>
>>      size_t be_buffer_size;
>> +
>> +    TPMPPI ppi;
>>  } CRBState;
>>
>>  #define CRB(obj) OBJECT_CHECK(CRBState, (obj), TYPE_TPM_CRB)
>> @@ -291,6 +294,8 @@ 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);
>>
>> +    tpm_ppi_init_io(&s->ppi, get_system_memory(), 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..0019c3e6fc
>> --- /dev/null
>> +++ b/hw/tpm/tpm_ppi.c
>> @@ -0,0 +1,56 @@
>> +/*
>> + * 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, 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, TPM_PPI_ADDR_BASE, &tpmppi->mmio);
> I'd push TPM_PPI_ADDR_BASE up to the stack so it would be obvious
> at tpm_ppi_init_io() call site wrt which region the offset is applied
>
> also maybe s/TPM_PPI_ADDR_BASE/TPM_PPI_ADDR_OFFSET/?

Hmm, it's a fixed address, and we already use TPM_TIS_ADDR_BASE /
TPM_CRB_ADDR_BASE.

>
>> +}
>> diff --git a/hw/tpm/tpm_tis.c b/hw/tpm/tpm_tis.c
>> index 12f5c9a759..9a2fec455a 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 */
>> @@ -81,6 +82,8 @@ typedef struct TPMState {
>>      TPMVersion be_tpm_version;
>>
>>      size_t be_buffer_size;
>> +
>> +    TPMPPI ppi;
>>  } TPMState;
>>
>>  #define TPM(obj) OBJECT_CHECK(TPMState, (obj), TYPE_TPM_TIS)
>> @@ -976,6 +979,8 @@ 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);
>> +
>> +    tpm_ppi_init_io(&s->ppi, isa_address_space(ISA_DEVICE(dev)), OBJECT(s));
> what address space it would get here and  at what offset address space is mapped?

Agree, that's a bit better, done

thanks

>
>>  }
>>
>>  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"
>
>



-- 
Marc-André Lureau

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

[Igor Mammedov]

On Thu, 21 Jun 2018 12:51:34 +0200
Marc-André Lureau <marcandre.lureau@gmail.com> wrote:

> Hi
> 
> On Thu, Jun 21, 2018 at 11:49 AM, Igor Mammedov <imammedo@redhat.com> wrote:
> > On Tue, 15 May 2018 14:14:30 +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 0xfffef000 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>
> >>
> >> ---
> >>
> >> 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      | 26 ++++++++++++++++++++
> >>  include/hw/acpi/tpm.h |  6 +++++
> >>  hw/tpm/tpm_crb.c      |  5 ++++
> >>  hw/tpm/tpm_ppi.c      | 56 +++++++++++++++++++++++++++++++++++++++++++
> >>  hw/tpm/tpm_tis.c      |  5 ++++
> >>  hw/tpm/Makefile.objs  |  2 +-
> >>  hw/tpm/trace-events   |  4 ++++
> >>  7 files changed, 103 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..17030bd989
> >> --- /dev/null
> >> +++ b/hw/tpm/tpm_ppi.h
> >> @@ -0,0 +1,26 @@
> >> +/*
> >> + * 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, 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 a92dd50437..4f585564d9 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 {
> >> @@ -41,6 +42,8 @@ typedef struct CRBState {
> >>      MemoryRegion cmdmem;
> >>
> >>      size_t be_buffer_size;
> >> +
> >> +    TPMPPI ppi;
> >>  } CRBState;
> >>
> >>  #define CRB(obj) OBJECT_CHECK(CRBState, (obj), TYPE_TPM_CRB)
> >> @@ -291,6 +294,8 @@ 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);
> >>
> >> +    tpm_ppi_init_io(&s->ppi, get_system_memory(), 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..0019c3e6fc
> >> --- /dev/null
> >> +++ b/hw/tpm/tpm_ppi.c
> >> @@ -0,0 +1,56 @@
> >> +/*
> >> + * 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, 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, TPM_PPI_ADDR_BASE, &tpmppi->mmio);  
> > I'd push TPM_PPI_ADDR_BASE up to the stack so it would be obvious
> > at tpm_ppi_init_io() call site wrt which region the offset is applied
> >
> > also maybe s/TPM_PPI_ADDR_BASE/TPM_PPI_ADDR_OFFSET/?  
> 
> Hmm, it's a fixed address, and we already use TPM_TIS_ADDR_BASE /
> TPM_CRB_ADDR_BASE.
problem here is that it is no clear if 'm' is address space where
absolute address is fine to use ot some subregion in which case
it should be offset relative to 'm'.

it would get worse/more confusing when called with isa address
space is passed in in wich case I have not a clue where it would end up.

> 
> >  
> >> +}
> >> diff --git a/hw/tpm/tpm_tis.c b/hw/tpm/tpm_tis.c
> >> index 12f5c9a759..9a2fec455a 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 */
> >> @@ -81,6 +82,8 @@ typedef struct TPMState {
> >>      TPMVersion be_tpm_version;
> >>
> >>      size_t be_buffer_size;
> >> +
> >> +    TPMPPI ppi;
> >>  } TPMState;
> >>
> >>  #define TPM(obj) OBJECT_CHECK(TPMState, (obj), TYPE_TPM_TIS)
> >> @@ -976,6 +979,8 @@ 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);
> >> +
> >> +    tpm_ppi_init_io(&s->ppi, isa_address_space(ISA_DEVICE(dev)), OBJECT(s));  
> > what address space it would get here and  at what offset address space is mapped?  
> 
> Agree, that's a bit better, done
> 
> thanks
> 
> >  
> >>  }
> >>
> >>  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"  
> >
> >  
> 
> 
> 

[Qemu-devel] [PATCH v3 2/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>
---
 include/hw/acpi/tpm.h |  3 +++
 hw/i386/acpi-build.c  | 17 +++++++++++++++++
 docs/specs/tpm.txt    | 20 ++++++++++++++++++++
 3 files changed, 40 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..f6d447f03a 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,7 @@ void acpi_setup(void)
     AcpiBuildTables tables;
     AcpiBuildState *build_state;
     Object *vmgenid_dev;
+    static FWCfgTPMConfig tpm_config;
 
     if (!pcms->fw_cfg) {
         ACPI_BUILD_DPRINTF("No fw cfg. Bailing out.\n");
@@ -2907,6 +2914,16 @@ void acpi_setup(void)
     fw_cfg_add_file(pcms->fw_cfg, ACPI_BUILD_TPMLOG_FILE,
                     tables.tcpalog->data, acpi_data_len(tables.tcpalog));
 
+    if (tpm_find()) {
+        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.17.0.253.g3dd125b46d

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

[Igor Mammedov]

On Tue, 15 May 2018 14:14:31 +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.
is it related to TPM_PPI_ADDR_BASE added in previous patch?

> 
> 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>
> ---
>  include/hw/acpi/tpm.h |  3 +++
>  hw/i386/acpi-build.c  | 17 +++++++++++++++++
>  docs/specs/tpm.txt    | 20 ++++++++++++++++++++
>  3 files changed, 40 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..f6d447f03a 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,7 @@ void acpi_setup(void)
>      AcpiBuildTables tables;
>      AcpiBuildState *build_state;
>      Object *vmgenid_dev;
> +    static FWCfgTPMConfig tpm_config;
>  
>      if (!pcms->fw_cfg) {
>          ACPI_BUILD_DPRINTF("No fw cfg. Bailing out.\n");
> @@ -2907,6 +2914,16 @@ void acpi_setup(void)
>      fw_cfg_add_file(pcms->fw_cfg, ACPI_BUILD_TPMLOG_FILE,
>                      tables.tcpalog->data, acpi_data_len(tables.tcpalog));
>  
> +    if (tpm_find()) {
> +        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);
> +    }
why it's in ACPI part of the code, shouldn't it be a part of device,
could TPM be used without ACPI at all (-noacpi CLI option)?

Wouldn't adding fwcfg entry unconditionally break migration?

> +
>      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 v3 2/4] acpi: add fw_cfg file for TPM and PPI virtual memory device

[Marc-André Lureau]

Hi

On Thu, Jun 21, 2018 at 12:00 PM, Igor Mammedov <imammedo@redhat.com> wrote:
> On Tue, 15 May 2018 14:14:31 +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.
> is it related to TPM_PPI_ADDR_BASE added in previous patch?
>
>>
>> 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>
>> ---
>>  include/hw/acpi/tpm.h |  3 +++
>>  hw/i386/acpi-build.c  | 17 +++++++++++++++++
>>  docs/specs/tpm.txt    | 20 ++++++++++++++++++++
>>  3 files changed, 40 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..f6d447f03a 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,7 @@ void acpi_setup(void)
>>      AcpiBuildTables tables;
>>      AcpiBuildState *build_state;
>>      Object *vmgenid_dev;
>> +    static FWCfgTPMConfig tpm_config;
>>
>>      if (!pcms->fw_cfg) {
>>          ACPI_BUILD_DPRINTF("No fw cfg. Bailing out.\n");
>> @@ -2907,6 +2914,16 @@ void acpi_setup(void)
>>      fw_cfg_add_file(pcms->fw_cfg, ACPI_BUILD_TPMLOG_FILE,
>>                      tables.tcpalog->data, acpi_data_len(tables.tcpalog));
>>
>> +    if (tpm_find()) {
>> +        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);
>> +    }
> why it's in ACPI part of the code, shouldn't it be a part of device,
> could TPM be used without ACPI at all (-noacpi CLI option)?
>
> Wouldn't adding fwcfg entry unconditionally break migration?

Because of unstable entry IDs? that could be problematic. (especially
during boot time) What do you think Laszlo?

I guess we could have a "ppi" device property, that would imply having
the etc/tpm/config fw_cfg entry. We would enable it by default in
newer machine types (3.0?)

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



-- 
Marc-André Lureau

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

[Igor Mammedov]

On Thu, 21 Jun 2018 12:10:32 +0200
Marc-André Lureau <marcandre.lureau@gmail.com> wrote:

> Hi
> 
> On Thu, Jun 21, 2018 at 12:00 PM, Igor Mammedov <imammedo@redhat.com> wrote:
> > On Tue, 15 May 2018 14:14:31 +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.  
> > is it related to TPM_PPI_ADDR_BASE added in previous patch?
> >  
> >>
> >> 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>
> >> ---
> >>  include/hw/acpi/tpm.h |  3 +++
> >>  hw/i386/acpi-build.c  | 17 +++++++++++++++++
> >>  docs/specs/tpm.txt    | 20 ++++++++++++++++++++
> >>  3 files changed, 40 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..f6d447f03a 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,7 @@ void acpi_setup(void)
> >>      AcpiBuildTables tables;
> >>      AcpiBuildState *build_state;
> >>      Object *vmgenid_dev;
> >> +    static FWCfgTPMConfig tpm_config;
> >>
> >>      if (!pcms->fw_cfg) {
> >>          ACPI_BUILD_DPRINTF("No fw cfg. Bailing out.\n");
> >> @@ -2907,6 +2914,16 @@ void acpi_setup(void)
> >>      fw_cfg_add_file(pcms->fw_cfg, ACPI_BUILD_TPMLOG_FILE,
> >>                      tables.tcpalog->data, acpi_data_len(tables.tcpalog));
> >>
> >> +    if (tpm_find()) {
> >> +        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);
> >> +    }  
> > why it's in ACPI part of the code, shouldn't it be a part of device,
> > could TPM be used without ACPI at all (-noacpi CLI option)?
> >
> > Wouldn't adding fwcfg entry unconditionally break migration?  
> 
> Because of unstable entry IDs? that could be problematic. (especially
> during boot time) What do you think Laszlo?
> 
> I guess we could have a "ppi" device property, that would imply having
> the etc/tpm/config fw_cfg entry. We would enable it by default in
> newer machine types (3.0?)
yep, something like this.
usual practice is to enable it by default and disable it for old
machine types using hw compat props (include/hw/compat.h)

> 
> >  
> >> +
> >>      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 v3 2/4] acpi: add fw_cfg file for TPM and PPI virtual memory device

[Laszlo Ersek]

On 06/21/18 12:10, Marc-André Lureau wrote:

> What do you think Laszlo?

Apologies, I'm currently lacking the bandwidth to even understand the
question. I'm tagging this message for later; it'll take a while before
I get to it.

Thanks
Laszlo

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

[Laszlo Ersek]

On 06/21/18 12:10, Marc-André Lureau wrote:
> Hi
> 
> On Thu, Jun 21, 2018 at 12:00 PM, Igor Mammedov <imammedo@redhat.com> wrote:
>> On Tue, 15 May 2018 14:14:31 +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.
>> is it related to TPM_PPI_ADDR_BASE added in previous patch?
>>
>>>
>>> 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>
>>> ---
>>>  include/hw/acpi/tpm.h |  3 +++
>>>  hw/i386/acpi-build.c  | 17 +++++++++++++++++
>>>  docs/specs/tpm.txt    | 20 ++++++++++++++++++++
>>>  3 files changed, 40 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..f6d447f03a 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,7 @@ void acpi_setup(void)
>>>      AcpiBuildTables tables;
>>>      AcpiBuildState *build_state;
>>>      Object *vmgenid_dev;
>>> +    static FWCfgTPMConfig tpm_config;
>>>
>>>      if (!pcms->fw_cfg) {
>>>          ACPI_BUILD_DPRINTF("No fw cfg. Bailing out.\n");
>>> @@ -2907,6 +2914,16 @@ void acpi_setup(void)
>>>      fw_cfg_add_file(pcms->fw_cfg, ACPI_BUILD_TPMLOG_FILE,
>>>                      tables.tcpalog->data, acpi_data_len(tables.tcpalog));
>>>
>>> +    if (tpm_find()) {
>>> +        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);
>>> +    }
>> why it's in ACPI part of the code, shouldn't it be a part of device,
>> could TPM be used without ACPI at all (-noacpi CLI option)?
>>
>> Wouldn't adding fwcfg entry unconditionally break migration?
> 
> Because of unstable entry IDs? that could be problematic. (especially
> during boot time) What do you think Laszlo?
> 
> I guess we could have a "ppi" device property, that would imply having
> the etc/tpm/config fw_cfg entry. We would enable it by default in
> newer machine types (3.0?)

Can we perhaps draw a parallel with "-device vmcoreinfo" here? For that
device model, fw_cfg_add_file_callback() is called in the realize
function, vmcoreinfo_realize(). If libvirt generates the identical
cmdline on both ends of the migration, and uses the same machine type, I
think the fw_cfg selectors should end up the same on both sides.

Thanks
Laszlo

>>
>>> +
>>>      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 v3 2/4] acpi: add fw_cfg file for TPM and PPI virtual memory device

[Marc-André Lureau]

Hi

On Mon, Jun 25, 2018 at 5:20 PM, Laszlo Ersek <lersek@redhat.com> wrote:
> On 06/21/18 12:10, Marc-André Lureau wrote:
>> Hi
>>
>> On Thu, Jun 21, 2018 at 12:00 PM, Igor Mammedov <imammedo@redhat.com> wrote:
>>> On Tue, 15 May 2018 14:14:31 +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.
>>> is it related to TPM_PPI_ADDR_BASE added in previous patch?
>>>
>>>>
>>>> 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>
>>>> ---
>>>>  include/hw/acpi/tpm.h |  3 +++
>>>>  hw/i386/acpi-build.c  | 17 +++++++++++++++++
>>>>  docs/specs/tpm.txt    | 20 ++++++++++++++++++++
>>>>  3 files changed, 40 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..f6d447f03a 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,7 @@ void acpi_setup(void)
>>>>      AcpiBuildTables tables;
>>>>      AcpiBuildState *build_state;
>>>>      Object *vmgenid_dev;
>>>> +    static FWCfgTPMConfig tpm_config;
>>>>
>>>>      if (!pcms->fw_cfg) {
>>>>          ACPI_BUILD_DPRINTF("No fw cfg. Bailing out.\n");
>>>> @@ -2907,6 +2914,16 @@ void acpi_setup(void)
>>>>      fw_cfg_add_file(pcms->fw_cfg, ACPI_BUILD_TPMLOG_FILE,
>>>>                      tables.tcpalog->data, acpi_data_len(tables.tcpalog));
>>>>
>>>> +    if (tpm_find()) {
>>>> +        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);
>>>> +    }
>>> why it's in ACPI part of the code, shouldn't it be a part of device,
>>> could TPM be used without ACPI at all (-noacpi CLI option)?
>>>
>>> Wouldn't adding fwcfg entry unconditionally break migration?
>>
>> Because of unstable entry IDs? that could be problematic. (especially
>> during boot time) What do you think Laszlo?
>>
>> I guess we could have a "ppi" device property, that would imply having
>> the etc/tpm/config fw_cfg entry. We would enable it by default in
>> newer machine types (3.0?)
>
> Can we perhaps draw a parallel with "-device vmcoreinfo" here? For that
> device model, fw_cfg_add_file_callback() is called in the realize
> function, vmcoreinfo_realize(). If libvirt generates the identical
> cmdline on both ends of the migration, and uses the same machine type, I
> think the fw_cfg selectors should end up the same on both sides.

-device vmcoreinfo is a  standalone fw-cfg entry. PPI is tied to a
TPM, the fw-cfg entry is an implementation detail between qemu and
firmware. So I prefer the "ppi" device property. Wrt to migration,
that should be equivalent to -device vmcoreinfo (except that -device
vmcoreinfo is not enabled by default in newer machine types)

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



-- 
Marc-André Lureau

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

[Laszlo Ersek]

On 06/26/18 12:38, Marc-André Lureau wrote:
> Hi
> 
> On Mon, Jun 25, 2018 at 5:20 PM, Laszlo Ersek <lersek@redhat.com> wrote:
>> On 06/21/18 12:10, Marc-André Lureau wrote:
>>> Hi
>>>
>>> On Thu, Jun 21, 2018 at 12:00 PM, Igor Mammedov <imammedo@redhat.com> wrote:
>>>> On Tue, 15 May 2018 14:14:31 +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.
>>>> is it related to TPM_PPI_ADDR_BASE added in previous patch?
>>>>
>>>>>
>>>>> 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>
>>>>> ---
>>>>>  include/hw/acpi/tpm.h |  3 +++
>>>>>  hw/i386/acpi-build.c  | 17 +++++++++++++++++
>>>>>  docs/specs/tpm.txt    | 20 ++++++++++++++++++++
>>>>>  3 files changed, 40 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..f6d447f03a 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,7 @@ void acpi_setup(void)
>>>>>      AcpiBuildTables tables;
>>>>>      AcpiBuildState *build_state;
>>>>>      Object *vmgenid_dev;
>>>>> +    static FWCfgTPMConfig tpm_config;
>>>>>
>>>>>      if (!pcms->fw_cfg) {
>>>>>          ACPI_BUILD_DPRINTF("No fw cfg. Bailing out.\n");
>>>>> @@ -2907,6 +2914,16 @@ void acpi_setup(void)
>>>>>      fw_cfg_add_file(pcms->fw_cfg, ACPI_BUILD_TPMLOG_FILE,
>>>>>                      tables.tcpalog->data, acpi_data_len(tables.tcpalog));
>>>>>
>>>>> +    if (tpm_find()) {
>>>>> +        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);
>>>>> +    }
>>>> why it's in ACPI part of the code, shouldn't it be a part of device,
>>>> could TPM be used without ACPI at all (-noacpi CLI option)?
>>>>
>>>> Wouldn't adding fwcfg entry unconditionally break migration?
>>>
>>> Because of unstable entry IDs? that could be problematic. (especially
>>> during boot time) What do you think Laszlo?
>>>
>>> I guess we could have a "ppi" device property, that would imply having
>>> the etc/tpm/config fw_cfg entry. We would enable it by default in
>>> newer machine types (3.0?)
>>
>> Can we perhaps draw a parallel with "-device vmcoreinfo" here? For that
>> device model, fw_cfg_add_file_callback() is called in the realize
>> function, vmcoreinfo_realize(). If libvirt generates the identical
>> cmdline on both ends of the migration, and uses the same machine type, I
>> think the fw_cfg selectors should end up the same on both sides.
> 
> -device vmcoreinfo is a  standalone fw-cfg entry. PPI is tied to a
> TPM, the fw-cfg entry is an implementation detail between qemu and
> firmware. So I prefer the "ppi" device property.

Ah, right, sorry I got confused for a moment. The TPM device makes sense
without the (optional) Physical Presence Interface too.

Thanks,
Laszlo

> Wrt to migration,
> that should be equivalent to -device vmcoreinfo (except that -device
> vmcoreinfo is not enabled by default in newer machine types)
> 
>> Thanks
>> Laszlo
>>
>>>>
>>>>> +
>>>>>      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
>>>>
>>>>
>>>
>>>
>>>
>>
> 
> 
> 

[Qemu-devel] [PATCH v3 3/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>

---

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 |  21 +++
 hw/i386/acpi-build.c  | 294 +++++++++++++++++++++++++++++++++++++++++-
 2 files changed, 314 insertions(+), 1 deletion(-)

diff --git a/include/hw/acpi/tpm.h b/include/hw/acpi/tpm.h
index f79d68a77a..fc53f08827 100644
--- a/include/hw/acpi/tpm.h
+++ b/include/hw/acpi/tpm.h
@@ -196,4 +196,25 @@ REG32(CRB_DATA_BUFFER, 0x80)
 #define TPM_PPI_VERSION_NONE        0
 #define TPM_PPI_VERSION_1_30        1
 
+struct tpm_ppi {
+    uint8_t  func[256];      /* 0x000: per TPM function implementation flags;
+                                       set by BIOS */
+/* 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)
+    uint8_t ppin;            /* 0x100 : set by BIOS */
+    uint32_t ppip;           /* 0x101 : set by ACPI; not used */
+    uint32_t pprp;           /* 0x105 : response from TPM; set by BIOS */
+    uint32_t pprq;           /* 0x109 : opcode; set by ACPI */
+    uint32_t pprm;           /* 0x10d : parameter for opcode; set by ACPI */
+    uint32_t lppr;           /* 0x111 : last opcode; set by BIOS */
+    uint32_t fret;           /* 0x115 : set by ACPI; not used */
+    uint8_t res1[0x40];      /* 0x119 : reserved for future use */
+    uint8_t next_step;       /* 0x159 : next step after reboot; set by BIOS */
+} QEMU_PACKED;
+
 #endif /* HW_ACPI_TPM_H */
diff --git a/hw/i386/acpi-build.c b/hw/i386/acpi-build.c
index f6d447f03a..95be4f0710 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,292 @@ static Aml *build_q35_osc_method(void)
     return method;
 }
 
+static void
+build_tpm_ppi(Aml *dev)
+{
+    Aml *method, *name, *field, *ifctx, *ifctx2, *ifctx3, *pak;
+    struct tpm_ppi *tpm_ppi = NULL;
+    int i;
+
+    /*
+     * 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),
+                                    sizeof(tpm_ppi->func)));
+    field = aml_field("TPP1", AML_ANY_ACC, AML_NOLOCK, AML_PRESERVE);
+    for (i = 0; i < sizeof(tpm_ppi->func); i++) {
+        char *tmp = g_strdup_printf("FN%02X", i);
+        aml_append(field, aml_named_field(tmp, BITS_PER_BYTE));
+        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 +
+                                            offsetof(struct tpm_ppi, ppin)),
+                                    sizeof(struct tpm_ppi) -
+                                        sizeof(tpm_ppi->func)));
+    field = aml_field("TPP2", AML_ANY_ACC, AML_NOLOCK, AML_PRESERVE);
+    aml_append(field, aml_named_field("PPIN",
+               sizeof(uint8_t) * BITS_PER_BYTE));
+    aml_append(field, aml_named_field("PPIP",
+               sizeof(uint32_t) * BITS_PER_BYTE));
+    aml_append(field, aml_named_field("PPRP",
+               sizeof(uint32_t) * BITS_PER_BYTE));
+    aml_append(field, aml_named_field("PPRQ",
+               sizeof(uint32_t) * BITS_PER_BYTE));
+    aml_append(field, aml_named_field("PPRM",
+               sizeof(uint32_t) * BITS_PER_BYTE));
+    aml_append(field, aml_named_field("LPPR",
+               sizeof(uint32_t) * BITS_PER_BYTE));
+    aml_append(dev, field);
+
+    method = aml_method("TPFN", 1, AML_SERIALIZED);
+    {
+        for (i = 0; i < sizeof(tpm_ppi->func); 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 };
+
+        ifctx = aml_if(
+            aml_equal(aml_arg(0),
+                      aml_touuid("3DDDFAA6-361B-4EB4-A424-8D10089D1653")));
+        {
+            aml_append(ifctx,
+                       aml_store(aml_to_integer(aml_arg(2)), aml_local(0)));
+
+            /* standard DSM query function */
+            ifctx2 = aml_if(aml_equal(aml_local(0), aml_int(0)));
+            {
+                uint8_t byte_list[2] = { 0xff, 0x01 };
+                aml_append(ifctx2, aml_return(aml_buffer(2, byte_list)));
+            }
+            aml_append(ifctx, ifctx2);
+
+            /* interface version: 1.3 */
+            ifctx2 = aml_if(aml_equal(aml_local(0), aml_int(1)));
+            {
+                aml_append(ifctx2, aml_return(aml_string("1.3")));
+            }
+            aml_append(ifctx, ifctx2);
+
+            /* submit TPM operation */
+            ifctx2 = aml_if(aml_equal(aml_local(0), aml_int(2)));
+            {
+                /* get opcode */
+                aml_append(ifctx2,
+                           aml_store(aml_derefof(aml_index(aml_arg(3),
+                                                           aml_int(0))),
+                                     aml_local(0)));
+                /* get opcode flags */
+                aml_append(ifctx2,
+                           aml_store(aml_call1("TPFN", aml_local(0)),
+                                     aml_local(1)));
+                ifctx3 = aml_if(
+                    aml_equal(
+                        aml_and(aml_local(1), 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);
+                aml_append(ifctx2, aml_store(aml_local(0), 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);
+
+            /* get pending TPM operation */
+            ifctx2 = aml_if(aml_equal(aml_local(0), aml_int(3)));
+            {
+                /* revision to integer */
+                aml_append(ifctx2,
+                           aml_store(
+                               aml_to_integer(aml_arg(1)),
+                               aml_local(1)));
+                ifctx3 = aml_if(aml_equal(aml_local(1), aml_int(1)));
+                {
+                    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);
+
+                ifctx3 = aml_if(aml_equal(aml_local(1), aml_int(2)));
+                {
+                    aml_append(ifctx3,
+                               aml_store(
+                                   aml_name("PPRQ"),
+                                   aml_index(aml_name("TPM3"), aml_int(1))));
+                    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);
+
+            /* get platform-specific action to transition to pre-OS env. */
+            ifctx2 = aml_if(aml_equal(aml_local(0), aml_int(4)));
+            {
+                /* reboot */
+                aml_append(ifctx2, aml_return(aml_int(2)));
+            }
+            aml_append(ifctx, ifctx2);
+
+            /* get TPM operation response */
+            ifctx2 = aml_if(aml_equal(aml_local(0), aml_int(5)));
+            {
+                aml_append(ifctx2,
+                           aml_store(
+                               aml_name("LPPR"),
+                               aml_index(aml_name("TPM3"), aml_int(1))));
+                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);
+
+            /* submit preferred user language */
+            ifctx2 = aml_if(aml_equal(aml_local(0), aml_int(6)));
+            {
+                /* 3 = not implemented */
+                aml_append(ifctx2, aml_return(aml_int(3)));
+            }
+            aml_append(ifctx, ifctx2);
+
+            /* submit TPM operation v2 */
+            ifctx2 = aml_if(aml_equal(aml_local(0), aml_int(7)));
+            {
+                /* get opcode */
+                aml_append(ifctx2,
+                           aml_store(aml_derefof(aml_index(aml_arg(3),
+                                                           aml_int(0))),
+                                     aml_local(0)));
+                /* get opcode flags */
+                aml_append(ifctx2,
+                           aml_store(aml_call1("TPFN", aml_local(0)),
+                                     aml_local(1)));
+                ifctx3 = aml_if(
+                    aml_equal(
+                        aml_and(aml_local(1), 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);
+
+                ifctx3 = aml_if(
+                    aml_equal(
+                        aml_and(aml_local(1), 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)));
+
+                ifctx3 = aml_if(aml_equal(aml_local(1), aml_int(1)));
+                {
+                    /* revision 1 */
+                    aml_append(ifctx3, aml_store(aml_local(0),
+                                                 aml_name("PPRQ")));
+                    aml_append(ifctx3, aml_store(aml_int(0),
+                                                 aml_name("PPRM")));
+                }
+                aml_append(ifctx2, ifctx3);
+
+                ifctx3 = aml_if(aml_equal(aml_local(1), aml_int(2)));
+                {
+                    /* revision 2 */
+                    aml_append(ifctx3,
+                               aml_store(aml_local(0), aml_name("PPRQ")));
+                    aml_append(ifctx3,
+                               aml_store(
+                                   aml_derefof(aml_index(aml_arg(3),
+                                                         aml_int(1))),
+                                   aml_name("PPRM")));
+                }
+                aml_append(ifctx2, ifctx3);
+                /* 0: success */
+                aml_append(ifctx2, aml_return(aml_int(0)));
+            }
+            aml_append(ifctx, ifctx2);
+
+            /* get user confirmation status for operation */
+            ifctx2 = aml_if(aml_equal(aml_local(0), aml_int(8)));
+            {
+                /* get opcode */
+                aml_append(ifctx2,
+                           aml_store(aml_derefof(aml_index(aml_arg(3),
+                                                           aml_int(0))),
+                                     aml_local(0)));
+                /* get opcode flags */
+                aml_append(ifctx2,
+                           aml_store(aml_call1("TPFN", aml_local(0)),
+                                     aml_local(1)));
+                /* return confirmation status code */
+                aml_append(ifctx2,
+                           aml_return(
+                               aml_and(aml_local(1),
+                                       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 +2440,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 +2462,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);
     }
 
@@ -2918,7 +3210,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);
-- 
2.17.0.253.g3dd125b46d

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

[Michael S. Tsirkin]

On Tue, May 15, 2018 at 02:14:32PM +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>
> 
> ---
> 
> 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 |  21 +++
>  hw/i386/acpi-build.c  | 294 +++++++++++++++++++++++++++++++++++++++++-
>  2 files changed, 314 insertions(+), 1 deletion(-)
> 
> diff --git a/include/hw/acpi/tpm.h b/include/hw/acpi/tpm.h
> index f79d68a77a..fc53f08827 100644
> --- a/include/hw/acpi/tpm.h
> +++ b/include/hw/acpi/tpm.h
> @@ -196,4 +196,25 @@ REG32(CRB_DATA_BUFFER, 0x80)
>  #define TPM_PPI_VERSION_NONE        0
>  #define TPM_PPI_VERSION_1_30        1
>  
> +struct tpm_ppi {

The name violate the coding style.


> +    uint8_t  func[256];      /* 0x000: per TPM function implementation flags;
> +                                       set by BIOS */
> +/* 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)
> +    uint8_t ppin;            /* 0x100 : set by BIOS */

Are you sure it's right? Below ints will all end up misaligned ...

> +    uint32_t ppip;           /* 0x101 : set by ACPI; not used */
> +    uint32_t pprp;           /* 0x105 : response from TPM; set by BIOS */
> +    uint32_t pprq;           /* 0x109 : opcode; set by ACPI */
> +    uint32_t pprm;           /* 0x10d : parameter for opcode; set by ACPI */
> +    uint32_t lppr;           /* 0x111 : last opcode; set by BIOS */
> +    uint32_t fret;           /* 0x115 : set by ACPI; not used */
> +    uint8_t res1[0x40];      /* 0x119 : reserved for future use */
> +    uint8_t next_step;       /* 0x159 : next step after reboot; set by BIOS */
> +} QEMU_PACKED;
> +
>  #endif /* HW_ACPI_TPM_H */

Igor could you pls take a quick look at the rest?

-- 
MST

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

[Marc-André Lureau]

Hi

On Wed, Jun 20, 2018 at 4:08 PM, Michael S. Tsirkin <mst@redhat.com> wrote:
> On Tue, May 15, 2018 at 02:14:32PM +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>
>>
>> ---
>>
>> 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 |  21 +++
>>  hw/i386/acpi-build.c  | 294 +++++++++++++++++++++++++++++++++++++++++-
>>  2 files changed, 314 insertions(+), 1 deletion(-)
>>
>> diff --git a/include/hw/acpi/tpm.h b/include/hw/acpi/tpm.h
>> index f79d68a77a..fc53f08827 100644
>> --- a/include/hw/acpi/tpm.h
>> +++ b/include/hw/acpi/tpm.h
>> @@ -196,4 +196,25 @@ REG32(CRB_DATA_BUFFER, 0x80)
>>  #define TPM_PPI_VERSION_NONE        0
>>  #define TPM_PPI_VERSION_1_30        1
>>
>> +struct tpm_ppi {
>
> The name violate the coding style.

That's easy to change. Stefan could do it on commit if the rest of the
patch is unchanged.
>
>
>> +    uint8_t  func[256];      /* 0x000: per TPM function implementation flags;
>> +                                       set by BIOS */
>> +/* 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)
>> +    uint8_t ppin;            /* 0x100 : set by BIOS */
>
> Are you sure it's right? Below ints will all end up misaligned ...

Hmm. Sadly, we didn't noticed when doing the edk2 part either. If we
change it in qemu, we will have to change it in edk2 as well

>> +    uint32_t ppip;           /* 0x101 : set by ACPI; not used */
>> +    uint32_t pprp;           /* 0x105 : response from TPM; set by BIOS */
>> +    uint32_t pprq;           /* 0x109 : opcode; set by ACPI */
>> +    uint32_t pprm;           /* 0x10d : parameter for opcode; set by ACPI */
>> +    uint32_t lppr;           /* 0x111 : last opcode; set by BIOS */
>> +    uint32_t fret;           /* 0x115 : set by ACPI; not used */
>> +    uint8_t res1[0x40];      /* 0x119 : reserved for future use */
>> +    uint8_t next_step;       /* 0x159 : next step after reboot; set by BIOS */
>> +} QEMU_PACKED;
>> +
>>  #endif /* HW_ACPI_TPM_H */
>
> Igor could you pls take a quick look at the rest?
>
> --
> MST
>

thanks


-- 
Marc-André Lureau

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

[Laszlo Ersek]

On 06/20/18 16:35, Marc-André Lureau wrote:
> Hi
> 
> On Wed, Jun 20, 2018 at 4:08 PM, Michael S. Tsirkin <mst@redhat.com> wrote:
>> On Tue, May 15, 2018 at 02:14:32PM +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>
>>>
>>> ---
>>>
>>> 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 |  21 +++
>>>  hw/i386/acpi-build.c  | 294 +++++++++++++++++++++++++++++++++++++++++-
>>>  2 files changed, 314 insertions(+), 1 deletion(-)
>>>
>>> diff --git a/include/hw/acpi/tpm.h b/include/hw/acpi/tpm.h
>>> index f79d68a77a..fc53f08827 100644
>>> --- a/include/hw/acpi/tpm.h
>>> +++ b/include/hw/acpi/tpm.h
>>> @@ -196,4 +196,25 @@ REG32(CRB_DATA_BUFFER, 0x80)
>>>  #define TPM_PPI_VERSION_NONE        0
>>>  #define TPM_PPI_VERSION_1_30        1
>>>
>>> +struct tpm_ppi {
>>
>> The name violate the coding style.
> 
> That's easy to change. Stefan could do it on commit if the rest of the
> patch is unchanged.
>>
>>
>>> +    uint8_t  func[256];      /* 0x000: per TPM function implementation flags;
>>> +                                       set by BIOS */
>>> +/* 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)
>>> +    uint8_t ppin;            /* 0x100 : set by BIOS */
>>
>> Are you sure it's right? Below ints will all end up misaligned ...
> 
> Hmm. Sadly, we didn't noticed when doing the edk2 part either. If we
> change it in qemu, we will have to change it in edk2 as well

I don't see why the misalignment is a problem. AIUI functionally it
shouldn't be an issue, and performance is not critical.

We did make sure the struct was packed in edk2 too.

Thanks,
Laszlo

> 
>>> +    uint32_t ppip;           /* 0x101 : set by ACPI; not used */
>>> +    uint32_t pprp;           /* 0x105 : response from TPM; set by BIOS */
>>> +    uint32_t pprq;           /* 0x109 : opcode; set by ACPI */
>>> +    uint32_t pprm;           /* 0x10d : parameter for opcode; set by ACPI */
>>> +    uint32_t lppr;           /* 0x111 : last opcode; set by BIOS */
>>> +    uint32_t fret;           /* 0x115 : set by ACPI; not used */
>>> +    uint8_t res1[0x40];      /* 0x119 : reserved for future use */
>>> +    uint8_t next_step;       /* 0x159 : next step after reboot; set by BIOS */
>>> +} QEMU_PACKED;
>>> +
>>>  #endif /* HW_ACPI_TPM_H */
>>
>> Igor could you pls take a quick look at the rest?
>>
>> --
>> MST
>>
> 
> thanks
> 
> 

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

[Michael S. Tsirkin]

On Wed, Jun 20, 2018 at 04:35:23PM +0200, Marc-André Lureau wrote:
> Hi
> 
> On Wed, Jun 20, 2018 at 4:08 PM, Michael S. Tsirkin <mst@redhat.com> wrote:
> > On Tue, May 15, 2018 at 02:14:32PM +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>
> >>
> >> ---
> >>
> >> 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 |  21 +++
> >>  hw/i386/acpi-build.c  | 294 +++++++++++++++++++++++++++++++++++++++++-
> >>  2 files changed, 314 insertions(+), 1 deletion(-)
> >>
> >> diff --git a/include/hw/acpi/tpm.h b/include/hw/acpi/tpm.h
> >> index f79d68a77a..fc53f08827 100644
> >> --- a/include/hw/acpi/tpm.h
> >> +++ b/include/hw/acpi/tpm.h
> >> @@ -196,4 +196,25 @@ REG32(CRB_DATA_BUFFER, 0x80)
> >>  #define TPM_PPI_VERSION_NONE        0
> >>  #define TPM_PPI_VERSION_1_30        1
> >>
> >> +struct tpm_ppi {
> >
> > The name violate the coding style.
> 
> That's easy to change. Stefan could do it on commit if the rest of the
> patch is unchanged.
> >
> >
> >> +    uint8_t  func[256];      /* 0x000: per TPM function implementation flags;
> >> +                                       set by BIOS */
> >> +/* 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)
> >> +    uint8_t ppin;            /* 0x100 : set by BIOS */
> >
> > Are you sure it's right? Below ints will all end up misaligned ...
> 
> Hmm. Sadly, we didn't noticed when doing the edk2 part either. If we
> change it in qemu, we will have to change it in edk2 as well

Might be worth it, it's often very slow to access unaligned fields.

> >> +    uint32_t ppip;           /* 0x101 : set by ACPI; not used */
> >> +    uint32_t pprp;           /* 0x105 : response from TPM; set by BIOS */
> >> +    uint32_t pprq;           /* 0x109 : opcode; set by ACPI */
> >> +    uint32_t pprm;           /* 0x10d : parameter for opcode; set by ACPI */
> >> +    uint32_t lppr;           /* 0x111 : last opcode; set by BIOS */
> >> +    uint32_t fret;           /* 0x115 : set by ACPI; not used */
> >> +    uint8_t res1[0x40];      /* 0x119 : reserved for future use */
> >> +    uint8_t next_step;       /* 0x159 : next step after reboot; set by BIOS */
> >> +} QEMU_PACKED;
> >> +
> >>  #endif /* HW_ACPI_TPM_H */
> >
> > Igor could you pls take a quick look at the rest?
> >
> > --
> > MST
> >
> 
> thanks
> 
> 
> -- 
> Marc-André Lureau

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

[Stefan Berger]

On 06/20/2018 10:35 AM, Marc-André Lureau wrote:
> Hi
>
> On Wed, Jun 20, 2018 at 4:08 PM, Michael S. Tsirkin <mst@redhat.com> wrote:
>> On Tue, May 15, 2018 at 02:14:32PM +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>
>>>
>>> ---
>>>
>>> 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 |  21 +++
>>>   hw/i386/acpi-build.c  | 294 +++++++++++++++++++++++++++++++++++++++++-
>>>   2 files changed, 314 insertions(+), 1 deletion(-)
>>>
>>> diff --git a/include/hw/acpi/tpm.h b/include/hw/acpi/tpm.h
>>> index f79d68a77a..fc53f08827 100644
>>> --- a/include/hw/acpi/tpm.h
>>> +++ b/include/hw/acpi/tpm.h
>>> @@ -196,4 +196,25 @@ REG32(CRB_DATA_BUFFER, 0x80)
>>>   #define TPM_PPI_VERSION_NONE        0
>>>   #define TPM_PPI_VERSION_1_30        1
>>>
>>> +struct tpm_ppi {
>> The name violate the coding style.
> That's easy to change. Stefan could do it on commit if the rest of the
> patch is unchanged.

Call it TPMPPIData?

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

[Igor Mammedov]

On Tue, 15 May 2018 14:14:32 +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.
> 
> Signed-off-by: Stefan Berger <stefanb@linux.vnet.ibm.com>
> 
> ---
> 
> 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 |  21 +++
>  hw/i386/acpi-build.c  | 294 +++++++++++++++++++++++++++++++++++++++++-
>  2 files changed, 314 insertions(+), 1 deletion(-)
> 
> diff --git a/include/hw/acpi/tpm.h b/include/hw/acpi/tpm.h
> index f79d68a77a..fc53f08827 100644
> --- a/include/hw/acpi/tpm.h
> +++ b/include/hw/acpi/tpm.h
> @@ -196,4 +196,25 @@ REG32(CRB_DATA_BUFFER, 0x80)
>  #define TPM_PPI_VERSION_NONE        0
>  #define TPM_PPI_VERSION_1_30        1
>  
> +struct tpm_ppi {
> +    uint8_t  func[256];      /* 0x000: per TPM function implementation flags;
> +                                       set by BIOS */
> +/* 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)
> +    uint8_t ppin;            /* 0x100 : set by BIOS */
> +    uint32_t ppip;           /* 0x101 : set by ACPI; not used */
> +    uint32_t pprp;           /* 0x105 : response from TPM; set by BIOS */
> +    uint32_t pprq;           /* 0x109 : opcode; set by ACPI */
> +    uint32_t pprm;           /* 0x10d : parameter for opcode; set by ACPI */
> +    uint32_t lppr;           /* 0x111 : last opcode; set by BIOS */
> +    uint32_t fret;           /* 0x115 : set by ACPI; not used */
> +    uint8_t res1[0x40];      /* 0x119 : reserved for future use */
> +    uint8_t next_step;       /* 0x159 : next step after reboot; set by BIOS */
> +} QEMU_PACKED;
is this structure plant to be used for accessing data from within QEMU
or it is just used for convenience of calculating offsets/sizes for AML?


> +
>  #endif /* HW_ACPI_TPM_H */
> diff --git a/hw/i386/acpi-build.c b/hw/i386/acpi-build.c
> index f6d447f03a..95be4f0710 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,292 @@ static Aml *build_q35_osc_method(void)
>      return method;
>  }
>  
> +static void
> +build_tpm_ppi(Aml *dev)
> +{
> +    Aml *method, *name, *field, *ifctx, *ifctx2, *ifctx3, *pak;
> +    struct tpm_ppi *tpm_ppi = NULL;
> +    int i;
plain AML variables throughout the function make code rather unreadable
(well, like any other AML code would be).
Pls take a look at how build_legacy_cpu_hotplug_aml() uses intermediate
more or less sanely named variables to make it more understandable.
that also should allow to reduce LOC this function takes.

> +    /*
> +     * 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),
> +                                    sizeof(tpm_ppi->func)));
> +    field = aml_field("TPP1", AML_ANY_ACC, AML_NOLOCK, AML_PRESERVE);
> +    for (i = 0; i < sizeof(tpm_ppi->func); i++) {
> +        char *tmp = g_strdup_printf("FN%02X", i);
> +        aml_append(field, aml_named_field(tmp, BITS_PER_BYTE));
> +        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 +
> +                                            offsetof(struct tpm_ppi, ppin)),
> +                                    sizeof(struct tpm_ppi) -
> +                                        sizeof(tpm_ppi->func)));
> +    field = aml_field("TPP2", AML_ANY_ACC, AML_NOLOCK, AML_PRESERVE);
> +    aml_append(field, aml_named_field("PPIN",
> +               sizeof(uint8_t) * BITS_PER_BYTE));
here you already don't use tpm_ppi for sizing, so it creates a confusing mix
of both approaches.

From ACPI pov I'd prefer PPI table documented somewhere in spec docs 
(similar docs/specs/acpi_mem_hotplug.txt) and would look like in
other use-cases:

   aml_append(field, aml_named_field("PPIN", 8)

and drop "struct tpm_ppi" altogether replacing places it was used by
explicit constants.

> +    aml_append(field, aml_named_field("PPIP",
> +               sizeof(uint32_t) * BITS_PER_BYTE));
> +    aml_append(field, aml_named_field("PPRP",
> +               sizeof(uint32_t) * BITS_PER_BYTE));
> +    aml_append(field, aml_named_field("PPRQ",
> +               sizeof(uint32_t) * BITS_PER_BYTE));
> +    aml_append(field, aml_named_field("PPRM",
> +               sizeof(uint32_t) * BITS_PER_BYTE));
> +    aml_append(field, aml_named_field("LPPR",
> +               sizeof(uint32_t) * BITS_PER_BYTE));
> +    aml_append(dev, field);
> +
> +    method = aml_method("TPFN", 1, AML_SERIALIZED);
not quite sure how this method (supposed to ) work(s),
it could use nice comment explaining mechanics.

> +    {
> +        for (i = 0; i < sizeof(tpm_ppi->func); 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 };
> +
> +        ifctx = aml_if(
> +            aml_equal(aml_arg(0),
> +                      aml_touuid("3DDDFAA6-361B-4EB4-A424-8D10089D1653")));
a comment pointing to a specific part/version of spec that documents this _DSM
for every uuid/function used here would help to review it.

> +        {
> +            aml_append(ifctx,
> +                       aml_store(aml_to_integer(aml_arg(2)), aml_local(0)));
> +
> +            /* standard DSM query function */
> +            ifctx2 = aml_if(aml_equal(aml_local(0), aml_int(0)));
> +            {
> +                uint8_t byte_list[2] = { 0xff, 0x01 };
> +                aml_append(ifctx2, aml_return(aml_buffer(2, byte_list)));
> +            }
> +            aml_append(ifctx, ifctx2);
> +
> +            /* interface version: 1.3 */
> +            ifctx2 = aml_if(aml_equal(aml_local(0), aml_int(1)));
> +            {
> +                aml_append(ifctx2, aml_return(aml_string("1.3")));
> +            }
> +            aml_append(ifctx, ifctx2);
> +
> +            /* submit TPM operation */
> +            ifctx2 = aml_if(aml_equal(aml_local(0), aml_int(2)));
> +            {
> +                /* get opcode */
> +                aml_append(ifctx2,
> +                           aml_store(aml_derefof(aml_index(aml_arg(3),
> +                                                           aml_int(0))),
> +                                     aml_local(0)));
> +                /* get opcode flags */
> +                aml_append(ifctx2,
> +                           aml_store(aml_call1("TPFN", aml_local(0)),
> +                                     aml_local(1)));
> +                ifctx3 = aml_if(
> +                    aml_equal(
> +                        aml_and(aml_local(1), 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);
> +                aml_append(ifctx2, aml_store(aml_local(0), 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);
> +
> +            /* get pending TPM operation */
> +            ifctx2 = aml_if(aml_equal(aml_local(0), aml_int(3)));
> +            {
> +                /* revision to integer */
> +                aml_append(ifctx2,
> +                           aml_store(
> +                               aml_to_integer(aml_arg(1)),
> +                               aml_local(1)));
> +                ifctx3 = aml_if(aml_equal(aml_local(1), aml_int(1)));
> +                {
> +                    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);
> +
> +                ifctx3 = aml_if(aml_equal(aml_local(1), aml_int(2)));
> +                {
> +                    aml_append(ifctx3,
> +                               aml_store(
> +                                   aml_name("PPRQ"),
> +                                   aml_index(aml_name("TPM3"), aml_int(1))));
> +                    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);
> +
> +            /* get platform-specific action to transition to pre-OS env. */
> +            ifctx2 = aml_if(aml_equal(aml_local(0), aml_int(4)));
> +            {
> +                /* reboot */
> +                aml_append(ifctx2, aml_return(aml_int(2)));
> +            }
> +            aml_append(ifctx, ifctx2);
> +
> +            /* get TPM operation response */
> +            ifctx2 = aml_if(aml_equal(aml_local(0), aml_int(5)));
> +            {
> +                aml_append(ifctx2,
> +                           aml_store(
> +                               aml_name("LPPR"),
> +                               aml_index(aml_name("TPM3"), aml_int(1))));
> +                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);
> +
> +            /* submit preferred user language */
> +            ifctx2 = aml_if(aml_equal(aml_local(0), aml_int(6)));
> +            {
> +                /* 3 = not implemented */
> +                aml_append(ifctx2, aml_return(aml_int(3)));
> +            }
> +            aml_append(ifctx, ifctx2);
> +
> +            /* submit TPM operation v2 */
> +            ifctx2 = aml_if(aml_equal(aml_local(0), aml_int(7)));
> +            {
> +                /* get opcode */
> +                aml_append(ifctx2,
> +                           aml_store(aml_derefof(aml_index(aml_arg(3),
> +                                                           aml_int(0))),
> +                                     aml_local(0)));
> +                /* get opcode flags */
> +                aml_append(ifctx2,
> +                           aml_store(aml_call1("TPFN", aml_local(0)),
> +                                     aml_local(1)));
> +                ifctx3 = aml_if(
> +                    aml_equal(
> +                        aml_and(aml_local(1), 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);
> +
> +                ifctx3 = aml_if(
> +                    aml_equal(
> +                        aml_and(aml_local(1), 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)));
> +
> +                ifctx3 = aml_if(aml_equal(aml_local(1), aml_int(1)));
> +                {
> +                    /* revision 1 */
> +                    aml_append(ifctx3, aml_store(aml_local(0),
> +                                                 aml_name("PPRQ")));
> +                    aml_append(ifctx3, aml_store(aml_int(0),
> +                                                 aml_name("PPRM")));
> +                }
> +                aml_append(ifctx2, ifctx3);
> +
> +                ifctx3 = aml_if(aml_equal(aml_local(1), aml_int(2)));
> +                {
> +                    /* revision 2 */
> +                    aml_append(ifctx3,
> +                               aml_store(aml_local(0), aml_name("PPRQ")));
> +                    aml_append(ifctx3,
> +                               aml_store(
> +                                   aml_derefof(aml_index(aml_arg(3),
> +                                                         aml_int(1))),
> +                                   aml_name("PPRM")));
> +                }
> +                aml_append(ifctx2, ifctx3);
> +                /* 0: success */
> +                aml_append(ifctx2, aml_return(aml_int(0)));
> +            }
> +            aml_append(ifctx, ifctx2);
> +
> +            /* get user confirmation status for operation */
> +            ifctx2 = aml_if(aml_equal(aml_local(0), aml_int(8)));
> +            {
> +                /* get opcode */
> +                aml_append(ifctx2,
> +                           aml_store(aml_derefof(aml_index(aml_arg(3),
> +                                                           aml_int(0))),
> +                                     aml_local(0)));
> +                /* get opcode flags */
> +                aml_append(ifctx2,
> +                           aml_store(aml_call1("TPFN", aml_local(0)),
> +                                     aml_local(1)));
> +                /* return confirmation status code */
> +                aml_append(ifctx2,
> +                           aml_return(
> +                               aml_and(aml_local(1),
> +                                       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 +2440,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 +2462,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);
>      }
>  
> @@ -2918,7 +3210,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)
what was the point of introducing TPM_PPI_VERSION_NONE if it would not be used?
maybe patch ordering is problem? what if we put fwcfg patch after this one?

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

hopefully patch would be more review-able with comments and nicely named
variables, so I could actually review it functionality wise without reading
whole TPM spec(s)

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

[Marc-André Lureau]

Hi

On Thu, Jun 21, 2018 at 2:54 PM, Igor Mammedov <imammedo@redhat.com> wrote:
> On Tue, 15 May 2018 14:14:32 +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.
>>
>> Signed-off-by: Stefan Berger <stefanb@linux.vnet.ibm.com>
>>
>> ---
>>
>> 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 |  21 +++
>>  hw/i386/acpi-build.c  | 294 +++++++++++++++++++++++++++++++++++++++++-
>>  2 files changed, 314 insertions(+), 1 deletion(-)
>>
>> diff --git a/include/hw/acpi/tpm.h b/include/hw/acpi/tpm.h
>> index f79d68a77a..fc53f08827 100644
>> --- a/include/hw/acpi/tpm.h
>> +++ b/include/hw/acpi/tpm.h
>> @@ -196,4 +196,25 @@ REG32(CRB_DATA_BUFFER, 0x80)
>>  #define TPM_PPI_VERSION_NONE        0
>>  #define TPM_PPI_VERSION_1_30        1
>>
>> +struct tpm_ppi {
>> +    uint8_t  func[256];      /* 0x000: per TPM function implementation flags;
>> +                                       set by BIOS */
>> +/* 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)
>> +    uint8_t ppin;            /* 0x100 : set by BIOS */
>> +    uint32_t ppip;           /* 0x101 : set by ACPI; not used */
>> +    uint32_t pprp;           /* 0x105 : response from TPM; set by BIOS */
>> +    uint32_t pprq;           /* 0x109 : opcode; set by ACPI */
>> +    uint32_t pprm;           /* 0x10d : parameter for opcode; set by ACPI */
>> +    uint32_t lppr;           /* 0x111 : last opcode; set by BIOS */
>> +    uint32_t fret;           /* 0x115 : set by ACPI; not used */
>> +    uint8_t res1[0x40];      /* 0x119 : reserved for future use */
>> +    uint8_t next_step;       /* 0x159 : next step after reboot; set by BIOS */
>> +} QEMU_PACKED;
> is this structure plant to be used for accessing data from within QEMU
> or it is just used for convenience of calculating offsets/sizes for AML?
>
>

For convenience (and it can be useful for debugging).

>> +
>>  #endif /* HW_ACPI_TPM_H */
>> diff --git a/hw/i386/acpi-build.c b/hw/i386/acpi-build.c
>> index f6d447f03a..95be4f0710 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,292 @@ static Aml *build_q35_osc_method(void)
>>      return method;
>>  }
>>
>> +static void
>> +build_tpm_ppi(Aml *dev)
>> +{
>> +    Aml *method, *name, *field, *ifctx, *ifctx2, *ifctx3, *pak;
>> +    struct tpm_ppi *tpm_ppi = NULL;
>> +    int i;
> plain AML variables throughout the function make code rather unreadable
> (well, like any other AML code would be).
> Pls take a look at how build_legacy_cpu_hotplug_aml() uses intermediate
> more or less sanely named variables to make it more understandable.
> that also should allow to reduce LOC this function takes.

Sorry, I don't understand what I should take from
build_legacy_cpu_hotplug_aml() approach. Could you describe a little
bit?


>
>> +    /*
>> +     * 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),
>> +                                    sizeof(tpm_ppi->func)));
>> +    field = aml_field("TPP1", AML_ANY_ACC, AML_NOLOCK, AML_PRESERVE);
>> +    for (i = 0; i < sizeof(tpm_ppi->func); i++) {
>> +        char *tmp = g_strdup_printf("FN%02X", i);
>> +        aml_append(field, aml_named_field(tmp, BITS_PER_BYTE));
>> +        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 +
>> +                                            offsetof(struct tpm_ppi, ppin)),
>> +                                    sizeof(struct tpm_ppi) -
>> +                                        sizeof(tpm_ppi->func)));
>> +    field = aml_field("TPP2", AML_ANY_ACC, AML_NOLOCK, AML_PRESERVE);
>> +    aml_append(field, aml_named_field("PPIN",
>> +               sizeof(uint8_t) * BITS_PER_BYTE));
> here you already don't use tpm_ppi for sizing, so it creates a confusing mix
> of both approaches.

True, sizeof_field() will become handy here. I can leave a TODO?


>
> From ACPI pov I'd prefer PPI table documented somewhere in spec docs
> (similar docs/specs/acpi_mem_hotplug.txt) and would look like in
> other use-cases:
>
>    aml_append(field, aml_named_field("PPIN", 8)
>
> and drop "struct tpm_ppi" altogether replacing places it was used by
> explicit constants.

I have a slight preference for the tpm_ppi structure. But ok with
replacing it with constant. Stefan, do you agree?

>> +    aml_append(field, aml_named_field("PPIP",
>> +               sizeof(uint32_t) * BITS_PER_BYTE));
>> +    aml_append(field, aml_named_field("PPRP",
>> +               sizeof(uint32_t) * BITS_PER_BYTE));
>> +    aml_append(field, aml_named_field("PPRQ",
>> +               sizeof(uint32_t) * BITS_PER_BYTE));
>> +    aml_append(field, aml_named_field("PPRM",
>> +               sizeof(uint32_t) * BITS_PER_BYTE));
>> +    aml_append(field, aml_named_field("LPPR",
>> +               sizeof(uint32_t) * BITS_PER_BYTE));
>> +    aml_append(dev, field);
>> +
>> +    method = aml_method("TPFN", 1, AML_SERIALIZED);
> not quite sure how this method (supposed to ) work(s),
> it could use nice comment explaining mechanics.
>

Windows doesn't like DerefOf (FUNC [N]), it returned wrong values. It
took me a while to figure this out. My laptop TPM ACPI table uses the
same trick, so I assume this is a Windows acpi bug/limitation. Instead
we use a function that returns the corresponding field.

So we declare each field / array entry:
            OperationRegion (TPP1, SystemMemory, 0xFED45000, 0x0100)
            Field (TPP1, AnyAcc, NoLock, Preserve)
            {
                FN00,   8,
                FN01,   8,....

And the method to access it:

           Method (TPFN, 1, Serialized)
            {
                If ((Zero == Arg0))
                {
                    Return (FN00) /* \_SB_.TPM_.FN00 */
                }
.....



>> +    {
>> +        for (i = 0; i < sizeof(tpm_ppi->func); 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 };
>> +
>> +        ifctx = aml_if(
>> +            aml_equal(aml_arg(0),
>> +                      aml_touuid("3DDDFAA6-361B-4EB4-A424-8D10089D1653")));
> a comment pointing to a specific part/version of spec that documents this _DSM
> for every uuid/function used here would help to review it.

ok, fixing that.

The spec PDF is fairly easy to read imho:

https://trustedcomputinggroup.org/wp-content/uploads/Physical-Presence-Interface_1-30_0-52.pdf
>
>> +        {
>> +            aml_append(ifctx,
>> +                       aml_store(aml_to_integer(aml_arg(2)), aml_local(0)));
>> +
>> +            /* standard DSM query function */
>> +            ifctx2 = aml_if(aml_equal(aml_local(0), aml_int(0)));
>> +            {
>> +                uint8_t byte_list[2] = { 0xff, 0x01 };
>> +                aml_append(ifctx2, aml_return(aml_buffer(2, byte_list)));
>> +            }
>> +            aml_append(ifctx, ifctx2);
>> +
>> +            /* interface version: 1.3 */
>> +            ifctx2 = aml_if(aml_equal(aml_local(0), aml_int(1)));
>> +            {
>> +                aml_append(ifctx2, aml_return(aml_string("1.3")));
>> +            }
>> +            aml_append(ifctx, ifctx2);
>> +
>> +            /* submit TPM operation */
>> +            ifctx2 = aml_if(aml_equal(aml_local(0), aml_int(2)));
>> +            {
>> +                /* get opcode */
>> +                aml_append(ifctx2,
>> +                           aml_store(aml_derefof(aml_index(aml_arg(3),
>> +                                                           aml_int(0))),
>> +                                     aml_local(0)));
>> +                /* get opcode flags */
>> +                aml_append(ifctx2,
>> +                           aml_store(aml_call1("TPFN", aml_local(0)),
>> +                                     aml_local(1)));
>> +                ifctx3 = aml_if(
>> +                    aml_equal(
>> +                        aml_and(aml_local(1), 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);
>> +                aml_append(ifctx2, aml_store(aml_local(0), 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);
>> +
>> +            /* get pending TPM operation */
>> +            ifctx2 = aml_if(aml_equal(aml_local(0), aml_int(3)));
>> +            {
>> +                /* revision to integer */
>> +                aml_append(ifctx2,
>> +                           aml_store(
>> +                               aml_to_integer(aml_arg(1)),
>> +                               aml_local(1)));
>> +                ifctx3 = aml_if(aml_equal(aml_local(1), aml_int(1)));
>> +                {
>> +                    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);
>> +
>> +                ifctx3 = aml_if(aml_equal(aml_local(1), aml_int(2)));
>> +                {
>> +                    aml_append(ifctx3,
>> +                               aml_store(
>> +                                   aml_name("PPRQ"),
>> +                                   aml_index(aml_name("TPM3"), aml_int(1))));
>> +                    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);
>> +
>> +            /* get platform-specific action to transition to pre-OS env. */
>> +            ifctx2 = aml_if(aml_equal(aml_local(0), aml_int(4)));
>> +            {
>> +                /* reboot */
>> +                aml_append(ifctx2, aml_return(aml_int(2)));
>> +            }
>> +            aml_append(ifctx, ifctx2);
>> +
>> +            /* get TPM operation response */
>> +            ifctx2 = aml_if(aml_equal(aml_local(0), aml_int(5)));
>> +            {
>> +                aml_append(ifctx2,
>> +                           aml_store(
>> +                               aml_name("LPPR"),
>> +                               aml_index(aml_name("TPM3"), aml_int(1))));
>> +                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);
>> +
>> +            /* submit preferred user language */
>> +            ifctx2 = aml_if(aml_equal(aml_local(0), aml_int(6)));
>> +            {
>> +                /* 3 = not implemented */
>> +                aml_append(ifctx2, aml_return(aml_int(3)));
>> +            }
>> +            aml_append(ifctx, ifctx2);
>> +
>> +            /* submit TPM operation v2 */
>> +            ifctx2 = aml_if(aml_equal(aml_local(0), aml_int(7)));
>> +            {
>> +                /* get opcode */
>> +                aml_append(ifctx2,
>> +                           aml_store(aml_derefof(aml_index(aml_arg(3),
>> +                                                           aml_int(0))),
>> +                                     aml_local(0)));
>> +                /* get opcode flags */
>> +                aml_append(ifctx2,
>> +                           aml_store(aml_call1("TPFN", aml_local(0)),
>> +                                     aml_local(1)));
>> +                ifctx3 = aml_if(
>> +                    aml_equal(
>> +                        aml_and(aml_local(1), 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);
>> +
>> +                ifctx3 = aml_if(
>> +                    aml_equal(
>> +                        aml_and(aml_local(1), 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)));
>> +
>> +                ifctx3 = aml_if(aml_equal(aml_local(1), aml_int(1)));
>> +                {
>> +                    /* revision 1 */
>> +                    aml_append(ifctx3, aml_store(aml_local(0),
>> +                                                 aml_name("PPRQ")));
>> +                    aml_append(ifctx3, aml_store(aml_int(0),
>> +                                                 aml_name("PPRM")));
>> +                }
>> +                aml_append(ifctx2, ifctx3);
>> +
>> +                ifctx3 = aml_if(aml_equal(aml_local(1), aml_int(2)));
>> +                {
>> +                    /* revision 2 */
>> +                    aml_append(ifctx3,
>> +                               aml_store(aml_local(0), aml_name("PPRQ")));
>> +                    aml_append(ifctx3,
>> +                               aml_store(
>> +                                   aml_derefof(aml_index(aml_arg(3),
>> +                                                         aml_int(1))),
>> +                                   aml_name("PPRM")));
>> +                }
>> +                aml_append(ifctx2, ifctx3);
>> +                /* 0: success */
>> +                aml_append(ifctx2, aml_return(aml_int(0)));
>> +            }
>> +            aml_append(ifctx, ifctx2);
>> +
>> +            /* get user confirmation status for operation */
>> +            ifctx2 = aml_if(aml_equal(aml_local(0), aml_int(8)));
>> +            {
>> +                /* get opcode */
>> +                aml_append(ifctx2,
>> +                           aml_store(aml_derefof(aml_index(aml_arg(3),
>> +                                                           aml_int(0))),
>> +                                     aml_local(0)));
>> +                /* get opcode flags */
>> +                aml_append(ifctx2,
>> +                           aml_store(aml_call1("TPFN", aml_local(0)),
>> +                                     aml_local(1)));
>> +                /* return confirmation status code */
>> +                aml_append(ifctx2,
>> +                           aml_return(
>> +                               aml_and(aml_local(1),
>> +                                       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 +2440,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 +2462,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);
>>      }
>>
>> @@ -2918,7 +3210,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)
> what was the point of introducing TPM_PPI_VERSION_NONE if it would not be used?
> maybe patch ordering is problem? what if we put fwcfg patch after this one?

edk2 already knows that NONE (0) is no PPI. So we at least have to
keep the same indexing.

I don't see much point in reordering. If you look at v4, you may want
to squash things together too, but that makes reviewing more
complicated. As the long as the split is natural, and no regression
are introduced, I would rather keep it the way it is.

>
>> +            .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);
>
> hopefully patch would be more review-able with comments and nicely named
> variables, so I could actually review it functionality wise without reading
> whole TPM spec(s)

Let see what I can do.

Thanks!


-- 
Marc-André Lureau

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

[Marc-André Lureau]

Hi

On Thu, Jun 21, 2018 at 3:21 PM, Marc-André Lureau
<marcandre.lureau@gmail.com> wrote:

>>> +    method = aml_method("TPFN", 1, AML_SERIALIZED);
>> not quite sure how this method (supposed to ) work(s),
>> it could use nice comment explaining mechanics.
>>
>
> Windows doesn't like DerefOf (FUNC [N]), it returned wrong values. It
> took me a while to figure this out. My laptop TPM ACPI table uses the
> same trick, so I assume this is a Windows acpi bug/limitation. Instead
> we use a function that returns the corresponding field.

Sorry, I am wrong, my laptop doesn't use such table. I don't know
where I saw someone using the same trick though..


-- 
Marc-André Lureau

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

[Marc-André Lureau]

Hi

On Thu, Jun 21, 2018 at 3:22 PM, Marc-André Lureau
<marcandre.lureau@gmail.com> wrote:
> Hi
>
> On Thu, Jun 21, 2018 at 3:21 PM, Marc-André Lureau
> <marcandre.lureau@gmail.com> wrote:
>
>>>> +    method = aml_method("TPFN", 1, AML_SERIALIZED);
>>> not quite sure how this method (supposed to ) work(s),
>>> it could use nice comment explaining mechanics.
>>>
>>
>> Windows doesn't like DerefOf (FUNC [N]), it returned wrong values. It
>> took me a while to figure this out. My laptop TPM ACPI table uses the
>> same trick, so I assume this is a Windows acpi bug/limitation. Instead
>> we use a function that returns the corresponding field.
>
> Sorry, I am wrong, my laptop doesn't use such table. I don't know
> where I saw someone using the same trick though..

fwiw, I also asked on OSR list for help but got no answer:
http://www.osronline.com/showThread.CFM?link=288617





-- 
Marc-André Lureau

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

[Igor Mammedov]

On Thu, 21 Jun 2018 16:13:32 +0200
Marc-André Lureau <marcandre.lureau@gmail.com> wrote:

> Hi
> 
> On Thu, Jun 21, 2018 at 3:22 PM, Marc-André Lureau
> <marcandre.lureau@gmail.com> wrote:
> > Hi
> >
> > On Thu, Jun 21, 2018 at 3:21 PM, Marc-André Lureau
> > <marcandre.lureau@gmail.com> wrote:
> >  
> >>>> +    method = aml_method("TPFN", 1, AML_SERIALIZED);  
> >>> not quite sure how this method (supposed to ) work(s),
> >>> it could use nice comment explaining mechanics.
> >>>  
> >>
> >> Windows doesn't like DerefOf (FUNC [N]), it returned wrong values. It
> >> took me a while to figure this out. My laptop TPM ACPI table uses the
> >> same trick, so I assume this is a Windows acpi bug/limitation. Instead
> >> we use a function that returns the corresponding field.  
> >
> > Sorry, I am wrong, my laptop doesn't use such table. I don't know
> > where I saw someone using the same trick though..  
> 
> fwiw, I also asked on OSR list for help but got no answer:
> http://www.osronline.com/showThread.CFM?link=288617
it is/might be fine if there is a workaround to make Windows work,
we should mention it in comment though (including windows versions
where it doesn't work) so in future someone else (including me and you)
won't break it by accident.

> 
> 
> 
> 
> 

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

[Stefan Berger]

On 06/21/2018 09:21 AM, Marc-André Lureau wrote:
> Hi
>
> On Thu, Jun 21, 2018 at 2:54 PM, Igor Mammedov <imammedo@redhat.com> wrote:
>> On Tue, 15 May 2018 14:14:32 +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.
>>>
>>> Signed-off-by: Stefan Berger <stefanb@linux.vnet.ibm.com>
>>>
>>> ---
>>>
>>> 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
>>> ---
>>>
>>>
>>  From ACPI pov I'd prefer PPI table documented somewhere in spec docs
>> (similar docs/specs/acpi_mem_hotplug.txt) and would look like in
>> other use-cases:
>>
>>     aml_append(field, aml_named_field("PPIN", 8)
>>
>> and drop "struct tpm_ppi" altogether replacing places it was used by
>> explicit constants.
> I have a slight preference for the tpm_ppi structure. But ok with
> replacing it with constant. Stefan, do you agree?

The PPI structure will appear in the firmware code and if it is the same 
and is correctly used we know that both are in sync. So I wouldn't get 
rid of it based on just that. If we have to changed all 
sizeof(uint32_t/uint8_t) to correctly use that shared structure, I'd 
prefer that.

     Stefan

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

[Igor Mammedov]

On Thu, 21 Jun 2018 15:21:02 +0200
Marc-André Lureau <marcandre.lureau@gmail.com> wrote:

> Hi
> 
> On Thu, Jun 21, 2018 at 2:54 PM, Igor Mammedov <imammedo@redhat.com> wrote:
> > On Tue, 15 May 2018 14:14:32 +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.
> >>
> >> Signed-off-by: Stefan Berger <stefanb@linux.vnet.ibm.com>
> >>
> >> ---
> >>
> >> 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 |  21 +++
> >>  hw/i386/acpi-build.c  | 294 +++++++++++++++++++++++++++++++++++++++++-
> >>  2 files changed, 314 insertions(+), 1 deletion(-)
> >>
> >> diff --git a/include/hw/acpi/tpm.h b/include/hw/acpi/tpm.h
> >> index f79d68a77a..fc53f08827 100644
> >> --- a/include/hw/acpi/tpm.h
> >> +++ b/include/hw/acpi/tpm.h
> >> @@ -196,4 +196,25 @@ REG32(CRB_DATA_BUFFER, 0x80)
> >>  #define TPM_PPI_VERSION_NONE        0
> >>  #define TPM_PPI_VERSION_1_30        1
> >>
> >> +struct tpm_ppi {
> >> +    uint8_t  func[256];      /* 0x000: per TPM function implementation flags;
> >> +                                       set by BIOS */
> >> +/* 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)
> >> +    uint8_t ppin;            /* 0x100 : set by BIOS */
> >> +    uint32_t ppip;           /* 0x101 : set by ACPI; not used */
> >> +    uint32_t pprp;           /* 0x105 : response from TPM; set by BIOS */
> >> +    uint32_t pprq;           /* 0x109 : opcode; set by ACPI */
> >> +    uint32_t pprm;           /* 0x10d : parameter for opcode; set by ACPI */
> >> +    uint32_t lppr;           /* 0x111 : last opcode; set by BIOS */
> >> +    uint32_t fret;           /* 0x115 : set by ACPI; not used */
> >> +    uint8_t res1[0x40];      /* 0x119 : reserved for future use */
> >> +    uint8_t next_step;       /* 0x159 : next step after reboot; set by BIOS */
> >> +} QEMU_PACKED;  
> > is this structure plant to be used for accessing data from within QEMU
> > or it is just used for convenience of calculating offsets/sizes for AML?
> >
> >  
> 
> For convenience (and it can be useful for debugging).
we are gradually getting rid of usage "struct foo" in ACPI code
(i.e. when I have time to convert old struct based approach
to build_append_int() table based format).
and for new code I usually require ACPI parts be struct less
(if there is no previous struct baggage to back it up).

Hence my request to drop dummy struct and document layout
properly in related spec doc (that's where FW should look for
documentation and not into struct definition somewhere in
the header). So I'd strongly prefer it be done this way.


> >> +
> >>  #endif /* HW_ACPI_TPM_H */
> >> diff --git a/hw/i386/acpi-build.c b/hw/i386/acpi-build.c
> >> index f6d447f03a..95be4f0710 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,292 @@ static Aml *build_q35_osc_method(void)
> >>      return method;
> >>  }
> >>
> >> +static void
> >> +build_tpm_ppi(Aml *dev)
> >> +{
> >> +    Aml *method, *name, *field, *ifctx, *ifctx2, *ifctx3, *pak;
> >> +    struct tpm_ppi *tpm_ppi = NULL;
> >> +    int i;  
> > plain AML variables throughout the function make code rather unreadable
> > (well, like any other AML code would be).
> > Pls take a look at how build_legacy_cpu_hotplug_aml() uses intermediate
> > more or less sanely named variables to make it more understandable.
> > that also should allow to reduce LOC this function takes.  
> 
> Sorry, I don't understand what I should take from
> build_legacy_cpu_hotplug_aml() approach. Could you describe a little
> bit?
I was talking about something like this:

    Aml *apic_id = aml_arg(0);                                                   
    Aml *cpu_on = aml_local(0);
    ...
    Aml *cpus_map = aml_name(CPU_ON_BITMAP);
    ...
    aml_append(method,                                                           
        aml_store(aml_derefof(aml_index(cpus_map, apic_id)), cpu_on))

    ^^^ this part becomes much more read-able for reviewer/maintainer versus pure ASL

        aml_store(aml_derefof(aml_index(aml_name("CPON"), aml_arg(0))), aml_local(0)))

> >  
> >> +    /*
> >> +     * TPP1 is for the CPONflags 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),
> >> +                                    sizeof(tpm_ppi->func)));
> >> +    field = aml_field("TPP1", AML_ANY_ACC, AML_NOLOCK, AML_PRESERVE);
> >> +    for (i = 0; i < sizeof(tpm_ppi->func); i++) {
> >> +        char *tmp = g_strdup_printf("FN%02X", i);
> >> +        aml_append(field, aml_named_field(tmp, BITS_PER_BYTE));
> >> +        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 +
> >> +                                            offsetof(struct tpm_ppi, ppin)),
> >> +                                    sizeof(struct tpm_ppi) -
> >> +                                        sizeof(tpm_ppi->func)));
> >> +    field = aml_field("TPP2", AML_ANY_ACC, AML_NOLOCK, AML_PRESERVE);
> >> +    aml_append(field, aml_named_field("PPIN",
> >> +               sizeof(uint8_t) * BITS_PER_BYTE));  
> > here you already don't use tpm_ppi for sizing, so it creates a confusing mix
> > of both approaches.  
> 
> True, sizeof_field() will become handy here. I can leave a TODO?

I'd just use a constant /8/ here like the rest of the code that uses aml_named_field()
(no need to over-complicate or create another precedent to copy from)

Why TODO, changes I suggested for this patch would change it quite heavily
so why merge patch that would be changed by follow up patch almost immediately.
I'd prefer cleaner code being merged unless there are very good reasons
to merge something that should be rewritten afterwards. 

> 
> 
> >
> > From ACPI pov I'd prefer PPI table documented somewhere in spec docs
> > (similar docs/specs/acpi_mem_hotplug.txt) and would look like in
> > other use-cases:
> >
> >    aml_append(field, aml_named_field("PPIN", 8)
> >
> > and drop "struct tpm_ppi" altogether replacing places it was used by
> > explicit constants.  
> 
> I have a slight preference for the tpm_ppi structure. But ok with
> replacing it with constant. Stefan, do you agree?
> 
> >> +    aml_append(field, aml_named_field("PPIP",
> >> +               sizeof(uint32_t) * BITS_PER_BYTE));
> >> +    aml_append(field, aml_named_field("PPRP",
> >> +               sizeof(uint32_t) * BITS_PER_BYTE));
> >> +    aml_append(field, aml_named_field("PPRQ",
> >> +               sizeof(uint32_t) * BITS_PER_BYTE));
> >> +    aml_append(field, aml_named_field("PPRM",
> >> +               sizeof(uint32_t) * BITS_PER_BYTE));
> >> +    aml_append(field, aml_named_field("LPPR",
> >> +               sizeof(uint32_t) * BITS_PER_BYTE));
> >> +    aml_append(dev, field);
> >> +
> >> +    method = aml_method("TPFN", 1, AML_SERIALIZED);  
> > not quite sure how this method (supposed to ) work(s),
> > it could use nice comment explaining mechanics.
> >  
> 
> Windows doesn't like DerefOf (FUNC [N]), it returned wrong values. It
> took me a while to figure this out. My laptop TPM ACPI table uses the
> same trick, so I assume this is a Windows acpi bug/limitation. Instead
> we use a function that returns the corresponding field.
> 
> So we declare each field / array entry:
>             OperationRegion (TPP1, SystemMemory, 0xFED45000, 0x0100)
>             Field (TPP1, AnyAcc, NoLock, Preserve)
>             {
>                 FN00,   8,
>                 FN01,   8,....
> 
> And the method to access it:
> 
>            Method (TPFN, 1, Serialized)
>             {
>                 If ((Zero == Arg0))
>                 {
>                     Return (FN00) /* \_SB_.TPM_.FN00 */
>                 }
> .....
> 
> 
> 
> >> +    {
> >> +        for (i = 0; i < sizeof(tpm_ppi->func); 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 };
> >> +
> >> +        ifctx = aml_if(
> >> +            aml_equal(aml_arg(0),
> >> +                      aml_touuid("3DDDFAA6-361B-4EB4-A424-8D10089D1653")));  
> > a comment pointing to a specific part/version of spec that documents this _DSM
> > for every uuid/function used here would help to review it.  
> 
> ok, fixing that.
> 
> The spec PDF is fairly easy to read imho:
> 
> https://trustedcomputinggroup.org/wp-content/uploads/Physical-Presence-Interface_1-30_0-52.pdf
> >  
> >> +        {
> >> +            aml_append(ifctx,
> >> +                       aml_store(aml_to_integer(aml_arg(2)), aml_local(0)));
> >> +
> >> +            /* standard DSM query function */
> >> +            ifctx2 = aml_if(aml_equal(aml_local(0), aml_int(0)));
> >> +            {
> >> +                uint8_t byte_list[2] = { 0xff, 0x01 };
> >> +                aml_append(ifctx2, aml_return(aml_buffer(2, byte_list)));
> >> +            }
> >> +            aml_append(ifctx, ifctx2);
> >> +
> >> +            /* interface version: 1.3 */
> >> +            ifctx2 = aml_if(aml_equal(aml_local(0), aml_int(1)));
> >> +            {
> >> +                aml_append(ifctx2, aml_return(aml_string("1.3")));
> >> +            }
> >> +            aml_append(ifctx, ifctx2);
> >> +
> >> +            /* submit TPM operation */
> >> +            ifctx2 = aml_if(aml_equal(aml_local(0), aml_int(2)));
> >> +            {
> >> +                /* get opcode */
> >> +                aml_append(ifctx2,
> >> +                           aml_store(aml_derefof(aml_index(aml_arg(3),
> >> +                                                           aml_int(0))),
> >> +                                     aml_local(0)));
> >> +                /* get opcode flags */
> >> +                aml_append(ifctx2,
> >> +                           aml_store(aml_call1("TPFN", aml_local(0)),
> >> +                                     aml_local(1)));
> >> +                ifctx3 = aml_if(
> >> +                    aml_equal(
> >> +                        aml_and(aml_local(1), 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);
> >> +                aml_append(ifctx2, aml_store(aml_local(0), 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);
> >> +
> >> +            /* get pending TPM operation */
> >> +            ifctx2 = aml_if(aml_equal(aml_local(0), aml_int(3)));
> >> +            {
> >> +                /* revision to integer */
> >> +                aml_append(ifctx2,
> >> +                           aml_store(
> >> +                               aml_to_integer(aml_arg(1)),
> >> +                               aml_local(1)));
> >> +                ifctx3 = aml_if(aml_equal(aml_local(1), aml_int(1)));
> >> +                {
> >> +                    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);
> >> +
> >> +                ifctx3 = aml_if(aml_equal(aml_local(1), aml_int(2)));
> >> +                {
> >> +                    aml_append(ifctx3,
> >> +                               aml_store(
> >> +                                   aml_name("PPRQ"),
> >> +                                   aml_index(aml_name("TPM3"), aml_int(1))));
> >> +                    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);
> >> +
> >> +            /* get platform-specific action to transition to pre-OS env. */
> >> +            ifctx2 = aml_if(aml_equal(aml_local(0), aml_int(4)));
> >> +            {
> >> +                /* reboot */
> >> +                aml_append(ifctx2, aml_return(aml_int(2)));
> >> +            }
> >> +            aml_append(ifctx, ifctx2);
> >> +
> >> +            /* get TPM operation response */
> >> +            ifctx2 = aml_if(aml_equal(aml_local(0), aml_int(5)));
> >> +            {
> >> +                aml_append(ifctx2,
> >> +                           aml_store(
> >> +                               aml_name("LPPR"),
> >> +                               aml_index(aml_name("TPM3"), aml_int(1))));
> >> +                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);
> >> +
> >> +            /* submit preferred user language */
> >> +            ifctx2 = aml_if(aml_equal(aml_local(0), aml_int(6)));
> >> +            {
> >> +                /* 3 = not implemented */
> >> +                aml_append(ifctx2, aml_return(aml_int(3)));
> >> +            }
> >> +            aml_append(ifctx, ifctx2);
> >> +
> >> +            /* submit TPM operation v2 */
> >> +            ifctx2 = aml_if(aml_equal(aml_local(0), aml_int(7)));
> >> +            {
> >> +                /* get opcode */
> >> +                aml_append(ifctx2,
> >> +                           aml_store(aml_derefof(aml_index(aml_arg(3),
> >> +                                                           aml_int(0))),
> >> +                                     aml_local(0)));
> >> +                /* get opcode flags */
> >> +                aml_append(ifctx2,
> >> +                           aml_store(aml_call1("TPFN", aml_local(0)),
> >> +                                     aml_local(1)));
> >> +                ifctx3 = aml_if(
> >> +                    aml_equal(
> >> +                        aml_and(aml_local(1), 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);
> >> +
> >> +                ifctx3 = aml_if(
> >> +                    aml_equal(
> >> +                        aml_and(aml_local(1), 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)));
> >> +
> >> +                ifctx3 = aml_if(aml_equal(aml_local(1), aml_int(1)));
> >> +                {
> >> +                    /* revision 1 */
> >> +                    aml_append(ifctx3, aml_store(aml_local(0),
> >> +                                                 aml_name("PPRQ")));
> >> +                    aml_append(ifctx3, aml_store(aml_int(0),
> >> +                                                 aml_name("PPRM")));
> >> +                }
> >> +                aml_append(ifctx2, ifctx3);
> >> +
> >> +                ifctx3 = aml_if(aml_equal(aml_local(1), aml_int(2)));
> >> +                {
> >> +                    /* revision 2 */
> >> +                    aml_append(ifctx3,
> >> +                               aml_store(aml_local(0), aml_name("PPRQ")));
> >> +                    aml_append(ifctx3,
> >> +                               aml_store(
> >> +                                   aml_derefof(aml_index(aml_arg(3),
> >> +                                                         aml_int(1))),
> >> +                                   aml_name("PPRM")));
> >> +                }
> >> +                aml_append(ifctx2, ifctx3);
> >> +                /* 0: success */
> >> +                aml_append(ifctx2, aml_return(aml_int(0)));
> >> +            }
> >> +            aml_append(ifctx, ifctx2);
> >> +
> >> +            /* get user confirmation status for operation */
> >> +            ifctx2 = aml_if(aml_equal(aml_local(0), aml_int(8)));
> >> +            {
> >> +                /* get opcode */
> >> +                aml_append(ifctx2,
> >> +                           aml_store(aml_derefof(aml_index(aml_arg(3),
> >> +                                                           aml_int(0))),
> >> +                                     aml_local(0)));
> >> +                /* get opcode flags */
> >> +                aml_append(ifctx2,
> >> +                           aml_store(aml_call1("TPFN", aml_local(0)),
> >> +                                     aml_local(1)));
> >> +                /* return confirmation status code */
> >> +                aml_append(ifctx2,
> >> +                           aml_return(
> >> +                               aml_and(aml_local(1),
> >> +                                       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 +2440,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 +2462,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);
> >>      }
> >>
> >> @@ -2918,7 +3210,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)  
> > what was the point of introducing TPM_PPI_VERSION_NONE if it would not be used?
> > maybe patch ordering is problem? what if we put fwcfg patch after this one?  
> 
> edk2 already knows that NONE (0) is no PPI. So we at least have to
> keep the same indexing.
> 
> I don't see much point in reordering. If you look at v4, you may want
> to squash things together too, but that makes reviewing more
> complicated. As the long as the split is natural, and no regression
> are introduced, I would rather keep it the way it is.
> 
> >  
> >> +            .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);  
> >
> > hopefully patch would be more review-able with comments and nicely named
> > variables, so I could actually review it functionality wise without reading
> > whole TPM spec(s)  
> 
> Let see what I can do.
> 
> Thanks!
> 
> 

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

[Marc-André Lureau]

Hi

On Thu, Jun 21, 2018 at 4:23 PM, Igor Mammedov <imammedo@redhat.com> wrote:
> On Thu, 21 Jun 2018 15:21:02 +0200
> Marc-André Lureau <marcandre.lureau@gmail.com> wrote:
>
>> Hi
>>
>> On Thu, Jun 21, 2018 at 2:54 PM, Igor Mammedov <imammedo@redhat.com> wrote:
>> > On Tue, 15 May 2018 14:14:32 +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.
>> >>
>> >> Signed-off-by: Stefan Berger <stefanb@linux.vnet.ibm.com>
>> >>
>> >> ---
>> >>
>> >> 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 |  21 +++
>> >>  hw/i386/acpi-build.c  | 294 +++++++++++++++++++++++++++++++++++++++++-
>> >>  2 files changed, 314 insertions(+), 1 deletion(-)
>> >>
>> >> diff --git a/include/hw/acpi/tpm.h b/include/hw/acpi/tpm.h
>> >> index f79d68a77a..fc53f08827 100644
>> >> --- a/include/hw/acpi/tpm.h
>> >> +++ b/include/hw/acpi/tpm.h
>> >> @@ -196,4 +196,25 @@ REG32(CRB_DATA_BUFFER, 0x80)
>> >>  #define TPM_PPI_VERSION_NONE        0
>> >>  #define TPM_PPI_VERSION_1_30        1
>> >>
>> >> +struct tpm_ppi {
>> >> +    uint8_t  func[256];      /* 0x000: per TPM function implementation flags;
>> >> +                                       set by BIOS */
>> >> +/* 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)
>> >> +    uint8_t ppin;            /* 0x100 : set by BIOS */
>> >> +    uint32_t ppip;           /* 0x101 : set by ACPI; not used */
>> >> +    uint32_t pprp;           /* 0x105 : response from TPM; set by BIOS */
>> >> +    uint32_t pprq;           /* 0x109 : opcode; set by ACPI */
>> >> +    uint32_t pprm;           /* 0x10d : parameter for opcode; set by ACPI */
>> >> +    uint32_t lppr;           /* 0x111 : last opcode; set by BIOS */
>> >> +    uint32_t fret;           /* 0x115 : set by ACPI; not used */
>> >> +    uint8_t res1[0x40];      /* 0x119 : reserved for future use */
>> >> +    uint8_t next_step;       /* 0x159 : next step after reboot; set by BIOS */
>> >> +} QEMU_PACKED;
>> > is this structure plant to be used for accessing data from within QEMU
>> > or it is just used for convenience of calculating offsets/sizes for AML?
>> >
>> >
>>
>> For convenience (and it can be useful for debugging).
> we are gradually getting rid of usage "struct foo" in ACPI code
> (i.e. when I have time to convert old struct based approach
> to build_append_int() table based format).
> and for new code I usually require ACPI parts be struct less
> (if there is no previous struct baggage to back it up).
>

The tpm_ppi struct is not an ACPI table, it's a fixed memory region /
layout to exchange between guest/os and firmware.

> Hence my request to drop dummy struct and document layout
> properly in related spec doc (that's where FW should look for
> documentation and not into struct definition somewhere in
> the header). So I'd strongly prefer it be done this way.

There is not much more than the field description for me ;). Stefan,
would you like to develop the structure usage?

>
>> >> +
>> >>  #endif /* HW_ACPI_TPM_H */
>> >> diff --git a/hw/i386/acpi-build.c b/hw/i386/acpi-build.c
>> >> index f6d447f03a..95be4f0710 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,292 @@ static Aml *build_q35_osc_method(void)
>> >>      return method;
>> >>  }
>> >>
>> >> +static void
>> >> +build_tpm_ppi(Aml *dev)
>> >> +{
>> >> +    Aml *method, *name, *field, *ifctx, *ifctx2, *ifctx3, *pak;
>> >> +    struct tpm_ppi *tpm_ppi = NULL;
>> >> +    int i;
>> > plain AML variables throughout the function make code rather unreadable
>> > (well, like any other AML code would be).
>> > Pls take a look at how build_legacy_cpu_hotplug_aml() uses intermediate
>> > more or less sanely named variables to make it more understandable.
>> > that also should allow to reduce LOC this function takes.
>>
>> Sorry, I don't understand what I should take from
>> build_legacy_cpu_hotplug_aml() approach. Could you describe a little
>> bit?
> I was talking about something like this:
>
>     Aml *apic_id = aml_arg(0);
>     Aml *cpu_on = aml_local(0);
>     ...
>     Aml *cpus_map = aml_name(CPU_ON_BITMAP);
>     ...
>     aml_append(method,
>         aml_store(aml_derefof(aml_index(cpus_map, apic_id)), cpu_on))
>
>     ^^^ this part becomes much more read-able for reviewer/maintainer versus pure ASL
>
>         aml_store(aml_derefof(aml_index(aml_name("CPON"), aml_arg(0))), aml_local(0)))

Ok

>
>> >
>> >> +    /*
>> >> +     * TPP1 is for the CPONflags 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),
>> >> +                                    sizeof(tpm_ppi->func)));
>> >> +    field = aml_field("TPP1", AML_ANY_ACC, AML_NOLOCK, AML_PRESERVE);
>> >> +    for (i = 0; i < sizeof(tpm_ppi->func); i++) {
>> >> +        char *tmp = g_strdup_printf("FN%02X", i);
>> >> +        aml_append(field, aml_named_field(tmp, BITS_PER_BYTE));
>> >> +        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 +
>> >> +                                            offsetof(struct tpm_ppi, ppin)),
>> >> +                                    sizeof(struct tpm_ppi) -
>> >> +                                        sizeof(tpm_ppi->func)));
>> >> +    field = aml_field("TPP2", AML_ANY_ACC, AML_NOLOCK, AML_PRESERVE);
>> >> +    aml_append(field, aml_named_field("PPIN",
>> >> +               sizeof(uint8_t) * BITS_PER_BYTE));
>> > here you already don't use tpm_ppi for sizing, so it creates a confusing mix
>> > of both approaches.
>>
>> True, sizeof_field() will become handy here. I can leave a TODO?
>
> I'd just use a constant /8/ here like the rest of the code that uses aml_named_field()
> (no need to over-complicate or create another precedent to copy from)

ok

>
> Why TODO, changes I suggested for this patch would change it quite heavily
> so why merge patch that would be changed by follow up patch almost immediately.
> I'd prefer cleaner code being merged unless there are very good reasons
> to merge something that should be rewritten afterwards.

I thought we should use the upcoming sizeof_field() here. In this
case, TODO would make sense to avoid waiting for the other series.

>
>>
>>
>> >
>> > From ACPI pov I'd prefer PPI table documented somewhere in spec docs
>> > (similar docs/specs/acpi_mem_hotplug.txt) and would look like in
>> > other use-cases:
>> >
>> >    aml_append(field, aml_named_field("PPIN", 8)
>> >
>> > and drop "struct tpm_ppi" altogether replacing places it was used by
>> > explicit constants.
>>
>> I have a slight preference for the tpm_ppi structure. But ok with
>> replacing it with constant. Stefan, do you agree?
>>
>> >> +    aml_append(field, aml_named_field("PPIP",
>> >> +               sizeof(uint32_t) * BITS_PER_BYTE));
>> >> +    aml_append(field, aml_named_field("PPRP",
>> >> +               sizeof(uint32_t) * BITS_PER_BYTE));
>> >> +    aml_append(field, aml_named_field("PPRQ",
>> >> +               sizeof(uint32_t) * BITS_PER_BYTE));
>> >> +    aml_append(field, aml_named_field("PPRM",
>> >> +               sizeof(uint32_t) * BITS_PER_BYTE));
>> >> +    aml_append(field, aml_named_field("LPPR",
>> >> +               sizeof(uint32_t) * BITS_PER_BYTE));
>> >> +    aml_append(dev, field);
>> >> +
>> >> +    method = aml_method("TPFN", 1, AML_SERIALIZED);
>> > not quite sure how this method (supposed to ) work(s),
>> > it could use nice comment explaining mechanics.
>> >
>>
>> Windows doesn't like DerefOf (FUNC [N]), it returned wrong values. It
>> took me a while to figure this out. My laptop TPM ACPI table uses the
>> same trick, so I assume this is a Windows acpi bug/limitation. Instead
>> we use a function that returns the corresponding field.
>>
>> So we declare each field / array entry:
>>             OperationRegion (TPP1, SystemMemory, 0xFED45000, 0x0100)
>>             Field (TPP1, AnyAcc, NoLock, Preserve)
>>             {
>>                 FN00,   8,
>>                 FN01,   8,....
>>
>> And the method to access it:
>>
>>            Method (TPFN, 1, Serialized)
>>             {
>>                 If ((Zero == Arg0))
>>                 {
>>                     Return (FN00) /* \_SB_.TPM_.FN00 */
>>                 }
>> .....
>>
>>
>>
>> >> +    {
>> >> +        for (i = 0; i < sizeof(tpm_ppi->func); 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 };
>> >> +
>> >> +        ifctx = aml_if(
>> >> +            aml_equal(aml_arg(0),
>> >> +                      aml_touuid("3DDDFAA6-361B-4EB4-A424-8D10089D1653")));
>> > a comment pointing to a specific part/version of spec that documents this _DSM
>> > for every uuid/function used here would help to review it.
>>
>> ok, fixing that.
>>
>> The spec PDF is fairly easy to read imho:
>>
>> https://trustedcomputinggroup.org/wp-content/uploads/Physical-Presence-Interface_1-30_0-52.pdf
>> >
>> >> +        {
>> >> +            aml_append(ifctx,
>> >> +                       aml_store(aml_to_integer(aml_arg(2)), aml_local(0)));
>> >> +
>> >> +            /* standard DSM query function */
>> >> +            ifctx2 = aml_if(aml_equal(aml_local(0), aml_int(0)));
>> >> +            {
>> >> +                uint8_t byte_list[2] = { 0xff, 0x01 };
>> >> +                aml_append(ifctx2, aml_return(aml_buffer(2, byte_list)));
>> >> +            }
>> >> +            aml_append(ifctx, ifctx2);
>> >> +
>> >> +            /* interface version: 1.3 */
>> >> +            ifctx2 = aml_if(aml_equal(aml_local(0), aml_int(1)));
>> >> +            {
>> >> +                aml_append(ifctx2, aml_return(aml_string("1.3")));
>> >> +            }
>> >> +            aml_append(ifctx, ifctx2);
>> >> +
>> >> +            /* submit TPM operation */
>> >> +            ifctx2 = aml_if(aml_equal(aml_local(0), aml_int(2)));
>> >> +            {
>> >> +                /* get opcode */
>> >> +                aml_append(ifctx2,
>> >> +                           aml_store(aml_derefof(aml_index(aml_arg(3),
>> >> +                                                           aml_int(0))),
>> >> +                                     aml_local(0)));
>> >> +                /* get opcode flags */
>> >> +                aml_append(ifctx2,
>> >> +                           aml_store(aml_call1("TPFN", aml_local(0)),
>> >> +                                     aml_local(1)));
>> >> +                ifctx3 = aml_if(
>> >> +                    aml_equal(
>> >> +                        aml_and(aml_local(1), 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);
>> >> +                aml_append(ifctx2, aml_store(aml_local(0), 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);
>> >> +
>> >> +            /* get pending TPM operation */
>> >> +            ifctx2 = aml_if(aml_equal(aml_local(0), aml_int(3)));
>> >> +            {
>> >> +                /* revision to integer */
>> >> +                aml_append(ifctx2,
>> >> +                           aml_store(
>> >> +                               aml_to_integer(aml_arg(1)),
>> >> +                               aml_local(1)));
>> >> +                ifctx3 = aml_if(aml_equal(aml_local(1), aml_int(1)));
>> >> +                {
>> >> +                    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);
>> >> +
>> >> +                ifctx3 = aml_if(aml_equal(aml_local(1), aml_int(2)));
>> >> +                {
>> >> +                    aml_append(ifctx3,
>> >> +                               aml_store(
>> >> +                                   aml_name("PPRQ"),
>> >> +                                   aml_index(aml_name("TPM3"), aml_int(1))));
>> >> +                    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);
>> >> +
>> >> +            /* get platform-specific action to transition to pre-OS env. */
>> >> +            ifctx2 = aml_if(aml_equal(aml_local(0), aml_int(4)));
>> >> +            {
>> >> +                /* reboot */
>> >> +                aml_append(ifctx2, aml_return(aml_int(2)));
>> >> +            }
>> >> +            aml_append(ifctx, ifctx2);
>> >> +
>> >> +            /* get TPM operation response */
>> >> +            ifctx2 = aml_if(aml_equal(aml_local(0), aml_int(5)));
>> >> +            {
>> >> +                aml_append(ifctx2,
>> >> +                           aml_store(
>> >> +                               aml_name("LPPR"),
>> >> +                               aml_index(aml_name("TPM3"), aml_int(1))));
>> >> +                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);
>> >> +
>> >> +            /* submit preferred user language */
>> >> +            ifctx2 = aml_if(aml_equal(aml_local(0), aml_int(6)));
>> >> +            {
>> >> +                /* 3 = not implemented */
>> >> +                aml_append(ifctx2, aml_return(aml_int(3)));
>> >> +            }
>> >> +            aml_append(ifctx, ifctx2);
>> >> +
>> >> +            /* submit TPM operation v2 */
>> >> +            ifctx2 = aml_if(aml_equal(aml_local(0), aml_int(7)));
>> >> +            {
>> >> +                /* get opcode */
>> >> +                aml_append(ifctx2,
>> >> +                           aml_store(aml_derefof(aml_index(aml_arg(3),
>> >> +                                                           aml_int(0))),
>> >> +                                     aml_local(0)));
>> >> +                /* get opcode flags */
>> >> +                aml_append(ifctx2,
>> >> +                           aml_store(aml_call1("TPFN", aml_local(0)),
>> >> +                                     aml_local(1)));
>> >> +                ifctx3 = aml_if(
>> >> +                    aml_equal(
>> >> +                        aml_and(aml_local(1), 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);
>> >> +
>> >> +                ifctx3 = aml_if(
>> >> +                    aml_equal(
>> >> +                        aml_and(aml_local(1), 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)));
>> >> +
>> >> +                ifctx3 = aml_if(aml_equal(aml_local(1), aml_int(1)));
>> >> +                {
>> >> +                    /* revision 1 */
>> >> +                    aml_append(ifctx3, aml_store(aml_local(0),
>> >> +                                                 aml_name("PPRQ")));
>> >> +                    aml_append(ifctx3, aml_store(aml_int(0),
>> >> +                                                 aml_name("PPRM")));
>> >> +                }
>> >> +                aml_append(ifctx2, ifctx3);
>> >> +
>> >> +                ifctx3 = aml_if(aml_equal(aml_local(1), aml_int(2)));
>> >> +                {
>> >> +                    /* revision 2 */
>> >> +                    aml_append(ifctx3,
>> >> +                               aml_store(aml_local(0), aml_name("PPRQ")));
>> >> +                    aml_append(ifctx3,
>> >> +                               aml_store(
>> >> +                                   aml_derefof(aml_index(aml_arg(3),
>> >> +                                                         aml_int(1))),
>> >> +                                   aml_name("PPRM")));
>> >> +                }
>> >> +                aml_append(ifctx2, ifctx3);
>> >> +                /* 0: success */
>> >> +                aml_append(ifctx2, aml_return(aml_int(0)));
>> >> +            }
>> >> +            aml_append(ifctx, ifctx2);
>> >> +
>> >> +            /* get user confirmation status for operation */
>> >> +            ifctx2 = aml_if(aml_equal(aml_local(0), aml_int(8)));
>> >> +            {
>> >> +                /* get opcode */
>> >> +                aml_append(ifctx2,
>> >> +                           aml_store(aml_derefof(aml_index(aml_arg(3),
>> >> +                                                           aml_int(0))),
>> >> +                                     aml_local(0)));
>> >> +                /* get opcode flags */
>> >> +                aml_append(ifctx2,
>> >> +                           aml_store(aml_call1("TPFN", aml_local(0)),
>> >> +                                     aml_local(1)));
>> >> +                /* return confirmation status code */
>> >> +                aml_append(ifctx2,
>> >> +                           aml_return(
>> >> +                               aml_and(aml_local(1),
>> >> +                                       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 +2440,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 +2462,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);
>> >>      }
>> >>
>> >> @@ -2918,7 +3210,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)
>> > what was the point of introducing TPM_PPI_VERSION_NONE if it would not be used?
>> > maybe patch ordering is problem? what if we put fwcfg patch after this one?
>>
>> edk2 already knows that NONE (0) is no PPI. So we at least have to
>> keep the same indexing.
>>
>> I don't see much point in reordering. If you look at v4, you may want
>> to squash things together too, but that makes reviewing more
>> complicated. As the long as the split is natural, and no regression
>> are introduced, I would rather keep it the way it is.
>>
>> >
>> >> +            .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);
>> >
>> > hopefully patch would be more review-able with comments and nicely named
>> > variables, so I could actually review it functionality wise without reading
>> > whole TPM spec(s)
>>
>> Let see what I can do.
>>
>> Thanks!
>>
>>
>



-- 
Marc-André Lureau

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

[Stefan Berger]

On 06/21/2018 01:10 PM, Marc-André Lureau wrote:
> Hi
>
> On Thu, Jun 21, 2018 at 4:23 PM, Igor Mammedov <imammedo@redhat.com> wrote:
>> On Thu, 21 Jun 2018 15:21:02 +0200
>> Marc-André Lureau <marcandre.lureau@gmail.com> wrote:
>>
>>> Hi
>>>
>>> On Thu, Jun 21, 2018 at 2:54 PM, Igor Mammedov <imammedo@redhat.com> wrote:
>>>> On Tue, 15 May 2018 14:14:32 +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.
>>>>>
>>>>> Signed-off-by: Stefan Berger <stefanb@linux.vnet.ibm.com>
>>>>>
>>>>> ---
>>>>>
>>>>> 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 |  21 +++
>>>>>   hw/i386/acpi-build.c  | 294 +++++++++++++++++++++++++++++++++++++++++-
>>>>>   2 files changed, 314 insertions(+), 1 deletion(-)
>>>>>
>>>>> diff --git a/include/hw/acpi/tpm.h b/include/hw/acpi/tpm.h
>>>>> index f79d68a77a..fc53f08827 100644
>>>>> --- a/include/hw/acpi/tpm.h
>>>>> +++ b/include/hw/acpi/tpm.h
>>>>> @@ -196,4 +196,25 @@ REG32(CRB_DATA_BUFFER, 0x80)
>>>>>   #define TPM_PPI_VERSION_NONE        0
>>>>>   #define TPM_PPI_VERSION_1_30        1
>>>>>
>>>>> +struct tpm_ppi {
>>>>> +    uint8_t  func[256];      /* 0x000: per TPM function implementation flags;
>>>>> +                                       set by BIOS */
>>>>> +/* 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)
>>>>> +    uint8_t ppin;            /* 0x100 : set by BIOS */
>>>>> +    uint32_t ppip;           /* 0x101 : set by ACPI; not used */
>>>>> +    uint32_t pprp;           /* 0x105 : response from TPM; set by BIOS */
>>>>> +    uint32_t pprq;           /* 0x109 : opcode; set by ACPI */
>>>>> +    uint32_t pprm;           /* 0x10d : parameter for opcode; set by ACPI */
>>>>> +    uint32_t lppr;           /* 0x111 : last opcode; set by BIOS */
>>>>> +    uint32_t fret;           /* 0x115 : set by ACPI; not used */
>>>>> +    uint8_t res1[0x40];      /* 0x119 : reserved for future use */
>>>>> +    uint8_t next_step;       /* 0x159 : next step after reboot; set by BIOS */
>>>>> +} QEMU_PACKED;
>>>> is this structure plant to be used for accessing data from within QEMU
>>>> or it is just used for convenience of calculating offsets/sizes for AML?
>>>>
>>>>
>>> For convenience (and it can be useful for debugging).
>> we are gradually getting rid of usage "struct foo" in ACPI code
>> (i.e. when I have time to convert old struct based approach
>> to build_append_int() table based format).
>> and for new code I usually require ACPI parts be struct less
>> (if there is no previous struct baggage to back it up).
>>
> The tpm_ppi struct is not an ACPI table, it's a fixed memory region /
> layout to exchange between guest/os and firmware.
>
>> Hence my request to drop dummy struct and document layout
>> properly in related spec doc (that's where FW should look for
>> documentation and not into struct definition somewhere in
>> the header). So I'd strongly prefer it be done this way.
> There is not much more than the field description for me ;). Stefan,
> would you like to develop the structure usage?

Will do.

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

[Igor Mammedov]

On Thu, 21 Jun 2018 19:10:38 +0200
Marc-André Lureau <marcandre.lureau@gmail.com> wrote:

> Hi
> 
> On Thu, Jun 21, 2018 at 4:23 PM, Igor Mammedov <imammedo@redhat.com> wrote:
> > On Thu, 21 Jun 2018 15:21:02 +0200
> > Marc-André Lureau <marcandre.lureau@gmail.com> wrote:
> >  
> >> Hi
> >>
> >> On Thu, Jun 21, 2018 at 2:54 PM, Igor Mammedov <imammedo@redhat.com> wrote:  
> >> > On Tue, 15 May 2018 14:14:32 +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.
> >> >>
> >> >> Signed-off-by: Stefan Berger <stefanb@linux.vnet.ibm.com>
> >> >>
> >> >> ---
> >> >>
> >> >> 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 |  21 +++
> >> >>  hw/i386/acpi-build.c  | 294 +++++++++++++++++++++++++++++++++++++++++-
> >> >>  2 files changed, 314 insertions(+), 1 deletion(-)
> >> >>
> >> >> diff --git a/include/hw/acpi/tpm.h b/include/hw/acpi/tpm.h
> >> >> index f79d68a77a..fc53f08827 100644
> >> >> --- a/include/hw/acpi/tpm.h
> >> >> +++ b/include/hw/acpi/tpm.h
> >> >> @@ -196,4 +196,25 @@ REG32(CRB_DATA_BUFFER, 0x80)
> >> >>  #define TPM_PPI_VERSION_NONE        0
> >> >>  #define TPM_PPI_VERSION_1_30        1
> >> >>
> >> >> +struct tpm_ppi {
> >> >> +    uint8_t  func[256];      /* 0x000: per TPM function implementation flags;
> >> >> +                                       set by BIOS */
> >> >> +/* 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)
> >> >> +    uint8_t ppin;            /* 0x100 : set by BIOS */
> >> >> +    uint32_t ppip;           /* 0x101 : set by ACPI; not used */
> >> >> +    uint32_t pprp;           /* 0x105 : response from TPM; set by BIOS */
> >> >> +    uint32_t pprq;           /* 0x109 : opcode; set by ACPI */
> >> >> +    uint32_t pprm;           /* 0x10d : parameter for opcode; set by ACPI */
> >> >> +    uint32_t lppr;           /* 0x111 : last opcode; set by BIOS */
> >> >> +    uint32_t fret;           /* 0x115 : set by ACPI; not used */
> >> >> +    uint8_t res1[0x40];      /* 0x119 : reserved for future use */
> >> >> +    uint8_t next_step;       /* 0x159 : next step after reboot; set by BIOS */
> >> >> +} QEMU_PACKED;  
> >> > is this structure plant to be used for accessing data from within QEMU
> >> > or it is just used for convenience of calculating offsets/sizes for AML?
> >> >
> >> >  
> >>
> >> For convenience (and it can be useful for debugging).  
> > we are gradually getting rid of usage "struct foo" in ACPI code
> > (i.e. when I have time to convert old struct based approach
> > to build_append_int() table based format).
> > and for new code I usually require ACPI parts be struct less
> > (if there is no previous struct baggage to back it up).
> >  
> 
> The tpm_ppi struct is not an ACPI table, it's a fixed memory region /
> layout to exchange between guest/os and firmware.
it's interface between ACPI and firmware and it's used in ACPI
parts of code (mainly as means to document layout). I wouldn't care
much if it were buried inside of TPM code, but it's used mainly as
dummy struct for documenting and by ACPI parts of code for an implicit
sizing math, for that I'd like you to use spec/constants like any
other new ACPI code does.
So lets keep things consistent here at least for ACPI parts that
I maintain.

 
> > Hence my request to drop dummy struct and document layout
> > properly in related spec doc (that's where FW should look for
> > documentation and not into struct definition somewhere in
> > the header). So I'd strongly prefer it be done this way.  
> 
> There is not much more than the field description for me ;). Stefan,
> would you like to develop the structure usage?
You should use table format to document ACPI interface in spec
(ex: docs/specs/acpi_mem_hotplug.txt) at least for consistence sake.
I don't care that firmware side uses C code as documentation as
it's up whatever maintainers prefer over there.
 
[...]

[Qemu-devel] [PATCH v3 4/4] tpm: add a fake ACPI memory clear interface

[Marc-André Lureau]

This allows to pass the last failing test from the Windows HLK TPM 2.0
TCG PPI 1.3 tests.

The interface is described in the "TCG Platform Reset Attack
Mitigation Specification", chapter 6 "ACPI _DSM Function". Whether or
not we should have a real implementation remains an open question to me.

Signed-off-by: Marc-André Lureau <marcandre.lureau@redhat.com>
---
 hw/i386/acpi-build.c | 9 +++++++++
 1 file changed, 9 insertions(+)

diff --git a/hw/i386/acpi-build.c b/hw/i386/acpi-build.c
index 95be4f0710..392a1e50bd 100644
--- a/hw/i386/acpi-build.c
+++ b/hw/i386/acpi-build.c
@@ -2072,6 +2072,15 @@ build_tpm_ppi(Aml *dev)
             aml_append(ifctx, aml_return(aml_buffer(1, zerobyte)));
         }
         aml_append(method, ifctx);
+
+       /* dummy MOR Memory Clear for the sake of WLK PPI test */
+        ifctx = aml_if(
+            aml_equal(aml_arg(0),
+                      aml_touuid("376054ED-CC13-4675-901C-4756D7F2D45D")));
+        {
+            aml_append(ifctx, aml_return(aml_int(0)));
+        }
+        aml_append(method, ifctx);
     }
     aml_append(dev, method);
 }
-- 
2.17.0.253.g3dd125b46d

Re: [Qemu-devel] [PATCH v3 4/4] tpm: add a fake ACPI memory clear interface

[Igor Mammedov]

On Tue, 15 May 2018 14:14:33 +0200
Marc-André Lureau <marcandre.lureau@redhat.com> wrote:

> This allows to pass the last failing test from the Windows HLK TPM 2.0
> TCG PPI 1.3 tests.
> 
> The interface is described in the "TCG Platform Reset Attack
> Mitigation Specification", chapter 6 "ACPI _DSM Function". Whether or
> not we should have a real implementation remains an open question to me.
might it cause security issues?
What are implications of faking it and how hard it's to implement thing
per spec?

 
> Signed-off-by: Marc-André Lureau <marcandre.lureau@redhat.com>
> ---
>  hw/i386/acpi-build.c | 9 +++++++++
>  1 file changed, 9 insertions(+)
> 
> diff --git a/hw/i386/acpi-build.c b/hw/i386/acpi-build.c
> index 95be4f0710..392a1e50bd 100644
> --- a/hw/i386/acpi-build.c
> +++ b/hw/i386/acpi-build.c
> @@ -2072,6 +2072,15 @@ build_tpm_ppi(Aml *dev)
>              aml_append(ifctx, aml_return(aml_buffer(1, zerobyte)));
>          }
>          aml_append(method, ifctx);
> +
> +       /* dummy MOR Memory Clear for the sake of WLK PPI test */
> +        ifctx = aml_if(
> +            aml_equal(aml_arg(0),
> +                      aml_touuid("376054ED-CC13-4675-901C-4756D7F2D45D")));
> +        {
> +            aml_append(ifctx, aml_return(aml_int(0)));
> +        }
> +        aml_append(method, ifctx);
>      }
>      aml_append(dev, method);
>  }

Re: [Qemu-devel] [PATCH v3 4/4] tpm: add a fake ACPI memory clear interface

[Marc-André Lureau]

Hi

On Thu, Jun 21, 2018 at 3:02 PM, Igor Mammedov <imammedo@redhat.com> wrote:
> On Tue, 15 May 2018 14:14:33 +0200
> Marc-André Lureau <marcandre.lureau@redhat.com> wrote:
>
>> This allows to pass the last failing test from the Windows HLK TPM 2.0
>> TCG PPI 1.3 tests.
>>
>> The interface is described in the "TCG Platform Reset Attack
>> Mitigation Specification", chapter 6 "ACPI _DSM Function". Whether or
>> not we should have a real implementation remains an open question to me.
> might it cause security issues?

Good question. If the guest assumes success of this operation perhaps.
I'll check the spec.

> What are implications of faking it and how hard it's to implement thing
> per spec?

Laszlo answerd that in "[Qemu-devel] investigating TPM for
OVMF-on-QEMU"  2f2b) TCG Memory Clear Interface

>
>
>> Signed-off-by: Marc-André Lureau <marcandre.lureau@redhat.com>
>> ---
>>  hw/i386/acpi-build.c | 9 +++++++++
>>  1 file changed, 9 insertions(+)
>>
>> diff --git a/hw/i386/acpi-build.c b/hw/i386/acpi-build.c
>> index 95be4f0710..392a1e50bd 100644
>> --- a/hw/i386/acpi-build.c
>> +++ b/hw/i386/acpi-build.c
>> @@ -2072,6 +2072,15 @@ build_tpm_ppi(Aml *dev)
>>              aml_append(ifctx, aml_return(aml_buffer(1, zerobyte)));
>>          }
>>          aml_append(method, ifctx);
>> +
>> +       /* dummy MOR Memory Clear for the sake of WLK PPI test */
>> +        ifctx = aml_if(
>> +            aml_equal(aml_arg(0),
>> +                      aml_touuid("376054ED-CC13-4675-901C-4756D7F2D45D")));
>> +        {
>> +            aml_append(ifctx, aml_return(aml_int(0)));
>> +        }
>> +        aml_append(method, ifctx);
>>      }
>>      aml_append(dev, method);
>>  }
>
>



-- 
Marc-André Lureau

Re: [Qemu-devel] [PATCH v3 4/4] tpm: add a fake ACPI memory clear interface

[Stefan Berger]

On 06/21/2018 09:24 AM, Marc-André Lureau wrote:
> Hi
>
> On Thu, Jun 21, 2018 at 3:02 PM, Igor Mammedov <imammedo@redhat.com> wrote:
>> On Tue, 15 May 2018 14:14:33 +0200
>> Marc-André Lureau <marcandre.lureau@redhat.com> wrote:
>>
>>> This allows to pass the last failing test from the Windows HLK TPM 2.0
>>> TCG PPI 1.3 tests.
>>>
>>> The interface is described in the "TCG Platform Reset Attack
>>> Mitigation Specification", chapter 6 "ACPI _DSM Function". Whether or
>>> not we should have a real implementation remains an open question to me.
>> might it cause security issues?
> Good question. If the guest assumes success of this operation perhaps.
> I'll check the spec.
We could reserve a flag in the PPI interface where the firmware can 
indicate that it supports it. ACPI would read that flag and it hide this 
interface if not supported.

>
>> What are implications of faking it and how hard it's to implement thing
>> per spec?
> Laszlo answerd that in "[Qemu-devel] investigating TPM for
> OVMF-on-QEMU"  2f2b) TCG Memory Clear Interface
>
>>
>>> Signed-off-by: Marc-André Lureau <marcandre.lureau@redhat.com>
>>> ---
>>>   hw/i386/acpi-build.c | 9 +++++++++
>>>   1 file changed, 9 insertions(+)
>>>
>>> diff --git a/hw/i386/acpi-build.c b/hw/i386/acpi-build.c
>>> index 95be4f0710..392a1e50bd 100644
>>> --- a/hw/i386/acpi-build.c
>>> +++ b/hw/i386/acpi-build.c
>>> @@ -2072,6 +2072,15 @@ build_tpm_ppi(Aml *dev)
>>>               aml_append(ifctx, aml_return(aml_buffer(1, zerobyte)));
>>>           }
>>>           aml_append(method, ifctx);
>>> +
>>> +       /* dummy MOR Memory Clear for the sake of WLK PPI test */
>>> +        ifctx = aml_if(
>>> +            aml_equal(aml_arg(0),
>>> +                      aml_touuid("376054ED-CC13-4675-901C-4756D7F2D45D")));
>>> +        {
>>> +            aml_append(ifctx, aml_return(aml_int(0)));
>>> +        }
>>> +        aml_append(method, ifctx);
>>>       }
>>>       aml_append(dev, method);
>>>   }
>>
>
>

Re: [Qemu-devel] [PATCH v3 4/4] tpm: add a fake ACPI memory clear interface

[Igor Mammedov]

On Thu, 21 Jun 2018 15:24:44 +0200
Marc-André Lureau <marcandre.lureau@gmail.com> wrote:

> Hi
> 
> On Thu, Jun 21, 2018 at 3:02 PM, Igor Mammedov <imammedo@redhat.com> wrote:
> > On Tue, 15 May 2018 14:14:33 +0200
> > Marc-André Lureau <marcandre.lureau@redhat.com> wrote:
> >  
> >> This allows to pass the last failing test from the Windows HLK TPM 2.0
> >> TCG PPI 1.3 tests.
> >>
> >> The interface is described in the "TCG Platform Reset Attack
> >> Mitigation Specification", chapter 6 "ACPI _DSM Function". Whether or
> >> not we should have a real implementation remains an open question to me.  
> > might it cause security issues?  
> 
> Good question. If the guest assumes success of this operation perhaps.
> I'll check the spec.
> 
> > What are implications of faking it and how hard it's to implement thing
> > per spec?  
> 
> Laszlo answerd that in "[Qemu-devel] investigating TPM for
> OVMF-on-QEMU"  2f2b) TCG Memory Clear Interface
I get that it's optional, but we probably shouldn't advertise/fake
feature if it's not supported.

> 
> >
> >  
> >> Signed-off-by: Marc-André Lureau <marcandre.lureau@redhat.com>
> >> ---
> >>  hw/i386/acpi-build.c | 9 +++++++++
> >>  1 file changed, 9 insertions(+)
> >>
> >> diff --git a/hw/i386/acpi-build.c b/hw/i386/acpi-build.c
> >> index 95be4f0710..392a1e50bd 100644
> >> --- a/hw/i386/acpi-build.c
> >> +++ b/hw/i386/acpi-build.c
> >> @@ -2072,6 +2072,15 @@ build_tpm_ppi(Aml *dev)
> >>              aml_append(ifctx, aml_return(aml_buffer(1, zerobyte)));
> >>          }
> >>          aml_append(method, ifctx);
> >> +
> >> +       /* dummy MOR Memory Clear for the sake of WLK PPI test */
> >> +        ifctx = aml_if(
> >> +            aml_equal(aml_arg(0),
> >> +                      aml_touuid("376054ED-CC13-4675-901C-4756D7F2D45D")));
> >> +        {
> >> +            aml_append(ifctx, aml_return(aml_int(0)));
> >> +        }
> >> +        aml_append(method, ifctx);
> >>      }
> >>      aml_append(dev, method);
> >>  }  
> >
> >  
> 
> 
> 

Re: [Qemu-devel] [PATCH v3 4/4] tpm: add a fake ACPI memory clear interface

[Marc-André Lureau]

On Thu, Jun 21, 2018 at 4:33 PM, Igor Mammedov <imammedo@redhat.com> wrote:
> On Thu, 21 Jun 2018 15:24:44 +0200
> Marc-André Lureau <marcandre.lureau@gmail.com> wrote:
>
>> Hi
>>
>> On Thu, Jun 21, 2018 at 3:02 PM, Igor Mammedov <imammedo@redhat.com> wrote:
>> > On Tue, 15 May 2018 14:14:33 +0200
>> > Marc-André Lureau <marcandre.lureau@redhat.com> wrote:
>> >
>> >> This allows to pass the last failing test from the Windows HLK TPM 2.0
>> >> TCG PPI 1.3 tests.
>> >>
>> >> The interface is described in the "TCG Platform Reset Attack
>> >> Mitigation Specification", chapter 6 "ACPI _DSM Function". Whether or
>> >> not we should have a real implementation remains an open question to me.
>> > might it cause security issues?
>>
>> Good question. If the guest assumes success of this operation perhaps.
>> I'll check the spec.
>>
>> > What are implications of faking it and how hard it's to implement thing
>> > per spec?
>>
>> Laszlo answerd that in "[Qemu-devel] investigating TPM for
>> OVMF-on-QEMU"  2f2b) TCG Memory Clear Interface
> I get that it's optional, but we probably shouldn't advertise/fake
> feature if it's not supported.

As said in the commit message, the objective was to pass the Windows
HLK test. If we don't want to advertize a fake interface, I am fine
droping this patch. We'll have to revisit with Laszlo the work needed
in the firmware to support it.

>
>>
>> >
>> >
>> >> Signed-off-by: Marc-André Lureau <marcandre.lureau@redhat.com>
>> >> ---
>> >>  hw/i386/acpi-build.c | 9 +++++++++
>> >>  1 file changed, 9 insertions(+)
>> >>
>> >> diff --git a/hw/i386/acpi-build.c b/hw/i386/acpi-build.c
>> >> index 95be4f0710..392a1e50bd 100644
>> >> --- a/hw/i386/acpi-build.c
>> >> +++ b/hw/i386/acpi-build.c
>> >> @@ -2072,6 +2072,15 @@ build_tpm_ppi(Aml *dev)
>> >>              aml_append(ifctx, aml_return(aml_buffer(1, zerobyte)));
>> >>          }
>> >>          aml_append(method, ifctx);
>> >> +
>> >> +       /* dummy MOR Memory Clear for the sake of WLK PPI test */
>> >> +        ifctx = aml_if(
>> >> +            aml_equal(aml_arg(0),
>> >> +                      aml_touuid("376054ED-CC13-4675-901C-4756D7F2D45D")));
>> >> +        {
>> >> +            aml_append(ifctx, aml_return(aml_int(0)));
>> >> +        }
>> >> +        aml_append(method, ifctx);
>> >>      }
>> >>      aml_append(dev, method);
>> >>  }
>> >
>> >
>>
>>
>>
>



-- 
Marc-André Lureau

Re: [Qemu-devel] [PATCH v3 4/4] tpm: add a fake ACPI memory clear interface

[Igor Mammedov]

On Tue, 26 Jun 2018 11:22:26 +0200
Marc-André Lureau <marcandre.lureau@gmail.com> wrote:

> On Thu, Jun 21, 2018 at 4:33 PM, Igor Mammedov <imammedo@redhat.com> wrote:
> > On Thu, 21 Jun 2018 15:24:44 +0200
> > Marc-André Lureau <marcandre.lureau@gmail.com> wrote:
> >  
> >> Hi
> >>
> >> On Thu, Jun 21, 2018 at 3:02 PM, Igor Mammedov <imammedo@redhat.com> wrote:  
> >> > On Tue, 15 May 2018 14:14:33 +0200
> >> > Marc-André Lureau <marcandre.lureau@redhat.com> wrote:
> >> >  
> >> >> This allows to pass the last failing test from the Windows HLK TPM 2.0
> >> >> TCG PPI 1.3 tests.
> >> >>
> >> >> The interface is described in the "TCG Platform Reset Attack
> >> >> Mitigation Specification", chapter 6 "ACPI _DSM Function". Whether or
> >> >> not we should have a real implementation remains an open question to me.  
> >> > might it cause security issues?  
> >>
> >> Good question. If the guest assumes success of this operation perhaps.
> >> I'll check the spec.
> >>  
> >> > What are implications of faking it and how hard it's to implement thing
> >> > per spec?  
> >>
> >> Laszlo answerd that in "[Qemu-devel] investigating TPM for
> >> OVMF-on-QEMU"  2f2b) TCG Memory Clear Interface  
> > I get that it's optional, but we probably shouldn't advertise/fake
> > feature if it's not supported.  
> 
> As said in the commit message, the objective was to pass the Windows
> HLK test. If we don't want to advertize a fake interface, I am fine
> droping this patch. We'll have to revisit with Laszlo the work needed
> in the firmware to support it.
I think it would be safer to drop this patch.


> >  
> >>  
> >> >
> >> >  
> >> >> Signed-off-by: Marc-André Lureau <marcandre.lureau@redhat.com>
> >> >> ---
> >> >>  hw/i386/acpi-build.c | 9 +++++++++
> >> >>  1 file changed, 9 insertions(+)
> >> >>
> >> >> diff --git a/hw/i386/acpi-build.c b/hw/i386/acpi-build.c
> >> >> index 95be4f0710..392a1e50bd 100644
> >> >> --- a/hw/i386/acpi-build.c
> >> >> +++ b/hw/i386/acpi-build.c
> >> >> @@ -2072,6 +2072,15 @@ build_tpm_ppi(Aml *dev)
> >> >>              aml_append(ifctx, aml_return(aml_buffer(1, zerobyte)));
> >> >>          }
> >> >>          aml_append(method, ifctx);
> >> >> +
> >> >> +       /* dummy MOR Memory Clear for the sake of WLK PPI test */
> >> >> +        ifctx = aml_if(
> >> >> +            aml_equal(aml_arg(0),
> >> >> +                      aml_touuid("376054ED-CC13-4675-901C-4756D7F2D45D")));
> >> >> +        {
> >> >> +            aml_append(ifctx, aml_return(aml_int(0)));
> >> >> +        }
> >> >> +        aml_append(method, ifctx);
> >> >>      }
> >> >>      aml_append(dev, method);
> >> >>  }  
> >> >
> >> >  
> >>
> >>
> >>  
> >  
> 
> 
> 

Re: [Qemu-devel] [PATCH v3 4/4] tpm: add a fake ACPI memory clear interface

[Laszlo Ersek]

On 06/26/18 14:34, Igor Mammedov wrote:
> On Tue, 26 Jun 2018 11:22:26 +0200
> Marc-André Lureau <marcandre.lureau@gmail.com> wrote:
> 
>> On Thu, Jun 21, 2018 at 4:33 PM, Igor Mammedov <imammedo@redhat.com> wrote:
>>> On Thu, 21 Jun 2018 15:24:44 +0200
>>> Marc-André Lureau <marcandre.lureau@gmail.com> wrote:
>>>  
>>>> Hi
>>>>
>>>> On Thu, Jun 21, 2018 at 3:02 PM, Igor Mammedov <imammedo@redhat.com> wrote:  
>>>>> On Tue, 15 May 2018 14:14:33 +0200
>>>>> Marc-André Lureau <marcandre.lureau@redhat.com> wrote:
>>>>>  
>>>>>> This allows to pass the last failing test from the Windows HLK TPM 2.0
>>>>>> TCG PPI 1.3 tests.
>>>>>>
>>>>>> The interface is described in the "TCG Platform Reset Attack
>>>>>> Mitigation Specification", chapter 6 "ACPI _DSM Function". Whether or
>>>>>> not we should have a real implementation remains an open question to me.  
>>>>> might it cause security issues?  
>>>>
>>>> Good question. If the guest assumes success of this operation perhaps.
>>>> I'll check the spec.
>>>>  
>>>>> What are implications of faking it and how hard it's to implement thing
>>>>> per spec?  
>>>>
>>>> Laszlo answerd that in "[Qemu-devel] investigating TPM for
>>>> OVMF-on-QEMU"  2f2b) TCG Memory Clear Interface  
>>> I get that it's optional, but we probably shouldn't advertise/fake
>>> feature if it's not supported.  
>>
>> As said in the commit message, the objective was to pass the Windows
>> HLK test. If we don't want to advertize a fake interface, I am fine
>> droping this patch. We'll have to revisit with Laszlo the work needed
>> in the firmware to support it.
> I think it would be safer to drop this patch.

This is BTW a feature that's very difficult for OVMF to implement, but
(I think) near trivial for QEMU to implement. The feature is about
clearing all of the guest RAM to zero at reboot.

For the firmware, it's difficult to solve, because in the 32-bit PEI
phase, we don't map DRAM beyond 4GB, so we can't re-set memory to zero
via normal addressing. (For physical platforms, this is different,
because their PEI phases have PEI modules that initialize the memory
controller(s), so they have platform-specific means to clear RAM.) For
QEMU, on the other hand, the feature "shouldn't be hard (TM)", just
implement a reset handler that clears all RAMBlocks on the host side (or
some such :) ).

Thanks,
Laszlo

> 
> 
>>>  
>>>>  
>>>>>
>>>>>  
>>>>>> Signed-off-by: Marc-André Lureau <marcandre.lureau@redhat.com>
>>>>>> ---
>>>>>>  hw/i386/acpi-build.c | 9 +++++++++
>>>>>>  1 file changed, 9 insertions(+)
>>>>>>
>>>>>> diff --git a/hw/i386/acpi-build.c b/hw/i386/acpi-build.c
>>>>>> index 95be4f0710..392a1e50bd 100644
>>>>>> --- a/hw/i386/acpi-build.c
>>>>>> +++ b/hw/i386/acpi-build.c
>>>>>> @@ -2072,6 +2072,15 @@ build_tpm_ppi(Aml *dev)
>>>>>>              aml_append(ifctx, aml_return(aml_buffer(1, zerobyte)));
>>>>>>          }
>>>>>>          aml_append(method, ifctx);
>>>>>> +
>>>>>> +       /* dummy MOR Memory Clear for the sake of WLK PPI test */
>>>>>> +        ifctx = aml_if(
>>>>>> +            aml_equal(aml_arg(0),
>>>>>> +                      aml_touuid("376054ED-CC13-4675-901C-4756D7F2D45D")));
>>>>>> +        {
>>>>>> +            aml_append(ifctx, aml_return(aml_int(0)));
>>>>>> +        }
>>>>>> +        aml_append(method, ifctx);
>>>>>>      }
>>>>>>      aml_append(dev, method);
>>>>>>  }  
>>>>>
>>>>>  
>>>>
>>>>
>>>>  
>>>  
>>
>>
>>
> 

Re: [Qemu-devel] [PATCH v3 4/4] tpm: add a fake ACPI memory clear interface

[Stefan Berger]

On 06/26/2018 08:47 AM, Laszlo Ersek wrote:
> On 06/26/18 14:34, Igor Mammedov wrote:
>> On Tue, 26 Jun 2018 11:22:26 +0200
>> Marc-André Lureau <marcandre.lureau@gmail.com> wrote:
>>
>>> On Thu, Jun 21, 2018 at 4:33 PM, Igor Mammedov <imammedo@redhat.com> wrote:
>>>> On Thu, 21 Jun 2018 15:24:44 +0200
>>>> Marc-André Lureau <marcandre.lureau@gmail.com> wrote:
>>>>   
>>>>> Hi
>>>>>
>>>>> On Thu, Jun 21, 2018 at 3:02 PM, Igor Mammedov <imammedo@redhat.com> wrote:
>>>>>> On Tue, 15 May 2018 14:14:33 +0200
>>>>>> Marc-André Lureau <marcandre.lureau@redhat.com> wrote:
>>>>>>   
>>>>>>> This allows to pass the last failing test from the Windows HLK TPM 2.0
>>>>>>> TCG PPI 1.3 tests.
>>>>>>>
>>>>>>> The interface is described in the "TCG Platform Reset Attack
>>>>>>> Mitigation Specification", chapter 6 "ACPI _DSM Function". Whether or
>>>>>>> not we should have a real implementation remains an open question to me.
>>>>>> might it cause security issues?
>>>>> Good question. If the guest assumes success of this operation perhaps.
>>>>> I'll check the spec.
>>>>>   
>>>>>> What are implications of faking it and how hard it's to implement thing
>>>>>> per spec?
>>>>> Laszlo answerd that in "[Qemu-devel] investigating TPM for
>>>>> OVMF-on-QEMU"  2f2b) TCG Memory Clear Interface
>>>> I get that it's optional, but we probably shouldn't advertise/fake
>>>> feature if it's not supported.
>>> As said in the commit message, the objective was to pass the Windows
>>> HLK test. If we don't want to advertize a fake interface, I am fine
>>> droping this patch. We'll have to revisit with Laszlo the work needed
>>> in the firmware to support it.
>> I think it would be safer to drop this patch.
> This is BTW a feature that's very difficult for OVMF to implement, but
> (I think) near trivial for QEMU to implement. The feature is about
> clearing all of the guest RAM to zero at reboot.
>
> For the firmware, it's difficult to solve, because in the 32-bit PEI
> phase, we don't map DRAM beyond 4GB, so we can't re-set memory to zero
> via normal addressing. (For physical platforms, this is different,
> because their PEI phases have PEI modules that initialize the memory
> controller(s), so they have platform-specific means to clear RAM.) For
> QEMU, on the other hand, the feature "shouldn't be hard (TM)", just
> implement a reset handler that clears all RAMBlocks on the host side (or
> some such :) ).

Except don't clear that PPI memory device where the user/ACPI posted the 
command for the firmware to act upon.

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

[Marc-André Lureau]

On Tue, May 15, 2018 at 2:14 PM, Marc-André Lureau
<marcandre.lureau@redhat.com> wrote:
> 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")
>
> Git branches:
> https://github.com/elmarco/qemu tpm-ppi
> https://github.com/elmarco/edk2 tpm-ppi
>
> Marc-André Lureau (1):
>   tpm: add a fake ACPI memory clear interface
>
> 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

ping

>
>  hw/tpm/tpm_ppi.h      |  26 ++++
>  include/hw/acpi/tpm.h |  30 ++++
>  hw/i386/acpi-build.c  | 318 ++++++++++++++++++++++++++++++++++++++++++
>  hw/tpm/tpm_crb.c      |   5 +
>  hw/tpm/tpm_ppi.c      |  56 ++++++++
>  hw/tpm/tpm_tis.c      |   5 +
>  docs/specs/tpm.txt    |  20 +++
>  hw/tpm/Makefile.objs  |   2 +-
>  hw/tpm/trace-events   |   4 +
>  9 files changed, 465 insertions(+), 1 deletion(-)
>  create mode 100644 hw/tpm/tpm_ppi.h
>  create mode 100644 hw/tpm/tpm_ppi.c
>
> --
> 2.17.0.253.g3dd125b46d
>
>



-- 
Marc-André Lureau