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

[Laszlo Ersek]

On 07/30/14 18:05, Michael S. Tsirkin wrote:
> 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?

As far as I recall without looking, this is exactly what edk2 does. The
OS is allowed to reclaim memory (EfiACPIReclaimMemory) that backs the
"other" tables at some point.

In this regard the OS only needs to preserve EfiACPIMemoryNVS. Whatever
the firmware needs for S3 resume, it allocates as EfiACPIMemoryNVS (or
EfiReservedMemoryType, yes), including the FACS, and some extra stuff
that leads to it (LockBox in particular).

As far as I understand, if OVMF had a full linker/loader client right
now, it couldn't distinguish what to allocate as EfiACPIReclaimMemory,
and what else to allocate as EfiACPIMemoryNVS. So I would just opt for
safety and alloc everything as EfiACPIMemoryNVS. (I gather from your
above words that this is what SeaBIOS does too, in essence.)

Thanks
Laszlo