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

[Kevin O'Connor]

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?

> >>--- /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).

-Kevin