Re: [Qemu-devel] [PATCH V1 3/8] Implementation of the TCG BIOS extensions

[Kevin O'Connor <kevin@koconnor.net>]

Hi Stefan,

I haven't had a chance to fully review, but I do have some comments.

On Wed, Mar 30, 2011 at 01:55:37PM -0400, Stefan Berger wrote:
[...]
> - a utility function called mssleep is added. It waits for a number
>   of milliseconds before it returns. I had tried to build a function
>   like that based on calc_future_time() and check_timer(), but those
>   didn't work once in an S3 resume.

There is already msleep() for this.  The inb(0x61) thing doesn't
actually work anywhere but on bochs.  If msleep() isn't working for
you, try rerunning calibrate_tsc() after S3 resume.

[...]
> +/*
> + *  Implementation of the TCG BIOS extension according to the specification
> + *  described in
> + *  https://www.trustedcomputinggroup.org/specs/PCClient/TCG_PCClientImplementationforBIOS_1-20_1-00.pdf
> + *
> + *  This library is free software; you can redistribute it and/or
> + *  modify it under the terms of the GNU Lesser General Public
> + *  License as published by the Free Software Foundation; either
> + *  version 2 of the License, or (at your option) any later version.

SeaBIOS is LGPLv3.  Also, I'm not a big fan of these big copyright
notices on the top of every file - is it necessary?

[...]
> +#if CONFIG_TCGBIOS

I'd prefer to avoid ifdefs in the code.  Everything should be setup to
have the compiler/linker automatically drop anything unused.  (So, a
simple "if (!CONFIG_TCGBIOS) return;" should drop any function and all
variables/functions only reachable from it.)

[...]
> +//#define DEBUG_TCGBIOS

Same here.  Can you use just set DEBUG_tcg to a value in config.h and
replace all the dprintf(1, ...) with dprintf(DEBUG_tcg, ...)?

> +static u8
> +calc_checksum(const u8 *addr, u32 length)

util.c:checksum()

> +static struct rsdp_descriptor *
> +find_rsdp(u8 *start, unsigned int len)

Should be unneeded - see how acpi.c:find_resume_vector() works.

-Kevin