2018-02-14 14:23:16

by Marc-André Lureau

Subject: [PATCH v14 0/9] fw_cfg: add DMA operations & etc/vmcoreinfo support

Hi,

This series adds DMA operations support to the qemu fw_cfg kernel
module and populates "etc/vmcoreinfo" with vmcoreinfo location
details (entry added since qemu 2.11 with -device vmcoreinfo).

v14:
- add "fw_cfg: add a public uapi header"
- fix sparse warnings & don't introduce new warnings
- add memory barriers to force IO ordering
- split fw_cfg_read_blob() in fw_cfg_read_blob_io() and
fw_cfg_read_blob_dma()
- add error handling to fw_cfg_read_blob() callers
- minor stylistic changes

v13:
- reorder patch series, introduce DMA write before DMA read
- do some measurements of DMA read speed-ups

v12:
- fix virt_to_phys(NULL) panic with CONFIG_DEBUG_VIRTUAL=y
- do not use DMA read, except for kmalloc() memory we allocated
ourself (only fw_cfg_register_dir_entries() so far)

v11:
- add #include <linux/crash_core.h> in last patch,
fixing kbuild .config test

Marc-André Lureau (9):
crash: export paddr_vmcoreinfo_note()
fw_cfg: add a public uapi header
fw_cfg: fix sparse warnings in fw_cfg_sel_endianness()
fw_cfg: fix sparse warnings with fw_cfg_file
fw_cfg: fix sparse warning reading FW_CFG_ID
fw_cfg: fix sparse warnings around FW_CFG_FILE_DIR read
fw_cfg: add DMA register
fw_cfg: write vmcoreinfo details
RFC: fw_cfg: do DMA read operation

MAINTAINERS | 1 +
drivers/firmware/qemu_fw_cfg.c | 333 +++++++++++++++++++++++++++++++++--------
include/uapi/linux/fw_cfg.h | 102 +++++++++++++
kernel/crash_core.c | 1 +
4 files changed, 374 insertions(+), 63 deletions(-)
create mode 100644 include/uapi/linux/fw_cfg.h

--
2.16.1.73.g5832b7e9f2

2018-02-14 14:20:20

by Marc-André Lureau

Subject: [PATCH v14 4/9] fw_cfg: fix sparse warnings with fw_cfg_file

Modify fw_cfg_sysfs_entry to store entry values, instead of reusing
the restricted types.

Fixes warnings such as:

$ make C=1 CF=-D__CHECK_ENDIAN__ drivers/firmware/qemu_fw_cfg.o

drivers/firmware/qemu_fw_cfg.c:491:29: warning: incorrect type in assignment (different base types)
drivers/firmware/qemu_fw_cfg.c:491:29: expected restricted __be32 [usertype] size
drivers/firmware/qemu_fw_cfg.c:491:29: got unsigned int
drivers/firmware/qemu_fw_cfg.c:492:31: warning: incorrect type in assignment (different base types)
drivers/firmware/qemu_fw_cfg.c:492:31: expected restricted __be16 [usertype] select
drivers/firmware/qemu_fw_cfg.c:492:31: got int

Signed-off-by: Marc-André Lureau <[email protected]>
---
drivers/firmware/qemu_fw_cfg.c | 28 +++++++++++++++-------------
1 file changed, 15 insertions(+), 13 deletions(-)

diff --git a/drivers/firmware/qemu_fw_cfg.c b/drivers/firmware/qemu_fw_cfg.c
index 85e693287d87..8ad19086e5c5 100644
--- a/drivers/firmware/qemu_fw_cfg.c
+++ b/drivers/firmware/qemu_fw_cfg.c
@@ -192,7 +192,9 @@ static const struct {
/* fw_cfg_sysfs_entry type */
struct fw_cfg_sysfs_entry {
struct kobject kobj;
- struct fw_cfg_file f;
+ u32 size;
+ u16 select;
+ char name[FW_CFG_MAX_FILE_PATH];
struct list_head list;
};

@@ -256,17 +258,17 @@ struct fw_cfg_sysfs_attribute fw_cfg_sysfs_attr_##_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);
+ return sprintf(buf, "%u\n", e->size);
}

static ssize_t fw_cfg_sysfs_show_key(struct fw_cfg_sysfs_entry *e, char *buf)
{
- return sprintf(buf, "%u\n", e->f.select);
+ return sprintf(buf, "%u\n", e->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);
+ return sprintf(buf, "%s\n", e->name);
}

static FW_CFG_SYSFS_ATTR(size);
@@ -317,13 +319,13 @@ static ssize_t fw_cfg_sysfs_read_raw(struct file *filp, struct kobject *kobj,
{
struct fw_cfg_sysfs_entry *entry = to_entry(kobj);

- if (pos > entry->f.size)
+ if (pos > entry->size)
return -EINVAL;

- if (count > entry->f.size - pos)
- count = entry->f.size - pos;
+ if (count > entry->size - pos)
+ count = entry->size - pos;

- fw_cfg_read_blob(entry->f.select, buf, pos, count);
+ fw_cfg_read_blob(entry->select, buf, pos, count);
return count;
}

@@ -442,11 +444,13 @@ static int fw_cfg_register_file(const struct fw_cfg_file *f)
return -ENOMEM;

/* set file entry information */
- memcpy(&entry->f, f, sizeof(struct fw_cfg_file));
+ entry->size = be32_to_cpu(f->size);
+ entry->select = be16_to_cpu(f->select);
+ memcpy(entry->name, f->name, FW_CFG_MAX_FILE_PATH);

/* register entry under "/sys/firmware/qemu_fw_cfg/by_key/" */
err = kobject_init_and_add(&entry->kobj, &fw_cfg_sysfs_entry_ktype,
- fw_cfg_sel_ko, "%d", entry->f.select);
+ fw_cfg_sel_ko, "%d", entry->select);
if (err)
goto err_register;

@@ -456,7 +460,7 @@ static int fw_cfg_register_file(const struct fw_cfg_file *f)
goto err_add_raw;

/* try adding "/sys/firmware/qemu_fw_cfg/by_name/" symlink */
- fw_cfg_build_symlink(fw_cfg_fname_kset, &entry->kobj, entry->f.name);
+ fw_cfg_build_symlink(fw_cfg_fname_kset, &entry->kobj, entry->name);

/* success, add entry to global cache */
fw_cfg_sysfs_cache_enlist(entry);
@@ -488,8 +492,6 @@ static int fw_cfg_register_dir_entries(void)
fw_cfg_read_blob(FW_CFG_FILE_DIR, dir, sizeof(count), dir_size);

for (i = 0; i < count; i++) {
- dir[i].size = be32_to_cpu(dir[i].size);
- dir[i].select = be16_to_cpu(dir[i].select);
ret = fw_cfg_register_file(&dir[i]);
if (ret)
break;
--
2.16.1.73.g5832b7e9f2

2018-02-14 14:20:37

by Marc-André Lureau

Subject: [PATCH v14 2/9] fw_cfg: add a public uapi header

Create a common header file for well-known values and structures to be
shared by the Linux kernel with qemu or other projects.

Suggested-by: Michael S. Tsirkin <[email protected]>
Signed-off-by: Marc-André Lureau <[email protected]>

---

The related qemu patch making use of it, to be submitted:
https://github.com/elmarco/qemu/commit/4884fc9e9c4c4467a371e5a40f3181239e1b70f5
---
MAINTAINERS | 1 +
drivers/firmware/qemu_fw_cfg.c | 22 +--------
include/uapi/linux/fw_cfg.h | 102 +++++++++++++++++++++++++++++++++++++++++
3 files changed, 105 insertions(+), 20 deletions(-)
create mode 100644 include/uapi/linux/fw_cfg.h

diff --git a/MAINTAINERS b/MAINTAINERS
index 3bdc260e36b7..a66b65f62811 100644
--- a/MAINTAINERS
+++ b/MAINTAINERS
@@ -11352,6 +11352,7 @@ M: "Michael S. Tsirkin" <[email protected]>
L: [email protected]
S: Maintained
F: drivers/firmware/qemu_fw_cfg.c
+F: include/uapi/linux/fw_cfg.h

QIB DRIVER
M: Dennis Dalessandro <[email protected]>
diff --git a/drivers/firmware/qemu_fw_cfg.c b/drivers/firmware/qemu_fw_cfg.c
index a41b572eeeb1..90f467232777 100644
--- a/drivers/firmware/qemu_fw_cfg.c
+++ b/drivers/firmware/qemu_fw_cfg.c
@@ -32,30 +32,12 @@
#include <linux/slab.h>
#include <linux/io.h>
#include <linux/ioport.h>
+#include <linux/fw_cfg.h>

MODULE_AUTHOR("Gabriel L. Somlo <[email protected]>");
MODULE_DESCRIPTION("QEMU fw_cfg sysfs support");
MODULE_LICENSE("GPL");

-/* selector key 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 {
- u32 size;
- u16 select;
- u16 reserved;
- char name[FW_CFG_MAX_FILE_PATH];
-};
-
/* fw_cfg device i/o register addresses */
static bool fw_cfg_is_mmio;
static phys_addr_t fw_cfg_p_base;
@@ -597,7 +579,7 @@ MODULE_DEVICE_TABLE(of, fw_cfg_sysfs_mmio_match);

#ifdef CONFIG_ACPI
static const struct acpi_device_id fw_cfg_sysfs_acpi_match[] = {
- { "QEMU0002", },
+ { FW_CFG_ACPI_DEVICE_ID, },
{},
};
MODULE_DEVICE_TABLE(acpi, fw_cfg_sysfs_acpi_match);
diff --git a/include/uapi/linux/fw_cfg.h b/include/uapi/linux/fw_cfg.h
new file mode 100644
index 000000000000..5b8136ce46ee
--- /dev/null
+++ b/include/uapi/linux/fw_cfg.h
@@ -0,0 +1,102 @@
+/* SPDX-License-Identifier: GPL-2.0 WITH Linux-syscall-note */
+#ifndef _LINUX_FW_CFG_H
+#define _LINUX_FW_CFG_H
+
+#include <linux/types.h>
+
+#define FW_CFG_ACPI_DEVICE_ID "QEMU0002"
+
+/* selector key values for "well-known" fw_cfg entries */
+#define FW_CFG_SIGNATURE 0x00
+#define FW_CFG_ID 0x01
+#define FW_CFG_UUID 0x02
+#define FW_CFG_RAM_SIZE 0x03
+#define FW_CFG_NOGRAPHIC 0x04
+#define FW_CFG_NB_CPUS 0x05
+#define FW_CFG_MACHINE_ID 0x06
+#define FW_CFG_KERNEL_ADDR 0x07
+#define FW_CFG_KERNEL_SIZE 0x08
+#define FW_CFG_KERNEL_CMDLINE 0x09
+#define FW_CFG_INITRD_ADDR 0x0a
+#define FW_CFG_INITRD_SIZE 0x0b
+#define FW_CFG_BOOT_DEVICE 0x0c
+#define FW_CFG_NUMA 0x0d
+#define FW_CFG_BOOT_MENU 0x0e
+#define FW_CFG_MAX_CPUS 0x0f
+#define FW_CFG_KERNEL_ENTRY 0x10
+#define FW_CFG_KERNEL_DATA 0x11
+#define FW_CFG_INITRD_DATA 0x12
+#define FW_CFG_CMDLINE_ADDR 0x13
+#define FW_CFG_CMDLINE_SIZE 0x14
+#define FW_CFG_CMDLINE_DATA 0x15
+#define FW_CFG_SETUP_ADDR 0x16
+#define FW_CFG_SETUP_SIZE 0x17
+#define FW_CFG_SETUP_DATA 0x18
+#define FW_CFG_FILE_DIR 0x19
+
+#define FW_CFG_FILE_FIRST 0x20
+#define FW_CFG_FILE_SLOTS_MIN 0x10
+
+#define FW_CFG_WRITE_CHANNEL 0x4000
+#define FW_CFG_ARCH_LOCAL 0x8000
+#define FW_CFG_ENTRY_MASK (~(FW_CFG_WRITE_CHANNEL | FW_CFG_ARCH_LOCAL))
+
+#define FW_CFG_INVALID 0xffff
+
+/* width in bytes of fw_cfg control register */
+#define FW_CFG_CTL_SIZE 0x02
+
+/* fw_cfg "file name" is up to 56 characters (including terminating nul) */
+#define FW_CFG_MAX_FILE_PATH 56
+
+/* size in bytes of fw_cfg signature */
+#define FW_CFG_SIG_SIZE 4
+
+/* FW_CFG_ID bits */
+#define FW_CFG_VERSION 0x01
+#define FW_CFG_VERSION_DMA 0x02
+
+/* fw_cfg file directory entry type */
+struct fw_cfg_file {
+ __be32 size; /* file size */
+ __be16 select; /* write this to 0x510 to read it */
+ __u16 reserved;
+ char name[FW_CFG_MAX_FILE_PATH];
+};
+
+struct fw_cfg_files {
+ __be32 count; /* number of entries */
+ struct fw_cfg_file f[];
+};
+
+/* FW_CFG_DMA_CONTROL bits */
+#define FW_CFG_DMA_CTL_ERROR 0x01
+#define FW_CFG_DMA_CTL_READ 0x02
+#define FW_CFG_DMA_CTL_SKIP 0x04
+#define FW_CFG_DMA_CTL_SELECT 0x08
+#define FW_CFG_DMA_CTL_WRITE 0x10
+
+#define FW_CFG_DMA_SIGNATURE 0x51454d5520434647ULL /* "QEMU CFG" */
+
+/* Control as first field allows for different structures selected by this
+ * field, which might be useful in the future
+ */
+struct fw_cfg_dma_access {
+ __be32 control;
+ __be32 length;
+ __be64 address;
+};
+
+#define FW_CFG_VMCOREINFO_FILENAME "etc/vmcoreinfo"
+
+#define FW_CFG_VMCOREINFO_FORMAT_NONE 0x0
+#define FW_CFG_VMCOREINFO_FORMAT_ELF 0x1
+
+struct fw_cfg_vmcoreinfo {
+ __le16 host_format;
+ __le16 guest_format;
+ __le32 size;
+ __le64 paddr;
+};
+
+#endif
--
2.16.1.73.g5832b7e9f2

2018-02-14 14:21:18

by Marc-André Lureau

Subject: [PATCH v14 7/9] fw_cfg: add DMA register

Add an optional <dma_off> kernel module (or command line) parameter
using the following syntax:

[qemu_fw_cfg.]ioport=<size>@<base>[:<ctrl_off>:<data_off>[:<dma_off>]]
or
[qemu_fw_cfg.]mmio=<size>@<base>[:<ctrl_off>:<data_off>[:<dma_off>]]

and initializes the register address using given or default offset.

Signed-off-by: Marc-André Lureau <[email protected]>
Reviewed-by: Gabriel Somlo <[email protected]>
---
drivers/firmware/qemu_fw_cfg.c | 53 ++++++++++++++++++++++++++++++++----------
1 file changed, 41 insertions(+), 12 deletions(-)

diff --git a/drivers/firmware/qemu_fw_cfg.c b/drivers/firmware/qemu_fw_cfg.c
index c4c726841ba7..37638b95cb45 100644
--- a/drivers/firmware/qemu_fw_cfg.c
+++ b/drivers/firmware/qemu_fw_cfg.c
@@ -10,20 +10,21 @@
* and select subsets of aarch64), a Device Tree node (on arm), or using
* a kernel module (or command line) parameter with the following syntax:
*
- * [qemu_fw_cfg.]ioport=<size>@<base>[:<ctrl_off>:<data_off>]
+ * [qemu_fw_cfg.]ioport=<size>@<base>[:<ctrl_off>:<data_off>[:<dma_off>]]
* or
- * [qemu_fw_cfg.]mmio=<size>@<base>[:<ctrl_off>:<data_off>]
+ * [qemu_fw_cfg.]mmio=<size>@<base>[:<ctrl_off>:<data_off>[:<dma_off>]]
*
* where:
* <size> := size of ioport or mmio range
* <base> := physical base address of ioport or mmio range
* <ctrl_off> := (optional) offset of control register
* <data_off> := (optional) offset of data register
+ * <dma_off> := (optional) offset of dma register
*
* e.g.:
- * qemu_fw_cfg.ioport=2@0x510:0:1 (the default on x86)
+ * qemu_fw_cfg.ioport=12@0x510:0:1:4 (the default on x86)
* or
- * qemu_fw_cfg.mmio=0xA@0x9020000:8:0 (the default on arm)
+ * qemu_fw_cfg.mmio=16@0x9020000:8:0:16 (the default on arm)
*/

#include <linux/module.h>
@@ -45,6 +46,7 @@ static resource_size_t fw_cfg_p_size;
static void __iomem *fw_cfg_dev_base;
static void __iomem *fw_cfg_reg_ctrl;
static void __iomem *fw_cfg_reg_data;
+static void __iomem *fw_cfg_reg_dma;

/* atomic access to fw_cfg device (potentially slow i/o, so using mutex) */
static DEFINE_MUTEX(fw_cfg_dev_lock);
@@ -102,12 +104,14 @@ static void fw_cfg_io_cleanup(void)
# if (defined(CONFIG_ARM) || defined(CONFIG_ARM64))
# define FW_CFG_CTRL_OFF 0x08
# define FW_CFG_DATA_OFF 0x00
+# define FW_CFG_DMA_OFF 0x10
# elif (defined(CONFIG_PPC_PMAC) || defined(CONFIG_SPARC32)) /* ppc/mac,sun4m */
# define FW_CFG_CTRL_OFF 0x00
# define FW_CFG_DATA_OFF 0x02
# elif (defined(CONFIG_X86) || defined(CONFIG_SPARC64)) /* x86, sun4u */
# define FW_CFG_CTRL_OFF 0x00
# define FW_CFG_DATA_OFF 0x01
+# define FW_CFG_DMA_OFF 0x04
# else
# error "QEMU FW_CFG not available on this architecture!"
# endif
@@ -117,7 +121,7 @@ static void fw_cfg_io_cleanup(void)
static int fw_cfg_do_platform_probe(struct platform_device *pdev)
{
char sig[FW_CFG_SIG_SIZE];
- struct resource *range, *ctrl, *data;
+ struct resource *range, *ctrl, *data, *dma;

/* acquire i/o range details */
fw_cfg_is_mmio = false;
@@ -154,6 +158,7 @@ static int fw_cfg_do_platform_probe(struct platform_device *pdev)
/* were custom register offsets provided (e.g. on the command line)? */
ctrl = platform_get_resource_byname(pdev, IORESOURCE_REG, "ctrl");
data = platform_get_resource_byname(pdev, IORESOURCE_REG, "data");
+ dma = platform_get_resource_byname(pdev, IORESOURCE_REG, "dma");
if (ctrl && data) {
fw_cfg_reg_ctrl = fw_cfg_dev_base + ctrl->start;
fw_cfg_reg_data = fw_cfg_dev_base + data->start;
@@ -163,6 +168,13 @@ static int fw_cfg_do_platform_probe(struct platform_device *pdev)
fw_cfg_reg_data = fw_cfg_dev_base + FW_CFG_DATA_OFF;
}

+ if (dma)
+ fw_cfg_reg_dma = fw_cfg_dev_base + dma->start;
+#ifdef FW_CFG_DMA_OFF
+ else
+ fw_cfg_reg_dma = fw_cfg_dev_base + FW_CFG_DMA_OFF;
+#endif
+
/* 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) {
@@ -617,6 +629,7 @@ static struct platform_device *fw_cfg_cmdline_dev;
/* use special scanf/printf modifier for phys_addr_t, resource_size_t */
#define PH_ADDR_SCAN_FMT "@%" __PHYS_ADDR_PREFIX "i%n" \
":%" __PHYS_ADDR_PREFIX "i" \
+ ":%" __PHYS_ADDR_PREFIX "i%n" \
":%" __PHYS_ADDR_PREFIX "i%n"

#define PH_ADDR_PR_1_FMT "0x%" __PHYS_ADDR_PREFIX "x@" \
@@ -626,12 +639,15 @@ static struct platform_device *fw_cfg_cmdline_dev;
":%" __PHYS_ADDR_PREFIX "u" \
":%" __PHYS_ADDR_PREFIX "u"

+#define PH_ADDR_PR_4_FMT PH_ADDR_PR_3_FMT \
+ ":%" __PHYS_ADDR_PREFIX "u"
+
static int fw_cfg_cmdline_set(const char *arg, const struct kernel_param *kp)
{
- struct resource res[3] = {};
+ struct resource res[4] = {};
char *str;
phys_addr_t base;
- resource_size_t size, ctrl_off, data_off;
+ resource_size_t size, ctrl_off, data_off, dma_off;
int processed, consumed = 0;

/* only one fw_cfg device can exist system-wide, so if one
@@ -647,19 +663,20 @@ static int fw_cfg_cmdline_set(const char *arg, const struct kernel_param *kp)
/* consume "<size>" portion of command line argument */
size = memparse(arg, &str);

- /* get "@<base>[:<ctrl_off>:<data_off>]" chunks */
+ /* get "@<base>[:<ctrl_off>:<data_off>[:<dma_off>]]" chunks */
processed = sscanf(str, PH_ADDR_SCAN_FMT,
&base, &consumed,
- &ctrl_off, &data_off, &consumed);
+ &ctrl_off, &data_off, &consumed,
+ &dma_off, &consumed);

- /* sscanf() must process precisely 1 or 3 chunks:
+ /* sscanf() must process precisely 1, 3 or 4 chunks:
* <base> is mandatory, optionally followed by <ctrl_off>
- * and <data_off>;
+ * and <data_off>, and <dma_off>;
* there must be no extra characters after the last chunk,
* so str[consumed] must be '\0'.
*/
if (str[consumed] ||
- (processed != 1 && processed != 3))
+ (processed != 1 && processed != 3 && processed != 4))
return -EINVAL;

res[0].start = base;
@@ -676,6 +693,11 @@ static int fw_cfg_cmdline_set(const char *arg, const struct kernel_param *kp)
res[2].start = data_off;
res[2].flags = IORESOURCE_REG;
}
+ if (processed > 3) {
+ res[3].name = "dma";
+ res[3].start = dma_off;
+ res[3].flags = IORESOURCE_REG;
+ }

/* "processed" happens to nicely match the number of resources
* we need to pass in to this platform device.
@@ -708,6 +730,13 @@ static int fw_cfg_cmdline_get(char *buf, const struct kernel_param *kp)
fw_cfg_cmdline_dev->resource[0].start,
fw_cfg_cmdline_dev->resource[1].start,
fw_cfg_cmdline_dev->resource[2].start);
+ case 4:
+ return snprintf(buf, PAGE_SIZE, PH_ADDR_PR_4_FMT,
+ resource_size(&fw_cfg_cmdline_dev->resource[0]),
+ fw_cfg_cmdline_dev->resource[0].start,
+ fw_cfg_cmdline_dev->resource[1].start,
+ fw_cfg_cmdline_dev->resource[2].start,
+ fw_cfg_cmdline_dev->resource[3].start);
}

/* Should never get here */
--
2.16.1.73.g5832b7e9f2

2018-02-14 14:21:30

by Marc-André Lureau

Subject: [PATCH v14 9/9] RFC: fw_cfg: do DMA read operation

Modify fw_cfg_read_blob() to use DMA if the device supports it.
Return errors, because the operation may fail.

So far, only one call in fw_cfg_register_dir_entries() is using
kmalloc'ed buf and is thus clearly eligible to DMA read.

Initially, I didn't implement DMA read to speed up boot time, but as a
first step before introducing DMA write (since read operations were
already presents). Even more, I didn't realize fw-cfg entries were
being read by the kernel during boot by default. But actally fw-cfg
entries are being populated during module probe. I knew DMA improved a
lot bios boot time (the main reason the DMA interface was added
afaik). Let see the time it would take to read the whole ACPI
tables (128kb allocated)

# time cat /sys/firmware/qemu_fw_cfg/by_name/etc/acpi/tables/raw
- with DMA: sys 0m0.003s
- without DMA (-global fw_cfg.dma_enabled=off): sys 0m7.674s

FW_CFG_FILE_DIR (0x19) is the only "file" that is read during kernel
boot to populate sysfs qemu_fw_cfg directory, and it is quite
small (1-2kb). Since it does not expose itself, in order to measure
the time it takes to read such small file, I took a comparable sized
file of 2048 bytes and exposed it (-fw_cfg test,file=file with a
modified read_raw enabling DMA)

# perf stat -r 100 cat /sys/firmware/qemu_fw_cfg/by_name/test/raw >/dev/null
- with DMA:
0.636037 task-clock (msec) # 0.141 CPUs utilized ( +- 1.19% )
- without DMA:
6.430128 task-clock (msec) # 0.622 CPUs utilized ( +- 0.22% )

That's a few msec saved during boot by enabling DMA read (the gain
would be more substantial if other & bigger fw-cfg entries are read by
others from sysfs, unfortunately, it's not clear if we can always
enable DMA there)

Signed-off-by: Marc-André Lureau <[email protected]>
---
drivers/firmware/qemu_fw_cfg.c | 80 ++++++++++++++++++++++++++++++++++--------
1 file changed, 66 insertions(+), 14 deletions(-)

diff --git a/drivers/firmware/qemu_fw_cfg.c b/drivers/firmware/qemu_fw_cfg.c
index 69939e2529f2..ba9b907a4399 100644
--- a/drivers/firmware/qemu_fw_cfg.c
+++ b/drivers/firmware/qemu_fw_cfg.c
@@ -124,12 +124,46 @@ static ssize_t fw_cfg_dma_transfer(void *address, u32 length, u32 control)
return ret;
}

+/* with acpi & dev locks taken */
+static ssize_t fw_cfg_read_blob_dma(u16 key,
+ void *buf, loff_t pos, size_t count)
+{
+ ssize_t ret;
+
+ if (pos == 0) {
+ ret = fw_cfg_dma_transfer(buf, count, key << 16
+ | FW_CFG_DMA_CTL_SELECT
+ | FW_CFG_DMA_CTL_READ);
+ } else {
+ iowrite16(fw_cfg_sel_endianness(key), fw_cfg_reg_ctrl);
+ ret = fw_cfg_dma_transfer(NULL, pos, FW_CFG_DMA_CTL_SKIP);
+ if (ret < 0)
+ return ret;
+ ret = fw_cfg_dma_transfer(buf, count,
+ FW_CFG_DMA_CTL_READ);
+ }
+
+ return ret;
+}
+
+/* with acpi & dev locks taken */
+static void fw_cfg_read_blob_io(u16 key,
+ void *buf, loff_t pos, size_t count)
+{
+ iowrite16(fw_cfg_sel_endianness(key), fw_cfg_reg_ctrl);
+ while (pos-- > 0)
+ ioread8(fw_cfg_reg_data);
+ ioread8_rep(fw_cfg_reg_data, buf, count);
+}
+
/* read chunk of given fw_cfg blob (caller responsible for sanity-check) */
-static inline void fw_cfg_read_blob(u16 key,
- void *buf, loff_t pos, size_t count)
+static ssize_t fw_cfg_read_blob(u16 key,
+ void *buf, loff_t pos, size_t count,
+ bool dma)
{
u32 glk = -1U;
acpi_status status;
+ ssize_t ret = count;

/* If we have ACPI, ensure mutual exclusion against any potential
* device access by the firmware, e.g. via AML methods:
@@ -139,17 +173,21 @@ static inline void fw_cfg_read_blob(u16 key,
/* Should never get here */
WARN(1, "fw_cfg_read_blob: Failed to lock ACPI!\n");
memset(buf, 0, count);
- return;
+ return -EINVAL;
}

mutex_lock(&fw_cfg_dev_lock);
- iowrite16(fw_cfg_sel_endianness(key), fw_cfg_reg_ctrl);
- while (pos-- > 0)
- ioread8(fw_cfg_reg_data);
- ioread8_rep(fw_cfg_reg_data, buf, count);
+ if (dma && fw_cfg_dma_enabled()) {
+ ret = fw_cfg_read_blob_dma(key, buf, pos, count);
+ } else {
+ fw_cfg_read_blob_io(key, buf, pos, count);
+ }
+
mutex_unlock(&fw_cfg_dev_lock);

acpi_release_global_lock(glk);
+
+ return ret;
}

#ifdef CONFIG_CRASH_CORE
@@ -282,8 +320,9 @@ static int fw_cfg_do_platform_probe(struct platform_device *pdev)
#endif

/* 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) {
+ if (fw_cfg_read_blob(FW_CFG_SIGNATURE, sig,
+ 0, FW_CFG_SIG_SIZE, false) < 0 ||
+ memcmp(sig, "QEMU", FW_CFG_SIG_SIZE) != 0) {
fw_cfg_io_cleanup();
return -ENODEV;
}
@@ -466,8 +505,8 @@ static ssize_t fw_cfg_sysfs_read_raw(struct file *filp, struct kobject *kobj,
if (count > entry->size - pos)
count = entry->size - pos;

- fw_cfg_read_blob(entry->select, buf, pos, count);
- return count;
+ /* do not use DMA, virt_to_phys(buf) might not be ok */
+ return fw_cfg_read_blob(entry->select, buf, pos, count, false);
}

static struct bin_attribute fw_cfg_sysfs_attr_raw = {
@@ -632,7 +671,12 @@ static int fw_cfg_register_dir_entries(void)
struct fw_cfg_file *dir;
size_t dir_size;

- fw_cfg_read_blob(FW_CFG_FILE_DIR, &files.count, 0, sizeof(files.count));
+ ret = fw_cfg_read_blob(FW_CFG_FILE_DIR, &files.count,
+ 0, sizeof(files.count), false);
+ if (ret < 0) {
+ return ret;
+ }
+
count = be32_to_cpu(files.count);
dir_size = count * sizeof(struct fw_cfg_file);

@@ -640,7 +684,11 @@ static int fw_cfg_register_dir_entries(void)
if (!dir)
return -ENOMEM;

- fw_cfg_read_blob(FW_CFG_FILE_DIR, dir, sizeof(files.count), dir_size);
+ ret = fw_cfg_read_blob(FW_CFG_FILE_DIR, dir,
+ sizeof(files.count), dir_size, false);
+ if (ret < 0) {
+ goto end;
+ }

for (i = 0; i < count; i++) {
ret = fw_cfg_register_file(&dir[i]);
@@ -648,6 +696,7 @@ static int fw_cfg_register_dir_entries(void)
break;
}

+end:
kfree(dir);
return ret;
}
@@ -688,7 +737,10 @@ static int fw_cfg_sysfs_probe(struct platform_device *pdev)
goto err_probe;

/* get revision number, add matching top-level attribute */
- fw_cfg_read_blob(FW_CFG_ID, &rev, 0, sizeof(rev));
+ err = fw_cfg_read_blob(FW_CFG_ID, &rev, 0, sizeof(rev), false);
+ if (err < 0) {
+ goto err_probe;
+ }
fw_cfg_rev = le32_to_cpu(rev);
err = sysfs_create_file(fw_cfg_top_ko, &fw_cfg_rev_attr.attr);
if (err)
--
2.16.1.73.g5832b7e9f2

2018-02-14 14:21:47

by Marc-André Lureau

Subject: [PATCH v14 5/9] fw_cfg: fix sparse warning reading FW_CFG_ID

Use a restricted type for reading FW_CFG_ID, fixing sparse warning:

drivers/firmware/qemu_fw_cfg.c:540:22: warning: cast to restricted __le32

Signed-off-by: Marc-André Lureau <[email protected]>
---
drivers/firmware/qemu_fw_cfg.c | 5 +++--
1 file changed, 3 insertions(+), 2 deletions(-)

diff --git a/drivers/firmware/qemu_fw_cfg.c b/drivers/firmware/qemu_fw_cfg.c
index 8ad19086e5c5..4c4813409447 100644
--- a/drivers/firmware/qemu_fw_cfg.c
+++ b/drivers/firmware/qemu_fw_cfg.c
@@ -511,6 +511,7 @@ static inline void fw_cfg_kobj_cleanup(struct kobject *kobj)
static int fw_cfg_sysfs_probe(struct platform_device *pdev)
{
int err;
+ __le32 rev;

/* NOTE: If we supported multiple fw_cfg devices, we'd first create
* a subdirectory named after e.g. pdev->id, then hang per-device
@@ -536,8 +537,8 @@ static int fw_cfg_sysfs_probe(struct platform_device *pdev)
goto err_probe;

/* 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);
+ fw_cfg_read_blob(FW_CFG_ID, &rev, 0, sizeof(rev));
+ fw_cfg_rev = le32_to_cpu(rev);
err = sysfs_create_file(fw_cfg_top_ko, &fw_cfg_rev_attr.attr);
if (err)
goto err_rev;
--
2.16.1.73.g5832b7e9f2

2018-02-14 14:22:37

by Marc-André Lureau

Subject: [PATCH v14 6/9] fw_cfg: fix sparse warnings around FW_CFG_FILE_DIR read

Use struct fw_cfg_files to read the directory size, fixing the sparse
warnings:

drivers/firmware/qemu_fw_cfg.c:485:17: warning: cast to restricted __be32

Signed-off-by: Marc-André Lureau <[email protected]>
---
drivers/firmware/qemu_fw_cfg.c | 7 ++++---
1 file changed, 4 insertions(+), 3 deletions(-)

diff --git a/drivers/firmware/qemu_fw_cfg.c b/drivers/firmware/qemu_fw_cfg.c
index 4c4813409447..c4c726841ba7 100644
--- a/drivers/firmware/qemu_fw_cfg.c
+++ b/drivers/firmware/qemu_fw_cfg.c
@@ -478,18 +478,19 @@ static int fw_cfg_register_dir_entries(void)
{
int ret = 0;
u32 count, i;
+ struct fw_cfg_files files;
struct fw_cfg_file *dir;
size_t dir_size;

- fw_cfg_read_blob(FW_CFG_FILE_DIR, &count, 0, sizeof(count));
- count = be32_to_cpu(count);
+ fw_cfg_read_blob(FW_CFG_FILE_DIR, &files.count, 0, sizeof(files.count));
+ count = be32_to_cpu(files.count);
dir_size = count * sizeof(struct fw_cfg_file);

dir = kmalloc(dir_size, GFP_KERNEL);
if (!dir)
return -ENOMEM;

- fw_cfg_read_blob(FW_CFG_FILE_DIR, dir, sizeof(count), dir_size);
+ fw_cfg_read_blob(FW_CFG_FILE_DIR, dir, sizeof(files.count), dir_size);

for (i = 0; i < count; i++) {
ret = fw_cfg_register_file(&dir[i]);
--
2.16.1.73.g5832b7e9f2

2018-02-14 14:23:30

by Marc-André Lureau

Subject: [PATCH v14 1/9] crash: export paddr_vmcoreinfo_note()

The following patch is going to use the symbol from the fw_cfg module,
to call the function and write the note location details in the
vmcoreinfo entry, so qemu can produce dumps with the vmcoreinfo note.

CC: Andrew Morton <[email protected]>
CC: Baoquan He <[email protected]>
CC: Dave Young <[email protected]>
CC: Dave Young <[email protected]>
CC: Hari Bathini <[email protected]>
CC: Tony Luck <[email protected]>
CC: Vivek Goyal <[email protected]>
Signed-off-by: Marc-André Lureau <[email protected]>
Acked-by: Gabriel Somlo <[email protected]>
---
kernel/crash_core.c | 1 +
1 file changed, 1 insertion(+)

diff --git a/kernel/crash_core.c b/kernel/crash_core.c
index 4f63597c824d..a93590cdd9e1 100644
--- a/kernel/crash_core.c
+++ b/kernel/crash_core.c
@@ -376,6 +376,7 @@ phys_addr_t __weak paddr_vmcoreinfo_note(void)
{
return __pa(vmcoreinfo_note);
}
+EXPORT_SYMBOL(paddr_vmcoreinfo_note);

static int __init crash_save_vmcoreinfo_init(void)
{
--
2.16.1.73.g5832b7e9f2

2018-02-14 14:24:06

by Marc-André Lureau

Subject: [PATCH v14 3/9] fw_cfg: fix sparse warnings in fw_cfg_sel_endianness()

The function is used for both LE & BE target type, use __force casting.

Fixes:
$ make C=1 CF=-D__CHECK_ENDIAN__ drivers/firmware/qemu_fw_cfg.o

drivers/firmware/qemu_fw_cfg.c:55:33: warning: restricted __be16 degrades to integer
drivers/firmware/qemu_fw_cfg.c:55:52: warning: restricted __le16 degrades to integer

Signed-off-by: Marc-André Lureau <[email protected]>
---
drivers/firmware/qemu_fw_cfg.c | 4 +++-
1 file changed, 3 insertions(+), 1 deletion(-)

diff --git a/drivers/firmware/qemu_fw_cfg.c b/drivers/firmware/qemu_fw_cfg.c
index 90f467232777..85e693287d87 100644
--- a/drivers/firmware/qemu_fw_cfg.c
+++ b/drivers/firmware/qemu_fw_cfg.c
@@ -52,7 +52,9 @@ static DEFINE_MUTEX(fw_cfg_dev_lock);
/* pick appropriate endianness for selector key */
static inline u16 fw_cfg_sel_endianness(u16 key)
{
- return fw_cfg_is_mmio ? cpu_to_be16(key) : cpu_to_le16(key);
+ return fw_cfg_is_mmio ?
+ (u16 __force)cpu_to_be16(key) :
+ (u16 __force)cpu_to_le16(key);
}

/* read chunk of given fw_cfg blob (caller responsible for sanity-check) */
--
2.16.1.73.g5832b7e9f2

2018-02-14 16:55:42

by Marc-Andre Lureau

Subject: Re: [PATCH v14 9/9] RFC: fw_cfg: do DMA read operation

Hi

On Wed, Feb 14, 2018 at 5:48 PM, Michael S. Tsirkin <[email protected]> wrote:
> On Wed, Feb 14, 2018 at 03:18:50PM +0100, Marc-André Lureau wrote:
>> Modify fw_cfg_read_blob() to use DMA if the device supports it.
>> Return errors, because the operation may fail.
>>
>> So far, only one call in fw_cfg_register_dir_entries() is using
>> kmalloc'ed buf and is thus clearly eligible to DMA read.
>>
>> Initially, I didn't implement DMA read to speed up boot time, but as a
>> first step before introducing DMA write (since read operations were
>> already presents). Even more, I didn't realize fw-cfg entries were
>> being read by the kernel during boot by default. But actally fw-cfg
>> entries are being populated during module probe. I knew DMA improved a
>> lot bios boot time (the main reason the DMA interface was added
>> afaik). Let see the time it would take to read the whole ACPI
>> tables (128kb allocated)
>>
>> # time cat /sys/firmware/qemu_fw_cfg/by_name/etc/acpi/tables/raw
>> - with DMA: sys 0m0.003s
>> - without DMA (-global fw_cfg.dma_enabled=off): sys 0m7.674s
>>
>> FW_CFG_FILE_DIR (0x19) is the only "file" that is read during kernel
>> boot to populate sysfs qemu_fw_cfg directory, and it is quite
>> small (1-2kb). Since it does not expose itself, in order to measure
>> the time it takes to read such small file, I took a comparable sized
>> file of 2048 bytes and exposed it (-fw_cfg test,file=file with a
>> modified read_raw enabling DMA)
>>
>> # perf stat -r 100 cat /sys/firmware/qemu_fw_cfg/by_name/test/raw >/dev/null
>> - with DMA:
>> 0.636037 task-clock (msec) # 0.141 CPUs utilized ( +- 1.19% )
>> - without DMA:
>> 6.430128 task-clock (msec) # 0.622 CPUs utilized ( +- 0.22% )
>>
>> That's a few msec saved during boot by enabling DMA read (the gain
>> would be more substantial if other & bigger fw-cfg entries are read by
>> others from sysfs, unfortunately, it's not clear if we can always
>> enable DMA there)
>>
>> Signed-off-by: Marc-André Lureau <[email protected]>
>> ---
>> drivers/firmware/qemu_fw_cfg.c | 80 ++++++++++++++++++++++++++++++++++--------
>> 1 file changed, 66 insertions(+), 14 deletions(-)
>>
>> diff --git a/drivers/firmware/qemu_fw_cfg.c b/drivers/firmware/qemu_fw_cfg.c
>> index 69939e2529f2..ba9b907a4399 100644
>> --- a/drivers/firmware/qemu_fw_cfg.c
>> +++ b/drivers/firmware/qemu_fw_cfg.c
>> @@ -124,12 +124,46 @@ static ssize_t fw_cfg_dma_transfer(void *address, u32 length, u32 control)
>> return ret;
>> }
>>
>> +/* with acpi & dev locks taken */
>> +static ssize_t fw_cfg_read_blob_dma(u16 key,
>> + void *buf, loff_t pos, size_t count)
>> +{
>> + ssize_t ret;
>> +
>> + if (pos == 0) {
>> + ret = fw_cfg_dma_transfer(buf, count, key << 16
>> + | FW_CFG_DMA_CTL_SELECT
>> + | FW_CFG_DMA_CTL_READ);
>> + } else {
>> + iowrite16(fw_cfg_sel_endianness(key), fw_cfg_reg_ctrl);
>> + ret = fw_cfg_dma_transfer(NULL, pos, FW_CFG_DMA_CTL_SKIP);
>> + if (ret < 0)
>> + return ret;
>> + ret = fw_cfg_dma_transfer(buf, count,
>> + FW_CFG_DMA_CTL_READ);
>> + }
>> +
>> + return ret;
>> +}
>> +
>> +/* with acpi & dev locks taken */
>> +static void fw_cfg_read_blob_io(u16 key,
>> + void *buf, loff_t pos, size_t count)
>> +{
>> + iowrite16(fw_cfg_sel_endianness(key), fw_cfg_reg_ctrl);
>> + while (pos-- > 0)
>> + ioread8(fw_cfg_reg_data);
>> + ioread8_rep(fw_cfg_reg_data, buf, count);
>> +}
>> +
>> /* read chunk of given fw_cfg blob (caller responsible for sanity-check) */
>> -static inline void fw_cfg_read_blob(u16 key,
>> - void *buf, loff_t pos, size_t count)
>> +static ssize_t fw_cfg_read_blob(u16 key,
>> + void *buf, loff_t pos, size_t count,
>> + bool dma)
>> {
>> u32 glk = -1U;
>> acpi_status status;
>> + ssize_t ret = count;
>>
>> /* If we have ACPI, ensure mutual exclusion against any potential
>> * device access by the firmware, e.g. via AML methods:
>> @@ -139,17 +173,21 @@ static inline void fw_cfg_read_blob(u16 key,
>> /* Should never get here */
>> WARN(1, "fw_cfg_read_blob: Failed to lock ACPI!\n");
>> memset(buf, 0, count);
>> - return;
>> + return -EINVAL;
>> }
>>
>> mutex_lock(&fw_cfg_dev_lock);
>> - iowrite16(fw_cfg_sel_endianness(key), fw_cfg_reg_ctrl);
>> - while (pos-- > 0)
>> - ioread8(fw_cfg_reg_data);
>> - ioread8_rep(fw_cfg_reg_data, buf, count);
>> + if (dma && fw_cfg_dma_enabled()) {
>> + ret = fw_cfg_read_blob_dma(key, buf, pos, count);
>> + } else {
>> + fw_cfg_read_blob_io(key, buf, pos, count);
>
> I'd set ret = count here.

Ok, why not.

>
>> + }
>> +
>> mutex_unlock(&fw_cfg_dev_lock);
>>
>> acpi_release_global_lock(glk);
>> +
>> + return ret;
>> }
>>
>> #ifdef CONFIG_CRASH_CORE
>> @@ -282,8 +320,9 @@ static int fw_cfg_do_platform_probe(struct platform_device *pdev)
>> #endif
>>
>> /* 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) {
>> + if (fw_cfg_read_blob(FW_CFG_SIGNATURE, sig,
>> + 0, FW_CFG_SIG_SIZE, false) < 0 ||
>> + memcmp(sig, "QEMU", FW_CFG_SIG_SIZE) != 0) {
>> fw_cfg_io_cleanup();
>> return -ENODEV;
>> }
>
> Rather than add dead code, how about a promise not to
> fail if dma is disabled? Patch will be smaller then.

Even with dma disabled, you could have a locking bug which was
silently ignored before and is now taken into account.

>
>> @@ -466,8 +505,8 @@ static ssize_t fw_cfg_sysfs_read_raw(struct file *filp, struct kobject *kobj,
>> if (count > entry->size - pos)
>> count = entry->size - pos;
>>
>> - fw_cfg_read_blob(entry->select, buf, pos, count);
>> - return count;
>> + /* do not use DMA, virt_to_phys(buf) might not be ok */
>> + return fw_cfg_read_blob(entry->select, buf, pos, count, false);
>> }
>>
>> static struct bin_attribute fw_cfg_sysfs_attr_raw = {
>> @@ -632,7 +671,12 @@ static int fw_cfg_register_dir_entries(void)
>> struct fw_cfg_file *dir;
>> size_t dir_size;
>>
>> - fw_cfg_read_blob(FW_CFG_FILE_DIR, &files.count, 0, sizeof(files.count));
>> + ret = fw_cfg_read_blob(FW_CFG_FILE_DIR, &files.count,
>> + 0, sizeof(files.count), false);
>> + if (ret < 0) {
>> + return ret;
>> + }
>> +
>> count = be32_to_cpu(files.count);
>> dir_size = count * sizeof(struct fw_cfg_file);
>>
>> @@ -640,7 +684,11 @@ static int fw_cfg_register_dir_entries(void)
>> if (!dir)
>> return -ENOMEM;
>>
>> - fw_cfg_read_blob(FW_CFG_FILE_DIR, dir, sizeof(files.count), dir_size);
>> + ret = fw_cfg_read_blob(FW_CFG_FILE_DIR, dir,
>> + sizeof(files.count), dir_size, false);
>> + if (ret < 0) {
>> + goto end;
>> + }
>>
>> for (i = 0; i < count; i++) {
>> ret = fw_cfg_register_file(&dir[i]);
>> @@ -648,6 +696,7 @@ static int fw_cfg_register_dir_entries(void)
>> break;
>> }
>>
>> +end:
>> kfree(dir);
>> return ret;
>> }
>> @@ -688,7 +737,10 @@ static int fw_cfg_sysfs_probe(struct platform_device *pdev)
>> goto err_probe;
>>
>> /* get revision number, add matching top-level attribute */
>> - fw_cfg_read_blob(FW_CFG_ID, &rev, 0, sizeof(rev));
>> + err = fw_cfg_read_blob(FW_CFG_ID, &rev, 0, sizeof(rev), false);
>> + if (err < 0) {
>> + goto err_probe;
>> + }
>> fw_cfg_rev = le32_to_cpu(rev);
>> err = sysfs_create_file(fw_cfg_top_ko, &fw_cfg_rev_attr.attr);
>> if (err)
>> --
>> 2.16.1.73.g5832b7e9f2

2018-02-14 17:01:14

by Michael S. Tsirkin

Subject: Re: [PATCH v14 9/9] RFC: fw_cfg: do DMA read operation

On Wed, Feb 14, 2018 at 05:52:10PM +0100, Marc-Andre Lureau wrote:
> >> @@ -282,8 +320,9 @@ static int fw_cfg_do_platform_probe(struct platform_device *pdev)
> >> #endif
> >>
> >> /* 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) {
> >> + if (fw_cfg_read_blob(FW_CFG_SIGNATURE, sig,
> >> + 0, FW_CFG_SIG_SIZE, false) < 0 ||
> >> + memcmp(sig, "QEMU", FW_CFG_SIG_SIZE) != 0) {
> >> fw_cfg_io_cleanup();
> >> return -ENODEV;
> >> }
> >
> > Rather than add dead code, how about a promise not to
> > fail if dma is disabled? Patch will be smaller then.
>
> Even with dma disabled, you could have a locking bug which was
> silently ignored before and is now taken into account.

I see. I'd start with a patch reporting errors to users then.
That would be a bugfix and can be merged for this version.

> >
> >> @@ -466,8 +505,8 @@ static ssize_t fw_cfg_sysfs_read_raw(struct file *filp, struct kobject *kobj,
> >> if (count > entry->size - pos)
> >> count = entry->size - pos;
> >>
> >> - fw_cfg_read_blob(entry->select, buf, pos, count);
> >> - return count;
> >> + /* do not use DMA, virt_to_phys(buf) might not be ok */
> >> + return fw_cfg_read_blob(entry->select, buf, pos, count, false);
> >> }
> >>
> >> static struct bin_attribute fw_cfg_sysfs_attr_raw = {
> >> @@ -632,7 +671,12 @@ static int fw_cfg_register_dir_entries(void)
> >> struct fw_cfg_file *dir;
> >> size_t dir_size;
> >>
> >> - fw_cfg_read_blob(FW_CFG_FILE_DIR, &files.count, 0, sizeof(files.count));
> >> + ret = fw_cfg_read_blob(FW_CFG_FILE_DIR, &files.count,
> >> + 0, sizeof(files.count), false);
> >> + if (ret < 0) {
> >> + return ret;
> >> + }
> >> +
> >> count = be32_to_cpu(files.count);
> >> dir_size = count * sizeof(struct fw_cfg_file);
> >>
> >> @@ -640,7 +684,11 @@ static int fw_cfg_register_dir_entries(void)
> >> if (!dir)
> >> return -ENOMEM;
> >>
> >> - fw_cfg_read_blob(FW_CFG_FILE_DIR, dir, sizeof(files.count), dir_size);
> >> + ret = fw_cfg_read_blob(FW_CFG_FILE_DIR, dir,
> >> + sizeof(files.count), dir_size, false);
> >> + if (ret < 0) {
> >> + goto end;
> >> + }
> >>
> >> for (i = 0; i < count; i++) {
> >> ret = fw_cfg_register_file(&dir[i]);
> >> @@ -648,6 +696,7 @@ static int fw_cfg_register_dir_entries(void)
> >> break;
> >> }
> >>
> >> +end:
> >> kfree(dir);
> >> return ret;
> >> }
> >> @@ -688,7 +737,10 @@ static int fw_cfg_sysfs_probe(struct platform_device *pdev)
> >> goto err_probe;
> >>
> >> /* get revision number, add matching top-level attribute */
> >> - fw_cfg_read_blob(FW_CFG_ID, &rev, 0, sizeof(rev));
> >> + err = fw_cfg_read_blob(FW_CFG_ID, &rev, 0, sizeof(rev), false);
> >> + if (err < 0) {
> >> + goto err_probe;
> >> + }
> >> fw_cfg_rev = le32_to_cpu(rev);
> >> err = sysfs_create_file(fw_cfg_top_ko, &fw_cfg_rev_attr.attr);
> >> if (err)
> >> --
> >> 2.16.1.73.g5832b7e9f2

2018-02-14 17:10:24

by Marc-Andre Lureau

Subject: Re: [PATCH v14 9/9] RFC: fw_cfg: do DMA read operation

Hi

On Wed, Feb 14, 2018 at 5:59 PM, Michael S. Tsirkin <[email protected]> wrote:
> On Wed, Feb 14, 2018 at 05:52:10PM +0100, Marc-Andre Lureau wrote:
>> >> @@ -282,8 +320,9 @@ static int fw_cfg_do_platform_probe(struct platform_device *pdev)
>> >> #endif
>> >>
>> >> /* 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) {
>> >> + if (fw_cfg_read_blob(FW_CFG_SIGNATURE, sig,
>> >> + 0, FW_CFG_SIG_SIZE, false) < 0 ||
>> >> + memcmp(sig, "QEMU", FW_CFG_SIG_SIZE) != 0) {
>> >> fw_cfg_io_cleanup();
>> >> return -ENODEV;
>> >> }
>> >
>> > Rather than add dead code, how about a promise not to
>> > fail if dma is disabled? Patch will be smaller then.
>>
>> Even with dma disabled, you could have a locking bug which was
>> silently ignored before and is now taken into account.
>
> I see. I'd start with a patch reporting errors to users then.
> That would be a bugfix and can be merged for this version.

silently is the wrong word for it, there is already a WARN(). However,
the memcmp was done on uninitialzed sig[] (which likely resulted in
returning -ENODEV in the function)

Now it can check if the function failed to read before doing the memcmp.

Hope that clarifies it.

>
>> >
>> >> @@ -466,8 +505,8 @@ static ssize_t fw_cfg_sysfs_read_raw(struct file *filp, struct kobject *kobj,
>> >> if (count > entry->size - pos)
>> >> count = entry->size - pos;
>> >>
>> >> - fw_cfg_read_blob(entry->select, buf, pos, count);
>> >> - return count;
>> >> + /* do not use DMA, virt_to_phys(buf) might not be ok */
>> >> + return fw_cfg_read_blob(entry->select, buf, pos, count, false);
>> >> }
>> >>
>> >> static struct bin_attribute fw_cfg_sysfs_attr_raw = {
>> >> @@ -632,7 +671,12 @@ static int fw_cfg_register_dir_entries(void)
>> >> struct fw_cfg_file *dir;
>> >> size_t dir_size;
>> >>
>> >> - fw_cfg_read_blob(FW_CFG_FILE_DIR, &files.count, 0, sizeof(files.count));
>> >> + ret = fw_cfg_read_blob(FW_CFG_FILE_DIR, &files.count,
>> >> + 0, sizeof(files.count), false);
>> >> + if (ret < 0) {
>> >> + return ret;
>> >> + }
>> >> +
>> >> count = be32_to_cpu(files.count);
>> >> dir_size = count * sizeof(struct fw_cfg_file);
>> >>
>> >> @@ -640,7 +684,11 @@ static int fw_cfg_register_dir_entries(void)
>> >> if (!dir)
>> >> return -ENOMEM;
>> >>
>> >> - fw_cfg_read_blob(FW_CFG_FILE_DIR, dir, sizeof(files.count), dir_size);
>> >> + ret = fw_cfg_read_blob(FW_CFG_FILE_DIR, dir,
>> >> + sizeof(files.count), dir_size, false);
>> >> + if (ret < 0) {
>> >> + goto end;
>> >> + }
>> >>
>> >> for (i = 0; i < count; i++) {
>> >> ret = fw_cfg_register_file(&dir[i]);
>> >> @@ -648,6 +696,7 @@ static int fw_cfg_register_dir_entries(void)
>> >> break;
>> >> }
>> >>
>> >> +end:
>> >> kfree(dir);
>> >> return ret;
>> >> }
>> >> @@ -688,7 +737,10 @@ static int fw_cfg_sysfs_probe(struct platform_device *pdev)
>> >> goto err_probe;
>> >>
>> >> /* get revision number, add matching top-level attribute */
>> >> - fw_cfg_read_blob(FW_CFG_ID, &rev, 0, sizeof(rev));
>> >> + err = fw_cfg_read_blob(FW_CFG_ID, &rev, 0, sizeof(rev), false);
>> >> + if (err < 0) {
>> >> + goto err_probe;
>> >> + }
>> >> fw_cfg_rev = le32_to_cpu(rev);
>> >> err = sysfs_create_file(fw_cfg_top_ko, &fw_cfg_rev_attr.attr);
>> >> if (err)
>> >> --
>> >> 2.16.1.73.g5832b7e9f2

2018-02-14 19:38:53

by Michael S. Tsirkin

Subject: Re: [PATCH v14 2/9] fw_cfg: add a public uapi header

On Wed, Feb 14, 2018 at 03:18:43PM +0100, Marc-Andr? Lureau wrote:
> Create a common header file for well-known values and structures to be
> shared by the Linux kernel with qemu or other projects.
>
> Suggested-by: Michael S. Tsirkin <[email protected]>
> Signed-off-by: Marc-Andr? Lureau <[email protected]>
>
> ---
>
> The related qemu patch making use of it, to be submitted:
> https://github.com/elmarco/qemu/commit/4884fc9e9c4c4467a371e5a40f3181239e1b70f5
> ---
> MAINTAINERS | 1 +
> drivers/firmware/qemu_fw_cfg.c | 22 +--------
> include/uapi/linux/fw_cfg.h | 102 +++++++++++++++++++++++++++++++++++++++++
> 3 files changed, 105 insertions(+), 20 deletions(-)
> create mode 100644 include/uapi/linux/fw_cfg.h
>
> diff --git a/MAINTAINERS b/MAINTAINERS
> index 3bdc260e36b7..a66b65f62811 100644
> --- a/MAINTAINERS
> +++ b/MAINTAINERS
> @@ -11352,6 +11352,7 @@ M: "Michael S. Tsirkin" <[email protected]>
> L: [email protected]
> S: Maintained
> F: drivers/firmware/qemu_fw_cfg.c
> +F: include/uapi/linux/fw_cfg.h
>
> QIB DRIVER
> M: Dennis Dalessandro <[email protected]>
> diff --git a/drivers/firmware/qemu_fw_cfg.c b/drivers/firmware/qemu_fw_cfg.c
> index a41b572eeeb1..90f467232777 100644
> --- a/drivers/firmware/qemu_fw_cfg.c
> +++ b/drivers/firmware/qemu_fw_cfg.c
> @@ -32,30 +32,12 @@
> #include <linux/slab.h>
> #include <linux/io.h>
> #include <linux/ioport.h>
> +#include <linux/fw_cfg.h>

You include the header from include/linux/fw_cfg.h ...

>
> MODULE_AUTHOR("Gabriel L. Somlo <[email protected]>");
> MODULE_DESCRIPTION("QEMU fw_cfg sysfs support");
> MODULE_LICENSE("GPL");
>
> -/* selector key 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 {
> - u32 size;
> - u16 select;
> - u16 reserved;
> - char name[FW_CFG_MAX_FILE_PATH];
> -};
> -
> /* fw_cfg device i/o register addresses */
> static bool fw_cfg_is_mmio;
> static phys_addr_t fw_cfg_p_base;
> @@ -597,7 +579,7 @@ MODULE_DEVICE_TABLE(of, fw_cfg_sysfs_mmio_match);
>
> #ifdef CONFIG_ACPI
> static const struct acpi_device_id fw_cfg_sysfs_acpi_match[] = {
> - { "QEMU0002", },
> + { FW_CFG_ACPI_DEVICE_ID, },
> {},
> };
> MODULE_DEVICE_TABLE(acpi, fw_cfg_sysfs_acpi_match);
> diff --git a/include/uapi/linux/fw_cfg.h b/include/uapi/linux/fw_cfg.h
> new file mode 100644
> index 000000000000..5b8136ce46ee
> --- /dev/null
> +++ b/include/uapi/linux/fw_cfg.h

Yet the header is located in include/uapi/linux/fw_cfg.h

How can this work?

> @@ -0,0 +1,102 @@
> +/* SPDX-License-Identifier: GPL-2.0 WITH Linux-syscall-note */

As far as I can see these have all been lifted from a BSD-licensed
hw/nvram/fw_cfg.c in qemu. So please make it BSD accordingly,
and include the explanation in the commit log.

> +#ifndef _LINUX_FW_CFG_H
> +#define _LINUX_FW_CFG_H
> +
> +#include <linux/types.h>
> +
> +#define FW_CFG_ACPI_DEVICE_ID "QEMU0002"
> +
> +/* selector key values for "well-known" fw_cfg entries */
> +#define FW_CFG_SIGNATURE 0x00
> +#define FW_CFG_ID 0x01
> +#define FW_CFG_UUID 0x02
> +#define FW_CFG_RAM_SIZE 0x03
> +#define FW_CFG_NOGRAPHIC 0x04
> +#define FW_CFG_NB_CPUS 0x05
> +#define FW_CFG_MACHINE_ID 0x06
> +#define FW_CFG_KERNEL_ADDR 0x07
> +#define FW_CFG_KERNEL_SIZE 0x08
> +#define FW_CFG_KERNEL_CMDLINE 0x09
> +#define FW_CFG_INITRD_ADDR 0x0a
> +#define FW_CFG_INITRD_SIZE 0x0b
> +#define FW_CFG_BOOT_DEVICE 0x0c
> +#define FW_CFG_NUMA 0x0d
> +#define FW_CFG_BOOT_MENU 0x0e
> +#define FW_CFG_MAX_CPUS 0x0f
> +#define FW_CFG_KERNEL_ENTRY 0x10
> +#define FW_CFG_KERNEL_DATA 0x11
> +#define FW_CFG_INITRD_DATA 0x12
> +#define FW_CFG_CMDLINE_ADDR 0x13
> +#define FW_CFG_CMDLINE_SIZE 0x14
> +#define FW_CFG_CMDLINE_DATA 0x15
> +#define FW_CFG_SETUP_ADDR 0x16
> +#define FW_CFG_SETUP_SIZE 0x17
> +#define FW_CFG_SETUP_DATA 0x18
> +#define FW_CFG_FILE_DIR 0x19
> +
> +#define FW_CFG_FILE_FIRST 0x20
> +#define FW_CFG_FILE_SLOTS_MIN 0x10
> +
> +#define FW_CFG_WRITE_CHANNEL 0x4000
> +#define FW_CFG_ARCH_LOCAL 0x8000
> +#define FW_CFG_ENTRY_MASK (~(FW_CFG_WRITE_CHANNEL | FW_CFG_ARCH_LOCAL))
> +
> +#define FW_CFG_INVALID 0xffff
> +
> +/* width in bytes of fw_cfg control register */
> +#define FW_CFG_CTL_SIZE 0x02
> +
> +/* fw_cfg "file name" is up to 56 characters (including terminating nul) */
> +#define FW_CFG_MAX_FILE_PATH 56
> +
> +/* size in bytes of fw_cfg signature */
> +#define FW_CFG_SIG_SIZE 4
> +
> +/* FW_CFG_ID bits */
> +#define FW_CFG_VERSION 0x01
> +#define FW_CFG_VERSION_DMA 0x02
> +
> +/* fw_cfg file directory entry type */
> +struct fw_cfg_file {
> + __be32 size; /* file size */
> + __be16 select; /* write this to 0x510 to read it */
> + __u16 reserved;
> + char name[FW_CFG_MAX_FILE_PATH];
> +};
> +
> +struct fw_cfg_files {
> + __be32 count; /* number of entries */
> + struct fw_cfg_file f[];
> +};
> +
> +/* FW_CFG_DMA_CONTROL bits */
> +#define FW_CFG_DMA_CTL_ERROR 0x01
> +#define FW_CFG_DMA_CTL_READ 0x02
> +#define FW_CFG_DMA_CTL_SKIP 0x04
> +#define FW_CFG_DMA_CTL_SELECT 0x08
> +#define FW_CFG_DMA_CTL_WRITE 0x10
> +
> +#define FW_CFG_DMA_SIGNATURE 0x51454d5520434647ULL /* "QEMU CFG" */
> +
> +/* Control as first field allows for different structures selected by this
> + * field, which might be useful in the future
> + */
> +struct fw_cfg_dma_access {
> + __be32 control;
> + __be32 length;
> + __be64 address;
> +};
> +
> +#define FW_CFG_VMCOREINFO_FILENAME "etc/vmcoreinfo"
> +
> +#define FW_CFG_VMCOREINFO_FORMAT_NONE 0x0
> +#define FW_CFG_VMCOREINFO_FORMAT_ELF 0x1
> +
> +struct fw_cfg_vmcoreinfo {
> + __le16 host_format;
> + __le16 guest_format;
> + __le32 size;
> + __le64 paddr;
> +};
> +
> +#endif
> --
> 2.16.1.73.g5832b7e9f2

2018-02-14 19:56:24

by Marc-André Lureau

Subject: [PATCH v14 8/9] fw_cfg: write vmcoreinfo details

If the "etc/vmcoreinfo" fw_cfg file is present and we are not running
the kdump kernel, write the addr/size of the vmcoreinfo ELF note.

The DMA operation is expected to run synchronously with today qemu,
but the specification states that it may become async, so we run
"control" field check in a loop for eventual changes.

Signed-off-by: Marc-André Lureau <[email protected]>
---
drivers/firmware/qemu_fw_cfg.c | 144 ++++++++++++++++++++++++++++++++++++++++-
1 file changed, 141 insertions(+), 3 deletions(-)

diff --git a/drivers/firmware/qemu_fw_cfg.c b/drivers/firmware/qemu_fw_cfg.c
index 37638b95cb45..69939e2529f2 100644
--- a/drivers/firmware/qemu_fw_cfg.c
+++ b/drivers/firmware/qemu_fw_cfg.c
@@ -34,11 +34,17 @@
#include <linux/io.h>
#include <linux/ioport.h>
#include <linux/fw_cfg.h>
+#include <linux/delay.h>
+#include <linux/crash_dump.h>
+#include <linux/crash_core.h>

MODULE_AUTHOR("Gabriel L. Somlo <[email protected]>");
MODULE_DESCRIPTION("QEMU fw_cfg sysfs support");
MODULE_LICENSE("GPL");

+/* fw_cfg revision attribute, in /sys/firmware/qemu_fw_cfg top-level dir. */
+static u32 fw_cfg_rev;
+
/* fw_cfg device i/o register addresses */
static bool fw_cfg_is_mmio;
static phys_addr_t fw_cfg_p_base;
@@ -59,6 +65,65 @@ static inline u16 fw_cfg_sel_endianness(u16 key)
(u16 __force)cpu_to_le16(key);
}

+static inline bool fw_cfg_dma_enabled(void)
+{
+ return (fw_cfg_rev & FW_CFG_VERSION_DMA) && fw_cfg_reg_dma;
+}
+
+/* qemu fw_cfg device is sync today, but spec says it may become async */
+static void fw_cfg_wait_for_control(struct fw_cfg_dma_access *d)
+{
+ do {
+ u32 ctrl = be32_to_cpu(READ_ONCE(d->control));
+
+ if ((ctrl & ~FW_CFG_DMA_CTL_ERROR) == 0)
+ return;
+
+ usleep_range(50, 100);
+ } while (true);
+
+ /* do not reorder the read to d->control */
+ rmb();
+}
+
+static ssize_t fw_cfg_dma_transfer(void *address, u32 length, u32 control)
+{
+ phys_addr_t dma;
+ struct fw_cfg_dma_access *d = NULL;
+ ssize_t ret = length;
+
+ d = kmalloc(sizeof(*d), GFP_KERNEL);
+ if (!d) {
+ ret = -ENOMEM;
+ goto end;
+ }
+
+ /* fw_cfg device does not need IOMMU protection, so use physical addresses */
+ *d = (struct fw_cfg_dma_access) {
+ .address = cpu_to_be64(address ? virt_to_phys(address) : 0),
+ .length = cpu_to_be32(length),
+ .control = cpu_to_be32(control)
+ };
+
+ dma = virt_to_phys(d);
+
+ iowrite32be((u64)dma >> 32, fw_cfg_reg_dma);
+ /* force memory to sync before notifying device via MMIO */
+ wmb();
+ iowrite32be(dma, fw_cfg_reg_dma + 4);
+
+ fw_cfg_wait_for_control(d);
+
+ if (be32_to_cpu(READ_ONCE(d->control)) & FW_CFG_DMA_CTL_ERROR) {
+ ret = -EIO;
+ }
+
+end:
+ kfree(d);
+
+ return ret;
+}
+
/* read chunk of given fw_cfg blob (caller responsible for sanity-check) */
static inline void fw_cfg_read_blob(u16 key,
void *buf, loff_t pos, size_t count)
@@ -87,6 +152,47 @@ static inline void fw_cfg_read_blob(u16 key,
acpi_release_global_lock(glk);
}

+#ifdef CONFIG_CRASH_CORE
+/* write chunk of given fw_cfg blob (caller responsible for sanity-check) */
+static ssize_t fw_cfg_write_blob(u16 key,
+ void *buf, loff_t pos, size_t count)
+{
+ u32 glk = -1U;
+ acpi_status status;
+ ssize_t ret = count;
+
+ /* If we have ACPI, ensure mutual exclusion against any potential
+ * device access by the firmware, e.g. via AML methods:
+ */
+ status = acpi_acquire_global_lock(ACPI_WAIT_FOREVER, &glk);
+ if (ACPI_FAILURE(status) && status != AE_NOT_CONFIGURED) {
+ /* Should never get here */
+ WARN(1, "%s: Failed to lock ACPI!\n", __func__);
+ return -EINVAL;
+ }
+
+ mutex_lock(&fw_cfg_dev_lock);
+ if (pos == 0) {
+ ret = fw_cfg_dma_transfer(buf, count, key << 16
+ | FW_CFG_DMA_CTL_SELECT
+ | FW_CFG_DMA_CTL_WRITE);
+ } else {
+ iowrite16(fw_cfg_sel_endianness(key), fw_cfg_reg_ctrl);
+ ret = fw_cfg_dma_transfer(NULL, pos, FW_CFG_DMA_CTL_SKIP);
+ if (ret < 0)
+ goto end;
+ ret = fw_cfg_dma_transfer(buf, count, FW_CFG_DMA_CTL_WRITE);
+ }
+
+end:
+ mutex_unlock(&fw_cfg_dev_lock);
+
+ acpi_release_global_lock(glk);
+
+ return ret;
+}
+#endif /* CONFIG_CRASH_CORE */
+
/* clean up fw_cfg device i/o */
static void fw_cfg_io_cleanup(void)
{
@@ -185,9 +291,6 @@ static int fw_cfg_do_platform_probe(struct platform_device *pdev)
return 0;
}

-/* fw_cfg revision attribute, in /sys/firmware/qemu_fw_cfg top-level dir. */
-static u32 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);
@@ -210,6 +313,32 @@ struct fw_cfg_sysfs_entry {
struct list_head list;
};

+#ifdef CONFIG_CRASH_CORE
+static ssize_t fw_cfg_write_vmcoreinfo(const struct fw_cfg_file *f)
+{
+ static struct fw_cfg_vmcoreinfo *data;
+ ssize_t ret;
+
+ data = kmalloc(sizeof(struct fw_cfg_vmcoreinfo), GFP_KERNEL);
+ if (!data)
+ return -ENOMEM;
+
+ *data = (struct fw_cfg_vmcoreinfo) {
+ .guest_format = cpu_to_le16(FW_CFG_VMCOREINFO_FORMAT_ELF),
+ .size = cpu_to_le32(VMCOREINFO_NOTE_SIZE),
+ .paddr = cpu_to_le64(paddr_vmcoreinfo_note())
+ };
+ /* spare ourself reading host format support for now since we
+ * don't know what else to format - host may ignore ours
+ */
+ ret = fw_cfg_write_blob(be16_to_cpu(f->select), data,
+ 0, sizeof(struct fw_cfg_vmcoreinfo));
+
+ kfree(data);
+ return ret;
+}
+#endif /* CONFIG_CRASH_CORE */
+
/* get fw_cfg_sysfs_entry from kobject member */
static inline struct fw_cfg_sysfs_entry *to_entry(struct kobject *kobj)
{
@@ -450,6 +579,15 @@ static int fw_cfg_register_file(const struct fw_cfg_file *f)
int err;
struct fw_cfg_sysfs_entry *entry;

+#ifdef CONFIG_CRASH_CORE
+ if (fw_cfg_dma_enabled() &&
+ strcmp(f->name, FW_CFG_VMCOREINFO_FILENAME) == 0 &&
+ !is_kdump_kernel()) {
+ if (fw_cfg_write_vmcoreinfo(f) < 0)
+ pr_warn("fw_cfg: failed to write vmcoreinfo");
+ }
+#endif
+
/* allocate new entry */
entry = kzalloc(sizeof(*entry), GFP_KERNEL);
if (!entry)
--
2.16.1.73.g5832b7e9f2

2018-02-14 20:08:23

by Michael S. Tsirkin

Subject: Re: [PATCH v14 9/9] RFC: fw_cfg: do DMA read operation

On Wed, Feb 14, 2018 at 03:18:50PM +0100, Marc-Andr? Lureau wrote:
> Modify fw_cfg_read_blob() to use DMA if the device supports it.
> Return errors, because the operation may fail.
>
> So far, only one call in fw_cfg_register_dir_entries() is using
> kmalloc'ed buf and is thus clearly eligible to DMA read.
>
> Initially, I didn't implement DMA read to speed up boot time, but as a
> first step before introducing DMA write (since read operations were
> already presents). Even more, I didn't realize fw-cfg entries were
> being read by the kernel during boot by default. But actally fw-cfg
> entries are being populated during module probe. I knew DMA improved a
> lot bios boot time (the main reason the DMA interface was added
> afaik). Let see the time it would take to read the whole ACPI
> tables (128kb allocated)
>
> # time cat /sys/firmware/qemu_fw_cfg/by_name/etc/acpi/tables/raw
> - with DMA: sys 0m0.003s
> - without DMA (-global fw_cfg.dma_enabled=off): sys 0m7.674s
>
> FW_CFG_FILE_DIR (0x19) is the only "file" that is read during kernel
> boot to populate sysfs qemu_fw_cfg directory, and it is quite
> small (1-2kb). Since it does not expose itself, in order to measure
> the time it takes to read such small file, I took a comparable sized
> file of 2048 bytes and exposed it (-fw_cfg test,file=file with a
> modified read_raw enabling DMA)
>
> # perf stat -r 100 cat /sys/firmware/qemu_fw_cfg/by_name/test/raw >/dev/null
> - with DMA:
> 0.636037 task-clock (msec) # 0.141 CPUs utilized ( +- 1.19% )
> - without DMA:
> 6.430128 task-clock (msec) # 0.622 CPUs utilized ( +- 0.22% )
>
> That's a few msec saved during boot by enabling DMA read (the gain
> would be more substantial if other & bigger fw-cfg entries are read by
> others from sysfs, unfortunately, it's not clear if we can always
> enable DMA there)
>
> Signed-off-by: Marc-Andr? Lureau <[email protected]>
> ---
> drivers/firmware/qemu_fw_cfg.c | 80 ++++++++++++++++++++++++++++++++++--------
> 1 file changed, 66 insertions(+), 14 deletions(-)
>
> diff --git a/drivers/firmware/qemu_fw_cfg.c b/drivers/firmware/qemu_fw_cfg.c
> index 69939e2529f2..ba9b907a4399 100644
> --- a/drivers/firmware/qemu_fw_cfg.c
> +++ b/drivers/firmware/qemu_fw_cfg.c
> @@ -124,12 +124,46 @@ static ssize_t fw_cfg_dma_transfer(void *address, u32 length, u32 control)
> return ret;
> }
>
> +/* with acpi & dev locks taken */
> +static ssize_t fw_cfg_read_blob_dma(u16 key,
> + void *buf, loff_t pos, size_t count)
> +{
> + ssize_t ret;
> +
> + if (pos == 0) {
> + ret = fw_cfg_dma_transfer(buf, count, key << 16
> + | FW_CFG_DMA_CTL_SELECT
> + | FW_CFG_DMA_CTL_READ);
> + } else {
> + iowrite16(fw_cfg_sel_endianness(key), fw_cfg_reg_ctrl);
> + ret = fw_cfg_dma_transfer(NULL, pos, FW_CFG_DMA_CTL_SKIP);
> + if (ret < 0)
> + return ret;
> + ret = fw_cfg_dma_transfer(buf, count,
> + FW_CFG_DMA_CTL_READ);
> + }
> +
> + return ret;
> +}
> +
> +/* with acpi & dev locks taken */
> +static void fw_cfg_read_blob_io(u16 key,
> + void *buf, loff_t pos, size_t count)
> +{
> + iowrite16(fw_cfg_sel_endianness(key), fw_cfg_reg_ctrl);
> + while (pos-- > 0)
> + ioread8(fw_cfg_reg_data);
> + ioread8_rep(fw_cfg_reg_data, buf, count);
> +}
> +
> /* read chunk of given fw_cfg blob (caller responsible for sanity-check) */
> -static inline void fw_cfg_read_blob(u16 key,
> - void *buf, loff_t pos, size_t count)
> +static ssize_t fw_cfg_read_blob(u16 key,
> + void *buf, loff_t pos, size_t count,
> + bool dma)
> {
> u32 glk = -1U;
> acpi_status status;
> + ssize_t ret = count;
>
> /* If we have ACPI, ensure mutual exclusion against any potential
> * device access by the firmware, e.g. via AML methods:
> @@ -139,17 +173,21 @@ static inline void fw_cfg_read_blob(u16 key,
> /* Should never get here */
> WARN(1, "fw_cfg_read_blob: Failed to lock ACPI!\n");
> memset(buf, 0, count);
> - return;
> + return -EINVAL;
> }
>
> mutex_lock(&fw_cfg_dev_lock);
> - iowrite16(fw_cfg_sel_endianness(key), fw_cfg_reg_ctrl);
> - while (pos-- > 0)
> - ioread8(fw_cfg_reg_data);
> - ioread8_rep(fw_cfg_reg_data, buf, count);
> + if (dma && fw_cfg_dma_enabled()) {
> + ret = fw_cfg_read_blob_dma(key, buf, pos, count);
> + } else {
> + fw_cfg_read_blob_io(key, buf, pos, count);

I'd set ret = count here.

> + }
> +
> mutex_unlock(&fw_cfg_dev_lock);
>
> acpi_release_global_lock(glk);
> +
> + return ret;
> }
>
> #ifdef CONFIG_CRASH_CORE
> @@ -282,8 +320,9 @@ static int fw_cfg_do_platform_probe(struct platform_device *pdev)
> #endif
>
> /* 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) {
> + if (fw_cfg_read_blob(FW_CFG_SIGNATURE, sig,
> + 0, FW_CFG_SIG_SIZE, false) < 0 ||
> + memcmp(sig, "QEMU", FW_CFG_SIG_SIZE) != 0) {
> fw_cfg_io_cleanup();
> return -ENODEV;
> }

Rather than add dead code, how about a promise not to
fail if dma is disabled? Patch will be smaller then.

> @@ -466,8 +505,8 @@ static ssize_t fw_cfg_sysfs_read_raw(struct file *filp, struct kobject *kobj,
> if (count > entry->size - pos)
> count = entry->size - pos;
>
> - fw_cfg_read_blob(entry->select, buf, pos, count);
> - return count;
> + /* do not use DMA, virt_to_phys(buf) might not be ok */
> + return fw_cfg_read_blob(entry->select, buf, pos, count, false);
> }
>
> static struct bin_attribute fw_cfg_sysfs_attr_raw = {
> @@ -632,7 +671,12 @@ static int fw_cfg_register_dir_entries(void)
> struct fw_cfg_file *dir;
> size_t dir_size;
>
> - fw_cfg_read_blob(FW_CFG_FILE_DIR, &files.count, 0, sizeof(files.count));
> + ret = fw_cfg_read_blob(FW_CFG_FILE_DIR, &files.count,
> + 0, sizeof(files.count), false);
> + if (ret < 0) {
> + return ret;
> + }
> +
> count = be32_to_cpu(files.count);
> dir_size = count * sizeof(struct fw_cfg_file);
>
> @@ -640,7 +684,11 @@ static int fw_cfg_register_dir_entries(void)
> if (!dir)
> return -ENOMEM;
>
> - fw_cfg_read_blob(FW_CFG_FILE_DIR, dir, sizeof(files.count), dir_size);
> + ret = fw_cfg_read_blob(FW_CFG_FILE_DIR, dir,
> + sizeof(files.count), dir_size, false);
> + if (ret < 0) {
> + goto end;
> + }
>
> for (i = 0; i < count; i++) {
> ret = fw_cfg_register_file(&dir[i]);
> @@ -648,6 +696,7 @@ static int fw_cfg_register_dir_entries(void)
> break;
> }
>
> +end:
> kfree(dir);
> return ret;
> }
> @@ -688,7 +737,10 @@ static int fw_cfg_sysfs_probe(struct platform_device *pdev)
> goto err_probe;
>
> /* get revision number, add matching top-level attribute */
> - fw_cfg_read_blob(FW_CFG_ID, &rev, 0, sizeof(rev));
> + err = fw_cfg_read_blob(FW_CFG_ID, &rev, 0, sizeof(rev), false);
> + if (err < 0) {
> + goto err_probe;
> + }
> fw_cfg_rev = le32_to_cpu(rev);
> err = sysfs_create_file(fw_cfg_top_ko, &fw_cfg_rev_attr.attr);
> if (err)
> --
> 2.16.1.73.g5832b7e9f2

2018-02-14 20:09:50

by Michael S. Tsirkin

Subject: Re: [PATCH v14 9/9] RFC: fw_cfg: do DMA read operation

On Wed, Feb 14, 2018 at 06:08:48PM +0100, Marc-Andre Lureau wrote:
> Hi
>
> On Wed, Feb 14, 2018 at 5:59 PM, Michael S. Tsirkin <[email protected]> wrote:
> > On Wed, Feb 14, 2018 at 05:52:10PM +0100, Marc-Andre Lureau wrote:
> >> >> @@ -282,8 +320,9 @@ static int fw_cfg_do_platform_probe(struct platform_device *pdev)
> >> >> #endif
> >> >>
> >> >> /* 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) {
> >> >> + if (fw_cfg_read_blob(FW_CFG_SIGNATURE, sig,
> >> >> + 0, FW_CFG_SIG_SIZE, false) < 0 ||
> >> >> + memcmp(sig, "QEMU", FW_CFG_SIG_SIZE) != 0) {
> >> >> fw_cfg_io_cleanup();
> >> >> return -ENODEV;
> >> >> }
> >> >
> >> > Rather than add dead code, how about a promise not to
> >> > fail if dma is disabled? Patch will be smaller then.
> >>
> >> Even with dma disabled, you could have a locking bug which was
> >> silently ignored before and is now taken into account.
> >
> > I see. I'd start with a patch reporting errors to users then.
> > That would be a bugfix and can be merged for this version.
>
> silently is the wrong word for it, there is already a WARN().

it's not visible to userspace though.

> However,
> the memcmp was done on uninitialzed sig[] (which likely resulted in
> returning -ENODEV in the function)

Oh, we have an access to an uninitialized memory too. Fun!

> Now it can check if the function failed to read before doing the memcmp.
>
> Hope that clarifies it.

I think I get it. I'd make it a bugfix patch as patch 7 in the series.
(patch 1-6 are applied in my tree, will merge in this window if they do not blow up).

> >
> >> >
> >> >> @@ -466,8 +505,8 @@ static ssize_t fw_cfg_sysfs_read_raw(struct file *filp, struct kobject *kobj,
> >> >> if (count > entry->size - pos)
> >> >> count = entry->size - pos;
> >> >>
> >> >> - fw_cfg_read_blob(entry->select, buf, pos, count);
> >> >> - return count;
> >> >> + /* do not use DMA, virt_to_phys(buf) might not be ok */
> >> >> + return fw_cfg_read_blob(entry->select, buf, pos, count, false);
> >> >> }
> >> >>
> >> >> static struct bin_attribute fw_cfg_sysfs_attr_raw = {
> >> >> @@ -632,7 +671,12 @@ static int fw_cfg_register_dir_entries(void)
> >> >> struct fw_cfg_file *dir;
> >> >> size_t dir_size;
> >> >>
> >> >> - fw_cfg_read_blob(FW_CFG_FILE_DIR, &files.count, 0, sizeof(files.count));
> >> >> + ret = fw_cfg_read_blob(FW_CFG_FILE_DIR, &files.count,
> >> >> + 0, sizeof(files.count), false);
> >> >> + if (ret < 0) {
> >> >> + return ret;
> >> >> + }
> >> >> +
> >> >> count = be32_to_cpu(files.count);
> >> >> dir_size = count * sizeof(struct fw_cfg_file);
> >> >>
> >> >> @@ -640,7 +684,11 @@ static int fw_cfg_register_dir_entries(void)
> >> >> if (!dir)
> >> >> return -ENOMEM;
> >> >>
> >> >> - fw_cfg_read_blob(FW_CFG_FILE_DIR, dir, sizeof(files.count), dir_size);
> >> >> + ret = fw_cfg_read_blob(FW_CFG_FILE_DIR, dir,
> >> >> + sizeof(files.count), dir_size, false);
> >> >> + if (ret < 0) {
> >> >> + goto end;
> >> >> + }
> >> >>
> >> >> for (i = 0; i < count; i++) {
> >> >> ret = fw_cfg_register_file(&dir[i]);
> >> >> @@ -648,6 +696,7 @@ static int fw_cfg_register_dir_entries(void)
> >> >> break;
> >> >> }
> >> >>
> >> >> +end:
> >> >> kfree(dir);
> >> >> return ret;
> >> >> }
> >> >> @@ -688,7 +737,10 @@ static int fw_cfg_sysfs_probe(struct platform_device *pdev)
> >> >> goto err_probe;
> >> >>
> >> >> /* get revision number, add matching top-level attribute */
> >> >> - fw_cfg_read_blob(FW_CFG_ID, &rev, 0, sizeof(rev));
> >> >> + err = fw_cfg_read_blob(FW_CFG_ID, &rev, 0, sizeof(rev), false);
> >> >> + if (err < 0) {
> >> >> + goto err_probe;
> >> >> + }
> >> >> fw_cfg_rev = le32_to_cpu(rev);
> >> >> err = sysfs_create_file(fw_cfg_top_ko, &fw_cfg_rev_attr.attr);
> >> >> if (err)
> >> >> --
> >> >> 2.16.1.73.g5832b7e9f2

2018-02-14 20:42:42

by Michael S. Tsirkin

Subject: Re: [PATCH v14 2/9] fw_cfg: add a public uapi header

On Wed, Feb 14, 2018 at 03:18:43PM +0100, Marc-Andr? Lureau wrote:
> Create a common header file for well-known values and structures to be
> shared by the Linux kernel with qemu or other projects.
>
> Suggested-by: Michael S. Tsirkin <[email protected]>
> Signed-off-by: Marc-Andr? Lureau <[email protected]>
>
> ---
>
> The related qemu patch making use of it, to be submitted:
> https://github.com/elmarco/qemu/commit/4884fc9e9c4c4467a371e5a40f3181239e1b70f5
> ---
> MAINTAINERS | 1 +
> drivers/firmware/qemu_fw_cfg.c | 22 +--------
> include/uapi/linux/fw_cfg.h | 102 +++++++++++++++++++++++++++++++++++++++++
> 3 files changed, 105 insertions(+), 20 deletions(-)
> create mode 100644 include/uapi/linux/fw_cfg.h
>
> diff --git a/MAINTAINERS b/MAINTAINERS
> index 3bdc260e36b7..a66b65f62811 100644
> --- a/MAINTAINERS
> +++ b/MAINTAINERS
> @@ -11352,6 +11352,7 @@ M: "Michael S. Tsirkin" <[email protected]>
> L: [email protected]
> S: Maintained
> F: drivers/firmware/qemu_fw_cfg.c
> +F: include/uapi/linux/fw_cfg.h
>
> QIB DRIVER
> M: Dennis Dalessandro <[email protected]>
> diff --git a/drivers/firmware/qemu_fw_cfg.c b/drivers/firmware/qemu_fw_cfg.c
> index a41b572eeeb1..90f467232777 100644
> --- a/drivers/firmware/qemu_fw_cfg.c
> +++ b/drivers/firmware/qemu_fw_cfg.c
> @@ -32,30 +32,12 @@
> #include <linux/slab.h>
> #include <linux/io.h>
> #include <linux/ioport.h>
> +#include <linux/fw_cfg.h>
>
> MODULE_AUTHOR("Gabriel L. Somlo <[email protected]>");
> MODULE_DESCRIPTION("QEMU fw_cfg sysfs support");
> MODULE_LICENSE("GPL");
>
> -/* selector key 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 {
> - u32 size;
> - u16 select;
> - u16 reserved;
> - char name[FW_CFG_MAX_FILE_PATH];
> -};
> -
> /* fw_cfg device i/o register addresses */
> static bool fw_cfg_is_mmio;
> static phys_addr_t fw_cfg_p_base;
> @@ -597,7 +579,7 @@ MODULE_DEVICE_TABLE(of, fw_cfg_sysfs_mmio_match);
>
> #ifdef CONFIG_ACPI
> static const struct acpi_device_id fw_cfg_sysfs_acpi_match[] = {
> - { "QEMU0002", },
> + { FW_CFG_ACPI_DEVICE_ID, },
> {},
> };
> MODULE_DEVICE_TABLE(acpi, fw_cfg_sysfs_acpi_match);
> diff --git a/include/uapi/linux/fw_cfg.h b/include/uapi/linux/fw_cfg.h
> new file mode 100644
> index 000000000000..5b8136ce46ee
> --- /dev/null
> +++ b/include/uapi/linux/fw_cfg.h
> @@ -0,0 +1,102 @@
> +/* SPDX-License-Identifier: GPL-2.0 WITH Linux-syscall-note */
> +#ifndef _LINUX_FW_CFG_H
> +#define _LINUX_FW_CFG_H
> +
> +#include <linux/types.h>
> +
> +#define FW_CFG_ACPI_DEVICE_ID "QEMU0002"
> +
> +/* selector key values for "well-known" fw_cfg entries */
> +#define FW_CFG_SIGNATURE 0x00
> +#define FW_CFG_ID 0x01
> +#define FW_CFG_UUID 0x02
> +#define FW_CFG_RAM_SIZE 0x03
> +#define FW_CFG_NOGRAPHIC 0x04
> +#define FW_CFG_NB_CPUS 0x05
> +#define FW_CFG_MACHINE_ID 0x06
> +#define FW_CFG_KERNEL_ADDR 0x07
> +#define FW_CFG_KERNEL_SIZE 0x08
> +#define FW_CFG_KERNEL_CMDLINE 0x09
> +#define FW_CFG_INITRD_ADDR 0x0a
> +#define FW_CFG_INITRD_SIZE 0x0b
> +#define FW_CFG_BOOT_DEVICE 0x0c
> +#define FW_CFG_NUMA 0x0d
> +#define FW_CFG_BOOT_MENU 0x0e
> +#define FW_CFG_MAX_CPUS 0x0f
> +#define FW_CFG_KERNEL_ENTRY 0x10
> +#define FW_CFG_KERNEL_DATA 0x11
> +#define FW_CFG_INITRD_DATA 0x12
> +#define FW_CFG_CMDLINE_ADDR 0x13
> +#define FW_CFG_CMDLINE_SIZE 0x14
> +#define FW_CFG_CMDLINE_DATA 0x15
> +#define FW_CFG_SETUP_ADDR 0x16
> +#define FW_CFG_SETUP_SIZE 0x17
> +#define FW_CFG_SETUP_DATA 0x18
> +#define FW_CFG_FILE_DIR 0x19
> +
> +#define FW_CFG_FILE_FIRST 0x20
> +#define FW_CFG_FILE_SLOTS_MIN 0x10
> +
> +#define FW_CFG_WRITE_CHANNEL 0x4000
> +#define FW_CFG_ARCH_LOCAL 0x8000
> +#define FW_CFG_ENTRY_MASK (~(FW_CFG_WRITE_CHANNEL | FW_CFG_ARCH_LOCAL))
> +
> +#define FW_CFG_INVALID 0xffff
> +
> +/* width in bytes of fw_cfg control register */
> +#define FW_CFG_CTL_SIZE 0x02
> +
> +/* fw_cfg "file name" is up to 56 characters (including terminating nul) */
> +#define FW_CFG_MAX_FILE_PATH 56
> +
> +/* size in bytes of fw_cfg signature */
> +#define FW_CFG_SIG_SIZE 4
> +
> +/* FW_CFG_ID bits */
> +#define FW_CFG_VERSION 0x01
> +#define FW_CFG_VERSION_DMA 0x02
> +
> +/* fw_cfg file directory entry type */
> +struct fw_cfg_file {
> + __be32 size; /* file size */
> + __be16 select; /* write this to 0x510 to read it */
> + __u16 reserved;
> + char name[FW_CFG_MAX_FILE_PATH];
> +};
> +
> +struct fw_cfg_files {
> + __be32 count; /* number of entries */
> + struct fw_cfg_file f[];
> +};

This struct wasn't there in the original file.
Is the source of these structures QEMU or Linux?
If Linux, then pls split this
1. move code out to a header
2. add more structs

> +
> +/* FW_CFG_DMA_CONTROL bits */
> +#define FW_CFG_DMA_CTL_ERROR 0x01
> +#define FW_CFG_DMA_CTL_READ 0x02
> +#define FW_CFG_DMA_CTL_SKIP 0x04
> +#define FW_CFG_DMA_CTL_SELECT 0x08
> +#define FW_CFG_DMA_CTL_WRITE 0x10
> +
> +#define FW_CFG_DMA_SIGNATURE 0x51454d5520434647ULL /* "QEMU CFG" */
> +
> +/* Control as first field allows for different structures selected by this
> + * field, which might be useful in the future
> + */
> +struct fw_cfg_dma_access {
> + __be32 control;
> + __be32 length;
> + __be64 address;
> +};

Maybe this should be part of the dma patch.

> +
> +#define FW_CFG_VMCOREINFO_FILENAME "etc/vmcoreinfo"
> +
> +#define FW_CFG_VMCOREINFO_FORMAT_NONE 0x0
> +#define FW_CFG_VMCOREINFO_FORMAT_ELF 0x1
> +
> +struct fw_cfg_vmcoreinfo {
> + __le16 host_format;
> + __le16 guest_format;
> + __le32 size;
> + __le64 paddr;
> +};

Maybe this should be part of the vmcore patch.

> +
> +#endif
> --
> 2.16.1.73.g5832b7e9f2

2018-02-14 20:47:43

by Michael S. Tsirkin

Subject: Re: [PATCH v14 3/9] fw_cfg: fix sparse warnings in fw_cfg_sel_endianness()

On Wed, Feb 14, 2018 at 03:18:44PM +0100, Marc-Andr? Lureau wrote:
> The function is used for both LE & BE target type, use __force casting.
>
> Fixes:
> $ make C=1 CF=-D__CHECK_ENDIAN__ drivers/firmware/qemu_fw_cfg.o
>
> drivers/firmware/qemu_fw_cfg.c:55:33: warning: restricted __be16 degrades to integer
> drivers/firmware/qemu_fw_cfg.c:55:52: warning: restricted __le16 degrades to integer
>
> Signed-off-by: Marc-Andr? Lureau <[email protected]>
> ---
> drivers/firmware/qemu_fw_cfg.c | 4 +++-
> 1 file changed, 3 insertions(+), 1 deletion(-)
>
> diff --git a/drivers/firmware/qemu_fw_cfg.c b/drivers/firmware/qemu_fw_cfg.c
> index 90f467232777..85e693287d87 100644
> --- a/drivers/firmware/qemu_fw_cfg.c
> +++ b/drivers/firmware/qemu_fw_cfg.c
> @@ -52,7 +52,9 @@ static DEFINE_MUTEX(fw_cfg_dev_lock);
> /* pick appropriate endianness for selector key */
> static inline u16 fw_cfg_sel_endianness(u16 key)
> {
> - return fw_cfg_is_mmio ? cpu_to_be16(key) : cpu_to_le16(key);
> + return fw_cfg_is_mmio ?
> + (u16 __force)cpu_to_be16(key) :
> + (u16 __force)cpu_to_le16(key);
> }
>
> /* read chunk of given fw_cfg blob (caller responsible for sanity-check) */

Well the caller does cpu_to_le16 on the result ...
All this makes my head spin.

IMHO what you want is a wrapper that does iowrite and iowritebe
rather than __force.


> --
> 2.16.1.73.g5832b7e9f2

2018-02-15 09:26:30

by Marc-Andre Lureau

Subject: Re: [PATCH v14 2/9] fw_cfg: add a public uapi header

Hi

On Wed, Feb 14, 2018 at 9:41 PM, Michael S. Tsirkin <[email protected]> wrote:
> On Wed, Feb 14, 2018 at 03:18:43PM +0100, Marc-André Lureau wrote:
>> Create a common header file for well-known values and structures to be
>> shared by the Linux kernel with qemu or other projects.
>>
>> Suggested-by: Michael S. Tsirkin <[email protected]>
>> Signed-off-by: Marc-André Lureau <[email protected]>
>>
>> ---
>>
>> The related qemu patch making use of it, to be submitted:
>> https://github.com/elmarco/qemu/commit/4884fc9e9c4c4467a371e5a40f3181239e1b70f5
>> ---
>> MAINTAINERS | 1 +
>> drivers/firmware/qemu_fw_cfg.c | 22 +--------
>> include/uapi/linux/fw_cfg.h | 102 +++++++++++++++++++++++++++++++++++++++++
>> 3 files changed, 105 insertions(+), 20 deletions(-)
>> create mode 100644 include/uapi/linux/fw_cfg.h
>>
>> diff --git a/MAINTAINERS b/MAINTAINERS
>> index 3bdc260e36b7..a66b65f62811 100644
>> --- a/MAINTAINERS
>> +++ b/MAINTAINERS
>> @@ -11352,6 +11352,7 @@ M: "Michael S. Tsirkin" <[email protected]>
>> L: [email protected]
>> S: Maintained
>> F: drivers/firmware/qemu_fw_cfg.c
>> +F: include/uapi/linux/fw_cfg.h
>>
>> QIB DRIVER
>> M: Dennis Dalessandro <[email protected]>
>> diff --git a/drivers/firmware/qemu_fw_cfg.c b/drivers/firmware/qemu_fw_cfg.c
>> index a41b572eeeb1..90f467232777 100644
>> --- a/drivers/firmware/qemu_fw_cfg.c
>> +++ b/drivers/firmware/qemu_fw_cfg.c
>> @@ -32,30 +32,12 @@
>> #include <linux/slab.h>
>> #include <linux/io.h>
>> #include <linux/ioport.h>
>> +#include <linux/fw_cfg.h>
>>
>> MODULE_AUTHOR("Gabriel L. Somlo <[email protected]>");
>> MODULE_DESCRIPTION("QEMU fw_cfg sysfs support");
>> MODULE_LICENSE("GPL");
>>
>> -/* selector key 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 {
>> - u32 size;
>> - u16 select;
>> - u16 reserved;
>> - char name[FW_CFG_MAX_FILE_PATH];
>> -};
>> -
>> /* fw_cfg device i/o register addresses */
>> static bool fw_cfg_is_mmio;
>> static phys_addr_t fw_cfg_p_base;
>> @@ -597,7 +579,7 @@ MODULE_DEVICE_TABLE(of, fw_cfg_sysfs_mmio_match);
>>
>> #ifdef CONFIG_ACPI
>> static const struct acpi_device_id fw_cfg_sysfs_acpi_match[] = {
>> - { "QEMU0002", },
>> + { FW_CFG_ACPI_DEVICE_ID, },
>> {},
>> };
>> MODULE_DEVICE_TABLE(acpi, fw_cfg_sysfs_acpi_match);
>> diff --git a/include/uapi/linux/fw_cfg.h b/include/uapi/linux/fw_cfg.h
>> new file mode 100644
>> index 000000000000..5b8136ce46ee
>> --- /dev/null
>> +++ b/include/uapi/linux/fw_cfg.h
>> @@ -0,0 +1,102 @@
>> +/* SPDX-License-Identifier: GPL-2.0 WITH Linux-syscall-note */
>> +#ifndef _LINUX_FW_CFG_H
>> +#define _LINUX_FW_CFG_H
>> +
>> +#include <linux/types.h>
>> +
>> +#define FW_CFG_ACPI_DEVICE_ID "QEMU0002"
>> +
>> +/* selector key values for "well-known" fw_cfg entries */
>> +#define FW_CFG_SIGNATURE 0x00
>> +#define FW_CFG_ID 0x01
>> +#define FW_CFG_UUID 0x02
>> +#define FW_CFG_RAM_SIZE 0x03
>> +#define FW_CFG_NOGRAPHIC 0x04
>> +#define FW_CFG_NB_CPUS 0x05
>> +#define FW_CFG_MACHINE_ID 0x06
>> +#define FW_CFG_KERNEL_ADDR 0x07
>> +#define FW_CFG_KERNEL_SIZE 0x08
>> +#define FW_CFG_KERNEL_CMDLINE 0x09
>> +#define FW_CFG_INITRD_ADDR 0x0a
>> +#define FW_CFG_INITRD_SIZE 0x0b
>> +#define FW_CFG_BOOT_DEVICE 0x0c
>> +#define FW_CFG_NUMA 0x0d
>> +#define FW_CFG_BOOT_MENU 0x0e
>> +#define FW_CFG_MAX_CPUS 0x0f
>> +#define FW_CFG_KERNEL_ENTRY 0x10
>> +#define FW_CFG_KERNEL_DATA 0x11
>> +#define FW_CFG_INITRD_DATA 0x12
>> +#define FW_CFG_CMDLINE_ADDR 0x13
>> +#define FW_CFG_CMDLINE_SIZE 0x14
>> +#define FW_CFG_CMDLINE_DATA 0x15
>> +#define FW_CFG_SETUP_ADDR 0x16
>> +#define FW_CFG_SETUP_SIZE 0x17
>> +#define FW_CFG_SETUP_DATA 0x18
>> +#define FW_CFG_FILE_DIR 0x19
>> +
>> +#define FW_CFG_FILE_FIRST 0x20
>> +#define FW_CFG_FILE_SLOTS_MIN 0x10
>> +
>> +#define FW_CFG_WRITE_CHANNEL 0x4000
>> +#define FW_CFG_ARCH_LOCAL 0x8000
>> +#define FW_CFG_ENTRY_MASK (~(FW_CFG_WRITE_CHANNEL | FW_CFG_ARCH_LOCAL))
>> +
>> +#define FW_CFG_INVALID 0xffff
>> +
>> +/* width in bytes of fw_cfg control register */
>> +#define FW_CFG_CTL_SIZE 0x02
>> +
>> +/* fw_cfg "file name" is up to 56 characters (including terminating nul) */
>> +#define FW_CFG_MAX_FILE_PATH 56
>> +
>> +/* size in bytes of fw_cfg signature */
>> +#define FW_CFG_SIG_SIZE 4
>> +
>> +/* FW_CFG_ID bits */
>> +#define FW_CFG_VERSION 0x01
>> +#define FW_CFG_VERSION_DMA 0x02
>> +
>> +/* fw_cfg file directory entry type */
>> +struct fw_cfg_file {
>> + __be32 size; /* file size */
>> + __be16 select; /* write this to 0x510 to read it */
>> + __u16 reserved;
>> + char name[FW_CFG_MAX_FILE_PATH];
>> +};
>> +
>> +struct fw_cfg_files {
>> + __be32 count; /* number of entries */
>> + struct fw_cfg_file f[];
>> +};
>
> This struct wasn't there in the original file.
> Is the source of these structures QEMU or Linux?
> If Linux, then pls split this
> 1. move code out to a header
> 2. add more structs

It is based from Linux source + qemu docs/specs/fw_cfg.txt which
references include/hw/nvram/fw_cfg_keys.h "for the most up-to-date and
authoritative list" & vmcoreinfo.txt.

QEMU ./include/hw/nvram/fw_cfg_keys.h doesn't have specific licence
text, and QEMU as a whole is released under the GNU General Public
License, version 2.

Let me know if I still need to change the license and what you
recommend instead.

>
>> +
>> +/* FW_CFG_DMA_CONTROL bits */
>> +#define FW_CFG_DMA_CTL_ERROR 0x01
>> +#define FW_CFG_DMA_CTL_READ 0x02
>> +#define FW_CFG_DMA_CTL_SKIP 0x04
>> +#define FW_CFG_DMA_CTL_SELECT 0x08
>> +#define FW_CFG_DMA_CTL_WRITE 0x10
>> +
>> +#define FW_CFG_DMA_SIGNATURE 0x51454d5520434647ULL /* "QEMU CFG" */
>> +
>> +/* Control as first field allows for different structures selected by this
>> + * field, which might be useful in the future
>> + */
>> +struct fw_cfg_dma_access {
>> + __be32 control;
>> + __be32 length;
>> + __be64 address;
>> +};
>
> Maybe this should be part of the dma patch.

I prefer to introduce all the spec defines in one patch, but I can
split it if it helps.

>
>> +
>> +#define FW_CFG_VMCOREINFO_FILENAME "etc/vmcoreinfo"
>> +
>> +#define FW_CFG_VMCOREINFO_FORMAT_NONE 0x0
>> +#define FW_CFG_VMCOREINFO_FORMAT_ELF 0x1
>> +
>> +struct fw_cfg_vmcoreinfo {
>> + __le16 host_format;
>> + __le16 guest_format;
>> + __le32 size;
>> + __le64 paddr;
>> +};
>
> Maybe this should be part of the vmcore patch.

same


Thanks

2018-02-15 09:30:34

by Marc-Andre Lureau

Subject: Re: [PATCH v14 2/9] fw_cfg: add a public uapi header

Hi

On Wed, Feb 14, 2018 at 8:37 PM, Michael S. Tsirkin <[email protected]> wrote:
> On Wed, Feb 14, 2018 at 03:18:43PM +0100, Marc-André Lureau wrote:
>> Create a common header file for well-known values and structures to be
>> shared by the Linux kernel with qemu or other projects.
>>
>> Suggested-by: Michael S. Tsirkin <[email protected]>
>> Signed-off-by: Marc-André Lureau <[email protected]>
>>
>> ---
>>
>> The related qemu patch making use of it, to be submitted:
>> https://github.com/elmarco/qemu/commit/4884fc9e9c4c4467a371e5a40f3181239e1b70f5
>> ---
>> MAINTAINERS | 1 +
>> drivers/firmware/qemu_fw_cfg.c | 22 +--------
>> include/uapi/linux/fw_cfg.h | 102 +++++++++++++++++++++++++++++++++++++++++
>> 3 files changed, 105 insertions(+), 20 deletions(-)
>> create mode 100644 include/uapi/linux/fw_cfg.h
>>
>> diff --git a/MAINTAINERS b/MAINTAINERS
>> index 3bdc260e36b7..a66b65f62811 100644
>> --- a/MAINTAINERS
>> +++ b/MAINTAINERS
>> @@ -11352,6 +11352,7 @@ M: "Michael S. Tsirkin" <[email protected]>
>> L: [email protected]
>> S: Maintained
>> F: drivers/firmware/qemu_fw_cfg.c
>> +F: include/uapi/linux/fw_cfg.h
>>
>> QIB DRIVER
>> M: Dennis Dalessandro <[email protected]>
>> diff --git a/drivers/firmware/qemu_fw_cfg.c b/drivers/firmware/qemu_fw_cfg.c
>> index a41b572eeeb1..90f467232777 100644
>> --- a/drivers/firmware/qemu_fw_cfg.c
>> +++ b/drivers/firmware/qemu_fw_cfg.c
>> @@ -32,30 +32,12 @@
>> #include <linux/slab.h>
>> #include <linux/io.h>
>> #include <linux/ioport.h>
>> +#include <linux/fw_cfg.h>
>
> You include the header from include/linux/fw_cfg.h ...
>
>>
>> MODULE_AUTHOR("Gabriel L. Somlo <[email protected]>");
>> MODULE_DESCRIPTION("QEMU fw_cfg sysfs support");
>> MODULE_LICENSE("GPL");
>>
>> -/* selector key 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 {
>> - u32 size;
>> - u16 select;
>> - u16 reserved;
>> - char name[FW_CFG_MAX_FILE_PATH];
>> -};
>> -
>> /* fw_cfg device i/o register addresses */
>> static bool fw_cfg_is_mmio;
>> static phys_addr_t fw_cfg_p_base;
>> @@ -597,7 +579,7 @@ MODULE_DEVICE_TABLE(of, fw_cfg_sysfs_mmio_match);
>>
>> #ifdef CONFIG_ACPI
>> static const struct acpi_device_id fw_cfg_sysfs_acpi_match[] = {
>> - { "QEMU0002", },
>> + { FW_CFG_ACPI_DEVICE_ID, },
>> {},
>> };
>> MODULE_DEVICE_TABLE(acpi, fw_cfg_sysfs_acpi_match);
>> diff --git a/include/uapi/linux/fw_cfg.h b/include/uapi/linux/fw_cfg.h
>> new file mode 100644
>> index 000000000000..5b8136ce46ee
>> --- /dev/null
>> +++ b/include/uapi/linux/fw_cfg.h
>
> Yet the header is located in include/uapi/linux/fw_cfg.h
>
> How can this work?
>

$ make drivers/firmware/qemu_fw_cfg.ko V=1

gcc -Wp,-MD,drivers/firmware/.qemu_fw_cfg.o.d -nostdinc -isystem
/usr/lib/gcc/x86_64-redhat-linux/7/include -I./arch/x86/include
-I./arch/x86/include/generated -I./include -I./arch/x86/include/uapi
...

>> @@ -0,0 +1,102 @@
>> +/* SPDX-License-Identifier: GPL-2.0 WITH Linux-syscall-note */
>
> As far as I can see these have all been lifted from a BSD-licensed
> hw/nvram/fw_cfg.c in qemu. So please make it BSD accordingly,
> and include the explanation in the commit log.
>

(see other reply)

>> +#ifndef _LINUX_FW_CFG_H
>> +#define _LINUX_FW_CFG_H
>> +
>> +#include <linux/types.h>
>> +
>> +#define FW_CFG_ACPI_DEVICE_ID "QEMU0002"
>> +
>> +/* selector key values for "well-known" fw_cfg entries */
>> +#define FW_CFG_SIGNATURE 0x00
>> +#define FW_CFG_ID 0x01
>> +#define FW_CFG_UUID 0x02
>> +#define FW_CFG_RAM_SIZE 0x03
>> +#define FW_CFG_NOGRAPHIC 0x04
>> +#define FW_CFG_NB_CPUS 0x05
>> +#define FW_CFG_MACHINE_ID 0x06
>> +#define FW_CFG_KERNEL_ADDR 0x07
>> +#define FW_CFG_KERNEL_SIZE 0x08
>> +#define FW_CFG_KERNEL_CMDLINE 0x09
>> +#define FW_CFG_INITRD_ADDR 0x0a
>> +#define FW_CFG_INITRD_SIZE 0x0b
>> +#define FW_CFG_BOOT_DEVICE 0x0c
>> +#define FW_CFG_NUMA 0x0d
>> +#define FW_CFG_BOOT_MENU 0x0e
>> +#define FW_CFG_MAX_CPUS 0x0f
>> +#define FW_CFG_KERNEL_ENTRY 0x10
>> +#define FW_CFG_KERNEL_DATA 0x11
>> +#define FW_CFG_INITRD_DATA 0x12
>> +#define FW_CFG_CMDLINE_ADDR 0x13
>> +#define FW_CFG_CMDLINE_SIZE 0x14
>> +#define FW_CFG_CMDLINE_DATA 0x15
>> +#define FW_CFG_SETUP_ADDR 0x16
>> +#define FW_CFG_SETUP_SIZE 0x17
>> +#define FW_CFG_SETUP_DATA 0x18
>> +#define FW_CFG_FILE_DIR 0x19
>> +
>> +#define FW_CFG_FILE_FIRST 0x20
>> +#define FW_CFG_FILE_SLOTS_MIN 0x10
>> +
>> +#define FW_CFG_WRITE_CHANNEL 0x4000
>> +#define FW_CFG_ARCH_LOCAL 0x8000
>> +#define FW_CFG_ENTRY_MASK (~(FW_CFG_WRITE_CHANNEL | FW_CFG_ARCH_LOCAL))
>> +
>> +#define FW_CFG_INVALID 0xffff
>> +
>> +/* width in bytes of fw_cfg control register */
>> +#define FW_CFG_CTL_SIZE 0x02
>> +
>> +/* fw_cfg "file name" is up to 56 characters (including terminating nul) */
>> +#define FW_CFG_MAX_FILE_PATH 56
>> +
>> +/* size in bytes of fw_cfg signature */
>> +#define FW_CFG_SIG_SIZE 4
>> +
>> +/* FW_CFG_ID bits */
>> +#define FW_CFG_VERSION 0x01
>> +#define FW_CFG_VERSION_DMA 0x02
>> +
>> +/* fw_cfg file directory entry type */
>> +struct fw_cfg_file {
>> + __be32 size; /* file size */
>> + __be16 select; /* write this to 0x510 to read it */
>> + __u16 reserved;
>> + char name[FW_CFG_MAX_FILE_PATH];
>> +};
>> +
>> +struct fw_cfg_files {
>> + __be32 count; /* number of entries */
>> + struct fw_cfg_file f[];
>> +};
>> +
>> +/* FW_CFG_DMA_CONTROL bits */
>> +#define FW_CFG_DMA_CTL_ERROR 0x01
>> +#define FW_CFG_DMA_CTL_READ 0x02
>> +#define FW_CFG_DMA_CTL_SKIP 0x04
>> +#define FW_CFG_DMA_CTL_SELECT 0x08
>> +#define FW_CFG_DMA_CTL_WRITE 0x10
>> +
>> +#define FW_CFG_DMA_SIGNATURE 0x51454d5520434647ULL /* "QEMU CFG" */
>> +
>> +/* Control as first field allows for different structures selected by this
>> + * field, which might be useful in the future
>> + */
>> +struct fw_cfg_dma_access {
>> + __be32 control;
>> + __be32 length;
>> + __be64 address;
>> +};
>> +
>> +#define FW_CFG_VMCOREINFO_FILENAME "etc/vmcoreinfo"
>> +
>> +#define FW_CFG_VMCOREINFO_FORMAT_NONE 0x0
>> +#define FW_CFG_VMCOREINFO_FORMAT_ELF 0x1
>> +
>> +struct fw_cfg_vmcoreinfo {
>> + __le16 host_format;
>> + __le16 guest_format;
>> + __le32 size;
>> + __le64 paddr;
>> +};
>> +
>> +#endif
>> --
>> 2.16.1.73.g5832b7e9f2

2018-02-15 09:35:28

by Marc-Andre Lureau

Subject: Re: [PATCH v14 3/9] fw_cfg: fix sparse warnings in fw_cfg_sel_endianness()

On Wed, Feb 14, 2018 at 9:46 PM, Michael S. Tsirkin <[email protected]> wrote:
> On Wed, Feb 14, 2018 at 03:18:44PM +0100, Marc-André Lureau wrote:
>> The function is used for both LE & BE target type, use __force casting.
>>
>> Fixes:
>> $ make C=1 CF=-D__CHECK_ENDIAN__ drivers/firmware/qemu_fw_cfg.o
>>
>> drivers/firmware/qemu_fw_cfg.c:55:33: warning: restricted __be16 degrades to integer
>> drivers/firmware/qemu_fw_cfg.c:55:52: warning: restricted __le16 degrades to integer
>>
>> Signed-off-by: Marc-André Lureau <[email protected]>
>> ---
>> drivers/firmware/qemu_fw_cfg.c | 4 +++-
>> 1 file changed, 3 insertions(+), 1 deletion(-)
>>
>> diff --git a/drivers/firmware/qemu_fw_cfg.c b/drivers/firmware/qemu_fw_cfg.c
>> index 90f467232777..85e693287d87 100644
>> --- a/drivers/firmware/qemu_fw_cfg.c
>> +++ b/drivers/firmware/qemu_fw_cfg.c
>> @@ -52,7 +52,9 @@ static DEFINE_MUTEX(fw_cfg_dev_lock);
>> /* pick appropriate endianness for selector key */
>> static inline u16 fw_cfg_sel_endianness(u16 key)
>> {
>> - return fw_cfg_is_mmio ? cpu_to_be16(key) : cpu_to_le16(key);
>> + return fw_cfg_is_mmio ?
>> + (u16 __force)cpu_to_be16(key) :
>> + (u16 __force)cpu_to_le16(key);
>> }
>>
>> /* read chunk of given fw_cfg blob (caller responsible for sanity-check) */
>
> Well the caller does cpu_to_le16 on the result ...
> All this makes my head spin.
>
> IMHO what you want is a wrapper that does iowrite and iowritebe
> rather than __force.

iowrite16(key) is the same as iowrite16(cpu_to_le16(key)) ? There is
no iowrite16le()...

Is this equivalent, and not introducing regressions?

static inline u16 fw_cfg_sel_endianness(u16 key)
+static void fw_cfg_sel_endianness(u16 key)
{
- return fw_cfg_is_mmio ? cpu_to_be16(key) : cpu_to_le16(key);
+ if (fw_cfg_is_mmio)
+ iowrite16be(key, fw_cfg_reg_ctrl);
+ else
+ iowrite16(key, fw_cfg_reg_ctrl);
}

/* read chunk of given fw_cfg blob (caller responsible for sanity-check) */
@@ -74,7 +77,7 @@ static inline void fw_cfg_read_blob(u16 key,
}

mutex_lock(&fw_cfg_dev_lock);
- iowrite16(fw_cfg_sel_endianness(key), fw_cfg_reg_ctrl);
+ fw_cfg_sel_endianness(key);

>
>
>> --
>> 2.16.1.73.g5832b7e9f2

2018-02-15 18:12:50

by Michael S. Tsirkin

Subject: Re: [PATCH v14 8/9] fw_cfg: write vmcoreinfo details

On Wed, Feb 14, 2018 at 03:18:49PM +0100, Marc-Andr? Lureau wrote:
> If the "etc/vmcoreinfo" fw_cfg file is present and we are not running
> the kdump kernel, write the addr/size of the vmcoreinfo ELF note.
>
> The DMA operation is expected to run synchronously with today qemu,
> but the specification states that it may become async, so we run
> "control" field check in a loop for eventual changes.
>
> Signed-off-by: Marc-Andr? Lureau <[email protected]>
> ---
> drivers/firmware/qemu_fw_cfg.c | 144 ++++++++++++++++++++++++++++++++++++++++-
> 1 file changed, 141 insertions(+), 3 deletions(-)
>
> diff --git a/drivers/firmware/qemu_fw_cfg.c b/drivers/firmware/qemu_fw_cfg.c
> index 37638b95cb45..69939e2529f2 100644
> --- a/drivers/firmware/qemu_fw_cfg.c
> +++ b/drivers/firmware/qemu_fw_cfg.c
> @@ -34,11 +34,17 @@
> #include <linux/io.h>
> #include <linux/ioport.h>
> #include <linux/fw_cfg.h>
> +#include <linux/delay.h>
> +#include <linux/crash_dump.h>
> +#include <linux/crash_core.h>
>
> MODULE_AUTHOR("Gabriel L. Somlo <[email protected]>");
> MODULE_DESCRIPTION("QEMU fw_cfg sysfs support");
> MODULE_LICENSE("GPL");
>
> +/* fw_cfg revision attribute, in /sys/firmware/qemu_fw_cfg top-level dir. */
> +static u32 fw_cfg_rev;
> +
> /* fw_cfg device i/o register addresses */
> static bool fw_cfg_is_mmio;
> static phys_addr_t fw_cfg_p_base;
> @@ -59,6 +65,65 @@ static inline u16 fw_cfg_sel_endianness(u16 key)
> (u16 __force)cpu_to_le16(key);
> }
>
> +static inline bool fw_cfg_dma_enabled(void)
> +{
> + return (fw_cfg_rev & FW_CFG_VERSION_DMA) && fw_cfg_reg_dma;
> +}
> +
> +/* qemu fw_cfg device is sync today, but spec says it may become async */
> +static void fw_cfg_wait_for_control(struct fw_cfg_dma_access *d)
> +{
> + do {
> + u32 ctrl = be32_to_cpu(READ_ONCE(d->control));
> +
> + if ((ctrl & ~FW_CFG_DMA_CTL_ERROR) == 0)
> + return;
> +
> + usleep_range(50, 100);

I would just do cpu_relax() here.

> + } while (true);
> +
> + /* do not reorder the read to d->control */
> + rmb();

Hmm. I don't really understand the comment.
Is this code ever reacheable? How does it help?

> +}
> +
> +static ssize_t fw_cfg_dma_transfer(void *address, u32 length, u32 control)
> +{
> + phys_addr_t dma;
> + struct fw_cfg_dma_access *d = NULL;
> + ssize_t ret = length;
> +
> + d = kmalloc(sizeof(*d), GFP_KERNEL);
> + if (!d) {
> + ret = -ENOMEM;
> + goto end;
> + }
> +
> + /* fw_cfg device does not need IOMMU protection, so use physical addresses */
> + *d = (struct fw_cfg_dma_access) {
> + .address = cpu_to_be64(address ? virt_to_phys(address) : 0),
> + .length = cpu_to_be32(length),
> + .control = cpu_to_be32(control)
> + };
> +
> + dma = virt_to_phys(d);
> +
> + iowrite32be((u64)dma >> 32, fw_cfg_reg_dma);
> + /* force memory to sync before notifying device via MMIO */
> + wmb();
> + iowrite32be(dma, fw_cfg_reg_dma + 4);
> +
> + fw_cfg_wait_for_control(d);
> +
> + if (be32_to_cpu(READ_ONCE(d->control)) & FW_CFG_DMA_CTL_ERROR) {
> + ret = -EIO;
> + }
> +
> +end:
> + kfree(d);
> +
> + return ret;
> +}
> +
> /* read chunk of given fw_cfg blob (caller responsible for sanity-check) */
> static inline void fw_cfg_read_blob(u16 key,
> void *buf, loff_t pos, size_t count)
> @@ -87,6 +152,47 @@ static inline void fw_cfg_read_blob(u16 key,
> acpi_release_global_lock(glk);
> }
>
> +#ifdef CONFIG_CRASH_CORE
> +/* write chunk of given fw_cfg blob (caller responsible for sanity-check) */
> +static ssize_t fw_cfg_write_blob(u16 key,
> + void *buf, loff_t pos, size_t count)
> +{
> + u32 glk = -1U;
> + acpi_status status;
> + ssize_t ret = count;
> +
> + /* If we have ACPI, ensure mutual exclusion against any potential
> + * device access by the firmware, e.g. via AML methods:
> + */
> + status = acpi_acquire_global_lock(ACPI_WAIT_FOREVER, &glk);
> + if (ACPI_FAILURE(status) && status != AE_NOT_CONFIGURED) {
> + /* Should never get here */
> + WARN(1, "%s: Failed to lock ACPI!\n", __func__);
> + return -EINVAL;
> + }
> +
> + mutex_lock(&fw_cfg_dev_lock);
> + if (pos == 0) {
> + ret = fw_cfg_dma_transfer(buf, count, key << 16
> + | FW_CFG_DMA_CTL_SELECT
> + | FW_CFG_DMA_CTL_WRITE);
> + } else {
> + iowrite16(fw_cfg_sel_endianness(key), fw_cfg_reg_ctrl);
> + ret = fw_cfg_dma_transfer(NULL, pos, FW_CFG_DMA_CTL_SKIP);
> + if (ret < 0)
> + goto end;
> + ret = fw_cfg_dma_transfer(buf, count, FW_CFG_DMA_CTL_WRITE);
> + }
> +
> +end:
> + mutex_unlock(&fw_cfg_dev_lock);
> +
> + acpi_release_global_lock(glk);
> +
> + return ret;
> +}
> +#endif /* CONFIG_CRASH_CORE */
> +
> /* clean up fw_cfg device i/o */
> static void fw_cfg_io_cleanup(void)
> {
> @@ -185,9 +291,6 @@ static int fw_cfg_do_platform_probe(struct platform_device *pdev)
> return 0;
> }
>
> -/* fw_cfg revision attribute, in /sys/firmware/qemu_fw_cfg top-level dir. */
> -static u32 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);
> @@ -210,6 +313,32 @@ struct fw_cfg_sysfs_entry {
> struct list_head list;
> };
>
> +#ifdef CONFIG_CRASH_CORE
> +static ssize_t fw_cfg_write_vmcoreinfo(const struct fw_cfg_file *f)
> +{
> + static struct fw_cfg_vmcoreinfo *data;
> + ssize_t ret;
> +
> + data = kmalloc(sizeof(struct fw_cfg_vmcoreinfo), GFP_KERNEL);
> + if (!data)
> + return -ENOMEM;
> +
> + *data = (struct fw_cfg_vmcoreinfo) {
> + .guest_format = cpu_to_le16(FW_CFG_VMCOREINFO_FORMAT_ELF),
> + .size = cpu_to_le32(VMCOREINFO_NOTE_SIZE),
> + .paddr = cpu_to_le64(paddr_vmcoreinfo_note())
> + };
> + /* spare ourself reading host format support for now since we
> + * don't know what else to format - host may ignore ours
> + */
> + ret = fw_cfg_write_blob(be16_to_cpu(f->select), data,
> + 0, sizeof(struct fw_cfg_vmcoreinfo));
> +
> + kfree(data);
> + return ret;
> +}
> +#endif /* CONFIG_CRASH_CORE */
> +
> /* get fw_cfg_sysfs_entry from kobject member */
> static inline struct fw_cfg_sysfs_entry *to_entry(struct kobject *kobj)
> {
> @@ -450,6 +579,15 @@ static int fw_cfg_register_file(const struct fw_cfg_file *f)
> int err;
> struct fw_cfg_sysfs_entry *entry;
>
> +#ifdef CONFIG_CRASH_CORE
> + if (fw_cfg_dma_enabled() &&
> + strcmp(f->name, FW_CFG_VMCOREINFO_FILENAME) == 0 &&
> + !is_kdump_kernel()) {
> + if (fw_cfg_write_vmcoreinfo(f) < 0)
> + pr_warn("fw_cfg: failed to write vmcoreinfo");
> + }
> +#endif
> +
> /* allocate new entry */
> entry = kzalloc(sizeof(*entry), GFP_KERNEL);
> if (!entry)
> --
> 2.16.1.73.g5832b7e9f2

2018-02-15 18:22:17

by Michael S. Tsirkin

Subject: Re: [PATCH v14 2/9] fw_cfg: add a public uapi header

On Thu, Feb 15, 2018 at 10:25:27AM +0100, Marc-Andre Lureau wrote:
> Hi
>
> On Wed, Feb 14, 2018 at 9:41 PM, Michael S. Tsirkin <[email protected]> wrote:
> > On Wed, Feb 14, 2018 at 03:18:43PM +0100, Marc-Andr? Lureau wrote:
> >> Create a common header file for well-known values and structures to be
> >> shared by the Linux kernel with qemu or other projects.
> >>
> >> Suggested-by: Michael S. Tsirkin <[email protected]>
> >> Signed-off-by: Marc-Andr? Lureau <[email protected]>
> >>
> >> ---
> >>
> >> The related qemu patch making use of it, to be submitted:
> >> https://github.com/elmarco/qemu/commit/4884fc9e9c4c4467a371e5a40f3181239e1b70f5
> >> ---
> >> MAINTAINERS | 1 +
> >> drivers/firmware/qemu_fw_cfg.c | 22 +--------
> >> include/uapi/linux/fw_cfg.h | 102 +++++++++++++++++++++++++++++++++++++++++
> >> 3 files changed, 105 insertions(+), 20 deletions(-)
> >> create mode 100644 include/uapi/linux/fw_cfg.h
> >>
> >> diff --git a/MAINTAINERS b/MAINTAINERS
> >> index 3bdc260e36b7..a66b65f62811 100644
> >> --- a/MAINTAINERS
> >> +++ b/MAINTAINERS
> >> @@ -11352,6 +11352,7 @@ M: "Michael S. Tsirkin" <[email protected]>
> >> L: [email protected]
> >> S: Maintained
> >> F: drivers/firmware/qemu_fw_cfg.c
> >> +F: include/uapi/linux/fw_cfg.h
> >>
> >> QIB DRIVER
> >> M: Dennis Dalessandro <[email protected]>
> >> diff --git a/drivers/firmware/qemu_fw_cfg.c b/drivers/firmware/qemu_fw_cfg.c
> >> index a41b572eeeb1..90f467232777 100644
> >> --- a/drivers/firmware/qemu_fw_cfg.c
> >> +++ b/drivers/firmware/qemu_fw_cfg.c
> >> @@ -32,30 +32,12 @@
> >> #include <linux/slab.h>
> >> #include <linux/io.h>
> >> #include <linux/ioport.h>
> >> +#include <linux/fw_cfg.h>
> >>
> >> MODULE_AUTHOR("Gabriel L. Somlo <[email protected]>");
> >> MODULE_DESCRIPTION("QEMU fw_cfg sysfs support");
> >> MODULE_LICENSE("GPL");
> >>
> >> -/* selector key 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 {
> >> - u32 size;
> >> - u16 select;
> >> - u16 reserved;
> >> - char name[FW_CFG_MAX_FILE_PATH];
> >> -};
> >> -
> >> /* fw_cfg device i/o register addresses */
> >> static bool fw_cfg_is_mmio;
> >> static phys_addr_t fw_cfg_p_base;
> >> @@ -597,7 +579,7 @@ MODULE_DEVICE_TABLE(of, fw_cfg_sysfs_mmio_match);
> >>
> >> #ifdef CONFIG_ACPI
> >> static const struct acpi_device_id fw_cfg_sysfs_acpi_match[] = {
> >> - { "QEMU0002", },
> >> + { FW_CFG_ACPI_DEVICE_ID, },
> >> {},
> >> };
> >> MODULE_DEVICE_TABLE(acpi, fw_cfg_sysfs_acpi_match);
> >> diff --git a/include/uapi/linux/fw_cfg.h b/include/uapi/linux/fw_cfg.h
> >> new file mode 100644
> >> index 000000000000..5b8136ce46ee
> >> --- /dev/null
> >> +++ b/include/uapi/linux/fw_cfg.h
> >> @@ -0,0 +1,102 @@
> >> +/* SPDX-License-Identifier: GPL-2.0 WITH Linux-syscall-note */
> >> +#ifndef _LINUX_FW_CFG_H
> >> +#define _LINUX_FW_CFG_H
> >> +
> >> +#include <linux/types.h>
> >> +
> >> +#define FW_CFG_ACPI_DEVICE_ID "QEMU0002"
> >> +
> >> +/* selector key values for "well-known" fw_cfg entries */
> >> +#define FW_CFG_SIGNATURE 0x00
> >> +#define FW_CFG_ID 0x01
> >> +#define FW_CFG_UUID 0x02
> >> +#define FW_CFG_RAM_SIZE 0x03
> >> +#define FW_CFG_NOGRAPHIC 0x04
> >> +#define FW_CFG_NB_CPUS 0x05
> >> +#define FW_CFG_MACHINE_ID 0x06
> >> +#define FW_CFG_KERNEL_ADDR 0x07
> >> +#define FW_CFG_KERNEL_SIZE 0x08
> >> +#define FW_CFG_KERNEL_CMDLINE 0x09
> >> +#define FW_CFG_INITRD_ADDR 0x0a
> >> +#define FW_CFG_INITRD_SIZE 0x0b
> >> +#define FW_CFG_BOOT_DEVICE 0x0c
> >> +#define FW_CFG_NUMA 0x0d
> >> +#define FW_CFG_BOOT_MENU 0x0e
> >> +#define FW_CFG_MAX_CPUS 0x0f
> >> +#define FW_CFG_KERNEL_ENTRY 0x10
> >> +#define FW_CFG_KERNEL_DATA 0x11
> >> +#define FW_CFG_INITRD_DATA 0x12
> >> +#define FW_CFG_CMDLINE_ADDR 0x13
> >> +#define FW_CFG_CMDLINE_SIZE 0x14
> >> +#define FW_CFG_CMDLINE_DATA 0x15
> >> +#define FW_CFG_SETUP_ADDR 0x16
> >> +#define FW_CFG_SETUP_SIZE 0x17
> >> +#define FW_CFG_SETUP_DATA 0x18
> >> +#define FW_CFG_FILE_DIR 0x19
> >> +
> >> +#define FW_CFG_FILE_FIRST 0x20
> >> +#define FW_CFG_FILE_SLOTS_MIN 0x10
> >> +
> >> +#define FW_CFG_WRITE_CHANNEL 0x4000
> >> +#define FW_CFG_ARCH_LOCAL 0x8000
> >> +#define FW_CFG_ENTRY_MASK (~(FW_CFG_WRITE_CHANNEL | FW_CFG_ARCH_LOCAL))
> >> +
> >> +#define FW_CFG_INVALID 0xffff
> >> +
> >> +/* width in bytes of fw_cfg control register */
> >> +#define FW_CFG_CTL_SIZE 0x02
> >> +
> >> +/* fw_cfg "file name" is up to 56 characters (including terminating nul) */
> >> +#define FW_CFG_MAX_FILE_PATH 56
> >> +
> >> +/* size in bytes of fw_cfg signature */
> >> +#define FW_CFG_SIG_SIZE 4
> >> +
> >> +/* FW_CFG_ID bits */
> >> +#define FW_CFG_VERSION 0x01
> >> +#define FW_CFG_VERSION_DMA 0x02
> >> +
> >> +/* fw_cfg file directory entry type */
> >> +struct fw_cfg_file {
> >> + __be32 size; /* file size */
> >> + __be16 select; /* write this to 0x510 to read it */
> >> + __u16 reserved;
> >> + char name[FW_CFG_MAX_FILE_PATH];
> >> +};
> >> +
> >> +struct fw_cfg_files {
> >> + __be32 count; /* number of entries */
> >> + struct fw_cfg_file f[];
> >> +};
> >
> > This struct wasn't there in the original file.
> > Is the source of these structures QEMU or Linux?
> > If Linux, then pls split this
> > 1. move code out to a header
> > 2. add more structs
>
> It is based from Linux source + qemu docs/specs/fw_cfg.txt which
> references include/hw/nvram/fw_cfg_keys.h "for the most up-to-date and
> authoritative list" & vmcoreinfo.txt.
>
> QEMU ./include/hw/nvram/fw_cfg_keys.h doesn't have specific licence
> text, and QEMU as a whole is released under the GNU General Public
> License, version 2.

Yes but it was split off from ./include/hw/nvram/fw_cfg.h
so it's a mistake that license was not copied.

But I think that for headers BSD is preferrable,
and this is exactly the license in QEMU.

> Let me know if I still need to change the license and what you
> recommend instead.


vmcoreinfo as I said should be a separate patch.

> >
> >> +
> >> +/* FW_CFG_DMA_CONTROL bits */
> >> +#define FW_CFG_DMA_CTL_ERROR 0x01
> >> +#define FW_CFG_DMA_CTL_READ 0x02
> >> +#define FW_CFG_DMA_CTL_SKIP 0x04
> >> +#define FW_CFG_DMA_CTL_SELECT 0x08
> >> +#define FW_CFG_DMA_CTL_WRITE 0x10
> >> +
> >> +#define FW_CFG_DMA_SIGNATURE 0x51454d5520434647ULL /* "QEMU CFG" */
> >> +
> >> +/* Control as first field allows for different structures selected by this
> >> + * field, which might be useful in the future
> >> + */
> >> +struct fw_cfg_dma_access {
> >> + __be32 control;
> >> + __be32 length;
> >> + __be64 address;
> >> +};
> >
> > Maybe this should be part of the dma patch.
>
> I prefer to introduce all the spec defines in one patch, but I can
> split it if it helps.


Probably - this way it's a harmless refactor.

> >
> >> +
> >> +#define FW_CFG_VMCOREINFO_FILENAME "etc/vmcoreinfo"
> >> +
> >> +#define FW_CFG_VMCOREINFO_FORMAT_NONE 0x0
> >> +#define FW_CFG_VMCOREINFO_FORMAT_ELF 0x1
> >> +
> >> +struct fw_cfg_vmcoreinfo {
> >> + __le16 host_format;
> >> + __le16 guest_format;
> >> + __le32 size;
> >> + __le64 paddr;
> >> +};
> >
> > Maybe this should be part of the vmcore patch.
>
> same
>
>
> Thanks

2018-02-15 18:26:02

by Marc-André Lureau

Subject: Re: [PATCH v14 8/9] fw_cfg: write vmcoreinfo details

Hi

On Thu, Feb 15, 2018 at 7:09 PM, Michael S. Tsirkin <[email protected]> wrote:
> On Wed, Feb 14, 2018 at 03:18:49PM +0100, Marc-André Lureau wrote:
>> If the "etc/vmcoreinfo" fw_cfg file is present and we are not running
>> the kdump kernel, write the addr/size of the vmcoreinfo ELF note.
>>
>> The DMA operation is expected to run synchronously with today qemu,
>> but the specification states that it may become async, so we run
>> "control" field check in a loop for eventual changes.
>>
>> Signed-off-by: Marc-André Lureau <[email protected]>
>> ---
>> drivers/firmware/qemu_fw_cfg.c | 144 ++++++++++++++++++++++++++++++++++++++++-
>> 1 file changed, 141 insertions(+), 3 deletions(-)
>>
>> diff --git a/drivers/firmware/qemu_fw_cfg.c b/drivers/firmware/qemu_fw_cfg.c
>> index 37638b95cb45..69939e2529f2 100644
>> --- a/drivers/firmware/qemu_fw_cfg.c
>> +++ b/drivers/firmware/qemu_fw_cfg.c
>> @@ -34,11 +34,17 @@
>> #include <linux/io.h>
>> #include <linux/ioport.h>
>> #include <linux/fw_cfg.h>
>> +#include <linux/delay.h>
>> +#include <linux/crash_dump.h>
>> +#include <linux/crash_core.h>
>>
>> MODULE_AUTHOR("Gabriel L. Somlo <[email protected]>");
>> MODULE_DESCRIPTION("QEMU fw_cfg sysfs support");
>> MODULE_LICENSE("GPL");
>>
>> +/* fw_cfg revision attribute, in /sys/firmware/qemu_fw_cfg top-level dir. */
>> +static u32 fw_cfg_rev;
>> +
>> /* fw_cfg device i/o register addresses */
>> static bool fw_cfg_is_mmio;
>> static phys_addr_t fw_cfg_p_base;
>> @@ -59,6 +65,65 @@ static inline u16 fw_cfg_sel_endianness(u16 key)
>> (u16 __force)cpu_to_le16(key);
>> }
>>
>> +static inline bool fw_cfg_dma_enabled(void)
>> +{
>> + return (fw_cfg_rev & FW_CFG_VERSION_DMA) && fw_cfg_reg_dma;
>> +}
>> +
>> +/* qemu fw_cfg device is sync today, but spec says it may become async */
>> +static void fw_cfg_wait_for_control(struct fw_cfg_dma_access *d)
>> +{
>> + do {
>> + u32 ctrl = be32_to_cpu(READ_ONCE(d->control));
>> +
>> + if ((ctrl & ~FW_CFG_DMA_CTL_ERROR) == 0)
>> + return;
>> +
>> + usleep_range(50, 100);
>
> I would just do cpu_relax() here.

ok, I didn't know that one.

>
>> + } while (true);
>> +
>> + /* do not reorder the read to d->control */
>> + rmb();
>
> Hmm. I don't really understand the comment.
> Is this code ever reacheable? How does it help?

I thought that's what you suggested in v13 review, but true, I should
replace the return with a break to reach it. Is that what you expect
too? (my understanding is to make sure the READ_ONCE(control) in
wait_for_control happens before READ_ONCE(control) after in
dma_transfer)

>
>> +}
>> +
>> +static ssize_t fw_cfg_dma_transfer(void *address, u32 length, u32 control)
>> +{
>> + phys_addr_t dma;
>> + struct fw_cfg_dma_access *d = NULL;
>> + ssize_t ret = length;
>> +
>> + d = kmalloc(sizeof(*d), GFP_KERNEL);
>> + if (!d) {
>> + ret = -ENOMEM;
>> + goto end;
>> + }
>> +
>> + /* fw_cfg device does not need IOMMU protection, so use physical addresses */
>> + *d = (struct fw_cfg_dma_access) {
>> + .address = cpu_to_be64(address ? virt_to_phys(address) : 0),
>> + .length = cpu_to_be32(length),
>> + .control = cpu_to_be32(control)
>> + };
>> +
>> + dma = virt_to_phys(d);
>> +
>> + iowrite32be((u64)dma >> 32, fw_cfg_reg_dma);
>> + /* force memory to sync before notifying device via MMIO */
>> + wmb();
>> + iowrite32be(dma, fw_cfg_reg_dma + 4);
>> +
>> + fw_cfg_wait_for_control(d);
>> +
>> + if (be32_to_cpu(READ_ONCE(d->control)) & FW_CFG_DMA_CTL_ERROR) {
>> + ret = -EIO;
>> + }
>> +
>> +end:
>> + kfree(d);
>> +
>> + return ret;
>> +}
>> +
>> /* read chunk of given fw_cfg blob (caller responsible for sanity-check) */
>> static inline void fw_cfg_read_blob(u16 key,
>> void *buf, loff_t pos, size_t count)
>> @@ -87,6 +152,47 @@ static inline void fw_cfg_read_blob(u16 key,
>> acpi_release_global_lock(glk);
>> }
>>
>> +#ifdef CONFIG_CRASH_CORE
>> +/* write chunk of given fw_cfg blob (caller responsible for sanity-check) */
>> +static ssize_t fw_cfg_write_blob(u16 key,
>> + void *buf, loff_t pos, size_t count)
>> +{
>> + u32 glk = -1U;
>> + acpi_status status;
>> + ssize_t ret = count;
>> +
>> + /* If we have ACPI, ensure mutual exclusion against any potential
>> + * device access by the firmware, e.g. via AML methods:
>> + */
>> + status = acpi_acquire_global_lock(ACPI_WAIT_FOREVER, &glk);
>> + if (ACPI_FAILURE(status) && status != AE_NOT_CONFIGURED) {
>> + /* Should never get here */
>> + WARN(1, "%s: Failed to lock ACPI!\n", __func__);
>> + return -EINVAL;
>> + }
>> +
>> + mutex_lock(&fw_cfg_dev_lock);
>> + if (pos == 0) {
>> + ret = fw_cfg_dma_transfer(buf, count, key << 16
>> + | FW_CFG_DMA_CTL_SELECT
>> + | FW_CFG_DMA_CTL_WRITE);
>> + } else {
>> + iowrite16(fw_cfg_sel_endianness(key), fw_cfg_reg_ctrl);
>> + ret = fw_cfg_dma_transfer(NULL, pos, FW_CFG_DMA_CTL_SKIP);
>> + if (ret < 0)
>> + goto end;
>> + ret = fw_cfg_dma_transfer(buf, count, FW_CFG_DMA_CTL_WRITE);
>> + }
>> +
>> +end:
>> + mutex_unlock(&fw_cfg_dev_lock);
>> +
>> + acpi_release_global_lock(glk);
>> +
>> + return ret;
>> +}
>> +#endif /* CONFIG_CRASH_CORE */
>> +
>> /* clean up fw_cfg device i/o */
>> static void fw_cfg_io_cleanup(void)
>> {
>> @@ -185,9 +291,6 @@ static int fw_cfg_do_platform_probe(struct platform_device *pdev)
>> return 0;
>> }
>>
>> -/* fw_cfg revision attribute, in /sys/firmware/qemu_fw_cfg top-level dir. */
>> -static u32 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);
>> @@ -210,6 +313,32 @@ struct fw_cfg_sysfs_entry {
>> struct list_head list;
>> };
>>
>> +#ifdef CONFIG_CRASH_CORE
>> +static ssize_t fw_cfg_write_vmcoreinfo(const struct fw_cfg_file *f)
>> +{
>> + static struct fw_cfg_vmcoreinfo *data;
>> + ssize_t ret;
>> +
>> + data = kmalloc(sizeof(struct fw_cfg_vmcoreinfo), GFP_KERNEL);
>> + if (!data)
>> + return -ENOMEM;
>> +
>> + *data = (struct fw_cfg_vmcoreinfo) {
>> + .guest_format = cpu_to_le16(FW_CFG_VMCOREINFO_FORMAT_ELF),
>> + .size = cpu_to_le32(VMCOREINFO_NOTE_SIZE),
>> + .paddr = cpu_to_le64(paddr_vmcoreinfo_note())
>> + };
>> + /* spare ourself reading host format support for now since we
>> + * don't know what else to format - host may ignore ours
>> + */
>> + ret = fw_cfg_write_blob(be16_to_cpu(f->select), data,
>> + 0, sizeof(struct fw_cfg_vmcoreinfo));
>> +
>> + kfree(data);
>> + return ret;
>> +}
>> +#endif /* CONFIG_CRASH_CORE */
>> +
>> /* get fw_cfg_sysfs_entry from kobject member */
>> static inline struct fw_cfg_sysfs_entry *to_entry(struct kobject *kobj)
>> {
>> @@ -450,6 +579,15 @@ static int fw_cfg_register_file(const struct fw_cfg_file *f)
>> int err;
>> struct fw_cfg_sysfs_entry *entry;
>>
>> +#ifdef CONFIG_CRASH_CORE
>> + if (fw_cfg_dma_enabled() &&
>> + strcmp(f->name, FW_CFG_VMCOREINFO_FILENAME) == 0 &&
>> + !is_kdump_kernel()) {
>> + if (fw_cfg_write_vmcoreinfo(f) < 0)
>> + pr_warn("fw_cfg: failed to write vmcoreinfo");
>> + }
>> +#endif
>> +
>> /* allocate new entry */
>> entry = kzalloc(sizeof(*entry), GFP_KERNEL);
>> if (!entry)
>> --
>> 2.16.1.73.g5832b7e9f2

2018-02-15 18:35:25

by Marc-André Lureau

Subject: Re: [PATCH v14 2/9] fw_cfg: add a public uapi header

Hi

On Thu, Feb 15, 2018 at 7:20 PM, Michael S. Tsirkin <[email protected]> wrote:
> On Thu, Feb 15, 2018 at 10:25:27AM +0100, Marc-Andre Lureau wrote:
>> Hi
>>
>> On Wed, Feb 14, 2018 at 9:41 PM, Michael S. Tsirkin <[email protected]> wrote:
>> > On Wed, Feb 14, 2018 at 03:18:43PM +0100, Marc-André Lureau wrote:
>> >> Create a common header file for well-known values and structures to be
>> >> shared by the Linux kernel with qemu or other projects.
>> >>
>> >> Suggested-by: Michael S. Tsirkin <[email protected]>
>> >> Signed-off-by: Marc-André Lureau <[email protected]>
>> >>
>> >> ---
>> >>
>> >> The related qemu patch making use of it, to be submitted:
>> >> https://github.com/elmarco/qemu/commit/4884fc9e9c4c4467a371e5a40f3181239e1b70f5
>> >> ---
>> >> MAINTAINERS | 1 +
>> >> drivers/firmware/qemu_fw_cfg.c | 22 +--------
>> >> include/uapi/linux/fw_cfg.h | 102 +++++++++++++++++++++++++++++++++++++++++
>> >> 3 files changed, 105 insertions(+), 20 deletions(-)
>> >> create mode 100644 include/uapi/linux/fw_cfg.h
>> >>
>> >> diff --git a/MAINTAINERS b/MAINTAINERS
>> >> index 3bdc260e36b7..a66b65f62811 100644
>> >> --- a/MAINTAINERS
>> >> +++ b/MAINTAINERS
>> >> @@ -11352,6 +11352,7 @@ M: "Michael S. Tsirkin" <[email protected]>
>> >> L: [email protected]
>> >> S: Maintained
>> >> F: drivers/firmware/qemu_fw_cfg.c
>> >> +F: include/uapi/linux/fw_cfg.h
>> >>
>> >> QIB DRIVER
>> >> M: Dennis Dalessandro <[email protected]>
>> >> diff --git a/drivers/firmware/qemu_fw_cfg.c b/drivers/firmware/qemu_fw_cfg.c
>> >> index a41b572eeeb1..90f467232777 100644
>> >> --- a/drivers/firmware/qemu_fw_cfg.c
>> >> +++ b/drivers/firmware/qemu_fw_cfg.c
>> >> @@ -32,30 +32,12 @@
>> >> #include <linux/slab.h>
>> >> #include <linux/io.h>
>> >> #include <linux/ioport.h>
>> >> +#include <linux/fw_cfg.h>
>> >>
>> >> MODULE_AUTHOR("Gabriel L. Somlo <[email protected]>");
>> >> MODULE_DESCRIPTION("QEMU fw_cfg sysfs support");
>> >> MODULE_LICENSE("GPL");
>> >>
>> >> -/* selector key 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 {
>> >> - u32 size;
>> >> - u16 select;
>> >> - u16 reserved;
>> >> - char name[FW_CFG_MAX_FILE_PATH];
>> >> -};
>> >> -
>> >> /* fw_cfg device i/o register addresses */
>> >> static bool fw_cfg_is_mmio;
>> >> static phys_addr_t fw_cfg_p_base;
>> >> @@ -597,7 +579,7 @@ MODULE_DEVICE_TABLE(of, fw_cfg_sysfs_mmio_match);
>> >>
>> >> #ifdef CONFIG_ACPI
>> >> static const struct acpi_device_id fw_cfg_sysfs_acpi_match[] = {
>> >> - { "QEMU0002", },
>> >> + { FW_CFG_ACPI_DEVICE_ID, },
>> >> {},
>> >> };
>> >> MODULE_DEVICE_TABLE(acpi, fw_cfg_sysfs_acpi_match);
>> >> diff --git a/include/uapi/linux/fw_cfg.h b/include/uapi/linux/fw_cfg.h
>> >> new file mode 100644
>> >> index 000000000000..5b8136ce46ee
>> >> --- /dev/null
>> >> +++ b/include/uapi/linux/fw_cfg.h
>> >> @@ -0,0 +1,102 @@
>> >> +/* SPDX-License-Identifier: GPL-2.0 WITH Linux-syscall-note */
>> >> +#ifndef _LINUX_FW_CFG_H
>> >> +#define _LINUX_FW_CFG_H
>> >> +
>> >> +#include <linux/types.h>
>> >> +
>> >> +#define FW_CFG_ACPI_DEVICE_ID "QEMU0002"
>> >> +
>> >> +/* selector key values for "well-known" fw_cfg entries */
>> >> +#define FW_CFG_SIGNATURE 0x00
>> >> +#define FW_CFG_ID 0x01
>> >> +#define FW_CFG_UUID 0x02
>> >> +#define FW_CFG_RAM_SIZE 0x03
>> >> +#define FW_CFG_NOGRAPHIC 0x04
>> >> +#define FW_CFG_NB_CPUS 0x05
>> >> +#define FW_CFG_MACHINE_ID 0x06
>> >> +#define FW_CFG_KERNEL_ADDR 0x07
>> >> +#define FW_CFG_KERNEL_SIZE 0x08
>> >> +#define FW_CFG_KERNEL_CMDLINE 0x09
>> >> +#define FW_CFG_INITRD_ADDR 0x0a
>> >> +#define FW_CFG_INITRD_SIZE 0x0b
>> >> +#define FW_CFG_BOOT_DEVICE 0x0c
>> >> +#define FW_CFG_NUMA 0x0d
>> >> +#define FW_CFG_BOOT_MENU 0x0e
>> >> +#define FW_CFG_MAX_CPUS 0x0f
>> >> +#define FW_CFG_KERNEL_ENTRY 0x10
>> >> +#define FW_CFG_KERNEL_DATA 0x11
>> >> +#define FW_CFG_INITRD_DATA 0x12
>> >> +#define FW_CFG_CMDLINE_ADDR 0x13
>> >> +#define FW_CFG_CMDLINE_SIZE 0x14
>> >> +#define FW_CFG_CMDLINE_DATA 0x15
>> >> +#define FW_CFG_SETUP_ADDR 0x16
>> >> +#define FW_CFG_SETUP_SIZE 0x17
>> >> +#define FW_CFG_SETUP_DATA 0x18
>> >> +#define FW_CFG_FILE_DIR 0x19
>> >> +
>> >> +#define FW_CFG_FILE_FIRST 0x20
>> >> +#define FW_CFG_FILE_SLOTS_MIN 0x10
>> >> +
>> >> +#define FW_CFG_WRITE_CHANNEL 0x4000
>> >> +#define FW_CFG_ARCH_LOCAL 0x8000
>> >> +#define FW_CFG_ENTRY_MASK (~(FW_CFG_WRITE_CHANNEL | FW_CFG_ARCH_LOCAL))
>> >> +
>> >> +#define FW_CFG_INVALID 0xffff
>> >> +
>> >> +/* width in bytes of fw_cfg control register */
>> >> +#define FW_CFG_CTL_SIZE 0x02
>> >> +
>> >> +/* fw_cfg "file name" is up to 56 characters (including terminating nul) */
>> >> +#define FW_CFG_MAX_FILE_PATH 56
>> >> +
>> >> +/* size in bytes of fw_cfg signature */
>> >> +#define FW_CFG_SIG_SIZE 4
>> >> +
>> >> +/* FW_CFG_ID bits */
>> >> +#define FW_CFG_VERSION 0x01
>> >> +#define FW_CFG_VERSION_DMA 0x02
>> >> +
>> >> +/* fw_cfg file directory entry type */
>> >> +struct fw_cfg_file {
>> >> + __be32 size; /* file size */
>> >> + __be16 select; /* write this to 0x510 to read it */
>> >> + __u16 reserved;
>> >> + char name[FW_CFG_MAX_FILE_PATH];
>> >> +};
>> >> +
>> >> +struct fw_cfg_files {
>> >> + __be32 count; /* number of entries */
>> >> + struct fw_cfg_file f[];
>> >> +};
>> >
>> > This struct wasn't there in the original file.
>> > Is the source of these structures QEMU or Linux?
>> > If Linux, then pls split this
>> > 1. move code out to a header
>> > 2. add more structs
>>
>> It is based from Linux source + qemu docs/specs/fw_cfg.txt which
>> references include/hw/nvram/fw_cfg_keys.h "for the most up-to-date and
>> authoritative list" & vmcoreinfo.txt.
>>
>> QEMU ./include/hw/nvram/fw_cfg_keys.h doesn't have specific licence
>> text, and QEMU as a whole is released under the GNU General Public
>> License, version 2.
>
> Yes but it was split off from ./include/hw/nvram/fw_cfg.h
> so it's a mistake that license was not copied.
>
> But I think that for headers BSD is preferrable,
> and this is exactly the license in QEMU.

So would top comment be fine?
/* SPDX-License-Identifier: BSD-3-Clause */


>
>> Let me know if I still need to change the license and what you
>> recommend instead.
>
>
> vmcoreinfo as I said should be a separate patch.
>
>> >
>> >> +
>> >> +/* FW_CFG_DMA_CONTROL bits */
>> >> +#define FW_CFG_DMA_CTL_ERROR 0x01
>> >> +#define FW_CFG_DMA_CTL_READ 0x02
>> >> +#define FW_CFG_DMA_CTL_SKIP 0x04
>> >> +#define FW_CFG_DMA_CTL_SELECT 0x08
>> >> +#define FW_CFG_DMA_CTL_WRITE 0x10
>> >> +
>> >> +#define FW_CFG_DMA_SIGNATURE 0x51454d5520434647ULL /* "QEMU CFG" */
>> >> +
>> >> +/* Control as first field allows for different structures selected by this
>> >> + * field, which might be useful in the future
>> >> + */
>> >> +struct fw_cfg_dma_access {
>> >> + __be32 control;
>> >> + __be32 length;
>> >> + __be64 address;
>> >> +};
>> >
>> > Maybe this should be part of the dma patch.
>>
>> I prefer to introduce all the spec defines in one patch, but I can
>> split it if it helps.
>
>
> Probably - this way it's a harmless refactor.

ok

>
>> >
>> >> +
>> >> +#define FW_CFG_VMCOREINFO_FILENAME "etc/vmcoreinfo"
>> >> +
>> >> +#define FW_CFG_VMCOREINFO_FORMAT_NONE 0x0
>> >> +#define FW_CFG_VMCOREINFO_FORMAT_ELF 0x1
>> >> +
>> >> +struct fw_cfg_vmcoreinfo {
>> >> + __le16 host_format;
>> >> + __le16 guest_format;
>> >> + __le32 size;
>> >> + __le64 paddr;
>> >> +};
>> >
>> > Maybe this should be part of the vmcore patch.
>>
>> same
>>
>>
>> Thanks

2018-02-15 18:38:20

by Michael S. Tsirkin

Subject: Re: [PATCH v14 8/9] fw_cfg: write vmcoreinfo details

On Thu, Feb 15, 2018 at 07:24:34PM +0100, Marc-Andr? Lureau wrote:
> Hi
>
> On Thu, Feb 15, 2018 at 7:09 PM, Michael S. Tsirkin <[email protected]> wrote:
> > On Wed, Feb 14, 2018 at 03:18:49PM +0100, Marc-Andr? Lureau wrote:
> >> If the "etc/vmcoreinfo" fw_cfg file is present and we are not running
> >> the kdump kernel, write the addr/size of the vmcoreinfo ELF note.
> >>
> >> The DMA operation is expected to run synchronously with today qemu,
> >> but the specification states that it may become async, so we run
> >> "control" field check in a loop for eventual changes.
> >>
> >> Signed-off-by: Marc-Andr? Lureau <[email protected]>
> >> ---
> >> drivers/firmware/qemu_fw_cfg.c | 144 ++++++++++++++++++++++++++++++++++++++++-
> >> 1 file changed, 141 insertions(+), 3 deletions(-)
> >>
> >> diff --git a/drivers/firmware/qemu_fw_cfg.c b/drivers/firmware/qemu_fw_cfg.c
> >> index 37638b95cb45..69939e2529f2 100644
> >> --- a/drivers/firmware/qemu_fw_cfg.c
> >> +++ b/drivers/firmware/qemu_fw_cfg.c
> >> @@ -34,11 +34,17 @@
> >> #include <linux/io.h>
> >> #include <linux/ioport.h>
> >> #include <linux/fw_cfg.h>
> >> +#include <linux/delay.h>
> >> +#include <linux/crash_dump.h>
> >> +#include <linux/crash_core.h>
> >>
> >> MODULE_AUTHOR("Gabriel L. Somlo <[email protected]>");
> >> MODULE_DESCRIPTION("QEMU fw_cfg sysfs support");
> >> MODULE_LICENSE("GPL");
> >>
> >> +/* fw_cfg revision attribute, in /sys/firmware/qemu_fw_cfg top-level dir. */
> >> +static u32 fw_cfg_rev;
> >> +
> >> /* fw_cfg device i/o register addresses */
> >> static bool fw_cfg_is_mmio;
> >> static phys_addr_t fw_cfg_p_base;
> >> @@ -59,6 +65,65 @@ static inline u16 fw_cfg_sel_endianness(u16 key)
> >> (u16 __force)cpu_to_le16(key);
> >> }
> >>
> >> +static inline bool fw_cfg_dma_enabled(void)
> >> +{
> >> + return (fw_cfg_rev & FW_CFG_VERSION_DMA) && fw_cfg_reg_dma;
> >> +}
> >> +
> >> +/* qemu fw_cfg device is sync today, but spec says it may become async */
> >> +static void fw_cfg_wait_for_control(struct fw_cfg_dma_access *d)
> >> +{
> >> + do {
> >> + u32 ctrl = be32_to_cpu(READ_ONCE(d->control));
> >> +
> >> + if ((ctrl & ~FW_CFG_DMA_CTL_ERROR) == 0)
> >> + return;
> >> +
> >> + usleep_range(50, 100);
> >
> > I would just do cpu_relax() here.
>
> ok, I didn't know that one.
>
> >
> >> + } while (true);

Pls write for (;;) and not do - while (true).

> >> +
> >> + /* do not reorder the read to d->control */
> >> + rmb();
> >
> > Hmm. I don't really understand the comment.
> > Is this code ever reacheable? How does it help?
>
> I thought that's what you suggested in v13 review, but true, I should
> replace the return with a break to reach it. Is that what you expect
> too?

I'd do it unconditionally after reading ctrl.

> (my understanding is to make sure the READ_ONCE(control) in
> wait_for_control happens before READ_ONCE(control) after in
> dma_transfer)

I expect you to understand memory-barriers.txt :)

> >
> >> +}
> >> +
> >> +static ssize_t fw_cfg_dma_transfer(void *address, u32 length, u32 control)
> >> +{
> >> + phys_addr_t dma;
> >> + struct fw_cfg_dma_access *d = NULL;
> >> + ssize_t ret = length;
> >> +
> >> + d = kmalloc(sizeof(*d), GFP_KERNEL);
> >> + if (!d) {
> >> + ret = -ENOMEM;
> >> + goto end;
> >> + }
> >> +
> >> + /* fw_cfg device does not need IOMMU protection, so use physical addresses */
> >> + *d = (struct fw_cfg_dma_access) {
> >> + .address = cpu_to_be64(address ? virt_to_phys(address) : 0),
> >> + .length = cpu_to_be32(length),
> >> + .control = cpu_to_be32(control)
> >> + };
> >> +
> >> + dma = virt_to_phys(d);
> >> +
> >> + iowrite32be((u64)dma >> 32, fw_cfg_reg_dma);
> >> + /* force memory to sync before notifying device via MMIO */
> >> + wmb();
> >> + iowrite32be(dma, fw_cfg_reg_dma + 4);
> >> +
> >> + fw_cfg_wait_for_control(d);
> >> +
> >> + if (be32_to_cpu(READ_ONCE(d->control)) & FW_CFG_DMA_CTL_ERROR) {
> >> + ret = -EIO;
> >> + }
> >> +
> >> +end:
> >> + kfree(d);
> >> +
> >> + return ret;
> >> +}
> >> +
> >> /* read chunk of given fw_cfg blob (caller responsible for sanity-check) */
> >> static inline void fw_cfg_read_blob(u16 key,
> >> void *buf, loff_t pos, size_t count)
> >> @@ -87,6 +152,47 @@ static inline void fw_cfg_read_blob(u16 key,
> >> acpi_release_global_lock(glk);
> >> }
> >>
> >> +#ifdef CONFIG_CRASH_CORE
> >> +/* write chunk of given fw_cfg blob (caller responsible for sanity-check) */
> >> +static ssize_t fw_cfg_write_blob(u16 key,
> >> + void *buf, loff_t pos, size_t count)
> >> +{
> >> + u32 glk = -1U;
> >> + acpi_status status;
> >> + ssize_t ret = count;
> >> +
> >> + /* If we have ACPI, ensure mutual exclusion against any potential
> >> + * device access by the firmware, e.g. via AML methods:
> >> + */
> >> + status = acpi_acquire_global_lock(ACPI_WAIT_FOREVER, &glk);
> >> + if (ACPI_FAILURE(status) && status != AE_NOT_CONFIGURED) {
> >> + /* Should never get here */
> >> + WARN(1, "%s: Failed to lock ACPI!\n", __func__);
> >> + return -EINVAL;
> >> + }
> >> +
> >> + mutex_lock(&fw_cfg_dev_lock);
> >> + if (pos == 0) {
> >> + ret = fw_cfg_dma_transfer(buf, count, key << 16
> >> + | FW_CFG_DMA_CTL_SELECT
> >> + | FW_CFG_DMA_CTL_WRITE);
> >> + } else {
> >> + iowrite16(fw_cfg_sel_endianness(key), fw_cfg_reg_ctrl);
> >> + ret = fw_cfg_dma_transfer(NULL, pos, FW_CFG_DMA_CTL_SKIP);
> >> + if (ret < 0)
> >> + goto end;
> >> + ret = fw_cfg_dma_transfer(buf, count, FW_CFG_DMA_CTL_WRITE);
> >> + }
> >> +
> >> +end:
> >> + mutex_unlock(&fw_cfg_dev_lock);
> >> +
> >> + acpi_release_global_lock(glk);
> >> +
> >> + return ret;
> >> +}
> >> +#endif /* CONFIG_CRASH_CORE */
> >> +
> >> /* clean up fw_cfg device i/o */
> >> static void fw_cfg_io_cleanup(void)
> >> {
> >> @@ -185,9 +291,6 @@ static int fw_cfg_do_platform_probe(struct platform_device *pdev)
> >> return 0;
> >> }
> >>
> >> -/* fw_cfg revision attribute, in /sys/firmware/qemu_fw_cfg top-level dir. */
> >> -static u32 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);
> >> @@ -210,6 +313,32 @@ struct fw_cfg_sysfs_entry {
> >> struct list_head list;
> >> };
> >>
> >> +#ifdef CONFIG_CRASH_CORE
> >> +static ssize_t fw_cfg_write_vmcoreinfo(const struct fw_cfg_file *f)
> >> +{
> >> + static struct fw_cfg_vmcoreinfo *data;
> >> + ssize_t ret;
> >> +
> >> + data = kmalloc(sizeof(struct fw_cfg_vmcoreinfo), GFP_KERNEL);
> >> + if (!data)
> >> + return -ENOMEM;
> >> +
> >> + *data = (struct fw_cfg_vmcoreinfo) {
> >> + .guest_format = cpu_to_le16(FW_CFG_VMCOREINFO_FORMAT_ELF),
> >> + .size = cpu_to_le32(VMCOREINFO_NOTE_SIZE),
> >> + .paddr = cpu_to_le64(paddr_vmcoreinfo_note())
> >> + };
> >> + /* spare ourself reading host format support for now since we
> >> + * don't know what else to format - host may ignore ours
> >> + */
> >> + ret = fw_cfg_write_blob(be16_to_cpu(f->select), data,
> >> + 0, sizeof(struct fw_cfg_vmcoreinfo));
> >> +
> >> + kfree(data);
> >> + return ret;
> >> +}
> >> +#endif /* CONFIG_CRASH_CORE */
> >> +
> >> /* get fw_cfg_sysfs_entry from kobject member */
> >> static inline struct fw_cfg_sysfs_entry *to_entry(struct kobject *kobj)
> >> {
> >> @@ -450,6 +579,15 @@ static int fw_cfg_register_file(const struct fw_cfg_file *f)
> >> int err;
> >> struct fw_cfg_sysfs_entry *entry;
> >>
> >> +#ifdef CONFIG_CRASH_CORE
> >> + if (fw_cfg_dma_enabled() &&
> >> + strcmp(f->name, FW_CFG_VMCOREINFO_FILENAME) == 0 &&
> >> + !is_kdump_kernel()) {
> >> + if (fw_cfg_write_vmcoreinfo(f) < 0)
> >> + pr_warn("fw_cfg: failed to write vmcoreinfo");
> >> + }
> >> +#endif
> >> +
> >> /* allocate new entry */
> >> entry = kzalloc(sizeof(*entry), GFP_KERNEL);
> >> if (!entry)
> >> --
> >> 2.16.1.73.g5832b7e9f2

2018-02-16 13:08:19

by Michael S. Tsirkin

Subject: Re: [PATCH v14 2/9] fw_cfg: add a public uapi header

On Thu, Feb 15, 2018 at 10:29:33AM +0100, Marc-Andre Lureau wrote:
> Hi
>
> On Wed, Feb 14, 2018 at 8:37 PM, Michael S. Tsirkin <[email protected]> wrote:
> > On Wed, Feb 14, 2018 at 03:18:43PM +0100, Marc-Andr? Lureau wrote:
> >> Create a common header file for well-known values and structures to be
> >> shared by the Linux kernel with qemu or other projects.
> >>
> >> Suggested-by: Michael S. Tsirkin <[email protected]>
> >> Signed-off-by: Marc-Andr? Lureau <[email protected]>
> >>
> >> ---
> >>
> >> The related qemu patch making use of it, to be submitted:
> >> https://github.com/elmarco/qemu/commit/4884fc9e9c4c4467a371e5a40f3181239e1b70f5
> >> ---
> >> MAINTAINERS | 1 +
> >> drivers/firmware/qemu_fw_cfg.c | 22 +--------
> >> include/uapi/linux/fw_cfg.h | 102 +++++++++++++++++++++++++++++++++++++++++
> >> 3 files changed, 105 insertions(+), 20 deletions(-)
> >> create mode 100644 include/uapi/linux/fw_cfg.h
> >>
> >> diff --git a/MAINTAINERS b/MAINTAINERS
> >> index 3bdc260e36b7..a66b65f62811 100644
> >> --- a/MAINTAINERS
> >> +++ b/MAINTAINERS
> >> @@ -11352,6 +11352,7 @@ M: "Michael S. Tsirkin" <[email protected]>
> >> L: [email protected]
> >> S: Maintained
> >> F: drivers/firmware/qemu_fw_cfg.c
> >> +F: include/uapi/linux/fw_cfg.h
> >>
> >> QIB DRIVER
> >> M: Dennis Dalessandro <[email protected]>
> >> diff --git a/drivers/firmware/qemu_fw_cfg.c b/drivers/firmware/qemu_fw_cfg.c
> >> index a41b572eeeb1..90f467232777 100644
> >> --- a/drivers/firmware/qemu_fw_cfg.c
> >> +++ b/drivers/firmware/qemu_fw_cfg.c
> >> @@ -32,30 +32,12 @@
> >> #include <linux/slab.h>
> >> #include <linux/io.h>
> >> #include <linux/ioport.h>
> >> +#include <linux/fw_cfg.h>
> >
> > You include the header from include/linux/fw_cfg.h ...
> >
> >>
> >> MODULE_AUTHOR("Gabriel L. Somlo <[email protected]>");
> >> MODULE_DESCRIPTION("QEMU fw_cfg sysfs support");
> >> MODULE_LICENSE("GPL");
> >>
> >> -/* selector key 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 {
> >> - u32 size;
> >> - u16 select;
> >> - u16 reserved;
> >> - char name[FW_CFG_MAX_FILE_PATH];
> >> -};
> >> -
> >> /* fw_cfg device i/o register addresses */
> >> static bool fw_cfg_is_mmio;
> >> static phys_addr_t fw_cfg_p_base;
> >> @@ -597,7 +579,7 @@ MODULE_DEVICE_TABLE(of, fw_cfg_sysfs_mmio_match);
> >>
> >> #ifdef CONFIG_ACPI
> >> static const struct acpi_device_id fw_cfg_sysfs_acpi_match[] = {
> >> - { "QEMU0002", },
> >> + { FW_CFG_ACPI_DEVICE_ID, },
> >> {},
> >> };
> >> MODULE_DEVICE_TABLE(acpi, fw_cfg_sysfs_acpi_match);
> >> diff --git a/include/uapi/linux/fw_cfg.h b/include/uapi/linux/fw_cfg.h
> >> new file mode 100644
> >> index 000000000000..5b8136ce46ee
> >> --- /dev/null
> >> +++ b/include/uapi/linux/fw_cfg.h
> >
> > Yet the header is located in include/uapi/linux/fw_cfg.h
> >
> > How can this work?
> >
>
> $ make drivers/firmware/qemu_fw_cfg.ko V=1
>
> gcc -Wp,-MD,drivers/firmware/.qemu_fw_cfg.o.d -nostdinc -isystem
> /usr/lib/gcc/x86_64-redhat-linux/7/include -I./arch/x86/include
> -I./arch/x86/include/generated -I./include -I./arch/x86/include/uapi
> ...

Oh an out of tree build.
Try to build whole kernel and it will fail.


> >> @@ -0,0 +1,102 @@
> >> +/* SPDX-License-Identifier: GPL-2.0 WITH Linux-syscall-note */
> >
> > As far as I can see these have all been lifted from a BSD-licensed
> > hw/nvram/fw_cfg.c in qemu. So please make it BSD accordingly,
> > and include the explanation in the commit log.
> >
>
> (see other reply)
>
> >> +#ifndef _LINUX_FW_CFG_H
> >> +#define _LINUX_FW_CFG_H
> >> +
> >> +#include <linux/types.h>
> >> +
> >> +#define FW_CFG_ACPI_DEVICE_ID "QEMU0002"
> >> +
> >> +/* selector key values for "well-known" fw_cfg entries */
> >> +#define FW_CFG_SIGNATURE 0x00
> >> +#define FW_CFG_ID 0x01
> >> +#define FW_CFG_UUID 0x02
> >> +#define FW_CFG_RAM_SIZE 0x03
> >> +#define FW_CFG_NOGRAPHIC 0x04
> >> +#define FW_CFG_NB_CPUS 0x05
> >> +#define FW_CFG_MACHINE_ID 0x06
> >> +#define FW_CFG_KERNEL_ADDR 0x07
> >> +#define FW_CFG_KERNEL_SIZE 0x08
> >> +#define FW_CFG_KERNEL_CMDLINE 0x09
> >> +#define FW_CFG_INITRD_ADDR 0x0a
> >> +#define FW_CFG_INITRD_SIZE 0x0b
> >> +#define FW_CFG_BOOT_DEVICE 0x0c
> >> +#define FW_CFG_NUMA 0x0d
> >> +#define FW_CFG_BOOT_MENU 0x0e
> >> +#define FW_CFG_MAX_CPUS 0x0f
> >> +#define FW_CFG_KERNEL_ENTRY 0x10
> >> +#define FW_CFG_KERNEL_DATA 0x11
> >> +#define FW_CFG_INITRD_DATA 0x12
> >> +#define FW_CFG_CMDLINE_ADDR 0x13
> >> +#define FW_CFG_CMDLINE_SIZE 0x14
> >> +#define FW_CFG_CMDLINE_DATA 0x15
> >> +#define FW_CFG_SETUP_ADDR 0x16
> >> +#define FW_CFG_SETUP_SIZE 0x17
> >> +#define FW_CFG_SETUP_DATA 0x18
> >> +#define FW_CFG_FILE_DIR 0x19
> >> +
> >> +#define FW_CFG_FILE_FIRST 0x20
> >> +#define FW_CFG_FILE_SLOTS_MIN 0x10
> >> +
> >> +#define FW_CFG_WRITE_CHANNEL 0x4000
> >> +#define FW_CFG_ARCH_LOCAL 0x8000
> >> +#define FW_CFG_ENTRY_MASK (~(FW_CFG_WRITE_CHANNEL | FW_CFG_ARCH_LOCAL))
> >> +
> >> +#define FW_CFG_INVALID 0xffff
> >> +
> >> +/* width in bytes of fw_cfg control register */
> >> +#define FW_CFG_CTL_SIZE 0x02
> >> +
> >> +/* fw_cfg "file name" is up to 56 characters (including terminating nul) */
> >> +#define FW_CFG_MAX_FILE_PATH 56
> >> +
> >> +/* size in bytes of fw_cfg signature */
> >> +#define FW_CFG_SIG_SIZE 4
> >> +
> >> +/* FW_CFG_ID bits */
> >> +#define FW_CFG_VERSION 0x01
> >> +#define FW_CFG_VERSION_DMA 0x02
> >> +
> >> +/* fw_cfg file directory entry type */
> >> +struct fw_cfg_file {
> >> + __be32 size; /* file size */
> >> + __be16 select; /* write this to 0x510 to read it */
> >> + __u16 reserved;
> >> + char name[FW_CFG_MAX_FILE_PATH];
> >> +};
> >> +
> >> +struct fw_cfg_files {
> >> + __be32 count; /* number of entries */
> >> + struct fw_cfg_file f[];
> >> +};
> >> +
> >> +/* FW_CFG_DMA_CONTROL bits */
> >> +#define FW_CFG_DMA_CTL_ERROR 0x01
> >> +#define FW_CFG_DMA_CTL_READ 0x02
> >> +#define FW_CFG_DMA_CTL_SKIP 0x04
> >> +#define FW_CFG_DMA_CTL_SELECT 0x08
> >> +#define FW_CFG_DMA_CTL_WRITE 0x10
> >> +
> >> +#define FW_CFG_DMA_SIGNATURE 0x51454d5520434647ULL /* "QEMU CFG" */
> >> +
> >> +/* Control as first field allows for different structures selected by this
> >> + * field, which might be useful in the future
> >> + */
> >> +struct fw_cfg_dma_access {
> >> + __be32 control;
> >> + __be32 length;
> >> + __be64 address;
> >> +};
> >> +
> >> +#define FW_CFG_VMCOREINFO_FILENAME "etc/vmcoreinfo"
> >> +
> >> +#define FW_CFG_VMCOREINFO_FORMAT_NONE 0x0
> >> +#define FW_CFG_VMCOREINFO_FORMAT_ELF 0x1
> >> +
> >> +struct fw_cfg_vmcoreinfo {
> >> + __le16 host_format;
> >> + __le16 guest_format;
> >> + __le32 size;
> >> + __le64 paddr;
> >> +};
> >> +
> >> +#endif
> >> --
> >> 2.16.1.73.g5832b7e9f2

2018-02-16 13:11:43

by Michael S. Tsirkin

Subject: Re: [PATCH v14 3/9] fw_cfg: fix sparse warnings in fw_cfg_sel_endianness()

On Thu, Feb 15, 2018 at 10:34:28AM +0100, Marc-Andre Lureau wrote:
> On Wed, Feb 14, 2018 at 9:46 PM, Michael S. Tsirkin <[email protected]> wrote:
> > On Wed, Feb 14, 2018 at 03:18:44PM +0100, Marc-Andr? Lureau wrote:
> >> The function is used for both LE & BE target type, use __force casting.
> >>
> >> Fixes:
> >> $ make C=1 CF=-D__CHECK_ENDIAN__ drivers/firmware/qemu_fw_cfg.o
> >>
> >> drivers/firmware/qemu_fw_cfg.c:55:33: warning: restricted __be16 degrades to integer
> >> drivers/firmware/qemu_fw_cfg.c:55:52: warning: restricted __le16 degrades to integer
> >>
> >> Signed-off-by: Marc-Andr? Lureau <[email protected]>
> >> ---
> >> drivers/firmware/qemu_fw_cfg.c | 4 +++-
> >> 1 file changed, 3 insertions(+), 1 deletion(-)
> >>
> >> diff --git a/drivers/firmware/qemu_fw_cfg.c b/drivers/firmware/qemu_fw_cfg.c
> >> index 90f467232777..85e693287d87 100644
> >> --- a/drivers/firmware/qemu_fw_cfg.c
> >> +++ b/drivers/firmware/qemu_fw_cfg.c
> >> @@ -52,7 +52,9 @@ static DEFINE_MUTEX(fw_cfg_dev_lock);
> >> /* pick appropriate endianness for selector key */
> >> static inline u16 fw_cfg_sel_endianness(u16 key)
> >> {
> >> - return fw_cfg_is_mmio ? cpu_to_be16(key) : cpu_to_le16(key);
> >> + return fw_cfg_is_mmio ?
> >> + (u16 __force)cpu_to_be16(key) :
> >> + (u16 __force)cpu_to_le16(key);
> >> }
> >>
> >> /* read chunk of given fw_cfg blob (caller responsible for sanity-check) */
> >
> > Well the caller does cpu_to_le16 on the result ...
> > All this makes my head spin.
> >
> > IMHO what you want is a wrapper that does iowrite and iowritebe
> > rather than __force.
>
> iowrite16(key) is the same as iowrite16(cpu_to_le16(key)) ? There is
> no iowrite16le()...

Yes.

> Is this equivalent, and not introducing regressions?
>
> static inline u16 fw_cfg_sel_endianness(u16 key)
> +static void fw_cfg_sel_endianness(u16 key)
> {
> - return fw_cfg_is_mmio ? cpu_to_be16(key) : cpu_to_le16(key);

For mmio on LE it does 1 swap. On BE 1 swap.
For non mmio on LE it does no swaps. On BE 1 swap.

Fair summary?

And is this actually the intended behaviour.

> + if (fw_cfg_is_mmio)
> + iowrite16be(key, fw_cfg_reg_ctrl);
> + else
> + iowrite16(key, fw_cfg_reg_ctrl);


this behaves differently. donnu if that's a bug or
a feature. You will have to find out.

> }
>
> /* read chunk of given fw_cfg blob (caller responsible for sanity-check) */
> @@ -74,7 +77,7 @@ static inline void fw_cfg_read_blob(u16 key,
> }
>
> mutex_lock(&fw_cfg_dev_lock);
> - iowrite16(fw_cfg_sel_endianness(key), fw_cfg_reg_ctrl);
> + fw_cfg_sel_endianness(key);
>
> >
> >
> >> --
> >> 2.16.1.73.g5832b7e9f2