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

[Thiago Jung Bauermann <bauerman@linux.ibm.com>]

Alexey Kardashevskiy <aik@ozlabs.ru> writes:

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

Ah, I hadn't noticed that the cast was already in the original code.

>>> @@ -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!

Thank you for the patches!

--
Thiago Jung Bauermann
IBM Linux Technology Center