lkml.org 
[lkml]   [2013]   [May]   [7]   [last100]   RSS Feed
Views: [wrap][no wrap]   [headers]  [forward] 
 
Messages in this thread
/
From
SubjectRe: [PATCH 2/4] driver: provide sysfs interfaces to access TXT config space
Date
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
\
 
 \ /
  Last update: 2013-05-08 05:21    [W:0.137 / U:0.084 seconds]
©2003-2020 Jasper Spaans|hosted at Digital Ocean and TransIP|Read the blog|Advertise on this site