| Message ID | dc9c8d2aa7bfcd82ba812f1ff5630a4cc0ffbbe7.1616519655.git.isaku.yamahata@intel.com (mailing list archive) |
|---|---|
| State | New, archived |
| Headers | show |
| Series | reinitialize ACPI PM device on reset | expand |
On Tue, 23 Mar 2021 at 17:26, Isaku Yamahata <isaku.yamahata@intel.com> wrote: > > If a device model > (a) doesn't set the value to a correct interrupt number and then > (b) triggers an interrupt for itself, > it's device model bug. Add assert on interrupt pin number to catch > this kind of bug more obviously. > > Suggested-by: Peter Maydell <Peter.maydel@linaro.org> > Signed-off-by: Isaku Yamahata <isaku.yamahata@intel.com> > --- > hw/pci/pci.c | 10 +++++++++- > 1 file changed, 9 insertions(+), 1 deletion(-) > > diff --git a/hw/pci/pci.c b/hw/pci/pci.c > index ac9a24889c..cb6bab999b 100644 > --- a/hw/pci/pci.c > +++ b/hw/pci/pci.c > @@ -1450,6 +1450,8 @@ static void pci_irq_handler(void *opaque, int irq_num, int level) > PCIDevice *pci_dev = opaque; > int change; > > + assert(0 <= irq_num && irq_num < PCI_NUM_PINS); > + assert(level == 0 || level == 1); If you have these... > change = level - pci_irq_state(pci_dev, irq_num); > if (!change) > return; > @@ -1463,7 +1465,13 @@ static void pci_irq_handler(void *opaque, int irq_num, int level) > > static inline int pci_intx(PCIDevice *pci_dev) > { > - return pci_get_byte(pci_dev->config + PCI_INTERRUPT_PIN) - 1; > + int intx = pci_get_byte(pci_dev->config + PCI_INTERRUPT_PIN) - 1; > + /* > + * This function is used to setup/trigger irq. > + * So PIN = 0 (interrupt isn't used) doesn't make sense. > + */ > + assert(0 <= intx && intx < PCI_NUM_PINS); ...you don't need this, because the assert in pci_irq_handler() covers all the uses of pci_intx(). See also https://patchew.org/QEMU/20210323164601.27200-1-peter.maydell@linaro.org/ thanks -- PMM
On Tue, Mar 23, 2021 at 10:24:30AM -0700, Isaku Yamahata wrote: > If a device model > (a) doesn't set the value to a correct interrupt number and then > (b) triggers an interrupt for itself, > it's device model bug. Add assert on interrupt pin number to catch > this kind of bug more obviously. > > Suggested-by: Peter Maydell <Peter.maydel@linaro.org> > Signed-off-by: Isaku Yamahata <isaku.yamahata@intel.com> > --- > hw/pci/pci.c | 10 +++++++++- > 1 file changed, 9 insertions(+), 1 deletion(-) > > diff --git a/hw/pci/pci.c b/hw/pci/pci.c > index ac9a24889c..cb6bab999b 100644 > --- a/hw/pci/pci.c > +++ b/hw/pci/pci.c > @@ -1450,6 +1450,8 @@ static void pci_irq_handler(void *opaque, int irq_num, int level) > PCIDevice *pci_dev = opaque; > int change; > > + assert(0 <= irq_num && irq_num < PCI_NUM_PINS); > + assert(level == 0 || level == 1); > change = level - pci_irq_state(pci_dev, irq_num); > if (!change) > return; > @@ -1463,7 +1465,13 @@ static void pci_irq_handler(void *opaque, int irq_num, int level) > > static inline int pci_intx(PCIDevice *pci_dev) > { > - return pci_get_byte(pci_dev->config + PCI_INTERRUPT_PIN) - 1; > + int intx = pci_get_byte(pci_dev->config + PCI_INTERRUPT_PIN) - 1; > + /* > + * This function is used to setup/trigger irq. > + * So PIN = 0 (interrupt isn't used) doesn't make sense. Well not really. It just returns it. But of course value -1 is not a valid index. Better: callers must make sure the device actually has an interrupt pin, otherwise PCI_INTERRUPT_PIN is 0 and intx is negative here. > + */ > + assert(0 <= intx && intx < PCI_NUM_PINS); > + return intx; > } > > qemu_irq pci_allocate_irq(PCIDevice *pci_dev) > -- > 2.25.1
diff --git a/hw/pci/pci.c b/hw/pci/pci.c index ac9a24889c..cb6bab999b 100644 --- a/hw/pci/pci.c +++ b/hw/pci/pci.c @@ -1450,6 +1450,8 @@ static void pci_irq_handler(void *opaque, int irq_num, int level) PCIDevice *pci_dev = opaque; int change; + assert(0 <= irq_num && irq_num < PCI_NUM_PINS); + assert(level == 0 || level == 1); change = level - pci_irq_state(pci_dev, irq_num); if (!change) return; @@ -1463,7 +1465,13 @@ static void pci_irq_handler(void *opaque, int irq_num, int level) static inline int pci_intx(PCIDevice *pci_dev) { - return pci_get_byte(pci_dev->config + PCI_INTERRUPT_PIN) - 1; + int intx = pci_get_byte(pci_dev->config + PCI_INTERRUPT_PIN) - 1; + /* + * This function is used to setup/trigger irq. + * So PIN = 0 (interrupt isn't used) doesn't make sense. + */ + assert(0 <= intx && intx < PCI_NUM_PINS); + return intx; } qemu_irq pci_allocate_irq(PCIDevice *pci_dev)
If a device model (a) doesn't set the value to a correct interrupt number and then (b) triggers an interrupt for itself, it's device model bug. Add assert on interrupt pin number to catch this kind of bug more obviously. Suggested-by: Peter Maydell <Peter.maydel@linaro.org> Signed-off-by: Isaku Yamahata <isaku.yamahata@intel.com> --- hw/pci/pci.c | 10 +++++++++- 1 file changed, 9 insertions(+), 1 deletion(-)