This is my current queue of patches to add acceleration of TCE
updates in KVM. This has a long history and was rewritten pretty
much completely again, this time I am teaching KVM about VFIO
containers. Some patches (such as 01/15) could be posted
separately but I keep all of them here to make review easier
(if the concept turns out be wrong - then I might still want
to have 01/15).
Please comment. Thanks.
Alexey Kardashevskiy (15):
Revert "iommu: Add a function to find an iommu group by id"
KVM: PPC: Finish enabling VFIO KVM device on POWER
KVM: PPC: Reserve KVM_CAP_SPAPR_TCE_VFIO capability number
powerpc/powernv/ioda: Fix TCE invalidate to work in real mode again
powerpc/iommu: Stop using @current in mm_iommu_xxx
powerpc/mm/iommu: Put pages on process exit
powerpc/iommu: Cleanup iommu_table disposal
powerpc/vfio_spapr_tce: Add reference counting to iommu_table
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
iommu_table_ops::exchange()
KVM: PPC: Enable IOMMU_API for KVM_BOOK3S_64 permanently
KVM: PPC: Pass kvm* to kvmppc_find_table()
vfio/spapr_tce: Export container API for external users
KVM: PPC: Add in-kernel acceleration for VFIO
arch/powerpc/include/asm/iommu.h | 12 +-
arch/powerpc/include/asm/kvm_host.h | 8 +
arch/powerpc/include/asm/kvm_ppc.h | 2 +-
arch/powerpc/include/asm/mmu_context.h | 23 +-
arch/powerpc/include/uapi/asm/kvm.h | 12 +
arch/powerpc/kernel/iommu.c | 49 +++-
arch/powerpc/kernel/setup-common.c | 2 +-
arch/powerpc/kernel/vio.c | 2 +-
arch/powerpc/kvm/Kconfig | 2 +
arch/powerpc/kvm/Makefile | 3 +
arch/powerpc/kvm/book3s_64_vio.c | 410 +++++++++++++++++++++++++++++-
arch/powerpc/kvm/book3s_64_vio_hv.c | 251 ++++++++++++++++--
arch/powerpc/kvm/powerpc.c | 2 +
arch/powerpc/mm/mmu_context_book3s64.c | 6 +-
arch/powerpc/mm/mmu_context_iommu.c | 96 ++++---
arch/powerpc/platforms/powernv/pci-ioda.c | 46 +++-
arch/powerpc/platforms/powernv/pci.c | 1 +
arch/powerpc/platforms/pseries/iommu.c | 3 +-
drivers/iommu/iommu.c | 29 ---
drivers/vfio/vfio.c | 30 +++
drivers/vfio/vfio_iommu_spapr_tce.c | 107 ++++++--
include/linux/iommu.h | 1 -
include/linux/vfio.h | 6 +
include/uapi/linux/kvm.h | 1 +
24 files changed, 959 insertions(+), 145 deletions(-)
--
2.5.0.rc3
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>
Reviewed-by: David Gibson <redacted>
---
include/uapi/linux/kvm.h | 1 +
1 file changed, 1 insertion(+)
In some situations the userspace memory context may live longer than
the userspace process itself so if we need to do proper memory context
cleanup, we better cache @mm and use it later when the process is gone
(@current or @current->mm are NULL).
This changes mm_iommu_xxx API to receive mm_struct instead of using one
from @current.
This is needed by the following patch to do proper cleanup in time.
This depends on "powerpc/powernv/ioda: Fix endianness when reading TCEs"
to do proper cleanup via tce_iommu_clear() patch.
To keep API consistent, this replaces mm_context_t with mm_struct;
we stick to mm_struct as mm_iommu_adjust_locked_vm() helper needs
access to &mm->mmap_sem.
This should cause no behavioral change.
Signed-off-by: Alexey Kardashevskiy <redacted>
---
arch/powerpc/include/asm/mmu_context.h | 20 +++++++------
arch/powerpc/kernel/setup-common.c | 2 +-
arch/powerpc/mm/mmu_context_book3s64.c | 4 +--
arch/powerpc/mm/mmu_context_iommu.c | 54 ++++++++++++++--------------------
drivers/vfio/vfio_iommu_spapr_tce.c | 41 ++++++++++++++++----------
5 files changed, 62 insertions(+), 59 deletions(-)
@@ -63,28 +63,22 @@ static long mm_iommu_adjust_locked_vm(struct mm_struct *mm,returnret;}-boolmm_iommu_preregistered(void)+boolmm_iommu_preregistered(structmm_struct*mm){-if(!current||!current->mm)-returnfalse;--return!list_empty(¤t->mm->context.iommu_group_mem_list);+return!list_empty(&mm->context.iommu_group_mem_list);}EXPORT_SYMBOL_GPL(mm_iommu_preregistered);-longmm_iommu_get(unsignedlongua,unsignedlongentries,+longmm_iommu_get(structmm_struct*mm,unsignedlongua,unsignedlongentries,structmm_iommu_table_group_mem_t**pmem){structmm_iommu_table_group_mem_t*mem;longi,j,ret=0,locked_entries=0;structpage*page=NULL;-if(!current||!current->mm)-return-ESRCH;/* process exited */-mutex_lock(&mem_list_mutex);-list_for_each_entry_rcu(mem,¤t->mm->context.iommu_group_mem_list,+list_for_each_entry_rcu(mem,&mm->context.iommu_group_mem_list,next){if((mem->ua==ua)&&(mem->entries==entries)){++mem->used;
@@ -102,7 +96,7 @@ long mm_iommu_get(unsigned long ua, unsigned long entries,}-ret=mm_iommu_adjust_locked_vm(current->mm,entries,true);+ret=mm_iommu_adjust_locked_vm(mm,entries,true);if(ret)gotounlock_exit;
@@ -142,11 +136,11 @@ long mm_iommu_get(unsigned long ua, unsigned long entries,mem->entries=entries;*pmem=mem;-list_add_rcu(&mem->next,¤t->mm->context.iommu_group_mem_list);+list_add_rcu(&mem->next,&mm->context.iommu_group_mem_list);unlock_exit:if(locked_entries&&ret)-mm_iommu_adjust_locked_vm(current->mm,locked_entries,false);+mm_iommu_adjust_locked_vm(mm,locked_entries,false);mutex_unlock(&mem_list_mutex);
@@ -224,6 +215,8 @@ long mm_iommu_put(struct mm_iommu_table_group_mem_t *mem)/* @mapped became 0 so now mappings are disabled, release the region */mm_iommu_release(mem);+mm_iommu_adjust_locked_vm(mm,mem->entries,false);+unlock_exit:mutex_unlock(&mem_list_mutex);
@@ -388,18 +399,18 @@ static int tce_iommu_prereg_ua_to_hpa(unsigned long tce, unsigned long size,return0;}-staticvoidtce_iommu_unuse_page_v2(structiommu_table*tbl,-unsignedlongentry)+staticvoidtce_iommu_unuse_page_v2(structtce_container*container,+structiommu_table*tbl,unsignedlongentry){structmm_iommu_table_group_mem_t*mem=NULL;intret;unsignedlonghpa=0;unsignedlong*pua=IOMMU_TABLE_USERSPACE_ENTRY(tbl,entry);-if(!pua||!current||!current->mm)+if(!pua)return;-ret=tce_iommu_prereg_ua_to_hpa(*pua,IOMMU_PAGE_SIZE(tbl),+ret=tce_iommu_prereg_ua_to_hpa(container,*pua,IOMMU_PAGE_SIZE(tbl),&hpa,&mem);if(ret)pr_debug("%s: tce %lx at #%lx was not cached, ret=%d\n",
@@ -429,7 +440,7 @@ static int tce_iommu_clear(struct tce_container *container,continue;if(container->v2){-tce_iommu_unuse_page_v2(tbl,entry);+tce_iommu_unuse_page_v2(container,tbl,entry);continue;}
@@ -514,8 +525,8 @@ static long tce_iommu_build_v2(struct tce_container *container,unsignedlong*pua=IOMMU_TABLE_USERSPACE_ENTRY(tbl,entry+i);-ret=tce_iommu_prereg_ua_to_hpa(tce,IOMMU_PAGE_SIZE(tbl),-&hpa,&mem);+ret=tce_iommu_prereg_ua_to_hpa(container,+tce,IOMMU_PAGE_SIZE(tbl),&hpa,&mem);if(ret)break;
@@ -536,7 +547,7 @@ static long tce_iommu_build_v2(struct tce_container *container,ret=iommu_tce_xchg(tbl,entry+i,&hpa,&dirtmp);if(ret){/* dirtmp cannot be DMA_NONE here */-tce_iommu_unuse_page_v2(tbl,entry+i);+tce_iommu_unuse_page_v2(container,tbl,entry+i);pr_err("iommu_tce: %s failed ioba=%lx, tce=%lx, ret=%ld\n",__func__,entry<<tbl->it_page_shift,tce,ret);
@@ -544,7 +555,7 @@ static long tce_iommu_build_v2(struct tce_container *container,}if(dirtmp!=DMA_NONE)-tce_iommu_unuse_page_v2(tbl,entry+i);+tce_iommu_unuse_page_v2(container,tbl,entry+i);*pua=tce;
At the moment VFIO IOMMU SPAPR v2 driver pins all guest RAM pages when
the userspace starts using VFIO. When the userspace process finishes,
all the pinned pages need to be put; this is done as a part of
the userspace memory context (MM) destruction which happens on
the very last mmdrop().
This approach has a problem that a MM of the userspace process
may live longer than the userspace process itself as kernel threads
use userspace process MMs which was runnning on a CPU where
the kernel thread was scheduled to. If this happened, the MM remains
referenced until this exact kernel thread wakes up again
and releases the very last reference to the MM, on an idle system this
can take even hours.
This references and caches MM once per container and adds tracking
how many times each preregistered area was registered in
a specific container. This way we do not depend on @current pointing to
a valid task descriptor.
This changes the userspace interface to return EBUSY if memory is
already registered (mm_iommu_get() used to increment the counter);
however it should not have any practical effect as the only
userspace tool available now does register memory area once per
container anyway.
As tce_iommu_register_pages/tce_iommu_unregister_pages are called
under container->lock, this does not need additional locking.
Signed-off-by: Alexey Kardashevskiy <redacted>
# Conflicts:
# arch/powerpc/include/asm/mmu_context.h
# arch/powerpc/mm/mmu_context_book3s64.c
# arch/powerpc/mm/mmu_context_iommu.c
---
arch/powerpc/include/asm/mmu_context.h | 1 -
arch/powerpc/mm/mmu_context_book3s64.c | 4 ---
arch/powerpc/mm/mmu_context_iommu.c | 11 -------
drivers/vfio/vfio_iommu_spapr_tce.c | 52 +++++++++++++++++++++++++++++++++-
4 files changed, 51 insertions(+), 17 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 as we know that all of guest memory is pinned and
we have a flat array mapping GPA to HPA which makes it 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. Note that the rmap pointer is initialized to NULL where declared
(not in this patch).
Signed-off-by: Alexey Kardashevskiy <redacted>
---
Changes:
v2:
* updated the commit log with Paul's comment
---
arch/powerpc/kvm/book3s_64_vio_hv.c | 65 ++++++++++++++++++++++++++++---------
1 file changed, 49 insertions(+), 16 deletions(-)
In real mode, TCE tables are invalidated using special
cache-inhibited store instructions which are not available in
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 | 23 +++++++++++++++++++++++
arch/powerpc/platforms/powernv/pci-ioda.c | 26 +++++++++++++++++++++++++-
3 files changed, 55 insertions(+), 1 deletion(-)
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(+)
This reverts commit aa16bea929ae
("iommu: Add a function to find an iommu group by id")
as the iommu_group_get_by_id() helper has never been used
and it is unlikely it will in foreseeable future. Dead code
is broken code.
Signed-off-by: Alexey Kardashevskiy <redacted>
---
drivers/iommu/iommu.c | 29 -----------------------------
include/linux/iommu.h | 1 -
2 files changed, 30 deletions(-)
The guest view TCE tables are per KVM anyway (not per VCPU) so pass kvm*
there. This will be used in the following patches where we will be
attaching VFIO containers to LIOBNs via ioctl() to KVM (rather than
to VCPU).
Signed-off-by: Alexey Kardashevskiy <redacted>
---
arch/powerpc/include/asm/kvm_ppc.h | 2 +-
arch/powerpc/kvm/book3s_64_vio.c | 7 ++++---
arch/powerpc/kvm/book3s_64_vio_hv.c | 13 +++++++------
3 files changed, 12 insertions(+), 10 deletions(-)
178a787502 "vfio: Enable VFIO device for powerpc" made an attempt to
enable VFIO KVM device on POWER.
However as CONFIG_KVM_BOOK3S_64 does not use "common-objs-y",
VFIO KVM device was not enabled for Book3s KVM, this adds VFIO to
the kvm-book3s_64-objs-y list.
While we are here, enforce KVM_VFIO on KVM_BOOK3S as other platforms
already do.
Signed-off-by: Alexey Kardashevskiy <redacted>
---
arch/powerpc/kvm/Kconfig | 1 +
arch/powerpc/kvm/Makefile | 3 +++
2 files changed, 4 insertions(+)
This exports helpers which are needed to keep a VFIO container in
memory while there are external users such as KVM.
Signed-off-by: Alexey Kardashevskiy <redacted>
---
drivers/vfio/vfio.c | 30 ++++++++++++++++++++++++++++++
drivers/vfio/vfio_iommu_spapr_tce.c | 16 +++++++++++++++-
include/linux/vfio.h | 6 ++++++
3 files changed, 51 insertions(+), 1 deletion(-)
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 adds ioctl() interface to SPAPR TCE fd which already handles
in-kernel acceleration for emulated IO by allocating the guest view of
the TCE table in KVM. New ioctls allows the userspace to attach/detach
VFIO containers to the kernel-allocated TCE table and handle
the hardware TCE table updates in the kernel. The new interface
accepts VFIO container fd and uses exported API to get to the actual
hardware TCE table. Until _unset() ioctl is called, the VFIO container
is referenced to guarantee the TCE table presense in the memory.
This also releases unused containers when new container is registered.
The criteria of "unused" is vfio_container_get_iommu_data_ext()
returning NULL which happens when the container fd is closed.
Note that this interface does not operate with IOMMU groups as
TCE tables are owned by VFIO containers (and even may have no IOMMU groups
attached).
This advertises the new KVM_CAP_SPAPR_TCE_VFIO capability to the user
space.
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/include/asm/kvm_host.h | 8 +
arch/powerpc/include/uapi/asm/kvm.h | 12 ++
arch/powerpc/kvm/book3s_64_vio.c | 403 ++++++++++++++++++++++++++++++++++++
arch/powerpc/kvm/book3s_64_vio_hv.c | 173 ++++++++++++++++
arch/powerpc/kvm/powerpc.c | 2 +
5 files changed, 598 insertions(+)
At the moment iommu_table could be disposed by either calling
iommu_table_free() directly or it_ops::free() which only implementation
for IODA2 calls iommu_table_free() anyway.
As we are going to have reference counting on tables, we need an unified
way of disposing tables.
This moves it_ops::free() call into iommu_free_table() and makes use
of the latter everywhere. The free() callback now handles only
platform-specific data.
This should cause no behavioral change.
Signed-off-by: Alexey Kardashevskiy <redacted>
---
arch/powerpc/kernel/iommu.c | 4 ++++
arch/powerpc/platforms/powernv/pci-ioda.c | 6 ++----
drivers/vfio/vfio_iommu_spapr_tce.c | 2 +-
3 files changed, 7 insertions(+), 5 deletions(-)
@@ -744,6 +747,7 @@ void iommu_free_table(struct iommu_table *tbl, const char *node_name)/* free table */kfree(tbl);}+EXPORT_SYMBOL_GPL(iommu_free_table);/* Creates TCEs for a user provided buffer. The user buffer must be*contiguousrealkernelstorage(notvmalloc).Theaddresspassedhere
So far iommu_table obejcts were only used in virtual mode and had
a single owner. We are going to change by implementing in-kernel
acceleration of DMA mapping requests, including real mode.
This adds a kref to iommu_table and defines new helpers to update it.
This replaces iommu_free_table() with iommu_table_put() and makes
iommu_free_table() static. iommu_table_get() is not used in this patch
but will be in the following one.
While we are here, this removes @node_name parameter as it has never been
really useful on powernv and carrying it for the pseries platform code to
iommu_free_table() seems to be quite useless too.
This should cause no behavioral change.
Signed-off-by: Alexey Kardashevskiy <redacted>
---
arch/powerpc/include/asm/iommu.h | 5 +++--
arch/powerpc/kernel/iommu.c | 24 +++++++++++++++++++-----
arch/powerpc/kernel/vio.c | 2 +-
arch/powerpc/platforms/powernv/pci-ioda.c | 14 +++++++-------
arch/powerpc/platforms/powernv/pci.c | 1 +
arch/powerpc/platforms/pseries/iommu.c | 3 ++-
drivers/vfio/vfio_iommu_spapr_tce.c | 2 +-
7 files changed, 34 insertions(+), 17 deletions(-)
@@ -114,6 +114,7 @@ struct iommu_table {structlist_headit_group_list;/* List of iommu_table_group_link */unsignedlong*it_userspace;/* userspace view of the table */structiommu_table_ops*it_ops;+structkrefit_kref;};#define IOMMU_TABLE_USERSPACE_ENTRY(tbl, entry) \
@@ -146,8 +147,8 @@ static inline void *get_iommu_table_base(struct device *dev)externintdma_iommu_dma_supported(structdevice*dev,u64mask);-/* Frees table for an individual device node */-externvoidiommu_free_table(structiommu_table*tbl,constchar*node_name);+externvoidiommu_table_get(structiommu_table*tbl);+externvoidiommu_table_put(structiommu_table*tbl);/* Initializes an iommu_table based in values set in the passed-in*structure
@@ -735,7 +735,7 @@ void iommu_free_table(struct iommu_table *tbl, const char *node_name)/* verify that table contains no entries */if(!bitmap_empty(tbl->it_map,tbl->it_size))-pr_warn("%s: Unexpected TCEs for %s\n",__func__,node_name);+pr_warn("%s: Unexpected TCEs\n",__func__);/* calculate bitmap size in bytes */bitmap_sz=BITS_TO_LONGS(tbl->it_size)*sizeof(unsignedlong);
@@ -747,7 +747,21 @@ void iommu_free_table(struct iommu_table *tbl, const char *node_name)/* free table */kfree(tbl);}-EXPORT_SYMBOL_GPL(iommu_free_table);++voidiommu_table_get(structiommu_table*tbl)+{+kref_get(&tbl->it_kref);+}+EXPORT_SYMBOL_GPL(iommu_table_get);++voidiommu_table_put(structiommu_table*tbl)+{+if(!tbl)+return;++kref_put(&tbl->it_kref,iommu_table_free);+}+EXPORT_SYMBOL_GPL(iommu_table_put);/* Creates TCEs for a user provided buffer. The user buffer must be*contiguousrealkernelstorage(notvmalloc).Theaddresspassedhere
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_ua_to_hpa_rm() 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 | 4 ++++
arch/powerpc/mm/mmu_context_iommu.c | 39 ++++++++++++++++++++++++++++++++++
2 files changed, 43 insertions(+)
From: Nicholas Piggin <npiggin@gmail.com> Date: 2016-08-03 10:10:14
On Wed, 3 Aug 2016 18:40:46 +1000
Alexey Kardashevskiy [off-list ref] wrote:
In some situations the userspace memory context may live longer than
the userspace process itself so if we need to do proper memory context
cleanup, we better cache @mm and use it later when the process is gone
(@current or @current->mm are NULL).
This changes mm_iommu_xxx API to receive mm_struct instead of using one
from @current.
This is needed by the following patch to do proper cleanup in time.
This depends on "powerpc/powernv/ioda: Fix endianness when reading TCEs"
to do proper cleanup via tce_iommu_clear() patch.
To keep API consistent, this replaces mm_context_t with mm_struct;
we stick to mm_struct as mm_iommu_adjust_locked_vm() helper needs
access to &mm->mmap_sem.
This should cause no behavioral change.
Signed-off-by: Alexey Kardashevskiy <redacted>
Reviewed-by: Nicholas Piggin <npiggin@gmail.com>
I still have some questions about the use of mm in the driver, but
those aren't issues introduced by this patch, so as it is I think
the bug fix of this and the next patch is good.
From: Nicholas Piggin <npiggin@gmail.com> Date: 2016-08-03 10:11:12
On Wed, 3 Aug 2016 18:40:47 +1000
Alexey Kardashevskiy [off-list ref] wrote:
At the moment VFIO IOMMU SPAPR v2 driver pins all guest RAM pages when
the userspace starts using VFIO. When the userspace process finishes,
all the pinned pages need to be put; this is done as a part of
the userspace memory context (MM) destruction which happens on
the very last mmdrop().
This approach has a problem that a MM of the userspace process
may live longer than the userspace process itself as kernel threads
use userspace process MMs which was runnning on a CPU where
the kernel thread was scheduled to. If this happened, the MM remains
referenced until this exact kernel thread wakes up again
and releases the very last reference to the MM, on an idle system this
can take even hours.
This references and caches MM once per container and adds tracking
how many times each preregistered area was registered in
a specific container. This way we do not depend on @current pointing to
a valid task descriptor.
This changes the userspace interface to return EBUSY if memory is
already registered (mm_iommu_get() used to increment the counter);
however it should not have any practical effect as the only
userspace tool available now does register memory area once per
container anyway.
As tce_iommu_register_pages/tce_iommu_unregister_pages are called
under container->lock, this does not need additional locking.
Signed-off-by: Alexey Kardashevskiy <redacted>
From: David Gibson <hidden> Date: 2016-08-04 06:01:38
On Wed, Aug 03, 2016 at 06:40:43PM +1000, Alexey Kardashevskiy wrote:
178a787502 "vfio: Enable VFIO device for powerpc" made an attempt to
enable VFIO KVM device on POWER.
However as CONFIG_KVM_BOOK3S_64 does not use "common-objs-y",
VFIO KVM device was not enabled for Book3s KVM, this adds VFIO to
the kvm-book3s_64-objs-y list.
While we are here, enforce KVM_VFIO on KVM_BOOK3S as other platforms
already do.
Signed-off-by: Alexey Kardashevskiy <redacted>
Reviewed-by: David Gibson <redacted>
This should be merged regardless of the rest of the series. There's
no reason not to include the kvm device on Power, and it makes life
easier for userspace because it doens't have to have conditionals
about whether to instantiate it or not.
--
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
--
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: Michael Ellerman <mpe@ellerman.id.au> Date: 2016-08-05 07:00:34
Alexey Kardashevskiy [off-list ref] writes:
In some situations the userspace memory context may live longer than
the userspace process itself so if we need to do proper memory context
cleanup, we better cache @mm and use it later when the process is gone
(@current or @current->mm are NULL).
This changes mm_iommu_xxx API to receive mm_struct instead of using one
from @current.
This is needed by the following patch to do proper cleanup in time.
This depends on "powerpc/powernv/ioda: Fix endianness when reading TCEs"
to do proper cleanup via tce_iommu_clear() patch.
To keep API consistent, this replaces mm_context_t with mm_struct;
we stick to mm_struct as mm_iommu_adjust_locked_vm() helper needs
access to &mm->mmap_sem.
This should cause no behavioral change.
Is this a theoretical bug, or do we hit it in practice?
In other words, should I merge this as a fix for 4.8, or can it wait for
4.9 with the rest of the series?
From: Alex Williamson <hidden> Date: 2016-08-08 16:43:18
On Wed, 3 Aug 2016 18:40:55 +1000
Alexey Kardashevskiy [off-list ref] wrote:
quoted hunk
This exports helpers which are needed to keep a VFIO container in
memory while there are external users such as KVM.
Signed-off-by: Alexey Kardashevskiy <redacted>
---
drivers/vfio/vfio.c | 30 ++++++++++++++++++++++++++++++
drivers/vfio/vfio_iommu_spapr_tce.c | 16 +++++++++++++++-
include/linux/vfio.h | 6 ++++++
3 files changed, 51 insertions(+), 1 deletion(-)
I think you need to take a closer look of the lifecycle of a container,
having a reference means the container itself won't go away, but only
having a group set within that container holds the actual IOMMU
references. container->iommu_data is going to be NULL once the
groups are lost. Thanks,
Alex
In some situations the userspace memory context may live longer than
the userspace process itself so if we need to do proper memory context
cleanup, we better cache @mm and use it later when the process is gone
(@current or @current->mm are NULL).
This changes mm_iommu_xxx API to receive mm_struct instead of using one
from @current.
This is needed by the following patch to do proper cleanup in time.
This depends on "powerpc/powernv/ioda: Fix endianness when reading TCEs"
to do proper cleanup via tce_iommu_clear() patch.
To keep API consistent, this replaces mm_context_t with mm_struct;
we stick to mm_struct as mm_iommu_adjust_locked_vm() helper needs
access to &mm->mmap_sem.
This should cause no behavioral change.
Looks good, minor nits below
Acked-by: Balbir Singh <bsingharora@gmail.com>
@@ -102,7 +96,7 @@ long mm_iommu_get(unsigned long ua, unsigned long entries, }- ret = mm_iommu_adjust_locked_vm(current->mm, entries, true);+ ret = mm_iommu_adjust_locked_vm(mm, entries, true); if (ret) goto unlock_exit;
@@ -142,11 +136,11 @@ long mm_iommu_get(unsigned long ua, unsigned long entries, mem->entries = entries; *pmem = mem;- list_add_rcu(&mem->next, ¤t->mm->context.iommu_group_mem_list);+ list_add_rcu(&mem->next, &mm->context.iommu_group_mem_list); unlock_exit: if (locked_entries && ret)- mm_iommu_adjust_locked_vm(current->mm, locked_entries, false);+ mm_iommu_adjust_locked_vm(mm, locked_entries, false); mutex_unlock(&mem_list_mutex);
@@ -224,6 +215,8 @@ long mm_iommu_put(struct mm_iommu_table_group_mem_t *mem) /* @mapped became 0 so now mappings are disabled, release the region */ mm_iommu_release(mem);+ mm_iommu_adjust_locked_vm(mm, mem->entries, false);+ unlock_exit: mutex_unlock(&mem_list_mutex);
@@ -369,13 +379,14 @@ static void tce_iommu_unuse_page(struct tce_container *container, put_page(page); }-static int tce_iommu_prereg_ua_to_hpa(unsigned long tce, unsigned long size,+static int tce_iommu_prereg_ua_to_hpa(struct tce_container *container,+ unsigned long tce, unsigned long size, unsigned long *phpa, struct mm_iommu_table_group_mem_t **pmem) { long ret = 0; struct mm_iommu_table_group_mem_t *mem;- mem = mm_iommu_lookup(tce, size);+ mem = mm_iommu_lookup(container->mm, tce, size); if (!mem) return -EINVAL;
@@ -388,18 +399,18 @@ static int tce_iommu_prereg_ua_to_hpa(unsigned long tce, unsigned long size, return 0; }-static void tce_iommu_unuse_page_v2(struct iommu_table *tbl,- unsigned long entry)+static void tce_iommu_unuse_page_v2(struct tce_container *container,+ struct iommu_table *tbl, unsigned long entry) { struct mm_iommu_table_group_mem_t *mem = NULL; int ret; unsigned long hpa = 0; unsigned long *pua = IOMMU_TABLE_USERSPACE_ENTRY(tbl, entry);- if (!pua || !current || !current->mm)+ if (!pua) return;- ret = tce_iommu_prereg_ua_to_hpa(*pua, IOMMU_PAGE_SIZE(tbl),+ ret = tce_iommu_prereg_ua_to_hpa(container, *pua, IOMMU_PAGE_SIZE(tbl), &hpa, &mem); if (ret) pr_debug("%s: tce %lx at #%lx was not cached, ret=%d\n",
@@ -429,7 +440,7 @@ static int tce_iommu_clear(struct tce_container *container, continue; if (container->v2) {- tce_iommu_unuse_page_v2(tbl, entry);+ tce_iommu_unuse_page_v2(container, tbl, entry); continue; }
@@ -514,8 +525,8 @@ static long tce_iommu_build_v2(struct tce_container *container, unsigned long *pua = IOMMU_TABLE_USERSPACE_ENTRY(tbl, entry + i);- ret = tce_iommu_prereg_ua_to_hpa(tce, IOMMU_PAGE_SIZE(tbl),- &hpa, &mem);+ ret = tce_iommu_prereg_ua_to_hpa(container,+ tce, IOMMU_PAGE_SIZE(tbl), &hpa, &mem); if (ret) break;
@@ -536,7 +547,7 @@ static long tce_iommu_build_v2(struct tce_container *container, ret = iommu_tce_xchg(tbl, entry + i, &hpa, &dirtmp); if (ret) { /* dirtmp cannot be DMA_NONE here */- tce_iommu_unuse_page_v2(tbl, entry + i);+ tce_iommu_unuse_page_v2(container, tbl, entry + i); pr_err("iommu_tce: %s failed ioba=%lx, tce=%lx, ret=%ld\n", __func__, entry << tbl->it_page_shift, tce, ret);
On Wed, 3 Aug 2016 18:40:55 +1000
Alexey Kardashevskiy [off-list ref] wrote:
quoted
This exports helpers which are needed to keep a VFIO container in
memory while there are external users such as KVM.
Signed-off-by: Alexey Kardashevskiy <redacted>
---
drivers/vfio/vfio.c | 30 ++++++++++++++++++++++++++++++
drivers/vfio/vfio_iommu_spapr_tce.c | 16 +++++++++++++++-
include/linux/vfio.h | 6 ++++++
3 files changed, 51 insertions(+), 1 deletion(-)
I think you need to take a closer look of the lifecycle of a container,
having a reference means the container itself won't go away, but only
having a group set within that container holds the actual IOMMU
references. container->iommu_data is going to be NULL once the
groups are lost. Thanks,
Container owns the iommu tables and this is what I care about here, groups
attached or not - this is handled separately via IOMMU group list in a
specific iommu_table struct, these groups get detached from iommu_table
when they are removed from a container.
--
Alexey
In some situations the userspace memory context may live longer than
the userspace process itself so if we need to do proper memory context
cleanup, we better cache @mm and use it later when the process is gone
(@current or @current->mm are NULL).
This changes mm_iommu_xxx API to receive mm_struct instead of using one
from @current.
This is needed by the following patch to do proper cleanup in time.
This depends on "powerpc/powernv/ioda: Fix endianness when reading TCEs"
to do proper cleanup via tce_iommu_clear() patch.
To keep API consistent, this replaces mm_context_t with mm_struct;
we stick to mm_struct as mm_iommu_adjust_locked_vm() helper needs
access to &mm->mmap_sem.
This should cause no behavioral change.
Is this a theoretical bug, or do we hit it in practice?
Actual bug.
In other words, should I merge this as a fix for 4.8, or can it wait for
4.9 with the rest of the series?
Assuming this does not have "rb" or "ab" from anyone familiar with IOMMU on
powernv, this has to wait :-/
From: Nicholas Piggin <hidden> Date: 2016-08-09 06:04:58
On Tue, 9 Aug 2016 14:43:00 +1000
Balbir Singh [off-list ref] wrote:
On 03/08/16 18:40, Alexey Kardashevskiy wrote:
quoted
-long mm_iommu_get(unsigned long ua, unsigned long entries,
+long mm_iommu_get(struct mm_struct *mm, unsigned long ua, unsigned long entries,
struct mm_iommu_table_group_mem_t **pmem)
{
struct mm_iommu_table_group_mem_t *mem;
long i, j, ret = 0, locked_entries = 0;
struct page *page = NULL;
- if (!current || !current->mm)
- return -ESRCH; /* process exited */
VM_BUG_ON(mm == NULL)?
quoted
@@ -128,10 +129,17 @@ static long tce_iommu_register_pages(struct tce_container *container, ((vaddr + size) < vaddr)) return -EINVAL;- ret = mm_iommu_get(vaddr, entries, &mem);+ if (!container->mm) {+ if (!current->mm)+ return -ESRCH; /* process exited */
You may even want to check for PF_EXITING and ignore those tasks?
These are related to some of the questions I had about the patch.
But I think it makes sense just to take this approach as a minimal
bug fix without changing logic too much or adding BUG_ONs, and then
if we we can consider how iommu takes references to mm and uses it
(if anybody finds the time).
Thanks,
Nick
On Tue, 9 Aug 2016 14:43:00 +1000
Balbir Singh [off-list ref] wrote:
quoted
On 03/08/16 18:40, Alexey Kardashevskiy wrote:
quoted
quoted
-long mm_iommu_get(unsigned long ua, unsigned long entries,
+long mm_iommu_get(struct mm_struct *mm, unsigned long ua, unsigned long entries,
struct mm_iommu_table_group_mem_t **pmem)
{
struct mm_iommu_table_group_mem_t *mem;
long i, j, ret = 0, locked_entries = 0;
struct page *page = NULL;
- if (!current || !current->mm)
- return -ESRCH; /* process exited */
VM_BUG_ON(mm == NULL)?
quoted
quoted
@@ -128,10 +129,17 @@ static long tce_iommu_register_pages(struct tce_container *container, ((vaddr + size) < vaddr)) return -EINVAL;- ret = mm_iommu_get(vaddr, entries, &mem);+ if (!container->mm) {+ if (!current->mm)+ return -ESRCH; /* process exited */
You may even want to check for PF_EXITING and ignore those tasks?
These are related to some of the questions I had about the patch.
But I think it makes sense just to take this approach as a minimal
bug fix without changing logic too much or adding BUG_ONs, and then
if we we can consider how iommu takes references to mm and uses it
(if anybody finds the time).
From: Michael Ellerman <hidden> Date: 2016-08-09 11:26:40
On Wed, 2016-03-08 at 08:40:45 UTC, Alexey Kardashevskiy wrote:
"powerpc/powernv/pci: Rework accessing the TCE invalidate register"
broke TCE invalidation on IODA2/PHB3 for real mode.
This makes invalidate work again.
Fixes: fd141d1a99a3
Signed-off-by: Alexey Kardashevskiy <redacted>
From: Alex Williamson <hidden> Date: 2016-08-09 12:16:33
On Tue, 9 Aug 2016 15:19:39 +1000
Alexey Kardashevskiy [off-list ref] wrote:
On 09/08/16 02:43, Alex Williamson wrote:
quoted
On Wed, 3 Aug 2016 18:40:55 +1000
Alexey Kardashevskiy [off-list ref] wrote:
quoted
This exports helpers which are needed to keep a VFIO container in
memory while there are external users such as KVM.
Signed-off-by: Alexey Kardashevskiy <redacted>
---
drivers/vfio/vfio.c | 30 ++++++++++++++++++++++++++++++
drivers/vfio/vfio_iommu_spapr_tce.c | 16 +++++++++++++++-
include/linux/vfio.h | 6 ++++++
3 files changed, 51 insertions(+), 1 deletion(-)
I think you need to take a closer look of the lifecycle of a container,
having a reference means the container itself won't go away, but only
having a group set within that container holds the actual IOMMU
references. container->iommu_data is going to be NULL once the
groups are lost. Thanks,
Container owns the iommu tables and this is what I care about here, groups
attached or not - this is handled separately via IOMMU group list in a
specific iommu_table struct, these groups get detached from iommu_table
when they are removed from a container.
The container doesn't own anything, the container is privileged by the
groups being attached to it. When groups are closed, they detach from
the container and once the container group list is empty the iommu
backend is released and iommu_data is NULL. A container reference
doesn't give you what you're looking for. It implies nothing about the
iommu backend.
On Tue, 9 Aug 2016 15:19:39 +1000
Alexey Kardashevskiy [off-list ref] wrote:
quoted
On 09/08/16 02:43, Alex Williamson wrote:
quoted
On Wed, 3 Aug 2016 18:40:55 +1000
Alexey Kardashevskiy [off-list ref] wrote:
quoted
This exports helpers which are needed to keep a VFIO container in
memory while there are external users such as KVM.
Signed-off-by: Alexey Kardashevskiy <redacted>
---
drivers/vfio/vfio.c | 30 ++++++++++++++++++++++++++++++
drivers/vfio/vfio_iommu_spapr_tce.c | 16 +++++++++++++++-
include/linux/vfio.h | 6 ++++++
3 files changed, 51 insertions(+), 1 deletion(-)
I think you need to take a closer look of the lifecycle of a container,
having a reference means the container itself won't go away, but only
having a group set within that container holds the actual IOMMU
references. container->iommu_data is going to be NULL once the
groups are lost. Thanks,
Container owns the iommu tables and this is what I care about here, groups
attached or not - this is handled separately via IOMMU group list in a
specific iommu_table struct, these groups get detached from iommu_table
when they are removed from a container.
The container doesn't own anything, the container is privileged by the
groups being attached to it. When groups are closed, they detach from
the container and once the container group list is empty the iommu
backend is released and iommu_data is NULL. A container reference
doesn't give you what you're looking for. It implies nothing about the
iommu backend.
Well. Backend is a part of a container and since a backend owns tables, a
container owns them too.
The problem I am trying to solve here is when KVM may release the
iommu_table objects.
"Set" ioctl() to KVM-spapr-tce-table (or KVM itself, does not really
matter) makes a link between KVM-spapr-tce-table and container and KVM can
start using tables (with referencing them).
First I tried adding an "unset" ioctl to KVM-spapr-tce-table, called it
from region_del() and this works if QEMU removes a window. However if QEMU
removes a vfio-pci device, region_del() is not called and KVM does not get
notified that it can release the iommu_table's because the
KVM-spapr-tce-table remains alive and does not get destroyed (as it is
still used by emulated devices or other containers).
So it was suggested that we could do such "unset" somehow later assuming,
for example, on every "set" I could check if some of currently attached
containers are no more used - and this is where being able to know if there
is no backend helps - KVM remembers a container pointer and can check this
via vfio_container_get_iommu_data_ext().
The other option would be changing vfio_container_get_ext() to take a
callback+opaque which container would call when it destroys iommu_data.
This looks more intrusive and not very intuitive how to make it right -
container would have to keep track of all registered external users and
vfio_container_put_ext() would have to pass the same callback+opaque to
unregister the exact external user.
Or I could store container file* in KVM. Then iommu_data would never be
released until KVM-spapr-tce-table is destroyed.
Recreating KVM-spapr-tce-table on every vfio-pci hotunplug (closing its fd
would "unset" container from KVM-spapr-tce-table) is not an option as there
still may be devices using this KVM-spapr-tce-table.
What obvious and nice solution am I missing here? Thanks.
--
Alexey
From: Alex Williamson <hidden> Date: 2016-08-10 16:46:49
On Wed, 10 Aug 2016 15:37:17 +1000
Alexey Kardashevskiy [off-list ref] wrote:
On 09/08/16 22:16, Alex Williamson wrote:
quoted
On Tue, 9 Aug 2016 15:19:39 +1000
Alexey Kardashevskiy [off-list ref] wrote:
quoted
On 09/08/16 02:43, Alex Williamson wrote:
quoted
On Wed, 3 Aug 2016 18:40:55 +1000
Alexey Kardashevskiy [off-list ref] wrote:
quoted
This exports helpers which are needed to keep a VFIO container in
memory while there are external users such as KVM.
Signed-off-by: Alexey Kardashevskiy <redacted>
---
drivers/vfio/vfio.c | 30 ++++++++++++++++++++++++++++++
drivers/vfio/vfio_iommu_spapr_tce.c | 16 +++++++++++++++-
include/linux/vfio.h | 6 ++++++
3 files changed, 51 insertions(+), 1 deletion(-)
I think you need to take a closer look of the lifecycle of a container,
having a reference means the container itself won't go away, but only
having a group set within that container holds the actual IOMMU
references. container->iommu_data is going to be NULL once the
groups are lost. Thanks,
Container owns the iommu tables and this is what I care about here, groups
attached or not - this is handled separately via IOMMU group list in a
specific iommu_table struct, these groups get detached from iommu_table
when they are removed from a container.
The container doesn't own anything, the container is privileged by the
groups being attached to it. When groups are closed, they detach from
the container and once the container group list is empty the iommu
backend is released and iommu_data is NULL. A container reference
doesn't give you what you're looking for. It implies nothing about the
iommu backend.
Well. Backend is a part of a container and since a backend owns tables, a
container owns them too.
The IOMMU backend is accessed through the container, but that backend
is privileged by the groups it contains. Once those groups are gone,
the IOMMU backend is released, regardless of whatever reference you
have to the container itself such as you're attempting to do here. In
that sense, the container does not own those tables.
The problem I am trying to solve here is when KVM may release the
iommu_table objects.
"Set" ioctl() to KVM-spapr-tce-table (or KVM itself, does not really
matter) makes a link between KVM-spapr-tce-table and container and KVM can
start using tables (with referencing them).
First I tried adding an "unset" ioctl to KVM-spapr-tce-table, called it
from region_del() and this works if QEMU removes a window. However if QEMU
removes a vfio-pci device, region_del() is not called and KVM does not get
notified that it can release the iommu_table's because the
KVM-spapr-tce-table remains alive and does not get destroyed (as it is
still used by emulated devices or other containers).
So it was suggested that we could do such "unset" somehow later assuming,
for example, on every "set" I could check if some of currently attached
containers are no more used - and this is where being able to know if there
is no backend helps - KVM remembers a container pointer and can check this
via vfio_container_get_iommu_data_ext().
The other option would be changing vfio_container_get_ext() to take a
callback+opaque which container would call when it destroys iommu_data.
This looks more intrusive and not very intuitive how to make it right -
container would have to keep track of all registered external users and
vfio_container_put_ext() would have to pass the same callback+opaque to
unregister the exact external user.
I'm not in favor of anything resembling the code above or extensions
beyond it, the container is the wrong place to do this.
Or I could store container file* in KVM. Then iommu_data would never be
released until KVM-spapr-tce-table is destroyed.
See above, holding a file pointer to the container doesn't do squat.
The groups that are held by the container empower the IOMMU backend,
references to the container itself don't matter. Those references will
not maintain the IOMMU data.
Recreating KVM-spapr-tce-table on every vfio-pci hotunplug (closing its fd
would "unset" container from KVM-spapr-tce-table) is not an option as there
still may be devices using this KVM-spapr-tce-table.
What obvious and nice solution am I missing here? Thanks.
The interactions with the IOMMU backend that seem relevant are
vfio_iommu_drivers_ops.{detach_group,release}. The kvm-vfio pseudo
device is also used to tell kvm about groups as they come and go and
has a way to check extensions, and thus properties of the IOMMU
backend. All of these are available for your {ab}use. Thanks,
Alex
From: David Gibson <hidden> Date: 2016-08-12 03:17:00
On Wed, Aug 03, 2016 at 06:40:46PM +1000, Alexey Kardashevskiy wrote:
quoted hunk
In some situations the userspace memory context may live longer than
the userspace process itself so if we need to do proper memory context
cleanup, we better cache @mm and use it later when the process is gone
(@current or @current->mm are NULL).
This changes mm_iommu_xxx API to receive mm_struct instead of using one
from @current.
This is needed by the following patch to do proper cleanup in time.
This depends on "powerpc/powernv/ioda: Fix endianness when reading TCEs"
to do proper cleanup via tce_iommu_clear() patch.
To keep API consistent, this replaces mm_context_t with mm_struct;
we stick to mm_struct as mm_iommu_adjust_locked_vm() helper needs
access to &mm->mmap_sem.
This should cause no behavioral change.
Signed-off-by: Alexey Kardashevskiy <redacted>
---
arch/powerpc/include/asm/mmu_context.h | 20 +++++++------
arch/powerpc/kernel/setup-common.c | 2 +-
arch/powerpc/mm/mmu_context_book3s64.c | 4 +--
arch/powerpc/mm/mmu_context_iommu.c | 54 ++++++++++++++--------------------
drivers/vfio/vfio_iommu_spapr_tce.c | 41 ++++++++++++++++----------
5 files changed, 62 insertions(+), 59 deletions(-)
@@ -63,28 +63,22 @@ static long mm_iommu_adjust_locked_vm(struct mm_struct *mm,returnret;}-boolmm_iommu_preregistered(void)+boolmm_iommu_preregistered(structmm_struct*mm){-if(!current||!current->mm)-returnfalse;--return!list_empty(¤t->mm->context.iommu_group_mem_list);+return!list_empty(&mm->context.iommu_group_mem_list);}EXPORT_SYMBOL_GPL(mm_iommu_preregistered);-longmm_iommu_get(unsignedlongua,unsignedlongentries,+longmm_iommu_get(structmm_struct*mm,unsignedlongua,unsignedlongentries,structmm_iommu_table_group_mem_t**pmem){structmm_iommu_table_group_mem_t*mem;longi,j,ret=0,locked_entries=0;structpage*page=NULL;-if(!current||!current->mm)-return-ESRCH;/* process exited */-mutex_lock(&mem_list_mutex);-list_for_each_entry_rcu(mem,¤t->mm->context.iommu_group_mem_list,+list_for_each_entry_rcu(mem,&mm->context.iommu_group_mem_list,next){if((mem->ua==ua)&&(mem->entries==entries)){++mem->used;
@@ -102,7 +96,7 @@ long mm_iommu_get(unsigned long ua, unsigned long entries,}-ret=mm_iommu_adjust_locked_vm(current->mm,entries,true);+ret=mm_iommu_adjust_locked_vm(mm,entries,true);if(ret)gotounlock_exit;
@@ -142,11 +136,11 @@ long mm_iommu_get(unsigned long ua, unsigned long entries,mem->entries=entries;*pmem=mem;-list_add_rcu(&mem->next,¤t->mm->context.iommu_group_mem_list);+list_add_rcu(&mem->next,&mm->context.iommu_group_mem_list);unlock_exit:if(locked_entries&&ret)-mm_iommu_adjust_locked_vm(current->mm,locked_entries,false);+mm_iommu_adjust_locked_vm(mm,locked_entries,false);mutex_unlock(&mem_list_mutex);
AFAICT, you've moved this call from _release() to _put(). Won't that cause a
behavioural change?
quoted hunk
call_rcu(&mem->rcu, mm_iommu_free);
}
-long mm_iommu_put(struct mm_iommu_table_group_mem_t *mem)
+long mm_iommu_put(struct mm_struct *mm, struct mm_iommu_table_group_mem_t *mem)
{
long ret = 0;
- if (!current || !current->mm)
- return -ESRCH; /* process exited */
mutex_lock(&mem_list_mutex);
@@ -224,6 +215,8 @@ long mm_iommu_put(struct mm_iommu_table_group_mem_t *mem) /* @mapped became 0 so now mappings are disabled, release the region */ mm_iommu_release(mem);+ mm_iommu_adjust_locked_vm(mm, mem->entries, false);+ unlock_exit: mutex_unlock(&mem_list_mutex);
@@ -110,11 +111,11 @@ static long tce_iommu_unregister_pages(struct tce_container *container,if((vaddr&~PAGE_MASK)||(size&~PAGE_MASK))return-EINVAL;-mem=mm_iommu_find(vaddr,size>>PAGE_SHIFT);+mem=mm_iommu_find(container->mm,vaddr,size>>PAGE_SHIFT);if(!mem)return-ENOENT;-returnmm_iommu_put(mem);+returnmm_iommu_put(container->mm,mem);}staticlongtce_iommu_register_pages(structtce_container*container,
@@ -128,10 +129,17 @@ static long tce_iommu_register_pages(struct tce_container *container,((vaddr+size)<vaddr))return-EINVAL;-r<et=mm_iommu_get(vaddr,entries,&mem);+if(!container->mm){+if(!current->mm)+return-ESRCH;/* process exited */
Can this ever happen? Surely the ioctl() path shouldn't be called
after the process mm has been cleaned up? i.e. should this be a
WARN_ON().
+
+ atomic_inc(¤t->mm->mm_count);
What balances this atomic_inc()? Is it the mmdrop() added to
tce_iommu_release()?
+ container->mm = current->mm;
+ }
Surely you need an error (or else a BUG_ON()) if current->mm !=
container->mm != NULL. I believe VFIO already assumes the container
is owned only by a single mm, but it looks like you should verify that here.
quoted hunk
+
+ ret = mm_iommu_get(container->mm, vaddr, entries, &mem);
if (ret)
return ret;
-
container->enabled = true;
return 0;
@@ -369,13 +379,14 @@ static void tce_iommu_unuse_page(struct tce_container *container, put_page(page); }-static int tce_iommu_prereg_ua_to_hpa(unsigned long tce, unsigned long size,+static int tce_iommu_prereg_ua_to_hpa(struct tce_container *container,+ unsigned long tce, unsigned long size, unsigned long *phpa, struct mm_iommu_table_group_mem_t **pmem) { long ret = 0; struct mm_iommu_table_group_mem_t *mem;- mem = mm_iommu_lookup(tce, size);+ mem = mm_iommu_lookup(container->mm, tce, size); if (!mem) return -EINVAL;
@@ -388,18 +399,18 @@ static int tce_iommu_prereg_ua_to_hpa(unsigned long tce, unsigned long size, return 0; }-static void tce_iommu_unuse_page_v2(struct iommu_table *tbl,- unsigned long entry)+static void tce_iommu_unuse_page_v2(struct tce_container *container,+ struct iommu_table *tbl, unsigned long entry) { struct mm_iommu_table_group_mem_t *mem = NULL; int ret; unsigned long hpa = 0; unsigned long *pua = IOMMU_TABLE_USERSPACE_ENTRY(tbl, entry);- if (!pua || !current || !current->mm)+ if (!pua) return;- ret = tce_iommu_prereg_ua_to_hpa(*pua, IOMMU_PAGE_SIZE(tbl),+ ret = tce_iommu_prereg_ua_to_hpa(container, *pua, IOMMU_PAGE_SIZE(tbl), &hpa, &mem); if (ret) pr_debug("%s: tce %lx at #%lx was not cached, ret=%d\n",
@@ -429,7 +440,7 @@ static int tce_iommu_clear(struct tce_container *container, continue; if (container->v2) {- tce_iommu_unuse_page_v2(tbl, entry);+ tce_iommu_unuse_page_v2(container, tbl, entry); continue; }
@@ -514,8 +525,8 @@ static long tce_iommu_build_v2(struct tce_container *container, unsigned long *pua = IOMMU_TABLE_USERSPACE_ENTRY(tbl, entry + i);- ret = tce_iommu_prereg_ua_to_hpa(tce, IOMMU_PAGE_SIZE(tbl),- &hpa, &mem);+ ret = tce_iommu_prereg_ua_to_hpa(container,+ tce, IOMMU_PAGE_SIZE(tbl), &hpa, &mem); if (ret) break;
@@ -536,7 +547,7 @@ static long tce_iommu_build_v2(struct tce_container *container, ret = iommu_tce_xchg(tbl, entry + i, &hpa, &dirtmp); if (ret) { /* dirtmp cannot be DMA_NONE here */- tce_iommu_unuse_page_v2(tbl, entry + i);+ tce_iommu_unuse_page_v2(container, tbl, entry + i); pr_err("iommu_tce: %s failed ioba=%lx, tce=%lx, ret=%ld\n", __func__, entry << tbl->it_page_shift, tce, 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-08-12 03:17:00
On Wed, Aug 03, 2016 at 06:40:48PM +1000, Alexey Kardashevskiy wrote:
At the moment iommu_table could be disposed by either calling
iommu_table_free() directly or it_ops::free() which only implementation
for IODA2 calls iommu_table_free() anyway.
As we are going to have reference counting on tables, we need an unified
way of disposing tables.
This moves it_ops::free() call into iommu_free_table() and makes use
of the latter everywhere. The free() callback now handles only
platform-specific data.
This should cause no behavioral change.
Signed-off-by: Alexey Kardashevskiy <redacted>
@@ -744,6 +747,7 @@ void iommu_free_table(struct iommu_table *tbl, const char *node_name)/* free table */kfree(tbl);}+EXPORT_SYMBOL_GPL(iommu_free_table);/* Creates TCEs for a user provided buffer. The user buffer must be*contiguousrealkernelstorage(notvmalloc).Theaddresspassedhere
--
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-08-12 03:17:00
On Wed, Aug 03, 2016 at 06:40:47PM +1000, Alexey Kardashevskiy wrote:
At the moment VFIO IOMMU SPAPR v2 driver pins all guest RAM pages when
the userspace starts using VFIO.
This doesn't sound accurate. Isn't it userspace that decides what
gets pinned, not the VFIO driver?
When the userspace process finishes,
all the pinned pages need to be put; this is done as a part of
the userspace memory context (MM) destruction which happens on
the very last mmdrop().
This approach has a problem that a MM of the userspace process
may live longer than the userspace process itself as kernel threads
use userspace process MMs which was runnning on a CPU where
the kernel thread was scheduled to. If this happened, the MM remains
referenced until this exact kernel thread wakes up again
and releases the very last reference to the MM, on an idle system this
can take even hours.
This references and caches MM once per container and adds tracking
how many times each preregistered area was registered in
a specific container. This way we do not depend on @current pointing to
a valid task descriptor.
The handling of @current and refcounting the mm sounds more like its
describing the previous patch.
THe description of counting how many times each prereg area is
registered doesn't seem accurate, since you block multiple
registration with an EBUSY. Or else it's describing the 'used'
counter in the lower-level mm_iommu_table_group_mem_t tracking,
rather than anything changed by this patch.
This changes the userspace interface to return EBUSY if memory is
already registered (mm_iommu_get() used to increment the counter);
however it should not have any practical effect as the only
userspace tool available now does register memory area once per
container anyway.
As tce_iommu_register_pages/tce_iommu_unregister_pages are called
under container->lock, this does not need additional locking.
Signed-off-by: Alexey Kardashevskiy <redacted>
# Conflicts:
# arch/powerpc/include/asm/mmu_context.h
# arch/powerpc/mm/mmu_context_book3s64.c
# arch/powerpc/mm/mmu_context_iommu.c
Looks like some lines to be cleaned up in the 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
From: David Gibson <hidden> Date: 2016-08-12 03:41:10
On Wed, Aug 03, 2016 at 06:40:49PM +1000, Alexey Kardashevskiy wrote:
So far iommu_table obejcts were only used in virtual mode and had
a single owner. We are going to change by implementing in-kernel
acceleration of DMA mapping requests, including real mode.
This adds a kref to iommu_table and defines new helpers to update it.
This replaces iommu_free_table() with iommu_table_put() and makes
iommu_free_table() static. iommu_table_get() is not used in this patch
but will be in the following one.
While we are here, this removes @node_name parameter as it has never been
really useful on powernv and carrying it for the pseries platform code to
iommu_free_table() seems to be quite useless too.
This should cause no behavioral change.
Signed-off-by: Alexey Kardashevskiy <redacted>
@@ -114,6 +114,7 @@ struct iommu_table {structlist_headit_group_list;/* List of iommu_table_group_link */unsignedlong*it_userspace;/* userspace view of the table */structiommu_table_ops*it_ops;+structkrefit_kref;};#define IOMMU_TABLE_USERSPACE_ENTRY(tbl, entry) \
@@ -146,8 +147,8 @@ static inline void *get_iommu_table_base(struct device *dev)externintdma_iommu_dma_supported(structdevice*dev,u64mask);-/* Frees table for an individual device node */-externvoidiommu_free_table(structiommu_table*tbl,constchar*node_name);+externvoidiommu_table_get(structiommu_table*tbl);+externvoidiommu_table_put(structiommu_table*tbl);/* Initializes an iommu_table based in values set in the passed-in*structure
@@ -735,7 +735,7 @@ void iommu_free_table(struct iommu_table *tbl, const char *node_name)/* verify that table contains no entries */if(!bitmap_empty(tbl->it_map,tbl->it_size))-pr_warn("%s: Unexpected TCEs for %s\n",__func__,node_name);+pr_warn("%s: Unexpected TCEs\n",__func__);/* calculate bitmap size in bytes */bitmap_sz=BITS_TO_LONGS(tbl->it_size)*sizeof(unsignedlong);
@@ -747,7 +747,21 @@ void iommu_free_table(struct iommu_table *tbl, const char *node_name)/* free table */kfree(tbl);}-EXPORT_SYMBOL_GPL(iommu_free_table);++voidiommu_table_get(structiommu_table*tbl)+{+kref_get(&tbl->it_kref);+}+EXPORT_SYMBOL_GPL(iommu_table_get);++voidiommu_table_put(structiommu_table*tbl)+{+if(!tbl)+return;++kref_put(&tbl->it_kref,iommu_table_free);+}+EXPORT_SYMBOL_GPL(iommu_table_put);/* Creates TCEs for a user provided buffer. The user buffer must be*contiguousrealkernelstorage(notvmalloc).Theaddresspassedhere
--
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-08-12 04:48:53
On Wed, Aug 03, 2016 at 06:40:51PM +1000, Alexey Kardashevskiy wrote:
quoted hunk
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 as we know that all of guest memory is pinned and
we have a flat array mapping GPA to HPA which makes it 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. Note that the rmap pointer is initialized to NULL where declared
(not in this patch).
Signed-off-by: Alexey Kardashevskiy <redacted>
---
Changes:
v2:
* updated the commit log with Paul's comment
---
arch/powerpc/kvm/book3s_64_vio_hv.c | 65 ++++++++++++++++++++++++++++---------
1 file changed, 49 insertions(+), 16 deletions(-)
@@ -180,6 +180,17 @@ long kvmppc_gpa_to_ua(struct kvm *kvm, unsigned long gpa,EXPORT_SYMBOL_GPL(kvmppc_gpa_to_ua);#ifdef CONFIG_KVM_BOOK3S_HV_POSSIBLE+staticinlineboolkvmppc_preregistered(structkvm_vcpu*vcpu)+{+returnmm_iommu_preregistered(vcpu->kvm->mm);+}++staticstructmm_iommu_table_group_mem_t*kvmppc_rm_iommu_lookup(+structkvm_vcpu*vcpu,unsignedlongua,unsignedlongsize)+{+returnmm_iommu_lookup_rm(vcpu->kvm->mm,ua,size);+}+longkvmppc_rm_h_put_tce(structkvm_vcpu*vcpu,unsignedlongliobn,unsignedlongioba,unsignedlongtce){
@@ -260,23 +271,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;
Wouldn't it be clearer to put the gpa->ua lookup outside the if?
You'd have to throw away the rmap you get in the prereg case, but it
shouldn't be harmful, should it?
- /*
- * Synchronize with the MMU notifier callbacks in
- * book3s_64_mmu_hv.c (kvm_unmap_hva_hv etc.).
- * While we have the rmap lock, code running on other CPUs
- * cannot finish unmapping the host real page that backs
- * this guest real page, so we are OK to access the host
- * real page.
- */
- lock_rmap(rmap);
- if (kvmppc_rm_ua_to_hpa(vcpu, ua, &tces)) {
- ret = H_TOO_HARD;
- goto unlock_exit;
+ mem = kvmppc_rm_iommu_lookup(vcpu, ua, IOMMU_PAGE_SIZE_4K);
+ if (!mem || mm_iommu_ua_to_hpa_rm(mem, ua, &tces))
+ return H_TOO_HARD;
This doesn't fall back to the rmap approach if it can't locate the
page in question in the prereg map. IIUC that means that this will
now work less well than previously if you have a userspace which
preregisters some memory, but not all of guest RAM. I'm not sure if
we care about that, since no such userspace currently exists.
+ } else {
+ /*
+ * This is emulated devices case.
This is a bit misleading - this case will only be triggered if there
are *no* prereg-ed VFIO devices. The case above can be used even for
emulated devices, if there happen to also be VFIO devices present
which have preregistered guest RAM.
quoted hunk
+ * We do not require memory to be preregistered in this case
+ * so lock rmap and do __find_linux_pte_or_hugepte().
+ */
+ if (kvmppc_gpa_to_ua(vcpu->kvm, tce_list, &ua, &rmap))
+ return H_TOO_HARD;
+
+ rmap = (void *) vmalloc_to_phys(rmap);
+
+ /*
+ * Synchronize with the MMU notifier callbacks in
+ * book3s_64_mmu_hv.c (kvm_unmap_hva_hv etc.).
+ * While we have the rmap lock, code running on other CPUs
+ * cannot finish unmapping the host real page that backs
+ * this guest real page, so we are OK to access the host
+ * real page.
+ */
+ lock_rmap(rmap);
+ if (kvmppc_rm_ua_to_hpa(vcpu, ua, &tces)) {
+ ret = H_TOO_HARD;
+ goto unlock_exit;
+ }
}
for (i = 0; i < npages; ++i) {
@@ -290,7 +322,8 @@ long kvmppc_rm_h_put_tce_indirect(struct kvm_vcpu *vcpu, } unlock_exit:- unlock_rmap(rmap);+ if (rmap)+ 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-08-12 04:48:53
On Wed, Aug 03, 2016 at 06:40:50PM +1000, 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_ua_to_hpa_rm() 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>
@@ -273,6 +292,26 @@ long mm_iommu_ua_to_hpa(struct mm_iommu_table_group_mem_t *mem,}EXPORT_SYMBOL_GPL(mm_iommu_ua_to_hpa);+longmm_iommu_ua_to_hpa_rm(structmm_iommu_table_group_mem_t*mem,+unsignedlongua,unsignedlong*hpa)+{+constlongentry=(ua-mem->ua)>>PAGE_SHIFT;+void*va=&mem->hpas[entry];+unsignedlong*ra;++if(entry>=mem->entries)+return-EFAULT;++ra=(void*)vmalloc_to_phys(va);+if(!ra)+return-EFAULT;++*hpa=*ra|(ua&~PAGE_MASK);++return0;+}+EXPORT_SYMBOL_GPL(mm_iommu_ua_to_hpa_rm);+longmm_iommu_mapped_inc(structmm_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-08-12 04:48:53
On Wed, Aug 03, 2016 at 06:40:53PM +1000, Alexey Kardashevskiy wrote:
quoted hunk
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(+)
I don't quite see how this change accomplishes the stated goal.
AFAICT even with this change you can still turn off IOMMU_SUPPORT,
which will break the IOMMU for VFIO passthrough, but not IOMMU
acceleration for emulated devices (since that requires no interaction
with the hardware IOMMU).
--
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-08-12 04:48:53
On Wed, Aug 03, 2016 at 06:40:52PM +1000, Alexey Kardashevskiy wrote:
quoted hunk
In real mode, TCE tables are invalidated using special
cache-inhibited store instructions which are not available in
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 | 23 +++++++++++++++++++++++
arch/powerpc/platforms/powernv/pci-ioda.c | 26 +++++++++++++++++++++++++-
3 files changed, 55 insertions(+), 1 deletion(-)
--
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-08-12 04:48:53
On Wed, Aug 03, 2016 at 06:40:54PM +1000, Alexey Kardashevskiy wrote:
The guest view TCE tables are per KVM anyway (not per VCPU) so pass kvm*
there. This will be used in the following patches where we will be
attaching VFIO containers to LIOBNs via ioctl() to KVM (rather than
to VCPU).
Signed-off-by: Alexey Kardashevskiy <redacted>
@@ -252,7 +252,7 @@ long kvmppc_rm_h_put_tce_indirect(struct kvm_vcpu *vcpu,unsignedlongtces,entry,ua=0;unsignedlong*rmap=NULL;-stt=kvmppc_find_table(vcpu,liobn);+stt=kvmppc_find_table(vcpu->kvm,liobn);if(!stt)returnH_TOO_HARD;
@@ -335,7 +335,7 @@ long kvmppc_rm_h_stuff_tce(struct kvm_vcpu *vcpu,structkvmppc_spapr_tce_table*stt;longi,ret;-stt=kvmppc_find_table(vcpu,liobn);+stt=kvmppc_find_table(vcpu->kvm,liobn);if(!stt)returnH_TOO_HARD;
@@ -356,12 +356,13 @@ long kvmppc_rm_h_stuff_tce(struct kvm_vcpu *vcpu,longkvmppc_h_get_tce(structkvm_vcpu*vcpu,unsignedlongliobn,unsignedlongioba){-structkvmppc_spapr_tce_table*stt=kvmppc_find_table(vcpu,liobn);+structkvmppc_spapr_tce_table*stt;longret;unsignedlongidx;structpage*page;u64*tbl;+stt=kvmppc_find_table(vcpu->kvm,liobn);if(!stt)returnH_TOO_HARD;
--
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, Aug 03, 2016 at 06:40:46PM +1000, Alexey Kardashevskiy wrote:
quoted
In some situations the userspace memory context may live longer than
the userspace process itself so if we need to do proper memory context
cleanup, we better cache @mm and use it later when the process is gone
(@current or @current->mm are NULL).
This changes mm_iommu_xxx API to receive mm_struct instead of using one
from @current.
This is needed by the following patch to do proper cleanup in time.
This depends on "powerpc/powernv/ioda: Fix endianness when reading TCEs"
to do proper cleanup via tce_iommu_clear() patch.
To keep API consistent, this replaces mm_context_t with mm_struct;
we stick to mm_struct as mm_iommu_adjust_locked_vm() helper needs
access to &mm->mmap_sem.
This should cause no behavioral change.
Signed-off-by: Alexey Kardashevskiy <redacted>
---
arch/powerpc/include/asm/mmu_context.h | 20 +++++++------
arch/powerpc/kernel/setup-common.c | 2 +-
arch/powerpc/mm/mmu_context_book3s64.c | 4 +--
arch/powerpc/mm/mmu_context_iommu.c | 54 ++++++++++++++--------------------
drivers/vfio/vfio_iommu_spapr_tce.c | 41 ++++++++++++++++----------
5 files changed, 62 insertions(+), 59 deletions(-)
@@ -63,28 +63,22 @@ static long mm_iommu_adjust_locked_vm(struct mm_struct *mm,returnret;}-boolmm_iommu_preregistered(void)+boolmm_iommu_preregistered(structmm_struct*mm){-if(!current||!current->mm)-returnfalse;--return!list_empty(¤t->mm->context.iommu_group_mem_list);+return!list_empty(&mm->context.iommu_group_mem_list);}EXPORT_SYMBOL_GPL(mm_iommu_preregistered);-longmm_iommu_get(unsignedlongua,unsignedlongentries,+longmm_iommu_get(structmm_struct*mm,unsignedlongua,unsignedlongentries,structmm_iommu_table_group_mem_t**pmem){structmm_iommu_table_group_mem_t*mem;longi,j,ret=0,locked_entries=0;structpage*page=NULL;-if(!current||!current->mm)-return-ESRCH;/* process exited */-mutex_lock(&mem_list_mutex);-list_for_each_entry_rcu(mem,¤t->mm->context.iommu_group_mem_list,+list_for_each_entry_rcu(mem,&mm->context.iommu_group_mem_list,next){if((mem->ua==ua)&&(mem->entries==entries)){++mem->used;
@@ -102,7 +96,7 @@ long mm_iommu_get(unsigned long ua, unsigned long entries,}-ret=mm_iommu_adjust_locked_vm(current->mm,entries,true);+ret=mm_iommu_adjust_locked_vm(mm,entries,true);if(ret)gotounlock_exit;
@@ -142,11 +136,11 @@ long mm_iommu_get(unsigned long ua, unsigned long entries,mem->entries=entries;*pmem=mem;-list_add_rcu(&mem->next,¤t->mm->context.iommu_group_mem_list);+list_add_rcu(&mem->next,&mm->context.iommu_group_mem_list);unlock_exit:if(locked_entries&&ret)-mm_iommu_adjust_locked_vm(current->mm,locked_entries,false);+mm_iommu_adjust_locked_vm(mm,locked_entries,false);mutex_unlock(&mem_list_mutex);
AFAICT, you've moved this call from _release() to _put(). Won't that cause a
behavioural change?
mm_iommu_put() calls mm_iommu_adjust_locked_vm() right after
m_iommu_release() so no, it does not look so.
quoted
call_rcu(&mem->rcu, mm_iommu_free);
}
-long mm_iommu_put(struct mm_iommu_table_group_mem_t *mem)
+long mm_iommu_put(struct mm_struct *mm, struct mm_iommu_table_group_mem_t *mem)
{
long ret = 0;
- if (!current || !current->mm)
- return -ESRCH; /* process exited */
mutex_lock(&mem_list_mutex);
@@ -224,6 +215,8 @@ long mm_iommu_put(struct mm_iommu_table_group_mem_t *mem) /* @mapped became 0 so now mappings are disabled, release the region */ mm_iommu_release(mem);+ mm_iommu_adjust_locked_vm(mm, mem->entries, false);+ unlock_exit: mutex_unlock(&mem_list_mutex);
@@ -110,11 +111,11 @@ static long tce_iommu_unregister_pages(struct tce_container *container,if((vaddr&~PAGE_MASK)||(size&~PAGE_MASK))return-EINVAL;-mem=mm_iommu_find(vaddr,size>>PAGE_SHIFT);+mem=mm_iommu_find(container->mm,vaddr,size>>PAGE_SHIFT);if(!mem)return-ENOENT;-returnmm_iommu_put(mem);+returnmm_iommu_put(container->mm,mem);}staticlongtce_iommu_register_pages(structtce_container*container,
@@ -128,10 +129,17 @@ static long tce_iommu_register_pages(struct tce_container *container,((vaddr+size)<vaddr))return-EINVAL;-r<et=mm_iommu_get(vaddr,entries,&mem);+if(!container->mm){+if(!current->mm)+return-ESRCH;/* process exited */
Can this ever happen? Surely the ioctl() path shouldn't be called
after the process mm has been cleaned up? i.e. should this be a
WARN_ON().
Not sure with SMP (one thread doing ioctl(), another - exiting QEMU) if it
is not that impossible but it is quite hard to trigger this check.
quoted
+
+ atomic_inc(¤t->mm->mm_count);
What balances this atomic_inc()? Is it the mmdrop() added to
tce_iommu_release()?
Yes. Surprisingly there is no mmget(), there is mmget_not_zero() but it is
for mm->mm_users.
quoted
+ container->mm = current->mm;
+ }
Surely you need an error (or else a BUG_ON()) if current->mm !=
container->mm != NULL. I believe VFIO already assumes the container
is owned only by a single mm, but it looks like you should verify that here.
I am not sure I really want to enforce it, do I? Who knows what kind of a
crazy person would create a container, pin pages and fork() that userspace
tool which may not be QEMU but something custom using DPDK or something.
What harm can not having this BUG_ON() cause?
quoted
+
+ ret = mm_iommu_get(container->mm, vaddr, entries, &mem);
if (ret)
return ret;
-
container->enabled = true;
return 0;
@@ -369,13 +379,14 @@ static void tce_iommu_unuse_page(struct tce_container *container, put_page(page); }-static int tce_iommu_prereg_ua_to_hpa(unsigned long tce, unsigned long size,+static int tce_iommu_prereg_ua_to_hpa(struct tce_container *container,+ unsigned long tce, unsigned long size, unsigned long *phpa, struct mm_iommu_table_group_mem_t **pmem) { long ret = 0; struct mm_iommu_table_group_mem_t *mem;- mem = mm_iommu_lookup(tce, size);+ mem = mm_iommu_lookup(container->mm, tce, size); if (!mem) return -EINVAL;
@@ -388,18 +399,18 @@ static int tce_iommu_prereg_ua_to_hpa(unsigned long tce, unsigned long size, return 0; }-static void tce_iommu_unuse_page_v2(struct iommu_table *tbl,- unsigned long entry)+static void tce_iommu_unuse_page_v2(struct tce_container *container,+ struct iommu_table *tbl, unsigned long entry) { struct mm_iommu_table_group_mem_t *mem = NULL; int ret; unsigned long hpa = 0; unsigned long *pua = IOMMU_TABLE_USERSPACE_ENTRY(tbl, entry);- if (!pua || !current || !current->mm)+ if (!pua) return;- ret = tce_iommu_prereg_ua_to_hpa(*pua, IOMMU_PAGE_SIZE(tbl),+ ret = tce_iommu_prereg_ua_to_hpa(container, *pua, IOMMU_PAGE_SIZE(tbl), &hpa, &mem); if (ret) pr_debug("%s: tce %lx at #%lx was not cached, ret=%d\n",
@@ -429,7 +440,7 @@ static int tce_iommu_clear(struct tce_container *container, continue; if (container->v2) {- tce_iommu_unuse_page_v2(tbl, entry);+ tce_iommu_unuse_page_v2(container, tbl, entry); continue; }
@@ -514,8 +525,8 @@ static long tce_iommu_build_v2(struct tce_container *container, unsigned long *pua = IOMMU_TABLE_USERSPACE_ENTRY(tbl, entry + i);- ret = tce_iommu_prereg_ua_to_hpa(tce, IOMMU_PAGE_SIZE(tbl),- &hpa, &mem);+ ret = tce_iommu_prereg_ua_to_hpa(container,+ tce, IOMMU_PAGE_SIZE(tbl), &hpa, &mem); if (ret) break;
@@ -536,7 +547,7 @@ static long tce_iommu_build_v2(struct tce_container *container, ret = iommu_tce_xchg(tbl, entry + i, &hpa, &dirtmp); if (ret) { /* dirtmp cannot be DMA_NONE here */- tce_iommu_unuse_page_v2(tbl, entry + i);+ tce_iommu_unuse_page_v2(container, tbl, entry + i); pr_err("iommu_tce: %s failed ioba=%lx, tce=%lx, ret=%ld\n", __func__, entry << tbl->it_page_shift, tce, ret);
From: David Gibson <hidden> Date: 2016-08-12 05:44:04
On Wed, Aug 10, 2016 at 10:46:30AM -0600, Alex Williamson wrote:
On Wed, 10 Aug 2016 15:37:17 +1000
Alexey Kardashevskiy [off-list ref] wrote:
quoted
On 09/08/16 22:16, Alex Williamson wrote:
quoted
On Tue, 9 Aug 2016 15:19:39 +1000
Alexey Kardashevskiy [off-list ref] wrote:
quoted
On 09/08/16 02:43, Alex Williamson wrote:
quoted
On Wed, 3 Aug 2016 18:40:55 +1000
Alexey Kardashevskiy [off-list ref] wrote:
quoted
This exports helpers which are needed to keep a VFIO container in
memory while there are external users such as KVM.
Signed-off-by: Alexey Kardashevskiy <redacted>
---
drivers/vfio/vfio.c | 30 ++++++++++++++++++++++++++++++
drivers/vfio/vfio_iommu_spapr_tce.c | 16 +++++++++++++++-
include/linux/vfio.h | 6 ++++++
3 files changed, 51 insertions(+), 1 deletion(-)
I think you need to take a closer look of the lifecycle of a container,
having a reference means the container itself won't go away, but only
having a group set within that container holds the actual IOMMU
references. container->iommu_data is going to be NULL once the
groups are lost. Thanks,
Container owns the iommu tables and this is what I care about here, groups
attached or not - this is handled separately via IOMMU group list in a
specific iommu_table struct, these groups get detached from iommu_table
when they are removed from a container.
The container doesn't own anything, the container is privileged by the
groups being attached to it. When groups are closed, they detach from
the container and once the container group list is empty the iommu
backend is released and iommu_data is NULL. A container reference
doesn't give you what you're looking for. It implies nothing about the
iommu backend.
Well. Backend is a part of a container and since a backend owns tables, a
container owns them too.
The IOMMU backend is accessed through the container, but that backend
is privileged by the groups it contains. Once those groups are gone,
the IOMMU backend is released, regardless of whatever reference you
have to the container itself such as you're attempting to do here. In
that sense, the container does not own those tables.
So, the thing is that what KVM fundamentally needs is a handle on the
container. KVM is essentially modelling the DMA address space of a
single guest bus, and the container is what's attached to that.
The first part of the problem is that KVM wants to basically invoke
vfio_dma_map() operations without bouncing via qemu. Because
vfio_dma_map() works on the container level, that's the handle that
KVM needs to hold.
The second part of the problem is that in order to reduce overhead
further, we want to operate in real mode, which means bypassing most
of the usual VFIO structure and going directly(ish) from the KVM
hcall emulation to the IOMMU backend behind VFIO. This complicates
matters a fair bit. Because it is, explicitly, a performance hack,
some degree of ugliness is probably inevitable.
Alexey - actually implementing this in two stages might make this
clearer. The first stage wouldn't allow real mode, and would call
through the same vfio_dma_map() path as qemu calls through now. The
second stage would then put in place the necessary hacks to add real
mode support.
quoted
The problem I am trying to solve here is when KVM may release the
iommu_table objects.
"Set" ioctl() to KVM-spapr-tce-table (or KVM itself, does not really
matter) makes a link between KVM-spapr-tce-table and container and KVM can
start using tables (with referencing them).
First I tried adding an "unset" ioctl to KVM-spapr-tce-table, called it
from region_del() and this works if QEMU removes a window. However if QEMU
removes a vfio-pci device, region_del() is not called and KVM does not get
notified that it can release the iommu_table's because the
KVM-spapr-tce-table remains alive and does not get destroyed (as it is
still used by emulated devices or other containers).
So it was suggested that we could do such "unset" somehow later assuming,
for example, on every "set" I could check if some of currently attached
containers are no more used - and this is where being able to know if there
is no backend helps - KVM remembers a container pointer and can check this
via vfio_container_get_iommu_data_ext().
The other option would be changing vfio_container_get_ext() to take a
callback+opaque which container would call when it destroys iommu_data.
This looks more intrusive and not very intuitive how to make it right -
container would have to keep track of all registered external users and
vfio_container_put_ext() would have to pass the same callback+opaque to
unregister the exact external user.
I'm not in favor of anything resembling the code above or extensions
beyond it, the container is the wrong place to do this.
quoted
Or I could store container file* in KVM. Then iommu_data would never be
released until KVM-spapr-tce-table is destroyed.
See above, holding a file pointer to the container doesn't do squat.
The groups that are held by the container empower the IOMMU backend,
references to the container itself don't matter. Those references will
not maintain the IOMMU data.
quoted
Recreating KVM-spapr-tce-table on every vfio-pci hotunplug (closing its fd
would "unset" container from KVM-spapr-tce-table) is not an option as there
still may be devices using this KVM-spapr-tce-table.
What obvious and nice solution am I missing here? Thanks.
The interactions with the IOMMU backend that seem relevant are
vfio_iommu_drivers_ops.{detach_group,release}. The kvm-vfio pseudo
device is also used to tell kvm about groups as they come and go and
has a way to check extensions, and thus properties of the IOMMU
backend. All of these are available for your {ab}use. Thanks,
So, Alexey started trying to do this via the KVM-VFIO device, but it's
a really bad fit. As noted above, fundamentally it's a container we
need to attach to the kvm-spapr-tce-table object, since what that
represents is a guest bus DMA address space, and by definition all the
groups in a container must have the same DMA address space.
--
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-08-12 05:44:04
On Wed, Aug 03, 2016 at 06:40:55PM +1000, Alexey Kardashevskiy wrote:
This exports helpers which are needed to keep a VFIO container in
memory while there are external users such as KVM.
Signed-off-by: Alexey Kardashevskiy <redacted>
I'll address Alex W's broader concerns in a different mail. But
there are some more superficial problems with this as well.
I really dislike this name. I was confused for a while why this
existed on top of vfio_container_get_ext(), the names are so similar.
Making it take a void * is also really nasty since that void * has to
be something specific. It would be better to have this take a
vfio_container *, verify that the container really does have an
spapr_tce backend, then lookup the tce_container and the actual IOMMU
tables within.
That might also let you drop vfio_container_get_iommu_data_ext()
entirely.
--
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, Aug 10, 2016 at 10:46:30AM -0600, Alex Williamson wrote:
quoted
On Wed, 10 Aug 2016 15:37:17 +1000
Alexey Kardashevskiy [off-list ref] wrote:
quoted
On 09/08/16 22:16, Alex Williamson wrote:
quoted
On Tue, 9 Aug 2016 15:19:39 +1000
Alexey Kardashevskiy [off-list ref] wrote:
quoted
On 09/08/16 02:43, Alex Williamson wrote:
quoted
On Wed, 3 Aug 2016 18:40:55 +1000
Alexey Kardashevskiy [off-list ref] wrote:
quoted
This exports helpers which are needed to keep a VFIO container in
memory while there are external users such as KVM.
Signed-off-by: Alexey Kardashevskiy <redacted>
---
drivers/vfio/vfio.c | 30 ++++++++++++++++++++++++++++++
drivers/vfio/vfio_iommu_spapr_tce.c | 16 +++++++++++++++-
include/linux/vfio.h | 6 ++++++
3 files changed, 51 insertions(+), 1 deletion(-)
I think you need to take a closer look of the lifecycle of a container,
having a reference means the container itself won't go away, but only
having a group set within that container holds the actual IOMMU
references. container->iommu_data is going to be NULL once the
groups are lost. Thanks,
Container owns the iommu tables and this is what I care about here, groups
attached or not - this is handled separately via IOMMU group list in a
specific iommu_table struct, these groups get detached from iommu_table
when they are removed from a container.
The container doesn't own anything, the container is privileged by the
groups being attached to it. When groups are closed, they detach from
the container and once the container group list is empty the iommu
backend is released and iommu_data is NULL. A container reference
doesn't give you what you're looking for. It implies nothing about the
iommu backend.
Well. Backend is a part of a container and since a backend owns tables, a
container owns them too.
The IOMMU backend is accessed through the container, but that backend
is privileged by the groups it contains. Once those groups are gone,
the IOMMU backend is released, regardless of whatever reference you
have to the container itself such as you're attempting to do here. In
that sense, the container does not own those tables.
So, the thing is that what KVM fundamentally needs is a handle on the
container. KVM is essentially modelling the DMA address space of a
single guest bus, and the container is what's attached to that.
The first part of the problem is that KVM wants to basically invoke
vfio_dma_map() operations without bouncing via qemu. Because
vfio_dma_map() works on the container level, that's the handle that
KVM needs to hold.
Well, I do not need to hold the reference to the container all the time, I
just need it to get to the IOMMU backend, get+reference an iommu_table from
it, referencing here helps to make sure the backend is not going away
before we reference iommu_table.
After that I only keep a reference to the container to know if/when I can
release a particular iommu_table. This is can workaround by counting how
many groups were attached to this particular KVM-spapt-tce-table and
looking at the IOMMU group list attached to an iommu_table - if the list is
empty, decrement the iommu_table reference counter and that's it, no extra
references to a VFIO container.
Or I need an alternative way of getting iommu_table's, i.e. QEMU should
somehow tell KVM that this LIOBN is this VFIO container fd (easy - can be
done via region_add/region_del interface) or VFIO IOMMU group fd(s) (more
tricky as this needs to be done from more places - vfio-pci hotplug/unplug,
window add/remove).
The second part of the problem is that in order to reduce overhead
further, we want to operate in real mode, which means bypassing most
of the usual VFIO structure and going directly(ish) from the KVM
hcall emulation to the IOMMU backend behind VFIO. This complicates
matters a fair bit. Because it is, explicitly, a performance hack,
some degree of ugliness is probably inevitable.
Alexey - actually implementing this in two stages might make this
clearer. The first stage wouldn't allow real mode, and would call
through the same vfio_dma_map() path as qemu calls through now. The
second stage would then put in place the necessary hacks to add real
mode support.
quoted
quoted
The problem I am trying to solve here is when KVM may release the
iommu_table objects.
"Set" ioctl() to KVM-spapr-tce-table (or KVM itself, does not really
matter) makes a link between KVM-spapr-tce-table and container and KVM can
start using tables (with referencing them).
First I tried adding an "unset" ioctl to KVM-spapr-tce-table, called it
from region_del() and this works if QEMU removes a window. However if QEMU
removes a vfio-pci device, region_del() is not called and KVM does not get
notified that it can release the iommu_table's because the
KVM-spapr-tce-table remains alive and does not get destroyed (as it is
still used by emulated devices or other containers).
So it was suggested that we could do such "unset" somehow later assuming,
for example, on every "set" I could check if some of currently attached
containers are no more used - and this is where being able to know if there
is no backend helps - KVM remembers a container pointer and can check this
via vfio_container_get_iommu_data_ext().
The other option would be changing vfio_container_get_ext() to take a
callback+opaque which container would call when it destroys iommu_data.
This looks more intrusive and not very intuitive how to make it right -
container would have to keep track of all registered external users and
vfio_container_put_ext() would have to pass the same callback+opaque to
unregister the exact external user.
I'm not in favor of anything resembling the code above or extensions
beyond it, the container is the wrong place to do this.
quoted
Or I could store container file* in KVM. Then iommu_data would never be
released until KVM-spapr-tce-table is destroyed.
See above, holding a file pointer to the container doesn't do squat.
The groups that are held by the container empower the IOMMU backend,
references to the container itself don't matter. Those references will
not maintain the IOMMU data.
quoted
Recreating KVM-spapr-tce-table on every vfio-pci hotunplug (closing its fd
would "unset" container from KVM-spapr-tce-table) is not an option as there
still may be devices using this KVM-spapr-tce-table.
What obvious and nice solution am I missing here? Thanks.
The interactions with the IOMMU backend that seem relevant are
vfio_iommu_drivers_ops.{detach_group,release}. The kvm-vfio pseudo
device is also used to tell kvm about groups as they come and go and
has a way to check extensions, and thus properties of the IOMMU
backend. All of these are available for your {ab}use. Thanks,
So, Alexey started trying to do this via the KVM-VFIO device, but it's
a really bad fit. As noted above, fundamentally it's a container we
need to attach to the kvm-spapr-tce-table object, since what that
represents is a guest bus DMA address space, and by definition all the
groups in a container must have the same DMA address space.
Well, in a bad case a LIOBN/kvm-spapr-tce-table has multiple containers
attached so it is not 1:1...
--
Alexey
From: Alex Williamson <hidden> Date: 2016-08-12 15:22:04
On Fri, 12 Aug 2016 15:46:01 +1000
David Gibson [off-list ref] wrote:
On Wed, Aug 10, 2016 at 10:46:30AM -0600, Alex Williamson wrote:
quoted
On Wed, 10 Aug 2016 15:37:17 +1000
Alexey Kardashevskiy [off-list ref] wrote:
quoted
On 09/08/16 22:16, Alex Williamson wrote:
quoted
On Tue, 9 Aug 2016 15:19:39 +1000
Alexey Kardashevskiy [off-list ref] wrote:
quoted
On 09/08/16 02:43, Alex Williamson wrote:
quoted
On Wed, 3 Aug 2016 18:40:55 +1000
Alexey Kardashevskiy [off-list ref] wrote:
quoted
This exports helpers which are needed to keep a VFIO container in
memory while there are external users such as KVM.
Signed-off-by: Alexey Kardashevskiy <redacted>
---
drivers/vfio/vfio.c | 30 ++++++++++++++++++++++++++++++
drivers/vfio/vfio_iommu_spapr_tce.c | 16 +++++++++++++++-
include/linux/vfio.h | 6 ++++++
3 files changed, 51 insertions(+), 1 deletion(-)
I think you need to take a closer look of the lifecycle of a container,
having a reference means the container itself won't go away, but only
having a group set within that container holds the actual IOMMU
references. container->iommu_data is going to be NULL once the
groups are lost. Thanks,
Container owns the iommu tables and this is what I care about here, groups
attached or not - this is handled separately via IOMMU group list in a
specific iommu_table struct, these groups get detached from iommu_table
when they are removed from a container.
The container doesn't own anything, the container is privileged by the
groups being attached to it. When groups are closed, they detach from
the container and once the container group list is empty the iommu
backend is released and iommu_data is NULL. A container reference
doesn't give you what you're looking for. It implies nothing about the
iommu backend.
Well. Backend is a part of a container and since a backend owns tables, a
container owns them too.
The IOMMU backend is accessed through the container, but that backend
is privileged by the groups it contains. Once those groups are gone,
the IOMMU backend is released, regardless of whatever reference you
have to the container itself such as you're attempting to do here. In
that sense, the container does not own those tables.
So, the thing is that what KVM fundamentally needs is a handle on the
container. KVM is essentially modelling the DMA address space of a
single guest bus, and the container is what's attached to that.
The first part of the problem is that KVM wants to basically invoke
vfio_dma_map() operations without bouncing via qemu. Because
vfio_dma_map() works on the container level, that's the handle that
KVM needs to hold.
The second part of the problem is that in order to reduce overhead
further, we want to operate in real mode, which means bypassing most
of the usual VFIO structure and going directly(ish) from the KVM
hcall emulation to the IOMMU backend behind VFIO. This complicates
matters a fair bit. Because it is, explicitly, a performance hack,
some degree of ugliness is probably inevitable.
Alexey - actually implementing this in two stages might make this
clearer. The first stage wouldn't allow real mode, and would call
through the same vfio_dma_map() path as qemu calls through now. The
second stage would then put in place the necessary hacks to add real
mode support.
quoted
quoted
The problem I am trying to solve here is when KVM may release the
iommu_table objects.
"Set" ioctl() to KVM-spapr-tce-table (or KVM itself, does not really
matter) makes a link between KVM-spapr-tce-table and container and KVM can
start using tables (with referencing them).
First I tried adding an "unset" ioctl to KVM-spapr-tce-table, called it
from region_del() and this works if QEMU removes a window. However if QEMU
removes a vfio-pci device, region_del() is not called and KVM does not get
notified that it can release the iommu_table's because the
KVM-spapr-tce-table remains alive and does not get destroyed (as it is
still used by emulated devices or other containers).
So it was suggested that we could do such "unset" somehow later assuming,
for example, on every "set" I could check if some of currently attached
containers are no more used - and this is where being able to know if there
is no backend helps - KVM remembers a container pointer and can check this
via vfio_container_get_iommu_data_ext().
The other option would be changing vfio_container_get_ext() to take a
callback+opaque which container would call when it destroys iommu_data.
This looks more intrusive and not very intuitive how to make it right -
container would have to keep track of all registered external users and
vfio_container_put_ext() would have to pass the same callback+opaque to
unregister the exact external user.
I'm not in favor of anything resembling the code above or extensions
beyond it, the container is the wrong place to do this.
quoted
Or I could store container file* in KVM. Then iommu_data would never be
released until KVM-spapr-tce-table is destroyed.
See above, holding a file pointer to the container doesn't do squat.
The groups that are held by the container empower the IOMMU backend,
references to the container itself don't matter. Those references will
not maintain the IOMMU data.
quoted
Recreating KVM-spapr-tce-table on every vfio-pci hotunplug (closing its fd
would "unset" container from KVM-spapr-tce-table) is not an option as there
still may be devices using this KVM-spapr-tce-table.
What obvious and nice solution am I missing here? Thanks.
The interactions with the IOMMU backend that seem relevant are
vfio_iommu_drivers_ops.{detach_group,release}. The kvm-vfio pseudo
device is also used to tell kvm about groups as they come and go and
has a way to check extensions, and thus properties of the IOMMU
backend. All of these are available for your {ab}use. Thanks,
So, Alexey started trying to do this via the KVM-VFIO device, but it's
a really bad fit. As noted above, fundamentally it's a container we
need to attach to the kvm-spapr-tce-table object, since what that
represents is a guest bus DMA address space, and by definition all the
groups in a container must have the same DMA address space.
That's all fine and good, but the point remains that a reference to the
container is no assurance of the iommu state. The iommu state is
maintained by the user and the groups attached to the container. If
the groups are removed, your container reference no long has any iommu
backing and iommu_data is worthless. The user can do this as well by
un-setting the iommu. I understand what you're trying to do, it's just
wrong. Thanks,
Alex
From: Paul Mackerras <hidden> Date: 2016-08-15 03:59:50
On Tue, Aug 09, 2016 at 06:16:30AM -0600, Alex Williamson wrote:
On Tue, 9 Aug 2016 15:19:39 +1000
Alexey Kardashevskiy [off-list ref] wrote:
quoted
On 09/08/16 02:43, Alex Williamson wrote:
quoted
I think you need to take a closer look of the lifecycle of a container,
having a reference means the container itself won't go away, but only
having a group set within that container holds the actual IOMMU
references. container->iommu_data is going to be NULL once the
groups are lost. Thanks,
Container owns the iommu tables and this is what I care about here, groups
attached or not - this is handled separately via IOMMU group list in a
specific iommu_table struct, these groups get detached from iommu_table
when they are removed from a container.
The container doesn't own anything, the container is privileged by the
groups being attached to it. When groups are closed, they detach from
the container and once the container group list is empty the iommu
backend is released and iommu_data is NULL. A container reference
doesn't give you what you're looking for. It implies nothing about the
iommu backend.
Alex, I'd like to understand more what the objection is here - is it
just about the object lifetimes, or is it a more fundamental objection
to the style of interface?
Regarding lifetimes, my understanding was that Alexey's previous
patches added refcounting to the iommu tables, so that KVM could get a
reference to the iommu tables through the container and then safely
use the iommu tables directly. There may still be a potential race in
the interval between asking the container about its iommu tables and
incrementing the tables' reference counts, but that should be able to
be solved. I don't see any unsolvable problem regarding lifetimes.
Or is your objection about any external access to the container?
As far as I know, when a group is not part of a container it has its
own iommu tables, but when it is put in a container it loses its own
iommu tables and instead uses a common pair of iommu tables (one for
the 32-bit window, one for the 64-bit window) that belong to the
container. So we do in fact need the container's iommu tables not the
individual groups' tables.
Regards,
Paul.
From: Paul Mackerras <hidden> Date: 2016-08-15 09:38:32
On Wed, Aug 03, 2016 at 06:40:42PM +1000, Alexey Kardashevskiy wrote:
This reverts commit aa16bea929ae
("iommu: Add a function to find an iommu group by id")
as the iommu_group_get_by_id() helper has never been used
and it is unlikely it will in foreseeable future. Dead code
is broken code.
Signed-off-by: Alexey Kardashevskiy <redacted>
There's another definition of iommu_group_get_by_id() further down in
iommu.h (static inline after the #else on CONFIG_IOMMU_API) which also
needs to be removed.
Paul.
From: David Gibson <hidden> Date: 2016-08-15 11:52:19
On Fri, Aug 12, 2016 at 04:12:17PM +1000, Alexey Kardashevskiy wrote:
On 12/08/16 15:46, David Gibson wrote:
quoted
On Wed, Aug 10, 2016 at 10:46:30AM -0600, Alex Williamson wrote:
quoted
On Wed, 10 Aug 2016 15:37:17 +1000
Alexey Kardashevskiy [off-list ref] wrote:
quoted
On 09/08/16 22:16, Alex Williamson wrote:
quoted
On Tue, 9 Aug 2016 15:19:39 +1000
Alexey Kardashevskiy [off-list ref] wrote:
quoted
On 09/08/16 02:43, Alex Williamson wrote:
quoted
On Wed, 3 Aug 2016 18:40:55 +1000
Alexey Kardashevskiy [off-list ref] wrote:
quoted
This exports helpers which are needed to keep a VFIO container in
memory while there are external users such as KVM.
Signed-off-by: Alexey Kardashevskiy <redacted>
---
drivers/vfio/vfio.c | 30 ++++++++++++++++++++++++++++++
drivers/vfio/vfio_iommu_spapr_tce.c | 16 +++++++++++++++-
include/linux/vfio.h | 6 ++++++
3 files changed, 51 insertions(+), 1 deletion(-)
I think you need to take a closer look of the lifecycle of a container,
having a reference means the container itself won't go away, but only
having a group set within that container holds the actual IOMMU
references. container->iommu_data is going to be NULL once the
groups are lost. Thanks,
Container owns the iommu tables and this is what I care about here, groups
attached or not - this is handled separately via IOMMU group list in a
specific iommu_table struct, these groups get detached from iommu_table
when they are removed from a container.
The container doesn't own anything, the container is privileged by the
groups being attached to it. When groups are closed, they detach from
the container and once the container group list is empty the iommu
backend is released and iommu_data is NULL. A container reference
doesn't give you what you're looking for. It implies nothing about the
iommu backend.
Well. Backend is a part of a container and since a backend owns tables, a
container owns them too.
The IOMMU backend is accessed through the container, but that backend
is privileged by the groups it contains. Once those groups are gone,
the IOMMU backend is released, regardless of whatever reference you
have to the container itself such as you're attempting to do here. In
that sense, the container does not own those tables.
So, the thing is that what KVM fundamentally needs is a handle on the
container. KVM is essentially modelling the DMA address space of a
single guest bus, and the container is what's attached to that.
The first part of the problem is that KVM wants to basically invoke
vfio_dma_map() operations without bouncing via qemu. Because
vfio_dma_map() works on the container level, that's the handle that
KVM needs to hold.
Well, I do not need to hold the reference to the container all the time, I
just need it to get to the IOMMU backend, get+reference an iommu_table from
it, referencing here helps to make sure the backend is not going away
before we reference iommu_table.
Yes, but I don't see a compelling reason *not* to hold the container
reference either - it seems like principle of least surprise would
suggest retaining the reference.
For example, I can imagine having a container reset call which threw
away the back end iommu table and created a new one. It seems like
what you'd expect in this case is for the guest bus to remain bound to
the same container, not to the now stale iommu table.
After that I only keep a reference to the container to know if/when I can
release a particular iommu_table. This is can workaround by counting how
many groups were attached to this particular KVM-spapt-tce-table and
looking at the IOMMU group list attached to an iommu_table - if the list is
empty, decrement the iommu_table reference counter and that's it, no extra
references to a VFIO container.
Or I need an alternative way of getting iommu_table's, i.e. QEMU should
somehow tell KVM that this LIOBN is this VFIO container fd (easy - can be
done via region_add/region_del interface)
Um.. yes.. that's what I was expecting, I thought that was what you
were doing.x
or VFIO IOMMU group fd(s) (more
tricky as this needs to be done from more places - vfio-pci hotplug/unplug,
window add/remove).
More tricky and also wrong. Again, having one group but not the whole
container bound to the guest LIOBN doesn't make any sense - by
definition, all the devices in the container should share the same DMA
address space.
quoted
The second part of the problem is that in order to reduce overhead
further, we want to operate in real mode, which means bypassing most
of the usual VFIO structure and going directly(ish) from the KVM
hcall emulation to the IOMMU backend behind VFIO. This complicates
matters a fair bit. Because it is, explicitly, a performance hack,
some degree of ugliness is probably inevitable.
Alexey - actually implementing this in two stages might make this
clearer. The first stage wouldn't allow real mode, and would call
through the same vfio_dma_map() path as qemu calls through now. The
second stage would then put in place the necessary hacks to add real
mode support.
quoted
quoted
The problem I am trying to solve here is when KVM may release the
iommu_table objects.
"Set" ioctl() to KVM-spapr-tce-table (or KVM itself, does not really
matter) makes a link between KVM-spapr-tce-table and container and KVM can
start using tables (with referencing them).
First I tried adding an "unset" ioctl to KVM-spapr-tce-table, called it
from region_del() and this works if QEMU removes a window. However if QEMU
removes a vfio-pci device, region_del() is not called and KVM does not get
notified that it can release the iommu_table's because the
KVM-spapr-tce-table remains alive and does not get destroyed (as it is
still used by emulated devices or other containers).
So it was suggested that we could do such "unset" somehow later assuming,
for example, on every "set" I could check if some of currently attached
containers are no more used - and this is where being able to know if there
is no backend helps - KVM remembers a container pointer and can check this
via vfio_container_get_iommu_data_ext().
The other option would be changing vfio_container_get_ext() to take a
callback+opaque which container would call when it destroys iommu_data.
This looks more intrusive and not very intuitive how to make it right -
container would have to keep track of all registered external users and
vfio_container_put_ext() would have to pass the same callback+opaque to
unregister the exact external user.
I'm not in favor of anything resembling the code above or extensions
beyond it, the container is the wrong place to do this.
quoted
Or I could store container file* in KVM. Then iommu_data would never be
released until KVM-spapr-tce-table is destroyed.
See above, holding a file pointer to the container doesn't do squat.
The groups that are held by the container empower the IOMMU backend,
references to the container itself don't matter. Those references will
not maintain the IOMMU data.
quoted
Recreating KVM-spapr-tce-table on every vfio-pci hotunplug (closing its fd
would "unset" container from KVM-spapr-tce-table) is not an option as there
still may be devices using this KVM-spapr-tce-table.
What obvious and nice solution am I missing here? Thanks.
The interactions with the IOMMU backend that seem relevant are
vfio_iommu_drivers_ops.{detach_group,release}. The kvm-vfio pseudo
device is also used to tell kvm about groups as they come and go and
has a way to check extensions, and thus properties of the IOMMU
backend. All of these are available for your {ab}use. Thanks,
So, Alexey started trying to do this via the KVM-VFIO device, but it's
a really bad fit. As noted above, fundamentally it's a container we
need to attach to the kvm-spapr-tce-table object, since what that
represents is a guest bus DMA address space, and by definition all the
groups in a container must have the same DMA address space.
Well, in a bad case a LIOBN/kvm-spapr-tce-table has multiple containers
attached so it is not 1:1...
I never said it was. It's n:1, but it's *not* n:m. You can have
multiple containers to a LIOBN, but never multiple LIOBNs ot a
container.
--
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-08-15 11:52:19
On Fri, Aug 12, 2016 at 02:56:59PM +1000, Alexey Kardashevskiy wrote:
On 12/08/16 12:57, David Gibson wrote:
quoted
On Wed, Aug 03, 2016 at 06:40:46PM +1000, Alexey Kardashevskiy wrote:
quoted
In some situations the userspace memory context may live longer than
the userspace process itself so if we need to do proper memory context
cleanup, we better cache @mm and use it later when the process is gone
(@current or @current->mm are NULL).
This changes mm_iommu_xxx API to receive mm_struct instead of using one
from @current.
This is needed by the following patch to do proper cleanup in time.
This depends on "powerpc/powernv/ioda: Fix endianness when reading TCEs"
to do proper cleanup via tce_iommu_clear() patch.
To keep API consistent, this replaces mm_context_t with mm_struct;
we stick to mm_struct as mm_iommu_adjust_locked_vm() helper needs
access to &mm->mmap_sem.
This should cause no behavioral change.
Signed-off-by: Alexey Kardashevskiy <redacted>
---
arch/powerpc/include/asm/mmu_context.h | 20 +++++++------
arch/powerpc/kernel/setup-common.c | 2 +-
arch/powerpc/mm/mmu_context_book3s64.c | 4 +--
arch/powerpc/mm/mmu_context_iommu.c | 54 ++++++++++++++--------------------
drivers/vfio/vfio_iommu_spapr_tce.c | 41 ++++++++++++++++----------
5 files changed, 62 insertions(+), 59 deletions(-)
@@ -63,28 +63,22 @@ static long mm_iommu_adjust_locked_vm(struct mm_struct *mm,returnret;}-boolmm_iommu_preregistered(void)+boolmm_iommu_preregistered(structmm_struct*mm){-if(!current||!current->mm)-returnfalse;--return!list_empty(¤t->mm->context.iommu_group_mem_list);+return!list_empty(&mm->context.iommu_group_mem_list);}EXPORT_SYMBOL_GPL(mm_iommu_preregistered);-longmm_iommu_get(unsignedlongua,unsignedlongentries,+longmm_iommu_get(structmm_struct*mm,unsignedlongua,unsignedlongentries,structmm_iommu_table_group_mem_t**pmem){structmm_iommu_table_group_mem_t*mem;longi,j,ret=0,locked_entries=0;structpage*page=NULL;-if(!current||!current->mm)-return-ESRCH;/* process exited */-mutex_lock(&mem_list_mutex);-list_for_each_entry_rcu(mem,¤t->mm->context.iommu_group_mem_list,+list_for_each_entry_rcu(mem,&mm->context.iommu_group_mem_list,next){if((mem->ua==ua)&&(mem->entries==entries)){++mem->used;
@@ -102,7 +96,7 @@ long mm_iommu_get(unsigned long ua, unsigned long entries,}-ret=mm_iommu_adjust_locked_vm(current->mm,entries,true);+ret=mm_iommu_adjust_locked_vm(mm,entries,true);if(ret)gotounlock_exit;
@@ -142,11 +136,11 @@ long mm_iommu_get(unsigned long ua, unsigned long entries,mem->entries=entries;*pmem=mem;-list_add_rcu(&mem->next,¤t->mm->context.iommu_group_mem_list);+list_add_rcu(&mem->next,&mm->context.iommu_group_mem_list);unlock_exit:if(locked_entries&&ret)-mm_iommu_adjust_locked_vm(current->mm,locked_entries,false);+mm_iommu_adjust_locked_vm(mm,locked_entries,false);mutex_unlock(&mem_list_mutex);
AFAICT, you've moved this call from _release() to _put(). Won't that cause a
behavioural change?
mm_iommu_put() calls mm_iommu_adjust_locked_vm() right after
m_iommu_release() so no, it does not look so.
Ah, I guess not. It seems a bit arbitrary in the context of the rest
of the changes, though.
quoted
quoted
call_rcu(&mem->rcu, mm_iommu_free);
}
-long mm_iommu_put(struct mm_iommu_table_group_mem_t *mem)
+long mm_iommu_put(struct mm_struct *mm, struct mm_iommu_table_group_mem_t *mem)
{
long ret = 0;
- if (!current || !current->mm)
- return -ESRCH; /* process exited */
mutex_lock(&mem_list_mutex);
@@ -224,6 +215,8 @@ long mm_iommu_put(struct mm_iommu_table_group_mem_t *mem) /* @mapped became 0 so now mappings are disabled, release the region */ mm_iommu_release(mem);+ mm_iommu_adjust_locked_vm(mm, mem->entries, false);+ unlock_exit: mutex_unlock(&mem_list_mutex);
@@ -110,11 +111,11 @@ static long tce_iommu_unregister_pages(struct tce_container *container,if((vaddr&~PAGE_MASK)||(size&~PAGE_MASK))return-EINVAL;-mem=mm_iommu_find(vaddr,size>>PAGE_SHIFT);+mem=mm_iommu_find(container->mm,vaddr,size>>PAGE_SHIFT);if(!mem)return-ENOENT;-returnmm_iommu_put(mem);+returnmm_iommu_put(container->mm,mem);}staticlongtce_iommu_register_pages(structtce_container*container,
@@ -128,10 +129,17 @@ static long tce_iommu_register_pages(struct tce_container *container,((vaddr+size)<vaddr))return-EINVAL;-r<et=mm_iommu_get(vaddr,entries,&mem);+if(!container->mm){+if(!current->mm)+return-ESRCH;/* process exited */
Can this ever happen? Surely the ioctl() path shouldn't be called
after the process mm has been cleaned up? i.e. should this be a
WARN_ON().
Not sure with SMP (one thread doing ioctl(), another - exiting QEMU) if it
is not that impossible but it is quite hard to trigger this check.
I'm pretty sure the mm can't be cleaned up until all threads have
definitely stopped executing.
quoted
quoted
+
+ atomic_inc(¤t->mm->mm_count);
What balances this atomic_inc()? Is it the mmdrop() added to
tce_iommu_release()?
Yes. Surprisingly there is no mmget(), there is mmget_not_zero() but it is
for mm->mm_users.
Ok.
quoted
quoted
+ container->mm = current->mm;
+ }
Surely you need an error (or else a BUG_ON()) if current->mm !=
container->mm != NULL. I believe VFIO already assumes the container
is owned only by a single mm, but it looks like you should verify that here.
I am not sure I really want to enforce it, do I? Who knows what kind of a
crazy person would create a container, pin pages and fork() that userspace
tool which may not be QEMU but something custom using DPDK or something.
What harm can not having this BUG_ON() cause?
Hrm. Well, if nothing else it lets one process lock (or unlock) pages
in an essentially unrelated process, which is pretty weird. I don't
see any obvious way it will cause more serious problems. But, as a
general rule it makes debugging easier if you check / enforce required
assumptions at the earliest possible point.
quoted
quoted
+
+ ret = mm_iommu_get(container->mm, vaddr, entries, &mem);
if (ret)
return ret;
-
container->enabled = true;
return 0;
@@ -369,13 +379,14 @@ static void tce_iommu_unuse_page(struct tce_container *container, put_page(page); }-static int tce_iommu_prereg_ua_to_hpa(unsigned long tce, unsigned long size,+static int tce_iommu_prereg_ua_to_hpa(struct tce_container *container,+ unsigned long tce, unsigned long size, unsigned long *phpa, struct mm_iommu_table_group_mem_t **pmem) { long ret = 0; struct mm_iommu_table_group_mem_t *mem;- mem = mm_iommu_lookup(tce, size);+ mem = mm_iommu_lookup(container->mm, tce, size); if (!mem) return -EINVAL;
@@ -388,18 +399,18 @@ static int tce_iommu_prereg_ua_to_hpa(unsigned long tce, unsigned long size, return 0; }-static void tce_iommu_unuse_page_v2(struct iommu_table *tbl,- unsigned long entry)+static void tce_iommu_unuse_page_v2(struct tce_container *container,+ struct iommu_table *tbl, unsigned long entry) { struct mm_iommu_table_group_mem_t *mem = NULL; int ret; unsigned long hpa = 0; unsigned long *pua = IOMMU_TABLE_USERSPACE_ENTRY(tbl, entry);- if (!pua || !current || !current->mm)+ if (!pua) return;- ret = tce_iommu_prereg_ua_to_hpa(*pua, IOMMU_PAGE_SIZE(tbl),+ ret = tce_iommu_prereg_ua_to_hpa(container, *pua, IOMMU_PAGE_SIZE(tbl), &hpa, &mem); if (ret) pr_debug("%s: tce %lx at #%lx was not cached, ret=%d\n",
@@ -429,7 +440,7 @@ static int tce_iommu_clear(struct tce_container *container, continue; if (container->v2) {- tce_iommu_unuse_page_v2(tbl, entry);+ tce_iommu_unuse_page_v2(container, tbl, entry); continue; }
@@ -514,8 +525,8 @@ static long tce_iommu_build_v2(struct tce_container *container, unsigned long *pua = IOMMU_TABLE_USERSPACE_ENTRY(tbl, entry + i);- ret = tce_iommu_prereg_ua_to_hpa(tce, IOMMU_PAGE_SIZE(tbl),- &hpa, &mem);+ ret = tce_iommu_prereg_ua_to_hpa(container,+ tce, IOMMU_PAGE_SIZE(tbl), &hpa, &mem); if (ret) break;
@@ -536,7 +547,7 @@ static long tce_iommu_build_v2(struct tce_container *container, ret = iommu_tce_xchg(tbl, entry + i, &hpa, &dirtmp); if (ret) { /* dirtmp cannot be DMA_NONE here */- tce_iommu_unuse_page_v2(tbl, entry + i);+ tce_iommu_unuse_page_v2(container, tbl, entry + i); pr_err("iommu_tce: %s failed ioba=%lx, tce=%lx, ret=%ld\n", __func__, entry << tbl->it_page_shift, tce, 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: Alex Williamson <hidden> Date: 2016-08-15 15:32:31
On Mon, 15 Aug 2016 13:59:47 +1000
Paul Mackerras [off-list ref] wrote:
On Tue, Aug 09, 2016 at 06:16:30AM -0600, Alex Williamson wrote:
quoted
On Tue, 9 Aug 2016 15:19:39 +1000
Alexey Kardashevskiy [off-list ref] wrote:
quoted
On 09/08/16 02:43, Alex Williamson wrote:
quoted
I think you need to take a closer look of the lifecycle of a container,
having a reference means the container itself won't go away, but only
having a group set within that container holds the actual IOMMU
references. container->iommu_data is going to be NULL once the
groups are lost. Thanks,
Container owns the iommu tables and this is what I care about here, groups
attached or not - this is handled separately via IOMMU group list in a
specific iommu_table struct, these groups get detached from iommu_table
when they are removed from a container.
The container doesn't own anything, the container is privileged by the
groups being attached to it. When groups are closed, they detach from
the container and once the container group list is empty the iommu
backend is released and iommu_data is NULL. A container reference
doesn't give you what you're looking for. It implies nothing about the
iommu backend.
Alex, I'd like to understand more what the objection is here - is it
just about the object lifetimes, or is it a more fundamental objection
to the style of interface?
Regarding lifetimes, my understanding was that Alexey's previous
patches added refcounting to the iommu tables, so that KVM could get a
reference to the iommu tables through the container and then safely
use the iommu tables directly. There may still be a potential race in
the interval between asking the container about its iommu tables and
incrementing the tables' reference counts, but that should be able to
be solved. I don't see any unsolvable problem regarding lifetimes.
Or is your objection about any external access to the container?
As far as I know, when a group is not part of a container it has its
own iommu tables, but when it is put in a container it loses its own
iommu tables and instead uses a common pair of iommu tables (one for
the 32-bit window, one for the 64-bit window) that belong to the
container. So we do in fact need the container's iommu tables not the
individual groups' tables.
Hi Paul,
Have you looked at this? The ends do not justify the means. First off
we're trying to create an external user interface to get and put a
reference to a container for the purpose of getting a reference to
iommu data. So you might expect that that reference actually maintains
that iommu data, right? Wrong. The container is just the gateway
through which we access the iommu, a reference to the container doesn't
actually include a reference to the iommu backing it. The user can
unset and reconstitute a new iommu state any time they want to and it's
actually the groups that privilege the container to have an iommu state
at all, so removal of groups automatically de-privileges the container
and the reference to the state is lost. So the reference we're
creating a meaningless for the intended context.
Furthermore, why are we trying to get this reference? Alexey wants to
add an interface that allows an _external_ user to get the _opaque_,
_private_ data structure for the iommu backend. Are bells and whistles
going off in your head yet? Without any validation of what iommu
backend is running we pass that void* to a function that casts it as a
struct tce_container and starts iterating through it. This is
horribly, horribly wrong. Thanks,
Alex
From: David Gibson <hidden> Date: 2016-08-17 06:00:05
On Fri, Aug 12, 2016 at 09:22:01AM -0600, Alex Williamson wrote:
On Fri, 12 Aug 2016 15:46:01 +1000
David Gibson [off-list ref] wrote:
quoted
On Wed, Aug 10, 2016 at 10:46:30AM -0600, Alex Williamson wrote:
quoted
On Wed, 10 Aug 2016 15:37:17 +1000
Alexey Kardashevskiy [off-list ref] wrote:
quoted
On 09/08/16 22:16, Alex Williamson wrote:
quoted
On Tue, 9 Aug 2016 15:19:39 +1000
Alexey Kardashevskiy [off-list ref] wrote:
quoted
On 09/08/16 02:43, Alex Williamson wrote:
quoted
On Wed, 3 Aug 2016 18:40:55 +1000
Alexey Kardashevskiy [off-list ref] wrote:
quoted
This exports helpers which are needed to keep a VFIO container in
memory while there are external users such as KVM.
Signed-off-by: Alexey Kardashevskiy <redacted>
---
drivers/vfio/vfio.c | 30 ++++++++++++++++++++++++++++++
drivers/vfio/vfio_iommu_spapr_tce.c | 16 +++++++++++++++-
include/linux/vfio.h | 6 ++++++
3 files changed, 51 insertions(+), 1 deletion(-)
I think you need to take a closer look of the lifecycle of a container,
having a reference means the container itself won't go away, but only
having a group set within that container holds the actual IOMMU
references. container->iommu_data is going to be NULL once the
groups are lost. Thanks,
Container owns the iommu tables and this is what I care about here, groups
attached or not - this is handled separately via IOMMU group list in a
specific iommu_table struct, these groups get detached from iommu_table
when they are removed from a container.
The container doesn't own anything, the container is privileged by the
groups being attached to it. When groups are closed, they detach from
the container and once the container group list is empty the iommu
backend is released and iommu_data is NULL. A container reference
doesn't give you what you're looking for. It implies nothing about the
iommu backend.
Well. Backend is a part of a container and since a backend owns tables, a
container owns them too.
The IOMMU backend is accessed through the container, but that backend
is privileged by the groups it contains. Once those groups are gone,
the IOMMU backend is released, regardless of whatever reference you
have to the container itself such as you're attempting to do here. In
that sense, the container does not own those tables.
So, the thing is that what KVM fundamentally needs is a handle on the
container. KVM is essentially modelling the DMA address space of a
single guest bus, and the container is what's attached to that.
The first part of the problem is that KVM wants to basically invoke
vfio_dma_map() operations without bouncing via qemu. Because
vfio_dma_map() works on the container level, that's the handle that
KVM needs to hold.
The second part of the problem is that in order to reduce overhead
further, we want to operate in real mode, which means bypassing most
of the usual VFIO structure and going directly(ish) from the KVM
hcall emulation to the IOMMU backend behind VFIO. This complicates
matters a fair bit. Because it is, explicitly, a performance hack,
some degree of ugliness is probably inevitable.
Alexey - actually implementing this in two stages might make this
clearer. The first stage wouldn't allow real mode, and would call
through the same vfio_dma_map() path as qemu calls through now. The
second stage would then put in place the necessary hacks to add real
mode support.
quoted
quoted
The problem I am trying to solve here is when KVM may release the
iommu_table objects.
"Set" ioctl() to KVM-spapr-tce-table (or KVM itself, does not really
matter) makes a link between KVM-spapr-tce-table and container and KVM can
start using tables (with referencing them).
First I tried adding an "unset" ioctl to KVM-spapr-tce-table, called it
from region_del() and this works if QEMU removes a window. However if QEMU
removes a vfio-pci device, region_del() is not called and KVM does not get
notified that it can release the iommu_table's because the
KVM-spapr-tce-table remains alive and does not get destroyed (as it is
still used by emulated devices or other containers).
So it was suggested that we could do such "unset" somehow later assuming,
for example, on every "set" I could check if some of currently attached
containers are no more used - and this is where being able to know if there
is no backend helps - KVM remembers a container pointer and can check this
via vfio_container_get_iommu_data_ext().
The other option would be changing vfio_container_get_ext() to take a
callback+opaque which container would call when it destroys iommu_data.
This looks more intrusive and not very intuitive how to make it right -
container would have to keep track of all registered external users and
vfio_container_put_ext() would have to pass the same callback+opaque to
unregister the exact external user.
I'm not in favor of anything resembling the code above or extensions
beyond it, the container is the wrong place to do this.
quoted
Or I could store container file* in KVM. Then iommu_data would never be
released until KVM-spapr-tce-table is destroyed.
See above, holding a file pointer to the container doesn't do squat.
The groups that are held by the container empower the IOMMU backend,
references to the container itself don't matter. Those references will
not maintain the IOMMU data.
quoted
Recreating KVM-spapr-tce-table on every vfio-pci hotunplug (closing its fd
would "unset" container from KVM-spapr-tce-table) is not an option as there
still may be devices using this KVM-spapr-tce-table.
What obvious and nice solution am I missing here? Thanks.
The interactions with the IOMMU backend that seem relevant are
vfio_iommu_drivers_ops.{detach_group,release}. The kvm-vfio pseudo
device is also used to tell kvm about groups as they come and go and
has a way to check extensions, and thus properties of the IOMMU
backend. All of these are available for your {ab}use. Thanks,
So, Alexey started trying to do this via the KVM-VFIO device, but it's
a really bad fit. As noted above, fundamentally it's a container we
need to attach to the kvm-spapr-tce-table object, since what that
represents is a guest bus DMA address space, and by definition all the
groups in a container must have the same DMA address space.
That's all fine and good, but the point remains that a reference to the
container is no assurance of the iommu state. The iommu state is
maintained by the user and the groups attached to the container. If
the groups are removed, your container reference no long has any iommu
backing and iommu_data is worthless. The user can do this as well by
un-setting the iommu. I understand what you're trying to do, it's just
wrong. Thanks,
I'm trying to figure out how to do this right, and it's not at all
obvious. The container may be wrong, but that doesn't have the
KVM-VFIO device any more useful. Attempting to do this at the group
level is at least as wrong for the reasons I've mentioned elsewhere.
--
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 Fri, Aug 12, 2016 at 04:12:17PM +1000, Alexey Kardashevskiy wrote:
quoted
On 12/08/16 15:46, David Gibson wrote:
quoted
On Wed, Aug 10, 2016 at 10:46:30AM -0600, Alex Williamson wrote:
quoted
On Wed, 10 Aug 2016 15:37:17 +1000
Alexey Kardashevskiy [off-list ref] wrote:
quoted
On 09/08/16 22:16, Alex Williamson wrote:
quoted
On Tue, 9 Aug 2016 15:19:39 +1000
Alexey Kardashevskiy [off-list ref] wrote:
quoted
On 09/08/16 02:43, Alex Williamson wrote:
quoted
On Wed, 3 Aug 2016 18:40:55 +1000
Alexey Kardashevskiy [off-list ref] wrote:
quoted
This exports helpers which are needed to keep a VFIO container in
memory while there are external users such as KVM.
Signed-off-by: Alexey Kardashevskiy <redacted>
---
drivers/vfio/vfio.c | 30 ++++++++++++++++++++++++++++++
drivers/vfio/vfio_iommu_spapr_tce.c | 16 +++++++++++++++-
include/linux/vfio.h | 6 ++++++
3 files changed, 51 insertions(+), 1 deletion(-)
I think you need to take a closer look of the lifecycle of a container,
having a reference means the container itself won't go away, but only
having a group set within that container holds the actual IOMMU
references. container->iommu_data is going to be NULL once the
groups are lost. Thanks,
Container owns the iommu tables and this is what I care about here, groups
attached or not - this is handled separately via IOMMU group list in a
specific iommu_table struct, these groups get detached from iommu_table
when they are removed from a container.
The container doesn't own anything, the container is privileged by the
groups being attached to it. When groups are closed, they detach from
the container and once the container group list is empty the iommu
backend is released and iommu_data is NULL. A container reference
doesn't give you what you're looking for. It implies nothing about the
iommu backend.
Well. Backend is a part of a container and since a backend owns tables, a
container owns them too.
The IOMMU backend is accessed through the container, but that backend
is privileged by the groups it contains. Once those groups are gone,
the IOMMU backend is released, regardless of whatever reference you
have to the container itself such as you're attempting to do here. In
that sense, the container does not own those tables.
So, the thing is that what KVM fundamentally needs is a handle on the
container. KVM is essentially modelling the DMA address space of a
single guest bus, and the container is what's attached to that.
The first part of the problem is that KVM wants to basically invoke
vfio_dma_map() operations without bouncing via qemu. Because
vfio_dma_map() works on the container level, that's the handle that
KVM needs to hold.
Well, I do not need to hold the reference to the container all the time, I
just need it to get to the IOMMU backend, get+reference an iommu_table from
it, referencing here helps to make sure the backend is not going away
before we reference iommu_table.
Yes, but I don't see a compelling reason *not* to hold the container
reference either - it seems like principle of least surprise would
suggest retaining the reference.
For example, I can imagine having a container reset call which threw
away the back end iommu table and created a new one. It seems like
what you'd expect in this case is for the guest bus to remain bound to
the same container, not to the now stale iommu table.
quoted
After that I only keep a reference to the container to know if/when I can
release a particular iommu_table. This is can workaround by counting how
many groups were attached to this particular KVM-spapt-tce-table and
looking at the IOMMU group list attached to an iommu_table - if the list is
empty, decrement the iommu_table reference counter and that's it, no extra
references to a VFIO container.
Or I need an alternative way of getting iommu_table's, i.e. QEMU should
somehow tell KVM that this LIOBN is this VFIO container fd (easy - can be
done via region_add/region_del interface)
Um.. yes.. that's what I was expecting, I thought that was what you
were doing.x
quoted
or VFIO IOMMU group fd(s) (more
tricky as this needs to be done from more places - vfio-pci hotplug/unplug,
window add/remove).
More tricky and also wrong. Again, having one group but not the whole
container bound to the guest LIOBN doesn't make any sense - by
definition, all the devices in the container should share the same DMA
address space.
quoted
quoted
The second part of the problem is that in order to reduce overhead
further, we want to operate in real mode, which means bypassing most
of the usual VFIO structure and going directly(ish) from the KVM
hcall emulation to the IOMMU backend behind VFIO. This complicates
matters a fair bit. Because it is, explicitly, a performance hack,
some degree of ugliness is probably inevitable.
Alexey - actually implementing this in two stages might make this
clearer. The first stage wouldn't allow real mode, and would call
through the same vfio_dma_map() path as qemu calls through now. The
second stage would then put in place the necessary hacks to add real
mode support.
quoted
quoted
The problem I am trying to solve here is when KVM may release the
iommu_table objects.
"Set" ioctl() to KVM-spapr-tce-table (or KVM itself, does not really
matter) makes a link between KVM-spapr-tce-table and container and KVM can
start using tables (with referencing them).
First I tried adding an "unset" ioctl to KVM-spapr-tce-table, called it
from region_del() and this works if QEMU removes a window. However if QEMU
removes a vfio-pci device, region_del() is not called and KVM does not get
notified that it can release the iommu_table's because the
KVM-spapr-tce-table remains alive and does not get destroyed (as it is
still used by emulated devices or other containers).
So it was suggested that we could do such "unset" somehow later assuming,
for example, on every "set" I could check if some of currently attached
containers are no more used - and this is where being able to know if there
is no backend helps - KVM remembers a container pointer and can check this
via vfio_container_get_iommu_data_ext().
The other option would be changing vfio_container_get_ext() to take a
callback+opaque which container would call when it destroys iommu_data.
This looks more intrusive and not very intuitive how to make it right -
container would have to keep track of all registered external users and
vfio_container_put_ext() would have to pass the same callback+opaque to
unregister the exact external user.
I'm not in favor of anything resembling the code above or extensions
beyond it, the container is the wrong place to do this.
quoted
Or I could store container file* in KVM. Then iommu_data would never be
released until KVM-spapr-tce-table is destroyed.
See above, holding a file pointer to the container doesn't do squat.
The groups that are held by the container empower the IOMMU backend,
references to the container itself don't matter. Those references will
not maintain the IOMMU data.
quoted
Recreating KVM-spapr-tce-table on every vfio-pci hotunplug (closing its fd
would "unset" container from KVM-spapr-tce-table) is not an option as there
still may be devices using this KVM-spapr-tce-table.
What obvious and nice solution am I missing here? Thanks.
The interactions with the IOMMU backend that seem relevant are
vfio_iommu_drivers_ops.{detach_group,release}. The kvm-vfio pseudo
device is also used to tell kvm about groups as they come and go and
has a way to check extensions, and thus properties of the IOMMU
backend. All of these are available for your {ab}use. Thanks,
So, Alexey started trying to do this via the KVM-VFIO device, but it's
a really bad fit. As noted above, fundamentally it's a container we
need to attach to the kvm-spapr-tce-table object, since what that
represents is a guest bus DMA address space, and by definition all the
groups in a container must have the same DMA address space.
Well, in a bad case a LIOBN/kvm-spapr-tce-table has multiple containers
attached so it is not 1:1...
I never said it was. It's n:1, but it's *not* n:m. You can have
multiple containers to a LIOBN, but never multiple LIOBNs ot a
container.
Just to clarify things - there are 2 LIOBNs (1 per window) per a container
actually - 32bit one and 64bit one :)
--
Alexey
On Fri, Aug 12, 2016 at 09:22:01AM -0600, Alex Williamson wrote:
quoted
On Fri, 12 Aug 2016 15:46:01 +1000
David Gibson [off-list ref] wrote:
quoted
On Wed, Aug 10, 2016 at 10:46:30AM -0600, Alex Williamson wrote:
quoted
On Wed, 10 Aug 2016 15:37:17 +1000
Alexey Kardashevskiy [off-list ref] wrote:
quoted
On 09/08/16 22:16, Alex Williamson wrote:
quoted
On Tue, 9 Aug 2016 15:19:39 +1000
Alexey Kardashevskiy [off-list ref] wrote:
quoted
On 09/08/16 02:43, Alex Williamson wrote:
quoted
On Wed, 3 Aug 2016 18:40:55 +1000
Alexey Kardashevskiy [off-list ref] wrote:
quoted
This exports helpers which are needed to keep a VFIO container in
memory while there are external users such as KVM.
Signed-off-by: Alexey Kardashevskiy <redacted>
---
drivers/vfio/vfio.c | 30 ++++++++++++++++++++++++++++++
drivers/vfio/vfio_iommu_spapr_tce.c | 16 +++++++++++++++-
include/linux/vfio.h | 6 ++++++
3 files changed, 51 insertions(+), 1 deletion(-)
I think you need to take a closer look of the lifecycle of a container,
having a reference means the container itself won't go away, but only
having a group set within that container holds the actual IOMMU
references. container->iommu_data is going to be NULL once the
groups are lost. Thanks,
Container owns the iommu tables and this is what I care about here, groups
attached or not - this is handled separately via IOMMU group list in a
specific iommu_table struct, these groups get detached from iommu_table
when they are removed from a container.
The container doesn't own anything, the container is privileged by the
groups being attached to it. When groups are closed, they detach from
the container and once the container group list is empty the iommu
backend is released and iommu_data is NULL. A container reference
doesn't give you what you're looking for. It implies nothing about the
iommu backend.
Well. Backend is a part of a container and since a backend owns tables, a
container owns them too.
The IOMMU backend is accessed through the container, but that backend
is privileged by the groups it contains. Once those groups are gone,
the IOMMU backend is released, regardless of whatever reference you
have to the container itself such as you're attempting to do here. In
that sense, the container does not own those tables.
So, the thing is that what KVM fundamentally needs is a handle on the
container. KVM is essentially modelling the DMA address space of a
single guest bus, and the container is what's attached to that.
The first part of the problem is that KVM wants to basically invoke
vfio_dma_map() operations without bouncing via qemu. Because
vfio_dma_map() works on the container level, that's the handle that
KVM needs to hold.
The second part of the problem is that in order to reduce overhead
further, we want to operate in real mode, which means bypassing most
of the usual VFIO structure and going directly(ish) from the KVM
hcall emulation to the IOMMU backend behind VFIO. This complicates
matters a fair bit. Because it is, explicitly, a performance hack,
some degree of ugliness is probably inevitable.
Alexey - actually implementing this in two stages might make this
clearer. The first stage wouldn't allow real mode, and would call
through the same vfio_dma_map() path as qemu calls through now. The
second stage would then put in place the necessary hacks to add real
mode support.
quoted
quoted
The problem I am trying to solve here is when KVM may release the
iommu_table objects.
"Set" ioctl() to KVM-spapr-tce-table (or KVM itself, does not really
matter) makes a link between KVM-spapr-tce-table and container and KVM can
start using tables (with referencing them).
First I tried adding an "unset" ioctl to KVM-spapr-tce-table, called it
from region_del() and this works if QEMU removes a window. However if QEMU
removes a vfio-pci device, region_del() is not called and KVM does not get
notified that it can release the iommu_table's because the
KVM-spapr-tce-table remains alive and does not get destroyed (as it is
still used by emulated devices or other containers).
So it was suggested that we could do such "unset" somehow later assuming,
for example, on every "set" I could check if some of currently attached
containers are no more used - and this is where being able to know if there
is no backend helps - KVM remembers a container pointer and can check this
via vfio_container_get_iommu_data_ext().
The other option would be changing vfio_container_get_ext() to take a
callback+opaque which container would call when it destroys iommu_data.
This looks more intrusive and not very intuitive how to make it right -
container would have to keep track of all registered external users and
vfio_container_put_ext() would have to pass the same callback+opaque to
unregister the exact external user.
I'm not in favor of anything resembling the code above or extensions
beyond it, the container is the wrong place to do this.
quoted
Or I could store container file* in KVM. Then iommu_data would never be
released until KVM-spapr-tce-table is destroyed.
See above, holding a file pointer to the container doesn't do squat.
The groups that are held by the container empower the IOMMU backend,
references to the container itself don't matter. Those references will
not maintain the IOMMU data.
quoted
Recreating KVM-spapr-tce-table on every vfio-pci hotunplug (closing its fd
would "unset" container from KVM-spapr-tce-table) is not an option as there
still may be devices using this KVM-spapr-tce-table.
What obvious and nice solution am I missing here? Thanks.
The interactions with the IOMMU backend that seem relevant are
vfio_iommu_drivers_ops.{detach_group,release}. The kvm-vfio pseudo
device is also used to tell kvm about groups as they come and go and
has a way to check extensions, and thus properties of the IOMMU
backend. All of these are available for your {ab}use. Thanks,
So, Alexey started trying to do this via the KVM-VFIO device, but it's
a really bad fit. As noted above, fundamentally it's a container we
need to attach to the kvm-spapr-tce-table object, since what that
represents is a guest bus DMA address space, and by definition all the
groups in a container must have the same DMA address space.
That's all fine and good, but the point remains that a reference to the
container is no assurance of the iommu state. The iommu state is
maintained by the user and the groups attached to the container. If
the groups are removed, your container reference no long has any iommu
backing and iommu_data is worthless. The user can do this as well by
un-setting the iommu. I understand what you're trying to do, it's just
wrong. Thanks,
I'm trying to figure out how to do this right, and it's not at all
obvious. The container may be wrong, but that doesn't have the
KVM-VFIO device any more useful. Attempting to do this at the group
level is at least as wrong for the reasons I've mentioned elsewhere.
I could create a new fd, one per iommu_table, the fd would reference the
iommu_table (not touching an iommu_table_group or a container), VFIO SPAPR
TCE backend would return it in VFIO_IOMMU_SPAPR_TCE_CREATE (ioctl which
creates windows) or I could add VFIO_IOMMU_SPAPR_TCE_GET_FD_BY_OFFSET; then
I'd pass this new fd to the KVM or KVM-spapr-tce-table to hook them up. To
release the reference, KVM-spapr-tce-table would have "unset" ioctl()
or/and on every "set" I would look if all attached tables have at least one
iommu_table_group attached, if none - release the table.
This would make no change to generic VFIO code and very little change in
SPAPR TCE backend. Would that be acceptable or it is horrible again? Thanks.
--
Alexey
On Fri, Aug 12, 2016 at 09:22:01AM -0600, Alex Williamson wrote:
quoted
On Fri, 12 Aug 2016 15:46:01 +1000
David Gibson [off-list ref] wrote:
quoted
On Wed, Aug 10, 2016 at 10:46:30AM -0600, Alex Williamson wrote:
quoted
On Wed, 10 Aug 2016 15:37:17 +1000
Alexey Kardashevskiy [off-list ref] wrote:
quoted
On 09/08/16 22:16, Alex Williamson wrote:
quoted
On Tue, 9 Aug 2016 15:19:39 +1000
Alexey Kardashevskiy [off-list ref] wrote:
quoted
On 09/08/16 02:43, Alex Williamson wrote:
quoted
On Wed, 3 Aug 2016 18:40:55 +1000
Alexey Kardashevskiy [off-list ref] wrote:
quoted
This exports helpers which are needed to keep a VFIO container in
memory while there are external users such as KVM.
Signed-off-by: Alexey Kardashevskiy <redacted>
---
drivers/vfio/vfio.c | 30 ++++++++++++++++++++++++++++++
drivers/vfio/vfio_iommu_spapr_tce.c | 16 +++++++++++++++-
include/linux/vfio.h | 6 ++++++
3 files changed, 51 insertions(+), 1 deletion(-)
I think you need to take a closer look of the lifecycle of a container,
having a reference means the container itself won't go away, but only
having a group set within that container holds the actual IOMMU
references. container->iommu_data is going to be NULL once the
groups are lost. Thanks,
Container owns the iommu tables and this is what I care about here, groups
attached or not - this is handled separately via IOMMU group list in a
specific iommu_table struct, these groups get detached from iommu_table
when they are removed from a container.
The container doesn't own anything, the container is privileged by the
groups being attached to it. When groups are closed, they detach from
the container and once the container group list is empty the iommu
backend is released and iommu_data is NULL. A container reference
doesn't give you what you're looking for. It implies nothing about the
iommu backend.
Well. Backend is a part of a container and since a backend owns tables, a
container owns them too.
The IOMMU backend is accessed through the container, but that backend
is privileged by the groups it contains. Once those groups are gone,
the IOMMU backend is released, regardless of whatever reference you
have to the container itself such as you're attempting to do here. In
that sense, the container does not own those tables.
So, the thing is that what KVM fundamentally needs is a handle on the
container. KVM is essentially modelling the DMA address space of a
single guest bus, and the container is what's attached to that.
The first part of the problem is that KVM wants to basically invoke
vfio_dma_map() operations without bouncing via qemu. Because
vfio_dma_map() works on the container level, that's the handle that
KVM needs to hold.
The second part of the problem is that in order to reduce overhead
further, we want to operate in real mode, which means bypassing most
of the usual VFIO structure and going directly(ish) from the KVM
hcall emulation to the IOMMU backend behind VFIO. This complicates
matters a fair bit. Because it is, explicitly, a performance hack,
some degree of ugliness is probably inevitable.
Alexey - actually implementing this in two stages might make this
clearer. The first stage wouldn't allow real mode, and would call
through the same vfio_dma_map() path as qemu calls through now. The
second stage would then put in place the necessary hacks to add real
mode support.
quoted
quoted
The problem I am trying to solve here is when KVM may release the
iommu_table objects.
"Set" ioctl() to KVM-spapr-tce-table (or KVM itself, does not really
matter) makes a link between KVM-spapr-tce-table and container and KVM can
start using tables (with referencing them).
First I tried adding an "unset" ioctl to KVM-spapr-tce-table, called it
from region_del() and this works if QEMU removes a window. However if QEMU
removes a vfio-pci device, region_del() is not called and KVM does not get
notified that it can release the iommu_table's because the
KVM-spapr-tce-table remains alive and does not get destroyed (as it is
still used by emulated devices or other containers).
So it was suggested that we could do such "unset" somehow later assuming,
for example, on every "set" I could check if some of currently attached
containers are no more used - and this is where being able to know if there
is no backend helps - KVM remembers a container pointer and can check this
via vfio_container_get_iommu_data_ext().
The other option would be changing vfio_container_get_ext() to take a
callback+opaque which container would call when it destroys iommu_data.
This looks more intrusive and not very intuitive how to make it right -
container would have to keep track of all registered external users and
vfio_container_put_ext() would have to pass the same callback+opaque to
unregister the exact external user.
I'm not in favor of anything resembling the code above or extensions
beyond it, the container is the wrong place to do this.
quoted
Or I could store container file* in KVM. Then iommu_data would never be
released until KVM-spapr-tce-table is destroyed.
See above, holding a file pointer to the container doesn't do squat.
The groups that are held by the container empower the IOMMU backend,
references to the container itself don't matter. Those references will
not maintain the IOMMU data.
quoted
Recreating KVM-spapr-tce-table on every vfio-pci hotunplug (closing its fd
would "unset" container from KVM-spapr-tce-table) is not an option as there
still may be devices using this KVM-spapr-tce-table.
What obvious and nice solution am I missing here? Thanks.
The interactions with the IOMMU backend that seem relevant are
vfio_iommu_drivers_ops.{detach_group,release}. The kvm-vfio pseudo
device is also used to tell kvm about groups as they come and go and
has a way to check extensions, and thus properties of the IOMMU
backend. All of these are available for your {ab}use. Thanks,
So, Alexey started trying to do this via the KVM-VFIO device, but it's
a really bad fit. As noted above, fundamentally it's a container we
need to attach to the kvm-spapr-tce-table object, since what that
represents is a guest bus DMA address space, and by definition all the
groups in a container must have the same DMA address space.
That's all fine and good, but the point remains that a reference to the
container is no assurance of the iommu state. The iommu state is
maintained by the user and the groups attached to the container. If
the groups are removed, your container reference no long has any iommu
backing and iommu_data is worthless. The user can do this as well by
un-setting the iommu. I understand what you're trying to do, it's just
wrong. Thanks,
I'm trying to figure out how to do this right, and it's not at all
obvious. The container may be wrong, but that doesn't have the
KVM-VFIO device any more useful. Attempting to do this at the group
level is at least as wrong for the reasons I've mentioned elsewhere.
I could create a new fd, one per iommu_table, the fd would reference the
iommu_table (not touching an iommu_table_group or a container), VFIO SPAPR
TCE backend would return it in VFIO_IOMMU_SPAPR_TCE_CREATE (ioctl which
creates windows) or I could add VFIO_IOMMU_SPAPR_TCE_GET_FD_BY_OFFSET; then
I'd pass this new fd to the KVM or KVM-spapr-tce-table to hook them up. To
release the reference, KVM-spapr-tce-table would have "unset" ioctl()
or/and on every "set" I would look if all attached tables have at least one
iommu_table_group attached, if none - release the table.
This would make no change to generic VFIO code and very little change in
SPAPR TCE backend. Would that be acceptable or it is horrible again? Thanks.
From: David Gibson <hidden> Date: 2016-08-29 13:29:50
On Mon, Aug 29, 2016 at 04:35:15PM +1000, Alexey Kardashevskiy wrote:
On 18/08/16 10:22, Alexey Kardashevskiy wrote:
quoted
On 17/08/16 13:17, David Gibson wrote:
quoted
On Fri, Aug 12, 2016 at 09:22:01AM -0600, Alex Williamson wrote:
quoted
On Fri, 12 Aug 2016 15:46:01 +1000
David Gibson [off-list ref] wrote:
quoted
On Wed, Aug 10, 2016 at 10:46:30AM -0600, Alex Williamson wrote:
quoted
On Wed, 10 Aug 2016 15:37:17 +1000
Alexey Kardashevskiy [off-list ref] wrote:
quoted
On 09/08/16 22:16, Alex Williamson wrote:
quoted
On Tue, 9 Aug 2016 15:19:39 +1000
Alexey Kardashevskiy [off-list ref] wrote:
quoted
On 09/08/16 02:43, Alex Williamson wrote:
quoted
On Wed, 3 Aug 2016 18:40:55 +1000
Alexey Kardashevskiy [off-list ref] wrote:
quoted
This exports helpers which are needed to keep a VFIO container in
memory while there are external users such as KVM.
Signed-off-by: Alexey Kardashevskiy <redacted>
---
drivers/vfio/vfio.c | 30 ++++++++++++++++++++++++++++++
drivers/vfio/vfio_iommu_spapr_tce.c | 16 +++++++++++++++-
include/linux/vfio.h | 6 ++++++
3 files changed, 51 insertions(+), 1 deletion(-)
I think you need to take a closer look of the lifecycle of a container,
having a reference means the container itself won't go away, but only
having a group set within that container holds the actual IOMMU
references. container->iommu_data is going to be NULL once the
groups are lost. Thanks,
Container owns the iommu tables and this is what I care about here, groups
attached or not - this is handled separately via IOMMU group list in a
specific iommu_table struct, these groups get detached from iommu_table
when they are removed from a container.
The container doesn't own anything, the container is privileged by the
groups being attached to it. When groups are closed, they detach from
the container and once the container group list is empty the iommu
backend is released and iommu_data is NULL. A container reference
doesn't give you what you're looking for. It implies nothing about the
iommu backend.
Well. Backend is a part of a container and since a backend owns tables, a
container owns them too.
The IOMMU backend is accessed through the container, but that backend
is privileged by the groups it contains. Once those groups are gone,
the IOMMU backend is released, regardless of whatever reference you
have to the container itself such as you're attempting to do here. In
that sense, the container does not own those tables.
So, the thing is that what KVM fundamentally needs is a handle on the
container. KVM is essentially modelling the DMA address space of a
single guest bus, and the container is what's attached to that.
The first part of the problem is that KVM wants to basically invoke
vfio_dma_map() operations without bouncing via qemu. Because
vfio_dma_map() works on the container level, that's the handle that
KVM needs to hold.
The second part of the problem is that in order to reduce overhead
further, we want to operate in real mode, which means bypassing most
of the usual VFIO structure and going directly(ish) from the KVM
hcall emulation to the IOMMU backend behind VFIO. This complicates
matters a fair bit. Because it is, explicitly, a performance hack,
some degree of ugliness is probably inevitable.
Alexey - actually implementing this in two stages might make this
clearer. The first stage wouldn't allow real mode, and would call
through the same vfio_dma_map() path as qemu calls through now. The
second stage would then put in place the necessary hacks to add real
mode support.
quoted
quoted
The problem I am trying to solve here is when KVM may release the
iommu_table objects.
"Set" ioctl() to KVM-spapr-tce-table (or KVM itself, does not really
matter) makes a link between KVM-spapr-tce-table and container and KVM can
start using tables (with referencing them).
First I tried adding an "unset" ioctl to KVM-spapr-tce-table, called it
from region_del() and this works if QEMU removes a window. However if QEMU
removes a vfio-pci device, region_del() is not called and KVM does not get
notified that it can release the iommu_table's because the
KVM-spapr-tce-table remains alive and does not get destroyed (as it is
still used by emulated devices or other containers).
So it was suggested that we could do such "unset" somehow later assuming,
for example, on every "set" I could check if some of currently attached
containers are no more used - and this is where being able to know if there
is no backend helps - KVM remembers a container pointer and can check this
via vfio_container_get_iommu_data_ext().
The other option would be changing vfio_container_get_ext() to take a
callback+opaque which container would call when it destroys iommu_data.
This looks more intrusive and not very intuitive how to make it right -
container would have to keep track of all registered external users and
vfio_container_put_ext() would have to pass the same callback+opaque to
unregister the exact external user.
I'm not in favor of anything resembling the code above or extensions
beyond it, the container is the wrong place to do this.
quoted
Or I could store container file* in KVM. Then iommu_data would never be
released until KVM-spapr-tce-table is destroyed.
See above, holding a file pointer to the container doesn't do squat.
The groups that are held by the container empower the IOMMU backend,
references to the container itself don't matter. Those references will
not maintain the IOMMU data.
quoted
Recreating KVM-spapr-tce-table on every vfio-pci hotunplug (closing its fd
would "unset" container from KVM-spapr-tce-table) is not an option as there
still may be devices using this KVM-spapr-tce-table.
What obvious and nice solution am I missing here? Thanks.
The interactions with the IOMMU backend that seem relevant are
vfio_iommu_drivers_ops.{detach_group,release}. The kvm-vfio pseudo
device is also used to tell kvm about groups as they come and go and
has a way to check extensions, and thus properties of the IOMMU
backend. All of these are available for your {ab}use. Thanks,
So, Alexey started trying to do this via the KVM-VFIO device, but it's
a really bad fit. As noted above, fundamentally it's a container we
need to attach to the kvm-spapr-tce-table object, since what that
represents is a guest bus DMA address space, and by definition all the
groups in a container must have the same DMA address space.
That's all fine and good, but the point remains that a reference to the
container is no assurance of the iommu state. The iommu state is
maintained by the user and the groups attached to the container. If
the groups are removed, your container reference no long has any iommu
backing and iommu_data is worthless. The user can do this as well by
un-setting the iommu. I understand what you're trying to do, it's just
wrong. Thanks,
I'm trying to figure out how to do this right, and it's not at all
obvious. The container may be wrong, but that doesn't have the
KVM-VFIO device any more useful. Attempting to do this at the group
level is at least as wrong for the reasons I've mentioned elsewhere.
I could create a new fd, one per iommu_table, the fd would reference the
iommu_table (not touching an iommu_table_group or a container), VFIO SPAPR
TCE backend would return it in VFIO_IOMMU_SPAPR_TCE_CREATE (ioctl which
creates windows) or I could add VFIO_IOMMU_SPAPR_TCE_GET_FD_BY_OFFSET; then
I'd pass this new fd to the KVM or KVM-spapr-tce-table to hook them up. To
release the reference, KVM-spapr-tce-table would have "unset" ioctl()
or/and on every "set" I would look if all attached tables have at least one
iommu_table_group attached, if none - release the table.
This would make no change to generic VFIO code and very little change in
SPAPR TCE backend. Would that be acceptable or it is horrible again? Thanks.
Ping?
I'm still in Toronto after KVM Forum. I had a detailed discussion
about this with Alex W, which I'll write up once I get back.
The short version is that Alex more-or-less convinced me that we do
need to go back to doing this with an interface based on linking
groups to LIOBNs. That leads to an interface that's kind of weird and
has some fairly counter-intuitive properties, but in the end it works
out better than doing it with containers.
--
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, Aug 29, 2016 at 04:35:15PM +1000, Alexey Kardashevskiy wrote:
quoted
On 18/08/16 10:22, Alexey Kardashevskiy wrote:
quoted
On 17/08/16 13:17, David Gibson wrote:
quoted
On Fri, Aug 12, 2016 at 09:22:01AM -0600, Alex Williamson wrote:
quoted
On Fri, 12 Aug 2016 15:46:01 +1000
David Gibson [off-list ref] wrote:
quoted
On Wed, Aug 10, 2016 at 10:46:30AM -0600, Alex Williamson wrote:
quoted
On Wed, 10 Aug 2016 15:37:17 +1000
Alexey Kardashevskiy [off-list ref] wrote:
quoted
On 09/08/16 22:16, Alex Williamson wrote:
quoted
On Tue, 9 Aug 2016 15:19:39 +1000
Alexey Kardashevskiy [off-list ref] wrote:
quoted
On 09/08/16 02:43, Alex Williamson wrote:
quoted
On Wed, 3 Aug 2016 18:40:55 +1000
Alexey Kardashevskiy [off-list ref] wrote:
quoted
This exports helpers which are needed to keep a VFIO container in
memory while there are external users such as KVM.
Signed-off-by: Alexey Kardashevskiy <redacted>
---
drivers/vfio/vfio.c | 30 ++++++++++++++++++++++++++++++
drivers/vfio/vfio_iommu_spapr_tce.c | 16 +++++++++++++++-
include/linux/vfio.h | 6 ++++++
3 files changed, 51 insertions(+), 1 deletion(-)
I think you need to take a closer look of the lifecycle of a container,
having a reference means the container itself won't go away, but only
having a group set within that container holds the actual IOMMU
references. container->iommu_data is going to be NULL once the
groups are lost. Thanks,
Container owns the iommu tables and this is what I care about here, groups
attached or not - this is handled separately via IOMMU group list in a
specific iommu_table struct, these groups get detached from iommu_table
when they are removed from a container.
The container doesn't own anything, the container is privileged by the
groups being attached to it. When groups are closed, they detach from
the container and once the container group list is empty the iommu
backend is released and iommu_data is NULL. A container reference
doesn't give you what you're looking for. It implies nothing about the
iommu backend.
Well. Backend is a part of a container and since a backend owns tables, a
container owns them too.
The IOMMU backend is accessed through the container, but that backend
is privileged by the groups it contains. Once those groups are gone,
the IOMMU backend is released, regardless of whatever reference you
have to the container itself such as you're attempting to do here. In
that sense, the container does not own those tables.
So, the thing is that what KVM fundamentally needs is a handle on the
container. KVM is essentially modelling the DMA address space of a
single guest bus, and the container is what's attached to that.
The first part of the problem is that KVM wants to basically invoke
vfio_dma_map() operations without bouncing via qemu. Because
vfio_dma_map() works on the container level, that's the handle that
KVM needs to hold.
The second part of the problem is that in order to reduce overhead
further, we want to operate in real mode, which means bypassing most
of the usual VFIO structure and going directly(ish) from the KVM
hcall emulation to the IOMMU backend behind VFIO. This complicates
matters a fair bit. Because it is, explicitly, a performance hack,
some degree of ugliness is probably inevitable.
Alexey - actually implementing this in two stages might make this
clearer. The first stage wouldn't allow real mode, and would call
through the same vfio_dma_map() path as qemu calls through now. The
second stage would then put in place the necessary hacks to add real
mode support.
quoted
quoted
The problem I am trying to solve here is when KVM may release the
iommu_table objects.
"Set" ioctl() to KVM-spapr-tce-table (or KVM itself, does not really
matter) makes a link between KVM-spapr-tce-table and container and KVM can
start using tables (with referencing them).
First I tried adding an "unset" ioctl to KVM-spapr-tce-table, called it
from region_del() and this works if QEMU removes a window. However if QEMU
removes a vfio-pci device, region_del() is not called and KVM does not get
notified that it can release the iommu_table's because the
KVM-spapr-tce-table remains alive and does not get destroyed (as it is
still used by emulated devices or other containers).
So it was suggested that we could do such "unset" somehow later assuming,
for example, on every "set" I could check if some of currently attached
containers are no more used - and this is where being able to know if there
is no backend helps - KVM remembers a container pointer and can check this
via vfio_container_get_iommu_data_ext().
The other option would be changing vfio_container_get_ext() to take a
callback+opaque which container would call when it destroys iommu_data.
This looks more intrusive and not very intuitive how to make it right -
container would have to keep track of all registered external users and
vfio_container_put_ext() would have to pass the same callback+opaque to
unregister the exact external user.
I'm not in favor of anything resembling the code above or extensions
beyond it, the container is the wrong place to do this.
quoted
Or I could store container file* in KVM. Then iommu_data would never be
released until KVM-spapr-tce-table is destroyed.
See above, holding a file pointer to the container doesn't do squat.
The groups that are held by the container empower the IOMMU backend,
references to the container itself don't matter. Those references will
not maintain the IOMMU data.
quoted
Recreating KVM-spapr-tce-table on every vfio-pci hotunplug (closing its fd
would "unset" container from KVM-spapr-tce-table) is not an option as there
still may be devices using this KVM-spapr-tce-table.
What obvious and nice solution am I missing here? Thanks.
The interactions with the IOMMU backend that seem relevant are
vfio_iommu_drivers_ops.{detach_group,release}. The kvm-vfio pseudo
device is also used to tell kvm about groups as they come and go and
has a way to check extensions, and thus properties of the IOMMU
backend. All of these are available for your {ab}use. Thanks,
So, Alexey started trying to do this via the KVM-VFIO device, but it's
a really bad fit. As noted above, fundamentally it's a container we
need to attach to the kvm-spapr-tce-table object, since what that
represents is a guest bus DMA address space, and by definition all the
groups in a container must have the same DMA address space.
That's all fine and good, but the point remains that a reference to the
container is no assurance of the iommu state. The iommu state is
maintained by the user and the groups attached to the container. If
the groups are removed, your container reference no long has any iommu
backing and iommu_data is worthless. The user can do this as well by
un-setting the iommu. I understand what you're trying to do, it's just
wrong. Thanks,
I'm trying to figure out how to do this right, and it's not at all
obvious. The container may be wrong, but that doesn't have the
KVM-VFIO device any more useful. Attempting to do this at the group
level is at least as wrong for the reasons I've mentioned elsewhere.
I could create a new fd, one per iommu_table, the fd would reference the
iommu_table (not touching an iommu_table_group or a container), VFIO SPAPR
TCE backend would return it in VFIO_IOMMU_SPAPR_TCE_CREATE (ioctl which
creates windows) or I could add VFIO_IOMMU_SPAPR_TCE_GET_FD_BY_OFFSET; then
I'd pass this new fd to the KVM or KVM-spapr-tce-table to hook them up. To
release the reference, KVM-spapr-tce-table would have "unset" ioctl()
or/and on every "set" I would look if all attached tables have at least one
iommu_table_group attached, if none - release the table.
This would make no change to generic VFIO code and very little change in
SPAPR TCE backend. Would that be acceptable or it is horrible again? Thanks.
Ping?
I'm still in Toronto after KVM Forum. I had a detailed discussion
about this with Alex W, which I'll write up once I get back.
The short version is that Alex more-or-less convinced me that we do
need to go back to doing this with an interface based on linking
groups to LIOBNs. That leads to an interface that's kind of weird and
has some fairly counter-intuitive properties, but in the end it works
out better than doing it with containers.
On Mon, Aug 29, 2016 at 04:35:15PM +1000, Alexey Kardashevskiy wrote:
quoted
On 18/08/16 10:22, Alexey Kardashevskiy wrote:
quoted
On 17/08/16 13:17, David Gibson wrote:
quoted
On Fri, Aug 12, 2016 at 09:22:01AM -0600, Alex Williamson wrote:
quoted
On Fri, 12 Aug 2016 15:46:01 +1000
David Gibson [off-list ref] wrote:
quoted
On Wed, Aug 10, 2016 at 10:46:30AM -0600, Alex Williamson wrote:
quoted
On Wed, 10 Aug 2016 15:37:17 +1000
Alexey Kardashevskiy [off-list ref] wrote:
quoted
On 09/08/16 22:16, Alex Williamson wrote:
quoted
On Tue, 9 Aug 2016 15:19:39 +1000
Alexey Kardashevskiy [off-list ref] wrote:
quoted
On 09/08/16 02:43, Alex Williamson wrote:
quoted
On Wed, 3 Aug 2016 18:40:55 +1000
Alexey Kardashevskiy [off-list ref] wrote:
quoted
This exports helpers which are needed to keep a VFIO container in
memory while there are external users such as KVM.
Signed-off-by: Alexey Kardashevskiy <redacted>
---
drivers/vfio/vfio.c | 30 ++++++++++++++++++++++++++++++
drivers/vfio/vfio_iommu_spapr_tce.c | 16 +++++++++++++++-
include/linux/vfio.h | 6 ++++++
3 files changed, 51 insertions(+), 1 deletion(-)
I think you need to take a closer look of the lifecycle of a container,
having a reference means the container itself won't go away, but only
having a group set within that container holds the actual IOMMU
references. container->iommu_data is going to be NULL once the
groups are lost. Thanks,
Container owns the iommu tables and this is what I care about here, groups
attached or not - this is handled separately via IOMMU group list in a
specific iommu_table struct, these groups get detached from iommu_table
when they are removed from a container.
The container doesn't own anything, the container is privileged by the
groups being attached to it. When groups are closed, they detach from
the container and once the container group list is empty the iommu
backend is released and iommu_data is NULL. A container reference
doesn't give you what you're looking for. It implies nothing about the
iommu backend.
Well. Backend is a part of a container and since a backend owns tables, a
container owns them too.
The IOMMU backend is accessed through the container, but that backend
is privileged by the groups it contains. Once those groups are gone,
the IOMMU backend is released, regardless of whatever reference you
have to the container itself such as you're attempting to do here. In
that sense, the container does not own those tables.
So, the thing is that what KVM fundamentally needs is a handle on the
container. KVM is essentially modelling the DMA address space of a
single guest bus, and the container is what's attached to that.
The first part of the problem is that KVM wants to basically invoke
vfio_dma_map() operations without bouncing via qemu. Because
vfio_dma_map() works on the container level, that's the handle that
KVM needs to hold.
The second part of the problem is that in order to reduce overhead
further, we want to operate in real mode, which means bypassing most
of the usual VFIO structure and going directly(ish) from the KVM
hcall emulation to the IOMMU backend behind VFIO. This complicates
matters a fair bit. Because it is, explicitly, a performance hack,
some degree of ugliness is probably inevitable.
Alexey - actually implementing this in two stages might make this
clearer. The first stage wouldn't allow real mode, and would call
through the same vfio_dma_map() path as qemu calls through now. The
second stage would then put in place the necessary hacks to add real
mode support.
quoted
quoted
The problem I am trying to solve here is when KVM may release the
iommu_table objects.
"Set" ioctl() to KVM-spapr-tce-table (or KVM itself, does not really
matter) makes a link between KVM-spapr-tce-table and container and KVM can
start using tables (with referencing them).
First I tried adding an "unset" ioctl to KVM-spapr-tce-table, called it
from region_del() and this works if QEMU removes a window. However if QEMU
removes a vfio-pci device, region_del() is not called and KVM does not get
notified that it can release the iommu_table's because the
KVM-spapr-tce-table remains alive and does not get destroyed (as it is
still used by emulated devices or other containers).
So it was suggested that we could do such "unset" somehow later assuming,
for example, on every "set" I could check if some of currently attached
containers are no more used - and this is where being able to know if there
is no backend helps - KVM remembers a container pointer and can check this
via vfio_container_get_iommu_data_ext().
The other option would be changing vfio_container_get_ext() to take a
callback+opaque which container would call when it destroys iommu_data.
This looks more intrusive and not very intuitive how to make it right -
container would have to keep track of all registered external users and
vfio_container_put_ext() would have to pass the same callback+opaque to
unregister the exact external user.
I'm not in favor of anything resembling the code above or extensions
beyond it, the container is the wrong place to do this.
quoted
Or I could store container file* in KVM. Then iommu_data would never be
released until KVM-spapr-tce-table is destroyed.
See above, holding a file pointer to the container doesn't do squat.
The groups that are held by the container empower the IOMMU backend,
references to the container itself don't matter. Those references will
not maintain the IOMMU data.
quoted
Recreating KVM-spapr-tce-table on every vfio-pci hotunplug (closing its fd
would "unset" container from KVM-spapr-tce-table) is not an option as there
still may be devices using this KVM-spapr-tce-table.
What obvious and nice solution am I missing here? Thanks.
The interactions with the IOMMU backend that seem relevant are
vfio_iommu_drivers_ops.{detach_group,release}. The kvm-vfio pseudo
device is also used to tell kvm about groups as they come and go and
has a way to check extensions, and thus properties of the IOMMU
backend. All of these are available for your {ab}use. Thanks,
So, Alexey started trying to do this via the KVM-VFIO device, but it's
a really bad fit. As noted above, fundamentally it's a container we
need to attach to the kvm-spapr-tce-table object, since what that
represents is a guest bus DMA address space, and by definition all the
groups in a container must have the same DMA address space.
That's all fine and good, but the point remains that a reference to the
container is no assurance of the iommu state. The iommu state is
maintained by the user and the groups attached to the container. If
the groups are removed, your container reference no long has any iommu
backing and iommu_data is worthless. The user can do this as well by
un-setting the iommu. I understand what you're trying to do, it's just
wrong. Thanks,
I'm trying to figure out how to do this right, and it's not at all
obvious. The container may be wrong, but that doesn't have the
KVM-VFIO device any more useful. Attempting to do this at the group
level is at least as wrong for the reasons I've mentioned elsewhere.
I could create a new fd, one per iommu_table, the fd would reference the
iommu_table (not touching an iommu_table_group or a container), VFIO SPAPR
TCE backend would return it in VFIO_IOMMU_SPAPR_TCE_CREATE (ioctl which
creates windows) or I could add VFIO_IOMMU_SPAPR_TCE_GET_FD_BY_OFFSET; then
I'd pass this new fd to the KVM or KVM-spapr-tce-table to hook them up. To
release the reference, KVM-spapr-tce-table would have "unset" ioctl()
or/and on every "set" I would look if all attached tables have at least one
iommu_table_group attached, if none - release the table.
This would make no change to generic VFIO code and very little change in
SPAPR TCE backend. Would that be acceptable or it is horrible again? Thanks.
Ping?
I'm still in Toronto after KVM Forum. I had a detailed discussion
about this with Alex W, which I'll write up once I get back.
The short version is that Alex more-or-less convinced me that we do
need to go back to doing this with an interface based on linking
groups to LIOBNs. That leads to an interface that's kind of weird and
has some fairly counter-intuitive properties, but in the end it works
out better than doing it with containers.
Soooo? :)
When can I expect a full version of how to do this in-kernel thingy? Thanks.
--
Alexey
From: David Gibson <hidden> Date: 2016-09-23 07:12:14
On Wed, Sep 21, 2016 at 04:56:52PM +1000, Alexey Kardashevskiy wrote:
On 07/09/16 19:09, Alexey Kardashevskiy wrote:
quoted
On 29/08/16 23:27, David Gibson wrote:
quoted
On Mon, Aug 29, 2016 at 04:35:15PM +1000, Alexey Kardashevskiy wrote:
quoted
On 18/08/16 10:22, Alexey Kardashevskiy wrote:
quoted
On 17/08/16 13:17, David Gibson wrote:
quoted
On Fri, Aug 12, 2016 at 09:22:01AM -0600, Alex Williamson wrote:
quoted
On Fri, 12 Aug 2016 15:46:01 +1000
David Gibson [off-list ref] wrote:
quoted
On Wed, Aug 10, 2016 at 10:46:30AM -0600, Alex Williamson wrote:
quoted
On Wed, 10 Aug 2016 15:37:17 +1000
Alexey Kardashevskiy [off-list ref] wrote:
quoted
On 09/08/16 22:16, Alex Williamson wrote:
quoted
On Tue, 9 Aug 2016 15:19:39 +1000
Alexey Kardashevskiy [off-list ref] wrote:
quoted
On 09/08/16 02:43, Alex Williamson wrote:
quoted
On Wed, 3 Aug 2016 18:40:55 +1000
Alexey Kardashevskiy [off-list ref] wrote:
quoted
This exports helpers which are needed to keep a VFIO container in
memory while there are external users such as KVM.
Signed-off-by: Alexey Kardashevskiy <redacted>
---
drivers/vfio/vfio.c | 30 ++++++++++++++++++++++++++++++
drivers/vfio/vfio_iommu_spapr_tce.c | 16 +++++++++++++++-
include/linux/vfio.h | 6 ++++++
3 files changed, 51 insertions(+), 1 deletion(-)
I think you need to take a closer look of the lifecycle of a container,
having a reference means the container itself won't go away, but only
having a group set within that container holds the actual IOMMU
references. container->iommu_data is going to be NULL once the
groups are lost. Thanks,
Container owns the iommu tables and this is what I care about here, groups
attached or not - this is handled separately via IOMMU group list in a
specific iommu_table struct, these groups get detached from iommu_table
when they are removed from a container.
The container doesn't own anything, the container is privileged by the
groups being attached to it. When groups are closed, they detach from
the container and once the container group list is empty the iommu
backend is released and iommu_data is NULL. A container reference
doesn't give you what you're looking for. It implies nothing about the
iommu backend.
Well. Backend is a part of a container and since a backend owns tables, a
container owns them too.
The IOMMU backend is accessed through the container, but that backend
is privileged by the groups it contains. Once those groups are gone,
the IOMMU backend is released, regardless of whatever reference you
have to the container itself such as you're attempting to do here. In
that sense, the container does not own those tables.
So, the thing is that what KVM fundamentally needs is a handle on the
container. KVM is essentially modelling the DMA address space of a
single guest bus, and the container is what's attached to that.
The first part of the problem is that KVM wants to basically invoke
vfio_dma_map() operations without bouncing via qemu. Because
vfio_dma_map() works on the container level, that's the handle that
KVM needs to hold.
The second part of the problem is that in order to reduce overhead
further, we want to operate in real mode, which means bypassing most
of the usual VFIO structure and going directly(ish) from the KVM
hcall emulation to the IOMMU backend behind VFIO. This complicates
matters a fair bit. Because it is, explicitly, a performance hack,
some degree of ugliness is probably inevitable.
Alexey - actually implementing this in two stages might make this
clearer. The first stage wouldn't allow real mode, and would call
through the same vfio_dma_map() path as qemu calls through now. The
second stage would then put in place the necessary hacks to add real
mode support.
quoted
quoted
The problem I am trying to solve here is when KVM may release the
iommu_table objects.
"Set" ioctl() to KVM-spapr-tce-table (or KVM itself, does not really
matter) makes a link between KVM-spapr-tce-table and container and KVM can
start using tables (with referencing them).
First I tried adding an "unset" ioctl to KVM-spapr-tce-table, called it
from region_del() and this works if QEMU removes a window. However if QEMU
removes a vfio-pci device, region_del() is not called and KVM does not get
notified that it can release the iommu_table's because the
KVM-spapr-tce-table remains alive and does not get destroyed (as it is
still used by emulated devices or other containers).
So it was suggested that we could do such "unset" somehow later assuming,
for example, on every "set" I could check if some of currently attached
containers are no more used - and this is where being able to know if there
is no backend helps - KVM remembers a container pointer and can check this
via vfio_container_get_iommu_data_ext().
The other option would be changing vfio_container_get_ext() to take a
callback+opaque which container would call when it destroys iommu_data.
This looks more intrusive and not very intuitive how to make it right -
container would have to keep track of all registered external users and
vfio_container_put_ext() would have to pass the same callback+opaque to
unregister the exact external user.
I'm not in favor of anything resembling the code above or extensions
beyond it, the container is the wrong place to do this.
quoted
Or I could store container file* in KVM. Then iommu_data would never be
released until KVM-spapr-tce-table is destroyed.
See above, holding a file pointer to the container doesn't do squat.
The groups that are held by the container empower the IOMMU backend,
references to the container itself don't matter. Those references will
not maintain the IOMMU data.
quoted
Recreating KVM-spapr-tce-table on every vfio-pci hotunplug (closing its fd
would "unset" container from KVM-spapr-tce-table) is not an option as there
still may be devices using this KVM-spapr-tce-table.
What obvious and nice solution am I missing here? Thanks.
The interactions with the IOMMU backend that seem relevant are
vfio_iommu_drivers_ops.{detach_group,release}. The kvm-vfio pseudo
device is also used to tell kvm about groups as they come and go and
has a way to check extensions, and thus properties of the IOMMU
backend. All of these are available for your {ab}use. Thanks,
So, Alexey started trying to do this via the KVM-VFIO device, but it's
a really bad fit. As noted above, fundamentally it's a container we
need to attach to the kvm-spapr-tce-table object, since what that
represents is a guest bus DMA address space, and by definition all the
groups in a container must have the same DMA address space.
That's all fine and good, but the point remains that a reference to the
container is no assurance of the iommu state. The iommu state is
maintained by the user and the groups attached to the container. If
the groups are removed, your container reference no long has any iommu
backing and iommu_data is worthless. The user can do this as well by
un-setting the iommu. I understand what you're trying to do, it's just
wrong. Thanks,
I'm trying to figure out how to do this right, and it's not at all
obvious. The container may be wrong, but that doesn't have the
KVM-VFIO device any more useful. Attempting to do this at the group
level is at least as wrong for the reasons I've mentioned elsewhere.
I could create a new fd, one per iommu_table, the fd would reference the
iommu_table (not touching an iommu_table_group or a container), VFIO SPAPR
TCE backend would return it in VFIO_IOMMU_SPAPR_TCE_CREATE (ioctl which
creates windows) or I could add VFIO_IOMMU_SPAPR_TCE_GET_FD_BY_OFFSET; then
I'd pass this new fd to the KVM or KVM-spapr-tce-table to hook them up. To
release the reference, KVM-spapr-tce-table would have "unset" ioctl()
or/and on every "set" I would look if all attached tables have at least one
iommu_table_group attached, if none - release the table.
This would make no change to generic VFIO code and very little change in
SPAPR TCE backend. Would that be acceptable or it is horrible again? Thanks.
Ping?
I'm still in Toronto after KVM Forum. I had a detailed discussion
about this with Alex W, which I'll write up once I get back.
The short version is that Alex more-or-less convinced me that we do
need to go back to doing this with an interface based on linking
groups to LIOBNs. That leads to an interface that's kind of weird and
has some fairly counter-intuitive properties, but in the end it works
out better than doing it with containers.
Soooo? :)
When can I expect a full version of how to do this in-kernel thingy?
Thanks.
When I can dig myself out from under other things in my queue. Which
turns out to be now.
Ok.. here's hoping I can remember enough of the conclusions I came to
with Alex W.
User <-> KVM interface
----------------------
This needs to take an LIOBN and a group fd and associate (or
disassociate) them. This should be possible to do by adding each
group to the vfio-kvm device as on x86, then setting an attribute on
the device to mark the associated liobn.
Attaching different (overlapping) LIOBNs to different groups in the
same container is boundedly undefined (i.e. it mustn't break the host,
but can do anything to the guest).
KVM <-> VFIO (in kernel) interface
----------------------------------
You'll need a special function which takes a vfio group fd and returns
a reference to an iommu_table object. It would also return an error
if the group isn't backed by the spapr_tce iommu driver (including any
calls to it on a non-ppc host). This should probably also increment
the iommu table's ref count (on success).
Implementation notes
--------------------
When a device in a new group is hotplugged, qemu would need to add the
group to the container *then* tell KVM to attach the group to the
correct liobn(s).
KVM would add the group to a list for that liobn. It would call the
vfio hook to get the associated iommu table. If there's an error,
then it's unable to enable acceleration, and would either return an
error immediately or ensure that later attempts to PUT_TCE will be
punted to qemu.
Assuming it is able to accelerate, it would add the iommu table to a
list of iommu tables associated with the liobn. It will need to
de-dupe here, since with multiple groups per container you'd expect
multiple groups with the same iommu table.
H_PUT_TCE would walk the list of attached iommu tables and update them
using the ppc kernel iommu interfaces.
When a group is removed from a liobn, kvm would need to recalculate
the list of iommu tables, in case that was the last group attached to
the table. It would need to decrement the refcount on the iommu table
and, obviously, make sure everything is sychronized with the real mode
PUT_TCE.
When a group is hot unplugged, it's qemu's resposibility to tell kvm
that the group is no longer associated with the liobn, before it
removes the group from the container. If it doesn't there may be a
stale iommu table attached to the liobn. That could certainly mess up
DMA on the guest for other devices, but shouldn't damage the host -
the group now belongs to the host again, but because the group was
detached from the container, the HW is no longer using the container's
iommu table (which KVM is touching) to actually serve the group.
If all the groups are unplugged, so the container becomes quiescent,
KVM's refcount(s) on the iommu table stop it going away. It won't be
looked at by the hardware any more, so updates will be useless, but
again that's only a problem for the guest, not the host.
Hope that covers it.
Alex, please let me know if I missed something from our discussion.
--
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
So far I got one question, below.
On 23/09/16 17:12, David Gibson wrote:
On Wed, Sep 21, 2016 at 04:56:52PM +1000, Alexey Kardashevskiy wrote:
quoted
On 07/09/16 19:09, Alexey Kardashevskiy wrote:
quoted
On 29/08/16 23:27, David Gibson wrote:
quoted
On Mon, Aug 29, 2016 at 04:35:15PM +1000, Alexey Kardashevskiy wrote:
quoted
On 18/08/16 10:22, Alexey Kardashevskiy wrote:
quoted
On 17/08/16 13:17, David Gibson wrote:
quoted
On Fri, Aug 12, 2016 at 09:22:01AM -0600, Alex Williamson wrote:
quoted
On Fri, 12 Aug 2016 15:46:01 +1000
David Gibson [off-list ref] wrote:
quoted
On Wed, Aug 10, 2016 at 10:46:30AM -0600, Alex Williamson wrote:
quoted
On Wed, 10 Aug 2016 15:37:17 +1000
Alexey Kardashevskiy [off-list ref] wrote:
quoted
On 09/08/16 22:16, Alex Williamson wrote:
quoted
On Tue, 9 Aug 2016 15:19:39 +1000
Alexey Kardashevskiy [off-list ref] wrote:
quoted
On 09/08/16 02:43, Alex Williamson wrote:
quoted
On Wed, 3 Aug 2016 18:40:55 +1000
Alexey Kardashevskiy [off-list ref] wrote:
quoted
This exports helpers which are needed to keep a VFIO container in
memory while there are external users such as KVM.
Signed-off-by: Alexey Kardashevskiy <redacted>
---
drivers/vfio/vfio.c | 30 ++++++++++++++++++++++++++++++
drivers/vfio/vfio_iommu_spapr_tce.c | 16 +++++++++++++++-
include/linux/vfio.h | 6 ++++++
3 files changed, 51 insertions(+), 1 deletion(-)
I think you need to take a closer look of the lifecycle of a container,
having a reference means the container itself won't go away, but only
having a group set within that container holds the actual IOMMU
references. container->iommu_data is going to be NULL once the
groups are lost. Thanks,
Container owns the iommu tables and this is what I care about here, groups
attached or not - this is handled separately via IOMMU group list in a
specific iommu_table struct, these groups get detached from iommu_table
when they are removed from a container.
The container doesn't own anything, the container is privileged by the
groups being attached to it. When groups are closed, they detach from
the container and once the container group list is empty the iommu
backend is released and iommu_data is NULL. A container reference
doesn't give you what you're looking for. It implies nothing about the
iommu backend.
Well. Backend is a part of a container and since a backend owns tables, a
container owns them too.
The IOMMU backend is accessed through the container, but that backend
is privileged by the groups it contains. Once those groups are gone,
the IOMMU backend is released, regardless of whatever reference you
have to the container itself such as you're attempting to do here. In
that sense, the container does not own those tables.
So, the thing is that what KVM fundamentally needs is a handle on the
container. KVM is essentially modelling the DMA address space of a
single guest bus, and the container is what's attached to that.
The first part of the problem is that KVM wants to basically invoke
vfio_dma_map() operations without bouncing via qemu. Because
vfio_dma_map() works on the container level, that's the handle that
KVM needs to hold.
The second part of the problem is that in order to reduce overhead
further, we want to operate in real mode, which means bypassing most
of the usual VFIO structure and going directly(ish) from the KVM
hcall emulation to the IOMMU backend behind VFIO. This complicates
matters a fair bit. Because it is, explicitly, a performance hack,
some degree of ugliness is probably inevitable.
Alexey - actually implementing this in two stages might make this
clearer. The first stage wouldn't allow real mode, and would call
through the same vfio_dma_map() path as qemu calls through now. The
second stage would then put in place the necessary hacks to add real
mode support.
quoted
quoted
The problem I am trying to solve here is when KVM may release the
iommu_table objects.
"Set" ioctl() to KVM-spapr-tce-table (or KVM itself, does not really
matter) makes a link between KVM-spapr-tce-table and container and KVM can
start using tables (with referencing them).
First I tried adding an "unset" ioctl to KVM-spapr-tce-table, called it
from region_del() and this works if QEMU removes a window. However if QEMU
removes a vfio-pci device, region_del() is not called and KVM does not get
notified that it can release the iommu_table's because the
KVM-spapr-tce-table remains alive and does not get destroyed (as it is
still used by emulated devices or other containers).
So it was suggested that we could do such "unset" somehow later assuming,
for example, on every "set" I could check if some of currently attached
containers are no more used - and this is where being able to know if there
is no backend helps - KVM remembers a container pointer and can check this
via vfio_container_get_iommu_data_ext().
The other option would be changing vfio_container_get_ext() to take a
callback+opaque which container would call when it destroys iommu_data.
This looks more intrusive and not very intuitive how to make it right -
container would have to keep track of all registered external users and
vfio_container_put_ext() would have to pass the same callback+opaque to
unregister the exact external user.
I'm not in favor of anything resembling the code above or extensions
beyond it, the container is the wrong place to do this.
quoted
Or I could store container file* in KVM. Then iommu_data would never be
released until KVM-spapr-tce-table is destroyed.
See above, holding a file pointer to the container doesn't do squat.
The groups that are held by the container empower the IOMMU backend,
references to the container itself don't matter. Those references will
not maintain the IOMMU data.
quoted
Recreating KVM-spapr-tce-table on every vfio-pci hotunplug (closing its fd
would "unset" container from KVM-spapr-tce-table) is not an option as there
still may be devices using this KVM-spapr-tce-table.
What obvious and nice solution am I missing here? Thanks.
The interactions with the IOMMU backend that seem relevant are
vfio_iommu_drivers_ops.{detach_group,release}. The kvm-vfio pseudo
device is also used to tell kvm about groups as they come and go and
has a way to check extensions, and thus properties of the IOMMU
backend. All of these are available for your {ab}use. Thanks,
So, Alexey started trying to do this via the KVM-VFIO device, but it's
a really bad fit. As noted above, fundamentally it's a container we
need to attach to the kvm-spapr-tce-table object, since what that
represents is a guest bus DMA address space, and by definition all the
groups in a container must have the same DMA address space.
That's all fine and good, but the point remains that a reference to the
container is no assurance of the iommu state. The iommu state is
maintained by the user and the groups attached to the container. If
the groups are removed, your container reference no long has any iommu
backing and iommu_data is worthless. The user can do this as well by
un-setting the iommu. I understand what you're trying to do, it's just
wrong. Thanks,
I'm trying to figure out how to do this right, and it's not at all
obvious. The container may be wrong, but that doesn't have the
KVM-VFIO device any more useful. Attempting to do this at the group
level is at least as wrong for the reasons I've mentioned elsewhere.
I could create a new fd, one per iommu_table, the fd would reference the
iommu_table (not touching an iommu_table_group or a container), VFIO SPAPR
TCE backend would return it in VFIO_IOMMU_SPAPR_TCE_CREATE (ioctl which
creates windows) or I could add VFIO_IOMMU_SPAPR_TCE_GET_FD_BY_OFFSET; then
I'd pass this new fd to the KVM or KVM-spapr-tce-table to hook them up. To
release the reference, KVM-spapr-tce-table would have "unset" ioctl()
or/and on every "set" I would look if all attached tables have at least one
iommu_table_group attached, if none - release the table.
This would make no change to generic VFIO code and very little change in
SPAPR TCE backend. Would that be acceptable or it is horrible again? Thanks.
Ping?
I'm still in Toronto after KVM Forum. I had a detailed discussion
about this with Alex W, which I'll write up once I get back.
The short version is that Alex more-or-less convinced me that we do
need to go back to doing this with an interface based on linking
groups to LIOBNs. That leads to an interface that's kind of weird and
has some fairly counter-intuitive properties, but in the end it works
out better than doing it with containers.
Soooo? :)
When can I expect a full version of how to do this in-kernel thingy?
Thanks.
When I can dig myself out from under other things in my queue. Which
turns out to be now.
Ok.. here's hoping I can remember enough of the conclusions I came to
with Alex W.
User <-> KVM interface
----------------------
This needs to take an LIOBN and a group fd and associate (or
disassociate) them. This should be possible to do by adding each
group to the vfio-kvm device as on x86, then setting an attribute on
the device to mark the associated liobn.
Attaching different (overlapping) LIOBNs to different groups in the
same container is boundedly undefined (i.e. it mustn't break the host,
but can do anything to the guest).
KVM <-> VFIO (in kernel) interface
----------------------------------
You'll need a special function which takes a vfio group fd and returns
a reference to an iommu_table object. It would also return an error
if the group isn't backed by the spapr_tce iommu driver (including any
calls to it on a non-ppc host). This should probably also increment
the iommu table's ref count (on success).
Implementation notes
--------------------
When a device in a new group is hotplugged, qemu would need to add the
group to the container *then* tell KVM to attach the group to the
correct liobn(s).
KVM would add the group to a list for that liobn. It would call the
vfio hook to get the associated iommu table. If there's an error,
then it's unable to enable acceleration, and would either return an
error immediately or ensure that later attempts to PUT_TCE will be
punted to qemu.
Assuming it is able to accelerate, it would add the iommu table to a
list of iommu tables associated with the liobn. It will need to
de-dupe here, since with multiple groups per container you'd expect
multiple groups with the same iommu table.
H_PUT_TCE would walk the list of attached iommu tables and update them
using the ppc kernel iommu interfaces.
When a group is removed from a liobn, kvm would need to recalculate
the list of iommu tables, in case that was the last group attached to
the table. It would need to decrement the refcount on the iommu table
and, obviously, make sure everything is sychronized with the real mode
PUT_TCE.
When a group is hot unplugged, it's qemu's resposibility to tell kvm
that the group is no longer associated with the liobn, before it
removes the group from the container.
Cannot VFIO KVM device just release this extra reference when QEMU calls
KVM_DEV_VFIO_GROUP_DEL from vfio_instance_finalize->vfio_put_group?
If it doesn't there may be a
stale iommu table attached to the liobn. That could certainly mess up
DMA on the guest for other devices, but shouldn't damage the host -
the group now belongs to the host again, but because the group was
detached from the container, the HW is no longer using the container's
iommu table (which KVM is touching) to actually serve the group.
If all the groups are unplugged, so the container becomes quiescent,
KVM's refcount(s) on the iommu table stop it going away. It won't be
looked at by the hardware any more, so updates will be useless, but
again that's only a problem for the guest, not the host.
Hope that covers it.
Alex, please let me know if I missed something from our discussion.
From: David Gibson <hidden> Date: 2016-10-18 01:46:43
On Mon, Oct 17, 2016 at 05:06:28PM +1100, Alexey Kardashevskiy wrote:
So far I got one question, below.
On 23/09/16 17:12, David Gibson wrote:
quoted
On Wed, Sep 21, 2016 at 04:56:52PM +1000, Alexey Kardashevskiy wrote:
quoted
On 07/09/16 19:09, Alexey Kardashevskiy wrote:
quoted
On 29/08/16 23:27, David Gibson wrote:
quoted
On Mon, Aug 29, 2016 at 04:35:15PM +1000, Alexey Kardashevskiy wrote:
quoted
On 18/08/16 10:22, Alexey Kardashevskiy wrote:
quoted
On 17/08/16 13:17, David Gibson wrote:
quoted
On Fri, Aug 12, 2016 at 09:22:01AM -0600, Alex Williamson wrote:
quoted
On Fri, 12 Aug 2016 15:46:01 +1000
David Gibson [off-list ref] wrote:
quoted
On Wed, Aug 10, 2016 at 10:46:30AM -0600, Alex Williamson wrote:
quoted
On Wed, 10 Aug 2016 15:37:17 +1000
Alexey Kardashevskiy [off-list ref] wrote:
quoted
On 09/08/16 22:16, Alex Williamson wrote:
quoted
On Tue, 9 Aug 2016 15:19:39 +1000
Alexey Kardashevskiy [off-list ref] wrote:
quoted
On 09/08/16 02:43, Alex Williamson wrote:
quoted
On Wed, 3 Aug 2016 18:40:55 +1000
Alexey Kardashevskiy [off-list ref] wrote:
quoted
This exports helpers which are needed to keep a VFIO container in
memory while there are external users such as KVM.
Signed-off-by: Alexey Kardashevskiy <redacted>
---
drivers/vfio/vfio.c | 30 ++++++++++++++++++++++++++++++
drivers/vfio/vfio_iommu_spapr_tce.c | 16 +++++++++++++++-
include/linux/vfio.h | 6 ++++++
3 files changed, 51 insertions(+), 1 deletion(-)
I think you need to take a closer look of the lifecycle of a container,
having a reference means the container itself won't go away, but only
having a group set within that container holds the actual IOMMU
references. container->iommu_data is going to be NULL once the
groups are lost. Thanks,
Container owns the iommu tables and this is what I care about here, groups
attached or not - this is handled separately via IOMMU group list in a
specific iommu_table struct, these groups get detached from iommu_table
when they are removed from a container.
The container doesn't own anything, the container is privileged by the
groups being attached to it. When groups are closed, they detach from
the container and once the container group list is empty the iommu
backend is released and iommu_data is NULL. A container reference
doesn't give you what you're looking for. It implies nothing about the
iommu backend.
Well. Backend is a part of a container and since a backend owns tables, a
container owns them too.
The IOMMU backend is accessed through the container, but that backend
is privileged by the groups it contains. Once those groups are gone,
the IOMMU backend is released, regardless of whatever reference you
have to the container itself such as you're attempting to do here. In
that sense, the container does not own those tables.
So, the thing is that what KVM fundamentally needs is a handle on the
container. KVM is essentially modelling the DMA address space of a
single guest bus, and the container is what's attached to that.
The first part of the problem is that KVM wants to basically invoke
vfio_dma_map() operations without bouncing via qemu. Because
vfio_dma_map() works on the container level, that's the handle that
KVM needs to hold.
The second part of the problem is that in order to reduce overhead
further, we want to operate in real mode, which means bypassing most
of the usual VFIO structure and going directly(ish) from the KVM
hcall emulation to the IOMMU backend behind VFIO. This complicates
matters a fair bit. Because it is, explicitly, a performance hack,
some degree of ugliness is probably inevitable.
Alexey - actually implementing this in two stages might make this
clearer. The first stage wouldn't allow real mode, and would call
through the same vfio_dma_map() path as qemu calls through now. The
second stage would then put in place the necessary hacks to add real
mode support.
quoted
quoted
The problem I am trying to solve here is when KVM may release the
iommu_table objects.
"Set" ioctl() to KVM-spapr-tce-table (or KVM itself, does not really
matter) makes a link between KVM-spapr-tce-table and container and KVM can
start using tables (with referencing them).
First I tried adding an "unset" ioctl to KVM-spapr-tce-table, called it
from region_del() and this works if QEMU removes a window. However if QEMU
removes a vfio-pci device, region_del() is not called and KVM does not get
notified that it can release the iommu_table's because the
KVM-spapr-tce-table remains alive and does not get destroyed (as it is
still used by emulated devices or other containers).
So it was suggested that we could do such "unset" somehow later assuming,
for example, on every "set" I could check if some of currently attached
containers are no more used - and this is where being able to know if there
is no backend helps - KVM remembers a container pointer and can check this
via vfio_container_get_iommu_data_ext().
The other option would be changing vfio_container_get_ext() to take a
callback+opaque which container would call when it destroys iommu_data.
This looks more intrusive and not very intuitive how to make it right -
container would have to keep track of all registered external users and
vfio_container_put_ext() would have to pass the same callback+opaque to
unregister the exact external user.
I'm not in favor of anything resembling the code above or extensions
beyond it, the container is the wrong place to do this.
quoted
Or I could store container file* in KVM. Then iommu_data would never be
released until KVM-spapr-tce-table is destroyed.
See above, holding a file pointer to the container doesn't do squat.
The groups that are held by the container empower the IOMMU backend,
references to the container itself don't matter. Those references will
not maintain the IOMMU data.
quoted
Recreating KVM-spapr-tce-table on every vfio-pci hotunplug (closing its fd
would "unset" container from KVM-spapr-tce-table) is not an option as there
still may be devices using this KVM-spapr-tce-table.
What obvious and nice solution am I missing here? Thanks.
The interactions with the IOMMU backend that seem relevant are
vfio_iommu_drivers_ops.{detach_group,release}. The kvm-vfio pseudo
device is also used to tell kvm about groups as they come and go and
has a way to check extensions, and thus properties of the IOMMU
backend. All of these are available for your {ab}use. Thanks,
So, Alexey started trying to do this via the KVM-VFIO device, but it's
a really bad fit. As noted above, fundamentally it's a container we
need to attach to the kvm-spapr-tce-table object, since what that
represents is a guest bus DMA address space, and by definition all the
groups in a container must have the same DMA address space.
That's all fine and good, but the point remains that a reference to the
container is no assurance of the iommu state. The iommu state is
maintained by the user and the groups attached to the container. If
the groups are removed, your container reference no long has any iommu
backing and iommu_data is worthless. The user can do this as well by
un-setting the iommu. I understand what you're trying to do, it's just
wrong. Thanks,
I'm trying to figure out how to do this right, and it's not at all
obvious. The container may be wrong, but that doesn't have the
KVM-VFIO device any more useful. Attempting to do this at the group
level is at least as wrong for the reasons I've mentioned elsewhere.
I could create a new fd, one per iommu_table, the fd would reference the
iommu_table (not touching an iommu_table_group or a container), VFIO SPAPR
TCE backend would return it in VFIO_IOMMU_SPAPR_TCE_CREATE (ioctl which
creates windows) or I could add VFIO_IOMMU_SPAPR_TCE_GET_FD_BY_OFFSET; then
I'd pass this new fd to the KVM or KVM-spapr-tce-table to hook them up. To
release the reference, KVM-spapr-tce-table would have "unset" ioctl()
or/and on every "set" I would look if all attached tables have at least one
iommu_table_group attached, if none - release the table.
This would make no change to generic VFIO code and very little change in
SPAPR TCE backend. Would that be acceptable or it is horrible again? Thanks.
Ping?
I'm still in Toronto after KVM Forum. I had a detailed discussion
about this with Alex W, which I'll write up once I get back.
The short version is that Alex more-or-less convinced me that we do
need to go back to doing this with an interface based on linking
groups to LIOBNs. That leads to an interface that's kind of weird and
has some fairly counter-intuitive properties, but in the end it works
out better than doing it with containers.
Soooo? :)
When can I expect a full version of how to do this in-kernel thingy?
Thanks.
When I can dig myself out from under other things in my queue. Which
turns out to be now.
Ok.. here's hoping I can remember enough of the conclusions I came to
with Alex W.
User <-> KVM interface
----------------------
This needs to take an LIOBN and a group fd and associate (or
disassociate) them. This should be possible to do by adding each
group to the vfio-kvm device as on x86, then setting an attribute on
the device to mark the associated liobn.
Attaching different (overlapping) LIOBNs to different groups in the
same container is boundedly undefined (i.e. it mustn't break the host,
but can do anything to the guest).
KVM <-> VFIO (in kernel) interface
----------------------------------
You'll need a special function which takes a vfio group fd and returns
a reference to an iommu_table object. It would also return an error
if the group isn't backed by the spapr_tce iommu driver (including any
calls to it on a non-ppc host). This should probably also increment
the iommu table's ref count (on success).
Implementation notes
--------------------
When a device in a new group is hotplugged, qemu would need to add the
group to the container *then* tell KVM to attach the group to the
correct liobn(s).
KVM would add the group to a list for that liobn. It would call the
vfio hook to get the associated iommu table. If there's an error,
then it's unable to enable acceleration, and would either return an
error immediately or ensure that later attempts to PUT_TCE will be
punted to qemu.
Assuming it is able to accelerate, it would add the iommu table to a
list of iommu tables associated with the liobn. It will need to
de-dupe here, since with multiple groups per container you'd expect
multiple groups with the same iommu table.
H_PUT_TCE would walk the list of attached iommu tables and update them
using the ppc kernel iommu interfaces.
When a group is removed from a liobn, kvm would need to recalculate
the list of iommu tables, in case that was the last group attached to
the table. It would need to decrement the refcount on the iommu table
and, obviously, make sure everything is sychronized with the real mode
PUT_TCE.
When a group is hot unplugged, it's qemu's resposibility to tell kvm
that the group is no longer associated with the liobn, before it
removes the group from the container.
Cannot VFIO KVM device just release this extra reference when QEMU calls
KVM_DEV_VFIO_GROUP_DEL from vfio_instance_finalize->vfio_put_group?
Yes, that should do it. That doesn't contradict the statement above,
it's just that we already have it - removing the group from the
vfio-kvm device is the mechanism by which qemu informs the kernel the
group is no longer associated with a liobn.
quoted
If it doesn't there may be a
stale iommu table attached to the liobn. That could certainly mess up
DMA on the guest for other devices, but shouldn't damage the host -
the group now belongs to the host again, but because the group was
detached from the container, the HW is no longer using the container's
iommu table (which KVM is touching) to actually serve the group.
If all the groups are unplugged, so the container becomes quiescent,
KVM's refcount(s) on the iommu table stop it going away. It won't be
looked at by the hardware any more, so updates will be useless, but
again that's only a problem for the guest, not the host.
Hope that covers it.
Alex, please let me know if I missed something from our discussion.
--
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