From: Leonardo Bras <hidden> Date: 2020-06-24 06:24:28
There are some devices that only allow 1 DMA window to exist at a time,
and in those cases, a DDW is never created to them, since the default DMA
window keeps using this resource.
LoPAR recommends this procedure:
1. Remove the default DMA window,
2. Query for which configs the DDW can be created,
3. Create a DDW.
Patch #1:
Create defines for outputs of ibm,ddw-applicable, so it's easier to
identify them.
Patch #2:
- After LoPAR level 2.8, there is an extension that can make
ibm,query-pe-dma-windows to have 6 outputs instead of 5. This changes the
order of the outputs, and that can cause some trouble.
- query_ddw() was updated to check how many outputs the
ibm,query-pe-dma-windows is supposed to have, update the rtas_call() and
deal correctly with the outputs in both cases.
- This patch looks somehow unrelated to the series, but it can avoid future
problems on DDW creation.
Patch #3 moves the window-removing code from remove_ddw() to
remove_dma_window(), creating a way to delete any DMA window, so it can be
used to delete the default DMA window.
Patch #4 makes use of the remove_dma_window() from patch #3 to remove the
default DMA window before query_ddw(). It also implements a new rtas call
to recover the default DMA window, in case anything fails after it was
removed, and a DDW couldn't be created.
Patch #5:
Instead of destroying the created DDW if it doesn't map the whole
partition, make use of it instead of the default DMA window.
Patch #6:
Changes the way iommu_bypass_supported_pSeriesLP() check for
iommu_bypass: instead of checking the address returned by enable_ddw(),
it checks a new output value that reflects if the DDW created maps
the whole partition.
All patches were tested into an LPAR with an Ethernet VF:
4005:01:00.0 Ethernet controller: Mellanox Technologies MT27700 Family
[ConnectX-4 Virtual Function]
---
Changes since v1:
- Add defines for ibm,ddw-applicable and ibm,ddw-extensions outputs
- Merge aux function query_ddw_out_sz() into query_ddw()
- Merge reset_dma_window() patch (prev. #2) into remove default DMA
window patch (#4).
- Keep device_node *np name instead of using pdn in remove_*()
- Rename 'device_node *pdn' into 'parent' in new functions
- Rename dfl_win to default_win
- Only remove the default DMA window if there is no window available
in first query.
- Check if default DMA window can be restored before removing it.
- Fix 'unitialized use' (found by travis mpe:ci-test)
- New patches #5 and #6
Special thanks for Alexey Kardashevskiy and Oliver O'Halloran for
the feedback provided!
Leonardo Bras (6):
powerpc/pseries/iommu: Create defines for operations in
ibm,ddw-applicable
powerpc/pseries/iommu: Update call to ibm,query-pe-dma-windows
powerpc/pseries/iommu: Move window-removing part of remove_ddw into
remove_dma_window
powerpc/pseries/iommu: Remove default DMA window before creating DDW
powerpc/pseries/iommu: Make use of DDW even if it does not map the
partition
powerpc/pseries/iommu: Avoid errors when DDW starts at 0x00
arch/powerpc/platforms/pseries/iommu.c | 239 ++++++++++++++++++-------
1 file changed, 176 insertions(+), 63 deletions(-)
--
2.25.4
From: Leonardo Bras <hidden> Date: 2020-06-24 06:24:31
Create defines to help handling ibm,ddw-applicable values, avoiding
confusion about the index of given operations.
Signed-off-by: Leonardo Bras <redacted>
---
arch/powerpc/platforms/pseries/iommu.c | 40 +++++++++++++++-----------
1 file changed, 23 insertions(+), 17 deletions(-)
@@ -798,15 +803,15 @@ static void remove_ddw(struct device_node *np, bool remove_prop)pr_debug("%pOF successfully cleared tces in window.\n",np);-ret=rtas_call(ddw_avail[2],1,1,NULL,liobn);+ret=rtas_call(ddw_avail[DDW_REMOVE_PE_DMA_WIN],1,1,NULL,liobn);if(ret)pr_warn("%pOF: failed to remove direct window: rtas returned ""%d to ibm,remove-pe-dma-window(%x) %llx\n",-np,ret,ddw_avail[2],liobn);+np,ret,ddw_avail[DDW_REMOVE_PE_DMA_WIN],liobn);elsepr_debug("%pOF: successfully removed direct window: rtas returned ""%d to ibm,remove-pe-dma-window(%x) %llx\n",-np,ret,ddw_avail[2],liobn);+np,ret,ddw_avail[DDW_REMOVE_PE_DMA_WIN],liobn);delprop:if(remove_prop)
@@ -889,11 +894,11 @@ static int query_ddw(struct pci_dev *dev, const u32 *ddw_avail,buid=pdn->phb->buid;cfg_addr=((pdn->busno<<16)|(pdn->devfn<<8));-ret=rtas_call(ddw_avail[0],3,5,(u32*)query,-cfg_addr,BUID_HI(buid),BUID_LO(buid));+ret=rtas_call(ddw_avail[DDW_QUERY_PE_DMA_WIN],3,5,(u32*)query,+cfg_addr,BUID_HI(buid),BUID_LO(buid));dev_info(&dev->dev,"ibm,query-pe-dma-windows(%x) %x %x %x"-" returned %d\n",ddw_avail[0],cfg_addr,BUID_HI(buid),-BUID_LO(buid),ret);+" returned %d\n",ddw_avail[DDW_QUERY_PE_DMA_WIN],cfg_addr,+BUID_HI(buid),BUID_LO(buid),ret);returnret;}
@@ -920,15 +925,16 @@ static int create_ddw(struct pci_dev *dev, const u32 *ddw_avail,do{/* extra outputs are LIOBN and dma-addr (hi, lo) */-ret=rtas_call(ddw_avail[1],5,4,(u32*)create,-cfg_addr,BUID_HI(buid),BUID_LO(buid),-page_shift,window_shift);+ret=rtas_call(ddw_avail[DDW_CREATE_PE_DMA_WIN],5,4,+(u32*)create,cfg_addr,BUID_HI(buid),+BUID_LO(buid),page_shift,window_shift);}while(rtas_busy_delay(ret));dev_info(&dev->dev,"ibm,create-pe-dma-window(%x) %x %x %x %x %x returned %d "-"(liobn = 0x%x starting addr = %x %x)\n",ddw_avail[1],-cfg_addr,BUID_HI(buid),BUID_LO(buid),page_shift,-window_shift,ret,create->liobn,create->addr_hi,create->addr_lo);+"(liobn = 0x%x starting addr = %x %x)\n",+ddw_avail[DDW_CREATE_PE_DMA_WIN],cfg_addr,BUID_HI(buid),+BUID_LO(buid),page_shift,window_shift,ret,create->liobn,+create->addr_hi,create->addr_lo);returnret;}
From: Leonardo Bras <hidden> Date: 2020-06-24 06:24:35
From LoPAR level 2.8, "ibm,ddw-extensions" index 3 can make the number of
outputs from "ibm,query-pe-dma-windows" go from 5 to 6.
This change of output size is meant to expand the address size of
largest_available_block PE TCE from 32-bit to 64-bit, which ends up
shifting page_size and migration_capable.
This ends up requiring the update of
ddw_query_response->largest_available_block from u32 to u64, and manually
assigning the values from the buffer into this struct, according to
output size.
Signed-off-by: Leonardo Bras <redacted>
---
arch/powerpc/platforms/pseries/iommu.c | 57 +++++++++++++++++++++-----
1 file changed, 47 insertions(+), 10 deletions(-)
From: Leonardo Bras <hidden> Date: 2020-06-24 06:24:40
Move the window-removing part of remove_ddw into a new function
(remove_dma_window), so it can be used to remove other DMA windows.
It's useful for removing DMA windows that don't create DIRECT64_PROPNAME
property, like the default DMA window from the device, which uses
"ibm,dma-window".
Signed-off-by: Leonardo Bras <redacted>
---
arch/powerpc/platforms/pseries/iommu.c | 45 +++++++++++++++-----------
1 file changed, 27 insertions(+), 18 deletions(-)
@@ -776,25 +776,14 @@ static int __init disable_ddw_setup(char *str)early_param("disable_ddw",disable_ddw_setup);-staticvoidremove_ddw(structdevice_node*np,boolremove_prop)+staticvoidremove_dma_window(structdevice_node*np,u32*ddw_avail,+structproperty*win){structdynamic_dma_window_prop*dwp;-structproperty*win64;-u32ddw_avail[DDW_APPLICABLE_SIZE];u64liobn;-intret=0;--ret=of_property_read_u32_array(np,"ibm,ddw-applicable",-&ddw_avail[0],DDW_APPLICABLE_SIZE);--win64=of_find_property(np,DIRECT64_PROPNAME,NULL);-if(!win64)-return;--if(ret||win64->length<sizeof(*dwp))-gotodelprop;+intret;-dwp=win64->value;+dwp=win->value;liobn=(u64)be32_to_cpu(dwp->liobn);/* clear the whole window, note the arg is in kernel pages */
@@ -816,10 +805,30 @@ static void remove_ddw(struct device_node *np, bool remove_prop)pr_debug("%pOF: successfully removed direct window: rtas returned ""%d to ibm,remove-pe-dma-window(%x) %llx\n",np,ret,ddw_avail[DDW_REMOVE_PE_DMA_WIN],liobn);+}++staticvoidremove_ddw(structdevice_node*np,boolremove_prop)+{+structproperty*win;+u32ddw_avail[DDW_APPLICABLE_SIZE];+intret=0;++ret=of_property_read_u32_array(np,"ibm,ddw-applicable",+&ddw_avail[0],DDW_APPLICABLE_SIZE);+if(ret)+return;++win=of_find_property(np,DIRECT64_PROPNAME,NULL);+if(!win)+return;++if(win->length>=sizeof(structdynamic_dma_window_prop))+remove_dma_window(np,ddw_avail,win);++if(!remove_prop)+return;-delprop:-if(remove_prop)-ret=of_remove_property(np,win64);+ret=of_remove_property(np,win);if(ret)pr_warn("%pOF: failed to remove direct window property: %d\n",np,ret);
From: Leonardo Bras <hidden> Date: 2020-06-24 06:24:43
On LoPAR "DMA Window Manipulation Calls", it's recommended to remove the
default DMA window for the device, before attempting to configure a DDW,
in order to make the maximum resources available for the next DDW to be
created.
This is a requirement for some devices to use DDW, given they only
allow one DMA window.
If setting up a new DDW fails anywhere after the removal of this
default DMA window, it's needed to restore the default DMA window.
For this, an implementation of ibm,reset-pe-dma-windows rtas call is
needed:
Platforms supporting the DDW option starting with LoPAR level 2.7 implement
ibm,ddw-extensions. The first extension available (index 2) carries the
token for ibm,reset-pe-dma-windows rtas call, which is used to restore
the default DMA window for a device, if it has been deleted.
It does so by resetting the TCE table allocation for the PE to it's
boot time value, available in "ibm,dma-window" device tree node.
Signed-off-by: Leonardo Bras <redacted>
---
arch/powerpc/platforms/pseries/iommu.c | 70 ++++++++++++++++++++++----
1 file changed, 61 insertions(+), 9 deletions(-)
@@ -1096,15 +1130,31 @@ static u64 enable_ddw(struct pci_dev *dev, struct device_node *pdn)if(ret!=0)gotoout_failed;+/*+*Ifthereisnowindowavailable,removethedefaultDMAwindow,+*ifit'spresent.Thiswillmakealltheresourcesavailabletothe+*newDDWwindow.+*Ifanythingfailsafterthis,weneedtorestoreit,soalsocheck+*forextensionspresence.+*/if(query.windows_available==0){-/*-*noadditionalwindowsareavailableforthisdevice.-*Wemightbeabletoreallocatetheexistingwindow,-*tradinginforalargerpagesize.-*/-dev_dbg(&dev->dev,"no free dynamic windows");-gotoout_failed;+default_win=of_find_property(pdn,"ibm,dma-window",NULL);+ddw_ext=of_find_property(pdn,"ibm,ddw-extensions",NULL);+if(default_win&&ddw_ext)+remove_dma_window(pdn,ddw_avail,default_win);++/* Query again, to check if the window is available */+ret=query_ddw(dev,ddw_avail,&query,pdn);+if(ret!=0)+gotoout_failed;++if(query.windows_available==0){+/* no windows are available for this device. */+dev_dbg(&dev->dev,"no free dynamic windows");+gotoout_failed;+}}+if(query.page_size&4){page_shift=24;/* 16MB */}elseif(query.page_size&2){
From: Leonardo Bras <hidden> Date: 2020-06-24 06:24:47
As of today, if a DDW is created and can't map the whole partition, it's
removed and the default DMA window "ibm,dma-window" is used instead.
Usually this DDW is bigger than the default DMA window, so it would be
better to make use of it instead.
Signed-off-by: Leonardo Bras <redacted>
---
arch/powerpc/platforms/pseries/iommu.c | 28 +++++++++++++++++---------
1 file changed, 19 insertions(+), 9 deletions(-)
@@ -699,8 +699,13 @@ static void pci_dma_bus_setup_pSeriesLP(struct pci_bus *bus)break;}+/* If there is a DDW available, use it instead */+alt_dma_window=of_get_property(pdn,DIRECT64_PROPNAME,NULL);+if(alt_dma_window)+dma_window=alt_dma_window;+if(dma_window==NULL){-pr_debug(" no ibm,dma-window property !\n");+pr_debug(" no ibm,dma-window nor linux,direct64-ddr-window-info property !\n");return;}
@@ -1166,16 +1171,19 @@ static u64 enable_ddw(struct pci_dev *dev, struct device_node *pdn)query.page_size);gotoout_failed;}+/* verify the window * number of ptes will map the partition */-/* check largest block * page size > max memory hotplug addr */max_addr=ddw_memory_hotplug_max();if(query.largest_available_block<(max_addr>>page_shift)){-dev_dbg(&dev->dev,"can't map partition max 0x%llx with %llu "-"%llu-sized pages\n",max_addr,query.largest_available_block,-1ULL<<page_shift);-gotoout_failed;+dev_dbg(&dev->dev,"can't map partition max 0x%llx with %llu %llu-sized pages\n",+max_addr,query.largest_available_block,+1ULL<<page_shift);++len=order_base_2(query.largest_available_block<<page_shift);+}else{+len=order_base_2(max_addr);}-len=order_base_2(max_addr);+win64=kzalloc(sizeof(structproperty),GFP_KERNEL);if(!win64){dev_info(&dev->dev,
@@ -1229,7 +1237,9 @@ static u64 enable_ddw(struct pci_dev *dev, struct device_node *pdn)list_add(&window->list,&direct_window_list);spin_unlock(&direct_window_list_lock);-dma_addr=be64_to_cpu(ddwprop->dma_base);+/* Only returns the dma_addr if DDW maps the whole partition */+if(len==order_base_2(max_addr))+dma_addr=be64_to_cpu(ddwprop->dma_base);gotoout_unlock;out_free_window:
From: Leonardo Bras <hidden> Date: 2020-06-24 06:24:51
As of today, enable_ddw() will return a non-null DMA address if the
created DDW maps the whole partition. If the address is valid,
iommu_bypass_supported_pSeriesLP() will consider iommu bypass enabled.
This can cause some trouble if the DDW happens to start at 0x00.
Instead if checking if the address is non-null, check directly if
the DDW maps the whole partition, so it can bypass iommu.
Signed-off-by: Leonardo Bras <redacted>
---
arch/powerpc/platforms/pseries/iommu.c | 17 ++++++++---------
1 file changed, 8 insertions(+), 9 deletions(-)
@@ -1237,9 +1238,9 @@ static u64 enable_ddw(struct pci_dev *dev, struct device_node *pdn)list_add(&window->list,&direct_window_list);spin_unlock(&direct_window_list_lock);-/* Only returns the dma_addr if DDW maps the whole partition */if(len==order_base_2(max_addr))-dma_addr=be64_to_cpu(ddwprop->dma_base);+*maps_partition=true;+dma_addr=be64_to_cpu(ddwprop->dma_base);gotoout_unlock;out_free_window:
@@ -1324,6 +1325,7 @@ static bool iommu_bypass_supported_pSeriesLP(struct pci_dev *pdev, u64 dma_mask){structdevice_node*dn=pci_device_to_OF_node(pdev),*pdn;const__be32*dma_window=NULL;+boolret=false;/* only attempt to use a new window if 64-bit DMA is requested */if(dma_mask<DMA_BIT_MASK(64))
From: Leonardo Bras <hidden> Date: 2020-06-24 15:54:37
On Wed, 2020-06-24 at 03:24 -0300, Leonardo Bras wrote:
Patch #5:
Instead of destroying the created DDW if it doesn't map the whole
partition, make use of it instead of the default DMA window.
Patch #6:
Changes the way iommu_bypass_supported_pSeriesLP() check for
iommu_bypass: instead of checking the address returned by enable_ddw(),
it checks a new output value that reflects if the DDW created maps
the whole partition.
Patches #5 and #6 were sent more as a RFC, but since they depend on the
series, I decided adding them here.
From: Leonardo Bras <hidden> Date: 2020-06-26 15:23:36
On Wed, 2020-06-24 at 03:24 -0300, Leonardo Bras wrote:
As of today, if a DDW is created and can't map the whole partition, it's
removed and the default DMA window "ibm,dma-window" is used instead.
Usually this DDW is bigger than the default DMA window, so it would be
better to make use of it instead.
Signed-off-by: Leonardo Bras <redacted>
---
I tested this change with a 256GB DDW which did not map the whole
partition, with a MT27700 Family [ConnectX-4 Virtual Function].
I noticed the performance improvement is about the same as using DDW
with IOMMU bypass.
64 thread write throughput: +203.0%
64 thread read throughput: +17.5%
1 thread write throughput: +20.5%
1 thread read throughput: +3.43%
Averag
e write latency: -23.0%
Average read latency: -2.26%
From: Leonardo Bras <hidden> Date: 2020-06-26 17:47:03
On Wed, 2020-06-24 at 03:24 -0300, Leonardo Bras wrote:
As of today, enable_ddw() will return a non-null DMA address if the
created DDW maps the whole partition. If the address is valid,
iommu_bypass_supported_pSeriesLP() will consider iommu bypass enabled.
This can cause some trouble if the DDW happens to start at 0x00.
Instead if checking if the address is non-null, check directly if
the DDW maps the whole partition, so it can bypass iommu.
Signed-off-by: Leonardo Bras <redacted>
This patch has a bug in it. I will rework it soon.
Please keep reviewing patches 1-5.
Best regards,
Leonardo
From: Leonardo Bras <hidden> Date: 2020-06-26 17:55:57
On Fri, 2020-06-26 at 12:23 -0300, Leonardo Bras wrote:
On Wed, 2020-06-24 at 03:24 -0300, Leonardo Bras wrote:
quoted
As of today, if a DDW is created and can't map the whole partition, it's
removed and the default DMA window "ibm,dma-window" is used instead.
Usually this DDW is bigger than the default DMA window, so it would be
better to make use of it instead.
Signed-off-by: Leonardo Bras <redacted>
---
I tested this change with a 256GB DDW which did not map the whole
partition, with a MT27700 Family [ConnectX-4 Virtual Function].
I noticed the performance improvement is about the same as using DDW
with IOMMU bypass.
64 thread write throughput: +203.0%
64 thread read throughput: +17.5%
1 thread write throughput: +20.5%
1 thread read throughput: +3.43%
Average write latency: -23.0%
Average read latency: -2.26%
The above improvements are based on the default DMA window, which is
currently used if DDW can't map the whole partition.
Those values are an average of 20 tests for each environment, 30
seconds each test.
I also did some intense testing, for 5 hour each:
64 thread write throughput
64 thread read throughput
The throughput values are stable in the whole test, and I noticed no
error on dmesg / journalctl.
On Wed, 2020-06-24 at 03:24 -0300, Leonardo Bras wrote:
quoted
As of today, enable_ddw() will return a non-null DMA address if the
created DDW maps the whole partition. If the address is valid,
iommu_bypass_supported_pSeriesLP() will consider iommu bypass enabled.
This can cause some trouble if the DDW happens to start at 0x00.
Instead if checking if the address is non-null, check directly if
the DDW maps the whole partition, so it can bypass iommu.
Signed-off-by: Leonardo Bras <redacted>
This patch has a bug in it. I will rework it soon.
As of today, if a DDW is created and can't map the whole partition, it's
removed and the default DMA window "ibm,dma-window" is used instead.
Usually this DDW is bigger than the default DMA window, so it would be
better to make use of it instead.
Signed-off-by: Leonardo Bras <redacted>
---
arch/powerpc/platforms/pseries/iommu.c | 28 +++++++++++++++++---------
1 file changed, 19 insertions(+), 9 deletions(-)
@@ -699,8 +699,13 @@ static void pci_dma_bus_setup_pSeriesLP(struct pci_bus *bus)break;}+/* If there is a DDW available, use it instead */+alt_dma_window=of_get_property(pdn,DIRECT64_PROPNAME,NULL);
It is not necessarily "direct" anymore as the name suggests, you may
want to change that. DMA64_PROPNAME, may be. Thanks,
quoted hunk
+ if (alt_dma_window)
+ dma_window = alt_dma_window;
+
if (dma_window == NULL) {
- pr_debug(" no ibm,dma-window property !\n");
+ pr_debug(" no ibm,dma-window nor linux,direct64-ddr-window-info property !\n");
return;
}
@@ -1166,16 +1171,19 @@ static u64 enable_ddw(struct pci_dev *dev, struct device_node *pdn) query.page_size); goto out_failed; }+ /* verify the window * number of ptes will map the partition */- /* check largest block * page size > max memory hotplug addr */ max_addr = ddw_memory_hotplug_max(); if (query.largest_available_block < (max_addr >> page_shift)) {- dev_dbg(&dev->dev, "can't map partition max 0x%llx with %llu "- "%llu-sized pages\n", max_addr, query.largest_available_block,- 1ULL << page_shift);- goto out_failed;+ dev_dbg(&dev->dev, "can't map partition max 0x%llx with %llu %llu-sized pages\n",+ max_addr, query.largest_available_block,+ 1ULL << page_shift);++ len = order_base_2(query.largest_available_block << page_shift);+ } else {+ len = order_base_2(max_addr); }- len = order_base_2(max_addr);+ win64 = kzalloc(sizeof(struct property), GFP_KERNEL); if (!win64) { dev_info(&dev->dev,
@@ -1229,7 +1237,9 @@ static u64 enable_ddw(struct pci_dev *dev, struct device_node *pdn) list_add(&window->list, &direct_window_list); spin_unlock(&direct_window_list_lock);- dma_addr = be64_to_cpu(ddwprop->dma_base);+ /* Only returns the dma_addr if DDW maps the whole partition */+ if (len == order_base_2(max_addr))+ dma_addr = be64_to_cpu(ddwprop->dma_base); goto out_unlock; out_free_window:
Create defines to help handling ibm,ddw-applicable values, avoiding
confusion about the index of given operations.
Signed-off-by: Leonardo Bras <redacted>
---
arch/powerpc/platforms/pseries/iommu.c | 40 +++++++++++++++-----------
1 file changed, 23 insertions(+), 17 deletions(-)
@@ -1029,7 +1035,7 @@ static u64 enable_ddw(struct pci_dev *dev, struct device_node *pdn) * the property is actually in the parent, not the PE */ ret = of_property_read_u32_array(pdn, "ibm,ddw-applicable",- &ddw_avail[0], 3);+ &ddw_avail[0], DDW_APPLICABLE_SIZE); if (ret) goto out_failed;
From LoPAR level 2.8, "ibm,ddw-extensions" index 3 can make the number of
outputs from "ibm,query-pe-dma-windows" go from 5 to 6.
This change of output size is meant to expand the address size of
largest_available_block PE TCE from 32-bit to 64-bit, which ends up
shifting page_size and migration_capable.
This ends up requiring the update of
ddw_query_response->largest_available_block from u32 to u64, and manually
assigning the values from the buffer into this struct, according to
output size.
Signed-off-by: Leonardo Bras <redacted>
---
arch/powerpc/platforms/pseries/iommu.c | 57 +++++++++++++++++++++-----
1 file changed, 47 insertions(+), 10 deletions(-)
On LoPAR "DMA Window Manipulation Calls", it's recommended to remove the
default DMA window for the device, before attempting to configure a DDW,
in order to make the maximum resources available for the next DDW to be
created.
This is a requirement for some devices to use DDW, given they only
allow one DMA window.
Devices never know about these windows, it is purely PHB's side of
things. A device can access any address on the bus, the bus can generate
an exception if there is no window behind the address OR some other
device's MMIO. We could actually create a second window in addition to
the first one and allocate bus addresses from both, we just simplifying
this by merging two separate non-adjacent windows into one.
quoted hunk
If setting up a new DDW fails anywhere after the removal of this
default DMA window, it's needed to restore the default DMA window.
For this, an implementation of ibm,reset-pe-dma-windows rtas call is
needed:
Platforms supporting the DDW option starting with LoPAR level 2.7 implement
ibm,ddw-extensions. The first extension available (index 2) carries the
token for ibm,reset-pe-dma-windows rtas call, which is used to restore
the default DMA window for a device, if it has been deleted.
It does so by resetting the TCE table allocation for the PE to it's
boot time value, available in "ibm,dma-window" device tree node.
Signed-off-by: Leonardo Bras <redacted>
---
arch/powerpc/platforms/pseries/iommu.c | 70 ++++++++++++++++++++++----
1 file changed, 61 insertions(+), 9 deletions(-)
+ ret);
+}
+
/*
* If the PE supports dynamic dma windows, and there is space for a table
* that can map all pages in a linear offset, then setup such a table,
@@ -1085,7 +1119,7 @@ static u64 enable_ddw(struct pci_dev *dev, struct device_node *pdn) if (ret) goto out_failed;- /*+ /* * Query if there is a second window of size to map the * whole partition. Query returns number of windows, largest * block assigned to PE (partition endpoint), and two bitmasks
@@ -1096,15 +1130,31 @@ static u64 enable_ddw(struct pci_dev *dev, struct device_node *pdn) if (ret != 0) goto out_failed;+ /*+ * If there is no window available, remove the default DMA window,+ * if it's present. This will make all the resources available to the+ * new DDW window.+ * If anything fails after this, we need to restore it, so also check+ * for extensions presence.+ */ if (query.windows_available == 0) {
Does phyp really always advertise 0 windows for these VFs? What is in
the largest_available_block when windows_available==0?
- /*
- * no additional windows are available for this device.
- * We might be able to reallocate the existing window,
- * trading in for a larger page size.
- */
- dev_dbg(&dev->dev, "no free dynamic windows");
- goto out_failed;
+ default_win = of_find_property(pdn, "ibm,dma-window", NULL);
+ ddw_ext = of_find_property(pdn, "ibm,ddw-extensions", NULL);
+ if (default_win && ddw_ext)
+ remove_dma_window(pdn, ddw_avail, default_win);
+
+ /* Query again, to check if the window is available */
+ ret = query_ddw(dev, ddw_avail, &query, pdn);
+ if (ret != 0)
+ goto out_failed;
+
+ if (query.windows_available == 0) {
+ /* no windows are available for this device. */
+ dev_dbg(&dev->dev, "no free dynamic windows");
+ goto out_failed;
+ }
}
+
Unrelated new empty line. Thanks,
quoted hunk
if (query.page_size & 4) {
page_shift = 24; /* 16MB */
} else if (query.page_size & 2) {
From: Leonardo Bras <hidden> Date: 2020-07-01 13:28:12
On Wed, 2020-07-01 at 18:16 +1000, Alexey Kardashevskiy wrote:
On 24/06/2020 16:24, Leonardo Bras wrote:
quoted
Create defines to help handling ibm,ddw-applicable values, avoiding
confusion about the index of given operations.
Signed-off-by: Leonardo Bras <redacted>
---
arch/powerpc/platforms/pseries/iommu.c | 40 +++++++++++++++-----------
1 file changed, 23 insertions(+), 17 deletions(-)
Thanks for the feedback!
About this (and patch #2), would it be better to use enum ?
enum {
DDW_QUERY_PE_DMA_WIN,
DDW_CREATE_PE_DMA_WIN,
DDW_REMOVE_PE_DMA_WIN,
DDW_APPLICABLE_SIZE
}
IMO, it looks better than all the defines before.
What do you think?
Best regards,
Because of the growing nature of ddw-extensions, I intentionally let
this be (DDW_EXT_QUERY_OUT_SIZE + 1). If we create a DDW_EXT_LAST, it
will be incremented in the future if more extensions come to exist.
I mean, I previously saw no reason for allocating space for extensions
after the desired one, as they won't be used here.
quoted
u64 buid;
- int ret;
+ int ret, out_sz;
+
+ /*
+ * From LoPAR level 2.8, "ibm,ddw-extensions" index 3 can rule how many
+ * output parameters ibm,query-pe-dma-windows will have, ranging from
+ * 5 to 6.
+ */
+
+ ret = of_property_read_u32_array(parent, "ibm,ddw-extensions",
+ &ddw_ext[0],
+ DDW_EXT_QUERY_OUT_SIZE + 1);
In this case, I made sure not to cross (DDW_EXT_QUERY_OUT_SIZE + 1)
while reading the extensions from the property.
What do you think about it?
Best regards,
Leonardo
From: Leonardo Bras <hidden> Date: 2020-07-01 19:48:43
On Wed, 2020-07-01 at 18:17 +1000, Alexey Kardashevskiy wrote:
On 24/06/2020 16:24, Leonardo Bras wrote:
quoted
On LoPAR "DMA Window Manipulation Calls", it's recommended to remove the
default DMA window for the device, before attempting to configure a DDW,
in order to make the maximum resources available for the next DDW to be
created.
This is a requirement for some devices to use DDW, given they only
allow one DMA window.
Devices never know about these windows, it is purely PHB's side of
things. A device can access any address on the bus, the bus can generate
an exception if there is no window behind the address OR some other
device's MMIO. We could actually create a second window in addition to
the first one and allocate bus addresses from both, we just simplifying
this by merging two separate non-adjacent windows into one.
That's interesting, I was not aware of this.
I will try to improve this commit message with this info.
Thanks for sharing!
quoted
quoted
quoted
If setting up a new DDW fails anywhere after the removal of this
default DMA window, it's needed to restore the default DMA window.
For this, an implementation of ibm,reset-pe-dma-windows rtas call is
needed:
Platforms supporting the DDW option starting with LoPAR level 2.7 implement
ibm,ddw-extensions. The first extension available (index 2) carries the
token for ibm,reset-pe-dma-windows rtas call, which is used to restore
the default DMA window for a device, if it has been deleted.
It does so by resetting the TCE table allocation for the PE to it's
boot time value, available in "ibm,dma-window" device tree node.
Signed-off-by: Leonardo Bras <redacted>
---
arch/powerpc/platforms/pseries/iommu.c | 70 ++++++++++++++++++++++----
1 file changed, 61 insertions(+), 9 deletions(-)
+ ret);
+}
+
/*
* If the PE supports dynamic dma windows, and there is space for a table
* that can map all pages in a linear offset, then setup such a table,
@@ -1085,7 +1119,7 @@ static u64 enable_ddw(struct pci_dev *dev, struct device_node *pdn) if (ret) goto out_failed;- /*+ /* * Query if there is a second window of size to map the * whole partition. Query returns number of windows, largest * block assigned to PE (partition endpoint), and two bitmasks
@@ -1096,15 +1130,31 @@ static u64 enable_ddw(struct pci_dev *dev, struct device_node *pdn) if (ret != 0) goto out_failed;+ /*+ * If there is no window available, remove the default DMA window,+ * if it's present. This will make all the resources available to the+ * new DDW window.+ * If anything fails after this, we need to restore it, so also check+ * for extensions presence.+ */ if (query.windows_available == 0) {
Does phyp really always advertise 0 windows for these VFs? What is in
the largest_available_block when windows_available==0?
For this VF, it always advertise 0 windows before removing the default
DMA window. The largest available block size is the same as after the
removal (256GB). The only value that changes after removal is the
number of available windows. Here some debug prints:
[ 3.473149] mlx5_core 4005:01:00.0: ibm,query-pe-dma-windows(53)
10000 8000000 29004005 returned 0
[ 3.473162] mlx5_core 4005:01:00.0: windows_available = 0,
largest_block = 400000, page_size = 3, migration_capable = 3
[ 3.473332] mlx5_core 4005:01:00.0: ibm,query-pe-dma-windows(53)
10000 8000000 29004005 returned 0
[ 3.473345] mlx5_core 4005:01:00.0: windows_available = 1,
largest_block = 400000, page_size = 3, migration_capable = 3
quoted
- /*
- * no additional windows are available for this device.
- * We might be able to reallocate the existing window,
- * trading in for a larger page size.
- */
- dev_dbg(&dev->dev, "no free dynamic windows");
- goto out_failed;
+ default_win = of_find_property(pdn, "ibm,dma-window", NULL);
+ ddw_ext = of_find_property(pdn, "ibm,ddw-extensions", NULL);
+ if (default_win && ddw_ext)
+ remove_dma_window(pdn, ddw_avail, default_win);
+
+ /* Query again, to check if the window is available */
+ ret = query_ddw(dev, ddw_avail, &query, pdn);
+ if (ret != 0)
+ goto out_failed;
+
+ if (query.windows_available == 0) {
+ /* no windows are available for this device. */
+ dev_dbg(&dev->dev, "no free dynamic windows");
+ goto out_failed;
+ }
}
+
Unrelated new empty line. Thanks,
Fixed!
Thank you!
quoted
if (query.page_size & 4) {
page_shift = 24; /* 16MB */
} else if (query.page_size & 2) {
From: Leonardo Bras <hidden> Date: 2020-07-01 19:57:56
On Wed, 2020-07-01 at 18:16 +1000, Alexey Kardashevskiy wrote:
On 24/06/2020 16:24, Leonardo Bras wrote:
quoted
As of today, if a DDW is created and can't map the whole partition, it's
removed and the default DMA window "ibm,dma-window" is used instead.
Usually this DDW is bigger than the default DMA window, so it would be
better to make use of it instead.
Signed-off-by: Leonardo Bras <redacted>
---
arch/powerpc/platforms/pseries/iommu.c | 28 +++++++++++++++++---------
1 file changed, 19 insertions(+), 9 deletions(-)
@@ -699,8 +699,13 @@ static void pci_dma_bus_setup_pSeriesLP(struct pci_bus *bus)break;}+/* If there is a DDW available, use it instead */+alt_dma_window=of_get_property(pdn,DIRECT64_PROPNAME,NULL);
It is not necessarily "direct" anymore as the name suggests, you may
want to change that. DMA64_PROPNAME, may be. Thanks,
Yeah, you are right.
I will change this for next version, also changing the string name to
reflect this.
-#define DIRECT64_PROPNAME "linux,direct64-ddr-window-info"
+#define DMA64_PROPNAME "linux,dma64-ddr-window-info"
Is that ok?
Thank you for helping!
From: Leonardo Bras <hidden> Date: 2020-07-01 19:59:45
On Wed, 2020-07-01 at 18:04 +1000, Alexey Kardashevskiy wrote:
On 27/06/2020 03:46, Leonardo Bras wrote:
quoted
On Wed, 2020-06-24 at 03:24 -0300, Leonardo Bras wrote:
quoted
As of today, enable_ddw() will return a non-null DMA address if the
created DDW maps the whole partition. If the address is valid,
iommu_bypass_supported_pSeriesLP() will consider iommu bypass enabled.
This can cause some trouble if the DDW happens to start at 0x00.
Instead if checking if the address is non-null, check directly if
the DDW maps the whole partition, so it can bypass iommu.
Signed-off-by: Leonardo Bras <redacted>
This patch has a bug in it. I will rework it soon.
From: Leonardo Bras <hidden> Date: 2020-07-01 23:48:55
On Wed, 2020-07-01 at 16:57 -0300, Leonardo Bras wrote:
quoted
It is not necessarily "direct" anymore as the name suggests, you may
want to change that. DMA64_PROPNAME, may be. Thanks,
Yeah, you are right.
I will change this for next version, also changing the string name to
reflect this.
-#define DIRECT64_PROPNAME "linux,direct64-ddr-window-info"
+#define DMA64_PROPNAME "linux,dma64-ddr-window-info"
Is that ok?
Thank you for helping!
In fact, there is a lot of places in this file where it's called direct
window. Should I replace everything?
Should it be in a separated patch?
Best regards,
Leonardo
Because of the growing nature of ddw-extensions, I intentionally let
this be (DDW_EXT_QUERY_OUT_SIZE + 1). If we create a DDW_EXT_LAST, it
will be incremented in the future if more extensions come to exist.
I mean, I previously saw no reason for allocating space for extensions
after the desired one, as they won't be used here.
Ah, my bad, you're right.
quoted
quoted
u64 buid;
- int ret;
+ int ret, out_sz;
+
+ /*
+ * From LoPAR level 2.8, "ibm,ddw-extensions" index 3 can rule how many
+ * output parameters ibm,query-pe-dma-windows will have, ranging from
+ * 5 to 6.
+ */
+
+ ret = of_property_read_u32_array(parent, "ibm,ddw-extensions",
+ &ddw_ext[0],
+ DDW_EXT_QUERY_OUT_SIZE + 1);
In this case, I made sure not to cross (DDW_EXT_QUERY_OUT_SIZE + 1)
while reading the extensions from the property.
What do you think about it?
I think you want something like:
static inline int ddw_read_ext(const struct device_node *np, int extnum,
u32 *ret)
{
retun of_property_read_u32_index(np, "ibm,ddw-extensions", extnum + 1, ret);
}
These "+1"'s all over the place are confusing.
--
Alexey
On Wed, 2020-07-01 at 18:16 +1000, Alexey Kardashevskiy wrote:
quoted
On 24/06/2020 16:24, Leonardo Bras wrote:
quoted
Create defines to help handling ibm,ddw-applicable values, avoiding
confusion about the index of given operations.
Signed-off-by: Leonardo Bras <redacted>
---
arch/powerpc/platforms/pseries/iommu.c | 40 +++++++++++++++-----------
1 file changed, 23 insertions(+), 17 deletions(-)
Thanks for the feedback!
About this (and patch #2), would it be better to use enum ?
enum {
DDW_QUERY_PE_DMA_WIN,
DDW_CREATE_PE_DMA_WIN,
DDW_REMOVE_PE_DMA_WIN,
DDW_APPLICABLE_SIZE
}
IMO, it looks better than all the defines before.
What do you think?
No, not really, these come from a binary interface so the reader of this
cares about absolute numbers and rather wants to see them explicitly.
--
Alexey
On Wed, 2020-07-01 at 18:17 +1000, Alexey Kardashevskiy wrote:
quoted
On 24/06/2020 16:24, Leonardo Bras wrote:
quoted
On LoPAR "DMA Window Manipulation Calls", it's recommended to remove the
default DMA window for the device, before attempting to configure a DDW,
in order to make the maximum resources available for the next DDW to be
created.
This is a requirement for some devices to use DDW, given they only
allow one DMA window.
Devices never know about these windows, it is purely PHB's side of
things. A device can access any address on the bus, the bus can generate
an exception if there is no window behind the address OR some other
device's MMIO. We could actually create a second window in addition to
the first one and allocate bus addresses from both, we just simplifying
this by merging two separate non-adjacent windows into one.
That's interesting, I was not aware of this.
I will try to improve this commit message with this info.
Thanks for sharing!
quoted
quoted
quoted
quoted
If setting up a new DDW fails anywhere after the removal of this
default DMA window, it's needed to restore the default DMA window.
For this, an implementation of ibm,reset-pe-dma-windows rtas call is
needed:
Platforms supporting the DDW option starting with LoPAR level 2.7 implement
ibm,ddw-extensions. The first extension available (index 2) carries the
token for ibm,reset-pe-dma-windows rtas call, which is used to restore
the default DMA window for a device, if it has been deleted.
It does so by resetting the TCE table allocation for the PE to it's
boot time value, available in "ibm,dma-window" device tree node.
Signed-off-by: Leonardo Bras <redacted>
---
arch/powerpc/platforms/pseries/iommu.c | 70 ++++++++++++++++++++++----
1 file changed, 61 insertions(+), 9 deletions(-)
+ ret);
+}
+
/*
* If the PE supports dynamic dma windows, and there is space for a table
* that can map all pages in a linear offset, then setup such a table,
@@ -1085,7 +1119,7 @@ static u64 enable_ddw(struct pci_dev *dev, struct device_node *pdn) if (ret) goto out_failed;- /*+ /* * Query if there is a second window of size to map the * whole partition. Query returns number of windows, largest * block assigned to PE (partition endpoint), and two bitmasks
@@ -1096,15 +1130,31 @@ static u64 enable_ddw(struct pci_dev *dev, struct device_node *pdn) if (ret != 0) goto out_failed;+ /*+ * If there is no window available, remove the default DMA window,+ * if it's present. This will make all the resources available to the+ * new DDW window.+ * If anything fails after this, we need to restore it, so also check+ * for extensions presence.+ */ if (query.windows_available == 0) {
Does phyp really always advertise 0 windows for these VFs? What is in
the largest_available_block when windows_available==0?
For this VF, it always advertise 0 windows before removing the default
DMA window. The largest available block size is the same as after the
removal (256GB). The only value that changes after removal is the
number of available windows. Here some debug prints:
Ah, I see, thanks for the info. Ok, they really do not want us to have 2
windows. Oh well.
quoted
quoted
- /*
- * no additional windows are available for this device.
- * We might be able to reallocate the existing window,
- * trading in for a larger page size.
- */
- dev_dbg(&dev->dev, "no free dynamic windows");
- goto out_failed;
+ default_win = of_find_property(pdn, "ibm,dma-window", NULL);
+ ddw_ext = of_find_property(pdn, "ibm,ddw-extensions", NULL);
+ if (default_win && ddw_ext)
+ remove_dma_window(pdn, ddw_avail, default_win);
+
+ /* Query again, to check if the window is available */
+ ret = query_ddw(dev, ddw_avail, &query, pdn);
+ if (ret != 0)
+ goto out_failed;
+
+ if (query.windows_available == 0) {
+ /* no windows are available for this device. */
+ dev_dbg(&dev->dev, "no free dynamic windows");
+ goto out_failed;
+ }
}
+
Unrelated new empty line. Thanks,
Fixed!
Thank you!
quoted
quoted
if (query.page_size & 4) {
page_shift = 24; /* 16MB */
} else if (query.page_size & 2) {
Because of the growing nature of ddw-extensions, I intentionally let
this be (DDW_EXT_QUERY_OUT_SIZE + 1). If we create a DDW_EXT_LAST, it
will be incremented in the future if more extensions come to exist.
I mean, I previously saw no reason for allocating space for extensions
after the desired one, as they won't be used here.
Ah, my bad, you're right.
quoted
quoted
quoted
u64 buid;
- int ret;
+ int ret, out_sz;
+
+ /*
+ * From LoPAR level 2.8, "ibm,ddw-extensions" index 3 can rule how many
+ * output parameters ibm,query-pe-dma-windows will have, ranging from
+ * 5 to 6.
+ */
+
+ ret = of_property_read_u32_array(parent, "ibm,ddw-extensions",
+ &ddw_ext[0],
+ DDW_EXT_QUERY_OUT_SIZE + 1);
In this case, I made sure not to cross (DDW_EXT_QUERY_OUT_SIZE + 1)
while reading the extensions from the property.
What do you think about it?
I think you want something like:
static inline int ddw_read_ext(const struct device_node *np, int extnum,
u32 *ret)
{
retun of_property_read_u32_index(np, "ibm,ddw-extensions", extnum + 1, ret);
}
These "+1"'s all over the place are confusing.
That's a great idea!
I was not aware it was possible to read a single value[index] directly
from the property, but it makes total sense to use it.
Thank you!
On Wed, 2020-07-01 at 16:57 -0300, Leonardo Bras wrote:
quoted
quoted
It is not necessarily "direct" anymore as the name suggests, you may
want to change that. DMA64_PROPNAME, may be. Thanks,
Yeah, you are right.
I will change this for next version, also changing the string name to
reflect this.
-#define DIRECT64_PROPNAME "linux,direct64-ddr-window-info"
+#define DMA64_PROPNAME "linux,dma64-ddr-window-info"
Is that ok?
Thank you for helping!
In fact, there is a lot of places in this file where it's called direct
window. Should I replace everything?
Should it be in a separated patch?
If it looks simple and you write a nice commit log explaining all that
and why you are not reusing the existing ibm,dma-window property (to
provide a clue what "reset" will reset to? is there any other reason?)
for that - sure, do it :)
--
Alexey
From: Leonardo Bras <hidden> Date: 2020-07-02 00:36:22
On Thu, 2020-07-02 at 10:21 +1000, Alexey Kardashevskiy wrote:
quoted
enum {
DDW_QUERY_PE_DMA_WIN,
DDW_CREATE_PE_DMA_WIN,
DDW_REMOVE_PE_DMA_WIN,
DDW_APPLICABLE_SIZE
}
IMO, it looks better than all the defines before.
What do you think?
No, not really, these come from a binary interface so the reader of this
cares about absolute numbers and rather wants to see them explicitly.
Makes sense to me.
I am still getting experience on where to use enum vs define. Thanks
for the tip!
Using something like
enum {
DDW_QUERY_PE_DMA_WIN = 0,
DDW_CREATE_PE_DMA_WIN = 1,
DDW_REMOVE_PE_DMA_WIN = 2,
DDW_APPLICABLE_SIZE
};
would be fine too?
Or should one stick to #define in this case?
Thank you,
On Thu, 2020-07-02 at 10:21 +1000, Alexey Kardashevskiy wrote:
quoted
quoted
enum {
DDW_QUERY_PE_DMA_WIN,
DDW_CREATE_PE_DMA_WIN,
DDW_REMOVE_PE_DMA_WIN,
DDW_APPLICABLE_SIZE
}
IMO, it looks better than all the defines before.
What do you think?
No, not really, these come from a binary interface so the reader of this
cares about absolute numbers and rather wants to see them explicitly.
Makes sense to me.
I am still getting experience on where to use enum vs define. Thanks
for the tip!
Using something like
enum {
DDW_QUERY_PE_DMA_WIN = 0,
DDW_CREATE_PE_DMA_WIN = 1,
DDW_REMOVE_PE_DMA_WIN = 2,
DDW_APPLICABLE_SIZE
};
would be fine too?
This is fine too.
Or should one stick to #define in this case?
imho a matter of taste but after some grepping it feels like #define is
mostly used which does not mean it is a good idea. Keep it enum and see
if it passed mpe's filter :)
--
Alexey
From: Leonardo Bras <hidden> Date: 2020-07-02 00:47:02
On Thu, 2020-07-02 at 10:43 +1000, Alexey Kardashevskiy wrote:
quoted
Or should one stick to #define in this case?
imho a matter of taste but after some grepping it feels like #define is
mostly used which does not mean it is a good idea. Keep it enum and see
if it passed mpe's filter :)
From: Leonardo Bras <hidden> Date: 2020-07-02 00:54:51
On Thu, 2020-07-02 at 10:31 +1000, Alexey Kardashevskiy wrote:
quoted
In fact, there is a lot of places in this file where it's called direct
window. Should I replace everything?
Should it be in a separated patch?
If it looks simple and you write a nice commit log explaining all that
and why you are not reusing the existing ibm,dma-window property
for that - sure, do it :)
Nice, I will do that :)
(to provide a clue what "reset" will reset to? is there any other
reason?)
That's the main reason here.
The way I perceive this, ibm,dma-window should only point to the
default DMA window, which is guaranteed to always be the same, even if
it's destroyed and re-created. So there I see no point destroying /
overwriting it.
On the other hand, I also thought about using a new node name for this
window, but it would be very troublesome and I could see no real gain.
Thanks !
From: Leonardo Bras <hidden> Date: 2020-07-03 06:27:16
On Thu, 2020-07-02 at 10:31 +1000, Alexey Kardashevskiy wrote:
On 02/07/2020 09:48, Leonardo Bras wrote:
quoted
On Wed, 2020-07-01 at 16:57 -0300, Leonardo Bras wrote:
quoted
quoted
It is not necessarily "direct" anymore as the name suggests, you may
want to change that. DMA64_PROPNAME, may be. Thanks,
Yeah, you are right.
I will change this for next version, also changing the string name to
reflect this.
-#define DIRECT64_PROPNAME "linux,direct64-ddr-window-info"
+#define DMA64_PROPNAME "linux,dma64-ddr-window-info"
Is that ok?
Thank you for helping!
In fact, there is a lot of places in this file where it's called direct
window. Should I replace everything?
Should it be in a separated patch?
If it looks simple and you write a nice commit log explaining all that
and why you are not reusing the existing ibm,dma-window property (to
provide a clue what "reset" will reset to? is there any other reason?)
for that - sure, do it :)