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

[Andrew Cooper <andrew.cooper3@xxxxxxxxxx>]

On 03/02/2025 3:53 pm, Jan Beulich wrote:
> 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.

Oh, right.

I definitely knew that at one point, and I've clearly forgotten.

I wonder if we want a /* Note, no MSI in this case */ inside the if(),
but that might be a stretch...

~Andrew