Re: [Qemu-devel] [PATCH v2 2/4] acpi: build QEMU table for PPI virtual memory device

[Stefan Berger]

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 <address@hidden>
> > ---
> >   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