[Qemu-devel] [PATCH v10 0/6] Add support for TPM Physical Presence interface

[Qemu-devel] [PATCH v10 0/6] Add support for TPM Physical Presence interface

[Marc-André Lureau]

Hi,

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

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

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

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

The edk2 support is merged upstream.

v10:
- fix 3.1 pc machines patch
- describe PPI memory size in doc
- change "Memory overwrite variable" location to offset 0x15a

Marc-André Lureau (3):
  hw/i386: add pc-i440fx-3.1 & pc-q35-3.1
  tpm: add a "ppi" boolean property
  tpm: add ACPI memory clear interface

Stefan Berger (3):
  tpm: allocate/map buffer for TPM Physical Presence interface
  acpi: expose TPM/PPI configuration parameters to firmware via fw_cfg
  acpi: build TPM Physical Presence interface

 hw/tpm/tpm_ppi.h      |  28 +++
 include/hw/acpi/tpm.h |  17 ++
 include/hw/compat.h   |  11 +-
 include/hw/i386/pc.h  |   5 +-
 hw/i386/acpi-build.c  | 456 +++++++++++++++++++++++++++++++++++++++++-
 hw/i386/pc_piix.c     |  15 +-
 hw/i386/pc_q35.c      |  13 +-
 hw/tpm/tpm_crb.c      |  12 ++
 hw/tpm/tpm_ppi.c      |  54 +++++
 hw/tpm/tpm_tis.c      |  12 ++
 docs/specs/tpm.txt    | 105 ++++++++++
 hw/tpm/Makefile.objs  |   1 +
 hw/tpm/trace-events   |   3 +
 13 files changed, 723 insertions(+), 9 deletions(-)
 create mode 100644 hw/tpm/tpm_ppi.h
 create mode 100644 hw/tpm/tpm_ppi.c

-- 
2.19.0.rc1

[Qemu-devel] [PATCH v10 1/6] hw/i386: add pc-i440fx-3.1 & pc-q35-3.1

[Marc-André Lureau]

The following patch is going to add compatiblity parameters.

Signed-off-by: Marc-André Lureau <marcandre.lureau@redhat.com>
---
 include/hw/i386/pc.h |  5 ++++-
 hw/i386/pc_piix.c    | 15 ++++++++++++---
 hw/i386/pc_q35.c     | 13 +++++++++++--
 3 files changed, 27 insertions(+), 6 deletions(-)

diff --git a/include/hw/i386/pc.h b/include/hw/i386/pc.h
index 6894f37df1..09b0365a8e 100644
--- a/include/hw/i386/pc.h
+++ b/include/hw/i386/pc.h
@@ -294,7 +294,10 @@ int e820_add_entry(uint64_t, uint64_t, uint32_t);
 int e820_get_num_entries(void);
 bool e820_get_entry(int, uint32_t, uint64_t *, uint64_t *);
 
-#define PC_COMPAT_2_12 \
+#define PC_COMPAT_3_0 \
+    HW_COMPAT_3_0
+
+#define PC_COMPAT_2_12                          \
     HW_COMPAT_2_12 \
     {\
         .driver   = TYPE_X86_CPU,\
diff --git a/hw/i386/pc_piix.c b/hw/i386/pc_piix.c
index dc09466b3e..7092d6d13f 100644
--- a/hw/i386/pc_piix.c
+++ b/hw/i386/pc_piix.c
@@ -428,21 +428,30 @@ static void pc_i440fx_machine_options(MachineClass *m)
     machine_class_allow_dynamic_sysbus_dev(m, TYPE_RAMFB_DEVICE);
 }
 
-static void pc_i440fx_3_0_machine_options(MachineClass *m)
+static void pc_i440fx_3_1_machine_options(MachineClass *m)
 {
     pc_i440fx_machine_options(m);
     m->alias = "pc";
     m->is_default = 1;
 }
 
+DEFINE_I440FX_MACHINE(v3_1, "pc-i440fx-3.1", NULL,
+                      pc_i440fx_3_1_machine_options);
+
+static void pc_i440fx_3_0_machine_options(MachineClass *m)
+{
+    pc_i440fx_3_1_machine_options(m);
+    m->is_default = 0;
+    m->alias = NULL;
+    SET_MACHINE_COMPAT(m, PC_COMPAT_3_0);
+}
+
 DEFINE_I440FX_MACHINE(v3_0, "pc-i440fx-3.0", NULL,
                       pc_i440fx_3_0_machine_options);
 
 static void pc_i440fx_2_12_machine_options(MachineClass *m)
 {
     pc_i440fx_3_0_machine_options(m);
-    m->is_default = 0;
-    m->alias = NULL;
     SET_MACHINE_COMPAT(m, PC_COMPAT_2_12);
 }
 
diff --git a/hw/i386/pc_q35.c b/hw/i386/pc_q35.c
index 532241e3f8..4702bb13c4 100644
--- a/hw/i386/pc_q35.c
+++ b/hw/i386/pc_q35.c
@@ -311,19 +311,28 @@ static void pc_q35_machine_options(MachineClass *m)
     m->max_cpus = 288;
 }
 
-static void pc_q35_3_0_machine_options(MachineClass *m)
+static void pc_q35_3_1_machine_options(MachineClass *m)
 {
     pc_q35_machine_options(m);
     m->alias = "q35";
 }
 
+DEFINE_Q35_MACHINE(v3_1, "pc-q35-3.1", NULL,
+                   pc_q35_3_1_machine_options);
+
+static void pc_q35_3_0_machine_options(MachineClass *m)
+{
+    pc_q35_3_1_machine_options(m);
+    m->alias = NULL;
+    SET_MACHINE_COMPAT(m, PC_COMPAT_3_0);
+}
+
 DEFINE_Q35_MACHINE(v3_0, "pc-q35-3.0", NULL,
                     pc_q35_3_0_machine_options);
 
 static void pc_q35_2_12_machine_options(MachineClass *m)
 {
     pc_q35_3_0_machine_options(m);
-    m->alias = NULL;
     SET_MACHINE_COMPAT(m, PC_COMPAT_2_12);
 }
 
-- 
2.19.0.rc1

[Qemu-devel] [PATCH v10 2/6] tpm: add a "ppi" boolean property

[Marc-André Lureau]

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

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

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

[Qemu-devel] [PATCH v10 3/6] tpm: allocate/map buffer for TPM Physical Presence interface

[Marc-André Lureau]

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

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

This 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>
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
-- 
2.19.0.rc1

Re: [Qemu-devel] [PATCH v10 3/6] tpm: allocate/map buffer for TPM Physical Presence interface

[Marc-André Lureau]

Hi

On Fri, Aug 31, 2018 at 7:32 PM 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 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>
> 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);

There is a (new) issue with the PPI ram region:

READ of size 32 at 0x61d000090480 thread T6
    #0 0x5622bd8de0f4 in buffer_zero_avx2
/home/elmarco/src/qq/util/bufferiszero.c:169
    #1 0x5622bd8de899 in select_accel_fn
/home/elmarco/src/qq/util/bufferiszero.c:282
    #2 0x5622bd8de8f1 in buffer_is_zero
/home/elmarco/src/qq/util/bufferiszero.c:309
    #3 0x5622bc209f94 in is_zero_range /home/elmarco/src/qq/migration/ram.c:82
    #4 0x5622bc21938d in save_zero_page_to_file
/home/elmarco/src/qq/migration/ram.c:1694
    #5 0x5622bc219452 in save_zero_page
/home/elmarco/src/qq/migration/ram.c:1713
    #6 0x5622bc21db67 in ram_save_target_page
/home/elmarco/src/qq/migration/ram.c:2289
    #7 0x5622bc21e13e in ram_save_host_page
/home/elmarco/src/qq/migration/ram.c:2351
    #8 0x5622bc21ea3a in ram_find_and_save_block
/home/elmarco/src/qq/migration/ram.c:2413
    #9 0x5622bc223b5d in ram_save_iterate
/home/elmarco/src/qq/migration/ram.c:3193
    #10 0x5622bd16f544 in qemu_savevm_state_iterate
/home/elmarco/src/qq/migration/savevm.c:1103
    #11 0x5622bd157e75 in migration_iteration_run
/home/elmarco/src/qq/migration/migration.c:2897
    #12 0x5622bd15892e in migration_thread
/home/elmarco/src/qq/migration/migration.c:3018
    #13 0x5622bd902f31 in qemu_thread_start
/home/elmarco/src/qq/util/qemu-thread-posix.c:504
    #14 0x7f42f0ef4593 in start_thread (/lib64/libpthread.so.0+0x7593)
    #15 0x7f42f0c280de in clone (/lib64/libc.so.6+0xfa0de)
0x61d000090490 is located 0 bytes to the right of 2064-byte region
[0x61d00008fc80,0x61d000090490)

migration code is assuming RAM is multiple of TARGET_PAGE_SIZE.

Should the migration code be fixed, or should the TPM code allocate
ram differently?

In all case, I think the migration code should either be fixed or have
an assert.


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


-- 
Marc-André Lureau

Re: [Qemu-devel] [PATCH v10 3/6] tpm: allocate/map buffer for TPM Physical Presence interface

[Juan Quintela]

Marc-André Lureau <marcandre.lureau@gmail.com> wrote:
> Hi
>
> On Fri, Aug 31, 2018 at 7:32 PM 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 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>
>> Reviewed-by: Igor Mammedov <imammedo@redhat.com>


....

>> + */
>> +#define TPM_PPI_ADDR_SIZE           0x400
>> +#define TPM_PPI_ADDR_BASE           0xFED45000

> There is a (new) issue with the PPI ram region:
>
> READ of size 32 at 0x61d000090480 thread T6
>     #0 0x5622bd8de0f4 in buffer_zero_avx2
> /home/elmarco/src/qq/util/bufferiszero.c:169
>     #1 0x5622bd8de899 in select_accel_fn
> /home/elmarco/src/qq/util/bufferiszero.c:282
>     #2 0x5622bd8de8f1 in buffer_is_zero
> /home/elmarco/src/qq/util/bufferiszero.c:309
>     #3 0x5622bc209f94 in is_zero_range /home/elmarco/src/qq/migration/ram.c:82
>     #4 0x5622bc21938d in save_zero_page_to_file
> /home/elmarco/src/qq/migration/ram.c:1694
>     #5 0x5622bc219452 in save_zero_page
> /home/elmarco/src/qq/migration/ram.c:1713
>     #6 0x5622bc21db67 in ram_save_target_page
> /home/elmarco/src/qq/migration/ram.c:2289
>     #7 0x5622bc21e13e in ram_save_host_page
> /home/elmarco/src/qq/migration/ram.c:2351
>     #8 0x5622bc21ea3a in ram_find_and_save_block
> /home/elmarco/src/qq/migration/ram.c:2413
>     #9 0x5622bc223b5d in ram_save_iterate
> /home/elmarco/src/qq/migration/ram.c:3193
>     #10 0x5622bd16f544 in qemu_savevm_state_iterate
> /home/elmarco/src/qq/migration/savevm.c:1103
>     #11 0x5622bd157e75 in migration_iteration_run
> /home/elmarco/src/qq/migration/migration.c:2897
>     #12 0x5622bd15892e in migration_thread
> /home/elmarco/src/qq/migration/migration.c:3018
>     #13 0x5622bd902f31 in qemu_thread_start
> /home/elmarco/src/qq/util/qemu-thread-posix.c:504
>     #14 0x7f42f0ef4593 in start_thread (/lib64/libpthread.so.0+0x7593)
>     #15 0x7f42f0c280de in clone (/lib64/libc.so.6+0xfa0de)
> 0x61d000090490 is located 0 bytes to the right of 2064-byte region
> [0x61d00008fc80,0x61d000090490)
>
> migration code is assuming RAM is multiple of TARGET_PAGE_SIZE.

Physical RAM is multiple of TARGET_PAGE_SIZE O:-)
That assumtion is held in too many places, if you can change the size to
be multiple of page size, that would be greate.

> Should the migration code be fixed, or should the TPM code allocate
> ram differently?

Migration people (i.e. me) would preffer that you fix the TPM
allocation.  Or you can decide that this is *not* RAM.  The unit of
transfer for migrate ram is ram pages, a.k.a. TARGET_PAGE_SIZE.

> In all case, I think the migration code should either be fixed or have
> an assert.

An assert for sure.

Fixed .... Do we have real devices that put ram regions that are smaller
than page size?

Later, Juan.

Re: [Qemu-devel] [PATCH v10 3/6] tpm: allocate/map buffer for TPM Physical Presence interface

[Igor Mammedov]

On Mon, 03 Sep 2018 23:48:15 +0200
Juan Quintela <quintela@redhat.com> wrote:

> Marc-André Lureau <marcandre.lureau@gmail.com> wrote:
> > Hi
> >
> > On Fri, Aug 31, 2018 at 7:32 PM 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 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>
> >> Reviewed-by: Igor Mammedov <imammedo@redhat.com>  
> 
> 
> ....
> 
> >> + */
> >> +#define TPM_PPI_ADDR_SIZE           0x400
> >> +#define TPM_PPI_ADDR_BASE           0xFED45000  
> 
> > There is a (new) issue with the PPI ram region:
> >
> > READ of size 32 at 0x61d000090480 thread T6
> >     #0 0x5622bd8de0f4 in buffer_zero_avx2
> > /home/elmarco/src/qq/util/bufferiszero.c:169
> >     #1 0x5622bd8de899 in select_accel_fn
> > /home/elmarco/src/qq/util/bufferiszero.c:282
> >     #2 0x5622bd8de8f1 in buffer_is_zero
> > /home/elmarco/src/qq/util/bufferiszero.c:309
> >     #3 0x5622bc209f94 in is_zero_range /home/elmarco/src/qq/migration/ram.c:82
> >     #4 0x5622bc21938d in save_zero_page_to_file
> > /home/elmarco/src/qq/migration/ram.c:1694
> >     #5 0x5622bc219452 in save_zero_page
> > /home/elmarco/src/qq/migration/ram.c:1713
> >     #6 0x5622bc21db67 in ram_save_target_page
> > /home/elmarco/src/qq/migration/ram.c:2289
> >     #7 0x5622bc21e13e in ram_save_host_page
> > /home/elmarco/src/qq/migration/ram.c:2351
> >     #8 0x5622bc21ea3a in ram_find_and_save_block
> > /home/elmarco/src/qq/migration/ram.c:2413
> >     #9 0x5622bc223b5d in ram_save_iterate
> > /home/elmarco/src/qq/migration/ram.c:3193
> >     #10 0x5622bd16f544 in qemu_savevm_state_iterate
> > /home/elmarco/src/qq/migration/savevm.c:1103
> >     #11 0x5622bd157e75 in migration_iteration_run
> > /home/elmarco/src/qq/migration/migration.c:2897
> >     #12 0x5622bd15892e in migration_thread
> > /home/elmarco/src/qq/migration/migration.c:3018
> >     #13 0x5622bd902f31 in qemu_thread_start
> > /home/elmarco/src/qq/util/qemu-thread-posix.c:504
> >     #14 0x7f42f0ef4593 in start_thread (/lib64/libpthread.so.0+0x7593)
> >     #15 0x7f42f0c280de in clone (/lib64/libc.so.6+0xfa0de)
> > 0x61d000090490 is located 0 bytes to the right of 2064-byte region
> > [0x61d00008fc80,0x61d000090490)
> >
> > migration code is assuming RAM is multiple of TARGET_PAGE_SIZE.  
> 
> Physical RAM is multiple of TARGET_PAGE_SIZE O:-)
> That assumtion is held in too many places, if you can change the size to
> be multiple of page size, that would be greate.
> 
> > Should the migration code be fixed, or should the TPM code allocate
> > ram differently?  
> 
> Migration people (i.e. me) would preffer that you fix the TPM
> allocation.  Or you can decide that this is *not* RAM.  The unit of
> transfer for migrate ram is ram pages, a.k.a. TARGET_PAGE_SIZE.
I'd loath to waste whole page in already cramped area.
Can we implement it as mmio region? (it will add some bolerplate code for read/write
handlers/migration and cause vmexits on every read/write but it's not a hot path so we might not care)
 
> > In all case, I think the migration code should either be fixed or have
> > an assert.  
> 
> An assert for sure.
> 
> Fixed .... Do we have real devices that put ram regions that are smaller
> than page size?
> 
> Later, Juan.

Re: [Qemu-devel] [PATCH v10 3/6] tpm: allocate/map buffer for TPM Physical Presence interface

[Marc-André Lureau]

Hi

On Tue, Sep 4, 2018 at 10:51 AM Igor Mammedov <imammedo@redhat.com> wrote:
>
> On Mon, 03 Sep 2018 23:48:15 +0200
> Juan Quintela <quintela@redhat.com> wrote:
>
> > Marc-André Lureau <marcandre.lureau@gmail.com> wrote:
> > > Hi
> > >
> > > On Fri, Aug 31, 2018 at 7:32 PM 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 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>
> > >> Reviewed-by: Igor Mammedov <imammedo@redhat.com>
> >
> >
> > ....
> >
> > >> + */
> > >> +#define TPM_PPI_ADDR_SIZE           0x400
> > >> +#define TPM_PPI_ADDR_BASE           0xFED45000
> >
> > > There is a (new) issue with the PPI ram region:
> > >
> > > READ of size 32 at 0x61d000090480 thread T6
> > >     #0 0x5622bd8de0f4 in buffer_zero_avx2
> > > /home/elmarco/src/qq/util/bufferiszero.c:169
> > >     #1 0x5622bd8de899 in select_accel_fn
> > > /home/elmarco/src/qq/util/bufferiszero.c:282
> > >     #2 0x5622bd8de8f1 in buffer_is_zero
> > > /home/elmarco/src/qq/util/bufferiszero.c:309
> > >     #3 0x5622bc209f94 in is_zero_range /home/elmarco/src/qq/migration/ram.c:82
> > >     #4 0x5622bc21938d in save_zero_page_to_file
> > > /home/elmarco/src/qq/migration/ram.c:1694
> > >     #5 0x5622bc219452 in save_zero_page
> > > /home/elmarco/src/qq/migration/ram.c:1713
> > >     #6 0x5622bc21db67 in ram_save_target_page
> > > /home/elmarco/src/qq/migration/ram.c:2289
> > >     #7 0x5622bc21e13e in ram_save_host_page
> > > /home/elmarco/src/qq/migration/ram.c:2351
> > >     #8 0x5622bc21ea3a in ram_find_and_save_block
> > > /home/elmarco/src/qq/migration/ram.c:2413
> > >     #9 0x5622bc223b5d in ram_save_iterate
> > > /home/elmarco/src/qq/migration/ram.c:3193
> > >     #10 0x5622bd16f544 in qemu_savevm_state_iterate
> > > /home/elmarco/src/qq/migration/savevm.c:1103
> > >     #11 0x5622bd157e75 in migration_iteration_run
> > > /home/elmarco/src/qq/migration/migration.c:2897
> > >     #12 0x5622bd15892e in migration_thread
> > > /home/elmarco/src/qq/migration/migration.c:3018
> > >     #13 0x5622bd902f31 in qemu_thread_start
> > > /home/elmarco/src/qq/util/qemu-thread-posix.c:504
> > >     #14 0x7f42f0ef4593 in start_thread (/lib64/libpthread.so.0+0x7593)
> > >     #15 0x7f42f0c280de in clone (/lib64/libc.so.6+0xfa0de)
> > > 0x61d000090490 is located 0 bytes to the right of 2064-byte region
> > > [0x61d00008fc80,0x61d000090490)
> > >
> > > migration code is assuming RAM is multiple of TARGET_PAGE_SIZE.
> >
> > Physical RAM is multiple of TARGET_PAGE_SIZE O:-)
> > That assumtion is held in too many places, if you can change the size to
> > be multiple of page size, that would be greate.

It needs the RAMBlock to be a multiple of TARGET_PAGE_SIZE, but the
memory region (in guest and qemu MemoryRegion) can be less. This is
already the case with for ex, "/rom@etc/acpi/rsdp", which is 36 bytes.

I suppose 2 different MemoryRegions (backed by different RAMBlock)
that would be placed on the same page (for ex, at offset 0x0, size
0x10 and offset 0x10, size 0x100) would work fine in general and with
migration?

> >
> > > Should the migration code be fixed, or should the TPM code allocate
> > > ram differently?
> >
> > Migration people (i.e. me) would preffer that you fix the TPM
> > allocation.  Or you can decide that this is *not* RAM.  The unit of
> > transfer for migrate ram is ram pages, a.k.a. TARGET_PAGE_SIZE.
> I'd loath to waste whole page in already cramped area.
> Can we implement it as mmio region? (it will add some bolerplate code for read/write
> handlers/migration and cause vmexits on every read/write but it's not a hot path so we might not care)
>


I have done some small adjustments to allow
memory_region_init_ram_device_ptr() to work with a MemoryRegions size
!= backed RAMBlock, and it seems to work fine (and doesn't need to
allocate more space of guest memory range, or fall back to mmio)

thanks

> > > In all case, I think the migration code should either be fixed or have
> > > an assert.
> >
> > An assert for sure.
> >
> > Fixed .... Do we have real devices that put ram regions that are smaller
> > than page size?
> >
> > Later, Juan.
>


-- 
Marc-André Lureau

Re: [Qemu-devel] [PATCH v10 3/6] tpm: allocate/map buffer for TPM Physical Presence interface

[Juan Quintela]

Marc-André Lureau <marcandre.lureau@gmail.com> wrote:
Hi

>> > Physical RAM is multiple of TARGET_PAGE_SIZE O:-)
>> > That assumtion is held in too many places, if you can change the size to
>> > be multiple of page size, that would be greate.
>
> It needs the RAMBlock to be a multiple of TARGET_PAGE_SIZE, but the
> memory region (in guest and qemu MemoryRegion) can be less. This is
> already the case with for ex, "/rom@etc/acpi/rsdp", which is 36 bytes.
>
> I suppose 2 different MemoryRegions (backed by different RAMBlock)
> that would be placed on the same page (for ex, at offset 0x0, size
> 0x10 and offset 0x10, size 0x100) would work fine in general and with
> migration?

Honestly I don't know.  But if the ramblock is correct, I think that we
have no problem at all.  Migration code just does:

for each ramblock
  for each page in ramblock

(Yes, I know,  I have oversimplified)

>> > > Should the migration code be fixed, or should the TPM code allocate
>> > > ram differently?
>> >
>> > Migration people (i.e. me) would preffer that you fix the TPM
>> > allocation.  Or you can decide that this is *not* RAM.  The unit of
>> > transfer for migrate ram is ram pages, a.k.a. TARGET_PAGE_SIZE.
>> I'd loath to waste whole page in already cramped area.
>> Can we implement it as mmio region? (it will add some bolerplate
>> code for read/write
>> handlers/migration and cause vmexits on every read/write but it's
>> not a hot path so we might not care)
>>
>
>
> I have done some small adjustments to allow
> memory_region_init_ram_device_ptr() to work with a MemoryRegions size
> != backed RAMBlock, and it seems to work fine (and doesn't need to
> allocate more space of guest memory range, or fall back to mmio)

If the ramblock are full pages, everything should be ok.

Later, Juan.

Re: [Qemu-devel] [PATCH v10 3/6] tpm: allocate/map buffer for TPM Physical Presence interface

[Igor Mammedov]

On Wed, 5 Sep 2018 12:21:39 +0400
Marc-André Lureau <marcandre.lureau@gmail.com> wrote:

> Hi
> 
> On Tue, Sep 4, 2018 at 10:51 AM Igor Mammedov <imammedo@redhat.com> wrote:
> >
> > On Mon, 03 Sep 2018 23:48:15 +0200
> > Juan Quintela <quintela@redhat.com> wrote:
> >
> > > Marc-André Lureau <marcandre.lureau@gmail.com> wrote:
> > > > Hi
> > > >
> > > > On Fri, Aug 31, 2018 at 7:32 PM 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 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>
> > > >> Reviewed-by: Igor Mammedov <imammedo@redhat.com>
> > >
> > >
> > > ....
> > >
> > > >> + */
> > > >> +#define TPM_PPI_ADDR_SIZE           0x400
> > > >> +#define TPM_PPI_ADDR_BASE           0xFED45000
> > >
> > > > There is a (new) issue with the PPI ram region:
> > > >
> > > > READ of size 32 at 0x61d000090480 thread T6
> > > >     #0 0x5622bd8de0f4 in buffer_zero_avx2
> > > > /home/elmarco/src/qq/util/bufferiszero.c:169
> > > >     #1 0x5622bd8de899 in select_accel_fn
> > > > /home/elmarco/src/qq/util/bufferiszero.c:282
> > > >     #2 0x5622bd8de8f1 in buffer_is_zero
> > > > /home/elmarco/src/qq/util/bufferiszero.c:309
> > > >     #3 0x5622bc209f94 in is_zero_range /home/elmarco/src/qq/migration/ram.c:82
> > > >     #4 0x5622bc21938d in save_zero_page_to_file
> > > > /home/elmarco/src/qq/migration/ram.c:1694
> > > >     #5 0x5622bc219452 in save_zero_page
> > > > /home/elmarco/src/qq/migration/ram.c:1713
> > > >     #6 0x5622bc21db67 in ram_save_target_page
> > > > /home/elmarco/src/qq/migration/ram.c:2289
> > > >     #7 0x5622bc21e13e in ram_save_host_page
> > > > /home/elmarco/src/qq/migration/ram.c:2351
> > > >     #8 0x5622bc21ea3a in ram_find_and_save_block
> > > > /home/elmarco/src/qq/migration/ram.c:2413
> > > >     #9 0x5622bc223b5d in ram_save_iterate
> > > > /home/elmarco/src/qq/migration/ram.c:3193
> > > >     #10 0x5622bd16f544 in qemu_savevm_state_iterate
> > > > /home/elmarco/src/qq/migration/savevm.c:1103
> > > >     #11 0x5622bd157e75 in migration_iteration_run
> > > > /home/elmarco/src/qq/migration/migration.c:2897
> > > >     #12 0x5622bd15892e in migration_thread
> > > > /home/elmarco/src/qq/migration/migration.c:3018
> > > >     #13 0x5622bd902f31 in qemu_thread_start
> > > > /home/elmarco/src/qq/util/qemu-thread-posix.c:504
> > > >     #14 0x7f42f0ef4593 in start_thread (/lib64/libpthread.so.0+0x7593)
> > > >     #15 0x7f42f0c280de in clone (/lib64/libc.so.6+0xfa0de)
> > > > 0x61d000090490 is located 0 bytes to the right of 2064-byte region
> > > > [0x61d00008fc80,0x61d000090490)
> > > >
> > > > migration code is assuming RAM is multiple of TARGET_PAGE_SIZE.
> > >
> > > Physical RAM is multiple of TARGET_PAGE_SIZE O:-)
> > > That assumtion is held in too many places, if you can change the size to
> > > be multiple of page size, that would be greate.
> 
> It needs the RAMBlock to be a multiple of TARGET_PAGE_SIZE, but the
> memory region (in guest and qemu MemoryRegion) can be less. This is
> already the case with for ex, "/rom@etc/acpi/rsdp", which is 36 bytes.
RSDP is different in sense that it's not mapped to GPA


> I suppose 2 different MemoryRegions (backed by different RAMBlock)
> that would be placed on the same page (for ex, at offset 0x0, size
> 0x10 and offset 0x10, size 0x100) would work fine in general and with
> migration?
memory_region_add_subregion() maps whole region only,
theoretically you can make an alias region and map that but it would
be rather ugly.

> 
> > >
> > > > Should the migration code be fixed, or should the TPM code allocate
> > > > ram differently?
> > >
> > > Migration people (i.e. me) would preffer that you fix the TPM
> > > allocation.  Or you can decide that this is *not* RAM.  The unit of
> > > transfer for migrate ram is ram pages, a.k.a. TARGET_PAGE_SIZE.
> > I'd loath to waste whole page in already cramped area.
> > Can we implement it as mmio region? (it will add some bolerplate code for read/write
> > handlers/migration and cause vmexits on every read/write but it's not a hot path so we might not care)
> >
> 
> 
> I have done some small adjustments to allow
> memory_region_init_ram_device_ptr() to work with a MemoryRegions size
> != backed RAMBlock, and it seems to work fine (and doesn't need to
> allocate more space of guest memory range, or fall back to mmio)
> 
> thanks
> 
> > > > In all case, I think the migration code should either be fixed or have
> > > > an assert.
> > >
> > > An assert for sure.
> > >
> > > Fixed .... Do we have real devices that put ram regions that are smaller
> > > than page size?
> > >
> > > Later, Juan.
> >
> 
> 

[Qemu-devel] [PATCH v10 4/6] acpi: expose TPM/PPI configuration parameters to firmware via fw_cfg

[Marc-André Lureau]

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

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

Signed-off-by: Stefan Berger <stefanb@linux.vnet.ibm.com>
[ Marc-André: renamed to etc/tpm/config, made it static, document it ]
Signed-off-by: Marc-André Lureau <marcandre.lureau@redhat.com>
Acked-by: Michael S. Tsirkin <mst@redhat.com>
---
 include/hw/acpi/tpm.h |  3 +++
 hw/i386/acpi-build.c  | 19 +++++++++++++++++++
 docs/specs/tpm.txt    | 20 ++++++++++++++++++++
 3 files changed, 42 insertions(+)

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

[Qemu-devel] [PATCH v10 5/6] acpi: build TPM Physical Presence interface

[Marc-André Lureau]

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

The TPM Physical Presence interface consists of an ACPI part, a shared
memory part, and code in the firmware. Users can send messages to the
firmware by writing a code into the shared memory through invoking the
ACPI code. When a reboot happens, the firmware looks for the code and
acts on it by sending sequences of commands to the TPM.

This patch adds the ACPI code. It is similar to the one in EDK2 but doesn't
assume that SMIs are necessary to use. It uses a similar datastructure for
the shared memory as EDK2 does so that EDK2 and SeaBIOS could both make use
of it. I extended the shared memory data structure with an array of 256
bytes, one for each code that could be implemented. The array contains
flags describing the individual codes. This decouples the ACPI implementation
from the firmware implementation.

The underlying TCG specification is accessible from the following page.

https://trustedcomputinggroup.org/tcg-physical-presence-interface-specification/

This patch implements version 1.30.

Signed-off-by: Stefan Berger <stefanb@linux.vnet.ibm.com>
[ Marc-André - ACPI code improvements and windows fixes ]
Signed-off-by: Marc-André Lureau <marcandre.lureau@redhat.com>
Acked-by: Michael S. Tsirkin <mst@redhat.com>
Reviewed-by: Igor Mammedov <imammedo@redhat.com>
---
 include/hw/acpi/tpm.h |   8 +
 hw/i386/acpi-build.c  | 393 +++++++++++++++++++++++++++++++++++++++++-
 docs/specs/tpm.txt    |  83 +++++++++
 3 files changed, 481 insertions(+), 3 deletions(-)

diff --git a/include/hw/acpi/tpm.h b/include/hw/acpi/tpm.h
index a6109a97fc..ecccb96933 100644
--- a/include/hw/acpi/tpm.h
+++ b/include/hw/acpi/tpm.h
@@ -197,4 +197,12 @@ REG32(CRB_DATA_BUFFER, 0x80)
 #define TPM_PPI_VERSION_NONE        0
 #define TPM_PPI_VERSION_1_30        1
 
+/* whether function is blocked by BIOS settings; bits 0, 1, 2 */
+#define TPM_PPI_FUNC_NOT_IMPLEMENTED     (0 << 0)
+#define TPM_PPI_FUNC_BIOS_ONLY           (1 << 0)
+#define TPM_PPI_FUNC_BLOCKED             (2 << 0)
+#define TPM_PPI_FUNC_ALLOWED_USR_REQ     (3 << 0)
+#define TPM_PPI_FUNC_ALLOWED_USR_NOT_REQ (4 << 0)
+#define TPM_PPI_FUNC_MASK                (7 << 0)
+
 #endif /* HW_ACPI_TPM_H */
diff --git a/hw/i386/acpi-build.c b/hw/i386/acpi-build.c
index c24f68df02..c5e9a6e11d 100644
--- a/hw/i386/acpi-build.c
+++ b/hw/i386/acpi-build.c
@@ -43,6 +43,7 @@
 #include "hw/acpi/memory_hotplug.h"
 #include "sysemu/tpm.h"
 #include "hw/acpi/tpm.h"
+#include "hw/tpm/tpm_ppi.h"
 #include "hw/acpi/vmgenid.h"
 #include "sysemu/tpm_backend.h"
 #include "hw/timer/mc146818rtc_regs.h"
@@ -1789,6 +1790,386 @@ static Aml *build_q35_osc_method(void)
     return method;
 }
 
+static void
+build_tpm_ppi(TPMIf *tpm, Aml *dev)
+{
+    Aml *method, *field, *ifctx, *ifctx2, *ifctx3, *func_mask,
+        *not_implemented, *pak, *tpm2, *tpm3, *pprm, *pprq, *zero, *one;
+
+    if (!object_property_get_bool(OBJECT(tpm), "ppi", &error_abort)) {
+        return;
+    }
+
+    zero = aml_int(0);
+    one = aml_int(1);
+    func_mask = aml_int(TPM_PPI_FUNC_MASK);
+    not_implemented = aml_int(TPM_PPI_FUNC_NOT_IMPLEMENTED);
+
+    /*
+     * TPP2 is for the registers that ACPI code used to pass
+     * the PPI code and parameter (PPRQ, PPRM) to the firmware.
+     */
+    aml_append(dev,
+               aml_operation_region("TPP2", AML_SYSTEM_MEMORY,
+                                    aml_int(TPM_PPI_ADDR_BASE + 0x100),
+                                    0x5A));
+    field = aml_field("TPP2", AML_ANY_ACC, AML_NOLOCK, AML_PRESERVE);
+    aml_append(field, aml_named_field("PPIN", 8));
+    aml_append(field, aml_named_field("PPIP", 32));
+    aml_append(field, aml_named_field("PPRP", 32));
+    aml_append(field, aml_named_field("PPRQ", 32));
+    aml_append(field, aml_named_field("PPRM", 32));
+    aml_append(field, aml_named_field("LPPR", 32));
+    aml_append(dev, field);
+    pprq = aml_name("PPRQ");
+    pprm = aml_name("PPRM");
+
+    /*
+     * DerefOf in Windows is broken with SYSTEM_MEMORY.  Use a dynamic
+     * operation region inside of a method for getting FUNC[op].
+     */
+    method = aml_method("TPFN", 1, AML_SERIALIZED);
+    {
+        Aml *op = aml_arg(0);
+        ifctx = aml_if(aml_lgreater_equal(op, aml_int(0x100)));
+        {
+            aml_append(ifctx, aml_return(zero));
+        }
+        aml_append(method, ifctx);
+
+        aml_append(method,
+            aml_operation_region("TPP1", AML_SYSTEM_MEMORY,
+                aml_add(aml_int(TPM_PPI_ADDR_BASE), op, NULL), 0x1));
+        field = aml_field("TPP1", AML_BYTE_ACC, AML_NOLOCK, AML_PRESERVE);
+        aml_append(field, aml_named_field("TPPF", 8));
+        aml_append(method, field);
+        aml_append(method, aml_return(aml_name("TPPF")));
+    }
+    aml_append(dev, method);
+
+    /*
+     * Use global TPM2 & TPM3 variables to workaround Windows ACPI bug
+     * when returning packages.
+     */
+    pak = aml_package(2);
+    aml_append(pak, zero);
+    aml_append(pak, zero);
+    aml_append(dev, aml_name_decl("TPM2", pak));
+    tpm2 = aml_name("TPM2");
+
+    pak = aml_package(3);
+    aml_append(pak, zero);
+    aml_append(pak, zero);
+    aml_append(pak, zero);
+    aml_append(dev, aml_name_decl("TPM3", pak));
+    tpm3 = aml_name("TPM3");
+
+    method = aml_method("_DSM", 4, AML_SERIALIZED);
+    {
+        uint8_t zerobyte[1] = { 0 };
+        Aml *function, *arguments, *rev, *op, *op_arg, *op_flags, *uuid;
+
+        uuid = aml_arg(0);
+        rev = aml_arg(1);
+        function = aml_arg(2);
+        arguments = aml_arg(3);
+        op = aml_local(0);
+        op_flags = aml_local(1);
+
+        ifctx = aml_if(
+            aml_equal(uuid,
+                      aml_touuid("3DDDFAA6-361B-4EB4-A424-8D10089D1653")));
+        {
+            /* standard DSM query function */
+            ifctx2 = aml_if(aml_equal(function, zero));
+            {
+                uint8_t byte_list[2] = { 0xff, 0x01 };
+                aml_append(ifctx2, aml_return(aml_buffer(2, byte_list)));
+            }
+            aml_append(ifctx, ifctx2);
+
+            /*
+             * PPI 1.0: 2.1.1 Get Physical Presence Interface Version
+             *
+             * Arg 2 (Integer): Function Index = 1
+             * Arg 3 (Package): Arguments = Empty Package
+             * Returns: Type: String
+             */
+            ifctx2 = aml_if(aml_equal(function, one));
+            {
+                aml_append(ifctx2, aml_return(aml_string("1.3")));
+            }
+            aml_append(ifctx, ifctx2);
+
+            /*
+             * PPI 1.0: 2.1.3 Submit TPM Operation Request to Pre-OS Environment
+             *
+             * Arg 2 (Integer): Function Index = 2
+             * Arg 3 (Package): Arguments = Package: Type: Integer
+             *                              Operation Value of the Request
+             * Returns: Type: Integer
+             *          0: Success
+             *          1: Operation Value of the Request Not Supported
+             *          2: General Failure
+             */
+            ifctx2 = aml_if(aml_equal(function, aml_int(2)));
+            {
+                /* get opcode */
+                aml_append(ifctx2,
+                           aml_store(aml_derefof(aml_index(arguments,
+                                                           zero)), op));
+
+                /* get opcode flags */
+                aml_append(ifctx2,
+                           aml_store(aml_call1("TPFN", op), op_flags));
+
+                /* if func[opcode] & TPM_PPI_FUNC_NOT_IMPLEMENTED */
+                ifctx3 = aml_if(
+                    aml_equal(
+                        aml_and(op_flags, func_mask, NULL),
+                        not_implemented));
+                {
+                    /* 1: Operation Value of the Request Not Supported */
+                    aml_append(ifctx3, aml_return(one));
+                }
+                aml_append(ifctx2, ifctx3);
+
+                aml_append(ifctx2, aml_store(op, pprq));
+                aml_append(ifctx2, aml_store(zero, pprm));
+                /* 0: success */
+                aml_append(ifctx2, aml_return(zero));
+            }
+            aml_append(ifctx, ifctx2);
+
+            /*
+             * PPI 1.0: 2.1.4 Get Pending TPM Operation Requested By the OS
+             *
+             * Arg 2 (Integer): Function Index = 3
+             * Arg 3 (Package): Arguments = Empty Package
+             * Returns: Type: Package of Integers
+             *          Integer 1: Function Return code
+             *                     0: Success
+             *                     1: General Failure
+             *          Integer 2: Pending operation requested by the OS
+             *                     0: None
+             *                    >0: Operation Value of the Pending Request
+             *          Integer 3: Optional argument to pending operation
+             *                     requested by the OS
+             *                     0: None
+             *                    >0: Argument Value of the Pending Request
+             */
+            ifctx2 = aml_if(aml_equal(function, aml_int(3)));
+            {
+                /*
+                 * Revision ID of 1, no integer parameter beyond
+                 * parameter two are expected
+                 */
+                ifctx3 = aml_if(aml_equal(rev, one));
+                {
+                    /* TPM2[1] = PPRQ */
+                    aml_append(ifctx3,
+                               aml_store(pprq, aml_index(tpm2, one)));
+                    aml_append(ifctx3, aml_return(tpm2));
+                }
+                aml_append(ifctx2, ifctx3);
+
+                /*
+                 * A return value of {0, 23, 1} indicates that
+                 * operation 23 with argument 1 is pending.
+                 */
+                ifctx3 = aml_if(aml_equal(rev, aml_int(2)));
+                {
+                    /* TPM3[1] = PPRQ */
+                    aml_append(ifctx3,
+                               aml_store(pprq, aml_index(tpm3, one)));
+                    /* TPM3[2] = PPRM */
+                    aml_append(ifctx3,
+                               aml_store(pprm, aml_index(tpm3, aml_int(2))));
+                    aml_append(ifctx3, aml_return(tpm3));
+                }
+                aml_append(ifctx2, ifctx3);
+            }
+            aml_append(ifctx, ifctx2);
+
+            /*
+             * PPI 1.0: 2.1.5 Get Platform-Specific Action to Transition to
+             *     Pre-OS Environment
+             *
+             * Arg 2 (Integer): Function Index = 4
+             * Arg 3 (Package): Arguments = Empty Package
+             * Returns: Type: Integer
+             *          0: None
+             *          1: Shutdown
+             *          2: Reboot
+             *          3: OS Vendor-specific
+             */
+            ifctx2 = aml_if(aml_equal(function, aml_int(4)));
+            {
+                /* reboot */
+                aml_append(ifctx2, aml_return(aml_int(2)));
+            }
+            aml_append(ifctx, ifctx2);
+
+            /*
+             * PPI 1.0: 2.1.6 Return TPM Operation Response to OS Environment
+             *
+             * Arg 2 (Integer): Function Index = 5
+             * Arg 3 (Package): Arguments = Empty Package
+             * Returns: Type: Package of Integer
+             *          Integer 1: Function Return code
+             *                     0: Success
+             *                     1: General Failure
+             *          Integer 2: Most recent operation request
+             *                     0: None
+             *                    >0: Operation Value of the most recent request
+             *          Integer 3: Response to the most recent operation request
+             *                     0: Success
+             *                     0x00000001..0x00000FFF: Corresponding TPM
+             *                                             error code
+             *                     0xFFFFFFF0: User Abort or timeout of dialog
+             *                     0xFFFFFFF1: firmware Failure
+             */
+            ifctx2 = aml_if(aml_equal(function, aml_int(5)));
+            {
+                /* TPM3[1] = LPPR */
+                aml_append(ifctx2,
+                           aml_store(aml_name("LPPR"),
+                                     aml_index(tpm3, one)));
+                /* TPM3[2] = PPRP */
+                aml_append(ifctx2,
+                           aml_store(aml_name("PPRP"),
+                                     aml_index(tpm3, aml_int(2))));
+                aml_append(ifctx2, aml_return(tpm3));
+            }
+            aml_append(ifctx, ifctx2);
+
+            /*
+             * PPI 1.0: 2.1.7 Submit preferred user language
+             *
+             * Arg 2 (Integer): Function Index = 6
+             * Arg 3 (Package): Arguments = String Package
+             *                  Preferred language code
+             * Returns: Type: Integer
+             * Function Return Code
+             *          3: Not implemented
+             */
+            ifctx2 = aml_if(aml_equal(function, aml_int(6)));
+            {
+                /* 3 = not implemented */
+                aml_append(ifctx2, aml_return(aml_int(3)));
+            }
+            aml_append(ifctx, ifctx2);
+
+            /*
+             * PPI 1.1: 2.1.7 Submit TPM Operation Request to
+             *     Pre-OS Environment 2
+             *
+             * Arg 2 (Integer): Function Index = 7
+             * Arg 3 (Package): Arguments = Package: Type: Integer
+             *                  Integer 1: Operation Value of the Request
+             *                  Integer 2: Argument for Operation (optional)
+             * Returns: Type: Integer
+             *          0: Success
+             *          1: Not Implemented
+             *          2: General Failure
+             *          3: Operation blocked by current firmware settings
+             */
+            ifctx2 = aml_if(aml_equal(function, aml_int(7)));
+            {
+                /* get opcode */
+                aml_append(ifctx2, aml_store(aml_derefof(aml_index(arguments,
+                                                                   zero)),
+                                             op));
+
+                /* get opcode flags */
+                aml_append(ifctx2, aml_store(aml_call1("TPFN", op),
+                                             op_flags));
+                /* if func[opcode] & TPM_PPI_FUNC_NOT_IMPLEMENTED */
+                ifctx3 = aml_if(
+                    aml_equal(
+                        aml_and(op_flags, func_mask, NULL),
+                        not_implemented));
+                {
+                    /* 1: not implemented */
+                    aml_append(ifctx3, aml_return(one));
+                }
+                aml_append(ifctx2, ifctx3);
+
+                /* if func[opcode] & TPM_PPI_FUNC_BLOCKED */
+                ifctx3 = aml_if(
+                    aml_equal(
+                        aml_and(op_flags, func_mask, NULL),
+                        aml_int(TPM_PPI_FUNC_BLOCKED)));
+                {
+                    /* 3: blocked by firmware */
+                    aml_append(ifctx3, aml_return(aml_int(3)));
+                }
+                aml_append(ifctx2, ifctx3);
+
+                /* revision to integer */
+                ifctx3 = aml_if(aml_equal(rev, one));
+                {
+                    /* revision 1 */
+                    /* PPRQ = op */
+                    aml_append(ifctx3, aml_store(op, pprq));
+                    /* no argument, PPRM = 0 */
+                    aml_append(ifctx3, aml_store(zero, pprm));
+                }
+                aml_append(ifctx2, ifctx3);
+
+                ifctx3 = aml_if(aml_equal(rev, aml_int(2)));
+                {
+                    /* revision 2 */
+                    /* PPRQ = op */
+                    op_arg = aml_derefof(aml_index(arguments, one));
+                    aml_append(ifctx3, aml_store(op, pprq));
+                    /* PPRM = arg3[1] */
+                    aml_append(ifctx3, aml_store(op_arg, pprm));
+                }
+                aml_append(ifctx2, ifctx3);
+                /* 0: success */
+                aml_append(ifctx2, aml_return(zero));
+            }
+            aml_append(ifctx, ifctx2);
+
+            /*
+             * PPI 1.1: 2.1.8 Get User Confirmation Status for Operation
+             *
+             * Arg 2 (Integer): Function Index = 8
+             * Arg 3 (Package): Arguments = Package: Type: Integer
+             *                  Operation Value that may need user confirmation
+             * Returns: Type: Integer
+             *          0: Not implemented
+             *          1: Firmware only
+             *          2: Blocked for OS by firmware configuration
+             *          3: Allowed and physically present user required
+             *          4: Allowed and physically present user not required
+             */
+            ifctx2 = aml_if(aml_equal(function, aml_int(8)));
+            {
+                /* get opcode */
+                aml_append(ifctx2,
+                           aml_store(aml_derefof(aml_index(arguments,
+                                                           zero)),
+                                     op));
+
+                /* get opcode flags */
+                aml_append(ifctx2, aml_store(aml_call1("TPFN", op),
+                                             op_flags));
+                /* return confirmation status code */
+                aml_append(ifctx2,
+                           aml_return(
+                               aml_and(op_flags, func_mask, NULL)));
+            }
+            aml_append(ifctx, ifctx2);
+
+            aml_append(ifctx, aml_return(aml_buffer(1, zerobyte)));
+        }
+        aml_append(method, ifctx);
+    }
+    aml_append(dev, method);
+}
+
 static void
 build_dsdt(GArray *table_data, BIOSLinker *linker,
            AcpiPmInfo *pm, AcpiMiscInfo *misc,
@@ -1802,6 +2183,7 @@ build_dsdt(GArray *table_data, BIOSLinker *linker,
     uint32_t nr_mem = machine->ram_slots;
     int root_bus_limit = 0xFF;
     PCIBus *bus = NULL;
+    TPMIf *tpm = tpm_find();
     int i;
 
     dsdt = init_aml_allocator();
@@ -2139,7 +2521,7 @@ build_dsdt(GArray *table_data, BIOSLinker *linker,
             /* Scan all PCI buses. Generate tables to support hotplug. */
             build_append_pci_bus_devices(scope, bus, pm->pcihp_bridge_en);
 
-            if (TPM_IS_TIS(tpm_find())) {
+            if (TPM_IS_TIS(tpm)) {
                 dev = aml_device("ISA.TPM");
                 aml_append(dev, aml_name_decl("_HID", aml_eisaid("PNP0C31")));
                 aml_append(dev, aml_name_decl("_STA", aml_int(0xF)));
@@ -2153,6 +2535,9 @@ build_dsdt(GArray *table_data, BIOSLinker *linker,
                  */
                 /* aml_append(crs, aml_irq_no_flags(TPM_TIS_IRQ)); */
                 aml_append(dev, aml_name_decl("_CRS", crs));
+
+                build_tpm_ppi(tpm, dev);
+
                 aml_append(scope, dev);
             }
 
@@ -2160,7 +2545,7 @@ build_dsdt(GArray *table_data, BIOSLinker *linker,
         }
     }
 
-    if (TPM_IS_CRB(tpm_find())) {
+    if (TPM_IS_CRB(tpm)) {
         dev = aml_device("TPM");
         aml_append(dev, aml_name_decl("_HID", aml_string("MSFT0101")));
         crs = aml_resource_template();
@@ -2172,6 +2557,8 @@ build_dsdt(GArray *table_data, BIOSLinker *linker,
         aml_append(method, aml_return(aml_int(0x0f)));
         aml_append(dev, method);
 
+        build_tpm_ppi(tpm, dev);
+
         aml_append(sb_scope, dev);
     }
 
@@ -2924,7 +3311,7 @@ void acpi_setup(void)
         tpm_config = (FWCfgTPMConfig) {
             .tpmppi_address = cpu_to_le32(TPM_PPI_ADDR_BASE),
             .tpm_version = tpm_get_version(tpm_find()),
-            .tpmppi_version = TPM_PPI_VERSION_NONE
+            .tpmppi_version = TPM_PPI_VERSION_1_30
         };
         fw_cfg_add_file(pcms->fw_cfg, "etc/tpm/config",
                         &tpm_config, sizeof tpm_config);
diff --git a/docs/specs/tpm.txt b/docs/specs/tpm.txt
index a5bdd5f26e..332c2ae597 100644
--- a/docs/specs/tpm.txt
+++ b/docs/specs/tpm.txt
@@ -62,6 +62,89 @@ URL:
 
 https://trustedcomputinggroup.org/tcg-acpi-specification/
 
+== ACPI PPI Interface ==
+
+QEMU supports the Physical Presence Interface (PPI) for TPM 1.2 and TPM 2. This
+interface requires ACPI and firmware support. The specification can be found at
+the following URL:
+
+https://trustedcomputinggroup.org/resource/tcg-physical-presence-interface-specification/
+
+PPI enables a system administrator (root) to request a modification to the
+TPM upon reboot. The PPI specification defines the operation requests and the
+actions the firmware has to take. The system administrator passes the operation
+request number to the firmware through an ACPI interface which writes this
+number to a memory location that the firmware knows. Upon reboot, the firmware
+finds the number and sends commands to the the TPM. The firmware writes the TPM
+result code and the operation request number to a memory location that ACPI can
+read from and pass the result on to the administrator.
+
+The PPI specification defines a set of mandatory and optional operations for
+the firmware to implement. The ACPI interface also allows an administrator to
+list the supported operations. In QEMU the ACPI code is generated by QEMU, yet
+the firmware needs to implement support on a per-operations basis, and
+different firmwares may support a different subset. Therefore, QEMU introduces
+the virtual memory device for PPI where the firmware can indicate which
+operations it supports and ACPI can enable the ones that are supported and
+disable all others. This interface lies in main memory and has the following
+layout:
+
+ +----------+--------+--------+-------------------------------------------+
+ |  Field   | Length | Offset | Description                               |
+ +----------+--------+--------+-------------------------------------------+
+ | func     |  0x100 |  0x000 | Firmware sets values for each supported   |
+ |          |        |        | operation. See defined values below.      |
+ +----------+--------+--------+-------------------------------------------+
+ | ppin     |   0x1  |  0x100 | SMI interrupt to use. Set by firmware.    |
+ |          |        |        | Not supported.                            |
+ +----------+--------+--------+-------------------------------------------+
+ | ppip     |   0x4  |  0x101 | ACPI function index to pass to SMM code.  |
+ |          |        |        | Set by ACPI. Not supported.               |
+ +----------+--------+--------+-------------------------------------------+
+ | pprp     |   0x4  |  0x105 | Result of last executed operation. Set by |
+ |          |        |        | firmware. See function index 5 for values.|
+ +----------+--------+--------+-------------------------------------------+
+ | pprq     |   0x4  |  0x109 | Operation request number to execute. See  |
+ |          |        |        | 'Physical Presence Interface Operation    |
+ |          |        |        | Summary' tables in specs. Set by ACPI.    |
+ +----------+--------+--------+-------------------------------------------+
+ | pprm     |   0x4  |  0x10d | Operation request optional parameter.     |
+ |          |        |        | Values depend on operation. Set by ACPI.  |
+ +----------+--------+--------+-------------------------------------------+
+ | lppr     |   0x4  |  0x111 | Last executed operation request number.   |
+ |          |        |        | Copied from pprq field by firmware.       |
+ +----------+--------+--------+-------------------------------------------+
+ | fret     |   0x4  |  0x115 | Result code from SMM function.            |
+ |          |        |        | Not supported.                            |
+ +----------+--------+--------+-------------------------------------------+
+ | res1     |  0x40  |  0x119 | Reserved for future use                   |
+ +----------+--------+--------+-------------------------------------------+
+ | next_step|   0x1  |  0x159 | Operation to execute after reboot by      |
+ |          |        |        | firmware. Used by firmware.               |
+ +----------+--------+--------+-------------------------------------------+
+
+   The following values are supported for the 'func' field. They correspond
+   to the values used by ACPI function index 8.
+
+ +----------+-------------------------------------------------------------+
+ | value    | Description                                                 |
+ +----------+-------------------------------------------------------------+
+ | 0        | Operation is not implemented.                               |
+ +----------+-------------------------------------------------------------+
+ | 1        | Operation is only accessible through firmware.              |
+ +----------+-------------------------------------------------------------+
+ | 2        | Operation is blocked for OS by firmware configuration.      |
+ +----------+-------------------------------------------------------------+
+ | 3        | Operation is allowed and physically present user required.  |
+ +----------+-------------------------------------------------------------+
+ | 4        | Operation is allowed and physically present user is not     |
+ |          | required.                                                   |
+ +----------+-------------------------------------------------------------+
+
+The location of the table is given by the fw_cfg tpmppi_address field.
+The PPI memory region size is 0x400 (TPM_PPI_ADDR_SIZE) to leave
+enough room for future updates.
+
 
 QEMU files related to TPM ACPI tables:
  - hw/i386/acpi-build.c
-- 
2.19.0.rc1

[Qemu-devel] [PATCH v10 6/6] tpm: add ACPI memory clear interface

[Marc-André Lureau]

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

The interface is described in the "TCG Platform Reset Attack
Mitigation Specification", chapter 6 "ACPI _DSM Function". According
to Laszlo, it's not so easy to implement in OVMF, he suggested to do
it in qemu instead.

Signed-off-by: Marc-André Lureau <marcandre.lureau@redhat.com>
---
 hw/tpm/tpm_ppi.h     |  2 ++
 hw/i386/acpi-build.c | 46 ++++++++++++++++++++++++++++++++++++++++++++
 hw/tpm/tpm_crb.c     |  1 +
 hw/tpm/tpm_ppi.c     | 23 ++++++++++++++++++++++
 hw/tpm/tpm_tis.c     |  1 +
 docs/specs/tpm.txt   |  2 ++
 hw/tpm/trace-events  |  3 +++
 7 files changed, 78 insertions(+)

diff --git a/hw/tpm/tpm_ppi.h b/hw/tpm/tpm_ppi.h
index f6458bf87e..3239751e9f 100644
--- a/hw/tpm/tpm_ppi.h
+++ b/hw/tpm/tpm_ppi.h
@@ -23,4 +23,6 @@ typedef struct TPMPPI {
 bool tpm_ppi_init(TPMPPI *tpmppi, struct MemoryRegion *m,
                   hwaddr addr, Object *obj, Error **errp);
 
+void tpm_ppi_reset(TPMPPI *tpmppi);
+
 #endif /* TPM_TPM_PPI_H */
diff --git a/hw/i386/acpi-build.c b/hw/i386/acpi-build.c
index c5e9a6e11d..2ab3e8fae7 100644
--- a/hw/i386/acpi-build.c
+++ b/hw/i386/acpi-build.c
@@ -1824,6 +1824,13 @@ build_tpm_ppi(TPMIf *tpm, Aml *dev)
     pprq = aml_name("PPRQ");
     pprm = aml_name("PPRM");
 
+    aml_append(dev,
+               aml_operation_region("TPP3", AML_SYSTEM_MEMORY,
+                                    aml_int(TPM_PPI_ADDR_BASE + 0x15a),
+                                    0x1));
+    field = aml_field("TPP3", AML_BYTE_ACC, AML_NOLOCK, AML_PRESERVE);
+    aml_append(field, aml_named_field("MOVV", 8));
+    aml_append(dev, field);
     /*
      * DerefOf in Windows is broken with SYSTEM_MEMORY.  Use a dynamic
      * operation region inside of a method for getting FUNC[op].
@@ -2166,7 +2173,46 @@ build_tpm_ppi(TPMIf *tpm, Aml *dev)
             aml_append(ifctx, aml_return(aml_buffer(1, zerobyte)));
         }
         aml_append(method, ifctx);
+
+        ifctx = aml_if(
+            aml_equal(uuid,
+                      aml_touuid("376054ED-CC13-4675-901C-4756D7F2D45D")));
+        {
+            /* standard DSM query function */
+            ifctx2 = aml_if(aml_equal(function, zero));
+            {
+                uint8_t byte_list[1] = { 0x03 };
+                aml_append(ifctx2, aml_return(aml_buffer(1, byte_list)));
+            }
+            aml_append(ifctx, ifctx2);
+
+            /*
+             * TCG Platform Reset Attack Mitigation Specification 1.0 Ch.6
+             *
+             * Arg 2 (Integer): Function Index = 1
+             * Arg 3 (Package): Arguments = Package: Type: Integer
+             *                  Operation Value of the Request
+             * Returns: Type: Integer
+             *          0: Success
+             *          1: General Failure
+             */
+            ifctx2 = aml_if(aml_equal(function, one));
+            {
+                aml_append(ifctx2,
+                           aml_store(aml_derefof(aml_index(arguments, zero)),
+                                     op));
+                {
+                    aml_append(ifctx2, aml_store(op, aml_name("MOVV")));
+
+                    /* 0: success */
+                    aml_append(ifctx2, aml_return(zero));
+                }
+            }
+            aml_append(ifctx, ifctx2);
+        }
+        aml_append(method, ifctx);
     }
+
     aml_append(dev, method);
 }
 
diff --git a/hw/tpm/tpm_crb.c b/hw/tpm/tpm_crb.c
index b243222fd6..48f6a716ad 100644
--- a/hw/tpm/tpm_crb.c
+++ b/hw/tpm/tpm_crb.c
@@ -233,6 +233,7 @@ static void tpm_crb_reset(void *dev)
 {
     CRBState *s = CRB(dev);
 
+    tpm_ppi_reset(&s->ppi);
     tpm_backend_reset(s->tpmbe);
 
     memset(s->regs, 0, sizeof(s->regs));
diff --git a/hw/tpm/tpm_ppi.c b/hw/tpm/tpm_ppi.c
index 8b46b9dd4b..ce43bc5729 100644
--- a/hw/tpm/tpm_ppi.c
+++ b/hw/tpm/tpm_ppi.c
@@ -16,8 +16,30 @@
 #include "qapi/error.h"
 #include "cpu.h"
 #include "sysemu/memory_mapping.h"
+#include "sysemu/reset.h"
 #include "migration/vmstate.h"
 #include "tpm_ppi.h"
+#include "trace.h"
+
+void tpm_ppi_reset(TPMPPI *tpmppi)
+{
+    char *ptr = memory_region_get_ram_ptr(&tpmppi->ram);
+
+    if (ptr[0x15a] & 0x1) {
+        GuestPhysBlockList guest_phys_blocks;
+        GuestPhysBlock *block;
+
+        guest_phys_blocks_init(&guest_phys_blocks);
+        guest_phys_blocks_append(&guest_phys_blocks);
+        QTAILQ_FOREACH(block, &guest_phys_blocks.head, next) {
+            trace_tpm_ppi_memset(block->host_addr,
+                             block->target_end - block->target_start);
+            memset(block->host_addr, 0,
+                   block->target_end - block->target_start);
+        }
+        guest_phys_blocks_free(&guest_phys_blocks);
+    }
+}
 
 bool tpm_ppi_init(TPMPPI *tpmppi, struct MemoryRegion *m,
                   hwaddr addr, Object *obj, Error **errp)
@@ -27,5 +49,6 @@ bool tpm_ppi_init(TPMPPI *tpmppi, struct MemoryRegion *m,
     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 70432ffe8b..d9bfa956cc 100644
--- a/hw/tpm/tpm_tis.c
+++ b/hw/tpm/tpm_tis.c
@@ -868,6 +868,7 @@ static void tpm_tis_reset(DeviceState *dev)
     s->be_buffer_size = MIN(tpm_backend_get_buffer_size(s->be_driver),
                             TPM_TIS_BUFFER_MAX);
 
+    tpm_ppi_reset(&s->ppi);
     tpm_backend_reset(s->be_driver);
 
     s->active_locty = TPM_TIS_NO_LOCALITY;
diff --git a/docs/specs/tpm.txt b/docs/specs/tpm.txt
index 332c2ae597..ce9bda3c89 100644
--- a/docs/specs/tpm.txt
+++ b/docs/specs/tpm.txt
@@ -121,6 +121,8 @@ layout:
  +----------+--------+--------+-------------------------------------------+
  | next_step|   0x1  |  0x159 | Operation to execute after reboot by      |
  |          |        |        | firmware. Used by firmware.               |
+ +----------+--------+--------+-------------------------------------------+
+ | movv     |   0x1  |  0x15a | Memory overwrite variable                 |
  +----------+--------+--------+-------------------------------------------+
 
    The following values are supported for the 'func' field. They correspond
diff --git a/hw/tpm/trace-events b/hw/tpm/trace-events
index 25bee0cecf..920d32ad55 100644
--- a/hw/tpm/trace-events
+++ b/hw/tpm/trace-events
@@ -51,3 +51,6 @@ tpm_tis_mmio_write_init_abort(void) "Initiating abort"
 tpm_tis_mmio_write_lowering_irq(void) "Lowering IRQ"
 tpm_tis_mmio_write_data2send(uint32_t value, unsigned size) "Data to send to TPM: 0x%08x (size=%d)"
 tpm_tis_pre_save(uint8_t locty, uint32_t rw_offset) "locty: %d, rw_offset = %u"
+
+# hw/tpm/tpm_ppi.c
+tpm_ppi_memset(uint8_t *ptr, size_t size) "memset: %p %zu"
-- 
2.19.0.rc1

Re: [Qemu-devel] [PATCH v10 6/6] tpm: add ACPI memory clear interface

[Igor Mammedov]

On Fri, 31 Aug 2018 19:24:24 +0200
Marc-André Lureau <marcandre.lureau@redhat.com> wrote:

> This allows to pass the last failing test from the Windows HLK TPM 2.0
> TCG PPI 1.3 tests.
> 
> The interface is described in the "TCG Platform Reset Attack
> Mitigation Specification", chapter 6 "ACPI _DSM Function". According
> to Laszlo, it's not so easy to implement in OVMF, he suggested to do
> it in qemu instead.
> 
> Signed-off-by: Marc-André Lureau <marcandre.lureau@redhat.com>
> ---
>  hw/tpm/tpm_ppi.h     |  2 ++
>  hw/i386/acpi-build.c | 46 ++++++++++++++++++++++++++++++++++++++++++++
>  hw/tpm/tpm_crb.c     |  1 +
>  hw/tpm/tpm_ppi.c     | 23 ++++++++++++++++++++++
>  hw/tpm/tpm_tis.c     |  1 +
>  docs/specs/tpm.txt   |  2 ++
>  hw/tpm/trace-events  |  3 +++
>  7 files changed, 78 insertions(+)
> 
> diff --git a/hw/tpm/tpm_ppi.h b/hw/tpm/tpm_ppi.h
> index f6458bf87e..3239751e9f 100644
> --- a/hw/tpm/tpm_ppi.h
> +++ b/hw/tpm/tpm_ppi.h
> @@ -23,4 +23,6 @@ typedef struct TPMPPI {
>  bool tpm_ppi_init(TPMPPI *tpmppi, struct MemoryRegion *m,
>                    hwaddr addr, Object *obj, Error **errp);
>  
> +void tpm_ppi_reset(TPMPPI *tpmppi);
> +
>  #endif /* TPM_TPM_PPI_H */
> diff --git a/hw/i386/acpi-build.c b/hw/i386/acpi-build.c
> index c5e9a6e11d..2ab3e8fae7 100644
> --- a/hw/i386/acpi-build.c
> +++ b/hw/i386/acpi-build.c
> @@ -1824,6 +1824,13 @@ build_tpm_ppi(TPMIf *tpm, Aml *dev)
>      pprq = aml_name("PPRQ");
>      pprm = aml_name("PPRM");
>  
> +    aml_append(dev,
> +               aml_operation_region("TPP3", AML_SYSTEM_MEMORY,
> +                                    aml_int(TPM_PPI_ADDR_BASE + 0x15a),
> +                                    0x1));
> +    field = aml_field("TPP3", AML_BYTE_ACC, AML_NOLOCK, AML_PRESERVE);
> +    aml_append(field, aml_named_field("MOVV", 8));
> +    aml_append(dev, field);
>      /*
>       * DerefOf in Windows is broken with SYSTEM_MEMORY.  Use a dynamic
>       * operation region inside of a method for getting FUNC[op].
> @@ -2166,7 +2173,46 @@ build_tpm_ppi(TPMIf *tpm, Aml *dev)
>              aml_append(ifctx, aml_return(aml_buffer(1, zerobyte)));
>          }
>          aml_append(method, ifctx);
> +
> +        ifctx = aml_if(
> +            aml_equal(uuid,
> +                      aml_touuid("376054ED-CC13-4675-901C-4756D7F2D45D")));
> +        {
> +            /* standard DSM query function */
> +            ifctx2 = aml_if(aml_equal(function, zero));
> +            {
> +                uint8_t byte_list[1] = { 0x03 };
> +                aml_append(ifctx2, aml_return(aml_buffer(1, byte_list)));
> +            }
> +            aml_append(ifctx, ifctx2);
> +
> +            /*
> +             * TCG Platform Reset Attack Mitigation Specification 1.0 Ch.6
> +             *
> +             * Arg 2 (Integer): Function Index = 1
> +             * Arg 3 (Package): Arguments = Package: Type: Integer
> +             *                  Operation Value of the Request
> +             * Returns: Type: Integer
> +             *          0: Success
> +             *          1: General Failure
> +             */
> +            ifctx2 = aml_if(aml_equal(function, one));
> +            {
> +                aml_append(ifctx2,
> +                           aml_store(aml_derefof(aml_index(arguments, zero)),
> +                                     op));
> +                {
> +                    aml_append(ifctx2, aml_store(op, aml_name("MOVV")));
> +
> +                    /* 0: success */
> +                    aml_append(ifctx2, aml_return(zero));
> +                }
> +            }
> +            aml_append(ifctx, ifctx2);
> +        }
> +        aml_append(method, ifctx);
>      }
> +
>      aml_append(dev, method);
>  }
>  
> diff --git a/hw/tpm/tpm_crb.c b/hw/tpm/tpm_crb.c
> index b243222fd6..48f6a716ad 100644
> --- a/hw/tpm/tpm_crb.c
> +++ b/hw/tpm/tpm_crb.c
> @@ -233,6 +233,7 @@ static void tpm_crb_reset(void *dev)
>  {
>      CRBState *s = CRB(dev);
>  
> +    tpm_ppi_reset(&s->ppi);
>      tpm_backend_reset(s->tpmbe);
>  
>      memset(s->regs, 0, sizeof(s->regs));
> diff --git a/hw/tpm/tpm_ppi.c b/hw/tpm/tpm_ppi.c
> index 8b46b9dd4b..ce43bc5729 100644
> --- a/hw/tpm/tpm_ppi.c
> +++ b/hw/tpm/tpm_ppi.c
> @@ -16,8 +16,30 @@
>  #include "qapi/error.h"
>  #include "cpu.h"
>  #include "sysemu/memory_mapping.h"
> +#include "sysemu/reset.h"
>  #include "migration/vmstate.h"
>  #include "tpm_ppi.h"
> +#include "trace.h"
> +
> +void tpm_ppi_reset(TPMPPI *tpmppi)
> +{


> +    char *ptr = memory_region_get_ram_ptr(&tpmppi->ram);
nvdimm seems to use cpu_physical_memory_read() to access guest
accessible memory, so question is what's difference?

> +
> +    if (ptr[0x15a] & 0x1) {
> +        GuestPhysBlockList guest_phys_blocks;
> +        GuestPhysBlock *block;
> +
> +        guest_phys_blocks_init(&guest_phys_blocks);
> +        guest_phys_blocks_append(&guest_phys_blocks);
> +        QTAILQ_FOREACH(block, &guest_phys_blocks.head, next) {
> +            trace_tpm_ppi_memset(block->host_addr,
> +                             block->target_end - block->target_start);
> +            memset(block->host_addr, 0,
> +                   block->target_end - block->target_start);
> +        }
> +        guest_phys_blocks_free(&guest_phys_blocks);
> +    }
> +}
>  
>  bool tpm_ppi_init(TPMPPI *tpmppi, struct MemoryRegion *m,
>                    hwaddr addr, Object *obj, Error **errp)
> @@ -27,5 +49,6 @@ bool tpm_ppi_init(TPMPPI *tpmppi, struct MemoryRegion *m,
>      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 70432ffe8b..d9bfa956cc 100644
> --- a/hw/tpm/tpm_tis.c
> +++ b/hw/tpm/tpm_tis.c
> @@ -868,6 +868,7 @@ static void tpm_tis_reset(DeviceState *dev)
>      s->be_buffer_size = MIN(tpm_backend_get_buffer_size(s->be_driver),
>                              TPM_TIS_BUFFER_MAX);
>  
> +    tpm_ppi_reset(&s->ppi);
>      tpm_backend_reset(s->be_driver);
>  
>      s->active_locty = TPM_TIS_NO_LOCALITY;
> diff --git a/docs/specs/tpm.txt b/docs/specs/tpm.txt
> index 332c2ae597..ce9bda3c89 100644
> --- a/docs/specs/tpm.txt
> +++ b/docs/specs/tpm.txt
> @@ -121,6 +121,8 @@ layout:
>   +----------+--------+--------+-------------------------------------------+
>   | next_step|   0x1  |  0x159 | Operation to execute after reboot by      |
>   |          |        |        | firmware. Used by firmware.               |
> + +----------+--------+--------+-------------------------------------------+
> + | movv     |   0x1  |  0x15a | Memory overwrite variable                 |
>   +----------+--------+--------+-------------------------------------------+
>  
>     The following values are supported for the 'func' field. They correspond
> diff --git a/hw/tpm/trace-events b/hw/tpm/trace-events
> index 25bee0cecf..920d32ad55 100644
> --- a/hw/tpm/trace-events
> +++ b/hw/tpm/trace-events
> @@ -51,3 +51,6 @@ tpm_tis_mmio_write_init_abort(void) "Initiating abort"
>  tpm_tis_mmio_write_lowering_irq(void) "Lowering IRQ"
>  tpm_tis_mmio_write_data2send(uint32_t value, unsigned size) "Data to send to TPM: 0x%08x (size=%d)"
>  tpm_tis_pre_save(uint8_t locty, uint32_t rw_offset) "locty: %d, rw_offset = %u"
> +
> +# hw/tpm/tpm_ppi.c
> +tpm_ppi_memset(uint8_t *ptr, size_t size) "memset: %p %zu"

Re: [Qemu-devel] [PATCH v10 6/6] tpm: add ACPI memory clear interface

[Marc-André Lureau]

Hi

On Tue, Sep 4, 2018 at 10:47 AM Igor Mammedov <imammedo@redhat.com> wrote:
>
> On Fri, 31 Aug 2018 19:24:24 +0200
> Marc-André Lureau <marcandre.lureau@redhat.com> wrote:
>
> > This allows to pass the last failing test from the Windows HLK TPM 2.0
> > TCG PPI 1.3 tests.
> >
> > The interface is described in the "TCG Platform Reset Attack
> > Mitigation Specification", chapter 6 "ACPI _DSM Function". According
> > to Laszlo, it's not so easy to implement in OVMF, he suggested to do
> > it in qemu instead.
> >
> > Signed-off-by: Marc-André Lureau <marcandre.lureau@redhat.com>
> > ---
> >  hw/tpm/tpm_ppi.h     |  2 ++
> >  hw/i386/acpi-build.c | 46 ++++++++++++++++++++++++++++++++++++++++++++
> >  hw/tpm/tpm_crb.c     |  1 +
> >  hw/tpm/tpm_ppi.c     | 23 ++++++++++++++++++++++
> >  hw/tpm/tpm_tis.c     |  1 +
> >  docs/specs/tpm.txt   |  2 ++
> >  hw/tpm/trace-events  |  3 +++
> >  7 files changed, 78 insertions(+)
> >
> > diff --git a/hw/tpm/tpm_ppi.h b/hw/tpm/tpm_ppi.h
> > index f6458bf87e..3239751e9f 100644
> > --- a/hw/tpm/tpm_ppi.h
> > +++ b/hw/tpm/tpm_ppi.h
> > @@ -23,4 +23,6 @@ typedef struct TPMPPI {
> >  bool tpm_ppi_init(TPMPPI *tpmppi, struct MemoryRegion *m,
> >                    hwaddr addr, Object *obj, Error **errp);
> >
> > +void tpm_ppi_reset(TPMPPI *tpmppi);
> > +
> >  #endif /* TPM_TPM_PPI_H */
> > diff --git a/hw/i386/acpi-build.c b/hw/i386/acpi-build.c
> > index c5e9a6e11d..2ab3e8fae7 100644
> > --- a/hw/i386/acpi-build.c
> > +++ b/hw/i386/acpi-build.c
> > @@ -1824,6 +1824,13 @@ build_tpm_ppi(TPMIf *tpm, Aml *dev)
> >      pprq = aml_name("PPRQ");
> >      pprm = aml_name("PPRM");
> >
> > +    aml_append(dev,
> > +               aml_operation_region("TPP3", AML_SYSTEM_MEMORY,
> > +                                    aml_int(TPM_PPI_ADDR_BASE + 0x15a),
> > +                                    0x1));
> > +    field = aml_field("TPP3", AML_BYTE_ACC, AML_NOLOCK, AML_PRESERVE);
> > +    aml_append(field, aml_named_field("MOVV", 8));
> > +    aml_append(dev, field);
> >      /*
> >       * DerefOf in Windows is broken with SYSTEM_MEMORY.  Use a dynamic
> >       * operation region inside of a method for getting FUNC[op].
> > @@ -2166,7 +2173,46 @@ build_tpm_ppi(TPMIf *tpm, Aml *dev)
> >              aml_append(ifctx, aml_return(aml_buffer(1, zerobyte)));
> >          }
> >          aml_append(method, ifctx);
> > +
> > +        ifctx = aml_if(
> > +            aml_equal(uuid,
> > +                      aml_touuid("376054ED-CC13-4675-901C-4756D7F2D45D")));
> > +        {
> > +            /* standard DSM query function */
> > +            ifctx2 = aml_if(aml_equal(function, zero));
> > +            {
> > +                uint8_t byte_list[1] = { 0x03 };
> > +                aml_append(ifctx2, aml_return(aml_buffer(1, byte_list)));
> > +            }
> > +            aml_append(ifctx, ifctx2);
> > +
> > +            /*
> > +             * TCG Platform Reset Attack Mitigation Specification 1.0 Ch.6
> > +             *
> > +             * Arg 2 (Integer): Function Index = 1
> > +             * Arg 3 (Package): Arguments = Package: Type: Integer
> > +             *                  Operation Value of the Request
> > +             * Returns: Type: Integer
> > +             *          0: Success
> > +             *          1: General Failure
> > +             */
> > +            ifctx2 = aml_if(aml_equal(function, one));
> > +            {
> > +                aml_append(ifctx2,
> > +                           aml_store(aml_derefof(aml_index(arguments, zero)),
> > +                                     op));
> > +                {
> > +                    aml_append(ifctx2, aml_store(op, aml_name("MOVV")));
> > +
> > +                    /* 0: success */
> > +                    aml_append(ifctx2, aml_return(zero));
> > +                }
> > +            }
> > +            aml_append(ifctx, ifctx2);
> > +        }
> > +        aml_append(method, ifctx);
> >      }
> > +
> >      aml_append(dev, method);
> >  }
> >
> > diff --git a/hw/tpm/tpm_crb.c b/hw/tpm/tpm_crb.c
> > index b243222fd6..48f6a716ad 100644
> > --- a/hw/tpm/tpm_crb.c
> > +++ b/hw/tpm/tpm_crb.c
> > @@ -233,6 +233,7 @@ static void tpm_crb_reset(void *dev)
> >  {
> >      CRBState *s = CRB(dev);
> >
> > +    tpm_ppi_reset(&s->ppi);
> >      tpm_backend_reset(s->tpmbe);
> >
> >      memset(s->regs, 0, sizeof(s->regs));
> > diff --git a/hw/tpm/tpm_ppi.c b/hw/tpm/tpm_ppi.c
> > index 8b46b9dd4b..ce43bc5729 100644
> > --- a/hw/tpm/tpm_ppi.c
> > +++ b/hw/tpm/tpm_ppi.c
> > @@ -16,8 +16,30 @@
> >  #include "qapi/error.h"
> >  #include "cpu.h"
> >  #include "sysemu/memory_mapping.h"
> > +#include "sysemu/reset.h"
> >  #include "migration/vmstate.h"
> >  #include "tpm_ppi.h"
> > +#include "trace.h"
> > +
> > +void tpm_ppi_reset(TPMPPI *tpmppi)
> > +{
>
>
> > +    char *ptr = memory_region_get_ram_ptr(&tpmppi->ram);
> nvdimm seems to use cpu_physical_memory_read() to access guest
> accessible memory, so question is what's difference?

cpu_physical_memory_read() is higher level, doing dispatch on address
and length checks.

This is a bit unnecessary, as ppi->buf could be accessed directly.

>
> > +
> > +    if (ptr[0x15a] & 0x1) {
> > +        GuestPhysBlockList guest_phys_blocks;
> > +        GuestPhysBlock *block;
> > +
> > +        guest_phys_blocks_init(&guest_phys_blocks);
> > +        guest_phys_blocks_append(&guest_phys_blocks);
> > +        QTAILQ_FOREACH(block, &guest_phys_blocks.head, next) {
> > +            trace_tpm_ppi_memset(block->host_addr,
> > +                             block->target_end - block->target_start);
> > +            memset(block->host_addr, 0,
> > +                   block->target_end - block->target_start);
> > +        }
> > +        guest_phys_blocks_free(&guest_phys_blocks);
> > +    }
> > +}
> >
> >  bool tpm_ppi_init(TPMPPI *tpmppi, struct MemoryRegion *m,
> >                    hwaddr addr, Object *obj, Error **errp)
> > @@ -27,5 +49,6 @@ bool tpm_ppi_init(TPMPPI *tpmppi, struct MemoryRegion *m,
> >      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 70432ffe8b..d9bfa956cc 100644
> > --- a/hw/tpm/tpm_tis.c
> > +++ b/hw/tpm/tpm_tis.c
> > @@ -868,6 +868,7 @@ static void tpm_tis_reset(DeviceState *dev)
> >      s->be_buffer_size = MIN(tpm_backend_get_buffer_size(s->be_driver),
> >                              TPM_TIS_BUFFER_MAX);
> >
> > +    tpm_ppi_reset(&s->ppi);
> >      tpm_backend_reset(s->be_driver);
> >
> >      s->active_locty = TPM_TIS_NO_LOCALITY;
> > diff --git a/docs/specs/tpm.txt b/docs/specs/tpm.txt
> > index 332c2ae597..ce9bda3c89 100644
> > --- a/docs/specs/tpm.txt
> > +++ b/docs/specs/tpm.txt
> > @@ -121,6 +121,8 @@ layout:
> >   +----------+--------+--------+-------------------------------------------+
> >   | next_step|   0x1  |  0x159 | Operation to execute after reboot by      |
> >   |          |        |        | firmware. Used by firmware.               |
> > + +----------+--------+--------+-------------------------------------------+
> > + | movv     |   0x1  |  0x15a | Memory overwrite variable                 |
> >   +----------+--------+--------+-------------------------------------------+
> >
> >     The following values are supported for the 'func' field. They correspond
> > diff --git a/hw/tpm/trace-events b/hw/tpm/trace-events
> > index 25bee0cecf..920d32ad55 100644
> > --- a/hw/tpm/trace-events
> > +++ b/hw/tpm/trace-events
> > @@ -51,3 +51,6 @@ tpm_tis_mmio_write_init_abort(void) "Initiating abort"
> >  tpm_tis_mmio_write_lowering_irq(void) "Lowering IRQ"
> >  tpm_tis_mmio_write_data2send(uint32_t value, unsigned size) "Data to send to TPM: 0x%08x (size=%d)"
> >  tpm_tis_pre_save(uint8_t locty, uint32_t rw_offset) "locty: %d, rw_offset = %u"
> > +
> > +# hw/tpm/tpm_ppi.c
> > +tpm_ppi_memset(uint8_t *ptr, size_t size) "memset: %p %zu"
>
>


-- 
Marc-André Lureau

Re: [Qemu-devel] [PATCH v10 6/6] tpm: add ACPI memory clear interface

[Igor Mammedov]

On Thu, 6 Sep 2018 07:50:09 +0400
Marc-André Lureau <marcandre.lureau@gmail.com> wrote:

> Hi
> 
> On Tue, Sep 4, 2018 at 10:47 AM Igor Mammedov <imammedo@redhat.com> wrote:
> >
> > On Fri, 31 Aug 2018 19:24:24 +0200
> > Marc-André Lureau <marcandre.lureau@redhat.com> wrote:
> >  
> > > This allows to pass the last failing test from the Windows HLK TPM 2.0
> > > TCG PPI 1.3 tests.
> > >
> > > The interface is described in the "TCG Platform Reset Attack
> > > Mitigation Specification", chapter 6 "ACPI _DSM Function". According
> > > to Laszlo, it's not so easy to implement in OVMF, he suggested to do
> > > it in qemu instead.
> > >
> > > Signed-off-by: Marc-André Lureau <marcandre.lureau@redhat.com>
> > > ---
> > >  hw/tpm/tpm_ppi.h     |  2 ++
> > >  hw/i386/acpi-build.c | 46 ++++++++++++++++++++++++++++++++++++++++++++
> > >  hw/tpm/tpm_crb.c     |  1 +
> > >  hw/tpm/tpm_ppi.c     | 23 ++++++++++++++++++++++
> > >  hw/tpm/tpm_tis.c     |  1 +
> > >  docs/specs/tpm.txt   |  2 ++
> > >  hw/tpm/trace-events  |  3 +++
> > >  7 files changed, 78 insertions(+)
> > >
> > > diff --git a/hw/tpm/tpm_ppi.h b/hw/tpm/tpm_ppi.h
> > > index f6458bf87e..3239751e9f 100644
> > > --- a/hw/tpm/tpm_ppi.h
> > > +++ b/hw/tpm/tpm_ppi.h
> > > @@ -23,4 +23,6 @@ typedef struct TPMPPI {
> > >  bool tpm_ppi_init(TPMPPI *tpmppi, struct MemoryRegion *m,
> > >                    hwaddr addr, Object *obj, Error **errp);
> > >
> > > +void tpm_ppi_reset(TPMPPI *tpmppi);
> > > +
> > >  #endif /* TPM_TPM_PPI_H */
> > > diff --git a/hw/i386/acpi-build.c b/hw/i386/acpi-build.c
> > > index c5e9a6e11d..2ab3e8fae7 100644
> > > --- a/hw/i386/acpi-build.c
> > > +++ b/hw/i386/acpi-build.c
> > > @@ -1824,6 +1824,13 @@ build_tpm_ppi(TPMIf *tpm, Aml *dev)
> > >      pprq = aml_name("PPRQ");
> > >      pprm = aml_name("PPRM");
> > >
> > > +    aml_append(dev,
> > > +               aml_operation_region("TPP3", AML_SYSTEM_MEMORY,
> > > +                                    aml_int(TPM_PPI_ADDR_BASE + 0x15a),
> > > +                                    0x1));
> > > +    field = aml_field("TPP3", AML_BYTE_ACC, AML_NOLOCK, AML_PRESERVE);
> > > +    aml_append(field, aml_named_field("MOVV", 8));
> > > +    aml_append(dev, field);
> > >      /*
> > >       * DerefOf in Windows is broken with SYSTEM_MEMORY.  Use a dynamic
> > >       * operation region inside of a method for getting FUNC[op].
> > > @@ -2166,7 +2173,46 @@ build_tpm_ppi(TPMIf *tpm, Aml *dev)
> > >              aml_append(ifctx, aml_return(aml_buffer(1, zerobyte)));
> > >          }
> > >          aml_append(method, ifctx);
> > > +
> > > +        ifctx = aml_if(
> > > +            aml_equal(uuid,
> > > +                      aml_touuid("376054ED-CC13-4675-901C-4756D7F2D45D")));
> > > +        {
> > > +            /* standard DSM query function */
> > > +            ifctx2 = aml_if(aml_equal(function, zero));
> > > +            {
> > > +                uint8_t byte_list[1] = { 0x03 };
> > > +                aml_append(ifctx2, aml_return(aml_buffer(1, byte_list)));
> > > +            }
> > > +            aml_append(ifctx, ifctx2);
> > > +
> > > +            /*
> > > +             * TCG Platform Reset Attack Mitigation Specification 1.0 Ch.6
> > > +             *
> > > +             * Arg 2 (Integer): Function Index = 1
> > > +             * Arg 3 (Package): Arguments = Package: Type: Integer
> > > +             *                  Operation Value of the Request
> > > +             * Returns: Type: Integer
> > > +             *          0: Success
> > > +             *          1: General Failure
> > > +             */
> > > +            ifctx2 = aml_if(aml_equal(function, one));
> > > +            {
> > > +                aml_append(ifctx2,
> > > +                           aml_store(aml_derefof(aml_index(arguments, zero)),
> > > +                                     op));
> > > +                {
> > > +                    aml_append(ifctx2, aml_store(op, aml_name("MOVV")));
> > > +
> > > +                    /* 0: success */
> > > +                    aml_append(ifctx2, aml_return(zero));
> > > +                }
> > > +            }
> > > +            aml_append(ifctx, ifctx2);
> > > +        }
> > > +        aml_append(method, ifctx);
> > >      }
> > > +
> > >      aml_append(dev, method);
> > >  }
> > >
> > > diff --git a/hw/tpm/tpm_crb.c b/hw/tpm/tpm_crb.c
> > > index b243222fd6..48f6a716ad 100644
> > > --- a/hw/tpm/tpm_crb.c
> > > +++ b/hw/tpm/tpm_crb.c
> > > @@ -233,6 +233,7 @@ static void tpm_crb_reset(void *dev)
> > >  {
> > >      CRBState *s = CRB(dev);
> > >
> > > +    tpm_ppi_reset(&s->ppi);
> > >      tpm_backend_reset(s->tpmbe);
> > >
> > >      memset(s->regs, 0, sizeof(s->regs));
> > > diff --git a/hw/tpm/tpm_ppi.c b/hw/tpm/tpm_ppi.c
> > > index 8b46b9dd4b..ce43bc5729 100644
> > > --- a/hw/tpm/tpm_ppi.c
> > > +++ b/hw/tpm/tpm_ppi.c
> > > @@ -16,8 +16,30 @@
> > >  #include "qapi/error.h"
> > >  #include "cpu.h"
> > >  #include "sysemu/memory_mapping.h"
> > > +#include "sysemu/reset.h"
> > >  #include "migration/vmstate.h"
> > >  #include "tpm_ppi.h"
> > > +#include "trace.h"
> > > +
> > > +void tpm_ppi_reset(TPMPPI *tpmppi)
> > > +{  
> >
> >  
> > > +    char *ptr = memory_region_get_ram_ptr(&tpmppi->ram);  
> > nvdimm seems to use cpu_physical_memory_read() to access guest
> > accessible memory, so question is what's difference?  
> 
> cpu_physical_memory_read() is higher level, doing dispatch on address
> and length checks.
> 
> This is a bit unnecessary, as ppi->buf could be accessed directly.
[...]
> > > +            memset(block->host_addr, 0,
> > > +                   block->target_end - block->target_start);
> > > +        }
my concern here is that if we directly touch guest memory here
we might get in trouble on migration without dirtying modified
ranges

PS:
feel free it ignore since I don't have a clue what I'm talking about :)

> > > +        guest_phys_blocks_free(&guest_phys_blocks);
> > > +    }
> > > +}
> > >
> > >  bool tpm_ppi_init(TPMPPI *tpmppi, struct MemoryRegion *m,
> > >                    hwaddr addr, Object *obj, Error **errp)
> > > @@ -27,5 +49,6 @@ bool tpm_ppi_init(TPMPPI *tpmppi, struct MemoryRegion *m,
> > >      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 70432ffe8b..d9bfa956cc 100644
> > > --- a/hw/tpm/tpm_tis.c
> > > +++ b/hw/tpm/tpm_tis.c
> > > @@ -868,6 +868,7 @@ static void tpm_tis_reset(DeviceState *dev)
> > >      s->be_buffer_size = MIN(tpm_backend_get_buffer_size(s->be_driver),
> > >                              TPM_TIS_BUFFER_MAX);
> > >
> > > +    tpm_ppi_reset(&s->ppi);
> > >      tpm_backend_reset(s->be_driver);
> > >
> > >      s->active_locty = TPM_TIS_NO_LOCALITY;
> > > diff --git a/docs/specs/tpm.txt b/docs/specs/tpm.txt
> > > index 332c2ae597..ce9bda3c89 100644
> > > --- a/docs/specs/tpm.txt
> > > +++ b/docs/specs/tpm.txt
> > > @@ -121,6 +121,8 @@ layout:
> > >   +----------+--------+--------+-------------------------------------------+
> > >   | next_step|   0x1  |  0x159 | Operation to execute after reboot by      |
> > >   |          |        |        | firmware. Used by firmware.               |
> > > + +----------+--------+--------+-------------------------------------------+
> > > + | movv     |   0x1  |  0x15a | Memory overwrite variable                 |
> > >   +----------+--------+--------+-------------------------------------------+
> > >
> > >     The following values are supported for the 'func' field. They correspond
> > > diff --git a/hw/tpm/trace-events b/hw/tpm/trace-events
> > > index 25bee0cecf..920d32ad55 100644
> > > --- a/hw/tpm/trace-events
> > > +++ b/hw/tpm/trace-events
> > > @@ -51,3 +51,6 @@ tpm_tis_mmio_write_init_abort(void) "Initiating abort"
> > >  tpm_tis_mmio_write_lowering_irq(void) "Lowering IRQ"
> > >  tpm_tis_mmio_write_data2send(uint32_t value, unsigned size) "Data to send to TPM: 0x%08x (size=%d)"
> > >  tpm_tis_pre_save(uint8_t locty, uint32_t rw_offset) "locty: %d, rw_offset = %u"
> > > +
> > > +# hw/tpm/tpm_ppi.c
> > > +tpm_ppi_memset(uint8_t *ptr, size_t size) "memset: %p %zu"  
> >
> >  
> 
> 

Re: [Qemu-devel] [PATCH v10 6/6] tpm: add ACPI memory clear interface

[Marc-André Lureau]

Hi

On Thu, Sep 6, 2018 at 11:58 AM Igor Mammedov <imammedo@redhat.com> wrote:
>
> On Thu, 6 Sep 2018 07:50:09 +0400
> Marc-André Lureau <marcandre.lureau@gmail.com> wrote:
>
> > Hi
> >
> > On Tue, Sep 4, 2018 at 10:47 AM Igor Mammedov <imammedo@redhat.com> wrote:
> > >
> > > On Fri, 31 Aug 2018 19:24:24 +0200
> > > Marc-André Lureau <marcandre.lureau@redhat.com> wrote:
> > >
> > > > This allows to pass the last failing test from the Windows HLK TPM 2.0
> > > > TCG PPI 1.3 tests.
> > > >
> > > > The interface is described in the "TCG Platform Reset Attack
> > > > Mitigation Specification", chapter 6 "ACPI _DSM Function". According
> > > > to Laszlo, it's not so easy to implement in OVMF, he suggested to do
> > > > it in qemu instead.
> > > >
> > > > Signed-off-by: Marc-André Lureau <marcandre.lureau@redhat.com>
> > > > ---
> > > >  hw/tpm/tpm_ppi.h     |  2 ++
> > > >  hw/i386/acpi-build.c | 46 ++++++++++++++++++++++++++++++++++++++++++++
> > > >  hw/tpm/tpm_crb.c     |  1 +
> > > >  hw/tpm/tpm_ppi.c     | 23 ++++++++++++++++++++++
> > > >  hw/tpm/tpm_tis.c     |  1 +
> > > >  docs/specs/tpm.txt   |  2 ++
> > > >  hw/tpm/trace-events  |  3 +++
> > > >  7 files changed, 78 insertions(+)
> > > >
> > > > diff --git a/hw/tpm/tpm_ppi.h b/hw/tpm/tpm_ppi.h
> > > > index f6458bf87e..3239751e9f 100644
> > > > --- a/hw/tpm/tpm_ppi.h
> > > > +++ b/hw/tpm/tpm_ppi.h
> > > > @@ -23,4 +23,6 @@ typedef struct TPMPPI {
> > > >  bool tpm_ppi_init(TPMPPI *tpmppi, struct MemoryRegion *m,
> > > >                    hwaddr addr, Object *obj, Error **errp);
> > > >
> > > > +void tpm_ppi_reset(TPMPPI *tpmppi);
> > > > +
> > > >  #endif /* TPM_TPM_PPI_H */
> > > > diff --git a/hw/i386/acpi-build.c b/hw/i386/acpi-build.c
> > > > index c5e9a6e11d..2ab3e8fae7 100644
> > > > --- a/hw/i386/acpi-build.c
> > > > +++ b/hw/i386/acpi-build.c
> > > > @@ -1824,6 +1824,13 @@ build_tpm_ppi(TPMIf *tpm, Aml *dev)
> > > >      pprq = aml_name("PPRQ");
> > > >      pprm = aml_name("PPRM");
> > > >
> > > > +    aml_append(dev,
> > > > +               aml_operation_region("TPP3", AML_SYSTEM_MEMORY,
> > > > +                                    aml_int(TPM_PPI_ADDR_BASE + 0x15a),
> > > > +                                    0x1));
> > > > +    field = aml_field("TPP3", AML_BYTE_ACC, AML_NOLOCK, AML_PRESERVE);
> > > > +    aml_append(field, aml_named_field("MOVV", 8));
> > > > +    aml_append(dev, field);
> > > >      /*
> > > >       * DerefOf in Windows is broken with SYSTEM_MEMORY.  Use a dynamic
> > > >       * operation region inside of a method for getting FUNC[op].
> > > > @@ -2166,7 +2173,46 @@ build_tpm_ppi(TPMIf *tpm, Aml *dev)
> > > >              aml_append(ifctx, aml_return(aml_buffer(1, zerobyte)));
> > > >          }
> > > >          aml_append(method, ifctx);
> > > > +
> > > > +        ifctx = aml_if(
> > > > +            aml_equal(uuid,
> > > > +                      aml_touuid("376054ED-CC13-4675-901C-4756D7F2D45D")));
> > > > +        {
> > > > +            /* standard DSM query function */
> > > > +            ifctx2 = aml_if(aml_equal(function, zero));
> > > > +            {
> > > > +                uint8_t byte_list[1] = { 0x03 };
> > > > +                aml_append(ifctx2, aml_return(aml_buffer(1, byte_list)));
> > > > +            }
> > > > +            aml_append(ifctx, ifctx2);
> > > > +
> > > > +            /*
> > > > +             * TCG Platform Reset Attack Mitigation Specification 1.0 Ch.6
> > > > +             *
> > > > +             * Arg 2 (Integer): Function Index = 1
> > > > +             * Arg 3 (Package): Arguments = Package: Type: Integer
> > > > +             *                  Operation Value of the Request
> > > > +             * Returns: Type: Integer
> > > > +             *          0: Success
> > > > +             *          1: General Failure
> > > > +             */
> > > > +            ifctx2 = aml_if(aml_equal(function, one));
> > > > +            {
> > > > +                aml_append(ifctx2,
> > > > +                           aml_store(aml_derefof(aml_index(arguments, zero)),
> > > > +                                     op));
> > > > +                {
> > > > +                    aml_append(ifctx2, aml_store(op, aml_name("MOVV")));
> > > > +
> > > > +                    /* 0: success */
> > > > +                    aml_append(ifctx2, aml_return(zero));
> > > > +                }
> > > > +            }
> > > > +            aml_append(ifctx, ifctx2);
> > > > +        }
> > > > +        aml_append(method, ifctx);
> > > >      }
> > > > +
> > > >      aml_append(dev, method);
> > > >  }
> > > >
> > > > diff --git a/hw/tpm/tpm_crb.c b/hw/tpm/tpm_crb.c
> > > > index b243222fd6..48f6a716ad 100644
> > > > --- a/hw/tpm/tpm_crb.c
> > > > +++ b/hw/tpm/tpm_crb.c
> > > > @@ -233,6 +233,7 @@ static void tpm_crb_reset(void *dev)
> > > >  {
> > > >      CRBState *s = CRB(dev);
> > > >
> > > > +    tpm_ppi_reset(&s->ppi);
> > > >      tpm_backend_reset(s->tpmbe);
> > > >
> > > >      memset(s->regs, 0, sizeof(s->regs));
> > > > diff --git a/hw/tpm/tpm_ppi.c b/hw/tpm/tpm_ppi.c
> > > > index 8b46b9dd4b..ce43bc5729 100644
> > > > --- a/hw/tpm/tpm_ppi.c
> > > > +++ b/hw/tpm/tpm_ppi.c
> > > > @@ -16,8 +16,30 @@
> > > >  #include "qapi/error.h"
> > > >  #include "cpu.h"
> > > >  #include "sysemu/memory_mapping.h"
> > > > +#include "sysemu/reset.h"
> > > >  #include "migration/vmstate.h"
> > > >  #include "tpm_ppi.h"
> > > > +#include "trace.h"
> > > > +
> > > > +void tpm_ppi_reset(TPMPPI *tpmppi)
> > > > +{
> > >
> > >
> > > > +    char *ptr = memory_region_get_ram_ptr(&tpmppi->ram);
> > > nvdimm seems to use cpu_physical_memory_read() to access guest
> > > accessible memory, so question is what's difference?
> >
> > cpu_physical_memory_read() is higher level, doing dispatch on address
> > and length checks.
> >
> > This is a bit unnecessary, as ppi->buf could be accessed directly.
> [...]
> > > > +            memset(block->host_addr, 0,
> > > > +                   block->target_end - block->target_start);
> > > > +        }
> my concern here is that if we directly touch guest memory here
> we might get in trouble on migration without dirtying modified
> ranges

It is a read-only of one byte.
by the time the reset handler is called, the memory must have been
already migrated.

>
> PS:
> feel free it ignore since I don't have a clue what I'm talking about :)
>
> > > > +        guest_phys_blocks_free(&guest_phys_blocks);
> > > > +    }
> > > > +}
> > > >
> > > >  bool tpm_ppi_init(TPMPPI *tpmppi, struct MemoryRegion *m,
> > > >                    hwaddr addr, Object *obj, Error **errp)
> > > > @@ -27,5 +49,6 @@ bool tpm_ppi_init(TPMPPI *tpmppi, struct MemoryRegion *m,
> > > >      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 70432ffe8b..d9bfa956cc 100644
> > > > --- a/hw/tpm/tpm_tis.c
> > > > +++ b/hw/tpm/tpm_tis.c
> > > > @@ -868,6 +868,7 @@ static void tpm_tis_reset(DeviceState *dev)
> > > >      s->be_buffer_size = MIN(tpm_backend_get_buffer_size(s->be_driver),
> > > >                              TPM_TIS_BUFFER_MAX);
> > > >
> > > > +    tpm_ppi_reset(&s->ppi);
> > > >      tpm_backend_reset(s->be_driver);
> > > >
> > > >      s->active_locty = TPM_TIS_NO_LOCALITY;
> > > > diff --git a/docs/specs/tpm.txt b/docs/specs/tpm.txt
> > > > index 332c2ae597..ce9bda3c89 100644
> > > > --- a/docs/specs/tpm.txt
> > > > +++ b/docs/specs/tpm.txt
> > > > @@ -121,6 +121,8 @@ layout:
> > > >   +----------+--------+--------+-------------------------------------------+
> > > >   | next_step|   0x1  |  0x159 | Operation to execute after reboot by      |
> > > >   |          |        |        | firmware. Used by firmware.               |
> > > > + +----------+--------+--------+-------------------------------------------+
> > > > + | movv     |   0x1  |  0x15a | Memory overwrite variable                 |
> > > >   +----------+--------+--------+-------------------------------------------+
> > > >
> > > >     The following values are supported for the 'func' field. They correspond
> > > > diff --git a/hw/tpm/trace-events b/hw/tpm/trace-events
> > > > index 25bee0cecf..920d32ad55 100644
> > > > --- a/hw/tpm/trace-events
> > > > +++ b/hw/tpm/trace-events
> > > > @@ -51,3 +51,6 @@ tpm_tis_mmio_write_init_abort(void) "Initiating abort"
> > > >  tpm_tis_mmio_write_lowering_irq(void) "Lowering IRQ"
> > > >  tpm_tis_mmio_write_data2send(uint32_t value, unsigned size) "Data to send to TPM: 0x%08x (size=%d)"
> > > >  tpm_tis_pre_save(uint8_t locty, uint32_t rw_offset) "locty: %d, rw_offset = %u"
> > > > +
> > > > +# hw/tpm/tpm_ppi.c
> > > > +tpm_ppi_memset(uint8_t *ptr, size_t size) "memset: %p %zu"
> > >
> > >
> >
> >
>


-- 
Marc-André Lureau

Re: [Qemu-devel] [PATCH v10 6/6] tpm: add ACPI memory clear interface

[Igor Mammedov]

On Thu, 6 Sep 2018 12:01:40 +0400
Marc-André Lureau <marcandre.lureau@gmail.com> wrote:

> Hi
> 
> On Thu, Sep 6, 2018 at 11:58 AM Igor Mammedov <imammedo@redhat.com> wrote:
> >
> > On Thu, 6 Sep 2018 07:50:09 +0400
> > Marc-André Lureau <marcandre.lureau@gmail.com> wrote:
> >  
> > > Hi
> > >
> > > On Tue, Sep 4, 2018 at 10:47 AM Igor Mammedov <imammedo@redhat.com> wrote:  
> > > >
> > > > On Fri, 31 Aug 2018 19:24:24 +0200
> > > > Marc-André Lureau <marcandre.lureau@redhat.com> wrote:
> > > >  
> > > > > This allows to pass the last failing test from the Windows HLK TPM 2.0
> > > > > TCG PPI 1.3 tests.
> > > > >
> > > > > The interface is described in the "TCG Platform Reset Attack
> > > > > Mitigation Specification", chapter 6 "ACPI _DSM Function". According
> > > > > to Laszlo, it's not so easy to implement in OVMF, he suggested to do
> > > > > it in qemu instead.
> > > > >
> > > > > Signed-off-by: Marc-André Lureau <marcandre.lureau@redhat.com>
> > > > > ---
> > > > >  hw/tpm/tpm_ppi.h     |  2 ++
> > > > >  hw/i386/acpi-build.c | 46 ++++++++++++++++++++++++++++++++++++++++++++
> > > > >  hw/tpm/tpm_crb.c     |  1 +
> > > > >  hw/tpm/tpm_ppi.c     | 23 ++++++++++++++++++++++
> > > > >  hw/tpm/tpm_tis.c     |  1 +
> > > > >  docs/specs/tpm.txt   |  2 ++
> > > > >  hw/tpm/trace-events  |  3 +++
> > > > >  7 files changed, 78 insertions(+)
> > > > >
> > > > > diff --git a/hw/tpm/tpm_ppi.h b/hw/tpm/tpm_ppi.h
> > > > > index f6458bf87e..3239751e9f 100644
> > > > > --- a/hw/tpm/tpm_ppi.h
> > > > > +++ b/hw/tpm/tpm_ppi.h
> > > > > @@ -23,4 +23,6 @@ typedef struct TPMPPI {
> > > > >  bool tpm_ppi_init(TPMPPI *tpmppi, struct MemoryRegion *m,
> > > > >                    hwaddr addr, Object *obj, Error **errp);
> > > > >
> > > > > +void tpm_ppi_reset(TPMPPI *tpmppi);
> > > > > +
> > > > >  #endif /* TPM_TPM_PPI_H */
> > > > > diff --git a/hw/i386/acpi-build.c b/hw/i386/acpi-build.c
> > > > > index c5e9a6e11d..2ab3e8fae7 100644
> > > > > --- a/hw/i386/acpi-build.c
> > > > > +++ b/hw/i386/acpi-build.c
> > > > > @@ -1824,6 +1824,13 @@ build_tpm_ppi(TPMIf *tpm, Aml *dev)
> > > > >      pprq = aml_name("PPRQ");
> > > > >      pprm = aml_name("PPRM");
> > > > >
> > > > > +    aml_append(dev,
> > > > > +               aml_operation_region("TPP3", AML_SYSTEM_MEMORY,
> > > > > +                                    aml_int(TPM_PPI_ADDR_BASE + 0x15a),
> > > > > +                                    0x1));
> > > > > +    field = aml_field("TPP3", AML_BYTE_ACC, AML_NOLOCK, AML_PRESERVE);
> > > > > +    aml_append(field, aml_named_field("MOVV", 8));
> > > > > +    aml_append(dev, field);
> > > > >      /*
> > > > >       * DerefOf in Windows is broken with SYSTEM_MEMORY.  Use a dynamic
> > > > >       * operation region inside of a method for getting FUNC[op].
> > > > > @@ -2166,7 +2173,46 @@ build_tpm_ppi(TPMIf *tpm, Aml *dev)
> > > > >              aml_append(ifctx, aml_return(aml_buffer(1, zerobyte)));
> > > > >          }
> > > > >          aml_append(method, ifctx);
> > > > > +
> > > > > +        ifctx = aml_if(
> > > > > +            aml_equal(uuid,
> > > > > +                      aml_touuid("376054ED-CC13-4675-901C-4756D7F2D45D")));
> > > > > +        {
> > > > > +            /* standard DSM query function */
> > > > > +            ifctx2 = aml_if(aml_equal(function, zero));
> > > > > +            {
> > > > > +                uint8_t byte_list[1] = { 0x03 };
> > > > > +                aml_append(ifctx2, aml_return(aml_buffer(1, byte_list)));
> > > > > +            }
> > > > > +            aml_append(ifctx, ifctx2);
> > > > > +
> > > > > +            /*
> > > > > +             * TCG Platform Reset Attack Mitigation Specification 1.0 Ch.6
> > > > > +             *
> > > > > +             * Arg 2 (Integer): Function Index = 1
> > > > > +             * Arg 3 (Package): Arguments = Package: Type: Integer
> > > > > +             *                  Operation Value of the Request
> > > > > +             * Returns: Type: Integer
> > > > > +             *          0: Success
> > > > > +             *          1: General Failure
> > > > > +             */
> > > > > +            ifctx2 = aml_if(aml_equal(function, one));
> > > > > +            {
> > > > > +                aml_append(ifctx2,
> > > > > +                           aml_store(aml_derefof(aml_index(arguments, zero)),
> > > > > +                                     op));
> > > > > +                {
> > > > > +                    aml_append(ifctx2, aml_store(op, aml_name("MOVV")));
> > > > > +
> > > > > +                    /* 0: success */
> > > > > +                    aml_append(ifctx2, aml_return(zero));
> > > > > +                }
> > > > > +            }
> > > > > +            aml_append(ifctx, ifctx2);
> > > > > +        }
> > > > > +        aml_append(method, ifctx);
> > > > >      }
> > > > > +
> > > > >      aml_append(dev, method);
> > > > >  }
> > > > >
> > > > > diff --git a/hw/tpm/tpm_crb.c b/hw/tpm/tpm_crb.c
> > > > > index b243222fd6..48f6a716ad 100644
> > > > > --- a/hw/tpm/tpm_crb.c
> > > > > +++ b/hw/tpm/tpm_crb.c
> > > > > @@ -233,6 +233,7 @@ static void tpm_crb_reset(void *dev)
> > > > >  {
> > > > >      CRBState *s = CRB(dev);
> > > > >
> > > > > +    tpm_ppi_reset(&s->ppi);
> > > > >      tpm_backend_reset(s->tpmbe);
> > > > >
> > > > >      memset(s->regs, 0, sizeof(s->regs));
> > > > > diff --git a/hw/tpm/tpm_ppi.c b/hw/tpm/tpm_ppi.c
> > > > > index 8b46b9dd4b..ce43bc5729 100644
> > > > > --- a/hw/tpm/tpm_ppi.c
> > > > > +++ b/hw/tpm/tpm_ppi.c
> > > > > @@ -16,8 +16,30 @@
> > > > >  #include "qapi/error.h"
> > > > >  #include "cpu.h"
> > > > >  #include "sysemu/memory_mapping.h"
> > > > > +#include "sysemu/reset.h"
> > > > >  #include "migration/vmstate.h"
> > > > >  #include "tpm_ppi.h"
> > > > > +#include "trace.h"
> > > > > +
> > > > > +void tpm_ppi_reset(TPMPPI *tpmppi)
> > > > > +{  
> > > >
> > > >  
> > > > > +    char *ptr = memory_region_get_ram_ptr(&tpmppi->ram);  
> > > > nvdimm seems to use cpu_physical_memory_read() to access guest
> > > > accessible memory, so question is what's difference?  
> > >
> > > cpu_physical_memory_read() is higher level, doing dispatch on address
> > > and length checks.
> > >
> > > This is a bit unnecessary, as ppi->buf could be accessed directly.  
> > [...]  
> > > > > +            memset(block->host_addr, 0,
> > > > > +                   block->target_end - block->target_start);
> > > > > +        }  
> > my concern here is that if we directly touch guest memory here
> > we might get in trouble on migration without dirtying modified
> > ranges  
> 
> It is a read-only of one byte.
> by the time the reset handler is called, the memory must have been
> already migrated.
what about memset(), it looks like we are writing to guest memory.
Shouldn't we dirty zeroed out blocks?


> >
> > PS:
> > feel free it ignore since I don't have a clue what I'm talking about :)
> >  
> > > > > +        guest_phys_blocks_free(&guest_phys_blocks);
> > > > > +    }
> > > > > +}
> > > > >
> > > > >  bool tpm_ppi_init(TPMPPI *tpmppi, struct MemoryRegion *m,
> > > > >                    hwaddr addr, Object *obj, Error **errp)
> > > > > @@ -27,5 +49,6 @@ bool tpm_ppi_init(TPMPPI *tpmppi, struct MemoryRegion *m,
> > > > >      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 70432ffe8b..d9bfa956cc 100644
> > > > > --- a/hw/tpm/tpm_tis.c
> > > > > +++ b/hw/tpm/tpm_tis.c
> > > > > @@ -868,6 +868,7 @@ static void tpm_tis_reset(DeviceState *dev)
> > > > >      s->be_buffer_size = MIN(tpm_backend_get_buffer_size(s->be_driver),
> > > > >                              TPM_TIS_BUFFER_MAX);
> > > > >
> > > > > +    tpm_ppi_reset(&s->ppi);
> > > > >      tpm_backend_reset(s->be_driver);
> > > > >
> > > > >      s->active_locty = TPM_TIS_NO_LOCALITY;
> > > > > diff --git a/docs/specs/tpm.txt b/docs/specs/tpm.txt
> > > > > index 332c2ae597..ce9bda3c89 100644
> > > > > --- a/docs/specs/tpm.txt
> > > > > +++ b/docs/specs/tpm.txt
> > > > > @@ -121,6 +121,8 @@ layout:
> > > > >   +----------+--------+--------+-------------------------------------------+
> > > > >   | next_step|   0x1  |  0x159 | Operation to execute after reboot by      |
> > > > >   |          |        |        | firmware. Used by firmware.               |
> > > > > + +----------+--------+--------+-------------------------------------------+
> > > > > + | movv     |   0x1  |  0x15a | Memory overwrite variable                 |
> > > > >   +----------+--------+--------+-------------------------------------------+
> > > > >
> > > > >     The following values are supported for the 'func' field. They correspond
> > > > > diff --git a/hw/tpm/trace-events b/hw/tpm/trace-events
> > > > > index 25bee0cecf..920d32ad55 100644
> > > > > --- a/hw/tpm/trace-events
> > > > > +++ b/hw/tpm/trace-events
> > > > > @@ -51,3 +51,6 @@ tpm_tis_mmio_write_init_abort(void) "Initiating abort"
> > > > >  tpm_tis_mmio_write_lowering_irq(void) "Lowering IRQ"
> > > > >  tpm_tis_mmio_write_data2send(uint32_t value, unsigned size) "Data to send to TPM: 0x%08x (size=%d)"
> > > > >  tpm_tis_pre_save(uint8_t locty, uint32_t rw_offset) "locty: %d, rw_offset = %u"
> > > > > +
> > > > > +# hw/tpm/tpm_ppi.c
> > > > > +tpm_ppi_memset(uint8_t *ptr, size_t size) "memset: %p %zu"  
> > > >
> > > >  
> > >
> > >  
> >  
> 
> 

Re: [Qemu-devel] [PATCH v10 6/6] tpm: add ACPI memory clear interface

[Dr. David Alan Gilbert]

* Marc-André Lureau (marcandre.lureau@gmail.com) wrote:
> Hi
> 
> On Thu, Sep 6, 2018 at 11:58 AM Igor Mammedov <imammedo@redhat.com> wrote:
> >
> > On Thu, 6 Sep 2018 07:50:09 +0400
> > Marc-André Lureau <marcandre.lureau@gmail.com> wrote:
> >
> > > Hi
> > >
> > > On Tue, Sep 4, 2018 at 10:47 AM Igor Mammedov <imammedo@redhat.com> wrote:
> > > >
> > > > On Fri, 31 Aug 2018 19:24:24 +0200
> > > > Marc-André Lureau <marcandre.lureau@redhat.com> wrote:
> > > >
> > > > > This allows to pass the last failing test from the Windows HLK TPM 2.0
> > > > > TCG PPI 1.3 tests.
> > > > >
> > > > > The interface is described in the "TCG Platform Reset Attack
> > > > > Mitigation Specification", chapter 6 "ACPI _DSM Function". According
> > > > > to Laszlo, it's not so easy to implement in OVMF, he suggested to do
> > > > > it in qemu instead.
> > > > >
> > > > > Signed-off-by: Marc-André Lureau <marcandre.lureau@redhat.com>
> > > > > ---
> > > > >  hw/tpm/tpm_ppi.h     |  2 ++
> > > > >  hw/i386/acpi-build.c | 46 ++++++++++++++++++++++++++++++++++++++++++++
> > > > >  hw/tpm/tpm_crb.c     |  1 +
> > > > >  hw/tpm/tpm_ppi.c     | 23 ++++++++++++++++++++++
> > > > >  hw/tpm/tpm_tis.c     |  1 +
> > > > >  docs/specs/tpm.txt   |  2 ++
> > > > >  hw/tpm/trace-events  |  3 +++
> > > > >  7 files changed, 78 insertions(+)
> > > > >
> > > > > diff --git a/hw/tpm/tpm_ppi.h b/hw/tpm/tpm_ppi.h
> > > > > index f6458bf87e..3239751e9f 100644
> > > > > --- a/hw/tpm/tpm_ppi.h
> > > > > +++ b/hw/tpm/tpm_ppi.h
> > > > > @@ -23,4 +23,6 @@ typedef struct TPMPPI {
> > > > >  bool tpm_ppi_init(TPMPPI *tpmppi, struct MemoryRegion *m,
> > > > >                    hwaddr addr, Object *obj, Error **errp);
> > > > >
> > > > > +void tpm_ppi_reset(TPMPPI *tpmppi);
> > > > > +
> > > > >  #endif /* TPM_TPM_PPI_H */
> > > > > diff --git a/hw/i386/acpi-build.c b/hw/i386/acpi-build.c
> > > > > index c5e9a6e11d..2ab3e8fae7 100644
> > > > > --- a/hw/i386/acpi-build.c
> > > > > +++ b/hw/i386/acpi-build.c
> > > > > @@ -1824,6 +1824,13 @@ build_tpm_ppi(TPMIf *tpm, Aml *dev)
> > > > >      pprq = aml_name("PPRQ");
> > > > >      pprm = aml_name("PPRM");
> > > > >
> > > > > +    aml_append(dev,
> > > > > +               aml_operation_region("TPP3", AML_SYSTEM_MEMORY,
> > > > > +                                    aml_int(TPM_PPI_ADDR_BASE + 0x15a),
> > > > > +                                    0x1));
> > > > > +    field = aml_field("TPP3", AML_BYTE_ACC, AML_NOLOCK, AML_PRESERVE);
> > > > > +    aml_append(field, aml_named_field("MOVV", 8));
> > > > > +    aml_append(dev, field);
> > > > >      /*
> > > > >       * DerefOf in Windows is broken with SYSTEM_MEMORY.  Use a dynamic
> > > > >       * operation region inside of a method for getting FUNC[op].
> > > > > @@ -2166,7 +2173,46 @@ build_tpm_ppi(TPMIf *tpm, Aml *dev)
> > > > >              aml_append(ifctx, aml_return(aml_buffer(1, zerobyte)));
> > > > >          }
> > > > >          aml_append(method, ifctx);
> > > > > +
> > > > > +        ifctx = aml_if(
> > > > > +            aml_equal(uuid,
> > > > > +                      aml_touuid("376054ED-CC13-4675-901C-4756D7F2D45D")));
> > > > > +        {
> > > > > +            /* standard DSM query function */
> > > > > +            ifctx2 = aml_if(aml_equal(function, zero));
> > > > > +            {
> > > > > +                uint8_t byte_list[1] = { 0x03 };
> > > > > +                aml_append(ifctx2, aml_return(aml_buffer(1, byte_list)));
> > > > > +            }
> > > > > +            aml_append(ifctx, ifctx2);
> > > > > +
> > > > > +            /*
> > > > > +             * TCG Platform Reset Attack Mitigation Specification 1.0 Ch.6
> > > > > +             *
> > > > > +             * Arg 2 (Integer): Function Index = 1
> > > > > +             * Arg 3 (Package): Arguments = Package: Type: Integer
> > > > > +             *                  Operation Value of the Request
> > > > > +             * Returns: Type: Integer
> > > > > +             *          0: Success
> > > > > +             *          1: General Failure
> > > > > +             */
> > > > > +            ifctx2 = aml_if(aml_equal(function, one));
> > > > > +            {
> > > > > +                aml_append(ifctx2,
> > > > > +                           aml_store(aml_derefof(aml_index(arguments, zero)),
> > > > > +                                     op));
> > > > > +                {
> > > > > +                    aml_append(ifctx2, aml_store(op, aml_name("MOVV")));
> > > > > +
> > > > > +                    /* 0: success */
> > > > > +                    aml_append(ifctx2, aml_return(zero));
> > > > > +                }
> > > > > +            }
> > > > > +            aml_append(ifctx, ifctx2);
> > > > > +        }
> > > > > +        aml_append(method, ifctx);
> > > > >      }
> > > > > +
> > > > >      aml_append(dev, method);
> > > > >  }
> > > > >
> > > > > diff --git a/hw/tpm/tpm_crb.c b/hw/tpm/tpm_crb.c
> > > > > index b243222fd6..48f6a716ad 100644
> > > > > --- a/hw/tpm/tpm_crb.c
> > > > > +++ b/hw/tpm/tpm_crb.c
> > > > > @@ -233,6 +233,7 @@ static void tpm_crb_reset(void *dev)
> > > > >  {
> > > > >      CRBState *s = CRB(dev);
> > > > >
> > > > > +    tpm_ppi_reset(&s->ppi);
> > > > >      tpm_backend_reset(s->tpmbe);
> > > > >
> > > > >      memset(s->regs, 0, sizeof(s->regs));
> > > > > diff --git a/hw/tpm/tpm_ppi.c b/hw/tpm/tpm_ppi.c
> > > > > index 8b46b9dd4b..ce43bc5729 100644
> > > > > --- a/hw/tpm/tpm_ppi.c
> > > > > +++ b/hw/tpm/tpm_ppi.c
> > > > > @@ -16,8 +16,30 @@
> > > > >  #include "qapi/error.h"
> > > > >  #include "cpu.h"
> > > > >  #include "sysemu/memory_mapping.h"
> > > > > +#include "sysemu/reset.h"
> > > > >  #include "migration/vmstate.h"
> > > > >  #include "tpm_ppi.h"
> > > > > +#include "trace.h"
> > > > > +
> > > > > +void tpm_ppi_reset(TPMPPI *tpmppi)
> > > > > +{
> > > >
> > > >
> > > > > +    char *ptr = memory_region_get_ram_ptr(&tpmppi->ram);
> > > > nvdimm seems to use cpu_physical_memory_read() to access guest
> > > > accessible memory, so question is what's difference?
> > >
> > > cpu_physical_memory_read() is higher level, doing dispatch on address
> > > and length checks.
> > >
> > > This is a bit unnecessary, as ppi->buf could be accessed directly.
> > [...]
> > > > > +            memset(block->host_addr, 0,
> > > > > +                   block->target_end - block->target_start);
> > > > > +        }
> > my concern here is that if we directly touch guest memory here
> > we might get in trouble on migration without dirtying modified
> > ranges
> 
> It is a read-only of one byte.
> by the time the reset handler is called, the memory must have been
> already migrated.

Looks like a write to me?
Also, don't forget that a guest reset can happen during a migration.

Dave

> >
> > PS:
> > feel free it ignore since I don't have a clue what I'm talking about :)
> >
> > > > > +        guest_phys_blocks_free(&guest_phys_blocks);
> > > > > +    }
> > > > > +}
> > > > >
> > > > >  bool tpm_ppi_init(TPMPPI *tpmppi, struct MemoryRegion *m,
> > > > >                    hwaddr addr, Object *obj, Error **errp)
> > > > > @@ -27,5 +49,6 @@ bool tpm_ppi_init(TPMPPI *tpmppi, struct MemoryRegion *m,
> > > > >      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 70432ffe8b..d9bfa956cc 100644
> > > > > --- a/hw/tpm/tpm_tis.c
> > > > > +++ b/hw/tpm/tpm_tis.c
> > > > > @@ -868,6 +868,7 @@ static void tpm_tis_reset(DeviceState *dev)
> > > > >      s->be_buffer_size = MIN(tpm_backend_get_buffer_size(s->be_driver),
> > > > >                              TPM_TIS_BUFFER_MAX);
> > > > >
> > > > > +    tpm_ppi_reset(&s->ppi);
> > > > >      tpm_backend_reset(s->be_driver);
> > > > >
> > > > >      s->active_locty = TPM_TIS_NO_LOCALITY;
> > > > > diff --git a/docs/specs/tpm.txt b/docs/specs/tpm.txt
> > > > > index 332c2ae597..ce9bda3c89 100644
> > > > > --- a/docs/specs/tpm.txt
> > > > > +++ b/docs/specs/tpm.txt
> > > > > @@ -121,6 +121,8 @@ layout:
> > > > >   +----------+--------+--------+-------------------------------------------+
> > > > >   | next_step|   0x1  |  0x159 | Operation to execute after reboot by      |
> > > > >   |          |        |        | firmware. Used by firmware.               |
> > > > > + +----------+--------+--------+-------------------------------------------+
> > > > > + | movv     |   0x1  |  0x15a | Memory overwrite variable                 |
> > > > >   +----------+--------+--------+-------------------------------------------+
> > > > >
> > > > >     The following values are supported for the 'func' field. They correspond
> > > > > diff --git a/hw/tpm/trace-events b/hw/tpm/trace-events
> > > > > index 25bee0cecf..920d32ad55 100644
> > > > > --- a/hw/tpm/trace-events
> > > > > +++ b/hw/tpm/trace-events
> > > > > @@ -51,3 +51,6 @@ tpm_tis_mmio_write_init_abort(void) "Initiating abort"
> > > > >  tpm_tis_mmio_write_lowering_irq(void) "Lowering IRQ"
> > > > >  tpm_tis_mmio_write_data2send(uint32_t value, unsigned size) "Data to send to TPM: 0x%08x (size=%d)"
> > > > >  tpm_tis_pre_save(uint8_t locty, uint32_t rw_offset) "locty: %d, rw_offset = %u"
> > > > > +
> > > > > +# hw/tpm/tpm_ppi.c
> > > > > +tpm_ppi_memset(uint8_t *ptr, size_t size) "memset: %p %zu"
> > > >
> > > >
> > >
> > >
> >
> 
> 
> -- 
> Marc-André Lureau
--
Dr. David Alan Gilbert / dgilbert@redhat.com / Manchester, UK

Re: [Qemu-devel] [PATCH v10 6/6] tpm: add ACPI memory clear interface

[Marc-André Lureau]

Hi

On Thu, Sep 6, 2018 at 12:59 PM Dr. David Alan Gilbert
<dgilbert@redhat.com> wrote:
>
> * Marc-André Lureau (marcandre.lureau@gmail.com) wrote:
> > Hi
> >
> > On Thu, Sep 6, 2018 at 11:58 AM Igor Mammedov <imammedo@redhat.com> wrote:
> > >
> > > On Thu, 6 Sep 2018 07:50:09 +0400
> > > Marc-André Lureau <marcandre.lureau@gmail.com> wrote:
> > >
> > > > Hi
> > > >
> > > > On Tue, Sep 4, 2018 at 10:47 AM Igor Mammedov <imammedo@redhat.com> wrote:
> > > > >
> > > > > On Fri, 31 Aug 2018 19:24:24 +0200
> > > > > Marc-André Lureau <marcandre.lureau@redhat.com> wrote:
> > > > >
> > > > > > This allows to pass the last failing test from the Windows HLK TPM 2.0
> > > > > > TCG PPI 1.3 tests.
> > > > > >
> > > > > > The interface is described in the "TCG Platform Reset Attack
> > > > > > Mitigation Specification", chapter 6 "ACPI _DSM Function". According
> > > > > > to Laszlo, it's not so easy to implement in OVMF, he suggested to do
> > > > > > it in qemu instead.
> > > > > >
> > > > > > Signed-off-by: Marc-André Lureau <marcandre.lureau@redhat.com>
> > > > > > ---
> > > > > >  hw/tpm/tpm_ppi.h     |  2 ++
> > > > > >  hw/i386/acpi-build.c | 46 ++++++++++++++++++++++++++++++++++++++++++++
> > > > > >  hw/tpm/tpm_crb.c     |  1 +
> > > > > >  hw/tpm/tpm_ppi.c     | 23 ++++++++++++++++++++++
> > > > > >  hw/tpm/tpm_tis.c     |  1 +
> > > > > >  docs/specs/tpm.txt   |  2 ++
> > > > > >  hw/tpm/trace-events  |  3 +++
> > > > > >  7 files changed, 78 insertions(+)
> > > > > >
> > > > > > diff --git a/hw/tpm/tpm_ppi.h b/hw/tpm/tpm_ppi.h
> > > > > > index f6458bf87e..3239751e9f 100644
> > > > > > --- a/hw/tpm/tpm_ppi.h
> > > > > > +++ b/hw/tpm/tpm_ppi.h
> > > > > > @@ -23,4 +23,6 @@ typedef struct TPMPPI {
> > > > > >  bool tpm_ppi_init(TPMPPI *tpmppi, struct MemoryRegion *m,
> > > > > >                    hwaddr addr, Object *obj, Error **errp);
> > > > > >
> > > > > > +void tpm_ppi_reset(TPMPPI *tpmppi);
> > > > > > +
> > > > > >  #endif /* TPM_TPM_PPI_H */
> > > > > > diff --git a/hw/i386/acpi-build.c b/hw/i386/acpi-build.c
> > > > > > index c5e9a6e11d..2ab3e8fae7 100644
> > > > > > --- a/hw/i386/acpi-build.c
> > > > > > +++ b/hw/i386/acpi-build.c
> > > > > > @@ -1824,6 +1824,13 @@ build_tpm_ppi(TPMIf *tpm, Aml *dev)
> > > > > >      pprq = aml_name("PPRQ");
> > > > > >      pprm = aml_name("PPRM");
> > > > > >
> > > > > > +    aml_append(dev,
> > > > > > +               aml_operation_region("TPP3", AML_SYSTEM_MEMORY,
> > > > > > +                                    aml_int(TPM_PPI_ADDR_BASE + 0x15a),
> > > > > > +                                    0x1));
> > > > > > +    field = aml_field("TPP3", AML_BYTE_ACC, AML_NOLOCK, AML_PRESERVE);
> > > > > > +    aml_append(field, aml_named_field("MOVV", 8));
> > > > > > +    aml_append(dev, field);
> > > > > >      /*
> > > > > >       * DerefOf in Windows is broken with SYSTEM_MEMORY.  Use a dynamic
> > > > > >       * operation region inside of a method for getting FUNC[op].
> > > > > > @@ -2166,7 +2173,46 @@ build_tpm_ppi(TPMIf *tpm, Aml *dev)
> > > > > >              aml_append(ifctx, aml_return(aml_buffer(1, zerobyte)));
> > > > > >          }
> > > > > >          aml_append(method, ifctx);
> > > > > > +
> > > > > > +        ifctx = aml_if(
> > > > > > +            aml_equal(uuid,
> > > > > > +                      aml_touuid("376054ED-CC13-4675-901C-4756D7F2D45D")));
> > > > > > +        {
> > > > > > +            /* standard DSM query function */
> > > > > > +            ifctx2 = aml_if(aml_equal(function, zero));
> > > > > > +            {
> > > > > > +                uint8_t byte_list[1] = { 0x03 };
> > > > > > +                aml_append(ifctx2, aml_return(aml_buffer(1, byte_list)));
> > > > > > +            }
> > > > > > +            aml_append(ifctx, ifctx2);
> > > > > > +
> > > > > > +            /*
> > > > > > +             * TCG Platform Reset Attack Mitigation Specification 1.0 Ch.6
> > > > > > +             *
> > > > > > +             * Arg 2 (Integer): Function Index = 1
> > > > > > +             * Arg 3 (Package): Arguments = Package: Type: Integer
> > > > > > +             *                  Operation Value of the Request
> > > > > > +             * Returns: Type: Integer
> > > > > > +             *          0: Success
> > > > > > +             *          1: General Failure
> > > > > > +             */
> > > > > > +            ifctx2 = aml_if(aml_equal(function, one));
> > > > > > +            {
> > > > > > +                aml_append(ifctx2,
> > > > > > +                           aml_store(aml_derefof(aml_index(arguments, zero)),
> > > > > > +                                     op));
> > > > > > +                {
> > > > > > +                    aml_append(ifctx2, aml_store(op, aml_name("MOVV")));
> > > > > > +
> > > > > > +                    /* 0: success */
> > > > > > +                    aml_append(ifctx2, aml_return(zero));
> > > > > > +                }
> > > > > > +            }
> > > > > > +            aml_append(ifctx, ifctx2);
> > > > > > +        }
> > > > > > +        aml_append(method, ifctx);
> > > > > >      }
> > > > > > +
> > > > > >      aml_append(dev, method);
> > > > > >  }
> > > > > >
> > > > > > diff --git a/hw/tpm/tpm_crb.c b/hw/tpm/tpm_crb.c
> > > > > > index b243222fd6..48f6a716ad 100644
> > > > > > --- a/hw/tpm/tpm_crb.c
> > > > > > +++ b/hw/tpm/tpm_crb.c
> > > > > > @@ -233,6 +233,7 @@ static void tpm_crb_reset(void *dev)
> > > > > >  {
> > > > > >      CRBState *s = CRB(dev);
> > > > > >
> > > > > > +    tpm_ppi_reset(&s->ppi);
> > > > > >      tpm_backend_reset(s->tpmbe);
> > > > > >
> > > > > >      memset(s->regs, 0, sizeof(s->regs));
> > > > > > diff --git a/hw/tpm/tpm_ppi.c b/hw/tpm/tpm_ppi.c
> > > > > > index 8b46b9dd4b..ce43bc5729 100644
> > > > > > --- a/hw/tpm/tpm_ppi.c
> > > > > > +++ b/hw/tpm/tpm_ppi.c
> > > > > > @@ -16,8 +16,30 @@
> > > > > >  #include "qapi/error.h"
> > > > > >  #include "cpu.h"
> > > > > >  #include "sysemu/memory_mapping.h"
> > > > > > +#include "sysemu/reset.h"
> > > > > >  #include "migration/vmstate.h"
> > > > > >  #include "tpm_ppi.h"
> > > > > > +#include "trace.h"
> > > > > > +
> > > > > > +void tpm_ppi_reset(TPMPPI *tpmppi)
> > > > > > +{
> > > > >
> > > > >
> > > > > > +    char *ptr = memory_region_get_ram_ptr(&tpmppi->ram);
> > > > > nvdimm seems to use cpu_physical_memory_read() to access guest
> > > > > accessible memory, so question is what's difference?
> > > >
> > > > cpu_physical_memory_read() is higher level, doing dispatch on address
> > > > and length checks.
> > > >
> > > > This is a bit unnecessary, as ppi->buf could be accessed directly.
> > > [...]
> > > > > > +            memset(block->host_addr, 0,
> > > > > > +                   block->target_end - block->target_start);
> > > > > > +        }
> > > my concern here is that if we directly touch guest memory here
> > > we might get in trouble on migration without dirtying modified
> > > ranges
> >
> > It is a read-only of one byte.
> > by the time the reset handler is called, the memory must have been
> > already migrated.
>
> Looks like a write to me?

the PPI RAM memory is read for the "memory clear" byte
The whole guest RAM is reset to 0 if set.

> Also, don't forget that a guest reset can happen during a migration.

Hmm, does cpu_physical_memory_read()  guarantee the memory has been migrated?
Is there a way to wait for migration to be completed in a reset handler?

>
> Dave
>
> > >
> > > PS:
> > > feel free it ignore since I don't have a clue what I'm talking about :)
> > >
> > > > > > +        guest_phys_blocks_free(&guest_phys_blocks);
> > > > > > +    }
> > > > > > +}
> > > > > >
> > > > > >  bool tpm_ppi_init(TPMPPI *tpmppi, struct MemoryRegion *m,
> > > > > >                    hwaddr addr, Object *obj, Error **errp)
> > > > > > @@ -27,5 +49,6 @@ bool tpm_ppi_init(TPMPPI *tpmppi, struct MemoryRegion *m,
> > > > > >      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 70432ffe8b..d9bfa956cc 100644
> > > > > > --- a/hw/tpm/tpm_tis.c
> > > > > > +++ b/hw/tpm/tpm_tis.c
> > > > > > @@ -868,6 +868,7 @@ static void tpm_tis_reset(DeviceState *dev)
> > > > > >      s->be_buffer_size = MIN(tpm_backend_get_buffer_size(s->be_driver),
> > > > > >                              TPM_TIS_BUFFER_MAX);
> > > > > >
> > > > > > +    tpm_ppi_reset(&s->ppi);
> > > > > >      tpm_backend_reset(s->be_driver);
> > > > > >
> > > > > >      s->active_locty = TPM_TIS_NO_LOCALITY;
> > > > > > diff --git a/docs/specs/tpm.txt b/docs/specs/tpm.txt
> > > > > > index 332c2ae597..ce9bda3c89 100644
> > > > > > --- a/docs/specs/tpm.txt
> > > > > > +++ b/docs/specs/tpm.txt
> > > > > > @@ -121,6 +121,8 @@ layout:
> > > > > >   +----------+--------+--------+-------------------------------------------+
> > > > > >   | next_step|   0x1  |  0x159 | Operation to execute after reboot by      |
> > > > > >   |          |        |        | firmware. Used by firmware.               |
> > > > > > + +----------+--------+--------+-------------------------------------------+
> > > > > > + | movv     |   0x1  |  0x15a | Memory overwrite variable                 |
> > > > > >   +----------+--------+--------+-------------------------------------------+
> > > > > >
> > > > > >     The following values are supported for the 'func' field. They correspond
> > > > > > diff --git a/hw/tpm/trace-events b/hw/tpm/trace-events
> > > > > > index 25bee0cecf..920d32ad55 100644
> > > > > > --- a/hw/tpm/trace-events
> > > > > > +++ b/hw/tpm/trace-events
> > > > > > @@ -51,3 +51,6 @@ tpm_tis_mmio_write_init_abort(void) "Initiating abort"
> > > > > >  tpm_tis_mmio_write_lowering_irq(void) "Lowering IRQ"
> > > > > >  tpm_tis_mmio_write_data2send(uint32_t value, unsigned size) "Data to send to TPM: 0x%08x (size=%d)"
> > > > > >  tpm_tis_pre_save(uint8_t locty, uint32_t rw_offset) "locty: %d, rw_offset = %u"
> > > > > > +
> > > > > > +# hw/tpm/tpm_ppi.c
> > > > > > +tpm_ppi_memset(uint8_t *ptr, size_t size) "memset: %p %zu"
> > > > >
> > > > >
> > > >
> > > >
> > >
> >
> >
> > --
> > Marc-André Lureau
> --
> Dr. David Alan Gilbert / dgilbert@redhat.com / Manchester, UK



-- 
Marc-André Lureau

Re: [Qemu-devel] [PATCH v10 6/6] tpm: add ACPI memory clear interface

[Dr. David Alan Gilbert]

* Marc-André Lureau (marcandre.lureau@gmail.com) wrote:
> Hi
> 
> On Thu, Sep 6, 2018 at 12:59 PM Dr. David Alan Gilbert
> <dgilbert@redhat.com> wrote:
> >
> > * Marc-André Lureau (marcandre.lureau@gmail.com) wrote:
> > > Hi
> > >
> > > On Thu, Sep 6, 2018 at 11:58 AM Igor Mammedov <imammedo@redhat.com> wrote:
> > > >
> > > > On Thu, 6 Sep 2018 07:50:09 +0400
> > > > Marc-André Lureau <marcandre.lureau@gmail.com> wrote:
> > > >
> > > > > Hi
> > > > >
> > > > > On Tue, Sep 4, 2018 at 10:47 AM Igor Mammedov <imammedo@redhat.com> wrote:
> > > > > >
> > > > > > On Fri, 31 Aug 2018 19:24:24 +0200
> > > > > > Marc-André Lureau <marcandre.lureau@redhat.com> wrote:
> > > > > >
> > > > > > > This allows to pass the last failing test from the Windows HLK TPM 2.0
> > > > > > > TCG PPI 1.3 tests.
> > > > > > >
> > > > > > > The interface is described in the "TCG Platform Reset Attack
> > > > > > > Mitigation Specification", chapter 6 "ACPI _DSM Function". According
> > > > > > > to Laszlo, it's not so easy to implement in OVMF, he suggested to do
> > > > > > > it in qemu instead.
> > > > > > >
> > > > > > > Signed-off-by: Marc-André Lureau <marcandre.lureau@redhat.com>
> > > > > > > ---
> > > > > > >  hw/tpm/tpm_ppi.h     |  2 ++
> > > > > > >  hw/i386/acpi-build.c | 46 ++++++++++++++++++++++++++++++++++++++++++++
> > > > > > >  hw/tpm/tpm_crb.c     |  1 +
> > > > > > >  hw/tpm/tpm_ppi.c     | 23 ++++++++++++++++++++++
> > > > > > >  hw/tpm/tpm_tis.c     |  1 +
> > > > > > >  docs/specs/tpm.txt   |  2 ++
> > > > > > >  hw/tpm/trace-events  |  3 +++
> > > > > > >  7 files changed, 78 insertions(+)
> > > > > > >
> > > > > > > diff --git a/hw/tpm/tpm_ppi.h b/hw/tpm/tpm_ppi.h
> > > > > > > index f6458bf87e..3239751e9f 100644
> > > > > > > --- a/hw/tpm/tpm_ppi.h
> > > > > > > +++ b/hw/tpm/tpm_ppi.h
> > > > > > > @@ -23,4 +23,6 @@ typedef struct TPMPPI {
> > > > > > >  bool tpm_ppi_init(TPMPPI *tpmppi, struct MemoryRegion *m,
> > > > > > >                    hwaddr addr, Object *obj, Error **errp);
> > > > > > >
> > > > > > > +void tpm_ppi_reset(TPMPPI *tpmppi);
> > > > > > > +
> > > > > > >  #endif /* TPM_TPM_PPI_H */
> > > > > > > diff --git a/hw/i386/acpi-build.c b/hw/i386/acpi-build.c
> > > > > > > index c5e9a6e11d..2ab3e8fae7 100644
> > > > > > > --- a/hw/i386/acpi-build.c
> > > > > > > +++ b/hw/i386/acpi-build.c
> > > > > > > @@ -1824,6 +1824,13 @@ build_tpm_ppi(TPMIf *tpm, Aml *dev)
> > > > > > >      pprq = aml_name("PPRQ");
> > > > > > >      pprm = aml_name("PPRM");
> > > > > > >
> > > > > > > +    aml_append(dev,
> > > > > > > +               aml_operation_region("TPP3", AML_SYSTEM_MEMORY,
> > > > > > > +                                    aml_int(TPM_PPI_ADDR_BASE + 0x15a),
> > > > > > > +                                    0x1));
> > > > > > > +    field = aml_field("TPP3", AML_BYTE_ACC, AML_NOLOCK, AML_PRESERVE);
> > > > > > > +    aml_append(field, aml_named_field("MOVV", 8));
> > > > > > > +    aml_append(dev, field);
> > > > > > >      /*
> > > > > > >       * DerefOf in Windows is broken with SYSTEM_MEMORY.  Use a dynamic
> > > > > > >       * operation region inside of a method for getting FUNC[op].
> > > > > > > @@ -2166,7 +2173,46 @@ build_tpm_ppi(TPMIf *tpm, Aml *dev)
> > > > > > >              aml_append(ifctx, aml_return(aml_buffer(1, zerobyte)));
> > > > > > >          }
> > > > > > >          aml_append(method, ifctx);
> > > > > > > +
> > > > > > > +        ifctx = aml_if(
> > > > > > > +            aml_equal(uuid,
> > > > > > > +                      aml_touuid("376054ED-CC13-4675-901C-4756D7F2D45D")));
> > > > > > > +        {
> > > > > > > +            /* standard DSM query function */
> > > > > > > +            ifctx2 = aml_if(aml_equal(function, zero));
> > > > > > > +            {
> > > > > > > +                uint8_t byte_list[1] = { 0x03 };
> > > > > > > +                aml_append(ifctx2, aml_return(aml_buffer(1, byte_list)));
> > > > > > > +            }
> > > > > > > +            aml_append(ifctx, ifctx2);
> > > > > > > +
> > > > > > > +            /*
> > > > > > > +             * TCG Platform Reset Attack Mitigation Specification 1.0 Ch.6
> > > > > > > +             *
> > > > > > > +             * Arg 2 (Integer): Function Index = 1
> > > > > > > +             * Arg 3 (Package): Arguments = Package: Type: Integer
> > > > > > > +             *                  Operation Value of the Request
> > > > > > > +             * Returns: Type: Integer
> > > > > > > +             *          0: Success
> > > > > > > +             *          1: General Failure
> > > > > > > +             */
> > > > > > > +            ifctx2 = aml_if(aml_equal(function, one));
> > > > > > > +            {
> > > > > > > +                aml_append(ifctx2,
> > > > > > > +                           aml_store(aml_derefof(aml_index(arguments, zero)),
> > > > > > > +                                     op));
> > > > > > > +                {
> > > > > > > +                    aml_append(ifctx2, aml_store(op, aml_name("MOVV")));
> > > > > > > +
> > > > > > > +                    /* 0: success */
> > > > > > > +                    aml_append(ifctx2, aml_return(zero));
> > > > > > > +                }
> > > > > > > +            }
> > > > > > > +            aml_append(ifctx, ifctx2);
> > > > > > > +        }
> > > > > > > +        aml_append(method, ifctx);
> > > > > > >      }
> > > > > > > +
> > > > > > >      aml_append(dev, method);
> > > > > > >  }
> > > > > > >
> > > > > > > diff --git a/hw/tpm/tpm_crb.c b/hw/tpm/tpm_crb.c
> > > > > > > index b243222fd6..48f6a716ad 100644
> > > > > > > --- a/hw/tpm/tpm_crb.c
> > > > > > > +++ b/hw/tpm/tpm_crb.c
> > > > > > > @@ -233,6 +233,7 @@ static void tpm_crb_reset(void *dev)
> > > > > > >  {
> > > > > > >      CRBState *s = CRB(dev);
> > > > > > >
> > > > > > > +    tpm_ppi_reset(&s->ppi);
> > > > > > >      tpm_backend_reset(s->tpmbe);
> > > > > > >
> > > > > > >      memset(s->regs, 0, sizeof(s->regs));
> > > > > > > diff --git a/hw/tpm/tpm_ppi.c b/hw/tpm/tpm_ppi.c
> > > > > > > index 8b46b9dd4b..ce43bc5729 100644
> > > > > > > --- a/hw/tpm/tpm_ppi.c
> > > > > > > +++ b/hw/tpm/tpm_ppi.c
> > > > > > > @@ -16,8 +16,30 @@
> > > > > > >  #include "qapi/error.h"
> > > > > > >  #include "cpu.h"
> > > > > > >  #include "sysemu/memory_mapping.h"
> > > > > > > +#include "sysemu/reset.h"
> > > > > > >  #include "migration/vmstate.h"
> > > > > > >  #include "tpm_ppi.h"
> > > > > > > +#include "trace.h"
> > > > > > > +
> > > > > > > +void tpm_ppi_reset(TPMPPI *tpmppi)
> > > > > > > +{
> > > > > >
> > > > > >
> > > > > > > +    char *ptr = memory_region_get_ram_ptr(&tpmppi->ram);
> > > > > > nvdimm seems to use cpu_physical_memory_read() to access guest
> > > > > > accessible memory, so question is what's difference?
> > > > >
> > > > > cpu_physical_memory_read() is higher level, doing dispatch on address
> > > > > and length checks.
> > > > >
> > > > > This is a bit unnecessary, as ppi->buf could be accessed directly.
> > > > [...]
> > > > > > > +            memset(block->host_addr, 0,
> > > > > > > +                   block->target_end - block->target_start);
> > > > > > > +        }
> > > > my concern here is that if we directly touch guest memory here
> > > > we might get in trouble on migration without dirtying modified
> > > > ranges
> > >
> > > It is a read-only of one byte.
> > > by the time the reset handler is called, the memory must have been
> > > already migrated.
> >
> > Looks like a write to me?
> 
> the PPI RAM memory is read for the "memory clear" byte
> The whole guest RAM is reset to 0 if set.

Oh, I see; hmm.
How do you avoid zeroing things like persistent memory? Or ROMs? Or EFI
pflash?

> > Also, don't forget that a guest reset can happen during a migration.
> 
> Hmm, does cpu_physical_memory_read()  guarantee the memory has been migrated?
> Is there a way to wait for migration to be completed in a reset handler?

No; remember that migration can take a significant amount of time (many
minutes) as you stuff many GB of RAM down a network.

So you can be in the situation where:
     a) Migration starts
     b) Migration sends a copy of most of RAM across
     c) Guest dirties lots of RAM in parallel with b
     d) migration sends some of the RAM again
     e) guest reboots
     f) migration keeps sending ram across
     g) Migration finally completes and starts on destination

a-f are all happening on the source side as the guest is still running
and doing whatever it wants (including reboots).

Given something like acpi-build.c's acpi_ram_update's call to
memory_region_set_dirty, would that work for you?

Dave

> >
> > Dave
> >
> > > >
> > > > PS:
> > > > feel free it ignore since I don't have a clue what I'm talking about :)
> > > >
> > > > > > > +        guest_phys_blocks_free(&guest_phys_blocks);
> > > > > > > +    }
> > > > > > > +}
> > > > > > >
> > > > > > >  bool tpm_ppi_init(TPMPPI *tpmppi, struct MemoryRegion *m,
> > > > > > >                    hwaddr addr, Object *obj, Error **errp)
> > > > > > > @@ -27,5 +49,6 @@ bool tpm_ppi_init(TPMPPI *tpmppi, struct MemoryRegion *m,
> > > > > > >      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 70432ffe8b..d9bfa956cc 100644
> > > > > > > --- a/hw/tpm/tpm_tis.c
> > > > > > > +++ b/hw/tpm/tpm_tis.c
> > > > > > > @@ -868,6 +868,7 @@ static void tpm_tis_reset(DeviceState *dev)
> > > > > > >      s->be_buffer_size = MIN(tpm_backend_get_buffer_size(s->be_driver),
> > > > > > >                              TPM_TIS_BUFFER_MAX);
> > > > > > >
> > > > > > > +    tpm_ppi_reset(&s->ppi);
> > > > > > >      tpm_backend_reset(s->be_driver);
> > > > > > >
> > > > > > >      s->active_locty = TPM_TIS_NO_LOCALITY;
> > > > > > > diff --git a/docs/specs/tpm.txt b/docs/specs/tpm.txt
> > > > > > > index 332c2ae597..ce9bda3c89 100644
> > > > > > > --- a/docs/specs/tpm.txt
> > > > > > > +++ b/docs/specs/tpm.txt
> > > > > > > @@ -121,6 +121,8 @@ layout:
> > > > > > >   +----------+--------+--------+-------------------------------------------+
> > > > > > >   | next_step|   0x1  |  0x159 | Operation to execute after reboot by      |
> > > > > > >   |          |        |        | firmware. Used by firmware.               |
> > > > > > > + +----------+--------+--------+-------------------------------------------+
> > > > > > > + | movv     |   0x1  |  0x15a | Memory overwrite variable                 |
> > > > > > >   +----------+--------+--------+-------------------------------------------+
> > > > > > >
> > > > > > >     The following values are supported for the 'func' field. They correspond
> > > > > > > diff --git a/hw/tpm/trace-events b/hw/tpm/trace-events
> > > > > > > index 25bee0cecf..920d32ad55 100644
> > > > > > > --- a/hw/tpm/trace-events
> > > > > > > +++ b/hw/tpm/trace-events
> > > > > > > @@ -51,3 +51,6 @@ tpm_tis_mmio_write_init_abort(void) "Initiating abort"
> > > > > > >  tpm_tis_mmio_write_lowering_irq(void) "Lowering IRQ"
> > > > > > >  tpm_tis_mmio_write_data2send(uint32_t value, unsigned size) "Data to send to TPM: 0x%08x (size=%d)"
> > > > > > >  tpm_tis_pre_save(uint8_t locty, uint32_t rw_offset) "locty: %d, rw_offset = %u"
> > > > > > > +
> > > > > > > +# hw/tpm/tpm_ppi.c
> > > > > > > +tpm_ppi_memset(uint8_t *ptr, size_t size) "memset: %p %zu"
> > > > > >
> > > > > >
> > > > >
> > > > >
> > > >
> > >
> > >
> > > --
> > > Marc-André Lureau
> > --
> > Dr. David Alan Gilbert / dgilbert@redhat.com / Manchester, UK
> 
> 
> 
> -- 
> Marc-André Lureau
--
Dr. David Alan Gilbert / dgilbert@redhat.com / Manchester, UK

Re: [Qemu-devel] [PATCH v10 6/6] tpm: add ACPI memory clear interface

[Marc-André Lureau]

Hi

On Thu, Sep 6, 2018 at 1:42 PM Dr. David Alan Gilbert
<dgilbert@redhat.com> wrote:
>
> * Marc-André Lureau (marcandre.lureau@gmail.com) wrote:
> > Hi
> >
> > On Thu, Sep 6, 2018 at 12:59 PM Dr. David Alan Gilbert
> > <dgilbert@redhat.com> wrote:
> > >
> > > * Marc-André Lureau (marcandre.lureau@gmail.com) wrote:
> > > > Hi
> > > >
> > > > On Thu, Sep 6, 2018 at 11:58 AM Igor Mammedov <imammedo@redhat.com> wrote:
> > > > >
> > > > > On Thu, 6 Sep 2018 07:50:09 +0400
> > > > > Marc-André Lureau <marcandre.lureau@gmail.com> wrote:
> > > > >
> > > > > > Hi
> > > > > >
> > > > > > On Tue, Sep 4, 2018 at 10:47 AM Igor Mammedov <imammedo@redhat.com> wrote:
> > > > > > >
> > > > > > > On Fri, 31 Aug 2018 19:24:24 +0200
> > > > > > > Marc-André Lureau <marcandre.lureau@redhat.com> wrote:
> > > > > > >
> > > > > > > > This allows to pass the last failing test from the Windows HLK TPM 2.0
> > > > > > > > TCG PPI 1.3 tests.
> > > > > > > >
> > > > > > > > The interface is described in the "TCG Platform Reset Attack
> > > > > > > > Mitigation Specification", chapter 6 "ACPI _DSM Function". According
> > > > > > > > to Laszlo, it's not so easy to implement in OVMF, he suggested to do
> > > > > > > > it in qemu instead.
> > > > > > > >
> > > > > > > > Signed-off-by: Marc-André Lureau <marcandre.lureau@redhat.com>
> > > > > > > > ---
> > > > > > > >  hw/tpm/tpm_ppi.h     |  2 ++
> > > > > > > >  hw/i386/acpi-build.c | 46 ++++++++++++++++++++++++++++++++++++++++++++
> > > > > > > >  hw/tpm/tpm_crb.c     |  1 +
> > > > > > > >  hw/tpm/tpm_ppi.c     | 23 ++++++++++++++++++++++
> > > > > > > >  hw/tpm/tpm_tis.c     |  1 +
> > > > > > > >  docs/specs/tpm.txt   |  2 ++
> > > > > > > >  hw/tpm/trace-events  |  3 +++
> > > > > > > >  7 files changed, 78 insertions(+)
> > > > > > > >
> > > > > > > > diff --git a/hw/tpm/tpm_ppi.h b/hw/tpm/tpm_ppi.h
> > > > > > > > index f6458bf87e..3239751e9f 100644
> > > > > > > > --- a/hw/tpm/tpm_ppi.h
> > > > > > > > +++ b/hw/tpm/tpm_ppi.h
> > > > > > > > @@ -23,4 +23,6 @@ typedef struct TPMPPI {
> > > > > > > >  bool tpm_ppi_init(TPMPPI *tpmppi, struct MemoryRegion *m,
> > > > > > > >                    hwaddr addr, Object *obj, Error **errp);
> > > > > > > >
> > > > > > > > +void tpm_ppi_reset(TPMPPI *tpmppi);
> > > > > > > > +
> > > > > > > >  #endif /* TPM_TPM_PPI_H */
> > > > > > > > diff --git a/hw/i386/acpi-build.c b/hw/i386/acpi-build.c
> > > > > > > > index c5e9a6e11d..2ab3e8fae7 100644
> > > > > > > > --- a/hw/i386/acpi-build.c
> > > > > > > > +++ b/hw/i386/acpi-build.c
> > > > > > > > @@ -1824,6 +1824,13 @@ build_tpm_ppi(TPMIf *tpm, Aml *dev)
> > > > > > > >      pprq = aml_name("PPRQ");
> > > > > > > >      pprm = aml_name("PPRM");
> > > > > > > >
> > > > > > > > +    aml_append(dev,
> > > > > > > > +               aml_operation_region("TPP3", AML_SYSTEM_MEMORY,
> > > > > > > > +                                    aml_int(TPM_PPI_ADDR_BASE + 0x15a),
> > > > > > > > +                                    0x1));
> > > > > > > > +    field = aml_field("TPP3", AML_BYTE_ACC, AML_NOLOCK, AML_PRESERVE);
> > > > > > > > +    aml_append(field, aml_named_field("MOVV", 8));
> > > > > > > > +    aml_append(dev, field);
> > > > > > > >      /*
> > > > > > > >       * DerefOf in Windows is broken with SYSTEM_MEMORY.  Use a dynamic
> > > > > > > >       * operation region inside of a method for getting FUNC[op].
> > > > > > > > @@ -2166,7 +2173,46 @@ build_tpm_ppi(TPMIf *tpm, Aml *dev)
> > > > > > > >              aml_append(ifctx, aml_return(aml_buffer(1, zerobyte)));
> > > > > > > >          }
> > > > > > > >          aml_append(method, ifctx);
> > > > > > > > +
> > > > > > > > +        ifctx = aml_if(
> > > > > > > > +            aml_equal(uuid,
> > > > > > > > +                      aml_touuid("376054ED-CC13-4675-901C-4756D7F2D45D")));
> > > > > > > > +        {
> > > > > > > > +            /* standard DSM query function */
> > > > > > > > +            ifctx2 = aml_if(aml_equal(function, zero));
> > > > > > > > +            {
> > > > > > > > +                uint8_t byte_list[1] = { 0x03 };
> > > > > > > > +                aml_append(ifctx2, aml_return(aml_buffer(1, byte_list)));
> > > > > > > > +            }
> > > > > > > > +            aml_append(ifctx, ifctx2);
> > > > > > > > +
> > > > > > > > +            /*
> > > > > > > > +             * TCG Platform Reset Attack Mitigation Specification 1.0 Ch.6
> > > > > > > > +             *
> > > > > > > > +             * Arg 2 (Integer): Function Index = 1
> > > > > > > > +             * Arg 3 (Package): Arguments = Package: Type: Integer
> > > > > > > > +             *                  Operation Value of the Request
> > > > > > > > +             * Returns: Type: Integer
> > > > > > > > +             *          0: Success
> > > > > > > > +             *          1: General Failure
> > > > > > > > +             */
> > > > > > > > +            ifctx2 = aml_if(aml_equal(function, one));
> > > > > > > > +            {
> > > > > > > > +                aml_append(ifctx2,
> > > > > > > > +                           aml_store(aml_derefof(aml_index(arguments, zero)),
> > > > > > > > +                                     op));
> > > > > > > > +                {
> > > > > > > > +                    aml_append(ifctx2, aml_store(op, aml_name("MOVV")));
> > > > > > > > +
> > > > > > > > +                    /* 0: success */
> > > > > > > > +                    aml_append(ifctx2, aml_return(zero));
> > > > > > > > +                }
> > > > > > > > +            }
> > > > > > > > +            aml_append(ifctx, ifctx2);
> > > > > > > > +        }
> > > > > > > > +        aml_append(method, ifctx);
> > > > > > > >      }
> > > > > > > > +
> > > > > > > >      aml_append(dev, method);
> > > > > > > >  }
> > > > > > > >
> > > > > > > > diff --git a/hw/tpm/tpm_crb.c b/hw/tpm/tpm_crb.c
> > > > > > > > index b243222fd6..48f6a716ad 100644
> > > > > > > > --- a/hw/tpm/tpm_crb.c
> > > > > > > > +++ b/hw/tpm/tpm_crb.c
> > > > > > > > @@ -233,6 +233,7 @@ static void tpm_crb_reset(void *dev)
> > > > > > > >  {
> > > > > > > >      CRBState *s = CRB(dev);
> > > > > > > >
> > > > > > > > +    tpm_ppi_reset(&s->ppi);
> > > > > > > >      tpm_backend_reset(s->tpmbe);
> > > > > > > >
> > > > > > > >      memset(s->regs, 0, sizeof(s->regs));
> > > > > > > > diff --git a/hw/tpm/tpm_ppi.c b/hw/tpm/tpm_ppi.c
> > > > > > > > index 8b46b9dd4b..ce43bc5729 100644
> > > > > > > > --- a/hw/tpm/tpm_ppi.c
> > > > > > > > +++ b/hw/tpm/tpm_ppi.c
> > > > > > > > @@ -16,8 +16,30 @@
> > > > > > > >  #include "qapi/error.h"
> > > > > > > >  #include "cpu.h"
> > > > > > > >  #include "sysemu/memory_mapping.h"
> > > > > > > > +#include "sysemu/reset.h"
> > > > > > > >  #include "migration/vmstate.h"
> > > > > > > >  #include "tpm_ppi.h"
> > > > > > > > +#include "trace.h"
> > > > > > > > +
> > > > > > > > +void tpm_ppi_reset(TPMPPI *tpmppi)
> > > > > > > > +{
> > > > > > >
> > > > > > >
> > > > > > > > +    char *ptr = memory_region_get_ram_ptr(&tpmppi->ram);
> > > > > > > nvdimm seems to use cpu_physical_memory_read() to access guest
> > > > > > > accessible memory, so question is what's difference?
> > > > > >
> > > > > > cpu_physical_memory_read() is higher level, doing dispatch on address
> > > > > > and length checks.
> > > > > >
> > > > > > This is a bit unnecessary, as ppi->buf could be accessed directly.
> > > > > [...]
> > > > > > > > +            memset(block->host_addr, 0,
> > > > > > > > +                   block->target_end - block->target_start);
> > > > > > > > +        }
> > > > > my concern here is that if we directly touch guest memory here
> > > > > we might get in trouble on migration without dirtying modified
> > > > > ranges
> > > >
> > > > It is a read-only of one byte.
> > > > by the time the reset handler is called, the memory must have been
> > > > already migrated.
> > >
> > > Looks like a write to me?
> >
> > the PPI RAM memory is read for the "memory clear" byte
> > The whole guest RAM is reset to 0 if set.
>
> Oh, I see; hmm.
> How do you avoid zeroing things like persistent memory? Or ROMs? Or EFI
> pflash?

guest_phys_blocks_append() only cares about RAM (see
guest_phys_blocks_region_add)

>
> > > Also, don't forget that a guest reset can happen during a migration.
> >
> > Hmm, does cpu_physical_memory_read()  guarantee the memory has been migrated?
> > Is there a way to wait for migration to be completed in a reset handler?
>
> No; remember that migration can take a significant amount of time (many
> minutes) as you stuff many GB of RAM down a network.
>
> So you can be in the situation where:
>      a) Migration starts
>      b) Migration sends a copy of most of RAM across
>      c) Guest dirties lots of RAM in parallel with b
>      d) migration sends some of the RAM again
>      e) guest reboots
>      f) migration keeps sending ram across
>      g) Migration finally completes and starts on destination
>
> a-f are all happening on the source side as the guest is still running
> and doing whatever it wants (including reboots).
>
> Given something like acpi-build.c's acpi_ram_update's call to
> memory_region_set_dirty, would that work for you?

after the memset(), it should then call:

memory_region_set_dirty(block->mr, 0, block->target_end - block->target_start);

looks about right?

thanks

> Dave
>
> > >
> > > Dave
> > >
> > > > >
> > > > > PS:
> > > > > feel free it ignore since I don't have a clue what I'm talking about :)
> > > > >
> > > > > > > > +        guest_phys_blocks_free(&guest_phys_blocks);
> > > > > > > > +    }
> > > > > > > > +}
> > > > > > > >
> > > > > > > >  bool tpm_ppi_init(TPMPPI *tpmppi, struct MemoryRegion *m,
> > > > > > > >                    hwaddr addr, Object *obj, Error **errp)
> > > > > > > > @@ -27,5 +49,6 @@ bool tpm_ppi_init(TPMPPI *tpmppi, struct MemoryRegion *m,
> > > > > > > >      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 70432ffe8b..d9bfa956cc 100644
> > > > > > > > --- a/hw/tpm/tpm_tis.c
> > > > > > > > +++ b/hw/tpm/tpm_tis.c
> > > > > > > > @@ -868,6 +868,7 @@ static void tpm_tis_reset(DeviceState *dev)
> > > > > > > >      s->be_buffer_size = MIN(tpm_backend_get_buffer_size(s->be_driver),
> > > > > > > >                              TPM_TIS_BUFFER_MAX);
> > > > > > > >
> > > > > > > > +    tpm_ppi_reset(&s->ppi);
> > > > > > > >      tpm_backend_reset(s->be_driver);
> > > > > > > >
> > > > > > > >      s->active_locty = TPM_TIS_NO_LOCALITY;
> > > > > > > > diff --git a/docs/specs/tpm.txt b/docs/specs/tpm.txt
> > > > > > > > index 332c2ae597..ce9bda3c89 100644
> > > > > > > > --- a/docs/specs/tpm.txt
> > > > > > > > +++ b/docs/specs/tpm.txt
> > > > > > > > @@ -121,6 +121,8 @@ layout:
> > > > > > > >   +----------+--------+--------+-------------------------------------------+
> > > > > > > >   | next_step|   0x1  |  0x159 | Operation to execute after reboot by      |
> > > > > > > >   |          |        |        | firmware. Used by firmware.               |
> > > > > > > > + +----------+--------+--------+-------------------------------------------+
> > > > > > > > + | movv     |   0x1  |  0x15a | Memory overwrite variable                 |
> > > > > > > >   +----------+--------+--------+-------------------------------------------+
> > > > > > > >
> > > > > > > >     The following values are supported for the 'func' field. They correspond
> > > > > > > > diff --git a/hw/tpm/trace-events b/hw/tpm/trace-events
> > > > > > > > index 25bee0cecf..920d32ad55 100644
> > > > > > > > --- a/hw/tpm/trace-events
> > > > > > > > +++ b/hw/tpm/trace-events
> > > > > > > > @@ -51,3 +51,6 @@ tpm_tis_mmio_write_init_abort(void) "Initiating abort"
> > > > > > > >  tpm_tis_mmio_write_lowering_irq(void) "Lowering IRQ"
> > > > > > > >  tpm_tis_mmio_write_data2send(uint32_t value, unsigned size) "Data to send to TPM: 0x%08x (size=%d)"
> > > > > > > >  tpm_tis_pre_save(uint8_t locty, uint32_t rw_offset) "locty: %d, rw_offset = %u"
> > > > > > > > +
> > > > > > > > +# hw/tpm/tpm_ppi.c
> > > > > > > > +tpm_ppi_memset(uint8_t *ptr, size_t size) "memset: %p %zu"
> > > > > > >
> > > > > > >
> > > > > >
> > > > > >
> > > > >
> > > >
> > > >
> > > > --
> > > > Marc-André Lureau
> > > --
> > > Dr. David Alan Gilbert / dgilbert@redhat.com / Manchester, UK
> >
> >
> >
> > --
> > Marc-André Lureau
> --
> Dr. David Alan Gilbert / dgilbert@redhat.com / Manchester, UK



-- 
Marc-André Lureau

Re: [Qemu-devel] [PATCH v10 6/6] tpm: add ACPI memory clear interface

[Dr. David Alan Gilbert]

* Marc-André Lureau (marcandre.lureau@gmail.com) wrote:
> Hi
> 
> On Thu, Sep 6, 2018 at 1:42 PM Dr. David Alan Gilbert
> <dgilbert@redhat.com> wrote:
> >
> > * Marc-André Lureau (marcandre.lureau@gmail.com) wrote:
> > > Hi
> > >
> > > On Thu, Sep 6, 2018 at 12:59 PM Dr. David Alan Gilbert
> > > <dgilbert@redhat.com> wrote:
> > > >
> > > > * Marc-André Lureau (marcandre.lureau@gmail.com) wrote:
> > > > > Hi
> > > > >
> > > > > On Thu, Sep 6, 2018 at 11:58 AM Igor Mammedov <imammedo@redhat.com> wrote:
> > > > > >
> > > > > > On Thu, 6 Sep 2018 07:50:09 +0400
> > > > > > Marc-André Lureau <marcandre.lureau@gmail.com> wrote:
> > > > > >
> > > > > > > Hi
> > > > > > >
> > > > > > > On Tue, Sep 4, 2018 at 10:47 AM Igor Mammedov <imammedo@redhat.com> wrote:
> > > > > > > >
> > > > > > > > On Fri, 31 Aug 2018 19:24:24 +0200
> > > > > > > > Marc-André Lureau <marcandre.lureau@redhat.com> wrote:
> > > > > > > >
> > > > > > > > > This allows to pass the last failing test from the Windows HLK TPM 2.0
> > > > > > > > > TCG PPI 1.3 tests.
> > > > > > > > >
> > > > > > > > > The interface is described in the "TCG Platform Reset Attack
> > > > > > > > > Mitigation Specification", chapter 6 "ACPI _DSM Function". According
> > > > > > > > > to Laszlo, it's not so easy to implement in OVMF, he suggested to do
> > > > > > > > > it in qemu instead.
> > > > > > > > >
> > > > > > > > > Signed-off-by: Marc-André Lureau <marcandre.lureau@redhat.com>
> > > > > > > > > ---
> > > > > > > > >  hw/tpm/tpm_ppi.h     |  2 ++
> > > > > > > > >  hw/i386/acpi-build.c | 46 ++++++++++++++++++++++++++++++++++++++++++++
> > > > > > > > >  hw/tpm/tpm_crb.c     |  1 +
> > > > > > > > >  hw/tpm/tpm_ppi.c     | 23 ++++++++++++++++++++++
> > > > > > > > >  hw/tpm/tpm_tis.c     |  1 +
> > > > > > > > >  docs/specs/tpm.txt   |  2 ++
> > > > > > > > >  hw/tpm/trace-events  |  3 +++
> > > > > > > > >  7 files changed, 78 insertions(+)
> > > > > > > > >
> > > > > > > > > diff --git a/hw/tpm/tpm_ppi.h b/hw/tpm/tpm_ppi.h
> > > > > > > > > index f6458bf87e..3239751e9f 100644
> > > > > > > > > --- a/hw/tpm/tpm_ppi.h
> > > > > > > > > +++ b/hw/tpm/tpm_ppi.h
> > > > > > > > > @@ -23,4 +23,6 @@ typedef struct TPMPPI {
> > > > > > > > >  bool tpm_ppi_init(TPMPPI *tpmppi, struct MemoryRegion *m,
> > > > > > > > >                    hwaddr addr, Object *obj, Error **errp);
> > > > > > > > >
> > > > > > > > > +void tpm_ppi_reset(TPMPPI *tpmppi);
> > > > > > > > > +
> > > > > > > > >  #endif /* TPM_TPM_PPI_H */
> > > > > > > > > diff --git a/hw/i386/acpi-build.c b/hw/i386/acpi-build.c
> > > > > > > > > index c5e9a6e11d..2ab3e8fae7 100644
> > > > > > > > > --- a/hw/i386/acpi-build.c
> > > > > > > > > +++ b/hw/i386/acpi-build.c
> > > > > > > > > @@ -1824,6 +1824,13 @@ build_tpm_ppi(TPMIf *tpm, Aml *dev)
> > > > > > > > >      pprq = aml_name("PPRQ");
> > > > > > > > >      pprm = aml_name("PPRM");
> > > > > > > > >
> > > > > > > > > +    aml_append(dev,
> > > > > > > > > +               aml_operation_region("TPP3", AML_SYSTEM_MEMORY,
> > > > > > > > > +                                    aml_int(TPM_PPI_ADDR_BASE + 0x15a),
> > > > > > > > > +                                    0x1));
> > > > > > > > > +    field = aml_field("TPP3", AML_BYTE_ACC, AML_NOLOCK, AML_PRESERVE);
> > > > > > > > > +    aml_append(field, aml_named_field("MOVV", 8));
> > > > > > > > > +    aml_append(dev, field);
> > > > > > > > >      /*
> > > > > > > > >       * DerefOf in Windows is broken with SYSTEM_MEMORY.  Use a dynamic
> > > > > > > > >       * operation region inside of a method for getting FUNC[op].
> > > > > > > > > @@ -2166,7 +2173,46 @@ build_tpm_ppi(TPMIf *tpm, Aml *dev)
> > > > > > > > >              aml_append(ifctx, aml_return(aml_buffer(1, zerobyte)));
> > > > > > > > >          }
> > > > > > > > >          aml_append(method, ifctx);
> > > > > > > > > +
> > > > > > > > > +        ifctx = aml_if(
> > > > > > > > > +            aml_equal(uuid,
> > > > > > > > > +                      aml_touuid("376054ED-CC13-4675-901C-4756D7F2D45D")));
> > > > > > > > > +        {
> > > > > > > > > +            /* standard DSM query function */
> > > > > > > > > +            ifctx2 = aml_if(aml_equal(function, zero));
> > > > > > > > > +            {
> > > > > > > > > +                uint8_t byte_list[1] = { 0x03 };
> > > > > > > > > +                aml_append(ifctx2, aml_return(aml_buffer(1, byte_list)));
> > > > > > > > > +            }
> > > > > > > > > +            aml_append(ifctx, ifctx2);
> > > > > > > > > +
> > > > > > > > > +            /*
> > > > > > > > > +             * TCG Platform Reset Attack Mitigation Specification 1.0 Ch.6
> > > > > > > > > +             *
> > > > > > > > > +             * Arg 2 (Integer): Function Index = 1
> > > > > > > > > +             * Arg 3 (Package): Arguments = Package: Type: Integer
> > > > > > > > > +             *                  Operation Value of the Request
> > > > > > > > > +             * Returns: Type: Integer
> > > > > > > > > +             *          0: Success
> > > > > > > > > +             *          1: General Failure
> > > > > > > > > +             */
> > > > > > > > > +            ifctx2 = aml_if(aml_equal(function, one));
> > > > > > > > > +            {
> > > > > > > > > +                aml_append(ifctx2,
> > > > > > > > > +                           aml_store(aml_derefof(aml_index(arguments, zero)),
> > > > > > > > > +                                     op));
> > > > > > > > > +                {
> > > > > > > > > +                    aml_append(ifctx2, aml_store(op, aml_name("MOVV")));
> > > > > > > > > +
> > > > > > > > > +                    /* 0: success */
> > > > > > > > > +                    aml_append(ifctx2, aml_return(zero));
> > > > > > > > > +                }
> > > > > > > > > +            }
> > > > > > > > > +            aml_append(ifctx, ifctx2);
> > > > > > > > > +        }
> > > > > > > > > +        aml_append(method, ifctx);
> > > > > > > > >      }
> > > > > > > > > +
> > > > > > > > >      aml_append(dev, method);
> > > > > > > > >  }
> > > > > > > > >
> > > > > > > > > diff --git a/hw/tpm/tpm_crb.c b/hw/tpm/tpm_crb.c
> > > > > > > > > index b243222fd6..48f6a716ad 100644
> > > > > > > > > --- a/hw/tpm/tpm_crb.c
> > > > > > > > > +++ b/hw/tpm/tpm_crb.c
> > > > > > > > > @@ -233,6 +233,7 @@ static void tpm_crb_reset(void *dev)
> > > > > > > > >  {
> > > > > > > > >      CRBState *s = CRB(dev);
> > > > > > > > >
> > > > > > > > > +    tpm_ppi_reset(&s->ppi);
> > > > > > > > >      tpm_backend_reset(s->tpmbe);
> > > > > > > > >
> > > > > > > > >      memset(s->regs, 0, sizeof(s->regs));
> > > > > > > > > diff --git a/hw/tpm/tpm_ppi.c b/hw/tpm/tpm_ppi.c
> > > > > > > > > index 8b46b9dd4b..ce43bc5729 100644
> > > > > > > > > --- a/hw/tpm/tpm_ppi.c
> > > > > > > > > +++ b/hw/tpm/tpm_ppi.c
> > > > > > > > > @@ -16,8 +16,30 @@
> > > > > > > > >  #include "qapi/error.h"
> > > > > > > > >  #include "cpu.h"
> > > > > > > > >  #include "sysemu/memory_mapping.h"
> > > > > > > > > +#include "sysemu/reset.h"
> > > > > > > > >  #include "migration/vmstate.h"
> > > > > > > > >  #include "tpm_ppi.h"
> > > > > > > > > +#include "trace.h"
> > > > > > > > > +
> > > > > > > > > +void tpm_ppi_reset(TPMPPI *tpmppi)
> > > > > > > > > +{
> > > > > > > >
> > > > > > > >
> > > > > > > > > +    char *ptr = memory_region_get_ram_ptr(&tpmppi->ram);
> > > > > > > > nvdimm seems to use cpu_physical_memory_read() to access guest
> > > > > > > > accessible memory, so question is what's difference?
> > > > > > >
> > > > > > > cpu_physical_memory_read() is higher level, doing dispatch on address
> > > > > > > and length checks.
> > > > > > >
> > > > > > > This is a bit unnecessary, as ppi->buf could be accessed directly.
> > > > > > [...]
> > > > > > > > > +            memset(block->host_addr, 0,
> > > > > > > > > +                   block->target_end - block->target_start);
> > > > > > > > > +        }
> > > > > > my concern here is that if we directly touch guest memory here
> > > > > > we might get in trouble on migration without dirtying modified
> > > > > > ranges
> > > > >
> > > > > It is a read-only of one byte.
> > > > > by the time the reset handler is called, the memory must have been
> > > > > already migrated.
> > > >
> > > > Looks like a write to me?
> > >
> > > the PPI RAM memory is read for the "memory clear" byte
> > > The whole guest RAM is reset to 0 if set.
> >
> > Oh, I see; hmm.
> > How do you avoid zeroing things like persistent memory? Or ROMs? Or EFI
> > pflash?
> 
> guest_phys_blocks_append() only cares about RAM (see
> guest_phys_blocks_region_add)

Hmm, promising; it uses:
    if (!memory_region_is_ram(section->mr) ||
        memory_region_is_ram_device(section->mr)) {
        return;
    }

so ram_device is used by vfio and vhost-user; I don't see anything else.
pflash init's as a rom_device so that's probably OK.
But things like backends/hostmem-file.c just use
memory_region_init_ram_from_file even if they're shared or PMEM.
So, I think this would wipe an attached PMEM device - do you want to or
not?

> >
> > > > Also, don't forget that a guest reset can happen during a migration.
> > >
> > > Hmm, does cpu_physical_memory_read()  guarantee the memory has been migrated?
> > > Is there a way to wait for migration to be completed in a reset handler?
> >
> > No; remember that migration can take a significant amount of time (many
> > minutes) as you stuff many GB of RAM down a network.
> >
> > So you can be in the situation where:
> >      a) Migration starts
> >      b) Migration sends a copy of most of RAM across
> >      c) Guest dirties lots of RAM in parallel with b
> >      d) migration sends some of the RAM again
> >      e) guest reboots
> >      f) migration keeps sending ram across
> >      g) Migration finally completes and starts on destination
> >
> > a-f are all happening on the source side as the guest is still running
> > and doing whatever it wants (including reboots).
> >
> > Given something like acpi-build.c's acpi_ram_update's call to
> > memory_region_set_dirty, would that work for you?
> 
> after the memset(), it should then call:
> 
> memory_region_set_dirty(block->mr, 0, block->target_end - block->target_start);
> 
> looks about right?

I think so.

Dave

> 
> thanks
> 
> > Dave
> >
> > > >
> > > > Dave
> > > >
> > > > > >
> > > > > > PS:
> > > > > > feel free it ignore since I don't have a clue what I'm talking about :)
> > > > > >
> > > > > > > > > +        guest_phys_blocks_free(&guest_phys_blocks);
> > > > > > > > > +    }
> > > > > > > > > +}
> > > > > > > > >
> > > > > > > > >  bool tpm_ppi_init(TPMPPI *tpmppi, struct MemoryRegion *m,
> > > > > > > > >                    hwaddr addr, Object *obj, Error **errp)
> > > > > > > > > @@ -27,5 +49,6 @@ bool tpm_ppi_init(TPMPPI *tpmppi, struct MemoryRegion *m,
> > > > > > > > >      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 70432ffe8b..d9bfa956cc 100644
> > > > > > > > > --- a/hw/tpm/tpm_tis.c
> > > > > > > > > +++ b/hw/tpm/tpm_tis.c
> > > > > > > > > @@ -868,6 +868,7 @@ static void tpm_tis_reset(DeviceState *dev)
> > > > > > > > >      s->be_buffer_size = MIN(tpm_backend_get_buffer_size(s->be_driver),
> > > > > > > > >                              TPM_TIS_BUFFER_MAX);
> > > > > > > > >
> > > > > > > > > +    tpm_ppi_reset(&s->ppi);
> > > > > > > > >      tpm_backend_reset(s->be_driver);
> > > > > > > > >
> > > > > > > > >      s->active_locty = TPM_TIS_NO_LOCALITY;
> > > > > > > > > diff --git a/docs/specs/tpm.txt b/docs/specs/tpm.txt
> > > > > > > > > index 332c2ae597..ce9bda3c89 100644
> > > > > > > > > --- a/docs/specs/tpm.txt
> > > > > > > > > +++ b/docs/specs/tpm.txt
> > > > > > > > > @@ -121,6 +121,8 @@ layout:
> > > > > > > > >   +----------+--------+--------+-------------------------------------------+
> > > > > > > > >   | next_step|   0x1  |  0x159 | Operation to execute after reboot by      |
> > > > > > > > >   |          |        |        | firmware. Used by firmware.               |
> > > > > > > > > + +----------+--------+--------+-------------------------------------------+
> > > > > > > > > + | movv     |   0x1  |  0x15a | Memory overwrite variable                 |
> > > > > > > > >   +----------+--------+--------+-------------------------------------------+
> > > > > > > > >
> > > > > > > > >     The following values are supported for the 'func' field. They correspond
> > > > > > > > > diff --git a/hw/tpm/trace-events b/hw/tpm/trace-events
> > > > > > > > > index 25bee0cecf..920d32ad55 100644
> > > > > > > > > --- a/hw/tpm/trace-events
> > > > > > > > > +++ b/hw/tpm/trace-events
> > > > > > > > > @@ -51,3 +51,6 @@ tpm_tis_mmio_write_init_abort(void) "Initiating abort"
> > > > > > > > >  tpm_tis_mmio_write_lowering_irq(void) "Lowering IRQ"
> > > > > > > > >  tpm_tis_mmio_write_data2send(uint32_t value, unsigned size) "Data to send to TPM: 0x%08x (size=%d)"
> > > > > > > > >  tpm_tis_pre_save(uint8_t locty, uint32_t rw_offset) "locty: %d, rw_offset = %u"
> > > > > > > > > +
> > > > > > > > > +# hw/tpm/tpm_ppi.c
> > > > > > > > > +tpm_ppi_memset(uint8_t *ptr, size_t size) "memset: %p %zu"
> > > > > > > >
> > > > > > > >
> > > > > > >
> > > > > > >
> > > > > >
> > > > >
> > > > >
> > > > > --
> > > > > Marc-André Lureau
> > > > --
> > > > Dr. David Alan Gilbert / dgilbert@redhat.com / Manchester, UK
> > >
> > >
> > >
> > > --
> > > Marc-André Lureau
> > --
> > Dr. David Alan Gilbert / dgilbert@redhat.com / Manchester, UK
> 
> 
> 
> -- 
> Marc-André Lureau
--
Dr. David Alan Gilbert / dgilbert@redhat.com / Manchester, UK

Re: [Qemu-devel] [PATCH v10 6/6] tpm: add ACPI memory clear interface

[Laszlo Ersek]

On 09/06/18 19:23, Dr. David Alan Gilbert wrote:
> * Marc-André Lureau (marcandre.lureau@gmail.com) wrote:
>> Hi
>>
>> On Thu, Sep 6, 2018 at 1:42 PM Dr. David Alan Gilbert
>> <dgilbert@redhat.com> wrote:
>>>
>>> * Marc-André Lureau (marcandre.lureau@gmail.com) wrote:
>>>> Hi
>>>>
>>>> On Thu, Sep 6, 2018 at 12:59 PM Dr. David Alan Gilbert
>>>> <dgilbert@redhat.com> wrote:
>>>>>
>>>>> * Marc-André Lureau (marcandre.lureau@gmail.com) wrote:
>>>>>> Hi
>>>>>>
>>>>>> On Thu, Sep 6, 2018 at 11:58 AM Igor Mammedov <imammedo@redhat.com> wrote:
>>>>>>>
>>>>>>> On Thu, 6 Sep 2018 07:50:09 +0400
>>>>>>> Marc-André Lureau <marcandre.lureau@gmail.com> wrote:
>>>>>>>
>>>>>>>> Hi
>>>>>>>>
>>>>>>>> On Tue, Sep 4, 2018 at 10:47 AM Igor Mammedov <imammedo@redhat.com> wrote:
>>>>>>>>>
>>>>>>>>> On Fri, 31 Aug 2018 19:24:24 +0200
>>>>>>>>> Marc-André Lureau <marcandre.lureau@redhat.com> wrote:
>>>>>>>>>
>>>>>>>>>> This allows to pass the last failing test from the Windows HLK TPM 2.0
>>>>>>>>>> TCG PPI 1.3 tests.
>>>>>>>>>>
>>>>>>>>>> The interface is described in the "TCG Platform Reset Attack
>>>>>>>>>> Mitigation Specification", chapter 6 "ACPI _DSM Function". According
>>>>>>>>>> to Laszlo, it's not so easy to implement in OVMF, he suggested to do
>>>>>>>>>> it in qemu instead.
>>>>>>>>>>
>>>>>>>>>> Signed-off-by: Marc-André Lureau <marcandre.lureau@redhat.com>
>>>>>>>>>> ---
>>>>>>>>>>  hw/tpm/tpm_ppi.h     |  2 ++
>>>>>>>>>>  hw/i386/acpi-build.c | 46 ++++++++++++++++++++++++++++++++++++++++++++
>>>>>>>>>>  hw/tpm/tpm_crb.c     |  1 +
>>>>>>>>>>  hw/tpm/tpm_ppi.c     | 23 ++++++++++++++++++++++
>>>>>>>>>>  hw/tpm/tpm_tis.c     |  1 +
>>>>>>>>>>  docs/specs/tpm.txt   |  2 ++
>>>>>>>>>>  hw/tpm/trace-events  |  3 +++
>>>>>>>>>>  7 files changed, 78 insertions(+)
>>>>>>>>>>
>>>>>>>>>> diff --git a/hw/tpm/tpm_ppi.h b/hw/tpm/tpm_ppi.h
>>>>>>>>>> index f6458bf87e..3239751e9f 100644
>>>>>>>>>> --- a/hw/tpm/tpm_ppi.h
>>>>>>>>>> +++ b/hw/tpm/tpm_ppi.h
>>>>>>>>>> @@ -23,4 +23,6 @@ typedef struct TPMPPI {
>>>>>>>>>>  bool tpm_ppi_init(TPMPPI *tpmppi, struct MemoryRegion *m,
>>>>>>>>>>                    hwaddr addr, Object *obj, Error **errp);
>>>>>>>>>>
>>>>>>>>>> +void tpm_ppi_reset(TPMPPI *tpmppi);
>>>>>>>>>> +
>>>>>>>>>>  #endif /* TPM_TPM_PPI_H */
>>>>>>>>>> diff --git a/hw/i386/acpi-build.c b/hw/i386/acpi-build.c
>>>>>>>>>> index c5e9a6e11d..2ab3e8fae7 100644
>>>>>>>>>> --- a/hw/i386/acpi-build.c
>>>>>>>>>> +++ b/hw/i386/acpi-build.c
>>>>>>>>>> @@ -1824,6 +1824,13 @@ build_tpm_ppi(TPMIf *tpm, Aml *dev)
>>>>>>>>>>      pprq = aml_name("PPRQ");
>>>>>>>>>>      pprm = aml_name("PPRM");
>>>>>>>>>>
>>>>>>>>>> +    aml_append(dev,
>>>>>>>>>> +               aml_operation_region("TPP3", AML_SYSTEM_MEMORY,
>>>>>>>>>> +                                    aml_int(TPM_PPI_ADDR_BASE + 0x15a),
>>>>>>>>>> +                                    0x1));
>>>>>>>>>> +    field = aml_field("TPP3", AML_BYTE_ACC, AML_NOLOCK, AML_PRESERVE);
>>>>>>>>>> +    aml_append(field, aml_named_field("MOVV", 8));
>>>>>>>>>> +    aml_append(dev, field);
>>>>>>>>>>      /*
>>>>>>>>>>       * DerefOf in Windows is broken with SYSTEM_MEMORY.  Use a dynamic
>>>>>>>>>>       * operation region inside of a method for getting FUNC[op].
>>>>>>>>>> @@ -2166,7 +2173,46 @@ build_tpm_ppi(TPMIf *tpm, Aml *dev)
>>>>>>>>>>              aml_append(ifctx, aml_return(aml_buffer(1, zerobyte)));
>>>>>>>>>>          }
>>>>>>>>>>          aml_append(method, ifctx);
>>>>>>>>>> +
>>>>>>>>>> +        ifctx = aml_if(
>>>>>>>>>> +            aml_equal(uuid,
>>>>>>>>>> +                      aml_touuid("376054ED-CC13-4675-901C-4756D7F2D45D")));
>>>>>>>>>> +        {
>>>>>>>>>> +            /* standard DSM query function */
>>>>>>>>>> +            ifctx2 = aml_if(aml_equal(function, zero));
>>>>>>>>>> +            {
>>>>>>>>>> +                uint8_t byte_list[1] = { 0x03 };
>>>>>>>>>> +                aml_append(ifctx2, aml_return(aml_buffer(1, byte_list)));
>>>>>>>>>> +            }
>>>>>>>>>> +            aml_append(ifctx, ifctx2);
>>>>>>>>>> +
>>>>>>>>>> +            /*
>>>>>>>>>> +             * TCG Platform Reset Attack Mitigation Specification 1.0 Ch.6
>>>>>>>>>> +             *
>>>>>>>>>> +             * Arg 2 (Integer): Function Index = 1
>>>>>>>>>> +             * Arg 3 (Package): Arguments = Package: Type: Integer
>>>>>>>>>> +             *                  Operation Value of the Request
>>>>>>>>>> +             * Returns: Type: Integer
>>>>>>>>>> +             *          0: Success
>>>>>>>>>> +             *          1: General Failure
>>>>>>>>>> +             */
>>>>>>>>>> +            ifctx2 = aml_if(aml_equal(function, one));
>>>>>>>>>> +            {
>>>>>>>>>> +                aml_append(ifctx2,
>>>>>>>>>> +                           aml_store(aml_derefof(aml_index(arguments, zero)),
>>>>>>>>>> +                                     op));
>>>>>>>>>> +                {
>>>>>>>>>> +                    aml_append(ifctx2, aml_store(op, aml_name("MOVV")));
>>>>>>>>>> +
>>>>>>>>>> +                    /* 0: success */
>>>>>>>>>> +                    aml_append(ifctx2, aml_return(zero));
>>>>>>>>>> +                }
>>>>>>>>>> +            }
>>>>>>>>>> +            aml_append(ifctx, ifctx2);
>>>>>>>>>> +        }
>>>>>>>>>> +        aml_append(method, ifctx);
>>>>>>>>>>      }
>>>>>>>>>> +
>>>>>>>>>>      aml_append(dev, method);
>>>>>>>>>>  }
>>>>>>>>>>
>>>>>>>>>> diff --git a/hw/tpm/tpm_crb.c b/hw/tpm/tpm_crb.c
>>>>>>>>>> index b243222fd6..48f6a716ad 100644
>>>>>>>>>> --- a/hw/tpm/tpm_crb.c
>>>>>>>>>> +++ b/hw/tpm/tpm_crb.c
>>>>>>>>>> @@ -233,6 +233,7 @@ static void tpm_crb_reset(void *dev)
>>>>>>>>>>  {
>>>>>>>>>>      CRBState *s = CRB(dev);
>>>>>>>>>>
>>>>>>>>>> +    tpm_ppi_reset(&s->ppi);
>>>>>>>>>>      tpm_backend_reset(s->tpmbe);
>>>>>>>>>>
>>>>>>>>>>      memset(s->regs, 0, sizeof(s->regs));
>>>>>>>>>> diff --git a/hw/tpm/tpm_ppi.c b/hw/tpm/tpm_ppi.c
>>>>>>>>>> index 8b46b9dd4b..ce43bc5729 100644
>>>>>>>>>> --- a/hw/tpm/tpm_ppi.c
>>>>>>>>>> +++ b/hw/tpm/tpm_ppi.c
>>>>>>>>>> @@ -16,8 +16,30 @@
>>>>>>>>>>  #include "qapi/error.h"
>>>>>>>>>>  #include "cpu.h"
>>>>>>>>>>  #include "sysemu/memory_mapping.h"
>>>>>>>>>> +#include "sysemu/reset.h"
>>>>>>>>>>  #include "migration/vmstate.h"
>>>>>>>>>>  #include "tpm_ppi.h"
>>>>>>>>>> +#include "trace.h"
>>>>>>>>>> +
>>>>>>>>>> +void tpm_ppi_reset(TPMPPI *tpmppi)
>>>>>>>>>> +{
>>>>>>>>>
>>>>>>>>>
>>>>>>>>>> +    char *ptr = memory_region_get_ram_ptr(&tpmppi->ram);
>>>>>>>>> nvdimm seems to use cpu_physical_memory_read() to access guest
>>>>>>>>> accessible memory, so question is what's difference?
>>>>>>>>
>>>>>>>> cpu_physical_memory_read() is higher level, doing dispatch on address
>>>>>>>> and length checks.
>>>>>>>>
>>>>>>>> This is a bit unnecessary, as ppi->buf could be accessed directly.
>>>>>>> [...]
>>>>>>>>>> +            memset(block->host_addr, 0,
>>>>>>>>>> +                   block->target_end - block->target_start);
>>>>>>>>>> +        }
>>>>>>> my concern here is that if we directly touch guest memory here
>>>>>>> we might get in trouble on migration without dirtying modified
>>>>>>> ranges
>>>>>>
>>>>>> It is a read-only of one byte.
>>>>>> by the time the reset handler is called, the memory must have been
>>>>>> already migrated.
>>>>>
>>>>> Looks like a write to me?
>>>>
>>>> the PPI RAM memory is read for the "memory clear" byte
>>>> The whole guest RAM is reset to 0 if set.
>>>
>>> Oh, I see; hmm.
>>> How do you avoid zeroing things like persistent memory? Or ROMs? Or EFI
>>> pflash?
>>
>> guest_phys_blocks_append() only cares about RAM (see
>> guest_phys_blocks_region_add)
> 
> Hmm, promising; it uses:
>     if (!memory_region_is_ram(section->mr) ||
>         memory_region_is_ram_device(section->mr)) {
>         return;
>     }
> 
> so ram_device is used by vfio and vhost-user; I don't see anything else.
> pflash init's as a rom_device so that's probably OK.
> But things like backends/hostmem-file.c just use
> memory_region_init_ram_from_file even if they're shared or PMEM.
> So, I think this would wipe an attached PMEM device - do you want to or
> not?

I think that question could be put, "does the reset attack mitigation
spec recommend / require clearing persistent memory"? I've got no idea.
The reset attack is that the platform is re-set (forcibly, uncleanly)
while the original OS holds some secrets in memory, then the attacker's
OS (or firmware application) is loaded, and those scavenge the
leftovers. Would the original OS keep secrets in PMEM? I don't know.

I guess all address space that a guest OS could consider as "read-write
memory" should be wiped. I think guest_phys_blocks_append() is a good
choice here; originally I wrote that for supporting guest RAM dump; see
commit c5d7f60f06142. Note that the is_ram_device bit was added
separately, see commits e4dc3f5909ab9 and 21e00fa55f3fd -- but that's a
change in the "right" direction here, because it *restricts* what
regions qualify (for dumping, and here for clearing).

> 
>>>
>>>>> Also, don't forget that a guest reset can happen during a migration.
>>>>
>>>> Hmm, does cpu_physical_memory_read()  guarantee the memory has been migrated?
>>>> Is there a way to wait for migration to be completed in a reset handler?
>>>
>>> No; remember that migration can take a significant amount of time (many
>>> minutes) as you stuff many GB of RAM down a network.
>>>
>>> So you can be in the situation where:
>>>      a) Migration starts
>>>      b) Migration sends a copy of most of RAM across
>>>      c) Guest dirties lots of RAM in parallel with b
>>>      d) migration sends some of the RAM again
>>>      e) guest reboots
>>>      f) migration keeps sending ram across
>>>      g) Migration finally completes and starts on destination
>>>
>>> a-f are all happening on the source side as the guest is still running
>>> and doing whatever it wants (including reboots).
>>>
>>> Given something like acpi-build.c's acpi_ram_update's call to
>>> memory_region_set_dirty, would that work for you?
>>
>> after the memset(), it should then call:
>>
>> memory_region_set_dirty(block->mr, 0, block->target_end - block->target_start);
>>
>> looks about right?
> 
> I think so.

I'll admit I can't follow the dirtying discussion. But, I'm reminded of
another commit of Dave's, namely 90c647db8d59 ("Fix pflash migration",
2016-04-15). Would the same trick apply here?

(

BTW, I'm sorry about not having following this series closely -- I feel
bad that we can't solve this within the firmware, but we really can't.
The issue is that this memory clearing would have to occur really early
into the firmware, at the latest in the PEI phase.

However, both the 32-bit and the 64-bit variants of OVMF's PEI phase
have access only to the lowest 4GB of the memory address space. Mapping
all RAM (even with a "sliding bank") for clearing it would be a real
mess. To that, add the fact that OVMF's PEI phase executes from RAM (not
from pflash -- in OVMF, only SEC executes from pflash, and it
decompresses the PEI + DXE firmware volumes to RAM), so PEI would have
to clear all RAM *except* the areas its own self occupies, such as code,
global variables (static data), heap, stack, other processor data
structures (IDT, GDT, page tables, ...). And, no gaps inside those
should be left out either, because the previous OS might have left
secrets there...

This is actually easier for firmware that runs on physical hardware; for
two reasons. First, on physical hardware, the PEI phase of the firmware
runs from pflash (it might use CAR, Cache-As-RAM, for a small temporary
r/w storage), so it doesn't have to worry about scribbling over itself.
Second, on physical hardware, the memory controller too has to be booted
up -- the PEI code that does this is all vendors' most closely guarded
secret, and *never* open source --; and when the firmware massages
chipset registers for that, it can use non-architected means to get the
RAM to clear itself.

In comparison, in QEMU/KVM guests, the RAM just *is*. While that's a
huge relief most of the time, in this case, the fact that the RAM can
start out as nonzero, is a big problem. Hence my plea to implement the
feature in QEMU.

)

Thanks,
Laszlo

Re: [Qemu-devel] [PATCH v10 6/6] tpm: add ACPI memory clear interface

[Dr. David Alan Gilbert]

* Laszlo Ersek (lersek@redhat.com) wrote:
> On 09/06/18 19:23, Dr. David Alan Gilbert wrote:
> > * Marc-André Lureau (marcandre.lureau@gmail.com) wrote:
> >> Hi
> >>
> >> On Thu, Sep 6, 2018 at 1:42 PM Dr. David Alan Gilbert
> >> <dgilbert@redhat.com> wrote:
> >>>
> >>> * Marc-André Lureau (marcandre.lureau@gmail.com) wrote:
> >>>> Hi
> >>>>
> >>>> On Thu, Sep 6, 2018 at 12:59 PM Dr. David Alan Gilbert
> >>>> <dgilbert@redhat.com> wrote:
> >>>>>
> >>>>> * Marc-André Lureau (marcandre.lureau@gmail.com) wrote:
> >>>>>> Hi
> >>>>>>
> >>>>>> On Thu, Sep 6, 2018 at 11:58 AM Igor Mammedov <imammedo@redhat.com> wrote:
> >>>>>>>
> >>>>>>> On Thu, 6 Sep 2018 07:50:09 +0400
> >>>>>>> Marc-André Lureau <marcandre.lureau@gmail.com> wrote:
> >>>>>>>
> >>>>>>>> Hi
> >>>>>>>>
> >>>>>>>> On Tue, Sep 4, 2018 at 10:47 AM Igor Mammedov <imammedo@redhat.com> wrote:
> >>>>>>>>>
> >>>>>>>>> On Fri, 31 Aug 2018 19:24:24 +0200
> >>>>>>>>> Marc-André Lureau <marcandre.lureau@redhat.com> wrote:
> >>>>>>>>>
> >>>>>>>>>> This allows to pass the last failing test from the Windows HLK TPM 2.0
> >>>>>>>>>> TCG PPI 1.3 tests.
> >>>>>>>>>>
> >>>>>>>>>> The interface is described in the "TCG Platform Reset Attack
> >>>>>>>>>> Mitigation Specification", chapter 6 "ACPI _DSM Function". According
> >>>>>>>>>> to Laszlo, it's not so easy to implement in OVMF, he suggested to do
> >>>>>>>>>> it in qemu instead.
> >>>>>>>>>>
> >>>>>>>>>> Signed-off-by: Marc-André Lureau <marcandre.lureau@redhat.com>
> >>>>>>>>>> ---
> >>>>>>>>>>  hw/tpm/tpm_ppi.h     |  2 ++
> >>>>>>>>>>  hw/i386/acpi-build.c | 46 ++++++++++++++++++++++++++++++++++++++++++++
> >>>>>>>>>>  hw/tpm/tpm_crb.c     |  1 +
> >>>>>>>>>>  hw/tpm/tpm_ppi.c     | 23 ++++++++++++++++++++++
> >>>>>>>>>>  hw/tpm/tpm_tis.c     |  1 +
> >>>>>>>>>>  docs/specs/tpm.txt   |  2 ++
> >>>>>>>>>>  hw/tpm/trace-events  |  3 +++
> >>>>>>>>>>  7 files changed, 78 insertions(+)
> >>>>>>>>>>
> >>>>>>>>>> diff --git a/hw/tpm/tpm_ppi.h b/hw/tpm/tpm_ppi.h
> >>>>>>>>>> index f6458bf87e..3239751e9f 100644
> >>>>>>>>>> --- a/hw/tpm/tpm_ppi.h
> >>>>>>>>>> +++ b/hw/tpm/tpm_ppi.h
> >>>>>>>>>> @@ -23,4 +23,6 @@ typedef struct TPMPPI {
> >>>>>>>>>>  bool tpm_ppi_init(TPMPPI *tpmppi, struct MemoryRegion *m,
> >>>>>>>>>>                    hwaddr addr, Object *obj, Error **errp);
> >>>>>>>>>>
> >>>>>>>>>> +void tpm_ppi_reset(TPMPPI *tpmppi);
> >>>>>>>>>> +
> >>>>>>>>>>  #endif /* TPM_TPM_PPI_H */
> >>>>>>>>>> diff --git a/hw/i386/acpi-build.c b/hw/i386/acpi-build.c
> >>>>>>>>>> index c5e9a6e11d..2ab3e8fae7 100644
> >>>>>>>>>> --- a/hw/i386/acpi-build.c
> >>>>>>>>>> +++ b/hw/i386/acpi-build.c
> >>>>>>>>>> @@ -1824,6 +1824,13 @@ build_tpm_ppi(TPMIf *tpm, Aml *dev)
> >>>>>>>>>>      pprq = aml_name("PPRQ");
> >>>>>>>>>>      pprm = aml_name("PPRM");
> >>>>>>>>>>
> >>>>>>>>>> +    aml_append(dev,
> >>>>>>>>>> +               aml_operation_region("TPP3", AML_SYSTEM_MEMORY,
> >>>>>>>>>> +                                    aml_int(TPM_PPI_ADDR_BASE + 0x15a),
> >>>>>>>>>> +                                    0x1));
> >>>>>>>>>> +    field = aml_field("TPP3", AML_BYTE_ACC, AML_NOLOCK, AML_PRESERVE);
> >>>>>>>>>> +    aml_append(field, aml_named_field("MOVV", 8));
> >>>>>>>>>> +    aml_append(dev, field);
> >>>>>>>>>>      /*
> >>>>>>>>>>       * DerefOf in Windows is broken with SYSTEM_MEMORY.  Use a dynamic
> >>>>>>>>>>       * operation region inside of a method for getting FUNC[op].
> >>>>>>>>>> @@ -2166,7 +2173,46 @@ build_tpm_ppi(TPMIf *tpm, Aml *dev)
> >>>>>>>>>>              aml_append(ifctx, aml_return(aml_buffer(1, zerobyte)));
> >>>>>>>>>>          }
> >>>>>>>>>>          aml_append(method, ifctx);
> >>>>>>>>>> +
> >>>>>>>>>> +        ifctx = aml_if(
> >>>>>>>>>> +            aml_equal(uuid,
> >>>>>>>>>> +                      aml_touuid("376054ED-CC13-4675-901C-4756D7F2D45D")));
> >>>>>>>>>> +        {
> >>>>>>>>>> +            /* standard DSM query function */
> >>>>>>>>>> +            ifctx2 = aml_if(aml_equal(function, zero));
> >>>>>>>>>> +            {
> >>>>>>>>>> +                uint8_t byte_list[1] = { 0x03 };
> >>>>>>>>>> +                aml_append(ifctx2, aml_return(aml_buffer(1, byte_list)));
> >>>>>>>>>> +            }
> >>>>>>>>>> +            aml_append(ifctx, ifctx2);
> >>>>>>>>>> +
> >>>>>>>>>> +            /*
> >>>>>>>>>> +             * TCG Platform Reset Attack Mitigation Specification 1.0 Ch.6
> >>>>>>>>>> +             *
> >>>>>>>>>> +             * Arg 2 (Integer): Function Index = 1
> >>>>>>>>>> +             * Arg 3 (Package): Arguments = Package: Type: Integer
> >>>>>>>>>> +             *                  Operation Value of the Request
> >>>>>>>>>> +             * Returns: Type: Integer
> >>>>>>>>>> +             *          0: Success
> >>>>>>>>>> +             *          1: General Failure
> >>>>>>>>>> +             */
> >>>>>>>>>> +            ifctx2 = aml_if(aml_equal(function, one));
> >>>>>>>>>> +            {
> >>>>>>>>>> +                aml_append(ifctx2,
> >>>>>>>>>> +                           aml_store(aml_derefof(aml_index(arguments, zero)),
> >>>>>>>>>> +                                     op));
> >>>>>>>>>> +                {
> >>>>>>>>>> +                    aml_append(ifctx2, aml_store(op, aml_name("MOVV")));
> >>>>>>>>>> +
> >>>>>>>>>> +                    /* 0: success */
> >>>>>>>>>> +                    aml_append(ifctx2, aml_return(zero));
> >>>>>>>>>> +                }
> >>>>>>>>>> +            }
> >>>>>>>>>> +            aml_append(ifctx, ifctx2);
> >>>>>>>>>> +        }
> >>>>>>>>>> +        aml_append(method, ifctx);
> >>>>>>>>>>      }
> >>>>>>>>>> +
> >>>>>>>>>>      aml_append(dev, method);
> >>>>>>>>>>  }
> >>>>>>>>>>
> >>>>>>>>>> diff --git a/hw/tpm/tpm_crb.c b/hw/tpm/tpm_crb.c
> >>>>>>>>>> index b243222fd6..48f6a716ad 100644
> >>>>>>>>>> --- a/hw/tpm/tpm_crb.c
> >>>>>>>>>> +++ b/hw/tpm/tpm_crb.c
> >>>>>>>>>> @@ -233,6 +233,7 @@ static void tpm_crb_reset(void *dev)
> >>>>>>>>>>  {
> >>>>>>>>>>      CRBState *s = CRB(dev);
> >>>>>>>>>>
> >>>>>>>>>> +    tpm_ppi_reset(&s->ppi);
> >>>>>>>>>>      tpm_backend_reset(s->tpmbe);
> >>>>>>>>>>
> >>>>>>>>>>      memset(s->regs, 0, sizeof(s->regs));
> >>>>>>>>>> diff --git a/hw/tpm/tpm_ppi.c b/hw/tpm/tpm_ppi.c
> >>>>>>>>>> index 8b46b9dd4b..ce43bc5729 100644
> >>>>>>>>>> --- a/hw/tpm/tpm_ppi.c
> >>>>>>>>>> +++ b/hw/tpm/tpm_ppi.c
> >>>>>>>>>> @@ -16,8 +16,30 @@
> >>>>>>>>>>  #include "qapi/error.h"
> >>>>>>>>>>  #include "cpu.h"
> >>>>>>>>>>  #include "sysemu/memory_mapping.h"
> >>>>>>>>>> +#include "sysemu/reset.h"
> >>>>>>>>>>  #include "migration/vmstate.h"
> >>>>>>>>>>  #include "tpm_ppi.h"
> >>>>>>>>>> +#include "trace.h"
> >>>>>>>>>> +
> >>>>>>>>>> +void tpm_ppi_reset(TPMPPI *tpmppi)
> >>>>>>>>>> +{
> >>>>>>>>>
> >>>>>>>>>
> >>>>>>>>>> +    char *ptr = memory_region_get_ram_ptr(&tpmppi->ram);
> >>>>>>>>> nvdimm seems to use cpu_physical_memory_read() to access guest
> >>>>>>>>> accessible memory, so question is what's difference?
> >>>>>>>>
> >>>>>>>> cpu_physical_memory_read() is higher level, doing dispatch on address
> >>>>>>>> and length checks.
> >>>>>>>>
> >>>>>>>> This is a bit unnecessary, as ppi->buf could be accessed directly.
> >>>>>>> [...]
> >>>>>>>>>> +            memset(block->host_addr, 0,
> >>>>>>>>>> +                   block->target_end - block->target_start);
> >>>>>>>>>> +        }
> >>>>>>> my concern here is that if we directly touch guest memory here
> >>>>>>> we might get in trouble on migration without dirtying modified
> >>>>>>> ranges
> >>>>>>
> >>>>>> It is a read-only of one byte.
> >>>>>> by the time the reset handler is called, the memory must have been
> >>>>>> already migrated.
> >>>>>
> >>>>> Looks like a write to me?
> >>>>
> >>>> the PPI RAM memory is read for the "memory clear" byte
> >>>> The whole guest RAM is reset to 0 if set.
> >>>
> >>> Oh, I see; hmm.
> >>> How do you avoid zeroing things like persistent memory? Or ROMs? Or EFI
> >>> pflash?
> >>
> >> guest_phys_blocks_append() only cares about RAM (see
> >> guest_phys_blocks_region_add)
> > 
> > Hmm, promising; it uses:
> >     if (!memory_region_is_ram(section->mr) ||
> >         memory_region_is_ram_device(section->mr)) {
> >         return;
> >     }
> > 
> > so ram_device is used by vfio and vhost-user; I don't see anything else.
> > pflash init's as a rom_device so that's probably OK.
> > But things like backends/hostmem-file.c just use
> > memory_region_init_ram_from_file even if they're shared or PMEM.
> > So, I think this would wipe an attached PMEM device - do you want to or
> > not?
> 
> I think that question could be put, "does the reset attack mitigation
> spec recommend / require clearing persistent memory"? I've got no idea.
> The reset attack is that the platform is re-set (forcibly, uncleanly)
> while the original OS holds some secrets in memory, then the attacker's
> OS (or firmware application) is loaded, and those scavenge the
> leftovers. Would the original OS keep secrets in PMEM? I don't know.

No, I don't know either; and I think part of the answer might depend
what PMEM is being used for; if it's being used as actual storage with
a filesystem or database on, you probably don't want to clear it - I
mean that's what the (P)ersistence is for.

> I guess all address space that a guest OS could consider as "read-write
> memory" should be wiped. I think guest_phys_blocks_append() is a good
> choice here; originally I wrote that for supporting guest RAM dump; see
> commit c5d7f60f06142. Note that the is_ram_device bit was added
> separately, see commits e4dc3f5909ab9 and 21e00fa55f3fd -- but that's a
> change in the "right" direction here, because it *restricts* what
> regions qualify (for dumping, and here for clearing).

Yem, I'm wondering if we should add a check for the pmem flag at the
same place.

Having said that; I don't think that's a question for this patch series;
if we agree that guest_phys_blocks* is the right thing to use then it's
a separate question about adding the pmem check to there.
(I didn't know about guest_phys_block* and would have probably just used
qemu_ram_foreach_block )

Dave


> > 
> >>>
> >>>>> Also, don't forget that a guest reset can happen during a migration.
> >>>>
> >>>> Hmm, does cpu_physical_memory_read()  guarantee the memory has been migrated?
> >>>> Is there a way to wait for migration to be completed in a reset handler?
> >>>
> >>> No; remember that migration can take a significant amount of time (many
> >>> minutes) as you stuff many GB of RAM down a network.
> >>>
> >>> So you can be in the situation where:
> >>>      a) Migration starts
> >>>      b) Migration sends a copy of most of RAM across
> >>>      c) Guest dirties lots of RAM in parallel with b
> >>>      d) migration sends some of the RAM again
> >>>      e) guest reboots
> >>>      f) migration keeps sending ram across
> >>>      g) Migration finally completes and starts on destination
> >>>
> >>> a-f are all happening on the source side as the guest is still running
> >>> and doing whatever it wants (including reboots).
> >>>
> >>> Given something like acpi-build.c's acpi_ram_update's call to
> >>> memory_region_set_dirty, would that work for you?
> >>
> >> after the memset(), it should then call:
> >>
> >> memory_region_set_dirty(block->mr, 0, block->target_end - block->target_start);
> >>
> >> looks about right?
> > 
> > I think so.
> 
> I'll admit I can't follow the dirtying discussion. But, I'm reminded of
> another commit of Dave's, namely 90c647db8d59 ("Fix pflash migration",
> 2016-04-15). Would the same trick apply here?
> 
> (
> 
> BTW, I'm sorry about not having following this series closely -- I feel
> bad that we can't solve this within the firmware, but we really can't.
> The issue is that this memory clearing would have to occur really early
> into the firmware, at the latest in the PEI phase.
> 
> However, both the 32-bit and the 64-bit variants of OVMF's PEI phase
> have access only to the lowest 4GB of the memory address space. Mapping
> all RAM (even with a "sliding bank") for clearing it would be a real
> mess. To that, add the fact that OVMF's PEI phase executes from RAM (not
> from pflash -- in OVMF, only SEC executes from pflash, and it
> decompresses the PEI + DXE firmware volumes to RAM), so PEI would have
> to clear all RAM *except* the areas its own self occupies, such as code,
> global variables (static data), heap, stack, other processor data
> structures (IDT, GDT, page tables, ...). And, no gaps inside those
> should be left out either, because the previous OS might have left
> secrets there...
> 
> This is actually easier for firmware that runs on physical hardware; for
> two reasons. First, on physical hardware, the PEI phase of the firmware
> runs from pflash (it might use CAR, Cache-As-RAM, for a small temporary
> r/w storage), so it doesn't have to worry about scribbling over itself.
> Second, on physical hardware, the memory controller too has to be booted
> up -- the PEI code that does this is all vendors' most closely guarded
> secret, and *never* open source --; and when the firmware massages
> chipset registers for that, it can use non-architected means to get the
> RAM to clear itself.
> 
> In comparison, in QEMU/KVM guests, the RAM just *is*. While that's a
> huge relief most of the time, in this case, the fact that the RAM can
> start out as nonzero, is a big problem. Hence my plea to implement the
> feature in QEMU.
> 
> )
> 
> Thanks,
> Laszlo
--
Dr. David Alan Gilbert / dgilbert@redhat.com / Manchester, UK

Re: [Qemu-devel] [PATCH v10 6/6] tpm: add ACPI memory clear interface

[Marc-André Lureau]

Hi

On Mon, Sep 10, 2018 at 2:44 PM Dr. David Alan Gilbert
<dgilbert@redhat.com> wrote:
> (I didn't know about guest_phys_block* and would have probably just used
> qemu_ram_foreach_block )
>

guest_phys_block*() seems to fit, as it lists only the blocks actually
used, and already skip the device RAM.

Laszlo, you wrote the functions
(https://git.qemu.org/?p=qemu.git;a=commit;h=c5d7f60f0614250bd925071e25220ce5958f75d0),
do you think it's appropriate to list the memory to clear, or we
should rather use qemu_ram_foreach_block() ?

thanks

-- 
Marc-André Lureau

Re: [Qemu-devel] [PATCH v10 6/6] tpm: add ACPI memory clear interface

[Laszlo Ersek]

+Alex, due to mention of 21e00fa55f3fd

On 09/10/18 15:03, Marc-André Lureau wrote:
> Hi
> 
> On Mon, Sep 10, 2018 at 2:44 PM Dr. David Alan Gilbert
> <dgilbert@redhat.com> wrote:
>> (I didn't know about guest_phys_block* and would have probably just used
>> qemu_ram_foreach_block )
>>
> 
> guest_phys_block*() seems to fit, as it lists only the blocks actually
> used, and already skip the device RAM.
> 
> Laszlo, you wrote the functions
> (https://git.qemu.org/?p=qemu.git;a=commit;h=c5d7f60f0614250bd925071e25220ce5958f75d0),
> do you think it's appropriate to list the memory to clear, or we
> should rather use qemu_ram_foreach_block() ?

Originally, I would have said, "use either, doesn't matter". Namely,
when I introduced the guest_phys_block*() functions, the original
purpose was not related to RAM *contents*, but to RAM *addresses*
(GPAs). This is evident if you look at the direct child commit of
c5d7f60f0614, namely 56c4bfb3f07f, which put GuestPhysBlockList to use.
And, for your use case (= wiping RAM), GPAs don't matter, only contents
matter.

However, with the commits I mentioned previously, namely e4dc3f5909ab9
and 21e00fa55f3fd, we now filter out some RAM blocks from the dumping
based on contents / backing as well. I think? So I believe we should
honor that for the wiping to. I guess I'd (vaguely) suggest using
guest_phys_block*().

(And then, as Dave suggests, maybe extend the filter to consider pmem
too, separately.)

Laszlo