Message-ID: <20160407122931.1ba3c01c@t450s.home>
Date:	Thu, 7 Apr 2016 12:29:31 -0600
From:	Alex Williamson <alex.williamson@...hat.com>
To:	Eric Auger <eric.auger@...aro.org>
Cc:	eric.auger@...com, robin.murphy@....com, will.deacon@....com,
	joro@...tes.org, tglx@...utronix.de, jason@...edaemon.net,
	marc.zyngier@....com, christoffer.dall@...aro.org,
	linux-arm-kernel@...ts.infradead.org, kvmarm@...ts.cs.columbia.edu,
	kvm@...r.kernel.org, suravee.suthikulpanit@....com,
	patches@...aro.org, linux-kernel@...r.kernel.org,
	Manish.Jaggi@...iumnetworks.com, Bharat.Bhushan@...escale.com,
	pranav.sawargaonkar@...il.com, p.fedin@...sung.com,
	iommu@...ts.linux-foundation.org, Jean-Philippe.Brucker@....com,
	julien.grall@....com
Subject: Re: [PATCH v6 2/5] vfio: allow the user to register reserved iova
 range for MSI mapping

On Thu, 7 Apr 2016 15:43:29 +0200
Eric Auger <eric.auger@...aro.org> wrote:

> Hi Alex,
> On 04/07/2016 12:07 AM, Alex Williamson wrote:
> > On Mon,  4 Apr 2016 08:30:08 +0000
> > Eric Auger <eric.auger@...aro.org> wrote:
> >   
> >> The user is allowed to [un]register a reserved IOVA range by using the
> >> DMA MAP API and setting the new flag: VFIO_DMA_MAP_FLAG_MSI_RESERVED_IOVA.
> >> It provides the base address and the size. This region is stored in the
> >> vfio_dma rb tree. At that point the iova range is not mapped to any target
> >> address yet. The host kernel will use those iova when needed, typically
> >> when the VFIO-PCI device allocates its MSIs.
> >>
> >> This patch also handles the destruction of the reserved binding RB-tree and
> >> domain's iova_domains.
> >>
> >> Signed-off-by: Eric Auger <eric.auger@...aro.org>
> >> Signed-off-by: Bharat Bhushan <Bharat.Bhushan@...escale.com>
> >>
> >> ---
> >> v3 -> v4:
> >> - use iommu_alloc/free_reserved_iova_domain exported by dma-reserved-iommu
> >> - protect vfio_register_reserved_iova_range implementation with
> >>   CONFIG_IOMMU_DMA_RESERVED
> >> - handle unregistration by user-space and on vfio_iommu_type1 release
> >>
> >> v1 -> v2:
> >> - set returned value according to alloc_reserved_iova_domain result
> >> - free the iova domains in case any error occurs
> >>
> >> RFC v1 -> v1:
> >> - takes into account Alex comments, based on
> >>   [RFC PATCH 1/6] vfio: Add interface for add/del reserved iova region:
> >> - use the existing dma map/unmap ioctl interface with a flag to register
> >>   a reserved IOVA range. A single reserved iova region is allowed.
> >>
> >> Conflicts:
> >> 	drivers/vfio/vfio_iommu_type1.c
> >> ---
> >>  drivers/vfio/vfio_iommu_type1.c | 141 +++++++++++++++++++++++++++++++++++++++-
> >>  include/uapi/linux/vfio.h       |  12 +++-
> >>  2 files changed, 150 insertions(+), 3 deletions(-)
> >>
> >> diff --git a/drivers/vfio/vfio_iommu_type1.c b/drivers/vfio/vfio_iommu_type1.c
> >> index c9ddbde..4497b20 100644
> >> --- a/drivers/vfio/vfio_iommu_type1.c
> >> +++ b/drivers/vfio/vfio_iommu_type1.c
> >> @@ -36,6 +36,7 @@
> >>  #include <linux/uaccess.h>
> >>  #include <linux/vfio.h>
> >>  #include <linux/workqueue.h>
> >> +#include <linux/dma-reserved-iommu.h>
> >>  
> >>  #define DRIVER_VERSION  "0.2"
> >>  #define DRIVER_AUTHOR   "Alex Williamson <alex.williamson@...hat.com>"
> >> @@ -403,10 +404,22 @@ static void vfio_unmap_unpin(struct vfio_iommu *iommu, struct vfio_dma *dma)
> >>  	vfio_lock_acct(-unlocked);
> >>  }
> >>  
> >> +static void vfio_unmap_reserved(struct vfio_iommu *iommu)
> >> +{
> >> +#ifdef CONFIG_IOMMU_DMA_RESERVED
> >> +	struct vfio_domain *d;
> >> +
> >> +	list_for_each_entry(d, &iommu->domain_list, next)
> >> +		iommu_unmap_reserved(d->domain);
> >> +#endif
> >> +}
> >> +
> >>  static void vfio_remove_dma(struct vfio_iommu *iommu, struct vfio_dma *dma)
> >>  {
> >>  	if (likely(dma->type != VFIO_IOVA_RESERVED))
> >>  		vfio_unmap_unpin(iommu, dma);
> >> +	else
> >> +		vfio_unmap_reserved(iommu);
> >>  	vfio_unlink_dma(iommu, dma);
> >>  	kfree(dma);
> >>  }  
> > 
> > This makes me nervous, apparently we can add reserved mappings
> > individually, but we have absolutely no granularity on remove, so if we
> > remove one, we've removed them all even though we still have them
> > linked in our rb tree.  I see later that only one reserved region is
> > allowed, but that seems very short sighted, especially to impose that
> > on the user level API.  
> On kernel-size the reserved region is currently backed by a unique
> iova_domain. Do you mean you would like me to handle a list of
> iova_domains instead of using a single "cookie"?

TBH, I'm not really sure how this works with a single iova domain.  If
we have multiple irq chips and each gets mapped by a separate page in
the iova space, then is it really sufficient to do a lookup from the
irq_data to the msi_desc to the device to the domain in order to get a
reserved iova to map that msi doorbell?  Don't we need an iova from the
pool mapping the specific irqchip associated with our device?  The IOMMU
domain might span any number of irq chips, how can we assume there's
one only reserved iova space?  Maybe I'm not understanding how the code
works.

Conceptually, this is a generic IOMMU API extension to include reserved
iova space, MSI mappings are a consumer of that reserved iova pool but
I don't think we can say they will necessarily be the only consumer.
So building into the interface that there's only one is like making a
fixed length array to hold a string, it works for the initial
implementation, but it's not a robust solution.

I'm also trying to figure out how this maps to x86, the difference of
course being that for ARM you have a user specified, explicit MSI iova
space while x86 has an implicit MSI iova space.  So should x86 be
creating a reserved iova pool for the implicit mapping?  Should the
user have some way to query the mapping, whether implicit or explicit?
For instance, a new capability within the vfio iommu INFO ioctl might
expose reserved regions.  It might be initially present on x86 due to
the implicit nature of the reservation, while it might only appear on
ARM after submitting a reserved mapping.  Thanks,

Alex

 
> >> @@ -489,7 +502,8 @@ static int vfio_dma_do_unmap(struct vfio_iommu *iommu,
> >>  	 */
> >>  	if (iommu->v2) {
> >>  		dma = vfio_find_dma(iommu, unmap->iova, 0);
> >> -		if (dma && dma->iova != unmap->iova) {
> >> +		if (dma && (dma->iova != unmap->iova ||
> >> +			   (dma->type == VFIO_IOVA_RESERVED))) {  
> > 
> > This seems unnecessary, won't the reserved entries fall out in the
> > while loop below?  
> yes that's correct
> >   
> >>  			ret = -EINVAL;
> >>  			goto unlock;
> >>  		}
> >> @@ -501,6 +515,10 @@ static int vfio_dma_do_unmap(struct vfio_iommu *iommu,
> >>  	}
> >>  
> >>  	while ((dma = vfio_find_dma(iommu, unmap->iova, unmap->size))) {
> >> +		if (dma->type == VFIO_IOVA_RESERVED) {
> >> +			ret = -EINVAL;
> >> +			goto unlock;
> >> +		}  
> > 
> > Hmm, API concerns here.  Previously a user could unmap from iova = 0 to
> > size = 2^64 - 1 and expect all mappings to get cleared.  Now they can't
> > do that if they've registered any reserved regions.  Seems like maybe
> > we should ignore it and continue instead of abort, but then we need to
> > change the parameters of vfio_find_dma() to get it to move on, or pass
> > the type to the function, which would prevent us from getting here in
> > the first place.  
> OK I will rework this to match the existing use cases
> >   
> >>  		if (!iommu->v2 && unmap->iova > dma->iova)
> >>  			break;
> >>  		unmapped += dma->size;
> >> @@ -650,6 +668,114 @@ static int vfio_dma_do_map(struct vfio_iommu *iommu,
> >>  	return ret;
> >>  }
> >>  
> >> +static int vfio_register_reserved_iova_range(struct vfio_iommu *iommu,
> >> +			   struct vfio_iommu_type1_dma_map *map)
> >> +{
> >> +#ifdef CONFIG_IOMMU_DMA_RESERVED
> >> +	dma_addr_t iova = map->iova;
> >> +	size_t size = map->size;
> >> +	uint64_t mask;
> >> +	struct vfio_dma *dma;
> >> +	int ret = 0;
> >> +	struct vfio_domain *d;
> >> +	unsigned long order;
> >> +
> >> +	/* Verify that none of our __u64 fields overflow */
> >> +	if (map->size != size || map->iova != iova)
> >> +		return -EINVAL;
> >> +
> >> +	order =  __ffs(vfio_pgsize_bitmap(iommu));
> >> +	mask = ((uint64_t)1 << order) - 1;
> >> +
> >> +	WARN_ON(mask & PAGE_MASK);
> >> +
> >> +	if (!size || (size | iova) & mask)
> >> +		return -EINVAL;
> >> +
> >> +	/* Don't allow IOVA address wrap */
> >> +	if (iova + size - 1 < iova)
> >> +		return -EINVAL;
> >> +
> >> +	mutex_lock(&iommu->lock);
> >> +
> >> +	if (vfio_find_dma(iommu, iova, size)) {
> >> +		ret =  -EEXIST;
> >> +		goto out;
> >> +	}
> >> +
> >> +	dma = kzalloc(sizeof(*dma), GFP_KERNEL);
> >> +	if (!dma) {
> >> +		ret = -ENOMEM;
> >> +		goto out;
> >> +	}
> >> +
> >> +	dma->iova = iova;
> >> +	dma->size = size;
> >> +	dma->type = VFIO_IOVA_RESERVED;
> >> +
> >> +	list_for_each_entry(d, &iommu->domain_list, next)
> >> +		ret |= iommu_alloc_reserved_iova_domain(d->domain, iova,
> >> +							size, order);
> >> +
> >> +	if (ret) {
> >> +		list_for_each_entry(d, &iommu->domain_list, next)
> >> +			iommu_free_reserved_iova_domain(d->domain);
> >> +		goto out;
> >> +	}
> >> +
> >> +	vfio_link_dma(iommu, dma);
> >> +
> >> +out:
> >> +	mutex_unlock(&iommu->lock);
> >> +	return ret;
> >> +#else /* CONFIG_IOMMU_DMA_RESERVED */
> >> +	return -ENODEV;
> >> +#endif
> >> +}
> >> +
> >> +static void vfio_unregister_reserved_iova_range(struct vfio_iommu *iommu,
> >> +				struct vfio_iommu_type1_dma_unmap *unmap)
> >> +{
> >> +#ifdef CONFIG_IOMMU_DMA_RESERVED
> >> +	dma_addr_t iova = unmap->iova;
> >> +	struct vfio_dma *dma;
> >> +	size_t size = unmap->size;
> >> +	uint64_t mask;
> >> +	unsigned long order;
> >> +
> >> +	/* Verify that none of our __u64 fields overflow */
> >> +	if (unmap->size != size || unmap->iova != iova)
> >> +		return;
> >> +
> >> +	order =  __ffs(vfio_pgsize_bitmap(iommu));
> >> +	mask = ((uint64_t)1 << order) - 1;
> >> +
> >> +	WARN_ON(mask & PAGE_MASK);
> >> +
> >> +	if (!size || (size | iova) & mask)
> >> +		return;
> >> +
> >> +	/* Don't allow IOVA address wrap */
> >> +	if (iova + size - 1 < iova)
> >> +		return;
> >> +
> >> +	mutex_lock(&iommu->lock);
> >> +
> >> +	dma = vfio_find_dma(iommu, iova, size);
> >> +
> >> +	if (!dma || (dma->type != VFIO_IOVA_RESERVED)) {
> >> +		unmap->size = 0;
> >> +		goto out;
> >> +	}
> >> +
> >> +	unmap->size =  dma->size;
> >> +	vfio_remove_dma(iommu, dma);
> >> +
> >> +out:
> >> +	mutex_unlock(&iommu->lock);
> >> +#endif  
> > 
> > Having a find_dma that accepts a type and a remove_reserved here seems
> > like it might simplify things.
> >   
> >> +}
> >> +
> >>  static int vfio_bus_type(struct device *dev, void *data)
> >>  {
> >>  	struct bus_type **bus = data;
> >> @@ -946,6 +1072,7 @@ static void vfio_iommu_type1_release(void *iommu_data)
> >>  	struct vfio_group *group, *group_tmp;
> >>  
> >>  	vfio_iommu_unmap_unpin_all(iommu);
> >> +	vfio_unmap_reserved(iommu);  
> > 
> > If we call vfio_unmap_reserved() here, then why does vfio_remove_dma()
> > need to handle reserved entries?  We might as well have a separate
> > vfio_remove_reserved_dma().
> >   
> >>  
> >>  	list_for_each_entry_safe(domain, domain_tmp,
> >>  				 &iommu->domain_list, next) {
> >> @@ -1020,7 +1147,8 @@ static long vfio_iommu_type1_ioctl(void *iommu_data,
> >>  	} else if (cmd == VFIO_IOMMU_MAP_DMA) {
> >>  		struct vfio_iommu_type1_dma_map map;
> >>  		uint32_t mask = VFIO_DMA_MAP_FLAG_READ |
> >> -				VFIO_DMA_MAP_FLAG_WRITE;
> >> +				VFIO_DMA_MAP_FLAG_WRITE |
> >> +				VFIO_DMA_MAP_FLAG_MSI_RESERVED_IOVA;
> >>  
> >>  		minsz = offsetofend(struct vfio_iommu_type1_dma_map, size);
> >>  
> >> @@ -1030,6 +1158,9 @@ static long vfio_iommu_type1_ioctl(void *iommu_data,
> >>  		if (map.argsz < minsz || map.flags & ~mask)
> >>  			return -EINVAL;
> >>  
> >> +		if (map.flags & VFIO_DMA_MAP_FLAG_MSI_RESERVED_IOVA)
> >> +			return vfio_register_reserved_iova_range(iommu, &map);
> >> +
> >>  		return vfio_dma_do_map(iommu, &map);
> >>  
> >>  	} else if (cmd == VFIO_IOMMU_UNMAP_DMA) {
> >> @@ -1044,10 +1175,16 @@ static long vfio_iommu_type1_ioctl(void *iommu_data,
> >>  		if (unmap.argsz < minsz || unmap.flags)
> >>  			return -EINVAL;
> >>  
> >> +		if (unmap.flags & VFIO_DMA_MAP_FLAG_MSI_RESERVED_IOVA) {
> >> +			vfio_unregister_reserved_iova_range(iommu, &unmap);
> >> +			goto out;
> >> +		}
> >> +
> >>  		ret = vfio_dma_do_unmap(iommu, &unmap);
> >>  		if (ret)
> >>  			return ret;
> >>  
> >> +out:
> >>  		return copy_to_user((void __user *)arg, &unmap, minsz) ?
> >>  			-EFAULT : 0;
> >>  	}
> >> diff --git a/include/uapi/linux/vfio.h b/include/uapi/linux/vfio.h
> >> index 255a211..a49be8a 100644
> >> --- a/include/uapi/linux/vfio.h
> >> +++ b/include/uapi/linux/vfio.h
> >> @@ -498,12 +498,21 @@ struct vfio_iommu_type1_info {
> >>   *
> >>   * Map process virtual addresses to IO virtual addresses using the
> >>   * provided struct vfio_dma_map. Caller sets argsz. READ &/ WRITE required.
> >> + *
> >> + * In case MSI_RESERVED_IOVA flag is set, the API only aims at registering an
> >> + * IOVA region which will be used on some platforms to map the host MSI frame.
> >> + * in that specific case, vaddr and prot are ignored. The requirement for
> >> + * provisioning such IOVA range can be checked by calling VFIO_IOMMU_GET_INFO
> >> + * with the VFIO_IOMMU_INFO_REQUIRE_MSI_MAP attribute. A single
> >> + * MSI_RESERVED_IOVA region can be registered
> >>   */  
> > 
> > Why do we ignore read/write flags?  I'm not sure how useful a read-only
> > reserved region might be, but certainly some platforms might support
> > write-only or read-write.  Isn't this something we should let the IOMMU
> > driver decide?  ie. pass it down and let it fail or not?  
> OK Makes sense. Actually I am not very clear about whether this API is
> used for MSI binding only or likely to be used for something else.
> 
>   Also why are
> > we making it the API spec to only allow a single reserved region of
> > this type?  We could simply let additional ones fail, or better yet add
> > a capability to the info ioctl to indicate the number available and
> > then fail if the user exceeds it.  
> But this means that underneath we need to manage several iova_domains,
> right?
> >   
> >>  struct vfio_iommu_type1_dma_map {
> >>  	__u32	argsz;
> >>  	__u32	flags;
> >>  #define VFIO_DMA_MAP_FLAG_READ (1 << 0)		/* readable from device */
> >>  #define VFIO_DMA_MAP_FLAG_WRITE (1 << 1)	/* writable from device */
> >> +/* reserved iova for MSI vectors*/
> >> +#define VFIO_DMA_MAP_FLAG_MSI_RESERVED_IOVA (1 << 2)  
> > 
> > nit, ...RESERVED_MSI_IOVA makes a tad more sense and if we add new
> > reserved flags seems like it puts the precedence in order.  
> OK
> >   
> >>  	__u64	vaddr;				/* Process virtual address */
> >>  	__u64	iova;				/* IO virtual address */
> >>  	__u64	size;				/* Size of mapping (bytes) */
> >> @@ -519,7 +528,8 @@ struct vfio_iommu_type1_dma_map {
> >>   * Caller sets argsz.  The actual unmapped size is returned in the size
> >>   * field.  No guarantee is made to the user that arbitrary unmaps of iova
> >>   * or size different from those used in the original mapping call will
> >> - * succeed.
> >> + * succeed. A Reserved DMA region must be unmapped with MSI_RESERVED_IOVA
> >> + * flag set.  
> > 
> > So map/unmap become bi-modal, with this flag set they should only
> > operate on reserved entries, otherwise they should operate on legacy
> > entries.  So clearly as a user I should be able to continue doing an
> > unmap from 0-(-1) of legacy entries and not stumble over reserved
> > entries.  Thanks,  
> OK that's clear
> 
> Best Regards
> 
> Eric
> > 
> > Alex
> >   
> 

Powered by blists - more mailing lists