Re: [PATCH v2 13/14] powerpc/pseries/iommu: Make use of DDW for indirect mapping

[Alexey Kardashevskiy <aik@ozlabs.ru>]

On 13/04/2021 17:58, Leonardo Bras wrote:
> On Tue, 2021-04-13 at 17:41 +1000, Alexey Kardashevskiy wrote:
>>
>> On 13/04/2021 17:33, Leonardo Bras wrote:
>>> On Tue, 2021-04-13 at 17:18 +1000, Alexey Kardashevskiy wrote:
>>>>
>>>> On 13/04/2021 15:49, Leonardo Bras wrote:
>>>>> Thanks for the feedback!
>>>>>
>>>>> On Tue, 2020-09-29 at 13:56 +1000, Alexey Kardashevskiy wrote:
>>>>>>> -static bool find_existing_ddw(struct device_node *pdn, u64 *dma_addr)
>>>>>>> +static phys_addr_t ddw_memory_hotplug_max(void)
>>>>>>
>>>>>>
>>>>>> Please, forward declaration or a separate patch; this creates
>>>>>> unnecessary noise to the actual change.
>>>>>>
>>>>>
>>>>> Sure, done!
>>>>>
>>>>>>
>>>>>>> +		_iommu_table_setparms(tbl, pci->phb->bus->number, create.liobn, win_addr,
>>>>>>> +				      1UL << len, page_shift, 0, &iommu_table_lpar_multi_ops);
>>>>>>> +		iommu_init_table(tbl, pci->phb->node, 0, 0);
>>>>>>
>>>>>>
>>>>>> It is 0,0 only if win_addr>0 which is not the QEMU case.
>>>>>>
>>>>>
>>>>> Oh, ok.
>>>>> I previously though it was ok to use 0,0 here as any other usage in
>>>>> this file was also 0,0.
>>>>>
>>>>> What should I use to get the correct parameters? Use the previous tbl
>>>>> it_reserved_start and tbl->it_reserved_end is enough?
>>>>
>>>> depends on whether you carry reserved start/end even if they are outside
>>>> of the dma window.
>>>>
>>>
>>> Oh, that makes sense.
>>> On a previous patch (5/14 IIRC), I changed the behavior to only store
>>> the valid range on tbl, but now I understand why it's important to
>>> store the raw value.
>>>
>>> Ok, I will change it back so the reserved range stays in tbl even if it
>>> does not intersect with the DMA window. This way I can reuse the values
>>> in case of indirect mapping with DDW.
>>>
>>> Is that ok? Are the reserved values are supposed to stay the same after
>>> changing from Default DMA window to DDW?
>>
>> I added them to know what bits in it_map to ignore when checking if
>> there is any active user of the table. If you have non zero reserved
>> start/end but they do not affect it_map, then it is rather weird way to
>> carry reserved start/end from DDW to no-DDW.
>>
> 
> Ok, agreed.
> 
>>   May be do not set these at
>> all for DDW with window start at 1<<59 and when going back to no-DDW (or
>> if DDW starts at 0) - just set them from MMIO32, just as they are
>> initialized in the first place.
>>
> 
> If I get it correctly from pci_of_scan.c, MMIO32 = {0, 32MB}, is that
> correct?

No, under QEMU it is 0x8000.0000-0x1.0000.0000:

/proc/device-tree/pci@800000020000000/ranges

7 cells for each resource, the second one is MMIO32 (the first is IO 
ports, the last is 64bit MMIO).

> 
> So, if DDW starts at any value in this range (most probably at zero),
> we should remove the rest, is that correct?
> 
> Could it always use iommu_init_table(..., 0, 32MB) here, so it always
> reserve any part of the DMA window that's in this range? Ot there may
> be other reserved values range?
> 
>> and when going back to no-DDW
> 
> After iommu_init_table() there should be no failure, so it looks like
> there is no 'going back to no-DDW'. Am I missing something?

Well, a random driver could request 32bit DMA and if the new window is 
1:1, then it would break but this does not seem to happen and we do not 
support it anyway so no loss here.


-- 
Alexey