| lists.openwall.net | lists / announce owl-users owl-dev john-users john-dev passwdqc-users yescrypt popa3d-users / oss-security kernel-hardening musl sabotage tlsify passwords / crypt-dev xvendor / Bugtraq Full-Disclosure linux-kernel linux-netdev linux-ext4 linux-hardening linux-cve-announce PHC | |
|
Open Source and information security mailing list archives
| ||
|
Message-ID: <9aeed880-c200-a070-a7a4-212ee38c15ed@nvidia.com>
Date: Wed, 14 Jun 2017 16:10:32 -0700
From: John Hubbard <jhubbard@...dia.com>
To: Jérôme Glisse <jglisse@...hat.com>,
<linux-kernel@...r.kernel.org>, <linux-mm@...ck.org>
CC: David Nellans <dnellans@...dia.com>,
Balbir Singh <balbirs@....ibm.com>,
Aneesh Kumar <aneesh.kumar@...ux.vnet.ibm.com>,
"Paul E . McKenney" <paulmck@...ux.vnet.ibm.com>,
Benjamin Herrenschmidt <benh@...nel.crashing.org>
Subject: Re: [HMM-CDM 5/5] mm/hmm: simplify kconfig and enable HMM and
DEVICE_PUBLIC for ppc64
On 06/14/2017 01:11 PM, Jérôme Glisse wrote:
> This just simplify kconfig and allow HMM and DEVICE_PUBLIC to be
> selected for ppc64 once ZONE_DEVICE is allowed on ppc64 (different
> patchset).
>
> Signed-off-by: Jérôme Glisse <jglisse@...hat.com>
> Signed-off-by: John Hubbard <jhubbard@...dia.com>
> Cc: Balbir Singh <balbirs@....ibm.com>
> Cc: Aneesh Kumar <aneesh.kumar@...ux.vnet.ibm.com>
> Cc: Paul E. McKenney <paulmck@...ux.vnet.ibm.com>
> Cc: Benjamin Herrenschmidt <benh@...nel.crashing.org>
> ---
> include/linux/hmm.h | 4 ++--
> mm/Kconfig | 27 ++++++---------------------
> mm/hmm.c | 4 ++--
> 3 files changed, 10 insertions(+), 25 deletions(-)
>
> diff --git a/include/linux/hmm.h b/include/linux/hmm.h
> index f6713b2..720d18c 100644
> --- a/include/linux/hmm.h
> +++ b/include/linux/hmm.h
> @@ -327,7 +327,7 @@ int hmm_vma_fault(struct vm_area_struct *vma,
> #endif /* IS_ENABLED(CONFIG_HMM_MIRROR) */
>
>
> -#if IS_ENABLED(CONFIG_HMM_DEVMEM)
> +#if IS_ENABLED(CONFIG_DEVICE_PRIVATE) || IS_ENABLED(CONFIG_DEVICE_PUBLIC)
> struct hmm_devmem;
>
> struct page *hmm_vma_alloc_locked_page(struct vm_area_struct *vma,
> @@ -456,7 +456,7 @@ struct hmm_device {
> */
> struct hmm_device *hmm_device_new(void *drvdata);
> void hmm_device_put(struct hmm_device *hmm_device);
> -#endif /* IS_ENABLED(CONFIG_HMM_DEVMEM) */
> +#endif /* CONFIG_DEVICE_PRIVATE || CONFIG_DEVICE_PUBLIC */
>
>
> /* Below are for HMM internal use only! Not to be used by device driver! */
> diff --git a/mm/Kconfig b/mm/Kconfig
> index ad082b9..7de939a 100644
> --- a/mm/Kconfig
> +++ b/mm/Kconfig
> @@ -265,7 +265,7 @@ config ARCH_ENABLE_HUGEPAGE_MIGRATION
> config ARCH_HAS_HMM
> bool
> default y
> - depends on X86_64
> + depends on X86_64 || PPC64
> depends on ZONE_DEVICE
> depends on MMU && 64BIT
> depends on MEMORY_HOTPLUG
> @@ -277,7 +277,7 @@ config HMM
>
> config HMM_MIRROR
> bool "HMM mirror CPU page table into a device page table"
> - depends on ARCH_HAS_HMM
> + depends on ARCH_HAS_HMM && X86_64
> select MMU_NOTIFIER
> select HMM
> help
Hi Jerome,
There are still some problems with using this configuration. First and foremost, it is still
possible (and likely, given the complete dissimilarity in naming, and difference in location on the
screen) to choose HMM_MIRROR, and *not* to choose either DEVICE_PRIVATE or DEVICE_PUBLIC. And then
we end up with a swath of important page fault handling code being ifdef'd out, and one ends up
having to investigate why.
As for solutions, at least for the x86 (DEVICE_PRIVATE)case, we could do this:
diff --git a/mm/Kconfig b/mm/Kconfig
index 7de939a29466..f64182d7b956 100644
--- a/mm/Kconfig
+++ b/mm/Kconfig
@@ -279,6 +279,7 @@ config HMM_MIRROR
bool "HMM mirror CPU page table into a device page table"
depends on ARCH_HAS_HMM && X86_64
select MMU_NOTIFIER
+ select DEVICE_PRIVATE
select HMM
help
Select HMM_MIRROR if you want to mirror range of the CPU page table of a
...and that is better than the other direction (having HMM_MIRROR depend on DEVICE_PRIVATE), because
in the latter case, HMM_MIRROR will disappear (and it's several lines above) until you select
DEVICE_PRIVATE. That is hard to work with for the user.
The user will tend to select HMM_MIRROR, but it is *not* obvious that he/she should also select
DEVICE_PRIVATE. So Kconfig should do it for them.
In fact, I'm not even sure if the DEVICE_PRIVATE and DEVICE_PUBLIC actually need Kconfig protection,
but if they don't, then life would be easier for whoever is configuring their kernel.
> @@ -287,15 +287,6 @@ config HMM_MIRROR
> page tables (at PAGE_SIZE granularity), and must be able to recover from
> the resulting potential page faults.
>
> -config HMM_DEVMEM
> - bool "HMM device memory helpers (to leverage ZONE_DEVICE)"
> - 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. See Documentation/vm/hmm.txt.
> -
Yes, probably good to remove HMM_DEVMEM as a separate conig choice.
> config PHYS_ADDR_T_64BIT
> def_bool 64BIT || ARCH_PHYS_ADDR_T_64BIT
>
> @@ -720,11 +711,8 @@ config ZONE_DEVICE
>
> config DEVICE_PRIVATE
> bool "Unaddressable device memory (GPU memory, ...)"
> - depends on X86_64
> - depends on ZONE_DEVICE
> - depends on MEMORY_HOTPLUG
> - depends on MEMORY_HOTREMOVE
> - depends on SPARSEMEM_VMEMMAP
> + depends on ARCH_HAS_HMM && X86_64
> + select HMM
>
> help
> Allows creation of struct pages to represent unaddressable device
> @@ -733,11 +721,8 @@ config DEVICE_PRIVATE
>
> config DEVICE_PUBLIC
> bool "Unaddressable device memory (GPU memory, ...)"
Typo: this is a copy-and-paste from DEVICE_PRIVATE, but the "Unaddressable" part wasn't changed, so
you'll end up with two identical-looking lines in `make menuconfig`.
Maybe "Directly addressable device memory"? And make the line less identical to DEVICE_PRIVATE?
thanks,
--
John Hubbard
NVIDIA
> - depends on X86_64
> - depends on ZONE_DEVICE
> - depends on MEMORY_HOTPLUG
> - depends on MEMORY_HOTREMOVE
> - depends on SPARSEMEM_VMEMMAP
> + depends on ARCH_HAS_HMM
> + select HMM
>
> help
> Allows creation of struct pages to represent addressable device
> diff --git a/mm/hmm.c b/mm/hmm.c
> index aed110e..085cc06 100644
> --- a/mm/hmm.c
> +++ b/mm/hmm.c
> @@ -747,7 +747,7 @@ EXPORT_SYMBOL(hmm_vma_fault);
> #endif /* IS_ENABLED(CONFIG_HMM_MIRROR) */
>
>
> -#if IS_ENABLED(CONFIG_HMM_DEVMEM)
> +#if IS_ENABLED(CONFIG_DEVICE_PRIVATE) || IS_ENABLED(CONFIG_DEVICE_PUBLIC)
> struct page *hmm_vma_alloc_locked_page(struct vm_area_struct *vma,
> unsigned long addr)
> {
> @@ -1306,4 +1306,4 @@ static int __init hmm_init(void)
> }
>
> device_initcall(hmm_init);
> -#endif /* IS_ENABLED(CONFIG_HMM_DEVMEM) */
> +#endif /* CONFIG_DEVICE_PRIVATE || CONFIG_DEVICE_PUBLIC */
> --
> 2.9.3
>
Powered by blists - more mailing lists