Re: [Qemu-devel] [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/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
>
> diff --git a/hw/tpm/Makefile.objs b/hw/tpm/Makefile.objs
> index 190e776..cba961c 100644
> --- a/hw/tpm/Makefile.objs
> +++ b/hw/tpm/Makefile.objs
> @@ -1,3 +1,3 @@
> -common-obj-$(CONFIG_TPM_TIS) += tpm_tis.o
> +common-obj-$(CONFIG_TPM_TIS) += tpm_tis.o tpm_util.o
>   common-obj-$(CONFIG_TPM_PASSTHROUGH) += tpm_passthrough.o
>   common-obj-$(CONFIG_TPM_XENSTUBDOMS) += tpm_xenstubdoms.o xen_vtpm_frontend.o
> diff --git a/hw/tpm/tpm_passthrough.c b/hw/tpm/tpm_passthrough.c
> index 2a45071..ff08e15 100644
> --- a/hw/tpm/tpm_passthrough.c
> +++ b/hw/tpm/tpm_passthrough.c
> @@ -112,17 +112,6 @@ static void tpm_write_fatal_error_response(uint8_t *out, uint32_t out_len)
>       }
>   }
>
> -static bool tpm_passthrough_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;
> -}
> -
>   static int tpm_passthrough_unix_tx_bufs(TPMPassthruState *tpm_pt,
>                                           const uint8_t *in, uint32_t in_len,
>                                           uint8_t *out, uint32_t out_len,
> @@ -136,7 +125,7 @@ static int tpm_passthrough_unix_tx_bufs(TPMPassthruState *tpm_pt,
>       tpm_pt->tpm_executing = true;
>       *selftest_done = false;
>
> -    is_selftest = tpm_passthrough_is_selftest(in, in_len);
> +    is_selftest = tpm_util_is_selftest(in, in_len);
>
>       ret = tpm_passthrough_unix_write(tpm_pt->tpm_fd, in, in_len);
>       if (ret != in_len) {
> diff --git a/hw/tpm/tpm_util.c b/hw/tpm/tpm_util.c
> new file mode 100644
> index 0000000..8566781
> --- /dev/null
> +++ b/hw/tpm/tpm_util.c
> @@ -0,0 +1,50 @@
> +/*
> + *  TPM util functions
> + *
> + * *  Copyright (c) 2015 Intel Corporation
> + *  Authors:
> + *    Quan Xu <quan.xu@intel.com>
> + *
> + *  Copyright (c) 2010 - 2013 IBM Corporation
> + *  Authors:
> + *    Stefan Berger <stefanb@us.ibm.com>
> + *
> + *  Copyright (C) 2011 IAIK, Graz University of Technology
> + *    Author: Andreas Niederl

I don't think that for this particular function this Copyright applies.

> + *
> + * This library is free software; you can redistribute it and/or
> + * modify it under the terms of the GNU Lesser General Public
> + * License as published by the Free Software Foundation; either
> + * version 2 of the License, or (at your option) any later version.
> + *
> + * This library is distributed in the hope that it will be useful,
> + * but WITHOUT ANY WARRANTY; without even the implied warranty of
> + * MERCHANTABILITY or FITNESS FOR A PARTICULAR PURPOSE.  See the GNU
> + * Lesser General Public License for more details.
> + *
> + * You should have received a copy of the GNU Lesser General Public
> + * License along with this library; if not, see <http://www.gnu.org/licenses/>
> + */
> +
> +#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);
>
>   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.

     Stefan