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

[Julien Grall <julien.grall@xxxxxxx>]

On 19/09/2019 14:26, Oleksandr wrote:
> On 19.09.19 15:29, Julien Grall wrote:
> > Hi,
> Hi, Julien
Hi,

> > > > > +
> > > > > +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.
Cheers,

--
Julien Grall

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