On Thu, 2 Mar 2023 at 12:37, Paolo Bonzini <[email protected]> wrote:
>
> From: David Woodhouse <[email protected]>
>
> 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 ?
thanks
-- PMM
