Messages in this thread | | | From | Matthew Garrett <> | Subject | Re: [PATCH 2/4] driver: provide sysfs interfaces to access TXT config space | Date | Wed, 8 May 2013 02:44:50 +0000 |
| |
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
| |