| Message ID | 20230302123029.153265-57-pbonzini@redhat.com |
|---|---|
| State | New |
| Headers | show |
| Series | [PULL,01/62] include: import Xen public headers to hw/xen/interface | expand |
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
On Tue, 4 Jul 2023 at 16:13, Woodhouse, David <dwmw@amazon.co.uk> wrote: > > Coverity points out (CID 1508128) a bounds checking error. We need to check > for gsi >= IOAPIC_NUM_PINS, not just greater-than. > > Also fix up an assert() that has the same problem, that Coverity didn't see. > > Signed-off-by: David Woodhouse <dwmw@amazon.co.uk> > --- > hw/i386/kvm/xen_evtchn.c | 4 ++-- > 1 file changed, 2 insertions(+), 2 deletions(-) > > diff --git a/hw/i386/kvm/xen_evtchn.c b/hw/i386/kvm/xen_evtchn.c > index 3d810dbd59..0e9c108614 100644 > --- a/hw/i386/kvm/xen_evtchn.c > +++ b/hw/i386/kvm/xen_evtchn.c > @@ -1587,7 +1587,7 @@ static int allocate_pirq(XenEvtchnState *s, int type, int gsi) > found: > pirq_inuse_word(s, pirq) |= pirq_inuse_bit(pirq); > if (gsi >= 0) { > - assert(gsi <= IOAPIC_NUM_PINS); > + assert(gsi < IOAPIC_NUM_PINS); > s->gsi_pirq[gsi] = pirq; > } > s->pirq[pirq].gsi = gsi; > @@ -1601,7 +1601,7 @@ bool xen_evtchn_set_gsi(int gsi, int level) > > assert(qemu_mutex_iothread_locked()); > > - if (!s || gsi < 0 || gsi > IOAPIC_NUM_PINS) { > + if (!s || gsi < 0 || gsi >= IOAPIC_NUM_PINS) { > return false; > } Reviewed-by: Peter Maydell <peter.maydell@linaro.org> thanks -- PMM
Hi David, On 4/7/23 17:12, Woodhouse, David via wrote: > Coverity points out (CID 1508128) a bounds checking error. We need to check > for gsi >= IOAPIC_NUM_PINS, not just greater-than. > > Also fix up an assert() that has the same problem, that Coverity didn't see. > > Signed-off-by: David Woodhouse <dwmw@amazon.co.uk> > --- > hw/i386/kvm/xen_evtchn.c | 4 ++-- > 1 file changed, 2 insertions(+), 2 deletions(-) Reviewed-by: Philippe Mathieu-Daudé <philmd@linaro.org> Better to post new patches as new thread: Patches are easier to find if they start a new top-level thread, rather than being buried in-reply-to another existing thread. (Per https://www.qemu.org/docs/master/devel/submitting-a-patch.html#use-git-format-patch) Regards, Phil.
diff --git a/hw/i386/kvm/xen_evtchn.c b/hw/i386/kvm/xen_evtchn.c index 59eea63272d0..f2c4b4387194 100644 --- a/hw/i386/kvm/xen_evtchn.c +++ b/hw/i386/kvm/xen_evtchn.c @@ -148,6 +148,9 @@ struct XenEvtchnState { /* GSI → PIRQ mapping (serialized) */ uint16_t gsi_pirq[IOAPIC_NUM_PINS]; + /* Per-GSI assertion state (serialized) */ + uint32_t pirq_gsi_set; + /* Per-PIRQ information (rebuilt on migration) */ struct pirq_info *pirq; }; @@ -246,6 +249,7 @@ static const VMStateDescription xen_evtchn_vmstate = { VMSTATE_VARRAY_UINT16_ALLOC(pirq_inuse_bitmap, XenEvtchnState, nr_pirq_inuse_words, 0, vmstate_info_uint64, uint64_t), + VMSTATE_UINT32(pirq_gsi_set, XenEvtchnState), VMSTATE_END_OF_LIST() } }; @@ -1510,6 +1514,51 @@ static int allocate_pirq(XenEvtchnState *s, int type, int gsi) return pirq; } +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) { + 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]; + if (!pirq) { + return false; + } + + if (level) { + int port = s->pirq[pirq].port; + + s->pirq_gsi_set |= (1U << gsi); + if (port) { + set_port_pending(s, port); + } + } else { + s->pirq_gsi_set &= ~(1U << gsi); + } + return true; +} + int xen_physdev_map_pirq(struct physdev_map_pirq *map) { XenEvtchnState *s = xen_evtchn_singleton; @@ -1621,7 +1670,14 @@ int xen_physdev_eoi_pirq(struct physdev_eoi *eoi) return -EINVAL; } - /* XX: Reassert a level IRQ if needed */ + /* Reassert a level IRQ if needed */ + if (s->pirq_gsi_set & (1U << gsi)) { + int port = s->pirq[pirq].port; + if (port) { + set_port_pending(s, port); + } + } + return 0; } diff --git a/hw/i386/kvm/xen_evtchn.h b/hw/i386/kvm/xen_evtchn.h index a7383f760ce2..95400b7fbf7d 100644 --- a/hw/i386/kvm/xen_evtchn.h +++ b/hw/i386/kvm/xen_evtchn.h @@ -24,6 +24,8 @@ void xen_evtchn_set_callback_level(int level); int xen_evtchn_set_port(uint16_t port); +bool xen_evtchn_set_gsi(int gsi, int level); + /* * These functions mirror the libxenevtchn library API, providing the QEMU * backend side of "interdomain" event channels. diff --git a/hw/i386/x86.c b/hw/i386/x86.c index c44846f47b5f..a56b10b2fb1c 100644 --- a/hw/i386/x86.c +++ b/hw/i386/x86.c @@ -61,6 +61,11 @@ #include CONFIG_DEVICES #include "kvm/kvm_i386.h" +#ifdef CONFIG_XEN_EMU +#include "hw/xen/xen.h" +#include "hw/i386/kvm/xen_evtchn.h" +#endif + /* Physical Address of PVH entry point read from kernel ELF NOTE */ static size_t pvh_start_addr; @@ -610,6 +615,17 @@ void gsi_handler(void *opaque, int n, int level) } /* fall through */ case ISA_NUM_IRQS ... IOAPIC_NUM_PINS - 1: +#ifdef CONFIG_XEN_EMU + /* + * Xen delivers the GSI to the Legacy PIC (not that Legacy PIC + * routing actually works properly under Xen). And then to + * *either* the PIRQ handling or the I/OAPIC depending on + * whether the former wants it. + */ + if (xen_mode == XEN_EMULATE && xen_evtchn_set_gsi(n, level)) { + break; + } +#endif qemu_set_irq(s->ioapic_irq[n], level); break; case IO_APIC_SECONDARY_IRQBASE