From mboxrd@z Thu Jan 1 00:00:00 1970 Received: from eggs.gnu.org ([208.118.235.92]:39340) by lists.gnu.org with esmtp (Exim 4.71) (envelope-from ) id 1TR6Jv-00057l-VF for qemu-devel@nongnu.org; Wed, 24 Oct 2012 15:08:52 -0400 Received: from Debian-exim by eggs.gnu.org with spam-scanned (Exim 4.71) (envelope-from ) id 1TR6Ju-00034H-I4 for qemu-devel@nongnu.org; Wed, 24 Oct 2012 15:08:51 -0400 Received: from e9.ny.us.ibm.com ([32.97.182.139]:52372) by eggs.gnu.org with esmtp (Exim 4.71) (envelope-from ) id 1TR6Ju-000345-DJ for qemu-devel@nongnu.org; Wed, 24 Oct 2012 15:08:50 -0400 Received: from /spool/local by e9.ny.us.ibm.com with IBM ESMTP SMTP Gateway: Authorized Use Only! Violators will be prosecuted for from ; Wed, 24 Oct 2012 15:08:49 -0400 Received: from d01relay05.pok.ibm.com (d01relay05.pok.ibm.com [9.56.227.237]) by d01dlp02.pok.ibm.com (Postfix) with ESMTP id CDF826E806E for ; Wed, 24 Oct 2012 15:06:50 -0400 (EDT) Received: from d01av02.pok.ibm.com (d01av02.pok.ibm.com [9.56.224.216]) by d01relay05.pok.ibm.com (8.13.8/8.13.8/NCO v10.0) with ESMTP id q9OJ6o5F065306 for ; Wed, 24 Oct 2012 15:06:50 -0400 Received: from d01av02.pok.ibm.com (loopback [127.0.0.1]) by d01av02.pok.ibm.com (8.14.4/8.13.1/NCO v10.0 AVout) with ESMTP id q9OJ6nw8022842 for ; Wed, 24 Oct 2012 17:06:49 -0200 Message-ID: <50883C48.5090607@linux.vnet.ibm.com> Date: Wed, 24 Oct 2012 15:06:48 -0400 From: Stefan Berger MIME-Version: 1.0 References: <1338838668-7544-1-git-send-email-stefanb@linux.vnet.ibm.com> <1338838668-7544-2-git-send-email-stefanb@linux.vnet.ibm.com> <50645EDF.8060105@linux.vnet.ibm.com> In-Reply-To: <50645EDF.8060105@linux.vnet.ibm.com> Content-Type: text/plain; charset=ISO-8859-1; format=flowed Content-Transfer-Encoding: 7bit Subject: Re: [Qemu-devel] [PATCH V19 1/7] Support for TPM command line options List-Id: List-Unsubscribe: , List-Archive: List-Post: List-Help: List-Subscribe: , To: Corey Bryant Cc: mst@redhat.com, qemu-devel@nongnu.org, anthony@codemonkey.ws, andreas.niederl@iaik.tugraz.at On 09/27/2012 10:12 AM, Corey Bryant wrote: > > > On 06/04/2012 03:37 PM, Stefan Berger wrote: >> + >> +#ifdef CONFIG_TPM >> + >> +static const TPMDriverOps *bes[] = { > > I think bes[] would be more descriptive if it were named be_drivers[] > or be_ops[]? > Renamed to be_drivers. >> + if (!QLIST_EMPTY(&tpm_backends)) { >> + error_report("Only one TPM is allowed.\n"); >> + return 1; >> + } > > A list of tpm_backends is maintained and walked in a few places, but > only one is allowed to be added to the list. Will it ever make sense > to enable multiple backends at one time? > A list is also returned through the monitor. This list can at the moment only have maximum of one entry. I would keep that list there unless someone else opposes. It may be possible to create different types of hardware emulation interfaces or simply replicate the TPM TIS at different addresses. So I cannot say whether it will 'ever make sense' to do that but I'd rather keep the opportunity there than close it and with that also let the monitor return a list of items rather than a single item. I removed the processing of the lists in this part of the code at least. >> + >> + value = qemu_opt_get(opts, "type"); >> + if (!value) { >> + qerror_report(QERR_MISSING_PARAMETER, "type"); >> + tpm_display_backend_drivers(); >> + return 1; >> + } >> + >> + be = tpm_get_backend_driver(value); > > The "type" value is being passed but the tpm_get_backend_driver() > defines the parameter as "id". Maybe "id" could be renamed to "type" > for consistency. See similar comment further down in this email. > Done. >> + */ >> +int tpm_config_parse(QemuOptsList *opts_list, const char *optarg) >> +{ >> + QemuOpts *opts; >> + >> + if (strcmp("none", optarg) != 0) { > > What's the point of supporting "-tpmdev none"? > Removed. >> +typedef struct TPMBackend { >> + char *id; > > For consistency, this could be named "type" instead of "id" since it > corresponds to -tpmdev's type. > Yes. >> + uint8_t command_locty; > > It would be easier to read if locality was spelled out fully, instead > of locty. But if that causes lines to be too long then maybe it's not > worth it. I rather keep it 'locty'. > >> + TPMLocality *cmd_locty; > > There's a cmd_locty and a command_locty. command_locty is the locality > number and cmd_locty is the locality data. Could these variable names > be updated to be more unique and descriptive? > Will rename them command_locty -> locty_number and cmd_locty -> locty_data. Regards, Stefan