On Wed, Nov 03, 2021 at 04:15:52PM +0100, Jan Beulich wrote: > On 03.11.2021 16:06, Roger Pau Monné wrote: > > On Wed, Nov 03, 2021 at 10:46:40AM +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. > >> > >> It occurs to me that this reply of yours here contradicts your R-b > >> on patch 1, in particular with its revision log saying: > >> > >> v2: Treat iommu_enable and iommu_intremap as separate options. > > > > Right, I see. patch 1 uses > > > > if ( !iommu_enable && !iommu_intremap ) > > return; > > > > Which I think should be: > > > > if ( !iommu_enable ) > > return; > > > > Sorry I didn't realize in that context. I think we need to decide > > whether we want to fix the documentation to match the code, or whether > > we should fix the code to match the documentation. > > Except that adjusting the conditional(s) in patch 1 would then > be a functional change that's not really the purpose of that > patch - it really only folds acpi_ivrs_init()'s and > acpi_parse_dmar()'s into a vendor-independent instance in > acpi_iommu_init().
Right. > Alternatively we could adjust the conditional > here (in patch 3), but that would feel unrelated once again, as > this change is supposed to be AMD-specific. Depending on what we end up doing regarding interrupt remapping being disabled with iommu=off we might want to rework patch 3. > > My preference would be for the latter, because I think the resulting > > interface would be clearer. That will require introducing a new > > dmaremap iommu suboption, but again I think this will result in a > > better interface overall. > > I guess we could do with a 3rd opinion: Paul, any chance? > > In any event I hope that we can agree that patches 1 and 2 are > okay for 4.16 in their present shape, and patch 3 (plus whichever > further ones) would better wait for post-4.16? I consider the issues either a bug in the documentation or the code, so it's likely I would suggest whatever fix we end up doing to be backported. At which point it might make sense to add to the release. I don't think it should be a blocked though, as this hasn't been introduced in this release. Thanks, Roger.
