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

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

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

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

---

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

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

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

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

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

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

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


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

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

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


    Stefan

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

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

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

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

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

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

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



-- 
Marc-André Lureau

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

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

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

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

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

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


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



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

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

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

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

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

CCing Laszlo just in case if I'm wrong.


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

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

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

Thanks
Laszlo