Hi
On Tue, Apr 4, 2017 at 12:32 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 tpm backend api,
> tpm_backend_get_tpm_options()
> to read the backend configured options.
>
> Added new method, get_tpm_options() to TPMDriverOps., which shall be
> implemented
> by the derived classes to return configured tpm options.
>
> Made TPMPassthroughOptions type inherited from newly defined TPMOptions
> base type.
> The TPMOptions base type is intentionally left empty and supposed to be
> inherited by all backend implementations to define their tpm configuration
> options.
>
>
General idea seems good to me
> Signed-off-by: Amarnath Valluri <amarnath.vall...@intel.com>
> ---
> backends/tpm.c | 12 ++++++----
> hw/tpm/tpm_passthrough.c | 55
> +++++++++++++++++++++++++++++++++++---------
> include/sysemu/tpm_backend.h | 15 ++++++++++--
> qapi-schema.json | 14 +++++++++--
> tpm.c | 13 ++---------
> 5 files changed, 78 insertions(+), 31 deletions(-)
>
> diff --git a/backends/tpm.c b/backends/tpm.c
> index 0bdc5af..98aafd8 100644
> --- a/backends/tpm.c
> +++ b/backends/tpm.c
> @@ -153,6 +153,13 @@ TPMVersion tpm_backend_get_tpm_version(TPMBackend *s)
> return k->ops->get_tpm_version(s);
> }
>
> +TPMOptions *tpm_backend_get_tpm_options(TPMBackend *s)
> +{
> + TPMBackendClass *k = TPM_BACKEND_GET_CLASS(s);
> +
> + return k->ops->get_tpm_options ? k->ops->get_tpm_options(s) : NULL;
> +}
>
Probably can be made mandatory instead.
static bool tpm_backend_prop_get_opened(Object *obj, Error **errp)
> {
> TPMBackend *s = TPM_BACKEND(obj);
> @@ -205,9 +212,6 @@ static void tpm_backend_instance_init(Object *obj)
> s->tpm_state = NULL;
> s->thread_pool = NULL;
> s->recv_data_callback = NULL;
> - s->path = NULL;
> - s->cancel_path = NULL;
> -
> }
>
> static void tpm_backend_instance_finalize(Object *obj)
> @@ -215,8 +219,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/hw/tpm/tpm_passthrough.c b/hw/tpm/tpm_passthrough.c
> index 5b3bf1c..fce1163 100644
> --- a/hw/tpm/tpm_passthrough.c
> +++ b/hw/tpm/tpm_passthrough.c
> @@ -50,6 +50,7 @@
> struct TPMPassthruState {
> TPMBackend parent;
>
> + TPMPassthroughOptions ops;
> char *tpm_dev;
> int tpm_fd;
> bool tpm_executing;
> @@ -314,15 +315,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)
> {
> - 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->ops.cancel_path) {
> + fd = qemu_open(tpm_pt->ops.cancel_path, O_WRONLY);
> if (fd < 0) {
> error_report("Could not open TPM cancel path : %s",
> strerror(errno));
> @@ -337,7 +337,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->ops.cancel_path = g_strdup(path);
> } else {
> error_report("tpm_passthrough: Could not open TPM cancel "
> "path %s : %s", path, strerror(errno));
> @@ -357,17 +357,24 @@ 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->ops.cancel_path = g_strdup(value);
> + tpm_pt->ops.has_cancel_path = true;
> + } else {
> + tpm_pt->ops.has_cancel_path = false;
> + }
>
> value = qemu_opt_get(opts, "path");
> if (!value) {
> value = TPM_PASSTHROUGH_DEFAULT_DEVICE;
> + tpm_pt->ops.has_path = false;
> + } else {
> + tpm_pt->ops.has_path = true;
> }
>
> + tpm_pt->ops.path = g_strdup(value);
> tpm_pt->tpm_dev = g_strdup(value);
>
> - tb->path = g_strdup(tpm_pt->tpm_dev);
> -
> 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",
> @@ -388,8 +395,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->ops.path);
> + tpm_pt->ops.path = NULL;
>
> g_free(tpm_pt->tpm_dev);
> tpm_pt->tpm_dev = NULL;
> @@ -409,7 +416,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;
> }
> @@ -430,9 +437,34 @@ static void tpm_passthrough_destroy(TPMBackend *tb)
>
> qemu_close(tpm_pt->tpm_fd);
> qemu_close(tpm_pt->cancel_fd);
> +
> + g_free(tpm_pt->ops.path);
> + g_free(tpm_pt->ops.cancel_path);
>
hmm, if it was allocated instead, you could use
qapi_free_TPMPassthroughOptions().
I think destroy() should be replaced by object finalizer.
g_free(tpm_pt->tpm_dev);
> }
>
> +static TPMOptions *tpm_passthrough_get_tpm_options(TPMBackend *tb)
> +{
> + TPMPassthruState *tpm_pt = TPM_PASSTHROUGH(tb);
> + TPMPassthroughOptions *ops = g_new0(TPMPassthroughOptions, 1);
> +
> + if (!ops) {
> + return NULL;
> + }
> +
> + if (tpm_pt->ops.has_path) {
> + ops->has_path = true;
> + ops->path = g_strdup(tpm_pt->ops.path);
> + }
> +
> + if (tpm_pt->ops.has_cancel_path) {
> + ops->has_cancel_path = true;
> + ops->cancel_path = g_strdup(tpm_pt->ops.cancel_path);
> + }
> +
> + return (TPMOptions *)ops;
> +}
> +
> static const QemuOptDesc tpm_passthrough_cmdline_opts[] = {
> TPM_STANDARD_CMDLINE_OPTS,
> {
> @@ -461,6 +493,7 @@ static const TPMDriverOps tpm_passthrough_driver = {
> .get_tpm_established_flag = tpm_passthrough_get_tpm_established_flag,
> .reset_tpm_established_flag =
> tpm_passthrough_reset_tpm_established_flag,
> .get_tpm_version = tpm_passthrough_get_tpm_version,
> + .get_tpm_options = tpm_passthrough_get_tpm_options,
> };
>
> static void tpm_passthrough_inst_init(Object *obj)
> diff --git a/include/sysemu/tpm_backend.h b/include/sysemu/tpm_backend.h
> index 98b6112..82348a3 100644
> --- a/include/sysemu/tpm_backend.h
> +++ b/include/sysemu/tpm_backend.h
> @@ -41,10 +41,9 @@ struct TPMBackend {
> GThreadPool *thread_pool;
> TPMRecvDataCB *recv_data_callback;
>
> + /* <public> */
> char *id;
> enum TpmModel fe_model;
> - char *path;
> - char *cancel_path;
>
> QLIST_ENTRY(TPMBackend) list;
> };
> @@ -81,6 +80,8 @@ struct TPMDriverOps {
> int (*reset_tpm_established_flag)(TPMBackend *t, uint8_t locty);
>
> TPMVersion (*get_tpm_version)(TPMBackend *t);
> +
> + TPMOptions* (*get_tpm_options)(TPMBackend *t);
> };
>
>
> @@ -213,6 +214,16 @@ void tpm_backend_open(TPMBackend *s, Error **errp);
> */
> TPMVersion tpm_backend_get_tpm_version(TPMBackend *s);
>
> +/**
> + * tpm_backend_get_tpm_options:
> + * @s: the backend
> + *
> + * Get the backend configuration options
> + *
> + * Returns newly allocated TPMOptions
> + */
> +TPMOptions *tpm_backend_get_tpm_options(TPMBackend *s);
> +
> TPMBackend *qemu_find_tpm(const char *id);
>
> const TPMDriverOps *tpm_get_backend_driver(const char *type);
> diff --git a/qapi-schema.json b/qapi-schema.json
> index b921994..2953cbc 100644
> --- a/qapi-schema.json
> +++ b/qapi-schema.json
> @@ -5140,6 +5140,16 @@
> { 'command': 'query-tpm-types', 'returns': ['TpmType'] }
>
> ##
> +# @TPMOptions:
> +#
> +# Base type for TPM options
> +#
> +# Since: 2.10
> +##
> +{ 'struct': 'TPMOptions',
> + 'data': { } }
> +
> +##
> # @TPMPassthroughOptions:
> #
> # Information about the TPM passthrough type
> @@ -5151,8 +5161,8 @@
> #
> # Since: 1.5
> ##
> -{ 'struct': 'TPMPassthroughOptions', 'data': { '*path' : 'str',
> - '*cancel-path' : 'str'} }
> +{ 'struct': 'TPMPassthroughOptions', 'base': 'TPMOptions',
> + 'data': { '*path' : 'str', '*cancel-path' : 'str'} }
>
> ##
> # @TpmTypeOptions:
> diff --git a/tpm.c b/tpm.c
> index 0ee021a..c221000 100644
> --- a/tpm.c
> +++ b/tpm.c
> @@ -252,7 +252,6 @@ static const TPMDriverOps
> *tpm_driver_find_by_type(enum TpmType type)
> static TPMInfo *qmp_query_tpm_inst(TPMBackend *drv)
> {
> TPMInfo *res = g_new0(TPMInfo, 1);
> - TPMPassthroughOptions *tpo;
>
> res->id = g_strdup(drv->id);
> res->model = drv->fe_model;
> @@ -261,16 +260,8 @@ static TPMInfo *qmp_query_tpm_inst(TPMBackend *drv)
> switch (tpm_backend_get_type(drv)) {
> case TPM_TYPE_PASSTHROUGH:
> res->options->type = TPM_TYPE_OPTIONS_KIND_PASSTHROUGH;
> - tpo = g_new0(TPMPassthroughOptions, 1);
> - res->options->u.passthrough.data = tpo;
> - if (drv->path) {
> - tpo->path = g_strdup(drv->path);
> - tpo->has_path = true;
> - }
> - if (drv->cancel_path) {
> - tpo->cancel_path = g_strdup(drv->cancel_path);
> - tpo->has_cancel_path = true;
> - }
> + res->options->u.passthrough.data =
> + (TPMPassthroughOptions *)tpm_backend_get_tpm_options(drv);
>
Instead of using a class method, and casting the result, I think it would
make sense to call directly a tpm_passthrough_get_options() function.
Alternatively, add a drv->query_tpm() callback to fill the subclass
implementation/instance details.
break;
> case TPM_TYPE__MAX:
> break;
> --
> 2.7.4
>
>
> --
Marc-André Lureau
