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

[Jan Beulich <jbeulich@xxxxxxxx>]

On 23.12.2020 14:50, Julien Grall wrote:
> 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?

Well, that's what I was trying to suggest.

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

In such a case the obvious thing is to make the teardown handler
check whether its init counterpart has run before. This will then
fit our apparently much wider goal of making cleanup functions
idempotent.

Jan

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