Re: [PATCH 11/12] xen/arm: if xen_force don't try to setup the IOMMU

[Julien Grall <julien@xxxxxxx>]

Hi Stefano,

On 29/04/2020 22:55, Stefano Stabellini wrote:
> On Wed, 15 Apr 2020, Julien Grall wrote:
> > Hi Stefano,
> >
> > On 15/04/2020 02:02, Stefano Stabellini wrote:
> > > If xen_force (which means xen,force-assign-without-iommu was requested)
> > > don't try to add the device to the IOMMU. Return early instead.
> >
> > Could you explain why this is an issue to call xen_force after
> > iommu_add_dt_device()?
> There are two issues. I should add info about both of them to the commit
> message.
>
>
> The first issue is that an error returned by iommu_add_dt_device (for
> any reason) would cause handle_passthrough_prop to stop and return error
> right away. But actually the iommu is not needed for that device if
> xen_force is set.
During boot, Xen will configure the IOMMUs to fault on any DMA 
transactions that are not valid. So if you don't call 
iommu_assign_dt_device(), then your device will be unusable.
Without your patch, the user will know directly something went wrong. 
With your patch, the fault may occur much later and be more difficult to 
diagnostics.
> (In fact, one of the reasons why a user might want to set
> force-assign-without-iommu is because there are iommu issues with a
> device.)
This would not work because of the reasons I explained above. The only 
way would be to configure the IOMMU in bypass mode for that device.
So you would still need to call the IOMMU subsystem.

> The second issue is about the usage of "xen,force-assign-without-iommu":
> it would be useful to let the user set "xen,force-assign-without-iommu"
> for devices that are described as behind a SMMU in device tree, but
> the SMMU can't actually be used for some reason. Of course, the user
> could always manually edit the device tree to make it look like as if
> the device is not behind an IOMMU. That would work OK. However, I think
> it would be better to avoid making that a requirement.
From the documentation:

"If xen,force-assign-without-iommu is present, Xen allows to assign a
device even if it is not behind an IOMMU. This renders your platform
*unsafe* if the device is DMA-capable."

xen,force-assign-without-iommu was never meant to be used if the device 
is protected behind an IOMMU. If you want to do that, then your patch is 
not going to be sufficient (see why above).
> If we want to allow "xen,force-assign-without-iommu" for a device behind
> a SMMU then we need this patch, otherwise this would happen:
>
>      res = iommu_add_dt_device(node); // succeeds
>      if ( xen_force && !dt_device_is_protected(node) ) // fails because the 
> device is protected
>          return 0;
>      return iommu_assign_dt_device(kinfo->d, node); // fails because 
> !is_iommu_enabled(d) which is fine but then handle_prop_pfdt returns error too
You are mixing two things here... xen,force-assign-without-iommu doesn't 
have a say on whether the IOMMU will be used for a domain. This decision 
is only based on whether a partial DT exists and (with your patch #3) 
whether the DomU memory is direct mapped.
The problem here is your are not enabling the IOMMU when a direct 
mapping is used. I don't think we want the direct mapping option to 
disable the IOMMU. This should be a separate option (maybe a bool 
property iommu).
Cheers,

--
Julien Grall