Re: [PATCH v4 10/11] powerpc/pseries/iommu: Make use of DDW for indirect mapping

On 13/07/2021 14:36, Leonardo Brás wrote:

On Tue, 2021-05-11 at 17:57 +1000, Alexey Kardashevskiy wrote:

quoted

On 01/05/2021 02:31, Leonardo Bras wrote:

quoted

[...]
       pmem_present = dn != NULL;

@@ -1218,8 +1224,12 @@ static bool enable_ddw(struct pci_dev *dev,

struct device_node *pdn)
   
         mutex_lock(&direct_window_init_mutex);
   
-       if (find_existing_ddw(pdn, &dev->dev.archdata.dma_offset,
&len))
-               goto out_unlock;
+       if (find_existing_ddw(pdn, &dev->dev.archdata.dma_offset,
&len)) {
+               direct_mapping = (len >= max_ram_len);
+
+               mutex_unlock(&direct_window_init_mutex);
+               return direct_mapping;

Does not this break the existing case when direct_mapping==true by
skipping setting dev->dev.bus_dma_limit before returning?

Yes, it does. Good catch!
I changed it to use a flag instead of win64 for return, and now I can
use the same success exit path for both the new config and the config
found in list. (out_unlock)

quoted

quoted

+       }
   
         /*
          * If we already went through this for a previous function of

@@ -1298,7 +1308,6 @@ static bool enable_ddw(struct pci_dev *dev,

struct device_node *pdn)
                 goto out_failed;
         }
         /* verify the window * number of ptes will map the partition
*/
-       /* check largest block * page size > max memory hotplug addr
*/
         /*
          * The "ibm,pmemory" can appear anywhere in the address
space.
          * Assuming it is still backed by page structs, try
MAX_PHYSMEM_BITS

@@ -1320,6 +1329,17 @@ static bool enable_ddw(struct pci_dev *dev,

struct device_node *pdn)
                         1ULL << len,
                         query.largest_available_block,
                         1ULL << page_shift);
+
+               len = order_base_2(query.largest_available_block <<
page_shift);
+               win_name = DMA64_PROPNAME;

[1] ....

quoted

+       } else {
+               direct_mapping = true;
+               win_name = DIRECT64_PROPNAME;
+       }
+
+       /* DDW + IOMMU on single window may fail if there is any
allocation */
+       if (default_win_removed && !direct_mapping &&
iommu_table_in_use(tbl)) {
+               dev_dbg(&dev->dev, "current IOMMU table in use, can't
be replaced.\n");

... remove !direct_mapping and move to [1]?

sure, done!

quoted

quoted

                 goto out_failed;
         }
   

@@ -1331,8 +1351,7 @@ static bool enable_ddw(struct pci_dev *dev,

struct device_node *pdn)
                   create.liobn, dn);
   
         win_addr = ((u64)create.addr_hi << 32) | create.addr_lo;
-       win64 = ddw_property_create(DIRECT64_PROPNAME, create.liobn,
win_addr,
-                                   page_shift, len);
+       win64 = ddw_property_create(win_name, create.liobn, win_addr,
page_shift, len);
         if (!win64) {
                 dev_info(&dev->dev,
                          "couldn't allocate property, property name,
or value\n");

@@ -1350,12 +1369,47 @@ static bool enable_ddw(struct pci_dev *dev,

struct device_node *pdn)
         if (!window)
                 goto out_del_prop;
   
-       ret = walk_system_ram_range(0, memblock_end_of_DRAM() >>
PAGE_SHIFT,
-                       win64->value,
tce_setrange_multi_pSeriesLP_walk);
-       if (ret) {
-               dev_info(&dev->dev, "failed to map direct window for
%pOF: %d\n",
-                        dn, ret);
-               goto out_del_list;
+       if (direct_mapping) {
+               /* DDW maps the whole partition, so enable direct DMA
mapping */
+               ret = walk_system_ram_range(0, memblock_end_of_DRAM()

quoted

quoted

PAGE_SHIFT,

+                                           win64->value,
tce_setrange_multi_pSeriesLP_walk);
+               if (ret) {
+                       dev_info(&dev->dev, "failed to map direct
window for %pOF: %d\n",
+                                dn, ret);
+                       goto out_del_list;
+               }
+       } else {
+               struct iommu_table *newtbl;
+               int i;
+
+               /* New table for using DDW instead of the default DMA
window */
+               newtbl = iommu_pseries_alloc_table(pci->phb->node);
+               if (!newtbl) {
+                       dev_dbg(&dev->dev, "couldn't create new IOMMU
table\n");
+                       goto out_del_list;
+               }
+
+               for (i = 0; i < ARRAY_SIZE(pci->phb->mem_resources);
i++) {
+                       const unsigned long mask = IORESOURCE_MEM_64
| IORESOURCE_MEM;
+
+                       /* Look for MMIO32 */
+                       if ((pci->phb->mem_resources[i].flags & mask)
== IORESOURCE_MEM)
+                               break;

What if there is no IORESOURCE_MEM? pci->phb->mem_resources[i].start
below will have garbage.

Yeah, that makes sense. I will add this lines after 'for':

if (i == ARRAY_SIZE(pci->phb->mem_resources)) {
  iommu_tce_table_put(newtbl);
  goto out_del_list;
}

What do you think?

Move this and that "for" before iommu_pseries_alloc_table() so you won't 
need to free if there is no IORESOURCE_MEM.

quoted

quoted

+               }
+
+               _iommu_table_setparms(newtbl, pci->phb->bus->number,
create.liobn, win_addr,
+                                     1UL << len, page_shift, 0,
&iommu_table_lpar_multi_ops);
+               iommu_init_table(newtbl, pci->phb->node, pci->phb-

quoted

mem_resources[i].start,

+                                pci->phb->mem_resources[i].end);
+
+               if (default_win_removed)
+                       iommu_tce_table_put(tbl);

iommu_tce_table_put() should have been called when the window was
removed.

Also after some thinking - what happens if there were 2 devices in the
PE and one requested 64bit DMA? This will only update
set_iommu_table_base() for the 64bit one but not for the other.

I think the right thing to do is:

1. check if table[0] is in use, if yes => fail (which this does
already)

2. remove default dma window but keep the iommu_table struct with one
change - set it_size to 0 (and free it_map) so the 32bit device won't
look at a stale structure and think there is some window (imaginery
situation for phyp but easy to recreate in qemu).

3. use table[1] for newly created indirect DDW window.

4. change get_iommu_table_base() to return a usable table (or may be
not
needed?).

If this sounds reasonable (does it?),

Looks ok, I will try your suggestion.
I was not aware of how pci->table_group->tables[] worked, so I replaced
pci->table_group->tables[0] with the new tbl, while moving the older in
pci->table_group->tables[1].

pci->table_group->tables[0] is window#0 at @0.
pci->table_group->tables[1] is window#1 at 0x0800.0000.0000.0000. That 
is all :)

pseries does not use tables[1] but powernv does (by VFIO only though).

(4) get_iommu_table_base() does not seem to need update, as it returns
the tlb set by set_iommu_table_base() which is already called in the
!direct_mapping path in current patch.

Sounds right.

quoted

  the question is now if you have
time to do that and the hardware to test that, or I'll have to finish
the work :)

Sorry, for some reason part of this got lost in Evolution mail client.

If possible, I do want to finish this work, and I am talking to IBM
Virt people in order to get testing HW.

Even I struggle to find a powervm machine :)

quoted

quoted

+               else
+                       pci->table_group->tables[1] = tbl;

What is this for?

I was thinking of adding the older table to pci->table_group->tables[1]
while keeping the newer table on pci->table_group->tables[0].
This did work, but I think your suggestion may work better.

Best regards,
Leonardo Bras

-- 
Alexey