[RFC] staging/vSMP: new driver

[RFC] staging/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>
---
 MAINTAINERS                             |   6 +
 drivers/staging/Kconfig                 |   2 +
 drivers/staging/Makefile                |   1 +
 drivers/staging/vsmp/Kconfig            |  14 +
 drivers/staging/vsmp/Makefile           |   7 +
 drivers/staging/vsmp/api.c              | 402 ++++++++++++++++++++++++
 drivers/staging/vsmp/api.h              |  61 ++++
 drivers/staging/vsmp/common/Kconfig     |  11 +
 drivers/staging/vsmp/common/Makefile    |   7 +
 drivers/staging/vsmp/common/common.c    |  64 ++++
 drivers/staging/vsmp/common/common.h    |  27 ++
 drivers/staging/vsmp/common/version.c   |  85 +++++
 drivers/staging/vsmp/logs/Kconfig       |  10 +
 drivers/staging/vsmp/logs/Makefile      |   7 +
 drivers/staging/vsmp/logs/active_logs.c | 112 +++++++
 drivers/staging/vsmp/registers.h        |  16 +
 16 files changed, 832 insertions(+)
 create mode 100644 drivers/staging/vsmp/Kconfig
 create mode 100644 drivers/staging/vsmp/Makefile
 create mode 100644 drivers/staging/vsmp/api.c
 create mode 100644 drivers/staging/vsmp/api.h
 create mode 100644 drivers/staging/vsmp/common/Kconfig
 create mode 100644 drivers/staging/vsmp/common/Makefile
 create mode 100644 drivers/staging/vsmp/common/common.c
 create mode 100644 drivers/staging/vsmp/common/common.h
 create mode 100644 drivers/staging/vsmp/common/version.c
 create mode 100644 drivers/staging/vsmp/logs/Kconfig
 create mode 100644 drivers/staging/vsmp/logs/Makefile
 create mode 100644 drivers/staging/vsmp/logs/active_logs.c
 create mode 100644 drivers/staging/vsmp/registers.h

diff --git a/MAINTAINERS b/MAINTAINERS
index 68f21d46614c..2fc61b4d13e5 100644
--- a/MAINTAINERS
+++ b/MAINTAINERS
@@ -18381,6 +18381,12 @@ F:	Documentation/core-api/printk-formats.rst
 F:	lib/test_printf.c
 F:	lib/vsprintf.c
 
+VSMP GUEST DRIVER
+M:     Eial Czerwacki <eial.czerwacki@sap.com>
+M:     linux-vsmp@sap.com
+S:     Maintained
+F:     drivers/staging/vsmp-guest 
+
 VT1211 HARDWARE MONITOR DRIVER
 M:	Juerg Haefliger <juergh@gmail.com>
 L:	linux-hwmon@vger.kernel.org
diff --git a/drivers/staging/Kconfig b/drivers/staging/Kconfig
index 4ec5528f89fa..f61778bccb0b 100644
--- a/drivers/staging/Kconfig
+++ b/drivers/staging/Kconfig
@@ -114,6 +114,8 @@ source "drivers/staging/axis-fifo/Kconfig"
 
 source "drivers/staging/fieldbus/Kconfig"
 
+source "drivers/staging/vsmp/Kconfig"
+
 source "drivers/staging/kpc2000/Kconfig"
 
 source "drivers/staging/qlge/Kconfig"
diff --git a/drivers/staging/Makefile b/drivers/staging/Makefile
index 4d34198151b3..726e704ba8bc 100644
--- a/drivers/staging/Makefile
+++ b/drivers/staging/Makefile
@@ -47,6 +47,7 @@ obj-$(CONFIG_SOC_MT7621)	+= mt7621-dts/
 obj-$(CONFIG_STAGING_GASKET_FRAMEWORK)	+= gasket/
 obj-$(CONFIG_XIL_AXIS_FIFO)	+= axis-fifo/
 obj-$(CONFIG_FIELDBUS_DEV)     += fieldbus/
+obj-$(CONFIG_VSMP)       += vsmp/
 obj-$(CONFIG_KPC2000)		+= kpc2000/
 obj-$(CONFIG_QLGE)		+= qlge/
 obj-$(CONFIG_WFX)		+= wfx/
diff --git a/drivers/staging/vsmp/Kconfig b/drivers/staging/vsmp/Kconfig
new file mode 100644
index 000000000000..c57cfcb55033
--- /dev/null
+++ b/drivers/staging/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 drivers allows information gathering of data from the vSMP hypervisor when
+	  running ontop of a vSMP based VM.
+
+	  If unsure, say no.
+
+source "drivers/staging/vsmp/common/Kconfig"
+source "drivers/staging/vsmp/logs/Kconfig"
diff --git a/drivers/staging/vsmp/Makefile b/drivers/staging/vsmp/Makefile
new file mode 100644
index 000000000000..88625cd3707d
--- /dev/null
+++ b/drivers/staging/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/staging/vsmp/api.c b/drivers/staging/vsmp/api.c
new file mode 100644
index 000000000000..b0d4b5a990d4
--- /dev/null
+++ b/drivers/staging/vsmp/api.c
@@ -0,0 +1,402 @@
+/* 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 DRIVER_LICENSE "GPL"
+#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 = 0;
+static unsigned short vsmp_dev = 0x1f;
+static unsigned char vsmp_func = 0;
+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 = NULL;
+static struct kobject *vsmp_sysfs_kobj;
+static struct pci_dev *vsmp_dev_obj = NULL;
+
+static const struct pci_device_id 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)
+{
+	if (strlen(vsmp_ctrl_pci_addr)) {
+		unsigned int bus;
+		unsigned short dev;
+		unsigned char func;
+
+		if (sscanf(vsmp_ctrl_pci_addr, "%04x:%02hx.%1hhx", &bus, &dev,
+			   &func) == 3) {
+			vsmp_bus = bus;
+			vsmp_dev = dev;
+			vsmp_func = func;
+			vsmp_bdf_static = true;
+		}
+	}
+}
+
+/**
+ * queries as specfic device in order to determine if it is a vSMP pci device
+ */
+static void query_pci_bus_for_vsmp_directly(unsigned int default_bus,
+					    unsigned int devfn)
+{
+	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);
+}
+
+/**
+ * scan all the devices on the system in order to locate the
+ * vSMP pic device for a given dev end func is provided
+ */
+static void scan_pci_devs_list_for_vsmp(unsigned int devfn)
+{
+	int found, i;
+	struct pci_dev *pdev = NULL;
+	const struct pci_device_id *ent;
+
+	for_each_pci_dev(pdev) {
+		if (devfn != -1) {
+			if ((pdev->bus->number == vsmp_bus) &&
+			    (devfn == pdev->devfn)) {
+				found = 1;
+				break;
+			}
+		} else {
+			ent = pci_match_id(pci_tbl, pdev);
+			if (ent) {
+				found = 1;
+				vsmp_bus = pdev->bus->number;
+				vsmp_dev = PCI_SLOT(pdev->devfn);
+				vsmp_func = PCI_FUNC(pdev->devfn);
+				break;
+			}
+		}
+	}
+
+	for (i = 0; (i < ARRAY_SIZE(mapped_bars)) && found; i++) {
+		mapped_bars[i].start = pci_resource_start(pdev, i);
+		mapped_bars[i].len = pci_resource_len(pdev, i);
+	}
+	vsmp_dev_obj = pdev;
+}
+
+/**
+ * open the cfg address space of the vSDP device
+ */
+static int open_cfg_addr(void)
+{
+	unsigned int devfn = (vsmp_bdf_static) ? PCI_DEVFN(vsmp_dev, vsmp_func)
+						: -1;
+
+	scan_pci_devs_list_for_vsmp(devfn);
+	if (!mapped_bars[0].start)
+		query_pci_bus_for_vsmp_directly(0, devfn);
+
+	if (!mapped_bars[0].len) {
+		printk(KERN_INFO "vSMP pci controller not found, exiting.\n");
+		return -1;
+	}
+
+	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 -1;
+	}
+
+	return 0;
+}
+
+/* exported functions */
+
+/**
+ * read a value from a specfic register in the vSMP's device config space
+ */
+unsigned long vsmp_read_reg_from_cfg(unsigned int reg, enum reg_size_type type)
+{
+	unsigned long 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 = (unsigned long)(-1);
+	}
+
+	dev_dbg(&vsmp_dev_obj->dev, "%s: read 0x%lx from reg 0x%x 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 specfic register in the vSMP's device config space
+ */
+void vsmp_write_reg_to_cfg(unsigned int reg, unsigned long value,
+			   enum reg_size_type type)
+{
+	switch (type) {
+	case VSMP_CTL_REG_SIZE_8BIT:
+		writeb((unsigned char)value, cfg_addr + reg);
+		break;
+
+	case VSMP_CTL_REG_SIZE_16BIT:
+		writew((unsigned short)value, cfg_addr + reg);
+		break;
+
+	case VSMP_CTL_REG_SIZE_32BIT:
+		writel((unsigned int)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%x to reg 0x%x of %u bits\n",
+		__func__, reg, reg, (type + 1) * 8);
+}
+EXPORT_SYMBOL_GPL(vsmp_write_reg_to_cfg);
+
+/**
+ * reag a buffer from a specific offset in a specific bar, maxed to a predefined len size-wise from the vSMP device
+ */
+unsigned int vsmp_read_buff_from_bar(unsigned int bar, unsigned int offset,
+				     char *out, unsigned int len,
+				     bool halt_on_null)
+{
+	unsigned char __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) {
+		int i;
+		unsigned char c;
+
+		for (i = 0; i < len; i++) {
+			c = ioread8(&buff[i]);
+			if (!c)
+				break;
+			out[i] = c;
+		}
+	} 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 = -1;
+
+	if (vsmp_sysfs_kobj && bin_attr) {
+		if ((error = sysfs_create_bin_file(vsmp_sysfs_kobj, bin_attr)))
+			printk(KERN_CRIT "%s returned error %d\n", __func__,
+			       error);
+	}
+
+	return error;
+}
+EXPORT_SYMBOL_GPL(vsmp_register_sysfs_group);
+
+/**
+ * deergister 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, unsigned bar, unsigned reg,
+			       char *buf, loff_t off, size_t count)
+{
+	ssize_t ret_val = 0;
+
+	if (op->buff_offset >= op->hwi_block_size) {	// preform H/W op
+		vsmp_reset_op(op);
+
+		if ((ret_val = vsmp_read_buff_from_bar(bar, reg, op->buff,
+						       op->hwi_block_size, false))) {
+			printk(KERN_ERR "%s operation failed\n",
+			       (op->action == FW_READ) ? "read" : "write");
+		}
+		op->buff_offset = 0;
+	}
+
+	if (!ret_val) {
+		ret_val = memory_read_from_buffer(buf, count, &op->buff_offset,
+						  op->buff, op->hwi_block_size);
+	}
+
+	return ret_val;
+}
+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;
+
+	if (!(op->buff = kzalloc(op->hwi_block_size, GFP_KERNEL)))
+		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 __init vsmp_api_init(void)
+{
+	int ret_val;
+
+	parse_vsmp_ctrl_pci_addr();
+
+	if (!(ret_val = open_cfg_addr())) {
+		printk(KERN_INFO "%s [%02x:%02x.%x] up and running\n",
+		       DRIVER_DESC, vsmp_bus, vsmp_dev, vsmp_func);
+
+		if (!(vsmp_sysfs_kobj = kobject_create_and_add("vsmp",
+								hypervisor_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);
+			ret_val = -1;
+		}
+	}
+
+	return ret_val;
+}
+
+/**
+ * cleanup after the module upon exit
+ */
+static void __exit vsmp_api_exit(void)
+{
+	if (cfg_addr)
+		iounmap(cfg_addr);
+
+	if (vsmp_sysfs_kobj)
+		kobject_put(vsmp_sysfs_kobj);
+}
+
+module_init(vsmp_api_init);
+module_exit(vsmp_api_exit);
diff --git a/drivers/staging/vsmp/api.h b/drivers/staging/vsmp/api.h
new file mode 100644
index 000000000000..88d81b80bd81
--- /dev/null
+++ b/drivers/staging/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_) ((unsigned char) vsmp_read_reg_from_cfg((_reg_), VSMP_CTL_REG_SIZE_8BIT))
+#define vsmp_read_reg16_from_cfg(_reg_) ((unsigned short) vsmp_read_reg_from_cfg((_reg_), VSMP_CTL_REG_SIZE_16BIT))
+#define vsmp_read_reg32_from_cfg(_reg_) ((unsigned int) vsmp_read_reg_from_cfg((_reg_), VSMP_CTL_REG_SIZE_32BIT))
+#define vsmp_read_reg64_from_cfg(_reg_) ((unsigned long) 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)
+
+unsigned long vsmp_read_reg_from_cfg(unsigned int reg, enum reg_size_type type);
+void vsmp_write_reg_to_cfg(unsigned int reg, unsigned long value,
+			   enum reg_size_type type);
+unsigned int vsmp_read_buff_from_bar(unsigned int bar, unsigned int offset,
+				     char *out, unsigned int 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, unsigned bar, unsigned reg,
+			       char *buf, loff_t off, size_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/staging/vsmp/common/Kconfig b/drivers/staging/vsmp/common/Kconfig
new file mode 100644
index 000000000000..fe0cfeaca1bb
--- /dev/null
+++ b/drivers/staging/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 retrive generic data from the vSMP
+	  VM.
+	  currently old version is supported.
+	  the data can be found under /sys/hypervisor/vsmp
+
+	  If unsure, say N.
diff --git a/drivers/staging/vsmp/common/Makefile b/drivers/staging/vsmp/common/Makefile
new file mode 100644
index 000000000000..e6f6aa1d73df
--- /dev/null
+++ b/drivers/staging/vsmp/common/Makefile
@@ -0,0 +1,7 @@
+# SPDX-License-Identifier: GPL-2.0
+#
+# Makefile for vSMP Guest common feature.
+#
+
+obj-$(CONFIG_VSMP_COMMON)	+= common_info.o 
+common_info-y		+= common.o version.o
diff --git a/drivers/staging/vsmp/common/common.c b/drivers/staging/vsmp/common/common.c
new file mode 100644
index 000000000000..31e6551f2b7f
--- /dev/null
+++ b/drivers/staging/vsmp/common/common.c
@@ -0,0 +1,64 @@
+/* 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"
+#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++) {
+		if ((ret_val = cbs_arr[i].reg_cb()))
+			printk(KERN_ERR "failed to init sysfs (%d), exiting.\n",
+			       ret_val);
+	}
+
+	return -1 * 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/staging/vsmp/common/common.h b/drivers/staging/vsmp/common/common.h
new file mode 100644
index 000000000000..08a096628c95
--- /dev/null
+++ b/drivers/staging/vsmp/common/common.h
@@ -0,0 +1,27 @@
+/* SPDX-License-Identifier: GPL-2.0 */
+/*
+ * vSMP common header
+ * Copyright (C) 2022 SAP.
+ */
+
+#ifndef VSM_COMMON_H
+#define VSM_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 // !VSM_COMMON_H
diff --git a/drivers/staging/vsmp/common/version.c b/drivers/staging/vsmp/common/version.c
new file mode 100644
index 000000000000..35b5a7785e9a
--- /dev/null
+++ b/drivers/staging/vsmp/common/version.c
@@ -0,0 +1,85 @@
+/* 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"
+
+#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)
+{
+	ssize_t ret_val = vsmp_generic_buff_read(&op, 0,
+						 vsmp_read_reg32_from_cfg(VSMP_VERSION_REG),
+						 buf, off, count);
+
+	if ((ret_val != -ENOMEM) && ((ret_val != -EINVAL)))
+	    buf[ret_val++] = '\n';
+
+	return ret_val;
+}
+
+struct bin_attribute version_raw_attr = __BIN_ATTR(version, FILE_PREM, version_read, NULL, VERSION_MAX_LEN);
+
+/**
+ * retrive str in order to determine the proper length.
+ * this is the best way to maintain backwards compatibility with all
+ * vSMP versions.
+ */
+static int get_version_len(void)
+{
+	unsigned reg;
+	char *version_str = kzalloc(VERSION_MAX_LEN, GFP_ATOMIC | __GFP_NOWARN);
+
+	if (!version_str)
+		return -1;
+
+	reg = vsmp_read_reg32_from_cfg(VSMP_VERSION_REG);
+	memset(version_str, 0, VERSION_MAX_LEN);
+
+	if (vsmp_read_buff_from_bar(0, reg, version_str, VERSION_MAX_LEN, true)) {
+		printk(KERN_ERR "failed to read buffer from bar\n");
+		return -1;
+	}
+
+	version_raw_attr.size = strlen(version_str);
+	kfree(version_str);
+
+	return 0;
+}
+
+/**
+ * register the version sysfs entry
+ */
+int sysfs_register_version_cb(void)
+{
+	if (get_version_len()) {
+		printk(KERN_ERR "failed to init vSMP version module\n");
+		return -1;
+	}
+
+	if (!vsmp_init_op(&op, version_raw_attr.size, FW_READ)) {
+		printk(KERN_ERR "failed to init vSMP version op\n");
+		return -1;
+	}
+
+	return vsmp_register_sysfs_group(&version_raw_attr);
+}
+
+/**
+ * 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/staging/vsmp/logs/Kconfig b/drivers/staging/vsmp/logs/Kconfig
new file mode 100644
index 000000000000..d41491472bab
--- /dev/null
+++ b/drivers/staging/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 retrive logs from the vSMP
+	  VM.
+	  the data can be found under /sys/hypervisor/logs
+
+	  If unsure, say N.
diff --git a/drivers/staging/vsmp/logs/Makefile b/drivers/staging/vsmp/logs/Makefile
new file mode 100644
index 000000000000..edc679803fe6
--- /dev/null
+++ b/drivers/staging/vsmp/logs/Makefile
@@ -0,0 +1,7 @@
+# SPDX-License-Identifier: GPL-2.0
+#
+# Makefile for vSMP Guest common feature.
+#
+
+obj-$(CONFIG_VSMP_LOGS)	+= logs.o 
+logs-y		+= active_logs.o
diff --git a/drivers/staging/vsmp/logs/active_logs.c b/drivers/staging/vsmp/logs/active_logs.c
new file mode 100644
index 000000000000..91492c72cc6f
--- /dev/null
+++ b/drivers/staging/vsmp/logs/active_logs.c
@@ -0,0 +1,112 @@
+/* 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"
+#define DRIVER_AUTHOR "Eial Czerwacki <eial.czerwacki@sap.com>"
+#define DRIVER_DESC "vSMP 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 log_start;
+DEFINE_MUTEX(log_mutex);
+
+/* module functions */
+static ssize_t 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) {
+		if ((ret_val = mutex_lock_interruptible(&log_mutex))) {
+			printk(KERN_ERR
+			       "error in atemmpt to lock mutex (%lx)\n",
+			       ret_val);
+			return ret_val;
+		}
+
+		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(&log_mutex);
+			return ret_val;
+		}
+	}
+
+	ret_val = vsmp_generic_buff_read(&op, 1, log_start, buf, off, count);
+	if (!ret_val || (ret_val == -ENOMEM) || (ret_val == -EINVAL)) {
+		printk(KERN_ERR "error reading from buffer\n");
+		mutex_unlock(&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 log_raw_attr = __BIN_ATTR(log, FILE_PREM, log_read, NULL, 0);
+
+/**
+ * init the log module
+ */
+static int __init vsmp_logs_init(void)
+{
+	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 -1;
+	}
+
+	log_start = vsmp_read_reg32_from_cfg(VSMP_LOGS_START_REG);
+	mutex_init(&log_mutex);
+
+	return vsmp_register_sysfs_group(&log_raw_attr);
+}
+
+/**
+ * cleanup after the module
+ */
+static void __exit vsmp_logs_exit(void)
+{
+	vsmp_deregister_sysfs_group(&log_raw_attr);
+	vsmp_release_op(&op);
+}
+
+module_init(vsmp_logs_init);
+module_exit(vsmp_logs_exit);
diff --git a/drivers/staging/vsmp/registers.h b/drivers/staging/vsmp/registers.h
new file mode 100644
index 000000000000..6cb75dfcccc6
--- /dev/null
+++ b/drivers/staging/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] staging/vSMP: new driver

[Randy Dunlap]

Hi--

On 3/16/22 11:13, 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.
> 
> the driver comprises of 3 modules, vsmp which includes all the api needed to interact with the

  The                                                            API

> 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>
> ---
>  MAINTAINERS                             |   6 +
>  drivers/staging/Kconfig                 |   2 +
>  drivers/staging/Makefile                |   1 +
>  drivers/staging/vsmp/Kconfig            |  14 +
>  drivers/staging/vsmp/Makefile           |   7 +
>  drivers/staging/vsmp/api.c              | 402 ++++++++++++++++++++++++
>  drivers/staging/vsmp/api.h              |  61 ++++
>  drivers/staging/vsmp/common/Kconfig     |  11 +
>  drivers/staging/vsmp/common/Makefile    |   7 +
>  drivers/staging/vsmp/common/common.c    |  64 ++++
>  drivers/staging/vsmp/common/common.h    |  27 ++
>  drivers/staging/vsmp/common/version.c   |  85 +++++
>  drivers/staging/vsmp/logs/Kconfig       |  10 +
>  drivers/staging/vsmp/logs/Makefile      |   7 +
>  drivers/staging/vsmp/logs/active_logs.c | 112 +++++++
>  drivers/staging/vsmp/registers.h        |  16 +
>  16 files changed, 832 insertions(+)
>  create mode 100644 drivers/staging/vsmp/Kconfig
>  create mode 100644 drivers/staging/vsmp/Makefile
>  create mode 100644 drivers/staging/vsmp/api.c
>  create mode 100644 drivers/staging/vsmp/api.h
>  create mode 100644 drivers/staging/vsmp/common/Kconfig
>  create mode 100644 drivers/staging/vsmp/common/Makefile
>  create mode 100644 drivers/staging/vsmp/common/common.c
>  create mode 100644 drivers/staging/vsmp/common/common.h
>  create mode 100644 drivers/staging/vsmp/common/version.c
>  create mode 100644 drivers/staging/vsmp/logs/Kconfig
>  create mode 100644 drivers/staging/vsmp/logs/Makefile
>  create mode 100644 drivers/staging/vsmp/logs/active_logs.c
>  create mode 100644 drivers/staging/vsmp/registers.h
> 


> diff --git a/drivers/staging/vsmp/Kconfig b/drivers/staging/vsmp/Kconfig
> new file mode 100644
> index 000000000000..c57cfcb55033
> --- /dev/null
> +++ b/drivers/staging/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 drivers allows information gathering of data from the vSMP hypervisor when

	  This driver

> +	  running ontop of a vSMP based VM.

	          on top of a vSMP-based VM.

> +
> +	  If unsure, say no.
> +
> +source "drivers/staging/vsmp/common/Kconfig"
> +source "drivers/staging/vsmp/logs/Kconfig"



> diff --git a/drivers/staging/vsmp/api.c b/drivers/staging/vsmp/api.c
> new file mode 100644
> index 000000000000..b0d4b5a990d4
> --- /dev/null
> +++ b/drivers/staging/vsmp/api.c
> @@ -0,0 +1,402 @@
> +/* 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 DRIVER_LICENSE "GPL"
> +#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 = 0;
> +static unsigned short vsmp_dev = 0x1f;
> +static unsigned char vsmp_func = 0;

No need to init globals to 0.

> +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 = NULL;
> +static struct kobject *vsmp_sysfs_kobj;
> +static struct pci_dev *vsmp_dev_obj = NULL;

ditto.

> +
> +static const struct pci_device_id 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
> + */

In the kernel source tree, "/**" means "This is the beginning of a
kernel-doc comment," but this (above) is not in kernel-doc format,
so either use "/*" to begin the comment or use kernel-doc notation.

(Repeat in several other places...)

> +static void parse_vsmp_ctrl_pci_addr(void)
> +{
> +	if (strlen(vsmp_ctrl_pci_addr)) {
> +		unsigned int bus;
> +		unsigned short dev;
> +		unsigned char func;
> +
> +		if (sscanf(vsmp_ctrl_pci_addr, "%04x:%02hx.%1hhx", &bus, &dev,
> +			   &func) == 3) {
> +			vsmp_bus = bus;
> +			vsmp_dev = dev;
> +			vsmp_func = func;
> +			vsmp_bdf_static = true;
> +		}
> +	}
> +}
> +
> +/**
> + * queries as specfic device in order to determine if it is a vSMP pci device

                 specific                                             PCI

> + */
> +static void query_pci_bus_for_vsmp_directly(unsigned int default_bus,
> +					    unsigned int devfn)
> +{
> +	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);
> +}
> +
> +/**
> + * scan all the devices on the system in order to locate the
> + * vSMP pic device for a given dev end func is provided

           PCI
I guess.

> + */
> +static void scan_pci_devs_list_for_vsmp(unsigned int devfn)
> +{
> +	int found, i;
> +	struct pci_dev *pdev = NULL;
> +	const struct pci_device_id *ent;
> +
> +	for_each_pci_dev(pdev) {
> +		if (devfn != -1) {
> +			if ((pdev->bus->number == vsmp_bus) &&
> +			    (devfn == pdev->devfn)) {
> +				found = 1;
> +				break;
> +			}
> +		} else {
> +			ent = pci_match_id(pci_tbl, pdev);
> +			if (ent) {
> +				found = 1;
> +				vsmp_bus = pdev->bus->number;
> +				vsmp_dev = PCI_SLOT(pdev->devfn);
> +				vsmp_func = PCI_FUNC(pdev->devfn);
> +				break;
> +			}
> +		}
> +	}
> +
> +	for (i = 0; (i < ARRAY_SIZE(mapped_bars)) && found; i++) {
> +		mapped_bars[i].start = pci_resource_start(pdev, i);
> +		mapped_bars[i].len = pci_resource_len(pdev, i);
> +	}
> +	vsmp_dev_obj = pdev;
> +}
> +
> +/**
> + * open the cfg address space of the vSDP device
> + */
> +static int open_cfg_addr(void)
> +{
> +	unsigned int devfn = (vsmp_bdf_static) ? PCI_DEVFN(vsmp_dev, vsmp_func)
> +						: -1;
> +
> +	scan_pci_devs_list_for_vsmp(devfn);
> +	if (!mapped_bars[0].start)
> +		query_pci_bus_for_vsmp_directly(0, devfn);
> +
> +	if (!mapped_bars[0].len) {
> +		printk(KERN_INFO "vSMP pci controller not found, exiting.\n");
> +		return -1;
> +	}
> +
> +	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 -1;
> +	}
> +
> +	return 0;
> +}
> +
> +/* exported functions */
> +
> +/**
> + * read a value from a specfic register in the vSMP's device config space

                          specific

> + */
> +unsigned long vsmp_read_reg_from_cfg(unsigned int reg, enum reg_size_type type)
> +{
> +	unsigned long 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 = (unsigned long)(-1);
> +	}
> +
> +	dev_dbg(&vsmp_dev_obj->dev, "%s: read 0x%lx from reg 0x%x 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 specfic register in the vSMP's device config space

                         specific

> + */
> +void vsmp_write_reg_to_cfg(unsigned int reg, unsigned long value,
> +			   enum reg_size_type type)
> +{
> +	switch (type) {
> +	case VSMP_CTL_REG_SIZE_8BIT:
> +		writeb((unsigned char)value, cfg_addr + reg);
> +		break;
> +
> +	case VSMP_CTL_REG_SIZE_16BIT:
> +		writew((unsigned short)value, cfg_addr + reg);
> +		break;
> +
> +	case VSMP_CTL_REG_SIZE_32BIT:
> +		writel((unsigned int)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%x to reg 0x%x of %u bits\n",
> +		__func__, reg, reg, (type + 1) * 8);
> +}
> +EXPORT_SYMBOL_GPL(vsmp_write_reg_to_cfg);
> +
> +/**
> + * reag a buffer from a specific offset in a specific bar, maxed to a predefined len size-wise from the vSMP device

      read

> + */
> +unsigned int vsmp_read_buff_from_bar(unsigned int bar, unsigned int offset,
> +				     char *out, unsigned int len,
> +				     bool halt_on_null)
> +{
> +	unsigned char __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) {
> +		int i;
> +		unsigned char c;
> +
> +		for (i = 0; i < len; i++) {
> +			c = ioread8(&buff[i]);
> +			if (!c)
> +				break;
> +			out[i] = c;
> +		}
> +	} 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 = -1;
> +
> +	if (vsmp_sysfs_kobj && bin_attr) {
> +		if ((error = sysfs_create_bin_file(vsmp_sysfs_kobj, bin_attr)))
> +			printk(KERN_CRIT "%s returned error %d\n", __func__,
> +			       error);
> +	}
> +
> +	return error;
> +}
> +EXPORT_SYMBOL_GPL(vsmp_register_sysfs_group);
> +
> +/**
> + * deergister the vSMP sysfs object for user space interaction

      deregister
or    unregister

> + */
> +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, unsigned bar, unsigned reg,
> +			       char *buf, loff_t off, size_t count)
> +{
> +	ssize_t ret_val = 0;
> +
> +	if (op->buff_offset >= op->hwi_block_size) {	// preform H/W op

	                              is that              perform
?

> +		vsmp_reset_op(op);
> +
> +		if ((ret_val = vsmp_read_buff_from_bar(bar, reg, op->buff,
> +						       op->hwi_block_size, false))) {
> +			printk(KERN_ERR "%s operation failed\n",
> +			       (op->action == FW_READ) ? "read" : "write");
> +		}
> +		op->buff_offset = 0;
> +	}
> +
> +	if (!ret_val) {
> +		ret_val = memory_read_from_buffer(buf, count, &op->buff_offset,
> +						  op->buff, op->hwi_block_size);
> +	}
> +
> +	return ret_val;
> +}
> +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;
> +
> +	if (!(op->buff = kzalloc(op->hwi_block_size, GFP_KERNEL)))
> +		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 __init vsmp_api_init(void)
> +{
> +	int ret_val;
> +
> +	parse_vsmp_ctrl_pci_addr();
> +
> +	if (!(ret_val = open_cfg_addr())) {
> +		printk(KERN_INFO "%s [%02x:%02x.%x] up and running\n",
> +		       DRIVER_DESC, vsmp_bus, vsmp_dev, vsmp_func);
> +
> +		if (!(vsmp_sysfs_kobj = kobject_create_and_add("vsmp",
> +								hypervisor_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);
> +			ret_val = -1;
> +		}
> +	}
> +
> +	return ret_val;
> +}
> +
> +/**
> + * cleanup after the module upon exit
> + */
> +static void __exit vsmp_api_exit(void)
> +{
> +	if (cfg_addr)
> +		iounmap(cfg_addr);
> +
> +	if (vsmp_sysfs_kobj)
> +		kobject_put(vsmp_sysfs_kobj);
> +}
> +
> +module_init(vsmp_api_init);
> +module_exit(vsmp_api_exit);


> diff --git a/drivers/staging/vsmp/common/Kconfig b/drivers/staging/vsmp/common/Kconfig
> new file mode 100644
> index 000000000000..fe0cfeaca1bb
> --- /dev/null
> +++ b/drivers/staging/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 retrive generic data from the vSMP

	  Select                                     retreive

> +	  VM.
> +	  currently old version is supported.

	  Currently only

> +	  the data can be found under /sys/hypervisor/vsmp

	  The

> +
> +	  If unsure, say N.


> diff --git a/drivers/staging/vsmp/common/common.c b/drivers/staging/vsmp/common/common.c
> new file mode 100644
> index 000000000000..31e6551f2b7f
> --- /dev/null
> +++ b/drivers/staging/vsmp/common/common.c
> @@ -0,0 +1,64 @@
> +/* 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"
> +#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 */
> +
> +/**

/*

(in multiple places)

> + * 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++) {
> +		if ((ret_val = cbs_arr[i].reg_cb()))
> +			printk(KERN_ERR "failed to init sysfs (%d), exiting.\n",
> +			       ret_val);
> +	}
> +
> +	return -1 * 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/staging/vsmp/common/version.c b/drivers/staging/vsmp/common/version.c
> new file mode 100644
> index 000000000000..35b5a7785e9a
> --- /dev/null
> +++ b/drivers/staging/vsmp/common/version.c
> @@ -0,0 +1,85 @@
> +/* 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"
> +
> +#define VERSION_MAX_LEN (1 << 19)

Wow, that's a lot of version info.  What is in it?

> +
> +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)
> +{
> +	ssize_t ret_val = vsmp_generic_buff_read(&op, 0,
> +						 vsmp_read_reg32_from_cfg(VSMP_VERSION_REG),
> +						 buf, off, count);
> +
> +	if ((ret_val != -ENOMEM) && ((ret_val != -EINVAL)))
> +	    buf[ret_val++] = '\n';
> +
> +	return ret_val;
> +}
> +
> +struct bin_attribute version_raw_attr = __BIN_ATTR(version, FILE_PREM, version_read, NULL, VERSION_MAX_LEN);
> +
> +/**
> + * retrive str in order to determine the proper length.

      retreive

> + * this is the best way to maintain backwards compatibility with all
> + * vSMP versions.
> + */
> +static int get_version_len(void)
> +{
> +	unsigned reg;
> +	char *version_str = kzalloc(VERSION_MAX_LEN, GFP_ATOMIC | __GFP_NOWARN);
> +
> +	if (!version_str)
> +		return -1;
> +
> +	reg = vsmp_read_reg32_from_cfg(VSMP_VERSION_REG);
> +	memset(version_str, 0, VERSION_MAX_LEN);
> +
> +	if (vsmp_read_buff_from_bar(0, reg, version_str, VERSION_MAX_LEN, true)) {
> +		printk(KERN_ERR "failed to read buffer from bar\n");
> +		return -1;
> +	}
> +
> +	version_raw_attr.size = strlen(version_str);
> +	kfree(version_str);
> +
> +	return 0;
> +}
> +
> +/**
> + * register the version sysfs entry
> + */
> +int sysfs_register_version_cb(void)
> +{
> +	if (get_version_len()) {
> +		printk(KERN_ERR "failed to init vSMP version module\n");
> +		return -1;
> +	}
> +
> +	if (!vsmp_init_op(&op, version_raw_attr.size, FW_READ)) {
> +		printk(KERN_ERR "failed to init vSMP version op\n");
> +		return -1;
> +	}
> +
> +	return vsmp_register_sysfs_group(&version_raw_attr);
> +}
> +
> +/**
> + * 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/staging/vsmp/logs/Kconfig b/drivers/staging/vsmp/logs/Kconfig
> new file mode 100644
> index 000000000000..d41491472bab
> --- /dev/null
> +++ b/drivers/staging/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 retrive logs from the vSMP

	  Select                                     retreive

> +	  VM.
> +	  the data can be found under /sys/hypervisor/logs

	  The

> +
> +	  If unsure, say N.


> diff --git a/drivers/staging/vsmp/logs/active_logs.c b/drivers/staging/vsmp/logs/active_logs.c
> new file mode 100644
> index 000000000000..91492c72cc6f
> --- /dev/null
> +++ b/drivers/staging/vsmp/logs/active_logs.c
> @@ -0,0 +1,112 @@
> +/* 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"
> +#define DRIVER_AUTHOR "Eial Czerwacki <eial.czerwacki@sap.com>"
> +#define DRIVER_DESC "vSMP 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 log_start;
> +DEFINE_MUTEX(log_mutex);
> +
> +/* module functions */
> +static ssize_t 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) {
> +		if ((ret_val = mutex_lock_interruptible(&log_mutex))) {
> +			printk(KERN_ERR
> +			       "error in atemmpt to lock mutex (%lx)\n",
> +			       ret_val);
> +			return ret_val;
> +		}
> +
> +		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(&log_mutex);
> +			return ret_val;
> +		}
> +	}
> +
> +	ret_val = vsmp_generic_buff_read(&op, 1, log_start, buf, off, count);
> +	if (!ret_val || (ret_val == -ENOMEM) || (ret_val == -EINVAL)) {
> +		printk(KERN_ERR "error reading from buffer\n");
> +		mutex_unlock(&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 log_raw_attr = __BIN_ATTR(log, FILE_PREM, log_read, NULL, 0);
> +
> +/**

/*

(in multiple places)

> + * init the log module
> + */
> +static int __init vsmp_logs_init(void)
> +{
> +	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 -1;
> +	}
> +
> +	log_start = vsmp_read_reg32_from_cfg(VSMP_LOGS_START_REG);
> +	mutex_init(&log_mutex);
> +
> +	return vsmp_register_sysfs_group(&log_raw_attr);
> +}
> +
> +/**
> + * cleanup after the module
> + */
> +static void __exit vsmp_logs_exit(void)
> +{
> +	vsmp_deregister_sysfs_group(&log_raw_attr);
> +	vsmp_release_op(&op);
> +}
> +
> +module_init(vsmp_logs_init);
> +module_exit(vsmp_logs_exit);


-- 
~Randy

Re: [RFC] staging/vSMP: new driver

[Czerwacki, Eial]

Greetings Randy,

>From: Randy Dunlap <rdunlap@infradead.org>Sent: Wednesday, March 16, 2022 20:31To: Czerwacki, Eial <eial.czerwacki@sap.com>; linux-staging@lists.linux.dev <linux-staging@lists.linux.dev>Cc: SAP vSMP Linux Maintainer <linux.vsmp@sap.com>Subject: Re: [RFC] staging/vSMP: new driver
> 
>[You don't often get email from rdunlap@infradead.org. Learn why this is important at http://aka.ms/LearnAboutSenderIdentification.]
>
>Hi--
>
>On 3/16/22 11:13, 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.
>
>
Done, thanks for the comments.

>> diff --git a/drivers/staging/vsmp/common/version.c b/drivers/staging/vsmp/common/version.c
>> new file mode 100644
>> index 000000000000..35b5a7785e9a
>> --- /dev/null
>> +++ b/drivers/staging/vsmp/common/version.c
>> @@ -0,0 +1,85 @@
>> +/* 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"
>> +
>> +#define VERSION_MAX_LEN (1 << 19)
>
>Wow, that's a lot of version info.  What is in it?
>
>--
>~Randy

the version comes in a text mode that displays the entire system configuration under vSMP.
the current max possible len of this str is 512 MB, usually, the used length is way lower

Thanks,

Eial

Re: [RFC] staging/vSMP: new driver

[Greg KH]

On Wed, Mar 16, 2022 at 06:13:04PM +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.
> 
> 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.

Please wrap changelog text at 72 columns, like git asks you to.

Also, why did you not cc: the staging maintainer?  :(

> Signed-off-by: Eial Czerwacki <eial.czerwacki@sap.com>
> ---
>  MAINTAINERS                             |   6 +
>  drivers/staging/Kconfig                 |   2 +
>  drivers/staging/Makefile                |   1 +
>  drivers/staging/vsmp/Kconfig            |  14 +
>  drivers/staging/vsmp/Makefile           |   7 +
>  drivers/staging/vsmp/api.c              | 402 ++++++++++++++++++++++++
>  drivers/staging/vsmp/api.h              |  61 ++++
>  drivers/staging/vsmp/common/Kconfig     |  11 +
>  drivers/staging/vsmp/common/Makefile    |   7 +
>  drivers/staging/vsmp/common/common.c    |  64 ++++
>  drivers/staging/vsmp/common/common.h    |  27 ++
>  drivers/staging/vsmp/common/version.c   |  85 +++++
>  drivers/staging/vsmp/logs/Kconfig       |  10 +
>  drivers/staging/vsmp/logs/Makefile      |   7 +
>  drivers/staging/vsmp/logs/active_logs.c | 112 +++++++
>  drivers/staging/vsmp/registers.h        |  16 +

Without looking at the code, I do not see a TODO file here that lists
the tasks that need to be completed to get this out of the
drivers/staging/ directory.  This is a requirement.

Also, why is this submitted for drivers/staging/ ?  What prevents it
from being merged to the "correct" place in the kernel tree today?

thanks,

greg k-h

Re: [RFC] staging/vSMP: new driver

[Czerwacki, Eial]

Greetings Greg,

>On Wed, Mar 16, 2022 at 06:13:04PM +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.
>> 
>> 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.
>
>Please wrap changelog text at 72 columns, like git asks you to.
git didn't asked me to do so, I'll fix it in the next iteration.

>
>Also, why did you not cc: the staging maintainer?  :(
I've probably missed that when I was going over the docs, my bad.

>
>> Signed-off-by: Eial Czerwacki <eial.czerwacki@sap.com>
>> ---
>>  MAINTAINERS                             |   6 +
>>  drivers/staging/Kconfig                 |   2 +
>>  drivers/staging/Makefile                |   1 +
>>  drivers/staging/vsmp/Kconfig            |  14 +
>>  drivers/staging/vsmp/Makefile           |   7 +
>>  drivers/staging/vsmp/api.c              | 402 ++++++++++++++++++++++++
>>  drivers/staging/vsmp/api.h              |  61 ++++
>>  drivers/staging/vsmp/common/Kconfig     |  11 +
>>  drivers/staging/vsmp/common/Makefile    |   7 +
>>  drivers/staging/vsmp/common/common.c    |  64 ++++
>>  drivers/staging/vsmp/common/common.h    |  27 ++
>>  drivers/staging/vsmp/common/version.c   |  85 +++++
>>  drivers/staging/vsmp/logs/Kconfig       |  10 +
>>  drivers/staging/vsmp/logs/Makefile      |   7 +
>>  drivers/staging/vsmp/logs/active_logs.c | 112 +++++++
>>  drivers/staging/vsmp/registers.h        |  16 +
>
>Without looking at the code, I do not see a TODO file here that lists
>the tasks that need to be completed to get this out of the
>drivers/staging/ directory.  This is a requirement.
there are tasks to complete it however I'm not sure they are blockers.

>
>Also, why is this submitted for drivers/staging/ ?  What prevents it
>from being merged to the "correct" place in the kernel tree today?
afaiu, the correct order for new drivers is staging => mainline.
in addition, from past experience with the main kernel mailing list, I thought the driver will get more traction here than in the main kernel mailing list.
unless there is a specific mailing list for drivers beside the staging tree one

Thank,

Eial

Re: [RFC] staging/vSMP: new driver

[Greg KH]

On Thu, Mar 17, 2022 at 07:34:30AM +0000, Czerwacki, Eial wrote:
> Greetings Greg,
> 
> >On Wed, Mar 16, 2022 at 06:13:04PM +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.
> >> 
> >> 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.
> >
> >Please wrap changelog text at 72 columns, like git asks you to.
> git didn't asked me to do so, I'll fix it in the next iteration.
> 
> >
> >Also, why did you not cc: the staging maintainer?  :(
> I've probably missed that when I was going over the docs, my bad.
> 
> >
> >> Signed-off-by: Eial Czerwacki <eial.czerwacki@sap.com>
> >> ---
> >>  MAINTAINERS                             |   6 +
> >>  drivers/staging/Kconfig                 |   2 +
> >>  drivers/staging/Makefile                |   1 +
> >>  drivers/staging/vsmp/Kconfig            |  14 +
> >>  drivers/staging/vsmp/Makefile           |   7 +
> >>  drivers/staging/vsmp/api.c              | 402 ++++++++++++++++++++++++
> >>  drivers/staging/vsmp/api.h              |  61 ++++
> >>  drivers/staging/vsmp/common/Kconfig     |  11 +
> >>  drivers/staging/vsmp/common/Makefile    |   7 +
> >>  drivers/staging/vsmp/common/common.c    |  64 ++++
> >>  drivers/staging/vsmp/common/common.h    |  27 ++
> >>  drivers/staging/vsmp/common/version.c   |  85 +++++
> >>  drivers/staging/vsmp/logs/Kconfig       |  10 +
> >>  drivers/staging/vsmp/logs/Makefile      |   7 +
> >>  drivers/staging/vsmp/logs/active_logs.c | 112 +++++++
> >>  drivers/staging/vsmp/registers.h        |  16 +
> >
> >Without looking at the code, I do not see a TODO file here that lists
> >the tasks that need to be completed to get this out of the
> >drivers/staging/ directory.  This is a requirement.
> there are tasks to complete it however I'm not sure they are blockers.

What tasks?

> >Also, why is this submitted for drivers/staging/ ?  What prevents it
> >from being merged to the "correct" place in the kernel tree today?
> afaiu, the correct order for new drivers is staging => mainline.

No, not at all.  This is the "hard way" to get drivers merged.  I would
never recommend doing it this way for anyone as it will take more time
and effort than just doing it the correct way the first time.

> in addition, from past experience with the main kernel mailing list, I
> thought the driver will get more traction here than in the main kernel
> mailing list.  unless there is a specific mailing list for drivers
> beside the staging tree one

There are zillions of subsystem mailing lists out there.  Always target
the specific one you are interested in.  For this code, it should live
in drivers/virt/ right?  Ah, no mailing list specific for that one, but
I usually am the one that merges the code in there, so cc: me and Arnd
and lkml and we can go from there!

Also, you have a bunch of sysfs files, all of those need to be
documented in Documentation/ABI/ entries as you will see if you run
scripts/get_abi.pl with your driver loaded.

Also, rip out the old-school PCI bus scanning stuff.  That hasn't been
needed since the 2.4 kernel days :)

thanks,

greg k-h

Re: [RFC] staging/vSMP: new driver

[Czerwacki, Eial]

Greetings Greg,

>On Thu, Mar 17, 2022 at 07:34:30AM +0000, Czerwacki, Eial wrote:
>> Greetings Greg,
>> 
>> >On Wed, Mar 16, 2022 at 06:13:04PM +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.
>> >> 
>> >> 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.
>> >
>> >Please wrap changelog text at 72 columns, like git asks you to.
>> git didn't asked me to do so, I'll fix it in the next iteration.
>> 
>> >
>> >Also, why did you not cc: the staging maintainer?  :(
>> I've probably missed that when I was going over the docs, my bad.
>> 
>> >
>> >> Signed-off-by: Eial Czerwacki <eial.czerwacki@sap.com>
>> >> ---
>> >>  MAINTAINERS                             |   6 +
>> >>  drivers/staging/Kconfig                 |   2 +
>> >>  drivers/staging/Makefile                |   1 +
>> >>  drivers/staging/vsmp/Kconfig            |  14 +
>> >>  drivers/staging/vsmp/Makefile           |   7 +
>> >>  drivers/staging/vsmp/api.c              | 402 ++++++++++++++++++++++++
>> >>  drivers/staging/vsmp/api.h              |  61 ++++
>> >>  drivers/staging/vsmp/common/Kconfig     |  11 +
>> >>  drivers/staging/vsmp/common/Makefile    |   7 +
>> >>  drivers/staging/vsmp/common/common.c    |  64 ++++
>> >>  drivers/staging/vsmp/common/common.h    |  27 ++
>> >>  drivers/staging/vsmp/common/version.c   |  85 +++++
>> >>  drivers/staging/vsmp/logs/Kconfig       |  10 +
>> >>  drivers/staging/vsmp/logs/Makefile      |   7 +
>> >>  drivers/staging/vsmp/logs/active_logs.c | 112 +++++++
>> >>  drivers/staging/vsmp/registers.h        |  16 +
>> >
>> >Without looking at the code, I do not see a TODO file here that lists
>> >the tasks that need to be completed to get this out of the
>> >drivers/staging/ directory.  This is a requirement.
>> there are tasks to complete it however I'm not sure they are blockers.
>
>What tasks?
support of other information bits like stats

>
>> >Also, why is this submitted for drivers/staging/ ?  What prevents it
>> >from being merged to the "correct" place in the kernel tree today?
>> afaiu, the correct order for new drivers is staging => mainline.
>
>No, not at all.  This is the "hard way" to get drivers merged.  I would
>never recommend doing it this way for anyone as it will take more time
>and effort than just doing it the correct way the first time.
I understand, thanks for clearing it up

>
>> in addition, from past experience with the main kernel mailing list, I
>> thought the driver will get more traction here than in the main kernel
>> mailing list.  unless there is a specific mailing list for drivers
>> beside the staging tree one
>
>There are zillions of subsystem mailing lists out there.  Always target
>the specific one you are interested in.  For this code, it should live
>in drivers/virt/ right?  Ah, no mailing list specific for that one, but
>I usually am the one that merges the code in there, so cc: me and Arnd
>and lkml and we can go from there!
drivers/virt looks like the correct place, so I need to send it as [PATCH] to the lkml with Arnd and yourself in the CC of the mail?

>
>Also, you have a bunch of sysfs files, all of those need to be
>documented in Documentation/ABI/ entries as you will see if you run
>scripts/get_abi.pl with your driver loaded.
will do.

>
>Also, rip out the old-school PCI bus scanning stuff.  That hasn't been
>needed since the 2.4 kernel days :)
can you point me to a doc that states how I can retrieve the dev struct of the device in question?

Thanks,

Eial

Re: [RFC] staging/vSMP: new driver

[Greg KH]

On Thu, Mar 17, 2022 at 08:17:04AM +0000, Czerwacki, Eial wrote:
> Greetings Greg,
> 
> >On Thu, Mar 17, 2022 at 07:34:30AM +0000, Czerwacki, Eial wrote:
> >> Greetings Greg,
> >> 
> >> >On Wed, Mar 16, 2022 at 06:13:04PM +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.
> >> >> 
> >> >> 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.
> >> >
> >> >Please wrap changelog text at 72 columns, like git asks you to.
> >> git didn't asked me to do so, I'll fix it in the next iteration.
> >> 
> >> >
> >> >Also, why did you not cc: the staging maintainer?  :(
> >> I've probably missed that when I was going over the docs, my bad.
> >> 
> >> >
> >> >> Signed-off-by: Eial Czerwacki <eial.czerwacki@sap.com>
> >> >> ---
> >> >>  MAINTAINERS                             |   6 +
> >> >>  drivers/staging/Kconfig                 |   2 +
> >> >>  drivers/staging/Makefile                |   1 +
> >> >>  drivers/staging/vsmp/Kconfig            |  14 +
> >> >>  drivers/staging/vsmp/Makefile           |   7 +
> >> >>  drivers/staging/vsmp/api.c              | 402 ++++++++++++++++++++++++
> >> >>  drivers/staging/vsmp/api.h              |  61 ++++
> >> >>  drivers/staging/vsmp/common/Kconfig     |  11 +
> >> >>  drivers/staging/vsmp/common/Makefile    |   7 +
> >> >>  drivers/staging/vsmp/common/common.c    |  64 ++++
> >> >>  drivers/staging/vsmp/common/common.h    |  27 ++
> >> >>  drivers/staging/vsmp/common/version.c   |  85 +++++
> >> >>  drivers/staging/vsmp/logs/Kconfig       |  10 +
> >> >>  drivers/staging/vsmp/logs/Makefile      |   7 +
> >> >>  drivers/staging/vsmp/logs/active_logs.c | 112 +++++++
> >> >>  drivers/staging/vsmp/registers.h        |  16 +
> >> >
> >> >Without looking at the code, I do not see a TODO file here that lists
> >> >the tasks that need to be completed to get this out of the
> >> >drivers/staging/ directory.  This is a requirement.
> >> there are tasks to complete it however I'm not sure they are blockers.
> >
> >What tasks?
> support of other information bits like stats

I have no idea what that means :)

> >> >Also, why is this submitted for drivers/staging/ ?  What prevents it
> >> >from being merged to the "correct" place in the kernel tree today?
> >> afaiu, the correct order for new drivers is staging => mainline.
> >
> >No, not at all.  This is the "hard way" to get drivers merged.  I would
> >never recommend doing it this way for anyone as it will take more time
> >and effort than just doing it the correct way the first time.
> I understand, thanks for clearing it up
> 
> >
> >> in addition, from past experience with the main kernel mailing list, I
> >> thought the driver will get more traction here than in the main kernel
> >> mailing list.  unless there is a specific mailing list for drivers
> >> beside the staging tree one
> >
> >There are zillions of subsystem mailing lists out there.  Always target
> >the specific one you are interested in.  For this code, it should live
> >in drivers/virt/ right?  Ah, no mailing list specific for that one, but
> >I usually am the one that merges the code in there, so cc: me and Arnd
> >and lkml and we can go from there!
> drivers/virt looks like the correct place, so I need to send it as [PATCH] to the lkml with Arnd and yourself in the CC of the mail?

Yes please!

> >Also, you have a bunch of sysfs files, all of those need to be
> >documented in Documentation/ABI/ entries as you will see if you run
> >scripts/get_abi.pl with your driver loaded.
> will do.
> 
> >
> >Also, rip out the old-school PCI bus scanning stuff.  That hasn't been
> >needed since the 2.4 kernel days :)
> can you point me to a doc that states how I can retrieve the dev struct of the device in question?

The device in question is passed to you in your pci probe function
callback.  You do NOT allow any random PCI device number to be passed as
a module parameter, as that way lies madness.

If you MUST allow userspace to bind a specific PCI device to your
driver, then use the sysfs "bind" file that the driver core provides to
you.  Then the normal pci probe callback will be called and all is good.

In short, your driver is doing a lot of extra work that it doesn't need
to do at all, just remove all of that code.

thanks,

greg k-h

Re: [RFC] staging/vSMP: new driver

[Czerwacki, Eial]

Greetings Greg,

>On Thu, Mar 17, 2022 at 08:17:04AM +0000, Czerwacki, Eial wrote:
>> Greetings Greg,
>> 
>> >On Thu, Mar 17, 2022 at 07:34:30AM +0000, Czerwacki, Eial wrote:
>> >> Greetings Greg,
>> >> 
>> >> >On Wed, Mar 16, 2022 at 06:13:04PM +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.
>> >> >> 
>> >> >> 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.
>> >> >
>> >> >Please wrap changelog text at 72 columns, like git asks you to.
>> >> git didn't asked me to do so, I'll fix it in the next iteration.
>> >> 
>> >> >
>> >> >Also, why did you not cc: the staging maintainer?  :(
>> >> I've probably missed that when I was going over the docs, my bad.
>> >> 
>> >> >
>> >> >> Signed-off-by: Eial Czerwacki <eial.czerwacki@sap.com>
>> >> >> ---
>> >> >>  MAINTAINERS                             |   6 +
>> >> >>  drivers/staging/Kconfig                 |   2 +
>> >> >>  drivers/staging/Makefile                |   1 +
>> >> >>  drivers/staging/vsmp/Kconfig            |  14 +
>> >> >>  drivers/staging/vsmp/Makefile           |   7 +
>> >> >>  drivers/staging/vsmp/api.c              | 402 ++++++++++++++++++++++++
>> >> >>  drivers/staging/vsmp/api.h              |  61 ++++
>> >> >>  drivers/staging/vsmp/common/Kconfig     |  11 +
>> >> >>  drivers/staging/vsmp/common/Makefile    |   7 +
>> >> >>  drivers/staging/vsmp/common/common.c    |  64 ++++
>> >> >>  drivers/staging/vsmp/common/common.h    |  27 ++
>> >> >>  drivers/staging/vsmp/common/version.c   |  85 +++++
>> >> >>  drivers/staging/vsmp/logs/Kconfig       |  10 +
>> >> >>  drivers/staging/vsmp/logs/Makefile      |   7 +
>> >> >>  drivers/staging/vsmp/logs/active_logs.c | 112 +++++++
>> >> >>  drivers/staging/vsmp/registers.h        |  16 +
>> >> >
>> >> >Without looking at the code, I do not see a TODO file here that lists
>> >> >the tasks that need to be completed to get this out of the
>> >> >drivers/staging/ directory.  This is a requirement.
>> >> there are tasks to complete it however I'm not sure they are blockers.
>> >
>> >What tasks?
>> support of other information bits like stats
>
>I have no idea what that means :)
in short, the hypervisor can provide stats it collects, future implementation will export that too

>
>> >> >Also, why is this submitted for drivers/staging/ ?  What prevents it
>> >> >from being merged to the "correct" place in the kernel tree today?
>> >> afaiu, the correct order for new drivers is staging => mainline.
>> >
>> >No, not at all.  This is the "hard way" to get drivers merged.  I would
>> >never recommend doing it this way for anyone as it will take more time
>> >and effort than just doing it the correct way the first time.
>> I understand, thanks for clearing it up
>> 
>> >
>> >> in addition, from past experience with the main kernel mailing list, I
>> >> thought the driver will get more traction here than in the main kernel
>> >> mailing list.  unless there is a specific mailing list for drivers
>> >> beside the staging tree one
>> >
>> >There are zillions of subsystem mailing lists out there.  Always target
>> >the specific one you are interested in.  For this code, it should live
>> >in drivers/virt/ right?  Ah, no mailing list specific for that one, but
>> >I usually am the one that merges the code in there, so cc: me and Arnd
>> >and lkml and we can go from there!
>> drivers/virt looks like the correct place, so I need to send it as [PATCH] to the lkml with Arnd and yourself in the CC of the mail?
>
>Yes please!
will do.

>
>> >Also, you have a bunch of sysfs files, all of those need to be
>> >documented in Documentation/ABI/ entries as you will see if you run
>> >scripts/get_abi.pl with your driver loaded.
>> will do.
>> 
>> >
>> >Also, rip out the old-school PCI bus scanning stuff.  That hasn't been
>> >needed since the 2.4 kernel days :)
>> can you point me to a doc that states how I can retrieve the dev struct of the device in question?
>
>The device in question is passed to you in your pci probe function
>callback.  You do NOT allow any random PCI device number to be passed as
>a module parameter, as that way lies madness.
>
>If you MUST allow userspace to bind a specific PCI device to your
>driver, then use the sysfs "bind" file that the driver core provides to
>you.  Then the normal pci probe callback will be called and all is good.
>
>In short, your driver is doing a lot of extra work that it doesn't need
>to do at all, just remove all of that code.
I see, I just need to add the ops struct with only the probe function implemented and move the init code there.
understood, will refractor and resend

Thanks,

Eial

Re: [RFC] staging/vSMP: new driver

[Greg KH]

On Thu, Mar 17, 2022 at 08:52:37AM +0000, Czerwacki, Eial wrote:
> >> >What tasks?
> >> support of other information bits like stats
> >
> >I have no idea what that means :)
> in short, the hypervisor can provide stats it collects, future implementation will export that too

The trick will be _how_ you export this information.  Let's wait on that
one for now, your current api has lots of other questions to work out
first :)

thanks,

greg k-h

Re: [RFC] staging/vSMP: new driver

[Czerwacki, Eial]

>On Thu, Mar 17, 2022 at 08:52:37AM +0000, Czerwacki, Eial wrote:
>> >> >What tasks?
>> >> support of other information bits like stats
>> >
>> >I have no idea what that means :)
>> in short, the hypervisor can provide stats it collects, future implementation will export that too
>
>The trick will be _how_ you export this information.  Let's wait on that
>one for now, your current api has lots of other questions to work out
>first :)

sure, thanks for all the help.

Eial

Re: [RFC] staging/vSMP: new driver

[Czerwacki, Eial]

Greetings Greg,

>From: Czerwacki, Eial <eial.czerwacki@sap.com>
>Sent: Thursday, March 17, 2022 11:04
>To: Greg KH <gregkh@linuxfoundation.org>
>Cc: linux-staging@lists.linux.dev <linux-staging@lists.linux.dev>; SAP vSMP Linux Maintainer <linux.vsmp@sap.com>
>Subject: Re: [RFC] staging/vSMP: new driver 
>
>>On Thu, Mar 17, 2022 at 08:52:37AM +0000, Czerwacki, Eial wrote:
>>> >> >What tasks?
>>> >> support of other information bits like stats
>>> >
>>> >I have no idea what that means :)
>>> in short, the hypervisor can provide stats it collects, future implementation will export that too
>>
>>The trick will be _how_ you export this information.  Let's wait on that
>>one for now, your current api has lots of other questions to work out
>>first :)
>
>sure, thanks for all the help.
>
>Eial

I was wondering, after I've switched the driver flow to the pci driver one, I lost the ability to prevent vsmp.ko from loading in case no device was found.
is there a way do to so?

Eial

Re: [RFC] staging/vSMP: new driver

[Greg KH]

On Wed, Apr 20, 2022 at 11:18:15AM +0000, Czerwacki, Eial wrote:
> Greetings Greg,
> 
> >From: Czerwacki, Eial <eial.czerwacki@sap.com>
> >Sent: Thursday, March 17, 2022 11:04
> >To: Greg KH <gregkh@linuxfoundation.org>
> >Cc: linux-staging@lists.linux.dev <linux-staging@lists.linux.dev>; SAP vSMP Linux Maintainer <linux.vsmp@sap.com>
> >Subject: Re: [RFC] staging/vSMP: new driver 
> >
> >>On Thu, Mar 17, 2022 at 08:52:37AM +0000, Czerwacki, Eial wrote:
> >>> >> >What tasks?
> >>> >> support of other information bits like stats
> >>> >
> >>> >I have no idea what that means :)
> >>> in short, the hypervisor can provide stats it collects, future implementation will export that too
> >>
> >>The trick will be _how_ you export this information.  Let's wait on that
> >>one for now, your current api has lots of other questions to work out
> >>first :)
> >
> >sure, thanks for all the help.
> >
> >Eial
> 
> I was wondering, after I've switched the driver flow to the pci driver one, I lost the ability to prevent vsmp.ko from loading in case no device was found.
> is there a way do to so?

No, that is not how drivers have worked since the 2.4 kernel days (i.e.
20 years ago?)  It is fine for your driver to be loaded even if there is
no device present.

But, your driver will NOT be loaded automatically unless a device is
found, so why would it be present in that situation?

thanks,

greg k-h

Re: [RFC] staging/vSMP: new driver

[Czerwacki, Eial]

>From: Greg KH <gregkh@linuxfoundation.org>
>Sent: Wednesday, April 20, 2022 14:24
>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] staging/vSMP: new driver 
> 
>On Wed, Apr 20, 2022 at 11:18:15AM +0000, Czerwacki, Eial wrote:
>> Greetings Greg,
>> 
>> >From: Czerwacki, Eial <eial.czerwacki@sap.com>
>> >Sent: Thursday, March 17, 2022 11:04
>> >To: Greg KH <gregkh@linuxfoundation.org>
>> >Cc: linux-staging@lists.linux.dev <linux-staging@lists.linux.dev>; SAP vSMP Linux Maintainer <linux.vsmp@sap.com>
>> >Subject: Re: [RFC] staging/vSMP: new driver 
>> >
>> >>On Thu, Mar 17, 2022 at 08:52:37AM +0000, Czerwacki, Eial wrote:
>> >>> >> >What tasks?
>> >>> >> support of other information bits like stats
>> >>> >
>> >>> >I have no idea what that means :)
>> >>> in short, the hypervisor can provide stats it collects, future implementation will export that too
>> >>
>> >>The trick will be _how_ you export this information.  Let's wait on that
>> >>one for now, your current api has lots of other questions to work out
>> >>first :)
>> >
>> >sure, thanks for all the help.
>> >
>> >Eial
>> 
>> I was wondering, after I've switched the driver flow to the pci driver one, I lost the ability to prevent vsmp.ko from loading in case no device was found.
>> is there a way do to so?
>
>No, that is not how drivers have worked since the 2.4 kernel days (i.e.
>20 years ago?)  It is fine for your driver to be loaded even if there is
>no device present.
>
>But, your driver will NOT be loaded automatically unless a device is
>found, so why would it be present in that situation?
>
>thanks,
>
>greg k-h

because there are other modules which depends on it.
I guess I can detect it as part of the other module's init

Thanks,

Eial

Re: [RFC] staging/vSMP: new driver

[Greg KH]

On Wed, Apr 20, 2022 at 11:38:57AM +0000, Czerwacki, Eial wrote:
> >From: Greg KH <gregkh@linuxfoundation.org>
> >Sent: Wednesday, April 20, 2022 14:24
> >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] staging/vSMP: new driver 
> > 
> >On Wed, Apr 20, 2022 at 11:18:15AM +0000, Czerwacki, Eial wrote:
> >> Greetings Greg,
> >> 
> >> >From: Czerwacki, Eial <eial.czerwacki@sap.com>
> >> >Sent: Thursday, March 17, 2022 11:04
> >> >To: Greg KH <gregkh@linuxfoundation.org>
> >> >Cc: linux-staging@lists.linux.dev <linux-staging@lists.linux.dev>; SAP vSMP Linux Maintainer <linux.vsmp@sap.com>
> >> >Subject: Re: [RFC] staging/vSMP: new driver 
> >> >
> >> >>On Thu, Mar 17, 2022 at 08:52:37AM +0000, Czerwacki, Eial wrote:
> >> >>> >> >What tasks?
> >> >>> >> support of other information bits like stats
> >> >>> >
> >> >>> >I have no idea what that means :)
> >> >>> in short, the hypervisor can provide stats it collects, future implementation will export that too
> >> >>
> >> >>The trick will be _how_ you export this information.  Let's wait on that
> >> >>one for now, your current api has lots of other questions to work out
> >> >>first :)
> >> >
> >> >sure, thanks for all the help.
> >> >
> >> >Eial
> >> 
> >> I was wondering, after I've switched the driver flow to the pci driver one, I lost the ability to prevent vsmp.ko from loading in case no device was found.
> >> is there a way do to so?
> >
> >No, that is not how drivers have worked since the 2.4 kernel days (i.e.
> >20 years ago?)  It is fine for your driver to be loaded even if there is
> >no device present.
> >
> >But, your driver will NOT be loaded automatically unless a device is
> >found, so why would it be present in that situation?
> >
> >thanks,
> >
> >greg k-h
> 
> because there are other modules which depends on it.

I do not understand the question.

> I guess I can detect it as part of the other module's init

Huh?

Try it and see, the linker will properly pull in the needed modules if
you have symbols in one module that are needed by another one.

But if that is the case, why split them up into different modules?

I do not understand the question really.  Try it and see first and if
you have specific problems, please post the code and we will be glad to
review it.

thanks,

greg k-h

Re: [RFC] staging/vSMP: new driver

[Czerwacki, Eial]

>From: Greg KH <gregkh@linuxfoundation.org>
>Sent: Wednesday, April 20, 2022 14:42
>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] staging/vSMP: new driver 
> 
>On Wed, Apr 20, 2022 at 11:38:57AM +0000, Czerwacki, Eial wrote:
>> >From: Greg KH <gregkh@linuxfoundation.org>
>> >Sent: Wednesday, April 20, 2022 14:24
>> >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] staging/vSMP: new driver 
>> > 
>> >On Wed, Apr 20, 2022 at 11:18:15AM +0000, Czerwacki, Eial wrote:
>> >> Greetings Greg,
>> >> 
>> >> >From: Czerwacki, Eial <eial.czerwacki@sap.com>
>> >> >Sent: Thursday, March 17, 2022 11:04
>> >> >To: Greg KH <gregkh@linuxfoundation.org>
>> >> >Cc: linux-staging@lists.linux.dev <linux-staging@lists.linux.dev>; SAP vSMP Linux Maintainer <linux.vsmp@sap.com>
>> >> >Subject: Re: [RFC] staging/vSMP: new driver 
>> >> >
>> >> >>On Thu, Mar 17, 2022 at 08:52:37AM +0000, Czerwacki, Eial wrote:
>> >> >>> >> >What tasks?
>> >> >>> >> support of other information bits like stats
>> >> >>> >
>> >> >>> >I have no idea what that means :)
>> >> >>> in short, the hypervisor can provide stats it collects, future implementation will export that too
>> >> >>
>> >> >>The trick will be _how_ you export this information.  Let's wait on that
>> >> >>one for now, your current api has lots of other questions to work out
>> >> >>first :)
>> >> >
>> >> >sure, thanks for all the help.
>> >> >
>> >> >Eial
>> >> 
>> >> I was wondering, after I've switched the driver flow to the pci driver one, I lost the ability to prevent vsmp.ko from loading in case no device was found.
>> >> is there a way do to so?
>> >
>> >No, that is not how drivers have worked since the 2.4 kernel days (i.e.
>> >20 years ago?)  It is fine for your driver to be loaded even if there is
>> >no device present.
>> >
>> >But, your driver will NOT be loaded automatically unless a device is
>> >found, so why would it be present in that situation?
>> >
>> >thanks,
>> >
>> >greg k-h
>> 
>> because there are other modules which depends on it.
>
>I do not understand the question.
>
>> I guess I can detect it as part of the other module's init
>
>Huh?
>
>Try it and see, the linker will properly pull in the needed modules if
>you have symbols in one module that are needed by another one.
nothing prevents a user from loading the modules even if the device is missing.
the driver's hierarchy is one api modules and two unrelated modules which depend on it.
running modprobe vsmp_logs on a system where the device doesn't exists results with a kernel trace.
obliviously, this is not something accepted by the community.

>
>But if that is the case, why split them up into different modules?
splitting the driver into three modules is purely for flexibility reasons.
e.g. on runtime a user can decide which parts of the driver to run

>
>I do not understand the question really.  Try it and see first and if
>you have specific problems, please post the code and we will be glad to
>review it.
maybe I just need to forgo the flexibility and add all features as a built-in which isn't configurable

Eial

Re: [RFC] staging/vSMP: new driver

[Greg KH]

On Wed, Apr 20, 2022 at 11:57:51AM +0000, Czerwacki, Eial wrote:
> >From: Greg KH <gregkh@linuxfoundation.org>
> >Sent: Wednesday, April 20, 2022 14:42
> >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] staging/vSMP: new driver 
> > 
> >On Wed, Apr 20, 2022 at 11:38:57AM +0000, Czerwacki, Eial wrote:
> >> >From: Greg KH <gregkh@linuxfoundation.org>
> >> >Sent: Wednesday, April 20, 2022 14:24
> >> >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] staging/vSMP: new driver 
> >> > 
> >> >On Wed, Apr 20, 2022 at 11:18:15AM +0000, Czerwacki, Eial wrote:
> >> >> Greetings Greg,
> >> >> 
> >> >> >From: Czerwacki, Eial <eial.czerwacki@sap.com>
> >> >> >Sent: Thursday, March 17, 2022 11:04
> >> >> >To: Greg KH <gregkh@linuxfoundation.org>
> >> >> >Cc: linux-staging@lists.linux.dev <linux-staging@lists.linux.dev>; SAP vSMP Linux Maintainer <linux.vsmp@sap.com>
> >> >> >Subject: Re: [RFC] staging/vSMP: new driver 
> >> >> >
> >> >> >>On Thu, Mar 17, 2022 at 08:52:37AM +0000, Czerwacki, Eial wrote:
> >> >> >>> >> >What tasks?
> >> >> >>> >> support of other information bits like stats
> >> >> >>> >
> >> >> >>> >I have no idea what that means :)
> >> >> >>> in short, the hypervisor can provide stats it collects, future implementation will export that too
> >> >> >>
> >> >> >>The trick will be _how_ you export this information.  Let's wait on that
> >> >> >>one for now, your current api has lots of other questions to work out
> >> >> >>first :)
> >> >> >
> >> >> >sure, thanks for all the help.
> >> >> >
> >> >> >Eial
> >> >> 
> >> >> I was wondering, after I've switched the driver flow to the pci driver one, I lost the ability to prevent vsmp.ko from loading in case no device was found.
> >> >> is there a way do to so?
> >> >
> >> >No, that is not how drivers have worked since the 2.4 kernel days (i.e.
> >> >20 years ago?)  It is fine for your driver to be loaded even if there is
> >> >no device present.
> >> >
> >> >But, your driver will NOT be loaded automatically unless a device is
> >> >found, so why would it be present in that situation?
> >> >
> >> >thanks,
> >> >
> >> >greg k-h
> >> 
> >> because there are other modules which depends on it.
> >
> >I do not understand the question.
> >
> >> I guess I can detect it as part of the other module's init
> >
> >Huh?
> >
> >Try it and see, the linker will properly pull in the needed modules if
> >you have symbols in one module that are needed by another one.
> nothing prevents a user from loading the modules even if the device is missing.
> the driver's hierarchy is one api modules and two unrelated modules which depend on it.
> running modprobe vsmp_logs on a system where the device doesn't exists results with a kernel trace.

What exact "kernel trace" is emitted?

> obliviously, this is not something accepted by the community.

I do not understand what is not acceptable?

> >
> >But if that is the case, why split them up into different modules?
> splitting the driver into three modules is purely for flexibility reasons.

Flexible for whom?

> e.g. on runtime a user can decide which parts of the driver to run

How?  And why?  A kernel driver should "just work" no need to configure
anything or only load, or not load, a specific set of drivers.  That's
not how kernel modules work at all.

> >
> >I do not understand the question really.  Try it and see first and if
> >you have specific problems, please post the code and we will be glad to
> >review it.
> maybe I just need to forgo the flexibility and add all features as a built-in which isn't configurable

What exact "flexibility" are you trying to do here?

And again, nothing should be configured, it should always "just work" if
the needed hardware is present.  If not, nothing should happen.

Again, post your code for better answers, this is all just
hand-waving...

thanks,

greg k-h

Re: [RFC] staging/vSMP: new driver

[Czerwacki, Eial]

>From: Greg KH <gregkh@linuxfoundation.org>
>Sent: Wednesday, April 20, 2022 15:17
>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] staging/vSMP: new driver 
> 
>On Wed, Apr 20, 2022 at 11:57:51AM +0000, Czerwacki, Eial wrote:
>> >From: Greg KH <gregkh@linuxfoundation.org>
>> >Sent: Wednesday, April 20, 2022 14:42
>> >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] staging/vSMP: new driver 
>> > 
>> >On Wed, Apr 20, 2022 at 11:38:57AM +0000, Czerwacki, Eial wrote:
>> >> >From: Greg KH <gregkh@linuxfoundation.org>
>> >> >Sent: Wednesday, April 20, 2022 14:24
>> >> >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] staging/vSMP: new driver 
>> >> > 
>> >> >On Wed, Apr 20, 2022 at 11:18:15AM +0000, Czerwacki, Eial wrote:
>> >> >> Greetings Greg,
>> >> >> 
>> >> >> >From: Czerwacki, Eial <eial.czerwacki@sap.com>
>> >> >> >Sent: Thursday, March 17, 2022 11:04
>> >> >> >To: Greg KH <gregkh@linuxfoundation.org>
>> >> >> >Cc: linux-staging@lists.linux.dev <linux-staging@lists.linux.dev>; SAP vSMP Linux Maintainer <linux.vsmp@sap.com>
>> >> >> >Subject: Re: [RFC] staging/vSMP: new driver 
>> >> >> >
>> >> >> >>On Thu, Mar 17, 2022 at 08:52:37AM +0000, Czerwacki, Eial wrote:
>> >> >> >>> >> >What tasks?
>> >> >> >>> >> support of other information bits like stats
>> >> >> >>> >
>> >> >> >>> >I have no idea what that means :)
>> >> >> >>> in short, the hypervisor can provide stats it collects, future implementation will export that too
>> >> >> >>
>> >> >> >>The trick will be _how_ you export this information.  Let's wait on that
>> >> >> >>one for now, your current api has lots of other questions to work out
>> >> >> >>first :)
>> >> >> >
>> >> >> >sure, thanks for all the help.
>> >> >> >
>> >> >> >Eial
>> >> >> 
>> >> >> I was wondering, after I've switched the driver flow to the pci driver one, I lost the ability to prevent vsmp.ko from loading in case no device was found.
>> >> >> is there a way do to so?
>> >> >
>> >> >No, that is not how drivers have worked since the 2.4 kernel days (i.e.
>> >> >20 years ago?)  It is fine for your driver to be loaded even if there is
>> >> >no device present.
>> >> >
>> >> >But, your driver will NOT be loaded automatically unless a device is
>> >> >found, so why would it be present in that situation?
>> >> >
>> >> >thanks,
>> >> >
>> >> >greg k-h
>> >> 
>> >> because there are other modules which depends on it.
>> >
>> >I do not understand the question.
>> >
>> >> I guess I can detect it as part of the other module's init
>> >
>> >Huh?
>> >
>> >Try it and see, the linker will properly pull in the needed modules if
>> >you have symbols in one module that are needed by another one.
>> nothing prevents a user from loading the modules even if the device is missing.
>> the driver's hierarchy is one api modules and two unrelated modules which depend on it.
>> running modprobe vsmp_logs on a system where the device doesn't exists results with a kernel trace.
>
>What exact "kernel trace" is emitted?
accessing the bar of an non existent device.
thinking of it now, the api should return error in case the mapped region is not inited.

>
>> obliviously, this is not something accepted by the community.
>
>I do not understand what is not acceptable?
a module crashing because a missing invariant testing

>
>> >
>> >But if that is the case, why split them up into different modules?
>> splitting the driver into three modules is purely for flexibility reasons.
>
>Flexible for whom?
the end user

>
>> e.g. on runtime a user can decide which parts of the driver to run
>
>How?  And why?  A kernel driver should "just work" no need to configure
>anything or only load, or not load, a specific set of drivers.  That's
>not how kernel modules work at all.
that is why I think I should merge all the modules into one

>
>> >
>> >I do not understand the question really.  Try it and see first and if
>> >you have specific problems, please post the code and we will be glad to
>> >review it.
>> maybe I just need to forgo the flexibility and add all features as a built-in which isn't configurable
>
>What exact "flexibility" are you trying to do here?
per feature flexibility, for example, the ability to read logs via the device and display generic information
each one is encapsulated inside a different module

>
>And again, nothing should be configured, it should always "just work" if
>the needed hardware is present.  If not, nothing should happen.
>
>Again, post your code for better answers, this is all just
>hand-waving...
I understand, I'd prefer to understand all the aspects of the issues I'm facing before I load more code for you to go through as I assume that your time is precious

Eial

Re: [RFC] staging/vSMP: new driver

[Greg KH]

On Wed, Apr 20, 2022 at 12:36:04PM +0000, Czerwacki, Eial wrote:
> >From: Greg KH <gregkh@linuxfoundation.org>
> >Sent: Wednesday, April 20, 2022 15:17
> >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] staging/vSMP: new driver 
> > 
> >On Wed, Apr 20, 2022 at 11:57:51AM +0000, Czerwacki, Eial wrote:
> >> >From: Greg KH <gregkh@linuxfoundation.org>
> >> >Sent: Wednesday, April 20, 2022 14:42
> >> >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] staging/vSMP: new driver 
> >> > 
> >> >On Wed, Apr 20, 2022 at 11:38:57AM +0000, Czerwacki, Eial wrote:
> >> >> >From: Greg KH <gregkh@linuxfoundation.org>
> >> >> >Sent: Wednesday, April 20, 2022 14:24
> >> >> >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] staging/vSMP: new driver 
> >> >> > 
> >> >> >On Wed, Apr 20, 2022 at 11:18:15AM +0000, Czerwacki, Eial wrote:
> >> >> >> Greetings Greg,
> >> >> >> 
> >> >> >> >From: Czerwacki, Eial <eial.czerwacki@sap.com>
> >> >> >> >Sent: Thursday, March 17, 2022 11:04
> >> >> >> >To: Greg KH <gregkh@linuxfoundation.org>
> >> >> >> >Cc: linux-staging@lists.linux.dev <linux-staging@lists.linux.dev>; SAP vSMP Linux Maintainer <linux.vsmp@sap.com>
> >> >> >> >Subject: Re: [RFC] staging/vSMP: new driver 
> >> >> >> >
> >> >> >> >>On Thu, Mar 17, 2022 at 08:52:37AM +0000, Czerwacki, Eial wrote:
> >> >> >> >>> >> >What tasks?
> >> >> >> >>> >> support of other information bits like stats
> >> >> >> >>> >
> >> >> >> >>> >I have no idea what that means :)
> >> >> >> >>> in short, the hypervisor can provide stats it collects, future implementation will export that too
> >> >> >> >>
> >> >> >> >>The trick will be _how_ you export this information.  Let's wait on that
> >> >> >> >>one for now, your current api has lots of other questions to work out
> >> >> >> >>first :)
> >> >> >> >
> >> >> >> >sure, thanks for all the help.
> >> >> >> >
> >> >> >> >Eial
> >> >> >> 
> >> >> >> I was wondering, after I've switched the driver flow to the pci driver one, I lost the ability to prevent vsmp.ko from loading in case no device was found.
> >> >> >> is there a way do to so?
> >> >> >
> >> >> >No, that is not how drivers have worked since the 2.4 kernel days (i.e.
> >> >> >20 years ago?)  It is fine for your driver to be loaded even if there is
> >> >> >no device present.
> >> >> >
> >> >> >But, your driver will NOT be loaded automatically unless a device is
> >> >> >found, so why would it be present in that situation?
> >> >> >
> >> >> >thanks,
> >> >> >
> >> >> >greg k-h
> >> >> 
> >> >> because there are other modules which depends on it.
> >> >
> >> >I do not understand the question.
> >> >
> >> >> I guess I can detect it as part of the other module's init
> >> >
> >> >Huh?
> >> >
> >> >Try it and see, the linker will properly pull in the needed modules if
> >> >you have symbols in one module that are needed by another one.
> >> nothing prevents a user from loading the modules even if the device is missing.
> >> the driver's hierarchy is one api modules and two unrelated modules which depend on it.
> >> running modprobe vsmp_logs on a system where the device doesn't exists results with a kernel trace.
> >
> >What exact "kernel trace" is emitted?
> accessing the bar of an non existent device.

Which is impossible to do if your driver is written properly.

> thinking of it now, the api should return error in case the mapped region is not inited.

No, your code will just not be called if the device is not present.

> >> obliviously, this is not something accepted by the community.
> >
> >I do not understand what is not acceptable?
> a module crashing because a missing invariant testing
> 
> >
> >> >
> >> >But if that is the case, why split them up into different modules?
> >> splitting the driver into three modules is purely for flexibility reasons.
> >
> >Flexible for whom?
> the end user

How?

> >> e.g. on runtime a user can decide which parts of the driver to run
> >
> >How?  And why?  A kernel driver should "just work" no need to configure
> >anything or only load, or not load, a specific set of drivers.  That's
> >not how kernel modules work at all.
> that is why I think I should merge all the modules into one
> 
> >
> >> >
> >> >I do not understand the question really.  Try it and see first and if
> >> >you have specific problems, please post the code and we will be glad to
> >> >review it.
> >> maybe I just need to forgo the flexibility and add all features as a built-in which isn't configurable
> >
> >What exact "flexibility" are you trying to do here?
> per feature flexibility, for example, the ability to read logs via the device and display generic information
> each one is encapsulated inside a different module
> 
> >
> >And again, nothing should be configured, it should always "just work" if
> >the needed hardware is present.  If not, nothing should happen.
> >
> >Again, post your code for better answers, this is all just
> >hand-waving...
> I understand, I'd prefer to understand all the aspects of the issues I'm facing before I load more code for you to go through as I assume that your time is precious

Yes, and as such, you should submit code so that we can talk about this
with concrete examples instead of unknown vague questions.

Please just redo the patches based on the review and resubmit.

thanks,

greg k-h

Re: [RFC] staging/vSMP: new driver

[Greg KH]

On Wed, Mar 16, 2022 at 06:13:04PM +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.
> 
> 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>
> ---
>  MAINTAINERS                             |   6 +
>  drivers/staging/Kconfig                 |   2 +
>  drivers/staging/Makefile                |   1 +
>  drivers/staging/vsmp/Kconfig            |  14 +
>  drivers/staging/vsmp/Makefile           |   7 +
>  drivers/staging/vsmp/api.c              | 402 ++++++++++++++++++++++++
>  drivers/staging/vsmp/api.h              |  61 ++++
>  drivers/staging/vsmp/common/Kconfig     |  11 +
>  drivers/staging/vsmp/common/Makefile    |   7 +
>  drivers/staging/vsmp/common/common.c    |  64 ++++
>  drivers/staging/vsmp/common/common.h    |  27 ++
>  drivers/staging/vsmp/common/version.c   |  85 +++++
>  drivers/staging/vsmp/logs/Kconfig       |  10 +
>  drivers/staging/vsmp/logs/Makefile      |   7 +
>  drivers/staging/vsmp/logs/active_logs.c | 112 +++++++
>  drivers/staging/vsmp/registers.h        |  16 +
>  16 files changed, 832 insertions(+)

800 lines of code turn into 3 modules?  Why isn't this just one module?
Why split them at all?

thanks,

greg k-h

Re: [RFC] staging/vSMP: new driver

[Czerwacki, Eial]

Greetings Greg,

>On Wed, Mar 16, 2022 at 06:13:04PM +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.
>> 
>> 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>
>> ---
>>  MAINTAINERS                             |   6 +
>>  drivers/staging/Kconfig                 |   2 +
>>  drivers/staging/Makefile                |   1 +
>>  drivers/staging/vsmp/Kconfig            |  14 +
>>  drivers/staging/vsmp/Makefile           |   7 +
>>  drivers/staging/vsmp/api.c              | 402 ++++++++++++++++++++++++
>>  drivers/staging/vsmp/api.h              |  61 ++++
>>  drivers/staging/vsmp/common/Kconfig     |  11 +
>>  drivers/staging/vsmp/common/Makefile    |   7 +
>>  drivers/staging/vsmp/common/common.c    |  64 ++++
>>  drivers/staging/vsmp/common/common.h    |  27 ++
>>  drivers/staging/vsmp/common/version.c   |  85 +++++
>>  drivers/staging/vsmp/logs/Kconfig       |  10 +
>>  drivers/staging/vsmp/logs/Makefile      |   7 +
>>  drivers/staging/vsmp/logs/active_logs.c | 112 +++++++
>>  drivers/staging/vsmp/registers.h        |  16 +
>>  16 files changed, 832 insertions(+)
>
>800 lines of code turn into 3 modules?  Why isn't this just one module?
>Why split them at all?
>

imho it is more flexible, if a developer needs only the api part but wants the ability to read only the logs, he can do so without the need to have the module up
if you think that merging it to one will increase the changes the driver will be merged to head soon, I'll merge them to one module

Thanks,

Eial

Re: [RFC] staging/vSMP: new driver

[Greg KH]

On Thu, Mar 17, 2022 at 07:38:28AM +0000, Czerwacki, Eial wrote:
> Greetings Greg,
> 
> >On Wed, Mar 16, 2022 at 06:13:04PM +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.
> >> 
> >> 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>
> >> ---
> >>  MAINTAINERS                             |   6 +
> >>  drivers/staging/Kconfig                 |   2 +
> >>  drivers/staging/Makefile                |   1 +
> >>  drivers/staging/vsmp/Kconfig            |  14 +
> >>  drivers/staging/vsmp/Makefile           |   7 +
> >>  drivers/staging/vsmp/api.c              | 402 ++++++++++++++++++++++++
> >>  drivers/staging/vsmp/api.h              |  61 ++++
> >>  drivers/staging/vsmp/common/Kconfig     |  11 +
> >>  drivers/staging/vsmp/common/Makefile    |   7 +
> >>  drivers/staging/vsmp/common/common.c    |  64 ++++
> >>  drivers/staging/vsmp/common/common.h    |  27 ++
> >>  drivers/staging/vsmp/common/version.c   |  85 +++++
> >>  drivers/staging/vsmp/logs/Kconfig       |  10 +
> >>  drivers/staging/vsmp/logs/Makefile      |   7 +
> >>  drivers/staging/vsmp/logs/active_logs.c | 112 +++++++
> >>  drivers/staging/vsmp/registers.h        |  16 +
> >>  16 files changed, 832 insertions(+)
> >
> >800 lines of code turn into 3 modules?  Why isn't this just one module?
> >Why split them at all?
> >
> 
> imho it is more flexible, if a developer needs only the api part but wants the ability to read only the logs, he can do so without the need to have the module up
> if you think that merging it to one will increase the changes the driver will be merged to head soon, I'll merge them to one module

I do not know, as I do not know what the code does (didn't really look
at it.)

Leave it as-is for now, we can review it properly in future submissions,
you have bigger issues to fix up first :)

thanks,

greg k-h

Re: [RFC] staging/vSMP: new driver

[Dan Carpenter]

"new driver" is too vague.  What is the driver for?

You need a README or a TODO which explains how to move the driver out of
staging and into the main kernel.

Run the patch through scripts/checkpatch.pl --strict.  You don't have
to fix everything because this is staging but a bunch of the style
issues are easy to fix so we may as well just do it.

There are a number of bugs and style issues.
1) Find and replace every -1 with a define or a proper error code
2) Never do assignments inside of a condition
3) Do success handling instead of error handling.  Try to return errors
   as directly as possible.

Bad:
	ret = frob();
	if (!ret)
		printk("success!");
	return ret;

Good:
	ret = frob();
	if (ret)
		return ret;
	printk("success!");
	return 0;

Keep scrolling down for the details.

On Wed, Mar 16, 2022 at 06:13:04PM +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.
> 
> 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>
> ---
>  MAINTAINERS                             |   6 +
>  drivers/staging/Kconfig                 |   2 +
>  drivers/staging/Makefile                |   1 +
>  drivers/staging/vsmp/Kconfig            |  14 +
>  drivers/staging/vsmp/Makefile           |   7 +
>  drivers/staging/vsmp/api.c              | 402 ++++++++++++++++++++++++
>  drivers/staging/vsmp/api.h              |  61 ++++
>  drivers/staging/vsmp/common/Kconfig     |  11 +
>  drivers/staging/vsmp/common/Makefile    |   7 +
>  drivers/staging/vsmp/common/common.c    |  64 ++++
>  drivers/staging/vsmp/common/common.h    |  27 ++
>  drivers/staging/vsmp/common/version.c   |  85 +++++
>  drivers/staging/vsmp/logs/Kconfig       |  10 +
>  drivers/staging/vsmp/logs/Makefile      |   7 +
>  drivers/staging/vsmp/logs/active_logs.c | 112 +++++++
>  drivers/staging/vsmp/registers.h        |  16 +
>  16 files changed, 832 insertions(+)
>  create mode 100644 drivers/staging/vsmp/Kconfig
>  create mode 100644 drivers/staging/vsmp/Makefile
>  create mode 100644 drivers/staging/vsmp/api.c
>  create mode 100644 drivers/staging/vsmp/api.h
>  create mode 100644 drivers/staging/vsmp/common/Kconfig
>  create mode 100644 drivers/staging/vsmp/common/Makefile
>  create mode 100644 drivers/staging/vsmp/common/common.c
>  create mode 100644 drivers/staging/vsmp/common/common.h
>  create mode 100644 drivers/staging/vsmp/common/version.c
>  create mode 100644 drivers/staging/vsmp/logs/Kconfig
>  create mode 100644 drivers/staging/vsmp/logs/Makefile
>  create mode 100644 drivers/staging/vsmp/logs/active_logs.c
>  create mode 100644 drivers/staging/vsmp/registers.h
> 
> diff --git a/MAINTAINERS b/MAINTAINERS
> index 68f21d46614c..2fc61b4d13e5 100644
> --- a/MAINTAINERS
> +++ b/MAINTAINERS
> @@ -18381,6 +18381,12 @@ F:	Documentation/core-api/printk-formats.rst
>  F:	lib/test_printf.c
>  F:	lib/vsprintf.c
>  
> +VSMP GUEST DRIVER
> +M:     Eial Czerwacki <eial.czerwacki@sap.com>
> +M:     linux-vsmp@sap.com
> +S:     Maintained
> +F:     drivers/staging/vsmp-guest 
> +
>  VT1211 HARDWARE MONITOR DRIVER
>  M:	Juerg Haefliger <juergh@gmail.com>
>  L:	linux-hwmon@vger.kernel.org
> diff --git a/drivers/staging/Kconfig b/drivers/staging/Kconfig
> index 4ec5528f89fa..f61778bccb0b 100644
> --- a/drivers/staging/Kconfig
> +++ b/drivers/staging/Kconfig
> @@ -114,6 +114,8 @@ source "drivers/staging/axis-fifo/Kconfig"
>  
>  source "drivers/staging/fieldbus/Kconfig"
>  
> +source "drivers/staging/vsmp/Kconfig"
> +
>  source "drivers/staging/kpc2000/Kconfig"
>  
>  source "drivers/staging/qlge/Kconfig"
> diff --git a/drivers/staging/Makefile b/drivers/staging/Makefile
> index 4d34198151b3..726e704ba8bc 100644
> --- a/drivers/staging/Makefile
> +++ b/drivers/staging/Makefile
> @@ -47,6 +47,7 @@ obj-$(CONFIG_SOC_MT7621)	+= mt7621-dts/
>  obj-$(CONFIG_STAGING_GASKET_FRAMEWORK)	+= gasket/
>  obj-$(CONFIG_XIL_AXIS_FIFO)	+= axis-fifo/
>  obj-$(CONFIG_FIELDBUS_DEV)     += fieldbus/
> +obj-$(CONFIG_VSMP)       += vsmp/
>  obj-$(CONFIG_KPC2000)		+= kpc2000/
>  obj-$(CONFIG_QLGE)		+= qlge/
>  obj-$(CONFIG_WFX)		+= wfx/
> diff --git a/drivers/staging/vsmp/Kconfig b/drivers/staging/vsmp/Kconfig
> new file mode 100644
> index 000000000000..c57cfcb55033
> --- /dev/null
> +++ b/drivers/staging/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 drivers allows information gathering of data from the vSMP hypervisor when
> +	  running ontop of a vSMP based VM.
> +
> +	  If unsure, say no.
> +
> +source "drivers/staging/vsmp/common/Kconfig"
> +source "drivers/staging/vsmp/logs/Kconfig"
> diff --git a/drivers/staging/vsmp/Makefile b/drivers/staging/vsmp/Makefile
> new file mode 100644
> index 000000000000..88625cd3707d
> --- /dev/null
> +++ b/drivers/staging/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/staging/vsmp/api.c b/drivers/staging/vsmp/api.c
> new file mode 100644
> index 000000000000..b0d4b5a990d4
> --- /dev/null
> +++ b/drivers/staging/vsmp/api.c
> @@ -0,0 +1,402 @@
> +/* 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 DRIVER_LICENSE "GPL"
> +#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 = 0;
> +static unsigned short vsmp_dev = 0x1f;
> +static unsigned char vsmp_func = 0;
> +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))

Put the operator on the first line.

> +
> +/* global vars */
> +void __iomem *cfg_addr = NULL;
> +static struct kobject *vsmp_sysfs_kobj;
> +static struct pci_dev *vsmp_dev_obj = NULL;
> +
> +static const struct pci_device_id 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)
> +{
> +	if (strlen(vsmp_ctrl_pci_addr)) {

Reverse this condition.

	if (strlen(vsmp_ctrl_pci_addr) != 0)
		return;

> +		unsigned int bus;
> +		unsigned short dev;
> +		unsigned char func;
> +
> +		if (sscanf(vsmp_ctrl_pci_addr, "%04x:%02hx.%1hhx", &bus, &dev,
> +			   &func) == 3) {

Same:

	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 specfic device in order to determine if it is a vSMP pci device
> + */
> +static void query_pci_bus_for_vsmp_directly(unsigned int default_bus,
> +					    unsigned int devfn)
> +{
> +	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);

Doesn't this need to set mapped_bars[0].len as well?

> +}
> +
> +/**
> + * scan all the devices on the system in order to locate the
> + * vSMP pic device for a given dev end func is provided
> + */
> +static void scan_pci_devs_list_for_vsmp(unsigned int devfn)

This function should return negatives if we do not find a device.
The current error hanlding is convoluted and relies on globals.

> +{
> +	int found, i;

"found" is never set to false.  Make it a bool.

> +	struct pci_dev *pdev = NULL;

Do not initialize "pdev" to a bogus value.

> +	const struct pci_device_id *ent;
> +
> +	for_each_pci_dev(pdev) {
> +		if (devfn != -1) {

Make this -1 a define instead of a magic number.

> +			if ((pdev->bus->number == vsmp_bus) &&
> +			    (devfn == pdev->devfn)) {
> +				found = 1;
> +				break;
> +			}
> +		} else {
> +			ent = pci_match_id(pci_tbl, pdev);
> +			if (ent) {
> +				found = 1;
> +				vsmp_bus = pdev->bus->number;
> +				vsmp_dev = PCI_SLOT(pdev->devfn);
> +				vsmp_func = PCI_FUNC(pdev->devfn);
> +				break;
> +			}
> +		}
> +	}

	if (!found)
		return -ENODEV;

> +
> +	for (i = 0; (i < ARRAY_SIZE(mapped_bars)) && found; i++) {
> +		mapped_bars[i].start = pci_resource_start(pdev, i);
> +		mapped_bars[i].len = pci_resource_len(pdev, i);
> +	}
> +	vsmp_dev_obj = pdev;
> +}
> +
> +/**
> + * open the cfg address space of the vSDP device
> + */
> +static int open_cfg_addr(void)
> +{
> +	unsigned int devfn = (vsmp_bdf_static) ? PCI_DEVFN(vsmp_dev, vsmp_func)
> +						: -1;
> +
> +	scan_pci_devs_list_for_vsmp(devfn);
> +	if (!mapped_bars[0].start)
> +		query_pci_bus_for_vsmp_directly(0, devfn);
> +
> +	if (!mapped_bars[0].len) {
> +		printk(KERN_INFO "vSMP pci controller not found, exiting.\n");
> +		return -1;

Return proper kernel error codes.  "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) {

Delete the blank link between the call and the error checking.  They are
part of the same thing.

> +		printk(KERN_ERR
> +		       "failed to map vSMP pci controller at %x:%x.%x, exiting.\n",
> +		       vsmp_bus, vsmp_dev, vsmp_func);
> +		return -1;
> +	}
> +
> +	return 0;
> +}
> +
> +/* exported functions */
> +
> +/**
> + * read a value from a specfic register in the vSMP's device config space
> + */
> +unsigned long vsmp_read_reg_from_cfg(unsigned int reg, enum reg_size_type type)


Use u64 when 64 bit types are intended.

> +{
> +	unsigned long ret_val;


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 = (unsigned long)(-1);


64 bit types.  "ret_val = -1ULL;"


> +	}
> +
> +	dev_dbg(&vsmp_dev_obj->dev, "%s: read 0x%lx from reg 0x%x 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 specfic register in the vSMP's device config space
> + */
> +void vsmp_write_reg_to_cfg(unsigned int reg, unsigned long value,
> +			   enum reg_size_type type)
> +{
> +	switch (type) {
> +	case VSMP_CTL_REG_SIZE_8BIT:
> +		writeb((unsigned char)value, cfg_addr + reg);
> +		break;
> +
> +	case VSMP_CTL_REG_SIZE_16BIT:
> +		writew((unsigned short)value, cfg_addr + reg);
> +		break;
> +
> +	case VSMP_CTL_REG_SIZE_32BIT:
> +		writel((unsigned int)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%x to reg 0x%x of %u bits\n",
> +		__func__, reg, reg, (type + 1) * 8);
> +}
> +EXPORT_SYMBOL_GPL(vsmp_write_reg_to_cfg);
> +
> +/**
> + * reag a buffer from a specific offset in a specific bar, maxed to a predefined len size-wise from the vSMP device
> + */
> +unsigned int vsmp_read_buff_from_bar(unsigned int bar, unsigned int offset,

This is unsigned int but it returns zero or negative error codes.

Create a separate function for "halt_on_null".  It will be cleaner that
way.

> +				     char *out, unsigned int len,
> +				     bool halt_on_null)
> +{
> +	unsigned char __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) {
> +		int i;
> +		unsigned char c;
> +
> +		for (i = 0; i < len; i++) {
> +			c = ioread8(&buff[i]);
> +			if (!c)
> +				break;
> +			out[i] = c;

Copy the NUL terminator to out[i] before the break.

> +		}
> +	} 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 = -1;
> +
> +	if (vsmp_sysfs_kobj && bin_attr) {
> +		if ((error = sysfs_create_bin_file(vsmp_sysfs_kobj, bin_attr)))
> +			printk(KERN_CRIT "%s returned error %d\n", __func__,
> +			       error);
> +	}
> +
> +	return error;
> +}
> +EXPORT_SYMBOL_GPL(vsmp_register_sysfs_group);
> +
> +/**
> + * deergister 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, unsigned bar, unsigned reg,
> +			       char *buf, loff_t off, size_t count)
> +{
> +	ssize_t ret_val = 0;
> +
> +	if (op->buff_offset >= op->hwi_block_size) {	// preform H/W op
> +		vsmp_reset_op(op);
> +
> +		if ((ret_val = vsmp_read_buff_from_bar(bar, reg, op->buff,
> +						       op->hwi_block_size, false))) {
> +			printk(KERN_ERR "%s operation failed\n",
> +			       (op->action == FW_READ) ? "read" : "write");
> +		}
> +		op->buff_offset = 0;
> +	}
> +
> +	if (!ret_val) {

Flip this around.  Do error handling.  Don't do success handling.

> +		ret_val = memory_read_from_buffer(buf, count, &op->buff_offset,
> +						  op->buff, op->hwi_block_size);
> +	}
> +
> +	return ret_val;
> +}
> +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;
> +
> +	if (!(op->buff = kzalloc(op->hwi_block_size, GFP_KERNEL)))
> +		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 __init vsmp_api_init(void)
> +{
> +	int ret_val;
> +
> +	parse_vsmp_ctrl_pci_addr();
> +
> +	if (!(ret_val = open_cfg_addr())) {

Error handling not success handling.

> +		printk(KERN_INFO "%s [%02x:%02x.%x] up and running\n",
> +		       DRIVER_DESC, vsmp_bus, vsmp_dev, vsmp_func);
> +
> +		if (!(vsmp_sysfs_kobj = kobject_create_and_add("vsmp",
> +								hypervisor_kobj))) {

Reverse.

> +			printk(KERN_ERR
> +			       "failed to create sysfs entry for vSMP pci controller at %x:%x.%x, exiting.\n",
> +			       vsmp_bus, vsmp_dev, vsmp_func);
> +			ret_val = -1;
> +		}
> +	}
> +
> +	return ret_val;
> +}
> +
> +/**
> + * cleanup after the module upon exit
> + */
> +static void __exit vsmp_api_exit(void)
> +{
> +	if (cfg_addr)
> +		iounmap(cfg_addr);
> +
> +	if (vsmp_sysfs_kobj)
> +		kobject_put(vsmp_sysfs_kobj);
> +}
> +
> +module_init(vsmp_api_init);
> +module_exit(vsmp_api_exit);
> diff --git a/drivers/staging/vsmp/api.h b/drivers/staging/vsmp/api.h
> new file mode 100644
> index 000000000000..88d81b80bd81
> --- /dev/null
> +++ b/drivers/staging/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_) ((unsigned char) vsmp_read_reg_from_cfg((_reg_), VSMP_CTL_REG_SIZE_8BIT))
> +#define vsmp_read_reg16_from_cfg(_reg_) ((unsigned short) vsmp_read_reg_from_cfg((_reg_), VSMP_CTL_REG_SIZE_16BIT))
> +#define vsmp_read_reg32_from_cfg(_reg_) ((unsigned int) vsmp_read_reg_from_cfg((_reg_), VSMP_CTL_REG_SIZE_32BIT))
> +#define vsmp_read_reg64_from_cfg(_reg_) ((unsigned long) 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)
> +
> +unsigned long vsmp_read_reg_from_cfg(unsigned int reg, enum reg_size_type type);
> +void vsmp_write_reg_to_cfg(unsigned int reg, unsigned long value,
> +			   enum reg_size_type type);
> +unsigned int vsmp_read_buff_from_bar(unsigned int bar, unsigned int offset,
> +				     char *out, unsigned int 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, unsigned bar, unsigned reg,
> +			       char *buf, loff_t off, size_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/staging/vsmp/common/Kconfig b/drivers/staging/vsmp/common/Kconfig
> new file mode 100644
> index 000000000000..fe0cfeaca1bb
> --- /dev/null
> +++ b/drivers/staging/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 retrive generic data from the vSMP
> +	  VM.
> +	  currently old version is supported.
> +	  the data can be found under /sys/hypervisor/vsmp
> +
> +	  If unsure, say N.
> diff --git a/drivers/staging/vsmp/common/Makefile b/drivers/staging/vsmp/common/Makefile
> new file mode 100644
> index 000000000000..e6f6aa1d73df
> --- /dev/null
> +++ b/drivers/staging/vsmp/common/Makefile
> @@ -0,0 +1,7 @@
> +# SPDX-License-Identifier: GPL-2.0
> +#
> +# Makefile for vSMP Guest common feature.
> +#
> +
> +obj-$(CONFIG_VSMP_COMMON)	+= common_info.o 
> +common_info-y		+= common.o version.o
> diff --git a/drivers/staging/vsmp/common/common.c b/drivers/staging/vsmp/common/common.c
> new file mode 100644
> index 000000000000..31e6551f2b7f
> --- /dev/null
> +++ b/drivers/staging/vsmp/common/common.c
> @@ -0,0 +1,64 @@
> +/* 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"
> +#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++) {
> +		if ((ret_val = cbs_arr[i].reg_cb()))
> +			printk(KERN_ERR "failed to init sysfs (%d), exiting.\n",
> +			       ret_val);
> +	}
> +
> +	return -1 * ret_val;

sysfs_register_version_cb() already returns negatives.  Why the "-1 *"?

> +}
> +
> +/**
> + * 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/staging/vsmp/common/common.h b/drivers/staging/vsmp/common/common.h
> new file mode 100644
> index 000000000000..08a096628c95
> --- /dev/null
> +++ b/drivers/staging/vsmp/common/common.h
> @@ -0,0 +1,27 @@
> +/* SPDX-License-Identifier: GPL-2.0 */
> +/*
> + * vSMP common header
> + * Copyright (C) 2022 SAP.
> + */
> +
> +#ifndef VSM_COMMON_H
> +#define VSM_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 // !VSM_COMMON_H
> diff --git a/drivers/staging/vsmp/common/version.c b/drivers/staging/vsmp/common/version.c
> new file mode 100644
> index 000000000000..35b5a7785e9a
> --- /dev/null
> +++ b/drivers/staging/vsmp/common/version.c
> @@ -0,0 +1,85 @@
> +/* 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"
> +
> +#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)
> +{
> +	ssize_t ret_val = vsmp_generic_buff_read(&op, 0,
> +						 vsmp_read_reg32_from_cfg(VSMP_VERSION_REG),
> +						 buf, off, count);

Don't put functions which can fail in the declaration block.

	ret_val = vsmp_generic_buff_read(&op, 0,
					 vsmp_read_reg32_from_cfg(VSMP_VERSION_REG),
					 buf, off, count);

> +
> +	if ((ret_val != -ENOMEM) && ((ret_val != -EINVAL)))

Don't test for specific error codes.

	if (ret_val < 0)
		return ret_val;


> +	    buf[ret_val++] = '\n';
> +
> +	return ret_val;
> +}
> +
> +struct bin_attribute version_raw_attr = __BIN_ATTR(version, FILE_PREM, version_read, NULL, VERSION_MAX_LEN);
> +
> +/**
> + * retrive str in order to determine the proper length.
> + * this is the best way to maintain backwards compatibility with all
> + * vSMP versions.
> + */
> +static int get_version_len(void)
> +{
> +	unsigned reg;
> +	char *version_str = kzalloc(VERSION_MAX_LEN, GFP_ATOMIC | __GFP_NOWARN);

VERSION_MAX_LEN is 524288 bytes.  That's too long.  Create a small
buffer and loop over it until you find the NUL terminator.  Why does
this need to be ATOMIC, are we holding a lock?  Hopefully if we can fix
the length the __GFP_NOWARN will be unnecessary.

> +
> +	if (!version_str)
> +		return -1;
> +
> +	reg = vsmp_read_reg32_from_cfg(VSMP_VERSION_REG);
> +	memset(version_str, 0, VERSION_MAX_LEN);
> +
> +	if (vsmp_read_buff_from_bar(0, reg, version_str, VERSION_MAX_LEN, true)) {
> +		printk(KERN_ERR "failed to read buffer from bar\n");
> +		return -1;
> +	}
> +
> +	version_raw_attr.size = strlen(version_str);

get_version_len() should return the length instead of setting a global.

> +	kfree(version_str);
> +
> +	return 0;
> +}
> +
> +/**
> + * register the version sysfs entry
> + */
> +int sysfs_register_version_cb(void)
> +{
> +	if (get_version_len()) {
> +		printk(KERN_ERR "failed to init vSMP version module\n");
> +		return -1;
> +	}
> +
> +	if (!vsmp_init_op(&op, version_raw_attr.size, FW_READ)) {

This if statement is reversed so this code can never work.

> +		printk(KERN_ERR "failed to init vSMP version op\n");
> +		return -1;
> +	}
> +
> +	return vsmp_register_sysfs_group(&version_raw_attr);
> +}
> +
> +/**
> + * 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/staging/vsmp/logs/Kconfig b/drivers/staging/vsmp/logs/Kconfig
> new file mode 100644
> index 000000000000..d41491472bab
> --- /dev/null
> +++ b/drivers/staging/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 retrive logs from the vSMP
> +	  VM.
> +	  the data can be found under /sys/hypervisor/logs
> +
> +	  If unsure, say N.
> diff --git a/drivers/staging/vsmp/logs/Makefile b/drivers/staging/vsmp/logs/Makefile
> new file mode 100644
> index 000000000000..edc679803fe6
> --- /dev/null
> +++ b/drivers/staging/vsmp/logs/Makefile
> @@ -0,0 +1,7 @@
> +# SPDX-License-Identifier: GPL-2.0
> +#
> +# Makefile for vSMP Guest common feature.
> +#
> +
> +obj-$(CONFIG_VSMP_LOGS)	+= logs.o 
> +logs-y		+= active_logs.o
> diff --git a/drivers/staging/vsmp/logs/active_logs.c b/drivers/staging/vsmp/logs/active_logs.c
> new file mode 100644
> index 000000000000..91492c72cc6f
> --- /dev/null
> +++ b/drivers/staging/vsmp/logs/active_logs.c
> @@ -0,0 +1,112 @@
> +/* 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"
> +#define DRIVER_AUTHOR "Eial Czerwacki <eial.czerwacki@sap.com>"
> +#define DRIVER_DESC "vSMP 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);

Just put:

MODULE_LICENSE("GPL v2");

Adding unnecessary indirection is against kernel style.  It says v2 at
the top.

> +
> +static struct fw_ops op;
> +static unsigned log_start;
> +DEFINE_MUTEX(log_mutex);
> +
> +/* module functions */
> +static ssize_t 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) {
> +		if ((ret_val = mutex_lock_interruptible(&log_mutex))) {

Don't do assignments inside of an if condition.

> +			printk(KERN_ERR
> +			       "error in atemmpt to lock mutex (%lx)\n",
> +			       ret_val);
> +			return ret_val;
> +		}
> +
> +		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(&log_mutex);
> +			return ret_val;

Return a literal when possible. "return 0;" is more clear than
"return ret_val;".

> +		}
> +	}
> +
> +	ret_val = vsmp_generic_buff_read(&op, 1, log_start, buf, off, count);
> +	if (!ret_val || (ret_val == -ENOMEM) || (ret_val == -EINVAL)) {
> +		printk(KERN_ERR "error reading from buffer\n");
> +		mutex_unlock(&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 log_raw_attr = __BIN_ATTR(log, FILE_PREM, log_read, NULL, 0);
> +
> +/**
> + * init the log module
> + */
> +static int __init vsmp_logs_init(void)
> +{
> +	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 -1;
> +	}
> +
> +	log_start = vsmp_read_reg32_from_cfg(VSMP_LOGS_START_REG);
> +	mutex_init(&log_mutex);
> +
> +	return vsmp_register_sysfs_group(&log_raw_attr);

If vsmp_register_sysfs_group() fails then it needs to vsmp_release_op().

I have written a lot about how to write error handling code:
https://lore.kernel.org/all/20210831084735.GL12231@kadam/

regards,
dan carpenter

Re: [RFC] staging/vSMP: new driver

[Dan Carpenter]

On Thu, Mar 17, 2022 at 01:19:43PM +0300, Dan Carpenter wrote:
> "new driver" is too vague.  What is the driver for?
> 
> You need a README or a TODO which explains how to move the driver out of
> staging and into the main kernel.
> 
> Run the patch through scripts/checkpatch.pl --strict.  You don't have
> to fix everything because this is staging but a bunch of the style
> issues are easy to fix so we may as well just do it.
> 
> There are a number of bugs and style issues.
> 1) Find and replace every -1 with a define or a proper error code
> 2) Never do assignments inside of a condition
> 3) Do success handling instead of error handling.  Try to return errors

I meant the other way around.  Do error handling.  If you return an
error directly then code looks like this:

	success;
	if (fail)
		cleanup;
	success;
	success;
	if (fail)
		more cleanup;

Otherwise if you have two successes then it starts getting indented a
lot.

	success;
	if (success)
		success;
		success;
		if (success)
			success;
		else
			more cleanup;
	else
		cleanup;

Pasta.

regards,
dan carpenter

Re: [RFC] staging/vSMP: new driver

[Czerwacki, Eial]

Greetings Dan,

>"new driver" is too vague.  What is the driver for?
the first line limitation makes it harder to describe it properly, I'll try to find a more
informative line

>
>You need a README or a TODO which explains how to move the driver out of
>staging and into the main kernel.
already discussed this with Greg, he pointed out that after I fix all the
rejects, I should post it in the lkml as proper driver

>
>Run the patch through scripts/checkpatch.pl --strict.  You don't have
>to fix everything because this is staging but a bunch of the style
>issues are easy to fix so we may as well just do it.
I'll run it and fix resulting issues

>
>There are a number of bugs and style issues.
>1) Find and replace every -1 with a define or a proper error code
>2) Never do assignments inside of a condition
>3) Do success handling instead of error handling.  Try to return errors
>   as directly as possible.
>
>Bad:
>        ret = frob();
>        if (!ret)
>                printk("success!");
>        return ret;
>
>Good:
>        ret = frob();
>        if (ret)
>                return ret;
>        printk("success!");
>        return 0;
>
>Keep scrolling down for the details.
sure thing, I'll go over the issues you've mentioned above and fix.

>
>On Wed, Mar 16, 2022 at 06:13:04PM +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.
>> 
>> 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>
>> ---
>>  MAINTAINERS                             |   6 +
>>  drivers/staging/Kconfig                 |   2 +
>>  drivers/staging/Makefile                |   1 +
>>  drivers/staging/vsmp/Kconfig            |  14 +
>>  drivers/staging/vsmp/Makefile           |   7 +
>>  drivers/staging/vsmp/api.c              | 402 ++++++++++++++++++++++++
>>  drivers/staging/vsmp/api.h              |  61 ++++
>>  drivers/staging/vsmp/common/Kconfig     |  11 +
>>  drivers/staging/vsmp/common/Makefile    |   7 +
>>  drivers/staging/vsmp/common/common.c    |  64 ++++
>>  drivers/staging/vsmp/common/common.h    |  27 ++
>>  drivers/staging/vsmp/common/version.c   |  85 +++++
>>  drivers/staging/vsmp/logs/Kconfig       |  10 +
>>  drivers/staging/vsmp/logs/Makefile      |   7 +
>>  drivers/staging/vsmp/logs/active_logs.c | 112 +++++++
>>  drivers/staging/vsmp/registers.h        |  16 +
>>  16 files changed, 832 insertions(+)
>>  create mode 100644 drivers/staging/vsmp/Kconfig
>>  create mode 100644 drivers/staging/vsmp/Makefile
>>  create mode 100644 drivers/staging/vsmp/api.c
>>  create mode 100644 drivers/staging/vsmp/api.h
>>  create mode 100644 drivers/staging/vsmp/common/Kconfig
>>  create mode 100644 drivers/staging/vsmp/common/Makefile
>>  create mode 100644 drivers/staging/vsmp/common/common.c
>>  create mode 100644 drivers/staging/vsmp/common/common.h
>>  create mode 100644 drivers/staging/vsmp/common/version.c
>>  create mode 100644 drivers/staging/vsmp/logs/Kconfig
>>  create mode 100644 drivers/staging/vsmp/logs/Makefile
>>  create mode 100644 drivers/staging/vsmp/logs/active_logs.c
>>  create mode 100644 drivers/staging/vsmp/registers.h
>> 
>> diff --git a/MAINTAINERS b/MAINTAINERS
>> index 68f21d46614c..2fc61b4d13e5 100644
>> --- a/MAINTAINERS
>> +++ b/MAINTAINERS
>> @@ -18381,6 +18381,12 @@ F:   Documentation/core-api/printk-formats.rst
>>  F:   lib/test_printf.c
>>  F:   lib/vsprintf.c
>>  
>> +VSMP GUEST DRIVER
>> +M:     Eial Czerwacki <eial.czerwacki@sap.com>
>> +M:     linux-vsmp@sap.com
>> +S:     Maintained
>> +F:     drivers/staging/vsmp-guest 
>> +
>>  VT1211 HARDWARE MONITOR DRIVER
>>  M:   Juerg Haefliger <juergh@gmail.com>
>>  L:   linux-hwmon@vger.kernel.org
>> diff --git a/drivers/staging/Kconfig b/drivers/staging/Kconfig
>> index 4ec5528f89fa..f61778bccb0b 100644
>> --- a/drivers/staging/Kconfig
>> +++ b/drivers/staging/Kconfig
>> @@ -114,6 +114,8 @@ source "drivers/staging/axis-fifo/Kconfig"
>>  
>>  source "drivers/staging/fieldbus/Kconfig"
>>  
>> +source "drivers/staging/vsmp/Kconfig"
>> +
>>  source "drivers/staging/kpc2000/Kconfig"
>>  
>>  source "drivers/staging/qlge/Kconfig"
>> diff --git a/drivers/staging/Makefile b/drivers/staging/Makefile
>> index 4d34198151b3..726e704ba8bc 100644
>> --- a/drivers/staging/Makefile
>> +++ b/drivers/staging/Makefile
>> @@ -47,6 +47,7 @@ obj-$(CONFIG_SOC_MT7621)    += mt7621-dts/
>>  obj-$(CONFIG_STAGING_GASKET_FRAMEWORK)       += gasket/
>>  obj-$(CONFIG_XIL_AXIS_FIFO)  += axis-fifo/
>>  obj-$(CONFIG_FIELDBUS_DEV)     += fieldbus/
>> +obj-$(CONFIG_VSMP)       += vsmp/
>>  obj-$(CONFIG_KPC2000)                += kpc2000/
>>  obj-$(CONFIG_QLGE)           += qlge/
>>  obj-$(CONFIG_WFX)            += wfx/
>> diff --git a/drivers/staging/vsmp/Kconfig b/drivers/staging/vsmp/Kconfig
>> new file mode 100644
>> index 000000000000..c57cfcb55033
>> --- /dev/null
>> +++ b/drivers/staging/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 drivers allows information gathering of data from the vSMP hypervisor when
>> +       running ontop of a vSMP based VM.
>> +
>> +       If unsure, say no.
>> +
>> +source "drivers/staging/vsmp/common/Kconfig"
>> +source "drivers/staging/vsmp/logs/Kconfig"
>> diff --git a/drivers/staging/vsmp/Makefile b/drivers/staging/vsmp/Makefile
>> new file mode 100644
>> index 000000000000..88625cd3707d
>> --- /dev/null
>> +++ b/drivers/staging/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/staging/vsmp/api.c b/drivers/staging/vsmp/api.c
>> new file mode 100644
>> index 000000000000..b0d4b5a990d4
>> --- /dev/null
>> +++ b/drivers/staging/vsmp/api.c
>> @@ -0,0 +1,402 @@
>> +/* 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 DRIVER_LICENSE "GPL"
>> +#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 = 0;
>> +static unsigned short vsmp_dev = 0x1f;
>> +static unsigned char vsmp_func = 0;
>> +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))
>
>Put the operator on the first line.
>
>> +
>> +/* global vars */
>> +void __iomem *cfg_addr = NULL;
>> +static struct kobject *vsmp_sysfs_kobj;
>> +static struct pci_dev *vsmp_dev_obj = NULL;
>> +
>> +static const struct pci_device_id 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)
>> +{
>> +     if (strlen(vsmp_ctrl_pci_addr)) {
>
>Reverse this condition.
>
>        if (strlen(vsmp_ctrl_pci_addr) != 0)
>                return;
>
will do.

>> +             unsigned int bus;
>> +             unsigned short dev;
>> +             unsigned char func;
>> +
>> +             if (sscanf(vsmp_ctrl_pci_addr, "%04x:%02hx.%1hhx", &bus, &dev,
>> +                        &func) == 3) {
>
>Same:
>
>        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 specfic device in order to determine if it is a vSMP pci device
>> + */
>> +static void query_pci_bus_for_vsmp_directly(unsigned int default_bus,
>> +                                         unsigned int devfn)
>> +{
>> +     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);
>
>Doesn't this need to set mapped_bars[0].len as well?
>
>> +}
>> +
>> +/**
>> + * scan all the devices on the system in order to locate the
>> + * vSMP pic device for a given dev end func is provided
>> + */
>> +static void scan_pci_devs_list_for_vsmp(unsigned int devfn)
>
>This function should return negatives if we do not find a device.
>The current error hanlding is convoluted and relies on globals.
>
>> +{
>> +     int found, i;
>
>"found" is never set to false.  Make it a bool.
>
>> +     struct pci_dev *pdev = NULL;
>
>Do not initialize "pdev" to a bogus value.
>
>> +     const struct pci_device_id *ent;
>> +
>> +     for_each_pci_dev(pdev) {
>> +             if (devfn != -1) {
>
>Make this -1 a define instead of a magic number.
>
>> +                     if ((pdev->bus->number == vsmp_bus) &&
>> +                         (devfn == pdev->devfn)) {
>> +                             found = 1;
>> +                             break;
>> +                     }
>> +             } else {
>> +                     ent = pci_match_id(pci_tbl, pdev);
>> +                     if (ent) {
>> +                             found = 1;
>> +                             vsmp_bus = pdev->bus->number;
>> +                             vsmp_dev = PCI_SLOT(pdev->devfn);
>> +                             vsmp_func = PCI_FUNC(pdev->devfn);
>> +                             break;
>> +                     }
>> +             }
>> +     }
>
>        if (!found)
>                return -ENODEV;
>
according to Greg, I should use the probe callback in the ops struct. that
will provide me with the proper dev struct without the need to scan anything

>> +
>> +     for (i = 0; (i < ARRAY_SIZE(mapped_bars)) && found; i++) {
>> +             mapped_bars[i].start = pci_resource_start(pdev, i);
>> +             mapped_bars[i].len = pci_resource_len(pdev, i);
>> +     }
>> +     vsmp_dev_obj = pdev;
>> +}
>> +
>> +/**
>> + * open the cfg address space of the vSDP device
>> + */
>> +static int open_cfg_addr(void)
>> +{
>> +     unsigned int devfn = (vsmp_bdf_static) ? PCI_DEVFN(vsmp_dev, vsmp_func)
>> +                                             : -1;
>> +
>> +     scan_pci_devs_list_for_vsmp(devfn);
>> +     if (!mapped_bars[0].start)
>> +             query_pci_bus_for_vsmp_directly(0, devfn);
>> +
>> +     if (!mapped_bars[0].len) {
>> +             printk(KERN_INFO "vSMP pci controller not found, exiting.\n");
>> +             return -1;
>
>Return proper kernel error codes.  "return -EINVAL;"
probably missed it when I was fixing such issues. will fix.

>
>> +     }
>> +
>> +     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) {
>
>Delete the blank link between the call and the error checking.  They are
>part of the same thing.
did you meant line?

>
>> +             printk(KERN_ERR
>> +                    "failed to map vSMP pci controller at %x:%x.%x, exiting.\n",
>> +                    vsmp_bus, vsmp_dev, vsmp_func);
>> +             return -1;
>> +     }
>> +
>> +     return 0;
>> +}
>> +
>> +/* exported functions */
>> +
>> +/**
>> + * read a value from a specfic register in the vSMP's device config space
>> + */
>> +unsigned long vsmp_read_reg_from_cfg(unsigned int reg, enum reg_size_type type)
>
>
>Use u64 when 64 bit types are intended.
>
>> +{
>> +     unsigned long ret_val;
>
>
>u64 ret_val;
sure

>
>> +
>> +     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 = (unsigned long)(-1);
>
>
>64 bit types.  "ret_val = -1ULL;"
>
>
>> +     }
>> +
>> +     dev_dbg(&vsmp_dev_obj->dev, "%s: read 0x%lx from reg 0x%x 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 specfic register in the vSMP's device config space
>> + */
>> +void vsmp_write_reg_to_cfg(unsigned int reg, unsigned long value,
>> +                        enum reg_size_type type)
>> +{
>> +     switch (type) {
>> +     case VSMP_CTL_REG_SIZE_8BIT:
>> +             writeb((unsigned char)value, cfg_addr + reg);
>> +             break;
>> +
>> +     case VSMP_CTL_REG_SIZE_16BIT:
>> +             writew((unsigned short)value, cfg_addr + reg);
>> +             break;
>> +
>> +     case VSMP_CTL_REG_SIZE_32BIT:
>> +             writel((unsigned int)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%x to reg 0x%x of %u bits\n",
>> +             __func__, reg, reg, (type + 1) * 8);
>> +}
>> +EXPORT_SYMBOL_GPL(vsmp_write_reg_to_cfg);
>> +
>> +/**
>> + * reag a buffer from a specific offset in a specific bar, maxed to a predefined len size-wise from the vSMP device
>> + */
>> +unsigned int vsmp_read_buff_from_bar(unsigned int bar, unsigned int offset,
>
>This is unsigned int but it returns zero or negative error codes.
>
>Create a separate function for "halt_on_null".  It will be cleaner that
>way.
I'll refractor this code properly.

>
>> +                                  char *out, unsigned int len,
>> +                                  bool halt_on_null)
>> +{
>> +     unsigned char __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) {
>> +             int i;
>> +             unsigned char c;
>> +
>> +             for (i = 0; i < len; i++) {
>> +                     c = ioread8(&buff[i]);
>> +                     if (!c)
>> +                             break;
>> +                     out[i] = c;
>
>Copy the NUL terminator to out[i] before the break.
>
should I memset out to zero instead?

>> +             }
>> +     } 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 = -1;
>> +
>> +     if (vsmp_sysfs_kobj && bin_attr) {
>> +             if ((error = sysfs_create_bin_file(vsmp_sysfs_kobj, bin_attr)))
>> +                     printk(KERN_CRIT "%s returned error %d\n", __func__,
>> +                            error);
>> +     }
>> +
>> +     return error;
>> +}
>> +EXPORT_SYMBOL_GPL(vsmp_register_sysfs_group);
>> +
>> +/**
>> + * deergister 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, unsigned bar, unsigned reg,
>> +                            char *buf, loff_t off, size_t count)
>> +{
>> +     ssize_t ret_val = 0;
>> +
>> +     if (op->buff_offset >= op->hwi_block_size) {    // preform H/W op
>> +             vsmp_reset_op(op);
>> +
>> +             if ((ret_val = vsmp_read_buff_from_bar(bar, reg, op->buff,
>> +                                                    op->hwi_block_size, false))) {
>> +                     printk(KERN_ERR "%s operation failed\n",
>> +                            (op->action == FW_READ) ? "read" : "write");
>> +             }
>> +             op->buff_offset = 0;
>> +     }
>> +
>> +     if (!ret_val) {
>
>Flip this around.  Do error handling.  Don't do success handling.
>
>> +             ret_val = memory_read_from_buffer(buf, count, &op->buff_offset,
>> +                                               op->buff, op->hwi_block_size);
>> +     }
>> +
>> +     return ret_val;
>> +}
>> +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;
>> +
>> +     if (!(op->buff = kzalloc(op->hwi_block_size, GFP_KERNEL)))
>> +             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 __init vsmp_api_init(void)
>> +{
>> +     int ret_val;
>> +
>> +     parse_vsmp_ctrl_pci_addr();
>> +
>> +     if (!(ret_val = open_cfg_addr())) {
>
>Error handling not success handling.
>
>> +             printk(KERN_INFO "%s [%02x:%02x.%x] up and running\n",
>> +                    DRIVER_DESC, vsmp_bus, vsmp_dev, vsmp_func);
>> +
>> +             if (!(vsmp_sysfs_kobj = kobject_create_and_add("vsmp",
>> +                                                             hypervisor_kobj))) {
>
>Reverse.
>
>> +                     printk(KERN_ERR
>> +                            "failed to create sysfs entry for vSMP pci controller at %x:%x.%x, exiting.\n",
>> +                            vsmp_bus, vsmp_dev, vsmp_func);
>> +                     ret_val = -1;
>> +             }
>> +     }
>> +
>> +     return ret_val;
>> +}
>> +
>> +/**
>> + * cleanup after the module upon exit
>> + */
>> +static void __exit vsmp_api_exit(void)
>> +{
>> +     if (cfg_addr)
>> +             iounmap(cfg_addr);
>> +
>> +     if (vsmp_sysfs_kobj)
>> +             kobject_put(vsmp_sysfs_kobj);
>> +}
>> +
>> +module_init(vsmp_api_init);
>> +module_exit(vsmp_api_exit);
>> diff --git a/drivers/staging/vsmp/api.h b/drivers/staging/vsmp/api.h
>> new file mode 100644
>> index 000000000000..88d81b80bd81
>> --- /dev/null
>> +++ b/drivers/staging/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_) ((unsigned char) vsmp_read_reg_from_cfg((_reg_), VSMP_CTL_REG_SIZE_8BIT))
>> +#define vsmp_read_reg16_from_cfg(_reg_) ((unsigned short) vsmp_read_reg_from_cfg((_reg_), VSMP_CTL_REG_SIZE_16BIT))
>> +#define vsmp_read_reg32_from_cfg(_reg_) ((unsigned int) vsmp_read_reg_from_cfg((_reg_), VSMP_CTL_REG_SIZE_32BIT))
>> +#define vsmp_read_reg64_from_cfg(_reg_) ((unsigned long) 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)
>> +
>> +unsigned long vsmp_read_reg_from_cfg(unsigned int reg, enum reg_size_type type);
>> +void vsmp_write_reg_to_cfg(unsigned int reg, unsigned long value,
>> +                        enum reg_size_type type);
>> +unsigned int vsmp_read_buff_from_bar(unsigned int bar, unsigned int offset,
>> +                                  char *out, unsigned int 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, unsigned bar, unsigned reg,
>> +                            char *buf, loff_t off, size_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/staging/vsmp/common/Kconfig b/drivers/staging/vsmp/common/Kconfig
>> new file mode 100644
>> index 000000000000..fe0cfeaca1bb
>> --- /dev/null
>> +++ b/drivers/staging/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 retrive generic data from the vSMP
>> +       VM.
>> +       currently old version is supported.
>> +       the data can be found under /sys/hypervisor/vsmp
>> +
>> +       If unsure, say N.
>> diff --git a/drivers/staging/vsmp/common/Makefile b/drivers/staging/vsmp/common/Makefile
>> new file mode 100644
>> index 000000000000..e6f6aa1d73df
>> --- /dev/null
>> +++ b/drivers/staging/vsmp/common/Makefile
>> @@ -0,0 +1,7 @@
>> +# SPDX-License-Identifier: GPL-2.0
>> +#
>> +# Makefile for vSMP Guest common feature.
>> +#
>> +
>> +obj-$(CONFIG_VSMP_COMMON)    += common_info.o 
>> +common_info-y                += common.o version.o
>> diff --git a/drivers/staging/vsmp/common/common.c b/drivers/staging/vsmp/common/common.c
>> new file mode 100644
>> index 000000000000..31e6551f2b7f
>> --- /dev/null
>> +++ b/drivers/staging/vsmp/common/common.c
>> @@ -0,0 +1,64 @@
>> +/* 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"
>> +#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++) {
>> +             if ((ret_val = cbs_arr[i].reg_cb()))
>> +                     printk(KERN_ERR "failed to init sysfs (%d), exiting.\n",
>> +                            ret_val);
>> +     }
>> +
>> +     return -1 * ret_val;
>
>sysfs_register_version_cb() already returns negatives.  Why the "-1 *"?
leftovers from prior iteration of the driver

>
>> +}
>> +
>> +/**
>> + * 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/staging/vsmp/common/common.h b/drivers/staging/vsmp/common/common.h
>> new file mode 100644
>> index 000000000000..08a096628c95
>> --- /dev/null
>> +++ b/drivers/staging/vsmp/common/common.h
>> @@ -0,0 +1,27 @@
>> +/* SPDX-License-Identifier: GPL-2.0 */
>> +/*
>> + * vSMP common header
>> + * Copyright (C) 2022 SAP.
>> + */
>> +
>> +#ifndef VSM_COMMON_H
>> +#define VSM_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 // !VSM_COMMON_H
>> diff --git a/drivers/staging/vsmp/common/version.c b/drivers/staging/vsmp/common/version.c
>> new file mode 100644
>> index 000000000000..35b5a7785e9a
>> --- /dev/null
>> +++ b/drivers/staging/vsmp/common/version.c
>> @@ -0,0 +1,85 @@
>> +/* 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"
>> +
>> +#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)
>> +{
>> +     ssize_t ret_val = vsmp_generic_buff_read(&op, 0,
>> +                                              vsmp_read_reg32_from_cfg(VSMP_VERSION_REG),
>> +                                              buf, off, count);
>
>Don't put functions which can fail in the declaration block.
>
>        ret_val = vsmp_generic_buff_read(&op, 0,
>                                         vsmp_read_reg32_from_cfg(VSMP_VERSION_REG),
>                                         buf, off, count);
>
can you explain what is the reason behind this recommendation?

>> +
>> +     if ((ret_val != -ENOMEM) && ((ret_val != -EINVAL)))
>
>Don't test for specific error codes.
>
>        if (ret_val < 0)
>                return ret_val;
>
>
now I understand, there was a patchset which fixes for all the -1 and the specific ret_val testing, I see it
was misplaced at some point of the work.
thanks for pointing it out

>> +         buf[ret_val++] = '\n';
>> +
>> +     return ret_val;
>> +}
>> +
>> +struct bin_attribute version_raw_attr = __BIN_ATTR(version, FILE_PREM, version_read, NULL, VERSION_MAX_LEN);
>> +
>> +/**
>> + * retrive str in order to determine the proper length.
>> + * this is the best way to maintain backwards compatibility with all
>> + * vSMP versions.
>> + */
>> +static int get_version_len(void)
>> +{
>> +     unsigned reg;
>> +     char *version_str = kzalloc(VERSION_MAX_LEN, GFP_ATOMIC | __GFP_NOWARN);
>
>VERSION_MAX_LEN is 524288 bytes.  That's too long.  Create a small
>buffer and loop over it until you find the NUL terminator.  Why does
>this need to be ATOMIC, are we holding a lock?  Hopefully if we can fix
>the length the __GFP_NOWARN will be unnecessary.
I'm not sure it is possible to loopover, I'll take that into consideration.
in this flow, only one access is possible, so I assume GFP_ATOMIC is not needed

>
>> +
>> +     if (!version_str)
>> +             return -1;
>> +
>> +     reg = vsmp_read_reg32_from_cfg(VSMP_VERSION_REG);
>> +     memset(version_str, 0, VERSION_MAX_LEN);
>> +
>> +     if (vsmp_read_buff_from_bar(0, reg, version_str, VERSION_MAX_LEN, true)) {
>> +             printk(KERN_ERR "failed to read buffer from bar\n");
>> +             return -1;
>> +     }
>> +
>> +     version_raw_attr.size = strlen(version_str);
>
>get_version_len() should return the length instead of setting a global.
do you mean version_raw_attr.size = get_version_len()?

>
>> +     kfree(version_str);
>> +
>> +     return 0;
>> +}
>> +
>> +/**
>> + * register the version sysfs entry
>> + */
>> +int sysfs_register_version_cb(void)
>> +{
>> +     if (get_version_len()) {
>> +             printk(KERN_ERR "failed to init vSMP version module\n");
>> +             return -1;
>> +     }
>> +
>> +     if (!vsmp_init_op(&op, version_raw_attr.size, FW_READ)) {
>
>This if statement is reversed so this code can never work.
not sure I follow, can you explain?

>
>> +             printk(KERN_ERR "failed to init vSMP version op\n");
>> +             return -1;
>> +     }
>> +
>> +     return vsmp_register_sysfs_group(&version_raw_attr);
>> +}
>> +
>> +/**
>> + * 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/staging/vsmp/logs/Kconfig b/drivers/staging/vsmp/logs/Kconfig
>> new file mode 100644
>> index 000000000000..d41491472bab
>> --- /dev/null
>> +++ b/drivers/staging/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 retrive logs from the vSMP
>> +       VM.
>> +       the data can be found under /sys/hypervisor/logs
>> +
>> +       If unsure, say N.
>> diff --git a/drivers/staging/vsmp/logs/Makefile b/drivers/staging/vsmp/logs/Makefile
>> new file mode 100644
>> index 000000000000..edc679803fe6
>> --- /dev/null
>> +++ b/drivers/staging/vsmp/logs/Makefile
>> @@ -0,0 +1,7 @@
>> +# SPDX-License-Identifier: GPL-2.0
>> +#
>> +# Makefile for vSMP Guest common feature.
>> +#
>> +
>> +obj-$(CONFIG_VSMP_LOGS)      += logs.o 
>> +logs-y               += active_logs.o
>> diff --git a/drivers/staging/vsmp/logs/active_logs.c b/drivers/staging/vsmp/logs/active_logs.c
>> new file mode 100644
>> index 000000000000..91492c72cc6f
>> --- /dev/null
>> +++ b/drivers/staging/vsmp/logs/active_logs.c
>> @@ -0,0 +1,112 @@
>> +/* 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"
>> +#define DRIVER_AUTHOR "Eial Czerwacki <eial.czerwacki@sap.com>"
>> +#define DRIVER_DESC "vSMP 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);
>
>Just put:
>
>MODULE_LICENSE("GPL v2");
>
>Adding unnecessary indirection is against kernel style.  It says v2 at
>the top.
>
>> +
>> +static struct fw_ops op;
>> +static unsigned log_start;
>> +DEFINE_MUTEX(log_mutex);
>> +
>> +/* module functions */
>> +static ssize_t 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) {
>> +             if ((ret_val = mutex_lock_interruptible(&log_mutex))) {
>
>Don't do assignments inside of an if condition.
>
>> +                     printk(KERN_ERR
>> +                            "error in atemmpt to lock mutex (%lx)\n",
>> +                            ret_val);
>> +                     return ret_val;
>> +             }
>> +
>> +             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(&log_mutex);
>> +                     return ret_val;
>
>Return a literal when possible. "return 0;" is more clear than
>"return ret_val;".
>
>> +             }
>> +     }
>> +
>> +     ret_val = vsmp_generic_buff_read(&op, 1, log_start, buf, off, count);
>> +     if (!ret_val || (ret_val == -ENOMEM) || (ret_val == -EINVAL)) {
>> +             printk(KERN_ERR "error reading from buffer\n");
>> +             mutex_unlock(&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 log_raw_attr = __BIN_ATTR(log, FILE_PREM, log_read, NULL, 0);
>> +
>> +/**
>> + * init the log module
>> + */
>> +static int __init vsmp_logs_init(void)
>> +{
>> +     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 -1;
>> +     }
>> +
>> +     log_start = vsmp_read_reg32_from_cfg(VSMP_LOGS_START_REG);
>> +     mutex_init(&log_mutex);
>> +
>> +     return vsmp_register_sysfs_group(&log_raw_attr);
>
>If vsmp_register_sysfs_group() fails then it needs to vsmp_release_op().
thanks, will fix

>
>I have written a lot about how to write error handling code:
>https://eur03.safelinks.protection.outlook.com/?url=https%3A%2F%2Flore.kernel.org%2Fall%2F20210831084735.GL12231%40kadam%2F&amp;data=04%7C01%7Ceial.czerwacki%40sap.com%7Cf1e83fe49d074f7552da08da07ffb8e1%7C42f7676cf455423c82f6dc2d99791af7%7C0%7C0%7C637831092157869913%7CUnknown%7CTWFpbGZsb3d8eyJWIjoiMC4wLjAwMDAiLCJQIjoiV2luMzIiLCJBTiI6Ik1haWwiLCJXVCI6Mn0%3D%7C3000&amp;sdata=C5q%2BpdsckU%2FAo%2BKu2t63n6sMRv%2FGx5HID0oVYS3ZxEI%3D&amp;reserved=0
>
I'll read the link above,.

Thanks for your remarks.

Eial

Re: [RFC] staging/vSMP: new driver

[Dan Carpenter]

On Thu, Mar 17, 2022 at 01:41:30PM +0000, Czerwacki, Eial wrote:
> >> +     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) {
> >
> >Delete the blank link between the call and the error checking.  They are
> >part of the same thing.
> did you meant line?
> 

What I mean is that it's like paragraphs.

	cfg_addr = ioremap(mapped_bars[0].start, mapped_bars[0].len);
	if (!cfg_addr)
		return -ENOMEM;

The call and the check go together.

> >
> >> +             printk(KERN_ERR
> >> +                    "failed to map vSMP pci controller at %x:%x.%x, exiting.\n",
> >> +                    vsmp_bus, vsmp_dev, vsmp_func);
> >> +             return -1;
> >> +     }
> >> +
> >> +     return 0;
> >> +}
> >> +
> >> +/* exported functions */
> >> +
> >> +/**
> >> + * read a value from a specfic register in the vSMP's device config space
> >> + */
> >> +unsigned long vsmp_read_reg_from_cfg(unsigned int reg, enum reg_size_type type)
> >
> >
> >Use u64 when 64 bit types are intended.
> >
> >> +{
> >> +     unsigned long ret_val;
> >
> >
> >u64 ret_val;
> sure
> 
> >
> >> +
> >> +     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 = (unsigned long)(-1);
> >
> >
> >64 bit types.  "ret_val = -1ULL;"
> >
> >
> >> +     }
> >> +
> >> +     dev_dbg(&vsmp_dev_obj->dev, "%s: read 0x%lx from reg 0x%x 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 specfic register in the vSMP's device config space
> >> + */
> >> +void vsmp_write_reg_to_cfg(unsigned int reg, unsigned long value,
> >> +                        enum reg_size_type type)
> >> +{
> >> +     switch (type) {
> >> +     case VSMP_CTL_REG_SIZE_8BIT:
> >> +             writeb((unsigned char)value, cfg_addr + reg);
> >> +             break;
> >> +
> >> +     case VSMP_CTL_REG_SIZE_16BIT:
> >> +             writew((unsigned short)value, cfg_addr + reg);
> >> +             break;
> >> +
> >> +     case VSMP_CTL_REG_SIZE_32BIT:
> >> +             writel((unsigned int)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%x to reg 0x%x of %u bits\n",
> >> +             __func__, reg, reg, (type + 1) * 8);
> >> +}
> >> +EXPORT_SYMBOL_GPL(vsmp_write_reg_to_cfg);
> >> +
> >> +/**
> >> + * reag a buffer from a specific offset in a specific bar, maxed to a predefined len size-wise from the vSMP device
> >> + */
> >> +unsigned int vsmp_read_buff_from_bar(unsigned int bar, unsigned int offset,
> >
> >This is unsigned int but it returns zero or negative error codes.
> >
> >Create a separate function for "halt_on_null".  It will be cleaner that
> >way.
> I'll refractor this code properly.
> 
> >
> >> +                                  char *out, unsigned int len,
> >> +                                  bool halt_on_null)
> >> +{
> >> +     unsigned char __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) {
> >> +             int i;
> >> +             unsigned char c;
> >> +
> >> +             for (i = 0; i < len; i++) {
> >> +                     c = ioread8(&buff[i]);
> >> +                     if (!c)
> >> +                             break;
> >> +                     out[i] = c;
> >
> >Copy the NUL terminator to out[i] before the break.
> >
> should I memset out to zero instead?

Right now you're relying on the caller to set the NUL terminator and
that's ugly.  Just do:

		for (i = 0; i < len; i++) {
			c = ioread8(&buff[i]);
			out[i] = c;
			if (!c)
				break;


> 
> >> +             }
> >> +     } else
> >> +             memcpy_fromio(out, buff, len);
> >> +
> >> +     iounmap(buff);
> >> +
> >> +     return 0;
> >> +}

[ snip ]

> >> +static ssize_t version_read(struct file *filp, struct kobject *kobj,
> >> +                         struct bin_attribute *bin_attr,
> >> +                         char *buf, loff_t off, size_t count)
> >> +{
> >> +     ssize_t ret_val = vsmp_generic_buff_read(&op, 0,
> >> +                                              vsmp_read_reg32_from_cfg(VSMP_VERSION_REG),
> >> +                                              buf, off, count);
> >
> >Don't put functions which can fail in the declaration block.
> >
> >        ret_val = vsmp_generic_buff_read(&op, 0,
> >                                         vsmp_read_reg32_from_cfg(VSMP_VERSION_REG),
> >                                         buf, off, count);
> >
> can you explain what is the reason behind this recommendation?
> 

That's just my own preference, not really a rule.  I stole that
guideline from someone else.  I forget who.  But it's a fact that the
declaration block is more bug prone than other code.  (Very clean in
static analysis results).  Plus it means that you put a blank line
between the call and the check which I do not like (again just my
preference).


> >> +
> >> +     if ((ret_val != -ENOMEM) && ((ret_val != -EINVAL)))
> >
> >Don't test for specific error codes.
> >
> >        if (ret_val < 0)
> >                return ret_val;
> >
> >
> now I understand, there was a patchset which fixes for all the -1 and the specific ret_val testing, I see it
> was misplaced at some point of the work.
> thanks for pointing it out
> 
> >> +         buf[ret_val++] = '\n';
> >> +
> >> +     return ret_val;
> >> +}
> >> +
> >> +struct bin_attribute version_raw_attr = __BIN_ATTR(version, FILE_PREM, version_read, NULL, VERSION_MAX_LEN);
> >> +
> >> +/**
> >> + * retrive str in order to determine the proper length.
> >> + * this is the best way to maintain backwards compatibility with all
> >> + * vSMP versions.
> >> + */
> >> +static int get_version_len(void)
> >> +{
> >> +     unsigned reg;
> >> +     char *version_str = kzalloc(VERSION_MAX_LEN, GFP_ATOMIC | __GFP_NOWARN);
> >
> >VERSION_MAX_LEN is 524288 bytes.  That's too long.  Create a small
> >buffer and loop over it until you find the NUL terminator.  Why does
> >this need to be ATOMIC, are we holding a lock?  Hopefully if we can fix
> >the length the __GFP_NOWARN will be unnecessary.
> I'm not sure it is possible to loopover, I'll take that into consideration.
> in this flow, only one access is possible, so I assume GFP_ATOMIC is not needed
> 

This get_version_len() is cray cray...  It needs to loop.

> >
> >> +
> >> +     if (!version_str)
> >> +             return -1;
> >> +
> >> +     reg = vsmp_read_reg32_from_cfg(VSMP_VERSION_REG);
> >> +     memset(version_str, 0, VERSION_MAX_LEN);
> >> +
> >> +     if (vsmp_read_buff_from_bar(0, reg, version_str, VERSION_MAX_LEN, true)) {
> >> +             printk(KERN_ERR "failed to read buffer from bar\n");
> >> +             return -1;
> >> +     }
> >> +
> >> +     version_raw_attr.size = strlen(version_str);
> >
> >get_version_len() should return the length instead of setting a global.
> do you mean version_raw_attr.size = get_version_len()?
> 

Yes, but with negative error codes.

> >
> >> +     kfree(version_str);
> >> +
> >> +     return 0;
> >> +}
> >> +
> >> +/**
> >> + * register the version sysfs entry
> >> + */
> >> +int sysfs_register_version_cb(void)
> >> +{
> >> +     if (get_version_len()) {
> >> +             printk(KERN_ERR "failed to init vSMP version module\n");
> >> +             return -1;
> >> +     }
> >> +
> >> +     if (!vsmp_init_op(&op, version_raw_attr.size, FW_READ)) {
> >
> >This if statement is reversed so this code can never work.
> not sure I follow, can you explain?
> 

The vsmp_init_op() function returns zero on success and this code treats
success as failure.

> >
> >> +             printk(KERN_ERR "failed to init vSMP version op\n");
> >> +             return -1;
> >> +     }
> >> +
> >> +     return vsmp_register_sysfs_group(&version_raw_attr);
> >> +}

regards,
dan carpenter

Re: [RFC] staging/vSMP: new driver

[Czerwacki, Eial]

>On Thu, Mar 17, 2022 at 01:41:30PM +0000, Czerwacki, Eial wrote:
>> >> +     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) {
>> >
>> >Delete the blank link between the call and the error checking.  They are
>> >part of the same thing.
>> did you meant line?
>> 
>
>What I mean is that it's like paragraphs.
>
>        cfg_addr = ioremap(mapped_bars[0].start, mapped_bars[0].len);
>        if (!cfg_addr)
>                return -ENOMEM;
>
>The call and the check go together.
you are correct

>
>> >
>> >> +             printk(KERN_ERR
>> >> +                    "failed to map vSMP pci controller at %x:%x.%x, exiting.\n",
>> >> +                    vsmp_bus, vsmp_dev, vsmp_func);
>> >> +             return -1;
>> >> +     }
>> >> +
>> >> +     return 0;
>> >> +}
>> >> +
>> >> +/* exported functions */
>> >> +
>> >> +/**
>> >> + * read a value from a specfic register in the vSMP's device config space
>> >> + */
>> >> +unsigned long vsmp_read_reg_from_cfg(unsigned int reg, enum reg_size_type type)
>> >
>> >
>> >Use u64 when 64 bit types are intended.
>> >
>> >> +{
>> >> +     unsigned long ret_val;
>> >
>> >
>> >u64 ret_val;
>> sure
>> 
>> >
>> >> +
>> >> +     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 = (unsigned long)(-1);
>> >
>> >
>> >64 bit types.  "ret_val = -1ULL;"
>> >
>> >
>> >> +     }
>> >> +
>> >> +     dev_dbg(&vsmp_dev_obj->dev, "%s: read 0x%lx from reg 0x%x 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 specfic register in the vSMP's device config space
>> >> + */
>> >> +void vsmp_write_reg_to_cfg(unsigned int reg, unsigned long value,
>> >> +                        enum reg_size_type type)
>> >> +{
>> >> +     switch (type) {
>> >> +     case VSMP_CTL_REG_SIZE_8BIT:
>> >> +             writeb((unsigned char)value, cfg_addr + reg);
>> >> +             break;
>> >> +
>> >> +     case VSMP_CTL_REG_SIZE_16BIT:
>> >> +             writew((unsigned short)value, cfg_addr + reg);
>> >> +             break;
>> >> +
>> >> +     case VSMP_CTL_REG_SIZE_32BIT:
>> >> +             writel((unsigned int)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%x to reg 0x%x of %u bits\n",
>> >> +             __func__, reg, reg, (type + 1) * 8);
>> >> +}
>> >> +EXPORT_SYMBOL_GPL(vsmp_write_reg_to_cfg);
>> >> +
>> >> +/**
>> >> + * reag a buffer from a specific offset in a specific bar, maxed to a predefined len size-wise from the vSMP device
>> >> + */
>> >> +unsigned int vsmp_read_buff_from_bar(unsigned int bar, unsigned int offset,
>> >
>> >This is unsigned int but it returns zero or negative error codes.
>> >
>> >Create a separate function for "halt_on_null".  It will be cleaner that
>> >way.
>> I'll refractor this code properly.
>> 
>> >
>> >> +                                  char *out, unsigned int len,
>> >> +                                  bool halt_on_null)
>> >> +{
>> >> +     unsigned char __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) {
>> >> +             int i;
>> >> +             unsigned char c;
>> >> +
>> >> +             for (i = 0; i < len; i++) {
>> >> +                     c = ioread8(&buff[i]);
>> >> +                     if (!c)
>> >> +                             break;
>> >> +                     out[i] = c;
>> >
>> >Copy the NUL terminator to out[i] before the break.
>> >
>> should I memset out to zero instead?
>
>Right now you're relying on the caller to set the NUL terminator and
>that's ugly.  Just do:
>
>                for (i = 0; i < len; i++) {
>                        c = ioread8(&buff[i]);
>                        out[i] = c;
>                        if (!c)
>                                break;
>
>
I think I've tried your suggestion and it didn't went well, I'll retry it.

>> 
>> >> +             }
>> >> +     } else
>> >> +             memcpy_fromio(out, buff, len);
>> >> +
>> >> +     iounmap(buff);
>> >> +
>> >> +     return 0;
>> >> +}
>
>[ snip ]
>
>> >> +static ssize_t version_read(struct file *filp, struct kobject *kobj,
>> >> +                         struct bin_attribute *bin_attr,
>> >> +                         char *buf, loff_t off, size_t count)
>> >> +{
>> >> +     ssize_t ret_val = vsmp_generic_buff_read(&op, 0,
>> >> +                                              vsmp_read_reg32_from_cfg(VSMP_VERSION_REG),
>> >> +                                              buf, off, count);
>> >
>> >Don't put functions which can fail in the declaration block.
>> >
>> >        ret_val = vsmp_generic_buff_read(&op, 0,
>> >                                         vsmp_read_reg32_from_cfg(VSMP_VERSION_REG),
>> >                                         buf, off, count);
>> >
>> can you explain what is the reason behind this recommendation?
>> 
>
>That's just my own preference, not really a rule.  I stole that
>guideline from someone else.  I forget who.  But it's a fact that the
>declaration block is more bug prone than other code.  (Very clean in
>static analysis results).  Plus it means that you put a blank line
>between the call and the check which I do not like (again just my
>preference).
>
>
>> >> +
>> >> +     if ((ret_val != -ENOMEM) && ((ret_val != -EINVAL)))
>> >
>> >Don't test for specific error codes.
>> >
>> >        if (ret_val < 0)
>> >                return ret_val;
>> >
>> >
>> now I understand, there was a patchset which fixes for all the -1 and the specific ret_val testing, I see it
>> was misplaced at some point of the work.
>> thanks for pointing it out
>> 
>> >> +         buf[ret_val++] = '\n';
>> >> +
>> >> +     return ret_val;
>> >> +}
>> >> +
>> >> +struct bin_attribute version_raw_attr = __BIN_ATTR(version, FILE_PREM, version_read, NULL, VERSION_MAX_LEN);
>> >> +
>> >> +/**
>> >> + * retrive str in order to determine the proper length.
>> >> + * this is the best way to maintain backwards compatibility with all
>> >> + * vSMP versions.
>> >> + */
>> >> +static int get_version_len(void)
>> >> +{
>> >> +     unsigned reg;
>> >> +     char *version_str = kzalloc(VERSION_MAX_LEN, GFP_ATOMIC | __GFP_NOWARN);
>> >
>> >VERSION_MAX_LEN is 524288 bytes.  That's too long.  Create a small
>> >buffer and loop over it until you find the NUL terminator.  Why does
>> >this need to be ATOMIC, are we holding a lock?  Hopefully if we can fix
>> >the length the __GFP_NOWARN will be unnecessary.
>> I'm not sure it is possible to loopover, I'll take that into consideration.
>> in this flow, only one access is possible, so I assume GFP_ATOMIC is not needed
>> 
>
>This get_version_len() is cray cray...  It needs to loop.
it all depends on the hypervisor's implementation below, I think loop over can be used but I'll need to check

>
>> >
>> >> +
>> >> +     if (!version_str)
>> >> +             return -1;
>> >> +
>> >> +     reg = vsmp_read_reg32_from_cfg(VSMP_VERSION_REG);
>> >> +     memset(version_str, 0, VERSION_MAX_LEN);
>> >> +
>> >> +     if (vsmp_read_buff_from_bar(0, reg, version_str, VERSION_MAX_LEN, true)) {
>> >> +             printk(KERN_ERR "failed to read buffer from bar\n");
>> >> +             return -1;
>> >> +     }
>> >> +
>> >> +     version_raw_attr.size = strlen(version_str);
>> >
>> >get_version_len() should return the length instead of setting a global.
>> do you mean version_raw_attr.size = get_version_len()?
>> 
>
>Yes, but with negative error codes.
ok

>
>> >
>> >> +     kfree(version_str);
>> >> +
>> >> +     return 0;
>> >> +}
>> >> +
>> >> +/**
>> >> + * register the version sysfs entry
>> >> + */
>> >> +int sysfs_register_version_cb(void)
>> >> +{
>> >> +     if (get_version_len()) {
>> >> +             printk(KERN_ERR "failed to init vSMP version module\n");
>> >> +             return -1;
>> >> +     }
>> >> +
>> >> +     if (!vsmp_init_op(&op, version_raw_attr.size, FW_READ)) {
>> >
>> >This if statement is reversed so this code can never work.
>> not sure I follow, can you explain?
>> 
>
>The vsmp_init_op() function returns zero on success and this code treats
>success as failure.
weird, I'll check it out.

>
>> >
>> >> +             printk(KERN_ERR "failed to init vSMP version op\n");
>> >> +             return -1;
>> >> +     }
>> >> +
>> >> +     return vsmp_register_sysfs_group(&version_raw_attr);
>> >> +}

Thanks,

Eial