On 12/10/20 12:34 PM, David Hildenbrand wrote:

quoted

Am 10.12.2020 um 07:58 schrieb Heiko Carstens [off-list ref]:

On Thu, Dec 10, 2020 at 09:48:11AM +0530, Anshuman Khandual wrote:

quoted

quoted

quoted

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;

Right, what I had in mind as reply to v1. Not sure if we really need new checks in common code. Having a new memhp_get_pluggable_range() would be sufficient for my use case (virtio-mem).

Hello David,

Quick question. Currently memhp_get_pluggable_range() is a mm/memory_hotplug.c
internal static inline function. Only memhp_range_allowed() is available via
the header include/linux/memory_hotplug.h But For memhp_get_pluggable_range()
to be visible to the drivers, it needs to get included in the header and also
be exported via EXPORT_SYMBOL_GPL() in mm/memory_hotplug.c OR just move the
entire definition as static inline into the header itself. Wondering which way
would be better ?

- Anshuman

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