Re: [PATCH kernel v9 29/32] vfio: powerpc/spapr: Register memory and define IOMMU v2

[David Gibson <david@gibson.dropbear.id.au>]

[-- Attachment #1: Type: text/plain, Size: 14735 bytes --]

On Fri, May 01, 2015 at 04:27:47PM +1000, Alexey Kardashevskiy wrote:
> On 05/01/2015 03:23 PM, David Gibson wrote:
> >On Fri, May 01, 2015 at 02:35:23PM +1000, Alexey Kardashevskiy wrote:
> >>On 04/30/2015 04:55 PM, David Gibson wrote:
> >>>On Sat, Apr 25, 2015 at 10:14:53PM +1000, Alexey Kardashevskiy wrote:
> >>>>The existing implementation accounts the whole DMA window in
> >>>>the locked_vm counter. This is going to be worse with multiple
> >>>>containers and huge DMA windows. Also, real-time accounting would requite
> >>>>additional tracking of accounted pages due to the page size difference -
> >>>>IOMMU uses 4K pages and system uses 4K or 64K pages.
> >>>>
> >>>>Another issue is that actual pages pinning/unpinning happens on every
> >>>>DMA map/unmap request. This does not affect the performance much now as
> >>>>we spend way too much time now on switching context between
> >>>>guest/userspace/host but this will start to matter when we add in-kernel
> >>>>DMA map/unmap acceleration.
> >>>>
> >>>>This introduces a new IOMMU type for SPAPR - VFIO_SPAPR_TCE_v2_IOMMU.
> >>>>New IOMMU deprecates VFIO_IOMMU_ENABLE/VFIO_IOMMU_DISABLE and introduces
> >>>>2 new ioctls to register/unregister DMA memory -
> >>>>VFIO_IOMMU_SPAPR_REGISTER_MEMORY and VFIO_IOMMU_SPAPR_UNREGISTER_MEMORY -
> >>>>which receive user space address and size of a memory region which
> >>>>needs to be pinned/unpinned and counted in locked_vm.
> >>>>New IOMMU splits physical pages pinning and TCE table update into 2 different
> >>>>operations. It requires 1) guest pages to be registered first 2) consequent
> >>>>map/unmap requests to work only with pre-registered memory.
> >>>>For the default single window case this means that the entire guest
> >>>>(instead of 2GB) needs to be pinned before using VFIO.
> >>>>When a huge DMA window is added, no additional pinning will be
> >>>>required, otherwise it would be guest RAM + 2GB.
> >>>>
> >>>>The new memory registration ioctls are not supported by
> >>>>VFIO_SPAPR_TCE_IOMMU. Dynamic DMA window and in-kernel acceleration
> >>>>will require memory to be preregistered in order to work.
> >>>>
> >>>>The accounting is done per the user process.
> >>>>
> >>>>This advertises v2 SPAPR TCE IOMMU and restricts what the userspace
> >>>>can do with v1 or v2 IOMMUs.
> >>>>
> >>>>Signed-off-by: Alexey Kardashevskiy <aik@ozlabs.ru>
> >>>>[aw: for the vfio related changes]
> >>>>Acked-by: Alex Williamson <alex.williamson@redhat.com>
> >>>>---
> >>>>Changes:
> >>>>v9:
> >>>>* s/tce_get_hva_cached/tce_iommu_use_page_v2/
> >>>>
> >>>>v7:
> >>>>* now memory is registered per mm (i.e. process)
> >>>>* moved memory registration code to powerpc/mmu
> >>>>* merged "vfio: powerpc/spapr: Define v2 IOMMU" into this
> >>>>* limited new ioctls to v2 IOMMU
> >>>>* updated doc
> >>>>* unsupported ioclts return -ENOTTY instead of -EPERM
> >>>>
> >>>>v6:
> >>>>* tce_get_hva_cached() returns hva via a pointer
> >>>>
> >>>>v4:
> >>>>* updated docs
> >>>>* s/kzmalloc/vzalloc/
> >>>>* in tce_pin_pages()/tce_unpin_pages() removed @vaddr, @size and
> >>>>replaced offset with index
> >>>>* renamed vfio_iommu_type_register_memory to vfio_iommu_spapr_register_memory
> >>>>and removed duplicating vfio_iommu_spapr_register_memory
> >>>>---
> >>>>  Documentation/vfio.txt              |  23 ++++
> >>>>  drivers/vfio/vfio_iommu_spapr_tce.c | 230 +++++++++++++++++++++++++++++++++++-
> >>>>  include/uapi/linux/vfio.h           |  27 +++++
> >>>>  3 files changed, 274 insertions(+), 6 deletions(-)
> >>>>
> >>>>diff --git a/Documentation/vfio.txt b/Documentation/vfio.txt
> >>>>index 96978ec..94328c8 100644
> >>>>--- a/Documentation/vfio.txt
> >>>>+++ b/Documentation/vfio.txt
> >>>>@@ -427,6 +427,29 @@ The code flow from the example above should be slightly changed:
> >>>>
> >>>>  	....
> >>>>
> >>>>+5) There is v2 of SPAPR TCE IOMMU. It deprecates VFIO_IOMMU_ENABLE/
> >>>>+VFIO_IOMMU_DISABLE and implements 2 new ioctls:
> >>>>+VFIO_IOMMU_SPAPR_REGISTER_MEMORY and VFIO_IOMMU_SPAPR_UNREGISTER_MEMORY
> >>>>+(which are unsupported in v1 IOMMU).
> >>>
> >>>A summary of the semantic differeces between v1 and v2 would be nice.
> >>>At this point it's not really clear to me if there's a case for
> >>>creating v2, or if this could just be done by adding (optional)
> >>>functionality to v1.
> >>
> >>v1: memory preregistration is not supported; explicit enable/disable ioctls
> >>are required
> >>
> >>v2: memory preregistration is required; explicit enable/disable are
> >>prohibited (as they are not needed).
> >>
> >>Mixing these in one IOMMU type caused a lot of problems like should I
> >>increment locked_vm by the 32bit window size on enable() or not; what do I
> >>do about pages pinning when map/map (check if it is from registered memory
> >>and do not pin?).
> >>
> >>Having 2 IOMMU models makes everything a lot simpler.
> >
> >Ok.  Would it simplify it further if you made v2 only usable on IODA2
> >hardware?
> 
> 
> Very little. V2 addresses memory pinning issue which is handled the same way
> on ioda2 and older hardware, including KVM acceleration. Whether enable DDW
> or not - this is handled just fine via extra properties in the GET_INFO
> ioctl().
> 
> IODA2 and others are different in handling multiple groups per container but
> this does not require changes to userspace API.
> 
> And remember, the only machine I can use 100% of time is POWER7/P5IOC2 so it
> is really useful if at least some bits of the patchset can be tested there;
> if it was a bit less different from IODA2, I would have even implemented DDW
> there too :)

Hm, ok.

> >>>>+PPC64 paravirtualized guests generate a lot of map/unmap requests,
> >>>>+and the handling of those includes pinning/unpinning pages and updating
> >>>>+mm::locked_vm counter to make sure we do not exceed the rlimit.
> >>>>+The v2 IOMMU splits accounting and pinning into separate operations:
> >>>>+
> >>>>+- VFIO_IOMMU_SPAPR_REGISTER_MEMORY/VFIO_IOMMU_SPAPR_UNREGISTER_MEMORY ioctls
> >>>>+receive a user space address and size of the block to be pinned.
> >>>>+Bisecting is not supported and VFIO_IOMMU_UNREGISTER_MEMORY is expected to
> >>>>+be called with the exact address and size used for registering
> >>>>+the memory block. The userspace is not expected to call these often.
> >>>>+The ranges are stored in a linked list in a VFIO container.
> >>>>+
> >>>>+- VFIO_IOMMU_MAP_DMA/VFIO_IOMMU_UNMAP_DMA ioctls only update the actual
> >>>>+IOMMU table and do not do pinning; instead these check that the userspace
> >>>>+address is from pre-registered range.
> >>>>+
> >>>>+This separation helps in optimizing DMA for guests.
> >>>>+
> >>>>  -------------------------------------------------------------------------------
> >>>>
> >>>>  [1] VFIO was originally an acronym for "Virtual Function I/O" in its
> >>>>diff --git a/drivers/vfio/vfio_iommu_spapr_tce.c b/drivers/vfio/vfio_iommu_spapr_tce.c
> >>>>index 892a584..4cfc2c1 100644
> >>>>--- a/drivers/vfio/vfio_iommu_spapr_tce.c
> >>>>+++ b/drivers/vfio/vfio_iommu_spapr_tce.c
> >>>
> >>>So, from things you said at other points, I thought the idea was that
> >>>this registration stuff could also be used on non-Power IOMMUs.  Did I
> >>>misunderstand, or is that a possibility for the future?
> >>
> >>
> >>I never said a thing about non-PPC :) I seriously doubt any other arch has
> >>this hypervisor interface with H_PUT_TCE (may be s390? :) ); for others
> >>there is no profit from memory preregistration as they (at least x86) do map
> >>the entire guest before it starts which essentially is that preregistration.
> >>
> >>
> >>btw later we may want to implement simple IOMMU v3 which will do pinning +
> >>locked_vm when mapping as x86 does, for http://dpdk.org/ - these things do
> >>not really have to bother with preregistration (even if it just a single
> >>additional ioctl).
> >>
> >>
> >>
> >>>>@@ -21,6 +21,7 @@
> >>>>  #include <linux/vfio.h>
> >>>>  #include <asm/iommu.h>
> >>>>  #include <asm/tce.h>
> >>>>+#include <asm/mmu_context.h>
> >>>>
> >>>>  #define DRIVER_VERSION  "0.1"
> >>>>  #define DRIVER_AUTHOR   "aik@ozlabs.ru"
> >>>>@@ -91,8 +92,58 @@ struct tce_container {
> >>>>  	struct iommu_group *grp;
> >>>>  	bool enabled;
> >>>>  	unsigned long locked_pages;
> >>>>+	bool v2;
> >>>>  };
> >>>>
> >>>>+static long tce_unregister_pages(struct tce_container *container,
> >>>>+		__u64 vaddr, __u64 size)
> >>>>+{
> >>>>+	long ret;
> >>>>+	struct mm_iommu_table_group_mem_t *mem;
> >>>>+
> >>>>+	if ((vaddr & ~PAGE_MASK) || (size & ~PAGE_MASK))
> >>>>+		return -EINVAL;
> >>>>+
> >>>>+	mem = mm_iommu_get(vaddr, size >> PAGE_SHIFT);
> >>>>+	if (!mem)
> >>>>+		return -EINVAL;
> >>>>+
> >>>>+	ret = mm_iommu_put(mem); /* undo kref_get() from mm_iommu_get() */
> >>>>+	if (!ret)
> >>>>+		ret = mm_iommu_put(mem);
> >>>>+
> >>>>+	return ret;
> >>>>+}
> >>>>+
> >>>>+static long tce_register_pages(struct tce_container *container,
> >>>>+		__u64 vaddr, __u64 size)
> >>>>+{
> >>>>+	long ret = 0;
> >>>>+	struct mm_iommu_table_group_mem_t *mem;
> >>>>+	unsigned long entries = size >> PAGE_SHIFT;
> >>>>+
> >>>>+	if ((vaddr & ~PAGE_MASK) || (size & ~PAGE_MASK) ||
> >>>>+			((vaddr + size) < vaddr))
> >>>>+		return -EINVAL;
> >>>>+
> >>>>+	mem = mm_iommu_get(vaddr, entries);
> >>>>+	if (!mem) {
> >>>>+		ret = try_increment_locked_vm(entries);
> >>>>+		if (ret)
> >>>>+			return ret;
> >>>>+
> >>>>+		ret = mm_iommu_alloc(vaddr, entries, &mem);
> >>>>+		if (ret) {
> >>>>+			decrement_locked_vm(entries);
> >>>>+			return ret;
> >>>>+		}
> >>>>+	}
> >>>>+
> >>>>+	container->enabled = true;
> >>>>+
> >>>>+	return 0;
> >>>>+}
> >>>
> >>>So requiring that registered regions get unregistered with exactly the
> >>>same addr/length is reasonable.  I'm a bit less convinced that
> >>>disallowing overlaps is a good idea.  What if two libraries in the
> >>>same process are trying to use VFIO - they may not know if the regions
> >>>they try to register are overlapping.
> >>
> >>
> >>Sorry, I do not understand. A library allocates RAM. A library is expected
> >>to do register it via additional ioctl, that's it. Another library allocates
> >>another chunk of memory and it won't overlap and the registered areas won't
> >>either.
> >
> >So the case I'm thinking is where the library does VFIO using a buffer
> >passed into it from the program at large.  Another library does the
> >same.
> >
> >The main program, unaware of the VFIO shenanigans passes different
> >parts of the same page to the 2 libraries.
> >
> >This is somewhat similar to the case of the horribly, horribly broken
> >semantics of POSIX file range locks (it's both hard to implement and
> >dangerous in the multi-library case similar to above).
> 
> 
> Ok. I'll implement x86-alike V3 SPAPR TCE IOMMU for these people, later :)
> 
> V2 addresses issues caused by H_PUT_TCE + DDW RTAS interfaces.
> 
> 
> 
> >>>>  static bool tce_page_is_contained(struct page *page, unsigned page_shift)
> >>>>  {
> >>>>  	/*
> >>>>@@ -205,7 +256,7 @@ static void *tce_iommu_open(unsigned long arg)
> >>>>  {
> >>>>  	struct tce_container *container;
> >>>>
> >>>>-	if (arg != VFIO_SPAPR_TCE_IOMMU) {
> >>>>+	if ((arg != VFIO_SPAPR_TCE_IOMMU) && (arg != VFIO_SPAPR_TCE_v2_IOMMU)) {
> >>>>  		pr_err("tce_vfio: Wrong IOMMU type\n");
> >>>>  		return ERR_PTR(-EINVAL);
> >>>>  	}
> >>>>@@ -215,6 +266,7 @@ static void *tce_iommu_open(unsigned long arg)
> >>>>  		return ERR_PTR(-ENOMEM);
> >>>>
> >>>>  	mutex_init(&container->lock);
> >>>>+	container->v2 = arg == VFIO_SPAPR_TCE_v2_IOMMU;
> >>>>
> >>>>  	return container;
> >>>>  }
> >>>>@@ -243,6 +295,47 @@ static void tce_iommu_unuse_page(struct tce_container *container,
> >>>>  	put_page(page);
> >>>>  }
> >>>>
> >>>>+static int tce_iommu_use_page_v2(unsigned long tce, unsigned long size,
> >>>>+		unsigned long *phpa, struct mm_iommu_table_group_mem_t **pmem)
> >>
> >>
> >>You suggested s/tce_get_hpa/tce_iommu_use_page/ but in this particular patch
> >>it is confusing as tce_iommu_unuse_page_v2() calls it to find corresponding
> >>mm_iommu_table_group_mem_t by the userspace address address of a page being
> >>stopped used.
> >>
> >>tce_iommu_use_page (without v2) does use the page but this one I'll rename
> >>back to tce_iommu_ua_to_hpa_v2(), is that ok?
> >
> >Sorry, I couldn't follow this comment.
> 
> 
> For V1 IOMMU, I used to have:
> tce_get_hpa() - this converted UA to linear address and did gup();
> tce_iommu_unuse_page() - this did put_page().
> 
> You suggested (*) to rename the first one to tce_use_page() which makes sense.
> 
> V2 introduces its own versions of use/unuse but these use preregistered
> memory and do not do gup()/put_page(). I named them:
> tce_get_hpa_cached()
> tce_iommu_unuse_page_v2()
> 
> then, replaying your comment (*) on V2 IOMMU, I renamed tce_get_hpa_cached()
> to tce_iommu_use_page_v2(). And I do not like the result now (in the chunk
> below). I'll rename it to tce_iommu_ua_to_hpa_v2(), will it be ok?

Uh, I guess so.  To me "use_page" suggests incrementing the reference
or locking it or something along those lines, so I think that name
should follow the "gup".

> 
> 
> 
> >
> >>
> >>
> >>>>+{
> >>>>+	long ret = 0;
> >>>>+	struct mm_iommu_table_group_mem_t *mem;
> >>>>+
> >>>>+	mem = mm_iommu_lookup(tce, size);
> >>>>+	if (!mem)
> >>>>+		return -EINVAL;
> >>>>+
> >>>>+	ret = mm_iommu_ua_to_hpa(mem, tce, phpa);
> >>>>+	if (ret)
> >>>>+		return -EINVAL;
> >>>>+
> >>>>+	*pmem = mem;
> >>>>+
> >>>>+	return 0;
> >>>>+}
> >>>>+
> >>>>+static void tce_iommu_unuse_page_v2(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)
> >>>>+		return;
> >>>>+
> >>>>+	ret = tce_iommu_use_page_v2(*pua, IOMMU_PAGE_SIZE(tbl),
> >>>>+			&hpa, &mem);
> >>>>+	if (ret)
> >>>>+		pr_debug("%s: tce %lx at #%lx was not cached, ret=%d\n",
> >>>>+				__func__, *pua, entry, ret);
> >>>>+	if (mem)
> >>>>+		mm_iommu_mapped_update(mem, false);
> >>>>+
> >>>>+	*pua = 0;
> >>>>+}
> >>>>+
> 
> 

-- 
David Gibson			| I'll have my music baroque, and my code
david AT gibson.dropbear.id.au	| minimalist, thank you.  NOT _the_ _other_
				| _way_ _around_!
http://www.ozlabs.org/~dgibson

[-- Attachment #2: Type: application/pgp-signature, Size: 819 bytes --]