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

[Jan Beulich <jbeulich@xxxxxxxx>]

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

Jan