Re: [Xen-devel] [tpmdd-devel] [PATCH v3] drivers/tpm: add xen tpmfront interface

[Daniel De Graaf <dgdegra@xxxxxxxxxxxxx>]

On 06/04/2013 06:14 PM, Peter Hüwe wrote:
> Hi Daniel,
>
> thanks for this v3.
> It's really nice to see the progress and I really like that
> sparse/smatch/clang/coccicheck do not complain at all - nice job!
>
> Konrad already did an excellent job at reviewing the driver (thanks for that),
> and all previously pointed out issues are fixed.
>
> Unfortunately I haven't had the opportunity to test it yet, but
> the driver looks clean from the TPM perspective.
>
>
> However I do have some minor comments from a general perspective - see below.
>
>
> > From the TPM point of view I'd say it is fine.
> (I'm currently _not_ the (official) maintainer of the tpm subsystem but at least
> take care of the incoming stuff as an interim)
>
>
> So if the comments are addressed you can add:
> Acked-by: Peter Huewe <peterhuewe@xxxxxx>
> Reviewed-by: Peter Huewe <peterhuewe@xxxxxx>
I believe I have addressed the comments in v4 (sorry for the delay, I was away
from this email for a few weeks).

> @Konrad: I can stage the driver and push it to James or you can take it.
> As it lives in drivers/tpm maybe it should go through the tpm (interim ;)
> maintainer and james' tree.
>
>
> So here are my comments:
>
> Am Dienstag, 28. Mai 2013, 17:40:32 schrieb Daniel De Graaf:
> > This is a complete rewrite of the Xen TPM frontend driver, taking
> > advantage of a simplified frontend/backend interface and adding support
> > for cancellation and timeouts.  The backend for this driver is provided
> > by a vTPM stub domain using the interface in Xen 4.3.
> >
> > Signed-off-by: Daniel De Graaf <dgdegra@xxxxxxxxxxxxx>
> > Acked-by: Matthew Fioravante <matthew.fioravante@xxxxxxxxxx>
> > diff --git a/Documentation/xen-tpmfront.txt
> > b/Documentation/xen-tpmfront.txt new file mode 100644
> > index 0000000..8a61d6f
> > --- /dev/null
> > +++ b/Documentation/xen-tpmfront.txt
> > @@ -0,0 +1,116 @@
> > +Copyright (c) 2010-2012 United States Government, as represented by
> > +the Secretary of Defense.  All rights reserved.
> I'm not 100% sure if this can stay this way, as it doesn't permit any changes
> to the documentation itself.
This copyright statement has been removed; the portions of the text under 
copyright
have been released under the GPL, so the kernel's COPYING statement suffices.

> > + * Linux DomU: The Linux based guest that wants to use a vTPM. There many
> > be +           more than one of these.
> -* Linux DomU: The Linux based guest that wants to use a vTPM. There many
> +* Linux DomU: The Linux based guest that wants to use a vTPM. There may
> Just a minor typo
Fixed, thanks.

> > diff --git a/drivers/char/tpm/Kconfig b/drivers/char/tpm/Kconfig
> > index dbfd564..205ed35 100644
> > --- a/drivers/char/tpm/Kconfig
> > +++ b/drivers/char/tpm/Kconfig
> > @@ -91,4 +91,15 @@ config TCG_ST33_I2C
> >           To compile this driver as a module, choose M here; the module will
> > be called tpm_stm_st33_i2c.
> >
> > +config TCG_XEN
> Maybe TCG_XEN_TPM would be better, but TCG_XEN is okay for me.
TCG_XEN seems to match the other Kconfig menu items better.
 
> > +static int vtpm_send(struct tpm_chip *chip, u8 *buf, size_t count)
> > +{
> > +       struct tpm_private *priv = TPM_VPRIV(chip);
> > +       struct vtpm_shared_page *shr = priv->shr;
> > +       unsigned int offset = shr_data_offset(shr);
> > +
> > +       u32 ordinal;
> > +       unsigned long duration;
> > +
> > +       if (offset > PAGE_SIZE)
> > +               return -EIO;
> Maybe -EINVAL?
> > +
> > +       if (offset + count > PAGE_SIZE)
> > +               return -EIO;
> Maybe -EINVAL?
Both changed.

> > +/*************************************************************************
> > ***** + * tpmif.h
> > + *
> > + * TPM I/O interface for Xen guest OSes, v2
> > + *
> > + * Permission is hereby granted, free of charge, to any person obtaining a
> > copy + * of this software and associated documentation files (the
> > "Software"), to + * deal in the Software without restriction, including
> > without limitation the + * rights to use, copy, modify, merge, publish,
> > distribute, sublicense, and/or + * sell copies of the Software, and to
> > permit persons to whom the Software is + * furnished to do so, subject to
> > the following conditions:
> > + *
> > + * The above copyright notice and this permission notice shall be included
> > in + * all copies or substantial portions of the Software.
> > + *
> > + * THE SOFTWARE IS PROVIDED "AS IS", WITHOUT WARRANTY OF ANY KIND, EXPRESS
> > OR + * IMPLIED, INCLUDING BUT NOT LIMITED TO THE WARRANTIES OF
> > MERCHANTABILITY, + * FITNESS FOR A PARTICULAR PURPOSE AND NONINFRINGEMENT.
> > IN NO EVENT SHALL THE + * AUTHORS OR COPYRIGHT HOLDERS BE LIABLE FOR ANY
> > CLAIM, DAMAGES OR OTHER + * LIABILITY, WHETHER IN AN ACTION OF CONTRACT,
> > TORT OR OTHERWISE, ARISING + * FROM, OUT OF OR IN CONNECTION WITH THE
> > SOFTWARE OR THE USE OR OTHER + * DEALINGS IN THE SOFTWARE.
> > + *
> > + */
> Also not sure if this license is correct/compliant with the kernel as it
> indicates no clear license to me.
>
> Peter
Since the portions of the header covered by copyright (the v1 interface) have 
been
removed from the Linux copy, the remainder is public domain and I have removed 
the
notice.


--
Daniel De Graaf
National Security Agency

_______________________________________________
Xen-devel mailing list
Xen-devel@xxxxxxxxxxxxx
http://lists.xen.org/xen-devel