Re: [PATCH 1/1] drivers: dma-contiguous: Ensure cma reserve region never cross the low/high mem boundary

[Laura Abbott]

On 11/16/2016 09:21 PM, Jason Liu wrote:
> 
> 
>> -----Original Message-----
>> From: Laura Abbott [mailto:labbott@xxxxxxxxxx]
>> Sent: Thursday, November 17, 2016 4:00 AM
>> To: Jason Liu <jason.hui.liu@xxxxxxx>; linux-arm-kernel@xxxxxxxxxxxxxxxxxxx
>> Cc: gregkh@xxxxxxxxxxxxxxxxxxx; iamjoonsoo.kim@xxxxxxx; linux-
>> kernel@xxxxxxxxxxxxxxx; m.szyprowski@xxxxxxxxxxx
>> Subject: Re: [PATCH 1/1] drivers: dma-contiguous: Ensure cma reserve region
>> never cross the low/high mem boundary
>>
>> On 11/16/2016 06:19 AM, Jason Liu wrote:
>>> If the cma reserve region goes through the device-tree method, also
>>> need ensure the cma reserved region not cross the low/high mem
>>> boundary. This patch did the similar fix as commit:16195dd
>>> ("mm: cma: Ensure that reservations never cross the low/high mem
>>> boundary")
>>>
>>> Signed-off-by: Jason Liu <jason.hui.liu@xxxxxxx>
>>> Cc: Marek Szyprowski <m.szyprowski@xxxxxxxxxxx>
>>> Cc: Joonsoo Kim <iamjoonsoo.kim@xxxxxxx>
>>> Cc: Greg Kroah-Hartman <gregkh@xxxxxxxxxxxxxxxxxxx>
>>> ---
>>>  drivers/base/dma-contiguous.c | 27 +++++++++++++++++++++++++++
>>>  1 file changed, 27 insertions(+)
>>>
>>> diff --git a/drivers/base/dma-contiguous.c
>>> b/drivers/base/dma-contiguous.c index e167a1e1..2bc093c 100644
>>> --- a/drivers/base/dma-contiguous.c
>>> +++ b/drivers/base/dma-contiguous.c
>>> @@ -244,6 +244,7 @@ static int __init rmem_cma_setup(struct
>>> reserved_mem *rmem)  {
>>>  	phys_addr_t align = PAGE_SIZE << max(MAX_ORDER - 1,
>> pageblock_order);
>>>  	phys_addr_t mask = align - 1;
>>> +	phys_addr_t highmem_start;
>>>  	unsigned long node = rmem->fdt_node;
>>>  	struct cma *cma;
>>>  	int err;
>>> @@ -256,6 +257,32 @@ static int __init rmem_cma_setup(struct
>> reserved_mem *rmem)
>>>  		pr_err("Reserved memory: incorrect alignment of CMA
>> region\n");
>>>  		return -EINVAL;
>>>  	}
>>> +#ifdef CONFIG_X86
>>> +	/*
>>> +	 * high_memory isn't direct mapped memory so retrieving its physical
>>> +	 * address isn't appropriate.  But it would be useful to check the
>>> +	 * physical address of the highmem boundary so it's justfiable to get
>>> +	 * the physical address from it.  On x86 there is a validation check for
>>> +	 * this case, so the following workaround is needed to avoid it.
>>> +	 */
>>> +	highmem_start = __pa_nodebug(high_memory); #else
>>> +	highmem_start = __pa(high_memory);
>>> +#endif
>>
>> The inline #ifdef is not great style, we shouldn't be spreading it around.
> 
> This is the similar fix in the 16195dd ("mm: cma: Ensure that reservations never cross
> the low/high mem boundary". Do you have a better idea for this? 
> 

See an example in https://marc.info/?l=linux-kernel&m=147812049024611&w=2
this is getting cleaned up as part of some work I'm doing for CONFIG_DEBUG_VIRTUAL

>>
>>> +
>>> +	/*
>>> +	 * All pages in the reserved area must come from the same zone.
>>> +	 * If the reserved region crosses the low/high memory boundary,
>>> +	 * try to fix it up and then fall back to allocate from the low mem
>>> +	 */
>>> +	if (rmem->base < highmem_start &&
>>> +		(rmem->base + rmem->size) > highmem_start) {
>>> +		memblock_free(rmem->base, rmem->size);
>>> +		rmem->base = memblock_alloc_range(rmem->size, align, 0,
>>> +						highmem_start,
>> MEMBLOCK_NONE);
>>> +		if (!rmem->base)
>>> +			return -ENOMEM;
>>> +	}
>>
>> Given the alloc happened in the of code, it seems bad form to be bringing the
>> free and re-alloc here. Perhaps we should be doing the limiting and checking in
>> the reserved mem code?
> 
> I original though to fix it into the drivers/of/of_reserved_mem.c, but hesitate to
> do it due to this of_reserved_mem is common code to do the reservation, which
> is something not related with CMA requirement. 
> 

Agreed this case is CMA specific. It might be worth discussion whether allowing
reservation across zones is even something that should be allowed by the generic
code though.

> Appreciated that anyone can provide comments to improve this solution. Without this,
> the Linux kernel will not boot up when do the CMA reservation from the DTS method,
> since the dma_alloc_coherent will fail when do the dma memory allocation. 
> 

I'd suggest bringing this up on the devicetree mailing list. If you get a negative
or no response there this approach can be reviewed some more.

Thanks,
Laura