[PATCH v3 0/3] block: sed-opal: Generic Read/Write Opal Tables

[PATCH v3 0/3] block: sed-opal: Generic Read/Write Opal Tables

[Revanth Rajashekar]

This series of patches aims at extending SED Opal support:
1. Generalizing write data to any opal table
2. Add an IOCTL for reading/writing any Opal Table with Admin-1 authority
3. Introduce Opal Datastore UID, which can be accessed using above ioctl

Datastore feature described in:
https://trustedcomputinggroup.org/wp-content/uploads/TCG_Storage-Opal_Feature_Set-Additional_DataStore_Tables_v1_00_r1_00_Final.pdf

Opal Application Note:
https://trustedcomputinggroup.org/wp-content/uploads/TCG_Storage_Opal_SSC_Application_Note_1-00_1-00-Final.pdf

This feature has been successfully tested on OPAL Datastore and MBR table using
internal tools with an Intel SSD and an Intel Optane.

Changes from v2:
	1. Drop a patch which exposes UIDs in UAPI.
	2. Fix coding styles wherever required based on LKML feedbacks.
	3. Eliminate a few redundant assignments in the code.
	4. Add a break under copy_from_user error condition in
           generic_table_write_data func.
	5. A few refactoring/cleanups in both the patches.
	6. Introduce a new patch which introduces Opal Datastore table UID.

Changes from v1:
	1. Fix the spelling mistake in the commit message.
	2. Introduce a length check condition before Copy To User in
           opal_read_table function to facilitate user with easy debugging.
	3. Introduce switch cases in the opal_generic_read_write_table ioctl
           function.
	4. Move read/write table opal_step to discrete functions to reduce the
           load on the ioctl function.
	5. Introduce 'opal table operations' enumeration in uapi.
	6. Remove tabs before the #defines in opal_read_write_table structure
           to improve the code readability.
	7. Drop a patch which exposes UIDs in UAPI.
	8. Eliminate a few redundant assignments in the code.
	9. Add a break under copy_from_user error condition in
           generic_table_write_data func.
	10. A few refactoring/cleanups in both the patches
	11. Introduce a new patch which introduces Opal Datastore table UID.


Revanth Rajashekar (3):
  block: sed-opal: Generalizing write data to any opal table
  block: sed-opal: Add support to read/write opal tables generically
  block: sed-opal: Introduce Opal Datastore UID

 block/opal_proto.h            |   2 +-
 block/sed-opal.c              | 312 +++++++++++++++++++++++++++-------
 include/linux/sed-opal.h      |   1 +
 include/uapi/linux/sed-opal.h |  20 +++
 4 files changed, 270 insertions(+), 65 deletions(-)

-- 
2.17.1

[PATCH v3 1/3] block: sed-opal: Generalizing write data to any opal table

[Revanth Rajashekar]

This patch refactors the existing "write_shadowmbr" func and
creates a new generalized function "generic_table_write_data",
to write data to any opal table. Also, a few cleanups are included
in this patch.

Reviewed-by: Jon Derrick <jonathan.derrick@intel.com>
Signed-off-by: Revanth Rajashekar <revanth.rajashekar@intel.com>
---
 block/sed-opal.c | 138 +++++++++++++++++++++++++----------------------
 1 file changed, 74 insertions(+), 64 deletions(-)

diff --git a/block/sed-opal.c b/block/sed-opal.c
index b4c761973ac1..d6e2ec0d8a3a 100644
--- a/block/sed-opal.c
+++ b/block/sed-opal.c
@@ -1139,11 +1139,11 @@ static int generic_get_column(struct opal_dev *dev, const u8 *table,
  *
  * the result is provided in dev->resp->tok[4]
  */
-static int generic_get_table_info(struct opal_dev *dev, enum opal_uid table,
+static int generic_get_table_info(struct opal_dev *dev, const u8 *table_uid,
 				  u64 column)
 {
 	u8 uid[OPAL_UID_LENGTH];
-	const unsigned int half = OPAL_UID_LENGTH/2;
+	const unsigned int half = OPAL_UID_LENGTH_HALF;
 
 	/* sed-opal UIDs can be split in two halves:
 	 *  first:  actual table index
@@ -1152,7 +1152,7 @@ static int generic_get_table_info(struct opal_dev *dev, enum opal_uid table,
 	 * first part of the target table as relative index into that table
 	 */
 	memcpy(uid, opaluid[OPAL_TABLE_TABLE], half);
-	memcpy(uid+half, opaluid[table], half);
+	memcpy(uid + half, table_uid, half);
 
 	return generic_get_column(dev, uid, column);
 }
@@ -1221,6 +1221,75 @@ static int get_active_key(struct opal_dev *dev, void *data)
 	return get_active_key_cont(dev);
 }
 
+static int generic_table_write_data(struct opal_dev *dev, const u64 data,
+				    u64 offset, u64 size, const u8 *uid)
+{
+	const u8 __user *src = (u8 __user *)(uintptr_t)data;
+	u8 *dst;
+	u64 len;
+	size_t off = 0;
+	int err;
+
+	/* do we fit in the available space? */
+	err = generic_get_table_info(dev, uid, OPAL_TABLE_ROWS);
+	if (err) {
+		pr_debug("Couldn't get the table size\n");
+		return err;
+	}
+
+	len = response_get_u64(&dev->parsed, 4);
+	if (size > len || offset > len - size) {
+		pr_debug("Does not fit in the table (%llu vs. %llu)\n",
+			  offset + size, len);
+		return -ENOSPC;
+	}
+
+	/* do the actual transmission(s) */
+	while (off < size) {
+		err = cmd_start(dev, uid, opalmethod[OPAL_SET]);
+		add_token_u8(&err, dev, OPAL_STARTNAME);
+		add_token_u8(&err, dev, OPAL_WHERE);
+		add_token_u64(&err, dev, offset + off);
+		add_token_u8(&err, dev, OPAL_ENDNAME);
+
+		add_token_u8(&err, dev, OPAL_STARTNAME);
+		add_token_u8(&err, dev, OPAL_VALUES);
+
+		/*
+		 * The bytestring header is either 1 or 2 bytes, so assume 2.
+		 * There also needs to be enough space to accommodate the
+		 * trailing OPAL_ENDNAME (1 byte) and tokens added by
+		 * cmd_finalize.
+		 */
+		len = min(remaining_size(dev) - (2+1+CMD_FINALIZE_BYTES_NEEDED),
+			  (size_t)(size - off));
+		pr_debug("Write bytes %zu+%llu/%llu\n", off, len, size);
+
+		dst = add_bytestring_header(&err, dev, len);
+		if (!dst)
+			break;
+
+		if (copy_from_user(dst, src + off, len)) {
+			err = -EFAULT;
+			break;
+		}
+
+		dev->pos += len;
+
+		add_token_u8(&err, dev, OPAL_ENDNAME);
+		if (err)
+			break;
+
+		err = finalize_and_send(dev, parse_and_check_status);
+		if (err)
+			break;
+
+		off += len;
+	}
+
+	return err;
+}
+
 static int generic_lr_enable_disable(struct opal_dev *dev,
 				     u8 *uid, bool rle, bool wle,
 				     bool rl, bool wl)
@@ -1583,68 +1652,9 @@ static int set_mbr_enable_disable(struct opal_dev *dev, void *data)
 static int write_shadow_mbr(struct opal_dev *dev, void *data)
 {
 	struct opal_shadow_mbr *shadow = data;
-	const u8 __user *src;
-	u8 *dst;
-	size_t off = 0;
-	u64 len;
-	int err = 0;
-
-	/* do we fit in the available shadow mbr space? */
-	err = generic_get_table_info(dev, OPAL_MBR, OPAL_TABLE_ROWS);
-	if (err) {
-		pr_debug("MBR: could not get shadow size\n");
-		return err;
-	}
-
-	len = response_get_u64(&dev->parsed, 4);
-	if (shadow->size > len || shadow->offset > len - shadow->size) {
-		pr_debug("MBR: does not fit in shadow (%llu vs. %llu)\n",
-			 shadow->offset + shadow->size, len);
-		return -ENOSPC;
-	}
-
-	/* do the actual transmission(s) */
-	src = (u8 __user *)(uintptr_t)shadow->data;
-	while (off < shadow->size) {
-		err = cmd_start(dev, opaluid[OPAL_MBR], opalmethod[OPAL_SET]);
-		add_token_u8(&err, dev, OPAL_STARTNAME);
-		add_token_u8(&err, dev, OPAL_WHERE);
-		add_token_u64(&err, dev, shadow->offset + off);
-		add_token_u8(&err, dev, OPAL_ENDNAME);
-
-		add_token_u8(&err, dev, OPAL_STARTNAME);
-		add_token_u8(&err, dev, OPAL_VALUES);
-
-		/*
-		 * The bytestring header is either 1 or 2 bytes, so assume 2.
-		 * There also needs to be enough space to accommodate the
-		 * trailing OPAL_ENDNAME (1 byte) and tokens added by
-		 * cmd_finalize.
-		 */
-		len = min(remaining_size(dev) - (2+1+CMD_FINALIZE_BYTES_NEEDED),
-			  (size_t)(shadow->size - off));
-		pr_debug("MBR: write bytes %zu+%llu/%llu\n",
-			 off, len, shadow->size);
-
-		dst = add_bytestring_header(&err, dev, len);
-		if (!dst)
-			break;
-		if (copy_from_user(dst, src + off, len))
-			err = -EFAULT;
-		dev->pos += len;
 
-		add_token_u8(&err, dev, OPAL_ENDNAME);
-		if (err)
-			break;
-
-		err = finalize_and_send(dev, parse_and_check_status);
-		if (err)
-			break;
-
-		off += len;
-	}
-
-	return err;
+	return generic_table_write_data(dev, shadow->data, shadow->offset,
+					shadow->size, opaluid[OPAL_MBR]);
 }
 
 static int generic_pw_cmd(u8 *key, size_t key_len, u8 *cpin_uid,
-- 
2.17.1

[PATCH v3 2/3] block: sed-opal: Add support to read/write opal tables generically

[Revanth Rajashekar]

This feature gives the user RW access to any opal table with admin1
authority. The flags described in the new structure determines if the user
wants to read/write the data. Flags are checked for valid values in
order to allow future features to be added to the ioctl.

The user can provide the desired table's UID. Also, the ioctl provides a
size and offset field and internally will loop data accesses to return
the full data block. Read overrun is prevented by the initiator's
sec_send_recv() backend. The ioctl provides a private field with the
intention to accommodate any future expansions to the ioctl.

Reviewed-by: Jon Derrick <jonathan.derrick@intel.com>
Signed-off-by: Revanth Rajashekar <revanth.rajashekar@intel.com>
---
 block/opal_proto.h            |   1 -
 block/sed-opal.c              | 172 ++++++++++++++++++++++++++++++++++
 include/linux/sed-opal.h      |   1 +
 include/uapi/linux/sed-opal.h |  20 ++++
 4 files changed, 193 insertions(+), 1 deletion(-)

diff --git a/block/opal_proto.h b/block/opal_proto.h
index 5532412d567c..710911864e1d 100644
--- a/block/opal_proto.h
+++ b/block/opal_proto.h
@@ -76,7 +76,6 @@ enum opal_response_token {
  * Derived from: TCG_Storage_Architecture_Core_Spec_v2.01_r1.00
  * Section: 6.3 Assigned UIDs
  */
-#define OPAL_UID_LENGTH 8
 #define OPAL_METHOD_LENGTH 8
 #define OPAL_MSID_KEYLEN 15
 #define OPAL_UID_LENGTH_HALF 4
diff --git a/block/sed-opal.c b/block/sed-opal.c
index d6e2ec0d8a3a..1d5afaaf0826 100644
--- a/block/sed-opal.c
+++ b/block/sed-opal.c
@@ -1967,6 +1967,113 @@ static int get_msid_cpin_pin(struct opal_dev *dev, void *data)
 	return 0;
 }
 
+static int write_table_data(struct opal_dev *dev, void *data)
+{
+	struct opal_read_write_table *write_tbl = data;
+
+	return generic_table_write_data(dev, write_tbl->data, write_tbl->offset,
+					write_tbl->size, write_tbl->table_uid);
+}
+
+static int read_table_data_cont(struct opal_dev *dev)
+{
+	int err;
+	const char *data_read;
+
+	err = parse_and_check_status(dev);
+	if (err)
+		return err;
+
+	dev->prev_d_len = response_get_string(&dev->parsed, 1, &data_read);
+	dev->prev_data = (void *)data_read;
+	if (!dev->prev_data) {
+		pr_debug("%s: Couldn't read data from the table.\n", __func__);
+		return OPAL_INVAL_PARAM;
+	}
+
+	return 0;
+}
+
+/*
+ * IO_BUFFER_LENGTH = 2048
+ * sizeof(header) = 56
+ * No. of Token Bytes in the Response = 11
+ * MAX size of data that can be carried in response buffer
+ * at a time is : 2048 - (56 + 11) = 1981 = 0x7BD.
+ */
+#define OPAL_MAX_READ_TABLE (0x7BD)
+
+static int read_table_data(struct opal_dev *dev, void *data)
+{
+	struct opal_read_write_table *read_tbl = data;
+	int err;
+	size_t off = 0, max_read_size = OPAL_MAX_READ_TABLE;
+	u64 table_len, len;
+	u64 offset = read_tbl->offset, read_size = read_tbl->size - 1;
+	u8 __user *dst;
+
+	err = generic_get_table_info(dev, read_tbl->table_uid, OPAL_TABLE_ROWS);
+	if (err) {
+		pr_debug("Couldn't get the table size\n");
+		return err;
+	}
+
+	table_len = response_get_u64(&dev->parsed, 4);
+
+	/* Check if the user is trying to read from the table limits */
+	if (read_size > table_len || offset > table_len - read_size) {
+		pr_debug("Read size exceeds the Table size limits (%llu vs. %llu)\n",
+			  offset + read_size, table_len);
+		return -EINVAL;
+	}
+
+	while (off < read_size) {
+		err = cmd_start(dev, read_tbl->table_uid, opalmethod[OPAL_GET]);
+
+		add_token_u8(&err, dev, OPAL_STARTLIST);
+		add_token_u8(&err, dev, OPAL_STARTNAME);
+		add_token_u8(&err, dev, OPAL_STARTROW);
+		add_token_u64(&err, dev, offset + off); /* start row value */
+		add_token_u8(&err, dev, OPAL_ENDNAME);
+
+		add_token_u8(&err, dev, OPAL_STARTNAME);
+		add_token_u8(&err, dev, OPAL_ENDROW);
+
+		len = min(max_read_size, (size_t)(read_size - off));
+		add_token_u64(&err, dev, offset + off + len); /* end row value
+							       */
+		add_token_u8(&err, dev, OPAL_ENDNAME);
+		add_token_u8(&err, dev, OPAL_ENDLIST);
+
+		if (err) {
+			pr_debug("Error building read table data command.\n");
+			break;
+		}
+
+		err = finalize_and_send(dev, read_table_data_cont);
+		if (err)
+			break;
+
+		/* len+1: This includes the NULL terminator at the end*/
+		if (dev->prev_d_len > len + 1) {
+			err = -EOVERFLOW;
+			break;
+		}
+
+		dst = (u8 __user *)(uintptr_t)read_tbl->data;
+		if (copy_to_user(dst + off, dev->prev_data, dev->prev_d_len)) {
+			pr_debug("Error copying data to userspace\n");
+			err = -EFAULT;
+			break;
+		}
+		dev->prev_data = NULL;
+
+		off += len;
+	}
+
+	return err;
+}
+
 static int end_opal_session(struct opal_dev *dev, void *data)
 {
 	int err = 0;
@@ -2453,6 +2560,68 @@ bool opal_unlock_from_suspend(struct opal_dev *dev)
 }
 EXPORT_SYMBOL(opal_unlock_from_suspend);
 
+static int opal_read_table(struct opal_dev *dev,
+			   struct opal_read_write_table *rw_tbl)
+{
+	const struct opal_step read_table_steps[] = {
+		{ start_admin1LSP_opal_session, &rw_tbl->key },
+		{ read_table_data, rw_tbl },
+		{ end_opal_session, }
+	};
+	int ret = 0;
+
+	if (!rw_tbl->size)
+		return ret;
+
+	return execute_steps(dev, read_table_steps,
+			     ARRAY_SIZE(read_table_steps));
+}
+
+static int opal_write_table(struct opal_dev *dev,
+			    struct opal_read_write_table *rw_tbl)
+{
+	const struct opal_step write_table_steps[] = {
+		{ start_admin1LSP_opal_session, &rw_tbl->key },
+		{ write_table_data, rw_tbl },
+		{ end_opal_session, }
+	};
+	int ret = 0;
+
+	if (!rw_tbl->size)
+		return ret;
+
+	return execute_steps(dev, write_table_steps,
+			     ARRAY_SIZE(write_table_steps));
+}
+
+static int opal_generic_read_write_table(struct opal_dev *dev,
+					 struct opal_read_write_table *rw_tbl)
+{
+	int ret, bit_set;
+
+	mutex_lock(&dev->dev_lock);
+	setup_opal_dev(dev);
+
+	bit_set = fls64(rw_tbl->flags) - 1;
+	switch (bit_set) {
+	case OPAL_READ_TABLE:
+		ret = opal_read_table(dev, rw_tbl);
+		break;
+	case OPAL_WRITE_TABLE:
+		ret = opal_write_table(dev, rw_tbl);
+		break;
+	default:
+		pr_debug("Invalid bit set in the flag (%016llx).\n",
+			 rw_tbl->flags);
+		ret = -EINVAL;
+		break;
+	}
+
+	mutex_unlock(&dev->dev_lock);
+
+	return ret;
+}
+
 int sed_ioctl(struct opal_dev *dev, unsigned int cmd, void __user *arg)
 {
 	void *p;
@@ -2515,6 +2684,9 @@ int sed_ioctl(struct opal_dev *dev, unsigned int cmd, void __user *arg)
 	case IOC_OPAL_PSID_REVERT_TPR:
 		ret = opal_reverttper(dev, p, true);
 		break;
+	case IOC_OPAL_GENERIC_TABLE_RW:
+		ret = opal_generic_read_write_table(dev, p);
+		break;
 	default:
 		break;
 	}
diff --git a/include/linux/sed-opal.h b/include/linux/sed-opal.h
index 53c28d750a45..1ac0d712a9c3 100644
--- a/include/linux/sed-opal.h
+++ b/include/linux/sed-opal.h
@@ -42,6 +42,7 @@ static inline bool is_sed_ioctl(unsigned int cmd)
 	case IOC_OPAL_PSID_REVERT_TPR:
 	case IOC_OPAL_MBR_DONE:
 	case IOC_OPAL_WRITE_SHADOW_MBR:
+	case IOC_OPAL_GENERIC_TABLE_RW:
 		return true;
 	}
 	return false;
diff --git a/include/uapi/linux/sed-opal.h b/include/uapi/linux/sed-opal.h
index c6d035fa1b6c..6f5af1a84213 100644
--- a/include/uapi/linux/sed-opal.h
+++ b/include/uapi/linux/sed-opal.h
@@ -113,6 +113,25 @@ struct opal_shadow_mbr {
 	__u64 size;
 };
 
+/* Opal table operations */
+enum opal_table_ops {
+	OPAL_READ_TABLE,
+	OPAL_WRITE_TABLE,
+};
+
+#define OPAL_UID_LENGTH 8
+struct opal_read_write_table {
+	struct opal_key key;
+	const __u64 data;
+	const __u8 table_uid[OPAL_UID_LENGTH];
+	__u64 offset;
+	__u64 size;
+#define OPAL_TABLE_READ (1 << OPAL_READ_TABLE)
+#define OPAL_TABLE_WRITE (1 << OPAL_WRITE_TABLE)
+	__u64 flags;
+	__u64 priv;
+};
+
 #define IOC_OPAL_SAVE		    _IOW('p', 220, struct opal_lock_unlock)
 #define IOC_OPAL_LOCK_UNLOCK	    _IOW('p', 221, struct opal_lock_unlock)
 #define IOC_OPAL_TAKE_OWNERSHIP	    _IOW('p', 222, struct opal_key)
@@ -128,5 +147,6 @@ struct opal_shadow_mbr {
 #define IOC_OPAL_PSID_REVERT_TPR    _IOW('p', 232, struct opal_key)
 #define IOC_OPAL_MBR_DONE           _IOW('p', 233, struct opal_mbr_done)
 #define IOC_OPAL_WRITE_SHADOW_MBR   _IOW('p', 234, struct opal_shadow_mbr)
+#define IOC_OPAL_GENERIC_TABLE_RW   _IOW('p', 235, struct opal_read_write_table)
 
 #endif /* _UAPI_SED_OPAL_H */
-- 
2.17.1

[PATCH v3 3/3] block: sed-opal: Introduce Opal Datastore UID

[Revanth Rajashekar]

This patch introduces Opal Datastore UID.
The generic read/write table ioctl can use this UID
to access the Opal Datastore.

Reviewed-by: Jon Derrick <jonathan.derrick@intel.com>
Signed-off-by: Revanth Rajashekar <revanth.rajashekar@intel.com>
---
 block/opal_proto.h | 1 +
 block/sed-opal.c   | 2 ++
 2 files changed, 3 insertions(+)

diff --git a/block/opal_proto.h b/block/opal_proto.h
index 710911864e1d..736e67c3e7c5 100644
--- a/block/opal_proto.h
+++ b/block/opal_proto.h
@@ -107,6 +107,7 @@ enum opal_uid {
 	OPAL_C_PIN_TABLE,
 	OPAL_LOCKING_INFO_TABLE,
 	OPAL_ENTERPRISE_LOCKING_INFO_TABLE,
+	OPAL_DATASTORE,
 	/* C_PIN_TABLE object ID's */
 	OPAL_C_PIN_MSID,
 	OPAL_C_PIN_SID,
diff --git a/block/sed-opal.c b/block/sed-opal.c
index 1d5afaaf0826..b2cacc9ddd11 100644
--- a/block/sed-opal.c
+++ b/block/sed-opal.c
@@ -149,6 +149,8 @@ static const u8 opaluid[][OPAL_UID_LENGTH] = {
 		{ 0x00, 0x00, 0x08, 0x01, 0x00, 0x00, 0x00, 0x01 },
 	[OPAL_ENTERPRISE_LOCKING_INFO_TABLE] =
 		{ 0x00, 0x00, 0x08, 0x01, 0x00, 0x00, 0x00, 0x00 },
+	[OPAL_DATASTORE] =
+		{ 0x00, 0x00, 0x10, 0x01, 0x00, 0x00, 0x00, 0x00 },
 
 	/* C_PIN_TABLE object ID's */
 	[OPAL_C_PIN_MSID] =
-- 
2.17.1

Re: [PATCH v3 1/3] block: sed-opal: Generalizing write data to any opal table

[Scott Bauer]

On Thu, Oct 31, 2019 at 10:13:20AM -0600, Revanth Rajashekar wrote:
> This patch refactors the existing "write_shadowmbr" func and
> creates a new generalized function "generic_table_write_data",
> to write data to any opal table. Also, a few cleanups are included
> in this patch.
> 
> Reviewed-by: Jon Derrick <jonathan.derrick@intel.com>
> Signed-off-by: Revanth Rajashekar <revanth.rajashekar@intel.com>

Reviewed-by: Scott Bauer <sbauer@plzdonthack.me>
Looks good.

Re: [PATCH v3 2/3] block: sed-opal: Add support to read/write opal tables generically

[Scott Bauer]

On Thu, Oct 31, 2019 at 10:13:21AM -0600, Revanth Rajashekar wrote:
> This feature gives the user RW access to any opal table with admin1
> authority. The flags described in the new structure determines if the user
> wants to read/write the data. Flags are checked for valid values in
> order to allow future features to be added to the ioctl.
> 
> The user can provide the desired table's UID. Also, the ioctl provides a
> size and offset field and internally will loop data accesses to return
> the full data block. Read overrun is prevented by the initiator's
> sec_send_recv() backend. The ioctl provides a private field with the
> intention to accommodate any future expansions to the ioctl.
> 
> Reviewed-by: Jon Derrick <jonathan.derrick@intel.com>
> Signed-off-by: Revanth Rajashekar <revanth.rajashekar@intel.com>
Looks fine
Reviewed-by: Scott Bauer <sbauer@plzdonthack.me>



> +static int read_table_data(struct opal_dev *dev, void *data)
> +{
> +	struct opal_read_write_table *read_tbl = data;
> +	int err;
> +	size_t off = 0, max_read_size = OPAL_MAX_READ_TABLE;
> +	u64 table_len, len;
> +	u64 offset = read_tbl->offset, read_size = read_tbl->size - 1;
> +	u8 __user *dst;

> +
> +		/* len+1: This includes the NULL terminator at the end*/
> +		if (dev->prev_d_len > len + 1) {
> +			err = -EOVERFLOW;
> +			break;
> +		}
> +
> +		dst = (u8 __user *)(uintptr_t)read_tbl->data;
> +		if (copy_to_user(dst + off, dev->prev_data, dev->prev_d_len)) {
> +			pr_debug("Error copying data to userspace\n");
> +			err = -EFAULT;
> +			break;
> +		}
> +		dev->prev_data = NULL;

If you end up needing to spin a v4 can you please add a comment here reminding me that prev_data in this scenario is not kmalloc'd memory but an offset into the response buffer. I had to go track down why you were not kfree()ing this. I know in a year I'll have forgotten it and will review it again.



> +static int opal_generic_read_write_table(struct opal_dev *dev,
> +					 struct opal_read_write_table *rw_tbl)
> +{
> +	int ret, bit_set;
> +
> +	mutex_lock(&dev->dev_lock);
> +	setup_opal_dev(dev);
> +
> +	bit_set = fls64(rw_tbl->flags) - 1;
Maybe I am missing something obvious but why don't we just use flags as a number? Like instead of bit packing flags we just use it as a number like number 0 is read_table, 1 is write_table, 2 is delete_table, 3 is clear_table etc etc. I don't understand the neccessity for fls-ing some bit field when a number would suffice.

I don't want another revision --it's fine-- I'm just wondering why this method was chosen. 

Re: [PATCH v3 3/3] block: sed-opal: Introduce Opal Datastore UID

[Scott Bauer]

On Thu, Oct 31, 2019 at 10:13:22AM -0600, Revanth Rajashekar wrote:
> This patch introduces Opal Datastore UID.
> The generic read/write table ioctl can use this UID
> to access the Opal Datastore.
> 
> Reviewed-by: Jon Derrick <jonathan.derrick@intel.com>
> Signed-off-by: Revanth Rajashekar <revanth.rajashekar@intel.com>
This is fine
Reviewed-by: Scott Bauer <sbauer@plzdonthack.me>

Re: [PATCH v3 0/3] block: sed-opal: Generic Read/Write Opal Tables

[Jens Axboe]

On 10/31/19 10:13 AM, Revanth Rajashekar wrote:
> This series of patches aims at extending SED Opal support:
> 1. Generalizing write data to any opal table
> 2. Add an IOCTL for reading/writing any Opal Table with Admin-1 authority
> 3. Introduce Opal Datastore UID, which can be accessed using above ioctl
> 
> Datastore feature described in:
> https://trustedcomputinggroup.org/wp-content/uploads/TCG_Storage-Opal_Feature_Set-Additional_DataStore_Tables_v1_00_r1_00_Final.pdf
> 
> Opal Application Note:
> https://trustedcomputinggroup.org/wp-content/uploads/TCG_Storage_Opal_SSC_Application_Note_1-00_1-00-Final.pdf
> 
> This feature has been successfully tested on OPAL Datastore and MBR table using
> internal tools with an Intel SSD and an Intel Optane.
> 
> Changes from v2:
> 	1. Drop a patch which exposes UIDs in UAPI.
> 	2. Fix coding styles wherever required based on LKML feedbacks.
> 	3. Eliminate a few redundant assignments in the code.
> 	4. Add a break under copy_from_user error condition in
>             generic_table_write_data func.
> 	5. A few refactoring/cleanups in both the patches.
> 	6. Introduce a new patch which introduces Opal Datastore table UID.
> 
> Changes from v1:
> 	1. Fix the spelling mistake in the commit message.
> 	2. Introduce a length check condition before Copy To User in
>             opal_read_table function to facilitate user with easy debugging.
> 	3. Introduce switch cases in the opal_generic_read_write_table ioctl
>             function.
> 	4. Move read/write table opal_step to discrete functions to reduce the
>             load on the ioctl function.
> 	5. Introduce 'opal table operations' enumeration in uapi.
> 	6. Remove tabs before the #defines in opal_read_write_table structure
>             to improve the code readability.
> 	7. Drop a patch which exposes UIDs in UAPI.
> 	8. Eliminate a few redundant assignments in the code.
> 	9. Add a break under copy_from_user error condition in
>             generic_table_write_data func.
> 	10. A few refactoring/cleanups in both the patches
> 	11. Introduce a new patch which introduces Opal Datastore table UID.

Applied for 5.5, thanks.

-- 
Jens Axboe