[V2 0/6] Auxiliary bus driver support for Intel PCIe VSEC/DVSEC

[V2 0/6] Auxiliary bus driver support for Intel PCIe VSEC/DVSEC

[David E. Box]

This series makes changes to the current intel_pmt driver to give it
broader support for Intel defined PCIe VSEC and DVSEC features. It
moves the implementation from MFD to the auxiliary bus and creates a
generic framework for enumerating the extended capabilities. It also
adds support for a new VSEC, Software Defined Silicon (SDSi).

Version 2 adds two new patches, sample code and testing.

David E. Box (6):
  PCI: Add #defines for accessing PCIe DVSEC fields
  driver core: auxiliary bus: Add driver data helpers
  platform/x86/intel: Move intel_pmt from MFD to Auxiliary Bus
  platform/x86: Add Intel Software Defined Silicon driver
  sample/sdsi: Sample of SDSi provisiong using sysfs
  selftests: sdsi: test sysfs setup

 .../ABI/testing/sysfs-driver-intel_sdsi       |  77 +++
 MAINTAINERS                                   |  19 +-
 drivers/mfd/Kconfig                           |  10 -
 drivers/mfd/Makefile                          |   1 -
 drivers/mfd/intel_pmt.c                       | 261 --------
 drivers/platform/x86/intel/Kconfig            |  23 +
 drivers/platform/x86/intel/Makefile           |   4 +
 drivers/platform/x86/intel/pmt/Kconfig        |   4 +-
 drivers/platform/x86/intel/pmt/class.c        |  21 +-
 drivers/platform/x86/intel/pmt/class.h        |   5 +-
 drivers/platform/x86/intel/pmt/crashlog.c     |  47 +-
 drivers/platform/x86/intel/pmt/telemetry.c    |  46 +-
 drivers/platform/x86/intel/sdsi.c             | 571 ++++++++++++++++++
 drivers/platform/x86/intel/vsec.c             | 418 +++++++++++++
 drivers/platform/x86/intel/vsec.h             |  43 ++
 include/linux/auxiliary_bus.h                 |  10 +
 include/uapi/linux/pci_regs.h                 |   4 +
 samples/sdsi/Makefile                         |   9 +
 samples/sdsi/sdsi-sample.c                    | 399 ++++++++++++
 tools/testing/selftests/drivers/sdsi/sdsi.sh  |  18 +
 .../selftests/drivers/sdsi/sdsi_test.py       | 166 +++++
 21 files changed, 1821 insertions(+), 335 deletions(-)
 create mode 100644 Documentation/ABI/testing/sysfs-driver-intel_sdsi
 delete mode 100644 drivers/mfd/intel_pmt.c
 create mode 100644 drivers/platform/x86/intel/sdsi.c
 create mode 100644 drivers/platform/x86/intel/vsec.c
 create mode 100644 drivers/platform/x86/intel/vsec.h
 create mode 100644 samples/sdsi/Makefile
 create mode 100644 samples/sdsi/sdsi-sample.c
 create mode 100755 tools/testing/selftests/drivers/sdsi/sdsi.sh
 create mode 100644 tools/testing/selftests/drivers/sdsi/sdsi_test.py

-- 
2.25.1

[V2 1/6] PCI: Add #defines for accessing PCIe DVSEC fields

[David E. Box]

Add #defines for accessing Vendor ID, Revision, Length, and ID offsets
in the Designated Vendor Specific Extended Capability (DVSEC). Defined
in PCIe r5.0, sec 7.9.6.

Signed-off-by: David E. Box <david.e.box@linux.intel.com>
Acked-by: Bjorn Helgaas <bhelgaas@google.com>
Reviewed-by: Rafael J. Wysocki <rafael.j.wysocki@intel.com>
---
V2
  - No changes

 include/uapi/linux/pci_regs.h | 4 ++++
 1 file changed, 4 insertions(+)

diff --git a/include/uapi/linux/pci_regs.h b/include/uapi/linux/pci_regs.h
index ff6ccbc6efe9..318f3f1f9e92 100644
--- a/include/uapi/linux/pci_regs.h
+++ b/include/uapi/linux/pci_regs.h
@@ -1086,7 +1086,11 @@
 
 /* Designated Vendor-Specific (DVSEC, PCI_EXT_CAP_ID_DVSEC) */
 #define PCI_DVSEC_HEADER1		0x4 /* Designated Vendor-Specific Header1 */
+#define  PCI_DVSEC_HEADER1_VID(x)	((x) & 0xffff)
+#define  PCI_DVSEC_HEADER1_REV(x)	(((x) >> 16) & 0xf)
+#define  PCI_DVSEC_HEADER1_LEN(x)	(((x) >> 20) & 0xfff)
 #define PCI_DVSEC_HEADER2		0x8 /* Designated Vendor-Specific Header2 */
+#define  PCI_DVSEC_HEADER2_ID(x)		((x) & 0xffff)
 
 /* Data Link Feature */
 #define PCI_DLF_CAP		0x04	/* Capabilities Register */
-- 
2.25.1

[V2 2/6] driver core: auxiliary bus: Add driver data helpers

[David E. Box]

Adds get/set driver data helpers for auxiliary devices.

Signed-off-by: David E. Box <david.e.box@linux.intel.com>
Reviewed-by: Mark Gross <markgross@kernel.org>
---
V2
  - No changes

 include/linux/auxiliary_bus.h | 10 ++++++++++
 1 file changed, 10 insertions(+)

diff --git a/include/linux/auxiliary_bus.h b/include/linux/auxiliary_bus.h
index fc51d45f106b..a8338d456e81 100644
--- a/include/linux/auxiliary_bus.h
+++ b/include/linux/auxiliary_bus.h
@@ -28,6 +28,16 @@ struct auxiliary_driver {
 	const struct auxiliary_device_id *id_table;
 };
 
+static inline void *auxiliary_get_drvdata(struct auxiliary_device *auxdev)
+{
+	return dev_get_drvdata(&auxdev->dev);
+}
+
+static inline void auxiliary_set_drvdata(struct auxiliary_device *auxdev, void *data)
+{
+	dev_set_drvdata(&auxdev->dev, data);
+}
+
 static inline struct auxiliary_device *to_auxiliary_dev(struct device *dev)
 {
 	return container_of(dev, struct auxiliary_device, dev);
-- 
2.25.1

Re: [V2 2/6] driver core: auxiliary bus: Add driver data helpers

[Andy Shevchenko]

On Tue, Dec 07, 2021 at 09:14:44AM -0800, David E. Box wrote:
> Adds get/set driver data helpers for auxiliary devices.

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

> Signed-off-by: David E. Box <david.e.box@linux.intel.com>
> Reviewed-by: Mark Gross <markgross@kernel.org>
> ---
> V2
>   - No changes
> 
>  include/linux/auxiliary_bus.h | 10 ++++++++++
>  1 file changed, 10 insertions(+)
> 
> diff --git a/include/linux/auxiliary_bus.h b/include/linux/auxiliary_bus.h
> index fc51d45f106b..a8338d456e81 100644
> --- a/include/linux/auxiliary_bus.h
> +++ b/include/linux/auxiliary_bus.h
> @@ -28,6 +28,16 @@ struct auxiliary_driver {
>  	const struct auxiliary_device_id *id_table;
>  };
>  
> +static inline void *auxiliary_get_drvdata(struct auxiliary_device *auxdev)
> +{
> +	return dev_get_drvdata(&auxdev->dev);
> +}
> +
> +static inline void auxiliary_set_drvdata(struct auxiliary_device *auxdev, void *data)
> +{
> +	dev_set_drvdata(&auxdev->dev, data);
> +}
> +
>  static inline struct auxiliary_device *to_auxiliary_dev(struct device *dev)
>  {
>  	return container_of(dev, struct auxiliary_device, dev);
> -- 
> 2.25.1
> 

-- 
With Best Regards,
Andy Shevchenko

Re: [V2 2/6] driver core: auxiliary bus: Add driver data helpers

[Leon Romanovsky]

On Tue, Dec 07, 2021 at 09:14:44AM -0800, David E. Box wrote:
> Adds get/set driver data helpers for auxiliary devices.
> 
> Signed-off-by: David E. Box <david.e.box@linux.intel.com>
> Reviewed-by: Mark Gross <markgross@kernel.org>
> ---
> V2
>   - No changes
> 
>  include/linux/auxiliary_bus.h | 10 ++++++++++
>  1 file changed, 10 insertions(+)

I would really like to see an explanation why such obfuscation is really
needed. dev_*_drvdata() is a standard way to access driver data.

Thanks

> 
> diff --git a/include/linux/auxiliary_bus.h b/include/linux/auxiliary_bus.h
> index fc51d45f106b..a8338d456e81 100644
> --- a/include/linux/auxiliary_bus.h
> +++ b/include/linux/auxiliary_bus.h
> @@ -28,6 +28,16 @@ struct auxiliary_driver {
>  	const struct auxiliary_device_id *id_table;
>  };
>  
> +static inline void *auxiliary_get_drvdata(struct auxiliary_device *auxdev)
> +{
> +	return dev_get_drvdata(&auxdev->dev);
> +}
> +
> +static inline void auxiliary_set_drvdata(struct auxiliary_device *auxdev, void *data)
> +{
> +	dev_set_drvdata(&auxdev->dev, data);
> +}
> +
>  static inline struct auxiliary_device *to_auxiliary_dev(struct device *dev)
>  {
>  	return container_of(dev, struct auxiliary_device, dev);
> -- 
> 2.25.1
> 

Re: [V2 2/6] driver core: auxiliary bus: Add driver data helpers

[Greg KH]

On Wed, Dec 08, 2021 at 09:03:16AM +0200, Leon Romanovsky wrote:
> On Tue, Dec 07, 2021 at 09:14:44AM -0800, David E. Box wrote:
> > Adds get/set driver data helpers for auxiliary devices.
> > 
> > Signed-off-by: David E. Box <david.e.box@linux.intel.com>
> > Reviewed-by: Mark Gross <markgross@kernel.org>
> > ---
> > V2
> >   - No changes
> > 
> >  include/linux/auxiliary_bus.h | 10 ++++++++++
> >  1 file changed, 10 insertions(+)
> 
> I would really like to see an explanation why such obfuscation is really
> needed. dev_*_drvdata() is a standard way to access driver data.

Lots of busses have this helper.  This is nothing new at all, and is
nice to have.  Look at all of the calls to dev_get_drvdata() in
include/linux/ for the examples.

thanks,

greg k-h

Re: [V2 2/6] driver core: auxiliary bus: Add driver data helpers

[Leon Romanovsky]

On Wed, Dec 08, 2021 at 08:07:39AM +0100, Greg KH wrote:
> On Wed, Dec 08, 2021 at 09:03:16AM +0200, Leon Romanovsky wrote:
> > On Tue, Dec 07, 2021 at 09:14:44AM -0800, David E. Box wrote:
> > > Adds get/set driver data helpers for auxiliary devices.
> > > 
> > > Signed-off-by: David E. Box <david.e.box@linux.intel.com>
> > > Reviewed-by: Mark Gross <markgross@kernel.org>
> > > ---
> > > V2
> > >   - No changes
> > > 
> > >  include/linux/auxiliary_bus.h | 10 ++++++++++
> > >  1 file changed, 10 insertions(+)
> > 
> > I would really like to see an explanation why such obfuscation is really
> > needed. dev_*_drvdata() is a standard way to access driver data.
> 
> Lots of busses have this helper.  This is nothing new at all, and is
> nice to have.  Look at all of the calls to dev_get_drvdata() in
> include/linux/ for the examples.

I looked and this is why I asked. From the point of person who does
reviews and refactoring, such obfuscations are evil.

If I understand your correctly, the explanation is just copy/paste, right?

Thanks

> 
> thanks,
> 
> greg k-h

Re: [V2 2/6] driver core: auxiliary bus: Add driver data helpers

[Greg KH]

On Wed, Dec 08, 2021 at 10:32:13AM +0200, Leon Romanovsky wrote:
> On Wed, Dec 08, 2021 at 08:07:39AM +0100, Greg KH wrote:
> > On Wed, Dec 08, 2021 at 09:03:16AM +0200, Leon Romanovsky wrote:
> > > On Tue, Dec 07, 2021 at 09:14:44AM -0800, David E. Box wrote:
> > > > Adds get/set driver data helpers for auxiliary devices.
> > > > 
> > > > Signed-off-by: David E. Box <david.e.box@linux.intel.com>
> > > > Reviewed-by: Mark Gross <markgross@kernel.org>
> > > > ---
> > > > V2
> > > >   - No changes
> > > > 
> > > >  include/linux/auxiliary_bus.h | 10 ++++++++++
> > > >  1 file changed, 10 insertions(+)
> > > 
> > > I would really like to see an explanation why such obfuscation is really
> > > needed. dev_*_drvdata() is a standard way to access driver data.
> > 
> > Lots of busses have this helper.  This is nothing new at all, and is
> > nice to have.  Look at all of the calls to dev_get_drvdata() in
> > include/linux/ for the examples.
> 
> I looked and this is why I asked. From the point of person who does
> reviews and refactoring, such obfuscations are evil.

Then you must consider about 80 busses evil :)

Again, this is a very common helper pattern in the kernel, one that we
have had for decades now.  It allows a driver writer to only worry about
the bus apis for that specific bus, and not have to dive down into the
driver core functions at all.  What is wrong with that?

> If I understand your correctly, the explanation is just copy/paste, right?

I do not understand what you mean by this, sorry.

greg k-h

Re: [V2 2/6] driver core: auxiliary bus: Add driver data helpers

[Leon Romanovsky]

On Wed, Dec 08, 2021 at 09:43:37AM +0100, Greg KH wrote:
> On Wed, Dec 08, 2021 at 10:32:13AM +0200, Leon Romanovsky wrote:
> > On Wed, Dec 08, 2021 at 08:07:39AM +0100, Greg KH wrote:
> > > On Wed, Dec 08, 2021 at 09:03:16AM +0200, Leon Romanovsky wrote:
> > > > On Tue, Dec 07, 2021 at 09:14:44AM -0800, David E. Box wrote:
> > > > > Adds get/set driver data helpers for auxiliary devices.
> > > > > 
> > > > > Signed-off-by: David E. Box <david.e.box@linux.intel.com>
> > > > > Reviewed-by: Mark Gross <markgross@kernel.org>
> > > > > ---
> > > > > V2
> > > > >   - No changes
> > > > > 
> > > > >  include/linux/auxiliary_bus.h | 10 ++++++++++
> > > > >  1 file changed, 10 insertions(+)
> > > > 
> > > > I would really like to see an explanation why such obfuscation is really
> > > > needed. dev_*_drvdata() is a standard way to access driver data.
> > > 
> > > Lots of busses have this helper.  This is nothing new at all, and is
> > > nice to have.  Look at all of the calls to dev_get_drvdata() in
> > > include/linux/ for the examples.
> > 
> > I looked and this is why I asked. From the point of person who does
> > reviews and refactoring, such obfuscations are evil.
> 
> Then you must consider about 80 busses evil :)
> 
> Again, this is a very common helper pattern in the kernel, one that we
> have had for decades now.  It allows a driver writer to only worry about
> the bus apis for that specific bus, and not have to dive down into the
> driver core functions at all.  What is wrong with that?

What is wrong?

The idea that you have two APIs which do the same thing, one is
obfuscated version of another.

If you don't want from people to use driver core function and structures,
you shouldn't expose them in global headers.

Thanks

Re: [V2 2/6] driver core: auxiliary bus: Add driver data helpers

[Andy Shevchenko]

On Wed, Dec 08, 2021 at 11:12:20AM +0200, Leon Romanovsky wrote:
> On Wed, Dec 08, 2021 at 09:43:37AM +0100, Greg KH wrote:

...

> The idea that you have two APIs which do the same thing, one is
> obfuscated version of another.
> 
> If you don't want from people to use driver core function and structures,
> you shouldn't expose them in global headers.

For all these APIs the rationale is very simple. If you have callback that
takes a pointer to the container (*), you better use the APIs related to
this container (no need to have an explicit dereferencing). Otherwise you
use dev_*() APIs (when it's pointer to the pure struct device).

The value is to have coherent APIs around struct device containers.

*) under container here I assume the data structure that has the embedded
   struct device in it.

-- 
With Best Regards,
Andy Shevchenko

Re: [V2 2/6] driver core: auxiliary bus: Add driver data helpers

[Leon Romanovsky]

On Wed, Dec 08, 2021 at 12:14:41PM +0200, Andy Shevchenko wrote:
> On Wed, Dec 08, 2021 at 11:12:20AM +0200, Leon Romanovsky wrote:
> > On Wed, Dec 08, 2021 at 09:43:37AM +0100, Greg KH wrote:
> 
> ...
> 
> > The idea that you have two APIs which do the same thing, one is
> > obfuscated version of another.
> > 
> > If you don't want from people to use driver core function and structures,
> > you shouldn't expose them in global headers.
> 
> For all these APIs the rationale is very simple. If you have callback that
> takes a pointer to the container (*), you better use the APIs related to
> this container (no need to have an explicit dereferencing). Otherwise you
> use dev_*() APIs (when it's pointer to the pure struct device).
> 
> The value is to have coherent APIs around struct device containers.
> 
> *) under container here I assume the data structure that has the embedded
>    struct device in it.

Thanks

> 
 -- 
> With Best Regards,
> Andy Shevchenko
> 
> 

Re: [V2 2/6] driver core: auxiliary bus: Add driver data helpers

[Lee Jones]

On Wed, 08 Dec 2021, Greg KH wrote:

> On Wed, Dec 08, 2021 at 09:03:16AM +0200, Leon Romanovsky wrote:
> > On Tue, Dec 07, 2021 at 09:14:44AM -0800, David E. Box wrote:
> > > Adds get/set driver data helpers for auxiliary devices.
> > > 
> > > Signed-off-by: David E. Box <david.e.box@linux.intel.com>
> > > Reviewed-by: Mark Gross <markgross@kernel.org>
> > > ---
> > > V2
> > >   - No changes
> > > 
> > >  include/linux/auxiliary_bus.h | 10 ++++++++++
> > >  1 file changed, 10 insertions(+)
> > 
> > I would really like to see an explanation why such obfuscation is really
> > needed. dev_*_drvdata() is a standard way to access driver data.

I wouldn't call it obfuscation, but it does looks like abstraction for
the sake of abstraction, which I usually push back on.  What are the
technical benefits over using the dev_*() variant?

> Lots of busses have this helper.  This is nothing new at all, and is
> nice to have.  Look at all of the calls to dev_get_drvdata() in
> include/linux/ for the examples.

-- 
Lee Jones [李琼斯]
Senior Technical Lead - Developer Services
Linaro.org │ Open source software for Arm SoCs
Follow Linaro: Facebook | Twitter | Blog

Re: [V2 2/6] driver core: auxiliary bus: Add driver data helpers

[Greg KH]

On Wed, Dec 08, 2021 at 08:43:53AM +0000, Lee Jones wrote:
> On Wed, 08 Dec 2021, Greg KH wrote:
> 
> > On Wed, Dec 08, 2021 at 09:03:16AM +0200, Leon Romanovsky wrote:
> > > On Tue, Dec 07, 2021 at 09:14:44AM -0800, David E. Box wrote:
> > > > Adds get/set driver data helpers for auxiliary devices.
> > > > 
> > > > Signed-off-by: David E. Box <david.e.box@linux.intel.com>
> > > > Reviewed-by: Mark Gross <markgross@kernel.org>
> > > > ---
> > > > V2
> > > >   - No changes
> > > > 
> > > >  include/linux/auxiliary_bus.h | 10 ++++++++++
> > > >  1 file changed, 10 insertions(+)
> > > 
> > > I would really like to see an explanation why such obfuscation is really
> > > needed. dev_*_drvdata() is a standard way to access driver data.
> 
> I wouldn't call it obfuscation, but it does looks like abstraction for
> the sake of abstraction, which I usually push back on.  What are the
> technical benefits over using the dev_*() variant?

See my response at:
	https://lore.kernel.org/r/YbBwOb6JvWkT3JWI@kroah.com
for why it is a good thing to do.

In short, driver authors should not have to worry about mixing
bus-specific and low-level driver core functions.

thanks,

greg k-h

Re: [V2 2/6] driver core: auxiliary bus: Add driver data helpers

[Lee Jones]

On Wed, 08 Dec 2021, Greg KH wrote:

> On Wed, Dec 08, 2021 at 08:43:53AM +0000, Lee Jones wrote:
> > On Wed, 08 Dec 2021, Greg KH wrote:
> > 
> > > On Wed, Dec 08, 2021 at 09:03:16AM +0200, Leon Romanovsky wrote:
> > > > On Tue, Dec 07, 2021 at 09:14:44AM -0800, David E. Box wrote:
> > > > > Adds get/set driver data helpers for auxiliary devices.
> > > > > 
> > > > > Signed-off-by: David E. Box <david.e.box@linux.intel.com>
> > > > > Reviewed-by: Mark Gross <markgross@kernel.org>
> > > > > ---
> > > > > V2
> > > > >   - No changes
> > > > > 
> > > > >  include/linux/auxiliary_bus.h | 10 ++++++++++
> > > > >  1 file changed, 10 insertions(+)
> > > > 
> > > > I would really like to see an explanation why such obfuscation is really
> > > > needed. dev_*_drvdata() is a standard way to access driver data.
> > 
> > I wouldn't call it obfuscation, but it does looks like abstraction for
> > the sake of abstraction, which I usually push back on.  What are the
> > technical benefits over using the dev_*() variant?
> 
> See my response at:
> 	https://lore.kernel.org/r/YbBwOb6JvWkT3JWI@kroah.com
> for why it is a good thing to do.

I saw this after I'd sent my query.

> In short, driver authors should not have to worry about mixing
> bus-specific and low-level driver core functions.

Okay, that makes sense.

I guess my view abstraction for the sake of it is slightly higher
level as I vehemently dislike it when driver-set writers create their
own APIs, such as; (just off the top of my head, not a real example)
cros_get_data() or cros_write() which are usually abstractions of top
level APIs like platform_get_data() and regmap_write() respectively.

Abstracting at *real* API level does seem like the right thing to do.

-- 
Lee Jones [李琼斯]
Senior Technical Lead - Developer Services
Linaro.org │ Open source software for Arm SoCs
Follow Linaro: Facebook | Twitter | Blog

Re: [V2 2/6] driver core: auxiliary bus: Add driver data helpers

[Andy Shevchenko]

On Wed, Dec 08, 2021 at 09:47:56AM +0100, Greg KH wrote:
> On Wed, Dec 08, 2021 at 08:43:53AM +0000, Lee Jones wrote:
> > On Wed, 08 Dec 2021, Greg KH wrote:
> > 
> > > On Wed, Dec 08, 2021 at 09:03:16AM +0200, Leon Romanovsky wrote:
> > > > On Tue, Dec 07, 2021 at 09:14:44AM -0800, David E. Box wrote:
> > > > > Adds get/set driver data helpers for auxiliary devices.
> > > > > 
> > > > > Signed-off-by: David E. Box <david.e.box@linux.intel.com>
> > > > > Reviewed-by: Mark Gross <markgross@kernel.org>
> > > > > ---
> > > > > V2
> > > > >   - No changes
> > > > > 
> > > > >  include/linux/auxiliary_bus.h | 10 ++++++++++
> > > > >  1 file changed, 10 insertions(+)
> > > > 
> > > > I would really like to see an explanation why such obfuscation is really
> > > > needed. dev_*_drvdata() is a standard way to access driver data.
> > 
> > I wouldn't call it obfuscation, but it does looks like abstraction for
> > the sake of abstraction, which I usually push back on.  What are the
> > technical benefits over using the dev_*() variant?
> 
> See my response at:
> 	https://lore.kernel.org/r/YbBwOb6JvWkT3JWI@kroah.com
> for why it is a good thing to do.
> 
> In short, driver authors should not have to worry about mixing
> bus-specific and low-level driver core functions.

Right! I just answered to Leon with the similar view behind (using
different words).

-- 
With Best Regards,
Andy Shevchenko

Re: [V2 2/6] driver core: auxiliary bus: Add driver data helpers

[Bjorn Helgaas]

[+cc Rafael, since I used generic PM as an example]

On Wed, Dec 08, 2021 at 09:47:56AM +0100, Greg KH wrote:
> On Wed, Dec 08, 2021 at 08:43:53AM +0000, Lee Jones wrote:
> > On Wed, 08 Dec 2021, Greg KH wrote:
> > > On Wed, Dec 08, 2021 at 09:03:16AM +0200, Leon Romanovsky wrote:
> > > > On Tue, Dec 07, 2021 at 09:14:44AM -0800, David E. Box wrote:
> > > > > Adds get/set driver data helpers for auxiliary devices.
> > > > > 
> > > > > Signed-off-by: David E. Box <david.e.box@linux.intel.com>
> > > > > Reviewed-by: Mark Gross <markgross@kernel.org>
> > > > > ---
> > > > > V2
> > > > >   - No changes
> > > > > 
> > > > >  include/linux/auxiliary_bus.h | 10 ++++++++++
> > > > >  1 file changed, 10 insertions(+)
> > > > 
> > > > I would really like to see an explanation why such obfuscation is really
> > > > needed. dev_*_drvdata() is a standard way to access driver data.
> > 
> > I wouldn't call it obfuscation, but it does looks like abstraction for
> > the sake of abstraction, which I usually push back on.  What are the
> > technical benefits over using the dev_*() variant?
> 
> See my response at:
> 	https://lore.kernel.org/r/YbBwOb6JvWkT3JWI@kroah.com
> for why it is a good thing to do.
> 
> In short, driver authors should not have to worry about mixing
> bus-specific and low-level driver core functions.

In the very common situation of PCI drivers that use generic power
management, authors *do* have to use both (example from [1]):

  ioh_gpio_probe(struct pci_dev *pdev)   # pci_driver.probe()
    pci_set_drvdata(pdev, chip);

  ioh_gpio_remove(struct pci_dev *pdev)  # pci_driver.remove()
    struct ioh_gpio *chip = pci_get_drvdata(pdev);

  ioh_gpio_suspend(struct device *dev)   # pci_driver.driver.pm.suspend()
    struct ioh_gpio *chip = dev_get_drvdata(dev);   <--

The pci_driver methods receive a struct pci_dev and use the
pci_get_drvdata() wrapper.

The generic power management methods receive a struct device and use
the underlying dev_get_drvdata().

It's kind of ugly that readers have to know that pci_get_drvdata()
gives you the same thing as dev_get_drvdata().

I guess the generic PM methods could do something like:

  pci_get_drvdata(to_pci_dev(dev));

but that seems a little bit circuitous.  It's slightly wordier, but I
might prefer to just use this everywhere and skip the pci_* wrappers:

  dev_get_drvdata(&pdev->dev);

Bjorn

[1] https://git.kernel.org/pub/scm/linux/kernel/git/torvalds/linux.git/tree/drivers/gpio/gpio-ml-ioh.c?id=v5.15#n505

Re: [V2 2/6] driver core: auxiliary bus: Add driver data helpers

[Andy Shevchenko]

On Thu, Dec 09, 2021 at 10:32:45AM -0600, Bjorn Helgaas wrote:
> [+cc Rafael, since I used generic PM as an example]
> On Wed, Dec 08, 2021 at 09:47:56AM +0100, Greg KH wrote:

...

Okay, more bikeshedding :-)

> In the very common situation of PCI drivers that use generic power
> management, authors *do* have to use both (example from [1]):
> 
>   ioh_gpio_probe(struct pci_dev *pdev)   # pci_driver.probe()
>     pci_set_drvdata(pdev, chip);
> 
>   ioh_gpio_remove(struct pci_dev *pdev)  # pci_driver.remove()
>     struct ioh_gpio *chip = pci_get_drvdata(pdev);
> 
>   ioh_gpio_suspend(struct device *dev)   # pci_driver.driver.pm.suspend()
>     struct ioh_gpio *chip = dev_get_drvdata(dev);   <--
> 
> The pci_driver methods receive a struct pci_dev and use the
> pci_get_drvdata() wrapper.
> 
> The generic power management methods receive a struct device and use
> the underlying dev_get_drvdata().
> 
> It's kind of ugly that readers have to know that pci_get_drvdata()
> gives you the same thing as dev_get_drvdata().
> 
> I guess the generic PM methods could do something like:
> 
>   pci_get_drvdata(to_pci_dev(dev));
> 
> but that seems a little bit circuitous.  It's slightly wordier, but I
> might prefer to just use this everywhere and skip the pci_* wrappers:
> 
>   dev_get_drvdata(&pdev->dev);

Strictly speaking the

   <$BUS)_get_drvdata(<$CONTAINER>) != dev_get_drvdata(dev)

it's completely up to the container handling code what to do.
In 99% (or 100%?) cases it's equal, but it's not obliged to be so.

> [1] https://git.kernel.org/pub/scm/linux/kernel/git/torvalds/linux.git/tree/drivers/gpio/gpio-ml-ioh.c?id=v5.15#n505

-- 
With Best Regards,
Andy Shevchenko

Re: [V2 2/6] driver core: auxiliary bus: Add driver data helpers

[Leon Romanovsky]

On Wed, Dec 08, 2021 at 08:43:53AM +0000, Lee Jones wrote:
> On Wed, 08 Dec 2021, Greg KH wrote:
> 
> > On Wed, Dec 08, 2021 at 09:03:16AM +0200, Leon Romanovsky wrote:
> > > On Tue, Dec 07, 2021 at 09:14:44AM -0800, David E. Box wrote:
> > > > Adds get/set driver data helpers for auxiliary devices.
> > > > 
> > > > Signed-off-by: David E. Box <david.e.box@linux.intel.com>
> > > > Reviewed-by: Mark Gross <markgross@kernel.org>
> > > > ---
> > > > V2
> > > >   - No changes
> > > > 
> > > >  include/linux/auxiliary_bus.h | 10 ++++++++++
> > > >  1 file changed, 10 insertions(+)
> > > 
> > > I would really like to see an explanation why such obfuscation is really
> > > needed. dev_*_drvdata() is a standard way to access driver data.
> 
> I wouldn't call it obfuscation, but it does looks like abstraction for
> the sake of abstraction, which I usually push back on.  What are the
> technical benefits over using the dev_*() variant?

You can see it in Greg's answer, there is no technical benefits in any
variant. It is simple copy/paste pattern from other buses.

Maybe it is not clear from my response, I don't care if this patch is
going to be applied or not, but I would like to hear someone explains
to me what are the benefits of such one liners.

Thanks

[V2 3/6] platform/x86/intel: Move intel_pmt from MFD to Auxiliary Bus

[David E. Box]

Intel Platform Monitoring Technology (PMT) support is indicated by presence
of an Intel defined PCIe Designated Vendor Specific Extended Capabilities
(DVSEC) structure with a PMT specific ID. The current MFD implementation
creates child devices for each PMT feature, currently telemetry, watcher,
and crashlog. However DVSEC structures may also be used by Intel to
indicate support for other features. The Out Of Band Management Services
Module (OOBMSM) uses DVSEC to enumerate several features, including PMT.
In order to support them it is necessary to modify the intel_pmt driver to
handle the creation of the child devices more generically. To that end,
modify the driver to create child devices for any VSEC/DVSEC features on
supported devices (indicated by PCI ID).  Additionally, move the
implementation from MFD to the Auxiliary bus.  VSEC/DVSEC features are
really multifunctional PCI devices, not platform devices as MFD was
designed for. Auxiliary bus gives more flexibility by allowing the
definition of custom structures that can be shared between associated
auxiliary devices and the parent device. Also, rename the driver from
intel_pmt to intel_vsec to better reflect the purpose.

This series also removes the current runtime pm support which was not
complete to begin with. None of the current devices require runtime pm.
However the support will be replaced when a device is added that requires
it.

Signed-off-by: David E. Box <david.e.box@linux.intel.com>
Reviewed-by: Mark Gross <markgross@kernel.org>
---
V2
  - Clarify status of missing pm support in commit message
  - Clarify why auxiliary bus is preferred in commit message

 MAINTAINERS                                |  12 +-
 drivers/mfd/Kconfig                        |  10 -
 drivers/mfd/Makefile                       |   1 -
 drivers/mfd/intel_pmt.c                    | 261 -------------
 drivers/platform/x86/intel/Kconfig         |  11 +
 drivers/platform/x86/intel/Makefile        |   2 +
 drivers/platform/x86/intel/pmt/Kconfig     |   4 +-
 drivers/platform/x86/intel/pmt/class.c     |  21 +-
 drivers/platform/x86/intel/pmt/class.h     |   5 +-
 drivers/platform/x86/intel/pmt/crashlog.c  |  47 +--
 drivers/platform/x86/intel/pmt/telemetry.c |  46 +--
 drivers/platform/x86/intel/vsec.c          | 408 +++++++++++++++++++++
 drivers/platform/x86/intel/vsec.h          |  43 +++
 13 files changed, 536 insertions(+), 335 deletions(-)
 delete mode 100644 drivers/mfd/intel_pmt.c
 create mode 100644 drivers/platform/x86/intel/vsec.c
 create mode 100644 drivers/platform/x86/intel/vsec.h

diff --git a/MAINTAINERS b/MAINTAINERS
index 43007f2d29e0..cd2b10a86f09 100644
--- a/MAINTAINERS
+++ b/MAINTAINERS
@@ -9752,10 +9752,9 @@ S:	Maintained
 F:	drivers/mfd/intel_soc_pmic*
 F:	include/linux/mfd/intel_soc_pmic*
 
-INTEL PMT DRIVER
-M:	"David E. Box" <david.e.box@linux.intel.com>
-S:	Maintained
-F:	drivers/mfd/intel_pmt.c
+INTEL PMT DRIVERS
+M:	David E. Box <david.e.box@linux.intel.com>
+S:	Supported
 F:	drivers/platform/x86/intel/pmt/
 
 INTEL PRO/WIRELESS 2100, 2200BG, 2915ABG NETWORK CONNECTION SUPPORT
@@ -9822,6 +9821,11 @@ L:	platform-driver-x86@vger.kernel.org
 S:	Maintained
 F:	drivers/platform/x86/intel/uncore-frequency.c
 
+INTEL VENDOR SPECIFIC EXTENDED CAPABILITIES DRIVER
+M:	David E. Box <david.e.box@linux.intel.com>
+S:	Supported
+F:	drivers/platform/x86/intel/vsec.*
+
 INTEL VIRTUAL BUTTON DRIVER
 M:	AceLan Kao <acelan.kao@canonical.com>
 L:	platform-driver-x86@vger.kernel.org
diff --git a/drivers/mfd/Kconfig b/drivers/mfd/Kconfig
index 3fb480818599..ac7b23eb62c2 100644
--- a/drivers/mfd/Kconfig
+++ b/drivers/mfd/Kconfig
@@ -692,16 +692,6 @@ config MFD_INTEL_PMC_BXT
 	  Register and P-unit access. In addition this creates devices
 	  for iTCO watchdog and telemetry that are part of the PMC.
 
-config MFD_INTEL_PMT
-	tristate "Intel Platform Monitoring Technology (PMT) support"
-	depends on X86 && PCI
-	select MFD_CORE
-	help
-	  The Intel Platform Monitoring Technology (PMT) is an interface that
-	  provides access to hardware monitor registers. This driver supports
-	  Telemetry, Watcher, and Crashlog PMT capabilities/devices for
-	  platforms starting from Tiger Lake.
-
 config MFD_IPAQ_MICRO
 	bool "Atmel Micro ASIC (iPAQ h3100/h3600/h3700) Support"
 	depends on SA1100_H3100 || SA1100_H3600
diff --git a/drivers/mfd/Makefile b/drivers/mfd/Makefile
index 0b1b629aef3e..31734d9318e2 100644
--- a/drivers/mfd/Makefile
+++ b/drivers/mfd/Makefile
@@ -211,7 +211,6 @@ obj-$(CONFIG_MFD_INTEL_LPSS)	+= intel-lpss.o
 obj-$(CONFIG_MFD_INTEL_LPSS_PCI)	+= intel-lpss-pci.o
 obj-$(CONFIG_MFD_INTEL_LPSS_ACPI)	+= intel-lpss-acpi.o
 obj-$(CONFIG_MFD_INTEL_PMC_BXT)	+= intel_pmc_bxt.o
-obj-$(CONFIG_MFD_INTEL_PMT)	+= intel_pmt.o
 obj-$(CONFIG_MFD_PALMAS)	+= palmas.o
 obj-$(CONFIG_MFD_VIPERBOARD)    += viperboard.o
 obj-$(CONFIG_MFD_NTXEC)		+= ntxec.o
diff --git a/drivers/mfd/intel_pmt.c b/drivers/mfd/intel_pmt.c
deleted file mode 100644
index dd7eb614c28e..000000000000
--- a/drivers/mfd/intel_pmt.c
+++ /dev/null
@@ -1,261 +0,0 @@
-// SPDX-License-Identifier: GPL-2.0
-/*
- * Intel Platform Monitoring Technology PMT driver
- *
- * Copyright (c) 2020, Intel Corporation.
- * All Rights Reserved.
- *
- * Author: David E. Box <david.e.box@linux.intel.com>
- */
-
-#include <linux/bits.h>
-#include <linux/kernel.h>
-#include <linux/mfd/core.h>
-#include <linux/module.h>
-#include <linux/pci.h>
-#include <linux/platform_device.h>
-#include <linux/pm.h>
-#include <linux/pm_runtime.h>
-#include <linux/types.h>
-
-/* Intel DVSEC capability vendor space offsets */
-#define INTEL_DVSEC_ENTRIES		0xA
-#define INTEL_DVSEC_SIZE		0xB
-#define INTEL_DVSEC_TABLE		0xC
-#define INTEL_DVSEC_TABLE_BAR(x)	((x) & GENMASK(2, 0))
-#define INTEL_DVSEC_TABLE_OFFSET(x)	((x) & GENMASK(31, 3))
-#define INTEL_DVSEC_ENTRY_SIZE		4
-
-/* PMT capabilities */
-#define DVSEC_INTEL_ID_TELEMETRY	2
-#define DVSEC_INTEL_ID_WATCHER		3
-#define DVSEC_INTEL_ID_CRASHLOG		4
-
-struct intel_dvsec_header {
-	u16	length;
-	u16	id;
-	u8	num_entries;
-	u8	entry_size;
-	u8	tbir;
-	u32	offset;
-};
-
-enum pmt_quirks {
-	/* Watcher capability not supported */
-	PMT_QUIRK_NO_WATCHER	= BIT(0),
-
-	/* Crashlog capability not supported */
-	PMT_QUIRK_NO_CRASHLOG	= BIT(1),
-
-	/* Use shift instead of mask to read discovery table offset */
-	PMT_QUIRK_TABLE_SHIFT	= BIT(2),
-
-	/* DVSEC not present (provided in driver data) */
-	PMT_QUIRK_NO_DVSEC	= BIT(3),
-};
-
-struct pmt_platform_info {
-	unsigned long quirks;
-	struct intel_dvsec_header **capabilities;
-};
-
-static const struct pmt_platform_info tgl_info = {
-	.quirks = PMT_QUIRK_NO_WATCHER | PMT_QUIRK_NO_CRASHLOG |
-		  PMT_QUIRK_TABLE_SHIFT,
-};
-
-/* DG1 Platform with DVSEC quirk*/
-static struct intel_dvsec_header dg1_telemetry = {
-	.length = 0x10,
-	.id = 2,
-	.num_entries = 1,
-	.entry_size = 3,
-	.tbir = 0,
-	.offset = 0x466000,
-};
-
-static struct intel_dvsec_header *dg1_capabilities[] = {
-	&dg1_telemetry,
-	NULL
-};
-
-static const struct pmt_platform_info dg1_info = {
-	.quirks = PMT_QUIRK_NO_DVSEC,
-	.capabilities = dg1_capabilities,
-};
-
-static int pmt_add_dev(struct pci_dev *pdev, struct intel_dvsec_header *header,
-		       unsigned long quirks)
-{
-	struct device *dev = &pdev->dev;
-	struct resource *res, *tmp;
-	struct mfd_cell *cell;
-	const char *name;
-	int count = header->num_entries;
-	int size = header->entry_size;
-	int id = header->id;
-	int i;
-
-	switch (id) {
-	case DVSEC_INTEL_ID_TELEMETRY:
-		name = "pmt_telemetry";
-		break;
-	case DVSEC_INTEL_ID_WATCHER:
-		if (quirks & PMT_QUIRK_NO_WATCHER) {
-			dev_info(dev, "Watcher not supported\n");
-			return -EINVAL;
-		}
-		name = "pmt_watcher";
-		break;
-	case DVSEC_INTEL_ID_CRASHLOG:
-		if (quirks & PMT_QUIRK_NO_CRASHLOG) {
-			dev_info(dev, "Crashlog not supported\n");
-			return -EINVAL;
-		}
-		name = "pmt_crashlog";
-		break;
-	default:
-		return -EINVAL;
-	}
-
-	if (!header->num_entries || !header->entry_size) {
-		dev_err(dev, "Invalid count or size for %s header\n", name);
-		return -EINVAL;
-	}
-
-	cell = devm_kzalloc(dev, sizeof(*cell), GFP_KERNEL);
-	if (!cell)
-		return -ENOMEM;
-
-	res = devm_kcalloc(dev, count, sizeof(*res), GFP_KERNEL);
-	if (!res)
-		return -ENOMEM;
-
-	if (quirks & PMT_QUIRK_TABLE_SHIFT)
-		header->offset >>= 3;
-
-	/*
-	 * The PMT DVSEC contains the starting offset and count for a block of
-	 * discovery tables, each providing access to monitoring facilities for
-	 * a section of the device. Create a resource list of these tables to
-	 * provide to the driver.
-	 */
-	for (i = 0, tmp = res; i < count; i++, tmp++) {
-		tmp->start = pdev->resource[header->tbir].start +
-			     header->offset + i * (size << 2);
-		tmp->end = tmp->start + (size << 2) - 1;
-		tmp->flags = IORESOURCE_MEM;
-	}
-
-	cell->resources = res;
-	cell->num_resources = count;
-	cell->name = name;
-
-	return devm_mfd_add_devices(dev, PLATFORM_DEVID_AUTO, cell, 1, NULL, 0,
-				    NULL);
-}
-
-static int pmt_pci_probe(struct pci_dev *pdev, const struct pci_device_id *id)
-{
-	struct pmt_platform_info *info;
-	unsigned long quirks = 0;
-	bool found_devices = false;
-	int ret, pos = 0;
-
-	ret = pcim_enable_device(pdev);
-	if (ret)
-		return ret;
-
-	info = (struct pmt_platform_info *)id->driver_data;
-
-	if (info)
-		quirks = info->quirks;
-
-	if (info && (info->quirks & PMT_QUIRK_NO_DVSEC)) {
-		struct intel_dvsec_header **header;
-
-		header = info->capabilities;
-		while (*header) {
-			ret = pmt_add_dev(pdev, *header, quirks);
-			if (ret)
-				dev_warn(&pdev->dev,
-					 "Failed to add device for DVSEC id %d\n",
-					 (*header)->id);
-			else
-				found_devices = true;
-
-			++header;
-		}
-	} else {
-		do {
-			struct intel_dvsec_header header;
-			u32 table;
-			u16 vid;
-
-			pos = pci_find_next_ext_capability(pdev, pos, PCI_EXT_CAP_ID_DVSEC);
-			if (!pos)
-				break;
-
-			pci_read_config_word(pdev, pos + PCI_DVSEC_HEADER1, &vid);
-			if (vid != PCI_VENDOR_ID_INTEL)
-				continue;
-
-			pci_read_config_word(pdev, pos + PCI_DVSEC_HEADER2,
-					     &header.id);
-			pci_read_config_byte(pdev, pos + INTEL_DVSEC_ENTRIES,
-					     &header.num_entries);
-			pci_read_config_byte(pdev, pos + INTEL_DVSEC_SIZE,
-					     &header.entry_size);
-			pci_read_config_dword(pdev, pos + INTEL_DVSEC_TABLE,
-					      &table);
-
-			header.tbir = INTEL_DVSEC_TABLE_BAR(table);
-			header.offset = INTEL_DVSEC_TABLE_OFFSET(table);
-
-			ret = pmt_add_dev(pdev, &header, quirks);
-			if (ret)
-				continue;
-
-			found_devices = true;
-		} while (true);
-	}
-
-	if (!found_devices)
-		return -ENODEV;
-
-	pm_runtime_put(&pdev->dev);
-	pm_runtime_allow(&pdev->dev);
-
-	return 0;
-}
-
-static void pmt_pci_remove(struct pci_dev *pdev)
-{
-	pm_runtime_forbid(&pdev->dev);
-	pm_runtime_get_sync(&pdev->dev);
-}
-
-#define PCI_DEVICE_ID_INTEL_PMT_ADL	0x467d
-#define PCI_DEVICE_ID_INTEL_PMT_DG1	0x490e
-#define PCI_DEVICE_ID_INTEL_PMT_OOBMSM	0x09a7
-#define PCI_DEVICE_ID_INTEL_PMT_TGL	0x9a0d
-static const struct pci_device_id pmt_pci_ids[] = {
-	{ PCI_DEVICE_DATA(INTEL, PMT_ADL, &tgl_info) },
-	{ PCI_DEVICE_DATA(INTEL, PMT_DG1, &dg1_info) },
-	{ PCI_DEVICE_DATA(INTEL, PMT_OOBMSM, NULL) },
-	{ PCI_DEVICE_DATA(INTEL, PMT_TGL, &tgl_info) },
-	{ }
-};
-MODULE_DEVICE_TABLE(pci, pmt_pci_ids);
-
-static struct pci_driver pmt_pci_driver = {
-	.name = "intel-pmt",
-	.id_table = pmt_pci_ids,
-	.probe = pmt_pci_probe,
-	.remove = pmt_pci_remove,
-};
-module_pci_driver(pmt_pci_driver);
-
-MODULE_AUTHOR("David E. Box <david.e.box@linux.intel.com>");
-MODULE_DESCRIPTION("Intel Platform Monitoring Technology PMT driver");
-MODULE_LICENSE("GPL v2");
diff --git a/drivers/platform/x86/intel/Kconfig b/drivers/platform/x86/intel/Kconfig
index 38ce3e344589..35a5d1a5eba8 100644
--- a/drivers/platform/x86/intel/Kconfig
+++ b/drivers/platform/x86/intel/Kconfig
@@ -184,4 +184,15 @@ config INTEL_UNCORE_FREQ_CONTROL
 	  To compile this driver as a module, choose M here: the module
 	  will be called intel-uncore-frequency.
 
+config INTEL_VSEC
+	tristate "Intel Vendor Specific Extended Capabilities Driver"
+	depends on PCI
+	select AUXILIARY_BUS
+	help
+	  Adds support for feature drivers exposed using Intel PCIe VSEC and
+	  DVSEC.
+
+	  To compile this driver as a module, choose M here: the module will
+	  be called intel_vsec.
+
 endif # X86_PLATFORM_DRIVERS_INTEL
diff --git a/drivers/platform/x86/intel/Makefile b/drivers/platform/x86/intel/Makefile
index 7c24be2423d8..8ecdf709fb17 100644
--- a/drivers/platform/x86/intel/Makefile
+++ b/drivers/platform/x86/intel/Makefile
@@ -26,6 +26,8 @@ intel_int0002_vgpio-y			:= int0002_vgpio.o
 obj-$(CONFIG_INTEL_INT0002_VGPIO)	+= intel_int0002_vgpio.o
 intel_oaktrail-y			:= oaktrail.o
 obj-$(CONFIG_INTEL_OAKTRAIL)		+= intel_oaktrail.o
+intel_vsec-y				:= vsec.o
+obj-$(CONFIG_INTEL_VSEC)		+= intel_vsec.o
 
 # Intel PMIC / PMC / P-Unit drivers
 intel_bxtwc_tmu-y			:= bxtwc_tmu.o
diff --git a/drivers/platform/x86/intel/pmt/Kconfig b/drivers/platform/x86/intel/pmt/Kconfig
index d630f883a717..e916fc966221 100644
--- a/drivers/platform/x86/intel/pmt/Kconfig
+++ b/drivers/platform/x86/intel/pmt/Kconfig
@@ -17,7 +17,7 @@ config INTEL_PMT_CLASS
 
 config INTEL_PMT_TELEMETRY
 	tristate "Intel Platform Monitoring Technology (PMT) Telemetry driver"
-	depends on MFD_INTEL_PMT
+	depends on INTEL_VSEC
 	select INTEL_PMT_CLASS
 	help
 	  The Intel Platform Monitory Technology (PMT) Telemetry driver provides
@@ -29,7 +29,7 @@ config INTEL_PMT_TELEMETRY
 
 config INTEL_PMT_CRASHLOG
 	tristate "Intel Platform Monitoring Technology (PMT) Crashlog driver"
-	depends on MFD_INTEL_PMT
+	depends on INTEL_VSEC
 	select INTEL_PMT_CLASS
 	help
 	  The Intel Platform Monitoring Technology (PMT) crashlog driver provides
diff --git a/drivers/platform/x86/intel/pmt/class.c b/drivers/platform/x86/intel/pmt/class.c
index 659b1073033c..1c9e3f3ea41c 100644
--- a/drivers/platform/x86/intel/pmt/class.c
+++ b/drivers/platform/x86/intel/pmt/class.c
@@ -13,6 +13,7 @@
 #include <linux/mm.h>
 #include <linux/pci.h>
 
+#include "../vsec.h"
 #include "class.h"
 
 #define PMT_XA_START		0
@@ -281,31 +282,29 @@ static int intel_pmt_dev_register(struct intel_pmt_entry *entry,
 	return ret;
 }
 
-int intel_pmt_dev_create(struct intel_pmt_entry *entry,
-			 struct intel_pmt_namespace *ns,
-			 struct platform_device *pdev, int idx)
+int intel_pmt_dev_create(struct intel_pmt_entry *entry, struct intel_pmt_namespace *ns,
+			 struct intel_vsec_device *intel_vsec_dev, int idx)
 {
+	struct device *dev = &intel_vsec_dev->auxdev.dev;
 	struct intel_pmt_header header;
 	struct resource	*disc_res;
-	int ret = -ENODEV;
+	int ret;
 
-	disc_res = platform_get_resource(pdev, IORESOURCE_MEM, idx);
-	if (!disc_res)
-		return ret;
+	disc_res = &intel_vsec_dev->resource[idx];
 
-	entry->disc_table = devm_platform_ioremap_resource(pdev, idx);
+	entry->disc_table = devm_ioremap_resource(dev, disc_res);
 	if (IS_ERR(entry->disc_table))
 		return PTR_ERR(entry->disc_table);
 
-	ret = ns->pmt_header_decode(entry, &header, &pdev->dev);
+	ret = ns->pmt_header_decode(entry, &header, dev);
 	if (ret)
 		return ret;
 
-	ret = intel_pmt_populate_entry(entry, &header, &pdev->dev, disc_res);
+	ret = intel_pmt_populate_entry(entry, &header, dev, disc_res);
 	if (ret)
 		return ret;
 
-	return intel_pmt_dev_register(entry, ns, &pdev->dev);
+	return intel_pmt_dev_register(entry, ns, dev);
 
 }
 EXPORT_SYMBOL_GPL(intel_pmt_dev_create);
diff --git a/drivers/platform/x86/intel/pmt/class.h b/drivers/platform/x86/intel/pmt/class.h
index 1337019c2873..db11d58867ce 100644
--- a/drivers/platform/x86/intel/pmt/class.h
+++ b/drivers/platform/x86/intel/pmt/class.h
@@ -2,13 +2,14 @@
 #ifndef _INTEL_PMT_CLASS_H
 #define _INTEL_PMT_CLASS_H
 
-#include <linux/platform_device.h>
 #include <linux/xarray.h>
 #include <linux/types.h>
 #include <linux/bits.h>
 #include <linux/err.h>
 #include <linux/io.h>
 
+#include "../vsec.h"
+
 /* PMT access types */
 #define ACCESS_BARID		2
 #define ACCESS_LOCAL		3
@@ -47,7 +48,7 @@ struct intel_pmt_namespace {
 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,
-			 struct platform_device *pdev, int idx);
+			 struct intel_vsec_device *dev, int idx);
 void intel_pmt_dev_destroy(struct intel_pmt_entry *entry,
 			   struct intel_pmt_namespace *ns);
 #endif
diff --git a/drivers/platform/x86/intel/pmt/crashlog.c b/drivers/platform/x86/intel/pmt/crashlog.c
index 1c1021f04d3c..34daf9df168b 100644
--- a/drivers/platform/x86/intel/pmt/crashlog.c
+++ b/drivers/platform/x86/intel/pmt/crashlog.c
@@ -8,6 +8,7 @@
  * Author: "Alexander Duyck" <alexander.h.duyck@linux.intel.com>
  */
 
+#include <linux/auxiliary_bus.h>
 #include <linux/kernel.h>
 #include <linux/module.h>
 #include <linux/pci.h>
@@ -15,10 +16,9 @@
 #include <linux/uaccess.h>
 #include <linux/overflow.h>
 
+#include "../vsec.h"
 #include "class.h"
 
-#define DRV_NAME		"pmt_crashlog"
-
 /* Crashlog discovery header types */
 #define CRASH_TYPE_OOBMSM	1
 
@@ -257,34 +257,34 @@ static struct intel_pmt_namespace pmt_crashlog_ns = {
 /*
  * initialization
  */
-static int pmt_crashlog_remove(struct platform_device *pdev)
+static void pmt_crashlog_remove(struct auxiliary_device *auxdev)
 {
-	struct pmt_crashlog_priv *priv = platform_get_drvdata(pdev);
+	struct pmt_crashlog_priv *priv = auxiliary_get_drvdata(auxdev);
 	int i;
 
 	for (i = 0; i < priv->num_entries; i++)
 		intel_pmt_dev_destroy(&priv->entry[i].entry, &pmt_crashlog_ns);
-
-	return 0;
 }
 
-static int pmt_crashlog_probe(struct platform_device *pdev)
+static int pmt_crashlog_probe(struct auxiliary_device *auxdev,
+			      const struct auxiliary_device_id *id)
 {
+	struct intel_vsec_device *intel_vsec_dev = auxdev_to_ivdev(auxdev);
 	struct pmt_crashlog_priv *priv;
 	size_t size;
 	int i, ret;
 
-	size = struct_size(priv, entry, pdev->num_resources);
-	priv = devm_kzalloc(&pdev->dev, size, GFP_KERNEL);
+	size = struct_size(priv, entry, intel_vsec_dev->num_resources);
+	priv = devm_kzalloc(&auxdev->dev, size, GFP_KERNEL);
 	if (!priv)
 		return -ENOMEM;
 
-	platform_set_drvdata(pdev, priv);
+	auxiliary_set_drvdata(auxdev, priv);
 
-	for (i = 0; i < pdev->num_resources; i++) {
+	for (i = 0; i < intel_vsec_dev->num_resources; i++) {
 		struct intel_pmt_entry *entry = &priv->entry[i].entry;
 
-		ret = intel_pmt_dev_create(entry, &pmt_crashlog_ns, pdev, i);
+		ret = intel_pmt_dev_create(entry, &pmt_crashlog_ns, intel_vsec_dev, i);
 		if (ret < 0)
 			goto abort_probe;
 		if (ret)
@@ -295,26 +295,30 @@ static int pmt_crashlog_probe(struct platform_device *pdev)
 
 	return 0;
 abort_probe:
-	pmt_crashlog_remove(pdev);
+	pmt_crashlog_remove(auxdev);
 	return ret;
 }
 
-static struct platform_driver pmt_crashlog_driver = {
-	.driver = {
-		.name   = DRV_NAME,
-	},
-	.remove = pmt_crashlog_remove,
-	.probe  = pmt_crashlog_probe,
+static const struct auxiliary_device_id pmt_crashlog_id_table[] = {
+	{ .name = "intel_vsec.crashlog" },
+	{}
+};
+MODULE_DEVICE_TABLE(auxiliary, pmt_crashlog_id_table);
+
+static struct auxiliary_driver pmt_crashlog_aux_driver = {
+	.id_table	= pmt_crashlog_id_table,
+	.remove		= pmt_crashlog_remove,
+	.probe		= pmt_crashlog_probe,
 };
 
 static int __init pmt_crashlog_init(void)
 {
-	return platform_driver_register(&pmt_crashlog_driver);
+	return auxiliary_driver_register(&pmt_crashlog_aux_driver);
 }
 
 static void __exit pmt_crashlog_exit(void)
 {
-	platform_driver_unregister(&pmt_crashlog_driver);
+	auxiliary_driver_unregister(&pmt_crashlog_aux_driver);
 	xa_destroy(&crashlog_array);
 }
 
@@ -323,5 +327,4 @@ module_exit(pmt_crashlog_exit);
 
 MODULE_AUTHOR("Alexander Duyck <alexander.h.duyck@linux.intel.com>");
 MODULE_DESCRIPTION("Intel PMT Crashlog driver");
-MODULE_ALIAS("platform:" DRV_NAME);
 MODULE_LICENSE("GPL v2");
diff --git a/drivers/platform/x86/intel/pmt/telemetry.c b/drivers/platform/x86/intel/pmt/telemetry.c
index 38d52651c572..6b6f3e2a617a 100644
--- a/drivers/platform/x86/intel/pmt/telemetry.c
+++ b/drivers/platform/x86/intel/pmt/telemetry.c
@@ -8,6 +8,7 @@
  * Author: "David E. Box" <david.e.box@linux.intel.com>
  */
 
+#include <linux/auxiliary_bus.h>
 #include <linux/kernel.h>
 #include <linux/module.h>
 #include <linux/pci.h>
@@ -15,10 +16,9 @@
 #include <linux/uaccess.h>
 #include <linux/overflow.h>
 
+#include "../vsec.h"
 #include "class.h"
 
-#define TELEM_DEV_NAME		"pmt_telemetry"
-
 #define TELEM_SIZE_OFFSET	0x0
 #define TELEM_GUID_OFFSET	0x4
 #define TELEM_BASE_OFFSET	0x8
@@ -79,34 +79,33 @@ static struct intel_pmt_namespace pmt_telem_ns = {
 	.pmt_header_decode = pmt_telem_header_decode,
 };
 
-static int pmt_telem_remove(struct platform_device *pdev)
+static void pmt_telem_remove(struct auxiliary_device *auxdev)
 {
-	struct pmt_telem_priv *priv = platform_get_drvdata(pdev);
+	struct pmt_telem_priv *priv = auxiliary_get_drvdata(auxdev);
 	int i;
 
 	for (i = 0; i < priv->num_entries; i++)
 		intel_pmt_dev_destroy(&priv->entry[i], &pmt_telem_ns);
-
-	return 0;
 }
 
-static int pmt_telem_probe(struct platform_device *pdev)
+static int pmt_telem_probe(struct auxiliary_device *auxdev, const struct auxiliary_device_id *id)
 {
+	struct intel_vsec_device *intel_vsec_dev = auxdev_to_ivdev(auxdev);
 	struct pmt_telem_priv *priv;
 	size_t size;
 	int i, ret;
 
-	size = struct_size(priv, entry, pdev->num_resources);
-	priv = devm_kzalloc(&pdev->dev, size, GFP_KERNEL);
+	size = struct_size(priv, entry, intel_vsec_dev->num_resources);
+	priv = devm_kzalloc(&auxdev->dev, size, GFP_KERNEL);
 	if (!priv)
 		return -ENOMEM;
 
-	platform_set_drvdata(pdev, priv);
+	auxiliary_set_drvdata(auxdev, priv);
 
-	for (i = 0; i < pdev->num_resources; i++) {
+	for (i = 0; i < intel_vsec_dev->num_resources; i++) {
 		struct intel_pmt_entry *entry = &priv->entry[i];
 
-		ret = intel_pmt_dev_create(entry, &pmt_telem_ns, pdev, i);
+		ret = intel_pmt_dev_create(entry, &pmt_telem_ns, intel_vsec_dev, i);
 		if (ret < 0)
 			goto abort_probe;
 		if (ret)
@@ -117,32 +116,35 @@ static int pmt_telem_probe(struct platform_device *pdev)
 
 	return 0;
 abort_probe:
-	pmt_telem_remove(pdev);
+	pmt_telem_remove(auxdev);
 	return ret;
 }
 
-static struct platform_driver pmt_telem_driver = {
-	.driver = {
-		.name   = TELEM_DEV_NAME,
-	},
-	.remove = pmt_telem_remove,
-	.probe  = pmt_telem_probe,
+static const struct auxiliary_device_id pmt_telem_id_table[] = {
+	{ .name = "intel_vsec.telemetry" },
+	{}
+};
+MODULE_DEVICE_TABLE(auxiliary, pmt_telem_id_table);
+
+static struct auxiliary_driver pmt_telem_aux_driver = {
+	.id_table	= pmt_telem_id_table,
+	.remove		= pmt_telem_remove,
+	.probe		= pmt_telem_probe,
 };
 
 static int __init pmt_telem_init(void)
 {
-	return platform_driver_register(&pmt_telem_driver);
+	return auxiliary_driver_register(&pmt_telem_aux_driver);
 }
 module_init(pmt_telem_init);
 
 static void __exit pmt_telem_exit(void)
 {
-	platform_driver_unregister(&pmt_telem_driver);
+	auxiliary_driver_unregister(&pmt_telem_aux_driver);
 	xa_destroy(&telem_array);
 }
 module_exit(pmt_telem_exit);
 
 MODULE_AUTHOR("David E. Box <david.e.box@linux.intel.com>");
 MODULE_DESCRIPTION("Intel PMT Telemetry driver");
-MODULE_ALIAS("platform:" TELEM_DEV_NAME);
 MODULE_LICENSE("GPL v2");
diff --git a/drivers/platform/x86/intel/vsec.c b/drivers/platform/x86/intel/vsec.c
new file mode 100644
index 000000000000..c3bdd75ed690
--- /dev/null
+++ b/drivers/platform/x86/intel/vsec.c
@@ -0,0 +1,408 @@
+// SPDX-License-Identifier: GPL-2.0
+/*
+ * Intel Vendor Specific Extended Capabilities auxiliary bus driver
+ *
+ * Copyright (c) 2021, Intel Corporation.
+ * All Rights Reserved.
+ *
+ * Author: David E. Box <david.e.box@linux.intel.com>
+ *
+ * This driver discovers and creates auxiliary devices for Intel defined PCIe
+ * "Vendor Specific" and "Designated Vendor Specific" Extended Capabilities,
+ * VSEC and DVSEC respectively. The driver supports features on specific PCIe
+ * endpoints that exist primarily to expose them.
+ */
+
+#include <linux/auxiliary_bus.h>
+#include <linux/bits.h>
+#include <linux/kernel.h>
+#include <linux/idr.h>
+#include <linux/module.h>
+#include <linux/pci.h>
+#include <linux/types.h>
+
+#include "vsec.h"
+
+/* Intel DVSEC offsets */
+#define INTEL_DVSEC_ENTRIES		0xA
+#define INTEL_DVSEC_SIZE		0xB
+#define INTEL_DVSEC_TABLE		0xC
+#define INTEL_DVSEC_TABLE_BAR(x)	((x) & GENMASK(2, 0))
+#define INTEL_DVSEC_TABLE_OFFSET(x)	((x) & GENMASK(31, 3))
+#define TABLE_OFFSET_SHIFT		3
+
+static DEFINE_IDA(intel_vsec_ida);
+
+/**
+ * struct intel_vsec_header - Common fields of Intel VSEC and DVSEC registers.
+ * @rev:         Revision ID of the VSEC/DVSEC register space
+ * @length:      Length of the VSEC/DVSEC register space
+ * @id:          ID of the feature
+ * @num_entries: Number of instances of the feature
+ * @entry_size:  Size of the discovery table for each feature
+ * @tbir:        BAR containing the discovery tables
+ * @offset:      BAR offset of start of the first discovery table
+ */
+struct intel_vsec_header {
+	u8	rev;
+	u16	length;
+	u16	id;
+	u8	num_entries;
+	u8	entry_size;
+	u8	tbir;
+	u32	offset;
+};
+
+/* Platform specific data */
+struct intel_vsec_platform_info {
+	struct intel_vsec_header **capabilities;
+	unsigned long quirks;
+};
+
+enum intel_vsec_id {
+	VSEC_ID_TELEMETRY	= 2,
+	VSEC_ID_WATCHER		= 3,
+	VSEC_ID_CRASHLOG	= 4,
+};
+
+static enum intel_vsec_id intel_vsec_allow_list[] = {
+	VSEC_ID_TELEMETRY,
+	VSEC_ID_WATCHER,
+	VSEC_ID_CRASHLOG,
+};
+
+static const char *intel_vsec_name(enum intel_vsec_id id)
+{
+	switch (id) {
+	case VSEC_ID_TELEMETRY:
+		return "telemetry";
+
+	case VSEC_ID_WATCHER:
+		return "watcher";
+
+	case VSEC_ID_CRASHLOG:
+		return "crashlog";
+
+	default:
+		return NULL;
+	}
+}
+
+static bool intel_vsec_allowed(u16 id)
+{
+	int i;
+
+	for (i = 0; i < ARRAY_SIZE(intel_vsec_allow_list); i++)
+		if (intel_vsec_allow_list[i] == id)
+			return true;
+
+	return false;
+}
+
+static bool intel_vsec_disabled(u16 id, unsigned long quirks)
+{
+	switch (id) {
+	case VSEC_ID_WATCHER:
+		return !!(quirks & VSEC_QUIRK_NO_WATCHER);
+
+	case VSEC_ID_CRASHLOG:
+		return !!(quirks & VSEC_QUIRK_NO_CRASHLOG);
+
+	default:
+		return false;
+	}
+}
+
+static void intel_vsec_remove_aux(void *data)
+{
+	auxiliary_device_delete(data);
+	auxiliary_device_uninit(data);
+}
+
+static void intel_vsec_dev_release(struct device *dev)
+{
+	struct intel_vsec_device *intel_vsec_dev = dev_to_ivdev(dev);
+
+	ida_free(intel_vsec_dev->ida, intel_vsec_dev->auxdev.id);
+	kfree(intel_vsec_dev->resource);
+	kfree(intel_vsec_dev);
+}
+
+static int intel_vsec_add_aux(struct pci_dev *pdev, struct intel_vsec_device *intel_vsec_dev,
+			      const char *name)
+{
+	struct auxiliary_device *auxdev = &intel_vsec_dev->auxdev;
+	int ret;
+
+	ret = ida_alloc(intel_vsec_dev->ida, GFP_KERNEL);
+	if (ret < 0) {
+		kfree(intel_vsec_dev);
+		return ret;
+	}
+
+	auxdev->id = ret;
+	auxdev->name = name;
+	auxdev->dev.parent = &pdev->dev;
+	auxdev->dev.release = intel_vsec_dev_release;
+
+	ret = auxiliary_device_init(auxdev);
+	if (ret < 0) {
+		ida_free(intel_vsec_dev->ida, auxdev->id);
+		kfree(intel_vsec_dev->resource);
+		kfree(intel_vsec_dev);
+		return ret;
+	}
+
+	ret = auxiliary_device_add(auxdev);
+	if (ret < 0) {
+		auxiliary_device_uninit(auxdev);
+		return ret;
+	}
+
+	return devm_add_action_or_reset(&pdev->dev, intel_vsec_remove_aux, auxdev);
+}
+
+static int intel_vsec_add_dev(struct pci_dev *pdev, struct intel_vsec_header *header,
+			   unsigned long quirks)
+{
+	struct intel_vsec_device *intel_vsec_dev;
+	struct resource *res, *tmp;
+	int i;
+
+	if (!intel_vsec_allowed(header->id) || intel_vsec_disabled(header->id, quirks))
+		return -EINVAL;
+
+	if (!header->num_entries) {
+		dev_dbg(&pdev->dev, "Invalid 0 entry count for header id %d\n", header->id);
+		return -EINVAL;
+	}
+
+	if (!header->entry_size) {
+		dev_dbg(&pdev->dev, "Invalid 0 entry size for header id %d\n", header->id);
+		return -EINVAL;
+	}
+
+	intel_vsec_dev = kzalloc(sizeof(*intel_vsec_dev), GFP_KERNEL);
+	if (!intel_vsec_dev)
+		return -ENOMEM;
+
+	res = kcalloc(header->num_entries, sizeof(*res), GFP_KERNEL);
+	if (!res) {
+		kfree(intel_vsec_dev);
+		return -ENOMEM;
+	}
+
+	if (quirks & VSEC_QUIRK_TABLE_SHIFT)
+		header->offset >>= TABLE_OFFSET_SHIFT;
+
+	/*
+	 * The DVSEC/VSEC contains the starting offset and count for a block of
+	 * discovery tables. Create a resource array of these tables to the
+	 * auxiliary device driver.
+	 */
+	for (i = 0, tmp = res; i < header->num_entries; i++, tmp++) {
+		tmp->start = pdev->resource[header->tbir].start +
+			     header->offset + i * (header->entry_size * sizeof(u32));
+		tmp->end = tmp->start + (header->entry_size * sizeof(u32)) - 1;
+		tmp->flags = IORESOURCE_MEM;
+	}
+
+	intel_vsec_dev->pcidev = pdev;
+	intel_vsec_dev->resource = res;
+	intel_vsec_dev->num_resources = header->num_entries;
+	intel_vsec_dev->quirks = quirks;
+	intel_vsec_dev->ida = &intel_vsec_ida;
+
+	return intel_vsec_add_aux(pdev, intel_vsec_dev, intel_vsec_name(header->id));
+}
+
+static bool intel_vsec_walk_header(struct pci_dev *pdev, unsigned long quirks,
+				struct intel_vsec_header **header)
+{
+	bool have_devices = false;
+	int ret;
+
+	for ( ; *header; header++) {
+		ret = intel_vsec_add_dev(pdev, *header, quirks);
+		if (ret)
+			dev_info(&pdev->dev, "Could not add device for DVSEC id %d\n",
+				 (*header)->id);
+		else
+			have_devices = true;
+	}
+
+	return have_devices;
+}
+
+static bool intel_vsec_walk_dvsec(struct pci_dev *pdev, unsigned long quirks)
+{
+	bool have_devices = false;
+	int pos = 0;
+
+	do {
+		struct intel_vsec_header header;
+		u32 table, hdr;
+		u16 vid;
+		int ret;
+
+		pos = pci_find_next_ext_capability(pdev, pos, PCI_EXT_CAP_ID_DVSEC);
+		if (!pos)
+			break;
+
+		pci_read_config_dword(pdev, pos + PCI_DVSEC_HEADER1, &hdr);
+		vid = PCI_DVSEC_HEADER1_VID(hdr);
+		if (vid != PCI_VENDOR_ID_INTEL)
+			continue;
+
+		/* Support only revision 1 */
+		header.rev = PCI_DVSEC_HEADER1_REV(hdr);
+		if (header.rev != 1) {
+			dev_info(&pdev->dev, "Unsupported DVSEC revision %d\n", header.rev);
+			continue;
+		}
+
+		header.length = PCI_DVSEC_HEADER1_LEN(hdr);
+
+		pci_read_config_byte(pdev, pos + INTEL_DVSEC_ENTRIES, &header.num_entries);
+		pci_read_config_byte(pdev, pos + INTEL_DVSEC_SIZE, &header.entry_size);
+		pci_read_config_dword(pdev, pos + INTEL_DVSEC_TABLE, &table);
+
+		header.tbir = INTEL_DVSEC_TABLE_BAR(table);
+		header.offset = INTEL_DVSEC_TABLE_OFFSET(table);
+
+		pci_read_config_dword(pdev, pos + PCI_DVSEC_HEADER2, &hdr);
+		header.id = PCI_DVSEC_HEADER2_ID(hdr);
+
+		ret = intel_vsec_add_dev(pdev, &header, quirks);
+		if (ret)
+			continue;
+
+		have_devices = true;
+	} while (true);
+
+	return have_devices;
+}
+
+static bool intel_vsec_walk_vsec(struct pci_dev *pdev, unsigned long quirks)
+{
+	bool have_devices = false;
+	int pos = 0;
+
+	do {
+		struct intel_vsec_header header;
+		u32 table, hdr;
+		int ret;
+
+		pos = pci_find_next_ext_capability(pdev, pos, PCI_EXT_CAP_ID_VNDR);
+		if (!pos)
+			break;
+
+		pci_read_config_dword(pdev, pos + PCI_VNDR_HEADER, &hdr);
+
+		/* Support only revision 1 */
+		header.rev = PCI_VNDR_HEADER_REV(hdr);
+		if (header.rev != 1) {
+			dev_info(&pdev->dev, "Unsupported VSEC revision %d\n", header.rev);
+			continue;
+		}
+
+		header.id = PCI_VNDR_HEADER_ID(hdr);
+		header.length = PCI_VNDR_HEADER_LEN(hdr);
+
+		/* entry, size, and table offset are the same as DVSEC */
+		pci_read_config_byte(pdev, pos + INTEL_DVSEC_ENTRIES, &header.num_entries);
+		pci_read_config_byte(pdev, pos + INTEL_DVSEC_SIZE, &header.entry_size);
+		pci_read_config_dword(pdev, pos + INTEL_DVSEC_TABLE, &table);
+
+		header.tbir = INTEL_DVSEC_TABLE_BAR(table);
+		header.offset = INTEL_DVSEC_TABLE_OFFSET(table);
+
+		ret = intel_vsec_add_dev(pdev, &header, quirks);
+		if (ret)
+			continue;
+
+		have_devices = true;
+	} while (true);
+
+	return have_devices;
+}
+
+static int intel_vsec_pci_probe(struct pci_dev *pdev, const struct pci_device_id *id)
+{
+	struct intel_vsec_platform_info *info;
+	bool have_devices = false;
+	unsigned long quirks = 0;
+	int ret;
+
+	ret = pcim_enable_device(pdev);
+	if (ret)
+		return ret;
+
+	info = (struct intel_vsec_platform_info *)id->driver_data;
+	if (info)
+		quirks = info->quirks;
+
+	if (intel_vsec_walk_dvsec(pdev, quirks))
+		have_devices = true;
+
+	if (intel_vsec_walk_vsec(pdev, quirks))
+		have_devices = true;
+
+	if (info && (info->quirks & VSEC_QUIRK_NO_DVSEC) &&
+	    intel_vsec_walk_header(pdev, quirks, info->capabilities))
+		have_devices = true;
+
+	if (!have_devices)
+		return -ENODEV;
+
+	return 0;
+}
+
+/* TGL info */
+static const struct intel_vsec_platform_info tgl_info = {
+	.quirks = VSEC_QUIRK_NO_WATCHER | VSEC_QUIRK_NO_CRASHLOG | VSEC_QUIRK_TABLE_SHIFT,
+};
+
+/* DG1 info */
+static struct intel_vsec_header dg1_telemetry = {
+	.length = 0x10,
+	.id = 2,
+	.num_entries = 1,
+	.entry_size = 3,
+	.tbir = 0,
+	.offset = 0x466000,
+};
+
+static struct intel_vsec_header *dg1_capabilities[] = {
+	&dg1_telemetry,
+	NULL
+};
+
+static const struct intel_vsec_platform_info dg1_info = {
+	.capabilities = dg1_capabilities,
+	.quirks = VSEC_QUIRK_NO_DVSEC,
+};
+
+#define PCI_DEVICE_ID_INTEL_VSEC_ADL		0x467d
+#define PCI_DEVICE_ID_INTEL_VSEC_DG1		0x490e
+#define PCI_DEVICE_ID_INTEL_VSEC_OOBMSM		0x09a7
+#define PCI_DEVICE_ID_INTEL_VSEC_TGL		0x9a0d
+static const struct pci_device_id intel_vsec_pci_ids[] = {
+	{ PCI_DEVICE_DATA(INTEL, VSEC_ADL, &tgl_info) },
+	{ PCI_DEVICE_DATA(INTEL, VSEC_DG1, &dg1_info) },
+	{ PCI_DEVICE_DATA(INTEL, VSEC_OOBMSM, NULL) },
+	{ PCI_DEVICE_DATA(INTEL, VSEC_TGL, &tgl_info) },
+	{ }
+};
+MODULE_DEVICE_TABLE(pci, intel_vsec_pci_ids);
+
+static struct pci_driver intel_vsec_pci_driver = {
+	.name = "intel_vsec",
+	.id_table = intel_vsec_pci_ids,
+	.probe = intel_vsec_pci_probe,
+};
+module_pci_driver(intel_vsec_pci_driver);
+
+MODULE_AUTHOR("David E. Box <david.e.box@linux.intel.com>");
+MODULE_DESCRIPTION("Intel Extended Capabilities auxiliary bus driver");
+MODULE_LICENSE("GPL v2");
diff --git a/drivers/platform/x86/intel/vsec.h b/drivers/platform/x86/intel/vsec.h
new file mode 100644
index 000000000000..4cc36678e8c5
--- /dev/null
+++ b/drivers/platform/x86/intel/vsec.h
@@ -0,0 +1,43 @@
+/* SPDX-License-Identifier: GPL-2.0 */
+#ifndef _VSEC_H
+#define _VSEC_H
+
+#include <linux/auxiliary_bus.h>
+#include <linux/bits.h>
+
+struct pci_dev;
+struct resource;
+
+enum intel_vsec_quirks {
+	/* Watcher feature not supported */
+	VSEC_QUIRK_NO_WATCHER	= BIT(0),
+
+	/* Crashlog feature not supported */
+	VSEC_QUIRK_NO_CRASHLOG	= BIT(1),
+
+	/* Use shift instead of mask to read discovery table offset */
+	VSEC_QUIRK_TABLE_SHIFT	= BIT(2),
+
+	/* DVSEC not present (provided in driver data) */
+	VSEC_QUIRK_NO_DVSEC	= BIT(3),
+};
+
+struct intel_vsec_device {
+	struct auxiliary_device auxdev;
+	struct pci_dev *pcidev;
+	struct resource *resource;
+	struct ida *ida;
+	unsigned long quirks;
+	int num_resources;
+};
+
+static inline struct intel_vsec_device *dev_to_ivdev(struct device *dev)
+{
+	return container_of(dev, struct intel_vsec_device, auxdev.dev);
+}
+
+static inline struct intel_vsec_device *auxdev_to_ivdev(struct auxiliary_device *auxdev)
+{
+	return container_of(auxdev, struct intel_vsec_device, auxdev);
+}
+#endif
-- 
2.25.1

[V2 4/6] platform/x86: Add Intel Software Defined Silicon driver

[David E. Box]

Intel Software Defined Silicon (SDSi) is a post manufacturing mechanism for
activating additional silicon features. Features are enabled through a
license activation process.  The SDSi driver provides a per socket, sysfs
attribute interface for applications to perform 3 main provisioning
functions:

1. Provision an Authentication Key Certificate (AKC), a key written to
   internal NVRAM that is used to authenticate a capability specific
   activation payload.

2. Provision a Capability Activation Payload (CAP), a token authenticated
   using the AKC and applied to the CPU configuration to activate a new
   feature.

3. Read the SDSi State Certificate, containing the CPU configuration
   state.

The operations perform function specific mailbox commands that forward the
requests to SDSi hardware to perform authentication of the payloads and
enable the silicon configuration (to be made available after power
cycling).

The SDSi device itself is enumerated as an auxiliary device from the
intel_vsec driver and as such has a build dependency on CONFIG_INTEL_VSEC.

Link: https://github.com/intel/intel-sdsi
Signed-off-by: David E. Box <david.e.box@linux.intel.com>
Reviewed-by: Mark Gross <markgross@kernel.org>
---
V2
  - Use sysfs_emit() in guid_show()
  - Fix language in ABI, suggested by Bjorn
  - Fix wrong directory name in ABI doc

 .../ABI/testing/sysfs-driver-intel_sdsi       |  77 +++
 MAINTAINERS                                   |   5 +
 drivers/platform/x86/intel/Kconfig            |  12 +
 drivers/platform/x86/intel/Makefile           |   2 +
 drivers/platform/x86/intel/sdsi.c             | 571 ++++++++++++++++++
 drivers/platform/x86/intel/vsec.c             |  12 +-
 6 files changed, 678 insertions(+), 1 deletion(-)
 create mode 100644 Documentation/ABI/testing/sysfs-driver-intel_sdsi
 create mode 100644 drivers/platform/x86/intel/sdsi.c

diff --git a/Documentation/ABI/testing/sysfs-driver-intel_sdsi b/Documentation/ABI/testing/sysfs-driver-intel_sdsi
new file mode 100644
index 000000000000..6544f35abb98
--- /dev/null
+++ b/Documentation/ABI/testing/sysfs-driver-intel_sdsi
@@ -0,0 +1,77 @@
+What:		/sys/bus/auxiliary/devices/intel_vsec.sdsi.X
+Date:		Dec 2021
+KernelVersion:	5.17
+Contact:	"David E. Box" <david.e.box@linux.intel.com>
+Description:
+		This folder contains interface files for accessing Intel
+		Software Defined Silicon (SDSi) features on a CPU. X
+		represents the socket instance (though not the socket ID).
+		The socket ID is determined by reading the registers file
+		and decoding it per the specification.
+
+		Some files communicate with SDSi hardware through a mailbox.
+		Should the operation fail, one of the following error codes
+		may be returned:
+
+		Error Code	Cause
+	        ----------	-----
+	        EIO		General mailbox failure. Log may indicate cause.
+	        EBUSY		Mailbox is owned by another agent.
+	        EPERM		SDSI capability is not enabled in hardware.
+	        EPROTO		Failure in mailbox protocol detected by driver.
+				See log for details.
+	        EOVERFLOW	For provision commands, the size of the data
+				exceeds what may be written.
+	        ESPIPE		Seeking is not allowed.
+	        ETIMEDOUT	Failure to complete mailbox transaction in time.
+
+What:		/sys/bus/auxiliary/devices/intel_vsec.sdsi.X/guid
+Date:		Dec 2021
+KernelVersion:	5.17
+Contact:	"David E. Box" <david.e.box@linux.intel.com>
+Description:
+		(RO) The GUID for the registers file. The GUID identifies
+		the layout of the registers file in this folder.
+		Information about the register layouts for a particular GUID
+		is available at http://github.com/intel/intel-sdsi
+
+What:		/sys/bus/auxiliary/devices/intel_vsec.sdsi.X/registers
+Date:		Dec 2021
+KernelVersion:	5.17
+Contact:	"David E. Box" <david.e.box@linux.intel.com>
+Description:
+		(RO) Contains information needed by applications to provision
+		a CPU and monitor status information. The layout of this file
+		is determined by the GUID in this folder. Information about the
+		layout for a particular GUID is available at
+		http://github.com/intel/intel-sdsi
+
+What:		/sys/bus/auxiliary/devices/intel_vsec.sdsi.X/provision_akc
+Date:		Dec 2021
+KernelVersion:	5.17
+Contact:	"David E. Box" <david.e.box@linux.intel.com>
+Description:
+		(WO) Used to write an Authentication Key Certificate (AKC) to
+		the SDSi NVRAM for the CPU. The AKC is used to authenticate a
+		Capability Activation Payload. Mailbox command.
+
+What:		/sys/bus/auxiliary/devices/intel_vsec.sdsi.X/provision_cap
+Date:		Dec 2021
+KernelVersion:	5.17
+Contact:	"David E. Box" <david.e.box@linux.intel.com>
+Description:
+		(WO) Used to write a Capability Activation Payload (CAP) to the
+		SDSi NVRAM for the CPU. CAPs are used to activate a given CPU
+		feature. A CAP is validated by SDSi hardware using a previously
+		provisioned AKC file. Upon successful authentication, the CPU
+		configuration is updated. A cold reboot is required to fully
+		activate the feature. Mailbox command.
+
+What:		/sys/bus/auxiliary/devices/intel_vsec.sdsi.X/state_certificate
+Date:		Dec 2021
+KernelVersion:	5.17
+Contact:	"David E. Box" <david.e.box@linux.intel.com>
+Description:
+		(RO) Used to read back the current State Certificate for the CPU
+		from SDSi hardware. The State Certificate contains information
+		about the current licenses on the CPU. Mailbox command.
diff --git a/MAINTAINERS b/MAINTAINERS
index cd2b10a86f09..af7f17e7400f 100644
--- a/MAINTAINERS
+++ b/MAINTAINERS
@@ -9783,6 +9783,11 @@ S:	Maintained
 F:	arch/x86/include/asm/intel_scu_ipc.h
 F:	drivers/platform/x86/intel_scu_*
 
+INTEL SDSI DRIVER
+M:	David E. Box <david.e.box@linux.intel.com>
+S:	Supported
+F:	drivers/platform/x86/intel/sdsi.c
+
 INTEL SKYLAKE INT3472 ACPI DEVICE DRIVER
 M:	Daniel Scally <djrscally@gmail.com>
 S:	Maintained
diff --git a/drivers/platform/x86/intel/Kconfig b/drivers/platform/x86/intel/Kconfig
index 35a5d1a5eba8..0c24b626c6ed 100644
--- a/drivers/platform/x86/intel/Kconfig
+++ b/drivers/platform/x86/intel/Kconfig
@@ -147,6 +147,18 @@ config INTEL_RST
 	  firmware will copy the memory contents back to RAM and resume the OS
 	  as usual.
 
+config INTEL_SDSI
+	tristate "Intel Software Defined Silicon Driver"
+	depends on INTEL_VSEC
+	depends on X86_64
+	help
+	  This driver enables access to the Intel Software Defined Silicon
+	  interface used to provision silicon features with an authentication
+	  certificate and capability license.
+
+	  To compile this driver as a module, choose M here: the module will
+	  be called intel_sdsi.
+
 config INTEL_SMARTCONNECT
 	tristate "Intel Smart Connect disabling driver"
 	depends on ACPI
diff --git a/drivers/platform/x86/intel/Makefile b/drivers/platform/x86/intel/Makefile
index 8ecdf709fb17..6a3a4510d89a 100644
--- a/drivers/platform/x86/intel/Makefile
+++ b/drivers/platform/x86/intel/Makefile
@@ -26,6 +26,8 @@ intel_int0002_vgpio-y			:= int0002_vgpio.o
 obj-$(CONFIG_INTEL_INT0002_VGPIO)	+= intel_int0002_vgpio.o
 intel_oaktrail-y			:= oaktrail.o
 obj-$(CONFIG_INTEL_OAKTRAIL)		+= intel_oaktrail.o
+intel_sdsi-y				:= sdsi.o
+obj-$(CONFIG_INTEL_SDSI)		+= intel_sdsi.o
 intel_vsec-y				:= vsec.o
 obj-$(CONFIG_INTEL_VSEC)		+= intel_vsec.o
 
diff --git a/drivers/platform/x86/intel/sdsi.c b/drivers/platform/x86/intel/sdsi.c
new file mode 100644
index 000000000000..626fe9558bee
--- /dev/null
+++ b/drivers/platform/x86/intel/sdsi.c
@@ -0,0 +1,571 @@
+// SPDX-License-Identifier: GPL-2.0
+/*
+ * Intel Software Defined Silicon driver
+ *
+ * Copyright (c) 2021, Intel Corporation.
+ * All Rights Reserved.
+ *
+ * Author: "David E. Box" <david.e.box@linux.intel.com>
+ */
+
+#include <linux/auxiliary_bus.h>
+#include <linux/bits.h>
+#include <linux/bitfield.h>
+#include <linux/device.h>
+#include <linux/iopoll.h>
+#include <linux/kernel.h>
+#include <linux/module.h>
+#include <linux/pci.h>
+#include <linux/slab.h>
+#include <linux/sysfs.h>
+#include <linux/types.h>
+#include <linux/uaccess.h>
+
+#include "vsec.h"
+
+#define ACCESS_TYPE_BARID		2
+#define ACCESS_TYPE_LOCAL		3
+
+#define SDSI_MIN_SIZE_DWORDS		276
+#define SDSI_SIZE_CONTROL		8
+#define SDSI_SIZE_MAILBOX		1024
+#define SDSI_SIZE_REGS			72
+#define SDSI_SIZE_CMD			sizeof(u64)
+
+/*
+ * Write messages are currently up to the size of the mailbox
+ * while read messages are up to 4 times the size of the
+ * mailbox, sent in packets
+ */
+#define SDSI_SIZE_WRITE_MSG		SDSI_SIZE_MAILBOX
+#define SDSI_SIZE_READ_MSG		(SDSI_SIZE_MAILBOX * 4)
+
+#define SDSI_ENABLED_FEATURES_OFFSET	16
+#define SDSI_ENABLED			BIT(3)
+#define SDSI_SOCKET_ID_OFFSET		64
+#define SDSI_SOCKET_ID			GENMASK(3, 0)
+
+#define SDSI_MBOX_CMD_SUCCESS		0x40
+#define SDSI_MBOX_CMD_TIMEOUT		0x80
+
+#define MBOX_TIMEOUT_US			2000
+#define MBOX_TIMEOUT_ACQUIRE_US		1000
+#define MBOX_POLLING_PERIOD_US		100
+#define MBOX_MAX_PACKETS		4
+
+#define MBOX_OWNER_NONE			0x00
+#define MBOX_OWNER_INBAND		0x01
+
+#define CTRL_RUN_BUSY			BIT(0)
+#define CTRL_READ_WRITE			BIT(1)
+#define CTRL_SOM			BIT(2)
+#define CTRL_EOM			BIT(3)
+#define CTRL_OWNER			GENMASK(5, 4)
+#define CTRL_COMPLETE			BIT(6)
+#define CTRL_READY			BIT(7)
+#define CTRL_STATUS			GENMASK(15, 8)
+#define CTRL_PACKET_SIZE		GENMASK(31, 16)
+#define CTRL_MSG_SIZE			GENMASK(63, 48)
+
+#define DISC_TABLE_SIZE			12
+#define DT_ACCESS_TYPE			GENMASK(3, 0)
+#define DT_SIZE				GENMASK(27, 12)
+#define DT_TBIR				GENMASK(2, 0)
+#define DT_OFFSET(v)			((v) & GENMASK(31, 3))
+
+enum sdsi_command {
+	SDSI_CMD_PROVISION_AKC		= 0x04,
+	SDSI_CMD_PROVISION_CAP		= 0x08,
+	SDSI_CMD_READ_STATE		= 0x10,
+};
+
+struct sdsi_mbox_info {
+	u64	*payload;
+	u64	*buffer;
+	int	size;
+};
+
+struct disc_table {
+	u32	access_info;
+	u32	guid;
+	u32	offset;
+};
+
+struct sdsi_priv {
+	struct mutex		mb_lock;	/* Mailbox access lock */
+	struct device		*dev;
+	void __iomem		*control_addr;
+	void __iomem		*mbox_addr;
+	void __iomem		*regs_addr;
+	u32			guid;
+	bool			sdsi_enabled;
+};
+
+static struct bin_attribute bin_attr_provision_akc;
+static struct bin_attribute bin_attr_provision_cap;
+
+/* SDSi mailbox operations must be performed using 64bit mov instructions */
+static __always_inline void
+sdsi_memcpy64_toio(u64 __iomem *to, const u64 *from, size_t count_bytes)
+{
+	size_t count = count_bytes / sizeof(*to);
+	int i;
+
+	for (i = 0; i < count; i++)
+		writeq(from[i], &to[i]);
+}
+
+static __always_inline void
+sdsi_memcpy64_fromio(u64 *to, const u64 __iomem *from, size_t count_bytes)
+{
+	size_t count = count_bytes / sizeof(*to);
+	int i;
+
+	for (i = 0; i < count; i++)
+		to[i] = readq(&from[i]);
+}
+
+static inline void sdsi_complete_transaction(struct sdsi_priv *priv)
+{
+	u64 control = FIELD_PREP(CTRL_COMPLETE, 1);
+
+	lockdep_assert_held(&priv->mb_lock);
+	writeq(control, priv->control_addr);
+}
+
+static int sdsi_status_to_errno(u32 status)
+{
+	switch (status) {
+	case SDSI_MBOX_CMD_SUCCESS:
+		return 0;
+	case SDSI_MBOX_CMD_TIMEOUT:
+		return -ETIMEDOUT;
+	default:
+		return -EIO;
+	}
+}
+
+static int sdsi_mbox_cmd_read(struct sdsi_priv *priv, struct sdsi_mbox_info *info, int *data_size)
+{
+	struct device *dev = priv->dev;
+	u32 total, loop, eom, status, message_size;
+	u64 control;
+	int ret;
+
+	lockdep_assert_held(&priv->mb_lock);
+
+	/* Format and send the read command */
+	control = FIELD_PREP(CTRL_EOM, 1) |
+		  FIELD_PREP(CTRL_SOM, 1) |
+		  FIELD_PREP(CTRL_RUN_BUSY, 1) |
+		  FIELD_PREP(CTRL_PACKET_SIZE, info->size);
+	writeq(control, priv->control_addr);
+
+	/* For reads, data sizes that are larger than the mailbox size are read in packets. */
+	total = 0;
+	loop = 0;
+	do {
+		int offset = SDSI_SIZE_MAILBOX * loop;
+		void __iomem *addr = priv->mbox_addr + offset;
+		u64 *buf = info->buffer + offset / SDSI_SIZE_CMD;
+		u32 packet_size;
+
+		/* Poll on ready bit */
+		ret = readq_poll_timeout(priv->control_addr, control, control & CTRL_READY,
+					 MBOX_POLLING_PERIOD_US, MBOX_TIMEOUT_US);
+		if (ret)
+			break;
+
+		eom = FIELD_GET(CTRL_EOM, control);
+		status = FIELD_GET(CTRL_STATUS, control);
+		packet_size = FIELD_GET(CTRL_PACKET_SIZE, control);
+		message_size = FIELD_GET(CTRL_MSG_SIZE, control);
+
+		ret = sdsi_status_to_errno(status);
+		if (ret)
+			break;
+
+		/* Only the last packet can be less than the mailbox size. */
+		if (!eom && packet_size != SDSI_SIZE_MAILBOX) {
+			dev_err(dev, "Invalid packet size\n");
+			ret = -EPROTO;
+			break;
+		}
+
+		if (packet_size > SDSI_SIZE_MAILBOX) {
+			dev_err(dev, "Packet size to large\n");
+			ret = -EPROTO;
+			break;
+		}
+
+		sdsi_memcpy64_fromio(buf, addr, round_up(packet_size, SDSI_SIZE_CMD));
+
+		total += packet_size;
+
+		sdsi_complete_transaction(priv);
+	} while (!eom && ++loop < MBOX_MAX_PACKETS);
+
+	if (ret) {
+		sdsi_complete_transaction(priv);
+		return ret;
+	}
+
+	if (!eom) {
+		dev_err(dev, "Exceeded read attempts\n");
+		return -EPROTO;
+	}
+
+	/* Message size check is only valid for multi-packet transfers */
+	if (loop && total != message_size)
+		dev_warn(dev, "Read count %d differs from expected count %d\n",
+			 total, message_size);
+
+	*data_size = total;
+
+	return 0;
+}
+
+static int sdsi_mbox_cmd_write(struct sdsi_priv *priv, struct sdsi_mbox_info *info)
+{
+	u64 control;
+	u32 status;
+	int ret;
+
+	lockdep_assert_held(&priv->mb_lock);
+
+	/* Write rest of the payload */
+	sdsi_memcpy64_toio(priv->mbox_addr + SDSI_SIZE_CMD, info->payload + 1,
+			   info->size - SDSI_SIZE_CMD);
+
+	/* Format and send the write command */
+	control = FIELD_PREP(CTRL_EOM, 1) |
+		  FIELD_PREP(CTRL_SOM, 1) |
+		  FIELD_PREP(CTRL_RUN_BUSY, 1) |
+		  FIELD_PREP(CTRL_READ_WRITE, 1) |
+		  FIELD_PREP(CTRL_PACKET_SIZE, info->size);
+	writeq(control, priv->control_addr);
+
+	/* Poll on run_busy bit */
+	ret = readq_poll_timeout(priv->control_addr, control, !(control & CTRL_RUN_BUSY),
+				 MBOX_POLLING_PERIOD_US, MBOX_TIMEOUT_US);
+
+	if (ret)
+		goto release_mbox;
+
+	status = FIELD_GET(CTRL_STATUS, control);
+	ret = sdsi_status_to_errno(status);
+
+release_mbox:
+	sdsi_complete_transaction(priv);
+
+	return ret;
+}
+
+static int sdsi_mbox_acquire(struct sdsi_priv *priv, struct sdsi_mbox_info *info)
+{
+	u64 control;
+	u32 owner;
+	int ret;
+
+	lockdep_assert_held(&priv->mb_lock);
+
+	/* Check mailbox is available */
+	control = readq(priv->control_addr);
+	owner = FIELD_GET(CTRL_OWNER, control);
+	if (owner != MBOX_OWNER_NONE)
+		return -EBUSY;
+
+	/* Write first qword of payload */
+	writeq(info->payload[0], priv->mbox_addr);
+
+	/* Check for ownership */
+	ret = readq_poll_timeout(priv->control_addr, control,
+				 FIELD_GET(CTRL_OWNER, control) & MBOX_OWNER_INBAND,
+				 MBOX_POLLING_PERIOD_US, MBOX_TIMEOUT_ACQUIRE_US);
+
+	return ret;
+}
+
+static int sdsi_mbox_write(struct sdsi_priv *priv, struct sdsi_mbox_info *info)
+{
+	int ret;
+
+	lockdep_assert_held(&priv->mb_lock);
+
+	ret = sdsi_mbox_acquire(priv, info);
+	if (ret)
+		return ret;
+
+	return sdsi_mbox_cmd_write(priv, info);
+}
+
+static int sdsi_mbox_read(struct sdsi_priv *priv, struct sdsi_mbox_info *info, int *data_size)
+{
+	int ret;
+
+	lockdep_assert_held(&priv->mb_lock);
+
+	ret = sdsi_mbox_acquire(priv, info);
+	if (ret)
+		return ret;
+
+	return sdsi_mbox_cmd_read(priv, info, data_size);
+}
+
+static ssize_t sdsi_provision(struct sdsi_priv *priv, char *buf, size_t count,
+			      enum sdsi_command command)
+{
+	struct sdsi_mbox_info info;
+	int ret;
+
+	if (!priv->sdsi_enabled)
+		return -EPERM;
+
+	if (count > (SDSI_SIZE_WRITE_MSG - SDSI_SIZE_CMD))
+		return -EOVERFLOW;
+
+	/* Qword aligned message + command qword */
+	info.size = round_up(count, SDSI_SIZE_CMD) + SDSI_SIZE_CMD;
+
+	info.payload = kzalloc(info.size, GFP_KERNEL);
+	if (!info.payload)
+		return -ENOMEM;
+
+	/* Copy message to payload buffer */
+	memcpy(info.payload, buf, count);
+
+	/* Command is last qword of payload buffer */
+	info.payload[(info.size - SDSI_SIZE_CMD) / SDSI_SIZE_CMD] = command;
+
+	ret = mutex_lock_interruptible(&priv->mb_lock);
+	if (ret)
+		goto free_payload;
+	ret = sdsi_mbox_write(priv, &info);
+	mutex_unlock(&priv->mb_lock);
+
+free_payload:
+	kfree(info.payload);
+
+	return (ret < 0) ? ret : count;
+}
+
+static ssize_t provision_akc_write(struct file *filp, struct kobject *kobj,
+				   struct bin_attribute *attr, char *buf, loff_t off,
+				   size_t count)
+{
+	struct device *dev = kobj_to_dev(kobj);
+	struct sdsi_priv *priv = dev_get_drvdata(dev);
+
+	if (off)
+		return -ESPIPE;
+
+	return sdsi_provision(priv, buf, count, SDSI_CMD_PROVISION_AKC);
+}
+static BIN_ATTR_WO(provision_akc, SDSI_SIZE_WRITE_MSG);
+
+static ssize_t provision_cap_write(struct file *filp, struct kobject *kobj,
+				   struct bin_attribute *attr, char *buf, loff_t off,
+				   size_t count)
+{
+	struct device *dev = kobj_to_dev(kobj);
+	struct sdsi_priv *priv = dev_get_drvdata(dev);
+
+	if (off)
+		return -ESPIPE;
+
+	return sdsi_provision(priv, buf, count, SDSI_CMD_PROVISION_CAP);
+}
+static BIN_ATTR_WO(provision_cap, SDSI_SIZE_WRITE_MSG);
+
+static long state_certificate_read(struct file *filp, struct kobject *kobj,
+				   struct bin_attribute *attr, char *buf, loff_t off,
+				   size_t count)
+{
+	struct device *dev = kobj_to_dev(kobj);
+	struct sdsi_priv *priv = dev_get_drvdata(dev);
+	u64 command = SDSI_CMD_READ_STATE;
+	struct sdsi_mbox_info info;
+	u32 size;
+	int ret;
+
+	if (!priv->sdsi_enabled)
+		return -EPERM;
+
+	if (off)
+		return -ESPIPE;
+
+	/* Buffer for return data */
+	info.buffer = kmalloc(SDSI_SIZE_READ_MSG, GFP_KERNEL);
+	if (!info.buffer)
+		return -ENOMEM;
+
+	info.payload = &command;
+	info.size = sizeof(command);
+
+	ret = mutex_lock_interruptible(&priv->mb_lock);
+	if (ret)
+		goto free_buffer;
+	ret = sdsi_mbox_read(priv, &info, &size);
+	mutex_unlock(&priv->mb_lock);
+	if (ret < 0)
+		goto free_buffer;
+
+	if (size > count)
+		size = count;
+	else
+		memset(buf, 0, count);
+
+	memcpy(buf, info.buffer, size);
+
+free_buffer:
+	kfree(info.buffer);
+
+	return (ret < 0) ? ret : count;
+}
+static BIN_ATTR(state_certificate, 0400, state_certificate_read, NULL, SDSI_SIZE_READ_MSG);
+
+static ssize_t registers_read(struct file *filp, struct kobject *kobj,
+			      struct bin_attribute *attr, char *buf, loff_t off,
+			      size_t count)
+{
+	struct device *dev = kobj_to_dev(kobj);
+	struct sdsi_priv *priv = dev_get_drvdata(dev);
+	void __iomem *addr = priv->regs_addr;
+
+	memcpy_fromio(buf, addr + off, count);
+
+	return count;
+}
+static BIN_ATTR(registers, 0400, registers_read, NULL, SDSI_SIZE_REGS);
+
+static struct bin_attribute *sdsi_bin_attrs[] = {
+	&bin_attr_registers,
+	&bin_attr_state_certificate,
+	&bin_attr_provision_akc,
+	&bin_attr_provision_cap,
+	NULL
+};
+
+static ssize_t guid_show(struct device *dev, struct device_attribute *attr, char *buf)
+{
+	struct sdsi_priv *priv = dev_get_drvdata(dev);
+
+	return sysfs_emit(buf, "0x%x\n", priv->guid);
+}
+static DEVICE_ATTR_RO(guid);
+
+static struct attribute *sdsi_attrs[] = {
+	&dev_attr_guid.attr,
+	NULL
+};
+
+static const struct attribute_group sdsi_group = {
+	.attrs = sdsi_attrs,
+	.bin_attrs = sdsi_bin_attrs,
+};
+__ATTRIBUTE_GROUPS(sdsi);
+
+static int sdsi_map_mbox_registers(struct sdsi_priv *priv, struct pci_dev *parent,
+				   struct disc_table *disc_table, struct resource *disc_res)
+{
+	u32 access_type = FIELD_GET(DT_ACCESS_TYPE, disc_table->access_info);
+	u32 size = FIELD_GET(DT_SIZE, disc_table->access_info);
+	u32 tbir = FIELD_GET(DT_TBIR, disc_table->offset);
+	u32 offset = DT_OFFSET(disc_table->offset);
+	u32 features_offset;
+	struct resource res = {};
+
+	/* Starting location of SDSi MMIO region based on access type */
+	switch (access_type) {
+	case ACCESS_TYPE_LOCAL:
+		if (tbir) {
+			dev_err(priv->dev, "Unsupported BAR index %d for access type %d\n",
+				tbir, access_type);
+			return -EINVAL;
+		}
+
+		/*
+		 * For access_type LOCAL, the base address is as follows:
+		 * base address = end of discovery region + base offset + 1
+		 */
+		res.start = disc_res->end + offset + 1;
+		break;
+
+	case ACCESS_TYPE_BARID:
+		res.start = pci_resource_start(parent, tbir) + offset;
+		break;
+
+	default:
+		dev_err(priv->dev, "Unrecognized access_type %d\n", access_type);
+		return -EINVAL;
+	}
+
+	res.end = res.start + size * sizeof(u32) - 1;
+	res.flags = IORESOURCE_MEM;
+
+	priv->control_addr = devm_ioremap_resource(priv->dev, &res);
+	if (IS_ERR(priv->control_addr))
+		return PTR_ERR(priv->control_addr);
+
+	priv->mbox_addr = priv->control_addr + SDSI_SIZE_CONTROL;
+	priv->regs_addr = priv->mbox_addr + SDSI_SIZE_MAILBOX;
+
+	features_offset = readq(priv->regs_addr + SDSI_ENABLED_FEATURES_OFFSET);
+	priv->sdsi_enabled = !!(features_offset & SDSI_ENABLED);
+
+	return 0;
+}
+
+static int sdsi_probe(struct auxiliary_device *auxdev, const struct auxiliary_device_id *id)
+{
+	struct intel_vsec_device *intel_cap_dev = auxdev_to_ivdev(auxdev);
+	struct disc_table disc_table;
+	struct resource *disc_res;
+	void __iomem *disc_addr;
+	struct sdsi_priv *priv;
+	int ret;
+
+	priv = devm_kzalloc(&auxdev->dev, sizeof(*priv), GFP_KERNEL);
+	if (!priv)
+		return -ENOMEM;
+
+	priv->dev = &auxdev->dev;
+	mutex_init(&priv->mb_lock);
+	auxiliary_set_drvdata(auxdev, priv);
+
+	/* Get the SDSi discovery table */
+	disc_res = &intel_cap_dev->resource[0];
+	disc_addr = devm_ioremap_resource(&auxdev->dev, disc_res);
+	if (IS_ERR(disc_addr))
+		return PTR_ERR(disc_addr);
+
+	memcpy_fromio(&disc_table, disc_addr, DISC_TABLE_SIZE);
+
+	priv->guid = disc_table.guid;
+
+	/* Map the SDSi mailbox registers */
+	ret = sdsi_map_mbox_registers(priv, intel_cap_dev->pcidev, &disc_table, disc_res);
+	if (ret)
+		return ret;
+
+	return 0;
+}
+
+static const struct auxiliary_device_id sdsi_aux_id_table[] = {
+	{ .name = "intel_vsec.sdsi" },
+	{}
+};
+MODULE_DEVICE_TABLE(auxiliary, sdsi_aux_id_table);
+
+static struct auxiliary_driver sdsi_aux_driver = {
+	.driver = {
+		.dev_groups = sdsi_groups,
+	},
+	.id_table	= sdsi_aux_id_table,
+	.probe		= sdsi_probe,
+};
+module_auxiliary_driver(sdsi_aux_driver);
+
+MODULE_AUTHOR("David E. Box <david.e.box@linux.intel.com>");
+MODULE_DESCRIPTION("Intel Software Defined Silicon driver");
+MODULE_LICENSE("GPL");
diff --git a/drivers/platform/x86/intel/vsec.c b/drivers/platform/x86/intel/vsec.c
index c3bdd75ed690..bed436bf181f 100644
--- a/drivers/platform/x86/intel/vsec.c
+++ b/drivers/platform/x86/intel/vsec.c
@@ -32,6 +32,7 @@
 #define TABLE_OFFSET_SHIFT		3
 
 static DEFINE_IDA(intel_vsec_ida);
+static DEFINE_IDA(intel_vsec_sdsi_ida);
 
 /**
  * struct intel_vsec_header - Common fields of Intel VSEC and DVSEC registers.
@@ -63,12 +64,14 @@ enum intel_vsec_id {
 	VSEC_ID_TELEMETRY	= 2,
 	VSEC_ID_WATCHER		= 3,
 	VSEC_ID_CRASHLOG	= 4,
+	VSEC_ID_SDSI		= 65,
 };
 
 static enum intel_vsec_id intel_vsec_allow_list[] = {
 	VSEC_ID_TELEMETRY,
 	VSEC_ID_WATCHER,
 	VSEC_ID_CRASHLOG,
+	VSEC_ID_SDSI,
 };
 
 static const char *intel_vsec_name(enum intel_vsec_id id)
@@ -83,6 +86,9 @@ static const char *intel_vsec_name(enum intel_vsec_id id)
 	case VSEC_ID_CRASHLOG:
 		return "crashlog";
 
+	case VSEC_ID_SDSI:
+		return "sdsi";
+
 	default:
 		return NULL;
 	}
@@ -211,7 +217,11 @@ static int intel_vsec_add_dev(struct pci_dev *pdev, struct intel_vsec_header *he
 	intel_vsec_dev->resource = res;
 	intel_vsec_dev->num_resources = header->num_entries;
 	intel_vsec_dev->quirks = quirks;
-	intel_vsec_dev->ida = &intel_vsec_ida;
+
+	if (header->id == VSEC_ID_SDSI)
+		intel_vsec_dev->ida = &intel_vsec_sdsi_ida;
+	else
+		intel_vsec_dev->ida = &intel_vsec_ida;
 
 	return intel_vsec_add_aux(pdev, intel_vsec_dev, intel_vsec_name(header->id));
 }
-- 
2.25.1

Re: [V2 4/6] platform/x86: Add Intel Software Defined Silicon driver

[Leon Romanovsky]

On Tue, Dec 07, 2021 at 09:14:46AM -0800, David E. Box wrote:
> Intel Software Defined Silicon (SDSi) is a post manufacturing mechanism for
> activating additional silicon features. Features are enabled through a
> license activation process.  The SDSi driver provides a per socket, sysfs
> attribute interface for applications to perform 3 main provisioning
> functions:
> 
> 1. Provision an Authentication Key Certificate (AKC), a key written to
>    internal NVRAM that is used to authenticate a capability specific
>    activation payload.
> 
> 2. Provision a Capability Activation Payload (CAP), a token authenticated
>    using the AKC and applied to the CPU configuration to activate a new
>    feature.
> 
> 3. Read the SDSi State Certificate, containing the CPU configuration
>    state.
> 
> The operations perform function specific mailbox commands that forward the
> requests to SDSi hardware to perform authentication of the payloads and
> enable the silicon configuration (to be made available after power
> cycling).
> 
> The SDSi device itself is enumerated as an auxiliary device from the
> intel_vsec driver and as such has a build dependency on CONFIG_INTEL_VSEC.
> 
> Link: https://github.com/intel/intel-sdsi
> Signed-off-by: David E. Box <david.e.box@linux.intel.com>
> Reviewed-by: Mark Gross <markgross@kernel.org>
> ---
> V2
>   - Use sysfs_emit() in guid_show()
>   - Fix language in ABI, suggested by Bjorn
>   - Fix wrong directory name in ABI doc

<...>

> @@ -0,0 +1,77 @@
> +What:		/sys/bus/auxiliary/devices/intel_vsec.sdsi.X

<...>

> +static const struct auxiliary_device_id sdsi_aux_id_table[] = {
> +	{ .name = "intel_vsec.sdsi" },

Are you sure that this sysfs is correct?

Auxiliary bus set device name as a combination of module name plus suffix.

  172 int __auxiliary_device_add(struct auxiliary_device *auxdev, const char *modname)
  173 {
  174         struct device *dev = &auxdev->dev;
  175         int ret;
 ....
  181
  182         ret = dev_set_name(dev, "%s.%s.%d", modname, auxdev->name, auxdev->id);

Thanks

Re: [V2 4/6] platform/x86: Add Intel Software Defined Silicon driver

[David E. Box]

On Wed, 2021-12-08 at 09:14 +0200, Leon Romanovsky wrote:
> On Tue, Dec 07, 2021 at 09:14:46AM -0800, David E. Box wrote:
> > Intel Software Defined Silicon (SDSi) is a post manufacturing mechanism for
> > activating additional silicon features. Features are enabled through a
> > license activation process.  The SDSi driver provides a per socket, sysfs
> > attribute interface for applications to perform 3 main provisioning
> > functions:
> > 
> > 1. Provision an Authentication Key Certificate (AKC), a key written to
> >    internal NVRAM that is used to authenticate a capability specific
> >    activation payload.
> > 
> > 2. Provision a Capability Activation Payload (CAP), a token authenticated
> >    using the AKC and applied to the CPU configuration to activate a new
> >    feature.
> > 
> > 3. Read the SDSi State Certificate, containing the CPU configuration
> >    state.
> > 
> > The operations perform function specific mailbox commands that forward the
> > requests to SDSi hardware to perform authentication of the payloads and
> > enable the silicon configuration (to be made available after power
> > cycling).
> > 
> > The SDSi device itself is enumerated as an auxiliary device from the
> > intel_vsec driver and as such has a build dependency on CONFIG_INTEL_VSEC.
> > 
> > Link: https://github.com/intel/intel-sdsi
> > Signed-off-by: David E. Box <david.e.box@linux.intel.com>
> > Reviewed-by: Mark Gross <markgross@kernel.org>
> > ---
> > V2
> >   - Use sysfs_emit() in guid_show()
> >   - Fix language in ABI, suggested by Bjorn
> >   - Fix wrong directory name in ABI doc
> 
> <...>
> 
> > @@ -0,0 +1,77 @@
> > +What:		/sys/bus/auxiliary/devices/intel_vsec.sdsi.X
> 
> <...>
> 
> > +static const struct auxiliary_device_id sdsi_aux_id_table[] = {
> > +	{ .name = "intel_vsec.sdsi" },
> 
> Are you sure that this sysfs is correct?
> 
> Auxiliary bus set device name as a combination of module name plus suffix.
> 
>   172 int __auxiliary_device_add(struct auxiliary_device *auxdev, const char
> *modname)
>   173 {
>   174         struct device *dev = &auxdev->dev;
>   175         int ret;
>  ....
>   181
>   182         ret = dev_set_name(dev, "%s.%s.%d", modname, auxdev->name,
> auxdev->id);
> 
> Thanks

Yes. 'intel_vsec' is the module name, 'sdsi' is the suffix, and 'X' is meant to
indicate the unique id. Will change to '*' instead of 'X'.

Thanks

David

Re: [V2 4/6] platform/x86: Add Intel Software Defined Silicon driver

[Leon Romanovsky]

On Wed, Dec 08, 2021 at 02:42:42AM -0800, David E. Box wrote:
> On Wed, 2021-12-08 at 09:14 +0200, Leon Romanovsky wrote:
> > On Tue, Dec 07, 2021 at 09:14:46AM -0800, David E. Box wrote:
> > > Intel Software Defined Silicon (SDSi) is a post manufacturing mechanism for
> > > activating additional silicon features. Features are enabled through a
> > > license activation process.  The SDSi driver provides a per socket, sysfs
> > > attribute interface for applications to perform 3 main provisioning
> > > functions:
> > > 
> > > 1. Provision an Authentication Key Certificate (AKC), a key written to
> > >    internal NVRAM that is used to authenticate a capability specific
> > >    activation payload.
> > > 
> > > 2. Provision a Capability Activation Payload (CAP), a token authenticated
> > >    using the AKC and applied to the CPU configuration to activate a new
> > >    feature.
> > > 
> > > 3. Read the SDSi State Certificate, containing the CPU configuration
> > >    state.
> > > 
> > > The operations perform function specific mailbox commands that forward the
> > > requests to SDSi hardware to perform authentication of the payloads and
> > > enable the silicon configuration (to be made available after power
> > > cycling).
> > > 
> > > The SDSi device itself is enumerated as an auxiliary device from the
> > > intel_vsec driver and as such has a build dependency on CONFIG_INTEL_VSEC.
> > > 
> > > Link: https://github.com/intel/intel-sdsi
> > > Signed-off-by: David E. Box <david.e.box@linux.intel.com>
> > > Reviewed-by: Mark Gross <markgross@kernel.org>
> > > ---
> > > V2
> > >   - Use sysfs_emit() in guid_show()
> > >   - Fix language in ABI, suggested by Bjorn
> > >   - Fix wrong directory name in ABI doc
> > 
> > <...>
> > 
> > > @@ -0,0 +1,77 @@
> > > +What:		/sys/bus/auxiliary/devices/intel_vsec.sdsi.X
> > 
> > <...>
> > 
> > > +static const struct auxiliary_device_id sdsi_aux_id_table[] = {
> > > +	{ .name = "intel_vsec.sdsi" },
> > 
> > Are you sure that this sysfs is correct?
> > 
> > Auxiliary bus set device name as a combination of module name plus suffix.
> > 
> >   172 int __auxiliary_device_add(struct auxiliary_device *auxdev, const char
> > *modname)
> >   173 {
> >   174         struct device *dev = &auxdev->dev;
> >   175         int ret;
> >  ....
> >   181
> >   182         ret = dev_set_name(dev, "%s.%s.%d", modname, auxdev->name,
> > auxdev->id);
> > 
> > Thanks
> 
> Yes. 'intel_vsec' is the module name, 'sdsi' is the suffix, and 'X' is meant to
> indicate the unique id. Will change to '*' instead of 'X'.

No, it is ok, I don't think that it is worth to change.

Thanks

> 
> Thanks
> 
> David
> 

[V2 5/6] sample/sdsi: Sample of SDSi provisiong using sysfs

[David E. Box]

Sample application showing usage of Intel Software Defined Silicon
sysfs ABI.

Signed-off-by: David E. Box <david.e.box@linux.intel.com>
---
V2
  - New patch

 MAINTAINERS                |   1 +
 samples/sdsi/Makefile      |   9 +
 samples/sdsi/sdsi-sample.c | 399 +++++++++++++++++++++++++++++++++++++
 3 files changed, 409 insertions(+)
 create mode 100644 samples/sdsi/Makefile
 create mode 100644 samples/sdsi/sdsi-sample.c

diff --git a/MAINTAINERS b/MAINTAINERS
index af7f17e7400f..ba9603fb7f62 100644
--- a/MAINTAINERS
+++ b/MAINTAINERS
@@ -9787,6 +9787,7 @@ INTEL SDSI DRIVER
 M:	David E. Box <david.e.box@linux.intel.com>
 S:	Supported
 F:	drivers/platform/x86/intel/sdsi.c
+F:	samples/sdsi/
 
 INTEL SKYLAKE INT3472 ACPI DEVICE DRIVER
 M:	Daniel Scally <djrscally@gmail.com>
diff --git a/samples/sdsi/Makefile b/samples/sdsi/Makefile
new file mode 100644
index 000000000000..17ac82a5623d
--- /dev/null
+++ b/samples/sdsi/Makefile
@@ -0,0 +1,9 @@
+# SPDX-License-Identifier: GPL-2.0
+
+.PHONY: sdsi-sample
+
+sdsi-sample: sdsi-sample.o
+	$(CC) -Wall $^ -o $@
+
+clean:
+	rm *.o sdsi-sample
diff --git a/samples/sdsi/sdsi-sample.c b/samples/sdsi/sdsi-sample.c
new file mode 100644
index 000000000000..6b3b48359aa0
--- /dev/null
+++ b/samples/sdsi/sdsi-sample.c
@@ -0,0 +1,399 @@
+// SPDX-License-Identifier: GPL-2.0
+/*
+ * sdsi_test: Example program using the sysfs interface of the
+ * Intel Software Defined Silicon Linux driver.
+ *
+ * See https://github.com/intel/intel-sdsi/blob/master/os-interface.rst
+ * for register descriptions.
+ *
+ * Copyright (C) 2021 Intel Corporation. All rights reserved.
+ */
+
+#include <dirent.h>
+#include <errno.h>
+#include <fcntl.h>
+#include <stdbool.h>
+#include <stdio.h>
+#include <stdint.h>
+#include <stdlib.h>
+#include <string.h>
+#include <unistd.h>
+
+#define SDSI_DIR		"/sys/bus/auxiliary/devices/intel_vsec.sdsi"
+#define GUID			"0x6dd191"
+#define REGISTERS_MIN_SIZE	72
+
+struct enabled_features {
+	uint64_t reserved:3;
+	uint64_t sdsi:1;
+	uint64_t reserved1:60;
+};
+
+struct auth_fail_count {
+	uint64_t key_failure_count:3;
+	uint64_t key_failure_threshold:3;
+	uint64_t auth_failure_count:3;
+	uint64_t auth_failure_threshold:3;
+	uint64_t reserved:52;
+};
+
+struct availability {
+	uint64_t reserved:58;
+	uint64_t updates_available:3;
+	uint64_t updates_threshold:3;
+};
+
+struct sdsi_reg_6dd191 {
+	uint64_t ppin;
+	uint64_t reserved;
+	struct enabled_features en_features;
+	uint64_t reserved1;
+	struct auth_fail_count auth_fail_count;
+	struct availability prov_avail;
+	uint64_t reserved2;
+	uint64_t reserved3;
+	uint64_t socket_id;
+};
+
+enum command {
+	CMD_NONE,
+	CMD_READ_LIC,
+	CMD_READ_REG,
+	CMD_PROV_AKC,
+	CMD_PROV_CAP,
+};
+
+static int get_file_size(FILE *stream, char *name)
+{
+	long size;
+	int ret;
+
+	ret = fseek(stream, 0L, SEEK_END);
+	if (ret == -1) {
+		fprintf(stderr, "...Could not seek to EOF %s: %s\n", name, strerror(errno));
+		return ret;
+	}
+
+	size = ftell(stream);
+	if (size == -1) {
+		fprintf(stderr, "...Could not get size of file %s: %s\n", name, strerror(errno));
+		return size;
+	}
+
+	rewind(stream);
+
+	return size;
+}
+
+static int sdsi_read_reg(char *socket)
+{
+	FILE *regs_ptr, *guid_ptr;
+	struct sdsi_reg_6dd191 registers;
+	char guid_val[20], *buf;
+	char regs_file[70];
+	char guid_file[70];
+	int ret, i;
+	long size;
+
+	snprintf(regs_file, sizeof(regs_file), "%s%s%s%s",
+		 SDSI_DIR, ".", socket, "/registers");
+
+	snprintf(guid_file, sizeof(guid_file), "%s%s%s%s",
+		 SDSI_DIR, ".", socket, "/guid");
+
+	memset(&registers, 0, sizeof(registers));
+
+	/* Open the guid file */
+	guid_ptr = fopen(guid_file, "r");
+	if (!guid_ptr) {
+		fprintf(stderr, "...Could not open file %s: %s\n", guid_file, strerror(errno));
+		return -1;
+	}
+
+	fscanf(guid_ptr, "%20s", guid_val);
+	fclose(guid_ptr);
+
+	/* Open the registers file */
+	regs_ptr = fopen(regs_file, "r");
+	if (!regs_ptr) {
+		fprintf(stderr, "...Could not open file %s: %s\n", regs_file, strerror(errno));
+		return -1;
+	}
+
+	/* Get size of the registers file */
+	size = get_file_size(regs_ptr, regs_file);
+	if (size < 0) {
+		ret = size;
+		goto close_regs_ptr;
+	}
+
+	/* Unknown guid. Just dump raw data */
+	if (strcmp(GUID, guid_val)) {
+		printf("Unrecognized guid, %s\n", guid_val);
+
+		buf = (char *)malloc(sizeof(char) * size);
+		if (!buf) {
+			perror("malloc");
+			goto close_regs_ptr;
+		}
+
+		ret = fread(buf, sizeof(uint8_t), size, regs_ptr);
+		if (!ret) {
+			fprintf(stderr, "...Could not read file %s: %s\n", regs_file,
+				strerror(errno));
+			free(buf);
+			goto close_regs_ptr;
+		}
+
+		for (i = 0; i < size; i += sizeof(uint64_t))
+			printf("%3d: 0x%lx\n", i, *(uint64_t *)&buf[i]);
+
+		free(buf);
+		goto close_regs_ptr;
+	}
+
+	/* Print register info for this guid */
+	ret = fread(&registers, sizeof(uint8_t), sizeof(registers), regs_ptr);
+	if (!ret) {
+		fprintf(stderr, "...Could not read file %s: %s\n", regs_file, strerror(errno));
+		goto close_regs_ptr;
+	}
+
+	printf("\n");
+	printf("Info for device %s.%s\n", "intel_vsec.sdsi", socket);
+	printf("\n");
+	printf("PPIN:                           0x%lx\n", registers.ppin);
+	printf("Enabled Features\n");
+	printf("    SDSi:                       %s\n", !!registers.en_features.sdsi ? "Enabled" : "Disabled");
+	printf("Authorization Failure Count\n");
+	printf("    Key Failure Count:          %d\n", registers.auth_fail_count.key_failure_count);
+	printf("    Key Failure Count:          %d\n", registers.auth_fail_count.key_failure_threshold);
+	printf("    Auth Failure Count:         %d\n", registers.auth_fail_count.auth_failure_count);
+	printf("    Auth Failure Count:         %d\n", registers.auth_fail_count.key_failure_threshold);
+	printf("Provisioning Availability\n");
+	printf("    Updates Available:          %d\n", registers.prov_avail.updates_available);
+	printf("    Updates Threshold:          %d\n", registers.prov_avail.updates_threshold);
+	printf("Socket ID:                      0x%lx\n", registers.socket_id);
+
+close_regs_ptr:
+	fclose(regs_ptr);
+
+	return 0;
+}
+
+static int sdsi_certificate_dump(char *socket)
+{
+	uint64_t state_certificate[512] = {0};
+	bool first_instance;
+	char cert_file[70];
+	uint64_t previous;
+	FILE *cert_ptr;
+	int i, ret;
+
+	snprintf(cert_file, sizeof(cert_file), "%s%s%s%s",
+		 SDSI_DIR, ".", socket, "/state_certificate");
+
+	/* Open the registers file */
+	cert_ptr = fopen(cert_file, "r");
+	if (!cert_ptr) {
+		fprintf(stderr, "...Could not open file %s: %s\n", cert_file, strerror(errno));
+		return -1;
+	}
+
+	/* Read registers */
+	ret = fread(state_certificate, sizeof(uint8_t), sizeof(state_certificate), cert_ptr);
+	if (!ret) {
+		fprintf(stderr, "...Could not read file %s: %s\n", cert_file, strerror(errno));
+		goto close_cert_ptr;
+	}
+
+	printf("%3d: 0x%lx\n", 0, state_certificate[0]);
+	previous = state_certificate[0];
+	first_instance = true;
+
+	for (i = 1; i < (sizeof(state_certificate)/sizeof(uint64_t)); i++) {
+		if (state_certificate[i] == previous) {
+			if (first_instance) {
+				puts("*");
+				first_instance = false;
+			}
+			continue;
+		}
+		printf("%3d: 0x%lx\n", i, state_certificate[i]);
+		previous = state_certificate[i];
+		first_instance = true;
+	}
+	printf("%3d\n", i);
+
+close_cert_ptr:
+	fclose(cert_ptr);
+
+	return 0;
+}
+
+static int sdsi_provision(char *prov_file, char *bin_file)
+{
+	char buf[4096] = { 0 };
+	int bin_fd, prov_fd, size, ret = 0;
+
+	if (!bin_file) {
+		fprintf(stderr, "...No binary file provided\n");
+		return -1;
+	}
+
+	/* Open the provision file */
+	prov_fd = open(prov_file, O_WRONLY);
+	if (prov_fd == -1) {
+		fprintf(stderr, "...Could not open file %s: %s\n", prov_file, strerror(errno));
+		return prov_fd;
+	}
+
+	/* Open the binary */
+	bin_fd = open(bin_file, O_RDONLY);
+	if (bin_fd == -1) {
+		fprintf(stderr, "...Could not open file %s: %s\n", bin_file, strerror(errno));
+		ret = bin_fd;
+		goto close_provision_fd;
+	}
+
+	/* Read the binary file into the buffer */
+	ret = read(bin_fd, buf, 4096);
+	if (ret == -1)
+		goto close_bin_fd;
+
+	size = ret;
+	ret = write(prov_fd, buf, size);
+	if (ret < size) {
+		fprintf(stderr, "...Could not write file %s: %s\n", prov_file, strerror(errno));
+		goto close_bin_fd;
+	}
+
+	printf("Provisioned %s file %s successfully\n", prov_file, bin_file);
+
+close_bin_fd:
+	close(bin_fd);
+close_provision_fd:
+	close(prov_fd);
+
+	return ret;
+}
+
+static int sdsi_provision_akc(char *socket, char *bin_file)
+{
+	char akc_file[70];
+
+	snprintf(akc_file, sizeof(akc_file), "%s%s%s%s",
+		 SDSI_DIR, ".", socket, "/provision_akc");
+
+	return sdsi_provision(akc_file, bin_file);
+}
+
+static int sdsi_provision_cap(char *socket, char *bin_file)
+{
+	char cap_file[70];
+
+	snprintf(cap_file, sizeof(cap_file), "%s%s%s%s",
+		 SDSI_DIR, ".", socket, "/provision_cap");
+
+	return sdsi_provision(cap_file, bin_file);
+}
+
+static void print_help(char *prog)
+{
+	printf("Usage:\n");
+
+	printf("\t%s -s socket [-r [lic] [reg]] [-a file] [-c file]\n", prog);
+
+	printf("Options:\n");
+	printf("%-13s\t%s\n", "-s <socket>", "socket number to open");
+	printf("%-13s\t%s\n", "-r lic", "read licence data");
+	printf("%-13s\t%s\n", "-r reg", "read SDSi register data");
+	printf("%-13s\t%s\n", "-a <file>", "provision socket with AKC file");
+	printf("%-13s\t%s\n", "-c <file>", "provision socket with CAP file");
+}
+
+int main(int argc, char *argv[])
+{
+	char *bin_file = NULL, *socket = NULL;
+	enum command command = CMD_NONE;
+	int ret, opt, cmd_count = 0;
+
+	while ((opt = getopt(argc, argv, "hs:ra:c:t:")) != -1) {
+		switch (opt) {
+		case 's':
+			socket = optarg;
+			break;
+		case 'r':
+			if (!argv[optind]) {
+				print_help(argv[0]);
+				return -1;
+			}
+
+			if (strlen(argv[optind]) != strlen("lic")) {
+				print_help(argv[0]);
+				return -1;
+			}
+
+			if (!strcmp(argv[optind], "lic")) {
+				command = CMD_READ_LIC;
+				++cmd_count;
+				break;
+			} else if (!strcmp(argv[optind], "reg")) {
+				command = CMD_READ_REG;
+				++cmd_count;
+				break;
+			}
+
+			print_help(argv[0]);
+			return -1;
+		case 'a':
+			command = CMD_PROV_AKC;
+			bin_file = optarg;
+			++cmd_count;
+			break;
+		case 'c':
+			command = CMD_PROV_CAP;
+			bin_file = optarg;
+			++cmd_count;
+			break;
+		case 'h':
+			print_help(argv[0]);
+			break;
+		default:
+			print_help(argv[0]);
+			return 0;
+		}
+	}
+
+	if (!socket) {
+		fprintf(stderr, "socket is required\n");
+		print_help(argv[0]);
+		return -1;
+	}
+
+	if (!cmd_count) {
+		fprintf(stderr, "need to specify a command\n");
+		print_help(argv[0]);
+		return -1;
+	}
+
+	/* If applicable, check file exists */
+	if (bin_file) {
+		if (!access(bin_file, F_OK) == 0) {
+			fprintf(stderr, "...Could not open file %s: %s\n", bin_file, strerror(errno));
+			return -1;
+		}
+	}
+
+	/* Run the command */
+	if (command == CMD_READ_LIC)
+		ret = sdsi_certificate_dump(socket);
+	else if (command == CMD_READ_REG)
+		ret = sdsi_read_reg(socket);
+	else if (command == CMD_PROV_AKC)
+		ret = sdsi_provision_akc(socket, bin_file);
+	else
+		ret = sdsi_provision_cap(socket, bin_file);
+
+	return ret;
+}
-- 
2.25.1

[V2 6/6] selftests: sdsi: test sysfs setup

[David E. Box]

Tests file configuration and error handling of the Intel Software
Defined Silicon sysfs ABI.

Signed-off-by: David E. Box <david.e.box@linux.intel.com>
---
V2
  - New patch

 MAINTAINERS                                   |   1 +
 tools/testing/selftests/drivers/sdsi/sdsi.sh  |  18 ++
 .../selftests/drivers/sdsi/sdsi_test.py       | 166 ++++++++++++++++++
 3 files changed, 185 insertions(+)
 create mode 100755 tools/testing/selftests/drivers/sdsi/sdsi.sh
 create mode 100644 tools/testing/selftests/drivers/sdsi/sdsi_test.py

diff --git a/MAINTAINERS b/MAINTAINERS
index ba9603fb7f62..3bc2a7bff168 100644
--- a/MAINTAINERS
+++ b/MAINTAINERS
@@ -9788,6 +9788,7 @@ M:	David E. Box <david.e.box@linux.intel.com>
 S:	Supported
 F:	drivers/platform/x86/intel/sdsi.c
 F:	samples/sdsi/
+F:	tools/testing/selftests/drivers/sdsi/
 
 INTEL SKYLAKE INT3472 ACPI DEVICE DRIVER
 M:	Daniel Scally <djrscally@gmail.com>
diff --git a/tools/testing/selftests/drivers/sdsi/sdsi.sh b/tools/testing/selftests/drivers/sdsi/sdsi.sh
new file mode 100755
index 000000000000..8db71961d164
--- /dev/null
+++ b/tools/testing/selftests/drivers/sdsi/sdsi.sh
@@ -0,0 +1,18 @@
+#!/bin/sh
+# SPDX-License-Identifier: GPL-2.0
+# Runs tests for the intel_sdsi driver
+
+if ! /sbin/modprobe -q -r intel_sdsi; then
+	echo "drivers/sdsi: [SKIP]"
+	exit 77
+fi
+
+if /sbin/modprobe -q intel_sdsi; then
+	python3 -m pytest sdsi_test.py
+	/sbin/modprobe -q -r intel_sdsi
+
+	echo "drivers/sdsi: ok"
+else
+	echo "drivers/sdsi: [FAIL]"
+	exit 1
+fi
diff --git a/tools/testing/selftests/drivers/sdsi/sdsi_test.py b/tools/testing/selftests/drivers/sdsi/sdsi_test.py
new file mode 100644
index 000000000000..fc99d9c23f51
--- /dev/null
+++ b/tools/testing/selftests/drivers/sdsi/sdsi_test.py
@@ -0,0 +1,166 @@
+#!/usr/bin/env python3
+# SPDX-License-Identifier: GPL-2.0
+
+from struct import pack
+from time import sleep
+
+import errno
+import glob
+import os
+import subprocess
+
+try:
+    import pytest
+except ImportError:
+    print("Unable to import pytest python module.")
+    print("\nIf not already installed, you may do so with:")
+    print("\t\tpip3 install pytest")
+    exit(1)
+
+SOCKETS = glob.glob('/sys/bus/auxiliary/devices/intel_vsec.sdsi.*')
+NUM_SOCKETS = len(SOCKETS)
+
+MODULE_NAME = 'sdsi'
+DEV_PREFIX = 'intel_vsec.sdsi'
+CLASS_DIR = '/sys/bus/auxiliary/devices'
+GUID = "0x6dd191"
+
+def read_bin_file(file):
+    with open(file, mode='rb') as f:
+        content = f.read()
+    return content
+
+def get_dev_file_path(socket, file):
+    return CLASS_DIR + '/' + DEV_PREFIX + '.' + str(socket) + '/' + file
+
+class TestSDSiDriver:
+    def test_driver_loaded(self):
+        lsmod_p = subprocess.Popen(('lsmod'), stdout=subprocess.PIPE)
+        result = subprocess.check_output(('grep', '-q', MODULE_NAME), stdin=lsmod_p.stdout)
+
+@pytest.mark.parametrize('socket', range(0, NUM_SOCKETS))
+class TestSDSiFilesClass:
+
+    def read_value(self, file):
+        f = open(file, "r")
+        value = f.read().strip("\n")
+        return value
+
+    def get_dev_folder(self, socket):
+        return CLASS_DIR + '/' + DEV_PREFIX + '.' + str(socket) + '/'
+
+    def test_sysfs_files_exist(self, socket):
+        folder = self.get_dev_folder(socket)
+        print (folder)
+        assert os.path.isfile(folder + "guid") == True
+        assert os.path.isfile(folder + "provision_akc") == True
+        assert os.path.isfile(folder + "provision_cap") == True
+        assert os.path.isfile(folder + "state_certificate") == True
+        assert os.path.isfile(folder + "registers") == True
+
+    def test_sysfs_file_permissions(self, socket):
+        folder = self.get_dev_folder(socket)
+        mode = os.stat(folder + "guid").st_mode & 0o777
+        assert mode == 0o444    # Read all
+        mode = os.stat(folder + "registers").st_mode & 0o777
+        assert mode == 0o400    # Read owner
+        mode = os.stat(folder + "provision_akc").st_mode & 0o777
+        assert mode == 0o200    # Read owner
+        mode = os.stat(folder + "provision_cap").st_mode & 0o777
+        assert mode == 0o200    # Read owner
+        mode = os.stat(folder + "state_certificate").st_mode & 0o777
+        assert mode == 0o400    # Read owner
+
+    def test_sysfs_file_ownership(self, socket):
+        folder = self.get_dev_folder(socket)
+
+        st = os.stat(folder + "guid")
+        assert st.st_uid == 0
+        assert st.st_gid == 0
+
+        st = os.stat(folder + "registers")
+        assert st.st_uid == 0
+        assert st.st_gid == 0
+
+        st = os.stat(folder + "provision_akc")
+        assert st.st_uid == 0
+        assert st.st_gid == 0
+
+        st = os.stat(folder + "provision_cap")
+        assert st.st_uid == 0
+        assert st.st_gid == 0
+
+        st = os.stat(folder + "state_certificate")
+        assert st.st_uid == 0
+        assert st.st_gid == 0
+
+    def test_sysfs_file_sizes(self, socket):
+        folder = self.get_dev_folder(socket)
+
+        if self.read_value(folder + "guid") == GUID:
+            st = os.stat(folder + "registers")
+            assert st.st_size == 72
+
+        st = os.stat(folder + "provision_akc")
+        assert st.st_size == 1024
+
+        st = os.stat(folder + "provision_cap")
+        assert st.st_size == 1024
+
+        st = os.stat(folder + "state_certificate")
+        assert st.st_size == 4096
+
+    def test_no_seek_allowed(self, socket):
+        folder = self.get_dev_folder(socket)
+        rand_file = bytes(os.urandom(8))
+
+        f = open(folder + "state_certificate", "rb")
+        f.seek(1)
+        with pytest.raises(OSError) as error:
+            f.read()
+        assert error.value.errno == errno.ESPIPE
+        f.close()
+
+        f = open(folder + "provision_cap", "wb", 0)
+        f.seek(1)
+        with pytest.raises(OSError) as error:
+            f.write(rand_file)
+        assert error.value.errno == errno.ESPIPE
+        f.close()
+
+        f = open(folder + "provision_akc", "wb", 0)
+        f.seek(1)
+        with pytest.raises(OSError) as error:
+            f.write(rand_file)
+        assert error.value.errno == errno.ESPIPE
+        f.close()
+
+    def test_registers_seek(self, socket):
+        folder = self.get_dev_folder(socket)
+
+        # Check that the value read from an offset of the entire
+        # file is none-zero and the same as the value read
+        # from seeking to the same location
+        f = open(folder + "registers", "rb")
+        data = f.read()
+        f.seek(64)
+        id = f.read()
+        assert id != bytes(0)
+        assert data[64:] == id
+        f.close()
+
+@pytest.mark.parametrize('socket', range(0, NUM_SOCKETS))
+class TestSDSiMailboxCmdsClass:
+    def test_provision_akc_eoverflow_1017_bytes(self, socket):
+
+        # The buffer for writes is 1k, of with 8 bytes must be
+        # reserved for the command, leaving 1016 bytes max.
+        # Check that we get an overflow error for 1017 bytes.
+        node = get_dev_file_path(socket, "provision_akc")
+        rand_file = bytes(os.urandom(1017))
+
+        f = open(node, 'wb', 0)
+        with pytest.raises(OSError) as error:
+            f.write(rand_file)
+        assert error.value.errno == errno.EOVERFLOW
+        f.close()
-- 
2.25.1