Re: [PATCH v6 13/14] x86: add iommu_ops to modify and flush IOMMU mappings

["Jan Beulich" <JBeulich@suse.com>]

>>> On 23.08.18 at 11:47, <paul.durrant@citrix.com> wrote:
> +static int iommuop_map(struct xen_iommu_op_map *op)
> +{
> +    struct domain *d, *currd = current->domain;
> +    struct domain_iommu *iommu = dom_iommu(currd);
> +    bool readonly = op->flags & XEN_IOMMUOP_map_readonly;
> +    bfn_t bfn = _bfn(op->bfn);
> +    struct page_info *page;
> +    unsigned int prot;
> +    int rc, ignore;
> +
> +    if ( op->pad ||
> +         (op->flags & ~(XEN_IOMMUOP_map_all |
> +                        XEN_IOMMUOP_map_readonly)) )
> +        return -EINVAL;
> +
> +    if ( !iommu->iommu_op_ranges )
> +        return -EOPNOTSUPP;
> +
> +    /* Per-device mapping not yet supported */
> +    if ( !(op->flags & XEN_IOMMUOP_map_all) )
> +        return -EINVAL;
> +
> +    /* Check whether the specified BFN falls in a reserved region */
> +    if ( rangeset_contains_singleton(iommu->reserved_ranges, bfn_x(bfn)) )
> +        return -EINVAL;
> +
> +    d = rcu_lock_domain_by_any_id(op->domid);
> +    if ( !d )
> +        return -ESRCH;
> +
> +    rc = get_paged_gfn(d, _gfn(op->gfn), readonly, NULL, &page);
> +    if ( rc )
> +        goto unlock;
> +
> +    rc = -EINVAL;
> +    if ( !readonly && !get_page_type(page, PGT_writable_page) )
> +    {
> +        put_page(page);
> +        goto unlock;
> +    }
> +
> +    prot = IOMMUF_readable;
> +    if ( !readonly )
> +        prot |= IOMMUF_writable;
> +
> +    rc = -EIO;
> +    if ( iommu_map_page(currd, bfn, page_to_mfn(page), prot) )
> +        goto release;
> +
> +    rc = rangeset_add_singleton(iommu->iommu_op_ranges, bfn_x(bfn));
> +    if ( rc )
> +        goto unmap;

When a mapping is requested for the same BFN that a prior mapping
was already established for, the page refs of that prior mapping get
leaked here. I don't think you want to require an intermediate unmap,
so checking the rangeset first is not an option. Hence I think you
need to look up the translation anyway, which may mean that the
rangeset's usefulness is quite limited (relevant with the additional
context of my question regarding it perhaps requiring a pretty much
unbounded amount of memory).

In order to avoid shooting down all pre-existing RAM mappings - is
there no way the page table entries could be marked to identify
their origin?

I also have another more general concern: Allowing the guest to
manipulate its IOMMU page tables means that it can deliberately
shatter large pages, growing the overall memory footprint of the
domain. I'm hesitant to say this, but I'm afraid that resource
tracking of such "behind the scenes" allocations might be a
necessary prereq for the PV IOMMU work.

Jan



_______________________________________________
Xen-devel mailing list
Xen-devel@lists.xenproject.org
https://lists.xenproject.org/mailman/listinfo/xen-devel