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

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

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.

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.

Thanks, Roger.