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.vall...@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.vall...@intel.com>
> > Reviewed-by: Stefan Berger <stef...@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
