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

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

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

Note: bios_linker cannot be used to allocate the PPI memory region,
since the reserved memory should stay stable across reboots, and might
be needed before the ACPI tables are installed.

Signed-off-by: Stefan Berger <stefanb@linux.vnet.ibm.com>
Signed-off-by: Marc-André Lureau <marcandre.lureau@redhat.com>
---
 hw/tpm/tpm_ppi.h      | 26 ++++++++++++++++++++++++++
 include/hw/acpi/tpm.h |  6 ++++++
 hw/tpm/tpm_crb.c      |  8 ++++++++
 hw/tpm/tpm_ppi.c      | 31 +++++++++++++++++++++++++++++++
 hw/tpm/tpm_tis.c      |  8 ++++++++
 hw/tpm/Makefile.objs  |  1 +
 6 files changed, 80 insertions(+)
 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..f6458bf87e
--- /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 ram;
+    uint8_t buf[TPM_PPI_ADDR_SIZE];
+} TPMPPI;
+
+bool tpm_ppi_init(TPMPPI *tpmppi, struct MemoryRegion *m,
+                  hwaddr addr, Object *obj, Error **errp);
+
+#endif /* TPM_TPM_PPI_H */
diff --git a/include/hw/acpi/tpm.h b/include/hw/acpi/tpm.h
index 3580ffd50c..b8796df916 100644
--- a/include/hw/acpi/tpm.h
+++ b/include/hw/acpi/tpm.h
@@ -188,4 +188,10 @@ REG32(CRB_DATA_BUFFER, 0x80)
 #define TPM2_START_METHOD_MMIO      6
 #define TPM2_START_METHOD_CRB       7
 
+/*
+ * Physical Presence Interface
+ */
+#define TPM_PPI_ADDR_SIZE           0x400
+#define TPM_PPI_ADDR_BASE           0xFED45000
+
 #endif /* HW_ACPI_TPM_H */
diff --git a/hw/tpm/tpm_crb.c b/hw/tpm/tpm_crb.c
index d5b0ac5920..b243222fd6 100644
--- a/hw/tpm/tpm_crb.c
+++ b/hw/tpm/tpm_crb.c
@@ -29,6 +29,7 @@
 #include "sysemu/reset.h"
 #include "tpm_int.h"
 #include "tpm_util.h"
+#include "tpm_ppi.h"
 #include "trace.h"
 
 typedef struct CRBState {
@@ -43,6 +44,7 @@ typedef struct CRBState {
     size_t be_buffer_size;
 
     bool ppi_enabled;
+    TPMPPI ppi;
 } CRBState;
 
 #define CRB(obj) OBJECT_CHECK(CRBState, (obj), TYPE_TPM_CRB)
@@ -294,6 +296,12 @@ static void tpm_crb_realize(DeviceState *dev, Error **errp)
     memory_region_add_subregion(get_system_memory(),
         TPM_CRB_ADDR_BASE + sizeof(s->regs), &s->cmdmem);
 
+    if (s->ppi_enabled &&
+        !tpm_ppi_init(&s->ppi, get_system_memory(),
+                      TPM_PPI_ADDR_BASE, OBJECT(s), errp)) {
+        return;
+    }
+
     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..8b46b9dd4b
--- /dev/null
+++ b/hw/tpm/tpm_ppi.c
@@ -0,0 +1,31 @@
+/*
+ * 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 "qapi/error.h"
+#include "cpu.h"
+#include "sysemu/memory_mapping.h"
+#include "migration/vmstate.h"
+#include "tpm_ppi.h"
+
+bool tpm_ppi_init(TPMPPI *tpmppi, struct MemoryRegion *m,
+                  hwaddr addr, Object *obj, Error **errp)
+{
+    memory_region_init_ram_device_ptr(&tpmppi->ram, obj, "tpm-ppi",
+                                      TPM_PPI_ADDR_SIZE, tpmppi->buf);
+    vmstate_register_ram(&tpmppi->ram, DEVICE(obj));
+
+    memory_region_add_subregion(m, addr, &tpmppi->ram);
+    return true;
+}
diff --git a/hw/tpm/tpm_tis.c b/hw/tpm/tpm_tis.c
index d9ddf9b723..70432ffe8b 100644
--- a/hw/tpm/tpm_tis.c
+++ b/hw/tpm/tpm_tis.c
@@ -31,6 +31,7 @@
 #include "sysemu/tpm_backend.h"
 #include "tpm_int.h"
 #include "tpm_util.h"
+#include "tpm_ppi.h"
 #include "trace.h"
 
 #define TPM_TIS_NUM_LOCALITIES      5     /* per spec */
@@ -83,6 +84,7 @@ typedef struct TPMState {
     size_t be_buffer_size;
 
     bool ppi_enabled;
+    TPMPPI ppi;
 } TPMState;
 
 #define TPM(obj) OBJECT_CHECK(TPMState, (obj), TYPE_TPM_TIS)
@@ -979,6 +981,12 @@ static void tpm_tis_realizefn(DeviceState *dev, Error **errp)
 
     memory_region_add_subregion(isa_address_space(ISA_DEVICE(dev)),
                                 TPM_TIS_ADDR_BASE, &s->mmio);
+
+    if (s->ppi_enabled &&
+        !tpm_ppi_init(&s->ppi, isa_address_space(ISA_DEVICE(dev)),
+                      TPM_PPI_ADDR_BASE, OBJECT(s), errp)) {
+        return;
+    }
 }
 
 static void tpm_tis_initfn(Object *obj)
diff --git a/hw/tpm/Makefile.objs b/hw/tpm/Makefile.objs
index 1dc9f8bf2c..700c878622 100644
--- a/hw/tpm/Makefile.objs
+++ b/hw/tpm/Makefile.objs
@@ -1,4 +1,5 @@
 common-obj-y += tpm_util.o
+obj-y += 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
-- 
2.18.0.129.ge3331758f1

On Mon, 16 Jul 2018 14:59:45 +0200
Marc-André Lureau <marcandre.lureau@redhat.com> wrote:

> From: Stefan Berger <stefanb@linux.vnet.ibm.com>
> 
> Implement a virtual memory device for the TPM Physical Presence interface.
> The memory is located at 0xFED45000 and used by ACPI to send messages to the
> firmware (BIOS) and by the firmware to provide parameters for each one of
> the supported codes.
it doesn't implement anything, maybe something like this would be better?

tpm: allocate/map buffer for TPM Physical Presence interface

...

> 
> This device should be used by all TPM interfaces on x86 and can be added
> by calling tpm_ppi_init_io().
Did you mean:
 This *interface* should be used by all TPM *devices* on x86 and can be added
 by calling tpm_ppi_init_io().


> Note: bios_linker cannot be used to allocate the PPI memory region,
> since the reserved memory should stay stable across reboots, and might
> be needed before the ACPI tables are installed.
> 
> Signed-off-by: Stefan Berger <stefanb@linux.vnet.ibm.com>
> Signed-off-by: Marc-André Lureau <marcandre.lureau@redhat.com>
> ---
>  hw/tpm/tpm_ppi.h      | 26 ++++++++++++++++++++++++++
>  include/hw/acpi/tpm.h |  6 ++++++
>  hw/tpm/tpm_crb.c      |  8 ++++++++
>  hw/tpm/tpm_ppi.c      | 31 +++++++++++++++++++++++++++++++
>  hw/tpm/tpm_tis.c      |  8 ++++++++
>  hw/tpm/Makefile.objs  |  1 +
>  6 files changed, 80 insertions(+)
>  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..f6458bf87e
> --- /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 ram;
> +    uint8_t buf[TPM_PPI_ADDR_SIZE];
> +} TPMPPI;
> +
> +bool tpm_ppi_init(TPMPPI *tpmppi, struct MemoryRegion *m,
> +                  hwaddr addr, Object *obj, Error **errp);
> +
> +#endif /* TPM_TPM_PPI_H */
> diff --git a/include/hw/acpi/tpm.h b/include/hw/acpi/tpm.h
> index 3580ffd50c..b8796df916 100644
> --- a/include/hw/acpi/tpm.h
> +++ b/include/hw/acpi/tpm.h
> @@ -188,4 +188,10 @@ REG32(CRB_DATA_BUFFER, 0x80)
>  #define TPM2_START_METHOD_MMIO      6
>  #define TPM2_START_METHOD_CRB       7
>  
> +/*
> + * Physical Presence Interface
> + */
> +#define TPM_PPI_ADDR_SIZE           0x400
> +#define TPM_PPI_ADDR_BASE           0xFED45000
> +
>  #endif /* HW_ACPI_TPM_H */
> diff --git a/hw/tpm/tpm_crb.c b/hw/tpm/tpm_crb.c
> index d5b0ac5920..b243222fd6 100644
> --- a/hw/tpm/tpm_crb.c
> +++ b/hw/tpm/tpm_crb.c
> @@ -29,6 +29,7 @@
>  #include "sysemu/reset.h"
>  #include "tpm_int.h"
>  #include "tpm_util.h"
> +#include "tpm_ppi.h"
>  #include "trace.h"
>  
>  typedef struct CRBState {
> @@ -43,6 +44,7 @@ typedef struct CRBState {
>      size_t be_buffer_size;
>  
>      bool ppi_enabled;
> +    TPMPPI ppi;
>  } CRBState;
>  
>  #define CRB(obj) OBJECT_CHECK(CRBState, (obj), TYPE_TPM_CRB)
> @@ -294,6 +296,12 @@ static void tpm_crb_realize(DeviceState *dev, Error **errp)
>      memory_region_add_subregion(get_system_memory(),
>          TPM_CRB_ADDR_BASE + sizeof(s->regs), &s->cmdmem);
>  
> +    if (s->ppi_enabled &&
> +        !tpm_ppi_init(&s->ppi, get_system_memory(),
> +                      TPM_PPI_ADDR_BASE, OBJECT(s), errp)) {
> +        return;
> +    }
> +
>      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..8b46b9dd4b
> --- /dev/null
> +++ b/hw/tpm/tpm_ppi.c
> @@ -0,0 +1,31 @@
> +/*
> + * 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 "qapi/error.h"
> +#include "cpu.h"
> +#include "sysemu/memory_mapping.h"
> +#include "migration/vmstate.h"
> +#include "tpm_ppi.h"
> +
> +bool tpm_ppi_init(TPMPPI *tpmppi, struct MemoryRegion *m,
> +                  hwaddr addr, Object *obj, Error **errp)
> +{
> +    memory_region_init_ram_device_ptr(&tpmppi->ram, obj, "tpm-ppi",
> +                                      TPM_PPI_ADDR_SIZE, tpmppi->buf);
why use buf and not a plain ram memory region?

> +    vmstate_register_ram(&tpmppi->ram, DEVICE(obj));
> +
> +    memory_region_add_subregion(m, addr, &tpmppi->ram);
> +    return true;
> +}
> diff --git a/hw/tpm/tpm_tis.c b/hw/tpm/tpm_tis.c
> index d9ddf9b723..70432ffe8b 100644
> --- a/hw/tpm/tpm_tis.c
> +++ b/hw/tpm/tpm_tis.c
> @@ -31,6 +31,7 @@
>  #include "sysemu/tpm_backend.h"
>  #include "tpm_int.h"
>  #include "tpm_util.h"
> +#include "tpm_ppi.h"
>  #include "trace.h"
>  
>  #define TPM_TIS_NUM_LOCALITIES      5     /* per spec */
> @@ -83,6 +84,7 @@ typedef struct TPMState {
>      size_t be_buffer_size;
>  
>      bool ppi_enabled;
> +    TPMPPI ppi;
>  } TPMState;
>  
>  #define TPM(obj) OBJECT_CHECK(TPMState, (obj), TYPE_TPM_TIS)
> @@ -979,6 +981,12 @@ static void tpm_tis_realizefn(DeviceState *dev, Error **errp)
>  
>      memory_region_add_subregion(isa_address_space(ISA_DEVICE(dev)),
>                                  TPM_TIS_ADDR_BASE, &s->mmio);
> +
> +    if (s->ppi_enabled &&
> +        !tpm_ppi_init(&s->ppi, isa_address_space(ISA_DEVICE(dev)),
> +                      TPM_PPI_ADDR_BASE, OBJECT(s), errp)) {
> +        return;
> +    }
>  }
>  
>  static void tpm_tis_initfn(Object *obj)
> diff --git a/hw/tpm/Makefile.objs b/hw/tpm/Makefile.objs
> index 1dc9f8bf2c..700c878622 100644
> --- a/hw/tpm/Makefile.objs
> +++ b/hw/tpm/Makefile.objs
> @@ -1,4 +1,5 @@
>  common-obj-y += tpm_util.o
> +obj-y += 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

Hi

On Mon, Jul 16, 2018 at 4:44 PM, Igor Mammedov <imammedo@redhat.com> wrote:
> On Mon, 16 Jul 2018 14:59:45 +0200
> Marc-André Lureau <marcandre.lureau@redhat.com> wrote:
>
>> From: Stefan Berger <stefanb@linux.vnet.ibm.com>
>>
>> Implement a virtual memory device for the TPM Physical Presence interface.
>> The memory is located at 0xFED45000 and used by ACPI to send messages to the
>> firmware (BIOS) and by the firmware to provide parameters for each one of
>> the supported codes.
> it doesn't implement anything, maybe something like this would be better?
>
> tpm: allocate/map buffer for TPM Physical Presence interface
>
> ...
>
>>
>> This device should be used by all TPM interfaces on x86 and can be added
>> by calling tpm_ppi_init_io().
> Did you mean:
>  This *interface* should be used by all TPM *devices* on x86 and can be added
>  by calling tpm_ppi_init_io().
>
>
>> Note: bios_linker cannot be used to allocate the PPI memory region,
>> since the reserved memory should stay stable across reboots, and might
>> be needed before the ACPI tables are installed.
>>
>> Signed-off-by: Stefan Berger <stefanb@linux.vnet.ibm.com>
>> Signed-off-by: Marc-André Lureau <marcandre.lureau@redhat.com>
>> ---
>>  hw/tpm/tpm_ppi.h      | 26 ++++++++++++++++++++++++++
>>  include/hw/acpi/tpm.h |  6 ++++++
>>  hw/tpm/tpm_crb.c      |  8 ++++++++
>>  hw/tpm/tpm_ppi.c      | 31 +++++++++++++++++++++++++++++++
>>  hw/tpm/tpm_tis.c      |  8 ++++++++
>>  hw/tpm/Makefile.objs  |  1 +
>>  6 files changed, 80 insertions(+)
>>  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..f6458bf87e
>> --- /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 ram;
>> +    uint8_t buf[TPM_PPI_ADDR_SIZE];
>> +} TPMPPI;
>> +
>> +bool tpm_ppi_init(TPMPPI *tpmppi, struct MemoryRegion *m,
>> +                  hwaddr addr, Object *obj, Error **errp);
>> +
>> +#endif /* TPM_TPM_PPI_H */
>> diff --git a/include/hw/acpi/tpm.h b/include/hw/acpi/tpm.h
>> index 3580ffd50c..b8796df916 100644
>> --- a/include/hw/acpi/tpm.h
>> +++ b/include/hw/acpi/tpm.h
>> @@ -188,4 +188,10 @@ REG32(CRB_DATA_BUFFER, 0x80)
>>  #define TPM2_START_METHOD_MMIO      6
>>  #define TPM2_START_METHOD_CRB       7
>>
>> +/*
>> + * Physical Presence Interface
>> + */
>> +#define TPM_PPI_ADDR_SIZE           0x400
>> +#define TPM_PPI_ADDR_BASE           0xFED45000
>> +
>>  #endif /* HW_ACPI_TPM_H */
>> diff --git a/hw/tpm/tpm_crb.c b/hw/tpm/tpm_crb.c
>> index d5b0ac5920..b243222fd6 100644
>> --- a/hw/tpm/tpm_crb.c
>> +++ b/hw/tpm/tpm_crb.c
>> @@ -29,6 +29,7 @@
>>  #include "sysemu/reset.h"
>>  #include "tpm_int.h"
>>  #include "tpm_util.h"
>> +#include "tpm_ppi.h"
>>  #include "trace.h"
>>
>>  typedef struct CRBState {
>> @@ -43,6 +44,7 @@ typedef struct CRBState {
>>      size_t be_buffer_size;
>>
>>      bool ppi_enabled;
>> +    TPMPPI ppi;
>>  } CRBState;
>>
>>  #define CRB(obj) OBJECT_CHECK(CRBState, (obj), TYPE_TPM_CRB)
>> @@ -294,6 +296,12 @@ static void tpm_crb_realize(DeviceState *dev, Error **errp)
>>      memory_region_add_subregion(get_system_memory(),
>>          TPM_CRB_ADDR_BASE + sizeof(s->regs), &s->cmdmem);
>>
>> +    if (s->ppi_enabled &&
>> +        !tpm_ppi_init(&s->ppi, get_system_memory(),
>> +                      TPM_PPI_ADDR_BASE, OBJECT(s), errp)) {
>> +        return;
>> +    }
>> +
>>      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..8b46b9dd4b
>> --- /dev/null
>> +++ b/hw/tpm/tpm_ppi.c
>> @@ -0,0 +1,31 @@
>> +/*
>> + * 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 "qapi/error.h"
>> +#include "cpu.h"
>> +#include "sysemu/memory_mapping.h"
>> +#include "migration/vmstate.h"
>> +#include "tpm_ppi.h"
>> +
>> +bool tpm_ppi_init(TPMPPI *tpmppi, struct MemoryRegion *m,
>> +                  hwaddr addr, Object *obj, Error **errp)
>> +{
>> +    memory_region_init_ram_device_ptr(&tpmppi->ram, obj, "tpm-ppi",
>> +                                      TPM_PPI_ADDR_SIZE, tpmppi->buf);
> why use buf and not a plain ram memory region?

It shouldn't be treated as regular RAM. (for example when iterating
guest_phys_blocks for dump or memory clear in last patch)

>
>> +    vmstate_register_ram(&tpmppi->ram, DEVICE(obj));
>> +
>> +    memory_region_add_subregion(m, addr, &tpmppi->ram);
>> +    return true;
>> +}
>> diff --git a/hw/tpm/tpm_tis.c b/hw/tpm/tpm_tis.c
>> index d9ddf9b723..70432ffe8b 100644
>> --- a/hw/tpm/tpm_tis.c
>> +++ b/hw/tpm/tpm_tis.c
>> @@ -31,6 +31,7 @@
>>  #include "sysemu/tpm_backend.h"
>>  #include "tpm_int.h"
>>  #include "tpm_util.h"
>> +#include "tpm_ppi.h"
>>  #include "trace.h"
>>
>>  #define TPM_TIS_NUM_LOCALITIES      5     /* per spec */
>> @@ -83,6 +84,7 @@ typedef struct TPMState {
>>      size_t be_buffer_size;
>>
>>      bool ppi_enabled;
>> +    TPMPPI ppi;
>>  } TPMState;
>>
>>  #define TPM(obj) OBJECT_CHECK(TPMState, (obj), TYPE_TPM_TIS)
>> @@ -979,6 +981,12 @@ static void tpm_tis_realizefn(DeviceState *dev, Error **errp)
>>
>>      memory_region_add_subregion(isa_address_space(ISA_DEVICE(dev)),
>>                                  TPM_TIS_ADDR_BASE, &s->mmio);
>> +
>> +    if (s->ppi_enabled &&
>> +        !tpm_ppi_init(&s->ppi, isa_address_space(ISA_DEVICE(dev)),
>> +                      TPM_PPI_ADDR_BASE, OBJECT(s), errp)) {
>> +        return;
>> +    }
>>  }
>>
>>  static void tpm_tis_initfn(Object *obj)
>> diff --git a/hw/tpm/Makefile.objs b/hw/tpm/Makefile.objs
>> index 1dc9f8bf2c..700c878622 100644
>> --- a/hw/tpm/Makefile.objs
>> +++ b/hw/tpm/Makefile.objs
>> @@ -1,4 +1,5 @@
>>  common-obj-y += tpm_util.o
>> +obj-y += 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
>
>



-- 
Marc-André Lureau

On Mon, 16 Jul 2018 16:56:36 +0200
Marc-André Lureau <marcandre.lureau@gmail.com> wrote:

> Hi
> 
> On Mon, Jul 16, 2018 at 4:44 PM, Igor Mammedov <imammedo@redhat.com> wrote:
> > On Mon, 16 Jul 2018 14:59:45 +0200
> > Marc-André Lureau <marcandre.lureau@redhat.com> wrote:
> >  
> >> From: Stefan Berger <stefanb@linux.vnet.ibm.com>
> >>
> >> Implement a virtual memory device for the TPM Physical Presence interface.
> >> The memory is located at 0xFED45000 and used by ACPI to send messages to the
> >> firmware (BIOS) and by the firmware to provide parameters for each one of
> >> the supported codes.  
> > it doesn't implement anything, maybe something like this would be better?
> >
> > tpm: allocate/map buffer for TPM Physical Presence interface
> >
> > ...
> >  
> >>
> >> This device should be used by all TPM interfaces on x86 and can be added
> >> by calling tpm_ppi_init_io().  
> > Did you mean:
> >  This *interface* should be used by all TPM *devices* on x86 and can be added
> >  by calling tpm_ppi_init_io().
> >
> >  
> >> Note: bios_linker cannot be used to allocate the PPI memory region,
> >> since the reserved memory should stay stable across reboots, and might
> >> be needed before the ACPI tables are installed.
> >>
> >> Signed-off-by: Stefan Berger <stefanb@linux.vnet.ibm.com>
> >> Signed-off-by: Marc-André Lureau <marcandre.lureau@redhat.com>
> >> ---
> >>  hw/tpm/tpm_ppi.h      | 26 ++++++++++++++++++++++++++
> >>  include/hw/acpi/tpm.h |  6 ++++++
> >>  hw/tpm/tpm_crb.c      |  8 ++++++++
> >>  hw/tpm/tpm_ppi.c      | 31 +++++++++++++++++++++++++++++++
> >>  hw/tpm/tpm_tis.c      |  8 ++++++++
> >>  hw/tpm/Makefile.objs  |  1 +
> >>  6 files changed, 80 insertions(+)
> >>  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..f6458bf87e
> >> --- /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 ram;
> >> +    uint8_t buf[TPM_PPI_ADDR_SIZE];
> >> +} TPMPPI;
> >> +
> >> +bool tpm_ppi_init(TPMPPI *tpmppi, struct MemoryRegion *m,
> >> +                  hwaddr addr, Object *obj, Error **errp);
> >> +
> >> +#endif /* TPM_TPM_PPI_H */
> >> diff --git a/include/hw/acpi/tpm.h b/include/hw/acpi/tpm.h
> >> index 3580ffd50c..b8796df916 100644
> >> --- a/include/hw/acpi/tpm.h
> >> +++ b/include/hw/acpi/tpm.h
> >> @@ -188,4 +188,10 @@ REG32(CRB_DATA_BUFFER, 0x80)
> >>  #define TPM2_START_METHOD_MMIO      6
> >>  #define TPM2_START_METHOD_CRB       7
> >>
> >> +/*
> >> + * Physical Presence Interface
> >> + */
> >> +#define TPM_PPI_ADDR_SIZE           0x400
> >> +#define TPM_PPI_ADDR_BASE           0xFED45000
> >> +
> >>  #endif /* HW_ACPI_TPM_H */
> >> diff --git a/hw/tpm/tpm_crb.c b/hw/tpm/tpm_crb.c
> >> index d5b0ac5920..b243222fd6 100644
> >> --- a/hw/tpm/tpm_crb.c
> >> +++ b/hw/tpm/tpm_crb.c
> >> @@ -29,6 +29,7 @@
> >>  #include "sysemu/reset.h"
> >>  #include "tpm_int.h"
> >>  #include "tpm_util.h"
> >> +#include "tpm_ppi.h"
> >>  #include "trace.h"
> >>
> >>  typedef struct CRBState {
> >> @@ -43,6 +44,7 @@ typedef struct CRBState {
> >>      size_t be_buffer_size;
> >>
> >>      bool ppi_enabled;
> >> +    TPMPPI ppi;
> >>  } CRBState;
> >>
> >>  #define CRB(obj) OBJECT_CHECK(CRBState, (obj), TYPE_TPM_CRB)
> >> @@ -294,6 +296,12 @@ static void tpm_crb_realize(DeviceState *dev, Error **errp)
> >>      memory_region_add_subregion(get_system_memory(),
> >>          TPM_CRB_ADDR_BASE + sizeof(s->regs), &s->cmdmem);
> >>
> >> +    if (s->ppi_enabled &&
> >> +        !tpm_ppi_init(&s->ppi, get_system_memory(),
> >> +                      TPM_PPI_ADDR_BASE, OBJECT(s), errp)) {
> >> +        return;
> >> +    }
> >> +
> >>      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..8b46b9dd4b
> >> --- /dev/null
> >> +++ b/hw/tpm/tpm_ppi.c
> >> @@ -0,0 +1,31 @@
> >> +/*
> >> + * 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 "qapi/error.h"
> >> +#include "cpu.h"
> >> +#include "sysemu/memory_mapping.h"
> >> +#include "migration/vmstate.h"
> >> +#include "tpm_ppi.h"
> >> +
> >> +bool tpm_ppi_init(TPMPPI *tpmppi, struct MemoryRegion *m,
> >> +                  hwaddr addr, Object *obj, Error **errp)
> >> +{
> >> +    memory_region_init_ram_device_ptr(&tpmppi->ram, obj, "tpm-ppi",
> >> +                                      TPM_PPI_ADDR_SIZE, tpmppi->buf);  
> > why use buf and not a plain ram memory region?  
> 
> It shouldn't be treated as regular RAM. (for example when iterating
> guest_phys_blocks for dump or memory clear in last patch)
why it shouldn't, I'd think having ppi flags in dump would be useful
and why it shouldn't be cleared on reset along with all other ram?

> 
> >  
> >> +    vmstate_register_ram(&tpmppi->ram, DEVICE(obj));
> >> +
> >> +    memory_region_add_subregion(m, addr, &tpmppi->ram);
> >> +    return true;
> >> +}
> >> diff --git a/hw/tpm/tpm_tis.c b/hw/tpm/tpm_tis.c
> >> index d9ddf9b723..70432ffe8b 100644
> >> --- a/hw/tpm/tpm_tis.c
> >> +++ b/hw/tpm/tpm_tis.c
> >> @@ -31,6 +31,7 @@
> >>  #include "sysemu/tpm_backend.h"
> >>  #include "tpm_int.h"
> >>  #include "tpm_util.h"
> >> +#include "tpm_ppi.h"
> >>  #include "trace.h"
> >>
> >>  #define TPM_TIS_NUM_LOCALITIES      5     /* per spec */
> >> @@ -83,6 +84,7 @@ typedef struct TPMState {
> >>      size_t be_buffer_size;
> >>
> >>      bool ppi_enabled;
> >> +    TPMPPI ppi;
> >>  } TPMState;
> >>
> >>  #define TPM(obj) OBJECT_CHECK(TPMState, (obj), TYPE_TPM_TIS)
> >> @@ -979,6 +981,12 @@ static void tpm_tis_realizefn(DeviceState *dev, Error **errp)
> >>
> >>      memory_region_add_subregion(isa_address_space(ISA_DEVICE(dev)),
> >>                                  TPM_TIS_ADDR_BASE, &s->mmio);
> >> +
> >> +    if (s->ppi_enabled &&
> >> +        !tpm_ppi_init(&s->ppi, isa_address_space(ISA_DEVICE(dev)),
> >> +                      TPM_PPI_ADDR_BASE, OBJECT(s), errp)) {
> >> +        return;
> >> +    }
> >>  }
> >>
> >>  static void tpm_tis_initfn(Object *obj)
> >> diff --git a/hw/tpm/Makefile.objs b/hw/tpm/Makefile.objs
> >> index 1dc9f8bf2c..700c878622 100644
> >> --- a/hw/tpm/Makefile.objs
> >> +++ b/hw/tpm/Makefile.objs
> >> @@ -1,4 +1,5 @@
> >>  common-obj-y += tpm_util.o
> >> +obj-y += 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  
> >
> >  
> 
> 
> 

On Tue, Jul 17, 2018 at 9:59 AM, Igor Mammedov <imammedo@redhat.com> wrote:
> On Mon, 16 Jul 2018 16:56:36 +0200
> Marc-André Lureau <marcandre.lureau@gmail.com> wrote:
>
>> Hi
>>
>> On Mon, Jul 16, 2018 at 4:44 PM, Igor Mammedov <imammedo@redhat.com> wrote:
>> > On Mon, 16 Jul 2018 14:59:45 +0200
>> > Marc-André Lureau <marcandre.lureau@redhat.com> wrote:
>> >
>> >> From: Stefan Berger <stefanb@linux.vnet.ibm.com>
>> >>
>> >> Implement a virtual memory device for the TPM Physical Presence interface.
>> >> The memory is located at 0xFED45000 and used by ACPI to send messages to the
>> >> firmware (BIOS) and by the firmware to provide parameters for each one of
>> >> the supported codes.
>> > it doesn't implement anything, maybe something like this would be better?
>> >
>> > tpm: allocate/map buffer for TPM Physical Presence interface
>> >
>> > ...
>> >
>> >>
>> >> This device should be used by all TPM interfaces on x86 and can be added
>> >> by calling tpm_ppi_init_io().
>> > Did you mean:
>> >  This *interface* should be used by all TPM *devices* on x86 and can be added
>> >  by calling tpm_ppi_init_io().
>> >
>> >
>> >> Note: bios_linker cannot be used to allocate the PPI memory region,
>> >> since the reserved memory should stay stable across reboots, and might
>> >> be needed before the ACPI tables are installed.
>> >>
>> >> Signed-off-by: Stefan Berger <stefanb@linux.vnet.ibm.com>
>> >> Signed-off-by: Marc-André Lureau <marcandre.lureau@redhat.com>
>> >> ---
>> >>  hw/tpm/tpm_ppi.h      | 26 ++++++++++++++++++++++++++
>> >>  include/hw/acpi/tpm.h |  6 ++++++
>> >>  hw/tpm/tpm_crb.c      |  8 ++++++++
>> >>  hw/tpm/tpm_ppi.c      | 31 +++++++++++++++++++++++++++++++
>> >>  hw/tpm/tpm_tis.c      |  8 ++++++++
>> >>  hw/tpm/Makefile.objs  |  1 +
>> >>  6 files changed, 80 insertions(+)
>> >>  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..f6458bf87e
>> >> --- /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 ram;
>> >> +    uint8_t buf[TPM_PPI_ADDR_SIZE];
>> >> +} TPMPPI;
>> >> +
>> >> +bool tpm_ppi_init(TPMPPI *tpmppi, struct MemoryRegion *m,
>> >> +                  hwaddr addr, Object *obj, Error **errp);
>> >> +
>> >> +#endif /* TPM_TPM_PPI_H */
>> >> diff --git a/include/hw/acpi/tpm.h b/include/hw/acpi/tpm.h
>> >> index 3580ffd50c..b8796df916 100644
>> >> --- a/include/hw/acpi/tpm.h
>> >> +++ b/include/hw/acpi/tpm.h
>> >> @@ -188,4 +188,10 @@ REG32(CRB_DATA_BUFFER, 0x80)
>> >>  #define TPM2_START_METHOD_MMIO      6
>> >>  #define TPM2_START_METHOD_CRB       7
>> >>
>> >> +/*
>> >> + * Physical Presence Interface
>> >> + */
>> >> +#define TPM_PPI_ADDR_SIZE           0x400
>> >> +#define TPM_PPI_ADDR_BASE           0xFED45000
>> >> +
>> >>  #endif /* HW_ACPI_TPM_H */
>> >> diff --git a/hw/tpm/tpm_crb.c b/hw/tpm/tpm_crb.c
>> >> index d5b0ac5920..b243222fd6 100644
>> >> --- a/hw/tpm/tpm_crb.c
>> >> +++ b/hw/tpm/tpm_crb.c
>> >> @@ -29,6 +29,7 @@
>> >>  #include "sysemu/reset.h"
>> >>  #include "tpm_int.h"
>> >>  #include "tpm_util.h"
>> >> +#include "tpm_ppi.h"
>> >>  #include "trace.h"
>> >>
>> >>  typedef struct CRBState {
>> >> @@ -43,6 +44,7 @@ typedef struct CRBState {
>> >>      size_t be_buffer_size;
>> >>
>> >>      bool ppi_enabled;
>> >> +    TPMPPI ppi;
>> >>  } CRBState;
>> >>
>> >>  #define CRB(obj) OBJECT_CHECK(CRBState, (obj), TYPE_TPM_CRB)
>> >> @@ -294,6 +296,12 @@ static void tpm_crb_realize(DeviceState *dev, Error **errp)
>> >>      memory_region_add_subregion(get_system_memory(),
>> >>          TPM_CRB_ADDR_BASE + sizeof(s->regs), &s->cmdmem);
>> >>
>> >> +    if (s->ppi_enabled &&
>> >> +        !tpm_ppi_init(&s->ppi, get_system_memory(),
>> >> +                      TPM_PPI_ADDR_BASE, OBJECT(s), errp)) {
>> >> +        return;
>> >> +    }
>> >> +
>> >>      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..8b46b9dd4b
>> >> --- /dev/null
>> >> +++ b/hw/tpm/tpm_ppi.c
>> >> @@ -0,0 +1,31 @@
>> >> +/*
>> >> + * 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 "qapi/error.h"
>> >> +#include "cpu.h"
>> >> +#include "sysemu/memory_mapping.h"
>> >> +#include "migration/vmstate.h"
>> >> +#include "tpm_ppi.h"
>> >> +
>> >> +bool tpm_ppi_init(TPMPPI *tpmppi, struct MemoryRegion *m,
>> >> +                  hwaddr addr, Object *obj, Error **errp)
>> >> +{
>> >> +    memory_region_init_ram_device_ptr(&tpmppi->ram, obj, "tpm-ppi",
>> >> +                                      TPM_PPI_ADDR_SIZE, tpmppi->buf);
>> > why use buf and not a plain ram memory region?
>>
>> It shouldn't be treated as regular RAM. (for example when iterating
>> guest_phys_blocks for dump or memory clear in last patch)
> why it shouldn't, I'd think having ppi flags in dump would be useful

We don't dump device memory.

> and why it shouldn't be cleared on reset along with all other ram?

It's a device memory, not system memory. The spec talks only about
system memory RAM, and Stefan also mentioned that we should not touch
device/tpm RAM in previous review.

However, we may clear the MOR bit after reset (shown in Figure 2 in
the spec - "the MemoryOverwriteRequestControl EFI variable described
in Section 5 can be updated to clear the MOR bit after secrets have
been removed from memory")

>>
>> >
>> >> +    vmstate_register_ram(&tpmppi->ram, DEVICE(obj));
>> >> +
>> >> +    memory_region_add_subregion(m, addr, &tpmppi->ram);
>> >> +    return true;
>> >> +}
>> >> diff --git a/hw/tpm/tpm_tis.c b/hw/tpm/tpm_tis.c
>> >> index d9ddf9b723..70432ffe8b 100644
>> >> --- a/hw/tpm/tpm_tis.c
>> >> +++ b/hw/tpm/tpm_tis.c
>> >> @@ -31,6 +31,7 @@
>> >>  #include "sysemu/tpm_backend.h"
>> >>  #include "tpm_int.h"
>> >>  #include "tpm_util.h"
>> >> +#include "tpm_ppi.h"
>> >>  #include "trace.h"
>> >>
>> >>  #define TPM_TIS_NUM_LOCALITIES      5     /* per spec */
>> >> @@ -83,6 +84,7 @@ typedef struct TPMState {
>> >>      size_t be_buffer_size;
>> >>
>> >>      bool ppi_enabled;
>> >> +    TPMPPI ppi;
>> >>  } TPMState;
>> >>
>> >>  #define TPM(obj) OBJECT_CHECK(TPMState, (obj), TYPE_TPM_TIS)
>> >> @@ -979,6 +981,12 @@ static void tpm_tis_realizefn(DeviceState *dev, Error **errp)
>> >>
>> >>      memory_region_add_subregion(isa_address_space(ISA_DEVICE(dev)),
>> >>                                  TPM_TIS_ADDR_BASE, &s->mmio);
>> >> +
>> >> +    if (s->ppi_enabled &&
>> >> +        !tpm_ppi_init(&s->ppi, isa_address_space(ISA_DEVICE(dev)),
>> >> +                      TPM_PPI_ADDR_BASE, OBJECT(s), errp)) {
>> >> +        return;
>> >> +    }
>> >>  }
>> >>
>> >>  static void tpm_tis_initfn(Object *obj)
>> >> diff --git a/hw/tpm/Makefile.objs b/hw/tpm/Makefile.objs
>> >> index 1dc9f8bf2c..700c878622 100644
>> >> --- a/hw/tpm/Makefile.objs
>> >> +++ b/hw/tpm/Makefile.objs
>> >> @@ -1,4 +1,5 @@
>> >>  common-obj-y += tpm_util.o
>> >> +obj-y += 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
>> >
>> >
>>
>>
>>
>



-- 
Marc-André Lureau

On Tue, 17 Jul 2018 12:22:49 +0200
Marc-André Lureau <marcandre.lureau@gmail.com> wrote:

> On Tue, Jul 17, 2018 at 9:59 AM, Igor Mammedov <imammedo@redhat.com> wrote:
> > On Mon, 16 Jul 2018 16:56:36 +0200
> > Marc-André Lureau <marcandre.lureau@gmail.com> wrote:
> >  
> >> Hi
> >>
> >> On Mon, Jul 16, 2018 at 4:44 PM, Igor Mammedov <imammedo@redhat.com> wrote:  
> >> > On Mon, 16 Jul 2018 14:59:45 +0200
> >> > Marc-André Lureau <marcandre.lureau@redhat.com> wrote:
> >> >  
> >> >> From: Stefan Berger <stefanb@linux.vnet.ibm.com>
> >> >>
> >> >> Implement a virtual memory device for the TPM Physical Presence interface.
> >> >> The memory is located at 0xFED45000 and used by ACPI to send messages to the
> >> >> firmware (BIOS) and by the firmware to provide parameters for each one of
> >> >> the supported codes.  
> >> > it doesn't implement anything, maybe something like this would be better?
> >> >
> >> > tpm: allocate/map buffer for TPM Physical Presence interface
> >> >
> >> > ...
> >> >  
> >> >>
> >> >> This device should be used by all TPM interfaces on x86 and can be added
> >> >> by calling tpm_ppi_init_io().  
> >> > Did you mean:
> >> >  This *interface* should be used by all TPM *devices* on x86 and can be added
> >> >  by calling tpm_ppi_init_io().
> >> >
> >> >  
> >> >> Note: bios_linker cannot be used to allocate the PPI memory region,
> >> >> since the reserved memory should stay stable across reboots, and might
> >> >> be needed before the ACPI tables are installed.
> >> >>
> >> >> Signed-off-by: Stefan Berger <stefanb@linux.vnet.ibm.com>
> >> >> Signed-off-by: Marc-André Lureau <marcandre.lureau@redhat.com>
> >> >> ---
> >> >>  hw/tpm/tpm_ppi.h      | 26 ++++++++++++++++++++++++++
> >> >>  include/hw/acpi/tpm.h |  6 ++++++
> >> >>  hw/tpm/tpm_crb.c      |  8 ++++++++
> >> >>  hw/tpm/tpm_ppi.c      | 31 +++++++++++++++++++++++++++++++
> >> >>  hw/tpm/tpm_tis.c      |  8 ++++++++
> >> >>  hw/tpm/Makefile.objs  |  1 +
> >> >>  6 files changed, 80 insertions(+)
> >> >>  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..f6458bf87e
> >> >> --- /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 ram;
> >> >> +    uint8_t buf[TPM_PPI_ADDR_SIZE];
> >> >> +} TPMPPI;
> >> >> +
> >> >> +bool tpm_ppi_init(TPMPPI *tpmppi, struct MemoryRegion *m,
> >> >> +                  hwaddr addr, Object *obj, Error **errp);
> >> >> +
> >> >> +#endif /* TPM_TPM_PPI_H */
> >> >> diff --git a/include/hw/acpi/tpm.h b/include/hw/acpi/tpm.h
> >> >> index 3580ffd50c..b8796df916 100644
> >> >> --- a/include/hw/acpi/tpm.h
> >> >> +++ b/include/hw/acpi/tpm.h
> >> >> @@ -188,4 +188,10 @@ REG32(CRB_DATA_BUFFER, 0x80)
> >> >>  #define TPM2_START_METHOD_MMIO      6
> >> >>  #define TPM2_START_METHOD_CRB       7
> >> >>
> >> >> +/*
> >> >> + * Physical Presence Interface
> >> >> + */
> >> >> +#define TPM_PPI_ADDR_SIZE           0x400
> >> >> +#define TPM_PPI_ADDR_BASE           0xFED45000
> >> >> +
> >> >>  #endif /* HW_ACPI_TPM_H */
> >> >> diff --git a/hw/tpm/tpm_crb.c b/hw/tpm/tpm_crb.c
> >> >> index d5b0ac5920..b243222fd6 100644
> >> >> --- a/hw/tpm/tpm_crb.c
> >> >> +++ b/hw/tpm/tpm_crb.c
> >> >> @@ -29,6 +29,7 @@
> >> >>  #include "sysemu/reset.h"
> >> >>  #include "tpm_int.h"
> >> >>  #include "tpm_util.h"
> >> >> +#include "tpm_ppi.h"
> >> >>  #include "trace.h"
> >> >>
> >> >>  typedef struct CRBState {
> >> >> @@ -43,6 +44,7 @@ typedef struct CRBState {
> >> >>      size_t be_buffer_size;
> >> >>
> >> >>      bool ppi_enabled;
> >> >> +    TPMPPI ppi;
> >> >>  } CRBState;
> >> >>
> >> >>  #define CRB(obj) OBJECT_CHECK(CRBState, (obj), TYPE_TPM_CRB)
> >> >> @@ -294,6 +296,12 @@ static void tpm_crb_realize(DeviceState *dev, Error **errp)
> >> >>      memory_region_add_subregion(get_system_memory(),
> >> >>          TPM_CRB_ADDR_BASE + sizeof(s->regs), &s->cmdmem);
> >> >>
> >> >> +    if (s->ppi_enabled &&
> >> >> +        !tpm_ppi_init(&s->ppi, get_system_memory(),
> >> >> +                      TPM_PPI_ADDR_BASE, OBJECT(s), errp)) {
> >> >> +        return;
> >> >> +    }
> >> >> +
> >> >>      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..8b46b9dd4b
> >> >> --- /dev/null
> >> >> +++ b/hw/tpm/tpm_ppi.c
> >> >> @@ -0,0 +1,31 @@
> >> >> +/*
> >> >> + * 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 "qapi/error.h"
> >> >> +#include "cpu.h"
> >> >> +#include "sysemu/memory_mapping.h"
> >> >> +#include "migration/vmstate.h"
> >> >> +#include "tpm_ppi.h"
> >> >> +
> >> >> +bool tpm_ppi_init(TPMPPI *tpmppi, struct MemoryRegion *m,
> >> >> +                  hwaddr addr, Object *obj, Error **errp)
> >> >> +{
> >> >> +    memory_region_init_ram_device_ptr(&tpmppi->ram, obj, "tpm-ppi",
> >> >> +                                      TPM_PPI_ADDR_SIZE, tpmppi->buf);  
> >> > why use buf and not a plain ram memory region?  
> >>
> >> It shouldn't be treated as regular RAM. (for example when iterating
> >> guest_phys_blocks for dump or memory clear in last patch)  
> > why it shouldn't, I'd think having ppi flags in dump would be useful  
> 
> We don't dump device memory.
> 
> > and why it shouldn't be cleared on reset along with all other ram?  
> 
> It's a device memory, not system memory. The spec talks only about
> system memory RAM, and Stefan also mentioned that we should not touch
> device/tpm RAM in previous review.
ok


> However, we may clear the MOR bit after reset (shown in Figure 2 in
> the spec - "the MemoryOverwriteRequestControl EFI variable described
> in Section 5 can be updated to clear the MOR bit after secrets have
> been removed from memory")
Also spec talks about MOR flags being saved in nonvolatile memory
so it would survive S4/S5 states. It's not the case with this
series but we probably should care since backend for ram/nvdimm
could be a persistent file.

Interesting, what should we do in case of nvdimm if it holds
an image of OS and aren't supposed to be cleared on reboot when
platform still sees it as part of RAM?

> 
> >>  
> >> >  
> >> >> +    vmstate_register_ram(&tpmppi->ram, DEVICE(obj));
> >> >> +
> >> >> +    memory_region_add_subregion(m, addr, &tpmppi->ram);
> >> >> +    return true;
> >> >> +}
> >> >> diff --git a/hw/tpm/tpm_tis.c b/hw/tpm/tpm_tis.c
> >> >> index d9ddf9b723..70432ffe8b 100644
> >> >> --- a/hw/tpm/tpm_tis.c
> >> >> +++ b/hw/tpm/tpm_tis.c
> >> >> @@ -31,6 +31,7 @@
> >> >>  #include "sysemu/tpm_backend.h"
> >> >>  #include "tpm_int.h"
> >> >>  #include "tpm_util.h"
> >> >> +#include "tpm_ppi.h"
> >> >>  #include "trace.h"
> >> >>
> >> >>  #define TPM_TIS_NUM_LOCALITIES      5     /* per spec */
> >> >> @@ -83,6 +84,7 @@ typedef struct TPMState {
> >> >>      size_t be_buffer_size;
> >> >>
> >> >>      bool ppi_enabled;
> >> >> +    TPMPPI ppi;
> >> >>  } TPMState;
> >> >>
> >> >>  #define TPM(obj) OBJECT_CHECK(TPMState, (obj), TYPE_TPM_TIS)
> >> >> @@ -979,6 +981,12 @@ static void tpm_tis_realizefn(DeviceState *dev, Error **errp)
> >> >>
> >> >>      memory_region_add_subregion(isa_address_space(ISA_DEVICE(dev)),
> >> >>                                  TPM_TIS_ADDR_BASE, &s->mmio);
> >> >> +
> >> >> +    if (s->ppi_enabled &&
> >> >> +        !tpm_ppi_init(&s->ppi, isa_address_space(ISA_DEVICE(dev)),
> >> >> +                      TPM_PPI_ADDR_BASE, OBJECT(s), errp)) {
> >> >> +        return;
> >> >> +    }
> >> >>  }
> >> >>
> >> >>  static void tpm_tis_initfn(Object *obj)
> >> >> diff --git a/hw/tpm/Makefile.objs b/hw/tpm/Makefile.objs
> >> >> index 1dc9f8bf2c..700c878622 100644
> >> >> --- a/hw/tpm/Makefile.objs
> >> >> +++ b/hw/tpm/Makefile.objs
> >> >> @@ -1,4 +1,5 @@
> >> >>  common-obj-y += tpm_util.o
> >> >> +obj-y += 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  
> >> >
> >> >  
> >>
> >>
> >>  
> >  
> 
> 
> 

On Mon, 16 Jul 2018 14:59:45 +0200
Marc-André Lureau <marcandre.lureau@redhat.com> wrote:

> From: Stefan Berger <stefanb@linux.vnet.ibm.com>
> 
> Implement a virtual memory device for the TPM Physical Presence interface.
> The memory is located at 0xFED45000 and used by ACPI to send messages to the
> firmware (BIOS) and by the firmware to provide parameters for each one of
> the supported codes.
...
another question wrt E820 table, is firmware putting a reservation record
for this RAM area overlay or should we do it QEMU?

Hi

On Tue, Jul 17, 2018 at 12:03 PM, Igor Mammedov <imammedo@redhat.com> wrote:
> On Mon, 16 Jul 2018 14:59:45 +0200
> Marc-André Lureau <marcandre.lureau@redhat.com> wrote:
>
>> From: Stefan Berger <stefanb@linux.vnet.ibm.com>
>>
>> Implement a virtual memory device for the TPM Physical Presence interface.
>> The memory is located at 0xFED45000 and used by ACPI to send messages to the
>> firmware (BIOS) and by the firmware to provide parameters for each one of
>> the supported codes.
> ...
> another question wrt E820 table, is firmware putting a reservation record
> for this RAM area overlay or should we do it QEMU?

There doesn't seem to be any reservation done in ovmf, or on my
laptop. I also wonder if it should be done. And how OS deal with that
today (/proc/iomem is correct).

Laszlo, any idea?

On 07/17/18 12:39, Marc-André Lureau wrote:
> Hi
> 
> On Tue, Jul 17, 2018 at 12:03 PM, Igor Mammedov <imammedo@redhat.com> wrote:
>> On Mon, 16 Jul 2018 14:59:45 +0200
>> Marc-André Lureau <marcandre.lureau@redhat.com> wrote:
>>
>>> From: Stefan Berger <stefanb@linux.vnet.ibm.com>
>>>
>>> Implement a virtual memory device for the TPM Physical Presence interface.
>>> The memory is located at 0xFED45000 and used by ACPI to send messages to the
>>> firmware (BIOS) and by the firmware to provide parameters for each one of
>>> the supported codes.
>> ...
>> another question wrt E820 table, is firmware putting a reservation record
>> for this RAM area overlay or should we do it QEMU?

What OS requires this memmap entry?

> There doesn't seem to be any reservation done in ovmf, or on my
> laptop.

Relevant point, about your laptop.

> I also wonder if it should be done.

IMO: no, it shouldn't.

> And how OS deal with that today (/proc/iomem is correct).
> 
> Laszlo, any idea?

In my experience, exposing such MMIO areas that belong to *platform*
devices, in the UEFI memory map, is an inconsistent practice. I can list
at least four different cases:

(1) A UEFI runtime driver is using the MMIO area for providing a UEFI
runtime service. In this case, the firmware must expose the area as
"MMIO" type memory, and set the "runtime" attribute on it. In response,
the OS will map the area with virtual addresses, directly in response to
the memmap entry.

(2) A firmware SMM driver is using the MMIO area for providing a
privileged part of a UEFI runtime service. Because SMM uses its own page
table anyway (protected from the OS), advertizing the area in the UEFI
memmap is unneeded. It can simply be left as a gap.

(3) Linux requires the firmware to cover every MMCONFIG area (each of
which belongs to a PCIe host bridge, which is a platform device) with a
"reserved" type memmap entry. To me this looks like an outlier, although
I *very* vaguely recall that some spec recommended this.

Ah, right, the PCI Firmware Specification says, under "MCFG Table
Description": "*If* the operating system does not natively comprehend
reserving the MMCFG region, the MMCFG region must be reserved by
firmware." (Emphasis mine.) In other words, this is a provision in the
PCI firmware spec so that platform firmware paper over the fact that the
ACPI subsystem in the OS kernel is limited. Linux, given that it
understand MCFG just fine, totally should not *require* the firmware to
reserve this region in the UEFI memory map as well. Anyway, Linux does
require it (incorrectly, in my opinion), so OVMF complies.

(4) To my understanding, unless given Linux or Windows quirks like (3),
or UEFI runtime drivers like (1), we shouldn't expose, in the UEFI
memmap, MMIO areas related to platform devices. In particular, the
"MMIO" type memory *without* the "runtime" attribute (see (1)) should
never be used in the UEFI memmap (and edk2 will never create such
entries) -- the UEFI spec says that all such areas (resources) should be
communicated through ACPI tables ("All system memory-mapped IO
information should come from ACPI tables" -- see EfiMemoryMappedIO in
"Table 26. Memory Type Usage after ExitBootServices()").


In brief, the TPM PPI MMIO area should just be left as a gap in the UEFI
memmap. (Case (4) applies.)

Thanks,
Laszlo

On Tue, 17 Jul 2018 15:04:25 +0200
Laszlo Ersek <lersek@redhat.com> wrote:

> On 07/17/18 12:39, Marc-André Lureau wrote:
> > Hi
> > 
> > On Tue, Jul 17, 2018 at 12:03 PM, Igor Mammedov <imammedo@redhat.com> wrote:  
> >> On Mon, 16 Jul 2018 14:59:45 +0200
> >> Marc-André Lureau <marcandre.lureau@redhat.com> wrote:
> >>  
> >>> From: Stefan Berger <stefanb@linux.vnet.ibm.com>
> >>>
> >>> Implement a virtual memory device for the TPM Physical Presence interface.
> >>> The memory is located at 0xFED45000 and used by ACPI to send messages to the
> >>> firmware (BIOS) and by the firmware to provide parameters for each one of
> >>> the supported codes.  
> >> ...
> >> another question wrt E820 table, is firmware putting a reservation record
> >> for this RAM area overlay or should we do it QEMU?  
> 
> What OS requires this memmap entry?
> 
> > There doesn't seem to be any reservation done in ovmf, or on my
> > laptop.  
> 
> Relevant point, about your laptop.
> 
> > I also wonder if it should be done.  
> 
> IMO: no, it shouldn't.
> 
> > And how OS deal with that today (/proc/iomem is correct).
> > 
> > Laszlo, any idea?  
> 
> In my experience, exposing such MMIO areas that belong to *platform*
> devices, in the UEFI memory map, is an inconsistent practice. I can list
> at least four different cases:
> 
> (1) A UEFI runtime driver is using the MMIO area for providing a UEFI
> runtime service. In this case, the firmware must expose the area as
> "MMIO" type memory, and set the "runtime" attribute on it. In response,
> the OS will map the area with virtual addresses, directly in response to
> the memmap entry.
> 
> (2) A firmware SMM driver is using the MMIO area for providing a
> privileged part of a UEFI runtime service. Because SMM uses its own page
> table anyway (protected from the OS), advertizing the area in the UEFI
> memmap is unneeded. It can simply be left as a gap.
> 
> (3) Linux requires the firmware to cover every MMCONFIG area (each of
> which belongs to a PCIe host bridge, which is a platform device) with a
> "reserved" type memmap entry. To me this looks like an outlier, although
> I *very* vaguely recall that some spec recommended this.
> 
> Ah, right, the PCI Firmware Specification says, under "MCFG Table
> Description": "*If* the operating system does not natively comprehend
> reserving the MMCFG region, the MMCFG region must be reserved by
> firmware." (Emphasis mine.) In other words, this is a provision in the
> PCI firmware spec so that platform firmware paper over the fact that the
> ACPI subsystem in the OS kernel is limited. Linux, given that it
> understand MCFG just fine, totally should not *require* the firmware to
> reserve this region in the UEFI memory map as well. Anyway, Linux does
> require it (incorrectly, in my opinion), so OVMF complies.
> 
> (4) To my understanding, unless given Linux or Windows quirks like (3),
> or UEFI runtime drivers like (1), we shouldn't expose, in the UEFI
> memmap, MMIO areas related to platform devices. In particular, the
> "MMIO" type memory *without* the "runtime" attribute (see (1)) should
> never be used in the UEFI memmap (and edk2 will never create such
> entries) -- the UEFI spec says that all such areas (resources) should be
> communicated through ACPI tables ("All system memory-mapped IO
> information should come from ACPI tables" -- see EfiMemoryMappedIO in
> "Table 26. Memory Type Usage after ExitBootServices()").
> 
> 
> In brief, the TPM PPI MMIO area should just be left as a gap in the UEFI
> memmap. (Case (4) applies.)
so I guess we are good as the patch should fit the bill for (4)

> 
> Thanks,
> Laszlo

On Mon, 16 Jul 2018 14:59:45 +0200
Marc-André Lureau <marcandre.lureau@redhat.com> wrote:

> From: Stefan Berger <stefanb@linux.vnet.ibm.com>
> 
> Implement a virtual memory device for the TPM Physical Presence interface.
> The memory is located at 0xFED45000 and used by ACPI to send messages to the
> firmware (BIOS) and by the firmware to provide parameters for each one of
> the supported codes.
> 
> This device should be used by all TPM interfaces on x86 and can be added
> by calling tpm_ppi_init_io().
> 
> Note: bios_linker cannot be used to allocate the PPI memory region,
> since the reserved memory should stay stable across reboots, and might
> be needed before the ACPI tables are installed.
> 
> Signed-off-by: Stefan Berger <stefanb@linux.vnet.ibm.com>
> Signed-off-by: Marc-André Lureau <marcandre.lureau@redhat.com>

with commit message fixed as suggested earlier

Reviewed-by: Igor Mammedov <imammedo@redhat.com>

> ---
>  hw/tpm/tpm_ppi.h      | 26 ++++++++++++++++++++++++++
>  include/hw/acpi/tpm.h |  6 ++++++
>  hw/tpm/tpm_crb.c      |  8 ++++++++
>  hw/tpm/tpm_ppi.c      | 31 +++++++++++++++++++++++++++++++
>  hw/tpm/tpm_tis.c      |  8 ++++++++
>  hw/tpm/Makefile.objs  |  1 +
>  6 files changed, 80 insertions(+)
>  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..f6458bf87e
> --- /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 ram;
> +    uint8_t buf[TPM_PPI_ADDR_SIZE];
> +} TPMPPI;
> +
> +bool tpm_ppi_init(TPMPPI *tpmppi, struct MemoryRegion *m,
> +                  hwaddr addr, Object *obj, Error **errp);
> +
> +#endif /* TPM_TPM_PPI_H */
> diff --git a/include/hw/acpi/tpm.h b/include/hw/acpi/tpm.h
> index 3580ffd50c..b8796df916 100644
> --- a/include/hw/acpi/tpm.h
> +++ b/include/hw/acpi/tpm.h
> @@ -188,4 +188,10 @@ REG32(CRB_DATA_BUFFER, 0x80)
>  #define TPM2_START_METHOD_MMIO      6
>  #define TPM2_START_METHOD_CRB       7
>  
> +/*
> + * Physical Presence Interface
> + */
> +#define TPM_PPI_ADDR_SIZE           0x400
> +#define TPM_PPI_ADDR_BASE           0xFED45000
> +
>  #endif /* HW_ACPI_TPM_H */
> diff --git a/hw/tpm/tpm_crb.c b/hw/tpm/tpm_crb.c
> index d5b0ac5920..b243222fd6 100644
> --- a/hw/tpm/tpm_crb.c
> +++ b/hw/tpm/tpm_crb.c
> @@ -29,6 +29,7 @@
>  #include "sysemu/reset.h"
>  #include "tpm_int.h"
>  #include "tpm_util.h"
> +#include "tpm_ppi.h"
>  #include "trace.h"
>  
>  typedef struct CRBState {
> @@ -43,6 +44,7 @@ typedef struct CRBState {
>      size_t be_buffer_size;
>  
>      bool ppi_enabled;
> +    TPMPPI ppi;
>  } CRBState;
>  
>  #define CRB(obj) OBJECT_CHECK(CRBState, (obj), TYPE_TPM_CRB)
> @@ -294,6 +296,12 @@ static void tpm_crb_realize(DeviceState *dev, Error **errp)
>      memory_region_add_subregion(get_system_memory(),
>          TPM_CRB_ADDR_BASE + sizeof(s->regs), &s->cmdmem);
>  
> +    if (s->ppi_enabled &&
> +        !tpm_ppi_init(&s->ppi, get_system_memory(),
> +                      TPM_PPI_ADDR_BASE, OBJECT(s), errp)) {
> +        return;
> +    }
> +
>      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..8b46b9dd4b
> --- /dev/null
> +++ b/hw/tpm/tpm_ppi.c
> @@ -0,0 +1,31 @@
> +/*
> + * 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 "qapi/error.h"
> +#include "cpu.h"
> +#include "sysemu/memory_mapping.h"
> +#include "migration/vmstate.h"
> +#include "tpm_ppi.h"
> +
> +bool tpm_ppi_init(TPMPPI *tpmppi, struct MemoryRegion *m,
> +                  hwaddr addr, Object *obj, Error **errp)
> +{
> +    memory_region_init_ram_device_ptr(&tpmppi->ram, obj, "tpm-ppi",
> +                                      TPM_PPI_ADDR_SIZE, tpmppi->buf);
> +    vmstate_register_ram(&tpmppi->ram, DEVICE(obj));
> +
> +    memory_region_add_subregion(m, addr, &tpmppi->ram);
> +    return true;
> +}
> diff --git a/hw/tpm/tpm_tis.c b/hw/tpm/tpm_tis.c
> index d9ddf9b723..70432ffe8b 100644
> --- a/hw/tpm/tpm_tis.c
> +++ b/hw/tpm/tpm_tis.c
> @@ -31,6 +31,7 @@
>  #include "sysemu/tpm_backend.h"
>  #include "tpm_int.h"
>  #include "tpm_util.h"
> +#include "tpm_ppi.h"
>  #include "trace.h"
>  
>  #define TPM_TIS_NUM_LOCALITIES      5     /* per spec */
> @@ -83,6 +84,7 @@ typedef struct TPMState {
>      size_t be_buffer_size;
>  
>      bool ppi_enabled;
> +    TPMPPI ppi;
>  } TPMState;
>  
>  #define TPM(obj) OBJECT_CHECK(TPMState, (obj), TYPE_TPM_TIS)
> @@ -979,6 +981,12 @@ static void tpm_tis_realizefn(DeviceState *dev, Error **errp)
>  
>      memory_region_add_subregion(isa_address_space(ISA_DEVICE(dev)),
>                                  TPM_TIS_ADDR_BASE, &s->mmio);
> +
> +    if (s->ppi_enabled &&
> +        !tpm_ppi_init(&s->ppi, isa_address_space(ISA_DEVICE(dev)),
> +                      TPM_PPI_ADDR_BASE, OBJECT(s), errp)) {
> +        return;
> +    }
>  }
>  
>  static void tpm_tis_initfn(Object *obj)
> diff --git a/hw/tpm/Makefile.objs b/hw/tpm/Makefile.objs
> index 1dc9f8bf2c..700c878622 100644
> --- a/hw/tpm/Makefile.objs
> +++ b/hw/tpm/Makefile.objs
> @@ -1,4 +1,5 @@
>  common-obj-y += tpm_util.o
> +obj-y += 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