[PATCH v3 0/4] Reinitialize ACPI PM device on reset

[PATCH v3 0/4] Reinitialize ACPI PM device on reset

[Isaku Yamahata]

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 v2:
- reorder patches
- split piix4 and v582c686 changes
- drop redundant assert in pcic

Changes from v1:
- bug work around hw/isa/vt82c686.c
- add assert in hw/pci/pci.c when setting/raising PCI interrupt

Isaku Yamahata (4):
  acpi/piix4: reinitialize acpi PM device on reset
  vt82c686.c: don't raise SCI when PCI_INTERRUPT_PIN isn't setup
  isa/v582c686: Reinitialize ACPI PM device on reset
  pci: sprinkle assert in PCI pin number

 hw/acpi/piix4.c   |  7 +++++++
 hw/isa/vt82c686.c | 18 +++++++++++++++++-
 hw/pci/pci.c      |  3 +++
 3 files changed, 27 insertions(+), 1 deletion(-)

-- 
2.25.1

[PATCH v3 1/4] acpi/piix4: reinitialize acpi PM device on reset

[Isaku Yamahata]

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 reset time and de-assert sci.
piix4_pm_realize() initializes acpi pm tmr, evt, cnt and gpe.
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 +++++++
 1 file changed, 7 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);
 }
-- 
2.25.1

[PATCH v3 2/4] vt82c686.c: don't raise SCI when PCI_INTERRUPT_PIN isn't setup

[Isaku Yamahata]

Without this patch, the following patch will triger clan runtime
sanitizer warnings as follows. This patch proactively works around it.
I leave a correct fix to v582c686.c maintainerfix 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: BALATON Zoltan <balaton@eik.bme.hu>
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.maydell@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

[PATCH v3 3/4] isa/v582c686: Reinitialize ACPI PM device on reset

[Isaku Yamahata]

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.

This patch adds reset ACPI PM related registers on vt82c686 reset time
and de-assert sci.
via_pm_realize() initializes acpi pm tmr, evt, cnt and gpe.
Reset them on device reset.

Cc: BALATON Zoltan <balaton@eik.bme.hu>
Cc: Huacai Chen <chenhuacai@kernel.org>
Cc: "Philippe Mathieu-Daudé" <f4bug@amsat.org>
Cc: Jiaxun Yang <jiaxun.yang@flygoat.com>
Signed-off-by: Isaku Yamahata <isaku.yamahata@intel.com>
---
 hw/isa/vt82c686.c | 5 +++++
 1 file changed, 5 insertions(+)

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

[PATCH v3 4/4] pci: sprinkle assert in PCI pin number

[Isaku Yamahata]

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.maydell@linaro.org>
Signed-off-by: Isaku Yamahata <isaku.yamahata@intel.com>
---
 hw/pci/pci.c | 3 +++
 1 file changed, 3 insertions(+)

diff --git a/hw/pci/pci.c b/hw/pci/pci.c
index ac9a24889c..8f35e13a0c 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;
@@ -1469,6 +1471,7 @@ static inline int pci_intx(PCIDevice *pci_dev)
 qemu_irq pci_allocate_irq(PCIDevice *pci_dev)
 {
     int intx = pci_intx(pci_dev);
+    assert(0 <= intx && intx < PCI_NUM_PINS);
 
     return qemu_allocate_irq(pci_irq_handler, pci_dev, intx);
 }
-- 
2.25.1