On 11/28/22 14:20, Jason Gunthorpe wrote:
On Mon, Nov 28, 2022 at 11:55:41AM +0100, Eric Auger wrote:
Not really. The name is a mess, but as it is implemented, it means the platform is implementing MSI security. How exactly that is done is not really defined, and it doesn't really belong as an iommu property. However the security is being created is done in a way that is transparent to the iommu_domain user.
Some 'ARM platforms' implement what you call MSI security but they do not advertise IOMMU_CAP_INTR_REMAP
Sounds like a bug.
Besides refering to include/linux/iommu.h: IOMMU_CAP_INTR_REMAP, /* IOMMU supports interrupt isolation */
Documentation doesn't match code.
It doesn't matter how it is done, if it remapping HW, fancy iommu_domain tricks, or built into the MSI controller. Set this flag if the platform is secure and doesn't need the code triggered by irq_domain_check_msi_remap().
this is not what is implemented as of now. If the IOMMU does support interrupt isolation, it advertises IOMMU_CAP_INTR_REMAP. On ARM this feature is implemented by the ITS MSI controller instead and the only way to retrieve the info whether the device MSIs are directed to that kind of MSI controller is to use irq_domain_check_msi_remap().
It is important to keep the Linux design seperated from what the architecture papers describes. In Linux the IOMMU is represented by the iommu_domain and the iommu_ops. On x86 neither of these objects play any role in interrupt delivery. Yes, the x86 architecture papers place some of the interrupt logic inside what they consider the iommu block, but that is just some historical stuff and shouldn't impact the SW design.
If we had put the IRTE bits inside the irqchip layer instead of in the iommu driver, it would have made a lot more sense.
The fact that ARM was allowed to be different (or rather the x86 mess wasn't cleaned up before the ARM mess was overlayed on top) is why this is so confusing. They are doing the same things, just in unnecessarily different ways.
fair enough. But that's a separate discussion that needs to happen with iommu and irqchip maintainers I think. At the moment things are implemented that way.
irq_domain_check_msi_remap() instead means the MSI controller implements that functionality (a given device id is able to trigger
Not quite, it means that MSI isolation is available, however it is not transparent and the iommu_domain user must do the little dance that follows.
No I do not agree on that point. The 'little dance' is needed because the SMMU does not bypass MSI writes as done on Intel. And someone must take care of the MSI transaction mapping. This is the role of the MSI cookie stuff. To me this is independent on the above discussion whether MSI isolation is implemented.
OK, so you are worried about someone who sets allow_unsafe_interrupts=1 they will not get the iommu_get_msi_cookie() call done even though they still need it? That does seem wrong.
yes
This was sort of sloppy copied from VFIO - we should just delete it. The is no driver that sets both, and once the platform asserts irq_domain_check_msi_remap() it is going down the non-transparent path anyhow and must set a cookie to work. [again the names doesn't make any sense for the functionality]
Failing with EPERM is probably not so bad since the platform is using an invalid configuration. I'm kind of inclined to leave this for right
I don't understand why it is invalid? HW MSI RESV region is a valid config and not sure you tested with that kind of setup, did you?
Why would it be a valid config? No driver sets both..
HW MSI RESV should set IOMMU_CAP_INTR_REMAP like Intel does.
irq_domain_check_msi_remap() is only for SW MSI RESV regions.
In theory no. Nothing prevents from having an MSI isolation capable controller (such as the ITS) with an IOMMU HW which wouldn't translate MSIs. In the past there has been such kind of problematic I think for HiSilicon HW https://lore.kernel.org/all/20171006140450.89652-1-shameerali.kolothum.thodi... This was eventually addressed differently but Shameer may precise ...
Eric
This is what the code implements, and yes it makes no sense.
Jason