"Aneesh Kumar K.V" <aneesh.kumar@xxxxxxxxxxxxx> writes:
> On 5/14/19 9:45 AM, Dan Williams wrote:
>> [ add Keith who was looking at something similar ]
>>
...
>>
>> If it's reserved then we should not be accessing, even if the above
>> works in practice. Isn't the fix something more like this to fix up
>> the assumptions at release time?
>>
>> diff --git a/kernel/memremap.c b/kernel/memremap.c
>> index a856cb5ff192..9074ba14572c 100644
>> --- a/kernel/memremap.c
>> +++ b/kernel/memremap.c
>> @@ -90,6 +90,7 @@ static void devm_memremap_pages_release(void *data)
>> struct device *dev = pgmap->dev;
>> struct resource *res = &pgmap->res;
>> resource_size_t align_start, align_size;
>> + struct vmem_altmap *altmap = pgmap->altmap_valid ? &pgmap->altmap : NULL;
>> unsigned long pfn;
>> int nid;
>>
>> @@ -102,7 +103,10 @@ static void devm_memremap_pages_release(void *data)
>> align_size = ALIGN(res->start + resource_size(res), SECTION_SIZE)
>> - align_start;
>>
>> - nid = page_to_nid(pfn_to_page(align_start >> PAGE_SHIFT));
>> + pfn = align_start >> PAGE_SHIFT;
>> + if (altmap)
>> + pfn += vmem_altmap_offset(altmap);
>> + nid = page_to_nid(pfn_to_page(pfn));
>>
>> mem_hotplug_begin();
>> if (pgmap->type == MEMORY_DEVICE_PRIVATE) {
>> @@ -110,8 +114,7 @@ static void devm_memremap_pages_release(void *data)
>> __remove_pages(page_zone(pfn_to_page(pfn)), pfn,
>> align_size >> PAGE_SHIFT, NULL);
>> } else {
>> - arch_remove_memory(nid, align_start, align_size,
>> - pgmap->altmap_valid ? &pgmap->altmap : NULL);
>> + arch_remove_memory(nid, align_start, align_size, altmap);
>> kasan_remove_zero_shadow(__va(align_start), align_size);
>> }
>> mem_hotplug_done();
>>
> I did try that first. I was not sure about that. From the memory add vs
> remove perspective.
>
> devm_memremap_pages:
>
> align_start = res->start & ~(SECTION_SIZE - 1);
> align_size = ALIGN(res->start + resource_size(res), SECTION_SIZE)
> - align_start;
> align_end = align_start + align_size - 1;
>
> error = arch_add_memory(nid, align_start, align_size, altmap,
> false);
>
>
> devm_memremap_pages_release:
>
> /* pages are dead and unused, undo the arch mapping */
> align_start = res->start & ~(SECTION_SIZE - 1);
> align_size = ALIGN(res->start + resource_size(res), SECTION_SIZE)
> - align_start;
>
> arch_remove_memory(nid, align_start, align_size,
> pgmap->altmap_valid ? &pgmap->altmap : NULL);
>
>
> Now if we are fixing the memremap_pages_release, shouldn't we adjust
> alig_start w.r.t memremap_pages too? and I was not sure what that means
> w.r.t add/remove alignment requirements.
>
> What is the intended usage of reserve area? I guess we want that part to
> be added? if so shouldn't we remove them?
We need to intialize the struct page backing the reserve area too right?
Where should we do that?
-aneesh
