On Thu, 2 Mar 2023 at 12:50, Paolo Bonzini <pbonz...@redhat.com> wrote:
>
> From: David Woodhouse <d...@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 <d...@amazon.co.uk>
> Reviewed-by: Paul Durrant <p...@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
