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

[Roger Pau Monné <roger.pau@xxxxxxxxxx>]

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.

Hope this makes more sense, sorry for not understanding your question
initially.

Thanks, Roger.