[PATCH 0/3] VIA PM Improvements

[PATCH 0/3] VIA PM Improvements

[Bernhard Beschow]

This series is part of my work to bring the VIA south bridges to the PC machine
[1]. First it resolves a fixme in the device model by using the dedicated ACPI
interrupt register for SCI routing. It then enables the device model to switch
to ACPI. Finally, ACPI shutdown is implemented which guests can take advantage
of after switching to ACPI mode.

Testing done:
- `make check`
- `qemu-system-ppc -M pegasos2 \
                   -rtc base=localtime \
                   -device ati-vga,guest_hwcursor=true,romfile="" \
                   -cdrom morphos-3.17.iso \
                   -kernel morphos-3.17/boot.img`
- `qemu-system-ppc -M pegasos2 \
                   -device ati-vga,romfile="" \
                   -cdrom morphos-3.17.iso \
                   -bios pegasos2.rom`
- `qemu-system-x86_64 -M pc -m 2G -accel kvm -cpu host -cdrom
   manjaro-kde-21.3.2-220704-linux515.iso` on my pc-via branch seems to work
   without any noticable differences to piix3 except that hotplugging isn't
   implemented.

[1] https://github.com/shentok/qemu/tree/pc-via

Bernhard Beschow (3):
  hw/isa/vt82c686: Fix SCI routing
  hw/isa/vt82c686: Allow PM controller to switch to ACPI mode
  hw/isa/vt82c686: Implement ACPI powerdown

 hw/isa/vt82c686.c | 75 +++++++++++++++++++++++++++++++++++++++--------
 1 file changed, 62 insertions(+), 13 deletions(-)

-- 
2.39.1

[PATCH 1/3] hw/isa/vt82c686: Fix SCI routing

[Bernhard Beschow]

According to the PCI specification, the hardware is not supposed to use
PCI_INTERRUPT_PIN for interrupt routing. Use the dedicated ACPI
Interrupt Select register for SCI routing instead.

Signed-off-by: Bernhard Beschow <shentey@gmail.com>
---
 hw/isa/vt82c686.c | 42 ++++++++++++++++++++++++++++++------------
 1 file changed, 30 insertions(+), 12 deletions(-)

diff --git a/hw/isa/vt82c686.c b/hw/isa/vt82c686.c
index 3f9bd0c04d..2189be6f20 100644
--- a/hw/isa/vt82c686.c
+++ b/hw/isa/vt82c686.c
@@ -46,6 +46,8 @@ struct ViaPMState {
     ACPIREGS ar;
     APMState apm;
     PMSMBus smb;
+
+    qemu_irq irq;
 };
 
 static void pm_io_space_update(ViaPMState *s)
@@ -148,18 +150,7 @@ static void pm_update_sci(ViaPMState *s)
                    ACPI_BITMASK_POWER_BUTTON_ENABLE |
                    ACPI_BITMASK_GLOBAL_LOCK_ENABLE |
                    ACPI_BITMASK_TIMER_ENABLE)) != 0);
-    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);
-    }
+    qemu_set_irq(s->irq, 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));
@@ -213,6 +204,13 @@ static void via_pm_realize(PCIDevice *dev, Error **errp)
     acpi_pm1_cnt_init(&s->ar, &s->io, false, false, 2, false);
 }
 
+static void via_pm_init(Object *obj)
+{
+    ViaPMState *s = VIA_PM(obj);
+
+    qdev_init_gpio_out(DEVICE(obj), &s->irq, 1);
+}
+
 typedef struct via_pm_init_info {
     uint16_t device_id;
 } ViaPMInitInfo;
@@ -238,6 +236,7 @@ static void via_pm_class_init(ObjectClass *klass, void *data)
 static const TypeInfo via_pm_info = {
     .name          = TYPE_VIA_PM,
     .parent        = TYPE_PCI_DEVICE,
+    .instance_init = via_pm_init,
     .instance_size = sizeof(ViaPMState),
     .abstract      = true,
     .interfaces = (InterfaceInfo[]) {
@@ -568,6 +567,21 @@ static const VMStateDescription vmstate_via = {
     }
 };
 
+static void via_isa_set_pm_irq(void *opaque, int n, int level)
+{
+    ViaISAState *s = opaque;
+    uint8_t irq = pci_get_byte(s->pm.dev.config + 0x42) & 0xf;
+
+    if (irq == 2) {
+        qemu_log_mask(LOG_GUEST_ERROR, "IRQ 2 for PM controller is reserved");
+        return;
+    }
+
+    if (irq != 0) {
+        qemu_set_irq(s->isa_irqs[irq], level);
+    }
+}
+
 static void via_isa_init(Object *obj)
 {
     ViaISAState *s = VIA_ISA(obj);
@@ -578,6 +592,8 @@ static void via_isa_init(Object *obj)
     object_initialize_child(obj, "uhci2", &s->uhci[1], TYPE_VT82C686B_USB_UHCI);
     object_initialize_child(obj, "ac97", &s->ac97, TYPE_VIA_AC97);
     object_initialize_child(obj, "mc97", &s->mc97, TYPE_VIA_MC97);
+
+    qdev_init_gpio_in_named(DEVICE(obj), via_isa_set_pm_irq, "sci", 1);
 }
 
 static const TypeInfo via_isa_info = {
@@ -664,6 +680,8 @@ static void via_isa_realize(PCIDevice *d, Error **errp)
     if (!qdev_realize(DEVICE(&s->pm), BUS(pci_bus), errp)) {
         return;
     }
+    qdev_connect_gpio_out(DEVICE(&s->pm), 0,
+                          qdev_get_gpio_in_named(DEVICE(d), "sci", 0));
 
     /* Function 5: AC97 Audio */
     qdev_prop_set_int32(DEVICE(&s->ac97), "addr", d->devfn + 5);
-- 
2.39.1

[PATCH 2/3] hw/isa/vt82c686: Allow PM controller to switch to ACPI mode

[Bernhard Beschow]

Adds missing functionality the real hardware supports.

Signed-off-by: Bernhard Beschow <shentey@gmail.com>
---
 hw/isa/vt82c686.c | 18 +++++++++++++++++-
 1 file changed, 17 insertions(+), 1 deletion(-)

diff --git a/hw/isa/vt82c686.c b/hw/isa/vt82c686.c
index 2189be6f20..b0765d4ed8 100644
--- a/hw/isa/vt82c686.c
+++ b/hw/isa/vt82c686.c
@@ -37,6 +37,9 @@
 #include "qemu/timer.h"
 #include "trace.h"
 
+#define ACPI_ENABLE 0xf1
+#define ACPI_DISABLE 0xf0
+
 #define TYPE_VIA_PM "via-pm"
 OBJECT_DECLARE_SIMPLE_TYPE(ViaPMState, VIA_PM)
 
@@ -50,6 +53,19 @@ struct ViaPMState {
     qemu_irq irq;
 };
 
+static void via_pm_apm_ctrl_changed(uint32_t val, void *arg)
+{
+    ViaPMState *s = arg;
+
+    /* ACPI specs 3.0, 4.7.2.5 */
+    acpi_pm1_cnt_update(&s->ar, val == ACPI_ENABLE, val == ACPI_DISABLE);
+    if (val == ACPI_ENABLE || val == ACPI_DISABLE) {
+        return;
+    }
+
+    qemu_log_mask(LOG_UNIMP, "%s: unimplemented value 0x%x", __func__, val);
+}
+
 static void pm_io_space_update(ViaPMState *s)
 {
     uint32_t pmbase = pci_get_long(s->dev.config + 0x48) & 0xff80UL;
@@ -193,7 +209,7 @@ static void via_pm_realize(PCIDevice *dev, Error **errp)
     memory_region_add_subregion(pci_address_space_io(dev), 0, &s->smb.io);
     memory_region_set_enabled(&s->smb.io, false);
 
-    apm_init(dev, &s->apm, NULL, s);
+    apm_init(dev, &s->apm, via_pm_apm_ctrl_changed, s);
 
     memory_region_init_io(&s->io, OBJECT(dev), &pm_io_ops, s, "via-pm", 128);
     memory_region_add_subregion(pci_address_space_io(dev), 0, &s->io);
-- 
2.39.1

[PATCH 3/3] hw/isa/vt82c686: Implement ACPI powerdown

[Bernhard Beschow]

Signed-off-by: Bernhard Beschow <shentey@gmail.com>
---
 hw/isa/vt82c686.c | 15 +++++++++++++++
 1 file changed, 15 insertions(+)

diff --git a/hw/isa/vt82c686.c b/hw/isa/vt82c686.c
index b0765d4ed8..2db54d1649 100644
--- a/hw/isa/vt82c686.c
+++ b/hw/isa/vt82c686.c
@@ -33,8 +33,10 @@
 #include "qapi/error.h"
 #include "qemu/log.h"
 #include "qemu/module.h"
+#include "qemu/notify.h"
 #include "qemu/range.h"
 #include "qemu/timer.h"
+#include "sysemu/runstate.h"
 #include "trace.h"
 
 #define ACPI_ENABLE 0xf1
@@ -50,6 +52,8 @@ struct ViaPMState {
     APMState apm;
     PMSMBus smb;
 
+    Notifier powerdown_notifier;
+
     qemu_irq irq;
 };
 
@@ -198,6 +202,14 @@ static void via_pm_reset(DeviceState *d)
     smb_io_space_update(s);
 }
 
+static void via_pm_powerdown_req(Notifier *n, void *opaque)
+{
+    ViaPMState *s = container_of(n, ViaPMState, powerdown_notifier);
+
+    assert(s != NULL);
+    acpi_pm1_evt_power_down(&s->ar);
+}
+
 static void via_pm_realize(PCIDevice *dev, Error **errp)
 {
     ViaPMState *s = VIA_PM(dev);
@@ -218,6 +230,9 @@ static void via_pm_realize(PCIDevice *dev, Error **errp)
     acpi_pm_tmr_init(&s->ar, pm_tmr_timer, &s->io);
     acpi_pm1_evt_init(&s->ar, pm_tmr_timer, &s->io);
     acpi_pm1_cnt_init(&s->ar, &s->io, false, false, 2, false);
+
+    s->powerdown_notifier.notify = via_pm_powerdown_req;
+    qemu_register_powerdown_notifier(&s->powerdown_notifier);
 }
 
 static void via_pm_init(Object *obj)
-- 
2.39.1

Re: [PATCH 0/3] VIA PM Improvements

[BALATON Zoltan]

On Sun, 29 Jan 2023, Bernhard Beschow wrote:
> This series is part of my work to bring the VIA south bridges to the PC machine
> [1]. First it resolves a fixme in the device model by using the dedicated ACPI
> interrupt register for SCI routing. It then enables the device model to switch
> to ACPI. Finally, ACPI shutdown is implemented which guests can take advantage
> of after switching to ACPI mode.
>
> Testing done:
> - `make check`
> - `qemu-system-ppc -M pegasos2 \
>                   -rtc base=localtime \
>                   -device ati-vga,guest_hwcursor=true,romfile="" \
>                   -cdrom morphos-3.17.iso \
>                   -kernel morphos-3.17/boot.img`
> - `qemu-system-ppc -M pegasos2 \
>                   -device ati-vga,romfile="" \
>                   -cdrom morphos-3.17.iso \
>                   -bios pegasos2.rom`

I can't review this in detail so I can only give some cosmetic comments, 
do with it what you like. For testing I think shutdown from MorphOS worked 
with the pegasos2.rom after this patch:

https://lists.nongnu.org/archive/html/qemu-devel/2021-11/msg01871.html

but that was not accepted as it was deemed to be an error in memory layer 
but then that wasn't fixed so probably this workaround is still needed to 
get it to work with big endian guest on little endian host. Does poweroff 
work from Linux?

Regards,
BALATON Zoltan

> - `qemu-system-x86_64 -M pc -m 2G -accel kvm -cpu host -cdrom
>   manjaro-kde-21.3.2-220704-linux515.iso` on my pc-via branch seems to work
>   without any noticable differences to piix3 except that hotplugging isn't
>   implemented.
>
> [1] https://github.com/shentok/qemu/tree/pc-via
>
> Bernhard Beschow (3):
>  hw/isa/vt82c686: Fix SCI routing
>  hw/isa/vt82c686: Allow PM controller to switch to ACPI mode
>  hw/isa/vt82c686: Implement ACPI powerdown
>
> hw/isa/vt82c686.c | 75 +++++++++++++++++++++++++++++++++++++++--------
> 1 file changed, 62 insertions(+), 13 deletions(-)
>
> --
> 2.39.1
>
>
>

Re: [PATCH 1/3] hw/isa/vt82c686: Fix SCI routing

[BALATON Zoltan]

On Sun, 29 Jan 2023, Bernhard Beschow wrote:
> According to the PCI specification, the hardware is not supposed to use
> PCI_INTERRUPT_PIN for interrupt routing. Use the dedicated ACPI
> Interrupt Select register for SCI routing instead.
>
> Signed-off-by: Bernhard Beschow <shentey@gmail.com>
> ---
> hw/isa/vt82c686.c | 42 ++++++++++++++++++++++++++++++------------
> 1 file changed, 30 insertions(+), 12 deletions(-)
>
> diff --git a/hw/isa/vt82c686.c b/hw/isa/vt82c686.c
> index 3f9bd0c04d..2189be6f20 100644
> --- a/hw/isa/vt82c686.c
> +++ b/hw/isa/vt82c686.c
> @@ -46,6 +46,8 @@ struct ViaPMState {
>     ACPIREGS ar;
>     APMState apm;
>     PMSMBus smb;
> +
> +    qemu_irq irq;
> };
>
> static void pm_io_space_update(ViaPMState *s)
> @@ -148,18 +150,7 @@ static void pm_update_sci(ViaPMState *s)
>                    ACPI_BITMASK_POWER_BUTTON_ENABLE |
>                    ACPI_BITMASK_GLOBAL_LOCK_ENABLE |
>                    ACPI_BITMASK_TIMER_ENABLE)) != 0);
> -    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);
> -    }
> +    qemu_set_irq(s->irq, 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));
> @@ -213,6 +204,13 @@ static void via_pm_realize(PCIDevice *dev, Error **errp)
>     acpi_pm1_cnt_init(&s->ar, &s->io, false, false, 2, false);
> }
>
> +static void via_pm_init(Object *obj)
> +{
> +    ViaPMState *s = VIA_PM(obj);
> +
> +    qdev_init_gpio_out(DEVICE(obj), &s->irq, 1);
> +}
> +
> typedef struct via_pm_init_info {
>     uint16_t device_id;
> } ViaPMInitInfo;
> @@ -238,6 +236,7 @@ static void via_pm_class_init(ObjectClass *klass, void *data)
> static const TypeInfo via_pm_info = {
>     .name          = TYPE_VIA_PM,
>     .parent        = TYPE_PCI_DEVICE,
> +    .instance_init = via_pm_init,
>     .instance_size = sizeof(ViaPMState),
>     .abstract      = true,
>     .interfaces = (InterfaceInfo[]) {
> @@ -568,6 +567,21 @@ static const VMStateDescription vmstate_via = {
>     }
> };
>
> +static void via_isa_set_pm_irq(void *opaque, int n, int level)
> +{
> +    ViaISAState *s = opaque;
> +    uint8_t irq = pci_get_byte(s->pm.dev.config + 0x42) & 0xf;
> +
> +    if (irq == 2) {

unlikely(irq == 2) but do we really need to log this? It really should not 
happen but if it does the guest is really broken and this will just flood 
the log so I think you can just test for it in the if below and drop the log.

Regards,
BALATON Zoltan

> +        qemu_log_mask(LOG_GUEST_ERROR, "IRQ 2 for PM controller is reserved");
> +        return;
> +    }
> +
> +    if (irq != 0) {
> +        qemu_set_irq(s->isa_irqs[irq], level);
> +    }
> +}
> +
> static void via_isa_init(Object *obj)
> {
>     ViaISAState *s = VIA_ISA(obj);
> @@ -578,6 +592,8 @@ static void via_isa_init(Object *obj)
>     object_initialize_child(obj, "uhci2", &s->uhci[1], TYPE_VT82C686B_USB_UHCI);
>     object_initialize_child(obj, "ac97", &s->ac97, TYPE_VIA_AC97);
>     object_initialize_child(obj, "mc97", &s->mc97, TYPE_VIA_MC97);
> +
> +    qdev_init_gpio_in_named(DEVICE(obj), via_isa_set_pm_irq, "sci", 1);
> }
>
> static const TypeInfo via_isa_info = {
> @@ -664,6 +680,8 @@ static void via_isa_realize(PCIDevice *d, Error **errp)
>     if (!qdev_realize(DEVICE(&s->pm), BUS(pci_bus), errp)) {
>         return;
>     }
> +    qdev_connect_gpio_out(DEVICE(&s->pm), 0,
> +                          qdev_get_gpio_in_named(DEVICE(d), "sci", 0));
>
>     /* Function 5: AC97 Audio */
>     qdev_prop_set_int32(DEVICE(&s->ac97), "addr", d->devfn + 5);
>

Re: [PATCH 2/3] hw/isa/vt82c686: Allow PM controller to switch to ACPI mode

[BALATON Zoltan]

On Sun, 29 Jan 2023, Bernhard Beschow wrote:
> Adds missing functionality the real hardware supports.
>
> Signed-off-by: Bernhard Beschow <shentey@gmail.com>
> ---
> hw/isa/vt82c686.c | 18 +++++++++++++++++-
> 1 file changed, 17 insertions(+), 1 deletion(-)
>
> diff --git a/hw/isa/vt82c686.c b/hw/isa/vt82c686.c
> index 2189be6f20..b0765d4ed8 100644
> --- a/hw/isa/vt82c686.c
> +++ b/hw/isa/vt82c686.c
> @@ -37,6 +37,9 @@
> #include "qemu/timer.h"
> #include "trace.h"
>
> +#define ACPI_ENABLE 0xf1
> +#define ACPI_DISABLE 0xf0

Are these some standard in which case they should be in a shared acpi 
header or chip specific, then we could just put it in a comment, see 
below.

> +
> #define TYPE_VIA_PM "via-pm"
> OBJECT_DECLARE_SIMPLE_TYPE(ViaPMState, VIA_PM)
>
> @@ -50,6 +53,19 @@ struct ViaPMState {
>     qemu_irq irq;
> };
>
> +static void via_pm_apm_ctrl_changed(uint32_t val, void *arg)
> +{
> +    ViaPMState *s = arg;
> +
> +    /* ACPI specs 3.0, 4.7.2.5 */
> +    acpi_pm1_cnt_update(&s->ar, val == ACPI_ENABLE, val == ACPI_DISABLE);
> +    if (val == ACPI_ENABLE || val == ACPI_DISABLE) {
> +        return;
> +    }
> +
> +    qemu_log_mask(LOG_UNIMP, "%s: unimplemented value 0x%x", __func__, val);

Maybe a switch is more readable here:

switch(val) {
case 0xf1: /* Enable */
case 0xf0: /* Disable */
     acpi_pm1_cnt_update(&s->ar, val & 1, val & 1);
     break;
default:
     qemu_log_mask(LOG_UNIMP, "%s: unimplemented value 0x%x", __func__, val);
}

Or maybe not, it can be personal preference. (Why does 
acpi_pm1_cnt_update() take two arguments for a bool value? Can these be 
both true or false at the same time?)

Regards,
BALATON Zoltan

> +}
> +
> static void pm_io_space_update(ViaPMState *s)
> {
>     uint32_t pmbase = pci_get_long(s->dev.config + 0x48) & 0xff80UL;
> @@ -193,7 +209,7 @@ static void via_pm_realize(PCIDevice *dev, Error **errp)
>     memory_region_add_subregion(pci_address_space_io(dev), 0, &s->smb.io);
>     memory_region_set_enabled(&s->smb.io, false);
>
> -    apm_init(dev, &s->apm, NULL, s);
> +    apm_init(dev, &s->apm, via_pm_apm_ctrl_changed, s);
>
>     memory_region_init_io(&s->io, OBJECT(dev), &pm_io_ops, s, "via-pm", 128);
>     memory_region_add_subregion(pci_address_space_io(dev), 0, &s->io);
>

Re: [PATCH 3/3] hw/isa/vt82c686: Implement ACPI powerdown

[BALATON Zoltan]

On Sun, 29 Jan 2023, Bernhard Beschow wrote:
> Signed-off-by: Bernhard Beschow <shentey@gmail.com>
> ---
> hw/isa/vt82c686.c | 15 +++++++++++++++
> 1 file changed, 15 insertions(+)
>
> diff --git a/hw/isa/vt82c686.c b/hw/isa/vt82c686.c
> index b0765d4ed8..2db54d1649 100644
> --- a/hw/isa/vt82c686.c
> +++ b/hw/isa/vt82c686.c
> @@ -33,8 +33,10 @@
> #include "qapi/error.h"
> #include "qemu/log.h"
> #include "qemu/module.h"
> +#include "qemu/notify.h"
> #include "qemu/range.h"
> #include "qemu/timer.h"
> +#include "sysemu/runstate.h"
> #include "trace.h"
>
> #define ACPI_ENABLE 0xf1
> @@ -50,6 +52,8 @@ struct ViaPMState {
>     APMState apm;
>     PMSMBus smb;
>
> +    Notifier powerdown_notifier;
> +
>     qemu_irq irq;

Is there a reason to leave blank lines here? Do they separate any logical 
blocks? If not please just drop them to allow mire lines to fit in one 
screen.

> };
>
> @@ -198,6 +202,14 @@ static void via_pm_reset(DeviceState *d)
>     smb_io_space_update(s);
> }
>
> +static void via_pm_powerdown_req(Notifier *n, void *opaque)
> +{
> +    ViaPMState *s = container_of(n, ViaPMState, powerdown_notifier);
> +
> +    assert(s != NULL);

Only piix4 seems to assert in this callback, all others assume this will 
work and indeed you register it from the realize method of the same object 
with its already type checked state struct so this should not need to 
check again so I think asserting here is overcautiousness.

As said in the cover all these are just small things I came across, sorry 
I can't give a better review of this.

Regards,
BALATON Zoltan

> +    acpi_pm1_evt_power_down(&s->ar);
> +}
> +
> static void via_pm_realize(PCIDevice *dev, Error **errp)
> {
>     ViaPMState *s = VIA_PM(dev);
> @@ -218,6 +230,9 @@ static void via_pm_realize(PCIDevice *dev, Error **errp)
>     acpi_pm_tmr_init(&s->ar, pm_tmr_timer, &s->io);
>     acpi_pm1_evt_init(&s->ar, pm_tmr_timer, &s->io);
>     acpi_pm1_cnt_init(&s->ar, &s->io, false, false, 2, false);
> +
> +    s->powerdown_notifier.notify = via_pm_powerdown_req;
> +    qemu_register_powerdown_notifier(&s->powerdown_notifier);
> }
>
> static void via_pm_init(Object *obj)
>

Re: [PATCH 2/3] hw/isa/vt82c686: Allow PM controller to switch to ACPI mode

[Philippe Mathieu-Daudé]

On 31/1/23 15:54, BALATON Zoltan wrote:
> On Sun, 29 Jan 2023, Bernhard Beschow wrote:
>> Adds missing functionality the real hardware supports.
>>
>> Signed-off-by: Bernhard Beschow <shentey@gmail.com>
>> ---
>> hw/isa/vt82c686.c | 18 +++++++++++++++++-
>> 1 file changed, 17 insertions(+), 1 deletion(-)
>>
>> diff --git a/hw/isa/vt82c686.c b/hw/isa/vt82c686.c
>> index 2189be6f20..b0765d4ed8 100644
>> --- a/hw/isa/vt82c686.c
>> +++ b/hw/isa/vt82c686.c
>> @@ -37,6 +37,9 @@
>> #include "qemu/timer.h"
>> #include "trace.h"

> Why does 
> acpi_pm1_cnt_update() take two arguments for a bool value? Can these be 
> both true or false at the same time?

No, this is a one-bit so boolean is enough...

Maybe unfinished refactor from commit eaba51c573 ("acpi, acpi_piix,
vt82c686: factor out PM1_CNT logic")?

Re: [PATCH 2/3] hw/isa/vt82c686: Allow PM controller to switch to ACPI mode

[Philippe Mathieu-Daudé]

Forgot to Cc ACPI maintainers.

On 6/2/23 09:00, Philippe Mathieu-Daudé wrote:
> On 31/1/23 15:54, BALATON Zoltan wrote:
>> On Sun, 29 Jan 2023, Bernhard Beschow wrote:
>>> Adds missing functionality the real hardware supports.
>>>
>>> Signed-off-by: Bernhard Beschow <shentey@gmail.com>
>>> ---
>>> hw/isa/vt82c686.c | 18 +++++++++++++++++-
>>> 1 file changed, 17 insertions(+), 1 deletion(-)
>>>
>>> diff --git a/hw/isa/vt82c686.c b/hw/isa/vt82c686.c
>>> index 2189be6f20..b0765d4ed8 100644
>>> --- a/hw/isa/vt82c686.c
>>> +++ b/hw/isa/vt82c686.c
>>> @@ -37,6 +37,9 @@
>>> #include "qemu/timer.h"
>>> #include "trace.h"
> 
>> Why does acpi_pm1_cnt_update() take two arguments for a bool value? 
>> Can these be both true or false at the same time?
> 
> No, this is a one-bit so boolean is enough...
> 
> Maybe unfinished refactor from commit eaba51c573 ("acpi, acpi_piix,
> vt82c686: factor out PM1_CNT logic")?

Re: [PATCH 1/3] hw/isa/vt82c686: Fix SCI routing

[Bernhard Beschow]

Am 31. Januar 2023 14:42:29 UTC schrieb BALATON Zoltan <balaton@eik.bme.hu>:
>On Sun, 29 Jan 2023, Bernhard Beschow wrote:
>> According to the PCI specification, the hardware is not supposed to use
>> PCI_INTERRUPT_PIN for interrupt routing. Use the dedicated ACPI
>> Interrupt Select register for SCI routing instead.
>> 
>> Signed-off-by: Bernhard Beschow <shentey@gmail.com>
>> ---
>> hw/isa/vt82c686.c | 42 ++++++++++++++++++++++++++++++------------
>> 1 file changed, 30 insertions(+), 12 deletions(-)
>> 
>> diff --git a/hw/isa/vt82c686.c b/hw/isa/vt82c686.c
>> index 3f9bd0c04d..2189be6f20 100644
>> --- a/hw/isa/vt82c686.c
>> +++ b/hw/isa/vt82c686.c
>> @@ -46,6 +46,8 @@ struct ViaPMState {
>>     ACPIREGS ar;
>>     APMState apm;
>>     PMSMBus smb;
>> +
>> +    qemu_irq irq;
>> };
>> 
>> static void pm_io_space_update(ViaPMState *s)
>> @@ -148,18 +150,7 @@ static void pm_update_sci(ViaPMState *s)
>>                    ACPI_BITMASK_POWER_BUTTON_ENABLE |
>>                    ACPI_BITMASK_GLOBAL_LOCK_ENABLE |
>>                    ACPI_BITMASK_TIMER_ENABLE)) != 0);
>> -    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);
>> -    }
>> +    qemu_set_irq(s->irq, 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));
>> @@ -213,6 +204,13 @@ static void via_pm_realize(PCIDevice *dev, Error **errp)
>>     acpi_pm1_cnt_init(&s->ar, &s->io, false, false, 2, false);
>> }
>> 
>> +static void via_pm_init(Object *obj)
>> +{
>> +    ViaPMState *s = VIA_PM(obj);
>> +
>> +    qdev_init_gpio_out(DEVICE(obj), &s->irq, 1);
>> +}
>> +
>> typedef struct via_pm_init_info {
>>     uint16_t device_id;
>> } ViaPMInitInfo;
>> @@ -238,6 +236,7 @@ static void via_pm_class_init(ObjectClass *klass, void *data)
>> static const TypeInfo via_pm_info = {
>>     .name          = TYPE_VIA_PM,
>>     .parent        = TYPE_PCI_DEVICE,
>> +    .instance_init = via_pm_init,
>>     .instance_size = sizeof(ViaPMState),
>>     .abstract      = true,
>>     .interfaces = (InterfaceInfo[]) {
>> @@ -568,6 +567,21 @@ static const VMStateDescription vmstate_via = {
>>     }
>> };
>> 
>> +static void via_isa_set_pm_irq(void *opaque, int n, int level)
>> +{
>> +    ViaISAState *s = opaque;
>> +    uint8_t irq = pci_get_byte(s->pm.dev.config + 0x42) & 0xf;
>> +
>> +    if (irq == 2) {
>
>unlikely(irq == 2) but do we really need to log this? It really should not happen but if it does the guest is really broken and this will just flood the log so I think you can just test for it in the if below and drop the log.

I'd prefer to log such guest errors precisely for above reason: To detect really broken guests. If it's really too much noise one can still filter the log with external tools such as grep.

Best regards,
Bernhard

>
>Regards,
>BALATON Zoltan
>
>> +        qemu_log_mask(LOG_GUEST_ERROR, "IRQ 2 for PM controller is reserved");
>> +        return;
>> +    }
>> +
>> +    if (irq != 0) {
>> +        qemu_set_irq(s->isa_irqs[irq], level);
>> +    }
>> +}
>> +
>> static void via_isa_init(Object *obj)
>> {
>>     ViaISAState *s = VIA_ISA(obj);
>> @@ -578,6 +592,8 @@ static void via_isa_init(Object *obj)
>>     object_initialize_child(obj, "uhci2", &s->uhci[1], TYPE_VT82C686B_USB_UHCI);
>>     object_initialize_child(obj, "ac97", &s->ac97, TYPE_VIA_AC97);
>>     object_initialize_child(obj, "mc97", &s->mc97, TYPE_VIA_MC97);
>> +
>> +    qdev_init_gpio_in_named(DEVICE(obj), via_isa_set_pm_irq, "sci", 1);
>> }
>> 
>> static const TypeInfo via_isa_info = {
>> @@ -664,6 +680,8 @@ static void via_isa_realize(PCIDevice *d, Error **errp)
>>     if (!qdev_realize(DEVICE(&s->pm), BUS(pci_bus), errp)) {
>>         return;
>>     }
>> +    qdev_connect_gpio_out(DEVICE(&s->pm), 0,
>> +                          qdev_get_gpio_in_named(DEVICE(d), "sci", 0));
>> 
>>     /* Function 5: AC97 Audio */
>>     qdev_prop_set_int32(DEVICE(&s->ac97), "addr", d->devfn + 5);
>> 

Re: [PATCH 3/3] hw/isa/vt82c686: Implement ACPI powerdown

[Bernhard Beschow]

Am 31. Januar 2023 14:58:01 UTC schrieb BALATON Zoltan <balaton@eik.bme.hu>:
>On Sun, 29 Jan 2023, Bernhard Beschow wrote:
>> Signed-off-by: Bernhard Beschow <shentey@gmail.com>
>> ---
>> hw/isa/vt82c686.c | 15 +++++++++++++++
>> 1 file changed, 15 insertions(+)
>> 
>> diff --git a/hw/isa/vt82c686.c b/hw/isa/vt82c686.c
>> index b0765d4ed8..2db54d1649 100644
>> --- a/hw/isa/vt82c686.c
>> +++ b/hw/isa/vt82c686.c
>> @@ -33,8 +33,10 @@
>> #include "qapi/error.h"
>> #include "qemu/log.h"
>> #include "qemu/module.h"
>> +#include "qemu/notify.h"
>> #include "qemu/range.h"
>> #include "qemu/timer.h"
>> +#include "sysemu/runstate.h"
>> #include "trace.h"
>> 
>> #define ACPI_ENABLE 0xf1
>> @@ -50,6 +52,8 @@ struct ViaPMState {
>>     APMState apm;
>>     PMSMBus smb;
>> 
>> +    Notifier powerdown_notifier;
>> +
>>     qemu_irq irq;
>
>Is there a reason to leave blank lines here? Do they separate any logical blocks? If not please just drop them to allow mire lines to fit in one screen.

I could stick closer to piix4 here indeed.

>> };
>> 
>> @@ -198,6 +202,14 @@ static void via_pm_reset(DeviceState *d)
>>     smb_io_space_update(s);
>> }
>> 
>> +static void via_pm_powerdown_req(Notifier *n, void *opaque)
>> +{
>> +    ViaPMState *s = container_of(n, ViaPMState, powerdown_notifier);
>> +
>> +    assert(s != NULL);
>
>Only piix4 seems to assert in this callback, all others assume this will work and indeed you register it from the realize method of the same object with its already type checked state struct so this should not need to check again so I think asserting here is overcautiousness.

Yes, I took a lot of inspitation from piix4. Piix4 also sets up the notifier in its realize method, so the situation here and there should be the same. I'd therefore tend to keep the code consistent. Also, an assert does communicate intention.

>
>As said in the cover all these are just small things I came across, sorry I can't give a better review of this.

Thanks still!

Best regards,
Bernhard

P.S.: Any news from the via-ac97 side? Mind rebasing onto 7.2, even if it's still work in progress?

>
>Regards,
>BALATON Zoltan
>
>> +    acpi_pm1_evt_power_down(&s->ar);
>> +}
>> +
>> static void via_pm_realize(PCIDevice *dev, Error **errp)
>> {
>>     ViaPMState *s = VIA_PM(dev);
>> @@ -218,6 +230,9 @@ static void via_pm_realize(PCIDevice *dev, Error **errp)
>>     acpi_pm_tmr_init(&s->ar, pm_tmr_timer, &s->io);
>>     acpi_pm1_evt_init(&s->ar, pm_tmr_timer, &s->io);
>>     acpi_pm1_cnt_init(&s->ar, &s->io, false, false, 2, false);
>> +
>> +    s->powerdown_notifier.notify = via_pm_powerdown_req;
>> +    qemu_register_powerdown_notifier(&s->powerdown_notifier);
>> }
>> 
>> static void via_pm_init(Object *obj)
>> 

Re: [PATCH 2/3] hw/isa/vt82c686: Allow PM controller to switch to ACPI mode

[Igor Mammedov]

On Mon, 6 Feb 2023 09:00:37 +0100
Philippe Mathieu-Daudé <philmd@linaro.org> wrote:

> On 31/1/23 15:54, BALATON Zoltan wrote:
> > On Sun, 29 Jan 2023, Bernhard Beschow wrote:  
> >> Adds missing functionality the real hardware supports.
> >>
> >> Signed-off-by: Bernhard Beschow <shentey@gmail.com>
> >> ---
> >> hw/isa/vt82c686.c | 18 +++++++++++++++++-
> >> 1 file changed, 17 insertions(+), 1 deletion(-)
> >>
> >> diff --git a/hw/isa/vt82c686.c b/hw/isa/vt82c686.c
> >> index 2189be6f20..b0765d4ed8 100644
> >> --- a/hw/isa/vt82c686.c
> >> +++ b/hw/isa/vt82c686.c
> >> @@ -37,6 +37,9 @@
> >> #include "qemu/timer.h"
> >> #include "trace.h"  
> 
> > Why does 
> > acpi_pm1_cnt_update() take two arguments for a bool value? Can these be 
> > both true or false at the same time?  
> 
> No, this is a one-bit so boolean is enough...

one boolean would be fine unless they were both false?

> 
> Maybe unfinished refactor from commit eaba51c573 ("acpi, acpi_piix,
> vt82c686: factor out PM1_CNT logic")?
>