[RFC 0/2] GENL interface for ACPI _DSM methods

[RFC 0/2] GENL interface for ACPI _DSM methods

[Wathsala Vithanage]

Linux v6.13-rc1 added support for PCIe TPH and direct cache injection.
As already described in the patch set[1] that introduced this feature,
the cache injection in supported hardware allows optimal utilization of
platform resources for specific requests on the PCIe bus. However, the
patch set [1] implements the functionality for usage within the kernel.
But certain user space applications, especially those whose performance
is sensitive to the latency of inbound writes as seen by a CPU core, may
benefit from using this information (E.g., the DPDK cache stashing
feature discussed in RFC [2]). This RFC is an attempt to obtain the PCIe
steering tag information from the kernel to be used by user mode
applications. We understand that there is more than one way to provide
this information. Please review and suggest alternatives if necessary.

The first of the two patches introduced in this RFC attempts to overcome
the kernel-only limitation by providing an API to kernel subsystems to
hook up relevant _DSM methods to a GENL interface. User space
applications can invoke a _DSM hooked up to this interface via the
"acpi-event" GENL family socket, granted they have the minimum
capabilities and message formats demanded by the kernel subsystem that
hooked up the _DSM method. This feature is added by extending the
"acpi-event" GENL family that multicasts ACPI events to the user-space
applications such as acpid.

The second patch of this RFC hooks up the PCIe root-port TLP Processing
Hints (TPH) _DSM to the ACPI GENL interface. User space applications
like [2] can now request the kernel to execute the _DSM on their behalf
and return steering-tag information.

[1] lore.kernel.org/linux-pci/20241002165954.128085-1-wei.huang2@amd.com
[2] inbox.dpdk.org/dev/20241021015246.304431-2-wathsala.vithanage@arm.com

Wathsala Vithanage (2):
  ACPI: Add support for invoking select _DSM methods from user space
  PCI: Add generic netlink interface to TPH _DSM

 drivers/acpi/Makefile                 |   3 +-
 drivers/acpi/{event.c => acpi_genl.c} | 110 ++++++++++++++++++++++-
 drivers/acpi/acpi_genl_dsm.c          |  76 ++++++++++++++++
 drivers/pci/tph.c                     | 121 ++++++++++++++++++++++++++
 include/acpi/acpi_genl.h              |  54 ++++++++++++
 include/linux/acpi.h                  |   1 +
 6 files changed, 360 insertions(+), 5 deletions(-)
 rename drivers/acpi/{event.c => acpi_genl.c} (63%)
 create mode 100644 drivers/acpi/acpi_genl_dsm.c
 create mode 100644 include/acpi/acpi_genl.h

-- 
2.43.0

[RFC PATCH 1/2] ACPI: Add support for invoking select _DSM methods from user space

[Wathsala Vithanage]

Applications that bypass the kernel to access devices can benefit from
the ability to execute ACPI _DSM methods from the user space. This
commit introduces a generic netlink interface for user-space
applications to invoke _DSM methods registered by kernel subsystems for
the user space. User space applications invoke these _DSMs by sending an
ACPI_GENL_CMD_DSM-typed message to an "acpi-event" GENL family socket.
Irrespective of the _DSM being invoked, all messages typed
ACPI_GENL_CMD_DSM must be prefixed by a struct acpi_genl_dsm_id header
to uniquely identify the target _DSM as specified by the ACPI firmware
specification. Reminder of the message is specific to the _DSM being
invoked. Data from the _DSM method will be returned to the application
as a response with the same message header. In addition, the calling
user application needs the minimal capabilities and message lengths
(including the header) specified by the kernel subsystem that registers
the callback for the _DSM method.

The GENL message header for _DSM (acpi_genl_dsm_id header) has the
following format.

 0       1       2       3       4       5       6       7
+-+-+-+-+-+-+-+-+-+-+-+-+-+-+-+-+-+-+-+-+-+-+-+-+-+-+-+-+-+-+-+-+
|                                                               |
+                          _DSM GUID                            +
|                                                               |
+-+-+-+-+-+-+-+-+-+-+-+-+-+-+-+-+-+-+-+-+-+-+-+-+-+-+-+-+-+-+-+-+
|        _DSM REVISION          |     _DSM FUNCTION INDEX       |
+-+-+-+-+-+-+-+-+-+-+-+-+-+-+-+-+-+-+-+-+-+-+-+-+-+-+-+-+-+-+-+-+

Signed-off-by: Wathsala Vithanage <wathsala.vithanage@arm.com>
---
 drivers/acpi/Makefile                 |   3 +-
 drivers/acpi/{event.c => acpi_genl.c} | 110 +++++++++++++++++++++++++-
 drivers/acpi/acpi_genl_dsm.c          |  79 ++++++++++++++++++
 include/acpi/acpi_genl.h              |  61 ++++++++++++++
 include/linux/acpi.h                  |   1 +
 5 files changed, 249 insertions(+), 5 deletions(-)
 rename drivers/acpi/{event.c => acpi_genl.c} (63%)
 create mode 100644 drivers/acpi/acpi_genl_dsm.c
 create mode 100644 include/acpi/acpi_genl.h

diff --git a/drivers/acpi/Makefile b/drivers/acpi/Makefile
index 40208a0f5dfb..4b1db58ff33a 100644
--- a/drivers/acpi/Makefile
+++ b/drivers/acpi/Makefile
@@ -49,7 +49,8 @@ acpi-y				+= acpi_apd.o
 acpi-y				+= acpi_platform.o
 acpi-y				+= acpi_pnp.o
 acpi-y				+= power.o
-acpi-y				+= event.o
+acpi-y				+= acpi_genl.o
+acpi-y				+= acpi_genl_dsm.o
 acpi-y				+= evged.o
 acpi-y				+= sysfs.o
 acpi-y				+= property.o
diff --git a/drivers/acpi/event.c b/drivers/acpi/acpi_genl.c
similarity index 63%
rename from drivers/acpi/event.c
rename to drivers/acpi/acpi_genl.c
index 96a9aaaaf9f7..53703f6b2805 100644
--- a/drivers/acpi/event.c
+++ b/drivers/acpi/acpi_genl.c
@@ -1,6 +1,6 @@
 // SPDX-License-Identifier: GPL-2.0
 /*
- * event.c - exporting ACPI events via procfs
+ * acpi_genl.c - accessing ACPI events and _DSM functions via netlink
  *
  *  Copyright (C) 2001, 2002 Andy Grover <andrew.grover@intel.com>
  *  Copyright (C) 2001, 2002 Paul Diefenbaugh <paul.s.diefenbaugh@intel.com>
@@ -50,7 +50,11 @@ int unregister_acpi_notifier(struct notifier_block *nb)
 EXPORT_SYMBOL(unregister_acpi_notifier);
 
 #ifdef CONFIG_NET
+
+int acpi_genl_dsm_invoke(struct sk_buff *skb, struct genl_info *info);
+
 static unsigned int acpi_event_seqnum;
+
 struct acpi_genl_event {
 	acpi_device_class device_class;
 	char bus_id[15];
@@ -59,17 +63,19 @@ struct acpi_genl_event {
 };
 
 /* attributes of acpi_genl_family */
-enum {
+enum acpi_genl_attr {
 	ACPI_GENL_ATTR_UNSPEC,
 	ACPI_GENL_ATTR_EVENT,	/* ACPI event info needed by user space */
+	ACPI_GENL_ATTR_DSM,	/* User space _DSM execution requests */
 	__ACPI_GENL_ATTR_MAX,
 };
 #define ACPI_GENL_ATTR_MAX (__ACPI_GENL_ATTR_MAX - 1)
 
 /* commands supported by the acpi_genl_family */
-enum {
+enum acpi_genl_cmd {
 	ACPI_GENL_CMD_UNSPEC,
 	ACPI_GENL_CMD_EVENT,	/* kernel->user notifications for ACPI events */
+	ACPI_GENL_CMD_DSM,	/* use<->kernel channel for _DSM execution */
 	__ACPI_GENL_CMD_MAX,
 };
 #define ACPI_GENL_CMD_MAX (__ACPI_GENL_CMD_MAX - 1)
@@ -78,6 +84,19 @@ enum {
 #define ACPI_GENL_VERSION		0x01
 #define ACPI_GENL_MCAST_GROUP_NAME 	"acpi_mc_group"
 
+
+static const struct nla_policy acpi_nla_policy[__ACPI_GENL_ATTR_MAX + 1] = {
+	[ACPI_GENL_ATTR_DSM] = {.type = NLA_BINARY},
+};
+
+static const struct genl_ops acpi_genl_ops[] = {
+	{
+		.cmd	= ACPI_GENL_CMD_DSM,
+		.policy = acpi_nla_policy,
+		.doit	= acpi_genl_dsm_invoke,
+	},
+};
+
 static const struct genl_multicast_group acpi_event_mcgrps[] = {
 	{ .name = ACPI_GENL_MCAST_GROUP_NAME, },
 };
@@ -87,6 +106,8 @@ static struct genl_family acpi_event_genl_family __ro_after_init = {
 	.name = ACPI_GENL_FAMILY_NAME,
 	.version = ACPI_GENL_VERSION,
 	.maxattr = ACPI_GENL_ATTR_MAX,
+	.ops = acpi_genl_ops,
+	.n_ops = ARRAY_SIZE(acpi_genl_ops),
 	.mcgrps = acpi_event_mcgrps,
 	.n_mcgrps = ARRAY_SIZE(acpi_event_mcgrps),
 };
@@ -144,11 +165,92 @@ int acpi_bus_generate_netlink_event(const char *device_class,
 
 EXPORT_SYMBOL(acpi_bus_generate_netlink_event);
 
+int acpi_genl_dsm_invoke(struct sk_buff *skb, struct genl_info *info)
+{
+	int ret = 0;
+	struct acpi_genl_dsm_id *dsm_arg = NULL, *dsm_ret = NULL;
+	u16 dsm_arg_len = 0;
+	struct acpi_genl_dsm_handle *handle;
+	struct sk_buff *reply_skb;
+	struct nlattr *attr;
+	void *hdr;
+
+	if (!info->attrs[ACPI_GENL_ATTR_DSM]) {
+		ret = -EINVAL;
+		goto out;
+	}
+
+	dsm_arg = (struct acpi_genl_dsm_id *)
+		   nla_data(info->attrs[ACPI_GENL_ATTR_DSM]);
+
+	/* Get the handle for the requested _DSM method. */
+	handle = acpi_genl_dsm_get_handle(dsm_arg);
+	if (!handle) {
+		ret = -EINVAL;
+		goto out;
+	}
+
+	/*
+	 * Make sure massage length matches the _DSM method argument size
+	 * specified in the _DSM handle registered by the dirver.
+	 */
+	dsm_arg_len = nla_len(info->attrs[ACPI_GENL_ATTR_DSM]);
+	if (dsm_arg_len != handle->arg_len) {
+		ret = -EINVAL;
+		goto out;
+	}
+
+	/*
+	 * Make sure the user-space caller has right capabilities to invoke this
+	 * _DSM handle.
+	 */
+	if (!capable(handle->cap)) {
+		ret = -EACCES;
+		goto out;
+	}
+
+	/*
+	 * Build a response of the size specified in the _DSM method handle.
+	 */
+	reply_skb = genlmsg_new(handle->ret_len, GFP_ATOMIC);
+	if (!skb) {
+		ret = -ENOMEM;
+		goto out;
+	}
+
+	hdr = genlmsg_put_reply(reply_skb, info, &acpi_event_genl_family,
+				0, ACPI_GENL_CMD_DSM);
+	if (!hdr) {
+		nlmsg_free(reply_skb);
+		ret = -EMSGSIZE;
+		goto out;
+	}
+
+	attr = nla_reserve(reply_skb, ACPI_GENL_ATTR_DSM, handle->ret_len);
+	if (!attr) {
+		nlmsg_free(reply_skb);
+		ret = -EMSGSIZE;
+		goto out;
+	}
+
+	dsm_ret = nla_data(attr);
+	memset(dsm_ret, 0, handle->ret_len);
+
+	/*
+	 * Invoke the _DSM method via the driver provided callback and send
+	 * response.
+	 */
+	handle->dsm_cb(dsm_arg, dsm_ret);
+	genlmsg_end(reply_skb, hdr);
+	ret = genlmsg_reply(reply_skb, info);
+out:
+	return ret;
+}
+
 static int __init acpi_event_genetlink_init(void)
 {
 	return genl_register_family(&acpi_event_genl_family);
 }
-
 #else
 int acpi_bus_generate_netlink_event(const char *device_class,
 				      const char *bus_id,
diff --git a/drivers/acpi/acpi_genl_dsm.c b/drivers/acpi/acpi_genl_dsm.c
new file mode 100644
index 000000000000..ae6e3656dad8
--- /dev/null
+++ b/drivers/acpi/acpi_genl_dsm.c
@@ -0,0 +1,79 @@
+// SPDX-License-Identifier: GPL-2.0
+/*
+ * acpi_genl_dsm.c - exports ACPI _DSM functions via netlink
+ *
+ *  Copyright (C) 2024  Wathsala Vithanage <wathsala.vithanage@arm.com>
+ *
+ */
+
+#include <linux/acpi.h>
+#include <linux/jhash.h>
+#include <linux/hashtable.h>
+
+#include "internal.h"
+
+#define acpi_genl_dsm_hash_key(id)				\
+	jhash2((u32 *)&(id), sizeof((id))/sizeof(u32), 0)
+
+DEFINE_HASHTABLE(acpi_dsm_tbl, 4);
+
+/**
+ * acpi_genl_dsm_add_handle - add netlink handle to invoke _DSM via netlink.
+ * @dsm_handle: _DSM handle
+ *
+ * Add handle to a _DSM method that will be invoked when user space calls it
+ * via netlink.
+ */
+int acpi_genl_dsm_add_handle(struct acpi_genl_dsm_handle *handle)
+{
+	if (!handle)
+		return -EINVAL;
+	u32 id_hash = acpi_genl_dsm_hash_key(handle->id);
+
+	hash_add_rcu(acpi_dsm_tbl, &handle->entry, id_hash);
+	return 0;
+}
+EXPORT_SYMBOL(acpi_genl_dsm_add_handle);
+
+/**
+ * acpi_genl_dsm_get_handle - find netlink handle to a _DSM method.
+ * @id: _DSM identifier
+ *
+ * Find netlink handle to _DSM method by _DSM identifier.
+ */
+struct acpi_genl_dsm_handle *
+acpi_genl_dsm_get_handle(const struct acpi_genl_dsm_id *id)
+{
+	struct acpi_genl_dsm_handle *cur_obj;
+
+	if (!id)
+		return NULL;
+	u32 id_hash = acpi_genl_dsm_hash_key(*id);
+
+	hash_for_each_possible_rcu(acpi_dsm_tbl, cur_obj, entry, id_hash) {
+		if (!memcmp(cur_obj, id, sizeof(struct acpi_genl_dsm_id)))
+			return cur_obj;
+	}
+	return NULL;
+}
+EXPORT_SYMBOL(acpi_genl_dsm_get_handle);
+
+/**
+ * acpi_genl_dsm_del_handle - remove netlink handler to a _DSM method.
+ * @id: _DSM identifier
+ *
+ * Remove netlink handler to a _DSM method by _DSM identifier.
+ */
+int acpi_genl_dsm_del_handle(const struct acpi_genl_dsm_id *id)
+{
+	if (!id)
+		return -EINVAL;
+	struct acpi_genl_dsm_handle *handle = acpi_genl_dsm_get_handle(id);
+
+	if (!handle)
+		return -ENOENT;
+	hash_del_rcu(&handle->entry);
+	return 0;
+}
+EXPORT_SYMBOL(acpi_genl_dsm_del_handle);
+
diff --git a/include/acpi/acpi_genl.h b/include/acpi/acpi_genl.h
new file mode 100644
index 000000000000..efce8a1a53b4
--- /dev/null
+++ b/include/acpi/acpi_genl.h
@@ -0,0 +1,61 @@
+/* SPDX-License-Identifier: GPL-2.0 */
+/*
+ * acpi_genl_dsm.c - exports ACPI _DSM functions via netlink
+ *
+ *  Copyright (C) 2024  Wathsala Vithanage <wathsala.vithanage@arm.com>
+ *
+ */
+
+#ifndef __ACPI_GENL_H__
+#define __ACPI_GENL_H__
+
+/* _DSM method identifier */
+struct acpi_genl_dsm_id {
+	guid_t	guid;	/* ACPI _DSM GUID */
+	u64	rev;	/* _DSM method revision ID */
+	u64	func;	/* _DSM method index */
+} __packed;
+
+/* _DSM method handle */
+struct acpi_genl_dsm_handle {
+	struct acpi_genl_dsm_id id;	/* Unique _DSM method identifier */
+	u16 arg_len;	/* Size of arg passed into dsm_fn */
+	u16 ret_len;	/* Size of ret passed into dsm_fn */
+	/*
+	 * Callback to invoke the _DSM method.
+	 */
+	int (*dsm_cb)(struct acpi_genl_dsm_id *arg,
+		      struct acpi_genl_dsm_id *ret);
+	int cap;	/* Min cap for invoking _DSM from the user space */
+	struct hlist_node entry;
+};
+
+#ifdef CONFIG_ACPI
+
+int acpi_genl_dsm_add_handle(struct acpi_genl_dsm_handle *handle);
+
+struct acpi_genl_dsm_handle *
+acpi_genl_dsm_get_handle(const struct acpi_genl_dsm_id *id);
+
+int acpi_genl_dsm_del_handle(const struct acpi_genl_dsm_id *id);
+
+#else
+
+int acpi_genl_dsm_add_handle(const struct acpi_genl_dsm_handle *handle)
+{
+	return -ENOTSUP;
+}
+
+struct acpi_genl_dsm_handle *
+acpi_genl_dsm_get_handle(const struct acpi_genl_dsm_id *id)
+{
+	return NULL;
+}
+
+int acpi_genl_dsm_del_handle(struct acpi_genl_dsm_id *id)
+{
+	return -ENOTSUP;
+}
+
+#endif //CONFIG_ACPI
+#endif //__ACPI_GENL_H__
diff --git a/include/linux/acpi.h b/include/linux/acpi.h
index 6adcd1b92b20..ff15ec320705 100644
--- a/include/linux/acpi.h
+++ b/include/linux/acpi.h
@@ -38,6 +38,7 @@ struct irq_domain_ops;
 #include <acpi/acpi_drivers.h>
 #include <acpi/acpi_io.h>
 #include <asm/acpi.h>
+#include <acpi/acpi_genl.h>
 
 #ifdef CONFIG_ACPI_TABLE_LIB
 #define EXPORT_SYMBOL_ACPI_LIB(x) EXPORT_SYMBOL_NS_GPL(x, "ACPI")
-- 
2.43.0

[RFC PATCH 2/2] PCI: Add generic netlink interface to TPH _DSM

[Wathsala Vithanage]

Applications capable of configuring direct cache injection for PCIe
devices from the user space require steering tags extracted from the
PCIe root port’s TPH _DSM method. This patch exposes the TPH _DSM method
via the ACPI GENL interface for such applications. The messages sent
over the GNEL interface querying PCIe steering tags should be prefixed
with an acpi_genl_dsm_id header and PCI domain, slot, and function
information, as shown in the message format diagram. PKG ARG0, ARG1, and
ARG2 correspond to input structures defined in PCIe firmware
specification ECN titled "Revised _DSM for Cache Locality TPH Features."

    0       1       2       3       4       5       6       7
   +-+-+-+-+-+-+-+-+-+-+-+-+-+-+-+-+-+-+-+-+-+-+-+-+-+-+-+-+-+-+-+-+
   |                                                               |
   +                     acpi_genl_dsm_id header                   +
   |                                                               |
   +-+-+-+-+-+-+-+-+-+-+-+-+-+-+-+-+-+-+-+-+-+-+-+-+-+-+-+-+-+-+-+-+
16 |           PCI DOMAIN          |           PCI SLOT            |
   +-+-+-+-+-+-+-+-+-+-+-+-+-+-+-+-+-+-+-+-+-+-+-+-+-+-+-+-+-+-+-+-+
   |           PCI DEVFN           |           PKG ARG0            |
   +-+-+-+-+-+-+-+-+-+-+-+-+-+-+-+-+-+-+-+-+-+-+-+-+-+-+-+-+-+-+-+-+
   |           PKG ARG1            |           PKG ARG2            |
   +-+-+-+-+-+-+-+-+-+-+-+-+-+-+-+-+-+-+-+-+-+-+-+-+-+-+-+-+-+-+-+-+
   |                               |
   +-+-+-+-+-+-+-+-+-+-+-+-+-+-+-+-+

Kernel responds via the same GENL socket with the steering tag info
and a status code in the following format. Steering tag info is the same
structure returned by TPH _DSM of the root port as specified in the PCI
firmware specification ECN titled "Revised _DSM for Cache Locality TPH
Features."

    0       1       2       3       4       5       6       7
   +-+-+-+-+-+-+-+-+-+-+-+-+-+-+-+-+-+-+-+-+-+-+-+-+-+-+-+-+-+-+-+-+
   |                                                               |
   +                     acpi_genl_dsm_id header                   +
   |                                                               |
   +-+-+-+-+-+-+-+-+-+-+-+-+-+-+-+-+-+-+-+-+-+-+-+-+-+-+-+-+-+-+-+-+
16 |            Status             |       Steering Tag Info       |
   +-+-+-+-+-+-+-+-+-+-+-+-+-+-+-+-+-+-+-+-+-+-+-+-+-+-+-+-+-+-+-+-+
   |                               |
   +-+-+-+-+-+-+-+-+-+-+-+-+-+-+-+-+

The status code is always 0 for successful invocations of the _DSM.
Status is -EINVAL, -ENODEV, or -EOPNOTSUPP if ACPI or TPH is disabled,
No PCI device found or TPH is not supported respectively.

Signed-off-by: Wathsala Vithanage <wathsala.vithanage@arm.com>
---
 drivers/pci/tph.c | 119 ++++++++++++++++++++++++++++++++++++++++++++++
 1 file changed, 119 insertions(+)

diff --git a/drivers/pci/tph.c b/drivers/pci/tph.c
index 1e604fbbda65..8a70b663cb78 100644
--- a/drivers/pci/tph.c
+++ b/drivers/pci/tph.c
@@ -53,6 +53,36 @@ union st_info {
 	u64 value;
 };
 
+/*
+ * The tph_genl_dsm_info struct defines both the input and the return container
+ * for Generic Netlink interface of the TPH _DSM of PCIe root ports.
+ */
+struct tph_genl_dsm_info {
+	/* _DSM method identfier {GUID, REV, FUNCTION-INDEX}*/
+	struct acpi_genl_dsm_id id;
+	union {
+		struct {
+			/* bus, dev, devfn of the PCIe device */
+			int bus;
+			u32 dev;
+			u32 devfn;
+			/*
+			 * args for the TPH _DSM of the PCIe root-port of
+			 * the specified device.
+			 */
+			u32 pkg_arg0;
+			u32 pkg_arg1;
+			u64 pkg_arg2;
+		} arg;
+		struct {
+			/* return status */
+			int status;
+			/* st_info returned by the PCIe root-port TPH _DSM */
+			union st_info st;
+		} ret;
+	} info;
+};
+
 static u16 tph_extract_tag(enum tph_mem_type mem_type, u8 req_type,
 			   union st_info *info)
 {
@@ -130,6 +160,95 @@ static acpi_status tph_invoke_dsm(acpi_handle handle, u32 cpu_uid,
 
 	return AE_OK;
 }
+
+static int tph_invoke_dsm_genl_cb(struct acpi_genl_dsm_id *in,
+				  struct acpi_genl_dsm_id *out)
+{
+	struct pci_dev *pdev, *pdev_rp;
+	acpi_handle handle;
+	u32 cpu_uid;
+	int status = 0;
+
+	struct tph_genl_dsm_info *arg = (struct tph_genl_dsm_info *)in;
+	struct tph_genl_dsm_info *ret = (struct tph_genl_dsm_info *)out;
+
+	/* Honor  notph and acpi kernel parameters */
+	if (acpi_disabled || acpi_pci_disabled || pci_tph_disabled) {
+		status = -EINVAL;
+		goto out;
+	}
+
+	/*
+	 * pkg_arg1 contains the kernel logical CPU id provided by the user,
+	 * make sure it's a valid CPU id before passing it to down to firmware.
+	 * pkg_arg2 is not use by tph_invoke_dsm, hence no validation is
+	 * required.
+	 */
+	if (!(arg->info.arg.pkg_arg1 < nr_cpu_ids &&
+	    cpu_present(arg->info.arg.pkg_arg1))) {
+		status = -EINVAL;
+		goto out;
+	}
+
+	cpu_uid = topology_core_id(arg->info.arg.pkg_arg1);
+
+	ret->id.guid = pci_acpi_dsm_guid;
+	ret->id.rev = 7;
+	ret->id.func = TPH_ST_DSM_FUNC_INDEX;
+
+	pdev = pci_get_domain_bus_and_slot(arg->info.arg.bus, arg->info.arg.dev,
+					   arg->info.arg.devfn);
+	if (!pdev) {
+		status = -ENODEV;
+		goto out;
+	}
+
+	pdev_rp = pcie_find_root_port(pdev);
+	if (!pdev_rp || !pdev_rp->bus || !pdev_rp->bus->bridge) {
+		status = -ENODEV;
+		goto out;
+	}
+
+	handle = ACPI_HANDLE(pdev_rp->bus->bridge);
+
+	if (tph_invoke_dsm(handle, arg->info.arg.pkg_arg1, &ret->info.ret.st) !=
+			   AE_OK) {
+		status = -EOPNOTSUPP;
+		goto out;
+	}
+out:
+	ret->info.ret.status = status;
+	return 0;
+}
+
+static int tph_register_genl_cb(void)
+{
+	int err = 0;
+	struct acpi_genl_dsm_handle *handle =
+		kzalloc(sizeof(struct acpi_genl_dsm_handle), GFP_ATOMIC);
+	if (!handle) {
+		err = -ENOMEM;
+		goto out;
+	}
+
+	handle->id.guid = pci_acpi_dsm_guid;
+	handle->id.rev  = 7;
+	handle->id.func = TPH_ST_DSM_FUNC_INDEX;
+	handle->arg_len = sizeof(struct tph_genl_dsm_info);
+	handle->ret_len = sizeof(struct tph_genl_dsm_info);
+	handle->dsm_cb  = tph_invoke_dsm_genl_cb;
+	handle->cap     = CAP_SYS_ADMIN;
+
+	err = acpi_genl_dsm_add_handle(handle);
+	if (err) {
+		kfree(handle);
+		goto out;
+	}
+out:
+	return err;
+}
+
+late_initcall(tph_register_genl_cb);
 #endif
 
 /* Update the TPH Requester Enable field of TPH Control Register */
-- 
2.43.0

Re: [RFC 0/2] GENL interface for ACPI _DSM methods

[Jonathan Cameron]

On Mon, 6 Jan 2025 16:30:43 +0000
Wathsala Vithanage <wathsala.vithanage@arm.com> wrote:

> Linux v6.13-rc1 added support for PCIe TPH and direct cache injection.
> As already described in the patch set[1] that introduced this feature,
> the cache injection in supported hardware allows optimal utilization of
> platform resources for specific requests on the PCIe bus. However, the
> patch set [1] implements the functionality for usage within the kernel.
> But certain user space applications, especially those whose performance
> is sensitive to the latency of inbound writes as seen by a CPU core, may
> benefit from using this information (E.g., the DPDK cache stashing
> feature discussed in RFC [2]). This RFC is an attempt to obtain the PCIe
> steering tag information from the kernel to be used by user mode
> applications. We understand that there is more than one way to provide
> this information. Please review and suggest alternatives if necessary.
> 
> The first of the two patches introduced in this RFC attempts to overcome
> the kernel-only limitation by providing an API to kernel subsystems to
> hook up relevant _DSM methods to a GENL interface. User space
> applications can invoke a _DSM hooked up to this interface via the
> "acpi-event" GENL family socket, granted they have the minimum
> capabilities and message formats demanded by the kernel subsystem that
> hooked up the _DSM method. This feature is added by extending the
> "acpi-event" GENL family that multicasts ACPI events to the user-space
> applications such as acpid.
> 
> The second patch of this RFC hooks up the PCIe root-port TLP Processing
> Hints (TPH) _DSM to the ACPI GENL interface. User space applications
> like [2] can now request the kernel to execute the _DSM on their behalf
> and return steering-tag information.
> 
> [1] lore.kernel.org/linux-pci/20241002165954.128085-1-wei.huang2@amd.com
> [2] inbox.dpdk.org/dev/20241021015246.304431-2-wathsala.vithanage@arm.com

Hi Wathsala,

Superficially this feels like another potential interface that could be wrapped
up under appropriate fwctl. Jason, what do you think?

Mind you I'm not personally convinced that an interface that focuses on
exposing _DSM calls to userspace makes sense as opposed to subsystem specific
stuff.

Maybe consider associating the actual interface with the individual PCI functions
(which provides the first chunk of the message directly).

Also, _DSM is just one form of firmware interface used for PCI supporting
system. Tying the userspace interface to that feels unwise.  I can certainly
foresee a PSCI/SCMI or similar interface for this on ARM platforms
wrapped up in _DSM where ACPI is present but directly accessed when DT
is in use.

I'd also request that you break out what goes in ARG0,1,2 as that is all
stuff that the kernel is aware of and not all reviewers have access to the
ECN (I do though).  In particular the fact there are ACPI UIDs may
need a more generic solution.

Jonathan

> Wathsala Vithanage (2):
>   ACPI: Add support for invoking select _DSM methods from user space
>   PCI: Add generic netlink interface to TPH _DSM
> 
>  drivers/acpi/Makefile                 |   3 +-
>  drivers/acpi/{event.c => acpi_genl.c} | 110 ++++++++++++++++++++++-
>  drivers/acpi/acpi_genl_dsm.c          |  76 ++++++++++++++++
>  drivers/pci/tph.c                     | 121 ++++++++++++++++++++++++++
>  include/acpi/acpi_genl.h              |  54 ++++++++++++
>  include/linux/acpi.h                  |   1 +
>  6 files changed, 360 insertions(+), 5 deletions(-)
>  rename drivers/acpi/{event.c => acpi_genl.c} (63%)
>  create mode 100644 drivers/acpi/acpi_genl_dsm.c
>  create mode 100644 include/acpi/acpi_genl.h
> 

Re: [RFC 0/2] GENL interface for ACPI _DSM methods

[Jeremy Linton]

Hi,

On 1/6/25 12:01 PM, Jonathan Cameron wrote:
> On Mon, 6 Jan 2025 16:30:43 +0000
> Wathsala Vithanage <wathsala.vithanage@arm.com> wrote:
> 
>> Linux v6.13-rc1 added support for PCIe TPH and direct cache injection.
>> As already described in the patch set[1] that introduced this feature,
>> the cache injection in supported hardware allows optimal utilization of
>> platform resources for specific requests on the PCIe bus. However, the
>> patch set [1] implements the functionality for usage within the kernel.
>> But certain user space applications, especially those whose performance
>> is sensitive to the latency of inbound writes as seen by a CPU core, may
>> benefit from using this information (E.g., the DPDK cache stashing
>> feature discussed in RFC [2]). This RFC is an attempt to obtain the PCIe
>> steering tag information from the kernel to be used by user mode
>> applications. We understand that there is more than one way to provide
>> this information. Please review and suggest alternatives if necessary.
>>
>> The first of the two patches introduced in this RFC attempts to overcome
>> the kernel-only limitation by providing an API to kernel subsystems to
>> hook up relevant _DSM methods to a GENL interface. User space
>> applications can invoke a _DSM hooked up to this interface via the
>> "acpi-event" GENL family socket, granted they have the minimum
>> capabilities and message formats demanded by the kernel subsystem that
>> hooked up the _DSM method. This feature is added by extending the
>> "acpi-event" GENL family that multicasts ACPI events to the user-space
>> applications such as acpid.
>>
>> The second patch of this RFC hooks up the PCIe root-port TLP Processing
>> Hints (TPH) _DSM to the ACPI GENL interface. User space applications
>> like [2] can now request the kernel to execute the _DSM on their behalf
>> and return steering-tag information.
>>
>> [1] lore.kernel.org/linux-pci/20241002165954.128085-1-wei.huang2@amd.com
>> [2] inbox.dpdk.org/dev/20241021015246.304431-2-wathsala.vithanage@arm.com
> 
> Hi Wathsala,
> 
> Superficially this feels like another potential interface that could be wrapped
> up under appropriate fwctl. Jason, what do you think?
> 
> Mind you I'm not personally convinced that an interface that focuses on
> exposing _DSM calls to userspace makes sense as opposed to subsystem specific
> stuff.
> 
> Maybe consider associating the actual interface with the individual PCI functions
> (which provides the first chunk of the message directly).

Right,

I think this was similar to a conversation we had internally, which was 
basically to detect the PCIe extended capability and export a 'steering' 
entry in sysfs on each PCIe device which can take a logical cpu/cache 
value, translate those on write to the ACPI cpu/cache id's, make the 
firmware call, then directly update the PCIe device's capability with 
the result. This also leaves the door open for future 
cpu/cache->steering tag translation methods to transparently replace the 
_DSM call while leaving the userspace API the same.


> 
> Also, _DSM is just one form of firmware interface used for PCI supporting
> system. Tying the userspace interface to that feels unwise.  I can certainly
> foresee a PSCI/SCMI or similar interface for this on ARM platforms
> wrapped up in _DSM where ACPI is present but directly accessed when DT
> is in use.
> 
> I'd also request that you break out what goes in ARG0,1,2 as that is all
> stuff that the kernel is aware of and not all reviewers have access to the
> ECN (I do though).  In particular the fact there are ACPI UIDs may
> need a more generic solution.
> 
> Jonathan
> 
>> Wathsala Vithanage (2):
>>    ACPI: Add support for invoking select _DSM methods from user space
>>    PCI: Add generic netlink interface to TPH _DSM
>>
>>   drivers/acpi/Makefile                 |   3 +-
>>   drivers/acpi/{event.c => acpi_genl.c} | 110 ++++++++++++++++++++++-
>>   drivers/acpi/acpi_genl_dsm.c          |  76 ++++++++++++++++
>>   drivers/pci/tph.c                     | 121 ++++++++++++++++++++++++++
>>   include/acpi/acpi_genl.h              |  54 ++++++++++++
>>   include/linux/acpi.h                  |   1 +
>>   6 files changed, 360 insertions(+), 5 deletions(-)
>>   rename drivers/acpi/{event.c => acpi_genl.c} (63%)
>>   create mode 100644 drivers/acpi/acpi_genl_dsm.c
>>   create mode 100644 include/acpi/acpi_genl.h
>>
> 

Re: [RFC 0/2] GENL interface for ACPI _DSM methods

[Jason Gunthorpe]

On Tue, Jan 07, 2025 at 11:37:01AM -0600, Jeremy Linton wrote:
> Hi,
> 
> On 1/6/25 12:01 PM, Jonathan Cameron wrote:
> > On Mon, 6 Jan 2025 16:30:43 +0000
> > Wathsala Vithanage <wathsala.vithanage@arm.com> wrote:
> > 
> > > Linux v6.13-rc1 added support for PCIe TPH and direct cache injection.
> > > As already described in the patch set[1] that introduced this feature,
> > > the cache injection in supported hardware allows optimal utilization of
> > > platform resources for specific requests on the PCIe bus. However, the
> > > patch set [1] implements the functionality for usage within the kernel.
> > > But certain user space applications, especially those whose performance
> > > is sensitive to the latency of inbound writes as seen by a CPU core, may
> > > benefit from using this information (E.g., the DPDK cache stashing
> > > feature discussed in RFC [2]). 

There is no way for userspace to program TPH information into a PCI
device without going through a kernel driver, and the kernel driver
must be the exclusive owner of the steering tag configuration or chaos
would ensue. Having a way for sysfs to override this seems very wrong
to me, and I think you should not go in this direction.

DPDK runs on VFIO or RDMA. It would natural to have an VFIO native API
to manipulate the steering tags, and we are already discussing what
RDMA support for steering tag would look like.

> > Superficially this feels like another potential interface that could be wrapped
> > up under appropriate fwctl. Jason, what do you think?

As above, I think this very squarely belongs under the appropriate
subsystems that are providing the kernel drivers for the device. There
is no reasonable way to share steering tags with unrelated userspace
through any mechanism. Basically it fails the independence test of
fwctl.

> I think this was similar to a conversation we had internally, which was
> basically to detect the PCIe extended capability and export a 'steering'
> entry in sysfs on each PCIe device which can take a logical cpu/cache value,
> translate those on write to the ACPI cpu/cache id's, make the firmware call,
> then directly update the PCIe device's capability with the result. 

Seems wrong, driver must do this. If the driver was already using that
entry for something else you've just wrecked it.

Jason

Re: [RFC 0/2] GENL interface for ACPI _DSM methods

[Jeremy Linton]

Hi,

On 1/7/25 11:48 AM, Jason Gunthorpe wrote:
> On Tue, Jan 07, 2025 at 11:37:01AM -0600, Jeremy Linton wrote:
>> Hi,
>>
>> On 1/6/25 12:01 PM, Jonathan Cameron wrote:
>>> On Mon, 6 Jan 2025 16:30:43 +0000
>>> Wathsala Vithanage <wathsala.vithanage@arm.com> wrote:
>>>
>>>> Linux v6.13-rc1 added support for PCIe TPH and direct cache injection.
>>>> As already described in the patch set[1] that introduced this feature,
>>>> the cache injection in supported hardware allows optimal utilization of
>>>> platform resources for specific requests on the PCIe bus. However, the
>>>> patch set [1] implements the functionality for usage within the kernel.
>>>> But certain user space applications, especially those whose performance
>>>> is sensitive to the latency of inbound writes as seen by a CPU core, may
>>>> benefit from using this information (E.g., the DPDK cache stashing
>>>> feature discussed in RFC [2]).
> 
> There is no way for userspace to program TPH information into a PCI
> device without going through a kernel driver, and the kernel driver
> must be the exclusive owner of the steering tag configuration or chaos
> would ensue. Having a way for sysfs to override this seems very wrong
> to me, and I think you should not go in this direction.
> 
> DPDK runs on VFIO or RDMA. It would natural to have an VFIO native API
> to manipulate the steering tags, and we are already discussing what
> RDMA support for steering tag would look like.
> 
>>> Superficially this feels like another potential interface that could be wrapped
>>> up under appropriate fwctl. Jason, what do you think?
> 
> As above, I think this very squarely belongs under the appropriate
> subsystems that are providing the kernel drivers for the device. There
> is no reasonable way to share steering tags with unrelated userspace
> through any mechanism. Basically it fails the independence test of
> fwctl.
> 
>> I think this was similar to a conversation we had internally, which was
>> basically to detect the PCIe extended capability and export a 'steering'
>> entry in sysfs on each PCIe device which can take a logical cpu/cache value,
>> translate those on write to the ACPI cpu/cache id's, make the firmware call,
>> then directly update the PCIe device's capability with the result.
> 
> Seems wrong, driver must do this. If the driver was already using that
> entry for something else you've just wrecked it.

Can you clarify what you mean by 'wrecked'? AFAIK a valid, if poorly 
chosen, steering tag is going to be sub-optimal performance.

I'm under the impression this is a similar problem to cpu/irq/numa 
affinity where the driver/subsystem should be making the choice, but the 
user is provided the opportunity to override the defaults if they think 
there is benefit in their environment. Again AFAIK, the whole 
OS/software stashing is already well down the 'I know better than the HW 
where to store this data' rabbit hole.


Thanks,

Re: [RFC 0/2] GENL interface for ACPI _DSM methods

[Jason Gunthorpe]

On Wed, Jan 08, 2025 at 01:59:35PM -0600, Jeremy Linton wrote:

> I'm under the impression this is a similar problem to cpu/irq/numa affinity
> where the driver/subsystem should be making the choice, but the user is
> provided the opportunity to override the defaults if they think there is
> benefit in their environment. 

Which I think has been proven to have been a mistake. Instead over
overriding irq affinity though proc/irq under the covers of the driver
and hoping for the best the driver itself should have the opportinuty
to set the affinity for its objects directly.

Lets us not repeat this mistake with steering tag. The driver should
always be involved in this stuff, if you want it to work with DPDK
then go through the kernel driver that DPDK is running on top of (VFIO
or RDMA)

Jason

RE: [RFC 0/2] GENL interface for ACPI _DSM methods

[Wathsala Wathawana Vithanage]

> 
> On Wed, Jan 08, 2025 at 01:59:35PM -0600, Jeremy Linton wrote:
> 
> > I'm under the impression this is a similar problem to cpu/irq/numa
> > affinity where the driver/subsystem should be making the choice, but
> > the user is provided the opportunity to override the defaults if they
> > think there is benefit in their environment.
> 
> Which I think has been proven to have been a mistake. Instead over overriding irq
> affinity though proc/irq under the covers of the driver and hoping for the best the
> driver itself should have the opportinuty to set the affinity for its objects directly.
> 

Do you mean that the driver should handle affinity requests from the user directly
as per its policy?

> Lets us not repeat this mistake with steering tag. The driver should always be
> involved in this stuff, if you want it to work with DPDK then go through the kernel
> driver that DPDK is running on top of (VFIO or RDMA)
> 

This RFC is only about acquiring the steering tag from the ACPI _DSM, which the DPDK
user space driver will set in the queue context of the device it manages.
Setting of the steering tag part happens in the DPDK device driver.
Are you suggesting that I should instead pass a CPU and a cache ID to VFIO and let VFIO
decide what's right for the application?


--wathsala

Re: [RFC 0/2] GENL interface for ACPI _DSM methods

[Jason Gunthorpe]

On Thu, Jan 09, 2025 at 12:34:31AM +0000, Wathsala Wathawana Vithanage wrote:

> > > I'm under the impression this is a similar problem to cpu/irq/numa
> > > affinity where the driver/subsystem should be making the choice, but
> > > the user is provided the opportunity to override the defaults if they
> > > think there is benefit in their environment.
> > 
> > Which I think has been proven to have been a mistake. Instead over overriding irq
> > affinity though proc/irq under the covers of the driver and hoping for the best the
> > driver itself should have the opportinuty to set the affinity for its objects directly.
> 
> Do you mean that the driver should handle affinity requests from the user directly
> as per its policy?

Yes, not every driver has tidy mappings of objects to interrupts
vectors.
 
> > Lets us not repeat this mistake with steering tag. The driver should always be
> > involved in this stuff, if you want it to work with DPDK then go through the kernel
> > driver that DPDK is running on top of (VFIO or RDMA)
>
> This RFC is only about acquiring the steering tag from the ACPI _DSM, which the DPDK
> user space driver will set in the queue context of the device it manages.
> Setting of the steering tag part happens in the DPDK device driver.
> Are you suggesting that I should instead pass a CPU and a cache ID to VFIO and let VFIO
> decide what's right for the application?

I think that would be better, yes. Get VFIO to give you the steering
tag information, and any related control of the config space you may
need, via an IOCTL.

Jason

RE: [RFC 0/2] GENL interface for ACPI _DSM methods

[Wathsala Wathawana Vithanage]

> 
> > > > I'm under the impression this is a similar problem to cpu/irq/numa
> > > > affinity where the driver/subsystem should be making the choice,
> > > > but the user is provided the opportunity to override the defaults
> > > > if they think there is benefit in their environment.
> > >
> > > Which I think has been proven to have been a mistake. Instead over
> > > overriding irq affinity though proc/irq under the covers of the
> > > driver and hoping for the best the driver itself should have the opportinuty to
> set the affinity for its objects directly.
> >
> > Do you mean that the driver should handle affinity requests from the
> > user directly as per its policy?
> 
> Yes, not every driver has tidy mappings of objects to interrupts vectors.
> 
> > > Lets us not repeat this mistake with steering tag. The driver should
> > > always be involved in this stuff, if you want it to work with DPDK
> > > then go through the kernel driver that DPDK is running on top of
> > > (VFIO or RDMA)
> >
> > This RFC is only about acquiring the steering tag from the ACPI _DSM,
> > which the DPDK user space driver will set in the queue context of the device it
> manages.
> > Setting of the steering tag part happens in the DPDK device driver.
> > Are you suggesting that I should instead pass a CPU and a cache ID to
> > VFIO and let VFIO decide what's right for the application?
> 
> I think that would be better, yes. Get VFIO to give you the steering tag
> information, and any related control of the config space you may need, via an
> IOCTL.

Sounds good, I like that idea.

--wathsala

Re: [RFC 0/2] GENL interface for ACPI _DSM methods

[Jason Gunthorpe]

> > > This RFC is only about acquiring the steering tag from the ACPI _DSM,
> > > which the DPDK user space driver will set in the queue context
> > > of the device it manages.
> > > Setting of the steering tag part happens in the DPDK device driver.
> > > Are you suggesting that I should instead pass a CPU and a cache ID to
> > > VFIO and let VFIO decide what's right for the application?
> > 
> > I think that would be better, yes. Get VFIO to give you the steering tag
> > information, and any related control of the config space you may need, via an
> > IOCTL.
> 
> Sounds good, I like that idea.

Yeah, remember there is a broad complexity here. Some devices want to
setup their steering tag information through the MSI-X table, for
instance. VFIO is the right place to assist DPDK with all of these
cases that need kernel involvement.

Jason