Re: [Qemu-devel] [PATCH V25 1/7] Support for TPM command line options

[Stefan Berger]

On 02/26/2013 04:58 PM, Corey Bryant wrote:
> I spent some time testing this patch series and just about everything 
> is working as expected.  Part of the testing included running Trousers 
> and the Trousers testsuite and they behaved as expected.  However I 
> did find a few minor issues (and a couple of nits) so I'll expand on 
> those inline.
>
> Assuming these issues get fixed though, I think the patches look good 
> and should be considered ready to merge.  That is my opinion at least.
>
> On 02/21/2013 11:33 AM, Stefan Berger wrote:
> > This patch adds support for TPM command line options.
> > The command line options supported here are
> >
> > ./qemu-... -tpmdev passthrough,path=<path to TPM device>,id=<id>
> >             -device tpm-tis,tpmdev=<id>
> >
> > and
> >
> > ./qemu-... -tpmdev help
> >
> > where the latter works similar to -soundhw help and shows a list of
> > available TPM backends (for example 'passthrough').
> >
> > Using the type parameter, the backend is chosen, i.e., 'passthrough' 
> > for the
> > passthrough driver. The interpretation of the other parameters along
> > with determining whether enough parameters were provided is pushed into
> > the backend driver, which needs to implement the interface function
> > 'create' and return a TPMDriver structure if the VM can be started or 
> > 'NULL'
>
> s/TPMDriver/TPMDriverOps

Fixed

> > (qemu) info tpm
> > TPM devices:
> >   tpm0: model=tpm-tis
> >    \ tpm0: 
> > type=passthrough,path=/dev/tpm0,cancel-path=/sys/devices/pnp0/00:09/cancel
>
> As we discussed offline, the code allows a guest to be started with a 
> backend and no frontend.  For example:
>
> qemu-system-x86_64 -tpmdev passthrough,id=tpm0,path=/dev/tpm0
>
> (It's missing -device tpm-tis,tpmdev=tpm0)

So with the necessary filtering this will then have to be -device 
tpm-tis,tpmdev=tpm0,id=tpm0 . I am not sure whether there is a better 
way of doing this. If there is, can someone please let me know? I need 
tpmdev=tpm0 to fill the Properties of the tpm_tis driver, which it uses 
to find the backend with the above 'id=tpm0'.For the hmp and qmp code to 
display the proper data and filter the above mention case I would then 
use the following patch:

Index: qemu-git.pt/tpm/tpm.c
===================================================================
--- qemu-git.pt.orig/tpm/tpm.c
+++ qemu-git.pt/tpm/tpm.c
@@ -281,6 +281,29 @@ static TPMInfo *qmp_query_tpm_inst(TPMBa
     return res;
 }

+static int tpm_find_tpmdev(const char *name, const char *val, void *id)
+{
+    if (!strcmp(name, "tpmdev")) {
+        return (strcmp(val, (char *)id) == 0);
+    }
+
+    return 0;
+}
+
+static bool tpm_find_frontend(const char *id)
+{
+    bool ret = false;
+
+    QemuOpts *qo = qemu_opts_find(qemu_find_opts("device"), id);
+    if (qo) {
+        if (qemu_opt_foreach(qo, tpm_find_tpmdev, (void *)id, 1) == 1) {
+            ret = true;
+        }
+    }
+
+    return ret;
+}
+
 /*
  * Walk the list of active TPM backends and collect information about them
  * following the schema description in qapi-schema.json.
@@ -291,6 +313,9 @@ TPMInfoList *qmp_query_tpm(Error **errp)
     TPMInfoList *info, *head = NULL, *cur_item = NULL;

     QLIST_FOREACH(drv, &tpm_backends, list) {
+        if (!tpm_find_frontend(drv->id)) {
+            continue;
+        }
         info = g_new0(TPMInfoList, 1);
         info->value = qmp_query_tpm_inst(drv);


I need to use 'tpm0' as search key in qemu_find_opts() which in turn 
requires that the device has id=tpm0 given.




> > +
> > +The specific backend type will determine the applicable options.
> > +The @code{-tpmdev} options requires a @code{-device} option.
>
> Drop the s from options?
Fixed.

> > +
> > +typedef struct TPMBackend {
> > +    char *id;
> > +    enum TpmModel fe_model;
> > +    char *path;
> > +    char *cancel_path;
>
> Should path and cancel_path be in the TPMPassthruState struct?

It should be, but the TPMPassthruState is is not a public structure. My 
suggestion is to move cancel_path into TPMPassthruState once the 
passthrough-specific options are parsed in the passthrough driver code. 
Now they are parsed where we don't have access to TPMPassthruState. This 
would be done in one of the future patches preparing for the other 
libtpms backend where I am separating the parsing for each backend. Agreed ?

Stefan