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@xxxxxxxxx>
> +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@xxxxxxxxx>
> +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@xxxxxxxxx>
> +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@xxxxxxxxx>
> +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@xxxxxxxxx>
> +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@xxxxxxxxx>
> +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@xxxxxxxxx>
> +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@xxxxxxxxx>
> +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@xxxxxxxxxxxxx
��.n��������+%������w��{.n������_���v��z����n�r������&��z�ޗ�zf���h���~����������_��+v���)ߣ�
