Re: [PATCH for-4.20? 1/3] AMD/IOMMU: drop stray MSI enabling

[Jan Beulich <jbeulich@xxxxxxxx>]

On 02.02.2025 14:50, Andrew Cooper wrote:
> On 30/01/2025 11:11 am, Jan Beulich wrote:
>> While the 2nd of the commits referenced below should have moved the call
>> to amd_iommu_msi_enable() instead of adding another one, the situation
>> wasn't quite right even before: It can't have done any good to enable
>> MSI when no IRQ was allocated for it, yet.
>>
>> Fixes: 5f569f1ac50e ("AMD/IOMMU: allow enabling with IRQ not yet set up")
>> Fixes: d9e49d1afe2e ("AMD/IOMMU: adjust setup of internal interrupt for 
>> x2APIC mode")
>> Signed-off-by: Jan Beulich <jbeulich@xxxxxxxx>
>>
>> --- a/xen/drivers/passthrough/amd/iommu_init.c
>> +++ b/xen/drivers/passthrough/amd/iommu_init.c
>> @@ -902,8 +902,6 @@ static void enable_iommu(struct amd_iomm
> 
> There's a call to amd_iommu_msi_enable() just out of context here which
> was added by the 2nd referenced commit.
> 
> Given that it's asymmetric in an if() condition regarding xt_en, and the
> calls are only set_affinity() calls, why is this retained?
> 
> (I think I know, and if it is the reason I suspect, then you're missing
> a very critical detail from the commit message.)

Hmm, you did read the commit message, didn't you? That commit should have
moved that call, rather than adding another one.

However, you have a point. It looks like 7a89f62dddee ("AMD IOMMU: make
interrupt work again") should already have removed that call. Prior to
that change request_irq()'s call (via setup_irq()) to iommu_msi_startup()
was in fact premature, as MSI address and data weren't set up yet (IOW
while still apparently redundant, the extra call served kind of a doc
purpose). Things apparently worked because the IOMMU itself wasn't
enabled yet, and hence shouldn't have raised any interrupts prior to MSI
being fully configured.

However, for S3 resume I think the call needs to stay there, as the
startup hook wouldn't be called in that case (which may be the detail
you're alluding to). Imo that wants solving differently though. Not sure
it's a good idea to do this right here, or perhaps better in a separate
change.

I've added

"The other call to amd_iommu_msi_enable(), just out of patch context,
 needs to stay there until S3 resume is re-worked. For the boot path that
 call should be unnecessary, as iommu{,_maskable}_msi_startup() will have
 done it already (by way of invoking iommu_msi_unmask())."

as a 2nd paragraph to the description, in the hope that's what you're
after.

Jan