From mboxrd@z Thu Jan 1 00:00:00 1970 Received: from eggs.gnu.org ([2001:4830:134:3::10]:46559) by lists.gnu.org with esmtp (Exim 4.71) (envelope-from ) id 1d355n-0003QL-Sz for qemu-devel@nongnu.org; Tue, 25 Apr 2017 14:21:41 -0400 Received: from Debian-exim by eggs.gnu.org with spam-scanned (Exim 4.71) (envelope-from ) id 1d355j-0007LC-OV for qemu-devel@nongnu.org; Tue, 25 Apr 2017 14:21:39 -0400 Received: from mx0a-001b2d01.pphosted.com ([148.163.156.1]:32810) by eggs.gnu.org with esmtps (TLS1.0:RSA_AES_256_CBC_SHA1:32) (Exim 4.71) (envelope-from ) id 1d355j-0007L3-EZ for qemu-devel@nongnu.org; Tue, 25 Apr 2017 14:21:35 -0400 Received: from pps.filterd (m0098409.ppops.net [127.0.0.1]) by mx0a-001b2d01.pphosted.com (8.16.0.20/8.16.0.20) with SMTP id v3PI8iKC099469 for ; Tue, 25 Apr 2017 14:21:34 -0400 Received: from e38.co.us.ibm.com (e38.co.us.ibm.com [32.97.110.159]) by mx0a-001b2d01.pphosted.com with ESMTP id 2a29sgdhbg-1 (version=TLSv1.2 cipher=AES256-SHA bits=256 verify=NOT) for ; Tue, 25 Apr 2017 14:21:34 -0400 Received: from localhost by e38.co.us.ibm.com with IBM ESMTP SMTP Gateway: Authorized Use Only! Violators will be prosecuted for from ; Tue, 25 Apr 2017 12:21:32 -0600 References: <1491575431-32170-1-git-send-email-amarnath.valluri@intel.com> <1491575431-32170-3-git-send-email-amarnath.valluri@intel.com> From: Stefan Berger Date: Tue, 25 Apr 2017 14:21:26 -0400 MIME-Version: 1.0 In-Reply-To: <1491575431-32170-3-git-send-email-amarnath.valluri@intel.com> Content-Type: text/plain; charset=windows-1252; format=flowed Content-Transfer-Encoding: 7bit Message-Id: <0f7826ca-cdcd-606c-ab4f-028fd5ac2681@linux.vnet.ibm.com> Subject: Re: [Qemu-devel] [PATCH v2 2/9] tpm-backend: Move thread handling inside TPMBackend List-Id: List-Unsubscribe: , List-Archive: List-Post: List-Help: List-Subscribe: , To: Amarnath Valluri , qemu-devel@nongnu.org Cc: patrick.ohly@intel.com, marcandre.lureau@gmail.com On 04/07/2017 10:30 AM, Amarnath Valluri wrote: > Move thread handling inside TPMBackend, this way backend implementations need > not to maintain their own thread life cycle, instead they needs to implement > 'handle_request()' class method that always been called from a thread. > > This change made tpm_backend_int.h kind of useless, hence removed it. > > Signed-off-by: Amarnath Valluri > --- > backends/tpm.c | 63 +++++++++++++++++++++++++--------------- > hw/tpm/tpm_passthrough.c | 58 +++++------------------------------- > include/sysemu/tpm_backend.h | 32 ++++++++++++-------- > include/sysemu/tpm_backend_int.h | 41 -------------------------- > 4 files changed, 68 insertions(+), 126 deletions(-) > delete mode 100644 include/sysemu/tpm_backend_int.h > > diff --git a/backends/tpm.c b/backends/tpm.c > index 536f262..0ec0c67 100644 > --- a/backends/tpm.c > +++ b/backends/tpm.c > @@ -18,7 +18,24 @@ > #include "qapi/qmp/qerror.h" > #include "sysemu/tpm.h" > #include "qemu/thread.h" > -#include "sysemu/tpm_backend_int.h" > + > +static void tpm_backend_worker_thread(gpointer data, gpointer user_data) > +{ > + TPMBackend *s = TPM_BACKEND(user_data); > + TPMBackendClass *k = TPM_BACKEND_GET_CLASS(s); > + > + assert (k->handle_request != NULL); > + k->handle_request(s, (TPMBackendCmd)data); > +} > + > +static void tpm_backend_thread_end(TPMBackend *s) > +{ > + if (s->thread_pool) { > + g_thread_pool_push(s->thread_pool, (gpointer)TPM_BACKEND_CMD_END, NULL); > + g_thread_pool_free(s->thread_pool, FALSE, TRUE); > + s->thread_pool = NULL; > + } > +} > > enum TpmType tpm_backend_get_type(TPMBackend *s) > { > @@ -39,6 +56,8 @@ void tpm_backend_destroy(TPMBackend *s) > TPMBackendClass *k = TPM_BACKEND_GET_CLASS(s); > > k->ops->destroy(s); > + > + tpm_backend_thread_end(s); > } > > int tpm_backend_init(TPMBackend *s, TPMState *state, > @@ -46,13 +65,23 @@ int tpm_backend_init(TPMBackend *s, TPMState *state, > { > TPMBackendClass *k = TPM_BACKEND_GET_CLASS(s); > > - return k->ops->init(s, state, datacb); > + s->tpm_state = state; > + s->recv_data_callback = datacb; > + > + return k->ops->init(s); > } > > int tpm_backend_startup_tpm(TPMBackend *s) > { > TPMBackendClass *k = TPM_BACKEND_GET_CLASS(s); > > + /* terminate a running TPM */ > + tpm_backend_thread_end(s); > + > + s->thread_pool = g_thread_pool_new(tpm_backend_worker_thread, s, 1, TRUE, > + NULL); > + g_thread_pool_push(s->thread_pool, (gpointer)TPM_BACKEND_CMD_INIT, NULL); > + > return k->ops->startup_tpm(s); > } > > @@ -72,9 +101,8 @@ size_t tpm_backend_realloc_buffer(TPMBackend *s, TPMSizedBuffer *sb) > > void tpm_backend_deliver_request(TPMBackend *s) > { > - TPMBackendClass *k = TPM_BACKEND_GET_CLASS(s); > - > - k->ops->deliver_request(s); > + g_thread_pool_push(s->thread_pool, (gpointer)TPM_BACKEND_CMD_PROCESS_CMD, > + NULL); > } > > void tpm_backend_reset(TPMBackend *s) > @@ -82,6 +110,8 @@ void tpm_backend_reset(TPMBackend *s) > TPMBackendClass *k = TPM_BACKEND_GET_CLASS(s); > > k->ops->reset(s); > + > + tpm_backend_thread_end(s); > } > > void tpm_backend_cancel_cmd(TPMBackend *s) > @@ -156,29 +186,15 @@ static void tpm_backend_instance_init(Object *obj) > tpm_backend_prop_get_opened, > tpm_backend_prop_set_opened, > NULL); > -} > > -void tpm_backend_thread_deliver_request(TPMBackendThread *tbt) > -{ > - g_thread_pool_push(tbt->pool, (gpointer)TPM_BACKEND_CMD_PROCESS_CMD, NULL); > } > > -void tpm_backend_thread_create(TPMBackendThread *tbt, > - GFunc func, gpointer user_data) > +static void tpm_backend_instance_finalize(Object *obj) > { > - if (!tbt->pool) { > - tbt->pool = g_thread_pool_new(func, user_data, 1, TRUE, NULL); > - g_thread_pool_push(tbt->pool, (gpointer)TPM_BACKEND_CMD_INIT, NULL); > - } > -} > + TPMBackend *s = TPM_BACKEND(obj); > > -void tpm_backend_thread_end(TPMBackendThread *tbt) > -{ > - if (tbt->pool) { > - g_thread_pool_push(tbt->pool, (gpointer)TPM_BACKEND_CMD_END, NULL); > - g_thread_pool_free(tbt->pool, FALSE, TRUE); > - tbt->pool = NULL; > - } > + g_free(s->id); I don't see this being removed somewhere else. Makes me think that we'd get a double-free here. > + tpm_backend_thread_end(s); > } > > static const TypeInfo tpm_backend_info = { > @@ -186,6 +202,7 @@ static const TypeInfo tpm_backend_info = { > .parent = TYPE_OBJECT, > .instance_size = sizeof(TPMBackend), > .instance_init = tpm_backend_instance_init, > + .instance_finalize = tpm_backend_instance_finalize, > .class_size = sizeof(TPMBackendClass), > .abstract = true, > }; > diff --git a/hw/tpm/tpm_passthrough.c b/hw/tpm/tpm_passthrough.c > index a0baf5f..f50d9cf 100644 > --- a/hw/tpm/tpm_passthrough.c > +++ b/hw/tpm/tpm_passthrough.c > @@ -30,7 +30,6 @@ > #include "tpm_int.h" > #include "hw/hw.h" > #include "hw/i386/pc.h" > -#include "sysemu/tpm_backend_int.h" > #include "tpm_tis.h" > #include "tpm_util.h" > > @@ -47,20 +46,9 @@ > OBJECT_CHECK(TPMPassthruState, (obj), TYPE_TPM_PASSTHROUGH) > > /* data structures */ > -typedef struct TPMPassthruThreadParams { > - TPMState *tpm_state; > - > - TPMRecvDataCB *recv_data_callback; > - TPMBackend *tb; > -} TPMPassthruThreadParams; > - > struct TPMPassthruState { > TPMBackend parent; > > - TPMBackendThread tbt; > - > - TPMPassthruThreadParams tpm_thread_params; > - > char *tpm_dev; > int tpm_fd; > bool tpm_executing; > @@ -214,12 +202,9 @@ static int tpm_passthrough_unix_transfer(TPMPassthruState *tpm_pt, > selftest_done); > } > > -static void tpm_passthrough_worker_thread(gpointer data, > - gpointer user_data) > +static void tpm_passthrough_handle_request(TPMBackend *tb, TPMBackendCmd cmd) > { > - TPMPassthruThreadParams *thr_parms = user_data; > - TPMPassthruState *tpm_pt = TPM_PASSTHROUGH(thr_parms->tb); > - TPMBackendCmd cmd = (TPMBackendCmd)data; > + TPMPassthruState *tpm_pt = TPM_PASSTHROUGH(tb); > bool selftest_done = false; > > DPRINTF("tpm_passthrough: processing command type %d\n", cmd); > @@ -227,12 +212,12 @@ static void tpm_passthrough_worker_thread(gpointer data, > switch (cmd) { > case TPM_BACKEND_CMD_PROCESS_CMD: > tpm_passthrough_unix_transfer(tpm_pt, > - thr_parms->tpm_state->locty_data, > + tb->tpm_state->locty_data, > &selftest_done); > > - thr_parms->recv_data_callback(thr_parms->tpm_state, > - thr_parms->tpm_state->locty_number, > - selftest_done); > + tb->recv_data_callback(tb->tpm_state, > + tb->tpm_state->locty_number, > + selftest_done); > break; > case TPM_BACKEND_CMD_INIT: > case TPM_BACKEND_CMD_END: > @@ -248,15 +233,6 @@ static void tpm_passthrough_worker_thread(gpointer data, > */ > static int tpm_passthrough_startup_tpm(TPMBackend *tb) > { > - TPMPassthruState *tpm_pt = TPM_PASSTHROUGH(tb); > - > - /* terminate a running TPM */ > - tpm_backend_thread_end(&tpm_pt->tbt); > - > - tpm_backend_thread_create(&tpm_pt->tbt, > - tpm_passthrough_worker_thread, > - &tpm_pt->tpm_thread_params); > - > return 0; > } > > @@ -268,20 +244,11 @@ static void tpm_passthrough_reset(TPMBackend *tb) > > tpm_passthrough_cancel_cmd(tb); > > - tpm_backend_thread_end(&tpm_pt->tbt); > - > tpm_pt->had_startup_error = false; > } > > -static int tpm_passthrough_init(TPMBackend *tb, TPMState *s, > - TPMRecvDataCB *recv_data_cb) > +static int tpm_passthrough_init(TPMBackend *tb) > { > - TPMPassthruState *tpm_pt = TPM_PASSTHROUGH(tb); > - > - tpm_pt->tpm_thread_params.tpm_state = s; > - tpm_pt->tpm_thread_params.recv_data_callback = recv_data_cb; > - tpm_pt->tpm_thread_params.tb = tb; > - > return 0; > } > > @@ -315,13 +282,6 @@ static size_t tpm_passthrough_realloc_buffer(TPMSizedBuffer *sb) > return sb->size; > } > > -static void tpm_passthrough_deliver_request(TPMBackend *tb) > -{ > - TPMPassthruState *tpm_pt = TPM_PASSTHROUGH(tb); > - > - tpm_backend_thread_deliver_request(&tpm_pt->tbt); > -} > - > static void tpm_passthrough_cancel_cmd(TPMBackend *tb) > { > TPMPassthruState *tpm_pt = TPM_PASSTHROUGH(tb); > @@ -483,8 +443,6 @@ static void tpm_passthrough_destroy(TPMBackend *tb) > > tpm_passthrough_cancel_cmd(tb); > > - tpm_backend_thread_end(&tpm_pt->tbt); > - > qemu_close(tpm_pt->tpm_fd); > qemu_close(tpm_pt->cancel_fd); > > @@ -520,7 +478,6 @@ static const TPMDriverOps tpm_passthrough_driver = { > .realloc_buffer = tpm_passthrough_realloc_buffer, > .reset = tpm_passthrough_reset, > .had_startup_error = tpm_passthrough_get_startup_error, > - .deliver_request = tpm_passthrough_deliver_request, > .cancel_cmd = tpm_passthrough_cancel_cmd, > .get_tpm_established_flag = tpm_passthrough_get_tpm_established_flag, > .reset_tpm_established_flag = tpm_passthrough_reset_tpm_established_flag, > @@ -540,6 +497,7 @@ static void tpm_passthrough_class_init(ObjectClass *klass, void *data) > TPMBackendClass *tbc = TPM_BACKEND_CLASS(klass); > > tbc->ops = &tpm_passthrough_driver; > + tbc->handle_request = tpm_passthrough_handle_request; > } > > static const TypeInfo tpm_passthrough_info = { > diff --git a/include/sysemu/tpm_backend.h b/include/sysemu/tpm_backend.h > index e7f590d..a538a7f 100644 > --- a/include/sysemu/tpm_backend.h > +++ b/include/sysemu/tpm_backend.h > @@ -29,22 +29,24 @@ > > typedef struct TPMBackendClass TPMBackendClass; > typedef struct TPMBackend TPMBackend; > - > typedef struct TPMDriverOps TPMDriverOps; > +typedef void (TPMRecvDataCB)(TPMState *, uint8_t locty, bool selftest_done); > > -struct TPMBackendClass { > - ObjectClass parent_class; > - > - const TPMDriverOps *ops; > - > - void (*opened)(TPMBackend *s, Error **errp); > -}; > +typedef enum TPMBackendCmd { > + TPM_BACKEND_CMD_INIT = 1, > + TPM_BACKEND_CMD_PROCESS_CMD, > + TPM_BACKEND_CMD_END, > + TPM_BACKEND_CMD_TPM_RESET, > +} TPMBackendCmd; > > struct TPMBackend { > Object parent; > > /*< protected >*/ > bool opened; > + TPMState *tpm_state; > + GThreadPool *thread_pool; > + TPMRecvDataCB *recv_data_callback; > > char *id; > enum TpmModel fe_model; > @@ -54,7 +56,15 @@ struct TPMBackend { > QLIST_ENTRY(TPMBackend) list; > }; > > -typedef void (TPMRecvDataCB)(TPMState *, uint8_t locty, bool selftest_done); > +struct TPMBackendClass { > + ObjectClass parent_class; > + > + const TPMDriverOps *ops; > + > + void (*opened)(TPMBackend *s, Error **errp); > + > + void (*handle_request)(TPMBackend *s, TPMBackendCmd cmd); > +}; > > typedef struct TPMSizedBuffer { > uint32_t size; > @@ -71,7 +81,7 @@ struct TPMDriverOps { > void (*destroy)(TPMBackend *t); > > /* initialize the backend */ > - int (*init)(TPMBackend *t, TPMState *s, TPMRecvDataCB *datacb); > + int (*init)(TPMBackend *t); > /* start up the TPM on the backend */ > int (*startup_tpm)(TPMBackend *t); > /* returns true if nothing will ever answer TPM requests */ > @@ -79,8 +89,6 @@ struct TPMDriverOps { > > size_t (*realloc_buffer)(TPMSizedBuffer *sb); > > - void (*deliver_request)(TPMBackend *t); > - > void (*reset)(TPMBackend *t); > > void (*cancel_cmd)(TPMBackend *t); > diff --git a/include/sysemu/tpm_backend_int.h b/include/sysemu/tpm_backend_int.h > deleted file mode 100644 > index 00639dd..0000000 > --- a/include/sysemu/tpm_backend_int.h > +++ /dev/null > @@ -1,41 +0,0 @@ > -/* > - * common TPM backend driver functions > - * > - * Copyright (c) 2012-2013 IBM Corporation > - * Authors: > - * Stefan Berger > - * > - * 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 > - */ > - > -#ifndef TPM_BACKEND_INT_H > -#define TPM_BACKEND_INT_H > - > -typedef struct TPMBackendThread { > - GThreadPool *pool; > -} TPMBackendThread; > - > -void tpm_backend_thread_deliver_request(TPMBackendThread *tbt); > -void tpm_backend_thread_create(TPMBackendThread *tbt, > - GFunc func, gpointer user_data); > -void tpm_backend_thread_end(TPMBackendThread *tbt); > - > -typedef enum TPMBackendCmd { > - TPM_BACKEND_CMD_INIT = 1, > - TPM_BACKEND_CMD_PROCESS_CMD, > - TPM_BACKEND_CMD_END, > - TPM_BACKEND_CMD_TPM_RESET, > -} TPMBackendCmd; > - > -#endif /* TPM_BACKEND_INT_H */ Otherwise looks good.