[PATCH kernel] powerpc/powernv/ioda2: Update iommu table base on ownership change

[PATCH kernel] powerpc/powernv/ioda2: Update iommu table base on ownership change

[Alexey Kardashevskiy]

On POWERNV platform, in order to do DMA via IOMMU (i.e. 32bit DMA in
our case), a device needs an iommu_table pointer set via
set_iommu_table_base().

The codeflow is:
- pnv_pci_ioda2_setup_dma_pe()
	- pnv_pci_ioda2_setup_default_config()
	- pnv_ioda_setup_bus_dma() [1]

pnv_pci_ioda2_setup_dma_pe() creates IOMMU groups,
pnv_pci_ioda2_setup_default_config() does default DMA setup,
pnv_ioda_setup_bus_dma() takes a bus PE (on IODA2, all physical function
PEs as bus PEs except NPU), walks through all underlying buses and
devices, adds all devices to an IOMMU group and sets iommu_table.

On IODA2, when VFIO is used, it takes ownership over a PE which means it
removes all tables and creates new ones (with a possibility of sharing
them among PEs). So when the ownership is returned from VFIO to
the kernel, the iommu_table pointer written to a device at [1] is
stale and needs an update.

This adds an "add_to_group" parameter to pnv_ioda_setup_bus_dma()
(in fact re-adds as it used to be there a while ago for different
reasons) to tell the helper if a device needs to be added to
an IOMMU group with an iommu_table update or just the latter.

This calls pnv_ioda_setup_bus_dma(..., false) from
pnv_ioda2_release_ownership() so when the ownership is restored,
32bit DMA can work again for a device. This does the same thing
on obtaining ownership as the iommu_table point is stale at this point
anyway and it is safer to have NULL there.

We did not hit this earlier as all tested devices in recent years were
only using 64bit DMA; the rare exception for this is MPT3 SAS adapter
which uses both 32bit and 64bit DMA access and it has not been tested
with VFIO much.

Cc: Gavin Shan <gwshan@linux.vnet.ibm.com>
Signed-off-by: Alexey Kardashevskiy <aik@ozlabs.ru>
---

If this is applied before "powerpc/powernv/npu: Remove dead iommu code",
there will be a minor conflict.
---
 arch/powerpc/platforms/powernv/pci-ioda.c | 17 ++++++++++++-----
 1 file changed, 12 insertions(+), 5 deletions(-)

diff --git a/arch/powerpc/platforms/powernv/pci-ioda.c b/arch/powerpc/platforms/powernv/pci-ioda.c
index 51ec0dc1dfde..f5a2421bf164 100644
--- a/arch/powerpc/platforms/powernv/pci-ioda.c
+++ b/arch/powerpc/platforms/powernv/pci-ioda.c
@@ -1774,17 +1774,20 @@ static u64 pnv_pci_ioda_dma_get_required_mask(struct pci_dev *pdev)
 }
 
 static void pnv_ioda_setup_bus_dma(struct pnv_ioda_pe *pe,
-				   struct pci_bus *bus)
+				   struct pci_bus *bus,
+				   bool add_to_group)
 {
 	struct pci_dev *dev;
 
 	list_for_each_entry(dev, &bus->devices, bus_list) {
 		set_iommu_table_base(&dev->dev, pe->table_group.tables[0]);
 		set_dma_offset(&dev->dev, pe->tce_bypass_base);
-		iommu_add_device(&dev->dev);
+		if (add_to_group)
+			iommu_add_device(&dev->dev);
 
 		if ((pe->flags & PNV_IODA_PE_BUS_ALL) && dev->subordinate)
-			pnv_ioda_setup_bus_dma(pe, dev->subordinate);
+			pnv_ioda_setup_bus_dma(pe, dev->subordinate,
+					add_to_group);
 	}
 }
 
@@ -2190,7 +2193,7 @@ static void pnv_pci_ioda1_setup_dma_pe(struct pnv_phb *phb,
 		set_iommu_table_base(&pe->pdev->dev, tbl);
 		iommu_add_device(&pe->pdev->dev);
 	} else if (pe->flags & (PNV_IODA_PE_BUS | PNV_IODA_PE_BUS_ALL))
-		pnv_ioda_setup_bus_dma(pe, pe->pbus);
+		pnv_ioda_setup_bus_dma(pe, pe->pbus, true);
 
 	return;
  fail:
@@ -2425,6 +2428,8 @@ static void pnv_ioda2_take_ownership(struct iommu_table_group *table_group)
 
 	pnv_pci_ioda2_set_bypass(pe, false);
 	pnv_pci_ioda2_unset_window(&pe->table_group, 0);
+	if (pe->pbus)
+		pnv_ioda_setup_bus_dma(pe, pe->pbus, false);
 	pnv_ioda2_table_free(tbl);
 }
 
@@ -2434,6 +2439,8 @@ static void pnv_ioda2_release_ownership(struct iommu_table_group *table_group)
 						table_group);
 
 	pnv_pci_ioda2_setup_default_config(pe);
+	if (pe->pbus)
+		pnv_ioda_setup_bus_dma(pe, pe->pbus, false);
 }
 
 static struct iommu_table_group_ops pnv_pci_ioda2_ops = {
@@ -2725,7 +2732,7 @@ static void pnv_pci_ioda2_setup_dma_pe(struct pnv_phb *phb,
 		return;
 
 	if (pe->flags & (PNV_IODA_PE_BUS | PNV_IODA_PE_BUS_ALL))
-		pnv_ioda_setup_bus_dma(pe, pe->pbus);
+		pnv_ioda_setup_bus_dma(pe, pe->pbus, true);
 }
 
 #ifdef CONFIG_PCI_MSI
-- 
2.11.0

Re: [PATCH kernel] powerpc/powernv/ioda2: Update iommu table base on ownership change

[Gavin Shan]

On Tue, Feb 21, 2017 at 01:41:31PM +1100, Alexey Kardashevskiy wrote:
>On POWERNV platform, in order to do DMA via IOMMU (i.e. 32bit DMA in
>our case), a device needs an iommu_table pointer set via
>set_iommu_table_base().
>
>The codeflow is:
>- pnv_pci_ioda2_setup_dma_pe()
>	- pnv_pci_ioda2_setup_default_config()
>	- pnv_ioda_setup_bus_dma() [1]
>
>pnv_pci_ioda2_setup_dma_pe() creates IOMMU groups,
>pnv_pci_ioda2_setup_default_config() does default DMA setup,
>pnv_ioda_setup_bus_dma() takes a bus PE (on IODA2, all physical function
>PEs as bus PEs except NPU), walks through all underlying buses and
>devices, adds all devices to an IOMMU group and sets iommu_table.
>
>On IODA2, when VFIO is used, it takes ownership over a PE which means it
>removes all tables and creates new ones (with a possibility of sharing
>them among PEs). So when the ownership is returned from VFIO to
>the kernel, the iommu_table pointer written to a device at [1] is
>stale and needs an update.
>
>This adds an "add_to_group" parameter to pnv_ioda_setup_bus_dma()
>(in fact re-adds as it used to be there a while ago for different
>reasons) to tell the helper if a device needs to be added to
>an IOMMU group with an iommu_table update or just the latter.
>
>This calls pnv_ioda_setup_bus_dma(..., false) from
>pnv_ioda2_release_ownership() so when the ownership is restored,
>32bit DMA can work again for a device. This does the same thing
>on obtaining ownership as the iommu_table point is stale at this point
>anyway and it is safer to have NULL there.
>
>We did not hit this earlier as all tested devices in recent years were
>only using 64bit DMA; the rare exception for this is MPT3 SAS adapter
>which uses both 32bit and 64bit DMA access and it has not been tested
>with VFIO much.
>
>Cc: Gavin Shan <gwshan@linux.vnet.ibm.com>
>Signed-off-by: Alexey Kardashevskiy <aik@ozlabs.ru>

Acked-by: Gavin Shan <gwshan@linux.vnet.ibm.com>

One thing would be improved in future, which isn't relevant to
this patch if my understanding is correct enough: The TCE table for
DMA32 space created during system boot is destroyed when VFIO takes
the ownership. The same TCE table (same level, page size, window size
etc) is created and associated to the PE again. Some CPU cycles would
be saved if the original table is picked up without creating a new one.

The involved function is pnv_pci_ioda2_create_table(). Its primary work
is to allocate pages from buddy. It's usually fast if there are enough
free pages. Otherwise, it would be relatively slow. It also has the risk
to fail the allocation. I guess it's not bad to save CPU cycles in this
critical (maybe hot?) path.

Thanks,
Gavin 

Re: [PATCH kernel] powerpc/powernv/ioda2: Update iommu table base on ownership change

[David Gibson]

[-- Attachment #1: Type: text/plain, Size: 4802 bytes --]

On Tue, Feb 21, 2017 at 01:41:31PM +1100, Alexey Kardashevskiy wrote:
> On POWERNV platform, in order to do DMA via IOMMU (i.e. 32bit DMA in
> our case), a device needs an iommu_table pointer set via
> set_iommu_table_base().
> 
> The codeflow is:
> - pnv_pci_ioda2_setup_dma_pe()
> 	- pnv_pci_ioda2_setup_default_config()
> 	- pnv_ioda_setup_bus_dma() [1]
> 
> pnv_pci_ioda2_setup_dma_pe() creates IOMMU groups,
> pnv_pci_ioda2_setup_default_config() does default DMA setup,
> pnv_ioda_setup_bus_dma() takes a bus PE (on IODA2, all physical function
> PEs as bus PEs except NPU), walks through all underlying buses and
> devices, adds all devices to an IOMMU group and sets iommu_table.
> 
> On IODA2, when VFIO is used, it takes ownership over a PE which means it
> removes all tables and creates new ones (with a possibility of sharing
> them among PEs). So when the ownership is returned from VFIO to
> the kernel, the iommu_table pointer written to a device at [1] is
> stale and needs an update.
> 
> This adds an "add_to_group" parameter to pnv_ioda_setup_bus_dma()
> (in fact re-adds as it used to be there a while ago for different
> reasons) to tell the helper if a device needs to be added to
> an IOMMU group with an iommu_table update or just the latter.
> 
> This calls pnv_ioda_setup_bus_dma(..., false) from
> pnv_ioda2_release_ownership() so when the ownership is restored,
> 32bit DMA can work again for a device. This does the same thing
> on obtaining ownership as the iommu_table point is stale at this point
> anyway and it is safer to have NULL there.
> 
> We did not hit this earlier as all tested devices in recent years were
> only using 64bit DMA; the rare exception for this is MPT3 SAS adapter
> which uses both 32bit and 64bit DMA access and it has not been tested
> with VFIO much.
> 
> Cc: Gavin Shan <gwshan@linux.vnet.ibm.com>
> Signed-off-by: Alexey Kardashevskiy <aik@ozlabs.ru>

Reviewed-by: David Gibson <david@gibson.dropbear.id.au>

> ---
> 
> If this is applied before "powerpc/powernv/npu: Remove dead iommu code",
> there will be a minor conflict.
> ---
>  arch/powerpc/platforms/powernv/pci-ioda.c | 17 ++++++++++++-----
>  1 file changed, 12 insertions(+), 5 deletions(-)
> 
> diff --git a/arch/powerpc/platforms/powernv/pci-ioda.c b/arch/powerpc/platforms/powernv/pci-ioda.c
> index 51ec0dc1dfde..f5a2421bf164 100644
> --- a/arch/powerpc/platforms/powernv/pci-ioda.c
> +++ b/arch/powerpc/platforms/powernv/pci-ioda.c
> @@ -1774,17 +1774,20 @@ static u64 pnv_pci_ioda_dma_get_required_mask(struct pci_dev *pdev)
>  }
>  
>  static void pnv_ioda_setup_bus_dma(struct pnv_ioda_pe *pe,
> -				   struct pci_bus *bus)
> +				   struct pci_bus *bus,
> +				   bool add_to_group)
>  {
>  	struct pci_dev *dev;
>  
>  	list_for_each_entry(dev, &bus->devices, bus_list) {
>  		set_iommu_table_base(&dev->dev, pe->table_group.tables[0]);
>  		set_dma_offset(&dev->dev, pe->tce_bypass_base);
> -		iommu_add_device(&dev->dev);
> +		if (add_to_group)
> +			iommu_add_device(&dev->dev);
>  
>  		if ((pe->flags & PNV_IODA_PE_BUS_ALL) && dev->subordinate)
> -			pnv_ioda_setup_bus_dma(pe, dev->subordinate);
> +			pnv_ioda_setup_bus_dma(pe, dev->subordinate,
> +					add_to_group);
>  	}
>  }
>  
> @@ -2190,7 +2193,7 @@ static void pnv_pci_ioda1_setup_dma_pe(struct pnv_phb *phb,
>  		set_iommu_table_base(&pe->pdev->dev, tbl);
>  		iommu_add_device(&pe->pdev->dev);
>  	} else if (pe->flags & (PNV_IODA_PE_BUS | PNV_IODA_PE_BUS_ALL))
> -		pnv_ioda_setup_bus_dma(pe, pe->pbus);
> +		pnv_ioda_setup_bus_dma(pe, pe->pbus, true);
>  
>  	return;
>   fail:
> @@ -2425,6 +2428,8 @@ static void pnv_ioda2_take_ownership(struct iommu_table_group *table_group)
>  
>  	pnv_pci_ioda2_set_bypass(pe, false);
>  	pnv_pci_ioda2_unset_window(&pe->table_group, 0);
> +	if (pe->pbus)
> +		pnv_ioda_setup_bus_dma(pe, pe->pbus, false);
>  	pnv_ioda2_table_free(tbl);
>  }
>  
> @@ -2434,6 +2439,8 @@ static void pnv_ioda2_release_ownership(struct iommu_table_group *table_group)
>  						table_group);
>  
>  	pnv_pci_ioda2_setup_default_config(pe);
> +	if (pe->pbus)
> +		pnv_ioda_setup_bus_dma(pe, pe->pbus, false);
>  }
>  
>  static struct iommu_table_group_ops pnv_pci_ioda2_ops = {
> @@ -2725,7 +2732,7 @@ static void pnv_pci_ioda2_setup_dma_pe(struct pnv_phb *phb,
>  		return;
>  
>  	if (pe->flags & (PNV_IODA_PE_BUS | PNV_IODA_PE_BUS_ALL))
> -		pnv_ioda_setup_bus_dma(pe, pe->pbus);
> +		pnv_ioda_setup_bus_dma(pe, pe->pbus, true);
>  }
>  
>  #ifdef CONFIG_PCI_MSI

-- 
David Gibson			| I'll have my music baroque, and my code
david AT gibson.dropbear.id.au	| minimalist, thank you.  NOT _the_ _other_
				| _way_ _around_!
http://www.ozlabs.org/~dgibson

[-- Attachment #2: signature.asc --]
[-- Type: application/pgp-signature, Size: 819 bytes --]

Re: [PATCH kernel] powerpc/powernv/ioda2: Update iommu table base on ownership change

[Alexey Kardashevskiy]

On 22/02/17 10:28, Gavin Shan wrote:
> On Tue, Feb 21, 2017 at 01:41:31PM +1100, Alexey Kardashevskiy wrote:
>> On POWERNV platform, in order to do DMA via IOMMU (i.e. 32bit DMA in
>> our case), a device needs an iommu_table pointer set via
>> set_iommu_table_base().
>>
>> The codeflow is:
>> - pnv_pci_ioda2_setup_dma_pe()
>> 	- pnv_pci_ioda2_setup_default_config()
>> 	- pnv_ioda_setup_bus_dma() [1]
>>
>> pnv_pci_ioda2_setup_dma_pe() creates IOMMU groups,
>> pnv_pci_ioda2_setup_default_config() does default DMA setup,
>> pnv_ioda_setup_bus_dma() takes a bus PE (on IODA2, all physical function
>> PEs as bus PEs except NPU), walks through all underlying buses and
>> devices, adds all devices to an IOMMU group and sets iommu_table.
>>
>> On IODA2, when VFIO is used, it takes ownership over a PE which means it
>> removes all tables and creates new ones (with a possibility of sharing
>> them among PEs). So when the ownership is returned from VFIO to
>> the kernel, the iommu_table pointer written to a device at [1] is
>> stale and needs an update.
>>
>> This adds an "add_to_group" parameter to pnv_ioda_setup_bus_dma()
>> (in fact re-adds as it used to be there a while ago for different
>> reasons) to tell the helper if a device needs to be added to
>> an IOMMU group with an iommu_table update or just the latter.
>>
>> This calls pnv_ioda_setup_bus_dma(..., false) from
>> pnv_ioda2_release_ownership() so when the ownership is restored,
>> 32bit DMA can work again for a device. This does the same thing
>> on obtaining ownership as the iommu_table point is stale at this point
>> anyway and it is safer to have NULL there.
>>
>> We did not hit this earlier as all tested devices in recent years were
>> only using 64bit DMA; the rare exception for this is MPT3 SAS adapter
>> which uses both 32bit and 64bit DMA access and it has not been tested
>> with VFIO much.
>>
>> Cc: Gavin Shan <gwshan@linux.vnet.ibm.com>
>> Signed-off-by: Alexey Kardashevskiy <aik@ozlabs.ru>
> 
> Acked-by: Gavin Shan <gwshan@linux.vnet.ibm.com>

Thanks!

> One thing would be improved in future, which isn't relevant to
> this patch if my understanding is correct enough: The TCE table for
> DMA32 space created during system boot is destroyed when VFIO takes
> the ownership. The same TCE table (same level, page size, window size
> etc) is created and associated to the PE again. Some CPU cycles would
> be saved if the original table is picked up without creating a new one.

It is not necessary same levels or window size, could be something
different. Also carrying a table will just make code bit more complicated
and it is complicated enough already - we need to consider very possible
case of IOMMU tables sharing.


> The involved function is pnv_pci_ioda2_create_table(). Its primary work
> is to allocate pages from buddy.

It allocates pages via alloc_pages_node(), not buddy.

> It's usually fast if there are enough
> free pages. Otherwise, it would be relatively slow. It also has the risk
> to fail the allocation. I guess it's not bad to save CPU cycles in this
> critical (maybe hot?) path.

It is not a critical path - it happens on a guest (re)boot only.


-- 
Alexey

Re: [PATCH kernel] powerpc/powernv/ioda2: Update iommu table base on ownership change

[Gavin Shan]

On Wed, Feb 22, 2017 at 02:05:15PM +1100, Alexey Kardashevskiy wrote:
>On 22/02/17 10:28, Gavin Shan wrote:
>> On Tue, Feb 21, 2017 at 01:41:31PM +1100, Alexey Kardashevskiy wrote:

[The subsequent discussion isn't related to the patch itself anymore]

>> One thing would be improved in future, which isn't relevant to
>> this patch if my understanding is correct enough: The TCE table for
>> DMA32 space created during system boot is destroyed when VFIO takes
>> the ownership. The same TCE table (same level, page size, window size
>> etc) is created and associated to the PE again. Some CPU cycles would
>> be saved if the original table is picked up without creating a new one.
>
>It is not necessary same levels or window size, could be something
>different. Also carrying a table will just make code bit more complicated
>and it is complicated enough already - we need to consider very possible
>case of IOMMU tables sharing.
>

Right after host boots up and VFIO isn't involved yet, each PE is associated
with a DMA32 space (0 - 2G) and the IO page size is 4KB. If the whole (window)
size, IO page size or levels are changed after the PE is released from guest
to host, it's not so much reasonable as the device (including its TCE table)
needs to be restored to previous state. Or we are talking about different
DMA space (TCE tables)?

Regarding the possiblity of sharing IOMMU tables, I don't quite understand.
Do you mean the situation of multiple functions adapter, some of them are
passed to guest and the left owned by host? I don't see how it works from
the DMA path. Would you please explain a bit?

>
>> The involved function is pnv_pci_ioda2_create_table(). Its primary work
>> is to allocate pages from buddy.
>
>It allocates pages via alloc_pages_node(), not buddy.
>

page allocator maybe? It's fetching page from PCP (PerCPU Pages) or buddy's
freelist depending on the requested size.

>> It's usually fast if there are enough
>> free pages. Otherwise, it would be relatively slow. It also has the risk
>> to fail the allocation. I guess it's not bad to save CPU cycles in this
>> critical (maybe hot?) path.
>
>It is not a critical path - it happens on a guest (re)boot only.
>

My point is: it sounds nice if less time needs for guest to (re)boot. I
don't know how much time could be saved though.

Thanks,
Gavin

Re: [kernel] powerpc/powernv/ioda2: Update iommu table base on ownership change

[Michael Ellerman]

On Tue, 2017-02-21 at 02:41:31 UTC, Alexey Kardashevskiy wrote:
> On POWERNV platform, in order to do DMA via IOMMU (i.e. 32bit DMA in
> our case), a device needs an iommu_table pointer set via
> set_iommu_table_base().
> 
> The codeflow is:
> - pnv_pci_ioda2_setup_dma_pe()
> 	- pnv_pci_ioda2_setup_default_config()
> 	- pnv_ioda_setup_bus_dma() [1]
> 
> pnv_pci_ioda2_setup_dma_pe() creates IOMMU groups,
> pnv_pci_ioda2_setup_default_config() does default DMA setup,
> pnv_ioda_setup_bus_dma() takes a bus PE (on IODA2, all physical function
> PEs as bus PEs except NPU), walks through all underlying buses and
> devices, adds all devices to an IOMMU group and sets iommu_table.
> 
> On IODA2, when VFIO is used, it takes ownership over a PE which means it
> removes all tables and creates new ones (with a possibility of sharing
> them among PEs). So when the ownership is returned from VFIO to
> the kernel, the iommu_table pointer written to a device at [1] is
> stale and needs an update.
> 
> This adds an "add_to_group" parameter to pnv_ioda_setup_bus_dma()
> (in fact re-adds as it used to be there a while ago for different
> reasons) to tell the helper if a device needs to be added to
> an IOMMU group with an iommu_table update or just the latter.
> 
> This calls pnv_ioda_setup_bus_dma(..., false) from
> pnv_ioda2_release_ownership() so when the ownership is restored,
> 32bit DMA can work again for a device. This does the same thing
> on obtaining ownership as the iommu_table point is stale at this point
> anyway and it is safer to have NULL there.
> 
> We did not hit this earlier as all tested devices in recent years were
> only using 64bit DMA; the rare exception for this is MPT3 SAS adapter
> which uses both 32bit and 64bit DMA access and it has not been tested
> with VFIO much.
> 
> Cc: Gavin Shan <gwshan@linux.vnet.ibm.com>
> Signed-off-by: Alexey Kardashevskiy <aik@ozlabs.ru>
> Acked-by: Gavin Shan <gwshan@linux.vnet.ibm.com>
> Reviewed-by: David Gibson <david@gibson.dropbear.id.au>

Applied to powerpc fixes, thanks.

https://git.kernel.org/powerpc/c/db08e1d53034a54fe177ced70476fd

cheers

Re: [PATCH kernel] powerpc/powernv/ioda2: Update iommu table base on ownership change

[Mauricio Faria de Oliveira]

On 02/20/2017 11:41 PM, Alexey Kardashevskiy wrote:
> Cc: Gavin Shan <gwshan@linux.vnet.ibm.com>
> Signed-off-by: Alexey Kardashevskiy <aik@ozlabs.ru>

Tested-by: Mauricio Faria de Oliveira <mauricfo@linux.vnet.ibm.com>

P.S.: sorry, late, but for the record.


-- 
Mauricio Faria de Oliveira
IBM Linux Technology Center