Re: [Xen-devel] [PATCH V4 7/8] iommu/arm: Introduce iommu_add_dt_device API

From: Oleksandr <olekstysh@gmail.com>
To: Julien Grall <julien.grall@arm.com>, xen-devel@lists.xenproject.org
Cc: Oleksandr Tyshchenko <oleksandr_tyshchenko@epam.com>,
	sstabellini@kernel.org, Volodymyr_Babchuk@epam.com,
	Jan Beulich <jbeulich@suse.com>
Subject: Re: [Xen-devel] [PATCH V4 7/8] iommu/arm: Introduce iommu_add_dt_device API
Date: Fri, 20 Sep 2019 16:24:49 +0300	[thread overview]
Message-ID: <b4faaef5-c99e-8ba2-49fb-2279d40edef3@gmail.com> (raw)
In-Reply-To: <5d306b6f-b657-4668-4c79-f624ca9350a5@arm.com>

Hi Julien

>
>>
>>>
>>>>>> +
>>>>>> +int __init iommu_add_dt_device(struct dt_device_node *np)
>>>>>
>>>>> Sorry to only realise it now. Would it make sense to have this 
>>>>> function implemented in xen/passthrough/device_tree.c? 
>>>>
>>>> Not entirely sure. device_tree.c is a common code. The iommu_fwspec 
>>>> stuff (widely used in this function) is ARM code.
>>>
>>> Some of the device_tree.c already contains Arm specific code (such 
>>> as device.h).
>>>
>>> DT has been only used by Arm so far, so it is sadly fairly tie to 
>>> the architecture. But it should be easy to make it generic if needs 
>>> be (such as for RISCv).
>>>
>>> While iommu_fwspec is been implemented in Arm headers, this could 
>>> potentially be made common. So I would still prefer this that 
>>> function is moved in device_tree.c
>>
>> Well, will move. Also I will remove __init as it can be called at 
>> runtime...
>>
>>
>> As for runtime:
>>
>> The current implementation allows us to fail at early stage if 
>> something is wrong with the device which is behind an IOMMU (and 
>> needs to be protected). As we scan for all present devices, but not 
>> only for "passthrough".
>> The "splitting" into handle_device() for hwdom and 
>> iommu_do_dt_domctl() for other guests will postpone an error 
>> recognition to the guest domain creation time. So, we would have non 
>> function system anyway. Wouldn't be better to fail early instead of 
>> continue and fail anyway?
>
> Yes your implementation allows us to fail at early stage but then you 
> are abusing the function handle_device(). There are actually no 
> promise this will be called for every device going forward. Think 
> about dom0less where the goal is to have no dom0 at all.
>
> You are also tying the order of the domains creation as dom0 would 
> have to be always created before any other domains are created.
>
> Similar (ab)use of handle_device() does not exist, so I would rather 
> not start to introduce them because this will become quickly 
> unmaintainable as we are mixing dom0 creation and Xen initialization.
>
> Even without this series, assigning a device to the guest may not be a 
> success because the IOMMU may not have enough context bank (used for 
> configuring the IOMMU stage-2) or there are not enough memory. Not 
> been able to add the device to the IOMMU driver is only another 
> example where it may fail.
>
> But I would not consider this as not functional. The rest of your 
> system may work perfectly even if this particular device is not 
> usable. There are no security risk as the IOMMU should block any 
> transaction by default.
>
> I am also not in favor of parsing again the Device-Tree (we have 
> enough of them), so this is the best solution I can come up. Feel free 
> to suggest something different.

I am happy with the explanation, sounds reasonable. Will modify patch as 
you suggested.

-- 
Regards,

Oleksandr Tyshchenko

_______________________________________________
Xen-devel mailing list
Xen-devel@lists.xenproject.org
https://lists.xenproject.org/mailman/listinfo/xen-devel