[PATCH V2 0/9] Intel On Demand changes

[PATCH V2 0/9] Intel On Demand changes

[David E. Box]

V2 removes the Attestation support while is it reworked into a separate
series. This series now focuses on smaller changes that address firmware
changes and makes fixes in the tool.

David E. Box (8):
  platform/x86/intel/sdsi: Set message size during writes
  platform/x86/intel/sdsi: Combine read and write mailbox flows
  platform/x86/intel/sdsi: Add attribute to read the current meter state
  tools/arch/x86/intel_sdsi: Fix maximum meter bundle length
  tools/arch/x86/intel_sdsi: Add missing version field
  tools/arch/x86/intel_sdsi: Fix meter_certificate decoding
  platform/x86/intel/sdsi: Simplify ascii printing
  tools: intel_sdsi: Add current meter support

Kuppuswamy Sathyanarayanan (1):
  platform/x86/intel/sdsi: Add in-band BIOS lock support

 drivers/platform/x86/intel/sdsi.c      | 118 ++++++++++++++++---------
 tools/arch/x86/intel_sdsi/intel_sdsi.c | 100 ++++++++++++---------
 2 files changed, 138 insertions(+), 80 deletions(-)


base-commit: d206a76d7d2726f3b096037f2079ce0bd3ba329b
-- 
2.34.1

[PATCH V2 1/9] platform/x86/intel/sdsi: Set message size during writes

[David E. Box]

New mailbox commands will support sending multi packet writes and updated
firmware now requires that the message size be written for all commands
along with the packet size. Since the driver doesn't perform writes larger
than the packet size, set the message size to the same value.

Signed-off-by: David E. Box <david.e.box@linux.intel.com>
Reviewed-by: Ilpo Järvinen <ilpo.jarvinen@linux.intel.com>
---

V2 - no changes

 drivers/platform/x86/intel/sdsi.c | 1 +
 1 file changed, 1 insertion(+)

diff --git a/drivers/platform/x86/intel/sdsi.c b/drivers/platform/x86/intel/sdsi.c
index 556e7c6dbb05..a70c071de6e2 100644
--- a/drivers/platform/x86/intel/sdsi.c
+++ b/drivers/platform/x86/intel/sdsi.c
@@ -252,6 +252,7 @@ static int sdsi_mbox_cmd_write(struct sdsi_priv *priv, struct sdsi_mbox_info *in
 		  FIELD_PREP(CTRL_SOM, 1) |
 		  FIELD_PREP(CTRL_RUN_BUSY, 1) |
 		  FIELD_PREP(CTRL_READ_WRITE, 1) |
+		  FIELD_PREP(CTRL_MSG_SIZE, info->size) |
 		  FIELD_PREP(CTRL_PACKET_SIZE, info->size);
 	writeq(control, priv->control_addr);
 
-- 
2.34.1

[PATCH V2 2/9] platform/x86/intel/sdsi: Combine read and write mailbox flows

[David E. Box]

The current mailbox commands are either read-only or write-only and the
flow is different for each. New commands will need to send and receive
data. In preparation for these commands, create a common polling function
to handle sending data and receiving in the same transaction.

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

V2 - In sdsi_cmd_read() remove unnecessary check for non-zero packet_size
     in do loop since the loop is exited earlier when packet_size is
     zero.

 drivers/platform/x86/intel/sdsi.c | 79 +++++++++++++++++--------------
 1 file changed, 44 insertions(+), 35 deletions(-)

diff --git a/drivers/platform/x86/intel/sdsi.c b/drivers/platform/x86/intel/sdsi.c
index a70c071de6e2..d80c2dc0ce71 100644
--- a/drivers/platform/x86/intel/sdsi.c
+++ b/drivers/platform/x86/intel/sdsi.c
@@ -15,6 +15,7 @@
 #include <linux/iopoll.h>
 #include <linux/kernel.h>
 #include <linux/module.h>
+#include <linux/overflow.h>
 #include <linux/pci.h>
 #include <linux/slab.h>
 #include <linux/sysfs.h>
@@ -156,8 +157,8 @@ static int sdsi_status_to_errno(u32 status)
 	}
 }
 
-static int sdsi_mbox_cmd_read(struct sdsi_priv *priv, struct sdsi_mbox_info *info,
-			      size_t *data_size)
+static int sdsi_mbox_poll(struct sdsi_priv *priv, struct sdsi_mbox_info *info,
+			  size_t *data_size)
 {
 	struct device *dev = priv->dev;
 	u32 total, loop, eom, status, message_size;
@@ -166,18 +167,10 @@ static int sdsi_mbox_cmd_read(struct sdsi_priv *priv, struct sdsi_mbox_info *inf
 
 	lockdep_assert_held(&priv->mb_lock);
 
-	/* Format and send the read command */
-	control = FIELD_PREP(CTRL_EOM, 1) |
-		  FIELD_PREP(CTRL_SOM, 1) |
-		  FIELD_PREP(CTRL_RUN_BUSY, 1) |
-		  FIELD_PREP(CTRL_PACKET_SIZE, info->size);
-	writeq(control, priv->control_addr);
-
 	/* For reads, data sizes that are larger than the mailbox size are read in packets. */
 	total = 0;
 	loop = 0;
 	do {
-		void *buf = info->buffer + (SDSI_SIZE_MAILBOX * loop);
 		u32 packet_size;
 
 		/* Poll on ready bit */
@@ -195,6 +188,11 @@ static int sdsi_mbox_cmd_read(struct sdsi_priv *priv, struct sdsi_mbox_info *inf
 		if (ret)
 			break;
 
+		if (!packet_size) {
+			sdsi_complete_transaction(priv);
+			break;
+		}
+
 		/* Only the last packet can be less than the mailbox size. */
 		if (!eom && packet_size != SDSI_SIZE_MAILBOX) {
 			dev_err(dev, "Invalid packet size\n");
@@ -208,9 +206,13 @@ static int sdsi_mbox_cmd_read(struct sdsi_priv *priv, struct sdsi_mbox_info *inf
 			break;
 		}
 
-		sdsi_memcpy64_fromio(buf, priv->mbox_addr, round_up(packet_size, SDSI_SIZE_CMD));
+		if (info->buffer) {
+			void *buf = info->buffer + array_size(SDSI_SIZE_MAILBOX, loop);
 
-		total += packet_size;
+			sdsi_memcpy64_fromio(buf, priv->mbox_addr,
+					     round_up(packet_size, SDSI_SIZE_CMD));
+			total += packet_size;
+		}
 
 		sdsi_complete_transaction(priv);
 	} while (!eom && ++loop < MBOX_MAX_PACKETS);
@@ -230,16 +232,33 @@ static int sdsi_mbox_cmd_read(struct sdsi_priv *priv, struct sdsi_mbox_info *inf
 		dev_warn(dev, "Read count %u differs from expected count %u\n",
 			 total, message_size);
 
-	*data_size = total;
+	if (data_size)
+		*data_size = total;
 
 	return 0;
 }
 
-static int sdsi_mbox_cmd_write(struct sdsi_priv *priv, struct sdsi_mbox_info *info)
+static int sdsi_mbox_cmd_read(struct sdsi_priv *priv, struct sdsi_mbox_info *info,
+			      size_t *data_size)
+{
+	u64 control;
+
+	lockdep_assert_held(&priv->mb_lock);
+
+	/* Format and send the read command */
+	control = FIELD_PREP(CTRL_EOM, 1) |
+		  FIELD_PREP(CTRL_SOM, 1) |
+		  FIELD_PREP(CTRL_RUN_BUSY, 1) |
+		  FIELD_PREP(CTRL_PACKET_SIZE, info->size);
+	writeq(control, priv->control_addr);
+
+	return sdsi_mbox_poll(priv, info, data_size);
+}
+
+static int sdsi_mbox_cmd_write(struct sdsi_priv *priv, struct sdsi_mbox_info *info,
+			       size_t *data_size)
 {
 	u64 control;
-	u32 status;
-	int ret;
 
 	lockdep_assert_held(&priv->mb_lock);
 
@@ -256,20 +275,7 @@ static int sdsi_mbox_cmd_write(struct sdsi_priv *priv, struct sdsi_mbox_info *in
 		  FIELD_PREP(CTRL_PACKET_SIZE, info->size);
 	writeq(control, priv->control_addr);
 
-	/* Poll on ready bit */
-	ret = readq_poll_timeout(priv->control_addr, control, control & CTRL_READY,
-				 MBOX_POLLING_PERIOD_US, MBOX_TIMEOUT_US);
-
-	if (ret)
-		goto release_mbox;
-
-	status = FIELD_GET(CTRL_STATUS, control);
-	ret = sdsi_status_to_errno(status);
-
-release_mbox:
-	sdsi_complete_transaction(priv);
-
-	return ret;
+	return sdsi_mbox_poll(priv, info, data_size);
 }
 
 static int sdsi_mbox_acquire(struct sdsi_priv *priv, struct sdsi_mbox_info *info)
@@ -313,7 +319,8 @@ static int sdsi_mbox_acquire(struct sdsi_priv *priv, struct sdsi_mbox_info *info
 	return ret;
 }
 
-static int sdsi_mbox_write(struct sdsi_priv *priv, struct sdsi_mbox_info *info)
+static int sdsi_mbox_write(struct sdsi_priv *priv, struct sdsi_mbox_info *info,
+			   size_t *data_size)
 {
 	int ret;
 
@@ -323,7 +330,7 @@ static int sdsi_mbox_write(struct sdsi_priv *priv, struct sdsi_mbox_info *info)
 	if (ret)
 		return ret;
 
-	return sdsi_mbox_cmd_write(priv, info);
+	return sdsi_mbox_cmd_write(priv, info, data_size);
 }
 
 static int sdsi_mbox_read(struct sdsi_priv *priv, struct sdsi_mbox_info *info, size_t *data_size)
@@ -342,7 +349,7 @@ static int sdsi_mbox_read(struct sdsi_priv *priv, struct sdsi_mbox_info *info, s
 static ssize_t sdsi_provision(struct sdsi_priv *priv, char *buf, size_t count,
 			      enum sdsi_command command)
 {
-	struct sdsi_mbox_info info;
+	struct sdsi_mbox_info info = {};
 	int ret;
 
 	if (count > (SDSI_SIZE_WRITE_MSG - SDSI_SIZE_CMD))
@@ -364,7 +371,9 @@ static ssize_t sdsi_provision(struct sdsi_priv *priv, char *buf, size_t count,
 	ret = mutex_lock_interruptible(&priv->mb_lock);
 	if (ret)
 		goto free_payload;
-	ret = sdsi_mbox_write(priv, &info);
+
+	ret = sdsi_mbox_write(priv, &info, NULL);
+
 	mutex_unlock(&priv->mb_lock);
 
 free_payload:
@@ -408,7 +417,7 @@ static ssize_t
 certificate_read(u64 command, struct sdsi_priv *priv, char *buf, loff_t off,
 		 size_t count)
 {
-	struct sdsi_mbox_info info;
+	struct sdsi_mbox_info info = {};
 	size_t size;
 	int ret;
 
-- 
2.34.1

[PATCH V2 3/9] platform/x86/intel/sdsi: Add in-band BIOS lock support

[David E. Box]

From: Kuppuswamy Sathyanarayanan <sathyanarayanan.kuppuswamy@linux.intel.com>

As per SDSi in-band interface specification, sec titled "BIOS lock for
in-band provisioning", when IB_LOCK bit is set in control qword, the
SDSI agent is only allowed to perform the read flow, but not allowed to
provision license blob or license key. So add check for it in
sdsi_provision().

Signed-off-by: Kuppuswamy Sathyanarayanan <sathyanarayanan.kuppuswamy@linux.intel.com>
Signed-off-by: David E. Box <david.e.box@linux.intel.com>
---

V2 - Move sdsi_ib_locked() check after overflow check

 drivers/platform/x86/intel/sdsi.c | 10 ++++++++++
 1 file changed, 10 insertions(+)

diff --git a/drivers/platform/x86/intel/sdsi.c b/drivers/platform/x86/intel/sdsi.c
index d80c2dc0ce71..bb3eaf5eb382 100644
--- a/drivers/platform/x86/intel/sdsi.c
+++ b/drivers/platform/x86/intel/sdsi.c
@@ -67,6 +67,7 @@
 #define CTRL_OWNER			GENMASK(5, 4)
 #define CTRL_COMPLETE			BIT(6)
 #define CTRL_READY			BIT(7)
+#define CTRL_INBAND_LOCK		BIT(32)
 #define CTRL_STATUS			GENMASK(15, 8)
 #define CTRL_PACKET_SIZE		GENMASK(31, 16)
 #define CTRL_MSG_SIZE			GENMASK(63, 48)
@@ -346,6 +347,11 @@ static int sdsi_mbox_read(struct sdsi_priv *priv, struct sdsi_mbox_info *info, s
 	return sdsi_mbox_cmd_read(priv, info, data_size);
 }
 
+static bool sdsi_ib_locked(struct sdsi_priv *priv)
+{
+	return !!FIELD_GET(CTRL_INBAND_LOCK, readq(priv->control_addr));
+}
+
 static ssize_t sdsi_provision(struct sdsi_priv *priv, char *buf, size_t count,
 			      enum sdsi_command command)
 {
@@ -355,6 +361,10 @@ static ssize_t sdsi_provision(struct sdsi_priv *priv, char *buf, size_t count,
 	if (count > (SDSI_SIZE_WRITE_MSG - SDSI_SIZE_CMD))
 		return -EOVERFLOW;
 
+	/* Make sure In-band lock is not set */
+	if (sdsi_ib_locked(priv))
+		return -EPERM;
+
 	/* Qword aligned message + command qword */
 	info.size = round_up(count, SDSI_SIZE_CMD) + SDSI_SIZE_CMD;
 
-- 
2.34.1

[PATCH V2 4/9] platform/x86/intel/sdsi: Add attribute to read the current meter state

[David E. Box]

The meter_certificate file provides access to metering information that may
be attested but is only updated every 8 hours. Add new attribute,
meter_current, to allow reading an untested snapshot of the current values.

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

V2 - make control_flags a parameter to be eventually passed to
     sdsi_mbox_cmd_read(). This removes the need for a lock which had been
     added to protect control_flags when it was a member of the private
     struct.

 drivers/platform/x86/intel/sdsi.c | 30 ++++++++++++++++++++++++------
 1 file changed, 24 insertions(+), 6 deletions(-)

diff --git a/drivers/platform/x86/intel/sdsi.c b/drivers/platform/x86/intel/sdsi.c
index bb3eaf5eb382..277e4f4b20ac 100644
--- a/drivers/platform/x86/intel/sdsi.c
+++ b/drivers/platform/x86/intel/sdsi.c
@@ -68,6 +68,7 @@
 #define CTRL_COMPLETE			BIT(6)
 #define CTRL_READY			BIT(7)
 #define CTRL_INBAND_LOCK		BIT(32)
+#define CTRL_METER_ENABLE_DRAM		BIT(33)
 #define CTRL_STATUS			GENMASK(15, 8)
 #define CTRL_PACKET_SIZE		GENMASK(31, 16)
 #define CTRL_MSG_SIZE			GENMASK(63, 48)
@@ -95,6 +96,7 @@ enum sdsi_command {
 struct sdsi_mbox_info {
 	u64	*payload;
 	void	*buffer;
+	u64	control_flags;
 	int	size;
 };
 
@@ -250,7 +252,8 @@ static int sdsi_mbox_cmd_read(struct sdsi_priv *priv, struct sdsi_mbox_info *inf
 	control = FIELD_PREP(CTRL_EOM, 1) |
 		  FIELD_PREP(CTRL_SOM, 1) |
 		  FIELD_PREP(CTRL_RUN_BUSY, 1) |
-		  FIELD_PREP(CTRL_PACKET_SIZE, info->size);
+		  FIELD_PREP(CTRL_PACKET_SIZE, info->size) |
+		  info->control_flags;
 	writeq(control, priv->control_addr);
 
 	return sdsi_mbox_poll(priv, info, data_size);
@@ -424,8 +427,8 @@ static ssize_t provision_cap_write(struct file *filp, struct kobject *kobj,
 static BIN_ATTR_WO(provision_cap, SDSI_SIZE_WRITE_MSG);
 
 static ssize_t
-certificate_read(u64 command, struct sdsi_priv *priv, char *buf, loff_t off,
-		 size_t count)
+certificate_read(u64 command, u64 control_flags, struct sdsi_priv *priv,
+		 char *buf, loff_t off, size_t count)
 {
 	struct sdsi_mbox_info info = {};
 	size_t size;
@@ -441,6 +444,7 @@ certificate_read(u64 command, struct sdsi_priv *priv, char *buf, loff_t off,
 
 	info.payload = &command;
 	info.size = sizeof(command);
+	info.control_flags = control_flags;
 
 	ret = mutex_lock_interruptible(&priv->mb_lock);
 	if (ret)
@@ -472,7 +476,7 @@ state_certificate_read(struct file *filp, struct kobject *kobj,
 	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);
+	return certificate_read(SDSI_CMD_READ_STATE, 0, priv, buf, off, count);
 }
 static BIN_ATTR_ADMIN_RO(state_certificate, SDSI_SIZE_READ_MSG);
 
@@ -484,10 +488,23 @@ meter_certificate_read(struct file *filp, struct kobject *kobj,
 	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);
+	return certificate_read(SDSI_CMD_READ_METER, 0, priv, buf, off, count);
 }
 static BIN_ATTR_ADMIN_RO(meter_certificate, SDSI_SIZE_READ_MSG);
 
+static ssize_t
+meter_current_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, CTRL_METER_ENABLE_DRAM,
+				priv, buf, off, count);
+}
+static BIN_ATTR_ADMIN_RO(meter_current, 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)
@@ -518,6 +535,7 @@ static struct bin_attribute *sdsi_bin_attrs[] = {
 	&bin_attr_registers,
 	&bin_attr_state_certificate,
 	&bin_attr_meter_certificate,
+	&bin_attr_meter_current,
 	&bin_attr_provision_akc,
 	&bin_attr_provision_cap,
 	NULL
@@ -537,7 +555,7 @@ sdsi_battr_is_visible(struct kobject *kobj, struct bin_attribute *attr, int n)
 	if (!(priv->features & SDSI_FEATURE_SDSI))
 		return 0;
 
-	if (attr == &bin_attr_meter_certificate)
+	if (attr == &bin_attr_meter_certificate || attr == &bin_attr_meter_current)
 		return (priv->features & SDSI_FEATURE_METERING) ?
 				attr->attr.mode : 0;
 
-- 
2.34.1

[PATCH V2 5/9] tools/arch/x86/intel_sdsi: Fix maximum meter bundle length

[David E. Box]

The maximum number of bundles in the meter certificate was set to 8 which
is much less than the maximum. Instead, since the bundles appear at the end
of the file, set it based on the remaining file size from the bundle start
position.

Fixes: aad129780bae ("platform/x86/intel/sdsi: Add support for reading the current meter state")
Signed-off-by: David E. Box <david.e.box@linux.intel.com>
---

V2 - Split of V1 patch 7

 tools/arch/x86/intel_sdsi/intel_sdsi.c | 6 ++++--
 1 file changed, 4 insertions(+), 2 deletions(-)

diff --git a/tools/arch/x86/intel_sdsi/intel_sdsi.c b/tools/arch/x86/intel_sdsi/intel_sdsi.c
index 2cd92761f171..a02850a710ee 100644
--- a/tools/arch/x86/intel_sdsi/intel_sdsi.c
+++ b/tools/arch/x86/intel_sdsi/intel_sdsi.c
@@ -43,7 +43,6 @@
 #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)
@@ -167,6 +166,9 @@ struct bundle_encoding_counter {
 	uint32_t encoding;
 	uint32_t counter;
 };
+#define METER_MAX_NUM_BUNDLES							\
+		((METER_CERT_MAX_SIZE - sizeof(struct meter_certificate)) /	\
+		 sizeof(struct bundle_encoding_counter))
 
 struct sdsi_dev {
 	struct sdsi_regs regs;
@@ -387,7 +389,7 @@ static int sdsi_meter_cert_show(struct sdsi_dev *s)
 	}
 
 	if (mc->bundle_length > METER_MAX_NUM_BUNDLES * 8)  {
-		fprintf(stderr, "More than %d bundles: %d\n",
+		fprintf(stderr, "More than %ld bundles: actual %d\n",
 			METER_MAX_NUM_BUNDLES, mc->bundle_length / 8);
 		return -1;
 	}
-- 
2.34.1

[PATCH V2 6/9] tools/arch/x86/intel_sdsi: Add missing version field

[David E. Box]

Add missing 'version' field to struct meter_certificate. Also fix output
string alignment.

Fixes: aad129780bae ("platform/x86/intel/sdsi: Add support for reading the current meter state")
Signed-off-by: David E. Box <david.e.box@linux.intel.com>
---

V2 - Split of V1 patch 7

 tools/arch/x86/intel_sdsi/intel_sdsi.c | 29 +++++++++++++++++---------
 1 file changed, 19 insertions(+), 10 deletions(-)

diff --git a/tools/arch/x86/intel_sdsi/intel_sdsi.c b/tools/arch/x86/intel_sdsi/intel_sdsi.c
index a02850a710ee..def1b9a01738 100644
--- a/tools/arch/x86/intel_sdsi/intel_sdsi.c
+++ b/tools/arch/x86/intel_sdsi/intel_sdsi.c
@@ -153,11 +153,12 @@ struct bundle_encoding {
 };
 
 struct meter_certificate {
-	uint32_t block_signature;
+	uint32_t signature;
+	uint32_t version;
+	uint64_t ppin;
 	uint32_t counter_unit;
-	uint64_t ppin;
 	uint32_t bundle_length;
-	uint32_t reserved;
+	uint64_t reserved;
 	uint32_t mmrc_encoding;
 	uint32_t mmrc_counter;
 };
@@ -336,6 +337,7 @@ static int sdsi_meter_cert_show(struct sdsi_dev *s)
 	uint32_t count = 0;
 	FILE *cert_ptr;
 	int ret, size;
+	char name[4];
 
 	ret = sdsi_update_registers(s);
 	if (ret)
@@ -377,12 +379,19 @@ static int sdsi_meter_cert_show(struct sdsi_dev *s)
 	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);
+
+	get_feature(mc->signature, name);
+	printf("Signature:                    %.4s\n", name);
+
+	printf("Version:                      %d\n", mc->version);
+	printf("Count Unit:                   %dms\n", mc->counter_unit);
+	printf("PPIN:                         0x%lx\n", mc->ppin);
+	printf("Feature Bundle Length:        %d\n", mc->bundle_length);
+
+	get_feature(mc->mmrc_encoding, name);
+	printf("MMRC encoding:                %.4s\n", name);
+
+	printf("MMRC counter:                 %d\n", mc->mmrc_counter);
 	if (mc->bundle_length % 8) {
 		fprintf(stderr, "Invalid bundle length\n");
 		return -1;
@@ -396,7 +405,7 @@ static int sdsi_meter_cert_show(struct sdsi_dev *s)
 
 	bec = (void *)(mc) + sizeof(mc);
 
-	printf("Number of Feature Counters:          %d\n", mc->bundle_length / 8);
+	printf("Number of Feature Counters:   %d\n", mc->bundle_length / 8);
 	while (count++ < mc->bundle_length / 8) {
 		char feature[5];
 
-- 
2.34.1

[PATCH V2 7/9] tools/arch/x86/intel_sdsi: Fix meter_certificate decoding

[David E. Box]

Fix errors in the calculation of the start position of the counters and in
the display loop.

Fixes: aad129780bae ("platform/x86/intel/sdsi: Add support for reading the current meter state")
Signed-off-by: David E. Box <david.e.box@linux.intel.com>
---

V2 - Split of V1 patch 7

 tools/arch/x86/intel_sdsi/intel_sdsi.c | 5 +++--
 1 file changed, 3 insertions(+), 2 deletions(-)

diff --git a/tools/arch/x86/intel_sdsi/intel_sdsi.c b/tools/arch/x86/intel_sdsi/intel_sdsi.c
index def1b9a01738..2acc6be5f5d6 100644
--- a/tools/arch/x86/intel_sdsi/intel_sdsi.c
+++ b/tools/arch/x86/intel_sdsi/intel_sdsi.c
@@ -403,15 +403,16 @@ static int sdsi_meter_cert_show(struct sdsi_dev *s)
 		return -1;
 	}
 
-	bec = (void *)(mc) + sizeof(mc);
+	bec = (void *)(mc) + sizeof(*mc);
 
 	printf("Number of Feature Counters:   %d\n", mc->bundle_length / 8);
-	while (count++ < 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);
+		++count;
 	}
 
 	return 0;
-- 
2.34.1

[PATCH V2 8/9] platform/x86/intel/sdsi: Simplify ascii printing

[David E. Box]

Use printf width specifier to set the display length of encoded feature
names.

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

V2 - Split of V1 patch 7

 tools/arch/x86/intel_sdsi/intel_sdsi.c | 11 ++++-------
 1 file changed, 4 insertions(+), 7 deletions(-)

diff --git a/tools/arch/x86/intel_sdsi/intel_sdsi.c b/tools/arch/x86/intel_sdsi/intel_sdsi.c
index 2acc6be5f5d6..a8fb6d17405f 100644
--- a/tools/arch/x86/intel_sdsi/intel_sdsi.c
+++ b/tools/arch/x86/intel_sdsi/intel_sdsi.c
@@ -407,11 +407,10 @@ static int sdsi_meter_cert_show(struct sdsi_dev *s)
 
 	printf("Number of Feature Counters:   %d\n", mc->bundle_length / 8);
 	while (count < mc->bundle_length / 8) {
-		char feature[5];
+		char feature[4];
 
-		feature[4] = '\0';
 		get_feature(bec[count].encoding, feature);
-		printf("    %s:          %d\n", feature, bec[count].counter);
+		printf("    %.4s:          %d\n", feature, bec[count].counter);
 		++count;
 	}
 
@@ -492,7 +491,7 @@ static int sdsi_state_cert_show(struct sdsi_dev *s)
 			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];
+		char feature[4];
 		uint32_t i;
 
 		printf("     Blob %d:\n", count - 1);
@@ -505,11 +504,9 @@ static int sdsi_state_cert_show(struct sdsi_dev *s)
 		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("                 Feature %d:         %.4s\n", i, feature);
 		}
 
 		if (lbc->num_bundles > STATE_MAX_NUM_IN_BUNDLE)
-- 
2.34.1

[PATCH V2 9/9] tools: intel_sdsi: Add current meter support

[David E. Box]

Add support to read the 'meter_current' file. The display is the same as
the 'meter_certificate', but will show the current snapshot of the
counters.

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

V2 - Set the name of the file to be opened once.

 tools/arch/x86/intel_sdsi/intel_sdsi.c | 49 ++++++++++++++++----------
 1 file changed, 30 insertions(+), 19 deletions(-)

diff --git a/tools/arch/x86/intel_sdsi/intel_sdsi.c b/tools/arch/x86/intel_sdsi/intel_sdsi.c
index a8fb6d17405f..325e1e41af1d 100644
--- a/tools/arch/x86/intel_sdsi/intel_sdsi.c
+++ b/tools/arch/x86/intel_sdsi/intel_sdsi.c
@@ -182,6 +182,7 @@ struct sdsi_dev {
 enum command {
 	CMD_SOCKET_INFO,
 	CMD_METER_CERT,
+	CMD_METER_CURRENT_CERT,
 	CMD_STATE_CERT,
 	CMD_PROV_AKC,
 	CMD_PROV_CAP,
@@ -329,13 +330,14 @@ static void get_feature(uint32_t encoding, char *feature)
 	feature[0] = name[3];
 }
 
-static int sdsi_meter_cert_show(struct sdsi_dev *s)
+static int sdsi_meter_cert_show(struct sdsi_dev *s, bool show_current)
 {
 	char buf[METER_CERT_MAX_SIZE] = {0};
 	struct bundle_encoding_counter *bec;
 	struct meter_certificate *mc;
 	uint32_t count = 0;
 	FILE *cert_ptr;
+	char *cert_fname;
 	int ret, size;
 	char name[4];
 
@@ -345,7 +347,6 @@ static int sdsi_meter_cert_show(struct sdsi_dev *s)
 
 	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;
 	}
 
@@ -360,15 +361,17 @@ static int sdsi_meter_cert_show(struct sdsi_dev *s)
 		return ret;
 	}
 
-	cert_ptr = fopen("meter_certificate", "r");
+	cert_fname = show_current ? "meter_current" : "meter_certificate";
+	cert_ptr = fopen(cert_fname, "r");
+
 	if (!cert_ptr) {
-		perror("Could not open 'meter_certificate' file");
+		fprintf(stderr, "Could not open '%s' file: %s", cert_fname, strerror(errno));
 		return -1;
 	}
 
 	size = fread(buf, 1, sizeof(buf), cert_ptr);
 	if (!size) {
-		fprintf(stderr, "Could not read 'meter_certificate' file\n");
+		fprintf(stderr, "Could not read '%s' file\n", cert_fname);
 		fclose(cert_ptr);
 		return -1;
 	}
@@ -734,7 +737,7 @@ static void sdsi_free_dev(struct sdsi_dev *s)
 
 static void usage(char *prog)
 {
-	printf("Usage: %s [-l] [-d DEVNO [-i] [-s] [-m] [-a FILE] [-c FILE]]\n", prog);
+	printf("Usage: %s [-l] [-d DEVNO [-i] [-s] [-m | -C] [-a FILE] [-c FILE]\n", prog);
 }
 
 static void show_help(void)
@@ -743,8 +746,9 @@ static void show_help(void)
 	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", "-m, --meter",          "show meter certificate");
+	printf("  %-18s\t%s\n", "-s, --state",          "show state certificate data");
+	printf("  %-18s\t%s\n", "-m, --meter",          "show meter certificate data");
+	printf("  %-18s\t%s\n", "-C, --meter_current",  "show live unattested meter 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");
 }
@@ -760,21 +764,22 @@ int main(int argc, char *argv[])
 	int option_index = 0;
 
 	static struct option long_options[] = {
-		{"akc",		required_argument,	0, 'a'},
-		{"cap",		required_argument,	0, 'c'},
-		{"devno",	required_argument,	0, 'd'},
-		{"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 }
+		{"akc",			required_argument,	0, 'a'},
+		{"cap",			required_argument,	0, 'c'},
+		{"devno",		required_argument,	0, 'd'},
+		{"help",		no_argument,		0, 'h'},
+		{"info",		no_argument,		0, 'i'},
+		{"list",		no_argument,		0, 'l'},
+		{"meter",		no_argument,		0, 'm'},
+		{"meter_current",	no_argument,		0, 'C'},
+		{"state",		no_argument,		0, 's'},
+		{0,			0,			0, 0 }
 	};
 
 
 	progname = argv[0];
 
-	while ((opt = getopt_long_only(argc, argv, "+a:c:d:hilms", long_options,
+	while ((opt = getopt_long_only(argc, argv, "+a:c:d:hilmCs", long_options,
 			&option_index)) != -1) {
 		switch (opt) {
 		case 'd':
@@ -790,6 +795,9 @@ int main(int argc, char *argv[])
 		case 'm':
 			command = CMD_METER_CERT;
 			break;
+		case 'C':
+			command = CMD_METER_CURRENT_CERT;
+			break;
 		case 's':
 			command = CMD_STATE_CERT;
 			break;
@@ -828,7 +836,10 @@ int main(int argc, char *argv[])
 			ret = sdsi_read_reg(s);
 			break;
 		case CMD_METER_CERT:
-			ret = sdsi_meter_cert_show(s);
+			ret = sdsi_meter_cert_show(s, false);
+			break;
+		case CMD_METER_CURRENT_CERT:
+			ret = sdsi_meter_cert_show(s, true);
 			break;
 		case CMD_STATE_CERT:
 			ret = sdsi_state_cert_show(s);
-- 
2.34.1

Re: [PATCH V2 1/9] platform/x86/intel/sdsi: Set message size during writes

[Kuppuswamy Sathyanarayanan]

On 2/27/24 4:00 PM, David E. Box wrote:
> New mailbox commands will support sending multi packet writes and updated
> firmware now requires that the message size be written for all commands
> along with the packet size. Since the driver doesn't perform writes larger
> than the packet size, set the message size to the same value.
>
> Signed-off-by: David E. Box <david.e.box@linux.intel.com>
> Reviewed-by: Ilpo Järvinen <ilpo.jarvinen@linux.intel.com>
> ---
Reviewed-by: Kuppuswamy Sathyanarayanan <sathyanarayanan.kuppuswamy@linux.intel.com>
>
> V2 - no changes
>
>  drivers/platform/x86/intel/sdsi.c | 1 +
>  1 file changed, 1 insertion(+)
>
> diff --git a/drivers/platform/x86/intel/sdsi.c b/drivers/platform/x86/intel/sdsi.c
> index 556e7c6dbb05..a70c071de6e2 100644
> --- a/drivers/platform/x86/intel/sdsi.c
> +++ b/drivers/platform/x86/intel/sdsi.c
> @@ -252,6 +252,7 @@ static int sdsi_mbox_cmd_write(struct sdsi_priv *priv, struct sdsi_mbox_info *in
>  		  FIELD_PREP(CTRL_SOM, 1) |
>  		  FIELD_PREP(CTRL_RUN_BUSY, 1) |
>  		  FIELD_PREP(CTRL_READ_WRITE, 1) |
> +		  FIELD_PREP(CTRL_MSG_SIZE, info->size) |
>  		  FIELD_PREP(CTRL_PACKET_SIZE, info->size);
>  	writeq(control, priv->control_addr);
>  

-- 
Sathyanarayanan Kuppuswamy
Linux Kernel Developer

Re: [PATCH V2 2/9] platform/x86/intel/sdsi: Combine read and write mailbox flows

[Kuppuswamy Sathyanarayanan]

On 2/27/24 4:00 PM, David E. Box wrote:
> The current mailbox commands are either read-only or write-only and the
> flow is different for each. New commands will need to send and receive
> data. In preparation for these commands, create a common polling function
> to handle sending data and receiving in the same transaction.
>
> Signed-off-by: David E. Box <david.e.box@linux.intel.com>
> ---
>
> V2 - In sdsi_cmd_read() remove unnecessary check for non-zero packet_size
>      in do loop since the loop is exited earlier when packet_size is
>      zero.
>
>  drivers/platform/x86/intel/sdsi.c | 79 +++++++++++++++++--------------
>  1 file changed, 44 insertions(+), 35 deletions(-)
>
> diff --git a/drivers/platform/x86/intel/sdsi.c b/drivers/platform/x86/intel/sdsi.c
> index a70c071de6e2..d80c2dc0ce71 100644
> --- a/drivers/platform/x86/intel/sdsi.c
> +++ b/drivers/platform/x86/intel/sdsi.c
> @@ -15,6 +15,7 @@
>  #include <linux/iopoll.h>
>  #include <linux/kernel.h>
>  #include <linux/module.h>
> +#include <linux/overflow.h>
>  #include <linux/pci.h>
>  #include <linux/slab.h>
>  #include <linux/sysfs.h>
> @@ -156,8 +157,8 @@ static int sdsi_status_to_errno(u32 status)
>  	}
>  }
>  
> -static int sdsi_mbox_cmd_read(struct sdsi_priv *priv, struct sdsi_mbox_info *info,
> -			      size_t *data_size)
> +static int sdsi_mbox_poll(struct sdsi_priv *priv, struct sdsi_mbox_info *info,
> +			  size_t *data_size)
>  {
>  	struct device *dev = priv->dev;
>  	u32 total, loop, eom, status, message_size;
> @@ -166,18 +167,10 @@ static int sdsi_mbox_cmd_read(struct sdsi_priv *priv, struct sdsi_mbox_info *inf
>  
>  	lockdep_assert_held(&priv->mb_lock);
>  
> -	/* Format and send the read command */
> -	control = FIELD_PREP(CTRL_EOM, 1) |
> -		  FIELD_PREP(CTRL_SOM, 1) |
> -		  FIELD_PREP(CTRL_RUN_BUSY, 1) |
> -		  FIELD_PREP(CTRL_PACKET_SIZE, info->size);
> -	writeq(control, priv->control_addr);
> -
>  	/* For reads, data sizes that are larger than the mailbox size are read in packets. */
>  	total = 0;
>  	loop = 0;
>  	do {
> -		void *buf = info->buffer + (SDSI_SIZE_MAILBOX * loop);
>  		u32 packet_size;
>  
>  		/* Poll on ready bit */
> @@ -195,6 +188,11 @@ static int sdsi_mbox_cmd_read(struct sdsi_priv *priv, struct sdsi_mbox_info *inf
>  		if (ret)
>  			break;
>  
> +		if (!packet_size) {
> +			sdsi_complete_transaction(priv);
> +			break;
> +		}
> +
>  		/* Only the last packet can be less than the mailbox size. */
>  		if (!eom && packet_size != SDSI_SIZE_MAILBOX) {
>  			dev_err(dev, "Invalid packet size\n");
> @@ -208,9 +206,13 @@ static int sdsi_mbox_cmd_read(struct sdsi_priv *priv, struct sdsi_mbox_info *inf
>  			break;
>  		}
>  
> -		sdsi_memcpy64_fromio(buf, priv->mbox_addr, round_up(packet_size, SDSI_SIZE_CMD));
> +		if (info->buffer) {
> +			void *buf = info->buffer + array_size(SDSI_SIZE_MAILBOX, loop);
>  
> -		total += packet_size;
> +			sdsi_memcpy64_fromio(buf, priv->mbox_addr,
> +					     round_up(packet_size, SDSI_SIZE_CMD));
> +			total += packet_size;
> +		}
>  
>  		sdsi_complete_transaction(priv);
>  	} while (!eom && ++loop < MBOX_MAX_PACKETS);
> @@ -230,16 +232,33 @@ static int sdsi_mbox_cmd_read(struct sdsi_priv *priv, struct sdsi_mbox_info *inf
>  		dev_warn(dev, "Read count %u differs from expected count %u\n",
>  			 total, message_size);
>  
> -	*data_size = total;
> +	if (data_size)
> +		*data_size = total;
>  

Is there a chance for it to be NULL with current callers?

>  	return 0;
>  }
>  
> -static int sdsi_mbox_cmd_write(struct sdsi_priv *priv, struct sdsi_mbox_info *info)
> +static int sdsi_mbox_cmd_read(struct sdsi_priv *priv, struct sdsi_mbox_info *info,
> +			      size_t *data_size)
> +{
> +	u64 control;
> +
> +	lockdep_assert_held(&priv->mb_lock);
> +
> +	/* Format and send the read command */
> +	control = FIELD_PREP(CTRL_EOM, 1) |
> +		  FIELD_PREP(CTRL_SOM, 1) |
> +		  FIELD_PREP(CTRL_RUN_BUSY, 1) |
> +		  FIELD_PREP(CTRL_PACKET_SIZE, info->size);
> +	writeq(control, priv->control_addr);
> +
> +	return sdsi_mbox_poll(priv, info, data_size);
> +}
> +
> +static int sdsi_mbox_cmd_write(struct sdsi_priv *priv, struct sdsi_mbox_info *info,
> +			       size_t *data_size)
>  {
>  	u64 control;
> -	u32 status;
> -	int ret;
>  
>  	lockdep_assert_held(&priv->mb_lock);
>  
> @@ -256,20 +275,7 @@ static int sdsi_mbox_cmd_write(struct sdsi_priv *priv, struct sdsi_mbox_info *in
>  		  FIELD_PREP(CTRL_PACKET_SIZE, info->size);
>  	writeq(control, priv->control_addr);
>  
> -	/* Poll on ready bit */
> -	ret = readq_poll_timeout(priv->control_addr, control, control & CTRL_READY,
> -				 MBOX_POLLING_PERIOD_US, MBOX_TIMEOUT_US);
> -
> -	if (ret)
> -		goto release_mbox;
> -
> -	status = FIELD_GET(CTRL_STATUS, control);
> -	ret = sdsi_status_to_errno(status);
> -
> -release_mbox:
> -	sdsi_complete_transaction(priv);
> -
> -	return ret;
> +	return sdsi_mbox_poll(priv, info, data_size);
>  }
>  
>  static int sdsi_mbox_acquire(struct sdsi_priv *priv, struct sdsi_mbox_info *info)
> @@ -313,7 +319,8 @@ static int sdsi_mbox_acquire(struct sdsi_priv *priv, struct sdsi_mbox_info *info
>  	return ret;
>  }
>  
> -static int sdsi_mbox_write(struct sdsi_priv *priv, struct sdsi_mbox_info *info)
> +static int sdsi_mbox_write(struct sdsi_priv *priv, struct sdsi_mbox_info *info,
> +			   size_t *data_size)
>  {
>  	int ret;
>  
> @@ -323,7 +330,7 @@ static int sdsi_mbox_write(struct sdsi_priv *priv, struct sdsi_mbox_info *info)
>  	if (ret)
>  		return ret;
>  
> -	return sdsi_mbox_cmd_write(priv, info);
> +	return sdsi_mbox_cmd_write(priv, info, data_size);
>  }
>  
>  static int sdsi_mbox_read(struct sdsi_priv *priv, struct sdsi_mbox_info *info, size_t *data_size)
> @@ -342,7 +349,7 @@ static int sdsi_mbox_read(struct sdsi_priv *priv, struct sdsi_mbox_info *info, s
>  static ssize_t sdsi_provision(struct sdsi_priv *priv, char *buf, size_t count,
>  			      enum sdsi_command command)
>  {
> -	struct sdsi_mbox_info info;
> +	struct sdsi_mbox_info info = {};
>  	int ret;
>  
>  	if (count > (SDSI_SIZE_WRITE_MSG - SDSI_SIZE_CMD))
> @@ -364,7 +371,9 @@ static ssize_t sdsi_provision(struct sdsi_priv *priv, char *buf, size_t count,
>  	ret = mutex_lock_interruptible(&priv->mb_lock);
>  	if (ret)
>  		goto free_payload;
> -	ret = sdsi_mbox_write(priv, &info);
> +
> +	ret = sdsi_mbox_write(priv, &info, NULL);
> +
>  	mutex_unlock(&priv->mb_lock);
>  
>  free_payload:
> @@ -408,7 +417,7 @@ static ssize_t
>  certificate_read(u64 command, struct sdsi_priv *priv, char *buf, loff_t off,
>  		 size_t count)
>  {
> -	struct sdsi_mbox_info info;
> +	struct sdsi_mbox_info info = {};
>  	size_t size;
>  	int ret;
>  

-- 
Sathyanarayanan Kuppuswamy
Linux Kernel Developer

Re: [PATCH V2 4/9] platform/x86/intel/sdsi: Add attribute to read the current meter state

[Kuppuswamy Sathyanarayanan]

On 2/27/24 4:00 PM, David E. Box wrote:
> The meter_certificate file provides access to metering information that may
> be attested but is only updated every 8 hours. Add new attribute,
> meter_current, to allow reading an untested snapshot of the current values.
>
> Signed-off-by: David E. Box <david.e.box@linux.intel.com>
> ---

Looks good to me.

Reviewed-by: Kuppuswamy Sathyanarayanan <sathyanarayanan.kuppuswamy@linux.intel.com>

>
> V2 - make control_flags a parameter to be eventually passed to
>      sdsi_mbox_cmd_read(). This removes the need for a lock which had been
>      added to protect control_flags when it was a member of the private
>      struct.
>
>  drivers/platform/x86/intel/sdsi.c | 30 ++++++++++++++++++++++++------
>  1 file changed, 24 insertions(+), 6 deletions(-)
>
> diff --git a/drivers/platform/x86/intel/sdsi.c b/drivers/platform/x86/intel/sdsi.c
> index bb3eaf5eb382..277e4f4b20ac 100644
> --- a/drivers/platform/x86/intel/sdsi.c
> +++ b/drivers/platform/x86/intel/sdsi.c
> @@ -68,6 +68,7 @@
>  #define CTRL_COMPLETE			BIT(6)
>  #define CTRL_READY			BIT(7)
>  #define CTRL_INBAND_LOCK		BIT(32)
> +#define CTRL_METER_ENABLE_DRAM		BIT(33)
>  #define CTRL_STATUS			GENMASK(15, 8)
>  #define CTRL_PACKET_SIZE		GENMASK(31, 16)
>  #define CTRL_MSG_SIZE			GENMASK(63, 48)
> @@ -95,6 +96,7 @@ enum sdsi_command {
>  struct sdsi_mbox_info {
>  	u64	*payload;
>  	void	*buffer;
> +	u64	control_flags;
>  	int	size;
>  };
>  
> @@ -250,7 +252,8 @@ static int sdsi_mbox_cmd_read(struct sdsi_priv *priv, struct sdsi_mbox_info *inf
>  	control = FIELD_PREP(CTRL_EOM, 1) |
>  		  FIELD_PREP(CTRL_SOM, 1) |
>  		  FIELD_PREP(CTRL_RUN_BUSY, 1) |
> -		  FIELD_PREP(CTRL_PACKET_SIZE, info->size);
> +		  FIELD_PREP(CTRL_PACKET_SIZE, info->size) |
> +		  info->control_flags;
>  	writeq(control, priv->control_addr);
>  
>  	return sdsi_mbox_poll(priv, info, data_size);
> @@ -424,8 +427,8 @@ static ssize_t provision_cap_write(struct file *filp, struct kobject *kobj,
>  static BIN_ATTR_WO(provision_cap, SDSI_SIZE_WRITE_MSG);
>  
>  static ssize_t
> -certificate_read(u64 command, struct sdsi_priv *priv, char *buf, loff_t off,
> -		 size_t count)
> +certificate_read(u64 command, u64 control_flags, struct sdsi_priv *priv,
> +		 char *buf, loff_t off, size_t count)
>  {
>  	struct sdsi_mbox_info info = {};
>  	size_t size;
> @@ -441,6 +444,7 @@ certificate_read(u64 command, struct sdsi_priv *priv, char *buf, loff_t off,
>  
>  	info.payload = &command;
>  	info.size = sizeof(command);
> +	info.control_flags = control_flags;
>  
>  	ret = mutex_lock_interruptible(&priv->mb_lock);
>  	if (ret)
> @@ -472,7 +476,7 @@ state_certificate_read(struct file *filp, struct kobject *kobj,
>  	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);
> +	return certificate_read(SDSI_CMD_READ_STATE, 0, priv, buf, off, count);
>  }
>  static BIN_ATTR_ADMIN_RO(state_certificate, SDSI_SIZE_READ_MSG);
>  
> @@ -484,10 +488,23 @@ meter_certificate_read(struct file *filp, struct kobject *kobj,
>  	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);
> +	return certificate_read(SDSI_CMD_READ_METER, 0, priv, buf, off, count);
>  }
>  static BIN_ATTR_ADMIN_RO(meter_certificate, SDSI_SIZE_READ_MSG);
>  
> +static ssize_t
> +meter_current_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, CTRL_METER_ENABLE_DRAM,
> +				priv, buf, off, count);
> +}
> +static BIN_ATTR_ADMIN_RO(meter_current, 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)
> @@ -518,6 +535,7 @@ static struct bin_attribute *sdsi_bin_attrs[] = {
>  	&bin_attr_registers,
>  	&bin_attr_state_certificate,
>  	&bin_attr_meter_certificate,
> +	&bin_attr_meter_current,
>  	&bin_attr_provision_akc,
>  	&bin_attr_provision_cap,
>  	NULL
> @@ -537,7 +555,7 @@ sdsi_battr_is_visible(struct kobject *kobj, struct bin_attribute *attr, int n)
>  	if (!(priv->features & SDSI_FEATURE_SDSI))
>  		return 0;
>  
> -	if (attr == &bin_attr_meter_certificate)
> +	if (attr == &bin_attr_meter_certificate || attr == &bin_attr_meter_current)
>  		return (priv->features & SDSI_FEATURE_METERING) ?
>  				attr->attr.mode : 0;
>  

-- 
Sathyanarayanan Kuppuswamy
Linux Kernel Developer

Re: [PATCH V2 5/9] tools/arch/x86/intel_sdsi: Fix maximum meter bundle length

[Kuppuswamy Sathyanarayanan]

On 2/27/24 4:00 PM, David E. Box wrote:
> The maximum number of bundles in the meter certificate was set to 8 which
> is much less than the maximum. Instead, since the bundles appear at the end
> of the file, set it based on the remaining file size from the bundle start
> position.
>
> Fixes: aad129780bae ("platform/x86/intel/sdsi: Add support for reading the current meter state")
> Signed-off-by: David E. Box <david.e.box@linux.intel.com>
> ---
>
> V2 - Split of V1 patch 7
>
>  tools/arch/x86/intel_sdsi/intel_sdsi.c | 6 ++++--
>  1 file changed, 4 insertions(+), 2 deletions(-)
>
> diff --git a/tools/arch/x86/intel_sdsi/intel_sdsi.c b/tools/arch/x86/intel_sdsi/intel_sdsi.c
> index 2cd92761f171..a02850a710ee 100644
> --- a/tools/arch/x86/intel_sdsi/intel_sdsi.c
> +++ b/tools/arch/x86/intel_sdsi/intel_sdsi.c
> @@ -43,7 +43,6 @@
>  #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)
> @@ -167,6 +166,9 @@ struct bundle_encoding_counter {
>  	uint32_t encoding;
>  	uint32_t counter;
>  };
> +#define METER_MAX_NUM_BUNDLES							\
> +		((METER_CERT_MAX_SIZE - sizeof(struct meter_certificate)) /	\
> +		 sizeof(struct bundle_encoding_counter))
>  
>  struct sdsi_dev {
>  	struct sdsi_regs regs;
> @@ -387,7 +389,7 @@ static int sdsi_meter_cert_show(struct sdsi_dev *s)
>  	}
>  
>  	if (mc->bundle_length > METER_MAX_NUM_BUNDLES * 8)  {
> -		fprintf(stderr, "More than %d bundles: %d\n",
> +		fprintf(stderr, "More than %ld bundles: actual %d\n",
>  			METER_MAX_NUM_BUNDLES, mc->bundle_length / 8);

I think you can avoid hardcoding here as well. sizeof(struct bundle_encoding_counter)

>  		return -1;
>  	}

-- 
Sathyanarayanan Kuppuswamy
Linux Kernel Developer

Re: [PATCH V2 6/9] tools/arch/x86/intel_sdsi: Add missing version field

[Kuppuswamy Sathyanarayanan]

On 2/27/24 4:00 PM, David E. Box wrote:
> Add missing 'version' field to struct meter_certificate. Also fix output
> string alignment.
>
> Fixes: aad129780bae ("platform/x86/intel/sdsi: Add support for reading the current meter state")
> Signed-off-by: David E. Box <david.e.box@linux.intel.com>
> ---
>
> V2 - Split of V1 patch 7
>
>  tools/arch/x86/intel_sdsi/intel_sdsi.c | 29 +++++++++++++++++---------
>  1 file changed, 19 insertions(+), 10 deletions(-)
>
> diff --git a/tools/arch/x86/intel_sdsi/intel_sdsi.c b/tools/arch/x86/intel_sdsi/intel_sdsi.c
> index a02850a710ee..def1b9a01738 100644
> --- a/tools/arch/x86/intel_sdsi/intel_sdsi.c
> +++ b/tools/arch/x86/intel_sdsi/intel_sdsi.c
> @@ -153,11 +153,12 @@ struct bundle_encoding {
>  };
>  
>  struct meter_certificate {
> -	uint32_t block_signature;
> +	uint32_t signature;
> +	uint32_t version;
> +	uint64_t ppin;
>  	uint32_t counter_unit;
> -	uint64_t ppin;
>  	uint32_t bundle_length;
> -	uint32_t reserved;
> +	uint64_t reserved;
>  	uint32_t mmrc_encoding;
>  	uint32_t mmrc_counter;
>  };
> @@ -336,6 +337,7 @@ static int sdsi_meter_cert_show(struct sdsi_dev *s)
>  	uint32_t count = 0;
>  	FILE *cert_ptr;
>  	int ret, size;
> +	char name[4];

Nit: IMO, instead of hardcoding, you can introduce a macro like FEATURE_LEN.
It is up to you.

>  
>  	ret = sdsi_update_registers(s);
>  	if (ret)
> @@ -377,12 +379,19 @@ static int sdsi_meter_cert_show(struct sdsi_dev *s)
>  	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);
> +
> +	get_feature(mc->signature, name);
> +	printf("Signature:                    %.4s\n", name);
> +
> +	printf("Version:                      %d\n", mc->version);
> +	printf("Count Unit:                   %dms\n", mc->counter_unit);
> +	printf("PPIN:                         0x%lx\n", mc->ppin);
> +	printf("Feature Bundle Length:        %d\n", mc->bundle_length);
> +
> +	get_feature(mc->mmrc_encoding, name);
> +	printf("MMRC encoding:                %.4s\n", name);
> +
> +	printf("MMRC counter:                 %d\n", mc->mmrc_counter);
>  	if (mc->bundle_length % 8) {
>  		fprintf(stderr, "Invalid bundle length\n");
>  		return -1;
> @@ -396,7 +405,7 @@ static int sdsi_meter_cert_show(struct sdsi_dev *s)
>  
>  	bec = (void *)(mc) + sizeof(mc);
>  
> -	printf("Number of Feature Counters:          %d\n", mc->bundle_length / 8);
> +	printf("Number of Feature Counters:   %d\n", mc->bundle_length / 8);

Try to use sizeof instead of hardcoded value.

>  	while (count++ < mc->bundle_length / 8) {
>  		char feature[5];
>  

-- 
Sathyanarayanan Kuppuswamy
Linux Kernel Developer

Re: [PATCH V2 7/9] tools/arch/x86/intel_sdsi: Fix meter_certificate decoding

[Kuppuswamy Sathyanarayanan]

On 2/27/24 4:00 PM, David E. Box wrote:
> Fix errors in the calculation of the start position of the counters and in
> the display loop.
>
> Fixes: aad129780bae ("platform/x86/intel/sdsi: Add support for reading the current meter state")
> Signed-off-by: David E. Box <david.e.box@linux.intel.com>
> ---

Looks good to me.

Reviewed-by: Kuppuswamy Sathyanarayanan <sathyanarayanan.kuppuswamy@linux.intel.com>

>
> V2 - Split of V1 patch 7
>
>  tools/arch/x86/intel_sdsi/intel_sdsi.c | 5 +++--
>  1 file changed, 3 insertions(+), 2 deletions(-)
>
> diff --git a/tools/arch/x86/intel_sdsi/intel_sdsi.c b/tools/arch/x86/intel_sdsi/intel_sdsi.c
> index def1b9a01738..2acc6be5f5d6 100644
> --- a/tools/arch/x86/intel_sdsi/intel_sdsi.c
> +++ b/tools/arch/x86/intel_sdsi/intel_sdsi.c
> @@ -403,15 +403,16 @@ static int sdsi_meter_cert_show(struct sdsi_dev *s)
>  		return -1;
>  	}
>  
> -	bec = (void *)(mc) + sizeof(mc);
> +	bec = (void *)(mc) + sizeof(*mc);
>  
>  	printf("Number of Feature Counters:   %d\n", mc->bundle_length / 8);
> -	while (count++ < 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);
> +		++count;
>  	}
>  
>  	return 0;

-- 
Sathyanarayanan Kuppuswamy
Linux Kernel Developer

Re: [PATCH V2 8/9] platform/x86/intel/sdsi: Simplify ascii printing

[Kuppuswamy Sathyanarayanan]

On 2/27/24 4:00 PM, David E. Box wrote:
> Use printf width specifier to set the display length of encoded feature
> names.
>
> Signed-off-by: David E. Box <david.e.box@linux.intel.com>
> ---

Looks fine to me.

Reviewed-by: Kuppuswamy Sathyanarayanan <sathyanarayanan.kuppuswamy@linux.intel.com>

> V2 - Split of V1 patch 7
>
>  tools/arch/x86/intel_sdsi/intel_sdsi.c | 11 ++++-------
>  1 file changed, 4 insertions(+), 7 deletions(-)
>
> diff --git a/tools/arch/x86/intel_sdsi/intel_sdsi.c b/tools/arch/x86/intel_sdsi/intel_sdsi.c
> index 2acc6be5f5d6..a8fb6d17405f 100644
> --- a/tools/arch/x86/intel_sdsi/intel_sdsi.c
> +++ b/tools/arch/x86/intel_sdsi/intel_sdsi.c
> @@ -407,11 +407,10 @@ static int sdsi_meter_cert_show(struct sdsi_dev *s)
>  
>  	printf("Number of Feature Counters:   %d\n", mc->bundle_length / 8);
>  	while (count < mc->bundle_length / 8) {
> -		char feature[5];
> +		char feature[4];
>  
> -		feature[4] = '\0';
>  		get_feature(bec[count].encoding, feature);
> -		printf("    %s:          %d\n", feature, bec[count].counter);
> +		printf("    %.4s:          %d\n", feature, bec[count].counter);
>  		++count;
>  	}
>  
> @@ -492,7 +491,7 @@ static int sdsi_state_cert_show(struct sdsi_dev *s)
>  			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];
> +		char feature[4];
>  		uint32_t i;
>  
>  		printf("     Blob %d:\n", count - 1);
> @@ -505,11 +504,9 @@ static int sdsi_state_cert_show(struct sdsi_dev *s)
>  		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("                 Feature %d:         %.4s\n", i, feature);
>  		}
>  
>  		if (lbc->num_bundles > STATE_MAX_NUM_IN_BUNDLE)

-- 
Sathyanarayanan Kuppuswamy
Linux Kernel Developer

Re: [PATCH V2 9/9] tools: intel_sdsi: Add current meter support

[Kuppuswamy Sathyanarayanan]

On 2/27/24 4:00 PM, David E. Box wrote:
> Add support to read the 'meter_current' file. The display is the same as
> the 'meter_certificate', but will show the current snapshot of the
> counters.
>
> Signed-off-by: David E. Box <david.e.box@linux.intel.com>
> ---

Looks good to me.

Reviewed-by: Kuppuswamy Sathyanarayanan <sathyanarayanan.kuppuswamy@linux.intel.com>
>
> V2 - Set the name of the file to be opened once.
>
>  tools/arch/x86/intel_sdsi/intel_sdsi.c | 49 ++++++++++++++++----------
>  1 file changed, 30 insertions(+), 19 deletions(-)
>
> diff --git a/tools/arch/x86/intel_sdsi/intel_sdsi.c b/tools/arch/x86/intel_sdsi/intel_sdsi.c
> index a8fb6d17405f..325e1e41af1d 100644
> --- a/tools/arch/x86/intel_sdsi/intel_sdsi.c
> +++ b/tools/arch/x86/intel_sdsi/intel_sdsi.c
> @@ -182,6 +182,7 @@ struct sdsi_dev {
>  enum command {
>  	CMD_SOCKET_INFO,
>  	CMD_METER_CERT,
> +	CMD_METER_CURRENT_CERT,
>  	CMD_STATE_CERT,
>  	CMD_PROV_AKC,
>  	CMD_PROV_CAP,
> @@ -329,13 +330,14 @@ static void get_feature(uint32_t encoding, char *feature)
>  	feature[0] = name[3];
>  }
>  
> -static int sdsi_meter_cert_show(struct sdsi_dev *s)
> +static int sdsi_meter_cert_show(struct sdsi_dev *s, bool show_current)
>  {
>  	char buf[METER_CERT_MAX_SIZE] = {0};
>  	struct bundle_encoding_counter *bec;
>  	struct meter_certificate *mc;
>  	uint32_t count = 0;
>  	FILE *cert_ptr;
> +	char *cert_fname;
Nit: I think this can be const char *
>  	int ret, size;
>  	char name[4];
>  
> @@ -345,7 +347,6 @@ static int sdsi_meter_cert_show(struct sdsi_dev *s)
>  
>  	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;
>  	}
>  
> @@ -360,15 +361,17 @@ static int sdsi_meter_cert_show(struct sdsi_dev *s)
>  		return ret;
>  	}
>  
> -	cert_ptr = fopen("meter_certificate", "r");
> +	cert_fname = show_current ? "meter_current" : "meter_certificate";
> +	cert_ptr = fopen(cert_fname, "r");
> +
>  	if (!cert_ptr) {
> -		perror("Could not open 'meter_certificate' file");
> +		fprintf(stderr, "Could not open '%s' file: %s", cert_fname, strerror(errno));
>  		return -1;
>  	}
>  
>  	size = fread(buf, 1, sizeof(buf), cert_ptr);
>  	if (!size) {
> -		fprintf(stderr, "Could not read 'meter_certificate' file\n");
> +		fprintf(stderr, "Could not read '%s' file\n", cert_fname);
>  		fclose(cert_ptr);
>  		return -1;
>  	}
> @@ -734,7 +737,7 @@ static void sdsi_free_dev(struct sdsi_dev *s)
>  
>  static void usage(char *prog)
>  {
> -	printf("Usage: %s [-l] [-d DEVNO [-i] [-s] [-m] [-a FILE] [-c FILE]]\n", prog);
> +	printf("Usage: %s [-l] [-d DEVNO [-i] [-s] [-m | -C] [-a FILE] [-c FILE]\n", prog);
>  }
>  
>  static void show_help(void)
> @@ -743,8 +746,9 @@ static void show_help(void)
>  	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", "-m, --meter",          "show meter certificate");
> +	printf("  %-18s\t%s\n", "-s, --state",          "show state certificate data");
> +	printf("  %-18s\t%s\n", "-m, --meter",          "show meter certificate data");
> +	printf("  %-18s\t%s\n", "-C, --meter_current",  "show live unattested meter 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");
>  }
> @@ -760,21 +764,22 @@ int main(int argc, char *argv[])
>  	int option_index = 0;
>  
>  	static struct option long_options[] = {
> -		{"akc",		required_argument,	0, 'a'},
> -		{"cap",		required_argument,	0, 'c'},
> -		{"devno",	required_argument,	0, 'd'},
> -		{"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 }
> +		{"akc",			required_argument,	0, 'a'},
> +		{"cap",			required_argument,	0, 'c'},
> +		{"devno",		required_argument,	0, 'd'},
> +		{"help",		no_argument,		0, 'h'},
> +		{"info",		no_argument,		0, 'i'},
> +		{"list",		no_argument,		0, 'l'},
> +		{"meter",		no_argument,		0, 'm'},
> +		{"meter_current",	no_argument,		0, 'C'},
> +		{"state",		no_argument,		0, 's'},
> +		{0,			0,			0, 0 }
>  	};
>  
>  
>  	progname = argv[0];
>  
> -	while ((opt = getopt_long_only(argc, argv, "+a:c:d:hilms", long_options,
> +	while ((opt = getopt_long_only(argc, argv, "+a:c:d:hilmCs", long_options,
>  			&option_index)) != -1) {
>  		switch (opt) {
>  		case 'd':
> @@ -790,6 +795,9 @@ int main(int argc, char *argv[])
>  		case 'm':
>  			command = CMD_METER_CERT;
>  			break;
> +		case 'C':
> +			command = CMD_METER_CURRENT_CERT;
> +			break;
>  		case 's':
>  			command = CMD_STATE_CERT;
>  			break;
> @@ -828,7 +836,10 @@ int main(int argc, char *argv[])
>  			ret = sdsi_read_reg(s);
>  			break;
>  		case CMD_METER_CERT:
> -			ret = sdsi_meter_cert_show(s);
> +			ret = sdsi_meter_cert_show(s, false);
> +			break;
> +		case CMD_METER_CURRENT_CERT:
> +			ret = sdsi_meter_cert_show(s, true);
>  			break;
>  		case CMD_STATE_CERT:
>  			ret = sdsi_state_cert_show(s);

-- 
Sathyanarayanan Kuppuswamy
Linux Kernel Developer

Re: [PATCH V2 2/9] platform/x86/intel/sdsi: Combine read and write mailbox flows

[David E. Box]

On Tue, 2024-02-27 at 18:38 -0800, Kuppuswamy Sathyanarayanan wrote:
> 
> On 2/27/24 4:00 PM, David E. Box wrote:
> > The current mailbox commands are either read-only or write-only and the
> > flow is different for each. New commands will need to send and receive
> > data. In preparation for these commands, create a common polling function
> > to handle sending data and receiving in the same transaction.
> > 
> > Signed-off-by: David E. Box <david.e.box@linux.intel.com>
> > ---
> > 
> > V2 - In sdsi_cmd_read() remove unnecessary check for non-zero packet_size
> >      in do loop since the loop is exited earlier when packet_size is
> >      zero.
> > 
> >  drivers/platform/x86/intel/sdsi.c | 79 +++++++++++++++++--------------
> >  1 file changed, 44 insertions(+), 35 deletions(-)
> > 
> > diff --git a/drivers/platform/x86/intel/sdsi.c
> > b/drivers/platform/x86/intel/sdsi.c
> > index a70c071de6e2..d80c2dc0ce71 100644
> > --- a/drivers/platform/x86/intel/sdsi.c
> > +++ b/drivers/platform/x86/intel/sdsi.c
> > @@ -15,6 +15,7 @@
> >  #include <linux/iopoll.h>
> >  #include <linux/kernel.h>
> >  #include <linux/module.h>
> > +#include <linux/overflow.h>
> >  #include <linux/pci.h>
> >  #include <linux/slab.h>
> >  #include <linux/sysfs.h>
> > @@ -156,8 +157,8 @@ static int sdsi_status_to_errno(u32 status)
> >  	}
> >  }
> >  
> > -static int sdsi_mbox_cmd_read(struct sdsi_priv *priv, struct sdsi_mbox_info
> > *info,
> > -			      size_t *data_size)
> > +static int sdsi_mbox_poll(struct sdsi_priv *priv, struct sdsi_mbox_info
> > *info,
> > +			  size_t *data_size)
> >  {
> >  	struct device *dev = priv->dev;
> >  	u32 total, loop, eom, status, message_size;
> > @@ -166,18 +167,10 @@ static int sdsi_mbox_cmd_read(struct sdsi_priv *priv,
> > struct sdsi_mbox_info *inf
> >  
> >  	lockdep_assert_held(&priv->mb_lock);
> >  
> > -	/* Format and send the read command */
> > -	control = FIELD_PREP(CTRL_EOM, 1) |
> > -		  FIELD_PREP(CTRL_SOM, 1) |
> > -		  FIELD_PREP(CTRL_RUN_BUSY, 1) |
> > -		  FIELD_PREP(CTRL_PACKET_SIZE, info->size);
> > -	writeq(control, priv->control_addr);
> > -
> >  	/* For reads, data sizes that are larger than the mailbox size are
> > read in packets. */
> >  	total = 0;
> >  	loop = 0;
> >  	do {
> > -		void *buf = info->buffer + (SDSI_SIZE_MAILBOX * loop);
> >  		u32 packet_size;
> >  
> >  		/* Poll on ready bit */
> > @@ -195,6 +188,11 @@ static int sdsi_mbox_cmd_read(struct sdsi_priv *priv,
> > struct sdsi_mbox_info *inf
> >  		if (ret)
> >  			break;
> >  
> > +		if (!packet_size) {
> > +			sdsi_complete_transaction(priv);
> > +			break;
> > +		}
> > +
> >  		/* Only the last packet can be less than the mailbox size.
> > */
> >  		if (!eom && packet_size != SDSI_SIZE_MAILBOX) {
> >  			dev_err(dev, "Invalid packet size\n");
> > @@ -208,9 +206,13 @@ static int sdsi_mbox_cmd_read(struct sdsi_priv *priv,
> > struct sdsi_mbox_info *inf
> >  			break;
> >  		}
> >  
> > -		sdsi_memcpy64_fromio(buf, priv->mbox_addr,
> > round_up(packet_size, SDSI_SIZE_CMD));
> > +		if (info->buffer) {
> > +			void *buf = info->buffer +
> > array_size(SDSI_SIZE_MAILBOX, loop);
> >  
> > -		total += packet_size;
> > +			sdsi_memcpy64_fromio(buf, priv->mbox_addr,
> > +					     round_up(packet_size,
> > SDSI_SIZE_CMD));
> > +			total += packet_size;
> > +		}
> >  
> >  		sdsi_complete_transaction(priv);
> >  	} while (!eom && ++loop < MBOX_MAX_PACKETS);
> > @@ -230,16 +232,33 @@ static int sdsi_mbox_cmd_read(struct sdsi_priv *priv,
> > struct sdsi_mbox_info *inf
> >  		dev_warn(dev, "Read count %u differs from expected count
> > %u\n",
> >  			 total, message_size);
> >  
> > -	*data_size = total;
> > +	if (data_size)
> > +		*data_size = total;
> >  
> 
> Is there a chance for it to be NULL with current callers?

Yes. Write only callers set this to NULL.

David

Re: [PATCH V2 2/9] platform/x86/intel/sdsi: Combine read and write mailbox flows

[Kuppuswamy Sathyanarayanan]

On 2/27/24 4:00 PM, David E. Box wrote:
> The current mailbox commands are either read-only or write-only and the
> flow is different for each. New commands will need to send and receive
> data. In preparation for these commands, create a common polling function
> to handle sending data and receiving in the same transaction.
>
> Signed-off-by: David E. Box <david.e.box@linux.intel.com>
> ---

Looks good to me.

Reviewed-by: Kuppuswamy Sathyanarayanan <sathyanarayanan.kuppuswamy@linux.intel.com>
> V2 - In sdsi_cmd_read() remove unnecessary check for non-zero packet_size
>      in do loop since the loop is exited earlier when packet_size is
>      zero.
>
>  drivers/platform/x86/intel/sdsi.c | 79 +++++++++++++++++--------------
>  1 file changed, 44 insertions(+), 35 deletions(-)
>
> diff --git a/drivers/platform/x86/intel/sdsi.c b/drivers/platform/x86/intel/sdsi.c
> index a70c071de6e2..d80c2dc0ce71 100644
> --- a/drivers/platform/x86/intel/sdsi.c
> +++ b/drivers/platform/x86/intel/sdsi.c
> @@ -15,6 +15,7 @@
>  #include <linux/iopoll.h>
>  #include <linux/kernel.h>
>  #include <linux/module.h>
> +#include <linux/overflow.h>
>  #include <linux/pci.h>
>  #include <linux/slab.h>
>  #include <linux/sysfs.h>
> @@ -156,8 +157,8 @@ static int sdsi_status_to_errno(u32 status)
>  	}
>  }
>  
> -static int sdsi_mbox_cmd_read(struct sdsi_priv *priv, struct sdsi_mbox_info *info,
> -			      size_t *data_size)
> +static int sdsi_mbox_poll(struct sdsi_priv *priv, struct sdsi_mbox_info *info,
> +			  size_t *data_size)
>  {
>  	struct device *dev = priv->dev;
>  	u32 total, loop, eom, status, message_size;
> @@ -166,18 +167,10 @@ static int sdsi_mbox_cmd_read(struct sdsi_priv *priv, struct sdsi_mbox_info *inf
>  
>  	lockdep_assert_held(&priv->mb_lock);
>  
> -	/* Format and send the read command */
> -	control = FIELD_PREP(CTRL_EOM, 1) |
> -		  FIELD_PREP(CTRL_SOM, 1) |
> -		  FIELD_PREP(CTRL_RUN_BUSY, 1) |
> -		  FIELD_PREP(CTRL_PACKET_SIZE, info->size);
> -	writeq(control, priv->control_addr);
> -
>  	/* For reads, data sizes that are larger than the mailbox size are read in packets. */
>  	total = 0;
>  	loop = 0;
>  	do {
> -		void *buf = info->buffer + (SDSI_SIZE_MAILBOX * loop);
>  		u32 packet_size;
>  
>  		/* Poll on ready bit */
> @@ -195,6 +188,11 @@ static int sdsi_mbox_cmd_read(struct sdsi_priv *priv, struct sdsi_mbox_info *inf
>  		if (ret)
>  			break;
>  
> +		if (!packet_size) {
> +			sdsi_complete_transaction(priv);
> +			break;
> +		}
> +
>  		/* Only the last packet can be less than the mailbox size. */
>  		if (!eom && packet_size != SDSI_SIZE_MAILBOX) {
>  			dev_err(dev, "Invalid packet size\n");
> @@ -208,9 +206,13 @@ static int sdsi_mbox_cmd_read(struct sdsi_priv *priv, struct sdsi_mbox_info *inf
>  			break;
>  		}
>  
> -		sdsi_memcpy64_fromio(buf, priv->mbox_addr, round_up(packet_size, SDSI_SIZE_CMD));
> +		if (info->buffer) {
> +			void *buf = info->buffer + array_size(SDSI_SIZE_MAILBOX, loop);
>  
> -		total += packet_size;
> +			sdsi_memcpy64_fromio(buf, priv->mbox_addr,
> +					     round_up(packet_size, SDSI_SIZE_CMD));
> +			total += packet_size;
> +		}
>  
>  		sdsi_complete_transaction(priv);
>  	} while (!eom && ++loop < MBOX_MAX_PACKETS);
> @@ -230,16 +232,33 @@ static int sdsi_mbox_cmd_read(struct sdsi_priv *priv, struct sdsi_mbox_info *inf
>  		dev_warn(dev, "Read count %u differs from expected count %u\n",
>  			 total, message_size);
>  
> -	*data_size = total;
> +	if (data_size)
> +		*data_size = total;
>  
>  	return 0;
>  }
>  
> -static int sdsi_mbox_cmd_write(struct sdsi_priv *priv, struct sdsi_mbox_info *info)
> +static int sdsi_mbox_cmd_read(struct sdsi_priv *priv, struct sdsi_mbox_info *info,
> +			      size_t *data_size)
> +{
> +	u64 control;
> +
> +	lockdep_assert_held(&priv->mb_lock);
> +
> +	/* Format and send the read command */
> +	control = FIELD_PREP(CTRL_EOM, 1) |
> +		  FIELD_PREP(CTRL_SOM, 1) |
> +		  FIELD_PREP(CTRL_RUN_BUSY, 1) |
> +		  FIELD_PREP(CTRL_PACKET_SIZE, info->size);
> +	writeq(control, priv->control_addr);
> +
> +	return sdsi_mbox_poll(priv, info, data_size);
> +}
> +
> +static int sdsi_mbox_cmd_write(struct sdsi_priv *priv, struct sdsi_mbox_info *info,
> +			       size_t *data_size)
>  {
>  	u64 control;
> -	u32 status;
> -	int ret;
>  
>  	lockdep_assert_held(&priv->mb_lock);
>  
> @@ -256,20 +275,7 @@ static int sdsi_mbox_cmd_write(struct sdsi_priv *priv, struct sdsi_mbox_info *in
>  		  FIELD_PREP(CTRL_PACKET_SIZE, info->size);
>  	writeq(control, priv->control_addr);
>  
> -	/* Poll on ready bit */
> -	ret = readq_poll_timeout(priv->control_addr, control, control & CTRL_READY,
> -				 MBOX_POLLING_PERIOD_US, MBOX_TIMEOUT_US);
> -
> -	if (ret)
> -		goto release_mbox;
> -
> -	status = FIELD_GET(CTRL_STATUS, control);
> -	ret = sdsi_status_to_errno(status);
> -
> -release_mbox:
> -	sdsi_complete_transaction(priv);
> -
> -	return ret;
> +	return sdsi_mbox_poll(priv, info, data_size);
>  }
>  
>  static int sdsi_mbox_acquire(struct sdsi_priv *priv, struct sdsi_mbox_info *info)
> @@ -313,7 +319,8 @@ static int sdsi_mbox_acquire(struct sdsi_priv *priv, struct sdsi_mbox_info *info
>  	return ret;
>  }
>  
> -static int sdsi_mbox_write(struct sdsi_priv *priv, struct sdsi_mbox_info *info)
> +static int sdsi_mbox_write(struct sdsi_priv *priv, struct sdsi_mbox_info *info,
> +			   size_t *data_size)
>  {
>  	int ret;
>  
> @@ -323,7 +330,7 @@ static int sdsi_mbox_write(struct sdsi_priv *priv, struct sdsi_mbox_info *info)
>  	if (ret)
>  		return ret;
>  
> -	return sdsi_mbox_cmd_write(priv, info);
> +	return sdsi_mbox_cmd_write(priv, info, data_size);
>  }
>  
>  static int sdsi_mbox_read(struct sdsi_priv *priv, struct sdsi_mbox_info *info, size_t *data_size)
> @@ -342,7 +349,7 @@ static int sdsi_mbox_read(struct sdsi_priv *priv, struct sdsi_mbox_info *info, s
>  static ssize_t sdsi_provision(struct sdsi_priv *priv, char *buf, size_t count,
>  			      enum sdsi_command command)
>  {
> -	struct sdsi_mbox_info info;
> +	struct sdsi_mbox_info info = {};
>  	int ret;
>  
>  	if (count > (SDSI_SIZE_WRITE_MSG - SDSI_SIZE_CMD))
> @@ -364,7 +371,9 @@ static ssize_t sdsi_provision(struct sdsi_priv *priv, char *buf, size_t count,
>  	ret = mutex_lock_interruptible(&priv->mb_lock);
>  	if (ret)
>  		goto free_payload;
> -	ret = sdsi_mbox_write(priv, &info);
> +
> +	ret = sdsi_mbox_write(priv, &info, NULL);
> +
>  	mutex_unlock(&priv->mb_lock);
>  
>  free_payload:
> @@ -408,7 +417,7 @@ static ssize_t
>  certificate_read(u64 command, struct sdsi_priv *priv, char *buf, loff_t off,
>  		 size_t count)
>  {
> -	struct sdsi_mbox_info info;
> +	struct sdsi_mbox_info info = {};
>  	size_t size;
>  	int ret;
>  

-- 
Sathyanarayanan Kuppuswamy
Linux Kernel Developer

Re: [PATCH V2 2/9] platform/x86/intel/sdsi: Combine read and write mailbox flows

[Ilpo Järvinen]

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

On Tue, 27 Feb 2024, Kuppuswamy Sathyanarayanan wrote:
> On 2/27/24 4:00 PM, David E. Box wrote:
> > The current mailbox commands are either read-only or write-only and the
> > flow is different for each. New commands will need to send and receive
> > data. In preparation for these commands, create a common polling function
> > to handle sending data and receiving in the same transaction.
> >
> > Signed-off-by: David E. Box <david.e.box@linux.intel.com>
> > ---
> >
> > V2 - In sdsi_cmd_read() remove unnecessary check for non-zero packet_size
> >      in do loop since the loop is exited earlier when packet_size is
> >      zero.

Reviewed-by: Ilpo Järvinen <ilpo.jarvinen@linux.intel.com>

-- 
 i.

Re: [PATCH V2 3/9] platform/x86/intel/sdsi: Add in-band BIOS lock support

[Ilpo Järvinen]

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

On Tue, 27 Feb 2024, David E. Box wrote:

> From: Kuppuswamy Sathyanarayanan <sathyanarayanan.kuppuswamy@linux.intel.com>
> 
> As per SDSi in-band interface specification, sec titled "BIOS lock for
> in-band provisioning", when IB_LOCK bit is set in control qword, the
> SDSI agent is only allowed to perform the read flow, but not allowed to
> provision license blob or license key. So add check for it in
> sdsi_provision().
> 
> Signed-off-by: Kuppuswamy Sathyanarayanan <sathyanarayanan.kuppuswamy@linux.intel.com>
> Signed-off-by: David E. Box <david.e.box@linux.intel.com>
> ---
> 
> V2 - Move sdsi_ib_locked() check after overflow check
> 
>  drivers/platform/x86/intel/sdsi.c | 10 ++++++++++
>  1 file changed, 10 insertions(+)
> 
> diff --git a/drivers/platform/x86/intel/sdsi.c b/drivers/platform/x86/intel/sdsi.c
> index d80c2dc0ce71..bb3eaf5eb382 100644
> --- a/drivers/platform/x86/intel/sdsi.c
> +++ b/drivers/platform/x86/intel/sdsi.c
> @@ -67,6 +67,7 @@
>  #define CTRL_OWNER			GENMASK(5, 4)
>  #define CTRL_COMPLETE			BIT(6)
>  #define CTRL_READY			BIT(7)
> +#define CTRL_INBAND_LOCK		BIT(32)
>  #define CTRL_STATUS			GENMASK(15, 8)
>  #define CTRL_PACKET_SIZE		GENMASK(31, 16)
>  #define CTRL_MSG_SIZE			GENMASK(63, 48)
> @@ -346,6 +347,11 @@ static int sdsi_mbox_read(struct sdsi_priv *priv, struct sdsi_mbox_info *info, s
>  	return sdsi_mbox_cmd_read(priv, info, data_size);
>  }
>  
> +static bool sdsi_ib_locked(struct sdsi_priv *priv)
> +{
> +	return !!FIELD_GET(CTRL_INBAND_LOCK, readq(priv->control_addr));
> +}
> +
>  static ssize_t sdsi_provision(struct sdsi_priv *priv, char *buf, size_t count,
>  			      enum sdsi_command command)
>  {
> @@ -355,6 +361,10 @@ static ssize_t sdsi_provision(struct sdsi_priv *priv, char *buf, size_t count,
>  	if (count > (SDSI_SIZE_WRITE_MSG - SDSI_SIZE_CMD))
>  		return -EOVERFLOW;
>  
> +	/* Make sure In-band lock is not set */
> +	if (sdsi_ib_locked(priv))
> +		return -EPERM;
> +

Reviewed-by: Ilpo Järvinen <ilpo.jarvinen@linux.intel.com>

-- 
 i.

Re: [PATCH V2 4/9] platform/x86/intel/sdsi: Add attribute to read the current meter state

[Ilpo Järvinen]

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

On Tue, 27 Feb 2024, David E. Box wrote:

> The meter_certificate file provides access to metering information that may
> be attested but is only updated every 8 hours. Add new attribute,
> meter_current, to allow reading an untested snapshot of the current values.
> 
> Signed-off-by: David E. Box <david.e.box@linux.intel.com>
> ---
> 
> V2 - make control_flags a parameter to be eventually passed to
>      sdsi_mbox_cmd_read(). This removes the need for a lock which had been
>      added to protect control_flags when it was a member of the private
>      struct.

Yes, thanks. It's more obvious now what's going on w/o all that lock 
trickery.

Reviewed-by: Ilpo Järvinen <ilpo.jarvinen@linux.intel.com>

-- 
 i.

Re: [PATCH V2 7/9] tools/arch/x86/intel_sdsi: Fix meter_certificate decoding

[Ilpo Järvinen]

On Tue, 27 Feb 2024, David E. Box wrote:

> Fix errors in the calculation of the start position of the counters and in
> the display loop.
> 
> Fixes: aad129780bae ("platform/x86/intel/sdsi: Add support for reading the current meter state")
> Signed-off-by: David E. Box <david.e.box@linux.intel.com>
> ---
> 
> V2 - Split of V1 patch 7
> 
>  tools/arch/x86/intel_sdsi/intel_sdsi.c | 5 +++--
>  1 file changed, 3 insertions(+), 2 deletions(-)
> 
> diff --git a/tools/arch/x86/intel_sdsi/intel_sdsi.c b/tools/arch/x86/intel_sdsi/intel_sdsi.c
> index def1b9a01738..2acc6be5f5d6 100644
> --- a/tools/arch/x86/intel_sdsi/intel_sdsi.c
> +++ b/tools/arch/x86/intel_sdsi/intel_sdsi.c
> @@ -403,15 +403,16 @@ static int sdsi_meter_cert_show(struct sdsi_dev *s)
>  		return -1;
>  	}
>  
> -	bec = (void *)(mc) + sizeof(mc);
> +	bec = (void *)(mc) + sizeof(*mc);
>  
>  	printf("Number of Feature Counters:   %d\n", mc->bundle_length / 8);
> -	while (count++ < mc->bundle_length / 8) {
> +	while (count < mc->bundle_length / 8) {

One more sizeof() case here that wasn't pointed up earlier.


-- 
 i.

Re: [PATCH V2 9/9] tools: intel_sdsi: Add current meter support

[Ilpo Järvinen]

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

On Tue, 27 Feb 2024, David E. Box wrote:

> Add support to read the 'meter_current' file. The display is the same as
> the 'meter_certificate', but will show the current snapshot of the
> counters.
> 
> Signed-off-by: David E. Box <david.e.box@linux.intel.com>
> ---
> 
> V2 - Set the name of the file to be opened once.

Reviewed-by: Ilpo Järvinen <ilpo.jarvinen@linux.intel.com>

-- 
 i.