Documentation: Update ioctl-number.rst for Intel Software Defined Silicon interface
Subsystems (3)
David E. Box
maintainerPATCH 1/2 Documentation: Update ioctl-number.rst for Intel Software Defined Silicon interface
Reserve ioctl number and range for the Intel Software Defined Silicon driver.
--- Documentation/userspace-api/ioctl/ioctl-number.rst | 1 + 1 file changed, 1 insertion(+)
Documentation/userspace-api/ioctl/ioctl-number.rst +1 -0
@@ -363,6 +363,7 @@ Code Seq# Include File Comments 0xDB 00-0F drivers/char/mwave/mwavepub.h 0xDD 00-3F ZFCP device driver see drivers/s390/scsi/ <mailto:[email protected]> +0xDF all linux/isdsi_if.h 0xE5 00-3F linux/fuse.h 0xEC 00-01 drivers/platform/chrome/cros_ec_dev.h ChromeOS EC driver 0xF3 00-3F drivers/usb/misc/sisusbvga/sisusb.h sisfb (in development)
Signature · 1 line
2.25.1
David E. Box
maintainerPATCH 2/2 platform/x86: Add Intel Software Defined Silicon driver
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, ioctl 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 ioctl 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 a PCIe VSEC capability on the Intel Out Of Band Management Services Module (OOBMSM) device. The SDSi device is a cell of the intel_pmt MFD driver and as such has a build dependency on CONFIG_MFD_INTEL_PMT.
--- MAINTAINERS | 6 + drivers/platform/x86/intel/Kconfig | 12 + drivers/platform/x86/intel/Makefile | 2 + drivers/platform/x86/intel/sdsi.c | 676 ++++++++++++++++++++++++++++ include/uapi/linux/sdsi_if.h | 47 ++ 5 files changed, 743 insertions(+) create mode 100644 drivers/platform/x86/intel/sdsi.c create mode 100644 include/uapi/linux/sdsi_if.h
MAINTAINERS +6 -0
@@ -9610,6 +9610,12 @@ S: Maintained F: arch/x86/include/asm/intel_scu_ipc.h F: drivers/platform/x86/intel_scu_* +INTEL SDSI DRIVER +M: David E. Box <[email protected]> +S: Supported +F: drivers/platform/x86/intel/sdsi.c +F: include/uapi/linux/sdsi_if.h + INTEL SKYLAKE INT3472 ACPI DEVICE DRIVER M: Daniel Scally <[email protected]> S: Maintained
drivers/platform/x86/intel/Kconfig +12 -0
@@ -131,6 +131,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 X86_64 + depends on MFD_INTEL_PMT + help + This driver enables access to the Intel Software Defined Silicon + interface used to provision silicon features with an authentication + certificate and capability activation payload. + + 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
drivers/platform/x86/intel/Makefile +2 -0
@@ -25,6 +25,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 PMIC / PMC / P-Unit drivers intel_bxtwc_tmu-y := bxtwc_tmu.o
drivers/platform/x86/intel/sdsi.c new file +676 -0
676 lines not on this page · raw message
include/uapi/linux/sdsi_if.h new file +47 -0
47 lines not on this page · raw message
Signature · 1 line
2.25.1
Re: [PATCH 2/2] platform/x86: Add Intel Software Defined Silicon driver
On Fri, Sep 24, 2021 at 02:31:57PM -0700, David E. Box wrote:
Quick review:
Quoted 22 lines
drivers/platform/x86/intel/sdsi.c new file +22
-0+static int sdsi_probe (struct platform_device *pdev) +{ +void __iomem *disc_addr; +struct sdsi_priv *priv; +int ret; + + disc_addr =devm_platform_ioremap_resource (pdev,0 ); +if (IS_ERR (disc_addr)) +return PTR_ERR (disc_addr); + + priv =kzalloc (sizeof (*priv),GFP_KERNEL ); +if (!priv) +return -ENOMEM ; + +kref_init (&priv->kref); + +platform_set_drvdata (pdev, priv); + priv->pdev = pdev; +mutex_init (&priv->mb_lock); +mutex_init (&priv->akc_lock); + +memcpy_fromio (&priv->disc_table, disc_addr,DISC_TABLE_SIZE );
drivers/platform/x86/intel/sdsi.c line 635
Quoted 10 lines
drivers/platform/x86/intel/sdsi.c new file +10
-0+ + ret =sdsi_map_sdsi_registers (pdev); +if (ret) + goto put_kref; + + ret =sdsi_create_misc_device (pdev); +if (ret) + goto put_kref; + + ret =sdsi_add_bin_attrs (pdev);
You just raced with userspace and lost. Please attach your attributes to the misc device before registering it.
Also, you need a Documentation/ABI/ entry for your new sysfs file(s).
Quoted 13 lines
drivers/platform/x86/intel/sdsi.c new file +13
-0+if (ret) + goto deregister_misc; + + priv->dev_present = true; + +return 0 ; + +deregister_misc: +misc_deregister (&priv->miscdev); +put_kref: +kref_put (&priv->kref, sdsi_priv_release); + +return ret;
drivers/platform/x86/intel/sdsi.c line 658
Quoted 10 lines
drivers/platform/x86/intel/sdsi.c new file +10
-0+} + +static int sdsi_remove (struct platform_device *pdev) +{ +struct sdsi_priv *priv =platform_get_drvdata (pdev); + + priv->dev_present = false; +sysfs_remove_bin_file (&priv->pdev->dev.kobj, &priv->registers_bin_attr); +misc_deregister (&priv->miscdev); +kref_put (&priv->kref, sdsi_priv_release);
Why do you need a kref for a structure that already can be controlled by a different lifetime rule?
drivers/platform/x86/intel/sdsi.c line 671
Quoted 13 lines
drivers/platform/x86/intel/sdsi.c new file +13
-0+ +return 0 ; +} + +static struct platform_driver sdsi_driver = { + .driver = { + .name =SDSI_DEV_NAME , + .dev_groups = sdsi_groups, + }, + .probe = sdsi_probe, + .remove = sdsi_remove, +}; +module_platform_driver (sdsi_driver);
What causes the platform to know to register, and enable, this platform driver? Shouldn't there be some hardware involved that is discoverable to enable it to load dynamically?
thanks,
greg k-h
David E. Box
maintainerRe: [PATCH 2/2] platform/x86: Add Intel Software Defined Silicon driver
Quoted 40 lines · Greg KH
On Fri, Sep 24, 2021 at 02:31:57PM -0700, David E. Box wrote:
Quick review:
Quoted 22 lines
drivers/platform/x86/intel/sdsi.c new file +22
-0+static int sdsi_probe (struct platform_device *pdev) +{ + void __iomem *disc_addr; + struct sdsi_priv *priv; + int ret; + + disc_addr = devm_platform_ioremap_resource(pdev, 0); + if (IS_ERR(disc_addr)) + return PTR_ERR(disc_addr); + + priv = kzalloc(sizeof(*priv), GFP_KERNEL); + if (!priv) + return -ENOMEM; + + kref_init(&priv->kref); + + platform_set_drvdata(pdev, priv); + priv->pdev = pdev; + mutex_init(&priv->mb_lock); + mutex_init(&priv->akc_lock); + + memcpy_fromio(&priv->disc_table, disc_addr, DISC_TABLE_SIZE);drivers/platform/x86/intel/sdsi.c line 635
Quoted 10 lines
drivers/platform/x86/intel/sdsi.c new file +10
-0+ + ret = sdsi_map_sdsi_registers(pdev); + if (ret) + goto put_kref; + + ret = sdsi_create_misc_device(pdev); + if (ret) + goto put_kref; + + ret = sdsi_add_bin_attrs(pdev);You just raced with userspace and lost. Please attach your attributes to the misc device before registering it.
Quoted 1 line
Also, you need a
Documentation/ABI/entry for your new sysfs file(s).
Ack
Quoted 19 lines
Quoted 19 lines
+ if (ret)
+ gotoderegister_misc;drivers/platform/x86/intel/sdsi.c new file +16
-0+ + priv->dev_present = true; + + return 0; + +deregister_misc: + misc_deregister(&priv->miscdev); +put_kref: + kref_put(&priv->kref, sdsi_priv_release); + + return ret; +} + +static int sdsi_remove (struct platform_device *pdev) +{ + struct sdsi_priv *priv = platform_get_drvdata(pdev);
Quoted 8 lines
Quoted 5 lines
+
+ priv->dev_present = false;
+ sysfs_remove_bin_file(&priv->pdev->dev.kobj,&priv->registers_bin_attr); + misc_deregister(&priv->miscdev);
+ kref_put(&priv->kref,sdsi_priv_release);Why do you need a kref for a structure that already can be controlled by a different lifetime rule?
Which rule am I missing? This kref allows the structure to remain in case the device is removed while the file is open.
Quoted 10 lines
Quoted 10 lines
drivers/platform/x86/intel/sdsi.c new file +9
-0+ + return 0; +} + +static struct platform_driver sdsi_driver = { + .driver = { + .name = SDSI_DEV_NAME, + .dev_groups = sdsi_groups, + },
Quoted 8 lines
drivers/platform/x86/intel/sdsi.c line 671
Quoted 4 lines
+ .probe =
sdsi_probe,
+ .remove =sdsi_remove,drivers/platform/x86/intel/sdsi.c new file +2
-0+}; +module_platform_driver (sdsi_driver);What causes the platform to know to register, and enable, this platform driver? Shouldn't there be some hardware involved that is discoverable to enable it to load dynamically?
Ah. The patch that adds the SDSi platform device string was added to a series for the intel_pmt MFD driver and it's still waiting review. I see that complicates things. I can combine the two series together.
David
Quoted 4 lines
thanks,
greg k-h
Re: [PATCH 2/2] platform/x86: Add Intel Software Defined Silicon driver
Quoted 12 lines · David E. Box
Quoted 11 lines
drivers/platform/x86/intel/sdsi.c line 658
Quoted 8 lines
drivers/platform/x86/intel/sdsi.c new file +8
-0+static int sdsi_remove (struct platform_device *pdev) +{ + struct sdsi_priv *priv = platform_get_drvdata(pdev); + + priv->dev_present = false; + sysfs_remove_bin_file(&priv->pdev->dev.kobj, &priv->registers_bin_attr); + misc_deregister(&priv->miscdev); + kref_put(&priv->kref, sdsi_priv_release);Why do you need a kref for a structure that already can be controlled by a different lifetime rule?
Quoted 2 lines
Which rule am I missing? This kref allows the structure to remain in case the device is removed while the file is open.
This device is on a hardware bus that allows removal?
Anyway, you now are dealing with lifetime rules of 3 structures all at once, and the interactions between them is not very obvious. It would probably be simpler just to stick with 2, right? You really only care about the misc structure here.
thanks,
greg k-h
David E. Box
maintainerRe: [PATCH 2/2] platform/x86: Add Intel Software Defined Silicon driver
Quoted 13 lines · Greg KH
Quoted 12 lines · David E. Box
Quoted 11 lines
drivers/platform/x86/intel/sdsi.c line 658
Quoted 8 lines
drivers/platform/x86/intel/sdsi.c new file +8
-0+static int sdsi_remove (struct platform_device *pdev) +{ + struct sdsi_priv *priv = platform_get_drvdata(pdev); + + priv->dev_present = false; + sysfs_remove_bin_file(&priv->pdev->dev.kobj, &priv->registers_bin_attr); + misc_deregister(&priv->miscdev); + kref_put(&priv->kref, sdsi_priv_release);Why do you need a kref for a structure that already can be controlled by a different lifetime rule?
Quoted 4 lines
Quoted 2 lines
Which rule am I missing? This kref allows the structure to remain in case the device is removed while the file is open.
This device is on a hardware bus that allows removal?
Well the device can be unbound. A test case covers this.
Quoted 5 lines
Anyway, you now are dealing with lifetime rules of 3 structures all at once, and the interactions between them is not very obvious. It would probably be simpler just to stick with 2, right? You really only care about the misc structure here.
In the case that the device is unbound, both the pdev and miscdev go away. Something has to outlive them in order to handle any open files still trying to use the ioctl.
David
Quoted 4 lines
thanks,
greg k-h
Re: [PATCH 2/2] platform/x86: Add Intel Software Defined Silicon driver
Quoted 14 lines · David E. Box
Quoted 13 lines · Greg KH
Quoted 12 lines · David E. Box
Quoted 11 lines
drivers/platform/x86/intel/sdsi.c line 658
Quoted 8 lines
drivers/platform/x86/intel/sdsi.c new file +8
-0+static int sdsi_remove (struct platform_device *pdev) +{ + struct sdsi_priv *priv = platform_get_drvdata(pdev); + + priv->dev_present = false; + sysfs_remove_bin_file(&priv->pdev->dev.kobj, &priv->registers_bin_attr); + misc_deregister(&priv->miscdev); + kref_put(&priv->kref, sdsi_priv_release);Why do you need a kref for a structure that already can be controlled by a different lifetime rule?
Quoted 6 lines
Quoted 4 lines
Quoted 2 lines
Which rule am I missing? This kref allows the structure to remain in case the device is removed while the file is open.
This device is on a hardware bus that allows removal?
Well the device can be unbound. A test case covers this.
Great, where are these tests? Why not add them to the kernel tree itself in the proper location?
And in the real-world, who would ever unbind this?
Quoted 7 lines
Quoted 4 lines
Anyway, you now are dealing with lifetime rules of 3 structures all at once, and the interactions between them is not very obvious. It would probably be simpler just to stick with 2, right? You really only care about the misc structure here.
In the case that the device is unbound, both the pdev and miscdev go away. Something has to outlive them in order to handle any open files still trying to use the ioctl.
I do not think that the miscdev goes away if the file handle is still open, right?
thanks,
greg k-h
Re: [PATCH 2/2] platform/x86: Add Intel Software Defined Silicon driver
In reply to David E. BoxQuoted 14 lines · David E. Box
Quoted 5 lines
Quoted 5 lines
+static struct
platform_driversdsi_driver= {
+ .driver = {
+ .name =SDSI_DEV_NAME,
+ .dev_groups =sdsi_groups,
+ },Quoted 8 lines
drivers/platform/x86/intel/sdsi.c line 671
Quoted 4 lines
+ .probe =
sdsi_probe,
+ .remove =sdsi_remove,drivers/platform/x86/intel/sdsi.c new file +2
-0+}; +module_platform_driver (sdsi_driver);What causes the platform to know to register, and enable, this platform driver? Shouldn't there be some hardware involved that is discoverable to enable it to load dynamically?
Quoted 3 lines
Ah. The patch that adds the SDSi platform device string was added to a series for the
intel_pmtMFD driver and it's still waiting review. I see that complicates things. I can combine the two series together.
Do you have a pointer to the lore.kernel.org location of that series?
Your code right here will bind to any system that it is loaded on, a very dangerous thing...
thanks,
greg k-h
David E. Box
maintainerRe: [PATCH 2/2] platform/x86: Add Intel Software Defined Silicon driver
Quoted 15 lines · Greg KH
Quoted 14 lines · David E. Box
Quoted 13 lines
drivers/platform/x86/intel/sdsi.c line 671
Quoted 9 lines
drivers/platform/x86/intel/sdsi.c new file +9
-0+static struct platform_driver sdsi_driver = { + .driver = { + .name = SDSI_DEV_NAME, + .dev_groups = sdsi_groups, + }, + .probe = sdsi_probe, + .remove = sdsi_remove, +}; +module_platform_driver (sdsi_driver);What causes the platform to know to register, and enable, this platform driver? Shouldn't there be some hardware involved that is discoverable to enable it to load dynamically?
Quoted 6 lines
Quoted 4 lines
Ah. The patch that adds the SDSi platform device string was added to a series for the
intel_pmtMFD
driver and it's still waiting review. I see that complicates things. I can combine the two series together.Do you have a pointer to the lore.kernel.org location of that series?
https://lore.kernel.org/all/[email protected]/
Quoted 3 lines
Your code right here will bind to any system that it is loaded on, a very dangerous thing...
It won't. It uses module alias to load against the SDSi specific MFD cell.
Quoted 4 lines
thanks,
greg k-h
Re: [PATCH 2/2] platform/x86: Add Intel Software Defined Silicon driver
Quoted 23 lines · David E. Box
Quoted 15 lines · Greg KH
Quoted 14 lines · David E. Box
Quoted 13 lines
drivers/platform/x86/intel/sdsi.c line 671
Quoted 9 lines
drivers/platform/x86/intel/sdsi.c new file +9
-0+static struct platform_driver sdsi_driver = { + .driver = { + .name = SDSI_DEV_NAME, + .dev_groups = sdsi_groups, + }, + .probe = sdsi_probe, + .remove = sdsi_remove, +}; +module_platform_driver (sdsi_driver);What causes the platform to know to register, and enable, this platform driver? Shouldn't there be some hardware involved that is discoverable to enable it to load dynamically?
Quoted 6 lines
Quoted 4 lines
Ah. The patch that adds the SDSi platform device string was added to a series for the
intel_pmtMFD
driver and it's still waiting review. I see that complicates things. I can combine the two series together.Do you have a pointer to the lore.kernel.org location of that series?
Quoted 7 lines
https://lore.kernel.org/all/[email protected]/
Quoted 3 lines
Your code right here will bind to any system that it is loaded on, a very dangerous thing...
It won't. It uses module alias to load against the SDSi specific MFD cell.
module aliases do not prevent someone from building the driver into their kernel, or doing a simple 'modprobe'. You need to bind your driver to a real hardware resource (and reject it if it is not present), otherwise this driver will break systems quite easily.
And again, why is this a platform driver and not just a "real" device on a bus? Heck, why not just use the auxiliary bus for stuff like this, why are you craming pci attributes into the MFD subsystem?
thanks,
greg k-h
Re: [PATCH 2/2] platform/x86: Add Intel Software Defined Silicon driver
In reply to David E. BoxQuoted 4 lines · David E. Box
The SDSi device itself is enumerated as a PCIe VSEC capability on the Intel Out Of Band Management Services Module (OOBMSM) device. The SDSi device is a cell of the
intel_pmtMFD driver and as such has a build dependency onCONFIG_MFD_INTEL_PMT.
I don't understand this. It looks like if this driver is loaded it instantly grabs the first iomem resource it can find and assumes that this is the device in the system. Where does it check to verify that the hardware really is present?
And this isn't really a platform device, so why is it written like one? Just because it is using the mfd subsystem?
PCIe devices are not platform devices, please use the real pci device structures for this type of thing.
thanks,
greg k-h