[PATCH v2 0/3] reinitialize ACPI PM device on reset

[PATCH v2 0/3] 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 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

[PATCH v2 1/3] 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 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

Re: [PATCH v2 1/3] vt82c686.c: don't raise SCI when PCI_INTERRUPT_PIN isn't setup

[Philippe Mathieu-Daudé]

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));
> 

Re: [PATCH v2 1/3] vt82c686.c: don't raise SCI when PCI_INTERRUPT_PIN isn't setup

[BALATON Zoltan]

[-- 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));
>>
>
>

[PATCH v2 2/3] 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.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

Re: [PATCH v2 2/3] pci: sprinkle assert in PCI pin number

[Peter Maydell]

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

Re: [PATCH v2 2/3] pci: sprinkle assert in PCI pin number

[Michael S. Tsirkin]

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

[PATCH v2 3/3] acpi:piix4, vt82c686: 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/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

Re: [PATCH v2 3/3] acpi:piix4, vt82c686: reinitialize acpi PM device on reset

[Michael S. Tsirkin]

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