[PATCH v4 0/2] misc: pvpanic: introduce capability & event attribute

[PATCH v4 0/2] misc: pvpanic: introduce capability & event attribute

[zhenwei pi]

v3 -> v4:
Use event sysfs attribute instead of module parameter.
Use driver dev_groups instead of creating files by sysfs_* API.

v2 -> v3:
Seperate the function to 2 parts:
    1, use sysfs to expose device capability.
    2, add a module parameter to set limitation by user.

v1 -> v2:
Remove device info log, use module parameter to expose capability.

v1:
The guest sides determines pvpanic capability by RDPT, before kicking
host side, check the event is supported or not.

zhenwei pi (2):
  misc: pvpanic: introduce device capability
  misc: pvpanic: introduce module parameter 'events'

 .../ABI/testing/sysfs-bus-pci-devices-pvpanic | 14 +++++
 drivers/misc/pvpanic.c                        | 58 +++++++++++++++++--
 2 files changed, 67 insertions(+), 5 deletions(-)
 create mode 100644 Documentation/ABI/testing/sysfs-bus-pci-devices-pvpanic

-- 
2.25.1

[PATCH v4 1/2] misc: pvpanic: introduce device capability

[zhenwei pi]

According to pvpanic spec:
https://git.qemu.org/?p=qemu.git;a=blob_plain;f=docs/specs/pvpanic.txt

The guest should determine pvpanic capability by RDPT, so initialize
capability during device probing. There is no need to register panic
notifier callback function if no events supported.

Before sending event to host side, check capability firstly.

Suggested by Greg KH, use sysfs to expose capability to user space,
also add new sysfs attribute in document.

Signed-off-by: zhenwei pi <pizhenwei@bytedance.com>
---
 .../ABI/testing/sysfs-bus-pci-devices-pvpanic |  7 ++++
 drivers/misc/pvpanic.c                        | 33 ++++++++++++++++---
 2 files changed, 35 insertions(+), 5 deletions(-)
 create mode 100644 Documentation/ABI/testing/sysfs-bus-pci-devices-pvpanic

diff --git a/Documentation/ABI/testing/sysfs-bus-pci-devices-pvpanic b/Documentation/ABI/testing/sysfs-bus-pci-devices-pvpanic
new file mode 100644
index 000000000000..57d014a2c339
--- /dev/null
+++ b/Documentation/ABI/testing/sysfs-bus-pci-devices-pvpanic
@@ -0,0 +1,7 @@
+What:		/sys/devices/pci0000:00/*/QEMU0001:00/capability
+Date:		Jan 2021
+Contact:	zhenwei pi <pizhenwei@bytedance.com>
+Description:
+		Read-only attribute. Capabilities of pvpanic device
+		which are supported by QEMU.
+		Format: %s.
diff --git a/drivers/misc/pvpanic.c b/drivers/misc/pvpanic.c
index 951b37da5e3c..c2f6a9e866b3 100644
--- a/drivers/misc/pvpanic.c
+++ b/drivers/misc/pvpanic.c
@@ -19,6 +19,22 @@
 #include <uapi/misc/pvpanic.h>
 
 static void __iomem *base;
+static unsigned int capability = PVPANIC_PANICKED | PVPANIC_CRASH_LOADED;
+
+static ssize_t capability_show(struct device *dev,
+			       struct device_attribute *attr, char *buf)
+{
+	return sprintf(buf, "%s%s",
+		capability & PVPANIC_PANICKED ? "PANICKED[BIT 0]\n" : "",
+		capability & PVPANIC_CRASH_LOADED ? "CRASH_LOADED[BIT 1]\n" : "");
+}
+static DEVICE_ATTR_RO(capability);
+
+static struct attribute *pvpanic_dev_attrs[] = {
+	&dev_attr_capability.attr,
+	NULL
+};
+ATTRIBUTE_GROUPS(pvpanic_dev);
 
 MODULE_AUTHOR("Hu Tao <hutao@cn.fujitsu.com>");
 MODULE_DESCRIPTION("pvpanic device driver");
@@ -27,7 +43,8 @@ MODULE_LICENSE("GPL");
 static void
 pvpanic_send_event(unsigned int event)
 {
-	iowrite8(event, base);
+	if (event & capability)
+		iowrite8(event, base);
 }
 
 static int
@@ -62,8 +79,12 @@ static int pvpanic_mmio_probe(struct platform_device *pdev)
 	if (IS_ERR(base))
 		return PTR_ERR(base);
 
-	atomic_notifier_chain_register(&panic_notifier_list,
-				       &pvpanic_panic_nb);
+	/* initlize capability by RDPT */
+	capability &= ioread8(base);
+
+	if (capability)
+		atomic_notifier_chain_register(&panic_notifier_list,
+					       &pvpanic_panic_nb);
 
 	return 0;
 }
@@ -71,8 +92,9 @@ static int pvpanic_mmio_probe(struct platform_device *pdev)
 static int pvpanic_mmio_remove(struct platform_device *pdev)
 {
 
-	atomic_notifier_chain_unregister(&panic_notifier_list,
-					 &pvpanic_panic_nb);
+	if (capability)
+		atomic_notifier_chain_unregister(&panic_notifier_list,
+						 &pvpanic_panic_nb);
 
 	return 0;
 }
@@ -93,6 +115,7 @@ static struct platform_driver pvpanic_mmio_driver = {
 		.name = "pvpanic-mmio",
 		.of_match_table = pvpanic_mmio_match,
 		.acpi_match_table = pvpanic_device_ids,
+		.dev_groups = pvpanic_dev_groups,
 	},
 	.probe = pvpanic_mmio_probe,
 	.remove = pvpanic_mmio_remove,
-- 
2.25.1

Re: [PATCH v4 1/2] misc: pvpanic: introduce device capability

[Greg KH]

On Sun, Jan 10, 2021 at 01:37:18PM +0800, zhenwei pi wrote:
> According to pvpanic spec:
> https://git.qemu.org/?p=qemu.git;a=blob_plain;f=docs/specs/pvpanic.txt
> 
> The guest should determine pvpanic capability by RDPT, so initialize
> capability during device probing. There is no need to register panic
> notifier callback function if no events supported.
> 
> Before sending event to host side, check capability firstly.
> 
> Suggested by Greg KH, use sysfs to expose capability to user space,
> also add new sysfs attribute in document.
> 
> Signed-off-by: zhenwei pi <pizhenwei@bytedance.com>
> ---
>  .../ABI/testing/sysfs-bus-pci-devices-pvpanic |  7 ++++
>  drivers/misc/pvpanic.c                        | 33 ++++++++++++++++---
>  2 files changed, 35 insertions(+), 5 deletions(-)
>  create mode 100644 Documentation/ABI/testing/sysfs-bus-pci-devices-pvpanic
> 
> diff --git a/Documentation/ABI/testing/sysfs-bus-pci-devices-pvpanic b/Documentation/ABI/testing/sysfs-bus-pci-devices-pvpanic
> new file mode 100644
> index 000000000000..57d014a2c339
> --- /dev/null
> +++ b/Documentation/ABI/testing/sysfs-bus-pci-devices-pvpanic
> @@ -0,0 +1,7 @@
> +What:		/sys/devices/pci0000:00/*/QEMU0001:00/capability
> +Date:		Jan 2021
> +Contact:	zhenwei pi <pizhenwei@bytedance.com>
> +Description:
> +		Read-only attribute. Capabilities of pvpanic device
> +		which are supported by QEMU.
> +		Format: %s.

Again, you are not saying exactly what the %s is, shouldn't you?  And
this does NOT match with the code below :(

> diff --git a/drivers/misc/pvpanic.c b/drivers/misc/pvpanic.c
> index 951b37da5e3c..c2f6a9e866b3 100644
> --- a/drivers/misc/pvpanic.c
> +++ b/drivers/misc/pvpanic.c
> @@ -19,6 +19,22 @@
>  #include <uapi/misc/pvpanic.h>
>  
>  static void __iomem *base;
> +static unsigned int capability = PVPANIC_PANICKED | PVPANIC_CRASH_LOADED;
> +
> +static ssize_t capability_show(struct device *dev,
> +			       struct device_attribute *attr, char *buf)
> +{
> +	return sprintf(buf, "%s%s",
> +		capability & PVPANIC_PANICKED ? "PANICKED[BIT 0]\n" : "",
> +		capability & PVPANIC_CRASH_LOADED ? "CRASH_LOADED[BIT 1]\n" : "");

Why do you have "BIT X" in here?  Why would userspace care?

The rule for sysfs is "one value per file".  You just printed out
multiple lines.  Not good, and totally not allowed.

Also please use sysfs_emit().

thanks,

greg k-h

[PATCH v4 2/2] misc: pvpanic: introduce events device attribue

[zhenwei pi]

Suggested by Paolo & Greg, add 'events' device attribute that can be
used to limit which capabilities the driver uses.

Finally, the pvpanic guest driver works by the limitation of both
device capability and user setting.

Signed-off-by: zhenwei pi <pizhenwei@bytedance.com>
---
 .../ABI/testing/sysfs-bus-pci-devices-pvpanic |  7 +++++
 drivers/misc/pvpanic.c                        | 26 ++++++++++++++++++-
 2 files changed, 32 insertions(+), 1 deletion(-)

diff --git a/Documentation/ABI/testing/sysfs-bus-pci-devices-pvpanic b/Documentation/ABI/testing/sysfs-bus-pci-devices-pvpanic
index 57d014a2c339..4750cfa0af2b 100644
--- a/Documentation/ABI/testing/sysfs-bus-pci-devices-pvpanic
+++ b/Documentation/ABI/testing/sysfs-bus-pci-devices-pvpanic
@@ -5,3 +5,10 @@ Description:
 		Read-only attribute. Capabilities of pvpanic device
 		which are supported by QEMU.
 		Format: %s.
+
+What:          /sys/devices/pci0000:00/*/QEMU0001:00/events
+Date:          Jan 2021
+Contact:       zhenwei pi <pizhenwei@bytedance.com>
+Description:
+               RW attribute. Set/get which features in-use.
+               Format: %x.
diff --git a/drivers/misc/pvpanic.c b/drivers/misc/pvpanic.c
index c2f6a9e866b3..07a008e15bd2 100644
--- a/drivers/misc/pvpanic.c
+++ b/drivers/misc/pvpanic.c
@@ -19,8 +19,31 @@
 #include <uapi/misc/pvpanic.h>
 
 static void __iomem *base;
+static unsigned int events = PVPANIC_PANICKED | PVPANIC_CRASH_LOADED;
 static unsigned int capability = PVPANIC_PANICKED | PVPANIC_CRASH_LOADED;
 
+static ssize_t events_show(struct device *dev,  struct device_attribute *attr, char *buf)
+{
+	return sprintf(buf, "%x\n", events);
+}
+
+static ssize_t events_store(struct device *dev,  struct device_attribute *attr,
+			    const char *buf, size_t count)
+{
+	unsigned int tmp;
+	int err;
+
+	err = kstrtouint(buf, 16, &tmp);
+	if (err)
+		return err;
+
+	events = tmp;
+
+	return count;
+
+}
+static DEVICE_ATTR_RW(events);
+
 static ssize_t capability_show(struct device *dev,
 			       struct device_attribute *attr, char *buf)
 {
@@ -32,6 +55,7 @@ static DEVICE_ATTR_RO(capability);
 
 static struct attribute *pvpanic_dev_attrs[] = {
 	&dev_attr_capability.attr,
+	&dev_attr_events.attr,
 	NULL
 };
 ATTRIBUTE_GROUPS(pvpanic_dev);
@@ -43,7 +67,7 @@ MODULE_LICENSE("GPL");
 static void
 pvpanic_send_event(unsigned int event)
 {
-	if (event & capability)
+	if (event & capability & events)
 		iowrite8(event, base);
 }
 
-- 
2.25.1

Re: [PATCH v4 2/2] misc: pvpanic: introduce events device attribue

[Greg KH]

On Sun, Jan 10, 2021 at 01:37:19PM +0800, zhenwei pi wrote:
> Suggested by Paolo & Greg, add 'events' device attribute that can be
> used to limit which capabilities the driver uses.
> 
> Finally, the pvpanic guest driver works by the limitation of both
> device capability and user setting.
> 
> Signed-off-by: zhenwei pi <pizhenwei@bytedance.com>
> ---
>  .../ABI/testing/sysfs-bus-pci-devices-pvpanic |  7 +++++
>  drivers/misc/pvpanic.c                        | 26 ++++++++++++++++++-
>  2 files changed, 32 insertions(+), 1 deletion(-)
> 
> diff --git a/Documentation/ABI/testing/sysfs-bus-pci-devices-pvpanic b/Documentation/ABI/testing/sysfs-bus-pci-devices-pvpanic
> index 57d014a2c339..4750cfa0af2b 100644
> --- a/Documentation/ABI/testing/sysfs-bus-pci-devices-pvpanic
> +++ b/Documentation/ABI/testing/sysfs-bus-pci-devices-pvpanic
> @@ -5,3 +5,10 @@ Description:
>  		Read-only attribute. Capabilities of pvpanic device
>  		which are supported by QEMU.
>  		Format: %s.
> +
> +What:          /sys/devices/pci0000:00/*/QEMU0001:00/events
> +Date:          Jan 2021
> +Contact:       zhenwei pi <pizhenwei@bytedance.com>
> +Description:
> +               RW attribute. Set/get which features in-use.
> +               Format: %x.

Please describe the allowed values.

> diff --git a/drivers/misc/pvpanic.c b/drivers/misc/pvpanic.c
> index c2f6a9e866b3..07a008e15bd2 100644
> --- a/drivers/misc/pvpanic.c
> +++ b/drivers/misc/pvpanic.c
> @@ -19,8 +19,31 @@
>  #include <uapi/misc/pvpanic.h>
>  
>  static void __iomem *base;
> +static unsigned int events = PVPANIC_PANICKED | PVPANIC_CRASH_LOADED;
>  static unsigned int capability = PVPANIC_PANICKED | PVPANIC_CRASH_LOADED;
>  
> +static ssize_t events_show(struct device *dev,  struct device_attribute *attr, char *buf)
> +{
> +	return sprintf(buf, "%x\n", events);
> +}
> +
> +static ssize_t events_store(struct device *dev,  struct device_attribute *attr,
> +			    const char *buf, size_t count)
> +{
> +	unsigned int tmp;
> +	int err;
> +
> +	err = kstrtouint(buf, 16, &tmp);
> +	if (err)
> +		return err;
> +
> +	events = tmp;
> +
> +	return count;
> +
> +}
> +static DEVICE_ATTR_RW(events);
> +
>  static ssize_t capability_show(struct device *dev,
>  			       struct device_attribute *attr, char *buf)
>  {
> @@ -32,6 +55,7 @@ static DEVICE_ATTR_RO(capability);
>  
>  static struct attribute *pvpanic_dev_attrs[] = {
>  	&dev_attr_capability.attr,
> +	&dev_attr_events.attr,
>  	NULL
>  };
>  ATTRIBUTE_GROUPS(pvpanic_dev);
> @@ -43,7 +67,7 @@ MODULE_LICENSE("GPL");
>  static void
>  pvpanic_send_event(unsigned int event)
>  {
> -	if (event & capability)
> +	if (event & capability & events)

That's just going to be so crazy to try to figure out, I'm glad I'm not
a user trying to configure this.

User apis are hard.

thanks,

greg k-h