[RFC 0/3] mm/hotplug: Pre-validate the address range with platform

[RFC 0/3] mm/hotplug: Pre-validate the address range with platform

[Anshuman Khandual]

This series adds a mechanism allowing platforms to weigh in and prevalidate
incoming address range before proceeding further with the memory hotplug.
This helps prevent potential platform errors for the given address range,
down the hotplug call chain, which inevitably fails the hotplug itself.

This mechanism was suggested by David Hildenbrand during another discussion
with respect to a memory hotplug fix on arm64 platform.

https://lore.kernel.org/linux-arm-kernel/1600332402-30123-1-git-send-email-anshuman.khandual@arm.com/

This mechanism focuses on the addressibility aspect and not [sub] section
alignment aspect. Hence check_hotplug_memory_range() and check_pfn_span()
have been left unchanged. Wondering if all these can still be unified in
an expanded memhp_range_allowed() check, that can be called from multiple
memory hot add and remove paths.

This series applies on v5.10-rc5 and has been slightly tested on arm64.
But looking for some early feedback here.

Cc: Heiko Carstens <hca@linux.ibm.com>
Cc: Vasily Gorbik <gor@linux.ibm.com>
Cc: Catalin Marinas <catalin.marinas@arm.com>
Cc: Will Deacon <will@kernel.org>
Cc: Ard Biesheuvel <ardb@kernel.org>
Cc: Mark Rutland <mark.rutland@arm.com>
Cc: David Hildenbrand <david@redhat.com>
Cc: Andrew Morton <akpm@linux-foundation.org>
Cc: linux-arm-kernel@lists.infradead.org
Cc: linux-s390@vger.kernel.org
Cc: linux-mm@kvack.org
Cc: linux-kernel@vger.kernel.org

Anshuman Khandual (3):
  mm/hotplug: Pre-validate the address range with platform
  arm64/mm: Define arch_get_addressable_range()
  s390/mm: Define arch_get_addressable_range()

 arch/arm64/include/asm/memory.h |  3 ++
 arch/arm64/mm/mmu.c             | 19 ++++++------
 arch/s390/include/asm/mmu.h     |  2 ++
 arch/s390/mm/vmem.c             | 16 ++++++++---
 include/linux/memory_hotplug.h  | 51 +++++++++++++++++++++++++++++++++
 mm/memory_hotplug.c             | 29 ++++++-------------
 mm/memremap.c                   |  9 +++++-
 7 files changed, 96 insertions(+), 33 deletions(-)

-- 
2.20.1

[RFC 0/3] mm/hotplug: Pre-validate the address range with platform

[Anshuman Khandual]

This series adds a mechanism allowing platforms to weigh in and prevalidate
incoming address range before proceeding further with the memory hotplug.
This helps prevent potential platform errors for the given address range,
down the hotplug call chain, which inevitably fails the hotplug itself.

This mechanism was suggested by David Hildenbrand during another discussion
with respect to a memory hotplug fix on arm64 platform.

https://lore.kernel.org/linux-arm-kernel/1600332402-30123-1-git-send-email-anshuman.khandual@arm.com/

This mechanism focuses on the addressibility aspect and not [sub] section
alignment aspect. Hence check_hotplug_memory_range() and check_pfn_span()
have been left unchanged. Wondering if all these can still be unified in
an expanded memhp_range_allowed() check, that can be called from multiple
memory hot add and remove paths.

This series applies on v5.10-rc5 and has been slightly tested on arm64.
But looking for some early feedback here.

Cc: Heiko Carstens <hca@linux.ibm.com>
Cc: Vasily Gorbik <gor@linux.ibm.com>
Cc: Catalin Marinas <catalin.marinas@arm.com>
Cc: Will Deacon <will@kernel.org>
Cc: Ard Biesheuvel <ardb@kernel.org>
Cc: Mark Rutland <mark.rutland@arm.com>
Cc: David Hildenbrand <david@redhat.com>
Cc: Andrew Morton <akpm@linux-foundation.org>
Cc: linux-arm-kernel@lists.infradead.org
Cc: linux-s390@vger.kernel.org
Cc: linux-mm@kvack.org
Cc: linux-kernel@vger.kernel.org

Anshuman Khandual (3):
  mm/hotplug: Pre-validate the address range with platform
  arm64/mm: Define arch_get_addressable_range()
  s390/mm: Define arch_get_addressable_range()

 arch/arm64/include/asm/memory.h |  3 ++
 arch/arm64/mm/mmu.c             | 19 ++++++------
 arch/s390/include/asm/mmu.h     |  2 ++
 arch/s390/mm/vmem.c             | 16 ++++++++---
 include/linux/memory_hotplug.h  | 51 +++++++++++++++++++++++++++++++++
 mm/memory_hotplug.c             | 29 ++++++-------------
 mm/memremap.c                   |  9 +++++-
 7 files changed, 96 insertions(+), 33 deletions(-)

-- 
2.20.1


_______________________________________________
linux-arm-kernel mailing list
linux-arm-kernel@lists.infradead.org
http://lists.infradead.org/mailman/listinfo/linux-arm-kernel

[RFC 1/3] mm/hotplug: Pre-validate the address range with platform

[Anshuman Khandual]

This introduces memhp_range_allowed() which gets called in various hotplug
paths. Then memhp_range_allowed() calls arch_get_addressable_range() via
memhp_get_pluggable_range(). This callback can be defined by the platform
to provide addressable physical range, depending on whether kernel linear
mapping is required or not. This mechanism will prevent platform specific
errors deep down during hotplug calls. While here, this drops now redundant
check_hotplug_memory_addressable() check in __add_pages().

Cc: David Hildenbrand <david@redhat.com>
Cc: Andrew Morton <akpm@linux-foundation.org>
Cc: linux-mm@kvack.org
Cc: linux-kernel@vger.kernel.org
Suggested-by: David Hildenbrand <david@redhat.com>
Signed-off-by: Anshuman Khandual <anshuman.khandual@arm.com>
---
 include/linux/memory_hotplug.h | 51 ++++++++++++++++++++++++++++++++++
 mm/memory_hotplug.c            | 29 ++++++-------------
 mm/memremap.c                  |  9 +++++-
 3 files changed, 68 insertions(+), 21 deletions(-)

diff --git a/include/linux/memory_hotplug.h b/include/linux/memory_hotplug.h
index 551093b74596..2018c0201672 100644
--- a/include/linux/memory_hotplug.h
+++ b/include/linux/memory_hotplug.h
@@ -6,6 +6,7 @@
 #include <linux/spinlock.h>
 #include <linux/notifier.h>
 #include <linux/bug.h>
+#include <linux/range.h>
 
 struct page;
 struct zone;
@@ -70,6 +71,56 @@ typedef int __bitwise mhp_t;
  */
 #define MEMHP_MERGE_RESOURCE	((__force mhp_t)BIT(0))
 
+/*
+ * Platforms should define arch_get_addressable_range() which provides
+ * addressable physical memory range depending upon whether the linear
+ * mapping is required or not. Returned address range must follow
+ *
+ * 1. range.start <= range.end
+ * 1. Must include end both points i.e range.start and range.end
+ *
+ * Nonetheless there is a fallback definition provided here providing
+ * maximum possible addressable physical range, irrespective of the
+ * linear mapping requirements.
+ */
+#ifndef arch_get_addressable_range
+static inline struct range arch_get_addressable_range(bool need_mapping)
+{
+	struct range memhp_range = {
+		.start = 0UL,
+		.end = -1ULL,
+	};
+	return memhp_range;
+}
+#endif
+
+static inline struct range memhp_get_pluggable_range(bool need_mapping)
+{
+	const u64 max_phys = (1ULL << (MAX_PHYSMEM_BITS + 1)) - 1;
+	struct range memhp_range = arch_get_addressable_range(need_mapping);
+
+	if (memhp_range.start > max_phys) {
+		memhp_range.start = 0;
+		memhp_range.end = 0;
+	}
+	memhp_range.end = min_t(u64, memhp_range.end, max_phys);
+	return memhp_range;
+}
+
+static inline bool memhp_range_allowed(u64 start, u64 size, bool need_mapping)
+{
+	struct range memhp_range = memhp_get_pluggable_range(need_mapping);
+	u64 end = start + size;
+
+	if ((start < end) && (start >= memhp_range.start) &&
+	   ((end - 1) <= memhp_range.end))
+		return true;
+
+	WARN(1, "Hotplug memory [%#llx-%#llx] exceeds maximum addressable range [%#llx-%#llx]\n",
+		start, end, memhp_range.start, memhp_range.end);
+	return false;
+}
+
 /*
  * Extended parameters for memory hotplug:
  * altmap: alternative allocator for memmap array (optional)
diff --git a/mm/memory_hotplug.c b/mm/memory_hotplug.c
index 63b2e46b6555..9efb6c8558ed 100644
--- a/mm/memory_hotplug.c
+++ b/mm/memory_hotplug.c
@@ -284,22 +284,6 @@ static int check_pfn_span(unsigned long pfn, unsigned long nr_pages,
 	return 0;
 }
 
-static int check_hotplug_memory_addressable(unsigned long pfn,
-					    unsigned long nr_pages)
-{
-	const u64 max_addr = PFN_PHYS(pfn + nr_pages) - 1;
-
-	if (max_addr >> MAX_PHYSMEM_BITS) {
-		const u64 max_allowed = (1ull << (MAX_PHYSMEM_BITS + 1)) - 1;
-		WARN(1,
-		     "Hotplugged memory exceeds maximum addressable address, range=%#llx-%#llx, maximum=%#llx\n",
-		     (u64)PFN_PHYS(pfn), max_addr, max_allowed);
-		return -E2BIG;
-	}
-
-	return 0;
-}
-
 /*
  * Reasonably generic function for adding memory.  It is
  * expected that archs that support memory hotplug will
@@ -317,10 +301,6 @@ int __ref __add_pages(int nid, unsigned long pfn, unsigned long nr_pages,
 	if (WARN_ON_ONCE(!params->pgprot.pgprot))
 		return -EINVAL;
 
-	err = check_hotplug_memory_addressable(pfn, nr_pages);
-	if (err)
-		return err;
-
 	if (altmap) {
 		/*
 		 * Validate altmap is within bounds of the total request
@@ -1109,6 +1089,9 @@ int __ref __add_memory(int nid, u64 start, u64 size, mhp_t mhp_flags)
 	struct resource *res;
 	int ret;
 
+	if (!memhp_range_allowed(start, size, 1))
+		return -ERANGE;
+
 	res = register_memory_resource(start, size, "System RAM");
 	if (IS_ERR(res))
 		return PTR_ERR(res);
@@ -1123,6 +1106,9 @@ int add_memory(int nid, u64 start, u64 size, mhp_t mhp_flags)
 {
 	int rc;
 
+	if (!memhp_range_allowed(start, size, 1))
+		return -ERANGE;
+
 	lock_device_hotplug();
 	rc = __add_memory(nid, start, size, mhp_flags);
 	unlock_device_hotplug();
@@ -1163,6 +1149,9 @@ int add_memory_driver_managed(int nid, u64 start, u64 size,
 	    resource_name[strlen(resource_name) - 1] != ')')
 		return -EINVAL;
 
+	if (!memhp_range_allowed(start, size, 0))
+		return -ERANGE;
+
 	lock_device_hotplug();
 
 	res = register_memory_resource(start, size, resource_name);
diff --git a/mm/memremap.c b/mm/memremap.c
index 16b2fb482da1..388a34b068c1 100644
--- a/mm/memremap.c
+++ b/mm/memremap.c
@@ -188,6 +188,7 @@ static int pagemap_range(struct dev_pagemap *pgmap, struct mhp_params *params,
 	struct range *range = &pgmap->ranges[range_id];
 	struct dev_pagemap *conflict_pgmap;
 	int error, is_ram;
+	bool is_private = false;
 
 	if (WARN_ONCE(pgmap_altmap(pgmap) && range_id > 0,
 				"altmap not supported for multiple ranges\n"))
@@ -207,6 +208,9 @@ static int pagemap_range(struct dev_pagemap *pgmap, struct mhp_params *params,
 		return -ENOMEM;
 	}
 
+	if (pgmap->type == MEMORY_DEVICE_PRIVATE)
+		is_private = true;
+
 	is_ram = region_intersects(range->start, range_len(range),
 		IORESOURCE_SYSTEM_RAM, IORES_DESC_NONE);
 
@@ -230,6 +234,9 @@ static int pagemap_range(struct dev_pagemap *pgmap, struct mhp_params *params,
 	if (error)
 		goto err_pfn_remap;
 
+	if (!memhp_range_allowed(range->start, range_len(range), !is_private))
+		goto err_pfn_remap;
+
 	mem_hotplug_begin();
 
 	/*
@@ -243,7 +250,7 @@ static int pagemap_range(struct dev_pagemap *pgmap, struct mhp_params *params,
 	 * the CPU, we do want the linear mapping and thus use
 	 * arch_add_memory().
 	 */
-	if (pgmap->type == MEMORY_DEVICE_PRIVATE) {
+	if (is_private) {
 		error = add_pages(nid, PHYS_PFN(range->start),
 				PHYS_PFN(range_len(range)), params);
 	} else {
-- 
2.20.1

Re: [RFC 1/3] mm/hotplug: Pre-validate the address range with platform

[David Hildenbrand]

On 23.11.20 03:28, Anshuman Khandual wrote:
> This introduces memhp_range_allowed() which gets called in various hotplug
> paths. Then memhp_range_allowed() calls arch_get_addressable_range() via
> memhp_get_pluggable_range(). This callback can be defined by the platform
> to provide addressable physical range, depending on whether kernel linear
> mapping is required or not. This mechanism will prevent platform specific
> errors deep down during hotplug calls. While here, this drops now redundant
> check_hotplug_memory_addressable() check in __add_pages().
> 
> Cc: David Hildenbrand <david@redhat.com>
> Cc: Andrew Morton <akpm@linux-foundation.org>
> Cc: linux-mm@kvack.org
> Cc: linux-kernel@vger.kernel.org
> Suggested-by: David Hildenbrand <david@redhat.com>
> Signed-off-by: Anshuman Khandual <anshuman.khandual@arm.com>
> ---
>  include/linux/memory_hotplug.h | 51 ++++++++++++++++++++++++++++++++++
>  mm/memory_hotplug.c            | 29 ++++++-------------
>  mm/memremap.c                  |  9 +++++-
>  3 files changed, 68 insertions(+), 21 deletions(-)
> 
> diff --git a/include/linux/memory_hotplug.h b/include/linux/memory_hotplug.h
> index 551093b74596..2018c0201672 100644
> --- a/include/linux/memory_hotplug.h
> +++ b/include/linux/memory_hotplug.h
> @@ -6,6 +6,7 @@
>  #include <linux/spinlock.h>
>  #include <linux/notifier.h>
>  #include <linux/bug.h>
> +#include <linux/range.h>
>  
>  struct page;
>  struct zone;
> @@ -70,6 +71,56 @@ typedef int __bitwise mhp_t;
>   */
>  #define MEMHP_MERGE_RESOURCE	((__force mhp_t)BIT(0))
>  
> +/*
> + * Platforms should define arch_get_addressable_range() which provides
> + * addressable physical memory range depending upon whether the linear
> + * mapping is required or not. Returned address range must follow
> + *
> + * 1. range.start <= range.end
> + * 1. Must include end both points i.e range.start and range.end
> + *
> + * Nonetheless there is a fallback definition provided here providing
> + * maximum possible addressable physical range, irrespective of the
> + * linear mapping requirements.
> + */
> +#ifndef arch_get_addressable_range
> +static inline struct range arch_get_addressable_range(bool need_mapping)

Why not simply

"arch_get_mappable_range(void)" (or similar) ?

AFAIKs, both implementations (arm64/s390x) simply do the exact same
thing as memhp_get_pluggable_range() for !need_mapping.

> +{
> +	struct range memhp_range = {
> +		.start = 0UL,
> +		.end = -1ULL,
> +	};
> +	return memhp_range;
> +}
> +#endif
> +
> +static inline struct range memhp_get_pluggable_range(bool need_mapping)
> +{
> +	const u64 max_phys = (1ULL << (MAX_PHYSMEM_BITS + 1)) - 1;
> +	struct range memhp_range = arch_get_addressable_range(need_mapping);
> +
> +	if (memhp_range.start > max_phys) {
> +		memhp_range.start = 0;
> +		memhp_range.end = 0;
> +	}
> +	memhp_range.end = min_t(u64, memhp_range.end, max_phys);
> +	return memhp_range;
> +}
> +
> +static inline bool memhp_range_allowed(u64 start, u64 size, bool need_mapping)
> +{
> +	struct range memhp_range = memhp_get_pluggable_range(need_mapping);
> +	u64 end = start + size;
> +
> +	if ((start < end) && (start >= memhp_range.start) &&
> +	   ((end - 1) <= memhp_range.end))

You can drop quite a lot of () here :)

> +		return true;
> +
> +	WARN(1, "Hotplug memory [%#llx-%#llx] exceeds maximum addressable range [%#llx-%#llx]\n",
> +		start, end, memhp_range.start, memhp_range.end);

pr_warn() (or even pr_warn_once())

while we're at it. No reason to eventually crash a system :)

> +	return false;
> +}
> +


I'd suggest moving these functions into mm/memory_hotplug.c and only
exposing what makes sense. These functions are not on any hot path. You
can then convert the arch_ function into a "__weak".

>  /*
>   * Extended parameters for memory hotplug:
>   * altmap: alternative allocator for memmap array (optional)
> diff --git a/mm/memory_hotplug.c b/mm/memory_hotplug.c
> index 63b2e46b6555..9efb6c8558ed 100644
> --- a/mm/memory_hotplug.c
> +++ b/mm/memory_hotplug.c
> @@ -284,22 +284,6 @@ static int check_pfn_span(unsigned long pfn, unsigned long nr_pages,
>  	return 0;
>  }
>  
> -static int check_hotplug_memory_addressable(unsigned long pfn,
> -					    unsigned long nr_pages)
> -{
> -	const u64 max_addr = PFN_PHYS(pfn + nr_pages) - 1;
> -
> -	if (max_addr >> MAX_PHYSMEM_BITS) {
> -		const u64 max_allowed = (1ull << (MAX_PHYSMEM_BITS + 1)) - 1;
> -		WARN(1,
> -		     "Hotplugged memory exceeds maximum addressable address, range=%#llx-%#llx, maximum=%#llx\n",
> -		     (u64)PFN_PHYS(pfn), max_addr, max_allowed);
> -		return -E2BIG;
> -	}
> -
> -	return 0;
> -}
> -
>  /*
>   * Reasonably generic function for adding memory.  It is
>   * expected that archs that support memory hotplug will
> @@ -317,10 +301,6 @@ int __ref __add_pages(int nid, unsigned long pfn, unsigned long nr_pages,
>  	if (WARN_ON_ONCE(!params->pgprot.pgprot))
>  		return -EINVAL;
>  
> -	err = check_hotplug_memory_addressable(pfn, nr_pages);
> -	if (err)
> -		return err;
> -
>  	if (altmap) {
>  		/*
>  		 * Validate altmap is within bounds of the total request
> @@ -1109,6 +1089,9 @@ int __ref __add_memory(int nid, u64 start, u64 size, mhp_t mhp_flags)
>  	struct resource *res;
>  	int ret;
>  
> +	if (!memhp_range_allowed(start, size, 1))
> +		return -ERANGE;

We used to return -E2BIG, no? Maybe better keep that.

> +
>  	res = register_memory_resource(start, size, "System RAM");
>  	if (IS_ERR(res))
>  		return PTR_ERR(res);
> @@ -1123,6 +1106,9 @@ int add_memory(int nid, u64 start, u64 size, mhp_t mhp_flags)
>  {
>  	int rc;
>  
> +	if (!memhp_range_allowed(start, size, 1))
> +		return -ERANGE;
> +
>  	lock_device_hotplug();
>  	rc = __add_memory(nid, start, size, mhp_flags);
>  	unlock_device_hotplug();
> @@ -1163,6 +1149,9 @@ int add_memory_driver_managed(int nid, u64 start, u64 size,
>  	    resource_name[strlen(resource_name) - 1] != ')')
>  		return -EINVAL;
>  
> +	if (!memhp_range_allowed(start, size, 0))
> +		return -ERANGE;
> +
>  	lock_device_hotplug();

For all 3 cases, doing a single check in register_memory_resource() is
sufficient.

>  
>  	res = register_memory_resource(start, size, resource_name);
> diff --git a/mm/memremap.c b/mm/memremap.c
> index 16b2fb482da1..388a34b068c1 100644
> --- a/mm/memremap.c
> +++ b/mm/memremap.c
> @@ -188,6 +188,7 @@ static int pagemap_range(struct dev_pagemap *pgmap, struct mhp_params *params,
>  	struct range *range = &pgmap->ranges[range_id];
>  	struct dev_pagemap *conflict_pgmap;
>  	int error, is_ram;
> +	bool is_private = false;

nit: Reverse christmas tree :)


const bool is_private = pgmap->type == MEMORY_DEVICE_PRIVATE;

>  
>  	if (WARN_ONCE(pgmap_altmap(pgmap) && range_id > 0,
>  				"altmap not supported for multiple ranges\n"))
> @@ -207,6 +208,9 @@ static int pagemap_range(struct dev_pagemap *pgmap, struct mhp_params *params,
>  		return -ENOMEM;
>  	}
>  
> +	if (pgmap->type == MEMORY_DEVICE_PRIVATE)
> +		is_private = true;
> +
>  	is_ram = region_intersects(range->start, range_len(range),
>  		IORESOURCE_SYSTEM_RAM, IORES_DESC_NONE);
>  
> @@ -230,6 +234,9 @@ static int pagemap_range(struct dev_pagemap *pgmap, struct mhp_params *params,
>  	if (error)
>  		goto err_pfn_remap;
>  
> +	if (!memhp_range_allowed(range->start, range_len(range), !is_private))
> +		goto err_pfn_remap;
> +
>  	mem_hotplug_begin();
>  
>  	/*
> @@ -243,7 +250,7 @@ static int pagemap_range(struct dev_pagemap *pgmap, struct mhp_params *params,
>  	 * the CPU, we do want the linear mapping and thus use
>  	 * arch_add_memory().
>  	 */
> -	if (pgmap->type == MEMORY_DEVICE_PRIVATE) {
> +	if (is_private) {
>  		error = add_pages(nid, PHYS_PFN(range->start),
>  				PHYS_PFN(range_len(range)), params);
>  	} else {
> 

Doing these checks in add_pages() / arch_add_memory() would be neater -
but as they we don't have clean generic wrappers yet, I consider this
good enough until we have reworked that part. (others might disagree :) )

-- 
Thanks,

David / dhildenb

Re: [RFC 1/3] mm/hotplug: Pre-validate the address range with platform

[Anshuman Khandual]

On 11/25/20 10:34 PM, David Hildenbrand wrote:
> On 23.11.20 03:28, Anshuman Khandual wrote:
>> This introduces memhp_range_allowed() which gets called in various hotplug
>> paths. Then memhp_range_allowed() calls arch_get_addressable_range() via
>> memhp_get_pluggable_range(). This callback can be defined by the platform
>> to provide addressable physical range, depending on whether kernel linear
>> mapping is required or not. This mechanism will prevent platform specific
>> errors deep down during hotplug calls. While here, this drops now redundant
>> check_hotplug_memory_addressable() check in __add_pages().
>>
>> Cc: David Hildenbrand <david@redhat.com>
>> Cc: Andrew Morton <akpm@linux-foundation.org>
>> Cc: linux-mm@kvack.org
>> Cc: linux-kernel@vger.kernel.org
>> Suggested-by: David Hildenbrand <david@redhat.com>
>> Signed-off-by: Anshuman Khandual <anshuman.khandual@arm.com>
>> ---
>>  include/linux/memory_hotplug.h | 51 ++++++++++++++++++++++++++++++++++
>>  mm/memory_hotplug.c            | 29 ++++++-------------
>>  mm/memremap.c                  |  9 +++++-
>>  3 files changed, 68 insertions(+), 21 deletions(-)
>>
>> diff --git a/include/linux/memory_hotplug.h b/include/linux/memory_hotplug.h
>> index 551093b74596..2018c0201672 100644
>> --- a/include/linux/memory_hotplug.h
>> +++ b/include/linux/memory_hotplug.h
>> @@ -6,6 +6,7 @@
>>  #include <linux/spinlock.h>
>>  #include <linux/notifier.h>
>>  #include <linux/bug.h>
>> +#include <linux/range.h>
>>  
>>  struct page;
>>  struct zone;
>> @@ -70,6 +71,56 @@ typedef int __bitwise mhp_t;
>>   */
>>  #define MEMHP_MERGE_RESOURCE	((__force mhp_t)BIT(0))
>>  
>> +/*
>> + * Platforms should define arch_get_addressable_range() which provides
>> + * addressable physical memory range depending upon whether the linear
>> + * mapping is required or not. Returned address range must follow
>> + *
>> + * 1. range.start <= range.end
>> + * 1. Must include end both points i.e range.start and range.end
>> + *
>> + * Nonetheless there is a fallback definition provided here providing
>> + * maximum possible addressable physical range, irrespective of the
>> + * linear mapping requirements.
>> + */
>> +#ifndef arch_get_addressable_range
>> +static inline struct range arch_get_addressable_range(bool need_mapping)
> 
> Why not simply
> 
> "arch_get_mappable_range(void)" (or similar) ?

The current name seems bit better (I guess). Because we are asking for
max addressable range with or without the linear mapping.

> 
> AFAIKs, both implementations (arm64/s390x) simply do the exact same
> thing as memhp_get_pluggable_range() for !need_mapping.

That is for now. Even the range without requiring linear mapping might not
be the same (like now) for every platform as some might have constraints.
So asking the platform ranges with or without linear mapping seems to be
better and could accommodate special cases going forward. Anyways, there
is an always an "all allowing" fallback option nonetheless.

> 
>> +{
>> +	struct range memhp_range = {
>> +		.start = 0UL,
>> +		.end = -1ULL,
>> +	};
>> +	return memhp_range;
>> +}
>> +#endif
>> +
>> +static inline struct range memhp_get_pluggable_range(bool need_mapping)
>> +{
>> +	const u64 max_phys = (1ULL << (MAX_PHYSMEM_BITS + 1)) - 1;
>> +	struct range memhp_range = arch_get_addressable_range(need_mapping);
>> +
>> +	if (memhp_range.start > max_phys) {
>> +		memhp_range.start = 0;
>> +		memhp_range.end = 0;
>> +	}
>> +	memhp_range.end = min_t(u64, memhp_range.end, max_phys);
>> +	return memhp_range;
>> +}
>> +
>> +static inline bool memhp_range_allowed(u64 start, u64 size, bool need_mapping)
>> +{
>> +	struct range memhp_range = memhp_get_pluggable_range(need_mapping);
>> +	u64 end = start + size;
>> +
>> +	if ((start < end) && (start >= memhp_range.start) &&
>> +	   ((end - 1) <= memhp_range.end))
> 
> You can drop quite a lot of () here :)

Will replace with.

if (start < end && start >= memhp_range.start && (end - 1) <= memhp_range.end)

But too much open comparisons looked bit risky initially :)

> 
>> +		return true;
>> +
>> +	WARN(1, "Hotplug memory [%#llx-%#llx] exceeds maximum addressable range [%#llx-%#llx]\n",
>> +		start, end, memhp_range.start, memhp_range.end);
> 
> pr_warn() (or even pr_warn_once())
> 
> while we're at it. No reason to eventually crash a system :)

Didn't quite get it. How could this crash the system ?

> 
>> +	return false;
>> +}
>> +
> 
> 
> I'd suggest moving these functions into mm/memory_hotplug.c and only
> exposing what makes sense. These functions are not on any hot path. You
> can then convert the arch_ function into a "__weak".

Sure, will move them to mm/memory_hotplug.c and change the arch
function into a '__weak'.

> 
>>  /*
>>   * Extended parameters for memory hotplug:
>>   * altmap: alternative allocator for memmap array (optional)
>> diff --git a/mm/memory_hotplug.c b/mm/memory_hotplug.c
>> index 63b2e46b6555..9efb6c8558ed 100644
>> --- a/mm/memory_hotplug.c
>> +++ b/mm/memory_hotplug.c
>> @@ -284,22 +284,6 @@ static int check_pfn_span(unsigned long pfn, unsigned long nr_pages,
>>  	return 0;
>>  }
>>  
>> -static int check_hotplug_memory_addressable(unsigned long pfn,
>> -					    unsigned long nr_pages)
>> -{
>> -	const u64 max_addr = PFN_PHYS(pfn + nr_pages) - 1;
>> -
>> -	if (max_addr >> MAX_PHYSMEM_BITS) {
>> -		const u64 max_allowed = (1ull << (MAX_PHYSMEM_BITS + 1)) - 1;
>> -		WARN(1,
>> -		     "Hotplugged memory exceeds maximum addressable address, range=%#llx-%#llx, maximum=%#llx\n",
>> -		     (u64)PFN_PHYS(pfn), max_addr, max_allowed);
>> -		return -E2BIG;
>> -	}
>> -
>> -	return 0;
>> -}
>> -
>>  /*
>>   * Reasonably generic function for adding memory.  It is
>>   * expected that archs that support memory hotplug will
>> @@ -317,10 +301,6 @@ int __ref __add_pages(int nid, unsigned long pfn, unsigned long nr_pages,
>>  	if (WARN_ON_ONCE(!params->pgprot.pgprot))
>>  		return -EINVAL;
>>  
>> -	err = check_hotplug_memory_addressable(pfn, nr_pages);
>> -	if (err)
>> -		return err;
>> -
>>  	if (altmap) {
>>  		/*
>>  		 * Validate altmap is within bounds of the total request
>> @@ -1109,6 +1089,9 @@ int __ref __add_memory(int nid, u64 start, u64 size, mhp_t mhp_flags)
>>  	struct resource *res;
>>  	int ret;
>>  
>> +	if (!memhp_range_allowed(start, size, 1))
>> +		return -ERANGE;
> 
> We used to return -E2BIG, no? Maybe better keep that.

ERANGE seems to be better as the range can overrun on either side.

> 
>> +
>>  	res = register_memory_resource(start, size, "System RAM");
>>  	if (IS_ERR(res))
>>  		return PTR_ERR(res);
>> @@ -1123,6 +1106,9 @@ int add_memory(int nid, u64 start, u64 size, mhp_t mhp_flags)
>>  {
>>  	int rc;
>>  
>> +	if (!memhp_range_allowed(start, size, 1))
>> +		return -ERANGE;
>> +
>>  	lock_device_hotplug();
>>  	rc = __add_memory(nid, start, size, mhp_flags);
>>  	unlock_device_hotplug();
>> @@ -1163,6 +1149,9 @@ int add_memory_driver_managed(int nid, u64 start, u64 size,
>>  	    resource_name[strlen(resource_name) - 1] != ')')
>>  		return -EINVAL;
>>  
>> +	if (!memhp_range_allowed(start, size, 0))
>> +		return -ERANGE;
>> +
>>  	lock_device_hotplug();
> 
> For all 3 cases, doing a single check in register_memory_resource() is
> sufficient.

Will replace with a single check in register_memory_resource(). But does
add_memory_driver_managed() always require linear mapping ? The proposed
check here did not ask for linear mapping in add_memory_driver_managed().

> 
>>  
>>  	res = register_memory_resource(start, size, resource_name);
>> diff --git a/mm/memremap.c b/mm/memremap.c
>> index 16b2fb482da1..388a34b068c1 100644
>> --- a/mm/memremap.c
>> +++ b/mm/memremap.c
>> @@ -188,6 +188,7 @@ static int pagemap_range(struct dev_pagemap *pgmap, struct mhp_params *params,
>>  	struct range *range = &pgmap->ranges[range_id];
>>  	struct dev_pagemap *conflict_pgmap;
>>  	int error, is_ram;
>> +	bool is_private = false;
> 
> nit: Reverse christmas tree :)
> 
> 
> const bool is_private = pgmap->type == MEMORY_DEVICE_PRIVATE;

Will replace with this right at the beginning.

> 
>>  
>>  	if (WARN_ONCE(pgmap_altmap(pgmap) && range_id > 0,
>>  				"altmap not supported for multiple ranges\n"))
>> @@ -207,6 +208,9 @@ static int pagemap_range(struct dev_pagemap *pgmap, struct mhp_params *params,
>>  		return -ENOMEM;
>>  	}
>>  
>> +	if (pgmap->type == MEMORY_DEVICE_PRIVATE)
>> +		is_private = true;
>> +
>>  	is_ram = region_intersects(range->start, range_len(range),
>>  		IORESOURCE_SYSTEM_RAM, IORES_DESC_NONE);
>>  
>> @@ -230,6 +234,9 @@ static int pagemap_range(struct dev_pagemap *pgmap, struct mhp_params *params,
>>  	if (error)
>>  		goto err_pfn_remap;
>>  
>> +	if (!memhp_range_allowed(range->start, range_len(range), !is_private))
>> +		goto err_pfn_remap;
>> +
>>  	mem_hotplug_begin();
>>  
>>  	/*
>> @@ -243,7 +250,7 @@ static int pagemap_range(struct dev_pagemap *pgmap, struct mhp_params *params,
>>  	 * the CPU, we do want the linear mapping and thus use
>>  	 * arch_add_memory().
>>  	 */
>> -	if (pgmap->type == MEMORY_DEVICE_PRIVATE) {
>> +	if (is_private) {
>>  		error = add_pages(nid, PHYS_PFN(range->start),
>>  				PHYS_PFN(range_len(range)), params);
>>  	} else {
>>
> 
> Doing these checks in add_pages() / arch_add_memory() would be neater -
> but as they we don't have clean generic wrappers yet, I consider this
> good enough until we have reworked that part. (others might disagree :) )

Sure, will leave the check as is.

Re: [RFC 1/3] mm/hotplug: Pre-validate the address range with platform

[David Hildenbrand]

>>
>> "arch_get_mappable_range(void)" (or similar) ?
> 
> The current name seems bit better (I guess). Because we are asking for
> max addressable range with or without the linear mapping.
> 
>>
>> AFAIKs, both implementations (arm64/s390x) simply do the exact same
>> thing as memhp_get_pluggable_range() for !need_mapping.
> 
> That is for now. Even the range without requiring linear mapping might not
> be the same (like now) for every platform as some might have constraints.
> So asking the platform ranges with or without linear mapping seems to be
> better and could accommodate special cases going forward. Anyways, there
> is an always an "all allowing" fallback option nonetheless.

Let's keep it simple as long as we don't have a real scenario where this
would apply.

[...]

> 
>>
>>> +		return true;
>>> +
>>> +	WARN(1, "Hotplug memory [%#llx-%#llx] exceeds maximum addressable range [%#llx-%#llx]\n",
>>> +		start, end, memhp_range.start, memhp_range.end);
>>
>> pr_warn() (or even pr_warn_once())
>>
>> while we're at it. No reason to eventually crash a system :)
> 
> Didn't quite get it. How could this crash the system ?

With panic_on_warn, which some distributions started to enable.

[...]

>>>  		/*
>>>  		 * Validate altmap is within bounds of the total request
>>> @@ -1109,6 +1089,9 @@ int __ref __add_memory(int nid, u64 start, u64 size, mhp_t mhp_flags)
>>>  	struct resource *res;
>>>  	int ret;
>>>  
>>> +	if (!memhp_range_allowed(start, size, 1))
>>> +		return -ERANGE;
>>
>> We used to return -E2BIG, no? Maybe better keep that.
> 
> ERANGE seems to be better as the range can overrun on either side.

Did you check all callers that they can handle it? Should mention that
in the patch description then.

> 
>>
>>> +
>>>  	res = register_memory_resource(start, size, "System RAM");
>>>  	if (IS_ERR(res))
>>>  		return PTR_ERR(res);
>>> @@ -1123,6 +1106,9 @@ int add_memory(int nid, u64 start, u64 size, mhp_t mhp_flags)
>>>  {
>>>  	int rc;
>>>  
>>> +	if (!memhp_range_allowed(start, size, 1))
>>> +		return -ERANGE;
>>> +
>>>  	lock_device_hotplug();
>>>  	rc = __add_memory(nid, start, size, mhp_flags);
>>>  	unlock_device_hotplug();
>>> @@ -1163,6 +1149,9 @@ int add_memory_driver_managed(int nid, u64 start, u64 size,
>>>  	    resource_name[strlen(resource_name) - 1] != ')')
>>>  		return -EINVAL;
>>>  
>>> +	if (!memhp_range_allowed(start, size, 0))
>>> +		return -ERANGE;
>>> +
>>>  	lock_device_hotplug();
>>
>> For all 3 cases, doing a single check in register_memory_resource() is
>> sufficient.
> 
> Will replace with a single check in register_memory_resource(). But does
> add_memory_driver_managed() always require linear mapping ? The proposed
> check here did not ask for linear mapping in add_memory_driver_managed().

Yes, in that regard, it behaves just like add_memory().



-- 
Thanks,

David / dhildenb

Re: [RFC 1/3] mm/hotplug: Pre-validate the address range with platform

[Anshuman Khandual]

On 11/27/20 2:31 PM, David Hildenbrand wrote:
>>>
>>> "arch_get_mappable_range(void)" (or similar) ?
>>
>> The current name seems bit better (I guess). Because we are asking for
>> max addressable range with or without the linear mapping.
>>
>>>
>>> AFAIKs, both implementations (arm64/s390x) simply do the exact same
>>> thing as memhp_get_pluggable_range() for !need_mapping.
>>
>> That is for now. Even the range without requiring linear mapping might not
>> be the same (like now) for every platform as some might have constraints.
>> So asking the platform ranges with or without linear mapping seems to be
>> better and could accommodate special cases going forward. Anyways, there
>> is an always an "all allowing" fallback option nonetheless.
> 
> Let's keep it simple as long as we don't have a real scenario where this
> would apply.

Sure, will have the arch callback only when the range needs linear mapping.
Otherwise, memhp_get_pluggable_range() can just fallback on [0...max_phys]
for non linear mapping requests.

> 
> [...]
> 
>>
>>>
>>>> +		return true;
>>>> +
>>>> +	WARN(1, "Hotplug memory [%#llx-%#llx] exceeds maximum addressable range [%#llx-%#llx]\n",
>>>> +		start, end, memhp_range.start, memhp_range.end);
>>>
>>> pr_warn() (or even pr_warn_once())
>>>
>>> while we're at it. No reason to eventually crash a system :)
>>
>> Didn't quite get it. How could this crash the system ?
> 
> With panic_on_warn, which some distributions started to enable.

Okay, got it.

> 
> [...]
> 
>>>>  		/*
>>>>  		 * Validate altmap is within bounds of the total request
>>>> @@ -1109,6 +1089,9 @@ int __ref __add_memory(int nid, u64 start, u64 size, mhp_t mhp_flags)
>>>>  	struct resource *res;
>>>>  	int ret;
>>>>  
>>>> +	if (!memhp_range_allowed(start, size, 1))
>>>> +		return -ERANGE;
>>>
>>> We used to return -E2BIG, no? Maybe better keep that.
>>
>> ERANGE seems to be better as the range can overrun on either side.
> 
> Did you check all callers that they can handle it? Should mention that
> in the patch description then.

Hmm, okay then. Lets keep -E2BIG to be less disruptive for the callers.

> 
>>
>>>
>>>> +
>>>>  	res = register_memory_resource(start, size, "System RAM");
>>>>  	if (IS_ERR(res))
>>>>  		return PTR_ERR(res);
>>>> @@ -1123,6 +1106,9 @@ int add_memory(int nid, u64 start, u64 size, mhp_t mhp_flags)
>>>>  {
>>>>  	int rc;
>>>>  
>>>> +	if (!memhp_range_allowed(start, size, 1))
>>>> +		return -ERANGE;
>>>> +
>>>>  	lock_device_hotplug();
>>>>  	rc = __add_memory(nid, start, size, mhp_flags);
>>>>  	unlock_device_hotplug();
>>>> @@ -1163,6 +1149,9 @@ int add_memory_driver_managed(int nid, u64 start, u64 size,
>>>>  	    resource_name[strlen(resource_name) - 1] != ')')
>>>>  		return -EINVAL;
>>>>  
>>>> +	if (!memhp_range_allowed(start, size, 0))
>>>> +		return -ERANGE;
>>>> +
>>>>  	lock_device_hotplug();
>>>
>>> For all 3 cases, doing a single check in register_memory_resource() is
>>> sufficient.
>>
>> Will replace with a single check in register_memory_resource(). But does
>> add_memory_driver_managed() always require linear mapping ? The proposed
>> check here did not ask for linear mapping in add_memory_driver_managed().
> 
> Yes, in that regard, it behaves just like add_memory().

Sure.

[RFC 2/3] arm64/mm: Define arch_get_addressable_range()

[Anshuman Khandual]

This overrides arch_get_addressable_range() on arm64 platform which will be
used with recently added generic framework. It drops inside_linear_region()
and subsequent check in arch_add_memory() which are no longer required.

Cc: Catalin Marinas <catalin.marinas@arm.com>
Cc: Will Deacon <will@kernel.org>
Cc: Ard Biesheuvel <ardb@kernel.org>
Cc: Mark Rutland <mark.rutland@arm.com>
Cc: David Hildenbrand <david@redhat.com>
Cc: linux-arm-kernel@lists.infradead.org
Cc: linux-kernel@vger.kernel.org
Signed-off-by: Anshuman Khandual <anshuman.khandual@arm.com>
---
 arch/arm64/include/asm/memory.h |  3 +++
 arch/arm64/mm/mmu.c             | 19 +++++++++++--------
 2 files changed, 14 insertions(+), 8 deletions(-)

diff --git a/arch/arm64/include/asm/memory.h b/arch/arm64/include/asm/memory.h
index cd61239bae8c..0ef7948eb58c 100644
--- a/arch/arm64/include/asm/memory.h
+++ b/arch/arm64/include/asm/memory.h
@@ -328,6 +328,9 @@ static inline void *phys_to_virt(phys_addr_t x)
 })
 
 void dump_mem_limit(void);
+
+#define arch_get_addressable_range arch_get_addressable_range
+struct range arch_get_addressable_range(bool need_mapping);
 #endif /* !ASSEMBLY */
 
 /*
diff --git a/arch/arm64/mm/mmu.c b/arch/arm64/mm/mmu.c
index ca692a815731..a6433caf337f 100644
--- a/arch/arm64/mm/mmu.c
+++ b/arch/arm64/mm/mmu.c
@@ -1444,16 +1444,24 @@ static void __remove_pgd_mapping(pgd_t *pgdir, unsigned long start, u64 size)
 	free_empty_tables(start, end, PAGE_OFFSET, PAGE_END);
 }
 
-static bool inside_linear_region(u64 start, u64 size)
+struct range arch_get_addressable_range(bool need_mapping)
 {
+	struct range memhp_range;
+
 	/*
 	 * Linear mapping region is the range [PAGE_OFFSET..(PAGE_END - 1)]
 	 * accommodating both its ends but excluding PAGE_END. Max physical
 	 * range which can be mapped inside this linear mapping range, must
 	 * also be derived from its end points.
 	 */
-	return start >= __pa(_PAGE_OFFSET(vabits_actual)) &&
-	       (start + size - 1) <= __pa(PAGE_END - 1);
+	if (need_mapping) {
+		memhp_range.start = __pa(_PAGE_OFFSET(vabits_actual));
+		memhp_range.end =  __pa(PAGE_END - 1);
+	} else {
+		memhp_range.start = 0;
+		memhp_range.end = (1ULL << (MAX_PHYSMEM_BITS + 1)) - 1;
+	}
+	return memhp_range;
 }
 
 int arch_add_memory(int nid, u64 start, u64 size,
@@ -1461,11 +1469,6 @@ int arch_add_memory(int nid, u64 start, u64 size,
 {
 	int ret, flags = 0;
 
-	if (!inside_linear_region(start, size)) {
-		pr_err("[%llx %llx] is outside linear mapping region\n", start, start + size);
-		return -EINVAL;
-	}
-
 	if (rodata_full || debug_pagealloc_enabled())
 		flags = NO_BLOCK_MAPPINGS | NO_CONT_MAPPINGS;
 
-- 
2.20.1

[RFC 2/3] arm64/mm: Define arch_get_addressable_range()

[Anshuman Khandual]

This overrides arch_get_addressable_range() on arm64 platform which will be
used with recently added generic framework. It drops inside_linear_region()
and subsequent check in arch_add_memory() which are no longer required.

Cc: Catalin Marinas <catalin.marinas@arm.com>
Cc: Will Deacon <will@kernel.org>
Cc: Ard Biesheuvel <ardb@kernel.org>
Cc: Mark Rutland <mark.rutland@arm.com>
Cc: David Hildenbrand <david@redhat.com>
Cc: linux-arm-kernel@lists.infradead.org
Cc: linux-kernel@vger.kernel.org
Signed-off-by: Anshuman Khandual <anshuman.khandual@arm.com>
---
 arch/arm64/include/asm/memory.h |  3 +++
 arch/arm64/mm/mmu.c             | 19 +++++++++++--------
 2 files changed, 14 insertions(+), 8 deletions(-)

diff --git a/arch/arm64/include/asm/memory.h b/arch/arm64/include/asm/memory.h
index cd61239bae8c..0ef7948eb58c 100644
--- a/arch/arm64/include/asm/memory.h
+++ b/arch/arm64/include/asm/memory.h
@@ -328,6 +328,9 @@ static inline void *phys_to_virt(phys_addr_t x)
 })
 
 void dump_mem_limit(void);
+
+#define arch_get_addressable_range arch_get_addressable_range
+struct range arch_get_addressable_range(bool need_mapping);
 #endif /* !ASSEMBLY */
 
 /*
diff --git a/arch/arm64/mm/mmu.c b/arch/arm64/mm/mmu.c
index ca692a815731..a6433caf337f 100644
--- a/arch/arm64/mm/mmu.c
+++ b/arch/arm64/mm/mmu.c
@@ -1444,16 +1444,24 @@ static void __remove_pgd_mapping(pgd_t *pgdir, unsigned long start, u64 size)
 	free_empty_tables(start, end, PAGE_OFFSET, PAGE_END);
 }
 
-static bool inside_linear_region(u64 start, u64 size)
+struct range arch_get_addressable_range(bool need_mapping)
 {
+	struct range memhp_range;
+
 	/*
 	 * Linear mapping region is the range [PAGE_OFFSET..(PAGE_END - 1)]
 	 * accommodating both its ends but excluding PAGE_END. Max physical
 	 * range which can be mapped inside this linear mapping range, must
 	 * also be derived from its end points.
 	 */
-	return start >= __pa(_PAGE_OFFSET(vabits_actual)) &&
-	       (start + size - 1) <= __pa(PAGE_END - 1);
+	if (need_mapping) {
+		memhp_range.start = __pa(_PAGE_OFFSET(vabits_actual));
+		memhp_range.end =  __pa(PAGE_END - 1);
+	} else {
+		memhp_range.start = 0;
+		memhp_range.end = (1ULL << (MAX_PHYSMEM_BITS + 1)) - 1;
+	}
+	return memhp_range;
 }
 
 int arch_add_memory(int nid, u64 start, u64 size,
@@ -1461,11 +1469,6 @@ int arch_add_memory(int nid, u64 start, u64 size,
 {
 	int ret, flags = 0;
 
-	if (!inside_linear_region(start, size)) {
-		pr_err("[%llx %llx] is outside linear mapping region\n", start, start + size);
-		return -EINVAL;
-	}
-
 	if (rodata_full || debug_pagealloc_enabled())
 		flags = NO_BLOCK_MAPPINGS | NO_CONT_MAPPINGS;
 
-- 
2.20.1


_______________________________________________
linux-arm-kernel mailing list
linux-arm-kernel@lists.infradead.org
http://lists.infradead.org/mailman/listinfo/linux-arm-kernel

[RFC 3/3] s390/mm: Define arch_get_addressable_range()

[Anshuman Khandual]

This overrides arch_get_addressable_range() on s390 platform and drops
now redudant similar check in vmem_add_mapping().

Cc: Heiko Carstens <hca@linux.ibm.com>
Cc: Vasily Gorbik <gor@linux.ibm.com>
Cc: David Hildenbrand <david@redhat.com>
Cc: linux-s390@vger.kernel.org
Cc: linux-kernel@vger.kernel.org
Signed-off-by: Anshuman Khandual <anshuman.khandual@arm.com>
---
 arch/s390/include/asm/mmu.h |  2 ++
 arch/s390/mm/vmem.c         | 16 ++++++++++++----
 2 files changed, 14 insertions(+), 4 deletions(-)

diff --git a/arch/s390/include/asm/mmu.h b/arch/s390/include/asm/mmu.h
index e12ff0f29d1a..f92d3926b188 100644
--- a/arch/s390/include/asm/mmu.h
+++ b/arch/s390/include/asm/mmu.h
@@ -55,4 +55,6 @@ static inline int tprot(unsigned long addr)
 	return rc;
 }
 
+#define arch_get_addressable_range arch_get_addressable_range
+struct range arch_get_addressable_range(bool need_mapping);
 #endif
diff --git a/arch/s390/mm/vmem.c b/arch/s390/mm/vmem.c
index b239f2ba93b0..e03ad0ed13a7 100644
--- a/arch/s390/mm/vmem.c
+++ b/arch/s390/mm/vmem.c
@@ -532,14 +532,22 @@ void vmem_remove_mapping(unsigned long start, unsigned long size)
 	mutex_unlock(&vmem_mutex);
 }
 
+struct range arch_get_addressable_range(bool need_mapping)
+{
+	struct range memhp_range;
+
+	memhp_range.start = 0;
+	if (need_mapping)
+		memhp_range.end =  VMEM_MAX_PHYS;
+	else
+		memhp_range.end = (1ULL << (MAX_PHYSMEM_BITS + 1)) - 1;
+	return memhp_range;
+}
+
 int vmem_add_mapping(unsigned long start, unsigned long size)
 {
 	int ret;
 
-	if (start + size > VMEM_MAX_PHYS ||
-	    start + size < start)
-		return -ERANGE;
-
 	mutex_lock(&vmem_mutex);
 	ret = vmem_add_range(start, size);
 	if (ret)
-- 
2.20.1

Re: [RFC 3/3] s390/mm: Define arch_get_addressable_range()

[David Hildenbrand]

On 23.11.20 03:28, Anshuman Khandual wrote:
> This overrides arch_get_addressable_range() on s390 platform and drops
> now redudant similar check in vmem_add_mapping().
> 
> Cc: Heiko Carstens <hca@linux.ibm.com>
> Cc: Vasily Gorbik <gor@linux.ibm.com>
> Cc: David Hildenbrand <david@redhat.com>
> Cc: linux-s390@vger.kernel.org
> Cc: linux-kernel@vger.kernel.org
> Signed-off-by: Anshuman Khandual <anshuman.khandual@arm.com>
> ---
>  arch/s390/include/asm/mmu.h |  2 ++
>  arch/s390/mm/vmem.c         | 16 ++++++++++++----
>  2 files changed, 14 insertions(+), 4 deletions(-)
> 
> diff --git a/arch/s390/include/asm/mmu.h b/arch/s390/include/asm/mmu.h
> index e12ff0f29d1a..f92d3926b188 100644
> --- a/arch/s390/include/asm/mmu.h
> +++ b/arch/s390/include/asm/mmu.h
> @@ -55,4 +55,6 @@ static inline int tprot(unsigned long addr)
>  	return rc;
>  }
>  
> +#define arch_get_addressable_range arch_get_addressable_range
> +struct range arch_get_addressable_range(bool need_mapping);
>  #endif
> diff --git a/arch/s390/mm/vmem.c b/arch/s390/mm/vmem.c
> index b239f2ba93b0..e03ad0ed13a7 100644
> --- a/arch/s390/mm/vmem.c
> +++ b/arch/s390/mm/vmem.c
> @@ -532,14 +532,22 @@ void vmem_remove_mapping(unsigned long start, unsigned long size)
>  	mutex_unlock(&vmem_mutex);
>  }
>  
> +struct range arch_get_addressable_range(bool need_mapping)
> +{
> +	struct range memhp_range;
> +
> +	memhp_range.start = 0;
> +	if (need_mapping)
> +		memhp_range.end =  VMEM_MAX_PHYS;
> +	else
> +		memhp_range.end = (1ULL << (MAX_PHYSMEM_BITS + 1)) - 1;
> +	return memhp_range;
> +}
> +
>  int vmem_add_mapping(unsigned long start, unsigned long size)
>  {
>  	int ret;
>  
> -	if (start + size > VMEM_MAX_PHYS ||
> -	    start + size < start)
> -		return -ERANGE;
> -
>  	mutex_lock(&vmem_mutex);
>  	ret = vmem_add_range(start, size);
>  	if (ret)
> 

Note that vmem_add_mapping() is also called from extmem
(arch/s390/mm/extmem.c).

-- 
Thanks,

David / dhildenb

Re: [RFC 3/3] s390/mm: Define arch_get_addressable_range()

[Anshuman Khandual]

On 11/25/20 10:57 PM, David Hildenbrand wrote:
> On 23.11.20 03:28, Anshuman Khandual wrote:
>> This overrides arch_get_addressable_range() on s390 platform and drops
>> now redudant similar check in vmem_add_mapping().
>>
>> Cc: Heiko Carstens <hca@linux.ibm.com>
>> Cc: Vasily Gorbik <gor@linux.ibm.com>
>> Cc: David Hildenbrand <david@redhat.com>
>> Cc: linux-s390@vger.kernel.org
>> Cc: linux-kernel@vger.kernel.org
>> Signed-off-by: Anshuman Khandual <anshuman.khandual@arm.com>
>> ---
>>  arch/s390/include/asm/mmu.h |  2 ++
>>  arch/s390/mm/vmem.c         | 16 ++++++++++++----
>>  2 files changed, 14 insertions(+), 4 deletions(-)
>>
>> diff --git a/arch/s390/include/asm/mmu.h b/arch/s390/include/asm/mmu.h
>> index e12ff0f29d1a..f92d3926b188 100644
>> --- a/arch/s390/include/asm/mmu.h
>> +++ b/arch/s390/include/asm/mmu.h
>> @@ -55,4 +55,6 @@ static inline int tprot(unsigned long addr)
>>  	return rc;
>>  }
>>  
>> +#define arch_get_addressable_range arch_get_addressable_range
>> +struct range arch_get_addressable_range(bool need_mapping);
>>  #endif
>> diff --git a/arch/s390/mm/vmem.c b/arch/s390/mm/vmem.c
>> index b239f2ba93b0..e03ad0ed13a7 100644
>> --- a/arch/s390/mm/vmem.c
>> +++ b/arch/s390/mm/vmem.c
>> @@ -532,14 +532,22 @@ void vmem_remove_mapping(unsigned long start, unsigned long size)
>>  	mutex_unlock(&vmem_mutex);
>>  }
>>  
>> +struct range arch_get_addressable_range(bool need_mapping)
>> +{
>> +	struct range memhp_range;
>> +
>> +	memhp_range.start = 0;
>> +	if (need_mapping)
>> +		memhp_range.end =  VMEM_MAX_PHYS;
>> +	else
>> +		memhp_range.end = (1ULL << (MAX_PHYSMEM_BITS + 1)) - 1;
>> +	return memhp_range;
>> +}
>> +
>>  int vmem_add_mapping(unsigned long start, unsigned long size)
>>  {
>>  	int ret;
>>  
>> -	if (start + size > VMEM_MAX_PHYS ||
>> -	    start + size < start)
>> -		return -ERANGE;
>> -
>>  	mutex_lock(&vmem_mutex);
>>  	ret = vmem_add_range(start, size);
>>  	if (ret)
>>
> 
> Note that vmem_add_mapping() is also called from extmem
> (arch/s390/mm/extmem.c).

Right, probably something like this should be able to take care of
the range check, it lost out earlier.

diff --git a/arch/s390/mm/extmem.c b/arch/s390/mm/extmem.c
index 5060956b8e7d..c61620ae5ee6 100644
--- a/arch/s390/mm/extmem.c
+++ b/arch/s390/mm/extmem.c
@@ -337,6 +337,11 @@ __segment_load (char *name, int do_nonshared, unsigned long *addr, unsigned long
                goto out_free_resource;
        }
 
+       if (seg->end + 1 > VMEM_MAX_PHYS || seg->end + 1 < seg->start) {
+               rc = -ERANGE;
+               goto out_resource;
+       }
+
        rc = vmem_add_mapping(seg->start_addr, seg->end - seg->start_addr + 1);
        if (rc)
                goto out_resource;