From: Heiko Carstens <hca@linux.ibm.com> To: Anshuman Khandual <anshuman.khandual@arm.com> Cc: linux-mm@kvack.org, akpm@linux-foundation.org, david@redhat.com, catalin.marinas@arm.com, linux-arm-kernel@lists.infradead.org, linux-s390@vger.kernel.org, linux-kernel@vger.kernel.org, Vasily Gorbik <gor@linux.ibm.com>, Will Deacon <will@kernel.org>, Ard Biesheuvel <ardb@kernel.org>, Mark Rutland <mark.rutland@arm.com> Subject: Re: [PATCH 3/3] s390/mm: Define arch_get_mappable_range() Date: Thu, 10 Dec 2020 07:58:45 +0100 [thread overview] Message-ID: <20201210065845.GA20691@osiris> (raw) In-Reply-To: <04da0f9c-d50e-7729-5e4c-b0dc4e76d608@arm.com> On Thu, Dec 10, 2020 at 09:48:11AM +0530, Anshuman Khandual wrote: > >> Alternatively leaving __segment_load() and vmem_add_memory() unchanged > >> will create three range checks i.e two memhp_range_allowed() and the > >> existing VMEM_MAX_PHYS check in vmem_add_mapping() on all the hotplug > >> paths, which is not optimal. > > > > Ah, sorry. I didn't follow this discussion too closely. I just thought > > my point of view would be clear: let's not have two different ways to > > check for the same thing which must be kept in sync. > > Therefore I was wondering why this next version is still doing > > that. Please find a way to solve this. > > The following change is after the current series and should work with > and without memory hotplug enabled. There will be just a single place > i.e vmem_get_max_addr() to update in case the maximum address changes > from VMEM_MAX_PHYS to something else later. Still not. That's way too much code churn for what you want to achieve. If the s390 specific patch would look like below you can add Acked-by: Heiko Carstens <hca@linux.ibm.com> But please make sure that the arch_get_mappable_range() prototype in linux/memory_hotplug.h is always visible and does not depend on CONFIG_MEMORY_HOTPLUG. I'd like to avoid seeing sparse warnings because of this. Thanks. diff --git a/arch/s390/mm/init.c b/arch/s390/mm/init.c index 77767850d0d0..e0e78234ae57 100644 --- a/arch/s390/mm/init.c +++ b/arch/s390/mm/init.c @@ -291,6 +291,7 @@ int arch_add_memory(int nid, u64 start, u64 size, if (WARN_ON_ONCE(params->pgprot.pgprot != PAGE_KERNEL.pgprot)) return -EINVAL; + VM_BUG_ON(!memhp_range_allowed(start, size, 1)); rc = vmem_add_mapping(start, size); if (rc) return rc; diff --git a/arch/s390/mm/vmem.c b/arch/s390/mm/vmem.c index b239f2ba93b0..ccd55e2f97f9 100644 --- a/arch/s390/mm/vmem.c +++ b/arch/s390/mm/vmem.c @@ -4,6 +4,7 @@ * Author(s): Heiko Carstens <heiko.carstens@de.ibm.com> */ +#include <linux/memory_hotplug.h> #include <linux/memblock.h> #include <linux/pfn.h> #include <linux/mm.h> @@ -532,11 +533,23 @@ void vmem_remove_mapping(unsigned long start, unsigned long size) mutex_unlock(&vmem_mutex); } +struct range arch_get_mappable_range(void) +{ + struct range range; + + range.start = 0; + range.end = VMEM_MAX_PHYS; + return range; +} + int vmem_add_mapping(unsigned long start, unsigned long size) { + struct range range; int ret; - if (start + size > VMEM_MAX_PHYS || + range = arch_get_mappable_range(); + if (start < range.start || + start + size > range.end || start + size < start) return -ERANGE;
WARNING: multiple messages have this Message-ID (diff)
From: Heiko Carstens <hca@linux.ibm.com> To: Anshuman Khandual <anshuman.khandual@arm.com> Cc: Mark Rutland <mark.rutland@arm.com>, linux-s390@vger.kernel.org, Vasily Gorbik <gor@linux.ibm.com>, david@redhat.com, catalin.marinas@arm.com, linux-kernel@vger.kernel.org, linux-mm@kvack.org, akpm@linux-foundation.org, Will Deacon <will@kernel.org>, Ard Biesheuvel <ardb@kernel.org>, linux-arm-kernel@lists.infradead.org Subject: Re: [PATCH 3/3] s390/mm: Define arch_get_mappable_range() Date: Thu, 10 Dec 2020 07:58:45 +0100 [thread overview] Message-ID: <20201210065845.GA20691@osiris> (raw) In-Reply-To: <04da0f9c-d50e-7729-5e4c-b0dc4e76d608@arm.com> On Thu, Dec 10, 2020 at 09:48:11AM +0530, Anshuman Khandual wrote: > >> Alternatively leaving __segment_load() and vmem_add_memory() unchanged > >> will create three range checks i.e two memhp_range_allowed() and the > >> existing VMEM_MAX_PHYS check in vmem_add_mapping() on all the hotplug > >> paths, which is not optimal. > > > > Ah, sorry. I didn't follow this discussion too closely. I just thought > > my point of view would be clear: let's not have two different ways to > > check for the same thing which must be kept in sync. > > Therefore I was wondering why this next version is still doing > > that. Please find a way to solve this. > > The following change is after the current series and should work with > and without memory hotplug enabled. There will be just a single place > i.e vmem_get_max_addr() to update in case the maximum address changes > from VMEM_MAX_PHYS to something else later. Still not. That's way too much code churn for what you want to achieve. If the s390 specific patch would look like below you can add Acked-by: Heiko Carstens <hca@linux.ibm.com> But please make sure that the arch_get_mappable_range() prototype in linux/memory_hotplug.h is always visible and does not depend on CONFIG_MEMORY_HOTPLUG. I'd like to avoid seeing sparse warnings because of this. Thanks. diff --git a/arch/s390/mm/init.c b/arch/s390/mm/init.c index 77767850d0d0..e0e78234ae57 100644 --- a/arch/s390/mm/init.c +++ b/arch/s390/mm/init.c @@ -291,6 +291,7 @@ int arch_add_memory(int nid, u64 start, u64 size, if (WARN_ON_ONCE(params->pgprot.pgprot != PAGE_KERNEL.pgprot)) return -EINVAL; + VM_BUG_ON(!memhp_range_allowed(start, size, 1)); rc = vmem_add_mapping(start, size); if (rc) return rc; diff --git a/arch/s390/mm/vmem.c b/arch/s390/mm/vmem.c index b239f2ba93b0..ccd55e2f97f9 100644 --- a/arch/s390/mm/vmem.c +++ b/arch/s390/mm/vmem.c @@ -4,6 +4,7 @@ * Author(s): Heiko Carstens <heiko.carstens@de.ibm.com> */ +#include <linux/memory_hotplug.h> #include <linux/memblock.h> #include <linux/pfn.h> #include <linux/mm.h> @@ -532,11 +533,23 @@ void vmem_remove_mapping(unsigned long start, unsigned long size) mutex_unlock(&vmem_mutex); } +struct range arch_get_mappable_range(void) +{ + struct range range; + + range.start = 0; + range.end = VMEM_MAX_PHYS; + return range; +} + int vmem_add_mapping(unsigned long start, unsigned long size) { + struct range range; int ret; - if (start + size > VMEM_MAX_PHYS || + range = arch_get_mappable_range(); + if (start < range.start || + start + size > range.end || start + size < start) return -ERANGE; _______________________________________________ linux-arm-kernel mailing list linux-arm-kernel@lists.infradead.org http://lists.infradead.org/mailman/listinfo/linux-arm-kernel
next prev parent reply other threads:[~2020-12-10 6:59 UTC|newest] Thread overview: 32+ messages / expand[flat|nested] mbox.gz Atom feed top 2020-12-08 4:16 [PATCH 0/3] mm/hotplug: Pre-validate the address range with platform Anshuman Khandual 2020-12-08 4:16 ` Anshuman Khandual 2020-12-08 4:16 ` [PATCH 1/3] mm/hotplug: Prevalidate the address range being added " Anshuman Khandual 2020-12-08 4:16 ` Anshuman Khandual 2020-12-08 4:16 ` [PATCH 2/3] arm64/mm: Define arch_get_mappable_range() Anshuman Khandual 2020-12-08 4:16 ` Anshuman Khandual 2020-12-08 4:16 ` [PATCH 3/3] s390/mm: " Anshuman Khandual 2020-12-08 4:16 ` Anshuman Khandual 2020-12-08 15:27 ` Heiko Carstens 2020-12-08 15:27 ` Heiko Carstens 2020-12-09 2:37 ` Anshuman Khandual 2020-12-09 2:37 ` Anshuman Khandual 2020-12-09 14:57 ` Heiko Carstens 2020-12-09 14:57 ` Heiko Carstens 2020-12-10 4:18 ` Anshuman Khandual 2020-12-10 4:18 ` Anshuman Khandual 2020-12-10 6:58 ` Heiko Carstens [this message] 2020-12-10 6:58 ` Heiko Carstens 2020-12-10 7:04 ` David Hildenbrand 2020-12-10 7:04 ` David Hildenbrand 2020-12-10 7:40 ` Anshuman Khandual 2020-12-10 7:40 ` Anshuman Khandual 2020-12-10 8:02 ` David Hildenbrand 2020-12-10 8:02 ` David Hildenbrand 2020-12-10 8:58 ` Anshuman Khandual 2020-12-10 8:58 ` Anshuman Khandual 2020-12-10 9:39 ` David Hildenbrand 2020-12-10 9:39 ` David Hildenbrand 2020-12-17 11:45 ` Anshuman Khandual 2020-12-17 11:45 ` Anshuman Khandual 2020-12-17 12:18 ` David Hildenbrand 2020-12-17 12:18 ` David Hildenbrand
Reply instructions: You may reply publicly to this message via plain-text email using any one of the following methods: * Save the following mbox file, import it into your mail client, and reply-to-all from there: mbox Avoid top-posting and favor interleaved quoting: https://en.wikipedia.org/wiki/Posting_style#Interleaved_style * Reply using the --to, --cc, and --in-reply-to switches of git-send-email(1): git send-email \ --in-reply-to=20201210065845.GA20691@osiris \ --to=hca@linux.ibm.com \ --cc=akpm@linux-foundation.org \ --cc=anshuman.khandual@arm.com \ --cc=ardb@kernel.org \ --cc=catalin.marinas@arm.com \ --cc=david@redhat.com \ --cc=gor@linux.ibm.com \ --cc=linux-arm-kernel@lists.infradead.org \ --cc=linux-kernel@vger.kernel.org \ --cc=linux-mm@kvack.org \ --cc=linux-s390@vger.kernel.org \ --cc=mark.rutland@arm.com \ --cc=will@kernel.org \ /path/to/YOUR_REPLY https://kernel.org/pub/software/scm/git/docs/git-send-email.html * If your mail client supports setting the In-Reply-To header via mailto: links, try the mailto: linkBe sure your reply has a Subject: header at the top and a blank line before the message body.
This is an external index of several public inboxes, see mirroring instructions on how to clone and mirror all data and code used by this external index.