On Tue, Nov 02, 2021 at 03:00:24PM +0100, Jan Beulich wrote: > On 02.11.2021 12:03, Roger Pau Monné wrote: > > On Tue, Nov 02, 2021 at 11:13:08AM +0100, Jan Beulich wrote: > >> On 25.10.2021 12:28, Roger Pau Monné wrote: > >>> On Thu, Oct 21, 2021 at 11:59:02AM +0200, Jan Beulich wrote: > >>>> The two are really meant to be independent settings; iov_supports_xt() > >>>> using || instead of && was simply wrong. The corrected check is, > >>>> however, redundant, just like the (correct) one in iov_detect(): These > >>>> hook functions are unreachable without acpi_ivrs_init() installing the > >>>> iommu_init_ops pointer, which it does only upon success. (Unlike for > >>>> VT-d there is no late clearing of iommu_enable due to quirks, and any > >>>> possible clearing of iommu_intremap happens only after iov_supports_xt() > >>>> has run.) > >>>> > >>>> Signed-off-by: Jan Beulich <jbeul...@suse.com> > >>>> --- > >>>> In fact in iov_detect() it could be iommu_enable alone which gets > >>>> checked, but this felt overly aggressive to me. Instead I'm getting the > >>>> impression that the function may wrongly not get called when "iommu=off" > >>>> but interrupt remapping is in use: We'd not get the interrupt handler > >>>> installed, and hence interrupt remapping related events would never get > >>>> reported. (Same on VT-d, FTAOD.) > >>> > >>> I've spend a non-trivial amount of time looking into this before > >>> reading this note. AFAICT you could set iommu=off and still get x2APIC > >>> enabled and relying on interrupt remapping. > >> > >> Right, contrary to ... > >> > >>>> For iov_supports_xt() the question is whether, like VT-d's > >>>> intel_iommu_supports_eim(), it shouldn't rather check iommu_intremap > >>>> alone (in which case it would need to remain a check rather than getting > >>>> converted to ASSERT()). > >>> > >>> Hm, no, I don't think so. I think iommu_enable should take precedence > >>> over iommu_intremap, and having iommu_enable == false should force > >>> interrupt remapping to be reported as disabled. Note that disabling it > >>> in iommu_setup is too late, as the APIC initialization will have > >>> already taken place. > >>> > >>> It's my reading of the command line parameter documentation that > >>> setting iommu=off should disable all usage of the IOMMU, and that > >>> includes the interrupt remapping support (ie: a user should not need > >>> to set iommu=off,no-intremap) > >> > >> ... that documentation. But I think it's the documentation that > >> wants fixing, such that iommu=off really only control DMA remap. > > > > IMO I think it's confusing to have sub-options that could be enabled > > when you set the global one to off. I would expect `iommu=off` to > > disable all the iommu related options, and I think it's fair for > > people to expect that behavior. > > > > I'm unsure whether it's fair to change the documentation now, we > > should instead fix the code, so that people using `iommu=off` get the > > expected behavior. Then we would likely need to introduce a way to > > disable just dma remapping (dmaremap, similar to intremap). That > > would make a much better and saner interface IMO. > > But from an x2APIC perspective it is a problem to have "iommu=off" > also turn off intremap.
I think we could log a message in that case? (x2APIC could be enabled but iommu explicitly disabled) And maybe expand the documentation to notice that disabling the iommu or interrupt remapping will disable x2APIC support. > And indeed the option has never (fully) > worked that way: It clears iommu_enable, but not iommu_intremap > (nor any of the other sub-options, but there it's less of a problem > because they're not used in isolation), and iommu_intremap only > may have happened to either get turned off later or to not get > evaluated in at least some of the case. While I understand there's some baggage here, I'm not sure keeping the current behavior is correct. I would rather have iommu=off to cover all iommu functionality, and then we should add dmaremap sub-option to disable remapping only. I think that would be a sane and logic interface for users to understand. We should also note the implications of disabling interrupt remapping regarding x2APIC support in the documentation. Thanks, Roger.
