* Re: [PATCH v9 0/9] Add simple NVMEM Framework via regmap.
From: Stefan Wahren @ 2015-08-10 11:24 UTC (permalink / raw)
To: Srinivas Kandagatla, Andrew Lunn
Cc: wxt, linux-api, Rob Herring, sboyd, arnd, linux-arm-kernel,
khilman, s.hauer, Greg Kroah-Hartman, linux-kernel, linux-arm-msm,
mporter, pantelis.antoniou, devicetree, Mark Brown,
maitysanchayan, p.zabel
In-Reply-To: <55C87EEF.6020609@linaro.org>
Hi Srinivas,
hi Andrew,
[add Sanchayan and Philipp]
> Srinivas Kandagatla <srinivas.kandagatla@linaro.org> hat am 10. August 2015 um
> 12:37 geschrieben:
>
>
> Hi Andrew,
>
> On 09/08/15 16:19, Andrew Lunn wrote:
> > Hi Srinivas
> >
> > The AT24 eeprom driver contains the comment:
> >
> > /*
> > * Export the EEPROM bytes through sysfs, since that's convenient.
> > * By default, only root should see the data (maybe passwords etc)
> > */
> >
> > and as you would expect from this:
> >
> > # ls -l
> > ./devices/platform/soc/soc:aips-bus@40000000/40066000.i2c/i2c-0/0-0050/eeprom
> > -rw------- 1 root root 512 Aug 9 10:16
> > ./devices/platform/soc/soc:aips-bus@40000000/40066000.i2c/i2c-0/0-0050/eeprom
> >
> > The AT25 and the MAX6875 driver are the same.
> >
> > However nvmem has different defaults:
> >
> > # ls -l
> > ./devices/platform/soc/soc:aips-bus@40000000/40066000.i2c/i2c-0/0-0050/0-00500/nvmem
> > -rw-r--r-- 1 root root 0 Aug 9 10:16
> > ./devices/platform/soc/soc:aips-bus@40000000/40066000.i2c/i2c-0/0-0050/0-00500/nvmem
> >
> Yes, nvmem by default sets the read permission for everyone S_IRUGO.
> Which was the most common use case.
i can only speak for mxs-ocotp, but root-only access is sufficient.
>
> > Has this been considered and discussed?
> Thanks for bringing this up, No, we did not discuss this explicitly, but
> it should not be an issue to accommodate this. Provider still have
> flexibility to configure such things.
IMHO it would better the default settings of the framework would be compatible
and same restrictive as the eeprom drivers.
In case a provider needs more privileges, it should be his job.
>
> As a first patchset we wanted to keep the framework simple and add
> features as we move on.
That's true, but it's an ABI too. So changing default settings soon is better.
Regards
Stefan
>
> Thanks,
> srini
> >
> > Thanks
> > Andrew
> >
>
> _______________________________________________
> linux-arm-kernel mailing list
> linux-arm-kernel@lists.infradead.org
> http://lists.infradead.org/mailman/listinfo/linux-arm-kernel
^ permalink raw reply
* [PATCH 0/3] SysFS driver for QEMU firmware config device (fw_cfg)
From: Gabriel L. Somlo @ 2015-08-10 16:31 UTC (permalink / raw)
To: gregkh, ralf, zajec5, paul, galak, linux-api, linux-kernel
Cc: linux-efi, rjones, jordan.l.justen, x86, qemu-devel, gleb,
matt.fleming, kernelnewbies, kraxel, pbonzini, lersek
From: "Gabriel Somlo" <somlo@cmu.edu>
This patch set makes QEMU fw_cfg blobs available for viewing (read-only)
via SysFS.
Several different architectures supported by QEMU are set up with a
"firmware configuration" (fw_cfg) device, used to pass configuration
"blobs" into the guest by the host running QEMU.
Historically, these config blobs were mostly of interest to the guest
BIOS, but since QEMU v2.4 it is possible to insert arbitrary blobs via
the command line, which makes them potentially interesting to userspace
(e.g. for passing early boot environment variables, etc.).
In addition to cc-ing the people and lists indicated by get-maintainer.pl,
I've added a few extra lists suggested by Matt Fleming on the qemu-devel
list, as well as the qemu-devel list itself.
Also cc-ing kernelnewbies, as this is my very first kenel contribution,
so please go easy on me for whatever silly n00b mistakes I might have still
missed, in spite of trying hard to do all my homework properly... :)
The series consists of three patches:
1/3 - probes for the qemu fw_cfg device in locations known to work on
the supported architectures, in decreasing order of "likelihood".
While it *may* be possible to detect the presence of fw_cfg via
acpi or dtb (on x86 and arm, respectively), there's no way I know
of attempting that on sun4 and ppc/mac, so I've stuck with simply
probing (the fw_cfg_modes[] structure and fw_cfg_io_probe() function)
in fw_cfg.c. I could use some advice on how else that could be
done more elegantly, if needed.
Upon successfully detecting a present fw_cfg device, we set up
/sys/firmware/fw_cfg/by_select entries for each blob available
on the fw_cfg device.
2/3 - export kset_find_obj() (in lib/kobject.c) for use with modules,
which will come in handy in patch #3, below.
3/3 - add a "user friendly" way of listing fw_cfg blobs by name rather
than by unique selector key.
Since fw_cfg blob names are traditionally set up to look like
path names, it would be nice to mirror that fact when displaying
them under /sys/firmware/fw_cfg in a "by_name" subdirectory.
I'm using ksets to reflect subdirectories matching "dirname"
tokens separated by '/' within each fw_cfg blob "filename",
and symlinks into the "by_select" subdirectory for each "basename".
Since the fw_cfg device doesn't enforce that blob names have
well-behaved and non-conflicting names, it is possible (though
unlikely) that there will be blobs named:
"etc/foo/bar"
and
"etc/foo"
where "foo" will try to be both a subdirectory AND a symlink under
/sys/firmware/fw_cfg/by_name/etc/. In such an event, the latter
fw_cfg blob will simply be skipped from having an entry created
under the user-friendly "by_name" subdirectory, remaining listed
only as an entry under the "by_select" subdirectory.
I have tested this on x86_64 and armv7hl+lpae kernels. Builds and installs
OK as both a module and linked directly into the kernel.
TIA for the feedback and advice,
--Gabriel
Gabriel Somlo (3):
firmware: introduce sysfs driver for QEMU's fw_cfg device
kobject: export kset_find_obj() to be used from modules
firmware: fw_cfg: create directory hierarchy for fw_cfg file names
Documentation/ABI/testing/sysfs-firmware-fw_cfg | 211 +++++++++
drivers/firmware/Kconfig | 10 +
drivers/firmware/Makefile | 1 +
drivers/firmware/fw_cfg.c | 542 ++++++++++++++++++++++++
lib/kobject.c | 1 +
5 files changed, 765 insertions(+)
create mode 100644 Documentation/ABI/testing/sysfs-firmware-fw_cfg
create mode 100644 drivers/firmware/fw_cfg.c
--
2.4.3
^ permalink raw reply
* [PATCH 1/3] firmware: introduce sysfs driver for QEMU's fw_cfg device
From: Gabriel L. Somlo @ 2015-08-10 16:31 UTC (permalink / raw)
To: gregkh, ralf, zajec5, paul, galak, linux-api, linux-kernel
Cc: linux-efi, rjones, jordan.l.justen, x86, qemu-devel, gleb,
matt.fleming, kernelnewbies, kraxel, pbonzini, lersek
In-Reply-To: <cover.1439220623.git.somlo@cmu.edu>
From: "Gabriel Somlo" <somlo@cmu.edu>
Make fw_cfg entries of type "file" available via sysfs. Entries
are listed under /sys/firmware/fw_cfg/by_select, in folders named
after each entry's selector key. Filename, selector value, and
size read-only attributes are included for each entry. Also, a
"raw" attribute allows retrieval of the full binary content of
each entry.
This patch also provides a documentation file outlining the
guest-side "hardware" interface exposed by the QEMU fw_cfg device.
Signed-off-by: Gabriel Somlo <somlo@cmu.edu>
---
Documentation/ABI/testing/sysfs-firmware-fw_cfg | 169 +++++++++
drivers/firmware/Kconfig | 10 +
drivers/firmware/Makefile | 1 +
drivers/firmware/fw_cfg.c | 438 ++++++++++++++++++++++++
4 files changed, 618 insertions(+)
create mode 100644 Documentation/ABI/testing/sysfs-firmware-fw_cfg
create mode 100644 drivers/firmware/fw_cfg.c
diff --git a/Documentation/ABI/testing/sysfs-firmware-fw_cfg b/Documentation/ABI/testing/sysfs-firmware-fw_cfg
new file mode 100644
index 0000000..3a7e7f2
--- /dev/null
+++ b/Documentation/ABI/testing/sysfs-firmware-fw_cfg
@@ -0,0 +1,169 @@
+What: /sys/firmware/fw_cfg/
+Date: August 2015
+Contact: Gabriel Somlo <somlo@cmu.edu>
+Description:
+ Several different architectures supported by QEMU (x86, arm,
+ sun4*, ppc/mac) are provisioned with a firmware configuration
+ (fw_cfg) device, used by the host to provide configuration data
+ to the starting guest. While most of this data is meant for use
+ by the guest BIOS, starting with QEMU v2.4, guest VMs may be
+ started with arbitrary fw_cfg entries supplied directly on the
+ command line, which therefore may be of interest to userspace.
+
+ === Guest-side Hardware Interface ===
+
+ The fw_cfg device is available to guest VMs as a pair (control
+ and data) of registers, accessible as either a IO ports or as
+ MMIO addresses, depending on the architecture.
+
+ --- Control Register ---
+
+ Width: 16-bit
+ Access: Write-Only
+ Endianness: LE (if IOport) or BE (if MMIO)
+
+ A write to the control register selects the index for one of
+ the firmware configuration items (or "blobs") available on the
+ fw_cfg device, which can subsequently be read from the data
+ register.
+
+ Each time the control register is written, an data offset
+ internal to the fw_cfg device will be set to zero. This data
+ offset impacts which portion of the selected fw_cfg blob is
+ accessed by reading the data register, as explained below.
+
+ --- Data Register ---
+
+ Width: 8-bit (if IOport), or 8/16/32/64-bit (if MMIO)
+ Access: Read-Only
+ Endianness: string preserving
+
+ The data register allows access to an array of bytes which
+ represent the fw_cfg blob last selected by a write to the
+ control register.
+
+ Immediately following a write to the control register, the data
+ offset will be set to zero. Each successful read access to the
+ data register will increment the data offset by the appropriate
+ access width.
+
+ Each fw_cfg blob has a maximum associated data length. Once the
+ data offset exceeds this maximum length, any subsequent reads
+ via the data register will return 0x00.
+
+ An N-byte wide read of the data register will return the next
+ available N bytes of the selected fw_cfg blob, as a substring,
+ in increasing address order, similar to memcpy(), zero-padded
+ if necessary should the maximum data length of the selected
+ item be reached, as described above.
+
+ --- Per-arch Register Details ---
+
+ -------------------------------------------------------------
+ arch access base ctrl ctrl data data
+ mode address offset endian offset width
+ max.
+ -------------------------------------------------------------
+ x86 IOport 0x510 0 LE 1 8
+ x86_64 IOport 0x510 0 LE 1 8
+ arm MMIO 0x9020000 8 BE 0 64
+ sun4u IOport 0x510 0 LE 1 8
+ sun4m MMIO 0xd00000510 0 BE 2 8
+ ppc/mac MMIO 0xf0000510 0 BE 2 8
+ -------------------------------------------------------------
+
+ NOTE: On platforms where the fw_cfg registers are exposed as
+ IO ports, the data port number will always be one greater than
+ the port number of the control register. I.e., the two ports
+ are overlapping, and can not be mapped separately.
+
+ === Firmware Configuration Items of Interest ===
+
+ Originally, the index key, size, and formatting of blobs in
+ fw_cfg was hard coded by mutual agreement between QEMU on the
+ host side, and the BIOS running on the guest. Later on, a file
+ transfer interface was added: by reading a special blob, the
+ fw_cfg consumer can retrieve a list of records containing the
+ name, selector key, and size of further fw_cfg blobs made
+ available by the host. Below we describe three fw_cfg blobs
+ of interest to the sysfs driver.
+
+ --- Signature (Key 0x0000, FW_CFG_SIGNATURE) ---
+
+ The presence of the fw_cfg device can be verified by selecting
+ the signature blob by writing 0x0000 to the control register,
+ and reading four bytes from the data register. If the fw_cfg
+ device is present, the four bytes read will match the ASCII
+ characters "QEMU".
+
+ --- Revision (Key 0x0001, FW_CFG_ID) ---
+
+ A 32-bit little-endian unsigned integer, this item is used as
+ an interface revision number.
+
+ --- File Directory (Key 0x0019, FW_CFG_FILE_DIR) ---
+
+ Any fw_cfg blobs stored at key 0x0020 FW_CFG_FILE_FIRST() or
+ higher will have an associated entry in this "directory" blob,
+ which facilitates the discovery of available items by software
+ (e.g. BIOS) running on the guest. The format of the directory
+ blob is shown below.
+
+ NOTE: All integers are stored in big-endian format!
+
+ /* the entire file directory "blob" */
+ struct FWCfgFiles {
+ uint32_t count; /* total number of entries */
+ struct FWCfgFile f[]; /* entry array, see below */
+ };
+
+ /* an individual directory entry, 64 bytes total */
+ struct FWCfgFile {
+ uint32_t size; /* size of referenced blob */
+ uint16_t select; /* blob selector key */
+ uint16_t reserved;
+ char name[56]; /* blob name, nul-term. ascii */
+ };
+
+ === SysFS fw_cfg Interface ===
+
+ The fw_cfg sysfs interface described in this document is only
+ intended to display discoverable blobs (i.e., those registered
+ with the file directory), as there is no way to determine the
+ presence or size of "legacy" blobs (with selector keys between
+ 0x0002 and 0x0018) programmatically.
+
+ All fw_cfg information is shown under:
+
+ /sys/firmware/fw_cfg/
+
+ The only legacy blob displayed is the fw_cfg device revision:
+
+ /sys/firmware/fw_cfg/rev
+
+ --- Discoverable fw_cfg blobs by selector key ---
+
+ All discoverable blobs listed in the fw_cfg file directory are
+ displayed as entries named after their unique selector key
+ value, e.g.:
+
+ /sys/firmware/fw_cfg/by_select/32
+ /sys/firmware/fw_cfg/by_select/33
+ /sys/firmware/fw_cfg/by_select/34
+ ...
+
+ Each such fw_cfg sysfs entry has the following values exported
+ as attributes:
+
+ name : The 56-byte nul-terminated ASCII string used as the
+ blob's 'file name' in the fw_cfg directory.
+ size : The length of the blob, as given in the fw_cfg
+ directory.
+ select : The value of the blob's selector key as given in the
+ fw_cfg directory. This value is the same as used in
+ the parent directory name.
+ how the rest of the entry should be interpreted.
+ raw : The raw bytes of the blob, obtained by selecting the
+ entry via the control register, and reading a number
+ of bytes equal to the blob size from the data
+ register.
diff --git a/drivers/firmware/Kconfig b/drivers/firmware/Kconfig
index 99c69a3..f5cbe81 100644
--- a/drivers/firmware/Kconfig
+++ b/drivers/firmware/Kconfig
@@ -136,6 +136,16 @@ config QCOM_SCM
bool
depends on ARM || ARM64
+config FW_CFG_SYSFS
+ tristate "QEMU fw_cfg device support in sysfs"
+ depends on SYSFS
+ default n
+ help
+ Say Y or M here to enable the exporting of the QEMU firmware
+ configuration (fw_cfg) file entries via sysfs. Entries are
+ found under /sys/firmware/fw_cfg when this option is enabled
+ and loaded.
+
source "drivers/firmware/broadcom/Kconfig"
source "drivers/firmware/google/Kconfig"
source "drivers/firmware/efi/Kconfig"
diff --git a/drivers/firmware/Makefile b/drivers/firmware/Makefile
index 4a4b897..b81b46e 100644
--- a/drivers/firmware/Makefile
+++ b/drivers/firmware/Makefile
@@ -13,6 +13,7 @@ obj-$(CONFIG_ISCSI_IBFT) += iscsi_ibft.o
obj-$(CONFIG_FIRMWARE_MEMMAP) += memmap.o
obj-$(CONFIG_QCOM_SCM) += qcom_scm.o
obj-$(CONFIG_QCOM_SCM) += qcom_scm-32.o
+obj-$(CONFIG_FW_CFG_SYSFS) += fw_cfg.o
CFLAGS_qcom_scm-32.o :=$(call as-instr,.arch_extension sec,-DREQUIRES_SEC=1)
obj-y += broadcom/
diff --git a/drivers/firmware/fw_cfg.c b/drivers/firmware/fw_cfg.c
new file mode 100644
index 0000000..be17411
--- /dev/null
+++ b/drivers/firmware/fw_cfg.c
@@ -0,0 +1,438 @@
+/*
+ * drivers/firmware/fw_cfg.c
+ *
+ * Expose entries from QEMU's firmware configuration (fw_cfg) device in
+ * sysfs (read-only, under "/sys/firmware/fw_cfg/...").
+ *
+ * Copyright 2015 Carnegie Mellon University
+ */
+
+#include <linux/module.h>
+#include <linux/capability.h>
+#include <linux/slab.h>
+#include <linux/io.h>
+#include <linux/ioport.h>
+#include <linux/ctype.h>
+
+/* selector values for "well-known" fw_cfg entries */
+#define FW_CFG_SIGNATURE 0x00
+#define FW_CFG_ID 0x01
+#define FW_CFG_FILE_DIR 0x19
+
+/* size in bytes of fw_cfg signature */
+#define FW_CFG_SIG_SIZE 4
+
+/* fw_cfg "file name" is up to 56 characters (including terminating nul) */
+#define FW_CFG_MAX_FILE_PATH 56
+
+/* fw_cfg file directory entry type */
+struct fw_cfg_file {
+ uint32_t size;
+ uint16_t select;
+ uint16_t reserved;
+ char name[FW_CFG_MAX_FILE_PATH];
+};
+
+/* fw_cfg device i/o access options type */
+struct fw_cfg_access {
+ phys_addr_t start;
+ uint8_t size;
+ uint8_t ctrl_offset;
+ uint8_t data_offset;
+ bool is_mmio;
+ const char *name;
+};
+
+/* fw_cfg device i/o access available options for known architectures */
+static struct fw_cfg_access fw_cfg_modes[] = {
+ { 0x510, 2, 0, 1, false, "fw_cfg on i386, sun4u" },
+ { 0x9020000, 10, 8, 0, true, "fw_cfg on arm" },
+ { 0xd00000510, 3, 0, 2, true, "fw_cfg on sun4m" },
+ { 0xf0000510, 3, 0, 2, true, "fw_cfg on ppc/mac" },
+ { }
+};
+
+/* fw_cfg device i/o currently selected option set */
+static struct fw_cfg_access *fw_cfg_mode;
+
+/* fw_cfg device i/o register addresses */
+static void __iomem *fw_cfg_dev_base;
+static void __iomem *fw_cfg_dev_ctrl;
+static void __iomem *fw_cfg_dev_data;
+
+/* atomic access to fw_cfg device (potentially slow i/o, so using mutex) */
+static DEFINE_MUTEX(fw_cfg_dev_lock);
+
+/* pick apropriate endianness for selector key */
+static inline uint16_t fw_cfg_sel_endianness(uint16_t select)
+{
+ return fw_cfg_mode->is_mmio ? cpu_to_be16(select) : cpu_to_le16(select);
+}
+
+/* type for fw_cfg "directory scan" visitor/callback function */
+typedef int (*fw_cfg_file_callback)(const struct fw_cfg_file *f);
+
+/* run a given callback on each fw_cfg directory entry */
+static int fw_cfg_scan_dir(fw_cfg_file_callback callback)
+{
+ int ret = 0;
+ uint32_t count, i;
+ struct fw_cfg_file f;
+
+ mutex_lock(&fw_cfg_dev_lock);
+ iowrite16(fw_cfg_sel_endianness(FW_CFG_FILE_DIR), fw_cfg_dev_ctrl);
+ ioread8_rep(fw_cfg_dev_data, &count, sizeof(count));
+ for (i = 0; i < be32_to_cpu(count); i++) {
+ ioread8_rep(fw_cfg_dev_data, &f, sizeof(f));
+ ret = callback(&f);
+ if (ret)
+ break;
+ }
+ mutex_unlock(&fw_cfg_dev_lock);
+ return ret;
+}
+
+/* read chunk of given fw_cfg blob (caller responsible for sanity-check) */
+static inline void fw_cfg_read_blob(uint16_t select,
+ void *buf, loff_t pos, size_t count)
+{
+ mutex_lock(&fw_cfg_dev_lock);
+ iowrite16(fw_cfg_sel_endianness(select), fw_cfg_dev_ctrl);
+ while (pos-- > 0)
+ ioread8(fw_cfg_dev_data);
+ ioread8_rep(fw_cfg_dev_data, buf, count);
+ mutex_unlock(&fw_cfg_dev_lock);
+}
+
+/* clean up fw_cfg device i/o setup */
+static void fw_cfg_io_cleanup(void)
+{
+ if (fw_cfg_mode->is_mmio) {
+ iounmap(fw_cfg_dev_base);
+ release_mem_region(fw_cfg_mode->start, fw_cfg_mode->size);
+ } else {
+ ioport_unmap(fw_cfg_dev_base);
+ release_region(fw_cfg_mode->start, fw_cfg_mode->size);
+ }
+}
+
+/* probe and map fw_cfg device */
+static int __init fw_cfg_io_probe(void)
+{
+ char sig[FW_CFG_SIG_SIZE];
+
+ for (fw_cfg_mode = &fw_cfg_modes[0];
+ fw_cfg_mode->start; fw_cfg_mode++) {
+
+ phys_addr_t start = fw_cfg_mode->start;
+ uint8_t size = fw_cfg_mode->size;
+
+ /* reserve and map mmio or ioport region */
+ if (fw_cfg_mode->is_mmio) {
+ if (!request_mem_region(start, size, fw_cfg_mode->name))
+ continue;
+ fw_cfg_dev_base = ioremap(start, size);
+ if (!fw_cfg_dev_base) {
+ release_mem_region(start, size);
+ continue;
+ }
+ } else {
+ if (!request_region(start, size, fw_cfg_mode->name))
+ continue;
+ fw_cfg_dev_base = ioport_map(start, size);
+ if (!fw_cfg_dev_base) {
+ release_region(start, size);
+ continue;
+ }
+ }
+
+ /* set control and data register addresses */
+ fw_cfg_dev_ctrl = fw_cfg_dev_base + fw_cfg_mode->ctrl_offset;
+ fw_cfg_dev_data = fw_cfg_dev_base + fw_cfg_mode->data_offset;
+
+ /* verify fw_cfg device signature */
+ fw_cfg_read_blob(FW_CFG_SIGNATURE, sig, 0, FW_CFG_SIG_SIZE);
+ if (memcmp(sig, "QEMU", FW_CFG_SIG_SIZE) == 0)
+ /* success, we're done */
+ return 0;
+
+ /* clean up before probing next access mode */
+ fw_cfg_io_cleanup();
+ }
+
+ return -ENODEV;
+}
+
+/* fw_cfg revision attribute, placed in top-level /sys/fw_cfg directory */
+static uint32_t fw_cfg_rev;
+
+static ssize_t fw_cfg_showrev(struct kobject *k, struct attribute *a, char *buf)
+{
+ return sprintf(buf, "%u\n", fw_cfg_rev);
+}
+
+static const struct {
+ struct attribute attr;
+ ssize_t (*show)(struct kobject *k, struct attribute *a, char *buf);
+} fw_cfg_rev_attr = {
+ .attr = { .name = "rev", .mode = S_IRUSR },
+ .show = fw_cfg_showrev,
+};
+
+/* fw_cfg_sysfs_entry type */
+struct fw_cfg_sysfs_entry {
+ struct kobject kobj;
+ struct fw_cfg_file f;
+ struct list_head list;
+};
+
+/* get fw_cfg_sysfs_entry from kobject member */
+static inline struct fw_cfg_sysfs_entry *to_entry(struct kobject *kobj)
+{
+ return container_of(kobj, struct fw_cfg_sysfs_entry, kobj);
+}
+
+/* fw_cfg_sysfs_attribute type */
+struct fw_cfg_sysfs_attribute {
+ struct attribute attr;
+ ssize_t (*show)(struct fw_cfg_sysfs_entry *entry, char *buf);
+};
+
+/* get fw_cfg_sysfs_attribute from attribute member */
+static inline struct fw_cfg_sysfs_attribute *to_attr(struct attribute *attr)
+{
+ return container_of(attr, struct fw_cfg_sysfs_attribute, attr);
+}
+
+/* global cache of fw_cfg_sysfs_entry objects */
+static LIST_HEAD(fw_cfg_entry_cache);
+
+/* kobjects removed lazily by kernel, mutual exclusion needed */
+static DEFINE_SPINLOCK(fw_cfg_cache_lock);
+
+static inline void fw_cfg_sysfs_cache_enlist(struct fw_cfg_sysfs_entry *entry)
+{
+ spin_lock(&fw_cfg_cache_lock);
+ list_add_tail(&entry->list, &fw_cfg_entry_cache);
+ spin_unlock(&fw_cfg_cache_lock);
+}
+
+static inline void fw_cfg_sysfs_cache_delist(struct fw_cfg_sysfs_entry *entry)
+{
+ spin_lock(&fw_cfg_cache_lock);
+ list_del(&entry->list);
+ spin_unlock(&fw_cfg_cache_lock);
+}
+
+static void fw_cfg_sysfs_cache_cleanup(void)
+{
+ struct fw_cfg_sysfs_entry *entry, *next;
+
+ list_for_each_entry_safe(entry, next, &fw_cfg_entry_cache, list) {
+ /* will end up invoking fw_cfg_sysfs_cache_delist()
+ * via each object's release() method (i.e. destructor) */
+ kobject_put(&entry->kobj);
+ }
+}
+
+/* default_attrs: per-entry attributes and show methods */
+
+#define FW_CFG_SYSFS_ATTR(_attr) \
+struct fw_cfg_sysfs_attribute fw_cfg_sysfs_attr_##_attr = { \
+ .attr = { .name = __stringify(_attr), .mode = S_IRUSR }, \
+ .show = fw_cfg_sysfs_show_##_attr, \
+}
+
+static ssize_t fw_cfg_sysfs_show_size(struct fw_cfg_sysfs_entry *e, char *buf)
+{
+ return sprintf(buf, "%u\n", e->f.size);
+}
+
+static ssize_t fw_cfg_sysfs_show_select(struct fw_cfg_sysfs_entry *e, char *buf)
+{
+ return sprintf(buf, "%u\n", e->f.select);
+}
+
+static ssize_t fw_cfg_sysfs_show_name(struct fw_cfg_sysfs_entry *e, char *buf)
+{
+ return sprintf(buf, "%s\n", e->f.name);
+}
+
+static FW_CFG_SYSFS_ATTR(size);
+static FW_CFG_SYSFS_ATTR(select);
+static FW_CFG_SYSFS_ATTR(name);
+
+static struct attribute *fw_cfg_sysfs_entry_attrs[] = {
+ &fw_cfg_sysfs_attr_size.attr,
+ &fw_cfg_sysfs_attr_select.attr,
+ &fw_cfg_sysfs_attr_name.attr,
+ NULL,
+};
+
+/* sysfs_ops: find fw_cfg_[entry, attribute] and call appropriate show method */
+static ssize_t fw_cfg_sysfs_attr_show(struct kobject *kobj, struct attribute *a,
+ char *buf)
+{
+ struct fw_cfg_sysfs_entry *entry = to_entry(kobj);
+ struct fw_cfg_sysfs_attribute *attr = to_attr(a);
+
+ if (!capable(CAP_SYS_ADMIN))
+ return -EACCES;
+
+ return attr->show(entry, buf);
+}
+
+static const struct sysfs_ops fw_cfg_sysfs_attr_ops = {
+ .show = fw_cfg_sysfs_attr_show,
+};
+
+/* release: destructor, to be called via kobject_put() */
+static void fw_cfg_sysfs_release_entry(struct kobject *kobj)
+{
+ struct fw_cfg_sysfs_entry *entry = to_entry(kobj);
+
+ fw_cfg_sysfs_cache_delist(entry);
+ kfree(entry);
+}
+
+/* kobj_type: ties together all properties required to register an entry */
+static struct kobj_type fw_cfg_sysfs_entry_ktype = {
+ .default_attrs = fw_cfg_sysfs_entry_attrs,
+ .sysfs_ops = &fw_cfg_sysfs_attr_ops,
+ .release = fw_cfg_sysfs_release_entry,
+};
+
+/* raw-read method and attribute */
+static ssize_t fw_cfg_sysfs_read_raw(struct file *filp, struct kobject *kobj,
+ struct bin_attribute *bin_attr,
+ char *buf, loff_t pos, size_t count)
+{
+ struct fw_cfg_sysfs_entry *entry = to_entry(kobj);
+
+ if (!capable(CAP_SYS_ADMIN))
+ return -EACCES;
+
+ if (pos > entry->f.size)
+ return -EINVAL;
+
+ if (count > entry->f.size - pos)
+ count = entry->f.size - pos;
+
+ fw_cfg_read_blob(entry->f.select, buf, pos, count);
+ return count;
+}
+
+static struct bin_attribute fw_cfg_sysfs_attr_raw = {
+ .attr = { .name = "raw", .mode = 0400 },
+ .read = fw_cfg_sysfs_read_raw,
+};
+
+/* kobjects & kset representing top-level, by_select, and by_name folders */
+static struct kobject *fw_cfg_top_ko;
+static struct kobject *fw_cfg_sel_ko;
+
+/* callback function to register an individual fw_cfg file */
+static int __init fw_cfg_register_file(const struct fw_cfg_file *f)
+{
+ int err;
+ struct fw_cfg_sysfs_entry *entry;
+
+ /* allocate new entry */
+ entry = kzalloc(sizeof(*entry), GFP_KERNEL);
+ if (!entry)
+ return -ENOMEM;
+
+ /* set file entry information */
+ entry->f.size = be32_to_cpu(f->size);
+ entry->f.select = be16_to_cpu(f->select);
+ strcpy(entry->f.name, f->name);
+
+ /* register entry under "/sys/firmware/fw_cfg/by_select/" */
+ err = kobject_init_and_add(&entry->kobj, &fw_cfg_sysfs_entry_ktype,
+ fw_cfg_sel_ko, "%d", entry->f.select);
+ if (err)
+ goto err_register;
+
+ /* add raw binary content access */
+ err = sysfs_create_bin_file(&entry->kobj, &fw_cfg_sysfs_attr_raw);
+ if (err)
+ goto err_add_raw;
+
+ /* success, add entry to global cache */
+ fw_cfg_sysfs_cache_enlist(entry);
+ return 0;
+
+err_add_raw:
+ kobject_del(&entry->kobj);
+err_register:
+ kfree(entry);
+ return err;
+}
+
+/* unregister top-level or by_select folder */
+static inline void fw_cfg_kobj_cleanup(struct kobject *kobj)
+{
+ kobject_del(kobj);
+ kobject_put(kobj);
+}
+
+static int __init fw_cfg_sysfs_init(void)
+{
+ int err;
+
+ /* probe for the fw_cfg "hardware" */
+ err = fw_cfg_io_probe();
+ if (err)
+ return err;
+
+ /* create /sys/firmware/fwcfg/ and its subdirectories */
+ err = -ENOMEM;
+ fw_cfg_top_ko = kobject_create_and_add("fw_cfg", firmware_kobj);
+ if (!fw_cfg_top_ko)
+ goto err_top;
+ fw_cfg_sel_ko = kobject_create_and_add("by_select", fw_cfg_top_ko);
+ if (!fw_cfg_sel_ko)
+ goto err_sel;
+
+ /* get revision number, add matching top-level attribute */
+ fw_cfg_read_blob(FW_CFG_ID, &fw_cfg_rev, 0, sizeof(fw_cfg_rev));
+ fw_cfg_rev = le32_to_cpu(fw_cfg_rev);
+ err = sysfs_create_file(fw_cfg_top_ko, &fw_cfg_rev_attr.attr);
+ if (err)
+ goto err_rev;
+
+ /* process fw_cfg file directory entry, registering each file */
+ err = fw_cfg_scan_dir(fw_cfg_register_file);
+ if (err)
+ goto err_scan;
+
+ /* success */
+ pr_debug("fw_cfg: loaded.\n");
+ return 0;
+
+err_scan:
+ fw_cfg_sysfs_cache_cleanup();
+ sysfs_remove_file(fw_cfg_top_ko, &fw_cfg_rev_attr.attr);
+err_rev:
+ fw_cfg_kobj_cleanup(fw_cfg_sel_ko);
+err_sel:
+ fw_cfg_kobj_cleanup(fw_cfg_top_ko);
+err_top:
+ fw_cfg_io_cleanup();
+ return err;
+}
+
+static void __exit fw_cfg_sysfs_exit(void)
+{
+ pr_debug("fw_cfg: unloading.\n");
+ fw_cfg_sysfs_cache_cleanup();
+ fw_cfg_kobj_cleanup(fw_cfg_sel_ko);
+ fw_cfg_kobj_cleanup(fw_cfg_top_ko);
+ fw_cfg_io_cleanup();
+}
+
+module_init(fw_cfg_sysfs_init);
+module_exit(fw_cfg_sysfs_exit);
+MODULE_AUTHOR("Gabriel L. Somlo <somlo@cmu.edu>");
+MODULE_DESCRIPTION("QEMU fw_cfg sysfs support");
+MODULE_LICENSE("GPL");
--
2.4.3
^ permalink raw reply related
* [PATCH 2/3] kobject: export kset_find_obj() to be used from modules
From: Gabriel L. Somlo @ 2015-08-10 16:31 UTC (permalink / raw)
To: gregkh, ralf, zajec5, paul, galak, linux-api, linux-kernel
Cc: linux-efi, rjones, jordan.l.justen, x86, qemu-devel, gleb,
matt.fleming, kernelnewbies, kraxel, pbonzini, lersek
In-Reply-To: <cover.1439220623.git.somlo@cmu.edu>
From: "Gabriel Somlo" <somlo@cmu.edu>
Signed-off-by: Gabriel Somlo <somlo@cmu.edu>
---
lib/kobject.c | 1 +
1 file changed, 1 insertion(+)
diff --git a/lib/kobject.c b/lib/kobject.c
index 3e3a5c3..f9754a0 100644
--- a/lib/kobject.c
+++ b/lib/kobject.c
@@ -1058,3 +1058,4 @@ EXPORT_SYMBOL(kobject_del);
EXPORT_SYMBOL(kset_register);
EXPORT_SYMBOL(kset_unregister);
+EXPORT_SYMBOL(kset_find_obj);
--
2.4.3
^ permalink raw reply related
* [PATCH 3/3] firmware: fw_cfg: create directory hierarchy for fw_cfg file names
From: Gabriel L. Somlo @ 2015-08-10 16:31 UTC (permalink / raw)
To: gregkh, ralf, zajec5, paul, galak, linux-api, linux-kernel
Cc: linux-efi, rjones, jordan.l.justen, x86, qemu-devel, gleb,
matt.fleming, kernelnewbies, kraxel, pbonzini, lersek
In-Reply-To: <cover.1439220623.git.somlo@cmu.edu>
From: "Gabriel Somlo" <somlo@cmu.edu>
Each fw_cfg entry of type "file" has an associated 56-char,
nul-terminated ASCII string which represents its name. While
the fw_cfg device doesn't itself impose any specific naming
convention, QEMU developers have traditionally used path name
semantics (i.e. "etc/acpi/rsdp") to descriptively name the
various fw_cfg "blobs" passed into the guest.
This patch attempts, on a best effort basis, to create a
directory hierarchy representing the content of fw_cfg file
names, under /sys/firmware/fw_cfg/by_name.
Upon successful creation of all directories representing the
"dirname" portion of a fw_cfg file, a symlink will be created
to represent the "basename", pointing at the appropriate
/sys/firmware/fw_cfg/by_select entry. If a file name is not
suitable for this procedure (e.g., if its basename or dirname
components collide with an already existing dirname component
or basename, respectively) the corresponding fw_cfg blob is
skipped and will remain available in sysfs only by its selector
key value.
Signed-off-by: Gabriel Somlo <somlo@cmu.edu>
---
Documentation/ABI/testing/sysfs-firmware-fw_cfg | 42 ++++++++++
drivers/firmware/fw_cfg.c | 104 ++++++++++++++++++++++++
2 files changed, 146 insertions(+)
diff --git a/Documentation/ABI/testing/sysfs-firmware-fw_cfg b/Documentation/ABI/testing/sysfs-firmware-fw_cfg
index 3a7e7f2..1024776 100644
--- a/Documentation/ABI/testing/sysfs-firmware-fw_cfg
+++ b/Documentation/ABI/testing/sysfs-firmware-fw_cfg
@@ -167,3 +167,45 @@ Description:
entry via the control register, and reading a number
of bytes equal to the blob size from the data
register.
+
+ --- Listing fw_cfg blobs by file name ---
+
+ While the fw_cfg device does not impose any specific naming
+ convention on the blobs registered in the file directory,
+ QEMU developers have traditionally used path name semantics
+ to give each blob a descriptive name. For example:
+
+ "bootorder"
+ "genroms/kvmvapic.bin"
+ "etc/e820"
+ "etc/boot-fail-wait"
+ "etc/system-states"
+ "etc/table-loader"
+ "etc/acpi/rsdp"
+ "etc/acpi/tables"
+ "etc/smbios/smbios-tables"
+ "etc/smbios/smbios-anchor"
+ ...
+
+ In addition to the listing by unique selector key described
+ above, the fw_cfg sysfs driver also attempts to build a tree
+ of directories matching the path name components of fw_cfg
+ blob names, ending in symlinks to the by_select entry for each
+ "basename", as illustrated below (assume current directory is
+ /sys/firmware):
+
+ fw_cfg/by_name/bootorder -> ../by_select/38
+ fw_cfg/by_name/etc/e820 -> ../../by_select/35
+ fw_cfg/by_name/etc/acpi/rsdp -> ../../../by_select/41
+ ...
+
+ Construction of the directory tree and symlinks is done on a
+ "best-effort" basis, as there is no guarantee that components
+ of fw_cfg blob names are always "well behaved". I.e., there is
+ the possibility that a symlink (basename) will conflict with
+ a dirname component of another fw_cfg blob, in which case the
+ creation of the offending /sys/firmware/fw_cfg/by_name entry
+ will be skipped.
+
+ The authoritative list of entries will continue to be found
+ under the /sys/firmware/fw_cfg/by_select directory.
diff --git a/drivers/firmware/fw_cfg.c b/drivers/firmware/fw_cfg.c
index be17411..94233a5 100644
--- a/drivers/firmware/fw_cfg.c
+++ b/drivers/firmware/fw_cfg.c
@@ -327,9 +327,104 @@ static struct bin_attribute fw_cfg_sysfs_attr_raw = {
.read = fw_cfg_sysfs_read_raw,
};
+/*
+ * Create a kset subdirectory matching each '/' delimited dirname token
+ * in 'name', starting with sysfs kset/folder 'dir'; At the end, create
+ * a symlink directed at the given 'target'.
+ * NOTE: We do this on a best-effort basis, since 'name' is not guaranteed
+ * to be a well-behaved path name. Whenever a symlink vs. kset directory
+ * name collision occurs, the kernel will issue big scary warnings while
+ * refusing to add the offending link or directory. We follow up with our
+ * own, slightly less scary error messages explaining the situation :)
+ */
+static int __init fw_cfg_build_symlink(struct kset *dir,
+ struct kobject *target,
+ const char *name)
+{
+ int ret;
+ struct kset *subdir;
+ struct kobject *ko;
+ char *name_copy, *p, *tok;
+
+ if (!dir || !target || !name || !*name)
+ return -EINVAL;
+
+ /* clone a copy of name for parsing */
+ name_copy = p = kstrdup(name, GFP_KERNEL);
+ if (!name_copy)
+ return -ENOMEM;
+
+ /* create folders for each dirname token, then symlink for basename */
+ while ((tok = strsep(&p, "/")) && *tok) {
+
+ /* last (basename) token? If so, add symlink here */
+ if (!p || !*p) {
+ ret = sysfs_create_link(&dir->kobj, target, tok);
+ break;
+ }
+
+ /* does the current dir contain an item named after tok ? */
+ ko = kset_find_obj(dir, tok);
+ if (ko) {
+ /* drop reference added by kset_find_obj */
+ kobject_put(ko);
+
+ /* ko MUST be a kset - we're about to use it as one ! */
+ if (ko->ktype != dir->kobj.ktype) {
+ ret = -EINVAL;
+ break;
+ }
+
+ /* descend into already existing subdirectory */
+ dir = to_kset(ko);
+ } else {
+ /* create new subdirectory kset */
+ subdir = kzalloc(sizeof(struct kset), GFP_KERNEL);
+ if (!subdir) {
+ ret = -ENOMEM;
+ break;
+ }
+ subdir->kobj.kset = dir;
+ subdir->kobj.ktype = dir->kobj.ktype;
+ ret = kobject_set_name(&subdir->kobj, "%s", tok);
+ if (ret) {
+ kfree(subdir);
+ break;
+ }
+ ret = kset_register(subdir);
+ if (ret) {
+ kfree(subdir);
+ break;
+ }
+
+ /* descend into newly created subdirectory */
+ dir = subdir;
+ }
+ }
+
+ /* we're done with cloned copy of name */
+ kfree(name_copy);
+ return ret;
+}
+
+/* recursively unregister fw_cfg/by_name/ kset directory tree */
+static void fw_cfg_kset_unregister_recursive(struct kset *kset)
+{
+ struct kobject *k, *next;
+
+ list_for_each_entry_safe(k, next, &kset->list, entry)
+ /* all set members are ksets too, but check just in case... */
+ if (k->ktype == kset->kobj.ktype)
+ fw_cfg_kset_unregister_recursive(to_kset(k));
+
+ /* symlinks are cleanly and automatically removed with the directory */
+ kset_unregister(kset);
+}
+
/* kobjects & kset representing top-level, by_select, and by_name folders */
static struct kobject *fw_cfg_top_ko;
static struct kobject *fw_cfg_sel_ko;
+static struct kset *fw_cfg_fname_kset;
/* callback function to register an individual fw_cfg file */
static int __init fw_cfg_register_file(const struct fw_cfg_file *f)
@@ -358,6 +453,9 @@ static int __init fw_cfg_register_file(const struct fw_cfg_file *f)
if (err)
goto err_add_raw;
+ /* try adding "/sys/firmware/fw_cfg/by_name/" symlink (best-effort) */
+ fw_cfg_build_symlink(fw_cfg_fname_kset, &entry->kobj, entry->f.name);
+
/* success, add entry to global cache */
fw_cfg_sysfs_cache_enlist(entry);
return 0;
@@ -393,6 +491,9 @@ static int __init fw_cfg_sysfs_init(void)
fw_cfg_sel_ko = kobject_create_and_add("by_select", fw_cfg_top_ko);
if (!fw_cfg_sel_ko)
goto err_sel;
+ fw_cfg_fname_kset = kset_create_and_add("by_name", NULL, fw_cfg_top_ko);
+ if (!fw_cfg_fname_kset)
+ goto err_name;
/* get revision number, add matching top-level attribute */
fw_cfg_read_blob(FW_CFG_ID, &fw_cfg_rev, 0, sizeof(fw_cfg_rev));
@@ -414,6 +515,8 @@ err_scan:
fw_cfg_sysfs_cache_cleanup();
sysfs_remove_file(fw_cfg_top_ko, &fw_cfg_rev_attr.attr);
err_rev:
+ fw_cfg_kset_unregister_recursive(fw_cfg_fname_kset);
+err_name:
fw_cfg_kobj_cleanup(fw_cfg_sel_ko);
err_sel:
fw_cfg_kobj_cleanup(fw_cfg_top_ko);
@@ -426,6 +529,7 @@ static void __exit fw_cfg_sysfs_exit(void)
{
pr_debug("fw_cfg: unloading.\n");
fw_cfg_sysfs_cache_cleanup();
+ fw_cfg_kset_unregister_recursive(fw_cfg_fname_kset);
fw_cfg_kobj_cleanup(fw_cfg_sel_ko);
fw_cfg_kobj_cleanup(fw_cfg_top_ko);
fw_cfg_io_cleanup();
--
2.4.3
^ permalink raw reply related
* Re: [PATCH 1/3] firmware: introduce sysfs driver for QEMU's fw_cfg device
From: Greg KH @ 2015-08-10 18:30 UTC (permalink / raw)
To: Gabriel L. Somlo
Cc: ralf-6z/3iImG2C8G8FEW9MqTrA, zajec5-Re5JQEeQqe8AvxtiuMwx3w,
paul-DWxLp4Yu+b8AvxtiuMwx3w, galak-sgV2jX0FEOL9JmXXK+q4OQ,
linux-api-u79uwXL29TY76Z2rM5mHXA,
linux-kernel-u79uwXL29TY76Z2rM5mHXA,
matt.fleming-ral2JQCrhuEAvxtiuMwx3w, x86-DgEjT+Ai2ygdnm+yROfE0A,
linux-efi-u79uwXL29TY76Z2rM5mHXA,
qemu-devel-qX2TKyscuCcdnm+yROfE0A, lersek-H+wXaHxf7aLQT0dZR+AlfA,
jordan.l.justen-ral2JQCrhuEAvxtiuMwx3w,
gleb-RmZWMc9puTNJc61us3aD9laTQe2KTcn/,
pbonzini-H+wXaHxf7aLQT0dZR+AlfA, kraxel-H+wXaHxf7aLQT0dZR+AlfA,
eblake-H+wXaHxf7aLQT0dZR+AlfA, rjones-H+wXaHxf7aLQT0dZR+AlfA,
kernelnewbies-7JyXY6prKcjpASu1u0TL5ti2O/JbrIOy
In-Reply-To: <3b039351aba44759627fa77a0003dad685a9f21d.1439220623.git.somlo-D+Gtc/HYRWM@public.gmane.org>
On Mon, Aug 10, 2015 at 12:31:18PM -0400, Gabriel L. Somlo wrote:
> From: "Gabriel Somlo" <somlo-D+Gtc/HYRWM@public.gmane.org>
>
> Make fw_cfg entries of type "file" available via sysfs. Entries
> are listed under /sys/firmware/fw_cfg/by_select, in folders named
> after each entry's selector key. Filename, selector value, and
> size read-only attributes are included for each entry. Also, a
> "raw" attribute allows retrieval of the full binary content of
> each entry.
>
> This patch also provides a documentation file outlining the
> guest-side "hardware" interface exposed by the QEMU fw_cfg device.
>
> Signed-off-by: Gabriel Somlo <somlo-D+Gtc/HYRWM@public.gmane.org>
> ---
> Documentation/ABI/testing/sysfs-firmware-fw_cfg | 169 +++++++++
> drivers/firmware/Kconfig | 10 +
> drivers/firmware/Makefile | 1 +
> drivers/firmware/fw_cfg.c | 438 ++++++++++++++++++++++++
> 4 files changed, 618 insertions(+)
> create mode 100644 Documentation/ABI/testing/sysfs-firmware-fw_cfg
> create mode 100644 drivers/firmware/fw_cfg.c
>
> diff --git a/Documentation/ABI/testing/sysfs-firmware-fw_cfg b/Documentation/ABI/testing/sysfs-firmware-fw_cfg
> new file mode 100644
> index 0000000..3a7e7f2
> --- /dev/null
> +++ b/Documentation/ABI/testing/sysfs-firmware-fw_cfg
> @@ -0,0 +1,169 @@
> +What: /sys/firmware/fw_cfg/
/sys/firmware/qemu_fw/ ?
"fw_cfg" is very vague and not descriptive at all.
> +Date: August 2015
> +Contact: Gabriel Somlo <somlo-D+Gtc/HYRWM@public.gmane.org>
> +Description:
> + Several different architectures supported by QEMU (x86, arm,
> + sun4*, ppc/mac) are provisioned with a firmware configuration
> + (fw_cfg) device, used by the host to provide configuration data
> + to the starting guest. While most of this data is meant for use
> + by the guest BIOS, starting with QEMU v2.4, guest VMs may be
> + started with arbitrary fw_cfg entries supplied directly on the
> + command line, which therefore may be of interest to userspace.
> +
> + === Guest-side Hardware Interface ===
> +
> + The fw_cfg device is available to guest VMs as a pair (control
> + and data) of registers, accessible as either a IO ports or as
> + MMIO addresses, depending on the architecture.
> +
> + --- Control Register ---
> +
> + Width: 16-bit
> + Access: Write-Only
> + Endianness: LE (if IOport) or BE (if MMIO)
> +
> + A write to the control register selects the index for one of
> + the firmware configuration items (or "blobs") available on the
> + fw_cfg device, which can subsequently be read from the data
> + register.
> +
> + Each time the control register is written, an data offset
> + internal to the fw_cfg device will be set to zero. This data
> + offset impacts which portion of the selected fw_cfg blob is
> + accessed by reading the data register, as explained below.
> +
> + --- Data Register ---
> +
> + Width: 8-bit (if IOport), or 8/16/32/64-bit (if MMIO)
> + Access: Read-Only
> + Endianness: string preserving
> +
> + The data register allows access to an array of bytes which
> + represent the fw_cfg blob last selected by a write to the
> + control register.
> +
> + Immediately following a write to the control register, the data
> + offset will be set to zero. Each successful read access to the
> + data register will increment the data offset by the appropriate
> + access width.
> +
> + Each fw_cfg blob has a maximum associated data length. Once the
> + data offset exceeds this maximum length, any subsequent reads
> + via the data register will return 0x00.
> +
> + An N-byte wide read of the data register will return the next
> + available N bytes of the selected fw_cfg blob, as a substring,
> + in increasing address order, similar to memcpy(), zero-padded
> + if necessary should the maximum data length of the selected
> + item be reached, as described above.
> +
> + --- Per-arch Register Details ---
> +
> + -------------------------------------------------------------
> + arch access base ctrl ctrl data data
> + mode address offset endian offset width
> + max.
> + -------------------------------------------------------------
> + x86 IOport 0x510 0 LE 1 8
> + x86_64 IOport 0x510 0 LE 1 8
> + arm MMIO 0x9020000 8 BE 0 64
> + sun4u IOport 0x510 0 LE 1 8
> + sun4m MMIO 0xd00000510 0 BE 2 8
> + ppc/mac MMIO 0xf0000510 0 BE 2 8
> + -------------------------------------------------------------
> +
> + NOTE: On platforms where the fw_cfg registers are exposed as
> + IO ports, the data port number will always be one greater than
> + the port number of the control register. I.e., the two ports
> + are overlapping, and can not be mapped separately.
> +
> + === Firmware Configuration Items of Interest ===
> +
> + Originally, the index key, size, and formatting of blobs in
> + fw_cfg was hard coded by mutual agreement between QEMU on the
> + host side, and the BIOS running on the guest. Later on, a file
> + transfer interface was added: by reading a special blob, the
> + fw_cfg consumer can retrieve a list of records containing the
> + name, selector key, and size of further fw_cfg blobs made
> + available by the host. Below we describe three fw_cfg blobs
> + of interest to the sysfs driver.
> +
> + --- Signature (Key 0x0000, FW_CFG_SIGNATURE) ---
> +
> + The presence of the fw_cfg device can be verified by selecting
> + the signature blob by writing 0x0000 to the control register,
> + and reading four bytes from the data register. If the fw_cfg
> + device is present, the four bytes read will match the ASCII
> + characters "QEMU".
Why is this a binary sysfs file? It really sounds like you want a char
device, so you can do ioctl commands on it, right?
> +
> + --- Revision (Key 0x0001, FW_CFG_ID) ---
> +
> + A 32-bit little-endian unsigned integer, this item is used as
> + an interface revision number.
> +
> + --- File Directory (Key 0x0019, FW_CFG_FILE_DIR) ---
> +
> + Any fw_cfg blobs stored at key 0x0020 FW_CFG_FILE_FIRST() or
> + higher will have an associated entry in this "directory" blob,
> + which facilitates the discovery of available items by software
> + (e.g. BIOS) running on the guest. The format of the directory
> + blob is shown below.
> +
> + NOTE: All integers are stored in big-endian format!
> +
> + /* the entire file directory "blob" */
> + struct FWCfgFiles {
> + uint32_t count; /* total number of entries */
> + struct FWCfgFile f[]; /* entry array, see below */
> + };
> +
> + /* an individual directory entry, 64 bytes total */
> + struct FWCfgFile {
> + uint32_t size; /* size of referenced blob */
> + uint16_t select; /* blob selector key */
> + uint16_t reserved;
> + char name[56]; /* blob name, nul-term. ascii */
> + };
> +
> + === SysFS fw_cfg Interface ===
> +
> + The fw_cfg sysfs interface described in this document is only
> + intended to display discoverable blobs (i.e., those registered
> + with the file directory), as there is no way to determine the
> + presence or size of "legacy" blobs (with selector keys between
> + 0x0002 and 0x0018) programmatically.
> +
> + All fw_cfg information is shown under:
> +
> + /sys/firmware/fw_cfg/
> +
> + The only legacy blob displayed is the fw_cfg device revision:
> +
> + /sys/firmware/fw_cfg/rev
> +
> + --- Discoverable fw_cfg blobs by selector key ---
> +
> + All discoverable blobs listed in the fw_cfg file directory are
> + displayed as entries named after their unique selector key
> + value, e.g.:
> +
> + /sys/firmware/fw_cfg/by_select/32
> + /sys/firmware/fw_cfg/by_select/33
> + /sys/firmware/fw_cfg/by_select/34
> + ...
> +
> + Each such fw_cfg sysfs entry has the following values exported
> + as attributes:
> +
> + name : The 56-byte nul-terminated ASCII string used as the
> + blob's 'file name' in the fw_cfg directory.
> + size : The length of the blob, as given in the fw_cfg
> + directory.
> + select : The value of the blob's selector key as given in the
> + fw_cfg directory. This value is the same as used in
> + the parent directory name.
> + how the rest of the entry should be interpreted.
> + raw : The raw bytes of the blob, obtained by selecting the
> + entry via the control register, and reading a number
> + of bytes equal to the blob size from the data
> + register.
> diff --git a/drivers/firmware/Kconfig b/drivers/firmware/Kconfig
> index 99c69a3..f5cbe81 100644
> --- a/drivers/firmware/Kconfig
> +++ b/drivers/firmware/Kconfig
> @@ -136,6 +136,16 @@ config QCOM_SCM
> bool
> depends on ARM || ARM64
>
> +config FW_CFG_SYSFS
> + tristate "QEMU fw_cfg device support in sysfs"
> + depends on SYSFS
> + default n
> + help
> + Say Y or M here to enable the exporting of the QEMU firmware
> + configuration (fw_cfg) file entries via sysfs. Entries are
> + found under /sys/firmware/fw_cfg when this option is enabled
> + and loaded.
> +
> source "drivers/firmware/broadcom/Kconfig"
> source "drivers/firmware/google/Kconfig"
> source "drivers/firmware/efi/Kconfig"
> diff --git a/drivers/firmware/Makefile b/drivers/firmware/Makefile
> index 4a4b897..b81b46e 100644
> --- a/drivers/firmware/Makefile
> +++ b/drivers/firmware/Makefile
> @@ -13,6 +13,7 @@ obj-$(CONFIG_ISCSI_IBFT) += iscsi_ibft.o
> obj-$(CONFIG_FIRMWARE_MEMMAP) += memmap.o
> obj-$(CONFIG_QCOM_SCM) += qcom_scm.o
> obj-$(CONFIG_QCOM_SCM) += qcom_scm-32.o
> +obj-$(CONFIG_FW_CFG_SYSFS) += fw_cfg.o
> CFLAGS_qcom_scm-32.o :=$(call as-instr,.arch_extension sec,-DREQUIRES_SEC=1)
>
> obj-y += broadcom/
> diff --git a/drivers/firmware/fw_cfg.c b/drivers/firmware/fw_cfg.c
> new file mode 100644
> index 0000000..be17411
> --- /dev/null
> +++ b/drivers/firmware/fw_cfg.c
> @@ -0,0 +1,438 @@
> +/*
> + * drivers/firmware/fw_cfg.c
> + *
> + * Expose entries from QEMU's firmware configuration (fw_cfg) device in
> + * sysfs (read-only, under "/sys/firmware/fw_cfg/...").
> + *
> + * Copyright 2015 Carnegie Mellon University
> + */
> +
> +#include <linux/module.h>
> +#include <linux/capability.h>
> +#include <linux/slab.h>
> +#include <linux/io.h>
> +#include <linux/ioport.h>
> +#include <linux/ctype.h>
> +
> +/* selector values for "well-known" fw_cfg entries */
> +#define FW_CFG_SIGNATURE 0x00
> +#define FW_CFG_ID 0x01
> +#define FW_CFG_FILE_DIR 0x19
> +
> +/* size in bytes of fw_cfg signature */
> +#define FW_CFG_SIG_SIZE 4
> +
> +/* fw_cfg "file name" is up to 56 characters (including terminating nul) */
> +#define FW_CFG_MAX_FILE_PATH 56
> +
> +/* fw_cfg file directory entry type */
> +struct fw_cfg_file {
> + uint32_t size;
> + uint16_t select;
> + uint16_t reserved;
Those aren't valid kernel types, use u32, and u16 please.
> + char name[FW_CFG_MAX_FILE_PATH];
> +};
> +
> +/* fw_cfg device i/o access options type */
> +struct fw_cfg_access {
> + phys_addr_t start;
> + uint8_t size;
> + uint8_t ctrl_offset;
> + uint8_t data_offset;
u8.
> + bool is_mmio;
> + const char *name;
> +};
> +
> +/* fw_cfg device i/o access available options for known architectures */
> +static struct fw_cfg_access fw_cfg_modes[] = {
> + { 0x510, 2, 0, 1, false, "fw_cfg on i386, sun4u" },
> + { 0x9020000, 10, 8, 0, true, "fw_cfg on arm" },
> + { 0xd00000510, 3, 0, 2, true, "fw_cfg on sun4m" },
> + { 0xf0000510, 3, 0, 2, true, "fw_cfg on ppc/mac" },
named identifiers please.
> + { }
> +};
> +
> +/* fw_cfg device i/o currently selected option set */
> +static struct fw_cfg_access *fw_cfg_mode;
> +
> +/* fw_cfg device i/o register addresses */
> +static void __iomem *fw_cfg_dev_base;
> +static void __iomem *fw_cfg_dev_ctrl;
> +static void __iomem *fw_cfg_dev_data;
> +
> +/* atomic access to fw_cfg device (potentially slow i/o, so using mutex) */
> +static DEFINE_MUTEX(fw_cfg_dev_lock);
> +
> +/* pick apropriate endianness for selector key */
> +static inline uint16_t fw_cfg_sel_endianness(uint16_t select)
> +{
> + return fw_cfg_mode->is_mmio ? cpu_to_be16(select) : cpu_to_le16(select);
> +}
> +
> +/* type for fw_cfg "directory scan" visitor/callback function */
> +typedef int (*fw_cfg_file_callback)(const struct fw_cfg_file *f);
> +
> +/* run a given callback on each fw_cfg directory entry */
> +static int fw_cfg_scan_dir(fw_cfg_file_callback callback)
> +{
> + int ret = 0;
> + uint32_t count, i;
u32. Please remove all the *_t variable types. and i should be an int
here, right?
> + struct fw_cfg_file f;
> +
> + mutex_lock(&fw_cfg_dev_lock);
> + iowrite16(fw_cfg_sel_endianness(FW_CFG_FILE_DIR), fw_cfg_dev_ctrl);
> + ioread8_rep(fw_cfg_dev_data, &count, sizeof(count));
> + for (i = 0; i < be32_to_cpu(count); i++) {
> + ioread8_rep(fw_cfg_dev_data, &f, sizeof(f));
> + ret = callback(&f);
> + if (ret)
> + break;
> + }
> + mutex_unlock(&fw_cfg_dev_lock);
> + return ret;
> +}
> +
> +/* read chunk of given fw_cfg blob (caller responsible for sanity-check) */
> +static inline void fw_cfg_read_blob(uint16_t select,
> + void *buf, loff_t pos, size_t count)
> +{
> + mutex_lock(&fw_cfg_dev_lock);
> + iowrite16(fw_cfg_sel_endianness(select), fw_cfg_dev_ctrl);
> + while (pos-- > 0)
> + ioread8(fw_cfg_dev_data);
> + ioread8_rep(fw_cfg_dev_data, buf, count);
> + mutex_unlock(&fw_cfg_dev_lock);
> +}
> +
> +/* clean up fw_cfg device i/o setup */
> +static void fw_cfg_io_cleanup(void)
> +{
> + if (fw_cfg_mode->is_mmio) {
> + iounmap(fw_cfg_dev_base);
> + release_mem_region(fw_cfg_mode->start, fw_cfg_mode->size);
> + } else {
> + ioport_unmap(fw_cfg_dev_base);
> + release_region(fw_cfg_mode->start, fw_cfg_mode->size);
> + }
> +}
> +
> +/* probe and map fw_cfg device */
> +static int __init fw_cfg_io_probe(void)
> +{
> + char sig[FW_CFG_SIG_SIZE];
> +
> + for (fw_cfg_mode = &fw_cfg_modes[0];
> + fw_cfg_mode->start; fw_cfg_mode++) {
> +
> + phys_addr_t start = fw_cfg_mode->start;
> + uint8_t size = fw_cfg_mode->size;
> +
> + /* reserve and map mmio or ioport region */
> + if (fw_cfg_mode->is_mmio) {
> + if (!request_mem_region(start, size, fw_cfg_mode->name))
> + continue;
> + fw_cfg_dev_base = ioremap(start, size);
> + if (!fw_cfg_dev_base) {
> + release_mem_region(start, size);
> + continue;
> + }
> + } else {
> + if (!request_region(start, size, fw_cfg_mode->name))
> + continue;
> + fw_cfg_dev_base = ioport_map(start, size);
> + if (!fw_cfg_dev_base) {
> + release_region(start, size);
> + continue;
> + }
> + }
> +
> + /* set control and data register addresses */
> + fw_cfg_dev_ctrl = fw_cfg_dev_base + fw_cfg_mode->ctrl_offset;
> + fw_cfg_dev_data = fw_cfg_dev_base + fw_cfg_mode->data_offset;
> +
> + /* verify fw_cfg device signature */
> + fw_cfg_read_blob(FW_CFG_SIGNATURE, sig, 0, FW_CFG_SIG_SIZE);
> + if (memcmp(sig, "QEMU", FW_CFG_SIG_SIZE) == 0)
> + /* success, we're done */
> + return 0;
> +
> + /* clean up before probing next access mode */
> + fw_cfg_io_cleanup();
> + }
> +
> + return -ENODEV;
> +}
> +
> +/* fw_cfg revision attribute, placed in top-level /sys/fw_cfg directory */
> +static uint32_t fw_cfg_rev;
> +
> +static ssize_t fw_cfg_showrev(struct kobject *k, struct attribute *a, char *buf)
> +{
> + return sprintf(buf, "%u\n", fw_cfg_rev);
> +}
> +
> +static const struct {
> + struct attribute attr;
> + ssize_t (*show)(struct kobject *k, struct attribute *a, char *buf);
> +} fw_cfg_rev_attr = {
> + .attr = { .name = "rev", .mode = S_IRUSR },
> + .show = fw_cfg_showrev,
> +};
> +
> +/* fw_cfg_sysfs_entry type */
> +struct fw_cfg_sysfs_entry {
> + struct kobject kobj;
> + struct fw_cfg_file f;
> + struct list_head list;
> +};
> +
> +/* get fw_cfg_sysfs_entry from kobject member */
> +static inline struct fw_cfg_sysfs_entry *to_entry(struct kobject *kobj)
> +{
> + return container_of(kobj, struct fw_cfg_sysfs_entry, kobj);
> +}
> +
> +/* fw_cfg_sysfs_attribute type */
> +struct fw_cfg_sysfs_attribute {
> + struct attribute attr;
> + ssize_t (*show)(struct fw_cfg_sysfs_entry *entry, char *buf);
> +};
> +
> +/* get fw_cfg_sysfs_attribute from attribute member */
> +static inline struct fw_cfg_sysfs_attribute *to_attr(struct attribute *attr)
> +{
> + return container_of(attr, struct fw_cfg_sysfs_attribute, attr);
> +}
> +
> +/* global cache of fw_cfg_sysfs_entry objects */
> +static LIST_HEAD(fw_cfg_entry_cache);
> +
> +/* kobjects removed lazily by kernel, mutual exclusion needed */
> +static DEFINE_SPINLOCK(fw_cfg_cache_lock);
> +
> +static inline void fw_cfg_sysfs_cache_enlist(struct fw_cfg_sysfs_entry *entry)
> +{
> + spin_lock(&fw_cfg_cache_lock);
> + list_add_tail(&entry->list, &fw_cfg_entry_cache);
> + spin_unlock(&fw_cfg_cache_lock);
> +}
> +
> +static inline void fw_cfg_sysfs_cache_delist(struct fw_cfg_sysfs_entry *entry)
> +{
> + spin_lock(&fw_cfg_cache_lock);
> + list_del(&entry->list);
> + spin_unlock(&fw_cfg_cache_lock);
> +}
> +
> +static void fw_cfg_sysfs_cache_cleanup(void)
> +{
> + struct fw_cfg_sysfs_entry *entry, *next;
> +
> + list_for_each_entry_safe(entry, next, &fw_cfg_entry_cache, list) {
> + /* will end up invoking fw_cfg_sysfs_cache_delist()
> + * via each object's release() method (i.e. destructor) */
> + kobject_put(&entry->kobj);
> + }
> +}
> +
> +/* default_attrs: per-entry attributes and show methods */
> +
> +#define FW_CFG_SYSFS_ATTR(_attr) \
> +struct fw_cfg_sysfs_attribute fw_cfg_sysfs_attr_##_attr = { \
> + .attr = { .name = __stringify(_attr), .mode = S_IRUSR }, \
> + .show = fw_cfg_sysfs_show_##_attr, \
> +}
> +
> +static ssize_t fw_cfg_sysfs_show_size(struct fw_cfg_sysfs_entry *e, char *buf)
> +{
> + return sprintf(buf, "%u\n", e->f.size);
> +}
> +
> +static ssize_t fw_cfg_sysfs_show_select(struct fw_cfg_sysfs_entry *e, char *buf)
> +{
> + return sprintf(buf, "%u\n", e->f.select);
> +}
> +
> +static ssize_t fw_cfg_sysfs_show_name(struct fw_cfg_sysfs_entry *e, char *buf)
> +{
> + return sprintf(buf, "%s\n", e->f.name);
> +}
> +
> +static FW_CFG_SYSFS_ATTR(size);
> +static FW_CFG_SYSFS_ATTR(select);
> +static FW_CFG_SYSFS_ATTR(name);
> +
> +static struct attribute *fw_cfg_sysfs_entry_attrs[] = {
> + &fw_cfg_sysfs_attr_size.attr,
> + &fw_cfg_sysfs_attr_select.attr,
> + &fw_cfg_sysfs_attr_name.attr,
> + NULL,
> +};
> +
> +/* sysfs_ops: find fw_cfg_[entry, attribute] and call appropriate show method */
> +static ssize_t fw_cfg_sysfs_attr_show(struct kobject *kobj, struct attribute *a,
> + char *buf)
> +{
> + struct fw_cfg_sysfs_entry *entry = to_entry(kobj);
> + struct fw_cfg_sysfs_attribute *attr = to_attr(a);
> +
> + if (!capable(CAP_SYS_ADMIN))
> + return -EACCES;
Shouldn't the file permissions handle this properly for you?
> +
> + return attr->show(entry, buf);
> +}
> +
> +static const struct sysfs_ops fw_cfg_sysfs_attr_ops = {
> + .show = fw_cfg_sysfs_attr_show,
> +};
> +
> +/* release: destructor, to be called via kobject_put() */
> +static void fw_cfg_sysfs_release_entry(struct kobject *kobj)
> +{
> + struct fw_cfg_sysfs_entry *entry = to_entry(kobj);
> +
> + fw_cfg_sysfs_cache_delist(entry);
> + kfree(entry);
> +}
> +
> +/* kobj_type: ties together all properties required to register an entry */
> +static struct kobj_type fw_cfg_sysfs_entry_ktype = {
> + .default_attrs = fw_cfg_sysfs_entry_attrs,
> + .sysfs_ops = &fw_cfg_sysfs_attr_ops,
> + .release = fw_cfg_sysfs_release_entry,
> +};
> +
> +/* raw-read method and attribute */
> +static ssize_t fw_cfg_sysfs_read_raw(struct file *filp, struct kobject *kobj,
> + struct bin_attribute *bin_attr,
> + char *buf, loff_t pos, size_t count)
> +{
> + struct fw_cfg_sysfs_entry *entry = to_entry(kobj);
> +
> + if (!capable(CAP_SYS_ADMIN))
> + return -EACCES;
Again, file permissions?
> +
> + if (pos > entry->f.size)
> + return -EINVAL;
> +
> + if (count > entry->f.size - pos)
> + count = entry->f.size - pos;
> +
> + fw_cfg_read_blob(entry->f.select, buf, pos, count);
> + return count;
> +}
> +
> +static struct bin_attribute fw_cfg_sysfs_attr_raw = {
> + .attr = { .name = "raw", .mode = 0400 },
> + .read = fw_cfg_sysfs_read_raw,
> +};
> +
> +/* kobjects & kset representing top-level, by_select, and by_name folders */
> +static struct kobject *fw_cfg_top_ko;
> +static struct kobject *fw_cfg_sel_ko;
> +
> +/* callback function to register an individual fw_cfg file */
> +static int __init fw_cfg_register_file(const struct fw_cfg_file *f)
> +{
> + int err;
> + struct fw_cfg_sysfs_entry *entry;
> +
> + /* allocate new entry */
> + entry = kzalloc(sizeof(*entry), GFP_KERNEL);
> + if (!entry)
> + return -ENOMEM;
> +
> + /* set file entry information */
> + entry->f.size = be32_to_cpu(f->size);
> + entry->f.select = be16_to_cpu(f->select);
> + strcpy(entry->f.name, f->name);
> +
> + /* register entry under "/sys/firmware/fw_cfg/by_select/" */
> + err = kobject_init_and_add(&entry->kobj, &fw_cfg_sysfs_entry_ktype,
> + fw_cfg_sel_ko, "%d", entry->f.select);
> + if (err)
> + goto err_register;
> +
> + /* add raw binary content access */
> + err = sysfs_create_bin_file(&entry->kobj, &fw_cfg_sysfs_attr_raw);
> + if (err)
> + goto err_add_raw;
> +
> + /* success, add entry to global cache */
> + fw_cfg_sysfs_cache_enlist(entry);
> + return 0;
> +
> +err_add_raw:
> + kobject_del(&entry->kobj);
> +err_register:
> + kfree(entry);
> + return err;
> +}
> +
> +/* unregister top-level or by_select folder */
> +static inline void fw_cfg_kobj_cleanup(struct kobject *kobj)
> +{
> + kobject_del(kobj);
> + kobject_put(kobj);
> +}
> +
> +static int __init fw_cfg_sysfs_init(void)
> +{
> + int err;
> +
> + /* probe for the fw_cfg "hardware" */
> + err = fw_cfg_io_probe();
> + if (err)
> + return err;
> +
> + /* create /sys/firmware/fwcfg/ and its subdirectories */
> + err = -ENOMEM;
> + fw_cfg_top_ko = kobject_create_and_add("fw_cfg", firmware_kobj);
> + if (!fw_cfg_top_ko)
> + goto err_top;
> + fw_cfg_sel_ko = kobject_create_and_add("by_select", fw_cfg_top_ko);
> + if (!fw_cfg_sel_ko)
> + goto err_sel;
> +
> + /* get revision number, add matching top-level attribute */
> + fw_cfg_read_blob(FW_CFG_ID, &fw_cfg_rev, 0, sizeof(fw_cfg_rev));
> + fw_cfg_rev = le32_to_cpu(fw_cfg_rev);
> + err = sysfs_create_file(fw_cfg_top_ko, &fw_cfg_rev_attr.attr);
> + if (err)
> + goto err_rev;
> +
> + /* process fw_cfg file directory entry, registering each file */
> + err = fw_cfg_scan_dir(fw_cfg_register_file);
Dealing with all of these "raw" kobjecs makes me nervous. Why can't you
use 'struct device' for this instead, on the system bus?
thanks,
greg k-h
^ permalink raw reply
* Re: [PATCH 3/3] firmware: fw_cfg: create directory hierarchy for fw_cfg file names
From: Greg KH @ 2015-08-10 18:32 UTC (permalink / raw)
To: Gabriel L. Somlo
Cc: ralf-6z/3iImG2C8G8FEW9MqTrA, zajec5-Re5JQEeQqe8AvxtiuMwx3w,
paul-DWxLp4Yu+b8AvxtiuMwx3w, galak-sgV2jX0FEOL9JmXXK+q4OQ,
linux-api-u79uwXL29TY76Z2rM5mHXA,
linux-kernel-u79uwXL29TY76Z2rM5mHXA,
matt.fleming-ral2JQCrhuEAvxtiuMwx3w, x86-DgEjT+Ai2ygdnm+yROfE0A,
linux-efi-u79uwXL29TY76Z2rM5mHXA,
qemu-devel-qX2TKyscuCcdnm+yROfE0A, lersek-H+wXaHxf7aLQT0dZR+AlfA,
jordan.l.justen-ral2JQCrhuEAvxtiuMwx3w,
gleb-RmZWMc9puTNJc61us3aD9laTQe2KTcn/,
pbonzini-H+wXaHxf7aLQT0dZR+AlfA, kraxel-H+wXaHxf7aLQT0dZR+AlfA,
eblake-H+wXaHxf7aLQT0dZR+AlfA, rjones-H+wXaHxf7aLQT0dZR+AlfA,
kernelnewbies-7JyXY6prKcjpASu1u0TL5ti2O/JbrIOy
In-Reply-To: <1cadfcda3c31d7470f677656f9c914e9c4b7c0cd.1439220623.git.somlo-D+Gtc/HYRWM@public.gmane.org>
On Mon, Aug 10, 2015 at 12:31:20PM -0400, Gabriel L. Somlo wrote:
> From: "Gabriel Somlo" <somlo-D+Gtc/HYRWM@public.gmane.org>
>
> Each fw_cfg entry of type "file" has an associated 56-char,
> nul-terminated ASCII string which represents its name. While
> the fw_cfg device doesn't itself impose any specific naming
> convention, QEMU developers have traditionally used path name
> semantics (i.e. "etc/acpi/rsdp") to descriptively name the
> various fw_cfg "blobs" passed into the guest.
>
> This patch attempts, on a best effort basis, to create a
> directory hierarchy representing the content of fw_cfg file
> names, under /sys/firmware/fw_cfg/by_name.
>
> Upon successful creation of all directories representing the
> "dirname" portion of a fw_cfg file, a symlink will be created
> to represent the "basename", pointing at the appropriate
> /sys/firmware/fw_cfg/by_select entry. If a file name is not
> suitable for this procedure (e.g., if its basename or dirname
> components collide with an already existing dirname component
> or basename, respectively) the corresponding fw_cfg blob is
> skipped and will remain available in sysfs only by its selector
> key value.
Shouldn't all of this be done in userspace with the symlinks and all?
It seems like you are trying to duplicate the /dev/block/by-name and
such. Policy decisions like symlinks and naming should be done there,
in userspace, and not directly in sysfs if at all possible.
Again, why can't this be a bunch of character device nodes? It seems
like you want to access them that way (read/write, ioctl, etc.)
thanks,
greg k-h
^ permalink raw reply
* Re: [PATCH 2/3] kobject: export kset_find_obj() to be used from modules
From: Greg KH @ 2015-08-10 18:33 UTC (permalink / raw)
To: Gabriel L. Somlo
Cc: ralf, zajec5, paul, galak, linux-api, linux-kernel, matt.fleming,
x86, linux-efi, qemu-devel, lersek, jordan.l.justen, gleb,
pbonzini, kraxel, eblake, rjones, kernelnewbies
In-Reply-To: <5c2bafb073e1386e39c4f73a83e7d835547b6b1f.1439220623.git.somlo@cmu.edu>
On Mon, Aug 10, 2015 at 12:31:19PM -0400, Gabriel L. Somlo wrote:
> From: "Gabriel Somlo" <somlo@cmu.edu>
>
> Signed-off-by: Gabriel Somlo <somlo@cmu.edu>
> ---
> lib/kobject.c | 1 +
> 1 file changed, 1 insertion(+)
>
> diff --git a/lib/kobject.c b/lib/kobject.c
> index 3e3a5c3..f9754a0 100644
> --- a/lib/kobject.c
> +++ b/lib/kobject.c
> @@ -1058,3 +1058,4 @@ EXPORT_SYMBOL(kobject_del);
>
> EXPORT_SYMBOL(kset_register);
> EXPORT_SYMBOL(kset_unregister);
> +EXPORT_SYMBOL(kset_find_obj);
checkpatch doesn't like this for obvious reasons :)
^ permalink raw reply
* Re: [PATCH 2/3] kobject: export kset_find_obj() to be used from modules
From: Gabriel L. Somlo @ 2015-08-10 18:43 UTC (permalink / raw)
To: Greg KH
Cc: x86, paul, matt.fleming, gleb, jordan.l.justen, rjones, linux-api,
zajec5, linux-kernel, qemu-devel, linux-efi, kernelnewbies,
kraxel, galak, pbonzini, lersek
In-Reply-To: <20150810183304.GC18817@kroah.com>
On Mon, Aug 10, 2015 at 11:33:04AM -0700, Greg KH wrote:
> On Mon, Aug 10, 2015 at 12:31:19PM -0400, Gabriel L. Somlo wrote:
> > From: "Gabriel Somlo" <somlo@cmu.edu>
> >
> > Signed-off-by: Gabriel Somlo <somlo@cmu.edu>
> > ---
> > lib/kobject.c | 1 +
> > 1 file changed, 1 insertion(+)
> >
> > diff --git a/lib/kobject.c b/lib/kobject.c
> > index 3e3a5c3..f9754a0 100644
> > --- a/lib/kobject.c
> > +++ b/lib/kobject.c
> > @@ -1058,3 +1058,4 @@ EXPORT_SYMBOL(kobject_del);
> >
> > EXPORT_SYMBOL(kset_register);
> > EXPORT_SYMBOL(kset_unregister);
> > +EXPORT_SYMBOL(kset_find_obj);
>
> checkpatch doesn't like this for obvious reasons :)
When I run checkpatch on this one, it comes back fine. Could you
please elaborate ?
[staging]$ scripts/checkpatch.pl
0002-kobject-export-kset_find_obj-to-be-used-from-modules.patch
total: 0 errors, 0 warnings, 4 lines checked
0002-kobject-export-kset_find_obj-to-be-used-from-modules.patch has no
obvious style problems and is ready for submission.
Thanks much,
--Gabriel
^ permalink raw reply
* Re: [PATCH 2/3] kobject: export kset_find_obj() to be used from modules
From: Greg KH @ 2015-08-10 18:54 UTC (permalink / raw)
To: Gabriel L. Somlo
Cc: ralf-6z/3iImG2C8G8FEW9MqTrA, zajec5-Re5JQEeQqe8AvxtiuMwx3w,
paul-DWxLp4Yu+b8AvxtiuMwx3w, galak-sgV2jX0FEOL9JmXXK+q4OQ,
linux-api-u79uwXL29TY76Z2rM5mHXA,
linux-kernel-u79uwXL29TY76Z2rM5mHXA,
matt.fleming-ral2JQCrhuEAvxtiuMwx3w, x86-DgEjT+Ai2ygdnm+yROfE0A,
linux-efi-u79uwXL29TY76Z2rM5mHXA,
qemu-devel-qX2TKyscuCcdnm+yROfE0A, lersek-H+wXaHxf7aLQT0dZR+AlfA,
jordan.l.justen-ral2JQCrhuEAvxtiuMwx3w,
gleb-RmZWMc9puTNJc61us3aD9laTQe2KTcn/,
pbonzini-H+wXaHxf7aLQT0dZR+AlfA, kraxel-H+wXaHxf7aLQT0dZR+AlfA,
eblake-H+wXaHxf7aLQT0dZR+AlfA, rjones-H+wXaHxf7aLQT0dZR+AlfA,
kernelnewbies-7JyXY6prKcjpASu1u0TL5ti2O/JbrIOy
In-Reply-To: <20150810184310.GM31816-h65ZQ0r4j6KKUezXOiBB2eW1CriLhL8O@public.gmane.org>
On Mon, Aug 10, 2015 at 02:43:10PM -0400, Gabriel L. Somlo wrote:
> On Mon, Aug 10, 2015 at 11:33:04AM -0700, Greg KH wrote:
> > On Mon, Aug 10, 2015 at 12:31:19PM -0400, Gabriel L. Somlo wrote:
> > > From: "Gabriel Somlo" <somlo-D+Gtc/HYRWM@public.gmane.org>
> > >
> > > Signed-off-by: Gabriel Somlo <somlo-D+Gtc/HYRWM@public.gmane.org>
> > > ---
> > > lib/kobject.c | 1 +
> > > 1 file changed, 1 insertion(+)
> > >
> > > diff --git a/lib/kobject.c b/lib/kobject.c
> > > index 3e3a5c3..f9754a0 100644
> > > --- a/lib/kobject.c
> > > +++ b/lib/kobject.c
> > > @@ -1058,3 +1058,4 @@ EXPORT_SYMBOL(kobject_del);
> > >
> > > EXPORT_SYMBOL(kset_register);
> > > EXPORT_SYMBOL(kset_unregister);
> > > +EXPORT_SYMBOL(kset_find_obj);
> >
> > checkpatch doesn't like this for obvious reasons :)
>
> When I run checkpatch on this one, it comes back fine. Could you
> please elaborate ?
>
> [staging]$ scripts/checkpatch.pl
> 0002-kobject-export-kset_find_obj-to-be-used-from-modules.patch
> total: 0 errors, 0 warnings, 4 lines checked
>
> 0002-kobject-export-kset_find_obj-to-be-used-from-modules.patch has no
> obvious style problems and is ready for submission.
EXPORT_SYMBOL() should be below the function definition itself, not at
the bottom of the file. Odd that checkpatch doesn't catch this when
done in this manner, it will if you run it on the whole file after your
patch is applied (along with the existing problems.)
But this is a minor issue, see my other review first...
greg k-h
^ permalink raw reply
* Re: [PATCH 1/3] firmware: introduce sysfs driver for QEMU's fw_cfg device
From: Gabriel L. Somlo @ 2015-08-10 19:02 UTC (permalink / raw)
To: Greg KH
Cc: ralf-6z/3iImG2C8G8FEW9MqTrA, zajec5-Re5JQEeQqe8AvxtiuMwx3w,
paul-DWxLp4Yu+b8AvxtiuMwx3w, galak-sgV2jX0FEOL9JmXXK+q4OQ,
linux-api-u79uwXL29TY76Z2rM5mHXA,
linux-kernel-u79uwXL29TY76Z2rM5mHXA,
matt.fleming-ral2JQCrhuEAvxtiuMwx3w, x86-DgEjT+Ai2ygdnm+yROfE0A,
linux-efi-u79uwXL29TY76Z2rM5mHXA,
qemu-devel-qX2TKyscuCcdnm+yROfE0A, lersek-H+wXaHxf7aLQT0dZR+AlfA,
jordan.l.justen-ral2JQCrhuEAvxtiuMwx3w,
gleb-RmZWMc9puTNJc61us3aD9laTQe2KTcn/,
pbonzini-H+wXaHxf7aLQT0dZR+AlfA, kraxel-H+wXaHxf7aLQT0dZR+AlfA,
eblake-H+wXaHxf7aLQT0dZR+AlfA, rjones-H+wXaHxf7aLQT0dZR+AlfA,
kernelnewbies-7JyXY6prKcjpASu1u0TL5ti2O/JbrIOy
In-Reply-To: <20150810183054.GA18817-U8xfFu+wG4EAvxtiuMwx3w@public.gmane.org>
On Mon, Aug 10, 2015 at 11:30:54AM -0700, Greg KH wrote:
> On Mon, Aug 10, 2015 at 12:31:18PM -0400, Gabriel L. Somlo wrote:
> > From: "Gabriel Somlo" <somlo-D+Gtc/HYRWM@public.gmane.org>
> >
> > Make fw_cfg entries of type "file" available via sysfs. Entries
> > are listed under /sys/firmware/fw_cfg/by_select, in folders named
> > after each entry's selector key. Filename, selector value, and
> > size read-only attributes are included for each entry. Also, a
> > "raw" attribute allows retrieval of the full binary content of
> > each entry.
> >
> > This patch also provides a documentation file outlining the
> > guest-side "hardware" interface exposed by the QEMU fw_cfg device.
> >
> > Signed-off-by: Gabriel Somlo <somlo-D+Gtc/HYRWM@public.gmane.org>
> > ---
> > Documentation/ABI/testing/sysfs-firmware-fw_cfg | 169 +++++++++
> > drivers/firmware/Kconfig | 10 +
> > drivers/firmware/Makefile | 1 +
> > drivers/firmware/fw_cfg.c | 438 ++++++++++++++++++++++++
> > 4 files changed, 618 insertions(+)
> > create mode 100644 Documentation/ABI/testing/sysfs-firmware-fw_cfg
> > create mode 100644 drivers/firmware/fw_cfg.c
> >
> > diff --git a/Documentation/ABI/testing/sysfs-firmware-fw_cfg b/Documentation/ABI/testing/sysfs-firmware-fw_cfg
> > new file mode 100644
> > index 0000000..3a7e7f2
> > --- /dev/null
> > +++ b/Documentation/ABI/testing/sysfs-firmware-fw_cfg
> > @@ -0,0 +1,169 @@
> > +What: /sys/firmware/fw_cfg/
>
> /sys/firmware/qemu_fw/ ?
>
> "fw_cfg" is very vague and not descriptive at all.
How about /sys/firmware/qemu_fw_cfg/, then ? In the QEMU universe,
"fw_cfg" is familiar. OTOH "qemu_fw" would not immediately ring a bell...
>
>
> > +Date: August 2015
> > +Contact: Gabriel Somlo <somlo-D+Gtc/HYRWM@public.gmane.org>
> > +Description:
> > + Several different architectures supported by QEMU (x86, arm,
> > + sun4*, ppc/mac) are provisioned with a firmware configuration
> > + (fw_cfg) device, used by the host to provide configuration data
> > + to the starting guest. While most of this data is meant for use
> > + by the guest BIOS, starting with QEMU v2.4, guest VMs may be
> > + started with arbitrary fw_cfg entries supplied directly on the
> > + command line, which therefore may be of interest to userspace.
> > +
> > + === Guest-side Hardware Interface ===
> > +
> > + The fw_cfg device is available to guest VMs as a pair (control
> > + and data) of registers, accessible as either a IO ports or as
> > + MMIO addresses, depending on the architecture.
> > +
> > + --- Control Register ---
> > +
> > + Width: 16-bit
> > + Access: Write-Only
> > + Endianness: LE (if IOport) or BE (if MMIO)
> > +
> > + A write to the control register selects the index for one of
> > + the firmware configuration items (or "blobs") available on the
> > + fw_cfg device, which can subsequently be read from the data
> > + register.
> > +
> > + Each time the control register is written, an data offset
> > + internal to the fw_cfg device will be set to zero. This data
> > + offset impacts which portion of the selected fw_cfg blob is
> > + accessed by reading the data register, as explained below.
> > +
> > + --- Data Register ---
> > +
> > + Width: 8-bit (if IOport), or 8/16/32/64-bit (if MMIO)
> > + Access: Read-Only
> > + Endianness: string preserving
> > +
> > + The data register allows access to an array of bytes which
> > + represent the fw_cfg blob last selected by a write to the
> > + control register.
> > +
> > + Immediately following a write to the control register, the data
> > + offset will be set to zero. Each successful read access to the
> > + data register will increment the data offset by the appropriate
> > + access width.
> > +
> > + Each fw_cfg blob has a maximum associated data length. Once the
> > + data offset exceeds this maximum length, any subsequent reads
> > + via the data register will return 0x00.
> > +
> > + An N-byte wide read of the data register will return the next
> > + available N bytes of the selected fw_cfg blob, as a substring,
> > + in increasing address order, similar to memcpy(), zero-padded
> > + if necessary should the maximum data length of the selected
> > + item be reached, as described above.
> > +
> > + --- Per-arch Register Details ---
> > +
> > + -------------------------------------------------------------
> > + arch access base ctrl ctrl data data
> > + mode address offset endian offset width
> > + max.
> > + -------------------------------------------------------------
> > + x86 IOport 0x510 0 LE 1 8
> > + x86_64 IOport 0x510 0 LE 1 8
> > + arm MMIO 0x9020000 8 BE 0 64
> > + sun4u IOport 0x510 0 LE 1 8
> > + sun4m MMIO 0xd00000510 0 BE 2 8
> > + ppc/mac MMIO 0xf0000510 0 BE 2 8
> > + -------------------------------------------------------------
> > +
> > + NOTE: On platforms where the fw_cfg registers are exposed as
> > + IO ports, the data port number will always be one greater than
> > + the port number of the control register. I.e., the two ports
> > + are overlapping, and can not be mapped separately.
> > +
> > + === Firmware Configuration Items of Interest ===
> > +
> > + Originally, the index key, size, and formatting of blobs in
> > + fw_cfg was hard coded by mutual agreement between QEMU on the
> > + host side, and the BIOS running on the guest. Later on, a file
> > + transfer interface was added: by reading a special blob, the
> > + fw_cfg consumer can retrieve a list of records containing the
> > + name, selector key, and size of further fw_cfg blobs made
> > + available by the host. Below we describe three fw_cfg blobs
> > + of interest to the sysfs driver.
> > +
> > + --- Signature (Key 0x0000, FW_CFG_SIGNATURE) ---
> > +
> > + The presence of the fw_cfg device can be verified by selecting
> > + the signature blob by writing 0x0000 to the control register,
> > + and reading four bytes from the data register. If the fw_cfg
> > + device is present, the four bytes read will match the ASCII
> > + characters "QEMU".
>
> Why is this a binary sysfs file? It really sounds like you want a char
> device, so you can do ioctl commands on it, right?
I felt it necessary to explain the most important fw_cfg blobs before
getting into the sysfs layout proper. This blob isn't displayed in
sysfs, just used by the module_init function to confirm we have, in
fact, a valid fw_cfg device present on the system.
>
>
> > +
> > + --- Revision (Key 0x0001, FW_CFG_ID) ---
> > +
> > + A 32-bit little-endian unsigned integer, this item is used as
> > + an interface revision number.
> > +
> > + --- File Directory (Key 0x0019, FW_CFG_FILE_DIR) ---
> > +
> > + Any fw_cfg blobs stored at key 0x0020 FW_CFG_FILE_FIRST() or
> > + higher will have an associated entry in this "directory" blob,
> > + which facilitates the discovery of available items by software
> > + (e.g. BIOS) running on the guest. The format of the directory
> > + blob is shown below.
> > +
> > + NOTE: All integers are stored in big-endian format!
> > +
> > + /* the entire file directory "blob" */
> > + struct FWCfgFiles {
> > + uint32_t count; /* total number of entries */
> > + struct FWCfgFile f[]; /* entry array, see below */
> > + };
> > +
> > + /* an individual directory entry, 64 bytes total */
> > + struct FWCfgFile {
> > + uint32_t size; /* size of referenced blob */
> > + uint16_t select; /* blob selector key */
> > + uint16_t reserved;
> > + char name[56]; /* blob name, nul-term. ascii */
> > + };
> > +
> > + === SysFS fw_cfg Interface ===
> > +
> > + The fw_cfg sysfs interface described in this document is only
> > + intended to display discoverable blobs (i.e., those registered
> > + with the file directory), as there is no way to determine the
> > + presence or size of "legacy" blobs (with selector keys between
> > + 0x0002 and 0x0018) programmatically.
> > +
> > + All fw_cfg information is shown under:
> > +
> > + /sys/firmware/fw_cfg/
> > +
> > + The only legacy blob displayed is the fw_cfg device revision:
> > +
> > + /sys/firmware/fw_cfg/rev
> > +
> > + --- Discoverable fw_cfg blobs by selector key ---
> > +
> > + All discoverable blobs listed in the fw_cfg file directory are
> > + displayed as entries named after their unique selector key
> > + value, e.g.:
> > +
> > + /sys/firmware/fw_cfg/by_select/32
> > + /sys/firmware/fw_cfg/by_select/33
> > + /sys/firmware/fw_cfg/by_select/34
> > + ...
> > +
> > + Each such fw_cfg sysfs entry has the following values exported
> > + as attributes:
> > +
> > + name : The 56-byte nul-terminated ASCII string used as the
> > + blob's 'file name' in the fw_cfg directory.
> > + size : The length of the blob, as given in the fw_cfg
> > + directory.
> > + select : The value of the blob's selector key as given in the
> > + fw_cfg directory. This value is the same as used in
> > + the parent directory name.
> > + how the rest of the entry should be interpreted.
> > + raw : The raw bytes of the blob, obtained by selecting the
> > + entry via the control register, and reading a number
> > + of bytes equal to the blob size from the data
> > + register.
> > diff --git a/drivers/firmware/Kconfig b/drivers/firmware/Kconfig
> > index 99c69a3..f5cbe81 100644
> > --- a/drivers/firmware/Kconfig
> > +++ b/drivers/firmware/Kconfig
> > @@ -136,6 +136,16 @@ config QCOM_SCM
> > bool
> > depends on ARM || ARM64
> >
> > +config FW_CFG_SYSFS
> > + tristate "QEMU fw_cfg device support in sysfs"
> > + depends on SYSFS
> > + default n
> > + help
> > + Say Y or M here to enable the exporting of the QEMU firmware
> > + configuration (fw_cfg) file entries via sysfs. Entries are
> > + found under /sys/firmware/fw_cfg when this option is enabled
> > + and loaded.
> > +
> > source "drivers/firmware/broadcom/Kconfig"
> > source "drivers/firmware/google/Kconfig"
> > source "drivers/firmware/efi/Kconfig"
> > diff --git a/drivers/firmware/Makefile b/drivers/firmware/Makefile
> > index 4a4b897..b81b46e 100644
> > --- a/drivers/firmware/Makefile
> > +++ b/drivers/firmware/Makefile
> > @@ -13,6 +13,7 @@ obj-$(CONFIG_ISCSI_IBFT) += iscsi_ibft.o
> > obj-$(CONFIG_FIRMWARE_MEMMAP) += memmap.o
> > obj-$(CONFIG_QCOM_SCM) += qcom_scm.o
> > obj-$(CONFIG_QCOM_SCM) += qcom_scm-32.o
> > +obj-$(CONFIG_FW_CFG_SYSFS) += fw_cfg.o
> > CFLAGS_qcom_scm-32.o :=$(call as-instr,.arch_extension sec,-DREQUIRES_SEC=1)
> >
> > obj-y += broadcom/
> > diff --git a/drivers/firmware/fw_cfg.c b/drivers/firmware/fw_cfg.c
> > new file mode 100644
> > index 0000000..be17411
> > --- /dev/null
> > +++ b/drivers/firmware/fw_cfg.c
> > @@ -0,0 +1,438 @@
> > +/*
> > + * drivers/firmware/fw_cfg.c
> > + *
> > + * Expose entries from QEMU's firmware configuration (fw_cfg) device in
> > + * sysfs (read-only, under "/sys/firmware/fw_cfg/...").
> > + *
> > + * Copyright 2015 Carnegie Mellon University
> > + */
> > +
> > +#include <linux/module.h>
> > +#include <linux/capability.h>
> > +#include <linux/slab.h>
> > +#include <linux/io.h>
> > +#include <linux/ioport.h>
> > +#include <linux/ctype.h>
> > +
> > +/* selector values for "well-known" fw_cfg entries */
> > +#define FW_CFG_SIGNATURE 0x00
> > +#define FW_CFG_ID 0x01
> > +#define FW_CFG_FILE_DIR 0x19
> > +
> > +/* size in bytes of fw_cfg signature */
> > +#define FW_CFG_SIG_SIZE 4
> > +
> > +/* fw_cfg "file name" is up to 56 characters (including terminating nul) */
> > +#define FW_CFG_MAX_FILE_PATH 56
> > +
> > +/* fw_cfg file directory entry type */
> > +struct fw_cfg_file {
> > + uint32_t size;
> > + uint16_t select;
> > + uint16_t reserved;
>
> Those aren't valid kernel types, use u32, and u16 please.
>
> > + char name[FW_CFG_MAX_FILE_PATH];
> > +};
> > +
> > +/* fw_cfg device i/o access options type */
> > +struct fw_cfg_access {
> > + phys_addr_t start;
> > + uint8_t size;
> > + uint8_t ctrl_offset;
> > + uint8_t data_offset;
>
> u8.
Got it, s/uintXX_t/uXX/g in v2 :)
>
> > + bool is_mmio;
> > + const char *name;
> > +};
> > +
> > +/* fw_cfg device i/o access available options for known architectures */
> > +static struct fw_cfg_access fw_cfg_modes[] = {
> > + { 0x510, 2, 0, 1, false, "fw_cfg on i386, sun4u" },
> > + { 0x9020000, 10, 8, 0, true, "fw_cfg on arm" },
> > + { 0xd00000510, 3, 0, 2, true, "fw_cfg on sun4m" },
> > + { 0xf0000510, 3, 0, 2, true, "fw_cfg on ppc/mac" },
>
> named identifiers please.
OK, will do.
>
>
> > + { }
> > +};
> > +
> > +/* fw_cfg device i/o currently selected option set */
> > +static struct fw_cfg_access *fw_cfg_mode;
> > +
> > +/* fw_cfg device i/o register addresses */
> > +static void __iomem *fw_cfg_dev_base;
> > +static void __iomem *fw_cfg_dev_ctrl;
> > +static void __iomem *fw_cfg_dev_data;
> > +
> > +/* atomic access to fw_cfg device (potentially slow i/o, so using mutex) */
> > +static DEFINE_MUTEX(fw_cfg_dev_lock);
> > +
> > +/* pick apropriate endianness for selector key */
> > +static inline uint16_t fw_cfg_sel_endianness(uint16_t select)
> > +{
> > + return fw_cfg_mode->is_mmio ? cpu_to_be16(select) : cpu_to_le16(select);
> > +}
> > +
> > +/* type for fw_cfg "directory scan" visitor/callback function */
> > +typedef int (*fw_cfg_file_callback)(const struct fw_cfg_file *f);
> > +
> > +/* run a given callback on each fw_cfg directory entry */
> > +static int fw_cfg_scan_dir(fw_cfg_file_callback callback)
> > +{
> > + int ret = 0;
> > + uint32_t count, i;
>
> u32. Please remove all the *_t variable types. and i should be an int
> here, right?
i goes from 0 up to count, where count is u32. So I figured i should
be u32 as well, but in practice int would work just as well (count is
highly unlikely to be large enough to go past INT_MAX). Let me know if
you still want me to make it an int.
>
> > + struct fw_cfg_file f;
> > +
> > + mutex_lock(&fw_cfg_dev_lock);
> > + iowrite16(fw_cfg_sel_endianness(FW_CFG_FILE_DIR), fw_cfg_dev_ctrl);
> > + ioread8_rep(fw_cfg_dev_data, &count, sizeof(count));
> > + for (i = 0; i < be32_to_cpu(count); i++) {
> > + ioread8_rep(fw_cfg_dev_data, &f, sizeof(f));
> > + ret = callback(&f);
> > + if (ret)
> > + break;
> > + }
> > + mutex_unlock(&fw_cfg_dev_lock);
> > + return ret;
> > +}
> > +
> > +/* read chunk of given fw_cfg blob (caller responsible for sanity-check) */
> > +static inline void fw_cfg_read_blob(uint16_t select,
> > + void *buf, loff_t pos, size_t count)
> > +{
> > + mutex_lock(&fw_cfg_dev_lock);
> > + iowrite16(fw_cfg_sel_endianness(select), fw_cfg_dev_ctrl);
> > + while (pos-- > 0)
> > + ioread8(fw_cfg_dev_data);
> > + ioread8_rep(fw_cfg_dev_data, buf, count);
> > + mutex_unlock(&fw_cfg_dev_lock);
> > +}
> > +
> > +/* clean up fw_cfg device i/o setup */
> > +static void fw_cfg_io_cleanup(void)
> > +{
> > + if (fw_cfg_mode->is_mmio) {
> > + iounmap(fw_cfg_dev_base);
> > + release_mem_region(fw_cfg_mode->start, fw_cfg_mode->size);
> > + } else {
> > + ioport_unmap(fw_cfg_dev_base);
> > + release_region(fw_cfg_mode->start, fw_cfg_mode->size);
> > + }
> > +}
> > +
> > +/* probe and map fw_cfg device */
> > +static int __init fw_cfg_io_probe(void)
> > +{
> > + char sig[FW_CFG_SIG_SIZE];
> > +
> > + for (fw_cfg_mode = &fw_cfg_modes[0];
> > + fw_cfg_mode->start; fw_cfg_mode++) {
> > +
> > + phys_addr_t start = fw_cfg_mode->start;
> > + uint8_t size = fw_cfg_mode->size;
> > +
> > + /* reserve and map mmio or ioport region */
> > + if (fw_cfg_mode->is_mmio) {
> > + if (!request_mem_region(start, size, fw_cfg_mode->name))
> > + continue;
> > + fw_cfg_dev_base = ioremap(start, size);
> > + if (!fw_cfg_dev_base) {
> > + release_mem_region(start, size);
> > + continue;
> > + }
> > + } else {
> > + if (!request_region(start, size, fw_cfg_mode->name))
> > + continue;
> > + fw_cfg_dev_base = ioport_map(start, size);
> > + if (!fw_cfg_dev_base) {
> > + release_region(start, size);
> > + continue;
> > + }
> > + }
> > +
> > + /* set control and data register addresses */
> > + fw_cfg_dev_ctrl = fw_cfg_dev_base + fw_cfg_mode->ctrl_offset;
> > + fw_cfg_dev_data = fw_cfg_dev_base + fw_cfg_mode->data_offset;
> > +
> > + /* verify fw_cfg device signature */
> > + fw_cfg_read_blob(FW_CFG_SIGNATURE, sig, 0, FW_CFG_SIG_SIZE);
> > + if (memcmp(sig, "QEMU", FW_CFG_SIG_SIZE) == 0)
> > + /* success, we're done */
> > + return 0;
> > +
> > + /* clean up before probing next access mode */
> > + fw_cfg_io_cleanup();
> > + }
> > +
> > + return -ENODEV;
> > +}
> > +
> > +/* fw_cfg revision attribute, placed in top-level /sys/fw_cfg directory */
> > +static uint32_t fw_cfg_rev;
> > +
> > +static ssize_t fw_cfg_showrev(struct kobject *k, struct attribute *a, char *buf)
> > +{
> > + return sprintf(buf, "%u\n", fw_cfg_rev);
> > +}
> > +
> > +static const struct {
> > + struct attribute attr;
> > + ssize_t (*show)(struct kobject *k, struct attribute *a, char *buf);
> > +} fw_cfg_rev_attr = {
> > + .attr = { .name = "rev", .mode = S_IRUSR },
> > + .show = fw_cfg_showrev,
> > +};
> > +
> > +/* fw_cfg_sysfs_entry type */
> > +struct fw_cfg_sysfs_entry {
> > + struct kobject kobj;
> > + struct fw_cfg_file f;
> > + struct list_head list;
> > +};
> > +
> > +/* get fw_cfg_sysfs_entry from kobject member */
> > +static inline struct fw_cfg_sysfs_entry *to_entry(struct kobject *kobj)
> > +{
> > + return container_of(kobj, struct fw_cfg_sysfs_entry, kobj);
> > +}
> > +
> > +/* fw_cfg_sysfs_attribute type */
> > +struct fw_cfg_sysfs_attribute {
> > + struct attribute attr;
> > + ssize_t (*show)(struct fw_cfg_sysfs_entry *entry, char *buf);
> > +};
> > +
> > +/* get fw_cfg_sysfs_attribute from attribute member */
> > +static inline struct fw_cfg_sysfs_attribute *to_attr(struct attribute *attr)
> > +{
> > + return container_of(attr, struct fw_cfg_sysfs_attribute, attr);
> > +}
> > +
> > +/* global cache of fw_cfg_sysfs_entry objects */
> > +static LIST_HEAD(fw_cfg_entry_cache);
> > +
> > +/* kobjects removed lazily by kernel, mutual exclusion needed */
> > +static DEFINE_SPINLOCK(fw_cfg_cache_lock);
> > +
> > +static inline void fw_cfg_sysfs_cache_enlist(struct fw_cfg_sysfs_entry *entry)
> > +{
> > + spin_lock(&fw_cfg_cache_lock);
> > + list_add_tail(&entry->list, &fw_cfg_entry_cache);
> > + spin_unlock(&fw_cfg_cache_lock);
> > +}
> > +
> > +static inline void fw_cfg_sysfs_cache_delist(struct fw_cfg_sysfs_entry *entry)
> > +{
> > + spin_lock(&fw_cfg_cache_lock);
> > + list_del(&entry->list);
> > + spin_unlock(&fw_cfg_cache_lock);
> > +}
> > +
> > +static void fw_cfg_sysfs_cache_cleanup(void)
> > +{
> > + struct fw_cfg_sysfs_entry *entry, *next;
> > +
> > + list_for_each_entry_safe(entry, next, &fw_cfg_entry_cache, list) {
> > + /* will end up invoking fw_cfg_sysfs_cache_delist()
> > + * via each object's release() method (i.e. destructor) */
> > + kobject_put(&entry->kobj);
> > + }
> > +}
> > +
> > +/* default_attrs: per-entry attributes and show methods */
> > +
> > +#define FW_CFG_SYSFS_ATTR(_attr) \
> > +struct fw_cfg_sysfs_attribute fw_cfg_sysfs_attr_##_attr = { \
> > + .attr = { .name = __stringify(_attr), .mode = S_IRUSR }, \
> > + .show = fw_cfg_sysfs_show_##_attr, \
> > +}
> > +
> > +static ssize_t fw_cfg_sysfs_show_size(struct fw_cfg_sysfs_entry *e, char *buf)
> > +{
> > + return sprintf(buf, "%u\n", e->f.size);
> > +}
> > +
> > +static ssize_t fw_cfg_sysfs_show_select(struct fw_cfg_sysfs_entry *e, char *buf)
> > +{
> > + return sprintf(buf, "%u\n", e->f.select);
> > +}
> > +
> > +static ssize_t fw_cfg_sysfs_show_name(struct fw_cfg_sysfs_entry *e, char *buf)
> > +{
> > + return sprintf(buf, "%s\n", e->f.name);
> > +}
> > +
> > +static FW_CFG_SYSFS_ATTR(size);
> > +static FW_CFG_SYSFS_ATTR(select);
> > +static FW_CFG_SYSFS_ATTR(name);
> > +
> > +static struct attribute *fw_cfg_sysfs_entry_attrs[] = {
> > + &fw_cfg_sysfs_attr_size.attr,
> > + &fw_cfg_sysfs_attr_select.attr,
> > + &fw_cfg_sysfs_attr_name.attr,
> > + NULL,
> > +};
> > +
> > +/* sysfs_ops: find fw_cfg_[entry, attribute] and call appropriate show method */
> > +static ssize_t fw_cfg_sysfs_attr_show(struct kobject *kobj, struct attribute *a,
> > + char *buf)
> > +{
> > + struct fw_cfg_sysfs_entry *entry = to_entry(kobj);
> > + struct fw_cfg_sysfs_attribute *attr = to_attr(a);
> > +
> > + if (!capable(CAP_SYS_ADMIN))
> > + return -EACCES;
>
> Shouldn't the file permissions handle this properly for you?
>
> > +
> > + return attr->show(entry, buf);
> > +}
> > +
> > +static const struct sysfs_ops fw_cfg_sysfs_attr_ops = {
> > + .show = fw_cfg_sysfs_attr_show,
> > +};
> > +
> > +/* release: destructor, to be called via kobject_put() */
> > +static void fw_cfg_sysfs_release_entry(struct kobject *kobj)
> > +{
> > + struct fw_cfg_sysfs_entry *entry = to_entry(kobj);
> > +
> > + fw_cfg_sysfs_cache_delist(entry);
> > + kfree(entry);
> > +}
> > +
> > +/* kobj_type: ties together all properties required to register an entry */
> > +static struct kobj_type fw_cfg_sysfs_entry_ktype = {
> > + .default_attrs = fw_cfg_sysfs_entry_attrs,
> > + .sysfs_ops = &fw_cfg_sysfs_attr_ops,
> > + .release = fw_cfg_sysfs_release_entry,
> > +};
> > +
> > +/* raw-read method and attribute */
> > +static ssize_t fw_cfg_sysfs_read_raw(struct file *filp, struct kobject *kobj,
> > + struct bin_attribute *bin_attr,
> > + char *buf, loff_t pos, size_t count)
> > +{
> > + struct fw_cfg_sysfs_entry *entry = to_entry(kobj);
> > +
> > + if (!capable(CAP_SYS_ADMIN))
> > + return -EACCES;
>
> Again, file permissions?
In retrospect, you're right. I used another, existing sysfs driver as
inspiration, and this particular redundancy slipped through the cracks
during cleanup. I'll get rid of it for v2.
>
> > +
> > + if (pos > entry->f.size)
> > + return -EINVAL;
> > +
> > + if (count > entry->f.size - pos)
> > + count = entry->f.size - pos;
> > +
> > + fw_cfg_read_blob(entry->f.select, buf, pos, count);
> > + return count;
> > +}
> > +
> > +static struct bin_attribute fw_cfg_sysfs_attr_raw = {
> > + .attr = { .name = "raw", .mode = 0400 },
> > + .read = fw_cfg_sysfs_read_raw,
> > +};
> > +
> > +/* kobjects & kset representing top-level, by_select, and by_name folders */
> > +static struct kobject *fw_cfg_top_ko;
> > +static struct kobject *fw_cfg_sel_ko;
> > +
> > +/* callback function to register an individual fw_cfg file */
> > +static int __init fw_cfg_register_file(const struct fw_cfg_file *f)
> > +{
> > + int err;
> > + struct fw_cfg_sysfs_entry *entry;
> > +
> > + /* allocate new entry */
> > + entry = kzalloc(sizeof(*entry), GFP_KERNEL);
> > + if (!entry)
> > + return -ENOMEM;
> > +
> > + /* set file entry information */
> > + entry->f.size = be32_to_cpu(f->size);
> > + entry->f.select = be16_to_cpu(f->select);
> > + strcpy(entry->f.name, f->name);
> > +
> > + /* register entry under "/sys/firmware/fw_cfg/by_select/" */
> > + err = kobject_init_and_add(&entry->kobj, &fw_cfg_sysfs_entry_ktype,
> > + fw_cfg_sel_ko, "%d", entry->f.select);
> > + if (err)
> > + goto err_register;
> > +
> > + /* add raw binary content access */
> > + err = sysfs_create_bin_file(&entry->kobj, &fw_cfg_sysfs_attr_raw);
> > + if (err)
> > + goto err_add_raw;
> > +
> > + /* success, add entry to global cache */
> > + fw_cfg_sysfs_cache_enlist(entry);
> > + return 0;
> > +
> > +err_add_raw:
> > + kobject_del(&entry->kobj);
> > +err_register:
> > + kfree(entry);
> > + return err;
> > +}
> > +
> > +/* unregister top-level or by_select folder */
> > +static inline void fw_cfg_kobj_cleanup(struct kobject *kobj)
> > +{
> > + kobject_del(kobj);
> > + kobject_put(kobj);
> > +}
> > +
> > +static int __init fw_cfg_sysfs_init(void)
> > +{
> > + int err;
> > +
> > + /* probe for the fw_cfg "hardware" */
> > + err = fw_cfg_io_probe();
> > + if (err)
> > + return err;
> > +
> > + /* create /sys/firmware/fwcfg/ and its subdirectories */
> > + err = -ENOMEM;
> > + fw_cfg_top_ko = kobject_create_and_add("fw_cfg", firmware_kobj);
> > + if (!fw_cfg_top_ko)
> > + goto err_top;
> > + fw_cfg_sel_ko = kobject_create_and_add("by_select", fw_cfg_top_ko);
> > + if (!fw_cfg_sel_ko)
> > + goto err_sel;
> > +
> > + /* get revision number, add matching top-level attribute */
> > + fw_cfg_read_blob(FW_CFG_ID, &fw_cfg_rev, 0, sizeof(fw_cfg_rev));
> > + fw_cfg_rev = le32_to_cpu(fw_cfg_rev);
> > + err = sysfs_create_file(fw_cfg_top_ko, &fw_cfg_rev_attr.attr);
> > + if (err)
> > + goto err_rev;
> > +
> > + /* process fw_cfg file directory entry, registering each file */
> > + err = fw_cfg_scan_dir(fw_cfg_register_file);
>
> Dealing with all of these "raw" kobjecs makes me nervous. Why can't you
> use 'struct device' for this instead, on the system bus?
I'll have to read up on 'struct device' to figure out why I would or
wouldn't want to do that instead -- sorry, still a n00b trying to
drink from the firehose :)
Mainly, qemu's fw_cfg is a read-only "device", so one wouldn't ever
care to try writing anything to it. /sys/firmware/... feels like a fit
because fw_cfg contains binary blobs originally meant to be used by
the bios (it's how SeaBIOS and OVMF pull smbios and acpi tables out of
the host and set them up in guest memory before booting the guest kernel,
as one of the many examples).
The most similar example (and the existing driver I used as an example
during implementation) is dmi-sysfs.c, which also exposes all the
metadata for each smbios table as numeric (or string) read-only attributes,
and has a "raw" attribute which allows dumping each table in its
entirety. Same thing here -- I want to expose the name, size, and
select key for each blob, but also allow access to the "payload", i.e.
the blob itself.
Thanks much for the feedback !
--Gabriel
^ permalink raw reply
* Re: [PATCH 2/3] kobject: export kset_find_obj() to be used from modules
From: Gabriel L. Somlo @ 2015-08-10 19:04 UTC (permalink / raw)
To: Greg KH
Cc: ralf-6z/3iImG2C8G8FEW9MqTrA, zajec5-Re5JQEeQqe8AvxtiuMwx3w,
paul-DWxLp4Yu+b8AvxtiuMwx3w, galak-sgV2jX0FEOL9JmXXK+q4OQ,
linux-api-u79uwXL29TY76Z2rM5mHXA,
linux-kernel-u79uwXL29TY76Z2rM5mHXA,
matt.fleming-ral2JQCrhuEAvxtiuMwx3w, x86-DgEjT+Ai2ygdnm+yROfE0A,
linux-efi-u79uwXL29TY76Z2rM5mHXA,
qemu-devel-qX2TKyscuCcdnm+yROfE0A, lersek-H+wXaHxf7aLQT0dZR+AlfA,
jordan.l.justen-ral2JQCrhuEAvxtiuMwx3w,
gleb-RmZWMc9puTNJc61us3aD9laTQe2KTcn/,
pbonzini-H+wXaHxf7aLQT0dZR+AlfA, kraxel-H+wXaHxf7aLQT0dZR+AlfA,
eblake-H+wXaHxf7aLQT0dZR+AlfA, rjones-H+wXaHxf7aLQT0dZR+AlfA,
kernelnewbies-7JyXY6prKcjpASu1u0TL5ti2O/JbrIOy
In-Reply-To: <20150810185400.GC19646-U8xfFu+wG4EAvxtiuMwx3w@public.gmane.org>
On Mon, Aug 10, 2015 at 11:54:00AM -0700, Greg KH wrote:
> On Mon, Aug 10, 2015 at 02:43:10PM -0400, Gabriel L. Somlo wrote:
> > On Mon, Aug 10, 2015 at 11:33:04AM -0700, Greg KH wrote:
> > > On Mon, Aug 10, 2015 at 12:31:19PM -0400, Gabriel L. Somlo wrote:
> > > > From: "Gabriel Somlo" <somlo-D+Gtc/HYRWM@public.gmane.org>
> > > >
> > > > Signed-off-by: Gabriel Somlo <somlo-D+Gtc/HYRWM@public.gmane.org>
> > > > ---
> > > > lib/kobject.c | 1 +
> > > > 1 file changed, 1 insertion(+)
> > > >
> > > > diff --git a/lib/kobject.c b/lib/kobject.c
> > > > index 3e3a5c3..f9754a0 100644
> > > > --- a/lib/kobject.c
> > > > +++ b/lib/kobject.c
> > > > @@ -1058,3 +1058,4 @@ EXPORT_SYMBOL(kobject_del);
> > > >
> > > > EXPORT_SYMBOL(kset_register);
> > > > EXPORT_SYMBOL(kset_unregister);
> > > > +EXPORT_SYMBOL(kset_find_obj);
> > >
> > > checkpatch doesn't like this for obvious reasons :)
> >
> > When I run checkpatch on this one, it comes back fine. Could you
> > please elaborate ?
> >
> > [staging]$ scripts/checkpatch.pl
> > 0002-kobject-export-kset_find_obj-to-be-used-from-modules.patch
> > total: 0 errors, 0 warnings, 4 lines checked
> >
> > 0002-kobject-export-kset_find_obj-to-be-used-from-modules.patch has no
> > obvious style problems and is ready for submission.
>
> EXPORT_SYMBOL() should be below the function definition itself, not at
> the bottom of the file. Odd that checkpatch doesn't catch this when
> done in this manner, it will if you run it on the whole file after your
> patch is applied (along with the existing problems.)
Got it, fix lined up for v2
> But this is a minor issue, see my other review first...
Yeah, but it was the low hanging fruit, so I replied to it first ;)
Thanks much,
--Gabriel
^ permalink raw reply
* Re: [PATCH 1/3] firmware: introduce sysfs driver for QEMU's fw_cfg device
From: Greg KH @ 2015-08-10 19:15 UTC (permalink / raw)
To: Gabriel L. Somlo
Cc: ralf-6z/3iImG2C8G8FEW9MqTrA, zajec5-Re5JQEeQqe8AvxtiuMwx3w,
paul-DWxLp4Yu+b8AvxtiuMwx3w, galak-sgV2jX0FEOL9JmXXK+q4OQ,
linux-api-u79uwXL29TY76Z2rM5mHXA,
linux-kernel-u79uwXL29TY76Z2rM5mHXA,
matt.fleming-ral2JQCrhuEAvxtiuMwx3w, x86-DgEjT+Ai2ygdnm+yROfE0A,
linux-efi-u79uwXL29TY76Z2rM5mHXA,
qemu-devel-qX2TKyscuCcdnm+yROfE0A, lersek-H+wXaHxf7aLQT0dZR+AlfA,
jordan.l.justen-ral2JQCrhuEAvxtiuMwx3w,
gleb-RmZWMc9puTNJc61us3aD9laTQe2KTcn/,
pbonzini-H+wXaHxf7aLQT0dZR+AlfA, kraxel-H+wXaHxf7aLQT0dZR+AlfA,
eblake-H+wXaHxf7aLQT0dZR+AlfA, rjones-H+wXaHxf7aLQT0dZR+AlfA,
kernelnewbies-7JyXY6prKcjpASu1u0TL5ti2O/JbrIOy
In-Reply-To: <20150810190222.GN31816-h65ZQ0r4j6KKUezXOiBB2eW1CriLhL8O@public.gmane.org>
On Mon, Aug 10, 2015 at 03:02:22PM -0400, Gabriel L. Somlo wrote:
> Mainly, qemu's fw_cfg is a read-only "device", so one wouldn't ever
> care to try writing anything to it. /sys/firmware/... feels like a fit
> because fw_cfg contains binary blobs originally meant to be used by
> the bios (it's how SeaBIOS and OVMF pull smbios and acpi tables out of
> the host and set them up in guest memory before booting the guest kernel,
> as one of the many examples).
>
> The most similar example (and the existing driver I used as an example
> during implementation) is dmi-sysfs.c, which also exposes all the
> metadata for each smbios table as numeric (or string) read-only attributes,
> and has a "raw" attribute which allows dumping each table in its
> entirety. Same thing here -- I want to expose the name, size, and
> select key for each blob, but also allow access to the "payload", i.e.
> the blob itself.
That's great, and you can have these "blobs" be an attribute for a
struct device. Just using a "raw" kobject as you are is hard, as you
have seen, and messy. Making it a "real" device makes this all much
easier and simpler.
Now if you want to keep things in /sys/firmware/ that's another issue,
and would have to stay as a kobject. so maybe it does need to remain,
need to think about that...
thanks,
greg k-h
^ permalink raw reply
* Re: [PATCH 1/4] selftests: vm: Fix mlock2-tests for 32-bit architectures
From: Shuah Khan @ 2015-08-10 19:27 UTC (permalink / raw)
To: Thierry Reding, Andrew Morton
Cc: Eric B Munson, Andrea Arcangeli, linux-api-u79uwXL29TY76Z2rM5mHXA,
linux-kernel-u79uwXL29TY76Z2rM5mHXA, Shuah Khan
In-Reply-To: <1438955055-587-1-git-send-email-thierry.reding-Re5JQEeQqe8AvxtiuMwx3w@public.gmane.org>
On 08/07/2015 07:44 AM, Thierry Reding wrote:
> From: Thierry Reding <treding-DDmLM1+adcrQT0dZR+AlfA@public.gmane.org>
>
> According to Documentation/vm/pagemap.txt, the /proc/pid/pagemap file
> contains one 64-bit value for each virtual page. The test code relies
> on the size of unsigned long being 64-bit, which breaks the test when
> run on 32-bit architectures. Use a uint64_t to store values read from
> the file instead, so that it works irrespective of the architecture's
> word size.
>
> Signed-off-by: Thierry Reding <treding-DDmLM1+adcrQT0dZR+AlfA@public.gmane.org>
> ---
Looks good to me. Andrew! Would you like me to take this patch
and the other two in this series through kselftest git?
thanks,
-- Shuah
--
Shuah Khan
Sr. Linux Kernel Developer
Open Source Innovation Group
Samsung Research America (Silicon Valley)
shuahkh-JPH+aEBZ4P+UEJcrhfAQsw@public.gmane.org | (970) 217-8978
^ permalink raw reply
* Re: [PATCH] eventfd: implementation of EFD_MASK flag
From: Martin Sustrik @ 2015-08-10 21:16 UTC (permalink / raw)
To: Damian Hobson-Garcia
Cc: viro-RmSDqhL/yNMiFSDQTTA3OLVCufUGDwFn,
linux-fsdevel-u79uwXL29TY76Z2rM5mHXA,
linux-kernel-u79uwXL29TY76Z2rM5mHXA,
netdev-u79uwXL29TY76Z2rM5mHXA, linux-api-u79uwXL29TY76Z2rM5mHXA
In-Reply-To: <55C86762.5010709-AlSX/UN32fvPDbFq/vQRIQ@public.gmane.org>
On 2015-08-10 10:57, Damian Hobson-Garcia wrote:
> Hi Martin,
>
> Thanks for your comments.
>
> On 2015-08-10 3:39 PM, Martin Sustrik wrote:
>> On 2015-08-10 08:23, Damian Hobson-Garcia wrote:
>>> Replying to my own post, but I had the following comments/questions.
>>> Martin, if you have any response to my comments I would be very happy
>>> to
>>> hear them.
>>>
>>> On 2015-08-10 2:51 PM, Damian Hobson-Garcia wrote:
>>>> From: Martin Sustrik <sustrik-8XN41Hm7QBXQT0dZR+AlfA@public.gmane.org>
>>>>
>>> [snip]
>>>>
>>>> write(2):
>>>>
>>>> User is allowed to write only buffers containing the following
>>>> structure:
>>>>
>>>> struct efd_mask {
>>>> __u32 events;
>>>> __u64 data;
>>>> };
>>>>
>>>> The value of 'events' should be any combination of event flags as
>>>> defined by
>>>> poll(2) function (POLLIN, POLLOUT, POLLERR, POLLHUP etc.) Specified
>>>> events will
>>>> be signaled when polling (select, poll, epoll) on the eventfd is
>>>> done
>>>> later on.
>>>> 'data' is opaque data that are not interpreted by eventfd object.
>>>>
>>> I'm not fully clear on the purpose that the 'data' member serves.
>>> Does
>>> this opaque handle need to be tied together with this event
>>> synchronization construct?
>>
>> It's a convenience thing. Imagine you are implementing your own file
>> descriptor type in user space. You create an EFD_MASK socket and a
>> structure that will hold any state that you need for the socket (tx/rx
>> buffers and such).
>>
>> Now you have two things to pass around. If you want to pass the fd to
>> a
>> function, it must have two parameters (fd and pointer to the
>> structure).
>>
>> To fix it you can put the fd into the structure. That way there's only
>> one thing to pass around (the structure).
>>
>> The problem with that approach is when you have generic code that
>> deals
>> with file descriptors. For example, a simple poller which accepts a
>> list
>> of (fd, callback) pairs and invokes the callback when one of the fds
>> signals POLLIN. You can't send a pointer to a structure to such
>> function. All you can send is the fd, but then, when the callback is
>> invoked, fd is all you have. You have no idea where your state is.
>>
>> 'data' member allows you to put the pointer to the state to the socket
>> itself. Thus, if you have a fd, you can always find out where the
>> associated data is by reading the mask structure from the fd.
>>
>
> Ok, I see what you're saying. I guess that keeping track of the mapping
> between the fd and the struct in user space could be non-trivial if
> there are a large number of active fds that are polling very
> frequently.
> Wouldn't it be sufficient to just use epoll() in this case though? It
> already seems to support this kind of thing.
My use case was like this:
int s = mysocket();
...
// myrecv() can get the pointer to the structure
// without user having to pass it as an argument
myrecv(s, buf, sizeof(buf));
However, same behaviour can be accomplished by simply keeping
a static array of pointers in the user space.
So let's cut this part out of the patch.
>
>>>
>>> [snip]
>>>
>>>> @@ -55,6 +69,9 @@ __u64 eventfd_signal(struct eventfd_ctx *ctx,
>>>> __u64 n)
>>>> {
>>>> + /* This function should never be used with eventfd in the mask
>>>> mode. */
>>>> + BUG_ON(ctx->flags & EFD_MASK);
>>>> +
>>> ...
>>>> @@ -158,6 +180,9 @@ int eventfd_ctx_remove_wait_queue(struct
>>>> eventfd_ctx *ctx, wait_queue_t *wait,
>>>> {
>>>> + /* This function should never be used with eventfd in the mask
>>>> mode. */
>>>> + BUG_ON(ctx->flags & EFD_MASK);
>>>> +
>>> ...
>>>> @@ -188,6 +213,9 @@ ssize_t eventfd_ctx_read(struct eventfd_ctx
>>>> *ctx,
>>>> int no_wait, __u64 *cnt)
>>>> + /* This function should never be used with eventfd in the mask
>>>> mode. */
>>>> + BUG_ON(ctx->flags & EFD_MASK);
>>>> +
>>>
>>> If eventfd_ctx_fileget() returns EINVAL when EFD_MASK is set, I don't
>>> think that there will be a way to call these functions in the mask
>>> mode,
>>> so it should be possible to get rid of the BUG_ON checks.
>>
>> Sure. Feel free to do so.
>>
>>>
>>> [snip]
>>>> @@ -230,6 +258,19 @@ static ssize_t eventfd_read(struct file *file,
>>>> char __user *buf, size_t count,
>>>> ssize_t res;
>>>> __u64 cnt;
>>>>
>>>> + if (ctx->flags & EFD_MASK) {
>>>> + struct efd_mask mask;
>>>> +
>>>> + if (count < sizeof(mask))
>>>> + return -EINVAL;
>>>> + spin_lock_irq(&ctx->wqh.lock);
>>>> + mask = ctx->mask;
>>>> + spin_unlock_irq(&ctx->wqh.lock);
>>>> + if (copy_to_user(buf, &mask, sizeof(mask)))
>>>> + return -EFAULT;
>>>> + return sizeof(mask);
>>>> + }
>>>> +
>>>
>>> For the other eventfd modes, reading the value will update the
>>> internal
>>> state of the eventfd (either clearing or decrementing the counter).
>>> Should something similar be done here? I'm thinking of a case where a
>>> process is polling on this fd in a loop. Clearing the efd_mask data
>>> on
>>> read should provide an easy way for the polling process to know if it
>>> is
>>> seeing new poll events.
>>
>> No. In this case reading the value has no effect on the state of the
>> fd.
>> How it should work is rather:
>>
>> // fd is in POLLIN state
>> poll(fd);
>> // function exits with POLLIN but fd remains in POLLIN state
>> my_recv(fd, buf, size);
>> // my_recv function have found out that there's no more data to recv
>> and
>> switched off the POLLIN flag
>> poll(fd); // we block here waiting for more data to arrive from the
>> network
>>
>
> How exactly doe the receiver switch off the POLLIN flag? Does the
> receiver also write to the eventfd? or do you mean that it just doesn't
> set POLLIN in the events mask? It seems cleaner to have the sender
> only
> write the eventfd and the receiver only read it. Your example would be
> exactly the same, except that my_recv(fd, buf, size) would read to
> clear
> instead of write.
Keep in mind that the user of your mysocket is not supposed to do
recv() or send() on the raw underlying fd. It's the implementation,
myrecv() and mysend(), that does that.
That being the case and also assuming that we cut the pointer out, there
seems
to be little use for recv() any more. The implementation of socket knows
what state it is in and so it doesn't have to retrieve it using recv().
All it has to do is perform whatever business logic is needed and then
set new
state of the socket via send().
And the fact there's no clear use case, the logic of recv() is not
obvious.
We can very well return ENOTIMPL.
>
>>>
>>>> @@ -292,8 +351,13 @@ static void eventfd_show_fdinfo(struct seq_file
>>>> *m, struct file *f)
>>>> struct eventfd_ctx *ctx = f->private_data;
>>>>
>>>> spin_lock_irq(&ctx->wqh.lock);
>>>> - seq_printf(m, "eventfd-count: %16llx\n",
>>>> - (unsigned long long)ctx->count);
>>>> + if (ctx->flags & EFD_MASK) {
>>>> + seq_printf(m, "eventfd-mask: %x\n",
>>>> + (unsigned)ctx->mask.events);
>>>> + } else {
>>>> + seq_printf(m, "eventfd-count: %16llx\n",
>>>> + (unsigned long long)ctx->count);
>>>> + }
>>>> spin_unlock_irq(&ctx->wqh.lock);
>>>> }
>>> I think that putting the EFD_MASK functionality into a different fops
>>> structure might be useful for reducing the number of if statements.
>>
>> Sure. No objections.
>>
>> Thanks for re-submitting the patch!
>
> My pleasure.
>
>> Martin
>>
>
> Damian
^ permalink raw reply
* Re: [PATCH] eventfd: implementation of EFD_MASK flag
From: Damian Hobson-Garcia @ 2015-08-11 7:54 UTC (permalink / raw)
To: Martin Sustrik; +Cc: viro, linux-fsdevel, linux-kernel, netdev, linux-api
In-Reply-To: <2a70fc7c97ec7cc57c3eb3c4a60e8a2f@imap.lucina.net>
On 2015-08-11 6:16 AM, Martin Sustrik wrote:
> On 2015-08-10 10:57, Damian Hobson-Garcia wrote:
>> Hi Martin,
>>
>> Thanks for your comments.
>>
>> On 2015-08-10 3:39 PM, Martin Sustrik wrote:
>>> On 2015-08-10 08:23, Damian Hobson-Garcia wrote:
>>>> Replying to my own post, but I had the following comments/questions.
>>>> Martin, if you have any response to my comments I would be very
>>>> happy to
>>>> hear them.
>>>>
>>>> On 2015-08-10 2:51 PM, Damian Hobson-Garcia wrote:
>>>>> From: Martin Sustrik <sustrik@250bpm.com>
>>>>>
>>>> [snip]
>>>>>
>>>>> write(2):
>>>>>
>>>>> User is allowed to write only buffers containing the following
>>>>> structure:
>>>>>
>>>>> struct efd_mask {
>>>>> __u32 events;
>>>>> __u64 data;
>>>>> };
>>>>>
>>>>> The value of 'events' should be any combination of event flags as
>>>>> defined by
>>>>> poll(2) function (POLLIN, POLLOUT, POLLERR, POLLHUP etc.) Specified
>>>>> events will
>>>>> be signaled when polling (select, poll, epoll) on the eventfd is done
>>>>> later on.
>>>>> 'data' is opaque data that are not interpreted by eventfd object.
>>>>>
>>>> I'm not fully clear on the purpose that the 'data' member serves. Does
>>>> this opaque handle need to be tied together with this event
>>>> synchronization construct?
>>>
>>> It's a convenience thing. Imagine you are implementing your own file
>>> descriptor type in user space. You create an EFD_MASK socket and a
>>> structure that will hold any state that you need for the socket (tx/rx
>>> buffers and such).
>>>
>>> Now you have two things to pass around. If you want to pass the fd to a
>>> function, it must have two parameters (fd and pointer to the structure).
>>>
>>> To fix it you can put the fd into the structure. That way there's only
>>> one thing to pass around (the structure).
>>>
>>> The problem with that approach is when you have generic code that deals
>>> with file descriptors. For example, a simple poller which accepts a list
>>> of (fd, callback) pairs and invokes the callback when one of the fds
>>> signals POLLIN. You can't send a pointer to a structure to such
>>> function. All you can send is the fd, but then, when the callback is
>>> invoked, fd is all you have. You have no idea where your state is.
>>>
>>> 'data' member allows you to put the pointer to the state to the socket
>>> itself. Thus, if you have a fd, you can always find out where the
>>> associated data is by reading the mask structure from the fd.
>>>
>>
>> Ok, I see what you're saying. I guess that keeping track of the mapping
>> between the fd and the struct in user space could be non-trivial if
>> there are a large number of active fds that are polling very frequently.
>> Wouldn't it be sufficient to just use epoll() in this case though? It
>> already seems to support this kind of thing.
>
> My use case was like this:
>
> int s = mysocket();
> ...
> // myrecv() can get the pointer to the structure
> // without user having to pass it as an argument
> myrecv(s, buf, sizeof(buf));
>
> However, same behaviour can be accomplished by simply keeping
> a static array of pointers in the user space.
>
> So let's cut this part out of the patch.
>
Ok, I'll drop the 'data' member. I could add some padding to the
efd_mask structure so that it is the same size as the 64-bit data size
used when EFD_MASK is not set.
>>
>>>>
>>>> [snip]
>>>>
>>>>> @@ -55,6 +69,9 @@ __u64 eventfd_signal(struct eventfd_ctx *ctx,
>>>>> __u64 n)
>>>>> {
>>>>> + /* This function should never be used with eventfd in the mask
>>>>> mode. */
>>>>> + BUG_ON(ctx->flags & EFD_MASK);
>>>>> +
>>>> ...
>>>>> @@ -158,6 +180,9 @@ int eventfd_ctx_remove_wait_queue(struct
>>>>> eventfd_ctx *ctx, wait_queue_t *wait,
>>>>> {
>>>>> + /* This function should never be used with eventfd in the mask
>>>>> mode. */
>>>>> + BUG_ON(ctx->flags & EFD_MASK);
>>>>> +
>>>> ...
>>>>> @@ -188,6 +213,9 @@ ssize_t eventfd_ctx_read(struct eventfd_ctx *ctx,
>>>>> int no_wait, __u64 *cnt)
>>>>> + /* This function should never be used with eventfd in the mask
>>>>> mode. */
>>>>> + BUG_ON(ctx->flags & EFD_MASK);
>>>>> +
>>>>
>>>> If eventfd_ctx_fileget() returns EINVAL when EFD_MASK is set, I don't
>>>> think that there will be a way to call these functions in the mask
>>>> mode,
>>>> so it should be possible to get rid of the BUG_ON checks.
>>>
>>> Sure. Feel free to do so.
>>>
>>>>
>>>> [snip]
>>>>> @@ -230,6 +258,19 @@ static ssize_t eventfd_read(struct file *file,
>>>>> char __user *buf, size_t count,
>>>>> ssize_t res;
>>>>> __u64 cnt;
>>>>>
>>>>> + if (ctx->flags & EFD_MASK) {
>>>>> + struct efd_mask mask;
>>>>> +
>>>>> + if (count < sizeof(mask))
>>>>> + return -EINVAL;
>>>>> + spin_lock_irq(&ctx->wqh.lock);
>>>>> + mask = ctx->mask;
>>>>> + spin_unlock_irq(&ctx->wqh.lock);
>>>>> + if (copy_to_user(buf, &mask, sizeof(mask)))
>>>>> + return -EFAULT;
>>>>> + return sizeof(mask);
>>>>> + }
>>>>> +
>>>>
>>>> For the other eventfd modes, reading the value will update the internal
>>>> state of the eventfd (either clearing or decrementing the counter).
>>>> Should something similar be done here? I'm thinking of a case where a
>>>> process is polling on this fd in a loop. Clearing the efd_mask data on
>>>> read should provide an easy way for the polling process to know if
>>>> it is
>>>> seeing new poll events.
>>>
>>> No. In this case reading the value has no effect on the state of the fd.
>>> How it should work is rather:
>>>
>>> // fd is in POLLIN state
>>> poll(fd);
>>> // function exits with POLLIN but fd remains in POLLIN state
>>> my_recv(fd, buf, size);
>>> // my_recv function have found out that there's no more data to recv and
>>> switched off the POLLIN flag
>>> poll(fd); // we block here waiting for more data to arrive from the
>>> network
>>>
>>
>> How exactly doe the receiver switch off the POLLIN flag? Does the
>> receiver also write to the eventfd? or do you mean that it just doesn't
>> set POLLIN in the events mask? It seems cleaner to have the sender only
>> write the eventfd and the receiver only read it. Your example would be
>> exactly the same, except that my_recv(fd, buf, size) would read to clear
>> instead of write.
>
> Keep in mind that the user of your mysocket is not supposed to do
> recv() or send() on the raw underlying fd. It's the implementation,
> myrecv() and mysend(), that does that.
>
> That being the case and also assuming that we cut the pointer out, there
> seems
> to be little use for recv() any more. The implementation of socket knows
> what state it is in and so it doesn't have to retrieve it using recv().
>
> All it has to do is perform whatever business logic is needed and then
> set new
> state of the socket via send().
>
> And the fact there's no clear use case, the logic of recv() is not obvious.
> We can very well return ENOTIMPL.
If the user of mysocket will only directly interact with the fd through
poll/select/epoll then yes, read()/recv() doesn't have any use, and I
agree, dropping it seems cleanest.
>
>
>>
>>>>
>>>>> @@ -292,8 +351,13 @@ static void eventfd_show_fdinfo(struct seq_file
>>>>> *m, struct file *f)
>>>>> struct eventfd_ctx *ctx = f->private_data;
>>>>>
>>>>> spin_lock_irq(&ctx->wqh.lock);
>>>>> - seq_printf(m, "eventfd-count: %16llx\n",
>>>>> - (unsigned long long)ctx->count);
>>>>> + if (ctx->flags & EFD_MASK) {
>>>>> + seq_printf(m, "eventfd-mask: %x\n",
>>>>> + (unsigned)ctx->mask.events);
>>>>> + } else {
>>>>> + seq_printf(m, "eventfd-count: %16llx\n",
>>>>> + (unsigned long long)ctx->count);
>>>>> + }
>>>>> spin_unlock_irq(&ctx->wqh.lock);
>>>>> }
>>>> I think that putting the EFD_MASK functionality into a different fops
>>>> structure might be useful for reducing the number of if statements.
>>>
>>> Sure. No objections.
>>>
>>> Thanks for re-submitting the patch!
>>
>> My pleasure.
>>
>>> Martin
>>>
>>
>> Damian
>
Damian
^ permalink raw reply
* Re: [Qemu-devel] [PATCH 19/23] userfaultfd: activate syscall
From: Bharata B Rao @ 2015-08-11 10:07 UTC (permalink / raw)
To: Andrea Arcangeli
Cc: Andrew Morton, linux-kernel, linux-mm, qemu-devel, kvm, linux-api,
zhang.zhanghailiang, Pavel Emelyanov, Johannes Weiner,
Hugh Dickins, Dr. David Alan Gilbert, Sanidhya Kashyap,
Dave Hansen, Andres Lagar-Cavilla, Mel Gorman, Paolo Bonzini,
Kirill A. Shutemov, Huangpeng (Peter), Andy Lutomirski,
Linus Torvalds, Peter Feiner
In-Reply-To: <1431624680-20153-20-git-send-email-aarcange@redhat.com>
On Thu, May 14, 2015 at 07:31:16PM +0200, Andrea Arcangeli wrote:
> This activates the userfaultfd syscall.
>
> Signed-off-by: Andrea Arcangeli <aarcange@redhat.com>
> ---
> arch/powerpc/include/asm/systbl.h | 1 +
> arch/powerpc/include/uapi/asm/unistd.h | 1 +
> arch/x86/syscalls/syscall_32.tbl | 1 +
> arch/x86/syscalls/syscall_64.tbl | 1 +
> include/linux/syscalls.h | 1 +
> kernel/sys_ni.c | 1 +
> 6 files changed, 6 insertions(+)
>
> diff --git a/arch/powerpc/include/asm/systbl.h b/arch/powerpc/include/asm/systbl.h
> index f1863a1..4741b15 100644
> --- a/arch/powerpc/include/asm/systbl.h
> +++ b/arch/powerpc/include/asm/systbl.h
> @@ -368,3 +368,4 @@ SYSCALL_SPU(memfd_create)
> SYSCALL_SPU(bpf)
> COMPAT_SYS(execveat)
> PPC64ONLY(switch_endian)
> +SYSCALL_SPU(userfaultfd)
> diff --git a/arch/powerpc/include/uapi/asm/unistd.h b/arch/powerpc/include/uapi/asm/unistd.h
> index e4aa173..6ad58d4 100644
> --- a/arch/powerpc/include/uapi/asm/unistd.h
> +++ b/arch/powerpc/include/uapi/asm/unistd.h
> @@ -386,5 +386,6 @@
> #define __NR_bpf 361
> #define __NR_execveat 362
> #define __NR_switch_endian 363
> +#define __NR_userfaultfd 364
May be it is a bit late to bring this up, but I needed the following fix
to userfault21 branch of your git tree to compile on powerpc.
----
powerpc: Bump up __NR_syscalls to account for __NR_userfaultfd
From: Bharata B Rao <bharata@linux.vnet.ibm.com>
With userfaultfd syscall, the number of syscalls will be 365 on PowerPC.
Reflect the same in __NR_syscalls.
Signed-off-by: Bharata B Rao <bharata@linux.vnet.ibm.com>
---
arch/powerpc/include/asm/unistd.h | 2 +-
1 file changed, 1 insertion(+), 1 deletion(-)
diff --git a/arch/powerpc/include/asm/unistd.h b/arch/powerpc/include/asm/unistd.h
index f4f8b66..4a055b6 100644
--- a/arch/powerpc/include/asm/unistd.h
+++ b/arch/powerpc/include/asm/unistd.h
@@ -12,7 +12,7 @@
#include <uapi/asm/unistd.h>
-#define __NR_syscalls 364
+#define __NR_syscalls 365
#define __NR__exit __NR_exit
#define NR_syscalls __NR_syscalls
^ permalink raw reply related
* [PATCH RFC 0/3] nvmem: make sysfs binary file permissions more flexible.
From: Srinivas Kandagatla @ 2015-08-11 11:02 UTC (permalink / raw)
To: Greg Kroah-Hartman
Cc: Andrew Lunn, wxt, linux-api, Rob Herring, sboyd, arnd,
linux-arm-kernel, khilman, s.hauer, linux-kernel, linux-arm-msm,
mporter, pantelis.antoniou, devicetree, Mark Brown,
maitysanchayan, p.zabel, Srinivas Kandagatla
Hi All,
Recent discussion raised by Andrew Lunn and others regarding default permission
set on the nvmem binary sysfs file resulted in this small fixup patchset.
By default nvmem core sets the readonly permission to everyone (S_IRUGO), this
is not desirable by many providers as they would not want everyone to view things
like passwords stored in the nvmem.
This patchset fixes this by making the default as root-only and then the
providers could supply with additional permissions if required. One of
the patch in this set also sets correct size for the binary file too,
so that the user would not even attempt to read past the size.
I have tested this on IFC6410 with qfprom.
Thanks,
srini
Srinivas Kandagatla (3):
nvmem: make default user binary file root-access only
nvmem: set the size for the nvmem binary file.
nvmem: add permission flags in nvmem_config
drivers/nvmem/core.c | 55 +++++++++---------------------------------
include/linux/nvmem-provider.h | 1 +
2 files changed, 13 insertions(+), 43 deletions(-)
--
1.9.1
^ permalink raw reply
* [PATCH RFC 1/3] nvmem: make default user binary file root-access only
From: Srinivas Kandagatla @ 2015-08-11 11:03 UTC (permalink / raw)
To: Greg Kroah-Hartman
Cc: Andrew Lunn, wxt, linux-api, Rob Herring, sboyd, arnd,
linux-arm-kernel, khilman, s.hauer, linux-kernel, linux-arm-msm,
mporter, pantelis.antoniou, devicetree, Mark Brown,
maitysanchayan, p.zabel, Srinivas Kandagatla
In-Reply-To: <1439290931-18232-1-git-send-email-srinivas.kandagatla@linaro.org>
As required by many providers like at24/at25/mxs-ocotp/qfprom and may be
other providers would want to allow root-only to read the nvmem content.
So making the defaults to be root-only access would address the request
and also provide flexibility to providers to specify there own permissions
on top of the root-only using the perm flag in nvmem_config.
Making this dynamic did cut down lot of static binary attributes in the
code.
Signed-off-by: Srinivas Kandagatla <srinivas.kandagatla@linaro.org>
---
drivers/nvmem/core.c | 53 ++++++++++------------------------------------------
1 file changed, 10 insertions(+), 43 deletions(-)
diff --git a/drivers/nvmem/core.c b/drivers/nvmem/core.c
index 35a42bb..103f6aa 100644
--- a/drivers/nvmem/core.c
+++ b/drivers/nvmem/core.c
@@ -31,6 +31,7 @@ struct nvmem_device {
struct regmap *regmap;
struct module *owner;
struct device dev;
+ struct bin_attribute bin;
int stride;
int word_size;
int ncells;
@@ -109,52 +110,15 @@ static ssize_t bin_attr_nvmem_write(struct file *filp, struct kobject *kobj,
}
/* default read/write permissions */
-static struct bin_attribute bin_attr_rw_nvmem = {
+static struct bin_attribute bin_attr_template = {
.attr = {
.name = "nvmem",
- .mode = S_IWUSR | S_IRUGO,
+ .mode = S_IRUSR,
},
.read = bin_attr_nvmem_read,
.write = bin_attr_nvmem_write,
};
-static struct bin_attribute *nvmem_bin_rw_attributes[] = {
- &bin_attr_rw_nvmem,
- NULL,
-};
-
-static const struct attribute_group nvmem_bin_rw_group = {
- .bin_attrs = nvmem_bin_rw_attributes,
-};
-
-static const struct attribute_group *nvmem_rw_dev_groups[] = {
- &nvmem_bin_rw_group,
- NULL,
-};
-
-/* read only permission */
-static struct bin_attribute bin_attr_ro_nvmem = {
- .attr = {
- .name = "nvmem",
- .mode = S_IRUGO,
- },
- .read = bin_attr_nvmem_read,
-};
-
-static struct bin_attribute *nvmem_bin_ro_attributes[] = {
- &bin_attr_ro_nvmem,
- NULL,
-};
-
-static const struct attribute_group nvmem_bin_ro_group = {
- .bin_attrs = nvmem_bin_ro_attributes,
-};
-
-static const struct attribute_group *nvmem_ro_dev_groups[] = {
- &nvmem_bin_ro_group,
- NULL,
-};
-
static void nvmem_release(struct device *dev)
{
struct nvmem_device *nvmem = to_nvmem_device(dev);
@@ -346,10 +310,8 @@ struct nvmem_device *nvmem_register(const struct nvmem_config *config)
nvmem->read_only = of_property_read_bool(np, "read-only") |
config->read_only;
-
- nvmem->dev.groups = nvmem->read_only ? nvmem_ro_dev_groups :
- nvmem_rw_dev_groups;
-
+ nvmem->bin = bin_attr_template;
+ nvmem->bin.attr.mode = nvmem->read_only ? : (S_IRUSR | S_IWUSR);
device_initialize(&nvmem->dev);
dev_dbg(&nvmem->dev, "Registering nvmem device %s\n", config->name);
@@ -361,6 +323,10 @@ struct nvmem_device *nvmem_register(const struct nvmem_config *config)
return ERR_PTR(rval);
}
+ rval = device_create_bin_file(&nvmem->dev, &nvmem->bin);
+ if (rval)
+ dev_err(&nvmem->dev, "Failed to create nvmem binary file\n");
+
if (config->cells)
nvmem_add_cells(nvmem, config);
@@ -385,6 +351,7 @@ int nvmem_unregister(struct nvmem_device *nvmem)
mutex_unlock(&nvmem_mutex);
nvmem_device_remove_all_cells(nvmem);
+ device_remove_bin_file(&nvmem->dev, &nvmem->bin);
device_del(&nvmem->dev);
return 0;
--
1.9.1
^ permalink raw reply related
* [PATCH RFC 2/3] nvmem: set the size for the nvmem binary file.
From: Srinivas Kandagatla @ 2015-08-11 11:03 UTC (permalink / raw)
To: Greg Kroah-Hartman
Cc: Andrew Lunn, wxt, linux-api, Rob Herring, sboyd, arnd,
linux-arm-kernel, khilman, s.hauer, linux-kernel, linux-arm-msm,
mporter, pantelis.antoniou, devicetree, Mark Brown,
maitysanchayan, p.zabel, Srinivas Kandagatla
In-Reply-To: <1439290931-18232-1-git-send-email-srinivas.kandagatla@linaro.org>
This patch sets the actual size of binary file to the nvmem size.
Previously this was not possible as the core was using the static global
data structures for attributes.
Signed-off-by: Srinivas Kandagatla <srinivas.kandagatla@linaro.org>
---
drivers/nvmem/core.c | 1 +
1 file changed, 1 insertion(+)
diff --git a/drivers/nvmem/core.c b/drivers/nvmem/core.c
index 103f6aa..414ed23 100644
--- a/drivers/nvmem/core.c
+++ b/drivers/nvmem/core.c
@@ -312,6 +312,7 @@ struct nvmem_device *nvmem_register(const struct nvmem_config *config)
config->read_only;
nvmem->bin = bin_attr_template;
nvmem->bin.attr.mode = nvmem->read_only ? : (S_IRUSR | S_IWUSR);
+ nvmem->bin.size = nvmem->size;
device_initialize(&nvmem->dev);
dev_dbg(&nvmem->dev, "Registering nvmem device %s\n", config->name);
--
1.9.1
^ permalink raw reply related
* [PATCH RFC 3/3] nvmem: add permission flags in nvmem_config
From: Srinivas Kandagatla @ 2015-08-11 11:03 UTC (permalink / raw)
To: Greg Kroah-Hartman
Cc: Andrew Lunn, arnd, khilman, devicetree, linux-api, s.hauer, sboyd,
linux-kernel, maitysanchayan, pantelis.antoniou, Rob Herring,
Srinivas Kandagatla, Mark Brown, p.zabel, linux-arm-msm, mporter,
linux-arm-kernel, wxt
In-Reply-To: <1439290931-18232-1-git-send-email-srinivas.kandagatla@linaro.org>
This patch adds perm variable to nvmem_config structure which will allow
providers to specify the permissions required for the sysfs binary file.
This permission is applied on top of root-only access permissions set by
the core.
Signed-off-by: Srinivas Kandagatla <srinivas.kandagatla@linaro.org>
---
drivers/nvmem/core.c | 1 +
include/linux/nvmem-provider.h | 1 +
2 files changed, 2 insertions(+)
diff --git a/drivers/nvmem/core.c b/drivers/nvmem/core.c
index 414ed23..7481387 100644
--- a/drivers/nvmem/core.c
+++ b/drivers/nvmem/core.c
@@ -312,6 +312,7 @@ struct nvmem_device *nvmem_register(const struct nvmem_config *config)
config->read_only;
nvmem->bin = bin_attr_template;
nvmem->bin.attr.mode = nvmem->read_only ? : (S_IRUSR | S_IWUSR);
+ nvmem->bin.attr.mode |= config->perm;
nvmem->bin.size = nvmem->size;
device_initialize(&nvmem->dev);
diff --git a/include/linux/nvmem-provider.h b/include/linux/nvmem-provider.h
index 0b68caf..de9a61f 100644
--- a/include/linux/nvmem-provider.h
+++ b/include/linux/nvmem-provider.h
@@ -23,6 +23,7 @@ struct nvmem_config {
const struct nvmem_cell_info *cells;
int ncells;
bool read_only;
+ u16 perm; /* visibility in sysfs */
};
#if IS_ENABLED(CONFIG_NVMEM)
--
1.9.1
^ permalink raw reply related
* Re: [Qemu-devel] [PATCH 19/23] userfaultfd: activate syscall
From: Andrea Arcangeli @ 2015-08-11 13:48 UTC (permalink / raw)
To: Bharata B Rao
Cc: Andrew Morton, linux-kernel, linux-mm, qemu-devel, kvm, linux-api,
zhang.zhanghailiang, Pavel Emelyanov, Johannes Weiner,
Hugh Dickins, Dr. David Alan Gilbert, Sanidhya Kashyap,
Dave Hansen, Andres Lagar-Cavilla, Mel Gorman, Paolo Bonzini,
Kirill A. Shutemov, Huangpeng (Peter), Andy Lutomirski,
Linus Torvalds, Peter Feiner
In-Reply-To: <20150811100728.GB4587@in.ibm.com>
Hello Bharata,
On Tue, Aug 11, 2015 at 03:37:29PM +0530, Bharata B Rao wrote:
> May be it is a bit late to bring this up, but I needed the following fix
> to userfault21 branch of your git tree to compile on powerpc.
Not late, just in time. I increased the number of syscalls in earlier
versions, it must have gotten lost during a rejecting rebase, sorry.
I applied it to my tree and it can be applied to -mm and linux-next,
thanks!
The syscall for arm32 are also ready and on their way to the arm tree,
the testsuite worked fine there. ppc also should work fine if you
could confirm it'd be interesting, just beware that I got a typo in
the testcase:
diff --git a/tools/testing/selftests/vm/userfaultfd.c b/tools/testing/selftests/vm/userfaultfd.c
index 76071b1..925c3c9 100644
--- a/tools/testing/selftests/vm/userfaultfd.c
+++ b/tools/testing/selftests/vm/userfaultfd.c
@@ -70,7 +70,7 @@
#define __NR_userfaultfd 323
#elif defined(__i386__)
#define __NR_userfaultfd 374
-#elif defined(__powewrpc__)
+#elif defined(__powerpc__)
#define __NR_userfaultfd 364
#else
#error "missing __NR_userfaultfd definition"
> ----
>
> powerpc: Bump up __NR_syscalls to account for __NR_userfaultfd
>
> From: Bharata B Rao <bharata@linux.vnet.ibm.com>
>
> With userfaultfd syscall, the number of syscalls will be 365 on PowerPC.
> Reflect the same in __NR_syscalls.
>
> Signed-off-by: Bharata B Rao <bharata@linux.vnet.ibm.com>
> ---
> arch/powerpc/include/asm/unistd.h | 2 +-
> 1 file changed, 1 insertion(+), 1 deletion(-)
>
> diff --git a/arch/powerpc/include/asm/unistd.h b/arch/powerpc/include/asm/unistd.h
> index f4f8b66..4a055b6 100644
> --- a/arch/powerpc/include/asm/unistd.h
> +++ b/arch/powerpc/include/asm/unistd.h
> @@ -12,7 +12,7 @@
> #include <uapi/asm/unistd.h>
>
>
> -#define __NR_syscalls 364
> +#define __NR_syscalls 365
>
> #define __NR__exit __NR_exit
> #define NR_syscalls __NR_syscalls
Reviewed-by: Andrea Arcangeli <aarcange@redhat.com>
--
To unsubscribe, send a message with 'unsubscribe linux-mm' in
the body to majordomo@kvack.org. For more info on Linux MM,
see: http://www.linux-mm.org/ .
Don't email: <a href=mailto:"dont@kvack.org"> email@kvack.org </a>
^ permalink raw reply related
* Re: [Xen-devel] [PATCH v3 0/9] Use correctly the Xen memory terminologies
From: David Vrabel @ 2015-08-11 17:37 UTC (permalink / raw)
To: Julien Grall, xen-devel
Cc: linux-fbdev, x86, netdev, H. Peter Anvin, Jiri Slaby,
Thomas Gleixner, Russell King, linux-scsi, Tomi Valkeinen,
stefano.stabellini, Ingo Molnar, linux-input,
Jean-Christophe Plagniol-Villard, ian.campbell,
James E.J. Bottomley, Boris Ostrovsky, linux-arm-kernel,
Juergen Gross, Wei Liu, Greg Kroah-Hartman, Dmitry Torokhov,
linux-kernel, David Vrabel, linux-api, linuxppc-d
In-Reply-To: <1438965282-19103-1-git-send-email-julien.grall@citrix.com>
On 07/08/15 17:34, Julien Grall wrote:
> Hi all,
>
> This patch series aims to use the memory terminologies described in
> include/xen/mm.h [1] for Linux xen code.
Applied to for-linus-4.3, thanks.
David
^ permalink raw reply
* Re: [CFT][PATCH 09/10] sysfs: Create mountpoints with sysfs_create_empty_dir
From: Tejun Heo @ 2015-08-11 18:44 UTC (permalink / raw)
To: Eric W. Biederman
Cc: Linux Containers, linux-fsdevel-u79uwXL29TY76Z2rM5mHXA, Linux API,
Serge E. Hallyn, Andy Lutomirski, Richard Weinberger,
Kenton Varda, Michael Kerrisk-manpages, Stéphane Graber,
Eric Windisch, Greg Kroah-Hartman
In-Reply-To: <878ucrhxi9.fsf-JOvCrm2gF+uungPnsOpG7nhyD016LWXt@public.gmane.org>
On Thu, May 14, 2015 at 12:36:30PM -0500, Eric W. Biederman wrote:
>
> This allows for better documentation in the code and
> it allows for a simpler and fully correct version of
> fs_fully_visible to be written.
>
> The mount points converted and their filesystems are:
> /sys/hypervisor/s390/ s390_hypfs
> /sys/kernel/config/ configfs
> /sys/kernel/debug/ debugfs
> /sys/firmware/efi/efivars/ efivarfs
> /sys/fs/fuse/connections/ fusectl
> /sys/fs/pstore/ pstore
> /sys/kernel/tracing/ tracefs
> /sys/fs/cgroup/ cgroup
> /sys/kernel/security/ securityfs
> /sys/fs/selinux/ selinuxfs
> /sys/fs/smackfs/ smackfs
>
> Cc: stable-u79uwXL29TY76Z2rM5mHXA@public.gmane.org
> Signed-off-by: "Eric W. Biederman" <ebiederm-aS9lmoZGLiVWk0Htik3J/w@public.gmane.org>
So, this somehow ends up confusing upstart on centos6 based systems
making it fail to mount tmpfs on /sys/fs/cgroup. It also skips sunrpc
and other mounts are different too. No idea why at this point. Can
we please revert this from -stable until we know what's going on?
Thanks.
--
tejun
^ permalink raw reply
* [PATCH v2 0/3] SysFS driver for QEMU fw_cfg device
From: Gabriel L. Somlo @ 2015-08-11 18:44 UTC (permalink / raw)
To: gregkh, ralf, zajec5, paul, galak, linux-api, linux-kernel
Cc: linux-efi, rjones, jordan.l.justen, x86, qemu-devel, gleb,
matt.fleming, kernelnewbies, kraxel, pbonzini, lersek
From: "Gabriel Somlo" <somlo@cmu.edu>
This patch set makes QEMU fw_cfg blobs available for viewing (read-only)
via SysFS.
New since v1:
1/3:
- renamed sysfs path components:
s/fw_cfg/qemu_fw_cfg/g, at Greg's suggestion
s/by_select/by_key/g since it feels a bit more intuitive
- attribute "select" renamed to "key", along the same train of
thought.
- renamed actual source file from "fw_cfg.c" to "qemu_fw_cfg.c"
and doc file in Documentation/ABI/testing to
"sysfs-firmware-qemu_fw_cfg"
- s/uintXX_t/uXX/g, at Greg's suggestion
- using named identifiers to initialize access mode table
(static struct fw_cfg_access fw_cfg_modes[]) per Greg's
suggestion
- removed redundant "capable(CAP_SYS_ADMIN)" checks (per Greg's
feedback)
2/3:
- EXPORT_SYMBOL(kset_find_obj) statement moved immediately
after kset_find_obj() definition.
Re. 'struct device' vs. straight-up kobject:
I dug around a bit, and found a bunch of stuff under /sys/devices
(such as e.g. /sys/devices/virtual/dmi and /sys/devices/platform/applesmc)
The first one is particularly interesting, as it presents a
somewhat different view of the same internal information displayed by
/sys/firmware/dmi/, only /sys/devices/virtual/dmi looks more "cooked",
in that it exposes individual fields of the several different DMI tables.
None of the subfolders of /sys/devices/... appears to facilitate access
to the "raw blobs" contained by the respective devices represented there.
Even applesmc (the other device with which I'm somewhat familiar) does
a lot of "cooking" of the values held by the device.
So, at the end of the day, I want the information I expose about fw_cfg
to look more like /sys/firmware/dmi than like /sys/devices/virtual/dmi
in that I have a bunch of cached metadata attributes for each of the
blobs, and want to offer access to the raw blob data, and there's no
rhyme or reason to the actual internals of any of the blobs, so offering
a "cooked" view like in /sys/devices/... doesn't feel like the best fit...
Obviously, I may be totally wrong... :)
Lastly, re. Greg's comment on patch 3/3:
> Shouldn't all of this be done in userspace with the symlinks and all?
> It seems like you are trying to duplicate the /dev/block/by-name and
> such. Policy decisions like symlinks and naming should be done there,
> in userspace, and not directly in sysfs if at all possible.
OK, so here's the "pseudo-bash" equivalent of what this patch does:
#upon successful qemu_fw_cfg.ko insertion
BY_KEY="/sys/firmware/qemu_fw_cfg/by_key"
BY_NAME="/sys/firmware/qemu_fw_cfg/by_name"
mkdir -p $BY_NAME
for KEY in $(ls $BY_KEY); do
FILE=$(cat $BY_KEY/$KEY/name)
[ mkdir -p $BY_NAME/$(dirname $FILE) ] || continue
pushd $BY_NAME/$(dirname $FILE)
ln -s $BY_KEY/$KEY $(basename $FILE)
popd
done
Note how it MAY fail to mkdir, in which case it skips to the next key,
or it MAY fail to create the symlink, in which case it just moves on
to the next key as well.
Again, the fw_cfg device doesn't have any notion of nested directories,
it just parrots back some random string someone choose as a name for a
given blob, but these strings typically contain slash-separated ASCII
which "looks like" it "could" be a path name :)
If there's a reasonable way to do this dynamically, immediately upon
insertion of the qemu_fw_cfg.ko module, patches 2/3 and 3/3 could be
dropped.
But before that, I'd appreciate a pointer to how/where this can be done
in userspace. Would it be available e.g. during early boot, early enough
to e.g. raw-dump an ASCII blob containing shell variable definitions (e.g.
to set hostname on the guest, etc ?)
Doing it as part of the qemu_fw_cfg.ko module itself has the advantage
of it being available immediately, without anything having to "watch"
for the module being inserted.
But if userspace is the standard, "canonical" place to do something like
this, I'd be happy to give it a try, once I find the right tree to bark
up on... :)
Thanks much,
Gabriel
Old "cover-letter" blurb for v1:
> Several different architectures supported by QEMU are set up with a
> "firmware configuration" (fw_cfg) device, used to pass configuration
> "blobs" into the guest by the host running QEMU.
>
> Historically, these config blobs were mostly of interest to the guest
> BIOS, but since QEMU v2.4 it is possible to insert arbitrary blobs via
> the command line, which makes them potentially interesting to userspace
> (e.g. for passing early boot environment variables, etc.).
>
> In addition to cc-ing the people and lists indicated by get-maintainer.pl,
> I've added a few extra lists suggested by Matt Fleming on the qemu-devel
> list, as well as the qemu-devel list itself.
>
> Also cc-ing kernelnewbies, as this is my very first kenel contribution,
> so please go easy on me for whatever silly n00b mistakes I might have still
> missed, in spite of trying hard to do all my homework properly... :)
>
> The series consists of three patches:
>
> 1/3 - probes for the qemu fw_cfg device in locations known to work on
> the supported architectures, in decreasing order of "likelihood".
>
> While it *may* be possible to detect the presence of fw_cfg via
> acpi or dtb (on x86 and arm, respectively), there's no way I know
> of attempting that on sun4 and ppc/mac, so I've stuck with simply
> probing (the fw_cfg_modes[] structure and fw_cfg_io_probe() function)
> in fw_cfg.c. I could use some advice on how else that could be
> done more elegantly, if needed.
>
> Upon successfully detecting a present fw_cfg device, we set up
> /sys/firmware/fw_cfg/by_select entries for each blob available
> on the fw_cfg device.
>
> 2/3 - export kset_find_obj() (in lib/kobject.c) for use with modules,
> which will come in handy in patch #3, below.
>
> 3/3 - add a "user friendly" way of listing fw_cfg blobs by name rather
> than by unique selector key.
>
> Since fw_cfg blob names are traditionally set up to look like
> path names, it would be nice to mirror that fact when displaying
> them under /sys/firmware/fw_cfg in a "by_name" subdirectory.
>
> I'm using ksets to reflect subdirectories matching "dirname"
> tokens separated by '/' within each fw_cfg blob "filename",
> and symlinks into the "by_select" subdirectory for each "basename".
>
> Since the fw_cfg device doesn't enforce that blob names have
> well-behaved and non-conflicting names, it is possible (though
> unlikely) that there will be blobs named:
>
> "etc/foo/bar"
> and
> "etc/foo"
>
> where "foo" will try to be both a subdirectory AND a symlink under
> /sys/firmware/fw_cfg/by_name/etc/. In such an event, the latter
> fw_cfg blob will simply be skipped from having an entry created
> under the user-friendly "by_name" subdirectory, remaining listed
> only as an entry under the "by_select" subdirectory.
>
> I have tested this on x86_64 and armv7hl+lpae kernels. Builds and installs
> OK as both a module and linked directly into the kernel.
Gabriel Somlo (3):
firmware: introduce sysfs driver for QEMU's fw_cfg device
kobject: export kset_find_obj() to be used from modules
firmware: fw_cfg: create directory hierarchy for fw_cfg file names
.../ABI/testing/sysfs-firmware-qemu_fw_cfg | 210 ++++++++
drivers/firmware/Kconfig | 10 +
drivers/firmware/Makefile | 1 +
drivers/firmware/qemu_fw_cfg.c | 560 +++++++++++++++++++++
lib/kobject.c | 1 +
5 files changed, 782 insertions(+)
create mode 100644 Documentation/ABI/testing/sysfs-firmware-qemu_fw_cfg
create mode 100644 drivers/firmware/qemu_fw_cfg.c
--
2.4.3
^ permalink raw reply
page: next (older) | prev (newer) | latest
- recent:[subjects (threaded)|topics (new)|topics (active)]
This is a public inbox, see mirroring instructions
for how to clone and mirror all data and code used for this inbox