[Date Prev][Date Next][Thread Prev][Thread Next][Date Index][Thread Index]
Re: [Qemu-devel] [PATCH v5 6/6] Qemu-Xen-vTPM: Add a parameter indicatin
From: |
Xu, Quan |
Subject: |
Re: [Qemu-devel] [PATCH v5 6/6] Qemu-Xen-vTPM: Add a parameter indicating whether the command that was a selftest |
Date: |
Mon, 13 Apr 2015 02:15:37 +0000 |
> -----Original Message-----
> From: Stefan Berger [mailto:address@hidden
> Sent: Monday, April 13, 2015 4:50 AM
> To: Xu, Quan; address@hidden; address@hidden
> Cc: address@hidden; address@hidden; address@hidden;
> address@hidden; address@hidden; address@hidden
> Subject: Re: [PATCH v5 6/6] Qemu-Xen-vTPM: Add a parameter indicating
> whether the command that was a selftest
>
> 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 <address@hidden>
> > ---
> > 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 <address@hidden>
> > + *
> > + * Copyright (c) 2010 - 2013 IBM Corporation
> > + * Authors:
> > + * Stefan Berger <address@hidden>
> > + *
> > + * Copyright (C) 2011 IAIK, Graz University of Technology
> > + * Author: Andreas Niederl
>
> I don't think that for this particular function this Copyright applies.
Thanks Stefan.
Could I just delete "Copyright (C) 2011 IAIK.."? share me more specific
copyright, if I am not correct.
>
> > + *
> > + * 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?
Agree, I will do it in next version ASAP.
>
> > +
> > +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.
>
Okay, I will do it in next version.
If the other patches are good, I can send out v6 soon.
Thanks for your review.
-Quan
> Stefan