Re: [PATCH v5 13/19] iommufd: Add kAPI toward external drivers for physical devices

[Jason Gunthorpe <jgg@nvidia.com>]

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.

> >> 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.

> > 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.

This is what the code implements, and yes it makes no sense.

Jason