On 03.01.2013, at 20:54, Scott Wood wrote: > On 01/03/2013 12:55:26 PM, Alexander Graf wrote: >> On 22.12.2012, at 03:15, Scott Wood wrote: >> > The two checks with abort() guard against potential QEMU-internal >> > problems, but the EOI check stops the guest from causing updates to queue >> > position -1 and other havoc if it writes EOI with no interrupt in >> > service. >> > >> > Signed-off-by: Scott Wood <scottw...@freescale.com> >> Did you ever actually experience this? > > Which one? EOI with no interrupt in service can be triggered by bad guest > behavior, and I did see it happen when the guest was confused by another bug > in QEMU's openpic (which is fixed elsewhere), resulting in an IRQ number of > -1 being thrown around.
That's the last hunk, which as I said is fine :). > The other checks were to try to be more robust against bad IRQ numbers in > general. > >> MAX_IRQ should match the memory region size, so we shouldn't be able to >> receive any interrupt above it. > > Right, that's why I didn't add checking to the MMIO code. In IRQ_check it > could happen due to bad bitmap contents (e.g. after a checkpoint restore), > and in openpic_set_irq() it could happen if some device raises an IRQ that is > out of bounds. How would a device raise an IRQ that is out of bounds? Devices can only raise IRQs that are passed down from the init function and that only creates MAX_INT irq lines. > >> I might be inclined to accept an assert() there for internal sanity checking >> though. The last hunk looks fine. > > Assert instead of abort is fine (there seem to be plenty of uses of both in > QEMU), though for the openpic_set_irq() case it would be nice to be able to > print the bad IRQ number before dying. Well, that's why I was asking where you've seen this happen. It really shouldn't. Ever. :) But the checks as is don't hurt either. If it makes you happy, I can apply the patch. Alex
