From mboxrd@z Thu Jan 1 00:00:00 1970 Received: from [140.186.70.92] (port=54065 helo=eggs.gnu.org) by lists.gnu.org with esmtp (Exim 4.43) id 1Q6l78-0004GO-C3 for qemu-devel@nongnu.org; Mon, 04 Apr 2011 10:50:57 -0400 Received: from Debian-exim by eggs.gnu.org with spam-scanned (Exim 4.71) (envelope-from ) id 1Q6l6E-0006tg-DD for qemu-devel@nongnu.org; Mon, 04 Apr 2011 10:49:51 -0400 Received: from e34.co.us.ibm.com ([32.97.110.152]:51774) by eggs.gnu.org with esmtp (Exim 4.71) (envelope-from ) id 1Q6l6E-0006rU-6K for qemu-devel@nongnu.org; Mon, 04 Apr 2011 10:49:50 -0400 Received: from d03relay01.boulder.ibm.com (d03relay01.boulder.ibm.com [9.17.195.226]) by e34.co.us.ibm.com (8.14.4/8.13.1) with ESMTP id p34EbiHG010421 for ; Mon, 4 Apr 2011 08:37:44 -0600 Received: from d03av02.boulder.ibm.com (d03av02.boulder.ibm.com [9.17.195.168]) by d03relay01.boulder.ibm.com (8.13.8/8.13.8/NCO v10.0) with ESMTP id p34Engrw078946 for ; Mon, 4 Apr 2011 08:49:42 -0600 Received: from d03av02.boulder.ibm.com (loopback [127.0.0.1]) by d03av02.boulder.ibm.com (8.14.4/8.13.1/NCO v10.0 AVout) with ESMTP id p34EngY3005443 for ; Mon, 4 Apr 2011 08:49:42 -0600 Message-ID: <4D99DA85.40009@linux.vnet.ibm.com> Date: Mon, 04 Apr 2011 10:49:41 -0400 From: Stefan Berger MIME-Version: 1.0 Subject: Re: [Qemu-devel] [PATCH V1 3/8] Implementation of the TCG BIOS extensions References: <20110330175534.302129463@linux.vnet.ibm.com> <20110330175558.859356867@linux.vnet.ibm.com> <20110404041432.GA13528@morn.localdomain> In-Reply-To: <20110404041432.GA13528@morn.localdomain> Content-Type: text/plain; charset=ISO-8859-1; format=flowed Content-Transfer-Encoding: 7bit List-Id: qemu-devel.nongnu.org List-Unsubscribe: , List-Archive: List-Post: List-Help: List-Subscribe: , To: Kevin O'Connor Cc: seabios@seabios.org, qemu-devel@nongnu.org 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 >