Re: [PATCH kernel v2 2/4] powerpc/pseries: Allow not having ibm,hypertas-functions::hcall-multi-tce for DDW

[Alexey Kardashevskiy <aik@ozlabs.ru>]

On 17/12/2019 10:07, Thiago Jung Bauermann wrote:
> 
> Alexey Kardashevskiy <aik@ozlabs.ru> writes:
> 
>> By default a pseries guest supports a H_PUT_TCE hypercall which maps
>> a single IOMMU page in a DMA window. Additionally the hypervisor may
>> support H_PUT_TCE_INDIRECT/H_STUFF_TCE which update multiple TCEs at once;
>> this is advertised via the device tree /rtas/ibm,hypertas-functions
>> property which Linux converts to FW_FEATURE_MULTITCE.
>>
>> FW_FEATURE_MULTITCE is checked when dma_iommu_ops is used; however
>> the code managing the huge DMA window (DDW) ignores it and calls
>> H_PUT_TCE_INDIRECT even if it is explicitly disabled via
>> the "multitce=off" kernel command line parameter.
>>
>> This adds FW_FEATURE_MULTITCE checking to the DDW code path.
>>
>> This changes tce_build_pSeriesLP to take liobn and page size as
>> the huge window does not have iommu_table descriptor which usually
>> the place to store these numbers.
>>
>> Fixes: 4e8b0cf46b25 ("powerpc/pseries: Add support for dynamic dma windows")
>> Signed-off-by: Alexey Kardashevskiy <aik@ozlabs.ru>
> 
> Reviewed-by: Thiago Jung Bauermann <bauerman@linux.ibm.com>
> Tested-by: Thiago Jung Bauermann <bauerman@linux.ibm.com>
> 
> Some minor nits below. Feel free to ignore.
> 
>> @@ -146,25 +146,25 @@ static int tce_build_pSeriesLP(struct iommu_table *tbl, long tcenum,
>>  	int ret = 0;
>>  	long tcenum_start = tcenum, npages_start = npages;
>>
>> -	rpn = __pa(uaddr) >> TCE_SHIFT;
>> +	rpn = __pa(uaddr) >> tceshift;
>>  	proto_tce = TCE_PCI_READ;
>>  	if (direction != DMA_TO_DEVICE)
>>  		proto_tce |= TCE_PCI_WRITE;
>>
>>  	while (npages--) {
>> -		tce = proto_tce | (rpn & TCE_RPN_MASK) << TCE_RPN_SHIFT;
>> -		rc = plpar_tce_put((u64)tbl->it_index, (u64)tcenum << 12, tce);
>> +		tce = proto_tce | (rpn & TCE_RPN_MASK) << tceshift;
>> +		rc = plpar_tce_put((u64)liobn, (u64)tcenum << tceshift, tce);
> 
> Is it necessary to cast to u64 here? plpar_tce_put() takes unsigned long
> for both arguments.


Looked as an unrelated change. Small but still unrelated.


> 
>> @@ -261,16 +263,16 @@ static int tce_buildmulti_pSeriesLP(struct iommu_table *tbl, long tcenum,
>>  	return ret;
>>  }
>>
>> -static void tce_free_pSeriesLP(struct iommu_table *tbl, long tcenum, long npages)
>> +static void tce_free_pSeriesLP(unsigned long liobn, long tcenum, long npages)
>>  {
>>  	u64 rc;
>>
>>  	while (npages--) {
>> -		rc = plpar_tce_put((u64)tbl->it_index, (u64)tcenum << 12, 0);
>> +		rc = plpar_tce_put((u64)liobn, (u64)tcenum << 12, 0);
> 
> Same comment regarding cast to u64.
> 
>> @@ -400,6 +402,20 @@ static int tce_setrange_multi_pSeriesLP(unsigned long start_pfn,
>>  	u64 rc = 0;
>>  	long l, limit;
>>
>> +	if (!firmware_has_feature(FW_FEATURE_MULTITCE)) {
>> +		unsigned long tceshift = be32_to_cpu(maprange->tce_shift);
>> +		unsigned long dmastart = (start_pfn << PAGE_SHIFT) +
>> +				be64_to_cpu(maprange->dma_base);
>> +		unsigned long tcenum = dmastart >> tceshift;
>> +		unsigned long npages = num_pfn << PAGE_SHIFT >>
>> +				be32_to_cpu(maprange->tce_shift);
> 
> Could use the tceshift variable here.


True, overlooked.
Thanks for the reviews!


> 
>> +		void *uaddr = __va(start_pfn << PAGE_SHIFT);
>> +
>> +		return tce_build_pSeriesLP(be32_to_cpu(maprange->liobn),
>> +				tcenum, tceshift, npages, (unsigned long) uaddr,
>> +				DMA_BIDIRECTIONAL, 0);
>> +	}
>> +
>>  	local_irq_disable();	/* to protect tcep and the page behind it */
>>  	tcep = __this_cpu_read(tce_page);
> 
> 
> --
> Thiago Jung Bauermann
> IBM Linux Technology Center
> 

-- 
Alexey