Re: [PATCH v2 3/3] AMD/IOMMU: iommu_enable vs iommu_intremap

[Roger Pau Monné <roger.pau@xxxxxxxxxx>]

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 <jbeulich@xxxxxxxx>
> >>>>>> ---
> >>>>>> 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.