[PATCH v2] drivers/virt/vSMP: new driver

[PATCH v2] drivers/virt/vSMP: new driver

[Czerwacki, Eial]

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

The driver comprises of api part which facilitates communication with
the hypervisor and version which displays the hypervisor's version.

This patch s based on previous patches sent to the staging tree mailing
lists

Signed-off-by: Eial Czerwacki <eial.czerwacki@sap.com>
Acked-by: Leonid Arsh <leonid.arsh@sap.com>
Acked-by: Oren Twaig <oren.twaig@sap.com>
CC: SAP vSMP Linux Maintainer <linux.vsmp@sap.com>
CC: Greg Kroah-Hartman <gregkh@linuxfoundation.org>
CC: Arnd Bergmann <arnd@arndb.de>
CC: Dan Carpenter <dan.carpenter@oracle.com>
CC: Andra Paraschiv <andraprs@amazon.com>
CC: Borislav Petkov <bp@suse.de>
CC: Brijesh Singh <brijesh.singh@amd.com>
CC: Eric Biggers <ebiggers@google.com>
CC: Fei Li <fei1.li@intel.com>
CC: Hans de Goede <hdegoede@redhat.com>
CC: Jens Axboe <axboe@kernel.dk>
CC: Mauro Carvalho Chehab <mchehab+huawei@kernel.org>

v1 -> v2:
	- fix -0 var init in add_sysfs_entries (pointed out by Dan Carpenter)
---
 Documentation/ABI/stable/sysfs-driver-vsmp |   5 +
 MAINTAINERS                                |   6 +
 drivers/virt/Kconfig                       |   2 +
 drivers/virt/Makefile                      |   2 +
 drivers/virt/vsmp/Kconfig                  |  11 +
 drivers/virt/vsmp/Makefile                 |   7 +
 drivers/virt/vsmp/api/api.c                | 249 +++++++++++++++++++++
 drivers/virt/vsmp/api/api.h                |  69 ++++++
 drivers/virt/vsmp/include/registers.h      |  12 +
 drivers/virt/vsmp/version/version.c        | 118 ++++++++++
 drivers/virt/vsmp/version/version.h        |  14 ++
 drivers/virt/vsmp/vsmp_main.c              | 110 +++++++++
 12 files changed, 605 insertions(+)
 create mode 100644 Documentation/ABI/stable/sysfs-driver-vsmp
 create mode 100644 drivers/virt/vsmp/Kconfig
 create mode 100644 drivers/virt/vsmp/Makefile
 create mode 100644 drivers/virt/vsmp/api/api.c
 create mode 100644 drivers/virt/vsmp/api/api.h
 create mode 100644 drivers/virt/vsmp/include/registers.h
 create mode 100644 drivers/virt/vsmp/version/version.c
 create mode 100644 drivers/virt/vsmp/version/version.h
 create mode 100644 drivers/virt/vsmp/vsmp_main.c

diff --git a/Documentation/ABI/stable/sysfs-driver-vsmp b/Documentation/ABI/stable/sysfs-driver-vsmp
new file mode 100644
index 000000000000..18a0a62f40ed
--- /dev/null
+++ b/Documentation/ABI/stable/sysfs-driver-vsmp
@@ -0,0 +1,5 @@
+What:           /sys/hypervisor/vsmp/version
+Date:           Aug 2022
+Contact:        Eial Czerwacki <eial.czerwacki@sap.com>
+		linux-vsmp@sap.com
+Description:    Shows the full version of the vSMP hypervisor
diff --git a/MAINTAINERS b/MAINTAINERS
index f512b430c7cb..cf74089c4d19 100644
--- a/MAINTAINERS
+++ b/MAINTAINERS
@@ -21783,6 +21783,12 @@ F:	lib/test_printf.c
 F:	lib/test_scanf.c
 F:	lib/vsprintf.c

+VSMP GUEST DRIVER
+M:	Eial Czerwacki <eial.czerwacki@sap.com>
+M:	linux-vsmp@sap.com
+S:	Maintained
+F:	drivers/virt/vsmp
+
 VT1211 HARDWARE MONITOR DRIVER
 M:	Juerg Haefliger <juergh@gmail.com>
 L:	linux-hwmon@vger.kernel.org
diff --git a/drivers/virt/Kconfig b/drivers/virt/Kconfig
index 87ef258cec64..9f283f476674 100644
--- a/drivers/virt/Kconfig
+++ b/drivers/virt/Kconfig
@@ -52,4 +52,6 @@ source "drivers/virt/coco/efi_secret/Kconfig"

 source "drivers/virt/coco/sev-guest/Kconfig"

+source "drivers/virt/vsmp/Kconfig"
+
 endif
diff --git a/drivers/virt/Makefile b/drivers/virt/Makefile
index 093674e05c40..159ba37cb471 100644
--- a/drivers/virt/Makefile
+++ b/drivers/virt/Makefile
@@ -11,3 +11,5 @@ obj-$(CONFIG_NITRO_ENCLAVES)	+= nitro_enclaves/
 obj-$(CONFIG_ACRN_HSM)		+= acrn/
 obj-$(CONFIG_EFI_SECRET)	+= coco/efi_secret/
 obj-$(CONFIG_SEV_GUEST)		+= coco/sev-guest/
+
+obj-$(CONFIG_VSMP)             += vsmp/
diff --git a/drivers/virt/vsmp/Kconfig b/drivers/virt/vsmp/Kconfig
new file mode 100644
index 000000000000..4e1d7e0dc746
--- /dev/null
+++ b/drivers/virt/vsmp/Kconfig
@@ -0,0 +1,11 @@
+# SPDX-License-Identifier: GPL-2.0-only
+config VSMP
+	tristate "vSMP Guest Support"
+	depends on SYS_HYPERVISOR && X86_64 && PCI
+	help
+	  Support for vSMP Guest Driver.
+
+	  This driver allows information gathering of data from the vSMP hypervisor when
+	  running on top of a vSMP-based hypervisor.
+
+	  If unsure, say no.
diff --git a/drivers/virt/vsmp/Makefile b/drivers/virt/vsmp/Makefile
new file mode 100644
index 000000000000..f637097e19f2
--- /dev/null
+++ b/drivers/virt/vsmp/Makefile
@@ -0,0 +1,7 @@
+# SPDX-License-Identifier: GPL-2.0
+#
+# Makefile for vSMP Guest drivers
+#
+
+obj-$(CONFIG_VSMP) = vsmp.o
+vsmp-y := vsmp_main.o api/api.o version/version.o
diff --git a/drivers/virt/vsmp/api/api.c b/drivers/virt/vsmp/api/api.c
new file mode 100644
index 000000000000..6e40935907bc
--- /dev/null
+++ b/drivers/virt/vsmp/api/api.c
@@ -0,0 +1,249 @@
+// SPDX-License-Identifier: GPL-2.0
+
+/*
+ * vSMP driver api
+ * (C) Copyright 2022 SAP SE
+ */
+
+#include "api.h"
+
+static void __iomem *cfg_addr;
+static struct kobject *vsmp_sysfs_kobj;
+static struct pci_dev *vsmp_dev_obj;
+
+/* R/W ops handlers */
+
+/*
+ * Init a vsmp firmware operation object
+ */
+int vsmp_init_op(struct fw_ops *op, ssize_t max_size,
+		 enum vsmp_fw_action action)
+{
+	op->hwi_block_size = max_size;
+	op->action = action;
+	op->buff_offset = op->hwi_block_size;
+
+	op->buff = kzalloc(op->hwi_block_size, GFP_KERNEL);
+	if (!op->buff)
+		return -ENOMEM;
+
+	vsmp_reset_op(op);
+
+	return 0;
+}
+
+/*
+ * Release an vsmp firmware operation object
+ */
+void vsmp_release_op(struct fw_ops *op)
+{
+	if (!op) {
+		WARN_ON(!op);
+		return;
+	}
+
+	if (!op->buff) {
+		WARN_ON(!op->buff);
+		return;
+	}
+
+	kfree(op->buff);
+	memset(op, 0, sizeof(*op));
+}
+
+/*
+ * Reset a vsmp firmware operation object
+ */
+void vsmp_reset_op(struct fw_ops *op)
+{
+	memset(op->buff, 0, op->hwi_block_size);
+	op->buff_offset = op->hwi_block_size;
+}
+
+/* Regs/Buffs R/W handlers */
+
+/*
+ * Read a value from a specific register in the vSMP's device config space
+ */
+u64 vsmp_read_reg_from_cfg(u64 reg, enum reg_size_type type)
+{
+	u64 ret_val;
+
+	switch (type) {
+	case VSMP_CTL_REG_SIZE_8BIT:
+		ret_val = readb(cfg_addr + reg);
+		break;
+
+	case VSMP_CTL_REG_SIZE_16BIT:
+		ret_val = readw(cfg_addr + reg);
+		break;
+
+	case VSMP_CTL_REG_SIZE_32BIT:
+		ret_val = readl(cfg_addr + reg);
+		break;
+
+	case VSMP_CTL_REG_SIZE_64BIT:
+		ret_val = readq(cfg_addr + reg);
+		break;
+
+	default:
+		dev_err(get_dev(), "Unsupported reg size type %d.\n", type);
+		ret_val = (u64) -EINVAL;
+	}
+
+	dev_dbg(get_dev(), "%s: read 0x%llx from reg 0x%llx of %d bits\n",
+		__func__, ret_val, reg, (type + 1) * 8);
+	return ret_val;
+}
+
+/*
+ * Read a buffer from the bar byte by byte for halt on
+ * null termination.
+ * Expected buffs are strings.
+ */
+static ssize_t read_buff_from_bar_in_bytes(char *out, u8 __iomem *buff, ssize_t len)
+{
+	u32 i;
+
+	for (i = 0; i < len; i++) {
+		out[i] = ioread8(&buff[i]);
+		if (!out[i])
+			break;
+	}
+
+	return i;
+}
+
+/*
+ * Read a buffer from a specific offset in a specific bar,
+ * maxed to a predefined len size-wise from the vSMP device
+ */
+int vsmp_read_buff_from_bar(u8 bar, u32 offset, char *out, ssize_t len,
+			    bool halt_on_null)
+{
+	u8 __iomem *buff;
+	u64 bar_start = pci_resource_start(vsmp_dev_obj, bar);
+	u32 bar_len = pci_resource_len(vsmp_dev_obj, bar);
+	ssize_t actual_len = len;
+
+	/* incase of overflow, warn and use max len possible */
+	if ((offset + len) > bar_len) {
+		WARN_ON((offset + len) > actual_len);
+		actual_len = bar_len - offset;
+		dev_dbg(get_dev(), "%lu overflows bar len, using %ld len instead\n",
+			len, actual_len);
+	}
+
+	buff = ioremap(bar_start + offset, actual_len);
+	if (!buff)
+		return -ENOMEM;
+
+	if (halt_on_null)
+		read_buff_from_bar_in_bytes(out, buff, len);
+	else
+		memcpy_fromio(out, buff, len);
+
+	iounmap(buff);
+
+	return 0;
+}
+
+/*
+ * Generic function to read from the bar
+ */
+ssize_t vsmp_generic_buff_read(struct fw_ops *op, u8 bar, u64 reg,
+			       char *buf, loff_t off, ssize_t count)
+{
+	ssize_t ret_val = 0;
+
+	if (op->buff_offset >= op->hwi_block_size) {	/* perform H/W op */
+		vsmp_reset_op(op);
+
+		ret_val = vsmp_read_buff_from_bar(bar, reg, op->buff, op->hwi_block_size, false);
+		if (ret_val) {
+			dev_err(get_dev(), "%s operation failed\n",
+				(op->action == FW_READ) ? "read" : "write");
+		}
+		op->buff_offset = 0;
+	}
+
+	if (ret_val)
+		return ret_val;
+
+	return memory_read_from_buffer(buf, count, &op->buff_offset, op->buff, op->hwi_block_size);
+}
+
+/* sysfs handlers */
+
+/*
+ * Register the vSMP sysfs object for user space interaction
+ */
+int vsmp_register_sysfs_group(const struct bin_attribute *bin_attr)
+{
+	int error = -EINVAL;
+
+	if (vsmp_sysfs_kobj && bin_attr) {
+		error = sysfs_create_bin_file(vsmp_sysfs_kobj, bin_attr);
+		if (error)
+			dev_err(get_dev(), "Failed to register sysfs entry (%d)\n", error);
+	}
+
+	return error;
+}
+
+/*
+ * Deregister the vSMP sysfs object for user space interaction
+ */
+void vsmp_deregister_sysfs_group(const struct bin_attribute *bin_attr)
+{
+	if (vsmp_sysfs_kobj && bin_attr)
+		sysfs_remove_bin_file(vsmp_sysfs_kobj, bin_attr);
+}
+
+/* Generic functions */
+
+/*
+ * Open the cfg address space of the vSDP device
+ */
+int open_cfg_addr(struct pci_dev *pdev)
+{
+	u64 cfg_start;
+	u32 cfg_len;
+
+	vsmp_dev_obj = pdev;
+	cfg_start = pci_resource_start(vsmp_dev_obj, 0);
+	cfg_len = pci_resource_len(vsmp_dev_obj, 0);
+
+	dev_dbg(get_dev(), "Mapping bar 0: [0x%llx,0x%llx]\n",
+		cfg_start, cfg_start + cfg_len);
+
+	cfg_addr = ioremap(cfg_start, cfg_len);
+	if (!cfg_addr) {
+		dev_err(get_dev(), "Failed to map vSMP pci controller, exiting.\n");
+		return -ENODEV;
+	}
+
+	return 0;
+}
+
+int init_sysfs(void)
+{
+	vsmp_sysfs_kobj = kobject_create_and_add("vsmp", hypervisor_kobj);
+	if (!vsmp_sysfs_kobj) {
+		dev_err(get_dev(), "Failed to create vSMP sysfs entry, exiting.\n");
+		return -EINVAL;
+	}
+
+	return 0;
+}
+
+void cleanup(void)
+{
+	iounmap(cfg_addr);
+	kobject_put(vsmp_sysfs_kobj);
+}
+
+const struct device *get_dev(void)
+{
+	return &vsmp_dev_obj->dev;
+}
diff --git a/drivers/virt/vsmp/api/api.h b/drivers/virt/vsmp/api/api.h
new file mode 100644
index 000000000000..6142e947979f
--- /dev/null
+++ b/drivers/virt/vsmp/api/api.h
@@ -0,0 +1,69 @@
+/* SPDX-License-Identifier: GPL-2.0 */
+
+/*
+ * vSMP driver api header
+ * (C) Copyright 2022 SAP SE
+ */
+
+#ifndef VSMP_API_H
+#define VSMP_API_H
+
+#include <linux/pci.h>
+
+// R/W ops handlers
+#define vsmp_read_reg32_from_cfg(_reg_) \
+	((u32) vsmp_read_reg_from_cfg((_reg_), VSMP_CTL_REG_SIZE_32BIT))
+
+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;
+};
+
+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);
+void vsmp_reset_op(struct fw_ops *op);
+
+#define FILE_PREM 0444
+
+/* Regs/Buffs R/W handlers */
+#define vsmp_read_reg32_from_cfg(_reg_) \
+	((u32) vsmp_read_reg_from_cfg((_reg_), VSMP_CTL_REG_SIZE_32BIT))
+
+u64 vsmp_read_reg_from_cfg(u64 reg, enum reg_size_type type);
+ssize_t vsmp_generic_buff_read(struct fw_ops *op, u8 bar, u64 reg,
+			       char *buf, loff_t off, ssize_t count);
+int vsmp_read_buff_from_bar(u8 bar, u32 offset, char *out, ssize_t len,
+			    bool halt_on_null);
+
+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 vsmp_register_sysfs_group(const struct bin_attribute *bin_attr);
+void vsmp_deregister_sysfs_group(const struct bin_attribute *bin_attr);
+
+int open_cfg_addr(struct pci_dev *pdev);
+int init_sysfs(void);
+void cleanup(void);
+const struct device *get_dev(void);
+#endif /* VSMP_API_H */
diff --git a/drivers/virt/vsmp/include/registers.h b/drivers/virt/vsmp/include/registers.h
new file mode 100644
index 000000000000..b6458d25e3b7
--- /dev/null
+++ b/drivers/virt/vsmp/include/registers.h
@@ -0,0 +1,12 @@
+/* SPDX-License-Identifier: GPL-2.0 */
+/*
+ * vSMP driver registers
+ * (C) Copyright 2022 SAP SE
+ */
+
+#ifndef VSMP_REGSITERS_H
+#define VSMP_REGSITERS_H
+
+#define VSMP_VERSION_REG 0x0c
+
+#endif /* VSMP_REGSITERS_H */
diff --git a/drivers/virt/vsmp/version/version.c b/drivers/virt/vsmp/version/version.c
new file mode 100644
index 000000000000..d8ad771daf28
--- /dev/null
+++ b/drivers/virt/vsmp/version/version.c
@@ -0,0 +1,118 @@
+// SPDX-License-Identifier: GPL-2.0
+
+/*
+ * vSMP driver version module
+ * (C) Copyright 2022 SAP SE
+ */
+
+#include <linux/slab.h>
+#include <linux/kobject.h>
+
+#include "../api/api.h"
+#include "../include/registers.h"
+
+/*
+ * This is the maximal possible length of the version which is a text string
+ * the real len is usually much smaller, thus the driver uses this once to read
+ * the version string and record it's actual len.
+ * From that point and on, the actual len will be used in each call.
+ */
+#define VERSION_MAX_LEN (1 << 19)
+
+static struct fw_ops op;
+
+static ssize_t version_read(struct file *filp, struct kobject *kobj,
+			    struct bin_attribute *bin_attr,
+			    char *buf, loff_t off, size_t count)
+{
+	u64 reg_val = vsmp_read_reg32_from_cfg(VSMP_VERSION_REG);
+	ssize_t ret_val;
+
+	if (reg_val < 0) {
+		dev_err(get_dev(), "Failed to value of reg 0x%x\n", VSMP_VERSION_REG);
+		return 0;
+	}
+
+	ret_val = vsmp_generic_buff_read(&op, 0, reg_val, buf, off, count);
+	if (ret_val < 0) {
+		dev_err(get_dev(), "Failed to read version (%ld)\n", ret_val);
+		return 0;
+	}
+
+	buf[ret_val++] = '\n';
+
+	return ret_val;
+}
+
+struct bin_attribute version_raw_attr = __BIN_ATTR(version, FILE_PREM,
+						   version_read, NULL, VERSION_MAX_LEN);
+
+/*
+ * Retrieve str in order to determine the proper length.
+ * This is the best way to maintain backwards compatibility with all
+ * vSMP versions.
+ */
+static ssize_t get_version_len(void)
+{
+	ssize_t len = 0;
+	u64 reg_val = vsmp_read_reg32_from_cfg(VSMP_VERSION_REG);
+	char *version_str = kzalloc(VERSION_MAX_LEN, GFP_KERNEL);
+
+	if (!version_str)
+		return len;
+
+	if (vsmp_read_reg32_from_cfg(VSMP_VERSION_REG) < 0) {
+		kfree(version_str);
+		dev_err(get_dev(), "Failed to read value of reg 0x%x\n", VSMP_VERSION_REG);
+		return len;
+	}
+
+	memset(version_str, 0, VERSION_MAX_LEN);
+	if (vsmp_read_buff_from_bar(0, reg_val, version_str, VERSION_MAX_LEN, true)) {
+		kfree(version_str);
+		dev_err(get_dev(), "Failed to read buffer from bar\n");
+		return len;
+	}
+
+	len = strlen(version_str);
+	kfree(version_str);
+
+	return len;
+}
+
+/*
+ * Register the version sysfs entry
+ */
+int sysfs_register_version_cb(void)
+{
+	ssize_t len = get_version_len();
+	int ret_val;
+
+	if (!len) {
+		dev_err(get_dev(), "Failed to init vSMP version module\n");
+		return -EINVAL;
+	}
+	version_raw_attr.size = len;
+
+	if (vsmp_init_op(&op, version_raw_attr.size, FW_READ)) {
+		dev_err(get_dev(), "Failed to init vSMP version op\n");
+		return -ENODEV;
+	}
+
+	ret_val = vsmp_register_sysfs_group(&version_raw_attr);
+	if (ret_val) {
+		dev_err(get_dev(), "Failed to init vSMP version support\n");
+		vsmp_release_op(&op);
+	}
+
+	return ret_val;
+}
+
+/*
+ * Deregister the version sysfs entry
+ */
+void sysfs_deregister_version_cb(void)
+{
+	vsmp_deregister_sysfs_group(&version_raw_attr);
+	vsmp_release_op(&op);
+}
diff --git a/drivers/virt/vsmp/version/version.h b/drivers/virt/vsmp/version/version.h
new file mode 100644
index 000000000000..c4430b3065e4
--- /dev/null
+++ b/drivers/virt/vsmp/version/version.h
@@ -0,0 +1,14 @@
+/* SPDX-License-Identifier: GPL-2.0 */
+
+/*
+ * vSMP driver version module header
+ * (C) Copyright 2022 SAP SE
+ */
+
+#ifndef VSMP_VERSION_COMMON_H
+#define VSMP_VERSION_COMMON_H
+
+int sysfs_register_version_cb(void);
+void sysfs_deregister_version_cb(void);
+
+#endif /* VSMP_VERSION_COMMON_H */
diff --git a/drivers/virt/vsmp/vsmp_main.c b/drivers/virt/vsmp/vsmp_main.c
new file mode 100644
index 000000000000..95704bc7a32f
--- /dev/null
+++ b/drivers/virt/vsmp/vsmp_main.c
@@ -0,0 +1,110 @@
+// SPDX-License-Identifier: GPL-2.0
+
+/*
+ * vSMP driver main
+ * (C) Copyright 2022 SAP SE
+ */
+
+#include <linux/module.h>
+
+#include "api/api.h"
+#include "version/version.h"
+
+/* modules info */
+#define DEVICE_NAME "vSMP"
+#define DRIVER_LICENSE "GPL v2"
+#define DRIVER_AUTHOR "Eial Czerwacki <eial.czerwacki@sap.com>"
+#define DRIVER_DESC "vSMP hypervisor driver"
+#define DRIVER_VERSION "0.1"
+
+#define PCI_DEVICE_ID_SAP_FLX_VSMP_CTL 0x1011
+
+MODULE_LICENSE(DRIVER_LICENSE);
+MODULE_AUTHOR(DRIVER_AUTHOR);
+MODULE_DESCRIPTION(DRIVER_DESC);
+MODULE_VERSION(DRIVER_VERSION);
+
+/* Sysfs handlers */
+#define create_entry(_label_) \
+	{ \
+		.reg_cb = sysfs_register_ ## _label_ ## _cb, \
+		.dereg_cb = sysfs_deregister_ ## _label_ ## _cb, \
+	}
+
+static struct sysfs_entry_cbs cbs_arr[] = {
+	create_entry(version),
+};
+
+static const struct pci_device_id vsmp_pci_table[] = {
+	{ PCI_VDEVICE(SCALEMP, PCI_DEVICE_ID_SAP_FLX_VSMP_CTL), 0, },
+	{ 0, },			/* terminate list */
+};
+
+/*
+ * Init all submodules's sysfs entries
+ */
+static int add_sysfs_entries(void)
+{
+	int ret_val = 0, i;
+
+	for (i = 0; (i < ARRAY_SIZE(cbs_arr) && !ret_val); i++) {
+		ret_val = cbs_arr[i].reg_cb();
+		if (ret_val) {
+			dev_err(get_dev(), "Failed to init sysfs entry %d (%d).\n",
+				i, ret_val);
+		}
+	}
+
+	return ret_val;
+}
+
+/*
+ * Remove all submodules's sysfs entries
+ */
+static void remove_sysfs_entries(void)
+{
+	int i;
+
+	for (i = 0; i < ARRAY_SIZE(cbs_arr); i++)
+		cbs_arr[i].dereg_cb();
+}
+
+static int vsmp_pci_probe(struct pci_dev *pci, const struct pci_device_id *id)
+{
+	int ret_val;
+
+	ret_val = open_cfg_addr(pci);
+	if (ret_val) {
+		dev_err(get_dev(), "Failed to open cfg addr\n");
+		return ret_val;
+	}
+
+	if (init_sysfs()) {
+		dev_err(get_dev(), "Failed to create sysfs folder\n");
+		return -ENODEV;
+	}
+
+	if (add_sysfs_entries()) {
+		dev_err(get_dev(), "Failed to create sysfs entries\n");
+		return -ENODEV;
+	}
+
+	dev_info(get_dev(), "%s up and running\n", DRIVER_DESC);
+
+	return 0;
+}
+
+static void vsmp_pci_remove(struct pci_dev *pci)
+{
+	remove_sysfs_entries();
+	cleanup();
+}
+
+static struct pci_driver vsmp_pci_driver = {
+	.name		= DEVICE_NAME,
+	.id_table	= vsmp_pci_table,
+	.probe		= vsmp_pci_probe,
+	.remove	= vsmp_pci_remove,
+};
+
+module_pci_driver(vsmp_pci_driver);
--
2.25.1

Re: [PATCH v2] drivers/virt/vSMP: new driver

[Greg Kroah-Hartman]

On Thu, Aug 25, 2022 at 06:24:02AM +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 api part which facilitates communication with
> the hypervisor and version which displays the hypervisor's version.
> 
> This patch s based on previous patches sent to the staging tree mailing
> lists
> 
> Signed-off-by: Eial Czerwacki <eial.czerwacki@sap.com>
> Acked-by: Leonid Arsh <leonid.arsh@sap.com>
> Acked-by: Oren Twaig <oren.twaig@sap.com>
> CC: SAP vSMP Linux Maintainer <linux.vsmp@sap.com>
> CC: Greg Kroah-Hartman <gregkh@linuxfoundation.org>
> CC: Arnd Bergmann <arnd@arndb.de>
> CC: Dan Carpenter <dan.carpenter@oracle.com>
> CC: Andra Paraschiv <andraprs@amazon.com>
> CC: Borislav Petkov <bp@suse.de>
> CC: Brijesh Singh <brijesh.singh@amd.com>
> CC: Eric Biggers <ebiggers@google.com>
> CC: Fei Li <fei1.li@intel.com>
> CC: Hans de Goede <hdegoede@redhat.com>
> CC: Jens Axboe <axboe@kernel.dk>
> CC: Mauro Carvalho Chehab <mchehab+huawei@kernel.org>
> 
> v1 -> v2:
> 	- fix -0 var init in add_sysfs_entries (pointed out by Dan Carpenter)
> ---
>  Documentation/ABI/stable/sysfs-driver-vsmp |   5 +
>  MAINTAINERS                                |   6 +
>  drivers/virt/Kconfig                       |   2 +
>  drivers/virt/Makefile                      |   2 +
>  drivers/virt/vsmp/Kconfig                  |  11 +
>  drivers/virt/vsmp/Makefile                 |   7 +
>  drivers/virt/vsmp/api/api.c                | 249 +++++++++++++++++++++
>  drivers/virt/vsmp/api/api.h                |  69 ++++++
>  drivers/virt/vsmp/include/registers.h      |  12 +
>  drivers/virt/vsmp/version/version.c        | 118 ++++++++++
>  drivers/virt/vsmp/version/version.h        |  14 ++
>  drivers/virt/vsmp/vsmp_main.c              | 110 +++++++++
>  12 files changed, 605 insertions(+)

Why do you have all of these different .c and .h files for only 600
lines of code?  Shouldn't this all just be in a single .c file?  Why
have a subdir for just 300 lines?

Please mush this all into a single .c file going forward, speading it
out like this makes no sense.

>  create mode 100644 Documentation/ABI/stable/sysfs-driver-vsmp
>  create mode 100644 drivers/virt/vsmp/Kconfig
>  create mode 100644 drivers/virt/vsmp/Makefile
>  create mode 100644 drivers/virt/vsmp/api/api.c
>  create mode 100644 drivers/virt/vsmp/api/api.h
>  create mode 100644 drivers/virt/vsmp/include/registers.h
>  create mode 100644 drivers/virt/vsmp/version/version.c
>  create mode 100644 drivers/virt/vsmp/version/version.h
>  create mode 100644 drivers/virt/vsmp/vsmp_main.c
> 
> diff --git a/Documentation/ABI/stable/sysfs-driver-vsmp b/Documentation/ABI/stable/sysfs-driver-vsmp
> new file mode 100644
> index 000000000000..18a0a62f40ed
> --- /dev/null
> +++ b/Documentation/ABI/stable/sysfs-driver-vsmp
> @@ -0,0 +1,5 @@
> +What:           /sys/hypervisor/vsmp/version
> +Date:           Aug 2022

August is almost over :(

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

No need for an alias here.

> +Description:    Shows the full version of the vSMP hypervisor

That's not very descriptive, what is this supposed to look like?


> diff --git a/MAINTAINERS b/MAINTAINERS
> index f512b430c7cb..cf74089c4d19 100644
> --- a/MAINTAINERS
> +++ b/MAINTAINERS
> @@ -21783,6 +21783,12 @@ F:	lib/test_printf.c
>  F:	lib/test_scanf.c
>  F:	lib/vsprintf.c
> 
> +VSMP GUEST DRIVER
> +M:	Eial Czerwacki <eial.czerwacki@sap.com>
> +M:	linux-vsmp@sap.com

Again, no random aliases please, stick to a person as a contact.

> +S:	Maintained
> +F:	drivers/virt/vsmp
> +
>  VT1211 HARDWARE MONITOR DRIVER
>  M:	Juerg Haefliger <juergh@gmail.com>
>  L:	linux-hwmon@vger.kernel.org
> diff --git a/drivers/virt/Kconfig b/drivers/virt/Kconfig
> index 87ef258cec64..9f283f476674 100644
> --- a/drivers/virt/Kconfig
> +++ b/drivers/virt/Kconfig
> @@ -52,4 +52,6 @@ source "drivers/virt/coco/efi_secret/Kconfig"
> 
>  source "drivers/virt/coco/sev-guest/Kconfig"
> 
> +source "drivers/virt/vsmp/Kconfig"
> +
>  endif
> diff --git a/drivers/virt/Makefile b/drivers/virt/Makefile
> index 093674e05c40..159ba37cb471 100644
> --- a/drivers/virt/Makefile
> +++ b/drivers/virt/Makefile
> @@ -11,3 +11,5 @@ obj-$(CONFIG_NITRO_ENCLAVES)	+= nitro_enclaves/
>  obj-$(CONFIG_ACRN_HSM)		+= acrn/
>  obj-$(CONFIG_EFI_SECRET)	+= coco/efi_secret/
>  obj-$(CONFIG_SEV_GUEST)		+= coco/sev-guest/
> +
> +obj-$(CONFIG_VSMP)             += vsmp/

Why a blank line?

> diff --git a/drivers/virt/vsmp/Kconfig b/drivers/virt/vsmp/Kconfig
> new file mode 100644
> index 000000000000..4e1d7e0dc746
> --- /dev/null
> +++ b/drivers/virt/vsmp/Kconfig
> @@ -0,0 +1,11 @@
> +# SPDX-License-Identifier: GPL-2.0-only
> +config VSMP
> +	tristate "vSMP Guest Support"
> +	depends on SYS_HYPERVISOR && X86_64 && PCI
> +	help
> +	  Support for vSMP Guest Driver.
> +
> +	  This driver allows information gathering of data from the vSMP hypervisor when
> +	  running on top of a vSMP-based hypervisor.
> +
> +	  If unsure, say no.

No module name information?


> diff --git a/drivers/virt/vsmp/Makefile b/drivers/virt/vsmp/Makefile
> new file mode 100644
> index 000000000000..f637097e19f2
> --- /dev/null
> +++ b/drivers/virt/vsmp/Makefile
> @@ -0,0 +1,7 @@
> +# SPDX-License-Identifier: GPL-2.0
> +#
> +# Makefile for vSMP Guest drivers
> +#
> +
> +obj-$(CONFIG_VSMP) = vsmp.o
> +vsmp-y := vsmp_main.o api/api.o version/version.o
> diff --git a/drivers/virt/vsmp/api/api.c b/drivers/virt/vsmp/api/api.c
> new file mode 100644
> index 000000000000..6e40935907bc
> --- /dev/null
> +++ b/drivers/virt/vsmp/api/api.c
> @@ -0,0 +1,249 @@
> +// SPDX-License-Identifier: GPL-2.0
> +
> +/*
> + * vSMP driver api

"driver api?"  What is that?

> + * (C) Copyright 2022 SAP SE
> + */
> +
> +#include "api.h"
> +
> +static void __iomem *cfg_addr;
> +static struct kobject *vsmp_sysfs_kobj;
> +static struct pci_dev *vsmp_dev_obj;

Do not make it so that you can only have one device in the system like
this.  Make it dynamic and properly tie into the driver model and then
you will have no such restrictions at all.

> +
> +/* R/W ops handlers */

I do not understand this comment at all, which is not a good sign...

> +
> +/*
> + * Init a vsmp firmware operation object
> + */
> +int vsmp_init_op(struct fw_ops *op, ssize_t max_size,
> +		 enum vsmp_fw_action action)
> +{
> +	op->hwi_block_size = max_size;
> +	op->action = action;
> +	op->buff_offset = op->hwi_block_size;
> +
> +	op->buff = kzalloc(op->hwi_block_size, GFP_KERNEL);
> +	if (!op->buff)
> +		return -ENOMEM;
> +
> +	vsmp_reset_op(op);
> +
> +	return 0;
> +}
> +
> +/*
> + * Release an vsmp firmware operation object
> + */
> +void vsmp_release_op(struct fw_ops *op)

Why are all of these global symbols?

If you put it all into one .c file, it's much easier.

> +{
> +	if (!op) {
> +		WARN_ON(!op);
> +		return;
> +	}
> +
> +	if (!op->buff) {
> +		WARN_ON(!op->buff);
> +		return;
> +	}
> +
> +	kfree(op->buff);
> +	memset(op, 0, sizeof(*op));
> +}
> +
> +/*
> + * Reset a vsmp firmware operation object
> + */
> +void vsmp_reset_op(struct fw_ops *op)
> +{
> +	memset(op->buff, 0, op->hwi_block_size);
> +	op->buff_offset = op->hwi_block_size;
> +}
> +
> +/* Regs/Buffs R/W handlers */
> +
> +/*
> + * Read a value from a specific register in the vSMP's device config space
> + */
> +u64 vsmp_read_reg_from_cfg(u64 reg, enum reg_size_type type)
> +{
> +	u64 ret_val;
> +
> +	switch (type) {
> +	case VSMP_CTL_REG_SIZE_8BIT:
> +		ret_val = readb(cfg_addr + reg);
> +		break;
> +
> +	case VSMP_CTL_REG_SIZE_16BIT:
> +		ret_val = readw(cfg_addr + reg);
> +		break;
> +
> +	case VSMP_CTL_REG_SIZE_32BIT:
> +		ret_val = readl(cfg_addr + reg);
> +		break;
> +
> +	case VSMP_CTL_REG_SIZE_64BIT:
> +		ret_val = readq(cfg_addr + reg);
> +		break;
> +
> +	default:
> +		dev_err(get_dev(), "Unsupported reg size type %d.\n", type);
> +		ret_val = (u64) -EINVAL;
> +	}
> +
> +	dev_dbg(get_dev(), "%s: read 0x%llx from reg 0x%llx of %d bits\n",
> +		__func__, ret_val, reg, (type + 1) * 8);
> +	return ret_val;
> +}
> +
> +/*
> + * Read a buffer from the bar byte by byte for halt on
> + * null termination.
> + * Expected buffs are strings.
> + */
> +static ssize_t read_buff_from_bar_in_bytes(char *out, u8 __iomem *buff, ssize_t len)
> +{
> +	u32 i;
> +
> +	for (i = 0; i < len; i++) {
> +		out[i] = ioread8(&buff[i]);
> +		if (!out[i])
> +			break;

So even if there is a failure here (0?), you return success?

> +	}
> +
> +	return i;
> +}
> +
> +/*
> + * Read a buffer from a specific offset in a specific bar,
> + * maxed to a predefined len size-wise from the vSMP device
> + */
> +int vsmp_read_buff_from_bar(u8 bar, u32 offset, char *out, ssize_t len,
> +			    bool halt_on_null)
> +{
> +	u8 __iomem *buff;
> +	u64 bar_start = pci_resource_start(vsmp_dev_obj, bar);
> +	u32 bar_len = pci_resource_len(vsmp_dev_obj, bar);
> +	ssize_t actual_len = len;
> +
> +	/* incase of overflow, warn and use max len possible */
> +	if ((offset + len) > bar_len) {
> +		WARN_ON((offset + len) > actual_len);

Please no new WARN_ON, just handl the error properly and recover from
it.

> +		actual_len = bar_len - offset;
> +		dev_dbg(get_dev(), "%lu overflows bar len, using %ld len instead\n",

get_dev() is not a good function name for the global symbol space at
all.  Just pass in your device (hint, you have it here already), and use
that.

> +			len, actual_len);
> +	}
> +
> +	buff = ioremap(bar_start + offset, actual_len);
> +	if (!buff)
> +		return -ENOMEM;
> +
> +	if (halt_on_null)
> +		read_buff_from_bar_in_bytes(out, buff, len);
> +	else
> +		memcpy_fromio(out, buff, len);
> +
> +	iounmap(buff);
> +
> +	return 0;
> +}
> +
> +/*
> + * Generic function to read from the bar
> + */
> +ssize_t vsmp_generic_buff_read(struct fw_ops *op, u8 bar, u64 reg,
> +			       char *buf, loff_t off, ssize_t count)
> +{
> +	ssize_t ret_val = 0;
> +
> +	if (op->buff_offset >= op->hwi_block_size) {	/* perform H/W op */
> +		vsmp_reset_op(op);
> +
> +		ret_val = vsmp_read_buff_from_bar(bar, reg, op->buff, op->hwi_block_size, false);
> +		if (ret_val) {
> +			dev_err(get_dev(), "%s operation failed\n",
> +				(op->action == FW_READ) ? "read" : "write");
> +		}
> +		op->buff_offset = 0;
> +	}
> +
> +	if (ret_val)
> +		return ret_val;
> +
> +	return memory_read_from_buffer(buf, count, &op->buff_offset, op->buff, op->hwi_block_size);
> +}
> +
> +/* sysfs handlers */
> +
> +/*
> + * Register the vSMP sysfs object for user space interaction
> + */
> +int vsmp_register_sysfs_group(const struct bin_attribute *bin_attr)
> +{
> +	int error = -EINVAL;
> +
> +	if (vsmp_sysfs_kobj && bin_attr) {
> +		error = sysfs_create_bin_file(vsmp_sysfs_kobj, bin_attr);

You raced userspace and lost :(

And why is your version file a binary file?  It should just be a small
text string, right?

> +		if (error)
> +			dev_err(get_dev(), "Failed to register sysfs entry (%d)\n", error);
> +	}
> +
> +	return error;
> +}
> +
> +/*
> + * Deregister the vSMP sysfs object for user space interaction
> + */
> +void vsmp_deregister_sysfs_group(const struct bin_attribute *bin_attr)
> +{
> +	if (vsmp_sysfs_kobj && bin_attr)
> +		sysfs_remove_bin_file(vsmp_sysfs_kobj, bin_attr);
> +}

Why all the indirection here?  Once you clean this up to be in one file,
it will be much smaller as you will not need this middle layer at all.

> +
> +/* Generic functions */
> +
> +/*
> + * Open the cfg address space of the vSDP device
> + */
> +int open_cfg_addr(struct pci_dev *pdev)
> +{
> +	u64 cfg_start;
> +	u32 cfg_len;
> +
> +	vsmp_dev_obj = pdev;
> +	cfg_start = pci_resource_start(vsmp_dev_obj, 0);
> +	cfg_len = pci_resource_len(vsmp_dev_obj, 0);
> +
> +	dev_dbg(get_dev(), "Mapping bar 0: [0x%llx,0x%llx]\n",
> +		cfg_start, cfg_start + cfg_len);

Again, you have a device, use that.  Goes for the whole driver.

> +#define FILE_PREM 0444

File permission of what?  And shouldn't it be "PERM", not "PREM"?  And
why do you need it at all?  Just use the proper sysfs macros and you
never need it.  See below.

> +
> +/* Regs/Buffs R/W handlers */
> +#define vsmp_read_reg32_from_cfg(_reg_) \
> +	((u32) vsmp_read_reg_from_cfg((_reg_), VSMP_CTL_REG_SIZE_32BIT))
> +
> +u64 vsmp_read_reg_from_cfg(u64 reg, enum reg_size_type type);
> +ssize_t vsmp_generic_buff_read(struct fw_ops *op, u8 bar, u64 reg,
> +			       char *buf, loff_t off, ssize_t count);
> +int vsmp_read_buff_from_bar(u8 bar, u32 offset, char *out, ssize_t len,
> +			    bool halt_on_null);
> +
> +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 vsmp_register_sysfs_group(const struct bin_attribute *bin_attr);
> +void vsmp_deregister_sysfs_group(const struct bin_attribute *bin_attr);
> +
> +int open_cfg_addr(struct pci_dev *pdev);
> +int init_sysfs(void);
> +void cleanup(void);
> +const struct device *get_dev(void);
> +#endif /* VSMP_API_H */
> diff --git a/drivers/virt/vsmp/include/registers.h b/drivers/virt/vsmp/include/registers.h
> new file mode 100644
> index 000000000000..b6458d25e3b7
> --- /dev/null
> +++ b/drivers/virt/vsmp/include/registers.h
> @@ -0,0 +1,12 @@
> +/* SPDX-License-Identifier: GPL-2.0 */
> +/*
> + * vSMP driver registers
> + * (C) Copyright 2022 SAP SE
> + */
> +
> +#ifndef VSMP_REGSITERS_H
> +#define VSMP_REGSITERS_H
> +
> +#define VSMP_VERSION_REG 0x0c
> +
> +#endif /* VSMP_REGSITERS_H */

Smallest .h file ever.   12 lines for a single #define, please don't do
that.


> diff --git a/drivers/virt/vsmp/version/version.c b/drivers/virt/vsmp/version/version.c
> new file mode 100644
> index 000000000000..d8ad771daf28
> --- /dev/null
> +++ b/drivers/virt/vsmp/version/version.c
> @@ -0,0 +1,118 @@
> +// SPDX-License-Identifier: GPL-2.0
> +
> +/*
> + * vSMP driver version module
> + * (C) Copyright 2022 SAP SE
> + */
> +
> +#include <linux/slab.h>
> +#include <linux/kobject.h>
> +
> +#include "../api/api.h"
> +#include "../include/registers.h"
> +
> +/*
> + * This is the maximal possible length of the version which is a text string
> + * the real len is usually much smaller, thus the driver uses this once to read
> + * the version string and record it's actual len.
> + * From that point and on, the actual len will be used in each call.
> + */
> +#define VERSION_MAX_LEN (1 << 19)
> +
> +static struct fw_ops op;
> +
> +static ssize_t version_read(struct file *filp, struct kobject *kobj,
> +			    struct bin_attribute *bin_attr,
> +			    char *buf, loff_t off, size_t count)
> +{
> +	u64 reg_val = vsmp_read_reg32_from_cfg(VSMP_VERSION_REG);
> +	ssize_t ret_val;
> +
> +	if (reg_val < 0) {
> +		dev_err(get_dev(), "Failed to value of reg 0x%x\n", VSMP_VERSION_REG);
> +		return 0;
> +	}
> +
> +	ret_val = vsmp_generic_buff_read(&op, 0, reg_val, buf, off, count);
> +	if (ret_val < 0) {
> +		dev_err(get_dev(), "Failed to read version (%ld)\n", ret_val);
> +		return 0;
> +	}
> +
> +	buf[ret_val++] = '\n';
> +
> +	return ret_val;
> +}
> +
> +struct bin_attribute version_raw_attr = __BIN_ATTR(version, FILE_PREM,
> +						   version_read, NULL, VERSION_MAX_LEN);

__BIN_ATTR_RO()?

But again, why is this a binary file?

I stopped reviewing here.  Please clean up all of the above first, and
then the rest of the file will be smaller and easier to review.

thanks,

greg k-h

Re: [PATCH v2] drivers/virt/vSMP: new driver

[Czerwacki, Eial]

Greetings Greg,

thank you for the comments, see my comment below.
>On Thu, Aug 25, 2022 at 06:24:02AM +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 api part which facilitates communication with
>> the hypervisor and version which displays the hypervisor's version.
>> 
>> This patch s based on previous patches sent to the staging tree mailing
>> lists
>> 
>> Signed-off-by: Eial Czerwacki <eial.czerwacki@sap.com>
>> Acked-by: Leonid Arsh <leonid.arsh@sap.com>
>> Acked-by: Oren Twaig <oren.twaig@sap.com>
>> CC: SAP vSMP Linux Maintainer <linux.vsmp@sap.com>
>> CC: Greg Kroah-Hartman <gregkh@linuxfoundation.org>
>> CC: Arnd Bergmann <arnd@arndb.de>
>> CC: Dan Carpenter <dan.carpenter@oracle.com>
>> CC: Andra Paraschiv <andraprs@amazon.com>
>> CC: Borislav Petkov <bp@suse.de>
>> CC: Brijesh Singh <brijesh.singh@amd.com>
>> CC: Eric Biggers <ebiggers@google.com>
>> CC: Fei Li <fei1.li@intel.com>
>> CC: Hans de Goede <hdegoede@redhat.com>
>> CC: Jens Axboe <axboe@kernel.dk>
>> CC: Mauro Carvalho Chehab <mchehab+huawei@kernel.org>
>> 
>> v1 -> v2:
>>        - fix -0 var init in add_sysfs_entries (pointed out by Dan Carpenter)
>> ---
>>  Documentation/ABI/stable/sysfs-driver-vsmp |   5 +
>>  MAINTAINERS                                |   6 +
>>  drivers/virt/Kconfig                       |   2 +
>>  drivers/virt/Makefile                      |   2 +
>>  drivers/virt/vsmp/Kconfig                  |  11 +
>>  drivers/virt/vsmp/Makefile                 |   7 +
>>  drivers/virt/vsmp/api/api.c                | 249 +++++++++++++++++++++
>>  drivers/virt/vsmp/api/api.h                |  69 ++++++
>>  drivers/virt/vsmp/include/registers.h      |  12 +
>>  drivers/virt/vsmp/version/version.c        | 118 ++++++++++
>>  drivers/virt/vsmp/version/version.h        |  14 ++
>>  drivers/virt/vsmp/vsmp_main.c              | 110 +++++++++
>>  12 files changed, 605 insertions(+)
>
>Why do you have all of these different .c and .h files for only 600
>lines of code?  Shouldn't this all just be in a single .c file?  Why
>have a subdir for just 300 lines?
>
>Please mush this all into a single .c file going forward, speading it
>out like this makes no sense.
the current driver has two modules, version and api, in later versions of the
driver support for additional features will be added.
at that time, this might cause the single file to inflate, so we thought to split it
to several files to make it more organized.
is there any way to keep it in different code files?

>
>>  create mode 100644 Documentation/ABI/stable/sysfs-driver-vsmp
>>  create mode 100644 drivers/virt/vsmp/Kconfig
>>  create mode 100644 drivers/virt/vsmp/Makefile
>>  create mode 100644 drivers/virt/vsmp/api/api.c
>>  create mode 100644 drivers/virt/vsmp/api/api.h
>>  create mode 100644 drivers/virt/vsmp/include/registers.h
>>  create mode 100644 drivers/virt/vsmp/version/version.c
>>  create mode 100644 drivers/virt/vsmp/version/version.h
>>  create mode 100644 drivers/virt/vsmp/vsmp_main.c
>> 
>> diff --git a/Documentation/ABI/stable/sysfs-driver-vsmp b/Documentation/ABI/stable/sysfs-driver-vsmp
>> new file mode 100644
>> index 000000000000..18a0a62f40ed
>> --- /dev/null
>> +++ b/Documentation/ABI/stable/sysfs-driver-vsmp
>> @@ -0,0 +1,5 @@
>> +What:           /sys/hypervisor/vsmp/version
>> +Date:           Aug 2022
>
>August is almost over :(
will fix, thank!

>
>> +Contact:        Eial Czerwacki <eial.czerwacki@sap.com>
>> +             linux-vsmp@sap.com
>
>No need for an alias here.
it's not an alias, it is a shard mailbox for the team so others can
view the mails history.
I saw that the same methodology is done with mailing lists.

>
>> +Description:    Shows the full version of the vSMP hypervisor
>
>That's not very descriptive, what is this supposed to look like?
I'll update the entry with the general structure of the format.

>
>
>> diff --git a/MAINTAINERS b/MAINTAINERS
>> index f512b430c7cb..cf74089c4d19 100644
>> --- a/MAINTAINERS
>> +++ b/MAINTAINERS
>> @@ -21783,6 +21783,12 @@ F:   lib/test_printf.c
>>  F:   lib/test_scanf.c
>>  F:   lib/vsprintf.c
>> 
>> +VSMP GUEST DRIVER
>> +M:   Eial Czerwacki <eial.czerwacki@sap.com>
>> +M:   linux-vsmp@sap.com
>
>Again, no random aliases please, stick to a person as a contact.
see above comment

>
>> +S:   Maintained
>> +F:   drivers/virt/vsmp
>> +
>>  VT1211 HARDWARE MONITOR DRIVER
>>  M:   Juerg Haefliger <juergh@gmail.com>
>>  L:   linux-hwmon@vger.kernel.org
>> diff --git a/drivers/virt/Kconfig b/drivers/virt/Kconfig
>> index 87ef258cec64..9f283f476674 100644
>> --- a/drivers/virt/Kconfig
>> +++ b/drivers/virt/Kconfig
>> @@ -52,4 +52,6 @@ source "drivers/virt/coco/efi_secret/Kconfig"
>> 
>>  source "drivers/virt/coco/sev-guest/Kconfig"
>> 
>> +source "drivers/virt/vsmp/Kconfig"
>> +
>>  endif
>> diff --git a/drivers/virt/Makefile b/drivers/virt/Makefile
>> index 093674e05c40..159ba37cb471 100644
>> --- a/drivers/virt/Makefile
>> +++ b/drivers/virt/Makefile
>> @@ -11,3 +11,5 @@ obj-$(CONFIG_NITRO_ENCLAVES)        += nitro_enclaves/
>>  obj-$(CONFIG_ACRN_HSM)               += acrn/
>>  obj-$(CONFIG_EFI_SECRET)     += coco/efi_secret/
>>  obj-$(CONFIG_SEV_GUEST)              += coco/sev-guest/
>> +
>> +obj-$(CONFIG_VSMP)             += vsmp/
>
>Why a blank line?
>
>> diff --git a/drivers/virt/vsmp/Kconfig b/drivers/virt/vsmp/Kconfig
>> new file mode 100644
>> index 000000000000..4e1d7e0dc746
>> --- /dev/null
>> +++ b/drivers/virt/vsmp/Kconfig
>> @@ -0,0 +1,11 @@
>> +# SPDX-License-Identifier: GPL-2.0-only
>> +config VSMP
>> +     tristate "vSMP Guest Support"
>> +     depends on SYS_HYPERVISOR && X86_64 && PCI
>> +     help
>> +       Support for vSMP Guest Driver.
>> +
>> +       This driver allows information gathering of data from the vSMP hypervisor when
>> +       running on top of a vSMP-based hypervisor.
>> +
>> +       If unsure, say no.
>
>No module name information?
will fix both

>
>
>> diff --git a/drivers/virt/vsmp/Makefile b/drivers/virt/vsmp/Makefile
>> new file mode 100644
>> index 000000000000..f637097e19f2
>> --- /dev/null
>> +++ b/drivers/virt/vsmp/Makefile
>> @@ -0,0 +1,7 @@
>> +# SPDX-License-Identifier: GPL-2.0
>> +#
>> +# Makefile for vSMP Guest drivers
>> +#
>> +
>> +obj-$(CONFIG_VSMP) = vsmp.o
>> +vsmp-y := vsmp_main.o api/api.o version/version.o
>> diff --git a/drivers/virt/vsmp/api/api.c b/drivers/virt/vsmp/api/api.c
>> new file mode 100644
>> index 000000000000..6e40935907bc
>> --- /dev/null
>> +++ b/drivers/virt/vsmp/api/api.c
>> @@ -0,0 +1,249 @@
>> +// SPDX-License-Identifier: GPL-2.0
>> +
>> +/*
>> + * vSMP driver api
>
>"driver api?"  What is that?
will update headers better after the files structure is decided

>
>> + * (C) Copyright 2022 SAP SE
>> + */
>> +
>> +#include "api.h"
>> +
>> +static void __iomem *cfg_addr;
>> +static struct kobject *vsmp_sysfs_kobj;
>> +static struct pci_dev *vsmp_dev_obj;
>
>Do not make it so that you can only have one device in the system like
>this.  Make it dynamic and properly tie into the driver model and then
>you will have no such restrictions at all.
there can be only one such device on the system.

>
>> +
>> +/* R/W ops handlers */
>
>I do not understand this comment at all, which is not a good sign...
will update such comments to be more informative

>
>> +
>> +/*
>> + * Init a vsmp firmware operation object
>> + */
>> +int vsmp_init_op(struct fw_ops *op, ssize_t max_size,
>> +              enum vsmp_fw_action action)
>> +{
>> +     op->hwi_block_size = max_size;
>> +     op->action = action;
>> +     op->buff_offset = op->hwi_block_size;
>> +
>> +     op->buff = kzalloc(op->hwi_block_size, GFP_KERNEL);
>> +     if (!op->buff)
>> +             return -ENOMEM;
>> +
>> +     vsmp_reset_op(op);
>> +
>> +     return 0;
>> +}
>> +
>> +/*
>> + * Release an vsmp firmware operation object
>> + */
>> +void vsmp_release_op(struct fw_ops *op)
>
>Why are all of these global symbols?
>
>If you put it all into one .c file, it's much easier.
>
>> +{
>> +     if (!op) {
>> +             WARN_ON(!op);
>> +             return;
>> +     }
>> +
>> +     if (!op->buff) {
>> +             WARN_ON(!op->buff);
>> +             return;
>> +     }
>> +
>> +     kfree(op->buff);
>> +     memset(op, 0, sizeof(*op));
>> +}
>> +
>> +/*
>> + * Reset a vsmp firmware operation object
>> + */
>> +void vsmp_reset_op(struct fw_ops *op)
>> +{
>> +     memset(op->buff, 0, op->hwi_block_size);
>> +     op->buff_offset = op->hwi_block_size;
>> +}
>> +
>> +/* Regs/Buffs R/W handlers */
>> +
>> +/*
>> + * Read a value from a specific register in the vSMP's device config space
>> + */
>> +u64 vsmp_read_reg_from_cfg(u64 reg, enum reg_size_type type)
>> +{
>> +     u64 ret_val;
>> +
>> +     switch (type) {
>> +     case VSMP_CTL_REG_SIZE_8BIT:
>> +             ret_val = readb(cfg_addr + reg);
>> +             break;
>> +
>> +     case VSMP_CTL_REG_SIZE_16BIT:
>> +             ret_val = readw(cfg_addr + reg);
>> +             break;
>> +
>> +     case VSMP_CTL_REG_SIZE_32BIT:
>> +             ret_val = readl(cfg_addr + reg);
>> +             break;
>> +
>> +     case VSMP_CTL_REG_SIZE_64BIT:
>> +             ret_val = readq(cfg_addr + reg);
>> +             break;
>> +
>> +     default:
>> +             dev_err(get_dev(), "Unsupported reg size type %d.\n", type);
>> +             ret_val = (u64) -EINVAL;
>> +     }
>> +
>> +     dev_dbg(get_dev(), "%s: read 0x%llx from reg 0x%llx of %d bits\n",
>> +             __func__, ret_val, reg, (type + 1) * 8);
>> +     return ret_val;
>> +}
>> +
>> +/*
>> + * Read a buffer from the bar byte by byte for halt on
>> + * null termination.
>> + * Expected buffs are strings.
>> + */
>> +static ssize_t read_buff_from_bar_in_bytes(char *out, u8 __iomem *buff, ssize_t len)
>> +{
>> +     u32 i;
>> +
>> +     for (i = 0; i < len; i++) {
>> +             out[i] = ioread8(&buff[i]);
>> +             if (!out[i])
>> +                     break;
>
>So even if there is a failure here (0?), you return success?
you are correct, thanks

>
>> +     }
>> +
>> +     return i;
>> +}
>> +
>> +/*
>> + * Read a buffer from a specific offset in a specific bar,
>> + * maxed to a predefined len size-wise from the vSMP device
>> + */
>> +int vsmp_read_buff_from_bar(u8 bar, u32 offset, char *out, ssize_t len,
>> +                         bool halt_on_null)
>> +{
>> +     u8 __iomem *buff;
>> +     u64 bar_start = pci_resource_start(vsmp_dev_obj, bar);
>> +     u32 bar_len = pci_resource_len(vsmp_dev_obj, bar);
>> +     ssize_t actual_len = len;
>> +
>> +     /* incase of overflow, warn and use max len possible */
>> +     if ((offset + len) > bar_len) {
>> +             WARN_ON((offset + len) > actual_len);
>
>Please no new WARN_ON, just handl the error properly and recover from
>it.
I miss understood the checkpatch output

>
>> +             actual_len = bar_len - offset;
>> +             dev_dbg(get_dev(), "%lu overflows bar len, using %ld len instead\n",
>
>get_dev() is not a good function name for the global symbol space at
>all.  Just pass in your device (hint, you have it here already), and use
>that.
thanks, I'll look into it

>
>> +                     len, actual_len);
>> +     }
>> +
>> +     buff = ioremap(bar_start + offset, actual_len);
>> +     if (!buff)
>> +             return -ENOMEM;
>> +
>> +     if (halt_on_null)
>> +             read_buff_from_bar_in_bytes(out, buff, len);
>> +     else
>> +             memcpy_fromio(out, buff, len);
>> +
>> +     iounmap(buff);
>> +
>> +     return 0;
>> +}
>> +
>> +/*
>> + * Generic function to read from the bar
>> + */
>> +ssize_t vsmp_generic_buff_read(struct fw_ops *op, u8 bar, u64 reg,
>> +                            char *buf, loff_t off, ssize_t count)
>> +{
>> +     ssize_t ret_val = 0;
>> +
>> +     if (op->buff_offset >= op->hwi_block_size) {    /* perform H/W op */
>> +             vsmp_reset_op(op);
>> +
>> +             ret_val = vsmp_read_buff_from_bar(bar, reg, op->buff, op->hwi_block_size, false);
>> +             if (ret_val) {
>> +                     dev_err(get_dev(), "%s operation failed\n",
>> +                             (op->action == FW_READ) ? "read" : "write");
>> +             }
>> +             op->buff_offset = 0;
>> +     }
>> +
>> +     if (ret_val)
>> +             return ret_val;
>> +
>> +     return memory_read_from_buffer(buf, count, &op->buff_offset, op->buff, op->hwi_block_size);
>> +}
>> +
>> +/* sysfs handlers */
>> +
>> +/*
>> + * Register the vSMP sysfs object for user space interaction
>> + */
>> +int vsmp_register_sysfs_group(const struct bin_attribute *bin_attr)
>> +{
>> +     int error = -EINVAL;
>> +
>> +     if (vsmp_sysfs_kobj && bin_attr) {
>> +             error = sysfs_create_bin_file(vsmp_sysfs_kobj, bin_attr);
>
>You raced userspace and lost :(
not sure I understand, can you elaborate more please?
>
>And why is your version file a binary file?  It should just be a small
>text string, right?
not so small, it can reach up to 512kb.
that is why I decided to go with binary, I understood that the text is rather limited.

>
>> +             if (error)
>> +                     dev_err(get_dev(), "Failed to register sysfs entry (%d)\n", error);
>> +     }
>> +
>> +     return error;
>> +}
>> +
>> +/*
>> + * Deregister the vSMP sysfs object for user space interaction
>> + */
>> +void vsmp_deregister_sysfs_group(const struct bin_attribute *bin_attr)
>> +{
>> +     if (vsmp_sysfs_kobj && bin_attr)
>> +             sysfs_remove_bin_file(vsmp_sysfs_kobj, bin_attr);
>> +}
>
>Why all the indirection here?  Once you clean this up to be in one file,
>it will be much smaller as you will not need this middle layer at all.
>
>> +
>> +/* Generic functions */
>> +
>> +/*
>> + * Open the cfg address space of the vSDP device
>> + */
>> +int open_cfg_addr(struct pci_dev *pdev)
>> +{
>> +     u64 cfg_start;
>> +     u32 cfg_len;
>> +
>> +     vsmp_dev_obj = pdev;
>> +     cfg_start = pci_resource_start(vsmp_dev_obj, 0);
>> +     cfg_len = pci_resource_len(vsmp_dev_obj, 0);
>> +
>> +     dev_dbg(get_dev(), "Mapping bar 0: [0x%llx,0x%llx]\n",
>> +             cfg_start, cfg_start + cfg_len);
>
>Again, you have a device, use that.  Goes for the whole driver.
I think I've missed something, please correct me if I'm wrong,
I don't need to save the pdev because there is an existing framework
that provides it?

>
>> +#define FILE_PREM 0444
>
>File permission of what?  And shouldn't it be "PERM", not "PREM"?  And
>why do you need it at all?  Just use the proper sysfs macros and you
>never need it.  See below.
all the sysfs files (1 for now) should be read for all users.

>
>> +
>> +/* Regs/Buffs R/W handlers */
>> +#define vsmp_read_reg32_from_cfg(_reg_) \
>> +     ((u32) vsmp_read_reg_from_cfg((_reg_), VSMP_CTL_REG_SIZE_32BIT))
>> +
>> +u64 vsmp_read_reg_from_cfg(u64 reg, enum reg_size_type type);
>> +ssize_t vsmp_generic_buff_read(struct fw_ops *op, u8 bar, u64 reg,
>> +                            char *buf, loff_t off, ssize_t count);
>> +int vsmp_read_buff_from_bar(u8 bar, u32 offset, char *out, ssize_t len,
>> +                         bool halt_on_null);
>> +
>> +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 vsmp_register_sysfs_group(const struct bin_attribute *bin_attr);
>> +void vsmp_deregister_sysfs_group(const struct bin_attribute *bin_attr);
>> +
>> +int open_cfg_addr(struct pci_dev *pdev);
>> +int init_sysfs(void);
>> +void cleanup(void);
>> +const struct device *get_dev(void);
>> +#endif /* VSMP_API_H */
>> diff --git a/drivers/virt/vsmp/include/registers.h b/drivers/virt/vsmp/include/registers.h
>> new file mode 100644
>> index 000000000000..b6458d25e3b7
>> --- /dev/null
>> +++ b/drivers/virt/vsmp/include/registers.h
>> @@ -0,0 +1,12 @@
>> +/* SPDX-License-Identifier: GPL-2.0 */
>> +/*
>> + * vSMP driver registers
>> + * (C) Copyright 2022 SAP SE
>> + */
>> +
>> +#ifndef VSMP_REGSITERS_H
>> +#define VSMP_REGSITERS_H
>> +
>> +#define VSMP_VERSION_REG 0x0c
>> +
>> +#endif /* VSMP_REGSITERS_H */
>
>Smallest .h file ever.   12 lines for a single #define, please don't do
>that.
you are right, it should be atleast part of the api header

>
>
>> diff --git a/drivers/virt/vsmp/version/version.c b/drivers/virt/vsmp/version/version.c
>> new file mode 100644
>> index 000000000000..d8ad771daf28
>> --- /dev/null
>> +++ b/drivers/virt/vsmp/version/version.c
>> @@ -0,0 +1,118 @@
>> +// SPDX-License-Identifier: GPL-2.0
>> +
>> +/*
>> + * vSMP driver version module
>> + * (C) Copyright 2022 SAP SE
>> + */
>> +
>> +#include <linux/slab.h>
>> +#include <linux/kobject.h>
>> +
>> +#include "../api/api.h"
>> +#include "../include/registers.h"
>> +
>> +/*
>> + * This is the maximal possible length of the version which is a text string
>> + * the real len is usually much smaller, thus the driver uses this once to read
>> + * the version string and record it's actual len.
>> + * From that point and on, the actual len will be used in each call.
>> + */
>> +#define VERSION_MAX_LEN (1 << 19)
>> +
>> +static struct fw_ops op;
>> +
>> +static ssize_t version_read(struct file *filp, struct kobject *kobj,
>> +                         struct bin_attribute *bin_attr,
>> +                         char *buf, loff_t off, size_t count)
>> +{
>> +     u64 reg_val = vsmp_read_reg32_from_cfg(VSMP_VERSION_REG);
>> +     ssize_t ret_val;
>> +
>> +     if (reg_val < 0) {
>> +             dev_err(get_dev(), "Failed to value of reg 0x%x\n", VSMP_VERSION_REG);
>> +             return 0;
>> +     }
>> +
>> +     ret_val = vsmp_generic_buff_read(&op, 0, reg_val, buf, off, count);
>> +     if (ret_val < 0) {
>> +             dev_err(get_dev(), "Failed to read version (%ld)\n", ret_val);
>> +             return 0;
>> +     }
>> +
>> +     buf[ret_val++] = '\n';
>> +
>> +     return ret_val;
>> +}
>> +
>> +struct bin_attribute version_raw_attr = __BIN_ATTR(version, FILE_PREM,
>> +                                                version_read, NULL, VERSION_MAX_LEN);
>
>__BIN_ATTR_RO()?
>
>But again, why is this a binary file?
thanks, will use that, my bad.
as for the bin, if there is no size limitation for the str attribute, I'll switch to it.
>
>I stopped reviewing here.  Please clean up all of the above first, and
>then the rest of the file will be smaller and easier to review.
>
>thanks,
>
>greg k-h

I'll submit a new version after a proper file structure will be decided
thanks again for you comments.

Eial

Re: [PATCH v2] drivers/virt/vSMP: new driver

[Greg Kroah-Hartman]

On Thu, Aug 25, 2022 at 09:17:57AM +0000, Czerwacki, Eial wrote:
> Greetings Greg,
> 
> thank you for the comments, see my comment below.
> >On Thu, Aug 25, 2022 at 06:24:02AM +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 api part which facilitates communication with
> >> the hypervisor and version which displays the hypervisor's version.
> >> 
> >> This patch s based on previous patches sent to the staging tree mailing
> >> lists
> >> 
> >> Signed-off-by: Eial Czerwacki <eial.czerwacki@sap.com>
> >> Acked-by: Leonid Arsh <leonid.arsh@sap.com>
> >> Acked-by: Oren Twaig <oren.twaig@sap.com>
> >> CC: SAP vSMP Linux Maintainer <linux.vsmp@sap.com>
> >> CC: Greg Kroah-Hartman <gregkh@linuxfoundation.org>
> >> CC: Arnd Bergmann <arnd@arndb.de>
> >> CC: Dan Carpenter <dan.carpenter@oracle.com>
> >> CC: Andra Paraschiv <andraprs@amazon.com>
> >> CC: Borislav Petkov <bp@suse.de>
> >> CC: Brijesh Singh <brijesh.singh@amd.com>
> >> CC: Eric Biggers <ebiggers@google.com>
> >> CC: Fei Li <fei1.li@intel.com>
> >> CC: Hans de Goede <hdegoede@redhat.com>
> >> CC: Jens Axboe <axboe@kernel.dk>
> >> CC: Mauro Carvalho Chehab <mchehab+huawei@kernel.org>
> >> 
> >> v1 -> v2:
> >>        - fix -0 var init in add_sysfs_entries (pointed out by Dan Carpenter)
> >> ---
> >>  Documentation/ABI/stable/sysfs-driver-vsmp |   5 +
> >>  MAINTAINERS                                |   6 +
> >>  drivers/virt/Kconfig                       |   2 +
> >>  drivers/virt/Makefile                      |   2 +
> >>  drivers/virt/vsmp/Kconfig                  |  11 +
> >>  drivers/virt/vsmp/Makefile                 |   7 +
> >>  drivers/virt/vsmp/api/api.c                | 249 +++++++++++++++++++++
> >>  drivers/virt/vsmp/api/api.h                |  69 ++++++
> >>  drivers/virt/vsmp/include/registers.h      |  12 +
> >>  drivers/virt/vsmp/version/version.c        | 118 ++++++++++
> >>  drivers/virt/vsmp/version/version.h        |  14 ++
> >>  drivers/virt/vsmp/vsmp_main.c              | 110 +++++++++
> >>  12 files changed, 605 insertions(+)
> >
> >Why do you have all of these different .c and .h files for only 600
> >lines of code?  Shouldn't this all just be in a single .c file?  Why
> >have a subdir for just 300 lines?
> >
> >Please mush this all into a single .c file going forward, speading it
> >out like this makes no sense.
> the current driver has two modules, version and api, in later versions of the
> driver support for additional features will be added.
> at that time, this might cause the single file to inflate, so we thought to split it
> to several files to make it more organized.
> is there any way to keep it in different code files?

Keep it small and together now.  If you need to change it in the future,
wonderful, do it then.  Don't do things that we never know will happen
or not.

One file, and one module, is great to start with.  Please do that for
now.


> 
> >
> >>  create mode 100644 Documentation/ABI/stable/sysfs-driver-vsmp
> >>  create mode 100644 drivers/virt/vsmp/Kconfig
> >>  create mode 100644 drivers/virt/vsmp/Makefile
> >>  create mode 100644 drivers/virt/vsmp/api/api.c
> >>  create mode 100644 drivers/virt/vsmp/api/api.h
> >>  create mode 100644 drivers/virt/vsmp/include/registers.h
> >>  create mode 100644 drivers/virt/vsmp/version/version.c
> >>  create mode 100644 drivers/virt/vsmp/version/version.h
> >>  create mode 100644 drivers/virt/vsmp/vsmp_main.c
> >> 
> >> diff --git a/Documentation/ABI/stable/sysfs-driver-vsmp b/Documentation/ABI/stable/sysfs-driver-vsmp
> >> new file mode 100644
> >> index 000000000000..18a0a62f40ed
> >> --- /dev/null
> >> +++ b/Documentation/ABI/stable/sysfs-driver-vsmp
> >> @@ -0,0 +1,5 @@
> >> +What:           /sys/hypervisor/vsmp/version
> >> +Date:           Aug 2022
> >
> >August is almost over :(
> will fix, thank!
> 
> >
> >> +Contact:        Eial Czerwacki <eial.czerwacki@sap.com>
> >> +             linux-vsmp@sap.com
> >
> >No need for an alias here.
> it's not an alias, it is a shard mailbox for the team so others can
> view the mails history.
> I saw that the same methodology is done with mailing lists.

That's a mailing list?  Ah, didn't realize that, sorry, was not obvious.

> >> diff --git a/MAINTAINERS b/MAINTAINERS
> >> index f512b430c7cb..cf74089c4d19 100644
> >> --- a/MAINTAINERS
> >> +++ b/MAINTAINERS
> >> @@ -21783,6 +21783,12 @@ F:   lib/test_printf.c
> >>  F:   lib/test_scanf.c
> >>  F:   lib/vsprintf.c
> >> 
> >> +VSMP GUEST DRIVER
> >> +M:   Eial Czerwacki <eial.czerwacki@sap.com>
> >> +M:   linux-vsmp@sap.com
> >
> >Again, no random aliases please, stick to a person as a contact.
> see above comment

If this is a mailing list, mark it as such, you did not do so.


> >> +/*
> >> + * Register the vSMP sysfs object for user space interaction
> >> + */
> >> +int vsmp_register_sysfs_group(const struct bin_attribute *bin_attr)
> >> +{
> >> +     int error = -EINVAL;
> >> +
> >> +     if (vsmp_sysfs_kobj && bin_attr) {
> >> +             error = sysfs_create_bin_file(vsmp_sysfs_kobj, bin_attr);
> >
> >You raced userspace and lost :(
> not sure I understand, can you elaborate more please?

Fix up your sysfs usage first, and then we can revisit this.


> >
> >And why is your version file a binary file?  It should just be a small
> >text string, right?
> not so small, it can reach up to 512kb.

That was not obvious at all.  Please document this.

And how in the world is a "version" that big?  What exactly does this
contain?

> that is why I decided to go with binary, I understood that the text is rather limited.

That is true, sysfs is "one value per file", this can not be a file that
you parse.


> >> +int open_cfg_addr(struct pci_dev *pdev)
> >> +{
> >> +     u64 cfg_start;
> >> +     u32 cfg_len;
> >> +
> >> +     vsmp_dev_obj = pdev;
> >> +     cfg_start = pci_resource_start(vsmp_dev_obj, 0);
> >> +     cfg_len = pci_resource_len(vsmp_dev_obj, 0);
> >> +
> >> +     dev_dbg(get_dev(), "Mapping bar 0: [0x%llx,0x%llx]\n",
> >> +             cfg_start, cfg_start + cfg_len);
> >
> >Again, you have a device, use that.  Goes for the whole driver.
> I think I've missed something, please correct me if I'm wrong,
> I don't need to save the pdev because there is an existing framework
> that provides it?

You have pdev, use that in your dev_dbg() call.

> >> +#define FILE_PREM 0444
> >
> >File permission of what?  And shouldn't it be "PERM", not "PREM"?  And
> >why do you need it at all?  Just use the proper sysfs macros and you
> >never need it.  See below.
> all the sysfs files (1 for now) should be read for all users.

Then use the proper sysfs macros for that, do not set it yourself.

> I'll submit a new version after a proper file structure will be decided
> thanks again for you comments.

Again, stick with one file to start with.  You can always change that
later once it is merged if you really need it.

thanks,

greg k-h

Re: [PATCH v2] drivers/virt/vSMP: new driver

[Czerwacki, Eial]

>On Thu, Aug 25, 2022 at 09:17:57AM +0000, Czerwacki, Eial wrote:
>> Greetings Greg,
>> 
>> thank you for the comments, see my comment below.
>> >On Thu, Aug 25, 2022 at 06:24:02AM +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 api part which facilitates communication with
>> >> the hypervisor and version which displays the hypervisor's version.
>> >> 
>> >> This patch s based on previous patches sent to the staging tree mailing
>> >> lists
>> >> 
>> >> Signed-off-by: Eial Czerwacki <eial.czerwacki@sap.com>
>> >> Acked-by: Leonid Arsh <leonid.arsh@sap.com>
>> >> Acked-by: Oren Twaig <oren.twaig@sap.com>
>> >> CC: SAP vSMP Linux Maintainer <linux.vsmp@sap.com>
>> >> CC: Greg Kroah-Hartman <gregkh@linuxfoundation.org>
>> >> CC: Arnd Bergmann <arnd@arndb.de>
>> >> CC: Dan Carpenter <dan.carpenter@oracle.com>
>> >> CC: Andra Paraschiv <andraprs@amazon.com>
>> >> CC: Borislav Petkov <bp@suse.de>
>> >> CC: Brijesh Singh <brijesh.singh@amd.com>
>> >> CC: Eric Biggers <ebiggers@google.com>
>> >> CC: Fei Li <fei1.li@intel.com>
>> >> CC: Hans de Goede <hdegoede@redhat.com>
>> >> CC: Jens Axboe <axboe@kernel.dk>
>> >> CC: Mauro Carvalho Chehab <mchehab+huawei@kernel.org>
>> >> 
>> >> v1 -> v2:
>> >>        - fix -0 var init in add_sysfs_entries (pointed out by Dan Carpenter)
>> >> ---
>> >>  Documentation/ABI/stable/sysfs-driver-vsmp |   5 +
>> >>  MAINTAINERS                                |   6 +
>> >>  drivers/virt/Kconfig                       |   2 +
>> >>  drivers/virt/Makefile                      |   2 +
>> >>  drivers/virt/vsmp/Kconfig                  |  11 +
>> >>  drivers/virt/vsmp/Makefile                 |   7 +
>> >>  drivers/virt/vsmp/api/api.c                | 249 +++++++++++++++++++++
>> >>  drivers/virt/vsmp/api/api.h                |  69 ++++++
>> >>  drivers/virt/vsmp/include/registers.h      |  12 +
>> >>  drivers/virt/vsmp/version/version.c        | 118 ++++++++++
>> >>  drivers/virt/vsmp/version/version.h        |  14 ++
>> >>  drivers/virt/vsmp/vsmp_main.c              | 110 +++++++++
>> >>  12 files changed, 605 insertions(+)
>> >
>> >Why do you have all of these different .c and .h files for only 600
>> >lines of code?  Shouldn't this all just be in a single .c file?  Why
>> >have a subdir for just 300 lines?
>> >
>> >Please mush this all into a single .c file going forward, speading it
>> >out like this makes no sense.
>> the current driver has two modules, version and api, in later versions of the
>> driver support for additional features will be added.
>> at that time, this might cause the single file to inflate, so we thought to split it
>> to several files to make it more organized.
>> is there any way to keep it in different code files?
>
>Keep it small and together now.  If you need to change it in the future,
>wonderful, do it then.  Don't do things that we never know will happen
>or not.
>
>One file, and one module, is great to start with.  Please do that for
>now.
>
I understand, I'll restrucure it into one file

>
>> 
>> >
>> >>  create mode 100644 Documentation/ABI/stable/sysfs-driver-vsmp
>> >>  create mode 100644 drivers/virt/vsmp/Kconfig
>> >>  create mode 100644 drivers/virt/vsmp/Makefile
>> >>  create mode 100644 drivers/virt/vsmp/api/api.c
>> >>  create mode 100644 drivers/virt/vsmp/api/api.h
>> >>  create mode 100644 drivers/virt/vsmp/include/registers.h
>> >>  create mode 100644 drivers/virt/vsmp/version/version.c
>> >>  create mode 100644 drivers/virt/vsmp/version/version.h
>> >>  create mode 100644 drivers/virt/vsmp/vsmp_main.c
>> >> 
>> >> diff --git a/Documentation/ABI/stable/sysfs-driver-vsmp b/Documentation/ABI/stable/sysfs-driver-vsmp
>> >> new file mode 100644
>> >> index 000000000000..18a0a62f40ed
>> >> --- /dev/null
>> >> +++ b/Documentation/ABI/stable/sysfs-driver-vsmp
>> >> @@ -0,0 +1,5 @@
>> >> +What:           /sys/hypervisor/vsmp/version
>> >> +Date:           Aug 2022
>> >
>> >August is almost over :(
>> will fix, thank!
>> 
>> >
>> >> +Contact:        Eial Czerwacki <eial.czerwacki@sap.com>
>> >> +             linux-vsmp@sap.com
>> >
>> >No need for an alias here.
>> it's not an alias, it is a shard mailbox for the team so others can
>> view the mails history.
>> I saw that the same methodology is done with mailing lists.
>
>That's a mailing list?  Ah, didn't realize that, sorry, was not obvious.
>
>> >> diff --git a/MAINTAINERS b/MAINTAINERS
>> >> index f512b430c7cb..cf74089c4d19 100644
>> >> --- a/MAINTAINERS
>> >> +++ b/MAINTAINERS
>> >> @@ -21783,6 +21783,12 @@ F:   lib/test_printf.c
>> >>  F:   lib/test_scanf.c
>> >>  F:   lib/vsprintf.c
>> >> 
>> >> +VSMP GUEST DRIVER
>> >> +M:   Eial Czerwacki <eial.czerwacki@sap.com>
>> >> +M:   linux-vsmp@sap.com
>> >
>> >Again, no random aliases please, stick to a person as a contact.
>> see above comment
>
>If this is a mailing list, mark it as such, you did not do so.
I'll mark it accordingly.

>
>
>> >> +/*
>> >> + * Register the vSMP sysfs object for user space interaction
>> >> + */
>> >> +int vsmp_register_sysfs_group(const struct bin_attribute *bin_attr)
>> >> +{
>> >> +     int error = -EINVAL;
>> >> +
>> >> +     if (vsmp_sysfs_kobj && bin_attr) {
>> >> +             error = sysfs_create_bin_file(vsmp_sysfs_kobj, bin_attr);
>> >
>> >You raced userspace and lost :(
>> not sure I understand, can you elaborate more please?
>
>Fix up your sysfs usage first, and then we can revisit this.
will do.

>
>
>> >
>> >And why is your version file a binary file?  It should just be a small
>> >text string, right?
>> not so small, it can reach up to 512kb.
>
>That was not obvious at all.  Please document this.
where should the document be?
in the code as a comment or in another file?

>
>And how in the world is a "version" that big?  What exactly does this
>contain?
it 's size depends on the number of resources it uses.
here is an example:
:~> cat /sys/hypervisor/vsmp/version  
SAP vSMP Foundation: 10.6.2862.0 (Aug 22 2022 15:21:02)
System configuration:
   Boards:      2
      1 x Proc. + I/O + Memory
      1 x NVM devices (Amazon.com Amazon EC2 NVMe Instance Storage)
   Processors:  1, Cores: 2, Threads: 4
       Intel(R) Xeon(R) Platinum 8124M CPU @ 3.00GHz Stepping 04
   Memory (MB): 30976 (of 103192), Cache: 7527, Private: 64689
      1 x  6400MB    [ 7825/ 321/ 1104]      
      1 x 24576MB    [95367/7206/63585]       00:1f.0#1
   Boot device: [HDD] NVMe: Amazon Elastic Block Store        
Supported until: Aug 22 2024

>
>> that is why I decided to go with binary, I understood that the text is rather limited.
>
>That is true, sysfs is "one value per file", this can not be a file that
>you parse.
should I keep it as bin then?

>
>
>> >> +int open_cfg_addr(struct pci_dev *pdev)
>> >> +{
>> >> +     u64 cfg_start;
>> >> +     u32 cfg_len;
>> >> +
>> >> +     vsmp_dev_obj = pdev;
>> >> +     cfg_start = pci_resource_start(vsmp_dev_obj, 0);
>> >> +     cfg_len = pci_resource_len(vsmp_dev_obj, 0);
>> >> +
>> >> +     dev_dbg(get_dev(), "Mapping bar 0: [0x%llx,0x%llx]\n",
>> >> +             cfg_start, cfg_start + cfg_len);
>> >
>> >Again, you have a device, use that.  Goes for the whole driver.
>> I think I've missed something, please correct me if I'm wrong,
>> I don't need to save the pdev because there is an existing framework
>> that provides it?
>
>You have pdev, use that in your dev_dbg() call.
I have pdev when the probe cb is called, however in other funcs I
don't have it.
I'll look into other drivers see what they are doing instead
of wasting your time

>
>> >> +#define FILE_PREM 0444
>> >
>> >File permission of what?  And shouldn't it be "PERM", not "PREM"?  And
>> >why do you need it at all?  Just use the proper sysfs macros and you
>> >never need it.  See below.
>> all the sysfs files (1 for now) should be read for all users.
>
>Then use the proper sysfs macros for that, do not set it yourself.
(Y)

>
>> I'll submit a new version after a proper file structure will be decided
>> thanks again for you comments.
>
>Again, stick with one file to start with.  You can always change that
>later once it is merged if you really need it.
>
>thanks,
>
>greg k-h

great, thanks.

Eial

Re: [PATCH v2] drivers/virt/vSMP: new driver

[Greg Kroah-Hartman]

On Thu, Aug 25, 2022 at 10:16:59AM +0000, Czerwacki, Eial wrote:
> >> >And why is your version file a binary file?  It should just be a small
> >> >text string, right?
> >> not so small, it can reach up to 512kb.
> >
> >That was not obvious at all.  Please document this.
> where should the document be?
> in the code as a comment or in another file?

In the Documentation/ABI/ file that describes this file.

> >And how in the world is a "version" that big?  What exactly does this
> >contain?
> it 's size depends on the number of resources it uses.
> here is an example:
> :~> cat /sys/hypervisor/vsmp/version  
> SAP vSMP Foundation: 10.6.2862.0 (Aug 22 2022 15:21:02)
> System configuration:
>    Boards:      2
>       1 x Proc. + I/O + Memory
>       1 x NVM devices (Amazon.com Amazon EC2 NVMe Instance Storage)
>    Processors:  1, Cores: 2, Threads: 4
>        Intel(R) Xeon(R) Platinum 8124M CPU @ 3.00GHz Stepping 04
>    Memory (MB): 30976 (of 103192), Cache: 7527, Private: 64689
>       1 x  6400MB    [ 7825/ 321/ 1104]      
>       1 x 24576MB    [95367/7206/63585]       00:1f.0#1
>    Boot device: [HDD] NVMe: Amazon Elastic Block Store        
> Supported until: Aug 22 2024

That is crazy, and is not a version.  It's a "configuration".

You have a version up there, just export that as a simple string
"10.6.2862.0".  What is the rest of that stuff needed for?  Who will use
it?  Should it just be in a debugfs file for debugging things?

Oh, and I love the "Supported until" tag, that's funny :)

> >> that is why I decided to go with binary, I understood that the text is rather limited.
> >
> >That is true, sysfs is "one value per file", this can not be a file that
> >you parse.
> should I keep it as bin then?

See above, make it text only for the version.  If you want to export
other things, be explicit and make them "one value per sysfs file" or
use debugfs for debugging things that no one relies on.

> >You have pdev, use that in your dev_dbg() call.
> I have pdev when the probe cb is called, however in other funcs I
> don't have it.

That's for the other functions to fix up.  Pass in the device pointer to
them.  You are a driver, there is no reason to NOT have a device pointer
in your functions, otherwise you are operating on global state which a
driver should never do.  You will notice that quickly when you remove
the static variable at the top of the file like I asked you to :)

thanks,

greg k-h

Re: [PATCH v2] drivers/virt/vSMP: new driver

[Czerwacki, Eial]

>On Thu, Aug 25, 2022 at 10:16:59AM +0000, Czerwacki, Eial wrote:
>> >> >And why is your version file a binary file?  It should just be a small
>> >> >text string, right?
>> >> not so small, it can reach up to 512kb.
>> >
>> >That was not obvious at all.  Please document this.
>> where should the document be?
>> in the code as a comment or in another file?
>
>In the Documentation/ABI/ file that describes this file.
ok, will place it there

>
>> >And how in the world is a "version" that big?  What exactly does this
>> >contain?
>> it 's size depends on the number of resources it uses.
>> here is an example:
>> :~> cat /sys/hypervisor/vsmp/version  
>> SAP vSMP Foundation: 10.6.2862.0 (Aug 22 2022 15:21:02)
>> System configuration:
>>    Boards:      2
>>       1 x Proc. + I/O + Memory
>>       1 x NVM devices (Amazon.com Amazon EC2 NVMe Instance Storage)
>>    Processors:  1, Cores: 2, Threads: 4
>>        Intel(R) Xeon(R) Platinum 8124M CPU @ 3.00GHz Stepping 04
>>    Memory (MB): 30976 (of 103192), Cache: 7527, Private: 64689
>>       1 x  6400MB    [ 7825/ 321/ 1104]      
>>       1 x 24576MB    [95367/7206/63585]       00:1f.0#1
>>    Boot device: [HDD] NVMe: Amazon Elastic Block Store        
>> Supported until: Aug 22 2024
>
>That is crazy, and is not a version.  It's a "configuration".
it is called version for history reasons...

>
>You have a version up there, just export that as a simple string
>"10.6.2862.0".  What is the rest of that stuff needed for?  Who will use
>it?  Should it just be in a debugfs file for debugging things?
this allows the user and us to know what is the system configuration
in a user friendly ui. it isn't for debugging.
it can be rename to summery instead of version but it is needed
to be accessible all the time, sysfs was a logical location because
afaik, it is less probable to have a system without debugfs rather than
one without sysfs

>
>Oh, and I love the "Supported until" tag, that's funny :)
today's comic relief :)

>
>> >> that is why I decided to go with binary, I understood that the text is rather limited.
>> >
>> >That is true, sysfs is "one value per file", this can not be a file that
>> >you parse.
>> should I keep it as bin then?
>
>See above, make it text only for the version.  If you want to export
>other things, be explicit and make them "one value per sysfs file" or
>use debugfs for debugging things that no one relies on.
so you suggest braking the summery into files, e.g. one for cpus, one for ram and etcetera?

>
>> >You have pdev, use that in your dev_dbg() call.
>> I have pdev when the probe cb is called, however in other funcs I
>> don't have it.
>
>That's for the other functions to fix up.  Pass in the device pointer to
>them.  You are a driver, there is no reason to NOT have a device pointer
>in your functions, otherwise you are operating on global state which a
>driver should never do.  You will notice that quickly when you remove
>the static variable at the top of the file like I asked you to :)
>
>thanks,
>
>greg k-h

understood.

Thanks,

Eial

Re: [PATCH v2] drivers/virt/vSMP: new driver

[Greg Kroah-Hartman]

On Thu, Aug 25, 2022 at 10:41:28AM +0000, Czerwacki, Eial wrote:
> >On Thu, Aug 25, 2022 at 10:16:59AM +0000, Czerwacki, Eial wrote:
> >> >> >And why is your version file a binary file?  It should just be a small
> >> >> >text string, right?
> >> >> not so small, it can reach up to 512kb.
> >> >
> >> >That was not obvious at all.  Please document this.
> >> where should the document be?
> >> in the code as a comment or in another file?
> >
> >In the Documentation/ABI/ file that describes this file.
> ok, will place it there
> 
> >
> >> >And how in the world is a "version" that big?  What exactly does this
> >> >contain?
> >> it 's size depends on the number of resources it uses.
> >> here is an example:
> >> :~> cat /sys/hypervisor/vsmp/version  
> >> SAP vSMP Foundation: 10.6.2862.0 (Aug 22 2022 15:21:02)
> >> System configuration:
> >>    Boards:      2
> >>       1 x Proc. + I/O + Memory
> >>       1 x NVM devices (Amazon.com Amazon EC2 NVMe Instance Storage)
> >>    Processors:  1, Cores: 2, Threads: 4
> >>        Intel(R) Xeon(R) Platinum 8124M CPU @ 3.00GHz Stepping 04
> >>    Memory (MB): 30976 (of 103192), Cache: 7527, Private: 64689
> >>       1 x  6400MB    [ 7825/ 321/ 1104]      
> >>       1 x 24576MB    [95367/7206/63585]       00:1f.0#1
> >>    Boot device: [HDD] NVMe: Amazon Elastic Block Store        
> >> Supported until: Aug 22 2024
> >
> >That is crazy, and is not a version.  It's a "configuration".
> it is called version for history reasons...

There is no "history" here, you can create whatever sane interface you
want right now, there is no backwards compatible issues involved at all.

> >See above, make it text only for the version.  If you want to export
> >other things, be explicit and make them "one value per sysfs file" or
> >use debugfs for debugging things that no one relies on.
> so you suggest braking the summery into files, e.g. one for cpus, one for ram and etcetera?

Again, who uses this information and what is it used for?

thanks,

greg k-h

Re: [PATCH v2] drivers/virt/vSMP: new driver

[Czerwacki, Eial]

>On Thu, Aug 25, 2022 at 10:41:28AM +0000, Czerwacki, Eial wrote:
>> >On Thu, Aug 25, 2022 at 10:16:59AM +0000, Czerwacki, Eial wrote:
>> >> >> >And why is your version file a binary file?  It should just be a small
>> >> >> >text string, right?
>> >> >> not so small, it can reach up to 512kb.
>> >> >
>> >> >That was not obvious at all.  Please document this.
>> >> where should the document be?
>> >> in the code as a comment or in another file?
>> >
>> >In the Documentation/ABI/ file that describes this file.
>> ok, will place it there
>> 
>> >
>> >> >And how in the world is a "version" that big?  What exactly does this
>> >> >contain?
>> >> it 's size depends on the number of resources it uses.
>> >> here is an example:
>> >> :~> cat /sys/hypervisor/vsmp/version  
>> >> SAP vSMP Foundation: 10.6.2862.0 (Aug 22 2022 15:21:02)
>> >> System configuration:
>> >>    Boards:      2
>> >>       1 x Proc. + I/O + Memory
>> >>       1 x NVM devices (Amazon.com Amazon EC2 NVMe Instance Storage)
>> >>    Processors:  1, Cores: 2, Threads: 4
>> >>        Intel(R) Xeon(R) Platinum 8124M CPU @ 3.00GHz Stepping 04
>> >>    Memory (MB): 30976 (of 103192), Cache: 7527, Private: 64689
>> >>       1 x  6400MB    [ 7825/ 321/ 1104]      
>> >>       1 x 24576MB    [95367/7206/63585]       00:1f.0#1
>> >>    Boot device: [HDD] NVMe: Amazon Elastic Block Store        
>> >> Supported until: Aug 22 2024
>> >
>> >That is crazy, and is not a version.  It's a "configuration".
>> it is called version for history reasons...
>
>There is no "history" here, you can create whatever sane interface you
>want right now, there is no backwards compatible issues involved at all.
you are correct, however, it depends on how much change the hypervisor code requires
if any (latter is preferable)

>
>> >See above, make it text only for the version.  If you want to export
>> >other things, be explicit and make them "one value per sysfs file" or
>> >use debugfs for debugging things that no one relies on.
>> so you suggest braking the summery into files, e.g. one for cpus, one for ram and etcetera?
>
>Again, who uses this information and what is it used for?
>
>thanks,
>
>greg k-h

both user who uses the product and the development team.
it is used to provide a summery of the system. for example, which devices are used
by the hypervisor.

Thanks,

Eial

Re: [PATCH v2] drivers/virt/vSMP: new driver

[Greg Kroah-Hartman]

On Thu, Aug 25, 2022 at 12:02:12PM +0000, Czerwacki, Eial wrote:
> >On Thu, Aug 25, 2022 at 10:41:28AM +0000, Czerwacki, Eial wrote:
> >> >On Thu, Aug 25, 2022 at 10:16:59AM +0000, Czerwacki, Eial wrote:
> >> >> >> >And why is your version file a binary file?  It should just be a small
> >> >> >> >text string, right?
> >> >> >> not so small, it can reach up to 512kb.
> >> >> >
> >> >> >That was not obvious at all.  Please document this.
> >> >> where should the document be?
> >> >> in the code as a comment or in another file?
> >> >
> >> >In the Documentation/ABI/ file that describes this file.
> >> ok, will place it there
> >> 
> >> >
> >> >> >And how in the world is a "version" that big?  What exactly does this
> >> >> >contain?
> >> >> it 's size depends on the number of resources it uses.
> >> >> here is an example:
> >> >> :~> cat /sys/hypervisor/vsmp/version  
> >> >> SAP vSMP Foundation: 10.6.2862.0 (Aug 22 2022 15:21:02)
> >> >> System configuration:
> >> >>    Boards:      2
> >> >>       1 x Proc. + I/O + Memory
> >> >>       1 x NVM devices (Amazon.com Amazon EC2 NVMe Instance Storage)
> >> >>    Processors:  1, Cores: 2, Threads: 4
> >> >>        Intel(R) Xeon(R) Platinum 8124M CPU @ 3.00GHz Stepping 04
> >> >>    Memory (MB): 30976 (of 103192), Cache: 7527, Private: 64689
> >> >>       1 x  6400MB    [ 7825/ 321/ 1104]      
> >> >>       1 x 24576MB    [95367/7206/63585]       00:1f.0#1
> >> >>    Boot device: [HDD] NVMe: Amazon Elastic Block Store        
> >> >> Supported until: Aug 22 2024
> >> >
> >> >That is crazy, and is not a version.  It's a "configuration".
> >> it is called version for history reasons...
> >
> >There is no "history" here, you can create whatever sane interface you
> >want right now, there is no backwards compatible issues involved at all.
> you are correct, however, it depends on how much change the hypervisor code requires
> if any (latter is preferable)

I do not understand, again, what tool consumes this today?

> >> >See above, make it text only for the version.  If you want to export
> >> >other things, be explicit and make them "one value per sysfs file" or
> >> >use debugfs for debugging things that no one relies on.
> >> so you suggest braking the summery into files, e.g. one for cpus, one for ram and etcetera?
> >
> >Again, who uses this information and what is it used for?
> >
> >thanks,
> >
> >greg k-h
> 
> both user who uses the product and the development team.
> it is used to provide a summery of the system. for example, which devices are used
> by the hypervisor.

That's a very odd way to display this as a free-flowing, impossible to
parse, file.  Please use something that will be able to be maintained
over time.

thanks,

greg k-h

Re: [PATCH v2] drivers/virt/vSMP: new driver

[Czerwacki, Eial]

>On Thu, Aug 25, 2022 at 12:02:12PM +0000, Czerwacki, Eial wrote:
>> >On Thu, Aug 25, 2022 at 10:41:28AM +0000, Czerwacki, Eial wrote:
>> >> >On Thu, Aug 25, 2022 at 10:16:59AM +0000, Czerwacki, Eial wrote:
>> >> >> >> >And why is your version file a binary file?  It should just be a small
>> >> >> >> >text string, right?
>> >> >> >> not so small, it can reach up to 512kb.
>> >> >> >
>> >> >> >That was not obvious at all.  Please document this.
>> >> >> where should the document be?
>> >> >> in the code as a comment or in another file?
>> >> >
>> >> >In the Documentation/ABI/ file that describes this file.
>> >> ok, will place it there
>> >> 
>> >> >
>> >> >> >And how in the world is a "version" that big?  What exactly does this
>> >> >> >contain?
>> >> >> it 's size depends on the number of resources it uses.
>> >> >> here is an example:
>> >> >> :~> cat /sys/hypervisor/vsmp/version  
>> >> >> SAP vSMP Foundation: 10.6.2862.0 (Aug 22 2022 15:21:02)
>> >> >> System configuration:
>> >> >>    Boards:      2
>> >> >>       1 x Proc. + I/O + Memory
>> >> >>       1 x NVM devices (Amazon.com Amazon EC2 NVMe Instance Storage)
>> >> >>    Processors:  1, Cores: 2, Threads: 4
>> >> >>        Intel(R) Xeon(R) Platinum 8124M CPU @ 3.00GHz Stepping 04
>> >> >>    Memory (MB): 30976 (of 103192), Cache: 7527, Private: 64689
>> >> >>       1 x  6400MB    [ 7825/ 321/ 1104]      
>> >> >>       1 x 24576MB    [95367/7206/63585]       00:1f.0#1
>> >> >>    Boot device: [HDD] NVMe: Amazon Elastic Block Store        
>> >> >> Supported until: Aug 22 2024
>> >> >
>> >> >That is crazy, and is not a version.  It's a "configuration".
>> >> it is called version for history reasons...
>> >
>> >There is no "history" here, you can create whatever sane interface you
>> >want right now, there is no backwards compatible issues involved at all.
>> you are correct, however, it depends on how much change the hypervisor code requires
>> if any (latter is preferable)
>
>I do not understand, again, what tool consumes this today?
there are monitoring utils that parses it.
see comment below for more on this

>
>> >> >See above, make it text only for the version.  If you want to export
>> >> >other things, be explicit and make them "one value per sysfs file" or
>> >> >use debugfs for debugging things that no one relies on.
>> >> so you suggest braking the summery into files, e.g. one for cpus, one for ram and etcetera?
>> >
>> >Again, who uses this information and what is it used for?
>> >
>> >thanks,
>> >
>> >greg k-h
>> 
>> both user who uses the product and the development team.
>> it is used to provide a summery of the system. for example, which devices are used
>> by the hypervisor.
>
>That's a very odd way to display this as a free-flowing, impossible to
>parse, file.  Please use something that will be able to be maintained
>over time.
>
>thanks,
>
>greg k-h

your suggestion to brake the configuration seems sound, we will implement that and
provide a new patch

Thanks for the reviews.

Eial

Re: [PATCH v2] drivers/virt/vSMP: new driver

[Greg Kroah-Hartman]

On Thu, Aug 25, 2022 at 12:38:46PM +0000, Czerwacki, Eial wrote:
> >On Thu, Aug 25, 2022 at 12:02:12PM +0000, Czerwacki, Eial wrote:
> >> >On Thu, Aug 25, 2022 at 10:41:28AM +0000, Czerwacki, Eial wrote:
> >> >> >On Thu, Aug 25, 2022 at 10:16:59AM +0000, Czerwacki, Eial wrote:
> >> >> >> >> >And why is your version file a binary file?  It should just be a small
> >> >> >> >> >text string, right?
> >> >> >> >> not so small, it can reach up to 512kb.
> >> >> >> >
> >> >> >> >That was not obvious at all.  Please document this.
> >> >> >> where should the document be?
> >> >> >> in the code as a comment or in another file?
> >> >> >
> >> >> >In the Documentation/ABI/ file that describes this file.
> >> >> ok, will place it there
> >> >> 
> >> >> >
> >> >> >> >And how in the world is a "version" that big?  What exactly does this
> >> >> >> >contain?
> >> >> >> it 's size depends on the number of resources it uses.
> >> >> >> here is an example:
> >> >> >> :~> cat /sys/hypervisor/vsmp/version  
> >> >> >> SAP vSMP Foundation: 10.6.2862.0 (Aug 22 2022 15:21:02)
> >> >> >> System configuration:
> >> >> >>    Boards:      2
> >> >> >>       1 x Proc. + I/O + Memory
> >> >> >>       1 x NVM devices (Amazon.com Amazon EC2 NVMe Instance Storage)
> >> >> >>    Processors:  1, Cores: 2, Threads: 4
> >> >> >>        Intel(R) Xeon(R) Platinum 8124M CPU @ 3.00GHz Stepping 04
> >> >> >>    Memory (MB): 30976 (of 103192), Cache: 7527, Private: 64689
> >> >> >>       1 x  6400MB    [ 7825/ 321/ 1104]      
> >> >> >>       1 x 24576MB    [95367/7206/63585]       00:1f.0#1
> >> >> >>    Boot device: [HDD] NVMe: Amazon Elastic Block Store        
> >> >> >> Supported until: Aug 22 2024
> >> >> >
> >> >> >That is crazy, and is not a version.  It's a "configuration".
> >> >> it is called version for history reasons...
> >> >
> >> >There is no "history" here, you can create whatever sane interface you
> >> >want right now, there is no backwards compatible issues involved at all.
> >> you are correct, however, it depends on how much change the hypervisor code requires
> >> if any (latter is preferable)
> >
> >I do not understand, again, what tool consumes this today?
> there are monitoring utils that parses it.

What exact tools are you referring to, and why can you not change them?
Where is the source for them?

thanks,

greg k-h

Re: [PATCH v2] drivers/virt/vSMP: new driver

[Czerwacki, Eial]

>On Thu, Aug 25, 2022 at 12:38:46PM +0000, Czerwacki, Eial wrote:
>> >On Thu, Aug 25, 2022 at 12:02:12PM +0000, Czerwacki, Eial wrote:
>> >> >On Thu, Aug 25, 2022 at 10:41:28AM +0000, Czerwacki, Eial wrote:
>> >> >> >On Thu, Aug 25, 2022 at 10:16:59AM +0000, Czerwacki, Eial wrote:
>> >> >> >> >> >And why is your version file a binary file?  It should just be a small
>> >> >> >> >> >text string, right?
>> >> >> >> >> not so small, it can reach up to 512kb.
>> >> >> >> >
>> >> >> >> >That was not obvious at all.  Please document this.
>> >> >> >> where should the document be?
>> >> >> >> in the code as a comment or in another file?
>> >> >> >
>> >> >> >In the Documentation/ABI/ file that describes this file.
>> >> >> ok, will place it there
>> >> >> 
>> >> >> >
>> >> >> >> >And how in the world is a "version" that big?  What exactly does this
>> >> >> >> >contain?
>> >> >> >> it 's size depends on the number of resources it uses.
>> >> >> >> here is an example:
>> >> >> >> :~> cat /sys/hypervisor/vsmp/version  
>> >> >> >> SAP vSMP Foundation: 10.6.2862.0 (Aug 22 2022 15:21:02)
>> >> >> >> System configuration:
>> >> >> >>    Boards:      2
>> >> >> >>       1 x Proc. + I/O + Memory
>> >> >> >>       1 x NVM devices (Amazon.com Amazon EC2 NVMe Instance Storage)
>> >> >> >>    Processors:  1, Cores: 2, Threads: 4
>> >> >> >>        Intel(R) Xeon(R) Platinum 8124M CPU @ 3.00GHz Stepping 04
>> >> >> >>    Memory (MB): 30976 (of 103192), Cache: 7527, Private: 64689
>> >> >> >>       1 x  6400MB    [ 7825/ 321/ 1104]      
>> >> >> >>       1 x 24576MB    [95367/7206/63585]       00:1f.0#1
>> >> >> >>    Boot device: [HDD] NVMe: Amazon Elastic Block Store        
>> >> >> >> Supported until: Aug 22 2024
>> >> >> >
>> >> >> >That is crazy, and is not a version.  It's a "configuration".
>> >> >> it is called version for history reasons...
>> >> >
>> >> >There is no "history" here, you can create whatever sane interface you
>> >> >want right now, there is no backwards compatible issues involved at all.
>> >> you are correct, however, it depends on how much change the hypervisor code requires
>> >> if any (latter is preferable)
>> >
>> >I do not understand, again, what tool consumes this today?
>> there are monitoring utils that parses it.
>
>What exact tools are you referring to, and why can you not change them?
>Where is the source for them?
>
>thanks,
>
>greg k-h

SAP has monitoring tools which parse this configuration text.
for example exporters integrated into Grafana. these exporters we will change
to support the new format.

Eial

Re: [PATCH v2] drivers/virt/vSMP: new driver

[Dan Carpenter]

On Thu, Aug 25, 2022 at 06:24:02AM +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 api part which facilitates communication with
> the hypervisor and version which displays the hypervisor's version.
> 
> This patch s based on previous patches sent to the staging tree mailing
> lists
> 
> Signed-off-by: Eial Czerwacki <eial.czerwacki@sap.com>
> Acked-by: Leonid Arsh <leonid.arsh@sap.com>
> Acked-by: Oren Twaig <oren.twaig@sap.com>
> CC: SAP vSMP Linux Maintainer <linux.vsmp@sap.com>
> CC: Greg Kroah-Hartman <gregkh@linuxfoundation.org>
> CC: Arnd Bergmann <arnd@arndb.de>
> CC: Dan Carpenter <dan.carpenter@oracle.com>
> CC: Andra Paraschiv <andraprs@amazon.com>
> CC: Borislav Petkov <bp@suse.de>
> CC: Brijesh Singh <brijesh.singh@amd.com>
> CC: Eric Biggers <ebiggers@google.com>
> CC: Fei Li <fei1.li@intel.com>
> CC: Hans de Goede <hdegoede@redhat.com>
> CC: Jens Axboe <axboe@kernel.dk>
> CC: Mauro Carvalho Chehab <mchehab+huawei@kernel.org>
> 
> v1 -> v2:
> 	- fix -0 var init in add_sysfs_entries (pointed out by Dan Carpenter)

Please don't resend patches the same day.  Always wait at least a day
but probably for a new driver wait 3 days.  There are going to be a lot
of comments on this.

My comment didn't even warrant a resend at all, it just something that
made me chuckle.  It will take a while to review this properly.

regards,
dan carpenter

Re: [PATCH v2] drivers/virt/vSMP: new driver

[Czerwacki, Eial]

>On Thu, Aug 25, 2022 at 06:24:02AM +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 api part which facilitates communication with
>> the hypervisor and version which displays the hypervisor's version.
>> 
>> This patch s based on previous patches sent to the staging tree mailing
>> lists
>> 
>> Signed-off-by: Eial Czerwacki <eial.czerwacki@sap.com>
>> Acked-by: Leonid Arsh <leonid.arsh@sap.com>
>> Acked-by: Oren Twaig <oren.twaig@sap.com>
>> CC: SAP vSMP Linux Maintainer <linux.vsmp@sap.com>
>> CC: Greg Kroah-Hartman <gregkh@linuxfoundation.org>
>> CC: Arnd Bergmann <arnd@arndb.de>
>> CC: Dan Carpenter <dan.carpenter@oracle.com>
>> CC: Andra Paraschiv <andraprs@amazon.com>
>> CC: Borislav Petkov <bp@suse.de>
>> CC: Brijesh Singh <brijesh.singh@amd.com>
>> CC: Eric Biggers <ebiggers@google.com>
>> CC: Fei Li <fei1.li@intel.com>
>> CC: Hans de Goede <hdegoede@redhat.com>
>> CC: Jens Axboe <axboe@kernel.dk>
>> CC: Mauro Carvalho Chehab <mchehab+huawei@kernel.org>
>> 
>> v1 -> v2:
>>        - fix -0 var init in add_sysfs_entries (pointed out by Dan Carpenter)
>
>Please don't resend patches the same day.  Always wait at least a day
>but probably for a new driver wait 3 days.  There are going to be a lot
>of comments on this.
>
>My comment didn't even warrant a resend at all, it just something that
>made me chuckle.  It will take a while to review this properly.
>
>regards,
>dan carpenter
>

I did not know that, sorry

Re: [PATCH v2] drivers/virt/vSMP: new driver

[Borislav Petkov]

On Thu, Aug 25, 2022 at 10:48:44AM +0000, Czerwacki, Eial wrote:
> I did not know that, sorry

This might have some valuable information while waiting:

https://kernel.org/doc/html/latest/process/index.html

HTH.

-- 
Regards/Gruss,
    Boris.

SUSE Software Solutions Germany GmbH
GF: Ivo Totev, Andrew Myers, Andrew McDonald, Martje Boudien Moerman
(HRB 36809, AG Nürnberg)

Re: [PATCH v2] drivers/virt/vSMP: new driver

[Czerwacki, Eial]

>
>
>From: Borislav Petkov <bp@suse.de>
>Sent: Thursday, August 25, 2022 13:59
>To: Czerwacki, Eial <eial.czerwacki@sap.com>
>Cc: Dan Carpenter <dan.carpenter@oracle.com>; linux-kernel@vger.kernel.org <linux-kernel@vger.kernel.org>; Arsh, Leonid <leonid.arsh@sap.com>; Twaig, Oren <oren.twaig@sap.com>; SAP vSMP Linux Maintainer <linux.vsmp@sap.com>; Greg Kroah-Hartman <gregkh@linuxfoundation.org>; Arnd Bergmann <arnd@arndb.de>; Andra Paraschiv <andraprs@amazon.com>; Brijesh Singh <brijesh.singh@amd.com>; Eric Biggers <ebiggers@google.com>; Fei Li <fei1.li@intel.com>; Hans de Goede <hdegoede@redhat.com>; Jens Axboe <axboe@kernel.dk>; Mauro Carvalho Chehab <mchehab+huawei@kernel.org>
>Subject: Re: [PATCH v2] drivers/virt/vSMP: new driver 
> 
>On Thu, Aug 25, 2022 at 10:48:44AM +0000, Czerwacki, Eial wrote:
>> I did not know that, sorry
>
>This might have some valuable information while waiting:
>
>https://eur03.safelinks.protection.outlook.com/?url=https%3A%2F%2Fkernel.org%2Fdoc%2Fhtml%2Flatest%2Fprocess%2Findex.html&amp;data=05%7C01%7Ceial.czerwacki%40sap.com%7C94e3d24f919341c5273508da8688f804%7C42f7676cf455423c82f6dc2d99791af7%7C0%7C0%7C637970220076350755%7CUnknown%7CTWFpbGZsb3d8eyJWIjoiMC4wLjAwMDAiLCJQIjoiV2luMzIiLCJBTiI6Ik1haWwiLCJXVCI6Mn0%3D%7C3000%7C%7C%7C&amp;sdata=B6CapwsmFNygl%2Bm6ffT4QHV%2BBAa%2BKCYA4vV8oz0oUSE%3D&amp;reserved=0
>
>HTH.
>
>-- 
>Regards/Gruss,
>    Boris.
>
>SUSE Software Solutions Germany GmbH
>GF: Ivo Totev, Andrew Myers, Andrew McDonald, Martje Boudien Moerman
>(HRB 36809, AG Nürnberg)

thanks, will go over them

Re: [PATCH v2] drivers/virt/vSMP: new driver

[Dan Carpenter]

On Thu, Aug 25, 2022 at 06:24:02AM +0000, Czerwacki, Eial wrote:
> +obj-$(CONFIG_VSMP) = vsmp.o
> +vsmp-y := vsmp_main.o api/api.o version/version.o
> diff --git a/drivers/virt/vsmp/api/api.c b/drivers/virt/vsmp/api/api.c
> new file mode 100644
> index 000000000000..6e40935907bc
> --- /dev/null
> +++ b/drivers/virt/vsmp/api/api.c
> @@ -0,0 +1,249 @@
> +// SPDX-License-Identifier: GPL-2.0
> +
> +/*
> + * vSMP driver api
> + * (C) Copyright 2022 SAP SE
> + */
> +
> +#include "api.h"
> +
> +static void __iomem *cfg_addr;
> +static struct kobject *vsmp_sysfs_kobj;
> +static struct pci_dev *vsmp_dev_obj;

Try to avoid globals as much as possible.

> +
> +/* R/W ops handlers */
> +
> +/*
> + * Init a vsmp firmware operation object
> + */
> +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;

The ->buff_offset is the number of empty bytes.  It's badly named,
because it's not an offset at all.

> +
> +	op->buff = kzalloc(op->hwi_block_size, GFP_KERNEL);
> +	if (!op->buff)
> +		return -ENOMEM;
> +
> +	vsmp_reset_op(op);

This is a no-op.  op->buff_offset is already set and ->buf is already
zero.  Delete.

> +
> +	return 0;
> +}
> +
> +/*
> + * Release an vsmp firmware operation object
> + */
> +void vsmp_release_op(struct fw_ops *op)
> +{
> +	if (!op) {
> +		WARN_ON(!op);

	if (WARN_ON(!op))
		return;

> +		return;
> +	}
> +
> +	if (!op->buff) {
> +		WARN_ON(!op->buff);
> +		return;
> +	}
> +
> +	kfree(op->buff);
> +	memset(op, 0, sizeof(*op));
> +}
> +
> +/*
> + * Reset a vsmp firmware operation object
> + */
> +void vsmp_reset_op(struct fw_ops *op)
> +{
> +	memset(op->buff, 0, op->hwi_block_size);
> +	op->buff_offset = op->hwi_block_size;
> +}
> +
> +/* Regs/Buffs R/W handlers */
> +
> +/*
> + * Read a value from a specific register in the vSMP's device config space
> + */
> +u64 vsmp_read_reg_from_cfg(u64 reg, enum reg_size_type type)
> +{
> +	u64 ret_val;
> +
> +	switch (type) {
> +	case VSMP_CTL_REG_SIZE_8BIT:
> +		ret_val = readb(cfg_addr + reg);
> +		break;
> +
> +	case VSMP_CTL_REG_SIZE_16BIT:
> +		ret_val = readw(cfg_addr + reg);
> +		break;
> +
> +	case VSMP_CTL_REG_SIZE_32BIT:
> +		ret_val = readl(cfg_addr + reg);
> +		break;
> +
> +	case VSMP_CTL_REG_SIZE_64BIT:
> +		ret_val = readq(cfg_addr + reg);
> +		break;
> +
> +	default:
> +		dev_err(get_dev(), "Unsupported reg size type %d.\n", type);
> +		ret_val = (u64) -EINVAL;

This can never happen and there are two places which check for negative
returns but both are wrong.  Delete the dead code or make it simple.

		dev_err(get_dev(), "Unsupported reg size type %d.\n", type);
		ret_val = 0;

Then delete all the error handling in the callers.

> +	}
> +
> +	dev_dbg(get_dev(), "%s: read 0x%llx from reg 0x%llx of %d bits\n",
> +		__func__, ret_val, reg, (type + 1) * 8);

This function only exists because we want this debug statement.  I have
a strong bias towards deleting debug code and calling readl() directly.
In other words, delete this whole function.  Or keep it...  But at least
consider deleting it.

> +	return ret_val;
> +}
> +
> +/*
> + * Read a buffer from the bar byte by byte for halt on
> + * null termination.
> + * Expected buffs are strings.
> + */
> +static ssize_t read_buff_from_bar_in_bytes(char *out, u8 __iomem *buff, ssize_t len)
> +{
> +	u32 i;
> +
> +	for (i = 0; i < len; i++) {
> +		out[i] = ioread8(&buff[i]);
> +		if (!out[i])
> +			break;
> +	}
> +
> +	return i;
> +}
> +
> +/*
> + * Read a buffer from a specific offset in a specific bar,
> + * maxed to a predefined len size-wise from the vSMP device
> + */
> +int vsmp_read_buff_from_bar(u8 bar, u32 offset, char *out, ssize_t len,

The type mismatches bother me.  In some of the callers "bar" and "offset"
are u64 reg_vals but here they are a u8 and a u32.

> +			    bool halt_on_null)
> +{
> +	u8 __iomem *buff;
> +	u64 bar_start = pci_resource_start(vsmp_dev_obj, bar);
> +	u32 bar_len = pci_resource_len(vsmp_dev_obj, bar);
> +	ssize_t actual_len = len;
> +
> +	/* incase of overflow, warn and use max len possible */
> +	if ((offset + len) > bar_len) {
> +		WARN_ON((offset + len) > actual_len);
> +		actual_len = bar_len - offset;
> +		dev_dbg(get_dev(), "%lu overflows bar len, using %ld len instead\n",
> +			len, actual_len);
> +	}
> +
> +	buff = ioremap(bar_start + offset, actual_len);
> +	if (!buff)
> +		return -ENOMEM;
> +
> +	if (halt_on_null)
> +		read_buff_from_bar_in_bytes(out, buff, len);
> +	else
> +		memcpy_fromio(out, buff, len);

These are buffer overflows.  s/len/actual_len/

> +
> +	iounmap(buff);
> +
> +	return 0;
> +}
> +
> +/*
> + * Generic function to read from the bar
> + */
> +ssize_t vsmp_generic_buff_read(struct fw_ops *op, u8 bar, u64 reg,
> +			       char *buf, loff_t off, ssize_t count)
> +{
> +	ssize_t ret_val = 0;
> +
> +	if (op->buff_offset >= op->hwi_block_size) {	/* perform H/W op */

Hopefully op->buff_offset can never be > op->hwi_block_size.  (I worry
that because "op" is a global maybe race conditions mean this is
possible.)  Remeber that op->buff_offset means "bytes_available", it is
not an offset.  So if all the bytes are available then...

> +		vsmp_reset_op(op);

Delete everything, and set op->buff_offset == op->hwi_block_size.

> +
> +		ret_val = vsmp_read_buff_from_bar(bar, reg, op->buff, op->hwi_block_size, false);

Read some data.

> +		if (ret_val) {
> +			dev_err(get_dev(), "%s operation failed\n",
> +				(op->action == FW_READ) ? "read" : "write");
> +		}
> +		op->buff_offset = 0;

No bytes available.

> +	}

Or else read what we have.

> +
> +	if (ret_val)
> +		return ret_val;
> +
> +	return memory_read_from_buffer(buf, count, &op->buff_offset, op->buff, op->hwi_block_size);
                                                   ^^^^^^^^^^^^^^^^
Here offset is used as an offset.  If it's not zero then this is read
nonsense data.


> +}
> +
> +/* sysfs handlers */
> +
> +/*
> + * Register the vSMP sysfs object for user space interaction
> + */
> +int vsmp_register_sysfs_group(const struct bin_attribute *bin_attr)
> +{
> +	int error = -EINVAL;
> +
> +	if (vsmp_sysfs_kobj && bin_attr) {

Always do error handling.  Never do success handling.

	if (!vsmp_sysfs_kobj || !bin_attr)
		return -EINVAL;

	return sysfs_create_bin_file(vsmp_sysfs_kobj, bin_attr);


> +		error = sysfs_create_bin_file(vsmp_sysfs_kobj, bin_attr);
> +		if (error)
> +			dev_err(get_dev(), "Failed to register sysfs entry (%d)\n", error);
> +	}
> +
> +	return error;
> +}
> +
> +/*
> + * Deregister the vSMP sysfs object for user space interaction
> + */
> +void vsmp_deregister_sysfs_group(const struct bin_attribute *bin_attr)
> +{
> +	if (vsmp_sysfs_kobj && bin_attr)
> +		sysfs_remove_bin_file(vsmp_sysfs_kobj, bin_attr);
> +}
> +
> +/* Generic functions */
> +
> +/*
> + * Open the cfg address space of the vSDP device
> + */
> +int open_cfg_addr(struct pci_dev *pdev)
> +{
> +	u64 cfg_start;
> +	u32 cfg_len;
> +
> +	vsmp_dev_obj = pdev;
> +	cfg_start = pci_resource_start(vsmp_dev_obj, 0);
> +	cfg_len = pci_resource_len(vsmp_dev_obj, 0);
> +
> +	dev_dbg(get_dev(), "Mapping bar 0: [0x%llx,0x%llx]\n",
> +		cfg_start, cfg_start + cfg_len);
> +
> +	cfg_addr = ioremap(cfg_start, cfg_len);
> +	if (!cfg_addr) {
> +		dev_err(get_dev(), "Failed to map vSMP pci controller, exiting.\n");
> +		return -ENODEV;
> +	}
> +
> +	return 0;
> +}
> +
> +int init_sysfs(void)
> +{
> +	vsmp_sysfs_kobj = kobject_create_and_add("vsmp", hypervisor_kobj);
> +	if (!vsmp_sysfs_kobj) {
> +		dev_err(get_dev(), "Failed to create vSMP sysfs entry, exiting.\n");
> +		return -EINVAL;
> +	}
> +
> +	return 0;
> +}
> +
> +void cleanup(void)
> +{
> +	iounmap(cfg_addr);
> +	kobject_put(vsmp_sysfs_kobj);
> +}
> +
> +const struct device *get_dev(void)
> +{
> +	return &vsmp_dev_obj->dev;
> +}
> diff --git a/drivers/virt/vsmp/api/api.h b/drivers/virt/vsmp/api/api.h
> new file mode 100644
> index 000000000000..6142e947979f
> --- /dev/null
> +++ b/drivers/virt/vsmp/api/api.h
> @@ -0,0 +1,69 @@
> +/* SPDX-License-Identifier: GPL-2.0 */
> +
> +/*
> + * vSMP driver api header
> + * (C) Copyright 2022 SAP SE
> + */
> +
> +#ifndef VSMP_API_H
> +#define VSMP_API_H
> +
> +#include <linux/pci.h>
> +
> +// R/W ops handlers
> +#define vsmp_read_reg32_from_cfg(_reg_) \
> +	((u32) vsmp_read_reg_from_cfg((_reg_), VSMP_CTL_REG_SIZE_32BIT))
> +
> +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;
> +};
> +
> +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);
> +void vsmp_reset_op(struct fw_ops *op);
> +
> +#define FILE_PREM 0444

Spelling mistake.  Delete the define.  Avoid pointless indirection.

> +
> +/* Regs/Buffs R/W handlers */
> +#define vsmp_read_reg32_from_cfg(_reg_) \
> +	((u32) vsmp_read_reg_from_cfg((_reg_), VSMP_CTL_REG_SIZE_32BIT))

Delete this duplicate define.

> +
> +u64 vsmp_read_reg_from_cfg(u64 reg, enum reg_size_type type);
> +ssize_t vsmp_generic_buff_read(struct fw_ops *op, u8 bar, u64 reg,
> +			       char *buf, loff_t off, ssize_t count);
> +int vsmp_read_buff_from_bar(u8 bar, u32 offset, char *out, ssize_t len,
> +			    bool halt_on_null);
> +
> +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 vsmp_register_sysfs_group(const struct bin_attribute *bin_attr);
> +void vsmp_deregister_sysfs_group(const struct bin_attribute *bin_attr);
> +
> +int open_cfg_addr(struct pci_dev *pdev);
> +int init_sysfs(void);
> +void cleanup(void);
> +const struct device *get_dev(void);
> +#endif /* VSMP_API_H */
> diff --git a/drivers/virt/vsmp/include/registers.h b/drivers/virt/vsmp/include/registers.h
> new file mode 100644
> index 000000000000..b6458d25e3b7
> --- /dev/null
> +++ b/drivers/virt/vsmp/include/registers.h
> @@ -0,0 +1,12 @@
> +/* SPDX-License-Identifier: GPL-2.0 */
> +/*
> + * vSMP driver registers
> + * (C) Copyright 2022 SAP SE
> + */
> +
> +#ifndef VSMP_REGSITERS_H
> +#define VSMP_REGSITERS_H
> +
> +#define VSMP_VERSION_REG 0x0c
> +
> +#endif /* VSMP_REGSITERS_H */
> diff --git a/drivers/virt/vsmp/version/version.c b/drivers/virt/vsmp/version/version.c
> new file mode 100644
> index 000000000000..d8ad771daf28
> --- /dev/null
> +++ b/drivers/virt/vsmp/version/version.c
> @@ -0,0 +1,118 @@
> +// SPDX-License-Identifier: GPL-2.0
> +
> +/*
> + * vSMP driver version module
> + * (C) Copyright 2022 SAP SE
> + */
> +
> +#include <linux/slab.h>
> +#include <linux/kobject.h>
> +
> +#include "../api/api.h"
> +#include "../include/registers.h"
> +
> +/*
> + * This is the maximal possible length of the version which is a text string
> + * the real len is usually much smaller, thus the driver uses this once to read
> + * the version string and record it's actual len.
> + * From that point and on, the actual len will be used in each call.
> + */
> +#define VERSION_MAX_LEN (1 << 19)
> +
> +static struct fw_ops op;

Hate hate hate this global.

> +
> +static ssize_t version_read(struct file *filp, struct kobject *kobj,
> +			    struct bin_attribute *bin_attr,
> +			    char *buf, loff_t off, size_t count)
> +{
> +	u64 reg_val = vsmp_read_reg32_from_cfg(VSMP_VERSION_REG);
> +	ssize_t ret_val;
> +
> +	if (reg_val < 0) {

Delete impossible error handling.

> +		dev_err(get_dev(), "Failed to value of reg 0x%x\n", VSMP_VERSION_REG);
> +		return 0;
> +	}
> +
> +	ret_val = vsmp_generic_buff_read(&op, 0, reg_val, buf, off, count);
> +	if (ret_val < 0) {
> +		dev_err(get_dev(), "Failed to read version (%ld)\n", ret_val);
> +		return 0;

Return the error code.

> +	}
> +
> +	buf[ret_val++] = '\n';

This is a buffer overflow.

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

Just make this an int.  kzalloc() cannot allocate anywhere close to
INT_MAX.

> +{
> +	ssize_t len = 0;
> +	u64 reg_val = vsmp_read_reg32_from_cfg(VSMP_VERSION_REG);
> +	char *version_str = kzalloc(VERSION_MAX_LEN, GFP_KERNEL);

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

> +
> +	if (!version_str)
> +		return len;

Return -ENOMEM instead of zero.

> +
> +	if (vsmp_read_reg32_from_cfg(VSMP_VERSION_REG) < 0) {

Delete this impossible error handling.  Delete this block.  We already
saved VSMP_VERSION_REG in the declaration block.

> +		kfree(version_str);
> +		dev_err(get_dev(), "Failed to read value of reg 0x%x\n", VSMP_VERSION_REG);
> +		return len;
> +	}
> +
> +	memset(version_str, 0, VERSION_MAX_LEN);

kzalloc() already zeroes your memory.

> +	if (vsmp_read_buff_from_bar(0, reg_val, version_str, VERSION_MAX_LEN, true)) {
> +		kfree(version_str);
> +		dev_err(get_dev(), "Failed to read buffer from bar\n");
> +		return len;
> +	}
> +
> +	len = strlen(version_str);

This is a read overflow because there is no guarantee that
vsmp_read_buff_from_bar() returns a NULL terminated string.

> +	kfree(version_str);
> +
> +	return len;
> +}
> +
> +/*
> + * Register the version sysfs entry
> + */
> +int sysfs_register_version_cb(void)
> +{
> +	ssize_t len = get_version_len();
> +	int ret_val;
> +
> +	if (!len) {
> +		dev_err(get_dev(), "Failed to init vSMP version module\n");
> +		return -EINVAL;
> +	}
> +	version_raw_attr.size = len;
> +
> +	if (vsmp_init_op(&op, version_raw_attr.size, FW_READ)) {

get_version_len() does not count the NUL terminator on the end of the
string so this does not allocate enough memory.

> +		dev_err(get_dev(), "Failed to init vSMP version op\n");
> +		return -ENODEV;

Preserve the error code.

> +	}
> +
> +	ret_val = vsmp_register_sysfs_group(&version_raw_attr);
> +	if (ret_val) {
> +		dev_err(get_dev(), "Failed to init vSMP version support\n");
> +		vsmp_release_op(&op);

Better to return here.  (Avoid mixing error path and success path).

> +	}
> +
> +	return ret_val;

return 0;

> +}
> +
> +/*
> + * Deregister the version sysfs entry
> + */
> +void sysfs_deregister_version_cb(void)
> +{
> +	vsmp_deregister_sysfs_group(&version_raw_attr);
> +	vsmp_release_op(&op);
> +}
> diff --git a/drivers/virt/vsmp/version/version.h b/drivers/virt/vsmp/version/version.h
> new file mode 100644
> index 000000000000..c4430b3065e4
> --- /dev/null
> +++ b/drivers/virt/vsmp/version/version.h
> @@ -0,0 +1,14 @@
> +/* SPDX-License-Identifier: GPL-2.0 */
> +
> +/*
> + * vSMP driver version module header
> + * (C) Copyright 2022 SAP SE
> + */
> +
> +#ifndef VSMP_VERSION_COMMON_H
> +#define VSMP_VERSION_COMMON_H
> +
> +int sysfs_register_version_cb(void);
> +void sysfs_deregister_version_cb(void);
> +
> +#endif /* VSMP_VERSION_COMMON_H */
> diff --git a/drivers/virt/vsmp/vsmp_main.c b/drivers/virt/vsmp/vsmp_main.c
> new file mode 100644
> index 000000000000..95704bc7a32f
> --- /dev/null
> +++ b/drivers/virt/vsmp/vsmp_main.c
> @@ -0,0 +1,110 @@
> +// SPDX-License-Identifier: GPL-2.0
> +
> +/*
> + * vSMP driver main
> + * (C) Copyright 2022 SAP SE
> + */
> +
> +#include <linux/module.h>
> +
> +#include "api/api.h"
> +#include "version/version.h"
> +
> +/* modules info */
> +#define DEVICE_NAME "vSMP"
> +#define DRIVER_LICENSE "GPL v2"
> +#define DRIVER_AUTHOR "Eial Czerwacki <eial.czerwacki@sap.com>"
> +#define DRIVER_DESC "vSMP hypervisor driver"
> +#define DRIVER_VERSION "0.1"
> +
> +#define PCI_DEVICE_ID_SAP_FLX_VSMP_CTL 0x1011
> +
> +MODULE_LICENSE(DRIVER_LICENSE);
> +MODULE_AUTHOR(DRIVER_AUTHOR);
> +MODULE_DESCRIPTION(DRIVER_DESC);
> +MODULE_VERSION(DRIVER_VERSION);
> +
> +/* Sysfs handlers */
> +#define create_entry(_label_) \
> +	{ \
> +		.reg_cb = sysfs_register_ ## _label_ ## _cb, \
> +		.dereg_cb = sysfs_deregister_ ## _label_ ## _cb, \
> +	}
> +
> +static struct sysfs_entry_cbs cbs_arr[] = {
> +	create_entry(version),
> +};
> +
> +static const struct pci_device_id vsmp_pci_table[] = {
> +	{ PCI_VDEVICE(SCALEMP, PCI_DEVICE_ID_SAP_FLX_VSMP_CTL), 0, },
> +	{ 0, },			/* terminate list */
> +};
> +
> +/*
> + * Init all submodules's sysfs entries
> + */
> +static int add_sysfs_entries(void)
> +{
> +	int ret_val = 0, i;
> +
> +	for (i = 0; (i < ARRAY_SIZE(cbs_arr) && !ret_val); i++) {
> +		ret_val = cbs_arr[i].reg_cb();
> +		if (ret_val) {
> +			dev_err(get_dev(), "Failed to init sysfs entry %d (%d).\n",
> +				i, ret_val);

Clean up on error.

> +		}
> +	}
> +
> +	return ret_val;
> +}

static int add_sysfs_entries(void)
{
	int ret_val, i;

	for (i = 0; (i < ARRAY_SIZE(cbs_arr) && !ret_val); i++) {
		ret_val = cbs_arr[i].reg_cb();
		if (ret_val) {
			dev_err(get_dev(), "Failed to init sysfs entry %d (%d).\n",
				i, ret_val);
			goto cleanup;
		}
	}

	return 0;

cleanup:
	while (--i >= 0)
		cbs_arr[i].dereg_cb()
	return ret_val;
}

> +
> +/*
> + * Remove all submodules's sysfs entries
> + */
> +static void remove_sysfs_entries(void)
> +{
> +	int i;
> +
> +	for (i = 0; i < ARRAY_SIZE(cbs_arr); i++)
> +		cbs_arr[i].dereg_cb();
> +}
> +
> +static int vsmp_pci_probe(struct pci_dev *pci, const struct pci_device_id *id)
> +{
> +	int ret_val;
> +
> +	ret_val = open_cfg_addr(pci);
> +	if (ret_val) {
> +		dev_err(get_dev(), "Failed to open cfg addr\n");
> +		return ret_val;
> +	}
> +
> +	if (init_sysfs()) {
> +		dev_err(get_dev(), "Failed to create sysfs folder\n");
> +		return -ENODEV;

Add cleanup.  Preserve the error code.

> +	}
> +
> +	if (add_sysfs_entries()) {
> +		dev_err(get_dev(), "Failed to create sysfs entries\n");
> +		return -ENODEV;

Don't leak.  Preserve the error code.

regards,
dan carpenter

> +	}
> +
> +	dev_info(get_dev(), "%s up and running\n", DRIVER_DESC);
> +
> +	return 0;
> +}
> +
> +static void vsmp_pci_remove(struct pci_dev *pci)
> +{
> +	remove_sysfs_entries();
> +	cleanup();
> +}
> +
> +static struct pci_driver vsmp_pci_driver = {
> +	.name		= DEVICE_NAME,
> +	.id_table	= vsmp_pci_table,
> +	.probe		= vsmp_pci_probe,
> +	.remove	= vsmp_pci_remove,
> +};
> +
> +module_pci_driver(vsmp_pci_driver);

Re: [PATCH v2] drivers/virt/vSMP: new driver

[Czerwacki, Eial]

Greetings Dan,

thanks for the comments below
>On Thu, Aug 25, 2022 at 06:24:02AM +0000, Czerwacki, Eial wrote:
>> +obj-$(CONFIG_VSMP) = vsmp.o
>> +vsmp-y := vsmp_main.o api/api.o version/version.o
>> diff --git a/drivers/virt/vsmp/api/api.c b/drivers/virt/vsmp/api/api.c
>> new file mode 100644
>> index 000000000000..6e40935907bc
>> --- /dev/null
>> +++ b/drivers/virt/vsmp/api/api.c
[...]

I'll take into account all of the rejects and will fix them
in the next iteration.

thanks,

Eial

Re: [PATCH v2] drivers/virt/vSMP: new driver

[Greg Kroah-Hartman]

On Thu, Aug 25, 2022 at 06:24:02AM +0000, Czerwacki, Eial wrote:
> --- /dev/null
> +++ b/drivers/virt/vsmp/Kconfig
> @@ -0,0 +1,11 @@
> +# SPDX-License-Identifier: GPL-2.0-only
> +config VSMP
> +	tristate "vSMP Guest Support"
> +	depends on SYS_HYPERVISOR && X86_64 && PCI
> +	help
> +	  Support for vSMP Guest Driver.
> +
> +	  This driver allows information gathering of data from the vSMP hypervisor when
> +	  running on top of a vSMP-based hypervisor.
> +
> +	  If unsure, say no.

In wanting to test this out, I tried it but this depends line is wrong,
you have to set SYS_HYPERVISOR, you can not depend on it otherwise your
code will never be selected :(

How did you test this?

thanks,

greg k-h

Re: [PATCH v2] drivers/virt/vSMP: new driver

[Greg Kroah-Hartman]

On Tue, Sep 06, 2022 at 03:50:37PM +0200, Greg Kroah-Hartman wrote:
> On Thu, Aug 25, 2022 at 06:24:02AM +0000, Czerwacki, Eial wrote:
> > --- /dev/null
> > +++ b/drivers/virt/vsmp/Kconfig
> > @@ -0,0 +1,11 @@
> > +# SPDX-License-Identifier: GPL-2.0-only
> > +config VSMP
> > +	tristate "vSMP Guest Support"
> > +	depends on SYS_HYPERVISOR && X86_64 && PCI
> > +	help
> > +	  Support for vSMP Guest Driver.
> > +
> > +	  This driver allows information gathering of data from the vSMP hypervisor when
> > +	  running on top of a vSMP-based hypervisor.
> > +
> > +	  If unsure, say no.
> 
> In wanting to test this out, I tried it but this depends line is wrong,
> you have to set SYS_HYPERVISOR, you can not depend on it otherwise your
> code will never be selected :(

Ok, based on the conversation happening on the staging list, I took a
look at the code here again and have deleted a ton of it and added a
framework for you to add some sysfs files in the hypervisor location,
but this is NOT where the device/board files go, that's a different
add-on patch on top of this.

Here's an updated patch, much smaller, and hopefully simpler to
understand and follow.  I didn't touch the Documentation/ABI/ entry, but
if you run this you should see 4 sysfs files, "version1-3" that just
print a single number, and "version_length" that does some i/o and gets
the length that you wanted to use in the past to show how to tie this
into the device-specific information (and not have any static
information.)

Feel free to build on this for your next submission.  And if you have
any questions about this, please let me know.

Note, I have only built this code, not tested it, for obvious reasons :)

thanks,

greg k-h

------------------

From: Greg Kroah-Hartman <gregkh@linuxfoundation.org>

vSMP driver framework and dummy sysfs files

Signed-off-by: Greg Kroah-Hartman <gregkh@linuxfoundation.org>

---
 Documentation/ABI/stable/sysfs-driver-vsmp |   5 +
 MAINTAINERS                                |   6 +
 drivers/virt/Kconfig                       |   2 +
 drivers/virt/Makefile                      |   2 +
 drivers/virt/vsmp/Kconfig                  |  12 +
 drivers/virt/vsmp/Makefile                 |   6 +
 drivers/virt/vsmp/vsmp.c                   | 294 +++++++++++++++++++++
 7 files changed, 327 insertions(+)
 create mode 100644 Documentation/ABI/stable/sysfs-driver-vsmp
 create mode 100644 drivers/virt/vsmp/Kconfig
 create mode 100644 drivers/virt/vsmp/Makefile
 create mode 100644 drivers/virt/vsmp/vsmp.c

diff --git a/Documentation/ABI/stable/sysfs-driver-vsmp b/Documentation/ABI/stable/sysfs-driver-vsmp
new file mode 100644
index 000000000000..18a0a62f40ed
--- /dev/null
+++ b/Documentation/ABI/stable/sysfs-driver-vsmp
@@ -0,0 +1,5 @@
+What:           /sys/hypervisor/vsmp/version
+Date:           Aug 2022
+Contact:        Eial Czerwacki <eial.czerwacki@sap.com>
+		linux-vsmp@sap.com
+Description:    Shows the full version of the vSMP hypervisor
diff --git a/MAINTAINERS b/MAINTAINERS
index d30f26e07cd3..ff5277853660 100644
--- a/MAINTAINERS
+++ b/MAINTAINERS
@@ -21789,6 +21789,12 @@ F:	lib/test_printf.c
 F:	lib/test_scanf.c
 F:	lib/vsprintf.c
 
+VSMP GUEST DRIVER
+M:	Eial Czerwacki <eial.czerwacki@sap.com>
+M:	linux-vsmp@sap.com
+S:	Maintained
+F:	drivers/virt/vsmp
+
 VT1211 HARDWARE MONITOR DRIVER
 M:	Juerg Haefliger <juergh@gmail.com>
 L:	linux-hwmon@vger.kernel.org
diff --git a/drivers/virt/Kconfig b/drivers/virt/Kconfig
index 87ef258cec64..9f283f476674 100644
--- a/drivers/virt/Kconfig
+++ b/drivers/virt/Kconfig
@@ -52,4 +52,6 @@ source "drivers/virt/coco/efi_secret/Kconfig"
 
 source "drivers/virt/coco/sev-guest/Kconfig"
 
+source "drivers/virt/vsmp/Kconfig"
+
 endif
diff --git a/drivers/virt/Makefile b/drivers/virt/Makefile
index 093674e05c40..159ba37cb471 100644
--- a/drivers/virt/Makefile
+++ b/drivers/virt/Makefile
@@ -11,3 +11,5 @@ obj-$(CONFIG_NITRO_ENCLAVES)	+= nitro_enclaves/
 obj-$(CONFIG_ACRN_HSM)		+= acrn/
 obj-$(CONFIG_EFI_SECRET)	+= coco/efi_secret/
 obj-$(CONFIG_SEV_GUEST)		+= coco/sev-guest/
+
+obj-$(CONFIG_VSMP)             += vsmp/
diff --git a/drivers/virt/vsmp/Kconfig b/drivers/virt/vsmp/Kconfig
new file mode 100644
index 000000000000..71eb06560a2a
--- /dev/null
+++ b/drivers/virt/vsmp/Kconfig
@@ -0,0 +1,12 @@
+# SPDX-License-Identifier: GPL-2.0-only
+config VSMP
+	tristate "vSMP Guest Support"
+	depends on X86_64 && PCI
+	select SYS_HYPERVISOR
+	help
+	  Support for vSMP Guest Driver.
+
+	  This driver allows information gathering of data from the vSMP hypervisor when
+	  running on top of a vSMP-based hypervisor.
+
+	  If unsure, say no.
diff --git a/drivers/virt/vsmp/Makefile b/drivers/virt/vsmp/Makefile
new file mode 100644
index 000000000000..28240ea3f170
--- /dev/null
+++ b/drivers/virt/vsmp/Makefile
@@ -0,0 +1,6 @@
+# SPDX-License-Identifier: GPL-2.0
+#
+# Makefile for vSMP Guest driver
+#
+
+obj-$(CONFIG_VSMP) = vsmp.o
diff --git a/drivers/virt/vsmp/vsmp.c b/drivers/virt/vsmp/vsmp.c
new file mode 100644
index 000000000000..2a3abf3c0b0b
--- /dev/null
+++ b/drivers/virt/vsmp/vsmp.c
@@ -0,0 +1,294 @@
+// SPDX-License-Identifier: GPL-2.0-only
+/*
+ * vSMP driver
+ *
+ * (C) Copyright 2022 SAP SE
+ * (C) Copyright 2022 Greg Kroah-Hartman <gregkh@linuxfoundation.org>
+ */
+#include <linux/module.h>
+#include <linux/slab.h>
+#include <linux/kobject.h>
+#include <linux/pci.h>
+#include <linux/sysfs.h>
+
+// R/W ops handlers
+#define vsmp_read_reg32_from_cfg(dev, _reg_) \
+	((u32) vsmp_read_reg_from_cfg(dev, (_reg_), VSMP_CTL_REG_SIZE_32BIT))
+
+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
+};
+
+struct vsmp_dev {
+	void __iomem *cfg_addr;
+	struct kobject kobj;
+	struct pci_dev *pdev;
+};
+
+#define VSMP_VERSION_REG 0x0c
+
+/*
+ * Read a value from a specific register in the vSMP's device config space
+ */
+static u64 vsmp_read_reg_from_cfg(struct vsmp_dev *vdev, u64 reg, enum reg_size_type type)
+{
+	u64 ret_val;
+
+	switch (type) {
+	case VSMP_CTL_REG_SIZE_8BIT:
+		ret_val = readb(vdev->cfg_addr + reg);
+		break;
+
+	case VSMP_CTL_REG_SIZE_16BIT:
+		ret_val = readw(vdev->cfg_addr + reg);
+		break;
+
+	case VSMP_CTL_REG_SIZE_32BIT:
+		ret_val = readl(vdev->cfg_addr + reg);
+		break;
+
+	case VSMP_CTL_REG_SIZE_64BIT:
+		ret_val = readq(vdev->cfg_addr + reg);
+		break;
+
+	default:
+		dev_err(&vdev->pdev->dev, "Unsupported reg size type %d.\n", type);
+		ret_val = (u64) -EINVAL;
+	}
+
+	dev_dbg(&vdev->pdev->dev, "%s: read 0x%llx from reg 0x%llx of %d bits\n",
+		__func__, ret_val, reg, (type + 1) * 8);
+	return ret_val;
+}
+
+/*
+ * Read a buffer from the bar byte by byte for halt on
+ * null termination.
+ * Expected buffs are strings.
+ */
+static ssize_t read_buff_from_bar_in_bytes(char *out, u8 __iomem *buff, ssize_t len)
+{
+	u32 i;
+
+	for (i = 0; i < len; i++) {
+		out[i] = ioread8(&buff[i]);
+		if (!out[i])
+			break;
+	}
+
+	return i;
+}
+
+/*
+ * Read a buffer from a specific offset in a specific bar,
+ * maxed to a predefined len size-wise from the vSMP device
+ */
+static int vsmp_read_buff_from_bar(struct vsmp_dev *vdev, u8 bar, u32 offset,
+				   char *out, ssize_t len, bool halt_on_null)
+{
+	u8 __iomem *buff;
+	u64 bar_start = pci_resource_start(vdev->pdev, bar);
+	u32 bar_len = pci_resource_len(vdev->pdev, bar);
+	ssize_t actual_len = len;
+
+	/* incase of overflow, warn and use max len possible */
+	if ((offset + len) > bar_len) {
+		WARN_ON((offset + len) > actual_len);
+		actual_len = bar_len - offset;
+		dev_dbg(&vdev->pdev->dev, "%lu overflows bar len, using %ld len instead\n", len, actual_len);
+	}
+
+	buff = ioremap(bar_start + offset, actual_len);
+	if (!buff)
+		return -ENOMEM;
+
+	if (halt_on_null)
+		read_buff_from_bar_in_bytes(out, buff, len);
+	else
+		memcpy_fromio(out, buff, len);
+
+	iounmap(buff);
+
+	return 0;
+}
+
+/*
+ * Generic function to read from the bar
+ */
+/*
+ * This is the maximal possible length of the version which is a text string
+ * the real len is usually much smaller, thus the driver uses this once to read
+ * the version string and record it's actual len.
+ * From that point and on, the actual len will be used in each call.
+ */
+#define VERSION_MAX_LEN (1 << 19)
+
+/*
+ * Retrieve str in order to determine the proper length.
+ * This is the best way to maintain backwards compatibility with all
+ * vSMP versions.
+ */
+static ssize_t get_version_len(struct vsmp_dev *vdev)
+{
+	ssize_t len = 0;
+	u64 reg_val = vsmp_read_reg32_from_cfg(vdev, VSMP_VERSION_REG);
+	char *version_str = kzalloc(VERSION_MAX_LEN, GFP_KERNEL);
+
+	if (!version_str)
+		return len;
+
+	if (vsmp_read_reg32_from_cfg(vdev, VSMP_VERSION_REG) < 0) {
+		kfree(version_str);
+		dev_err(&vdev->pdev->dev, "Failed to read value of reg 0x%x\n", VSMP_VERSION_REG);
+		return len;
+	}
+
+	memset(version_str, 0, VERSION_MAX_LEN);
+	if (vsmp_read_buff_from_bar(vdev, 0, reg_val, version_str, VERSION_MAX_LEN, true)) {
+		kfree(version_str);
+		dev_err(&vdev->pdev->dev, "Failed to read buffer from bar\n");
+		return len;
+	}
+
+	len = strlen(version_str);
+	kfree(version_str);
+
+	return len;
+}
+
+static ssize_t version1_show(struct kobject *kobj,
+			     struct kobj_attribute *attr, char *buf)
+{
+	return sysfs_emit(buf, "1\n");
+}
+
+static ssize_t version2_show(struct kobject *kobj,
+			     struct kobj_attribute *attr, char *buf)
+{
+	return sysfs_emit(buf, "2\n");
+}
+
+static ssize_t version3_show(struct kobject *kobj,
+			     struct kobj_attribute *attr, char *buf)
+{
+	return sysfs_emit(buf, "3\n");
+}
+
+static ssize_t version_len_show(struct kobject *kobj,
+				struct kobj_attribute *attr, char *buf)
+{
+	struct vsmp_dev *vdev = container_of(kobj, struct vsmp_dev, kobj);
+	int length;
+
+	length = (int)get_version_len(vdev);
+
+	return sysfs_emit(buf, "%d\n", length);
+}
+
+static struct kobj_attribute version1 = __ATTR_RO(version1);
+static struct kobj_attribute version2 = __ATTR_RO(version2);
+static struct kobj_attribute version3 = __ATTR_RO(version3);
+static struct kobj_attribute version_len = __ATTR_RO(version_len);
+
+static struct attribute *vsmp_attrs[] = {
+	&version1.attr,
+	&version2.attr,
+	&version3.attr,
+	&version_len.attr,
+};
+ATTRIBUTE_GROUPS(vsmp);
+
+static void vsmp_release(struct kobject *kobj)
+{
+	struct vsmp_dev *vdev = container_of(kobj, struct vsmp_dev, kobj);
+
+	kfree(vdev);
+}
+
+static ssize_t vsmp_show(struct kobject *kobj, struct attribute *attr, char *buf)
+{
+	struct kobj_attribute *kobj_attr;
+
+	kobj_attr = container_of(attr, struct kobj_attribute, attr);
+	return kobj_attr->show(kobj, kobj_attr, buf);
+}
+
+static const struct sysfs_ops vsmp_ops = {
+	.show		= vsmp_show,
+};
+
+static struct kobj_type vsmp_type = {
+	.release	= vsmp_release,
+	.sysfs_ops	= &vsmp_ops,
+	.default_groups	= vsmp_groups,
+};
+
+static int vsmp_pci_probe(struct pci_dev *pci, const struct pci_device_id *id)
+{
+	struct vsmp_dev *vdev;
+	u64 cfg_start;
+	u32 cfg_len;
+	int retval;
+
+	vdev = kzalloc(sizeof(*vdev), GFP_KERNEL);
+	if (!vdev)
+		return -ENOMEM;
+
+	vdev->pdev = pci;
+
+	/*
+	 * Open the cfg address space of the vSDP device
+	 */
+	cfg_start = pci_resource_start(pci, 0);
+	cfg_len = pci_resource_len(pci, 0);
+
+	dev_dbg(&pci->dev, "Mapping bar 0: [0x%llx,0x%llx]\n", cfg_start, cfg_start + cfg_len);
+
+	vdev->cfg_addr = ioremap(cfg_start, cfg_len);
+	if (!vdev->cfg_addr) {
+		dev_err(&pci->dev, "Failed to map vSMP pci controller, exiting.\n");
+		kfree(vdev);
+		return -ENODEV;
+	}
+
+	pci_set_drvdata(pci, vdev);
+
+	retval = kobject_init_and_add(&vdev->kobj, &vsmp_type, hypervisor_kobj, "vsmp");
+	if (retval) {
+		kobject_put(&vdev->kobj);
+		dev_err(&vdev->pdev->dev, "Failed to create vSMP sysfs entry, exiting.\n");
+		return -ENODEV;
+	}
+
+	return 0;
+}
+
+static void vsmp_pci_remove(struct pci_dev *pci)
+{
+	struct vsmp_dev *vdev = pci_get_drvdata(pci);
+
+	iounmap(vdev->cfg_addr);
+	kobject_del(&vdev->kobj);
+}
+
+#define PCI_DEVICE_ID_SAP_FLX_VSMP_CTL 0x1011
+static const struct pci_device_id vsmp_pci_table[] = {
+	{ PCI_VDEVICE(SCALEMP, PCI_DEVICE_ID_SAP_FLX_VSMP_CTL), 0, },
+	{ 0, },			/* terminate list */
+};
+
+static struct pci_driver vsmp_pci_driver = {
+	.name		= "vSMP",
+	.id_table	= vsmp_pci_table,
+	.probe		= vsmp_pci_probe,
+	.remove		= vsmp_pci_remove,
+};
+
+module_pci_driver(vsmp_pci_driver);
+
+MODULE_LICENSE("GPL v2");
+MODULE_AUTHOR("Eial Czerwacki <eial.czerwacki@sap.com>");
+MODULE_DESCRIPTION("vSMP hypervisor driver");
-- 
2.37.3

Re: [PATCH v2] drivers/virt/vSMP: new driver

[Czerwacki, Eial]

>On Tue, Sep 06, 2022 at 03:50:37PM +0200, Greg Kroah-Hartman wrote:
>> On Thu, Aug 25, 2022 at 06:24:02AM +0000, Czerwacki, Eial wrote:
>> > --- /dev/null
>> > +++ b/drivers/virt/vsmp/Kconfig
>> > @@ -0,0 +1,11 @@
>> > +# SPDX-License-Identifier: GPL-2.0-only
>> > +config VSMP
>> > +   tristate "vSMP Guest Support"
>> > +   depends on SYS_HYPERVISOR && X86_64 && PCI
>> > +   help
>> > +     Support for vSMP Guest Driver.
>> > +
>> > +     This driver allows information gathering of data from the vSMP hypervisor when
>> > +     running on top of a vSMP-based hypervisor.
>> > +
>> > +     If unsure, say no.
>> 
>> In wanting to test this out, I tried it but this depends line is wrong,
>> you have to set SYS_HYPERVISOR, you can not depend on it otherwise your
>> code will never be selected :(
>
>Ok, based on the conversation happening on the staging list, I took a
>look at the code here again and have deleted a ton of it and added a
>framework for you to add some sysfs files in the hypervisor location,
>but this is NOT where the device/board files go, that's a different
>add-on patch on top of this.
>
>Here's an updated patch, much smaller, and hopefully simpler to
>understand and follow.  I didn't touch the Documentation/ABI/ entry, but
>if you run this you should see 4 sysfs files, "version1-3" that just
>print a single number, and "version_length" that does some i/o and gets
>the length that you wanted to use in the past to show how to tie this
>into the device-specific information (and not have any static
>information.)
>
>Feel free to build on this for your next submission.  And if you have
>any questions about this, please let me know.
>
>Note, I have only built this code, not tested it, for obvious reasons :)
>
>thanks,
>
oh wow! thanks a lot!
I'll try it and of course will learn from it

Eial

Re: [PATCH v2] drivers/virt/vSMP: new driver

[Czerwacki, Eial]

>On Thu, Aug 25, 2022 at 06:24:02AM +0000, Czerwacki, Eial wrote:
>> --- /dev/null
>> +++ b/drivers/virt/vsmp/Kconfig
>> @@ -0,0 +1,11 @@
>> +# SPDX-License-Identifier: GPL-2.0-only
>> +config VSMP
>> +     tristate "vSMP Guest Support"
>> +     depends on SYS_HYPERVISOR && X86_64 && PCI
>> +     help
>> +       Support for vSMP Guest Driver.
>> +
>> +       This driver allows information gathering of data from the vSMP hypervisor when
>> +       running on top of a vSMP-based hypervisor.
>> +
>> +       If unsure, say no.
>
>In wanting to test this out, I tried it but this depends line is wrong,
>you have to set SYS_HYPERVISOR, you can not depend on it otherwise your
>code will never be selected :(
>
>How did you test this?
>
>thanks,
>
>greg k-h

as you pointed out another mail thread, I've made a mistake and developed the code  on the a kernel which isn't mainline, my apologies, this patch should be dropped until I generate a new patch which is based on mainline kernel.

Eial