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

[Corey Bryant]

On 02/26/2013 07:18 PM, Stefan Berger wrote:
> 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:
> 

I think you want to stick with tpmdev rather than id.  It looks like id should 
be the identifier for the option it is specified with.  So in this case it 
should really represent the passthrough device.

Would something like this work instead?  This way you could keep the options as 
they were in v24.  This assumes fe_model is initialized to zero and the enum 
starts at 1 (TPM_MODEL_TPM_TIS=1).

TPMInfoList *qmp_query_tpm(Error **errp)
{
    TPMBackend *drv;
    TPMInfoList *info, *head = NULL, *cur_item = NULL;

    QLIST_FOREACH(drv, &tpm_backends, list) {
+       // if no matching frontend, then skip
+       if (drv->fe_model == 0) {
+           continue;
+       }
        info = g_new0(TPMInfoList, 1);
        info->value = qmp_query_tpm_inst(drv);

        if (!cur_item) {
            head = cur_item = info;
        } else {
            cur_item->next = info;
            cur_item = info;
        }
    }

    return head;
}

> 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
> 
> 

It's fine with me if you update this later.  It's just internal implementation 
details and it won't have an affect on any user interfaces.

-- 
Regards,
Corey Bryant