| Message ID | 1516117900-11382-3-git-send-email-stefanb@linux.vnet.ibm.com (mailing list archive) |
|---|---|
| State | New, archived |
| Headers | show |
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
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 >
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 --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
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(+)