[PATCH v2 0/3] TPM2.0: Added eventlog support for TPM2.0

[PATCH v2 0/3] TPM2.0: Added eventlog support for TPM2.0

[Nayna Jain]

Overview:
=========

This patch adds support for enabling securityfs for TPM2.0, currently
driver has eventlog support only for TPM1.2.
The patch currently adds support for only binary_bios_measurements.

The structure for TPM2.0 is compliant with TCG Spec for 2.0 family.
Also , the reading of data has the assumption that writer would have
followed TCG Spec and so everything is in little-endian.

The tpm device driver code has been refactored to:
* Identify the TPM version - 1.2 or 2.0
* Calls corresponding compatible seq_ops for iterating over eventlog.

Files Description:
===================

* tpm-chip.c : Adds call to setup bios log for TPM2.0.

* tpm2_of.c : Reads the device tree entries to find the location
and size of event.

* tpm_eventlog_init.c : Provides common initialization functions
 between TPM2.0 and TPM1.2 to setup securityfs entries and seq_ops
  iterator.  The functions has been moved from tpm_eventlog.c into this file.

  * tpm_eventlog.c : Provides functions only specific to TPM1.2
  version. Common initialization functions are moved to tpm_eventlog_init.c

  * tpm2_eventlog.c : Provides functions specific only for TPM2.0
  eventlog format.

  * tpm2.h : Header file for TPM2.0 structures and functions.

Changelog v2:
=============

	* Fixes issues as given in feedback by Jason.
	* Adds documentation for device tree.

Nayna Jain (3):
  TPM2.0: Refactored eventlog init functions.
  TPM2.0: TPM Device Tree Documentation
  TPM2.0:Adds securityfs support for TPM2.0 eventlog

 Documentation/devicetree/bindings/i2c/i2c-tpm.txt |  31 +++
 drivers/char/tpm/Makefile                         |   8 +-
 drivers/char/tpm/tpm-chip.c                       |  22 +--
 drivers/char/tpm/tpm.h                            |   2 +-
 drivers/char/tpm/tpm2.h                           |  85 ++++++++
 drivers/char/tpm/tpm2_eventlog.c                  | 224 ++++++++++++++++++++++
 drivers/char/tpm/tpm_acpi.c                       |   2 +-
 drivers/char/tpm/tpm_eventlog.c                   | 156 +--------------
 drivers/char/tpm/tpm_eventlog.h                   |  18 +-
 drivers/char/tpm/tpm_eventlog_init.c              | 174 +++++++++++++++++
 drivers/char/tpm/tpm_of.c                         |  39 ++--
 11 files changed, 570 insertions(+), 191 deletions(-)
 create mode 100644 Documentation/devicetree/bindings/i2c/i2c-tpm.txt
 create mode 100644 drivers/char/tpm/tpm2.h
 create mode 100644 drivers/char/tpm/tpm2_eventlog.c
 create mode 100644 drivers/char/tpm/tpm_eventlog_init.c

-- 
2.5.0


------------------------------------------------------------------------------
What NetFlow Analyzer can do for you? Monitors network bandwidth and traffic
patterns at an interface-level. Reveals which users, apps, and protocols are 
consuming the most bandwidth. Provides multi-vendor support for NetFlow, 
J-Flow, sFlow and other flows. Make informed decisions using capacity 
planning reports. http://sdm.link/zohodev2dev

[PATCH v2 1/3] TPM2.0: Refactored eventlog init functions.

[Nayna Jain]

Refactored eventlog.c file into tpm_eventlog.c and tpm_eventlog_init.c

Breakdown is:

* tpm_eventlog_init.c : Moved eventlog initialization methods like
to setup securityfs, to open and release seqfile from tpm_eventlog.c
to this file. This is to keep the logic of initialization for TPM1.2
and TPM2.0 in common file.

* tpm_eventlog.c : This file now has only methods specific to parsing
and iterate TPM1.2 entry log formats. It can understand only TPM1.2
and is called by methods in tpm_eventlog_init if identified TPM device
is TPM1.2.

Changelog v2:

        * Using of_node property of device rather than direct reading
        the device node.
        * Cleaned up the code to have generic open() for ascii and bios
        measurements
        * Removed dyncamic allocation for bios_dir and having dentry array
	directly into tpm-chip.
        * Using dev_dbg instead of pr_err in tpm_of.c
        * readlog(...) now accepts struct tpm_chip * as parameter.


Signed-off-by: Nayna Jain <nayna-23VcF4HTsmIX0ybBhKVfKdBPR1lH4CV8@public.gmane.org>
---
 drivers/char/tpm/Makefile            |   4 +-
 drivers/char/tpm/tpm-chip.c          |   6 +-
 drivers/char/tpm/tpm.h               |   2 +-
 drivers/char/tpm/tpm_acpi.c          |   2 +-
 drivers/char/tpm/tpm_eventlog.c      | 156 +----------------------------------
 drivers/char/tpm/tpm_eventlog.h      |  16 ++--
 drivers/char/tpm/tpm_eventlog_init.c | 155 ++++++++++++++++++++++++++++++++++
 drivers/char/tpm/tpm_of.c            |  22 +++--
 8 files changed, 189 insertions(+), 174 deletions(-)
 create mode 100644 drivers/char/tpm/tpm_eventlog_init.c

diff --git a/drivers/char/tpm/Makefile b/drivers/char/tpm/Makefile
index a385fb8..9136762 100644
--- a/drivers/char/tpm/Makefile
+++ b/drivers/char/tpm/Makefile
@@ -6,10 +6,10 @@ tpm-y := tpm-interface.o tpm-dev.o tpm-sysfs.o tpm-chip.o tpm2-cmd.o
 tpm-$(CONFIG_ACPI) += tpm_ppi.o
 
 ifdef CONFIG_ACPI
-	tpm-y += tpm_eventlog.o tpm_acpi.o
+	tpm-y += tpm_eventlog_init.o tpm_eventlog.o tpm_acpi.o
 else
 ifdef CONFIG_TCG_IBMVTPM
-	tpm-y += tpm_eventlog.o tpm_of.o
+	tpm-y += tpm_eventlog_init.o tpm_eventlog.o tpm_of.o
 endif
 endif
 obj-$(CONFIG_TCG_TIS_CORE) += tpm_tis_core.o
diff --git a/drivers/char/tpm/tpm-chip.c b/drivers/char/tpm/tpm-chip.c
index e595013..7f6cdab 100644
--- a/drivers/char/tpm/tpm-chip.c
+++ b/drivers/char/tpm/tpm-chip.c
@@ -171,6 +171,8 @@ struct tpm_chip *tpm_chip_alloc(struct device *dev,
 	chip->dev.release = tpm_dev_release;
 	chip->dev.parent = dev;
 	chip->dev.groups = chip->groups;
+	if (dev->of_node)
+		chip->dev.of_node = dev->of_node;
 
 	if (chip->dev_num == 0)
 		chip->dev.devt = MKDEV(MISC_MAJOR, TPM_MINOR);
@@ -283,7 +285,7 @@ static int tpm1_chip_register(struct tpm_chip *chip)
 
 	tpm_sysfs_add_device(chip);
 
-	chip->bios_dir = tpm_bios_log_setup(dev_name(&chip->dev));
+	tpm_bios_log_setup(chip);
 
 	return 0;
 }
@@ -294,7 +296,7 @@ static void tpm1_chip_unregister(struct tpm_chip *chip)
 		return;
 
 	if (chip->bios_dir)
-		tpm_bios_log_teardown(chip->bios_dir);
+		tpm_bios_log_teardown(chip);
 }
 
 static void tpm_del_legacy_sysfs(struct tpm_chip *chip)
diff --git a/drivers/char/tpm/tpm.h b/drivers/char/tpm/tpm.h
index 6e002c4..cfa408f 100644
--- a/drivers/char/tpm/tpm.h
+++ b/drivers/char/tpm/tpm.h
@@ -171,7 +171,7 @@ struct tpm_chip {
 	unsigned long duration[3]; /* jiffies */
 	bool duration_adjusted;
 
-	struct dentry **bios_dir;
+	struct dentry *bios_dir[3];
 
 	const struct attribute_group *groups[3];
 	unsigned int groups_cnt;
diff --git a/drivers/char/tpm/tpm_acpi.c b/drivers/char/tpm/tpm_acpi.c
index 565a947..c2a122a 100644
--- a/drivers/char/tpm/tpm_acpi.c
+++ b/drivers/char/tpm/tpm_acpi.c
@@ -45,7 +45,7 @@ struct acpi_tcpa {
 };
 
 /* read binary bios log */
-int read_log(struct tpm_bios_log *log)
+int read_log(struct tpm_bios_log *log, struct tpm_chip *chip)
 {
 	struct acpi_tcpa *buff;
 	acpi_status status;
diff --git a/drivers/char/tpm/tpm_eventlog.c b/drivers/char/tpm/tpm_eventlog.c
index e722886..b8f22ec 100644
--- a/drivers/char/tpm/tpm_eventlog.c
+++ b/drivers/char/tpm/tpm_eventlog.c
@@ -1,5 +1,5 @@
 /*
- * Copyright (C) 2005, 2012 IBM Corporation
+ * Copyright (C) 2005, 2012, 2016 IBM Corporation
  *
  * Authors:
  *	Kent Yoder <key-23VcF4HTsmIX0ybBhKVfKdBPR1lH4CV8@public.gmane.org>
@@ -11,6 +11,7 @@
  * Maintained by: <tpmdd-devel-5NWGOfrQmneRv+LV9MX5uipxlwaOVQ5f@public.gmane.org>
  *
  * Access to the eventlog created by a system's firmware / BIOS
+ * specific to TPM 1.2.
  *
  * This program is free software; you can redistribute it and/or
  * modify it under the terms of the GNU General Public License
@@ -257,20 +258,6 @@ static int tpm_binary_bios_measurements_show(struct seq_file *m, void *v)
 
 }
 
-static int tpm_bios_measurements_release(struct inode *inode,
-					 struct file *file)
-{
-	struct seq_file *seq = file->private_data;
-	struct tpm_bios_log *log = seq->private;
-
-	if (log) {
-		kfree(log->bios_event_log);
-		kfree(log);
-	}
-
-	return seq_release(inode, file);
-}
-
 static int tpm_ascii_bios_measurements_show(struct seq_file *m, void *v)
 {
 	int len = 0;
@@ -304,151 +291,16 @@ static int tpm_ascii_bios_measurements_show(struct seq_file *m, void *v)
 	return 0;
 }
 
-static const struct seq_operations tpm_ascii_b_measurments_seqops = {
+const struct seq_operations tpm_ascii_b_measurments_seqops = {
 	.start = tpm_bios_measurements_start,
 	.next = tpm_bios_measurements_next,
 	.stop = tpm_bios_measurements_stop,
 	.show = tpm_ascii_bios_measurements_show,
 };
 
-static const struct seq_operations tpm_binary_b_measurments_seqops = {
+const struct seq_operations tpm_binary_b_measurments_seqops = {
 	.start = tpm_bios_measurements_start,
 	.next = tpm_bios_measurements_next,
 	.stop = tpm_bios_measurements_stop,
 	.show = tpm_binary_bios_measurements_show,
 };
-
-static int tpm_ascii_bios_measurements_open(struct inode *inode,
-					    struct file *file)
-{
-	int err;
-	struct tpm_bios_log *log;
-	struct seq_file *seq;
-
-	log = kzalloc(sizeof(struct tpm_bios_log), GFP_KERNEL);
-	if (!log)
-		return -ENOMEM;
-
-	if ((err = read_log(log)))
-		goto out_free;
-
-	/* now register seq file */
-	err = seq_open(file, &tpm_ascii_b_measurments_seqops);
-	if (!err) {
-		seq = file->private_data;
-		seq->private = log;
-	} else {
-		goto out_free;
-	}
-
-out:
-	return err;
-out_free:
-	kfree(log->bios_event_log);
-	kfree(log);
-	goto out;
-}
-
-static const struct file_operations tpm_ascii_bios_measurements_ops = {
-	.open = tpm_ascii_bios_measurements_open,
-	.read = seq_read,
-	.llseek = seq_lseek,
-	.release = tpm_bios_measurements_release,
-};
-
-static int tpm_binary_bios_measurements_open(struct inode *inode,
-					     struct file *file)
-{
-	int err;
-	struct tpm_bios_log *log;
-	struct seq_file *seq;
-
-	log = kzalloc(sizeof(struct tpm_bios_log), GFP_KERNEL);
-	if (!log)
-		return -ENOMEM;
-
-	if ((err = read_log(log)))
-		goto out_free;
-
-	/* now register seq file */
-	err = seq_open(file, &tpm_binary_b_measurments_seqops);
-	if (!err) {
-		seq = file->private_data;
-		seq->private = log;
-	} else {
-		goto out_free;
-	}
-
-out:
-	return err;
-out_free:
-	kfree(log->bios_event_log);
-	kfree(log);
-	goto out;
-}
-
-static const struct file_operations tpm_binary_bios_measurements_ops = {
-	.open = tpm_binary_bios_measurements_open,
-	.read = seq_read,
-	.llseek = seq_lseek,
-	.release = tpm_bios_measurements_release,
-};
-
-static int is_bad(void *p)
-{
-	if (!p)
-		return 1;
-	if (IS_ERR(p) && (PTR_ERR(p) != -ENODEV))
-		return 1;
-	return 0;
-}
-
-struct dentry **tpm_bios_log_setup(const char *name)
-{
-	struct dentry **ret = NULL, *tpm_dir, *bin_file, *ascii_file;
-
-	tpm_dir = securityfs_create_dir(name, NULL);
-	if (is_bad(tpm_dir))
-		goto out;
-
-	bin_file =
-	    securityfs_create_file("binary_bios_measurements",
-				   S_IRUSR | S_IRGRP, tpm_dir, NULL,
-				   &tpm_binary_bios_measurements_ops);
-	if (is_bad(bin_file))
-		goto out_tpm;
-
-	ascii_file =
-	    securityfs_create_file("ascii_bios_measurements",
-				   S_IRUSR | S_IRGRP, tpm_dir, NULL,
-				   &tpm_ascii_bios_measurements_ops);
-	if (is_bad(ascii_file))
-		goto out_bin;
-
-	ret = kmalloc(3 * sizeof(struct dentry *), GFP_KERNEL);
-	if (!ret)
-		goto out_ascii;
-
-	ret[0] = ascii_file;
-	ret[1] = bin_file;
-	ret[2] = tpm_dir;
-
-	return ret;
-
-out_ascii:
-	securityfs_remove(ascii_file);
-out_bin:
-	securityfs_remove(bin_file);
-out_tpm:
-	securityfs_remove(tpm_dir);
-out:
-	return NULL;
-}
-
-void tpm_bios_log_teardown(struct dentry **lst)
-{
-	int i;
-
-	for (i = 0; i < 3; i++)
-		securityfs_remove(lst[i]);
-}
diff --git a/drivers/char/tpm/tpm_eventlog.h b/drivers/char/tpm/tpm_eventlog.h
index 8de62b0..b888c77 100644
--- a/drivers/char/tpm/tpm_eventlog.h
+++ b/drivers/char/tpm/tpm_eventlog.h
@@ -1,4 +1,3 @@
-
 #ifndef __TPM_EVENTLOG_H__
 #define __TPM_EVENTLOG_H__
 
@@ -12,6 +11,9 @@
 #define do_endian_conversion(x) x
 #endif
 
+extern const struct seq_operations tpm_ascii_b_measurments_seqops;
+extern const struct seq_operations tpm_binary_b_measurments_seqops;
+
 enum bios_platform_class {
 	BIOS_CLIENT = 0x00,
 	BIOS_SERVER = 0x01,
@@ -73,18 +75,18 @@ enum tcpa_pc_event_ids {
 	HOST_TABLE_OF_DEVICES,
 };
 
-int read_log(struct tpm_bios_log *log);
+int read_log(struct tpm_bios_log *log, struct tpm_chip *chip);
 
 #if defined(CONFIG_TCG_IBMVTPM) || defined(CONFIG_TCG_IBMVTPM_MODULE) || \
 	defined(CONFIG_ACPI)
-extern struct dentry **tpm_bios_log_setup(const char *);
-extern void tpm_bios_log_teardown(struct dentry **);
+extern void tpm_bios_log_setup(struct tpm_chip *chip);
+extern void tpm_bios_log_teardown(struct tpm_chip *chip);
 #else
-static inline struct dentry **tpm_bios_log_setup(const char *name)
+static inline void tpm_bios_log_setup(struct tpm_chip *chip)
 {
-	return NULL;
+	return;
 }
-static inline void tpm_bios_log_teardown(struct dentry **dir)
+static inline void tpm_bios_log_teardown(struct tpm_chip *chip)
 {
 }
 #endif
diff --git a/drivers/char/tpm/tpm_eventlog_init.c b/drivers/char/tpm/tpm_eventlog_init.c
new file mode 100644
index 0000000..dd5dbc4
--- /dev/null
+++ b/drivers/char/tpm/tpm_eventlog_init.c
@@ -0,0 +1,155 @@
+/*
+ * Copyright (C) 2005, 2012, 2016 IBM Corporation
+ *
+ * Authors:
+ *	Kent Yoder <key-23VcF4HTsmIX0ybBhKVfKdBPR1lH4CV8@public.gmane.org>
+ *	Seiji Munetoh <munetoh-JE5g2YyFxFHQT0dZR+AlfA@public.gmane.org>
+ *	Stefan Berger <stefanb-r/Jw6+rmf7HQT0dZR+AlfA@public.gmane.org>
+ *	Reiner Sailer <sailer-aZOuKsOsJu3MbYB6QlFGEg@public.gmane.org>
+ *	Kylene Hall <kjhall-r/Jw6+rmf7HQT0dZR+AlfA@public.gmane.org>
+ *	Nayna Jain <nayna-23VcF4HTsmIX0ybBhKVfKdBPR1lH4CV8@public.gmane.org>
+ *
+ * Maintained by: <tpmdd-devel-5NWGOfrQmneRv+LV9MX5uipxlwaOVQ5f@public.gmane.org>
+ *
+ * TPM 1.2 and TPM 2.0 common initialization methods to
+ * access the eventlog created by a system's firmware / BIOS.
+ *
+ * This program is free software; you can redistribute it and/or
+ * modify it under the terms of the GNU General Public License
+ * as published by the Free Software Foundation; either version
+ * 2 of the License, or (at your option) any later version.
+ *
+ */
+
+#include <linux/seq_file.h>
+#include <linux/fs.h>
+#include <linux/security.h>
+#include <linux/module.h>
+#include <linux/slab.h>
+
+#include "tpm.h"
+#include "tpm_eventlog.h"
+
+
+static int tpm_bios_measurements_release(struct inode *inode,
+					 struct file *file)
+{
+	struct seq_file *seq = file->private_data;
+	struct tpm_bios_log *log = seq->private;
+
+	if (log) {
+		kfree(log->bios_event_log);
+		kfree(log);
+	}
+
+	return seq_release(inode, file);
+}
+
+static int tpm_bios_measurements_open(struct inode *inode,
+					    struct file *file)
+{
+	int err;
+	struct tpm_bios_log *log;
+	struct seq_file *seq;
+	struct tpm_chip *chip;
+	const struct seq_operations *seqops = (struct seq_operations
+	*)inode->i_private;
+
+	log = kzalloc(sizeof(struct tpm_bios_log), GFP_KERNEL);
+	if (!log)
+		return -ENOMEM;
+
+	chip = (struct tpm_chip
+	*)file->f_path.dentry->d_parent->d_inode->i_private;
+
+	err = read_log(log, chip);
+	if (err)
+		goto out_free;
+
+	/* now register seq file */
+	err = seq_open(file, seqops);
+	if (!err) {
+		seq = file->private_data;
+		seq->private = log;
+	} else {
+		goto out_free;
+	}
+
+out:
+	return err;
+out_free:
+	kfree(log->bios_event_log);
+	kfree(log);
+	goto out;
+}
+
+static const struct file_operations tpm_bios_measurements_ops = {
+	.open = tpm_bios_measurements_open,
+	.read = seq_read,
+	.llseek = seq_lseek,
+	.release = tpm_bios_measurements_release,
+};
+
+static int is_bad(void *p)
+{
+	if (!p)
+		return 1;
+	if (IS_ERR(p) && (PTR_ERR(p) != -ENODEV))
+		return 1;
+	return 0;
+}
+
+void tpm_bios_log_setup(struct tpm_chip *chip)
+{
+	struct dentry *tpm_dir, *bin_file, *ascii_file;
+	const char *name = dev_name(&chip->dev);
+	int i;
+
+	for (i = 0; i < 3; i++)
+		chip->bios_dir[i] = NULL;
+
+	tpm_dir = securityfs_create_dir(name, NULL);
+	if (is_bad(tpm_dir))
+		goto out;
+
+	tpm_dir->d_inode->i_private = chip;
+
+	bin_file =
+	    securityfs_create_file("binary_bios_measurements",
+				   S_IRUSR | S_IRGRP, tpm_dir,
+				   (void *)&tpm_binary_b_measurments_seqops,
+				   &tpm_bios_measurements_ops);
+	if (is_bad(bin_file))
+		goto out_tpm;
+
+	ascii_file =
+	    securityfs_create_file("ascii_bios_measurements",
+				   S_IRUSR | S_IRGRP, tpm_dir,
+				   (void *)&tpm_ascii_b_measurments_seqops,
+				   &tpm_bios_measurements_ops);
+	if (is_bad(ascii_file))
+		goto out_bin;
+
+	chip->bios_dir[0] = ascii_file;
+	chip->bios_dir[1] = bin_file;
+	chip->bios_dir[2] = tpm_dir;
+
+	return;
+
+out_bin:
+	securityfs_remove(bin_file);
+out_tpm:
+	securityfs_remove(tpm_dir);
+out:
+	return;
+}
+
+void tpm_bios_log_teardown(struct tpm_chip *chip)
+{
+	int i;
+
+	for (i = 0; i < 3; i++) {
+		if (chip->bios_dir[i])
+			securityfs_remove(chip->bios_dir[i]);
+	}
+}
diff --git a/drivers/char/tpm/tpm_of.c b/drivers/char/tpm/tpm_of.c
index 570f30c..30a9905 100644
--- a/drivers/char/tpm/tpm_of.c
+++ b/drivers/char/tpm/tpm_of.c
@@ -1,7 +1,8 @@
 /*
- * Copyright 2012 IBM Corporation
+ * Copyright 2012, 2016 IBM Corporation
  *
  * Author: Ashley Lai <ashleydlai-Re5JQEeQqe8AvxtiuMwx3w@public.gmane.org>
+ *         Nayna Jain <nayna-23VcF4HTsmIX0ybBhKVfKdBPR1lH4CV8@public.gmane.org>
  *
  * Maintained by: <tpmdd-devel-5NWGOfrQmneRv+LV9MX5uipxlwaOVQ5f@public.gmane.org>
  *
@@ -20,7 +21,7 @@
 #include "tpm.h"
 #include "tpm_eventlog.h"
 
-int read_log(struct tpm_bios_log *log)
+int read_log(struct tpm_bios_log *log, struct tpm_chip *chip)
 {
 	struct device_node *np;
 	const u32 *sizep;
@@ -31,32 +32,35 @@ int read_log(struct tpm_bios_log *log)
 		return -EFAULT;
 	}
 
-	np = of_find_node_by_name(NULL, "vtpm");
+	if (chip->dev.of_node)
+		np = chip->dev.of_node;
 	if (!np) {
-		pr_err("%s: ERROR - IBMVTPM not supported\n", __func__);
+		dev_dbg(&chip->dev, "%s: ERROR - IBMVTPM not supported\n",
+		__func__);
 		return -ENODEV;
 	}
 
 	sizep = of_get_property(np, "linux,sml-size", NULL);
 	if (sizep == NULL) {
-		pr_err("%s: ERROR - SML size not found\n", __func__);
+		dev_dbg(&chip->dev, "%s: ERROR - SML size not found\n",
+		__func__);
 		goto cleanup_eio;
 	}
 	if (*sizep == 0) {
-		pr_err("%s: ERROR - event log area empty\n", __func__);
+		dev_dbg(&chip->dev, "%s: ERROR - event log area empty\n",
+		__func__);
 		goto cleanup_eio;
 	}
 
 	basep = of_get_property(np, "linux,sml-base", NULL);
 	if (basep == NULL) {
-		pr_err("%s: ERROR - SML not found\n", __func__);
+		dev_dbg(&chip->dev, "%s: ERROR - SML not found\n",
+		__func__);
 		goto cleanup_eio;
 	}
 
 	log->bios_event_log = kmalloc(*sizep, GFP_KERNEL);
 	if (!log->bios_event_log) {
-		pr_err("%s: ERROR - Not enough memory for BIOS measurements\n",
-		       __func__);
 		of_node_put(np);
 		return -ENOMEM;
 	}
-- 
2.5.0


------------------------------------------------------------------------------
What NetFlow Analyzer can do for you? Monitors network bandwidth and traffic
patterns at an interface-level. Reveals which users, apps, and protocols are 
consuming the most bandwidth. Provides multi-vendor support for NetFlow, 
J-Flow, sFlow and other flows. Make informed decisions using capacity 
planning reports. http://sdm.link/zohodev2dev

[PATCH v2 2/3] TPM2.0: TPM Device Tree Documentation

[Nayna Jain]

Added device tree binding documentation for I2C base TPM.

Changelog v2:
        * Added documentation in v2.

Signed-off-by: Nayna Jain <nayna-23VcF4HTsmIX0ybBhKVfKdBPR1lH4CV8@public.gmane.org>
---
 Documentation/devicetree/bindings/i2c/i2c-tpm.txt | 31 +++++++++++++++++++++++
 1 file changed, 31 insertions(+)
 create mode 100644 Documentation/devicetree/bindings/i2c/i2c-tpm.txt

diff --git a/Documentation/devicetree/bindings/i2c/i2c-tpm.txt b/Documentation/devicetree/bindings/i2c/i2c-tpm.txt
new file mode 100644
index 0000000..1df05cf
--- /dev/null
+++ b/Documentation/devicetree/bindings/i2c/i2c-tpm.txt
@@ -0,0 +1,31 @@
+Device Tree Bindings for I2C based Trusted Platform Module(TPM)
+---------------------------------------------------------------
+
+This node describes a TPM device connected to Processor on i2c bus.
+
+Required properties:
+
+- compatible : 'manufacturer,model'
+- label : "tpm" indicates master TPM; "tpm-backup" indicates backup TPM.
+- linux,sml-base : base address of the Event Log. It is a physical address.
+		   sml stands for shared memory log.
+- linux,sml-size : size of the memory allocated for the Event Log.
+
+Optional properties:
+
+- status: indicates whether the device is enabled or disabled. "okay" for
+          enabled and "disabled" for disabled.
+
+Example
+-------
+
+tpm@57 {
+	reg = <0x57>;
+	label = "tpm";
+	compatible = "nuvoton,npct650", "nuvoton,npct601";
+	linux,sml-base = <0x7f 0xfd450000>;
+	linux,sml-size = <0x10000>;
+	status = "okay";
+	phandle = <0x10000017>;
+	linux,phandle = <0x10000017>;
+};
-- 
2.5.0


------------------------------------------------------------------------------
What NetFlow Analyzer can do for you? Monitors network bandwidth and traffic
patterns at an interface-level. Reveals which users, apps, and protocols are 
consuming the most bandwidth. Provides multi-vendor support for NetFlow, 
J-Flow, sFlow and other flows. Make informed decisions using capacity 
planning reports. http://sdm.link/zohodev2dev

[PATCH v2 3/3] TPM2.0:Adds securityfs support for TPM2.0 eventlog

[Nayna Jain]

Adds securityfs support for TPM2.0.
This patch currently supports only binary_bios_measurements.

Changelog v2:
        * Single tpm_of.c for reading both tpm and vtpm device tree values.
        * Some of the issues are fixed in Patch 1 itself.
        * Comments in tpm2.h give reference to the standard from where structs
	are taken.
	* Now, tpm_of.c has same code applied for both tpm and vtpm, so I think
	that now it is needed to have generic types rather than endian specific type.

There are few preexisting issues as being mentioned in feedback and are not 
addressed in this patch. Reason being, I don't have much expertise of ACPI side as of now, 
and these changes will affect acpi,tpm,vtpm, all paths, so I would like to go slow
and fix them as different patch later after better understanding.
Hope this sounds ok to have them as different patch.

Issues which are not addressed are as below:
        * tpm_eventlog.h still has #ifdef defined, for tpm_bios_log_setup()
        * tpm_bios_log_setup is still being called in tpm-chip register function.

Signed-off-by: Nayna Jain <nayna-23VcF4HTsmIX0ybBhKVfKdBPR1lH4CV8@public.gmane.org>
---
 drivers/char/tpm/Makefile            |   8 +-
 drivers/char/tpm/tpm-chip.c          |  20 +---
 drivers/char/tpm/tpm2.h              |  85 +++++++++++++
 drivers/char/tpm/tpm2_eventlog.c     | 224 +++++++++++++++++++++++++++++++++++
 drivers/char/tpm/tpm_eventlog.h      |   2 +-
 drivers/char/tpm/tpm_eventlog_init.c |  43 +++++--
 drivers/char/tpm/tpm_of.c            |  17 ++-
 7 files changed, 366 insertions(+), 33 deletions(-)
 create mode 100644 drivers/char/tpm/tpm2.h
 create mode 100644 drivers/char/tpm/tpm2_eventlog.c

diff --git a/drivers/char/tpm/Makefile b/drivers/char/tpm/Makefile
index 9136762..509ace2 100644
--- a/drivers/char/tpm/Makefile
+++ b/drivers/char/tpm/Makefile
@@ -6,10 +6,14 @@ tpm-y := tpm-interface.o tpm-dev.o tpm-sysfs.o tpm-chip.o tpm2-cmd.o
 tpm-$(CONFIG_ACPI) += tpm_ppi.o
 
 ifdef CONFIG_ACPI
-	tpm-y += tpm_eventlog_init.o tpm_eventlog.o tpm_acpi.o
+	tpm-y += tpm_eventlog_init.o tpm2_eventlog.o tpm_eventlog.o tpm_acpi.o
 else
 ifdef CONFIG_TCG_IBMVTPM
-	tpm-y += tpm_eventlog_init.o tpm_eventlog.o tpm_of.o
+	tpm-y += tpm_eventlog_init.o tpm2_eventlog.o tpm_eventlog.o tpm_of.o
+else
+ifdef CONFIG_PPC64
+	tpm-y += tpm_eventlog_init.o tpm2_eventlog.o tpm_eventlog.o tpm_of.o
+endif
 endif
 endif
 obj-$(CONFIG_TCG_TIS_CORE) += tpm_tis_core.o
diff --git a/drivers/char/tpm/tpm-chip.c b/drivers/char/tpm/tpm-chip.c
index 7f6cdab..3f1c489 100644
--- a/drivers/char/tpm/tpm-chip.c
+++ b/drivers/char/tpm/tpm-chip.c
@@ -285,20 +285,9 @@ static int tpm1_chip_register(struct tpm_chip *chip)
 
 	tpm_sysfs_add_device(chip);
 
-	tpm_bios_log_setup(chip);
-
 	return 0;
 }
 
-static void tpm1_chip_unregister(struct tpm_chip *chip)
-{
-	if (chip->flags & TPM_CHIP_FLAG_TPM2)
-		return;
-
-	if (chip->bios_dir)
-		tpm_bios_log_teardown(chip);
-}
-
 static void tpm_del_legacy_sysfs(struct tpm_chip *chip)
 {
 	struct attribute **i;
@@ -372,10 +361,8 @@ int tpm_chip_register(struct tpm_chip *chip)
 	tpm_add_ppi(chip);
 
 	rc = tpm_add_char_device(chip);
-	if (rc) {
-		tpm1_chip_unregister(chip);
+	if (rc)
 		return rc;
-	}
 
 	chip->flags |= TPM_CHIP_FLAG_REGISTERED;
 
@@ -385,6 +372,8 @@ int tpm_chip_register(struct tpm_chip *chip)
 		return rc;
 	}
 
+	tpm_bios_log_setup(chip);
+
 	return 0;
 }
 EXPORT_SYMBOL_GPL(tpm_chip_register);
@@ -409,7 +398,8 @@ void tpm_chip_unregister(struct tpm_chip *chip)
 
 	tpm_del_legacy_sysfs(chip);
 
-	tpm1_chip_unregister(chip);
+	tpm_bios_log_teardown(chip);
+
 	tpm_del_char_device(chip);
 }
 EXPORT_SYMBOL_GPL(tpm_chip_unregister);
diff --git a/drivers/char/tpm/tpm2.h b/drivers/char/tpm/tpm2.h
new file mode 100644
index 0000000..38a8073
--- /dev/null
+++ b/drivers/char/tpm/tpm2.h
@@ -0,0 +1,85 @@
+#ifndef __TPM2_H__
+#define __TPM2_H__
+
+#define TPM_ALG_SHA1_DIGEST_SIZE	20
+#define TPM_ALG_SHA256_DIGEST_SIZE	32
+#define TPM_ALG_SHA384_DIGEST_SIZE	48
+
+#define HASH_COUNT	3
+#define MAX_TPM_LOG_MSG	128
+
+/**
+ * All the structures related to Event Log are taken from TCG EFI Protocol
+ * Specification, Family "2.0". Document is available on link
+ * http://www.trustedcomputinggroup.org/tcg-efi-protocol-specification/
+ * Information is also available on TCG PC Client Platform Firmware Profile
+ * Specification, Family "2.0"
+ * Detailed digest structures for TPM2.0 are defined in document
+ * Trusted Platform Module Library Part 2: Structures, Family "2.0".
+ */
+
+/* Event log header algorithm spec. */
+struct tcg_efispecideventalgorithmsize {
+	u16	algorithm_id;
+	u16	digest_size;
+} __packed;
+
+/* Event log header data. */
+struct tcg_efispecideventstruct {
+	u8					signature[16];
+	u32					platform_class;
+	u8					spec_version_minor;
+	u8					spec_version_major;
+	u8					spec_errata;
+	u8					uintnsize;
+	u32					num_algs;
+	struct tcg_efispecideventalgorithmsize	digest_sizes[HASH_COUNT];
+	u8					vendor_info_size;
+	u8					vendor_info[0];
+} __packed;
+
+/* Header entry for eventlog. */
+struct tcg_pcr_event {
+	u32	pcr_index;
+	u32	event_type;
+	u8	digest[20];
+	u32	event_size;
+	u8	event[MAX_TPM_LOG_MSG];
+} __packed;
+
+/* Digest union for crypto agility. */
+union tpmu_ha {
+	u8	 sha1[TPM_ALG_SHA1_DIGEST_SIZE];
+	u8	 sha256[TPM_ALG_SHA256_DIGEST_SIZE];
+	u8	 sha384[TPM_ALG_SHA384_DIGEST_SIZE];
+} __packed;
+
+/* Crypto Agile algorithm and respective digest. */
+struct tpmt_ha {
+	u16		algorithm_id;
+	union tpmu_ha	digest;
+} __packed;
+
+/* Crypto agile digests list. */
+struct tpml_digest_values {
+	u32		count;
+	struct tpmt_ha	digests[HASH_COUNT];
+} __packed;
+
+/* Event field structure. */
+struct tcg_event_field {
+	u32	event_size;
+	u8      event[MAX_TPM_LOG_MSG];
+} __packed;
+
+/* Crypto agile log entry format for TPM 2.0. */
+struct tcg_pcr_event2 {
+	u32				pcr_index;
+	u32				event_type;
+	struct tpml_digest_values	digests;
+	struct tcg_event_field		event;
+} __packed;
+
+extern const struct seq_operations tpm2_binary_b_measurments_seqops;
+
+#endif
diff --git a/drivers/char/tpm/tpm2_eventlog.c b/drivers/char/tpm/tpm2_eventlog.c
new file mode 100644
index 0000000..f1e8c4a
--- /dev/null
+++ b/drivers/char/tpm/tpm2_eventlog.c
@@ -0,0 +1,224 @@
+/*
+ * Copyright (C) 2016 IBM Corporation
+ *
+ * Authors:
+ *      Nayna Jain <nayna-23VcF4HTsmIX0ybBhKVfKdBPR1lH4CV8@public.gmane.org>
+
+ * Access to TPM2.0 event log as written by Firmware.
+ * It assumes that writer of event log has followed TCG Spec 2.0
+ * has written the event struct data in little endian. With that,
+ * it doesn't need any endian conversion for structure content.
+ *
+ * This program is free software; you can redistribute it and/or
+ * modify it under the terms of the GNU General Public License
+ * as published by the Free Software Foundation; either version
+ * 2 of the License, or (at your option) any later version.
+ */
+
+#include <linux/seq_file.h>
+#include <linux/fs.h>
+#include <linux/security.h>
+#include <linux/module.h>
+#include <linux/slab.h>
+
+#include "tpm.h"
+#include "tpm2.h"
+#include "tpm_eventlog.h"
+
+
+static int calc_tpm2_event_size(struct tcg_pcr_event2 *event,
+		struct tcg_pcr_event *event_header)
+{
+	struct tcg_efispecideventstruct *efispecid;
+	struct tcg_event_field *event_field;
+	void *marker, *marker_start;
+	int j;
+	size_t size = 0;
+
+	/*
+	 * NOTE: TPM2.0 allows support for extend to multiple PCR Banks.
+	 * This implies that eventlog also has multiple digest values
+	 * one for each PCR Bank. This is called Crypto Agile Log Entry
+	 * Format. Current support implementation is for SHA1 and SHA256.
+	 * Number of digest values are identified by parsing first
+	 * structure stored in event log also called event header.
+	 * Further, the eventlog is written in packed form so to calculate
+	 * offset it is important to know the type of algorithm used.
+	 * Eg. 1:
+	 * digest_values.count = 1;
+	 * digest_values.digest[0].algorithm_id = sha1;
+	 * digest_values.digest[0].digest.sha1 = {20 bytes raw data};
+	 * Offset of eventsize is sizeof(count) + sizeof(algorithm_id) + 20
+	 *
+	 * Eg. 2:
+	 * digest_values.count = 1;
+	 * digest_values.digest[0].algorithm_id = sha256;
+	 * digest_values.digest[0].digest.sha1 = {32 bytes raw data};
+	 * Offset of eventsize is sizeof(count) + sizeof(algorithm_id) + 32
+
+	 * Eg. 3:
+	 * digest_values.count = 2;
+	 * digest_values.digest[0].algorithm_id = sha1;
+	 * digest_values.digest[0].digest.sha1 = {20 bytes raw data};
+	 * digest_values.digest[1].algorithm_id = sha256;
+	 * digest_values.digest[1].digest.sha256 = {32 bytes raw data};
+	 * Offset of eventsize is sizeof(count) + sizeof(algorithm_id) + 20
+	 *			+ sizeof(algorithm_id) + 32;
+	 *
+	 * So, it implies that offset of event_size can vary based on digest
+	 * values as defined by vendor. And so we have to calculate the
+	 * offset by parsing through number and type of digests being used.
+	 * And this is the purpose of using *marker to traverse the structure
+	 * and calculate the offset of event_size. This function uses *marker
+	 * to parse and calculate the dynamic size of the whole event structure.
+	 */
+
+	marker = event;
+	marker_start = marker;
+	marker = marker + sizeof(event->pcr_index) + sizeof(event->event_type)
+		+ sizeof(event->digests.count);
+
+	efispecid = (struct tcg_efispecideventstruct *) event_header->event;
+
+	for (j = 0; (j < efispecid->num_algs) && (j < HASH_COUNT); j++) {
+		marker = marker
+			+ sizeof(efispecid->digest_sizes[j].algorithm_id);
+		marker = marker + efispecid->digest_sizes[j].digest_size;
+	}
+
+	event_field = (struct tcg_event_field *) marker;
+	marker = marker + sizeof(event_field->event_size)
+		+ event_field->event_size;
+	size = marker - marker_start;
+
+	if ((event->event_type == 0) && (event_field->event_size == 0))
+		return 0;
+
+	return size;
+}
+
+static void *tpm2_bios_measurements_start(struct seq_file *m, loff_t *pos)
+{
+	struct tpm_bios_log *log = m->private;
+	void *addr = log->bios_event_log;
+	void *limit = log->bios_event_log_end;
+	struct tcg_pcr_event *event_header;
+	struct tcg_pcr_event2 *event;
+	int i;
+	size_t size = 0;
+
+	event_header = addr;
+
+	size = sizeof(struct tcg_pcr_event) - sizeof(event_header->event)
+		+ event_header->event_size;
+
+
+	if (*pos == 0) {
+		if (addr + size < limit) {
+			if ((event_header->event_type == 0) &&
+					(event_header->event_size == 0))
+				return NULL;
+			return SEQ_START_TOKEN;
+		}
+	}
+
+	if (*pos > 0) {
+		addr += size;
+		event = addr;
+		size = calc_tpm2_event_size(event, event_header);
+		if ((addr + size >=  limit) || (size == 0))
+			return NULL;
+	}
+
+	/* read over *pos measurements */
+	for (i = 0; i < (*pos - 1); i++) {
+		event = addr;
+		size = calc_tpm2_event_size(event, event_header);
+
+		if ((addr + size >= limit) || (size == 0))
+			return NULL;
+		addr += size;
+	}
+
+	return addr;
+}
+
+static void *tpm2_bios_measurements_next(struct seq_file *m, void *v,
+		loff_t *pos)
+{
+	struct tcg_pcr_event *event_header;
+	struct tcg_pcr_event2 *event;
+	struct tpm_bios_log *log = m->private;
+	void *limit = log->bios_event_log_end;
+	void *marker;
+	size_t event_size = 0;
+
+	event_header = log->bios_event_log;
+
+	if (v == SEQ_START_TOKEN) {
+		event_size = sizeof(struct tcg_pcr_event)
+			- sizeof(event_header->event)
+			+ event_header->event_size;
+		marker = event_header;
+	} else {
+		event = v;
+		event_size = calc_tpm2_event_size(event, event_header);
+		if (event_size == 0)
+			return NULL;
+		marker =  event;
+	}
+
+	marker = marker + event_size;
+	if (marker >= limit)
+		return NULL;
+	v = marker;
+	event = v;
+
+	event_size = calc_tpm2_event_size(event, event_header);
+	if (((v + event_size) >= limit) || (event_size == 0))
+		return NULL;
+
+	(*pos)++;
+	return v;
+}
+
+static void tpm2_bios_measurements_stop(struct seq_file *m, void *v)
+{
+}
+
+static int tpm2_binary_bios_measurements_show(struct seq_file *m, void *v)
+{
+	struct tpm_bios_log *log = m->private;
+	struct tcg_pcr_event *event_header = log->bios_event_log;
+	struct tcg_pcr_event2 *event = v;
+	void *temp_ptr;
+	size_t size = 0;
+
+	if (v == SEQ_START_TOKEN) {
+
+		size = sizeof(struct tcg_pcr_event)
+			- sizeof(event_header->event)
+			+ event_header->event_size;
+
+		temp_ptr = event_header;
+
+		if (size > 0)
+			seq_write(m, temp_ptr, size);
+	} else {
+
+		size = calc_tpm2_event_size(event, event_header);
+
+		temp_ptr = event;
+		if (size > 0)
+			seq_write(m, temp_ptr, size);
+	}
+
+	return 0;
+}
+
+const struct seq_operations tpm2_binary_b_measurments_seqops = {
+	.start = tpm2_bios_measurements_start,
+	.next = tpm2_bios_measurements_next,
+	.stop = tpm2_bios_measurements_stop,
+	.show = tpm2_binary_bios_measurements_show,
+};
diff --git a/drivers/char/tpm/tpm_eventlog.h b/drivers/char/tpm/tpm_eventlog.h
index b888c77..3297fda 100644
--- a/drivers/char/tpm/tpm_eventlog.h
+++ b/drivers/char/tpm/tpm_eventlog.h
@@ -78,7 +78,7 @@ enum tcpa_pc_event_ids {
 int read_log(struct tpm_bios_log *log, struct tpm_chip *chip);
 
 #if defined(CONFIG_TCG_IBMVTPM) || defined(CONFIG_TCG_IBMVTPM_MODULE) || \
-	defined(CONFIG_ACPI)
+	defined(CONFIG_ACPI) || defined(CONFIG_PPC64)
 extern void tpm_bios_log_setup(struct tpm_chip *chip);
 extern void tpm_bios_log_teardown(struct tpm_chip *chip);
 #else
diff --git a/drivers/char/tpm/tpm_eventlog_init.c b/drivers/char/tpm/tpm_eventlog_init.c
index dd5dbc4..a3fb5bc 100644
--- a/drivers/char/tpm/tpm_eventlog_init.c
+++ b/drivers/char/tpm/tpm_eventlog_init.c
@@ -28,6 +28,7 @@
 #include <linux/slab.h>
 
 #include "tpm.h"
+#include "tpm2.h"
 #include "tpm_eventlog.h"
 
 
@@ -103,36 +104,54 @@ void tpm_bios_log_setup(struct tpm_chip *chip)
 {
 	struct dentry *tpm_dir, *bin_file, *ascii_file;
 	const char *name = dev_name(&chip->dev);
+	void *seq_ops;
+	int num_files = 0;
 	int i;
 
 	for (i = 0; i < 3; i++)
 		chip->bios_dir[i] = NULL;
 
+	/*
+	 * NOTE: Currently, support is added only for binary_bios_measurements
+	 * for TPM2.0. And so it is required to check that if device is TPM2,
+	 * only 2 entries are allocated i.e. tpm0 dir and
+	 * binary_bios_measurements file.
+	 */
+
+	num_files = chip->flags & TPM_CHIP_FLAG_TPM2 ? 2 : 3;
+
 	tpm_dir = securityfs_create_dir(name, NULL);
 	if (is_bad(tpm_dir))
 		goto out;
 
 	tpm_dir->d_inode->i_private = chip;
 
+	if (chip->flags & TPM_CHIP_FLAG_TPM2)
+		seq_ops = (void *)&tpm2_binary_b_measurments_seqops;
+	else
+		seq_ops = (void *)&tpm_binary_b_measurments_seqops;
+
 	bin_file =
 	    securityfs_create_file("binary_bios_measurements",
 				   S_IRUSR | S_IRGRP, tpm_dir,
-				   (void *)&tpm_binary_b_measurments_seqops,
+				   seq_ops,
 				   &tpm_bios_measurements_ops);
 	if (is_bad(bin_file))
 		goto out_tpm;
 
-	ascii_file =
-	    securityfs_create_file("ascii_bios_measurements",
-				   S_IRUSR | S_IRGRP, tpm_dir,
-				   (void *)&tpm_ascii_b_measurments_seqops,
-				   &tpm_bios_measurements_ops);
-	if (is_bad(ascii_file))
-		goto out_bin;
-
-	chip->bios_dir[0] = ascii_file;
-	chip->bios_dir[1] = bin_file;
-	chip->bios_dir[2] = tpm_dir;
+	chip->bios_dir[num_files-1] = tpm_dir;
+	chip->bios_dir[num_files-2] = bin_file;
+
+	if (num_files == 3) {
+		ascii_file =
+			securityfs_create_file("ascii_bios_measurements",
+					S_IRUSR | S_IRGRP, tpm_dir,
+					(void *)&tpm_ascii_b_measurments_seqops,
+					&tpm_bios_measurements_ops);
+		if (is_bad(ascii_file))
+			goto out_bin;
+		chip->bios_dir[num_files-3] = ascii_file;
+	}
 
 	return;
 
diff --git a/drivers/char/tpm/tpm_of.c b/drivers/char/tpm/tpm_of.c
index 30a9905..d3c72ce 100644
--- a/drivers/char/tpm/tpm_of.c
+++ b/drivers/char/tpm/tpm_of.c
@@ -26,6 +26,7 @@ int read_log(struct tpm_bios_log *log, struct tpm_chip *chip)
 	struct device_node *np;
 	const u32 *sizep;
 	const u64 *basep;
+	u32 log_size;
 
 	if (log->bios_event_log != NULL) {
 		pr_err("%s: ERROR - Eventlog already initialized\n", __func__);
@@ -52,6 +53,11 @@ int read_log(struct tpm_bios_log *log, struct tpm_chip *chip)
 		goto cleanup_eio;
 	}
 
+	if (!strncmp(np->name, "tpm", 3))
+		log_size = be32_to_cpup(sizep);
+	else
+		log_size = *sizep;
+
 	basep = of_get_property(np, "linux,sml-base", NULL);
 	if (basep == NULL) {
 		dev_dbg(&chip->dev, "%s: ERROR - SML not found\n",
@@ -59,15 +65,20 @@ int read_log(struct tpm_bios_log *log, struct tpm_chip *chip)
 		goto cleanup_eio;
 	}
 
-	log->bios_event_log = kmalloc(*sizep, GFP_KERNEL);
+	log->bios_event_log = kmalloc(log_size, GFP_KERNEL);
 	if (!log->bios_event_log) {
 		of_node_put(np);
 		return -ENOMEM;
 	}
 
-	log->bios_event_log_end = log->bios_event_log + *sizep;
+	log->bios_event_log_end = log->bios_event_log + log_size;
+
+	if (!strncmp(np->name, "tpm", 3))
+		memcpy(log->bios_event_log, __va(be64_to_cpup(basep)),
+		log_size);
+	else
+		memcpy(log->bios_event_log, __va(*basep), log_size);
 
-	memcpy(log->bios_event_log, __va(*basep), *sizep);
 	of_node_put(np);
 
 	return 0;
-- 
2.5.0


------------------------------------------------------------------------------
What NetFlow Analyzer can do for you? Monitors network bandwidth and traffic
patterns at an interface-level. Reveals which users, apps, and protocols are 
consuming the most bandwidth. Provides multi-vendor support for NetFlow, 
J-Flow, sFlow and other flows. Make informed decisions using capacity 
planning reports. http://sdm.link/zohodev2dev

Re: [PATCH v2 2/3] TPM2.0: TPM Device Tree Documentation

[Jason Gunthorpe]

On Tue, Aug 09, 2016 at 03:34:54PM -0400, Nayna Jain wrote:
> Added device tree binding documentation for I2C base TPM.

You need to review

 Documentation/devicetree/bindings/submitting-patches.txt

And be sure to follow all the requirements, including CC'ing the
right maintainers

Also explain that this is a documenting a historical practice that IBM
has used in systems  X Y Z.

> Changelog v2:
>         * Added documentation in v2.

Do not include these lines in commit messages, put them after the
diff stat.

> +This node describes a TPM device connected to Processor on i2c bus.
> +
> +Required properties:
> +
> +- compatible : 'manufacturer,model'
> +- label : "tpm" indicates master TPM; "tpm-backup" indicates backup TPM.
> +- linux,sml-base : base address of the Event Log. It is a physical address.
> +		   sml stands for shared memory log.
> +- linux,sml-size : size of the memory allocated for the Event Log.
> +
> +Optional properties:
> +
> +- status: indicates whether the device is enabled or disabled. "okay" for
> +          enabled and "disabled" for disabled.
> +
> +Example
> +-------
> +
> +tpm@57 {
> +	reg = <0x57>;
> +	label = "tpm";
> +	compatible = "nuvoton,npct650", "nuvoton,npct601";
> +	linux,sml-base = <0x7f 0xfd450000>;
> +	linux,sml-size = <0x10000>;
> +	status = "okay";

> +	phandle = <0x10000017>;
> +	linux,phandle = <0x10000017>;

 What are these?

Jason

------------------------------------------------------------------------------
What NetFlow Analyzer can do for you? Monitors network bandwidth and traffic
patterns at an interface-level. Reveals which users, apps, and protocols are 
consuming the most bandwidth. Provides multi-vendor support for NetFlow, 
J-Flow, sFlow and other flows. Make informed decisions using capacity 
planning reports. http://sdm.link/zohodev2dev

Re: [PATCH v2 3/3] TPM2.0:Adds securityfs support for TPM2.0 eventlog

[Jason Gunthorpe]

On Tue, Aug 09, 2016 at 03:34:55PM -0400, Nayna Jain wrote:
> Adds securityfs support for TPM2.0.
> This patch currently supports only binary_bios_measurements.
> 
> Changelog v2:
>         * Single tpm_of.c for reading both tpm and vtpm device tree values.
>         * Some of the issues are fixed in Patch 1 itself.
>         * Comments in tpm2.h give reference to the standard from where structs
> 	are taken.
> 	* Now, tpm_of.c has same code applied for both tpm and vtpm, so I think
> 	that now it is needed to have generic types rather than endian specific type.
> 
> There are few preexisting issues as being mentioned in feedback and are not 
> addressed in this patch. Reason being, I don't have much expertise of ACPI side as of now, 
> and these changes will affect acpi,tpm,vtpm, all paths, so I would like to go slow
> and fix them as different patch later after better understanding.
> Hope this sounds ok to have them as different patch.

Yes, but do the patches first please, this stuff is obviously broken,
do not make the mess larger.

You are the only person ever to come up who can test the _of side of
this, so the rest of us have the opposite problem to you.

If you have doubts about the acpi stuff then ask.

> +	/*
> +	 * NOTE: Currently, support is added only for binary_bios_measurements
> +	 * for TPM2.0. And so it is required to check that if device is TPM2,
> +	 * only 2 entries are allocated i.e. tpm0 dir and
> +	 * binary_bios_measurements file.
> +	 */
> +
> +	num_files = chip->flags & TPM_CHIP_FLAG_TPM2 ? 2 : 3;

?? just ++ as you go like sysfs does

>  	const u32 *sizep;
>  	const u64 *basep;
> +	u32 log_size;
>  
>  	if (log->bios_event_log != NULL) {
>  		pr_err("%s: ERROR - Eventlog already initialized\n", __func__);
> @@ -52,6 +53,11 @@ int read_log(struct tpm_bios_log *log, struct tpm_chip *chip)
>  		goto cleanup_eio;
>  	}
>  
> +	if (!strncmp(np->name, "tpm", 3))
> +		log_size = be32_to_cpup(sizep);
> +	else
> +		log_size = *sizep;

?? That deserves a comment, one of your systems has swapped
endianness?? 'np->name' is not an appropriate way to detect
that. compatible string would be better, or something else..

------------------------------------------------------------------------------
What NetFlow Analyzer can do for you? Monitors network bandwidth and traffic
patterns at an interface-level. Reveals which users, apps, and protocols are 
consuming the most bandwidth. Provides multi-vendor support for NetFlow, 
J-Flow, sFlow and other flows. Make informed decisions using capacity 
planning reports. http://sdm.link/zohodev2dev

Re: [PATCH v2 1/3] TPM2.0: Refactored eventlog init functions.

[Jason Gunthorpe]

On Tue, Aug 09, 2016 at 03:34:53PM -0400, Nayna Jain wrote:
> Refactored eventlog.c file into tpm_eventlog.c and tpm_eventlog_init.c
> 
> Breakdown is:
> 
> * tpm_eventlog_init.c : Moved eventlog initialization methods like
> to setup securityfs, to open and release seqfile from tpm_eventlog.c
> to this file. This is to keep the logic of initialization for TPM1.2
> and TPM2.0 in common file.
> 
> * tpm_eventlog.c : This file now has only methods specific to parsing
> and iterate TPM1.2 entry log formats. It can understand only TPM1.2
> and is called by methods in tpm_eventlog_init if identified TPM device
> is TPM1.2.
> 
> Changelog v2:
> 
>         * Using of_node property of device rather than direct reading
>         the device node.
>         * Cleaned up the code to have generic open() for ascii and bios
>         measurements
>         * Removed dyncamic allocation for bios_dir and having dentry array
> 	directly into tpm-chip.
>         * Using dev_dbg instead of pr_err in tpm_of.c
>         * readlog(...) now accepts struct tpm_chip * as parameter.

This patch needs to be split.

One patch per idea please.

>  ifdef CONFIG_ACPI
> -	tpm-y += tpm_eventlog.o tpm_acpi.o
> +	tpm-y += tpm_eventlog_init.o tpm_eventlog.o tpm_acpi.o
>  else
>  ifdef CONFIG_TCG_IBMVTPM
> -	tpm-y += tpm_eventlog.o tpm_of.o
> +	tpm-y += tpm_eventlog_init.o tpm_eventlog.o tpm_of.o
>  endif

Still need to fix this, more like

tpm_of should be included if CONFIG_OF is set,
and tpm_acpi if CONFIG_ACPI is set, do not do this based on random
other configus..


>  endif
>  obj-$(CONFIG_TCG_TIS_CORE) += tpm_tis_core.o
> diff --git a/drivers/char/tpm/tpm-chip.c b/drivers/char/tpm/tpm-chip.c
> index e595013..7f6cdab 100644
> +++ b/drivers/char/tpm/tpm-chip.c
> @@ -171,6 +171,8 @@ struct tpm_chip *tpm_chip_alloc(struct device *dev,
>  	chip->dev.release = tpm_dev_release;
>  	chip->dev.parent = dev;
>  	chip->dev.groups = chip->groups;
> +	if (dev->of_node)
> +		chip->dev.of_node = dev->of_node;

No. chip->dev.parent->of_node.

> -extern struct dentry **tpm_bios_log_setup(const char *);
> -extern void tpm_bios_log_teardown(struct dentry **);
> +extern void tpm_bios_log_setup(struct tpm_chip *chip);
> +extern void tpm_bios_log_teardown(struct tpm_chip *chip);

We are trying to get rid of these extra externs..

> +
> +	bin_file =
> +	    securityfs_create_file("binary_bios_measurements",

No, do

chip->bios_dir_count = 0;
chip->bios_dir[chip->bios_dir_count] = [..]
if (is_bad(chip->bios_dir[chip->bios_dir_count])
  goto err
chip->bios_dir_count++

err:
for (I = 0; I != chip->bios_dir_count; ++I)
   securityfs_remove(chip->bios_dir[I]);

> +	for (i = 0; i < 3; i++) {
> +		if (chip->bios_dir[i])
> +			securityfs_remove(chip->bios_dir[i]);

Same logic as err: example above
>  
> -	np = of_find_node_by_name(NULL, "vtpm");
> +	if (chip->dev.of_node)
> +		np = chip->dev.of_node;
>  	if (!np) {
> -		pr_err("%s: ERROR - IBMVTPM not supported\n", __func__);
> +		dev_dbg(&chip->dev, "%s: ERROR - IBMVTPM not supported\n",
> +		__func__);
>  		return -ENODEV;
>  	}

Where you able to test this on IBM's 'vtpm' stuff as well?

>  
>  	sizep = of_get_property(np, "linux,sml-size", NULL);
>  	if (sizep == NULL) {
> -		pr_err("%s: ERROR - SML size not found\n", __func__);
> +		dev_dbg(&chip->dev, "%s: ERROR - SML size not found\n",
> +		__func__);
>  		goto cleanup_eio;
>  	}
>  	if (*sizep == 0) {
> -		pr_err("%s: ERROR - event log area empty\n", __func__);
> +		dev_dbg(&chip->dev, "%s: ERROR - event log area empty\n",
> +		__func__);
>  		goto cleanup_eio;
>  	}
>  
>  	basep = of_get_property(np, "linux,sml-base", NULL);
>  	if (basep == NULL) {
> -		pr_err("%s: ERROR - SML not found\n", __func__);
> +		dev_dbg(&chip->dev, "%s: ERROR - SML not found\n",
> +		__func__);
>  		goto cleanup_eio;
>  	}
>  
>  	log->bios_event_log = kmalloc(*sizep, GFP_KERNEL);
>  	if (!log->bios_event_log) {
> -		pr_err("%s: ERROR - Not enough memory for BIOS measurements\n",
> -		       __func__);
>  		of_node_put(np);

Why is there an of_node_put here but not in other error paths? Where is
the get this is putting?

Jason

------------------------------------------------------------------------------
What NetFlow Analyzer can do for you? Monitors network bandwidth and traffic
patterns at an interface-level. Reveals which users, apps, and protocols are 
consuming the most bandwidth. Provides multi-vendor support for NetFlow, 
J-Flow, sFlow and other flows. Make informed decisions using capacity 
planning reports. http://sdm.link/zohodev2dev

Re: [PATCH v2 1/3] TPM2.0: Refactored eventlog init functions.

[Jarkko Sakkinen]

On Tue, Aug 09, 2016 at 04:27:40PM -0600, Jason Gunthorpe wrote:
> On Tue, Aug 09, 2016 at 03:34:53PM -0400, Nayna Jain wrote:
> > Refactored eventlog.c file into tpm_eventlog.c and tpm_eventlog_init.c
> > 
> > Breakdown is:
> > 
> > * tpm_eventlog_init.c : Moved eventlog initialization methods like
> > to setup securityfs, to open and release seqfile from tpm_eventlog.c
> > to this file. This is to keep the logic of initialization for TPM1.2
> > and TPM2.0 in common file.
> > 
> > * tpm_eventlog.c : This file now has only methods specific to parsing
> > and iterate TPM1.2 entry log formats. It can understand only TPM1.2
> > and is called by methods in tpm_eventlog_init if identified TPM device
> > is TPM1.2.
> > 
> > Changelog v2:
> > 
> >         * Using of_node property of device rather than direct reading
> >         the device node.
> >         * Cleaned up the code to have generic open() for ascii and bios
> >         measurements
> >         * Removed dyncamic allocation for bios_dir and having dentry array
> > 	directly into tpm-chip.
> >         * Using dev_dbg instead of pr_err in tpm_of.c
> >         * readlog(...) now accepts struct tpm_chip * as parameter.
> 
> This patch needs to be split.
> 
> One patch per idea please.

And changelog per patch *set*.

/Jarkko

> >  ifdef CONFIG_ACPI
> > -	tpm-y += tpm_eventlog.o tpm_acpi.o
> > +	tpm-y += tpm_eventlog_init.o tpm_eventlog.o tpm_acpi.o
> >  else
> >  ifdef CONFIG_TCG_IBMVTPM
> > -	tpm-y += tpm_eventlog.o tpm_of.o
> > +	tpm-y += tpm_eventlog_init.o tpm_eventlog.o tpm_of.o
> >  endif
> 
> Still need to fix this, more like
> 
> tpm_of should be included if CONFIG_OF is set,
> and tpm_acpi if CONFIG_ACPI is set, do not do this based on random
> other configus..
> 
> 
> >  endif
> >  obj-$(CONFIG_TCG_TIS_CORE) += tpm_tis_core.o
> > diff --git a/drivers/char/tpm/tpm-chip.c b/drivers/char/tpm/tpm-chip.c
> > index e595013..7f6cdab 100644
> > +++ b/drivers/char/tpm/tpm-chip.c
> > @@ -171,6 +171,8 @@ struct tpm_chip *tpm_chip_alloc(struct device *dev,
> >  	chip->dev.release = tpm_dev_release;
> >  	chip->dev.parent = dev;
> >  	chip->dev.groups = chip->groups;
> > +	if (dev->of_node)
> > +		chip->dev.of_node = dev->of_node;
> 
> No. chip->dev.parent->of_node.
> 
> > -extern struct dentry **tpm_bios_log_setup(const char *);
> > -extern void tpm_bios_log_teardown(struct dentry **);
> > +extern void tpm_bios_log_setup(struct tpm_chip *chip);
> > +extern void tpm_bios_log_teardown(struct tpm_chip *chip);
> 
> We are trying to get rid of these extra externs..
> 
> > +
> > +	bin_file =
> > +	    securityfs_create_file("binary_bios_measurements",
> 
> No, do
> 
> chip->bios_dir_count = 0;
> chip->bios_dir[chip->bios_dir_count] = [..]
> if (is_bad(chip->bios_dir[chip->bios_dir_count])
>   goto err
> chip->bios_dir_count++
> 
> err:
> for (I = 0; I != chip->bios_dir_count; ++I)
>    securityfs_remove(chip->bios_dir[I]);
> 
> > +	for (i = 0; i < 3; i++) {
> > +		if (chip->bios_dir[i])
> > +			securityfs_remove(chip->bios_dir[i]);
> 
> Same logic as err: example above
> >  
> > -	np = of_find_node_by_name(NULL, "vtpm");
> > +	if (chip->dev.of_node)
> > +		np = chip->dev.of_node;
> >  	if (!np) {
> > -		pr_err("%s: ERROR - IBMVTPM not supported\n", __func__);
> > +		dev_dbg(&chip->dev, "%s: ERROR - IBMVTPM not supported\n",
> > +		__func__);
> >  		return -ENODEV;
> >  	}
> 
> Where you able to test this on IBM's 'vtpm' stuff as well?
> 
> >  
> >  	sizep = of_get_property(np, "linux,sml-size", NULL);
> >  	if (sizep == NULL) {
> > -		pr_err("%s: ERROR - SML size not found\n", __func__);
> > +		dev_dbg(&chip->dev, "%s: ERROR - SML size not found\n",
> > +		__func__);
> >  		goto cleanup_eio;
> >  	}
> >  	if (*sizep == 0) {
> > -		pr_err("%s: ERROR - event log area empty\n", __func__);
> > +		dev_dbg(&chip->dev, "%s: ERROR - event log area empty\n",
> > +		__func__);
> >  		goto cleanup_eio;
> >  	}
> >  
> >  	basep = of_get_property(np, "linux,sml-base", NULL);
> >  	if (basep == NULL) {
> > -		pr_err("%s: ERROR - SML not found\n", __func__);
> > +		dev_dbg(&chip->dev, "%s: ERROR - SML not found\n",
> > +		__func__);
> >  		goto cleanup_eio;
> >  	}
> >  
> >  	log->bios_event_log = kmalloc(*sizep, GFP_KERNEL);
> >  	if (!log->bios_event_log) {
> > -		pr_err("%s: ERROR - Not enough memory for BIOS measurements\n",
> > -		       __func__);
> >  		of_node_put(np);
> 
> Why is there an of_node_put here but not in other error paths? Where is
> the get this is putting?
> 
> Jason
> 
> ------------------------------------------------------------------------------
> What NetFlow Analyzer can do for you? Monitors network bandwidth and traffic
> patterns at an interface-level. Reveals which users, apps, and protocols are 
> consuming the most bandwidth. Provides multi-vendor support for NetFlow, 
> J-Flow, sFlow and other flows. Make informed decisions using capacity 
> planning reports. http://sdm.link/zohodev2dev
> _______________________________________________
> tpmdd-devel mailing list
> tpmdd-devel-5NWGOfrQmneRv+LV9MX5uipxlwaOVQ5f@public.gmane.org
> https://lists.sourceforge.net/lists/listinfo/tpmdd-devel

------------------------------------------------------------------------------
What NetFlow Analyzer can do for you? Monitors network bandwidth and traffic
patterns at an interface-level. Reveals which users, apps, and protocols are 
consuming the most bandwidth. Provides multi-vendor support for NetFlow, 
J-Flow, sFlow and other flows. Make informed decisions using capacity 
planning reports. http://sdm.link/zohodev2dev

Re: [PATCH v2 1/3] TPM2.0: Refactored eventlog init functions.

[Nayna]

Thanks for reviewing.

Sure, I will post next version with split as two patches and other fixes 
as suggested. Below is the breakdown of two patches, let me know if this 
doesn't sound ok.

1. First patch to clean up the code related to tpm_eventlog_init.c - 
generic open() and bios_dir as dentry array.

2. Second patch to have changes related to using of_node property and 
struct tpm_chip in tpm_of.c. Includes adding CONFIG_OF.

And one feedback which I didn't understand and so need your help with 
that is

 >> -extern struct dentry **tpm_bios_log_setup(const char *);
 >> -extern void tpm_bios_log_teardown(struct dentry **);
 >> +extern void tpm_bios_log_setup(struct tpm_chip *chip);
 >> +extern void tpm_bios_log_teardown(struct tpm_chip *chip);
 >
 > We are trying to get rid of these extra externs..

This is currently called by tpm_chip_register to setup the bios log.
So, what did it meant by getting rid of these ?

Thanks & Regards,
    - Nayna

On 08/10/2016 03:57 AM, Jason Gunthorpe wrote:
> On Tue, Aug 09, 2016 at 03:34:53PM -0400, Nayna Jain wrote:
>> Refactored eventlog.c file into tpm_eventlog.c and tpm_eventlog_init.c
>>
>> Breakdown is:
>>
>> * tpm_eventlog_init.c : Moved eventlog initialization methods like
>> to setup securityfs, to open and release seqfile from tpm_eventlog.c
>> to this file. This is to keep the logic of initialization for TPM1.2
>> and TPM2.0 in common file.
>>
>> * tpm_eventlog.c : This file now has only methods specific to parsing
>> and iterate TPM1.2 entry log formats. It can understand only TPM1.2
>> and is called by methods in tpm_eventlog_init if identified TPM device
>> is TPM1.2.
>>
>> Changelog v2:
>>
>>          * Using of_node property of device rather than direct reading
>>          the device node.
>>          * Cleaned up the code to have generic open() for ascii and bios
>>          measurements
>>          * Removed dyncamic allocation for bios_dir and having dentry array
>> 	directly into tpm-chip.
>>          * Using dev_dbg instead of pr_err in tpm_of.c
>>          * readlog(...) now accepts struct tpm_chip * as parameter.
>
> This patch needs to be split.
>
> One patch per idea please.
>
>>   ifdef CONFIG_ACPI
>> -	tpm-y += tpm_eventlog.o tpm_acpi.o
>> +	tpm-y += tpm_eventlog_init.o tpm_eventlog.o tpm_acpi.o
>>   else
>>   ifdef CONFIG_TCG_IBMVTPM
>> -	tpm-y += tpm_eventlog.o tpm_of.o
>> +	tpm-y += tpm_eventlog_init.o tpm_eventlog.o tpm_of.o
>>   endif
>
> Still need to fix this, more like
>
> tpm_of should be included if CONFIG_OF is set,
> and tpm_acpi if CONFIG_ACPI is set, do not do this based on random
> other configus..
>
>
>>   endif
>>   obj-$(CONFIG_TCG_TIS_CORE) += tpm_tis_core.o
>> diff --git a/drivers/char/tpm/tpm-chip.c b/drivers/char/tpm/tpm-chip.c
>> index e595013..7f6cdab 100644
>> +++ b/drivers/char/tpm/tpm-chip.c
>> @@ -171,6 +171,8 @@ struct tpm_chip *tpm_chip_alloc(struct device *dev,
>>   	chip->dev.release = tpm_dev_release;
>>   	chip->dev.parent = dev;
>>   	chip->dev.groups = chip->groups;
>> +	if (dev->of_node)
>> +		chip->dev.of_node = dev->of_node;
>
> No. chip->dev.parent->of_node.
>
>> -extern struct dentry **tpm_bios_log_setup(const char *);
>> -extern void tpm_bios_log_teardown(struct dentry **);
>> +extern void tpm_bios_log_setup(struct tpm_chip *chip);
>> +extern void tpm_bios_log_teardown(struct tpm_chip *chip);
>
> We are trying to get rid of these extra externs..
>
>> +
>> +	bin_file =
>> +	    securityfs_create_file("binary_bios_measurements",
>
> No, do
>
> chip->bios_dir_count = 0;
> chip->bios_dir[chip->bios_dir_count] = [..]
> if (is_bad(chip->bios_dir[chip->bios_dir_count])
>    goto err
> chip->bios_dir_count++
>
> err:
> for (I = 0; I != chip->bios_dir_count; ++I)
>     securityfs_remove(chip->bios_dir[I]);
>
>> +	for (i = 0; i < 3; i++) {
>> +		if (chip->bios_dir[i])
>> +			securityfs_remove(chip->bios_dir[i]);
>
> Same logic as err: example above
>>
>> -	np = of_find_node_by_name(NULL, "vtpm");
>> +	if (chip->dev.of_node)
>> +		np = chip->dev.of_node;
>>   	if (!np) {
>> -		pr_err("%s: ERROR - IBMVTPM not supported\n", __func__);
>> +		dev_dbg(&chip->dev, "%s: ERROR - IBMVTPM not supported\n",
>> +		__func__);
>>   		return -ENODEV;
>>   	}
>
> Where you able to test this on IBM's 'vtpm' stuff as well?
>
>>
>>   	sizep = of_get_property(np, "linux,sml-size", NULL);
>>   	if (sizep == NULL) {
>> -		pr_err("%s: ERROR - SML size not found\n", __func__);
>> +		dev_dbg(&chip->dev, "%s: ERROR - SML size not found\n",
>> +		__func__);
>>   		goto cleanup_eio;
>>   	}
>>   	if (*sizep == 0) {
>> -		pr_err("%s: ERROR - event log area empty\n", __func__);
>> +		dev_dbg(&chip->dev, "%s: ERROR - event log area empty\n",
>> +		__func__);
>>   		goto cleanup_eio;
>>   	}
>>
>>   	basep = of_get_property(np, "linux,sml-base", NULL);
>>   	if (basep == NULL) {
>> -		pr_err("%s: ERROR - SML not found\n", __func__);
>> +		dev_dbg(&chip->dev, "%s: ERROR - SML not found\n",
>> +		__func__);
>>   		goto cleanup_eio;
>>   	}
>>
>>   	log->bios_event_log = kmalloc(*sizep, GFP_KERNEL);
>>   	if (!log->bios_event_log) {
>> -		pr_err("%s: ERROR - Not enough memory for BIOS measurements\n",
>> -		       __func__);
>>   		of_node_put(np);
>
> Why is there an of_node_put here but not in other error paths? Where is
> the get this is putting?
>
> Jason
>


------------------------------------------------------------------------------
What NetFlow Analyzer can do for you? Monitors network bandwidth and traffic
patterns at an interface-level. Reveals which users, apps, and protocols are 
consuming the most bandwidth. Provides multi-vendor support for NetFlow, 
J-Flow, sFlow and other flows. Make informed decisions using capacity 
planning reports. http://sdm.link/zohodev2dev

Re: [PATCH v2 1/3] TPM2.0: Refactored eventlog init functions.

[Jarkko Sakkinen]

On Tue, Aug 09, 2016 at 03:34:53PM -0400, Nayna Jain wrote:
> Refactored eventlog.c file into tpm_eventlog.c and tpm_eventlog_init.c
> 
> Breakdown is:
> 
> * tpm_eventlog_init.c : Moved eventlog initialization methods like
> to setup securityfs, to open and release seqfile from tpm_eventlog.c
> to this file. This is to keep the logic of initialization for TPM1.2
> and TPM2.0 in common file.
> 
> * tpm_eventlog.c : This file now has only methods specific to parsing
> and iterate TPM1.2 entry log formats. It can understand only TPM1.2
> and is called by methods in tpm_eventlog_init if identified TPM device
> is TPM1.2.

I'm not going to review this in the current form. I would rather see a
clean one paragraph "why" and "how" description and the commit do only
one thing.

That said I NAK the split. It adds more complexity than brings value in
this case.

/Jarkko

> Changelog v2:
> 
>         * Using of_node property of device rather than direct reading
>         the device node.
>         * Cleaned up the code to have generic open() for ascii and bios
>         measurements
>         * Removed dyncamic allocation for bios_dir and having dentry array
> 	directly into tpm-chip.
>         * Using dev_dbg instead of pr_err in tpm_of.c
>         * readlog(...) now accepts struct tpm_chip * as parameter.
> 
> 
> Signed-off-by: Nayna Jain <nayna-23VcF4HTsmIX0ybBhKVfKdBPR1lH4CV8@public.gmane.org>
> ---
>  drivers/char/tpm/Makefile            |   4 +-
>  drivers/char/tpm/tpm-chip.c          |   6 +-
>  drivers/char/tpm/tpm.h               |   2 +-
>  drivers/char/tpm/tpm_acpi.c          |   2 +-
>  drivers/char/tpm/tpm_eventlog.c      | 156 +----------------------------------
>  drivers/char/tpm/tpm_eventlog.h      |  16 ++--
>  drivers/char/tpm/tpm_eventlog_init.c | 155 ++++++++++++++++++++++++++++++++++
>  drivers/char/tpm/tpm_of.c            |  22 +++--
>  8 files changed, 189 insertions(+), 174 deletions(-)
>  create mode 100644 drivers/char/tpm/tpm_eventlog_init.c
> 
> diff --git a/drivers/char/tpm/Makefile b/drivers/char/tpm/Makefile
> index a385fb8..9136762 100644
> --- a/drivers/char/tpm/Makefile
> +++ b/drivers/char/tpm/Makefile
> @@ -6,10 +6,10 @@ tpm-y := tpm-interface.o tpm-dev.o tpm-sysfs.o tpm-chip.o tpm2-cmd.o
>  tpm-$(CONFIG_ACPI) += tpm_ppi.o
>  
>  ifdef CONFIG_ACPI
> -	tpm-y += tpm_eventlog.o tpm_acpi.o
> +	tpm-y += tpm_eventlog_init.o tpm_eventlog.o tpm_acpi.o
>  else
>  ifdef CONFIG_TCG_IBMVTPM
> -	tpm-y += tpm_eventlog.o tpm_of.o
> +	tpm-y += tpm_eventlog_init.o tpm_eventlog.o tpm_of.o
>  endif
>  endif
>  obj-$(CONFIG_TCG_TIS_CORE) += tpm_tis_core.o
> diff --git a/drivers/char/tpm/tpm-chip.c b/drivers/char/tpm/tpm-chip.c
> index e595013..7f6cdab 100644
> --- a/drivers/char/tpm/tpm-chip.c
> +++ b/drivers/char/tpm/tpm-chip.c
> @@ -171,6 +171,8 @@ struct tpm_chip *tpm_chip_alloc(struct device *dev,
>  	chip->dev.release = tpm_dev_release;
>  	chip->dev.parent = dev;
>  	chip->dev.groups = chip->groups;
> +	if (dev->of_node)
> +		chip->dev.of_node = dev->of_node;
>  
>  	if (chip->dev_num == 0)
>  		chip->dev.devt = MKDEV(MISC_MAJOR, TPM_MINOR);
> @@ -283,7 +285,7 @@ static int tpm1_chip_register(struct tpm_chip *chip)
>  
>  	tpm_sysfs_add_device(chip);
>  
> -	chip->bios_dir = tpm_bios_log_setup(dev_name(&chip->dev));
> +	tpm_bios_log_setup(chip);
>  
>  	return 0;
>  }
> @@ -294,7 +296,7 @@ static void tpm1_chip_unregister(struct tpm_chip *chip)
>  		return;
>  
>  	if (chip->bios_dir)
> -		tpm_bios_log_teardown(chip->bios_dir);
> +		tpm_bios_log_teardown(chip);
>  }
>  
>  static void tpm_del_legacy_sysfs(struct tpm_chip *chip)
> diff --git a/drivers/char/tpm/tpm.h b/drivers/char/tpm/tpm.h
> index 6e002c4..cfa408f 100644
> --- a/drivers/char/tpm/tpm.h
> +++ b/drivers/char/tpm/tpm.h
> @@ -171,7 +171,7 @@ struct tpm_chip {
>  	unsigned long duration[3]; /* jiffies */
>  	bool duration_adjusted;
>  
> -	struct dentry **bios_dir;
> +	struct dentry *bios_dir[3];
>  
>  	const struct attribute_group *groups[3];
>  	unsigned int groups_cnt;
> diff --git a/drivers/char/tpm/tpm_acpi.c b/drivers/char/tpm/tpm_acpi.c
> index 565a947..c2a122a 100644
> --- a/drivers/char/tpm/tpm_acpi.c
> +++ b/drivers/char/tpm/tpm_acpi.c
> @@ -45,7 +45,7 @@ struct acpi_tcpa {
>  };
>  
>  /* read binary bios log */
> -int read_log(struct tpm_bios_log *log)
> +int read_log(struct tpm_bios_log *log, struct tpm_chip *chip)
>  {
>  	struct acpi_tcpa *buff;
>  	acpi_status status;
> diff --git a/drivers/char/tpm/tpm_eventlog.c b/drivers/char/tpm/tpm_eventlog.c
> index e722886..b8f22ec 100644
> --- a/drivers/char/tpm/tpm_eventlog.c
> +++ b/drivers/char/tpm/tpm_eventlog.c
> @@ -1,5 +1,5 @@
>  /*
> - * Copyright (C) 2005, 2012 IBM Corporation
> + * Copyright (C) 2005, 2012, 2016 IBM Corporation
>   *
>   * Authors:
>   *	Kent Yoder <key-23VcF4HTsmIX0ybBhKVfKdBPR1lH4CV8@public.gmane.org>
> @@ -11,6 +11,7 @@
>   * Maintained by: <tpmdd-devel-5NWGOfrQmneRv+LV9MX5uipxlwaOVQ5f@public.gmane.org>
>   *
>   * Access to the eventlog created by a system's firmware / BIOS
> + * specific to TPM 1.2.
>   *
>   * This program is free software; you can redistribute it and/or
>   * modify it under the terms of the GNU General Public License
> @@ -257,20 +258,6 @@ static int tpm_binary_bios_measurements_show(struct seq_file *m, void *v)
>  
>  }
>  
> -static int tpm_bios_measurements_release(struct inode *inode,
> -					 struct file *file)
> -{
> -	struct seq_file *seq = file->private_data;
> -	struct tpm_bios_log *log = seq->private;
> -
> -	if (log) {
> -		kfree(log->bios_event_log);
> -		kfree(log);
> -	}
> -
> -	return seq_release(inode, file);
> -}
> -
>  static int tpm_ascii_bios_measurements_show(struct seq_file *m, void *v)
>  {
>  	int len = 0;
> @@ -304,151 +291,16 @@ static int tpm_ascii_bios_measurements_show(struct seq_file *m, void *v)
>  	return 0;
>  }
>  
> -static const struct seq_operations tpm_ascii_b_measurments_seqops = {
> +const struct seq_operations tpm_ascii_b_measurments_seqops = {
>  	.start = tpm_bios_measurements_start,
>  	.next = tpm_bios_measurements_next,
>  	.stop = tpm_bios_measurements_stop,
>  	.show = tpm_ascii_bios_measurements_show,
>  };
>  
> -static const struct seq_operations tpm_binary_b_measurments_seqops = {
> +const struct seq_operations tpm_binary_b_measurments_seqops = {
>  	.start = tpm_bios_measurements_start,
>  	.next = tpm_bios_measurements_next,
>  	.stop = tpm_bios_measurements_stop,
>  	.show = tpm_binary_bios_measurements_show,
>  };
> -
> -static int tpm_ascii_bios_measurements_open(struct inode *inode,
> -					    struct file *file)
> -{
> -	int err;
> -	struct tpm_bios_log *log;
> -	struct seq_file *seq;
> -
> -	log = kzalloc(sizeof(struct tpm_bios_log), GFP_KERNEL);
> -	if (!log)
> -		return -ENOMEM;
> -
> -	if ((err = read_log(log)))
> -		goto out_free;
> -
> -	/* now register seq file */
> -	err = seq_open(file, &tpm_ascii_b_measurments_seqops);
> -	if (!err) {
> -		seq = file->private_data;
> -		seq->private = log;
> -	} else {
> -		goto out_free;
> -	}
> -
> -out:
> -	return err;
> -out_free:
> -	kfree(log->bios_event_log);
> -	kfree(log);
> -	goto out;
> -}
> -
> -static const struct file_operations tpm_ascii_bios_measurements_ops = {
> -	.open = tpm_ascii_bios_measurements_open,
> -	.read = seq_read,
> -	.llseek = seq_lseek,
> -	.release = tpm_bios_measurements_release,
> -};
> -
> -static int tpm_binary_bios_measurements_open(struct inode *inode,
> -					     struct file *file)
> -{
> -	int err;
> -	struct tpm_bios_log *log;
> -	struct seq_file *seq;
> -
> -	log = kzalloc(sizeof(struct tpm_bios_log), GFP_KERNEL);
> -	if (!log)
> -		return -ENOMEM;
> -
> -	if ((err = read_log(log)))
> -		goto out_free;
> -
> -	/* now register seq file */
> -	err = seq_open(file, &tpm_binary_b_measurments_seqops);
> -	if (!err) {
> -		seq = file->private_data;
> -		seq->private = log;
> -	} else {
> -		goto out_free;
> -	}
> -
> -out:
> -	return err;
> -out_free:
> -	kfree(log->bios_event_log);
> -	kfree(log);
> -	goto out;
> -}
> -
> -static const struct file_operations tpm_binary_bios_measurements_ops = {
> -	.open = tpm_binary_bios_measurements_open,
> -	.read = seq_read,
> -	.llseek = seq_lseek,
> -	.release = tpm_bios_measurements_release,
> -};
> -
> -static int is_bad(void *p)
> -{
> -	if (!p)
> -		return 1;
> -	if (IS_ERR(p) && (PTR_ERR(p) != -ENODEV))
> -		return 1;
> -	return 0;
> -}
> -
> -struct dentry **tpm_bios_log_setup(const char *name)
> -{
> -	struct dentry **ret = NULL, *tpm_dir, *bin_file, *ascii_file;
> -
> -	tpm_dir = securityfs_create_dir(name, NULL);
> -	if (is_bad(tpm_dir))
> -		goto out;
> -
> -	bin_file =
> -	    securityfs_create_file("binary_bios_measurements",
> -				   S_IRUSR | S_IRGRP, tpm_dir, NULL,
> -				   &tpm_binary_bios_measurements_ops);
> -	if (is_bad(bin_file))
> -		goto out_tpm;
> -
> -	ascii_file =
> -	    securityfs_create_file("ascii_bios_measurements",
> -				   S_IRUSR | S_IRGRP, tpm_dir, NULL,
> -				   &tpm_ascii_bios_measurements_ops);
> -	if (is_bad(ascii_file))
> -		goto out_bin;
> -
> -	ret = kmalloc(3 * sizeof(struct dentry *), GFP_KERNEL);
> -	if (!ret)
> -		goto out_ascii;
> -
> -	ret[0] = ascii_file;
> -	ret[1] = bin_file;
> -	ret[2] = tpm_dir;
> -
> -	return ret;
> -
> -out_ascii:
> -	securityfs_remove(ascii_file);
> -out_bin:
> -	securityfs_remove(bin_file);
> -out_tpm:
> -	securityfs_remove(tpm_dir);
> -out:
> -	return NULL;
> -}
> -
> -void tpm_bios_log_teardown(struct dentry **lst)
> -{
> -	int i;
> -
> -	for (i = 0; i < 3; i++)
> -		securityfs_remove(lst[i]);
> -}
> diff --git a/drivers/char/tpm/tpm_eventlog.h b/drivers/char/tpm/tpm_eventlog.h
> index 8de62b0..b888c77 100644
> --- a/drivers/char/tpm/tpm_eventlog.h
> +++ b/drivers/char/tpm/tpm_eventlog.h
> @@ -1,4 +1,3 @@
> -
>  #ifndef __TPM_EVENTLOG_H__
>  #define __TPM_EVENTLOG_H__
>  
> @@ -12,6 +11,9 @@
>  #define do_endian_conversion(x) x
>  #endif
>  
> +extern const struct seq_operations tpm_ascii_b_measurments_seqops;
> +extern const struct seq_operations tpm_binary_b_measurments_seqops;
> +
>  enum bios_platform_class {
>  	BIOS_CLIENT = 0x00,
>  	BIOS_SERVER = 0x01,
> @@ -73,18 +75,18 @@ enum tcpa_pc_event_ids {
>  	HOST_TABLE_OF_DEVICES,
>  };
>  
> -int read_log(struct tpm_bios_log *log);
> +int read_log(struct tpm_bios_log *log, struct tpm_chip *chip);
>  
>  #if defined(CONFIG_TCG_IBMVTPM) || defined(CONFIG_TCG_IBMVTPM_MODULE) || \
>  	defined(CONFIG_ACPI)
> -extern struct dentry **tpm_bios_log_setup(const char *);
> -extern void tpm_bios_log_teardown(struct dentry **);
> +extern void tpm_bios_log_setup(struct tpm_chip *chip);
> +extern void tpm_bios_log_teardown(struct tpm_chip *chip);
>  #else
> -static inline struct dentry **tpm_bios_log_setup(const char *name)
> +static inline void tpm_bios_log_setup(struct tpm_chip *chip)
>  {
> -	return NULL;
> +	return;
>  }
> -static inline void tpm_bios_log_teardown(struct dentry **dir)
> +static inline void tpm_bios_log_teardown(struct tpm_chip *chip)
>  {
>  }
>  #endif
> diff --git a/drivers/char/tpm/tpm_eventlog_init.c b/drivers/char/tpm/tpm_eventlog_init.c
> new file mode 100644
> index 0000000..dd5dbc4
> --- /dev/null
> +++ b/drivers/char/tpm/tpm_eventlog_init.c
> @@ -0,0 +1,155 @@
> +/*
> + * Copyright (C) 2005, 2012, 2016 IBM Corporation
> + *
> + * Authors:
> + *	Kent Yoder <key-23VcF4HTsmIX0ybBhKVfKdBPR1lH4CV8@public.gmane.org>
> + *	Seiji Munetoh <munetoh-JE5g2YyFxFHQT0dZR+AlfA@public.gmane.org>
> + *	Stefan Berger <stefanb-r/Jw6+rmf7HQT0dZR+AlfA@public.gmane.org>
> + *	Reiner Sailer <sailer-aZOuKsOsJu3MbYB6QlFGEg@public.gmane.org>
> + *	Kylene Hall <kjhall-r/Jw6+rmf7HQT0dZR+AlfA@public.gmane.org>
> + *	Nayna Jain <nayna-23VcF4HTsmIX0ybBhKVfKdBPR1lH4CV8@public.gmane.org>
> + *
> + * Maintained by: <tpmdd-devel-5NWGOfrQmneRv+LV9MX5uipxlwaOVQ5f@public.gmane.org>
> + *
> + * TPM 1.2 and TPM 2.0 common initialization methods to
> + * access the eventlog created by a system's firmware / BIOS.
> + *
> + * This program is free software; you can redistribute it and/or
> + * modify it under the terms of the GNU General Public License
> + * as published by the Free Software Foundation; either version
> + * 2 of the License, or (at your option) any later version.
> + *
> + */
> +
> +#include <linux/seq_file.h>
> +#include <linux/fs.h>
> +#include <linux/security.h>
> +#include <linux/module.h>
> +#include <linux/slab.h>
> +
> +#include "tpm.h"
> +#include "tpm_eventlog.h"
> +
> +
> +static int tpm_bios_measurements_release(struct inode *inode,
> +					 struct file *file)
> +{
> +	struct seq_file *seq = file->private_data;
> +	struct tpm_bios_log *log = seq->private;
> +
> +	if (log) {
> +		kfree(log->bios_event_log);
> +		kfree(log);
> +	}
> +
> +	return seq_release(inode, file);
> +}
> +
> +static int tpm_bios_measurements_open(struct inode *inode,
> +					    struct file *file)
> +{
> +	int err;
> +	struct tpm_bios_log *log;
> +	struct seq_file *seq;
> +	struct tpm_chip *chip;
> +	const struct seq_operations *seqops = (struct seq_operations
> +	*)inode->i_private;
> +
> +	log = kzalloc(sizeof(struct tpm_bios_log), GFP_KERNEL);
> +	if (!log)
> +		return -ENOMEM;
> +
> +	chip = (struct tpm_chip
> +	*)file->f_path.dentry->d_parent->d_inode->i_private;
> +
> +	err = read_log(log, chip);
> +	if (err)
> +		goto out_free;
> +
> +	/* now register seq file */
> +	err = seq_open(file, seqops);
> +	if (!err) {
> +		seq = file->private_data;
> +		seq->private = log;
> +	} else {
> +		goto out_free;
> +	}
> +
> +out:
> +	return err;
> +out_free:
> +	kfree(log->bios_event_log);
> +	kfree(log);
> +	goto out;
> +}
> +
> +static const struct file_operations tpm_bios_measurements_ops = {
> +	.open = tpm_bios_measurements_open,
> +	.read = seq_read,
> +	.llseek = seq_lseek,
> +	.release = tpm_bios_measurements_release,
> +};
> +
> +static int is_bad(void *p)
> +{
> +	if (!p)
> +		return 1;
> +	if (IS_ERR(p) && (PTR_ERR(p) != -ENODEV))
> +		return 1;
> +	return 0;
> +}
> +
> +void tpm_bios_log_setup(struct tpm_chip *chip)
> +{
> +	struct dentry *tpm_dir, *bin_file, *ascii_file;
> +	const char *name = dev_name(&chip->dev);
> +	int i;
> +
> +	for (i = 0; i < 3; i++)
> +		chip->bios_dir[i] = NULL;
> +
> +	tpm_dir = securityfs_create_dir(name, NULL);
> +	if (is_bad(tpm_dir))
> +		goto out;
> +
> +	tpm_dir->d_inode->i_private = chip;
> +
> +	bin_file =
> +	    securityfs_create_file("binary_bios_measurements",
> +				   S_IRUSR | S_IRGRP, tpm_dir,
> +				   (void *)&tpm_binary_b_measurments_seqops,
> +				   &tpm_bios_measurements_ops);
> +	if (is_bad(bin_file))
> +		goto out_tpm;
> +
> +	ascii_file =
> +	    securityfs_create_file("ascii_bios_measurements",
> +				   S_IRUSR | S_IRGRP, tpm_dir,
> +				   (void *)&tpm_ascii_b_measurments_seqops,
> +				   &tpm_bios_measurements_ops);
> +	if (is_bad(ascii_file))
> +		goto out_bin;
> +
> +	chip->bios_dir[0] = ascii_file;
> +	chip->bios_dir[1] = bin_file;
> +	chip->bios_dir[2] = tpm_dir;
> +
> +	return;
> +
> +out_bin:
> +	securityfs_remove(bin_file);
> +out_tpm:
> +	securityfs_remove(tpm_dir);
> +out:
> +	return;
> +}
> +
> +void tpm_bios_log_teardown(struct tpm_chip *chip)
> +{
> +	int i;
> +
> +	for (i = 0; i < 3; i++) {
> +		if (chip->bios_dir[i])
> +			securityfs_remove(chip->bios_dir[i]);
> +	}
> +}
> diff --git a/drivers/char/tpm/tpm_of.c b/drivers/char/tpm/tpm_of.c
> index 570f30c..30a9905 100644
> --- a/drivers/char/tpm/tpm_of.c
> +++ b/drivers/char/tpm/tpm_of.c
> @@ -1,7 +1,8 @@
>  /*
> - * Copyright 2012 IBM Corporation
> + * Copyright 2012, 2016 IBM Corporation
>   *
>   * Author: Ashley Lai <ashleydlai-Re5JQEeQqe8AvxtiuMwx3w@public.gmane.org>
> + *         Nayna Jain <nayna-23VcF4HTsmIX0ybBhKVfKdBPR1lH4CV8@public.gmane.org>
>   *
>   * Maintained by: <tpmdd-devel-5NWGOfrQmneRv+LV9MX5uipxlwaOVQ5f@public.gmane.org>
>   *
> @@ -20,7 +21,7 @@
>  #include "tpm.h"
>  #include "tpm_eventlog.h"
>  
> -int read_log(struct tpm_bios_log *log)
> +int read_log(struct tpm_bios_log *log, struct tpm_chip *chip)
>  {
>  	struct device_node *np;
>  	const u32 *sizep;
> @@ -31,32 +32,35 @@ int read_log(struct tpm_bios_log *log)
>  		return -EFAULT;
>  	}
>  
> -	np = of_find_node_by_name(NULL, "vtpm");
> +	if (chip->dev.of_node)
> +		np = chip->dev.of_node;
>  	if (!np) {
> -		pr_err("%s: ERROR - IBMVTPM not supported\n", __func__);
> +		dev_dbg(&chip->dev, "%s: ERROR - IBMVTPM not supported\n",
> +		__func__);
>  		return -ENODEV;
>  	}
>  
>  	sizep = of_get_property(np, "linux,sml-size", NULL);
>  	if (sizep == NULL) {
> -		pr_err("%s: ERROR - SML size not found\n", __func__);
> +		dev_dbg(&chip->dev, "%s: ERROR - SML size not found\n",
> +		__func__);
>  		goto cleanup_eio;
>  	}
>  	if (*sizep == 0) {
> -		pr_err("%s: ERROR - event log area empty\n", __func__);
> +		dev_dbg(&chip->dev, "%s: ERROR - event log area empty\n",
> +		__func__);
>  		goto cleanup_eio;
>  	}
>  
>  	basep = of_get_property(np, "linux,sml-base", NULL);
>  	if (basep == NULL) {
> -		pr_err("%s: ERROR - SML not found\n", __func__);
> +		dev_dbg(&chip->dev, "%s: ERROR - SML not found\n",
> +		__func__);
>  		goto cleanup_eio;
>  	}
>  
>  	log->bios_event_log = kmalloc(*sizep, GFP_KERNEL);
>  	if (!log->bios_event_log) {
> -		pr_err("%s: ERROR - Not enough memory for BIOS measurements\n",
> -		       __func__);
>  		of_node_put(np);
>  		return -ENOMEM;
>  	}
> -- 
> 2.5.0
> 
> 
> ------------------------------------------------------------------------------
> What NetFlow Analyzer can do for you? Monitors network bandwidth and traffic
> patterns at an interface-level. Reveals which users, apps, and protocols are 
> consuming the most bandwidth. Provides multi-vendor support for NetFlow, 
> J-Flow, sFlow and other flows. Make informed decisions using capacity 
> planning reports. http://sdm.link/zohodev2dev
> _______________________________________________
> tpmdd-devel mailing list
> tpmdd-devel-5NWGOfrQmneRv+LV9MX5uipxlwaOVQ5f@public.gmane.org
> https://lists.sourceforge.net/lists/listinfo/tpmdd-devel

------------------------------------------------------------------------------
What NetFlow Analyzer can do for you? Monitors network bandwidth and traffic
patterns at an interface-level. Reveals which users, apps, and protocols are 
consuming the most bandwidth. Provides multi-vendor support for NetFlow, 
J-Flow, sFlow and other flows. Make informed decisions using capacity 
planning reports. http://sdm.link/zohodev2dev

Re: [PATCH v2 3/3] TPM2.0:Adds securityfs support for TPM2.0 eventlog

[Jarkko Sakkinen]

On Tue, Aug 09, 2016 at 03:34:55PM -0400, Nayna Jain wrote:
> Adds securityfs support for TPM2.0.
> This patch currently supports only binary_bios_measurements.
> 
> Changelog v2:
>         * Single tpm_of.c for reading both tpm and vtpm device tree values.
>         * Some of the issues are fixed in Patch 1 itself.
>         * Comments in tpm2.h give reference to the standard from where structs
> 	are taken.
> 	* Now, tpm_of.c has same code applied for both tpm and vtpm, so I think
> 	that now it is needed to have generic types rather than endian specific type.
> 
> There are few preexisting issues as being mentioned in feedback and are not 
> addressed in this patch. Reason being, I don't have much expertise of ACPI side as of now, 
> and these changes will affect acpi,tpm,vtpm, all paths, so I would like to go slow
> and fix them as different patch later after better understanding.
> Hope this sounds ok to have them as different patch.
> 
> Issues which are not addressed are as below:
>         * tpm_eventlog.h still has #ifdef defined, for tpm_bios_log_setup()
>         * tpm_bios_log_setup is still being called in tpm-chip register function.

I do not understand your changelog entries. Please provide a nice one or
most two paragraph english language description and keep breakdowns and
changelogs in the cover letter.

Commit message does not equal to a discussion forum and I do not have
any idea what feedback you are talking about...

Not reviewing this further. This is just terrible.

/Jarkko

> Signed-off-by: Nayna Jain <nayna-23VcF4HTsmIX0ybBhKVfKdBPR1lH4CV8@public.gmane.org>
> ---
>  drivers/char/tpm/Makefile            |   8 +-
>  drivers/char/tpm/tpm-chip.c          |  20 +---
>  drivers/char/tpm/tpm2.h              |  85 +++++++++++++
>  drivers/char/tpm/tpm2_eventlog.c     | 224 +++++++++++++++++++++++++++++++++++
>  drivers/char/tpm/tpm_eventlog.h      |   2 +-
>  drivers/char/tpm/tpm_eventlog_init.c |  43 +++++--
>  drivers/char/tpm/tpm_of.c            |  17 ++-
>  7 files changed, 366 insertions(+), 33 deletions(-)
>  create mode 100644 drivers/char/tpm/tpm2.h
>  create mode 100644 drivers/char/tpm/tpm2_eventlog.c
> 
> diff --git a/drivers/char/tpm/Makefile b/drivers/char/tpm/Makefile
> index 9136762..509ace2 100644
> --- a/drivers/char/tpm/Makefile
> +++ b/drivers/char/tpm/Makefile
> @@ -6,10 +6,14 @@ tpm-y := tpm-interface.o tpm-dev.o tpm-sysfs.o tpm-chip.o tpm2-cmd.o
>  tpm-$(CONFIG_ACPI) += tpm_ppi.o
>  
>  ifdef CONFIG_ACPI
> -	tpm-y += tpm_eventlog_init.o tpm_eventlog.o tpm_acpi.o
> +	tpm-y += tpm_eventlog_init.o tpm2_eventlog.o tpm_eventlog.o tpm_acpi.o
>  else
>  ifdef CONFIG_TCG_IBMVTPM
> -	tpm-y += tpm_eventlog_init.o tpm_eventlog.o tpm_of.o
> +	tpm-y += tpm_eventlog_init.o tpm2_eventlog.o tpm_eventlog.o tpm_of.o
> +else
> +ifdef CONFIG_PPC64
> +	tpm-y += tpm_eventlog_init.o tpm2_eventlog.o tpm_eventlog.o tpm_of.o
> +endif
>  endif
>  endif
>  obj-$(CONFIG_TCG_TIS_CORE) += tpm_tis_core.o
> diff --git a/drivers/char/tpm/tpm-chip.c b/drivers/char/tpm/tpm-chip.c
> index 7f6cdab..3f1c489 100644
> --- a/drivers/char/tpm/tpm-chip.c
> +++ b/drivers/char/tpm/tpm-chip.c
> @@ -285,20 +285,9 @@ static int tpm1_chip_register(struct tpm_chip *chip)
>  
>  	tpm_sysfs_add_device(chip);
>  
> -	tpm_bios_log_setup(chip);
> -
>  	return 0;
>  }
>  
> -static void tpm1_chip_unregister(struct tpm_chip *chip)
> -{
> -	if (chip->flags & TPM_CHIP_FLAG_TPM2)
> -		return;
> -
> -	if (chip->bios_dir)
> -		tpm_bios_log_teardown(chip);
> -}
> -
>  static void tpm_del_legacy_sysfs(struct tpm_chip *chip)
>  {
>  	struct attribute **i;
> @@ -372,10 +361,8 @@ int tpm_chip_register(struct tpm_chip *chip)
>  	tpm_add_ppi(chip);
>  
>  	rc = tpm_add_char_device(chip);
> -	if (rc) {
> -		tpm1_chip_unregister(chip);
> +	if (rc)
>  		return rc;
> -	}
>  
>  	chip->flags |= TPM_CHIP_FLAG_REGISTERED;
>  
> @@ -385,6 +372,8 @@ int tpm_chip_register(struct tpm_chip *chip)
>  		return rc;
>  	}
>  
> +	tpm_bios_log_setup(chip);
> +
>  	return 0;
>  }
>  EXPORT_SYMBOL_GPL(tpm_chip_register);
> @@ -409,7 +398,8 @@ void tpm_chip_unregister(struct tpm_chip *chip)
>  
>  	tpm_del_legacy_sysfs(chip);
>  
> -	tpm1_chip_unregister(chip);
> +	tpm_bios_log_teardown(chip);
> +
>  	tpm_del_char_device(chip);
>  }
>  EXPORT_SYMBOL_GPL(tpm_chip_unregister);
> diff --git a/drivers/char/tpm/tpm2.h b/drivers/char/tpm/tpm2.h
> new file mode 100644
> index 0000000..38a8073
> --- /dev/null
> +++ b/drivers/char/tpm/tpm2.h
> @@ -0,0 +1,85 @@
> +#ifndef __TPM2_H__
> +#define __TPM2_H__
> +
> +#define TPM_ALG_SHA1_DIGEST_SIZE	20
> +#define TPM_ALG_SHA256_DIGEST_SIZE	32
> +#define TPM_ALG_SHA384_DIGEST_SIZE	48
> +
> +#define HASH_COUNT	3
> +#define MAX_TPM_LOG_MSG	128
> +
> +/**
> + * All the structures related to Event Log are taken from TCG EFI Protocol
> + * Specification, Family "2.0". Document is available on link
> + * http://www.trustedcomputinggroup.org/tcg-efi-protocol-specification/
> + * Information is also available on TCG PC Client Platform Firmware Profile
> + * Specification, Family "2.0"
> + * Detailed digest structures for TPM2.0 are defined in document
> + * Trusted Platform Module Library Part 2: Structures, Family "2.0".
> + */
> +
> +/* Event log header algorithm spec. */
> +struct tcg_efispecideventalgorithmsize {
> +	u16	algorithm_id;
> +	u16	digest_size;
> +} __packed;
> +
> +/* Event log header data. */
> +struct tcg_efispecideventstruct {
> +	u8					signature[16];
> +	u32					platform_class;
> +	u8					spec_version_minor;
> +	u8					spec_version_major;
> +	u8					spec_errata;
> +	u8					uintnsize;
> +	u32					num_algs;
> +	struct tcg_efispecideventalgorithmsize	digest_sizes[HASH_COUNT];
> +	u8					vendor_info_size;
> +	u8					vendor_info[0];
> +} __packed;
> +
> +/* Header entry for eventlog. */
> +struct tcg_pcr_event {
> +	u32	pcr_index;
> +	u32	event_type;
> +	u8	digest[20];
> +	u32	event_size;
> +	u8	event[MAX_TPM_LOG_MSG];
> +} __packed;
> +
> +/* Digest union for crypto agility. */
> +union tpmu_ha {
> +	u8	 sha1[TPM_ALG_SHA1_DIGEST_SIZE];
> +	u8	 sha256[TPM_ALG_SHA256_DIGEST_SIZE];
> +	u8	 sha384[TPM_ALG_SHA384_DIGEST_SIZE];
> +} __packed;
> +
> +/* Crypto Agile algorithm and respective digest. */
> +struct tpmt_ha {
> +	u16		algorithm_id;
> +	union tpmu_ha	digest;
> +} __packed;
> +
> +/* Crypto agile digests list. */
> +struct tpml_digest_values {
> +	u32		count;
> +	struct tpmt_ha	digests[HASH_COUNT];
> +} __packed;
> +
> +/* Event field structure. */
> +struct tcg_event_field {
> +	u32	event_size;
> +	u8      event[MAX_TPM_LOG_MSG];
> +} __packed;
> +
> +/* Crypto agile log entry format for TPM 2.0. */
> +struct tcg_pcr_event2 {
> +	u32				pcr_index;
> +	u32				event_type;
> +	struct tpml_digest_values	digests;
> +	struct tcg_event_field		event;
> +} __packed;
> +
> +extern const struct seq_operations tpm2_binary_b_measurments_seqops;
> +
> +#endif
> diff --git a/drivers/char/tpm/tpm2_eventlog.c b/drivers/char/tpm/tpm2_eventlog.c
> new file mode 100644
> index 0000000..f1e8c4a
> --- /dev/null
> +++ b/drivers/char/tpm/tpm2_eventlog.c
> @@ -0,0 +1,224 @@
> +/*
> + * Copyright (C) 2016 IBM Corporation
> + *
> + * Authors:
> + *      Nayna Jain <nayna-23VcF4HTsmIX0ybBhKVfKdBPR1lH4CV8@public.gmane.org>
> +
> + * Access to TPM2.0 event log as written by Firmware.
> + * It assumes that writer of event log has followed TCG Spec 2.0
> + * has written the event struct data in little endian. With that,
> + * it doesn't need any endian conversion for structure content.
> + *
> + * This program is free software; you can redistribute it and/or
> + * modify it under the terms of the GNU General Public License
> + * as published by the Free Software Foundation; either version
> + * 2 of the License, or (at your option) any later version.
> + */
> +
> +#include <linux/seq_file.h>
> +#include <linux/fs.h>
> +#include <linux/security.h>
> +#include <linux/module.h>
> +#include <linux/slab.h>
> +
> +#include "tpm.h"
> +#include "tpm2.h"
> +#include "tpm_eventlog.h"
> +
> +
> +static int calc_tpm2_event_size(struct tcg_pcr_event2 *event,
> +		struct tcg_pcr_event *event_header)
> +{
> +	struct tcg_efispecideventstruct *efispecid;
> +	struct tcg_event_field *event_field;
> +	void *marker, *marker_start;
> +	int j;
> +	size_t size = 0;
> +
> +	/*
> +	 * NOTE: TPM2.0 allows support for extend to multiple PCR Banks.
> +	 * This implies that eventlog also has multiple digest values
> +	 * one for each PCR Bank. This is called Crypto Agile Log Entry
> +	 * Format. Current support implementation is for SHA1 and SHA256.
> +	 * Number of digest values are identified by parsing first
> +	 * structure stored in event log also called event header.
> +	 * Further, the eventlog is written in packed form so to calculate
> +	 * offset it is important to know the type of algorithm used.
> +	 * Eg. 1:
> +	 * digest_values.count = 1;
> +	 * digest_values.digest[0].algorithm_id = sha1;
> +	 * digest_values.digest[0].digest.sha1 = {20 bytes raw data};
> +	 * Offset of eventsize is sizeof(count) + sizeof(algorithm_id) + 20
> +	 *
> +	 * Eg. 2:
> +	 * digest_values.count = 1;
> +	 * digest_values.digest[0].algorithm_id = sha256;
> +	 * digest_values.digest[0].digest.sha1 = {32 bytes raw data};
> +	 * Offset of eventsize is sizeof(count) + sizeof(algorithm_id) + 32
> +
> +	 * Eg. 3:
> +	 * digest_values.count = 2;
> +	 * digest_values.digest[0].algorithm_id = sha1;
> +	 * digest_values.digest[0].digest.sha1 = {20 bytes raw data};
> +	 * digest_values.digest[1].algorithm_id = sha256;
> +	 * digest_values.digest[1].digest.sha256 = {32 bytes raw data};
> +	 * Offset of eventsize is sizeof(count) + sizeof(algorithm_id) + 20
> +	 *			+ sizeof(algorithm_id) + 32;
> +	 *
> +	 * So, it implies that offset of event_size can vary based on digest
> +	 * values as defined by vendor. And so we have to calculate the
> +	 * offset by parsing through number and type of digests being used.
> +	 * And this is the purpose of using *marker to traverse the structure
> +	 * and calculate the offset of event_size. This function uses *marker
> +	 * to parse and calculate the dynamic size of the whole event structure.
> +	 */
> +
> +	marker = event;
> +	marker_start = marker;
> +	marker = marker + sizeof(event->pcr_index) + sizeof(event->event_type)
> +		+ sizeof(event->digests.count);
> +
> +	efispecid = (struct tcg_efispecideventstruct *) event_header->event;
> +
> +	for (j = 0; (j < efispecid->num_algs) && (j < HASH_COUNT); j++) {
> +		marker = marker
> +			+ sizeof(efispecid->digest_sizes[j].algorithm_id);
> +		marker = marker + efispecid->digest_sizes[j].digest_size;
> +	}
> +
> +	event_field = (struct tcg_event_field *) marker;
> +	marker = marker + sizeof(event_field->event_size)
> +		+ event_field->event_size;
> +	size = marker - marker_start;
> +
> +	if ((event->event_type == 0) && (event_field->event_size == 0))
> +		return 0;
> +
> +	return size;
> +}
> +
> +static void *tpm2_bios_measurements_start(struct seq_file *m, loff_t *pos)
> +{
> +	struct tpm_bios_log *log = m->private;
> +	void *addr = log->bios_event_log;
> +	void *limit = log->bios_event_log_end;
> +	struct tcg_pcr_event *event_header;
> +	struct tcg_pcr_event2 *event;
> +	int i;
> +	size_t size = 0;
> +
> +	event_header = addr;
> +
> +	size = sizeof(struct tcg_pcr_event) - sizeof(event_header->event)
> +		+ event_header->event_size;
> +
> +
> +	if (*pos == 0) {
> +		if (addr + size < limit) {
> +			if ((event_header->event_type == 0) &&
> +					(event_header->event_size == 0))
> +				return NULL;
> +			return SEQ_START_TOKEN;
> +		}
> +	}
> +
> +	if (*pos > 0) {
> +		addr += size;
> +		event = addr;
> +		size = calc_tpm2_event_size(event, event_header);
> +		if ((addr + size >=  limit) || (size == 0))
> +			return NULL;
> +	}
> +
> +	/* read over *pos measurements */
> +	for (i = 0; i < (*pos - 1); i++) {
> +		event = addr;
> +		size = calc_tpm2_event_size(event, event_header);
> +
> +		if ((addr + size >= limit) || (size == 0))
> +			return NULL;
> +		addr += size;
> +	}
> +
> +	return addr;
> +}
> +
> +static void *tpm2_bios_measurements_next(struct seq_file *m, void *v,
> +		loff_t *pos)
> +{
> +	struct tcg_pcr_event *event_header;
> +	struct tcg_pcr_event2 *event;
> +	struct tpm_bios_log *log = m->private;
> +	void *limit = log->bios_event_log_end;
> +	void *marker;
> +	size_t event_size = 0;
> +
> +	event_header = log->bios_event_log;
> +
> +	if (v == SEQ_START_TOKEN) {
> +		event_size = sizeof(struct tcg_pcr_event)
> +			- sizeof(event_header->event)
> +			+ event_header->event_size;
> +		marker = event_header;
> +	} else {
> +		event = v;
> +		event_size = calc_tpm2_event_size(event, event_header);
> +		if (event_size == 0)
> +			return NULL;
> +		marker =  event;
> +	}
> +
> +	marker = marker + event_size;
> +	if (marker >= limit)
> +		return NULL;
> +	v = marker;
> +	event = v;
> +
> +	event_size = calc_tpm2_event_size(event, event_header);
> +	if (((v + event_size) >= limit) || (event_size == 0))
> +		return NULL;
> +
> +	(*pos)++;
> +	return v;
> +}
> +
> +static void tpm2_bios_measurements_stop(struct seq_file *m, void *v)
> +{
> +}
> +
> +static int tpm2_binary_bios_measurements_show(struct seq_file *m, void *v)
> +{
> +	struct tpm_bios_log *log = m->private;
> +	struct tcg_pcr_event *event_header = log->bios_event_log;
> +	struct tcg_pcr_event2 *event = v;
> +	void *temp_ptr;
> +	size_t size = 0;
> +
> +	if (v == SEQ_START_TOKEN) {
> +
> +		size = sizeof(struct tcg_pcr_event)
> +			- sizeof(event_header->event)
> +			+ event_header->event_size;
> +
> +		temp_ptr = event_header;
> +
> +		if (size > 0)
> +			seq_write(m, temp_ptr, size);
> +	} else {
> +
> +		size = calc_tpm2_event_size(event, event_header);
> +
> +		temp_ptr = event;
> +		if (size > 0)
> +			seq_write(m, temp_ptr, size);
> +	}
> +
> +	return 0;
> +}
> +
> +const struct seq_operations tpm2_binary_b_measurments_seqops = {
> +	.start = tpm2_bios_measurements_start,
> +	.next = tpm2_bios_measurements_next,
> +	.stop = tpm2_bios_measurements_stop,
> +	.show = tpm2_binary_bios_measurements_show,
> +};
> diff --git a/drivers/char/tpm/tpm_eventlog.h b/drivers/char/tpm/tpm_eventlog.h
> index b888c77..3297fda 100644
> --- a/drivers/char/tpm/tpm_eventlog.h
> +++ b/drivers/char/tpm/tpm_eventlog.h
> @@ -78,7 +78,7 @@ enum tcpa_pc_event_ids {
>  int read_log(struct tpm_bios_log *log, struct tpm_chip *chip);
>  
>  #if defined(CONFIG_TCG_IBMVTPM) || defined(CONFIG_TCG_IBMVTPM_MODULE) || \
> -	defined(CONFIG_ACPI)
> +	defined(CONFIG_ACPI) || defined(CONFIG_PPC64)
>  extern void tpm_bios_log_setup(struct tpm_chip *chip);
>  extern void tpm_bios_log_teardown(struct tpm_chip *chip);
>  #else
> diff --git a/drivers/char/tpm/tpm_eventlog_init.c b/drivers/char/tpm/tpm_eventlog_init.c
> index dd5dbc4..a3fb5bc 100644
> --- a/drivers/char/tpm/tpm_eventlog_init.c
> +++ b/drivers/char/tpm/tpm_eventlog_init.c
> @@ -28,6 +28,7 @@
>  #include <linux/slab.h>
>  
>  #include "tpm.h"
> +#include "tpm2.h"
>  #include "tpm_eventlog.h"
>  
>  
> @@ -103,36 +104,54 @@ void tpm_bios_log_setup(struct tpm_chip *chip)
>  {
>  	struct dentry *tpm_dir, *bin_file, *ascii_file;
>  	const char *name = dev_name(&chip->dev);
> +	void *seq_ops;
> +	int num_files = 0;
>  	int i;
>  
>  	for (i = 0; i < 3; i++)
>  		chip->bios_dir[i] = NULL;
>  
> +	/*
> +	 * NOTE: Currently, support is added only for binary_bios_measurements
> +	 * for TPM2.0. And so it is required to check that if device is TPM2,
> +	 * only 2 entries are allocated i.e. tpm0 dir and
> +	 * binary_bios_measurements file.
> +	 */
> +
> +	num_files = chip->flags & TPM_CHIP_FLAG_TPM2 ? 2 : 3;
> +
>  	tpm_dir = securityfs_create_dir(name, NULL);
>  	if (is_bad(tpm_dir))
>  		goto out;
>  
>  	tpm_dir->d_inode->i_private = chip;
>  
> +	if (chip->flags & TPM_CHIP_FLAG_TPM2)
> +		seq_ops = (void *)&tpm2_binary_b_measurments_seqops;
> +	else
> +		seq_ops = (void *)&tpm_binary_b_measurments_seqops;
> +
>  	bin_file =
>  	    securityfs_create_file("binary_bios_measurements",
>  				   S_IRUSR | S_IRGRP, tpm_dir,
> -				   (void *)&tpm_binary_b_measurments_seqops,
> +				   seq_ops,
>  				   &tpm_bios_measurements_ops);
>  	if (is_bad(bin_file))
>  		goto out_tpm;
>  
> -	ascii_file =
> -	    securityfs_create_file("ascii_bios_measurements",
> -				   S_IRUSR | S_IRGRP, tpm_dir,
> -				   (void *)&tpm_ascii_b_measurments_seqops,
> -				   &tpm_bios_measurements_ops);
> -	if (is_bad(ascii_file))
> -		goto out_bin;
> -
> -	chip->bios_dir[0] = ascii_file;
> -	chip->bios_dir[1] = bin_file;
> -	chip->bios_dir[2] = tpm_dir;
> +	chip->bios_dir[num_files-1] = tpm_dir;
> +	chip->bios_dir[num_files-2] = bin_file;
> +
> +	if (num_files == 3) {
> +		ascii_file =
> +			securityfs_create_file("ascii_bios_measurements",
> +					S_IRUSR | S_IRGRP, tpm_dir,
> +					(void *)&tpm_ascii_b_measurments_seqops,
> +					&tpm_bios_measurements_ops);
> +		if (is_bad(ascii_file))
> +			goto out_bin;
> +		chip->bios_dir[num_files-3] = ascii_file;
> +	}
>  
>  	return;
>  
> diff --git a/drivers/char/tpm/tpm_of.c b/drivers/char/tpm/tpm_of.c
> index 30a9905..d3c72ce 100644
> --- a/drivers/char/tpm/tpm_of.c
> +++ b/drivers/char/tpm/tpm_of.c
> @@ -26,6 +26,7 @@ int read_log(struct tpm_bios_log *log, struct tpm_chip *chip)
>  	struct device_node *np;
>  	const u32 *sizep;
>  	const u64 *basep;
> +	u32 log_size;
>  
>  	if (log->bios_event_log != NULL) {
>  		pr_err("%s: ERROR - Eventlog already initialized\n", __func__);
> @@ -52,6 +53,11 @@ int read_log(struct tpm_bios_log *log, struct tpm_chip *chip)
>  		goto cleanup_eio;
>  	}
>  
> +	if (!strncmp(np->name, "tpm", 3))
> +		log_size = be32_to_cpup(sizep);
> +	else
> +		log_size = *sizep;
> +
>  	basep = of_get_property(np, "linux,sml-base", NULL);
>  	if (basep == NULL) {
>  		dev_dbg(&chip->dev, "%s: ERROR - SML not found\n",
> @@ -59,15 +65,20 @@ int read_log(struct tpm_bios_log *log, struct tpm_chip *chip)
>  		goto cleanup_eio;
>  	}
>  
> -	log->bios_event_log = kmalloc(*sizep, GFP_KERNEL);
> +	log->bios_event_log = kmalloc(log_size, GFP_KERNEL);
>  	if (!log->bios_event_log) {
>  		of_node_put(np);
>  		return -ENOMEM;
>  	}
>  
> -	log->bios_event_log_end = log->bios_event_log + *sizep;
> +	log->bios_event_log_end = log->bios_event_log + log_size;
> +
> +	if (!strncmp(np->name, "tpm", 3))
> +		memcpy(log->bios_event_log, __va(be64_to_cpup(basep)),
> +		log_size);
> +	else
> +		memcpy(log->bios_event_log, __va(*basep), log_size);
>  
> -	memcpy(log->bios_event_log, __va(*basep), *sizep);
>  	of_node_put(np);
>  
>  	return 0;
> -- 
> 2.5.0
> 
> 
> ------------------------------------------------------------------------------
> What NetFlow Analyzer can do for you? Monitors network bandwidth and traffic
> patterns at an interface-level. Reveals which users, apps, and protocols are 
> consuming the most bandwidth. Provides multi-vendor support for NetFlow, 
> J-Flow, sFlow and other flows. Make informed decisions using capacity 
> planning reports. http://sdm.link/zohodev2dev
> _______________________________________________
> tpmdd-devel mailing list
> tpmdd-devel-5NWGOfrQmneRv+LV9MX5uipxlwaOVQ5f@public.gmane.org
> https://lists.sourceforge.net/lists/listinfo/tpmdd-devel

------------------------------------------------------------------------------
What NetFlow Analyzer can do for you? Monitors network bandwidth and traffic
patterns at an interface-level. Reveals which users, apps, and protocols are 
consuming the most bandwidth. Provides multi-vendor support for NetFlow, 
J-Flow, sFlow and other flows. Make informed decisions using capacity 
planning reports. http://sdm.link/zohodev2dev

Re: [PATCH v2 3/3] TPM2.0:Adds securityfs support for TPM2.0 eventlog

[Jarkko Sakkinen]

On Wed, Aug 10, 2016 at 02:25:30PM +0300, Jarkko Sakkinen wrote:
> On Tue, Aug 09, 2016 at 03:34:55PM -0400, Nayna Jain wrote:
> > Adds securityfs support for TPM2.0.
> > This patch currently supports only binary_bios_measurements.
> > 
> > Changelog v2:
> >         * Single tpm_of.c for reading both tpm and vtpm device tree values.
> >         * Some of the issues are fixed in Patch 1 itself.
> >         * Comments in tpm2.h give reference to the standard from where structs
> > 	are taken.
> > 	* Now, tpm_of.c has same code applied for both tpm and vtpm, so I think
> > 	that now it is needed to have generic types rather than endian specific type.
> > 
> > There are few preexisting issues as being mentioned in feedback and are not 
> > addressed in this patch. Reason being, I don't have much expertise of ACPI side as of now, 
> > and these changes will affect acpi,tpm,vtpm, all paths, so I would like to go slow
> > and fix them as different patch later after better understanding.
> > Hope this sounds ok to have them as different patch.
> > 
> > Issues which are not addressed are as below:
> >         * tpm_eventlog.h still has #ifdef defined, for tpm_bios_log_setup()
> >         * tpm_bios_log_setup is still being called in tpm-chip register function.
> 
> I do not understand your changelog entries. Please provide a nice one or
> most two paragraph english language description and keep breakdowns and
> changelogs in the cover letter.
> 
> Commit message does not equal to a discussion forum and I do not have
> any idea what feedback you are talking about...
> 
> Not reviewing this further. This is just terrible.

Whoops, my email client did tricks for me. Please ignore the two other
responses from me.

/Jarkko

------------------------------------------------------------------------------
What NetFlow Analyzer can do for you? Monitors network bandwidth and traffic
patterns at an interface-level. Reveals which users, apps, and protocols are 
consuming the most bandwidth. Provides multi-vendor support for NetFlow, 
J-Flow, sFlow and other flows. Make informed decisions using capacity 
planning reports. http://sdm.link/zohodev2dev

Re: [PATCH v2 2/3] TPM2.0: TPM Device Tree Documentation

[Jarkko Sakkinen]

On Tue, Aug 09, 2016 at 03:34:54PM -0400, Nayna Jain wrote:
> Added device tree binding documentation for I2C base TPM.

Where is this binding defined originally? Is it standardized?

/Jarkko

> Changelog v2:
>         * Added documentation in v2.
> 
> Signed-off-by: Nayna Jain <nayna-23VcF4HTsmIX0ybBhKVfKdBPR1lH4CV8@public.gmane.org>
> ---
>  Documentation/devicetree/bindings/i2c/i2c-tpm.txt | 31 +++++++++++++++++++++++
>  1 file changed, 31 insertions(+)
>  create mode 100644 Documentation/devicetree/bindings/i2c/i2c-tpm.txt
> 
> diff --git a/Documentation/devicetree/bindings/i2c/i2c-tpm.txt b/Documentation/devicetree/bindings/i2c/i2c-tpm.txt
> new file mode 100644
> index 0000000..1df05cf
> --- /dev/null
> +++ b/Documentation/devicetree/bindings/i2c/i2c-tpm.txt
> @@ -0,0 +1,31 @@
> +Device Tree Bindings for I2C based Trusted Platform Module(TPM)
> +---------------------------------------------------------------
> +
> +This node describes a TPM device connected to Processor on i2c bus.
> +
> +Required properties:
> +
> +- compatible : 'manufacturer,model'
> +- label : "tpm" indicates master TPM; "tpm-backup" indicates backup TPM.
> +- linux,sml-base : base address of the Event Log. It is a physical address.
> +		   sml stands for shared memory log.
> +- linux,sml-size : size of the memory allocated for the Event Log.
> +
> +Optional properties:
> +
> +- status: indicates whether the device is enabled or disabled. "okay" for
> +          enabled and "disabled" for disabled.
> +
> +Example
> +-------
> +
> +tpm@57 {
> +	reg = <0x57>;
> +	label = "tpm";
> +	compatible = "nuvoton,npct650", "nuvoton,npct601";
> +	linux,sml-base = <0x7f 0xfd450000>;
> +	linux,sml-size = <0x10000>;
> +	status = "okay";
> +	phandle = <0x10000017>;
> +	linux,phandle = <0x10000017>;
> +};
> -- 
> 2.5.0
> 
> 
> ------------------------------------------------------------------------------
> What NetFlow Analyzer can do for you? Monitors network bandwidth and traffic
> patterns at an interface-level. Reveals which users, apps, and protocols are 
> consuming the most bandwidth. Provides multi-vendor support for NetFlow, 
> J-Flow, sFlow and other flows. Make informed decisions using capacity 
> planning reports. http://sdm.link/zohodev2dev
> _______________________________________________
> tpmdd-devel mailing list
> tpmdd-devel-5NWGOfrQmneRv+LV9MX5uipxlwaOVQ5f@public.gmane.org
> https://lists.sourceforge.net/lists/listinfo/tpmdd-devel

------------------------------------------------------------------------------
What NetFlow Analyzer can do for you? Monitors network bandwidth and traffic
patterns at an interface-level. Reveals which users, apps, and protocols are 
consuming the most bandwidth. Provides multi-vendor support for NetFlow, 
J-Flow, sFlow and other flows. Make informed decisions using capacity 
planning reports. http://sdm.link/zohodev2dev

Re: [PATCH v2 0/3] TPM2.0: Added eventlog support for TPM2.0

[Jarkko Sakkinen]

On Tue, Aug 09, 2016 at 03:34:52PM -0400, Nayna Jain wrote:
> Overview:
> =========
> 
> This patch adds support for enabling securityfs for TPM2.0, currently
> driver has eventlog support only for TPM1.2.
> The patch currently adds support for only binary_bios_measurements.
> 
> The structure for TPM2.0 is compliant with TCG Spec for 2.0 family.
> Also , the reading of data has the assumption that writer would have
> followed TCG Spec and so everything is in little-endian.
> 
> The tpm device driver code has been refactored to:
> * Identify the TPM version - 1.2 or 2.0
> * Calls corresponding compatible seq_ops for iterating over eventlog.
> 
> Files Description:
> ===================
> 
> * tpm-chip.c : Adds call to setup bios log for TPM2.0.
> 
> * tpm2_of.c : Reads the device tree entries to find the location
> and size of event.
> 
> * tpm_eventlog_init.c : Provides common initialization functions
>  between TPM2.0 and TPM1.2 to setup securityfs entries and seq_ops
>   iterator.  The functions has been moved from tpm_eventlog.c into this file.
> 
>   * tpm_eventlog.c : Provides functions only specific to TPM1.2
>   version. Common initialization functions are moved to tpm_eventlog_init.c
> 
>   * tpm2_eventlog.c : Provides functions specific only for TPM2.0
>   eventlog format.
> 
>   * tpm2.h : Header file for TPM2.0 structures and functions.

diffstat shows changed files. Your commit messages will give detailed
descriptions. Please remove this. Cover letter would be the right place
to tell about missing ACPI support.

BTW, how this can be tested?

/Jarkko

> Changelog v2:
> =============
> 
> 	* Fixes issues as given in feedback by Jason.
> 	* Adds documentation for device tree.
> 
> Nayna Jain (3):
>   TPM2.0: Refactored eventlog init functions.
>   TPM2.0: TPM Device Tree Documentation
>   TPM2.0:Adds securityfs support for TPM2.0 eventlog
> 
>  Documentation/devicetree/bindings/i2c/i2c-tpm.txt |  31 +++
>  drivers/char/tpm/Makefile                         |   8 +-
>  drivers/char/tpm/tpm-chip.c                       |  22 +--
>  drivers/char/tpm/tpm.h                            |   2 +-
>  drivers/char/tpm/tpm2.h                           |  85 ++++++++
>  drivers/char/tpm/tpm2_eventlog.c                  | 224 ++++++++++++++++++++++
>  drivers/char/tpm/tpm_acpi.c                       |   2 +-
>  drivers/char/tpm/tpm_eventlog.c                   | 156 +--------------
>  drivers/char/tpm/tpm_eventlog.h                   |  18 +-
>  drivers/char/tpm/tpm_eventlog_init.c              | 174 +++++++++++++++++
>  drivers/char/tpm/tpm_of.c                         |  39 ++--
>  11 files changed, 570 insertions(+), 191 deletions(-)
>  create mode 100644 Documentation/devicetree/bindings/i2c/i2c-tpm.txt
>  create mode 100644 drivers/char/tpm/tpm2.h
>  create mode 100644 drivers/char/tpm/tpm2_eventlog.c
>  create mode 100644 drivers/char/tpm/tpm_eventlog_init.c
> 
> -- 
> 2.5.0
> 
> 
> ------------------------------------------------------------------------------
> What NetFlow Analyzer can do for you? Monitors network bandwidth and traffic
> patterns at an interface-level. Reveals which users, apps, and protocols are 
> consuming the most bandwidth. Provides multi-vendor support for NetFlow, 
> J-Flow, sFlow and other flows. Make informed decisions using capacity 
> planning reports. http://sdm.link/zohodev2dev
> _______________________________________________
> tpmdd-devel mailing list
> tpmdd-devel-5NWGOfrQmneRv+LV9MX5uipxlwaOVQ5f@public.gmane.org
> https://lists.sourceforge.net/lists/listinfo/tpmdd-devel

------------------------------------------------------------------------------
What NetFlow Analyzer can do for you? Monitors network bandwidth and traffic
patterns at an interface-level. Reveals which users, apps, and protocols are 
consuming the most bandwidth. Provides multi-vendor support for NetFlow, 
J-Flow, sFlow and other flows. Make informed decisions using capacity 
planning reports. http://sdm.link/zohodev2dev

Re: [PATCH v2 0/3] TPM2.0: Added eventlog support for TPM2.0

[Jarkko Sakkinen]

On Wed, Aug 10, 2016 at 02:32:43PM +0300, Jarkko Sakkinen wrote:
> On Tue, Aug 09, 2016 at 03:34:52PM -0400, Nayna Jain wrote:
> > Overview:
> > =========
> > 
> > This patch adds support for enabling securityfs for TPM2.0, currently
> > driver has eventlog support only for TPM1.2.
> > The patch currently adds support for only binary_bios_measurements.
> > 
> > The structure for TPM2.0 is compliant with TCG Spec for 2.0 family.
> > Also , the reading of data has the assumption that writer would have
> > followed TCG Spec and so everything is in little-endian.
> > 
> > The tpm device driver code has been refactored to:
> > * Identify the TPM version - 1.2 or 2.0
> > * Calls corresponding compatible seq_ops for iterating over eventlog.
> > 
> > Files Description:
> > ===================
> > 
> > * tpm-chip.c : Adds call to setup bios log for TPM2.0.
> > 
> > * tpm2_of.c : Reads the device tree entries to find the location
> > and size of event.
> > 
> > * tpm_eventlog_init.c : Provides common initialization functions
> >  between TPM2.0 and TPM1.2 to setup securityfs entries and seq_ops
> >   iterator.  The functions has been moved from tpm_eventlog.c into this file.
> > 
> >   * tpm_eventlog.c : Provides functions only specific to TPM1.2
> >   version. Common initialization functions are moved to tpm_eventlog_init.c
> > 
> >   * tpm2_eventlog.c : Provides functions specific only for TPM2.0
> >   eventlog format.
> > 
> >   * tpm2.h : Header file for TPM2.0 structures and functions.
> 
> diffstat shows changed files. Your commit messages will give detailed
> descriptions. Please remove this. Cover letter would be the right place
> to tell about missing ACPI support.
> 
> BTW, how this can be tested?

As for ACPI you stated that

"Reason being, I don't have much expertise of ACPI side as of now, and
these changes will affect acpi,tpm,vtpm, all paths, so I would like to
go slow and fix them as different patch later after better
understanding."

Don't get me wrong but that really is an unacceptable statement. It
gives very strong evidence that you don't know what you are doing and
the patches are not ready for public consumption.

You have two better alternatives:

1. Do a little bit of research. You would have found that there's no
   ACPI support for the event log defined by TCG at the moment.
2. Ask first. This is the right mailing list to do that.

You do not have to implement ACPI support as part of the patch set but
at least it would be good to research the constraints so that we can
check that the implementation will be best for ACPI too.

/Jarkko

------------------------------------------------------------------------------
What NetFlow Analyzer can do for you? Monitors network bandwidth and traffic
patterns at an interface-level. Reveals which users, apps, and protocols are 
consuming the most bandwidth. Provides multi-vendor support for NetFlow, 
J-Flow, sFlow and other flows. Make informed decisions using capacity 
planning reports. http://sdm.link/zohodev2dev

Re: [PATCH v2 0/3] TPM2.0: Added eventlog support for TPM2.0

[Nayna]

Hi Jarkko/Jason,

Thanks for the review and sharing the perspective, I understand your 
point and will take care from next time.

I will include the feedbacks for code/commit msgs as suggested by both 
of you in next version of patch.

And I do have few questions before fixing the issues I didn't address in 
this patch. Quoting my thoughts with <nayna>

So, there were two feedbacks by Jason.

#1. And the next somewhat pre-existing issue is that we call
tpm_bios_log_setup even if we don't have access to a bios log.

Does the bios log ever change or is it static at boot? Can we move
read_log into the chip and do it once so we can tell if it worked
before creating security fs stuff??

<nayna>

So, by statement "if we don't have access to a bios log?".. Do you mean 
bios log is empty ? or has no properties to access the log.

Regarding bios log, so it is supposed to be written by each component in 
boot stack during boot.
As per moving read_log into the chip, do you mean something like as below ?

struct tpm_chip {
....
     struct tpm_bios_log *bios_log;
....
}

read_log(struct tpm_chip *chip)
{

  allocates to chip->bios_log->bios_event_log
  allocates to chip->bios_log->bios_event_log_end;

  return 0; // for success;

}

tpm_chip_register()
{
...
if (read_log(chip))
    tpm_bios_log_setup(chip);
...
}

And as per readlog is concerned, it just reads the start and max size of 
the log for both ACPI/OF. So, it is possible that readlog returns 
successfully giving start and max allocated size. but actual log size is 
zero. So, when we cat bios_measurements_file, it doesn't show anything. 
  And I think that is fine.

Please let me know if I missed something.

</nayna>

#2.
 >  #if defined(CONFIG_TCG_IBMVTPM) || 
defined(CONFIG_TCG_IBMVTPM_MODULE) || \
 > -	defined(CONFIG_ACPI)
 > -extern struct dentry **tpm_bios_log_setup(const char *);
 > -extern void tpm_bios_log_teardown(struct dentry **);
 > +	defined(CONFIG_ACPI) || defined(CONFIG_PPC64)
 > +extern struct dentry **tpm_bios_log_setup(struct tpm_chip *chip);
 > +extern void tpm_bios_log_teardown(struct tpm_chip *chip);

I'm really deeply unhappy about these ifdefs and the general scheme
that was used to force this special difference into the tpm core.

Please find another way to do it.

IMHO, 'read_log' should simply try ACPI and OF in sequence to find the
log.

<nayna> Can you help me to understand the reason for which it was done 
this way ?

Secondly, did you mean something like (just a sample psudocode)
read_log()
{
read_of_node;
if (read_of_node_fails)
	read_acpi_way
         if (read_acpi_succeeds)
            do acpi_way of reading for start of log and max log size.
	else
            return err;
do of_node way of reading for start of log and max log size
}

So, is it safe to assume that if there is no of_node property , that 
means it is ACPI ?

Thirdly, you also said that "We are trying to get rid of these extra 
externs.." So, was thinking how does fixing readlog will fix these 
externs, because even if readlog is called in tpm-chip, we still need to 
call tpm_bios_log_setup().

So, I guess you meant that inline ones will go away and thereby #ifdef, 
externs will still be there. right ? Please let me know if I am missing 
something. </nayna>

#3. Where you able to test this on IBM's 'vtpm' stuff as well?,

<nayna>This was for patch where I am directly using dev.of_node 
property. Sorry I missed to reply in previous response. And answer is 
"yes", I did test it. </nayna>

Jarrko, You also asked "
 >> BTW, how this can be tested?"

<nayna> So, it can be tested with system having TPM2.0 version of 
tpm/vtpm and its firmware writing eventlog following TCG Spec for TPM2.0.

Jarkko, Please let me know if it doesn't answer your question. </nayna>

Thanks & Regards,
    - Nayna


On 08/10/2016 10:49 PM, Jarkko Sakkinen wrote:
> On Wed, Aug 10, 2016 at 02:32:43PM +0300, Jarkko Sakkinen wrote:
>> On Tue, Aug 09, 2016 at 03:34:52PM -0400, Nayna Jain wrote:
>>> Overview:
>>> =========
>>>
>>> This patch adds support for enabling securityfs for TPM2.0, currently
>>> driver has eventlog support only for TPM1.2.
>>> The patch currently adds support for only binary_bios_measurements.
>>>
>>> The structure for TPM2.0 is compliant with TCG Spec for 2.0 family.
>>> Also , the reading of data has the assumption that writer would have
>>> followed TCG Spec and so everything is in little-endian.
>>>
>>> The tpm device driver code has been refactored to:
>>> * Identify the TPM version - 1.2 or 2.0
>>> * Calls corresponding compatible seq_ops for iterating over eventlog.
>>>
>>> Files Description:
>>> ===================
>>>
>>> * tpm-chip.c : Adds call to setup bios log for TPM2.0.
>>>
>>> * tpm2_of.c : Reads the device tree entries to find the location
>>> and size of event.
>>>
>>> * tpm_eventlog_init.c : Provides common initialization functions
>>>   between TPM2.0 and TPM1.2 to setup securityfs entries and seq_ops
>>>    iterator.  The functions has been moved from tpm_eventlog.c into this file.
>>>
>>>    * tpm_eventlog.c : Provides functions only specific to TPM1.2
>>>    version. Common initialization functions are moved to tpm_eventlog_init.c
>>>
>>>    * tpm2_eventlog.c : Provides functions specific only for TPM2.0
>>>    eventlog format.
>>>
>>>    * tpm2.h : Header file for TPM2.0 structures and functions.
>>
>> diffstat shows changed files. Your commit messages will give detailed
>> descriptions. Please remove this. Cover letter would be the right place
>> to tell about missing ACPI support.
>>
>> BTW, how this can be tested?
>
> As for ACPI you stated that
>
> "Reason being, I don't have much expertise of ACPI side as of now, and
> these changes will affect acpi,tpm,vtpm, all paths, so I would like to
> go slow and fix them as different patch later after better
> understanding."
>
> Don't get me wrong but that really is an unacceptable statement. It
> gives very strong evidence that you don't know what you are doing and
> the patches are not ready for public consumption.
>
> You have two better alternatives:
>
> 1. Do a little bit of research. You would have found that there's no
>     ACPI support for the event log defined by TCG at the moment.
> 2. Ask first. This is the right mailing list to do that.
>
> You do not have to implement ACPI support as part of the patch set but
> at least it would be good to research the constraints so that we can
> check that the implementation will be best for ACPI too.
>
> /Jarkko
>


------------------------------------------------------------------------------
What NetFlow Analyzer can do for you? Monitors network bandwidth and traffic
patterns at an interface-level. Reveals which users, apps, and protocols are 
consuming the most bandwidth. Provides multi-vendor support for NetFlow, 
J-Flow, sFlow and other flows. Make informed decisions using capacity 
planning reports. http://sdm.link/zohodev2dev

Re: [PATCH v2 0/3] TPM2.0: Added eventlog support for TPM2.0

[Jarkko Sakkinen]

On Thu, Aug 11, 2016 at 04:18:34PM +0530, Nayna wrote:

[SNIP too much text out of context]

> Jarrko, You also asked "
> >> BTW, how this can be tested?"
> 
> <nayna> So, it can be tested with system having TPM2.0 version of tpm/vtpm
> and its firmware writing eventlog following TCG Spec for TPM2.0.
> 
> Jarkko, Please let me know if it doesn't answer your question. </nayna>

1. EFI does not pass the log by any means AFAIK before a boot loader
   calls ExitBootServices().
2. I do not have any system with TPM2 that uses DT. And as I stated
   before you didn't have any reference where you derived the DT
   node fields.

> Thanks & Regards,
>    - Nayna

/Jarkko

------------------------------------------------------------------------------
What NetFlow Analyzer can do for you? Monitors network bandwidth and traffic
patterns at an interface-level. Reveals which users, apps, and protocols are 
consuming the most bandwidth. Provides multi-vendor support for NetFlow, 
J-Flow, sFlow and other flows. Make informed decisions using capacity 
planning reports. http://sdm.link/zohodev2dev

Re: [PATCH v2 0/3] TPM2.0: Added eventlog support for TPM2.0

[Nayna]

Hi Jarkko,

My responses inline.

On 08/11/2016 06:28 PM, Jarkko Sakkinen wrote:
> On Thu, Aug 11, 2016 at 04:18:34PM +0530, Nayna wrote:
>
> [SNIP too much text out of context]
>
>> Jarrko, You also asked "
>>>> BTW, how this can be tested?"
>>
>> <nayna> So, it can be tested with system having TPM2.0 version of tpm/vtpm
>> and its firmware writing eventlog following TCG Spec for TPM2.0.
>>
>> Jarkko, Please let me know if it doesn't answer your question. </nayna>
>
> 1. EFI does not pass the log by any means AFAIK before a boot loader
>     calls ExitBootServices().

So, is current TCPA support only for TPM1.2 ?

#2, TCG Spec 
http://www.trustedcomputinggroup.org/wp-content/uploads/EFI-Protocol-Specification-rev13-160330final.pdf 
talks about
EFI_TCG2_PROTOCOL.GetEventLog (Section 6.5), what is that supposed to do ?

> 2. I do not have any system with TPM2 that uses DT. And as I stated
>     before you didn't have any reference where you derived the DT
>     node fields.

As per Device Tree, so this is the new node introduced in the device 
tree to support TPM. And the fields are defined maintaining the 
requirements from ePAPR specification.
>
>> Thanks & Regards,
>>     - Nayna
>
> /Jarkko
>


------------------------------------------------------------------------------
What NetFlow Analyzer can do for you? Monitors network bandwidth and traffic
patterns at an interface-level. Reveals which users, apps, and protocols are 
consuming the most bandwidth. Provides multi-vendor support for NetFlow, 
J-Flow, sFlow and other flows. Make informed decisions using capacity 
planning reports. http://sdm.link/zohodev2dev

Re: [PATCH v2 2/3] TPM2.0: TPM Device Tree Documentation

[Nayna]

Hi Jason,

Thanks for review, My responses inline.

On 08/10/2016 03:42 AM, Jason Gunthorpe wrote:
> On Tue, Aug 09, 2016 at 03:34:54PM -0400, Nayna Jain wrote:
>> Added device tree binding documentation for I2C base TPM.
>
> You need to review
>
>   Documentation/devicetree/bindings/submitting-patches.txt
>
> And be sure to follow all the requirements, including CC'ing the
> right maintainers
Sure, will do. Can you tell me who all should I copy ?

>
> Also explain that this is a documenting a historical practice that IBM
> has used in systems  X Y Z.

Ok.

>
>> Changelog v2:
>>          * Added documentation in v2.
>
> Do not include these lines in commit messages, put them after the
> diff stat.

Actually I have not added Changelog during commit, I have put them in 
patch before posting. However, will add them after diff next time.
>
>> +This node describes a TPM device connected to Processor on i2c bus.
>> +
>> +Required properties:
>> +
>> +- compatible : 'manufacturer,model'
>> +- label : "tpm" indicates master TPM; "tpm-backup" indicates backup TPM.
>> +- linux,sml-base : base address of the Event Log. It is a physical address.
>> +		   sml stands for shared memory log.
>> +- linux,sml-size : size of the memory allocated for the Event Log.
>> +
>> +Optional properties:
>> +
>> +- status: indicates whether the device is enabled or disabled. "okay" for
>> +          enabled and "disabled" for disabled.
>> +
>> +Example
>> +-------
>> +
>> +tpm@57 {
>> +	reg = <0x57>;
>> +	label = "tpm";
>> +	compatible = "nuvoton,npct650", "nuvoton,npct601";
>> +	linux,sml-base = <0x7f 0xfd450000>;
>> +	linux,sml-size = <0x10000>;
>> +	status = "okay";
>
>> +	phandle = <0x10000017>;
>> +	linux,phandle = <0x10000017>;
>
>   What are these?

Sorry I didn't get your question here. I have written in the format as 
suggested for device-tree documentation and looking at other 
device-nodes documentation. And so this is an example entry as I saw in 
other docs.

>
> Jason
>


------------------------------------------------------------------------------
What NetFlow Analyzer can do for you? Monitors network bandwidth and traffic
patterns at an interface-level. Reveals which users, apps, and protocols are 
consuming the most bandwidth. Provides multi-vendor support for NetFlow, 
J-Flow, sFlow and other flows. Make informed decisions using capacity 
planning reports. http://sdm.link/zohodev2dev

Re: [PATCH v2 2/3] TPM2.0: TPM Device Tree Documentation

[Jason Gunthorpe]

On Fri, Aug 12, 2016 at 06:06:05PM +0530, Nayna wrote:
> >  Documentation/devicetree/bindings/submitting-patches.txt
> >
> >And be sure to follow all the requirements, including CC'ing the
> >right maintainers

> Sure, will do. Can you tell me who all should I copy ?

That information is in the file

> >>Changelog v2:
> >>         * Added documentation in v2.
> >
> >Do not include these lines in commit messages, put them after the
> >diff stat.
> 
> Actually I have not added Changelog during commit, I have put them in patch
> before posting. However, will add them after diff next time.

No diff, after the diffstat, before the deiff.

> >>+	phandle = <0x10000017>;
> >>+	linux,phandle = <0x10000017>;
> >
> >  What are these?
> 
> Sorry I didn't get your question here. I have written in the format as
> suggested for device-tree documentation and looking at other device-nodes
> documentation. And so this is an example entry as I saw in other docs.

I mean the two phandle things

Jason

------------------------------------------------------------------------------
What NetFlow Analyzer can do for you? Monitors network bandwidth and traffic
patterns at an interface-level. Reveals which users, apps, and protocols are 
consuming the most bandwidth. Provides multi-vendor support for NetFlow, 
J-Flow, sFlow and other flows. Make informed decisions using capacity 
planning reports. http://sdm.link/zohodev2dev

Re: [PATCH v2 1/3] TPM2.0: Refactored eventlog init functions.

[Jason Gunthorpe]

On Wed, Aug 10, 2016 at 04:42:20PM +0530, Nayna wrote:
> Thanks for reviewing.
> 
> Sure, I will post next version with split as two patches and other fixes as
> suggested. Below is the breakdown of two patches, let me know if this
> doesn't sound ok.
> 
> 1. First patch to clean up the code related to tpm_eventlog_init.c - generic
> open() and bios_dir as dentry array.

Split the patches further. One idea per patch.

> 2. Second patch to have changes related to using of_node property and struct
> tpm_chip in tpm_of.c. Includes adding CONFIG_OF.

Yes

> And one feedback which I didn't understand and so need your help with that
> is
> 
> >> -extern struct dentry **tpm_bios_log_setup(const char *);
> >> -extern void tpm_bios_log_teardown(struct dentry **);
> >> +extern void tpm_bios_log_setup(struct tpm_chip *chip);
> >> +extern void tpm_bios_log_teardown(struct tpm_chip *chip);
> >
> > We are trying to get rid of these extra externs..
> 
> This is currently called by tpm_chip_register to setup the bios log.
> So, what did it meant by getting rid of these ?

The word extern in this context is unnecessary, just drop it when you
edit the line.

Jason

------------------------------------------------------------------------------
What NetFlow Analyzer can do for you? Monitors network bandwidth and traffic
patterns at an interface-level. Reveals which users, apps, and protocols are 
consuming the most bandwidth. Provides multi-vendor support for NetFlow, 
J-Flow, sFlow and other flows. Make informed decisions using capacity 
planning reports. http://sdm.link/zohodev2dev

Re: [PATCH v2 0/3] TPM2.0: Added eventlog support for TPM2.0

[Jason Gunthorpe]

On Thu, Aug 11, 2016 at 04:18:34PM +0530, Nayna wrote:
>> #1. And the next somewhat pre-existing issue is that we call
>> tpm_bios_log_setup even if we don't have access to a bios log.
>> 
>> Does the bios log ever change or is it static at boot? Can we move
>> read_log into the chip and do it once so we can tell if it worked
>> before creating security fs stuff??
> 
> <nayna>
> 
> So, by statement "if we don't have access to a bios log?".. Do you mean bios
> log is empty ? or has no properties to access the log.

More along the lines we call down to tpm_of and it just exits if the
properties do not exist. For instance my DT using TPM systems do not
have a bios log.

> Regarding bios log, so it is supposed to be written by each component in
> boot stack during boot.

Only if the boot firmware supports this functionality and indicates it
via DT attributes or ACPI. That is what we need to check earlier.

> As per moving read_log into the chip, do you mean something like as below ?
>
> struct tpm_chip {
> ....
>     struct tpm_bios_log *bios_log;
> ....
> }
> 
> read_log(struct tpm_chip *chip)
> {
> 
>  allocates to chip->bios_log->bios_event_log
>  allocates to chip->bios_log->bios_event_log_end;
> 
>  return 0; // for success;
> 
> }
> 
> tpm_chip_register()
> {
> ...
> if (read_log(chip))
>    tpm_bios_log_setup(chip);
> ...
> }

Yes, exactly. The challenge will be to get the chip into the security
file context, give that a try and we can probably help you. The
locking and lifetime will probably be very subtle.

> And as per readlog is concerned, it just reads the start and max size of the
> log for both ACPI/OF. So, it is possible that readlog returns successfully
> giving start and max allocated size. but actual log size is zero. So, when
> we cat bios_measurements_file, it doesn't show anything.  And I think that
> is fine.

That would be a firmware that says it support a log but generated an
empty one, so that seems reasonble to report to user space.

> 
> #2.
> >  #if defined(CONFIG_TCG_IBMVTPM) || defined(CONFIG_TCG_IBMVTPM_MODULE) ||
> \
> > -	defined(CONFIG_ACPI)
> > -extern struct dentry **tpm_bios_log_setup(const char *);
> > -extern void tpm_bios_log_teardown(struct dentry **);
> > +	defined(CONFIG_ACPI) || defined(CONFIG_PPC64)
> > +extern struct dentry **tpm_bios_log_setup(struct tpm_chip *chip);
> > +extern void tpm_bios_log_teardown(struct tpm_chip *chip);
> 
> I'm really deeply unhappy about these ifdefs and the general scheme
> that was used to force this special difference into the tpm core.
> 
> Please find another way to do it.
> 
> IMHO, 'read_log' should simply try ACPI and OF in sequence to find the
> log.
> 
> <nayna> Can you help me to understand the reason for which it was done this
> way ?

When this was written the TPM subsystem was very old and lacked
infrastructure to do this better, we have largely solved those
problems, event log is the last large area that needs some attention..

> Secondly, did you mean something like (just a sample psudocode)
> read_log()
> {
> read_of_node;
> if (read_of_node_fails)
> 	read_acpi_way
>         if (read_acpi_succeeds)
>            do acpi_way of reading for start of log and max log size.
> 	else
>            return err;
> do of_node way of reading for start of log and max log size
> }

I would do more like:

read_log_of(
{
  if (! chip->pdev->of_node .. has attribute)
     return 0
  .. return 1;
}

read_log()
{
  if ((res = read_log_of(chip)) != 0)
     return res;
  if ((res = read_log_acpi(chip)) != 0)
     return res;
  return 0;
}
   
> So, is it safe to assume that if there is no of_node property , that means
> it is ACPI ?

No, ACPI also reads a property from the ACPI table, the existance of
that should be tested directly.

> #3. Where you able to test this on IBM's 'vtpm' stuff as well?,
> 
> <nayna>This was for patch where I am directly using dev.of_node property.
> Sorry I missed to reply in previous response. And answer is "yes", I did
> test it. </nayna>

That is excellent

Jason

------------------------------------------------------------------------------
What NetFlow Analyzer can do for you? Monitors network bandwidth and traffic
patterns at an interface-level. Reveals which users, apps, and protocols are 
consuming the most bandwidth. Provides multi-vendor support for NetFlow, 
J-Flow, sFlow and other flows. Make informed decisions using capacity 
planning reports. http://sdm.link/zohodev2dev

Re: [PATCH v2 0/3] TPM2.0: Added eventlog support for TPM2.0

[Jarkko Sakkinen]

On Fri, Aug 12, 2016 at 06:02:00PM +0530, Nayna wrote:
> >>Jarkko, Please let me know if it doesn't answer your question. </nayna>
> >
> >1. EFI does not pass the log by any means AFAIK before a boot loader
> >    calls ExitBootServices().
> 
> So, is current TCPA support only for TPM1.2 ?

TCPA ACPI table is only available for TPM 1.2.

TPM2 ACPI table does not provide a memory ref for the event log.

> #2, TCG Spec http://www.trustedcomputinggroup.org/wp-content/uploads/EFI-Protocol-Specification-rev13-160330final.pdf
> talks about
> EFI_TCG2_PROTOCOL.GetEventLog (Section 6.5), what is that supposed to do ?

Direct quote from your reference:

"Boot Service Drivers are terminated when ExitBootServices() is called
and all memory resources consumed by the Boot Services Drivers are
released for use in the operating system environment."

> >2. I do not have any system with TPM2 that uses DT. And as I stated
> >    before you didn't have any reference where you derived the DT
> >    node fields.
> 
> As per Device Tree, so this is the new node introduced in the device tree to
> support TPM. And the fields are defined maintaining the requirements from
> ePAPR specification.

What is ePAPR specification? Can you provide a reference?

/Jarkko

------------------------------------------------------------------------------

Re: [PATCH v2 0/3] TPM2.0: Added eventlog support for TPM2.0

[Nayna]

Hi Jason,

Thanks for your inputs.. Further responses inline.

On 08/13/2016 08:29 AM, Jason Gunthorpe wrote:
> On Thu, Aug 11, 2016 at 04:18:34PM +0530, Nayna wrote:
>>> #1. And the next somewhat pre-existing issue is that we call
>>> tpm_bios_log_setup even if we don't have access to a bios log.
>>>
>>> Does the bios log ever change or is it static at boot? Can we move
>>> read_log into the chip and do it once so we can tell if it worked
>>> before creating security fs stuff??
>>
>> <nayna>
>>
>> So, by statement "if we don't have access to a bios log?".. Do you mean bios
>> log is empty ? or has no properties to access the log.
>
> More along the lines we call down to tpm_of and it just exits if the
> properties do not exist. For instance my DT using TPM systems do not
> have a bios log.

Ok.

>
>> Regarding bios log, so it is supposed to be written by each component in
>> boot stack during boot.
>
> Only if the boot firmware supports this functionality and indicates it
> via DT attributes or ACPI. That is what we need to check earlier.
>
>> As per moving read_log into the chip, do you mean something like as below ?
>>
>> struct tpm_chip {
>> ....
>>      struct tpm_bios_log *bios_log;
>> ....
>> }
>>
>> read_log(struct tpm_chip *chip)
>> {
>>
>>   allocates to chip->bios_log->bios_event_log
>>   allocates to chip->bios_log->bios_event_log_end;
>>
>>   return 0; // for success;
>>
>> }
>>
>> tpm_chip_register()
>> {
>> ...
>> if (read_log(chip))
>>     tpm_bios_log_setup(chip);
>> ...
>> }
>
> Yes, exactly. The challenge will be to get the chip into the security
> file context, give that a try and we can probably help you. The
> locking and lifetime will probably be very subtle.

Sure, I have done this change now in my next patch, should be able to 
post soon.
>
>> And as per readlog is concerned, it just reads the start and max size of the
>> log for both ACPI/OF. So, it is possible that readlog returns successfully
>> giving start and max allocated size. but actual log size is zero. So, when
>> we cat bios_measurements_file, it doesn't show anything.  And I think that
>> is fine.
>
> That would be a firmware that says it support a log but generated an
> empty one, so that seems reasonble to report to user space.
>
>>
>> #2.
>>>   #if defined(CONFIG_TCG_IBMVTPM) || defined(CONFIG_TCG_IBMVTPM_MODULE) ||
>> \
>>> -	defined(CONFIG_ACPI)
>>> -extern struct dentry **tpm_bios_log_setup(const char *);
>>> -extern void tpm_bios_log_teardown(struct dentry **);
>>> +	defined(CONFIG_ACPI) || defined(CONFIG_PPC64)
>>> +extern struct dentry **tpm_bios_log_setup(struct tpm_chip *chip);
>>> +extern void tpm_bios_log_teardown(struct tpm_chip *chip);
>>
>> I'm really deeply unhappy about these ifdefs and the general scheme
>> that was used to force this special difference into the tpm core.
>>
>> Please find another way to do it.
>>
>> IMHO, 'read_log' should simply try ACPI and OF in sequence to find the
>> log.
>>
>> <nayna> Can you help me to understand the reason for which it was done this
>> way ?
>
> When this was written the TPM subsystem was very old and lacked
> infrastructure to do this better, we have largely solved those
> problems, event log is the last large area that needs some attention..
>

Ok.

>> Secondly, did you mean something like (just a sample psudocode)
>> read_log()
>> {
>> read_of_node;
>> if (read_of_node_fails)
>> 	read_acpi_way
>>          if (read_acpi_succeeds)
>>             do acpi_way of reading for start of log and max log size.
>> 	else
>>             return err;
>> do of_node way of reading for start of log and max log size
>> }
>
> I would do more like:
>
> read_log_of(
> {
>    if (! chip->pdev->of_node .. has attribute)
>       return 0
>    .. return 1;
> }
>
> read_log()
> {
>    if ((res = read_log_of(chip)) != 0)
>       return res;
>    if ((res = read_log_acpi(chip)) != 0)
>       return res;
>    return 0;
> }

I tried the suggested approach and since ACPI specific functions won't 
be available for arch using CONFIG_OF, so the compilation fails and vice 
versa for CONFIG_ACPI..

Jason, for understanding.. can you explain the issue with existing 
design for read_log, where arch specific read_log is compiled based on 
CONFIG option.. And then, we can just change in Makefile.. where 
currently it is like

ifdef CONFIG_ACPI
        tpm-y += tpm_acpi.o
else
ifdef CONFIG_OF
        tpm-y += tpm_of.o
endif
endif


this can be changed to
tpm-$(CONFIG_ACPI) += tpm_acpi.o
tpm-$(CONFIG_OF) += tpm_of.o


Please let me know if I missed something.

Thanks & Regards,
    - Nayna

>
>> So, is it safe to assume that if there is no of_node property , that means
>> it is ACPI ?
>
> No, ACPI also reads a property from the ACPI table, the existance of
> that should be tested directly.
>
>> #3. Where you able to test this on IBM's 'vtpm' stuff as well?,
>>
>> <nayna>This was for patch where I am directly using dev.of_node property.
>> Sorry I missed to reply in previous response. And answer is "yes", I did
>> test it. </nayna>
>
> That is excellent
>
> Jason
>


------------------------------------------------------------------------------

Re: [PATCH v2 1/3] TPM2.0: Refactored eventlog init functions.

[Nayna]

Sure Jason.. Taking care of split and other fixes in my V3 version of patch.

Thanks & Regards,
   - Nayna

On 08/13/2016 08:15 AM, Jason Gunthorpe wrote:
> On Wed, Aug 10, 2016 at 04:42:20PM +0530, Nayna wrote:
>> Thanks for reviewing.
>>
>> Sure, I will post next version with split as two patches and other fixes as
>> suggested. Below is the breakdown of two patches, let me know if this
>> doesn't sound ok.
>>
>> 1. First patch to clean up the code related to tpm_eventlog_init.c - generic
>> open() and bios_dir as dentry array.
>
> Split the patches further. One idea per patch.
>
>> 2. Second patch to have changes related to using of_node property and struct
>> tpm_chip in tpm_of.c. Includes adding CONFIG_OF.
>
> Yes
>
>> And one feedback which I didn't understand and so need your help with that
>> is
>>
>>>> -extern struct dentry **tpm_bios_log_setup(const char *);
>>>> -extern void tpm_bios_log_teardown(struct dentry **);
>>>> +extern void tpm_bios_log_setup(struct tpm_chip *chip);
>>>> +extern void tpm_bios_log_teardown(struct tpm_chip *chip);
>>>
>>> We are trying to get rid of these extra externs..
>>
>> This is currently called by tpm_chip_register to setup the bios log.
>> So, what did it meant by getting rid of these ?
>
> The word extern in this context is unnecessary, just drop it when you
> edit the line.
>
> Jason
>


------------------------------------------------------------------------------

Re: [PATCH v2 2/3] TPM2.0: TPM Device Tree Documentation

[Nayna]

Hi Jason,

Thanks for the review. Please find my responses inline.

On 08/13/2016 08:12 AM, Jason Gunthorpe wrote:
> On Fri, Aug 12, 2016 at 06:06:05PM +0530, Nayna wrote:
>>>   Documentation/devicetree/bindings/submitting-patches.txt
>>>
>>> And be sure to follow all the requirements, including CC'ing the
>>> right maintainers
>
>> Sure, will do. Can you tell me who all should I copy ?
>
> That information is in the file

Sure..

>
>>>> Changelog v2:
>>>>          * Added documentation in v2.
>>>
>>> Do not include these lines in commit messages, put them after the
>>> diff stat.
Ok

>>
>> Actually I have not added Changelog during commit, I have put them in patch
>> before posting. However, will add them after diff next time.
>
> No diff, after the diffstat, before the deiff.

Ok.
>
>>>> +	phandle = <0x10000017>;
>>>> +	linux,phandle = <0x10000017>;
>>>
>>>   What are these?
>>
>> Sorry I didn't get your question here. I have written in the format as
>> suggested for device-tree documentation and looking at other device-nodes
>> documentation. And so this is an example entry as I saw in other docs.
>
> I mean the two phandle things

Here is the description:

phandle property:

The phandle property specifies a numerical identifier for a node that is 
unique within the device tree. The phandle property value is used by 
other nodes that need to refer to the node associated with the property.

linux,phandle property:

Older versions of device trees may be encountered that contain a 
deprecated form of this property called linux,phandle. For 
compatibility, a client program might want to support linux,phandle if a 
phandle property is not present. The meaning and use of the two 
properties is identical.

>
> Jason
>


------------------------------------------------------------------------------

Re: [PATCH v2 0/3] TPM2.0: Added eventlog support for TPM2.0

[Nayna]

Hi Jarkko,



On 08/16/2016 02:56 AM, Jarkko Sakkinen wrote:
> On Fri, Aug 12, 2016 at 06:02:00PM +0530, Nayna wrote:
>>>> Jarkko, Please let me know if it doesn't answer your question. </nayna>
>>>
>>> 1. EFI does not pass the log by any means AFAIK before a boot loader
>>>     calls ExitBootServices().
>>
>> So, is current TCPA support only for TPM1.2 ?
>
> TCPA ACPI table is only available for TPM 1.2.
>
> TPM2 ACPI table does not provide a memory ref for the event log.
>
>> #2, TCG Spec http://www.trustedcomputinggroup.org/wp-content/uploads/EFI-Protocol-Specification-rev13-160330final.pdf
>> talks about
>> EFI_TCG2_PROTOCOL.GetEventLog (Section 6.5), what is that supposed to do ?
>
> Direct quote from your reference:
>
> "Boot Service Drivers are terminated when ExitBootServices() is called
> and all memory resources consumed by the Boot Services Drivers are
> released for use in the operating system environment."

Thanks Jarkko, I understand now what you meant.
>
>>> 2. I do not have any system with TPM2 that uses DT. And as I stated
>>>     before you didn't have any reference where you derived the DT
>>>     node fields.
>>
>> As per Device Tree, so this is the new node introduced in the device tree to
>> support TPM. And the fields are defined maintaining the requirements from
>> ePAPR specification.
>
> What is ePAPR specification? Can you provide a reference?

PowerPC systems are based on device tree and derive that from ePAPR 
specification, link below for ePAPR specification..

https://www.power.org/documentation/power-org-standard-for-embedded-power-architecture-platform-requirements-epapr-v1-1-2/

I have also shared TPM2.0 device tree kernel documentation as Patch v2 2/3.

>
> /Jarkko
>


------------------------------------------------------------------------------

Re: [PATCH v2 0/3] TPM2.0: Added eventlog support for TPM2.0

[Jarkko Sakkinen]

On Wed, Aug 17, 2016 at 12:46:40AM +0530, Nayna wrote:
> Hi Jarkko,
> 
> 
> 
> On 08/16/2016 02:56 AM, Jarkko Sakkinen wrote:
> >On Fri, Aug 12, 2016 at 06:02:00PM +0530, Nayna wrote:
> >>>>Jarkko, Please let me know if it doesn't answer your question. </nayna>
> >>>
> >>>1. EFI does not pass the log by any means AFAIK before a boot loader
> >>>    calls ExitBootServices().
> >>
> >>So, is current TCPA support only for TPM1.2 ?
> >
> >TCPA ACPI table is only available for TPM 1.2.
> >
> >TPM2 ACPI table does not provide a memory ref for the event log.
> >
> >>#2, TCG Spec http://www.trustedcomputinggroup.org/wp-content/uploads/EFI-Protocol-Specification-rev13-160330final.pdf
> >>talks about
> >>EFI_TCG2_PROTOCOL.GetEventLog (Section 6.5), what is that supposed to do ?
> >
> >Direct quote from your reference:
> >
> >"Boot Service Drivers are terminated when ExitBootServices() is called
> >and all memory resources consumed by the Boot Services Drivers are
> >released for use in the operating system environment."
> 
> Thanks Jarkko, I understand now what you meant.
> >
> >>>2. I do not have any system with TPM2 that uses DT. And as I stated
> >>>    before you didn't have any reference where you derived the DT
> >>>    node fields.
> >>
> >>As per Device Tree, so this is the new node introduced in the device tree to
> >>support TPM. And the fields are defined maintaining the requirements from
> >>ePAPR specification.
> >
> >What is ePAPR specification? Can you provide a reference?
> 
> PowerPC systems are based on device tree and derive that from ePAPR
> specification, link below for ePAPR specification..
> 
> https://www.power.org/documentation/power-org-standard-for-embedded-power-architecture-platform-requirements-epapr-v1-1-2/

Thanks. I'll check that through when I review the next version.

PS. Use subsystem prefix in your next patches (tpm: not TPM2.0:)

/Jarkko

------------------------------------------------------------------------------

Re: [PATCH v2 0/3] TPM2.0: Added eventlog support for TPM2.0

[Jarkko Sakkinen]

On Tue, Aug 16, 2016 at 10:48:53PM +0300, Jarkko Sakkinen wrote:
> On Wed, Aug 17, 2016 at 12:46:40AM +0530, Nayna wrote:
> > Hi Jarkko,
> > 
> > 
> > 
> > On 08/16/2016 02:56 AM, Jarkko Sakkinen wrote:
> > >On Fri, Aug 12, 2016 at 06:02:00PM +0530, Nayna wrote:
> > >>>>Jarkko, Please let me know if it doesn't answer your question. </nayna>
> > >>>
> > >>>1. EFI does not pass the log by any means AFAIK before a boot loader
> > >>>    calls ExitBootServices().
> > >>
> > >>So, is current TCPA support only for TPM1.2 ?
> > >
> > >TCPA ACPI table is only available for TPM 1.2.
> > >
> > >TPM2 ACPI table does not provide a memory ref for the event log.
> > >
> > >>#2, TCG Spec http://www.trustedcomputinggroup.org/wp-content/uploads/EFI-Protocol-Specification-rev13-160330final.pdf
> > >>talks about
> > >>EFI_TCG2_PROTOCOL.GetEventLog (Section 6.5), what is that supposed to do ?
> > >
> > >Direct quote from your reference:
> > >
> > >"Boot Service Drivers are terminated when ExitBootServices() is called
> > >and all memory resources consumed by the Boot Services Drivers are
> > >released for use in the operating system environment."
> > 
> > Thanks Jarkko, I understand now what you meant.
> > >
> > >>>2. I do not have any system with TPM2 that uses DT. And as I stated
> > >>>    before you didn't have any reference where you derived the DT
> > >>>    node fields.
> > >>
> > >>As per Device Tree, so this is the new node introduced in the device tree to
> > >>support TPM. And the fields are defined maintaining the requirements from
> > >>ePAPR specification.
> > >
> > >What is ePAPR specification? Can you provide a reference?
> > 
> > PowerPC systems are based on device tree and derive that from ePAPR
> > specification, link below for ePAPR specification..
> > 
> > https://www.power.org/documentation/power-org-standard-for-embedded-power-architecture-platform-requirements-epapr-v1-1-2/
> 
> Thanks. I'll check that through when I review the next version.

This specification did not define the TPM binding for DT. I searched
with "tpm" keyword from the specification. Why did you give that link?

You earlier said that fields in TPM binding are derived from that
specification. For me this looks like total nonsense.

/Jarkko


------------------------------------------------------------------------------

Re: [PATCH v2 0/3] TPM2.0: Added eventlog support for TPM2.0

[Nayna]

Hi Jarkko,

My response inline.

On 08/17/2016 09:45 AM, Jarkko Sakkinen wrote:
> On Tue, Aug 16, 2016 at 10:48:53PM +0300, Jarkko Sakkinen wrote:
>> On Wed, Aug 17, 2016 at 12:46:40AM +0530, Nayna wrote:
>>> Hi Jarkko,
>>>
>>>
>>>
>>> On 08/16/2016 02:56 AM, Jarkko Sakkinen wrote:
>>>> On Fri, Aug 12, 2016 at 06:02:00PM +0530, Nayna wrote:
>>>>>>> Jarkko, Please let me know if it doesn't answer your question. </nayna>
>>>>>>
>>>>>> 1. EFI does not pass the log by any means AFAIK before a boot loader
>>>>>>     calls ExitBootServices().
>>>>>
>>>>> So, is current TCPA support only for TPM1.2 ?
>>>>
>>>> TCPA ACPI table is only available for TPM 1.2.
>>>>
>>>> TPM2 ACPI table does not provide a memory ref for the event log.
>>>>
>>>>> #2, TCG Spec http://www.trustedcomputinggroup.org/wp-content/uploads/EFI-Protocol-Specification-rev13-160330final.pdf
>>>>> talks about
>>>>> EFI_TCG2_PROTOCOL.GetEventLog (Section 6.5), what is that supposed to do ?
>>>>
>>>> Direct quote from your reference:
>>>>
>>>> "Boot Service Drivers are terminated when ExitBootServices() is called
>>>> and all memory resources consumed by the Boot Services Drivers are
>>>> released for use in the operating system environment."
>>>
>>> Thanks Jarkko, I understand now what you meant.
>>>>
>>>>>> 2. I do not have any system with TPM2 that uses DT. And as I stated
>>>>>>     before you didn't have any reference where you derived the DT
>>>>>>     node fields.
>>>>>
>>>>> As per Device Tree, so this is the new node introduced in the device tree to
>>>>> support TPM. And the fields are defined maintaining the requirements from
>>>>> ePAPR specification.
>>>>
>>>> What is ePAPR specification? Can you provide a reference?
>>>
>>> PowerPC systems are based on device tree and derive that from ePAPR
>>> specification, link below for ePAPR specification..
>>>
>>> https://www.power.org/documentation/power-org-standard-for-embedded-power-architecture-platform-requirements-epapr-v1-1-2/
>>
>> Thanks. I'll check that through when I review the next version.
>
> This specification did not define the TPM binding for DT. I searched
> with "tpm" keyword from the specification. Why did you give that link?
>
> You earlier said that fields in TPM binding are derived from that
> specification. For me this looks like total nonsense.

I am sorry Jarkko, if I didn't clearly communicated.
I was trying to say that tpm device tree binding is new binding added 
within i2c node. And for that reason, I am now submitting a patch for 
its binding documentation.

And as ePAPR document explains about device trees and its properties, I 
just meant that fields defined for this new tpm node are also defined as 
per ePAPR spec.

Please let me know if I am still not answering your question, and sorry 
but if you can explain me again what exactly are you looking for.

Thanks & Regards,
   - Nayna

>
> /Jarkko
>


------------------------------------------------------------------------------

Re: [PATCH v2 0/3] TPM2.0: Added eventlog support for TPM2.0

[Jarkko Sakkinen]

On Wed, Aug 17, 2016 at 11:28:51AM +0530, Nayna wrote:
> Hi Jarkko,
> 
> My response inline.
> 
> On 08/17/2016 09:45 AM, Jarkko Sakkinen wrote:
> >On Tue, Aug 16, 2016 at 10:48:53PM +0300, Jarkko Sakkinen wrote:
> >>On Wed, Aug 17, 2016 at 12:46:40AM +0530, Nayna wrote:
> >>>Hi Jarkko,
> >>>
> >>>
> >>>
> >>>On 08/16/2016 02:56 AM, Jarkko Sakkinen wrote:
> >>>>On Fri, Aug 12, 2016 at 06:02:00PM +0530, Nayna wrote:
> >>>>>>>Jarkko, Please let me know if it doesn't answer your question. </nayna>
> >>>>>>
> >>>>>>1. EFI does not pass the log by any means AFAIK before a boot loader
> >>>>>>    calls ExitBootServices().
> >>>>>
> >>>>>So, is current TCPA support only for TPM1.2 ?
> >>>>
> >>>>TCPA ACPI table is only available for TPM 1.2.
> >>>>
> >>>>TPM2 ACPI table does not provide a memory ref for the event log.
> >>>>
> >>>>>#2, TCG Spec http://www.trustedcomputinggroup.org/wp-content/uploads/EFI-Protocol-Specification-rev13-160330final.pdf
> >>>>>talks about
> >>>>>EFI_TCG2_PROTOCOL.GetEventLog (Section 6.5), what is that supposed to do ?
> >>>>
> >>>>Direct quote from your reference:
> >>>>
> >>>>"Boot Service Drivers are terminated when ExitBootServices() is called
> >>>>and all memory resources consumed by the Boot Services Drivers are
> >>>>released for use in the operating system environment."
> >>>
> >>>Thanks Jarkko, I understand now what you meant.
> >>>>
> >>>>>>2. I do not have any system with TPM2 that uses DT. And as I stated
> >>>>>>    before you didn't have any reference where you derived the DT
> >>>>>>    node fields.
> >>>>>
> >>>>>As per Device Tree, so this is the new node introduced in the device tree to
> >>>>>support TPM. And the fields are defined maintaining the requirements from
> >>>>>ePAPR specification.
> >>>>
> >>>>What is ePAPR specification? Can you provide a reference?
> >>>
> >>>PowerPC systems are based on device tree and derive that from ePAPR
> >>>specification, link below for ePAPR specification..
> >>>
> >>>https://www.power.org/documentation/power-org-standard-for-embedded-power-architecture-platform-requirements-epapr-v1-1-2/
> >>
> >>Thanks. I'll check that through when I review the next version.
> >
> >This specification did not define the TPM binding for DT. I searched
> >with "tpm" keyword from the specification. Why did you give that link?
> >
> >You earlier said that fields in TPM binding are derived from that
> >specification. For me this looks like total nonsense.
> 
> I am sorry Jarkko, if I didn't clearly communicated.
> I was trying to say that tpm device tree binding is new binding added within
> i2c node. And for that reason, I am now submitting a patch for its binding
> documentation.
> 
> And as ePAPR document explains about device trees and its properties, I just
> meant that fields defined for this new tpm node are also defined as per
> ePAPR spec.
> 
> Please let me know if I am still not answering your question, and sorry but
> if you can explain me again what exactly are you looking for.

1. Does TCG have a standard for this I2C binding in a DT like they have
   for ACPI?
2. Can I expect that the binding has the same fields with the same names
   in some other platform that POWER? For example, what if there was an
   ARM based platform. Would the same binding work there?

I'm looking for document or something where you got your information.
Right now your code is based on nothing from my point of view.

/Jarkko

------------------------------------------------------------------------------

Re: [PATCH v2 0/3] TPM2.0: Added eventlog support for TPM2.0

[Jason Gunthorpe]

On Tue, Aug 16, 2016 at 02:21:53PM +0530, Nayna wrote:

> I tried the suggested approach and since ACPI specific functions won't be
> available for arch using CONFIG_OF, so the compilation fails and vice versa
> for CONFIG_ACPI..

Right, you need to stub out the read_X function with an empty inline
when not available.

> Jason, for understanding.. can you explain the issue with existing design
> for read_log, where arch specific read_log is compiled based on CONFIG
> option.. And then, we can just change in Makefile.. where currently it is
> like
> 
> ifdef CONFIG_ACPI
>        tpm-y += tpm_acpi.o
> else
> ifdef CONFIG_OF
>        tpm-y += tpm_of.o
> endif
> endif

The 'else' is the main issue

 
> this can be changed to
> tpm-$(CONFIG_ACPI) += tpm_acpi.o
> tpm-$(CONFIG_OF) += tpm_of.o

This is fine by me, *however* only if the .o files do not define
overlapping symbols as they do today (eg tpm_acpi contains read_log_acpi)

Use some small #ifdef logic in the header file to create the inline
stub.

Jason

------------------------------------------------------------------------------

Re: [PATCH v2 0/3] TPM2.0: Added eventlog support for TPM2.0

[Jason Gunthorpe]

On Wed, Aug 17, 2016 at 11:09:14AM +0300, Jarkko Sakkinen wrote:

> 1. Does TCG have a standard for this I2C binding in a DT like they have
>    for ACPI?
> 2. Can I expect that the binding has the same fields with the same names
>    in some other platform that POWER? For example, what if there was an
>    ARM based platform. Would the same binding work there?
> 
> I'm looking for document or something where you got your information.
> Right now your code is based on nothing from my point of view.

There is no real standards body for device tree right now.

Submitting a Docuemtation patch to the device tree maintainers is the
best we have today, and we as the TPM community should review it to
ensure it makes sense from our perspective.

Other DT platforms like ARM will use the bindings defined in
Documentation/DeviceTree.

Jason

------------------------------------------------------------------------------

Re: [PATCH v2 2/3] TPM2.0: TPM Device Tree Documentation

[Jason Gunthorpe]

On Tue, Aug 16, 2016 at 11:30:40PM +0530, Nayna wrote:
> Here is the description:
> 
> phandle property:

IIRC these are automatically generated by the dt compiler and are not
part of the binding. Please confirm if you need them in the binding
document.

Jason

------------------------------------------------------------------------------

Re: [PATCH v2 0/3] TPM2.0: Added eventlog support for TPM2.0

[Nayna]

On 08/19/2016 01:29 AM, Jason Gunthorpe wrote:
> On Wed, Aug 17, 2016 at 11:09:14AM +0300, Jarkko Sakkinen wrote:
>
>> 1. Does TCG have a standard for this I2C binding in a DT like they have
>>     for ACPI?
>> 2. Can I expect that the binding has the same fields with the same names
>>     in some other platform that POWER? For example, what if there was an
>>     ARM based platform. Would the same binding work there?
>>
>> I'm looking for document or something where you got your information.
>> Right now your code is based on nothing from my point of view.
>
> There is no real standards body for device tree right now.
>
> Submitting a Docuemtation patch to the device tree maintainers is the
> best we have today, and we as the TPM community should review it to
> ensure it makes sense from our perspective.
>
> Other DT platforms like ARM will use the bindings defined in
> Documentation/DeviceTree.
>
> Jason

Thanks, Jarkko, for stating exactly what is required to review the 
patch.  I will be providing the TPM Device Tree Documentation in my 
patchset as per Jason's suggestion.  We'll also be updating the PAPR to 
document the new node.

Thanks & Regards,
    - Nayna
>


------------------------------------------------------------------------------

Re: [PATCH v2 0/3] TPM2.0: Added eventlog support for TPM2.0

[Jarkko Sakkinen]

On Thu, Aug 18, 2016 at 01:59:00PM -0600, Jason Gunthorpe wrote:
> On Wed, Aug 17, 2016 at 11:09:14AM +0300, Jarkko Sakkinen wrote:
> 
> > 1. Does TCG have a standard for this I2C binding in a DT like they have
> >    for ACPI?
> > 2. Can I expect that the binding has the same fields with the same names
> >    in some other platform that POWER? For example, what if there was an
> >    ARM based platform. Would the same binding work there?
> > 
> > I'm looking for document or something where you got your information.
> > Right now your code is based on nothing from my point of view.
> 
> There is no real standards body for device tree right now.
> 
> Submitting a Docuemtation patch to the device tree maintainers is the
> best we have today, and we as the TPM community should review it to
> ensure it makes sense from our perspective.
> 
> Other DT platforms like ARM will use the bindings defined in
> Documentation/DeviceTree.

So how do I know that this the right way to specify the attibutes for a
TPM device and all vendors would like to have the attributes like this?

> Jason

/Jarkko

------------------------------------------------------------------------------

Re: [PATCH v2 0/3] TPM2.0: Added eventlog support for TPM2.0

[Jason Gunthorpe]

On Thu, Aug 25, 2016 at 05:13:04PM -0400, Jarkko Sakkinen wrote:
> > Other DT platforms like ARM will use the bindings defined in
> > Documentation/DeviceTree.
> 
> So how do I know that this the right way to specify the attibutes for a
> TPM device and all vendors would like to have the attributes like this?

If you accept the patch it becomes the right way for DT.

Jason

------------------------------------------------------------------------------

Re: [PATCH v2 0/3] TPM2.0: Added eventlog support for TPM2.0

[Jarkko Sakkinen]

On Thu, Aug 25, 2016 at 03:20:38PM -0600, Jason Gunthorpe wrote:
> On Thu, Aug 25, 2016 at 05:13:04PM -0400, Jarkko Sakkinen wrote:
> > > Other DT platforms like ARM will use the bindings defined in
> > > Documentation/DeviceTree.
> > 
> > So how do I know that this the right way to specify the attibutes for a
> > TPM device and all vendors would like to have the attributes like this?
> 
> If you accept the patch it becomes the right way for DT.

OK, thanks for educating me with this! My knowledge of DT is thin. I wasn't
aware that things where so unstandardized.

I'll take that point of view for the next version of the patch set and
just try to make sense whether the attributes make sense to me.

> Jason

/Jarkko

------------------------------------------------------------------------------

Re: [PATCH v2 0/3] TPM2.0: Added eventlog support for TPM2.0

[Jarkko Sakkinen]

On Thu, Aug 25, 2016 at 09:25:36PM -0400, Jarkko Sakkinen wrote:
> On Thu, Aug 25, 2016 at 03:20:38PM -0600, Jason Gunthorpe wrote:
> > On Thu, Aug 25, 2016 at 05:13:04PM -0400, Jarkko Sakkinen wrote:
> > > > Other DT platforms like ARM will use the bindings defined in
> > > > Documentation/DeviceTree.
> > > 
> > > So how do I know that this the right way to specify the attibutes for a
> > > TPM device and all vendors would like to have the attributes like this?
> > 
> > If you accept the patch it becomes the right way for DT.
> 
> OK, thanks for educating me with this! My knowledge of DT is thin. I wasn't
> aware that things where so unstandardized.
> 
> I'll take that point of view for the next version of the patch set and
> just try to make sense whether the attributes make sense to me.

I asked my employer to order me a Raspberry PI 3 in order to have a
device that utilizes a device tree instead of ACPI (and also to have
something to test SPI and I2C connected TPMs). I'm looking forward to
use that to test this patch set.

/Jarkko

------------------------------------------------------------------------------

Re: [PATCH v2 0/3] TPM2.0: Added eventlog support for TPM2.0

[Jarkko Sakkinen]

On Fri, Aug 26, 2016 at 07:43:16AM -0400, Jarkko Sakkinen wrote:
> On Thu, Aug 25, 2016 at 09:25:36PM -0400, Jarkko Sakkinen wrote:
> > On Thu, Aug 25, 2016 at 03:20:38PM -0600, Jason Gunthorpe wrote:
> > > On Thu, Aug 25, 2016 at 05:13:04PM -0400, Jarkko Sakkinen wrote:
> > > > > Other DT platforms like ARM will use the bindings defined in
> > > > > Documentation/DeviceTree.
> > > > 
> > > > So how do I know that this the right way to specify the attibutes for a
> > > > TPM device and all vendors would like to have the attributes like this?
> > > 
> > > If you accept the patch it becomes the right way for DT.
> > 
> > OK, thanks for educating me with this! My knowledge of DT is thin. I wasn't
> > aware that things where so unstandardized.
> > 
> > I'll take that point of view for the next version of the patch set and
> > just try to make sense whether the attributes make sense to me.
> 
> I asked my employer to order me a Raspberry PI 3 in order to have a
> device that utilizes a device tree instead of ACPI (and also to have
> something to test SPI and I2C connected TPMs). I'm looking forward to
> use that to test this patch set.

Or actually after some investigation Minnowboard is a better choice.
It's quite trivial to test either ACPI or DT with it, it seems.

/Jarkko

------------------------------------------------------------------------------

Re: [PATCH v2 2/3] TPM2.0: TPM Device Tree Documentation

[Nayna]

On 08/19/2016 01:36 AM, Jason Gunthorpe wrote:
> On Tue, Aug 16, 2016 at 11:30:40PM +0530, Nayna wrote:
>> Here is the description:
>>
>> phandle property:
>
> IIRC these are automatically generated by the dt compiler and are not
> part of the binding. Please confirm if you need them in the binding
> document.
>
> Jason

Yeah Jason, these were not needed, have removed them in my new patch of 
Device Tree Documentation, posted just now.

Thanks & Regards,
Nayna

>


------------------------------------------------------------------------------

Re: [PATCH v2 0/3] TPM2.0: Added eventlog support for TPM2.0

[Nayna]

On 08/19/2016 01:25 AM, Jason Gunthorpe wrote:
> On Tue, Aug 16, 2016 at 02:21:53PM +0530, Nayna wrote:
>
>> I tried the suggested approach and since ACPI specific functions won't be
>> available for arch using CONFIG_OF, so the compilation fails and vice versa
>> for CONFIG_ACPI..
>
> Right, you need to stub out the read_X function with an empty inline
> when not available.
Ok

>
>> Jason, for understanding.. can you explain the issue with existing design
>> for read_log, where arch specific read_log is compiled based on CONFIG
>> option.. And then, we can just change in Makefile.. where currently it is
>> like
>>
>> ifdef CONFIG_ACPI
>>         tpm-y += tpm_acpi.o
>> else
>> ifdef CONFIG_OF
>>         tpm-y += tpm_of.o
>> endif
>> endif
>
> The 'else' is the main issue
Ok
>
>
>> this can be changed to
>> tpm-$(CONFIG_ACPI) += tpm_acpi.o
>> tpm-$(CONFIG_OF) += tpm_of.o
>
> This is fine by me, *however* only if the .o files do not define
> overlapping symbols as they do today (eg tpm_acpi contains read_log_acpi)
>
> Use some small #ifdef logic in the header file to create the inline
> stub.

Sure, my latest posted PATCH v3 4/7 does the fix as suggested here.

Thanks & Regards,
   - Nayna

>
> Jason
>


------------------------------------------------------------------------------