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

[Wei Liu <wei.liu2@xxxxxxxxxx>]

On Mon, Jul 23, 2018 at 02:40:05PM +0100, Paul Durrant wrote:
> > -----Original Message-----
> > From: Wei Liu [mailto:wei.liu2@xxxxxxxxxx]
> > Sent: 23 July 2018 14:35
> > To: Paul Durrant <Paul.Durrant@xxxxxxxxxx>
> > Cc: xen-devel@xxxxxxxxxxxxxxxxxxxx; Jan Beulich <jbeulich@xxxxxxxx>;
> > Andrew Cooper <Andrew.Cooper3@xxxxxxxxxx>; George Dunlap
> > <George.Dunlap@xxxxxxxxxx>; Ian Jackson <Ian.Jackson@xxxxxxxxxx>; Julien
> > Grall <julien.grall@xxxxxxx>; Konrad Rzeszutek Wilk
> > <konrad.wilk@xxxxxxxxxx>; Stefano Stabellini <sstabellini@xxxxxxxxxx>; Tim
> > (Xen.org) <tim@xxxxxxx>; Wei Liu <wei.liu2@xxxxxxxxxx>
> > Subject: Re: [PATCH v3 12/13] x86: add iommu_ops to modify and flush
> > IOMMU mappings
> > 
> > On Tue, Jul 17, 2018 at 02:38:15PM +0100, Paul Durrant 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_readonly) )
> > > +        return -EINVAL;
> > > +
> > > +    if ( !iommu->iommu_op_ranges )
> > > +        return -EOPNOTSUPP;
> > > +
> > > +    /* 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);
> > 
> > I think this needs to be moved earlier before dereferencing assigning
> > iommu -- it depends on d being valid.
> > 
> > Same applies to the unmap function.
> > 
> 
> Are you referring to the dom_iommu() macro? The code is uninterested
> in the IOMMU mappings of the target domain, only the current, so I
> can't see a problem here. Am I missing something?

Oh, I misread. Sorry for the noise.

> 
> > > +
> > > +static int iommuop_flush(void)
> > > +{
> > > +    return !iommu_iotlb_flush_all(current->domain) ? 0 : -EIO;
> > 
> > I don't follow: why does this only flush current->domain? But
> > map/unmap use explicit argument to specify a domain?
> > 
> 
> Because the page may come from another domain but the IOMMU mappings
> are only ever for the local domain.

I see. That's fine then.

Wei.

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