papr SCM driver fixes

papr SCM driver fixes

[Oliver O'Halloran]

Various bug fixes for the papr_scm driver that were found while bringing
up the PowerVM implementation of the interface. There's a few minor bugs
there were a result of bugs in the original QEMU implementation, a few
due to the memory layouts being different and one due to a change to the
DT bindings.

Mostly self-contained to the driver with the exception of the last patch
which touches the powerpc specific parts of the vmemmap handling.

[PATCH 1/7] powerpc/papr_scm: Use depend instead of select

[Oliver O'Halloran]

Making PAPR_SCM select LIBNVDIMM results in circular dependencies in
Kconfig when another symbol depends on it. Fix this by replacing the
select with a depends.

Fixes: b5beae5e224f ("powerpc/pseries: Add driver for PAPR SCM regions")
Reported-by: Alastair D'Silva <alastair@d-silva.org>
Signed-off-by: Oliver O'Halloran <oohall@gmail.com>
---
 arch/powerpc/platforms/pseries/Kconfig | 3 +--
 1 file changed, 1 insertion(+), 2 deletions(-)

diff --git a/arch/powerpc/platforms/pseries/Kconfig b/arch/powerpc/platforms/pseries/Kconfig
index ed91c9aea78e..e31e397003b3 100644
--- a/arch/powerpc/platforms/pseries/Kconfig
+++ b/arch/powerpc/platforms/pseries/Kconfig
@@ -141,8 +141,7 @@ config IBMEBUS
 	  Bus device driver for GX bus based adapters.
 
 config PAPR_SCM
-	depends on PPC_PSERIES && MEMORY_HOTPLUG
-	select LIBNVDIMM
+	depends on PPC_PSERIES && MEMORY_HOTPLUG && LIBNVDIMM
 	tristate "Support for the PAPR Storage Class Memory interface"
 	help
 	  Enable access to hypervisor provided storage class memory.
-- 
2.17.2

[PATCH 2/7] powerpc/papr_scm: Fix resource end address

[Oliver O'Halloran]

Fix an off-by-one error in the memory resource range. This resource is
used to determine the address range of the memory to be hot-plugged as
ZONE_DEVICE memory. The current end address results in the kernel
attempting to map an additional memblock and the hypervisor may reject
the mapping resulting in the entire hot-plug failing.

Fixes: b5beae5e224f ("powerpc/pseries: Add driver for PAPR SCM regions")
Signed-off-by: Oliver O'Halloran <oohall@gmail.com>
---
 arch/powerpc/platforms/pseries/papr_scm.c | 2 +-
 1 file changed, 1 insertion(+), 1 deletion(-)

diff --git a/arch/powerpc/platforms/pseries/papr_scm.c b/arch/powerpc/platforms/pseries/papr_scm.c
index ee9372b65ca5..390badd33547 100644
--- a/arch/powerpc/platforms/pseries/papr_scm.c
+++ b/arch/powerpc/platforms/pseries/papr_scm.c
@@ -296,7 +296,7 @@ static int papr_scm_probe(struct platform_device *pdev)
 
 	/* setup the resource for the newly bound range */
 	p->res.start = p->bound_addr;
-	p->res.end   = p->bound_addr + p->blocks * p->block_size;
+	p->res.end   = p->bound_addr + p->blocks * p->block_size - 1;
 	p->res.name  = pdev->name;
 	p->res.flags = IORESOURCE_MEM;
 
-- 
2.17.2

[PATCH 3/7] powerpc/papr_scm: Update DT properties

[Oliver O'Halloran]

The ibm,unit-sizes property was originally specified as an array of two
u32s corresponding to the memory block size, and the number of blocks
available in that region. A fairly last-minute change to the SCM DT
specification was splitting that into two seperate u64 properties:
ibm,block-sizes and ibm,number-of-blocks that convey the same
information. No firmware / hypervisor that emitted the ibm,unit-size
property ever appeared in the wild.

Fixes: b5beae5e224f ("powerpc/pseries: Add driver for PAPR SCM regions")
Signed-off-by: Oliver O'Halloran <oohall@gmail.com>
---
 arch/powerpc/platforms/pseries/papr_scm.c | 16 +++++++++++-----
 1 file changed, 11 insertions(+), 5 deletions(-)

diff --git a/arch/powerpc/platforms/pseries/papr_scm.c b/arch/powerpc/platforms/pseries/papr_scm.c
index 390badd33547..ed1082cc1d27 100644
--- a/arch/powerpc/platforms/pseries/papr_scm.c
+++ b/arch/powerpc/platforms/pseries/papr_scm.c
@@ -257,8 +257,9 @@ err:	nvdimm_bus_unregister(p->bus);
 
 static int papr_scm_probe(struct platform_device *pdev)
 {
-	uint32_t drc_index, metadata_size, unit_cap[2];
 	struct device_node *dn = pdev->dev.of_node;
+	uint32_t drc_index, metadata_size;
+	uint64_t blocks, block_size;
 	struct papr_scm_priv *p;
 	int rc;
 
@@ -268,8 +269,13 @@ static int papr_scm_probe(struct platform_device *pdev)
 		return -ENODEV;
 	}
 
-	if (of_property_read_u32_array(dn, "ibm,unit-capacity", unit_cap, 2)) {
-		dev_err(&pdev->dev, "%pOF: missing unit-capacity!\n", dn);
+	if (of_property_read_u64(dn, "ibm,block-size", &block_size)) {
+		dev_err(&pdev->dev, "%pOF: missing block-size!\n", dn);
+		return -ENODEV;
+	}
+
+	if (of_property_read_u64(dn, "ibm,number-of-blocks", &blocks)) {
+		dev_err(&pdev->dev, "%pOF: missing number-of-blocks!\n", dn);
 		return -ENODEV;
 	}
 
@@ -282,8 +288,8 @@ static int papr_scm_probe(struct platform_device *pdev)
 
 	p->dn = dn;
 	p->drc_index = drc_index;
-	p->block_size = unit_cap[0];
-	p->blocks     = unit_cap[1];
+	p->block_size = block_size;
+	p->blocks = blocks;
 
 	/* might be zero */
 	p->metadata_size = metadata_size;
-- 
2.17.2

[PATCH 4/7] powerpc/papr_scm: Remove endian conversions

[Oliver O'Halloran]

The return values of a h-call are returned in the CPU registers and
written to the provided buffer by the plpar_hcall() wrapper. As a result
the values written to memory are always in the native endian and should
not be byte swapped.

The inital implementation of the H-Call interface was done in qemu and
the returned values were byte swapped unnecessarily in both the
hypervisor and in the driver so this was only noticed when bringing up
the PowerVM implementation.

Fixes: b5beae5e224f ("powerpc/pseries: Add driver for PAPR SCM regions")
Signed-off-by: Oliver O'Halloran <oohall@gmail.com>
---
 arch/powerpc/platforms/pseries/papr_scm.c | 6 +++---
 1 file changed, 3 insertions(+), 3 deletions(-)

diff --git a/arch/powerpc/platforms/pseries/papr_scm.c b/arch/powerpc/platforms/pseries/papr_scm.c
index ed1082cc1d27..90203039dee8 100644
--- a/arch/powerpc/platforms/pseries/papr_scm.c
+++ b/arch/powerpc/platforms/pseries/papr_scm.c
@@ -55,7 +55,7 @@ static int drc_pmem_bind(struct papr_scm_priv *p)
 	do {
 		rc = plpar_hcall(H_SCM_BIND_MEM, ret, p->drc_index, 0,
 				p->blocks, BIND_ANY_ADDR, token);
-		token = be64_to_cpu(ret[0]);
+		token = ret[0];
 		cond_resched();
 	} while (rc == H_BUSY);
 
@@ -64,7 +64,7 @@ static int drc_pmem_bind(struct papr_scm_priv *p)
 		return -ENXIO;
 	}
 
-	p->bound_addr = be64_to_cpu(ret[1]);
+	p->bound_addr = ret[1];
 
 	dev_dbg(&p->pdev->dev, "bound drc %x to %pR\n", p->drc_index, &p->res);
 
@@ -82,7 +82,7 @@ static int drc_pmem_unbind(struct papr_scm_priv *p)
 	do {
 		rc = plpar_hcall(H_SCM_UNBIND_MEM, ret, p->drc_index,
 				p->bound_addr, p->blocks, token);
-		token = be64_to_cpu(ret);
+		token = ret[0];
 		cond_resched();
 	} while (rc == H_BUSY);
 
-- 
2.17.2

[PATCH 5/7] powerpc/papr_scm: Fix DIMM device registration race

[Oliver O'Halloran]

When a new nvdimm device is registered with libnvdimm via
nvdimm_create() it is added as a device on the nvdimm bus. The probe
function for the DIMM driver is potentially quite slow so actually
registering and probing the device is done in an async domain rather
than immediately after device creation. This can result in a race where
the region device (created 2nd) is probed first and fails to activate at
boot.

To fix this we use the same approach as the ACPI/NFIT driver which is to
check that all the DIMM devices registered successfully. LibNVDIMM
provides the nvdimm_bus_count_dimms() function which synchronises with
the async domain and verifies that the dimm was successfully registered
with the bus.

If either of these does not occur then we bail.

Fixes: b5beae5e224f ("powerpc/pseries: Add driver for PAPR SCM regions")
Signed-off-by: Oliver O'Halloran <oohall@gmail.com>
---
 arch/powerpc/platforms/pseries/papr_scm.c | 3 +++
 1 file changed, 3 insertions(+)

diff --git a/arch/powerpc/platforms/pseries/papr_scm.c b/arch/powerpc/platforms/pseries/papr_scm.c
index 90203039dee8..871b386dd4b2 100644
--- a/arch/powerpc/platforms/pseries/papr_scm.c
+++ b/arch/powerpc/platforms/pseries/papr_scm.c
@@ -223,6 +223,9 @@ static int papr_scm_nvdimm_init(struct papr_scm_priv *p)
 		goto err;
 	}
 
+	if (nvdimm_bus_check_dimm_count(p->bus, 1))
+		goto err;
+
 	/* now add the region */
 
 	memset(&mapping, 0, sizeof(mapping));
-- 
2.17.2

[PATCH 6/7] powerpc/papr_scm: Use ibm,unit-guid as the iset cookie

[Oliver O'Halloran]

The interleave set cookie is used to determine if a label stored in the
metadata space should be applied to the current region. This is
important in the case of NVDIMMs since the firmware may change the
interleaving configuration of a DIMM which would invalidate the existing
labels. In our case the hypervisor hides those details from us so we
don't really care, but libnvdimm still requires the interleave set
cookie to be non-zero.

For our purposes we just need the set cookie to be unique and fixed for
a given PAPR SCM region and using the unit-guid (really a UUID) is fine
for this purpose.

Fixes: b5beae5e224f ("powerpc/pseries: Add driver for PAPR SCM regions")
Signed-off-by: Oliver O'Halloran <oohall@gmail.com>
---
 arch/powerpc/platforms/pseries/papr_scm.c | 12 ++++++++++++
 1 file changed, 12 insertions(+)

diff --git a/arch/powerpc/platforms/pseries/papr_scm.c b/arch/powerpc/platforms/pseries/papr_scm.c
index 871b386dd4b2..92f76c656f85 100644
--- a/arch/powerpc/platforms/pseries/papr_scm.c
+++ b/arch/powerpc/platforms/pseries/papr_scm.c
@@ -264,6 +264,8 @@ static int papr_scm_probe(struct platform_device *pdev)
 	uint32_t drc_index, metadata_size;
 	uint64_t blocks, block_size;
 	struct papr_scm_priv *p;
+	const char *uuid_str;
+	uint64_t uuid[2];
 	int rc;
 
 	/* check we have all the required DT properties */
@@ -282,6 +284,11 @@ static int papr_scm_probe(struct platform_device *pdev)
 		return -ENODEV;
 	}
 
+	if (of_property_read_string(dn, "ibm,unit-guid", &uuid_str)) {
+		dev_err(&pdev->dev, "%pOF: missing unit-guid!\n", dn);
+		return -ENODEV;
+	}
+
 	p = kzalloc(sizeof(*p), GFP_KERNEL);
 	if (!p)
 		return -ENOMEM;
@@ -294,6 +301,11 @@ static int papr_scm_probe(struct platform_device *pdev)
 	p->block_size = block_size;
 	p->blocks = blocks;
 
+	/* We just need to ensure that set cookies are unique across */
+	uuid_parse(uuid_str, (uuid_t *) uuid);
+	p->nd_set.cookie1 = uuid[0];
+	p->nd_set.cookie2 = uuid[1];
+
 	/* might be zero */
 	p->metadata_size = metadata_size;
 	p->pdev = pdev;
-- 
2.17.2

[PATCH 7/7] powerpc/mm: Fallback to RAM if the altmap is unusable

[Oliver O'Halloran]

The "altmap" is used to provide a pool of memory that is reserved for
the vmemmap backing of hot-plugged memory. This is useful when adding
large amount of ZONE_DEVICE memory to a system with a limited amount of
normal memory.

On ppc64 we use huge pages to map the vmemmap which requires the backing
storage to be contigious and aligned to the hugepage size. The altmap
implementation allows for the altmap provider to reserve a few PFNs at
the start of the range for it's own uses and when this occurs the
first chunk of the altmap is not usage for hugepage mappings. On hash
there is no sane way to fall back to a normal sized page mapping so we
fail the allocation. This results in memory hotplug failing with
ENOMEM when the new range doesn't fall into an existing vmemmap block.

This patch handles this case by falling back to using system memory
rather than failing if we cannot allocate from the altmap. This
fallback should only ever be used for the first vmemmap block so it
should not cause excess memory consumption.

Fixes: 7b73d978a5d0 ("mm: pass the vmem_altmap to vmemmap_populate")
Signed-off-by: Oliver O'Halloran <oohall@gmail.com>
---
The Fixes here is a little dubious since the bug was present when altmap
support for powerpc was first added in b584c2544041 ("powerpc/vmemmap:
Add altmap support"). However, the rework in 7b73d978a5d0 ("mm: pass the
vmem_altmap to vmemmap_populate") moved the altmap allocation out of
generic code and into arch code so a fix for b584c2544041 would
be a completely different patch.
---
 arch/powerpc/mm/init_64.c | 19 ++++++++++++++++---
 1 file changed, 16 insertions(+), 3 deletions(-)

diff --git a/arch/powerpc/mm/init_64.c b/arch/powerpc/mm/init_64.c
index be6e964625da..960f2a44c525 100644
--- a/arch/powerpc/mm/init_64.c
+++ b/arch/powerpc/mm/init_64.c
@@ -173,15 +173,20 @@ int __meminit vmemmap_populate(unsigned long start, unsigned long end, int node,
 	pr_debug("vmemmap_populate %lx..%lx, node %d\n", start, end, node);
 
 	for (; start < end; start += page_size) {
-		void *p;
+		void *p = NULL;
 		int rc;
 
 		if (vmemmap_populated(start, page_size))
 			continue;
 
+		/*
+		 * Allocate from the altmap first if we have one. This may
+		 * fail due to alignment issues when using 16MB hugepages, so
+		 * fall back to system memory if the altmap allocation fail.
+		 */
 		if (altmap)
 			p = altmap_alloc_block_buf(page_size, altmap);
-		else
+		if (!p)
 			p = vmemmap_alloc_block_buf(page_size, node);
 		if (!p)
 			return -ENOMEM;
@@ -241,8 +246,15 @@ void __ref vmemmap_free(unsigned long start, unsigned long end,
 {
 	unsigned long page_size = 1 << mmu_psize_defs[mmu_vmemmap_psize].shift;
 	unsigned long page_order = get_order(page_size);
+	unsigned long alt_start = ~0, alt_end = ~0;
+	unsigned long base_pfn;
 
 	start = _ALIGN_DOWN(start, page_size);
+	if (altmap) {
+		alt_start = altmap->base_pfn;
+		alt_end = altmap->base_pfn + altmap->reserve +
+			  altmap->free + altmap->alloc + altmap->align;
+	}
 
 	pr_debug("vmemmap_free %lx...%lx\n", start, end);
 
@@ -266,8 +278,9 @@ void __ref vmemmap_free(unsigned long start, unsigned long end,
 		page = pfn_to_page(addr >> PAGE_SHIFT);
 		section_base = pfn_to_page(vmemmap_section_start(start));
 		nr_pages = 1 << page_order;
+		base_pfn = PHYS_PFN(addr);
 
-		if (altmap) {
+		if (base_pfn >= alt_start && base_pfn < alt_end) {
 			vmem_altmap_free(altmap, nr_pages);
 		} else if (PageReserved(page)) {
 			/* allocated from bootmem */
-- 
2.17.2

Re: [1/7] powerpc/papr_scm: Use depend instead of select

[Michael Ellerman]

On Thu, 2018-12-06 at 15:17:08 UTC, Oliver O'Halloran wrote:
> Making PAPR_SCM select LIBNVDIMM results in circular dependencies in
> Kconfig when another symbol depends on it. Fix this by replacing the
> select with a depends.
> 
> Fixes: b5beae5e224f ("powerpc/pseries: Add driver for PAPR SCM regions")
> Reported-by: Alastair D'Silva <alastair@d-silva.org>
> Signed-off-by: Oliver O'Halloran <oohall@gmail.com>

Series applied to powerpc fixes, thanks.

https://git.kernel.org/powerpc/c/14ebfec0712f66a4ef037fb7ac0df6

cheers