Re: [RFC 11/19] xen/passthrough: Call arch_iommu_domain_destroy before calling iommu_teardown

[Julien Grall <julien.grall@linaro.org>]

On 06/17/2014 10:29 AM, Jan Beulich wrote:
>>>> On 17.06.14 at 11:18, <julien.grall@linaro.org> wrote:
>> On 17/06/14 09:07, Jan Beulich wrote:
>>>>>> On 16.06.14 at 18:17, <julien.grall@linaro.org> wrote:
>>>> --- a/xen/drivers/passthrough/iommu.c
>>>> +++ b/xen/drivers/passthrough/iommu.c
>>>> @@ -219,10 +219,10 @@ void iommu_domain_destroy(struct domain *d)
>>>>       if ( !iommu_enabled || !hd->platform_ops )
>>>>           return;
>>>>
>>>> +    arch_iommu_domain_destroy(d);
>>>> +
>>>>       if ( need_iommu(d) )
>>>>           iommu_teardown(d);
>>>> -
>>>> -    arch_iommu_domain_destroy(d);
>>>
>>> At the first glance this doesn't look right, including the explanation
>>> you gave (why would devices still be assigned to a guest at this
>>> point).
>>
>> Because the toolstack may forget to deassign a device. How do you handle 
>> this case in x86? In the SMMU case, this will mean a memory leak and 
>> misconfiguration of the registers.
> 
> Proper tool stack behavior is required (and not just here).

I think this is important to handle toolstack failure (such as crash)
just in case. Hence it doesn't add much code for this purpose.

>>> And it's rather hard to properly decide with the series here
>>> depending on two other series, i.e. there not being a
>>> arch_iommu_domain_destroy() at all in current staging.
>>
>> Are you sure? The other series doesn't deal with the IOMMU stuff. This 
>> change has been pushed upstream a month ago (see commit 4905b35c " 
>> iommu: introduce arch specific code").
> 
> Oops, indeed - I'm sorry, I looked at a stale branch. Looking at the
> correct code I still think the current order is the correct one, and if
> you need to take extra steps you ought to do so from the .teardown
> hook.

I though about implement it in .teardown, but it results to non-obvious
code.

I could call iommu_dt_domain_destroy in .teardown, that will mean to
call "arch dt" code in the SMMU drivers which I think break the design.
I would prefer call it the arch specific function. Do you mind if I add
a new function called arch_iommu_reassign_devices? This function will
reassign every devices of a given domain to the hardware domain.

The iommmu_domain_destroy will look like:

void iommu_domain_destroy(struct domain *d)
{
	if ( !iommu_enabled )
		return;

	arch_iommu_reassign_devices(d);
	if ( need_iommu(d) )
	  iommu_teardown(d);
	arch_iommu_domain_destroy(d);
}

Regards,

-- 
Julien Grall