[PATCH 0/3] Hotplug support for libnvdimm

[PATCH 0/3] Hotplug support for libnvdimm

[Vishal Verma]

This series adds support for hotplug of NVDIMMs. Upon hotplug, the ACPI
core calls the .notify callback we register. From this, we evaluate the
_FIT method which returns an updated NFIT. This is scanned for any new
tables, and any new regions found from it are registered and made
available for use.

The series is tested with nfit_test (tools/testing/nvdimm) only, which
means the parts of getting a notification from the acpi core, and calling
_FIT are untested.

Dan/Rafael, I have a couple of 'TODO' comments in patch 3, where I wasn't
sure of what we should do - if you have any suggestions, let me know.

Vishal Verma (3):
  nfit: in acpi_nfit_init, break on a 0-length table
  acpi: add a utility function for evaluating _FIT
  acpi: nfit: Add support for hotplug

 drivers/acpi/nfit.c              | 139 +++++++++++++++++++++++++++++++++----
 drivers/acpi/nfit.h              |   2 +
 drivers/acpi/utils.c             |  23 +++++++
 include/acpi/acpi_bus.h          |   1 +
 tools/testing/nvdimm/test/nfit.c | 144 ++++++++++++++++++++++++++++++++++++++-
 5 files changed, 294 insertions(+), 15 deletions(-)

-- 
2.4.3

[PATCH 1/3] nfit: in acpi_nfit_init, break on a 0-length table

[Vishal Verma]

If acpi_nfit_init is called (such as from nfit_test), with an nfit table
that has more memory allocated than it needs (and a similarly large
'size' field, add_tables would happily keep adding null SPA Range tables
filling up all available memory.

Make it friendlier by breaking out if a 0-length header is found in any
of the tables.

Cc: Dan Williams <dan.j.williams@intel.com>
Cc: Rafael J. Wysocki <rafael.j.wysocki@intel.com>
Cc: <linux-acpi@vger.kernel.org>
Cc: <linux-nvdimm@lists.01.org>
Signed-off-by: Vishal Verma <vishal.l.verma@intel.com>
---
 drivers/acpi/nfit.c | 3 +++
 1 file changed, 3 insertions(+)

diff --git a/drivers/acpi/nfit.c b/drivers/acpi/nfit.c
index c1b8d03..ed599d1 100644
--- a/drivers/acpi/nfit.c
+++ b/drivers/acpi/nfit.c
@@ -335,6 +335,9 @@ static void *add_table(struct acpi_nfit_desc *acpi_desc, void *table,
 		return NULL;
 
 	hdr = table;
+	if (!hdr->length)
+		return NULL;
+
 	switch (hdr->type) {
 	case ACPI_NFIT_TYPE_SYSTEM_ADDRESS:
 		if (!add_spa(acpi_desc, table))
-- 
2.4.3

[PATCH 2/3] acpi: add a utility function for evaluating _FIT

[Vishal Verma]

_FIT is an ACPI method to request an updated NFIT (Nvdimm Firmware
Interface Table) after a hotplug event. Add a function to evaluate the
_FIT method and return a buffer with the updated NFIT.

Cc: Dan Williams <dan.j.williams@intel.com>
Cc: Rafael J. Wysocki <rafael.j.wysocki@intel.com>
Cc: <linux-acpi@vger.kernel.org>
Cc: <linux-nvdimm@lists.01.org>
Signed-off-by: Vishal Verma <vishal.l.verma@intel.com>
---
 drivers/acpi/utils.c    | 23 +++++++++++++++++++++++
 include/acpi/acpi_bus.h |  1 +
 2 files changed, 24 insertions(+)

diff --git a/drivers/acpi/utils.c b/drivers/acpi/utils.c
index 475c907..78f7c69 100644
--- a/drivers/acpi/utils.c
+++ b/drivers/acpi/utils.c
@@ -614,6 +614,29 @@ acpi_status acpi_evaluate_lck(acpi_handle handle, int lock)
 }
 
 /**
+ * acpi_evaluate_fit: Evaluate _FIT method to get an updated NFIT
+ * @handle: ACPI device handle
+ * @buf: buffer for the updated NFIT
+ *
+ * Evaluate device's _FIT method if present to get an updated NFIT
+ */
+acpi_status acpi_evaluate_fit(acpi_handle handle, struct acpi_buffer **buf)
+{
+	acpi_status status;
+	struct acpi_buffer buffer = { ACPI_ALLOCATE_BUFFER, NULL };
+
+	status = acpi_evaluate_object(handle, "_FIT", NULL, &buffer);
+
+	if (ACPI_FAILURE(status))
+		return status;
+
+	*buf = &buffer;
+
+	return status;
+}
+EXPORT_SYMBOL(acpi_evaluate_fit);
+
+/**
  * acpi_evaluate_dsm - evaluate device's _DSM method
  * @handle: ACPI device handle
  * @uuid: UUID of requested functions, should be 16 bytes
diff --git a/include/acpi/acpi_bus.h b/include/acpi/acpi_bus.h
index 5ba8fb6..7483399 100644
--- a/include/acpi/acpi_bus.h
+++ b/include/acpi/acpi_bus.h
@@ -61,6 +61,7 @@ bool acpi_ata_match(acpi_handle handle);
 bool acpi_bay_match(acpi_handle handle);
 bool acpi_dock_match(acpi_handle handle);
 
+acpi_status acpi_evaluate_fit(acpi_handle handle, struct acpi_buffer **buf);
 bool acpi_check_dsm(acpi_handle handle, const u8 *uuid, int rev, u64 funcs);
 union acpi_object *acpi_evaluate_dsm(acpi_handle handle, const u8 *uuid,
 			int rev, int func, union acpi_object *argv4);
-- 
2.4.3

[PATCH 3/3] acpi: nfit: Add support for hotplug

[Vishal Verma]

Add a .notify callback to the acpi_nfit_driver that gets called on a
hotplug event. From this, evaluate the _FIT ACPI method which returns
the updated NFIT with handles for the hot-plugged NVDIMM.

Iterate over the new NFIT, and add any new tables found, and
register/enable the corresponding regions.

In the nfit test framework, after normal initialization, update the NFIT
with a new hot-plugged NVDIMM, and directly call into the driver to
update its view of the available regions.

Cc: Dan Williams <dan.j.williams@intel.com>
Cc: Rafael J. Wysocki <rafael.j.wysocki@intel.com>
Cc: <linux-acpi@vger.kernel.org>
Cc: <linux-nvdimm@lists.01.org>
Signed-off-by: Vishal Verma <vishal.l.verma@intel.com>
---
 drivers/acpi/nfit.c              | 136 ++++++++++++++++++++++++++++++++----
 drivers/acpi/nfit.h              |   2 +
 tools/testing/nvdimm/test/nfit.c | 144 ++++++++++++++++++++++++++++++++++++++-
 3 files changed, 267 insertions(+), 15 deletions(-)

diff --git a/drivers/acpi/nfit.c b/drivers/acpi/nfit.c
index ed599d1..2c24466 100644
--- a/drivers/acpi/nfit.c
+++ b/drivers/acpi/nfit.c
@@ -224,9 +224,13 @@ static bool add_spa(struct acpi_nfit_desc *acpi_desc,
 		struct acpi_nfit_system_address *spa)
 {
 	struct device *dev = acpi_desc->dev;
-	struct nfit_spa *nfit_spa = devm_kzalloc(dev, sizeof(*nfit_spa),
-			GFP_KERNEL);
+	struct nfit_spa *nfit_spa;
+
+	list_for_each_entry(nfit_spa, &acpi_desc->spas, list)
+		if (memcmp(nfit_spa->spa, spa, sizeof(*spa)) == 0)
+			return true;
 
+	nfit_spa = devm_kzalloc(dev, sizeof(*nfit_spa), GFP_KERNEL);
 	if (!nfit_spa)
 		return false;
 	INIT_LIST_HEAD(&nfit_spa->list);
@@ -242,9 +246,13 @@ static bool add_memdev(struct acpi_nfit_desc *acpi_desc,
 		struct acpi_nfit_memory_map *memdev)
 {
 	struct device *dev = acpi_desc->dev;
-	struct nfit_memdev *nfit_memdev = devm_kzalloc(dev,
-			sizeof(*nfit_memdev), GFP_KERNEL);
+	struct nfit_memdev *nfit_memdev;
+
+	list_for_each_entry(nfit_memdev, &acpi_desc->memdevs, list)
+		if (memcmp(nfit_memdev->memdev, memdev, sizeof(*memdev)) == 0)
+			return true;
 
+	nfit_memdev = devm_kzalloc(dev, sizeof(*nfit_memdev), GFP_KERNEL);
 	if (!nfit_memdev)
 		return false;
 	INIT_LIST_HEAD(&nfit_memdev->list);
@@ -260,9 +268,13 @@ static bool add_dcr(struct acpi_nfit_desc *acpi_desc,
 		struct acpi_nfit_control_region *dcr)
 {
 	struct device *dev = acpi_desc->dev;
-	struct nfit_dcr *nfit_dcr = devm_kzalloc(dev, sizeof(*nfit_dcr),
-			GFP_KERNEL);
+	struct nfit_dcr *nfit_dcr;
 
+	list_for_each_entry(nfit_dcr, &acpi_desc->dcrs, list)
+		if (memcmp(nfit_dcr->dcr, dcr, sizeof(*dcr)) == 0)
+			return true;
+
+	nfit_dcr = devm_kzalloc(dev, sizeof(*nfit_dcr), GFP_KERNEL);
 	if (!nfit_dcr)
 		return false;
 	INIT_LIST_HEAD(&nfit_dcr->list);
@@ -277,9 +289,13 @@ static bool add_bdw(struct acpi_nfit_desc *acpi_desc,
 		struct acpi_nfit_data_region *bdw)
 {
 	struct device *dev = acpi_desc->dev;
-	struct nfit_bdw *nfit_bdw = devm_kzalloc(dev, sizeof(*nfit_bdw),
-			GFP_KERNEL);
+	struct nfit_bdw *nfit_bdw;
 
+	list_for_each_entry(nfit_bdw, &acpi_desc->bdws, list)
+		if (memcmp(nfit_bdw->bdw, bdw, sizeof(*bdw)) == 0)
+			return true;
+
+	nfit_bdw = devm_kzalloc(dev, sizeof(*nfit_bdw), GFP_KERNEL);
 	if (!nfit_bdw)
 		return false;
 	INIT_LIST_HEAD(&nfit_bdw->list);
@@ -294,9 +310,13 @@ static bool add_idt(struct acpi_nfit_desc *acpi_desc,
 		struct acpi_nfit_interleave *idt)
 {
 	struct device *dev = acpi_desc->dev;
-	struct nfit_idt *nfit_idt = devm_kzalloc(dev, sizeof(*nfit_idt),
-			GFP_KERNEL);
+	struct nfit_idt *nfit_idt;
 
+	list_for_each_entry(nfit_idt, &acpi_desc->idts, list)
+		if (memcmp(nfit_idt->idt, idt, sizeof(*idt)) == 0)
+			return true;
+
+	nfit_idt = devm_kzalloc(dev, sizeof(*nfit_idt), GFP_KERNEL);
 	if (!nfit_idt)
 		return false;
 	INIT_LIST_HEAD(&nfit_idt->list);
@@ -311,9 +331,13 @@ static bool add_flush(struct acpi_nfit_desc *acpi_desc,
 		struct acpi_nfit_flush_address *flush)
 {
 	struct device *dev = acpi_desc->dev;
-	struct nfit_flush *nfit_flush = devm_kzalloc(dev, sizeof(*nfit_flush),
-			GFP_KERNEL);
+	struct nfit_flush *nfit_flush;
 
+	list_for_each_entry(nfit_flush, &acpi_desc->flushes, list)
+		if (memcmp(nfit_flush->flush, flush, sizeof(*flush)) == 0)
+			return true;
+
+	nfit_flush = devm_kzalloc(dev, sizeof(*nfit_flush), GFP_KERNEL);
 	if (!nfit_flush)
 		return false;
 	INIT_LIST_HEAD(&nfit_flush->list);
@@ -811,6 +835,8 @@ static int acpi_nfit_register_dimms(struct acpi_nfit_desc *acpi_desc)
 			 */
 			dev_err(acpi_desc->dev, "duplicate DCR detected: %s\n",
 					nvdimm_name(nvdimm));
+			/* TODO Do we need the warning? */
+			dimm_count++;
 			continue;
 		}
 
@@ -1532,6 +1558,8 @@ static int acpi_nfit_register_region(struct acpi_nfit_desc *acpi_desc,
 		if (!nvdimm_volatile_region_create(nvdimm_bus, ndr_desc))
 			return -ENOMEM;
 	}
+
+	acpi_desc->last_init_spa = nfit_spa;
 	return 0;
 }
 
@@ -1539,7 +1567,15 @@ static int acpi_nfit_register_regions(struct acpi_nfit_desc *acpi_desc)
 {
 	struct nfit_spa *nfit_spa;
 
-	list_for_each_entry(nfit_spa, &acpi_desc->spas, list) {
+	if (acpi_desc->last_init_spa) {
+		nfit_spa = acpi_desc->last_init_spa;
+		nfit_spa = list_next_entry(nfit_spa, list);
+
+	} else
+		nfit_spa = list_first_entry(&acpi_desc->spas,
+				typeof(*nfit_spa), list);
+
+	list_for_each_entry_from(nfit_spa, &acpi_desc->spas, list) {
 		int rc = acpi_nfit_register_region(acpi_desc, nfit_spa);
 
 		if (rc)
@@ -1590,6 +1626,52 @@ int acpi_nfit_init(struct acpi_nfit_desc *acpi_desc, acpi_size sz)
 }
 EXPORT_SYMBOL_GPL(acpi_nfit_init);
 
+int acpi_nfit_merge(struct acpi_nfit_desc *acpi_desc, acpi_size sz)
+{
+	struct device *dev = acpi_desc->dev;
+	const void *end;
+	u8 *data;
+	int rc;
+
+	/*
+	 * TODO Can we get here with an uninitialized acpi_desc?
+	 * In the case where the only nvdimm in the system is hotplugged?
+	 */
+	if (!acpi_desc) {
+		dev_err(dev, "%s: acpi_desc uninitialized\n", __func__);
+		return -ENXIO;
+	}
+
+	/*
+	 * At this point, acpi_desc->nfit will have the updated nfit table,
+	 * but the various lists in acpi_dev correspond to the old table.
+	 */
+	data = (u8 *) acpi_desc->nfit;
+	end = data + sz;
+	data += sizeof(struct acpi_table_nfit);
+	while (!IS_ERR_OR_NULL(data))
+		data = add_table(acpi_desc, data, end);
+
+	if (IS_ERR(data)) {
+		dev_dbg(dev, "%s: nfit table parsing error: %ld\n", __func__,
+				PTR_ERR(data));
+		return PTR_ERR(data);
+	}
+
+	if (nfit_mem_init(acpi_desc) != 0)
+		return -ENOMEM;
+
+	acpi_nfit_init_dsms(acpi_desc);
+
+	rc = acpi_nfit_register_dimms(acpi_desc);
+	if (rc)
+		return rc;
+
+	rc = acpi_nfit_register_regions(acpi_desc);
+	return rc;
+}
+EXPORT_SYMBOL_GPL(acpi_nfit_merge);
+
 static int acpi_nfit_add(struct acpi_device *adev)
 {
 	struct nvdimm_bus_descriptor *nd_desc;
@@ -1639,6 +1721,33 @@ static int acpi_nfit_remove(struct acpi_device *adev)
 	return 0;
 }
 
+static void acpi_nfit_notify(struct acpi_device *adev, u32 event)
+{
+	struct acpi_nfit_desc *acpi_desc = dev_get_drvdata(&adev->dev);
+	struct acpi_table_nfit *nfit_saved;
+	struct device *dev = &adev->dev;
+	struct acpi_buffer *buf;
+	acpi_status status;
+	int ret;
+
+	dev_dbg(dev, "%s: event: %d\n", __func__, event);
+
+	/* Evaluate _FIT */
+	status = acpi_evaluate_fit(adev->handle, &buf);
+	if (ACPI_FAILURE(status))
+		dev_err(dev, "failed to evaluate _FIT\n");
+
+	nfit_saved = acpi_desc->nfit;
+	acpi_desc->nfit = (struct acpi_table_nfit *)buf;
+	ret = acpi_nfit_merge(acpi_desc, buf->length);
+	if (!ret) {
+		/* Merge failed, restore old nfit buf, and exit */
+		acpi_desc->nfit = nfit_saved;
+		dev_err(dev, "failed to merge updated NFIT\n");
+	}
+	kfree(buf->pointer);
+}
+
 static const struct acpi_device_id acpi_nfit_ids[] = {
 	{ "ACPI0012", 0 },
 	{ "", 0 },
@@ -1651,6 +1760,7 @@ static struct acpi_driver acpi_nfit_driver = {
 	.ops = {
 		.add = acpi_nfit_add,
 		.remove = acpi_nfit_remove,
+		.notify = acpi_nfit_notify,
 	},
 };
 
diff --git a/drivers/acpi/nfit.h b/drivers/acpi/nfit.h
index 7e74015..9f4758f 100644
--- a/drivers/acpi/nfit.h
+++ b/drivers/acpi/nfit.h
@@ -105,6 +105,7 @@ struct acpi_nfit_desc {
 	struct list_head dcrs;
 	struct list_head bdws;
 	struct list_head idts;
+	struct nfit_spa *last_init_spa;
 	struct nvdimm_bus *nvdimm_bus;
 	struct device *dev;
 	unsigned long dimm_dsm_force_en;
@@ -179,5 +180,6 @@ static inline struct acpi_nfit_desc *to_acpi_desc(
 
 const u8 *to_nfit_uuid(enum nfit_uuids id);
 int acpi_nfit_init(struct acpi_nfit_desc *nfit, acpi_size sz);
+int acpi_nfit_merge(struct acpi_nfit_desc *acpi_desc, acpi_size sz);
 extern const struct attribute_group *acpi_nfit_attribute_groups[];
 #endif /* __NFIT_H__ */
diff --git a/tools/testing/nvdimm/test/nfit.c b/tools/testing/nvdimm/test/nfit.c
index 021e6f9..3a7b691 100644
--- a/tools/testing/nvdimm/test/nfit.c
+++ b/tools/testing/nvdimm/test/nfit.c
@@ -44,6 +44,15 @@
  * +------+  |                 blk5.0             |  pm1.0  |    3      region5
  *           +-------------------------+----------+-+-------+
  *
+ * +--+---+
+ * | cpu1 |
+ * +--+---+                   (Hotplug DIMM)
+ *    |      +----------------------------------------------+
+ * +--+---+  |                 blk6.0/pm7.0                 |    4      region6
+ * | imc0 +--+----------------------------------------------+
+ * +------+
+ *
+ *
  * *) In this layout we have four dimms and two memory controllers in one
  *    socket.  Each unique interface (BLK or PMEM) to DPA space
  *    is identified by a region device with a dynamically assigned id.
@@ -85,8 +94,8 @@
  *    reference an NVDIMM.
  */
 enum {
-	NUM_PM  = 2,
-	NUM_DCR = 4,
+	NUM_PM  = 3,
+	NUM_DCR = 5,
 	NUM_BDW = NUM_DCR,
 	NUM_SPA = NUM_PM + NUM_DCR + NUM_BDW,
 	NUM_MEM = NUM_DCR + NUM_BDW + 2 /* spa0 iset */ + 4 /* spa1 iset */,
@@ -115,6 +124,7 @@ static u32 handle[NUM_DCR] = {
 	[1] = NFIT_DIMM_HANDLE(0, 0, 0, 0, 1),
 	[2] = NFIT_DIMM_HANDLE(0, 0, 1, 0, 0),
 	[3] = NFIT_DIMM_HANDLE(0, 0, 1, 0, 1),
+	[4] = NFIT_DIMM_HANDLE(0, 1, 0, 0, 0),
 };
 
 struct nfit_test {
@@ -138,6 +148,7 @@ struct nfit_test {
 	dma_addr_t *dcr_dma;
 	int (*alloc)(struct nfit_test *t);
 	void (*setup)(struct nfit_test *t);
+	int setup_hotplug;
 };
 
 static struct nfit_test *to_nfit_test(struct device *dev)
@@ -428,6 +439,10 @@ static int nfit_test0_alloc(struct nfit_test *t)
 	if (!t->spa_set[1])
 		return -ENOMEM;
 
+	t->spa_set[2] = test_alloc_coherent(t, SPA0_SIZE, &t->spa_set_dma[2]);
+	if (!t->spa_set[2])
+		return -ENOMEM;
+
 	for (i = 0; i < NUM_DCR; i++) {
 		t->dimm[i] = test_alloc(t, DIMM_SIZE, &t->dimm_dma[i]);
 		if (!t->dimm[i])
@@ -950,6 +965,119 @@ static void nfit_test0_setup(struct nfit_test *t)
 	flush->hint_count = 1;
 	flush->hint_address[0] = t->flush_dma[3];
 
+
+	if (t->setup_hotplug) {
+		offset = offset + sizeof(struct acpi_nfit_flush_address) * 4;
+		/* dcr-descriptor4 */
+		dcr = nfit_buf + offset;
+		dcr->header.type = ACPI_NFIT_TYPE_CONTROL_REGION;
+		dcr->header.length = sizeof(struct acpi_nfit_control_region);
+		dcr->region_index = 4+1;
+		dcr->vendor_id = 0xabcd;
+		dcr->device_id = 0;
+		dcr->revision_id = 1;
+		dcr->serial_number = ~handle[4];
+		dcr->windows = 0;
+		dcr->window_size = 0;
+		dcr->command_offset = 0;
+		dcr->command_size = 0;
+		dcr->status_offset = 0;
+		dcr->status_size = 0;
+
+		offset = offset + sizeof(struct acpi_nfit_control_region);
+		/* bdw4 (spa/dcr4, dimm4) */
+		bdw = nfit_buf + offset;
+		bdw->header.type = ACPI_NFIT_TYPE_DATA_REGION;
+		bdw->header.length = sizeof(struct acpi_nfit_data_region);
+		bdw->region_index = 4+1;
+		bdw->windows = 1;
+		bdw->offset = 0;
+		bdw->size = BDW_SIZE;
+		bdw->capacity = DIMM_SIZE;
+		bdw->start_address = 0;
+
+		offset = offset + sizeof(struct acpi_nfit_data_region);
+		/* spa10 (dcr4) dimm4 */
+		spa = nfit_buf + offset;
+		spa->header.type = ACPI_NFIT_TYPE_SYSTEM_ADDRESS;
+		spa->header.length = sizeof(*spa);
+		memcpy(spa->range_guid, to_nfit_uuid(NFIT_SPA_DCR), 16);
+		spa->range_index = 10+1;
+		spa->address = t->dcr_dma[4];
+		spa->length = DCR_SIZE;
+
+		/*
+		 * spa11 (single-dimm interleave for hotplug, note storage
+		 * does not actually alias the related block-data-window
+		 * regions)
+		 */
+		spa = nfit_buf + offset + sizeof(*spa);
+		spa->header.type = ACPI_NFIT_TYPE_SYSTEM_ADDRESS;
+		spa->header.length = sizeof(*spa);
+		memcpy(spa->range_guid, to_nfit_uuid(NFIT_SPA_PM), 16);
+		spa->range_index = 11+1;
+		spa->address = t->spa_set_dma[2];
+		spa->length = SPA0_SIZE;
+
+		/* spa12 (bdw for dcr4) dimm4 */
+		spa = nfit_buf + offset + sizeof(*spa) * 2;
+		spa->header.type = ACPI_NFIT_TYPE_SYSTEM_ADDRESS;
+		spa->header.length = sizeof(*spa);
+		memcpy(spa->range_guid, to_nfit_uuid(NFIT_SPA_BDW), 16);
+		spa->range_index = 12+1;
+		spa->address = t->dimm_dma[4];
+		spa->length = DIMM_SIZE;
+
+		offset = offset + sizeof(*spa) * 3;
+		/* mem-region14 (spa/dcr4, dimm4) */
+		memdev = nfit_buf + offset;
+		memdev->header.type = ACPI_NFIT_TYPE_MEMORY_MAP;
+		memdev->header.length = sizeof(*memdev);
+		memdev->device_handle = handle[4];
+		memdev->physical_id = 4;
+		memdev->region_id = 0;
+		memdev->range_index = 10+1;
+		memdev->region_index = 4+1;
+		memdev->region_size = 0;
+		memdev->region_offset = 0;
+		memdev->address = 0;
+		memdev->interleave_index = 0;
+		memdev->interleave_ways = 1;
+
+		/* mem-region15 (spa0, dimm4) */
+		memdev = nfit_buf + offset +
+				sizeof(struct acpi_nfit_memory_map);
+		memdev->header.type = ACPI_NFIT_TYPE_MEMORY_MAP;
+		memdev->header.length = sizeof(*memdev);
+		memdev->device_handle = handle[4];
+		memdev->physical_id = 4;
+		memdev->region_id = 0;
+		memdev->range_index = 11+1;
+		memdev->region_index = 4+1;
+		memdev->region_size = SPA0_SIZE;
+		memdev->region_offset = t->spa_set_dma[2];
+		memdev->address = 0;
+		memdev->interleave_index = 0;
+		memdev->interleave_ways = 1;
+
+		/* mem-region16 (spa/dcr4, dimm4) */
+		memdev = nfit_buf + offset +
+				sizeof(struct acpi_nfit_memory_map) * 2;
+		memdev->header.type = ACPI_NFIT_TYPE_MEMORY_MAP;
+		memdev->header.length = sizeof(*memdev);
+		memdev->device_handle = handle[4];
+		memdev->physical_id = 4;
+		memdev->region_id = 0;
+		memdev->range_index = 12+1;
+		memdev->region_index = 4+1;
+		memdev->region_size = 0;
+		memdev->region_offset = 0;
+		memdev->address = 0;
+		memdev->interleave_index = 0;
+		memdev->interleave_ways = 1;
+
+	}
+
 	acpi_desc = &t->acpi_desc;
 	set_bit(ND_CMD_GET_CONFIG_SIZE, &acpi_desc->dimm_dsm_force_en);
 	set_bit(ND_CMD_GET_CONFIG_DATA, &acpi_desc->dimm_dsm_force_en);
@@ -1114,6 +1242,18 @@ static int nfit_test_probe(struct platform_device *pdev)
 		return rc;
 	}
 
+	if (nfit_test->setup != nfit_test0_setup)
+		return 0;
+
+	nfit_test->setup_hotplug = 1;
+	nfit_test->setup(nfit_test);
+
+	rc = acpi_nfit_merge(acpi_desc, nfit_test->nfit_size);
+	if (rc) {
+		nvdimm_bus_unregister(acpi_desc->nvdimm_bus);
+		return rc;
+	}
+
 	return 0;
 }
 
-- 
2.4.3

Re: [PATCH 1/3] nfit: in acpi_nfit_init, break on a 0-length table

[Jeff Moyer]

Vishal Verma <vishal.l.verma@intel.com> writes:

> If acpi_nfit_init is called (such as from nfit_test), with an nfit table
> that has more memory allocated than it needs (and a similarly large
> 'size' field, add_tables would happily keep adding null SPA Range tables
> filling up all available memory.
>
> Make it friendlier by breaking out if a 0-length header is found in any
> of the tables.

Shouldn't that at least spew a warning?  Or does the spec allow for
zero-length tables?

-Jeff

>
> Cc: Dan Williams <dan.j.williams@intel.com>
> Cc: Rafael J. Wysocki <rafael.j.wysocki@intel.com>
> Cc: <linux-acpi@vger.kernel.org>
> Cc: <linux-nvdimm@lists.01.org>
> Signed-off-by: Vishal Verma <vishal.l.verma@intel.com>
> ---
>  drivers/acpi/nfit.c | 3 +++
>  1 file changed, 3 insertions(+)
>
> diff --git a/drivers/acpi/nfit.c b/drivers/acpi/nfit.c
> index c1b8d03..ed599d1 100644
> --- a/drivers/acpi/nfit.c
> +++ b/drivers/acpi/nfit.c
> @@ -335,6 +335,9 @@ static void *add_table(struct acpi_nfit_desc *acpi_desc, void *table,
>  		return NULL;
>  
>  	hdr = table;
> +	if (!hdr->length)
> +		return NULL;
> +
>  	switch (hdr->type) {
>  	case ACPI_NFIT_TYPE_SYSTEM_ADDRESS:
>  		if (!add_spa(acpi_desc, table))

Re: [PATCH 1/3] nfit: in acpi_nfit_init, break on a 0-length table

[Dan Williams]

On Fri, Oct 9, 2015 at 10:23 AM, Jeff Moyer <jmoyer@redhat.com> wrote:
> Vishal Verma <vishal.l.verma@intel.com> writes:
>
>> If acpi_nfit_init is called (such as from nfit_test), with an nfit table
>> that has more memory allocated than it needs (and a similarly large
>> 'size' field, add_tables would happily keep adding null SPA Range tables
>> filling up all available memory.
>>
>> Make it friendlier by breaking out if a 0-length header is found in any
>> of the tables.
>
> Shouldn't that at least spew a warning?  Or does the spec allow for
> zero-length tables?
>

The spec allows for zero length tables but the firmware implementation
should be self consistent and not report a total NFIT size that is
greater than the sum of the size of the sub-structures.  A warning /
nudge to firmware developers to fix their stuff seems appropriate.

Re: [PATCH 2/3] acpi: add a utility function for evaluating _FIT

[Jeff Moyer]

Vishal Verma <vishal.l.verma@intel.com> writes:

>  /**
> + * acpi_evaluate_fit: Evaluate _FIT method to get an updated NFIT
> + * @handle: ACPI device handle
> + * @buf: buffer for the updated NFIT
> + *
> + * Evaluate device's _FIT method if present to get an updated NFIT
> + */
> +acpi_status acpi_evaluate_fit(acpi_handle handle, struct acpi_buffer **buf)
> +{
> +	acpi_status status;
> +	struct acpi_buffer buffer = { ACPI_ALLOCATE_BUFFER, NULL };
> +
> +	status = acpi_evaluate_object(handle, "_FIT", NULL, &buffer);
> +
> +	if (ACPI_FAILURE(status))
> +		return status;
> +
> +	*buf = &buffer;

Umm, unless I'm missing something, you're returning a stack address.

-Jeff

Re: [PATCH 3/3] acpi: nfit: Add support for hotplug

[Jeff Moyer]

Vishal Verma <vishal.l.verma@intel.com> writes:

> Add a .notify callback to the acpi_nfit_driver that gets called on a
> hotplug event. From this, evaluate the _FIT ACPI method which returns
> the updated NFIT with handles for the hot-plugged NVDIMM.
>
> Iterate over the new NFIT, and add any new tables found, and
> register/enable the corresponding regions.
>
> In the nfit test framework, after normal initialization, update the NFIT
> with a new hot-plugged NVDIMM, and directly call into the driver to
> update its view of the available regions.

OK, so just plugging in new NVDIMMs is supported, right?  Are there
plans to support unplugging DIMMs?

-Jeff

>
> Cc: Dan Williams <dan.j.williams@intel.com>
> Cc: Rafael J. Wysocki <rafael.j.wysocki@intel.com>
> Cc: <linux-acpi@vger.kernel.org>
> Cc: <linux-nvdimm@lists.01.org>
> Signed-off-by: Vishal Verma <vishal.l.verma@intel.com>
> ---
>  drivers/acpi/nfit.c              | 136 ++++++++++++++++++++++++++++++++----
>  drivers/acpi/nfit.h              |   2 +
>  tools/testing/nvdimm/test/nfit.c | 144 ++++++++++++++++++++++++++++++++++++++-
>  3 files changed, 267 insertions(+), 15 deletions(-)
>
> diff --git a/drivers/acpi/nfit.c b/drivers/acpi/nfit.c
> index ed599d1..2c24466 100644
> --- a/drivers/acpi/nfit.c
> +++ b/drivers/acpi/nfit.c
> @@ -224,9 +224,13 @@ static bool add_spa(struct acpi_nfit_desc *acpi_desc,
>  		struct acpi_nfit_system_address *spa)
>  {
>  	struct device *dev = acpi_desc->dev;
> -	struct nfit_spa *nfit_spa = devm_kzalloc(dev, sizeof(*nfit_spa),
> -			GFP_KERNEL);
> +	struct nfit_spa *nfit_spa;
> +
> +	list_for_each_entry(nfit_spa, &acpi_desc->spas, list)
> +		if (memcmp(nfit_spa->spa, spa, sizeof(*spa)) == 0)
> +			return true;
>  
> +	nfit_spa = devm_kzalloc(dev, sizeof(*nfit_spa), GFP_KERNEL);
>  	if (!nfit_spa)
>  		return false;
>  	INIT_LIST_HEAD(&nfit_spa->list);
> @@ -242,9 +246,13 @@ static bool add_memdev(struct acpi_nfit_desc *acpi_desc,
>  		struct acpi_nfit_memory_map *memdev)
>  {
>  	struct device *dev = acpi_desc->dev;
> -	struct nfit_memdev *nfit_memdev = devm_kzalloc(dev,
> -			sizeof(*nfit_memdev), GFP_KERNEL);
> +	struct nfit_memdev *nfit_memdev;
> +
> +	list_for_each_entry(nfit_memdev, &acpi_desc->memdevs, list)
> +		if (memcmp(nfit_memdev->memdev, memdev, sizeof(*memdev)) == 0)
> +			return true;
>  
> +	nfit_memdev = devm_kzalloc(dev, sizeof(*nfit_memdev), GFP_KERNEL);
>  	if (!nfit_memdev)
>  		return false;
>  	INIT_LIST_HEAD(&nfit_memdev->list);
> @@ -260,9 +268,13 @@ static bool add_dcr(struct acpi_nfit_desc *acpi_desc,
>  		struct acpi_nfit_control_region *dcr)
>  {
>  	struct device *dev = acpi_desc->dev;
> -	struct nfit_dcr *nfit_dcr = devm_kzalloc(dev, sizeof(*nfit_dcr),
> -			GFP_KERNEL);
> +	struct nfit_dcr *nfit_dcr;
>  
> +	list_for_each_entry(nfit_dcr, &acpi_desc->dcrs, list)
> +		if (memcmp(nfit_dcr->dcr, dcr, sizeof(*dcr)) == 0)
> +			return true;
> +
> +	nfit_dcr = devm_kzalloc(dev, sizeof(*nfit_dcr), GFP_KERNEL);
>  	if (!nfit_dcr)
>  		return false;
>  	INIT_LIST_HEAD(&nfit_dcr->list);
> @@ -277,9 +289,13 @@ static bool add_bdw(struct acpi_nfit_desc *acpi_desc,
>  		struct acpi_nfit_data_region *bdw)
>  {
>  	struct device *dev = acpi_desc->dev;
> -	struct nfit_bdw *nfit_bdw = devm_kzalloc(dev, sizeof(*nfit_bdw),
> -			GFP_KERNEL);
> +	struct nfit_bdw *nfit_bdw;
>  
> +	list_for_each_entry(nfit_bdw, &acpi_desc->bdws, list)
> +		if (memcmp(nfit_bdw->bdw, bdw, sizeof(*bdw)) == 0)
> +			return true;
> +
> +	nfit_bdw = devm_kzalloc(dev, sizeof(*nfit_bdw), GFP_KERNEL);
>  	if (!nfit_bdw)
>  		return false;
>  	INIT_LIST_HEAD(&nfit_bdw->list);
> @@ -294,9 +310,13 @@ static bool add_idt(struct acpi_nfit_desc *acpi_desc,
>  		struct acpi_nfit_interleave *idt)
>  {
>  	struct device *dev = acpi_desc->dev;
> -	struct nfit_idt *nfit_idt = devm_kzalloc(dev, sizeof(*nfit_idt),
> -			GFP_KERNEL);
> +	struct nfit_idt *nfit_idt;
>  
> +	list_for_each_entry(nfit_idt, &acpi_desc->idts, list)
> +		if (memcmp(nfit_idt->idt, idt, sizeof(*idt)) == 0)
> +			return true;
> +
> +	nfit_idt = devm_kzalloc(dev, sizeof(*nfit_idt), GFP_KERNEL);
>  	if (!nfit_idt)
>  		return false;
>  	INIT_LIST_HEAD(&nfit_idt->list);
> @@ -311,9 +331,13 @@ static bool add_flush(struct acpi_nfit_desc *acpi_desc,
>  		struct acpi_nfit_flush_address *flush)
>  {
>  	struct device *dev = acpi_desc->dev;
> -	struct nfit_flush *nfit_flush = devm_kzalloc(dev, sizeof(*nfit_flush),
> -			GFP_KERNEL);
> +	struct nfit_flush *nfit_flush;
>  
> +	list_for_each_entry(nfit_flush, &acpi_desc->flushes, list)
> +		if (memcmp(nfit_flush->flush, flush, sizeof(*flush)) == 0)
> +			return true;
> +
> +	nfit_flush = devm_kzalloc(dev, sizeof(*nfit_flush), GFP_KERNEL);
>  	if (!nfit_flush)
>  		return false;
>  	INIT_LIST_HEAD(&nfit_flush->list);
> @@ -811,6 +835,8 @@ static int acpi_nfit_register_dimms(struct acpi_nfit_desc *acpi_desc)
>  			 */
>  			dev_err(acpi_desc->dev, "duplicate DCR detected: %s\n",
>  					nvdimm_name(nvdimm));
> +			/* TODO Do we need the warning? */
> +			dimm_count++;
>  			continue;
>  		}
>  
> @@ -1532,6 +1558,8 @@ static int acpi_nfit_register_region(struct acpi_nfit_desc *acpi_desc,
>  		if (!nvdimm_volatile_region_create(nvdimm_bus, ndr_desc))
>  			return -ENOMEM;
>  	}
> +
> +	acpi_desc->last_init_spa = nfit_spa;
>  	return 0;
>  }
>  
> @@ -1539,7 +1567,15 @@ static int acpi_nfit_register_regions(struct acpi_nfit_desc *acpi_desc)
>  {
>  	struct nfit_spa *nfit_spa;
>  
> -	list_for_each_entry(nfit_spa, &acpi_desc->spas, list) {
> +	if (acpi_desc->last_init_spa) {
> +		nfit_spa = acpi_desc->last_init_spa;
> +		nfit_spa = list_next_entry(nfit_spa, list);
> +
> +	} else
> +		nfit_spa = list_first_entry(&acpi_desc->spas,
> +				typeof(*nfit_spa), list);
> +
> +	list_for_each_entry_from(nfit_spa, &acpi_desc->spas, list) {
>  		int rc = acpi_nfit_register_region(acpi_desc, nfit_spa);
>  
>  		if (rc)
> @@ -1590,6 +1626,52 @@ int acpi_nfit_init(struct acpi_nfit_desc *acpi_desc, acpi_size sz)
>  }
>  EXPORT_SYMBOL_GPL(acpi_nfit_init);
>  
> +int acpi_nfit_merge(struct acpi_nfit_desc *acpi_desc, acpi_size sz)
> +{
> +	struct device *dev = acpi_desc->dev;
> +	const void *end;
> +	u8 *data;
> +	int rc;
> +
> +	/*
> +	 * TODO Can we get here with an uninitialized acpi_desc?
> +	 * In the case where the only nvdimm in the system is hotplugged?
> +	 */
> +	if (!acpi_desc) {
> +		dev_err(dev, "%s: acpi_desc uninitialized\n", __func__);
> +		return -ENXIO;
> +	}
> +
> +	/*
> +	 * At this point, acpi_desc->nfit will have the updated nfit table,
> +	 * but the various lists in acpi_dev correspond to the old table.
> +	 */
> +	data = (u8 *) acpi_desc->nfit;
> +	end = data + sz;
> +	data += sizeof(struct acpi_table_nfit);
> +	while (!IS_ERR_OR_NULL(data))
> +		data = add_table(acpi_desc, data, end);
> +
> +	if (IS_ERR(data)) {
> +		dev_dbg(dev, "%s: nfit table parsing error: %ld\n", __func__,
> +				PTR_ERR(data));
> +		return PTR_ERR(data);
> +	}
> +
> +	if (nfit_mem_init(acpi_desc) != 0)
> +		return -ENOMEM;
> +
> +	acpi_nfit_init_dsms(acpi_desc);
> +
> +	rc = acpi_nfit_register_dimms(acpi_desc);
> +	if (rc)
> +		return rc;
> +
> +	rc = acpi_nfit_register_regions(acpi_desc);
> +	return rc;
> +}
> +EXPORT_SYMBOL_GPL(acpi_nfit_merge);
> +
>  static int acpi_nfit_add(struct acpi_device *adev)
>  {
>  	struct nvdimm_bus_descriptor *nd_desc;
> @@ -1639,6 +1721,33 @@ static int acpi_nfit_remove(struct acpi_device *adev)
>  	return 0;
>  }
>  
> +static void acpi_nfit_notify(struct acpi_device *adev, u32 event)
> +{
> +	struct acpi_nfit_desc *acpi_desc = dev_get_drvdata(&adev->dev);
> +	struct acpi_table_nfit *nfit_saved;
> +	struct device *dev = &adev->dev;
> +	struct acpi_buffer *buf;
> +	acpi_status status;
> +	int ret;
> +
> +	dev_dbg(dev, "%s: event: %d\n", __func__, event);
> +
> +	/* Evaluate _FIT */
> +	status = acpi_evaluate_fit(adev->handle, &buf);
> +	if (ACPI_FAILURE(status))
> +		dev_err(dev, "failed to evaluate _FIT\n");
> +
> +	nfit_saved = acpi_desc->nfit;
> +	acpi_desc->nfit = (struct acpi_table_nfit *)buf;
> +	ret = acpi_nfit_merge(acpi_desc, buf->length);
> +	if (!ret) {
> +		/* Merge failed, restore old nfit buf, and exit */
> +		acpi_desc->nfit = nfit_saved;
> +		dev_err(dev, "failed to merge updated NFIT\n");
> +	}
> +	kfree(buf->pointer);
> +}
> +
>  static const struct acpi_device_id acpi_nfit_ids[] = {
>  	{ "ACPI0012", 0 },
>  	{ "", 0 },
> @@ -1651,6 +1760,7 @@ static struct acpi_driver acpi_nfit_driver = {
>  	.ops = {
>  		.add = acpi_nfit_add,
>  		.remove = acpi_nfit_remove,
> +		.notify = acpi_nfit_notify,
>  	},
>  };
>  
> diff --git a/drivers/acpi/nfit.h b/drivers/acpi/nfit.h
> index 7e74015..9f4758f 100644
> --- a/drivers/acpi/nfit.h
> +++ b/drivers/acpi/nfit.h
> @@ -105,6 +105,7 @@ struct acpi_nfit_desc {
>  	struct list_head dcrs;
>  	struct list_head bdws;
>  	struct list_head idts;
> +	struct nfit_spa *last_init_spa;
>  	struct nvdimm_bus *nvdimm_bus;
>  	struct device *dev;
>  	unsigned long dimm_dsm_force_en;
> @@ -179,5 +180,6 @@ static inline struct acpi_nfit_desc *to_acpi_desc(
>  
>  const u8 *to_nfit_uuid(enum nfit_uuids id);
>  int acpi_nfit_init(struct acpi_nfit_desc *nfit, acpi_size sz);
> +int acpi_nfit_merge(struct acpi_nfit_desc *acpi_desc, acpi_size sz);
>  extern const struct attribute_group *acpi_nfit_attribute_groups[];
>  #endif /* __NFIT_H__ */
> diff --git a/tools/testing/nvdimm/test/nfit.c b/tools/testing/nvdimm/test/nfit.c
> index 021e6f9..3a7b691 100644
> --- a/tools/testing/nvdimm/test/nfit.c
> +++ b/tools/testing/nvdimm/test/nfit.c
> @@ -44,6 +44,15 @@
>   * +------+  |                 blk5.0             |  pm1.0  |    3      region5
>   *           +-------------------------+----------+-+-------+
>   *
> + * +--+---+
> + * | cpu1 |
> + * +--+---+                   (Hotplug DIMM)
> + *    |      +----------------------------------------------+
> + * +--+---+  |                 blk6.0/pm7.0                 |    4      region6
> + * | imc0 +--+----------------------------------------------+
> + * +------+
> + *
> + *
>   * *) In this layout we have four dimms and two memory controllers in one
>   *    socket.  Each unique interface (BLK or PMEM) to DPA space
>   *    is identified by a region device with a dynamically assigned id.
> @@ -85,8 +94,8 @@
>   *    reference an NVDIMM.
>   */
>  enum {
> -	NUM_PM  = 2,
> -	NUM_DCR = 4,
> +	NUM_PM  = 3,
> +	NUM_DCR = 5,
>  	NUM_BDW = NUM_DCR,
>  	NUM_SPA = NUM_PM + NUM_DCR + NUM_BDW,
>  	NUM_MEM = NUM_DCR + NUM_BDW + 2 /* spa0 iset */ + 4 /* spa1 iset */,
> @@ -115,6 +124,7 @@ static u32 handle[NUM_DCR] = {
>  	[1] = NFIT_DIMM_HANDLE(0, 0, 0, 0, 1),
>  	[2] = NFIT_DIMM_HANDLE(0, 0, 1, 0, 0),
>  	[3] = NFIT_DIMM_HANDLE(0, 0, 1, 0, 1),
> +	[4] = NFIT_DIMM_HANDLE(0, 1, 0, 0, 0),
>  };
>  
>  struct nfit_test {
> @@ -138,6 +148,7 @@ struct nfit_test {
>  	dma_addr_t *dcr_dma;
>  	int (*alloc)(struct nfit_test *t);
>  	void (*setup)(struct nfit_test *t);
> +	int setup_hotplug;
>  };
>  
>  static struct nfit_test *to_nfit_test(struct device *dev)
> @@ -428,6 +439,10 @@ static int nfit_test0_alloc(struct nfit_test *t)
>  	if (!t->spa_set[1])
>  		return -ENOMEM;
>  
> +	t->spa_set[2] = test_alloc_coherent(t, SPA0_SIZE, &t->spa_set_dma[2]);
> +	if (!t->spa_set[2])
> +		return -ENOMEM;
> +
>  	for (i = 0; i < NUM_DCR; i++) {
>  		t->dimm[i] = test_alloc(t, DIMM_SIZE, &t->dimm_dma[i]);
>  		if (!t->dimm[i])
> @@ -950,6 +965,119 @@ static void nfit_test0_setup(struct nfit_test *t)
>  	flush->hint_count = 1;
>  	flush->hint_address[0] = t->flush_dma[3];
>  
> +
> +	if (t->setup_hotplug) {
> +		offset = offset + sizeof(struct acpi_nfit_flush_address) * 4;
> +		/* dcr-descriptor4 */
> +		dcr = nfit_buf + offset;
> +		dcr->header.type = ACPI_NFIT_TYPE_CONTROL_REGION;
> +		dcr->header.length = sizeof(struct acpi_nfit_control_region);
> +		dcr->region_index = 4+1;
> +		dcr->vendor_id = 0xabcd;
> +		dcr->device_id = 0;
> +		dcr->revision_id = 1;
> +		dcr->serial_number = ~handle[4];
> +		dcr->windows = 0;
> +		dcr->window_size = 0;
> +		dcr->command_offset = 0;
> +		dcr->command_size = 0;
> +		dcr->status_offset = 0;
> +		dcr->status_size = 0;
> +
> +		offset = offset + sizeof(struct acpi_nfit_control_region);
> +		/* bdw4 (spa/dcr4, dimm4) */
> +		bdw = nfit_buf + offset;
> +		bdw->header.type = ACPI_NFIT_TYPE_DATA_REGION;
> +		bdw->header.length = sizeof(struct acpi_nfit_data_region);
> +		bdw->region_index = 4+1;
> +		bdw->windows = 1;
> +		bdw->offset = 0;
> +		bdw->size = BDW_SIZE;
> +		bdw->capacity = DIMM_SIZE;
> +		bdw->start_address = 0;
> +
> +		offset = offset + sizeof(struct acpi_nfit_data_region);
> +		/* spa10 (dcr4) dimm4 */
> +		spa = nfit_buf + offset;
> +		spa->header.type = ACPI_NFIT_TYPE_SYSTEM_ADDRESS;
> +		spa->header.length = sizeof(*spa);
> +		memcpy(spa->range_guid, to_nfit_uuid(NFIT_SPA_DCR), 16);
> +		spa->range_index = 10+1;
> +		spa->address = t->dcr_dma[4];
> +		spa->length = DCR_SIZE;
> +
> +		/*
> +		 * spa11 (single-dimm interleave for hotplug, note storage
> +		 * does not actually alias the related block-data-window
> +		 * regions)
> +		 */
> +		spa = nfit_buf + offset + sizeof(*spa);
> +		spa->header.type = ACPI_NFIT_TYPE_SYSTEM_ADDRESS;
> +		spa->header.length = sizeof(*spa);
> +		memcpy(spa->range_guid, to_nfit_uuid(NFIT_SPA_PM), 16);
> +		spa->range_index = 11+1;
> +		spa->address = t->spa_set_dma[2];
> +		spa->length = SPA0_SIZE;
> +
> +		/* spa12 (bdw for dcr4) dimm4 */
> +		spa = nfit_buf + offset + sizeof(*spa) * 2;
> +		spa->header.type = ACPI_NFIT_TYPE_SYSTEM_ADDRESS;
> +		spa->header.length = sizeof(*spa);
> +		memcpy(spa->range_guid, to_nfit_uuid(NFIT_SPA_BDW), 16);
> +		spa->range_index = 12+1;
> +		spa->address = t->dimm_dma[4];
> +		spa->length = DIMM_SIZE;
> +
> +		offset = offset + sizeof(*spa) * 3;
> +		/* mem-region14 (spa/dcr4, dimm4) */
> +		memdev = nfit_buf + offset;
> +		memdev->header.type = ACPI_NFIT_TYPE_MEMORY_MAP;
> +		memdev->header.length = sizeof(*memdev);
> +		memdev->device_handle = handle[4];
> +		memdev->physical_id = 4;
> +		memdev->region_id = 0;
> +		memdev->range_index = 10+1;
> +		memdev->region_index = 4+1;
> +		memdev->region_size = 0;
> +		memdev->region_offset = 0;
> +		memdev->address = 0;
> +		memdev->interleave_index = 0;
> +		memdev->interleave_ways = 1;
> +
> +		/* mem-region15 (spa0, dimm4) */
> +		memdev = nfit_buf + offset +
> +				sizeof(struct acpi_nfit_memory_map);
> +		memdev->header.type = ACPI_NFIT_TYPE_MEMORY_MAP;
> +		memdev->header.length = sizeof(*memdev);
> +		memdev->device_handle = handle[4];
> +		memdev->physical_id = 4;
> +		memdev->region_id = 0;
> +		memdev->range_index = 11+1;
> +		memdev->region_index = 4+1;
> +		memdev->region_size = SPA0_SIZE;
> +		memdev->region_offset = t->spa_set_dma[2];
> +		memdev->address = 0;
> +		memdev->interleave_index = 0;
> +		memdev->interleave_ways = 1;
> +
> +		/* mem-region16 (spa/dcr4, dimm4) */
> +		memdev = nfit_buf + offset +
> +				sizeof(struct acpi_nfit_memory_map) * 2;
> +		memdev->header.type = ACPI_NFIT_TYPE_MEMORY_MAP;
> +		memdev->header.length = sizeof(*memdev);
> +		memdev->device_handle = handle[4];
> +		memdev->physical_id = 4;
> +		memdev->region_id = 0;
> +		memdev->range_index = 12+1;
> +		memdev->region_index = 4+1;
> +		memdev->region_size = 0;
> +		memdev->region_offset = 0;
> +		memdev->address = 0;
> +		memdev->interleave_index = 0;
> +		memdev->interleave_ways = 1;
> +
> +	}
> +
>  	acpi_desc = &t->acpi_desc;
>  	set_bit(ND_CMD_GET_CONFIG_SIZE, &acpi_desc->dimm_dsm_force_en);
>  	set_bit(ND_CMD_GET_CONFIG_DATA, &acpi_desc->dimm_dsm_force_en);
> @@ -1114,6 +1242,18 @@ static int nfit_test_probe(struct platform_device *pdev)
>  		return rc;
>  	}
>  
> +	if (nfit_test->setup != nfit_test0_setup)
> +		return 0;
> +
> +	nfit_test->setup_hotplug = 1;
> +	nfit_test->setup(nfit_test);
> +
> +	rc = acpi_nfit_merge(acpi_desc, nfit_test->nfit_size);
> +	if (rc) {
> +		nvdimm_bus_unregister(acpi_desc->nvdimm_bus);
> +		return rc;
> +	}
> +
>  	return 0;
>  }

Re: [PATCH 1/3] nfit: in acpi_nfit_init, break on a 0-length table

[Verma, Vishal L]

On Fri, 2015-10-09 at 10:27 -0700, Dan Williams wrote:
> On Fri, Oct 9, 2015 at 10:23 AM, Jeff Moyer <jmoyer@redhat.com>
> wrote:
> > Vishal Verma <vishal.l.verma@intel.com> writes:
> > 
> > > If acpi_nfit_init is called (such as from nfit_test), with an
> > > nfit table
> > > that has more memory allocated than it needs (and a similarly
> > > large
> > > 'size' field, add_tables would happily keep adding null SPA Range
> > > tables
> > > filling up all available memory.
> > > 
> > > Make it friendlier by breaking out if a 0-length header is found
> > > in any
> > > of the tables.
> > 
> > Shouldn't that at least spew a warning?  Or does the spec allow for
> > zero-length tables?
> > 
> 
> The spec allows for zero length tables but the firmware
> implementation
> should be self consistent and not report a total NFIT size that is
> greater than the sum of the size of the sub-structures.  A warning /
> nudge to firmware developers to fix their stuff seems appropriate.

Agreed, I'll add a warning. The caveat is, the following (3/3) patch to
nfit test will trigger the warning for the first, non-hotplug pass, as
I just calculate the size and allocate the buffer once, and the first
pass will have the regular nfit, and the second pass will have new
hotplug entries.. Is that OK or should I look at reworking that?

Re: [PATCH 2/3] acpi: add a utility function for evaluating _FIT

[Verma, Vishal L]

On Fri, 2015-10-09 at 13:28 -0400, Jeff Moyer wrote:
> Vishal Verma <vishal.l.verma@intel.com> writes:
> 
> >  /**
> > + * acpi_evaluate_fit: Evaluate _FIT method to get an updated NFIT
> > + * @handle: ACPI device handle
> > + * @buf: buffer for the updated NFIT
> > + *
> > + * Evaluate device's _FIT method if present to get an updated NFIT
> > + */
> > +acpi_status acpi_evaluate_fit(acpi_handle handle, struct
> > acpi_buffer **buf)
> > +{
> > +	acpi_status status;
> > +	struct acpi_buffer buffer = { ACPI_ALLOCATE_BUFFER, NULL
> > };
> > +
> > +	status = acpi_evaluate_object(handle, "_FIT", NULL,
> > &buffer);
> > +
> > +	if (ACPI_FAILURE(status))
> > +		return status;
> > +
> > +	*buf = &buffer;
> 
> Umm, unless I'm missing something, you're returning a stack address.

Good point, you're right. Dan/Rafael, is it OK to just remove this
patch entirely, and call acpi_evaluate_object directly from nfit.c? The
current way did feel a bit kludgey to me any way because I was
allocating a buffer here (above), but trying to free it in the caller,
which seems very ugly..

Re: [PATCH 2/3] acpi: add a utility function for evaluating _FIT

[Dan Williams]

On Fri, Oct 9, 2015 at 10:54 AM, Verma, Vishal L
<vishal.l.verma@intel.com> wrote:
> On Fri, 2015-10-09 at 13:28 -0400, Jeff Moyer wrote:
>> Vishal Verma <vishal.l.verma@intel.com> writes:
>>
>> >  /**
>> > + * acpi_evaluate_fit: Evaluate _FIT method to get an updated NFIT
>> > + * @handle: ACPI device handle
>> > + * @buf: buffer for the updated NFIT
>> > + *
>> > + * Evaluate device's _FIT method if present to get an updated NFIT
>> > + */
>> > +acpi_status acpi_evaluate_fit(acpi_handle handle, struct
>> > acpi_buffer **buf)
>> > +{
>> > +   acpi_status status;
>> > +   struct acpi_buffer buffer = { ACPI_ALLOCATE_BUFFER, NULL
>> > };
>> > +
>> > +   status = acpi_evaluate_object(handle, "_FIT", NULL,
>> > &buffer);
>> > +
>> > +   if (ACPI_FAILURE(status))
>> > +           return status;
>> > +
>> > +   *buf = &buffer;
>>
>> Umm, unless I'm missing something, you're returning a stack address.
>
> Good point, you're right. Dan/Rafael, is it OK to just remove this
> patch entirely, and call acpi_evaluate_object directly from nfit.c? The
> current way did feel a bit kludgey to me any way because I was
> allocating a buffer here (above), but trying to free it in the caller,
> which seems very ugly..

Open coding a call to acpi_evaluate_object() sounds good to me.

Re: [PATCH 3/3] acpi: nfit: Add support for hotplug

[Verma, Vishal L]

On Fri, 2015-10-09 at 13:33 -0400, Jeff Moyer wrote:
> Vishal Verma <vishal.l.verma@intel.com> writes:
> 
> > Add a .notify callback to the acpi_nfit_driver that gets called on
> > a
> > hotplug event. From this, evaluate the _FIT ACPI method which
> > returns
> > the updated NFIT with handles for the hot-plugged NVDIMM.
> > 
> > Iterate over the new NFIT, and add any new tables found, and
> > register/enable the corresponding regions.
> > 
> > In the nfit test framework, after normal initialization, update the
> > NFIT
> > with a new hot-plugged NVDIMM, and directly call into the driver to
> > update its view of the available regions.
> 
> OK, so just plugging in new NVDIMMs is supported, right?  Are there
> plans to support unplugging DIMMs?

Right, this is hot-add only until someone has a hot-remove use case.

> 
> -Jeff
> 
> > 
> > Cc: Dan Williams <dan.j.williams@intel.com>
> > Cc: Rafael J. Wysocki <rafael.j.wysocki@intel.com>
> > Cc: <linux-acpi@vger.kernel.org>
> > Cc: <linux-nvdimm@lists.01.org>
> > Signed-off-by: Vishal Verma <vishal.l.verma@intel.com>
> > ---
> >  drivers/acpi/nfit.c              | 136
> > ++++++++++++++++++++++++++++++++----
> >  drivers/acpi/nfit.h              |   2 +
> >  tools/testing/nvdimm/test/nfit.c | 144
> > ++++++++++++++++++++++++++++++++++++++-
> >  3 files changed, 267 insertions(+), 15 deletions(-)
> > 
> > diff --git a/drivers/acpi/nfit.c b/drivers/acpi/nfit.c
> > index ed599d1..2c24466 100644
> > --- a/drivers/acpi/nfit.c
> > +++ b/drivers/acpi/nfit.c
> > @@ -224,9 +224,13 @@ static bool add_spa(struct acpi_nfit_desc
> > *acpi_desc,
> >  		struct acpi_nfit_system_address *spa)
> >  {
> >  	struct device *dev = acpi_desc->dev;
> > -	struct nfit_spa *nfit_spa = devm_kzalloc(dev,
> > sizeof(*nfit_spa),
> > -			GFP_KERNEL);
> > +	struct nfit_spa *nfit_spa;
> > +
> > +	list_for_each_entry(nfit_spa, &acpi_desc->spas, list)
> > +		if (memcmp(nfit_spa->spa, spa, sizeof(*spa)) == 0)
> > +			return true;
> >  
> > +	nfit_spa = devm_kzalloc(dev, sizeof(*nfit_spa),
> > GFP_KERNEL);
> >  	if (!nfit_spa)
> >  		return false;
> >  	INIT_LIST_HEAD(&nfit_spa->list);
> > @@ -242,9 +246,13 @@ static bool add_memdev(struct acpi_nfit_desc
> > *acpi_desc,
> >  		struct acpi_nfit_memory_map *memdev)
> >  {
> >  	struct device *dev = acpi_desc->dev;
> > -	struct nfit_memdev *nfit_memdev = devm_kzalloc(dev,
> > -			sizeof(*nfit_memdev), GFP_KERNEL);
> > +	struct nfit_memdev *nfit_memdev;
> > +
> > +	list_for_each_entry(nfit_memdev, &acpi_desc->memdevs,
> > list)
> > +		if (memcmp(nfit_memdev->memdev, memdev,
> > sizeof(*memdev)) == 0)
> > +			return true;
> >  
> > +	nfit_memdev = devm_kzalloc(dev, sizeof(*nfit_memdev),
> > GFP_KERNEL);
> >  	if (!nfit_memdev)
> >  		return false;
> >  	INIT_LIST_HEAD(&nfit_memdev->list);
> > @@ -260,9 +268,13 @@ static bool add_dcr(struct acpi_nfit_desc
> > *acpi_desc,
> >  		struct acpi_nfit_control_region *dcr)
> >  {
> >  	struct device *dev = acpi_desc->dev;
> > -	struct nfit_dcr *nfit_dcr = devm_kzalloc(dev,
> > sizeof(*nfit_dcr),
> > -			GFP_KERNEL);
> > +	struct nfit_dcr *nfit_dcr;
> >  
> > +	list_for_each_entry(nfit_dcr, &acpi_desc->dcrs, list)
> > +		if (memcmp(nfit_dcr->dcr, dcr, sizeof(*dcr)) == 0)
> > +			return true;
> > +
> > +	nfit_dcr = devm_kzalloc(dev, sizeof(*nfit_dcr),
> > GFP_KERNEL);
> >  	if (!nfit_dcr)
> >  		return false;
> >  	INIT_LIST_HEAD(&nfit_dcr->list);
> > @@ -277,9 +289,13 @@ static bool add_bdw(struct acpi_nfit_desc
> > *acpi_desc,
> >  		struct acpi_nfit_data_region *bdw)
> >  {
> >  	struct device *dev = acpi_desc->dev;
> > -	struct nfit_bdw *nfit_bdw = devm_kzalloc(dev,
> > sizeof(*nfit_bdw),
> > -			GFP_KERNEL);
> > +	struct nfit_bdw *nfit_bdw;
> >  
> > +	list_for_each_entry(nfit_bdw, &acpi_desc->bdws, list)
> > +		if (memcmp(nfit_bdw->bdw, bdw, sizeof(*bdw)) == 0)
> > +			return true;
> > +
> > +	nfit_bdw = devm_kzalloc(dev, sizeof(*nfit_bdw),
> > GFP_KERNEL);
> >  	if (!nfit_bdw)
> >  		return false;
> >  	INIT_LIST_HEAD(&nfit_bdw->list);
> > @@ -294,9 +310,13 @@ static bool add_idt(struct acpi_nfit_desc
> > *acpi_desc,
> >  		struct acpi_nfit_interleave *idt)
> >  {
> >  	struct device *dev = acpi_desc->dev;
> > -	struct nfit_idt *nfit_idt = devm_kzalloc(dev,
> > sizeof(*nfit_idt),
> > -			GFP_KERNEL);
> > +	struct nfit_idt *nfit_idt;
> >  
> > +	list_for_each_entry(nfit_idt, &acpi_desc->idts, list)
> > +		if (memcmp(nfit_idt->idt, idt, sizeof(*idt)) == 0)
> > +			return true;
> > +
> > +	nfit_idt = devm_kzalloc(dev, sizeof(*nfit_idt),
> > GFP_KERNEL);
> >  	if (!nfit_idt)
> >  		return false;
> >  	INIT_LIST_HEAD(&nfit_idt->list);
> > @@ -311,9 +331,13 @@ static bool add_flush(struct acpi_nfit_desc
> > *acpi_desc,
> >  		struct acpi_nfit_flush_address *flush)
> >  {
> >  	struct device *dev = acpi_desc->dev;
> > -	struct nfit_flush *nfit_flush = devm_kzalloc(dev,
> > sizeof(*nfit_flush),
> > -			GFP_KERNEL);
> > +	struct nfit_flush *nfit_flush;
> >  
> > +	list_for_each_entry(nfit_flush, &acpi_desc->flushes, list)
> > +		if (memcmp(nfit_flush->flush, flush,
> > sizeof(*flush)) == 0)
> > +			return true;
> > +
> > +	nfit_flush = devm_kzalloc(dev, sizeof(*nfit_flush),
> > GFP_KERNEL);
> >  	if (!nfit_flush)
> >  		return false;
> >  	INIT_LIST_HEAD(&nfit_flush->list);
> > @@ -811,6 +835,8 @@ static int acpi_nfit_register_dimms(struct
> > acpi_nfit_desc *acpi_desc)
> >  			 */
> >  			dev_err(acpi_desc->dev, "duplicate DCR
> > detected: %s\n",
> >  					nvdimm_name(nvdimm));
> > +			/* TODO Do we need the warning? */
> > +			dimm_count++;
> >  			continue;
> >  		}
> >  
> > @@ -1532,6 +1558,8 @@ static int acpi_nfit_register_region(struct
> > acpi_nfit_desc *acpi_desc,
> >  		if (!nvdimm_volatile_region_create(nvdimm_bus,
> > ndr_desc))
> >  			return -ENOMEM;
> >  	}
> > +
> > +	acpi_desc->last_init_spa = nfit_spa;
> >  	return 0;
> >  }
> >  
> > @@ -1539,7 +1567,15 @@ static int acpi_nfit_register_regions(struct
> > acpi_nfit_desc *acpi_desc)
> >  {
> >  	struct nfit_spa *nfit_spa;
> >  
> > -	list_for_each_entry(nfit_spa, &acpi_desc->spas, list) {
> > +	if (acpi_desc->last_init_spa) {
> > +		nfit_spa = acpi_desc->last_init_spa;
> > +		nfit_spa = list_next_entry(nfit_spa, list);
> > +
> > +	} else
> > +		nfit_spa = list_first_entry(&acpi_desc->spas,
> > +				typeof(*nfit_spa), list);
> > +
> > +	list_for_each_entry_from(nfit_spa, &acpi_desc->spas, list)
> > {
> >  		int rc = acpi_nfit_register_region(acpi_desc,
> > nfit_spa);
> >  
> >  		if (rc)
> > @@ -1590,6 +1626,52 @@ int acpi_nfit_init(struct acpi_nfit_desc
> > *acpi_desc, acpi_size sz)
> >  }
> >  EXPORT_SYMBOL_GPL(acpi_nfit_init);
> >  
> > +int acpi_nfit_merge(struct acpi_nfit_desc *acpi_desc, acpi_size
> > sz)
> > +{
> > +	struct device *dev = acpi_desc->dev;
> > +	const void *end;
> > +	u8 *data;
> > +	int rc;
> > +
> > +	/*
> > +	 * TODO Can we get here with an uninitialized acpi_desc?
> > +	 * In the case where the only nvdimm in the system is
> > hotplugged?
> > +	 */
> > +	if (!acpi_desc) {
> > +		dev_err(dev, "%s: acpi_desc uninitialized\n",
> > __func__);
> > +		return -ENXIO;
> > +	}
> > +
> > +	/*
> > +	 * At this point, acpi_desc->nfit will have the updated
> > nfit table,
> > +	 * but the various lists in acpi_dev correspond to the old
> > table.
> > +	 */
> > +	data = (u8 *) acpi_desc->nfit;
> > +	end = data + sz;
> > +	data += sizeof(struct acpi_table_nfit);
> > +	while (!IS_ERR_OR_NULL(data))
> > +		data = add_table(acpi_desc, data, end);
> > +
> > +	if (IS_ERR(data)) {
> > +		dev_dbg(dev, "%s: nfit table parsing error:
> > %ld\n", __func__,
> > +				PTR_ERR(data));
> > +		return PTR_ERR(data);
> > +	}
> > +
> > +	if (nfit_mem_init(acpi_desc) != 0)
> > +		return -ENOMEM;
> > +
> > +	acpi_nfit_init_dsms(acpi_desc);
> > +
> > +	rc = acpi_nfit_register_dimms(acpi_desc);
> > +	if (rc)
> > +		return rc;
> > +
> > +	rc = acpi_nfit_register_regions(acpi_desc);
> > +	return rc;
> > +}
> > +EXPORT_SYMBOL_GPL(acpi_nfit_merge);
> > +
> >  static int acpi_nfit_add(struct acpi_device *adev)
> >  {
> >  	struct nvdimm_bus_descriptor *nd_desc;
> > @@ -1639,6 +1721,33 @@ static int acpi_nfit_remove(struct
> > acpi_device *adev)
> >  	return 0;
> >  }
> >  
> > +static void acpi_nfit_notify(struct acpi_device *adev, u32 event)
> > +{
> > +	struct acpi_nfit_desc *acpi_desc = dev_get_drvdata(&adev
> > ->dev);
> > +	struct acpi_table_nfit *nfit_saved;
> > +	struct device *dev = &adev->dev;
> > +	struct acpi_buffer *buf;
> > +	acpi_status status;
> > +	int ret;
> > +
> > +	dev_dbg(dev, "%s: event: %d\n", __func__, event);
> > +
> > +	/* Evaluate _FIT */
> > +	status = acpi_evaluate_fit(adev->handle, &buf);
> > +	if (ACPI_FAILURE(status))
> > +		dev_err(dev, "failed to evaluate _FIT\n");
> > +
> > +	nfit_saved = acpi_desc->nfit;
> > +	acpi_desc->nfit = (struct acpi_table_nfit *)buf;
> > +	ret = acpi_nfit_merge(acpi_desc, buf->length);
> > +	if (!ret) {
> > +		/* Merge failed, restore old nfit buf, and exit */
> > +		acpi_desc->nfit = nfit_saved;
> > +		dev_err(dev, "failed to merge updated NFIT\n");
> > +	}
> > +	kfree(buf->pointer);
> > +}
> > +
> >  static const struct acpi_device_id acpi_nfit_ids[] = {
> >  	{ "ACPI0012", 0 },
> >  	{ "", 0 },
> > @@ -1651,6 +1760,7 @@ static struct acpi_driver acpi_nfit_driver =
> > {
> >  	.ops = {
> >  		.add = acpi_nfit_add,
> >  		.remove = acpi_nfit_remove,
> > +		.notify = acpi_nfit_notify,
> >  	},
> >  };
> >  
> > diff --git a/drivers/acpi/nfit.h b/drivers/acpi/nfit.h
> > index 7e74015..9f4758f 100644
> > --- a/drivers/acpi/nfit.h
> > +++ b/drivers/acpi/nfit.h
> > @@ -105,6 +105,7 @@ struct acpi_nfit_desc {
> >  	struct list_head dcrs;
> >  	struct list_head bdws;
> >  	struct list_head idts;
> > +	struct nfit_spa *last_init_spa;
> >  	struct nvdimm_bus *nvdimm_bus;
> >  	struct device *dev;
> >  	unsigned long dimm_dsm_force_en;
> > @@ -179,5 +180,6 @@ static inline struct acpi_nfit_desc
> > *to_acpi_desc(
> >  
> >  const u8 *to_nfit_uuid(enum nfit_uuids id);
> >  int acpi_nfit_init(struct acpi_nfit_desc *nfit, acpi_size sz);
> > +int acpi_nfit_merge(struct acpi_nfit_desc *acpi_desc, acpi_size
> > sz);
> >  extern const struct attribute_group *acpi_nfit_attribute_groups[];
> >  #endif /* __NFIT_H__ */
> > diff --git a/tools/testing/nvdimm/test/nfit.c
> > b/tools/testing/nvdimm/test/nfit.c
> > index 021e6f9..3a7b691 100644
> > --- a/tools/testing/nvdimm/test/nfit.c
> > +++ b/tools/testing/nvdimm/test/nfit.c
> > @@ -44,6 +44,15 @@
> >   * +------+  |                 blk5.0             |  pm1.0  |    3
> >       region5
> >   *           +-------------------------+----------+-+-------+
> >   *
> > + * +--+---+
> > + * | cpu1 |
> > + * +--+---+                   (Hotplug DIMM)
> > + *    |      +----------------------------------------------+
> > + * +--+---+  |                 blk6.0/pm7.0                 |    4
> >       region6
> > + * | imc0 +--+----------------------------------------------+
> > + * +------+
> > + *
> > + *
> >   * *) In this layout we have four dimms and two memory controllers
> > in one
> >   *    socket.  Each unique interface (BLK or PMEM) to DPA space
> >   *    is identified by a region device with a dynamically assigned
> > id.
> > @@ -85,8 +94,8 @@
> >   *    reference an NVDIMM.
> >   */
> >  enum {
> > -	NUM_PM  = 2,
> > -	NUM_DCR = 4,
> > +	NUM_PM  = 3,
> > +	NUM_DCR = 5,
> >  	NUM_BDW = NUM_DCR,
> >  	NUM_SPA = NUM_PM + NUM_DCR + NUM_BDW,
> >  	NUM_MEM = NUM_DCR + NUM_BDW + 2 /* spa0 iset */ + 4 /*
> > spa1 iset */,
> > @@ -115,6 +124,7 @@ static u32 handle[NUM_DCR] = {
> >  	[1] = NFIT_DIMM_HANDLE(0, 0, 0, 0, 1),
> >  	[2] = NFIT_DIMM_HANDLE(0, 0, 1, 0, 0),
> >  	[3] = NFIT_DIMM_HANDLE(0, 0, 1, 0, 1),
> > +	[4] = NFIT_DIMM_HANDLE(0, 1, 0, 0, 0),
> >  };
> >  
> >  struct nfit_test {
> > @@ -138,6 +148,7 @@ struct nfit_test {
> >  	dma_addr_t *dcr_dma;
> >  	int (*alloc)(struct nfit_test *t);
> >  	void (*setup)(struct nfit_test *t);
> > +	int setup_hotplug;
> >  };
> >  
> >  static struct nfit_test *to_nfit_test(struct device *dev)
> > @@ -428,6 +439,10 @@ static int nfit_test0_alloc(struct nfit_test
> > *t)
> >  	if (!t->spa_set[1])
> >  		return -ENOMEM;
> >  
> > +	t->spa_set[2] = test_alloc_coherent(t, SPA0_SIZE, &t
> > ->spa_set_dma[2]);
> > +	if (!t->spa_set[2])
> > +		return -ENOMEM;
> > +
> >  	for (i = 0; i < NUM_DCR; i++) {
> >  		t->dimm[i] = test_alloc(t, DIMM_SIZE, &t
> > ->dimm_dma[i]);
> >  		if (!t->dimm[i])
> > @@ -950,6 +965,119 @@ static void nfit_test0_setup(struct nfit_test
> > *t)
> >  	flush->hint_count = 1;
> >  	flush->hint_address[0] = t->flush_dma[3];
> >  
> > +
> > +	if (t->setup_hotplug) {
> > +		offset = offset + sizeof(struct
> > acpi_nfit_flush_address) * 4;
> > +		/* dcr-descriptor4 */
> > +		dcr = nfit_buf + offset;
> > +		dcr->header.type = ACPI_NFIT_TYPE_CONTROL_REGION;
> > +		dcr->header.length = sizeof(struct
> > acpi_nfit_control_region);
> > +		dcr->region_index = 4+1;
> > +		dcr->vendor_id = 0xabcd;
> > +		dcr->device_id = 0;
> > +		dcr->revision_id = 1;
> > +		dcr->serial_number = ~handle[4];
> > +		dcr->windows = 0;
> > +		dcr->window_size = 0;
> > +		dcr->command_offset = 0;
> > +		dcr->command_size = 0;
> > +		dcr->status_offset = 0;
> > +		dcr->status_size = 0;
> > +
> > +		offset = offset + sizeof(struct
> > acpi_nfit_control_region);
> > +		/* bdw4 (spa/dcr4, dimm4) */
> > +		bdw = nfit_buf + offset;
> > +		bdw->header.type = ACPI_NFIT_TYPE_DATA_REGION;
> > +		bdw->header.length = sizeof(struct
> > acpi_nfit_data_region);
> > +		bdw->region_index = 4+1;
> > +		bdw->windows = 1;
> > +		bdw->offset = 0;
> > +		bdw->size = BDW_SIZE;
> > +		bdw->capacity = DIMM_SIZE;
> > +		bdw->start_address = 0;
> > +
> > +		offset = offset + sizeof(struct
> > acpi_nfit_data_region);
> > +		/* spa10 (dcr4) dimm4 */
> > +		spa = nfit_buf + offset;
> > +		spa->header.type = ACPI_NFIT_TYPE_SYSTEM_ADDRESS;
> > +		spa->header.length = sizeof(*spa);
> > +		memcpy(spa->range_guid,
> > to_nfit_uuid(NFIT_SPA_DCR), 16);
> > +		spa->range_index = 10+1;
> > +		spa->address = t->dcr_dma[4];
> > +		spa->length = DCR_SIZE;
> > +
> > +		/*
> > +		 * spa11 (single-dimm interleave for hotplug, note
> > storage
> > +		 * does not actually alias the related block-data
> > -window
> > +		 * regions)
> > +		 */
> > +		spa = nfit_buf + offset + sizeof(*spa);
> > +		spa->header.type = ACPI_NFIT_TYPE_SYSTEM_ADDRESS;
> > +		spa->header.length = sizeof(*spa);
> > +		memcpy(spa->range_guid, to_nfit_uuid(NFIT_SPA_PM),
> > 16);
> > +		spa->range_index = 11+1;
> > +		spa->address = t->spa_set_dma[2];
> > +		spa->length = SPA0_SIZE;
> > +
> > +		/* spa12 (bdw for dcr4) dimm4 */
> > +		spa = nfit_buf + offset + sizeof(*spa) * 2;
> > +		spa->header.type = ACPI_NFIT_TYPE_SYSTEM_ADDRESS;
> > +		spa->header.length = sizeof(*spa);
> > +		memcpy(spa->range_guid,
> > to_nfit_uuid(NFIT_SPA_BDW), 16);
> > +		spa->range_index = 12+1;
> > +		spa->address = t->dimm_dma[4];
> > +		spa->length = DIMM_SIZE;
> > +
> > +		offset = offset + sizeof(*spa) * 3;
> > +		/* mem-region14 (spa/dcr4, dimm4) */
> > +		memdev = nfit_buf + offset;
> > +		memdev->header.type = ACPI_NFIT_TYPE_MEMORY_MAP;
> > +		memdev->header.length = sizeof(*memdev);
> > +		memdev->device_handle = handle[4];
> > +		memdev->physical_id = 4;
> > +		memdev->region_id = 0;
> > +		memdev->range_index = 10+1;
> > +		memdev->region_index = 4+1;
> > +		memdev->region_size = 0;
> > +		memdev->region_offset = 0;
> > +		memdev->address = 0;
> > +		memdev->interleave_index = 0;
> > +		memdev->interleave_ways = 1;
> > +
> > +		/* mem-region15 (spa0, dimm4) */
> > +		memdev = nfit_buf + offset +
> > +				sizeof(struct
> > acpi_nfit_memory_map);
> > +		memdev->header.type = ACPI_NFIT_TYPE_MEMORY_MAP;
> > +		memdev->header.length = sizeof(*memdev);
> > +		memdev->device_handle = handle[4];
> > +		memdev->physical_id = 4;
> > +		memdev->region_id = 0;
> > +		memdev->range_index = 11+1;
> > +		memdev->region_index = 4+1;
> > +		memdev->region_size = SPA0_SIZE;
> > +		memdev->region_offset = t->spa_set_dma[2];
> > +		memdev->address = 0;
> > +		memdev->interleave_index = 0;
> > +		memdev->interleave_ways = 1;
> > +
> > +		/* mem-region16 (spa/dcr4, dimm4) */
> > +		memdev = nfit_buf + offset +
> > +				sizeof(struct
> > acpi_nfit_memory_map) * 2;
> > +		memdev->header.type = ACPI_NFIT_TYPE_MEMORY_MAP;
> > +		memdev->header.length = sizeof(*memdev);
> > +		memdev->device_handle = handle[4];
> > +		memdev->physical_id = 4;
> > +		memdev->region_id = 0;
> > +		memdev->range_index = 12+1;
> > +		memdev->region_index = 4+1;
> > +		memdev->region_size = 0;
> > +		memdev->region_offset = 0;
> > +		memdev->address = 0;
> > +		memdev->interleave_index = 0;
> > +		memdev->interleave_ways = 1;
> > +
> > +	}
> > +
> >  	acpi_desc = &t->acpi_desc;
> >  	set_bit(ND_CMD_GET_CONFIG_SIZE, &acpi_desc
> > ->dimm_dsm_force_en);
> >  	set_bit(ND_CMD_GET_CONFIG_DATA, &acpi_desc
> > ->dimm_dsm_force_en);
> > @@ -1114,6 +1242,18 @@ static int nfit_test_probe(struct
> > platform_device *pdev)
> >  		return rc;
> >  	}
> >  
> > +	if (nfit_test->setup != nfit_test0_setup)
> > +		return 0;
> > +
> > +	nfit_test->setup_hotplug = 1;
> > +	nfit_test->setup(nfit_test);
> > +
> > +	rc = acpi_nfit_merge(acpi_desc, nfit_test->nfit_size);
> > +	if (rc) {
> > +		nvdimm_bus_unregister(acpi_desc->nvdimm_bus);
> > +		return rc;
> > +	}
> > +
> >  	return 0;
> >  }

Re: [PATCH 3/3] acpi: nfit: Add support for hotplug

[Dan Williams]

On Fri, Oct 9, 2015 at 11:08 AM, Verma, Vishal L
<vishal.l.verma@intel.com> wrote:
> On Fri, 2015-10-09 at 13:33 -0400, Jeff Moyer wrote:
>> Vishal Verma <vishal.l.verma@intel.com> writes:
>>
>> > Add a .notify callback to the acpi_nfit_driver that gets called on
>> > a
>> > hotplug event. From this, evaluate the _FIT ACPI method which
>> > returns
>> > the updated NFIT with handles for the hot-plugged NVDIMM.
>> >
>> > Iterate over the new NFIT, and add any new tables found, and
>> > register/enable the corresponding regions.
>> >
>> > In the nfit test framework, after normal initialization, update the
>> > NFIT
>> > with a new hot-plugged NVDIMM, and directly call into the driver to
>> > update its view of the available regions.
>>
>> OK, so just plugging in new NVDIMMs is supported, right?  Are there
>> plans to support unplugging DIMMs?
>
> Right, this is hot-add only until someone has a hot-remove use case.
>

...someone meaning ACPI, since it only defines a hot-add flow.  We
could prototype something for the virtualization provisioning use
case, but it's not part of the current spec.

Re: [PATCH 3/3] acpi: nfit: Add support for hotplug

[Dan Williams]

[ adding Robert and Toshi ]

On Wed, Oct 7, 2015 at 2:49 PM, Vishal Verma <vishal.l.verma@intel.com> wrote:
> Add a .notify callback to the acpi_nfit_driver that gets called on a
> hotplug event. From this, evaluate the _FIT ACPI method which returns
> the updated NFIT with handles for the hot-plugged NVDIMM.
>
> Iterate over the new NFIT, and add any new tables found, and
> register/enable the corresponding regions.
>
> In the nfit test framework, after normal initialization, update the NFIT
> with a new hot-plugged NVDIMM, and directly call into the driver to
> update its view of the available regions.
>
> Cc: Dan Williams <dan.j.williams@intel.com>
> Cc: Rafael J. Wysocki <rafael.j.wysocki@intel.com>
> Cc: <linux-acpi@vger.kernel.org>
> Cc: <linux-nvdimm@lists.01.org>
> Signed-off-by: Vishal Verma <vishal.l.verma@intel.com>
> ---
>  drivers/acpi/nfit.c              | 136 ++++++++++++++++++++++++++++++++----
>  drivers/acpi/nfit.h              |   2 +
>  tools/testing/nvdimm/test/nfit.c | 144 ++++++++++++++++++++++++++++++++++++++-
>  3 files changed, 267 insertions(+), 15 deletions(-)
>
> diff --git a/drivers/acpi/nfit.c b/drivers/acpi/nfit.c
> index ed599d1..2c24466 100644
> --- a/drivers/acpi/nfit.c
> +++ b/drivers/acpi/nfit.c
> @@ -224,9 +224,13 @@ static bool add_spa(struct acpi_nfit_desc *acpi_desc,
>                 struct acpi_nfit_system_address *spa)
>  {
>         struct device *dev = acpi_desc->dev;
> -       struct nfit_spa *nfit_spa = devm_kzalloc(dev, sizeof(*nfit_spa),
> -                       GFP_KERNEL);
> +       struct nfit_spa *nfit_spa;
> +
> +       list_for_each_entry(nfit_spa, &acpi_desc->spas, list)
> +               if (memcmp(nfit_spa->spa, spa, sizeof(*spa)) == 0)
> +                       return true;
>
> +       nfit_spa = devm_kzalloc(dev, sizeof(*nfit_spa), GFP_KERNEL);
>         if (!nfit_spa)
>                 return false;
>         INIT_LIST_HEAD(&nfit_spa->list);
> @@ -242,9 +246,13 @@ static bool add_memdev(struct acpi_nfit_desc *acpi_desc,
>                 struct acpi_nfit_memory_map *memdev)
>  {
>         struct device *dev = acpi_desc->dev;
> -       struct nfit_memdev *nfit_memdev = devm_kzalloc(dev,
> -                       sizeof(*nfit_memdev), GFP_KERNEL);
> +       struct nfit_memdev *nfit_memdev;
> +
> +       list_for_each_entry(nfit_memdev, &acpi_desc->memdevs, list)
> +               if (memcmp(nfit_memdev->memdev, memdev, sizeof(*memdev)) == 0)
> +                       return true;

I'm wondering if we need to be cognizant of flag changes here.
Robert, Toshi are you expecting that the flags of an existing memory
device entry will be updated by the this notification mechanism?

>
> +       nfit_memdev = devm_kzalloc(dev, sizeof(*nfit_memdev), GFP_KERNEL);
>         if (!nfit_memdev)
>                 return false;
>         INIT_LIST_HEAD(&nfit_memdev->list);
> @@ -260,9 +268,13 @@ static bool add_dcr(struct acpi_nfit_desc *acpi_desc,
>                 struct acpi_nfit_control_region *dcr)
>  {
>         struct device *dev = acpi_desc->dev;
> -       struct nfit_dcr *nfit_dcr = devm_kzalloc(dev, sizeof(*nfit_dcr),
> -                       GFP_KERNEL);
> +       struct nfit_dcr *nfit_dcr;
>
> +       list_for_each_entry(nfit_dcr, &acpi_desc->dcrs, list)
> +               if (memcmp(nfit_dcr->dcr, dcr, sizeof(*dcr)) == 0)
> +                       return true;
>
> +
> +       nfit_dcr = devm_kzalloc(dev, sizeof(*nfit_dcr), GFP_KERNEL);
>         if (!nfit_dcr)
>                 return false;
>         INIT_LIST_HEAD(&nfit_dcr->list);
> @@ -277,9 +289,13 @@ static bool add_bdw(struct acpi_nfit_desc *acpi_desc,
>                 struct acpi_nfit_data_region *bdw)
>  {
>         struct device *dev = acpi_desc->dev;
> -       struct nfit_bdw *nfit_bdw = devm_kzalloc(dev, sizeof(*nfit_bdw),
> -                       GFP_KERNEL);
> +       struct nfit_bdw *nfit_bdw;
>
> +       list_for_each_entry(nfit_bdw, &acpi_desc->bdws, list)
> +               if (memcmp(nfit_bdw->bdw, bdw, sizeof(*bdw)) == 0)
> +                       return true;
> +
> +       nfit_bdw = devm_kzalloc(dev, sizeof(*nfit_bdw), GFP_KERNEL);
>         if (!nfit_bdw)
>                 return false;
>         INIT_LIST_HEAD(&nfit_bdw->list);
> @@ -294,9 +310,13 @@ static bool add_idt(struct acpi_nfit_desc *acpi_desc,
>                 struct acpi_nfit_interleave *idt)
>  {
>         struct device *dev = acpi_desc->dev;
> -       struct nfit_idt *nfit_idt = devm_kzalloc(dev, sizeof(*nfit_idt),
> -                       GFP_KERNEL);
> +       struct nfit_idt *nfit_idt;
>
> +       list_for_each_entry(nfit_idt, &acpi_desc->idts, list)
> +               if (memcmp(nfit_idt->idt, idt, sizeof(*idt)) == 0)
> +                       return true;
> +
> +       nfit_idt = devm_kzalloc(dev, sizeof(*nfit_idt), GFP_KERNEL);
>         if (!nfit_idt)
>                 return false;
>         INIT_LIST_HEAD(&nfit_idt->list);
> @@ -311,9 +331,13 @@ static bool add_flush(struct acpi_nfit_desc *acpi_desc,
>                 struct acpi_nfit_flush_address *flush)
>  {
>         struct device *dev = acpi_desc->dev;
> -       struct nfit_flush *nfit_flush = devm_kzalloc(dev, sizeof(*nfit_flush),
> -                       GFP_KERNEL);
> +       struct nfit_flush *nfit_flush;
>
> +       list_for_each_entry(nfit_flush, &acpi_desc->flushes, list)
> +               if (memcmp(nfit_flush->flush, flush, sizeof(*flush)) == 0)
> +                       return true;
> +
> +       nfit_flush = devm_kzalloc(dev, sizeof(*nfit_flush), GFP_KERNEL);
>         if (!nfit_flush)
>                 return false;
>         INIT_LIST_HEAD(&nfit_flush->list);
> @@ -811,6 +835,8 @@ static int acpi_nfit_register_dimms(struct acpi_nfit_desc *acpi_desc)
>                          */
>                         dev_err(acpi_desc->dev, "duplicate DCR detected: %s\n",
>                                         nvdimm_name(nvdimm));
> +                       /* TODO Do we need the warning? */
> +                       dimm_count++;

Robert, comments?

>                         continue;
>                 }
>
> @@ -1532,6 +1558,8 @@ static int acpi_nfit_register_region(struct acpi_nfit_desc *acpi_desc,
>                 if (!nvdimm_volatile_region_create(nvdimm_bus, ndr_desc))
>                         return -ENOMEM;
>         }
> +
> +       acpi_desc->last_init_spa = nfit_spa;
>         return 0;
>  }
>
> @@ -1539,7 +1567,15 @@ static int acpi_nfit_register_regions(struct acpi_nfit_desc *acpi_desc)
>  {
>         struct nfit_spa *nfit_spa;
>
> -       list_for_each_entry(nfit_spa, &acpi_desc->spas, list) {
> +       if (acpi_desc->last_init_spa) {
> +               nfit_spa = acpi_desc->last_init_spa;
> +               nfit_spa = list_next_entry(nfit_spa, list);

This pre-supposes no deletions... but I guess that's ok for now.

> +
> +       } else
> +               nfit_spa = list_first_entry(&acpi_desc->spas,
> +                               typeof(*nfit_spa), list);
> +
> +       list_for_each_entry_from(nfit_spa, &acpi_desc->spas, list) {
>                 int rc = acpi_nfit_register_region(acpi_desc, nfit_spa);
>
>                 if (rc)
> @@ -1590,6 +1626,52 @@ int acpi_nfit_init(struct acpi_nfit_desc *acpi_desc, acpi_size sz)
>  }
>  EXPORT_SYMBOL_GPL(acpi_nfit_init);
>
> +int acpi_nfit_merge(struct acpi_nfit_desc *acpi_desc, acpi_size sz)
> +{
> +       struct device *dev = acpi_desc->dev;
> +       const void *end;
> +       u8 *data;
> +       int rc;
> +
> +       /*
> +        * TODO Can we get here with an uninitialized acpi_desc?
> +        * In the case where the only nvdimm in the system is hotplugged?
> +        */
> +       if (!acpi_desc) {
> +               dev_err(dev, "%s: acpi_desc uninitialized\n", __func__);
> +               return -ENXIO;
> +       }

This raises an interesting question about when a notification event
can arrive relative to the driver finishing initialization.  It seems
acpi_bus_notify() has no exclusion against racing probe or remove.
See below, I think we need some locking to protect the new list
traversals and manipulations.

> +
> +       /*
> +        * At this point, acpi_desc->nfit will have the updated nfit table,
> +        * but the various lists in acpi_dev correspond to the old table.
> +        */
> +       data = (u8 *) acpi_desc->nfit;
> +       end = data + sz;
> +       data += sizeof(struct acpi_table_nfit);
> +       while (!IS_ERR_OR_NULL(data))
> +               data = add_table(acpi_desc, data, end);
> +
> +       if (IS_ERR(data)) {
> +               dev_dbg(dev, "%s: nfit table parsing error: %ld\n", __func__,
> +                               PTR_ERR(data));
> +               return PTR_ERR(data);
> +       }
> +
> +       if (nfit_mem_init(acpi_desc) != 0)
> +               return -ENOMEM;
> +
> +       acpi_nfit_init_dsms(acpi_desc);
> +
> +       rc = acpi_nfit_register_dimms(acpi_desc);
> +       if (rc)
> +               return rc;
> +
> +       rc = acpi_nfit_register_regions(acpi_desc);
> +       return rc;
> +}
> +EXPORT_SYMBOL_GPL(acpi_nfit_merge);
> +
>  static int acpi_nfit_add(struct acpi_device *adev)
>  {
>         struct nvdimm_bus_descriptor *nd_desc;
> @@ -1639,6 +1721,33 @@ static int acpi_nfit_remove(struct acpi_device *adev)
>         return 0;
>  }
>
> +static void acpi_nfit_notify(struct acpi_device *adev, u32 event)
> +{
> +       struct acpi_nfit_desc *acpi_desc = dev_get_drvdata(&adev->dev);
> +       struct acpi_table_nfit *nfit_saved;
> +       struct device *dev = &adev->dev;
> +       struct acpi_buffer *buf;
> +       acpi_status status;
> +       int ret;
> +
> +       dev_dbg(dev, "%s: event: %d\n", __func__, event);
> +
> +       /* Evaluate _FIT */
> +       status = acpi_evaluate_fit(adev->handle, &buf);
> +       if (ACPI_FAILURE(status))
> +               dev_err(dev, "failed to evaluate _FIT\n");
> +
> +       nfit_saved = acpi_desc->nfit;> +       if (!ret) {
> +               /* Merge failed, restore old nfit buf, and exit */
> +               acpi_desc->nfit = nfit_saved;
> +               dev_err(dev, "failed to merge updated NFIT\n");
> +       }
> +       kfree(buf->pointer);
> +}
> +
>  static const struct acpi_device_id acpi_nfit_ids[] = {
>         { "ACPI0012", 0 },
>         { "", 0 },
> @@ -1651,6 +1760,7 @@ static struct acpi_driver acpi_nfit_driver = {
>         .ops = {
>                 .add = acpi_nfit_add,
>                 .remove = acpi_nfit_remove,
> +               .notify = acpi_nfit_notify,
>         },
>  };
>
> diff --git a/drivers/acpi/nfit.h b/drivers/acpi/nfit.h
> index 7e74015..9f4758f 100644
> --- a/drivers/acpi/nfit.h
> +++ b/drivers/acpi/nfit.h
> @@ -105,6 +105,7 @@ struct acpi_nfit_desc {
>         struct list_head dcrs;
>         struct list_head bdws;
>         struct list_head idts;
> +       struct nfit_spa *last_init_spa;
>         struct nvdimm_bus *nvdimm_bus;
>         struct device *dev;
>         unsigned long dimm_dsm_force_en;
> @@ -179,5 +180,6 @@ static inline struct acpi_nfit_desc *to_acpi_desc(
>
>  const u8 *to_nfit_uuid(enum nfit_uuids id);
>  int acpi_nfit_init(struct acpi_nfit_desc *nfit, acpi_size sz);
> +int acpi_nfit_merge(struct acpi_nfit_desc *acpi_desc, acpi_size sz);
>  extern const struct attribute_group *acpi_nfit_attribute_groups[];
>  #endif /* __NFIT_H__ */
> diff --git a/tools/testing/nvdimm/test/nfit.c b/tools/testing/nvdimm/test/nfit.c
> index 021e6f9..3a7b691 100644
> --- a/tools/testing/nvdimm/test/nfit.c
> +++ b/tools/testing/nvdimm/test/nfit.c
> @@ -44,6 +44,15 @@
>   * +------+  |                 blk5.0             |  pm1.0  |    3      region5
>   *           +-------------------------+----------+-+-------+
>   *
> + * +--+---+
> + * | cpu1 |
> + * +--+---+                   (Hotplug DIMM)
> + *    |      +----------------------------------------------+
> + * +--+---+  |                 blk6.0/pm7.0                 |    4      region6
> + * | imc0 +--+----------------------------------------------+
> + * +------+
> + *
> + *
>   * *) In this layout we have four dimms and two memory controllers in one
>   *    socket.  Each unique interface (BLK or PMEM) to DPA space
>   *    is identified by a region device with a dynamically assigned id.
> @@ -85,8 +94,8 @@
>   *    reference an NVDIMM.
>   */
>  enum {
> -       NUM_PM  = 2,
> -       NUM_DCR = 4,
> +       NUM_PM  = 3,
> +       NUM_DCR = 5,
>         NUM_BDW = NUM_DCR,
>         NUM_SPA = NUM_PM + NUM_DCR + NUM_BDW,
>         NUM_MEM = NUM_DCR + NUM_BDW + 2 /* spa0 iset */ + 4 /* spa1 iset */,
> @@ -115,6 +124,7 @@ static u32 handle[NUM_DCR] = {
>         [1] = NFIT_DIMM_HANDLE(0, 0, 0, 0, 1),
>         [2] = NFIT_DIMM_HANDLE(0, 0, 1, 0, 0),
>         [3] = NFIT_DIMM_HANDLE(0, 0, 1, 0, 1),
> +       [4] = NFIT_DIMM_HANDLE(0, 1, 0, 0, 0),
>  };
>
>  struct nfit_test {
> @@ -138,6 +148,7 @@ struct nfit_test {
>         dma_addr_t *dcr_dma;
>         int (*alloc)(struct nfit_test *t);
>         void (*setup)(struct nfit_test *t);
> +       int setup_hotplug;
>  };
>
>  static struct nfit_test *to_nfit_test(struct device *dev)
> @@ -428,6 +439,10 @@ static int nfit_test0_alloc(struct nfit_test *t)
>         if (!t->spa_set[1])
>                 return -ENOMEM;
>
> +       t->spa_set[2] = test_alloc_coherent(t, SPA0_SIZE, &t->spa_set_dma[2]);
> +       if (!t->spa_set[2])
> +               return -ENOMEM;
> +
>         for (i = 0; i < NUM_DCR; i++) {
>                 t->dimm[i] = test_alloc(t, DIMM_SIZE, &t->dimm_dma[i]);
>                 if (!t->dimm[i])
> @@ -950,6 +965,119 @@ static void nfit_test0_setup(struct nfit_test *t)
>         flush->hint_count = 1;
>         flush->hint_address[0] = t->flush_dma[3];
>
> +
> +       if (t->setup_hotplug) {
> +               offset = offset + sizeof(struct acpi_nfit_flush_address) * 4;
> +               /* dcr-descriptor4 */
> +               dcr = nfit_buf + offset;
> +               dcr->header.type = ACPI_NFIT_TYPE_CONTROL_REGION;
> +               dcr->header.length = sizeof(struct acpi_nfit_control_region);
> +               dcr->region_index = 4+1;
> +               dcr->vendor_id = 0xabcd;
> +               dcr->device_id = 0;
> +               dcr->revision_id = 1;
> +               dcr->serial_number = ~handle[4];
> +               dcr->windows = 0;
> +               dcr->window_size = 0;
> +               dcr->command_offset = 0;
> +               dcr->command_size = 0;
> +               dcr->status_offset = 0;
> +               dcr->status_size = 0;
> +
> +               offset = offset + sizeof(struct acpi_nfit_control_region);
> +               /* bdw4 (spa/dcr4, dimm4) */
> +               bdw = nfit_buf + offset;
> +               bdw->header.type = ACPI_NFIT_TYPE_DATA_REGION;
> +               bdw->header.length = sizeof(struct acpi_nfit_data_region);
> +               bdw->region_index = 4+1;
> +               bdw->windows = 1;
> +               bdw->offset = 0;
> +               bdw->size = BDW_SIZE;
> +               bdw->capacity = DIMM_SIZE;
> +               bdw->start_address = 0;
> +
> +               offset = offset + sizeof(struct acpi_nfit_data_region);
> +               /* spa10 (dcr4) dimm4 */
> +               spa = nfit_buf + offset;
> +               spa->header.type = ACPI_NFIT_TYPE_SYSTEM_ADDRESS;
> +               spa->header.length = sizeof(*spa);
> +               memcpy(spa->range_guid, to_nfit_uuid(NFIT_SPA_DCR), 16);
> +               spa->range_index = 10+1;
> +               spa->address = t->dcr_dma[4];
> +               spa->length = DCR_SIZE;
> +
> +               /*
> +                * spa11 (single-dimm interleave for hotplug, note storage
> +                * does not actually alias the related block-data-window
> +                * regions)
> +                */
> +               spa = nfit_buf + offset + sizeof(*spa);
> +               spa->header.type = ACPI_NFIT_TYPE_SYSTEM_ADDRESS;
> +               spa->header.length = sizeof(*spa);
> +               memcpy(spa->range_guid, to_nfit_uuid(NFIT_SPA_PM), 16);
> +               spa->range_index = 11+1;
> +               spa->address = t->spa_set_dma[2];
> +               spa->length = SPA0_SIZE;
> +
> +               /* spa12 (bdw for dcr4) dimm4 */
> +               spa = nfit_buf + offset + sizeof(*spa) * 2;
> +               spa->header.type = ACPI_NFIT_TYPE_SYSTEM_ADDRESS;
> +               spa->header.length = sizeof(*spa);
> +               memcpy(spa->range_guid, to_nfit_uuid(NFIT_SPA_BDW), 16);
> +               spa->range_index = 12+1;
> +               spa->address = t->dimm_dma[4];
> +               spa->length = DIMM_SIZE;
> +
> +               offset = offset + sizeof(*spa) * 3;
> +               /* mem-region14 (spa/dcr4, dimm4) */
> +               memdev = nfit_buf + offset;
> +               memdev->header.type = ACPI_NFIT_TYPE_MEMORY_MAP;
> +               memdev->header.length = sizeof(*memdev);
> +               memdev->device_handle = handle[4];
> +               memdev->physical_id = 4;
> +               memdev->region_id = 0;
> +               memdev->range_index = 10+1;
> +               memdev->region_index = 4+1;
> +               memdev->region_size = 0;
> +               memdev->region_offset = 0;
> +               memdev->address = 0;
> +               memdev->interleave_index = 0;
> +               memdev->interleave_ways = 1;
> +
> +               /* mem-region15 (spa0, dimm4) */
> +               memdev = nfit_buf + offset +
> +                               sizeof(struct acpi_nfit_memory_map);
> +               memdev->header.type = ACPI_NFIT_TYPE_MEMORY_MAP;
> +               memdev->header.length = sizeof(*memdev);
> +               memdev->device_handle = handle[4];
> +               memdev->physical_id = 4;
> +               memdev->region_id = 0;
> +               memdev->range_index = 11+1;
> +               memdev->region_index = 4+1;
> +               memdev->region_size = SPA0_SIZE;
> +               memdev->region_offset = t->spa_set_dma[2];
> +               memdev->address = 0;
> +               memdev->interleave_index = 0;
> +               memdev->interleave_ways = 1;
> +
> +               /* mem-region16 (spa/dcr4, dimm4) */
> +               memdev = nfit_buf + offset +
> +                               sizeof(struct acpi_nfit_memory_map) * 2;
> +               memdev->header.type = ACPI_NFIT_TYPE_MEMORY_MAP;
> +               memdev->header.length = sizeof(*memdev);
> +               memdev->device_handle = handle[4];
> +               memdev->physical_id = 4;
> +               memdev->region_id = 0;
> +               memdev->range_index = 12+1;
> +               memdev->region_index = 4+1;
> +               memdev->region_size = 0;
> +               memdev->region_offset = 0;
> +               memdev->address = 0;
> +               memdev->interleave_index = 0;
> +               memdev->interleave_ways = 1;
> +
> +       }
> +
>         acpi_desc = &t->acpi_desc;
>         set_bit(ND_CMD_GET_CONFIG_SIZE, &acpi_desc->dimm_dsm_force_en);
>         set_bit(ND_CMD_GET_CONFIG_DATA, &acpi_desc->dimm_dsm_force_en);
> @@ -1114,6 +1242,18 @@ static int nfit_test_probe(struct platform_device *pdev)
>                 return rc;
>         }
>
> +       if (nfit_test->setup != nfit_test0_setup)
> +               return 0;
> +
> +       nfit_test->setup_hotplug = 1;
> +       nfit_test->setup(nfit_test);
> +
> +       rc = acpi_nfit_merge(acpi_desc, nfit_test->nfit_size);
> +       if (rc) {
> +               nvdimm_bus_unregister(acpi_desc->nvdimm_bus);
> +               return rc;
> +       }
> +
>         return 0;
>  }
>
> --
> 2.4.3
>
> +       acpi_desc->nfit = (struct acpi_table_nfit *)buf;
> +       ret = acpi_nfit_merge(acpi_desc, buf->length);

As far as I can see this should be done under device_lock(dev) to
flush in-flight ->probe().  However, we can't take it here because
->remove() appears to trigger a ->notify() as well.  Rafael, should we
add device_lock() to acpi_bus_notify(), or am I missing some other
lock that addresses this?

Re: [PATCH 2/3] acpi: add a utility function for evaluating _FIT

[Rafael J. Wysocki]

On Friday, October 09, 2015 10:56:26 AM Dan Williams wrote:
> On Fri, Oct 9, 2015 at 10:54 AM, Verma, Vishal L
> <vishal.l.verma@intel.com> wrote:
> > On Fri, 2015-10-09 at 13:28 -0400, Jeff Moyer wrote:
> >> Vishal Verma <vishal.l.verma@intel.com> writes:
> >>
> >> >  /**
> >> > + * acpi_evaluate_fit: Evaluate _FIT method to get an updated NFIT
> >> > + * @handle: ACPI device handle
> >> > + * @buf: buffer for the updated NFIT
> >> > + *
> >> > + * Evaluate device's _FIT method if present to get an updated NFIT
> >> > + */
> >> > +acpi_status acpi_evaluate_fit(acpi_handle handle, struct
> >> > acpi_buffer **buf)
> >> > +{
> >> > +   acpi_status status;
> >> > +   struct acpi_buffer buffer = { ACPI_ALLOCATE_BUFFER, NULL
> >> > };
> >> > +
> >> > +   status = acpi_evaluate_object(handle, "_FIT", NULL,
> >> > &buffer);
> >> > +
> >> > +   if (ACPI_FAILURE(status))
> >> > +           return status;
> >> > +
> >> > +   *buf = &buffer;
> >>
> >> Umm, unless I'm missing something, you're returning a stack address.
> >
> > Good point, you're right. Dan/Rafael, is it OK to just remove this
> > patch entirely, and call acpi_evaluate_object directly from nfit.c? The
> > current way did feel a bit kludgey to me any way because I was
> > allocating a buffer here (above), but trying to free it in the caller,
> > which seems very ugly..
> 
> Open coding a call to acpi_evaluate_object() sounds good to me.

Agreed.

Thanks,
Rafael

Re: [PATCH 3/3] acpi: nfit: Add support for hotplug

[Toshi Kani]

On Fri, 2015-10-09 at 12:44 -0700, Dan Williams wrote:
> [ adding Robert and Toshi ]
> 
> On Wed, Oct 7, 2015 at 2:49 PM, Vishal Verma <vishal.l.verma@intel.com>
> wrote:
 :
> > @@ -242,9 +246,13 @@ static bool add_memdev(struct acpi_nfit_desc
> > *acpi_desc,
> >                 struct acpi_nfit_memory_map *memdev)
> >  {
> >         struct device *dev = acpi_desc->dev;
> > -       struct nfit_memdev *nfit_memdev = devm_kzalloc(dev,
> > -                       sizeof(*nfit_memdev), GFP_KERNEL);
> > +       struct nfit_memdev *nfit_memdev;
> > +
> > +       list_for_each_entry(nfit_memdev, &acpi_desc->memdevs, list)
> > +               if (memcmp(nfit_memdev->memdev, memdev,
> > sizeof(*memdev)) == 0)
> > +                       return true;
> 
> I'm wondering if we need to be cognizant of flag changes here.
> Robert, Toshi are you expecting that the flags of an existing memory
> device entry will be updated by the this notification mechanism?

This patch-set can focus on the simple hot-add case for now.  We think bit
[4] of the flags, smart and health event bit, could be updated on an
existing memory device when such an event has occurred during runtime, but
it may require clarification to the spec since the bit is currently defined
as "SMART and health events prior to OSPM handoff" (which does not make
sense for _FIT).  We will need to support other updates in the flags, but
we will first need to understand what the OS should do in such scenarios.

> > @@ -811,6 +835,8 @@ static int acpi_nfit_register_dimms(struct
> > acpi_nfit_desc *acpi_desc)
> >                          */
> >                         dev_err(acpi_desc->dev, "duplicate DCR
> > detected: %s\n",
> >                                         nvdimm_name(nvdimm));
> > +                       /* TODO Do we need the warning? */
> > +                       dimm_count++;
> 
> Robert, comments?

Yes, this warning message should be removed.

Thanks,
-Toshi