From mboxrd@z Thu Jan 1 00:00:00 1970 Received: from eggs.gnu.org ([140.186.70.92]:52169) by lists.gnu.org with esmtp (Exim 4.71) (envelope-from ) id 1RaHvL-0008La-KW for qemu-devel@nongnu.org; Mon, 12 Dec 2011 21:16:56 -0500 Received: from Debian-exim by eggs.gnu.org with spam-scanned (Exim 4.71) (envelope-from ) id 1RaHvK-0005ki-6R for qemu-devel@nongnu.org; Mon, 12 Dec 2011 21:16:55 -0500 Received: from e32.co.us.ibm.com ([32.97.110.150]:34178) by eggs.gnu.org with esmtp (Exim 4.71) (envelope-from ) id 1RaHvJ-0005jA-TF for qemu-devel@nongnu.org; Mon, 12 Dec 2011 21:16:54 -0500 Received: from /spool/local by e32.co.us.ibm.com with IBM ESMTP SMTP Gateway: Authorized Use Only! Violators will be prosecuted for from ; Mon, 12 Dec 2011 19:16:52 -0700 Received: from d03av02.boulder.ibm.com (d03av02.boulder.ibm.com [9.17.195.168]) by d03relay04.boulder.ibm.com (8.13.8/8.13.8/NCO v10.0) with ESMTP id pBD2GCHi163164 for ; Mon, 12 Dec 2011 19:16:12 -0700 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 pBD2GApm025492 for ; Mon, 12 Dec 2011 19:16:12 -0700 Message-ID: <4EE6B56A.7090508@linux.vnet.ibm.com> Date: Mon, 12 Dec 2011 21:16:10 -0500 From: Stefan Berger MIME-Version: 1.0 References: <1323717136-21661-1-git-send-email-stefanb@linux.vnet.ibm.com> <1323717136-21661-2-git-send-email-stefanb@linux.vnet.ibm.com> <4EE68B56.30704@codemonkey.ws> In-Reply-To: <4EE68B56.30704@codemonkey.ws> Content-Type: text/plain; charset=ISO-8859-1; format=flowed Content-Transfer-Encoding: 7bit Subject: Re: [Qemu-devel] [PATCH V13 1/7] Support for TPM command line options List-Id: List-Unsubscribe: , List-Archive: List-Post: List-Help: List-Subscribe: , To: Anthony Liguori Cc: andreas.niederl@iaik.tugraz.at, qemu-devel@nongnu.org, mst@redhat.com On 12/12/2011 06:16 PM, Anthony Liguori wrote: > On 12/12/2011 01:12 PM, Stefan Berger wrote: >> @@ -2735,6 +2736,15 @@ static mon_cmd_t info_cmds[] = { >> .help = "show available trace-events& their state", >> .mhandler.info = do_trace_print_events, >> }, >> +#if defined(CONFIG_TPM) >> + { >> + .name = "tpm", >> + .args_type = "", >> + .params = "", >> + .help = "show the TPM device", >> + .mhandler.info = do_info_tpm, >> + }, >> +#endif > > Please don't make monitor commands conditional. Make it fail in a > predictable fashion if tpm isn't configured. > This will then require tpm.c to always be compiled. You'll find the CONFIG_TPM there then. @@ -563,6 +582,7 @@ static QemuOptsList *vm_config_groups[32] = { >> &qemu_option_rom_opts, >> &qemu_machine_opts, >> &qemu_boot_opts, >> +&qemu_tpmdev_opts, > > I assume this is my mailer or is the whitespace munged here? > Must be your mailer.@@ -0,0 +1,167 @@ >> +/* >> + * TPM configuration >> + * >> + * Copyright (C) 2011 IBM Corporation >> + * >> + * Authors: >> + * Stefan Berger >> + * >> + * This work is licensed under the terms of the GNU GPL, version 2. >> See >> + * the COPYING file in the top-level directory. >> > > v2 or later please. ok. >> + >> +void tpm_display_backend_drivers(void) >> +{ >> + int i; >> + >> + fprintf(stderr, "Supported TPM types (choose only one):\n"); > > Having fprintfs to stderr is a sign something is wrong. > > In this case, we should have a programatic way to query the backends > (like via qmp_query_tpm) and then in the option handling code, we > should use that function and print to the screen from there. > Will try to convert but keep this function ? >> + >> +void do_info_tpm(Monitor *mon) >> +{ >> + TPMBackend *drv; >> + unsigned int c = 0; >> + >> + monitor_printf(mon, "TPM device:\n"); >> + >> + QLIST_FOREACH(drv,&tpm_backends, list) { >> + monitor_printf(mon, " tpm%d: model=%s\n", >> + c, drv->fe_model); >> + monitor_printf(mon, " \\ %s: type=%s%s%s\n", >> + drv->id, drv->ops->id, >> + drv->parameters ? "," : "", >> + drv->parameters ? drv->parameters : ""); >> + c++; >> + } >> +} > > We should do this through sure QAPI now that it's in the the tree with > a proper schema entry and an implementation in hmp.c. True... >> +void tpm_config_parse(QemuOptsList *opts_list, const char *optarg) >> +{ >> + QemuOpts *opts; >> + >> + if (strcmp("none", optarg) != 0) { >> + if (*optarg == '?') { >> + tpm_display_backend_drivers(); >> + exit(0); > > Don't exit from something other than vl.c. Return an error code and > let vl.c exit. I implemented this following along the lines of qemu-system-x86_64 -soundhw ? which then also shows and error code 0. What error code should it return to the shell ? > >> + } >> + opts = qemu_opts_parse(opts_list, optarg, 1); >> + if (!opts) { >> + exit(1); >> + } >> + } >> +} >> diff --git a/tpm.h b/tpm.h >> new file mode 100644 >> index 0000000..85c2a35 >> --- /dev/null >> +++ b/tpm.h >> @@ -0,0 +1,90 @@ > > Needs a copyright. Ok. >> +static inline void tpm_dump_buffer(FILE *stream, >> + unsigned char *buffer, unsigned >> int len) >> +{ >> + int i; >> + >> + for (i = 0; i< len; i++) { >> + if (i&& !(i % 16)) { >> + fprintf(stream, "\n"); >> + } >> + fprintf(stream, "%.2X ", buffer[i]); >> + } >> + fprintf(stream, "\n"); >> +} > > This definitely shouldn't be static inline and it's questionable > whether it should exist in the first place. Do you have an alternative for this function? Assuming it's useful for debugging, should I just move it into tpm.c ? > >> +#define TPM_DEFAULT_DEVICE_MODEL "tpm-tis" >> + >> +void tpm_config_parse(QemuOptsList *opts_list, const char *optarg); >> +int tpm_init(void); >> +void tpm_cleanup(void); >> +TPMBackend *qemu_find_tpm(const char *id); >> +void do_info_tpm(Monitor *mon); >> +void tpm_display_backend_drivers(void); >> +const TPMDriverOps *tpm_get_backend_driver(const char *id); > > Please document these functions. Will document them in tpm.c where their implementation is. @@ -2550,6 +2551,11 @@ int main(int argc, char **argv, char **envp) >> ram_size = value; >> break; >> } >> +#ifdef CONFIG_TPM >> + case QEMU_OPTION_tpmdev: >> + tpm_config_parse(qemu_find_opts("tpmdev"), optarg); >> + break; >> +#endif > > Don't make options conditional. Can I have an #ifdef-#else-#endif construct there along the lines of CONFIG_SDL with an exit(1) in the #else branch? Stefan