diff mbox

[tpmdd-devel,v9,2/4] tpm: Proxy driver for supporting multiple emulated TPMs

Message ID 20160411181403.GB371@obsidianresearch.com
State New
Headers show

Commit Message

Jason Gunthorpe April 11, 2016, 6:14 p.m. UTC
On Mon, Apr 11, 2016 at 11:43:58AM +0300, Jarkko Sakkinen wrote:
> On Thu, Apr 07, 2016 at 11:49:44AM -0400, Stefan Berger wrote:
> > On 04/07/2016 08:35 AM, Jarkko Sakkinen wrote:
> > >On Tue, Mar 29, 2016 at 02:19:12PM -0400, Stefan Berger wrote:
> > >>This patch implements a proxy driver for supporting multiple emulated TPMs
> > >>in a system.
> > >>
> > >>The driver implements a device /dev/vtpmx that is used to created
> > >>a client device pair /dev/tpmX (e.g., /dev/tpm10) and a server side that
> > >>is accessed using a file descriptor returned by an ioctl.
> > >>The device /dev/tpmX is the usual TPM device created by the core TPM
> > >>driver. Applications or kernel subsystems can send TPM commands to it
> > >>and the corresponding server-side file descriptor receives these
> > >>commands and delivers them to an emulated TPM.
> > >>
> > >>Signed-off-by: Stefan Berger <stefanb@linux.vnet.ibm.com>
> > >>CC: linux-kernel@vger.kernel.org
> > >>CC: linux-doc@vger.kernel.org
> > >>CC: linux-api@vger.kernel.org
> > >Reviewed-by: Jarkko Sakkinen <jarkko.sakkinen@linux.intel.com>
> > >Tested-by: Jarkko Sakkinen <jarkko.sakkinen@linux.intel.com>
> > 
> > Thanks. So I can post a v10 where I have to re-introduce the priv field but
> > put it into the tpm_chip struct. Obviously it needs this field. I am not
> > sure whether you'll let me take the Reviewed-by and Tested-by, though?
> 
> Lets hold for them then. I'll do retest when I get the new series.

Lets just fix the sysfs stuff the same way we fixed ppi and be done
with this issue.

Something that looks kinda like this untested thing:


------------------------------------------------------------------------------
Find and fix application performance issues faster with Applications Manager
Applications Manager provides deep performance insights into multiple tiers of
your business applications. It resolves application problems quickly and
reduces your MTTR. Get your free trial! http://pubads.g.doubleclick.net/
gampad/clk?id=1444514301&iu=/ca-pub-7940484522588532

Comments

Stefan Berger April 11, 2016, 7:33 p.m. UTC | #1
Jason Gunthorpe <jgunthorpe@obsidianresearch.com> wrote on 04/11/2016 
02:14:03 PM:


> 
> On Mon, Apr 11, 2016 at 11:43:58AM +0300, Jarkko Sakkinen wrote:
> > On Thu, Apr 07, 2016 at 11:49:44AM -0400, Stefan Berger wrote:
> > > On 04/07/2016 08:35 AM, Jarkko Sakkinen wrote:
> > > >On Tue, Mar 29, 2016 at 02:19:12PM -0400, Stefan Berger wrote:
> > > >>This patch implements a proxy driver for supporting multiple 
> emulated TPMs
> > > >>in a system.
> > > >>
> > > >>The driver implements a device /dev/vtpmx that is used to created
> > > >>a client device pair /dev/tpmX (e.g., /dev/tpm10) and a serverside 
that
> > > >>is accessed using a file descriptor returned by an ioctl.
> > > >>The device /dev/tpmX is the usual TPM device created by the core 
TPM
> > > >>driver. Applications or kernel subsystems can send TPM commands to 
it
> > > >>and the corresponding server-side file descriptor receives these
> > > >>commands and delivers them to an emulated TPM.
> > > >>
> > > >>Signed-off-by: Stefan Berger <stefanb@linux.vnet.ibm.com>
> > > >>CC: linux-kernel@vger.kernel.org
> > > >>CC: linux-doc@vger.kernel.org
> > > >>CC: linux-api@vger.kernel.org
> > > >Reviewed-by: Jarkko Sakkinen <jarkko.sakkinen@linux.intel.com>
> > > >Tested-by: Jarkko Sakkinen <jarkko.sakkinen@linux.intel.com>
> > > 
> > > Thanks. So I can post a v10 where I have to re-introduce the 
> priv field but
> > > put it into the tpm_chip struct. Obviously it needs this field. I am 
not
> > > sure whether you'll let me take the Reviewed-by and Tested-by, 
though?
> > 
> > Lets hold for them then. I'll do retest when I get the new series.
> 
> Lets just fix the sysfs stuff the same way we fixed ppi and be done
> with this issue.
> 
> Something that looks kinda like this untested thing:

If the intent is to get rid of the priv field, I am not sure whether we 
can solve the problem this way. You may remember that the vtpm proxy 
driver only registers the chip once startup and retrieval of durations and 
timeouts successfully ran. But we need to access the proxy_dev before 
that:

https://github.com/stefanberger/linux/commit/3811e51743637efe11f9df99ddb616881fc34c04#diff-4f54b830a2a73696c2455277f4abbc41R230

Here the retrieval of timeouts and durations.

https://github.com/stefanberger/linux/commit/3b284694c802f718f36b07a3ddc005e343432338#diff-4f54b830a2a73696c2455277f4abbc41R369


If this patch is some form of cleanup, that may be a different story.


    Stefan


> 
> diff --git a/drivers/char/tpm/tpm-sysfs.c b/drivers/char/tpm/tpm-sysfs.c
> index a7c3473c3421..51e898be4307 100644
> --- a/drivers/char/tpm/tpm-sysfs.c
> +++ b/drivers/char/tpm/tpm-sysfs.c
> @@ -36,7 +36,7 @@ static ssize_t pubek_show(struct device *dev, 
> struct device_attribute *attr,
>     int i, rc;
>     char *str = buf;
> 
> -   struct tpm_chip *chip = dev_get_drvdata(dev);
> +   struct tpm_chip *chip = to_tpm_chip(dev);
> 
>     tpm_cmd.header.in = tpm_readpubek_header;
>     err = tpm_transmit_cmd(chip, &tpm_cmd, READ_PUBEK_RESULT_SIZE,
> @@ -92,7 +92,7 @@ static ssize_t pcrs_show(struct device *dev, 
> struct device_attribute *attr,
>     ssize_t rc;
>     int i, j, num_pcrs;
>     char *str = buf;
> -   struct tpm_chip *chip = dev_get_drvdata(dev);
> +   struct tpm_chip *chip = to_tpm_chip(dev);
> 
>     rc = tpm_getcap(dev, TPM_CAP_PROP_PCR, &cap,
>           "attempting to determine the number of PCRS");
> @@ -222,7 +222,7 @@ static DEVICE_ATTR_RO(caps);
>  static ssize_t cancel_store(struct device *dev, struct 
> device_attribute *attr,
>               const char *buf, size_t count)
>  {
> -   struct tpm_chip *chip = dev_get_drvdata(dev);
> +   struct tpm_chip *chip = to_tpm_chip(dev);
>     if (chip == NULL)
>        return 0;
> 
> @@ -234,7 +234,7 @@ static DEVICE_ATTR_WO(cancel);
>  static ssize_t durations_show(struct device *dev, struct 
> device_attribute *attr,
>                 char *buf)
>  {
> -   struct tpm_chip *chip = dev_get_drvdata(dev);
> +   struct tpm_chip *chip = to_tpm_chip(dev);
> 
>     if (chip->duration[TPM_LONG] == 0)
>        return 0;
> @@ -251,7 +251,7 @@ static DEVICE_ATTR_RO(durations);
>  static ssize_t timeouts_show(struct device *dev, struct 
> device_attribute *attr,
>                char *buf)
>  {
> -   struct tpm_chip *chip = dev_get_drvdata(dev);
> +   struct tpm_chip *chip = to_tpm_chip(dev);
> 
>     return sprintf(buf, "%d %d %d %d [%s]\n",
>               jiffies_to_usecs(chip->timeout_a),
> @@ -283,22 +283,33 @@ static const struct attribute_group tpm_dev_group 
= {
> 
>  int tpm_sysfs_add_device(struct tpm_chip *chip)
>  {
> -   int err;
> -   err = sysfs_create_group(&chip->dev.parent->kobj,
> -             &tpm_dev_group);
> +   const struct attribute **i;
> 
> -   if (err)
> -      dev_err(&chip->dev,
> -         "failed to create sysfs attributes, %d\n", err);
> -   return err;
> +   chip->groups[chip->groups_cnt++] = &tpm_dev_group;
> +   if (chip->flags & TPM_CHIP_FLAG_TPM2)
> +      return 0;
> +
> +   for (i = tpm_dev_attrs; *i != NULL; ++i) {
> +      rc = __compat_only_sysfs_link_entry_to_kobj(
> +          &chip->dev.parent->kobj, &chip->dev.kobj, (*i)->name);
> +      if (rc && rc != -ENOENT) {
> +         tpm_sysfs_del_device(chip);
> +         return rc;
> +      }
> +   }
> +
> +   return 0;
>  }
> 
>  void tpm_sysfs_del_device(struct tpm_chip *chip)
>  {
> -   /* The sysfs routines rely on an implicit tpm_try_get_ops, this
> -    * function is called before ops is null'd and the sysfs core
> -    * synchronizes this removal so that no callbacks are running or can
> -    * run again
> +   const struct attribute **i;
> +
> +   /* The sysfs routines rely on an implicit tpm_try_get_ops, 
device_del
> +    * is called before ops is null'd and the sysfs core synchronizes 
this
> +    * removal so that no callbacks are running or can run again
>      */
> -   sysfs_remove_group(&chip->dev.parent->kobj, &tpm_dev_group);
> +
> +   for (i = tpm_dev_attrs; *i != NULL; ++i)
> +      sysfs_remove_link(&chip->dev.parent->kobj, (*i)->name);
>  }
> 
> 
------------------------------------------------------------------------------
> Find and fix application performance issues faster with Applications 
Manager
> Applications Manager provides deep performance insights into multiple 
tiers of
> your business applications. It resolves application problems quickly and
> reduces your MTTR. Get your free trial! http://pubads.g.doubleclick.net/
> gampad/clk?id=1444514301&iu=/ca-pub-7940484522588532
> _______________________________________________
> tpmdd-devel mailing list
> tpmdd-devel@lists.sourceforge.net
> https://lists.sourceforge.net/lists/listinfo/tpmdd-devel
>
------------------------------------------------------------------------------
Find and fix application performance issues faster with Applications Manager
Applications Manager provides deep performance insights into multiple tiers of
your business applications. It resolves application problems quickly and
reduces your MTTR. Get your free trial!
https://ad.doubleclick.net/ddm/clk/302982198;130105516;z
Jason Gunthorpe April 11, 2016, 8:28 p.m. UTC | #2
On Mon, Apr 11, 2016 at 03:33:10PM -0400, Stefan Berger wrote:
>    If the intent is to get rid of the priv field, I am not sure whether we
>    can solve the problem this way.

We are not 'getting rid' of priv, the desire is to replace chip->priv with
dev_get_drvdata(&chip->dev), which is more consistent with the core driver
model.

> You may remember that the vtpm proxy driver only registers the chip
> once startup and retrieval of durations and timeouts successfully

Doesn't matter, just dev_set_drvdata right after the chip is
allocated, just like vtpm does already for chip->priv.

> If this patch is some form of cleanup, that may be a different
> story.

The issue with swapping in drvdata is that your patch series causes
the sysfs code to conflict with it here:

>    > @@ -36,7 +36,7 @@ static ssize_t pubek_show(struct device *dev,
>    > struct device_attribute *attr,
>    >     int i, rc;
>    >     char *str = buf;
>    >
>    > -   struct tpm_chip *chip = dev_get_drvdata(dev);
>    > +   struct tpm_chip *chip = to_tpm_chip(dev);

This broad outline is a different way for you to address the NULL
parent & sysfs issue by using optional symlinks instead. By doing this
it is no longer necessary to abuse drvdata in the core code.

ie your sysfs patch would not longer call drv_set_drvdata.

Jason

------------------------------------------------------------------------------
Find and fix application performance issues faster with Applications Manager
Applications Manager provides deep performance insights into multiple tiers of
your business applications. It resolves application problems quickly and
reduces your MTTR. Get your free trial!
https://ad.doubleclick.net/ddm/clk/302982198;130105516;z
Stefan Berger April 11, 2016, 8:30 p.m. UTC | #3
Jason Gunthorpe <jgunthorpe@obsidianresearch.com> wrote on 04/11/2016 
04:28:11 PM:


> 
> On Mon, Apr 11, 2016 at 03:33:10PM -0400, Stefan Berger wrote:
> >    If the intent is to get rid of the priv field, I am not sure 
whether we
> >    can solve the problem this way.
> 
> We are not 'getting rid' of priv, the desire is to replace chip->priv 
with
> dev_get_drvdata(&chip->dev), which is more consistent with the core 
driver
> model.
> 
> > You may remember that the vtpm proxy driver only registers the chip
> > once startup and retrieval of durations and timeouts successfully
> 
> Doesn't matter, just dev_set_drvdata right after the chip is
> allocated, just like vtpm does already for chip->priv.

We don't have a dev at this point.

   Stefan
------------------------------------------------------------------------------
Find and fix application performance issues faster with Applications Manager
Applications Manager provides deep performance insights into multiple tiers of
your business applications. It resolves application problems quickly and
reduces your MTTR. Get your free trial!
https://ad.doubleclick.net/ddm/clk/302982198;130105516;z
Jason Gunthorpe April 11, 2016, 8:57 p.m. UTC | #4
On Mon, Apr 11, 2016 at 04:30:34PM -0400, Stefan Berger wrote:

>    > Doesn't matter, just dev_set_drvdata right after the chip is
>    > allocated, just like vtpm does already for chip->priv.
>    We don't have a dev at this point.

Eh? If you have a chip you have a dev.

Jason

------------------------------------------------------------------------------
Find and fix application performance issues faster with Applications Manager
Applications Manager provides deep performance insights into multiple tiers of
your business applications. It resolves application problems quickly and
reduces your MTTR. Get your free trial!
https://ad.doubleclick.net/ddm/clk/302982198;130105516;z
Stefan Berger April 11, 2016, 10:12 p.m. UTC | #5
Jason Gunthorpe <jgunthorpe@obsidianresearch.com> wrote on 04/11/2016 
04:57:18 PM:


> 
> On Mon, Apr 11, 2016 at 04:30:34PM -0400, Stefan Berger wrote:
> 
> >    > Doesn't matter, just dev_set_drvdata right after the chip is
> >    > allocated, just like vtpm does already for chip->priv.
> >    We don't have a dev at this point.
> 
> Eh? If you have a chip you have a dev.

Ok. I would nevertheless like to reduce the churn in the series where I 
would post next a v10.

I am using chip->dev.platform_data = proxy_dev to store the proxy_dev. 
Here's the current v10:

https://github.com/stefanberger/linux/commits/vtpm-driver.v10


> 
> Jason
>
------------------------------------------------------------------------------
Find and fix application performance issues faster with Applications Manager
Applications Manager provides deep performance insights into multiple tiers of
your business applications. It resolves application problems quickly and
reduces your MTTR. Get your free trial!
https://ad.doubleclick.net/ddm/clk/302982198;130105516;z
diff mbox

Patch

diff --git a/drivers/char/tpm/tpm-sysfs.c b/drivers/char/tpm/tpm-sysfs.c
index a7c3473c3421..51e898be4307 100644
--- a/drivers/char/tpm/tpm-sysfs.c
+++ b/drivers/char/tpm/tpm-sysfs.c
@@ -36,7 +36,7 @@  static ssize_t pubek_show(struct device *dev, struct device_attribute *attr,
 	int i, rc;
 	char *str = buf;
 
-	struct tpm_chip *chip = dev_get_drvdata(dev);
+	struct tpm_chip *chip = to_tpm_chip(dev);
 
 	tpm_cmd.header.in = tpm_readpubek_header;
 	err = tpm_transmit_cmd(chip, &tpm_cmd, READ_PUBEK_RESULT_SIZE,
@@ -92,7 +92,7 @@  static ssize_t pcrs_show(struct device *dev, struct device_attribute *attr,
 	ssize_t rc;
 	int i, j, num_pcrs;
 	char *str = buf;
-	struct tpm_chip *chip = dev_get_drvdata(dev);
+	struct tpm_chip *chip = to_tpm_chip(dev);
 
 	rc = tpm_getcap(dev, TPM_CAP_PROP_PCR, &cap,
 			"attempting to determine the number of PCRS");
@@ -222,7 +222,7 @@  static DEVICE_ATTR_RO(caps);
 static ssize_t cancel_store(struct device *dev, struct device_attribute *attr,
 			    const char *buf, size_t count)
 {
-	struct tpm_chip *chip = dev_get_drvdata(dev);
+	struct tpm_chip *chip = to_tpm_chip(dev);
 	if (chip == NULL)
 		return 0;
 
@@ -234,7 +234,7 @@  static DEVICE_ATTR_WO(cancel);
 static ssize_t durations_show(struct device *dev, struct device_attribute *attr,
 			      char *buf)
 {
-	struct tpm_chip *chip = dev_get_drvdata(dev);
+	struct tpm_chip *chip = to_tpm_chip(dev);
 
 	if (chip->duration[TPM_LONG] == 0)
 		return 0;
@@ -251,7 +251,7 @@  static DEVICE_ATTR_RO(durations);
 static ssize_t timeouts_show(struct device *dev, struct device_attribute *attr,
 			     char *buf)
 {
-	struct tpm_chip *chip = dev_get_drvdata(dev);
+	struct tpm_chip *chip = to_tpm_chip(dev);
 
 	return sprintf(buf, "%d %d %d %d [%s]\n",
 		       jiffies_to_usecs(chip->timeout_a),
@@ -283,22 +283,33 @@  static const struct attribute_group tpm_dev_group = {
 
 int tpm_sysfs_add_device(struct tpm_chip *chip)
 {
-	int err;
-	err = sysfs_create_group(&chip->dev.parent->kobj,
-				 &tpm_dev_group);
+	const struct attribute **i;
 
-	if (err)
-		dev_err(&chip->dev,
-			"failed to create sysfs attributes, %d\n", err);
-	return err;
+	chip->groups[chip->groups_cnt++] = &tpm_dev_group;
+	if (chip->flags & TPM_CHIP_FLAG_TPM2)
+		return 0;
+
+	for (i = tpm_dev_attrs; *i != NULL; ++i) {
+		rc = __compat_only_sysfs_link_entry_to_kobj(
+		    &chip->dev.parent->kobj, &chip->dev.kobj, (*i)->name);
+		if (rc && rc != -ENOENT) {
+			tpm_sysfs_del_device(chip);
+			return rc;
+		}
+	}
+
+	return 0;
 }
 
 void tpm_sysfs_del_device(struct tpm_chip *chip)
 {
-	/* The sysfs routines rely on an implicit tpm_try_get_ops, this
-	 * function is called before ops is null'd and the sysfs core
-	 * synchronizes this removal so that no callbacks are running or can
-	 * run again
+	const struct attribute **i;
+
+	/* The sysfs routines rely on an implicit tpm_try_get_ops, device_del
+	 * is called before ops is null'd and the sysfs core synchronizes this
+	 * removal so that no callbacks are running or can run again
 	 */
-	sysfs_remove_group(&chip->dev.parent->kobj, &tpm_dev_group);
+
+	for (i = tpm_dev_attrs; *i != NULL; ++i)
+		sysfs_remove_link(&chip->dev.parent->kobj, (*i)->name);
 }