[RFC V2] drivers/virt/vSMP: new driver

[RFC V2] drivers/virt/vSMP: new driver

[Czerwacki, Eial]

Introducing the vSMP guest driver which allows interaction with the vSMP control device when
running a Linux OS atop of the vSMP hypervisor.
vSMP is a resource aggregation hypervisor from SAP.

the driver comprises of 3 modules, vsmp which includes all the api needed to interact with the
control driver, vsmp_logs which allows reading logs from the hypervisor and vsmp_common_info which
allows reading generic information the hypervisor exposes, currently only the version is exposed.

Signed-off-by: Eial Czerwacki <eial.czerwacki@sap.com>

v1 -> v2:
	- fix multiple rejects pointed out by Greg KH
	- fix multiple rejects pointed out by Dan Carpenter
	- fix multiple rejects pointed out by Randy Dunlap
---
 .../ABI/stable/sysfs-driver-vsmp_common_info  |   5 +
 .../ABI/stable/sysfs-driver-vsmp_logs         |   5 +
 drivers/virt/vsmp/Kconfig                     |  14 +
 drivers/virt/vsmp/Makefile                    |   7 +
 drivers/virt/vsmp/api.c                       | 416 ++++++++++++++++++
 drivers/virt/vsmp/api.h                       |  61 +++
 drivers/virt/vsmp/common/Kconfig              |  11 +
 drivers/virt/vsmp/common/Makefile             |   7 +
 drivers/virt/vsmp/common/common.c             |  66 +++
 drivers/virt/vsmp/common/common.h             |  27 ++
 drivers/virt/vsmp/common/version.c            | 117 +++++
 drivers/virt/vsmp/logs/Kconfig                |  10 +
 drivers/virt/vsmp/logs/Makefile               |   7 +
 drivers/virt/vsmp/logs/active_logs.c          | 121 +++++
 drivers/virt/vsmp/registers.h                 |  16 +
 15 files changed, 890 insertions(+)
 create mode 100644 Documentation/ABI/stable/sysfs-driver-vsmp_common_info
 create mode 100644 Documentation/ABI/stable/sysfs-driver-vsmp_logs
 create mode 100644 drivers/virt/vsmp/Kconfig
 create mode 100644 drivers/virt/vsmp/Makefile
 create mode 100644 drivers/virt/vsmp/api.c
 create mode 100644 drivers/virt/vsmp/api.h
 create mode 100644 drivers/virt/vsmp/common/Kconfig
 create mode 100644 drivers/virt/vsmp/common/Makefile
 create mode 100644 drivers/virt/vsmp/common/common.c
 create mode 100644 drivers/virt/vsmp/common/common.h
 create mode 100644 drivers/virt/vsmp/common/version.c
 create mode 100644 drivers/virt/vsmp/logs/Kconfig
 create mode 100644 drivers/virt/vsmp/logs/Makefile
 create mode 100644 drivers/virt/vsmp/logs/active_logs.c
 create mode 100644 drivers/virt/vsmp/registers.h

diff --git a/Documentation/ABI/stable/sysfs-driver-vsmp_common_info b/Documentation/ABI/stable/sysfs-driver-vsmp_common_info
new file mode 100644
index 000000000000..a8a6afe417f3
--- /dev/null
+++ b/Documentation/ABI/stable/sysfs-driver-vsmp_common_info
@@ -0,0 +1,5 @@
+What:           /sys/hypervisor/vsmp/version
+Date:           Apr 2022
+Contact:        Eial Czerwacki <eial.czerwacki@sap.com>
+		linux-vsmp@sap.com
+Description:    Shows the full version of the vSMP hypervisor
diff --git a/Documentation/ABI/stable/sysfs-driver-vsmp_logs b/Documentation/ABI/stable/sysfs-driver-vsmp_logs
new file mode 100644
index 000000000000..a863898f5356
--- /dev/null
+++ b/Documentation/ABI/stable/sysfs-driver-vsmp_logs
@@ -0,0 +1,5 @@
+What:           /sys/hypervisor/vsmp/logs
+Date:           Apr 2022
+Contact:        Eial Czerwacki <eial.czerwacki@sap.com>
+		linux-vsmp@sap.com
+Description:    Dumps the logs of the current session from the vSMP hypervisor
diff --git a/drivers/virt/vsmp/Kconfig b/drivers/virt/vsmp/Kconfig
new file mode 100644
index 000000000000..e75879435769
--- /dev/null
+++ b/drivers/virt/vsmp/Kconfig
@@ -0,0 +1,14 @@
+# SPDX-License-Identifier: GPL-2.0-only
+menuconfig VSMP
+	tristate "vSMP Guest Support"
+	depends on SYS_HYPERVISOR && X86_64 && PCI
+	help
+	  Support for vSMP Guest Driver.
+
+	  This driver allows information gathering of data from the vSMP hypervisor when
+	  running on top of a vSMP-based hypervisor.
+
+	  If unsure, say no.
+
+source "drivers/virt/vsmp/common/Kconfig"
+source "drivers/virt/vsmp/logs/Kconfig"
diff --git a/drivers/virt/vsmp/Makefile b/drivers/virt/vsmp/Makefile
new file mode 100644
index 000000000000..88625cd3707d
--- /dev/null
+++ b/drivers/virt/vsmp/Makefile
@@ -0,0 +1,7 @@
+# SPDX-License-Identifier: GPL-2.0
+#
+# Makefile for vSMP Guest drivers.
+#
+
+obj-$(CONFIG_VSMP)	+= vsmp.o common/ logs/
+vsmp-y			:= api.o
diff --git a/drivers/virt/vsmp/api.c b/drivers/virt/vsmp/api.c
new file mode 100644
index 000000000000..57c345eaaa0c
--- /dev/null
+++ b/drivers/virt/vsmp/api.c
@@ -0,0 +1,416 @@
+/* SPDX-License-Identifier: GPL-2.0 */
+/*
+ * vSMP api
+ * Copyright (C) 2022 SAP.
+ */
+
+#include <linux/init.h>
+#include <linux/module.h>
+#include <linux/moduleparam.h>
+#include <linux/pci_regs.h>
+#include <linux/pci.h>
+#include <linux/kobject.h>
+#include <linux/kernel.h>
+
+#include <asm/io.h>
+
+#include "api.h"
+
+#define DEVICE_NAME "vsmp"
+#define DRIVER_LICENSE "GPL v2"
+#define DRIVER_AUTHOR "Eial Czerwacki <eial.czerwacki@sap.com>"
+#define DRIVER_DESC "vSMP api driver"
+#define DRIVER_VERSION "0.1"
+
+#define PCI_DEVICE_ID_SAP_FLX_VSMP_CTL 0x1011
+#define VSMP_CTL_MAX_PCI_BARS 2
+
+/* modules info */
+MODULE_LICENSE(DRIVER_LICENSE);
+MODULE_AUTHOR(DRIVER_AUTHOR);
+MODULE_DESCRIPTION(DRIVER_DESC);
+MODULE_VERSION(DRIVER_VERSION);
+
+static unsigned int vsmp_bus;
+static unsigned short vsmp_dev = 0x1f;
+static unsigned char vsmp_func;
+static bool vsmp_bdf_static = false;
+static char *vsmp_ctrl_pci_addr = "";
+module_param(vsmp_ctrl_pci_addr, charp, 0660);
+
+/* defines */
+// copied from arch/x86/pci/direct.c
+#define PCI_CONF1_ADDRESS(bus, devfn, reg) (0x80000000 | ((reg & 0xF00) << 16) | (bus << 16) \
+	   | (devfn << 8) | (reg & 0xFC))
+
+/* global vars */
+void __iomem *cfg_addr;
+static struct kobject *vsmp_sysfs_kobj;
+static struct pci_dev *vsmp_dev_obj;
+
+static const struct pci_device_id vsmp_pci_tbl[] = {
+	{ PCI_VDEVICE(SCALEMP, PCI_DEVICE_ID_SAP_FLX_VSMP_CTL), 0, },
+	{ 0, },			/* terminate list */
+};
+
+static struct {
+	uint64_t start;
+	uint64_t len;
+} mapped_bars[VSMP_CTL_MAX_PCI_BARS] = { 0 };
+
+/* internal functions */
+
+/*
+ * parses the pci address of a given address where the vSMP
+ * pci device is found
+ */
+static void parse_vsmp_ctrl_pci_addr(void)
+{
+	unsigned int bus;
+	unsigned short dev;
+	unsigned char func;
+
+	if (!strlen(vsmp_ctrl_pci_addr))
+		return;
+
+	if (sscanf(vsmp_ctrl_pci_addr, "%04x:%02hx.%1hhx", &bus, &dev, &func) < 3)
+		return;
+
+	vsmp_bus = bus;
+	vsmp_dev = dev;
+	vsmp_func = func;
+	vsmp_bdf_static = true;
+}
+
+/*
+ * queries as specific device in order to determine if it is a vSMP PCI device
+ */
+static void query_pci_bus_for_vsmp_directly(void)
+					    
+{
+	unsigned int devfn = PCI_DEVFN(vsmp_dev, vsmp_func);
+	printk(KERN_INFO
+	       "vSMP pci controller not found in devices tree, trying directly at %x:%x.%x...\n",
+	       vsmp_bus, vsmp_dev, vsmp_func);
+
+	outl(PCI_CONF1_ADDRESS(vsmp_bus, devfn, PCI_BASE_ADDRESS_0), 0xcf8);
+	mapped_bars[0].start = inl(0xcfc);
+
+	// define a knwon value which might be enough and backwars compataible
+	mapped_bars[0].len = 0x100000;
+}
+
+/*
+ * scan all the devices on the system in order to locate the
+ * vSMP PCI device for a given dev end func is provided
+ */
+static void scan_pci_devs_list_for_static_vsmp(unsigned int static_devfn)
+{
+	struct pci_dev *pdev = NULL;
+
+	for_each_pci_dev(pdev) {
+		if ((pdev->bus->number == vsmp_bus) &&
+		    (pdev->devfn == static_devfn)) {
+			vsmp_dev_obj = pdev;
+			break;
+		}
+	}
+}
+
+/*
+ * open the cfg address space of the vSDP device
+ */
+static int open_cfg_addr(struct pci_dev *pdev)
+{
+	unsigned int static_devfn = PCI_DEVFN(vsmp_dev, vsmp_func);
+	int i;
+
+	if (vsmp_bdf_static) {
+		scan_pci_devs_list_for_static_vsmp(static_devfn);
+		if (!vsmp_dev_obj) {
+			printk(KERN_INFO "vSMP pci controller not found, exiting.\n");
+			return -ENODEV;
+		}
+	} else
+		vsmp_dev_obj = pdev;
+
+	for (i = 0; i < ARRAY_SIZE(mapped_bars); i++) {
+		mapped_bars[i].start = pci_resource_start(vsmp_dev_obj, i);
+		mapped_bars[i].len = pci_resource_len(vsmp_dev_obj, i);
+	}
+
+	if (!mapped_bars[0].start)
+		query_pci_bus_for_vsmp_directly();
+
+	if (!mapped_bars[0].len) {
+		printk(KERN_INFO "vSMP pci controller not found, exiting.\n");
+		return -EINVAL;
+	}
+
+	printk(KERN_INFO "mapping bar 0: [0x%llx,0x%llx]\n",
+	       mapped_bars[0].start, mapped_bars[0].start + mapped_bars[0].len);
+	cfg_addr = ioremap(mapped_bars[0].start, mapped_bars[0].len);
+
+	if (!cfg_addr) {
+		printk(KERN_ERR
+		       "failed to map vSMP pci controller at %x:%x.%x, exiting.\n",
+		       vsmp_bus, vsmp_dev, vsmp_func);
+		return -ENODEV;
+	}
+
+	return 0;
+}
+
+/* exported functions */
+
+/*
+ * read a value from a specific register in the vSMP's device config space
+ */
+u64 vsmp_read_reg_from_cfg(u64 reg, enum reg_size_type type)
+{
+	u64 ret_val;
+
+	switch (type) {
+	case VSMP_CTL_REG_SIZE_8BIT:
+		ret_val = readb(cfg_addr + reg);
+		break;
+
+	case VSMP_CTL_REG_SIZE_16BIT:
+		ret_val = readw(cfg_addr + reg);
+		break;
+
+	case VSMP_CTL_REG_SIZE_32BIT:
+		ret_val = readl(cfg_addr + reg);
+		break;
+
+	case VSMP_CTL_REG_SIZE_64BIT:
+		ret_val = readq(cfg_addr + reg);
+		break;
+
+	default:
+		printk(KERN_ERR "unsupported reg size type %d.\n", type);
+		ret_val = (u64) -EINVAL;
+	}
+
+	dev_dbg(&vsmp_dev_obj->dev, "%s: read 0x%llx from reg 0x%llx of %d bits\n",
+		__func__, ret_val, reg, (type + 1) * 8);
+	return ret_val;
+}
+EXPORT_SYMBOL_GPL(vsmp_read_reg_from_cfg);
+
+/*
+ * write a value to a specific register in the vSMP's device config space
+ */
+void vsmp_write_reg_to_cfg(u64 reg, u64 value,
+			   enum reg_size_type type)
+{
+	switch (type) {
+	case VSMP_CTL_REG_SIZE_8BIT:
+		writeb((u8) value, cfg_addr + reg);
+		break;
+
+	case VSMP_CTL_REG_SIZE_16BIT:
+		writew((u16) value, cfg_addr + reg);
+		break;
+
+	case VSMP_CTL_REG_SIZE_32BIT:
+		writel((u32) value, cfg_addr + reg);
+		break;
+
+	case VSMP_CTL_REG_SIZE_64BIT:
+		writeq(value, cfg_addr + reg);
+		break;
+
+	default:
+		printk(KERN_ERR "unsupported reg size type %d.\n", type);
+	}
+
+	dev_dbg(&vsmp_dev_obj->dev, "%s: wrote 0x%llx to reg 0x%llx of %u bits\n",
+		__func__, value, reg, (type + 1) * 8);
+}
+EXPORT_SYMBOL_GPL(vsmp_write_reg_to_cfg);
+
+static ssize_t read_buff_from_bar_in_bytes(char *out, u8 __iomem *buff, ssize_t len)
+{
+	u32 i;
+
+	for (i = 0; i < len; i++) {
+		out[i] = ioread8(&buff[i]);
+		if (!out[i])
+			break;
+	}
+
+	return i;
+}
+
+/*
+ * read a buffer from a specific offset in a specific bar, maxed to a predefined len size-wise from the vSMP device
+ */
+ssize_t vsmp_read_buff_from_bar(u8 bar, u32 offset,
+			        char *out, ssize_t len,
+			        bool halt_on_null)
+{
+	u8 __iomem *buff;
+
+	BUG_ON(!mapped_bars[bar].start);
+	BUG_ON(ARRAY_SIZE(mapped_bars) <= bar);
+	BUG_ON((offset + len) > mapped_bars[bar].len);
+
+	buff = ioremap(mapped_bars[bar].start + offset, len);
+	if (!buff)
+		return -ENOMEM;
+
+	if (halt_on_null)
+		read_buff_from_bar_in_bytes(out, buff, len);
+	else
+		memcpy_fromio(out, buff, len);
+
+	iounmap(buff);
+
+	return 0;
+}
+EXPORT_SYMBOL_GPL(vsmp_read_buff_from_bar);
+
+/*
+ * register the vSMP sysfs object for user space interaction
+ */
+int vsmp_register_sysfs_group(const struct bin_attribute *bin_attr)
+{
+	int error = -EINVAL;
+
+	if (vsmp_sysfs_kobj && bin_attr) {
+		error = sysfs_create_bin_file(vsmp_sysfs_kobj, bin_attr);
+		if (error)
+			printk(KERN_CRIT "%s returned error %d\n", __func__,
+			       error);
+	}
+
+	return error;
+}
+EXPORT_SYMBOL_GPL(vsmp_register_sysfs_group);
+
+/*
+ * deregister the vSMP sysfs object for user space interaction
+ */
+void vsmp_deregister_sysfs_group(const struct bin_attribute *bin_attr)
+{
+	if (vsmp_sysfs_kobj && bin_attr)
+		sysfs_remove_bin_file(vsmp_sysfs_kobj, bin_attr);
+}
+EXPORT_SYMBOL_GPL(vsmp_deregister_sysfs_group);
+
+/*
+ * generic function to read from the bar
+ */
+ssize_t vsmp_generic_buff_read(struct fw_ops *op, u8 bar, u64 reg,
+			       char *buf, loff_t off, ssize_t count)
+{
+	ssize_t ret_val = 0;
+
+	if (op->buff_offset >= op->hwi_block_size) {	// perform H/W op
+		vsmp_reset_op(op);
+
+		ret_val = vsmp_read_buff_from_bar(bar, reg, op->buff, op->hwi_block_size, false);
+		if (ret_val) {
+			printk(KERN_ERR "%s operation failed\n",
+			       (op->action == FW_READ) ? "read" : "write");
+		}
+		op->buff_offset = 0;
+	}
+
+	if (ret_val)
+		return ret_val;
+
+	return memory_read_from_buffer(buf, count, &op->buff_offset, op->buff, op->hwi_block_size);;
+}
+EXPORT_SYMBOL_GPL(vsmp_generic_buff_read);
+
+void vsmp_reset_op(struct fw_ops *op)
+{
+	memset(op->buff, 0, op->hwi_block_size);
+	op->buff_offset = op->hwi_block_size;
+}
+EXPORT_SYMBOL_GPL(vsmp_reset_op);
+
+int vsmp_init_op(struct fw_ops *op, ssize_t max_size,
+		 enum vsmp_fw_action action)
+{
+	op->hwi_block_size = max_size;
+	op->action = action;
+	op->buff_offset = op->hwi_block_size;
+
+	op->buff = kzalloc(op->hwi_block_size, GFP_KERNEL);
+	if (!op->buff)
+		return -ENOMEM;
+
+	vsmp_reset_op(op);
+
+	return 0;
+}
+EXPORT_SYMBOL_GPL(vsmp_init_op);
+
+void vsmp_release_op(struct fw_ops *op)
+{
+	BUG_ON(!op->buff);
+	kfree(op->buff);
+	memset(op, 0, sizeof(*op));
+}
+EXPORT_SYMBOL_GPL(vsmp_release_op);
+
+bool vsmp_op_buffer_depleted(struct fw_ops *op)
+{
+	return op->buff_offset >= op->hwi_block_size;
+}
+EXPORT_SYMBOL_GPL(vsmp_op_buffer_depleted);
+
+/* module functions */
+
+/*
+ * init the module
+ */
+static int vsmp_pci_probe(struct pci_dev *pci, const struct pci_device_id *id)
+{
+	int ret_val;
+
+	parse_vsmp_ctrl_pci_addr();
+
+	ret_val = open_cfg_addr(pci);
+	if (ret_val) {
+		printk(KERN_ERR "failed to open cfg addr for vSMP pci controller at %x:%x.%x, exiting.\n",
+		       vsmp_bus, vsmp_dev, vsmp_func);
+		return ret_val;
+	}
+
+	vsmp_sysfs_kobj = kobject_create_and_add("vsmp", hypervisor_kobj);
+	if (!vsmp_sysfs_kobj) {
+		printk(KERN_ERR "failed to create sysfs entry for vSMP pci controller at %x:%x.%x, exiting.\n",
+		       vsmp_bus, vsmp_dev, vsmp_func);
+		return -ENODEV;
+	}
+
+	printk(KERN_INFO "%s [%02x:%02x.%x] up and running\n",
+	       DRIVER_DESC, vsmp_bus, vsmp_dev, vsmp_func);
+
+	return 0;
+}
+
+/*
+ * cleanup after the module upon exit
+ */
+static void vsmp_pci_remove(struct pci_dev *pci)
+{
+	if (cfg_addr)
+		iounmap(cfg_addr);
+
+	if (vsmp_sysfs_kobj)
+		kobject_put(vsmp_sysfs_kobj);
+}
+
+static struct pci_driver vsmp_pci_driver = {
+	.name		= DEVICE_NAME,
+	.id_table	= vsmp_pci_tbl,
+	.probe		= vsmp_pci_probe,
+	.remove		= vsmp_pci_remove,
+};
+
+module_pci_driver(vsmp_pci_driver);
diff --git a/drivers/virt/vsmp/api.h b/drivers/virt/vsmp/api.h
new file mode 100644
index 000000000000..4f5377cc9f72
--- /dev/null
+++ b/drivers/virt/vsmp/api.h
@@ -0,0 +1,61 @@
+/* SPDX-License-Identifier: GPL-2.0 */
+/*
+ * vSMP api header
+ * Copyright (C) 2022 SAP.
+ */
+
+#ifndef VSMP_API_H
+#define VSMP_API_H
+
+#include <linux/sysfs.h>
+
+enum reg_size_type {
+	VSMP_CTL_REG_SIZE_8BIT = 0,
+	VSMP_CTL_REG_SIZE_16BIT,
+	VSMP_CTL_REG_SIZE_32BIT,
+	VSMP_CTL_REG_SIZE_64BIT
+};
+
+enum vsmp_fw_action {
+	FW_READ = 0,
+	FW_WRITE = 1
+};
+
+struct fw_ops {
+	enum vsmp_fw_action action;
+	ssize_t hwi_block_size;
+	unsigned char *buff;
+	loff_t buff_offset;
+	bool in_progress;
+};
+
+#define FILE_PREM (S_IRUSR | S_IRGRP | S_IROTH)
+
+#define VSMP_ATTR_RO(_name) static struct kobj_attribute _name##_attr = __ATTR_RO(_name)
+#define vsmp_read_reg8_from_cfg(_reg_) ((u6) vsmp_read_reg_from_cfg((_reg_), VSMP_CTL_REG_SIZE_8BIT))
+#define vsmp_read_reg16_from_cfg(_reg_) ((u16) vsmp_read_reg_from_cfg((_reg_), VSMP_CTL_REG_SIZE_16BIT))
+#define vsmp_read_reg32_from_cfg(_reg_) ((u32) vsmp_read_reg_from_cfg((_reg_), VSMP_CTL_REG_SIZE_32BIT))
+#define vsmp_read_reg64_from_cfg(_reg_) ((u64) vsmp_read_reg_from_cfg((_reg_), VSMP_CTL_REG_SIZE_64BIT))
+#define vsmp_write_reg8_to_cfg(_reg_, _val_) vsmp_write_reg_to_cfg((_reg_),(_val_), VSMP_CTL_REG_SIZE_8BIT)
+#define vsmp_write_reg16_to_cfg(_reg_, _val_) vsmp_write_reg_to_cfg((_reg_), (_val_), VSMP_CTL_REG_SIZE_16BIT)
+#define vsmp_write_reg32_to_cfg(_reg_, _val_) vsmp_write_reg_to_cfg((_reg_), (_val_), VSMP_CTL_REG_SIZE_32BIT)
+#define vsmp_write_reg64_to_cfg(_reg_, _val_) vsmp_write_reg_to_cfg((_reg_), (_val_), VSMP_CTL_REG_SIZE_64BIT)
+
+u64 vsmp_read_reg_from_cfg(u64 reg, enum reg_size_type type);
+void vsmp_write_reg_to_cfg(u64 reg, u64 value,
+			   enum reg_size_type type);
+ssize_t vsmp_read_buff_from_bar(u8 bar, u32 offset,
+				     char *out, ssize_t len,
+				     bool halt_on_null);
+int vsmp_register_sysfs_group(const struct bin_attribute *bin_attr);
+void vsmp_deregister_sysfs_group(const struct bin_attribute *bin_attr);
+
+ssize_t vsmp_generic_buff_read(struct fw_ops *op, u8 bar, u64 reg,
+			       char *buf, loff_t off, ssize_t count);
+void vsmp_reset_op(struct fw_ops *op);
+int vsmp_init_op(struct fw_ops *op, ssize_t max_size,
+		 enum vsmp_fw_action action);
+void vsmp_release_op(struct fw_ops *op);
+bool vsmp_op_buffer_depleted(struct fw_ops *op);
+
+#endif // VSMP_API_H
diff --git a/drivers/virt/vsmp/common/Kconfig b/drivers/virt/vsmp/common/Kconfig
new file mode 100644
index 000000000000..5da4665acd38
--- /dev/null
+++ b/drivers/virt/vsmp/common/Kconfig
@@ -0,0 +1,11 @@
+# SPDX-License-Identifier: GPL-2.0-only
+config VSMP_COMMON
+	tristate "vSMP Guest driver common Support"
+	depends on VSMP
+	help
+	  Select this feature in order to be able to retreive generic data from the vSMP
+	  hypervisor.
+	  Currently only version is supported.
+	  The data can be found under /sys/hypervisor/vsmp
+
+	  If unsure, say N.
diff --git a/drivers/virt/vsmp/common/Makefile b/drivers/virt/vsmp/common/Makefile
new file mode 100644
index 000000000000..9db2f2389f8c
--- /dev/null
+++ b/drivers/virt/vsmp/common/Makefile
@@ -0,0 +1,7 @@
+# SPDX-License-Identifier: GPL-2.0
+#
+# Makefile for vSMP Guest common feature.
+#
+
+obj-$(CONFIG_VSMP_COMMON)	+= vsmp_common_info.o 
+vsmp_common_info-y		+= common.o version.o
diff --git a/drivers/virt/vsmp/common/common.c b/drivers/virt/vsmp/common/common.c
new file mode 100644
index 000000000000..4d88948b3029
--- /dev/null
+++ b/drivers/virt/vsmp/common/common.c
@@ -0,0 +1,66 @@
+/* SPDX-License-Identifier: GPL-2.0 */
+/*
+ * vSMP common
+ * Copyright (C) 2022 SAP.
+ */
+
+#include <linux/init.h>
+#include <linux/module.h>
+#include <linux/pci_regs.h>
+#include <linux/pci.h>
+#include <linux/kobject.h>
+#include <linux/kernel.h>
+
+#include <asm/io.h>
+
+#include "../api.h"
+#include "common.h"
+
+#define DRIVER_LICENSE "GPL v2"
+#define DRIVER_AUTHOR "Eial Czerwacki <eial.czerwacki@sap.com>"
+#define DRIVER_DESC "vSMP common info driver"
+#define DRIVER_VERSION "0.1"
+
+/* modules info */
+MODULE_LICENSE(DRIVER_LICENSE);
+MODULE_AUTHOR(DRIVER_AUTHOR);
+MODULE_DESCRIPTION(DRIVER_DESC);
+MODULE_VERSION(DRIVER_VERSION);
+
+static struct sysfs_entry_cbs cbs_arr[] = {
+	create_entry(version),
+};
+
+/* module functions */
+
+/*
+ * init the common module
+ */
+static int __init vsmp_common_info_init(void)
+{
+	int ret_val = -0, i;
+
+	for (i = 0; (i < ARRAY_SIZE(cbs_arr) && !ret_val); i++) {
+		ret_val = cbs_arr[i].reg_cb();
+		if (ret_val) {
+			printk(KERN_ERR "failed to init sysfs entry %d (%d), exiting.\n",
+			       i, ret_val);
+		}
+	}
+
+	return ret_val;
+}
+
+/*
+ * cleanup after the module
+ */
+static void __exit vsmp_common_info_exit(void)
+{
+	int i;
+
+	for (i = 0; i < ARRAY_SIZE(cbs_arr); i++)
+		cbs_arr[i].dereg_cb();
+}
+
+module_init(vsmp_common_info_init);
+module_exit(vsmp_common_info_exit);
diff --git a/drivers/virt/vsmp/common/common.h b/drivers/virt/vsmp/common/common.h
new file mode 100644
index 000000000000..08a096628c95
--- /dev/null
+++ b/drivers/virt/vsmp/common/common.h
@@ -0,0 +1,27 @@
+/* SPDX-License-Identifier: GPL-2.0 */
+/*
+ * vSMP common header
+ * Copyright (C) 2022 SAP.
+ */
+
+#ifndef VSMP_COMMON_H
+#define VSMP_COMMON_H
+
+#define create_entry(_label_) \
+	{ \
+		.reg_cb = sysfs_register_ ## _label_ ## _cb, \
+		.dereg_cb = sysfs_deregister_ ## _label_ ## _cb, \
+	}
+
+typedef int (*sysfs_register_cb)(void);
+typedef void (*sysfs_deregister_cb)(void);
+
+struct sysfs_entry_cbs {
+	sysfs_register_cb reg_cb;
+	sysfs_deregister_cb dereg_cb;
+};
+
+int sysfs_register_version_cb(void);
+void sysfs_deregister_version_cb(void);
+
+#endif // !VSMP_COMMON_H
diff --git a/drivers/virt/vsmp/common/version.c b/drivers/virt/vsmp/common/version.c
new file mode 100644
index 000000000000..9af47085fd1c
--- /dev/null
+++ b/drivers/virt/vsmp/common/version.c
@@ -0,0 +1,117 @@
+/* SPDX-License-Identifier: GPL-2.0 */
+/*
+ * vSMP version
+ * Copyright (C) 2022 SAP.
+ */
+
+#include <linux/slab.h>
+#include <linux/kobject.h>
+
+#include "../api.h"
+#include "../registers.h"
+
+/*
+ * this is the maximal possible length of the version which is a text string
+ * the real len is usually much smaller, thus the driver uses this once to read
+ * the version string and record it's actual len.
+ * from that point and on, the actual len will be used in each call.
+ */
+#define VERSION_MAX_LEN (1 << 19)
+
+static struct fw_ops op;
+
+static ssize_t version_read(struct file *filp, struct kobject *kobj,
+			    struct bin_attribute *bin_attr,
+			    char *buf, loff_t off, size_t count)
+{
+	s64 reg_val = vsmp_read_reg32_from_cfg(VSMP_VERSION_REG);
+	ssize_t ret_val;
+
+	if (reg_val < 0) {
+		printk(KERN_ERR "failed to value of reg 0x%x\n", VSMP_VERSION_REG);
+		return 0;
+	}
+
+	ret_val = vsmp_generic_buff_read(&op, 0, reg_val, buf, off, count);
+	if (ret_val < 0) {
+		printk(KERN_ERR "failed to read version (%ld)\n", ret_val);
+		return 0;
+	}
+
+	buf[ret_val++] = '\n';
+
+	return ret_val;
+}
+
+struct bin_attribute version_raw_attr = __BIN_ATTR(version, FILE_PREM, version_read, NULL, VERSION_MAX_LEN);
+
+/*
+ * retreive str in order to determine the proper length.
+ * this is the best way to maintain backwards compatibility with all
+ * vSMP versions.
+ */
+static ssize_t get_version_len(void)
+{
+	ssize_t ret_val = 0;
+	u64 reg_val;
+	char *version_str = kzalloc(VERSION_MAX_LEN, GFP_ATOMIC | __GFP_NOWARN);
+
+	if (!version_str) {
+		printk(KERN_ERR "failed to allocate buffer\n");
+		return -ENOMEM;
+	}
+
+	reg_val = vsmp_read_reg32_from_cfg(VSMP_VERSION_REG);
+	if (reg_val < 0) {
+		printk(KERN_ERR "failed to value of reg 0x%x\n", VSMP_VERSION_REG);
+		return 0;
+	}
+
+	memset(version_str, 0, VERSION_MAX_LEN);
+	ret_val = vsmp_read_buff_from_bar(0, reg_val, version_str, VERSION_MAX_LEN, true);
+	if (ret_val) {
+		kfree(version_str);
+		printk(KERN_ERR "failed to read buffer from bar\n");
+		return ret_val;
+	}
+	kfree(version_str);
+
+	return strlen(version_str);
+}
+
+/*
+ * register the version sysfs entry
+ */
+int sysfs_register_version_cb(void)
+{
+	ssize_t len = get_version_len();
+	int ret_val;
+
+	if (len < 1) {
+		printk(KERN_ERR "failed to init vSMP version module\n");
+		return len;
+	}
+	version_raw_attr.size = len;
+
+	if (vsmp_init_op(&op, version_raw_attr.size, FW_READ)) {
+		printk(KERN_ERR "failed to init vSMP version op\n");
+		return -ENODEV;
+	}
+
+	ret_val = vsmp_register_sysfs_group(&version_raw_attr);
+	if (ret_val) {
+		printk(KERN_ERR "failed to init vSMP version support\n");
+		vsmp_release_op(&op);
+	}
+
+	return ret_val;
+}
+
+/*
+ * deregister the version sysfs entry
+ */
+void sysfs_deregister_version_cb(void)
+{
+	vsmp_deregister_sysfs_group(&version_raw_attr);
+	vsmp_release_op(&op);
+}
diff --git a/drivers/virt/vsmp/logs/Kconfig b/drivers/virt/vsmp/logs/Kconfig
new file mode 100644
index 000000000000..02949c351b86
--- /dev/null
+++ b/drivers/virt/vsmp/logs/Kconfig
@@ -0,0 +1,10 @@
+# SPDX-License-Identifier: GPL-2.0-only
+config VSMP_LOGS
+	tristate "vSMP Guest driver logs Support"
+	depends on VSMP
+	help
+	  Select this feature in order to be able to retreive logs from the vSMP
+	  hypervisor.
+	  The data can be found under /sys/hypervisor/logs
+
+	  If unsure, say N.
diff --git a/drivers/virt/vsmp/logs/Makefile b/drivers/virt/vsmp/logs/Makefile
new file mode 100644
index 000000000000..8170cf639b92
--- /dev/null
+++ b/drivers/virt/vsmp/logs/Makefile
@@ -0,0 +1,7 @@
+# SPDX-License-Identifier: GPL-2.0
+#
+# Makefile for vSMP Guest common feature.
+#
+
+obj-$(CONFIG_VSMP_LOGS)	+= vsmp_logs.o 
+vsmp_logs-y		+= active_logs.o
diff --git a/drivers/virt/vsmp/logs/active_logs.c b/drivers/virt/vsmp/logs/active_logs.c
new file mode 100644
index 000000000000..0b8e9b60fb9b
--- /dev/null
+++ b/drivers/virt/vsmp/logs/active_logs.c
@@ -0,0 +1,121 @@
+/* SPDX-License-Identifier: GPL-2.0 */
+/*
+ * vSMP logs
+ * Copyright (C) 2022 SAP.
+ */
+
+#include <linux/init.h>
+#include <linux/module.h>
+#include <linux/pci_regs.h>
+#include <linux/pci.h>
+#include <linux/kobject.h>
+#include <linux/kernel.h>
+#include <linux/mutex.h>
+
+#include <asm/io.h>
+
+#include "../api.h"
+#include "../registers.h"
+
+#define DRIVER_LICENSE "GPL v2"
+#define DRIVER_AUTHOR "Eial Czerwacki <eial.czerwacki@sap.com>"
+#define DRIVER_DESC "vSMP active logs driver"
+#define DRIVER_VERSION "0.1"
+
+#define LOGS_MASK (1 << 0)
+
+/* modules info */
+MODULE_LICENSE(DRIVER_LICENSE);
+MODULE_AUTHOR(DRIVER_AUTHOR);
+MODULE_DESCRIPTION(DRIVER_DESC);
+MODULE_VERSION(DRIVER_VERSION);
+
+static struct fw_ops op;
+static unsigned active_log_start;
+DEFINE_MUTEX(active_log_mutex);
+
+/* module functions */
+static ssize_t active_log_read(struct file *filp, struct kobject *kobj,
+			       struct bin_attribute *bin_attr,
+			       char *buf, loff_t off, size_t count)
+{
+	ssize_t ret_val = 0;
+
+	if (!op.in_progress) {
+		ret_val = mutex_lock_interruptible(&active_log_mutex);
+		if (ret_val) {
+			printk(KERN_ERR
+			       "error in atemmpt to lock mutex (%lx)\n",
+			       ret_val);
+			return 0;
+		}
+
+		vsmp_write_reg16_to_cfg(VSMP_LOGS_CTRL_REG,
+					vsmp_read_reg16_from_cfg
+					(VSMP_LOGS_CTRL_REG) | LOGS_MASK);
+		op.in_progress = true;
+		vsmp_write_reg64_to_cfg(VSMP_LOGS_STATUS_REG, 0);
+	}
+
+	if (vsmp_op_buffer_depleted(&op)) {
+		if (!(vsmp_read_reg16_from_cfg(VSMP_LOGS_CTRL_REG) & LOGS_MASK)) {
+			vsmp_reset_op(&op);
+			op.in_progress = 0;
+			mutex_unlock(&active_log_mutex);
+			return ret_val;
+		}
+	}
+
+	ret_val = vsmp_generic_buff_read(&op, 1, active_log_start, buf, off, count);
+	if (ret_val < 0) {
+		printk(KERN_ERR "error reading from buffer\n");
+		mutex_unlock(&active_log_mutex);
+		return 0;
+	}
+
+	if (vsmp_op_buffer_depleted(&op)) {
+		vsmp_write_reg64_to_cfg(VSMP_LOGS_STATUS_REG,
+					~0LL & ~(1LL << 0));
+	}
+
+	return count;
+}
+
+static struct bin_attribute active_log_raw_attr = __BIN_ATTR(log, FILE_PREM, active_log_read, NULL, 0);
+
+/*
+ * init the log module
+ */
+static int __init vsmp_active_logs_init(void)
+{
+	int ret_val;
+
+	if (!vsmp_init_op(&op, vsmp_read_reg32_from_cfg(VSMP_LOGS_LEN_REG),
+							 FW_READ)) {
+		printk(KERN_ERR "failed to init vSMP log op\n");
+		return -ENODEV;
+	}
+
+	active_log_start = vsmp_read_reg32_from_cfg(VSMP_LOGS_START_REG);
+	mutex_init(&active_log_mutex);
+
+	ret_val = vsmp_register_sysfs_group(&active_log_raw_attr);
+	if (ret_val) {
+		printk(KERN_ERR "failed to init vSMP active logs support\n");
+		vsmp_release_op(&op);
+	}
+
+	return ret_val;
+}
+
+/*
+ * cleanup after the module
+ */
+static void __exit vsmp_active_logs_exit(void)
+{
+	vsmp_deregister_sysfs_group(&active_log_raw_attr);
+	vsmp_release_op(&op);
+}
+
+module_init(vsmp_active_logs_init);
+module_exit(vsmp_active_logs_exit);
diff --git a/drivers/virt/vsmp/registers.h b/drivers/virt/vsmp/registers.h
new file mode 100644
index 000000000000..6cb75dfcccc6
--- /dev/null
+++ b/drivers/virt/vsmp/registers.h
@@ -0,0 +1,16 @@
+/* SPDX-License-Identifier: GPL-2.0 */
+/*
+ * vSMP offsets
+ * Copyright (C) 2022 SAP.
+ */
+
+#ifndef VSMP_REGSITERS_H
+#define VSMP_REGSITERS_H
+
+#define VSMP_LOGS_CTRL_REG 0x22
+#define VSMP_LOGS_START_REG 0x30
+#define VSMP_LOGS_LEN_REG 0x34
+#define VSMP_LOGS_STATUS_REG 0x38
+#define VSMP_VERSION_REG 0x0c
+
+#endif // VSMP_REGSITERS_H
-- 
2.25.1

Re: [RFC V2] drivers/virt/vSMP: new driver

[Greg KH]

On Sun, Apr 24, 2022 at 11:44:33AM +0000, Czerwacki, Eial wrote:
> Introducing the vSMP guest driver which allows interaction with the vSMP control device when
> running a Linux OS atop of the vSMP hypervisor.
> vSMP is a resource aggregation hypervisor from SAP.

Please properly wrap your lines at 72 columns like git asks you to.

> 
> the driver comprises of 3 modules, vsmp which includes all the api needed to interact with the
> control driver, vsmp_logs which allows reading logs from the hypervisor and vsmp_common_info which
> allows reading generic information the hypervisor exposes, currently only the version is exposed.


Should this be one patch per module?

> 
> Signed-off-by: Eial Czerwacki <eial.czerwacki@sap.com>
> 
> v1 -> v2:
> 	- fix multiple rejects pointed out by Greg KH
> 	- fix multiple rejects pointed out by Dan Carpenter
> 	- fix multiple rejects pointed out by Randy Dunlap

You you didn't cc: any of us?

> ---
>  .../ABI/stable/sysfs-driver-vsmp_common_info  |   5 +
>  .../ABI/stable/sysfs-driver-vsmp_logs         |   5 +
>  drivers/virt/vsmp/Kconfig                     |  14 +
>  drivers/virt/vsmp/Makefile                    |   7 +
>  drivers/virt/vsmp/api.c                       | 416 ++++++++++++++++++
>  drivers/virt/vsmp/api.h                       |  61 +++
>  drivers/virt/vsmp/common/Kconfig              |  11 +
>  drivers/virt/vsmp/common/Makefile             |   7 +
>  drivers/virt/vsmp/common/common.c             |  66 +++
>  drivers/virt/vsmp/common/common.h             |  27 ++
>  drivers/virt/vsmp/common/version.c            | 117 +++++
>  drivers/virt/vsmp/logs/Kconfig                |  10 +
>  drivers/virt/vsmp/logs/Makefile               |   7 +
>  drivers/virt/vsmp/logs/active_logs.c          | 121 +++++
>  drivers/virt/vsmp/registers.h                 |  16 +

linux-staging@ is not the proper list for this to go to.  Please run
scripts/get_maintainer.pl on your patch and resend it.

>  15 files changed, 890 insertions(+)
>  create mode 100644 Documentation/ABI/stable/sysfs-driver-vsmp_common_info
>  create mode 100644 Documentation/ABI/stable/sysfs-driver-vsmp_logs
>  create mode 100644 drivers/virt/vsmp/Kconfig
>  create mode 100644 drivers/virt/vsmp/Makefile
>  create mode 100644 drivers/virt/vsmp/api.c
>  create mode 100644 drivers/virt/vsmp/api.h
>  create mode 100644 drivers/virt/vsmp/common/Kconfig
>  create mode 100644 drivers/virt/vsmp/common/Makefile
>  create mode 100644 drivers/virt/vsmp/common/common.c
>  create mode 100644 drivers/virt/vsmp/common/common.h
>  create mode 100644 drivers/virt/vsmp/common/version.c
>  create mode 100644 drivers/virt/vsmp/logs/Kconfig
>  create mode 100644 drivers/virt/vsmp/logs/Makefile
>  create mode 100644 drivers/virt/vsmp/logs/active_logs.c
>  create mode 100644 drivers/virt/vsmp/registers.h
> 
> diff --git a/Documentation/ABI/stable/sysfs-driver-vsmp_common_info b/Documentation/ABI/stable/sysfs-driver-vsmp_common_info
> new file mode 100644
> index 000000000000..a8a6afe417f3
> --- /dev/null
> +++ b/Documentation/ABI/stable/sysfs-driver-vsmp_common_info
> @@ -0,0 +1,5 @@
> +What:           /sys/hypervisor/vsmp/version
> +Date:           Apr 2022
> +Contact:        Eial Czerwacki <eial.czerwacki@sap.com>
> +		linux-vsmp@sap.com
> +Description:    Shows the full version of the vSMP hypervisor
> diff --git a/Documentation/ABI/stable/sysfs-driver-vsmp_logs b/Documentation/ABI/stable/sysfs-driver-vsmp_logs
> new file mode 100644
> index 000000000000..a863898f5356
> --- /dev/null
> +++ b/Documentation/ABI/stable/sysfs-driver-vsmp_logs
> @@ -0,0 +1,5 @@
> +What:           /sys/hypervisor/vsmp/logs

All of the vsmp/ sysfs api files should be in the same file, no need to
spread them around.

> +Date:           Apr 2022
> +Contact:        Eial Czerwacki <eial.czerwacki@sap.com>
> +		linux-vsmp@sap.com

Why a mix of tabs and spaces?  Please just use tabs.

> +Description:    Dumps the logs of the current session from the vSMP hypervisor
> diff --git a/drivers/virt/vsmp/Kconfig b/drivers/virt/vsmp/Kconfig
> new file mode 100644
> index 000000000000..e75879435769
> --- /dev/null
> +++ b/drivers/virt/vsmp/Kconfig
> @@ -0,0 +1,14 @@
> +# SPDX-License-Identifier: GPL-2.0-only
> +menuconfig VSMP
> +	tristate "vSMP Guest Support"
> +	depends on SYS_HYPERVISOR && X86_64 && PCI
> +	help
> +	  Support for vSMP Guest Driver.
> +
> +	  This driver allows information gathering of data from the vSMP hypervisor when
> +	  running on top of a vSMP-based hypervisor.
> +
> +	  If unsure, say no.
> +
> +source "drivers/virt/vsmp/common/Kconfig"
> +source "drivers/virt/vsmp/logs/Kconfig"
> diff --git a/drivers/virt/vsmp/Makefile b/drivers/virt/vsmp/Makefile
> new file mode 100644
> index 000000000000..88625cd3707d
> --- /dev/null
> +++ b/drivers/virt/vsmp/Makefile
> @@ -0,0 +1,7 @@
> +# SPDX-License-Identifier: GPL-2.0
> +#
> +# Makefile for vSMP Guest drivers.
> +#
> +
> +obj-$(CONFIG_VSMP)	+= vsmp.o common/ logs/
> +vsmp-y			:= api.o
> diff --git a/drivers/virt/vsmp/api.c b/drivers/virt/vsmp/api.c
> new file mode 100644
> index 000000000000..57c345eaaa0c
> --- /dev/null
> +++ b/drivers/virt/vsmp/api.c
> @@ -0,0 +1,416 @@
> +/* SPDX-License-Identifier: GPL-2.0 */
> +/*
> + * vSMP api
> + * Copyright (C) 2022 SAP.
> + */
> +
> +#include <linux/init.h>
> +#include <linux/module.h>
> +#include <linux/moduleparam.h>
> +#include <linux/pci_regs.h>
> +#include <linux/pci.h>
> +#include <linux/kobject.h>
> +#include <linux/kernel.h>
> +
> +#include <asm/io.h>
> +
> +#include "api.h"
> +
> +#define DEVICE_NAME "vsmp"

KBUILD_MODNAME?

> +#define DRIVER_LICENSE "GPL v2"
> +#define DRIVER_AUTHOR "Eial Czerwacki <eial.czerwacki@sap.com>"
> +#define DRIVER_DESC "vSMP api driver"
> +#define DRIVER_VERSION "0.1"

In-kernel drivers do not need a version, please remove.

> +
> +#define PCI_DEVICE_ID_SAP_FLX_VSMP_CTL 0x1011
> +#define VSMP_CTL_MAX_PCI_BARS 2
> +
> +/* modules info */
> +MODULE_LICENSE(DRIVER_LICENSE);
> +MODULE_AUTHOR(DRIVER_AUTHOR);
> +MODULE_DESCRIPTION(DRIVER_DESC);
> +MODULE_VERSION(DRIVER_VERSION);
> +
> +static unsigned int vsmp_bus;
> +static unsigned short vsmp_dev = 0x1f;
> +static unsigned char vsmp_func;
> +static bool vsmp_bdf_static = false;
> +static char *vsmp_ctrl_pci_addr = "";
> +module_param(vsmp_ctrl_pci_addr, charp, 0660);
> +
> +/* defines */
> +// copied from arch/x86/pci/direct.c
> +#define PCI_CONF1_ADDRESS(bus, devfn, reg) (0x80000000 | ((reg & 0xF00) << 16) | (bus << 16) \
> +	   | (devfn << 8) | (reg & 0xFC))

You should never need this type of thing, why not use the normal pci
apis?

> +
> +/* global vars */
> +void __iomem *cfg_addr;

Horrible global symbol name :(

> +static struct kobject *vsmp_sysfs_kobj;
> +static struct pci_dev *vsmp_dev_obj;

static variables are not global :)

And you should not need this, again, use the proper PCI api insted.

> +
> +static const struct pci_device_id vsmp_pci_tbl[] = {
> +	{ PCI_VDEVICE(SCALEMP, PCI_DEVICE_ID_SAP_FLX_VSMP_CTL), 0, },
> +	{ 0, },			/* terminate list */
> +};
> +
> +static struct {
> +	uint64_t start;
> +	uint64_t len;

u64 is the proper kernel type, uint* is for userspace code.

> +} mapped_bars[VSMP_CTL_MAX_PCI_BARS] = { 0 };
> +
> +/* internal functions */
> +
> +/*
> + * parses the pci address of a given address where the vSMP
> + * pci device is found
> + */
> +static void parse_vsmp_ctrl_pci_addr(void)
> +{
> +	unsigned int bus;
> +	unsigned short dev;
> +	unsigned char func;
> +
> +	if (!strlen(vsmp_ctrl_pci_addr))

Do not use global data for anything, that's a huge hint that your driver
is incorrectly written.

> +		return;

No error reporting?


> +
> +	if (sscanf(vsmp_ctrl_pci_addr, "%04x:%02hx.%1hhx", &bus, &dev, &func) < 3)
> +		return;
> +
> +	vsmp_bus = bus;
> +	vsmp_dev = dev;
> +	vsmp_func = func;
> +	vsmp_bdf_static = true;
> +}
> +
> +/*
> + * queries as specific device in order to determine if it is a vSMP PCI device
> + */
> +static void query_pci_bus_for_vsmp_directly(void)
> +					    

Always run checkpatch on your patches before sending them out.

> +{
> +	unsigned int devfn = PCI_DEVFN(vsmp_dev, vsmp_func);
> +	printk(KERN_INFO
> +	       "vSMP pci controller not found in devices tree, trying directly at %x:%x.%x...\n",
> +	       vsmp_bus, vsmp_dev, vsmp_func);

Drivers use dev_* calls, not "raw" printk().  Please fix.

And when drivers work properly, they are totally quiet.

> +
> +	outl(PCI_CONF1_ADDRESS(vsmp_bus, devfn, PCI_BASE_ADDRESS_0), 0xcf8);
> +	mapped_bars[0].start = inl(0xcfc);
> +
> +	// define a knwon value which might be enough and backwars compataible
> +	mapped_bars[0].len = 0x100000;

Again, drop the global variables.

> +}
> +
> +/*
> + * scan all the devices on the system in order to locate the
> + * vSMP PCI device for a given dev end func is provided
> + */
> +static void scan_pci_devs_list_for_static_vsmp(unsigned int static_devfn)
> +{
> +	struct pci_dev *pdev = NULL;
> +
> +	for_each_pci_dev(pdev) {

No, this is not allowed, please just bind to the pci device that you
control.  Your probe() function will be called when it is present.  If
it is not present, your code will never be loaded or called at all.
It's been this way for a very long time, you should never scan the pci
bus (hint, this way of doing it is also broken, it will not work
properly even if you wanted to do it.)

Again, please move this code to be a proper pci driver and all will be
good and much much simpler.

greg k-h

Re: [RFC V2] drivers/virt/vSMP: new driver

[Czerwacki, Eial]

On Sun, Apr 24, 2022 at 11:44:33AM +0000, Czerwacki, Eial wrote:
> Introducing the vSMP guest driver which allows interaction with the vSMP control device when
> running a Linux OS atop of the vSMP hypervisor.
> vSMP is a resource aggregation hypervisor from SAP.
> [snip]
>
>Again, please move this code to be a proper pci driver and all will be
>good and much much simpler.
>
>greg k-h

will fix and send again.

Thanks,

Eial

Re: [RFC V2] drivers/virt/vSMP: new driver

[Czerwacki, Eial]

>From: Greg KH <gregkh@linuxfoundation.org>
>Sent: Sunday, April 24, 2022 16:40
>To: Czerwacki, Eial <eial.czerwacki@sap.com>
>Cc: linux-staging@lists.linux.dev <linux-staging@lists.linux.dev>; SAP vSMP Linux Maintainer <linux.vsmp@sap.com>
>Subject: Re: [RFC V2] drivers/virt/vSMP: new driver 
> 
>On Sun, Apr 24, 2022 at 11:44:33AM +0000, Czerwacki, Eial wrote:
[snip]

>> +static struct kobject *vsmp_sysfs_kobj;
>> +static struct pci_dev *vsmp_dev_obj;
>
>static variables are not global :)
>
>And you should not need this, again, use the proper PCI api insted.
>
[snip]

>
>Do not use global data for anything, that's a huge hint that your driver
>is incorrectly written.
>
[snip]

>
>greg k-h

I understand the logic behind that, however, I'm not sure I understand what PCI api I can use to replace such vars?
for example for the pci_dev struct of the device or the sysfs object which is used for the other modules
is there a doc on this I can be pointed to?
as for saving the iomapping, I assume that I should map the bar regions rather than having it mapped all the time, am I right?

Eial

Re: [RFC V2] drivers/virt/vSMP: new driver

[Greg KH]

On Tue, Apr 26, 2022 at 11:58:19AM +0000, Czerwacki, Eial wrote:
> >From: Greg KH <gregkh@linuxfoundation.org>
> >Sent: Sunday, April 24, 2022 16:40
> >To: Czerwacki, Eial <eial.czerwacki@sap.com>
> >Cc: linux-staging@lists.linux.dev <linux-staging@lists.linux.dev>; SAP vSMP Linux Maintainer <linux.vsmp@sap.com>
> >Subject: Re: [RFC V2] drivers/virt/vSMP: new driver 
> > 
> >On Sun, Apr 24, 2022 at 11:44:33AM +0000, Czerwacki, Eial wrote:
> [snip]
> 
> >> +static struct kobject *vsmp_sysfs_kobj;
> >> +static struct pci_dev *vsmp_dev_obj;
> >
> >static variables are not global :)
> >
> >And you should not need this, again, use the proper PCI api insted.
> >
> [snip]
> 
> >
> >Do not use global data for anything, that's a huge hint that your driver
> >is incorrectly written.
> >
> [snip]
> 
> >
> >greg k-h
> 
> I understand the logic behind that, however, I'm not sure I understand what PCI api I can use to replace such vars?

pci_register_driver() is all you need.

> for example for the pci_dev struct of the device or the sysfs object which is used for the other modules
> is there a doc on this I can be pointed to?

I do not understand, why would your driver ever care about any other PCI
device in the system other than the one that you are wishing to be bound
to?

> as for saving the iomapping, I assume that I should map the bar regions rather than having it mapped all the time, am I right?

I have no context here, sorry.

thanks,

greg k-h

Re: [RFC V2] drivers/virt/vSMP: new driver

[Czerwacki, Eial]

>On Tue, Apr 26, 2022 at 11:58:19AM +0000, Czerwacki, Eial wrote:
>> >From: Greg KH <gregkh@linuxfoundation.org>
>> >Sent: Sunday, April 24, 2022 16:40
>> >To: Czerwacki, Eial <eial.czerwacki@sap.com>
>> >Cc: linux-staging@lists.linux.dev <linux-staging@lists.linux.dev>; SAP vSMP Linux Maintainer <linux.vsmp@sap.com>
>> >Subject: Re: [RFC V2] drivers/virt/vSMP: new driver 
>> > 
>> >On Sun, Apr 24, 2022 at 11:44:33AM +0000, Czerwacki, Eial wrote:
>> [snip]
>> 
>> >> +static struct kobject *vsmp_sysfs_kobj;
>> >> +static struct pci_dev *vsmp_dev_obj;
>> >
>> >static variables are not global :)
>> >
>> >And you should not need this, again, use the proper PCI api insted.
>> >
>> [snip]
>> 
>> >
>> >Do not use global data for anything, that's a huge hint that your driver
>> >is incorrectly written.
>> >
>> [snip]
>> 
>> >
>> >greg k-h
>> 
>> I understand the logic behind that, however, I'm not sure I understand what PCI api I can use to replace such vars?
>
>pci_register_driver() is all you need.
thanks, I'll look into it

>
>> for example for the pci_dev struct of the device or the sysfs object which is used for the other modules
>> is there a doc on this I can be pointed to?
>
>I do not understand, why would your driver ever care about any other PCI
>device in the system other than the one that you are wishing to be bound
>to?
ahh, I understand now, the structure I want to have for the driver is incompatible which the existing framework as it assumes
that each module is for a specific device whereas my the structure I want to have is module per feature.
I'll adjust the structure of the driver to match the framework's structure

>
>> as for saving the iomapping, I assume that I should map the bar regions rather than having it mapped all the time, am I right?
>
>I have no context here, sorry.
I'll try to find some examples in the code, maybe someone did something similar

Thanks

Eial

Re: [RFC V2] drivers/virt/vSMP: new driver

[Dan Carpenter]

On Sun, Apr 24, 2022 at 11:44:33AM +0000, Czerwacki, Eial wrote:
> +u64 vsmp_read_reg_from_cfg(u64 reg, enum reg_size_type type)
> +{
> +	u64 ret_val;
> +
> +	switch (type) {
> +	case VSMP_CTL_REG_SIZE_8BIT:
> +		ret_val = readb(cfg_addr + reg);
> +		break;
> +
> +	case VSMP_CTL_REG_SIZE_16BIT:
> +		ret_val = readw(cfg_addr + reg);
> +		break;
> +
> +	case VSMP_CTL_REG_SIZE_32BIT:
> +		ret_val = readl(cfg_addr + reg);
> +		break;
> +
> +	case VSMP_CTL_REG_SIZE_64BIT:
> +		ret_val = readq(cfg_addr + reg);
> +		break;
> +
> +	default:
> +		printk(KERN_ERR "unsupported reg size type %d.\n", type);
> +		ret_val = (u64) -EINVAL;

This error code gets truncated to a u16 or whatever so it doesn't work.
It's dead code anyway, right?  Just return zero.

> +	}
> +
> +	dev_dbg(&vsmp_dev_obj->dev, "%s: read 0x%llx from reg 0x%llx of %d bits\n",
> +		__func__, ret_val, reg, (type + 1) * 8);
> +	return ret_val;
> +}
> +EXPORT_SYMBOL_GPL(vsmp_read_reg_from_cfg);

[ snip ]

> +
> +/*
> + * deregister the vSMP sysfs object for user space interaction
> + */
> +void vsmp_deregister_sysfs_group(const struct bin_attribute *bin_attr)
> +{
> +	if (vsmp_sysfs_kobj && bin_attr)
> +		sysfs_remove_bin_file(vsmp_sysfs_kobj, bin_attr);
> +}
> +EXPORT_SYMBOL_GPL(vsmp_deregister_sysfs_group);
> +
> +/*
> + * generic function to read from the bar
> + */
> +ssize_t vsmp_generic_buff_read(struct fw_ops *op, u8 bar, u64 reg,
> +			       char *buf, loff_t off, ssize_t count)
> +{
> +	ssize_t ret_val = 0;
> +
> +	if (op->buff_offset >= op->hwi_block_size) {	// perform H/W op
> +		vsmp_reset_op(op);
> +
> +		ret_val = vsmp_read_buff_from_bar(bar, reg, op->buff, op->hwi_block_size, false);
> +		if (ret_val) {
> +			printk(KERN_ERR "%s operation failed\n",
> +			       (op->action == FW_READ) ? "read" : "write");

This read vs write is weird.  We're in a read function.  Also it's
always read.

> +		}
> +		op->buff_offset = 0;
> +	}
> +
> +	if (ret_val)
> +		return ret_val;
> +
> +	return memory_read_from_buffer(buf, count, &op->buff_offset, op->buff, op->hwi_block_size);;
> +}
> +EXPORT_SYMBOL_GPL(vsmp_generic_buff_read);

[ snip ]

> +
> +/*
> + * init the common module
> + */
> +static int __init vsmp_common_info_init(void)
> +{
> +	int ret_val = -0, i;
                      ^^
Negative zero?


> +
> +	for (i = 0; (i < ARRAY_SIZE(cbs_arr) && !ret_val); i++) {
> +		ret_val = cbs_arr[i].reg_cb();
> +		if (ret_val) {
> +			printk(KERN_ERR "failed to init sysfs entry %d (%d), exiting.\n",
> +			       i, ret_val);
> +		}
> +	}
> +
> +	return ret_val;
> +}

[ snip ]

> +
> +/*
> + * this is the maximal possible length of the version which is a text string
> + * the real len is usually much smaller, thus the driver uses this once to read
> + * the version string and record it's actual len.
> + * from that point and on, the actual len will be used in each call.
> + */
> +#define VERSION_MAX_LEN (1 << 19)
> +
> +static struct fw_ops op;

Greg already complained about globals, but this one is particularly bad
because there is no locking so it will lead to corruption.

> +
> +static ssize_t version_read(struct file *filp, struct kobject *kobj,
> +			    struct bin_attribute *bin_attr,
> +			    char *buf, loff_t off, size_t count)
> +{
> +	s64 reg_val = vsmp_read_reg32_from_cfg(VSMP_VERSION_REG);
> +	ssize_t ret_val;
> +
> +	if (reg_val < 0) {
> +		printk(KERN_ERR "failed to value of reg 0x%x\n", VSMP_VERSION_REG);
> +		return 0;
> +	}
> +
> +	ret_val = vsmp_generic_buff_read(&op, 0, reg_val, buf, off, count);
> +	if (ret_val < 0) {
> +		printk(KERN_ERR "failed to read version (%ld)\n", ret_val);
> +		return 0;
> +	}
> +
> +	buf[ret_val++] = '\n';

You need to consider "count" before you print the newline.  #corruption.

> +
> +	return ret_val;
> +}
> +
> +struct bin_attribute version_raw_attr = __BIN_ATTR(version, FILE_PREM, version_read, NULL, VERSION_MAX_LEN);
> +
> +/*
> + * retreive str in order to determine the proper length.
> + * this is the best way to maintain backwards compatibility with all
> + * vSMP versions.
> + */
> +static ssize_t get_version_len(void)
> +{
> +	ssize_t ret_val = 0;

This assignment is never used.  It just disables static analysis for
uninitialized variables and invites bugs.

> +	u64 reg_val;
> +	char *version_str = kzalloc(VERSION_MAX_LEN, GFP_ATOMIC | __GFP_NOWARN);
> +
> +	if (!version_str) {
> +		printk(KERN_ERR "failed to allocate buffer\n");
> +		return -ENOMEM;
> +	}
> +
> +	reg_val = vsmp_read_reg32_from_cfg(VSMP_VERSION_REG);
> +	if (reg_val < 0) {
> +		printk(KERN_ERR "failed to value of reg 0x%x\n", VSMP_VERSION_REG);
> +		return 0;
> +	}
> +
> +	memset(version_str, 0, VERSION_MAX_LEN);
> +	ret_val = vsmp_read_buff_from_bar(0, reg_val, version_str, VERSION_MAX_LEN, true);
> +	if (ret_val) {
> +		kfree(version_str);
> +		printk(KERN_ERR "failed to read buffer from bar\n");
> +		return ret_val;
> +	}
> +	kfree(version_str);
        ^^^^^^^^^^^^^^^^^^


> +
> +	return strlen(version_str);
               ^^^^^^^^^^^^^^^^^^^
Use after free.  Try running Smatch against your code to detect these
bugs.

https://staticthinking.wordpress.com/2022/04/25/how-to-run-smatch-on-your-code/


> +}
> +
> +/*
> + * register the version sysfs entry
> + */
> +int sysfs_register_version_cb(void)
> +{
> +	ssize_t len = get_version_len();
> +	int ret_val;
> +
> +	if (len < 1) {
> +		printk(KERN_ERR "failed to init vSMP version module\n");
> +		return len;

What if len is zero?  (Please return a proper error code).


> +	}
> +	version_raw_attr.size = len;
> +
> +	if (vsmp_init_op(&op, version_raw_attr.size, FW_READ)) {
> +		printk(KERN_ERR "failed to init vSMP version op\n");
> +		return -ENODEV;
> +	}
> +
> +	ret_val = vsmp_register_sysfs_group(&version_raw_attr);
> +	if (ret_val) {
> +		printk(KERN_ERR "failed to init vSMP version support\n");
> +		vsmp_release_op(&op);
> +	}
> +
> +	return ret_val;
> +}
> +

[ snip ]

> +
> +/* module functions */
> +static ssize_t active_log_read(struct file *filp, struct kobject *kobj,
> +			       struct bin_attribute *bin_attr,
> +			       char *buf, loff_t off, size_t count)
> +{
> +	ssize_t ret_val = 0;
> +
> +	if (!op.in_progress) {
> +		ret_val = mutex_lock_interruptible(&active_log_mutex);
> +		if (ret_val) {
> +			printk(KERN_ERR
> +			       "error in atemmpt to lock mutex (%lx)\n",
> +			       ret_val);
> +			return 0;
> +		}
> +
> +		vsmp_write_reg16_to_cfg(VSMP_LOGS_CTRL_REG,
> +					vsmp_read_reg16_from_cfg
> +					(VSMP_LOGS_CTRL_REG) | LOGS_MASK);
> +		op.in_progress = true;
> +		vsmp_write_reg64_to_cfg(VSMP_LOGS_STATUS_REG, 0);
> +	}
> +
> +	if (vsmp_op_buffer_depleted(&op)) {
> +		if (!(vsmp_read_reg16_from_cfg(VSMP_LOGS_CTRL_REG) & LOGS_MASK)) {
> +			vsmp_reset_op(&op);
> +			op.in_progress = 0;
> +			mutex_unlock(&active_log_mutex);
> +			return ret_val;
> +		}
> +	}
> +
> +	ret_val = vsmp_generic_buff_read(&op, 1, active_log_start, buf, off, count);
> +	if (ret_val < 0) {
> +		printk(KERN_ERR "error reading from buffer\n");
> +		mutex_unlock(&active_log_mutex);
> +		return 0;

return ret_val;

> +	}
> +
> +	if (vsmp_op_buffer_depleted(&op)) {
> +		vsmp_write_reg64_to_cfg(VSMP_LOGS_STATUS_REG,
> +					~0LL & ~(1LL << 0));
> +	}
> +
> +	return count;

This isn't correct necessarily.  It should be something like

	return min(count, ret_val);

> +}

regards,
dan carpenter

Re: [RFC V2] drivers/virt/vSMP: new driver

[Czerwacki, Eial]

>From: Dan Carpenter <dan.carpenter@oracle.com>
>Sent: Monday, April 25, 2022 16:22
>To: Czerwacki, Eial <eial.czerwacki@sap.com>
>Cc: linux-staging@lists.linux.dev <linux-staging@lists.linux.dev>; SAP vSMP Linux Maintainer <linux.vsmp@sap.com>
>Subject: Re: [RFC V2] drivers/virt/vSMP: new driver 
> 
>On Sun, Apr 24, 2022 at 11:44:33AM +0000, Czerwacki, Eial wrote:
>[ snip ]

Thanks for the review, I'll handle them.

Eial