Re: [Qemu-devel] [PATCH v2] Add ACPI tables for TPM

[Laszlo Ersek]

On 07/30/14 17:29, Stefan Berger wrote:
> "Michael S. Tsirkin" <address@hidden> wrote on 07/30/2014 11:20:41 AM:
> 
>> From: "Michael S. Tsirkin" <address@hidden>
>> To: Stefan Berger/Watson/address@hidden
>> Cc: Laszlo Ersek <address@hidden>, address@hidden, Stefan
>> Berger <address@hidden>
>> Date: 07/30/2014 11:20 AM
>> Subject: Re: [PATCH v2] Add ACPI tables for TPM
>>
>> On Wed, Jul 30, 2014 at 11:10:27AM -0400, Stefan Berger wrote:
>> > Laszlo Ersek <address@hidden> wrote on 07/30/2014 10:36:38 AM:
>> >
>> > > From: Laszlo Ersek <address@hidden>
>> > > To: "Michael S. Tsirkin" <address@hidden>, Stefan
> Berger/Watson/address@hidden
>> > > Cc: address@hidden, Stefan Berger <address@hidden>
>> > > Date: 07/30/2014 10:36 AM
>> > > Subject: Re: [PATCH v2] Add ACPI tables for TPM
>> > >
>> > > On 07/30/14 15:20, Michael S. Tsirkin wrote:
>> > > > On Tue, Jul 29, 2014 at 06:52:19AM -0400, Stefan Berger wrote:
>> > > >> From: Stefan Berger <address@hidden>
>> > > >>
>> > > >> Add an SSDT ACPI table for the TPM device.
>> > > >> Add a TCPA table for BIOS logging area when a TPM is being used.
>> > > >>
>> > > >> The latter follows this spec here:
>> > > >>
>> > > >> http://www.trustedcomputinggroup.org/files/static_page_files/
>> > > DCD4188E-1A4B-B294-D050A155FB6F7385/
>> > > TCG_ACPIGeneralSpecification_PublicReview.pdf
>> > >
>> > > (Thanks for CC'ing me, Michael.)
>> > >
>> > > I skimmed this spec.
>> > >
>> > > >> +static void
>> > > >> +build_tpm_tcpa(GArray *table_data, GArray *linker)
>> > > >> +{
>> > > >> +    Acpi20Tcpa *tcpa;
>> > > >> +    uint32_t log_area_minimum_length = TPM_LOG_AREA_MINIMUM_SIZE;
>> > > >> +    uint64_t log_area_start_address;
>> > > >> +    size_t len = log_area_minimum_length + sizeof(*tcpa);
>> > > >> +
>> > > >> +    log_area_start_address = table_data->len + sizeof(*tcpa);
>> > > >> +
>> > > >> +    tcpa = acpi_data_push(table_data, len);
>> > > >> +
>> > > >> +    tcpa->platform_class =
> cpu_to_le16(TPM_TCPA_ACPI_CLASS_CLIENT);
>> > > >> +    tcpa->log_area_minimum_length = cpu_to_le32
>> (log_area_minimum_length);
>> > > >> +    tcpa->log_area_start_address = cpu_to_le64
>> (log_area_start_address);
>> > > >> +
>> > > >> +    /* LASA address to be filled by Guest linker */
>> > > >
>> > > > Hmm, you are simply allocating log area as part of the ACPI
> table.  It
>> > > > works because bios happens to allocate tables from high memory.
>> > > > But I think this is a problem in practice because
>> > > > bios is allowed to allocate acpi memory differently.
>> > > > On the other hand log presumably needs to reside in
>> > > > physical memory somewhere.
>> > > >
>> > > > If you need bios to allocate this memory, then we will
>> > > > need a new allocation type for this, add it to linker
>> > > > in bios and qemu.
>> > > >
>> > > > Alternatively, find some other way to get hold of
>> > > > physical memory.
>> > > > Is there a way to disable the log completely?
>> > > > As defined in your patch, I doubt there's anything there, ever ..
>> > > >
>> > > >
>> > > >
>> > > >> +    bios_linker_loader_add_pointer(linker, ACPI_BUILD_TABLE_FILE,
>> > > >> +                                   ACPI_BUILD_TABLE_FILE,
>> > > >> +                                   table_data,
>> > > &tcpa->log_area_start_address,
>> > > >> +                                   sizeof
>> (tcpa->log_area_start_address));
>> > > >> +    build_header(linker, table_data,
>> > > >> +                 (void *)tcpa, "TCPA", sizeof(*tcpa), 2);
>> > > >> +}
>> > >
>> > > So here's my understanding. The spec referenced above describes three
>> > > ACPI tables: two (client vs. server) for TPM 1.2, and a third one
>> > > (usable by both client & server platforms) for TPM 2.0.
>> > >
>> > > The code above prepares a TPM 1.2 table. (Signature: "TCPA".)
>> > >
>> > > This table has a field called LASA (Log Area Start Address) which
> points
>> > > to somewhere in (guest-)physical memory. The patch adds a "dummy
> range"
>> > > to the end of the TCPA table itself, and asks the linker to set
> LASA to
>> > > the beginning of that range.
>> > >
>> > > This won't work in OVMF, and not just because of the reason that
> Michael
>> > > mentions (ie. because the firmware, in particular SeaBIOS, might
>> > > allocate the TCPA table in an area that is unsuitable as LASA target).
>> > >
>> > > Rather, in OVMF this won't work because OVMF doesn't implement the
>> > > linking part of the linker. The *generic* edk2 protocol
>> > > (EFI_ACPI_TABLE_PROTOCOL, which is coded outside of OVMF) that
> OVMF uses
>> > > (as a client) to install ACPI tables in guest-phys memory requires
>> > > tables to be passed in one-by-one.
>> > >
>> > > The EFI_ACPI_TABLE_PROTOCOL implementation in edk2 handles *some*
>> > > well-known tables specially. It has knowledge of their internal
>> > > pointers, and when you install an ACPI table, EFI_ACPI_TABLE_PROTOCOL
>> > > updates pointers automatically. (For example when you install the
> FACS,
>> > > the protocol links it automatically into FACP.)
>> > >
>> > > The EFI_ACPI_TABLE_PROTOCOL implementation in edk2 doesn't seem to
> know
>> > > anything about the TCPA table, let alone the unstructured (?) TCG
> event
>> > > log that is pointed-to by TCPA.LASA.
>> > >
>> > > (I grepped for the TCPA signature,
>> > >
>> >
>>
> EFI_ACPI_5_0_TRUSTED_COMPUTING_PLATFORM_ALLIANCE_CAPABILITIES_TABLE_SIGNATURE.)
>> > >
>> > > This means that if you pass down a TCPA table, OVMF will install it
>> > > right now, but TCPA.LASA will be bogus.
>> > >
>> > > If I wanted to implement the complete linker as Michael envisioned it,
>> > > then I'd have to avoid edk2's EFI_ACPI_TABLE_PROTOCOL, and implement
>> > > ACPI table installation from zero, trying to mimic the SeaBIOS client
>> > > code, but in a way that matches the UEFI environment. I'm not ready to
>> > > do that. Definitely not without an "official" human-language
>> > > specification of the linker-loader interface.
>> > >
>> > > I skimmed the patch but I'm not sure what exactly the TPM emulation in
>> > > qemu depends on. Is it a command line option? Is it default for some
>> > > machine types?
>> > >
>> > > Alternatively, I could recognize the TCPA signature in OVMF when
> parsing
>> > > the ACPI blobs for table headers, and filter it out.
>> >
>> > This is the code for what I would call 'pointer relocation'. The
>> TCPA table is
>> > not the only place where this is used, but why is it an issue
>> there while not
>> > with the following?
>> >
>> >     fadt->firmware_ctrl = cpu_to_le32(facs);
>> >     /* FACS address to be filled by Guest linker */
>> >     bios_linker_loader_add_pointer(linker, ACPI_BUILD_TABLE_FILE,  
>> >                                    ACPI_BUILD_TABLE_FILE,
>> >                                    table_data, &fadt->firmware_ctrl,
>> >                                    sizeof fadt->firmware_ctrl);
>> >
>> > Regards,
>> >     Stefan
>>
>>
>> Becase FACS is an ACPI table. So BIOS allocates it
>> from E820_RESERVED at the moment but it does not have to,
>> it could mark it with E820_ACPI.
>> Guest can then interpret the tables and then release the
>> memory if it wishes.
>>
>> If you want to do it for TCPA you must tell bios that
>> this is not ACPI memory.
> 
> I see. Presumably the whole slew of FADT, FACS, RSDP, & RSDT would need
> a similar tag to keep the S3 resume vector around?

Not in OVMF, because edk2's EFI_ACPI_TABLE_PROTOCOL special cases FACS
(containing the S3 resume vector), allocating it in EfiACPIMemoryNVS memory.

Table 26. Memory Type Usage after ExitBootServices()
EfiACPIMemoryNVS: This memory is to be preserved by the loader and OS
                  in the working and ACPI S1–S3 states.

Thanks
Laszlo