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