by Gleb Natapov

[permalink] [raw]

Subject: Re: [PATCH v10 12/13] KVM: PPC: Add support for IOMMU in-kernel handling

On Fri, Sep 06, 2013 at 08:40:26PM +1000, Alexey Kardashevskiy wrote:
> This allows the host kernel to handle H_PUT_TCE, H_PUT_TCE_INDIRECT
> and H_STUFF_TCE requests targeted an IOMMU TCE table without passing
> them to user space which saves time on switching to user space and back.
>
> Both real and virtual modes are supported. The kernel tries to
> handle a TCE request in the real mode, if fails it passes the request
> to the virtual mode to complete the operation. If it a virtual mode
> handler fails, the request is passed to user space.
>
> The first user of this is VFIO on POWER. Trampolines to the VFIO external
> user API functions are required for this patch.
>
> This adds a "SPAPR TCE IOMMU" KVM device to associate a logical bus
> number (LIOBN) with an VFIO IOMMU group fd and enable in-kernel handling
> of map/unmap requests. The device supports a single attribute which is
> a struct with LIOBN and IOMMU fd. When the attribute is set, the device
> establishes the connection between KVM and VFIO.
>
> Tests show that this patch increases transmission speed from 220MB/s
> to 750..1020MB/s on 10Gb network (Chelsea CXGB3 10Gb ethernet card).
>
> Signed-off-by: Paul Mackerras <[email protected]>
> Signed-off-by: Alexey Kardashevskiy <[email protected]>
>
> ---
>
> Changes:
> v10:
> * all IOMMU TCE links are handled by one KVM device now
> * KVM device has its own list of TCE descriptors
> * the search-by-liobn function was extended to search through
> emulated and IOMMU lists
>
> v9:
> * KVM_CAP_SPAPR_TCE_IOMMU ioctl to KVM replaced with "SPAPR TCE IOMMU"
> KVM device
> * release_spapr_tce_table() is not shared between different TCE types
> * reduced the patch size by moving KVM device bits and VFIO external API
> trampolines to separate patches
> * moved documentation from Documentation/virtual/kvm/api.txt to
> Documentation/virtual/kvm/devices/spapr_tce_iommu.txt
>
> v8:
> * fixed warnings from check_patch.pl
>
> 2013/07/11:
> * removed multiple #ifdef IOMMU_API as IOMMU_API is always enabled
> for KVM_BOOK3S_64
> * kvmppc_gpa_to_hva_and_get also returns host phys address. Not much sense
> for this here but the next patch for hugepages support will use it more.
>
> 2013/07/06:
> * added realmode arch_spin_lock to protect TCE table from races
> in real and virtual modes
> * POWERPC IOMMU API is changed to support real mode
> * iommu_take_ownership and iommu_release_ownership are protected by
> iommu_table's locks
> * VFIO external user API use rewritten
> * multiple small fixes
>
> 2013/06/27:
> * tce_list page is referenced now in order to protect it from accident
> invalidation during H_PUT_TCE_INDIRECT execution
> * added use of the external user VFIO API
>
> 2013/06/05:
> * changed capability number
> * changed ioctl number
> * update the doc article number
>
> 2013/05/20:
> * removed get_user() from real mode handlers
> * kvm_vcpu_arch::tce_tmp usage extended. Now real mode handler puts there
> translated TCEs, tries realmode_get_page() on those and if it fails, it
> passes control over the virtual mode handler which tries to finish
> the request handling
> * kvmppc_lookup_pte() now does realmode_get_page() protected by BUSY bit
> on a page
> * The only reason to pass the request to user mode now is when the user mode
> did not register TCE table in the kernel, in all other cases the virtual mode
> handler is expected to do the job
> ---
> .../virtual/kvm/devices/spapr_tce_iommu.txt | 40 +++
> arch/powerpc/include/asm/kvm_host.h | 8 +
> arch/powerpc/include/uapi/asm/kvm.h | 5 -
> arch/powerpc/kvm/book3s_64_vio.c | 327 ++++++++++++++++++++-
> arch/powerpc/kvm/book3s_64_vio_hv.c | 142 +++++++++
> arch/powerpc/kvm/powerpc.c | 1 +
> include/linux/kvm_host.h | 1 +
> virt/kvm/kvm_main.c | 5 +
> 8 files changed, 517 insertions(+), 12 deletions(-)
> create mode 100644 Documentation/virtual/kvm/devices/spapr_tce_iommu.txt
>
> diff --git a/Documentation/virtual/kvm/devices/spapr_tce_iommu.txt b/Documentation/virtual/kvm/devices/spapr_tce_iommu.txt
> new file mode 100644
> index 0000000..b911945
> --- /dev/null
> +++ b/Documentation/virtual/kvm/devices/spapr_tce_iommu.txt
> @@ -0,0 +1,40 @@
> +SPAPR TCE IOMMU device
> +
> +Capability: KVM_CAP_SPAPR_TCE_IOMMU
> +Architectures: powerpc
> +
> +Device type supported: KVM_DEV_TYPE_SPAPR_TCE_IOMMU
> +
> +Groups:
> + KVM_DEV_SPAPR_TCE_IOMMU_ATTR_LINKAGE
> + Attributes: one VFIO IOMMU fd per LIOBN, indexed by LIOBN
> +
> +This is completely made up device which provides API to link
> +logical bus number (LIOBN) and IOMMU group. The user space has
> +to create a new SPAPR TCE IOMMU device once per KVM session
> +and use "set_attr" to add or remove a logical bus.
> +
> +LIOBN is a PCI bus identifier from PPC64-server (sPAPR) DMA hypercalls
> +(H_PUT_TCE, H_PUT_TCE_INDIRECT, H_STUFF_TCE).
> +IOMMU group is a minimal isolated device set which can be passed to
> +the user space via VFIO.
> +
> +The userspace adds the new LIOBN-IOMMU link by calling KVM_SET_DEVICE_ATTR
> +with the attribute initialized as shown below:
> +struct kvm_device_attr attr = {
> + .flags = 0,
> + .group = KVM_DEV_SPAPR_TCE_IOMMU_ATTR_LINKAGE,
> + .attr = liobn,
> + .addr = (uint64_t)(uintptr_t)&group_fd,
> +};
> +
> +To remove the link, the userspace calls KVM_SET_DEVICE_ATTR with
> +the group_fd equal to zero.
> +
Zero is a valid fd descriptor. Lest make it -1.

> +As the device opens VFIO group fds and holds the file pointer,
> +it does not need to keep an fd internally and therefore KVM_GET_DEVICE_ATTR
> +is not supported.
> +
> +When KVM exits, all links are destroyed automatically.
> +
> +The kernel advertises this feature via KVM_CAP_SPAPR_TCE_IOMMU capability.
Why KVM_CAP_SPAPR_TCE_IOMMU is needed? Supported devices are
discoverable without capabilities.

> diff --git a/arch/powerpc/include/asm/kvm_host.h b/arch/powerpc/include/asm/kvm_host.h
> index a23f132..a2a481e 100644
> --- a/arch/powerpc/include/asm/kvm_host.h
> +++ b/arch/powerpc/include/asm/kvm_host.h
> @@ -181,9 +181,15 @@ struct kvmppc_spapr_tce_table {
> struct kvm *kvm;
> u64 liobn;
> u32 window_size;
> + struct iommu_group *grp; /* used for IOMMU groups */
> + struct vfio_group *vfio_grp; /* used for IOMMU groups */
> struct page *pages[0];
> };
>
> +struct kvmppc_spapr_tce_iommu_device {
> + struct list_head tables;
> +};
> +
> struct kvmppc_linear_info {
> void *base_virt;
> unsigned long base_pfn;
> @@ -264,6 +270,7 @@ struct kvm_arch {
> #endif /* CONFIG_KVM_BOOK3S_64_HV */
> #ifdef CONFIG_PPC_BOOK3S_64
> struct list_head spapr_tce_tables;
> + struct kvmppc_spapr_tce_iommu_device *tcedev;
> struct list_head rtas_tokens;
> #endif
> #ifdef CONFIG_KVM_MPIC
> @@ -612,6 +619,7 @@ struct kvm_vcpu_arch {
> u64 busy_preempt;
>
> unsigned long *tce_tmp_hpas; /* TCE cache for TCE_PUT_INDIRECT */
> + unsigned long tce_tmp_num; /* Number of handled TCEs in cache */
> enum {
> TCERM_NONE,
> TCERM_GETPAGE,
> diff --git a/arch/powerpc/include/uapi/asm/kvm.h b/arch/powerpc/include/uapi/asm/kvm.h
> index c1ae1e5..a9d3d73 100644
> --- a/arch/powerpc/include/uapi/asm/kvm.h
> +++ b/arch/powerpc/include/uapi/asm/kvm.h
> @@ -512,11 +512,6 @@ struct kvm_get_htab_header {
> #define KVM_XICS_PENDING (1ULL << 42)
>
> /* SPAPR TCE IOMMU device specification */
> -struct kvm_create_spapr_tce_iommu_linkage {
> - __u64 liobn;
> - __u32 fd;
> - __u32 flags;
> -};
> #define KVM_DEV_SPAPR_TCE_IOMMU_ATTR_LINKAGE 0
>
> #endif /* __LINUX_KVM_POWERPC_H */
> diff --git a/arch/powerpc/kvm/book3s_64_vio.c b/arch/powerpc/kvm/book3s_64_vio.c
> index 2880d2b..0978794 100644
> --- a/arch/powerpc/kvm/book3s_64_vio.c
> +++ b/arch/powerpc/kvm/book3s_64_vio.c
> @@ -29,6 +29,8 @@
> #include <linux/anon_inodes.h>
> #include <linux/module.h>
> #include <linux/vfio.h>
> +#include <linux/iommu.h>
> +#include <linux/file.h>
>
> #include <asm/tlbflush.h>
> #include <asm/kvm_ppc.h>
> @@ -158,10 +160,8 @@ long kvm_vm_ioctl_create_spapr_tce(struct kvm *kvm,
> int i;
>
> /* Check this LIOBN hasn't been previously allocated */
> - list_for_each_entry(stt, &kvm->arch.spapr_tce_tables, list) {
> - if (stt->liobn == args->liobn)
> - return -EBUSY;
> - }
> + if (kvmppc_find_tce_table(kvm, args->liobn))
> + return -EBUSY;
>
> npages = kvmppc_stt_npages(args->window_size);
>
> @@ -201,9 +201,175 @@ fail:
> return ret;
> }
>
> -/* Converts guest physical address to host virtual address */
> +static void kvmppc_spapr_tce_iommu_table_destroy(
> + struct kvm_device *dev,
> + struct kvmppc_spapr_tce_table *tt)
> +{
> + struct kvm *kvm = dev->kvm;
> +
> + mutex_lock(&kvm->lock);
> + list_del(&tt->list);
> +
> + if (tt->vfio_grp)
> + kvmppc_vfio_group_put_external_user(tt->vfio_grp);
> + iommu_group_put(tt->grp);
> +
> + kfree(tt);
> + mutex_unlock(&kvm->lock);
> +}
> +
> +static int kvmppc_spapr_tce_iommu_create(struct kvm_device *dev, u32 type)
> +{
> + struct kvmppc_spapr_tce_iommu_device *tcedev;
> + int ret = 0;
> +
> + tcedev = kzalloc(sizeof(*tcedev), GFP_KERNEL);
> + if (!tcedev)
> + return -ENOMEM;
> + dev->private = tcedev;
> +
> + INIT_LIST_HEAD(&tcedev->tables);
> +
> + /* Already there ? */
> + mutex_lock(&dev->kvm->lock);
> + if (dev->kvm->arch.tcedev)
> + ret = -EEXIST;
> + else
> + dev->kvm->arch.tcedev = tcedev;
> + mutex_unlock(&dev->kvm->lock);
> +
> + if (ret)
Need to free tcedev here.

> + return ret;
> +
> + return 0;
> +}
> +

--
Gleb.

2013-10-30 05:33:54

by Bharat Bhushan

[permalink] [raw]

Subject: RE: [PATCH v9 01/13] KVM: PPC: POWERNV: move iommu_add_device earlier

Hi Alex,

Looks like this patch is not picked by anyone, Are you going to pick this patch?
My vfio/iommu patches have dependency on this patch (this is already tested by me).

Thanks
-Bharat

> -----Original Message-----
> From: Linuxppc-dev [mailto:linuxppc-dev-
> [email protected]] On Behalf Of Alexey
> Kardashevskiy
> Sent: Wednesday, August 28, 2013 2:08 PM
> To: [email protected]
> Cc: [email protected]; Gleb Natapov; Alexey Kardashevskiy; Alexander Graf;
> [email protected]; [email protected]; [email protected]; Paul
> Mackerras; Paolo Bonzini; David Gibson
> Subject: [PATCH v9 01/13] KVM: PPC: POWERNV: move iommu_add_device earlier
>
> The current implementation of IOMMU on sPAPR does not use iommu_ops and
> therefore does not call IOMMU API's bus_set_iommu() which
> 1) sets iommu_ops for a bus
> 2) registers a bus notifier
> Instead, PCI devices are added to IOMMU groups from
> subsys_initcall_sync(tce_iommu_init) which does basically the same thing without
> using iommu_ops callbacks.
>
> However Freescale PAMU driver (https://lkml.org/lkml/2013/7/1/158)
> implements iommu_ops and when tce_iommu_init is called, every PCI device is
> already added to some group so there is a conflict.
>
> This patch does 2 things:
> 1. removes the loop in which PCI devices were added to groups and adds explicit
> iommu_add_device() calls to add devices as soon as they get the iommu_table
> pointer assigned to them.
> 2. moves a bus notifier to powernv code in order to avoid conflict with the
> notifier from Freescale driver.
>
> iommu_add_device() and iommu_del_device() are public now.
>
> Signed-off-by: Alexey Kardashevskiy <[email protected]>
> ---
> Changes:
> v8:
> * added the check for iommu_group!=NULL before removing device from a group as
> suggested by Wei Yang <[email protected]>
>
> v2:
> * added a helper - set_iommu_table_base_and_group - which does
> set_iommu_table_base() and iommu_add_device()
> ---
> arch/powerpc/include/asm/iommu.h | 9 +++++++
> arch/powerpc/kernel/iommu.c | 41 +++--------------------------
> arch/powerpc/platforms/powernv/pci-ioda.c | 8 +++---
> arch/powerpc/platforms/powernv/pci-p5ioc2.c | 2 +-
> arch/powerpc/platforms/powernv/pci.c | 33 ++++++++++++++++++++++-
> arch/powerpc/platforms/pseries/iommu.c | 8 +++---
> 6 files changed, 55 insertions(+), 46 deletions(-)
>
> diff --git a/arch/powerpc/include/asm/iommu.h b/arch/powerpc/include/asm/iommu.h
> index c34656a..19ad77f 100644
> --- a/arch/powerpc/include/asm/iommu.h
> +++ b/arch/powerpc/include/asm/iommu.h
> @@ -103,6 +103,15 @@ extern struct iommu_table *iommu_init_table(struct
> iommu_table * tbl,
> int nid);
> extern void iommu_register_group(struct iommu_table *tbl,
> int pci_domain_number, unsigned long pe_num);
> +extern int iommu_add_device(struct device *dev); extern void
> +iommu_del_device(struct device *dev);
> +
> +static inline void set_iommu_table_base_and_group(struct device *dev,
> + void *base)
> +{
> + set_iommu_table_base(dev, base);
> + iommu_add_device(dev);
> +}
>
> extern int iommu_map_sg(struct device *dev, struct iommu_table *tbl,
> struct scatterlist *sglist, int nelems, diff --git
> a/arch/powerpc/kernel/iommu.c b/arch/powerpc/kernel/iommu.c index
> b20ff17..15f8ca8 100644
> --- a/arch/powerpc/kernel/iommu.c
> +++ b/arch/powerpc/kernel/iommu.c
> @@ -1105,7 +1105,7 @@ void iommu_release_ownership(struct iommu_table *tbl) }
> EXPORT_SYMBOL_GPL(iommu_release_ownership);
>
> -static int iommu_add_device(struct device *dev)
> +int iommu_add_device(struct device *dev)
> {
> struct iommu_table *tbl;
> int ret = 0;
> @@ -1134,46 +1134,13 @@ static int iommu_add_device(struct device *dev)
>
> return ret;
> }
> +EXPORT_SYMBOL_GPL(iommu_add_device);
>
> -static void iommu_del_device(struct device *dev)
> +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)
> -{
> - struct pci_dev *pdev = NULL;
> -
> - BUILD_BUG_ON(PAGE_SIZE < IOMMU_PAGE_SIZE);
> -
> - for_each_pci_dev(pdev)
> - iommu_add_device(&pdev->dev);
> -
> - bus_register_notifier(&pci_bus_type, &tce_iommu_bus_nb);
> - return 0;
> -}
> -
> -subsys_initcall_sync(tce_iommu_init);
> +EXPORT_SYMBOL_GPL(iommu_del_device);
>
> #else
>
> diff --git a/arch/powerpc/platforms/powernv/pci-ioda.c
> b/arch/powerpc/platforms/powernv/pci-ioda.c
> index d8140b1..756bb58 100644
> --- a/arch/powerpc/platforms/powernv/pci-ioda.c
> +++ b/arch/powerpc/platforms/powernv/pci-ioda.c
> @@ -440,7 +440,7 @@ static void pnv_pci_ioda_dma_dev_setup(struct pnv_phb *phb,
> struct pci_dev *pdev
> return;
>
> pe = &phb->ioda.pe_array[pdn->pe_number];
> - set_iommu_table_base(&pdev->dev, &pe->tce32_table);
> + set_iommu_table_base_and_group(&pdev->dev, &pe->tce32_table);
> }
>
> static void pnv_ioda_setup_bus_dma(struct pnv_ioda_pe *pe, struct pci_bus *bus)
> @@ -448,7 +448,7 @@ static void pnv_ioda_setup_bus_dma(struct pnv_ioda_pe *pe,
> struct pci_bus *bus)
> struct pci_dev *dev;
>
> list_for_each_entry(dev, &bus->devices, bus_list) {
> - set_iommu_table_base(&dev->dev, &pe->tce32_table);
> + set_iommu_table_base_and_group(&dev->dev, &pe->tce32_table);
> if (dev->subordinate)
> pnv_ioda_setup_bus_dma(pe, dev->subordinate);
> }
> @@ -611,7 +611,7 @@ static void pnv_pci_ioda_setup_dma_pe(struct pnv_phb *phb,
> iommu_register_group(tbl, pci_domain_nr(pe->pbus), pe->pe_number);
>
> if (pe->pdev)
> - set_iommu_table_base(&pe->pdev->dev, tbl);
> + set_iommu_table_base_and_group(&pe->pdev->dev, tbl);
> else
> pnv_ioda_setup_bus_dma(pe, pe->pbus);
>
> @@ -687,7 +687,7 @@ static void pnv_pci_ioda2_setup_dma_pe(struct pnv_phb *phb,
> iommu_init_table(tbl, phb->hose->node);
>
> if (pe->pdev)
> - set_iommu_table_base(&pe->pdev->dev, tbl);
> + set_iommu_table_base_and_group(&pe->pdev->dev, tbl);
> else
> pnv_ioda_setup_bus_dma(pe, pe->pbus);
>
> diff --git a/arch/powerpc/platforms/powernv/pci-p5ioc2.c
> b/arch/powerpc/platforms/powernv/pci-p5ioc2.c
> index b68db63..ede341b 100644
> --- a/arch/powerpc/platforms/powernv/pci-p5ioc2.c
> +++ b/arch/powerpc/platforms/powernv/pci-p5ioc2.c
> @@ -92,7 +92,7 @@ static void pnv_pci_p5ioc2_dma_dev_setup(struct pnv_phb *phb,
> pci_domain_nr(phb->hose->bus), phb->opal_id);
> }
>
> - set_iommu_table_base(&pdev->dev, &phb->p5ioc2.iommu_table);
> + set_iommu_table_base_and_group(&pdev->dev, &phb->p5ioc2.iommu_table);
> }
>
> static void __init pnv_pci_init_p5ioc2_phb(struct device_node *np, u64 hub_id,
> diff --git a/arch/powerpc/platforms/powernv/pci.c
> b/arch/powerpc/platforms/powernv/pci.c
> index a28d3b5..c005011 100644
> --- a/arch/powerpc/platforms/powernv/pci.c
> +++ b/arch/powerpc/platforms/powernv/pci.c
> @@ -504,7 +504,7 @@ static void pnv_pci_dma_fallback_setup(struct pci_controller
> *hose,
> pdn->iommu_table = pnv_pci_setup_bml_iommu(hose);
> if (!pdn->iommu_table)
> return;
> - set_iommu_table_base(&pdev->dev, pdn->iommu_table);
> + set_iommu_table_base_and_group(&pdev->dev, pdn->iommu_table);
> }
>
> static void pnv_pci_dma_dev_setup(struct pci_dev *pdev) @@ -623,3 +623,34 @@
> void __init pnv_pci_init(void)
> ppc_md.teardown_msi_irqs = pnv_teardown_msi_irqs; #endif }
> +
> +static int tce_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:
> + if (dev->iommu_group)
> + iommu_del_device(dev);
> + return 0;
> + default:
> + return 0;
> + }
> +}
> +
> +static struct notifier_block tce_iommu_bus_nb = {
> + .notifier_call = tce_iommu_bus_notifier, };
> +
> +static int __init tce_iommu_bus_notifier_init(void) {
> + BUILD_BUG_ON(PAGE_SIZE < IOMMU_PAGE_SIZE);
> +
> + bus_register_notifier(&pci_bus_type, &tce_iommu_bus_nb);
> + return 0;
> +}
> +
> +subsys_initcall_sync(tce_iommu_bus_notifier_init);
> diff --git a/arch/powerpc/platforms/pseries/iommu.c
> b/arch/powerpc/platforms/pseries/iommu.c
> index 23fc1dc..884ae71 100644
> --- a/arch/powerpc/platforms/pseries/iommu.c
> +++ b/arch/powerpc/platforms/pseries/iommu.c
> @@ -687,7 +687,8 @@ static void pci_dma_dev_setup_pSeries(struct pci_dev *dev)
> iommu_table_setparms(phb, dn, tbl);
> PCI_DN(dn)->iommu_table = iommu_init_table(tbl, phb->node);
> iommu_register_group(tbl, pci_domain_nr(phb->bus), 0);
> - set_iommu_table_base(&dev->dev, PCI_DN(dn)->iommu_table);
> + set_iommu_table_base_and_group(&dev->dev,
> + PCI_DN(dn)->iommu_table);
> return;
> }
>
> @@ -699,7 +700,8 @@ static void pci_dma_dev_setup_pSeries(struct pci_dev *dev)
> dn = dn->parent;
>
> if (dn && PCI_DN(dn))
> - set_iommu_table_base(&dev->dev, PCI_DN(dn)->iommu_table);
> + set_iommu_table_base_and_group(&dev->dev,
> + PCI_DN(dn)->iommu_table);
> else
> printk(KERN_WARNING "iommu: Device %s has no iommu table\n",
> pci_name(dev));
> @@ -1193,7 +1195,7 @@ static void pci_dma_dev_setup_pSeriesLP(struct pci_dev
> *dev)
> pr_debug(" found DMA window, table: %p\n", pci->iommu_table);
> }
>
> - set_iommu_table_base(&dev->dev, pci->iommu_table);
> + set_iommu_table_base_and_group(&dev->dev, pci->iommu_table);
> }
>
> static int dma_set_mask_pSeriesLP(struct device *dev, u64 dma_mask)
> --
> 1.8.4.rc4
>
> _______________________________________________
> Linuxppc-dev mailing list
> [email protected]
> https://lists.ozlabs.org/listinfo/linuxppc-dev