| Message ID | 1490965817-16913-3-git-send-email-amarnath.valluri@intel.com (mailing list archive) |
|---|---|
| State | New, archived |
| Headers | show |
Hi On Fri, Mar 31, 2017 at 5:04 PM Amarnath Valluri <amarnath.valluri@intel.com> 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 also demands to move TPMBackendClass definition to > tpm_backend_int.h. As handle_request() method is internal to TPMBackend > and its > derived classes, and shall not be visible for others. > Alternatively, I think you could remove tpm_backend_int.h, it doesn't bring much. > Signed-off-by: Amarnath Valluri <amarnath.valluri@intel.com> > --- > backends/tpm.c | 66 > ++++++++++++++++++++++++++-------------- > hw/tpm/tpm_passthrough.c | 57 +++++----------------------------- > include/sysemu/tpm_backend.h | 19 +++--------- > include/sysemu/tpm_backend_int.h | 19 ++++++------ > 4 files changed, 67 insertions(+), 94 deletions(-) > > diff --git a/backends/tpm.c b/backends/tpm.c > index 536f262..50a7809 100644 > --- a/backends/tpm.c > +++ b/backends/tpm.c > @@ -20,6 +20,25 @@ > #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); > + > + if (k->handle_request) { > + k->handle_request(s, (TPMBackendCmd)data); > + } > +} > Shouldn't handle_request be required by subclasses? I would have an assert() > + > +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) > { > TPMBackendClass *k = TPM_BACKEND_GET_CLASS(s); > @@ -39,6 +58,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 +67,26 @@ 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); > + > + if (!s->thread_pool) { > That check seems pointless to me, after thread_end() s->thread_pool is always NULL + 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 +106,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 +115,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 +191,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); > + tpm_backend_thread_end(s); > } > > static const TypeInfo tpm_backend_info = { > @@ -186,6 +207,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..606e064 100644 > --- a/hw/tpm/tpm_passthrough.c > +++ b/hw/tpm/tpm_passthrough.c > @@ -47,20 +47,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 +203,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 +213,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 +234,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; > } > > I would remove startup_tpm callback (could be a seperate patch) > @@ -268,20 +245,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 +283,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 +444,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 +479,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 +498,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..98b6112 100644 > --- a/include/sysemu/tpm_backend.h > +++ b/include/sysemu/tpm_backend.h > @@ -29,22 +29,17 @@ > > typedef struct TPMBackendClass TPMBackendClass; > typedef struct TPMBackend TPMBackend; > - > typedef struct TPMDriverOps TPMDriverOps; > - > -struct TPMBackendClass { > - ObjectClass parent_class; > - > - const TPMDriverOps *ops; > - > - void (*opened)(TPMBackend *s, Error **errp); > -}; > +typedef void (TPMRecvDataCB)(TPMState *, uint8_t locty, bool > selftest_done); > > struct TPMBackend { > Object parent; > > /*< protected >*/ > bool opened; > + TPMState *tpm_state; > + GThreadPool *thread_pool; > + TPMRecvDataCB *recv_data_callback; > > char *id; > enum TpmModel fe_model; > @@ -54,8 +49,6 @@ struct TPMBackend { > QLIST_ENTRY(TPMBackend) list; > }; > > -typedef void (TPMRecvDataCB)(TPMState *, uint8_t locty, bool > selftest_done); > - > typedef struct TPMSizedBuffer { > uint32_t size; > uint8_t *buffer; > @@ -71,7 +64,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 +72,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 > index 00639dd..9b48a35 100644 > --- a/include/sysemu/tpm_backend_int.h > +++ b/include/sysemu/tpm_backend_int.h > @@ -22,15 +22,6 @@ > #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, > @@ -38,4 +29,14 @@ typedef enum TPMBackendCmd { > TPM_BACKEND_CMD_TPM_RESET, > } TPMBackendCmd; > > +struct TPMBackendClass { > + ObjectClass parent_class; > + > + const TPMDriverOps *ops; > + > + void (*opened)(TPMBackend *s, Error **errp); > + > + void (*handle_request)(TPMBackend *s, TPMBackendCmd cmd); > +}; > + > #endif /* TPM_BACKEND_INT_H */ > -- > 2.7.4 > looks good otherwise
On 04.04.2017 13:56, Marc-André Lureau wrote: > Hi > > On Fri, Mar 31, 2017 at 5:04 PM Amarnath Valluri > <amarnath.valluri@intel.com <mailto:amarnath.valluri@intel.com>> 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 also demands to move TPMBackendClass definition to > tpm_backend_int.h. As handle_request() method is internal to > TPMBackend and its > derived classes, and shall not be visible for others. > > > Alternatively, I think you could remove tpm_backend_int.h, it doesn't > bring much. Yes, initially i even thought the same. _int.h makes sense, if we could move both TPMBackend, and TPMBackendClass _int.h, which shall be used by TPM backend implementations. And the actual backend API is exposed by tpm_backend.h, but the issue with QLIST member defined in TPMBackend which is been used by QLIST_FOREACH_SAFE in tpm.c > > > Signed-off-by: Amarnath Valluri <amarnath.valluri@intel.com > <mailto:amarnath.valluri@intel.com>> > --- > backends/tpm.c | 66 > ++++++++++++++++++++++++++-------------- > hw/tpm/tpm_passthrough.c | 57 > +++++----------------------------- > include/sysemu/tpm_backend.h | 19 +++--------- > include/sysemu/tpm_backend_int.h | 19 ++++++------ > 4 files changed, 67 insertions(+), 94 deletions(-) > > diff --git a/backends/tpm.c b/backends/tpm.c > index 536f262..50a7809 100644 > --- a/backends/tpm.c > +++ b/backends/tpm.c > @@ -20,6 +20,25 @@ > #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); > + > + if (k->handle_request) { > + k->handle_request(s, (TPMBackendCmd)data); > + } > +} > > > Shouldn't handle_request be required by subclasses? I would have an > assert() Yes, i agree, i will do the change > > + > +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) > { > TPMBackendClass *k = TPM_BACKEND_GET_CLASS(s); > @@ -39,6 +58,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 +67,26 @@ 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); > + > + if (!s->thread_pool) { > > > That check seems pointless to me, after thread_end() s->thread_pool is > always NULL Ok, I will remove the check. > > + 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 +106,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 +115,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 +191,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); > + tpm_backend_thread_end(s); > } > > static const TypeInfo tpm_backend_info = { > @@ -186,6 +207,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..606e064 100644 > --- a/hw/tpm/tpm_passthrough.c > +++ b/hw/tpm/tpm_passthrough.c > @@ -47,20 +47,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 +203,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 +213,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 +234,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; > } > > > I would remove startup_tpm callback (could be a seperate patch) Do you mean, remove the method from TPMDriverOps interface itself, of just the empty method here. The empty method is removed in 4/7 where we made them optional methods. > > @@ -268,20 +245,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 +283,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 +444,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 +479,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 +498,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..98b6112 100644 > --- a/include/sysemu/tpm_backend.h > +++ b/include/sysemu/tpm_backend.h > @@ -29,22 +29,17 @@ > > typedef struct TPMBackendClass TPMBackendClass; > typedef struct TPMBackend TPMBackend; > - > typedef struct TPMDriverOps TPMDriverOps; > - > -struct TPMBackendClass { > - ObjectClass parent_class; > - > - const TPMDriverOps *ops; > - > - void (*opened)(TPMBackend *s, Error **errp); > -}; > +typedef void (TPMRecvDataCB)(TPMState *, uint8_t locty, bool > selftest_done); > > struct TPMBackend { > Object parent; > > /*< protected >*/ > bool opened; > + TPMState *tpm_state; > + GThreadPool *thread_pool; > + TPMRecvDataCB *recv_data_callback; > > char *id; > enum TpmModel fe_model; > @@ -54,8 +49,6 @@ struct TPMBackend { > QLIST_ENTRY(TPMBackend) list; > }; > > -typedef void (TPMRecvDataCB)(TPMState *, uint8_t locty, bool > selftest_done); > - > typedef struct TPMSizedBuffer { > uint32_t size; > uint8_t *buffer; > @@ -71,7 +64,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 +72,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 > index 00639dd..9b48a35 100644 > --- a/include/sysemu/tpm_backend_int.h > +++ b/include/sysemu/tpm_backend_int.h > @@ -22,15 +22,6 @@ > #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, > @@ -38,4 +29,14 @@ typedef enum TPMBackendCmd { > TPM_BACKEND_CMD_TPM_RESET, > } TPMBackendCmd; > > +struct TPMBackendClass { > + ObjectClass parent_class; > + > + const TPMDriverOps *ops; > + > + void (*opened)(TPMBackend *s, Error **errp); > + > + void (*handle_request)(TPMBackend *s, TPMBackendCmd cmd); > +}; > + > #endif /* TPM_BACKEND_INT_H */ > -- > 2.7.4 > > > looks good otherwise > -- > Marc-André Lureau
diff --git a/backends/tpm.c b/backends/tpm.c index 536f262..50a7809 100644 --- a/backends/tpm.c +++ b/backends/tpm.c @@ -20,6 +20,25 @@ #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); + + if (k->handle_request) { + 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) { TPMBackendClass *k = TPM_BACKEND_GET_CLASS(s); @@ -39,6 +58,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 +67,26 @@ 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); + + if (!s->thread_pool) { + 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 +106,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 +115,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 +191,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); + tpm_backend_thread_end(s); } static const TypeInfo tpm_backend_info = { @@ -186,6 +207,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..606e064 100644 --- a/hw/tpm/tpm_passthrough.c +++ b/hw/tpm/tpm_passthrough.c @@ -47,20 +47,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 +203,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 +213,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 +234,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 +245,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 +283,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 +444,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 +479,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 +498,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..98b6112 100644 --- a/include/sysemu/tpm_backend.h +++ b/include/sysemu/tpm_backend.h @@ -29,22 +29,17 @@ typedef struct TPMBackendClass TPMBackendClass; typedef struct TPMBackend TPMBackend; - typedef struct TPMDriverOps TPMDriverOps; - -struct TPMBackendClass { - ObjectClass parent_class; - - const TPMDriverOps *ops; - - void (*opened)(TPMBackend *s, Error **errp); -}; +typedef void (TPMRecvDataCB)(TPMState *, uint8_t locty, bool selftest_done); struct TPMBackend { Object parent; /*< protected >*/ bool opened; + TPMState *tpm_state; + GThreadPool *thread_pool; + TPMRecvDataCB *recv_data_callback; char *id; enum TpmModel fe_model; @@ -54,8 +49,6 @@ struct TPMBackend { QLIST_ENTRY(TPMBackend) list; }; -typedef void (TPMRecvDataCB)(TPMState *, uint8_t locty, bool selftest_done); - typedef struct TPMSizedBuffer { uint32_t size; uint8_t *buffer; @@ -71,7 +64,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 +72,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 index 00639dd..9b48a35 100644 --- a/include/sysemu/tpm_backend_int.h +++ b/include/sysemu/tpm_backend_int.h @@ -22,15 +22,6 @@ #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, @@ -38,4 +29,14 @@ typedef enum TPMBackendCmd { TPM_BACKEND_CMD_TPM_RESET, } TPMBackendCmd; +struct TPMBackendClass { + ObjectClass parent_class; + + const TPMDriverOps *ops; + + void (*opened)(TPMBackend *s, Error **errp); + + void (*handle_request)(TPMBackend *s, TPMBackendCmd cmd); +}; + #endif /* TPM_BACKEND_INT_H */
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 also demands to move TPMBackendClass definition to tpm_backend_int.h. As handle_request() method is internal to TPMBackend and its derived classes, and shall not be visible for others. Signed-off-by: Amarnath Valluri <amarnath.valluri@intel.com> --- backends/tpm.c | 66 ++++++++++++++++++++++++++-------------- hw/tpm/tpm_passthrough.c | 57 +++++----------------------------- include/sysemu/tpm_backend.h | 19 +++--------- include/sysemu/tpm_backend_int.h | 19 ++++++------ 4 files changed, 67 insertions(+), 94 deletions(-)