[prev in list] [next in list] [prev in thread] [next in thread] 

List:       tpmdd-devel
Subject:    Re: [tpmdd-devel] [PATCH] add tpm_xenu.ko: Xen Virtual TPM frontend driver
From:       "Fioravante, Matthew E." <Matthew.Fioravante () jhuapl ! edu>
Date:       2012-11-08 15:40:34
Message-ID: 068F06DC4D106941B297C0C5F9F446EA48A229E705 () aplesstripe ! dom1 ! jhuapl ! edu
[Download RAW message or body]



-----Original Message-----
From: Konrad Rzeszutek Wilk [mailto:konrad.wilk@oracle.com] 
Sent: Wednesday, November 07, 2012 11:07 AM
To: Fioravante, Matthew E.
Cc: key@linux.vnet.ibm.com; mail@srajiv.net; jeremy@goop.org; \
tpmdd-devel@lists.sourceforge.net; xen-devel@lists.xensource.com; \
                linux-kernel@vger.kernel.org
Subject: Re: [PATCH] add tpm_xenu.ko: Xen Virtual TPM frontend driver

On Wed, Nov 07, 2012 at 09:05:26AM -0500, Matthew Fioravante wrote:
> On 11/06/2012 02:39 PM, Konrad Rzeszutek Wilk wrote:
> > On Mon, Nov 05, 2012 at 10:09:57AM -0500, Matthew Fioravante wrote:
> > > This patch ports the xen vtpm frontend driver for linux from the 
> > > linux-2.6.18-xen.hg tree to linux-stable.
> > So how does on test it ? Set it up? Use it? Is there some 
> > documentation about it - if so it should be in the patch description.
> Thats actually a question I had. To use this driver now you have to 
> use my vtpm mini-os domains which are currently being evaluated in the 
> xen-devel mailing list. Once they are accepted I will submit a 
> documentation update to the Xen tree.
> 
> Whats the best practice for documentation in this case? All in xen?
> Some linux/some xen? If the latter, how much goes in linux and where?

As much as possible. I would say stick both of them in Xen and in Linux.
And you can designate one of them as primary (say the Xen one) and say in the Linux:

"For up-to-date information, please refer to XXYYZZ"

I grepped through the linux source code and didn't see any documentation for the \
other xen drivers. I could put a "please refer to XXX in xen" line in the Kconfig or \
as a comment in the source code or both. Am I missing something or is there some \
standard way the xen devs handle this documentation issue for linux drivers?

> > 
> > I did a very very cursory look at it, see some of the comments.
> > 
> > > 
> > > +
> > > +
> > > +static inline struct transmission *transmission_alloc(void) {
> > > +     return kzalloc(sizeof(struct transmission), GFP_ATOMIC); }
> > > +
> > > +     static unsigned char *
> > 
> > That is very weird tabbing? Did you run this patch through 
> > scripts/checkpatch.pl ?
> Wow thats ugly. I ran the check script and it looks like it didn't 
> pick this up. For some reason my editor wants to autoindent like that.
> Fixed.
> > 
> > > +
> > > +static const struct file_operations vtpm_ops = {
> > > +     .owner = THIS_MODULE,
> > > +     .llseek = no_llseek,
> > > +     .open = tpm_open,
> > > +     .read = tpm_read,
> > > +     .write = tpm_write,
> > > +     .release = tpm_release,
> > > +};
> > > +
> > > +static DEVICE_ATTR(pubek, S_IRUGO, tpm_show_pubek, NULL); static 
> > > +DEVICE_ATTR(pcrs, S_IRUGO, tpm_show_pcrs, NULL); static 
> > > +DEVICE_ATTR(enabled, S_IRUGO, tpm_show_enabled, NULL); static 
> > > +DEVICE_ATTR(active, S_IRUGO, tpm_show_active, NULL); static 
> > > +DEVICE_ATTR(owned, S_IRUGO, tpm_show_owned, NULL); static 
> > > +DEVICE_ATTR(temp_deactivated, S_IRUGO, tpm_show_temp_deactivated,
> > > +             NULL);
> > > +static DEVICE_ATTR(caps, S_IRUGO, tpm_show_caps, NULL); static 
> > > +DEVICE_ATTR(cancel, S_IWUSR | S_IWGRP, NULL, tpm_store_cancel);
> > > +
> > > +static struct attribute *vtpm_attrs[] = {
> > > +     &dev_attr_pubek.attr,
> > > +     &dev_attr_pcrs.attr,
> > > +     &dev_attr_enabled.attr,
> > > +     &dev_attr_active.attr,
> > > +     &dev_attr_owned.attr,
> > > +     &dev_attr_temp_deactivated.attr,
> > > +     &dev_attr_caps.attr,
> > > +     &dev_attr_cancel.attr,
> > > +     NULL,
> > So are these going to show up in SysFS? If so, there should also be a 
> > corresponding file in Documentation/.../sysfs/something.
> These are similar to the entries made by the other tpm drivers. I 
> don't see any documentation about those either. TPM maintainers, any 
> guidance there?
> > 
> > > +#include "tpm.h"
> > > +#include "tpm_vtpm.h"
> > > +
> > > +#undef DEBUG
> > > +
> > > +#define GRANT_INVALID_REF 0
> > Interesting. The 0 grant value is actually a valid one. I think you 
> > want (-1ULL).
> Is it?
> drivers/block/xen-blkfront.c and
> drivers/net/xen-netfront.c
> 
> do the exact same thing

You are right. Just leave it as that then.

> > > +
> > > +     init_tpm_xenbus();
> > > +     return 0;
> > > +}
> > > +
> > > +
> > > +module_init(tpmif_init);
> > no module_exit?
> Will fix
> 
> 



------------------------------------------------------------------------------
Everyone hates slow websites. So do we.
Make your web apps faster with AppDynamics
Download AppDynamics Lite for free today:
http://p.sf.net/sfu/appdyn_d2d_nov
_______________________________________________
tpmdd-devel mailing list
tpmdd-devel@lists.sourceforge.net
https://lists.sourceforge.net/lists/listinfo/tpmdd-devel


[prev in list] [next in list] [prev in thread] [next in thread]