Re: [PATCH] arm64/mm: Validate hotplug range before creating linear mapping

[David Hildenbrand]

>> We do have __add_pages()->check_hotplug_memory_addressable() where we
>> already check against MAX_PHYSMEM_BITS.
> 
> Initially, I thought about check_hotplug_memory_addressable() but the
> existing check that asserts end of hotplug wrt MAX_PHYSMEM_BITS, is
> generic in nature. AFAIK the linear mapping problem is arm64 specific,
> hence I was not sure whether to add an arch specific callback which
> will give platform an opportunity to weigh in for these ranges.

Also on s390x, the range where you can create an identity mapping depends on
- early kernel setup
- kasan

(I assume it's the same for all archs)

See arch/s390/mm/vmem.c:vmem_add_mapping(), which contains similar
checks (VMEM_MAX_PHYS).

> 
> But hold on, check_hotplug_memory_addressable() only gets called from
> __add_pages() after linear mapping creation in arch_add_memory(). How
> would it help ? We need some thing for add_memory(), its variants and
> also possibly for memremap_pages() when it calls arch_add_memory().
> 

Good point. We chose that place for simplicity when adding it (I was
favoring calling it at two places back then). Now, we might have good
reason to move the checks further up the call chain.

Most probably,

struct range memhp_get_addressable_range(bool need_mapping)
{
	...
}

Would make sense, to deal with memremap_pages() without identity mappings.

We have two options:

1. Generalize the checks, check early in applicable functions. Have a
single way to get applicable ranges, both in callers, and inside the
functions.

2. Keep the checks where they are. Add memhp_get_addressable_range() so
callers can figure limits out. It's less clear what the relation between
the different checks is. And it's likely if things change at one place
that we miss the other place.

>> struct range memhp_get_addressable_range(void)
>> {
>> 	const u64 max_phys = (1ull << (MAX_PHYSMEM_BITS + 1)) - 1;
>> 	struct range range = arch_get_mappable_range();
> 
> What would you suggest as the default fallback range if a platform
> does not define this callback.

Just the largest possible range until we implement them. IIRC, an s390x
version should be easy to add.

> 
>>
>> 	if (range.start > max_phys) {
>> 		range.start = 0;
>> 		range.end = 0;
>> 	}
>> 	range.end = max_t(u64, range.end, max_phys);
> 
> min_t instead ?

Yeah :)

> 
>>
>> 	return range;
>> }
>>
>>
>> That, we can use in check_hotplug_memory_addressable(), and also allow
>> add_memory*() users to make use of it.
> 
> So this check would happen twice during a hotplug ?

Right now it's like calling a function with wrong arguments - you just
don't have a clue what valid arguments are, because non-obvious errors
(besides -ENOMEM, which is a temporary error) pop up deep down the call
chain.

For example, virito-mem would use it to detect during device
initialization the usable device range, and warn the user accordingly.
It currently manually checks for MAX_PHYSMEM_BITS, but that's just ugly.
Failing at random add_memory() calls (permanently!) is not so nice.

In case of DIMMs, we could use it to detect if adding parts of a DIMM
won't work (and warn the user early). We could try to add as much as
possible.

-- 
Thanks,

David / dhildenb