RE: [PATCH v6 5/6] platform/x86/intel/pmt: Add support for PMT base adjust

["Ilpo Järvinen" <ilpo.jarvinen@linux.intel.com>]

[-- Attachment #1: Type: text/plain, Size: 2914 bytes --]

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.

-- 
 i.