[Qemu-devel] [PATCH v3 4/8] tpm-backend: Made few interface methods optional

Amarnath Valluri posted 8 patches 8 years, 9 months ago
There is a newer version of this series
[Qemu-devel] [PATCH v3 4/8] tpm-backend: Made few interface methods optional
Posted by Amarnath Valluri 8 years, 9 months ago
This allows backend implementations left optional interface methods.
For mandatory methods assertion checks added.

Signed-off-by: Amarnath Valluri <amarnath.valluri@intel.com>
---
 backends/tpm.c           | 24 ++++++++++++++++++------
 hw/tpm/tpm_passthrough.c | 16 ----------------
 2 files changed, 18 insertions(+), 22 deletions(-)

diff --git a/backends/tpm.c b/backends/tpm.c
index cf5abf1..8245426 100644
--- a/backends/tpm.c
+++ b/backends/tpm.c
@@ -48,7 +48,7 @@ const char *tpm_backend_get_desc(TPMBackend *s)
 {
     TPMBackendClass *k = TPM_BACKEND_GET_CLASS(s);
 
-    return k->ops->desc();
+    return k->ops->desc ? k->ops->desc() : "";
 }
 
 int tpm_backend_init(TPMBackend *s, TPMState *state,
@@ -59,7 +59,7 @@ int tpm_backend_init(TPMBackend *s, TPMState *state,
     s->tpm_state = state;
     s->recv_data_callback = datacb;
 
-    return k->ops->init(s);
+    return k->ops->init ? k->ops->init(s) : 0;
 }
 
 int tpm_backend_startup_tpm(TPMBackend *s)
@@ -73,13 +73,15 @@ int tpm_backend_startup_tpm(TPMBackend *s)
                                        NULL);
     g_thread_pool_push(s->thread_pool, (gpointer)TPM_BACKEND_CMD_INIT, NULL);
 
-    return k->ops->startup_tpm(s);
+    return k->ops->startup_tpm ? k->ops->startup_tpm(s) : 0;
 }
 
 bool tpm_backend_had_startup_error(TPMBackend *s)
 {
     TPMBackendClass *k = TPM_BACKEND_GET_CLASS(s);
 
+    assert(k->ops->had_startup_error);
+
     return k->ops->had_startup_error(s);
 }
 
@@ -87,6 +89,8 @@ size_t tpm_backend_realloc_buffer(TPMBackend *s, TPMSizedBuffer *sb)
 {
     TPMBackendClass *k = TPM_BACKEND_GET_CLASS(s);
 
+    assert(k->ops->realloc_buffer);
+
     return k->ops->realloc_buffer(sb);
 }
 
@@ -100,7 +104,9 @@ void tpm_backend_reset(TPMBackend *s)
 {
     TPMBackendClass *k = TPM_BACKEND_GET_CLASS(s);
 
-    k->ops->reset(s);
+    if (k->ops->reset) {
+        k->ops->reset(s);
+    }
 
     tpm_backend_thread_end(s);
 }
@@ -109,6 +115,8 @@ void tpm_backend_cancel_cmd(TPMBackend *s)
 {
     TPMBackendClass *k = TPM_BACKEND_GET_CLASS(s);
 
+    g_assert(k->ops->cancel_cmd);
+
     k->ops->cancel_cmd(s);
 }
 
@@ -116,20 +124,24 @@ bool tpm_backend_get_tpm_established_flag(TPMBackend *s)
 {
     TPMBackendClass *k = TPM_BACKEND_GET_CLASS(s);
 
-    return k->ops->get_tpm_established_flag(s);
+    return k->ops->get_tpm_established_flag ?
+           k->ops->get_tpm_established_flag(s) : false;
 }
 
 int tpm_backend_reset_tpm_established_flag(TPMBackend *s, uint8_t locty)
 {
     TPMBackendClass *k = TPM_BACKEND_GET_CLASS(s);
 
-    return k->ops->reset_tpm_established_flag(s, locty);
+    return k->ops->reset_tpm_established_flag ?
+           k->ops->reset_tpm_established_flag(s, locty) : 0;
 }
 
 TPMVersion tpm_backend_get_tpm_version(TPMBackend *s)
 {
     TPMBackendClass *k = TPM_BACKEND_GET_CLASS(s);
 
+    assert(k->ops->get_tpm_version);
+
     return k->ops->get_tpm_version(s);
 }
 
diff --git a/hw/tpm/tpm_passthrough.c b/hw/tpm/tpm_passthrough.c
index 815a72e..bbe9e5a 100644
--- a/hw/tpm/tpm_passthrough.c
+++ b/hw/tpm/tpm_passthrough.c
@@ -227,15 +227,6 @@ static void tpm_passthrough_handle_request(TPMBackend *tb, TPMBackendCmd cmd)
     }
 }
 
-/*
- * Start the TPM (thread). If it had been started before, then terminate
- * and start it again.
- */
-static int tpm_passthrough_startup_tpm(TPMBackend *tb)
-{
-    return 0;
-}
-
 static void tpm_passthrough_reset(TPMBackend *tb)
 {
     TPMPassthruState *tpm_pt = TPM_PASSTHROUGH(tb);
@@ -247,11 +238,6 @@ static void tpm_passthrough_reset(TPMBackend *tb)
     tpm_pt->had_startup_error = false;
 }
 
-static int tpm_passthrough_init(TPMBackend *tb)
-{
-    return 0;
-}
-
 static bool tpm_passthrough_get_tpm_established_flag(TPMBackend *tb)
 {
     return false;
@@ -455,8 +441,6 @@ static const TPMDriverOps tpm_passthrough_driver = {
     .opts                     = tpm_passthrough_cmdline_opts,
     .desc                     = tpm_passthrough_create_desc,
     .create                   = tpm_passthrough_create,
-    .init                     = tpm_passthrough_init,
-    .startup_tpm              = tpm_passthrough_startup_tpm,
     .realloc_buffer           = tpm_passthrough_realloc_buffer,
     .reset                    = tpm_passthrough_reset,
     .had_startup_error        = tpm_passthrough_get_startup_error,
-- 
2.7.4


Re: [Qemu-devel] [PATCH v3 4/8] tpm-backend: Made few interface methods optional
Posted by Marc-André Lureau 8 years, 9 months ago
On Tue, May 2, 2017 at 3:52 PM Amarnath Valluri <amarnath.valluri@intel.com>
wrote:

> This allows backend implementations left optional interface methods.
> For mandatory methods assertion checks added.
>
> Signed-off-by: Amarnath Valluri <amarnath.valluri@intel.com>

---
>  backends/tpm.c           | 24 ++++++++++++++++++------
>  hw/tpm/tpm_passthrough.c | 16 ----------------
>  2 files changed, 18 insertions(+), 22 deletions(-)
>
> diff --git a/backends/tpm.c b/backends/tpm.c
> index cf5abf1..8245426 100644
> --- a/backends/tpm.c
> +++ b/backends/tpm.c
> @@ -48,7 +48,7 @@ const char *tpm_backend_get_desc(TPMBackend *s)
>  {
>      TPMBackendClass *k = TPM_BACKEND_GET_CLASS(s);
>
> -    return k->ops->desc();
> +    return k->ops->desc ? k->ops->desc() : "";
>  }
>

If it only need to returns const, we may consider using a class field,
defaulting to null.

Who is using it? tpm_backend_get_desc() not being called anywhere. Let's
remove it?


>  int tpm_backend_init(TPMBackend *s, TPMState *state,
> @@ -59,7 +59,7 @@ int tpm_backend_init(TPMBackend *s, TPMState *state,
>      s->tpm_state = state;
>      s->recv_data_callback = datacb;
>
> -    return k->ops->init(s);
> +    return k->ops->init ? k->ops->init(s) : 0;
>  }
>
>  int tpm_backend_startup_tpm(TPMBackend *s)
> @@ -73,13 +73,15 @@ int tpm_backend_startup_tpm(TPMBackend *s)
>                                         NULL);
>      g_thread_pool_push(s->thread_pool, (gpointer)TPM_BACKEND_CMD_INIT,
> NULL);
>
> -    return k->ops->startup_tpm(s);
> +    return k->ops->startup_tpm ? k->ops->startup_tpm(s) : 0;
>  }
>
>  bool tpm_backend_had_startup_error(TPMBackend *s)
>  {
>      TPMBackendClass *k = TPM_BACKEND_GET_CLASS(s);
>
> +    assert(k->ops->had_startup_error);
> +
>      return k->ops->had_startup_error(s);
>  }
>
> @@ -87,6 +89,8 @@ size_t tpm_backend_realloc_buffer(TPMBackend *s,
> TPMSizedBuffer *sb)
>  {
>      TPMBackendClass *k = TPM_BACKEND_GET_CLASS(s);
>
> +    assert(k->ops->realloc_buffer);
> +
>      return k->ops->realloc_buffer(sb);
>  }
>
> @@ -100,7 +104,9 @@ void tpm_backend_reset(TPMBackend *s)
>  {
>      TPMBackendClass *k = TPM_BACKEND_GET_CLASS(s);
>
> -    k->ops->reset(s);
> +    if (k->ops->reset) {
> +        k->ops->reset(s);
> +    }
>
>      tpm_backend_thread_end(s);
>  }
> @@ -109,6 +115,8 @@ void tpm_backend_cancel_cmd(TPMBackend *s)
>  {
>      TPMBackendClass *k = TPM_BACKEND_GET_CLASS(s);
>
> +    g_assert(k->ops->cancel_cmd);
>

Let's use assert() for consistency in the file


> +
>      k->ops->cancel_cmd(s);
>  }
>
> @@ -116,20 +124,24 @@ bool tpm_backend_get_tpm_established_flag(TPMBackend
> *s)
>  {
>      TPMBackendClass *k = TPM_BACKEND_GET_CLASS(s);
>
> -    return k->ops->get_tpm_established_flag(s);
> +    return k->ops->get_tpm_established_flag ?
> +           k->ops->get_tpm_established_flag(s) : false;
>  }
>
>  int tpm_backend_reset_tpm_established_flag(TPMBackend *s, uint8_t locty)
>  {
>      TPMBackendClass *k = TPM_BACKEND_GET_CLASS(s);
>
> -    return k->ops->reset_tpm_established_flag(s, locty);
> +    return k->ops->reset_tpm_established_flag ?
> +           k->ops->reset_tpm_established_flag(s, locty) : 0;
>  }
>
>  TPMVersion tpm_backend_get_tpm_version(TPMBackend *s)
>  {
>      TPMBackendClass *k = TPM_BACKEND_GET_CLASS(s);
>
> +    assert(k->ops->get_tpm_version);
> +
>      return k->ops->get_tpm_version(s);
>  }
>
> diff --git a/hw/tpm/tpm_passthrough.c b/hw/tpm/tpm_passthrough.c
> index 815a72e..bbe9e5a 100644
> --- a/hw/tpm/tpm_passthrough.c
> +++ b/hw/tpm/tpm_passthrough.c
> @@ -227,15 +227,6 @@ static void tpm_passthrough_handle_request(TPMBackend
> *tb, TPMBackendCmd cmd)
>      }
>  }
>
> -/*
> - * Start the TPM (thread). If it had been started before, then terminate
> - * and start it again.
> - */
> -static int tpm_passthrough_startup_tpm(TPMBackend *tb)
> -{
> -    return 0;
> -}
> -
>  static void tpm_passthrough_reset(TPMBackend *tb)
>  {
>      TPMPassthruState *tpm_pt = TPM_PASSTHROUGH(tb);
> @@ -247,11 +238,6 @@ static void tpm_passthrough_reset(TPMBackend *tb)
>      tpm_pt->had_startup_error = false;
>  }
>
> -static int tpm_passthrough_init(TPMBackend *tb)
> -{
> -    return 0;
> -}
> -
>  static bool tpm_passthrough_get_tpm_established_flag(TPMBackend *tb)
>  {
>      return false;
> @@ -455,8 +441,6 @@ static const TPMDriverOps tpm_passthrough_driver = {
>      .opts                     = tpm_passthrough_cmdline_opts,
>      .desc                     = tpm_passthrough_create_desc,
>      .create                   = tpm_passthrough_create,
> -    .init                     = tpm_passthrough_init,
> -    .startup_tpm              = tpm_passthrough_startup_tpm,
>      .realloc_buffer           = tpm_passthrough_realloc_buffer,
>      .reset                    = tpm_passthrough_reset,
>      .had_startup_error        = tpm_passthrough_get_startup_error,
> --
> 2.7.4
>
>
othewise,
Reviewed-by: Marc-André Lureau <marcandre.lureau@redhat.com>
-- 
Marc-André Lureau
Re: [Qemu-devel] [PATCH v3 4/8] tpm-backend: Made few interface methods optional
Posted by Amarnath Valluri 8 years, 9 months ago

On 05/02/2017 03:29 PM, Marc-André Lureau wrote:
>
>
> On Tue, May 2, 2017 at 3:52 PM Amarnath Valluri 
> <amarnath.valluri@intel.com <mailto:amarnath.valluri@intel.com>> wrote:
>
>     This allows backend implementations left optional interface methods.
>     For mandatory methods assertion checks added.
>
>     Signed-off-by: Amarnath Valluri <amarnath.valluri@intel.com
>     <mailto:amarnath.valluri@intel.com>> 
>
>     ---
>      backends/tpm.c           | 24 ++++++++++++++++++------
>      hw/tpm/tpm_passthrough.c | 16 ----------------
>      2 files changed, 18 insertions(+), 22 deletions(-)
>
>     diff --git a/backends/tpm.c b/backends/tpm.c
>     index cf5abf1..8245426 100644
>     --- a/backends/tpm.c
>     +++ b/backends/tpm.c
>     @@ -48,7 +48,7 @@ const char *tpm_backend_get_desc(TPMBackend *s)
>      {
>          TPMBackendClass *k = TPM_BACKEND_GET_CLASS(s);
>
>     -    return k->ops->desc();
>     +    return k->ops->desc ? k->ops->desc() : "";
>      }
>
>
> If it only need to returns const, we may consider using a class field, 
> defaulting to null.
Yes, it makes more sense being a const member than a function pointer.
>
> Who is using it? tpm_backend_get_desc() not being called anywhere. 
> Let's remove it?
Frankly no one using this API, rather it's been accessed 
directly(be_drivers[i]->desc())  in tpm.c. It can be removed.

- Amarnath
[Qemu-devel] [PATCH v4 4/8] tpm-backend: Made few interface methods optional
Posted by Amarnath Valluri 8 years, 9 months ago
This allows backend implementations left optional interface methods.
For mandatory methods assertion checks added.

Took the opportunity to remove unused methods:
  tpm_backend_get_type()
  tpm_backend_get_desc()

Signed-off-by: Amarnath Valluri <amarnath.valluri@intel.com>
---
 backends/tpm.c               | 36 +++++++++++++++++-------------------
 hw/tpm/tpm_passthrough.c     | 23 +----------------------
 include/sysemu/tpm_backend.h | 19 +------------------
 tpm.c                        |  2 +-
 4 files changed, 20 insertions(+), 60 deletions(-)

diff --git a/backends/tpm.c b/backends/tpm.c
index cf5abf1..05002e6 100644
--- a/backends/tpm.c
+++ b/backends/tpm.c
@@ -37,20 +37,6 @@ static void tpm_backend_thread_end(TPMBackend *s)
     }
 }
 
-enum TpmType tpm_backend_get_type(TPMBackend *s)
-{
-    TPMBackendClass *k = TPM_BACKEND_GET_CLASS(s);
-
-    return k->ops->type;
-}
-
-const char *tpm_backend_get_desc(TPMBackend *s)
-{
-    TPMBackendClass *k = TPM_BACKEND_GET_CLASS(s);
-
-    return k->ops->desc();
-}
-
 int tpm_backend_init(TPMBackend *s, TPMState *state,
                      TPMRecvDataCB *datacb)
 {
@@ -59,7 +45,7 @@ int tpm_backend_init(TPMBackend *s, TPMState *state,
     s->tpm_state = state;
     s->recv_data_callback = datacb;
 
-    return k->ops->init(s);
+    return k->ops->init ? k->ops->init(s) : 0;
 }
 
 int tpm_backend_startup_tpm(TPMBackend *s)
@@ -73,13 +59,15 @@ int tpm_backend_startup_tpm(TPMBackend *s)
                                        NULL);
     g_thread_pool_push(s->thread_pool, (gpointer)TPM_BACKEND_CMD_INIT, NULL);
 
-    return k->ops->startup_tpm(s);
+    return k->ops->startup_tpm ? k->ops->startup_tpm(s) : 0;
 }
 
 bool tpm_backend_had_startup_error(TPMBackend *s)
 {
     TPMBackendClass *k = TPM_BACKEND_GET_CLASS(s);
 
+    assert(k->ops->had_startup_error);
+
     return k->ops->had_startup_error(s);
 }
 
@@ -87,6 +75,8 @@ size_t tpm_backend_realloc_buffer(TPMBackend *s, TPMSizedBuffer *sb)
 {
     TPMBackendClass *k = TPM_BACKEND_GET_CLASS(s);
 
+    assert(k->ops->realloc_buffer);
+
     return k->ops->realloc_buffer(sb);
 }
 
@@ -100,7 +90,9 @@ void tpm_backend_reset(TPMBackend *s)
 {
     TPMBackendClass *k = TPM_BACKEND_GET_CLASS(s);
 
-    k->ops->reset(s);
+    if (k->ops->reset) {
+        k->ops->reset(s);
+    }
 
     tpm_backend_thread_end(s);
 }
@@ -109,6 +101,8 @@ void tpm_backend_cancel_cmd(TPMBackend *s)
 {
     TPMBackendClass *k = TPM_BACKEND_GET_CLASS(s);
 
+    assert(k->ops->cancel_cmd);
+
     k->ops->cancel_cmd(s);
 }
 
@@ -116,20 +110,24 @@ bool tpm_backend_get_tpm_established_flag(TPMBackend *s)
 {
     TPMBackendClass *k = TPM_BACKEND_GET_CLASS(s);
 
-    return k->ops->get_tpm_established_flag(s);
+    return k->ops->get_tpm_established_flag ?
+           k->ops->get_tpm_established_flag(s) : false;
 }
 
 int tpm_backend_reset_tpm_established_flag(TPMBackend *s, uint8_t locty)
 {
     TPMBackendClass *k = TPM_BACKEND_GET_CLASS(s);
 
-    return k->ops->reset_tpm_established_flag(s, locty);
+    return k->ops->reset_tpm_established_flag ?
+           k->ops->reset_tpm_established_flag(s, locty) : 0;
 }
 
 TPMVersion tpm_backend_get_tpm_version(TPMBackend *s)
 {
     TPMBackendClass *k = TPM_BACKEND_GET_CLASS(s);
 
+    assert(k->ops->get_tpm_version);
+
     return k->ops->get_tpm_version(s);
 }
 
diff --git a/hw/tpm/tpm_passthrough.c b/hw/tpm/tpm_passthrough.c
index 815a72e..a0459a6 100644
--- a/hw/tpm/tpm_passthrough.c
+++ b/hw/tpm/tpm_passthrough.c
@@ -227,15 +227,6 @@ static void tpm_passthrough_handle_request(TPMBackend *tb, TPMBackendCmd cmd)
     }
 }
 
-/*
- * Start the TPM (thread). If it had been started before, then terminate
- * and start it again.
- */
-static int tpm_passthrough_startup_tpm(TPMBackend *tb)
-{
-    return 0;
-}
-
 static void tpm_passthrough_reset(TPMBackend *tb)
 {
     TPMPassthruState *tpm_pt = TPM_PASSTHROUGH(tb);
@@ -247,11 +238,6 @@ static void tpm_passthrough_reset(TPMBackend *tb)
     tpm_pt->had_startup_error = false;
 }
 
-static int tpm_passthrough_init(TPMBackend *tb)
-{
-    return 0;
-}
-
 static bool tpm_passthrough_get_tpm_established_flag(TPMBackend *tb)
 {
     return false;
@@ -309,11 +295,6 @@ static void tpm_passthrough_cancel_cmd(TPMBackend *tb)
     }
 }
 
-static const char *tpm_passthrough_create_desc(void)
-{
-    return "Passthrough TPM backend driver";
-}
-
 static TPMVersion tpm_passthrough_get_tpm_version(TPMBackend *tb)
 {
     TPMPassthruState *tpm_pt = TPM_PASSTHROUGH(tb);
@@ -453,10 +434,8 @@ static const QemuOptDesc tpm_passthrough_cmdline_opts[] = {
 static const TPMDriverOps tpm_passthrough_driver = {
     .type                     = TPM_TYPE_PASSTHROUGH,
     .opts                     = tpm_passthrough_cmdline_opts,
-    .desc                     = tpm_passthrough_create_desc,
+    .desc                     = "Passthrough TPM backend driver",
     .create                   = tpm_passthrough_create,
-    .init                     = tpm_passthrough_init,
-    .startup_tpm              = tpm_passthrough_startup_tpm,
     .realloc_buffer           = tpm_passthrough_realloc_buffer,
     .reset                    = tpm_passthrough_reset,
     .had_startup_error        = tpm_passthrough_get_startup_error,
diff --git a/include/sysemu/tpm_backend.h b/include/sysemu/tpm_backend.h
index f61bcfe..d6ae463 100644
--- a/include/sysemu/tpm_backend.h
+++ b/include/sysemu/tpm_backend.h
@@ -75,7 +75,7 @@ struct TPMDriverOps {
     enum TpmType type;
     const QemuOptDesc *opts;
     /* get a descriptive text of the backend to display to the user */
-    const char *(*desc)(void);
+    const char *desc;
 
     TPMBackend *(*create)(QemuOpts *opts, const char *id);
 
@@ -99,23 +99,6 @@ struct TPMDriverOps {
     TPMVersion (*get_tpm_version)(TPMBackend *t);
 };
 
-
-/**
- * tpm_backend_get_type:
- * @s: the backend
- *
- * Returns the TpmType of the backend.
- */
-enum TpmType tpm_backend_get_type(TPMBackend *s);
-
-/**
- * tpm_backend_get_desc:
- * @s: the backend
- *
- * Returns a human readable description of the backend.
- */
-const char *tpm_backend_get_desc(TPMBackend *s);
-
 /**
  * tpm_backend_init:
  * @s: the backend to initialized
diff --git a/tpm.c b/tpm.c
index 70f74fa..2979508 100644
--- a/tpm.c
+++ b/tpm.c
@@ -102,7 +102,7 @@ static void tpm_display_backend_drivers(void)
 
     for (i = 0; i < TPM_MAX_DRIVERS && be_drivers[i] != NULL; i++) {
         fprintf(stderr, "%12s   %s\n",
-                TpmType_lookup[be_drivers[i]->type], be_drivers[i]->desc());
+                TpmType_lookup[be_drivers[i]->type], be_drivers[i]->desc);
     }
     fprintf(stderr, "\n");
 }
-- 
2.7.4