Fix various issues in new hmm driver

My build coverage for non-modules adding dead modular code tripped on this recently new addition. After removing that and rebuilding, I saw the driver still had warnings that were independent of the "demodular" change, which the 2nd commit fixes.

Then there was "select" usage that was bound to cause troubles because it was selecting an option with explicit dependencies. We decouple by adding an ARCH_HAS so the selects are valid while keeping the depends listed in a single place. We do this so we can update the parent depends properly (and only once) in 4/4.

Build tested for x86-64, arm64 and s390. Other 64 bit arch don't build and hence appear to be not applicable, as per details in patch 4/4.

---

Paul Gortmaker (4):
mm/hmm: make it explicitly non-modular
mm/hmm: fix warnings and SECTION_SIZE definition overlap mm/hmm: fix Kconfig to have valid usage of "select" mm/hmm: exclude 64 bit arch that explicitly fail to work.

 mm/Kconfig | 23 +++++++++--------------
 mm/hmm.c   | 37 +++++++++++++++----------------------
 2 files changed, 24 insertions(+), 36 deletions(-)

2.11.0

The Kconfig currently controlling compilation of this code is:

mm/Kconfig:config HMM
mm/Kconfig: bool

...meaning that it currently is not being built as a module by anyone.

Lets remove the modular code that is essentially orphaned, so that when reading the driver there is no doubt it is builtin-only.

Since module_init translates to device_initcall in the non-modular case, the init ordering remains unchanged with this commit.

We also delete the MODULE_LICENSE tag etc. since all that information is already contained at the top of the file in the comments.

---
 mm/hmm.c | 13 ++-----------
 1 file changed, 2 insertions(+), 11 deletions(-)

index f567a8b3b079..8200cf275fb9 100644
@@ -19,12 +19,12 @@
  */
 #include <linux/mm.h>
 #include <linux/hmm.h>
+#include <linux/init.h>
 #include <linux/rmap.h>
 #include <linux/swap.h>
 #include <linux/slab.h>
 #include <linux/sched.h>
 #include <linux/mmzone.h>
-#include <linux/module.h>
 #include <linux/pagemap.h>
 #include <linux/swapops.h>
 #include <linux/hugetlb.h>

@@ -1192,14 +1192,5 @@ static int __init hmm_init(void)
 	}
 	return 0;
 }
-
-static void __exit hmm_exit(void)
-{
-	unregister_chrdev_region(hmm_device_devt, HMM_DEVICE_MAX);
-	class_destroy(hmm_device_class);
-}
-
-module_init(hmm_init);
-module_exit(hmm_exit);
-MODULE_LICENSE("GPL");
+device_initcall(hmm_init);
 #endif /* IS_ENABLED(CONFIG_HMM_DEVMEM) */

2.11.0

SECTION_SIZE is defined twice in the same file; also it is already used by arch specific headers, which leads to:

mm/hmm.c:34:0: warning: "SECTION_SIZE" redefined [enabled by default]
#define SECTION_SIZE (1UL << PA_SECTION_SHIFT)
^
arch/arm64/include/asm/pgtable-hwdef.h:87:0: note: this is the location of the previous definition
#define SECTION_SIZE (_AC(1, UL) << SECTION_SHIFT)

Replace instances of SECTION_SIZE with PA_SECTION_SIZE to avoid all conflict and delete the 2nd instance of the definition in the file.

Also we see this warning:

mm/hmm.c: In function ‘hmm_devmem_release’:
mm/hmm.c:816:2: error: implicit declaration of function ‘arch_remove_memory’ [-Werror=implicit-function-declaration]
arch_remove_memory(align_start, align_size, devmem->pagemap.type);

... which has two factors: (1) there is an implicit expectation of the memory hotplug header being present, and (2) the arch_remove_memory() support depends on CONFIG_MEMORY_HOTREMOVE. So we fix that as well.

---
 mm/hmm.c | 24 +++++++++++++-----------
 1 file changed, 13 insertions(+), 11 deletions(-)

index 8200cf275fb9..080d71e403e7 100644
@@ -30,8 +30,9 @@
 #include <linux/hugetlb.h>
 #include <linux/memremap.h>
 #include <linux/mmu_notifier.h>
+#include <linux/memory_hotplug.h>
 
-#define SECTION_SIZE (1UL << PA_SECTION_SHIFT)
+#define PA_SECTION_SIZE (1UL << PA_SECTION_SHIFT)
 
 static const struct mmu_notifier_ops hmm_mmu_notifier_ops;
 

@@ -781,18 +782,17 @@ static void hmm_devmem_free(struct page *page, void *data)
 
 static DEFINE_MUTEX(hmm_devmem_lock);
 static RADIX_TREE(hmm_devmem_radix, GFP_KERNEL);
-#define SECTION_SIZE (1UL << PA_SECTION_SHIFT)
 
 static void hmm_devmem_radix_release(struct resource *resource)
 {
 	resource_size_t key, align_start, align_size, align_end;
 
-	align_start = resource->start & ~(SECTION_SIZE - 1);
-	align_size = ALIGN(resource_size(resource), SECTION_SIZE);
+	align_start = resource->start & ~(PA_SECTION_SIZE - 1);
+	align_size = ALIGN(resource_size(resource), PA_SECTION_SIZE);
 	align_end = align_start + align_size - 1;
 
 	mutex_lock(&hmm_devmem_lock);
-	for (key = resource->start; key <= resource->end; key += SECTION_SIZE)
+	for (key = resource->start; key <= resource->end; key += PA_SECTION_SIZE)
 		radix_tree_delete(&hmm_devmem_radix, key >> PA_SECTION_SHIFT);
 	mutex_unlock(&hmm_devmem_lock);
 }

@@ -809,12 +809,14 @@ static void hmm_devmem_release(struct device *dev, void *data)
 	}
 
 	/* pages are dead and unused, undo the arch mapping */
-	align_start = resource->start & ~(SECTION_SIZE - 1);
-	align_size = ALIGN(resource_size(resource), SECTION_SIZE);
+	align_start = resource->start & ~(PA_SECTION_SIZE - 1);
+	align_size = ALIGN(resource_size(resource), PA_SECTION_SIZE);
 
+#ifdef CONFIG_MEMORY_HOTREMOVE
 	mem_hotplug_begin();
 	arch_remove_memory(align_start, align_size, devmem->pagemap.type);
 	mem_hotplug_done();
+#endif
 
 	untrack_pfn(NULL, PHYS_PFN(align_start), align_size);
 	hmm_devmem_radix_release(resource);

@@ -835,10 +837,10 @@ static int hmm_devmem_pages_create(struct hmm_devmem *devmem)
 	int ret, nid, is_ram;
 	unsigned long pfn;
 
-	align_start = devmem->resource->start & ~(SECTION_SIZE - 1);
+	align_start = devmem->resource->start & ~(PA_SECTION_SIZE - 1);
 	align_size = ALIGN(devmem->resource->start +
 			   resource_size(devmem->resource),
-			   SECTION_SIZE) - align_start;
+			   PA_SECTION_SIZE) - align_start;
 
 	is_ram = region_intersects(align_start, align_size,
 				   IORESOURCE_SYSTEM_RAM,

@@ -861,7 +863,7 @@ static int hmm_devmem_pages_create(struct hmm_devmem *devmem)
 
 	mutex_lock(&hmm_devmem_lock);
 	align_end = align_start + align_size - 1;
-	for (key = align_start; key <= align_end; key += SECTION_SIZE) {
+	for (key = align_start; key <= align_end; key += PA_SECTION_SIZE) {
 		struct hmm_devmem *dup;
 
 		rcu_read_lock();

@@ -979,7 +981,7 @@ struct hmm_devmem *hmm_devmem_add(const struct hmm_devmem_ops *ops,
 	if (ret)
 		goto error_devm_add_action;
 
-	size = ALIGN(size, SECTION_SIZE);
+	size = ALIGN(size, PA_SECTION_SIZE);
 	addr = (iomem_resource.end + 1ULL) - size;
 
 	/*

2.11.0

The use of "select" like this will most likely cause new randconfig warnings and failures because the child categories get chosen, and force select the parent, even though the parent dependencies are not met. For further details, see "select" in Documentation/kbuild/kconfig-language.txt file.

But we also don't want to be duplicating the main feature dependencies across all the child options, which is annoying when the dependencies change. So we use the typical ARCH_HAS as a dependency collector, and the existing HMM option is now dependency free, and hence a valid option for use with "select". It retains its original purpose which is to be set if and only if any of the child options are set.

Finally, the existing help text is currently orphaned, since the HMM bool doesn't have a following short description, and hence it will never be offered to the end user. So delete it, but ensure the two child options point to the Documentation/vm/hmm.txt file.

---
 mm/Kconfig | 22 ++++++++--------------
 1 file changed, 8 insertions(+), 14 deletions(-)

index 43d000e44424..3e9c31bf9aaa 100644
@@ -289,23 +289,17 @@ config MIGRATION
 config ARCH_ENABLE_HUGEPAGE_MIGRATION
 	bool
 
-config HMM
+config ARCH_HAS_HMM
 	bool
+	default y
 	depends on MMU && 64BIT
-	help
-	  HMM provides a set of helpers to share a virtual address
-	  space between CPU and a device, so that the device can access any valid
-	  address of the process (while still obeying memory protection). HMM also
-	  provides helpers to migrate process memory to device memory, and back.
-	  Each set of functionality (address space mirroring, and migration to and
-	  from device memory) can be used independently of the other.
 
-	  This is primarily useful for devices like GPU, for GPGPU compute workload,
-	  with APIs such as OpenCL or CUDA. See Documentation/vm/hmm.txt.
+config HMM
+	bool
 
 config HMM_MIRROR
 	bool "HMM mirror CPU page table into a device page table"
-	depends on MMU && 64BIT
+	depends on ARCH_HAS_HMM
 	select HMM
 	select MMU_NOTIFIER
 	help

@@ -313,16 +307,16 @@ config HMM_MIRROR
 	  process into a device page table. Here, mirror means "keep synchronized".
 	  Prerequisites: the device must provide the ability to write-protect its
 	  page tables (at PAGE_SIZE granularity), and must be able to recover from
-	  the resulting potential page faults.
+	  the resulting potential page faults.  See Documentation/vm/hmm.txt.
 
 config HMM_DEVMEM
 	bool "HMM device memory helpers (to leverage ZONE_DEVICE)"
-	depends on MMU && 64BIT
+	depends on ARCH_HAS_HMM
 	select HMM
 	help
 	  HMM devmem is a set of helper routines to leverage the ZONE_DEVICE
 	  feature. This is just to avoid having device drivers to replicating a lot
-	  of boiler plate code.
+	  of boiler plate code.  See Documentation/vm/hmm.txt.
 
 config PHYS_ADDR_T_64BIT
 	def_bool 64BIT || ARCH_PHYS_ADDR_T_64BIT

2.11.0

On sparc64, we see:

mm/hmm.c: In function 'hmm_vma_walk_pmd':
mm/hmm.c:371:53: error: macro "pte_index" requires 2 arguments, but only 1 given mm/hmm.c:371:39: error: 'pte_index' undeclared (first use in this function)

...and on MIPS 64, we see:

mm/hmm.c:57:22: error: field 'mmu_notifier' has incomplete type
mm/hmm.c: In function 'hmm_register':
mm/hmm.c:98:2: error: implicit declaration of function '__mmu_notifier_register' [-Werror=implicit-function-declaration] mm/hmm.c:111:3: error: implicit declaration of function 'mmu_notifier_unregister' [-Werror=implicit-function-declaration]

...and on Alpha, we see:

mm/hmm.c: In function 'hmm_vma_walk_pmd':
mm/hmm.c:371:4: error: implicit declaration of function 'pmd_pfn' mm/hmm.c:371:4: error: implicit declaration of function 'pte_index'

...and on PaRISC 64 we see:

include/linux/hmm.h:405:7: warning: 'struct migrate_vma_ops' declared inside parameter list include/linux/hmm.h:405:7: warning: its scope is only this definition or declaration, which is probably not what you want mm/hmm.c: In function 'hmm_vma_walk_pmd':
mm/hmm.c:371:4: error: implicit declaration of function 'pmd_pfn'
[...]

Set the dependency to the three arch that currently seem to build without issue -- ARM_64, X86_64 and S390.

Since ia64 and ppc64 don't set CONFIG_64BIT, they were already excluded by the original dependency.

The failing arch can be re-added as demand dictates, if/when they have been validated to build and function.

---
 mm/Kconfig | 1 +
 1 file changed, 1 insertion(+)

index 3e9c31bf9aaa..6c27d2003a63 100644
@@ -293,6 +293,7 @@ config ARCH_HAS_HMM
 	bool
 	default y
 	depends on MMU && 64BIT
+	depends on ARM64 || X86_64 || S390
 
 config HMM
 	bool

2.11.0

Hi Paul,

My powerpc ppc64_defconfig builds have CONFIG_64BIT set ...

$ grep CONFIG_64BIT ~/next/powerpc_ppc64_defconfig/.config
CONFIG_64BIT=y

Cheers,
Stephen Rothwell

I must have fat fingered the grep; I was using the PA Semi defconfig since I knew that was 64 bit. I probably searched for 64_BIT or something stupid. The underscore ARM64 vs. X86_64 always gets me.

In the end, it doesn't change the commit itself, since the driver still only builds on X86_64, S390 and ARM64(but only after my patch). The ppc64 was also never compile tested it seems.

I can tweak the commit log of this patch in a v2 once there has been a chance for others to put in their feedback as well.

Here is the spew I got when I tried to compile hmm on next/master ppc64.

Paul.
--

CC mm/hmm.o
mm/hmm.c: In function 'hmm_devmem_radix_release':
mm/hmm.c:784:30: error: 'PA_SECTION_SHIFT' undeclared (first use in this function)

 #define SECTION_SIZE (1UL << PA_SECTION_SHIFT)
                              ^
mm/hmm.c:790:36: note: in expansion of macro 'SECTION_SIZE'
  align_start = resource->start & ~(SECTION_SIZE - 1);
                                    ^
mm/hmm.c:784:30: note: each undeclared identifier is reported only once for each function it appears in
 #define SECTION_SIZE (1UL << PA_SECTION_SHIFT)
                              ^
mm/hmm.c:790:36: note: in expansion of macro 'SECTION_SIZE'
  align_start = resource->start & ~(SECTION_SIZE - 1);
                                    ^
mm/hmm.c: In function 'hmm_devmem_release':
mm/hmm.c:784:30: error: 'PA_SECTION_SHIFT' undeclared (first use in this function)
 #define SECTION_SIZE (1UL << PA_SECTION_SHIFT)
                              ^
mm/hmm.c:812:36: note: in expansion of macro 'SECTION_SIZE'
  align_start = resource->start & ~(SECTION_SIZE - 1);
                                    ^

mm/hmm.c:816:2: error: implicit declaration of function 'arch_remove_memory' [-Werror=implicit-function-declaration]
arch_remove_memory(align_start, align_size, devmem->pagemap.type);
^
mm/hmm.c: In function 'hmm_devmem_find':
mm/hmm.c:827:54: error: 'PA_SECTION_SHIFT' undeclared (first use in this function)

  return radix_tree_lookup(&hmm_devmem_radix, phys >> PA_SECTION_SHIFT);
                                                      ^
mm/hmm.c: In function 'hmm_devmem_pages_create':
mm/hmm.c:784:30: error: 'PA_SECTION_SHIFT' undeclared (first use in this function)
 #define SECTION_SIZE (1UL << PA_SECTION_SHIFT)
                              ^
mm/hmm.c:838:44: note: in expansion of macro 'SECTION_SIZE'
  align_start = devmem->resource->start & ~(SECTION_SIZE - 1);
                                            ^
In file included from include/linux/cache.h:4:0,
                 from include/linux/printk.h:8,
                 from include/linux/kernel.h:13,
                 from include/asm-generic/bug.h:15,
                 from arch/powerpc/include/asm/bug.h:127,
                 from include/linux/bug.h:4,
                 from include/linux/mmdebug.h:4,
                 from include/linux/mm.h:8,
                 from mm/hmm.c:20:
mm/hmm.c: In function 'hmm_devmem_add':
mm/hmm.c:784:30: error: 'PA_SECTION_SHIFT' undeclared (first use in this function)
 #define SECTION_SIZE (1UL << PA_SECTION_SHIFT)
                              ^
include/uapi/linux/kernel.h:10:47: note: in definition of macro '__ALIGN_KERNEL_MASK'
 #define __ALIGN_KERNEL_MASK(x, mask) (((x) + (mask)) & ~(mask))
                                               ^
include/linux/kernel.h:49:22: note: in expansion of macro '__ALIGN_KERNEL'
 #define ALIGN(x, a)  __ALIGN_KERNEL((x), (a))
                      ^
mm/hmm.c:982:9: note: in expansion of macro 'ALIGN'
  size = ALIGN(size, SECTION_SIZE);
         ^
mm/hmm.c:982:21: note: in expansion of macro 'SECTION_SIZE'
  size = ALIGN(size, SECTION_SIZE);
                     ^

mm/hmm.c: In function 'hmm_devmem_find':
mm/hmm.c:828:1: warning: control reaches end of non-void function [-Wreturn-type]
}
^
cc1: some warnings being treated as errors scripts/Makefile.build:294: recipe for target 'mm/hmm.o' failed make[2]: *** [mm/hmm.o] Error 1
make[2]: *** Waiting for unfinished jobs....
Makefile:1663: recipe for target 'mm/' failed
make[1]: *** [mm/] Error 2
make[1]: Leaving directory '/home/paul/git/ppc-build'
Makefile:152: recipe for target 'sub-make' failed
make: *** [sub-make] Error 2

Yeah, arch/powerpc/Kconfig:

config 64BIT
bool
default y if PPC64

cheers

Yep, as I said to Stephen earlier in this thread, I think I mistakenly searched for 64_BIT; unfortunately I can't tell since all history has is:

16 make O=../ppc-build/ pasemi_defconfig
17 vi ../ppc-build/.config

Anyway, that mis-documentation in the commit log aside, it seems like the driver has been pulled from the linux-next content for today; presumably to be reworked and resubmitted with better arch coverage to address all the issues raised here. That is probably the right approach - we don't need to pollute git history with all that..

Thanks,
Paul.
--