Re: [PATCH v4 10/11] powerpc/pseries/iommu: Make use of DDW for indirect mapping
On Wed, 2021-07-14 at 18:38 +1000, Alexey Kardashevskiy wrote:
> > 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.
Done!
>
>
> >
> >
> > >
> > >
> > > > + }
> > > > +
> > > > + _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-
> > > > > 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....
> That
> is all :)
>
> pseries does not use tables[1] but powernv does (by VFIO only
> though).
Thanks! This helped a lot!
>
>
> > (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.
>
> >
> > > 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 :)
>
> > >
> > >
> > > > + 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
> >
> >
>
Thanks a lot for reviewing Alexey!
I will send a v5 soon.
Best regards,
Leonardo Bras
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:
On 01/05/2021 02:31, Leonardo Bras wrote:
[...]
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)
+ }
/*
* 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]
+ } 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!
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()
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
Re: [PATCH v4 10/11] powerpc/pseries/iommu: Make use of DDW for indirect mapping
On Tue, 2021-05-11 at 17:57 +1000, Alexey Kardashevskiy wrote:
>
>
> On 01/05/2021 02:31, Leonardo Bras wrote:
> > [...]
> > 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)
>
>
> > + }
> >
> > /*
> > * 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]
>
>
> > + } 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!
>
>
> > 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()
> > >> 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 */
> > +
Re: [PATCH v4 10/11] powerpc/pseries/iommu: Make use of DDW for indirect mapping
On 01/05/2021 02:31, Leonardo Bras wrote:
So far it's assumed possible to map the guest RAM 1:1 to the bus, which
works with a small number of devices. SRIOV changes it as the user can
configure hundreds VFs and since phyp preallocates TCEs and does not
allow IOMMU pages bigger than 64K, it has to limit the number of TCEs
per a PE to limit waste of physical pages.
As of today, if the assumed direct mapping is not possible, DDW creation
is skipped and the default DMA window "ibm,dma-window" is used instead.
By using DDW, indirect mapping can get more TCEs than available for the
default DMA window, and also get access to using much larger pagesizes
(16MB as implemented in qemu vs 4k from default DMA window), causing a
significant increase on the maximum amount of memory that can be IOMMU
mapped at the same time.
Indirect mapping will only be used if direct mapping is not a
possibility.
For indirect mapping, it's necessary to re-create the iommu_table with
the new DMA window parameters, so iommu_alloc() can use it.
Removing the default DMA window for using DDW with indirect mapping
is only allowed if there is no current IOMMU memory allocated in
the iommu_table. enable_ddw() is aborted otherwise.
Even though there won't be both direct and indirect mappings at the
same time, we can't reuse the DIRECT64_PROPNAME property name, or else
an older kexec()ed kernel can assume direct mapping, and skip
iommu_alloc(), causing undesirable behavior.
So a new property name DMA64_PROPNAME "linux,dma64-ddr-window-info"
was created to represent a DDW that does not allow direct mapping.
Signed-off-by: Leonardo Bras
---
arch/powerpc/platforms/pseries/iommu.c | 87 +-
1 file changed, 72 insertions(+), 15 deletions(-)
diff --git a/arch/powerpc/platforms/pseries/iommu.c
b/arch/powerpc/platforms/pseries/iommu.c
index de54ddd9decd..572879af0211 100644
--- a/arch/powerpc/platforms/pseries/iommu.c
+++ b/arch/powerpc/platforms/pseries/iommu.c
@@ -53,6 +53,7 @@ enum {
DDW_EXT_QUERY_OUT_SIZE = 2
};
+static phys_addr_t ddw_memory_hotplug_max(void);
#ifdef CONFIG_IOMMU_API
static int tce_exchange_pseries(struct iommu_table *tbl, long index, unsigned
long *tce,
enum dma_data_direction *direction, bool
realmode);
@@ -380,6 +381,7 @@ static DEFINE_SPINLOCK(direct_window_list_lock);
/* protects initializing window twice for same device */
static DEFINE_MUTEX(direct_window_init_mutex);
#define DIRECT64_PROPNAME "linux,direct64-ddr-window-info"
+#define DMA64_PROPNAME "linux,dma64-ddr-window-info"
static int tce_clearrange_multi_pSeriesLP(unsigned long start_pfn,
unsigned long num_pfn, const void *arg)
@@ -918,6 +920,7 @@ static int find_existing_ddw_windows(void)
return 0;
find_existing_ddw_windows_named(DIRECT64_PROPNAME);
+ find_existing_ddw_windows_named(DMA64_PROPNAME);
return 0;
}
@@ -1207,10 +1210,13 @@ static bool enable_ddw(struct pci_dev *dev, struct
device_node *pdn)
struct device_node *dn;
u32 ddw_avail[DDW_APPLICABLE_SIZE];
struct direct_window *window;
+ const char *win_name;
struct property *win64 = NULL;
struct failed_ddw_pdn *fpdn;
- bool default_win_removed = false;
+ bool default_win_removed = false, direct_mapping = false;
bool pmem_present;
+ struct pci_dn *pci = PCI_DN(pdn);
+ struct iommu_table *tbl = pci->table_group->tables[0];
dn = of_find_node_by_type(NULL, "ibm,pmemory");
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?
+ }
/*
* 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);
+
+
[PATCH v4 10/11] powerpc/pseries/iommu: Make use of DDW for indirect mapping
So far it's assumed possible to map the guest RAM 1:1 to the bus, which
works with a small number of devices. SRIOV changes it as the user can
configure hundreds VFs and since phyp preallocates TCEs and does not
allow IOMMU pages bigger than 64K, it has to limit the number of TCEs
per a PE to limit waste of physical pages.
As of today, if the assumed direct mapping is not possible, DDW creation
is skipped and the default DMA window "ibm,dma-window" is used instead.
By using DDW, indirect mapping can get more TCEs than available for the
default DMA window, and also get access to using much larger pagesizes
(16MB as implemented in qemu vs 4k from default DMA window), causing a
significant increase on the maximum amount of memory that can be IOMMU
mapped at the same time.
Indirect mapping will only be used if direct mapping is not a
possibility.
For indirect mapping, it's necessary to re-create the iommu_table with
the new DMA window parameters, so iommu_alloc() can use it.
Removing the default DMA window for using DDW with indirect mapping
is only allowed if there is no current IOMMU memory allocated in
the iommu_table. enable_ddw() is aborted otherwise.
Even though there won't be both direct and indirect mappings at the
same time, we can't reuse the DIRECT64_PROPNAME property name, or else
an older kexec()ed kernel can assume direct mapping, and skip
iommu_alloc(), causing undesirable behavior.
So a new property name DMA64_PROPNAME "linux,dma64-ddr-window-info"
was created to represent a DDW that does not allow direct mapping.
Signed-off-by: Leonardo Bras
---
arch/powerpc/platforms/pseries/iommu.c | 87 +-
1 file changed, 72 insertions(+), 15 deletions(-)
diff --git a/arch/powerpc/platforms/pseries/iommu.c
b/arch/powerpc/platforms/pseries/iommu.c
index de54ddd9decd..572879af0211 100644
--- a/arch/powerpc/platforms/pseries/iommu.c
+++ b/arch/powerpc/platforms/pseries/iommu.c
@@ -53,6 +53,7 @@ enum {
DDW_EXT_QUERY_OUT_SIZE = 2
};
+static phys_addr_t ddw_memory_hotplug_max(void);
#ifdef CONFIG_IOMMU_API
static int tce_exchange_pseries(struct iommu_table *tbl, long index, unsigned
long *tce,
enum dma_data_direction *direction, bool
realmode);
@@ -380,6 +381,7 @@ static DEFINE_SPINLOCK(direct_window_list_lock);
/* protects initializing window twice for same device */
static DEFINE_MUTEX(direct_window_init_mutex);
#define DIRECT64_PROPNAME "linux,direct64-ddr-window-info"
+#define DMA64_PROPNAME "linux,dma64-ddr-window-info"
static int tce_clearrange_multi_pSeriesLP(unsigned long start_pfn,
unsigned long num_pfn, const void *arg)
@@ -918,6 +920,7 @@ static int find_existing_ddw_windows(void)
return 0;
find_existing_ddw_windows_named(DIRECT64_PROPNAME);
+ find_existing_ddw_windows_named(DMA64_PROPNAME);
return 0;
}
@@ -1207,10 +1210,13 @@ static bool enable_ddw(struct pci_dev *dev, struct
device_node *pdn)
struct device_node *dn;
u32 ddw_avail[DDW_APPLICABLE_SIZE];
struct direct_window *window;
+ const char *win_name;
struct property *win64 = NULL;
struct failed_ddw_pdn *fpdn;
- bool default_win_removed = false;
+ bool default_win_removed = false, direct_mapping = false;
bool pmem_present;
+ struct pci_dn *pci = PCI_DN(pdn);
+ struct iommu_table *tbl = pci->table_group->tables[0];
dn = of_find_node_by_type(NULL, "ibm,pmemory");
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;
+ }
/*
* 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;
+ } else {
+ direct_mapping = true;
+
