[PATCH v2 0/2] acpi, nfit: support for new NVDIMM_FAMILY_INTEL commands

[PATCH v2 0/2] acpi, nfit: support for new NVDIMM_FAMILY_INTEL commands

[Dan Williams]

Change since v1 [1]:
* Introduce NVDIMM_STANDARD_CMDMASK and NVDIMM_INTEL_CMDMASK to replace
  magic number usage in the driver. (Dave)

[1]: https://lists.01.org/pipermail/linux-nvdimm/2017-October/013082.html
---

The latest version of the NVDIMM_FAMILY_INTEL command set adds support
for firmware updates and setting SMART health alarms / thresholds among
other things. Given that these are command payloads that will only ever
be issued by userspace we only wire up the command numbers and revision
ids for use through the ND_CMD_CALL interface.

---

Dan Williams (2):
      acpi, nfit: hide unknown commands from nmemX/commands
      acpi, nfit: add support for NVDIMM_FAMILY_INTEL v1.6 DSMs


 drivers/acpi/nfit/core.c |   55 ++++++++++++++++++++++++++++++++++++++++------
 drivers/acpi/nfit/nfit.h |   32 ++++++++++++++++++++++++++-
 2 files changed, 79 insertions(+), 8 deletions(-)
_______________________________________________
Linux-nvdimm mailing list
Linux-nvdimm@lists.01.org
https://lists.01.org/mailman/listinfo/linux-nvdimm

[PATCH v2 0/2] acpi, nfit: support for new NVDIMM_FAMILY_INTEL commands

[Dan Williams]

Change since v1 [1]:
* Introduce NVDIMM_STANDARD_CMDMASK and NVDIMM_INTEL_CMDMASK to replace
  magic number usage in the driver. (Dave)

[1]: https://lists.01.org/pipermail/linux-nvdimm/2017-October/013082.html
---

The latest version of the NVDIMM_FAMILY_INTEL command set adds support
for firmware updates and setting SMART health alarms / thresholds among
other things. Given that these are command payloads that will only ever
be issued by userspace we only wire up the command numbers and revision
ids for use through the ND_CMD_CALL interface.

---

Dan Williams (2):
      acpi, nfit: hide unknown commands from nmemX/commands
      acpi, nfit: add support for NVDIMM_FAMILY_INTEL v1.6 DSMs


 drivers/acpi/nfit/core.c |   55 ++++++++++++++++++++++++++++++++++++++++------
 drivers/acpi/nfit/nfit.h |   32 ++++++++++++++++++++++++++-
 2 files changed, 79 insertions(+), 8 deletions(-)

[PATCH v2 1/2] acpi, nfit: hide unknown commands from nmemX/commands

[Dan Williams]

For vendor specific commands that do not have a common kernel
translation, hide them from nmemX/commands. For example, the following
results from new enabling to probe for support of the new
NVDIMM_FAMILY_INTEL DSMs specified in v1.6 of the command specification
[1]:

    # cat /sys/bus/nd/devices/nmem0/commands
    smart smart_thresh flags get_size get_data set_data effect_size
    effect_log vendor cmd_call unknown unknown unknown unknown unknown
    unknown unknown unknown

[1]: https://pmem.io/documents/NVDIMM_DSM_Interface-V1.6.pdf

Signed-off-by: Dan Williams <dan.j.williams@intel.com>
---
 drivers/acpi/nfit/core.c |   10 ++++++++--
 drivers/acpi/nfit/nfit.h |    6 ++++++
 2 files changed, 14 insertions(+), 2 deletions(-)

diff --git a/drivers/acpi/nfit/core.c b/drivers/acpi/nfit/core.c
index ebe0857ac346..444832b372ec 100644
--- a/drivers/acpi/nfit/core.c
+++ b/drivers/acpi/nfit/core.c
@@ -1769,8 +1769,14 @@ static int acpi_nfit_register_dimms(struct acpi_nfit_desc *acpi_desc)
 		 * userspace interface.
 		 */
 		cmd_mask = 1UL << ND_CMD_CALL;
-		if (nfit_mem->family == NVDIMM_FAMILY_INTEL)
-			cmd_mask |= nfit_mem->dsm_mask;
+		if (nfit_mem->family == NVDIMM_FAMILY_INTEL) {
+			/*
+			 * These commands have a 1:1 correspondence
+			 * between DSM payload and libnvdimm ioctl
+			 * payload format.
+			 */
+			cmd_mask |= nfit_mem->dsm_mask & NVDIMM_STANDARD_CMDMASK;
+		}
 
 		if (nfit_mem->has_lsi)
 			set_bit(ND_CMD_GET_CONFIG_SIZE, &cmd_mask);
diff --git a/drivers/acpi/nfit/nfit.h b/drivers/acpi/nfit/nfit.h
index 3976d649f27c..b987196bf132 100644
--- a/drivers/acpi/nfit/nfit.h
+++ b/drivers/acpi/nfit/nfit.h
@@ -38,6 +38,12 @@
 		| ACPI_NFIT_MEM_RESTORE_FAILED | ACPI_NFIT_MEM_FLUSH_FAILED \
 		| ACPI_NFIT_MEM_NOT_ARMED | ACPI_NFIT_MEM_MAP_FAILED)
 
+#define NVDIMM_STANDARD_CMDMASK \
+(1 << ND_CMD_SMART | 1 << ND_CMD_SMART_THRESHOLD | 1 << ND_CMD_DIMM_FLAGS \
+ | 1 << ND_CMD_GET_CONFIG_SIZE | 1 << ND_CMD_GET_CONFIG_DATA \
+ | 1 << ND_CMD_SET_CONFIG_DATA | 1 << ND_CMD_VENDOR_EFFECT_LOG_SIZE \
+ | 1 << ND_CMD_VENDOR_EFFECT_LOG | 1 << ND_CMD_VENDOR)
+
 enum nfit_uuids {
 	/* for simplicity alias the uuid index with the family id */
 	NFIT_DEV_DIMM = NVDIMM_FAMILY_INTEL,

_______________________________________________
Linux-nvdimm mailing list
Linux-nvdimm@lists.01.org
https://lists.01.org/mailman/listinfo/linux-nvdimm

[PATCH v2 1/2] acpi, nfit: hide unknown commands from nmemX/commands

[Dan Williams]

For vendor specific commands that do not have a common kernel
translation, hide them from nmemX/commands. For example, the following
results from new enabling to probe for support of the new
NVDIMM_FAMILY_INTEL DSMs specified in v1.6 of the command specification
[1]:

    # cat /sys/bus/nd/devices/nmem0/commands
    smart smart_thresh flags get_size get_data set_data effect_size
    effect_log vendor cmd_call unknown unknown unknown unknown unknown
    unknown unknown unknown

[1]: https://pmem.io/documents/NVDIMM_DSM_Interface-V1.6.pdf

Signed-off-by: Dan Williams <dan.j.williams-ral2JQCrhuEAvxtiuMwx3w@public.gmane.org>
---
 drivers/acpi/nfit/core.c |   10 ++++++++--
 drivers/acpi/nfit/nfit.h |    6 ++++++
 2 files changed, 14 insertions(+), 2 deletions(-)

diff --git a/drivers/acpi/nfit/core.c b/drivers/acpi/nfit/core.c
index ebe0857ac346..444832b372ec 100644
--- a/drivers/acpi/nfit/core.c
+++ b/drivers/acpi/nfit/core.c
@@ -1769,8 +1769,14 @@ static int acpi_nfit_register_dimms(struct acpi_nfit_desc *acpi_desc)
 		 * userspace interface.
 		 */
 		cmd_mask = 1UL << ND_CMD_CALL;
-		if (nfit_mem->family == NVDIMM_FAMILY_INTEL)
-			cmd_mask |= nfit_mem->dsm_mask;
+		if (nfit_mem->family == NVDIMM_FAMILY_INTEL) {
+			/*
+			 * These commands have a 1:1 correspondence
+			 * between DSM payload and libnvdimm ioctl
+			 * payload format.
+			 */
+			cmd_mask |= nfit_mem->dsm_mask & NVDIMM_STANDARD_CMDMASK;
+		}
 
 		if (nfit_mem->has_lsi)
 			set_bit(ND_CMD_GET_CONFIG_SIZE, &cmd_mask);
diff --git a/drivers/acpi/nfit/nfit.h b/drivers/acpi/nfit/nfit.h
index 3976d649f27c..b987196bf132 100644
--- a/drivers/acpi/nfit/nfit.h
+++ b/drivers/acpi/nfit/nfit.h
@@ -38,6 +38,12 @@
 		| ACPI_NFIT_MEM_RESTORE_FAILED | ACPI_NFIT_MEM_FLUSH_FAILED \
 		| ACPI_NFIT_MEM_NOT_ARMED | ACPI_NFIT_MEM_MAP_FAILED)
 
+#define NVDIMM_STANDARD_CMDMASK \
+(1 << ND_CMD_SMART | 1 << ND_CMD_SMART_THRESHOLD | 1 << ND_CMD_DIMM_FLAGS \
+ | 1 << ND_CMD_GET_CONFIG_SIZE | 1 << ND_CMD_GET_CONFIG_DATA \
+ | 1 << ND_CMD_SET_CONFIG_DATA | 1 << ND_CMD_VENDOR_EFFECT_LOG_SIZE \
+ | 1 << ND_CMD_VENDOR_EFFECT_LOG | 1 << ND_CMD_VENDOR)
+
 enum nfit_uuids {
 	/* for simplicity alias the uuid index with the family id */
 	NFIT_DEV_DIMM = NVDIMM_FAMILY_INTEL,

[PATCH v2 2/2] acpi, nfit: add support for NVDIMM_FAMILY_INTEL v1.6 DSMs

[Dan Williams]

Per v1.6 of the NVDIMM_FAMILY_INTEL command set [1] some of the new
commands require rev-id 2. In addition to enabling ND_CMD_CALL for these
new function numbers, add a lookup table for revision-ids by family
and function number.

[1]: http://pmem.io/documents/NVDIMM_DSM_Interface-V1.6.pdf

Signed-off-by: Dan Williams <dan.j.williams@intel.com>
---
 drivers/acpi/nfit/core.c |   45 ++++++++++++++++++++++++++++++++++++++++-----
 drivers/acpi/nfit/nfit.h |   26 +++++++++++++++++++++++++-
 2 files changed, 65 insertions(+), 6 deletions(-)

diff --git a/drivers/acpi/nfit/core.c b/drivers/acpi/nfit/core.c
index 444832b372ec..1b2c613a9d8b 100644
--- a/drivers/acpi/nfit/core.c
+++ b/drivers/acpi/nfit/core.c
@@ -370,6 +370,32 @@ static union acpi_object *acpi_label_info(acpi_handle handle)
 	return pkg_to_buf(buf.pointer);
 }
 
+static u8 nfit_dsm_revid(unsigned family, unsigned func)
+{
+	static const u8 revid_table[NVDIMM_FAMILY_MAX][32] = {
+		[NVDIMM_FAMILY_INTEL] = {
+			[NVDIMM_INTEL_GET_MODES] = 2,
+			[NVDIMM_INTEL_GET_FWINFO] = 2,
+			[NVDIMM_INTEL_START_FWUPDATE] = 2,
+			[NVDIMM_INTEL_SEND_FWUPDATE] = 2,
+			[NVDIMM_INTEL_FINISH_FWUPDATE] = 2,
+			[NVDIMM_INTEL_QUERY_FWUPDATE] = 2,
+			[NVDIMM_INTEL_SET_THRESHOLD] = 2,
+			[NVDIMM_INTEL_INJECT_ERROR] = 2,
+		},
+	};
+	u8 id;
+
+	if (family > NVDIMM_FAMILY_MAX)
+		return 0;
+	if (func > 31)
+		return 0;
+	id = revid_table[family][func];
+	if (id == 0)
+		return 1; /* default */
+	return id;
+}
+
 int acpi_nfit_ctl(struct nvdimm_bus_descriptor *nd_desc, struct nvdimm *nvdimm,
 		unsigned int cmd, void *buf, unsigned int buf_len, int *cmd_rc)
 {
@@ -468,8 +494,15 @@ int acpi_nfit_ctl(struct nvdimm_bus_descriptor *nd_desc, struct nvdimm *nvdimm,
 
 		out_obj = acpi_label_write(handle, p->in_offset, p->in_length,
 				p->in_buf);
-	} else
-		out_obj = acpi_evaluate_dsm(handle, guid, 1, func, &in_obj);
+	} else {
+		u8 revid;
+
+		if (nfit_mem)
+			revid = nfit_dsm_revid(nfit_mem->family, func);
+		else
+			revid = 1;
+		out_obj = acpi_evaluate_dsm(handle, guid, revid, func, &in_obj);
+	}
 
 	if (!out_obj) {
 		dev_dbg(dev, "%s:%s _DSM failed cmd: %s\n", __func__, dimm_name,
@@ -1640,7 +1673,7 @@ static int acpi_nfit_add_dimm(struct acpi_nfit_desc *acpi_desc,
 	 * different command sets.  Note, that checking for function0 (bit0)
 	 * tells us if any commands are reachable through this GUID.
 	 */
-	for (i = NVDIMM_FAMILY_INTEL; i <= NVDIMM_FAMILY_MSFT; i++)
+	for (i = 0; i <= NVDIMM_FAMILY_MAX; i++)
 		if (acpi_check_dsm(adev_dimm->handle, to_nfit_uuid(i), 1, 1))
 			if (family < 0 || i == default_dsm_family)
 				family = i;
@@ -1650,7 +1683,7 @@ static int acpi_nfit_add_dimm(struct acpi_nfit_desc *acpi_desc,
 	if (override_dsm_mask && !disable_vendor_specific)
 		dsm_mask = override_dsm_mask;
 	else if (nfit_mem->family == NVDIMM_FAMILY_INTEL) {
-		dsm_mask = 0x3fe;
+		dsm_mask = NVDIMM_INTEL_CMDMASK;
 		if (disable_vendor_specific)
 			dsm_mask &= ~(1 << ND_CMD_VENDOR);
 	} else if (nfit_mem->family == NVDIMM_FAMILY_HPE1) {
@@ -1670,7 +1703,9 @@ static int acpi_nfit_add_dimm(struct acpi_nfit_desc *acpi_desc,
 
 	guid = to_nfit_uuid(nfit_mem->family);
 	for_each_set_bit(i, &dsm_mask, BITS_PER_LONG)
-		if (acpi_check_dsm(adev_dimm->handle, guid, 1, 1ULL << i))
+		if (acpi_check_dsm(adev_dimm->handle, guid,
+					nfit_dsm_revid(nfit_mem->family, i),
+					1ULL << i))
 			set_bit(i, &nfit_mem->dsm_mask);
 
 	obj = acpi_label_info(adev_dimm->handle);
diff --git a/drivers/acpi/nfit/nfit.h b/drivers/acpi/nfit/nfit.h
index b987196bf132..341be9511d0e 100644
--- a/drivers/acpi/nfit/nfit.h
+++ b/drivers/acpi/nfit/nfit.h
@@ -24,7 +24,7 @@
 /* ACPI 6.1 */
 #define UUID_NFIT_BUS "2f10e7a4-9e91-11e4-89d3-123b93f75cba"
 
-/* http://pmem.io/documents/NVDIMM_DSM_Interface_Example.pdf */
+/* http://pmem.io/documents/NVDIMM_DSM_Interface-V1.6.pdf */
 #define UUID_NFIT_DIMM "4309ac30-0d11-11e4-9191-0800200c9a66"
 
 /* https://github.com/HewlettPackard/hpe-nvm/blob/master/Documentation/ */
@@ -38,12 +38,36 @@
 		| ACPI_NFIT_MEM_RESTORE_FAILED | ACPI_NFIT_MEM_FLUSH_FAILED \
 		| ACPI_NFIT_MEM_NOT_ARMED | ACPI_NFIT_MEM_MAP_FAILED)
 
+#define NVDIMM_FAMILY_MAX NVDIMM_FAMILY_MSFT
+
 #define NVDIMM_STANDARD_CMDMASK \
 (1 << ND_CMD_SMART | 1 << ND_CMD_SMART_THRESHOLD | 1 << ND_CMD_DIMM_FLAGS \
  | 1 << ND_CMD_GET_CONFIG_SIZE | 1 << ND_CMD_GET_CONFIG_DATA \
  | 1 << ND_CMD_SET_CONFIG_DATA | 1 << ND_CMD_VENDOR_EFFECT_LOG_SIZE \
  | 1 << ND_CMD_VENDOR_EFFECT_LOG | 1 << ND_CMD_VENDOR)
 
+/*
+ * Command numbers that the kernel needs to know about to handle
+ * non-default DSM revision ids
+ */
+enum nvdimm_family_cmds {
+	NVDIMM_INTEL_GET_MODES = 11,
+	NVDIMM_INTEL_GET_FWINFO = 12,
+	NVDIMM_INTEL_START_FWUPDATE = 13,
+	NVDIMM_INTEL_SEND_FWUPDATE = 14,
+	NVDIMM_INTEL_FINISH_FWUPDATE = 15,
+	NVDIMM_INTEL_QUERY_FWUPDATE = 16,
+	NVDIMM_INTEL_SET_THRESHOLD = 17,
+	NVDIMM_INTEL_INJECT_ERROR = 18,
+};
+
+#define NVDIMM_INTEL_CMDMASK \
+(NVDIMM_STANDARD_CMDMASK | 1 << NVDIMM_INTEL_GET_MODES \
+ | 1 << NVDIMM_INTEL_GET_FWINFO | 1 << NVDIMM_INTEL_START_FWUPDATE \
+ | 1 << NVDIMM_INTEL_SEND_FWUPDATE | 1 << NVDIMM_INTEL_FINISH_FWUPDATE \
+ | 1 << NVDIMM_INTEL_QUERY_FWUPDATE | 1 << NVDIMM_INTEL_SET_THRESHOLD \
+ | 1 << NVDIMM_INTEL_INJECT_ERROR)
+
 enum nfit_uuids {
 	/* for simplicity alias the uuid index with the family id */
 	NFIT_DEV_DIMM = NVDIMM_FAMILY_INTEL,

_______________________________________________
Linux-nvdimm mailing list
Linux-nvdimm@lists.01.org
https://lists.01.org/mailman/listinfo/linux-nvdimm

[PATCH v2 2/2] acpi, nfit: add support for NVDIMM_FAMILY_INTEL v1.6 DSMs

[Dan Williams]

Per v1.6 of the NVDIMM_FAMILY_INTEL command set [1] some of the new
commands require rev-id 2. In addition to enabling ND_CMD_CALL for these
new function numbers, add a lookup table for revision-ids by family
and function number.

[1]: http://pmem.io/documents/NVDIMM_DSM_Interface-V1.6.pdf

Signed-off-by: Dan Williams <dan.j.williams@intel.com>
---
 drivers/acpi/nfit/core.c |   45 ++++++++++++++++++++++++++++++++++++++++-----
 drivers/acpi/nfit/nfit.h |   26 +++++++++++++++++++++++++-
 2 files changed, 65 insertions(+), 6 deletions(-)

diff --git a/drivers/acpi/nfit/core.c b/drivers/acpi/nfit/core.c
index 444832b372ec..1b2c613a9d8b 100644
--- a/drivers/acpi/nfit/core.c
+++ b/drivers/acpi/nfit/core.c
@@ -370,6 +370,32 @@ static union acpi_object *acpi_label_info(acpi_handle handle)
 	return pkg_to_buf(buf.pointer);
 }
 
+static u8 nfit_dsm_revid(unsigned family, unsigned func)
+{
+	static const u8 revid_table[NVDIMM_FAMILY_MAX][32] = {
+		[NVDIMM_FAMILY_INTEL] = {
+			[NVDIMM_INTEL_GET_MODES] = 2,
+			[NVDIMM_INTEL_GET_FWINFO] = 2,
+			[NVDIMM_INTEL_START_FWUPDATE] = 2,
+			[NVDIMM_INTEL_SEND_FWUPDATE] = 2,
+			[NVDIMM_INTEL_FINISH_FWUPDATE] = 2,
+			[NVDIMM_INTEL_QUERY_FWUPDATE] = 2,
+			[NVDIMM_INTEL_SET_THRESHOLD] = 2,
+			[NVDIMM_INTEL_INJECT_ERROR] = 2,
+		},
+	};
+	u8 id;
+
+	if (family > NVDIMM_FAMILY_MAX)
+		return 0;
+	if (func > 31)
+		return 0;
+	id = revid_table[family][func];
+	if (id == 0)
+		return 1; /* default */
+	return id;
+}
+
 int acpi_nfit_ctl(struct nvdimm_bus_descriptor *nd_desc, struct nvdimm *nvdimm,
 		unsigned int cmd, void *buf, unsigned int buf_len, int *cmd_rc)
 {
@@ -468,8 +494,15 @@ int acpi_nfit_ctl(struct nvdimm_bus_descriptor *nd_desc, struct nvdimm *nvdimm,
 
 		out_obj = acpi_label_write(handle, p->in_offset, p->in_length,
 				p->in_buf);
-	} else
-		out_obj = acpi_evaluate_dsm(handle, guid, 1, func, &in_obj);
+	} else {
+		u8 revid;
+
+		if (nfit_mem)
+			revid = nfit_dsm_revid(nfit_mem->family, func);
+		else
+			revid = 1;
+		out_obj = acpi_evaluate_dsm(handle, guid, revid, func, &in_obj);
+	}
 
 	if (!out_obj) {
 		dev_dbg(dev, "%s:%s _DSM failed cmd: %s\n", __func__, dimm_name,
@@ -1640,7 +1673,7 @@ static int acpi_nfit_add_dimm(struct acpi_nfit_desc *acpi_desc,
 	 * different command sets.  Note, that checking for function0 (bit0)
 	 * tells us if any commands are reachable through this GUID.
 	 */
-	for (i = NVDIMM_FAMILY_INTEL; i <= NVDIMM_FAMILY_MSFT; i++)
+	for (i = 0; i <= NVDIMM_FAMILY_MAX; i++)
 		if (acpi_check_dsm(adev_dimm->handle, to_nfit_uuid(i), 1, 1))
 			if (family < 0 || i == default_dsm_family)
 				family = i;
@@ -1650,7 +1683,7 @@ static int acpi_nfit_add_dimm(struct acpi_nfit_desc *acpi_desc,
 	if (override_dsm_mask && !disable_vendor_specific)
 		dsm_mask = override_dsm_mask;
 	else if (nfit_mem->family == NVDIMM_FAMILY_INTEL) {
-		dsm_mask = 0x3fe;
+		dsm_mask = NVDIMM_INTEL_CMDMASK;
 		if (disable_vendor_specific)
 			dsm_mask &= ~(1 << ND_CMD_VENDOR);
 	} else if (nfit_mem->family == NVDIMM_FAMILY_HPE1) {
@@ -1670,7 +1703,9 @@ static int acpi_nfit_add_dimm(struct acpi_nfit_desc *acpi_desc,
 
 	guid = to_nfit_uuid(nfit_mem->family);
 	for_each_set_bit(i, &dsm_mask, BITS_PER_LONG)
-		if (acpi_check_dsm(adev_dimm->handle, guid, 1, 1ULL << i))
+		if (acpi_check_dsm(adev_dimm->handle, guid,
+					nfit_dsm_revid(nfit_mem->family, i),
+					1ULL << i))
 			set_bit(i, &nfit_mem->dsm_mask);
 
 	obj = acpi_label_info(adev_dimm->handle);
diff --git a/drivers/acpi/nfit/nfit.h b/drivers/acpi/nfit/nfit.h
index b987196bf132..341be9511d0e 100644
--- a/drivers/acpi/nfit/nfit.h
+++ b/drivers/acpi/nfit/nfit.h
@@ -24,7 +24,7 @@
 /* ACPI 6.1 */
 #define UUID_NFIT_BUS "2f10e7a4-9e91-11e4-89d3-123b93f75cba"
 
-/* http://pmem.io/documents/NVDIMM_DSM_Interface_Example.pdf */
+/* http://pmem.io/documents/NVDIMM_DSM_Interface-V1.6.pdf */
 #define UUID_NFIT_DIMM "4309ac30-0d11-11e4-9191-0800200c9a66"
 
 /* https://github.com/HewlettPackard/hpe-nvm/blob/master/Documentation/ */
@@ -38,12 +38,36 @@
 		| ACPI_NFIT_MEM_RESTORE_FAILED | ACPI_NFIT_MEM_FLUSH_FAILED \
 		| ACPI_NFIT_MEM_NOT_ARMED | ACPI_NFIT_MEM_MAP_FAILED)
 
+#define NVDIMM_FAMILY_MAX NVDIMM_FAMILY_MSFT
+
 #define NVDIMM_STANDARD_CMDMASK \
 (1 << ND_CMD_SMART | 1 << ND_CMD_SMART_THRESHOLD | 1 << ND_CMD_DIMM_FLAGS \
  | 1 << ND_CMD_GET_CONFIG_SIZE | 1 << ND_CMD_GET_CONFIG_DATA \
  | 1 << ND_CMD_SET_CONFIG_DATA | 1 << ND_CMD_VENDOR_EFFECT_LOG_SIZE \
  | 1 << ND_CMD_VENDOR_EFFECT_LOG | 1 << ND_CMD_VENDOR)
 
+/*
+ * Command numbers that the kernel needs to know about to handle
+ * non-default DSM revision ids
+ */
+enum nvdimm_family_cmds {
+	NVDIMM_INTEL_GET_MODES = 11,
+	NVDIMM_INTEL_GET_FWINFO = 12,
+	NVDIMM_INTEL_START_FWUPDATE = 13,
+	NVDIMM_INTEL_SEND_FWUPDATE = 14,
+	NVDIMM_INTEL_FINISH_FWUPDATE = 15,
+	NVDIMM_INTEL_QUERY_FWUPDATE = 16,
+	NVDIMM_INTEL_SET_THRESHOLD = 17,
+	NVDIMM_INTEL_INJECT_ERROR = 18,
+};
+
+#define NVDIMM_INTEL_CMDMASK \
+(NVDIMM_STANDARD_CMDMASK | 1 << NVDIMM_INTEL_GET_MODES \
+ | 1 << NVDIMM_INTEL_GET_FWINFO | 1 << NVDIMM_INTEL_START_FWUPDATE \
+ | 1 << NVDIMM_INTEL_SEND_FWUPDATE | 1 << NVDIMM_INTEL_FINISH_FWUPDATE \
+ | 1 << NVDIMM_INTEL_QUERY_FWUPDATE | 1 << NVDIMM_INTEL_SET_THRESHOLD \
+ | 1 << NVDIMM_INTEL_INJECT_ERROR)
+
 enum nfit_uuids {
 	/* for simplicity alias the uuid index with the family id */
 	NFIT_DEV_DIMM = NVDIMM_FAMILY_INTEL,

Re: [PATCH v2 2/2] acpi, nfit: add support for NVDIMM_FAMILY_INTEL v1.6 DSMs

[Jerry Hoemann]

On Mon, Oct 30, 2017 at 01:47:08PM -0700, Dan Williams wrote:
> Per v1.6 of the NVDIMM_FAMILY_INTEL command set [1] some of the new
> commands require rev-id 2. In addition to enabling ND_CMD_CALL for these
> new function numbers, add a lookup table for revision-ids by family
> and function number.
> 
> [1]: http://pmem.io/documents/NVDIMM_DSM_Interface-V1.6.pdf
> 
> Signed-off-by: Dan Williams <dan.j.williams@intel.com>
> ---
>  drivers/acpi/nfit/core.c |   45 ++++++++++++++++++++++++++++++++++++++++-----
>  drivers/acpi/nfit/nfit.h |   26 +++++++++++++++++++++++++-
>  2 files changed, 65 insertions(+), 6 deletions(-)
> 
> diff --git a/drivers/acpi/nfit/core.c b/drivers/acpi/nfit/core.c
> index 444832b372ec..1b2c613a9d8b 100644
> --- a/drivers/acpi/nfit/core.c
> +++ b/drivers/acpi/nfit/core.c
> @@ -370,6 +370,32 @@ static union acpi_object *acpi_label_info(acpi_handle handle)
>  	return pkg_to_buf(buf.pointer);
>  }
>  
> +static u8 nfit_dsm_revid(unsigned family, unsigned func)
> +{
> +	static const u8 revid_table[NVDIMM_FAMILY_MAX][32] = {
> +		[NVDIMM_FAMILY_INTEL] = {
> +			[NVDIMM_INTEL_GET_MODES] = 2,
> +			[NVDIMM_INTEL_GET_FWINFO] = 2,
> +			[NVDIMM_INTEL_START_FWUPDATE] = 2,
> +			[NVDIMM_INTEL_SEND_FWUPDATE] = 2,
> +			[NVDIMM_INTEL_FINISH_FWUPDATE] = 2,
> +			[NVDIMM_INTEL_QUERY_FWUPDATE] = 2,
> +			[NVDIMM_INTEL_SET_THRESHOLD] = 2,
> +			[NVDIMM_INTEL_INJECT_ERROR] = 2,
> +		},
> +	};
> +	u8 id;
> +
> +	if (family > NVDIMM_FAMILY_MAX)
> +		return 0;
> +	if (func > 31)
> +		return 0;
> +	id = revid_table[family][func];
> +	if (id == 0)
> +		return 1; /* default */
> +	return id;
> +}
> +


Elsewhere we find NVDIMM_FAMILY_MAX == NVDIMM_FAMILY_MSFT.
Code is off by 1 in the dimension the array revid_table.

This approach only ever calls revision ID 1 of a function that
was initially defined in revision ID 1 of a spec.

Is it required that FW returns same data for a given function if
called with different revision IDs?  I.e. could firmware implement
a function N such that it returns different information if called
with revision ID 2 versus being called with revision ID 1?
E.g. a field that is reserved in revision 1 could have a definition
in revision 2.  My reading of the ACPI spec is this is acceptable.

Not sure if this makes a difference w/ the Intel FW or not, but
some of the structures returns were modified from the earlier
spec.  I have concerns that this approach can be correctly
extended to other vendors.




>  int acpi_nfit_ctl(struct nvdimm_bus_descriptor *nd_desc, struct nvdimm *nvdimm,
>  		unsigned int cmd, void *buf, unsigned int buf_len, int *cmd_rc)
>  {
> @@ -468,8 +494,15 @@ int acpi_nfit_ctl(struct nvdimm_bus_descriptor *nd_desc, struct nvdimm *nvdimm,
>  
>  		out_obj = acpi_label_write(handle, p->in_offset, p->in_length,
>  				p->in_buf);
> -	} else
> -		out_obj = acpi_evaluate_dsm(handle, guid, 1, func, &in_obj);
> +	} else {
> +		u8 revid;
> +
> +		if (nfit_mem)
> +			revid = nfit_dsm_revid(nfit_mem->family, func);
> +		else
> +			revid = 1;
> +		out_obj = acpi_evaluate_dsm(handle, guid, revid, func, &in_obj);
> +	}
>  
>  	if (!out_obj) {
>  		dev_dbg(dev, "%s:%s _DSM failed cmd: %s\n", __func__, dimm_name,
> @@ -1640,7 +1673,7 @@ static int acpi_nfit_add_dimm(struct acpi_nfit_desc *acpi_desc,
>  	 * different command sets.  Note, that checking for function0 (bit0)
>  	 * tells us if any commands are reachable through this GUID.
>  	 */
> -	for (i = NVDIMM_FAMILY_INTEL; i <= NVDIMM_FAMILY_MSFT; i++)
> +	for (i = 0; i <= NVDIMM_FAMILY_MAX; i++)
>  		if (acpi_check_dsm(adev_dimm->handle, to_nfit_uuid(i), 1, 1))
>  			if (family < 0 || i == default_dsm_family)
>  				family = i;
> @@ -1650,7 +1683,7 @@ static int acpi_nfit_add_dimm(struct acpi_nfit_desc *acpi_desc,
>  	if (override_dsm_mask && !disable_vendor_specific)
>  		dsm_mask = override_dsm_mask;
>  	else if (nfit_mem->family == NVDIMM_FAMILY_INTEL) {
> -		dsm_mask = 0x3fe;
> +		dsm_mask = NVDIMM_INTEL_CMDMASK;
>  		if (disable_vendor_specific)
>  			dsm_mask &= ~(1 << ND_CMD_VENDOR);
>  	} else if (nfit_mem->family == NVDIMM_FAMILY_HPE1) {
> @@ -1670,7 +1703,9 @@ static int acpi_nfit_add_dimm(struct acpi_nfit_desc *acpi_desc,
>  
>  	guid = to_nfit_uuid(nfit_mem->family);
>  	for_each_set_bit(i, &dsm_mask, BITS_PER_LONG)
> -		if (acpi_check_dsm(adev_dimm->handle, guid, 1, 1ULL << i))
> +		if (acpi_check_dsm(adev_dimm->handle, guid,
> +					nfit_dsm_revid(nfit_mem->family, i),
> +					1ULL << i))
>  			set_bit(i, &nfit_mem->dsm_mask);
>  
>  	obj = acpi_label_info(adev_dimm->handle);
> diff --git a/drivers/acpi/nfit/nfit.h b/drivers/acpi/nfit/nfit.h
> index b987196bf132..341be9511d0e 100644
> --- a/drivers/acpi/nfit/nfit.h
> +++ b/drivers/acpi/nfit/nfit.h
> @@ -24,7 +24,7 @@
>  /* ACPI 6.1 */
>  #define UUID_NFIT_BUS "2f10e7a4-9e91-11e4-89d3-123b93f75cba"
>  
> -/* http://pmem.io/documents/NVDIMM_DSM_Interface_Example.pdf */
> +/* http://pmem.io/documents/NVDIMM_DSM_Interface-V1.6.pdf */
>  #define UUID_NFIT_DIMM "4309ac30-0d11-11e4-9191-0800200c9a66"
>  
>  /* https://github.com/HewlettPackard/hpe-nvm/blob/master/Documentation/ */
> @@ -38,12 +38,36 @@
>  		| ACPI_NFIT_MEM_RESTORE_FAILED | ACPI_NFIT_MEM_FLUSH_FAILED \
>  		| ACPI_NFIT_MEM_NOT_ARMED | ACPI_NFIT_MEM_MAP_FAILED)
>  
> +#define NVDIMM_FAMILY_MAX NVDIMM_FAMILY_MSFT


Suggest this be defined immediately after where NVDIMM_FAMILY_MSFT
is defined.  That way if a new family is ever defined, its more obvious
that NVDIMM_FAMILY_MAX needs to be redefined as well.



> +
>  #define NVDIMM_STANDARD_CMDMASK \
>  (1 << ND_CMD_SMART | 1 << ND_CMD_SMART_THRESHOLD | 1 << ND_CMD_DIMM_FLAGS \
>   | 1 << ND_CMD_GET_CONFIG_SIZE | 1 << ND_CMD_GET_CONFIG_DATA \
>   | 1 << ND_CMD_SET_CONFIG_DATA | 1 << ND_CMD_VENDOR_EFFECT_LOG_SIZE \
>   | 1 << ND_CMD_VENDOR_EFFECT_LOG | 1 << ND_CMD_VENDOR)
>  
> +/*
> + * Command numbers that the kernel needs to know about to handle
> + * non-default DSM revision ids
> + */
> +enum nvdimm_family_cmds {
> +	NVDIMM_INTEL_GET_MODES = 11,
> +	NVDIMM_INTEL_GET_FWINFO = 12,
> +	NVDIMM_INTEL_START_FWUPDATE = 13,
> +	NVDIMM_INTEL_SEND_FWUPDATE = 14,
> +	NVDIMM_INTEL_FINISH_FWUPDATE = 15,
> +	NVDIMM_INTEL_QUERY_FWUPDATE = 16,
> +	NVDIMM_INTEL_SET_THRESHOLD = 17,
> +	NVDIMM_INTEL_INJECT_ERROR = 18,
> +};
> +
> +#define NVDIMM_INTEL_CMDMASK \
> +(NVDIMM_STANDARD_CMDMASK | 1 << NVDIMM_INTEL_GET_MODES \
> + | 1 << NVDIMM_INTEL_GET_FWINFO | 1 << NVDIMM_INTEL_START_FWUPDATE \
> + | 1 << NVDIMM_INTEL_SEND_FWUPDATE | 1 << NVDIMM_INTEL_FINISH_FWUPDATE \
> + | 1 << NVDIMM_INTEL_QUERY_FWUPDATE | 1 << NVDIMM_INTEL_SET_THRESHOLD \
> + | 1 << NVDIMM_INTEL_INJECT_ERROR)
> +
>  enum nfit_uuids {
>  	/* for simplicity alias the uuid index with the family id */
>  	NFIT_DEV_DIMM = NVDIMM_FAMILY_INTEL,
> 
> _______________________________________________
> Linux-nvdimm mailing list
> Linux-nvdimm@lists.01.org
> https://lists.01.org/mailman/listinfo/linux-nvdimm

-- 

-----------------------------------------------------------------------------
Jerry Hoemann                  Software Engineer   Hewlett Packard Enterprise
-----------------------------------------------------------------------------
_______________________________________________
Linux-nvdimm mailing list
Linux-nvdimm@lists.01.org
https://lists.01.org/mailman/listinfo/linux-nvdimm

Re: [PATCH v2 2/2] acpi, nfit: add support for NVDIMM_FAMILY_INTEL v1.6 DSMs

[Jerry Hoemann]

On Mon, Oct 30, 2017 at 01:47:08PM -0700, Dan Williams wrote:
> Per v1.6 of the NVDIMM_FAMILY_INTEL command set [1] some of the new
> commands require rev-id 2. In addition to enabling ND_CMD_CALL for these
> new function numbers, add a lookup table for revision-ids by family
> and function number.
> 
> [1]: http://pmem.io/documents/NVDIMM_DSM_Interface-V1.6.pdf
> 
> Signed-off-by: Dan Williams <dan.j.williams@intel.com>
> ---
>  drivers/acpi/nfit/core.c |   45 ++++++++++++++++++++++++++++++++++++++++-----
>  drivers/acpi/nfit/nfit.h |   26 +++++++++++++++++++++++++-
>  2 files changed, 65 insertions(+), 6 deletions(-)
> 
> diff --git a/drivers/acpi/nfit/core.c b/drivers/acpi/nfit/core.c
> index 444832b372ec..1b2c613a9d8b 100644
> --- a/drivers/acpi/nfit/core.c
> +++ b/drivers/acpi/nfit/core.c
> @@ -370,6 +370,32 @@ static union acpi_object *acpi_label_info(acpi_handle handle)
>  	return pkg_to_buf(buf.pointer);
>  }
>  
> +static u8 nfit_dsm_revid(unsigned family, unsigned func)
> +{
> +	static const u8 revid_table[NVDIMM_FAMILY_MAX][32] = {
> +		[NVDIMM_FAMILY_INTEL] = {
> +			[NVDIMM_INTEL_GET_MODES] = 2,
> +			[NVDIMM_INTEL_GET_FWINFO] = 2,
> +			[NVDIMM_INTEL_START_FWUPDATE] = 2,
> +			[NVDIMM_INTEL_SEND_FWUPDATE] = 2,
> +			[NVDIMM_INTEL_FINISH_FWUPDATE] = 2,
> +			[NVDIMM_INTEL_QUERY_FWUPDATE] = 2,
> +			[NVDIMM_INTEL_SET_THRESHOLD] = 2,
> +			[NVDIMM_INTEL_INJECT_ERROR] = 2,
> +		},
> +	};
> +	u8 id;
> +
> +	if (family > NVDIMM_FAMILY_MAX)
> +		return 0;
> +	if (func > 31)
> +		return 0;
> +	id = revid_table[family][func];
> +	if (id == 0)
> +		return 1; /* default */
> +	return id;
> +}
> +


Elsewhere we find NVDIMM_FAMILY_MAX == NVDIMM_FAMILY_MSFT.
Code is off by 1 in the dimension the array revid_table.

This approach only ever calls revision ID 1 of a function that
was initially defined in revision ID 1 of a spec.

Is it required that FW returns same data for a given function if
called with different revision IDs?  I.e. could firmware implement
a function N such that it returns different information if called
with revision ID 2 versus being called with revision ID 1?
E.g. a field that is reserved in revision 1 could have a definition
in revision 2.  My reading of the ACPI spec is this is acceptable.

Not sure if this makes a difference w/ the Intel FW or not, but
some of the structures returns were modified from the earlier
spec.  I have concerns that this approach can be correctly
extended to other vendors.




>  int acpi_nfit_ctl(struct nvdimm_bus_descriptor *nd_desc, struct nvdimm *nvdimm,
>  		unsigned int cmd, void *buf, unsigned int buf_len, int *cmd_rc)
>  {
> @@ -468,8 +494,15 @@ int acpi_nfit_ctl(struct nvdimm_bus_descriptor *nd_desc, struct nvdimm *nvdimm,
>  
>  		out_obj = acpi_label_write(handle, p->in_offset, p->in_length,
>  				p->in_buf);
> -	} else
> -		out_obj = acpi_evaluate_dsm(handle, guid, 1, func, &in_obj);
> +	} else {
> +		u8 revid;
> +
> +		if (nfit_mem)
> +			revid = nfit_dsm_revid(nfit_mem->family, func);
> +		else
> +			revid = 1;
> +		out_obj = acpi_evaluate_dsm(handle, guid, revid, func, &in_obj);
> +	}
>  
>  	if (!out_obj) {
>  		dev_dbg(dev, "%s:%s _DSM failed cmd: %s\n", __func__, dimm_name,
> @@ -1640,7 +1673,7 @@ static int acpi_nfit_add_dimm(struct acpi_nfit_desc *acpi_desc,
>  	 * different command sets.  Note, that checking for function0 (bit0)
>  	 * tells us if any commands are reachable through this GUID.
>  	 */
> -	for (i = NVDIMM_FAMILY_INTEL; i <= NVDIMM_FAMILY_MSFT; i++)
> +	for (i = 0; i <= NVDIMM_FAMILY_MAX; i++)
>  		if (acpi_check_dsm(adev_dimm->handle, to_nfit_uuid(i), 1, 1))
>  			if (family < 0 || i == default_dsm_family)
>  				family = i;
> @@ -1650,7 +1683,7 @@ static int acpi_nfit_add_dimm(struct acpi_nfit_desc *acpi_desc,
>  	if (override_dsm_mask && !disable_vendor_specific)
>  		dsm_mask = override_dsm_mask;
>  	else if (nfit_mem->family == NVDIMM_FAMILY_INTEL) {
> -		dsm_mask = 0x3fe;
> +		dsm_mask = NVDIMM_INTEL_CMDMASK;
>  		if (disable_vendor_specific)
>  			dsm_mask &= ~(1 << ND_CMD_VENDOR);
>  	} else if (nfit_mem->family == NVDIMM_FAMILY_HPE1) {
> @@ -1670,7 +1703,9 @@ static int acpi_nfit_add_dimm(struct acpi_nfit_desc *acpi_desc,
>  
>  	guid = to_nfit_uuid(nfit_mem->family);
>  	for_each_set_bit(i, &dsm_mask, BITS_PER_LONG)
> -		if (acpi_check_dsm(adev_dimm->handle, guid, 1, 1ULL << i))
> +		if (acpi_check_dsm(adev_dimm->handle, guid,
> +					nfit_dsm_revid(nfit_mem->family, i),
> +					1ULL << i))
>  			set_bit(i, &nfit_mem->dsm_mask);
>  
>  	obj = acpi_label_info(adev_dimm->handle);
> diff --git a/drivers/acpi/nfit/nfit.h b/drivers/acpi/nfit/nfit.h
> index b987196bf132..341be9511d0e 100644
> --- a/drivers/acpi/nfit/nfit.h
> +++ b/drivers/acpi/nfit/nfit.h
> @@ -24,7 +24,7 @@
>  /* ACPI 6.1 */
>  #define UUID_NFIT_BUS "2f10e7a4-9e91-11e4-89d3-123b93f75cba"
>  
> -/* http://pmem.io/documents/NVDIMM_DSM_Interface_Example.pdf */
> +/* http://pmem.io/documents/NVDIMM_DSM_Interface-V1.6.pdf */
>  #define UUID_NFIT_DIMM "4309ac30-0d11-11e4-9191-0800200c9a66"
>  
>  /* https://github.com/HewlettPackard/hpe-nvm/blob/master/Documentation/ */
> @@ -38,12 +38,36 @@
>  		| ACPI_NFIT_MEM_RESTORE_FAILED | ACPI_NFIT_MEM_FLUSH_FAILED \
>  		| ACPI_NFIT_MEM_NOT_ARMED | ACPI_NFIT_MEM_MAP_FAILED)
>  
> +#define NVDIMM_FAMILY_MAX NVDIMM_FAMILY_MSFT


Suggest this be defined immediately after where NVDIMM_FAMILY_MSFT
is defined.  That way if a new family is ever defined, its more obvious
that NVDIMM_FAMILY_MAX needs to be redefined as well.



> +
>  #define NVDIMM_STANDARD_CMDMASK \
>  (1 << ND_CMD_SMART | 1 << ND_CMD_SMART_THRESHOLD | 1 << ND_CMD_DIMM_FLAGS \
>   | 1 << ND_CMD_GET_CONFIG_SIZE | 1 << ND_CMD_GET_CONFIG_DATA \
>   | 1 << ND_CMD_SET_CONFIG_DATA | 1 << ND_CMD_VENDOR_EFFECT_LOG_SIZE \
>   | 1 << ND_CMD_VENDOR_EFFECT_LOG | 1 << ND_CMD_VENDOR)
>  
> +/*
> + * Command numbers that the kernel needs to know about to handle
> + * non-default DSM revision ids
> + */
> +enum nvdimm_family_cmds {
> +	NVDIMM_INTEL_GET_MODES = 11,
> +	NVDIMM_INTEL_GET_FWINFO = 12,
> +	NVDIMM_INTEL_START_FWUPDATE = 13,
> +	NVDIMM_INTEL_SEND_FWUPDATE = 14,
> +	NVDIMM_INTEL_FINISH_FWUPDATE = 15,
> +	NVDIMM_INTEL_QUERY_FWUPDATE = 16,
> +	NVDIMM_INTEL_SET_THRESHOLD = 17,
> +	NVDIMM_INTEL_INJECT_ERROR = 18,
> +};
> +
> +#define NVDIMM_INTEL_CMDMASK \
> +(NVDIMM_STANDARD_CMDMASK | 1 << NVDIMM_INTEL_GET_MODES \
> + | 1 << NVDIMM_INTEL_GET_FWINFO | 1 << NVDIMM_INTEL_START_FWUPDATE \
> + | 1 << NVDIMM_INTEL_SEND_FWUPDATE | 1 << NVDIMM_INTEL_FINISH_FWUPDATE \
> + | 1 << NVDIMM_INTEL_QUERY_FWUPDATE | 1 << NVDIMM_INTEL_SET_THRESHOLD \
> + | 1 << NVDIMM_INTEL_INJECT_ERROR)
> +
>  enum nfit_uuids {
>  	/* for simplicity alias the uuid index with the family id */
>  	NFIT_DEV_DIMM = NVDIMM_FAMILY_INTEL,
> 
> _______________________________________________
> Linux-nvdimm mailing list
> Linux-nvdimm@lists.01.org
> https://lists.01.org/mailman/listinfo/linux-nvdimm

-- 

-----------------------------------------------------------------------------
Jerry Hoemann                  Software Engineer   Hewlett Packard Enterprise
-----------------------------------------------------------------------------

Re: [PATCH v2 2/2] acpi, nfit: add support for NVDIMM_FAMILY_INTEL v1.6 DSMs

[Dan Williams]

On Tue, Oct 31, 2017 at 2:27 PM, Jerry Hoemann <jerry.hoemann@hpe.com> wrote:
> On Mon, Oct 30, 2017 at 01:47:08PM -0700, Dan Williams wrote:
>> Per v1.6 of the NVDIMM_FAMILY_INTEL command set [1] some of the new
>> commands require rev-id 2. In addition to enabling ND_CMD_CALL for these
>> new function numbers, add a lookup table for revision-ids by family
>> and function number.
>>
>> [1]: http://pmem.io/documents/NVDIMM_DSM_Interface-V1.6.pdf
>>
>> Signed-off-by: Dan Williams <dan.j.williams@intel.com>
>> ---
>>  drivers/acpi/nfit/core.c |   45 ++++++++++++++++++++++++++++++++++++++++-----
>>  drivers/acpi/nfit/nfit.h |   26 +++++++++++++++++++++++++-
>>  2 files changed, 65 insertions(+), 6 deletions(-)
>>
>> diff --git a/drivers/acpi/nfit/core.c b/drivers/acpi/nfit/core.c
>> index 444832b372ec..1b2c613a9d8b 100644
>> --- a/drivers/acpi/nfit/core.c
>> +++ b/drivers/acpi/nfit/core.c
>> @@ -370,6 +370,32 @@ static union acpi_object *acpi_label_info(acpi_handle handle)
>>       return pkg_to_buf(buf.pointer);
>>  }
>>
>> +static u8 nfit_dsm_revid(unsigned family, unsigned func)
>> +{
>> +     static const u8 revid_table[NVDIMM_FAMILY_MAX][32] = {
>> +             [NVDIMM_FAMILY_INTEL] = {
>> +                     [NVDIMM_INTEL_GET_MODES] = 2,
>> +                     [NVDIMM_INTEL_GET_FWINFO] = 2,
>> +                     [NVDIMM_INTEL_START_FWUPDATE] = 2,
>> +                     [NVDIMM_INTEL_SEND_FWUPDATE] = 2,
>> +                     [NVDIMM_INTEL_FINISH_FWUPDATE] = 2,
>> +                     [NVDIMM_INTEL_QUERY_FWUPDATE] = 2,
>> +                     [NVDIMM_INTEL_SET_THRESHOLD] = 2,
>> +                     [NVDIMM_INTEL_INJECT_ERROR] = 2,
>> +             },
>> +     };
>> +     u8 id;
>> +
>> +     if (family > NVDIMM_FAMILY_MAX)
>> +             return 0;
>> +     if (func > 31)
>> +             return 0;
>> +     id = revid_table[family][func];
>> +     if (id == 0)
>> +             return 1; /* default */
>> +     return id;
>> +}
>> +
>
>
> Elsewhere we find NVDIMM_FAMILY_MAX == NVDIMM_FAMILY_MSFT.
> Code is off by 1 in the dimension the array revid_table.

Nice catch, will fix.

At least the compiler will complain if we use c99 style initialization
for that NVDIMM_FAMILY_MAX index (error: array index in initializer
exceeds array bounds).

> This approach only ever calls revision ID 1 of a function that
> was initially defined in revision ID 1 of a spec.
>
> Is it required that FW returns same data for a given function if
> called with different revision IDs?  I.e. could firmware implement
> a function N such that it returns different information if called
> with revision ID 2 versus being called with revision ID 1?
> E.g. a field that is reserved in revision 1 could have a definition
> in revision 2.  My reading of the ACPI spec is this is acceptable.

Yes, this can happen. However, for the kernel implementation it can
decide to move to rev-id2 at its leisure since ACPI mandates that
rev-id1 implementations stick around, and if the function number was
reserved in rev-id1 the kernel will already not support it.

>
> Not sure if this makes a difference w/ the Intel FW or not, but
> some of the structures returns were modified from the earlier
> spec.  I have concerns that this approach can be correctly
> extended to other vendors.
>
[..]
>> +#define NVDIMM_FAMILY_MAX NVDIMM_FAMILY_MSFT
>
>
> Suggest this be defined immediately after where NVDIMM_FAMILY_MSFT
> is defined.  That way if a new family is ever defined, its more obvious
> that NVDIMM_FAMILY_MAX needs to be redefined as well.

These are currently defined in the userspace api header and that value
is not relevant to userspace since the kernel is free to support more
families than NVDIMM_FAMILY_MAX would imply. Also, if we as an
industry continue to add NVDIMM families something is broken with the
standardization process.
_______________________________________________
Linux-nvdimm mailing list
Linux-nvdimm@lists.01.org
https://lists.01.org/mailman/listinfo/linux-nvdimm

Re: [PATCH v2 2/2] acpi, nfit: add support for NVDIMM_FAMILY_INTEL v1.6 DSMs

[Dan Williams]

On Tue, Oct 31, 2017 at 2:27 PM, Jerry Hoemann <jerry.hoemann@hpe.com> wrote:
> On Mon, Oct 30, 2017 at 01:47:08PM -0700, Dan Williams wrote:
>> Per v1.6 of the NVDIMM_FAMILY_INTEL command set [1] some of the new
>> commands require rev-id 2. In addition to enabling ND_CMD_CALL for these
>> new function numbers, add a lookup table for revision-ids by family
>> and function number.
>>
>> [1]: http://pmem.io/documents/NVDIMM_DSM_Interface-V1.6.pdf
>>
>> Signed-off-by: Dan Williams <dan.j.williams@intel.com>
>> ---
>>  drivers/acpi/nfit/core.c |   45 ++++++++++++++++++++++++++++++++++++++++-----
>>  drivers/acpi/nfit/nfit.h |   26 +++++++++++++++++++++++++-
>>  2 files changed, 65 insertions(+), 6 deletions(-)
>>
>> diff --git a/drivers/acpi/nfit/core.c b/drivers/acpi/nfit/core.c
>> index 444832b372ec..1b2c613a9d8b 100644
>> --- a/drivers/acpi/nfit/core.c
>> +++ b/drivers/acpi/nfit/core.c
>> @@ -370,6 +370,32 @@ static union acpi_object *acpi_label_info(acpi_handle handle)
>>       return pkg_to_buf(buf.pointer);
>>  }
>>
>> +static u8 nfit_dsm_revid(unsigned family, unsigned func)
>> +{
>> +     static const u8 revid_table[NVDIMM_FAMILY_MAX][32] = {
>> +             [NVDIMM_FAMILY_INTEL] = {
>> +                     [NVDIMM_INTEL_GET_MODES] = 2,
>> +                     [NVDIMM_INTEL_GET_FWINFO] = 2,
>> +                     [NVDIMM_INTEL_START_FWUPDATE] = 2,
>> +                     [NVDIMM_INTEL_SEND_FWUPDATE] = 2,
>> +                     [NVDIMM_INTEL_FINISH_FWUPDATE] = 2,
>> +                     [NVDIMM_INTEL_QUERY_FWUPDATE] = 2,
>> +                     [NVDIMM_INTEL_SET_THRESHOLD] = 2,
>> +                     [NVDIMM_INTEL_INJECT_ERROR] = 2,
>> +             },
>> +     };
>> +     u8 id;
>> +
>> +     if (family > NVDIMM_FAMILY_MAX)
>> +             return 0;
>> +     if (func > 31)
>> +             return 0;
>> +     id = revid_table[family][func];
>> +     if (id == 0)
>> +             return 1; /* default */
>> +     return id;
>> +}
>> +
>
>
> Elsewhere we find NVDIMM_FAMILY_MAX == NVDIMM_FAMILY_MSFT.
> Code is off by 1 in the dimension the array revid_table.

Nice catch, will fix.

At least the compiler will complain if we use c99 style initialization
for that NVDIMM_FAMILY_MAX index (error: array index in initializer
exceeds array bounds).

> This approach only ever calls revision ID 1 of a function that
> was initially defined in revision ID 1 of a spec.
>
> Is it required that FW returns same data for a given function if
> called with different revision IDs?  I.e. could firmware implement
> a function N such that it returns different information if called
> with revision ID 2 versus being called with revision ID 1?
> E.g. a field that is reserved in revision 1 could have a definition
> in revision 2.  My reading of the ACPI spec is this is acceptable.

Yes, this can happen. However, for the kernel implementation it can
decide to move to rev-id2 at its leisure since ACPI mandates that
rev-id1 implementations stick around, and if the function number was
reserved in rev-id1 the kernel will already not support it.

>
> Not sure if this makes a difference w/ the Intel FW or not, but
> some of the structures returns were modified from the earlier
> spec.  I have concerns that this approach can be correctly
> extended to other vendors.
>
[..]
>> +#define NVDIMM_FAMILY_MAX NVDIMM_FAMILY_MSFT
>
>
> Suggest this be defined immediately after where NVDIMM_FAMILY_MSFT
> is defined.  That way if a new family is ever defined, its more obvious
> that NVDIMM_FAMILY_MAX needs to be redefined as well.

These are currently defined in the userspace api header and that value
is not relevant to userspace since the kernel is free to support more
families than NVDIMM_FAMILY_MAX would imply. Also, if we as an
industry continue to add NVDIMM families something is broken with the
standardization process.

Re: [PATCH v2 2/2] acpi, nfit: add support for NVDIMM_FAMILY_INTEL v1.6 DSMs

[Jerry Hoemann]

On Tue, Oct 31, 2017 at 02:42:01PM -0700, Dan Williams wrote:
> On Tue, Oct 31, 2017 at 2:27 PM, Jerry Hoemann <jerry.hoemann@hpe.com> wrote:
> > On Mon, Oct 30, 2017 at 01:47:08PM -0700, Dan Williams wrote:
> >> Per v1.6 of the NVDIMM_FAMILY_INTEL command set [1] some of the new
> >> commands require rev-id 2. In addition to enabling ND_CMD_CALL for these
> >> new function numbers, add a lookup table for revision-ids by family
> >> and function number.
> >>
> >> [1]: http://pmem.io/documents/NVDIMM_DSM_Interface-V1.6.pdf
> >>
> >> Signed-off-by: Dan Williams <dan.j.williams@intel.com>
> >> ---
> >>  drivers/acpi/nfit/core.c |   45 ++++++++++++++++++++++++++++++++++++++++-----
> >>  drivers/acpi/nfit/nfit.h |   26 +++++++++++++++++++++++++-
> >>  2 files changed, 65 insertions(+), 6 deletions(-)
> >>
> >> diff --git a/drivers/acpi/nfit/core.c b/drivers/acpi/nfit/core.c
> >> index 444832b372ec..1b2c613a9d8b 100644
> >> --- a/drivers/acpi/nfit/core.c
> >> +++ b/drivers/acpi/nfit/core.c
> >> @@ -370,6 +370,32 @@ static union acpi_object *acpi_label_info(acpi_handle handle)
> >>       return pkg_to_buf(buf.pointer);
> >>  }
> >>
> >> +static u8 nfit_dsm_revid(unsigned family, unsigned func)
> >> +{
> >> +     static const u8 revid_table[NVDIMM_FAMILY_MAX][32] = {
> >> +             [NVDIMM_FAMILY_INTEL] = {
> >> +                     [NVDIMM_INTEL_GET_MODES] = 2,
> >> +                     [NVDIMM_INTEL_GET_FWINFO] = 2,
> >> +                     [NVDIMM_INTEL_START_FWUPDATE] = 2,
> >> +                     [NVDIMM_INTEL_SEND_FWUPDATE] = 2,
> >> +                     [NVDIMM_INTEL_FINISH_FWUPDATE] = 2,
> >> +                     [NVDIMM_INTEL_QUERY_FWUPDATE] = 2,
> >> +                     [NVDIMM_INTEL_SET_THRESHOLD] = 2,
> >> +                     [NVDIMM_INTEL_INJECT_ERROR] = 2,
> >> +             },
> >> +     };
> >> +     u8 id;
> >> +
> >> +     if (family > NVDIMM_FAMILY_MAX)
> >> +             return 0;
> >> +     if (func > 31)
> >> +             return 0;
> >> +     id = revid_table[family][func];
> >> +     if (id == 0)
> >> +             return 1; /* default */
> >> +     return id;
> >> +}
> >> +
> >
> >
> > Elsewhere we find NVDIMM_FAMILY_MAX == NVDIMM_FAMILY_MSFT.
> > Code is off by 1 in the dimension the array revid_table.
> 
> Nice catch, will fix.
> 
> At least the compiler will complain if we use c99 style initialization
> for that NVDIMM_FAMILY_MAX index (error: array index in initializer
> exceeds array bounds).
> 
> > This approach only ever calls revision ID 1 of a function that
> > was initially defined in revision ID 1 of a spec.
> >
> > Is it required that FW returns same data for a given function if
> > called with different revision IDs?  I.e. could firmware implement
> > a function N such that it returns different information if called
> > with revision ID 2 versus being called with revision ID 1?
> > E.g. a field that is reserved in revision 1 could have a definition
> > in revision 2.  My reading of the ACPI spec is this is acceptable.
> 
> Yes, this can happen. However, for the kernel implementation it can
> decide to move to rev-id2 at its leisure since ACPI mandates that
> rev-id1 implementations stick around, and if the function number was
> reserved in rev-id1 the kernel will already not support it.

Unfortunately, we can't do that.  The kernel will still need to support
firmware that only knows about Rev ID 1.

Example, HPE currently supports customers w/ NVDIMMS with DSM that only
responds to rev id 1.

Say in the future we ship a new generation of NVDIMMs 
that extends the DSM health function.  We could make that
additional info available only for rev id 2.  If the table
above were extended to call health function with rev id 2
so that we get the additional info for the new nvdimm, that
call will stop working on what will then be legacy systems
that only know rev id 1.


> 
> >
> > Not sure if this makes a difference w/ the Intel FW or not, but
> > some of the structures returns were modified from the earlier
> > spec.  I have concerns that this approach can be correctly
> > extended to other vendors.
> >
> [..]
> >> +#define NVDIMM_FAMILY_MAX NVDIMM_FAMILY_MSFT
> >
> >
> > Suggest this be defined immediately after where NVDIMM_FAMILY_MSFT
> > is defined.  That way if a new family is ever defined, its more obvious
> > that NVDIMM_FAMILY_MAX needs to be redefined as well.
> 
> These are currently defined in the userspace api header and that value
> is not relevant to userspace since the kernel is free to support more
> families than NVDIMM_FAMILY_MAX would imply. Also, if we as an
> industry continue to add NVDIMM families something is broken with the
> standardization process.
> --
> To unsubscribe from this list: send the line "unsubscribe linux-acpi" in
> the body of a message to majordomo@vger.kernel.org
> More majordomo info at  http://vger.kernel.org/majordomo-info.html

-- 

-----------------------------------------------------------------------------
Jerry Hoemann                  Software Engineer   Hewlett Packard Enterprise
-----------------------------------------------------------------------------
_______________________________________________
Linux-nvdimm mailing list
Linux-nvdimm@lists.01.org
https://lists.01.org/mailman/listinfo/linux-nvdimm

Re: [PATCH v2 2/2] acpi, nfit: add support for NVDIMM_FAMILY_INTEL v1.6 DSMs

[Jerry Hoemann]

On Tue, Oct 31, 2017 at 02:42:01PM -0700, Dan Williams wrote:
> On Tue, Oct 31, 2017 at 2:27 PM, Jerry Hoemann <jerry.hoemann@hpe.com> wrote:
> > On Mon, Oct 30, 2017 at 01:47:08PM -0700, Dan Williams wrote:
> >> Per v1.6 of the NVDIMM_FAMILY_INTEL command set [1] some of the new
> >> commands require rev-id 2. In addition to enabling ND_CMD_CALL for these
> >> new function numbers, add a lookup table for revision-ids by family
> >> and function number.
> >>
> >> [1]: http://pmem.io/documents/NVDIMM_DSM_Interface-V1.6.pdf
> >>
> >> Signed-off-by: Dan Williams <dan.j.williams@intel.com>
> >> ---
> >>  drivers/acpi/nfit/core.c |   45 ++++++++++++++++++++++++++++++++++++++++-----
> >>  drivers/acpi/nfit/nfit.h |   26 +++++++++++++++++++++++++-
> >>  2 files changed, 65 insertions(+), 6 deletions(-)
> >>
> >> diff --git a/drivers/acpi/nfit/core.c b/drivers/acpi/nfit/core.c
> >> index 444832b372ec..1b2c613a9d8b 100644
> >> --- a/drivers/acpi/nfit/core.c
> >> +++ b/drivers/acpi/nfit/core.c
> >> @@ -370,6 +370,32 @@ static union acpi_object *acpi_label_info(acpi_handle handle)
> >>       return pkg_to_buf(buf.pointer);
> >>  }
> >>
> >> +static u8 nfit_dsm_revid(unsigned family, unsigned func)
> >> +{
> >> +     static const u8 revid_table[NVDIMM_FAMILY_MAX][32] = {
> >> +             [NVDIMM_FAMILY_INTEL] = {
> >> +                     [NVDIMM_INTEL_GET_MODES] = 2,
> >> +                     [NVDIMM_INTEL_GET_FWINFO] = 2,
> >> +                     [NVDIMM_INTEL_START_FWUPDATE] = 2,
> >> +                     [NVDIMM_INTEL_SEND_FWUPDATE] = 2,
> >> +                     [NVDIMM_INTEL_FINISH_FWUPDATE] = 2,
> >> +                     [NVDIMM_INTEL_QUERY_FWUPDATE] = 2,
> >> +                     [NVDIMM_INTEL_SET_THRESHOLD] = 2,
> >> +                     [NVDIMM_INTEL_INJECT_ERROR] = 2,
> >> +             },
> >> +     };
> >> +     u8 id;
> >> +
> >> +     if (family > NVDIMM_FAMILY_MAX)
> >> +             return 0;
> >> +     if (func > 31)
> >> +             return 0;
> >> +     id = revid_table[family][func];
> >> +     if (id == 0)
> >> +             return 1; /* default */
> >> +     return id;
> >> +}
> >> +
> >
> >
> > Elsewhere we find NVDIMM_FAMILY_MAX == NVDIMM_FAMILY_MSFT.
> > Code is off by 1 in the dimension the array revid_table.
> 
> Nice catch, will fix.
> 
> At least the compiler will complain if we use c99 style initialization
> for that NVDIMM_FAMILY_MAX index (error: array index in initializer
> exceeds array bounds).
> 
> > This approach only ever calls revision ID 1 of a function that
> > was initially defined in revision ID 1 of a spec.
> >
> > Is it required that FW returns same data for a given function if
> > called with different revision IDs?  I.e. could firmware implement
> > a function N such that it returns different information if called
> > with revision ID 2 versus being called with revision ID 1?
> > E.g. a field that is reserved in revision 1 could have a definition
> > in revision 2.  My reading of the ACPI spec is this is acceptable.
> 
> Yes, this can happen. However, for the kernel implementation it can
> decide to move to rev-id2 at its leisure since ACPI mandates that
> rev-id1 implementations stick around, and if the function number was
> reserved in rev-id1 the kernel will already not support it.

Unfortunately, we can't do that.  The kernel will still need to support
firmware that only knows about Rev ID 1.

Example, HPE currently supports customers w/ NVDIMMS with DSM that only
responds to rev id 1.

Say in the future we ship a new generation of NVDIMMs 
that extends the DSM health function.  We could make that
additional info available only for rev id 2.  If the table
above were extended to call health function with rev id 2
so that we get the additional info for the new nvdimm, that
call will stop working on what will then be legacy systems
that only know rev id 1.


> 
> >
> > Not sure if this makes a difference w/ the Intel FW or not, but
> > some of the structures returns were modified from the earlier
> > spec.  I have concerns that this approach can be correctly
> > extended to other vendors.
> >
> [..]
> >> +#define NVDIMM_FAMILY_MAX NVDIMM_FAMILY_MSFT
> >
> >
> > Suggest this be defined immediately after where NVDIMM_FAMILY_MSFT
> > is defined.  That way if a new family is ever defined, its more obvious
> > that NVDIMM_FAMILY_MAX needs to be redefined as well.
> 
> These are currently defined in the userspace api header and that value
> is not relevant to userspace since the kernel is free to support more
> families than NVDIMM_FAMILY_MAX would imply. Also, if we as an
> industry continue to add NVDIMM families something is broken with the
> standardization process.
> --
> To unsubscribe from this list: send the line "unsubscribe linux-acpi" in
> the body of a message to majordomo@vger.kernel.org
> More majordomo info at  http://vger.kernel.org/majordomo-info.html

-- 

-----------------------------------------------------------------------------
Jerry Hoemann                  Software Engineer   Hewlett Packard Enterprise
-----------------------------------------------------------------------------

Re: [PATCH v2 2/2] acpi, nfit: add support for NVDIMM_FAMILY_INTEL v1.6 DSMs

[Dan Williams]

On Tue, Oct 31, 2017 at 3:05 PM, Jerry Hoemann <jerry.hoemann@hpe.com> wrote:
> On Tue, Oct 31, 2017 at 02:42:01PM -0700, Dan Williams wrote:
[..]
>> Yes, this can happen. However, for the kernel implementation it can
>> decide to move to rev-id2 at its leisure since ACPI mandates that
>> rev-id1 implementations stick around, and if the function number was
>> reserved in rev-id1 the kernel will already not support it.
>
> Unfortunately, we can't do that.  The kernel will still need to support
> firmware that only knows about Rev ID 1.
>
> Example, HPE currently supports customers w/ NVDIMMS with DSM that only
> responds to rev id 1.
>
> Say in the future we ship a new generation of NVDIMMs
> that extends the DSM health function.  We could make that
> additional info available only for rev id 2.  If the table
> above were extended to call health function with rev id 2
> so that we get the additional info for the new nvdimm, that
> call will stop working on what will then be legacy systems
> that only know rev id 1.

Right, I'm explicitly not supporting that degree of compatibility
freedom. If a platform wants to change the payload of an existing
function it had better expand the payload with new information that
can optionally be ignored by older kernels rather than spin the revid.
The kernel can't break legacy and neither should the platform, either
the kernel can call revid1 forever, the function is only available in
revid2 or later, or that function number is burned and we need to live
with the breakage that the platform shipped.

We are already in this position with the "smart threshold" payload
that has been broken in recent revisions of the Intel spec. In this
specific case we have ndctl binaries in the wild that are broken, but
are somewhat saved by the fact that the interface for *setting* alarms
and thresholds is brand new in v1.6. So my plan is to document that
smart threshold is broken and require new ndctl binaries with v1.6
support to implement the new format via the ND_CMD_CALL interface. The
fact that this compatibility management is on a case by case basis and
somewhat painful is a feature. Platform implementations must hide DSM
update thrash from the kernel and shipping binaries as much as
possible.
_______________________________________________
Linux-nvdimm mailing list
Linux-nvdimm@lists.01.org
https://lists.01.org/mailman/listinfo/linux-nvdimm

Re: [PATCH v2 2/2] acpi, nfit: add support for NVDIMM_FAMILY_INTEL v1.6 DSMs

[Dan Williams]

On Tue, Oct 31, 2017 at 3:05 PM, Jerry Hoemann <jerry.hoemann@hpe.com> wrote:
> On Tue, Oct 31, 2017 at 02:42:01PM -0700, Dan Williams wrote:
[..]
>> Yes, this can happen. However, for the kernel implementation it can
>> decide to move to rev-id2 at its leisure since ACPI mandates that
>> rev-id1 implementations stick around, and if the function number was
>> reserved in rev-id1 the kernel will already not support it.
>
> Unfortunately, we can't do that.  The kernel will still need to support
> firmware that only knows about Rev ID 1.
>
> Example, HPE currently supports customers w/ NVDIMMS with DSM that only
> responds to rev id 1.
>
> Say in the future we ship a new generation of NVDIMMs
> that extends the DSM health function.  We could make that
> additional info available only for rev id 2.  If the table
> above were extended to call health function with rev id 2
> so that we get the additional info for the new nvdimm, that
> call will stop working on what will then be legacy systems
> that only know rev id 1.

Right, I'm explicitly not supporting that degree of compatibility
freedom. If a platform wants to change the payload of an existing
function it had better expand the payload with new information that
can optionally be ignored by older kernels rather than spin the revid.
The kernel can't break legacy and neither should the platform, either
the kernel can call revid1 forever, the function is only available in
revid2 or later, or that function number is burned and we need to live
with the breakage that the platform shipped.

We are already in this position with the "smart threshold" payload
that has been broken in recent revisions of the Intel spec. In this
specific case we have ndctl binaries in the wild that are broken, but
are somewhat saved by the fact that the interface for *setting* alarms
and thresholds is brand new in v1.6. So my plan is to document that
smart threshold is broken and require new ndctl binaries with v1.6
support to implement the new format via the ND_CMD_CALL interface. The
fact that this compatibility management is on a case by case basis and
somewhat painful is a feature. Platform implementations must hide DSM
update thrash from the kernel and shipping binaries as much as
possible.

Re: [PATCH v2 2/2] acpi, nfit: add support for NVDIMM_FAMILY_INTEL v1.6 DSMs

[Jerry Hoemann]

On Tue, Oct 31, 2017 at 04:19:32PM -0700, Dan Williams wrote:
> On Tue, Oct 31, 2017 at 3:05 PM, Jerry Hoemann <jerry.hoemann@hpe.com> wrote:
> > On Tue, Oct 31, 2017 at 02:42:01PM -0700, Dan Williams wrote:
> [..]
> >> Yes, this can happen. However, for the kernel implementation it can
> >> decide to move to rev-id2 at its leisure since ACPI mandates that
> >> rev-id1 implementations stick around, and if the function number was
> >> reserved in rev-id1 the kernel will already not support it.
> >
> > Unfortunately, we can't do that.  The kernel will still need to support
> > firmware that only knows about Rev ID 1.
> >
> > Example, HPE currently supports customers w/ NVDIMMS with DSM that only
> > responds to rev id 1.
> >
> > Say in the future we ship a new generation of NVDIMMs
> > that extends the DSM health function.  We could make that
> > additional info available only for rev id 2.  If the table
> > above were extended to call health function with rev id 2
> > so that we get the additional info for the new nvdimm, that
> > call will stop working on what will then be legacy systems
> > that only know rev id 1.
> 
> Right, I'm explicitly not supporting that degree of compatibility
> freedom. If a platform wants to change the payload of an existing
> function it had better expand the payload with new information that
> can optionally be ignored by older kernels rather than spin the revid.
> The kernel can't break legacy and neither should the platform, either
> the kernel can call revid1 forever, the function is only available in
> revid2 or later, or that function number is burned and we need to live
> with the breakage that the platform shipped.


Old software would still be calling firmware with revision id 1 and
firmware would still respond with the revision id 1 version of the
call.  So there is no problem there.  This would be the case for
legacy firmware that only understand revision id 1, or new firmware
that understands both revision id 1 and revision id 2.  Calling a
function with revision id 1 should get the revision id 1 version
of the call.  If not, then bug in firmware.

The idea behind revision ids here is to allow both software and firmware
to progress but still work together.   Firmware can extend functionality
by increasing revision ID, but will still answer to old software that
calls with older revision IDS.

Likewise, OS can add support for new revision IDs/functions but still
needs to be aware that firmware at customer sites may be legacy firmware that
doesn't understand the newer firmware revision.  So software needs
to call firmware with revision IDs that the firmware understands.

Specifically, I am pushing back on the idea that the OS could
just start calling a function with revision ID 2.  That doesn't
work on firmware that only understands revision ID 1.  The call
will fail.


> 
> We are already in this position with the "smart threshold" payload
> that has been broken in recent revisions of the Intel spec. In this
> specific case we have ndctl binaries in the wild that are broken, but
> are somewhat saved by the fact that the interface for *setting* alarms
> and thresholds is brand new in v1.6. So my plan is to document that
> smart threshold is broken and require new ndctl binaries with v1.6
> support to implement the new format via the ND_CMD_CALL interface. The
> fact that this compatibility management is on a case by case basis and
> somewhat painful is a feature. Platform implementations must hide DSM
> update thrash from the kernel and shipping binaries as much as
> possible.


I haven't thought through the changes in v1.6.  The threshold one
doesn't seem backwards compatible and it would have been better for
this to have been caught during design review. Oh, well.....

Fortunately, this isn't a customer/legacy bios issue yet, so solutions
are a bit more flexible.

I am concerned how the ND_CMD_CALL interface can handle revision ID
changes in general.  The interface doesn't specify a revision ID so
it can't control the revision of data being returned to the level
it can handle.  Likewise,  there is no mechanism to communicate
back to the application the revision of the call being returned.
This make me think that any application that uses ND_CMD_CALL will
be rev locked to specific kernel revisions.  I need to think about
this more.



-- 

-----------------------------------------------------------------------------
Jerry Hoemann                  Software Engineer   Hewlett Packard Enterprise
-----------------------------------------------------------------------------
_______________________________________________
Linux-nvdimm mailing list
Linux-nvdimm@lists.01.org
https://lists.01.org/mailman/listinfo/linux-nvdimm

Re: [PATCH v2 2/2] acpi, nfit: add support for NVDIMM_FAMILY_INTEL v1.6 DSMs

[Jerry Hoemann]

On Tue, Oct 31, 2017 at 04:19:32PM -0700, Dan Williams wrote:
> On Tue, Oct 31, 2017 at 3:05 PM, Jerry Hoemann <jerry.hoemann@hpe.com> wrote:
> > On Tue, Oct 31, 2017 at 02:42:01PM -0700, Dan Williams wrote:
> [..]
> >> Yes, this can happen. However, for the kernel implementation it can
> >> decide to move to rev-id2 at its leisure since ACPI mandates that
> >> rev-id1 implementations stick around, and if the function number was
> >> reserved in rev-id1 the kernel will already not support it.
> >
> > Unfortunately, we can't do that.  The kernel will still need to support
> > firmware that only knows about Rev ID 1.
> >
> > Example, HPE currently supports customers w/ NVDIMMS with DSM that only
> > responds to rev id 1.
> >
> > Say in the future we ship a new generation of NVDIMMs
> > that extends the DSM health function.  We could make that
> > additional info available only for rev id 2.  If the table
> > above were extended to call health function with rev id 2
> > so that we get the additional info for the new nvdimm, that
> > call will stop working on what will then be legacy systems
> > that only know rev id 1.
> 
> Right, I'm explicitly not supporting that degree of compatibility
> freedom. If a platform wants to change the payload of an existing
> function it had better expand the payload with new information that
> can optionally be ignored by older kernels rather than spin the revid.
> The kernel can't break legacy and neither should the platform, either
> the kernel can call revid1 forever, the function is only available in
> revid2 or later, or that function number is burned and we need to live
> with the breakage that the platform shipped.


Old software would still be calling firmware with revision id 1 and
firmware would still respond with the revision id 1 version of the
call.  So there is no problem there.  This would be the case for
legacy firmware that only understand revision id 1, or new firmware
that understands both revision id 1 and revision id 2.  Calling a
function with revision id 1 should get the revision id 1 version
of the call.  If not, then bug in firmware.

The idea behind revision ids here is to allow both software and firmware
to progress but still work together.   Firmware can extend functionality
by increasing revision ID, but will still answer to old software that
calls with older revision IDS.

Likewise, OS can add support for new revision IDs/functions but still
needs to be aware that firmware at customer sites may be legacy firmware that
doesn't understand the newer firmware revision.  So software needs
to call firmware with revision IDs that the firmware understands.

Specifically, I am pushing back on the idea that the OS could
just start calling a function with revision ID 2.  That doesn't
work on firmware that only understands revision ID 1.  The call
will fail.


> 
> We are already in this position with the "smart threshold" payload
> that has been broken in recent revisions of the Intel spec. In this
> specific case we have ndctl binaries in the wild that are broken, but
> are somewhat saved by the fact that the interface for *setting* alarms
> and thresholds is brand new in v1.6. So my plan is to document that
> smart threshold is broken and require new ndctl binaries with v1.6
> support to implement the new format via the ND_CMD_CALL interface. The
> fact that this compatibility management is on a case by case basis and
> somewhat painful is a feature. Platform implementations must hide DSM
> update thrash from the kernel and shipping binaries as much as
> possible.


I haven't thought through the changes in v1.6.  The threshold one
doesn't seem backwards compatible and it would have been better for
this to have been caught during design review. Oh, well.....

Fortunately, this isn't a customer/legacy bios issue yet, so solutions
are a bit more flexible.

I am concerned how the ND_CMD_CALL interface can handle revision ID
changes in general.  The interface doesn't specify a revision ID so
it can't control the revision of data being returned to the level
it can handle.  Likewise,  there is no mechanism to communicate
back to the application the revision of the call being returned.
This make me think that any application that uses ND_CMD_CALL will
be rev locked to specific kernel revisions.  I need to think about
this more.



-- 

-----------------------------------------------------------------------------
Jerry Hoemann                  Software Engineer   Hewlett Packard Enterprise
-----------------------------------------------------------------------------

Re: [PATCH v2 2/2] acpi, nfit: add support for NVDIMM_FAMILY_INTEL v1.6 DSMs

[Dan Williams]

On Wed, Nov 1, 2017 at 10:35 AM, Jerry Hoemann <jerry.hoemann@hpe.com> wrote:
> On Tue, Oct 31, 2017 at 04:19:32PM -0700, Dan Williams wrote:
>> On Tue, Oct 31, 2017 at 3:05 PM, Jerry Hoemann <jerry.hoemann@hpe.com> wrote:
>> > On Tue, Oct 31, 2017 at 02:42:01PM -0700, Dan Williams wrote:
>> [..]
>> >> Yes, this can happen. However, for the kernel implementation it can
>> >> decide to move to rev-id2 at its leisure since ACPI mandates that
>> >> rev-id1 implementations stick around, and if the function number was
>> >> reserved in rev-id1 the kernel will already not support it.
>> >
>> > Unfortunately, we can't do that.  The kernel will still need to support
>> > firmware that only knows about Rev ID 1.
>> >
>> > Example, HPE currently supports customers w/ NVDIMMS with DSM that only
>> > responds to rev id 1.
>> >
>> > Say in the future we ship a new generation of NVDIMMs
>> > that extends the DSM health function.  We could make that
>> > additional info available only for rev id 2.  If the table
>> > above were extended to call health function with rev id 2
>> > so that we get the additional info for the new nvdimm, that
>> > call will stop working on what will then be legacy systems
>> > that only know rev id 1.
>>
>> Right, I'm explicitly not supporting that degree of compatibility
>> freedom. If a platform wants to change the payload of an existing
>> function it had better expand the payload with new information that
>> can optionally be ignored by older kernels rather than spin the revid.
>> The kernel can't break legacy and neither should the platform, either
>> the kernel can call revid1 forever, the function is only available in
>> revid2 or later, or that function number is burned and we need to live
>> with the breakage that the platform shipped.
>
>
> Old software would still be calling firmware with revision id 1 and
> firmware would still respond with the revision id 1 version of the
> call.  So there is no problem there.  This would be the case for
> legacy firmware that only understand revision id 1, or new firmware
> that understands both revision id 1 and revision id 2.  Calling a
> function with revision id 1 should get the revision id 1 version
> of the call.  If not, then bug in firmware.
>
> The idea behind revision ids here is to allow both software and firmware
> to progress but still work together.   Firmware can extend functionality
> by increasing revision ID, but will still answer to old software that
> calls with older revision IDS.
>
> Likewise, OS can add support for new revision IDs/functions but still
> needs to be aware that firmware at customer sites may be legacy firmware that
> doesn't understand the newer firmware revision.  So software needs
> to call firmware with revision IDs that the firmware understands.
>
> Specifically, I am pushing back on the idea that the OS could
> just start calling a function with revision ID 2.  That doesn't
> work on firmware that only understands revision ID 1.  The call
> will fail.

Right, the OS can only switch to rev-id2 of the same function after a
long deprecation period. The timeframe to deprecate support for
rev-id1 and switch the kernel to start using rev-id2 of the same
function is on the order of several years until all old hardware with
only rev-id1 is end-of-life. The way to upgrade functionality in a
manner that does not break the world is to extend payloads with
optional functionality.

This is why DSM is a dangerous interface, it gives entirely too much
power to the platform to do something incompatible, so we should
mitigate that by communicating to platform implementers that the
kernel will continue to use rev-id1 indefinitely for a given function.
_______________________________________________
Linux-nvdimm mailing list
Linux-nvdimm@lists.01.org
https://lists.01.org/mailman/listinfo/linux-nvdimm

Re: [PATCH v2 2/2] acpi, nfit: add support for NVDIMM_FAMILY_INTEL v1.6 DSMs

[Dan Williams]

On Wed, Nov 1, 2017 at 10:35 AM, Jerry Hoemann <jerry.hoemann@hpe.com> wrote:
> On Tue, Oct 31, 2017 at 04:19:32PM -0700, Dan Williams wrote:
>> On Tue, Oct 31, 2017 at 3:05 PM, Jerry Hoemann <jerry.hoemann@hpe.com> wrote:
>> > On Tue, Oct 31, 2017 at 02:42:01PM -0700, Dan Williams wrote:
>> [..]
>> >> Yes, this can happen. However, for the kernel implementation it can
>> >> decide to move to rev-id2 at its leisure since ACPI mandates that
>> >> rev-id1 implementations stick around, and if the function number was
>> >> reserved in rev-id1 the kernel will already not support it.
>> >
>> > Unfortunately, we can't do that.  The kernel will still need to support
>> > firmware that only knows about Rev ID 1.
>> >
>> > Example, HPE currently supports customers w/ NVDIMMS with DSM that only
>> > responds to rev id 1.
>> >
>> > Say in the future we ship a new generation of NVDIMMs
>> > that extends the DSM health function.  We could make that
>> > additional info available only for rev id 2.  If the table
>> > above were extended to call health function with rev id 2
>> > so that we get the additional info for the new nvdimm, that
>> > call will stop working on what will then be legacy systems
>> > that only know rev id 1.
>>
>> Right, I'm explicitly not supporting that degree of compatibility
>> freedom. If a platform wants to change the payload of an existing
>> function it had better expand the payload with new information that
>> can optionally be ignored by older kernels rather than spin the revid.
>> The kernel can't break legacy and neither should the platform, either
>> the kernel can call revid1 forever, the function is only available in
>> revid2 or later, or that function number is burned and we need to live
>> with the breakage that the platform shipped.
>
>
> Old software would still be calling firmware with revision id 1 and
> firmware would still respond with the revision id 1 version of the
> call.  So there is no problem there.  This would be the case for
> legacy firmware that only understand revision id 1, or new firmware
> that understands both revision id 1 and revision id 2.  Calling a
> function with revision id 1 should get the revision id 1 version
> of the call.  If not, then bug in firmware.
>
> The idea behind revision ids here is to allow both software and firmware
> to progress but still work together.   Firmware can extend functionality
> by increasing revision ID, but will still answer to old software that
> calls with older revision IDS.
>
> Likewise, OS can add support for new revision IDs/functions but still
> needs to be aware that firmware at customer sites may be legacy firmware that
> doesn't understand the newer firmware revision.  So software needs
> to call firmware with revision IDs that the firmware understands.
>
> Specifically, I am pushing back on the idea that the OS could
> just start calling a function with revision ID 2.  That doesn't
> work on firmware that only understands revision ID 1.  The call
> will fail.

Right, the OS can only switch to rev-id2 of the same function after a
long deprecation period. The timeframe to deprecate support for
rev-id1 and switch the kernel to start using rev-id2 of the same
function is on the order of several years until all old hardware with
only rev-id1 is end-of-life. The way to upgrade functionality in a
manner that does not break the world is to extend payloads with
optional functionality.

This is why DSM is a dangerous interface, it gives entirely too much
power to the platform to do something incompatible, so we should
mitigate that by communicating to platform implementers that the
kernel will continue to use rev-id1 indefinitely for a given function.