Re: [PULL 57/62] hw/xen: Support MSI mapping to PIRQ

[David Woodhouse]

On Fri, 2023-06-23 at 14:27 +0100, Peter Maydell wrote:
> On Thu, 6 Apr 2023 at 17:25, Woodhouse, David <dwmw@amazon.co.uk> wrote:
> > 
> > On Thu, 2023-04-06 at 16:48 +0100, Peter Maydell wrote:
> > > On Thu, 2 Mar 2023 at 12:37, Paolo Bonzini <pbonzini@redhat.com> wrote:
> > > > 
> > > > From: David Woodhouse <dwmw@amazon.co.uk>
> > > > 
> > > > The way that Xen handles MSI PIRQs is kind of awful.
> > > 
> > > > Now that this is working we can finally enable XENFEAT_hvm_pirqs and
> > > > let the guest use it all.
> > > > 
> > > 
> > > Hi; Coverity points out a logic error in this code (CID 1507603):
> > > 
> > > > @@ -1638,6 +1877,7 @@ int xen_physdev_unmap_pirq(struct 
> > > > physdev_unmap_pirq *unmap)
> > > > 
> > > >      /* We can only unmap GSI PIRQs */
> > > >      if (gsi < 0) {
> > > > +        qemu_mutex_unlock(&s->port_lock);
> > > >          return -EINVAL;
> > > >      }
> > > 
> > > One of the things xen_physdev_unmap_pirq() does early is return
> > > if gsi is a negative value...
> > > 
> > > > @@ -1646,6 +1886,12 @@ int xen_physdev_unmap_pirq(struct 
> > > > physdev_unmap_pirq *unmap)
> > > >      pirq_inuse_word(s, pirq) &= ~pirq_inuse_bit(pirq);
> > > > 
> > > >      trace_kvm_xen_unmap_pirq(pirq, gsi);
> > > > +    qemu_mutex_unlock(&s->port_lock);
> > > > +
> > > > +    if (gsi == IRQ_MSI_EMU) {
> > > 
> > > ...but then later we try to test to see if it is IRQ_MSI_EMU.
> > > IRQ_MSI_EMU is -3, so this condition can never be true.
> > > 
> > > > +        kvm_update_msi_routes_all(NULL, true, 0, 0);
> > > > +    }
> > > 
> > > What was the intention here ?
> > 
> > 
> > Hrm.... the way that Xen automatically maps the MSI to a PIRQ by
> > snooping on the (masked) writes to the MSI target is awful, as noted.
> > 
> > I don't think Linux guests ever do unmap the MSI PIRQ but it might be
> > possible; I'll have to do some experiments in Xen to see what happens.
> 
> Did you ever figure out what the right thing here was ?

Yeah, I do I think that Xen will actually allow the guest to unmap the
IRQ_MSI_EMU pirq, and it will mask the underlying MSI when it does so,
just as it *unmasks* the MSI when the guest binds the irq.

But that's partly because Xen handles the awfulness with snooping on
the MSI table writes differently. The PIRQ isn't automatically marked
as IRQ_MSI_EMU when the MSI message is written; there's a different
structure which handles that. And the PIRQ is only set to IRQ_MSI_EMU
when it's actually *delivered*, AFAICT.

Emulating that more faithfully might actually simplify the locking in
qemu, which is fairly complex around the MSI→PIRQ→MSI call paths which
need care to avoid deadlock. I'll see if I can make it simpler.

In the short term though, I think it's harmless that we don't support
unmapping IRQ_MSI_EMU because AFAICT nobody actually *does* it. But
I'll look at fixing it anyway, and that dead code will no longer be
dead.

smime.p7s
Description: S/MIME cryptographic signature