diff mbox series

[V3,06/10] PCI/TPH: Introduce API to retrieve TPH steering tags from ACPI

Message ID 20240717205511.2541693-7-wei.huang2@amd.com (mailing list archive)
State Superseded
Delegated to: Bjorn Helgaas
Headers show
Series PCIe TPH and cache direct injection support | expand

Commit Message

Wei Huang July 17, 2024, 8:55 p.m. UTC 
Add an API function to allow endpoint device drivers to retrieve
steering tags for a specific cpu_uid. This is achieved by invoking
ACPI _DSM on device's root port.

Co-developed-by: Eric Van Tassell <Eric.VanTassell@amd.com>
Signed-off-by: Eric Van Tassell <Eric.VanTassell@amd.com>
Signed-off-by: Wei Huang <wei.huang2@amd.com>
Reviewed-by: Ajit Khaparde <ajit.khaparde@broadcom.com>
Reviewed-by: Somnath Kotur <somnath.kotur@broadcom.com>
Reviewed-by: Andy Gospodarek <andrew.gospodarek@broadcom.com>
---
 drivers/pci/pcie/tph.c  | 162 ++++++++++++++++++++++++++++++++++++++++
 include/linux/pci-tph.h |  12 +++
 2 files changed, 174 insertions(+)

Comments

Bjorn Helgaas July 23, 2024, 10:22 p.m. UTC | #1 
On Wed, Jul 17, 2024 at 03:55:07PM -0500, Wei Huang wrote:
> Add an API function to allow endpoint device drivers to retrieve
> steering tags for a specific cpu_uid. This is achieved by invoking
> ACPI _DSM on device's root port.

s/an API function/<name of function>/

Also include function name in subject line.

> + * The st_info struct defines the steering tag returned by the firmware _DSM
> + * method defined in PCI Firmware Spec r3.3, sect 4.6.15 "_DSM to Query Cache
> + * Locality TPH Features"

I don't know what I'm missing, but my copy of the r3.3 spec, dated Jan
20, 2021, doesn't have sec 4.6.15.

> +static u16 tph_extract_tag(enum tph_mem_type mem_type, u8 req_type,
> +			   union st_info *st_tag)
> +{
> +	switch (req_type) {
> +	case PCI_TPH_REQ_TPH_ONLY: /* 8 bit tags */
> +		switch (mem_type) {
> +		case TPH_MEM_TYPE_VM:
> +			if (st_tag->vm_st_valid)
> +				return st_tag->vm_st;
> +			break;
> +		case TPH_MEM_TYPE_PM:
> +			if (st_tag->pm_st_valid)
> +				return st_tag->pm_st;
> +			break;
> +		}
> +		break;
> +	case PCI_TPH_REQ_EXT_TPH: /* 16 bit tags */
> +		switch (mem_type) {
> +		case TPH_MEM_TYPE_VM:
> +			if (st_tag->vm_xst_valid)
> +				return st_tag->vm_xst;
> +			break;
> +		case TPH_MEM_TYPE_PM:
> +			if (st_tag->pm_xst_valid)
> +				return st_tag->pm_xst;
> +			break;
> +		}
> +		break;
> +	default:
> +		pr_err("invalid steering tag in ACPI _DSM\n");

Needs to mention a specific device.

> +static acpi_status tph_invoke_dsm(acpi_handle handle, u32 cpu_uid, u8 ph,
> +				  u8 target_type, bool cache_ref_valid,
> +				  u64 cache_ref, union st_info *st_out)

IMO you can omit ph, target_type, cache_ref_valid, etc until you have
a need for them.  I don't see the point of parameters that always have
the same constant values.

> +{
> +	union acpi_object arg3[3], in_obj, *out_obj;
> +
> +	if (!acpi_check_dsm(handle, &pci_acpi_dsm_guid, 7, BIT(TPH_ST_DSM_FUNC_INDEX)))

Wrap the code and comments in this file to fit in 80 columns instead
of 85 or whatever you used.  Lines longer than 80 are OK for printf
strings but pointless for comments, etc.

> +		return AE_ERROR;
> +
> +	/* DWORD: feature ID (0 for processor cache ST query) */
> +	arg3[0].integer.type = ACPI_TYPE_INTEGER;
> +	arg3[0].integer.value = 0;
> +
> +	/* DWORD: target UID */
> +	arg3[1].integer.type = ACPI_TYPE_INTEGER;
> +	arg3[1].integer.value = cpu_uid;
> +
> +	/* QWORD: properties */
> +	arg3[2].integer.type = ACPI_TYPE_INTEGER;
> +	arg3[2].integer.value = ph & 3;
> +	arg3[2].integer.value |= (target_type & 1) << 2;
> +	arg3[2].integer.value |= (cache_ref_valid & 1) << 3;
> +	arg3[2].integer.value |= (cache_ref << 32);
> +
> +	in_obj.type = ACPI_TYPE_PACKAGE;
> +	in_obj.package.count = ARRAY_SIZE(arg3);
> +	in_obj.package.elements = arg3;
> +
> +	out_obj = acpi_evaluate_dsm(handle, &pci_acpi_dsm_guid, 7,
> +				    TPH_ST_DSM_FUNC_INDEX, &in_obj);
> +
> +	if (!out_obj)
> +		return AE_ERROR;
> +
> +	if (out_obj->type != ACPI_TYPE_BUFFER) {
> +		ACPI_FREE(out_obj);
> +		return AE_ERROR;
> +	}
> +
> +	st_out->value = *((u64 *)(out_obj->buffer.pointer));
> +
> +	ACPI_FREE(out_obj);
> +
> +	return AE_OK;
> +}
> +
>  /* Update the ST Mode Select field of TPH Control Register */
>  static void set_ctrl_reg_mode_sel(struct pci_dev *pdev, u8 st_mode)
>  {
> @@ -89,3 +210,44 @@ bool pcie_tph_intr_vec_supported(struct pci_dev *pdev)
>  	return true;
>  }
>  EXPORT_SYMBOL(pcie_tph_intr_vec_supported);
> +
> +/**
> + * pcie_tph_get_st_from_acpi() - Retrieve steering tag for a specific CPU
> + * using platform ACPI _DSM

1) TPH and Steering Tags are not ACPI-specific, even though the only
current mechanism to learn the tags happens to be an ACPI _DSM, so I
think we should omit "acpi" from the name drivers use.

2) The spec doesn't restrict Steering Tags to be for a CPU; it says
"processing resource such as a host processor or system cache
hierarchy ..."  But obviously this interface only comprehends an ACPI
CPU ID.  Maybe the function name should include "cpu".

3) Nobody outside the file calls this yet, so it should probably be
static (and removed from the doc) until a driver needs it.

> + * @pdev: pci device
> + * @cpu_acpi_uid: the acpi cpu_uid.
> + * @mem_type: memory type (vram, nvram)
> + * @req_type: request type (disable, tph, extended tph)
> + * @tag: steering tag return value
> + *
> + * Return: 0 if success, otherwise errno
> + */
> +int pcie_tph_get_st_from_acpi(struct pci_dev *pdev, unsigned int cpu_acpi_uid,
> +			      enum tph_mem_type mem_type, u8 req_type,
> +			      u16 *tag)
> +{
> +	struct pci_dev *rp;
> +	acpi_handle rp_acpi_handle;
> +	union st_info info;
> +
> +	if (!pdev->tph_cap)
> +		return -ENODEV;
> +
> +	/* find ACPI handler for device's root port */

Superfluous comments since the code is obvious.  And this finds a
"handle", not a "handler" :)

> +	rp = pcie_find_root_port(pdev);
> +	if (!rp || !rp->bus || !rp->bus->bridge)
> +		return -ENODEV;
> +	rp_acpi_handle = ACPI_HANDLE(rp->bus->bridge);
> +
> +	/* invoke _DSM to extract tag value */
> +	if (tph_invoke_dsm(rp_acpi_handle, cpu_acpi_uid, 0, 0, false, 0, &info) != AE_OK) {
> +		*tag = 0;
> +		return -EINVAL;
> +	}
> +
> +	*tag = tph_extract_tag(mem_type, req_type, &info);
> +	pci_dbg(pdev, "%s: cpu=%d tag=%d\n", __func__, cpu_acpi_uid, *tag);
> +
> +	return 0;
> +}
> +EXPORT_SYMBOL(pcie_tph_get_st_from_acpi);
> diff --git a/include/linux/pci-tph.h b/include/linux/pci-tph.h
> index 854677651d81..b12a592f3d49 100644
> --- a/include/linux/pci-tph.h
> +++ b/include/linux/pci-tph.h
> @@ -9,15 +9,27 @@
>  #ifndef LINUX_PCI_TPH_H
>  #define LINUX_PCI_TPH_H
>  
> +enum tph_mem_type {
> +	TPH_MEM_TYPE_VM,	/* volatile memory type */
> +	TPH_MEM_TYPE_PM		/* persistent memory type */

Where does this come from?  I don't see "vram" or "volatile" used in
the PCIe spec in this context.  Maybe this is from the PCI Firmware
spec?

> +static inline int pcie_tph_get_st_from_acpi(struct pci_dev *dev, unsigned int cpu_acpi_uid,
> +					    enum tph_mem_type tag_type, u8 req_enable,
> +					    u16 *tag)
> +{ return false; }

"false" is not "int".

Apparently you want to return "success" in this case, when
CONFIG_PCIE_TPH is not enabled?  I suggested leaving this non-exported
for now, which would mean removing this altogether.  But if/when we do
export it, I think maybe it should return error so a caller doesn't
assume it succeeded, since *tag will be garbage.

Bjorn
Wei Huang Aug. 2, 2024, 4:58 a.m. UTC | #2 
On 7/23/24 17:22, Bjorn Helgaas wrote:
>> + * The st_info struct defines the steering tag returned by the firmware _DSM
>> + * method defined in PCI Firmware Spec r3.3, sect 4.6.15 "_DSM to Query Cache
>> + * Locality TPH Features"
> 
> I don't know what I'm missing, but my copy of the r3.3 spec, dated Jan
> 20, 2021, doesn't have sec 4.6.15.

According to https://members.pcisig.com/wg/PCI-SIG/document/15470, the 
revision has "4.6.15. _DSM to Query Cache Locality TPH Features". 
PCI-SIG approved this ECN, but haven't merged it into PCI Firmware 
Specification 3.3 yet.

<snip>

>> +
>> +/**
>> + * pcie_tph_get_st_from_acpi() - Retrieve steering tag for a specific CPU
>> + * using platform ACPI _DSM
> 
> 1) TPH and Steering Tags are not ACPI-specific, even though the only
> current mechanism to learn the tags happens to be an ACPI _DSM, so I
> think we should omit "acpi" from the name drivers use.
> 
> 2) The spec doesn't restrict Steering Tags to be for a CPU; it says
> "processing resource such as a host processor or system cache
> hierarchy ..."  But obviously this interface only comprehends an ACPI
> CPU ID.  Maybe the function name should include "cpu".

How about pcie_tph_get_st_for_cpu() or pcie_tph_retreive_st_for_cpu()?

>> diff --git a/include/linux/pci-tph.h b/include/linux/pci-tph.h
>> index 854677651d81..b12a592f3d49 100644
>> --- a/include/linux/pci-tph.h
>> +++ b/include/linux/pci-tph.h
>> @@ -9,15 +9,27 @@
>>   #ifndef LINUX_PCI_TPH_H
>>   #define LINUX_PCI_TPH_H
>>   
>> +enum tph_mem_type {
>> +	TPH_MEM_TYPE_VM,	/* volatile memory type */
>> +	TPH_MEM_TYPE_PM		/* persistent memory type */
> 
> Where does this come from?  I don't see "vram" or "volatile" used in
> the PCIe spec in this context.  Maybe this is from the PCI Firmware
> spec?
> 

Yes, this is defined in the ECN mentioned above. Do you have concerns 
about defining them here? If we want to remove it, then 
pcie_tph_get_st_from_acpi() function can only support one memory type 
(e.g. vram). Any advice?

>> +static inline int pcie_tph_get_st_from_acpi(struct pci_dev *dev, unsigned int cpu_acpi_uid,
>> +					    enum tph_mem_type tag_type, u8 req_enable,
>> +					    u16 *tag)
>> +{ return false; }
> 
> "false" is not "int".
> 
> Apparently you want to return "success" in this case, when
> CONFIG_PCIE_TPH is not enabled?  I suggested leaving this non-exported
> for now, which would mean removing this altogether.  But if/when we do
> export it, I think maybe it should return error so a caller doesn't
> assume it succeeded, since *tag will be garbage.
> 
> Bjorn
Bjorn Helgaas Aug. 2, 2024, 4:58 p.m. UTC | #3 
On Thu, Aug 01, 2024 at 11:58:46PM -0500, Wei Huang wrote:
> On 7/23/24 17:22, Bjorn Helgaas wrote:
> > > + * The st_info struct defines the steering tag returned by the firmware _DSM
> > > + * method defined in PCI Firmware Spec r3.3, sect 4.6.15 "_DSM to Query Cache
> > > + * Locality TPH Features"
> > 
> > I don't know what I'm missing, but my copy of the r3.3 spec, dated Jan
> > 20, 2021, doesn't have sec 4.6.15.
> 
> According to https://members.pcisig.com/wg/PCI-SIG/document/15470,
> the revision has "4.6.15. _DSM to Query Cache Locality TPH
> Features". PCI-SIG approved this ECN, but haven't merged it into PCI
> Firmware Specification 3.3 yet.

Thanks for the pointer.  Please update to refer to this as an approved
ECN to r3.3 and include the URL.  When it is eventually incorporated
into a PCI Firmware spec revision, it will not be r3.3.  It will
likely be r3.4 or r4.0, so r3.3 will never be the correct citation.

> > > + * pcie_tph_get_st_from_acpi() - Retrieve steering tag for a specific CPU
> > > + * using platform ACPI _DSM
> > 
> > 1) TPH and Steering Tags are not ACPI-specific, even though the only
> > current mechanism to learn the tags happens to be an ACPI _DSM, so I
> > think we should omit "acpi" from the name drivers use.
> > 
> > 2) The spec doesn't restrict Steering Tags to be for a CPU; it says
> > "processing resource such as a host processor or system cache
> > hierarchy ..."  But obviously this interface only comprehends an ACPI
> > CPU ID.  Maybe the function name should include "cpu".
> 
> How about pcie_tph_get_st_for_cpu() or pcie_tph_retreive_st_for_cpu()?

Sounds good.  The former is nice because it's shorter.
"pcie_tph_cpu_st()" would even be fine with me.  s/retreive/retrieve/
if you use that.

> > > diff --git a/include/linux/pci-tph.h b/include/linux/pci-tph.h
> > > index 854677651d81..b12a592f3d49 100644
> > > --- a/include/linux/pci-tph.h
> > > +++ b/include/linux/pci-tph.h
> > > @@ -9,15 +9,27 @@
> > >   #ifndef LINUX_PCI_TPH_H
> > >   #define LINUX_PCI_TPH_H
> > > +enum tph_mem_type {
> > > +	TPH_MEM_TYPE_VM,	/* volatile memory type */
> > > +	TPH_MEM_TYPE_PM		/* persistent memory type */
> > 
> > Where does this come from?  I don't see "vram" or "volatile" used in
> > the PCIe spec in this context.  Maybe this is from the PCI Firmware
> > spec?
> 
> Yes, this is defined in the ECN mentioned above. Do you have
> concerns about defining them here? If we want to remove it, then
> pcie_tph_get_st_from_acpi() function can only support one memory
> type (e.g.  vram). Any advice?

No concerns if they're defined in the ECN, but we need a citation so
we know where to look for these values.  Cite the ECN for now, and we
can update to the actual firmware spec citation when it becomes
available.

Bjorn
diff mbox series

Patch

diff --git a/drivers/pci/pcie/tph.c b/drivers/pci/pcie/tph.c
index 7183370b0977..c805c8b1a7d2 100644
--- a/drivers/pci/pcie/tph.c
+++ b/drivers/pci/pcie/tph.c
@@ -7,12 +7,133 @@ 
  *     Wei Huang <wei.huang2@amd.com>
  */
 
+#define pr_fmt(fmt) "TPH: " fmt
+#define dev_fmt pr_fmt
+
 #include <linux/pci.h>
+#include <linux/pci-acpi.h>
 #include <linux/bitfield.h>
 #include <linux/pci-tph.h>
 
 #include "../pci.h"
 
+/*
+ * The st_info struct defines the steering tag returned by the firmware _DSM
+ * method defined in PCI Firmware Spec r3.3, sect 4.6.15 "_DSM to Query Cache
+ * Locality TPH Features"
+ *
+ * @vm_st_valid:  8 bit tag for volatile memory is valid
+ * @vm_xst_valid: 16 bit tag for volatile memory is valid
+ * @vm_ignore:    1 => was and will be ignored, 0 => ph should be supplied
+ * @vm_st:        8 bit steering tag for volatile mem
+ * @vm_xst:       16 bit steering tag for volatile mem
+ * @pm_st_valid:  8 bit tag for persistent memory is valid
+ * @pm_xst_valid: 16 bit tag for persistent memory is valid
+ * @pm_ph_ignore: 1 => was and will be ignore, 0 => ph should be supplied
+ * @pm_st:        8 bit steering tag for persistent mem
+ * @pm_xst:       16 bit steering tag for persistent mem
+ */
+union st_info {
+	struct {
+		u64 vm_st_valid : 1;
+		u64 vm_xst_valid : 1;
+		u64 vm_ph_ignore : 1;
+		u64 rsvd1 : 5;
+		u64 vm_st : 8;
+		u64 vm_xst : 16;
+		u64 pm_st_valid : 1;
+		u64 pm_xst_valid : 1;
+		u64 pm_ph_ignore : 1;
+		u64 rsvd2 : 5;
+		u64 pm_st : 8;
+		u64 pm_xst : 16;
+	};
+	u64 value;
+};
+
+static u16 tph_extract_tag(enum tph_mem_type mem_type, u8 req_type,
+			   union st_info *st_tag)
+{
+	switch (req_type) {
+	case PCI_TPH_REQ_TPH_ONLY: /* 8 bit tags */
+		switch (mem_type) {
+		case TPH_MEM_TYPE_VM:
+			if (st_tag->vm_st_valid)
+				return st_tag->vm_st;
+			break;
+		case TPH_MEM_TYPE_PM:
+			if (st_tag->pm_st_valid)
+				return st_tag->pm_st;
+			break;
+		}
+		break;
+	case PCI_TPH_REQ_EXT_TPH: /* 16 bit tags */
+		switch (mem_type) {
+		case TPH_MEM_TYPE_VM:
+			if (st_tag->vm_xst_valid)
+				return st_tag->vm_xst;
+			break;
+		case TPH_MEM_TYPE_PM:
+			if (st_tag->pm_xst_valid)
+				return st_tag->pm_xst;
+			break;
+		}
+		break;
+	default:
+		pr_err("invalid steering tag in ACPI _DSM\n");
+		return 0;
+	}
+
+	return 0;
+}
+
+#define TPH_ST_DSM_FUNC_INDEX	0xF
+static acpi_status tph_invoke_dsm(acpi_handle handle, u32 cpu_uid, u8 ph,
+				  u8 target_type, bool cache_ref_valid,
+				  u64 cache_ref, union st_info *st_out)
+{
+	union acpi_object arg3[3], in_obj, *out_obj;
+
+	if (!acpi_check_dsm(handle, &pci_acpi_dsm_guid, 7, BIT(TPH_ST_DSM_FUNC_INDEX)))
+		return AE_ERROR;
+
+	/* DWORD: feature ID (0 for processor cache ST query) */
+	arg3[0].integer.type = ACPI_TYPE_INTEGER;
+	arg3[0].integer.value = 0;
+
+	/* DWORD: target UID */
+	arg3[1].integer.type = ACPI_TYPE_INTEGER;
+	arg3[1].integer.value = cpu_uid;
+
+	/* QWORD: properties */
+	arg3[2].integer.type = ACPI_TYPE_INTEGER;
+	arg3[2].integer.value = ph & 3;
+	arg3[2].integer.value |= (target_type & 1) << 2;
+	arg3[2].integer.value |= (cache_ref_valid & 1) << 3;
+	arg3[2].integer.value |= (cache_ref << 32);
+
+	in_obj.type = ACPI_TYPE_PACKAGE;
+	in_obj.package.count = ARRAY_SIZE(arg3);
+	in_obj.package.elements = arg3;
+
+	out_obj = acpi_evaluate_dsm(handle, &pci_acpi_dsm_guid, 7,
+				    TPH_ST_DSM_FUNC_INDEX, &in_obj);
+
+	if (!out_obj)
+		return AE_ERROR;
+
+	if (out_obj->type != ACPI_TYPE_BUFFER) {
+		ACPI_FREE(out_obj);
+		return AE_ERROR;
+	}
+
+	st_out->value = *((u64 *)(out_obj->buffer.pointer));
+
+	ACPI_FREE(out_obj);
+
+	return AE_OK;
+}
+
 /* Update the ST Mode Select field of TPH Control Register */
 static void set_ctrl_reg_mode_sel(struct pci_dev *pdev, u8 st_mode)
 {
@@ -89,3 +210,44 @@  bool pcie_tph_intr_vec_supported(struct pci_dev *pdev)
 	return true;
 }
 EXPORT_SYMBOL(pcie_tph_intr_vec_supported);
+
+/**
+ * pcie_tph_get_st_from_acpi() - Retrieve steering tag for a specific CPU
+ * using platform ACPI _DSM
+ * @pdev: pci device
+ * @cpu_acpi_uid: the acpi cpu_uid.
+ * @mem_type: memory type (vram, nvram)
+ * @req_type: request type (disable, tph, extended tph)
+ * @tag: steering tag return value
+ *
+ * Return: 0 if success, otherwise errno
+ */
+int pcie_tph_get_st_from_acpi(struct pci_dev *pdev, unsigned int cpu_acpi_uid,
+			      enum tph_mem_type mem_type, u8 req_type,
+			      u16 *tag)
+{
+	struct pci_dev *rp;
+	acpi_handle rp_acpi_handle;
+	union st_info info;
+
+	if (!pdev->tph_cap)
+		return -ENODEV;
+
+	/* find ACPI handler for device's root port */
+	rp = pcie_find_root_port(pdev);
+	if (!rp || !rp->bus || !rp->bus->bridge)
+		return -ENODEV;
+	rp_acpi_handle = ACPI_HANDLE(rp->bus->bridge);
+
+	/* invoke _DSM to extract tag value */
+	if (tph_invoke_dsm(rp_acpi_handle, cpu_acpi_uid, 0, 0, false, 0, &info) != AE_OK) {
+		*tag = 0;
+		return -EINVAL;
+	}
+
+	*tag = tph_extract_tag(mem_type, req_type, &info);
+	pci_dbg(pdev, "%s: cpu=%d tag=%d\n", __func__, cpu_acpi_uid, *tag);
+
+	return 0;
+}
+EXPORT_SYMBOL(pcie_tph_get_st_from_acpi);
diff --git a/include/linux/pci-tph.h b/include/linux/pci-tph.h
index 854677651d81..b12a592f3d49 100644
--- a/include/linux/pci-tph.h
+++ b/include/linux/pci-tph.h
@@ -9,15 +9,27 @@ 
 #ifndef LINUX_PCI_TPH_H
 #define LINUX_PCI_TPH_H
 
+enum tph_mem_type {
+	TPH_MEM_TYPE_VM,	/* volatile memory type */
+	TPH_MEM_TYPE_PM		/* persistent memory type */
+};
+
 #ifdef CONFIG_PCIE_TPH
 void pcie_tph_disable(struct pci_dev *dev);
 void pcie_tph_set_nostmode(struct pci_dev *dev);
 bool pcie_tph_intr_vec_supported(struct pci_dev *dev);
+int pcie_tph_get_st_from_acpi(struct pci_dev *dev, unsigned int cpu_acpi_uid,
+			      enum tph_mem_type tag_type, u8 req_enable,
+			      u16 *tag);
 #else
 static inline void pcie_tph_disable(struct pci_dev *dev) {}
 static inline void pcie_tph_set_nostmode(struct pci_dev *dev) {}
 static inline bool pcie_tph_intr_vec_supported(struct pci_dev *dev)
 { return false; }
+static inline int pcie_tph_get_st_from_acpi(struct pci_dev *dev, unsigned int cpu_acpi_uid,
+					    enum tph_mem_type tag_type, u8 req_enable,
+					    u16 *tag)
+{ return false; }
 #endif
 
 #endif /* LINUX_PCI_TPH_H */