Re: [Qemu-devel] [PATCH 24/42] tpm-be: call request_completed() out of thread

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

On 10/19/2017 06:02 PM, Stefan Berger wrote:
> On 10/09/2017 06:56 PM, Marc-André Lureau wrote:
>> Lift from the backend implementation the responsability to call the
>> request_completed() callback outside of thread context. This also
>
> I don't think this is what you are doing here. It's still in thread 
> context.
>
> Something is breaking the TIS interface in this patch. The symptom is 
> that SeaBIOS doesn't show its menu anymore.
>
> I have to withdraw my Reviewed-by from this one. It looked 
> sufficiently harmless but it's not.
>
>     Stefan

This here fixes the problem for TIS:

diff --git a/hw/tpm/tpm_tis.c b/hw/tpm/tpm_tis.c
index 355427a..cd29925 100644
--- a/hw/tpm/tpm_tis.c
+++ b/hw/tpm/tpm_tis.c
@@ -416,7 +416,7 @@ static void tpm_tis_prep_abort(TPMState *s, uint8_t 
locty, uint8_t newlocty)
  static void tpm_tis_request_completed(TPMIf *ti)
  {
      TPMState *s = TPM(ti);
-    uint8_t locty = s->locty_number;
+    uint8_t locty = s->cmd.locty;
      uint8_t l;

      if (s->cmd.selftest_done) {






>
>> 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>
>> ---
>>   hw/tpm/tpm_int.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             | 36 +++++++++++++-----------------------
>>   6 files changed, 28 insertions(+), 30 deletions(-)
>>
>> diff --git a/hw/tpm/tpm_int.h b/hw/tpm/tpm_int.h
>> index 9c045b6691..9c49325f03 100644
>> --- a/hw/tpm/tpm_int.h
>> +++ b/hw/tpm/tpm_int.h
>> @@ -30,7 +30,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 8d08765b3c..dd4fb288ea 100644
>> --- a/include/sysemu/tpm_backend.h
>> +++ b/include/sysemu/tpm_backend.h
>> @@ -48,6 +48,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 07e7aa4abc..36454837b3 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 b3757bfbda..355427ab29 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];
>>
>> @@ -411,10 +410,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;
>> -    uint8_t locty = s->cmd.locty;
>> +    TPMState *s = TPM(ti);
>> +    uint8_t locty = s->locty_number;
>> +    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);
>> @@ -430,23 +439,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
>>    */
>> @@ -1089,8 +1081,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)),
>
>