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

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()).
---
v2: New.

--- a/xen/drivers/passthrough/amd/iommu_intr.c
+++ b/xen/drivers/passthrough/amd/iommu_intr.c
@@ -731,8 +731,7 @@ bool __init iov_supports_xt(void)
 {
     unsigned int apic;
 
-    if ( !iommu_enable || !iommu_intremap )
-        return false;
+    ASSERT(iommu_enable || iommu_intremap);
 
     if ( amd_iommu_prepare(true) )
         return false;
--- a/xen/drivers/passthrough/amd/pci_amd_iommu.c
+++ b/xen/drivers/passthrough/amd/pci_amd_iommu.c
@@ -199,8 +199,7 @@ int __init acpi_ivrs_init(void)
 
 static int __init iov_detect(void)
 {
-    if ( !iommu_enable && !iommu_intremap )
-        return 0;
+    ASSERT(iommu_enable || iommu_intremap);
 
     if ( (init_done ? amd_iommu_init_late()
                     : amd_iommu_init(false)) != 0 )

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

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

> ---
> v2: New.
> 
> --- a/xen/drivers/passthrough/amd/iommu_intr.c
> +++ b/xen/drivers/passthrough/amd/iommu_intr.c
> @@ -731,8 +731,7 @@ bool __init iov_supports_xt(void)
>  {
>      unsigned int apic;
>  
> -    if ( !iommu_enable || !iommu_intremap )
> -        return false;
> +    ASSERT(iommu_enable || iommu_intremap);

I think this should be && in order to match my comments above.

Thanks, Roger.

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@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.
With that ...

>> ---
>> v2: New.
>>
>> --- a/xen/drivers/passthrough/amd/iommu_intr.c
>> +++ b/xen/drivers/passthrough/amd/iommu_intr.c
>> @@ -731,8 +731,7 @@ bool __init iov_supports_xt(void)
>>  {
>>      unsigned int apic;
>>  
>> -    if ( !iommu_enable || !iommu_intremap )
>> -        return false;
>> +    ASSERT(iommu_enable || iommu_intremap);
> 
> I think this should be && in order to match my comments above.

... I think || is correct to use here.

Jan