lkml.org 
[lkml]   [2013]   [Jun]   [4]   [last100]   RSS Feed
Views: [wrap][no wrap]   [headers]  [forward] 
 
Messages in this thread
/
From
SubjectRe: [tpmdd-devel] [PATCH v3] drivers/tpm: add xen tpmfront interface
Date
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@gmx.de>
Reviewed-by: Peter Huewe <peterhuewe@gmx.de>

@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@tycho.nsa.gov>
> Acked-by: Matthew Fioravante <matthew.fioravante@jhuapl.edu>

> 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.


> + * 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


> 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.


> +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?
> +


> +/*************************************************************************
> ***** + * 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


\
 
 \ /
  Last update: 2013-06-05 00:41    [W:0.176 / U:0.080 seconds]
©2003-2020 Jasper Spaans|hosted at Digital Ocean and TransIP|Read the blog|Advertise on this site