From mboxrd@z Thu Jan 1 00:00:00 1970 Received: from eggs.gnu.org ([2001:4830:134:3::10]:46086) by lists.gnu.org with esmtp (Exim 4.71) (envelope-from ) id 1e5K85-0004QJ-97 for qemu-devel@nongnu.org; Thu, 19 Oct 2017 19:21:34 -0400 Received: from Debian-exim by eggs.gnu.org with spam-scanned (Exim 4.71) (envelope-from ) id 1e5K80-0005HP-53 for qemu-devel@nongnu.org; Thu, 19 Oct 2017 19:21:33 -0400 Received: from mx0b-001b2d01.pphosted.com ([148.163.158.5]:42408 helo=mx0a-001b2d01.pphosted.com) by eggs.gnu.org with esmtps (TLS1.0:RSA_AES_256_CBC_SHA1:32) (Exim 4.71) (envelope-from ) id 1e5K7z-0005HG-VY for qemu-devel@nongnu.org; Thu, 19 Oct 2017 19:21:28 -0400 Received: from pps.filterd (m0098419.ppops.net [127.0.0.1]) by mx0b-001b2d01.pphosted.com (8.16.0.21/8.16.0.21) with SMTP id v9JNIjtb019034 for ; Thu, 19 Oct 2017 19:21:23 -0400 Received: from e18.ny.us.ibm.com (e18.ny.us.ibm.com [129.33.205.208]) by mx0b-001b2d01.pphosted.com with ESMTP id 2dpyrmya0q-1 (version=TLSv1.2 cipher=AES256-SHA bits=256 verify=NOT) for ; Thu, 19 Oct 2017 19:21:23 -0400 Received: from localhost by e18.ny.us.ibm.com with IBM ESMTP SMTP Gateway: Authorized Use Only! Violators will be prosecuted for from ; Thu, 19 Oct 2017 19:21:22 -0400 References: <20171009225623.29232-1-marcandre.lureau@redhat.com> <20171009225623.29232-25-marcandre.lureau@redhat.com> <8ada207e-8b98-2f24-5e53-8d60870dea4a@linux.vnet.ibm.com> From: Stefan Berger Date: Thu, 19 Oct 2017 19:21:19 -0400 MIME-Version: 1.0 In-Reply-To: <8ada207e-8b98-2f24-5e53-8d60870dea4a@linux.vnet.ibm.com> Content-Type: text/plain; charset=utf-8; format=flowed Message-Id: <6ec9edd8-aad4-709e-131d-85c7fb462407@linux.vnet.ibm.com> Content-Transfer-Encoding: quoted-printable Subject: Re: [Qemu-devel] [PATCH 24/42] tpm-be: call request_completed() out of thread List-Id: List-Unsubscribe: , List-Archive: List-Post: List-Help: List-Subscribe: , To: =?UTF-8?Q?Marc-Andr=c3=a9_Lureau?= , qemu-devel@nongnu.org Cc: amarnath.valluri@intel.com On 10/19/2017 06:02 PM, Stefan Berger wrote: > On 10/09/2017 06:56 PM, Marc-Andr=C3=A9 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=20 > context. > > Something is breaking the TIS interface in this patch. The symptom is=20 > that SeaBIOS doesn't show its menu anymore. > > I have to withdraw my Reviewed-by from this one. It looked=20 > 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=20 locty, uint8_t newlocty) static void tpm_tis_request_completed(TPMIf *ti) { TPMState *s =3D TPM(ti); - uint8_t locty =3D s->locty_number; + uint8_t locty =3D 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=C3=A9 Lureau >> --- >> 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; >> >> /* */ >> 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 =3D TPM_BACKEND(opaque); >> + TPMIfClass *tic =3D TPM_IF_GET_CLASS(s->tpmif); >> + >> + tic->request_completed(s->tpmif); >> +} >> >> static void tpm_backend_worker_thread(gpointer data, gpointer=20 >> user_data) >> { >> TPMBackend *s =3D TPM_BACKEND(user_data); >> - TPMBackendClass *k =3D TPM_BACKEND_GET_CLASS(s); >> + TPMBackendClass *k =3D TPM_BACKEND_GET_CLASS(s); >> >> assert(k->handle_request !=3D 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 =3D -1; >> + s->bh =3D 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=20 >> *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 =3D { >> 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=20 >> *tpm_emu, uint8_t locty_number, >> static void tpm_emulator_handle_request(TPMBackend *tb,=20 >> TPMBackendCmd *cmd) >> { >> TPMEmulator *tpm_emu =3D TPM_EMULATOR(tb); >> - TPMIfClass *tic =3D TPM_IF_GET_CLASS(tb->tpmif); >> Error *err =3D NULL; >> >> DPRINTF("processing TPM command"); >> @@ -191,7 +190,6 @@ static void=20 >> 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,=20 >> TPMBackendCmd *cmd) >> { >> TPMPassthruState *tpm_pt =3D TPM_PASSTHROUGH(tb); >> - TPMIfClass *tic =3D 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,=20 >> &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,=20 >> 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 =3D opaque; >> - uint8_t locty =3D s->cmd.locty; >> + TPMState *s =3D TPM(ti); >> + uint8_t locty =3D s->locty_number; >> + uint8_t l; >> + >> + if (s->cmd.selftest_done) { >> + for (l =3D 0; l < TPM_TIS_NUM_LOCALITIES; l++) { >> + s->loc[locty].sts |=3D 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 |=20 >> TPM_TIS_INT_STS_VALID); >> } >> >> -static void tpm_tis_request_completed(TPMIf *ti) >> -{ >> - TPMState *s =3D TPM(ti); >> - >> - bool is_selftest_done =3D s->cmd.selftest_done; >> - uint8_t locty =3D s->cmd.locty; >> - uint8_t l; >> - >> - if (is_selftest_done) { >> - for (l =3D 0; l < TPM_TIS_NUM_LOCALITIES; l++) { >> - s->loc[locty].sts |=3D 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,=20 >> Error **errp) >> return; >> } >> >> - s->bh =3D 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)), > >