[{"id":1773698,"web_url":"http://patchwork.ozlabs.org/comment/1773698/","msgid":"","list_archive_url":null,"date":"2017-09-22T15:35:02","subject":"Re: [Qemu-devel] [PATCH v7 5/8] tmp backend: Add new api to read\n\tbackend TpmInfo","submitter":{"id":6442,"url":"http://patchwork.ozlabs.org/api/people/6442/","name":"Marc-André Lureau","email":"marcandre.lureau@gmail.com"},"content":"Hi\n\nOn Fri, Sep 22, 2017 at 2:33 PM, Amarnath Valluri\n wrote:\n> TPM configuration options are backend implementation details and shall not be\n> part of base TPMBackend object, and these shall not be accessed directly outside\n> of the class, hence added a new interface method, get_tpm_options() to\n> TPMDriverOps., which shall be implemented by the derived classes to return\n> configured tpm options.\n>\n> A new tpm backend api - tpm_backend_query_tpm() which uses _get_tpm_options() to\n> prepare TpmInfo.\n>\n> Signed-off-by: Amarnath Valluri \n> Reviewed-by: Stefan Berger \n> ---\n> backends/tpm.c | 22 +++++++++++++--\n> hmp.c | 7 ++---\n> hw/tpm/tpm_passthrough.c | 64 +++++++++++++++++++++++++++++++-------------\n> include/sysemu/tpm_backend.h | 25 +++++++++++++++--\n> tpm.c | 32 +---------------------\n> 5 files changed, 93 insertions(+), 57 deletions(-)\n>\n> diff --git a/backends/tpm.c b/backends/tpm.c\n> index c409a46..6ade9e4 100644\n> --- a/backends/tpm.c\n> +++ b/backends/tpm.c\n> @@ -138,6 +138,26 @@ TPMVersion tpm_backend_get_tpm_version(TPMBackend *s)\n> return k->ops->get_tpm_version(s);\n> }\n>\n> +TpmTypeOptions *tpm_backend_get_tpm_options(TPMBackend *s)\n> +{\n> + TPMBackendClass *k = TPM_BACKEND_GET_CLASS(s);\n> +\n> + assert(k->ops->get_tpm_options);\n> +\n> + return k->ops->get_tpm_options(s);\n> +}\n> +\n\nWhy make this public API?\n\n> +TPMInfo *tpm_backend_query_tpm(TPMBackend *s)\n> +{\n> + TPMInfo *info = g_new0(TPMInfo, 1);\n> +\n> + info->id = g_strdup(s->id);\n> + info->model = s->fe_model;\n> + info->options = tpm_backend_get_tpm_options(s);\n\nCallback could be called directly from here.\n\n> +\n> + return info;\n> +}\n> +\n> static bool tpm_backend_prop_get_opened(Object *obj, Error **errp)\n> {\n> TPMBackend *s = TPM_BACKEND(obj);\n> @@ -192,8 +212,6 @@ static void tpm_backend_instance_finalize(Object *obj)\n> TPMBackend *s = TPM_BACKEND(obj);\n>\n> g_free(s->id);\n> - g_free(s->path);\n> - g_free(s->cancel_path);\n> tpm_backend_thread_end(s);\n> }\n>\n> diff --git a/hmp.c b/hmp.c\n> index 0fb2bc7..cf62b2e 100644\n> --- a/hmp.c\n> +++ b/hmp.c\n> @@ -31,6 +31,7 @@\n> #include \"qapi/qmp/qerror.h\"\n> #include \"qapi/string-input-visitor.h\"\n> #include \"qapi/string-output-visitor.h\"\n> +#include \"qapi/util.h\"\n> #include \"qapi-visit.h\"\n> #include \"qom/object_interfaces.h\"\n> #include \"ui/console.h\"\n> @@ -1012,10 +1013,10 @@ void hmp_info_tpm(Monitor *mon, const QDict *qdict)\n> c, TpmModel_str(ti->model));\n>\n> monitor_printf(mon, \" \\\\ %s: type=%s\",\n> - ti->id, TpmTypeOptionsKind_str(ti->options->type));\n> + ti->id, TpmType_str(ti->options->type));\n>\n\nWhy this change? It is still a TpmTypeOptionsKind no?\n\n> switch (ti->options->type) {\n> - case TPM_TYPE_OPTIONS_KIND_PASSTHROUGH:\n> + case TPM_TYPE_PASSTHROUGH:\n> tpo = ti->options->u.passthrough.data;\n> monitor_printf(mon, \"%s%s%s%s\",\n> tpo->has_path ? \",path=\" : \"\",\n> @@ -1023,7 +1024,7 @@ void hmp_info_tpm(Monitor *mon, const QDict *qdict)\n> tpo->has_cancel_path ? \",cancel-path=\" : \"\",\n> tpo->has_cancel_path ? tpo->cancel_path : \"\");\n> break;\n> - case TPM_TYPE_OPTIONS_KIND__MAX:\n> + case TPM_TYPE__MAX:\n> break;\n> }\n> monitor_printf(mon, \"\\n\");\n> diff --git a/hw/tpm/tpm_passthrough.c b/hw/tpm/tpm_passthrough.c\n> index a0459a6..fb7dad8 100644\n> --- a/hw/tpm/tpm_passthrough.c\n> +++ b/hw/tpm/tpm_passthrough.c\n> @@ -49,7 +49,8 @@\n> struct TPMPassthruState {\n> TPMBackend parent;\n>\n> - char *tpm_dev;\n> + TPMPassthroughOptions *options;\n> + const char *tpm_dev;\n> int tpm_fd;\n> bool tpm_executing;\n> bool tpm_op_canceled;\n> @@ -308,15 +309,14 @@ static TPMVersion tpm_passthrough_get_tpm_version(TPMBackend *tb)\n> * in Documentation/ABI/stable/sysfs-class-tpm.\n> * From /dev/tpm0 create /sys/class/misc/tpm0/device/cancel\n> */\n> -static int tpm_passthrough_open_sysfs_cancel(TPMBackend *tb)\n> +static int tpm_passthrough_open_sysfs_cancel(TPMPassthruState *tpm_pt)\n\nI suspect this change could be done in an earlier/separate patch\n\n> {\n> - TPMPassthruState *tpm_pt = TPM_PASSTHROUGH(tb);\n> int fd = -1;\n> char *dev;\n> char path[PATH_MAX];\n>\n> - if (tb->cancel_path) {\n> - fd = qemu_open(tb->cancel_path, O_WRONLY);\n> + if (tpm_pt->options->cancel_path) {\n> + fd = qemu_open(tpm_pt->options->cancel_path, O_WRONLY);\n> if (fd < 0) {\n> error_report(\"Could not open TPM cancel path : %s\",\n> strerror(errno));\n> @@ -331,7 +331,7 @@ static int tpm_passthrough_open_sysfs_cancel(TPMBackend *tb)\n> dev) < sizeof(path)) {\n> fd = qemu_open(path, O_WRONLY);\n> if (fd >= 0) {\n> - tb->cancel_path = g_strdup(path);\n> + tpm_pt->options->cancel_path = g_strdup(path);\n> } else {\n> error_report(\"tpm_passthrough: Could not open TPM cancel \"\n> \"path %s : %s\", path, strerror(errno));\n> @@ -351,17 +351,18 @@ static int tpm_passthrough_handle_device_opts(QemuOpts *opts, TPMBackend *tb)\n> const char *value;\n>\n> value = qemu_opt_get(opts, \"cancel-path\");\n> - tb->cancel_path = g_strdup(value);\n> + if (value) {\n> + tpm_pt->options->cancel_path = g_strdup(value);\n> + tpm_pt->options->has_cancel_path = true;\n> + }\n>\n> value = qemu_opt_get(opts, \"path\");\n> - if (!value) {\n> - value = TPM_PASSTHROUGH_DEFAULT_DEVICE;\n> + if (value) {\n> + tpm_pt->options->has_path = true;\n> + tpm_pt->options->path = g_strdup(value);\n> }\n>\n> - tpm_pt->tpm_dev = g_strdup(value);\n> -\n> - tb->path = g_strdup(tpm_pt->tpm_dev);\n> -\n> + tpm_pt->tpm_dev = value ? value : TPM_PASSTHROUGH_DEFAULT_DEVICE;\n\nIsn't that duplicated with tpm_pt->options->path ? And different values...\n\n> tpm_pt->tpm_fd = qemu_open(tpm_pt->tpm_dev, O_RDWR);\n> if (tpm_pt->tpm_fd < 0) {\n> error_report(\"Cannot access TPM device using '%s': %s\",\n> @@ -382,10 +383,8 @@ static int tpm_passthrough_handle_device_opts(QemuOpts *opts, TPMBackend *tb)\n> tpm_pt->tpm_fd = -1;\n>\n> err_free_parameters:\n> - g_free(tb->path);\n> - tb->path = NULL;\n> -\n> - g_free(tpm_pt->tpm_dev);\n> + qapi_free_TPMPassthroughOptions(tpm_pt->options);\n> + tpm_pt->options = NULL;\n> tpm_pt->tpm_dev = NULL;\n>\n> return 1;\n> @@ -403,7 +402,7 @@ static TPMBackend *tpm_passthrough_create(QemuOpts *opts, const char *id)\n> goto err_exit;\n> }\n>\n> - tpm_pt->cancel_fd = tpm_passthrough_open_sysfs_cancel(tb);\n> + tpm_pt->cancel_fd = tpm_passthrough_open_sysfs_cancel(tpm_pt);\n> if (tpm_pt->cancel_fd < 0) {\n> goto err_exit;\n> }\n> @@ -416,6 +415,31 @@ err_exit:\n> return NULL;\n> }\n>\n> +static TpmTypeOptions *tpm_passthrough_get_tpm_options(TPMBackend *tb)\n> +{\n> + TPMPassthruState *tpm_pt = TPM_PASSTHROUGH(tb);\n> + TpmTypeOptions *options = NULL;\n> + TPMPassthroughOptions *poptions = NULL;\n> +\n> + poptions = g_new0(TPMPassthroughOptions, 1);\n> +\n> + if (tpm_pt->options->has_path) {\n> + poptions->has_path = true;\n> + poptions->path = g_strdup(tpm_pt->options->path);\n> + }\n> +\n> + if (tpm_pt->options->has_cancel_path) {\n> + poptions->has_cancel_path = true;\n> + poptions->cancel_path = g_strdup(tpm_pt->options->cancel_path);\n> + }\n> +\n\nThat looks like a job for QAPI_CLONE.\n\n> + options = g_new0(TpmTypeOptions, 1);\n> + options->type = TPM_TYPE_PASSTHROUGH;\n> + options->u.passthrough.data = poptions;\n> +\n> + return options;\n> +}\n> +\n> static const QemuOptDesc tpm_passthrough_cmdline_opts[] = {\n> TPM_STANDARD_CMDLINE_OPTS,\n> {\n> @@ -443,12 +467,14 @@ static const TPMDriverOps tpm_passthrough_driver = {\n> .get_tpm_established_flag = tpm_passthrough_get_tpm_established_flag,\n> .reset_tpm_established_flag = tpm_passthrough_reset_tpm_established_flag,\n> .get_tpm_version = tpm_passthrough_get_tpm_version,\n> + .get_tpm_options = tpm_passthrough_get_tpm_options,\n> };\n>\n> static void tpm_passthrough_inst_init(Object *obj)\n> {\n> TPMPassthruState *tpm_pt = TPM_PASSTHROUGH(obj);\n>\n> + tpm_pt->options = g_new0(TPMPassthroughOptions, 1);\n\nI don't see much point in allocating the struct. Could it just be an\nflat member instead?\n\n> tpm_pt->tpm_fd = -1;\n> tpm_pt->cancel_fd = -1;\n> }\n> @@ -461,7 +487,7 @@ static void tpm_passthrough_inst_finalize(Object *obj)\n>\n> qemu_close(tpm_pt->tpm_fd);\n> qemu_close(tpm_pt->cancel_fd);\n> - g_free(tpm_pt->tpm_dev);\n> + qapi_free_TPMPassthroughOptions(tpm_pt->options);\n> }\n>\n> static void tpm_passthrough_class_init(ObjectClass *klass, void *data)\n> diff --git a/include/sysemu/tpm_backend.h b/include/sysemu/tpm_backend.h\n> index 13311cf..809eec2 100644\n> --- a/include/sysemu/tpm_backend.h\n> +++ b/include/sysemu/tpm_backend.h\n> @@ -48,10 +48,9 @@ struct TPMBackend {\n> GThreadPool *thread_pool;\n> TPMRecvDataCB *recv_data_callback;\n>\n> + /* */\n> char *id;\n> enum TpmModel fe_model;\n> - char *path;\n> - char *cancel_path;\n>\n> QLIST_ENTRY(TPMBackend) list;\n> };\n> @@ -97,6 +96,8 @@ struct TPMDriverOps {\n> int (*reset_tpm_established_flag)(TPMBackend *t, uint8_t locty);\n>\n> TPMVersion (*get_tpm_version)(TPMBackend *t);\n> +\n> + TpmTypeOptions *(*get_tpm_options)(TPMBackend *t);\n> };\n>\n>\n> @@ -215,6 +216,26 @@ void tpm_backend_open(TPMBackend *s, Error **errp);\n> */\n> TPMVersion tpm_backend_get_tpm_version(TPMBackend *s);\n>\n> +/**\n> + * tpm_backend_get_tpm_options:\n> + * @s: the backend\n> + *\n> + * Get the backend configuration options\n> + *\n> + * Returns newly allocated TpmTypeOptions\n> + */\n> +TpmTypeOptions *tpm_backend_get_tpm_options(TPMBackend *s);\n> +\n> +/**\n> + * tpm_backend_query_tpm:\n> + * @s: the backend\n> + *\n> + * Query backend tpm info\n> + *\n> + * Returns newly allocated TPMInfo\n> + */\n> +TPMInfo *tpm_backend_query_tpm(TPMBackend *s);\n> +\n> TPMBackend *qemu_find_tpm(const char *id);\n>\n> const TPMDriverOps *tpm_get_backend_driver(const char *type);\n> diff --git a/tpm.c b/tpm.c\n> index db14849..3b8c7ed 100644\n> --- a/tpm.c\n> +++ b/tpm.c\n> @@ -202,36 +202,6 @@ static const TPMDriverOps *tpm_driver_find_by_type(enum TpmType type)\n> return be_drivers[type];\n> }\n>\n> -static TPMInfo *qmp_query_tpm_inst(TPMBackend *drv)\n> -{\n> - TPMInfo *res = g_new0(TPMInfo, 1);\n> - TPMPassthroughOptions *tpo;\n> -\n> - res->id = g_strdup(drv->id);\n> - res->model = drv->fe_model;\n> - res->options = g_new0(TpmTypeOptions, 1);\n> -\n> - switch (tpm_backend_get_type(drv)) {\n> - case TPM_TYPE_PASSTHROUGH:\n> - res->options->type = TPM_TYPE_OPTIONS_KIND_PASSTHROUGH;\n> - tpo = g_new0(TPMPassthroughOptions, 1);\n> - res->options->u.passthrough.data = tpo;\n> - if (drv->path) {\n> - tpo->path = g_strdup(drv->path);\n> - tpo->has_path = true;\n> - }\n> - if (drv->cancel_path) {\n> - tpo->cancel_path = g_strdup(drv->cancel_path);\n> - tpo->has_cancel_path = true;\n> - }\n> - break;\n> - case TPM_TYPE__MAX:\n> - break;\n> - }\n> -\n> - return res;\n> -}\n> -\n> /*\n> * Walk the list of active TPM backends and collect information about them\n> * following the schema description in qapi-schema.json.\n> @@ -246,7 +216,7 @@ TPMInfoList *qmp_query_tpm(Error **errp)\n> continue;\n> }\n> info = g_new0(TPMInfoList, 1);\n> - info->value = qmp_query_tpm_inst(drv);\n> + info->value = tpm_backend_query_tpm(drv);\n>\n> if (!cur_item) {\n> head = cur_item = info;\n> --\n> 2.7.4\n>","headers":{"Return-Path":"","X-Original-To":"incoming@patchwork.ozlabs.org","Delivered-To":"patchwork-incoming@bilbo.ozlabs.org","Authentication-Results":["ozlabs.org;\n\tspf=pass (mailfrom) smtp.mailfrom=nongnu.org\n\t(client-ip=2001:4830:134:3::11; helo=lists.gnu.org;\n\tenvelope-from=qemu-devel-bounces+incoming=patchwork.ozlabs.org@nongnu.org;\n\treceiver=)","ozlabs.org;\n\tdkim=fail reason=\"signature verification failed\" (2048-bit key;\n\tunprotected) header.d=gmail.com header.i=@gmail.com\n\theader.b=\"hcvyux2d\"; dkim-atps=neutral"],"Received":["from lists.gnu.org (lists.gnu.org [IPv6:2001:4830:134:3::11])\n\t(using TLSv1 with cipher AES256-SHA (256/256 bits))\n\t(No client certificate requested)\n\tby ozlabs.org (Postfix) with ESMTPS id 3xzHcm2dQdz9t3h\n\tfor ;\n\tSat, 23 Sep 2017 01:35:46 +1000 (AEST)","from localhost ([::1]:59540 helo=lists.gnu.org)\n\tby lists.gnu.org with esmtp (Exim 4.71) (envelope-from\n\t)\n\tid 1dvPzT-0001Uu-3n\n\tfor incoming@patchwork.ozlabs.org; Fri, 22 Sep 2017 11:35:43 -0400","from eggs.gnu.org ([2001:4830:134:3::10]:38714)\n\tby lists.gnu.org with esmtp (Exim 4.71)\n\t(envelope-from ) id 1dvPyw-0001Pt-N0\n\tfor qemu-devel@nongnu.org; Fri, 22 Sep 2017 11:35:12 -0400","from Debian-exim by eggs.gnu.org with spam-scanned (Exim 4.71)\n\t(envelope-from ) id 1dvPyu-0000Lx-RO\n\tfor qemu-devel@nongnu.org; Fri, 22 Sep 2017 11:35:10 -0400","from mail-io0-x241.google.com ([2607:f8b0:4001:c06::241]:37385)\n\tby eggs.gnu.org with esmtps (TLS1.0:RSA_AES_128_CBC_SHA1:16)\n\t(Exim 4.71) (envelope-from )\n\tid 1dvPyu-0000JH-LF\n\tfor qemu-devel@nongnu.org; Fri, 22 Sep 2017 11:35:08 -0400","by mail-io0-x241.google.com with SMTP id 93so203976iol.4\n\tfor ; Fri, 22 Sep 2017 08:35:07 -0700 (PDT)","by 10.58.17.173 with HTTP; Fri, 22 Sep 2017 08:35:02 -0700 (PDT)"],"DKIM-Signature":"v=1; a=rsa-sha256; c=relaxed/relaxed; d=gmail.com; s=20161025;\n\th=mime-version:in-reply-to:references:from:date:message-id:subject:to\n\t:cc:content-transfer-encoding;\n\tbh=P3jwsU3UAk7eegVYpHnJ0QUVqRsfiSBtH49p88V22lo=;\n\tb=hcvyux2dL/0WjENU9mtwvNjU88xVqpLiF4yLG/x/4Bf0dh2SmkZ5ppjqMBPpokHi6W\n\tB7s2GeIhxUBTLz/Ai3KC432d8NANeqUnJsA5bBSmcTTDvJv8CrGbOilzSkMxBU1Tp8Mr\n\t7wKve+e+alzkp/4PSqHbNZ+4PR7yibWa1xx42pD6N7LhL43xquTn51Lav+92cQ02IKRE\n\t6/HqDCH3fFgqeSYQ72jYey/bv86OTN8X8fguAUqYpSo2CBUcE6Nqu0B19gpDnNs4sixN\n\tiOwA+lr/YWIdJ6YsbwjNq0c7MTk9sawa7bKv5/EFpKMRHjJ4XNsQjNudHQpIqqey6pHz\n\tsA6g==","X-Google-DKIM-Signature":"v=1; a=rsa-sha256; c=relaxed/relaxed;\n\td=1e100.net; s=20161025;\n\th=x-gm-message-state:mime-version:in-reply-to:references:from:date\n\t:message-id:subject:to:cc:content-transfer-encoding;\n\tbh=P3jwsU3UAk7eegVYpHnJ0QUVqRsfiSBtH49p88V22lo=;\n\tb=snEOfSvwxl2SFSjLIzR3AJAwA879jLr14Ku/+u32Y96svrp7RgYaDQZk0yH/unCmoE\n\tUPXE9Tj8VOeLYnKPBmlS/AvYMFLYFYNF+XO33KWJpL8SwxyTR1cTNB6fmWTPUqyK3Gvp\n\tv/yn3u6EPawGm1uiKAqUswQcvNW8FTuGZRxJPEkfnkhX4XptMPI31tmmi33fCDzLavjE\n\tskbSDDLk+A1Th9q9TtZzyh56GU4HdMl2rqAd88l8a/zYYYXM6AO2Nl7WPCI9ediw6+qn\n\teogspL0OAwcBQWFzrYfqPA6hAMolOBAqGx4bKYmCR46+Mj9HYXZHvrblgTWDar29m5TX\n\tCTKA==","X-Gm-Message-State":"AHPjjUiWVgdra4hFJcqrn4fPUuojeom9raHcZvmqqimGw1i9ERgSN7bF\n\tRqFVpPQHLnj+IzqjZyoYrI+t0GgVv5/WgGhDjOM=","X-Google-Smtp-Source":"AOwi7QCLY63ELIQRHMI6beES0IXWQ1zLbZy76KBDmvXluj8YX81g8xtO1AqDfPPgfDWS7Cp7o0Czu1z/fMQYtxH8QWM=","X-Received":"by 10.202.96.131 with SMTP id u125mr6727186oib.304.1506094503834;\n\tFri, 22 Sep 2017 08:35:03 -0700 (PDT)","MIME-Version":"1.0","In-Reply-To":"<1506083624-20621-6-git-send-email-amarnath.valluri@intel.com>","References":"<1506083624-20621-1-git-send-email-amarnath.valluri@intel.com>\n\t<1506083624-20621-6-git-send-email-amarnath.valluri@intel.com>","From":"=?utf-8?q?Marc-Andr=C3=A9_Lureau?= ","Date":"Fri, 22 Sep 2017 17:35:02 +0200","Message-ID":"","To":"Amarnath Valluri ","Content-Type":"text/plain; charset=\"UTF-8\"","Content-Transfer-Encoding":"quoted-printable","X-detected-operating-system":"by eggs.gnu.org: Genre and OS details not\n\trecognized.","X-Received-From":"2607:f8b0:4001:c06::241","Subject":"Re: [Qemu-devel] [PATCH v7 5/8] tmp backend: Add new api to read\n\tbackend TpmInfo","X-BeenThere":"qemu-devel@nongnu.org","X-Mailman-Version":"2.1.21","Precedence":"list","List-Id":"","List-Unsubscribe":",\n\t","List-Archive":"","List-Post":"","List-Help":"","List-Subscribe":",\n\t","Cc":"QEMU ,\n\t\"Dr. David Alan Gilbert\" ,\n\tStefan Berger ","Errors-To":"qemu-devel-bounces+incoming=patchwork.ozlabs.org@nongnu.org","Sender":"\"Qemu-devel\"\n\t"}},{"id":1774683,"web_url":"http://patchwork.ozlabs.org/comment/1774683/","msgid":"<1506340994.5843.84.camel@intel.com>","list_archive_url":null,"date":"2017-09-25T12:01:51","subject":"Re: [Qemu-devel] [PATCH v7 5/8] tmp backend: Add new api to read\n\tbackend TpmInfo","submitter":{"id":71320,"url":"http://patchwork.ozlabs.org/api/people/71320/","name":"Valluri, Amarnath","email":"amarnath.valluri@intel.com"},"content":"On Fri, 2017-09-22 at 17:35 +0200, Marc-André Lureau wrote:\r\n> Hi\r\n> \r\n> On Fri, Sep 22, 2017 at 2:33 PM, Amarnath Valluri\r\n> wrote:\r\n> > \r\n> > TPM configuration options are backend implementation details and\r\n> > shall not be\r\n> > part of base TPMBackend object, and these shall not be accessed\r\n> > directly outside\r\n> > of the class, hence added a new interface method, get_tpm_options()\r\n> > to\r\n> > TPMDriverOps., which shall be implemented by the derived classes to\r\n> > return\r\n> > configured tpm options.\r\n> > \r\n> > A new tpm backend api - tpm_backend_query_tpm() which uses\r\n> > _get_tpm_options() to\r\n> > prepare TpmInfo.\r\n> > \r\n> > Signed-off-by: Amarnath Valluri \r\n> > Reviewed-by: Stefan Berger \r\n> > ---\r\n> >  backends/tpm.c               | 22 +++++++++++++--\r\n> >  hmp.c                        |  7 ++---\r\n> >  hw/tpm/tpm_passthrough.c     | 64 +++++++++++++++++++++++++++++++-\r\n> > ------------\r\n> >  include/sysemu/tpm_backend.h | 25 +++++++++++++++--\r\n> >  tpm.c                        | 32 +---------------------\r\n> >  5 files changed, 93 insertions(+), 57 deletions(-)\r\n> > \r\n> > diff --git a/backends/tpm.c b/backends/tpm.c\r\n> > index c409a46..6ade9e4 100644\r\n> > --- a/backends/tpm.c\r\n> > +++ b/backends/tpm.c\r\n> > @@ -138,6 +138,26 @@ TPMVersion\r\n> > tpm_backend_get_tpm_version(TPMBackend *s)\r\n> >      return k->ops->get_tpm_version(s);\r\n> >  }\r\n> > \r\n> > +TpmTypeOptions *tpm_backend_get_tpm_options(TPMBackend *s)\r\n> > +{\r\n> > +    TPMBackendClass *k = TPM_BACKEND_GET_CLASS(s);\r\n> > +\r\n> > +    assert(k->ops->get_tpm_options);\r\n> > +\r\n> > +    return k->ops->get_tpm_options(s);\r\n> > +}\r\n> > +\r\n> Why make this public API?\r\nYes, I agree no need of this API, i will remove this\r\n> \r\n> > \r\n> > +TPMInfo *tpm_backend_query_tpm(TPMBackend *s)\r\n> > +{\r\n> > +    TPMInfo *info = g_new0(TPMInfo, 1);\r\n> > +\r\n> > +    info->id = g_strdup(s->id);\r\n> > +    info->model = s->fe_model;\r\n> > +    info->options = tpm_backend_get_tpm_options(s);\r\n> Callback could be called directly from here.\r\n> \r\n> > \r\n> > +\r\n> > +    return info;\r\n> > +}\r\n> > +\r\n> >  static bool tpm_backend_prop_get_opened(Object *obj, Error **errp)\r\n> >  {\r\n> >      TPMBackend *s = TPM_BACKEND(obj);\r\n> > @@ -192,8 +212,6 @@ static void\r\n> > tpm_backend_instance_finalize(Object *obj)\r\n> >      TPMBackend *s = TPM_BACKEND(obj);\r\n> > \r\n> >      g_free(s->id);\r\n> > -    g_free(s->path);\r\n> > -    g_free(s->cancel_path);\r\n> >      tpm_backend_thread_end(s);\r\n> >  }\r\n> > \r\n> > diff --git a/hmp.c b/hmp.c\r\n> > index 0fb2bc7..cf62b2e 100644\r\n> > --- a/hmp.c\r\n> > +++ b/hmp.c\r\n> > @@ -31,6 +31,7 @@\r\n> >  #include \"qapi/qmp/qerror.h\"\r\n> >  #include \"qapi/string-input-visitor.h\"\r\n> >  #include \"qapi/string-output-visitor.h\"\r\n> > +#include \"qapi/util.h\"\r\n> >  #include \"qapi-visit.h\"\r\n> >  #include \"qom/object_interfaces.h\"\r\n> >  #include \"ui/console.h\"\r\n> > @@ -1012,10 +1013,10 @@ void hmp_info_tpm(Monitor *mon, const QDict\r\n> > *qdict)\r\n> >                         c, TpmModel_str(ti->model));\r\n> > \r\n> >          monitor_printf(mon, \"  \\\\ %s: type=%s\",\r\n> > -                       ti->id, TpmTypeOptionsKind_str(ti->options-\r\n> > >type));\r\n> > +                       ti->id, TpmType_str(ti->options->type));\r\n> > \r\n> Why this change? It is still a TpmTypeOptionsKind no?\r\nThis is was issue with my rebase, i will fix.\r\n> \r\n> > \r\n> >          switch (ti->options->type) {\r\n> > -        case TPM_TYPE_OPTIONS_KIND_PASSTHROUGH:\r\n> > +        case TPM_TYPE_PASSTHROUGH:\r\n> >              tpo = ti->options->u.passthrough.data;\r\n> >              monitor_printf(mon, \"%s%s%s%s\",\r\n> >                             tpo->has_path ? \",path=\" : \"\",\r\n> > @@ -1023,7 +1024,7 @@ void hmp_info_tpm(Monitor *mon, const QDict\r\n> > *qdict)\r\n> >                             tpo->has_cancel_path ? \",cancel-path=\"\r\n> > : \"\",\r\n> >                             tpo->has_cancel_path ? tpo->cancel_path \r\n> > : \"\");\r\n> >              break;\r\n> > -        case TPM_TYPE_OPTIONS_KIND__MAX:\r\n> > +        case TPM_TYPE__MAX:\r\n> >              break;\r\n> >          }\r\n> >          monitor_printf(mon, \"\\n\");\r\n> > diff --git a/hw/tpm/tpm_passthrough.c b/hw/tpm/tpm_passthrough.c\r\n> > index a0459a6..fb7dad8 100644\r\n> > --- a/hw/tpm/tpm_passthrough.c\r\n> > +++ b/hw/tpm/tpm_passthrough.c\r\n> > @@ -49,7 +49,8 @@\r\n> >  struct TPMPassthruState {\r\n> >      TPMBackend parent;\r\n> > \r\n> > -    char *tpm_dev;\r\n> > +    TPMPassthroughOptions *options;\r\n> > +    const char *tpm_dev;\r\n> >      int tpm_fd;\r\n> >      bool tpm_executing;\r\n> >      bool tpm_op_canceled;\r\n> > @@ -308,15 +309,14 @@ static TPMVersion\r\n> > tpm_passthrough_get_tpm_version(TPMBackend *tb)\r\n> >   * in Documentation/ABI/stable/sysfs-class-tpm.\r\n> >   * From /dev/tpm0 create /sys/class/misc/tpm0/device/cancel\r\n> >   */\r\n> > -static int tpm_passthrough_open_sysfs_cancel(TPMBackend *tb)\r\n> > +static int tpm_passthrough_open_sysfs_cancel(TPMPassthruState\r\n> > *tpm_pt)\r\n> I suspect this change could be done in an earlier/separate patch\r\nI feel this change is better part of this commit as this is side effect\r\nof rearragend tpm options, now these are part of TPMPasstrhuState so\r\n_open_sysfs_cancel() need not work on TPMBackend.\r\n> \r\n> > \r\n> >  {\r\n> > -    TPMPassthruState *tpm_pt = TPM_PASSTHROUGH(tb);\r\n> >      int fd = -1;\r\n> >      char *dev;\r\n> >      char path[PATH_MAX];\r\n> > \r\n> > -    if (tb->cancel_path) {\r\n> > -        fd = qemu_open(tb->cancel_path, O_WRONLY);\r\n> > +    if (tpm_pt->options->cancel_path) {\r\n> > +        fd = qemu_open(tpm_pt->options->cancel_path, O_WRONLY);\r\n> >          if (fd < 0) {\r\n> >              error_report(\"Could not open TPM cancel path : %s\",\r\n> >                           strerror(errno));\r\n> > @@ -331,7 +331,7 @@ static int\r\n> > tpm_passthrough_open_sysfs_cancel(TPMBackend *tb)\r\n> >                       dev) < sizeof(path)) {\r\n> >              fd = qemu_open(path, O_WRONLY);\r\n> >              if (fd >= 0) {\r\n> > -                tb->cancel_path = g_strdup(path);\r\n> > +                tpm_pt->options->cancel_path = g_strdup(path);\r\n> >              } else {\r\n> >                  error_report(\"tpm_passthrough: Could not open TPM\r\n> > cancel \"\r\n> >                               \"path %s : %s\", path,\r\n> > strerror(errno));\r\n> > @@ -351,17 +351,18 @@ static int\r\n> > tpm_passthrough_handle_device_opts(QemuOpts *opts, TPMBackend *tb)\r\n> >      const char *value;\r\n> > \r\n> >      value = qemu_opt_get(opts, \"cancel-path\");\r\n> > -    tb->cancel_path = g_strdup(value);\r\n> > +    if (value) {\r\n> > +        tpm_pt->options->cancel_path = g_strdup(value);\r\n> > +        tpm_pt->options->has_cancel_path = true;\r\n> > +    }\r\n> > \r\n> >      value = qemu_opt_get(opts, \"path\");\r\n> > -    if (!value) {\r\n> > -        value = TPM_PASSTHROUGH_DEFAULT_DEVICE;\r\n> > +    if (value) {\r\n> > +        tpm_pt->options->has_path = true;\r\n> > +        tpm_pt->options->path = g_strdup(value);\r\n> >      }\r\n> > \r\n> > -    tpm_pt->tpm_dev = g_strdup(value);\r\n> > -\r\n> > -    tb->path = g_strdup(tpm_pt->tpm_dev);\r\n> > -\r\n> > +    tpm_pt->tpm_dev = value ? value :\r\n> > TPM_PASSTHROUGH_DEFAULT_DEVICE;\r\n> Isn't that duplicated with tpm_pt->options->path ? And different\r\n> values...\r\nThis is intentional, options->path holds the configuration value, where\r\nas tpm_dev points the real device path used.\r\n> \r\n> > \r\n> >      tpm_pt->tpm_fd = qemu_open(tpm_pt->tpm_dev, O_RDWR);\r\n> >      if (tpm_pt->tpm_fd < 0) {\r\n> >          error_report(\"Cannot access TPM device using '%s': %s\",\r\n> > @@ -382,10 +383,8 @@ static int\r\n> > tpm_passthrough_handle_device_opts(QemuOpts *opts, TPMBackend *tb)\r\n> >      tpm_pt->tpm_fd = -1;\r\n> > \r\n> >   err_free_parameters:\r\n> > -    g_free(tb->path);\r\n> > -    tb->path = NULL;\r\n> > -\r\n> > -    g_free(tpm_pt->tpm_dev);\r\n> > +    qapi_free_TPMPassthroughOptions(tpm_pt->options);\r\n> > +    tpm_pt->options = NULL;\r\n> >      tpm_pt->tpm_dev = NULL;\r\n> > \r\n> >      return 1;\r\n> > @@ -403,7 +402,7 @@ static TPMBackend\r\n> > *tpm_passthrough_create(QemuOpts *opts, const char *id)\r\n> >          goto err_exit;\r\n> >      }\r\n> > \r\n> > -    tpm_pt->cancel_fd = tpm_passthrough_open_sysfs_cancel(tb);\r\n> > +    tpm_pt->cancel_fd = tpm_passthrough_open_sysfs_cancel(tpm_pt);\r\n> >      if (tpm_pt->cancel_fd < 0) {\r\n> >          goto err_exit;\r\n> >      }\r\n> > @@ -416,6 +415,31 @@ err_exit:\r\n> >      return NULL;\r\n> >  }\r\n> > \r\n> > +static TpmTypeOptions *tpm_passthrough_get_tpm_options(TPMBackend\r\n> > *tb)\r\n> > +{\r\n> > +    TPMPassthruState *tpm_pt = TPM_PASSTHROUGH(tb);\r\n> > +    TpmTypeOptions *options = NULL;\r\n> > +    TPMPassthroughOptions *poptions = NULL;\r\n> > +\r\n> > +    poptions = g_new0(TPMPassthroughOptions, 1);\r\n> > +\r\n> > +    if (tpm_pt->options->has_path) {\r\n> > +        poptions->has_path = true;\r\n> > +        poptions->path = g_strdup(tpm_pt->options->path);\r\n> > +    }\r\n> > +\r\n> > +    if (tpm_pt->options->has_cancel_path) {\r\n> > +        poptions->has_cancel_path = true;\r\n> > +        poptions->cancel_path = g_strdup(tpm_pt->options-\r\n> > >cancel_path);\r\n> > +    }\r\n> > +\r\n> That looks like a job for QAPI_CLONE.\r\nOk, I was not knowing this, Thanks for pointing it. I will fix.\r\n\r\n\r\n- Amarnath","headers":{"Return-Path":"","X-Original-To":"incoming@patchwork.ozlabs.org","Delivered-To":"patchwork-incoming@bilbo.ozlabs.org","Authentication-Results":"ozlabs.org;\n\tspf=pass (mailfrom) smtp.mailfrom=nongnu.org\n\t(client-ip=2001:4830:134:3::11; helo=lists.gnu.org;\n\tenvelope-from=qemu-devel-bounces+incoming=patchwork.ozlabs.org@nongnu.org;\n\treceiver=)","Received":["from lists.gnu.org (lists.gnu.org [IPv6:2001:4830:134:3::11])\n\t(using TLSv1 with cipher AES256-SHA (256/256 bits))\n\t(No client certificate requested)\n\tby ozlabs.org (Postfix) with ESMTPS id 3y12lR0pbBz9sRq\n\tfor ;\n\tMon, 25 Sep 2017 22:02:38 +1000 (AEST)","from localhost ([::1]:42168 helo=lists.gnu.org)\n\tby lists.gnu.org with esmtp (Exim 4.71) (envelope-from\n\t)\n\tid 1dwS5r-0004ar-7T\n\tfor incoming@patchwork.ozlabs.org; Mon, 25 Sep 2017 08:02:35 -0400","from eggs.gnu.org ([2001:4830:134:3::10]:38409)\n\tby lists.gnu.org with esmtp (Exim 4.71)\n\t(envelope-from ) id 1dwS5N-0004VC-W8\n\tfor qemu-devel@nongnu.org; Mon, 25 Sep 2017 08:02:14 -0400","from Debian-exim by eggs.gnu.org with spam-scanned (Exim 4.71)\n\t(envelope-from ) id 1dwS5H-0000Lp-HQ\n\tfor qemu-devel@nongnu.org; Mon, 25 Sep 2017 08:02:06 -0400","from mga07.intel.com ([134.134.136.100]:59799)\n\tby eggs.gnu.org with esmtps (TLS1.0:DHE_RSA_AES_256_CBC_SHA1:32)\n\t(Exim 4.71) (envelope-from )\n\tid 1dwS5H-0000In-4i\n\tfor qemu-devel@nongnu.org; Mon, 25 Sep 2017 08:01:59 -0400","from fmsmga003.fm.intel.com ([10.253.24.29])\n\tby orsmga105.jf.intel.com with ESMTP; 25 Sep 2017 05:01:53 -0700","from irsmsx108.ger.corp.intel.com ([163.33.3.3])\n\tby FMSMGA003.fm.intel.com with ESMTP; 25 Sep 2017 05:01:52 -0700","from irsmsx110.ger.corp.intel.com ([169.254.15.8]) by\n\tIRSMSX108.ger.corp.intel.com ([169.254.11.167]) with mapi id\n\t14.03.0319.002; Mon, 25 Sep 2017 13:01:52 +0100"],"X-ExtLoop1":"1","X-IronPort-AV":"E=Sophos;i=\"5.42,436,1500966000\"; d=\"scan'208\";a=\"903584184\"","From":"\"Valluri, Amarnath\" ","To":"\"marcandre.lureau@gmail.com\" ","Thread-Topic":"[Qemu-devel][PATCH v7 5/8] tmp backend: Add new api to read\n\tbackend TpmInfo","Thread-Index":"AQHTM573kOiYp0a/P0KvXqDBc+ne+6LA+LQAgAR70gA=","Date":"Mon, 25 Sep 2017 12:01:51 +0000","Message-ID":"<1506340994.5843.84.camel@intel.com>","References":"<1506083624-20621-1-git-send-email-amarnath.valluri@intel.com>\n\t<1506083624-20621-6-git-send-email-amarnath.valluri@intel.com>\n\t","In-Reply-To":"","Accept-Language":"en-US","Content-Language":"en-US","X-MS-Has-Attach":"","X-MS-TNEF-Correlator":"","x-originating-ip":"[10.237.68.147]","Content-Type":"text/plain; charset=\"utf-8\"","Content-ID":"","Content-Transfer-Encoding":"base64","MIME-Version":"1.0","X-detected-operating-system":"by eggs.gnu.org: Genre and OS details not\n\trecognized.","X-Received-From":"134.134.136.100","Subject":"Re: [Qemu-devel] [PATCH v7 5/8] tmp backend: Add new api to read\n\tbackend TpmInfo","X-BeenThere":"qemu-devel@nongnu.org","X-Mailman-Version":"2.1.21","Precedence":"list","List-Id":"","List-Unsubscribe":",\n\t","List-Archive":"","List-Post":"","List-Help":"","List-Subscribe":",\n\t","Cc":"\"qemu-devel@nongnu.org\" ,\n\t\"dgilbert@redhat.com\" ,\n\t\"stefanb@linux.vnet.ibm.com\" ","Errors-To":"qemu-devel-bounces+incoming=patchwork.ozlabs.org@nongnu.org","Sender":"\"Qemu-devel\"\n\t"}}]