| Message ID | 20240710192249.3915396-6-michael.j.ruhl@intel.com (mailing list archive) |
|---|---|
| State | Changes Requested, archived |
| Headers | show |
| Series | Support PMT features in Xe | expand |
On Wed, 10 Jul 2024, Michael J. Ruhl wrote: > DVSEC offsets are based on the endpoint BAR. If an endpoint is > not avialable allow the offset information to be adjusted by the available > parent driver. > > Signed-off-by: Michael J. Ruhl <michael.j.ruhl@intel.com> > --- > drivers/platform/x86/intel/pmt/class.h | 1 + > drivers/platform/x86/intel/pmt/telemetry.c | 9 +++++++++ > drivers/platform/x86/intel/vsec.c | 1 + > include/linux/intel_vsec.h | 2 ++ > 4 files changed, 13 insertions(+) > > diff --git a/drivers/platform/x86/intel/pmt/class.h b/drivers/platform/x86/intel/pmt/class.h > index a267ac964423..984cd40ee814 100644 > --- a/drivers/platform/x86/intel/pmt/class.h > +++ b/drivers/platform/x86/intel/pmt/class.h > @@ -46,6 +46,7 @@ struct intel_pmt_entry { > void __iomem *base; > struct pmt_callbacks *cb; > unsigned long base_addr; > + s32 base_adjust; > size_t size; > u32 guid; > int devid; > diff --git a/drivers/platform/x86/intel/pmt/telemetry.c b/drivers/platform/x86/intel/pmt/telemetry.c > index c9feac859e57..5c44e500e8f6 100644 > --- a/drivers/platform/x86/intel/pmt/telemetry.c > +++ b/drivers/platform/x86/intel/pmt/telemetry.c > @@ -78,6 +78,13 @@ static int pmt_telem_header_decode(struct intel_pmt_entry *entry, > header->access_type = TELEM_ACCESS(readl(disc_table)); > header->guid = readl(disc_table + TELEM_GUID_OFFSET); > header->base_offset = readl(disc_table + TELEM_BASE_OFFSET); > + if (entry->base_adjust) { > + u32 new_base = header->base_offset + entry->base_adjust; The code setting ->base_adjust is responsible for avoiding stupid settings that would lead to underflows and overflows? > + > + dev_dbg(dev, "Adjusting baseoffset from 0x%x to 0x%x\n", base offset
>-----Original Message----- >From: Ilpo Järvinen <ilpo.jarvinen@linux.intel.com> >Sent: Thursday, July 11, 2024 7:34 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; Brost, Matthew <matthew.brost@intel.com> >Subject: Re: [PATCH v6 5/6] platform/x86/intel/pmt: Add support for PMT >base adjust > >On Wed, 10 Jul 2024, Michael J. Ruhl wrote: > >> DVSEC offsets are based on the endpoint BAR. If an endpoint is >> not avialable allow the offset information to be adjusted by the > >available > >> parent driver. >> >> Signed-off-by: Michael J. Ruhl <michael.j.ruhl@intel.com> >> --- >> drivers/platform/x86/intel/pmt/class.h | 1 + >> drivers/platform/x86/intel/pmt/telemetry.c | 9 +++++++++ >> drivers/platform/x86/intel/vsec.c | 1 + >> include/linux/intel_vsec.h | 2 ++ >> 4 files changed, 13 insertions(+) >> >> diff --git a/drivers/platform/x86/intel/pmt/class.h >b/drivers/platform/x86/intel/pmt/class.h >> index a267ac964423..984cd40ee814 100644 >> --- a/drivers/platform/x86/intel/pmt/class.h >> +++ b/drivers/platform/x86/intel/pmt/class.h >> @@ -46,6 +46,7 @@ struct intel_pmt_entry { >> void __iomem *base; >> struct pmt_callbacks *cb; >> unsigned long base_addr; >> + s32 base_adjust; >> size_t size; >> u32 guid; >> int devid; >> diff --git a/drivers/platform/x86/intel/pmt/telemetry.c >b/drivers/platform/x86/intel/pmt/telemetry.c >> index c9feac859e57..5c44e500e8f6 100644 >> --- a/drivers/platform/x86/intel/pmt/telemetry.c >> +++ b/drivers/platform/x86/intel/pmt/telemetry.c >> @@ -78,6 +78,13 @@ static int pmt_telem_header_decode(struct >intel_pmt_entry *entry, >> header->access_type = TELEM_ACCESS(readl(disc_table)); >> header->guid = readl(disc_table + TELEM_GUID_OFFSET); >> header->base_offset = readl(disc_table + TELEM_BASE_OFFSET); >> + if (entry->base_adjust) { >> + u32 new_base = header->base_offset + entry->base_adjust; > >The code setting ->base_adjust is responsible for avoiding stupid settings >that would lead to underflows and overflows? I would think so. Since this driver is not aware of why the adjust is set, it is not clear how it would validate it. Is this a request for documentation of the usage, or did you have a concern that we need to verify the value? Thanks Mike >> + >> + dev_dbg(dev, "Adjusting baseoffset from 0x%x to 0x%x\n", > >base offset > >-- > i. > >> + header->base_offset, new_base); >> + header->base_offset = new_base; >> + } >> >> /* Size is measured in DWORDS, but accessor returns bytes */ >> header->size = TELEM_SIZE(readl(disc_table)); >> @@ -302,6 +309,8 @@ static int pmt_telem_probe(struct auxiliary_device >*auxdev, const struct auxilia >> for (i = 0; i < intel_vsec_dev->num_resources; i++) { >> struct intel_pmt_entry *entry = &priv->entry[priv- >>num_entries]; >> >> + entry->base_adjust = intel_vsec_dev->base_adjust; >> + >> mutex_lock(&ep_lock); >> ret = intel_pmt_dev_create(entry, &pmt_telem_ns, >intel_vsec_dev, i); >> mutex_unlock(&ep_lock); >> diff --git a/drivers/platform/x86/intel/vsec.c >b/drivers/platform/x86/intel/vsec.c >> index 7b5cc9993974..be079d62a7bc 100644 >> --- a/drivers/platform/x86/intel/vsec.c >> +++ b/drivers/platform/x86/intel/vsec.c >> @@ -212,6 +212,7 @@ static int intel_vsec_add_dev(struct pci_dev *pdev, >struct intel_vsec_header *he >> intel_vsec_dev->num_resources = header->num_entries; >> intel_vsec_dev->quirks = info->quirks; >> intel_vsec_dev->base_addr = info->base_addr; >> + intel_vsec_dev->base_adjust = info->base_adjust; >> intel_vsec_dev->priv_data = info->priv_data; >> >> if (header->id == VSEC_ID_SDSI) >> diff --git a/include/linux/intel_vsec.h b/include/linux/intel_vsec.h >> index 4569a55e8645..1fd0fcc5615d 100644 >> --- a/include/linux/intel_vsec.h >> +++ b/include/linux/intel_vsec.h >> @@ -95,6 +95,7 @@ struct intel_vsec_platform_info { >> unsigned long caps; >> unsigned long quirks; >> u64 base_addr; >> + s32 base_adjust; >> }; >> >> /** >> @@ -120,6 +121,7 @@ struct intel_vsec_device { >> size_t priv_data_size; >> unsigned long quirks; >> u64 base_addr; >> + s32 base_adjust; >> }; >> >> int intel_vsec_add_aux(struct pci_dev *pdev, struct device *parent, >>
On Thu, 11 Jul 2024, Ruhl, Michael J wrote: > >-----Original Message----- > >From: Ilpo Järvinen <ilpo.jarvinen@linux.intel.com> > >Sent: Thursday, July 11, 2024 7:34 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; Brost, Matthew <matthew.brost@intel.com> > >Subject: Re: [PATCH v6 5/6] platform/x86/intel/pmt: Add support for PMT > >base adjust > > > >On Wed, 10 Jul 2024, Michael J. Ruhl wrote: > > > >> DVSEC offsets are based on the endpoint BAR. If an endpoint is > >> not avialable allow the offset information to be adjusted by the > > > >available > > > >> parent driver. > >> > >> Signed-off-by: Michael J. Ruhl <michael.j.ruhl@intel.com> > >> --- > >> drivers/platform/x86/intel/pmt/class.h | 1 + > >> drivers/platform/x86/intel/pmt/telemetry.c | 9 +++++++++ > >> drivers/platform/x86/intel/vsec.c | 1 + > >> include/linux/intel_vsec.h | 2 ++ > >> 4 files changed, 13 insertions(+) > >> > >> diff --git a/drivers/platform/x86/intel/pmt/class.h > >b/drivers/platform/x86/intel/pmt/class.h > >> index a267ac964423..984cd40ee814 100644 > >> --- a/drivers/platform/x86/intel/pmt/class.h > >> +++ b/drivers/platform/x86/intel/pmt/class.h > >> @@ -46,6 +46,7 @@ struct intel_pmt_entry { > >> void __iomem *base; > >> struct pmt_callbacks *cb; > >> unsigned long base_addr; > >> + s32 base_adjust; > >> size_t size; > >> u32 guid; > >> int devid; > >> diff --git a/drivers/platform/x86/intel/pmt/telemetry.c > >b/drivers/platform/x86/intel/pmt/telemetry.c > >> index c9feac859e57..5c44e500e8f6 100644 > >> --- a/drivers/platform/x86/intel/pmt/telemetry.c > >> +++ b/drivers/platform/x86/intel/pmt/telemetry.c > >> @@ -78,6 +78,13 @@ static int pmt_telem_header_decode(struct > >intel_pmt_entry *entry, > >> header->access_type = TELEM_ACCESS(readl(disc_table)); > >> header->guid = readl(disc_table + TELEM_GUID_OFFSET); > >> header->base_offset = readl(disc_table + TELEM_BASE_OFFSET); > >> + if (entry->base_adjust) { > >> + u32 new_base = header->base_offset + entry->base_adjust; > > > >The code setting ->base_adjust is responsible for avoiding stupid settings > >that would lead to underflows and overflows? > > I would think so. Since this driver is not aware of why the adjust is set, > it is not clear how it would validate it. > > Is this a request for documentation of the usage, or did you have a concern > that we need to verify the value? It's just that usually in-kernel interfaces too feature sanity checks but perhaps you're right and the telemetry side just doesn't know enough to even make a sanity check on the value. Let's just leave this as is for now, it can be revisited later if there starts to be many usecases for this.
diff --git a/drivers/platform/x86/intel/pmt/class.h b/drivers/platform/x86/intel/pmt/class.h index a267ac964423..984cd40ee814 100644 --- a/drivers/platform/x86/intel/pmt/class.h +++ b/drivers/platform/x86/intel/pmt/class.h @@ -46,6 +46,7 @@ struct intel_pmt_entry { void __iomem *base; struct pmt_callbacks *cb; unsigned long base_addr; + s32 base_adjust; size_t size; u32 guid; int devid; diff --git a/drivers/platform/x86/intel/pmt/telemetry.c b/drivers/platform/x86/intel/pmt/telemetry.c index c9feac859e57..5c44e500e8f6 100644 --- a/drivers/platform/x86/intel/pmt/telemetry.c +++ b/drivers/platform/x86/intel/pmt/telemetry.c @@ -78,6 +78,13 @@ static int pmt_telem_header_decode(struct intel_pmt_entry *entry, header->access_type = TELEM_ACCESS(readl(disc_table)); header->guid = readl(disc_table + TELEM_GUID_OFFSET); header->base_offset = readl(disc_table + TELEM_BASE_OFFSET); + if (entry->base_adjust) { + u32 new_base = header->base_offset + entry->base_adjust; + + dev_dbg(dev, "Adjusting baseoffset from 0x%x to 0x%x\n", + header->base_offset, new_base); + header->base_offset = new_base; + } /* Size is measured in DWORDS, but accessor returns bytes */ header->size = TELEM_SIZE(readl(disc_table)); @@ -302,6 +309,8 @@ static int pmt_telem_probe(struct auxiliary_device *auxdev, const struct auxilia for (i = 0; i < intel_vsec_dev->num_resources; i++) { struct intel_pmt_entry *entry = &priv->entry[priv->num_entries]; + entry->base_adjust = intel_vsec_dev->base_adjust; + mutex_lock(&ep_lock); ret = intel_pmt_dev_create(entry, &pmt_telem_ns, intel_vsec_dev, i); mutex_unlock(&ep_lock); diff --git a/drivers/platform/x86/intel/vsec.c b/drivers/platform/x86/intel/vsec.c index 7b5cc9993974..be079d62a7bc 100644 --- a/drivers/platform/x86/intel/vsec.c +++ b/drivers/platform/x86/intel/vsec.c @@ -212,6 +212,7 @@ static int intel_vsec_add_dev(struct pci_dev *pdev, struct intel_vsec_header *he intel_vsec_dev->num_resources = header->num_entries; intel_vsec_dev->quirks = info->quirks; intel_vsec_dev->base_addr = info->base_addr; + intel_vsec_dev->base_adjust = info->base_adjust; intel_vsec_dev->priv_data = info->priv_data; if (header->id == VSEC_ID_SDSI) diff --git a/include/linux/intel_vsec.h b/include/linux/intel_vsec.h index 4569a55e8645..1fd0fcc5615d 100644 --- a/include/linux/intel_vsec.h +++ b/include/linux/intel_vsec.h @@ -95,6 +95,7 @@ struct intel_vsec_platform_info { unsigned long caps; unsigned long quirks; u64 base_addr; + s32 base_adjust; }; /** @@ -120,6 +121,7 @@ struct intel_vsec_device { size_t priv_data_size; unsigned long quirks; u64 base_addr; + s32 base_adjust; }; int intel_vsec_add_aux(struct pci_dev *pdev, struct device *parent,
DVSEC offsets are based on the endpoint BAR. If an endpoint is not avialable allow the offset information to be adjusted by the parent driver. Signed-off-by: Michael J. Ruhl <michael.j.ruhl@intel.com> --- drivers/platform/x86/intel/pmt/class.h | 1 + drivers/platform/x86/intel/pmt/telemetry.c | 9 +++++++++ drivers/platform/x86/intel/vsec.c | 1 + include/linux/intel_vsec.h | 2 ++ 4 files changed, 13 insertions(+)