This enables in-kernel acceleration of H_PUT_TCE/etc hypercalls for pseries
guests using VFIO. As pseries is a para-virtualized environment, the guest
can see and control IOMMUs via special hypercalls which let the guest
to add and remove mappings in real hardware IOMMU.
This was posted last time quite a long time ago so I dropped versions now,
this re-respin is v1. This was successfully used in the PowerKVM product
for quite a while now.
This is based on git://git.kernel.org/pub/scm/virt/kvm/kvm.git , "next"
branch which got "multi-tce in-kernel acceleration" and "64 bit in-kernel
TCE" support.
Please comment. Thanks!
Alexey Kardashevskiy (9):
KVM: PPC: Reserve KVM_CAP_SPAPR_TCE_VFIO capability number
powerpc/mmu: Add real mode support for IOMMU preregistered memory
KVM: PPC: Use preregistered memory API to access TCE list
powerpc/powernv/iommu: Add real mode version of xchg()
KVM: PPC: Enable IOMMU_API for KVM_BOOK3S_64 permanently
KVM: PPC: Associate IOMMU group with guest view of TCE table
KVM: PPC: Create a virtual-mode only TCE table handlers
KVM: PPC: Add in-kernel handling for VFIO
KVM: PPC: VFIO device: support SPAPR TCE
Documentation/virtual/kvm/devices/vfio.txt | 21 +-
arch/powerpc/include/asm/iommu.h | 7 +
arch/powerpc/include/asm/kvm_host.h | 8 +
arch/powerpc/include/asm/kvm_ppc.h | 6 +
arch/powerpc/include/asm/mmu_context.h | 6 +-
arch/powerpc/kernel/iommu.c | 15 ++
arch/powerpc/kvm/Kconfig | 2 +
arch/powerpc/kvm/Makefile | 5 +-
arch/powerpc/kvm/book3s_64_vio.c | 344 +++++++++++++++++++++++++++++
arch/powerpc/kvm/book3s_64_vio_hv.c | 280 +++++++++++++++++++++--
arch/powerpc/kvm/book3s_hv_rmhandlers.S | 4 +-
arch/powerpc/kvm/powerpc.c | 1 +
arch/powerpc/mm/mmu_context_iommu.c | 45 +++-
arch/powerpc/platforms/powernv/pci-ioda.c | 28 ++-
include/uapi/linux/kvm.h | 10 +
virt/kvm/vfio.c | 106 +++++++++
16 files changed, 855 insertions(+), 33 deletions(-)
--
2.5.0.rc3
In real mode, TCE tables are invalidated using different
cache-inhibited store instructions which is different from
the virtual mode.
This defines and implements exchange_rm() callback. This does not
define set_rm/clear_rm/flush_rm callbacks as there is no user for those -
exchange/exchange_rm are only to be used by KVM for VFIO.
The exchange_rm callback is defined for IODA1/IODA2 powernv platforms.
This replaces list_for_each_entry_rcu with its lockless version as
from now on pnv_pci_ioda2_tce_invalidate() can be called in
the real mode too.
Signed-off-by: Alexey Kardashevskiy <redacted>
---
arch/powerpc/include/asm/iommu.h | 7 +++++++
arch/powerpc/kernel/iommu.c | 15 +++++++++++++++
arch/powerpc/platforms/powernv/pci-ioda.c | 28 +++++++++++++++++++++++++++-
3 files changed, 49 insertions(+), 1 deletion(-)
The existing in-kernel TCE table for emulated devices contains
guest physical addresses which are accesses by emulated devices.
Since we need to keep this information for VFIO devices too
in order to implement H_GET_TCE, we are reusing it.
This adds IOMMU group list to kvmppc_spapr_tce_table. Each group
will have an iommu_table pointer.
This adds kvm_spapr_tce_attach_iommu_group() helper and its detach
counterpart to manage the lists.
This puts a group when:
- guest copy of TCE table is destroyed when TCE table fd is closed;
- kvm_spapr_tce_detach_iommu_group() is called from
the KVM_DEV_VFIO_GROUP_DEL ioctl handler in the case vfio-pci hotunplug
(will be added in the following patch).
Signed-off-by: Alexey Kardashevskiy <redacted>
---
arch/powerpc/include/asm/kvm_host.h | 8 +++
arch/powerpc/include/asm/kvm_ppc.h | 6 ++
arch/powerpc/kvm/book3s_64_vio.c | 108 ++++++++++++++++++++++++++++++++++++
3 files changed, 122 insertions(+)
In-kernel VFIO acceleration needs different handling in real and virtual
modes which makes it hard to support both modes in the same handler.
This creates a copy of kvmppc_rm_h_stuff_tce and kvmppc_rm_h_put_tce
in addition to the existing kvmppc_rm_h_put_tce_indirect.
Signed-off-by: Alexey Kardashevskiy <redacted>
---
arch/powerpc/kvm/book3s_64_vio.c | 52 +++++++++++++++++++++++++++++++++
arch/powerpc/kvm/book3s_64_vio_hv.c | 8 ++---
arch/powerpc/kvm/book3s_hv_rmhandlers.S | 4 +--
3 files changed, 57 insertions(+), 7 deletions(-)
sPAPR TCE IOMMU is para-virtualized and the guest does map/unmap
via hypercalls which take a logical bus id (LIOBN) as a target IOMMU
identifier. LIOBNs are made up, advertised to guest systems and
linked to IOMMU groups by the user space.
In order to enable acceleration for IOMMU operations in KVM, we need
to tell KVM the information about the LIOBN-to-group mapping.
For that, a new KVM_DEV_VFIO_GROUP_SET_SPAPR_TCE_LIOBN parameter
is added which accepts:
- a VFIO group fd and IO base address to find the actual hardware
TCE table;
- a LIOBN to assign to the found table.
Before notifying KVM about new link, this check the group for being
registered with KVM device in order to release them at unexpected KVM
finish.
This advertises the new KVM_CAP_SPAPR_TCE_VFIO capability to the user
space.
While we are here, this also fixes VFIO KVM device compiling to let it
link to a KVM module.
Signed-off-by: Alexey Kardashevskiy <redacted>
---
Documentation/virtual/kvm/devices/vfio.txt | 21 +++++-
arch/powerpc/kvm/Kconfig | 1 +
arch/powerpc/kvm/Makefile | 5 +-
arch/powerpc/kvm/powerpc.c | 1 +
include/uapi/linux/kvm.h | 9 +++
virt/kvm/vfio.c | 106 +++++++++++++++++++++++++++++
6 files changed, 140 insertions(+), 3 deletions(-)
@@ -16,7 +16,24 @@ Groups: KVM_DEV_VFIO_GROUP attributes: KVM_DEV_VFIO_GROUP_ADD: Add a VFIO group to VFIO-KVM device tracking+ kvm_device_attr.addr points to an int32_t file descriptor+ for the VFIO group.+ KVM_DEV_VFIO_GROUP_DEL: Remove a VFIO group from VFIO-KVM device tracking+ kvm_device_attr.addr points to an int32_t file descriptor+ for the VFIO group.-For each, kvm_device_attr.addr points to an int32_t file descriptor-for the VFIO group.+ KVM_DEV_VFIO_GROUP_SET_SPAPR_TCE_LIOBN: sets a liobn for a VFIO group+ kvm_device_attr.addr points to a struct:+ struct kvm_vfio_spapr_tce_liobn {+ __u32 argsz;+ __s32 fd;+ __u32 liobn;+ __u8 pad[4];+ __u64 start_addr;+ };+ where+ @argsz is the size of kvm_vfio_spapr_tce_liobn;+ @fd is a file descriptor for a VFIO group;+ @liobn is a logical bus id to be associated with the group;+ @start_addr is a DMA window offset on the IO (PCI) bus
This adds a capability number for in-kernel support for VFIO on
SPAPR platform.
The capability will tell the user space whether in-kernel handlers of
H_PUT_TCE can handle VFIO-targeted requests or not. If not, the user space
must not attempt allocating a TCE table in the host kernel via
the KVM_CREATE_SPAPR_TCE KVM ioctl because in that case TCE requests
will not be passed to the user space which is desired action in
the situation like that.
Signed-off-by: Alexey Kardashevskiy <redacted>
---
include/uapi/linux/kvm.h | 1 +
1 file changed, 1 insertion(+)
This makes mm_iommu_lookup() able to work in realmode by replacing
list_for_each_entry_rcu() (which can do debug stuff which can fail in
real mode) with list_for_each_entry_lockless().
This adds realmode version of mm_iommu_ua_to_hpa() which adds
explicit vmalloc'd-to-linear address conversion.
Unlike mm_iommu_ua_to_hpa(), mm_iommu_rm_ua_to_hpa() can fail.
This changes mm_iommu_preregistered() to receive @mm as in real mode
@current does not always have a correct pointer.
This adds realmode version of mm_iommu_lookup() which receives @mm
(for the same reason as for mm_iommu_preregistered()) and uses
lockless version of list_for_each_entry_rcu().
Signed-off-by: Alexey Kardashevskiy <redacted>
---
arch/powerpc/include/asm/mmu_context.h | 6 ++++-
arch/powerpc/mm/mmu_context_iommu.c | 45 ++++++++++++++++++++++++++++++----
2 files changed, 45 insertions(+), 6 deletions(-)
VFIO on sPAPR already implements guest memory pre-registration
when the entire guest RAM gets pinned. This can be used to translate
the physical address of a guest page containing the TCE list
from H_PUT_TCE_INDIRECT.
This makes use of the pre-registrered memory API to access TCE list
pages in order to avoid unnecessary locking on the KVM memory
reverse map.
Signed-off-by: Alexey Kardashevskiy <redacted>
---
arch/powerpc/kvm/book3s_64_vio_hv.c | 86 ++++++++++++++++++++++++++++++-------
1 file changed, 70 insertions(+), 16 deletions(-)
This allows the host kernel to handle H_PUT_TCE, H_PUT_TCE_INDIRECT
and H_STUFF_TCE requests targeted an IOMMU TCE table used for VFIO
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; this is not expected
to happen ever though.
The first user of this is VFIO on POWER. Trampolines to the VFIO external
user API functions are required for this patch.
This uses a VFIO KVM device to associate a logical bus number (LIOBN)
with an VFIO IOMMU group fd and enable in-kernel handling of map/unmap
requests.
To make use of the feature, the user space has to create a guest view
of the TCE table via KVM_CAP_SPAPR_TCE/KVM_CAP_SPAPR_TCE_64 and
then associate a LIOBN with this table via VFIO KVM device,
a KVM_DEV_VFIO_GROUP_SET_SPAPR_TCE_LIOBN property (which is added in
the next patch).
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: Alexey Kardashevskiy <redacted>
---
arch/powerpc/kvm/book3s_64_vio.c | 184 +++++++++++++++++++++++++++++++++++
arch/powerpc/kvm/book3s_64_vio_hv.c | 186 ++++++++++++++++++++++++++++++++++++
2 files changed, 370 insertions(+)
It does not make much sense to have KVM in book3s-64 and
not to have IOMMU bits for PCI pass through support as it costs little
and allows VFIO to function on book3s KVM.
Having IOMMU_API always enabled makes it unnecessary to have a lot of
"#ifdef IOMMU_API" in arch/powerpc/kvm/book3s_64_vio*. With those
ifdef's we could have only user space emulated devices accelerated
(but not VFIO) which do not seem to be very useful.
Signed-off-by: Alexey Kardashevskiy <redacted>
---
arch/powerpc/kvm/Kconfig | 1 +
1 file changed, 1 insertion(+)
From: David Gibson <hidden> Date: 2016-03-07 05:31:00
On Mon, Mar 07, 2016 at 02:41:10PM +1100, Alexey Kardashevskiy wrote:
This makes mm_iommu_lookup() able to work in realmode by replacing
list_for_each_entry_rcu() (which can do debug stuff which can fail in
real mode) with list_for_each_entry_lockless().
This adds realmode version of mm_iommu_ua_to_hpa() which adds
explicit vmalloc'd-to-linear address conversion.
Unlike mm_iommu_ua_to_hpa(), mm_iommu_rm_ua_to_hpa() can fail.
This changes mm_iommu_preregistered() to receive @mm as in real mode
@current does not always have a correct pointer.
So, I'd generally expect a parameter called @mm to be an mm_struct *,
not a mm_context_t.
This adds realmode version of mm_iommu_lookup() which receives @mm
(for the same reason as for mm_iommu_preregistered()) and uses
lockless version of list_for_each_entry_rcu().
Signed-off-by: Alexey Kardashevskiy <redacted>
@@ -284,6 +299,26 @@ long mm_iommu_ua_to_hpa(struct mm_iommu_table_group_mem_t *mem, } EXPORT_SYMBOL_GPL(mm_iommu_ua_to_hpa);+long mm_iommu_rm_ua_to_hpa(struct mm_iommu_table_group_mem_t *mem,+ unsigned long ua, unsigned long *hpa)+{+ const long entry = (ua - mem->ua) >> PAGE_SHIFT;+ void *va = &mem->hpas[entry];+ unsigned long *ra;++ if (entry >= mem->entries)+ return -EFAULT;++ ra = (void *) vmalloc_to_phys(va);+ if (!ra)+ return -EFAULT;++ *hpa = *ra | (ua & ~PAGE_MASK);++ return 0;+}+EXPORT_SYMBOL_GPL(mm_iommu_rm_ua_to_hpa);+ long mm_iommu_mapped_inc(struct mm_iommu_table_group_mem_t *mem) { if (atomic64_inc_not_zero(&mem->mapped))
--
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
From: David Gibson <hidden> Date: 2016-03-07 05:31:00
On Mon, Mar 07, 2016 at 02:41:09PM +1100, Alexey Kardashevskiy wrote:
This adds a capability number for in-kernel support for VFIO on
SPAPR platform.
The capability will tell the user space whether in-kernel handlers of
H_PUT_TCE can handle VFIO-targeted requests or not. If not, the user space
must not attempt allocating a TCE table in the host kernel via
the KVM_CREATE_SPAPR_TCE KVM ioctl because in that case TCE requests
will not be passed to the user space which is desired action in
the situation like that.
Signed-off-by: Alexey Kardashevskiy <redacted>
--
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
From: David Gibson <hidden> Date: 2016-03-07 07:30:25
On Mon, Mar 07, 2016 at 02:41:12PM +1100, Alexey Kardashevskiy wrote:
quoted hunk
In real mode, TCE tables are invalidated using different
cache-inhibited store instructions which is different from
the virtual mode.
This defines and implements exchange_rm() callback. This does not
define set_rm/clear_rm/flush_rm callbacks as there is no user for those -
exchange/exchange_rm are only to be used by KVM for VFIO.
The exchange_rm callback is defined for IODA1/IODA2 powernv platforms.
This replaces list_for_each_entry_rcu with its lockless version as
from now on pnv_pci_ioda2_tce_invalidate() can be called in
the real mode too.
Signed-off-by: Alexey Kardashevskiy <redacted>
---
arch/powerpc/include/asm/iommu.h | 7 +++++++
arch/powerpc/kernel/iommu.c | 15 +++++++++++++++
arch/powerpc/platforms/powernv/pci-ioda.c | 28 +++++++++++++++++++++++++++-
3 files changed, 49 insertions(+), 1 deletion(-)
@@ -1791,6 +1791,18 @@ static int pnv_ioda1_tce_xchg(struct iommu_table *tbl, long index,returnret;}++staticintpnv_ioda1_tce_xchg_rm(structiommu_table*tbl,longindex,+unsignedlong*hpa,enumdma_data_direction*direction)+{+longret=pnv_tce_xchg(tbl,index,hpa,direction);++if(!ret&&(tbl->it_type&+(TCE_PCI_SWINV_CREATE|TCE_PCI_SWINV_FREE)))+pnv_pci_ioda1_tce_invalidate(tbl,index,1,true);++returnret;+}#endif
Both your _rm variants are identical to the non _rm versions. Why not
just set the function poiinter to the same thing, rather than copying
the whole function.
quoted hunk
static void pnv_ioda1_tce_free(struct iommu_table *tbl, long index,
--
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
From: David Gibson <hidden> Date: 2016-03-07 07:30:25
On Mon, Mar 07, 2016 at 02:41:11PM +1100, Alexey Kardashevskiy wrote:
VFIO on sPAPR already implements guest memory pre-registration
when the entire guest RAM gets pinned. This can be used to translate
the physical address of a guest page containing the TCE list
from H_PUT_TCE_INDIRECT.
This makes use of the pre-registrered memory API to access TCE list
pages in order to avoid unnecessary locking on the KVM memory
reverse map.
Signed-off-by: Alexey Kardashevskiy <redacted>
Ok.. so, what's the benefit of not having to lock the rmap?
@@ -180,6 +180,38 @@ long kvmppc_gpa_to_ua(struct kvm *kvm, unsigned long gpa,EXPORT_SYMBOL_GPL(kvmppc_gpa_to_ua);#ifdef CONFIG_KVM_BOOK3S_HV_POSSIBLE+staticmm_context_t*kvmppc_mm_context(structkvm_vcpu*vcpu)+{+structtask_struct*task;++task=vcpu->arch.run_task;+if(unlikely(!task||!task->mm))+returnNULL;++return&task->mm->context;+}++staticinlineboolkvmppc_preregistered(structkvm_vcpu*vcpu)+{+mm_context_t*mm=kvmppc_mm_context(vcpu);++if(unlikely(!mm))+returnfalse;++returnmm_iommu_preregistered(mm);+}++staticstructmm_iommu_table_group_mem_t*kvmppc_rm_iommu_lookup(+structkvm_vcpu*vcpu,unsignedlongua,unsignedlongsize)+{+mm_context_t*mm=kvmppc_mm_context(vcpu);++if(unlikely(!mm))+returnNULL;++returnmm_iommu_lookup_rm(mm,ua,size);+}+longkvmppc_h_put_tce(structkvm_vcpu*vcpu,unsignedlongliobn,unsignedlongioba,unsignedlongtce){
@@ -261,23 +293,44 @@ long kvmppc_rm_h_put_tce_indirect(struct kvm_vcpu *vcpu,if(ret!=H_SUCCESS)returnret;-if(kvmppc_gpa_to_ua(vcpu->kvm,tce_list,&ua,&rmap))-returnH_TOO_HARD;+if(kvmppc_preregistered(vcpu)){+/*+*Wegethereifguestmemorywaspre-registeredwhich+*isnormallyVFIOcaseandgpa->hpatranslationdoesnot+*dependonhpt.+*/+structmm_iommu_table_group_mem_t*mem;-rmap=(void*)vmalloc_to_phys(rmap);+if(kvmppc_gpa_to_ua(vcpu->kvm,tce_list,&ua,NULL))+returnH_TOO_HARD;-/*-*SynchronizewiththeMMUnotifiercallbacksin-*book3s_64_mmu_hv.c(kvm_unmap_hva_hvetc.).-*Whilewehavethermaplock,coderunningonotherCPUs-*cannotfinishunmappingthehostrealpagethatbacks-*thisguestrealpage,soweareOKtoaccessthehost-*realpage.-*/-lock_rmap(rmap);-if(kvmppc_rm_ua_to_hpa(vcpu,ua,&tces)){-ret=H_TOO_HARD;-gotounlock_exit;+mem=kvmppc_rm_iommu_lookup(vcpu,ua,IOMMU_PAGE_SIZE_4K);+if(!mem||mm_iommu_rm_ua_to_hpa(mem,ua,&tces))+returnH_TOO_HARD;+}else{+/*+*Thisisemulateddevicescase.+*Wedonotrequirememorytobepreregisteredinthiscase+*solockrmapanddo__find_linux_pte_or_hugepte().+*/+if(kvmppc_gpa_to_ua(vcpu->kvm,tce_list,&ua,&rmap))+returnH_TOO_HARD;++rmap=(void*)vmalloc_to_phys(rmap);++/*+*SynchronizewiththeMMUnotifiercallbacksin+*book3s_64_mmu_hv.c(kvm_unmap_hva_hvetc.).+*Whilewehavethermaplock,coderunningonotherCPUs+*cannotfinishunmappingthehostrealpagethatbacks+*thisguestrealpage,soweareOKtoaccessthehost+*realpage.+*/+lock_rmap(rmap);+if(kvmppc_rm_ua_to_hpa(vcpu,ua,&tces)){+ret=H_TOO_HARD;+gotounlock_exit;+}}for(i=0;i<npages;++i){
@@ -291,7 +344,8 @@ long kvmppc_rm_h_put_tce_indirect(struct kvm_vcpu *vcpu,}unlock_exit:-unlock_rmap(rmap);+if(rmap)
I don't see where rmap is initialized to NULL in the case where it's
not being used.
+ unlock_rmap(rmap);
return ret;
}
--
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
From: David Gibson <hidden> Date: 2016-03-07 07:30:25
On Mon, Mar 07, 2016 at 02:41:14PM +1100, Alexey Kardashevskiy wrote:
quoted hunk
The existing in-kernel TCE table for emulated devices contains
guest physical addresses which are accesses by emulated devices.
Since we need to keep this information for VFIO devices too
in order to implement H_GET_TCE, we are reusing it.
This adds IOMMU group list to kvmppc_spapr_tce_table. Each group
will have an iommu_table pointer.
This adds kvm_spapr_tce_attach_iommu_group() helper and its detach
counterpart to manage the lists.
This puts a group when:
- guest copy of TCE table is destroyed when TCE table fd is closed;
- kvm_spapr_tce_detach_iommu_group() is called from
the KVM_DEV_VFIO_GROUP_DEL ioctl handler in the case vfio-pci hotunplug
(will be added in the following patch).
Signed-off-by: Alexey Kardashevskiy <redacted>
---
arch/powerpc/include/asm/kvm_host.h | 8 +++
arch/powerpc/include/asm/kvm_ppc.h | 6 ++
arch/powerpc/kvm/book3s_64_vio.c | 108 ++++++++++++++++++++++++++++++++++++
3 files changed, 122 insertions(+)
@@ -181,6 +288,7 @@ long kvm_vm_ioctl_create_spapr_tce(struct kvm *kvm, stt->offset = args->offset; stt->size = size; stt->kvm = kvm;+ INIT_LIST_HEAD_RCU(&stt->groups); for (i = 0; i < npages; i++) { stt->pages[i] = alloc_page(GFP_KERNEL | __GFP_ZERO);
--
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
On Mon, Mar 07, 2016 at 02:41:12PM +1100, Alexey Kardashevskiy wrote:
quoted
In real mode, TCE tables are invalidated using different
cache-inhibited store instructions which is different from
the virtual mode.
This defines and implements exchange_rm() callback. This does not
define set_rm/clear_rm/flush_rm callbacks as there is no user for those -
exchange/exchange_rm are only to be used by KVM for VFIO.
The exchange_rm callback is defined for IODA1/IODA2 powernv platforms.
This replaces list_for_each_entry_rcu with its lockless version as
from now on pnv_pci_ioda2_tce_invalidate() can be called in
the real mode too.
Signed-off-by: Alexey Kardashevskiy <redacted>
---
arch/powerpc/include/asm/iommu.h | 7 +++++++
arch/powerpc/kernel/iommu.c | 15 +++++++++++++++
arch/powerpc/platforms/powernv/pci-ioda.c | 28 +++++++++++++++++++++++++++-
3 files changed, 49 insertions(+), 1 deletion(-)
@@ -1791,6 +1791,18 @@ static int pnv_ioda1_tce_xchg(struct iommu_table *tbl, long index,returnret;}++staticintpnv_ioda1_tce_xchg_rm(structiommu_table*tbl,longindex,+unsignedlong*hpa,enumdma_data_direction*direction)+{+longret=pnv_tce_xchg(tbl,index,hpa,direction);++if(!ret&&(tbl->it_type&+(TCE_PCI_SWINV_CREATE|TCE_PCI_SWINV_FREE)))+pnv_pci_ioda1_tce_invalidate(tbl,index,1,true);++returnret;+}#endif
Both your _rm variants are identical to the non _rm versions. Why not
just set the function poiinter to the same thing, rather than copying
the whole function.
The last parameter - "rm" - to pnv_pci_ioda1_tce_invalidate() is different.
quoted
static void pnv_ioda1_tce_free(struct iommu_table *tbl, long index,
On Mon, Mar 07, 2016 at 02:41:14PM +1100, Alexey Kardashevskiy wrote:
quoted
The existing in-kernel TCE table for emulated devices contains
guest physical addresses which are accesses by emulated devices.
Since we need to keep this information for VFIO devices too
in order to implement H_GET_TCE, we are reusing it.
This adds IOMMU group list to kvmppc_spapr_tce_table. Each group
will have an iommu_table pointer.
This adds kvm_spapr_tce_attach_iommu_group() helper and its detach
counterpart to manage the lists.
This puts a group when:
- guest copy of TCE table is destroyed when TCE table fd is closed;
- kvm_spapr_tce_detach_iommu_group() is called from
the KVM_DEV_VFIO_GROUP_DEL ioctl handler in the case vfio-pci hotunplug
(will be added in the following patch).
Signed-off-by: Alexey Kardashevskiy <redacted>
---
arch/powerpc/include/asm/kvm_host.h | 8 +++
arch/powerpc/include/asm/kvm_ppc.h | 6 ++
arch/powerpc/kvm/book3s_64_vio.c | 108 ++++++++++++++++++++++++++++++++++++
3 files changed, 122 insertions(+)
What's the reason for this kind of two-phase deletion? Dereffing the
group here, and setting to NULL, then actually removing from the liast above.
Well, this way I have only one RCU-delayed release_spapr_tce_table(). The
other option would be to call for each @kg:
- list_del(&kg->next);
- call_rcu()
as release_spapr_tce_table() won't be able to delete them - they are not in
the list anymore.
I suppose I can reuse kvm_spapr_tce_put_group(), this looks inaccurate...
quoted
kvm_put_kvm(stt->kvm);
kvmppc_account_memlimit(
@@ -146,6 +161,98 @@ static const struct file_operations kvm_spapr_tce_fops = { .release = kvm_spapr_tce_release, };+extern long kvm_spapr_tce_attach_iommu_group(struct kvm *kvm,+ unsigned long liobn,+ phys_addr_t start_addr,+ struct iommu_group *grp)+{+ struct kvmppc_spapr_tce_table *stt = NULL;+ struct iommu_table_group *table_group;+ long i;+ bool found = false;+ struct kvmppc_spapr_tce_group *kg;+ struct iommu_table *tbltmp;++ /* Check this LIOBN hasn't been previously allocated */
Hrm. attach takes an explicit liobn, but this one iterates over all
liobns. Why the asymmetry?
For attach(), LIOBN is specified in an additional (to VFIO KVM device's
"add group") ioctl(). There is no need for "detach" ioctl() as we only want
this detach() to happen when a group is removed from a container, and in
this case the usual KVM_DEV_VFIO_GROUP_DEL is good enough hint that we need
to detach LIOBN. Since _DEL does not take LIOBN, here I have a loop.
I'll put this in the commit log next time.
From: David Gibson <hidden> Date: 2016-03-08 05:10:53
On Mon, Mar 07, 2016 at 08:38:13PM +1100, Alexey Kardashevskiy wrote:
On 03/07/2016 05:25 PM, David Gibson wrote:
quoted
On Mon, Mar 07, 2016 at 02:41:14PM +1100, Alexey Kardashevskiy wrote:
quoted
The existing in-kernel TCE table for emulated devices contains
guest physical addresses which are accesses by emulated devices.
Since we need to keep this information for VFIO devices too
in order to implement H_GET_TCE, we are reusing it.
This adds IOMMU group list to kvmppc_spapr_tce_table. Each group
will have an iommu_table pointer.
This adds kvm_spapr_tce_attach_iommu_group() helper and its detach
counterpart to manage the lists.
This puts a group when:
- guest copy of TCE table is destroyed when TCE table fd is closed;
- kvm_spapr_tce_detach_iommu_group() is called from
the KVM_DEV_VFIO_GROUP_DEL ioctl handler in the case vfio-pci hotunplug
(will be added in the following patch).
Signed-off-by: Alexey Kardashevskiy <redacted>
---
arch/powerpc/include/asm/kvm_host.h | 8 +++
arch/powerpc/include/asm/kvm_ppc.h | 6 ++
arch/powerpc/kvm/book3s_64_vio.c | 108 ++++++++++++++++++++++++++++++++++++
3 files changed, 122 insertions(+)
What's the reason for this kind of two-phase deletion? Dereffing the
group here, and setting to NULL, then actually removing from the liast above.
Well, this way I have only one RCU-delayed release_spapr_tce_table(). The
other option would be to call for each @kg:
- list_del(&kg->next);
- call_rcu()
as release_spapr_tce_table() won't be able to delete them - they are not in
the list anymore.
Ah, ok, that makes sense.
I suppose I can reuse kvm_spapr_tce_put_group(), this looks inaccurate...
quoted
quoted
kvm_put_kvm(stt->kvm);
kvmppc_account_memlimit(
@@ -146,6 +161,98 @@ static const struct file_operations kvm_spapr_tce_fops = { .release = kvm_spapr_tce_release, };+extern long kvm_spapr_tce_attach_iommu_group(struct kvm *kvm,+ unsigned long liobn,+ phys_addr_t start_addr,+ struct iommu_group *grp)+{+ struct kvmppc_spapr_tce_table *stt = NULL;+ struct iommu_table_group *table_group;+ long i;+ bool found = false;+ struct kvmppc_spapr_tce_group *kg;+ struct iommu_table *tbltmp;++ /* Check this LIOBN hasn't been previously allocated */
Hrm. attach takes an explicit liobn, but this one iterates over all
liobns. Why the asymmetry?
For attach(), LIOBN is specified in an additional (to VFIO KVM device's "add
group") ioctl(). There is no need for "detach" ioctl() as we only want this
detach() to happen when a group is removed from a container, and in this
case the usual KVM_DEV_VFIO_GROUP_DEL is good enough hint that we need to
detach LIOBN. Since _DEL does not take LIOBN, here I have a loop.
I'll put this in the commit log next time.
@@ -181,6 +288,7 @@ long kvm_vm_ioctl_create_spapr_tce(struct kvm *kvm, stt->offset = args->offset; stt->size = size; stt->kvm = kvm;+ INIT_LIST_HEAD_RCU(&stt->groups); for (i = 0; i < npages; i++) { stt->pages[i] = alloc_page(GFP_KERNEL | __GFP_ZERO);
--
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
From: David Gibson <hidden> Date: 2016-03-08 05:10:53
On Mon, Mar 07, 2016 at 06:32:23PM +1100, Alexey Kardashevskiy wrote:
On 03/07/2016 05:05 PM, David Gibson wrote:
quoted
On Mon, Mar 07, 2016 at 02:41:12PM +1100, Alexey Kardashevskiy wrote:
quoted
In real mode, TCE tables are invalidated using different
cache-inhibited store instructions which is different from
the virtual mode.
This defines and implements exchange_rm() callback. This does not
define set_rm/clear_rm/flush_rm callbacks as there is no user for those -
exchange/exchange_rm are only to be used by KVM for VFIO.
The exchange_rm callback is defined for IODA1/IODA2 powernv platforms.
This replaces list_for_each_entry_rcu with its lockless version as
from now on pnv_pci_ioda2_tce_invalidate() can be called in
the real mode too.
Signed-off-by: Alexey Kardashevskiy <redacted>
---
arch/powerpc/include/asm/iommu.h | 7 +++++++
arch/powerpc/kernel/iommu.c | 15 +++++++++++++++
arch/powerpc/platforms/powernv/pci-ioda.c | 28 +++++++++++++++++++++++++++-
3 files changed, 49 insertions(+), 1 deletion(-)
@@ -1791,6 +1791,18 @@ static int pnv_ioda1_tce_xchg(struct iommu_table *tbl, long index,returnret;}++staticintpnv_ioda1_tce_xchg_rm(structiommu_table*tbl,longindex,+unsignedlong*hpa,enumdma_data_direction*direction)+{+longret=pnv_tce_xchg(tbl,index,hpa,direction);++if(!ret&&(tbl->it_type&+(TCE_PCI_SWINV_CREATE|TCE_PCI_SWINV_FREE)))+pnv_pci_ioda1_tce_invalidate(tbl,index,1,true);++returnret;+}#endif
Both your _rm variants are identical to the non _rm versions. Why not
just set the function poiinter to the same thing, rather than copying
the whole function.
The last parameter - "rm" - to pnv_pci_ioda1_tce_invalidate() is
different.
Ah, missed that, sorry.
quoted
quoted
static void pnv_ioda1_tce_free(struct iommu_table *tbl, long index,
--
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
On Mon, Mar 07, 2016 at 02:41:11PM +1100, Alexey Kardashevskiy wrote:
quoted
VFIO on sPAPR already implements guest memory pre-registration
when the entire guest RAM gets pinned. This can be used to translate
the physical address of a guest page containing the TCE list
from H_PUT_TCE_INDIRECT.
This makes use of the pre-registrered memory API to access TCE list
pages in order to avoid unnecessary locking on the KVM memory
reverse map.
Signed-off-by: Alexey Kardashevskiy <redacted>
Ok.. so, what's the benefit of not having to lock the rmap?
From: David Gibson <hidden> Date: 2016-03-08 06:33:01
On Tue, Mar 08, 2016 at 04:47:20PM +1100, Alexey Kardashevskiy wrote:
On 03/07/2016 05:00 PM, David Gibson wrote:
quoted
On Mon, Mar 07, 2016 at 02:41:11PM +1100, Alexey Kardashevskiy wrote:
quoted
VFIO on sPAPR already implements guest memory pre-registration
when the entire guest RAM gets pinned. This can be used to translate
the physical address of a guest page containing the TCE list
from H_PUT_TCE_INDIRECT.
This makes use of the pre-registrered memory API to access TCE list
pages in order to avoid unnecessary locking on the KVM memory
reverse map.
Signed-off-by: Alexey Kardashevskiy <redacted>
Ok.. so, what's the benefit of not having to lock the rmap?
Less locking -> less racing == good, no?
Well.. maybe. The increased difficulty in verifying that the code is
correct isn't always a good price to pay.
@@ -180,6 +180,38 @@ long kvmppc_gpa_to_ua(struct kvm *kvm, unsigned long gpa,EXPORT_SYMBOL_GPL(kvmppc_gpa_to_ua);#ifdef CONFIG_KVM_BOOK3S_HV_POSSIBLE+staticmm_context_t*kvmppc_mm_context(structkvm_vcpu*vcpu)+{+structtask_struct*task;++task=vcpu->arch.run_task;+if(unlikely(!task||!task->mm))+returnNULL;++return&task->mm->context;+}++staticinlineboolkvmppc_preregistered(structkvm_vcpu*vcpu)+{+mm_context_t*mm=kvmppc_mm_context(vcpu);++if(unlikely(!mm))+returnfalse;++returnmm_iommu_preregistered(mm);+}++staticstructmm_iommu_table_group_mem_t*kvmppc_rm_iommu_lookup(+structkvm_vcpu*vcpu,unsignedlongua,unsignedlongsize)+{+mm_context_t*mm=kvmppc_mm_context(vcpu);++if(unlikely(!mm))+returnNULL;++returnmm_iommu_lookup_rm(mm,ua,size);+}+longkvmppc_h_put_tce(structkvm_vcpu*vcpu,unsignedlongliobn,unsignedlongioba,unsignedlongtce){
@@ -261,23 +293,44 @@ long kvmppc_rm_h_put_tce_indirect(struct kvm_vcpu *vcpu,if(ret!=H_SUCCESS)returnret;-if(kvmppc_gpa_to_ua(vcpu->kvm,tce_list,&ua,&rmap))-returnH_TOO_HARD;+if(kvmppc_preregistered(vcpu)){+/*+*Wegethereifguestmemorywaspre-registeredwhich+*isnormallyVFIOcaseandgpa->hpatranslationdoesnot+*dependonhpt.+*/+structmm_iommu_table_group_mem_t*mem;-rmap=(void*)vmalloc_to_phys(rmap);+if(kvmppc_gpa_to_ua(vcpu->kvm,tce_list,&ua,NULL))+returnH_TOO_HARD;-/*-*SynchronizewiththeMMUnotifiercallbacksin-*book3s_64_mmu_hv.c(kvm_unmap_hva_hvetc.).-*Whilewehavethermaplock,coderunningonotherCPUs-*cannotfinishunmappingthehostrealpagethatbacks-*thisguestrealpage,soweareOKtoaccessthehost-*realpage.-*/-lock_rmap(rmap);-if(kvmppc_rm_ua_to_hpa(vcpu,ua,&tces)){-ret=H_TOO_HARD;-gotounlock_exit;+mem=kvmppc_rm_iommu_lookup(vcpu,ua,IOMMU_PAGE_SIZE_4K);+if(!mem||mm_iommu_rm_ua_to_hpa(mem,ua,&tces))+returnH_TOO_HARD;+}else{+/*+*Thisisemulateddevicescase.+*Wedonotrequirememorytobepreregisteredinthiscase+*solockrmapanddo__find_linux_pte_or_hugepte().+*/+if(kvmppc_gpa_to_ua(vcpu->kvm,tce_list,&ua,&rmap))+returnH_TOO_HARD;++rmap=(void*)vmalloc_to_phys(rmap);++/*+*SynchronizewiththeMMUnotifiercallbacksin+*book3s_64_mmu_hv.c(kvm_unmap_hva_hvetc.).+*Whilewehavethermaplock,coderunningonotherCPUs+*cannotfinishunmappingthehostrealpagethatbacks+*thisguestrealpage,soweareOKtoaccessthehost+*realpage.+*/+lock_rmap(rmap);+if(kvmppc_rm_ua_to_hpa(vcpu,ua,&tces)){+ret=H_TOO_HARD;+gotounlock_exit;+}}for(i=0;i<npages;++i){
@@ -291,7 +344,8 @@ long kvmppc_rm_h_put_tce_indirect(struct kvm_vcpu *vcpu,}unlock_exit:-unlock_rmap(rmap);+if(rmap)
I don't see where rmap is initialized to NULL in the case where it's
not being used.
@rmap is not new to this function, and it has always been initialized to
NULL as it was returned via a pointer from kvmppc_gpa_to_ua().
This comment confuses me. Looking closer at the code I see you're
right, and it's initialized to NULL where defined, which I missed.
But that has nothing to do with being returned by pointer from
kvmppc_gpa_to_ua(), since one of your branches in the new code no
longer passes &rmap to that function.
--
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
From: David Gibson <hidden> Date: 2016-03-08 06:33:02
On Mon, Mar 07, 2016 at 02:41:15PM +1100, Alexey Kardashevskiy wrote:
In-kernel VFIO acceleration needs different handling in real and virtual
modes which makes it hard to support both modes in the same handler.
This creates a copy of kvmppc_rm_h_stuff_tce and kvmppc_rm_h_put_tce
in addition to the existing kvmppc_rm_h_put_tce_indirect.
Signed-off-by: Alexey Kardashevskiy <redacted>
--
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
From: David Gibson <hidden> Date: 2016-03-08 11:08:17
On Mon, Mar 07, 2016 at 02:41:16PM +1100, 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 used for VFIO
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; this is not expected
to happen ever though.
Well... not expect to happen with a qemu which uses this. Presumably
it will fall back to userspace routinely if you have an old qemu that
doesn't add the liobn mappings.
The first user of this is VFIO on POWER. Trampolines to the VFIO external
user API functions are required for this patch.
I'm not sure what you mean by "trampoline" here.
This uses a VFIO KVM device to associate a logical bus number (LIOBN)
with an VFIO IOMMU group fd and enable in-kernel handling of map/unmap
requests.
Group fd? Or container fd? The group fd wouldn't make a lot of
sense.
To make use of the feature, the user space has to create a guest view
of the TCE table via KVM_CAP_SPAPR_TCE/KVM_CAP_SPAPR_TCE_64 and
then associate a LIOBN with this table via VFIO KVM device,
a KVM_DEV_VFIO_GROUP_SET_SPAPR_TCE_LIOBN property (which is added in
the next patch).
Tests show that this patch increases transmission speed from 220MB/s
to 750..1020MB/s on 10Gb network (Chelsea CXGB3 10Gb ethernet card).
Is that with or without DDW (i.e. with or without a 64-bit DMA window)?
@@ -232,6 +384,16 @@ long kvmppc_rm_h_put_tce(struct kvm_vcpu *vcpu, unsigned long liobn,if(ret!=H_SUCCESS)returnret;+list_for_each_entry_lockless(kg,&stt->groups,next){+if(kg->tbl==tbltmp)+continue;+tbltmp=kg->tbl;+ret=kvmppc_rm_h_put_tce_iommu(vcpu,kg->tbl,+liobn,ioba,tce);+if(ret!=H_SUCCESS)+returnret;+}+kvmppc_tce_put(stt,ioba>>stt->page_shift,tce);returnH_SUCCESS;
@@ -272,6 +434,7 @@ long kvmppc_rm_h_put_tce_indirect(struct kvm_vcpu *vcpu,longi,ret=H_SUCCESS;unsignedlongtces,entry,ua=0;unsignedlong*rmap=NULL;+structiommu_table*tbltmp=NULL;stt=kvmppc_find_table(vcpu,liobn);if(!stt)
@@ -299,6 +462,7 @@ long kvmppc_rm_h_put_tce_indirect(struct kvm_vcpu *vcpu,*dependonhpt.*/structmm_iommu_table_group_mem_t*mem;+structkvmppc_spapr_tce_group*kg;if(kvmppc_gpa_to_ua(vcpu->kvm,tce_list,&ua,NULL))returnH_TOO_HARD;
@@ -306,6 +470,16 @@ long kvmppc_rm_h_put_tce_indirect(struct kvm_vcpu *vcpu,mem=kvmppc_rm_iommu_lookup(vcpu,ua,IOMMU_PAGE_SIZE_4K);if(!mem||mm_iommu_rm_ua_to_hpa(mem,ua,&tces))returnH_TOO_HARD;++list_for_each_entry_lockless(kg,&stt->groups,next){+if(kg->tbl==tbltmp)+continue;+tbltmp=kg->tbl;+ret=kvmppc_rm_h_put_tce_indirect_iommu(vcpu,+kg->tbl,ioba,(u64*)tces,npages);+if(ret!=H_SUCCESS)+returnret;+}}else{/**Thisisemulateddevicescase.
@@ -355,6 +529,8 @@ long kvmppc_rm_h_stuff_tce(struct kvm_vcpu *vcpu,{structkvmppc_spapr_tce_table*stt;longi,ret;+structkvmppc_spapr_tce_group*kg;+structiommu_table*tbltmp=NULL;stt=kvmppc_find_table(vcpu,liobn);if(!stt)
@@ -368,6 +544,16 @@ long kvmppc_rm_h_stuff_tce(struct kvm_vcpu *vcpu,if(tce_value&(TCE_PCI_WRITE|TCE_PCI_READ))returnH_PARAMETER;+list_for_each_entry_lockless(kg,&stt->groups,next){+if(kg->tbl==tbltmp)+continue;+tbltmp=kg->tbl;+ret=kvmppc_rm_h_stuff_tce_iommu(vcpu,kg->tbl,+liobn,ioba,tce_value,npages);+if(ret!=H_SUCCESS)+returnret;+}+for(i=0;i<npages;++i,ioba+=(1ULL<<stt->page_shift))kvmppc_tce_put(stt,ioba>>stt->page_shift,tce_value);
--
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
From: David Gibson <hidden> Date: 2016-03-09 05:50:26
On Mon, Mar 07, 2016 at 02:41:17PM +1100, Alexey Kardashevskiy wrote:
quoted hunk
sPAPR TCE IOMMU is para-virtualized and the guest does map/unmap
via hypercalls which take a logical bus id (LIOBN) as a target IOMMU
identifier. LIOBNs are made up, advertised to guest systems and
linked to IOMMU groups by the user space.
In order to enable acceleration for IOMMU operations in KVM, we need
to tell KVM the information about the LIOBN-to-group mapping.
For that, a new KVM_DEV_VFIO_GROUP_SET_SPAPR_TCE_LIOBN parameter
is added which accepts:
- a VFIO group fd and IO base address to find the actual hardware
TCE table;
- a LIOBN to assign to the found table.
Before notifying KVM about new link, this check the group for being
registered with KVM device in order to release them at unexpected KVM
finish.
This advertises the new KVM_CAP_SPAPR_TCE_VFIO capability to the user
space.
While we are here, this also fixes VFIO KVM device compiling to let it
link to a KVM module.
Signed-off-by: Alexey Kardashevskiy <redacted>
---
Documentation/virtual/kvm/devices/vfio.txt | 21 +++++-
arch/powerpc/kvm/Kconfig | 1 +
arch/powerpc/kvm/Makefile | 5 +-
arch/powerpc/kvm/powerpc.c | 1 +
include/uapi/linux/kvm.h | 9 +++
virt/kvm/vfio.c | 106 +++++++++++++++++++++++++++++
6 files changed, 140 insertions(+), 3 deletions(-)
@@ -16,7 +16,24 @@ Groups: KVM_DEV_VFIO_GROUP attributes: KVM_DEV_VFIO_GROUP_ADD: Add a VFIO group to VFIO-KVM device tracking+ kvm_device_attr.addr points to an int32_t file descriptor+ for the VFIO group.
AFAICT these changes are accurate for VFIO as it is already, in which
case it might be clearer to put them in a separate patch.
KVM_DEV_VFIO_GROUP_DEL: Remove a VFIO group from VFIO-KVM device tracking
+ kvm_device_attr.addr points to an int32_t file descriptor
+ for the VFIO group.
-For each, kvm_device_attr.addr points to an int32_t file descriptor
-for the VFIO group.
+ KVM_DEV_VFIO_GROUP_SET_SPAPR_TCE_LIOBN: sets a liobn for a VFIO group
+ kvm_device_attr.addr points to a struct:
+ struct kvm_vfio_spapr_tce_liobn {
+ __u32 argsz;
+ __s32 fd;
+ __u32 liobn;
+ __u8 pad[4];
+ __u64 start_addr;
+ };
+ where
+ @argsz is the size of kvm_vfio_spapr_tce_liobn;
+ @fd is a file descriptor for a VFIO group;
+ @liobn is a logical bus id to be associated with the group;
+ @start_addr is a DMA window offset on the IO (PCI) bus
For the cause of DDW and multiple windows, I'm assuming you can call
this multiple times with different LIOBNs and the same IOMMU group?
Please don't disable the VFIO device for the non-book3s case. I added
it (even though it didn't do anything until now) so that libvirt
wouldn't choke when it finds it's not available. Obviously the new
ioctl needs to be only for the right IOMMU setup, but the device
itself should be available always.
@@ -225,6 +328,9 @@ static int kvm_vfio_has_attr(struct kvm_device *dev, switch (attr->attr) { case KVM_DEV_VFIO_GROUP_ADD: case KVM_DEV_VFIO_GROUP_DEL:+#ifdef CONFIG_SPAPR_TCE_IOMMU+ case KVM_DEV_VFIO_GROUP_SET_SPAPR_TCE_LIOBN:+#endif return 0; }
--
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
On Mon, Mar 07, 2016 at 02:41:16PM +1100, Alexey Kardashevskiy wrote:
quoted
This allows the host kernel to handle H_PUT_TCE, H_PUT_TCE_INDIRECT
and H_STUFF_TCE requests targeted an IOMMU TCE table used for VFIO
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; this is not expected
to happen ever though.
Well... not expect to happen with a qemu which uses this. Presumably
it will fall back to userspace routinely if you have an old qemu that
doesn't add the liobn mappings.
Ah. Ok, thanks, I'll add this to the commit log.
quoted
The first user of this is VFIO on POWER. Trampolines to the VFIO external
user API functions are required for this patch.
I'm not sure what you mean by "trampoline" here.
For example, look at kvm_vfio_group_get_external_user. It calls
symbol_get(vfio_group_get_external_user) and then calls a function via the
returned pointer.
Is there a better word for this?
quoted
This uses a VFIO KVM device to associate a logical bus number (LIOBN)
with an VFIO IOMMU group fd and enable in-kernel handling of map/unmap
requests.
Group fd? Or container fd? The group fd wouldn't make a lot of
sense.
Group. KVM has no idea about containers.
quoted
To make use of the feature, the user space has to create a guest view
of the TCE table via KVM_CAP_SPAPR_TCE/KVM_CAP_SPAPR_TCE_64 and
then associate a LIOBN with this table via VFIO KVM device,
a KVM_DEV_VFIO_GROUP_SET_SPAPR_TCE_LIOBN property (which is added in
the next patch).
Tests show that this patch increases transmission speed from 220MB/s
to 750..1020MB/s on 10Gb network (Chelsea CXGB3 10Gb ethernet card).
Is that with or without DDW (i.e. with or without a 64-bit DMA window)?
Without DDW, I should have mentioned this. The patch is from the times when
there was no DDW :(
H_HARDWARE? Or H_PARAMETER? This essentially means the guest has
supplied a bad physical address, doesn't it?
Well, may be. I'll change. If it not H_TOO_HARD, it does not make any
difference after all :)
quoted
+ if (kvmppc_gpa_to_ua(kvm, gpa, &ua, NULL))
+ return H_HARDWARE;
+
+ mem = mm_iommu_lookup(ua, 1ULL << tbl->it_page_shift);
+ if (!mem)
+ return H_HARDWARE;
+
+ if (mm_iommu_ua_to_hpa(mem, ua, &hpa))
+ return H_HARDWARE;
+
+ if (mm_iommu_mapped_inc(mem))
+ return H_HARDWARE;
+
+ ret = iommu_tce_xchg(tbl, entry, &hpa, &dir);
+ if (ret) {
+ mm_iommu_mapped_dec(mem);
+ return H_TOO_HARD;
+ }
+
+ if (dir != DMA_NONE)
+ kvmppc_tce_iommu_mapped_dec(tbl, entry);
+
+ *pua = ua;
IIUC this means you have a copy of the UA for every group attached to
the TCE table, but they'll all be the same. Any way to avoid that
duplication?
It is for every container, not a group. On P8, I allow multiple groups to
go to the same container, that means that a container has one or two
iommu_table, and each iommu_table has this "ua" list but since tables are
different (window size, page size, content), these "ua" arrays are also
different.
--
Alexey
On Tue, Mar 08, 2016 at 04:47:20PM +1100, Alexey Kardashevskiy wrote:
quoted
On 03/07/2016 05:00 PM, David Gibson wrote:
quoted
On Mon, Mar 07, 2016 at 02:41:11PM +1100, Alexey Kardashevskiy wrote:
quoted
VFIO on sPAPR already implements guest memory pre-registration
when the entire guest RAM gets pinned. This can be used to translate
the physical address of a guest page containing the TCE list
from H_PUT_TCE_INDIRECT.
This makes use of the pre-registrered memory API to access TCE list
pages in order to avoid unnecessary locking on the KVM memory
reverse map.
Signed-off-by: Alexey Kardashevskiy <redacted>
Ok.. so, what's the benefit of not having to lock the rmap?
Less locking -> less racing == good, no?
Well.. maybe. The increased difficulty in verifying that the code is
correct isn't always a good price to pay.
@@ -180,6 +180,38 @@ long kvmppc_gpa_to_ua(struct kvm *kvm, unsigned long gpa,EXPORT_SYMBOL_GPL(kvmppc_gpa_to_ua);#ifdef CONFIG_KVM_BOOK3S_HV_POSSIBLE+staticmm_context_t*kvmppc_mm_context(structkvm_vcpu*vcpu)+{+structtask_struct*task;++task=vcpu->arch.run_task;+if(unlikely(!task||!task->mm))+returnNULL;++return&task->mm->context;+}++staticinlineboolkvmppc_preregistered(structkvm_vcpu*vcpu)+{+mm_context_t*mm=kvmppc_mm_context(vcpu);++if(unlikely(!mm))+returnfalse;++returnmm_iommu_preregistered(mm);+}++staticstructmm_iommu_table_group_mem_t*kvmppc_rm_iommu_lookup(+structkvm_vcpu*vcpu,unsignedlongua,unsignedlongsize)+{+mm_context_t*mm=kvmppc_mm_context(vcpu);++if(unlikely(!mm))+returnNULL;++returnmm_iommu_lookup_rm(mm,ua,size);+}+longkvmppc_h_put_tce(structkvm_vcpu*vcpu,unsignedlongliobn,unsignedlongioba,unsignedlongtce){
@@ -261,23 +293,44 @@ long kvmppc_rm_h_put_tce_indirect(struct kvm_vcpu *vcpu,if(ret!=H_SUCCESS)returnret;-if(kvmppc_gpa_to_ua(vcpu->kvm,tce_list,&ua,&rmap))-returnH_TOO_HARD;+if(kvmppc_preregistered(vcpu)){+/*+*Wegethereifguestmemorywaspre-registeredwhich+*isnormallyVFIOcaseandgpa->hpatranslationdoesnot+*dependonhpt.+*/+structmm_iommu_table_group_mem_t*mem;-rmap=(void*)vmalloc_to_phys(rmap);+if(kvmppc_gpa_to_ua(vcpu->kvm,tce_list,&ua,NULL))+returnH_TOO_HARD;-/*-*SynchronizewiththeMMUnotifiercallbacksin-*book3s_64_mmu_hv.c(kvm_unmap_hva_hvetc.).-*Whilewehavethermaplock,coderunningonotherCPUs-*cannotfinishunmappingthehostrealpagethatbacks-*thisguestrealpage,soweareOKtoaccessthehost-*realpage.-*/-lock_rmap(rmap);-if(kvmppc_rm_ua_to_hpa(vcpu,ua,&tces)){-ret=H_TOO_HARD;-gotounlock_exit;+mem=kvmppc_rm_iommu_lookup(vcpu,ua,IOMMU_PAGE_SIZE_4K);+if(!mem||mm_iommu_rm_ua_to_hpa(mem,ua,&tces))+returnH_TOO_HARD;+}else{+/*+*Thisisemulateddevicescase.+*Wedonotrequirememorytobepreregisteredinthiscase+*solockrmapanddo__find_linux_pte_or_hugepte().+*/+if(kvmppc_gpa_to_ua(vcpu->kvm,tce_list,&ua,&rmap))+returnH_TOO_HARD;++rmap=(void*)vmalloc_to_phys(rmap);++/*+*SynchronizewiththeMMUnotifiercallbacksin+*book3s_64_mmu_hv.c(kvm_unmap_hva_hvetc.).+*Whilewehavethermaplock,coderunningonotherCPUs+*cannotfinishunmappingthehostrealpagethatbacks+*thisguestrealpage,soweareOKtoaccessthehost+*realpage.+*/+lock_rmap(rmap);+if(kvmppc_rm_ua_to_hpa(vcpu,ua,&tces)){+ret=H_TOO_HARD;+gotounlock_exit;+}}for(i=0;i<npages;++i){
@@ -291,7 +344,8 @@ long kvmppc_rm_h_put_tce_indirect(struct kvm_vcpu *vcpu,}unlock_exit:-unlock_rmap(rmap);+if(rmap)
I don't see where rmap is initialized to NULL in the case where it's
not being used.
@rmap is not new to this function, and it has always been initialized to
NULL as it was returned via a pointer from kvmppc_gpa_to_ua().
This comment confuses me. Looking closer at the code I see you're
right, and it's initialized to NULL where defined, which I missed.
But that has nothing to do with being returned by pointer from
kvmppc_gpa_to_ua(), since one of your branches in the new code no
longer passes &rmap to that function.
So? The code is still correct - the "preregistered branch" does not touch
NULL pointer, it remains NULL and unlock_rmap() is not called. I agree the
patch is not the easiest to read but how can I improve it to get your "rb"?
Replace "if(rmap)" with "if (kvmppc_preregistered(vcpu))"? Move that loop
between lock_rmap/unlock_rmap to a helper? kvmppc_rm_h_put_tce_indirect()
is not big enough to justify splitting, the comments inside it are though...
--
Alexey
On Mon, Mar 07, 2016 at 02:41:17PM +1100, Alexey Kardashevskiy wrote:
quoted
sPAPR TCE IOMMU is para-virtualized and the guest does map/unmap
via hypercalls which take a logical bus id (LIOBN) as a target IOMMU
identifier. LIOBNs are made up, advertised to guest systems and
linked to IOMMU groups by the user space.
In order to enable acceleration for IOMMU operations in KVM, we need
to tell KVM the information about the LIOBN-to-group mapping.
For that, a new KVM_DEV_VFIO_GROUP_SET_SPAPR_TCE_LIOBN parameter
is added which accepts:
- a VFIO group fd and IO base address to find the actual hardware
TCE table;
- a LIOBN to assign to the found table.
Before notifying KVM about new link, this check the group for being
registered with KVM device in order to release them at unexpected KVM
finish.
This advertises the new KVM_CAP_SPAPR_TCE_VFIO capability to the user
space.
While we are here, this also fixes VFIO KVM device compiling to let it
link to a KVM module.
Signed-off-by: Alexey Kardashevskiy <redacted>
---
Documentation/virtual/kvm/devices/vfio.txt | 21 +++++-
arch/powerpc/kvm/Kconfig | 1 +
arch/powerpc/kvm/Makefile | 5 +-
arch/powerpc/kvm/powerpc.c | 1 +
include/uapi/linux/kvm.h | 9 +++
virt/kvm/vfio.c | 106 +++++++++++++++++++++++++++++
6 files changed, 140 insertions(+), 3 deletions(-)
@@ -16,7 +16,24 @@ Groups: KVM_DEV_VFIO_GROUP attributes: KVM_DEV_VFIO_GROUP_ADD: Add a VFIO group to VFIO-KVM device tracking+ kvm_device_attr.addr points to an int32_t file descriptor+ for the VFIO group.
AFAICT these changes are accurate for VFIO as it is already, in which
case it might be clearer to put them in a separate patch.
quoted
KVM_DEV_VFIO_GROUP_DEL: Remove a VFIO group from VFIO-KVM device tracking
+ kvm_device_attr.addr points to an int32_t file descriptor
+ for the VFIO group.
-For each, kvm_device_attr.addr points to an int32_t file descriptor
-for the VFIO group.
+ KVM_DEV_VFIO_GROUP_SET_SPAPR_TCE_LIOBN: sets a liobn for a VFIO group
+ kvm_device_attr.addr points to a struct:
+ struct kvm_vfio_spapr_tce_liobn {
+ __u32 argsz;
+ __s32 fd;
+ __u32 liobn;
+ __u8 pad[4];
+ __u64 start_addr;
+ };
+ where
+ @argsz is the size of kvm_vfio_spapr_tce_liobn;
+ @fd is a file descriptor for a VFIO group;
+ @liobn is a logical bus id to be associated with the group;
+ @start_addr is a DMA window offset on the IO (PCI) bus
For the cause of DDW and multiple windows, I'm assuming you can call
this multiple times with different LIOBNs and the same IOMMU group?
Yes. It is called twice per each group (when DDW is activated) - for 32bit
and 64bit windows, this is why @start_addr is there.
Please don't disable the VFIO device for the non-book3s case. I added
it (even though it didn't do anything until now) so that libvirt
wouldn't choke when it finds it's not available. Obviously the new
ioctl needs to be only for the right IOMMU setup, but the device
itself should be available always.
Ah. Ok, I'll fix this. I just wanted to be able to compile kvm as a module.
Shouldn't this go in the same patch that introduced the attach
function?
Having less patches which touch different maintainers areas is better. I
cannot avoid touching both PPC KVM and VFIO in this patch but I can in
"[PATCH kernel 6/9] KVM: PPC: Associate IOMMU group with guest view of TCE
table".
@@ -186,6 +222,10 @@ static int kvm_vfio_set_group(struct kvm_device *dev, long attr, u64 arg) continue; list_del(&kvg->node);+#ifdef CONFIG_SPAPR_TCE_IOMMU
Better to make a no-op version of the call than have to #ifdef at the
callsite.
It is questionable. A x86 reader may decide that
KVM_DEV_VFIO_GROUP_SET_SPAPR_TCE_LIOBN is implemented for x86 and get confused.
quoted
+ kvm_vfio_spapr_detach_iommu_group(dev->kvm,
+ kvg->vfio_group);
+#endif
kvm_vfio_group_put_external_user(kvg->vfio_group);
kfree(kvg);
ret = 0;
@@ -201,6 +241,69 @@ static int kvm_vfio_set_group(struct kvm_device *dev, long attr, u64 arg) kvm_vfio_update_coherency(dev); return ret;++#ifdef CONFIG_SPAPR_TCE_IOMMU+ case KVM_DEV_VFIO_GROUP_SET_SPAPR_TCE_LIOBN: {+ struct kvm_vfio_spapr_tce_liobn param;+ unsigned long minsz;+ struct kvm_vfio *kv = dev->private;+ struct vfio_group *vfio_group;+ struct kvm_vfio_group *kvg;+ struct fd f;++ minsz = offsetofend(struct kvm_vfio_spapr_tce_liobn,+ start_addr);++ if (copy_from_user(¶m, (void __user *)arg, minsz))+ return -EFAULT;++ if (param.argsz < minsz)+ return -EINVAL;++ f = fdget(param.fd);+ if (!f.file)+ return -EBADF;++ vfio_group = kvm_vfio_group_get_external_user(f.file);+ fdput(f);++ if (IS_ERR(vfio_group))+ return PTR_ERR(vfio_group);++ ret = -ENOENT;
Shouldn't there be some runtime test for the type of the IOMMU? It's
possible a kernel could be built for a platform supporting multiple
IOMMU types.
Well, may make sense but I do not know to test that. The IOMMU type is a
VFIO container property, not a group property and here (KVM) we only have
groups.
And calling iommu_group_get_iommudata() is quite useless as it returns a
void pointer... I could probably check that the release() callback is the
one I set via iommu_group_set_iommudata() but there is no API to get it
from a group.
And I cannot really imagine a kernel with CONFIG_PPC_BOOK3S_64 (and
therefore KVM_CAP_SPAPR_TCE_VFIO enabled) with different IOMMU types. Can
the same kernel binary image work on both BOOK3S and embedded PPC? Where
these other types can come from?
From: David Gibson <hidden> Date: 2016-03-09 23:52:28
On Wed, Mar 09, 2016 at 07:55:53PM +1100, Alexey Kardashevskiy wrote:
On 03/08/2016 05:30 PM, David Gibson wrote:
quoted
On Tue, Mar 08, 2016 at 04:47:20PM +1100, Alexey Kardashevskiy wrote:
quoted
On 03/07/2016 05:00 PM, David Gibson wrote:
quoted
On Mon, Mar 07, 2016 at 02:41:11PM +1100, Alexey Kardashevskiy wrote:
quoted
VFIO on sPAPR already implements guest memory pre-registration
when the entire guest RAM gets pinned. This can be used to translate
the physical address of a guest page containing the TCE list
from H_PUT_TCE_INDIRECT.
This makes use of the pre-registrered memory API to access TCE list
pages in order to avoid unnecessary locking on the KVM memory
reverse map.
Signed-off-by: Alexey Kardashevskiy <redacted>
Ok.. so, what's the benefit of not having to lock the rmap?
Less locking -> less racing == good, no?
Well.. maybe. The increased difficulty in verifying that the code is
correct isn't always a good price to pay.
@@ -180,6 +180,38 @@ long kvmppc_gpa_to_ua(struct kvm *kvm, unsigned long gpa,EXPORT_SYMBOL_GPL(kvmppc_gpa_to_ua);#ifdef CONFIG_KVM_BOOK3S_HV_POSSIBLE+staticmm_context_t*kvmppc_mm_context(structkvm_vcpu*vcpu)+{+structtask_struct*task;++task=vcpu->arch.run_task;+if(unlikely(!task||!task->mm))+returnNULL;++return&task->mm->context;+}++staticinlineboolkvmppc_preregistered(structkvm_vcpu*vcpu)+{+mm_context_t*mm=kvmppc_mm_context(vcpu);++if(unlikely(!mm))+returnfalse;++returnmm_iommu_preregistered(mm);+}++staticstructmm_iommu_table_group_mem_t*kvmppc_rm_iommu_lookup(+structkvm_vcpu*vcpu,unsignedlongua,unsignedlongsize)+{+mm_context_t*mm=kvmppc_mm_context(vcpu);++if(unlikely(!mm))+returnNULL;++returnmm_iommu_lookup_rm(mm,ua,size);+}+longkvmppc_h_put_tce(structkvm_vcpu*vcpu,unsignedlongliobn,unsignedlongioba,unsignedlongtce){
@@ -261,23 +293,44 @@ long kvmppc_rm_h_put_tce_indirect(struct kvm_vcpu *vcpu,if(ret!=H_SUCCESS)returnret;-if(kvmppc_gpa_to_ua(vcpu->kvm,tce_list,&ua,&rmap))-returnH_TOO_HARD;+if(kvmppc_preregistered(vcpu)){+/*+*Wegethereifguestmemorywaspre-registeredwhich+*isnormallyVFIOcaseandgpa->hpatranslationdoesnot+*dependonhpt.+*/+structmm_iommu_table_group_mem_t*mem;-rmap=(void*)vmalloc_to_phys(rmap);+if(kvmppc_gpa_to_ua(vcpu->kvm,tce_list,&ua,NULL))+returnH_TOO_HARD;-/*-*SynchronizewiththeMMUnotifiercallbacksin-*book3s_64_mmu_hv.c(kvm_unmap_hva_hvetc.).-*Whilewehavethermaplock,coderunningonotherCPUs-*cannotfinishunmappingthehostrealpagethatbacks-*thisguestrealpage,soweareOKtoaccessthehost-*realpage.-*/-lock_rmap(rmap);-if(kvmppc_rm_ua_to_hpa(vcpu,ua,&tces)){-ret=H_TOO_HARD;-gotounlock_exit;+mem=kvmppc_rm_iommu_lookup(vcpu,ua,IOMMU_PAGE_SIZE_4K);+if(!mem||mm_iommu_rm_ua_to_hpa(mem,ua,&tces))+returnH_TOO_HARD;+}else{+/*+*Thisisemulateddevicescase.+*Wedonotrequirememorytobepreregisteredinthiscase+*solockrmapanddo__find_linux_pte_or_hugepte().+*/+if(kvmppc_gpa_to_ua(vcpu->kvm,tce_list,&ua,&rmap))+returnH_TOO_HARD;++rmap=(void*)vmalloc_to_phys(rmap);++/*+*SynchronizewiththeMMUnotifiercallbacksin+*book3s_64_mmu_hv.c(kvm_unmap_hva_hvetc.).+*Whilewehavethermaplock,coderunningonotherCPUs+*cannotfinishunmappingthehostrealpagethatbacks+*thisguestrealpage,soweareOKtoaccessthehost+*realpage.+*/+lock_rmap(rmap);+if(kvmppc_rm_ua_to_hpa(vcpu,ua,&tces)){+ret=H_TOO_HARD;+gotounlock_exit;+}}for(i=0;i<npages;++i){
@@ -291,7 +344,8 @@ long kvmppc_rm_h_put_tce_indirect(struct kvm_vcpu *vcpu,}unlock_exit:-unlock_rmap(rmap);+if(rmap)
I don't see where rmap is initialized to NULL in the case where it's
not being used.
@rmap is not new to this function, and it has always been initialized to
NULL as it was returned via a pointer from kvmppc_gpa_to_ua().
This comment confuses me. Looking closer at the code I see you're
right, and it's initialized to NULL where defined, which I missed.
But that has nothing to do with being returned by pointer from
kvmppc_gpa_to_ua(), since one of your branches in the new code no
longer passes &rmap to that function.
So? The code is still correct - the "preregistered branch" does not touch
NULL pointer, it remains NULL and unlock_rmap() is not called. I agree the
patch is not the easiest to read but how can I improve it to get your "rb"?
Replace "if(rmap)" with "if (kvmppc_preregistered(vcpu))"? Move that loop
between lock_rmap/unlock_rmap to a helper? kvmppc_rm_h_put_tce_indirect() is
not big enough to justify splitting, the comments inside it are though...
Sorry, I wasn't clear. I no longer have a specific objection. I left
out the R-b, since there have been enough other comments on the series
that I was expecting a respin, so I was planning to re-review in the
context of an updated series.
--
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
From: David Gibson <hidden> Date: 2016-03-10 05:23:00
On Wed, Mar 09, 2016 at 07:46:47PM +1100, Alexey Kardashevskiy wrote:
On 03/08/2016 10:08 PM, David Gibson wrote:
quoted
On Mon, Mar 07, 2016 at 02:41:16PM +1100, Alexey Kardashevskiy wrote:
quoted
This allows the host kernel to handle H_PUT_TCE, H_PUT_TCE_INDIRECT
and H_STUFF_TCE requests targeted an IOMMU TCE table used for VFIO
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; this is not expected
to happen ever though.
Well... not expect to happen with a qemu which uses this. Presumably
it will fall back to userspace routinely if you have an old qemu that
doesn't add the liobn mappings.
Ah. Ok, thanks, I'll add this to the commit log.
Ok.
quoted
quoted
The first user of this is VFIO on POWER. Trampolines to the VFIO external
user API functions are required for this patch.
I'm not sure what you mean by "trampoline" here.
For example, look at kvm_vfio_group_get_external_user. It calls
symbol_get(vfio_group_get_external_user) and then calls a function via the
returned pointer.
Is there a better word for this?
Uh.. probably although I don't immediately know what. "Trampoline"
usually refers to code on the stack used for bouncing places, which
isn't what this resembles.
quoted
quoted
This uses a VFIO KVM device to associate a logical bus number (LIOBN)
with an VFIO IOMMU group fd and enable in-kernel handling of map/unmap
requests.
Group fd? Or container fd? The group fd wouldn't make a lot of
sense.
Group. KVM has no idea about containers.
That's not going to fly. Having a liobn registered against just one
group in a container makes no sense at all. Conceptually, if not
physically, the container shares a single set of TCE tables. If
handling that means teaching KVM the concept of containers, then so be
it.
Btw, I'm not sure yet if extending the existing vfio kvm device to
make the vfio<->kvm linkages makes sense. I think the reason some x86
machines need that is quite different from how we're using it for
Power. I haven't got a clear enough picture yet to be sure either
way.
The other option that would seem likely to me would be a "bind VFIO
container" ioctl() on the fd associated with a kernel accelerated TCE table.
quoted
quoted
To make use of the feature, the user space has to create a guest view
of the TCE table via KVM_CAP_SPAPR_TCE/KVM_CAP_SPAPR_TCE_64 and
then associate a LIOBN with this table via VFIO KVM device,
a KVM_DEV_VFIO_GROUP_SET_SPAPR_TCE_LIOBN property (which is added in
the next patch).
Tests show that this patch increases transmission speed from 220MB/s
to 750..1020MB/s on 10Gb network (Chelsea CXGB3 10Gb ethernet card).
Is that with or without DDW (i.e. with or without a 64-bit DMA window)?
Without DDW, I should have mentioned this. The patch is from the times when
there was no DDW :(
H_HARDWARE? Or H_PARAMETER? This essentially means the guest has
supplied a bad physical address, doesn't it?
Well, may be. I'll change. If it not H_TOO_HARD, it does not make any
difference after all :)
quoted
quoted
+ if (kvmppc_gpa_to_ua(kvm, gpa, &ua, NULL))
+ return H_HARDWARE;
+
+ mem = mm_iommu_lookup(ua, 1ULL << tbl->it_page_shift);
+ if (!mem)
+ return H_HARDWARE;
+
+ if (mm_iommu_ua_to_hpa(mem, ua, &hpa))
+ return H_HARDWARE;
+
+ if (mm_iommu_mapped_inc(mem))
+ return H_HARDWARE;
+
+ ret = iommu_tce_xchg(tbl, entry, &hpa, &dir);
+ if (ret) {
+ mm_iommu_mapped_dec(mem);
+ return H_TOO_HARD;
+ }
+
+ if (dir != DMA_NONE)
+ kvmppc_tce_iommu_mapped_dec(tbl, entry);
+
+ *pua = ua;
IIUC this means you have a copy of the UA for every group attached to
the TCE table, but they'll all be the same. Any way to avoid that
duplication?
It is for every container, not a group. On P8, I allow multiple groups to go
to the same container, that means that a container has one or two
iommu_table, and each iommu_table has this "ua" list but since tables are
different (window size, page size, content), these "ua" arrays are also
different.
Erm.. but h_put_tce iterates h_put_tce_iommu through all the groups
attached to the stt, and each one seems to update pua.
Or is that what the if (kg->tbl == tbltmp) continue; is supposed to
avoid? In which case what ensures that the stt->groups list is
ordered by tbl pointer?
--
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
From: David Gibson <hidden> Date: 2016-03-10 05:23:00
On Wed, Mar 09, 2016 at 08:20:12PM +1100, Alexey Kardashevskiy wrote:
On 03/09/2016 04:45 PM, David Gibson wrote:
quoted
On Mon, Mar 07, 2016 at 02:41:17PM +1100, Alexey Kardashevskiy wrote:
quoted
sPAPR TCE IOMMU is para-virtualized and the guest does map/unmap
via hypercalls which take a logical bus id (LIOBN) as a target IOMMU
identifier. LIOBNs are made up, advertised to guest systems and
linked to IOMMU groups by the user space.
In order to enable acceleration for IOMMU operations in KVM, we need
to tell KVM the information about the LIOBN-to-group mapping.
For that, a new KVM_DEV_VFIO_GROUP_SET_SPAPR_TCE_LIOBN parameter
is added which accepts:
- a VFIO group fd and IO base address to find the actual hardware
TCE table;
- a LIOBN to assign to the found table.
Before notifying KVM about new link, this check the group for being
registered with KVM device in order to release them at unexpected KVM
finish.
This advertises the new KVM_CAP_SPAPR_TCE_VFIO capability to the user
space.
While we are here, this also fixes VFIO KVM device compiling to let it
link to a KVM module.
Signed-off-by: Alexey Kardashevskiy <redacted>
---
Documentation/virtual/kvm/devices/vfio.txt | 21 +++++-
arch/powerpc/kvm/Kconfig | 1 +
arch/powerpc/kvm/Makefile | 5 +-
arch/powerpc/kvm/powerpc.c | 1 +
include/uapi/linux/kvm.h | 9 +++
virt/kvm/vfio.c | 106 +++++++++++++++++++++++++++++
6 files changed, 140 insertions(+), 3 deletions(-)
@@ -16,7 +16,24 @@ Groups: KVM_DEV_VFIO_GROUP attributes: KVM_DEV_VFIO_GROUP_ADD: Add a VFIO group to VFIO-KVM device tracking+ kvm_device_attr.addr points to an int32_t file descriptor+ for the VFIO group.
AFAICT these changes are accurate for VFIO as it is already, in which
case it might be clearer to put them in a separate patch.
quoted
KVM_DEV_VFIO_GROUP_DEL: Remove a VFIO group from VFIO-KVM device tracking
+ kvm_device_attr.addr points to an int32_t file descriptor
+ for the VFIO group.
-For each, kvm_device_attr.addr points to an int32_t file descriptor
-for the VFIO group.
+ KVM_DEV_VFIO_GROUP_SET_SPAPR_TCE_LIOBN: sets a liobn for a VFIO group
+ kvm_device_attr.addr points to a struct:
+ struct kvm_vfio_spapr_tce_liobn {
+ __u32 argsz;
+ __s32 fd;
+ __u32 liobn;
+ __u8 pad[4];
+ __u64 start_addr;
+ };
+ where
+ @argsz is the size of kvm_vfio_spapr_tce_liobn;
+ @fd is a file descriptor for a VFIO group;
+ @liobn is a logical bus id to be associated with the group;
+ @start_addr is a DMA window offset on the IO (PCI) bus
For the cause of DDW and multiple windows, I'm assuming you can call
this multiple times with different LIOBNs and the same IOMMU group?
Yes. It is called twice per each group (when DDW is activated) - for 32bit
and 64bit windows, this is why @start_addr is there.
Please don't disable the VFIO device for the non-book3s case. I added
it (even though it didn't do anything until now) so that libvirt
wouldn't choke when it finds it's not available. Obviously the new
ioctl needs to be only for the right IOMMU setup, but the device
itself should be available always.
Ah. Ok, I'll fix this. I just wanted to be able to compile kvm as a module.
Shouldn't this go in the same patch that introduced the attach
function?
Having less patches which touch different maintainers areas is better. I
cannot avoid touching both PPC KVM and VFIO in this patch but I can in
"[PATCH kernel 6/9] KVM: PPC: Associate IOMMU group with guest view of TCE
table".
@@ -186,6 +222,10 @@ static int kvm_vfio_set_group(struct kvm_device *dev, long attr, u64 arg) continue; list_del(&kvg->node);+#ifdef CONFIG_SPAPR_TCE_IOMMU
Better to make a no-op version of the call than have to #ifdef at the
callsite.
It is questionable. A x86 reader may decide that
KVM_DEV_VFIO_GROUP_SET_SPAPR_TCE_LIOBN is implemented for x86 and get
confused.
quoted
quoted
+ kvm_vfio_spapr_detach_iommu_group(dev->kvm,
+ kvg->vfio_group);
+#endif
kvm_vfio_group_put_external_user(kvg->vfio_group);
kfree(kvg);
ret = 0;
@@ -201,6 +241,69 @@ static int kvm_vfio_set_group(struct kvm_device *dev, long attr, u64 arg) kvm_vfio_update_coherency(dev); return ret;++#ifdef CONFIG_SPAPR_TCE_IOMMU+ case KVM_DEV_VFIO_GROUP_SET_SPAPR_TCE_LIOBN: {+ struct kvm_vfio_spapr_tce_liobn param;+ unsigned long minsz;+ struct kvm_vfio *kv = dev->private;+ struct vfio_group *vfio_group;+ struct kvm_vfio_group *kvg;+ struct fd f;++ minsz = offsetofend(struct kvm_vfio_spapr_tce_liobn,+ start_addr);++ if (copy_from_user(¶m, (void __user *)arg, minsz))+ return -EFAULT;++ if (param.argsz < minsz)+ return -EINVAL;++ f = fdget(param.fd);+ if (!f.file)+ return -EBADF;++ vfio_group = kvm_vfio_group_get_external_user(f.file);+ fdput(f);++ if (IS_ERR(vfio_group))+ return PTR_ERR(vfio_group);++ ret = -ENOENT;
Shouldn't there be some runtime test for the type of the IOMMU? It's
possible a kernel could be built for a platform supporting multiple
IOMMU types.
Well, may make sense but I do not know to test that. The IOMMU type is a
VFIO container property, not a group property and here (KVM) we only have
groups.
Which, as mentioned previously, is broken.
And calling iommu_group_get_iommudata() is quite useless as it returns a
void pointer... I could probably check that the release() callback is the
one I set via iommu_group_set_iommudata() but there is no API to get it from
a group.
And I cannot really imagine a kernel with CONFIG_PPC_BOOK3S_64 (and
therefore KVM_CAP_SPAPR_TCE_VFIO enabled) with different IOMMU types. Can
the same kernel binary image work on both BOOK3S and embedded PPC? Where
these other types can come from?
@@ -225,6 +328,9 @@ static int kvm_vfio_has_attr(struct kvm_device *dev, switch (attr->attr) { case KVM_DEV_VFIO_GROUP_ADD: case KVM_DEV_VFIO_GROUP_DEL:+#ifdef CONFIG_SPAPR_TCE_IOMMU+ case KVM_DEV_VFIO_GROUP_SET_SPAPR_TCE_LIOBN:+#endif return 0; }
--
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
From: Paul Mackerras <hidden> Date: 2016-03-10 08:40:08
On Mon, Mar 07, 2016 at 05:00:14PM +1100, David Gibson wrote:
On Mon, Mar 07, 2016 at 02:41:11PM +1100, Alexey Kardashevskiy wrote:
quoted
VFIO on sPAPR already implements guest memory pre-registration
when the entire guest RAM gets pinned. This can be used to translate
the physical address of a guest page containing the TCE list
from H_PUT_TCE_INDIRECT.
This makes use of the pre-registrered memory API to access TCE list
pages in order to avoid unnecessary locking on the KVM memory
reverse map.
Signed-off-by: Alexey Kardashevskiy <redacted>
Ok.. so, what's the benefit of not having to lock the rmap?
It's not primarily about locking or not locking the rmap. The point
is that when memory is pre-registered, we know that all of guest
memory is pinned and we have a flat array mapping GPA to HPA. It's
simpler and quicker to index into that array (even with looking up the
kernel page tables in vmalloc_to_phys) than it is to find the memslot,
lock the rmap entry, look up the user page tables, and unlock the rmap
entry. We were only locking the rmap entry to stop the page being
unmapped and reallocated to something else, but if memory is
pre-registered, it's all pinned, so it can't be reallocated.
Paul.
From: Paul Mackerras <hidden> Date: 2016-03-10 08:44:03
On Mon, Mar 07, 2016 at 02:41:12PM +1100, Alexey Kardashevskiy wrote:
In real mode, TCE tables are invalidated using different
cache-inhibited store instructions which is different from
the virtual mode.
This defines and implements exchange_rm() callback. This does not
define set_rm/clear_rm/flush_rm callbacks as there is no user for those -
exchange/exchange_rm are only to be used by KVM for VFIO.
The exchange_rm callback is defined for IODA1/IODA2 powernv platforms.
This replaces list_for_each_entry_rcu with its lockless version as
from now on pnv_pci_ioda2_tce_invalidate() can be called in
the real mode too.
[snip]
quoted hunk
@@ -1062,6 +1062,21 @@ void iommu_release_ownership(struct iommu_table *tbl) } EXPORT_SYMBOL_GPL(iommu_release_ownership);+long iommu_tce_xchg_rm(struct iommu_table *tbl, unsigned long entry,+ unsigned long *hpa, enum dma_data_direction *direction)+{+ long ret;++ ret = tbl->it_ops->exchange_rm(tbl, entry, hpa, direction);++ if (!ret && ((*direction == DMA_FROM_DEVICE) ||+ (*direction == DMA_BIDIRECTIONAL)))+ SetPageDirty(realmode_pfn_to_page(*hpa >> PAGE_SHIFT));
realmode_pfn_to_page can fail and return NULL, can't it? You need to
handle that situation somehow.
Paul.
From: Paul Mackerras <hidden> Date: 2016-03-10 08:46:25
On Mon, Mar 07, 2016 at 02:41:12PM +1100, Alexey Kardashevskiy wrote:
In real mode, TCE tables are invalidated using different
cache-inhibited store instructions which is different from
the virtual mode.
I suggest "In real mode, TCE tables are invalidated using special
cache-inhibited store instructions which are not available in
virtual mode".
Also, the subject could make people think it's about the kernel xchg()
function defined in <asm/cmpxchg.h>.
Paul.
On Wed, Mar 09, 2016 at 08:20:12PM +1100, Alexey Kardashevskiy wrote:
quoted
On 03/09/2016 04:45 PM, David Gibson wrote:
quoted
On Mon, Mar 07, 2016 at 02:41:17PM +1100, Alexey Kardashevskiy wrote:
quoted
sPAPR TCE IOMMU is para-virtualized and the guest does map/unmap
via hypercalls which take a logical bus id (LIOBN) as a target IOMMU
identifier. LIOBNs are made up, advertised to guest systems and
linked to IOMMU groups by the user space.
In order to enable acceleration for IOMMU operations in KVM, we need
to tell KVM the information about the LIOBN-to-group mapping.
For that, a new KVM_DEV_VFIO_GROUP_SET_SPAPR_TCE_LIOBN parameter
is added which accepts:
- a VFIO group fd and IO base address to find the actual hardware
TCE table;
- a LIOBN to assign to the found table.
Before notifying KVM about new link, this check the group for being
registered with KVM device in order to release them at unexpected KVM
finish.
This advertises the new KVM_CAP_SPAPR_TCE_VFIO capability to the user
space.
While we are here, this also fixes VFIO KVM device compiling to let it
link to a KVM module.
Signed-off-by: Alexey Kardashevskiy <redacted>
---
Documentation/virtual/kvm/devices/vfio.txt | 21 +++++-
arch/powerpc/kvm/Kconfig | 1 +
arch/powerpc/kvm/Makefile | 5 +-
arch/powerpc/kvm/powerpc.c | 1 +
include/uapi/linux/kvm.h | 9 +++
virt/kvm/vfio.c | 106 +++++++++++++++++++++++++++++
6 files changed, 140 insertions(+), 3 deletions(-)
@@ -16,7 +16,24 @@ Groups: KVM_DEV_VFIO_GROUP attributes: KVM_DEV_VFIO_GROUP_ADD: Add a VFIO group to VFIO-KVM device tracking+ kvm_device_attr.addr points to an int32_t file descriptor+ for the VFIO group.
AFAICT these changes are accurate for VFIO as it is already, in which
case it might be clearer to put them in a separate patch.
quoted
KVM_DEV_VFIO_GROUP_DEL: Remove a VFIO group from VFIO-KVM device tracking
+ kvm_device_attr.addr points to an int32_t file descriptor
+ for the VFIO group.
-For each, kvm_device_attr.addr points to an int32_t file descriptor
-for the VFIO group.
+ KVM_DEV_VFIO_GROUP_SET_SPAPR_TCE_LIOBN: sets a liobn for a VFIO group
+ kvm_device_attr.addr points to a struct:
+ struct kvm_vfio_spapr_tce_liobn {
+ __u32 argsz;
+ __s32 fd;
+ __u32 liobn;
+ __u8 pad[4];
+ __u64 start_addr;
+ };
+ where
+ @argsz is the size of kvm_vfio_spapr_tce_liobn;
+ @fd is a file descriptor for a VFIO group;
+ @liobn is a logical bus id to be associated with the group;
+ @start_addr is a DMA window offset on the IO (PCI) bus
For the cause of DDW and multiple windows, I'm assuming you can call
this multiple times with different LIOBNs and the same IOMMU group?
Yes. It is called twice per each group (when DDW is activated) - for 32bit
and 64bit windows, this is why @start_addr is there.
Please don't disable the VFIO device for the non-book3s case. I added
it (even though it didn't do anything until now) so that libvirt
wouldn't choke when it finds it's not available. Obviously the new
ioctl needs to be only for the right IOMMU setup, but the device
itself should be available always.
Ah. Ok, I'll fix this. I just wanted to be able to compile kvm as a module.
Shouldn't this go in the same patch that introduced the attach
function?
Having less patches which touch different maintainers areas is better. I
cannot avoid touching both PPC KVM and VFIO in this patch but I can in
"[PATCH kernel 6/9] KVM: PPC: Associate IOMMU group with guest view of TCE
table".
@@ -186,6 +222,10 @@ static int kvm_vfio_set_group(struct kvm_device *dev, long attr, u64 arg) continue; list_del(&kvg->node);+#ifdef CONFIG_SPAPR_TCE_IOMMU
Better to make a no-op version of the call than have to #ifdef at the
callsite.
It is questionable. A x86 reader may decide that
KVM_DEV_VFIO_GROUP_SET_SPAPR_TCE_LIOBN is implemented for x86 and get
confused.
quoted
quoted
+ kvm_vfio_spapr_detach_iommu_group(dev->kvm,
+ kvg->vfio_group);
+#endif
kvm_vfio_group_put_external_user(kvg->vfio_group);
kfree(kvg);
ret = 0;
@@ -201,6 +241,69 @@ static int kvm_vfio_set_group(struct kvm_device *dev, long attr, u64 arg) kvm_vfio_update_coherency(dev); return ret;++#ifdef CONFIG_SPAPR_TCE_IOMMU+ case KVM_DEV_VFIO_GROUP_SET_SPAPR_TCE_LIOBN: {+ struct kvm_vfio_spapr_tce_liobn param;+ unsigned long minsz;+ struct kvm_vfio *kv = dev->private;+ struct vfio_group *vfio_group;+ struct kvm_vfio_group *kvg;+ struct fd f;++ minsz = offsetofend(struct kvm_vfio_spapr_tce_liobn,+ start_addr);++ if (copy_from_user(¶m, (void __user *)arg, minsz))+ return -EFAULT;++ if (param.argsz < minsz)+ return -EINVAL;++ f = fdget(param.fd);+ if (!f.file)+ return -EBADF;++ vfio_group = kvm_vfio_group_get_external_user(f.file);+ fdput(f);++ if (IS_ERR(vfio_group))+ return PTR_ERR(vfio_group);++ ret = -ENOENT;
Shouldn't there be some runtime test for the type of the IOMMU? It's
possible a kernel could be built for a platform supporting multiple
IOMMU types.
Well, may make sense but I do not know to test that. The IOMMU type is a
VFIO container property, not a group property and here (KVM) we only have
groups.
Which, as mentioned previously, is broken.
Which I am failing to follow you on this.
What I am trying to achieve here is pretty much referencing a group so it
cannot be reused. Plus LIOBNs. Passing a container fd does not make much
sense here as the VFIO device would walk through groups, reference them and
that is it, there is no locking on VFIO containters and so far there was no
need to teach KVM about containers.
What do I miss now?
quoted
And calling iommu_group_get_iommudata() is quite useless as it returns a
void pointer... I could probably check that the release() callback is the
one I set via iommu_group_set_iommudata() but there is no API to get it from
a group.
And I cannot really imagine a kernel with CONFIG_PPC_BOOK3S_64 (and
therefore KVM_CAP_SPAPR_TCE_VFIO enabled) with different IOMMU types. Can
the same kernel binary image work on both BOOK3S and embedded PPC? Where
these other types can come from?
From: David Gibson <hidden> Date: 2016-03-10 23:53:24
On Thu, Mar 10, 2016 at 07:33:05PM +1100, Paul Mackerras wrote:
On Mon, Mar 07, 2016 at 05:00:14PM +1100, David Gibson wrote:
quoted
On Mon, Mar 07, 2016 at 02:41:11PM +1100, Alexey Kardashevskiy wrote:
quoted
VFIO on sPAPR already implements guest memory pre-registration
when the entire guest RAM gets pinned. This can be used to translate
the physical address of a guest page containing the TCE list
from H_PUT_TCE_INDIRECT.
This makes use of the pre-registrered memory API to access TCE list
pages in order to avoid unnecessary locking on the KVM memory
reverse map.
Signed-off-by: Alexey Kardashevskiy <redacted>
Ok.. so, what's the benefit of not having to lock the rmap?
It's not primarily about locking or not locking the rmap. The point
is that when memory is pre-registered, we know that all of guest
memory is pinned and we have a flat array mapping GPA to HPA. It's
simpler and quicker to index into that array (even with looking up the
kernel page tables in vmalloc_to_phys) than it is to find the memslot,
lock the rmap entry, look up the user page tables, and unlock the rmap
entry. We were only locking the rmap entry to stop the page being
unmapped and reallocated to something else, but if memory is
pre-registered, it's all pinned, so it can't be reallocated.
Ok, that makes sense.
Alexey, can you fold some of that rationale into the commit message?
--
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
On Wed, Mar 09, 2016 at 07:46:47PM +1100, Alexey Kardashevskiy wrote:
quoted
On 03/08/2016 10:08 PM, David Gibson wrote:
quoted
On Mon, Mar 07, 2016 at 02:41:16PM +1100, Alexey Kardashevskiy wrote:
quoted
This allows the host kernel to handle H_PUT_TCE, H_PUT_TCE_INDIRECT
and H_STUFF_TCE requests targeted an IOMMU TCE table used for VFIO
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; this is not expected
to happen ever though.
Well... not expect to happen with a qemu which uses this. Presumably
it will fall back to userspace routinely if you have an old qemu that
doesn't add the liobn mappings.
Ah. Ok, thanks, I'll add this to the commit log.
Ok.
quoted
quoted
quoted
The first user of this is VFIO on POWER. Trampolines to the VFIO external
user API functions are required for this patch.
I'm not sure what you mean by "trampoline" here.
For example, look at kvm_vfio_group_get_external_user. It calls
symbol_get(vfio_group_get_external_user) and then calls a function via the
returned pointer.
Is there a better word for this?
Uh.. probably although I don't immediately know what. "Trampoline"
usually refers to code on the stack used for bouncing places, which
isn't what this resembles.
"Dynamic wrapper"?
quoted
quoted
quoted
This uses a VFIO KVM device to associate a logical bus number (LIOBN)
with an VFIO IOMMU group fd and enable in-kernel handling of map/unmap
requests.
Group fd? Or container fd? The group fd wouldn't make a lot of
sense.
Group. KVM has no idea about containers.
That's not going to fly. Having a liobn registered against just one
group in a container makes no sense at all. Conceptually, if not
physically, the container shares a single set of TCE tables. If
handling that means teaching KVM the concept of containers, then so be
it.
Btw, I'm not sure yet if extending the existing vfio kvm device to
make the vfio<->kvm linkages makes sense. I think the reason some x86
machines need that is quite different from how we're using it for
Power. I haven't got a clear enough picture yet to be sure either
way.
The other option that would seem likely to me would be a "bind VFIO
container" ioctl() on the fd associated with a kernel accelerated TCE table.
Oh, I just noticed this response. I need to digest it. Looks like this is
going to take other 2 years to upstream...
quoted
quoted
quoted
To make use of the feature, the user space has to create a guest view
of the TCE table via KVM_CAP_SPAPR_TCE/KVM_CAP_SPAPR_TCE_64 and
then associate a LIOBN with this table via VFIO KVM device,
a KVM_DEV_VFIO_GROUP_SET_SPAPR_TCE_LIOBN property (which is added in
the next patch).
Tests show that this patch increases transmission speed from 220MB/s
to 750..1020MB/s on 10Gb network (Chelsea CXGB3 10Gb ethernet card).
Is that with or without DDW (i.e. with or without a 64-bit DMA window)?
Without DDW, I should have mentioned this. The patch is from the times when
there was no DDW :(
H_HARDWARE? Or H_PARAMETER? This essentially means the guest has
supplied a bad physical address, doesn't it?
Well, may be. I'll change. If it not H_TOO_HARD, it does not make any
difference after all :)
quoted
quoted
+ if (kvmppc_gpa_to_ua(kvm, gpa, &ua, NULL))
+ return H_HARDWARE;
+
+ mem = mm_iommu_lookup(ua, 1ULL << tbl->it_page_shift);
+ if (!mem)
+ return H_HARDWARE;
+
+ if (mm_iommu_ua_to_hpa(mem, ua, &hpa))
+ return H_HARDWARE;
+
+ if (mm_iommu_mapped_inc(mem))
+ return H_HARDWARE;
+
+ ret = iommu_tce_xchg(tbl, entry, &hpa, &dir);
+ if (ret) {
+ mm_iommu_mapped_dec(mem);
+ return H_TOO_HARD;
+ }
+
+ if (dir != DMA_NONE)
+ kvmppc_tce_iommu_mapped_dec(tbl, entry);
+
+ *pua = ua;
IIUC this means you have a copy of the UA for every group attached to
the TCE table, but they'll all be the same. Any way to avoid that
duplication?
It is for every container, not a group. On P8, I allow multiple groups to go
to the same container, that means that a container has one or two
iommu_table, and each iommu_table has this "ua" list but since tables are
different (window size, page size, content), these "ua" arrays are also
different.
Erm.. but h_put_tce iterates h_put_tce_iommu through all the groups
attached to the stt, and each one seems to update pua.
Or is that what the if (kg->tbl == tbltmp) continue; is supposed to
avoid? In which case what ensures that the stt->groups list is
ordered by tbl pointer?
Nothing. In the normal case (POWER8 IODA2) all groups on the same liobn
have the same iommu_table, so the first group's one gets updated, other do
not but it is ok as they use the same table.
In a bad case (POWER7 IODA1, multiple containers per LIOBN) the same @ua
can be updated more than once. Well, not a huge loss.
--
Alexey
From: David Gibson <hidden> Date: 2016-03-15 06:29:23
On Fri, Mar 11, 2016 at 10:09:50AM +1100, Alexey Kardashevskiy wrote:
On 03/10/2016 04:21 PM, David Gibson wrote:
quoted
On Wed, Mar 09, 2016 at 08:20:12PM +1100, Alexey Kardashevskiy wrote:
quoted
On 03/09/2016 04:45 PM, David Gibson wrote:
quoted
On Mon, Mar 07, 2016 at 02:41:17PM +1100, Alexey Kardashevskiy wrote:
quoted
sPAPR TCE IOMMU is para-virtualized and the guest does map/unmap
via hypercalls which take a logical bus id (LIOBN) as a target IOMMU
identifier. LIOBNs are made up, advertised to guest systems and
linked to IOMMU groups by the user space.
In order to enable acceleration for IOMMU operations in KVM, we need
to tell KVM the information about the LIOBN-to-group mapping.
For that, a new KVM_DEV_VFIO_GROUP_SET_SPAPR_TCE_LIOBN parameter
is added which accepts:
- a VFIO group fd and IO base address to find the actual hardware
TCE table;
- a LIOBN to assign to the found table.
Before notifying KVM about new link, this check the group for being
registered with KVM device in order to release them at unexpected KVM
finish.
This advertises the new KVM_CAP_SPAPR_TCE_VFIO capability to the user
space.
While we are here, this also fixes VFIO KVM device compiling to let it
link to a KVM module.
Signed-off-by: Alexey Kardashevskiy <redacted>
---
Documentation/virtual/kvm/devices/vfio.txt | 21 +++++-
arch/powerpc/kvm/Kconfig | 1 +
arch/powerpc/kvm/Makefile | 5 +-
arch/powerpc/kvm/powerpc.c | 1 +
include/uapi/linux/kvm.h | 9 +++
virt/kvm/vfio.c | 106 +++++++++++++++++++++++++++++
6 files changed, 140 insertions(+), 3 deletions(-)
@@ -16,7 +16,24 @@ Groups: KVM_DEV_VFIO_GROUP attributes: KVM_DEV_VFIO_GROUP_ADD: Add a VFIO group to VFIO-KVM device tracking+ kvm_device_attr.addr points to an int32_t file descriptor+ for the VFIO group.
AFAICT these changes are accurate for VFIO as it is already, in which
case it might be clearer to put them in a separate patch.
quoted
KVM_DEV_VFIO_GROUP_DEL: Remove a VFIO group from VFIO-KVM device tracking
+ kvm_device_attr.addr points to an int32_t file descriptor
+ for the VFIO group.
-For each, kvm_device_attr.addr points to an int32_t file descriptor
-for the VFIO group.
+ KVM_DEV_VFIO_GROUP_SET_SPAPR_TCE_LIOBN: sets a liobn for a VFIO group
+ kvm_device_attr.addr points to a struct:
+ struct kvm_vfio_spapr_tce_liobn {
+ __u32 argsz;
+ __s32 fd;
+ __u32 liobn;
+ __u8 pad[4];
+ __u64 start_addr;
+ };
+ where
+ @argsz is the size of kvm_vfio_spapr_tce_liobn;
+ @fd is a file descriptor for a VFIO group;
+ @liobn is a logical bus id to be associated with the group;
+ @start_addr is a DMA window offset on the IO (PCI) bus
For the cause of DDW and multiple windows, I'm assuming you can call
this multiple times with different LIOBNs and the same IOMMU group?
Yes. It is called twice per each group (when DDW is activated) - for 32bit
and 64bit windows, this is why @start_addr is there.
Please don't disable the VFIO device for the non-book3s case. I added
it (even though it didn't do anything until now) so that libvirt
wouldn't choke when it finds it's not available. Obviously the new
ioctl needs to be only for the right IOMMU setup, but the device
itself should be available always.
Ah. Ok, I'll fix this. I just wanted to be able to compile kvm as a module.
Shouldn't this go in the same patch that introduced the attach
function?
Having less patches which touch different maintainers areas is better. I
cannot avoid touching both PPC KVM and VFIO in this patch but I can in
"[PATCH kernel 6/9] KVM: PPC: Associate IOMMU group with guest view of TCE
table".
@@ -186,6 +222,10 @@ static int kvm_vfio_set_group(struct kvm_device *dev, long attr, u64 arg) continue; list_del(&kvg->node);+#ifdef CONFIG_SPAPR_TCE_IOMMU
Better to make a no-op version of the call than have to #ifdef at the
callsite.
It is questionable. A x86 reader may decide that
KVM_DEV_VFIO_GROUP_SET_SPAPR_TCE_LIOBN is implemented for x86 and get
confused.
quoted
quoted
+ kvm_vfio_spapr_detach_iommu_group(dev->kvm,
+ kvg->vfio_group);
+#endif
kvm_vfio_group_put_external_user(kvg->vfio_group);
kfree(kvg);
ret = 0;
@@ -201,6 +241,69 @@ static int kvm_vfio_set_group(struct kvm_device *dev, long attr, u64 arg) kvm_vfio_update_coherency(dev); return ret;++#ifdef CONFIG_SPAPR_TCE_IOMMU+ case KVM_DEV_VFIO_GROUP_SET_SPAPR_TCE_LIOBN: {+ struct kvm_vfio_spapr_tce_liobn param;+ unsigned long minsz;+ struct kvm_vfio *kv = dev->private;+ struct vfio_group *vfio_group;+ struct kvm_vfio_group *kvg;+ struct fd f;++ minsz = offsetofend(struct kvm_vfio_spapr_tce_liobn,+ start_addr);++ if (copy_from_user(¶m, (void __user *)arg, minsz))+ return -EFAULT;++ if (param.argsz < minsz)+ return -EINVAL;++ f = fdget(param.fd);+ if (!f.file)+ return -EBADF;++ vfio_group = kvm_vfio_group_get_external_user(f.file);+ fdput(f);++ if (IS_ERR(vfio_group))+ return PTR_ERR(vfio_group);++ ret = -ENOENT;
Shouldn't there be some runtime test for the type of the IOMMU? It's
possible a kernel could be built for a platform supporting multiple
IOMMU types.
Well, may make sense but I do not know to test that. The IOMMU type is a
VFIO container property, not a group property and here (KVM) we only have
groups.
Which, as mentioned previously, is broken.
Which I am failing to follow you on this.
What I am trying to achieve here is pretty much referencing a group so it
cannot be reused. Plus LIOBNs.
"Plus LIOBNs" is not a trivial change. You are establishing a linkage
from LIOBNs to groups. But that doesn't make sense; if mapping in one
(guest) LIOBN affects a group it must affect all groups in the
container. i.e. LIOBN->container is the natural mapping, *not* LIOBN
to group.
Passing a container fd does not make much
sense here as the VFIO device would walk through groups, reference them and
that is it, there is no locking on VFIO containters and so far there was no
need to teach KVM about containers.
What do I miss now?
Referencing the groups is essentially just a useful side effect. The
important functionality is informing VFIO of the guest LIOBNs; and
LIOBNs map to containers, not groups.
--
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
From: David Gibson <hidden> Date: 2016-03-15 06:29:24
On Fri, Mar 11, 2016 at 01:15:20PM +1100, Alexey Kardashevskiy wrote:
On 03/10/2016 04:18 PM, David Gibson wrote:
quoted
On Wed, Mar 09, 2016 at 07:46:47PM +1100, Alexey Kardashevskiy wrote:
quoted
On 03/08/2016 10:08 PM, David Gibson wrote:
quoted
On Mon, Mar 07, 2016 at 02:41:16PM +1100, Alexey Kardashevskiy wrote:
quoted
This allows the host kernel to handle H_PUT_TCE, H_PUT_TCE_INDIRECT
and H_STUFF_TCE requests targeted an IOMMU TCE table used for VFIO
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; this is not expected
to happen ever though.
Well... not expect to happen with a qemu which uses this. Presumably
it will fall back to userspace routinely if you have an old qemu that
doesn't add the liobn mappings.
Ah. Ok, thanks, I'll add this to the commit log.
Ok.
quoted
quoted
quoted
The first user of this is VFIO on POWER. Trampolines to the VFIO external
user API functions are required for this patch.
I'm not sure what you mean by "trampoline" here.
For example, look at kvm_vfio_group_get_external_user. It calls
symbol_get(vfio_group_get_external_user) and then calls a function via the
returned pointer.
Is there a better word for this?
Uh.. probably although I don't immediately know what. "Trampoline"
usually refers to code on the stack used for bouncing places, which
isn't what this resembles.
"Dynamic wrapper"?
Sure, that'll do.
quoted
quoted
quoted
quoted
This uses a VFIO KVM device to associate a logical bus number (LIOBN)
with an VFIO IOMMU group fd and enable in-kernel handling of map/unmap
requests.
Group fd? Or container fd? The group fd wouldn't make a lot of
sense.
Group. KVM has no idea about containers.
That's not going to fly. Having a liobn registered against just one
group in a container makes no sense at all. Conceptually, if not
physically, the container shares a single set of TCE tables. If
handling that means teaching KVM the concept of containers, then so be
it.
Btw, I'm not sure yet if extending the existing vfio kvm device to
make the vfio<->kvm linkages makes sense. I think the reason some x86
machines need that is quite different from how we're using it for
Power. I haven't got a clear enough picture yet to be sure either
way.
The other option that would seem likely to me would be a "bind VFIO
container" ioctl() on the fd associated with a kernel accelerated TCE table.
Oh, I just noticed this response. I need to digest it. Looks like this is
going to take other 2 years to upstream...
quoted
quoted
quoted
quoted
To make use of the feature, the user space has to create a guest view
of the TCE table via KVM_CAP_SPAPR_TCE/KVM_CAP_SPAPR_TCE_64 and
then associate a LIOBN with this table via VFIO KVM device,
a KVM_DEV_VFIO_GROUP_SET_SPAPR_TCE_LIOBN property (which is added in
the next patch).
Tests show that this patch increases transmission speed from 220MB/s
to 750..1020MB/s on 10Gb network (Chelsea CXGB3 10Gb ethernet card).
Is that with or without DDW (i.e. with or without a 64-bit DMA window)?
Without DDW, I should have mentioned this. The patch is from the times when
there was no DDW :(
H_HARDWARE? Or H_PARAMETER? This essentially means the guest has
supplied a bad physical address, doesn't it?
Well, may be. I'll change. If it not H_TOO_HARD, it does not make any
difference after all :)
quoted
quoted
+ if (kvmppc_gpa_to_ua(kvm, gpa, &ua, NULL))
+ return H_HARDWARE;
+
+ mem = mm_iommu_lookup(ua, 1ULL << tbl->it_page_shift);
+ if (!mem)
+ return H_HARDWARE;
+
+ if (mm_iommu_ua_to_hpa(mem, ua, &hpa))
+ return H_HARDWARE;
+
+ if (mm_iommu_mapped_inc(mem))
+ return H_HARDWARE;
+
+ ret = iommu_tce_xchg(tbl, entry, &hpa, &dir);
+ if (ret) {
+ mm_iommu_mapped_dec(mem);
+ return H_TOO_HARD;
+ }
+
+ if (dir != DMA_NONE)
+ kvmppc_tce_iommu_mapped_dec(tbl, entry);
+
+ *pua = ua;
IIUC this means you have a copy of the UA for every group attached to
the TCE table, but they'll all be the same. Any way to avoid that
duplication?
It is for every container, not a group. On P8, I allow multiple groups to go
to the same container, that means that a container has one or two
iommu_table, and each iommu_table has this "ua" list but since tables are
different (window size, page size, content), these "ua" arrays are also
different.
Erm.. but h_put_tce iterates h_put_tce_iommu through all the groups
attached to the stt, and each one seems to update pua.
Or is that what the if (kg->tbl == tbltmp) continue; is supposed to
avoid? In which case what ensures that the stt->groups list is
ordered by tbl pointer?
Nothing. In the normal case (POWER8 IODA2) all groups on the same liobn have
the same iommu_table, so the first group's one gets updated, other do not
but it is ok as they use the same table.
Right, which is another indication that group is the wrong concept to
use here.
In a bad case (POWER7 IODA1, multiple containers per LIOBN) the same @ua can
be updated more than once. Well, not a huge loss.
Ugh.. this really seems to be based on knowing the specific cases we
have in practice, rather than writing code that's correct based on ly
on the properties that the objects are defined to have. The latter
approach will make for much more robust and extensible code.
--
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
Please don't disable the VFIO device for the non-book3s case. I added
it (even though it didn't do anything until now) so that libvirt
wouldn't choke when it finds it's not available. Obviously the new
ioctl needs to be only for the right IOMMU setup, but the device
itself should be available always.
After having a closer look, the statement above does not enable VFIO KVM
device on book3s but does for everything else:
common-objs-$(CONFIG_KVM_VFIO) += $(KVM)/vfio.o
[...]
kvm-e500-objs := \
$(common-objs-y) \
[...]
kvm-objs-$(CONFIG_KVM_E500V2) := $(kvm-e500-objs)
[...]
kvm-e500mc-objs := \
$(common-objs-y) \
[...]
kvm-objs-$(CONFIG_KVM_E500MC) := $(kvm-e500mc-objs)
[...]
kvm-book3s_32-objs := \
$(common-objs-y) \
[...]
kvm-objs-$(CONFIG_KVM_BOOK3S_32) := $(kvm-book3s_32-objs)
This is becaise CONFIG_KVM_BOOK3S_64 does not use "common-objs-y":
kvm-objs-$(CONFIG_KVM_BOOK3S_64) := $(kvm-book3s_64-module-objs)
So I will keep vfio.o in the "common-objs-y" list and add:
+kvm-book3s_64-objs-$(CONFIG_KVM_VFIO) += \
+ $(KVM)/vfio.o
--
Alexey
Please don't disable the VFIO device for the non-book3s case. I added
it (even though it didn't do anything until now) so that libvirt
wouldn't choke when it finds it's not available. Obviously the new
ioctl needs to be only for the right IOMMU setup, but the device
itself should be available always.
After having a closer look, the statement above does not enable VFIO KVM
device on book3s but does for everything else:
common-objs-$(CONFIG_KVM_VFIO) += $(KVM)/vfio.o
[...]
kvm-e500-objs := \
$(common-objs-y) \
[...]
kvm-objs-$(CONFIG_KVM_E500V2) := $(kvm-e500-objs)
[...]
kvm-e500mc-objs := \
$(common-objs-y) \
[...]
kvm-objs-$(CONFIG_KVM_E500MC) := $(kvm-e500mc-objs)
[...]
kvm-book3s_32-objs := \
$(common-objs-y) \
[...]
kvm-objs-$(CONFIG_KVM_BOOK3S_32) := $(kvm-book3s_32-objs)
This is becaise CONFIG_KVM_BOOK3S_64 does not use "common-objs-y":
Oh, good grief.
kvm-objs-$(CONFIG_KVM_BOOK3S_64) := $(kvm-book3s_64-module-objs)
So I will keep vfio.o in the "common-objs-y" list and add:
+kvm-book3s_64-objs-$(CONFIG_KVM_VFIO) += \
+ $(KVM)/vfio.o
Ok.
--
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