Re: [PATCH for-4.15 1/4] xen/iommu: Check if the IOMMU was initialized before tearing down

[Julien Grall <julien@xxxxxxx>]

Hi Jan,

On 23/12/2020 13:27, Jan Beulich wrote:
> On 22.12.2020 16:43, Julien Grall wrote:
> > --- a/xen/drivers/passthrough/iommu.c
> > +++ b/xen/drivers/passthrough/iommu.c
> > @@ -226,7 +226,15 @@ static void iommu_teardown(struct domain *d)
> >   
> >   void iommu_domain_destroy(struct domain *d)
> >   {
> > -    if ( !is_iommu_enabled(d) )
> > +    struct domain_iommu *hd = dom_iommu(d);
> > +
> > +    /*
> > +     * In case of failure during the domain construction, it would be
> > +     * possible to reach this function with the IOMMU enabled but not
> > +     * yet initialized. We assume that hd->platforms will be non-NULL as
> > +     * soon as we start to initialize the IOMMU.
> > +     */
> > +    if ( !is_iommu_enabled(d) || !hd->platform_ops )
> >           return;
>  From your description I'd rather say is_iommu_enabled() is the
> wrong predicate to use here. IOW I'd rather see it be replaced.
!hd->platform_ops should be sufficient. So, I think we can drop 
!is_iommu_enabled(d). Would that be fine with you?
> A couple of additional nits: "hd" isn't really necessary to
> have as a local variable, but if you insist on introducing it
> despite being used just once, it should be pointer-to-const.
> Plus the comment would better spell correctly the field it
> talks about. But I consider this comment excessive anyway, as
> the check of ->platform_ops is quite clear by itself.
Well, I added the comment because I think check hd->platform_ops may not 
be that obvious (see more below).
> And finally "we assume" is in at least latent conflict with
> ->platform_ops getting set only after arch_iommu_domain_init()
> was called. Right now neither x86'es nor Arm's do anything
> that would need undoing, but I'd still suggest to replace
> "assume" here (if you want to keep the comment in the first
> place) and move the assignment up a few lines (right after the
> is_iommu_enabled() check there).
My initial implementation of this patch moved the initialization of 
hd->platform_ops before arch_iommu_domain_init().
However, this is going to lead to some issues with Paul's series which 
add an IOMMU page-table pool ([1]).
The function arch_iommu_domain_init() may now fail. If we initialize 
hd->platform_ops earlier on, then the ->teardown() will be called before 
->init().
This may be an issue if ->teardown() expects some list pointer to be 
initialized by ->init(). I am not aware of any today, but this seems 
quite risky to me.
So I think it is better if we initialize hd->platform_ops after 
arch_iommu_domain_init() and expect the function to clean-up nything 
that was allocated before the error.
The alternative is we set hd->platform_ops if both 
arch_iommu_domain_init() and ->init() have succeeded. This means they 
will both have to clean-up in case of an error.
Any thoughts?

Cheers,

[1] <20201005094905.2929-6-paul@xxxxxxx>

--
Julien Grall