Re: [Qemu-devel] [PATCH v7 5/8] tmp backend: Add new api to read backend TpmInfo

["Valluri, Amarnath" <amarnath.valluri@intel.com>]

On Fri, 2017-09-22 at 17:35 +0200, Marc-André Lureau wrote:
> Hi
> 
> On Fri, Sep 22, 2017 at 2:33 PM, Amarnath Valluri
> <amarnath.valluri@intel.com> wrote:
> > 
> > TPM configuration options are backend implementation details and
> > shall not be
> > part of base TPMBackend object, and these shall not be accessed
> > directly outside
> > of the class, hence added a new interface method, get_tpm_options()
> > to
> > TPMDriverOps., which shall be implemented by the derived classes to
> > return
> > configured tpm options.
> > 
> > A new tpm backend api - tpm_backend_query_tpm() which uses
> > _get_tpm_options() to
> > prepare TpmInfo.
> > 
> > Signed-off-by: Amarnath Valluri <amarnath.valluri@intel.com>
> > Reviewed-by: Stefan Berger <stefanb@linux.vnet.ibm.com>
> > ---
> >  backends/tpm.c               | 22 +++++++++++++--
> >  hmp.c                        |  7 ++---
> >  hw/tpm/tpm_passthrough.c     | 64 +++++++++++++++++++++++++++++++-
> > ------------
> >  include/sysemu/tpm_backend.h | 25 +++++++++++++++--
> >  tpm.c                        | 32 +---------------------
> >  5 files changed, 93 insertions(+), 57 deletions(-)
> > 
> > diff --git a/backends/tpm.c b/backends/tpm.c
> > index c409a46..6ade9e4 100644
> > --- a/backends/tpm.c
> > +++ b/backends/tpm.c
> > @@ -138,6 +138,26 @@ TPMVersion
> > tpm_backend_get_tpm_version(TPMBackend *s)
> >      return k->ops->get_tpm_version(s);
> >  }
> > 
> > +TpmTypeOptions *tpm_backend_get_tpm_options(TPMBackend *s)
> > +{
> > +    TPMBackendClass *k = TPM_BACKEND_GET_CLASS(s);
> > +
> > +    assert(k->ops->get_tpm_options);
> > +
> > +    return k->ops->get_tpm_options(s);
> > +}
> > +
> Why make this public API?
Yes, I agree no need of this API, i will remove this
> 
> > 
> > +TPMInfo *tpm_backend_query_tpm(TPMBackend *s)
> > +{
> > +    TPMInfo *info = g_new0(TPMInfo, 1);
> > +
> > +    info->id = g_strdup(s->id);
> > +    info->model = s->fe_model;
> > +    info->options = tpm_backend_get_tpm_options(s);
> Callback could be called directly from here.
> 
> > 
> > +
> > +    return info;
> > +}
> > +
> >  static bool tpm_backend_prop_get_opened(Object *obj, Error **errp)
> >  {
> >      TPMBackend *s = TPM_BACKEND(obj);
> > @@ -192,8 +212,6 @@ static void
> > tpm_backend_instance_finalize(Object *obj)
> >      TPMBackend *s = TPM_BACKEND(obj);
> > 
> >      g_free(s->id);
> > -    g_free(s->path);
> > -    g_free(s->cancel_path);
> >      tpm_backend_thread_end(s);
> >  }
> > 
> > diff --git a/hmp.c b/hmp.c
> > index 0fb2bc7..cf62b2e 100644
> > --- a/hmp.c
> > +++ b/hmp.c
> > @@ -31,6 +31,7 @@
> >  #include "qapi/qmp/qerror.h"
> >  #include "qapi/string-input-visitor.h"
> >  #include "qapi/string-output-visitor.h"
> > +#include "qapi/util.h"
> >  #include "qapi-visit.h"
> >  #include "qom/object_interfaces.h"
> >  #include "ui/console.h"
> > @@ -1012,10 +1013,10 @@ void hmp_info_tpm(Monitor *mon, const QDict
> > *qdict)
> >                         c, TpmModel_str(ti->model));
> > 
> >          monitor_printf(mon, "  \\ %s: type=%s",
> > -                       ti->id, TpmTypeOptionsKind_str(ti->options-
> > >type));
> > +                       ti->id, TpmType_str(ti->options->type));
> > 
> Why this change? It is still a TpmTypeOptionsKind no?
This is was issue with my rebase, i will fix.
> 
> > 
> >          switch (ti->options->type) {
> > -        case TPM_TYPE_OPTIONS_KIND_PASSTHROUGH:
> > +        case TPM_TYPE_PASSTHROUGH:
> >              tpo = ti->options->u.passthrough.data;
> >              monitor_printf(mon, "%s%s%s%s",
> >                             tpo->has_path ? ",path=" : "",
> > @@ -1023,7 +1024,7 @@ void hmp_info_tpm(Monitor *mon, const QDict
> > *qdict)
> >                             tpo->has_cancel_path ? ",cancel-path="
> > : "",
> >                             tpo->has_cancel_path ? tpo->cancel_path 
> > : "");
> >              break;
> > -        case TPM_TYPE_OPTIONS_KIND__MAX:
> > +        case TPM_TYPE__MAX:
> >              break;
> >          }
> >          monitor_printf(mon, "\n");
> > diff --git a/hw/tpm/tpm_passthrough.c b/hw/tpm/tpm_passthrough.c
> > index a0459a6..fb7dad8 100644
> > --- a/hw/tpm/tpm_passthrough.c
> > +++ b/hw/tpm/tpm_passthrough.c
> > @@ -49,7 +49,8 @@
> >  struct TPMPassthruState {
> >      TPMBackend parent;
> > 
> > -    char *tpm_dev;
> > +    TPMPassthroughOptions *options;
> > +    const char *tpm_dev;
> >      int tpm_fd;
> >      bool tpm_executing;
> >      bool tpm_op_canceled;
> > @@ -308,15 +309,14 @@ static TPMVersion
> > tpm_passthrough_get_tpm_version(TPMBackend *tb)
> >   * in Documentation/ABI/stable/sysfs-class-tpm.
> >   * From /dev/tpm0 create /sys/class/misc/tpm0/device/cancel
> >   */
> > -static int tpm_passthrough_open_sysfs_cancel(TPMBackend *tb)
> > +static int tpm_passthrough_open_sysfs_cancel(TPMPassthruState
> > *tpm_pt)
> I suspect this change could be done in an earlier/separate patch
I feel this change is better part of this commit as this is side effect
of rearragend tpm options, now these are part of TPMPasstrhuState so
_open_sysfs_cancel() need not work on TPMBackend.
> 
> > 
> >  {
> > -    TPMPassthruState *tpm_pt = TPM_PASSTHROUGH(tb);
> >      int fd = -1;
> >      char *dev;
> >      char path[PATH_MAX];
> > 
> > -    if (tb->cancel_path) {
> > -        fd = qemu_open(tb->cancel_path, O_WRONLY);
> > +    if (tpm_pt->options->cancel_path) {
> > +        fd = qemu_open(tpm_pt->options->cancel_path, O_WRONLY);
> >          if (fd < 0) {
> >              error_report("Could not open TPM cancel path : %s",
> >                           strerror(errno));
> > @@ -331,7 +331,7 @@ static int
> > tpm_passthrough_open_sysfs_cancel(TPMBackend *tb)
> >                       dev) < sizeof(path)) {
> >              fd = qemu_open(path, O_WRONLY);
> >              if (fd >= 0) {
> > -                tb->cancel_path = g_strdup(path);
> > +                tpm_pt->options->cancel_path = g_strdup(path);
> >              } else {
> >                  error_report("tpm_passthrough: Could not open TPM
> > cancel "
> >                               "path %s : %s", path,
> > strerror(errno));
> > @@ -351,17 +351,18 @@ static int
> > tpm_passthrough_handle_device_opts(QemuOpts *opts, TPMBackend *tb)
> >      const char *value;
> > 
> >      value = qemu_opt_get(opts, "cancel-path");
> > -    tb->cancel_path = g_strdup(value);
> > +    if (value) {
> > +        tpm_pt->options->cancel_path = g_strdup(value);
> > +        tpm_pt->options->has_cancel_path = true;
> > +    }
> > 
> >      value = qemu_opt_get(opts, "path");
> > -    if (!value) {
> > -        value = TPM_PASSTHROUGH_DEFAULT_DEVICE;
> > +    if (value) {
> > +        tpm_pt->options->has_path = true;
> > +        tpm_pt->options->path = g_strdup(value);
> >      }
> > 
> > -    tpm_pt->tpm_dev = g_strdup(value);
> > -
> > -    tb->path = g_strdup(tpm_pt->tpm_dev);
> > -
> > +    tpm_pt->tpm_dev = value ? value :
> > TPM_PASSTHROUGH_DEFAULT_DEVICE;
> Isn't that duplicated with tpm_pt->options->path ? And different
> values...
This is intentional, options->path holds the configuration value, where
as tpm_dev points the real device path used.
> 
> > 
> >      tpm_pt->tpm_fd = qemu_open(tpm_pt->tpm_dev, O_RDWR);
> >      if (tpm_pt->tpm_fd < 0) {
> >          error_report("Cannot access TPM device using '%s': %s",
> > @@ -382,10 +383,8 @@ static int
> > tpm_passthrough_handle_device_opts(QemuOpts *opts, TPMBackend *tb)
> >      tpm_pt->tpm_fd = -1;
> > 
> >   err_free_parameters:
> > -    g_free(tb->path);
> > -    tb->path = NULL;
> > -
> > -    g_free(tpm_pt->tpm_dev);
> > +    qapi_free_TPMPassthroughOptions(tpm_pt->options);
> > +    tpm_pt->options = NULL;
> >      tpm_pt->tpm_dev = NULL;
> > 
> >      return 1;
> > @@ -403,7 +402,7 @@ static TPMBackend
> > *tpm_passthrough_create(QemuOpts *opts, const char *id)
> >          goto err_exit;
> >      }
> > 
> > -    tpm_pt->cancel_fd = tpm_passthrough_open_sysfs_cancel(tb);
> > +    tpm_pt->cancel_fd = tpm_passthrough_open_sysfs_cancel(tpm_pt);
> >      if (tpm_pt->cancel_fd < 0) {
> >          goto err_exit;
> >      }
> > @@ -416,6 +415,31 @@ err_exit:
> >      return NULL;
> >  }
> > 
> > +static TpmTypeOptions *tpm_passthrough_get_tpm_options(TPMBackend
> > *tb)
> > +{
> > +    TPMPassthruState *tpm_pt = TPM_PASSTHROUGH(tb);
> > +    TpmTypeOptions *options = NULL;
> > +    TPMPassthroughOptions *poptions = NULL;
> > +
> > +    poptions = g_new0(TPMPassthroughOptions, 1);
> > +
> > +    if (tpm_pt->options->has_path) {
> > +        poptions->has_path = true;
> > +        poptions->path = g_strdup(tpm_pt->options->path);
> > +    }
> > +
> > +    if (tpm_pt->options->has_cancel_path) {
> > +        poptions->has_cancel_path = true;
> > +        poptions->cancel_path = g_strdup(tpm_pt->options-
> > >cancel_path);
> > +    }
> > +
> That looks like a job for QAPI_CLONE.
Ok, I was not knowing this, Thanks for pointing it. I will fix.


- Amarnath