papr SCM driver fixes
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
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.
--- arch/powerpc/platforms/pseries/Kconfig | 3 +-- 1 file changed, 1 insertion(+), 2 deletions(-)
arch/powerpc/platforms/pseries/Kconfig +1 -2
@@ -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.
Signature · 1 line
2.17.2
Re: [1/7] powerpc/papr_scm: Use depend instead of select
Quoted 7 lines · Oliver O'Halloran
Making
PAPR_SCMselect LIBNVDIMM results in circular dependencies inKconfigwhen 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 <[email protected]>Signed-off-by Oliver O'Halloran <[email protected]>
Series applied to powerpc fixes, thanks.
https://git.kernel.org/powerpc/c/14ebfec0712f66a4ef037fb7ac0df6
cheers
PATCH 2/7 powerpc/papr_scm: Fix resource end address
In reply to Oliver O'HalloranFix 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.
--- arch/powerpc/platforms/pseries/papr_scm.c | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-)
arch/powerpc/platforms/pseries/papr_scm.c +1 -1
@@ -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 ;
Signature · 1 line
2.17.2
PATCH 3/7 powerpc/papr_scm: Update DT properties
In reply to Oliver O'HalloranThe 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.
--- arch/powerpc/platforms/pseries/papr_scm.c | 16 +++++++++++----- 1 file changed, 11 insertions(+), 5 deletions(-)
arch/powerpc/platforms/pseries/papr_scm.c +11 -5
@@ -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;
Signature · 1 line
2.17.2
PATCH 4/7 powerpc/papr_scm: Remove endian conversions
In reply to Oliver O'HalloranThe 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.
--- arch/powerpc/platforms/pseries/papr_scm.c | 6 +++--- 1 file changed, 3 insertions(+), 3 deletions(-)
arch/powerpc/platforms/pseries/papr_scm.c +3 -3
@@ -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 );
Signature · 1 line
2.17.2
PATCH 5/7 powerpc/papr_scm: Fix DIMM device registration race
In reply to Oliver O'HalloranWhen 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.
--- arch/powerpc/platforms/pseries/papr_scm.c | 3 +++ 1 file changed, 3 insertions(+)
arch/powerpc/platforms/pseries/papr_scm.c +3 -0
@@ -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));
Signature · 1 line
2.17.2
PATCH 6/7 powerpc/papr_scm: Use ibm,unit-guid as the iset cookie
In reply to Oliver O'HalloranThe 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.
--- arch/powerpc/platforms/pseries/papr_scm.c | 12 ++++++++++++ 1 file changed, 12 insertions(+)
arch/powerpc/platforms/pseries/papr_scm.c +12 -0
@@ -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;
Signature · 1 line
2.17.2
PATCH 7/7 powerpc/mm: Fallback to RAM if the altmap is unusable
In reply to Oliver O'HalloranThe "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.
---
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(-)
arch/powerpc/mm/init_64.c +16 -3
@@ -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 */
Signature · 1 line
2.17.2