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@xxxxxxxxxx> 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@xxxxxxxxxx>
>> Signed-off-by: Bharat Bhushan <Bharat.Bhushan@xxxxxxxxxxxxx>
>>
>> ---
>> 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@xxxxxxxxxx>"
>> @@ -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"?
>
>> @@ -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
>
--
To unsubscribe from this list: send the line "unsubscribe kvm" in
the body of a message to majordomo@xxxxxxxxxxxxxxx
More majordomo info at http://vger.kernel.org/majordomo-info.html
