Re: [PATCH 1/2] vfio powerpc: enabled on powernv platform

From: Alexey Kardashevskiy
Date: Mon Feb 11 2013 - 18:19:21 EST

On 12/02/13 09:16, Alex Williamson wrote:
On Mon, 2013-02-11 at 22:54 +1100, Alexey Kardashevskiy wrote:
This patch initializes IOMMU groups based on the IOMMU
configuration discovered during the PCI scan on POWERNV
(POWER non virtualized) platform. The IOMMU groups are
to be used later by VFIO driver (PCI pass through).

It also implements an API for mapping/unmapping pages for
guest PCI drivers and providing DMA window properties.
This API is going to be used later by QEMU-VFIO to handle
h_put_tce hypercalls from the KVM guest.

The iommu_put_tce_user_mode() does only a single page mapping
as an API for adding many mappings at once is going to be
added later.

Although this driver has been tested only on the POWERNV
platform, it should work on any platform which supports
TCE tables. As h_put_tce hypercall is received by the host
kernel and processed by the QEMU (what involves calling
the host kernel again), performance is not the best -
circa 220MB/s on 10Gb ethernet network.

To enable VFIO on POWER, enable SPAPR_TCE_IOMMU config
option and configure VFIO as required.

Cc: David Gibson <david@xxxxxxxxxxxxxxxxxxxxx>
Signed-off-by: Alexey Kardashevskiy <aik@xxxxxxxxx>

Yay, it's not dead! ;)

No, still alive :) I have been implementing real mode and multiple pages support (for real mode too) so I changed the interface a bit as I did not realize some bits from the powerpc arch.


I'd love some kind of changelog to know what to look for in here,
especially given 2mo since the last version.

There is no serious change actually.

---
arch/powerpc/include/asm/iommu.h | 15 ++
arch/powerpc/kernel/iommu.c | 343 +++++++++++++++++++++++++++
arch/powerpc/platforms/powernv/pci-ioda.c | 1 +
arch/powerpc/platforms/powernv/pci-p5ioc2.c | 5 +-
arch/powerpc/platforms/powernv/pci.c | 3 +
drivers/iommu/Kconfig | 8 +
6 files changed, 374 insertions(+), 1 deletion(-)

diff --git a/arch/powerpc/include/asm/iommu.h b/arch/powerpc/include/asm/iommu.h
index cbfe678..900294b 100644
--- a/arch/powerpc/include/asm/iommu.h
+++ b/arch/powerpc/include/asm/iommu.h
@@ -76,6 +76,9 @@ struct iommu_table {
struct iommu_pool large_pool;
struct iommu_pool pools[IOMMU_NR_POOLS];
unsigned long *it_map; /* A simple allocation bitmap for now */
+#ifdef CONFIG_IOMMU_API
+ struct iommu_group *it_group;
+#endif
};

struct scatterlist;
@@ -98,6 +101,8 @@ 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);
+extern void iommu_register_group(struct iommu_table * tbl,
+ int domain_number, unsigned long pe_num);

extern int iommu_map_sg(struct device *dev, struct iommu_table *tbl,
struct scatterlist *sglist, int nelems,
@@ -147,5 +152,15 @@ static inline void iommu_restore(void)
}
#endif

+/* The API to support IOMMU operations for VFIO */
+extern long iommu_clear_tce_user_mode(struct iommu_table *tbl,
+ unsigned long ioba, unsigned long tce_value,
+ unsigned long npages);
+extern long iommu_put_tce_user_mode(struct iommu_table *tbl,
+ unsigned long ioba, unsigned long tce);
+
+extern void iommu_flush_tce(struct iommu_table *tbl);
+extern long iommu_lock_table(struct iommu_table *tbl, bool lock);
+
#endif /* __KERNEL__ */
#endif /* _ASM_IOMMU_H */
diff --git a/arch/powerpc/kernel/iommu.c b/arch/powerpc/kernel/iommu.c
index 7c309fe..b4fdabc 100644
--- a/arch/powerpc/kernel/iommu.c
+++ b/arch/powerpc/kernel/iommu.c
@@ -37,6 +37,7 @@
#include <linux/fault-inject.h>
#include <linux/pci.h>
#include <linux/kvm_host.h>
+#include <linux/iommu.h>
#include <asm/io.h>
#include <asm/prom.h>
#include <asm/iommu.h>
@@ -45,6 +46,7 @@
#include <asm/kdump.h>
#include <asm/fadump.h>
#include <asm/vio.h>
+#include <asm/tce.h>

#define DBG(...)

@@ -707,11 +709,39 @@ struct iommu_table *iommu_init_table(struct iommu_table *tbl, int nid)
return tbl;
}

+static void group_release(void *iommu_data)
+{
+ struct iommu_table *tbl = iommu_data;
+ tbl->it_group = NULL;
+}
+
+void iommu_register_group(struct iommu_table * tbl,
+ int domain_number, unsigned long pe_num)
+{
+ struct iommu_group *grp;
+
+ grp = iommu_group_alloc();
+ if (IS_ERR(grp)) {
+ pr_info("powerpc iommu api: cannot create new group, err=%ld\n",
+ PTR_ERR(grp));
+ return;
+ }
+ tbl->it_group = grp;
+ iommu_group_set_iommudata(grp, tbl, group_release);
+ iommu_group_set_name(grp, kasprintf(GFP_KERNEL, "domain%d-pe%lx",
+ domain_number, pe_num));
+}
+
void iommu_free_table(struct iommu_table *tbl, const char *node_name)
{
unsigned long bitmap_sz;
unsigned int order;

+ if (tbl && tbl->it_group) {
+ iommu_group_put(tbl->it_group);
+ BUG_ON(tbl->it_group);
+ }
+
if (!tbl || !tbl->it_map) {
printk(KERN_ERR "%s: expected TCE map for %s\n", __func__,
node_name);
@@ -876,4 +906,317 @@ void kvm_iommu_unmap_pages(struct kvm *kvm, struct kvm_memory_slot *slot)
{
}

+static enum dma_data_direction tce_direction(unsigned long tce)
+{
+ if ((tce & TCE_PCI_READ) && (tce & TCE_PCI_WRITE))
+ return DMA_BIDIRECTIONAL;
+ else if (tce & TCE_PCI_READ)
+ return DMA_TO_DEVICE;
+ else if (tce & TCE_PCI_WRITE)
+ return DMA_FROM_DEVICE;
+ else
+ return DMA_NONE;
+}
+
+void iommu_flush_tce(struct iommu_table *tbl)
+{
+ /* Flush/invalidate TLB caches if necessary */
+ if (ppc_md.tce_flush)
+ ppc_md.tce_flush(tbl);
+
+ /* Make sure updates are seen by hardware */
+ mb();
+}
+EXPORT_SYMBOL_GPL(iommu_flush_tce);
+
+static long tce_clear_param_check(struct iommu_table *tbl,
+ unsigned long ioba, unsigned long tce_value,
+ unsigned long npages)
+{
+ unsigned long size = npages << IOMMU_PAGE_SHIFT;
+
+ /* ppc_md.tce_free() does not support any value but 0 */
+ if (tce_value)
+ return -EINVAL;
+
+ if (ioba & ~IOMMU_PAGE_MASK)
+ return -EINVAL;
+
+ if ((ioba + size) > ((tbl->it_offset + tbl->it_size)
+ << IOMMU_PAGE_SHIFT))
+ return -EINVAL;
+
+ if (ioba < (tbl->it_offset << IOMMU_PAGE_SHIFT))
+ return -EINVAL;
+
+ return 0;

Why do these all return long (vs int)? Is this a POWER-ism?

No, not really but yeah, I picked it in powerpc code :) I tried to keep them "long" but I noticed "int" below so what is the rule? Change all to int?


+}
+
+static long tce_put_param_check(struct iommu_table *tbl,
+ unsigned long ioba, unsigned long tce)
+{
+ if (!(tce & (TCE_PCI_WRITE | TCE_PCI_READ)))
+ return -EINVAL;
+
+ if (tce & ~(IOMMU_PAGE_MASK | TCE_PCI_WRITE | TCE_PCI_READ))
+ return -EINVAL;
+
+ if (ioba & ~IOMMU_PAGE_MASK)
+ return -EINVAL;
+
+ if ((ioba + IOMMU_PAGE_SIZE) > ((tbl->it_offset + tbl->it_size)
+ << IOMMU_PAGE_SHIFT))
+ return -EINVAL;
+
+ if (ioba < (tbl->it_offset << IOMMU_PAGE_SHIFT))
+ return -EINVAL;
+
+ return 0;
+}
+
+static long clear_tce(struct iommu_table *tbl,
+ unsigned long entry, unsigned long pages)
+{
+ unsigned long oldtce;
+ struct page *page;
+ struct iommu_pool *pool;
+
+ for ( ; pages; --pages, ++entry) {
+ pool = get_pool(tbl, entry);
+ spin_lock(&(pool->lock));
+
+ oldtce = ppc_md.tce_get(tbl, entry);
+ if (oldtce & (TCE_PCI_WRITE | TCE_PCI_READ)) {
+ ppc_md.tce_free(tbl, entry, 1);
+
+ page = pfn_to_page(oldtce >> PAGE_SHIFT);
+ WARN_ON(!page);
+ if (page) {
+ if (oldtce & TCE_PCI_WRITE)
+ SetPageDirty(page);
+ put_page(page);
+ }
+ }
+ spin_unlock(&(pool->lock));
+ }
+
+ return 0;

No non-zero return, make it void?

ah, ok. The prototype will change for real mode either way, it will get a "realmode" flag and become able to fail (which will switch the virtual mode).

+}
+
+long iommu_clear_tce_user_mode(struct iommu_table *tbl, unsigned long ioba,
+ unsigned long tce_value, unsigned long npages)
+{
+ long ret;
+ unsigned long entry = ioba >> IOMMU_PAGE_SHIFT;
+
+ ret = tce_clear_param_check(tbl, ioba, tce_value, npages);
+ if (!ret)
+ ret = clear_tce(tbl, entry, npages);
+
+ if (ret < 0)
+ pr_err("iommu_tce: %s failed ioba=%lx, tce_value=%lx ret=%ld\n",
+ __func__, ioba, tce_value, ret);
+
+ return ret;
+}
+EXPORT_SYMBOL_GPL(iommu_clear_tce_user_mode);
+
+/* hwaddr is a virtual address here, tce_build converts it to physical */
+static long do_tce_build(struct iommu_table *tbl, unsigned long entry,
+ unsigned long hwaddr, enum dma_data_direction direction)
+{
+ long ret = -EBUSY;
+ unsigned long oldtce;
+ struct iommu_pool *pool = get_pool(tbl, entry);
+
+ spin_lock(&(pool->lock));
+
+ oldtce = ppc_md.tce_get(tbl, entry);
+ /* Add new entry if it is not busy */
+ if (!(oldtce & (TCE_PCI_WRITE | TCE_PCI_READ)))
+ ret = ppc_md.tce_build(tbl, entry, 1, hwaddr, direction, NULL);
+
+ spin_unlock(&(pool->lock));
+
+ if (unlikely(ret))
+ pr_err("iommu_tce: %s failed on hwaddr=%lx ioba=%lx kva=%lx ret=%ld\n",
+ __func__, hwaddr, entry << IOMMU_PAGE_SHIFT,
+ hwaddr, ret);
+
+ return ret;
+}
+
+static int put_tce_user_mode(struct iommu_table *tbl, unsigned long entry,
+ unsigned long tce)
+{
+ int ret;

Now we're back to returning ints.
>
+ struct page *page = NULL;
+ unsigned long hwaddr, offset = tce & IOMMU_PAGE_MASK & ~PAGE_MASK;
+ enum dma_data_direction direction = tce_direction(tce);
+
+ ret = get_user_pages_fast(tce & PAGE_MASK, 1,
+ direction != DMA_TO_DEVICE, &page);
+ if (unlikely(ret != 1)) {
+ pr_err("iommu_tce: get_user_pages_fast failed tce=%lx ioba=%lx ret=%d\n",
+ tce, entry << IOMMU_PAGE_SHIFT, ret);
+ return -EFAULT;
+ }
+ hwaddr = (unsigned long) page_address(page) + offset;
+
+ ret = do_tce_build(tbl, entry, hwaddr, direction);
+ if (ret)
+ put_page(page);
+
+ return ret;
+}
+
+long iommu_put_tce_user_mode(struct iommu_table *tbl, unsigned long ioba,
+ unsigned long tce)
+{
+ long ret;

Oops, back to longs.

+ unsigned long entry = ioba >> IOMMU_PAGE_SHIFT;
+
+ ret = tce_put_param_check(tbl, ioba, tce);
+ if (!ret)
+ ret = put_tce_user_mode(tbl, entry, tce);
+
+ if (ret < 0)
+ pr_err("iommu_tce: %s failed ioba=%lx, tce=%lx, ret=%ld\n",
+ __func__, ioba, tce, ret);
+
+ return ret;
+}
+EXPORT_SYMBOL_GPL(iommu_put_tce_user_mode);
+
+/*
+ * Helpers to do locked pages accounting.
+ * Called from ioctl so down_write_trylock is not necessary.
+ */
+static void lock_acct(long npage)
+{
+ if (!current->mm)
+ return; /* process exited */
+
+ down_write(&current->mm->mmap_sem);
+ current->mm->locked_vm += npage;
+ up_write(&current->mm->mmap_sem);
+}
+
+/*
+ * iommu_lock_table - Start/stop using the table by VFIO
+ * @tbl: Pointer to the IOMMU table
+ * @lock: true when VFIO starts using the table
+ */
+long iommu_lock_table(struct iommu_table *tbl, bool lock)
+{
+ unsigned long sz = (tbl->it_size + 7) >> 3;
+ unsigned long locked, lock_limit;
+
+ if (lock) {
+ /*
+ * Account for locked pages
+ * The worst case is when every IOMMU page
+ * is mapped to separate system page
+ */
+ locked = current->mm->locked_vm + tbl->it_size;
+ lock_limit = rlimit(RLIMIT_MEMLOCK) >> PAGE_SHIFT;
+ if (locked > lock_limit && !capable(CAP_IPC_LOCK)) {
+ pr_warn("RLIMIT_MEMLOCK (%ld) exceeded\n",
+ rlimit(RLIMIT_MEMLOCK));
+ return -ENOMEM;
+ }
+
+ if (tbl->it_offset == 0)
+ clear_bit(0, tbl->it_map);
+
+ if (!bitmap_empty(tbl->it_map, tbl->it_size)) {
+ pr_err("iommu_tce: it_map is not empty");
+ return -EBUSY;
+ }
+
+ lock_acct(tbl->it_size);
+ memset(tbl->it_map, 0xff, sz);
+ }
+
+ /* Clear TCE table */
+ clear_tce(tbl, tbl->it_offset, tbl->it_size);
+
+ if (!lock) {
+ lock_acct(-tbl->it_size);
+ memset(tbl->it_map, 0, sz);
+
+ /* Restore bit#0 set by iommu_init_table() */
+ if (tbl->it_offset == 0)
+ set_bit(0, tbl->it_map);
+ }
+
+ return 0;
+}
+EXPORT_SYMBOL_GPL(iommu_lock_table);
+
+int iommu_add_device(struct device *dev)
+{
+ struct iommu_table *tbl;
+ int ret = 0;
+
+ if (WARN_ON(dev->iommu_group)) {
+ pr_warn("iommu_tce: device %s is already in iommu group %d, skipping\n",
+ dev_name(dev),
+ iommu_group_id(dev->iommu_group));
+ return -EBUSY;
+ }
+
+ tbl = get_iommu_table_base(dev);
+ if (!tbl) {
+ pr_debug("iommu_tce: skipping device %s with no tbl\n",
+ dev_name(dev));
+ return 0;
+ }
+
+ pr_debug("iommu_tce: adding %s to iommu group %d\n",
+ dev_name(dev), iommu_group_id(tbl->it_group));
+
+ ret = iommu_group_add_device(tbl->it_group, dev);
+ if (ret < 0)
+ pr_err("iommu_tce: %s has not been added, ret=%d\n",
+ dev_name(dev), ret);
+
+ return ret;
+}
+
+void iommu_del_device(struct device *dev)
+{
+ iommu_group_remove_device(dev);
+}
+
+static int iommu_bus_notifier(struct notifier_block *nb,
+ unsigned long action, void *data)
+{
+ struct device *dev = data;
+
+ switch (action) {
+ case BUS_NOTIFY_ADD_DEVICE:
+ return iommu_add_device(dev);
+ case BUS_NOTIFY_DEL_DEVICE:
+ iommu_del_device(dev);
+ return 0;
+ default:
+ return 0;
+ }
+}
+
+static struct notifier_block tce_iommu_bus_nb = {
+ .notifier_call = iommu_bus_notifier,
+};
+
+static int __init tce_iommu_init(void)
+{
+ BUILD_BUG_ON(PAGE_SIZE < IOMMU_PAGE_SIZE);
+
+ bus_register_notifier(&pci_bus_type, &tce_iommu_bus_nb);
+ return 0;
+}
+
+arch_initcall(tce_iommu_init);
+
#endif /* CONFIG_IOMMU_API */
diff --git a/arch/powerpc/platforms/powernv/pci-ioda.c b/arch/powerpc/platforms/powernv/pci-ioda.c
index 8e90e89..04dbc49 100644
--- a/arch/powerpc/platforms/powernv/pci-ioda.c
+++ b/arch/powerpc/platforms/powernv/pci-ioda.c
@@ -522,6 +522,7 @@ static void pnv_pci_ioda_setup_dma_pe(struct pnv_phb *phb,
| TCE_PCI_SWINV_PAIR;
}
iommu_init_table(tbl, phb->hose->node);
+ iommu_register_group(tbl, pci_domain_nr(pe->pbus), pe->pe_number);

if (pe->pdev)
set_iommu_table_base(&pe->pdev->dev, tbl);
diff --git a/arch/powerpc/platforms/powernv/pci-p5ioc2.c b/arch/powerpc/platforms/powernv/pci-p5ioc2.c
index abe6780..7ce75b0 100644
--- a/arch/powerpc/platforms/powernv/pci-p5ioc2.c
+++ b/arch/powerpc/platforms/powernv/pci-p5ioc2.c
@@ -87,8 +87,11 @@ static void pnv_pci_init_p5ioc2_msis(struct pnv_phb *phb) { }
static void pnv_pci_p5ioc2_dma_dev_setup(struct pnv_phb *phb,
struct pci_dev *pdev)
{
- if (phb->p5ioc2.iommu_table.it_map == NULL)
+ if (phb->p5ioc2.iommu_table.it_map == NULL) {
iommu_init_table(&phb->p5ioc2.iommu_table, phb->hose->node);
+ iommu_register_group(&phb->p5ioc2.iommu_table,
+ pci_domain_nr(phb->hose->bus), phb->opal_id);
+ }

set_iommu_table_base(&pdev->dev, &phb->p5ioc2.iommu_table);
}
diff --git a/arch/powerpc/platforms/powernv/pci.c b/arch/powerpc/platforms/powernv/pci.c
index f60a188..d112701 100644
--- a/arch/powerpc/platforms/powernv/pci.c
+++ b/arch/powerpc/platforms/powernv/pci.c
@@ -20,6 +20,7 @@
#include <linux/irq.h>
#include <linux/io.h>
#include <linux/msi.h>
+#include <linux/iommu.h>

#include <asm/sections.h>
#include <asm/io.h>
@@ -503,6 +504,7 @@ static struct iommu_table *pnv_pci_setup_bml_iommu(struct pci_controller *hose)
pnv_pci_setup_iommu_table(tbl, __va(be64_to_cpup(basep)),
be32_to_cpup(sizep), 0);
iommu_init_table(tbl, hose->node);
+ iommu_register_group(tbl, pci_domain_nr(hose->bus), 0);

/* Deal with SW invalidated TCEs when needed (BML way) */
swinvp = of_get_property(hose->dn, "linux,tce-sw-invalidate-info",
@@ -631,3 +633,4 @@ void __init pnv_pci_init(void)
ppc_md.teardown_msi_irqs = pnv_teardown_msi_irqs;
#endif
}
+

stray newline

diff --git a/drivers/iommu/Kconfig b/drivers/iommu/Kconfig
index e39f9db..ce6b186 100644
--- a/drivers/iommu/Kconfig
+++ b/drivers/iommu/Kconfig
@@ -187,4 +187,12 @@ config EXYNOS_IOMMU_DEBUG

Say N unless you need kernel log message for IOMMU debugging

+config SPAPR_TCE_IOMMU
+ bool "sPAPR TCE IOMMU Support"
+ depends on PPC_POWERNV
+ select IOMMU_API
+ help
+ Enables bits of IOMMU API required by VFIO. The iommu_ops is
+ still not implemented.
+
endif # IOMMU_SUPPORT

Looks mostly ok to me other than the minor notes. As mentioned
previously, this one needs to go in through power maintainers before I
can include 2/2 . Thanks,

Alex





--
Alexey
--
To unsubscribe from this list: send the line "unsubscribe linux-kernel" in
the body of a message to majordomo@xxxxxxxxxxxxxxx
More majordomo info at http://vger.kernel.org/majordomo-info.html
Please read the FAQ at http://www.tux.org/lkml/