Re: [PATCH v1 02/10] powerpc/kernel/iommu: Align size for IOMMU_PAGE_SIZE on iommu_*_coherent()

[Alexey Kardashevskiy]

On 28/08/2020 02:51, Leonardo Bras wrote:
> On Sat, 2020-08-22 at 20:07 +1000, Alexey Kardashevskiy wrote:
>>
>> On 18/08/2020 09:40, Leonardo Bras wrote:
>>> Both iommu_alloc_coherent() and iommu_free_coherent() assume that once
>>> size is aligned to PAGE_SIZE it will be aligned to IOMMU_PAGE_SIZE.
>>
>> The only case when it is not aligned is when IOMMU_PAGE_SIZE > PAGE_SIZE
>> which is unlikely but not impossible, we could configure the kernel for
>> 4K system pages and 64K IOMMU pages I suppose. Do we really want to do
>> this here, or simply put WARN_ON(tbl->it_page_shift > PAGE_SHIFT)?
> 
> I think it would be better to keep the code as much generic as possible
> regarding page sizes. 

Then you need to test it. Does 4K guest even boot (it should but I would
not bet much on it)?

> 
>> Because if we want the former (==support), then we'll have to align the
>> size up to the bigger page size when allocating/zeroing system pages,
>> etc. 
> 
> This part I don't understand. Why do we need to align everything to the
> bigger pagesize? 
> 
> I mean, is not that enough that the range [ret, ret + size[ is both
> allocated by mm and mapped on a iommu range?
> 
> Suppose a iommu_alloc_coherent() of 16kB on PAGESIZE = 4k and
> IOMMU_PAGE_SIZE() == 64k.
> Why 4 * cpu_pages mapped by a 64k IOMMU page is not enough? 
> All the space the user asked for is allocated and mapped for DMA.


The user asked to map 16K, the rest - 48K - is used for something else
(may be even mapped to another device) but you are making all 64K
accessible by the device which only should be able to access 16K.

In practice, if this happens, H_PUT_TCE will simply fail.


> 
>> Bigger pages are not the case here as I understand it.
> 
> I did not get this part, what do you mean?


Possible IOMMU page sizes are 4K, 64K, 2M, 16M, 256M, 1GB, and the
supported set of sizes is different for P8/P9 and type of IO (PHB,
NVLink/CAPI).


> 
>>> Update those functions to guarantee alignment with requested size
>>> using IOMMU_PAGE_ALIGN() before doing iommu_alloc() / iommu_free().
>>>
>>> Also, on iommu_range_alloc(), replace ALIGN(n, 1 << tbl->it_page_shift)
>>> with IOMMU_PAGE_ALIGN(n, tbl), which seems easier to read.
>>>
>>> Signed-off-by: Leonardo Bras <leobras.c@xxxxxxxxx>
>>> ---
>>>  arch/powerpc/kernel/iommu.c | 17 +++++++++--------
>>>  1 file changed, 9 insertions(+), 8 deletions(-)
>>>
>>> diff --git a/arch/powerpc/kernel/iommu.c b/arch/powerpc/kernel/iommu.c
>>> index 9704f3f76e63..d7086087830f 100644
>>> --- a/arch/powerpc/kernel/iommu.c
>>> +++ b/arch/powerpc/kernel/iommu.c
>>> @@ -237,10 +237,9 @@ static unsigned long iommu_range_alloc(struct device *dev,
>>>  	}
>>>  
>>>  	if (dev)
>>> -		boundary_size = ALIGN(dma_get_seg_boundary(dev) + 1,
>>> -				      1 << tbl->it_page_shift);
>>> +		boundary_size = IOMMU_PAGE_ALIGN(dma_get_seg_boundary(dev) + 1, tbl);
>>
>> Run checkpatch.pl, should complain about a long line.
> 
> It's 86 columns long, which is less than the new limit of 100 columns
> Linus announced a few weeks ago. checkpatch.pl was updated too:
> https://www.phoronix.com/scan.php?page=news_item&px=Linux-Kernel-Deprecates-80-Col

Yay finally :) Thanks,


> 
>>
>>
>>>  	else
>>> -		boundary_size = ALIGN(1UL << 32, 1 << tbl->it_page_shift);
>>> +		boundary_size = IOMMU_PAGE_ALIGN(1UL << 32, tbl);
>>>  	/* 4GB boundary for iseries_hv_alloc and iseries_hv_map */
>>>  
>>>  	n = iommu_area_alloc(tbl->it_map, limit, start, npages, tbl->it_offset,
>>> @@ -858,6 +857,7 @@ void *iommu_alloc_coherent(struct device *dev, struct iommu_table *tbl,
>>>  	unsigned int order;
>>>  	unsigned int nio_pages, io_order;
>>>  	struct page *page;
>>> +	size_t size_io = size;
>>>  
>>>  	size = PAGE_ALIGN(size);
>>>  	order = get_order(size);
>>> @@ -884,8 +884,9 @@ void *iommu_alloc_coherent(struct device *dev, struct iommu_table *tbl,
>>>  	memset(ret, 0, size);
>>>  
>>>  	/* Set up tces to cover the allocated range */
>>> -	nio_pages = size >> tbl->it_page_shift;
>>> -	io_order = get_iommu_order(size, tbl);
>>> +	size_io = IOMMU_PAGE_ALIGN(size_io, tbl);
>>> +	nio_pages = size_io >> tbl->it_page_shift;
>>> +	io_order = get_iommu_order(size_io, tbl);
>>>  	mapping = iommu_alloc(dev, tbl, ret, nio_pages, DMA_BIDIRECTIONAL,
>>>  			      mask >> tbl->it_page_shift, io_order, 0);
>>>  	if (mapping == DMA_MAPPING_ERROR) {
>>> @@ -900,11 +901,11 @@ void iommu_free_coherent(struct iommu_table *tbl, size_t size,
>>>  			 void *vaddr, dma_addr_t dma_handle)
>>>  {
>>>  	if (tbl) {
>>> -		unsigned int nio_pages;
>>> +		size_t size_io = IOMMU_PAGE_ALIGN(size, tbl);
>>> +		unsigned int nio_pages = size_io >> tbl->it_page_shift;
>>>  
>>> -		size = PAGE_ALIGN(size);
>>> -		nio_pages = size >> tbl->it_page_shift;
>>>  		iommu_free(tbl, dma_handle, nio_pages);
>>> +
>>
>> Unrelated new line.
> 
> Will be removed. Thanks!
> 
>>
>>
>>>  		size = PAGE_ALIGN(size);
>>>  		free_pages((unsigned long)vaddr, get_order(size));
>>>  	}
>>>
> 

-- 
Alexey