qemu-devel
[Top][All Lists]
Advanced

[Date Prev][Date Next][Thread Prev][Thread Next][Date Index][Thread Index]

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


From: Michael S. Tsirkin
Subject: Re: [Qemu-devel] [PATCH v2] Add ACPI tables for TPM
Date: Wed, 30 Jul 2014 18:05:56 +0200

On Wed, Jul 30, 2014 at 11:59:43AM -0400, Stefan Berger wrote:
> "Michael S. Tsirkin" <address@hidden> wrote on 07/30/2014 11:50:36 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:50 AM
> > Subject: Re: [PATCH v2] Add ACPI tables for TPM
> >
> > On Wed, Jul 30, 2014 at 11:29:36AM -0400, 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?
> > >
> > > Stefan
> >
> > Interesting, good point.
> > Yes ACPI spec says
> >    The BIOS aligns the FACS on a 64-byte boundary anywhere within the
> >    system’s memory address space. The memory where the FACS structure
> >    resides must not be reported as system AddressRangeMemory in the system
> >    address map. For example, the E820 address map reporting interface would
> >    report the region as AddressRangeReserved. For more information about
> >    system address map reporting interfaces, see Section 15, “System Address
> >    Map Interfaces.”
> >
> > I don't see where would the requirement for other tables come from.
> > Can you clarify please?
> >
> 
> Looking at SeaBIOS:
> 
> resume.c::s3_resume() calls find_resume_vector()
> 
> fw/biostables::find_resume_vector() calls find_fadt()
> 
> fw/biostables::find_fadt() accesses the RSDP then the RSDT then traverses its
> table of pointers to other ACPI tables and checks all their signatures until
> the FACP_SIGNATURE is found.
> 
> fw/biostables::find_resume_vector() then accesses the FACS and takes the
> firmware_waking_vector from it.
> 
> Had any of the tables been deallocated, S3 resume wouldn't work anymore.
> Besides that the signature checking wouldn't be all that great if the memory
> was now used for something else.
>    Stefan

Well this works because it reserves all tables.
If seabios wanted to stop doing this,
I guess it would have to stop looking for these things,
instead store FACS address somewhere else in reserved memory.

But nothing says it can't right?

-- 
MST



reply via email to

[Prev in Thread] Current Thread [Next in Thread]