[PATCH 0/3] platform/x86/amd/hsmp: ACPI, Non-ACPI code split and other misc changes

[PATCH 0/3] platform/x86/amd/hsmp: ACPI, Non-ACPI code split and other misc changes

[Suma Hegde]

Patch1:
The first patch is to address the concerns mentioned by Limonciello,
Mario i.e to address the issue of 
"Probing registers that don't match the same mailboxes randomly on
client or embedded parts might lead to unexpected behaviors".

Because of some limitations, instead of the methods suggested by Mario,
family check is added to differentiate server socs and client socs.
This check is only for backward compatibility, Going forward all
systems will have ACPI based probing only.


Patch2:
The second patch splits ACPI code into  separate file there by removing the
dependency of ACPI in Kconfig.
This is based on the suggestion from Ilpo Jarvinen.


Patch3:
The third patch makes HSMP as "default m" so that anyone who uses standard
distros will be able to use HSMP by dynamically loading it.


Suma Hegde (3):
  platform/x86/amd/hsmp: Check HSMP support on AMD family of processors
  platform/x86/amd/hsmp: Split the ACPI and non-ACPI code
  platform/x86/amd/hsmp: Make HSMP as default m

 MAINTAINERS                                |   2 +-
 drivers/platform/x86/amd/Kconfig           |  14 +-
 drivers/platform/x86/amd/Makefile          |   3 +-
 drivers/platform/x86/amd/hsmp/Kconfig      |  18 +
 drivers/platform/x86/amd/hsmp/Makefile     |   9 +
 drivers/platform/x86/amd/hsmp/hsmp-acpi.c  | 219 ++++++++++++
 drivers/platform/x86/amd/{ => hsmp}/hsmp.c | 378 +++++----------------
 drivers/platform/x86/amd/hsmp/hsmp.h       |  82 +++++
 8 files changed, 425 insertions(+), 300 deletions(-)
 create mode 100644 drivers/platform/x86/amd/hsmp/Kconfig
 create mode 100644 drivers/platform/x86/amd/hsmp/Makefile
 create mode 100644 drivers/platform/x86/amd/hsmp/hsmp-acpi.c
 rename drivers/platform/x86/amd/{ => hsmp}/hsmp.c (71%)
 create mode 100644 drivers/platform/x86/amd/hsmp/hsmp.h

-- 
2.25.1

[PATCH 1/3] platform/x86/amd/hsmp: Check HSMP support on AMD family of processors

[Suma Hegde]

HSMP interface is supported only on few x86 processors from AMD.
Accessing HSMP registers on rest of the platforms might cause
unexpected behaviour. So add a check.

Also unavailability of this interface on rest of the processors
is not an error. Hence, use pr_info() instead of the pr_err() to
log the message.

Signed-off-by: Suma Hegde <suma.hegde@amd.com>
Reviewed-by: Naveen Krishna Chatradhi <naveenkrishna.chatradhi@amd.com>
---
 drivers/platform/x86/amd/hsmp.c | 54 ++++++++++++++++++++++++++++-----
 1 file changed, 47 insertions(+), 7 deletions(-)

diff --git a/drivers/platform/x86/amd/hsmp.c b/drivers/platform/x86/amd/hsmp.c
index 1927be901108..807a5066dacc 100644
--- a/drivers/platform/x86/amd/hsmp.c
+++ b/drivers/platform/x86/amd/hsmp.c
@@ -907,16 +907,48 @@ static int hsmp_plat_dev_register(void)
 	return ret;
 }
 
+/*
+ * This check is only needed for backward compatibility of previous platforms.
+ * All new platforms are expected to support ACPI based probing.
+ */
+static bool is_hsmp_supported(void)
+{
+	bool ret = true;
+
+	if (boot_cpu_data.x86_vendor != X86_VENDOR_AMD)
+		return false;
+
+	switch (boot_cpu_data.x86) {
+	case 0x19:
+		switch (boot_cpu_data.x86_model) {
+		case 0x00 ... 0x1F:
+		case 0x30 ... 0x3F:
+		case 0x90 ... 0x9F:
+		case 0xA0 ... 0xAF:
+			break;
+		default:
+			ret = false;
+		}
+		break;
+	case 0x1A:
+		switch (boot_cpu_data.x86_model) {
+		case 0x00 ... 0x1F:
+			break;
+		default:
+			ret = false;
+		}
+		break;
+	default:
+		ret = false;
+	}
+
+	return ret;
+}
+
 static int __init hsmp_plt_init(void)
 {
 	int ret = -ENODEV;
 
-	if (boot_cpu_data.x86_vendor != X86_VENDOR_AMD || boot_cpu_data.x86 < 0x19) {
-		pr_err("HSMP is not supported on Family:%x model:%x\n",
-		       boot_cpu_data.x86, boot_cpu_data.x86_model);
-		return ret;
-	}
-
 	/*
 	 * amd_nb_num() returns number of SMN/DF interfaces present in the system
 	 * if we have N SMN/DF interfaces that ideally means N sockets
@@ -930,7 +962,15 @@ static int __init hsmp_plt_init(void)
 		return ret;
 
 	if (!plat_dev.is_acpi_device) {
-		ret = hsmp_plat_dev_register();
+		if (is_hsmp_supported()) {
+			/* Not ACPI device, but supports HSMP, register a plat_dev */
+			ret = hsmp_plat_dev_register();
+		} else {
+			/* Not ACPI, Does not support HSMP */
+			pr_info("HSMP is not supported on Family:%x model:%x\n",
+				boot_cpu_data.x86, boot_cpu_data.x86_model);
+			ret = -ENODEV;
+		}
 		if (ret)
 			platform_driver_unregister(&amd_hsmp_driver);
 	}
-- 
2.25.1

[PATCH 2/3] platform/x86/amd/hsmp: Split the ACPI and non-ACPI code

[Suma Hegde]

HSMP driver is supported on ACPI as well as non-ACPI based platforms.
Making it dependent on ACPI in Kconfig, causes it to be compiled only on
systems which have ACPI enabled.
Remove this dependency by seggregating acpi specific code into
hsmp-acpi.c file.
This ensures that HSMP can be compiled with or without
ACPI enabled.

Also create hsmp/ directory with Kconfig and Makefile
to keep hsmp related files.

Signed-off-by: Suma Hegde <suma.hegde@amd.com>
Reviewed-by: Naveen Krishna Chatradhi <naveenkrishna.chatradhi@amd.com>
---
 MAINTAINERS                                |   2 +-
 drivers/platform/x86/amd/Kconfig           |  14 +-
 drivers/platform/x86/amd/Makefile          |   3 +-
 drivers/platform/x86/amd/hsmp/Kconfig      |  17 ++
 drivers/platform/x86/amd/hsmp/Makefile     |   9 +
 drivers/platform/x86/amd/hsmp/hsmp-acpi.c  | 219 ++++++++++++++
 drivers/platform/x86/amd/{ => hsmp}/hsmp.c | 324 +++------------------
 drivers/platform/x86/amd/hsmp/hsmp.h       |  82 ++++++
 8 files changed, 377 insertions(+), 293 deletions(-)
 create mode 100644 drivers/platform/x86/amd/hsmp/Kconfig
 create mode 100644 drivers/platform/x86/amd/hsmp/Makefile
 create mode 100644 drivers/platform/x86/amd/hsmp/hsmp-acpi.c
 rename drivers/platform/x86/amd/{ => hsmp}/hsmp.c (72%)
 create mode 100644 drivers/platform/x86/amd/hsmp/hsmp.h

diff --git a/MAINTAINERS b/MAINTAINERS
index ebf03f5f0619..bdc390ddcc4b 100644
--- a/MAINTAINERS
+++ b/MAINTAINERS
@@ -999,7 +999,7 @@ S:	Maintained
 F:	Documentation/arch/x86/amd_hsmp.rst
 F:	arch/x86/include/asm/amd_hsmp.h
 F:	arch/x86/include/uapi/asm/amd_hsmp.h
-F:	drivers/platform/x86/amd/hsmp.c
+F:	drivers/platform/x86/amd/hsmp/
 
 AMD IOMMU (AMD-VI)
 M:	Joerg Roedel <joro@8bytes.org>
diff --git a/drivers/platform/x86/amd/Kconfig b/drivers/platform/x86/amd/Kconfig
index f88682d36447..c2f16ccebec4 100644
--- a/drivers/platform/x86/amd/Kconfig
+++ b/drivers/platform/x86/amd/Kconfig
@@ -5,19 +5,7 @@
 
 source "drivers/platform/x86/amd/pmf/Kconfig"
 source "drivers/platform/x86/amd/pmc/Kconfig"
-
-config AMD_HSMP
-	tristate "AMD HSMP Driver"
-	depends on AMD_NB && X86_64 && ACPI
-	help
-	  The driver provides a way for user space tools to monitor and manage
-	  system management functionality on EPYC server CPUs from AMD.
-
-	  Host System Management Port (HSMP) interface is a mailbox interface
-	  between the x86 core and the System Management Unit (SMU) firmware.
-
-	  If you choose to compile this driver as a module the module will be
-	  called amd_hsmp.
+source "drivers/platform/x86/amd/hsmp/Kconfig"
 
 config AMD_WBRF
 	bool "AMD Wifi RF Band mitigations (WBRF)"
diff --git a/drivers/platform/x86/amd/Makefile b/drivers/platform/x86/amd/Makefile
index dcec0a46f8af..f0b2fe81c685 100644
--- a/drivers/platform/x86/amd/Makefile
+++ b/drivers/platform/x86/amd/Makefile
@@ -5,7 +5,6 @@
 #
 
 obj-$(CONFIG_AMD_PMC)		+= pmc/
-amd_hsmp-y			:= hsmp.o
-obj-$(CONFIG_AMD_HSMP)		+= amd_hsmp.o
+obj-$(CONFIG_AMD_HSMP)		+= hsmp/
 obj-$(CONFIG_AMD_PMF)		+= pmf/
 obj-$(CONFIG_AMD_WBRF)		+= wbrf.o
diff --git a/drivers/platform/x86/amd/hsmp/Kconfig b/drivers/platform/x86/amd/hsmp/Kconfig
new file mode 100644
index 000000000000..26096f901c22
--- /dev/null
+++ b/drivers/platform/x86/amd/hsmp/Kconfig
@@ -0,0 +1,17 @@
+# SPDX-License-Identifier: GPL-2.0-only
+#
+# AMD HSMP driver
+#
+
+config AMD_HSMP
+	tristate "AMD HSMP Driver"
+	depends on AMD_NB && X86_64
+	help
+	  The driver provides a way for user space tools to monitor and manage
+	  system management functionality on EPYC server CPUs from AMD.
+
+	  Host System Management Port (HSMP) interface is a mailbox interface
+	  between the x86 core and the System Management Unit (SMU) firmware.
+
+	  If you choose to compile this driver as a module the module will be
+	  called amd_hsmp.
diff --git a/drivers/platform/x86/amd/hsmp/Makefile b/drivers/platform/x86/amd/hsmp/Makefile
new file mode 100644
index 000000000000..0e5e4cc68145
--- /dev/null
+++ b/drivers/platform/x86/amd/hsmp/Makefile
@@ -0,0 +1,9 @@
+# SPDX-License-Identifier: GPL-2.0
+#
+# Makefile for linux/drivers/platform/x86/amd/hsmp
+# AMD Host System Management Port
+#
+
+amd_hsmp-y			:= hsmp.o
+amd_hsmp-$(CONFIG_ACPI)		+= hsmp-acpi.o
+obj-$(CONFIG_AMD_HSMP)		+= amd_hsmp.o
diff --git a/drivers/platform/x86/amd/hsmp/hsmp-acpi.c b/drivers/platform/x86/amd/hsmp/hsmp-acpi.c
new file mode 100644
index 000000000000..90aa1bdbf7f1
--- /dev/null
+++ b/drivers/platform/x86/amd/hsmp/hsmp-acpi.c
@@ -0,0 +1,219 @@
+// SPDX-License-Identifier: GPL-2.0
+/*
+ * AMD HSMP Platform Driver
+ * Copyright (c) 2024, AMD.
+ * All Rights Reserved.
+ *
+ * This file provides ACPI parsing implementation for HSMP interface
+ */
+
+#include "hsmp.h"
+
+/* These are the strings specified in ACPI table */
+#define MSG_IDOFF_STR           "MsgIdOffset"
+#define MSG_ARGOFF_STR          "MsgArgOffset"
+#define MSG_RESPOFF_STR         "MsgRspOffset"
+
+/* This is the UUID used for HSMP */
+static const guid_t acpi_hsmp_uuid = GUID_INIT(0xb74d619d, 0x5707, 0x48bd,
+						0xa6, 0x9f, 0x4e, 0xa2,
+						0x87, 0x1f, 0xc2, 0xf6);
+
+static inline bool is_acpi_hsmp_uuid(union acpi_object *obj)
+{
+	if (obj->type == ACPI_TYPE_BUFFER && obj->buffer.length == UUID_SIZE)
+		return guid_equal((guid_t *)obj->buffer.pointer, &acpi_hsmp_uuid);
+
+	return false;
+}
+
+static acpi_status hsmp_resource(struct acpi_resource *res, void *data)
+{
+	struct hsmp_socket *sock = data;
+	struct resource r;
+
+	switch (res->type) {
+	case ACPI_RESOURCE_TYPE_FIXED_MEMORY32:
+		if (!acpi_dev_resource_memory(res, &r))
+			return AE_ERROR;
+		if (!r.start || r.end < r.start || !(r.flags & IORESOURCE_MEM_WRITEABLE))
+			return AE_ERROR;
+		sock->mbinfo.base_addr = r.start;
+		sock->mbinfo.size = resource_size(&r);
+		break;
+	case ACPI_RESOURCE_TYPE_END_TAG:
+		break;
+	default:
+		return AE_ERROR;
+	}
+
+	return AE_OK;
+}
+
+static int hsmp_read_acpi_crs(struct hsmp_socket *sock)
+{
+	acpi_status status;
+
+	status = acpi_walk_resources(ACPI_HANDLE(sock->dev), METHOD_NAME__CRS,
+				     hsmp_resource, sock);
+	if (ACPI_FAILURE(status)) {
+		dev_err(sock->dev, "Failed to look up MP1 base address from CRS method, err: %s\n",
+			acpi_format_exception(status));
+		return -EINVAL;
+	}
+	if (!sock->mbinfo.base_addr || !sock->mbinfo.size)
+		return -EINVAL;
+
+	/* The mapped region should be un cached */
+	sock->virt_base_addr = devm_ioremap_uc(sock->dev, sock->mbinfo.base_addr,
+					       sock->mbinfo.size);
+	if (!sock->virt_base_addr) {
+		dev_err(sock->dev, "Failed to ioremap MP1 base address\n");
+		return -ENOMEM;
+	}
+
+	return 0;
+}
+
+static int hsmp_read_acpi_dsd(struct hsmp_socket *sock)
+{
+	struct acpi_buffer buf = { ACPI_ALLOCATE_BUFFER, NULL };
+	union acpi_object *guid, *mailbox_package;
+	union acpi_object *dsd;
+	acpi_status status;
+	int ret = 0;
+	int j;
+
+	status = acpi_evaluate_object_typed(ACPI_HANDLE(sock->dev), "_DSD", NULL,
+					    &buf, ACPI_TYPE_PACKAGE);
+	if (ACPI_FAILURE(status)) {
+		dev_err(sock->dev, "Failed to read mailbox reg offsets from DSD table, err: %s\n",
+			acpi_format_exception(status));
+		return -ENODEV;
+	}
+
+	dsd = buf.pointer;
+
+	/* HSMP _DSD property should contain 2 objects.
+	 * 1. guid which is an acpi object of type ACPI_TYPE_BUFFER
+	 * 2. mailbox which is an acpi object of type ACPI_TYPE_PACKAGE
+	 *    This mailbox object contains 3 more acpi objects of type
+	 *    ACPI_TYPE_PACKAGE for holding msgid, msgresp, msgarg offsets
+	 *    these packages inturn contain 2 acpi objects of type
+	 *    ACPI_TYPE_STRING and ACPI_TYPE_INTEGER
+	 */
+	if (!dsd || dsd->type != ACPI_TYPE_PACKAGE || dsd->package.count != 2) {
+		ret = -EINVAL;
+		goto free_buf;
+	}
+
+	guid = &dsd->package.elements[0];
+	mailbox_package = &dsd->package.elements[1];
+	if (!is_acpi_hsmp_uuid(guid) || mailbox_package->type != ACPI_TYPE_PACKAGE) {
+		dev_err(sock->dev, "Invalid hsmp _DSD table data\n");
+		ret = -EINVAL;
+		goto free_buf;
+	}
+
+	for (j = 0; j < mailbox_package->package.count; j++) {
+		union acpi_object *msgobj, *msgstr, *msgint;
+
+		msgobj	= &mailbox_package->package.elements[j];
+		msgstr	= &msgobj->package.elements[0];
+		msgint	= &msgobj->package.elements[1];
+
+		/* package should have 1 string and 1 integer object */
+		if (msgobj->type != ACPI_TYPE_PACKAGE ||
+		    msgstr->type != ACPI_TYPE_STRING ||
+		    msgint->type != ACPI_TYPE_INTEGER) {
+			ret = -EINVAL;
+			goto free_buf;
+		}
+
+		if (!strncmp(msgstr->string.pointer, MSG_IDOFF_STR,
+			     msgstr->string.length)) {
+			sock->mbinfo.msg_id_off = msgint->integer.value;
+		} else if (!strncmp(msgstr->string.pointer, MSG_RESPOFF_STR,
+				    msgstr->string.length)) {
+			sock->mbinfo.msg_resp_off =  msgint->integer.value;
+		} else if (!strncmp(msgstr->string.pointer, MSG_ARGOFF_STR,
+				    msgstr->string.length)) {
+			sock->mbinfo.msg_arg_off = msgint->integer.value;
+		} else {
+			ret = -ENOENT;
+			goto free_buf;
+		}
+	}
+
+	if (!sock->mbinfo.msg_id_off || !sock->mbinfo.msg_resp_off ||
+	    !sock->mbinfo.msg_arg_off)
+		ret = -EINVAL;
+
+free_buf:
+	ACPI_FREE(buf.pointer);
+	return ret;
+}
+
+int init_acpi(struct hsmp_plat_device *plat_dev, struct device *dev)
+{
+	struct hsmp_socket *sock;
+	u16 sock_ind;
+	int ret;
+
+	plat_dev->is_acpi_device = true;
+
+	ret = hsmp_get_uid(dev, &sock_ind);
+	if (ret)
+		return ret;
+	if (sock_ind >= plat_dev->num_sockets)
+		return -EINVAL;
+
+	sock = &plat_dev->sock[sock_ind];
+
+	sock->sock_ind		= sock_ind;
+	sock->dev		= dev;
+
+	sema_init(&sock->hsmp_sem, 1);
+
+	/* Read MP1 base address from CRS method */
+	ret = hsmp_read_acpi_crs(sock);
+	if (ret)
+		return ret;
+
+	/* Read mailbox offsets from DSD table */
+	return hsmp_read_acpi_dsd(sock);
+}
+
+int check_acpi_support(struct device *dev, const struct acpi_device_id *amd_hsmp_acpi_ids)
+{
+	struct acpi_device *adev;
+
+	adev = ACPI_COMPANION(dev);
+	if (adev && !acpi_match_device_ids(adev, amd_hsmp_acpi_ids))
+		return 0;
+
+	return -ENODEV;
+}
+
+int hsmp_get_uid(struct device *dev, u16 *sock_ind)
+{
+	char *uid;
+
+	/*
+	 * UID (ID00, ID01..IDXX) is used for differentiating sockets,
+	 * read it and strip the "ID" part of it and convert the remaining
+	 * bytes to integer.
+	 */
+	uid = acpi_device_uid(ACPI_COMPANION(dev));
+
+	return kstrtou16(uid + 2, 10, sock_ind);
+}
+
+void amd_hsmp_acpi_rdwr(struct hsmp_socket *sock, u32 offset,
+			u32 *value, bool write)
+{
+	if (write)
+		iowrite32(*value, sock->virt_base_addr + offset);
+	else
+		*value = ioread32(sock->virt_base_addr + offset);
+}
diff --git a/drivers/platform/x86/amd/hsmp.c b/drivers/platform/x86/amd/hsmp/hsmp.c
similarity index 72%
rename from drivers/platform/x86/amd/hsmp.c
rename to drivers/platform/x86/amd/hsmp/hsmp.c
index 807a5066dacc..1df2b34ddbbd 100644
--- a/drivers/platform/x86/amd/hsmp.c
+++ b/drivers/platform/x86/amd/hsmp/hsmp.c
@@ -10,18 +10,10 @@
 #define pr_fmt(fmt) KBUILD_MODNAME ": " fmt
 
 #include <asm/amd_hsmp.h>
-#include <asm/amd_nb.h>
-#include <linux/delay.h>
-#include <linux/io.h>
-#include <linux/miscdevice.h>
-#include <linux/module.h>
-#include <linux/pci.h>
-#include <linux/platform_device.h>
-#include <linux/semaphore.h>
-#include <linux/acpi.h>
+#include "hsmp.h"
 
 #define DRIVER_NAME		"amd_hsmp"
-#define DRIVER_VERSION		"2.2"
+#define DRIVER_VERSION		"2.3"
 #define ACPI_HSMP_DEVICE_HID	"AMDI0097"
 
 /* HSMP Status / Error codes */
@@ -55,44 +47,8 @@
 #define HSMP_DEVNODE_NAME	"hsmp"
 #define HSMP_METRICS_TABLE_NAME	"metrics_bin"
 
-#define HSMP_ATTR_GRP_NAME_SIZE	10
-
-/* These are the strings specified in ACPI table */
-#define MSG_IDOFF_STR		"MsgIdOffset"
-#define MSG_ARGOFF_STR		"MsgArgOffset"
-#define MSG_RESPOFF_STR		"MsgRspOffset"
-
 #define MAX_AMD_SOCKETS 8
 
-struct hsmp_mbaddr_info {
-	u32 base_addr;
-	u32 msg_id_off;
-	u32 msg_resp_off;
-	u32 msg_arg_off;
-	u32 size;
-};
-
-struct hsmp_socket {
-	struct bin_attribute hsmp_attr;
-	struct hsmp_mbaddr_info mbinfo;
-	void __iomem *metric_tbl_addr;
-	void __iomem *virt_base_addr;
-	struct semaphore hsmp_sem;
-	char name[HSMP_ATTR_GRP_NAME_SIZE];
-	struct pci_dev *root;
-	struct device *dev;
-	u16 sock_ind;
-};
-
-struct hsmp_plat_device {
-	struct miscdevice hsmp_device;
-	struct hsmp_socket *sock;
-	u32 proto_ver;
-	u16 num_sockets;
-	bool is_acpi_device;
-	bool is_probed;
-};
-
 static struct hsmp_plat_device plat_dev;
 
 static int amd_hsmp_pci_rdwr(struct hsmp_socket *sock, u32 offset,
@@ -114,15 +70,6 @@ static int amd_hsmp_pci_rdwr(struct hsmp_socket *sock, u32 offset,
 	return ret;
 }
 
-static void amd_hsmp_acpi_rdwr(struct hsmp_socket *sock, u32 offset,
-			       u32 *value, bool write)
-{
-	if (write)
-		iowrite32(*value, sock->virt_base_addr + offset);
-	else
-		*value = ioread32(sock->virt_base_addr + offset);
-}
-
 static int amd_hsmp_rdwr(struct hsmp_socket *sock, u32 offset,
 			 u32 *value, bool write)
 {
@@ -321,6 +268,22 @@ static int hsmp_test(u16 sock_ind, u32 value)
 	return ret;
 }
 
+static int hsmp_cache_proto_ver(u16 sock_ind)
+{
+	struct hsmp_message msg = { 0 };
+	int ret;
+
+	msg.msg_id	= HSMP_GET_PROTO_VER;
+	msg.sock_ind	= sock_ind;
+	msg.response_sz = hsmp_msg_desc_table[HSMP_GET_PROTO_VER].response_sz;
+
+	ret = hsmp_send_message(&msg);
+	if (!ret)
+		plat_dev.proto_ver = msg.args[0];
+
+	return ret;
+}
+
 static long hsmp_ioctl(struct file *fp, unsigned int cmd, unsigned long arg)
 {
 	int __user *arguser = (int  __user *)arg;
@@ -383,181 +346,6 @@ static const struct file_operations hsmp_fops = {
 	.compat_ioctl	= hsmp_ioctl,
 };
 
-/* This is the UUID used for HSMP */
-static const guid_t acpi_hsmp_uuid = GUID_INIT(0xb74d619d, 0x5707, 0x48bd,
-						0xa6, 0x9f, 0x4e, 0xa2,
-						0x87, 0x1f, 0xc2, 0xf6);
-
-static inline bool is_acpi_hsmp_uuid(union acpi_object *obj)
-{
-	if (obj->type == ACPI_TYPE_BUFFER && obj->buffer.length == UUID_SIZE)
-		return guid_equal((guid_t *)obj->buffer.pointer, &acpi_hsmp_uuid);
-
-	return false;
-}
-
-static inline int hsmp_get_uid(struct device *dev, u16 *sock_ind)
-{
-	char *uid;
-
-	/*
-	 * UID (ID00, ID01..IDXX) is used for differentiating sockets,
-	 * read it and strip the "ID" part of it and convert the remaining
-	 * bytes to integer.
-	 */
-	uid = acpi_device_uid(ACPI_COMPANION(dev));
-
-	return kstrtou16(uid + 2, 10, sock_ind);
-}
-
-static acpi_status hsmp_resource(struct acpi_resource *res, void *data)
-{
-	struct hsmp_socket *sock = data;
-	struct resource r;
-
-	switch (res->type) {
-	case ACPI_RESOURCE_TYPE_FIXED_MEMORY32:
-		if (!acpi_dev_resource_memory(res, &r))
-			return AE_ERROR;
-		if (!r.start || r.end < r.start || !(r.flags & IORESOURCE_MEM_WRITEABLE))
-			return AE_ERROR;
-		sock->mbinfo.base_addr = r.start;
-		sock->mbinfo.size = resource_size(&r);
-		break;
-	case ACPI_RESOURCE_TYPE_END_TAG:
-		break;
-	default:
-		return AE_ERROR;
-	}
-
-	return AE_OK;
-}
-
-static int hsmp_read_acpi_dsd(struct hsmp_socket *sock)
-{
-	struct acpi_buffer buf = { ACPI_ALLOCATE_BUFFER, NULL };
-	union acpi_object *guid, *mailbox_package;
-	union acpi_object *dsd;
-	acpi_status status;
-	int ret = 0;
-	int j;
-
-	status = acpi_evaluate_object_typed(ACPI_HANDLE(sock->dev), "_DSD", NULL,
-					    &buf, ACPI_TYPE_PACKAGE);
-	if (ACPI_FAILURE(status)) {
-		dev_err(sock->dev, "Failed to read mailbox reg offsets from DSD table, err: %s\n",
-			acpi_format_exception(status));
-		return -ENODEV;
-	}
-
-	dsd = buf.pointer;
-
-	/* HSMP _DSD property should contain 2 objects.
-	 * 1. guid which is an acpi object of type ACPI_TYPE_BUFFER
-	 * 2. mailbox which is an acpi object of type ACPI_TYPE_PACKAGE
-	 *    This mailbox object contains 3 more acpi objects of type
-	 *    ACPI_TYPE_PACKAGE for holding msgid, msgresp, msgarg offsets
-	 *    these packages inturn contain 2 acpi objects of type
-	 *    ACPI_TYPE_STRING and ACPI_TYPE_INTEGER
-	 */
-	if (!dsd || dsd->type != ACPI_TYPE_PACKAGE || dsd->package.count != 2) {
-		ret = -EINVAL;
-		goto free_buf;
-	}
-
-	guid = &dsd->package.elements[0];
-	mailbox_package = &dsd->package.elements[1];
-	if (!is_acpi_hsmp_uuid(guid) || mailbox_package->type != ACPI_TYPE_PACKAGE) {
-		dev_err(sock->dev, "Invalid hsmp _DSD table data\n");
-		ret = -EINVAL;
-		goto free_buf;
-	}
-
-	for (j = 0; j < mailbox_package->package.count; j++) {
-		union acpi_object *msgobj, *msgstr, *msgint;
-
-		msgobj	= &mailbox_package->package.elements[j];
-		msgstr	= &msgobj->package.elements[0];
-		msgint	= &msgobj->package.elements[1];
-
-		/* package should have 1 string and 1 integer object */
-		if (msgobj->type != ACPI_TYPE_PACKAGE ||
-		    msgstr->type != ACPI_TYPE_STRING ||
-		    msgint->type != ACPI_TYPE_INTEGER) {
-			ret = -EINVAL;
-			goto free_buf;
-		}
-
-		if (!strncmp(msgstr->string.pointer, MSG_IDOFF_STR,
-			     msgstr->string.length)) {
-			sock->mbinfo.msg_id_off = msgint->integer.value;
-		} else if (!strncmp(msgstr->string.pointer, MSG_RESPOFF_STR,
-				    msgstr->string.length)) {
-			sock->mbinfo.msg_resp_off =  msgint->integer.value;
-		} else if (!strncmp(msgstr->string.pointer, MSG_ARGOFF_STR,
-				    msgstr->string.length)) {
-			sock->mbinfo.msg_arg_off = msgint->integer.value;
-		} else {
-			ret = -ENOENT;
-			goto free_buf;
-		}
-	}
-
-	if (!sock->mbinfo.msg_id_off || !sock->mbinfo.msg_resp_off ||
-	    !sock->mbinfo.msg_arg_off)
-		ret = -EINVAL;
-
-free_buf:
-	ACPI_FREE(buf.pointer);
-	return ret;
-}
-
-static int hsmp_read_acpi_crs(struct hsmp_socket *sock)
-{
-	acpi_status status;
-
-	status = acpi_walk_resources(ACPI_HANDLE(sock->dev), METHOD_NAME__CRS,
-				     hsmp_resource, sock);
-	if (ACPI_FAILURE(status)) {
-		dev_err(sock->dev, "Failed to look up MP1 base address from CRS method, err: %s\n",
-			acpi_format_exception(status));
-		return -EINVAL;
-	}
-	if (!sock->mbinfo.base_addr || !sock->mbinfo.size)
-		return -EINVAL;
-
-	/* The mapped region should be un cached */
-	sock->virt_base_addr = devm_ioremap_uc(sock->dev, sock->mbinfo.base_addr,
-					       sock->mbinfo.size);
-	if (!sock->virt_base_addr) {
-		dev_err(sock->dev, "Failed to ioremap MP1 base address\n");
-		return -ENOMEM;
-	}
-
-	return 0;
-}
-
-/* Parse the ACPI table to read the data */
-static int hsmp_parse_acpi_table(struct device *dev, u16 sock_ind)
-{
-	struct hsmp_socket *sock = &plat_dev.sock[sock_ind];
-	int ret;
-
-	sock->sock_ind		= sock_ind;
-	sock->dev		= dev;
-	plat_dev.is_acpi_device	= true;
-
-	sema_init(&sock->hsmp_sem, 1);
-
-	/* Read MP1 base address from CRS method */
-	ret = hsmp_read_acpi_crs(sock);
-	if (ret)
-		return ret;
-
-	/* Read mailbox offsets from DSD table */
-	return hsmp_read_acpi_dsd(sock);
-}
-
 static ssize_t hsmp_metric_tbl_read(struct file *filp, struct kobject *kobj,
 				    struct bin_attribute *bin_attr, char *buf,
 				    loff_t off, size_t count)
@@ -670,8 +458,15 @@ static int hsmp_create_non_acpi_sysfs_if(struct device *dev)
 {
 	const struct attribute_group **hsmp_attr_grps;
 	struct attribute_group *attr_grp;
+	int ret;
 	u16 i;
 
+	ret = hsmp_cache_proto_ver(0);
+	if (ret) {
+		dev_err(dev, "Failed to read HSMP protocol version\n");
+		return ret;
+	}
+
 	hsmp_attr_grps = devm_kcalloc(dev, plat_dev.num_sockets + 1,
 				      sizeof(*hsmp_attr_grps),
 				      GFP_KERNEL);
@@ -706,12 +501,18 @@ static int hsmp_create_acpi_sysfs_if(struct device *dev)
 	if (!attr_grp)
 		return -ENOMEM;
 
-	attr_grp->is_bin_visible = hsmp_is_sock_attr_visible;
-
 	ret = hsmp_get_uid(dev, &sock_ind);
 	if (ret)
 		return ret;
 
+	ret = hsmp_cache_proto_ver(sock_ind);
+	if (ret) {
+		dev_err(dev, "Failed to read HSMP protocol version\n");
+		return ret;
+	}
+
+	attr_grp->is_bin_visible = hsmp_is_sock_attr_visible;
+
 	ret = hsmp_create_attr_list(attr_grp, dev, sock_ind);
 	if (ret)
 		return ret;
@@ -719,22 +520,6 @@ static int hsmp_create_acpi_sysfs_if(struct device *dev)
 	return devm_device_add_group(dev, attr_grp);
 }
 
-static int hsmp_cache_proto_ver(u16 sock_ind)
-{
-	struct hsmp_message msg = { 0 };
-	int ret;
-
-	msg.msg_id	= HSMP_GET_PROTO_VER;
-	msg.sock_ind	= sock_ind;
-	msg.response_sz = hsmp_msg_desc_table[HSMP_GET_PROTO_VER].response_sz;
-
-	ret = hsmp_send_message(&msg);
-	if (!ret)
-		plat_dev.proto_ver = msg.args[0];
-
-	return ret;
-}
-
 static inline bool is_f1a_m0h(void)
 {
 	if (boot_cpu_data.x86 == 0x1A && boot_cpu_data.x86_model <= 0x0F)
@@ -791,8 +576,6 @@ MODULE_DEVICE_TABLE(acpi, amd_hsmp_acpi_ids);
 
 static int hsmp_pltdrv_probe(struct platform_device *pdev)
 {
-	struct acpi_device *adev;
-	u16 sock_ind = 0;
 	int ret;
 
 	/*
@@ -809,46 +592,33 @@ static int hsmp_pltdrv_probe(struct platform_device *pdev)
 		if (!plat_dev.sock)
 			return -ENOMEM;
 	}
-	adev = ACPI_COMPANION(&pdev->dev);
-	if (adev && !acpi_match_device_ids(adev, amd_hsmp_acpi_ids)) {
-		ret = hsmp_get_uid(&pdev->dev, &sock_ind);
-		if (ret)
-			return ret;
-		if (sock_ind >= plat_dev.num_sockets)
-			return -EINVAL;
-		ret = hsmp_parse_acpi_table(&pdev->dev, sock_ind);
+
+	ret = check_acpi_support(&pdev->dev, amd_hsmp_acpi_ids);
+	if (!ret) {
+		ret = init_acpi(&plat_dev, &pdev->dev);
 		if (ret) {
-			dev_err(&pdev->dev, "Failed to parse ACPI table\n");
+			dev_err(&pdev->dev, "Failed to initialize HSMP interface.\n");
 			return ret;
 		}
-		/* Test the hsmp interface */
-		ret = hsmp_test(sock_ind, 0xDEADBEEF);
+
+		ret = hsmp_create_acpi_sysfs_if(&pdev->dev);
 		if (ret) {
-			dev_err(&pdev->dev, "HSMP test message failed on Fam:%x model:%x\n",
-				boot_cpu_data.x86, boot_cpu_data.x86_model);
-			dev_err(&pdev->dev, "Is HSMP disabled in BIOS ?\n");
+			dev_err(&pdev->dev, "Failed to create HSMP sysfs interface\n");
 			return ret;
 		}
 	} else {
 		ret = init_platform_device(&pdev->dev);
 		if (ret) {
-			dev_err(&pdev->dev, "Failed to init HSMP mailbox\n");
+			dev_err(&pdev->dev, "Failed to initialize HSMP interface.\n");
 			return ret;
 		}
-	}
-
-	ret = hsmp_cache_proto_ver(sock_ind);
-	if (ret) {
-		dev_err(&pdev->dev, "Failed to read HSMP protocol version\n");
-		return ret;
-	}
 
-	if (plat_dev.is_acpi_device)
-		ret = hsmp_create_acpi_sysfs_if(&pdev->dev);
-	else
 		ret = hsmp_create_non_acpi_sysfs_if(&pdev->dev);
-	if (ret)
-		dev_err(&pdev->dev, "Failed to create HSMP sysfs interface\n");
+		if (ret) {
+			dev_err(&pdev->dev, "Failed to create HSMP sysfs interface\n");
+			return ret;
+		}
+	}
 
 	if (!plat_dev.is_probed) {
 		plat_dev.hsmp_device.name	= HSMP_CDEV_NAME;
diff --git a/drivers/platform/x86/amd/hsmp/hsmp.h b/drivers/platform/x86/amd/hsmp/hsmp.h
new file mode 100644
index 000000000000..23629ccb1fb0
--- /dev/null
+++ b/drivers/platform/x86/amd/hsmp/hsmp.h
@@ -0,0 +1,82 @@
+/* SPDX-License-Identifier: GPL-2.0 */
+/*
+ * AMD HSMP Platform Driver
+ * Copyright (c) 2024, AMD.
+ * All Rights Reserved.
+ *
+ * This file provides a header implementation for HSMP interface
+ */
+
+#ifndef _HSMP_H
+#define _HSMP_H
+
+#include <asm/amd_nb.h>
+#include <linux/delay.h>
+#include <linux/io.h>
+#include <linux/module.h>
+#include <linux/pci.h>
+#include <linux/platform_device.h>
+#include <linux/semaphore.h>
+#include <linux/acpi.h>
+#include <linux/miscdevice.h>
+
+#define HSMP_ATTR_GRP_NAME_SIZE 10
+
+struct hsmp_mbaddr_info {
+	u32 base_addr;
+	u32 msg_id_off;
+	u32 msg_resp_off;
+	u32 msg_arg_off;
+	u32 size;
+};
+
+struct hsmp_socket {
+	struct bin_attribute hsmp_attr;
+	struct hsmp_mbaddr_info mbinfo;
+	void __iomem *metric_tbl_addr;
+	void __iomem *virt_base_addr;
+	struct semaphore hsmp_sem;
+	char name[HSMP_ATTR_GRP_NAME_SIZE];
+	struct pci_dev *root;
+	struct device *dev;
+	u16 sock_ind;
+};
+
+struct hsmp_plat_device {
+	struct miscdevice hsmp_device;
+	struct hsmp_socket *sock;
+	u32 proto_ver;
+	u16 num_sockets;
+	bool is_acpi_device;
+	bool is_probed;
+};
+
+#ifdef CONFIG_ACPI
+int hsmp_get_uid(struct device *dev, u16 *sock_ind);
+void amd_hsmp_acpi_rdwr(struct hsmp_socket *sock, u32 offset,
+			u32 *value, bool write);
+int check_acpi_support(struct device *dev, const struct acpi_device_id *amd_hsmp_acpi_ids);
+int init_acpi(struct hsmp_plat_device *plat_dev, struct device *dev);
+#else
+static inline void amd_hsmp_acpi_rdwr(struct hsmp_socket *sock, u32 offset,
+				      u32 *value, bool write)
+{
+}
+
+static inline int hsmp_get_uid(struct device *dev, u16 *sock_ind)
+{
+	return -ENODEV;
+}
+
+static inline int check_acpi_support(struct device *dev,
+				     const struct acpi_device_id *amd_hsmp_acpi_ids)
+{
+	return -ENODEV;
+}
+
+static inline int init_acpi(struct hsmp_plat_device *plat_dev, struct device *dev)
+{
+	return -ENODEV;
+}
+#endif
+#endif
-- 
2.25.1

[PATCH 3/3] platform/x86/amd/hsmp: Make HSMP as default m

[Suma Hegde]

Making HSMP as "default m" in Kconfig, enables customers
who rely on the standard distros to use the HSMP by dynamically
loading it.

Signed-off-by: Suma Hegde <suma.hegde@amd.com>
Reviewed-by: Naveen Krishna Chatradhi <naveenkrishna.chatradhi@amd.com>
---
 drivers/platform/x86/amd/hsmp/Kconfig | 1 +
 1 file changed, 1 insertion(+)

diff --git a/drivers/platform/x86/amd/hsmp/Kconfig b/drivers/platform/x86/amd/hsmp/Kconfig
index 26096f901c22..9d3f422b3f81 100644
--- a/drivers/platform/x86/amd/hsmp/Kconfig
+++ b/drivers/platform/x86/amd/hsmp/Kconfig
@@ -6,6 +6,7 @@
 config AMD_HSMP
 	tristate "AMD HSMP Driver"
 	depends on AMD_NB && X86_64
+	default m
 	help
 	  The driver provides a way for user space tools to monitor and manage
 	  system management functionality on EPYC server CPUs from AMD.
-- 
2.25.1

Re: [PATCH 0/3] platform/x86/amd/hsmp: ACPI, Non-ACPI code split and other misc changes

[Mario Limonciello]

On 4/23/2024 04:14, Suma Hegde wrote:
> Patch1:
> The first patch is to address the concerns mentioned by Limonciello,
> Mario i.e to address the issue of
> "Probing registers that don't match the same mailboxes randomly on
> client or embedded parts might lead to unexpected behaviors".
> 
> Because of some limitations, instead of the methods suggested by Mario,
> family check is added to differentiate server socs and client socs.
> This check is only for backward compatibility, Going forward all
> systems will have ACPI based probing only.
> 

Thanks!  Hans, please drop the PM profile patch from your review queue 
in favor of this series instead.

> 
> Patch2:
> The second patch splits ACPI code into  separate file there by removing the
> dependency of ACPI in Kconfig.
> This is based on the suggestion from Ilpo Jarvinen.
> 
> 
> Patch3:
> The third patch makes HSMP as "default m" so that anyone who uses standard
> distros will be able to use HSMP by dynamically loading it.
> 
> 
> Suma Hegde (3):
>    platform/x86/amd/hsmp: Check HSMP support on AMD family of processors
>    platform/x86/amd/hsmp: Split the ACPI and non-ACPI code
>    platform/x86/amd/hsmp: Make HSMP as default m
> 
>   MAINTAINERS                                |   2 +-
>   drivers/platform/x86/amd/Kconfig           |  14 +-
>   drivers/platform/x86/amd/Makefile          |   3 +-
>   drivers/platform/x86/amd/hsmp/Kconfig      |  18 +
>   drivers/platform/x86/amd/hsmp/Makefile     |   9 +
>   drivers/platform/x86/amd/hsmp/hsmp-acpi.c  | 219 ++++++++++++
>   drivers/platform/x86/amd/{ => hsmp}/hsmp.c | 378 +++++----------------
>   drivers/platform/x86/amd/hsmp/hsmp.h       |  82 +++++
>   8 files changed, 425 insertions(+), 300 deletions(-)
>   create mode 100644 drivers/platform/x86/amd/hsmp/Kconfig
>   create mode 100644 drivers/platform/x86/amd/hsmp/Makefile
>   create mode 100644 drivers/platform/x86/amd/hsmp/hsmp-acpi.c
>   rename drivers/platform/x86/amd/{ => hsmp}/hsmp.c (71%)
>   create mode 100644 drivers/platform/x86/amd/hsmp/hsmp.h
> 

Re: [PATCH 1/3] platform/x86/amd/hsmp: Check HSMP support on AMD family of processors

[Mario Limonciello]

On 4/23/2024 04:14, Suma Hegde wrote:
> HSMP interface is supported only on few x86 processors from AMD.
> Accessing HSMP registers on rest of the platforms might cause
> unexpected behaviour. So add a check.
> 
> Also unavailability of this interface on rest of the processors
> is not an error. Hence, use pr_info() instead of the pr_err() to
> log the message.
> 
> Signed-off-by: Suma Hegde <suma.hegde@amd.com>
> Reviewed-by: Naveen Krishna Chatradhi <naveenkrishna.chatradhi@amd.com>
> ---
>   drivers/platform/x86/amd/hsmp.c | 54 ++++++++++++++++++++++++++++-----
>   1 file changed, 47 insertions(+), 7 deletions(-)
> 
> diff --git a/drivers/platform/x86/amd/hsmp.c b/drivers/platform/x86/amd/hsmp.c
> index 1927be901108..807a5066dacc 100644
> --- a/drivers/platform/x86/amd/hsmp.c
> +++ b/drivers/platform/x86/amd/hsmp.c
> @@ -907,16 +907,48 @@ static int hsmp_plat_dev_register(void)
>   	return ret;
>   }
>   
> +/*
> + * This check is only needed for backward compatibility of previous platforms.
> + * All new platforms are expected to support ACPI based probing.
> + */
> +static bool is_hsmp_supported(void)

I think it's better to use "legacy" in this function name to make it 
clear that this isn't a requirement for newer platforms going forward. 
Maybe like

legacy_hsmp_support()?

> +{
> +	bool ret = true;

As this is a simple boolean return with no needing to clear memory, I 
think you can just "return true" or "return false" everywhere in this 
function.

> +
> +	if (boot_cpu_data.x86_vendor != X86_VENDOR_AMD)
> +		return false;
> +
> +	switch (boot_cpu_data.x86) {
> +	case 0x19:
> +		switch (boot_cpu_data.x86_model) {
> +		case 0x00 ... 0x1F:
> +		case 0x30 ... 0x3F:
> +		case 0x90 ... 0x9F:
> +		case 0xA0 ... 0xAF:
> +			break;
> +		default:
> +			ret = false;
> +		}
> +		break;
> +	case 0x1A:
> +		switch (boot_cpu_data.x86_model) {
> +		case 0x00 ... 0x1F:
> +			break;

There actually are going to be 0x1A platforms that don't use ACPI?
I guess I'm surprised.

> +		default:
> +			ret = false;
> +		}
> +		break;
> +	default:
> +		ret = false;
> +	}
> +
> +	return ret;
> +}
> +
>   static int __init hsmp_plt_init(void)
>   {
>   	int ret = -ENODEV;
>   
> -	if (boot_cpu_data.x86_vendor != X86_VENDOR_AMD || boot_cpu_data.x86 < 0x19) {
> -		pr_err("HSMP is not supported on Family:%x model:%x\n",
> -		       boot_cpu_data.x86, boot_cpu_data.x86_model);
> -		return ret;
> -	}
> -
>   	/*
>   	 * amd_nb_num() returns number of SMN/DF interfaces present in the system
>   	 * if we have N SMN/DF interfaces that ideally means N sockets
> @@ -930,7 +962,15 @@ static int __init hsmp_plt_init(void)
>   		return ret;
>   
>   	if (!plat_dev.is_acpi_device) {
> -		ret = hsmp_plat_dev_register();
> +		if (is_hsmp_supported()) {
> +			/* Not ACPI device, but supports HSMP, register a plat_dev */
> +			ret = hsmp_plat_dev_register();
> +		} else {
> +			/* Not ACPI, Does not support HSMP */
> +			pr_info("HSMP is not supported on Family:%x model:%x\n",
> +				boot_cpu_data.x86, boot_cpu_data.x86_model);
> +			ret = -ENODEV;
> +		}
>   		if (ret)
>   			platform_driver_unregister(&amd_hsmp_driver);
>   	}

Re: [PATCH 2/3] platform/x86/amd/hsmp: Split the ACPI and non-ACPI code

[Mario Limonciello]

On 4/23/2024 04:14, Suma Hegde wrote:
> HSMP driver is supported on ACPI as well as non-ACPI based platforms.
> Making it dependent on ACPI in Kconfig, causes it to be compiled only on
> systems which have ACPI enabled.
> Remove this dependency by seggregating acpi specific code into
segregating
> hsmp-acpi.c file.
> This ensures that HSMP can be compiled with or without
> ACPI enabled.
> 
> Also create hsmp/ directory with Kconfig and Makefile
> to keep hsmp related files.
> 
> Signed-off-by: Suma Hegde <suma.hegde@amd.com>
> Reviewed-by: Naveen Krishna Chatradhi <naveenkrishna.chatradhi@amd.com>
> ---
>   MAINTAINERS                                |   2 +-
>   drivers/platform/x86/amd/Kconfig           |  14 +-
>   drivers/platform/x86/amd/Makefile          |   3 +-
>   drivers/platform/x86/amd/hsmp/Kconfig      |  17 ++
>   drivers/platform/x86/amd/hsmp/Makefile     |   9 +
>   drivers/platform/x86/amd/hsmp/hsmp-acpi.c  | 219 ++++++++++++++
>   drivers/platform/x86/amd/{ => hsmp}/hsmp.c | 324 +++------------------
>   drivers/platform/x86/amd/hsmp/hsmp.h       |  82 ++++++
>   8 files changed, 377 insertions(+), 293 deletions(-)
>   create mode 100644 drivers/platform/x86/amd/hsmp/Kconfig
>   create mode 100644 drivers/platform/x86/amd/hsmp/Makefile
>   create mode 100644 drivers/platform/x86/amd/hsmp/hsmp-acpi.c
>   rename drivers/platform/x86/amd/{ => hsmp}/hsmp.c (72%)
>   create mode 100644 drivers/platform/x86/amd/hsmp/hsmp.h
> 
> diff --git a/MAINTAINERS b/MAINTAINERS
> index ebf03f5f0619..bdc390ddcc4b 100644
> --- a/MAINTAINERS
> +++ b/MAINTAINERS
> @@ -999,7 +999,7 @@ S:	Maintained
>   F:	Documentation/arch/x86/amd_hsmp.rst
>   F:	arch/x86/include/asm/amd_hsmp.h
>   F:	arch/x86/include/uapi/asm/amd_hsmp.h
> -F:	drivers/platform/x86/amd/hsmp.c
> +F:	drivers/platform/x86/amd/hsmp/
>   
>   AMD IOMMU (AMD-VI)
>   M:	Joerg Roedel <joro@8bytes.org>
> diff --git a/drivers/platform/x86/amd/Kconfig b/drivers/platform/x86/amd/Kconfig
> index f88682d36447..c2f16ccebec4 100644
> --- a/drivers/platform/x86/amd/Kconfig
> +++ b/drivers/platform/x86/amd/Kconfig
> @@ -5,19 +5,7 @@
>   
>   source "drivers/platform/x86/amd/pmf/Kconfig"
>   source "drivers/platform/x86/amd/pmc/Kconfig"
> -
> -config AMD_HSMP
> -	tristate "AMD HSMP Driver"
> -	depends on AMD_NB && X86_64 && ACPI
> -	help
> -	  The driver provides a way for user space tools to monitor and manage
> -	  system management functionality on EPYC server CPUs from AMD.
> -
> -	  Host System Management Port (HSMP) interface is a mailbox interface
> -	  between the x86 core and the System Management Unit (SMU) firmware.
> -
> -	  If you choose to compile this driver as a module the module will be
> -	  called amd_hsmp.
> +source "drivers/platform/x86/amd/hsmp/Kconfig"
>   
>   config AMD_WBRF
>   	bool "AMD Wifi RF Band mitigations (WBRF)"
> diff --git a/drivers/platform/x86/amd/Makefile b/drivers/platform/x86/amd/Makefile
> index dcec0a46f8af..f0b2fe81c685 100644
> --- a/drivers/platform/x86/amd/Makefile
> +++ b/drivers/platform/x86/amd/Makefile
> @@ -5,7 +5,6 @@
>   #
>   
>   obj-$(CONFIG_AMD_PMC)		+= pmc/
> -amd_hsmp-y			:= hsmp.o
> -obj-$(CONFIG_AMD_HSMP)		+= amd_hsmp.o
> +obj-$(CONFIG_AMD_HSMP)		+= hsmp/
>   obj-$(CONFIG_AMD_PMF)		+= pmf/
>   obj-$(CONFIG_AMD_WBRF)		+= wbrf.o
> diff --git a/drivers/platform/x86/amd/hsmp/Kconfig b/drivers/platform/x86/amd/hsmp/Kconfig
> new file mode 100644
> index 000000000000..26096f901c22
> --- /dev/null
> +++ b/drivers/platform/x86/amd/hsmp/Kconfig
> @@ -0,0 +1,17 @@
> +# SPDX-License-Identifier: GPL-2.0-only
> +#
> +# AMD HSMP driver
> +#
> +
> +config AMD_HSMP
> +	tristate "AMD HSMP Driver"
> +	depends on AMD_NB && X86_64
> +	help
> +	  The driver provides a way for user space tools to monitor and manage
> +	  system management functionality on EPYC server CPUs from AMD.
> +
> +	  Host System Management Port (HSMP) interface is a mailbox interface
> +	  between the x86 core and the System Management Unit (SMU) firmware.
> +
> +	  If you choose to compile this driver as a module the module will be
> +	  called amd_hsmp.
> diff --git a/drivers/platform/x86/amd/hsmp/Makefile b/drivers/platform/x86/amd/hsmp/Makefile
> new file mode 100644
> index 000000000000..0e5e4cc68145
> --- /dev/null
> +++ b/drivers/platform/x86/amd/hsmp/Makefile
> @@ -0,0 +1,9 @@
> +# SPDX-License-Identifier: GPL-2.0
> +#
> +# Makefile for linux/drivers/platform/x86/amd/hsmp
> +# AMD Host System Management Port
> +#
> +
> +amd_hsmp-y			:= hsmp.o
> +amd_hsmp-$(CONFIG_ACPI)		+= hsmp-acpi.o
> +obj-$(CONFIG_AMD_HSMP)		+= amd_hsmp.o
> diff --git a/drivers/platform/x86/amd/hsmp/hsmp-acpi.c b/drivers/platform/x86/amd/hsmp/hsmp-acpi.c
> new file mode 100644
> index 000000000000..90aa1bdbf7f1
> --- /dev/null
> +++ b/drivers/platform/x86/amd/hsmp/hsmp-acpi.c
> @@ -0,0 +1,219 @@
> +// SPDX-License-Identifier: GPL-2.0
> +/*
> + * AMD HSMP Platform Driver
> + * Copyright (c) 2024, AMD.
> + * All Rights Reserved.
> + *
> + * This file provides ACPI parsing implementation for HSMP interface
> + */
> +
> +#include "hsmp.h"
> +
> +/* These are the strings specified in ACPI table */
> +#define MSG_IDOFF_STR           "MsgIdOffset"
> +#define MSG_ARGOFF_STR          "MsgArgOffset"
> +#define MSG_RESPOFF_STR         "MsgRspOffset"
> +
> +/* This is the UUID used for HSMP */
> +static const guid_t acpi_hsmp_uuid = GUID_INIT(0xb74d619d, 0x5707, 0x48bd,
> +						0xa6, 0x9f, 0x4e, 0xa2,
> +						0x87, 0x1f, 0xc2, 0xf6);
> +
> +static inline bool is_acpi_hsmp_uuid(union acpi_object *obj)
> +{
> +	if (obj->type == ACPI_TYPE_BUFFER && obj->buffer.length == UUID_SIZE)
> +		return guid_equal((guid_t *)obj->buffer.pointer, &acpi_hsmp_uuid);
> +
> +	return false;
> +}
> +
> +static acpi_status hsmp_resource(struct acpi_resource *res, void *data)
> +{
> +	struct hsmp_socket *sock = data;
> +	struct resource r;
> +
> +	switch (res->type) {
> +	case ACPI_RESOURCE_TYPE_FIXED_MEMORY32:
> +		if (!acpi_dev_resource_memory(res, &r))
> +			return AE_ERROR;
> +		if (!r.start || r.end < r.start || !(r.flags & IORESOURCE_MEM_WRITEABLE))
> +			return AE_ERROR;
> +		sock->mbinfo.base_addr = r.start;
> +		sock->mbinfo.size = resource_size(&r);
> +		break;
> +	case ACPI_RESOURCE_TYPE_END_TAG:
> +		break;
> +	default:
> +		return AE_ERROR;
> +	}
> +
> +	return AE_OK;
> +}
> +
> +static int hsmp_read_acpi_crs(struct hsmp_socket *sock)
> +{
> +	acpi_status status;
> +
> +	status = acpi_walk_resources(ACPI_HANDLE(sock->dev), METHOD_NAME__CRS,
> +				     hsmp_resource, sock);
> +	if (ACPI_FAILURE(status)) {
> +		dev_err(sock->dev, "Failed to look up MP1 base address from CRS method, err: %s\n",
> +			acpi_format_exception(status));
> +		return -EINVAL;
> +	}
> +	if (!sock->mbinfo.base_addr || !sock->mbinfo.size)
> +		return -EINVAL;
> +
> +	/* The mapped region should be un cached */
> +	sock->virt_base_addr = devm_ioremap_uc(sock->dev, sock->mbinfo.base_addr,
> +					       sock->mbinfo.size);
> +	if (!sock->virt_base_addr) {
> +		dev_err(sock->dev, "Failed to ioremap MP1 base address\n");
> +		return -ENOMEM;
> +	}
> +
> +	return 0;
> +}
> +
> +static int hsmp_read_acpi_dsd(struct hsmp_socket *sock)
> +{
> +	struct acpi_buffer buf = { ACPI_ALLOCATE_BUFFER, NULL };
> +	union acpi_object *guid, *mailbox_package;
> +	union acpi_object *dsd;
> +	acpi_status status;
> +	int ret = 0;
> +	int j;
> +
> +	status = acpi_evaluate_object_typed(ACPI_HANDLE(sock->dev), "_DSD", NULL,
> +					    &buf, ACPI_TYPE_PACKAGE);
> +	if (ACPI_FAILURE(status)) {
> +		dev_err(sock->dev, "Failed to read mailbox reg offsets from DSD table, err: %s\n",
> +			acpi_format_exception(status));
> +		return -ENODEV;
> +	}
> +
> +	dsd = buf.pointer;
> +
> +	/* HSMP _DSD property should contain 2 objects.
> +	 * 1. guid which is an acpi object of type ACPI_TYPE_BUFFER
> +	 * 2. mailbox which is an acpi object of type ACPI_TYPE_PACKAGE
> +	 *    This mailbox object contains 3 more acpi objects of type
> +	 *    ACPI_TYPE_PACKAGE for holding msgid, msgresp, msgarg offsets
> +	 *    these packages inturn contain 2 acpi objects of type
> +	 *    ACPI_TYPE_STRING and ACPI_TYPE_INTEGER
> +	 */
> +	if (!dsd || dsd->type != ACPI_TYPE_PACKAGE || dsd->package.count != 2) {
> +		ret = -EINVAL;
> +		goto free_buf;
> +	}
> +
> +	guid = &dsd->package.elements[0];
> +	mailbox_package = &dsd->package.elements[1];
> +	if (!is_acpi_hsmp_uuid(guid) || mailbox_package->type != ACPI_TYPE_PACKAGE) {
> +		dev_err(sock->dev, "Invalid hsmp _DSD table data\n");
> +		ret = -EINVAL;
> +		goto free_buf;
> +	}
> +
> +	for (j = 0; j < mailbox_package->package.count; j++) {
> +		union acpi_object *msgobj, *msgstr, *msgint;
> +
> +		msgobj	= &mailbox_package->package.elements[j];
> +		msgstr	= &msgobj->package.elements[0];
> +		msgint	= &msgobj->package.elements[1];
> +
> +		/* package should have 1 string and 1 integer object */
> +		if (msgobj->type != ACPI_TYPE_PACKAGE ||
> +		    msgstr->type != ACPI_TYPE_STRING ||
> +		    msgint->type != ACPI_TYPE_INTEGER) {
> +			ret = -EINVAL;
> +			goto free_buf;
> +		}
> +
> +		if (!strncmp(msgstr->string.pointer, MSG_IDOFF_STR,
> +			     msgstr->string.length)) {
> +			sock->mbinfo.msg_id_off = msgint->integer.value;
> +		} else if (!strncmp(msgstr->string.pointer, MSG_RESPOFF_STR,
> +				    msgstr->string.length)) {
> +			sock->mbinfo.msg_resp_off =  msgint->integer.value;
> +		} else if (!strncmp(msgstr->string.pointer, MSG_ARGOFF_STR,
> +				    msgstr->string.length)) {
> +			sock->mbinfo.msg_arg_off = msgint->integer.value;
> +		} else {
> +			ret = -ENOENT;
> +			goto free_buf;
> +		}
> +	}
> +
> +	if (!sock->mbinfo.msg_id_off || !sock->mbinfo.msg_resp_off ||
> +	    !sock->mbinfo.msg_arg_off)
> +		ret = -EINVAL;
> +
> +free_buf:
> +	ACPI_FREE(buf.pointer);
> +	return ret;
> +}
> +
> +int init_acpi(struct hsmp_plat_device *plat_dev, struct device *dev)
> +{
> +	struct hsmp_socket *sock;
> +	u16 sock_ind;
> +	int ret;
> +
> +	plat_dev->is_acpi_device = true;
> +
> +	ret = hsmp_get_uid(dev, &sock_ind);
> +	if (ret)
> +		return ret;
> +	if (sock_ind >= plat_dev->num_sockets)
> +		return -EINVAL;
> +
> +	sock = &plat_dev->sock[sock_ind];
> +
> +	sock->sock_ind		= sock_ind;
> +	sock->dev		= dev;
> +
> +	sema_init(&sock->hsmp_sem, 1);
> +
> +	/* Read MP1 base address from CRS method */
> +	ret = hsmp_read_acpi_crs(sock);
> +	if (ret)
> +		return ret;
> +
> +	/* Read mailbox offsets from DSD table */
> +	return hsmp_read_acpi_dsd(sock);
> +}
> +
> +int check_acpi_support(struct device *dev, const struct acpi_device_id *amd_hsmp_acpi_ids)
> +{
> +	struct acpi_device *adev;
> +
> +	adev = ACPI_COMPANION(dev);
> +	if (adev && !acpi_match_device_ids(adev, amd_hsmp_acpi_ids))
> +		return 0;
> +
> +	return -ENODEV;
> +}
> +
> +int hsmp_get_uid(struct device *dev, u16 *sock_ind)
> +{
> +	char *uid;
> +
> +	/*
> +	 * UID (ID00, ID01..IDXX) is used for differentiating sockets,
> +	 * read it and strip the "ID" part of it and convert the remaining
> +	 * bytes to integer.
> +	 */
> +	uid = acpi_device_uid(ACPI_COMPANION(dev));
> +
> +	return kstrtou16(uid + 2, 10, sock_ind);
> +}
> +
> +void amd_hsmp_acpi_rdwr(struct hsmp_socket *sock, u32 offset,
> +			u32 *value, bool write)
> +{
> +	if (write)
> +		iowrite32(*value, sock->virt_base_addr + offset);
> +	else
> +		*value = ioread32(sock->virt_base_addr + offset);
> +}
> diff --git a/drivers/platform/x86/amd/hsmp.c b/drivers/platform/x86/amd/hsmp/hsmp.c
> similarity index 72%
> rename from drivers/platform/x86/amd/hsmp.c
> rename to drivers/platform/x86/amd/hsmp/hsmp.c
> index 807a5066dacc..1df2b34ddbbd 100644
> --- a/drivers/platform/x86/amd/hsmp.c
> +++ b/drivers/platform/x86/amd/hsmp/hsmp.c
> @@ -10,18 +10,10 @@
>   #define pr_fmt(fmt) KBUILD_MODNAME ": " fmt
>   
>   #include <asm/amd_hsmp.h>
> -#include <asm/amd_nb.h>
> -#include <linux/delay.h>
> -#include <linux/io.h>
> -#include <linux/miscdevice.h>
> -#include <linux/module.h>
> -#include <linux/pci.h>
> -#include <linux/platform_device.h>
> -#include <linux/semaphore.h>
> -#include <linux/acpi.h>
> +#include "hsmp.h"
>   
>   #define DRIVER_NAME		"amd_hsmp"
> -#define DRIVER_VERSION		"2.2"
> +#define DRIVER_VERSION		"2.3"
>   #define ACPI_HSMP_DEVICE_HID	"AMDI0097"
>   
>   /* HSMP Status / Error codes */
> @@ -55,44 +47,8 @@
>   #define HSMP_DEVNODE_NAME	"hsmp"
>   #define HSMP_METRICS_TABLE_NAME	"metrics_bin"
>   
> -#define HSMP_ATTR_GRP_NAME_SIZE	10
> -
> -/* These are the strings specified in ACPI table */
> -#define MSG_IDOFF_STR		"MsgIdOffset"
> -#define MSG_ARGOFF_STR		"MsgArgOffset"
> -#define MSG_RESPOFF_STR		"MsgRspOffset"
> -
>   #define MAX_AMD_SOCKETS 8
>   
> -struct hsmp_mbaddr_info {
> -	u32 base_addr;
> -	u32 msg_id_off;
> -	u32 msg_resp_off;
> -	u32 msg_arg_off;
> -	u32 size;
> -};
> -
> -struct hsmp_socket {
> -	struct bin_attribute hsmp_attr;
> -	struct hsmp_mbaddr_info mbinfo;
> -	void __iomem *metric_tbl_addr;
> -	void __iomem *virt_base_addr;
> -	struct semaphore hsmp_sem;
> -	char name[HSMP_ATTR_GRP_NAME_SIZE];
> -	struct pci_dev *root;
> -	struct device *dev;
> -	u16 sock_ind;
> -};
> -
> -struct hsmp_plat_device {
> -	struct miscdevice hsmp_device;
> -	struct hsmp_socket *sock;
> -	u32 proto_ver;
> -	u16 num_sockets;
> -	bool is_acpi_device;
> -	bool is_probed;
> -};
> -
>   static struct hsmp_plat_device plat_dev;
>   
>   static int amd_hsmp_pci_rdwr(struct hsmp_socket *sock, u32 offset,
> @@ -114,15 +70,6 @@ static int amd_hsmp_pci_rdwr(struct hsmp_socket *sock, u32 offset,
>   	return ret;
>   }
>   
> -static void amd_hsmp_acpi_rdwr(struct hsmp_socket *sock, u32 offset,
> -			       u32 *value, bool write)
> -{
> -	if (write)
> -		iowrite32(*value, sock->virt_base_addr + offset);
> -	else
> -		*value = ioread32(sock->virt_base_addr + offset);
> -}
> -
>   static int amd_hsmp_rdwr(struct hsmp_socket *sock, u32 offset,
>   			 u32 *value, bool write)
>   {
> @@ -321,6 +268,22 @@ static int hsmp_test(u16 sock_ind, u32 value)
>   	return ret;
>   }
>   
> +static int hsmp_cache_proto_ver(u16 sock_ind)
> +{
> +	struct hsmp_message msg = { 0 };
> +	int ret;
> +
> +	msg.msg_id	= HSMP_GET_PROTO_VER;
> +	msg.sock_ind	= sock_ind;
> +	msg.response_sz = hsmp_msg_desc_table[HSMP_GET_PROTO_VER].response_sz;
> +
> +	ret = hsmp_send_message(&msg);
> +	if (!ret)
> +		plat_dev.proto_ver = msg.args[0];
> +
> +	return ret;
> +}
> +
>   static long hsmp_ioctl(struct file *fp, unsigned int cmd, unsigned long arg)
>   {
>   	int __user *arguser = (int  __user *)arg;
> @@ -383,181 +346,6 @@ static const struct file_operations hsmp_fops = {
>   	.compat_ioctl	= hsmp_ioctl,
>   };
>   
> -/* This is the UUID used for HSMP */
> -static const guid_t acpi_hsmp_uuid = GUID_INIT(0xb74d619d, 0x5707, 0x48bd,
> -						0xa6, 0x9f, 0x4e, 0xa2,
> -						0x87, 0x1f, 0xc2, 0xf6);
> -
> -static inline bool is_acpi_hsmp_uuid(union acpi_object *obj)
> -{
> -	if (obj->type == ACPI_TYPE_BUFFER && obj->buffer.length == UUID_SIZE)
> -		return guid_equal((guid_t *)obj->buffer.pointer, &acpi_hsmp_uuid);
> -
> -	return false;
> -}
> -
> -static inline int hsmp_get_uid(struct device *dev, u16 *sock_ind)
> -{
> -	char *uid;
> -
> -	/*
> -	 * UID (ID00, ID01..IDXX) is used for differentiating sockets,
> -	 * read it and strip the "ID" part of it and convert the remaining
> -	 * bytes to integer.
> -	 */
> -	uid = acpi_device_uid(ACPI_COMPANION(dev));
> -
> -	return kstrtou16(uid + 2, 10, sock_ind);
> -}
> -
> -static acpi_status hsmp_resource(struct acpi_resource *res, void *data)
> -{
> -	struct hsmp_socket *sock = data;
> -	struct resource r;
> -
> -	switch (res->type) {
> -	case ACPI_RESOURCE_TYPE_FIXED_MEMORY32:
> -		if (!acpi_dev_resource_memory(res, &r))
> -			return AE_ERROR;
> -		if (!r.start || r.end < r.start || !(r.flags & IORESOURCE_MEM_WRITEABLE))
> -			return AE_ERROR;
> -		sock->mbinfo.base_addr = r.start;
> -		sock->mbinfo.size = resource_size(&r);
> -		break;
> -	case ACPI_RESOURCE_TYPE_END_TAG:
> -		break;
> -	default:
> -		return AE_ERROR;
> -	}
> -
> -	return AE_OK;
> -}
> -
> -static int hsmp_read_acpi_dsd(struct hsmp_socket *sock)
> -{
> -	struct acpi_buffer buf = { ACPI_ALLOCATE_BUFFER, NULL };
> -	union acpi_object *guid, *mailbox_package;
> -	union acpi_object *dsd;
> -	acpi_status status;
> -	int ret = 0;
> -	int j;
> -
> -	status = acpi_evaluate_object_typed(ACPI_HANDLE(sock->dev), "_DSD", NULL,
> -					    &buf, ACPI_TYPE_PACKAGE);
> -	if (ACPI_FAILURE(status)) {
> -		dev_err(sock->dev, "Failed to read mailbox reg offsets from DSD table, err: %s\n",
> -			acpi_format_exception(status));
> -		return -ENODEV;
> -	}
> -
> -	dsd = buf.pointer;
> -
> -	/* HSMP _DSD property should contain 2 objects.
> -	 * 1. guid which is an acpi object of type ACPI_TYPE_BUFFER
> -	 * 2. mailbox which is an acpi object of type ACPI_TYPE_PACKAGE
> -	 *    This mailbox object contains 3 more acpi objects of type
> -	 *    ACPI_TYPE_PACKAGE for holding msgid, msgresp, msgarg offsets
> -	 *    these packages inturn contain 2 acpi objects of type
> -	 *    ACPI_TYPE_STRING and ACPI_TYPE_INTEGER
> -	 */
> -	if (!dsd || dsd->type != ACPI_TYPE_PACKAGE || dsd->package.count != 2) {
> -		ret = -EINVAL;
> -		goto free_buf;
> -	}
> -
> -	guid = &dsd->package.elements[0];
> -	mailbox_package = &dsd->package.elements[1];
> -	if (!is_acpi_hsmp_uuid(guid) || mailbox_package->type != ACPI_TYPE_PACKAGE) {
> -		dev_err(sock->dev, "Invalid hsmp _DSD table data\n");
> -		ret = -EINVAL;
> -		goto free_buf;
> -	}
> -
> -	for (j = 0; j < mailbox_package->package.count; j++) {
> -		union acpi_object *msgobj, *msgstr, *msgint;
> -
> -		msgobj	= &mailbox_package->package.elements[j];
> -		msgstr	= &msgobj->package.elements[0];
> -		msgint	= &msgobj->package.elements[1];
> -
> -		/* package should have 1 string and 1 integer object */
> -		if (msgobj->type != ACPI_TYPE_PACKAGE ||
> -		    msgstr->type != ACPI_TYPE_STRING ||
> -		    msgint->type != ACPI_TYPE_INTEGER) {
> -			ret = -EINVAL;
> -			goto free_buf;
> -		}
> -
> -		if (!strncmp(msgstr->string.pointer, MSG_IDOFF_STR,
> -			     msgstr->string.length)) {
> -			sock->mbinfo.msg_id_off = msgint->integer.value;
> -		} else if (!strncmp(msgstr->string.pointer, MSG_RESPOFF_STR,
> -				    msgstr->string.length)) {
> -			sock->mbinfo.msg_resp_off =  msgint->integer.value;
> -		} else if (!strncmp(msgstr->string.pointer, MSG_ARGOFF_STR,
> -				    msgstr->string.length)) {
> -			sock->mbinfo.msg_arg_off = msgint->integer.value;
> -		} else {
> -			ret = -ENOENT;
> -			goto free_buf;
> -		}
> -	}
> -
> -	if (!sock->mbinfo.msg_id_off || !sock->mbinfo.msg_resp_off ||
> -	    !sock->mbinfo.msg_arg_off)
> -		ret = -EINVAL;
> -
> -free_buf:
> -	ACPI_FREE(buf.pointer);
> -	return ret;
> -}
> -
> -static int hsmp_read_acpi_crs(struct hsmp_socket *sock)
> -{
> -	acpi_status status;
> -
> -	status = acpi_walk_resources(ACPI_HANDLE(sock->dev), METHOD_NAME__CRS,
> -				     hsmp_resource, sock);
> -	if (ACPI_FAILURE(status)) {
> -		dev_err(sock->dev, "Failed to look up MP1 base address from CRS method, err: %s\n",
> -			acpi_format_exception(status));
> -		return -EINVAL;
> -	}
> -	if (!sock->mbinfo.base_addr || !sock->mbinfo.size)
> -		return -EINVAL;
> -
> -	/* The mapped region should be un cached */
> -	sock->virt_base_addr = devm_ioremap_uc(sock->dev, sock->mbinfo.base_addr,
> -					       sock->mbinfo.size);
> -	if (!sock->virt_base_addr) {
> -		dev_err(sock->dev, "Failed to ioremap MP1 base address\n");
> -		return -ENOMEM;
> -	}
> -
> -	return 0;
> -}
> -
> -/* Parse the ACPI table to read the data */
> -static int hsmp_parse_acpi_table(struct device *dev, u16 sock_ind)
> -{
> -	struct hsmp_socket *sock = &plat_dev.sock[sock_ind];
> -	int ret;
> -
> -	sock->sock_ind		= sock_ind;
> -	sock->dev		= dev;
> -	plat_dev.is_acpi_device	= true;
> -
> -	sema_init(&sock->hsmp_sem, 1);
> -
> -	/* Read MP1 base address from CRS method */
> -	ret = hsmp_read_acpi_crs(sock);
> -	if (ret)
> -		return ret;
> -
> -	/* Read mailbox offsets from DSD table */
> -	return hsmp_read_acpi_dsd(sock);
> -}
> -
>   static ssize_t hsmp_metric_tbl_read(struct file *filp, struct kobject *kobj,
>   				    struct bin_attribute *bin_attr, char *buf,
>   				    loff_t off, size_t count)
> @@ -670,8 +458,15 @@ static int hsmp_create_non_acpi_sysfs_if(struct device *dev)
>   {
>   	const struct attribute_group **hsmp_attr_grps;
>   	struct attribute_group *attr_grp;
> +	int ret;
>   	u16 i;
>   
> +	ret = hsmp_cache_proto_ver(0);
> +	if (ret) {
> +		dev_err(dev, "Failed to read HSMP protocol version\n");
> +		return ret;
> +	}
> +
>   	hsmp_attr_grps = devm_kcalloc(dev, plat_dev.num_sockets + 1,
>   				      sizeof(*hsmp_attr_grps),
>   				      GFP_KERNEL);
> @@ -706,12 +501,18 @@ static int hsmp_create_acpi_sysfs_if(struct device *dev)
>   	if (!attr_grp)
>   		return -ENOMEM;
>   
> -	attr_grp->is_bin_visible = hsmp_is_sock_attr_visible;
> -
>   	ret = hsmp_get_uid(dev, &sock_ind);
>   	if (ret)
>   		return ret;
>   
> +	ret = hsmp_cache_proto_ver(sock_ind);
> +	if (ret) {
> +		dev_err(dev, "Failed to read HSMP protocol version\n");
> +		return ret;
> +	}
> +
> +	attr_grp->is_bin_visible = hsmp_is_sock_attr_visible;
> +
>   	ret = hsmp_create_attr_list(attr_grp, dev, sock_ind);
>   	if (ret)
>   		return ret;
> @@ -719,22 +520,6 @@ static int hsmp_create_acpi_sysfs_if(struct device *dev)
>   	return devm_device_add_group(dev, attr_grp);
>   }
>   
> -static int hsmp_cache_proto_ver(u16 sock_ind)
> -{
> -	struct hsmp_message msg = { 0 };
> -	int ret;
> -
> -	msg.msg_id	= HSMP_GET_PROTO_VER;
> -	msg.sock_ind	= sock_ind;
> -	msg.response_sz = hsmp_msg_desc_table[HSMP_GET_PROTO_VER].response_sz;
> -
> -	ret = hsmp_send_message(&msg);
> -	if (!ret)
> -		plat_dev.proto_ver = msg.args[0];
> -
> -	return ret;
> -}
> -
>   static inline bool is_f1a_m0h(void)
>   {
>   	if (boot_cpu_data.x86 == 0x1A && boot_cpu_data.x86_model <= 0x0F)
> @@ -791,8 +576,6 @@ MODULE_DEVICE_TABLE(acpi, amd_hsmp_acpi_ids);
>   
>   static int hsmp_pltdrv_probe(struct platform_device *pdev)
>   {
> -	struct acpi_device *adev;
> -	u16 sock_ind = 0;
>   	int ret;
>   
>   	/*
> @@ -809,46 +592,33 @@ static int hsmp_pltdrv_probe(struct platform_device *pdev)
>   		if (!plat_dev.sock)
>   			return -ENOMEM;
>   	}
> -	adev = ACPI_COMPANION(&pdev->dev);
> -	if (adev && !acpi_match_device_ids(adev, amd_hsmp_acpi_ids)) {
> -		ret = hsmp_get_uid(&pdev->dev, &sock_ind);
> -		if (ret)
> -			return ret;
> -		if (sock_ind >= plat_dev.num_sockets)
> -			return -EINVAL;
> -		ret = hsmp_parse_acpi_table(&pdev->dev, sock_ind);
> +
> +	ret = check_acpi_support(&pdev->dev, amd_hsmp_acpi_ids);
> +	if (!ret) {
> +		ret = init_acpi(&plat_dev, &pdev->dev);
>   		if (ret) {
> -			dev_err(&pdev->dev, "Failed to parse ACPI table\n");
> +			dev_err(&pdev->dev, "Failed to initialize HSMP interface.\n");
>   			return ret;
>   		}
> -		/* Test the hsmp interface */
> -		ret = hsmp_test(sock_ind, 0xDEADBEEF);
> +
> +		ret = hsmp_create_acpi_sysfs_if(&pdev->dev);
>   		if (ret) {
> -			dev_err(&pdev->dev, "HSMP test message failed on Fam:%x model:%x\n",
> -				boot_cpu_data.x86, boot_cpu_data.x86_model);
> -			dev_err(&pdev->dev, "Is HSMP disabled in BIOS ?\n");
> +			dev_err(&pdev->dev, "Failed to create HSMP sysfs interface\n");
>   			return ret;
>   		}
>   	} else {
>   		ret = init_platform_device(&pdev->dev);
>   		if (ret) {
> -			dev_err(&pdev->dev, "Failed to init HSMP mailbox\n");
> +			dev_err(&pdev->dev, "Failed to initialize HSMP interface.\n");
>   			return ret;
>   		}
> -	}
> -
> -	ret = hsmp_cache_proto_ver(sock_ind);
> -	if (ret) {
> -		dev_err(&pdev->dev, "Failed to read HSMP protocol version\n");
> -		return ret;
> -	}
>   
> -	if (plat_dev.is_acpi_device)
> -		ret = hsmp_create_acpi_sysfs_if(&pdev->dev);
> -	else
>   		ret = hsmp_create_non_acpi_sysfs_if(&pdev->dev);
> -	if (ret)
> -		dev_err(&pdev->dev, "Failed to create HSMP sysfs interface\n");
> +		if (ret) {
> +			dev_err(&pdev->dev, "Failed to create HSMP sysfs interface\n");
> +			return ret;
> +		}
> +	}
>   
>   	if (!plat_dev.is_probed) {
>   		plat_dev.hsmp_device.name	= HSMP_CDEV_NAME;
> diff --git a/drivers/platform/x86/amd/hsmp/hsmp.h b/drivers/platform/x86/amd/hsmp/hsmp.h
> new file mode 100644
> index 000000000000..23629ccb1fb0
> --- /dev/null
> +++ b/drivers/platform/x86/amd/hsmp/hsmp.h
> @@ -0,0 +1,82 @@
> +/* SPDX-License-Identifier: GPL-2.0 */
> +/*
> + * AMD HSMP Platform Driver
> + * Copyright (c) 2024, AMD.
> + * All Rights Reserved.
> + *
> + * This file provides a header implementation for HSMP interface
> + */
> +
> +#ifndef _HSMP_H
> +#define _HSMP_H
> +
> +#include <asm/amd_nb.h>
> +#include <linux/delay.h>
> +#include <linux/io.h>
> +#include <linux/module.h>
> +#include <linux/pci.h>
> +#include <linux/platform_device.h>
> +#include <linux/semaphore.h>
> +#include <linux/acpi.h>
> +#include <linux/miscdevice.h>
> +
> +#define HSMP_ATTR_GRP_NAME_SIZE 10
> +
> +struct hsmp_mbaddr_info {
> +	u32 base_addr;
> +	u32 msg_id_off;
> +	u32 msg_resp_off;
> +	u32 msg_arg_off;
> +	u32 size;
> +};
> +
> +struct hsmp_socket {
> +	struct bin_attribute hsmp_attr;
> +	struct hsmp_mbaddr_info mbinfo;
> +	void __iomem *metric_tbl_addr;
> +	void __iomem *virt_base_addr;
> +	struct semaphore hsmp_sem;
> +	char name[HSMP_ATTR_GRP_NAME_SIZE];
> +	struct pci_dev *root;
> +	struct device *dev;
> +	u16 sock_ind;
> +};
> +
> +struct hsmp_plat_device {
> +	struct miscdevice hsmp_device;
> +	struct hsmp_socket *sock;
> +	u32 proto_ver;
> +	u16 num_sockets;
> +	bool is_acpi_device;
> +	bool is_probed;
> +};
> +
> +#ifdef CONFIG_ACPI
> +int hsmp_get_uid(struct device *dev, u16 *sock_ind);
> +void amd_hsmp_acpi_rdwr(struct hsmp_socket *sock, u32 offset,
> +			u32 *value, bool write);
> +int check_acpi_support(struct device *dev, const struct acpi_device_id *amd_hsmp_acpi_ids);
> +int init_acpi(struct hsmp_plat_device *plat_dev, struct device *dev);
> +#else
> +static inline void amd_hsmp_acpi_rdwr(struct hsmp_socket *sock, u32 offset,
> +				      u32 *value, bool write)
> +{
> +}
> +
> +static inline int hsmp_get_uid(struct device *dev, u16 *sock_ind)
> +{
> +	return -ENODEV;
> +}
> +
> +static inline int check_acpi_support(struct device *dev,
> +				     const struct acpi_device_id *amd_hsmp_acpi_ids)
> +{
> +	return -ENODEV;
> +}
> +
> +static inline int init_acpi(struct hsmp_plat_device *plat_dev, struct device *dev)
> +{
> +	return -ENODEV;
> +}
> +#endif
> +#endif

Re: [PATCH 3/3] platform/x86/amd/hsmp: Make HSMP as default m

[Mario Limonciello]

On 4/23/2024 04:14, Suma Hegde wrote:
> Making HSMP as "default m" in Kconfig, enables customers
> who rely on the standard distros to use the HSMP by dynamically
> loading it.
> 
> Signed-off-by: Suma Hegde <suma.hegde@amd.com>
> Reviewed-by: Naveen Krishna Chatradhi <naveenkrishna.chatradhi@amd.com>
> ---
>   drivers/platform/x86/amd/hsmp/Kconfig | 1 +
>   1 file changed, 1 insertion(+)
> 
> diff --git a/drivers/platform/x86/amd/hsmp/Kconfig b/drivers/platform/x86/amd/hsmp/Kconfig
> index 26096f901c22..9d3f422b3f81 100644
> --- a/drivers/platform/x86/amd/hsmp/Kconfig
> +++ b/drivers/platform/x86/amd/hsmp/Kconfig
> @@ -6,6 +6,7 @@
>   config AMD_HSMP
>   	tristate "AMD HSMP Driver"
>   	depends on AMD_NB && X86_64
> +	default m

I don't think Linus likes to see things default to 'm'.  I don't see 
anything in the platform-x86 subsystem that is defaulted 'm'.

IMO It's better to request the distros to enable it.

>   	help
>   	  The driver provides a way for user space tools to monitor and manage
>   	  system management functionality on EPYC server CPUs from AMD.

Re: [PATCH 3/3] platform/x86/amd/hsmp: Make HSMP as default m

[Hegde, Suma]

Hi Mario,

On 4/23/2024 10:47 PM, Mario Limonciello wrote:
> On 4/23/2024 04:14, Suma Hegde wrote:
>> Making HSMP as "default m" in Kconfig, enables customers
>> who rely on the standard distros to use the HSMP by dynamically
>> loading it.
>>
>> Signed-off-by: Suma Hegde <suma.hegde@amd.com>
>> Reviewed-by: Naveen Krishna Chatradhi <naveenkrishna.chatradhi@amd.com>
>> ---
>>   drivers/platform/x86/amd/hsmp/Kconfig | 1 +
>>   1 file changed, 1 insertion(+)
>>
>> diff --git a/drivers/platform/x86/amd/hsmp/Kconfig 
>> b/drivers/platform/x86/amd/hsmp/Kconfig
>> index 26096f901c22..9d3f422b3f81 100644
>> --- a/drivers/platform/x86/amd/hsmp/Kconfig
>> +++ b/drivers/platform/x86/amd/hsmp/Kconfig
>> @@ -6,6 +6,7 @@
>>   config AMD_HSMP
>>       tristate "AMD HSMP Driver"
>>       depends on AMD_NB && X86_64
>> +    default m
>
> I don't think Linus likes to see things default to 'm'.  I don't see 
> anything in the platform-x86 subsystem that is defaulted 'm'.
>
> IMO It's better to request the distros to enable it.
>
Ok, then we will request the distros to enable it.

Hans, request you to drop this patch.

>>       help
>>         The driver provides a way for user space tools to monitor and 
>> manage
>>         system management functionality on EPYC server CPUs from AMD.
>
Thanks and Regards,

Suma

Re: [PATCH 1/3] platform/x86/amd/hsmp: Check HSMP support on AMD family of processors

[Hegde, Suma]

Hi Mario,

On 4/23/2024 10:41 PM, Mario Limonciello wrote:
> On 4/23/2024 04:14, Suma Hegde wrote:
>> HSMP interface is supported only on few x86 processors from AMD.
>> Accessing HSMP registers on rest of the platforms might cause
>> unexpected behaviour. So add a check.
>>
>> Also unavailability of this interface on rest of the processors
>> is not an error. Hence, use pr_info() instead of the pr_err() to
>> log the message.
>>
>> Signed-off-by: Suma Hegde <suma.hegde@amd.com>
>> Reviewed-by: Naveen Krishna Chatradhi <naveenkrishna.chatradhi@amd.com>
>> ---
>>   drivers/platform/x86/amd/hsmp.c | 54 ++++++++++++++++++++++++++++-----
>>   1 file changed, 47 insertions(+), 7 deletions(-)
>>
>> diff --git a/drivers/platform/x86/amd/hsmp.c 
>> b/drivers/platform/x86/amd/hsmp.c
>> index 1927be901108..807a5066dacc 100644
>> --- a/drivers/platform/x86/amd/hsmp.c
>> +++ b/drivers/platform/x86/amd/hsmp.c
>> @@ -907,16 +907,48 @@ static int hsmp_plat_dev_register(void)
>>       return ret;
>>   }
>>   +/*
>> + * This check is only needed for backward compatibility of previous 
>> platforms.
>> + * All new platforms are expected to support ACPI based probing.
>> + */
>> +static bool is_hsmp_supported(void)
>
> I think it's better to use "legacy" in this function name to make it 
> clear that this isn't a requirement for newer platforms going forward. 
> Maybe like
>
> legacy_hsmp_support()?
Ok. I will rename it.
>
>> +{
>> +    bool ret = true;
>
> As this is a simple boolean return with no needing to clear memory, I 
> think you can just "return true" or "return false" everywhere in this 
> function.
Ok, I will check this.
>
>> +
>> +    if (boot_cpu_data.x86_vendor != X86_VENDOR_AMD)
>> +        return false;
>> +
>> +    switch (boot_cpu_data.x86) {
>> +    case 0x19:
>> +        switch (boot_cpu_data.x86_model) {
>> +        case 0x00 ... 0x1F:
>> +        case 0x30 ... 0x3F:
>> +        case 0x90 ... 0x9F:
>> +        case 0xA0 ... 0xAF:
>> +            break;
>> +        default:
>> +            ret = false;
>> +        }
>> +        break;
>> +    case 0x1A:
>> +        switch (boot_cpu_data.x86_model) {
>> +        case 0x00 ... 0x1F:
>> +            break;
>
> There actually are going to be 0x1A platforms that don't use ACPI?
> I guess I'm surprised.
>
Model 0x00-0x1f are transitional platforms in 0x1A family, where both 
ACPI and non-ACPI is supported.
>> +        default:
>> +            ret = false;
>> +        }
>> +        break;
>> +    default:
>> +        ret = false;
>> +    }
>> +
>> +    return ret;
>> +}
>> +
>>   static int __init hsmp_plt_init(void)
>>   {
>>       int ret = -ENODEV;
>>   -    if (boot_cpu_data.x86_vendor != X86_VENDOR_AMD || 
>> boot_cpu_data.x86 < 0x19) {
>> -        pr_err("HSMP is not supported on Family:%x model:%x\n",
>> -               boot_cpu_data.x86, boot_cpu_data.x86_model);
>> -        return ret;
>> -    }
>> -
>>       /*
>>        * amd_nb_num() returns number of SMN/DF interfaces present in 
>> the system
>>        * if we have N SMN/DF interfaces that ideally means N sockets
>> @@ -930,7 +962,15 @@ static int __init hsmp_plt_init(void)
>>           return ret;
>>         if (!plat_dev.is_acpi_device) {
>> -        ret = hsmp_plat_dev_register();
>> +        if (is_hsmp_supported()) {
>> +            /* Not ACPI device, but supports HSMP, register a 
>> plat_dev */
>> +            ret = hsmp_plat_dev_register();
>> +        } else {
>> +            /* Not ACPI, Does not support HSMP */
>> +            pr_info("HSMP is not supported on Family:%x model:%x\n",
>> +                boot_cpu_data.x86, boot_cpu_data.x86_model);
>> +            ret = -ENODEV;
>> +        }
>>           if (ret)
>>               platform_driver_unregister(&amd_hsmp_driver);
>>       }
>
Thanks and Regards,

Suma

Re: [PATCH 0/3] platform/x86/amd/hsmp: ACPI, Non-ACPI code split and other misc changes

[Hans de Goede]

Hi Mario,

On 4/23/24 7:07 PM, Mario Limonciello wrote:
> On 4/23/2024 04:14, Suma Hegde wrote:
>> Patch1:
>> The first patch is to address the concerns mentioned by Limonciello,
>> Mario i.e to address the issue of
>> "Probing registers that don't match the same mailboxes randomly on
>> client or embedded parts might lead to unexpected behaviors".
>>
>> Because of some limitations, instead of the methods suggested by Mario,
>> family check is added to differentiate server socs and client socs.
>> This check is only for backward compatibility, Going forward all
>> systems will have ACPI based probing only.
>>
> 
> Thanks!  Hans, please drop the PM profile patch from your review queue in favor of this series instead.

You mean: "platform/x86/amd: Don't allow HSMP to be loaded on non-server hardware", right ?

now dropped.

Regards,

Hans



> 
>>
>> Patch2:
>> The second patch splits ACPI code into  separate file there by removing the
>> dependency of ACPI in Kconfig.
>> This is based on the suggestion from Ilpo Jarvinen.
>>
>>
>> Patch3:
>> The third patch makes HSMP as "default m" so that anyone who uses standard
>> distros will be able to use HSMP by dynamically loading it.
>>
>>
>> Suma Hegde (3):
>>    platform/x86/amd/hsmp: Check HSMP support on AMD family of processors
>>    platform/x86/amd/hsmp: Split the ACPI and non-ACPI code
>>    platform/x86/amd/hsmp: Make HSMP as default m
>>
>>   MAINTAINERS                                |   2 +-
>>   drivers/platform/x86/amd/Kconfig           |  14 +-
>>   drivers/platform/x86/amd/Makefile          |   3 +-
>>   drivers/platform/x86/amd/hsmp/Kconfig      |  18 +
>>   drivers/platform/x86/amd/hsmp/Makefile     |   9 +
>>   drivers/platform/x86/amd/hsmp/hsmp-acpi.c  | 219 ++++++++++++
>>   drivers/platform/x86/amd/{ => hsmp}/hsmp.c | 378 +++++----------------
>>   drivers/platform/x86/amd/hsmp/hsmp.h       |  82 +++++
>>   8 files changed, 425 insertions(+), 300 deletions(-)
>>   create mode 100644 drivers/platform/x86/amd/hsmp/Kconfig
>>   create mode 100644 drivers/platform/x86/amd/hsmp/Makefile
>>   create mode 100644 drivers/platform/x86/amd/hsmp/hsmp-acpi.c
>>   rename drivers/platform/x86/amd/{ => hsmp}/hsmp.c (71%)
>>   create mode 100644 drivers/platform/x86/amd/hsmp/hsmp.h
>>
>