Re: [PATCH 2/4] driver: provide sysfs interfaces to access TXT config space

[Matthew Garrett <matthew.garrett@nebula.com>]

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