Re: [PATCH 04/11] platform/x86/intel/pmt: telemetry: Export API to read telemetry
From: David E. Box
Date: Tue Sep 26 2023 - 20:46:20 EST
On Tue, 2023-09-26 at 18:40 +0300, Ilpo Järvinen wrote:
> On Fri, 22 Sep 2023, David E. Box wrote:
>
> > Export symbols to allow access to Intel PMT Telemetry data on available
> > devices. Provides APIs to search, register, and read telemetry using a
> > kref managed pointer that serves as a handle to a telemetry endpoint.
> >
> > Signed-off-by: David E. Box <david.e.box@xxxxxxxxxxxxxxx>
> > ---
> > drivers/platform/x86/intel/pmt/class.c | 21 ++-
> > drivers/platform/x86/intel/pmt/class.h | 14 ++
> > drivers/platform/x86/intel/pmt/telemetry.c | 198 ++++++++++++++++++++-
> > drivers/platform/x86/intel/pmt/telemetry.h | 129 ++++++++++++++
> > 4 files changed, 354 insertions(+), 8 deletions(-)
> > create mode 100644 drivers/platform/x86/intel/pmt/telemetry.h
> >
> > diff --git a/drivers/platform/x86/intel/pmt/class.c
> > b/drivers/platform/x86/intel/pmt/class.c
> > index 142a24e3727d..4b53940a64e2 100644
> > --- a/drivers/platform/x86/intel/pmt/class.c
> > +++ b/drivers/platform/x86/intel/pmt/class.c
> > @@ -17,7 +17,7 @@
> > #include "../vsec.h"
> > #include "class.h"
> >
> > -#define PMT_XA_START 0
> > +#define PMT_XA_START 1
>
> How is that related to what the changelog states, that is, exporting some
> API???
...
>
> > #define PMT_XA_MAX INT_MAX
> > #define PMT_XA_LIMIT XA_LIMIT(PMT_XA_START, PMT_XA_MAX)
> > #define GUID_SPR_PUNIT 0x9956f43f
> > @@ -247,6 +247,7 @@ static int intel_pmt_dev_register(struct intel_pmt_entry
> > *entry,
> > struct intel_pmt_namespace *ns,
> > struct device *parent)
> > {
> > + struct intel_vsec_device *ivdev = dev_to_ivdev(parent);
> > struct resource res = {0};
> > struct device *dev;
> > int ret;
> > @@ -270,7 +271,7 @@ static int intel_pmt_dev_register(struct intel_pmt_entry
> > *entry,
> > if (ns->attr_grp) {
> > ret = sysfs_create_group(entry->kobj, ns->attr_grp);
> > if (ret)
> > - goto fail_sysfs;
> > + goto fail_sysfs_create_group;
> > }
> >
> > /* if size is 0 assume no data buffer, so no file needed */
> > @@ -295,13 +296,23 @@ static int intel_pmt_dev_register(struct
> > intel_pmt_entry *entry,
> > entry->pmt_bin_attr.size = entry->size;
> >
> > ret = sysfs_create_bin_file(&dev->kobj, &entry->pmt_bin_attr);
> > - if (!ret)
> > - return 0;
> > + if (ret)
> > + goto fail_ioremap;
> >
> > + if (ns->pmt_add_endpoint) {
> > + ret = ns->pmt_add_endpoint(entry, ivdev->pcidev);
> > + if (ret)
> > + goto fail_add_endpoint;
> > + }
> > +
> > + return 0;
> > +
> > +fail_add_endpoint:
> > + sysfs_remove_bin_file(entry->kobj, &entry->pmt_bin_attr);
> > fail_ioremap:
> > if (ns->attr_grp)
> > sysfs_remove_group(entry->kobj, ns->attr_grp);
> > -fail_sysfs:
> > +fail_sysfs_create_group:
> > device_unregister(dev);
> > fail_dev_create:
> > xa_erase(ns->xa, entry->devid);
> > diff --git a/drivers/platform/x86/intel/pmt/class.h
> > b/drivers/platform/x86/intel/pmt/class.h
> > index e477a19f6700..d23c63b73ab7 100644
> > --- a/drivers/platform/x86/intel/pmt/class.h
> > +++ b/drivers/platform/x86/intel/pmt/class.h
> > @@ -9,6 +9,7 @@
> > #include <linux/io.h>
> >
> > #include "../vsec.h"
> > +#include "telemetry.h"
> >
> > /* PMT access types */
> > #define ACCESS_BARID 2
> > @@ -18,6 +19,16 @@
> > #define GET_BIR(v) ((v) & GENMASK(2, 0))
> > #define GET_ADDRESS(v) ((v) & GENMASK(31, 3))
> >
> > +struct pci_dev;
> > +
> > +struct telem_endpoint {
> > + struct pci_dev *pcidev;
> > + struct telem_header header;
> > + void __iomem *base;
> > + bool present;
> > + struct kref kref;
> > +};
> > +
> > struct intel_pmt_header {
> > u32 base_offset;
> > u32 size;
> > @@ -26,6 +37,7 @@ struct intel_pmt_header {
> > };
> >
> > struct intel_pmt_entry {
> > + struct telem_endpoint *ep;
> > struct intel_pmt_header header;
> > struct bin_attribute pmt_bin_attr;
> > struct kobject *kobj;
> > @@ -43,6 +55,8 @@ struct intel_pmt_namespace {
> > const struct attribute_group *attr_grp;
> > int (*pmt_header_decode)(struct intel_pmt_entry *entry,
> > struct device *dev);
> > + int (*pmt_add_endpoint)(struct intel_pmt_entry *entry,
> > + struct pci_dev *pdev);
> > };
> >
> > bool intel_pmt_is_early_client_hw(struct device *dev);
> > diff --git a/drivers/platform/x86/intel/pmt/telemetry.c
> > b/drivers/platform/x86/intel/pmt/telemetry.c
> > index f86080e8bebd..8b099580cc2c 100644
> > --- a/drivers/platform/x86/intel/pmt/telemetry.c
> > +++ b/drivers/platform/x86/intel/pmt/telemetry.c
> > @@ -30,6 +30,14 @@
> > /* Used by client hardware to identify a fixed telemetry entry*/
> > #define TELEM_CLIENT_FIXED_BLOCK_GUID 0x10000000
> >
> > +#define NUM_BYTES_QWORD(v) ((v) << 3)
> > +#define SAMPLE_ID_OFFSET(v) ((v) << 3)
> > +
> > +#define NUM_BYTES_DWORD(v) ((v) << 2)
> > +#define SAMPLE_ID_OFFSET32(v) ((v) << 2)
> > +
> > +static DEFINE_MUTEX(ep_lock);
> > +
> > enum telem_type {
> > TELEM_TYPE_PUNIT = 0,
> > TELEM_TYPE_CRASHLOG,
> > @@ -84,21 +92,203 @@ static int pmt_telem_header_decode(struct
> > intel_pmt_entry *entry,
> > return 0;
> > }
> >
> > +static int pmt_telem_add_endpoint(struct intel_pmt_entry *entry,
> > + struct pci_dev *pdev)
> > +{
> > + struct telem_endpoint *ep;
> > +
> > + /*
> > + * Endpoint lifetimes are managed by kref, not devres.
> > + */
> > + entry->ep = kzalloc(sizeof(*(entry->ep)), GFP_KERNEL);
> > + if (!entry->ep)
> > + return -ENOMEM;
> > +
> > + ep = entry->ep;
> > + ep->pcidev = pdev;
> > + ep->header.access_type = entry->header.access_type;
> > + ep->header.guid = entry->header.guid;
> > + ep->header.base_offset = entry->header.base_offset;
> > + ep->header.size = entry->header.size;
> > + ep->base = entry->base;
> > + ep->present = true;
> > +
> > + kref_init(&ep->kref);
> > +
> > + return 0;
> > +}
> > +
> > static DEFINE_XARRAY_ALLOC(telem_array);
> > static struct intel_pmt_namespace pmt_telem_ns = {
> > .name = "telem",
> > .xa = &telem_array,
> > .pmt_header_decode = pmt_telem_header_decode,
> > + .pmt_add_endpoint = pmt_telem_add_endpoint,
> > };
> >
> > +/* Called when all users unregister and the device is removed */
> > +static void pmt_telem_ep_release(struct kref *kref)
> > +{
> > + struct telem_endpoint *ep;
> > +
> > + ep = container_of(kref, struct telem_endpoint, kref);
> > + kfree(ep);
> > +}
> > +
> > +/*
> > + * driver api
> > + */
> > +int pmt_telem_get_next_endpoint(int start)
> > +{
> > + struct intel_pmt_entry *entry;
> > + unsigned long found_idx;
> > +
> > + mutex_lock(&ep_lock);
> > + xa_for_each_start(&telem_array, found_idx, entry, start) {
> > + /*
> > + * Return first found index after start.
> > + * 0 is not valid id.
> > + */
>
> I guess this has to do with the 0->1 change I flagged above. But if that's
> the case, it absolutely must be mentioned in the changelog with
> explanation!
Yes it does. Will mention in the changelog.
>
> > + if (found_idx > start)
> > + break;
> > + }
> > + mutex_unlock(&ep_lock);
> > +
> > + return found_idx == start ? 0 : found_idx;
>
> Why you need signed values in/out? Should this function just be using
> unsigned int (or long) for start and return value?
It not needed anymore (originally was returning an error). Will change it to
unsigned.
>
> > +}
> > +EXPORT_SYMBOL_NS_GPL(pmt_telem_get_next_endpoint, INTEL_PMT_TELEMETRY);
> > +
> > +struct telem_endpoint *pmt_telem_register_endpoint(int devid)
> > +{
> > + struct intel_pmt_entry *entry;
> > + unsigned long index = devid;
> > +
> > + mutex_lock(&ep_lock);
> > + entry = xa_find(&telem_array, &index, index, XA_PRESENT);
> > + if (!entry) {
> > + mutex_unlock(&ep_lock);
> > + return ERR_PTR(-ENXIO);
> > + }
> > +
> > + kref_get(&entry->ep->kref);
> > + mutex_unlock(&ep_lock);
> > +
> > + return entry->ep;
> > +}
> > +EXPORT_SYMBOL_NS_GPL(pmt_telem_register_endpoint, INTEL_PMT_TELEMETRY);
> > +
> > +void pmt_telem_unregister_endpoint(struct telem_endpoint *ep)
> > +{
> > + kref_put(&ep->kref, pmt_telem_ep_release);
> > +}
> > +EXPORT_SYMBOL_NS_GPL(pmt_telem_unregister_endpoint, INTEL_PMT_TELEMETRY);
> > +
> > +int pmt_telem_get_endpoint_info(int devid,
> > + struct telem_endpoint_info *info)
> > +{
> > + struct intel_pmt_entry *entry;
> > + unsigned long index = devid;
> > + int err = 0;
> > +
> > + if (!info)
> > + return -EINVAL;
> > +
> > + mutex_lock(&ep_lock);
> > + entry = xa_find(&telem_array, &index, index, XA_PRESENT);
> > + if (!entry) {
> > + err = -ENXIO;
> > + goto unlock;
> > + }
> > +
> > + info->pdev = entry->ep->pcidev;
> > + info->header = entry->ep->header;
> > +
> > +unlock:
> > + mutex_unlock(&ep_lock);
> > + return err;
> > +
> > +}
> > +EXPORT_SYMBOL_NS_GPL(pmt_telem_get_endpoint_info, INTEL_PMT_TELEMETRY);
> > +
> > +int
> > +pmt_telem_read(struct telem_endpoint *ep, u32 id, u64 *data, u32 count)
> > +{
> > + u32 offset, size;
> > +
> > + if (!ep->present)
> > + return -ENODEV;
> > +
> > + offset = SAMPLE_ID_OFFSET(id);
> > + size = ep->header.size;
> > +
> > + if ((offset + NUM_BYTES_QWORD(count)) > size)
>
> Extra parenthesis.
Ack
David
>
>