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

[Andrew Cooper <andrew.cooper3@xxxxxxxxxx>]

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?

~Andrew