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

[Jan Beulich <jbeulich@xxxxxxxx>]

On 03.02.2025 15:19, Andrew Cooper wrote:
> On 03/02/2025 8:41 am, Jan Beulich wrote:
>> 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.
> 
> Ok, not the reason I was thinking.  I was thinking it was an x vs x2
> APIC issue, and split setup path.
> 
> It is specifically weird to have:
> 
>     if ( msi )
>     {
>         if ( cap_xt_en )
>             ...
>         else
>         {
>             ...
>             amd_iommu_msi_enable();
>         }
>         // should enable here ?
>     }
> 
> If this call really is only necessary for the S3 path, that explains
> half the problem, but what activates MSIs for the xt_en case after S3?

The write of the control register where the enable bit is. There's no
actual "MSI" anymore in that case.

Jan