[SeaBIOS] [PATCH v9 2/6] Implementation of the TCG BIOS extensions

[Stefan Berger]

On 03/20/2015 09:15 PM, Kevin O'Connor wrote:
> On Fri, Mar 20, 2015 at 02:00:37PM -0400, Stefan Berger wrote:
>> From: Stefan Berger <stefann at linux.vnet.ibm.com>
>>
>> This patch implements the main part of the TCG BIOS extensions. It provides
>> the following functionality:
>>
>> - initialization of the TCPA ACPI table used for logging of measurements
>> - initialization of the TPM by sending a sequence of commands to it
>> - proper setup of the TPM before the BIOS hands over control to the bootloader
>> - support for S3 resume; BIOS sends TPM_Startup(ST_STATE) to TPM
>> - enable configuration of SeaBIOS to be built with TCGBIOS extensions
>>    All TCG BIOS extensions are activated with CONFIG_TCGBIOS.
> [...]
>> --- a/src/Kconfig
>> +++ b/src/Kconfig
>> @@ -421,6 +421,13 @@ menu "BIOS interfaces"
>>               modified by programs.  However, some old DOS high memory
>>               managers may require the UMB region to be read-only.
>>   
>> +    config TCGBIOS
>> +        select S3_RESUME
>> +        bool "TPM support and TCG BIOS extensions"
>> +        default y
>> +        help
>> +            Provide TPM support along with TCG BIOS extensions
> I don't think TCGBIOS should select S3_RESUME.  If resume processing
> is an absolute requirement then I think it should use "depends on".

Ok.


>
> [...]
>> --- a/src/boot.c
>> +++ b/src/boot.c
>> @@ -19,6 +19,7 @@
>>   #include "std/disk.h" // struct mbr_s
>>   #include "string.h" // memset
>>   #include "util.h" // irqtimer_calc
>> +#include "tcgbios.h" // tpm_*
>>   
>>   
>>   /****************************************************************
>> @@ -475,6 +476,7 @@ interactive_bootmenu(void)
>>   
>>       printf("Select boot device:\n\n");
>>       wait_threads();
>> +    tpm_leave_bios();
> Is this call really supposed to be in the boot menu?  The call wont
> get invoked if the boot menu isn't selected.


Right. I moved it into post.c maininit() after the wait_threads() call 
there.


>
>> --- a/src/post.c
>> +++ b/src/post.c
>> @@ -28,6 +28,7 @@
>>   #include "output.h" // dprintf
>>   #include "string.h" // memset
>>   #include "util.h" // kbd_init
>> +#include "tcgbios.h" // tpm_*
>>   
>>   
>>   /****************************************************************
>> @@ -220,6 +221,10 @@ maininit(void)
>>       if (threads_during_optionroms())
>>           device_hardware_setup();
>>   
>> +    // Initialize tpm (after acpi tables were written)
>> +    tpm_acpi_init();
>> +    tpm_startup();
> I think this should be just one call - such as "tpm_setup()", which in
> turn calls tpm_xyz().
>
> Also, functions with "_init" and "_setup" suffixes have specific
> meaning in the bootstrap ordering sequence.  So tpm shouldn't export
> functions with these suffixes unless they are invoked during the given
> sequence.


I move the tpm_acpi_init() call into tpm_startup().


>
> [...]
>> +u32
>> +tpm_startup(void)
>> +{
>> +    u32 rc;
>> +    u32 returnCode;
>> +
>> +    if (!CONFIG_TCGBIOS)
>> +        return 0;
>> +
>> +    if (!has_working_tpm())
>> +        return TCG_GENERAL_ERROR;
>> +
>> +    dprintf(DEBUG_tcg, "TCGBIOS: Starting with TPM_Startup(ST_CLEAR)\n");
>> +    rc = build_and_send_cmd(TPM_ORD_Startup,
>> +                            Startup_ST_CLEAR, sizeof(Startup_ST_CLEAR),
>> +                            NULL, 10, &returnCode, TPM_DURATION_TYPE_SHORT);
>> +
>> +    dprintf(DEBUG_tcg, "Return code from TPM_Startup = 0x%08x\n",
>> +            returnCode);
>> +
>> +#if CONFIG_COREBOOT
>> +    /* with other firmware on the system the TPM may already have been
>> +     * initialized
>> +     */
>> +    if (returnCode == TPM_INVALID_POSTINIT)
>> +        returnCode = 0;
>> +#endif
> That '#if' should be turned into an 'if (...)'.
>
> [...]

Done

>> +u32
>> +tpm_s3_resume(void)
>> +{
>> +    u32 rc;
>> +    u32 returnCode;
>> +
>> +    if (!CONFIG_TCGBIOS)
>> +        return 0;
>> +
>> +    if (!has_working_tpm())
>> +        return TCG_GENERAL_ERROR;
>> +
>> +    timer_setup();
> Why is the timer reinitialized here?  There may be implications in
> touching that hardware during an S3 resume.

I am initializing the time because of the TPM driver's usage of 
msleep(). Your comment reminds me of a comment years ago from Keir 
Fraser from Xen. There, we ended up using I think a DRAM refresh timer 
bit that was emulated by QEMU and flipped every few ns.


>
>> +
>> +    dprintf(DEBUG_tcg, "TCGBIOS: Resuming with TPM_Startup(ST_STATE)\n");
>> +
>> +    rc = build_and_send_cmd(TPM_ORD_Startup,
>> +                            Startup_ST_STATE, sizeof(Startup_ST_STATE),
>> +                            NULL, 10, &returnCode, TPM_DURATION_TYPE_SHORT);
>> +
>> +    dprintf(DEBUG_tcg, "TCGBIOS: ReturnCode from TPM_Startup = 0x%08x\n",
>> +            returnCode);
>> +
>> +    if (rc || returnCode)
>> +        goto err_exit;
>> +
>> +    return 0;
>> +
>> +err_exit:
>> +    dprintf(DEBUG_tcg, "TCGBIOS: TPM malfunctioning (line %d).\n", __LINE__);
>> +
>> +    tpm_state.tpm_working = 0;
>> +    if (rc)
>> +        return rc;
>> +    return TCG_TCG_COMMAND_ERROR;
>> +}
>> diff --git a/src/tcgbios.h b/src/tcgbios.h
>> new file mode 100644
>> index 0000000..7be89f8
>> --- /dev/null
>> +++ b/src/tcgbios.h
>> @@ -0,0 +1,388 @@
>> +#ifndef TCGBIOS_H
>> +#define TCGBIOS_H
>> +
>> +#include "types.h"
>> +#include "bregs.h" /* struct bregs */
> Why is "bregs.h" needed in the header - can it be included in the
> individual C files that need it?

This #include needs to only appear in the 3rd patch when the interrupt 
handler prototype appears. I move it there now.

    Stefan