[PATCH v6 0/5] fw_cfg: add DMA operations & etc/vmcoreinfo support

[PATCH v6 0/5] fw_cfg: add DMA operations & etc/vmcoreinfo support

[Marc-André Lureau]

Hi,

This series adds DMA operations support to the qemu fw_cfg kernel
module and populates "etc/vmcoreinfo" with vmcoreinfo location
details.

Note: the support for this entry handling has been merged for upcoming
qemu release (2.11).

v6:
- change acpi_acquire_global_lock() error to return EINVAL
  (instead of EBUSY)
- replace 0 as pointer argument for NULL
- add Gabriel r-b/a-b tags

v5:
- resent to CC kdump people on the paddr_vmcoreinfo_note() export patch

v4:
- export paddr_vmcoreinfo_note() to fix fw_cfg.ko build
- fix build with !CONFIG_CRASH_CORE
- replace the unbounded yield() loop with a usleep_range() loop and a
  200ms timeout
- do not write vmcoreinfo entry when running the kdump kernel (D. Hatayama)
- drop the experimental sysfs write support patch from this series

v3: (thanks kbuild)
- add "fw_cfg: fix the command line module name" patch
- fix build of "fw_cfg: add DMA register" with CONFIG_FW_CFG_SYSFS_CMDLINE=y
- fix 'Wshift-count-overflow'

v2:
- use platform device for dma mapping
- add etc/vmcoreinfo patch
- some code cleanups

Marc-André Lureau (5):
  fw_cfg: fix the command line module name
  fw_cfg: add DMA register
  fw_cfg: do DMA read operation
  crash: export paddr_vmcoreinfo_note()
  fw_cfg: write vmcoreinfo details

 drivers/firmware/qemu_fw_cfg.c | 292 +++++++++++++++++++++++++++++++++++++----
 kernel/crash_core.c            |   1 +
 2 files changed, 264 insertions(+), 29 deletions(-)

-- 
2.15.0.125.g8f49766d64

[Qemu-devel] [PATCH v6 0/5] fw_cfg: add DMA operations & etc/vmcoreinfo support

[Marc-André Lureau]

Hi,

This series adds DMA operations support to the qemu fw_cfg kernel
module and populates "etc/vmcoreinfo" with vmcoreinfo location
details.

Note: the support for this entry handling has been merged for upcoming
qemu release (2.11).

v6:
- change acpi_acquire_global_lock() error to return EINVAL
  (instead of EBUSY)
- replace 0 as pointer argument for NULL
- add Gabriel r-b/a-b tags

v5:
- resent to CC kdump people on the paddr_vmcoreinfo_note() export patch

v4:
- export paddr_vmcoreinfo_note() to fix fw_cfg.ko build
- fix build with !CONFIG_CRASH_CORE
- replace the unbounded yield() loop with a usleep_range() loop and a
  200ms timeout
- do not write vmcoreinfo entry when running the kdump kernel (D. Hatayama)
- drop the experimental sysfs write support patch from this series

v3: (thanks kbuild)
- add "fw_cfg: fix the command line module name" patch
- fix build of "fw_cfg: add DMA register" with CONFIG_FW_CFG_SYSFS_CMDLINE=y
- fix 'Wshift-count-overflow'

v2:
- use platform device for dma mapping
- add etc/vmcoreinfo patch
- some code cleanups

Marc-André Lureau (5):
  fw_cfg: fix the command line module name
  fw_cfg: add DMA register
  fw_cfg: do DMA read operation
  crash: export paddr_vmcoreinfo_note()
  fw_cfg: write vmcoreinfo details

 drivers/firmware/qemu_fw_cfg.c | 292 +++++++++++++++++++++++++++++++++++++----
 kernel/crash_core.c            |   1 +
 2 files changed, 264 insertions(+), 29 deletions(-)

-- 
2.15.0.125.g8f49766d64

[PATCH v6 1/5] fw_cfg: fix the command line module name

[Marc-André Lureau]

Signed-off-by: Marc-André Lureau <marcandre.lureau@redhat.com>
Acked-by: Gabriel Somlo <somlo@cmu.edu>
---
 drivers/firmware/qemu_fw_cfg.c | 8 ++++----
 1 file changed, 4 insertions(+), 4 deletions(-)

diff --git a/drivers/firmware/qemu_fw_cfg.c b/drivers/firmware/qemu_fw_cfg.c
index 0e2011636fbb..5cfe39f7a45f 100644
--- a/drivers/firmware/qemu_fw_cfg.c
+++ b/drivers/firmware/qemu_fw_cfg.c
@@ -10,9 +10,9 @@
  * and select subsets of aarch64), a Device Tree node (on arm), or using
  * a kernel module (or command line) parameter with the following syntax:
  *
- *      [fw_cfg.]ioport=<size>@<base>[:<ctrl_off>:<data_off>]
+ *      [qemu_fw_cfg.]ioport=<size>@<base>[:<ctrl_off>:<data_off>]
  * or
- *      [fw_cfg.]mmio=<size>@<base>[:<ctrl_off>:<data_off>]
+ *      [qemu_fw_cfg.]mmio=<size>@<base>[:<ctrl_off>:<data_off>]
  *
  * where:
  *      <size>     := size of ioport or mmio range
@@ -21,9 +21,9 @@
  *      <data_off> := (optional) offset of data register
  *
  * e.g.:
- *      fw_cfg.ioport=2@0x510:0:1		(the default on x86)
+ *      qemu_fw_cfg.ioport=2@0x510:0:1		(the default on x86)
  * or
- *      fw_cfg.mmio=0xA@0x9020000:8:0		(the default on arm)
+ *      qemu_fw_cfg.mmio=0xA@0x9020000:8:0	(the default on arm)
  */
 
 #include <linux/module.h>
-- 
2.15.0.125.g8f49766d64

[Qemu-devel] [PATCH v6 1/5] fw_cfg: fix the command line module name

[Marc-André Lureau]

Signed-off-by: Marc-André Lureau <marcandre.lureau@redhat.com>
Acked-by: Gabriel Somlo <somlo@cmu.edu>
---
 drivers/firmware/qemu_fw_cfg.c | 8 ++++----
 1 file changed, 4 insertions(+), 4 deletions(-)

diff --git a/drivers/firmware/qemu_fw_cfg.c b/drivers/firmware/qemu_fw_cfg.c
index 0e2011636fbb..5cfe39f7a45f 100644
--- a/drivers/firmware/qemu_fw_cfg.c
+++ b/drivers/firmware/qemu_fw_cfg.c
@@ -10,9 +10,9 @@
  * and select subsets of aarch64), a Device Tree node (on arm), or using
  * a kernel module (or command line) parameter with the following syntax:
  *
- *      [fw_cfg.]ioport=<size>@<base>[:<ctrl_off>:<data_off>]
+ *      [qemu_fw_cfg.]ioport=<size>@<base>[:<ctrl_off>:<data_off>]
  * or
- *      [fw_cfg.]mmio=<size>@<base>[:<ctrl_off>:<data_off>]
+ *      [qemu_fw_cfg.]mmio=<size>@<base>[:<ctrl_off>:<data_off>]
  *
  * where:
  *      <size>     := size of ioport or mmio range
@@ -21,9 +21,9 @@
  *      <data_off> := (optional) offset of data register
  *
  * e.g.:
- *      fw_cfg.ioport=2@0x510:0:1		(the default on x86)
+ *      qemu_fw_cfg.ioport=2@0x510:0:1		(the default on x86)
  * or
- *      fw_cfg.mmio=0xA@0x9020000:8:0		(the default on arm)
+ *      qemu_fw_cfg.mmio=0xA@0x9020000:8:0	(the default on arm)
  */
 
 #include <linux/module.h>
-- 
2.15.0.125.g8f49766d64

[PATCH v6 2/5] fw_cfg: add DMA register

[Marc-André Lureau]

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 <marcandre.lureau@redhat.com>
Reviewed-by: Gabriel Somlo <somlo@cmu.edu>
---
 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 5cfe39f7a45f..1f3e8545dab7 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>
@@ -63,6 +64,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);
@@ -118,12 +120,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
@@ -133,7 +137,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;
@@ -170,6 +174,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;
@@ -179,6 +184,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) {
@@ -628,6 +640,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@" \
@@ -637,12 +650,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
@@ -658,19 +674,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;
@@ -687,6 +704,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.
@@ -721,6 +743,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.15.0.125.g8f49766d64

[Qemu-devel] [PATCH v6 2/5] fw_cfg: add DMA register

[Marc-André Lureau]

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 <marcandre.lureau@redhat.com>
Reviewed-by: Gabriel Somlo <somlo@cmu.edu>
---
 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 5cfe39f7a45f..1f3e8545dab7 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>
@@ -63,6 +64,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);
@@ -118,12 +120,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
@@ -133,7 +137,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;
@@ -170,6 +174,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;
@@ -179,6 +184,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) {
@@ -628,6 +640,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@" \
@@ -637,12 +650,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
@@ -658,19 +674,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;
@@ -687,6 +704,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.
@@ -721,6 +743,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.15.0.125.g8f49766d64

[PATCH v6 3/5] fw_cfg: do DMA read operation

[Marc-André Lureau]

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

To avoid polling with unbound amount of time, the DMA operation is
expected to complete within 200ms, or will return ETIME error.

We may want to switch all the *buf addresses to use only kmalloc'ed
buffers (instead of using stack/image addresses with dma=false).

Signed-off-by: Marc-André Lureau <marcandre.lureau@redhat.com>
Reviewed-by: Gabriel Somlo <somlo@cmu.edu>
---
 drivers/firmware/qemu_fw_cfg.c | 154 ++++++++++++++++++++++++++++++++++++-----
 1 file changed, 137 insertions(+), 17 deletions(-)

diff --git a/drivers/firmware/qemu_fw_cfg.c b/drivers/firmware/qemu_fw_cfg.c
index 1f3e8545dab7..2ac4cd869fe6 100644
--- a/drivers/firmware/qemu_fw_cfg.c
+++ b/drivers/firmware/qemu_fw_cfg.c
@@ -33,6 +33,8 @@
 #include <linux/slab.h>
 #include <linux/io.h>
 #include <linux/ioport.h>
+#include <linux/delay.h>
+#include <linux/dma-mapping.h>
 
 MODULE_AUTHOR("Gabriel L. Somlo <somlo@cmu.edu>");
 MODULE_DESCRIPTION("QEMU fw_cfg sysfs support");
@@ -43,12 +45,26 @@ MODULE_LICENSE("GPL");
 #define FW_CFG_ID         0x01
 #define FW_CFG_FILE_DIR   0x19
 
+#define FW_CFG_VERSION_DMA     0x02
+#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_TIMEOUT     200 /* ms */
+
 /* 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
 
+/* platform device for dma mapping */
+static struct device *dev;
+
+/* fw_cfg revision attribute, in /sys/firmware/qemu_fw_cfg top-level dir. */
+static u32 fw_cfg_rev;
+
 /* fw_cfg file directory entry type */
 struct fw_cfg_file {
 	u32 size;
@@ -57,6 +73,12 @@ struct fw_cfg_file {
 	char name[FW_CFG_MAX_FILE_PATH];
 };
 
+struct fw_cfg_dma {
+	u32 control;
+	u32 length;
+	u64 address;
+} __packed;
+
 /* fw_cfg device i/o register addresses */
 static bool fw_cfg_is_mmio;
 static phys_addr_t fw_cfg_p_base;
@@ -75,12 +97,93 @@ static inline u16 fw_cfg_sel_endianness(u16 key)
 	return fw_cfg_is_mmio ? cpu_to_be16(key) : cpu_to_le16(key);
 }
 
+static inline bool fw_cfg_dma_enabled(void)
+{
+	return fw_cfg_rev & FW_CFG_VERSION_DMA && fw_cfg_reg_dma;
+}
+
+static bool fw_cfg_wait_for_control(struct fw_cfg_dma *d, unsigned long timeout)
+{
+	ktime_t start;
+	ktime_t stop;
+
+	start = ktime_get();
+	stop = ktime_add(start, ms_to_ktime(timeout));
+
+	do {
+		if ((be32_to_cpu(d->control) & ~FW_CFG_DMA_CTL_ERROR) == 0)
+			return true;
+
+		usleep_range(50, 100);
+	} while (ktime_before(ktime_get(), stop));
+
+	return false;
+}
+
+static ssize_t fw_cfg_dma_transfer(void *address, u32 length, u32 control)
+{
+	dma_addr_t dma_addr = 0;
+	struct fw_cfg_dma *d;
+	dma_addr_t dma;
+	ssize_t ret = length;
+	enum dma_data_direction dir =
+		(control & FW_CFG_DMA_CTL_READ ? DMA_FROM_DEVICE : 0);
+
+	if (address && length) {
+		dma_addr = dma_map_single(dev, address, length, dir);
+		if (dma_mapping_error(NULL, dma_addr)) {
+			WARN(1, "%s: failed to map address\n", __func__);
+			return -EFAULT;
+		}
+	}
+
+	d = kmalloc(sizeof(*d), GFP_KERNEL | GFP_DMA);
+	if (!d) {
+		ret = -ENOMEM;
+		goto end;
+	}
+
+	dma = dma_map_single(dev, d, sizeof(*d), DMA_BIDIRECTIONAL);
+	if (dma_mapping_error(NULL, dma)) {
+		WARN(1, "%s: failed to map fw_cfg_dma\n", __func__);
+		ret = -EFAULT;
+		goto end;
+	}
+
+	*d = (struct fw_cfg_dma) {
+		.address = cpu_to_be64(dma_addr),
+		.length = cpu_to_be32(length),
+		.control = cpu_to_be32(control)
+	};
+
+	iowrite32be((u64)dma >> 32, fw_cfg_reg_dma);
+	iowrite32be(dma, fw_cfg_reg_dma + 4);
+
+	if (!fw_cfg_wait_for_control(d, FW_CFG_DMA_TIMEOUT)) {
+		WARN(1, "%s: timeout", __func__);
+		ret = -ETIME;
+	} else if (be32_to_cpu(d->control) & FW_CFG_DMA_CTL_ERROR) {
+		ret = -EIO;
+	}
+
+	dma_unmap_single(dev, dma, sizeof(*d), DMA_BIDIRECTIONAL);
+
+end:
+	kfree(d);
+	if (dma_addr)
+		dma_unmap_single(dev, dma_addr, length, dir);
+
+	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)
+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:
@@ -90,17 +193,36 @@ 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()) {
+		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)
+				goto end;
+			ret = fw_cfg_dma_transfer(buf, count,
+						  FW_CFG_DMA_CTL_READ);
+		}
+	} else {
+		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);
+	}
+
+end:
 	mutex_unlock(&fw_cfg_dev_lock);
 
 	acpi_release_global_lock(glk);
+
+	return ret;
 }
 
 /* clean up fw_cfg device i/o */
@@ -192,7 +314,7 @@ 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);
+	fw_cfg_read_blob(FW_CFG_SIGNATURE, sig, 0, FW_CFG_SIG_SIZE, false);
 	if (memcmp(sig, "QEMU", FW_CFG_SIG_SIZE) != 0) {
 		fw_cfg_io_cleanup();
 		return -ENODEV;
@@ -201,9 +323,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);
@@ -351,8 +470,7 @@ static ssize_t fw_cfg_sysfs_read_raw(struct file *filp, struct kobject *kobj,
 	if (count > entry->f.size - pos)
 		count = entry->f.size - pos;
 
-	fw_cfg_read_blob(entry->f.select, buf, pos, count);
-	return count;
+	return fw_cfg_read_blob(entry->f.select, buf, pos, count, true);
 }
 
 static struct bin_attribute fw_cfg_sysfs_attr_raw = {
@@ -505,7 +623,7 @@ 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, &count, 0, sizeof(count));
+	fw_cfg_read_blob(FW_CFG_FILE_DIR, &count, 0, sizeof(count), false);
 	count = be32_to_cpu(count);
 	dir_size = count * sizeof(struct fw_cfg_file);
 
@@ -513,7 +631,7 @@ static int fw_cfg_register_dir_entries(void)
 	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(count), dir_size, true);
 
 	for (i = 0; i < count; i++) {
 		dir[i].size = be32_to_cpu(dir[i].size);
@@ -544,9 +662,10 @@ static int fw_cfg_sysfs_probe(struct platform_device *pdev)
 	 * one fw_cfg device exist system-wide, so if one was already found
 	 * earlier, we might as well stop here.
 	 */
-	if (fw_cfg_sel_ko)
+	if (dev)
 		return -EBUSY;
 
+	dev = &pdev->dev;
 	/* create by_key and by_name subdirs of /sys/firmware/qemu_fw_cfg/ */
 	err = -ENOMEM;
 	fw_cfg_sel_ko = kobject_create_and_add("by_key", fw_cfg_top_ko);
@@ -562,7 +681,7 @@ 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_read_blob(FW_CFG_ID, &fw_cfg_rev, 0, sizeof(fw_cfg_rev), false);
 	fw_cfg_rev = le32_to_cpu(fw_cfg_rev);
 	err = sysfs_create_file(fw_cfg_top_ko, &fw_cfg_rev_attr.attr);
 	if (err)
@@ -587,6 +706,7 @@ static int fw_cfg_sysfs_probe(struct platform_device *pdev)
 err_name:
 	fw_cfg_kobj_cleanup(fw_cfg_sel_ko);
 err_sel:
+	dev = NULL;
 	return err;
 }
 
-- 
2.15.0.125.g8f49766d64

[Qemu-devel] [PATCH v6 3/5] fw_cfg: do DMA read operation

[Marc-André Lureau]

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

To avoid polling with unbound amount of time, the DMA operation is
expected to complete within 200ms, or will return ETIME error.

We may want to switch all the *buf addresses to use only kmalloc'ed
buffers (instead of using stack/image addresses with dma=false).

Signed-off-by: Marc-André Lureau <marcandre.lureau@redhat.com>
Reviewed-by: Gabriel Somlo <somlo@cmu.edu>
---
 drivers/firmware/qemu_fw_cfg.c | 154 ++++++++++++++++++++++++++++++++++++-----
 1 file changed, 137 insertions(+), 17 deletions(-)

diff --git a/drivers/firmware/qemu_fw_cfg.c b/drivers/firmware/qemu_fw_cfg.c
index 1f3e8545dab7..2ac4cd869fe6 100644
--- a/drivers/firmware/qemu_fw_cfg.c
+++ b/drivers/firmware/qemu_fw_cfg.c
@@ -33,6 +33,8 @@
 #include <linux/slab.h>
 #include <linux/io.h>
 #include <linux/ioport.h>
+#include <linux/delay.h>
+#include <linux/dma-mapping.h>
 
 MODULE_AUTHOR("Gabriel L. Somlo <somlo@cmu.edu>");
 MODULE_DESCRIPTION("QEMU fw_cfg sysfs support");
@@ -43,12 +45,26 @@ MODULE_LICENSE("GPL");
 #define FW_CFG_ID         0x01
 #define FW_CFG_FILE_DIR   0x19
 
+#define FW_CFG_VERSION_DMA     0x02
+#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_TIMEOUT     200 /* ms */
+
 /* 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
 
+/* platform device for dma mapping */
+static struct device *dev;
+
+/* fw_cfg revision attribute, in /sys/firmware/qemu_fw_cfg top-level dir. */
+static u32 fw_cfg_rev;
+
 /* fw_cfg file directory entry type */
 struct fw_cfg_file {
 	u32 size;
@@ -57,6 +73,12 @@ struct fw_cfg_file {
 	char name[FW_CFG_MAX_FILE_PATH];
 };
 
+struct fw_cfg_dma {
+	u32 control;
+	u32 length;
+	u64 address;
+} __packed;
+
 /* fw_cfg device i/o register addresses */
 static bool fw_cfg_is_mmio;
 static phys_addr_t fw_cfg_p_base;
@@ -75,12 +97,93 @@ static inline u16 fw_cfg_sel_endianness(u16 key)
 	return fw_cfg_is_mmio ? cpu_to_be16(key) : cpu_to_le16(key);
 }
 
+static inline bool fw_cfg_dma_enabled(void)
+{
+	return fw_cfg_rev & FW_CFG_VERSION_DMA && fw_cfg_reg_dma;
+}
+
+static bool fw_cfg_wait_for_control(struct fw_cfg_dma *d, unsigned long timeout)
+{
+	ktime_t start;
+	ktime_t stop;
+
+	start = ktime_get();
+	stop = ktime_add(start, ms_to_ktime(timeout));
+
+	do {
+		if ((be32_to_cpu(d->control) & ~FW_CFG_DMA_CTL_ERROR) == 0)
+			return true;
+
+		usleep_range(50, 100);
+	} while (ktime_before(ktime_get(), stop));
+
+	return false;
+}
+
+static ssize_t fw_cfg_dma_transfer(void *address, u32 length, u32 control)
+{
+	dma_addr_t dma_addr = 0;
+	struct fw_cfg_dma *d;
+	dma_addr_t dma;
+	ssize_t ret = length;
+	enum dma_data_direction dir =
+		(control & FW_CFG_DMA_CTL_READ ? DMA_FROM_DEVICE : 0);
+
+	if (address && length) {
+		dma_addr = dma_map_single(dev, address, length, dir);
+		if (dma_mapping_error(NULL, dma_addr)) {
+			WARN(1, "%s: failed to map address\n", __func__);
+			return -EFAULT;
+		}
+	}
+
+	d = kmalloc(sizeof(*d), GFP_KERNEL | GFP_DMA);
+	if (!d) {
+		ret = -ENOMEM;
+		goto end;
+	}
+
+	dma = dma_map_single(dev, d, sizeof(*d), DMA_BIDIRECTIONAL);
+	if (dma_mapping_error(NULL, dma)) {
+		WARN(1, "%s: failed to map fw_cfg_dma\n", __func__);
+		ret = -EFAULT;
+		goto end;
+	}
+
+	*d = (struct fw_cfg_dma) {
+		.address = cpu_to_be64(dma_addr),
+		.length = cpu_to_be32(length),
+		.control = cpu_to_be32(control)
+	};
+
+	iowrite32be((u64)dma >> 32, fw_cfg_reg_dma);
+	iowrite32be(dma, fw_cfg_reg_dma + 4);
+
+	if (!fw_cfg_wait_for_control(d, FW_CFG_DMA_TIMEOUT)) {
+		WARN(1, "%s: timeout", __func__);
+		ret = -ETIME;
+	} else if (be32_to_cpu(d->control) & FW_CFG_DMA_CTL_ERROR) {
+		ret = -EIO;
+	}
+
+	dma_unmap_single(dev, dma, sizeof(*d), DMA_BIDIRECTIONAL);
+
+end:
+	kfree(d);
+	if (dma_addr)
+		dma_unmap_single(dev, dma_addr, length, dir);
+
+	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)
+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:
@@ -90,17 +193,36 @@ 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()) {
+		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)
+				goto end;
+			ret = fw_cfg_dma_transfer(buf, count,
+						  FW_CFG_DMA_CTL_READ);
+		}
+	} else {
+		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);
+	}
+
+end:
 	mutex_unlock(&fw_cfg_dev_lock);
 
 	acpi_release_global_lock(glk);
+
+	return ret;
 }
 
 /* clean up fw_cfg device i/o */
@@ -192,7 +314,7 @@ 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);
+	fw_cfg_read_blob(FW_CFG_SIGNATURE, sig, 0, FW_CFG_SIG_SIZE, false);
 	if (memcmp(sig, "QEMU", FW_CFG_SIG_SIZE) != 0) {
 		fw_cfg_io_cleanup();
 		return -ENODEV;
@@ -201,9 +323,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);
@@ -351,8 +470,7 @@ static ssize_t fw_cfg_sysfs_read_raw(struct file *filp, struct kobject *kobj,
 	if (count > entry->f.size - pos)
 		count = entry->f.size - pos;
 
-	fw_cfg_read_blob(entry->f.select, buf, pos, count);
-	return count;
+	return fw_cfg_read_blob(entry->f.select, buf, pos, count, true);
 }
 
 static struct bin_attribute fw_cfg_sysfs_attr_raw = {
@@ -505,7 +623,7 @@ 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, &count, 0, sizeof(count));
+	fw_cfg_read_blob(FW_CFG_FILE_DIR, &count, 0, sizeof(count), false);
 	count = be32_to_cpu(count);
 	dir_size = count * sizeof(struct fw_cfg_file);
 
@@ -513,7 +631,7 @@ static int fw_cfg_register_dir_entries(void)
 	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(count), dir_size, true);
 
 	for (i = 0; i < count; i++) {
 		dir[i].size = be32_to_cpu(dir[i].size);
@@ -544,9 +662,10 @@ static int fw_cfg_sysfs_probe(struct platform_device *pdev)
 	 * one fw_cfg device exist system-wide, so if one was already found
 	 * earlier, we might as well stop here.
 	 */
-	if (fw_cfg_sel_ko)
+	if (dev)
 		return -EBUSY;
 
+	dev = &pdev->dev;
 	/* create by_key and by_name subdirs of /sys/firmware/qemu_fw_cfg/ */
 	err = -ENOMEM;
 	fw_cfg_sel_ko = kobject_create_and_add("by_key", fw_cfg_top_ko);
@@ -562,7 +681,7 @@ 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_read_blob(FW_CFG_ID, &fw_cfg_rev, 0, sizeof(fw_cfg_rev), false);
 	fw_cfg_rev = le32_to_cpu(fw_cfg_rev);
 	err = sysfs_create_file(fw_cfg_top_ko, &fw_cfg_rev_attr.attr);
 	if (err)
@@ -587,6 +706,7 @@ static int fw_cfg_sysfs_probe(struct platform_device *pdev)
 err_name:
 	fw_cfg_kobj_cleanup(fw_cfg_sel_ko);
 err_sel:
+	dev = NULL;
 	return err;
 }
 
-- 
2.15.0.125.g8f49766d64

[PATCH v6 4/5] crash: export paddr_vmcoreinfo_note()

[Marc-André Lureau]

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 <akpm@linux-foundation.org>
CC: Baoquan He <bhe@redhat.com>
CC: Dave Young <dyoung@redhat.com>
CC: Dave Young <dyoung@redhat.com>
CC: Hari Bathini <hbathini@linux.vnet.ibm.com>
CC: Tony Luck <tony.luck@intel.com>
CC: Vivek Goyal <vgoyal@redhat.com>
Signed-off-by: Marc-André Lureau <marcandre.lureau@redhat.com>
Acked-by: Gabriel Somlo <somlo@cmu.edu>
---
 kernel/crash_core.c | 1 +
 1 file changed, 1 insertion(+)

diff --git a/kernel/crash_core.c b/kernel/crash_core.c
index 6db80fc0810b..47541c891810 100644
--- a/kernel/crash_core.c
+++ b/kernel/crash_core.c
@@ -375,6 +375,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.15.0.125.g8f49766d64

[Qemu-devel] [PATCH v6 4/5] crash: export paddr_vmcoreinfo_note()

[Marc-André Lureau]

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 <akpm@linux-foundation.org>
CC: Baoquan He <bhe@redhat.com>
CC: Dave Young <dyoung@redhat.com>
CC: Dave Young <dyoung@redhat.com>
CC: Hari Bathini <hbathini@linux.vnet.ibm.com>
CC: Tony Luck <tony.luck@intel.com>
CC: Vivek Goyal <vgoyal@redhat.com>
Signed-off-by: Marc-André Lureau <marcandre.lureau@redhat.com>
Acked-by: Gabriel Somlo <somlo@cmu.edu>
---
 kernel/crash_core.c | 1 +
 1 file changed, 1 insertion(+)

diff --git a/kernel/crash_core.c b/kernel/crash_core.c
index 6db80fc0810b..47541c891810 100644
--- a/kernel/crash_core.c
+++ b/kernel/crash_core.c
@@ -375,6 +375,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.15.0.125.g8f49766d64

[PATCH v6 5/5] fw_cfg: write vmcoreinfo details

[Marc-André Lureau]

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.

Signed-off-by: Marc-André Lureau <marcandre.lureau@redhat.com>
Reviewed-by: Gabriel Somlo <somlo@cmu.edu>
---
 drivers/firmware/qemu_fw_cfg.c | 87 +++++++++++++++++++++++++++++++++++++++++-
 1 file changed, 86 insertions(+), 1 deletion(-)

diff --git a/drivers/firmware/qemu_fw_cfg.c b/drivers/firmware/qemu_fw_cfg.c
index 2ac4cd869fe6..7a70e7a549f6 100644
--- a/drivers/firmware/qemu_fw_cfg.c
+++ b/drivers/firmware/qemu_fw_cfg.c
@@ -35,6 +35,8 @@
 #include <linux/ioport.h>
 #include <linux/delay.h>
 #include <linux/dma-mapping.h>
+#include <linux/crash_core.h>
+#include <linux/crash_dump.h>
 
 MODULE_AUTHOR("Gabriel L. Somlo <somlo@cmu.edu>");
 MODULE_DESCRIPTION("QEMU fw_cfg sysfs support");
@@ -59,6 +61,8 @@ MODULE_LICENSE("GPL");
 /* fw_cfg "file name" is up to 56 characters (including terminating nul) */
 #define FW_CFG_MAX_FILE_PATH 56
 
+#define VMCOREINFO_FORMAT_ELF 0x1
+
 /* platform device for dma mapping */
 static struct device *dev;
 
@@ -127,7 +131,8 @@ static ssize_t fw_cfg_dma_transfer(void *address, u32 length, u32 control)
 	dma_addr_t dma;
 	ssize_t ret = length;
 	enum dma_data_direction dir =
-		(control & FW_CFG_DMA_CTL_READ ? DMA_FROM_DEVICE : 0);
+		(control & FW_CFG_DMA_CTL_READ ? DMA_FROM_DEVICE : 0) |
+		(control & FW_CFG_DMA_CTL_WRITE ? DMA_TO_DEVICE : 0);
 
 	if (address && length) {
 		dma_addr = dma_map_single(dev, address, length, dir);
@@ -225,6 +230,48 @@ static ssize_t fw_cfg_read_blob(u16 key,
 	return ret;
 }
 
+#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__);
+		memset(buf, 0, count);
+		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)
 {
@@ -343,6 +390,37 @@ struct fw_cfg_sysfs_entry {
 	struct list_head list;
 };
 
+#ifdef CONFIG_CRASH_CORE
+static ssize_t write_vmcoreinfo(const struct fw_cfg_file *f)
+{
+	struct vmci {
+		__le16 host_format;
+		__le16 guest_format;
+		__le32 size;
+		__le64 paddr;
+	} __packed;
+	struct vmci *data;
+	ssize_t ret;
+
+	data = kmalloc(sizeof(struct vmci), GFP_KERNEL | GFP_DMA);
+	if (!data)
+		return -ENOMEM;
+
+	/* spare ourself reading host format support for now since we
+	 * don't know what else to format - host may ignore ours
+	 */
+	*data = (struct vmci) {
+		.guest_format = cpu_to_le16(VMCOREINFO_FORMAT_ELF),
+		.size = cpu_to_le32(VMCOREINFO_NOTE_SIZE),
+		.paddr = cpu_to_le64(paddr_vmcoreinfo_note())
+	};
+	ret = fw_cfg_write_blob(f->select, data, 0, sizeof(struct vmci));
+
+	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)
 {
@@ -582,6 +660,13 @@ 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 (strcmp(f->name, "etc/vmcoreinfo") == 0 && !is_kdump_kernel()) {
+		if (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.15.0.125.g8f49766d64

[Qemu-devel] [PATCH v6 5/5] fw_cfg: write vmcoreinfo details

[Marc-André Lureau]

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.

Signed-off-by: Marc-André Lureau <marcandre.lureau@redhat.com>
Reviewed-by: Gabriel Somlo <somlo@cmu.edu>
---
 drivers/firmware/qemu_fw_cfg.c | 87 +++++++++++++++++++++++++++++++++++++++++-
 1 file changed, 86 insertions(+), 1 deletion(-)

diff --git a/drivers/firmware/qemu_fw_cfg.c b/drivers/firmware/qemu_fw_cfg.c
index 2ac4cd869fe6..7a70e7a549f6 100644
--- a/drivers/firmware/qemu_fw_cfg.c
+++ b/drivers/firmware/qemu_fw_cfg.c
@@ -35,6 +35,8 @@
 #include <linux/ioport.h>
 #include <linux/delay.h>
 #include <linux/dma-mapping.h>
+#include <linux/crash_core.h>
+#include <linux/crash_dump.h>
 
 MODULE_AUTHOR("Gabriel L. Somlo <somlo@cmu.edu>");
 MODULE_DESCRIPTION("QEMU fw_cfg sysfs support");
@@ -59,6 +61,8 @@ MODULE_LICENSE("GPL");
 /* fw_cfg "file name" is up to 56 characters (including terminating nul) */
 #define FW_CFG_MAX_FILE_PATH 56
 
+#define VMCOREINFO_FORMAT_ELF 0x1
+
 /* platform device for dma mapping */
 static struct device *dev;
 
@@ -127,7 +131,8 @@ static ssize_t fw_cfg_dma_transfer(void *address, u32 length, u32 control)
 	dma_addr_t dma;
 	ssize_t ret = length;
 	enum dma_data_direction dir =
-		(control & FW_CFG_DMA_CTL_READ ? DMA_FROM_DEVICE : 0);
+		(control & FW_CFG_DMA_CTL_READ ? DMA_FROM_DEVICE : 0) |
+		(control & FW_CFG_DMA_CTL_WRITE ? DMA_TO_DEVICE : 0);
 
 	if (address && length) {
 		dma_addr = dma_map_single(dev, address, length, dir);
@@ -225,6 +230,48 @@ static ssize_t fw_cfg_read_blob(u16 key,
 	return ret;
 }
 
+#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__);
+		memset(buf, 0, count);
+		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)
 {
@@ -343,6 +390,37 @@ struct fw_cfg_sysfs_entry {
 	struct list_head list;
 };
 
+#ifdef CONFIG_CRASH_CORE
+static ssize_t write_vmcoreinfo(const struct fw_cfg_file *f)
+{
+	struct vmci {
+		__le16 host_format;
+		__le16 guest_format;
+		__le32 size;
+		__le64 paddr;
+	} __packed;
+	struct vmci *data;
+	ssize_t ret;
+
+	data = kmalloc(sizeof(struct vmci), GFP_KERNEL | GFP_DMA);
+	if (!data)
+		return -ENOMEM;
+
+	/* spare ourself reading host format support for now since we
+	 * don't know what else to format - host may ignore ours
+	 */
+	*data = (struct vmci) {
+		.guest_format = cpu_to_le16(VMCOREINFO_FORMAT_ELF),
+		.size = cpu_to_le32(VMCOREINFO_NOTE_SIZE),
+		.paddr = cpu_to_le64(paddr_vmcoreinfo_note())
+	};
+	ret = fw_cfg_write_blob(f->select, data, 0, sizeof(struct vmci));
+
+	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)
 {
@@ -582,6 +660,13 @@ 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 (strcmp(f->name, "etc/vmcoreinfo") == 0 && !is_kdump_kernel()) {
+		if (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.15.0.125.g8f49766d64

Re: [PATCH v6 4/5] crash: export paddr_vmcoreinfo_note()

[Dave Young]

On 11/13/17 at 08:29pm, Marc-André Lureau wrote:
> 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 <akpm@linux-foundation.org>
> CC: Baoquan He <bhe@redhat.com>
> CC: Dave Young <dyoung@redhat.com>
> CC: Dave Young <dyoung@redhat.com>
> CC: Hari Bathini <hbathini@linux.vnet.ibm.com>
> CC: Tony Luck <tony.luck@intel.com>
> CC: Vivek Goyal <vgoyal@redhat.com>
> Signed-off-by: Marc-André Lureau <marcandre.lureau@redhat.com>
> Acked-by: Gabriel Somlo <somlo@cmu.edu>
> ---
>  kernel/crash_core.c | 1 +
>  1 file changed, 1 insertion(+)
> 
> diff --git a/kernel/crash_core.c b/kernel/crash_core.c
> index 6db80fc0810b..47541c891810 100644
> --- a/kernel/crash_core.c
> +++ b/kernel/crash_core.c
> @@ -375,6 +375,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.15.0.125.g8f49766d64
> 

Acked-by: Dave Young <dyoung@redhat.com>

Thanks
Dave

Re: [Qemu-devel] [PATCH v6 4/5] crash: export paddr_vmcoreinfo_note()

[Dave Young]

On 11/13/17 at 08:29pm, Marc-André Lureau wrote:
> 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 <akpm@linux-foundation.org>
> CC: Baoquan He <bhe@redhat.com>
> CC: Dave Young <dyoung@redhat.com>
> CC: Dave Young <dyoung@redhat.com>
> CC: Hari Bathini <hbathini@linux.vnet.ibm.com>
> CC: Tony Luck <tony.luck@intel.com>
> CC: Vivek Goyal <vgoyal@redhat.com>
> Signed-off-by: Marc-André Lureau <marcandre.lureau@redhat.com>
> Acked-by: Gabriel Somlo <somlo@cmu.edu>
> ---
>  kernel/crash_core.c | 1 +
>  1 file changed, 1 insertion(+)
> 
> diff --git a/kernel/crash_core.c b/kernel/crash_core.c
> index 6db80fc0810b..47541c891810 100644
> --- a/kernel/crash_core.c
> +++ b/kernel/crash_core.c
> @@ -375,6 +375,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.15.0.125.g8f49766d64
> 

Acked-by: Dave Young <dyoung@redhat.com>

Thanks
Dave

Re: [PATCH v6 4/5] crash: export paddr_vmcoreinfo_note()

[Dave Young]

On 11/13/17 at 08:29pm, Marc-André Lureau wrote:
> 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 <akpm@linux-foundation.org>
> CC: Baoquan He <bhe@redhat.com>
> CC: Dave Young <dyoung@redhat.com>
> CC: Dave Young <dyoung@redhat.com>
> CC: Hari Bathini <hbathini@linux.vnet.ibm.com>
> CC: Tony Luck <tony.luck@intel.com>
> CC: Vivek Goyal <vgoyal@redhat.com>
> Signed-off-by: Marc-André Lureau <marcandre.lureau@redhat.com>
> Acked-by: Gabriel Somlo <somlo@cmu.edu>
> ---
>  kernel/crash_core.c | 1 +
>  1 file changed, 1 insertion(+)
> 
> diff --git a/kernel/crash_core.c b/kernel/crash_core.c
> index 6db80fc0810b..47541c891810 100644
> --- a/kernel/crash_core.c
> +++ b/kernel/crash_core.c
> @@ -375,6 +375,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.15.0.125.g8f49766d64
> 

Acked-by: Dave Young <dyoung@redhat.com>

Thanks
Dave

_______________________________________________
kexec mailing list
kexec@lists.infradead.org
http://lists.infradead.org/mailman/listinfo/kexec

Re: [PATCH v6 4/5] crash: export paddr_vmcoreinfo_note()

[Baoquan He]

On 11/13/17 at 08:29pm, Marc-André Lureau wrote:
> 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 <akpm@linux-foundation.org>
> CC: Baoquan He <bhe@redhat.com>
> CC: Dave Young <dyoung@redhat.com>
> CC: Dave Young <dyoung@redhat.com>
> CC: Hari Bathini <hbathini@linux.vnet.ibm.com>
> CC: Tony Luck <tony.luck@intel.com>
> CC: Vivek Goyal <vgoyal@redhat.com>
> Signed-off-by: Marc-André Lureau <marcandre.lureau@redhat.com>
> Acked-by: Gabriel Somlo <somlo@cmu.edu>

ACK

Acked-by: Baoquan He <bhe@redhat.com>

Thanks
Baoquan

> ---
>  kernel/crash_core.c | 1 +
>  1 file changed, 1 insertion(+)
> 
> diff --git a/kernel/crash_core.c b/kernel/crash_core.c
> index 6db80fc0810b..47541c891810 100644
> --- a/kernel/crash_core.c
> +++ b/kernel/crash_core.c
> @@ -375,6 +375,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.15.0.125.g8f49766d64
> 

Re: [Qemu-devel] [PATCH v6 4/5] crash: export paddr_vmcoreinfo_note()

[Baoquan He]

On 11/13/17 at 08:29pm, Marc-André Lureau wrote:
> 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 <akpm@linux-foundation.org>
> CC: Baoquan He <bhe@redhat.com>
> CC: Dave Young <dyoung@redhat.com>
> CC: Dave Young <dyoung@redhat.com>
> CC: Hari Bathini <hbathini@linux.vnet.ibm.com>
> CC: Tony Luck <tony.luck@intel.com>
> CC: Vivek Goyal <vgoyal@redhat.com>
> Signed-off-by: Marc-André Lureau <marcandre.lureau@redhat.com>
> Acked-by: Gabriel Somlo <somlo@cmu.edu>

ACK

Acked-by: Baoquan He <bhe@redhat.com>

Thanks
Baoquan

> ---
>  kernel/crash_core.c | 1 +
>  1 file changed, 1 insertion(+)
> 
> diff --git a/kernel/crash_core.c b/kernel/crash_core.c
> index 6db80fc0810b..47541c891810 100644
> --- a/kernel/crash_core.c
> +++ b/kernel/crash_core.c
> @@ -375,6 +375,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.15.0.125.g8f49766d64
> 

Re: [PATCH v6 3/5] fw_cfg: do DMA read operation

[Michael S. Tsirkin]

On Mon, Nov 13, 2017 at 08:29:56PM +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.
> 
> To avoid polling with unbound amount of time, the DMA operation is
> expected to complete within 200ms, or will return ETIME error.
> 
> We may want to switch all the *buf addresses to use only kmalloc'ed
> buffers (instead of using stack/image addresses with dma=false).
> 
> Signed-off-by: Marc-André Lureau <marcandre.lureau@redhat.com>
> Reviewed-by: Gabriel Somlo <somlo@cmu.edu>
> ---
>  drivers/firmware/qemu_fw_cfg.c | 154 ++++++++++++++++++++++++++++++++++++-----
>  1 file changed, 137 insertions(+), 17 deletions(-)
> 
> diff --git a/drivers/firmware/qemu_fw_cfg.c b/drivers/firmware/qemu_fw_cfg.c
> index 1f3e8545dab7..2ac4cd869fe6 100644
> --- a/drivers/firmware/qemu_fw_cfg.c
> +++ b/drivers/firmware/qemu_fw_cfg.c
> @@ -33,6 +33,8 @@
>  #include <linux/slab.h>
>  #include <linux/io.h>
>  #include <linux/ioport.h>
> +#include <linux/delay.h>
> +#include <linux/dma-mapping.h>
>  
>  MODULE_AUTHOR("Gabriel L. Somlo <somlo@cmu.edu>");
>  MODULE_DESCRIPTION("QEMU fw_cfg sysfs support");
> @@ -43,12 +45,26 @@ MODULE_LICENSE("GPL");
>  #define FW_CFG_ID         0x01
>  #define FW_CFG_FILE_DIR   0x19
>  
> +#define FW_CFG_VERSION_DMA     0x02
> +#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_TIMEOUT     200 /* ms */
> +
>  /* 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
>  
> +/* platform device for dma mapping */
> +static struct device *dev;
> +
> +/* fw_cfg revision attribute, in /sys/firmware/qemu_fw_cfg top-level dir. */
> +static u32 fw_cfg_rev;
> +
>  /* fw_cfg file directory entry type */
>  struct fw_cfg_file {
>  	u32 size;
> @@ -57,6 +73,12 @@ struct fw_cfg_file {
>  	char name[FW_CFG_MAX_FILE_PATH];
>  };
>  
> +struct fw_cfg_dma {
> +	u32 control;
> +	u32 length;
> +	u64 address;
> +} __packed;
> +
>  /* fw_cfg device i/o register addresses */
>  static bool fw_cfg_is_mmio;
>  static phys_addr_t fw_cfg_p_base;
> @@ -75,12 +97,93 @@ static inline u16 fw_cfg_sel_endianness(u16 key)
>  	return fw_cfg_is_mmio ? cpu_to_be16(key) : cpu_to_le16(key);
>  }
>  
> +static inline bool fw_cfg_dma_enabled(void)
> +{
> +	return fw_cfg_rev & FW_CFG_VERSION_DMA && fw_cfg_reg_dma;
> +}
> +
> +static bool fw_cfg_wait_for_control(struct fw_cfg_dma *d, unsigned long timeout)
> +{
> +	ktime_t start;
> +	ktime_t stop;
> +
> +	start = ktime_get();
> +	stop = ktime_add(start, ms_to_ktime(timeout));
> +
> +	do {
> +		if ((be32_to_cpu(d->control) & ~FW_CFG_DMA_CTL_ERROR) == 0)
> +			return true;
> +
> +		usleep_range(50, 100);

BTW it's not nice that this is uninterruptible. I think we need a
variant of usleep_range that is interruptible and call that here.
Thoughts?


> +	} while (ktime_before(ktime_get(), stop));
> +
> +	return false;
> +}
> +
> +static ssize_t fw_cfg_dma_transfer(void *address, u32 length, u32 control)
> +{
> +	dma_addr_t dma_addr = 0;
> +	struct fw_cfg_dma *d;
> +	dma_addr_t dma;
> +	ssize_t ret = length;
> +	enum dma_data_direction dir =
> +		(control & FW_CFG_DMA_CTL_READ ? DMA_FROM_DEVICE : 0);
> +
> +	if (address && length) {
> +		dma_addr = dma_map_single(dev, address, length, dir);
> +		if (dma_mapping_error(NULL, dma_addr)) {
> +			WARN(1, "%s: failed to map address\n", __func__);
> +			return -EFAULT;
> +		}
> +	}
> +
> +	d = kmalloc(sizeof(*d), GFP_KERNEL | GFP_DMA);
> +	if (!d) {
> +		ret = -ENOMEM;
> +		goto end;
> +	}
> +
> +	dma = dma_map_single(dev, d, sizeof(*d), DMA_BIDIRECTIONAL);
> +	if (dma_mapping_error(NULL, dma)) {
> +		WARN(1, "%s: failed to map fw_cfg_dma\n", __func__);
> +		ret = -EFAULT;
> +		goto end;
> +	}
> +
> +	*d = (struct fw_cfg_dma) {
> +		.address = cpu_to_be64(dma_addr),
> +		.length = cpu_to_be32(length),
> +		.control = cpu_to_be32(control)
> +	};
> +
> +	iowrite32be((u64)dma >> 32, fw_cfg_reg_dma);
> +	iowrite32be(dma, fw_cfg_reg_dma + 4);
> +
> +	if (!fw_cfg_wait_for_control(d, FW_CFG_DMA_TIMEOUT)) {
> +		WARN(1, "%s: timeout", __func__);
> +		ret = -ETIME;
> +	} else if (be32_to_cpu(d->control) & FW_CFG_DMA_CTL_ERROR) {
> +		ret = -EIO;
> +	}
> +
> +	dma_unmap_single(dev, dma, sizeof(*d), DMA_BIDIRECTIONAL);
> +
> +end:
> +	kfree(d);
> +	if (dma_addr)
> +		dma_unmap_single(dev, dma_addr, length, dir);

So if host was delayed what this does is corrupt guest memory.
IMHO things are already bad, let's at least keep it mapped
and allocated. Also what will heppen on next access attempt?
Maybe we need to prevent further attempts.
And maybe fw cfg needs a reset register so we can recover
faster.



> +
> +	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)
> +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:
> @@ -90,17 +193,36 @@ 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()) {
> +		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)
> +				goto end;
> +			ret = fw_cfg_dma_transfer(buf, count,
> +						  FW_CFG_DMA_CTL_READ);
> +		}
> +	} else {
> +		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);
> +	}
> +
> +end:
>  	mutex_unlock(&fw_cfg_dev_lock);
>  
>  	acpi_release_global_lock(glk);
> +
> +	return ret;
>  }
>  
>  /* clean up fw_cfg device i/o */
> @@ -192,7 +314,7 @@ 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);
> +	fw_cfg_read_blob(FW_CFG_SIGNATURE, sig, 0, FW_CFG_SIG_SIZE, false);
>  	if (memcmp(sig, "QEMU", FW_CFG_SIG_SIZE) != 0) {
>  		fw_cfg_io_cleanup();
>  		return -ENODEV;
> @@ -201,9 +323,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);
> @@ -351,8 +470,7 @@ static ssize_t fw_cfg_sysfs_read_raw(struct file *filp, struct kobject *kobj,
>  	if (count > entry->f.size - pos)
>  		count = entry->f.size - pos;
>  
> -	fw_cfg_read_blob(entry->f.select, buf, pos, count);
> -	return count;
> +	return fw_cfg_read_blob(entry->f.select, buf, pos, count, true);
>  }
>  
>  static struct bin_attribute fw_cfg_sysfs_attr_raw = {
> @@ -505,7 +623,7 @@ 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, &count, 0, sizeof(count));
> +	fw_cfg_read_blob(FW_CFG_FILE_DIR, &count, 0, sizeof(count), false);
>  	count = be32_to_cpu(count);
>  	dir_size = count * sizeof(struct fw_cfg_file);
>  
> @@ -513,7 +631,7 @@ static int fw_cfg_register_dir_entries(void)
>  	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(count), dir_size, true);
>  
>  	for (i = 0; i < count; i++) {
>  		dir[i].size = be32_to_cpu(dir[i].size);
> @@ -544,9 +662,10 @@ static int fw_cfg_sysfs_probe(struct platform_device *pdev)
>  	 * one fw_cfg device exist system-wide, so if one was already found
>  	 * earlier, we might as well stop here.
>  	 */
> -	if (fw_cfg_sel_ko)
> +	if (dev)
>  		return -EBUSY;
>  
> +	dev = &pdev->dev;
>  	/* create by_key and by_name subdirs of /sys/firmware/qemu_fw_cfg/ */
>  	err = -ENOMEM;
>  	fw_cfg_sel_ko = kobject_create_and_add("by_key", fw_cfg_top_ko);
> @@ -562,7 +681,7 @@ 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_read_blob(FW_CFG_ID, &fw_cfg_rev, 0, sizeof(fw_cfg_rev), false);
>  	fw_cfg_rev = le32_to_cpu(fw_cfg_rev);
>  	err = sysfs_create_file(fw_cfg_top_ko, &fw_cfg_rev_attr.attr);
>  	if (err)
> @@ -587,6 +706,7 @@ static int fw_cfg_sysfs_probe(struct platform_device *pdev)
>  err_name:
>  	fw_cfg_kobj_cleanup(fw_cfg_sel_ko);
>  err_sel:
> +	dev = NULL;
>  	return err;
>  }
>  
> -- 
> 2.15.0.125.g8f49766d64

Re: [Qemu-devel] [PATCH v6 3/5] fw_cfg: do DMA read operation

[Michael S. Tsirkin]

On Mon, Nov 13, 2017 at 08:29:56PM +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.
> 
> To avoid polling with unbound amount of time, the DMA operation is
> expected to complete within 200ms, or will return ETIME error.
> 
> We may want to switch all the *buf addresses to use only kmalloc'ed
> buffers (instead of using stack/image addresses with dma=false).
> 
> Signed-off-by: Marc-André Lureau <marcandre.lureau@redhat.com>
> Reviewed-by: Gabriel Somlo <somlo@cmu.edu>
> ---
>  drivers/firmware/qemu_fw_cfg.c | 154 ++++++++++++++++++++++++++++++++++++-----
>  1 file changed, 137 insertions(+), 17 deletions(-)
> 
> diff --git a/drivers/firmware/qemu_fw_cfg.c b/drivers/firmware/qemu_fw_cfg.c
> index 1f3e8545dab7..2ac4cd869fe6 100644
> --- a/drivers/firmware/qemu_fw_cfg.c
> +++ b/drivers/firmware/qemu_fw_cfg.c
> @@ -33,6 +33,8 @@
>  #include <linux/slab.h>
>  #include <linux/io.h>
>  #include <linux/ioport.h>
> +#include <linux/delay.h>
> +#include <linux/dma-mapping.h>
>  
>  MODULE_AUTHOR("Gabriel L. Somlo <somlo@cmu.edu>");
>  MODULE_DESCRIPTION("QEMU fw_cfg sysfs support");
> @@ -43,12 +45,26 @@ MODULE_LICENSE("GPL");
>  #define FW_CFG_ID         0x01
>  #define FW_CFG_FILE_DIR   0x19
>  
> +#define FW_CFG_VERSION_DMA     0x02
> +#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_TIMEOUT     200 /* ms */
> +
>  /* 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
>  
> +/* platform device for dma mapping */
> +static struct device *dev;
> +
> +/* fw_cfg revision attribute, in /sys/firmware/qemu_fw_cfg top-level dir. */
> +static u32 fw_cfg_rev;
> +
>  /* fw_cfg file directory entry type */
>  struct fw_cfg_file {
>  	u32 size;
> @@ -57,6 +73,12 @@ struct fw_cfg_file {
>  	char name[FW_CFG_MAX_FILE_PATH];
>  };
>  
> +struct fw_cfg_dma {
> +	u32 control;
> +	u32 length;
> +	u64 address;
> +} __packed;
> +
>  /* fw_cfg device i/o register addresses */
>  static bool fw_cfg_is_mmio;
>  static phys_addr_t fw_cfg_p_base;
> @@ -75,12 +97,93 @@ static inline u16 fw_cfg_sel_endianness(u16 key)
>  	return fw_cfg_is_mmio ? cpu_to_be16(key) : cpu_to_le16(key);
>  }
>  
> +static inline bool fw_cfg_dma_enabled(void)
> +{
> +	return fw_cfg_rev & FW_CFG_VERSION_DMA && fw_cfg_reg_dma;
> +}
> +
> +static bool fw_cfg_wait_for_control(struct fw_cfg_dma *d, unsigned long timeout)
> +{
> +	ktime_t start;
> +	ktime_t stop;
> +
> +	start = ktime_get();
> +	stop = ktime_add(start, ms_to_ktime(timeout));
> +
> +	do {
> +		if ((be32_to_cpu(d->control) & ~FW_CFG_DMA_CTL_ERROR) == 0)
> +			return true;
> +
> +		usleep_range(50, 100);

BTW it's not nice that this is uninterruptible. I think we need a
variant of usleep_range that is interruptible and call that here.
Thoughts?


> +	} while (ktime_before(ktime_get(), stop));
> +
> +	return false;
> +}
> +
> +static ssize_t fw_cfg_dma_transfer(void *address, u32 length, u32 control)
> +{
> +	dma_addr_t dma_addr = 0;
> +	struct fw_cfg_dma *d;
> +	dma_addr_t dma;
> +	ssize_t ret = length;
> +	enum dma_data_direction dir =
> +		(control & FW_CFG_DMA_CTL_READ ? DMA_FROM_DEVICE : 0);
> +
> +	if (address && length) {
> +		dma_addr = dma_map_single(dev, address, length, dir);
> +		if (dma_mapping_error(NULL, dma_addr)) {
> +			WARN(1, "%s: failed to map address\n", __func__);
> +			return -EFAULT;
> +		}
> +	}
> +
> +	d = kmalloc(sizeof(*d), GFP_KERNEL | GFP_DMA);
> +	if (!d) {
> +		ret = -ENOMEM;
> +		goto end;
> +	}
> +
> +	dma = dma_map_single(dev, d, sizeof(*d), DMA_BIDIRECTIONAL);
> +	if (dma_mapping_error(NULL, dma)) {
> +		WARN(1, "%s: failed to map fw_cfg_dma\n", __func__);
> +		ret = -EFAULT;
> +		goto end;
> +	}
> +
> +	*d = (struct fw_cfg_dma) {
> +		.address = cpu_to_be64(dma_addr),
> +		.length = cpu_to_be32(length),
> +		.control = cpu_to_be32(control)
> +	};
> +
> +	iowrite32be((u64)dma >> 32, fw_cfg_reg_dma);
> +	iowrite32be(dma, fw_cfg_reg_dma + 4);
> +
> +	if (!fw_cfg_wait_for_control(d, FW_CFG_DMA_TIMEOUT)) {
> +		WARN(1, "%s: timeout", __func__);
> +		ret = -ETIME;
> +	} else if (be32_to_cpu(d->control) & FW_CFG_DMA_CTL_ERROR) {
> +		ret = -EIO;
> +	}
> +
> +	dma_unmap_single(dev, dma, sizeof(*d), DMA_BIDIRECTIONAL);
> +
> +end:
> +	kfree(d);
> +	if (dma_addr)
> +		dma_unmap_single(dev, dma_addr, length, dir);

So if host was delayed what this does is corrupt guest memory.
IMHO things are already bad, let's at least keep it mapped
and allocated. Also what will heppen on next access attempt?
Maybe we need to prevent further attempts.
And maybe fw cfg needs a reset register so we can recover
faster.



> +
> +	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)
> +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:
> @@ -90,17 +193,36 @@ 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()) {
> +		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)
> +				goto end;
> +			ret = fw_cfg_dma_transfer(buf, count,
> +						  FW_CFG_DMA_CTL_READ);
> +		}
> +	} else {
> +		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);
> +	}
> +
> +end:
>  	mutex_unlock(&fw_cfg_dev_lock);
>  
>  	acpi_release_global_lock(glk);
> +
> +	return ret;
>  }
>  
>  /* clean up fw_cfg device i/o */
> @@ -192,7 +314,7 @@ 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);
> +	fw_cfg_read_blob(FW_CFG_SIGNATURE, sig, 0, FW_CFG_SIG_SIZE, false);
>  	if (memcmp(sig, "QEMU", FW_CFG_SIG_SIZE) != 0) {
>  		fw_cfg_io_cleanup();
>  		return -ENODEV;
> @@ -201,9 +323,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);
> @@ -351,8 +470,7 @@ static ssize_t fw_cfg_sysfs_read_raw(struct file *filp, struct kobject *kobj,
>  	if (count > entry->f.size - pos)
>  		count = entry->f.size - pos;
>  
> -	fw_cfg_read_blob(entry->f.select, buf, pos, count);
> -	return count;
> +	return fw_cfg_read_blob(entry->f.select, buf, pos, count, true);
>  }
>  
>  static struct bin_attribute fw_cfg_sysfs_attr_raw = {
> @@ -505,7 +623,7 @@ 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, &count, 0, sizeof(count));
> +	fw_cfg_read_blob(FW_CFG_FILE_DIR, &count, 0, sizeof(count), false);
>  	count = be32_to_cpu(count);
>  	dir_size = count * sizeof(struct fw_cfg_file);
>  
> @@ -513,7 +631,7 @@ static int fw_cfg_register_dir_entries(void)
>  	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(count), dir_size, true);
>  
>  	for (i = 0; i < count; i++) {
>  		dir[i].size = be32_to_cpu(dir[i].size);
> @@ -544,9 +662,10 @@ static int fw_cfg_sysfs_probe(struct platform_device *pdev)
>  	 * one fw_cfg device exist system-wide, so if one was already found
>  	 * earlier, we might as well stop here.
>  	 */
> -	if (fw_cfg_sel_ko)
> +	if (dev)
>  		return -EBUSY;
>  
> +	dev = &pdev->dev;
>  	/* create by_key and by_name subdirs of /sys/firmware/qemu_fw_cfg/ */
>  	err = -ENOMEM;
>  	fw_cfg_sel_ko = kobject_create_and_add("by_key", fw_cfg_top_ko);
> @@ -562,7 +681,7 @@ 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_read_blob(FW_CFG_ID, &fw_cfg_rev, 0, sizeof(fw_cfg_rev), false);
>  	fw_cfg_rev = le32_to_cpu(fw_cfg_rev);
>  	err = sysfs_create_file(fw_cfg_top_ko, &fw_cfg_rev_attr.attr);
>  	if (err)
> @@ -587,6 +706,7 @@ static int fw_cfg_sysfs_probe(struct platform_device *pdev)
>  err_name:
>  	fw_cfg_kobj_cleanup(fw_cfg_sel_ko);
>  err_sel:
> +	dev = NULL;
>  	return err;
>  }
>  
> -- 
> 2.15.0.125.g8f49766d64

Re: [PATCH v6 5/5] fw_cfg: write vmcoreinfo details

[Michael S. Tsirkin]

On Mon, Nov 13, 2017 at 08:29:58PM +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.
> 
> Signed-off-by: Marc-André Lureau <marcandre.lureau@redhat.com>
> Reviewed-by: Gabriel Somlo <somlo@cmu.edu>
> ---
>  drivers/firmware/qemu_fw_cfg.c | 87 +++++++++++++++++++++++++++++++++++++++++-
>  1 file changed, 86 insertions(+), 1 deletion(-)
> 
> diff --git a/drivers/firmware/qemu_fw_cfg.c b/drivers/firmware/qemu_fw_cfg.c
> index 2ac4cd869fe6..7a70e7a549f6 100644
> --- a/drivers/firmware/qemu_fw_cfg.c
> +++ b/drivers/firmware/qemu_fw_cfg.c
> @@ -35,6 +35,8 @@
>  #include <linux/ioport.h>
>  #include <linux/delay.h>
>  #include <linux/dma-mapping.h>
> +#include <linux/crash_core.h>
> +#include <linux/crash_dump.h>
>  
>  MODULE_AUTHOR("Gabriel L. Somlo <somlo@cmu.edu>");
>  MODULE_DESCRIPTION("QEMU fw_cfg sysfs support");
> @@ -59,6 +61,8 @@ MODULE_LICENSE("GPL");
>  /* fw_cfg "file name" is up to 56 characters (including terminating nul) */
>  #define FW_CFG_MAX_FILE_PATH 56
>  
> +#define VMCOREINFO_FORMAT_ELF 0x1
> +
>  /* platform device for dma mapping */
>  static struct device *dev;
>  
> @@ -127,7 +131,8 @@ static ssize_t fw_cfg_dma_transfer(void *address, u32 length, u32 control)
>  	dma_addr_t dma;
>  	ssize_t ret = length;
>  	enum dma_data_direction dir =
> -		(control & FW_CFG_DMA_CTL_READ ? DMA_FROM_DEVICE : 0);
> +		(control & FW_CFG_DMA_CTL_READ ? DMA_FROM_DEVICE : 0) |
> +		(control & FW_CFG_DMA_CTL_WRITE ? DMA_TO_DEVICE : 0);
>  
>  	if (address && length) {
>  		dma_addr = dma_map_single(dev, address, length, dir);
> @@ -225,6 +230,48 @@ static ssize_t fw_cfg_read_blob(u16 key,
>  	return ret;
>  }
>  
> +#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__);
> +		memset(buf, 0, count);
> +		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)
>  {
> @@ -343,6 +390,37 @@ struct fw_cfg_sysfs_entry {
>  	struct list_head list;
>  };
>  
> +#ifdef CONFIG_CRASH_CORE
> +static ssize_t write_vmcoreinfo(const struct fw_cfg_file *f)
> +{
> +	struct vmci {
> +		__le16 host_format;
> +		__le16 guest_format;
> +		__le32 size;
> +		__le64 paddr;
> +	} __packed;
> +	struct vmci *data;
> +	ssize_t ret;
> +
> +	data = kmalloc(sizeof(struct vmci), GFP_KERNEL | GFP_DMA);
> +	if (!data)
> +		return -ENOMEM;

It's a small bit of data - you can just keep it in a global variable,
this way failures won't be an issue.

> +
> +	/* spare ourself reading host format support for now since we
> +	 * don't know what else to format - host may ignore ours
> +	 */
> +	*data = (struct vmci) {
> +		.guest_format = cpu_to_le16(VMCOREINFO_FORMAT_ELF),
> +		.size = cpu_to_le32(VMCOREINFO_NOTE_SIZE),
> +		.paddr = cpu_to_le64(paddr_vmcoreinfo_note())
> +	};
> +	ret = fw_cfg_write_blob(f->select, data, 0, sizeof(struct vmci));
> +
> +	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)
>  {
> @@ -582,6 +660,13 @@ 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 (strcmp(f->name, "etc/vmcoreinfo") == 0 && !is_kdump_kernel()) {
> +		if (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.15.0.125.g8f49766d64

Re: [Qemu-devel] [PATCH v6 5/5] fw_cfg: write vmcoreinfo details

[Michael S. Tsirkin]

On Mon, Nov 13, 2017 at 08:29:58PM +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.
> 
> Signed-off-by: Marc-André Lureau <marcandre.lureau@redhat.com>
> Reviewed-by: Gabriel Somlo <somlo@cmu.edu>
> ---
>  drivers/firmware/qemu_fw_cfg.c | 87 +++++++++++++++++++++++++++++++++++++++++-
>  1 file changed, 86 insertions(+), 1 deletion(-)
> 
> diff --git a/drivers/firmware/qemu_fw_cfg.c b/drivers/firmware/qemu_fw_cfg.c
> index 2ac4cd869fe6..7a70e7a549f6 100644
> --- a/drivers/firmware/qemu_fw_cfg.c
> +++ b/drivers/firmware/qemu_fw_cfg.c
> @@ -35,6 +35,8 @@
>  #include <linux/ioport.h>
>  #include <linux/delay.h>
>  #include <linux/dma-mapping.h>
> +#include <linux/crash_core.h>
> +#include <linux/crash_dump.h>
>  
>  MODULE_AUTHOR("Gabriel L. Somlo <somlo@cmu.edu>");
>  MODULE_DESCRIPTION("QEMU fw_cfg sysfs support");
> @@ -59,6 +61,8 @@ MODULE_LICENSE("GPL");
>  /* fw_cfg "file name" is up to 56 characters (including terminating nul) */
>  #define FW_CFG_MAX_FILE_PATH 56
>  
> +#define VMCOREINFO_FORMAT_ELF 0x1
> +
>  /* platform device for dma mapping */
>  static struct device *dev;
>  
> @@ -127,7 +131,8 @@ static ssize_t fw_cfg_dma_transfer(void *address, u32 length, u32 control)
>  	dma_addr_t dma;
>  	ssize_t ret = length;
>  	enum dma_data_direction dir =
> -		(control & FW_CFG_DMA_CTL_READ ? DMA_FROM_DEVICE : 0);
> +		(control & FW_CFG_DMA_CTL_READ ? DMA_FROM_DEVICE : 0) |
> +		(control & FW_CFG_DMA_CTL_WRITE ? DMA_TO_DEVICE : 0);
>  
>  	if (address && length) {
>  		dma_addr = dma_map_single(dev, address, length, dir);
> @@ -225,6 +230,48 @@ static ssize_t fw_cfg_read_blob(u16 key,
>  	return ret;
>  }
>  
> +#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__);
> +		memset(buf, 0, count);
> +		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)
>  {
> @@ -343,6 +390,37 @@ struct fw_cfg_sysfs_entry {
>  	struct list_head list;
>  };
>  
> +#ifdef CONFIG_CRASH_CORE
> +static ssize_t write_vmcoreinfo(const struct fw_cfg_file *f)
> +{
> +	struct vmci {
> +		__le16 host_format;
> +		__le16 guest_format;
> +		__le32 size;
> +		__le64 paddr;
> +	} __packed;
> +	struct vmci *data;
> +	ssize_t ret;
> +
> +	data = kmalloc(sizeof(struct vmci), GFP_KERNEL | GFP_DMA);
> +	if (!data)
> +		return -ENOMEM;

It's a small bit of data - you can just keep it in a global variable,
this way failures won't be an issue.

> +
> +	/* spare ourself reading host format support for now since we
> +	 * don't know what else to format - host may ignore ours
> +	 */
> +	*data = (struct vmci) {
> +		.guest_format = cpu_to_le16(VMCOREINFO_FORMAT_ELF),
> +		.size = cpu_to_le32(VMCOREINFO_NOTE_SIZE),
> +		.paddr = cpu_to_le64(paddr_vmcoreinfo_note())
> +	};
> +	ret = fw_cfg_write_blob(f->select, data, 0, sizeof(struct vmci));
> +
> +	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)
>  {
> @@ -582,6 +660,13 @@ 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 (strcmp(f->name, "etc/vmcoreinfo") == 0 && !is_kdump_kernel()) {
> +		if (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.15.0.125.g8f49766d64

Re: [PATCH v6 3/5] fw_cfg: do DMA read operation

[Marc-André Lureau]

Hi

----- Original Message -----
> On Mon, Nov 13, 2017 at 08:29:56PM +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.
> > 
> > To avoid polling with unbound amount of time, the DMA operation is
> > expected to complete within 200ms, or will return ETIME error.
> > 
> > We may want to switch all the *buf addresses to use only kmalloc'ed
> > buffers (instead of using stack/image addresses with dma=false).
> > 
> > Signed-off-by: Marc-André Lureau <marcandre.lureau@redhat.com>
> > Reviewed-by: Gabriel Somlo <somlo@cmu.edu>
> > ---
> >  drivers/firmware/qemu_fw_cfg.c | 154
> >  ++++++++++++++++++++++++++++++++++++-----
> >  1 file changed, 137 insertions(+), 17 deletions(-)
> > 
> > diff --git a/drivers/firmware/qemu_fw_cfg.c
> > b/drivers/firmware/qemu_fw_cfg.c
> > index 1f3e8545dab7..2ac4cd869fe6 100644
> > --- a/drivers/firmware/qemu_fw_cfg.c
> > +++ b/drivers/firmware/qemu_fw_cfg.c
> > @@ -33,6 +33,8 @@
> >  #include <linux/slab.h>
> >  #include <linux/io.h>
> >  #include <linux/ioport.h>
> > +#include <linux/delay.h>
> > +#include <linux/dma-mapping.h>
> >  
> >  MODULE_AUTHOR("Gabriel L. Somlo <somlo@cmu.edu>");
> >  MODULE_DESCRIPTION("QEMU fw_cfg sysfs support");
> > @@ -43,12 +45,26 @@ MODULE_LICENSE("GPL");
> >  #define FW_CFG_ID         0x01
> >  #define FW_CFG_FILE_DIR   0x19
> >  
> > +#define FW_CFG_VERSION_DMA     0x02
> > +#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_TIMEOUT     200 /* ms */
> > +
> >  /* 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
> >  
> > +/* platform device for dma mapping */
> > +static struct device *dev;
> > +
> > +/* fw_cfg revision attribute, in /sys/firmware/qemu_fw_cfg top-level dir.
> > */
> > +static u32 fw_cfg_rev;
> > +
> >  /* fw_cfg file directory entry type */
> >  struct fw_cfg_file {
> >  	u32 size;
> > @@ -57,6 +73,12 @@ struct fw_cfg_file {
> >  	char name[FW_CFG_MAX_FILE_PATH];
> >  };
> >  
> > +struct fw_cfg_dma {
> > +	u32 control;
> > +	u32 length;
> > +	u64 address;
> > +} __packed;
> > +
> >  /* fw_cfg device i/o register addresses */
> >  static bool fw_cfg_is_mmio;
> >  static phys_addr_t fw_cfg_p_base;
> > @@ -75,12 +97,93 @@ static inline u16 fw_cfg_sel_endianness(u16 key)
> >  	return fw_cfg_is_mmio ? cpu_to_be16(key) : cpu_to_le16(key);
> >  }
> >  
> > +static inline bool fw_cfg_dma_enabled(void)
> > +{
> > +	return fw_cfg_rev & FW_CFG_VERSION_DMA && fw_cfg_reg_dma;
> > +}
> > +
> > +static bool fw_cfg_wait_for_control(struct fw_cfg_dma *d, unsigned long
> > timeout)
> > +{
> > +	ktime_t start;
> > +	ktime_t stop;
> > +
> > +	start = ktime_get();
> > +	stop = ktime_add(start, ms_to_ktime(timeout));
> > +
> > +	do {
> > +		if ((be32_to_cpu(d->control) & ~FW_CFG_DMA_CTL_ERROR) == 0)
> > +			return true;
> > +
> > +		usleep_range(50, 100);
> 
> BTW it's not nice that this is uninterruptible. I think we need a
> variant of usleep_range that is interruptible and call that here.
> Thoughts?
> 

This usleep_range() pattern is pretty common apparently.

(and it's probably better than calling yield() in a loop, like v1-3 did) 
> 
> > +	} while (ktime_before(ktime_get(), stop));
> > +
> > +	return false;
> > +}
> > +
> > +static ssize_t fw_cfg_dma_transfer(void *address, u32 length, u32 control)
> > +{
> > +	dma_addr_t dma_addr = 0;
> > +	struct fw_cfg_dma *d;
> > +	dma_addr_t dma;
> > +	ssize_t ret = length;
> > +	enum dma_data_direction dir =
> > +		(control & FW_CFG_DMA_CTL_READ ? DMA_FROM_DEVICE : 0);
> > +
> > +	if (address && length) {
> > +		dma_addr = dma_map_single(dev, address, length, dir);
> > +		if (dma_mapping_error(NULL, dma_addr)) {
> > +			WARN(1, "%s: failed to map address\n", __func__);
> > +			return -EFAULT;
> > +		}
> > +	}
> > +
> > +	d = kmalloc(sizeof(*d), GFP_KERNEL | GFP_DMA);
> > +	if (!d) {
> > +		ret = -ENOMEM;
> > +		goto end;
> > +	}
> > +
> > +	dma = dma_map_single(dev, d, sizeof(*d), DMA_BIDIRECTIONAL);
> > +	if (dma_mapping_error(NULL, dma)) {
> > +		WARN(1, "%s: failed to map fw_cfg_dma\n", __func__);
> > +		ret = -EFAULT;
> > +		goto end;
> > +	}
> > +
> > +	*d = (struct fw_cfg_dma) {
> > +		.address = cpu_to_be64(dma_addr),
> > +		.length = cpu_to_be32(length),
> > +		.control = cpu_to_be32(control)
> > +	};
> > +
> > +	iowrite32be((u64)dma >> 32, fw_cfg_reg_dma);
> > +	iowrite32be(dma, fw_cfg_reg_dma + 4);
> > +
> > +	if (!fw_cfg_wait_for_control(d, FW_CFG_DMA_TIMEOUT)) {
> > +		WARN(1, "%s: timeout", __func__);
> > +		ret = -ETIME;
> > +	} else if (be32_to_cpu(d->control) & FW_CFG_DMA_CTL_ERROR) {
> > +		ret = -EIO;
> > +	}
> > +
> > +	dma_unmap_single(dev, dma, sizeof(*d), DMA_BIDIRECTIONAL);
> > +
> > +end:
> > +	kfree(d);
> > +	if (dma_addr)
> > +		dma_unmap_single(dev, dma_addr, length, dir);
> 
> So if host was delayed what this does is corrupt guest memory.

By "delayed" you mean fw_cfg_wait_for_control() timed out?

Hmm, perhaps we should loop infinitely? that's also what the firmware does.

By the way, I failed to understand vcpu guest & qemu interaction here, shouldn't the vcpu thread be running the qemu dma_transfer until completion before returning to guest?

> IMHO things are already bad, let's at least keep it mapped
> and allocated. Also what will heppen on next access attempt?

> Maybe we need to prevent further attempts.
> And maybe fw cfg needs a reset register so we can recover
> faster.

Or perhaps since it's all software, it should never fail? ;) That's apparently the device design.

> 
> 
> > +
> > +	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)
> > +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:
> > @@ -90,17 +193,36 @@ 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()) {
> > +		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)
> > +				goto end;
> > +			ret = fw_cfg_dma_transfer(buf, count,
> > +						  FW_CFG_DMA_CTL_READ);
> > +		}
> > +	} else {
> > +		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);
> > +	}
> > +
> > +end:
> >  	mutex_unlock(&fw_cfg_dev_lock);
> >  
> >  	acpi_release_global_lock(glk);
> > +
> > +	return ret;
> >  }
> >  
> >  /* clean up fw_cfg device i/o */
> > @@ -192,7 +314,7 @@ 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);
> > +	fw_cfg_read_blob(FW_CFG_SIGNATURE, sig, 0, FW_CFG_SIG_SIZE, false);
> >  	if (memcmp(sig, "QEMU", FW_CFG_SIG_SIZE) != 0) {
> >  		fw_cfg_io_cleanup();
> >  		return -ENODEV;
> > @@ -201,9 +323,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);
> > @@ -351,8 +470,7 @@ static ssize_t fw_cfg_sysfs_read_raw(struct file *filp,
> > struct kobject *kobj,
> >  	if (count > entry->f.size - pos)
> >  		count = entry->f.size - pos;
> >  
> > -	fw_cfg_read_blob(entry->f.select, buf, pos, count);
> > -	return count;
> > +	return fw_cfg_read_blob(entry->f.select, buf, pos, count, true);
> >  }
> >  
> >  static struct bin_attribute fw_cfg_sysfs_attr_raw = {
> > @@ -505,7 +623,7 @@ 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, &count, 0, sizeof(count));
> > +	fw_cfg_read_blob(FW_CFG_FILE_DIR, &count, 0, sizeof(count), false);
> >  	count = be32_to_cpu(count);
> >  	dir_size = count * sizeof(struct fw_cfg_file);
> >  
> > @@ -513,7 +631,7 @@ static int fw_cfg_register_dir_entries(void)
> >  	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(count), dir_size, true);
> >  
> >  	for (i = 0; i < count; i++) {
> >  		dir[i].size = be32_to_cpu(dir[i].size);
> > @@ -544,9 +662,10 @@ static int fw_cfg_sysfs_probe(struct platform_device
> > *pdev)
> >  	 * one fw_cfg device exist system-wide, so if one was already found
> >  	 * earlier, we might as well stop here.
> >  	 */
> > -	if (fw_cfg_sel_ko)
> > +	if (dev)
> >  		return -EBUSY;
> >  
> > +	dev = &pdev->dev;
> >  	/* create by_key and by_name subdirs of /sys/firmware/qemu_fw_cfg/ */
> >  	err = -ENOMEM;
> >  	fw_cfg_sel_ko = kobject_create_and_add("by_key", fw_cfg_top_ko);
> > @@ -562,7 +681,7 @@ 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_read_blob(FW_CFG_ID, &fw_cfg_rev, 0, sizeof(fw_cfg_rev), false);
> >  	fw_cfg_rev = le32_to_cpu(fw_cfg_rev);
> >  	err = sysfs_create_file(fw_cfg_top_ko, &fw_cfg_rev_attr.attr);
> >  	if (err)
> > @@ -587,6 +706,7 @@ static int fw_cfg_sysfs_probe(struct platform_device
> > *pdev)
> >  err_name:
> >  	fw_cfg_kobj_cleanup(fw_cfg_sel_ko);
> >  err_sel:
> > +	dev = NULL;
> >  	return err;
> >  }
> >  
> > --
> > 2.15.0.125.g8f49766d64
> 

Re: [Qemu-devel] [PATCH v6 3/5] fw_cfg: do DMA read operation

[Marc-André Lureau]

Hi

----- Original Message -----
> On Mon, Nov 13, 2017 at 08:29:56PM +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.
> > 
> > To avoid polling with unbound amount of time, the DMA operation is
> > expected to complete within 200ms, or will return ETIME error.
> > 
> > We may want to switch all the *buf addresses to use only kmalloc'ed
> > buffers (instead of using stack/image addresses with dma=false).
> > 
> > Signed-off-by: Marc-André Lureau <marcandre.lureau@redhat.com>
> > Reviewed-by: Gabriel Somlo <somlo@cmu.edu>
> > ---
> >  drivers/firmware/qemu_fw_cfg.c | 154
> >  ++++++++++++++++++++++++++++++++++++-----
> >  1 file changed, 137 insertions(+), 17 deletions(-)
> > 
> > diff --git a/drivers/firmware/qemu_fw_cfg.c
> > b/drivers/firmware/qemu_fw_cfg.c
> > index 1f3e8545dab7..2ac4cd869fe6 100644
> > --- a/drivers/firmware/qemu_fw_cfg.c
> > +++ b/drivers/firmware/qemu_fw_cfg.c
> > @@ -33,6 +33,8 @@
> >  #include <linux/slab.h>
> >  #include <linux/io.h>
> >  #include <linux/ioport.h>
> > +#include <linux/delay.h>
> > +#include <linux/dma-mapping.h>
> >  
> >  MODULE_AUTHOR("Gabriel L. Somlo <somlo@cmu.edu>");
> >  MODULE_DESCRIPTION("QEMU fw_cfg sysfs support");
> > @@ -43,12 +45,26 @@ MODULE_LICENSE("GPL");
> >  #define FW_CFG_ID         0x01
> >  #define FW_CFG_FILE_DIR   0x19
> >  
> > +#define FW_CFG_VERSION_DMA     0x02
> > +#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_TIMEOUT     200 /* ms */
> > +
> >  /* 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
> >  
> > +/* platform device for dma mapping */
> > +static struct device *dev;
> > +
> > +/* fw_cfg revision attribute, in /sys/firmware/qemu_fw_cfg top-level dir.
> > */
> > +static u32 fw_cfg_rev;
> > +
> >  /* fw_cfg file directory entry type */
> >  struct fw_cfg_file {
> >  	u32 size;
> > @@ -57,6 +73,12 @@ struct fw_cfg_file {
> >  	char name[FW_CFG_MAX_FILE_PATH];
> >  };
> >  
> > +struct fw_cfg_dma {
> > +	u32 control;
> > +	u32 length;
> > +	u64 address;
> > +} __packed;
> > +
> >  /* fw_cfg device i/o register addresses */
> >  static bool fw_cfg_is_mmio;
> >  static phys_addr_t fw_cfg_p_base;
> > @@ -75,12 +97,93 @@ static inline u16 fw_cfg_sel_endianness(u16 key)
> >  	return fw_cfg_is_mmio ? cpu_to_be16(key) : cpu_to_le16(key);
> >  }
> >  
> > +static inline bool fw_cfg_dma_enabled(void)
> > +{
> > +	return fw_cfg_rev & FW_CFG_VERSION_DMA && fw_cfg_reg_dma;
> > +}
> > +
> > +static bool fw_cfg_wait_for_control(struct fw_cfg_dma *d, unsigned long
> > timeout)
> > +{
> > +	ktime_t start;
> > +	ktime_t stop;
> > +
> > +	start = ktime_get();
> > +	stop = ktime_add(start, ms_to_ktime(timeout));
> > +
> > +	do {
> > +		if ((be32_to_cpu(d->control) & ~FW_CFG_DMA_CTL_ERROR) == 0)
> > +			return true;
> > +
> > +		usleep_range(50, 100);
> 
> BTW it's not nice that this is uninterruptible. I think we need a
> variant of usleep_range that is interruptible and call that here.
> Thoughts?
> 

This usleep_range() pattern is pretty common apparently.

(and it's probably better than calling yield() in a loop, like v1-3 did) 
> 
> > +	} while (ktime_before(ktime_get(), stop));
> > +
> > +	return false;
> > +}
> > +
> > +static ssize_t fw_cfg_dma_transfer(void *address, u32 length, u32 control)
> > +{
> > +	dma_addr_t dma_addr = 0;
> > +	struct fw_cfg_dma *d;
> > +	dma_addr_t dma;
> > +	ssize_t ret = length;
> > +	enum dma_data_direction dir =
> > +		(control & FW_CFG_DMA_CTL_READ ? DMA_FROM_DEVICE : 0);
> > +
> > +	if (address && length) {
> > +		dma_addr = dma_map_single(dev, address, length, dir);
> > +		if (dma_mapping_error(NULL, dma_addr)) {
> > +			WARN(1, "%s: failed to map address\n", __func__);
> > +			return -EFAULT;
> > +		}
> > +	}
> > +
> > +	d = kmalloc(sizeof(*d), GFP_KERNEL | GFP_DMA);
> > +	if (!d) {
> > +		ret = -ENOMEM;
> > +		goto end;
> > +	}
> > +
> > +	dma = dma_map_single(dev, d, sizeof(*d), DMA_BIDIRECTIONAL);
> > +	if (dma_mapping_error(NULL, dma)) {
> > +		WARN(1, "%s: failed to map fw_cfg_dma\n", __func__);
> > +		ret = -EFAULT;
> > +		goto end;
> > +	}
> > +
> > +	*d = (struct fw_cfg_dma) {
> > +		.address = cpu_to_be64(dma_addr),
> > +		.length = cpu_to_be32(length),
> > +		.control = cpu_to_be32(control)
> > +	};
> > +
> > +	iowrite32be((u64)dma >> 32, fw_cfg_reg_dma);
> > +	iowrite32be(dma, fw_cfg_reg_dma + 4);
> > +
> > +	if (!fw_cfg_wait_for_control(d, FW_CFG_DMA_TIMEOUT)) {
> > +		WARN(1, "%s: timeout", __func__);
> > +		ret = -ETIME;
> > +	} else if (be32_to_cpu(d->control) & FW_CFG_DMA_CTL_ERROR) {
> > +		ret = -EIO;
> > +	}
> > +
> > +	dma_unmap_single(dev, dma, sizeof(*d), DMA_BIDIRECTIONAL);
> > +
> > +end:
> > +	kfree(d);
> > +	if (dma_addr)
> > +		dma_unmap_single(dev, dma_addr, length, dir);
> 
> So if host was delayed what this does is corrupt guest memory.

By "delayed" you mean fw_cfg_wait_for_control() timed out?

Hmm, perhaps we should loop infinitely? that's also what the firmware does.

By the way, I failed to understand vcpu guest & qemu interaction here, shouldn't the vcpu thread be running the qemu dma_transfer until completion before returning to guest?

> IMHO things are already bad, let's at least keep it mapped
> and allocated. Also what will heppen on next access attempt?

> Maybe we need to prevent further attempts.
> And maybe fw cfg needs a reset register so we can recover
> faster.

Or perhaps since it's all software, it should never fail? ;) That's apparently the device design.

> 
> 
> > +
> > +	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)
> > +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:
> > @@ -90,17 +193,36 @@ 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()) {
> > +		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)
> > +				goto end;
> > +			ret = fw_cfg_dma_transfer(buf, count,
> > +						  FW_CFG_DMA_CTL_READ);
> > +		}
> > +	} else {
> > +		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);
> > +	}
> > +
> > +end:
> >  	mutex_unlock(&fw_cfg_dev_lock);
> >  
> >  	acpi_release_global_lock(glk);
> > +
> > +	return ret;
> >  }
> >  
> >  /* clean up fw_cfg device i/o */
> > @@ -192,7 +314,7 @@ 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);
> > +	fw_cfg_read_blob(FW_CFG_SIGNATURE, sig, 0, FW_CFG_SIG_SIZE, false);
> >  	if (memcmp(sig, "QEMU", FW_CFG_SIG_SIZE) != 0) {
> >  		fw_cfg_io_cleanup();
> >  		return -ENODEV;
> > @@ -201,9 +323,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);
> > @@ -351,8 +470,7 @@ static ssize_t fw_cfg_sysfs_read_raw(struct file *filp,
> > struct kobject *kobj,
> >  	if (count > entry->f.size - pos)
> >  		count = entry->f.size - pos;
> >  
> > -	fw_cfg_read_blob(entry->f.select, buf, pos, count);
> > -	return count;
> > +	return fw_cfg_read_blob(entry->f.select, buf, pos, count, true);
> >  }
> >  
> >  static struct bin_attribute fw_cfg_sysfs_attr_raw = {
> > @@ -505,7 +623,7 @@ 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, &count, 0, sizeof(count));
> > +	fw_cfg_read_blob(FW_CFG_FILE_DIR, &count, 0, sizeof(count), false);
> >  	count = be32_to_cpu(count);
> >  	dir_size = count * sizeof(struct fw_cfg_file);
> >  
> > @@ -513,7 +631,7 @@ static int fw_cfg_register_dir_entries(void)
> >  	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(count), dir_size, true);
> >  
> >  	for (i = 0; i < count; i++) {
> >  		dir[i].size = be32_to_cpu(dir[i].size);
> > @@ -544,9 +662,10 @@ static int fw_cfg_sysfs_probe(struct platform_device
> > *pdev)
> >  	 * one fw_cfg device exist system-wide, so if one was already found
> >  	 * earlier, we might as well stop here.
> >  	 */
> > -	if (fw_cfg_sel_ko)
> > +	if (dev)
> >  		return -EBUSY;
> >  
> > +	dev = &pdev->dev;
> >  	/* create by_key and by_name subdirs of /sys/firmware/qemu_fw_cfg/ */
> >  	err = -ENOMEM;
> >  	fw_cfg_sel_ko = kobject_create_and_add("by_key", fw_cfg_top_ko);
> > @@ -562,7 +681,7 @@ 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_read_blob(FW_CFG_ID, &fw_cfg_rev, 0, sizeof(fw_cfg_rev), false);
> >  	fw_cfg_rev = le32_to_cpu(fw_cfg_rev);
> >  	err = sysfs_create_file(fw_cfg_top_ko, &fw_cfg_rev_attr.attr);
> >  	if (err)
> > @@ -587,6 +706,7 @@ static int fw_cfg_sysfs_probe(struct platform_device
> > *pdev)
> >  err_name:
> >  	fw_cfg_kobj_cleanup(fw_cfg_sel_ko);
> >  err_sel:
> > +	dev = NULL;
> >  	return err;
> >  }
> >  
> > --
> > 2.15.0.125.g8f49766d64
> 

Re: [PATCH v6 5/5] fw_cfg: write vmcoreinfo details

[Marc-André Lureau]

Hi

----- Original Message -----
> On Mon, Nov 13, 2017 at 08:29:58PM +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.
> > 
> > Signed-off-by: Marc-André Lureau <marcandre.lureau@redhat.com>
> > Reviewed-by: Gabriel Somlo <somlo@cmu.edu>
> > ---
> >  drivers/firmware/qemu_fw_cfg.c | 87
> >  +++++++++++++++++++++++++++++++++++++++++-
> >  1 file changed, 86 insertions(+), 1 deletion(-)
> > 
> > diff --git a/drivers/firmware/qemu_fw_cfg.c
> > b/drivers/firmware/qemu_fw_cfg.c
> > index 2ac4cd869fe6..7a70e7a549f6 100644
> > --- a/drivers/firmware/qemu_fw_cfg.c
> > +++ b/drivers/firmware/qemu_fw_cfg.c
> > @@ -35,6 +35,8 @@
> >  #include <linux/ioport.h>
> >  #include <linux/delay.h>
> >  #include <linux/dma-mapping.h>
> > +#include <linux/crash_core.h>
> > +#include <linux/crash_dump.h>
> >  
> >  MODULE_AUTHOR("Gabriel L. Somlo <somlo@cmu.edu>");
> >  MODULE_DESCRIPTION("QEMU fw_cfg sysfs support");
> > @@ -59,6 +61,8 @@ MODULE_LICENSE("GPL");
> >  /* fw_cfg "file name" is up to 56 characters (including terminating nul)
> >  */
> >  #define FW_CFG_MAX_FILE_PATH 56
> >  
> > +#define VMCOREINFO_FORMAT_ELF 0x1
> > +
> >  /* platform device for dma mapping */
> >  static struct device *dev;
> >  
> > @@ -127,7 +131,8 @@ static ssize_t fw_cfg_dma_transfer(void *address, u32
> > length, u32 control)
> >  	dma_addr_t dma;
> >  	ssize_t ret = length;
> >  	enum dma_data_direction dir =
> > -		(control & FW_CFG_DMA_CTL_READ ? DMA_FROM_DEVICE : 0);
> > +		(control & FW_CFG_DMA_CTL_READ ? DMA_FROM_DEVICE : 0) |
> > +		(control & FW_CFG_DMA_CTL_WRITE ? DMA_TO_DEVICE : 0);
> >  
> >  	if (address && length) {
> >  		dma_addr = dma_map_single(dev, address, length, dir);
> > @@ -225,6 +230,48 @@ static ssize_t fw_cfg_read_blob(u16 key,
> >  	return ret;
> >  }
> >  
> > +#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__);
> > +		memset(buf, 0, count);
> > +		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)
> >  {
> > @@ -343,6 +390,37 @@ struct fw_cfg_sysfs_entry {
> >  	struct list_head list;
> >  };
> >  
> > +#ifdef CONFIG_CRASH_CORE
> > +static ssize_t write_vmcoreinfo(const struct fw_cfg_file *f)
> > +{
> > +	struct vmci {
> > +		__le16 host_format;
> > +		__le16 guest_format;
> > +		__le32 size;
> > +		__le64 paddr;
> > +	} __packed;
> > +	struct vmci *data;
> > +	ssize_t ret;
> > +
> > +	data = kmalloc(sizeof(struct vmci), GFP_KERNEL | GFP_DMA);
> > +	if (!data)
> > +		return -ENOMEM;
> 
> It's a small bit of data - you can just keep it in a global variable,
> this way failures won't be an issue.

It would still need to be allocated with GFP_DMA. Since it's a one time thing, this is just moving the problem isn't it?

> 
> > +
> > +	/* spare ourself reading host format support for now since we
> > +	 * don't know what else to format - host may ignore ours
> > +	 */
> > +	*data = (struct vmci) {
> > +		.guest_format = cpu_to_le16(VMCOREINFO_FORMAT_ELF),
> > +		.size = cpu_to_le32(VMCOREINFO_NOTE_SIZE),
> > +		.paddr = cpu_to_le64(paddr_vmcoreinfo_note())
> > +	};
> > +	ret = fw_cfg_write_blob(f->select, data, 0, sizeof(struct vmci));
> > +
> > +	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)
> >  {
> > @@ -582,6 +660,13 @@ 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 (strcmp(f->name, "etc/vmcoreinfo") == 0 && !is_kdump_kernel()) {
> > +		if (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.15.0.125.g8f49766d64
> 

Re: [Qemu-devel] [PATCH v6 5/5] fw_cfg: write vmcoreinfo details

[Marc-André Lureau]

Hi

----- Original Message -----
> On Mon, Nov 13, 2017 at 08:29:58PM +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.
> > 
> > Signed-off-by: Marc-André Lureau <marcandre.lureau@redhat.com>
> > Reviewed-by: Gabriel Somlo <somlo@cmu.edu>
> > ---
> >  drivers/firmware/qemu_fw_cfg.c | 87
> >  +++++++++++++++++++++++++++++++++++++++++-
> >  1 file changed, 86 insertions(+), 1 deletion(-)
> > 
> > diff --git a/drivers/firmware/qemu_fw_cfg.c
> > b/drivers/firmware/qemu_fw_cfg.c
> > index 2ac4cd869fe6..7a70e7a549f6 100644
> > --- a/drivers/firmware/qemu_fw_cfg.c
> > +++ b/drivers/firmware/qemu_fw_cfg.c
> > @@ -35,6 +35,8 @@
> >  #include <linux/ioport.h>
> >  #include <linux/delay.h>
> >  #include <linux/dma-mapping.h>
> > +#include <linux/crash_core.h>
> > +#include <linux/crash_dump.h>
> >  
> >  MODULE_AUTHOR("Gabriel L. Somlo <somlo@cmu.edu>");
> >  MODULE_DESCRIPTION("QEMU fw_cfg sysfs support");
> > @@ -59,6 +61,8 @@ MODULE_LICENSE("GPL");
> >  /* fw_cfg "file name" is up to 56 characters (including terminating nul)
> >  */
> >  #define FW_CFG_MAX_FILE_PATH 56
> >  
> > +#define VMCOREINFO_FORMAT_ELF 0x1
> > +
> >  /* platform device for dma mapping */
> >  static struct device *dev;
> >  
> > @@ -127,7 +131,8 @@ static ssize_t fw_cfg_dma_transfer(void *address, u32
> > length, u32 control)
> >  	dma_addr_t dma;
> >  	ssize_t ret = length;
> >  	enum dma_data_direction dir =
> > -		(control & FW_CFG_DMA_CTL_READ ? DMA_FROM_DEVICE : 0);
> > +		(control & FW_CFG_DMA_CTL_READ ? DMA_FROM_DEVICE : 0) |
> > +		(control & FW_CFG_DMA_CTL_WRITE ? DMA_TO_DEVICE : 0);
> >  
> >  	if (address && length) {
> >  		dma_addr = dma_map_single(dev, address, length, dir);
> > @@ -225,6 +230,48 @@ static ssize_t fw_cfg_read_blob(u16 key,
> >  	return ret;
> >  }
> >  
> > +#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__);
> > +		memset(buf, 0, count);
> > +		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)
> >  {
> > @@ -343,6 +390,37 @@ struct fw_cfg_sysfs_entry {
> >  	struct list_head list;
> >  };
> >  
> > +#ifdef CONFIG_CRASH_CORE
> > +static ssize_t write_vmcoreinfo(const struct fw_cfg_file *f)
> > +{
> > +	struct vmci {
> > +		__le16 host_format;
> > +		__le16 guest_format;
> > +		__le32 size;
> > +		__le64 paddr;
> > +	} __packed;
> > +	struct vmci *data;
> > +	ssize_t ret;
> > +
> > +	data = kmalloc(sizeof(struct vmci), GFP_KERNEL | GFP_DMA);
> > +	if (!data)
> > +		return -ENOMEM;
> 
> It's a small bit of data - you can just keep it in a global variable,
> this way failures won't be an issue.

It would still need to be allocated with GFP_DMA. Since it's a one time thing, this is just moving the problem isn't it?

> 
> > +
> > +	/* spare ourself reading host format support for now since we
> > +	 * don't know what else to format - host may ignore ours
> > +	 */
> > +	*data = (struct vmci) {
> > +		.guest_format = cpu_to_le16(VMCOREINFO_FORMAT_ELF),
> > +		.size = cpu_to_le32(VMCOREINFO_NOTE_SIZE),
> > +		.paddr = cpu_to_le64(paddr_vmcoreinfo_note())
> > +	};
> > +	ret = fw_cfg_write_blob(f->select, data, 0, sizeof(struct vmci));
> > +
> > +	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)
> >  {
> > @@ -582,6 +660,13 @@ 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 (strcmp(f->name, "etc/vmcoreinfo") == 0 && !is_kdump_kernel()) {
> > +		if (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.15.0.125.g8f49766d64
> 

Re: [PATCH v6 5/5] fw_cfg: write vmcoreinfo details

[Michael S. Tsirkin]

On Thu, Nov 16, 2017 at 08:34:23AM -0500, Marc-André Lureau wrote:
> Hi
> 
> ----- Original Message -----
> > On Mon, Nov 13, 2017 at 08:29:58PM +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.
> > > 
> > > Signed-off-by: Marc-André Lureau <marcandre.lureau@redhat.com>
> > > Reviewed-by: Gabriel Somlo <somlo@cmu.edu>
> > > ---
> > >  drivers/firmware/qemu_fw_cfg.c | 87
> > >  +++++++++++++++++++++++++++++++++++++++++-
> > >  1 file changed, 86 insertions(+), 1 deletion(-)
> > > 
> > > diff --git a/drivers/firmware/qemu_fw_cfg.c
> > > b/drivers/firmware/qemu_fw_cfg.c
> > > index 2ac4cd869fe6..7a70e7a549f6 100644
> > > --- a/drivers/firmware/qemu_fw_cfg.c
> > > +++ b/drivers/firmware/qemu_fw_cfg.c
> > > @@ -35,6 +35,8 @@
> > >  #include <linux/ioport.h>
> > >  #include <linux/delay.h>
> > >  #include <linux/dma-mapping.h>
> > > +#include <linux/crash_core.h>
> > > +#include <linux/crash_dump.h>
> > >  
> > >  MODULE_AUTHOR("Gabriel L. Somlo <somlo@cmu.edu>");
> > >  MODULE_DESCRIPTION("QEMU fw_cfg sysfs support");
> > > @@ -59,6 +61,8 @@ MODULE_LICENSE("GPL");
> > >  /* fw_cfg "file name" is up to 56 characters (including terminating nul)
> > >  */
> > >  #define FW_CFG_MAX_FILE_PATH 56
> > >  
> > > +#define VMCOREINFO_FORMAT_ELF 0x1
> > > +
> > >  /* platform device for dma mapping */
> > >  static struct device *dev;
> > >  
> > > @@ -127,7 +131,8 @@ static ssize_t fw_cfg_dma_transfer(void *address, u32
> > > length, u32 control)
> > >  	dma_addr_t dma;
> > >  	ssize_t ret = length;
> > >  	enum dma_data_direction dir =
> > > -		(control & FW_CFG_DMA_CTL_READ ? DMA_FROM_DEVICE : 0);
> > > +		(control & FW_CFG_DMA_CTL_READ ? DMA_FROM_DEVICE : 0) |
> > > +		(control & FW_CFG_DMA_CTL_WRITE ? DMA_TO_DEVICE : 0);
> > >  
> > >  	if (address && length) {
> > >  		dma_addr = dma_map_single(dev, address, length, dir);
> > > @@ -225,6 +230,48 @@ static ssize_t fw_cfg_read_blob(u16 key,
> > >  	return ret;
> > >  }
> > >  
> > > +#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__);
> > > +		memset(buf, 0, count);
> > > +		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)
> > >  {
> > > @@ -343,6 +390,37 @@ struct fw_cfg_sysfs_entry {
> > >  	struct list_head list;
> > >  };
> > >  
> > > +#ifdef CONFIG_CRASH_CORE
> > > +static ssize_t write_vmcoreinfo(const struct fw_cfg_file *f)
> > > +{
> > > +	struct vmci {
> > > +		__le16 host_format;
> > > +		__le16 guest_format;
> > > +		__le32 size;
> > > +		__le64 paddr;
> > > +	} __packed;
> > > +	struct vmci *data;
> > > +	ssize_t ret;
> > > +
> > > +	data = kmalloc(sizeof(struct vmci), GFP_KERNEL | GFP_DMA);
> > > +	if (!data)
> > > +		return -ENOMEM;
> > 
> > It's a small bit of data - you can just keep it in a global variable,
> > this way failures won't be an issue.
> 
> It would still need to be allocated

No - you can just make it a global variable.

> with GFP_DMA.

Are you sure?

 * GFP_DMA exists for historical reasons and should be avoided where possible.
 *   The flags indicates that the caller requires that the lowest zone be
 *   used (ZONE_DMA or 16M on x86-64). Ideally, this would be removed but
 *   it would require careful auditing as some users really require it and
 *   others use the flag to avoid lowmem reserves in ZONE_DMA and treat the
 *   lowest zone as a type of emergency reserve.


> Since it's a one time thing, this is just moving the problem isn't it?

Isn't the timeout just moving the problem too?
Avoiding memory corruption is a priority.

> > 
> > > +
> > > +	/* spare ourself reading host format support for now since we
> > > +	 * don't know what else to format - host may ignore ours
> > > +	 */
> > > +	*data = (struct vmci) {
> > > +		.guest_format = cpu_to_le16(VMCOREINFO_FORMAT_ELF),
> > > +		.size = cpu_to_le32(VMCOREINFO_NOTE_SIZE),
> > > +		.paddr = cpu_to_le64(paddr_vmcoreinfo_note())
> > > +	};
> > > +	ret = fw_cfg_write_blob(f->select, data, 0, sizeof(struct vmci));
> > > +
> > > +	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)
> > >  {
> > > @@ -582,6 +660,13 @@ 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 (strcmp(f->name, "etc/vmcoreinfo") == 0 && !is_kdump_kernel()) {
> > > +		if (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.15.0.125.g8f49766d64
> > 

Re: [Qemu-devel] [PATCH v6 5/5] fw_cfg: write vmcoreinfo details

[Michael S. Tsirkin]

On Thu, Nov 16, 2017 at 08:34:23AM -0500, Marc-André Lureau wrote:
> Hi
> 
> ----- Original Message -----
> > On Mon, Nov 13, 2017 at 08:29:58PM +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.
> > > 
> > > Signed-off-by: Marc-André Lureau <marcandre.lureau@redhat.com>
> > > Reviewed-by: Gabriel Somlo <somlo@cmu.edu>
> > > ---
> > >  drivers/firmware/qemu_fw_cfg.c | 87
> > >  +++++++++++++++++++++++++++++++++++++++++-
> > >  1 file changed, 86 insertions(+), 1 deletion(-)
> > > 
> > > diff --git a/drivers/firmware/qemu_fw_cfg.c
> > > b/drivers/firmware/qemu_fw_cfg.c
> > > index 2ac4cd869fe6..7a70e7a549f6 100644
> > > --- a/drivers/firmware/qemu_fw_cfg.c
> > > +++ b/drivers/firmware/qemu_fw_cfg.c
> > > @@ -35,6 +35,8 @@
> > >  #include <linux/ioport.h>
> > >  #include <linux/delay.h>
> > >  #include <linux/dma-mapping.h>
> > > +#include <linux/crash_core.h>
> > > +#include <linux/crash_dump.h>
> > >  
> > >  MODULE_AUTHOR("Gabriel L. Somlo <somlo@cmu.edu>");
> > >  MODULE_DESCRIPTION("QEMU fw_cfg sysfs support");
> > > @@ -59,6 +61,8 @@ MODULE_LICENSE("GPL");
> > >  /* fw_cfg "file name" is up to 56 characters (including terminating nul)
> > >  */
> > >  #define FW_CFG_MAX_FILE_PATH 56
> > >  
> > > +#define VMCOREINFO_FORMAT_ELF 0x1
> > > +
> > >  /* platform device for dma mapping */
> > >  static struct device *dev;
> > >  
> > > @@ -127,7 +131,8 @@ static ssize_t fw_cfg_dma_transfer(void *address, u32
> > > length, u32 control)
> > >  	dma_addr_t dma;
> > >  	ssize_t ret = length;
> > >  	enum dma_data_direction dir =
> > > -		(control & FW_CFG_DMA_CTL_READ ? DMA_FROM_DEVICE : 0);
> > > +		(control & FW_CFG_DMA_CTL_READ ? DMA_FROM_DEVICE : 0) |
> > > +		(control & FW_CFG_DMA_CTL_WRITE ? DMA_TO_DEVICE : 0);
> > >  
> > >  	if (address && length) {
> > >  		dma_addr = dma_map_single(dev, address, length, dir);
> > > @@ -225,6 +230,48 @@ static ssize_t fw_cfg_read_blob(u16 key,
> > >  	return ret;
> > >  }
> > >  
> > > +#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__);
> > > +		memset(buf, 0, count);
> > > +		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)
> > >  {
> > > @@ -343,6 +390,37 @@ struct fw_cfg_sysfs_entry {
> > >  	struct list_head list;
> > >  };
> > >  
> > > +#ifdef CONFIG_CRASH_CORE
> > > +static ssize_t write_vmcoreinfo(const struct fw_cfg_file *f)
> > > +{
> > > +	struct vmci {
> > > +		__le16 host_format;
> > > +		__le16 guest_format;
> > > +		__le32 size;
> > > +		__le64 paddr;
> > > +	} __packed;
> > > +	struct vmci *data;
> > > +	ssize_t ret;
> > > +
> > > +	data = kmalloc(sizeof(struct vmci), GFP_KERNEL | GFP_DMA);
> > > +	if (!data)
> > > +		return -ENOMEM;
> > 
> > It's a small bit of data - you can just keep it in a global variable,
> > this way failures won't be an issue.
> 
> It would still need to be allocated

No - you can just make it a global variable.

> with GFP_DMA.

Are you sure?

 * GFP_DMA exists for historical reasons and should be avoided where possible.
 *   The flags indicates that the caller requires that the lowest zone be
 *   used (ZONE_DMA or 16M on x86-64). Ideally, this would be removed but
 *   it would require careful auditing as some users really require it and
 *   others use the flag to avoid lowmem reserves in ZONE_DMA and treat the
 *   lowest zone as a type of emergency reserve.


> Since it's a one time thing, this is just moving the problem isn't it?

Isn't the timeout just moving the problem too?
Avoiding memory corruption is a priority.

> > 
> > > +
> > > +	/* spare ourself reading host format support for now since we
> > > +	 * don't know what else to format - host may ignore ours
> > > +	 */
> > > +	*data = (struct vmci) {
> > > +		.guest_format = cpu_to_le16(VMCOREINFO_FORMAT_ELF),
> > > +		.size = cpu_to_le32(VMCOREINFO_NOTE_SIZE),
> > > +		.paddr = cpu_to_le64(paddr_vmcoreinfo_note())
> > > +	};
> > > +	ret = fw_cfg_write_blob(f->select, data, 0, sizeof(struct vmci));
> > > +
> > > +	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)
> > >  {
> > > @@ -582,6 +660,13 @@ 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 (strcmp(f->name, "etc/vmcoreinfo") == 0 && !is_kdump_kernel()) {
> > > +		if (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.15.0.125.g8f49766d64
> > 

Re: [PATCH v6 3/5] fw_cfg: do DMA read operation

[Michael S. Tsirkin]

On Thu, Nov 16, 2017 at 08:17:12AM -0500, Marc-André Lureau wrote:
> Hi
> 
> ----- Original Message -----
> > On Mon, Nov 13, 2017 at 08:29:56PM +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.
> > > 
> > > To avoid polling with unbound amount of time, the DMA operation is
> > > expected to complete within 200ms, or will return ETIME error.
> > > 
> > > We may want to switch all the *buf addresses to use only kmalloc'ed
> > > buffers (instead of using stack/image addresses with dma=false).
> > > 
> > > Signed-off-by: Marc-André Lureau <marcandre.lureau@redhat.com>
> > > Reviewed-by: Gabriel Somlo <somlo@cmu.edu>
> > > ---
> > >  drivers/firmware/qemu_fw_cfg.c | 154
> > >  ++++++++++++++++++++++++++++++++++++-----
> > >  1 file changed, 137 insertions(+), 17 deletions(-)
> > > 
> > > diff --git a/drivers/firmware/qemu_fw_cfg.c
> > > b/drivers/firmware/qemu_fw_cfg.c
> > > index 1f3e8545dab7..2ac4cd869fe6 100644
> > > --- a/drivers/firmware/qemu_fw_cfg.c
> > > +++ b/drivers/firmware/qemu_fw_cfg.c
> > > @@ -33,6 +33,8 @@
> > >  #include <linux/slab.h>
> > >  #include <linux/io.h>
> > >  #include <linux/ioport.h>
> > > +#include <linux/delay.h>
> > > +#include <linux/dma-mapping.h>
> > >  
> > >  MODULE_AUTHOR("Gabriel L. Somlo <somlo@cmu.edu>");
> > >  MODULE_DESCRIPTION("QEMU fw_cfg sysfs support");
> > > @@ -43,12 +45,26 @@ MODULE_LICENSE("GPL");
> > >  #define FW_CFG_ID         0x01
> > >  #define FW_CFG_FILE_DIR   0x19
> > >  
> > > +#define FW_CFG_VERSION_DMA     0x02
> > > +#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_TIMEOUT     200 /* ms */
> > > +
> > >  /* 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
> > >  
> > > +/* platform device for dma mapping */
> > > +static struct device *dev;
> > > +
> > > +/* fw_cfg revision attribute, in /sys/firmware/qemu_fw_cfg top-level dir.
> > > */
> > > +static u32 fw_cfg_rev;
> > > +
> > >  /* fw_cfg file directory entry type */
> > >  struct fw_cfg_file {
> > >  	u32 size;
> > > @@ -57,6 +73,12 @@ struct fw_cfg_file {
> > >  	char name[FW_CFG_MAX_FILE_PATH];
> > >  };
> > >  
> > > +struct fw_cfg_dma {
> > > +	u32 control;
> > > +	u32 length;
> > > +	u64 address;
> > > +} __packed;
> > > +
> > >  /* fw_cfg device i/o register addresses */
> > >  static bool fw_cfg_is_mmio;
> > >  static phys_addr_t fw_cfg_p_base;
> > > @@ -75,12 +97,93 @@ static inline u16 fw_cfg_sel_endianness(u16 key)
> > >  	return fw_cfg_is_mmio ? cpu_to_be16(key) : cpu_to_le16(key);
> > >  }
> > >  
> > > +static inline bool fw_cfg_dma_enabled(void)
> > > +{
> > > +	return fw_cfg_rev & FW_CFG_VERSION_DMA && fw_cfg_reg_dma;
> > > +}
> > > +
> > > +static bool fw_cfg_wait_for_control(struct fw_cfg_dma *d, unsigned long
> > > timeout)
> > > +{
> > > +	ktime_t start;
> > > +	ktime_t stop;
> > > +
> > > +	start = ktime_get();
> > > +	stop = ktime_add(start, ms_to_ktime(timeout));
> > > +
> > > +	do {
> > > +		if ((be32_to_cpu(d->control) & ~FW_CFG_DMA_CTL_ERROR) == 0)
> > > +			return true;
> > > +
> > > +		usleep_range(50, 100);
> > 
> > BTW it's not nice that this is uninterruptible. I think we need a
> > variant of usleep_range that is interruptible and call that here.
> > Thoughts?
> > 
> 
> This usleep_range() pattern is pretty common apparently.

In kernel - right, but doing it from userspace context means you can't
kill userspace until timeout expires.

> 
> (and it's probably better than calling yield() in a loop, like v1-3 did) 
> > 
> > > +	} while (ktime_before(ktime_get(), stop));
> > > +
> > > +	return false;
> > > +}
> > > +
> > > +static ssize_t fw_cfg_dma_transfer(void *address, u32 length, u32 control)
> > > +{
> > > +	dma_addr_t dma_addr = 0;
> > > +	struct fw_cfg_dma *d;
> > > +	dma_addr_t dma;
> > > +	ssize_t ret = length;
> > > +	enum dma_data_direction dir =
> > > +		(control & FW_CFG_DMA_CTL_READ ? DMA_FROM_DEVICE : 0);
> > > +
> > > +	if (address && length) {
> > > +		dma_addr = dma_map_single(dev, address, length, dir);
> > > +		if (dma_mapping_error(NULL, dma_addr)) {
> > > +			WARN(1, "%s: failed to map address\n", __func__);
> > > +			return -EFAULT;
> > > +		}
> > > +	}
> > > +
> > > +	d = kmalloc(sizeof(*d), GFP_KERNEL | GFP_DMA);
> > > +	if (!d) {
> > > +		ret = -ENOMEM;
> > > +		goto end;
> > > +	}
> > > +
> > > +	dma = dma_map_single(dev, d, sizeof(*d), DMA_BIDIRECTIONAL);
> > > +	if (dma_mapping_error(NULL, dma)) {
> > > +		WARN(1, "%s: failed to map fw_cfg_dma\n", __func__);
> > > +		ret = -EFAULT;
> > > +		goto end;
> > > +	}
> > > +
> > > +	*d = (struct fw_cfg_dma) {
> > > +		.address = cpu_to_be64(dma_addr),
> > > +		.length = cpu_to_be32(length),
> > > +		.control = cpu_to_be32(control)
> > > +	};
> > > +
> > > +	iowrite32be((u64)dma >> 32, fw_cfg_reg_dma);
> > > +	iowrite32be(dma, fw_cfg_reg_dma + 4);
> > > +
> > > +	if (!fw_cfg_wait_for_control(d, FW_CFG_DMA_TIMEOUT)) {
> > > +		WARN(1, "%s: timeout", __func__);
> > > +		ret = -ETIME;
> > > +	} else if (be32_to_cpu(d->control) & FW_CFG_DMA_CTL_ERROR) {
> > > +		ret = -EIO;
> > > +	}
> > > +
> > > +	dma_unmap_single(dev, dma, sizeof(*d), DMA_BIDIRECTIONAL);
> > > +
> > > +end:
> > > +	kfree(d);
> > > +	if (dma_addr)
> > > +		dma_unmap_single(dev, dma_addr, length, dir);
> > 
> > So if host was delayed what this does is corrupt guest memory.
> 
> By "delayed" you mean fw_cfg_wait_for_control() timed out?
> 
> Hmm, perhaps we should loop infinitely? that's also what the firmware does.
> 
> By the way, I failed to understand vcpu guest & qemu interaction here, shouldn't the vcpu thread be running the qemu dma_transfer until completion before returning to guest?
> 
> > IMHO things are already bad, let's at least keep it mapped
> > and allocated. Also what will heppen on next access attempt?
> 
> > Maybe we need to prevent further attempts.
> > And maybe fw cfg needs a reset register so we can recover
> > faster.
> 
> Or perhaps since it's all software, it should never fail? ;) That's apparently the device design.

As far as I can see, that is true. IOW the time-out is not needed,
if it's not completed on the first attempt, you can fail
immediately.


But a bigger issue is it seems to trigger failures for people.

> > 
> > 
> > > +
> > > +	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)
> > > +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:
> > > @@ -90,17 +193,36 @@ 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()) {
> > > +		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)
> > > +				goto end;
> > > +			ret = fw_cfg_dma_transfer(buf, count,
> > > +						  FW_CFG_DMA_CTL_READ);
> > > +		}
> > > +	} else {
> > > +		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);
> > > +	}
> > > +
> > > +end:
> > >  	mutex_unlock(&fw_cfg_dev_lock);
> > >  
> > >  	acpi_release_global_lock(glk);
> > > +
> > > +	return ret;
> > >  }
> > >  
> > >  /* clean up fw_cfg device i/o */
> > > @@ -192,7 +314,7 @@ 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);
> > > +	fw_cfg_read_blob(FW_CFG_SIGNATURE, sig, 0, FW_CFG_SIG_SIZE, false);
> > >  	if (memcmp(sig, "QEMU", FW_CFG_SIG_SIZE) != 0) {
> > >  		fw_cfg_io_cleanup();
> > >  		return -ENODEV;
> > > @@ -201,9 +323,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);
> > > @@ -351,8 +470,7 @@ static ssize_t fw_cfg_sysfs_read_raw(struct file *filp,
> > > struct kobject *kobj,
> > >  	if (count > entry->f.size - pos)
> > >  		count = entry->f.size - pos;
> > >  
> > > -	fw_cfg_read_blob(entry->f.select, buf, pos, count);
> > > -	return count;
> > > +	return fw_cfg_read_blob(entry->f.select, buf, pos, count, true);
> > >  }
> > >  
> > >  static struct bin_attribute fw_cfg_sysfs_attr_raw = {
> > > @@ -505,7 +623,7 @@ 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, &count, 0, sizeof(count));
> > > +	fw_cfg_read_blob(FW_CFG_FILE_DIR, &count, 0, sizeof(count), false);
> > >  	count = be32_to_cpu(count);
> > >  	dir_size = count * sizeof(struct fw_cfg_file);
> > >  
> > > @@ -513,7 +631,7 @@ static int fw_cfg_register_dir_entries(void)
> > >  	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(count), dir_size, true);
> > >  
> > >  	for (i = 0; i < count; i++) {
> > >  		dir[i].size = be32_to_cpu(dir[i].size);
> > > @@ -544,9 +662,10 @@ static int fw_cfg_sysfs_probe(struct platform_device
> > > *pdev)
> > >  	 * one fw_cfg device exist system-wide, so if one was already found
> > >  	 * earlier, we might as well stop here.
> > >  	 */
> > > -	if (fw_cfg_sel_ko)
> > > +	if (dev)
> > >  		return -EBUSY;
> > >  
> > > +	dev = &pdev->dev;
> > >  	/* create by_key and by_name subdirs of /sys/firmware/qemu_fw_cfg/ */
> > >  	err = -ENOMEM;
> > >  	fw_cfg_sel_ko = kobject_create_and_add("by_key", fw_cfg_top_ko);
> > > @@ -562,7 +681,7 @@ 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_read_blob(FW_CFG_ID, &fw_cfg_rev, 0, sizeof(fw_cfg_rev), false);
> > >  	fw_cfg_rev = le32_to_cpu(fw_cfg_rev);
> > >  	err = sysfs_create_file(fw_cfg_top_ko, &fw_cfg_rev_attr.attr);
> > >  	if (err)
> > > @@ -587,6 +706,7 @@ static int fw_cfg_sysfs_probe(struct platform_device
> > > *pdev)
> > >  err_name:
> > >  	fw_cfg_kobj_cleanup(fw_cfg_sel_ko);
> > >  err_sel:
> > > +	dev = NULL;
> > >  	return err;
> > >  }
> > >  
> > > --
> > > 2.15.0.125.g8f49766d64
> > 

Re: [Qemu-devel] [PATCH v6 3/5] fw_cfg: do DMA read operation

[Michael S. Tsirkin]

On Thu, Nov 16, 2017 at 08:17:12AM -0500, Marc-André Lureau wrote:
> Hi
> 
> ----- Original Message -----
> > On Mon, Nov 13, 2017 at 08:29:56PM +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.
> > > 
> > > To avoid polling with unbound amount of time, the DMA operation is
> > > expected to complete within 200ms, or will return ETIME error.
> > > 
> > > We may want to switch all the *buf addresses to use only kmalloc'ed
> > > buffers (instead of using stack/image addresses with dma=false).
> > > 
> > > Signed-off-by: Marc-André Lureau <marcandre.lureau@redhat.com>
> > > Reviewed-by: Gabriel Somlo <somlo@cmu.edu>
> > > ---
> > >  drivers/firmware/qemu_fw_cfg.c | 154
> > >  ++++++++++++++++++++++++++++++++++++-----
> > >  1 file changed, 137 insertions(+), 17 deletions(-)
> > > 
> > > diff --git a/drivers/firmware/qemu_fw_cfg.c
> > > b/drivers/firmware/qemu_fw_cfg.c
> > > index 1f3e8545dab7..2ac4cd869fe6 100644
> > > --- a/drivers/firmware/qemu_fw_cfg.c
> > > +++ b/drivers/firmware/qemu_fw_cfg.c
> > > @@ -33,6 +33,8 @@
> > >  #include <linux/slab.h>
> > >  #include <linux/io.h>
> > >  #include <linux/ioport.h>
> > > +#include <linux/delay.h>
> > > +#include <linux/dma-mapping.h>
> > >  
> > >  MODULE_AUTHOR("Gabriel L. Somlo <somlo@cmu.edu>");
> > >  MODULE_DESCRIPTION("QEMU fw_cfg sysfs support");
> > > @@ -43,12 +45,26 @@ MODULE_LICENSE("GPL");
> > >  #define FW_CFG_ID         0x01
> > >  #define FW_CFG_FILE_DIR   0x19
> > >  
> > > +#define FW_CFG_VERSION_DMA     0x02
> > > +#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_TIMEOUT     200 /* ms */
> > > +
> > >  /* 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
> > >  
> > > +/* platform device for dma mapping */
> > > +static struct device *dev;
> > > +
> > > +/* fw_cfg revision attribute, in /sys/firmware/qemu_fw_cfg top-level dir.
> > > */
> > > +static u32 fw_cfg_rev;
> > > +
> > >  /* fw_cfg file directory entry type */
> > >  struct fw_cfg_file {
> > >  	u32 size;
> > > @@ -57,6 +73,12 @@ struct fw_cfg_file {
> > >  	char name[FW_CFG_MAX_FILE_PATH];
> > >  };
> > >  
> > > +struct fw_cfg_dma {
> > > +	u32 control;
> > > +	u32 length;
> > > +	u64 address;
> > > +} __packed;
> > > +
> > >  /* fw_cfg device i/o register addresses */
> > >  static bool fw_cfg_is_mmio;
> > >  static phys_addr_t fw_cfg_p_base;
> > > @@ -75,12 +97,93 @@ static inline u16 fw_cfg_sel_endianness(u16 key)
> > >  	return fw_cfg_is_mmio ? cpu_to_be16(key) : cpu_to_le16(key);
> > >  }
> > >  
> > > +static inline bool fw_cfg_dma_enabled(void)
> > > +{
> > > +	return fw_cfg_rev & FW_CFG_VERSION_DMA && fw_cfg_reg_dma;
> > > +}
> > > +
> > > +static bool fw_cfg_wait_for_control(struct fw_cfg_dma *d, unsigned long
> > > timeout)
> > > +{
> > > +	ktime_t start;
> > > +	ktime_t stop;
> > > +
> > > +	start = ktime_get();
> > > +	stop = ktime_add(start, ms_to_ktime(timeout));
> > > +
> > > +	do {
> > > +		if ((be32_to_cpu(d->control) & ~FW_CFG_DMA_CTL_ERROR) == 0)
> > > +			return true;
> > > +
> > > +		usleep_range(50, 100);
> > 
> > BTW it's not nice that this is uninterruptible. I think we need a
> > variant of usleep_range that is interruptible and call that here.
> > Thoughts?
> > 
> 
> This usleep_range() pattern is pretty common apparently.

In kernel - right, but doing it from userspace context means you can't
kill userspace until timeout expires.

> 
> (and it's probably better than calling yield() in a loop, like v1-3 did) 
> > 
> > > +	} while (ktime_before(ktime_get(), stop));
> > > +
> > > +	return false;
> > > +}
> > > +
> > > +static ssize_t fw_cfg_dma_transfer(void *address, u32 length, u32 control)
> > > +{
> > > +	dma_addr_t dma_addr = 0;
> > > +	struct fw_cfg_dma *d;
> > > +	dma_addr_t dma;
> > > +	ssize_t ret = length;
> > > +	enum dma_data_direction dir =
> > > +		(control & FW_CFG_DMA_CTL_READ ? DMA_FROM_DEVICE : 0);
> > > +
> > > +	if (address && length) {
> > > +		dma_addr = dma_map_single(dev, address, length, dir);
> > > +		if (dma_mapping_error(NULL, dma_addr)) {
> > > +			WARN(1, "%s: failed to map address\n", __func__);
> > > +			return -EFAULT;
> > > +		}
> > > +	}
> > > +
> > > +	d = kmalloc(sizeof(*d), GFP_KERNEL | GFP_DMA);
> > > +	if (!d) {
> > > +		ret = -ENOMEM;
> > > +		goto end;
> > > +	}
> > > +
> > > +	dma = dma_map_single(dev, d, sizeof(*d), DMA_BIDIRECTIONAL);
> > > +	if (dma_mapping_error(NULL, dma)) {
> > > +		WARN(1, "%s: failed to map fw_cfg_dma\n", __func__);
> > > +		ret = -EFAULT;
> > > +		goto end;
> > > +	}
> > > +
> > > +	*d = (struct fw_cfg_dma) {
> > > +		.address = cpu_to_be64(dma_addr),
> > > +		.length = cpu_to_be32(length),
> > > +		.control = cpu_to_be32(control)
> > > +	};
> > > +
> > > +	iowrite32be((u64)dma >> 32, fw_cfg_reg_dma);
> > > +	iowrite32be(dma, fw_cfg_reg_dma + 4);
> > > +
> > > +	if (!fw_cfg_wait_for_control(d, FW_CFG_DMA_TIMEOUT)) {
> > > +		WARN(1, "%s: timeout", __func__);
> > > +		ret = -ETIME;
> > > +	} else if (be32_to_cpu(d->control) & FW_CFG_DMA_CTL_ERROR) {
> > > +		ret = -EIO;
> > > +	}
> > > +
> > > +	dma_unmap_single(dev, dma, sizeof(*d), DMA_BIDIRECTIONAL);
> > > +
> > > +end:
> > > +	kfree(d);
> > > +	if (dma_addr)
> > > +		dma_unmap_single(dev, dma_addr, length, dir);
> > 
> > So if host was delayed what this does is corrupt guest memory.
> 
> By "delayed" you mean fw_cfg_wait_for_control() timed out?
> 
> Hmm, perhaps we should loop infinitely? that's also what the firmware does.
> 
> By the way, I failed to understand vcpu guest & qemu interaction here, shouldn't the vcpu thread be running the qemu dma_transfer until completion before returning to guest?
> 
> > IMHO things are already bad, let's at least keep it mapped
> > and allocated. Also what will heppen on next access attempt?
> 
> > Maybe we need to prevent further attempts.
> > And maybe fw cfg needs a reset register so we can recover
> > faster.
> 
> Or perhaps since it's all software, it should never fail? ;) That's apparently the device design.

As far as I can see, that is true. IOW the time-out is not needed,
if it's not completed on the first attempt, you can fail
immediately.


But a bigger issue is it seems to trigger failures for people.

> > 
> > 
> > > +
> > > +	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)
> > > +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:
> > > @@ -90,17 +193,36 @@ 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()) {
> > > +		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)
> > > +				goto end;
> > > +			ret = fw_cfg_dma_transfer(buf, count,
> > > +						  FW_CFG_DMA_CTL_READ);
> > > +		}
> > > +	} else {
> > > +		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);
> > > +	}
> > > +
> > > +end:
> > >  	mutex_unlock(&fw_cfg_dev_lock);
> > >  
> > >  	acpi_release_global_lock(glk);
> > > +
> > > +	return ret;
> > >  }
> > >  
> > >  /* clean up fw_cfg device i/o */
> > > @@ -192,7 +314,7 @@ 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);
> > > +	fw_cfg_read_blob(FW_CFG_SIGNATURE, sig, 0, FW_CFG_SIG_SIZE, false);
> > >  	if (memcmp(sig, "QEMU", FW_CFG_SIG_SIZE) != 0) {
> > >  		fw_cfg_io_cleanup();
> > >  		return -ENODEV;
> > > @@ -201,9 +323,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);
> > > @@ -351,8 +470,7 @@ static ssize_t fw_cfg_sysfs_read_raw(struct file *filp,
> > > struct kobject *kobj,
> > >  	if (count > entry->f.size - pos)
> > >  		count = entry->f.size - pos;
> > >  
> > > -	fw_cfg_read_blob(entry->f.select, buf, pos, count);
> > > -	return count;
> > > +	return fw_cfg_read_blob(entry->f.select, buf, pos, count, true);
> > >  }
> > >  
> > >  static struct bin_attribute fw_cfg_sysfs_attr_raw = {
> > > @@ -505,7 +623,7 @@ 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, &count, 0, sizeof(count));
> > > +	fw_cfg_read_blob(FW_CFG_FILE_DIR, &count, 0, sizeof(count), false);
> > >  	count = be32_to_cpu(count);
> > >  	dir_size = count * sizeof(struct fw_cfg_file);
> > >  
> > > @@ -513,7 +631,7 @@ static int fw_cfg_register_dir_entries(void)
> > >  	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(count), dir_size, true);
> > >  
> > >  	for (i = 0; i < count; i++) {
> > >  		dir[i].size = be32_to_cpu(dir[i].size);
> > > @@ -544,9 +662,10 @@ static int fw_cfg_sysfs_probe(struct platform_device
> > > *pdev)
> > >  	 * one fw_cfg device exist system-wide, so if one was already found
> > >  	 * earlier, we might as well stop here.
> > >  	 */
> > > -	if (fw_cfg_sel_ko)
> > > +	if (dev)
> > >  		return -EBUSY;
> > >  
> > > +	dev = &pdev->dev;
> > >  	/* create by_key and by_name subdirs of /sys/firmware/qemu_fw_cfg/ */
> > >  	err = -ENOMEM;
> > >  	fw_cfg_sel_ko = kobject_create_and_add("by_key", fw_cfg_top_ko);
> > > @@ -562,7 +681,7 @@ 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_read_blob(FW_CFG_ID, &fw_cfg_rev, 0, sizeof(fw_cfg_rev), false);
> > >  	fw_cfg_rev = le32_to_cpu(fw_cfg_rev);
> > >  	err = sysfs_create_file(fw_cfg_top_ko, &fw_cfg_rev_attr.attr);
> > >  	if (err)
> > > @@ -587,6 +706,7 @@ static int fw_cfg_sysfs_probe(struct platform_device
> > > *pdev)
> > >  err_name:
> > >  	fw_cfg_kobj_cleanup(fw_cfg_sel_ko);
> > >  err_sel:
> > > +	dev = NULL;
> > >  	return err;
> > >  }
> > >  
> > > --
> > > 2.15.0.125.g8f49766d64
> >