On Tue, 2021-04-13 at 18:24 +1000, Alexey Kardashevskiy wrote: > > 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. >
So you would recommend reading "ranges" with of_get_property() and using the second entry (cells 7 - 13) in this point, get base & size to make sure it does not map anything here? (should have no effect if the value does not intersect with the DMA window) Thank you for reviewing! Leonardo Bras