[PATCH v2 0/2] Support BMG PMT features for Xe

[PATCH v2 0/2] Support BMG PMT features for Xe

[Michael J. Ruhl]

Updates for PMT to support user offsets from the sysfs API.

Addressed review comments for the Xe driver udpates.

Michael J. Ruhl (2):
  platform/x86/intel/pmt: allow user offset for PMT callbacks
  drm/xe/vsec: Support BMG devices

 drivers/gpu/drm/xe/Makefile                |   1 +
 drivers/gpu/drm/xe/regs/xe_pmt.h           |  19 ++
 drivers/gpu/drm/xe/xe_device.c             |   7 +
 drivers/gpu/drm/xe/xe_device_types.h       |   6 +
 drivers/gpu/drm/xe/xe_vsec.c               | 232 +++++++++++++++++++++
 drivers/gpu/drm/xe/xe_vsec.h               |  11 +
 drivers/platform/x86/intel/pmt/class.c     |  10 +-
 drivers/platform/x86/intel/pmt/class.h     |   2 +-
 drivers/platform/x86/intel/pmt/telemetry.c |   2 +-
 include/linux/intel_vsec.h                 |   3 +-
 10 files changed, 285 insertions(+), 8 deletions(-)
 create mode 100644 drivers/gpu/drm/xe/regs/xe_pmt.h
 create mode 100644 drivers/gpu/drm/xe/xe_vsec.c
 create mode 100644 drivers/gpu/drm/xe/xe_vsec.h

-- 
2.44.0

[PATCH v2 1/2] platform/x86/intel/pmt: allow user offset for PMT callbacks

[Michael J. Ruhl]

Usage of the telem sysfs file allows for partial reads at
an offset.

The current callback method returns the buffer starting
from offset 0 only.

Include the requested offset in the callback.
Update the necessary address calculations with the offset.

Note: offset addition is moved from the caller to the local
usage.  For non-callback usage this unchanged behavior.

Fixes: e92affc74cd8 ("platform/x86/intel/vsec: Add PMT read callbacks")
Signed-off-by: Michael J. Ruhl <michael.j.ruhl@intel.com>
---
 drivers/platform/x86/intel/pmt/class.c     | 10 +++++-----
 drivers/platform/x86/intel/pmt/class.h     |  2 +-
 drivers/platform/x86/intel/pmt/telemetry.c |  2 +-
 include/linux/intel_vsec.h                 |  3 ++-
 4 files changed, 9 insertions(+), 8 deletions(-)

diff --git a/drivers/platform/x86/intel/pmt/class.c b/drivers/platform/x86/intel/pmt/class.c
index c04bb7f97a4d..657d72b9e675 100644
--- a/drivers/platform/x86/intel/pmt/class.c
+++ b/drivers/platform/x86/intel/pmt/class.c
@@ -59,16 +59,16 @@ pmt_memcpy64_fromio(void *to, const u64 __iomem *from, size_t count)
 }
 
 int pmt_telem_read_mmio(struct pci_dev *pdev, struct pmt_callbacks *cb, u32 guid, void *buf,
-			void __iomem *addr, u32 count)
+			void __iomem *addr, loff_t off, u32 count)
 {
 	if (cb && cb->read_telem)
-		return cb->read_telem(pdev, guid, buf, count);
+		return cb->read_telem(pdev, guid, buf, off, count);
 
 	if (guid == GUID_SPR_PUNIT)
 		/* PUNIT on SPR only supports aligned 64-bit read */
-		return pmt_memcpy64_fromio(buf, addr, count);
+		return pmt_memcpy64_fromio(buf, addr + off, count);
 
-	memcpy_fromio(buf, addr, count);
+	memcpy_fromio(buf, addr + off, count);
 
 	return count;
 }
@@ -96,7 +96,7 @@ intel_pmt_read(struct file *filp, struct kobject *kobj,
 		count = entry->size - off;
 
 	count = pmt_telem_read_mmio(entry->ep->pcidev, entry->cb, entry->header.guid, buf,
-				    entry->base + off, count);
+				    entry->base, off, count);
 
 	return count;
 }
diff --git a/drivers/platform/x86/intel/pmt/class.h b/drivers/platform/x86/intel/pmt/class.h
index a267ac964423..b2006d57779d 100644
--- a/drivers/platform/x86/intel/pmt/class.h
+++ b/drivers/platform/x86/intel/pmt/class.h
@@ -62,7 +62,7 @@ struct intel_pmt_namespace {
 };
 
 int pmt_telem_read_mmio(struct pci_dev *pdev, struct pmt_callbacks *cb, u32 guid, void *buf,
-			void __iomem *addr, u32 count);
+			void __iomem *addr, loff_t off, u32 count);
 bool intel_pmt_is_early_client_hw(struct device *dev);
 int intel_pmt_dev_create(struct intel_pmt_entry *entry,
 			 struct intel_pmt_namespace *ns,
diff --git a/drivers/platform/x86/intel/pmt/telemetry.c b/drivers/platform/x86/intel/pmt/telemetry.c
index c9feac859e57..0cea617c6c2e 100644
--- a/drivers/platform/x86/intel/pmt/telemetry.c
+++ b/drivers/platform/x86/intel/pmt/telemetry.c
@@ -219,7 +219,7 @@ int pmt_telem_read(struct telem_endpoint *ep, u32 id, u64 *data, u32 count)
 	if (offset + NUM_BYTES_QWORD(count) > size)
 		return -EINVAL;
 
-	pmt_telem_read_mmio(ep->pcidev, ep->cb, ep->header.guid, data, ep->base + offset,
+	pmt_telem_read_mmio(ep->pcidev, ep->cb, ep->header.guid, data, ep->base, offset,
 			    NUM_BYTES_QWORD(count));
 
 	return ep->present ? 0 : -EPIPE;
diff --git a/include/linux/intel_vsec.h b/include/linux/intel_vsec.h
index 11ee185566c3..b94beab64610 100644
--- a/include/linux/intel_vsec.h
+++ b/include/linux/intel_vsec.h
@@ -74,10 +74,11 @@ enum intel_vsec_quirks {
  * @pdev:  PCI device reference for the callback's use
  * @guid:  ID of data to acccss
  * @data:  buffer for the data to be copied
+ * @off:   offset into the requested buffer
  * @count: size of buffer
  */
 struct pmt_callbacks {
-	int (*read_telem)(struct pci_dev *pdev, u32 guid, u64 *data, u32 count);
+	int (*read_telem)(struct pci_dev *pdev, u32 guid, u64 *data, loff_t off, u32 count);
 };
 
 /**
-- 
2.44.0

[PATCH v2 2/2] drm/xe/vsec: Support BMG devices

[Michael J. Ruhl]

The Battlemage (BMG) discrete graphics card supports the
Platform, Monitoring Technology (PMT) feature directly
on the primary PCI device.

Utilize the PMT callback API to add support for the BMG
devices.

Reviewed-by: Ilpo Järvinen <ilpo.jarvinen@linux.intel.com>
Reviewed-by: Rodrigo Vivi <rodrigo.vivi@intel.com>
Signed-off-by: Michael J. Ruhl <michael.j.ruhl@intel.com>
---
 drivers/gpu/drm/xe/Makefile          |   1 +
 drivers/gpu/drm/xe/regs/xe_pmt.h     |  19 +++
 drivers/gpu/drm/xe/xe_device.c       |   7 +
 drivers/gpu/drm/xe/xe_device_types.h |   6 +
 drivers/gpu/drm/xe/xe_vsec.c         | 232 +++++++++++++++++++++++++++
 drivers/gpu/drm/xe/xe_vsec.h         |  11 ++
 6 files changed, 276 insertions(+)
 create mode 100644 drivers/gpu/drm/xe/regs/xe_pmt.h
 create mode 100644 drivers/gpu/drm/xe/xe_vsec.c
 create mode 100644 drivers/gpu/drm/xe/xe_vsec.h

diff --git a/drivers/gpu/drm/xe/Makefile b/drivers/gpu/drm/xe/Makefile
index a93e6fcc0ad9..7730e0596299 100644
--- a/drivers/gpu/drm/xe/Makefile
+++ b/drivers/gpu/drm/xe/Makefile
@@ -111,6 +111,7 @@ xe-y += xe_bb.o \
 	xe_vm.o \
 	xe_vram.o \
 	xe_vram_freq.o \
+	xe_vsec.o \
 	xe_wait_user_fence.o \
 	xe_wa.o \
 	xe_wopcm.o
diff --git a/drivers/gpu/drm/xe/regs/xe_pmt.h b/drivers/gpu/drm/xe/regs/xe_pmt.h
new file mode 100644
index 000000000000..f45abcd96ba8
--- /dev/null
+++ b/drivers/gpu/drm/xe/regs/xe_pmt.h
@@ -0,0 +1,19 @@
+/* SPDX-License-Identifier: MIT */
+/*
+ * Copyright © 2024 Intel Corporation
+ */
+#ifndef _XE_PMT_H_
+#define _XE_PMT_H_
+
+#define SOC_BASE			0x280000
+
+#define BMG_PMT_BASE_OFFSET		0xDB000
+#define BMG_DISCOVERY_OFFSET		(SOC_BASE + BMG_PMT_BASE_OFFSET)
+
+#define BMG_TELEMETRY_BASE_OFFSET	0xE0000
+#define BMG_TELEMETRY_OFFSET		(SOC_BASE + BMG_TELEMETRY_BASE_OFFSET)
+
+#define SG_REMAP_INDEX1			XE_REG(SOC_BASE + 0x08)
+#define   SG_REMAP_BITS			REG_GENMASK(31, 24)
+
+#endif
diff --git a/drivers/gpu/drm/xe/xe_device.c b/drivers/gpu/drm/xe/xe_device.c
index 0e2dd691bdae..51da1ea4d39b 100644
--- a/drivers/gpu/drm/xe/xe_device.c
+++ b/drivers/gpu/drm/xe/xe_device.c
@@ -55,6 +55,7 @@
 #include "xe_ttm_sys_mgr.h"
 #include "xe_vm.h"
 #include "xe_vram.h"
+#include "xe_vsec.h"
 #include "xe_wait_user_fence.h"
 #include "xe_wa.h"
 
@@ -365,6 +366,10 @@ struct xe_device *xe_device_create(struct pci_dev *pdev,
 		goto err;
 	}
 
+	err = drmm_mutex_init(&xe->drm, &xe->pmt.lock);
+	if (err)
+		goto err;
+
 	err = xe_display_create(xe);
 	if (WARN_ON(err))
 		goto err;
@@ -759,6 +764,8 @@ int xe_device_probe(struct xe_device *xe)
 	for_each_gt(gt, xe, id)
 		xe_gt_sanitize_freq(gt);
 
+	xe_vsec_init(xe);
+
 	return devm_add_action_or_reset(xe->drm.dev, xe_device_sanitize, xe);
 
 err_fini_display:
diff --git a/drivers/gpu/drm/xe/xe_device_types.h b/drivers/gpu/drm/xe/xe_device_types.h
index bccca63c8a48..13164ea2bddb 100644
--- a/drivers/gpu/drm/xe/xe_device_types.h
+++ b/drivers/gpu/drm/xe/xe_device_types.h
@@ -483,6 +483,12 @@ struct xe_device {
 		struct mutex lock;
 	} d3cold;
 
+	/** @pmt: Support the PMT driver callback interface */
+	struct {
+		/** @pmt.lock: protect access for telemetry data */
+		struct mutex lock;
+	} pmt;
+
 	/**
 	 * @pm_callback_task: Track the active task that is running in either
 	 * the runtime_suspend or runtime_resume callbacks.
diff --git a/drivers/gpu/drm/xe/xe_vsec.c b/drivers/gpu/drm/xe/xe_vsec.c
new file mode 100644
index 000000000000..4c2d5c6e293a
--- /dev/null
+++ b/drivers/gpu/drm/xe/xe_vsec.c
@@ -0,0 +1,232 @@
+// SPDX-License-Identifier: GPL-2.0
+/* Copyright © 2024 Intel Corporation */
+#include <linux/bitfield.h>
+#include <linux/bits.h>
+#include <linux/cleanup.h>
+#include <linux/errno.h>
+#include <linux/intel_vsec.h>
+#include <linux/module.h>
+#include <linux/mutex.h>
+#include <linux/pci.h>
+#include <linux/types.h>
+
+#include "xe_device.h"
+#include "xe_device_types.h"
+#include "xe_drv.h"
+#include "xe_mmio.h"
+#include "xe_platform_types.h"
+#include "xe_pm.h"
+#include "xe_vsec.h"
+
+#include "regs/xe_pmt.h"
+
+/* PMT GUID value for BMG devices.  NOTE: this is NOT a PCI id */
+#define BMG_DEVICE_ID 0xE2F8
+
+static struct intel_vsec_header bmg_telemetry = {
+	.length = 0x10,
+	.id = VSEC_ID_TELEMETRY,
+	.num_entries = 2,
+	.entry_size = 4,
+	.tbir = 0,
+	.offset = BMG_DISCOVERY_OFFSET,
+};
+
+static struct intel_vsec_header bmg_punit_crashlog = {
+	.length = 0x10,
+	.id = VSEC_ID_CRASHLOG,
+	.num_entries = 1,
+	.entry_size = 4,
+	.tbir = 0,
+	.offset = BMG_DISCOVERY_OFFSET + 0x60,
+};
+
+static struct intel_vsec_header bmg_oobmsm_crashlog = {
+	.length = 0x10,
+	.id = VSEC_ID_CRASHLOG,
+	.num_entries = 1,
+	.entry_size = 4,
+	.tbir = 0,
+	.offset = BMG_DISCOVERY_OFFSET + 0x78,
+};
+
+static struct intel_vsec_header *bmg_capabilities[] = {
+	&bmg_telemetry,
+	&bmg_punit_crashlog,
+	&bmg_oobmsm_crashlog,
+	NULL
+};
+
+enum xe_vsec {
+	XE_VSEC_UNKNOWN = 0,
+	XE_VSEC_BMG,
+};
+
+static struct intel_vsec_platform_info xe_vsec_info[] = {
+	[XE_VSEC_BMG] = {
+		.caps = VSEC_CAP_TELEMETRY | VSEC_CAP_CRASHLOG,
+		.headers = bmg_capabilities,
+	},
+	{ }
+};
+
+/*
+ * The GUID will have the following bits to decode:
+ *   [0:3]   - {Telemetry space iteration number (0,1,..)}
+ *   [4:7]   - Segment (SEGMENT_INDEPENDENT-0, Client-1, Server-2)
+ *   [8:11]  - SOC_SKU
+ *   [12:27] – Device ID – changes for each down bin SKU’s
+ *   [28:29] - Capability Type (Crashlog-0, Telemetry Aggregator-1, Watcher-2)
+ *   [30:31] - Record-ID (0-PUNIT, 1-OOBMSM_0, 2-OOBMSM_1)
+ */
+#define GUID_TELEM_ITERATION	GENMASK(3, 0)
+#define GUID_SEGMENT		GENMASK(7, 4)
+#define GUID_SOC_SKU		GENMASK(11, 8)
+#define GUID_DEVICE_ID		GENMASK(27, 12)
+#define GUID_CAP_TYPE		GENMASK(29, 28)
+#define GUID_RECORD_ID		GENMASK(31, 30)
+
+#define PUNIT_TELEMETRY_OFFSET		0x0200
+#define PUNIT_WATCHER_OFFSET		0x14A0
+#define OOBMSM_0_WATCHER_OFFSET		0x18D8
+#define OOBMSM_1_TELEMETRY_OFFSET	0x1000
+
+enum record_id {
+	PUNIT,
+	OOBMSM_0,
+	OOBMSM_1,
+};
+
+enum capability {
+	CRASHLOG,
+	TELEMETRY,
+	WATCHER,
+};
+
+static int xe_guid_decode(u32 guid, int *index, u32 *offset)
+{
+	u32 record_id = FIELD_GET(GUID_RECORD_ID, guid);
+	u32 cap_type  = FIELD_GET(GUID_CAP_TYPE, guid);
+	u32 device_id = FIELD_GET(GUID_DEVICE_ID, guid);
+
+	if (device_id != BMG_DEVICE_ID)
+		return -ENODEV;
+
+	if (cap_type > WATCHER)
+		return -EINVAL;
+
+	*offset = 0;
+
+	if (cap_type == CRASHLOG) {
+		*index = record_id == PUNIT ? 2 : 4;
+		return 0;
+	}
+
+	switch (record_id) {
+	case PUNIT:
+		*index = 0;
+		if (cap_type == TELEMETRY)
+			*offset = PUNIT_TELEMETRY_OFFSET;
+		else
+			*offset = PUNIT_WATCHER_OFFSET;
+		break;
+
+	case OOBMSM_0:
+		*index = 1;
+		if (cap_type == WATCHER)
+			*offset = OOBMSM_0_WATCHER_OFFSET;
+		break;
+
+	case OOBMSM_1:
+		*index = 1;
+		if (cap_type == TELEMETRY)
+			*offset = OOBMSM_1_TELEMETRY_OFFSET;
+		break;
+	default:
+		return -EINVAL;
+	}
+
+	return 0;
+}
+
+static int xe_pmt_telem_read(struct pci_dev *pdev, u32 guid, u64 *data, loff_t user_offset, u32 count)
+{
+	struct xe_device *xe = pdev_to_xe_device(pdev);
+	void __iomem *telem_addr = xe->mmio.regs + BMG_TELEMETRY_OFFSET;
+	u32 mem_region;
+	u32 offset;
+	int ret;
+
+	ret = xe_guid_decode(guid, &mem_region, &offset);
+	if (ret)
+		return ret;
+
+	telem_addr += offset + user_offset;
+
+	guard(mutex)(&xe->pmt.lock);
+
+	/* indicate that we are not at an appropriate power level */
+	if (!xe_pm_runtime_get_if_active(xe))
+		return -ENODATA;
+
+	/* set SoC re-mapper index register based on GUID memory region */
+	xe_mmio_rmw32(xe_root_tile_mmio(xe), SG_REMAP_INDEX1, SG_REMAP_BITS,
+		      REG_FIELD_PREP(SG_REMAP_BITS, mem_region));
+
+	memcpy_fromio(data, telem_addr, count);
+	xe_pm_runtime_put(xe);
+
+	return count;
+}
+
+struct pmt_callbacks xe_pmt_cb = {
+	.read_telem = xe_pmt_telem_read,
+};
+
+static const int vsec_platforms[] = {
+	[XE_BATTLEMAGE] = XE_VSEC_BMG,
+};
+
+static enum xe_vsec get_platform_info(struct xe_device *xe)
+{
+	if (xe->info.platform > XE_BATTLEMAGE)
+		return XE_VSEC_UNKNOWN;
+
+	return vsec_platforms[xe->info.platform];
+}
+
+/**
+ * xe_vsec_init - Initialize resources and add intel_vsec auxiliary
+ * interface
+ * @xe: valid xe instance
+ */
+void xe_vsec_init(struct xe_device *xe)
+{
+	struct intel_vsec_platform_info *info;
+	struct device *dev = xe->drm.dev;
+	struct pci_dev *pdev = to_pci_dev(dev);
+	enum xe_vsec platform;
+
+	platform = get_platform_info(xe);
+	if (platform == XE_VSEC_UNKNOWN)
+		return;
+
+	info = &xe_vsec_info[platform];
+	if (!info->headers)
+		return;
+
+	switch (platform) {
+	case XE_VSEC_BMG:
+		info->priv_data = &xe_pmt_cb;
+		break;
+	default:
+		break;
+	}
+
+	/*
+	 * Register a VSEC. Cleanup is handled using device managed
+	 * resources.
+	 */
+	intel_vsec_register(pdev, info);
+}
+MODULE_IMPORT_NS(INTEL_VSEC);
diff --git a/drivers/gpu/drm/xe/xe_vsec.h b/drivers/gpu/drm/xe/xe_vsec.h
new file mode 100644
index 000000000000..5777c53faec2
--- /dev/null
+++ b/drivers/gpu/drm/xe/xe_vsec.h
@@ -0,0 +1,11 @@
+/* SPDX-License-Identifier: GPL-2.0 */
+/* Copyright © 2024 Intel Corporation */
+
+#ifndef _XE_VSEC_H_
+#define _XE_VSEC_H_
+
+struct xe_device;
+
+void xe_vsec_init(struct xe_device *xe);
+
+#endif
-- 
2.44.0

Re: [PATCH v2 0/2] Support BMG PMT features for Xe

[Andy Shevchenko]

On Tue, Nov 12, 2024 at 11:30:33AM -0500, Michael J. Ruhl wrote:
> Updates for PMT to support user offsets from the sysfs API.
> 
> Addressed review comments for the Xe driver udpates.

Not sure where the changelog (v1 --> v2) is...

-- 
With Best Regards,
Andy Shevchenko

Re: [PATCH v2 1/2] platform/x86/intel/pmt: allow user offset for PMT callbacks

[Andy Shevchenko]

On Tue, Nov 12, 2024 at 11:30:34AM -0500, Michael J. Ruhl wrote:
> Usage of the telem sysfs file allows for partial reads at
> an offset.
> 
> The current callback method returns the buffer starting
> from offset 0 only.
> 
> Include the requested offset in the callback.
> Update the necessary address calculations with the offset.
> 
> Note: offset addition is moved from the caller to the local
> usage.  For non-callback usage this unchanged behavior.

...

>  int pmt_telem_read_mmio(struct pci_dev *pdev, struct pmt_callbacks *cb, u32 guid, void *buf,
> -			void __iomem *addr, u32 count)
> +			void __iomem *addr, loff_t off, u32 count)
>  {
>  	if (cb && cb->read_telem)
> -		return cb->read_telem(pdev, guid, buf, count);
> +		return cb->read_telem(pdev, guid, buf, off, count);

Also possible instead of the below changes is to add here

	addr += off;

>  	if (guid == GUID_SPR_PUNIT)
>  		/* PUNIT on SPR only supports aligned 64-bit read */
> -		return pmt_memcpy64_fromio(buf, addr, count);
> +		return pmt_memcpy64_fromio(buf, addr + off, count);
>  
> -	memcpy_fromio(buf, addr, count);
> +	memcpy_fromio(buf, addr + off, count);
>  
>  	return count;
>  }

-- 
With Best Regards,
Andy Shevchenko

Re: [PATCH v2 0/2] Support BMG PMT features for Xe

[Andy Shevchenko]

On Tue, Nov 12, 2024 at 11:30:33AM -0500, Michael J. Ruhl wrote:
> Updates for PMT to support user offsets from the sysfs API.
> 
> Addressed review comments for the Xe driver udpates.

FWIW,
Reviewed-by: Andy Shevchenko <andriy.shevchenko@linux.intel.com>

If you have wish and time, there are problems with the drivers of different
severities (from "fine as is" to "good to be fixed, but okay as is") I have
noticed so far:
- it uses s*printf() instead of sysfs_emit*()
- it most likely never tested the corner cases. e.g.,

	if (disc_res->start >= pci_resource_start(pci_dev, i) &&
	    (disc_res->start <= pci_resource_end(pci_dev, i))) {

  what is this supposed to mean? Probably someone wanted resource_contains() or
  alike to be called here.
- slightly above the above piece the for-loop

	for (i = 0; i < 6; i++)

  which probably want to use PCI_STD_RESOURCE_END)

-- 
With Best Regards,
Andy Shevchenko

Re: [PATCH v2 0/2] Support BMG PMT features for Xe

[Ilpo Järvinen]

On Wed, 13 Nov 2024, Andy Shevchenko wrote:

> On Tue, Nov 12, 2024 at 11:30:33AM -0500, Michael J. Ruhl wrote:
> > Updates for PMT to support user offsets from the sysfs API.
> > 
> > Addressed review comments for the Xe driver udpates.
> 
> FWIW,
> Reviewed-by: Andy Shevchenko <andriy.shevchenko@linux.intel.com>
> 
> If you have wish and time, there are problems with the drivers of different
> severities (from "fine as is" to "good to be fixed, but okay as is") I have
> noticed so far:
> - it uses s*printf() instead of sysfs_emit*()
> - it most likely never tested the corner cases. e.g.,
> 
> 	if (disc_res->start >= pci_resource_start(pci_dev, i) &&
> 	    (disc_res->start <= pci_resource_end(pci_dev, i))) {
> 
>   what is this supposed to mean? Probably someone wanted resource_contains() or
>   alike to be called here.
> - slightly above the above piece the for-loop
> 
> 	for (i = 0; i < 6; i++)
> 
>   which probably want to use PCI_STD_RESOURCE_END)

While both work, in practice PCI_STD_NUM_BARS is way more common than 
PCI_STD_RESOURCE_END.

-- 
 i.

Re: [PATCH v2 0/2] Support BMG PMT features for Xe

[Andy Shevchenko]

On Wed, Nov 13, 2024 at 03:52:01PM +0200, Ilpo Järvinen wrote:
> On Wed, 13 Nov 2024, Andy Shevchenko wrote:
> > On Tue, Nov 12, 2024 at 11:30:33AM -0500, Michael J. Ruhl wrote:
> > > Updates for PMT to support user offsets from the sysfs API.
> > > 
> > > Addressed review comments for the Xe driver udpates.
> > 
> > FWIW,
> > Reviewed-by: Andy Shevchenko <andriy.shevchenko@linux.intel.com>
> > 
> > If you have wish and time, there are problems with the drivers of different
> > severities (from "fine as is" to "good to be fixed, but okay as is") I have
> > noticed so far:
> > - it uses s*printf() instead of sysfs_emit*()
> > - it most likely never tested the corner cases. e.g.,
> > 
> > 	if (disc_res->start >= pci_resource_start(pci_dev, i) &&
> > 	    (disc_res->start <= pci_resource_end(pci_dev, i))) {
> > 
> >   what is this supposed to mean? Probably someone wanted resource_contains() or
> >   alike to be called here.
> > - slightly above the above piece the for-loop
> > 
> > 	for (i = 0; i < 6; i++)
> > 
> >   which probably want to use PCI_STD_RESOURCE_END)
> 
> While both work, in practice PCI_STD_NUM_BARS is way more common than 
> PCI_STD_RESOURCE_END.

Ah, indeed. I always forget that we have two.

-- 
With Best Regards,
Andy Shevchenko

RE: [PATCH v2 0/2] Support BMG PMT features for Xe

[Ruhl, Michael J]

> -----Original Message-----
> From: Andy Shevchenko <andriy.shevchenko@linux.intel.com>
> Sent: Wednesday, November 13, 2024 5:39 AM
> To: Ruhl, Michael J <michael.j.ruhl@intel.com>
> Cc: intel-xe@lists.freedesktop.org; platform-driver-x86@vger.kernel.org;
> david.e.box@linux.intel.com; ilpo.jarvinen@linux.intel.com;
> hdegoede@redhat.com; Vivi, Rodrigo <rodrigo.vivi@intel.com>; De Marchi,
> Lucas <lucas.demarchi@intel.com>
> Subject: Re: [PATCH v2 0/2] Support BMG PMT features for Xe
> 
> On Tue, Nov 12, 2024 at 11:30:33AM -0500, Michael J. Ruhl wrote:
> > Updates for PMT to support user offsets from the sysfs API.
> >
> > Addressed review comments for the Xe driver udpates.
> 
> FWIW,
> Reviewed-by: Andy Shevchenko <andriy.shevchenko@linux.intel.com>

Thank you!
 
> If you have wish and time, there are problems with the drivers of different

I am working on this patch set in my spare time...If anything loosens up I will keep this
in mind.

M

> severities (from "fine as is" to "good to be fixed, but okay as is") I have noticed
> so far:
> - it uses s*printf() instead of sysfs_emit*()
> - it most likely never tested the corner cases. e.g.,
> 
> 	if (disc_res->start >= pci_resource_start(pci_dev, i) &&
> 	    (disc_res->start <= pci_resource_end(pci_dev, i))) {
> 
>   what is this supposed to mean? Probably someone wanted
> resource_contains() or
>   alike to be called here.
> - slightly above the above piece the for-loop
> 
> 	for (i = 0; i < 6; i++)
> 
>   which probably want to use PCI_STD_RESOURCE_END)
> 
> --
> With Best Regards,
> Andy Shevchenko
> 

Re: [PATCH v2 0/2] Support BMG PMT features for Xe

[David E. Box]

On Wed, 2024-11-13 at 15:52 +0200, Ilpo Järvinen wrote:
> On Wed, 13 Nov 2024, Andy Shevchenko wrote:
> 
> > On Tue, Nov 12, 2024 at 11:30:33AM -0500, Michael J. Ruhl wrote:
> > > Updates for PMT to support user offsets from the sysfs API.
> > > 
> > > Addressed review comments for the Xe driver udpates.
> > 
> > FWIW,
> > Reviewed-by: Andy Shevchenko <andriy.shevchenko@linux.intel.com>
> > 
> > If you have wish and time, there are problems with the drivers of different
> > severities (from "fine as is" to "good to be fixed, but okay as is") I have
> > noticed so far:
> > - it uses s*printf() instead of sysfs_emit*()
> > - it most likely never tested the corner cases. e.g.,
> > 
> > 	if (disc_res->start >= pci_resource_start(pci_dev, i) &&
> > 	    (disc_res->start <= pci_resource_end(pci_dev, i))) {
> > 
> >   what is this supposed to mean? Probably someone wanted resource_contains()
> > or
> >   alike to be called here.

This is a corner case that occurs for devices that are non-compliant, in this
case meaning devices that don't follow our PMT spec convention of specifying
which BAR an address belongs to. Without this information, we have to deduce the
BAR manually to access other needed registers that are offset from the base of
that BAR.

I can change this to use resource_contains().

> > - slightly above the above piece the for-loop
> > 
> > 	for (i = 0; i < 6; i++)
> > 
> >   which probably want to use PCI_STD_RESOURCE_END)
> 
> While both work, in practice PCI_STD_NUM_BARS is way more common than 
> PCI_STD_RESOURCE_END.
> 

Will change this too. Thanks.

David

Re: [PATCH v2 0/2] Support BMG PMT features for Xe

[Andy Shevchenko]

On Wed, Nov 13, 2024 at 10:40:42AM -0800, David E. Box wrote:
> On Wed, 2024-11-13 at 15:52 +0200, Ilpo Järvinen wrote:
> > On Wed, 13 Nov 2024, Andy Shevchenko wrote:
> > > On Tue, Nov 12, 2024 at 11:30:33AM -0500, Michael J. Ruhl wrote:
> > > > Updates for PMT to support user offsets from the sysfs API.
> > > > 
> > > > Addressed review comments for the Xe driver udpates.
> > > 
> > > FWIW,
> > > Reviewed-by: Andy Shevchenko <andriy.shevchenko@linux.intel.com>
> > > 
> > > If you have wish and time, there are problems with the drivers of different
> > > severities (from "fine as is" to "good to be fixed, but okay as is") I have
> > > noticed so far:
> > > - it uses s*printf() instead of sysfs_emit*()
> > > - it most likely never tested the corner cases. e.g.,
> > > 
> > > 	if (disc_res->start >= pci_resource_start(pci_dev, i) &&
> > > 	    (disc_res->start <= pci_resource_end(pci_dev, i))) {
> > > 
> > >   what is this supposed to mean? Probably someone wanted resource_contains()
> > > or
> > >   alike to be called here.
> 
> This is a corner case that occurs for devices that are non-compliant, in this
> case meaning devices that don't follow our PMT spec convention of specifying
> which BAR an address belongs to. Without this information, we have to deduce the
> BAR manually to access other needed registers that are offset from the base of
> that BAR.

What I am pointing out is that we compare start address (and only start!) to
both, start _and_ end of the given resource. So currently the second check is
redundant and that looks suspicious. I believe one wanted to have

	if (disc_res->start >= pci_resource_start(pci_dev, i) &&
	    (disc_res->end <= pci_resource_end(pci_dev, i))) {

(note end!) and if using helpers, this would never happened :-)

> I can change this to use resource_contains().

Please, will clarify the above confusion..

> > > - slightly above the above piece the for-loop
> > > 
> > > 	for (i = 0; i < 6; i++)
> > > 
> > >   which probably want to use PCI_STD_RESOURCE_END)
> > 
> > While both work, in practice PCI_STD_NUM_BARS is way more common than 
> > PCI_STD_RESOURCE_END.
> 
> Will change this too. Thanks.

You are welcome!

-- 
With Best Regards,
Andy Shevchenko

Re: [PATCH v2 0/2] Support BMG PMT features for Xe

[David E. Box]

On Wed, 2024-11-13 at 20:59 +0200, Andy Shevchenko wrote:
> On Wed, Nov 13, 2024 at 10:40:42AM -0800, David E. Box wrote:
> > On Wed, 2024-11-13 at 15:52 +0200, Ilpo Järvinen wrote:
> > > On Wed, 13 Nov 2024, Andy Shevchenko wrote:
> > > > On Tue, Nov 12, 2024 at 11:30:33AM -0500, Michael J. Ruhl wrote:
> > > > > Updates for PMT to support user offsets from the sysfs API.
> > > > > 
> > > > > Addressed review comments for the Xe driver udpates.
> > > > 
> > > > FWIW,
> > > > Reviewed-by: Andy Shevchenko <andriy.shevchenko@linux.intel.com>
> > > > 
> > > > If you have wish and time, there are problems with the drivers of
> > > > different
> > > > severities (from "fine as is" to "good to be fixed, but okay as is") I
> > > > have
> > > > noticed so far:
> > > > - it uses s*printf() instead of sysfs_emit*()
> > > > - it most likely never tested the corner cases. e.g.,
> > > > 
> > > > 	if (disc_res->start >= pci_resource_start(pci_dev, i) &&
> > > > 	    (disc_res->start <= pci_resource_end(pci_dev, i))) {
> > > > 
> > > >   what is this supposed to mean? Probably someone wanted
> > > > resource_contains()
> > > > or
> > > >   alike to be called here.
> > 
> > This is a corner case that occurs for devices that are non-compliant, in
> > this
> > case meaning devices that don't follow our PMT spec convention of specifying
> > which BAR an address belongs to. Without this information, we have to deduce
> > the
> > BAR manually to access other needed registers that are offset from the base
> > of
> > that BAR.
> 
> What I am pointing out is that we compare start address (and only start!) to
> both, start _and_ end of the given resource. So currently the second check is
> redundant and that looks suspicious. I believe one wanted to have
> 
> 	if (disc_res->start >= pci_resource_start(pci_dev, i) &&
> 	    (disc_res->end <= pci_resource_end(pci_dev, i))) {

I see. The assumption was that the device didn't provide us an address range
that crosses BARs, so testing that the start address is within a BAR would be
enough. But the above is a better check in case that's not true which would be a
hardware bug.

David

> 
> (note end!) and if using helpers, this would never happened :-)
> 
> > I can change this to use resource_contains().
> 
> Please, will clarify the above confusion..
> 
> > > > - slightly above the above piece the for-loop
> > > > 
> > > > 	for (i = 0; i < 6; i++)
> > > > 
> > > >   which probably want to use PCI_STD_RESOURCE_END)
> > > 
> > > While both work, in practice PCI_STD_NUM_BARS is way more common than 
> > > PCI_STD_RESOURCE_END.
> > 
> > Will change this too. Thanks.
> 
> You are welcome!
> 

RE: [PATCH v2 1/2] platform/x86/intel/pmt: allow user offset for PMT callbacks

[Ruhl, Michael J]

> -----Original Message-----
> From: Andy Shevchenko <andriy.shevchenko@linux.intel.com>
> Sent: Wednesday, November 13, 2024 5:26 AM
> To: Ruhl, Michael J <michael.j.ruhl@intel.com>
> Cc: intel-xe@lists.freedesktop.org; platform-driver-x86@vger.kernel.org;
> david.e.box@linux.intel.com; ilpo.jarvinen@linux.intel.com;
> hdegoede@redhat.com; Vivi, Rodrigo <rodrigo.vivi@intel.com>; De Marchi,
> Lucas <lucas.demarchi@intel.com>
> Subject: Re: [PATCH v2 1/2] platform/x86/intel/pmt: allow user offset for PMT
> callbacks
> 
> On Tue, Nov 12, 2024 at 11:30:34AM -0500, Michael J. Ruhl wrote:
> > Usage of the telem sysfs file allows for partial reads at an offset.
> >
> > The current callback method returns the buffer starting from offset 0
> > only.
> >
> > Include the requested offset in the callback.
> > Update the necessary address calculations with the offset.
> >
> > Note: offset addition is moved from the caller to the local usage.
> > For non-callback usage this unchanged behavior.
> 
> ...
> 
> >  int pmt_telem_read_mmio(struct pci_dev *pdev, struct pmt_callbacks *cb,
> u32 guid, void *buf,
> > -			void __iomem *addr, u32 count)
> > +			void __iomem *addr, loff_t off, u32 count)
> >  {
> >  	if (cb && cb->read_telem)
> > -		return cb->read_telem(pdev, guid, buf, count);
> > +		return cb->read_telem(pdev, guid, buf, off, count);
> 
> Also possible instead of the below changes is to add here
> 
> 	addr += off;

Yeah, that makes sense.   Was not happy with adding it twice.  I will update.

Thanks!

M

 
> >  	if (guid == GUID_SPR_PUNIT)
> >  		/* PUNIT on SPR only supports aligned 64-bit read */
> > -		return pmt_memcpy64_fromio(buf, addr, count);
> > +		return pmt_memcpy64_fromio(buf, addr + off, count);
> >
> > -	memcpy_fromio(buf, addr, count);
> > +	memcpy_fromio(buf, addr + off, count);
> >
> >  	return count;
> >  }
> 
> --
> With Best Regards,
> Andy Shevchenko
>