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@xxxxxxxxxx/ This was eventually addressed differently but Shameer may precise ... Eric > > This is what the code implements, and yes it makes no sense. > > Jason >
