Messages in this thread | | | Subject | Re: [PATCH] arm64/mm: Validate hotplug range before creating linear mapping | From | David Hildenbrand <> | Date | Mon, 19 Oct 2020 16:58:49 +0200 |
| |
>> >> Most probably, >> >> struct range memhp_get_addressable_range(bool need_mapping) >> { >> ... >> } > > Something like this... > > +struct memhp_range { > + u64 start; > + u64 end; > +};
We do have struct range already in include/linux/range.h
> + > +#ifndef arch_get_addressable_range > +static inline struct memhp_range arch_get_mappable_range(bool need_mapping) > +{ > + struct memhp_range range = { > + .start = 0UL, > + .end = (1ull << (MAX_PHYSMEM_BITS + 1)) - 1,
Or just set to -1ULL if it's only used in memhp_get_mappable_range(), to keep things simple ().
> + }; > + return range; > +} > +#endif > + > +static inline struct memhp_range memhp_get_mappable_range(bool need_mapping)
due to "need_mapping" the function might better be called
memhp_get_pluggable_range()
or similar
> +{ > + const u64 max_phys = (1ull << (MAX_PHYSMEM_BITS + 1)) - 1; > + struct memhp_range range = arch_get_mappable_range(need_mapping); > + > + if (range.start > max_phys) { > + range.start = 0; > + range.end = 0; > + } > + range.end = min_t(u64, range.end, max_phys); > + return range; > +} > + > +static inline bool memhp_range_allowed(u64 start, u64 end, bool need_mapping) > +{ > + struct memhp_range range = memhp_get_mappable_range(need_mapping); > + > + return (start <= end) && (start >= range.start) && (end <= range.end);
Keep in mind that in memory hotplug code, "end" is usually exclusive, and "end" in "struct range" is inclusive (see range_len(), or how you calculate max_phys.
So depending on the semantics, you might have to fixup your comparisons.
return start < end && start >= range.start && end <= range.end - 1;
[...]
>> 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. > > Agreed. > > Planning to add memhp_range_allowed() check in add_memory(), __add_memory(), > add_memory_driver_managed() and memremap_pages(). This check might just get > called twice depending on the hotplug path. Wondering if this needs to be > added any where else ?
So
add_memory() needs to - add sections via arch_add_memory() - create a mapping via arch_add_memory()->add_pages()
memremap_pages() via arch_add_memory() needs to - add sections via arch_add_memory() - create a mapping via arch_add_memory()->add_pages()
memremap_pages() via add_pages() needs to - add sections
I'll reuse the functions from virtio-mem code once in place (exposing memhp_get_pluggable_range()).
I do agree that having the callers of arch_add_memory() / add_pages() validate stuff isn't completely nice. I already raised that I would much rather want to see !arch wrappers for these arch functions that could validate stuff. But then we would have to do a bigger cleanup to get naming right.
1. Rename functions for handling system ram like
s/add_memory/add_sysram/ s/remove_memory/remove_sysram/ ...
2. Have a new add_memory() that validates + calls arch_add_memory()
3. s/add_pages/arch_add_pages/
4. Have a new add_pages() that validates + calls arch_add_pages()
...
Long story short, handling it in the 2 (!) callers might be easier for now.
-- Thanks,
David / dhildenb
| |