Re: [PATCH kernel v8 12/31] powerpc/spapr: vfio: Switch from iommu_table to new iommu_table_group

[David Gibson <david@gibson.dropbear.id.au>]

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

On Fri, Apr 17, 2015 at 01:48:13AM +1000, Alexey Kardashevskiy wrote:
> On 04/16/2015 03:55 PM, David Gibson wrote:
> >On Fri, Apr 10, 2015 at 04:30:54PM +1000, Alexey Kardashevskiy wrote:
> >>Modern IBM POWERPC systems support multiple (currently two) TCE tables
> >>per IOMMU group (a.k.a. PE). This adds a iommu_table_group container
> >>for TCE tables. Right now just one table is supported.
> >>
> >>Signed-off-by: Alexey Kardashevskiy <aik@ozlabs.ru>
> >>---
> >>  arch/powerpc/include/asm/iommu.h            |  18 +++--
> >>  arch/powerpc/kernel/iommu.c                 |  34 ++++----
> >>  arch/powerpc/platforms/powernv/pci-ioda.c   |  38 +++++----
> >>  arch/powerpc/platforms/powernv/pci-p5ioc2.c |  17 ++--
> >>  arch/powerpc/platforms/powernv/pci.c        |   2 +-
> >>  arch/powerpc/platforms/powernv/pci.h        |   4 +-
> >>  arch/powerpc/platforms/pseries/iommu.c      |   9 ++-
> >>  drivers/vfio/vfio_iommu_spapr_tce.c         | 120 ++++++++++++++++++++--------
> >>  8 files changed, 160 insertions(+), 82 deletions(-)
> >>
> >>diff --git a/arch/powerpc/include/asm/iommu.h b/arch/powerpc/include/asm/iommu.h
> >>index eb75726..667aa1a 100644
> >>--- a/arch/powerpc/include/asm/iommu.h
> >>+++ b/arch/powerpc/include/asm/iommu.h
> >>@@ -90,9 +90,7 @@ struct iommu_table {
> >>  	struct iommu_pool pools[IOMMU_NR_POOLS];
> >>  	unsigned long *it_map;       /* A simple allocation bitmap for now */
> >>  	unsigned long  it_page_shift;/* table iommu page size */
> >>-#ifdef CONFIG_IOMMU_API
> >>-	struct iommu_group *it_group;
> >>-#endif
> >>+	struct iommu_table_group *it_group;
> >>  	struct iommu_table_ops *it_ops;
> >>  	void (*set_bypass)(struct iommu_table *tbl, bool enable);
> >>  };
> >>@@ -126,14 +124,24 @@ extern void iommu_free_table(struct iommu_table *tbl, const char *node_name);
> >>   */
> >>  extern struct iommu_table *iommu_init_table(struct iommu_table * tbl,
> >>  					    int nid);
> >>+
> >>+#define IOMMU_TABLE_GROUP_MAX_TABLES	1
> >>+
> >>+struct iommu_table_group {
> >>  #ifdef CONFIG_IOMMU_API
> >>-extern void iommu_register_group(struct iommu_table *tbl,
> >>+	struct iommu_group *group;
> >>+#endif
> >>+	struct iommu_table tables[IOMMU_TABLE_GROUP_MAX_TABLES];
> >
> >There's nothing to indicate which of the tables are in use at the
> >current time.  I mean, it doesn't matter now because there's only one,
> >but the patch doesn't make a whole lot of sense without that.
> >
> >>+};
> >>+
> >>+#ifdef CONFIG_IOMMU_API
> >>+extern void iommu_register_group(struct iommu_table_group *table_group,
> >>  				 int pci_domain_number, unsigned long pe_num);
> >>  extern int iommu_add_device(struct device *dev);
> >>  extern void iommu_del_device(struct device *dev);
> >>  extern int __init tce_iommu_bus_notifier_init(void);
> >>  #else
> >>-static inline void iommu_register_group(struct iommu_table *tbl,
> >>+static inline void iommu_register_group(struct iommu_table_group *table_group,
> >>  					int pci_domain_number,
> >>  					unsigned long pe_num)
> >>  {
> >>diff --git a/arch/powerpc/kernel/iommu.c b/arch/powerpc/kernel/iommu.c
> >>index b39d00a..fd49c8e 100644
> >>--- a/arch/powerpc/kernel/iommu.c
> >>+++ b/arch/powerpc/kernel/iommu.c
> >>@@ -712,17 +712,20 @@ struct iommu_table *iommu_init_table(struct iommu_table *tbl, int nid)
> >>
> >>  struct iommu_table *iommu_table_alloc(int node)
> >>  {
> >>-	struct iommu_table *tbl;
> >>+	struct iommu_table_group *table_group;
> >>
> >>-	tbl = kzalloc_node(sizeof(struct iommu_table), GFP_KERNEL, node);
> >>+	table_group = kzalloc_node(sizeof(struct iommu_table_group), GFP_KERNEL,
> >>+			   node);
> >>+	table_group->tables[0].it_group = table_group;
> >>
> >>-	return tbl;
> >>+	return &table_group->tables[0];
> >>  }
> >>
> >>  void iommu_free_table(struct iommu_table *tbl, const char *node_name)
> >
> >Surely the free function should take a table group rather than a table
> >as argument.
> 
> 
> No, it should not. Tables lifetime is not the same even within the
> same group.

If that's so, then this function shouldn't free the group...

-- 
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: Type: application/pgp-signature, Size: 819 bytes --]