From mboxrd@z Thu Jan 1 00:00:00 1970 From: "Stefan Berger" Subject: Re: [PATCH v3 09/11] tpm: Driver for supporting multiple emulated TPMs Date: Tue, 23 Feb 2016 07:09:44 -0500 Message-ID: <201602231210.u1NCADWB008643@d03av03.boulder.ibm.com> References: <1455885728-10315-1-git-send-email-stefanb@linux.vnet.ibm.com><1455885728-10315-10-git-send-email-stefanb@linux.vnet.ibm.com> <20160223102211.GA9474@intel.com> Mime-Version: 1.0 Content-Type: multipart/mixed; boundary="===============4792615263352736671==" Return-path: In-Reply-To: <20160223102211.GA9474-ral2JQCrhuEAvxtiuMwx3w@public.gmane.org> List-Unsubscribe: , List-Archive: List-Post: List-Help: List-Subscribe: , Errors-To: tpmdd-devel-bounces-5NWGOfrQmneRv+LV9MX5uipxlwaOVQ5f@public.gmane.org To: Jarkko Sakkinen Cc: tpmdd-devel-5NWGOfrQmneRv+LV9MX5uipxlwaOVQ5f@public.gmane.org List-Id: tpmdd-devel@lists.sourceforge.net --===============4792615263352736671== Content-Type: multipart/alternative; boundary="=_alternative 0042D7A685257F62_=" --=_alternative 0042D7A685257F62_= Content-Transfer-Encoding: quoted-printable Content-Type: text/plain; charset="US-ASCII" Jarkko Sakkinen wrote on 02/23/2016=20 05:22:11 AM: >=20 > On Fri, Feb 19, 2016 at 07:42:06AM -0500, Stefan Berger wrote: > > This patch implements a driver for supporting multiple emulated TPMs=20 in a > > system. > >=20 > > 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=20 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. > >=20 > > Signed-off-by: Stefan Berger > > --- > > drivers/char/tpm/Kconfig | 10 + > > drivers/char/tpm/Makefile | 1 + > > drivers/char/tpm/tpm-vtpm.c | 543 +++++++++++++++++++++++++++++++ > +++++++++++++ > > include/uapi/linux/Kbuild | 1 + > > include/uapi/linux/vtpm.h | 38 ++++ > > 5 files changed, 593 insertions(+) > > create mode 100644 drivers/char/tpm/tpm-vtpm.c > > create mode 100644 include/uapi/linux/vtpm.h > >=20 > > diff --git a/drivers/char/tpm/Kconfig b/drivers/char/tpm/Kconfig > > index 3b84a8b..4c4e843 100644 > > --- a/drivers/char/tpm/Kconfig > > +++ b/drivers/char/tpm/Kconfig > > @@ -122,5 +122,15 @@ config TCG=5FCRB > > from within Linux. To compile this driver as a module, choose > > M here; the module will be called tpm=5Fcrb. > >=20 > > + > > +struct vtpm=5Fdev { > > + struct tpm=5Fchip *chip; > > + > > + u32 flags; /* public API flags */ > > + > > + long state; > > +#define STATE=5FOPENED=5FBIT 0 > > +#define STATE=5FWAIT=5FRESPONSE=5FBIT 1 /* waiting for emulator to = > give response */ >=20 > I'd prefer something like this before declaring the struct: >=20 > enum vtpm=5Fdev=5Fstates { > VTPM=5FDEV=5FOPENED =3D BIT(0), > VTPM=5FDEV=5FWAITING=5FFOR=5FRESPONSE =3D BIT(1), > }; >=20 > This whole use of set/clear=5Fbit macros when you don't have variable > number of bits just makes code less transparent. Though read-modify-writes with the bit ops are atomic and need no spinlock = protection to avoid concurrency mess. Now we would need a spinlock for=20 such ops. >=20 > > + > > + spinlock=5Ft buf=5Flock; /* lock for buffers */ > > + > > + wait=5Fqueue=5Fhead=5Ft wq; > > + > > + size=5Ft req=5Flen; /* length of queued TPM request */ > > + size=5Ft resp=5Flen; /* length of queued TPM response */ > > + u8 buffer[TPM=5FBUFSIZE]; /* request/response buffer */ >=20 > I'd use alloc=5Fpage() with GFP=5FUSERHIGH in order to be a better citizen > in 32-bit environments. You can kmap() it when you need it. >=20 > > +}; > > + > > + > > +static void vtpm=5Fdelete=5Fdevice=5Fpair(struct vtpm=5Fdev *vtpm=5Fde= v); > > + > > +/* > > + * Functions related to 'server side' > > + */ > > + > > +/** > > + * vtpm=5Ffops=5Fread - Read TPM commands on 'server side' > > + * > > + * Return value: > > + * Number of bytes read or negative error code > > + */ > > +static ssize=5Ft vtpm=5Ffops=5Fread(struct file *filp, char =5F=5Fuser= *buf, > > + size=5Ft count, loff=5Ft *off) > > +{ > > + struct vtpm=5Fdev *vtpm=5Fdev =3D filp->private=5Fdata; > > + size=5Ft len; > > + int sig, rc; > > + > > + sig =3D wait=5Fevent=5Finterruptible(vtpm=5Fdev->wq, vtpm=5Fdev->re= q=5Flen !=3D=20 0); > > + if (sig) > > + return -EINTR; > > + > > + spin=5Flock(&vtpm=5Fdev->buf=5Flock); > > + > > + len =3D vtpm=5Fdev->req=5Flen; > > + > > + if (count < len) { > > + spin=5Funlock(&vtpm=5Fdev->buf=5Flock); > > + pr=5Fdebug("Invalid size in recv: count=3D%zd, req=5Flen=3D%zd\n= ", > > + count, len); > > + return -EIO; > > + } > > + > > + rc =3D copy=5Fto=5Fuser(buf, vtpm=5Fdev->buffer, len); > > + memset(vtpm=5Fdev->buffer, 0, len); > > + vtpm=5Fdev->req=5Flen =3D 0; >=20 > I saw already Jason's comment but maybe you could just use a mutex > instead of a spinlock? I'll let Jason respond to it whether it's the difference between spinlock=20 and mutex or just no locking at all. If no locking, I'd be inclined to=20 work with two buffers, one for requests and one for responses. > > + > > +/* > > + * Called when core TPM driver reads TPM responses from 'server side' > > + * > > + * Return value: > > + * Number of TPM response bytes read, negative error value=20 otherwise > > + */ > > +static int vtpm=5Ftpm=5Fop=5Frecv(struct tpm=5Fchip *chip, u8 *buf, si= ze=5Ft=20 count) > > +{ > > + struct vtpm=5Fdev *vtpm=5Fdev =3D chip->vendor.priv; > > + int sig; > > + size=5Ft len; > > + > > + if (!vtpm=5Fdev) > > + return -EIO; > > + > > + /* wait for response or responder gone */ > > + sig =3D wait=5Fevent=5Finterruptible(vtpm=5Fdev->wq, > > + (vtpm=5Fdev->resp=5Flen !=3D 0 > > + || !test=5Fbit(STATE=5FOPENED=5FBIT, &vtpm=5Fdev->state))); > > + > > + if (sig) > > + return -EINTR; With us not operating this driver in interrupt mode, we don't need the=20 wait=5Fevent=5Finterruptible if we make another change further below: > > + > > +static u8 vtpm=5Ftpm=5Fop=5Fstatus(struct tpm=5Fchip *chip) > > +{ > > + return 0; > > +} I modified this to=20 static u8 vtpm=5Ftpm=5Fop=5Fstatus(struct tpm=5Fchip *chip) { return (chip->resp=5Flen) ? VTPM=5FREQ=5FCOMPLETE=5FFLAG : 0; } > > + > > +static bool vtpm=5Ftpm=5Freq=5Fcanceled(struct tpm=5Fchip *chip, u8 s= tatus) > > +{ > > + return (status =3D=3D 0); > > +} This will have to remain like this. > > + > > +static const struct tpm=5Fclass=5Fops vtpm=5Ftpm=5Fops =3D { > > + .recv =3D vtpm=5Ftpm=5Fop=5Frecv, > > + .send =3D vtpm=5Ftpm=5Fop=5Fsend, > > + .cancel =3D vtpm=5Ftpm=5Fop=5Fcancel, > > + .status =3D vtpm=5Ftpm=5Fop=5Fstatus, > > + .req=5Fcomplete=5Fmask =3D 0, > > + .req=5Fcomplete=5Fval =3D 0, .req=5Fcomplete=5Fmask =3D VTPM=5FREQ=5FCOMPLETE=5FFLAG, .req=5Fcomplete=5Fval =3D VTPM=5FREQ=5FCOMPLETE=5FFLAG, with #define VTPM=5FREQ=5FCOMPLETE=5FFLAG BIT(0) Agreed? Stefan --=_alternative 0042D7A685257F62_= Content-Transfer-Encoding: quoted-printable Content-Type: text/html; charset="US-ASCII" Jarkko Sakkinen <jarkko.sakkinen-VuQAYsv1563Yd54FQh9/CA@public.gmane.org> wrote on 02/23/2016 05:22:11 AM:


= >
> On Fri, Feb 19, 2016 at 07:42:06AM -0500, Stefan Berger wrote= :
> > This patch implements a driver for supporting multiple emula= ted TPMs in a
> > system.
> >
> > The driver implem= ents a device /dev/vtpmx that is used to created
> > a client devi= ce pair /dev/tpmX (e.g., /dev/tpm10) and a server side that
> > is accessed using a file descriptor returned by an i= octl.
> > The device /dev/tpmX is the usual TPM device created by = the core TPM
> > driver. Applications or kernel subsystems can send TPM com= mands to it
> > and the corresponding server-side file descriptor receiv= es these
> > commands and delivers them to an emulated TPM.
>= ; >
> > Signed-off-by: Stefan Berger <stefanb-23VcF4HTsmITcAY+lMofuw@public.gmane.org= m.com>
> > ---
> >  drivers/char/tpm/Kconfig &nbs= p;  |  10 +
> >  drivers/char/tpm/Makefile   |=   1 +
> >  drivers/char/tpm/tpm-vtpm.c | 543 ++++++++++= +++++++++++++++++++++
> +++++++++++++
> >  include/uapi= /linux/Kbuild   |   1 +
> >  include/uapi/linux/vtp= m.h   |  38 ++++
> >  5 files changed, 593 insertio= ns(+)
> >  create mode 100644 drivers/char/tpm/tpm-vtpm.c
= > >  create mode 100644 include/uapi/linux/vtpm.h
> > <= br>> > diff --git a/drivers/char/tpm/Kconfig b/drivers/char/tpm/Kconf= ig
> > index 3b84a8b..4c4e843 100644
> > --- a/drivers/ch= ar/tpm/Kconfig
> > +++ b/drivers/char/tpm/Kconfig
> > @@ = -122,5 +122,15 @@ config TCG=5FCRB
> >       from w= ithin Linux.  To compile this driver as a module, choose
> >       M here; the mo= dule will be called tpm=5Fcrb.
> >  

<= font size=3D2>> > +
> > +struct vtpm=5Fdev {
> > + =   struct tpm=5Fchip *chip;
> > +
> > +   u32 fl= ags;                   /* public API flags */
> > +
> > + &= nbsp; long state;
> > +#define STATE=5FOPENED=5FBIT     =    0
> > +#define STATE=5FWAIT=5FRESPONSE=5FBIT 1  =  /* waiting for emulator to
> give response */
>
> I'd prefer something= like this before declaring the struct:
>
> enum vtpm=5Fdev=5F= states {
>    VTPM=5FDEV=5FOPENED       &nbs= p; =3D BIT(0),
>    VTPM=5FDEV=5FWAITING=5FFOR=5FRESPONSE &= nbsp; =3D BIT(1),
> };
>
> This whole use of set/clear= =5Fbit macros when you don't have variable
> number of bits just make= s code less transparent.

Though read-= modify-writes with the bit ops are atomic and need no spinlock protection to avoid concurrency mess. Now we would need a spinlock for such ops.


>= ;
> > +
> > +   spinlock=5Ft buf=5Flock;   &nb= sp;     /* lock for buffers */
> > +
> > +   wait=5Fqueue=5Fhea= d=5Ft wq;
> > +
> > +   size=5Ft req=5Flen;   &= nbsp;          /* length of queued TPM request */
> > +   size=5Ft res= p=5Flen;             /* length of queued TPM response */
> > +   u8 buffer[= TPM=5FBUFSIZE];      /* request/response buffer */
>
> I'd use alloc=5Fpage() with GFP=5FUSERHIGH in or= der to be a better citizen
> in 32-bit environments. You can kmap() i= t when you need it.
>
> > +};
> > +
> > +=
> > +static void vtpm=5Fdelete=5Fdevice=5Fpair(struct vtpm=5Fdev = *vtpm=5Fdev);
> > +
> > +/*
> > + * Functions re= lated to 'server side'
> > + */
> > +
> > +/**> > + * vtpm=5Ffops=5Fread - Read TPM commands on 'server side'
= > > + *
> > + * Return value:
> > + *   Number= of bytes read or negative error code
> > + */
> > +stati= c ssize=5Ft vtpm=5Ffops=5Fread(struct file *filp, char =5F=5Fuser *buf,
> > +               size= =5Ft count, loff=5Ft *off)
> > +{
> > +   struct vtpm=5Fdev *vtp= m=5Fdev =3D filp->private=5Fdata;
> > +   size=5Ft len;> > +   int sig, rc;
> > +
> > +   sig = =3D wait=5Fevent=5Finterruptible(vtpm=5Fdev->wq, vtpm=5Fdev->req=5Flen !=3D 0);
> > +   if (sig)
> > +      = return -EINTR;
> > +
> > +   spin=5Flock(&vtpm= =5Fdev->buf=5Flock);
> > +
> > +   len =3D vtpm= =5Fdev->req=5Flen;
> > +
> > +   if (count < l= en) {
> > +      spin=5Funlock(&vtpm=5Fdev->= buf=5Flock);
> > +      pr=5Fdebug("Invalid si= ze in recv: count=3D%zd, req=5Flen=3D%zd\n",
> > +          c= ount, len);
> > +      return -EIO;
> > + =   }
> > +
> > +   rc =3D copy=5Fto=5Fuser(buf, = vtpm=5Fdev->buffer, len);
> > +   memset(vtpm=5Fdev->bu= ffer, 0, len);
> > +   vtpm=5Fdev->req=5Flen =3D 0;
>= ;
> I saw already Jason's comment but maybe you could just use a mut= ex
> instead of a spinlock?

I'l= l let Jason respond to it whether it's the difference between spinlock and mutex or just no locking at all. If no locking, I'd be inclined to work with two buffers, one for requests and one for response= s.



> > +
> > += /*
> > + * Called when core TPM driver reads TPM responses from 's= erver side'
> > + *
> > + * Return value:
> > + * &nbs= p;    Number of TPM response bytes read, negative error value otherwise
> > + */
> > +static int vtpm=5Ftpm= =5Fop=5Frecv(struct tpm=5Fchip *chip, u8 *buf, size=5Ft count)
> > +{
> > +   struct vtpm=5Fdev *vt= pm=5Fdev =3D chip->vendor.priv;
> > +   int sig;
> &= gt; +   size=5Ft len;
> > +
> > +   if (!vtpm= =5Fdev)
> > +      return -EIO;
> > +
&= gt; > +   /* wait for response or responder gone */
> > + =   sig =3D wait=5Fevent=5Finterruptible(vtpm=5Fdev->wq,
> >= +      (vtpm=5Fdev->resp=5Flen !=3D 0
> > + &nb= sp;    || !test=5Fbit(STATE=5FOPENED=5FBIT, &vtpm=5Fdev->s= tate)));
> > +
> > +   if (sig)
> > +  = ;    return -EINTR;


Wit= h us not operating this driver in interrupt mode, we don't need the wait=5Fevent=5Finterruptible if we make another change fu= rther below:


> > +
> > += static u8 vtpm=5Ftpm=5Fop=5Fstatus(struct tpm=5Fchip *chip)
> > +{=
> > +   return 0;
> > +}


I modified this to

static u8 vtpm=5Ftpm=5Fop=5Fstatus(struct= tpm=5Fchip *chip)
{
    &n= bsp;   return(chip->resp=5Flen) ? VTPM=5FREQ=5FCOMPLETE=5FFL= AG : 0= ;
}


> = > +
> > +static bool vtpm=5Ftpm=5Freq=5Fcanceled(struct tpm=5Fc= hip  *chip, u8 status)
> > +{
> > +   return (status =3D=3D 0);<= br>> > +}

This will have to rem= ain like this.

> > +
> &g= t; +static const struct tpm=5Fclass=5Fops vtpm=5Ftpm=5Fops =3D {
> &g= t; +   .recv =3D vtpm=5Ftpm=5Fop=5Frecv,
> > +   .send = =3D vtpm=5Ftpm=5Fop=5Fsend,
> > +   .cancel =3D vtpm=5Ftpm=5F= op=5Fcancel,
> > +   .status =3D vtpm=5Ftpm=5Fop=5Fstatus,> > +   .req=5Fcomplete=5Fmask =3D 0,
> > +   .re= q=5Fcomplete=5Fval =3D 0,

        .req=5Fcomplete=5Fmask =3D VTPM=5FREQ=5FCOMPLETE=5FFLAG,
        .req=5Fcomplete=5Fval =3D VTPM=5FREQ=5FCOMPLETE=5FFLAG,

with

#def= ine VTPM=5FRE= Q=5FCOMPLETE=5FFLAG BIT(= 0)


Ag= reed?

   Stefan=

--=_alternative 0042D7A685257F62_=-- --===============4792615263352736671== Content-Type: text/plain; charset="us-ascii" MIME-Version: 1.0 Content-Transfer-Encoding: 7bit Content-Disposition: inline ------------------------------------------------------------------------------ Site24x7 APM Insight: Get Deep Visibility into Application Performance APM + Mobile APM + RUM: Monitor 3 App instances at just $35/Month Monitor end-to-end web transactions and take corrective actions now Troubleshoot faster and improve end-user experience. Signup Now! http://pubads.g.doubleclick.net/gampad/clk?id=272487151&iu=/4140 --===============4792615263352736671== Content-Type: text/plain; charset="us-ascii" MIME-Version: 1.0 Content-Transfer-Encoding: 7bit Content-Disposition: inline _______________________________________________ tpmdd-devel mailing list tpmdd-devel-5NWGOfrQmneRv+LV9MX5uipxlwaOVQ5f@public.gmane.org https://lists.sourceforge.net/lists/listinfo/tpmdd-devel --===============4792615263352736671==--