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

[Stefan Berger]

"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