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

From: Alexey Kardashevskiy <[email-masked]>
Date: Tue, 12 Feb 2013 10:19:06 +1100

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 <[email-masked]>
>> Signed-off-by: Alexey Kardashevskiy <[email-masked]>
>
> 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