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

[Jan Beulich <jbeulich@xxxxxxxx>]

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

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

Jan