Hi
On Fri, Mar 31, 2017 at 5:04 PM Amarnath Valluri <amarnath.vall...@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.vall...@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
--
Marc-André Lureau
