Re: [Xen-devel] [PATCH v3 10/13] x86: add iommu_op to enable modification of IOMMU mappings

[Paul Durrant <Paul.Durrant@xxxxxxxxxx>]

> -----Original Message-----
> From: Wei Liu [mailto:wei.liu2@xxxxxxxxxx]
> Sent: 20 July 2018 09:59
> 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 10/13] x86: add iommu_op to enable modification of
> IOMMU mappings
> 
> On Tue, Jul 17, 2018 at 02:38:13PM +0100, Paul Durrant wrote:
> > +static int iommu_op_enable_modification(void)
> > +{
> > +    struct domain *currd = current->domain;
> > +    struct domain_iommu *iommu = dom_iommu(currd);
> > +    const struct iommu_ops *ops = iommu->platform_ops;
> > +
> > +    /* Has modification already been enabled? */
> > +    if ( iommu->iommu_op_ranges )
> > +        return 0;
> > +
> > +    /*
> > +     * The IOMMU mappings cannot be modified if:
> > +     * - the IOMMU is not enabled or,
> > +     * - the current domain is dom0 and tranlsation is disabled or,
> > +     * - HAP is enabled and the IOMMU shares the mappings.
> > +     */
> > +    if ( !iommu_enabled ||
> > +         (is_hardware_domain(currd) && iommu_passthrough) ||
> > +         iommu_use_hap_pt(currd) )
> > +        return -EACCES;
> > +
> > +    /*
> > +     * The IOMMU implementation must provide the lookup method if
> > +     * modification of the mappings is to be supported.
> > +     */
> > +    if ( !ops->lookup_page )
> > +        return -EOPNOTSUPP;
> > +
> > +    iommu->iommu_op_ranges = rangeset_new(currd, NULL, 0);
> > +    if ( !iommu->iommu_op_ranges )
> > +        return -ENOMEM;
> > +
> > +    currd->need_iommu = 0; /* Disable identity GFN mapping */
> 
> I think this flag currently serves two purposes: one is to indicate
> whether there are resources allocated within xen, the other it indicate
> whether it is in use (as suggested in the comment).
> 
> Suppose a domain gets all its devices deassigned after this call,
> original iommu_teardown would have been called, but now it won't. The
> iommu will still be torn down when it is destroyed, but this is a very
> subtle change in behaviour.

That is pretty subtle.
 
> 
> IMHO we should clearly separate these two cases if possible.
> 

Agreed. I never liked the 'need_iommu' name anyway. I'll add a patch before 
this one to do the rename/split.

  Paul

> > +    return 0;
> > +}
> > +
> [...]
> > diff --git a/xen/drivers/passthrough/iommu.c
> b/xen/drivers/passthrough/iommu.c
> > index fb9d0e1848..c517428621 100644
> > --- a/xen/drivers/passthrough/iommu.c
> > +++ b/xen/drivers/passthrough/iommu.c
> > @@ -26,7 +26,6 @@ static void iommu_dump_p2m_table(unsigned char
> key);
> >
> >  unsigned int __read_mostly iommu_dev_iotlb_timeout = 1000;
> >  integer_param("iommu_dev_iotlb_timeout", iommu_dev_iotlb_timeout);
> > -
> 
> Please keep the blank line here.
> 
> Wei.

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