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: Xen-devel [mailto:xen-devel-bounces@xxxxxxxxxxxxxxxxxxxx] On Behalf
> Of Paul Durrant
> Sent: 20 July 2018 10:05
> To: Wei Liu <wei.liu2@xxxxxxxxxx>
> Cc: Stefano Stabellini <sstabellini@xxxxxxxxxx>; Wei Liu
> <wei.liu2@xxxxxxxxxx>; Andrew Cooper <Andrew.Cooper3@xxxxxxxxxx>; Tim
> (Xen.org) <tim@xxxxxxx>; George Dunlap <George.Dunlap@xxxxxxxxxx>;
> Julien Grall <julien.grall@xxxxxxx>; Jan Beulich <jbeulich@xxxxxxxx>; Ian
> Jackson <Ian.Jackson@xxxxxxxxxx>; xen-devel@xxxxxxxxxxxxxxxxxxxx
> Subject: Re: [Xen-devel] [PATCH v3 10/13] x86: add iommu_op to enable
> modification of IOMMU mappings
> 
> > -----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.

Actually, thinking a bit more, that is the semantic I want. The domain is 
taking control of its own mappings. It doesn't want Xen to tear them down under 
its feet.

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

I still think this renaming/clarification is worthwhile so I'll still add 
something into the series.

  Paul

>   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
_______________________________________________
Xen-devel mailing list
Xen-devel@xxxxxxxxxxxxxxxxxxxx
https://lists.xenproject.org/mailman/listinfo/xen-devel