Re: [PATCH] x86/msi: always propagate MSI writes when not in active system mode

[Jan Beulich <jbeulich@xxxxxxxx>]

On 18.03.2025 11:45, Roger Pau Monné wrote:
> On Tue, Mar 18, 2025 at 11:14:59AM +0100, Jan Beulich wrote:
>> On 18.03.2025 09:54, Roger Pau Monné wrote:
>>> On Tue, Mar 18, 2025 at 09:36:37AM +0100, Jan Beulich wrote:
>>>> On 18.03.2025 09:29, Roger Pau Monne wrote:
>>>>> --- a/xen/drivers/passthrough/amd/iommu_intr.c
>>>>> +++ b/xen/drivers/passthrough/amd/iommu_intr.c
>>>>> @@ -546,7 +546,7 @@ int cf_check amd_iommu_msi_msg_update_ire(
>>>>>      rc = update_intremap_entry_from_msi_msg(iommu, bdf, nr,
>>>>>                                              &msi_desc->remap_index,
>>>>>                                              msg, &data);
>>>>> -    if ( rc > 0 )
>>>>> +    if ( rc >= 0 )
>>>>>      {
>>>>>          for ( i = 1; i < nr; ++i )
>>>>>              msi_desc[i].remap_index = msi_desc->remap_index + i;
>>>>
>>>> I understand that Marek's testing has made clear that this change is 
>>>> needed,
>>>> yet I don't understand it. If we didn't allocate a new index, why would we
>>>> need to update in-memory state, when memory is preserved across S3?
>>>
>>> Is this always the case for device memory? (iow: contents of the BARs
>>> and possibly the PCI config space?)
>>
>> Of course not. But msi_desc[] is in RAM.
> 
> Sorry, I think I didn't understand your earlier question, and hence
> the reply I provided didn't make any sense to you.
> 
>>>> (This
>>>> lack of understanding on my part is why I didn't associate the last
>>>> paragraph of the description with this extra change, when you first sent it
>>>> in this shape on the original thread.)
>>>
>>> At least for the AMD IOMMU driver it seems to be expected.  See how
>>> amd_iommu_resume() performs a pair of disable_iommu() and
>>> enable_iommu() calls, and in the enable_iommu() function there's a
>>> call to set_{msi,x2apic}_affinity() that's expected to (re)set the
>>> interrupts.  Or at least that would be my understanding.
>>>
>>> This change reverts the behavior to what it used to be prior to
>>> 8e60d47cf011 for the suspend and resume paths.  I'm afraid I don't
>>> have a sensible way to test changes in that area, so I cannot
>>> investigate much.
>>
>> So how did you end up considering this may have been the reason for the
>> failure Marek was still seeing with the earlier form of the patch? I'm
>> simply hesitant to ack something that I don't understand at all.
> 
> Oh, I think I know what you are missing, and it's because it's out of
> patch context.  The adjusted chunk in amd_iommu_msi_msg_update_ire()
> does:
> 
>     if ( rc >= 0 )
>     {
>         for ( i = 1; i < nr; ++i )
>             msi_desc[i].remap_index = msi_desc->remap_index + i;
>         msg->data = data;
>     }
> 
> Note how it sets msg->data, as otherwise the field won't be properly
> set, and hence the caller propagating the contents of `msg` to the
> registers would be incorrect.
> 
> The change forces msg->data to be correctly set when returning either
> 0 or 1, so that propagation to the hardware can be done in both
> cases.  Previously the contents of msg->data where only correct when
> returning 1 on AMD.

Oh, I see. The loop is entirely benign in this case. I did look at the
full function, but I didn't make the connection from the writing of
msg->data.

Reviewed-by: Jan Beulich <jbeulich@xxxxxxxx>

Jan