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.

v13:
- removed needless error handling in tpm_ppi_init()
- splitted "add ACPI memory clear interface"
- moved acpi build function in dedicated hw/acpi/tpm.c
- added some function documentation in headers
- various code cleanups suggested by Philippe
- rebased

Marc-André Lureau (3):
  tpm: add a "ppi" boolean property
  acpi: add ACPI memory clear interface
  tpm: clear RAM when "memory overwrite" requested

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      |  47 +++++
 include/hw/acpi/tpm.h |  23 +++
 include/hw/compat.h   |  11 +-
 hw/acpi/tpm.c         | 447 ++++++++++++++++++++++++++++++++++++++++++
 hw/i386/acpi-build.c  |  29 ++-
 hw/tpm/tpm_crb.c      |  11 ++
 hw/tpm/tpm_ppi.c      |  53 +++++
 hw/tpm/tpm_tis.c      |  11 ++
 docs/specs/tpm.txt    | 104 ++++++++++
 hw/acpi/Makefile.objs |   1 +
 hw/tpm/Makefile.objs  |   1 +
 hw/tpm/trace-events   |   3 +
 12 files changed, 738 insertions(+), 3 deletions(-)
 create mode 100644 hw/tpm/tpm_ppi.h
 create mode 100644 hw/acpi/tpm.c
 create mode 100644 hw/tpm/tpm_ppi.c

-- 
2.20.0

On Thu, Dec 13, 2018 at 02:26:42AM +0400, Marc-André Lureau wrote:
> Hi,
> 
> The following patches implement the TPM Physical Presence Interface
> that allows a user to set a command via ACPI (sysfs entry in Linux)
> that, upon the next reboot, the firmware looks for and acts upon by
> sending sequences of commands to the TPM.
> 
> A dedicated memory region is added to the TPM CRB & TIS devices, at
> address/size 0xFED45000/0x400. A new "etc/tpm/config" fw_cfg entry
> holds the location for that PPI region and some version details, to
> allow for future flexibility.
> 
> With the associated edk2/ovmf firmware, the Windows HLK "PPI 1.3" test
> now runs successfully.
> 
> It is based on previous work from Stefan Berger ("[PATCH v2 0/4]
> Implement Physical Presence interface for TPM 1.2 and 2")
> 
> The edk2 support is merged upstream.


Breaks build with tpm disabled:

/usr/bin/ld: hw/i386/acpi-build.o: in function `build_dsdt':
/scm/qemu/hw/i386/acpi-build.c:2158: undefined reference to `tpm_build_ppi_acpi'
/usr/bin/ld: /scm/qemu/hw/i386/acpi-build.c:2179: undefined reference to `tpm_build_ppi_acpi'
collect2: error: ld returned 1 exit status


> v13:
> - removed needless error handling in tpm_ppi_init()
> - splitted "add ACPI memory clear interface"
> - moved acpi build function in dedicated hw/acpi/tpm.c
> - added some function documentation in headers
> - various code cleanups suggested by Philippe
> - rebased
> 
> Marc-André Lureau (3):
>   tpm: add a "ppi" boolean property
>   acpi: add ACPI memory clear interface
>   tpm: clear RAM when "memory overwrite" requested
> 
> 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      |  47 +++++
>  include/hw/acpi/tpm.h |  23 +++
>  include/hw/compat.h   |  11 +-
>  hw/acpi/tpm.c         | 447 ++++++++++++++++++++++++++++++++++++++++++
>  hw/i386/acpi-build.c  |  29 ++-
>  hw/tpm/tpm_crb.c      |  11 ++
>  hw/tpm/tpm_ppi.c      |  53 +++++
>  hw/tpm/tpm_tis.c      |  11 ++
>  docs/specs/tpm.txt    | 104 ++++++++++
>  hw/acpi/Makefile.objs |   1 +
>  hw/tpm/Makefile.objs  |   1 +
>  hw/tpm/trace-events   |   3 +
>  12 files changed, 738 insertions(+), 3 deletions(-)
>  create mode 100644 hw/tpm/tpm_ppi.h
>  create mode 100644 hw/acpi/tpm.c
>  create mode 100644 hw/tpm/tpm_ppi.c
> 
> -- 
> 2.20.0

Hi Michael,

On Tue, Dec 18, 2018 at 5:42 AM Michael S. Tsirkin <mst@redhat.com> wrote:
>
> On Thu, Dec 13, 2018 at 02:26:42AM +0400, Marc-André Lureau wrote:
> > Hi,
> >
> > The following patches implement the TPM Physical Presence Interface
> > that allows a user to set a command via ACPI (sysfs entry in Linux)
> > that, upon the next reboot, the firmware looks for and acts upon by
> > sending sequences of commands to the TPM.
> >
> > A dedicated memory region is added to the TPM CRB & TIS devices, at
> > address/size 0xFED45000/0x400. A new "etc/tpm/config" fw_cfg entry
> > holds the location for that PPI region and some version details, to
> > allow for future flexibility.
> >
> > With the associated edk2/ovmf firmware, the Windows HLK "PPI 1.3" test
> > now runs successfully.
> >
> > It is based on previous work from Stefan Berger ("[PATCH v2 0/4]
> > Implement Physical Presence interface for TPM 1.2 and 2")
> >
> > The edk2 support is merged upstream.
>
>
> Breaks build with tpm disabled:
>
> /usr/bin/ld: hw/i386/acpi-build.o: in function `build_dsdt':
> /scm/qemu/hw/i386/acpi-build.c:2158: undefined reference to `tpm_build_ppi_acpi'
> /usr/bin/ld: /scm/qemu/hw/i386/acpi-build.c:2179: undefined reference to `tpm_build_ppi_acpi'
> collect2: error: ld returned 1 exit status

Thanks for noticing, this is probably a regression from v13, since the
tpm_build_ppi_acpi was moved to a separate unit that is not always
built.

There is a lot of TPM code that we always build in acpi-build. Either
the code should have #ifdef TPM, or we use stubs.

Fwiw, the preliminary #ifdef patch would look like that, is that
desirable, or should we just add some stub:

diff --git a/hw/i386/acpi-build.c b/hw/i386/acpi-build.c
index 236a20eaa8..e6ca350067 100644
--- a/hw/i386/acpi-build.c
+++ b/hw/i386/acpi-build.c
@@ -41,14 +41,16 @@
 #include "hw/isa/isa.h"
 #include "hw/block/fdc.h"
 #include "hw/acpi/memory_hotplug.h"
+#ifdef CONFIG_TPM
 #include "sysemu/tpm.h"
 #include "hw/acpi/tpm.h"
-#include "hw/acpi/vmgenid.h"
 #include "sysemu/tpm_backend.h"
+#endif
+#include "hw/acpi/vmgenid.h"
 #include "hw/timer/mc146818rtc_regs.h"
 #include "hw/mem/memory-device.h"
 #include "sysemu/numa.h"
-
+#include "qemu/units.h"
 /* Supported chipsets: */
 #include "hw/acpi/piix4.h"
 #include "hw/acpi/pcihp.h"
@@ -105,7 +107,9 @@ typedef struct AcpiPmInfo {
 typedef struct AcpiMiscInfo {
     bool is_piix4;
     bool has_hpet;
+#ifdef CONFIG_TPM
     TPMVersion tpm_version;
+#endif
     const unsigned char *dsdt_code;
     unsigned dsdt_size;
     uint16_t pvpanic_port;
@@ -234,7 +238,9 @@ static void acpi_get_misc_info(AcpiMiscInfo *info)
     }

     info->has_hpet = hpet_find();
+#ifdef CONFIG_TPM
     info->tpm_version = tpm_get_version(tpm_find());
+#endif
     info->pvpanic_port = pvpanic_port();
     info->applesmc_io_base = applesmc_port();
 }
@@ -1962,10 +1968,12 @@ build_dsdt(GArray *table_data, BIOSLinker *linker,
         }
     }

+#ifdef CONFIG_TPM
     if (TPM_IS_TIS(tpm_find())) {
         aml_append(crs, aml_memory32_fixed(TPM_TIS_ADDR_BASE,
                    TPM_TIS_ADDR_SIZE, AML_READ_WRITE));
     }
+#endif
     aml_append(scope, aml_name_decl("_CRS", crs));

     /* reserve GPE0 block resources */
@@ -2133,6 +2141,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);

+#ifdef CONFIG_TPM
             if (TPM_IS_TIS(tpm_find())) {
                 dev = aml_device("ISA.TPM");
                 aml_append(dev, aml_name_decl("_HID", aml_eisaid("PNP0C31")));
@@ -2149,11 +2158,12 @@ build_dsdt(GArray *table_data, BIOSLinker *linker,
                 aml_append(dev, aml_name_decl("_CRS", crs));
                 aml_append(scope, dev);
             }
-
+#endif
             aml_append(sb_scope, scope);
         }
     }

+#ifdef CONFIG_TPM
     if (TPM_IS_CRB(tpm_find())) {
         dev = aml_device("TPM");
         aml_append(dev, aml_name_decl("_HID", aml_string("MSFT0101")));
@@ -2168,7 +2178,7 @@ build_dsdt(GArray *table_data, BIOSLinker *linker,

         aml_append(sb_scope, dev);
     }
-
+#endif
     aml_append(dsdt, sb_scope);

     /* copy AML table into ACPI tables blob and patch header there */
@@ -2194,6 +2204,7 @@ build_hpet(GArray *table_data, BIOSLinker *linker)
                  (void *)hpet, "HPET", sizeof(*hpet), 1, NULL, NULL);
 }

+#ifdef CONFIG_TPM
 static void
 build_tpm_tcpa(GArray *table_data, BIOSLinker *linker, GArray *tcpalog)
 {
@@ -2247,6 +2258,7 @@ build_tpm2(GArray *table_data, BIOSLinker
*linker, GArray *tcpalog)
     build_header(linker, table_data,
                  (void *)tpm2_ptr, "TPM2", sizeof(*tpm2_ptr), 4, NULL, NULL);
 }
+#endif

 #define HOLE_640K_START  (640 * KiB)
 #define HOLE_640K_END   (1 * MiB)
@@ -2679,6 +2691,7 @@ void acpi_build(AcpiBuildTables *tables,
MachineState *machine)
         acpi_add_table(table_offsets, tables_blob);
         build_hpet(tables_blob, tables->linker);
     }
+#ifdef CONFIG_TPM
     if (misc.tpm_version != TPM_VERSION_UNSPEC) {
         acpi_add_table(table_offsets, tables_blob);
         build_tpm_tcpa(tables_blob, tables->linker, tables->tcpalog);
@@ -2688,6 +2701,7 @@ void acpi_build(AcpiBuildTables *tables,
MachineState *machine)
             build_tpm2(tables_blob, tables->linker, tables->tcpalog);
         }
     }
+#endif
     if (pcms->numa_nodes) {
         acpi_add_table(table_offsets, tables_blob);
         build_srat(tables_blob, tables->linker, machine);
@@ -2886,9 +2900,10 @@ void acpi_setup(void)
         acpi_add_rom_blob(build_state, tables.linker->cmd_blob,
                           "etc/table-loader", 0);

+#ifdef CONFIG_TPM
     fw_cfg_add_file(pcms->fw_cfg, ACPI_BUILD_TPMLOG_FILE,
                     tables.tcpalog->data, acpi_data_len(tables.tcpalog));
-
+#endif
     vmgenid_dev = find_vmgenid_dev();
     if (vmgenid_dev) {
         vmgenid_add_fw_cfg(VMGENID(vmgenid_dev), pcms->fw_cfg,


>
> > v13:
> > - removed needless error handling in tpm_ppi_init()
> > - splitted "add ACPI memory clear interface"
> > - moved acpi build function in dedicated hw/acpi/tpm.c
> > - added some function documentation in headers
> > - various code cleanups suggested by Philippe
> > - rebased
> >
> > Marc-André Lureau (3):
> >   tpm: add a "ppi" boolean property
> >   acpi: add ACPI memory clear interface
> >   tpm: clear RAM when "memory overwrite" requested
> >
> > 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      |  47 +++++
> >  include/hw/acpi/tpm.h |  23 +++
> >  include/hw/compat.h   |  11 +-
> >  hw/acpi/tpm.c         | 447 ++++++++++++++++++++++++++++++++++++++++++
> >  hw/i386/acpi-build.c  |  29 ++-
> >  hw/tpm/tpm_crb.c      |  11 ++
> >  hw/tpm/tpm_ppi.c      |  53 +++++
> >  hw/tpm/tpm_tis.c      |  11 ++
> >  docs/specs/tpm.txt    | 104 ++++++++++
> >  hw/acpi/Makefile.objs |   1 +
> >  hw/tpm/Makefile.objs  |   1 +
> >  hw/tpm/trace-events   |   3 +
> >  12 files changed, 738 insertions(+), 3 deletions(-)
> >  create mode 100644 hw/tpm/tpm_ppi.h
> >  create mode 100644 hw/acpi/tpm.c
> >  create mode 100644 hw/tpm/tpm_ppi.c
> >
> > --
> > 2.20.0
>


-- 
Marc-André Lureau

On Tue, Dec 18, 2018 at 01:28:53PM +0400, Marc-André Lureau wrote:
> Hi Michael,
> 
> On Tue, Dec 18, 2018 at 5:42 AM Michael S. Tsirkin <mst@redhat.com> wrote:
> >
> > On Thu, Dec 13, 2018 at 02:26:42AM +0400, Marc-André Lureau wrote:
> > > Hi,
> > >
> > > The following patches implement the TPM Physical Presence Interface
> > > that allows a user to set a command via ACPI (sysfs entry in Linux)
> > > that, upon the next reboot, the firmware looks for and acts upon by
> > > sending sequences of commands to the TPM.
> > >
> > > A dedicated memory region is added to the TPM CRB & TIS devices, at
> > > address/size 0xFED45000/0x400. A new "etc/tpm/config" fw_cfg entry
> > > holds the location for that PPI region and some version details, to
> > > allow for future flexibility.
> > >
> > > With the associated edk2/ovmf firmware, the Windows HLK "PPI 1.3" test
> > > now runs successfully.
> > >
> > > It is based on previous work from Stefan Berger ("[PATCH v2 0/4]
> > > Implement Physical Presence interface for TPM 1.2 and 2")
> > >
> > > The edk2 support is merged upstream.
> >
> >
> > Breaks build with tpm disabled:
> >
> > /usr/bin/ld: hw/i386/acpi-build.o: in function `build_dsdt':
> > /scm/qemu/hw/i386/acpi-build.c:2158: undefined reference to `tpm_build_ppi_acpi'
> > /usr/bin/ld: /scm/qemu/hw/i386/acpi-build.c:2179: undefined reference to `tpm_build_ppi_acpi'
> > collect2: error: ld returned 1 exit status
> 
> Thanks for noticing, this is probably a regression from v13, since the
> tpm_build_ppi_acpi was moved to a separate unit that is not always
> built.
> 
> There is a lot of TPM code that we always build in acpi-build. Either
> the code should have #ifdef TPM, or we use stubs.
> 
> Fwiw, the preliminary #ifdef patch would look like that, is that
> desirable, or should we just add some stub:

Seems way too messy. I'd prefer stubs.

> diff --git a/hw/i386/acpi-build.c b/hw/i386/acpi-build.c
> index 236a20eaa8..e6ca350067 100644
> --- a/hw/i386/acpi-build.c
> +++ b/hw/i386/acpi-build.c
> @@ -41,14 +41,16 @@
>  #include "hw/isa/isa.h"
>  #include "hw/block/fdc.h"
>  #include "hw/acpi/memory_hotplug.h"
> +#ifdef CONFIG_TPM
>  #include "sysemu/tpm.h"
>  #include "hw/acpi/tpm.h"
> -#include "hw/acpi/vmgenid.h"
>  #include "sysemu/tpm_backend.h"
> +#endif
> +#include "hw/acpi/vmgenid.h"
>  #include "hw/timer/mc146818rtc_regs.h"
>  #include "hw/mem/memory-device.h"
>  #include "sysemu/numa.h"
> -
> +#include "qemu/units.h"
>  /* Supported chipsets: */
>  #include "hw/acpi/piix4.h"
>  #include "hw/acpi/pcihp.h"
> @@ -105,7 +107,9 @@ typedef struct AcpiPmInfo {
>  typedef struct AcpiMiscInfo {
>      bool is_piix4;
>      bool has_hpet;
> +#ifdef CONFIG_TPM
>      TPMVersion tpm_version;
> +#endif
>      const unsigned char *dsdt_code;
>      unsigned dsdt_size;
>      uint16_t pvpanic_port;
> @@ -234,7 +238,9 @@ static void acpi_get_misc_info(AcpiMiscInfo *info)
>      }
> 
>      info->has_hpet = hpet_find();
> +#ifdef CONFIG_TPM
>      info->tpm_version = tpm_get_version(tpm_find());
> +#endif
>      info->pvpanic_port = pvpanic_port();
>      info->applesmc_io_base = applesmc_port();
>  }
> @@ -1962,10 +1968,12 @@ build_dsdt(GArray *table_data, BIOSLinker *linker,
>          }
>      }
> 
> +#ifdef CONFIG_TPM
>      if (TPM_IS_TIS(tpm_find())) {
>          aml_append(crs, aml_memory32_fixed(TPM_TIS_ADDR_BASE,
>                     TPM_TIS_ADDR_SIZE, AML_READ_WRITE));
>      }
> +#endif
>      aml_append(scope, aml_name_decl("_CRS", crs));
> 
>      /* reserve GPE0 block resources */
> @@ -2133,6 +2141,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);
> 
> +#ifdef CONFIG_TPM
>              if (TPM_IS_TIS(tpm_find())) {
>                  dev = aml_device("ISA.TPM");
>                  aml_append(dev, aml_name_decl("_HID", aml_eisaid("PNP0C31")));
> @@ -2149,11 +2158,12 @@ build_dsdt(GArray *table_data, BIOSLinker *linker,
>                  aml_append(dev, aml_name_decl("_CRS", crs));
>                  aml_append(scope, dev);
>              }
> -
> +#endif
>              aml_append(sb_scope, scope);
>          }
>      }
> 
> +#ifdef CONFIG_TPM
>      if (TPM_IS_CRB(tpm_find())) {
>          dev = aml_device("TPM");
>          aml_append(dev, aml_name_decl("_HID", aml_string("MSFT0101")));
> @@ -2168,7 +2178,7 @@ build_dsdt(GArray *table_data, BIOSLinker *linker,
> 
>          aml_append(sb_scope, dev);
>      }
> -
> +#endif
>      aml_append(dsdt, sb_scope);
> 
>      /* copy AML table into ACPI tables blob and patch header there */
> @@ -2194,6 +2204,7 @@ build_hpet(GArray *table_data, BIOSLinker *linker)
>                   (void *)hpet, "HPET", sizeof(*hpet), 1, NULL, NULL);
>  }
> 
> +#ifdef CONFIG_TPM
>  static void
>  build_tpm_tcpa(GArray *table_data, BIOSLinker *linker, GArray *tcpalog)
>  {
> @@ -2247,6 +2258,7 @@ build_tpm2(GArray *table_data, BIOSLinker
> *linker, GArray *tcpalog)
>      build_header(linker, table_data,
>                   (void *)tpm2_ptr, "TPM2", sizeof(*tpm2_ptr), 4, NULL, NULL);
>  }
> +#endif
> 
>  #define HOLE_640K_START  (640 * KiB)
>  #define HOLE_640K_END   (1 * MiB)
> @@ -2679,6 +2691,7 @@ void acpi_build(AcpiBuildTables *tables,
> MachineState *machine)
>          acpi_add_table(table_offsets, tables_blob);
>          build_hpet(tables_blob, tables->linker);
>      }
> +#ifdef CONFIG_TPM
>      if (misc.tpm_version != TPM_VERSION_UNSPEC) {
>          acpi_add_table(table_offsets, tables_blob);
>          build_tpm_tcpa(tables_blob, tables->linker, tables->tcpalog);
> @@ -2688,6 +2701,7 @@ void acpi_build(AcpiBuildTables *tables,
> MachineState *machine)
>              build_tpm2(tables_blob, tables->linker, tables->tcpalog);
>          }
>      }
> +#endif
>      if (pcms->numa_nodes) {
>          acpi_add_table(table_offsets, tables_blob);
>          build_srat(tables_blob, tables->linker, machine);
> @@ -2886,9 +2900,10 @@ void acpi_setup(void)
>          acpi_add_rom_blob(build_state, tables.linker->cmd_blob,
>                            "etc/table-loader", 0);
> 
> +#ifdef CONFIG_TPM
>      fw_cfg_add_file(pcms->fw_cfg, ACPI_BUILD_TPMLOG_FILE,
>                      tables.tcpalog->data, acpi_data_len(tables.tcpalog));
> -
> +#endif
>      vmgenid_dev = find_vmgenid_dev();
>      if (vmgenid_dev) {
>          vmgenid_add_fw_cfg(VMGENID(vmgenid_dev), pcms->fw_cfg,
> 
> 
> >
> > > v13:
> > > - removed needless error handling in tpm_ppi_init()
> > > - splitted "add ACPI memory clear interface"
> > > - moved acpi build function in dedicated hw/acpi/tpm.c
> > > - added some function documentation in headers
> > > - various code cleanups suggested by Philippe
> > > - rebased
> > >
> > > Marc-André Lureau (3):
> > >   tpm: add a "ppi" boolean property
> > >   acpi: add ACPI memory clear interface
> > >   tpm: clear RAM when "memory overwrite" requested
> > >
> > > 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      |  47 +++++
> > >  include/hw/acpi/tpm.h |  23 +++
> > >  include/hw/compat.h   |  11 +-
> > >  hw/acpi/tpm.c         | 447 ++++++++++++++++++++++++++++++++++++++++++
> > >  hw/i386/acpi-build.c  |  29 ++-
> > >  hw/tpm/tpm_crb.c      |  11 ++
> > >  hw/tpm/tpm_ppi.c      |  53 +++++
> > >  hw/tpm/tpm_tis.c      |  11 ++
> > >  docs/specs/tpm.txt    | 104 ++++++++++
> > >  hw/acpi/Makefile.objs |   1 +
> > >  hw/tpm/Makefile.objs  |   1 +
> > >  hw/tpm/trace-events   |   3 +
> > >  12 files changed, 738 insertions(+), 3 deletions(-)
> > >  create mode 100644 hw/tpm/tpm_ppi.h
> > >  create mode 100644 hw/acpi/tpm.c
> > >  create mode 100644 hw/tpm/tpm_ppi.c
> > >
> > > --
> > > 2.20.0
> >
> 
> 
> -- 
> Marc-André Lureau

Hi

On Tue, Dec 18, 2018 at 6:20 PM Michael S. Tsirkin <mst@redhat.com> wrote:
>
> On Tue, Dec 18, 2018 at 01:28:53PM +0400, Marc-André Lureau wrote:
> > Hi Michael,
> >
> > On Tue, Dec 18, 2018 at 5:42 AM Michael S. Tsirkin <mst@redhat.com> wrote:
> > >
> > > On Thu, Dec 13, 2018 at 02:26:42AM +0400, Marc-André Lureau wrote:
> > > > Hi,
> > > >
> > > > The following patches implement the TPM Physical Presence Interface
> > > > that allows a user to set a command via ACPI (sysfs entry in Linux)
> > > > that, upon the next reboot, the firmware looks for and acts upon by
> > > > sending sequences of commands to the TPM.
> > > >
> > > > A dedicated memory region is added to the TPM CRB & TIS devices, at
> > > > address/size 0xFED45000/0x400. A new "etc/tpm/config" fw_cfg entry
> > > > holds the location for that PPI region and some version details, to
> > > > allow for future flexibility.
> > > >
> > > > With the associated edk2/ovmf firmware, the Windows HLK "PPI 1.3" test
> > > > now runs successfully.
> > > >
> > > > It is based on previous work from Stefan Berger ("[PATCH v2 0/4]
> > > > Implement Physical Presence interface for TPM 1.2 and 2")
> > > >
> > > > The edk2 support is merged upstream.
> > >
> > >
> > > Breaks build with tpm disabled:
> > >
> > > /usr/bin/ld: hw/i386/acpi-build.o: in function `build_dsdt':
> > > /scm/qemu/hw/i386/acpi-build.c:2158: undefined reference to `tpm_build_ppi_acpi'
> > > /usr/bin/ld: /scm/qemu/hw/i386/acpi-build.c:2179: undefined reference to `tpm_build_ppi_acpi'
> > > collect2: error: ld returned 1 exit status
> >
> > Thanks for noticing, this is probably a regression from v13, since the
> > tpm_build_ppi_acpi was moved to a separate unit that is not always
> > built.
> >
> > There is a lot of TPM code that we always build in acpi-build. Either
> > the code should have #ifdef TPM, or we use stubs.
> >
> > Fwiw, the preliminary #ifdef patch would look like that, is that
> > desirable, or should we just add some stub:
>
> Seems way too messy. I'd prefer stubs.

Beside the missing stub, and an indentation fix, could someone review
the last 2 patches?

>
> > diff --git a/hw/i386/acpi-build.c b/hw/i386/acpi-build.c
> > index 236a20eaa8..e6ca350067 100644
> > --- a/hw/i386/acpi-build.c
> > +++ b/hw/i386/acpi-build.c
> > @@ -41,14 +41,16 @@
> >  #include "hw/isa/isa.h"
> >  #include "hw/block/fdc.h"
> >  #include "hw/acpi/memory_hotplug.h"
> > +#ifdef CONFIG_TPM
> >  #include "sysemu/tpm.h"
> >  #include "hw/acpi/tpm.h"
> > -#include "hw/acpi/vmgenid.h"
> >  #include "sysemu/tpm_backend.h"
> > +#endif
> > +#include "hw/acpi/vmgenid.h"
> >  #include "hw/timer/mc146818rtc_regs.h"
> >  #include "hw/mem/memory-device.h"
> >  #include "sysemu/numa.h"
> > -
> > +#include "qemu/units.h"
> >  /* Supported chipsets: */
> >  #include "hw/acpi/piix4.h"
> >  #include "hw/acpi/pcihp.h"
> > @@ -105,7 +107,9 @@ typedef struct AcpiPmInfo {
> >  typedef struct AcpiMiscInfo {
> >      bool is_piix4;
> >      bool has_hpet;
> > +#ifdef CONFIG_TPM
> >      TPMVersion tpm_version;
> > +#endif
> >      const unsigned char *dsdt_code;
> >      unsigned dsdt_size;
> >      uint16_t pvpanic_port;
> > @@ -234,7 +238,9 @@ static void acpi_get_misc_info(AcpiMiscInfo *info)
> >      }
> >
> >      info->has_hpet = hpet_find();
> > +#ifdef CONFIG_TPM
> >      info->tpm_version = tpm_get_version(tpm_find());
> > +#endif
> >      info->pvpanic_port = pvpanic_port();
> >      info->applesmc_io_base = applesmc_port();
> >  }
> > @@ -1962,10 +1968,12 @@ build_dsdt(GArray *table_data, BIOSLinker *linker,
> >          }
> >      }
> >
> > +#ifdef CONFIG_TPM
> >      if (TPM_IS_TIS(tpm_find())) {
> >          aml_append(crs, aml_memory32_fixed(TPM_TIS_ADDR_BASE,
> >                     TPM_TIS_ADDR_SIZE, AML_READ_WRITE));
> >      }
> > +#endif
> >      aml_append(scope, aml_name_decl("_CRS", crs));
> >
> >      /* reserve GPE0 block resources */
> > @@ -2133,6 +2141,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);
> >
> > +#ifdef CONFIG_TPM
> >              if (TPM_IS_TIS(tpm_find())) {
> >                  dev = aml_device("ISA.TPM");
> >                  aml_append(dev, aml_name_decl("_HID", aml_eisaid("PNP0C31")));
> > @@ -2149,11 +2158,12 @@ build_dsdt(GArray *table_data, BIOSLinker *linker,
> >                  aml_append(dev, aml_name_decl("_CRS", crs));
> >                  aml_append(scope, dev);
> >              }
> > -
> > +#endif
> >              aml_append(sb_scope, scope);
> >          }
> >      }
> >
> > +#ifdef CONFIG_TPM
> >      if (TPM_IS_CRB(tpm_find())) {
> >          dev = aml_device("TPM");
> >          aml_append(dev, aml_name_decl("_HID", aml_string("MSFT0101")));
> > @@ -2168,7 +2178,7 @@ build_dsdt(GArray *table_data, BIOSLinker *linker,
> >
> >          aml_append(sb_scope, dev);
> >      }
> > -
> > +#endif
> >      aml_append(dsdt, sb_scope);
> >
> >      /* copy AML table into ACPI tables blob and patch header there */
> > @@ -2194,6 +2204,7 @@ build_hpet(GArray *table_data, BIOSLinker *linker)
> >                   (void *)hpet, "HPET", sizeof(*hpet), 1, NULL, NULL);
> >  }
> >
> > +#ifdef CONFIG_TPM
> >  static void
> >  build_tpm_tcpa(GArray *table_data, BIOSLinker *linker, GArray *tcpalog)
> >  {
> > @@ -2247,6 +2258,7 @@ build_tpm2(GArray *table_data, BIOSLinker
> > *linker, GArray *tcpalog)
> >      build_header(linker, table_data,
> >                   (void *)tpm2_ptr, "TPM2", sizeof(*tpm2_ptr), 4, NULL, NULL);
> >  }
> > +#endif
> >
> >  #define HOLE_640K_START  (640 * KiB)
> >  #define HOLE_640K_END   (1 * MiB)
> > @@ -2679,6 +2691,7 @@ void acpi_build(AcpiBuildTables *tables,
> > MachineState *machine)
> >          acpi_add_table(table_offsets, tables_blob);
> >          build_hpet(tables_blob, tables->linker);
> >      }
> > +#ifdef CONFIG_TPM
> >      if (misc.tpm_version != TPM_VERSION_UNSPEC) {
> >          acpi_add_table(table_offsets, tables_blob);
> >          build_tpm_tcpa(tables_blob, tables->linker, tables->tcpalog);
> > @@ -2688,6 +2701,7 @@ void acpi_build(AcpiBuildTables *tables,
> > MachineState *machine)
> >              build_tpm2(tables_blob, tables->linker, tables->tcpalog);
> >          }
> >      }
> > +#endif
> >      if (pcms->numa_nodes) {
> >          acpi_add_table(table_offsets, tables_blob);
> >          build_srat(tables_blob, tables->linker, machine);
> > @@ -2886,9 +2900,10 @@ void acpi_setup(void)
> >          acpi_add_rom_blob(build_state, tables.linker->cmd_blob,
> >                            "etc/table-loader", 0);
> >
> > +#ifdef CONFIG_TPM
> >      fw_cfg_add_file(pcms->fw_cfg, ACPI_BUILD_TPMLOG_FILE,
> >                      tables.tcpalog->data, acpi_data_len(tables.tcpalog));
> > -
> > +#endif
> >      vmgenid_dev = find_vmgenid_dev();
> >      if (vmgenid_dev) {
> >          vmgenid_add_fw_cfg(VMGENID(vmgenid_dev), pcms->fw_cfg,
> >
> >
> > >
> > > > v13:
> > > > - removed needless error handling in tpm_ppi_init()
> > > > - splitted "add ACPI memory clear interface"
> > > > - moved acpi build function in dedicated hw/acpi/tpm.c
> > > > - added some function documentation in headers
> > > > - various code cleanups suggested by Philippe
> > > > - rebased
> > > >
> > > > Marc-André Lureau (3):
> > > >   tpm: add a "ppi" boolean property
> > > >   acpi: add ACPI memory clear interface
> > > >   tpm: clear RAM when "memory overwrite" requested
> > > >
> > > > 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      |  47 +++++
> > > >  include/hw/acpi/tpm.h |  23 +++
> > > >  include/hw/compat.h   |  11 +-
> > > >  hw/acpi/tpm.c         | 447 ++++++++++++++++++++++++++++++++++++++++++
> > > >  hw/i386/acpi-build.c  |  29 ++-
> > > >  hw/tpm/tpm_crb.c      |  11 ++
> > > >  hw/tpm/tpm_ppi.c      |  53 +++++
> > > >  hw/tpm/tpm_tis.c      |  11 ++
> > > >  docs/specs/tpm.txt    | 104 ++++++++++
> > > >  hw/acpi/Makefile.objs |   1 +
> > > >  hw/tpm/Makefile.objs  |   1 +
> > > >  hw/tpm/trace-events   |   3 +
> > > >  12 files changed, 738 insertions(+), 3 deletions(-)
> > > >  create mode 100644 hw/tpm/tpm_ppi.h
> > > >  create mode 100644 hw/acpi/tpm.c
> > > >  create mode 100644 hw/tpm/tpm_ppi.c
> > > >
> > > > --
> > > > 2.20.0
> > >
> >
> >
> > --
> > Marc-André Lureau



-- 
Marc-André Lureau

On Tue, Dec 18, 2018 at 09:56:35PM +0400, Marc-André Lureau wrote:
> Hi
> 
> On Tue, Dec 18, 2018 at 6:20 PM Michael S. Tsirkin <mst@redhat.com> wrote:
> >
> > On Tue, Dec 18, 2018 at 01:28:53PM +0400, Marc-André Lureau wrote:
> > > Hi Michael,
> > >
> > > On Tue, Dec 18, 2018 at 5:42 AM Michael S. Tsirkin <mst@redhat.com> wrote:
> > > >
> > > > On Thu, Dec 13, 2018 at 02:26:42AM +0400, Marc-André Lureau wrote:
> > > > > Hi,
> > > > >
> > > > > The following patches implement the TPM Physical Presence Interface
> > > > > that allows a user to set a command via ACPI (sysfs entry in Linux)
> > > > > that, upon the next reboot, the firmware looks for and acts upon by
> > > > > sending sequences of commands to the TPM.
> > > > >
> > > > > A dedicated memory region is added to the TPM CRB & TIS devices, at
> > > > > address/size 0xFED45000/0x400. A new "etc/tpm/config" fw_cfg entry
> > > > > holds the location for that PPI region and some version details, to
> > > > > allow for future flexibility.
> > > > >
> > > > > With the associated edk2/ovmf firmware, the Windows HLK "PPI 1.3" test
> > > > > now runs successfully.
> > > > >
> > > > > It is based on previous work from Stefan Berger ("[PATCH v2 0/4]
> > > > > Implement Physical Presence interface for TPM 1.2 and 2")
> > > > >
> > > > > The edk2 support is merged upstream.
> > > >
> > > >
> > > > Breaks build with tpm disabled:
> > > >
> > > > /usr/bin/ld: hw/i386/acpi-build.o: in function `build_dsdt':
> > > > /scm/qemu/hw/i386/acpi-build.c:2158: undefined reference to `tpm_build_ppi_acpi'
> > > > /usr/bin/ld: /scm/qemu/hw/i386/acpi-build.c:2179: undefined reference to `tpm_build_ppi_acpi'
> > > > collect2: error: ld returned 1 exit status
> > >
> > > Thanks for noticing, this is probably a regression from v13, since the
> > > tpm_build_ppi_acpi was moved to a separate unit that is not always
> > > built.
> > >
> > > There is a lot of TPM code that we always build in acpi-build. Either
> > > the code should have #ifdef TPM, or we use stubs.
> > >
> > > Fwiw, the preliminary #ifdef patch would look like that, is that
> > > desirable, or should we just add some stub:
> >
> > Seems way too messy. I'd prefer stubs.
> 
> Beside the missing stub, and an indentation fix, could someone review
> the last 2 patches?

I think they are fine was going to merge.

> >
> > > diff --git a/hw/i386/acpi-build.c b/hw/i386/acpi-build.c
> > > index 236a20eaa8..e6ca350067 100644
> > > --- a/hw/i386/acpi-build.c
> > > +++ b/hw/i386/acpi-build.c
> > > @@ -41,14 +41,16 @@
> > >  #include "hw/isa/isa.h"
> > >  #include "hw/block/fdc.h"
> > >  #include "hw/acpi/memory_hotplug.h"
> > > +#ifdef CONFIG_TPM
> > >  #include "sysemu/tpm.h"
> > >  #include "hw/acpi/tpm.h"
> > > -#include "hw/acpi/vmgenid.h"
> > >  #include "sysemu/tpm_backend.h"
> > > +#endif
> > > +#include "hw/acpi/vmgenid.h"
> > >  #include "hw/timer/mc146818rtc_regs.h"
> > >  #include "hw/mem/memory-device.h"
> > >  #include "sysemu/numa.h"
> > > -
> > > +#include "qemu/units.h"
> > >  /* Supported chipsets: */
> > >  #include "hw/acpi/piix4.h"
> > >  #include "hw/acpi/pcihp.h"
> > > @@ -105,7 +107,9 @@ typedef struct AcpiPmInfo {
> > >  typedef struct AcpiMiscInfo {
> > >      bool is_piix4;
> > >      bool has_hpet;
> > > +#ifdef CONFIG_TPM
> > >      TPMVersion tpm_version;
> > > +#endif
> > >      const unsigned char *dsdt_code;
> > >      unsigned dsdt_size;
> > >      uint16_t pvpanic_port;
> > > @@ -234,7 +238,9 @@ static void acpi_get_misc_info(AcpiMiscInfo *info)
> > >      }
> > >
> > >      info->has_hpet = hpet_find();
> > > +#ifdef CONFIG_TPM
> > >      info->tpm_version = tpm_get_version(tpm_find());
> > > +#endif
> > >      info->pvpanic_port = pvpanic_port();
> > >      info->applesmc_io_base = applesmc_port();
> > >  }
> > > @@ -1962,10 +1968,12 @@ build_dsdt(GArray *table_data, BIOSLinker *linker,
> > >          }
> > >      }
> > >
> > > +#ifdef CONFIG_TPM
> > >      if (TPM_IS_TIS(tpm_find())) {
> > >          aml_append(crs, aml_memory32_fixed(TPM_TIS_ADDR_BASE,
> > >                     TPM_TIS_ADDR_SIZE, AML_READ_WRITE));
> > >      }
> > > +#endif
> > >      aml_append(scope, aml_name_decl("_CRS", crs));
> > >
> > >      /* reserve GPE0 block resources */
> > > @@ -2133,6 +2141,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);
> > >
> > > +#ifdef CONFIG_TPM
> > >              if (TPM_IS_TIS(tpm_find())) {
> > >                  dev = aml_device("ISA.TPM");
> > >                  aml_append(dev, aml_name_decl("_HID", aml_eisaid("PNP0C31")));
> > > @@ -2149,11 +2158,12 @@ build_dsdt(GArray *table_data, BIOSLinker *linker,
> > >                  aml_append(dev, aml_name_decl("_CRS", crs));
> > >                  aml_append(scope, dev);
> > >              }
> > > -
> > > +#endif
> > >              aml_append(sb_scope, scope);
> > >          }
> > >      }
> > >
> > > +#ifdef CONFIG_TPM
> > >      if (TPM_IS_CRB(tpm_find())) {
> > >          dev = aml_device("TPM");
> > >          aml_append(dev, aml_name_decl("_HID", aml_string("MSFT0101")));
> > > @@ -2168,7 +2178,7 @@ build_dsdt(GArray *table_data, BIOSLinker *linker,
> > >
> > >          aml_append(sb_scope, dev);
> > >      }
> > > -
> > > +#endif
> > >      aml_append(dsdt, sb_scope);
> > >
> > >      /* copy AML table into ACPI tables blob and patch header there */
> > > @@ -2194,6 +2204,7 @@ build_hpet(GArray *table_data, BIOSLinker *linker)
> > >                   (void *)hpet, "HPET", sizeof(*hpet), 1, NULL, NULL);
> > >  }
> > >
> > > +#ifdef CONFIG_TPM
> > >  static void
> > >  build_tpm_tcpa(GArray *table_data, BIOSLinker *linker, GArray *tcpalog)
> > >  {
> > > @@ -2247,6 +2258,7 @@ build_tpm2(GArray *table_data, BIOSLinker
> > > *linker, GArray *tcpalog)
> > >      build_header(linker, table_data,
> > >                   (void *)tpm2_ptr, "TPM2", sizeof(*tpm2_ptr), 4, NULL, NULL);
> > >  }
> > > +#endif
> > >
> > >  #define HOLE_640K_START  (640 * KiB)
> > >  #define HOLE_640K_END   (1 * MiB)
> > > @@ -2679,6 +2691,7 @@ void acpi_build(AcpiBuildTables *tables,
> > > MachineState *machine)
> > >          acpi_add_table(table_offsets, tables_blob);
> > >          build_hpet(tables_blob, tables->linker);
> > >      }
> > > +#ifdef CONFIG_TPM
> > >      if (misc.tpm_version != TPM_VERSION_UNSPEC) {
> > >          acpi_add_table(table_offsets, tables_blob);
> > >          build_tpm_tcpa(tables_blob, tables->linker, tables->tcpalog);
> > > @@ -2688,6 +2701,7 @@ void acpi_build(AcpiBuildTables *tables,
> > > MachineState *machine)
> > >              build_tpm2(tables_blob, tables->linker, tables->tcpalog);
> > >          }
> > >      }
> > > +#endif
> > >      if (pcms->numa_nodes) {
> > >          acpi_add_table(table_offsets, tables_blob);
> > >          build_srat(tables_blob, tables->linker, machine);
> > > @@ -2886,9 +2900,10 @@ void acpi_setup(void)
> > >          acpi_add_rom_blob(build_state, tables.linker->cmd_blob,
> > >                            "etc/table-loader", 0);
> > >
> > > +#ifdef CONFIG_TPM
> > >      fw_cfg_add_file(pcms->fw_cfg, ACPI_BUILD_TPMLOG_FILE,
> > >                      tables.tcpalog->data, acpi_data_len(tables.tcpalog));
> > > -
> > > +#endif
> > >      vmgenid_dev = find_vmgenid_dev();
> > >      if (vmgenid_dev) {
> > >          vmgenid_add_fw_cfg(VMGENID(vmgenid_dev), pcms->fw_cfg,
> > >
> > >
> > > >
> > > > > v13:
> > > > > - removed needless error handling in tpm_ppi_init()
> > > > > - splitted "add ACPI memory clear interface"
> > > > > - moved acpi build function in dedicated hw/acpi/tpm.c
> > > > > - added some function documentation in headers
> > > > > - various code cleanups suggested by Philippe
> > > > > - rebased
> > > > >
> > > > > Marc-André Lureau (3):
> > > > >   tpm: add a "ppi" boolean property
> > > > >   acpi: add ACPI memory clear interface
> > > > >   tpm: clear RAM when "memory overwrite" requested
> > > > >
> > > > > 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      |  47 +++++
> > > > >  include/hw/acpi/tpm.h |  23 +++
> > > > >  include/hw/compat.h   |  11 +-
> > > > >  hw/acpi/tpm.c         | 447 ++++++++++++++++++++++++++++++++++++++++++
> > > > >  hw/i386/acpi-build.c  |  29 ++-
> > > > >  hw/tpm/tpm_crb.c      |  11 ++
> > > > >  hw/tpm/tpm_ppi.c      |  53 +++++
> > > > >  hw/tpm/tpm_tis.c      |  11 ++
> > > > >  docs/specs/tpm.txt    | 104 ++++++++++
> > > > >  hw/acpi/Makefile.objs |   1 +
> > > > >  hw/tpm/Makefile.objs  |   1 +
> > > > >  hw/tpm/trace-events   |   3 +
> > > > >  12 files changed, 738 insertions(+), 3 deletions(-)
> > > > >  create mode 100644 hw/tpm/tpm_ppi.h
> > > > >  create mode 100644 hw/acpi/tpm.c
> > > > >  create mode 100644 hw/tpm/tpm_ppi.c
> > > > >
> > > > > --
> > > > > 2.20.0
> > > >
> > >
> > >
> > > --
> > > Marc-André Lureau
> 
> 
> 
> -- 
> Marc-André Lureau