| Message ID | 20200415010255.10081-11-sstabellini@kernel.org (mailing list archive) |
|---|---|
| State | New, archived |
| Headers | show |
| Series | [01/12] xen: introduce xen_dom_flags | expand |
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()? Cheers,
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. (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.) 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. 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 All in all, I thought it would make sense to avoid any iommu settings and potential iommu errors altogether if xen_force is set and return early.
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,
On Thu, 30 Apr 2020, Julien Grall wrote: > 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. What you wrote makes sense and also matches my understanding. But some of my testing results confused me. I am going to go back and look at this closely again, but I am thinking of dropping this patch and avoiding interfering with IOMMU settings.
diff --git a/xen/arch/arm/domain_build.c b/xen/arch/arm/domain_build.c index 9bc0b810a7..d0fc497d07 100644 --- a/xen/arch/arm/domain_build.c +++ b/xen/arch/arm/domain_build.c @@ -1884,12 +1884,14 @@ static int __init handle_passthrough_prop(struct kernel_info *kinfo, if ( res < 0 ) return res; + if ( xen_force ) + return 0; + res = iommu_add_dt_device(node); if ( res < 0 ) return res; - /* If xen_force, we allow assignment of devices without IOMMU protection. */ - if ( xen_force && !dt_device_is_protected(node) ) + if ( !dt_device_is_protected(node) ) return 0; return iommu_assign_dt_device(kinfo->d, node);
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. Signed-off-by: Stefano Stabellini <stefano.stabellini@xilinx.com> --- xen/arch/arm/domain_build.c | 6 ++++-- 1 file changed, 4 insertions(+), 2 deletions(-)