From: Anshuman Khandual <anshuman.khandual@arm.com> To: David Hildenbrand <david@redhat.com>, linux-mm@kvack.org, akpm@linux-foundation.org, hca@linux.ibm.com, catalin.marinas@arm.com Cc: Oscar Salvador <osalvador@suse.de>, Vasily Gorbik <gor@linux.ibm.com>, Will Deacon <will@kernel.org>, Ard Biesheuvel <ardb@kernel.org>, Mark Rutland <mark.rutland@arm.com>, linux-arm-kernel@lists.infradead.org, linux-s390@vger.kernel.org, linux-kernel@vger.kernel.org Subject: Re: [PATCH V3 1/3] mm/memory_hotplug: Prevalidate the address range being added with platform Date: Fri, 22 Jan 2021 16:11:25 +0530 [thread overview] Message-ID: <897c31ba-d3bd-b694-8c87-82e784a60c51@arm.com> (raw) In-Reply-To: <9916f217-ec29-33ff-a260-7a26792d23a1@redhat.com> On 1/22/21 2:48 PM, David Hildenbrand wrote: > >> +/* >> + * Platforms should define arch_get_mappable_range() that provides >> + * maximum possible addressable physical memory range for which the >> + * linear mapping could be created. The platform returned address >> + * range must adhere to these following semantics. >> + * >> + * - range.start <= range.end >> + * - Range includes both end points [range.start..range.end] >> + * >> + * There is also a fallback definition provided here, allowing the >> + * entire possible physical address range in case any platform does >> + * not define arch_get_mappable_range(). >> + */ >> +struct range __weak arch_get_mappable_range(void) >> +{ >> + struct range memhp_range = { >> + .start = 0UL, >> + .end = -1ULL, >> + }; >> + return memhp_range; >> +} >> + >> +struct range memhp_get_pluggable_range(bool need_mapping) >> +{ >> + const u64 max_phys = (1ULL << (MAX_PHYSMEM_BITS + 1)) - 1; > > Sorry, thought about that line a bit more, and I think this is just > wrong (took me longer to realize as it should). The old code used this > calculation to print the limit only (in a wrong way), let's recap: > > Assume MAX_PHYSMEM_BITS=32 > > max_phys = (1ULL << (32 + 1)) - 1 = 0x1ffffffffull; > > Ehm, these are 33 bit. > > OTOH, old code checked for > > if (max_addr >> MAX_PHYSMEM_BITS) { > > Which makes sense, because > > 0x1ffffffffull >> 32 = 1 > > results in "true", meaning it's to big, while > > 0xffffffffull >> 32 = 0 > > correctly results in "false", meaning the address is fine. > > > > So, this should just be > > const u64 max_phys = 1ULL << MAX_PHYSMEM_BITS; > > (similarly as calculated in virito-mem code, or in kernel/resource.c) Should this be 1ULL << MAX_PHYSMEM_BITS - 1 instead ? Currently there are three usage for this variable in the function. - if (mhp_range.start > max_phys) - mhp_range.end = min_t(u64, mhp_range.end, max_phys) - mhp_range.end = max_phys mhp_range.end being always inclusive on the higher end and could be maximum (1ULL << MAX_PHYSMEM_BITS - 1) which is 0xFFFFFFFF instead of 0x100000000 when (1ULL << MAX_PHYSMEM_BITS) is followed for a 32 bit system. This seems consistent with the default fallback (range.end = -1ULL) defined above. > > >> + struct range memhp_range; >> + >> + if (need_mapping) { >> + memhp_range = arch_get_mappable_range(); >> + 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); >> + } else { >> + memhp_range.start = 0; >> + memhp_range.end = max_phys; >> + } >> + return memhp_range; >> +} >> +EXPORT_SYMBOL_GPL(memhp_get_pluggable_range); > >
WARNING: multiple messages have this Message-ID (diff)
From: Anshuman Khandual <anshuman.khandual@arm.com> To: David Hildenbrand <david@redhat.com>, linux-mm@kvack.org, akpm@linux-foundation.org, hca@linux.ibm.com, catalin.marinas@arm.com Cc: Mark Rutland <mark.rutland@arm.com>, linux-s390@vger.kernel.org, Vasily Gorbik <gor@linux.ibm.com>, linux-kernel@vger.kernel.org, Will Deacon <will@kernel.org>, Ard Biesheuvel <ardb@kernel.org>, linux-arm-kernel@lists.infradead.org, Oscar Salvador <osalvador@suse.de> Subject: Re: [PATCH V3 1/3] mm/memory_hotplug: Prevalidate the address range being added with platform Date: Fri, 22 Jan 2021 16:11:25 +0530 [thread overview] Message-ID: <897c31ba-d3bd-b694-8c87-82e784a60c51@arm.com> (raw) In-Reply-To: <9916f217-ec29-33ff-a260-7a26792d23a1@redhat.com> On 1/22/21 2:48 PM, David Hildenbrand wrote: > >> +/* >> + * Platforms should define arch_get_mappable_range() that provides >> + * maximum possible addressable physical memory range for which the >> + * linear mapping could be created. The platform returned address >> + * range must adhere to these following semantics. >> + * >> + * - range.start <= range.end >> + * - Range includes both end points [range.start..range.end] >> + * >> + * There is also a fallback definition provided here, allowing the >> + * entire possible physical address range in case any platform does >> + * not define arch_get_mappable_range(). >> + */ >> +struct range __weak arch_get_mappable_range(void) >> +{ >> + struct range memhp_range = { >> + .start = 0UL, >> + .end = -1ULL, >> + }; >> + return memhp_range; >> +} >> + >> +struct range memhp_get_pluggable_range(bool need_mapping) >> +{ >> + const u64 max_phys = (1ULL << (MAX_PHYSMEM_BITS + 1)) - 1; > > Sorry, thought about that line a bit more, and I think this is just > wrong (took me longer to realize as it should). The old code used this > calculation to print the limit only (in a wrong way), let's recap: > > Assume MAX_PHYSMEM_BITS=32 > > max_phys = (1ULL << (32 + 1)) - 1 = 0x1ffffffffull; > > Ehm, these are 33 bit. > > OTOH, old code checked for > > if (max_addr >> MAX_PHYSMEM_BITS) { > > Which makes sense, because > > 0x1ffffffffull >> 32 = 1 > > results in "true", meaning it's to big, while > > 0xffffffffull >> 32 = 0 > > correctly results in "false", meaning the address is fine. > > > > So, this should just be > > const u64 max_phys = 1ULL << MAX_PHYSMEM_BITS; > > (similarly as calculated in virito-mem code, or in kernel/resource.c) Should this be 1ULL << MAX_PHYSMEM_BITS - 1 instead ? Currently there are three usage for this variable in the function. - if (mhp_range.start > max_phys) - mhp_range.end = min_t(u64, mhp_range.end, max_phys) - mhp_range.end = max_phys mhp_range.end being always inclusive on the higher end and could be maximum (1ULL << MAX_PHYSMEM_BITS - 1) which is 0xFFFFFFFF instead of 0x100000000 when (1ULL << MAX_PHYSMEM_BITS) is followed for a 32 bit system. This seems consistent with the default fallback (range.end = -1ULL) defined above. > > >> + struct range memhp_range; >> + >> + if (need_mapping) { >> + memhp_range = arch_get_mappable_range(); >> + 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); >> + } else { >> + memhp_range.start = 0; >> + memhp_range.end = max_phys; >> + } >> + return memhp_range; >> +} >> +EXPORT_SYMBOL_GPL(memhp_get_pluggable_range); > > _______________________________________________ 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:[~2021-01-22 10:44 UTC|newest] Thread overview: 54+ messages / expand[flat|nested] mbox.gz Atom feed top 2021-01-18 13:12 [PATCH V3 0/3] mm/memory_hotplug: Pre-validate the address range with platform Anshuman Khandual 2021-01-18 13:12 ` Anshuman Khandual 2021-01-18 13:12 ` [PATCH V3 1/3] mm/memory_hotplug: Prevalidate the address range being added " Anshuman Khandual 2021-01-18 13:12 ` Anshuman Khandual 2021-01-19 12:21 ` David Hildenbrand 2021-01-19 12:21 ` David Hildenbrand 2021-01-20 8:33 ` Anshuman Khandual 2021-01-20 8:33 ` Anshuman Khandual 2021-01-20 10:41 ` David Hildenbrand 2021-01-20 10:41 ` David Hildenbrand 2021-01-20 11:58 ` Oscar Salvador 2021-01-20 11:58 ` Oscar Salvador 2021-01-21 9:23 ` Oscar Salvador 2021-01-21 9:23 ` Oscar Salvador 2021-01-22 9:18 ` David Hildenbrand 2021-01-22 9:18 ` David Hildenbrand 2021-01-22 10:41 ` Anshuman Khandual [this message] 2021-01-22 10:41 ` Anshuman Khandual 2021-01-22 10:42 ` David Hildenbrand 2021-01-22 10:42 ` David Hildenbrand 2021-01-22 10:43 ` David Hildenbrand 2021-01-22 10:43 ` David Hildenbrand 2021-01-18 13:13 ` [PATCH V3 2/3] arm64/mm: Define arch_get_mappable_range() Anshuman Khandual 2021-01-18 13:13 ` Anshuman Khandual 2021-01-19 12:24 ` David Hildenbrand 2021-01-19 12:24 ` David Hildenbrand 2021-01-18 13:13 ` [PATCH V3 3/3] s390/mm: " Anshuman Khandual 2021-01-18 13:13 ` Anshuman Khandual 2021-01-19 12:26 ` David Hildenbrand 2021-01-19 12:26 ` David Hildenbrand 2021-01-20 8:28 ` Anshuman Khandual 2021-01-20 8:28 ` Anshuman Khandual 2021-01-20 10:39 ` David Hildenbrand 2021-01-20 10:39 ` David Hildenbrand 2021-01-18 13:13 ` [PATCH RFC] virtio-mem: check against memhp_get_pluggable_range() which memory we can hotplug Anshuman Khandual 2021-01-18 13:13 ` Anshuman Khandual 2021-01-18 13:21 ` Anshuman Khandual 2021-01-18 13:21 ` Anshuman Khandual 2021-01-19 12:27 ` David Hildenbrand 2021-01-19 12:27 ` David Hildenbrand 2021-01-21 9:57 ` David Hildenbrand 2021-01-21 9:57 ` David Hildenbrand 2021-01-22 3:32 ` Anshuman Khandual 2021-01-22 3:32 ` Anshuman Khandual 2021-01-19 13:33 ` [PATCH V3 0/3] mm/memory_hotplug: Pre-validate the address range with platform David Hildenbrand 2021-01-19 13:33 ` David Hildenbrand 2021-01-19 13:40 ` Oscar Salvador 2021-01-19 13:40 ` Oscar Salvador 2021-01-20 8:37 ` Anshuman Khandual 2021-01-20 8:37 ` Anshuman Khandual 2021-01-22 6:04 ` Anshuman Khandual 2021-01-22 6:04 ` Anshuman Khandual 2021-01-22 8:34 ` David Hildenbrand 2021-01-22 8:34 ` 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=897c31ba-d3bd-b694-8c87-82e784a60c51@arm.com \ --to=anshuman.khandual@arm.com \ --cc=akpm@linux-foundation.org \ --cc=ardb@kernel.org \ --cc=catalin.marinas@arm.com \ --cc=david@redhat.com \ --cc=gor@linux.ibm.com \ --cc=hca@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=osalvador@suse.de \ --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.