Re: [RFC PATCH] xen/amd-iommu: Add interrupt remapping quirk for ath11k

[Jason Andryuk <jason.andryuk@xxxxxxx>]

On 2025-02-28 04:36, Roger Pau Monné wrote:
> On Thu, Feb 27, 2025 at 01:28:11PM -0500, Jason Andryuk wrote:
> > On 2025-02-27 05:23, Roger Pau Monné wrote:
> > > On Wed, Feb 26, 2025 at 04:11:25PM -0500, Jason Andryuk wrote:
> > > > To work around this, we can, for per-device IRTs, program the hardware
> > > > to use the guest data & associated IRTE.  The address doesn't matter
> > > > since the IRTE handles that, and the Xen address & vector can be used as
> > > > expected.
> > > All this work on AMD because when interrupt remapping is enabled all
> > > MSIs are handled by the remapping table, while on Intel there's still
> > > a bit in the MSI address field to signal whether the MSI is using a
> > > remapping entry, or is using the "compatibility" format (iow: no
> > > remapping).
> > So, on Intel, if the guest hands the device the MSI address, it can decided
> > to bypass remapping?
> >
> > Thanks for providing insight into the Intel inner workings.  That's why I am
> > asking.
> Yes, sorry, I'm afraid I don't have any good solution for Intel, at
> least not anything similar to what you propose to do on AMD-Vi.  I
> guess we could take a partial solution for AMD-Vi only, but it's
> sub-optimal from Xen perspective to have a piece of hardware working
> fine on AMD and not on Intel.
I only need AMD to work ;)

But yeah, I thought I should make an effort to get both working.

> > > > For vPCI, the guest MSI data is available at the time of initial MSI
> > > > setup, but that is not the case for HVM.  With HVM, the initial MSI
> > > > setup is done when PHYSDEVOP_map_pirq is called, but the guest vector is
> > > > only available later when XEN_DOMCTL_bind_pt_irq is called.  In that
> > > > case, we need to tear down and create a new IRTE.  This later location
> > > > can also handle vPCI.
> > > >
> > > > Add pirq_guest_bind_gvec to plumb down the gvec without modifying all
> > > > call sites.  Use msi_desc->gvec to pass through the desired value.
> > > So basically the solution is to use the guest selected MSI vector as
> > > the interrupt remapping table index, as then the guest can use the MSI
> > > data and address fields without requiring Xen translation.
> > >
> > > What about the guest using the same vector across multiple vCPUs?  So
> > > MSI entries having the same vector field, but different target
> > > destination CPUs?  That won't work correctly as all those MSIs will
> > > attempt to use the same IRTE?
> I think you will also need to add some extra checks to ensure that
> when this quirk is active the guest will always set APIC ID 0 as the
> interrupt destination for all MSI entries for the affected device, so
> that there cannot be vector overlap between CPUs.  Otherwise the quirk
> won't work as expected.
Ok.

> > > > e.g. Replace amd_iommu_perdev_intremap with something generic.
> > > >
> > > > The ath11k device supports and tries to enable 32 MSIs.  Linux in PVH
> > > > dom0 and HVM domU fails enabling 32 and falls back to just 1, so that is
> > > > all that has been tested.
> > > DYK why it fails to enable 32?
> > Not exactly - someone else had the card.  msi_capability_init() failed. If
> > it ends up in arch_setup_msi_irqs(), only 1 MSI is supported.  But precisely
> > where the mutiple nvecs was denied was not tracked down.
> Does it also fail on native?  I'm mostly asking because it would be
> good to get to the bottom of this, so that we don't come up with a
> partial solution that will break if multi-msi is used later in Linux.
My understanding is native and PV dom0 work with 32, and it's Linux 
deciding not to use multiple MSI.
It might be this:
static int xen_hvm_setup_msi_irqs(struct pci_dev *dev, int nvec, int type)
{
        int irq, pirq;
        struct msi_desc *msidesc;
        struct msi_msg msg;

        if (type == PCI_CAP_ID_MSI && nvec > 1)
                return 1;

I'll have to look into this more.


> > > > +            new_remap_index = msi_desc->gvec;
> > > > +        }
> > > > +
> > > > +        if ( new_remap_index && new_remap_index != msi_desc->remap_index &&
> > > > +             msi_desc->remap_index != -1 )
> > > > +        {
> > > > +            /* Clear any existing entries */
> > > > +            update_intremap_entry_from_msi_msg(iommu, bdf, nr,
> > > > +                                               &msi_desc->remap_index,
> > > > +                                               NULL, NULL);
> > > Why do you need to clear any entries?  This will cause a window where
> > > MSI entries targeting this IRTEs to generate faults because the
> > > entries are not setup.
> > >
> > > Just re-use them, update_intremap_entry_from_msi_msg() will update the
> > > IRTE atomically so that there's no window where the entries would be
> > > invalid, and thus to faults will be generated.
> > I see your point about the window.  I was trying to keep it clean as
> > different indices get populated.  Initially, IRT[0..n-1] is populated.
> Hm, I see.  For this specific use-case you are changing the IRT index
> when the guest updates the MSI vector.  Tearing down of the old
> entries would better be done _after_ the MSI entry has been updated,
> so that at all times the pointed IRTE is valid.
>
> > Later, when the gvec is available, we want IRT[gvec..gvec+n-1] populated.  I
> > guess the new gvec ones could be added, and then 0...gvec-1 removed.  Or
> > don't bother?
> Indeed, that would be a better approach, as then the IRTE would always
> be valid.
>
> In fact you could possibly leave the old IRTE entries as-is, they
> would be unhooked from any MSI entry, and if re-used they would be
> setup correctly.  For this specific quirk where vector == IRT index
> there's never the need to search for a free IRTE, as the guest set
> vector will dictate which IRTE to use.
>
> I guess it would be nice to attempt to keep the inuse IRTE bitmap in
> sync if possible.
>
> > I considered leaving IRTE[0] and adding IRTE[gvec].  I think that could
> > work, but would be more hacky.
> >
> > I was trying to keep the irte accounting bitmap correct, but it doesn't
> > really matter for per-device IRT.
> Yes, that's my thinking too.  If you can move the call to teardown the
> old IRTE after the new one has been setup and the MSI entry has been
> updated that would be the best approach IMO.
Ok.

Thanks,
Jason