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

[Peter Maydell]

On Tue, 19 Dec 2023 at 13:36, Peter Maydell <peter.maydell@linaro.org> 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.
> >
> > There is a special MSI message which targets a PIRQ. The vector in the
> > low bits of data must be zero. The low 8 bits of the PIRQ# are in the
> > destination ID field, the extended destination ID field is unused, and
> > instead the high bits of the PIRQ# are in the high 32 bits of the address.
>
> Hi; Coverity thinks this change introduced a locking error
> (CID 1527403):
>
>
> > @@ -1226,21 +1256,54 @@ int xen_evtchn_bind_pirq_op(struct evtchn_bind_pirq 
> > *pirq)
> >          return -EINVAL;
> >      }
> >
> > -    QEMU_LOCK_GUARD(&s->port_lock);
> > +    QEMU_IOTHREAD_LOCK_GUARD();
>
> We used to take the port_lock before looking at s->pirq[pirq->pirq].port,
> but now we don't...
>
> >      if (s->pirq[pirq->pirq].port) {
> >          return -EBUSY;
> >      }
> >
> > +    qemu_mutex_lock(&s->port_lock);
>
> ...until down here after that "exit if already allocated" check.
> So Coverity thinks that two threads might both get into
> the "take the lock, allocate a port, set the port field in the
> struct" codepath simultaneously.
>
> > +
> >      ret = allocate_port(s, 0, EVTCHNSTAT_pirq, pirq->pirq,
> >                          &pirq->port);
> >      if (ret) {
> > +        qemu_mutex_unlock(&s->port_lock);
> >          return ret;
> >      }
> >
> >      s->pirq[pirq->pirq].port = pirq->port;
> >      trace_kvm_xen_bind_pirq(pirq->pirq, pirq->port);
> >
> > +    qemu_mutex_unlock(&s->port_lock);
> > +
>
> I think in practice the iothread-lock guard will prevent this,
> but it does look rather odd. Is there a reason not to have
> the port lock for the whole stretch of "check whether we've
> already allocated this, and if not then allocate it" code ?

Ping? This is still in Coverity's list of untriaged issues.
I'm inclined to say we should take the port_lock before the
EBUSY check. Using the WITH_QEMU_LOCK_GUARD(&s->port_lock) {...}
would make this straightforward and avoid having to do
explicit unlock in the early-return codepath (both here and
in the allocate_port() failed codepath).

thanks
-- PMM