> -----Original Message-----
> From: Wei Liu [mailto:[email protected]]
> Sent: 20 July 2018 09:59
> To: Paul Durrant <[email protected]>
> Cc: [email protected]; Jan Beulich <[email protected]>;
> Andrew Cooper <[email protected]>; George Dunlap
> <[email protected]>; Ian Jackson <[email protected]>; Julien
> Grall <[email protected]>; Konrad Rzeszutek Wilk
> <[email protected]>; Stefano Stabellini <[email protected]>; Tim
> (Xen.org) <[email protected]>; Wei Liu <[email protected]>
> 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
[email protected]
https://lists.xenproject.org/mailman/listinfo/xen-devel
