Re: [PATCH v5 6/6] Qemu-Xen-vTPM: Add a parameter indicating whether the command that was a selftest

[Stefan Berger <stefanb@linux.vnet.ibm.com>]

On 04/12/2015 04:50 PM, Stefan Berger wrote:
> On 04/10/2015 02:59 AM, Quan Xu wrote:
>> and whether it completed successfully. Move 
>> tpm_passthrough_is_selftest() into
>> tpm_util.c and rename it to tpm_util_is_selftest().
>>
>> Signed-off-by: Quan Xu <quan.xu@intel.com>
>> ---
>>   hw/tpm/Makefile.objs             |  2 +-
>>   hw/tpm/tpm_passthrough.c         | 13 +----------
>>   hw/tpm/tpm_util.c                | 50 
>> ++++++++++++++++++++++++++++++++++++++++
>>   hw/tpm/tpm_xenstubdoms.c         | 36 +++++++++++++++++++++++------
>>   include/sysemu/tpm_backend_int.h |  1 +
>>   5 files changed, 82 insertions(+), 20 deletions(-)
>>   create mode 100644 hw/tpm/tpm_util.c
>>

>> +#include <dirent.h>
>> +
>> +#include "qemu-common.h"
>> +#include "qapi/error.h"
>> +#include "qemu/sockets.h"
>> +#include "sysemu/tpm_backend.h"
>> +#include "tpm_int.h"
>> +#include "hw/hw.h"
>> +#include "hw/i386/pc.h"
>> +#include "sysemu/tpm_backend_int.h"
>> +#include "tpm_tis.h"
>
> Can you reduce the includes to its minimum?
>
>> +
>> +bool tpm_util_is_selftest(const uint8_t *in, uint32_t in_len)
>> +{
>> +    struct tpm_req_hdr *hdr = (struct tpm_req_hdr *)in;
>> +
>> +    if (in_len >= sizeof(*hdr)) {
>> +        return (be32_to_cpu(hdr->ordinal) == TPM_ORD_ContinueSelfTest);
>> +    }
>> +
>> +    return false;
>> +}
>> diff --git a/hw/tpm/tpm_xenstubdoms.c b/hw/tpm/tpm_xenstubdoms.c
>> index 3d046fc..992f2ae 100644
>> --- a/hw/tpm/tpm_xenstubdoms.c
>> +++ b/hw/tpm/tpm_xenstubdoms.c
>> @@ -65,11 +65,17 @@ typedef struct TPMXenstubdomsState 
>> TPMXenstubdomsState;
>>   /* Functions */
>>   static void tpm_xenstubdoms_cancel_cmd(TPMBackend *tb);
>>
>> -static int tpm_xenstubdoms_unix_transfer(const TPMLocality *locty_data)
>> +static int tpm_xenstubdoms_unix_transfer(const TPMLocality *locty_data,
>> +                                         bool *selftest_done)
>>   {
>>       size_t rlen;
>>       struct XenDevice *xendev;
>>       int ret;
>> +    bool is_selftest;
>> +    const struct tpm_resp_hdr *hdr;
>> +
>> +    is_selftest = tpm_util_is_selftest(locty_data->w_buffer.buffer,
>> + locty_data->w_buffer.size);
>>
>>       xendev = xen_find_xendev("vtpm", xen_domid, xenstore_dev);
>>       if (xendev == NULL) {
>> @@ -81,12 +87,26 @@ static int tpm_xenstubdoms_unix_transfer(const 
>> TPMLocality *locty_data)
>>                       locty_data->r_buffer.size, locty_data->w_offset);
>>       if (ret < 0) {
>>           xen_be_printf(xendev, 0, "Can not send vtpm command.\n");
>> -        return -1;
>> +        goto err_exit;
>>       }
>>
>> -    vtpm_recv(xendev, locty_data->r_buffer.buffer, 
>> locty_data->r_buffer.size,
>> -              &rlen);
>> -    return 0;
>> +    ret = vtpm_recv(xendev, locty_data->r_buffer.buffer,
>> +                    locty_data->r_buffer.size, &rlen);
>> +    if (ret < 0) {
>> +        xen_be_printf(xendev, 0, "vtpm reception command error.\n");
>> +        goto err_exit;
>> +    }
>> +
>> +    if (is_selftest && (ret >= sizeof(struct tpm_resp_hdr))) {
>> +        hdr = (struct tpm_resp_hdr *)locty_data->r_buffer.buffer;
>> +        *selftest_done = (be32_to_cpu(hdr->errcode) == 0);
>> +    }
>> +
>> +err_exit:
>> +    if (ret < 0) {
>> +        xen_be_printf(xendev, 0, "vtpm command error.\n");
>> +    }
>> +    return ret;
>>   }
>>
>>   static void tpm_xenstubdoms_worker_thread(gpointer data,
>> @@ -94,13 +114,15 @@ static void 
>> tpm_xenstubdoms_worker_thread(gpointer data,
>>   {
>>       TPMXenstubdomsThreadParams *thr_parms = user_data;
>>       TPMBackendCmd cmd = (TPMBackendCmd)data;
>> +    bool selftest_done = false;
>>
>>       switch (cmd) {
>>       case TPM_BACKEND_CMD_PROCESS_CMD:
>> - tpm_xenstubdoms_unix_transfer(thr_parms->tpm_state->locty_data);
>> + tpm_xenstubdoms_unix_transfer(thr_parms->tpm_state->locty_data,
>> +                                      &selftest_done);
>> thr_parms->recv_data_callback(thr_parms->tpm_state,
>> thr_parms->tpm_state->locty_number,
>> -                                      false);
>> +                                      selftest_done);
>>           break;
>>       case TPM_BACKEND_CMD_INIT:
>>       case TPM_BACKEND_CMD_END:
>> diff --git a/include/sysemu/tpm_backend_int.h 
>> b/include/sysemu/tpm_backend_int.h
>> index 05d94d0..e18acab 100644
>> --- a/include/sysemu/tpm_backend_int.h
>> +++ b/include/sysemu/tpm_backend_int.h
>> @@ -34,6 +34,7 @@ void tpm_backend_thread_create(TPMBackendThread *tbt,
>>   void tpm_backend_thread_end(TPMBackendThread *tbt);
>>   void tpm_backend_thread_tpm_reset(TPMBackendThread *tbt,
>>                                     GFunc func, gpointer user_data);
>> +bool tpm_util_is_selftest(const uint8_t *in, uint32_t in_len);
>>

Put this into tpm_util.h.

>>   typedef enum TPMBackendCmd {
>>       TPM_BACKEND_CMD_INIT = 1,
>
> This patch per se looks good, but some of your modifications will not 
> compile until this patch is applied, due to the missing selftest_done 
> parameter. So you should merge this patch into the 3rd patch in this 
> series.

Actually, please put the part that splits out the tpm_util_is_selftest 
function into tpm_util.c,.h and the changes to the Makefile into a patch 
before your current patch 3.

    Stefan