On Thu, 6 Apr 2023 at 17:25, Woodhouse, David <d...@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 <pbonz...@redhat.com> wrote:
> > >
> > > From: David Woodhouse <d...@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 ?
thanks
-- PMM
