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

[David Hildenbrand (Arm)]

>> 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.

...

>>> +		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.


-- 
Cheers,

David