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

[Paul Durrant <Paul.Durrant@xxxxxxxxxx>]

> -----Original Message-----
> From: Jan Beulich [mailto:JBeulich@xxxxxxxx]
> Sent: 12 September 2018 08:04
> To: Paul Durrant <Paul.Durrant@xxxxxxxxxx>
> Cc: Julien Grall <julien.grall@xxxxxxx>; Andrew Cooper
> <Andrew.Cooper3@xxxxxxxxxx>; Wei Liu <wei.liu2@xxxxxxxxxx>; George
> Dunlap <George.Dunlap@xxxxxxxxxx>; Ian Jackson <Ian.Jackson@xxxxxxxxxx>;
> Stefano Stabellini <sstabellini@xxxxxxxxxx>; xen-devel <xen-
> devel@xxxxxxxxxxxxxxxxxxxx>; Konrad Rzeszutek Wilk
> <konrad.wilk@xxxxxxxxxx>; Tim (Xen.org) <tim@xxxxxxx>
> Subject: Re: [PATCH v6 13/14] x86: add iommu_ops to modify and flush
> IOMMU mappings
> 
> >>> On 23.08.18 at 11:47, <paul.durrant@xxxxxxxxxx> 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).
> 

Yes, that's a good point. I could do a lookup to check whether the B[D]FN is 
already there though. Agreed that the memory is unounded unless the number of 
ranges is limited, which there is already a facility for. It is not ideal 
though.

> 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 don't know whether that is possible; I'll have to find specs for Intel and 
AMD IOMMUs and see if they have PTE bits available for such a use.

> 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.
> 

Remember that PV-IOMMU is only available for dom0 as it stands (and that is the 
only use-case that XenServer currently has) so I think that, whilst the concern 
is valid, there is no need danger in putting the code without such tracking. 
Such work can be deferred to making PV-IOMMU for de-privileged guests... if 
that facility is needed.

  Paul

> Jan
> 


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