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 <jbeul...@suse.com>
Jan
