Re: [PATCH V3 2/2] arm64/mm: Reject memory removal that splits a kernel leaf mapping

[Anshuman Khandual]

On 03/03/26 2:30 PM, David Hildenbrand (Arm) wrote:
>>> I can understand the desire for this check, but I am confused about Fixes:
>>
>> Probably nothing is broken now I guess but the original memory hot remove
>> patch should have taken care of this scenario. Although don't have strong
>> opinions either way. We could drop both "Fixes" and "Closes" tags here if
>> that is preferred.
>>
> 
> We tend to only tag actual fixes. If we consider this a possible fix, we
> should ask ourselves whether this would be stable material.

Ryan had earlier asked for the Cc: stable to be dropped
as this was not an actual fix. But seems like we should
drop these Fixes/Closes tags as well.

> 
> ...
> 
>>>> +		return false;
>>>> +
>>>> +	pgdp = pgd_offset_k(addr);
>>>> +	pgd = pgdp_get(pgdp);
>>>> +	if (!pgd_present(pgd))
>>>> +		return false;
>>>
>>> How could we end up with non-present areas in a range we hotplugged earlier?
>>
>> We might not in reality but in order to be sure just an additional protection.
> 
> I'm rather wondering if this would indicate a real bug somewhere else
> that we would silently swallow.
> 
> Anyhow, no real preference from my side, just something I considered weird.
> 
> [...]
> 
>>>> +
>>>> +static bool can_unmap_without_split(unsigned long pfn, unsigned long nr_pages)
>>>> +{
>>>> +	unsigned long phys_start, phys_end, size, start, end;
>>>> +
>>>> +	phys_start = PFN_PHYS(pfn);
>>>> +	phys_end = phys_start + nr_pages * PAGE_SIZE;
>>>> +
>>>> +	/*
>>>> +	 * PFN range's linear map edges are leaf entry aligned
>>>> +	 */
>>>> +	start = __phys_to_virt(phys_start);
>>>> +	end =  __phys_to_virt(phys_end);
>>>> +	if (addr_splits_kernel_leaf(start) || addr_splits_kernel_leaf(end)) {
>>>> +		pr_warn("[%lx %lx] splits a leaf entry in linear map\n",
>>>> +			phys_start, phys_end);
>>>> +		return false;
>>>> +	}
>>>> +
>>>> +	/*
>>>> +	 * PFN range's vmemmap edges are leaf entry aligned
>>>> +	 */
>>>> +	size = nr_pages * sizeof(struct page);
>>>> +	start = (unsigned long)pfn_to_page(pfn);
>>>> +	end = start + size;
>>>
>>> As arm64 cannot be used with CONFIG_SPARSEMEM (only with
>>> CONFIG_SPARSEMEM_VMEMMAP, as it sets SPARSEMEM_VMEMMAP_ENABLE),
>>> I think you can just do
> 
> Just to clarify:
> 
> What I meant here is: with CONFIG_SPARSEMEM but without
> CONFIG_SPARSEMEM_VMEMMAP.

Alright but does the commit message or pfn_to_page() code
block here needs a comment about this ? OR it is apparent
enough ?