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

[Stefan Berger]

On 03/22/2015 10:10 AM, Kevin O'Connor wrote:
> On Sun, Mar 22, 2015 at 09:55:47AM -0400, Stefan Berger wrote:
>> On 03/20/2015 09:15 PM, Kevin O'Connor wrote:
>>> On Fri, Mar 20, 2015 at 02:00:37PM -0400, Stefan Berger wrote:
>>>> +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.
> I think msleep() should work in s3 even without the call to
> timer_setup().  The settings found during the first boot should still
> be applicable during s3 resume.  Does it not work if the timer_setup()
> isn't invoked?

At least with Linux as OS, the timer_setup() call can be removed.

>
>>>> --- /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.
> If it's just for a function prototype the prefered approach is to
> forward declare the struct (ie, add the line "struct bregs;" to the
> header).

Ok, I will change it to that.

>
> -Kevin
>