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

[Stefan Berger <stefanb@linux.vnet.ibm.com>]

On 04/04/2011 12:14 AM, Kevin O'Connor wrote:
> Hi Stefan,
>
> I haven't had a chance to fully review, but I do have some comments.
Thanks. I can post a v2 as a followup to your changes.
> 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.
Using that one now.
> [...]
>> +/*
>> + *  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?
I will change it to look like the other files.
> [...]
>> +#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.)
Ok, I will adapt it to that.
> [...]
>> +//#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, ...)?

Did that now.
>> +static u8
>> +calc_checksum(const u8 *addr, u32 length)
> util.c:checksum()
Using that one.
>> +static struct rsdp_descriptor *
>> +find_rsdp(u8 *start, unsigned int len)
> Should be unneeded - see how acpi.c:find_resume_vector() works.
I was going to keep the function find_rsdp but change its implementation 
to the test you are doing in find_resume_vector() regarding rsdp signature.

    Stefan

> -Kevin
>