Reinitialize ACPI PM device on reset to get preper device state. Oppertunistically add assert on PCI interrupt pin logic to detect device model issues and workaround for found one. Changes from v1: - bug work around hw/isa/vt82c686.c - add assert in hw/pci/pci.c when setting/raising PCI interrupt Isaku Yamahata (3): vt82c686.c: don't raise SCI when PCI_INTERRUPT_PIN isn't setup pci: sprinkle assert in PCI pin number acpi:piix4, vt82c686: reinitialize acpi PM device on reset hw/acpi/piix4.c | 7 +++++++ hw/isa/vt82c686.c | 18 +++++++++++++++++- hw/pci/pci.c | 10 +++++++++- 3 files changed, 33 insertions(+), 2 deletions(-) -- 2.25.1
Without this patch, the following patch will triger clan runtime sanitizer warnings as follows. This patch proactively works around it. I let v582c686.c maintainer address a correct fix as I'm not sure about fuloong2e device model. > MALLOC_PERTURB_=${MALLOC_PERTURB_:-$(( ${RANDOM:-0} % 255 + 1))} > QTEST_QEMU_IMG=./qemu-img > G_TEST_DBUS_DAEMON=/home/petmay01/linaro/qemu-for-merges/tests/dbus-vmstate-daemon.sh > QTEST_QEMU_BINARY=./qemu-system-mips64el tests/qtest/qom-test --tap -k > PASS 1 qtest-mips64el/qom-test /mips64el/qom/loongson3-virt > PASS 2 qtest-mips64el/qom-test /mips64el/qom/none > PASS 3 qtest-mips64el/qom-test /mips64el/qom/magnum > PASS 4 qtest-mips64el/qom-test /mips64el/qom/mipssim > PASS 5 qtest-mips64el/qom-test /mips64el/qom/malta > ../../hw/pci/pci.c:252:30: runtime error: shift exponent -1 is negative > PASS 6 qtest-mips64el/qom-test /mips64el/qom/fuloong2e > PASS 7 qtest-mips64el/qom-test /mips64el/qom/boston > PASS 8 qtest-mips64el/qom-test /mips64el/qom/pica61 > > and similarly for eg > > MALLOC_PERTURB_=${MALLOC_PERTURB_:-$(( ${RANDOM:-0} % 255 + 1))} > QTEST_QEMU_IMG=./qemu-img > G_TEST_DBUS_DAEMON=/home/petmay01/linaro/qemu-for-merges/tests/dbus-vmstate-daemon.sh > QTEST_QEMU_BINARY=./qemu-system-mips64el tests/qtest/endianness-test > --tap -k > ../../hw/pci/pci.c:252:30: runtime error: shift exponent -1 is negative > PASS 1 qtest-mips64el/endianness-test /mips64el/endianness/fuloong2e > ../../hw/pci/pci.c:252:30: runtime error: shift exponent -1 is negative > PASS 2 qtest-mips64el/endianness-test /mips64el/endianness/split/fuloong2e > ../../hw/pci/pci.c:252:30: runtime error: shift exponent -1 is negative > PASS 3 qtest-mips64el/endianness-test /mips64el/endianness/combine/fuloong2e Cc: Huacai Chen <chenhuacai@kernel.org> Cc: "Philippe Mathieu-Daudé" <f4bug@amsat.org> Cc: Jiaxun Yang <jiaxun.yang@flygoat.com> Reported-by: Peter Maydell <Peter.maydel@linaro.org> Signed-off-by: Isaku Yamahata <isaku.yamahata@intel.com> --- hw/isa/vt82c686.c | 13 ++++++++++++- 1 file changed, 12 insertions(+), 1 deletion(-) diff --git a/hw/isa/vt82c686.c b/hw/isa/vt82c686.c index 05d084f698..f0fb309f12 100644 --- a/hw/isa/vt82c686.c +++ b/hw/isa/vt82c686.c @@ -144,7 +144,18 @@ static void pm_update_sci(ViaPMState *s) ACPI_BITMASK_POWER_BUTTON_ENABLE | ACPI_BITMASK_GLOBAL_LOCK_ENABLE | ACPI_BITMASK_TIMER_ENABLE)) != 0); - pci_set_irq(&s->dev, sci_level); + if (pci_get_byte(s->dev.config + PCI_INTERRUPT_PIN)) { + /* + * FIXME: + * Fix device model that realizes this PM device and remove + * this work around. + * The device model should wire SCI and setup + * PCI_INTERRUPT_PIN properly. + * If PIN# = 0(interrupt pin isn't used), don't raise SCI as + * work around. + */ + pci_set_irq(&s->dev, sci_level); + } /* schedule a timer interruption if needed */ acpi_pm_tmr_update(&s->ar, (s->ar.pm1.evt.en & ACPI_BITMASK_TIMER_ENABLE) && !(pmsts & ACPI_BITMASK_TIMER_STATUS)); -- 2.25.1
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. + */ + assert(0 <= intx && intx < PCI_NUM_PINS); + return intx; } qemu_irq pci_allocate_irq(PCIDevice *pci_dev) -- 2.25.1
Commit 6be8cf56bc8b made sure that SCI is enabled in PM1.CNT on reset in acpi_only mode by modifying acpi_pm1_cnt_reset() and that worked for q35 as expected. The function was introduced by commit eaba51c573a (acpi, acpi_piix, vt82c686: factor out PM1_CNT logic) that forgot to actually call it at piix4 reset time and as result SCI_EN wasn't set as was expected by 6be8cf56bc8b in acpi_only mode. So Windows crashes when it notices that SCI_EN is not set and FADT is not providing information about how to enable it anymore. Reproducer: qemu-system-x86_64 -enable-kvm -M pc-i440fx-6.0,smm=off -cdrom any_windows_10x64.iso Fix it by calling acpi_pm1_cnt_reset() at piix4 reset time. Occasionally this patch adds reset acpi PM related registers on piix4/vt582c686 reset time and de-assert sci. piix4_pm_realize() initializes acpi pm tmr, evt, cnt and gpe. via_pm_realize() initializes acpi pm tmr, evt and cnt. reset them on device reset. pm_reset() in ich9.c correctly calls corresponding reset functions. Fixes: 6be8cf56bc8b (acpi/core: always set SCI_EN when SMM isn't supported) Reported-by: Reinoud Zandijk <reinoud@NetBSD.org> Co-developed-by: Igor Mammedov <imammedo@redhat.com> Signed-off-by: Igor Mammedov <imammedo@redhat.com> Signed-off-by: Isaku Yamahata <isaku.yamahata@intel.com> --- CC: imammedo@redhat.com CC: isaku.yamahata@intel.com CC: mst@redhat.com CC: reinoud@NetBSD.org CC: isaku.yamahata@gmail.com CC: berrange@redhat.com CC: pbonzini@redhat.com CC: f4bug@amsat.org CC: aurelien@aurel32.net --- hw/acpi/piix4.c | 7 +++++++ hw/isa/vt82c686.c | 5 +++++ 2 files changed, 12 insertions(+) diff --git a/hw/acpi/piix4.c b/hw/acpi/piix4.c index 6056d51667..8f8b0e95e5 100644 --- a/hw/acpi/piix4.c +++ b/hw/acpi/piix4.c @@ -326,6 +326,13 @@ static void piix4_pm_reset(DeviceState *dev) /* Mark SMM as already inited (until KVM supports SMM). */ pci_conf[0x5B] = 0x02; } + + acpi_pm1_evt_reset(&s->ar); + acpi_pm1_cnt_reset(&s->ar); + acpi_pm_tmr_reset(&s->ar); + acpi_gpe_reset(&s->ar); + acpi_update_sci(&s->ar, s->irq); + pm_io_space_update(s); acpi_pcihp_reset(&s->acpi_pci_hotplug, !s->use_acpi_root_pci_hotplug); } diff --git a/hw/isa/vt82c686.c b/hw/isa/vt82c686.c index f0fb309f12..98325bb32b 100644 --- a/hw/isa/vt82c686.c +++ b/hw/isa/vt82c686.c @@ -178,6 +178,11 @@ static void via_pm_reset(DeviceState *d) /* SMBus IO base */ pci_set_long(s->dev.config + 0x90, 1); + acpi_pm1_evt_reset(&s->ar); + acpi_pm1_cnt_reset(&s->ar); + acpi_pm_tmr_reset(&s->ar); + pm_update_sci(s); + pm_io_space_update(s); smb_io_space_update(s); } -- 2.25.1
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
Hi Isaku, On 3/23/21 6:24 PM, Isaku Yamahata wrote: > Without this patch, the following patch will triger clan runtime > sanitizer warnings as follows. This patch proactively works around it. > I let v582c686.c maintainer address a correct fix as I'm not sure > about fuloong2e device model. > >> MALLOC_PERTURB_=${MALLOC_PERTURB_:-$(( ${RANDOM:-0} % 255 + 1))} >> QTEST_QEMU_IMG=./qemu-img >> G_TEST_DBUS_DAEMON=/home/petmay01/linaro/qemu-for-merges/tests/dbus-vmstate-daemon.sh >> QTEST_QEMU_BINARY=./qemu-system-mips64el tests/qtest/qom-test --tap -k >> PASS 1 qtest-mips64el/qom-test /mips64el/qom/loongson3-virt >> PASS 2 qtest-mips64el/qom-test /mips64el/qom/none >> PASS 3 qtest-mips64el/qom-test /mips64el/qom/magnum >> PASS 4 qtest-mips64el/qom-test /mips64el/qom/mipssim >> PASS 5 qtest-mips64el/qom-test /mips64el/qom/malta >> ../../hw/pci/pci.c:252:30: runtime error: shift exponent -1 is negative >> PASS 6 qtest-mips64el/qom-test /mips64el/qom/fuloong2e >> PASS 7 qtest-mips64el/qom-test /mips64el/qom/boston >> PASS 8 qtest-mips64el/qom-test /mips64el/qom/pica61 >> >> and similarly for eg >> >> MALLOC_PERTURB_=${MALLOC_PERTURB_:-$(( ${RANDOM:-0} % 255 + 1))} >> QTEST_QEMU_IMG=./qemu-img >> G_TEST_DBUS_DAEMON=/home/petmay01/linaro/qemu-for-merges/tests/dbus-vmstate-daemon.sh >> QTEST_QEMU_BINARY=./qemu-system-mips64el tests/qtest/endianness-test >> --tap -k >> ../../hw/pci/pci.c:252:30: runtime error: shift exponent -1 is negative >> PASS 1 qtest-mips64el/endianness-test /mips64el/endianness/fuloong2e >> ../../hw/pci/pci.c:252:30: runtime error: shift exponent -1 is negative >> PASS 2 qtest-mips64el/endianness-test /mips64el/endianness/split/fuloong2e >> ../../hw/pci/pci.c:252:30: runtime error: shift exponent -1 is negative >> PASS 3 qtest-mips64el/endianness-test /mips64el/endianness/combine/fuloong2e > > Cc: Huacai Chen <chenhuacai@kernel.org> > Cc: "Philippe Mathieu-Daudé" <f4bug@amsat.org> > Cc: Jiaxun Yang <jiaxun.yang@flygoat.com> > Reported-by: Peter Maydell <Peter.maydel@linaro.org> > Signed-off-by: Isaku Yamahata <isaku.yamahata@intel.com> > --- > hw/isa/vt82c686.c | 13 ++++++++++++- > 1 file changed, 12 insertions(+), 1 deletion(-) > > diff --git a/hw/isa/vt82c686.c b/hw/isa/vt82c686.c > index 05d084f698..f0fb309f12 100644 > --- a/hw/isa/vt82c686.c > +++ b/hw/isa/vt82c686.c > @@ -144,7 +144,18 @@ static void pm_update_sci(ViaPMState *s) > ACPI_BITMASK_POWER_BUTTON_ENABLE | > ACPI_BITMASK_GLOBAL_LOCK_ENABLE | > ACPI_BITMASK_TIMER_ENABLE)) != 0); > - pci_set_irq(&s->dev, sci_level); > + if (pci_get_byte(s->dev.config + PCI_INTERRUPT_PIN)) { > + /* > + * FIXME: > + * Fix device model that realizes this PM device and remove > + * this work around. > + * The device model should wire SCI and setup > + * PCI_INTERRUPT_PIN properly. > + * If PIN# = 0(interrupt pin isn't used), don't raise SCI as > + * work around. > + */ > + pci_set_irq(&s->dev, sci_level); I'll defer this to Zoltan. Personally I wouldn't care about SCI_EN on the vt82c686, as it is not used by x86 machines (IOW, I'd not modify via_pm_reset and KISS). > + } > /* schedule a timer interruption if needed */ > acpi_pm_tmr_update(&s->ar, (s->ar.pm1.evt.en & ACPI_BITMASK_TIMER_ENABLE) && > !(pmsts & ACPI_BITMASK_TIMER_STATUS)); >
On Tue, Mar 23, 2021 at 10:24:31AM -0700, Isaku Yamahata wrote: > Commit 6be8cf56bc8b made sure that SCI is enabled in PM1.CNT > on reset in acpi_only mode by modifying acpi_pm1_cnt_reset() and > that worked for q35 as expected. > > The function was introduced by commit > eaba51c573a (acpi, acpi_piix, vt82c686: factor out PM1_CNT logic) > that forgot to actually call it at piix4 reset time and as result > SCI_EN wasn't set as was expected by 6be8cf56bc8b in acpi_only mode. > > So Windows crashes when it notices that SCI_EN is not set and FADT is > not providing information about how to enable it anymore. > Reproducer: > qemu-system-x86_64 -enable-kvm -M pc-i440fx-6.0,smm=off -cdrom any_windows_10x64.iso > > Fix it by calling acpi_pm1_cnt_reset() at piix4 reset time. > > Occasionally this patch adds reset acpi PM related registers on > piix4/vt582c686 reset time and de-assert sci. > piix4_pm_realize() initializes acpi pm tmr, evt, cnt and gpe. > via_pm_realize() initializes acpi pm tmr, evt and cnt. > reset them on device reset. pm_reset() in ich9.c correctly calls > corresponding reset functions. > > Fixes: 6be8cf56bc8b (acpi/core: always set SCI_EN when SMM isn't supported) > Reported-by: Reinoud Zandijk <reinoud@NetBSD.org> > Co-developed-by: Igor Mammedov <imammedo@redhat.com> > Signed-off-by: Igor Mammedov <imammedo@redhat.com> > Signed-off-by: Isaku Yamahata <isaku.yamahata@intel.com> > --- > CC: imammedo@redhat.com > CC: isaku.yamahata@intel.com > CC: mst@redhat.com > CC: reinoud@NetBSD.org > CC: isaku.yamahata@gmail.com > CC: berrange@redhat.com > CC: pbonzini@redhat.com > CC: f4bug@amsat.org > CC: aurelien@aurel32.net Can you split acpi and vt82c686 changes to separate patches please? This way we can merge them independently. Thanks! > --- > hw/acpi/piix4.c | 7 +++++++ > hw/isa/vt82c686.c | 5 +++++ > 2 files changed, 12 insertions(+) > > diff --git a/hw/acpi/piix4.c b/hw/acpi/piix4.c > index 6056d51667..8f8b0e95e5 100644 > --- a/hw/acpi/piix4.c > +++ b/hw/acpi/piix4.c > @@ -326,6 +326,13 @@ static void piix4_pm_reset(DeviceState *dev) > /* Mark SMM as already inited (until KVM supports SMM). */ > pci_conf[0x5B] = 0x02; > } > + > + acpi_pm1_evt_reset(&s->ar); > + acpi_pm1_cnt_reset(&s->ar); > + acpi_pm_tmr_reset(&s->ar); > + acpi_gpe_reset(&s->ar); > + acpi_update_sci(&s->ar, s->irq); > + > pm_io_space_update(s); > acpi_pcihp_reset(&s->acpi_pci_hotplug, !s->use_acpi_root_pci_hotplug); > } > diff --git a/hw/isa/vt82c686.c b/hw/isa/vt82c686.c > index f0fb309f12..98325bb32b 100644 > --- a/hw/isa/vt82c686.c > +++ b/hw/isa/vt82c686.c > @@ -178,6 +178,11 @@ static void via_pm_reset(DeviceState *d) > /* SMBus IO base */ > pci_set_long(s->dev.config + 0x90, 1); > > + acpi_pm1_evt_reset(&s->ar); > + acpi_pm1_cnt_reset(&s->ar); > + acpi_pm_tmr_reset(&s->ar); > + pm_update_sci(s); > + > pm_io_space_update(s); > smb_io_space_update(s); > } > -- > 2.25.1
[-- Attachment #1: Type: text/plain, Size: 4255 bytes --] On Tue, 23 Mar 2021, Philippe Mathieu-Daudé wrote: > Hi Isaku, > > On 3/23/21 6:24 PM, Isaku Yamahata wrote: >> Without this patch, the following patch will triger clan runtime >> sanitizer warnings as follows. This patch proactively works around it. >> I let v582c686.c maintainer address a correct fix as I'm not sure >> about fuloong2e device model. >> >>> MALLOC_PERTURB_=${MALLOC_PERTURB_:-$(( ${RANDOM:-0} % 255 + 1))} >>> QTEST_QEMU_IMG=./qemu-img >>> G_TEST_DBUS_DAEMON=/home/petmay01/linaro/qemu-for-merges/tests/dbus-vmstate-daemon.sh >>> QTEST_QEMU_BINARY=./qemu-system-mips64el tests/qtest/qom-test --tap -k >>> PASS 1 qtest-mips64el/qom-test /mips64el/qom/loongson3-virt >>> PASS 2 qtest-mips64el/qom-test /mips64el/qom/none >>> PASS 3 qtest-mips64el/qom-test /mips64el/qom/magnum >>> PASS 4 qtest-mips64el/qom-test /mips64el/qom/mipssim >>> PASS 5 qtest-mips64el/qom-test /mips64el/qom/malta >>> ../../hw/pci/pci.c:252:30: runtime error: shift exponent -1 is negative >>> PASS 6 qtest-mips64el/qom-test /mips64el/qom/fuloong2e >>> PASS 7 qtest-mips64el/qom-test /mips64el/qom/boston >>> PASS 8 qtest-mips64el/qom-test /mips64el/qom/pica61 >>> >>> and similarly for eg >>> >>> MALLOC_PERTURB_=${MALLOC_PERTURB_:-$(( ${RANDOM:-0} % 255 + 1))} >>> QTEST_QEMU_IMG=./qemu-img >>> G_TEST_DBUS_DAEMON=/home/petmay01/linaro/qemu-for-merges/tests/dbus-vmstate-daemon.sh >>> QTEST_QEMU_BINARY=./qemu-system-mips64el tests/qtest/endianness-test >>> --tap -k >>> ../../hw/pci/pci.c:252:30: runtime error: shift exponent -1 is negative >>> PASS 1 qtest-mips64el/endianness-test /mips64el/endianness/fuloong2e >>> ../../hw/pci/pci.c:252:30: runtime error: shift exponent -1 is negative >>> PASS 2 qtest-mips64el/endianness-test /mips64el/endianness/split/fuloong2e >>> ../../hw/pci/pci.c:252:30: runtime error: shift exponent -1 is negative >>> PASS 3 qtest-mips64el/endianness-test /mips64el/endianness/combine/fuloong2e >> >> Cc: Huacai Chen <chenhuacai@kernel.org> >> Cc: "Philippe Mathieu-Daudé" <f4bug@amsat.org> >> Cc: Jiaxun Yang <jiaxun.yang@flygoat.com> >> Reported-by: Peter Maydell <Peter.maydel@linaro.org> >> Signed-off-by: Isaku Yamahata <isaku.yamahata@intel.com> >> --- >> hw/isa/vt82c686.c | 13 ++++++++++++- >> 1 file changed, 12 insertions(+), 1 deletion(-) >> >> diff --git a/hw/isa/vt82c686.c b/hw/isa/vt82c686.c >> index 05d084f698..f0fb309f12 100644 >> --- a/hw/isa/vt82c686.c >> +++ b/hw/isa/vt82c686.c >> @@ -144,7 +144,18 @@ static void pm_update_sci(ViaPMState *s) >> ACPI_BITMASK_POWER_BUTTON_ENABLE | >> ACPI_BITMASK_GLOBAL_LOCK_ENABLE | >> ACPI_BITMASK_TIMER_ENABLE)) != 0); >> - pci_set_irq(&s->dev, sci_level); >> + if (pci_get_byte(s->dev.config + PCI_INTERRUPT_PIN)) { >> + /* >> + * FIXME: >> + * Fix device model that realizes this PM device and remove >> + * this work around. >> + * The device model should wire SCI and setup >> + * PCI_INTERRUPT_PIN properly. >> + * If PIN# = 0(interrupt pin isn't used), don't raise SCI as >> + * work around. >> + */ >> + pci_set_irq(&s->dev, sci_level); > > I'll defer this to Zoltan. I don't know anything about this, this was there well before I've touched this device model: https://git.qemu.org/?p=qemu.git;a=blame;f=hw/isa/vt82c686.c;hb=8063396bf3459a810d24e3efd6110b8480f0de5b > Personally I wouldn't care about SCI_EN on the vt82c686, as > it is not used by x86 machines (IOW, I'd not modify via_pm_reset > and KISS). I'm not sure but maybe then you could also just remove the PM parts from the device model as it probably does not work correctly anyway at the moment as it may not be correctly wired up to config registers. I'm not sure if it's needed by any guests but it was there for some reason and maybe better to fix it if possible than dropping it. As a workaround I'm OK with the proposed patch, I don't think it would break anything but haven't tested it. Regards, BALATON Zoltan >> + } >> /* schedule a timer interruption if needed */ >> acpi_pm_tmr_update(&s->ar, (s->ar.pm1.evt.en & ACPI_BITMASK_TIMER_ENABLE) && >> !(pmsts & ACPI_BITMASK_TIMER_STATUS)); >> > >
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