Re: [PULL 56/62] hw/xen: Support GSI mapping to PIRQ

[Peter Maydell]

On Thu, 2 Mar 2023 at 12:50, Paolo Bonzini <pbonzini@redhat.com> wrote:
>
> From: David Woodhouse <dwmw@amazon.co.uk>
>
> If I advertise XENFEAT_hvm_pirqs then a guest now boots successfully as
> long as I tell it 'pci=nomsi'.
>
> [root@localhost ~]# cat /proc/interrupts
>            CPU0
>   0:         52   IO-APIC   2-edge      timer
>   1:         16  xen-pirq   1-ioapic-edge  i8042
>   4:       1534  xen-pirq   4-ioapic-edge  ttyS0
>   8:          1  xen-pirq   8-ioapic-edge  rtc0
>   9:          0  xen-pirq   9-ioapic-level  acpi
>  11:       5648  xen-pirq  11-ioapic-level  ahci[0000:00:04.0]
>  12:        257  xen-pirq  12-ioapic-edge  i8042
> ...
>
> Signed-off-by: David Woodhouse <dwmw@amazon.co.uk>
> Reviewed-by: Paul Durrant <paul@xen.org>

Hi; Coverity points out an off-by-one error in this code
(CID 1508128):

> @@ -148,6 +148,9 @@ struct XenEvtchnState {
>      /* GSI → PIRQ mapping (serialized) */
>      uint16_t gsi_pirq[IOAPIC_NUM_PINS];

The gsi_pirq[] array has IOAPIC_NUM_PINS elements...

> +bool xen_evtchn_set_gsi(int gsi, int level)
> +{
> +    XenEvtchnState *s = xen_evtchn_singleton;
> +    int pirq;
> +
> +    assert(qemu_mutex_iothread_locked());
> +
> +    if (!s || gsi < 0 || gsi > IOAPIC_NUM_PINS) {

...but here our guard on gsi is off-by-one, and
allows gsi == IOAPIC_NUM_PINS through...

> +        return false;
> +    }
> +
> +    /*
> +     * Check that that it *isn't* the event channel GSI, and thus
> +     * that we are not recursing and it's safe to take s->port_lock.
> +     *
> +     * Locking aside, it's perfectly sane to bail out early for that
> +     * special case, as it would make no sense for the event channel
> +     * GSI to be routed back to event channels, when the delivery
> +     * method is to raise the GSI... that recursion wouldn't *just*
> +     * be a locking issue.
> +     */
> +    if (gsi && gsi == s->callback_gsi) {
> +        return false;
> +    }
> +
> +    QEMU_LOCK_GUARD(&s->port_lock);
> +
> +    pirq = s->gsi_pirq[gsi];

...and we will reference off the end of the gsi_pirq[] array here.

> +    if (!pirq) {
> +        return false;
> +    }

Presumably the guard should be 'gsi >= IOAPIC_NUM_PINS' ?

thanks
-- PMM