[PATCH 0/9] Extend Intel On Demand (SDSi) support

[PATCH 0/9] Extend Intel On Demand (SDSi) support

[David E. Box]

Intel Software Defined Silicon (SDSi) is now known as Intel On Demand. The
following patches do the following:

1. Identify the driver/tools as Intel On Demand. Only text descriptions are
changed. Kconfig and filenames remain the same.
2. Perform some attribute cleanup by preventing the showing of files when
features are not supported.
3. Adds support for a new GUID. GUIDs are used to identify the layout of
the On Demand registers in sysfs. Layouts are described in the
documentation on github [1].
4. Add support for reading On Demand meter certificates in sysfs.
5. The rest of the patches modify the existing tool to support discovery
and reading of On Demand registers and the meter certificate.

[1] https://github.com/intel/intel-sdsi/blob/master/os-interface.rst

David E. Box (9):
  platform/x86/intel/sdsi: Add Intel On Demand text
  platform/x86/intel/sdsi: Hide attributes if hardware doesn't support
  platform/x86/intel/sdsi: Support different GUIDs
  platform/x86/intel/sdsi: Add meter certificate support
  tools/arch/x86: intel_sdsi: Add support for reading state certificates
  tools/arch/x86: intel_sdsi: Add Intel On Demand text
  tools/arch/x86: intel_sdsi: Read more On Demand registers
  tools/arch/x86: intel_sdsi: Add support for new GUID
  tools/arch/x86: intel_sdsi: Add support for reading meter certificates

 .../ABI/testing/sysfs-driver-intel_sdsi       |  47 +-
 drivers/platform/x86/intel/Kconfig            |   8 +-
 drivers/platform/x86/intel/sdsi.c             | 134 ++++-
 tools/arch/x86/intel_sdsi/intel_sdsi.c        | 458 ++++++++++++++----
 4 files changed, 513 insertions(+), 134 deletions(-)


base-commit: 225469d4acbcb873358d7618bad6e0203b67b964
-- 
2.25.1

[PATCH 1/9] platform/x86/intel/sdsi: Add Intel On Demand text

[David E. Box]

Intel Software Defined Silicon (SDSi) is now officially known as Intel
On Demand. Add On Demand to the description in the kconfig, documentation,
and driver source.

Signed-off-by: David E. Box <david.e.box@linux.intel.com>
---
 .../ABI/testing/sysfs-driver-intel_sdsi       | 37 ++++++++++---------
 drivers/platform/x86/intel/Kconfig            |  8 ++--
 drivers/platform/x86/intel/sdsi.c             |  4 +-
 3 files changed, 25 insertions(+), 24 deletions(-)

diff --git a/Documentation/ABI/testing/sysfs-driver-intel_sdsi b/Documentation/ABI/testing/sysfs-driver-intel_sdsi
index 96b92c105ec4..9d77f30d9b9a 100644
--- a/Documentation/ABI/testing/sysfs-driver-intel_sdsi
+++ b/Documentation/ABI/testing/sysfs-driver-intel_sdsi
@@ -4,21 +4,21 @@ KernelVersion:	5.18
 Contact:	"David E. Box" <david.e.box@linux.intel.com>
 Description:
 		This directory 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.
+		On Demand (formerly Software Defined Silicon or 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:
+		Some files communicate with On Demand 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.
+		EPERM		On Demand 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
@@ -54,8 +54,8 @@ KernelVersion:	5.18
 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.
+		the On Demand 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:		Feb 2022
@@ -63,17 +63,18 @@ KernelVersion:	5.18
 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.
+		On Demand NVRAM for the CPU. CAPs are used to activate a given
+		CPU feature. A CAP is validated by On Demand 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:		Feb 2022
 KernelVersion:	5.18
 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.
+		(RO) Used to read back the current state certificate for the CPU
+		from On Demand hardware. The state certificate contains
+		information about the current licenses on the CPU. Mailbox
+		command.
diff --git a/drivers/platform/x86/intel/Kconfig b/drivers/platform/x86/intel/Kconfig
index 794968bda115..d5a33473e838 100644
--- a/drivers/platform/x86/intel/Kconfig
+++ b/drivers/platform/x86/intel/Kconfig
@@ -157,13 +157,13 @@ config INTEL_RST
 	  as usual.
 
 config INTEL_SDSI
-	tristate "Intel Software Defined Silicon Driver"
+	tristate "Intel On Demand (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.
+	  This driver enables access to the Intel On Demand (formerly 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.
diff --git a/drivers/platform/x86/intel/sdsi.c b/drivers/platform/x86/intel/sdsi.c
index c830e98dfa38..32793919473d 100644
--- a/drivers/platform/x86/intel/sdsi.c
+++ b/drivers/platform/x86/intel/sdsi.c
@@ -1,6 +1,6 @@
 // SPDX-License-Identifier: GPL-2.0
 /*
- * Intel Software Defined Silicon driver
+ * Intel On Demand (Software Defined Silicon) driver
  *
  * Copyright (c) 2022, Intel Corporation.
  * All Rights Reserved.
@@ -586,5 +586,5 @@ static struct auxiliary_driver sdsi_aux_driver = {
 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_DESCRIPTION("Intel On Demand (SDSi) driver");
 MODULE_LICENSE("GPL");
-- 
2.25.1

Re: [PATCH 1/9] platform/x86/intel/sdsi: Add Intel On Demand text

[Hans de Goede]

Hi,

On 11/1/22 20:10, David E. Box wrote:
> Intel Software Defined Silicon (SDSi) is now officially known as Intel
> On Demand. Add On Demand to the description in the kconfig, documentation,
> and driver source.
> 
> Signed-off-by: David E. Box <david.e.box@linux.intel.com>

Thanks, patch looks good to me:

Reviewed-by: Hans de Goede <hdegoede@redhat.com>

Regards,

Hans



> ---
>  .../ABI/testing/sysfs-driver-intel_sdsi       | 37 ++++++++++---------
>  drivers/platform/x86/intel/Kconfig            |  8 ++--
>  drivers/platform/x86/intel/sdsi.c             |  4 +-
>  3 files changed, 25 insertions(+), 24 deletions(-)
> 
> diff --git a/Documentation/ABI/testing/sysfs-driver-intel_sdsi b/Documentation/ABI/testing/sysfs-driver-intel_sdsi
> index 96b92c105ec4..9d77f30d9b9a 100644
> --- a/Documentation/ABI/testing/sysfs-driver-intel_sdsi
> +++ b/Documentation/ABI/testing/sysfs-driver-intel_sdsi
> @@ -4,21 +4,21 @@ KernelVersion:	5.18
>  Contact:	"David E. Box" <david.e.box@linux.intel.com>
>  Description:
>  		This directory 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.
> +		On Demand (formerly Software Defined Silicon or 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:
> +		Some files communicate with On Demand 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.
> +		EPERM		On Demand 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
> @@ -54,8 +54,8 @@ KernelVersion:	5.18
>  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.
> +		the On Demand 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:		Feb 2022
> @@ -63,17 +63,18 @@ KernelVersion:	5.18
>  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.
> +		On Demand NVRAM for the CPU. CAPs are used to activate a given
> +		CPU feature. A CAP is validated by On Demand 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:		Feb 2022
>  KernelVersion:	5.18
>  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.
> +		(RO) Used to read back the current state certificate for the CPU
> +		from On Demand hardware. The state certificate contains
> +		information about the current licenses on the CPU. Mailbox
> +		command.
> diff --git a/drivers/platform/x86/intel/Kconfig b/drivers/platform/x86/intel/Kconfig
> index 794968bda115..d5a33473e838 100644
> --- a/drivers/platform/x86/intel/Kconfig
> +++ b/drivers/platform/x86/intel/Kconfig
> @@ -157,13 +157,13 @@ config INTEL_RST
>  	  as usual.
>  
>  config INTEL_SDSI
> -	tristate "Intel Software Defined Silicon Driver"
> +	tristate "Intel On Demand (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.
> +	  This driver enables access to the Intel On Demand (formerly 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.
> diff --git a/drivers/platform/x86/intel/sdsi.c b/drivers/platform/x86/intel/sdsi.c
> index c830e98dfa38..32793919473d 100644
> --- a/drivers/platform/x86/intel/sdsi.c
> +++ b/drivers/platform/x86/intel/sdsi.c
> @@ -1,6 +1,6 @@
>  // SPDX-License-Identifier: GPL-2.0
>  /*
> - * Intel Software Defined Silicon driver
> + * Intel On Demand (Software Defined Silicon) driver
>   *
>   * Copyright (c) 2022, Intel Corporation.
>   * All Rights Reserved.
> @@ -586,5 +586,5 @@ static struct auxiliary_driver sdsi_aux_driver = {
>  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_DESCRIPTION("Intel On Demand (SDSi) driver");
>  MODULE_LICENSE("GPL");

[PATCH 2/9] platform/x86/intel/sdsi: Hide attributes if hardware doesn't support

[David E. Box]

Provisioning capabilities are enabled by a bit set by BIOS. Read this bit
and hide the provisioning attributes if the On Demand feature is not
enabled.

Also, remove the sdsi_enabled boolean from private and instead add a
features register since this will be used for future features.

Signed-off-by: David E. Box <david.e.box@linux.intel.com>
---
 drivers/platform/x86/intel/sdsi.c | 33 ++++++++++++++++++++-----------
 1 file changed, 22 insertions(+), 11 deletions(-)

diff --git a/drivers/platform/x86/intel/sdsi.c b/drivers/platform/x86/intel/sdsi.c
index 32793919473d..bca05b4dd983 100644
--- a/drivers/platform/x86/intel/sdsi.c
+++ b/drivers/platform/x86/intel/sdsi.c
@@ -41,7 +41,8 @@
 #define SDSI_SIZE_READ_MSG		(SDSI_SIZE_MAILBOX * 4)
 
 #define SDSI_ENABLED_FEATURES_OFFSET	16
-#define SDSI_ENABLED			BIT(3)
+#define SDSI_FEATURE_SDSI		BIT(3)
+
 #define SDSI_SOCKET_ID_OFFSET		64
 #define SDSI_SOCKET_ID			GENMASK(3, 0)
 
@@ -100,7 +101,7 @@ struct sdsi_priv {
 	void __iomem		*mbox_addr;
 	void __iomem		*regs_addr;
 	u32			guid;
-	bool			sdsi_enabled;
+	u32			features;
 };
 
 /* SDSi mailbox operations must be performed using 64bit mov instructions */
@@ -332,9 +333,6 @@ static ssize_t sdsi_provision(struct sdsi_priv *priv, char *buf, size_t count,
 	struct sdsi_mbox_info info;
 	int ret;
 
-	if (!priv->sdsi_enabled)
-		return -EPERM;
-
 	if (count > (SDSI_SIZE_WRITE_MSG - SDSI_SIZE_CMD))
 		return -EOVERFLOW;
 
@@ -405,9 +403,6 @@ static long state_certificate_read(struct file *filp, struct kobject *kobj,
 	size_t size;
 	int ret;
 
-	if (!priv->sdsi_enabled)
-		return -EPERM;
-
 	if (off)
 		return 0;
 
@@ -464,6 +459,23 @@ static struct bin_attribute *sdsi_bin_attrs[] = {
 	NULL
 };
 
+static umode_t
+sdsi_battr_is_visible(struct kobject *kobj, struct bin_attribute *attr, int n)
+{
+	struct device *dev = kobj_to_dev(kobj);
+	struct sdsi_priv *priv = dev_get_drvdata(dev);
+
+	/* Registers file is always readable if the device is present */
+	if (attr == &bin_attr_registers)
+		return attr->attr.mode;
+
+	/* All other attributes not visible if BIOS has not enabled On Demand */
+	if (!(priv->features & SDSI_FEATURE_SDSI))
+		return 0;
+
+	return attr->attr.mode;
+}
+
 static ssize_t guid_show(struct device *dev, struct device_attribute *attr, char *buf)
 {
 	struct sdsi_priv *priv = dev_get_drvdata(dev);
@@ -480,6 +492,7 @@ static struct attribute *sdsi_attrs[] = {
 static const struct attribute_group sdsi_group = {
 	.attrs = sdsi_attrs,
 	.bin_attrs = sdsi_bin_attrs,
+	.is_bin_visible = sdsi_battr_is_visible,
 };
 __ATTRIBUTE_GROUPS(sdsi);
 
@@ -490,7 +503,6 @@ static int sdsi_map_mbox_registers(struct sdsi_priv *priv, struct pci_dev *paren
 	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 */
@@ -528,8 +540,7 @@ static int sdsi_map_mbox_registers(struct sdsi_priv *priv, struct pci_dev *paren
 	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);
+	priv->features = readq(priv->regs_addr + SDSI_ENABLED_FEATURES_OFFSET);
 
 	return 0;
 }
-- 
2.25.1

Re: [PATCH 2/9] platform/x86/intel/sdsi: Hide attributes if hardware doesn't support

[Hans de Goede]

Hi,

On 11/1/22 20:10, David E. Box wrote:
> Provisioning capabilities are enabled by a bit set by BIOS. Read this bit
> and hide the provisioning attributes if the On Demand feature is not
> enabled.
> 
> Also, remove the sdsi_enabled boolean from private and instead add a
> features register since this will be used for future features.
> 
> Signed-off-by: David E. Box <david.e.box@linux.intel.com>

Thanks, patch looks good to me:

Reviewed-by: Hans de Goede <hdegoede@redhat.com>

Regards,

Hans


> ---
>  drivers/platform/x86/intel/sdsi.c | 33 ++++++++++++++++++++-----------
>  1 file changed, 22 insertions(+), 11 deletions(-)
> 
> diff --git a/drivers/platform/x86/intel/sdsi.c b/drivers/platform/x86/intel/sdsi.c
> index 32793919473d..bca05b4dd983 100644
> --- a/drivers/platform/x86/intel/sdsi.c
> +++ b/drivers/platform/x86/intel/sdsi.c
> @@ -41,7 +41,8 @@
>  #define SDSI_SIZE_READ_MSG		(SDSI_SIZE_MAILBOX * 4)
>  
>  #define SDSI_ENABLED_FEATURES_OFFSET	16
> -#define SDSI_ENABLED			BIT(3)
> +#define SDSI_FEATURE_SDSI		BIT(3)
> +
>  #define SDSI_SOCKET_ID_OFFSET		64
>  #define SDSI_SOCKET_ID			GENMASK(3, 0)
>  
> @@ -100,7 +101,7 @@ struct sdsi_priv {
>  	void __iomem		*mbox_addr;
>  	void __iomem		*regs_addr;
>  	u32			guid;
> -	bool			sdsi_enabled;
> +	u32			features;
>  };
>  
>  /* SDSi mailbox operations must be performed using 64bit mov instructions */
> @@ -332,9 +333,6 @@ static ssize_t sdsi_provision(struct sdsi_priv *priv, char *buf, size_t count,
>  	struct sdsi_mbox_info info;
>  	int ret;
>  
> -	if (!priv->sdsi_enabled)
> -		return -EPERM;
> -
>  	if (count > (SDSI_SIZE_WRITE_MSG - SDSI_SIZE_CMD))
>  		return -EOVERFLOW;
>  
> @@ -405,9 +403,6 @@ static long state_certificate_read(struct file *filp, struct kobject *kobj,
>  	size_t size;
>  	int ret;
>  
> -	if (!priv->sdsi_enabled)
> -		return -EPERM;
> -
>  	if (off)
>  		return 0;
>  
> @@ -464,6 +459,23 @@ static struct bin_attribute *sdsi_bin_attrs[] = {
>  	NULL
>  };
>  
> +static umode_t
> +sdsi_battr_is_visible(struct kobject *kobj, struct bin_attribute *attr, int n)
> +{
> +	struct device *dev = kobj_to_dev(kobj);
> +	struct sdsi_priv *priv = dev_get_drvdata(dev);
> +
> +	/* Registers file is always readable if the device is present */
> +	if (attr == &bin_attr_registers)
> +		return attr->attr.mode;
> +
> +	/* All other attributes not visible if BIOS has not enabled On Demand */
> +	if (!(priv->features & SDSI_FEATURE_SDSI))
> +		return 0;
> +
> +	return attr->attr.mode;
> +}
> +
>  static ssize_t guid_show(struct device *dev, struct device_attribute *attr, char *buf)
>  {
>  	struct sdsi_priv *priv = dev_get_drvdata(dev);
> @@ -480,6 +492,7 @@ static struct attribute *sdsi_attrs[] = {
>  static const struct attribute_group sdsi_group = {
>  	.attrs = sdsi_attrs,
>  	.bin_attrs = sdsi_bin_attrs,
> +	.is_bin_visible = sdsi_battr_is_visible,
>  };
>  __ATTRIBUTE_GROUPS(sdsi);
>  
> @@ -490,7 +503,6 @@ static int sdsi_map_mbox_registers(struct sdsi_priv *priv, struct pci_dev *paren
>  	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 */
> @@ -528,8 +540,7 @@ static int sdsi_map_mbox_registers(struct sdsi_priv *priv, struct pci_dev *paren
>  	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);
> +	priv->features = readq(priv->regs_addr + SDSI_ENABLED_FEATURES_OFFSET);
>  
>  	return 0;
>  }

[PATCH 3/9] platform/x86/intel/sdsi: Support different GUIDs

[David E. Box]

Newer versions of Intel On Demand hardware may have an expanded list of
registers to support new features. The register layout is identified by a
unique GUID that's read during driver probe. Add support for handling
different GUIDs and add support for current GUIDs [1].

[1] https://github.com/intel/intel-sdsi/blob/master/os-interface.rst

Signed-off-by: David E. Box <david.e.box@linux.intel.com>
---
 drivers/platform/x86/intel/sdsi.c | 47 +++++++++++++++++++++++++++++--
 1 file changed, 44 insertions(+), 3 deletions(-)

diff --git a/drivers/platform/x86/intel/sdsi.c b/drivers/platform/x86/intel/sdsi.c
index bca05b4dd983..ab1f65919fc5 100644
--- a/drivers/platform/x86/intel/sdsi.c
+++ b/drivers/platform/x86/intel/sdsi.c
@@ -27,10 +27,10 @@
 #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_REGS			80
 #define SDSI_SIZE_CMD			sizeof(u64)
+#define SDSI_SIZE_MAILBOX		1024
 
 /*
  * Write messages are currently up to the size of the mailbox
@@ -76,6 +76,9 @@
 #define DT_TBIR				GENMASK(2, 0)
 #define DT_OFFSET(v)			((v) & GENMASK(31, 3))
 
+#define SDSI_GUID_V1			0x006DD191
+#define SDSI_GUID_V2			0xF210D9EF
+
 enum sdsi_command {
 	SDSI_CMD_PROVISION_AKC		= 0x04,
 	SDSI_CMD_PROVISION_CAP		= 0x08,
@@ -100,6 +103,9 @@ struct sdsi_priv {
 	void __iomem		*control_addr;
 	void __iomem		*mbox_addr;
 	void __iomem		*regs_addr;
+	int			control_size;
+	int			maibox_size;
+	int			registers_size;
 	u32			guid;
 	u32			features;
 };
@@ -444,6 +450,18 @@ static ssize_t registers_read(struct file *filp, struct kobject *kobj,
 	struct device *dev = kobj_to_dev(kobj);
 	struct sdsi_priv *priv = dev_get_drvdata(dev);
 	void __iomem *addr = priv->regs_addr;
+	int size =  priv->registers_size;
+
+	/*
+	 * The check below is performed by the sysfs caller based on the static
+	 * file size. But this may be greater than the actual size which is based
+	 * on the GUID. So check here again based on actual size before reading.
+	 */
+	if (off >= size)
+		return 0;
+
+	if (off + count > size)
+		count = size - off;
 
 	memcpy_fromio(buf, addr + off, count);
 
@@ -496,6 +514,24 @@ static const struct attribute_group sdsi_group = {
 };
 __ATTRIBUTE_GROUPS(sdsi);
 
+static int sdsi_get_layout(struct sdsi_priv *priv, struct disc_table *table)
+{
+	switch (table->guid) {
+	case SDSI_GUID_V1:
+		priv->control_size = 8;
+		priv->registers_size = 72;
+		break;
+	case SDSI_GUID_V2:
+		priv->control_size = 16;
+		priv->registers_size = 80;
+		break;
+	default:
+		dev_err(priv->dev, "Unrecognized GUID 0x%x\n", table->guid);
+		return -EINVAL;
+	}
+	return 0;
+}
+
 static int sdsi_map_mbox_registers(struct sdsi_priv *priv, struct pci_dev *parent,
 				   struct disc_table *disc_table, struct resource *disc_res)
 {
@@ -537,7 +573,7 @@ static int sdsi_map_mbox_registers(struct sdsi_priv *priv, struct pci_dev *paren
 	if (IS_ERR(priv->control_addr))
 		return PTR_ERR(priv->control_addr);
 
-	priv->mbox_addr = priv->control_addr + SDSI_SIZE_CONTROL;
+	priv->mbox_addr = priv->control_addr + priv->control_size;
 	priv->regs_addr = priv->mbox_addr + SDSI_SIZE_MAILBOX;
 
 	priv->features = readq(priv->regs_addr + SDSI_ENABLED_FEATURES_OFFSET);
@@ -572,6 +608,11 @@ static int sdsi_probe(struct auxiliary_device *auxdev, const struct auxiliary_de
 
 	priv->guid = disc_table.guid;
 
+	/* Get guid based layout info */
+	ret = sdsi_get_layout(priv, &disc_table);
+	if (ret)
+		return ret;
+
 	/* Map the SDSi mailbox registers */
 	ret = sdsi_map_mbox_registers(priv, intel_cap_dev->pcidev, &disc_table, disc_res);
 	if (ret)
-- 
2.25.1

Re: [PATCH 3/9] platform/x86/intel/sdsi: Support different GUIDs

[Andy Shevchenko]

On Tue, Nov 01, 2022 at 12:10:17PM -0700, David E. Box wrote:
> Newer versions of Intel On Demand hardware may have an expanded list of
> registers to support new features. The register layout is identified by a
> unique GUID that's read during driver probe. Add support for handling
> different GUIDs and add support for current GUIDs [1].

> [1] https://github.com/intel/intel-sdsi/blob/master/os-interface.rst

Link: tag?

...

>  #define SDSI_MIN_SIZE_DWORDS		276
> -#define SDSI_SIZE_CONTROL		8
>  #define SDSI_SIZE_MAILBOX		1024
> -#define SDSI_SIZE_REGS			72
> +#define SDSI_SIZE_REGS			80
>  #define SDSI_SIZE_CMD			sizeof(u64)

> +#define SDSI_SIZE_MAILBOX		1024

Why do you need this second time?

...

> +static int sdsi_get_layout(struct sdsi_priv *priv, struct disc_table *table)
> +{
> +	switch (table->guid) {
> +	case SDSI_GUID_V1:
> +		priv->control_size = 8;
> +		priv->registers_size = 72;
> +		break;
> +	case SDSI_GUID_V2:
> +		priv->control_size = 16;
> +		priv->registers_size = 80;

Maybe it makes sense to use previously defined constants here instead of magics?

> +		break;
> +	default:
> +		dev_err(priv->dev, "Unrecognized GUID 0x%x\n", table->guid);
> +		return -EINVAL;
> +	}
> +	return 0;
> +}

-- 
With Best Regards,
Andy Shevchenko

Re: [PATCH 3/9] platform/x86/intel/sdsi: Support different GUIDs

[David E. Box]

On Wed, 2022-11-02 at 12:44 +0200, Andy Shevchenko wrote:
> On Tue, Nov 01, 2022 at 12:10:17PM -0700, David E. Box wrote:
> > Newer versions of Intel On Demand hardware may have an expanded list of
> > registers to support new features. The register layout is identified by a
> > unique GUID that's read during driver probe. Add support for handling
> > different GUIDs and add support for current GUIDs [1].
> > [1] https://github.com/intel/intel-sdsi/blob/master/os-interface.rst
> 
> Link: tag?

Ack

> 
> ...
> 
> >  #define SDSI_MIN_SIZE_DWORDS		276
> > -#define SDSI_SIZE_CONTROL		8
> >  #define SDSI_SIZE_MAILBOX		1024
> > -#define SDSI_SIZE_REGS			72
> > +#define SDSI_SIZE_REGS			80
> >  #define SDSI_SIZE_CMD			sizeof(u64)
> > +#define SDSI_SIZE_MAILBOX		1024
> 
> Why do you need this second time?

typo

> 
> ...
> 
> > +static int sdsi_get_layout(struct sdsi_priv *priv, struct disc_table
> > *table)
> > +{
> > +	switch (table->guid) {
> > +	case SDSI_GUID_V1:
> > +		priv->control_size = 8;
> > +		priv->registers_size = 72;
> > +		break;
> > +	case SDSI_GUID_V2:
> > +		priv->control_size = 16;
> > +		priv->registers_size = 80;
> 
> Maybe it makes sense to use previously defined constants here instead of
> magics?

The constant is used for the file size, which since is static is set to the max.
But I'll add defines for these.

> 
> > +		break;
> > +	default:
> > +		dev_err(priv->dev, "Unrecognized GUID 0x%x\n", table->guid);
> > +		return -EINVAL;
> > +	}
> > +	return 0;
> > +}

Re: [PATCH 3/9] platform/x86/intel/sdsi: Support different GUIDs

[Hans de Goede]

Hi,

On 11/1/22 20:10, David E. Box wrote:
> Newer versions of Intel On Demand hardware may have an expanded list of
> registers to support new features. The register layout is identified by a
> unique GUID that's read during driver probe. Add support for handling
> different GUIDs and add support for current GUIDs [1].
> 
> [1] https://github.com/intel/intel-sdsi/blob/master/os-interface.rst
> 
> Signed-off-by: David E. Box <david.e.box@linux.intel.com>

With Andy's remarks fixed this looks good to me:

Reviewed-by: Hans de Goede <hdegoede@redhat.com>

Regards,

Hans



> ---
>  drivers/platform/x86/intel/sdsi.c | 47 +++++++++++++++++++++++++++++--
>  1 file changed, 44 insertions(+), 3 deletions(-)
> 
> diff --git a/drivers/platform/x86/intel/sdsi.c b/drivers/platform/x86/intel/sdsi.c
> index bca05b4dd983..ab1f65919fc5 100644
> --- a/drivers/platform/x86/intel/sdsi.c
> +++ b/drivers/platform/x86/intel/sdsi.c
> @@ -27,10 +27,10 @@
>  #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_REGS			80
>  #define SDSI_SIZE_CMD			sizeof(u64)
> +#define SDSI_SIZE_MAILBOX		1024
>  
>  /*
>   * Write messages are currently up to the size of the mailbox
> @@ -76,6 +76,9 @@
>  #define DT_TBIR				GENMASK(2, 0)
>  #define DT_OFFSET(v)			((v) & GENMASK(31, 3))
>  
> +#define SDSI_GUID_V1			0x006DD191
> +#define SDSI_GUID_V2			0xF210D9EF
> +
>  enum sdsi_command {
>  	SDSI_CMD_PROVISION_AKC		= 0x04,
>  	SDSI_CMD_PROVISION_CAP		= 0x08,
> @@ -100,6 +103,9 @@ struct sdsi_priv {
>  	void __iomem		*control_addr;
>  	void __iomem		*mbox_addr;
>  	void __iomem		*regs_addr;
> +	int			control_size;
> +	int			maibox_size;
> +	int			registers_size;
>  	u32			guid;
>  	u32			features;
>  };
> @@ -444,6 +450,18 @@ static ssize_t registers_read(struct file *filp, struct kobject *kobj,
>  	struct device *dev = kobj_to_dev(kobj);
>  	struct sdsi_priv *priv = dev_get_drvdata(dev);
>  	void __iomem *addr = priv->regs_addr;
> +	int size =  priv->registers_size;
> +
> +	/*
> +	 * The check below is performed by the sysfs caller based on the static
> +	 * file size. But this may be greater than the actual size which is based
> +	 * on the GUID. So check here again based on actual size before reading.
> +	 */
> +	if (off >= size)
> +		return 0;
> +
> +	if (off + count > size)
> +		count = size - off;
>  
>  	memcpy_fromio(buf, addr + off, count);
>  
> @@ -496,6 +514,24 @@ static const struct attribute_group sdsi_group = {
>  };
>  __ATTRIBUTE_GROUPS(sdsi);
>  
> +static int sdsi_get_layout(struct sdsi_priv *priv, struct disc_table *table)
> +{
> +	switch (table->guid) {
> +	case SDSI_GUID_V1:
> +		priv->control_size = 8;
> +		priv->registers_size = 72;
> +		break;
> +	case SDSI_GUID_V2:
> +		priv->control_size = 16;
> +		priv->registers_size = 80;
> +		break;
> +	default:
> +		dev_err(priv->dev, "Unrecognized GUID 0x%x\n", table->guid);
> +		return -EINVAL;
> +	}
> +	return 0;
> +}
> +
>  static int sdsi_map_mbox_registers(struct sdsi_priv *priv, struct pci_dev *parent,
>  				   struct disc_table *disc_table, struct resource *disc_res)
>  {
> @@ -537,7 +573,7 @@ static int sdsi_map_mbox_registers(struct sdsi_priv *priv, struct pci_dev *paren
>  	if (IS_ERR(priv->control_addr))
>  		return PTR_ERR(priv->control_addr);
>  
> -	priv->mbox_addr = priv->control_addr + SDSI_SIZE_CONTROL;
> +	priv->mbox_addr = priv->control_addr + priv->control_size;
>  	priv->regs_addr = priv->mbox_addr + SDSI_SIZE_MAILBOX;
>  
>  	priv->features = readq(priv->regs_addr + SDSI_ENABLED_FEATURES_OFFSET);
> @@ -572,6 +608,11 @@ static int sdsi_probe(struct auxiliary_device *auxdev, const struct auxiliary_de
>  
>  	priv->guid = disc_table.guid;
>  
> +	/* Get guid based layout info */
> +	ret = sdsi_get_layout(priv, &disc_table);
> +	if (ret)
> +		return ret;
> +
>  	/* Map the SDSi mailbox registers */
>  	ret = sdsi_map_mbox_registers(priv, intel_cap_dev->pcidev, &disc_table, disc_res);
>  	if (ret)

[PATCH 4/9] platform/x86/intel/sdsi: Add meter certificate support

[David E. Box]

Add support for reading the meter certificate from Intel On Demand
hardware.  The meter certificate [1] is used to access the utilization
metrics of enabled features in support of the Intel On Demand consumption
model. Similar to the state certificate, the meter certificate is read by
mailbox command.

[1] https://github.com/intel-sandbox/debox1.intel_sdsi/blob/gnr-review/meter-certificate.rst

Signed-off-by: David E. Box <david.e.box@linux.intel.com>
---
 .../ABI/testing/sysfs-driver-intel_sdsi       | 10 ++++
 drivers/platform/x86/intel/sdsi.c             | 52 +++++++++++++++----
 2 files changed, 52 insertions(+), 10 deletions(-)

diff --git a/Documentation/ABI/testing/sysfs-driver-intel_sdsi b/Documentation/ABI/testing/sysfs-driver-intel_sdsi
index 9d77f30d9b9a..f8afed127107 100644
--- a/Documentation/ABI/testing/sysfs-driver-intel_sdsi
+++ b/Documentation/ABI/testing/sysfs-driver-intel_sdsi
@@ -69,6 +69,16 @@ Description:
 		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/meter_certificate
+Date:		Nov 2022
+KernelVersion:	6.2
+Contact:	"David E. Box" <david.e.box@linux.intel.com>
+Description:
+		(RO) Used to read back the current meter certificate for the CPU
+		from Intel On Demand hardware. The meter certificate contains
+		utilization metrics of On Demand enabled features. Mailbox
+		command.
+
 What:		/sys/bus/auxiliary/devices/intel_vsec.sdsi.X/state_certificate
 Date:		Feb 2022
 KernelVersion:	5.18
diff --git a/drivers/platform/x86/intel/sdsi.c b/drivers/platform/x86/intel/sdsi.c
index ab1f65919fc5..1dee10822df7 100644
--- a/drivers/platform/x86/intel/sdsi.c
+++ b/drivers/platform/x86/intel/sdsi.c
@@ -42,6 +42,7 @@
 
 #define SDSI_ENABLED_FEATURES_OFFSET	16
 #define SDSI_FEATURE_SDSI		BIT(3)
+#define SDSI_FEATURE_METERING		BIT(26)
 
 #define SDSI_SOCKET_ID_OFFSET		64
 #define SDSI_SOCKET_ID			GENMASK(3, 0)
@@ -80,9 +81,10 @@
 #define SDSI_GUID_V2			0xF210D9EF
 
 enum sdsi_command {
-	SDSI_CMD_PROVISION_AKC		= 0x04,
-	SDSI_CMD_PROVISION_CAP		= 0x08,
-	SDSI_CMD_READ_STATE		= 0x10,
+	SDSI_CMD_PROVISION_AKC		= 0x0004,
+	SDSI_CMD_PROVISION_CAP		= 0x0008,
+	SDSI_CMD_READ_STATE		= 0x0010,
+	SDSI_CMD_READ_METER		= 0x0014,
 };
 
 struct sdsi_mbox_info {
@@ -398,13 +400,10 @@ static ssize_t provision_cap_write(struct file *filp, struct kobject *kobj,
 }
 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)
+static ssize_t
+certificate_read(u64 command, struct sdsi_priv *priv, 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;
 	size_t size;
 	int ret;
@@ -441,8 +440,31 @@ static long state_certificate_read(struct file *filp, struct kobject *kobj,
 
 	return size;
 }
+
+static ssize_t
+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);
+
+	return certificate_read(SDSI_CMD_READ_STATE, priv, buf, off, count);
+}
 static BIN_ATTR(state_certificate, 0400, state_certificate_read, NULL, SDSI_SIZE_READ_MSG);
 
+static ssize_t
+meter_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);
+
+	return certificate_read(SDSI_CMD_READ_METER, priv, buf, off, count);
+}
+static BIN_ATTR(meter_certificate, 0400, meter_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)
@@ -472,6 +494,7 @@ 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_meter_certificate,
 	&bin_attr_provision_akc,
 	&bin_attr_provision_cap,
 	NULL
@@ -491,7 +514,16 @@ sdsi_battr_is_visible(struct kobject *kobj, struct bin_attribute *attr, int n)
 	if (!(priv->features & SDSI_FEATURE_SDSI))
 		return 0;
 
-	return attr->attr.mode;
+	if (attr == &bin_attr_state_certificate ||
+	    attr == &bin_attr_provision_akc ||
+	    attr == &bin_attr_provision_cap)
+		return attr->attr.mode;
+
+	if (attr == &bin_attr_meter_certificate &&
+	    !!(priv->features & SDSI_FEATURE_METERING))
+		return attr->attr.mode;
+
+	return 0;
 }
 
 static ssize_t guid_show(struct device *dev, struct device_attribute *attr, char *buf)
-- 
2.25.1

Re: [PATCH 4/9] platform/x86/intel/sdsi: Add meter certificate support

[Andy Shevchenko]

On Tue, Nov 01, 2022 at 12:10:18PM -0700, David E. Box wrote:
> Add support for reading the meter certificate from Intel On Demand
> hardware.  The meter certificate [1] is used to access the utilization
> metrics of enabled features in support of the Intel On Demand consumption
> model. Similar to the state certificate, the meter certificate is read by
> mailbox command.
> 
> [1] https://github.com/intel-sandbox/debox1.intel_sdsi/blob/gnr-review/meter-certificate.rst

Link: tag?

...

>  static BIN_ATTR(state_certificate, 0400, state_certificate_read, NULL, SDSI_SIZE_READ_MSG);

BIN_ATTR_ADMiN_RO() ?

...

> +static BIN_ATTR(meter_certificate, 0400, meter_certificate_read, NULL, SDSI_SIZE_READ_MSG);

Ditto.

-- 
With Best Regards,
Andy Shevchenko

Re: [PATCH 4/9] platform/x86/intel/sdsi: Add meter certificate support

[David E. Box]

On Wed, 2022-11-02 at 12:46 +0200, Andy Shevchenko wrote:
> On Tue, Nov 01, 2022 at 12:10:18PM -0700, David E. Box wrote:
> > Add support for reading the meter certificate from Intel On Demand
> > hardware.  The meter certificate [1] is used to access the utilization
> > metrics of enabled features in support of the Intel On Demand consumption
> > model. Similar to the state certificate, the meter certificate is read by
> > mailbox command.
> > 
> > [1] 
> > https://github.com/intel-sandbox/debox1.intel_sdsi/blob/gnr-review/meter-certificate.rst
> 
> Link: tag?
> 
> ...
> 
> >  static BIN_ATTR(state_certificate, 0400, state_certificate_read, NULL,
> > SDSI_SIZE_READ_MSG);
> 
> BIN_ATTR_ADMiN_RO() ?
> 
> ...
> 
> > +static BIN_ATTR(meter_certificate, 0400, meter_certificate_read, NULL,
> > SDSI_SIZE_READ_MSG);
> 
> Ditto.
> 

Ack on all. Thanks Andy.

Re: [PATCH 4/9] platform/x86/intel/sdsi: Add meter certificate support

[Hans de Goede]

Hi,

On 11/1/22 20:10, David E. Box wrote:
> Add support for reading the meter certificate from Intel On Demand
> hardware.  The meter certificate [1] is used to access the utilization
> metrics of enabled features in support of the Intel On Demand consumption
> model. Similar to the state certificate, the meter certificate is read by
> mailbox command.
> 
> [1] https://github.com/intel-sandbox/debox1.intel_sdsi/blob/gnr-review/meter-certificate.rst
> 
> Signed-off-by: David E. Box <david.e.box@linux.intel.com>

Besides Andy's remarks I also have 1 remark myself, see below.

> ---
>  .../ABI/testing/sysfs-driver-intel_sdsi       | 10 ++++
>  drivers/platform/x86/intel/sdsi.c             | 52 +++++++++++++++----
>  2 files changed, 52 insertions(+), 10 deletions(-)
> 
> diff --git a/Documentation/ABI/testing/sysfs-driver-intel_sdsi b/Documentation/ABI/testing/sysfs-driver-intel_sdsi
> index 9d77f30d9b9a..f8afed127107 100644
> --- a/Documentation/ABI/testing/sysfs-driver-intel_sdsi
> +++ b/Documentation/ABI/testing/sysfs-driver-intel_sdsi
> @@ -69,6 +69,16 @@ Description:
>  		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/meter_certificate
> +Date:		Nov 2022
> +KernelVersion:	6.2
> +Contact:	"David E. Box" <david.e.box@linux.intel.com>
> +Description:
> +		(RO) Used to read back the current meter certificate for the CPU
> +		from Intel On Demand hardware. The meter certificate contains
> +		utilization metrics of On Demand enabled features. Mailbox
> +		command.
> +
>  What:		/sys/bus/auxiliary/devices/intel_vsec.sdsi.X/state_certificate
>  Date:		Feb 2022
>  KernelVersion:	5.18
> diff --git a/drivers/platform/x86/intel/sdsi.c b/drivers/platform/x86/intel/sdsi.c
> index ab1f65919fc5..1dee10822df7 100644
> --- a/drivers/platform/x86/intel/sdsi.c
> +++ b/drivers/platform/x86/intel/sdsi.c
> @@ -42,6 +42,7 @@
>  
>  #define SDSI_ENABLED_FEATURES_OFFSET	16
>  #define SDSI_FEATURE_SDSI		BIT(3)
> +#define SDSI_FEATURE_METERING		BIT(26)
>  
>  #define SDSI_SOCKET_ID_OFFSET		64
>  #define SDSI_SOCKET_ID			GENMASK(3, 0)
> @@ -80,9 +81,10 @@
>  #define SDSI_GUID_V2			0xF210D9EF
>  
>  enum sdsi_command {
> -	SDSI_CMD_PROVISION_AKC		= 0x04,
> -	SDSI_CMD_PROVISION_CAP		= 0x08,
> -	SDSI_CMD_READ_STATE		= 0x10,
> +	SDSI_CMD_PROVISION_AKC		= 0x0004,
> +	SDSI_CMD_PROVISION_CAP		= 0x0008,
> +	SDSI_CMD_READ_STATE		= 0x0010,
> +	SDSI_CMD_READ_METER		= 0x0014,
>  };
>  
>  struct sdsi_mbox_info {
> @@ -398,13 +400,10 @@ static ssize_t provision_cap_write(struct file *filp, struct kobject *kobj,
>  }
>  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)
> +static ssize_t
> +certificate_read(u64 command, struct sdsi_priv *priv, 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;
>  	size_t size;
>  	int ret;
> @@ -441,8 +440,31 @@ static long state_certificate_read(struct file *filp, struct kobject *kobj,
>  
>  	return size;
>  }
> +
> +static ssize_t
> +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);
> +
> +	return certificate_read(SDSI_CMD_READ_STATE, priv, buf, off, count);
> +}
>  static BIN_ATTR(state_certificate, 0400, state_certificate_read, NULL, SDSI_SIZE_READ_MSG);
>  
> +static ssize_t
> +meter_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);
> +
> +	return certificate_read(SDSI_CMD_READ_METER, priv, buf, off, count);
> +}
> +static BIN_ATTR(meter_certificate, 0400, meter_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)
> @@ -472,6 +494,7 @@ 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_meter_certificate,
>  	&bin_attr_provision_akc,
>  	&bin_attr_provision_cap,
>  	NULL
> @@ -491,7 +514,16 @@ sdsi_battr_is_visible(struct kobject *kobj, struct bin_attribute *attr, int n)
>  	if (!(priv->features & SDSI_FEATURE_SDSI))
>  		return 0;
>  
> -	return attr->attr.mode;
> +	if (attr == &bin_attr_state_certificate ||
> +	    attr == &bin_attr_provision_akc ||
> +	    attr == &bin_attr_provision_cap)
> +		return attr->attr.mode;

I would prefer for you to just drop this and then change:

> +
> +	if (attr == &bin_attr_meter_certificate &&
> +	    !!(priv->features & SDSI_FEATURE_METERING))
> +		return attr->attr.mode;

to:

	if (attr == &bin_attr_meter_certificate)
		return (priv->features & SDSI_FEATURE_METERING) ?
				attr->attr.mode : 0;

And then keep:

	return attr->attr.mode;

at the end of this function, because now you are hiding all
new attributes by default and then you have to add any new
attributes to the if above, leading to an ever growing lists
of attr to check in that if, so just having:

	return attr->attr.mode;

As default for any non-matches attr would be much better IMHO.

Regards,

Hans





> +
> +	return 0;
>  }
>  
>  static ssize_t guid_show(struct device *dev, struct device_attribute *attr, char *buf)

[PATCH 5/9] tools/arch/x86: intel_sdsi: Add support for reading state certificates

[David E. Box]

Add option to read and decode On Demand state certificates.

Link: https://github.com/intel/intel-sdsi/blob/master/state-certificate-encoding.rst

Signed-off-by: David E. Box <david.e.box@linux.intel.com>
---
 tools/arch/x86/intel_sdsi/intel_sdsi.c | 268 ++++++++++++++++++-------
 1 file changed, 198 insertions(+), 70 deletions(-)

diff --git a/tools/arch/x86/intel_sdsi/intel_sdsi.c b/tools/arch/x86/intel_sdsi/intel_sdsi.c
index c0e2f2349db4..9dd94014a672 100644
--- a/tools/arch/x86/intel_sdsi/intel_sdsi.c
+++ b/tools/arch/x86/intel_sdsi/intel_sdsi.c
@@ -22,11 +22,24 @@
 
 #include <sys/types.h>
 
+#ifndef __packed
+#define __packed __attribute__((packed))
+#endif
+
+#define min(x, y) ({                            \
+	typeof(x) _min1 = (x);                  \
+	typeof(y) _min2 = (y);                  \
+	(void) (&_min1 == &_min2);              \
+	_min1 < _min2 ? _min1 : _min2; })
+
 #define SDSI_DEV		"intel_vsec.sdsi"
 #define AUX_DEV_PATH		"/sys/bus/auxiliary/devices/"
 #define SDSI_PATH		(AUX_DEV_DIR SDSI_DEV)
 #define GUID			0x6dd191
 #define REGISTERS_MIN_SIZE	72
+#define STATE_CERT_MAX_SIZE	4096
+#define STATE_MAX_NUM_LICENSES	16
+#define STATE_MAX_NUM_IN_BUNDLE	(uint32_t)8
 
 #define __round_mask(x, y) ((__typeof__(x))((y) - 1))
 #define round_up(x, y) ((((x) - 1) | __round_mask(x, y)) + 1)
@@ -49,6 +62,7 @@ struct availability {
 	uint64_t reserved:48;
 	uint64_t available:3;
 	uint64_t threshold:3;
+	uint64_t reserved2:10;
 };
 
 struct sdsi_regs {
@@ -63,17 +77,55 @@ struct sdsi_regs {
 	uint64_t socket_id;
 };
 
+#define CONTENT_TYPE_LK_ENC		0xD
+#define CONTENT_TYPE_LK_BLOB_ENC	0xE
+
+struct state_certificate {
+	uint32_t content_type;
+	uint32_t region_rev_id;
+	uint32_t header_size;
+	uint32_t total_size;
+	uint32_t key_size;
+	uint32_t num_licenses;
+};
+
+struct license_key_info {
+	uint32_t key_rev_id;
+	uint64_t key_image_content[6];
+} __packed;
+
+#define LICENSE_BLOB_SIZE(l)	(((l) & 0x7fffffff) * 4)
+#define LICENSE_VALID(l)	(!!((l) & 0x80000000))
+
+// License Group Types
+#define LBT_ONE_TIME_UPGRADE	1
+#define LBT_METERED_UPGRADE	2
+
+struct license_blob_content {
+	uint32_t type;
+	uint64_t id;
+	uint64_t ppin;
+	uint64_t previous_ppin;
+	uint32_t rev_id;
+	uint32_t num_bundles;
+} __packed;
+
+struct bundle_encoding {
+	uint32_t encoding;
+	uint32_t encoding_rsvd[7];
+};
+
 struct sdsi_dev {
 	struct sdsi_regs regs;
+	struct state_certificate sc;
 	char *dev_name;
 	char *dev_path;
 	int guid;
 };
 
 enum command {
-	CMD_NONE,
 	CMD_SOCKET_INFO,
-	CMD_DUMP_CERT,
+	CMD_STATE_CERT,
 	CMD_PROV_AKC,
 	CMD_PROV_CAP,
 };
@@ -168,20 +220,56 @@ static int sdsi_read_reg(struct sdsi_dev *s)
 	return 0;
 }
 
-static int sdsi_certificate_dump(struct sdsi_dev *s)
+static char *license_blob_type(uint32_t type)
+{
+	switch (type) {
+	case LBT_ONE_TIME_UPGRADE:
+		return "One time upgrade";
+	case LBT_METERED_UPGRADE:
+		return "Metered upgrade";
+	default:
+		return "Unknown license blob type";
+	}
+}
+
+static char *content_type(uint32_t type)
+{
+	switch (type) {
+	case  CONTENT_TYPE_LK_ENC:
+		return "Licencse key encoding";
+	case CONTENT_TYPE_LK_BLOB_ENC:
+		return "License key + Blob encoding";
+	default:
+		return "Unknown content type";
+	}
+}
+
+static void get_feature(uint32_t encoding, char *feature)
+{
+	char *name = (char *)&encoding;
+
+	feature[3] = name[0];
+	feature[2] = name[1];
+	feature[1] = name[2];
+	feature[0] = name[3];
+}
+
+static int sdsi_state_cert_show(struct sdsi_dev *s)
 {
-	uint64_t state_certificate[512] = {0};
-	bool first_instance;
-	uint64_t previous;
+	char buf[STATE_CERT_MAX_SIZE] = {0};
+	struct state_certificate *sc;
+	struct license_key_info *lki;
+	uint32_t offset = 0;
+	uint32_t count = 0;
 	FILE *cert_ptr;
-	int i, ret, size;
+	int ret, size;
 
 	ret = sdsi_update_registers(s);
 	if (ret)
 		return ret;
 
 	if (!s->regs.en_features.sdsi) {
-		fprintf(stderr, "SDSi feature is present but not enabled.");
+		fprintf(stderr, "On Demand feature is present but not enabled.");
 		fprintf(stderr, " Unable to read state certificate");
 		return -1;
 	}
@@ -198,32 +286,74 @@ static int sdsi_certificate_dump(struct sdsi_dev *s)
 		return -1;
 	}
 
-	size = fread(state_certificate, 1, sizeof(state_certificate), cert_ptr);
+	size = fread(buf, 1, sizeof(buf), cert_ptr);
 	if (!size) {
 		fprintf(stderr, "Could not read 'state_certificate' file\n");
 		fclose(cert_ptr);
 		return -1;
 	}
+	fclose(cert_ptr);
 
-	printf("%3d: 0x%lx\n", 0, state_certificate[0]);
-	previous = state_certificate[0];
-	first_instance = true;
+	sc = (struct state_certificate *)buf;
 
-	for (i = 1; i < (int)(round_up(size, sizeof(uint64_t))/sizeof(uint64_t)); i++) {
-		if (state_certificate[i] == previous) {
-			if (first_instance) {
-				puts("*");
-				first_instance = false;
-			}
-			continue;
+	/* Print register info for this guid */
+	printf("\n");
+	printf("State certificate for device %s\n", s->dev_name);
+	printf("\n");
+	printf("Content Type:          %s\n", content_type(sc->content_type));
+	printf("Region Revision ID:    %d\n", sc->region_rev_id);
+	printf("Header Size:           %d\n", sc->header_size * 4);
+	printf("Total Size:            %d\n", sc->total_size);
+	printf("OEM Key Size:          %d\n", sc->key_size * 4);
+	printf("Number of Licenses:    %d\n", sc->num_licenses);
+
+	/* Skip over the license sizes 4 bytes per license) to get the license key info */
+	lki = (void *)sc + sizeof(*sc) + (4 * sc->num_licenses);
+
+	printf("License blob Info:\n");
+	printf("    License Key Revision ID:    0x%x\n", lki->key_rev_id);
+	printf("    License Key Image Content:  0x%lx%lx%lx%lx%lx%lx\n",
+	       lki->key_image_content[5], lki->key_image_content[4],
+	       lki->key_image_content[3], lki->key_image_content[2],
+	       lki->key_image_content[1], lki->key_image_content[0]);
+
+	while (count++ < sc->num_licenses) {
+		uint32_t blob_size_field = *(uint32_t *)(buf + 0x14 + count * 4);
+		uint32_t blob_size = LICENSE_BLOB_SIZE(blob_size_field);
+		bool license_valid = LICENSE_VALID(blob_size_field);
+		struct license_blob_content *lbc =
+			(void *)(sc) +			// start of the state certificate
+			sizeof(*sc) +			// size of the state certificate
+			(4 * sc->num_licenses) +	// total size of the blob size blocks
+			sizeof(*lki) +			// size of the license key info
+			offset;				// offset to this blob content
+		struct bundle_encoding *bundle = (void *)(lbc) + sizeof(*lbc);
+		char feature[5];
+		uint32_t i;
+
+		printf("     Blob %d:\n", count - 1);
+		printf("        License blob size:          %u\n", blob_size);
+		printf("        License is valid:           %s\n", license_valid ? "Yes" : "No");
+		printf("        License blob type:          %s\n", license_blob_type(lbc->type));
+		printf("        License blob ID:            0x%lx\n", lbc->id);
+		printf("        PPIN:                       0x%lx\n", lbc->ppin);
+		printf("        Previous PPIN:              0x%lx\n", lbc->previous_ppin);
+		printf("        Blob revision ID:           %u\n", lbc->rev_id);
+		printf("        Number of Features:         %u\n", lbc->num_bundles);
+
+		feature[4] = '\0';
+
+		for (i = 0; i < min(lbc->num_bundles, STATE_MAX_NUM_IN_BUNDLE); i++) {
+			get_feature(bundle[i].encoding, feature);
+			printf("                 Feature %d:         %s\n", i, feature);
 		}
-		printf("%3d: 0x%lx\n", i, state_certificate[i]);
-		previous = state_certificate[i];
-		first_instance = true;
-	}
-	printf("%3d\n", i);
 
-	fclose(cert_ptr);
+		if (lbc->num_bundles > STATE_MAX_NUM_IN_BUNDLE)
+			fprintf(stderr, "        Warning: %d > %d licenses in bundle reported.\n",
+				lbc->num_bundles, STATE_MAX_NUM_IN_BUNDLE);
+
+		offset += blob_size;
+	};
 
 	return 0;
 }
@@ -231,7 +361,7 @@ static int sdsi_certificate_dump(struct sdsi_dev *s)
 static int sdsi_provision(struct sdsi_dev *s, char *bin_file, enum command command)
 {
 	int bin_fd, prov_fd, size, ret;
-	char buf[4096] = { 0 };
+	char buf[STATE_CERT_MAX_SIZE] = { 0 };
 	char cap[] = "provision_cap";
 	char akc[] = "provision_akc";
 	char *prov_file;
@@ -266,7 +396,7 @@ static int sdsi_provision(struct sdsi_dev *s, char *bin_file, enum command comma
 	}
 
 	/* Read the binary file into the buffer */
-	size = read(bin_fd, buf, 4096);
+	size = read(bin_fd, buf, STATE_CERT_MAX_SIZE);
 	if (size == -1) {
 		close(bin_fd);
 		close(prov_fd);
@@ -443,25 +573,26 @@ static void sdsi_free_dev(struct sdsi_dev *s)
 
 static void usage(char *prog)
 {
-	printf("Usage: %s [-l] [-d DEVNO [-iD] [-a FILE] [-c FILE]]\n", prog);
+	printf("Usage: %s [-l] [-d DEVNO [-i] [-s] [-a FILE] [-c FILE]]\n", prog);
 }
 
 static void show_help(void)
 {
 	printf("Commands:\n");
-	printf("  %-18s\t%s\n", "-l, --list",		"list available sdsi devices");
-	printf("  %-18s\t%s\n", "-d, --devno DEVNO",	"sdsi device number");
-	printf("  %-18s\t%s\n", "-i --info",		"show socket information");
-	printf("  %-18s\t%s\n", "-D --dump",		"dump state certificate data");
-	printf("  %-18s\t%s\n", "-a --akc FILE",	"provision socket with AKC FILE");
-	printf("  %-18s\t%s\n", "-c --cap FILE>",	"provision socket with CAP FILE");
+	printf("  %-18s\t%s\n", "-l, --list",           "list available On Demand devices");
+	printf("  %-18s\t%s\n", "-d, --devno DEVNO",    "On Demand device number");
+	printf("  %-18s\t%s\n", "-i, --info",           "show socket information");
+	printf("  %-18s\t%s\n", "-s, --state",          "show state certificate");
+	printf("  %-18s\t%s\n", "-a, --akc FILE",       "provision socket with AKC FILE");
+	printf("  %-18s\t%s\n", "-c, --cap FILE>",      "provision socket with CAP FILE");
 }
 
 int main(int argc, char *argv[])
 {
 	char bin_file[PATH_MAX], *dev_no = NULL;
+	bool device_selected = false;
 	char *progname;
-	enum command command = CMD_NONE;
+	enum command command = -1;
 	struct sdsi_dev *s;
 	int ret = 0, opt;
 	int option_index = 0;
@@ -470,21 +601,22 @@ int main(int argc, char *argv[])
 		{"akc",		required_argument,	0, 'a'},
 		{"cap",		required_argument,	0, 'c'},
 		{"devno",	required_argument,	0, 'd'},
-		{"dump",	no_argument,		0, 'D'},
 		{"help",	no_argument,		0, 'h'},
 		{"info",	no_argument,		0, 'i'},
 		{"list",	no_argument,		0, 'l'},
+		{"state",	no_argument,		0, 's'},
 		{0,		0,			0, 0 }
 	};
 
 
 	progname = argv[0];
 
-	while ((opt = getopt_long_only(argc, argv, "+a:c:d:Da:c:h", long_options,
+	while ((opt = getopt_long_only(argc, argv, "+a:c:d:hils", long_options,
 			&option_index)) != -1) {
 		switch (opt) {
 		case 'd':
 			dev_no = optarg;
+			device_selected = true;
 			break;
 		case 'l':
 			sdsi_list_devices();
@@ -492,8 +624,8 @@ int main(int argc, char *argv[])
 		case 'i':
 			command = CMD_SOCKET_INFO;
 			break;
-		case 'D':
-			command = CMD_DUMP_CERT;
+		case 's':
+			command = CMD_STATE_CERT;
 			break;
 		case 'a':
 		case 'c':
@@ -520,39 +652,35 @@ int main(int argc, char *argv[])
 		}
 	}
 
-	if (!dev_no) {
-		if (command != CMD_NONE)
-			fprintf(stderr, "Missing device number, DEVNO, for this command\n");
-		usage(progname);
-		return -1;
-	}
+	if (device_selected) {
+		s = sdsi_create_dev(dev_no);
+		if (!s)
+			return -1;
 
-	s = sdsi_create_dev(dev_no);
-	if (!s)
-		return -1;
+		switch (command) {
+		case CMD_SOCKET_INFO:
+			ret = sdsi_read_reg(s);
+			break;
+		case CMD_STATE_CERT:
+			ret = sdsi_state_cert_show(s);
+			break;
+		case CMD_PROV_AKC:
+			ret = sdsi_provision_akc(s, bin_file);
+			break;
+		case CMD_PROV_CAP:
+			ret = sdsi_provision_cap(s, bin_file);
+			break;
+		default:
+			fprintf(stderr, "No command specified\n");
+			return -1;
+		}
+
+		sdsi_free_dev(s);
 
-	/* Run the command */
-	switch (command) {
-	case CMD_NONE:
-		fprintf(stderr, "Missing command for device %s\n", dev_no);
-		usage(progname);
-		break;
-	case CMD_SOCKET_INFO:
-		ret = sdsi_read_reg(s);
-		break;
-	case CMD_DUMP_CERT:
-		ret = sdsi_certificate_dump(s);
-		break;
-	case CMD_PROV_AKC:
-		ret = sdsi_provision_akc(s, bin_file);
-		break;
-	case CMD_PROV_CAP:
-		ret = sdsi_provision_cap(s, bin_file);
-		break;
-	}
-
-
-	sdsi_free_dev(s);
+	} else {
+		fprintf(stderr, "No device specified\n");
+		return -1;
+	}
 
 	return ret;
 }
-- 
2.25.1

Re: [PATCH 5/9] tools/arch/x86: intel_sdsi: Add support for reading state certificates

[Hans de Goede]

Hi,

On 11/1/22 20:10, David E. Box wrote:
> Add option to read and decode On Demand state certificates.
> 
> Link: https://github.com/intel/intel-sdsi/blob/master/state-certificate-encoding.rst
> 
> Signed-off-by: David E. Box <david.e.box@linux.intel.com>

Thanks, patch looks good to me:

Reviewed-by: Hans de Goede <hdegoede@redhat.com>

Regards,

Hans


> ---
>  tools/arch/x86/intel_sdsi/intel_sdsi.c | 268 ++++++++++++++++++-------
>  1 file changed, 198 insertions(+), 70 deletions(-)
> 
> diff --git a/tools/arch/x86/intel_sdsi/intel_sdsi.c b/tools/arch/x86/intel_sdsi/intel_sdsi.c
> index c0e2f2349db4..9dd94014a672 100644
> --- a/tools/arch/x86/intel_sdsi/intel_sdsi.c
> +++ b/tools/arch/x86/intel_sdsi/intel_sdsi.c
> @@ -22,11 +22,24 @@
>  
>  #include <sys/types.h>
>  
> +#ifndef __packed
> +#define __packed __attribute__((packed))
> +#endif
> +
> +#define min(x, y) ({                            \
> +	typeof(x) _min1 = (x);                  \
> +	typeof(y) _min2 = (y);                  \
> +	(void) (&_min1 == &_min2);              \
> +	_min1 < _min2 ? _min1 : _min2; })
> +
>  #define SDSI_DEV		"intel_vsec.sdsi"
>  #define AUX_DEV_PATH		"/sys/bus/auxiliary/devices/"
>  #define SDSI_PATH		(AUX_DEV_DIR SDSI_DEV)
>  #define GUID			0x6dd191
>  #define REGISTERS_MIN_SIZE	72
> +#define STATE_CERT_MAX_SIZE	4096
> +#define STATE_MAX_NUM_LICENSES	16
> +#define STATE_MAX_NUM_IN_BUNDLE	(uint32_t)8
>  
>  #define __round_mask(x, y) ((__typeof__(x))((y) - 1))
>  #define round_up(x, y) ((((x) - 1) | __round_mask(x, y)) + 1)
> @@ -49,6 +62,7 @@ struct availability {
>  	uint64_t reserved:48;
>  	uint64_t available:3;
>  	uint64_t threshold:3;
> +	uint64_t reserved2:10;
>  };
>  
>  struct sdsi_regs {
> @@ -63,17 +77,55 @@ struct sdsi_regs {
>  	uint64_t socket_id;
>  };
>  
> +#define CONTENT_TYPE_LK_ENC		0xD
> +#define CONTENT_TYPE_LK_BLOB_ENC	0xE
> +
> +struct state_certificate {
> +	uint32_t content_type;
> +	uint32_t region_rev_id;
> +	uint32_t header_size;
> +	uint32_t total_size;
> +	uint32_t key_size;
> +	uint32_t num_licenses;
> +};
> +
> +struct license_key_info {
> +	uint32_t key_rev_id;
> +	uint64_t key_image_content[6];
> +} __packed;
> +
> +#define LICENSE_BLOB_SIZE(l)	(((l) & 0x7fffffff) * 4)
> +#define LICENSE_VALID(l)	(!!((l) & 0x80000000))
> +
> +// License Group Types
> +#define LBT_ONE_TIME_UPGRADE	1
> +#define LBT_METERED_UPGRADE	2
> +
> +struct license_blob_content {
> +	uint32_t type;
> +	uint64_t id;
> +	uint64_t ppin;
> +	uint64_t previous_ppin;
> +	uint32_t rev_id;
> +	uint32_t num_bundles;
> +} __packed;
> +
> +struct bundle_encoding {
> +	uint32_t encoding;
> +	uint32_t encoding_rsvd[7];
> +};
> +
>  struct sdsi_dev {
>  	struct sdsi_regs regs;
> +	struct state_certificate sc;
>  	char *dev_name;
>  	char *dev_path;
>  	int guid;
>  };
>  
>  enum command {
> -	CMD_NONE,
>  	CMD_SOCKET_INFO,
> -	CMD_DUMP_CERT,
> +	CMD_STATE_CERT,
>  	CMD_PROV_AKC,
>  	CMD_PROV_CAP,
>  };
> @@ -168,20 +220,56 @@ static int sdsi_read_reg(struct sdsi_dev *s)
>  	return 0;
>  }
>  
> -static int sdsi_certificate_dump(struct sdsi_dev *s)
> +static char *license_blob_type(uint32_t type)
> +{
> +	switch (type) {
> +	case LBT_ONE_TIME_UPGRADE:
> +		return "One time upgrade";
> +	case LBT_METERED_UPGRADE:
> +		return "Metered upgrade";
> +	default:
> +		return "Unknown license blob type";
> +	}
> +}
> +
> +static char *content_type(uint32_t type)
> +{
> +	switch (type) {
> +	case  CONTENT_TYPE_LK_ENC:
> +		return "Licencse key encoding";
> +	case CONTENT_TYPE_LK_BLOB_ENC:
> +		return "License key + Blob encoding";
> +	default:
> +		return "Unknown content type";
> +	}
> +}
> +
> +static void get_feature(uint32_t encoding, char *feature)
> +{
> +	char *name = (char *)&encoding;
> +
> +	feature[3] = name[0];
> +	feature[2] = name[1];
> +	feature[1] = name[2];
> +	feature[0] = name[3];
> +}
> +
> +static int sdsi_state_cert_show(struct sdsi_dev *s)
>  {
> -	uint64_t state_certificate[512] = {0};
> -	bool first_instance;
> -	uint64_t previous;
> +	char buf[STATE_CERT_MAX_SIZE] = {0};
> +	struct state_certificate *sc;
> +	struct license_key_info *lki;
> +	uint32_t offset = 0;
> +	uint32_t count = 0;
>  	FILE *cert_ptr;
> -	int i, ret, size;
> +	int ret, size;
>  
>  	ret = sdsi_update_registers(s);
>  	if (ret)
>  		return ret;
>  
>  	if (!s->regs.en_features.sdsi) {
> -		fprintf(stderr, "SDSi feature is present but not enabled.");
> +		fprintf(stderr, "On Demand feature is present but not enabled.");
>  		fprintf(stderr, " Unable to read state certificate");
>  		return -1;
>  	}
> @@ -198,32 +286,74 @@ static int sdsi_certificate_dump(struct sdsi_dev *s)
>  		return -1;
>  	}
>  
> -	size = fread(state_certificate, 1, sizeof(state_certificate), cert_ptr);
> +	size = fread(buf, 1, sizeof(buf), cert_ptr);
>  	if (!size) {
>  		fprintf(stderr, "Could not read 'state_certificate' file\n");
>  		fclose(cert_ptr);
>  		return -1;
>  	}
> +	fclose(cert_ptr);
>  
> -	printf("%3d: 0x%lx\n", 0, state_certificate[0]);
> -	previous = state_certificate[0];
> -	first_instance = true;
> +	sc = (struct state_certificate *)buf;
>  
> -	for (i = 1; i < (int)(round_up(size, sizeof(uint64_t))/sizeof(uint64_t)); i++) {
> -		if (state_certificate[i] == previous) {
> -			if (first_instance) {
> -				puts("*");
> -				first_instance = false;
> -			}
> -			continue;
> +	/* Print register info for this guid */
> +	printf("\n");
> +	printf("State certificate for device %s\n", s->dev_name);
> +	printf("\n");
> +	printf("Content Type:          %s\n", content_type(sc->content_type));
> +	printf("Region Revision ID:    %d\n", sc->region_rev_id);
> +	printf("Header Size:           %d\n", sc->header_size * 4);
> +	printf("Total Size:            %d\n", sc->total_size);
> +	printf("OEM Key Size:          %d\n", sc->key_size * 4);
> +	printf("Number of Licenses:    %d\n", sc->num_licenses);
> +
> +	/* Skip over the license sizes 4 bytes per license) to get the license key info */
> +	lki = (void *)sc + sizeof(*sc) + (4 * sc->num_licenses);
> +
> +	printf("License blob Info:\n");
> +	printf("    License Key Revision ID:    0x%x\n", lki->key_rev_id);
> +	printf("    License Key Image Content:  0x%lx%lx%lx%lx%lx%lx\n",
> +	       lki->key_image_content[5], lki->key_image_content[4],
> +	       lki->key_image_content[3], lki->key_image_content[2],
> +	       lki->key_image_content[1], lki->key_image_content[0]);
> +
> +	while (count++ < sc->num_licenses) {
> +		uint32_t blob_size_field = *(uint32_t *)(buf + 0x14 + count * 4);
> +		uint32_t blob_size = LICENSE_BLOB_SIZE(blob_size_field);
> +		bool license_valid = LICENSE_VALID(blob_size_field);
> +		struct license_blob_content *lbc =
> +			(void *)(sc) +			// start of the state certificate
> +			sizeof(*sc) +			// size of the state certificate
> +			(4 * sc->num_licenses) +	// total size of the blob size blocks
> +			sizeof(*lki) +			// size of the license key info
> +			offset;				// offset to this blob content
> +		struct bundle_encoding *bundle = (void *)(lbc) + sizeof(*lbc);
> +		char feature[5];
> +		uint32_t i;
> +
> +		printf("     Blob %d:\n", count - 1);
> +		printf("        License blob size:          %u\n", blob_size);
> +		printf("        License is valid:           %s\n", license_valid ? "Yes" : "No");
> +		printf("        License blob type:          %s\n", license_blob_type(lbc->type));
> +		printf("        License blob ID:            0x%lx\n", lbc->id);
> +		printf("        PPIN:                       0x%lx\n", lbc->ppin);
> +		printf("        Previous PPIN:              0x%lx\n", lbc->previous_ppin);
> +		printf("        Blob revision ID:           %u\n", lbc->rev_id);
> +		printf("        Number of Features:         %u\n", lbc->num_bundles);
> +
> +		feature[4] = '\0';
> +
> +		for (i = 0; i < min(lbc->num_bundles, STATE_MAX_NUM_IN_BUNDLE); i++) {
> +			get_feature(bundle[i].encoding, feature);
> +			printf("                 Feature %d:         %s\n", i, feature);
>  		}
> -		printf("%3d: 0x%lx\n", i, state_certificate[i]);
> -		previous = state_certificate[i];
> -		first_instance = true;
> -	}
> -	printf("%3d\n", i);
>  
> -	fclose(cert_ptr);
> +		if (lbc->num_bundles > STATE_MAX_NUM_IN_BUNDLE)
> +			fprintf(stderr, "        Warning: %d > %d licenses in bundle reported.\n",
> +				lbc->num_bundles, STATE_MAX_NUM_IN_BUNDLE);
> +
> +		offset += blob_size;
> +	};
>  
>  	return 0;
>  }
> @@ -231,7 +361,7 @@ static int sdsi_certificate_dump(struct sdsi_dev *s)
>  static int sdsi_provision(struct sdsi_dev *s, char *bin_file, enum command command)
>  {
>  	int bin_fd, prov_fd, size, ret;
> -	char buf[4096] = { 0 };
> +	char buf[STATE_CERT_MAX_SIZE] = { 0 };
>  	char cap[] = "provision_cap";
>  	char akc[] = "provision_akc";
>  	char *prov_file;
> @@ -266,7 +396,7 @@ static int sdsi_provision(struct sdsi_dev *s, char *bin_file, enum command comma
>  	}
>  
>  	/* Read the binary file into the buffer */
> -	size = read(bin_fd, buf, 4096);
> +	size = read(bin_fd, buf, STATE_CERT_MAX_SIZE);
>  	if (size == -1) {
>  		close(bin_fd);
>  		close(prov_fd);
> @@ -443,25 +573,26 @@ static void sdsi_free_dev(struct sdsi_dev *s)
>  
>  static void usage(char *prog)
>  {
> -	printf("Usage: %s [-l] [-d DEVNO [-iD] [-a FILE] [-c FILE]]\n", prog);
> +	printf("Usage: %s [-l] [-d DEVNO [-i] [-s] [-a FILE] [-c FILE]]\n", prog);
>  }
>  
>  static void show_help(void)
>  {
>  	printf("Commands:\n");
> -	printf("  %-18s\t%s\n", "-l, --list",		"list available sdsi devices");
> -	printf("  %-18s\t%s\n", "-d, --devno DEVNO",	"sdsi device number");
> -	printf("  %-18s\t%s\n", "-i --info",		"show socket information");
> -	printf("  %-18s\t%s\n", "-D --dump",		"dump state certificate data");
> -	printf("  %-18s\t%s\n", "-a --akc FILE",	"provision socket with AKC FILE");
> -	printf("  %-18s\t%s\n", "-c --cap FILE>",	"provision socket with CAP FILE");
> +	printf("  %-18s\t%s\n", "-l, --list",           "list available On Demand devices");
> +	printf("  %-18s\t%s\n", "-d, --devno DEVNO",    "On Demand device number");
> +	printf("  %-18s\t%s\n", "-i, --info",           "show socket information");
> +	printf("  %-18s\t%s\n", "-s, --state",          "show state certificate");
> +	printf("  %-18s\t%s\n", "-a, --akc FILE",       "provision socket with AKC FILE");
> +	printf("  %-18s\t%s\n", "-c, --cap FILE>",      "provision socket with CAP FILE");
>  }
>  
>  int main(int argc, char *argv[])
>  {
>  	char bin_file[PATH_MAX], *dev_no = NULL;
> +	bool device_selected = false;
>  	char *progname;
> -	enum command command = CMD_NONE;
> +	enum command command = -1;
>  	struct sdsi_dev *s;
>  	int ret = 0, opt;
>  	int option_index = 0;
> @@ -470,21 +601,22 @@ int main(int argc, char *argv[])
>  		{"akc",		required_argument,	0, 'a'},
>  		{"cap",		required_argument,	0, 'c'},
>  		{"devno",	required_argument,	0, 'd'},
> -		{"dump",	no_argument,		0, 'D'},
>  		{"help",	no_argument,		0, 'h'},
>  		{"info",	no_argument,		0, 'i'},
>  		{"list",	no_argument,		0, 'l'},
> +		{"state",	no_argument,		0, 's'},
>  		{0,		0,			0, 0 }
>  	};
>  
>  
>  	progname = argv[0];
>  
> -	while ((opt = getopt_long_only(argc, argv, "+a:c:d:Da:c:h", long_options,
> +	while ((opt = getopt_long_only(argc, argv, "+a:c:d:hils", long_options,
>  			&option_index)) != -1) {
>  		switch (opt) {
>  		case 'd':
>  			dev_no = optarg;
> +			device_selected = true;
>  			break;
>  		case 'l':
>  			sdsi_list_devices();
> @@ -492,8 +624,8 @@ int main(int argc, char *argv[])
>  		case 'i':
>  			command = CMD_SOCKET_INFO;
>  			break;
> -		case 'D':
> -			command = CMD_DUMP_CERT;
> +		case 's':
> +			command = CMD_STATE_CERT;
>  			break;
>  		case 'a':
>  		case 'c':
> @@ -520,39 +652,35 @@ int main(int argc, char *argv[])
>  		}
>  	}
>  
> -	if (!dev_no) {
> -		if (command != CMD_NONE)
> -			fprintf(stderr, "Missing device number, DEVNO, for this command\n");
> -		usage(progname);
> -		return -1;
> -	}
> +	if (device_selected) {
> +		s = sdsi_create_dev(dev_no);
> +		if (!s)
> +			return -1;
>  
> -	s = sdsi_create_dev(dev_no);
> -	if (!s)
> -		return -1;
> +		switch (command) {
> +		case CMD_SOCKET_INFO:
> +			ret = sdsi_read_reg(s);
> +			break;
> +		case CMD_STATE_CERT:
> +			ret = sdsi_state_cert_show(s);
> +			break;
> +		case CMD_PROV_AKC:
> +			ret = sdsi_provision_akc(s, bin_file);
> +			break;
> +		case CMD_PROV_CAP:
> +			ret = sdsi_provision_cap(s, bin_file);
> +			break;
> +		default:
> +			fprintf(stderr, "No command specified\n");
> +			return -1;
> +		}
> +
> +		sdsi_free_dev(s);
>  
> -	/* Run the command */
> -	switch (command) {
> -	case CMD_NONE:
> -		fprintf(stderr, "Missing command for device %s\n", dev_no);
> -		usage(progname);
> -		break;
> -	case CMD_SOCKET_INFO:
> -		ret = sdsi_read_reg(s);
> -		break;
> -	case CMD_DUMP_CERT:
> -		ret = sdsi_certificate_dump(s);
> -		break;
> -	case CMD_PROV_AKC:
> -		ret = sdsi_provision_akc(s, bin_file);
> -		break;
> -	case CMD_PROV_CAP:
> -		ret = sdsi_provision_cap(s, bin_file);
> -		break;
> -	}
> -
> -
> -	sdsi_free_dev(s);
> +	} else {
> +		fprintf(stderr, "No device specified\n");
> +		return -1;
> +	}
>  
>  	return ret;
>  }

[PATCH 6/9] tools/arch/x86: intel_sdsi: Add Intel On Demand text

[David E. Box]

Intel Software Defined Silicon (SDSi) is now officially known as Intel
On Demand. Change text in tool to indicate this.

Signed-off-by: David E. Box <david.e.box@linux.intel.com>
---
 tools/arch/x86/intel_sdsi/intel_sdsi.c | 12 ++++++------
 1 file changed, 6 insertions(+), 6 deletions(-)

diff --git a/tools/arch/x86/intel_sdsi/intel_sdsi.c b/tools/arch/x86/intel_sdsi/intel_sdsi.c
index 9dd94014a672..3718bd0c05cb 100644
--- a/tools/arch/x86/intel_sdsi/intel_sdsi.c
+++ b/tools/arch/x86/intel_sdsi/intel_sdsi.c
@@ -1,7 +1,7 @@
 // SPDX-License-Identifier: GPL-2.0
 /*
- * sdsi: Intel Software Defined Silicon tool for provisioning certificates
- * and activation payloads on supported cpus.
+ * sdsi: Intel On Demand (formerly Software Defined Silicon) tool for
+ * provisioning certificates and activation payloads on supported cpus.
  *
  * See https://github.com/intel/intel-sdsi/blob/master/os-interface.rst
  * for register descriptions.
@@ -150,7 +150,7 @@ static void sdsi_list_devices(void)
 	}
 
 	if (!found)
-		fprintf(stderr, "No sdsi devices found.\n");
+		fprintf(stderr, "No On Demand devices found.\n");
 }
 
 static int sdsi_update_registers(struct sdsi_dev *s)
@@ -206,7 +206,7 @@ static int sdsi_read_reg(struct sdsi_dev *s)
 	printf("\n");
 	printf("PPIN:                           0x%lx\n", s->regs.ppin);
 	printf("Enabled Features\n");
-	printf("    SDSi:                       %s\n", !!s->regs.en_features.sdsi ? "Enabled" : "Disabled");
+	printf("    On Demand:                  %s\n", !!s->regs.en_features.sdsi ? "Enabled" : "Disabled");
 	printf("Authorization Failure Count\n");
 	printf("    AKC Failure Count:          %d\n", s->regs.auth_fail_count.key_failure_count);
 	printf("    AKC Failure Threshold:      %d\n", s->regs.auth_fail_count.key_failure_threshold);
@@ -428,7 +428,7 @@ static int sdsi_provision_akc(struct sdsi_dev *s, char *bin_file)
 		return ret;
 
 	if (!s->regs.en_features.sdsi) {
-		fprintf(stderr, "SDSi feature is present but not enabled. Unable to provision");
+		fprintf(stderr, "On Demand feature is present but not enabled. Unable to provision");
 		return -1;
 	}
 
@@ -458,7 +458,7 @@ static int sdsi_provision_cap(struct sdsi_dev *s, char *bin_file)
 		return ret;
 
 	if (!s->regs.en_features.sdsi) {
-		fprintf(stderr, "SDSi feature is present but not enabled. Unable to provision");
+		fprintf(stderr, "On Demand feature is present but not enabled. Unable to provision");
 		return -1;
 	}
 
-- 
2.25.1

Re: [PATCH 6/9] tools/arch/x86: intel_sdsi: Add Intel On Demand text

[Hans de Goede]

Hi,

On 11/1/22 20:10, David E. Box wrote:
> Intel Software Defined Silicon (SDSi) is now officially known as Intel
> On Demand. Change text in tool to indicate this.
> 
> Signed-off-by: David E. Box <david.e.box@linux.intel.com>

Thanks, patch looks good to me:

Reviewed-by: Hans de Goede <hdegoede@redhat.com>

Regards,

Hans

> ---
>  tools/arch/x86/intel_sdsi/intel_sdsi.c | 12 ++++++------
>  1 file changed, 6 insertions(+), 6 deletions(-)
> 
> diff --git a/tools/arch/x86/intel_sdsi/intel_sdsi.c b/tools/arch/x86/intel_sdsi/intel_sdsi.c
> index 9dd94014a672..3718bd0c05cb 100644
> --- a/tools/arch/x86/intel_sdsi/intel_sdsi.c
> +++ b/tools/arch/x86/intel_sdsi/intel_sdsi.c
> @@ -1,7 +1,7 @@
>  // SPDX-License-Identifier: GPL-2.0
>  /*
> - * sdsi: Intel Software Defined Silicon tool for provisioning certificates
> - * and activation payloads on supported cpus.
> + * sdsi: Intel On Demand (formerly Software Defined Silicon) tool for
> + * provisioning certificates and activation payloads on supported cpus.
>   *
>   * See https://github.com/intel/intel-sdsi/blob/master/os-interface.rst
>   * for register descriptions.
> @@ -150,7 +150,7 @@ static void sdsi_list_devices(void)
>  	}
>  
>  	if (!found)
> -		fprintf(stderr, "No sdsi devices found.\n");
> +		fprintf(stderr, "No On Demand devices found.\n");
>  }
>  
>  static int sdsi_update_registers(struct sdsi_dev *s)
> @@ -206,7 +206,7 @@ static int sdsi_read_reg(struct sdsi_dev *s)
>  	printf("\n");
>  	printf("PPIN:                           0x%lx\n", s->regs.ppin);
>  	printf("Enabled Features\n");
> -	printf("    SDSi:                       %s\n", !!s->regs.en_features.sdsi ? "Enabled" : "Disabled");
> +	printf("    On Demand:                  %s\n", !!s->regs.en_features.sdsi ? "Enabled" : "Disabled");
>  	printf("Authorization Failure Count\n");
>  	printf("    AKC Failure Count:          %d\n", s->regs.auth_fail_count.key_failure_count);
>  	printf("    AKC Failure Threshold:      %d\n", s->regs.auth_fail_count.key_failure_threshold);
> @@ -428,7 +428,7 @@ static int sdsi_provision_akc(struct sdsi_dev *s, char *bin_file)
>  		return ret;
>  
>  	if (!s->regs.en_features.sdsi) {
> -		fprintf(stderr, "SDSi feature is present but not enabled. Unable to provision");
> +		fprintf(stderr, "On Demand feature is present but not enabled. Unable to provision");
>  		return -1;
>  	}
>  
> @@ -458,7 +458,7 @@ static int sdsi_provision_cap(struct sdsi_dev *s, char *bin_file)
>  		return ret;
>  
>  	if (!s->regs.en_features.sdsi) {
> -		fprintf(stderr, "SDSi feature is present but not enabled. Unable to provision");
> +		fprintf(stderr, "On Demand feature is present but not enabled. Unable to provision");
>  		return -1;
>  	}
>  

[PATCH 7/9] tools/arch/x86: intel_sdsi: Read more On Demand registers

[David E. Box]

Add decoding of the following On Demand register fields:

1. NVRAM content authorization error status
2. Enabled features: telemetry and attestation
3. Key provisioning status
4. NVRAM update limit
5. PCU_CR3_CAPID_CFG

Link: https://github.com/intel/intel-sdsi/blob/master/state-certificate-encoding.rst

Signed-off-by: David E. Box <david.e.box@linux.intel.com>
---
 tools/arch/x86/intel_sdsi/intel_sdsi.c | 50 +++++++++++++++++++++++---
 1 file changed, 45 insertions(+), 5 deletions(-)

diff --git a/tools/arch/x86/intel_sdsi/intel_sdsi.c b/tools/arch/x86/intel_sdsi/intel_sdsi.c
index 3718bd0c05cb..01b5f9994e11 100644
--- a/tools/arch/x86/intel_sdsi/intel_sdsi.c
+++ b/tools/arch/x86/intel_sdsi/intel_sdsi.c
@@ -44,10 +44,28 @@
 #define __round_mask(x, y) ((__typeof__(x))((y) - 1))
 #define round_up(x, y) ((((x) - 1) | __round_mask(x, y)) + 1)
 
+struct nvram_content_auth_err_sts {
+	uint64_t reserved:3;
+	uint64_t sdsi_content_auth_err:1;
+	uint64_t reserved1:1;
+	uint64_t sdsi_metering_auth_err:1;
+	uint64_t reserved2:58;
+};
+
 struct enabled_features {
 	uint64_t reserved:3;
 	uint64_t sdsi:1;
-	uint64_t reserved1:60;
+	uint64_t reserved1:8;
+	uint64_t attestation:1;
+	uint64_t reserved2:13;
+	uint64_t metering:1;
+	uint64_t reserved3:37;
+};
+
+struct key_provision_status {
+	uint64_t reserved:1;
+	uint64_t license_key_provisioned:1;
+	uint64_t reserved2:62;
 };
 
 struct auth_fail_count {
@@ -65,15 +83,23 @@ struct availability {
 	uint64_t reserved2:10;
 };
 
+struct nvram_update_limit {
+	uint64_t reserved:12;
+	uint64_t sdsi_50_pct:1;
+	uint64_t sdsi_75_pct:1;
+	uint64_t sdsi_90_pct:1;
+	uint64_t reserved2:49;
+};
+
 struct sdsi_regs {
 	uint64_t ppin;
-	uint64_t reserved;
+	struct nvram_content_auth_err_sts auth_err_sts;
 	struct enabled_features en_features;
-	uint64_t reserved1;
+	struct key_provision_status key_prov_sts;
 	struct auth_fail_count auth_fail_count;
 	struct availability prov_avail;
-	uint64_t reserved2;
-	uint64_t reserved3;
+	struct nvram_update_limit limits;
+	uint64_t pcu_cr3_capid_cfg;
 	uint64_t socket_id;
 };
 
@@ -205,8 +231,18 @@ static int sdsi_read_reg(struct sdsi_dev *s)
 	printf("Socket information for device %s\n", s->dev_name);
 	printf("\n");
 	printf("PPIN:                           0x%lx\n", s->regs.ppin);
+	printf("NVRAM Content Authorization Error Status\n");
+	printf("    SDSi Auth Err Sts:          %s\n", !!s->regs.auth_err_sts.sdsi_content_auth_err ? "Error" : "Okay");
+
+	if (!!s->regs.en_features.metering)
+		printf("    Metering Auth Err Sts:      %s\n", !!s->regs.auth_err_sts.sdsi_metering_auth_err ? "Error" : "Okay");
+
 	printf("Enabled Features\n");
 	printf("    On Demand:                  %s\n", !!s->regs.en_features.sdsi ? "Enabled" : "Disabled");
+	printf("    Attestation:                %s\n", !!s->regs.en_features.attestation ? "Enabled" : "Disabled");
+	printf("    On Demand:                  %s\n", !!s->regs.en_features.sdsi ? "Enabled" : "Disabled");
+	printf("    Metering:                   %s\n", !!s->regs.en_features.metering ? "Enabled" : "Disabled");
+	printf("License Key (AKC) Provisioned:  %s\n", !!s->regs.key_prov_sts.license_key_provisioned ? "Yes" : "No");
 	printf("Authorization Failure Count\n");
 	printf("    AKC Failure Count:          %d\n", s->regs.auth_fail_count.key_failure_count);
 	printf("    AKC Failure Threshold:      %d\n", s->regs.auth_fail_count.key_failure_threshold);
@@ -215,6 +251,10 @@ static int sdsi_read_reg(struct sdsi_dev *s)
 	printf("Provisioning Availability\n");
 	printf("    Updates Available:          %d\n", s->regs.prov_avail.available);
 	printf("    Updates Threshold:          %d\n", s->regs.prov_avail.threshold);
+	printf("NVRAM Udate Limit\n");
+	printf("    50%% Limit Reached:         %s\n", !!s->regs.limits.sdsi_50_pct ? "Yes" : "No");
+	printf("    75%% Limit Reached:         %s\n", !!s->regs.limits.sdsi_75_pct ? "Yes" : "No");
+	printf("    90%% Limit Reached:         %s\n", !!s->regs.limits.sdsi_90_pct ? "Yes" : "No");
 	printf("Socket ID:                      %ld\n", s->regs.socket_id & 0xF);
 
 	return 0;
-- 
2.25.1

Re: [PATCH 7/9] tools/arch/x86: intel_sdsi: Read more On Demand registers

[Hans de Goede]

Hi,

On 11/1/22 20:10, David E. Box wrote:
> Add decoding of the following On Demand register fields:
> 
> 1. NVRAM content authorization error status
> 2. Enabled features: telemetry and attestation
> 3. Key provisioning status
> 4. NVRAM update limit
> 5. PCU_CR3_CAPID_CFG
> 
> Link: https://github.com/intel/intel-sdsi/blob/master/state-certificate-encoding.rst
> 
> Signed-off-by: David E. Box <david.e.box@linux.intel.com>

Thanks, patch looks good to me:

Reviewed-by: Hans de Goede <hdegoede@redhat.com>

Regards,

Hans



> ---
>  tools/arch/x86/intel_sdsi/intel_sdsi.c | 50 +++++++++++++++++++++++---
>  1 file changed, 45 insertions(+), 5 deletions(-)
> 
> diff --git a/tools/arch/x86/intel_sdsi/intel_sdsi.c b/tools/arch/x86/intel_sdsi/intel_sdsi.c
> index 3718bd0c05cb..01b5f9994e11 100644
> --- a/tools/arch/x86/intel_sdsi/intel_sdsi.c
> +++ b/tools/arch/x86/intel_sdsi/intel_sdsi.c
> @@ -44,10 +44,28 @@
>  #define __round_mask(x, y) ((__typeof__(x))((y) - 1))
>  #define round_up(x, y) ((((x) - 1) | __round_mask(x, y)) + 1)
>  
> +struct nvram_content_auth_err_sts {
> +	uint64_t reserved:3;
> +	uint64_t sdsi_content_auth_err:1;
> +	uint64_t reserved1:1;
> +	uint64_t sdsi_metering_auth_err:1;
> +	uint64_t reserved2:58;
> +};
> +
>  struct enabled_features {
>  	uint64_t reserved:3;
>  	uint64_t sdsi:1;
> -	uint64_t reserved1:60;
> +	uint64_t reserved1:8;
> +	uint64_t attestation:1;
> +	uint64_t reserved2:13;
> +	uint64_t metering:1;
> +	uint64_t reserved3:37;
> +};
> +
> +struct key_provision_status {
> +	uint64_t reserved:1;
> +	uint64_t license_key_provisioned:1;
> +	uint64_t reserved2:62;
>  };
>  
>  struct auth_fail_count {
> @@ -65,15 +83,23 @@ struct availability {
>  	uint64_t reserved2:10;
>  };
>  
> +struct nvram_update_limit {
> +	uint64_t reserved:12;
> +	uint64_t sdsi_50_pct:1;
> +	uint64_t sdsi_75_pct:1;
> +	uint64_t sdsi_90_pct:1;
> +	uint64_t reserved2:49;
> +};
> +
>  struct sdsi_regs {
>  	uint64_t ppin;
> -	uint64_t reserved;
> +	struct nvram_content_auth_err_sts auth_err_sts;
>  	struct enabled_features en_features;
> -	uint64_t reserved1;
> +	struct key_provision_status key_prov_sts;
>  	struct auth_fail_count auth_fail_count;
>  	struct availability prov_avail;
> -	uint64_t reserved2;
> -	uint64_t reserved3;
> +	struct nvram_update_limit limits;
> +	uint64_t pcu_cr3_capid_cfg;
>  	uint64_t socket_id;
>  };
>  
> @@ -205,8 +231,18 @@ static int sdsi_read_reg(struct sdsi_dev *s)
>  	printf("Socket information for device %s\n", s->dev_name);
>  	printf("\n");
>  	printf("PPIN:                           0x%lx\n", s->regs.ppin);
> +	printf("NVRAM Content Authorization Error Status\n");
> +	printf("    SDSi Auth Err Sts:          %s\n", !!s->regs.auth_err_sts.sdsi_content_auth_err ? "Error" : "Okay");
> +
> +	if (!!s->regs.en_features.metering)
> +		printf("    Metering Auth Err Sts:      %s\n", !!s->regs.auth_err_sts.sdsi_metering_auth_err ? "Error" : "Okay");
> +
>  	printf("Enabled Features\n");
>  	printf("    On Demand:                  %s\n", !!s->regs.en_features.sdsi ? "Enabled" : "Disabled");
> +	printf("    Attestation:                %s\n", !!s->regs.en_features.attestation ? "Enabled" : "Disabled");
> +	printf("    On Demand:                  %s\n", !!s->regs.en_features.sdsi ? "Enabled" : "Disabled");
> +	printf("    Metering:                   %s\n", !!s->regs.en_features.metering ? "Enabled" : "Disabled");
> +	printf("License Key (AKC) Provisioned:  %s\n", !!s->regs.key_prov_sts.license_key_provisioned ? "Yes" : "No");
>  	printf("Authorization Failure Count\n");
>  	printf("    AKC Failure Count:          %d\n", s->regs.auth_fail_count.key_failure_count);
>  	printf("    AKC Failure Threshold:      %d\n", s->regs.auth_fail_count.key_failure_threshold);
> @@ -215,6 +251,10 @@ static int sdsi_read_reg(struct sdsi_dev *s)
>  	printf("Provisioning Availability\n");
>  	printf("    Updates Available:          %d\n", s->regs.prov_avail.available);
>  	printf("    Updates Threshold:          %d\n", s->regs.prov_avail.threshold);
> +	printf("NVRAM Udate Limit\n");
> +	printf("    50%% Limit Reached:         %s\n", !!s->regs.limits.sdsi_50_pct ? "Yes" : "No");
> +	printf("    75%% Limit Reached:         %s\n", !!s->regs.limits.sdsi_75_pct ? "Yes" : "No");
> +	printf("    90%% Limit Reached:         %s\n", !!s->regs.limits.sdsi_90_pct ? "Yes" : "No");
>  	printf("Socket ID:                      %ld\n", s->regs.socket_id & 0xF);
>  
>  	return 0;

[PATCH 8/9] tools/arch/x86: intel_sdsi: Add support for new GUID

[David E. Box]

The structure and content of the On Demand registers is based on the GUID
which is read from hardware through sysfs. Add support for decoding the
registers of a new GUID 0xF210D9EF.

Signed-off-by: David E. Box <david.e.box@linux.intel.com>
---
 tools/arch/x86/intel_sdsi/intel_sdsi.c | 32 ++++++++++++++++++--------
 1 file changed, 22 insertions(+), 10 deletions(-)

diff --git a/tools/arch/x86/intel_sdsi/intel_sdsi.c b/tools/arch/x86/intel_sdsi/intel_sdsi.c
index 01b5f9994e11..0680eda78b1a 100644
--- a/tools/arch/x86/intel_sdsi/intel_sdsi.c
+++ b/tools/arch/x86/intel_sdsi/intel_sdsi.c
@@ -35,7 +35,8 @@
 #define SDSI_DEV		"intel_vsec.sdsi"
 #define AUX_DEV_PATH		"/sys/bus/auxiliary/devices/"
 #define SDSI_PATH		(AUX_DEV_DIR SDSI_DEV)
-#define GUID			0x6dd191
+#define GUID_V1			0x6dd191
+#define GUID_V2			0xF210D9EF
 #define REGISTERS_MIN_SIZE	72
 #define STATE_CERT_MAX_SIZE	4096
 #define STATE_MAX_NUM_LICENSES	16
@@ -100,9 +101,17 @@ struct sdsi_regs {
 	struct availability prov_avail;
 	struct nvram_update_limit limits;
 	uint64_t pcu_cr3_capid_cfg;
-	uint64_t socket_id;
+	union {
+		struct {
+			uint64_t socket_id;
+		} v1;
+		struct {
+			uint64_t reserved;
+			uint64_t socket_id;
+			uint64_t reserved2;
+		} v2;
+	} extra;
 };
-
 #define CONTENT_TYPE_LK_ENC		0xD
 #define CONTENT_TYPE_LK_BLOB_ENC	0xE
 
@@ -146,7 +155,7 @@ struct sdsi_dev {
 	struct state_certificate sc;
 	char *dev_name;
 	char *dev_path;
-	int guid;
+	uint32_t guid;
 };
 
 enum command {
@@ -199,7 +208,7 @@ static int sdsi_update_registers(struct sdsi_dev *s)
 		return -1;
 	}
 
-	if (s->guid != GUID) {
+	if (s->guid != GUID_V1 && s->guid != GUID_V2) {
 		fprintf(stderr, "Unrecognized guid, 0x%x\n", s->guid);
 		fclose(regs_ptr);
 		return -1;
@@ -207,7 +216,7 @@ static int sdsi_update_registers(struct sdsi_dev *s)
 
 	/* Update register info for this guid */
 	ret = fread(&s->regs, sizeof(uint8_t), sizeof(s->regs), regs_ptr);
-	if (ret != sizeof(s->regs)) {
+	if (ret > (int)sizeof(s->regs)) { /* FIXME: Check size by guid */
 		fprintf(stderr, "Could not read 'registers' file\n");
 		fclose(regs_ptr);
 		return -1;
@@ -252,10 +261,13 @@ static int sdsi_read_reg(struct sdsi_dev *s)
 	printf("    Updates Available:          %d\n", s->regs.prov_avail.available);
 	printf("    Updates Threshold:          %d\n", s->regs.prov_avail.threshold);
 	printf("NVRAM Udate Limit\n");
-	printf("    50%% Limit Reached:         %s\n", !!s->regs.limits.sdsi_50_pct ? "Yes" : "No");
-	printf("    75%% Limit Reached:         %s\n", !!s->regs.limits.sdsi_75_pct ? "Yes" : "No");
-	printf("    90%% Limit Reached:         %s\n", !!s->regs.limits.sdsi_90_pct ? "Yes" : "No");
-	printf("Socket ID:                      %ld\n", s->regs.socket_id & 0xF);
+	printf("    50%% Limit Reached:          %s\n", !!s->regs.limits.sdsi_50_pct ? "Yes" : "No");
+	printf("    75%% Limit Reached:          %s\n", !!s->regs.limits.sdsi_75_pct ? "Yes" : "No");
+	printf("    90%% Limit Reached:          %s\n", !!s->regs.limits.sdsi_90_pct ? "Yes" : "No");
+	if (s->guid == GUID_V1)
+		printf("Socket ID:                      %ld\n", s->regs.extra.v1.socket_id & 0xF);
+	else
+		printf("Socket ID:                      %ld\n", s->regs.extra.v2.socket_id & 0xF);
 
 	return 0;
 }
-- 
2.25.1

Re: [PATCH 8/9] tools/arch/x86: intel_sdsi: Add support for new GUID

[Hans de Goede]

Hi,

On 11/1/22 20:10, David E. Box wrote:
> The structure and content of the On Demand registers is based on the GUID
> which is read from hardware through sysfs. Add support for decoding the
> registers of a new GUID 0xF210D9EF.
> 
> Signed-off-by: David E. Box <david.e.box@linux.intel.com>
> ---
>  tools/arch/x86/intel_sdsi/intel_sdsi.c | 32 ++++++++++++++++++--------
>  1 file changed, 22 insertions(+), 10 deletions(-)
> 
> diff --git a/tools/arch/x86/intel_sdsi/intel_sdsi.c b/tools/arch/x86/intel_sdsi/intel_sdsi.c
> index 01b5f9994e11..0680eda78b1a 100644
> --- a/tools/arch/x86/intel_sdsi/intel_sdsi.c
> +++ b/tools/arch/x86/intel_sdsi/intel_sdsi.c
> @@ -35,7 +35,8 @@
>  #define SDSI_DEV		"intel_vsec.sdsi"
>  #define AUX_DEV_PATH		"/sys/bus/auxiliary/devices/"
>  #define SDSI_PATH		(AUX_DEV_DIR SDSI_DEV)
> -#define GUID			0x6dd191
> +#define GUID_V1			0x6dd191
> +#define GUID_V2			0xF210D9EF
>  #define REGISTERS_MIN_SIZE	72
>  #define STATE_CERT_MAX_SIZE	4096
>  #define STATE_MAX_NUM_LICENSES	16
> @@ -100,9 +101,17 @@ struct sdsi_regs {
>  	struct availability prov_avail;
>  	struct nvram_update_limit limits;
>  	uint64_t pcu_cr3_capid_cfg;
> -	uint64_t socket_id;
> +	union {
> +		struct {
> +			uint64_t socket_id;
> +		} v1;
> +		struct {
> +			uint64_t reserved;
> +			uint64_t socket_id;
> +			uint64_t reserved2;
> +		} v2;
> +	} extra;
>  };
> -
>  #define CONTENT_TYPE_LK_ENC		0xD
>  #define CONTENT_TYPE_LK_BLOB_ENC	0xE
>  
> @@ -146,7 +155,7 @@ struct sdsi_dev {
>  	struct state_certificate sc;
>  	char *dev_name;
>  	char *dev_path;
> -	int guid;
> +	uint32_t guid;
>  };
>  
>  enum command {
> @@ -199,7 +208,7 @@ static int sdsi_update_registers(struct sdsi_dev *s)
>  		return -1;
>  	}
>  
> -	if (s->guid != GUID) {
> +	if (s->guid != GUID_V1 && s->guid != GUID_V2) {
>  		fprintf(stderr, "Unrecognized guid, 0x%x\n", s->guid);
>  		fclose(regs_ptr);
>  		return -1;
> @@ -207,7 +216,7 @@ static int sdsi_update_registers(struct sdsi_dev *s)
>  
>  	/* Update register info for this guid */
>  	ret = fread(&s->regs, sizeof(uint8_t), sizeof(s->regs), regs_ptr);
> -	if (ret != sizeof(s->regs)) {
> +	if (ret > (int)sizeof(s->regs)) { /* FIXME: Check size by guid */

This is wrong, fread will never return more then requested, that
would lead to buffer overflows. But it may return 0 on errors, or
a short read on an error.

So you need to fix the FIXME comment you added here as now you
have just disabled all error checking.

Regards,

Hans



>  		fprintf(stderr, "Could not read 'registers' file\n");
>  		fclose(regs_ptr);
>  		return -1;
> @@ -252,10 +261,13 @@ static int sdsi_read_reg(struct sdsi_dev *s)
>  	printf("    Updates Available:          %d\n", s->regs.prov_avail.available);
>  	printf("    Updates Threshold:          %d\n", s->regs.prov_avail.threshold);
>  	printf("NVRAM Udate Limit\n");
> -	printf("    50%% Limit Reached:         %s\n", !!s->regs.limits.sdsi_50_pct ? "Yes" : "No");
> -	printf("    75%% Limit Reached:         %s\n", !!s->regs.limits.sdsi_75_pct ? "Yes" : "No");
> -	printf("    90%% Limit Reached:         %s\n", !!s->regs.limits.sdsi_90_pct ? "Yes" : "No");
> -	printf("Socket ID:                      %ld\n", s->regs.socket_id & 0xF);
> +	printf("    50%% Limit Reached:          %s\n", !!s->regs.limits.sdsi_50_pct ? "Yes" : "No");
> +	printf("    75%% Limit Reached:          %s\n", !!s->regs.limits.sdsi_75_pct ? "Yes" : "No");
> +	printf("    90%% Limit Reached:          %s\n", !!s->regs.limits.sdsi_90_pct ? "Yes" : "No");
> +	if (s->guid == GUID_V1)
> +		printf("Socket ID:                      %ld\n", s->regs.extra.v1.socket_id & 0xF);
> +	else
> +		printf("Socket ID:                      %ld\n", s->regs.extra.v2.socket_id & 0xF);
>  
>  	return 0;
>  }

[PATCH 9/9] tools/arch/x86: intel_sdsi: Add support for reading meter certificates

[David E. Box]

Add option to read and decode On Demand meter certificates.

Link: https://github.com/intel/intel-sdsi/blob/master/meter-certificate.rst

Signed-off-by: David E. Box <david.e.box@linux.intel.com>
---
 tools/arch/x86/intel_sdsi/intel_sdsi.c | 110 ++++++++++++++++++++++++-
 1 file changed, 107 insertions(+), 3 deletions(-)

diff --git a/tools/arch/x86/intel_sdsi/intel_sdsi.c b/tools/arch/x86/intel_sdsi/intel_sdsi.c
index 0680eda78b1a..ebf076ee6ef8 100644
--- a/tools/arch/x86/intel_sdsi/intel_sdsi.c
+++ b/tools/arch/x86/intel_sdsi/intel_sdsi.c
@@ -39,8 +39,10 @@
 #define GUID_V2			0xF210D9EF
 #define REGISTERS_MIN_SIZE	72
 #define STATE_CERT_MAX_SIZE	4096
+#define METER_CERT_MAX_SIZE	4096
 #define STATE_MAX_NUM_LICENSES	16
 #define STATE_MAX_NUM_IN_BUNDLE	(uint32_t)8
+#define METER_MAX_NUM_BUNDLES	8
 
 #define __round_mask(x, y) ((__typeof__(x))((y) - 1))
 #define round_up(x, y) ((((x) - 1) | __round_mask(x, y)) + 1)
@@ -150,6 +152,21 @@ struct bundle_encoding {
 	uint32_t encoding_rsvd[7];
 };
 
+struct meter_certificate {
+	uint32_t block_signature;
+	uint32_t counter_unit;
+	uint64_t ppin;
+	uint32_t bundle_length;
+	uint32_t reserved;
+	uint32_t mmrc_encoding;
+	uint32_t mmrc_counter;
+};
+
+struct bundle_encoding_counter {
+	uint32_t encoding;
+	uint32_t counter;
+};
+
 struct sdsi_dev {
 	struct sdsi_regs regs;
 	struct state_certificate sc;
@@ -160,6 +177,7 @@ struct sdsi_dev {
 
 enum command {
 	CMD_SOCKET_INFO,
+	CMD_METER_CERT,
 	CMD_STATE_CERT,
 	CMD_PROV_AKC,
 	CMD_PROV_CAP,
@@ -306,6 +324,86 @@ static void get_feature(uint32_t encoding, char *feature)
 	feature[0] = name[3];
 }
 
+static int sdsi_meter_cert_show(struct sdsi_dev *s)
+{
+	char buf[METER_CERT_MAX_SIZE] = {0};
+	struct bundle_encoding_counter *bec;
+	struct meter_certificate *mc;
+	uint32_t count = 0;
+	FILE *cert_ptr;
+	int ret, size;
+
+	ret = sdsi_update_registers(s);
+	if (ret)
+		return ret;
+
+	if (!s->regs.en_features.sdsi) {
+		fprintf(stderr, "SDSi feature is present but not enabled.\n");
+		fprintf(stderr, " Unable to read meter certificate\n");
+		return -1;
+	}
+
+	if (!s->regs.en_features.metering) {
+		fprintf(stderr, "Metering not supporting on this socket.\n");
+		return -1;
+	}
+
+	ret = chdir(s->dev_path);
+	if (ret == -1) {
+		perror("chdir");
+		return ret;
+	}
+
+	cert_ptr = fopen("meter_certificate", "r");
+	if (!cert_ptr) {
+		perror("Could not open 'meter_certificate' file");
+		return -1;
+	}
+
+	size = fread(buf, 1, sizeof(buf), cert_ptr);
+	if (!size) {
+		fprintf(stderr, "Could not read 'meter_certificate' file\n");
+		fclose(cert_ptr);
+		return -1;
+	}
+	fclose(cert_ptr);
+
+	mc = (struct meter_certificate *)buf;
+
+	printf("\n");
+	printf("Meter certificate for device %s\n", s->dev_name);
+	printf("\n");
+	printf("Block Signature:       0x%x\n", mc->block_signature);
+	printf("Count Unit:            %dms\n", mc->counter_unit);
+	printf("PPIN:                  0x%lx\n", mc->ppin);
+	printf("Feature Bundle Length: %d\n", mc->bundle_length);
+	printf("MMRC encoding:         %d\n", mc->mmrc_encoding);
+	printf("MMRC counter:          %d\n", mc->mmrc_counter);
+	if (mc->bundle_length % 8) {
+		fprintf(stderr, "Invalid bundle length\n");
+		return -1;
+	}
+
+	if (mc->bundle_length > METER_MAX_NUM_BUNDLES * 8)  {
+		fprintf(stderr, "More the %d bundles: %d\n",
+			METER_MAX_NUM_BUNDLES, mc->bundle_length / 8);
+		return -1;
+	}
+
+	bec = (void *)(mc) + sizeof(mc);
+
+	printf("Number of Feature Counters:          %d\n", mc->bundle_length / 8);
+	while (count++ < mc->bundle_length / 8) {
+		char feature[5];
+
+		feature[4] = '\0';
+		get_feature(bec[count].encoding, feature);
+		printf("    %s:          %d\n", feature, bec[count].counter);
+	}
+
+	return 0;
+}
+
 static int sdsi_state_cert_show(struct sdsi_dev *s)
 {
 	char buf[STATE_CERT_MAX_SIZE] = {0};
@@ -625,7 +723,7 @@ static void sdsi_free_dev(struct sdsi_dev *s)
 
 static void usage(char *prog)
 {
-	printf("Usage: %s [-l] [-d DEVNO [-i] [-s] [-a FILE] [-c FILE]]\n", prog);
+	printf("Usage: %s [-l] [-d DEVNO [-i] [-s] [-m] [-a FILE] [-c FILE]]\n", prog);
 }
 
 static void show_help(void)
@@ -635,6 +733,7 @@ static void show_help(void)
 	printf("  %-18s\t%s\n", "-d, --devno DEVNO",    "On Demand device number");
 	printf("  %-18s\t%s\n", "-i, --info",           "show socket information");
 	printf("  %-18s\t%s\n", "-s, --state",          "show state certificate");
+	printf("  %-18s\t%s\n", "-m, --meter",          "show meter certificate");
 	printf("  %-18s\t%s\n", "-a, --akc FILE",       "provision socket with AKC FILE");
 	printf("  %-18s\t%s\n", "-c, --cap FILE>",      "provision socket with CAP FILE");
 }
@@ -656,6 +755,7 @@ int main(int argc, char *argv[])
 		{"help",	no_argument,		0, 'h'},
 		{"info",	no_argument,		0, 'i'},
 		{"list",	no_argument,		0, 'l'},
+		{"meter",	no_argument,		0, 'm'},
 		{"state",	no_argument,		0, 's'},
 		{0,		0,			0, 0 }
 	};
@@ -663,7 +763,7 @@ int main(int argc, char *argv[])
 
 	progname = argv[0];
 
-	while ((opt = getopt_long_only(argc, argv, "+a:c:d:hils", long_options,
+	while ((opt = getopt_long_only(argc, argv, "+a:c:d:hilms", long_options,
 			&option_index)) != -1) {
 		switch (opt) {
 		case 'd':
@@ -676,8 +776,9 @@ int main(int argc, char *argv[])
 		case 'i':
 			command = CMD_SOCKET_INFO;
 			break;
+		case 'm':
 		case 's':
-			command = CMD_STATE_CERT;
+			command = (opt == 'm') ? CMD_METER_CERT : CMD_STATE_CERT;
 			break;
 		case 'a':
 		case 'c':
@@ -713,6 +814,9 @@ int main(int argc, char *argv[])
 		case CMD_SOCKET_INFO:
 			ret = sdsi_read_reg(s);
 			break;
+		case CMD_METER_CERT:
+			ret = sdsi_meter_cert_show(s);
+			break;
 		case CMD_STATE_CERT:
 			ret = sdsi_state_cert_show(s);
 			break;
-- 
2.25.1

Re: [PATCH 9/9] tools/arch/x86: intel_sdsi: Add support for reading meter certificates

[Hans de Goede]

Hi,

On 11/1/22 20:10, David E. Box wrote:
> Add option to read and decode On Demand meter certificates.
> 
> Link: https://github.com/intel/intel-sdsi/blob/master/meter-certificate.rst
> 
> Signed-off-by: David E. Box <david.e.box@linux.intel.com>
> ---
>  tools/arch/x86/intel_sdsi/intel_sdsi.c | 110 ++++++++++++++++++++++++-
>  1 file changed, 107 insertions(+), 3 deletions(-)
> 
> diff --git a/tools/arch/x86/intel_sdsi/intel_sdsi.c b/tools/arch/x86/intel_sdsi/intel_sdsi.c
> index 0680eda78b1a..ebf076ee6ef8 100644
> --- a/tools/arch/x86/intel_sdsi/intel_sdsi.c
> +++ b/tools/arch/x86/intel_sdsi/intel_sdsi.c
> @@ -39,8 +39,10 @@
>  #define GUID_V2			0xF210D9EF
>  #define REGISTERS_MIN_SIZE	72
>  #define STATE_CERT_MAX_SIZE	4096
> +#define METER_CERT_MAX_SIZE	4096
>  #define STATE_MAX_NUM_LICENSES	16
>  #define STATE_MAX_NUM_IN_BUNDLE	(uint32_t)8
> +#define METER_MAX_NUM_BUNDLES	8
>  
>  #define __round_mask(x, y) ((__typeof__(x))((y) - 1))
>  #define round_up(x, y) ((((x) - 1) | __round_mask(x, y)) + 1)
> @@ -150,6 +152,21 @@ struct bundle_encoding {
>  	uint32_t encoding_rsvd[7];
>  };
>  
> +struct meter_certificate {
> +	uint32_t block_signature;
> +	uint32_t counter_unit;
> +	uint64_t ppin;
> +	uint32_t bundle_length;
> +	uint32_t reserved;
> +	uint32_t mmrc_encoding;
> +	uint32_t mmrc_counter;
> +};
> +
> +struct bundle_encoding_counter {
> +	uint32_t encoding;
> +	uint32_t counter;
> +};
> +
>  struct sdsi_dev {
>  	struct sdsi_regs regs;
>  	struct state_certificate sc;
> @@ -160,6 +177,7 @@ struct sdsi_dev {
>  
>  enum command {
>  	CMD_SOCKET_INFO,
> +	CMD_METER_CERT,
>  	CMD_STATE_CERT,
>  	CMD_PROV_AKC,
>  	CMD_PROV_CAP,
> @@ -306,6 +324,86 @@ static void get_feature(uint32_t encoding, char *feature)
>  	feature[0] = name[3];
>  }
>  
> +static int sdsi_meter_cert_show(struct sdsi_dev *s)
> +{
> +	char buf[METER_CERT_MAX_SIZE] = {0};
> +	struct bundle_encoding_counter *bec;
> +	struct meter_certificate *mc;
> +	uint32_t count = 0;
> +	FILE *cert_ptr;
> +	int ret, size;
> +
> +	ret = sdsi_update_registers(s);
> +	if (ret)
> +		return ret;
> +
> +	if (!s->regs.en_features.sdsi) {
> +		fprintf(stderr, "SDSi feature is present but not enabled.\n");
> +		fprintf(stderr, " Unable to read meter certificate\n");
> +		return -1;
> +	}
> +
> +	if (!s->regs.en_features.metering) {
> +		fprintf(stderr, "Metering not supporting on this socket.\n");
> +		return -1;
> +	}
> +
> +	ret = chdir(s->dev_path);
> +	if (ret == -1) {
> +		perror("chdir");
> +		return ret;
> +	}
> +
> +	cert_ptr = fopen("meter_certificate", "r");
> +	if (!cert_ptr) {
> +		perror("Could not open 'meter_certificate' file");
> +		return -1;
> +	}
> +
> +	size = fread(buf, 1, sizeof(buf), cert_ptr);
> +	if (!size) {
> +		fprintf(stderr, "Could not read 'meter_certificate' file\n");
> +		fclose(cert_ptr);
> +		return -1;
> +	}
> +	fclose(cert_ptr);
> +
> +	mc = (struct meter_certificate *)buf;
> +
> +	printf("\n");
> +	printf("Meter certificate for device %s\n", s->dev_name);
> +	printf("\n");
> +	printf("Block Signature:       0x%x\n", mc->block_signature);
> +	printf("Count Unit:            %dms\n", mc->counter_unit);
> +	printf("PPIN:                  0x%lx\n", mc->ppin);
> +	printf("Feature Bundle Length: %d\n", mc->bundle_length);
> +	printf("MMRC encoding:         %d\n", mc->mmrc_encoding);
> +	printf("MMRC counter:          %d\n", mc->mmrc_counter);
> +	if (mc->bundle_length % 8) {
> +		fprintf(stderr, "Invalid bundle length\n");
> +		return -1;
> +	}
> +
> +	if (mc->bundle_length > METER_MAX_NUM_BUNDLES * 8)  {
> +		fprintf(stderr, "More the %d bundles: %d\n",
> +			METER_MAX_NUM_BUNDLES, mc->bundle_length / 8);
> +		return -1;
> +	}
> +
> +	bec = (void *)(mc) + sizeof(mc);
> +
> +	printf("Number of Feature Counters:          %d\n", mc->bundle_length / 8);
> +	while (count++ < mc->bundle_length / 8) {
> +		char feature[5];
> +
> +		feature[4] = '\0';
> +		get_feature(bec[count].encoding, feature);
> +		printf("    %s:          %d\n", feature, bec[count].counter);
> +	}
> +
> +	return 0;
> +}
> +
>  static int sdsi_state_cert_show(struct sdsi_dev *s)
>  {
>  	char buf[STATE_CERT_MAX_SIZE] = {0};
> @@ -625,7 +723,7 @@ static void sdsi_free_dev(struct sdsi_dev *s)
>  
>  static void usage(char *prog)
>  {
> -	printf("Usage: %s [-l] [-d DEVNO [-i] [-s] [-a FILE] [-c FILE]]\n", prog);
> +	printf("Usage: %s [-l] [-d DEVNO [-i] [-s] [-m] [-a FILE] [-c FILE]]\n", prog);
>  }
>  
>  static void show_help(void)
> @@ -635,6 +733,7 @@ static void show_help(void)
>  	printf("  %-18s\t%s\n", "-d, --devno DEVNO",    "On Demand device number");
>  	printf("  %-18s\t%s\n", "-i, --info",           "show socket information");
>  	printf("  %-18s\t%s\n", "-s, --state",          "show state certificate");
> +	printf("  %-18s\t%s\n", "-m, --meter",          "show meter certificate");
>  	printf("  %-18s\t%s\n", "-a, --akc FILE",       "provision socket with AKC FILE");
>  	printf("  %-18s\t%s\n", "-c, --cap FILE>",      "provision socket with CAP FILE");
>  }
> @@ -656,6 +755,7 @@ int main(int argc, char *argv[])
>  		{"help",	no_argument,		0, 'h'},
>  		{"info",	no_argument,		0, 'i'},
>  		{"list",	no_argument,		0, 'l'},
> +		{"meter",	no_argument,		0, 'm'},
>  		{"state",	no_argument,		0, 's'},
>  		{0,		0,			0, 0 }
>  	};
> @@ -663,7 +763,7 @@ int main(int argc, char *argv[])
>  
>  	progname = argv[0];
>  
> -	while ((opt = getopt_long_only(argc, argv, "+a:c:d:hils", long_options,
> +	while ((opt = getopt_long_only(argc, argv, "+a:c:d:hilms", long_options,
>  			&option_index)) != -1) {
>  		switch (opt) {
>  		case 'd':
> @@ -676,8 +776,9 @@ int main(int argc, char *argv[])
>  		case 'i':
>  			command = CMD_SOCKET_INFO;
>  			break;
> +		case 'm':
>  		case 's':
> -			command = CMD_STATE_CERT;
> +			command = (opt == 'm') ? CMD_METER_CERT : CMD_STATE_CERT;
>  			break;

please just make this 2 separate cases rather then testing opt after
just having done a switch-case on opt.

Other then that this looks good to me, so with that fixed:

Reviewed-by: Hans de Goede <hdegoede@redhat.com>

Regards,

Hans


>  		case 'a':
>  		case 'c':
> @@ -713,6 +814,9 @@ int main(int argc, char *argv[])
>  		case CMD_SOCKET_INFO:
>  			ret = sdsi_read_reg(s);
>  			break;
> +		case CMD_METER_CERT:
> +			ret = sdsi_meter_cert_show(s);
> +			break;
>  		case CMD_STATE_CERT:
>  			ret = sdsi_state_cert_show(s);
>  			break;

Re: [PATCH 0/9] Extend Intel On Demand (SDSi) support

[Hans de Goede]

Hi David,

On 11/1/22 20:10, David E. Box wrote:
> Intel Software Defined Silicon (SDSi) is now known as Intel On Demand. The
> following patches do the following:
> 
> 1. Identify the driver/tools as Intel On Demand. Only text descriptions are
> changed. Kconfig and filenames remain the same.
> 2. Perform some attribute cleanup by preventing the showing of files when
> features are not supported.
> 3. Adds support for a new GUID. GUIDs are used to identify the layout of
> the On Demand registers in sysfs. Layouts are described in the
> documentation on github [1].
> 4. Add support for reading On Demand meter certificates in sysfs.
> 5. The rest of the patches modify the existing tool to support discovery
> and reading of On Demand registers and the meter certificate.
> 
> [1] https://github.com/intel/intel-sdsi/blob/master/os-interface.rst

Sorry for the long silence, I have not done any pdx86 patch review
the last 2 weeks due to personal circumstances.

I will try to get this reviewed at the end of this week or the beginning
of next week.

Regards,

Hans




> 
> David E. Box (9):
>   platform/x86/intel/sdsi: Add Intel On Demand text
>   platform/x86/intel/sdsi: Hide attributes if hardware doesn't support
>   platform/x86/intel/sdsi: Support different GUIDs
>   platform/x86/intel/sdsi: Add meter certificate support
>   tools/arch/x86: intel_sdsi: Add support for reading state certificates
>   tools/arch/x86: intel_sdsi: Add Intel On Demand text
>   tools/arch/x86: intel_sdsi: Read more On Demand registers
>   tools/arch/x86: intel_sdsi: Add support for new GUID
>   tools/arch/x86: intel_sdsi: Add support for reading meter certificates
> 
>  .../ABI/testing/sysfs-driver-intel_sdsi       |  47 +-
>  drivers/platform/x86/intel/Kconfig            |   8 +-
>  drivers/platform/x86/intel/sdsi.c             | 134 ++++-
>  tools/arch/x86/intel_sdsi/intel_sdsi.c        | 458 ++++++++++++++----
>  4 files changed, 513 insertions(+), 134 deletions(-)
> 
> 
> base-commit: 225469d4acbcb873358d7618bad6e0203b67b964

Re: [PATCH 0/9] Extend Intel On Demand (SDSi) support

[Hans de Goede]

Hi David,

On 11/1/22 20:10, David E. Box wrote:
> Intel Software Defined Silicon (SDSi) is now known as Intel On Demand. The
> following patches do the following:
> 
> 1. Identify the driver/tools as Intel On Demand. Only text descriptions are
> changed. Kconfig and filenames remain the same.
> 2. Perform some attribute cleanup by preventing the showing of files when
> features are not supported.
> 3. Adds support for a new GUID. GUIDs are used to identify the layout of
> the On Demand registers in sysfs. Layouts are described in the
> documentation on github [1].
> 4. Add support for reading On Demand meter certificates in sysfs.
> 5. The rest of the patches modify the existing tool to support discovery
> and reading of On Demand registers and the meter certificate.
> 
> [1] https://github.com/intel/intel-sdsi/blob/master/os-interface.rst
> 
> David E. Box (9):
>   platform/x86/intel/sdsi: Add Intel On Demand text
>   platform/x86/intel/sdsi: Hide attributes if hardware doesn't support
>   platform/x86/intel/sdsi: Support different GUIDs
>   platform/x86/intel/sdsi: Add meter certificate support
>   tools/arch/x86: intel_sdsi: Add support for reading state certificates
>   tools/arch/x86: intel_sdsi: Add Intel On Demand text
>   tools/arch/x86: intel_sdsi: Read more On Demand registers
>   tools/arch/x86: intel_sdsi: Add support for new GUID
>   tools/arch/x86: intel_sdsi: Add support for reading meter certificates

Thank you, over all this looks good. I have some small remarks
on patches 4, 8 and 9 see my replies to those.

Please prepare a v2 addressing Andy's + my review remarks and get
that v2 to me no later then next week Tuesday, then I can still
merge this in time for 6.2 .

Regards,

Hans

Re: [PATCH 0/9] Extend Intel On Demand (SDSi) support

[David E. Box]

On Thu, 2022-11-17 at 15:01 +0100, Hans de Goede wrote:
> Hi David,
> 
> On 11/1/22 20:10, David E. Box wrote:
> > Intel Software Defined Silicon (SDSi) is now known as Intel On Demand. The
> > following patches do the following:
> > 
> > 1. Identify the driver/tools as Intel On Demand. Only text descriptions are
> > changed. Kconfig and filenames remain the same.
> > 2. Perform some attribute cleanup by preventing the showing of files when
> > features are not supported.
> > 3. Adds support for a new GUID. GUIDs are used to identify the layout of
> > the On Demand registers in sysfs. Layouts are described in the
> > documentation on github [1].
> > 4. Add support for reading On Demand meter certificates in sysfs.
> > 5. The rest of the patches modify the existing tool to support discovery
> > and reading of On Demand registers and the meter certificate.
> > 
> > [1] https://github.com/intel/intel-sdsi/blob/master/os-interface.rst
> > 
> > David E. Box (9):
> >   platform/x86/intel/sdsi: Add Intel On Demand text
> >   platform/x86/intel/sdsi: Hide attributes if hardware doesn't support
> >   platform/x86/intel/sdsi: Support different GUIDs
> >   platform/x86/intel/sdsi: Add meter certificate support
> >   tools/arch/x86: intel_sdsi: Add support for reading state certificates
> >   tools/arch/x86: intel_sdsi: Add Intel On Demand text
> >   tools/arch/x86: intel_sdsi: Read more On Demand registers
> >   tools/arch/x86: intel_sdsi: Add support for new GUID
> >   tools/arch/x86: intel_sdsi: Add support for reading meter certificates
> 
> Thank you, over all this looks good. I have some small remarks
> on patches 4, 8 and 9 see my replies to those.
> 
> Please prepare a v2 addressing Andy's + my review remarks and get
> that v2 to me no later then next week Tuesday, then I can still
> merge this in time for 6.2 .

Will do. Thanks Hans, Andy.

> 
> Regards,
> 
> Hans
> 
> 

Re: [PATCH 0/9] Extend Intel On Demand (SDSi) support

[Pavel Machek]

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

Hi!

> Intel Software Defined Silicon (SDSi) is now known as Intel On Demand. The
> following patches do the following:

Quick google gets me to
https://www.intel.com/content/www/us/en/products/docs/ondemand/overview.html
... which is not really clear.

Do I understand it correctly that this is support for unlocking
features on silicon user already owns?

That is rather user hostile, is it?

Best regards,
								Pavel
-- 
People of Russia, stop Putin before his war on Ukraine escalates.

[-- Attachment #2: signature.asc --]
[-- Type: application/pgp-signature, Size: 195 bytes --]