From: Matthew Garrett <matthew.garrett@nebula.com>
To: Qiaowei Ren <qiaowei.ren@intel.com>
CC: "linux-kernel@vger.kernel.org" <linux-kernel@vger.kernel.org>,
        "platform-driver-x86@vger.kernel.org" 
	<platform-driver-x86@vger.kernel.org>,
        Xiaoyan Zhang <xiaoyan.zhang@intel.com>, Gang Wei <gang.wei@intel.com>
Subject: Re: [PATCH 2/4] driver: provide sysfs interfaces to access TXT
 config space
Thread-Topic: [PATCH 2/4] driver: provide sysfs interfaces to access TXT
 config space
Thread-Index: AQHOSvE/a+ghBFBXCUi0/GPSGvVi2pj6liGA
Date: Wed, 8 May 2013 02:44:50 +0000
Message-ID: <1367981089.2425.18.camel@x230>
References: <1367938519-840-1-git-send-email-qiaowei.ren@intel.com>
	 <1367938519-840-3-git-send-email-qiaowei.ren@intel.com>
In-Reply-To: <1367938519-840-3-git-send-email-qiaowei.ren@intel.com>
Accept-Language: en-US
Content-Language: en-US
Content-Type: text/plain; charset="utf-8"
Content-ID: <A26D3EAD3F3FED48A3EF429A14366771@namprd05.prod.outlook.com>
MIME-Version: 1.0
Sender: linux-kernel-owner@vger.kernel.org
Content-Transfer-Encoding: 8bit
Content-Length: 4575
Lines: 126

On Tue, 2013-05-07 at 22:55 +0800, Qiaowei Ren wrote:

> +		Registers in the private space can only be accessed after a
> +		measured environment has been established and before the
> +		TXT.CMD.CLOSE-PRIVATE command has been issued. 

Is userspace ever going to be running in this situation?

> +What:		/sys/devices/platform/intel_txt/config/STS_raw
> +Date:		May 2013
> +KernelVersion:	3.9
> +Contact:	"Qiaowei Ren" <qiaowei.ren@intel.com>
> +Description:	TXT.STS is the general status register. This read-only register
> +		is used by AC modules and the MLE to get the status of various
> +		Intel TXT features.

AC? MLE?

> +What:		/sys/devices/platform/intel_txt/config/STS_enter_done
> +Date:		May 2013
> +KernelVersion:	3.9
> +Contact:	"Qiaowei Ren" <qiaowei.ren@intel.com>
> +Description:	The chipset sets SENTER.DONE.STS status bit when it sees all
> +		of the threads have done an TXT.CYC.SENTER-ACK.

All of which threads? It might be worth adding a general introduction to
TXT in Documentation and referencing it in these entries.

> +What:		/sys/devices/platform/intel_txt/config/STS_sexit_done
> +Date:		May 2013
> +KernelVersion:	3.9
> +Contact:	"Qiaowei Ren" <qiaowei.ren@intel.com>
> +Description:	SEXIT.DONE.STS status bit is set when all of the bits in the
> +		TXT.THREADS.JOIN register are clear. Thus, this bit will be
> +		set immediately after reset.

It will? When will it be clear? What would userspace ever want this for?

> +Contact:	"Qiaowei Ren" <qiaowei.ren@intel.com>
> +Description:	TXT.SINIT.BASE register contains the physical base address
> +		of the memory region set aside by the BIOS for loading an
> +		SINIT AC module. If BIOS has provided an SINIT AC module,
> +		it will be located at this address. System software that
> +		provides an SINIT AC module must store it to this location.

Why would userspace ever care about this?

> +What:		/sys/devices/platform/intel_txt/config/SINIT_SIZE
> +Date:		May 2013
> +KernelVersion:	3.9
> +Contact:	"Qiaowei Ren" <qiaowei.ren@intel.com>
> +Description:	TXT.SINIT.SIZE register contains the size (in bytes) of
> +		the memory region set aside by the BIOS for loading an
> +		SINIT AC module. This register is initialized by the BIOS.

Or this?

> +What:		/sys/devices/platform/intel_txt/config/MLE_JOIN
> +Date:		May 2013
> +KernelVersion:	3.9
> +Contact:	"Qiaowei Ren" <qiaowei.ren@intel.com>
> +Description:	Holds a physical address pointer to the base of the join
> +		data structure used to initialize RLPs in response to
> +		GETSEC[WAKEUP].

RLPs?

> +What:		/sys/devices/platform/intel_txt/config/HEAP_BASE
> +Date:		May 2013
> +KernelVersion:	3.9
> +Contact:	"Qiaowei Ren" <qiaowei.ren@intel.com>
> +Description:	TXT.HEAP.BASE register contains the physical base address
> +		of the Intel TXT Heap memory region. The BIOS initializes
> +		this register.

Again, it doesn't seem obvious why userspace would ever want this...

> +What:		/sys/devices/platform/intel_txt/config/HEAP_SIZE
> +Date:		May 2013
> +KernelVersion:	3.9
> +Contact:	"Qiaowei Ren" <qiaowei.ren@intel.com>
> +Description:	TXT.HEAP.SIZE register contains the size (in bytes) of the
> +		Intel TXT Heap memory region. The BIOS initializes this
> +		register.

Or this.

Basically, don't just define what these files do - make it clear why
they'd be used. Right now I have no idea whether these are diagnostic,
likely to be used during runtime or basically completely useless.

> +static ssize_t txt_show_ERRORCODE(struct device *dev,
> +				  struct device_attribute *attr, char *buf)
> +{
> +	return show_config(buf, off_ERRORCODE);
> +}

Much as I usually hate it, these all seem pretty boilerplate. It would
arguably be cleaner to replace them all with something like:

#define config_func(x) static size_t txt_show_x(struct device *dev,
struct device_attribute *attr, char *buf) { return show_config(buf
off_x);}\ntxt_store_x(struct device *dev, struct device_attribute *attr,
const char *buf, size_t count) { blah blah

config_func(ERRORCODE)
config_func(ESTS_raw)

and so on.

> +int sysfs_create_config(struct kobject *parent)
> +{
> +	return sysfs_create_group(parent, &config_attr_grp);
> +}
> +EXPORT_SYMBOL_GPL(sysfs_create_config);

This doesn't seem right. You're linking this into the existing txt
module - you don't need to export symbols.

> +MODULE_LICENSE("GPL");

Or declare a module license.

-- 
Matthew Garrett | mjg59@srcf.ucam.org
????{.n?+???????+%?????ݶ??w??{.n?+????{??G?????{ay?ʇڙ?,j??f???h?????????z_??(?階?ݢj"???m??????G????????????&???~???iO???z??v?^?m????????????I?