diff mbox series

[v2,2/4] acpi: build QEMU table for PPI virtual memory device

Message ID 1516117900-11382-3-git-send-email-stefanb@linux.vnet.ibm.com
State New
Headers show
Series Implement Physical Presence interface for TPM 1.2 and 2 | expand

Commit Message

Stefan Berger Jan. 16, 2018, 3:51 p.m. UTC 
To avoid having to hard code the base address of the PPI virtual memory
device we introduce a QEMU ACPI table that holds the base address, if a
TPM 1.2 or 2 is used. This table gives us flexibility to move the base
address later on.

Signed-off-by: Stefan Berger <stefanb@linux.vnet.ibm.com>
---
 hw/i386/acpi-build.c        | 19 +++++++++++++++++++
 include/hw/acpi/acpi-defs.h |  8 ++++++++
 2 files changed, 27 insertions(+)

Comments

Michael S. Tsirkin Jan. 16, 2018, 4:35 p.m. UTC | #1 
On Tue, Jan 16, 2018 at 10:51:38AM -0500, Stefan Berger wrote:
> To avoid having to hard code the base address of the PPI virtual memory
> device we introduce a QEMU ACPI table that holds the base address, if a
> TPM 1.2 or 2 is used. This table gives us flexibility to move the base
> address later on.
> 
> Signed-off-by: Stefan Berger <stefanb@linux.vnet.ibm.com>
> ---
>  hw/i386/acpi-build.c        | 19 +++++++++++++++++++
>  include/hw/acpi/acpi-defs.h |  8 ++++++++
>  2 files changed, 27 insertions(+)
> 
> diff --git a/hw/i386/acpi-build.c b/hw/i386/acpi-build.c
> index 18b939e..522d6d2 100644
> --- a/hw/i386/acpi-build.c
> +++ b/hw/i386/acpi-build.c
> @@ -2628,6 +2628,20 @@ static bool acpi_get_mcfg(AcpiMcfgInfo *mcfg)
>      return true;
>  }
>  
> +static void build_qemu(GArray *table_data, BIOSLinker *linker,
> +                       TPMVersion tpm_version)
> +{
> +    AcpiTableQemu *qemu = acpi_data_push(table_data, sizeof(*qemu));
> +
> +    if (tpm_version != TPM_VERSION_UNSPEC) {
> +        qemu->tpmppi_addr = TPM_PPI_ADDR_BASE;
> +        qemu->tpm_version = tpm_version;
> +    }
> +
> +    build_header(linker, table_data,
> +                 (void *)qemu, "QEMU", sizeof(*qemu), 1, "QEMU", "CONF");
> +}
> +
>  static
>  void acpi_build(AcpiBuildTables *tables, MachineState *machine)
>  {
> @@ -2734,6 +2748,11 @@ void acpi_build(AcpiBuildTables *tables, MachineState *machine)
>                            &pcms->acpi_nvdimm_state, machine->ram_slots);
>      }
>  
> +    if (misc.tpm_version != TPM_VERSION_UNSPEC) {
> +        acpi_add_table(table_offsets, tables_blob);
> +        build_qemu(tables_blob, tables->linker, misc.tpm_version);
> +    }
> +
>      /* Add tables supplied by user (if any) */
>      for (u = acpi_table_first(); u; u = acpi_table_next(u)) {
>          unsigned len = acpi_table_len(u);
> diff --git a/include/hw/acpi/acpi-defs.h b/include/hw/acpi/acpi-defs.h
> index 80c8099..98764c1 100644
> --- a/include/hw/acpi/acpi-defs.h
> +++ b/include/hw/acpi/acpi-defs.h
> @@ -573,6 +573,14 @@ struct Acpi20TPM2 {
>  } QEMU_PACKED;
>  typedef struct Acpi20TPM2 Acpi20TPM2;
>  
> +/* QEMU - Custom QEMU table */
> +struct AcpiTableQemu {
> +    ACPI_TABLE_HEADER_DEF

Since we already have an 8 byte QEMU table due to patching of MCFG
(which we should drop eventually I think) I think it's a good idea to
reserve the first 8 bytes here after the header.


> +    uint32_t tpmppi_addr;
> +    uint8_t tpm_version; /* 1 = 1.2, 2 = 2 */

There are 3 bytes of padding here. Pls make them explicit
as a reserved field.

> +};
> +typedef struct AcpiTableQemu AcpiTableQemu;
> +

>  /* DMAR - DMA Remapping table r2.2 */
>  struct AcpiTableDmar {
>      ACPI_TABLE_HEADER_DEF
> -- 
> 2.5.5
Laszlo Ersek Jan. 16, 2018, 8:42 p.m. UTC | #2 
On 01/16/18 16:51, Stefan Berger wrote:
> To avoid having to hard code the base address of the PPI virtual memory
> device we introduce a QEMU ACPI table that holds the base address, if a
> TPM 1.2 or 2 is used. This table gives us flexibility to move the base
> address later on.
> 
> Signed-off-by: Stefan Berger <stefanb@linux.vnet.ibm.com>
> ---
>  hw/i386/acpi-build.c        | 19 +++++++++++++++++++
>  include/hw/acpi/acpi-defs.h |  8 ++++++++
>  2 files changed, 27 insertions(+)

I don't understand how the guest OS is supposed to consume the QEMU
table. The AML code in patch #4 does not seem to consume the QEMU table,
for locating the operation region. I'm not saying that it *should*
consume the QEMU table, only that I'm currently not seeing a use for the
QEMU table.

How is the QEMU table useful? What breaks if we drop it?

(Sorry if we've been through this; then I must have lost context.)

Thanks,
Laszlo

> 
> diff --git a/hw/i386/acpi-build.c b/hw/i386/acpi-build.c
> index 18b939e..522d6d2 100644
> --- a/hw/i386/acpi-build.c
> +++ b/hw/i386/acpi-build.c
> @@ -2628,6 +2628,20 @@ static bool acpi_get_mcfg(AcpiMcfgInfo *mcfg)
>      return true;
>  }
>  
> +static void build_qemu(GArray *table_data, BIOSLinker *linker,
> +                       TPMVersion tpm_version)
> +{
> +    AcpiTableQemu *qemu = acpi_data_push(table_data, sizeof(*qemu));
> +
> +    if (tpm_version != TPM_VERSION_UNSPEC) {
> +        qemu->tpmppi_addr = TPM_PPI_ADDR_BASE;
> +        qemu->tpm_version = tpm_version;
> +    }
> +
> +    build_header(linker, table_data,
> +                 (void *)qemu, "QEMU", sizeof(*qemu), 1, "QEMU", "CONF");
> +}
> +
>  static
>  void acpi_build(AcpiBuildTables *tables, MachineState *machine)
>  {
> @@ -2734,6 +2748,11 @@ void acpi_build(AcpiBuildTables *tables, MachineState *machine)
>                            &pcms->acpi_nvdimm_state, machine->ram_slots);
>      }
>  
> +    if (misc.tpm_version != TPM_VERSION_UNSPEC) {
> +        acpi_add_table(table_offsets, tables_blob);
> +        build_qemu(tables_blob, tables->linker, misc.tpm_version);
> +    }
> +
>      /* Add tables supplied by user (if any) */
>      for (u = acpi_table_first(); u; u = acpi_table_next(u)) {
>          unsigned len = acpi_table_len(u);
> diff --git a/include/hw/acpi/acpi-defs.h b/include/hw/acpi/acpi-defs.h
> index 80c8099..98764c1 100644
> --- a/include/hw/acpi/acpi-defs.h
> +++ b/include/hw/acpi/acpi-defs.h
> @@ -573,6 +573,14 @@ struct Acpi20TPM2 {
>  } QEMU_PACKED;
>  typedef struct Acpi20TPM2 Acpi20TPM2;
>  
> +/* QEMU - Custom QEMU table */
> +struct AcpiTableQemu {
> +    ACPI_TABLE_HEADER_DEF
> +    uint32_t tpmppi_addr;
> +    uint8_t tpm_version; /* 1 = 1.2, 2 = 2 */
> +};
> +typedef struct AcpiTableQemu AcpiTableQemu;
> +
>  /* DMAR - DMA Remapping table r2.2 */
>  struct AcpiTableDmar {
>      ACPI_TABLE_HEADER_DEF
>
Stefan Berger Jan. 16, 2018, 9:20 p.m. UTC | #3 
On 01/16/2018 03:42 PM, Laszlo Ersek wrote:
> On 01/16/18 16:51, Stefan Berger wrote:
>> To avoid having to hard code the base address of the PPI virtual memory
>> device we introduce a QEMU ACPI table that holds the base address, if a
>> TPM 1.2 or 2 is used. This table gives us flexibility to move the base
>> address later on.
>>
>> Signed-off-by: Stefan Berger <stefanb@linux.vnet.ibm.com>
>> ---
>>   hw/i386/acpi-build.c        | 19 +++++++++++++++++++
>>   include/hw/acpi/acpi-defs.h |  8 ++++++++
>>   2 files changed, 27 insertions(+)
> I don't understand how the guest OS is supposed to consume the QEMU
> table. The AML code in patch #4 does not seem to consume the QEMU table,
> for locating the operation region. I'm not saying that it *should*
> consume the QEMU table, only that I'm currently not seeing a use for the
> QEMU table.
>
> How is the QEMU table useful? What breaks if we drop it?

The QEMU table would only be consumed by SeaBIOS. Following the 
discussion on the SeaBIOS ML, we'll move this into a new fw_cfg file.

     Stefan

>
> (Sorry if we've been through this; then I must have lost context.)
>
> Thanks,
> Laszlo
>
>> diff --git a/hw/i386/acpi-build.c b/hw/i386/acpi-build.c
>> index 18b939e..522d6d2 100644
>> --- a/hw/i386/acpi-build.c
>> +++ b/hw/i386/acpi-build.c
>> @@ -2628,6 +2628,20 @@ static bool acpi_get_mcfg(AcpiMcfgInfo *mcfg)
>>       return true;
>>   }
>>   
>> +static void build_qemu(GArray *table_data, BIOSLinker *linker,
>> +                       TPMVersion tpm_version)
>> +{
>> +    AcpiTableQemu *qemu = acpi_data_push(table_data, sizeof(*qemu));
>> +
>> +    if (tpm_version != TPM_VERSION_UNSPEC) {
>> +        qemu->tpmppi_addr = TPM_PPI_ADDR_BASE;
>> +        qemu->tpm_version = tpm_version;
>> +    }
>> +
>> +    build_header(linker, table_data,
>> +                 (void *)qemu, "QEMU", sizeof(*qemu), 1, "QEMU", "CONF");
>> +}
>> +
>>   static
>>   void acpi_build(AcpiBuildTables *tables, MachineState *machine)
>>   {
>> @@ -2734,6 +2748,11 @@ void acpi_build(AcpiBuildTables *tables, MachineState *machine)
>>                             &pcms->acpi_nvdimm_state, machine->ram_slots);
>>       }
>>   
>> +    if (misc.tpm_version != TPM_VERSION_UNSPEC) {
>> +        acpi_add_table(table_offsets, tables_blob);
>> +        build_qemu(tables_blob, tables->linker, misc.tpm_version);
>> +    }
>> +
>>       /* Add tables supplied by user (if any) */
>>       for (u = acpi_table_first(); u; u = acpi_table_next(u)) {
>>           unsigned len = acpi_table_len(u);
>> diff --git a/include/hw/acpi/acpi-defs.h b/include/hw/acpi/acpi-defs.h
>> index 80c8099..98764c1 100644
>> --- a/include/hw/acpi/acpi-defs.h
>> +++ b/include/hw/acpi/acpi-defs.h
>> @@ -573,6 +573,14 @@ struct Acpi20TPM2 {
>>   } QEMU_PACKED;
>>   typedef struct Acpi20TPM2 Acpi20TPM2;
>>   
>> +/* QEMU - Custom QEMU table */
>> +struct AcpiTableQemu {
>> +    ACPI_TABLE_HEADER_DEF
>> +    uint32_t tpmppi_addr;
>> +    uint8_t tpm_version; /* 1 = 1.2, 2 = 2 */
>> +};
>> +typedef struct AcpiTableQemu AcpiTableQemu;
>> +
>>   /* DMAR - DMA Remapping table r2.2 */
>>   struct AcpiTableDmar {
>>       ACPI_TABLE_HEADER_DEF
>>
diff mbox series

Patch

diff --git a/hw/i386/acpi-build.c b/hw/i386/acpi-build.c
index 18b939e..522d6d2 100644
--- a/hw/i386/acpi-build.c
+++ b/hw/i386/acpi-build.c
@@ -2628,6 +2628,20 @@  static bool acpi_get_mcfg(AcpiMcfgInfo *mcfg)
     return true;
 }
 
+static void build_qemu(GArray *table_data, BIOSLinker *linker,
+                       TPMVersion tpm_version)
+{
+    AcpiTableQemu *qemu = acpi_data_push(table_data, sizeof(*qemu));
+
+    if (tpm_version != TPM_VERSION_UNSPEC) {
+        qemu->tpmppi_addr = TPM_PPI_ADDR_BASE;
+        qemu->tpm_version = tpm_version;
+    }
+
+    build_header(linker, table_data,
+                 (void *)qemu, "QEMU", sizeof(*qemu), 1, "QEMU", "CONF");
+}
+
 static
 void acpi_build(AcpiBuildTables *tables, MachineState *machine)
 {
@@ -2734,6 +2748,11 @@  void acpi_build(AcpiBuildTables *tables, MachineState *machine)
                           &pcms->acpi_nvdimm_state, machine->ram_slots);
     }
 
+    if (misc.tpm_version != TPM_VERSION_UNSPEC) {
+        acpi_add_table(table_offsets, tables_blob);
+        build_qemu(tables_blob, tables->linker, misc.tpm_version);
+    }
+
     /* Add tables supplied by user (if any) */
     for (u = acpi_table_first(); u; u = acpi_table_next(u)) {
         unsigned len = acpi_table_len(u);
diff --git a/include/hw/acpi/acpi-defs.h b/include/hw/acpi/acpi-defs.h
index 80c8099..98764c1 100644
--- a/include/hw/acpi/acpi-defs.h
+++ b/include/hw/acpi/acpi-defs.h
@@ -573,6 +573,14 @@  struct Acpi20TPM2 {
 } QEMU_PACKED;
 typedef struct Acpi20TPM2 Acpi20TPM2;
 
+/* QEMU - Custom QEMU table */
+struct AcpiTableQemu {
+    ACPI_TABLE_HEADER_DEF
+    uint32_t tpmppi_addr;
+    uint8_t tpm_version; /* 1 = 1.2, 2 = 2 */
+};
+typedef struct AcpiTableQemu AcpiTableQemu;
+
 /* DMAR - DMA Remapping table r2.2 */
 struct AcpiTableDmar {
     ACPI_TABLE_HEADER_DEF