qemu-devel
[Top][All Lists]
Advanced
[Date Prev][Date Next][Thread Prev][Thread Next][Date Index][Thread Index]

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

From: Stefan Berger
Subject: Re: [Qemu-devel] [PATCH 24/42] tpm-be: call request_completed() out of thread
Date: Thu, 19 Oct 2017 19:21:19 -0400
User-agent: Mozilla/5.0 (X11; Linux x86_64; rv:45.0) Gecko/20100101 Thunderbird/45.4.0 
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 <address@hidden>
---
  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)),
reply via email to
[Prev in Thread] Current Thread [Next in Thread]