Re: [Qemu-devel] [PATCH v2 05/28] tpm-be: call request_completed() out of thread

[Stefan Berger <stefanb@linux.vnet.ibm.com>]

On 11/06/2017 01:39 PM, Marc-André Lureau wrote:
> Lift from the backend implementation the responsability to call the
> request_completed() callback outside of thread context. This also
> simplify frontend/interface work, as they no longer need to care
> whether the callback is called from a different thread.
>
> Signed-off-by: Marc-André Lureau <marcandre.lureau@redhat.com>

Reviewed-by: Stefan Berger <stefanb@linux.vnet.ibm.com>



> ---
>   include/sysemu/tpm.h         |  1 -
>   include/sysemu/tpm_backend.h |  1 +
>   backends/tpm.c               | 15 ++++++++++++++-
>   hw/tpm/tpm_emulator.c        |  2 --
>   hw/tpm/tpm_passthrough.c     |  3 ---
>   hw/tpm/tpm_tis.c             | 34 ++++++++++++----------------------
>   6 files changed, 27 insertions(+), 29 deletions(-)
>
> diff --git a/include/sysemu/tpm.h b/include/sysemu/tpm.h
> index 7a1713a81e..e0879620e7 100644
> --- a/include/sysemu/tpm.h
> +++ b/include/sysemu/tpm.h
> @@ -40,7 +40,6 @@ typedef struct TPMIf {
>   typedef struct TPMIfClass {
>       InterfaceClass parent_class;
>   
> -    /* run in thread pool by backend */
>       void (*request_completed)(TPMIf *obj);
>   } TPMIfClass;
>   
> diff --git a/include/sysemu/tpm_backend.h b/include/sysemu/tpm_backend.h
> index b5f21ed8f1..c5d1a6818a 100644
> --- a/include/sysemu/tpm_backend.h
> +++ b/include/sysemu/tpm_backend.h
> @@ -47,6 +47,7 @@ struct TPMBackend {
>       bool opened;
>       GThreadPool *thread_pool;
>       bool had_startup_error;
> +    QEMUBH *bh;
>   
>       /* <public> */
>       char *id;
> diff --git a/backends/tpm.c b/backends/tpm.c
> index 86f0e7e915..58f823d54c 100644
> --- a/backends/tpm.c
> +++ b/backends/tpm.c
> @@ -18,14 +18,25 @@
>   #include "qapi/qmp/qerror.h"
>   #include "sysemu/tpm.h"
>   #include "qemu/thread.h"
> +#include "qemu/main-loop.h"
> +
> +static void tpm_backend_request_completed_bh(void *opaque)
> +{
> +    TPMBackend *s = TPM_BACKEND(opaque);
> +    TPMIfClass *tic = TPM_IF_GET_CLASS(s->tpmif);
> +
> +    tic->request_completed(s->tpmif);
> +}
>   
>   static void tpm_backend_worker_thread(gpointer data, gpointer user_data)
>   {
>       TPMBackend *s = TPM_BACKEND(user_data);
> -    TPMBackendClass *k  = TPM_BACKEND_GET_CLASS(s);
> +    TPMBackendClass *k = TPM_BACKEND_GET_CLASS(s);
>   
>       assert(k->handle_request != NULL);
>       k->handle_request(s, (TPMBackendCmd *)data);
> +
> +    qemu_bh_schedule(s->bh);
>   }
>   
>   static void tpm_backend_thread_end(TPMBackend *s)
> @@ -193,6 +204,7 @@ static void tpm_backend_instance_init(Object *obj)
>                                tpm_backend_prop_set_opened,
>                                NULL);
>       s->fe_model = -1;
> +    s->bh = qemu_bh_new(tpm_backend_request_completed_bh, s);
>   }
>   
>   static void tpm_backend_instance_finalize(Object *obj)
> @@ -202,6 +214,7 @@ static void tpm_backend_instance_finalize(Object *obj)
>       object_unref(OBJECT(s->tpmif));
>       g_free(s->id);
>       tpm_backend_thread_end(s);
> +    qemu_bh_delete(s->bh);
>   }
>   
>   static const TypeInfo tpm_backend_info = {
> diff --git a/hw/tpm/tpm_emulator.c b/hw/tpm/tpm_emulator.c
> index 6bf025c7e2..883e8c0c2d 100644
> --- a/hw/tpm/tpm_emulator.c
> +++ b/hw/tpm/tpm_emulator.c
> @@ -176,7 +176,6 @@ static int tpm_emulator_set_locality(TPMEmulator *tpm_emu, uint8_t locty_number,
>   static void tpm_emulator_handle_request(TPMBackend *tb, TPMBackendCmd *cmd)
>   {
>       TPMEmulator *tpm_emu = TPM_EMULATOR(tb);
> -    TPMIfClass *tic = TPM_IF_GET_CLASS(tb->tpmif);
>       Error *err = NULL;
>   
>       DPRINTF("processing TPM command");
> @@ -191,7 +190,6 @@ static void tpm_emulator_handle_request(TPMBackend *tb, TPMBackendCmd *cmd)
>           goto error;
>       }
>   
> -    tic->request_completed(tb->tpmif);
>       return;
>   
>   error:
> diff --git a/hw/tpm/tpm_passthrough.c b/hw/tpm/tpm_passthrough.c
> index 2ad74badca..8c002e4da6 100644
> --- a/hw/tpm/tpm_passthrough.c
> +++ b/hw/tpm/tpm_passthrough.c
> @@ -139,14 +139,11 @@ err_exit:
>   static void tpm_passthrough_handle_request(TPMBackend *tb, TPMBackendCmd *cmd)
>   {
>       TPMPassthruState *tpm_pt = TPM_PASSTHROUGH(tb);
> -    TPMIfClass *tic = TPM_IF_GET_CLASS(tb->tpmif);
>   
>       DPRINTF("tpm_passthrough: processing command %p\n", cmd);
>   
>       tpm_passthrough_unix_tx_bufs(tpm_pt, cmd->in, cmd->in_len,
>                                    cmd->out, cmd->out_len, &cmd->selftest_done);
> -
> -    tic->request_completed(tb->tpmif);
>   }
>   
>   static void tpm_passthrough_reset(TPMBackend *tb)
> diff --git a/hw/tpm/tpm_tis.c b/hw/tpm/tpm_tis.c
> index 4a8a2ffc79..f81aaf10c3 100644
> --- a/hw/tpm/tpm_tis.c
> +++ b/hw/tpm/tpm_tis.c
> @@ -76,7 +76,6 @@ typedef struct TPMState {
>       ISADevice busdev;
>       MemoryRegion mmio;
>   
> -    QEMUBH *bh;
>       uint32_t offset;
>       uint8_t buf[TPM_TIS_BUFFER_MAX];
>   
> @@ -410,10 +409,20 @@ static void tpm_tis_prep_abort(TPMState *s, uint8_t locty, uint8_t newlocty)
>       tpm_tis_abort(s, locty);
>   }
>   
> -static void tpm_tis_receive_bh(void *opaque)
> +/*
> + * Callback from the TPM to indicate that the response was received.
> + */
> +static void tpm_tis_request_completed(TPMIf *ti)
>   {
> -    TPMState *s = opaque;
> +    TPMState *s = TPM(ti);
>       uint8_t locty = s->cmd.locty;
> +    uint8_t l;
> +
> +    if (s->cmd.selftest_done) {
> +        for (l = 0; l < TPM_TIS_NUM_LOCALITIES; l++) {
> +            s->loc[locty].sts |= TPM_TIS_STS_SELFTEST_DONE;
> +        }
> +    }
>   
>       tpm_tis_sts_set(&s->loc[locty],
>                       TPM_TIS_STS_VALID | TPM_TIS_STS_DATA_AVAILABLE);
> @@ -431,23 +440,6 @@ static void tpm_tis_receive_bh(void *opaque)
>                         TPM_TIS_INT_DATA_AVAILABLE | TPM_TIS_INT_STS_VALID);
>   }
>   
> -static void tpm_tis_request_completed(TPMIf *ti)
> -{
> -    TPMState *s = TPM(ti);
> -
> -    bool is_selftest_done = s->cmd.selftest_done;
> -    uint8_t locty = s->cmd.locty;
> -    uint8_t l;
> -
> -    if (is_selftest_done) {
> -        for (l = 0; l < TPM_TIS_NUM_LOCALITIES; l++) {
> -            s->loc[locty].sts |= TPM_TIS_STS_SELFTEST_DONE;
> -        }
> -    }
> -
> -    qemu_bh_schedule(s->bh);
> -}
> -
>   /*
>    * Read a byte of response data
>    */
> @@ -1090,8 +1082,6 @@ static void tpm_tis_realizefn(DeviceState *dev, Error **errp)
>           return;
>       }
>   
> -    s->bh = qemu_bh_new(tpm_tis_receive_bh, s);
> -
>       isa_init_irq(&s->busdev, &s->irq, s->irq_num);
>   
>       memory_region_add_subregion(isa_address_space(ISA_DEVICE(dev)),