[PATCH v2] add support for pci in the pvpanic driver

[PATCH v2] add support for pci in the pvpanic driver

[Mihai Carabas]

This patchset adds support for PCI in the pvpanic driver. The device already
got in qemu [1].

v2:
- mmio -> MMIO, pci -> PCI suggested by Randy Dunlap.
- group pvpanic-common.c and mmio.c in the same module. The intention was to
  have only one module and the common code splitted up to be re-used by the
  pvpanic-pci module in the next patch.
- add a new patch where add the licenses for the new files (when I moved the
  code to mmio.c I preserved the original licenses there)
- properly clean-up PCI device when driver removed.

How to test:

Run a VM with latest qemu which has the pvpanic-pci device:
qemu-system-aarch64  -device pvpanic-pci

Generate a panic in the guest:
echo 1 > /proc/sys/kernel/sysrq
echo c > /proc/sysrq-trigger

Observe in the QMP console the panic events received by the device:

{"timestamp": {"seconds": 1613122186, "microseconds": 141729}, "event":
"GUEST_PANICKED", "data": {"action": "pause"}}

{"timestamp": {"seconds": 1613122186, "microseconds": 141833}, "event":
"GUEST_PANICKED", "data": {"action": "poweroff"}}

{"timestamp": {"seconds": 1613122186, "microseconds": 141896}, "event":
"SHUTDOWN", "data": {"guest": true, "reason": "guest-panic"}}


[1] https://github.com/qemu/qemu/commit/9df52f58e76e904fb141b10318362d718f470db2

Mihai Carabas (3):
  misc/pvpanic: split-up generic and platform dependent code
  misc/pvpanic: add PCI driver
  misc/pvpanic: add license

 drivers/misc/Kconfig                  |   9 +--
 drivers/misc/Makefile                 |   2 +-
 drivers/misc/pvpanic.c                | 111 ----------------------------------
 drivers/misc/pvpanic/Kconfig          |  27 +++++++++
 drivers/misc/pvpanic/Makefile         |  11 ++++
 drivers/misc/pvpanic/mmio.c           |  83 +++++++++++++++++++++++++
 drivers/misc/pvpanic/pci.c            |  56 +++++++++++++++++
 drivers/misc/pvpanic/pvpanic-common.c |  60 ++++++++++++++++++
 drivers/misc/pvpanic/pvpanic.h        |  17 ++++++
 9 files changed, 256 insertions(+), 120 deletions(-)
 delete mode 100644 drivers/misc/pvpanic.c
 create mode 100644 drivers/misc/pvpanic/Kconfig
 create mode 100644 drivers/misc/pvpanic/Makefile
 create mode 100644 drivers/misc/pvpanic/mmio.c
 create mode 100644 drivers/misc/pvpanic/pci.c
 create mode 100644 drivers/misc/pvpanic/pvpanic-common.c
 create mode 100644 drivers/misc/pvpanic/pvpanic.h

-- 
1.8.3.1

[PATCH v2 1/3] misc/pvpanic: split-up generic and platform dependent code

[Mihai Carabas]

Split-up generic and platform dependent code in order to be able to re-use
generic event handling code in pvpanic PCI device driver in the next patch.

The code from pvpanic.c was split in two new files:
- pvpanic-common.c: generic code that handles pvpanic events
- mmio.c: platform/bus dependent code

Signed-off-by: Mihai Carabas <mihai.carabas@oracle.com>
---
 drivers/misc/Kconfig                  |   9 +--
 drivers/misc/Makefile                 |   2 +-
 drivers/misc/pvpanic.c                | 111 ----------------------------------
 drivers/misc/pvpanic/Kconfig          |  12 ++++
 drivers/misc/pvpanic/Makefile         |   2 +
 drivers/misc/pvpanic/mmio.c           |  83 +++++++++++++++++++++++++
 drivers/misc/pvpanic/pvpanic-common.c |  60 ++++++++++++++++++
 drivers/misc/pvpanic/pvpanic.h        |  10 +++
 8 files changed, 169 insertions(+), 120 deletions(-)
 delete mode 100644 drivers/misc/pvpanic.c
 create mode 100644 drivers/misc/pvpanic/Kconfig
 create mode 100644 drivers/misc/pvpanic/Makefile
 create mode 100644 drivers/misc/pvpanic/mmio.c
 create mode 100644 drivers/misc/pvpanic/pvpanic-common.c
 create mode 100644 drivers/misc/pvpanic/pvpanic.h

diff --git a/drivers/misc/Kconfig b/drivers/misc/Kconfig
index fafa8b0..0273ecb 100644
--- a/drivers/misc/Kconfig
+++ b/drivers/misc/Kconfig
@@ -448,14 +448,6 @@ config MISC_RTSX
 	tristate
 	default MISC_RTSX_PCI || MISC_RTSX_USB
 
-config PVPANIC
-	tristate "pvpanic device support"
-	depends on HAS_IOMEM && (ACPI || OF)
-	help
-	  This driver provides support for the pvpanic device.  pvpanic is
-	  a paravirtualized device provided by QEMU; it lets a virtual machine
-	  (guest) communicate panic events to the host.
-
 config HISI_HIKEY_USB
 	tristate "USB GPIO Hub on HiSilicon Hikey 960/970 Platform"
 	depends on (OF && GPIOLIB) || COMPILE_TEST
@@ -481,4 +473,5 @@ source "drivers/misc/ocxl/Kconfig"
 source "drivers/misc/cardreader/Kconfig"
 source "drivers/misc/habanalabs/Kconfig"
 source "drivers/misc/uacce/Kconfig"
+source "drivers/misc/pvpanic/Kconfig"
 endmenu
diff --git a/drivers/misc/Makefile b/drivers/misc/Makefile
index d23231e..9f411b8 100644
--- a/drivers/misc/Makefile
+++ b/drivers/misc/Makefile
@@ -52,7 +52,7 @@ obj-$(CONFIG_CXL_BASE)		+= cxl/
 obj-$(CONFIG_PCI_ENDPOINT_TEST)	+= pci_endpoint_test.o
 obj-$(CONFIG_OCXL)		+= ocxl/
 obj-y				+= cardreader/
-obj-$(CONFIG_PVPANIC)   	+= pvpanic.o
+obj-$(CONFIG_PVPANIC)   	+= pvpanic/
 obj-$(CONFIG_HABANA_AI)		+= habanalabs/
 obj-$(CONFIG_UACCE)		+= uacce/
 obj-$(CONFIG_XILINX_SDFEC)	+= xilinx_sdfec.o
diff --git a/drivers/misc/pvpanic.c b/drivers/misc/pvpanic.c
deleted file mode 100644
index 41cab29..00000000
--- a/drivers/misc/pvpanic.c
+++ /dev/null
@@ -1,111 +0,0 @@
-// SPDX-License-Identifier: GPL-2.0+
-/*
- *  Pvpanic Device Support
- *
- *  Copyright (C) 2013 Fujitsu.
- *  Copyright (C) 2018 ZTE.
- */
-
-#define pr_fmt(fmt) KBUILD_MODNAME ": " fmt
-
-#include <linux/io.h>
-#include <linux/kernel.h>
-#include <linux/kexec.h>
-#include <linux/mod_devicetable.h>
-#include <linux/module.h>
-#include <linux/platform_device.h>
-#include <linux/types.h>
-
-#include <uapi/misc/pvpanic.h>
-
-static void __iomem *base;
-
-MODULE_AUTHOR("Hu Tao <hutao@cn.fujitsu.com>");
-MODULE_DESCRIPTION("pvpanic device driver");
-MODULE_LICENSE("GPL");
-
-static void
-pvpanic_send_event(unsigned int event)
-{
-	iowrite8(event, base);
-}
-
-static int
-pvpanic_panic_notify(struct notifier_block *nb, unsigned long code,
-		     void *unused)
-{
-	unsigned int event = PVPANIC_PANICKED;
-
-	if (kexec_crash_loaded())
-		event = PVPANIC_CRASH_LOADED;
-
-	pvpanic_send_event(event);
-
-	return NOTIFY_DONE;
-}
-
-static struct notifier_block pvpanic_panic_nb = {
-	.notifier_call = pvpanic_panic_notify,
-	.priority = 1, /* let this called before broken drm_fb_helper */
-};
-
-static int pvpanic_mmio_probe(struct platform_device *pdev)
-{
-	struct device *dev = &pdev->dev;
-	struct resource *res;
-
-	res = platform_get_mem_or_io(pdev, 0);
-	if (!res)
-		return -EINVAL;
-
-	switch (resource_type(res)) {
-	case IORESOURCE_IO:
-		base = devm_ioport_map(dev, res->start, resource_size(res));
-		if (!base)
-			return -ENOMEM;
-		break;
-	case IORESOURCE_MEM:
-		base = devm_ioremap_resource(dev, res);
-		if (IS_ERR(base))
-			return PTR_ERR(base);
-		break;
-	default:
-		return -EINVAL;
-	}
-
-	atomic_notifier_chain_register(&panic_notifier_list,
-				       &pvpanic_panic_nb);
-
-	return 0;
-}
-
-static int pvpanic_mmio_remove(struct platform_device *pdev)
-{
-
-	atomic_notifier_chain_unregister(&panic_notifier_list,
-					 &pvpanic_panic_nb);
-
-	return 0;
-}
-
-static const struct of_device_id pvpanic_mmio_match[] = {
-	{ .compatible = "qemu,pvpanic-mmio", },
-	{}
-};
-
-static const struct acpi_device_id pvpanic_device_ids[] = {
-	{ "QEMU0001", 0 },
-	{ "", 0 }
-};
-MODULE_DEVICE_TABLE(acpi, pvpanic_device_ids);
-
-static struct platform_driver pvpanic_mmio_driver = {
-	.driver = {
-		.name = "pvpanic-mmio",
-		.of_match_table = pvpanic_mmio_match,
-		.acpi_match_table = pvpanic_device_ids,
-	},
-	.probe = pvpanic_mmio_probe,
-	.remove = pvpanic_mmio_remove,
-};
-module_platform_driver(pvpanic_mmio_driver);
diff --git a/drivers/misc/pvpanic/Kconfig b/drivers/misc/pvpanic/Kconfig
new file mode 100644
index 00000000..0dce6ef
--- /dev/null
+++ b/drivers/misc/pvpanic/Kconfig
@@ -0,0 +1,12 @@
+config PVPANIC
+	bool "pvpanic device support"
+	help
+	  This option enables pvpanic device driver.
+
+config PVPANIC_MMIO
+	tristate "pvpanic MMIO device support"
+	depends on HAS_IOMEM && (ACPI || OF) && PVPANIC
+	help
+	  This driver provides support for the pvpanic device.  pvpanic is
+	  a paravirtualized device provided by QEMU; it lets a virtual machine
+	  (guest) communicate panic events to the host.
diff --git a/drivers/misc/pvpanic/Makefile b/drivers/misc/pvpanic/Makefile
new file mode 100644
index 00000000..9ea3355
--- /dev/null
+++ b/drivers/misc/pvpanic/Makefile
@@ -0,0 +1,2 @@
+obj-$(CONFIG_PVPANIC_MMIO)	+= pvpanic-mmio.o
+pvpanic-mmio-objs := pvpanic-common.o mmio.o
diff --git a/drivers/misc/pvpanic/mmio.c b/drivers/misc/pvpanic/mmio.c
new file mode 100644
index 00000000..7454eeb
--- /dev/null
+++ b/drivers/misc/pvpanic/mmio.c
@@ -0,0 +1,83 @@
+// SPDX-License-Identifier: GPL-2.0+
+/*
+ *  Pvpanic MMIO Device Support
+ *
+ *  Copyright (C) 2013 Fujitsu.
+ *  Copyright (C) 2018 ZTE.
+ *  Copyright (C) 2021 Oracle.
+*/
+
+#include <linux/io.h>
+#include <linux/kernel.h>
+#include <linux/kexec.h>
+#include <linux/mod_devicetable.h>
+#include <linux/module.h>
+#include <linux/platform_device.h>
+#include <linux/types.h>
+
+#include <uapi/misc/pvpanic.h>
+#include "pvpanic.h"
+
+MODULE_AUTHOR("Hu Tao <hutao@cn.fujitsu.com>");
+MODULE_DESCRIPTION("pvpanic device driver");
+MODULE_LICENSE("GPL");
+
+static int pvpanic_mmio_probe(struct platform_device *pdev)
+{
+	struct device *dev = &pdev->dev;
+	struct resource *res;
+	void __iomem *base;
+
+	res = platform_get_mem_or_io(pdev, 0);
+	if (!res)
+		return -EINVAL;
+
+	switch (resource_type(res)) {
+	case IORESOURCE_IO:
+		base = devm_ioport_map(dev, res->start, resource_size(res));
+		if (!base)
+			return -ENOMEM;
+		break;
+	case IORESOURCE_MEM:
+		base = devm_ioremap_resource(dev, res);
+		if (IS_ERR(base))
+			return PTR_ERR(base);
+		break;
+	default:
+		return -EINVAL;
+	}
+
+	pvpanic_probe(base);
+
+	return 0;
+}
+
+static int pvpanic_mmio_remove(struct platform_device *pdev)
+{
+
+	pvpanic_remove();
+
+	return 0;
+}
+
+static const struct of_device_id pvpanic_mmio_match[] = {
+	{ .compatible = "qemu,pvpanic-mmio", },
+	{}
+};
+
+static const struct acpi_device_id pvpanic_device_ids[] = {
+	{ "QEMU0001", 0 },
+	{ "", 0 }
+};
+MODULE_DEVICE_TABLE(acpi, pvpanic_device_ids);
+
+static struct platform_driver pvpanic_mmio_driver = {
+	.driver = {
+		.name = "pvpanic-mmio",
+		.of_match_table = pvpanic_mmio_match,
+		.acpi_match_table = pvpanic_device_ids,
+	},
+	.probe = pvpanic_mmio_probe,
+	.remove = pvpanic_mmio_remove,
+};
+module_platform_driver(pvpanic_mmio_driver);
diff --git a/drivers/misc/pvpanic/pvpanic-common.c b/drivers/misc/pvpanic/pvpanic-common.c
new file mode 100644
index 00000000..395ccae
--- /dev/null
+++ b/drivers/misc/pvpanic/pvpanic-common.c
@@ -0,0 +1,60 @@
+// SPDX-License-Identifier: GPL-2.0+
+/*
+ *  Pvpanic Device Support
+ *
+ *  Copyright (C) 2013 Fujitsu.
+ *  Copyright (C) 2018 ZTE.
+ *  Copyright (C) 2021 Oracle.
+ */
+
+#include <linux/io.h>
+#include <linux/kernel.h>
+#include <linux/kexec.h>
+#include <linux/mod_devicetable.h>
+#include <linux/module.h>
+#include <linux/platform_device.h>
+#include <linux/types.h>
+
+#include <uapi/misc/pvpanic.h>
+
+static void __iomem *base;
+
+static void
+pvpanic_send_event(unsigned int event)
+{
+	iowrite8(event, base);
+}
+
+static int
+pvpanic_panic_notify(struct notifier_block *nb, unsigned long code,
+		     void *unused)
+{
+	unsigned int event = PVPANIC_PANICKED;
+
+	if (kexec_crash_loaded())
+		event = PVPANIC_CRASH_LOADED;
+
+	pvpanic_send_event(event);
+
+	return NOTIFY_DONE;
+}
+
+static struct notifier_block pvpanic_panic_nb = {
+	.notifier_call = pvpanic_panic_notify,
+	.priority = 1, /* let this called before broken drm_fb_helper */
+};
+
+void pvpanic_probe(void __iomem *pbase)
+{
+	base = pbase;
+	atomic_notifier_chain_register(&panic_notifier_list,
+				       &pvpanic_panic_nb);
+}
+
+void pvpanic_remove(void)
+{
+
+	atomic_notifier_chain_unregister(&panic_notifier_list,
+					 &pvpanic_panic_nb);
+	base = NULL;
+}
diff --git a/drivers/misc/pvpanic/pvpanic.h b/drivers/misc/pvpanic/pvpanic.h
new file mode 100644
index 00000000..4d6c221
--- /dev/null
+++ b/drivers/misc/pvpanic/pvpanic.h
@@ -0,0 +1,10 @@
+#ifndef PVPANIC_H_
+#define PVPANIC_H_
+
+
+#define pr_fmt(fmt) KBUILD_MODNAME ": " fmt
+
+void pvpanic_probe(void __iomem *base);
+void pvpanic_remove(void);
+
+#endif /* PVPANIC_H_ */
-- 
1.8.3.1

[PATCH v2 2/3] misc/pvpanic: add PCI driver

[Mihai Carabas]

Add support for pvpanic PCI device added in qemu [1]. At probe time, obtain the
address where to read/write pvpanic events and pass it to the generic handling
code. Will follow the same logic as pvpanic MMIO device driver. At remove time,
unmap base address and disable PCI device.

[1] https://github.com/qemu/qemu/commit/9df52f58e76e904fb141b10318362d718f470db2

Signed-off-by: Mihai Carabas <mihai.carabas@oracle.com>
---
 drivers/misc/pvpanic/Kconfig  |  8 +++++++
 drivers/misc/pvpanic/Makefile |  2 ++
 drivers/misc/pvpanic/pci.c    | 49 +++++++++++++++++++++++++++++++++++++++++++
 3 files changed, 59 insertions(+)
 create mode 100644 drivers/misc/pvpanic/pci.c

diff --git a/drivers/misc/pvpanic/Kconfig b/drivers/misc/pvpanic/Kconfig
index 0dce6ef..ce8b93e 100644
--- a/drivers/misc/pvpanic/Kconfig
+++ b/drivers/misc/pvpanic/Kconfig
@@ -10,3 +10,11 @@ config PVPANIC_MMIO
 	  This driver provides support for the pvpanic device.  pvpanic is
 	  a paravirtualized device provided by QEMU; it lets a virtual machine
 	  (guest) communicate panic events to the host.
+
+config PVPANIC_PCI
+	tristate "pvpanic PCI device support"
+	depends on PCI && PVPANIC
+	help
+	  This driver provides support for the pvpanic device.  pvpanic is
+	  a paravirtualized device provided by QEMU; it lets a virtual machine
+	  (guest) communicate panic events to the host.
diff --git a/drivers/misc/pvpanic/Makefile b/drivers/misc/pvpanic/Makefile
index 9ea3355..1763450 100644
--- a/drivers/misc/pvpanic/Makefile
+++ b/drivers/misc/pvpanic/Makefile
@@ -1,2 +1,4 @@
 obj-$(CONFIG_PVPANIC_MMIO)	+= pvpanic-mmio.o
 pvpanic-mmio-objs := pvpanic-common.o mmio.o
+obj-$(CONFIG_PVPANIC_PCI)	+= pvpanic-pci.o
+pvpanic-pci-objs := pvpanic-common.o pci.o
diff --git a/drivers/misc/pvpanic/pci.c b/drivers/misc/pvpanic/pci.c
new file mode 100644
index 00000000..b672727
--- /dev/null
+++ b/drivers/misc/pvpanic/pci.c
@@ -0,0 +1,49 @@
+#include <linux/kernel.h>
+#include <linux/module.h>
+#include <linux/pci.h>
+#include <linux/types.h>
+#include "pvpanic.h"
+
+#define PCI_VENDOR_ID_REDHAT             0x1b36
+#define PCI_DEVICE_ID_REDHAT_PVPANIC     0x0011
+
+static void __iomem *base;
+static const struct pci_device_id pvpanic_pci_id_tbl[]  = {
+	{ PCI_DEVICE(PCI_VENDOR_ID_REDHAT, PCI_DEVICE_ID_REDHAT_PVPANIC),},
+	{}
+};
+
+static int pvpanic_pci_probe(struct pci_dev *pdev,
+			     const struct pci_device_id *ent)
+{
+	int ret;
+	struct resource res;
+
+	ret = pci_enable_device(pdev);
+	if (ret < 0)
+		return ret;
+
+	base = pci_iomap(pdev, 0, 0);
+	if (IS_ERR(base))
+		return PTR_ERR(base);
+
+	pvpanic_probe(base);
+
+	return 0;
+}
+
+static void pvpanic_pci_remove(struct pci_dev *pdev)
+{
+	pvpanic_remove();
+	iounmap(base);
+	pci_disable_device(pdev);
+}
+
+static struct pci_driver pvpanic_pci_driver = {
+	.name =         "pvpanic-pci",
+	.id_table =     pvpanic_pci_id_tbl,
+	.probe =        pvpanic_pci_probe,
+	.remove =       pvpanic_pci_remove,
+};
+
+module_pci_driver(pvpanic_pci_driver);
-- 
1.8.3.1

[PATCH v2 3/3] misc/pvpanic: add license

[Mihai Carabas]

Add license to the newly created files in adding support for pvpanic PCI device
driver.

Signed-off-by: Mihai Carabas <mihai.carabas@oracle.com>
---
 drivers/misc/pvpanic/Kconfig   | 7 +++++++
 drivers/misc/pvpanic/Makefile  | 7 +++++++
 drivers/misc/pvpanic/pci.c     | 7 +++++++
 drivers/misc/pvpanic/pvpanic.h | 7 +++++++
 4 files changed, 28 insertions(+)

diff --git a/drivers/misc/pvpanic/Kconfig b/drivers/misc/pvpanic/Kconfig
index ce8b93e..004c2b7 100644
--- a/drivers/misc/pvpanic/Kconfig
+++ b/drivers/misc/pvpanic/Kconfig
@@ -1,3 +1,10 @@
+# SPDX-License-Identifier: GPL-2.0+
+#
+# Pvpanic Kconfig
+#
+# Copyright (C) 2021 Oracle.
+#
+
 config PVPANIC
 	bool "pvpanic device support"
 	help
diff --git a/drivers/misc/pvpanic/Makefile b/drivers/misc/pvpanic/Makefile
index 1763450..898a731 100644
--- a/drivers/misc/pvpanic/Makefile
+++ b/drivers/misc/pvpanic/Makefile
@@ -1,3 +1,10 @@
+# SPDX-License-Identifier: GPL-2.0+
+#
+# Pvpanic Makefile
+#
+# Copyright (C) 2021 Oracle.
+#
+
 obj-$(CONFIG_PVPANIC_MMIO)	+= pvpanic-mmio.o
 pvpanic-mmio-objs := pvpanic-common.o mmio.o
 obj-$(CONFIG_PVPANIC_PCI)	+= pvpanic-pci.o
diff --git a/drivers/misc/pvpanic/pci.c b/drivers/misc/pvpanic/pci.c
index b672727..d1c1ed9 100644
--- a/drivers/misc/pvpanic/pci.c
+++ b/drivers/misc/pvpanic/pci.c
@@ -1,3 +1,10 @@
+// SPDX-License-Identifier: GPL-2.0+
+/*
+ *  Pvpanic PCI Device Support
+ *
+ *  Copyright (C) 2021 Oracle.
+*/
+
 #include <linux/kernel.h>
 #include <linux/module.h>
 #include <linux/pci.h>
diff --git a/drivers/misc/pvpanic/pvpanic.h b/drivers/misc/pvpanic/pvpanic.h
index 4d6c221..7e75dce 100644
--- a/drivers/misc/pvpanic/pvpanic.h
+++ b/drivers/misc/pvpanic/pvpanic.h
@@ -1,3 +1,10 @@
+// SPDX-License-Identifier: GPL-2.0+
+/*
+ *  Pvpanic Device Support
+ *
+ *  Copyright (C) 2021 Oracle.
+*/
+
 #ifndef PVPANIC_H_
 #define PVPANIC_H_
 
-- 
1.8.3.1

Re: [PATCH v2 1/3] misc/pvpanic: split-up generic and platform dependent code

[Arnd Bergmann]

On Fri, Feb 12, 2021 at 10:17 AM Mihai Carabas <mihai.carabas@oracle.com> wrote:
>
> Split-up generic and platform dependent code in order to be able to re-use
> generic event handling code in pvpanic PCI device driver in the next patch.
>
> The code from pvpanic.c was split in two new files:
> - pvpanic-common.c: generic code that handles pvpanic events
> - mmio.c: platform/bus dependent code
>
> Signed-off-by: Mihai Carabas <mihai.carabas@oracle.com>

The patch looks fine, but for review purposes it's better to generate
the email with 'git format-patch -M'.

Reviewed-by: Arnd Bergmann <arnd@arndb.de>

Re: [PATCH v2 3/3] misc/pvpanic: add license

[Arnd Bergmann]

On Fri, Feb 12, 2021 at 10:17 AM Mihai Carabas <mihai.carabas@oracle.com> wrote:
>
> Add license to the newly created files in adding support for pvpanic PCI device
> driver.
>
> Signed-off-by: Mihai Carabas <mihai.carabas@oracle.com>

I don't see why this is a separate patch, rather than part of the patch
that creates these files.

        Arnd

Re: [PATCH v2 2/3] misc/pvpanic: add PCI driver

[Arnd Bergmann]

On Fri, Feb 12, 2021 at 10:17 AM Mihai Carabas <mihai.carabas@oracle.com> wrote:
>
> Add support for pvpanic PCI device added in qemu [1]. At probe time, obtain the
> address where to read/write pvpanic events and pass it to the generic handling
> code. Will follow the same logic as pvpanic MMIO device driver. At remove time,
> unmap base address and disable PCI device.
>
> [1] https://github.com/qemu/qemu/commit/9df52f58e76e904fb141b10318362d718f470db2
>
> Signed-off-by: Mihai Carabas <mihai.carabas@oracle.com>

Reviewed-by: Arnd Bergmann <arnd@arndb.de>

Re: [PATCH v2 3/3] misc/pvpanic: add license

[Greg KH]

On Fri, Feb 12, 2021 at 11:17:06AM +0200, Mihai Carabas wrote:
> Add license to the newly created files in adding support for pvpanic PCI device
> driver.
> 
> Signed-off-by: Mihai Carabas <mihai.carabas@oracle.com>
> ---
>  drivers/misc/pvpanic/Kconfig   | 7 +++++++
>  drivers/misc/pvpanic/Makefile  | 7 +++++++
>  drivers/misc/pvpanic/pci.c     | 7 +++++++
>  drivers/misc/pvpanic/pvpanic.h | 7 +++++++

Do not add this after-the-fact, add them when you create these files.

thanks,

greg k-h

Re: [PATCH v2 1/3] misc/pvpanic: split-up generic and platform dependent code

[Greg KH]

On Fri, Feb 12, 2021 at 11:17:04AM +0200, Mihai Carabas wrote:
> Split-up generic and platform dependent code in order to be able to re-use
> generic event handling code in pvpanic PCI device driver in the next patch.
> 
> The code from pvpanic.c was split in two new files:
> - pvpanic-common.c: generic code that handles pvpanic events
> - mmio.c: platform/bus dependent code
> 
> Signed-off-by: Mihai Carabas <mihai.carabas@oracle.com>
> ---
>  drivers/misc/Kconfig                  |   9 +--
>  drivers/misc/Makefile                 |   2 +-
>  drivers/misc/pvpanic.c                | 111 ----------------------------------
>  drivers/misc/pvpanic/Kconfig          |  12 ++++
>  drivers/misc/pvpanic/Makefile         |   2 +
>  drivers/misc/pvpanic/mmio.c           |  83 +++++++++++++++++++++++++
>  drivers/misc/pvpanic/pvpanic-common.c |  60 ++++++++++++++++++
>  drivers/misc/pvpanic/pvpanic.h        |  10 +++
>  8 files changed, 169 insertions(+), 120 deletions(-)
>  delete mode 100644 drivers/misc/pvpanic.c
>  create mode 100644 drivers/misc/pvpanic/Kconfig
>  create mode 100644 drivers/misc/pvpanic/Makefile
>  create mode 100644 drivers/misc/pvpanic/mmio.c
>  create mode 100644 drivers/misc/pvpanic/pvpanic-common.c
>  create mode 100644 drivers/misc/pvpanic/pvpanic.h
> 
> diff --git a/drivers/misc/Kconfig b/drivers/misc/Kconfig
> index fafa8b0..0273ecb 100644
> --- a/drivers/misc/Kconfig
> +++ b/drivers/misc/Kconfig
> @@ -448,14 +448,6 @@ config MISC_RTSX
>  	tristate
>  	default MISC_RTSX_PCI || MISC_RTSX_USB
>  
> -config PVPANIC
> -	tristate "pvpanic device support"
> -	depends on HAS_IOMEM && (ACPI || OF)
> -	help
> -	  This driver provides support for the pvpanic device.  pvpanic is
> -	  a paravirtualized device provided by QEMU; it lets a virtual machine
> -	  (guest) communicate panic events to the host.
> -
>  config HISI_HIKEY_USB
>  	tristate "USB GPIO Hub on HiSilicon Hikey 960/970 Platform"
>  	depends on (OF && GPIOLIB) || COMPILE_TEST
> @@ -481,4 +473,5 @@ source "drivers/misc/ocxl/Kconfig"
>  source "drivers/misc/cardreader/Kconfig"
>  source "drivers/misc/habanalabs/Kconfig"
>  source "drivers/misc/uacce/Kconfig"
> +source "drivers/misc/pvpanic/Kconfig"
>  endmenu
> diff --git a/drivers/misc/Makefile b/drivers/misc/Makefile
> index d23231e..9f411b8 100644
> --- a/drivers/misc/Makefile
> +++ b/drivers/misc/Makefile
> @@ -52,7 +52,7 @@ obj-$(CONFIG_CXL_BASE)		+= cxl/
>  obj-$(CONFIG_PCI_ENDPOINT_TEST)	+= pci_endpoint_test.o
>  obj-$(CONFIG_OCXL)		+= ocxl/
>  obj-y				+= cardreader/
> -obj-$(CONFIG_PVPANIC)   	+= pvpanic.o
> +obj-$(CONFIG_PVPANIC)   	+= pvpanic/
>  obj-$(CONFIG_HABANA_AI)		+= habanalabs/
>  obj-$(CONFIG_UACCE)		+= uacce/
>  obj-$(CONFIG_XILINX_SDFEC)	+= xilinx_sdfec.o
> diff --git a/drivers/misc/pvpanic.c b/drivers/misc/pvpanic.c
> deleted file mode 100644
> index 41cab29..00000000
> --- a/drivers/misc/pvpanic.c
> +++ /dev/null
> @@ -1,111 +0,0 @@
> -// SPDX-License-Identifier: GPL-2.0+
> -/*
> - *  Pvpanic Device Support
> - *
> - *  Copyright (C) 2013 Fujitsu.
> - *  Copyright (C) 2018 ZTE.
> - */
> -
> -#define pr_fmt(fmt) KBUILD_MODNAME ": " fmt
> -
> -#include <linux/io.h>
> -#include <linux/kernel.h>
> -#include <linux/kexec.h>
> -#include <linux/mod_devicetable.h>
> -#include <linux/module.h>
> -#include <linux/platform_device.h>
> -#include <linux/types.h>
> -
> -#include <uapi/misc/pvpanic.h>
> -
> -static void __iomem *base;
> -
> -MODULE_AUTHOR("Hu Tao <hutao@cn.fujitsu.com>");
> -MODULE_DESCRIPTION("pvpanic device driver");
> -MODULE_LICENSE("GPL");
> -
> -static void
> -pvpanic_send_event(unsigned int event)
> -{
> -	iowrite8(event, base);
> -}
> -
> -static int
> -pvpanic_panic_notify(struct notifier_block *nb, unsigned long code,
> -		     void *unused)
> -{
> -	unsigned int event = PVPANIC_PANICKED;
> -
> -	if (kexec_crash_loaded())
> -		event = PVPANIC_CRASH_LOADED;
> -
> -	pvpanic_send_event(event);
> -
> -	return NOTIFY_DONE;
> -}
> -
> -static struct notifier_block pvpanic_panic_nb = {
> -	.notifier_call = pvpanic_panic_notify,
> -	.priority = 1, /* let this called before broken drm_fb_helper */
> -};
> -
> -static int pvpanic_mmio_probe(struct platform_device *pdev)
> -{
> -	struct device *dev = &pdev->dev;
> -	struct resource *res;
> -
> -	res = platform_get_mem_or_io(pdev, 0);
> -	if (!res)
> -		return -EINVAL;
> -
> -	switch (resource_type(res)) {
> -	case IORESOURCE_IO:
> -		base = devm_ioport_map(dev, res->start, resource_size(res));
> -		if (!base)
> -			return -ENOMEM;
> -		break;
> -	case IORESOURCE_MEM:
> -		base = devm_ioremap_resource(dev, res);
> -		if (IS_ERR(base))
> -			return PTR_ERR(base);
> -		break;
> -	default:
> -		return -EINVAL;
> -	}
> -
> -	atomic_notifier_chain_register(&panic_notifier_list,
> -				       &pvpanic_panic_nb);
> -
> -	return 0;
> -}
> -
> -static int pvpanic_mmio_remove(struct platform_device *pdev)
> -{
> -
> -	atomic_notifier_chain_unregister(&panic_notifier_list,
> -					 &pvpanic_panic_nb);
> -
> -	return 0;
> -}
> -
> -static const struct of_device_id pvpanic_mmio_match[] = {
> -	{ .compatible = "qemu,pvpanic-mmio", },
> -	{}
> -};
> -
> -static const struct acpi_device_id pvpanic_device_ids[] = {
> -	{ "QEMU0001", 0 },
> -	{ "", 0 }
> -};
> -MODULE_DEVICE_TABLE(acpi, pvpanic_device_ids);
> -
> -static struct platform_driver pvpanic_mmio_driver = {
> -	.driver = {
> -		.name = "pvpanic-mmio",
> -		.of_match_table = pvpanic_mmio_match,
> -		.acpi_match_table = pvpanic_device_ids,
> -	},
> -	.probe = pvpanic_mmio_probe,
> -	.remove = pvpanic_mmio_remove,
> -};
> -module_platform_driver(pvpanic_mmio_driver);
> diff --git a/drivers/misc/pvpanic/Kconfig b/drivers/misc/pvpanic/Kconfig
> new file mode 100644
> index 00000000..0dce6ef
> --- /dev/null
> +++ b/drivers/misc/pvpanic/Kconfig
> @@ -0,0 +1,12 @@
> +config PVPANIC
> +	bool "pvpanic device support"
> +	help
> +	  This option enables pvpanic device driver.

No it doesn't, it allows you to _select_ a specific pvpanic driver, on
its own, it is not a driver, right?

> +
> +config PVPANIC_MMIO
> +	tristate "pvpanic MMIO device support"
> +	depends on HAS_IOMEM && (ACPI || OF) && PVPANIC
> +	help
> +	  This driver provides support for the pvpanic device.  pvpanic is
> +	  a paravirtualized device provided by QEMU; it lets a virtual machine
> +	  (guest) communicate panic events to the host.
> diff --git a/drivers/misc/pvpanic/Makefile b/drivers/misc/pvpanic/Makefile
> new file mode 100644
> index 00000000..9ea3355
> --- /dev/null
> +++ b/drivers/misc/pvpanic/Makefile
> @@ -0,0 +1,2 @@
> +obj-$(CONFIG_PVPANIC_MMIO)	+= pvpanic-mmio.o
> +pvpanic-mmio-objs := pvpanic-common.o mmio.o

You put the "common" logic in the mmio driver?  How is that going to
work for the PCI driver?

Why is there not a pvpanic.ko that contains the "core" code only?

thanks,

greg k-h

Re: [PATCH v2 2/3] misc/pvpanic: add PCI driver

[Greg KH]

On Fri, Feb 12, 2021 at 11:17:05AM +0200, Mihai Carabas wrote:
> Add support for pvpanic PCI device added in qemu [1]. At probe time, obtain the
> address where to read/write pvpanic events and pass it to the generic handling
> code. Will follow the same logic as pvpanic MMIO device driver. At remove time,
> unmap base address and disable PCI device.
> 
> [1] https://github.com/qemu/qemu/commit/9df52f58e76e904fb141b10318362d718f470db2
> 
> Signed-off-by: Mihai Carabas <mihai.carabas@oracle.com>
> ---
>  drivers/misc/pvpanic/Kconfig  |  8 +++++++
>  drivers/misc/pvpanic/Makefile |  2 ++
>  drivers/misc/pvpanic/pci.c    | 49 +++++++++++++++++++++++++++++++++++++++++++
>  3 files changed, 59 insertions(+)
>  create mode 100644 drivers/misc/pvpanic/pci.c
> 
> diff --git a/drivers/misc/pvpanic/Kconfig b/drivers/misc/pvpanic/Kconfig
> index 0dce6ef..ce8b93e 100644
> --- a/drivers/misc/pvpanic/Kconfig
> +++ b/drivers/misc/pvpanic/Kconfig
> @@ -10,3 +10,11 @@ config PVPANIC_MMIO
>  	  This driver provides support for the pvpanic device.  pvpanic is
>  	  a paravirtualized device provided by QEMU; it lets a virtual machine
>  	  (guest) communicate panic events to the host.
> +
> +config PVPANIC_PCI
> +	tristate "pvpanic PCI device support"
> +	depends on PCI && PVPANIC
> +	help
> +	  This driver provides support for the pvpanic device.  pvpanic is
> +	  a paravirtualized device provided by QEMU; it lets a virtual machine
> +	  (guest) communicate panic events to the host.
> diff --git a/drivers/misc/pvpanic/Makefile b/drivers/misc/pvpanic/Makefile
> index 9ea3355..1763450 100644
> --- a/drivers/misc/pvpanic/Makefile
> +++ b/drivers/misc/pvpanic/Makefile
> @@ -1,2 +1,4 @@
>  obj-$(CONFIG_PVPANIC_MMIO)	+= pvpanic-mmio.o
>  pvpanic-mmio-objs := pvpanic-common.o mmio.o
> +obj-$(CONFIG_PVPANIC_PCI)	+= pvpanic-pci.o
> +pvpanic-pci-objs := pvpanic-common.o pci.o

So you now have the pvpanic-common.o file linked into both modules at
the same time?  What happens if they are both loaded?

This feels really broken...

greg k-h

Re: [PATCH v2 2/3] misc/pvpanic: add PCI driver

[Arnd Bergmann]

On Fri, Feb 12, 2021 at 11:18 AM Greg KH <gregkh@linuxfoundation.org> wrote:
le
> > @@ -1,2 +1,4 @@
> >  obj-$(CONFIG_PVPANIC_MMIO)   += pvpanic-mmio.o
> >  pvpanic-mmio-objs := pvpanic-common.o mmio.o
> > +obj-$(CONFIG_PVPANIC_PCI)    += pvpanic-pci.o
> > +pvpanic-pci-objs := pvpanic-common.o pci.o
>
> So you now have the pvpanic-common.o file linked into both modules at
> the same time?  What happens if they are both loaded?
>
> This feels really broken...

Loading them as two modules should work, but this will probably lead to
a link failure if one of the two drivers is built-in and the other one is
a loadable module.

The usual way to fix this is to make the common part a separate module
with exported symbols for the probe/release functions and built
based on CONFIG_PVPANIC, which then needs to be a tristate
symbol again.

      Arnd

Re: [PATCH v2 1/3] misc/pvpanic: split-up generic and platform dependent code

[Mihai Carabas]

..snip
>> -};
>> -module_platform_driver(pvpanic_mmio_driver);
>> diff --git a/drivers/misc/pvpanic/Kconfig b/drivers/misc/pvpanic/Kconfig
>> new file mode 100644
>> index 00000000..0dce6ef
>> --- /dev/null
>> +++ b/drivers/misc/pvpanic/Kconfig
>> @@ -0,0 +1,12 @@
>> +config PVPANIC
>> +	bool "pvpanic device support"
>> +	help
>> +	  This option enables pvpanic device driver.
> No it doesn't, it allows you to _select_ a specific pvpanic driver, on
> its own, it is not a driver, right?

Yes. I will update the comment.


>> +
>> +config PVPANIC_MMIO
>> +	tristate "pvpanic MMIO device support"
>> +	depends on HAS_IOMEM && (ACPI || OF) && PVPANIC
>> +	help
>> +	  This driver provides support for the pvpanic device.  pvpanic is
>> +	  a paravirtualized device provided by QEMU; it lets a virtual machine
>> +	  (guest) communicate panic events to the host.
>> diff --git a/drivers/misc/pvpanic/Makefile b/drivers/misc/pvpanic/Makefile
>> new file mode 100644
>> index 00000000..9ea3355
>> --- /dev/null
>> +++ b/drivers/misc/pvpanic/Makefile
>> @@ -0,0 +1,2 @@
>> +obj-$(CONFIG_PVPANIC_MMIO)	+= pvpanic-mmio.o
>> +pvpanic-mmio-objs := pvpanic-common.o mmio.o
> You put the "common" logic in the mmio driver?  How is that going to
> work for the PCI driver?
>
> Why is there not a pvpanic.ko that contains the "core" code only?

My intention was to put the code in both drivers. There is no generic 
module as it will complicate things: the generic module would have to 
have its own state (e.g. pvpanic_probe would have to create a queue of 
base addresses).

Do you want to see a generic module pvpanic.ko with its own state?

Thank you,
Mihai

Re: [PATCH v2 1/3] misc/pvpanic: split-up generic and platform dependent code

[Greg KH]

On Fri, Feb 12, 2021 at 12:29:49PM +0200, Mihai Carabas wrote:
> ..snip
> > > -};
> > > -module_platform_driver(pvpanic_mmio_driver);
> > > diff --git a/drivers/misc/pvpanic/Kconfig b/drivers/misc/pvpanic/Kconfig
> > > new file mode 100644
> > > index 00000000..0dce6ef
> > > --- /dev/null
> > > +++ b/drivers/misc/pvpanic/Kconfig
> > > @@ -0,0 +1,12 @@
> > > +config PVPANIC
> > > +	bool "pvpanic device support"
> > > +	help
> > > +	  This option enables pvpanic device driver.
> > No it doesn't, it allows you to _select_ a specific pvpanic driver, on
> > its own, it is not a driver, right?
> 
> Yes. I will update the comment.
> 
> 
> > > +
> > > +config PVPANIC_MMIO
> > > +	tristate "pvpanic MMIO device support"
> > > +	depends on HAS_IOMEM && (ACPI || OF) && PVPANIC
> > > +	help
> > > +	  This driver provides support for the pvpanic device.  pvpanic is
> > > +	  a paravirtualized device provided by QEMU; it lets a virtual machine
> > > +	  (guest) communicate panic events to the host.
> > > diff --git a/drivers/misc/pvpanic/Makefile b/drivers/misc/pvpanic/Makefile
> > > new file mode 100644
> > > index 00000000..9ea3355
> > > --- /dev/null
> > > +++ b/drivers/misc/pvpanic/Makefile
> > > @@ -0,0 +1,2 @@
> > > +obj-$(CONFIG_PVPANIC_MMIO)	+= pvpanic-mmio.o
> > > +pvpanic-mmio-objs := pvpanic-common.o mmio.o
> > You put the "common" logic in the mmio driver?  How is that going to
> > work for the PCI driver?
> > 
> > Why is there not a pvpanic.ko that contains the "core" code only?
> 
> My intention was to put the code in both drivers. There is no generic module
> as it will complicate things: the generic module would have to have its own
> state (e.g. pvpanic_probe would have to create a queue of base addresses).

And when you link both into the kernel image directly, you end up with
duplicate symbols that break the build :(

> Do you want to see a generic module pvpanic.ko with its own state?

As it is, it will just not work so you have to do something...

thanks,

greg k-h

Re: [PATCH v2 1/3] misc/pvpanic: split-up generic and platform dependent code

[kernel test robot]

[-- Attachment #1: Type: text/plain, Size: 2188 bytes --]

Hi Mihai,

Thank you for the patch! Perhaps something to improve:

[auto build test WARNING on linux/master]
[also build test WARNING on soc/for-next linus/master v5.11-rc7]
[cannot apply to char-misc/char-misc-testing next-20210211]
[If your patch is applied to the wrong git tree, kindly drop us a note.
And when submitting patch, we suggest to use '--base' as documented in
https://git-scm.com/docs/git-format-patch]

url:    https://github.com/0day-ci/linux/commits/Mihai-Carabas/misc-pvpanic-split-up-generic-and-platform-dependent-code/20210212-181711
base:   https://git.kernel.org/pub/scm/linux/kernel/git/torvalds/linux.git 2ab38c17aac10bf55ab3efde4c4db3893d8691d2
config: i386-allyesconfig (attached as .config)
compiler: gcc-9 (Debian 9.3.0-15) 9.3.0
reproduce (this is a W=1 build):
        # https://github.com/0day-ci/linux/commit/17d14b4802b75b93f10e01153b0e395f6a638492
        git remote add linux-review https://github.com/0day-ci/linux
        git fetch --no-tags linux-review Mihai-Carabas/misc-pvpanic-split-up-generic-and-platform-dependent-code/20210212-181711
        git checkout 17d14b4802b75b93f10e01153b0e395f6a638492
        # save the attached .config to linux build tree
        make W=1 ARCH=i386 

If you fix the issue, kindly add following tag as appropriate
Reported-by: kernel test robot <lkp@intel.com>

All warnings (new ones prefixed by >>):

>> drivers/misc/pvpanic/pvpanic-common.c:47:6: warning: no previous prototype for 'pvpanic_probe' [-Wmissing-prototypes]
      47 | void pvpanic_probe(void __iomem *pbase)
         |      ^~~~~~~~~~~~~
>> drivers/misc/pvpanic/pvpanic-common.c:54:6: warning: no previous prototype for 'pvpanic_remove' [-Wmissing-prototypes]
      54 | void pvpanic_remove(void)
         |      ^~~~~~~~~~~~~~


vim +/pvpanic_probe +47 drivers/misc/pvpanic/pvpanic-common.c

    46	
  > 47	void pvpanic_probe(void __iomem *pbase)
    48	{
    49		base = pbase;
    50		atomic_notifier_chain_register(&panic_notifier_list,
    51					       &pvpanic_panic_nb);
    52	}
    53	
  > 54	void pvpanic_remove(void)

---
0-DAY CI Kernel Test Service, Intel Corporation
https://lists.01.org/hyperkitty/list/kbuild-all@lists.01.org

[-- Attachment #2: .config.gz --]
[-- Type: application/gzip, Size: 64183 bytes --]

Re: [PATCH v2 1/3] misc/pvpanic: split-up generic and platform dependent code

[kernel test robot]

[-- Attachment #1: Type: text/plain, Size: 2241 bytes --]

Hi Mihai,

Thank you for the patch! Perhaps something to improve:

[auto build test WARNING on linux/master]
[also build test WARNING on soc/for-next linus/master v5.11-rc7]
[cannot apply to char-misc/char-misc-testing next-20210211]
[If your patch is applied to the wrong git tree, kindly drop us a note.
And when submitting patch, we suggest to use '--base' as documented in
https://git-scm.com/docs/git-format-patch]

url:    https://github.com/0day-ci/linux/commits/Mihai-Carabas/misc-pvpanic-split-up-generic-and-platform-dependent-code/20210212-181711
base:   https://git.kernel.org/pub/scm/linux/kernel/git/torvalds/linux.git 2ab38c17aac10bf55ab3efde4c4db3893d8691d2
config: i386-allyesconfig (attached as .config)
compiler: gcc-9 (Debian 9.3.0-15) 9.3.0
reproduce (this is a W=1 build):
        # https://github.com/0day-ci/linux/commit/17d14b4802b75b93f10e01153b0e395f6a638492
        git remote add linux-review https://github.com/0day-ci/linux
        git fetch --no-tags linux-review Mihai-Carabas/misc-pvpanic-split-up-generic-and-platform-dependent-code/20210212-181711
        git checkout 17d14b4802b75b93f10e01153b0e395f6a638492
        # save the attached .config to linux build tree
        make W=1 ARCH=i386 

If you fix the issue, kindly add following tag as appropriate
Reported-by: kernel test robot <lkp@intel.com>

All warnings (new ones prefixed by >>):

>> drivers/misc/pvpanic/pvpanic-common.c:47:6: warning: no previous prototype for 'pvpanic_probe' [-Wmissing-prototypes]
      47 | void pvpanic_probe(void __iomem *pbase)
         |      ^~~~~~~~~~~~~
>> drivers/misc/pvpanic/pvpanic-common.c:54:6: warning: no previous prototype for 'pvpanic_remove' [-Wmissing-prototypes]
      54 | void pvpanic_remove(void)
         |      ^~~~~~~~~~~~~~


vim +/pvpanic_probe +47 drivers/misc/pvpanic/pvpanic-common.c

    46	
  > 47	void pvpanic_probe(void __iomem *pbase)
    48	{
    49		base = pbase;
    50		atomic_notifier_chain_register(&panic_notifier_list,
    51					       &pvpanic_panic_nb);
    52	}
    53	
  > 54	void pvpanic_remove(void)

---
0-DAY CI Kernel Test Service, Intel Corporation
https://lists.01.org/hyperkitty/list/kbuild-all(a)lists.01.org

[-- Attachment #2: config.gz --]
[-- Type: application/gzip, Size: 64183 bytes --]