> -----Original Message----- > From: Jan Beulich [mailto:jbeul...@suse.com] > Sent: 12 September 2018 08:04 > To: Paul Durrant <paul.durr...@citrix.com> > Cc: Julien Grall <julien.gr...@arm.com>; Andrew Cooper > <andrew.coop...@citrix.com>; Wei Liu <wei.l...@citrix.com>; George > Dunlap <george.dun...@citrix.com>; Ian Jackson <ian.jack...@citrix.com>; > Stefano Stabellini <sstabell...@kernel.org>; xen-devel <xen- > de...@lists.xenproject.org>; Konrad Rzeszutek Wilk > <konrad.w...@oracle.com>; Tim (Xen.org) <t...@xen.org> > Subject: Re: [PATCH v6 13/14] x86: add iommu_ops to modify and flush > IOMMU mappings > > >>> On 23.08.18 at 11:47, <paul.durr...@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). >
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@lists.xenproject.org https://lists.xenproject.org/mailman/listinfo/xen-devel
