[PATCH 0/3] fix NVDIMM hotplug changes

[PATCH 0/3] fix NVDIMM hotplug changes

[Linda Knippers]

Since commit 209851649dc4f7900a6bfe1de5e2640ab2c7d931 
("acpi: nfit: Add support for hot-add"), we no longer see 
NVDIMM devices on our NVDIMM-N systems.  The NFIT/_FIT processing 
at initialization gets a table from _FIT but thinks it is 
malformed and exits, leaving us with no NFIT at all.

This series fixes the problem caused by a difference in ACPI
spec interpretation regarding the layout of the _FIT vs. NFIT
tables.  In addition to changing the code, we are proposing
wording changes to the ACPI spec.

While testing the notify code path, I discovered that the 
way the old and new tables were compared didn't take into account
that there is some variation in table length.  I also determined
that the check for whether the NFIT merge was successful was
backwards.

I couldn't find a generic way to invoke the notify function
so my testing was done with a change to the the revision_show()
function that calls acpi_nfit_notify() any time the sysfs
nfit/revision file is read.  That change is not included here.

These patches were tested on a system with NVDIMM-N hardware
so changes related to NFIT tables not provided on a NVDIMM-N
system have only been compile-tested.  

Linda Knippers (3):
  nfit: Account for table size length variation
  nfit: Fix the check for a successful NFIT merge
  nfit: Adjust for different _FIT and NFIT headers

 drivers/acpi/nfit.c | 83 +++++++++++++++++++++++++++++++++++++++++------------
 drivers/acpi/nfit.h |  3 +-
 2 files changed, 67 insertions(+), 19 deletions(-)

-- 
1.8.3.1

[PATCH 1/3] nfit: Account for table size length variation

[Linda Knippers]

The size of NFIT tables don't necessarily match the size of the
data structures that we use for them.  For example, the NVDIMM
Control Region Structure table is shorter for a device with
no block control windows than for a device with block control windows.
Other tables, such as Flush Hint Address Structure and the Interleave
Structure are variable length by definition.

Account for the size difference when comparing table entries by
using the actual table size from the table header if it's less
than the structure size.  

Signed-off-by: Linda Knippers <linda.knippers@hpe.com>
---
 drivers/acpi/nfit.c | 36 ++++++++++++++++++++++++++++++------
 1 file changed, 30 insertions(+), 6 deletions(-)

diff --git a/drivers/acpi/nfit.c b/drivers/acpi/nfit.c
index f7dab53..710092a 100644
--- a/drivers/acpi/nfit.c
+++ b/drivers/acpi/nfit.c
@@ -235,9 +235,13 @@ static bool add_spa(struct acpi_nfit_desc *acpi_desc,
 {
 	struct device *dev = acpi_desc->dev;
 	struct nfit_spa *nfit_spa;
+	size_t length = sizeof(*spa);
+
+	if (length > spa->header.length)
+		length = spa->header.length;
 
 	list_for_each_entry(nfit_spa, &prev->spas, list) {
-		if (memcmp(nfit_spa->spa, spa, sizeof(*spa)) == 0) {
+		if (memcmp(nfit_spa->spa, spa, length) == 0) {
 			list_move_tail(&nfit_spa->list, &acpi_desc->spas);
 			return true;
 		}
@@ -261,9 +265,13 @@ static bool add_memdev(struct acpi_nfit_desc *acpi_desc,
 {
 	struct device *dev = acpi_desc->dev;
 	struct nfit_memdev *nfit_memdev;
+	size_t length = sizeof(*memdev);
+
+	if (length > memdev->header.length)
+		length = memdev->header.length;
 
 	list_for_each_entry(nfit_memdev, &prev->memdevs, list)
-		if (memcmp(nfit_memdev->memdev, memdev, sizeof(*memdev)) == 0) {
+		if (memcmp(nfit_memdev->memdev, memdev, length) == 0) {
 			list_move_tail(&nfit_memdev->list, &acpi_desc->memdevs);
 			return true;
 		}
@@ -286,9 +294,13 @@ static bool add_dcr(struct acpi_nfit_desc *acpi_desc,
 {
 	struct device *dev = acpi_desc->dev;
 	struct nfit_dcr *nfit_dcr;
+	size_t length = sizeof(*dcr);
+
+	if (length > dcr->header.length)
+		length = dcr->header.length;
 
 	list_for_each_entry(nfit_dcr, &prev->dcrs, list)
-		if (memcmp(nfit_dcr->dcr, dcr, sizeof(*dcr)) == 0) {
+		if (memcmp(nfit_dcr->dcr, dcr, length) == 0) {
 			list_move_tail(&nfit_dcr->list, &acpi_desc->dcrs);
 			return true;
 		}
@@ -310,9 +322,13 @@ static bool add_bdw(struct acpi_nfit_desc *acpi_desc,
 {
 	struct device *dev = acpi_desc->dev;
 	struct nfit_bdw *nfit_bdw;
+	size_t length = sizeof(*bdw);
+
+	if (length > bdw->header.length)
+		length = bdw->header.length;
 
 	list_for_each_entry(nfit_bdw, &prev->bdws, list)
-		if (memcmp(nfit_bdw->bdw, bdw, sizeof(*bdw)) == 0) {
+		if (memcmp(nfit_bdw->bdw, bdw, length) == 0) {
 			list_move_tail(&nfit_bdw->list, &acpi_desc->bdws);
 			return true;
 		}
@@ -334,9 +350,13 @@ static bool add_idt(struct acpi_nfit_desc *acpi_desc,
 {
 	struct device *dev = acpi_desc->dev;
 	struct nfit_idt *nfit_idt;
+	size_t length = sizeof(*idt);
+
+	if (length > idt->header.length)
+		length = idt->header.length;
 
 	list_for_each_entry(nfit_idt, &prev->idts, list)
-		if (memcmp(nfit_idt->idt, idt, sizeof(*idt)) == 0) {
+		if (memcmp(nfit_idt->idt, idt, length) == 0) {
 			list_move_tail(&nfit_idt->list, &acpi_desc->idts);
 			return true;
 		}
@@ -358,9 +378,13 @@ static bool add_flush(struct acpi_nfit_desc *acpi_desc,
 {
 	struct device *dev = acpi_desc->dev;
 	struct nfit_flush *nfit_flush;
+	size_t length = sizeof(*flush);
+
+	if (length > flush->header.length)
+		length = flush->header.length;
 
 	list_for_each_entry(nfit_flush, &prev->flushes, list)
-		if (memcmp(nfit_flush->flush, flush, sizeof(*flush)) == 0) {
+		if (memcmp(nfit_flush->flush, flush, length) == 0) {
 			list_move_tail(&nfit_flush->list, &acpi_desc->flushes);
 			return true;
 		}
-- 
1.8.3.1

[PATCH 2/3] nfit: Fix the check for a successful NFIT merge

[Linda Knippers]

Signed-off-by: Linda Knippers <linda.knippers@hpe.com>
---
 drivers/acpi/nfit.c | 2 +-
 1 file changed, 1 insertion(+), 1 deletion(-)

diff --git a/drivers/acpi/nfit.c b/drivers/acpi/nfit.c
index 710092a..c140194d 100644
--- a/drivers/acpi/nfit.c
+++ b/drivers/acpi/nfit.c
@@ -1834,7 +1834,7 @@ static void acpi_nfit_notify(struct acpi_device *adev, u32 event)
 	nfit_saved = acpi_desc->nfit;
 	acpi_desc->nfit = (struct acpi_table_nfit *)buf.pointer;
 	ret = acpi_nfit_init(acpi_desc, buf.length);
-	if (!ret) {
+	if (ret) {
 		/* Merge failed, restore old nfit, and exit */
 		acpi_desc->nfit = nfit_saved;
 		dev_err(dev, "failed to merge updated NFIT\n");
-- 
1.8.3.1

[PATCH 3/3] nfit: Adjust for different _FIT and NFIT headers

[Linda Knippers]

When support for _FIT was added, the code presumed that the data
returned by the _FIT method is identical to the NFIT table, which
starts with an acpi_table_header.  However, the _FIT is defined
to return a data in the format of a series of NFIT type structure
entries and as a method, has an acpi_object header rather tahn
an acpi_table_header.

To address the differences, explicitly save the acpi_table_header
from the NFIT, since it is accessible through /sys, and change
the nfit pointer in the acpi_desc structure to point to the
table entries rather than the headers.

Reported-by: Jeff Moyer (jmoyer@redhat.com>
Signed-off-by: Linda Knippers <linda.knippers@hpe.com>
---
 drivers/acpi/nfit.c | 47 +++++++++++++++++++++++++++++++++++------------
 drivers/acpi/nfit.h |  3 ++-
 2 files changed, 37 insertions(+), 13 deletions(-)

diff --git a/drivers/acpi/nfit.c b/drivers/acpi/nfit.c
index c140194d..b1fae13 100644
--- a/drivers/acpi/nfit.c
+++ b/drivers/acpi/nfit.c
@@ -679,7 +679,7 @@ static ssize_t revision_show(struct device *dev,
 	struct nvdimm_bus_descriptor *nd_desc = to_nd_desc(nvdimm_bus);
 	struct acpi_nfit_desc *acpi_desc = to_acpi_desc(nd_desc);
 
-	return sprintf(buf, "%d\n", acpi_desc->nfit->header.revision);
+	return sprintf(buf, "%d\n", acpi_desc->acpi_header.revision);
 }
 static DEVICE_ATTR_RO(revision);
 
@@ -1676,7 +1676,6 @@ int acpi_nfit_init(struct acpi_nfit_desc *acpi_desc, acpi_size sz)
 
 	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, &prev, data, end);
 
@@ -1772,13 +1771,29 @@ static int acpi_nfit_add(struct acpi_device *adev)
 		return PTR_ERR(acpi_desc);
 	}
 
-	acpi_desc->nfit = (struct acpi_table_nfit *) tbl;
+	/*
+	 * Save the acpi header for later and then skip it,
+	 * making nfit point to the first nfit table header.
+	 */
+	acpi_desc->acpi_header = *tbl;
+	acpi_desc->nfit = (void *) tbl + sizeof(struct acpi_table_nfit);
+	sz -= sizeof(struct acpi_table_nfit);
 
 	/* Evaluate _FIT and override with that if present */
 	status = acpi_evaluate_object(adev->handle, "_FIT", NULL, &buf);
 	if (ACPI_SUCCESS(status) && buf.length > 0) {
-		acpi_desc->nfit = (struct acpi_table_nfit *)buf.pointer;
-		sz = buf.length;
+		union acpi_object *obj;
+		/*
+		 * Adjust for the acpi_object header of the _FIT
+		 */
+		obj = buf.pointer;
+		if (obj->type == ACPI_TYPE_BUFFER) {
+			acpi_desc->nfit =
+				(struct acpi_nfit_header *)obj->buffer.pointer;
+			sz = obj->buffer.length;
+		} else
+			dev_dbg(dev, "%s invalid type %d, ignoring _FIT\n",
+				 __func__, (int) obj->type);
 	}
 
 	rc = acpi_nfit_init(acpi_desc, sz);
@@ -1801,7 +1816,8 @@ static void acpi_nfit_notify(struct acpi_device *adev, u32 event)
 {
 	struct acpi_nfit_desc *acpi_desc = dev_get_drvdata(&adev->dev);
 	struct acpi_buffer buf = { ACPI_ALLOCATE_BUFFER, NULL };
-	struct acpi_table_nfit *nfit_saved;
+	struct acpi_nfit_header *nfit_saved;
+	union acpi_object *obj;
 	struct device *dev = &adev->dev;
 	acpi_status status;
 	int ret;
@@ -1832,12 +1848,19 @@ static void acpi_nfit_notify(struct acpi_device *adev, u32 event)
 	}
 
 	nfit_saved = acpi_desc->nfit;
-	acpi_desc->nfit = (struct acpi_table_nfit *)buf.pointer;
-	ret = acpi_nfit_init(acpi_desc, buf.length);
-	if (ret) {
-		/* Merge failed, restore old nfit, and exit */
-		acpi_desc->nfit = nfit_saved;
-		dev_err(dev, "failed to merge updated NFIT\n");
+	obj = buf.pointer;
+	if (obj->type == ACPI_TYPE_BUFFER) {
+		acpi_desc->nfit =
+			(struct acpi_nfit_header *)obj->buffer.pointer;
+		ret = acpi_nfit_init(acpi_desc, obj->buffer.length);
+		if (ret) {
+			/* Merge failed, restore old nfit, and exit */
+			acpi_desc->nfit = nfit_saved;
+			dev_err(dev, "failed to merge updated NFIT\n");
+		}
+	} else {
+		/* Bad _FIT, restore old nfit */
+		dev_err(dev, "Invalid _FIT\n");
 	}
 	kfree(buf.pointer);
 
diff --git a/drivers/acpi/nfit.h b/drivers/acpi/nfit.h
index 2ea5c07..3d549a3 100644
--- a/drivers/acpi/nfit.h
+++ b/drivers/acpi/nfit.h
@@ -96,7 +96,8 @@ struct nfit_mem {
 
 struct acpi_nfit_desc {
 	struct nvdimm_bus_descriptor nd_desc;
-	struct acpi_table_nfit *nfit;
+	struct acpi_table_header acpi_header;
+	struct acpi_nfit_header *nfit;
 	struct mutex spa_map_mutex;
 	struct mutex init_mutex;
 	struct list_head spa_maps;
-- 
1.8.3.1

Re: [PATCH 1/3] nfit: Account for table size length variation

[Verma, Vishal L]

On Fri, 2015-11-20 at 19:05 -0500, Linda Knippers wrote:
> The size of NFIT tables don't necessarily match the size of the
> data structures that we use for them.  For example, the NVDIMM
> Control Region Structure table is shorter for a device with
> no block control windows than for a device with block control windows.
> Other tables, such as Flush Hint Address Structure and the Interleave
> Structure are variable length by definition.
> 
> Account for the size difference when comparing table entries by
> using the actual table size from the table header if it's less
> than the structure size.  
> 

Agreed about the variable length tables - Flush Hint and Interleave. But
for the others, this makes it possible for a buggy bios implementation
to have a *really* small table - one that doesn't even have enough
fields to work - to pass the add_tables stage where it might have failed
previously.

I feel there may be need for an ACPI clarification for this specifying
whether if certain fields are irrelevant, they can be excluded entirely.

For example, the DCR wording is:
Number of Block Control Windows must match the
corresponding number of Block Data Windows. Fields that
follow this field are valid only if the number of Block Control
Windows is non-zero.

This leads me to believe that those fields should be 'present' but
ignored in the case of zero block windows.

If we make add_tables process only header.length and accept the
shortened table, there is nothing to tell future code that the structure
that piece of memory is casted to is a truncated one.

Thoughts?

	-Vishal

Re: [PATCH 3/3] nfit: Adjust for different _FIT and NFIT headers

[Verma, Vishal L]

On Fri, 2015-11-20 at 19:05 -0500, Linda Knippers wrote:
> When support for _FIT was added, the code presumed that the data
> returned by the _FIT method is identical to the NFIT table, which
> starts with an acpi_table_header.  However, the _FIT is defined
> to return a data in the format of a series of NFIT type structure
> entries and as a method, has an acpi_object header rather tahn
> an acpi_table_header.
> 
> To address the differences, explicitly save the acpi_table_header
> from the NFIT, since it is accessible through /sys, and change
> the nfit pointer in the acpi_desc structure to point to the
> table entries rather than the headers.
> 
> Reported-by: Jeff Moyer (jmoyer@redhat.com>
> Signed-off-by: Linda Knippers <linda.knippers@hpe.com>
> ---
>  drivers/acpi/nfit.c | 47 +++++++++++++++++++++++++++++++++++---------
> ---
>  drivers/acpi/nfit.h |  3 ++-
>  2 files changed, 37 insertions(+), 13 deletions(-)
> 

This patch looks good, but the fact that we're doing the header
processing earlier (before acpi_nfit_init) means that the nfit supplied
by the nfit_test framework (tools/testing/nvdimm/test) can no longer
have a header. The following patch fixes this.

Apart from that, and the open question for patch 1, for the series,

Reviewed-by: Vishal Verma <vishal.l.verma@intel.com>



8<-----

From da2fd5730bc5125c72101a6b55b85fce97663e40 Mon Sep 17 00:00:00 2001
From: Vishal Verma <vishal.l.verma@intel.com>
Date: Mon, 23 Nov 2015 15:41:00 -0700
Subject: [PATCH] nfit_test: Fix nfit generation for test framework

The hotplug fixes move header parsing to happen earlier,
and the way nfit_test calls into the nfit initialization code,
a header is no longer needed. Remove the header from the table
generated by nfit_test entirely.

Signed-off-by: Vishal Verma <vishal.l.verma@intel.com>
---
 tools/testing/nvdimm/test/nfit.c | 46 ++++++++++++----------------------------
 1 file changed, 14 insertions(+), 32 deletions(-)

diff --git a/tools/testing/nvdimm/test/nfit.c b/tools/testing/nvdimm/test/nfit.c
index 40ab447..3f92d7f 100644
--- a/tools/testing/nvdimm/test/nfit.c
+++ b/tools/testing/nvdimm/test/nfit.c
@@ -420,8 +420,7 @@ static struct nfit_test_resource *nfit_test_lookup(resource_size_t addr)
 
 static int nfit_test0_alloc(struct nfit_test *t)
 {
-	size_t nfit_size = sizeof(struct acpi_table_nfit)
-			+ sizeof(struct acpi_nfit_system_address) * NUM_SPA
+	size_t nfit_size = sizeof(struct acpi_nfit_system_address) * NUM_SPA
 			+ sizeof(struct acpi_nfit_memory_map) * NUM_MEM
 			+ sizeof(struct acpi_nfit_control_region) * NUM_DCR
 			+ sizeof(struct acpi_nfit_data_region) * NUM_BDW
@@ -471,8 +470,7 @@ static int nfit_test0_alloc(struct nfit_test *t)
 
 static int nfit_test1_alloc(struct nfit_test *t)
 {
-	size_t nfit_size = sizeof(struct acpi_table_nfit)
-		+ sizeof(struct acpi_nfit_system_address)
+	size_t nfit_size = sizeof(struct acpi_nfit_system_address)
 		+ sizeof(struct acpi_nfit_memory_map)
 		+ sizeof(struct acpi_nfit_control_region);
 
@@ -488,18 +486,6 @@ static int nfit_test1_alloc(struct nfit_test *t)
 	return 0;
 }
 
-static void nfit_test_init_header(struct acpi_table_nfit *nfit, size_t size)
-{
-	memcpy(nfit->header.signature, ACPI_SIG_NFIT, 4);
-	nfit->header.length = size;
-	nfit->header.revision = 1;
-	memcpy(nfit->header.oem_id, "LIBND", 6);
-	memcpy(nfit->header.oem_table_id, "TEST", 5);
-	nfit->header.oem_revision = 1;
-	memcpy(nfit->header.asl_compiler_id, "TST", 4);
-	nfit->header.asl_compiler_revision = 1;
-}
-
 static void nfit_test0_setup(struct nfit_test *t)
 {
 	struct nvdimm_bus_descriptor *nd_desc;
@@ -513,14 +499,12 @@ static void nfit_test0_setup(struct nfit_test *t)
 	struct acpi_nfit_flush_address *flush;
 	unsigned int offset;
 
-	nfit_test_init_header(nfit_buf, size);
-
 	/*
 	 * spa0 (interleave first half of dimm0 and dimm1, note storage
 	 * does not actually alias the related block-data-window
 	 * regions)
 	 */
-	spa = nfit_buf + sizeof(struct acpi_table_nfit);
+	spa = nfit_buf;
 	spa->header.type = ACPI_NFIT_TYPE_SYSTEM_ADDRESS;
 	spa->header.length = sizeof(*spa);
 	memcpy(spa->range_guid, to_nfit_uuid(NFIT_SPA_PM), 16);
@@ -533,7 +517,7 @@ static void nfit_test0_setup(struct nfit_test *t)
 	 * does not actually alias the related block-data-window
 	 * regions)
 	 */
-	spa = nfit_buf + sizeof(struct acpi_table_nfit) + sizeof(*spa);
+	spa = nfit_buf + 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);
@@ -542,7 +526,7 @@ static void nfit_test0_setup(struct nfit_test *t)
 	spa->length = SPA1_SIZE;
 
 	/* spa2 (dcr0) dimm0 */
-	spa = nfit_buf + sizeof(struct acpi_table_nfit) + sizeof(*spa) * 2;
+	spa = nfit_buf + 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_DCR), 16);
@@ -551,7 +535,7 @@ static void nfit_test0_setup(struct nfit_test *t)
 	spa->length = DCR_SIZE;
 
 	/* spa3 (dcr1) dimm1 */
-	spa = nfit_buf + sizeof(struct acpi_table_nfit) + sizeof(*spa) * 3;
+	spa = nfit_buf + sizeof(*spa) * 3;
 	spa->header.type = ACPI_NFIT_TYPE_SYSTEM_ADDRESS;
 	spa->header.length = sizeof(*spa);
 	memcpy(spa->range_guid, to_nfit_uuid(NFIT_SPA_DCR), 16);
@@ -560,7 +544,7 @@ static void nfit_test0_setup(struct nfit_test *t)
 	spa->length = DCR_SIZE;
 
 	/* spa4 (dcr2) dimm2 */
-	spa = nfit_buf + sizeof(struct acpi_table_nfit) + sizeof(*spa) * 4;
+	spa = nfit_buf + sizeof(*spa) * 4;
 	spa->header.type = ACPI_NFIT_TYPE_SYSTEM_ADDRESS;
 	spa->header.length = sizeof(*spa);
 	memcpy(spa->range_guid, to_nfit_uuid(NFIT_SPA_DCR), 16);
@@ -569,7 +553,7 @@ static void nfit_test0_setup(struct nfit_test *t)
 	spa->length = DCR_SIZE;
 
 	/* spa5 (dcr3) dimm3 */
-	spa = nfit_buf + sizeof(struct acpi_table_nfit) + sizeof(*spa) * 5;
+	spa = nfit_buf + sizeof(*spa) * 5;
 	spa->header.type = ACPI_NFIT_TYPE_SYSTEM_ADDRESS;
 	spa->header.length = sizeof(*spa);
 	memcpy(spa->range_guid, to_nfit_uuid(NFIT_SPA_DCR), 16);
@@ -578,7 +562,7 @@ static void nfit_test0_setup(struct nfit_test *t)
 	spa->length = DCR_SIZE;
 
 	/* spa6 (bdw for dcr0) dimm0 */
-	spa = nfit_buf + sizeof(struct acpi_table_nfit) + sizeof(*spa) * 6;
+	spa = nfit_buf + sizeof(*spa) * 6;
 	spa->header.type = ACPI_NFIT_TYPE_SYSTEM_ADDRESS;
 	spa->header.length = sizeof(*spa);
 	memcpy(spa->range_guid, to_nfit_uuid(NFIT_SPA_BDW), 16);
@@ -587,7 +571,7 @@ static void nfit_test0_setup(struct nfit_test *t)
 	spa->length = DIMM_SIZE;
 
 	/* spa7 (bdw for dcr1) dimm1 */
-	spa = nfit_buf + sizeof(struct acpi_table_nfit) + sizeof(*spa) * 7;
+	spa = nfit_buf + sizeof(*spa) * 7;
 	spa->header.type = ACPI_NFIT_TYPE_SYSTEM_ADDRESS;
 	spa->header.length = sizeof(*spa);
 	memcpy(spa->range_guid, to_nfit_uuid(NFIT_SPA_BDW), 16);
@@ -596,7 +580,7 @@ static void nfit_test0_setup(struct nfit_test *t)
 	spa->length = DIMM_SIZE;
 
 	/* spa8 (bdw for dcr2) dimm2 */
-	spa = nfit_buf + sizeof(struct acpi_table_nfit) + sizeof(*spa) * 8;
+	spa = nfit_buf + sizeof(*spa) * 8;
 	spa->header.type = ACPI_NFIT_TYPE_SYSTEM_ADDRESS;
 	spa->header.length = sizeof(*spa);
 	memcpy(spa->range_guid, to_nfit_uuid(NFIT_SPA_BDW), 16);
@@ -605,7 +589,7 @@ static void nfit_test0_setup(struct nfit_test *t)
 	spa->length = DIMM_SIZE;
 
 	/* spa9 (bdw for dcr3) dimm3 */
-	spa = nfit_buf + sizeof(struct acpi_table_nfit) + sizeof(*spa) * 9;
+	spa = nfit_buf + sizeof(*spa) * 9;
 	spa->header.type = ACPI_NFIT_TYPE_SYSTEM_ADDRESS;
 	spa->header.length = sizeof(*spa);
 	memcpy(spa->range_guid, to_nfit_uuid(NFIT_SPA_BDW), 16);
@@ -613,7 +597,7 @@ static void nfit_test0_setup(struct nfit_test *t)
 	spa->address = t->dimm_dma[3];
 	spa->length = DIMM_SIZE;
 
-	offset = sizeof(struct acpi_table_nfit) + sizeof(*spa) * 10;
+	offset = sizeof(*spa) * 10;
 	/* mem-region0 (spa0, dimm0) */
 	memdev = nfit_buf + offset;
 	memdev->header.type = ACPI_NFIT_TYPE_MEMORY_MAP;
@@ -1106,9 +1090,7 @@ static void nfit_test1_setup(struct nfit_test *t)
 	struct acpi_nfit_control_region *dcr;
 	struct acpi_nfit_system_address *spa;
 
-	nfit_test_init_header(nfit_buf, size);
-
-	offset = sizeof(struct acpi_table_nfit);
+	offset = 0;
 	/* spa0 (flat range with no bdw aliasing) */
 	spa = nfit_buf + offset;
 	spa->header.type = ACPI_NFIT_TYPE_SYSTEM_ADDRESS;
-- 
2.5.0

Re: [PATCH 1/3] nfit: Account for table size length variation

[Linda Knippers]

On 11/23/2015 6:21 PM, Verma, Vishal L wrote:
> On Fri, 2015-11-20 at 19:05 -0500, Linda Knippers wrote:
>> The size of NFIT tables don't necessarily match the size of the
>> data structures that we use for them.  For example, the NVDIMM
>> Control Region Structure table is shorter for a device with
>> no block control windows than for a device with block control windows.
>> Other tables, such as Flush Hint Address Structure and the Interleave
>> Structure are variable length by definition.
>>
>> Account for the size difference when comparing table entries by
>> using the actual table size from the table header if it's less
>> than the structure size.
>>
>
> Agreed about the variable length tables - Flush Hint and Interleave. But
> for the others, this makes it possible for a buggy bios implementation
> to have a *really* small table - one that doesn't even have enough
> fields to work - to pass the add_tables stage where it might have failed
> previously.
>
> I feel there may be need for an ACPI clarification for this specifying
> whether if certain fields are irrelevant, they can be excluded entirely.
>
> For example, the DCR wording is:
> Number of Block Control Windows must match the
> corresponding number of Block Data Windows. Fields that
> follow this field are valid only if the number of Block Control
> Windows is non-zero.
>
> This leads me to believe that those fields should be 'present' but
> ignored in the case of zero block windows.

Actually, the spec is pretty clear in this case.  If you look at the
length definition for that table (5-133) it says:

	Length in bytes for entire structure.
	The length of this structure is either 32 bytes or 80 bytes. The
	length of the structure can be 32 bytes only if the Number of
	Block Control Windows field has a value of 0.

The structure is 80 bytes but it is legal to have a 32-byte table.
We hit a similar problem with the original NFIT code.  We could
explicitly check for a size of 32 but we didn't before.

> If we make add_tables process only header.length and accept the
> shortened table, there is nothing to tell future code that the structure
> that piece of memory is casted to is a truncated one.
>
> Thoughts?

If we want to be more paranoid about buggy FW when we're comparing old
and new tables, we could compare based on the length of the old and new
tables since we have both pieces of information.  That would let you catch
the case where a table size changes during a hotplug event or whenever the
_FIT is processed.  Since you were comparing based on structure size instead
of header length, I didn't change that.

-- ljk
>
> 	-Vishal
>

Re: [PATCH 3/3] nfit: Adjust for different _FIT and NFIT headers

[Linda Knippers]

On 11/23/2015 6:26 PM, Verma, Vishal L wrote:
> On Fri, 2015-11-20 at 19:05 -0500, Linda Knippers wrote:
>> When support for _FIT was added, the code presumed that the data
>> returned by the _FIT method is identical to the NFIT table, which
>> starts with an acpi_table_header.  However, the _FIT is defined
>> to return a data in the format of a series of NFIT type structure
>> entries and as a method, has an acpi_object header rather tahn
>> an acpi_table_header.
>>
>> To address the differences, explicitly save the acpi_table_header
>> from the NFIT, since it is accessible through /sys, and change
>> the nfit pointer in the acpi_desc structure to point to the
>> table entries rather than the headers.
>>
>> Reported-by: Jeff Moyer (jmoyer@redhat.com>
>> Signed-off-by: Linda Knippers <linda.knippers@hpe.com>
>> ---
>>   drivers/acpi/nfit.c | 47 +++++++++++++++++++++++++++++++++++---------
>> ---
>>   drivers/acpi/nfit.h |  3 ++-
>>   2 files changed, 37 insertions(+), 13 deletions(-)
>>
>
> This patch looks good, but the fact that we're doing the header
> processing earlier (before acpi_nfit_init) means that the nfit supplied
> by the nfit_test framework (tools/testing/nvdimm/test) can no longer
> have a header. The following patch fixes this.
>
> Apart from that, and the open question for patch 1, for the series,
>
> Reviewed-by: Vishal Verma <vishal.l.verma@intel.com>

Thanks Vishal.

I hope I've addressed your question on patch 1.  I think patch 1 works
as well as the original code did so if you have concerns about making
the code more bullet proof in the face of buggy firmware, perhaps we can
address that separately from this series.

I would really like to see this series go in soon because since the
hotplug code went in I can't run an upstream kernel on my hardware.

-- ljk
>
>
>
> 8<-----
>
>  From da2fd5730bc5125c72101a6b55b85fce97663e40 Mon Sep 17 00:00:00 2001
> From: Vishal Verma <vishal.l.verma@intel.com>
> Date: Mon, 23 Nov 2015 15:41:00 -0700
> Subject: [PATCH] nfit_test: Fix nfit generation for test framework
>
> The hotplug fixes move header parsing to happen earlier,
> and the way nfit_test calls into the nfit initialization code,
> a header is no longer needed. Remove the header from the table
> generated by nfit_test entirely.
>
> Signed-off-by: Vishal Verma <vishal.l.verma@intel.com>
> ---
>   tools/testing/nvdimm/test/nfit.c | 46 ++++++++++++----------------------------
>   1 file changed, 14 insertions(+), 32 deletions(-)
>
> diff --git a/tools/testing/nvdimm/test/nfit.c b/tools/testing/nvdimm/test/nfit.c
> index 40ab447..3f92d7f 100644
> --- a/tools/testing/nvdimm/test/nfit.c
> +++ b/tools/testing/nvdimm/test/nfit.c
> @@ -420,8 +420,7 @@ static struct nfit_test_resource *nfit_test_lookup(resource_size_t addr)
>
>   static int nfit_test0_alloc(struct nfit_test *t)
>   {
> -	size_t nfit_size = sizeof(struct acpi_table_nfit)
> -			+ sizeof(struct acpi_nfit_system_address) * NUM_SPA
> +	size_t nfit_size = sizeof(struct acpi_nfit_system_address) * NUM_SPA
>   			+ sizeof(struct acpi_nfit_memory_map) * NUM_MEM
>   			+ sizeof(struct acpi_nfit_control_region) * NUM_DCR
>   			+ sizeof(struct acpi_nfit_data_region) * NUM_BDW
> @@ -471,8 +470,7 @@ static int nfit_test0_alloc(struct nfit_test *t)
>
>   static int nfit_test1_alloc(struct nfit_test *t)
>   {
> -	size_t nfit_size = sizeof(struct acpi_table_nfit)
> -		+ sizeof(struct acpi_nfit_system_address)
> +	size_t nfit_size = sizeof(struct acpi_nfit_system_address)
>   		+ sizeof(struct acpi_nfit_memory_map)
>   		+ sizeof(struct acpi_nfit_control_region);
>
> @@ -488,18 +486,6 @@ static int nfit_test1_alloc(struct nfit_test *t)
>   	return 0;
>   }
>
> -static void nfit_test_init_header(struct acpi_table_nfit *nfit, size_t size)
> -{
> -	memcpy(nfit->header.signature, ACPI_SIG_NFIT, 4);
> -	nfit->header.length = size;
> -	nfit->header.revision = 1;
> -	memcpy(nfit->header.oem_id, "LIBND", 6);
> -	memcpy(nfit->header.oem_table_id, "TEST", 5);
> -	nfit->header.oem_revision = 1;
> -	memcpy(nfit->header.asl_compiler_id, "TST", 4);
> -	nfit->header.asl_compiler_revision = 1;
> -}
> -
>   static void nfit_test0_setup(struct nfit_test *t)
>   {
>   	struct nvdimm_bus_descriptor *nd_desc;
> @@ -513,14 +499,12 @@ static void nfit_test0_setup(struct nfit_test *t)
>   	struct acpi_nfit_flush_address *flush;
>   	unsigned int offset;
>
> -	nfit_test_init_header(nfit_buf, size);
> -
>   	/*
>   	 * spa0 (interleave first half of dimm0 and dimm1, note storage
>   	 * does not actually alias the related block-data-window
>   	 * regions)
>   	 */
> -	spa = nfit_buf + sizeof(struct acpi_table_nfit);
> +	spa = nfit_buf;
>   	spa->header.type = ACPI_NFIT_TYPE_SYSTEM_ADDRESS;
>   	spa->header.length = sizeof(*spa);
>   	memcpy(spa->range_guid, to_nfit_uuid(NFIT_SPA_PM), 16);
> @@ -533,7 +517,7 @@ static void nfit_test0_setup(struct nfit_test *t)
>   	 * does not actually alias the related block-data-window
>   	 * regions)
>   	 */
> -	spa = nfit_buf + sizeof(struct acpi_table_nfit) + sizeof(*spa);
> +	spa = nfit_buf + 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);
> @@ -542,7 +526,7 @@ static void nfit_test0_setup(struct nfit_test *t)
>   	spa->length = SPA1_SIZE;
>
>   	/* spa2 (dcr0) dimm0 */
> -	spa = nfit_buf + sizeof(struct acpi_table_nfit) + sizeof(*spa) * 2;
> +	spa = nfit_buf + 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_DCR), 16);
> @@ -551,7 +535,7 @@ static void nfit_test0_setup(struct nfit_test *t)
>   	spa->length = DCR_SIZE;
>
>   	/* spa3 (dcr1) dimm1 */
> -	spa = nfit_buf + sizeof(struct acpi_table_nfit) + sizeof(*spa) * 3;
> +	spa = nfit_buf + sizeof(*spa) * 3;
>   	spa->header.type = ACPI_NFIT_TYPE_SYSTEM_ADDRESS;
>   	spa->header.length = sizeof(*spa);
>   	memcpy(spa->range_guid, to_nfit_uuid(NFIT_SPA_DCR), 16);
> @@ -560,7 +544,7 @@ static void nfit_test0_setup(struct nfit_test *t)
>   	spa->length = DCR_SIZE;
>
>   	/* spa4 (dcr2) dimm2 */
> -	spa = nfit_buf + sizeof(struct acpi_table_nfit) + sizeof(*spa) * 4;
> +	spa = nfit_buf + sizeof(*spa) * 4;
>   	spa->header.type = ACPI_NFIT_TYPE_SYSTEM_ADDRESS;
>   	spa->header.length = sizeof(*spa);
>   	memcpy(spa->range_guid, to_nfit_uuid(NFIT_SPA_DCR), 16);
> @@ -569,7 +553,7 @@ static void nfit_test0_setup(struct nfit_test *t)
>   	spa->length = DCR_SIZE;
>
>   	/* spa5 (dcr3) dimm3 */
> -	spa = nfit_buf + sizeof(struct acpi_table_nfit) + sizeof(*spa) * 5;
> +	spa = nfit_buf + sizeof(*spa) * 5;
>   	spa->header.type = ACPI_NFIT_TYPE_SYSTEM_ADDRESS;
>   	spa->header.length = sizeof(*spa);
>   	memcpy(spa->range_guid, to_nfit_uuid(NFIT_SPA_DCR), 16);
> @@ -578,7 +562,7 @@ static void nfit_test0_setup(struct nfit_test *t)
>   	spa->length = DCR_SIZE;
>
>   	/* spa6 (bdw for dcr0) dimm0 */
> -	spa = nfit_buf + sizeof(struct acpi_table_nfit) + sizeof(*spa) * 6;
> +	spa = nfit_buf + sizeof(*spa) * 6;
>   	spa->header.type = ACPI_NFIT_TYPE_SYSTEM_ADDRESS;
>   	spa->header.length = sizeof(*spa);
>   	memcpy(spa->range_guid, to_nfit_uuid(NFIT_SPA_BDW), 16);
> @@ -587,7 +571,7 @@ static void nfit_test0_setup(struct nfit_test *t)
>   	spa->length = DIMM_SIZE;
>
>   	/* spa7 (bdw for dcr1) dimm1 */
> -	spa = nfit_buf + sizeof(struct acpi_table_nfit) + sizeof(*spa) * 7;
> +	spa = nfit_buf + sizeof(*spa) * 7;
>   	spa->header.type = ACPI_NFIT_TYPE_SYSTEM_ADDRESS;
>   	spa->header.length = sizeof(*spa);
>   	memcpy(spa->range_guid, to_nfit_uuid(NFIT_SPA_BDW), 16);
> @@ -596,7 +580,7 @@ static void nfit_test0_setup(struct nfit_test *t)
>   	spa->length = DIMM_SIZE;
>
>   	/* spa8 (bdw for dcr2) dimm2 */
> -	spa = nfit_buf + sizeof(struct acpi_table_nfit) + sizeof(*spa) * 8;
> +	spa = nfit_buf + sizeof(*spa) * 8;
>   	spa->header.type = ACPI_NFIT_TYPE_SYSTEM_ADDRESS;
>   	spa->header.length = sizeof(*spa);
>   	memcpy(spa->range_guid, to_nfit_uuid(NFIT_SPA_BDW), 16);
> @@ -605,7 +589,7 @@ static void nfit_test0_setup(struct nfit_test *t)
>   	spa->length = DIMM_SIZE;
>
>   	/* spa9 (bdw for dcr3) dimm3 */
> -	spa = nfit_buf + sizeof(struct acpi_table_nfit) + sizeof(*spa) * 9;
> +	spa = nfit_buf + sizeof(*spa) * 9;
>   	spa->header.type = ACPI_NFIT_TYPE_SYSTEM_ADDRESS;
>   	spa->header.length = sizeof(*spa);
>   	memcpy(spa->range_guid, to_nfit_uuid(NFIT_SPA_BDW), 16);
> @@ -613,7 +597,7 @@ static void nfit_test0_setup(struct nfit_test *t)
>   	spa->address = t->dimm_dma[3];
>   	spa->length = DIMM_SIZE;
>
> -	offset = sizeof(struct acpi_table_nfit) + sizeof(*spa) * 10;
> +	offset = sizeof(*spa) * 10;
>   	/* mem-region0 (spa0, dimm0) */
>   	memdev = nfit_buf + offset;
>   	memdev->header.type = ACPI_NFIT_TYPE_MEMORY_MAP;
> @@ -1106,9 +1090,7 @@ static void nfit_test1_setup(struct nfit_test *t)
>   	struct acpi_nfit_control_region *dcr;
>   	struct acpi_nfit_system_address *spa;
>
> -	nfit_test_init_header(nfit_buf, size);
> -
> -	offset = sizeof(struct acpi_table_nfit);
> +	offset = 0;
>   	/* spa0 (flat range with no bdw aliasing) */
>   	spa = nfit_buf + offset;
>   	spa->header.type = ACPI_NFIT_TYPE_SYSTEM_ADDRESS;
>

Re: [PATCH 1/3] nfit: Account for table size length variation

[Dan Williams]

On Tue, Nov 24, 2015 at 8:24 AM, Linda Knippers <linda.knippers@hpe.com> wrote:
[..]
> If we want to be more paranoid about buggy FW when we're comparing old
> and new tables, we could compare based on the length of the old and new
> tables since we have both pieces of information.  That would let you catch
> the case where a table size changes during a hotplug event or whenever the
> _FIT is processed.  Since you were comparing based on structure size instead
> of header length, I didn't change that.

Comparing that the size didn't change sounds like a good incremental
change to tag on later.

Re: [PATCH 1/3] nfit: Account for table size length variation

[Verma, Vishal L]

On Tue, 2015-11-24 at 11:24 -0500, Linda Knippers wrote:
> 
> Actually, the spec is pretty clear in this case.  If you look at the
> length definition for that table (5-133) it says:
> 
> 	Length in bytes for entire structure.
> 	The length of this structure is either 32 bytes or 80 bytes. The
> 	length of the structure can be 32 bytes only if the Number of
> 	Block Control Windows field has a value of 0.
> 
> The structure is 80 bytes but it is legal to have a 32-byte table.
> We hit a similar problem with the original NFIT code.  We could
> explicitly check for a size of 32 but we didn't before.

Thanks, I missed that. No objections from me any more :)

> 
> > If we make add_tables process only header.length and accept the
> > shortened table, there is nothing to tell future code that the
> > structure
> > that piece of memory is casted to is a truncated one.
> > 
> > Thoughts?
> 
> If we want to be more paranoid about buggy FW when we're comparing old
> and new tables, we could compare based on the length of the old and
> new
> tables since we have both pieces of information.  That would let you
> catch
> the case where a table size changes during a hotplug event or whenever
> the
> _FIT is processed.  Since you were comparing based on structure size
> instead
> of header length, I didn't change that.

Agreed this can be incremental work.

> 
> -- ljk
> > 
> > 	-Vishal
> > 
> 

Re: [PATCH 3/3] nfit: Adjust for different _FIT and NFIT headers

[Verma, Vishal L]

On Tue, 2015-11-24 at 11:31 -0500, Linda Knippers wrote:
> 
> I would really like to see this series go in soon because since the
> hotplug code went in I can't run an upstream kernel on my hardware.
> 
Agreed - I'm guessing Dan will queue this for the next -rc pull request.
Thanks for the fixes!