[PATCH v5 0/5] VIA PM: Implement basic ACPI support

[PATCH v5 0/5] VIA PM: Implement basic ACPI support

[Bernhard Beschow]

This series is part of my work to bring the VIA south bridges to the PC machine
[1]. It implements missing ACPI functionality which ACPI-aware x86 guests
expect for a smooth experience. The implementation is heavily inspired by PIIX4.

Further quirks are needed in order to use the VIA south bridges in the PC
machine. These were deliberately left out for a future series. The idea for now
is to get the device model in shape for adding support for it in SeaBIOS.

The series is structured as follows: The first patch fixes ACPI events to be
signalled by SCI interrupts. The next three patches implement typical ACPI
event handling. The last patch adds software-based SMI triggering which is the
mechanism used in ACPI to transition the system into ACPI mode.

Testing done:
* `make check`
* `make check-avocado`
* `qemu-system-ppc -M pegasos2 \
                   -device ati-vga,romfile="" \
                   -cdrom morphos-3.18.iso \
                   -bios pegasos2.rom`

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

v5:
* Implement software-based SMI triggering and handling of ACPI events based on
  v3

v4:
* Alternative proposal (Zoltan)

v3: https://patchew.org/QEMU/20231005115159.81202-1-shentey@gmail.com/
* Rename SCI irq attribute to sci_irq (Zoltan)
* Fix confusion about location of ACPI interrupt select register (Zoltan)
* Model SCI as named GPIO (Bernhard)
* Perform upcast via macro rather than sub structure selection (Bernhard)

v2:
* Introduce named constants for the ACPI interrupt select register at offset
  0x42 (Phil)

Bernhard Beschow (5):
  hw/isa/vt82c686: Respect SCI interrupt assignment
  hw/isa/vt82c686: Add missing initialization of ACPI general purpose
    event registers
  hw/isa/vt82c686: Reuse acpi_update_sci()
  hw/isa/vt82c686: Implement ACPI powerdown
  hw/isa/vt82c686: Implement software-based SMI triggering

 hw/isa/vt82c686.c | 179 ++++++++++++++++++++++++++++++++++++----------
 1 file changed, 142 insertions(+), 37 deletions(-)

-- 
2.42.0

[PATCH v5 1/5] hw/isa/vt82c686: Respect SCI interrupt assignment

[Bernhard Beschow]

According to the datasheet, SCI interrupts of the power management function
aren't routed through the PCI pins but rather directly to the integrated PIC.
The routing is configurable through the ACPI interrupt select register at offset
0x42 in the PCI configuration space of the power management function.

Note that pm_update_sci() is now redundant to acpi_update_sci() and shall be
removed soon.

Signed-off-by: Bernhard Beschow <shentey@gmail.com>
Reviewed-by: Philippe Mathieu-Daudé <philmd@linaro.org>
---
 hw/isa/vt82c686.c | 48 +++++++++++++++++++++++++++++++++++------------
 1 file changed, 36 insertions(+), 12 deletions(-)

diff --git a/hw/isa/vt82c686.c b/hw/isa/vt82c686.c
index 57bdfb4e78..aeb9434a46 100644
--- a/hw/isa/vt82c686.c
+++ b/hw/isa/vt82c686.c
@@ -40,12 +40,17 @@
 #define TYPE_VIA_PM "via-pm"
 OBJECT_DECLARE_SIMPLE_TYPE(ViaPMState, VIA_PM)
 
+#define VIA_PM_SCI_SELECT_OFS 0x42
+#define VIA_PM_SCI_SELECT_MASK 0xf
+
 struct ViaPMState {
     PCIDevice dev;
     MemoryRegion io;
     ACPIREGS ar;
     APMState apm;
     PMSMBus smb;
+
+    qemu_irq sci_irq;
 };
 
 static void pm_io_space_update(ViaPMState *s)
@@ -148,18 +153,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->sci_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 +207,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_named(DEVICE(obj), &s->sci_irq, "sci", 1);
+}
+
 typedef struct via_pm_init_info {
     uint16_t device_id;
 } ViaPMInitInfo;
@@ -238,6 +239,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,9 +570,27 @@ static const VMStateDescription vmstate_via = {
     }
 };
 
+static void via_isa_set_pm_irq(void *opaque, int n, int level)
+{
+    ViaISAState *s = opaque;
+    PCIDevice *pci_dev = PCI_DEVICE(&s->pm);
+    uint8_t irq = pci_get_byte(pci_dev->config + VIA_PM_SCI_SELECT_OFS)
+                  & VIA_PM_SCI_SELECT_MASK;
+
+    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_in[irq], level);
+    }
+}
+
 static void via_isa_init(Object *obj)
 {
     ViaISAState *s = VIA_ISA(obj);
+    DeviceState *dev = DEVICE(s);
 
     object_initialize_child(obj, "rtc", &s->rtc, TYPE_MC146818_RTC);
     object_initialize_child(obj, "ide", &s->ide, TYPE_VIA_IDE);
@@ -578,6 +598,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(dev, via_isa_set_pm_irq, "sci", 1);
 }
 
 static const TypeInfo via_isa_info = {
@@ -704,6 +726,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_named(DEVICE(&s->pm), "sci", 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.42.0

[PATCH v5 2/5] hw/isa/vt82c686: Add missing initialization of ACPI general purpose event registers

[Bernhard Beschow]

In order to be able to reuse acpi_update_sci(), these registers need to be
initialized.

In PIIX4, these registers are used to implement the Frankenstein hotplug
functionality and are mapped outside of the device's IO region. Don't do that
for VIA for now to avoid the Frankenstein functionality. Still, initialize them
to the size of the real VIA PM controller general purpose event registers but
stay conservative and don't map them for now.

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

diff --git a/hw/isa/vt82c686.c b/hw/isa/vt82c686.c
index aeb9434a46..60ca781e03 100644
--- a/hw/isa/vt82c686.c
+++ b/hw/isa/vt82c686.c
@@ -40,6 +40,8 @@
 #define TYPE_VIA_PM "via-pm"
 OBJECT_DECLARE_SIMPLE_TYPE(ViaPMState, VIA_PM)
 
+#define VIA_PM_GPE_LEN 4
+
 #define VIA_PM_SCI_SELECT_OFS 0x42
 #define VIA_PM_SCI_SELECT_MASK 0xf
 
@@ -179,6 +181,7 @@ static void via_pm_reset(DeviceState *d)
     acpi_pm1_evt_reset(&s->ar);
     acpi_pm1_cnt_reset(&s->ar);
     acpi_pm_tmr_reset(&s->ar);
+    acpi_gpe_reset(&s->ar);
     pm_update_sci(s);
 
     pm_io_space_update(s);
@@ -205,6 +208,7 @@ 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);
+    acpi_gpe_init(&s->ar, VIA_PM_GPE_LEN);
 }
 
 static void via_pm_init(Object *obj)
-- 
2.42.0

[PATCH v5 3/5] hw/isa/vt82c686: Reuse acpi_update_sci()

[Bernhard Beschow]

acpi_update_sci() covers everything pm_update_sci() does. It implements common
ACPI funtionality in a generic fashion. Note that it agnostic to any
Frankenstein usage of the general purpose event registers in other device
models. It just implements a generic mechanism which can be wired to arbitrary
functionality.

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

diff --git a/hw/isa/vt82c686.c b/hw/isa/vt82c686.c
index 60ca781e03..7b44ad9485 100644
--- a/hw/isa/vt82c686.c
+++ b/hw/isa/vt82c686.c
@@ -145,26 +145,10 @@ static const MemoryRegionOps pm_io_ops = {
     },
 };
 
-static void pm_update_sci(ViaPMState *s)
-{
-    int sci_level, pmsts;
-
-    pmsts = acpi_pm1_evt_get_sts(&s->ar);
-    sci_level = (((pmsts & s->ar.pm1.evt.en) &
-                  (ACPI_BITMASK_RT_CLOCK_ENABLE |
-                   ACPI_BITMASK_POWER_BUTTON_ENABLE |
-                   ACPI_BITMASK_GLOBAL_LOCK_ENABLE |
-                   ACPI_BITMASK_TIMER_ENABLE)) != 0);
-    qemu_set_irq(s->sci_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));
-}
-
 static void pm_tmr_timer(ACPIREGS *ar)
 {
     ViaPMState *s = container_of(ar, ViaPMState, ar);
-    pm_update_sci(s);
+    acpi_update_sci(&s->ar, s->sci_irq);
 }
 
 static void via_pm_reset(DeviceState *d)
@@ -182,7 +166,7 @@ static void via_pm_reset(DeviceState *d)
     acpi_pm1_cnt_reset(&s->ar);
     acpi_pm_tmr_reset(&s->ar);
     acpi_gpe_reset(&s->ar);
-    pm_update_sci(s);
+    acpi_update_sci(&s->ar, s->sci_irq);
 
     pm_io_space_update(s);
     smb_io_space_update(s);
-- 
2.42.0

[PATCH v5 4/5] hw/isa/vt82c686: Implement ACPI powerdown

[Bernhard Beschow]

Allows guests to be powered off via an ACPI power button event which can be
triggered e.g. through the GTK GUI.

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

diff --git a/hw/isa/vt82c686.c b/hw/isa/vt82c686.c
index 7b44ad9485..e8ec63dea9 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 TYPE_VIA_PM "via-pm"
@@ -52,6 +54,8 @@ struct ViaPMState {
     APMState apm;
     PMSMBus smb;
 
+    Notifier powerdown_notifier;
+
     qemu_irq sci_irq;
 };
 
@@ -172,6 +176,13 @@ 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);
+
+    acpi_pm1_evt_power_down(&s->ar);
+}
+
 static void via_pm_realize(PCIDevice *dev, Error **errp)
 {
     ViaPMState *s = VIA_PM(dev);
@@ -193,6 +204,9 @@ static void via_pm_realize(PCIDevice *dev, Error **errp)
     acpi_pm1_evt_init(&s->ar, pm_tmr_timer, &s->io);
     acpi_pm1_cnt_init(&s->ar, &s->io, false, false, 2, false);
     acpi_gpe_init(&s->ar, VIA_PM_GPE_LEN);
+
+    s->powerdown_notifier.notify = via_pm_powerdown_req;
+    qemu_register_powerdown_notifier(&s->powerdown_notifier);
 }
 
 static void via_pm_init(Object *obj)
-- 
2.42.0

[PATCH v5 5/5] hw/isa/vt82c686: Implement software-based SMI triggering

[Bernhard Beschow]

If enabled, SMIs can be triggered via software by writing to an IO-mapped port.
SMIs usually trigger execution of BIOS code. If appropriate values are written
to the port, the BIOS transitions the system into or out of ACPI mode.

Note that APMState implements Intel-specific behavior where there are two IO
ports which are mapped at fixed addresses. In VIA, there is only one such port
which is located inside a relocatable IO-mapped region. Hence, there is no point
in reusing APMState.

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

diff --git a/hw/isa/vt82c686.c b/hw/isa/vt82c686.c
index e8ec63dea9..361b3bed0a 100644
--- a/hw/isa/vt82c686.c
+++ b/hw/isa/vt82c686.c
@@ -27,7 +27,6 @@
 #include "hw/timer/i8254.h"
 #include "hw/rtc/mc146818rtc.h"
 #include "migration/vmstate.h"
-#include "hw/isa/apm.h"
 #include "hw/acpi/acpi.h"
 #include "hw/i2c/pm_smbus.h"
 #include "qapi/error.h"
@@ -42,6 +41,16 @@
 #define TYPE_VIA_PM "via-pm"
 OBJECT_DECLARE_SIMPLE_TYPE(ViaPMState, VIA_PM)
 
+#define VIA_PM_IO_GBLEN 0x2a
+#define VIA_PM_IO_GBLEN_SW_SMI_EN (1 << 6)
+
+#define VIA_PM_IO_GBLCTL 0x2c
+#define VIA_PM_IO_GBLCTL_SMI_EN 1
+#define VIA_PM_IO_GBLCTL_SMIIG (1 << 4)
+#define VIA_PM_IO_GBLCTL_INSMI (1 << 8)
+
+#define VIA_PM_IO_SMI_CMD 0x2f
+
 #define VIA_PM_GPE_LEN 4
 
 #define VIA_PM_SCI_SELECT_OFS 0x42
@@ -49,14 +58,19 @@ OBJECT_DECLARE_SIMPLE_TYPE(ViaPMState, VIA_PM)
 
 struct ViaPMState {
     PCIDevice dev;
+
     MemoryRegion io;
     ACPIREGS ar;
-    APMState apm;
+    uint16_t gbl_en;
+    uint16_t gbl_ctl;
+    uint8_t smi_cmd;
+
     PMSMBus smb;
 
     Notifier powerdown_notifier;
 
     qemu_irq sci_irq;
+    qemu_irq smi_irq;
 };
 
 static void pm_io_space_update(ViaPMState *s)
@@ -90,7 +104,7 @@ static int vmstate_acpi_post_load(void *opaque, int version_id)
 
 static const VMStateDescription vmstate_acpi = {
     .name = "vt82c686b_pm",
-    .version_id = 1,
+    .version_id = 2,
     .minimum_version_id = 1,
     .post_load = vmstate_acpi_post_load,
     .fields = (VMStateField[]) {
@@ -98,9 +112,11 @@ static const VMStateDescription vmstate_acpi = {
         VMSTATE_UINT16(ar.pm1.evt.sts, ViaPMState),
         VMSTATE_UINT16(ar.pm1.evt.en, ViaPMState),
         VMSTATE_UINT16(ar.pm1.cnt.cnt, ViaPMState),
-        VMSTATE_STRUCT(apm, ViaPMState, 0, vmstate_apm, APMState),
         VMSTATE_TIMER_PTR(ar.tmr.timer, ViaPMState),
         VMSTATE_INT64(ar.tmr.overflow_time, ViaPMState),
+        VMSTATE_UINT16(gbl_en, ViaPMState),
+        VMSTATE_UINT16(gbl_ctl, ViaPMState),
+        VMSTATE_UINT8(smi_cmd, ViaPMState),
         VMSTATE_END_OF_LIST()
     }
 };
@@ -128,15 +144,75 @@ static void pm_write_config(PCIDevice *d, uint32_t addr, uint32_t val, int len)
     }
 }
 
+static void via_pm_apm_ctrl_changed(ViaPMState *s, uint8_t val)
+{
+    s->smi_cmd = val;
+
+    if (s->gbl_en & VIA_PM_IO_GBLEN_SW_SMI_EN
+        && s->gbl_ctl & VIA_PM_IO_GBLCTL_SMI_EN
+        && !(s->gbl_ctl & VIA_PM_IO_GBLCTL_SMIIG
+             && s->gbl_ctl & VIA_PM_IO_GBLCTL_INSMI)) {
+        s->gbl_ctl |= VIA_PM_IO_GBLCTL_INSMI;
+
+        if (s->smi_irq) {
+            qemu_irq_raise(s->smi_irq);
+        }
+    }
+}
+
 static void pm_io_write(void *op, hwaddr addr, uint64_t data, unsigned size)
 {
+    ViaPMState *s = op;
+
     trace_via_pm_io_write(addr, data, size);
+
+    switch (addr) {
+    case VIA_PM_IO_GBLEN:
+        s->gbl_en = (s->gbl_en & 0xff00) | data;
+        break;
+    case VIA_PM_IO_GBLEN + 1:
+        s->gbl_en = (s->gbl_en & 0x00ff) | (data << 8);
+        break;
+    case VIA_PM_IO_GBLCTL:
+        s->gbl_ctl = (s->gbl_ctl & 0xff00) | data;
+        break;
+    case VIA_PM_IO_GBLCTL + 1:
+        data <<= 8;
+        data &= ~(s->gbl_ctl & VIA_PM_IO_GBLCTL_INSMI);
+        s->gbl_ctl = (s->gbl_ctl & 0x00ff) | data;
+        break;
+    case VIA_PM_IO_SMI_CMD:
+        via_pm_apm_ctrl_changed(s, data);
+        break;
+    }
 }
 
 static uint64_t pm_io_read(void *op, hwaddr addr, unsigned size)
 {
-    trace_via_pm_io_read(addr, 0, size);
-    return 0;
+    ViaPMState *s = op;
+    uint64_t data = 0;
+
+    switch (addr) {
+    case VIA_PM_IO_GBLEN:
+        data = s->gbl_en & 0xff;
+        break;
+    case VIA_PM_IO_GBLEN + 1:
+        data = s->gbl_en >> 8;
+        break;
+    case VIA_PM_IO_GBLCTL:
+        data = s->gbl_ctl & 0xff;
+        break;
+    case VIA_PM_IO_GBLCTL + 1:
+        data = (s->gbl_ctl >> 8) & 0xd;
+        break;
+    case VIA_PM_IO_SMI_CMD:
+        data = s->smi_cmd;
+        break;
+    }
+
+    trace_via_pm_io_read(addr, data, size);
+
+    return data;
 }
 
 static const MemoryRegionOps pm_io_ops = {
@@ -166,6 +242,10 @@ static void via_pm_reset(DeviceState *d)
     /* SMBus IO base */
     pci_set_long(s->dev.config + 0x90, 1);
 
+    s->gbl_en = 0;
+    s->gbl_ctl = VIA_PM_IO_GBLCTL_SMIIG;
+    s->smi_cmd = 0;
+
     acpi_pm1_evt_reset(&s->ar);
     acpi_pm1_cnt_reset(&s->ar);
     acpi_pm_tmr_reset(&s->ar);
@@ -194,8 +274,6 @@ 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);
-
     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);
     memory_region_set_enabled(&s->io, false);
@@ -214,6 +292,7 @@ static void via_pm_init(Object *obj)
     ViaPMState *s = VIA_PM(obj);
 
     qdev_init_gpio_out_named(DEVICE(obj), &s->sci_irq, "sci", 1);
+    qdev_init_gpio_out_named(DEVICE(obj), &s->smi_irq, "smi-irq", 1);
 }
 
 typedef struct via_pm_init_info {
-- 
2.42.0

Re: [PATCH v5 0/5] VIA PM: Implement basic ACPI support

[BALATON Zoltan]

Hello,

On Sat, 28 Oct 2023, Bernhard Beschow wrote:
> This series is part of my work to bring the VIA south bridges to the PC machine
> [1]. It implements missing ACPI functionality which ACPI-aware x86 guests
> expect for a smooth experience. The implementation is heavily inspired by PIIX4.

I think first the interrupt routing should be fixed because that may 
change a few things in this series so that should be the next step and 
then rebase this series on top of that.

What I mean by fixing interrupt routing? You may remember this discussion:

https://patchew.org/QEMU/cover.1677004414.git.balaton@eik.bme.hu/

With pegasos2 your (over)simplification worked only because the firmware 
of that machine maps everythnig to one ISA IRQ and guests were happy with 
that. I told you that back then but could not convince you and Mark about 
that. Now with the amigaone machine the firmware maps VIA devices and PCI 
interuupt pins to different ISA IRQs so we need to go back either to my 
otiginal implementation or something else you come up with. You can test 
this trying to use USB devices with amigaone machine which only works 
after reverting 4e5a20b6da9b1 and 422a6e8075752. So please either propose 
something to fix that first or wait with this series until I can update my 
pathches to solve interrupt routing. I think this series should wait until 
after that because it adds more interrupt handling which should follow 
whatever way we come up with for that so it's too early fir this series 
yet. (If you want to try fixing it keep in mind that in both amigaone and 
pegasos2 the PCI buses are in the north bridge not in the VIA south bridge 
so don't try to force the IRQ mapping into the PCI bus. All the VIA chip 
needs to do is mapping its PIRQ/PINT pins to ISA IRQs as the VIA is only 
handling ISA IRQs and all other pci stuff is handled in the north bridge. 
So I think we need a via_set_isa_irq function but we could change it 
according to Mark's idea to pass the PCI device and use its function 
number to select itq source instead of the enum I had in my original 
series.)

I have some other comments that I'll add in reply to individual patches 
for the future/

Regards,
BALATON Zoltan

> Further quirks are needed in order to use the VIA south bridges in the PC
> machine. These were deliberately left out for a future series. The idea for now
> is to get the device model in shape for adding support for it in SeaBIOS.
>
> The series is structured as follows: The first patch fixes ACPI events to be
> signalled by SCI interrupts. The next three patches implement typical ACPI
> event handling. The last patch adds software-based SMI triggering which is the
> mechanism used in ACPI to transition the system into ACPI mode.
>
> Testing done:
> * `make check`
> * `make check-avocado`
> * `qemu-system-ppc -M pegasos2 \
>                   -device ati-vga,romfile="" \
>                   -cdrom morphos-3.18.iso \
>                   -bios pegasos2.rom`
>
> [1] https://github.com/shentok/qemu/tree/pc-via
>
> v5:
> * Implement software-based SMI triggering and handling of ACPI events based on
>  v3
>
> v4:
> * Alternative proposal (Zoltan)
>
> v3: https://patchew.org/QEMU/20231005115159.81202-1-shentey@gmail.com/
> * Rename SCI irq attribute to sci_irq (Zoltan)
> * Fix confusion about location of ACPI interrupt select register (Zoltan)
> * Model SCI as named GPIO (Bernhard)
> * Perform upcast via macro rather than sub structure selection (Bernhard)
>
> v2:
> * Introduce named constants for the ACPI interrupt select register at offset
>  0x42 (Phil)
>
> Bernhard Beschow (5):
>  hw/isa/vt82c686: Respect SCI interrupt assignment
>  hw/isa/vt82c686: Add missing initialization of ACPI general purpose
>    event registers
>  hw/isa/vt82c686: Reuse acpi_update_sci()
>  hw/isa/vt82c686: Implement ACPI powerdown
>  hw/isa/vt82c686: Implement software-based SMI triggering
>
> hw/isa/vt82c686.c | 179 ++++++++++++++++++++++++++++++++++++----------
> 1 file changed, 142 insertions(+), 37 deletions(-)
>
> --
> 2.42.0
>
>
>

Re: [PATCH v5 3/5] hw/isa/vt82c686: Reuse acpi_update_sci()

[BALATON Zoltan]

On Sat, 28 Oct 2023, Bernhard Beschow wrote:
> acpi_update_sci() covers everything pm_update_sci() does. It implements common
> ACPI funtionality in a generic fashion. Note that it agnostic to any
> Frankenstein usage of the general purpose event registers in other device
> models. It just implements a generic mechanism which can be wired to arbitrary
> functionality.
>
> Signed-off-by: Bernhard Beschow <shentey@gmail.com>
> ---
> hw/isa/vt82c686.c | 20 ++------------------
> 1 file changed, 2 insertions(+), 18 deletions(-)
>
> diff --git a/hw/isa/vt82c686.c b/hw/isa/vt82c686.c
> index 60ca781e03..7b44ad9485 100644
> --- a/hw/isa/vt82c686.c
> +++ b/hw/isa/vt82c686.c
> @@ -145,26 +145,10 @@ static const MemoryRegionOps pm_io_ops = {
>     },
> };
>
> -static void pm_update_sci(ViaPMState *s)
> -{
> -    int sci_level, pmsts;
> -
> -    pmsts = acpi_pm1_evt_get_sts(&s->ar);
> -    sci_level = (((pmsts & s->ar.pm1.evt.en) &
> -                  (ACPI_BITMASK_RT_CLOCK_ENABLE |
> -                   ACPI_BITMASK_POWER_BUTTON_ENABLE |
> -                   ACPI_BITMASK_GLOBAL_LOCK_ENABLE |
> -                   ACPI_BITMASK_TIMER_ENABLE)) != 0);

The level calculation in acpi_update_sci() is different than the above. 
Which one is correct and why?

Regards,
BALATON Zoltan

> -    qemu_set_irq(s->sci_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));
> -}
> -
> static void pm_tmr_timer(ACPIREGS *ar)
> {
>     ViaPMState *s = container_of(ar, ViaPMState, ar);
> -    pm_update_sci(s);
> +    acpi_update_sci(&s->ar, s->sci_irq);
> }
>
> static void via_pm_reset(DeviceState *d)
> @@ -182,7 +166,7 @@ static void via_pm_reset(DeviceState *d)
>     acpi_pm1_cnt_reset(&s->ar);
>     acpi_pm_tmr_reset(&s->ar);
>     acpi_gpe_reset(&s->ar);
> -    pm_update_sci(s);
> +    acpi_update_sci(&s->ar, s->sci_irq);
>
>     pm_io_space_update(s);
>     smb_io_space_update(s);
>

Re: [PATCH v5 5/5] hw/isa/vt82c686: Implement software-based SMI triggering

[BALATON Zoltan]

On Sat, 28 Oct 2023, Bernhard Beschow wrote:
> If enabled, SMIs can be triggered via software by writing to an IO-mapped port.
> SMIs usually trigger execution of BIOS code. If appropriate values are written
> to the port, the BIOS transitions the system into or out of ACPI mode.
>
> Note that APMState implements Intel-specific behavior where there are two IO
> ports which are mapped at fixed addresses. In VIA, there is only one such port
> which is located inside a relocatable IO-mapped region. Hence, there is no point
> in reusing APMState.
>
> Signed-off-by: Bernhard Beschow <shentey@gmail.com>
> ---
> hw/isa/vt82c686.c | 95 +++++++++++++++++++++++++++++++++++++++++++----
> 1 file changed, 87 insertions(+), 8 deletions(-)
>
> diff --git a/hw/isa/vt82c686.c b/hw/isa/vt82c686.c
> index e8ec63dea9..361b3bed0a 100644
> --- a/hw/isa/vt82c686.c
> +++ b/hw/isa/vt82c686.c
> @@ -27,7 +27,6 @@
> #include "hw/timer/i8254.h"
> #include "hw/rtc/mc146818rtc.h"
> #include "migration/vmstate.h"
> -#include "hw/isa/apm.h"
> #include "hw/acpi/acpi.h"
> #include "hw/i2c/pm_smbus.h"
> #include "qapi/error.h"
> @@ -42,6 +41,16 @@
> #define TYPE_VIA_PM "via-pm"
> OBJECT_DECLARE_SIMPLE_TYPE(ViaPMState, VIA_PM)
>
> +#define VIA_PM_IO_GBLEN 0x2a
> +#define VIA_PM_IO_GBLEN_SW_SMI_EN (1 << 6)
> +
> +#define VIA_PM_IO_GBLCTL 0x2c
> +#define VIA_PM_IO_GBLCTL_SMI_EN 1
> +#define VIA_PM_IO_GBLCTL_SMIIG (1 << 4)
> +#define VIA_PM_IO_GBLCTL_INSMI (1 << 8)
> +
> +#define VIA_PM_IO_SMI_CMD 0x2f
> +
> #define VIA_PM_GPE_LEN 4
>
> #define VIA_PM_SCI_SELECT_OFS 0x42

If we'll make a copy of the data sheet in form of #defines could these be 
in the header to less clutter the source?

Regards,
BALATON Zoltan

> @@ -49,14 +58,19 @@ OBJECT_DECLARE_SIMPLE_TYPE(ViaPMState, VIA_PM)
>
> struct ViaPMState {
>     PCIDevice dev;
> +
>     MemoryRegion io;
>     ACPIREGS ar;
> -    APMState apm;
> +    uint16_t gbl_en;
> +    uint16_t gbl_ctl;
> +    uint8_t smi_cmd;
> +
>     PMSMBus smb;
>
>     Notifier powerdown_notifier;
>
>     qemu_irq sci_irq;
> +    qemu_irq smi_irq;
> };
>
> static void pm_io_space_update(ViaPMState *s)
> @@ -90,7 +104,7 @@ static int vmstate_acpi_post_load(void *opaque, int version_id)
>
> static const VMStateDescription vmstate_acpi = {
>     .name = "vt82c686b_pm",
> -    .version_id = 1,
> +    .version_id = 2,
>     .minimum_version_id = 1,
>     .post_load = vmstate_acpi_post_load,
>     .fields = (VMStateField[]) {
> @@ -98,9 +112,11 @@ static const VMStateDescription vmstate_acpi = {
>         VMSTATE_UINT16(ar.pm1.evt.sts, ViaPMState),
>         VMSTATE_UINT16(ar.pm1.evt.en, ViaPMState),
>         VMSTATE_UINT16(ar.pm1.cnt.cnt, ViaPMState),
> -        VMSTATE_STRUCT(apm, ViaPMState, 0, vmstate_apm, APMState),
>         VMSTATE_TIMER_PTR(ar.tmr.timer, ViaPMState),
>         VMSTATE_INT64(ar.tmr.overflow_time, ViaPMState),
> +        VMSTATE_UINT16(gbl_en, ViaPMState),
> +        VMSTATE_UINT16(gbl_ctl, ViaPMState),
> +        VMSTATE_UINT8(smi_cmd, ViaPMState),
>         VMSTATE_END_OF_LIST()
>     }
> };
> @@ -128,15 +144,75 @@ static void pm_write_config(PCIDevice *d, uint32_t addr, uint32_t val, int len)
>     }
> }
>
> +static void via_pm_apm_ctrl_changed(ViaPMState *s, uint8_t val)
> +{
> +    s->smi_cmd = val;
> +
> +    if (s->gbl_en & VIA_PM_IO_GBLEN_SW_SMI_EN
> +        && s->gbl_ctl & VIA_PM_IO_GBLCTL_SMI_EN
> +        && !(s->gbl_ctl & VIA_PM_IO_GBLCTL_SMIIG
> +             && s->gbl_ctl & VIA_PM_IO_GBLCTL_INSMI)) {
> +        s->gbl_ctl |= VIA_PM_IO_GBLCTL_INSMI;
> +
> +        if (s->smi_irq) {
> +            qemu_irq_raise(s->smi_irq);
> +        }
> +    }
> +}
> +
> static void pm_io_write(void *op, hwaddr addr, uint64_t data, unsigned size)
> {
> +    ViaPMState *s = op;
> +
>     trace_via_pm_io_write(addr, data, size);
> +
> +    switch (addr) {
> +    case VIA_PM_IO_GBLEN:
> +        s->gbl_en = (s->gbl_en & 0xff00) | data;
> +        break;
> +    case VIA_PM_IO_GBLEN + 1:
> +        s->gbl_en = (s->gbl_en & 0x00ff) | (data << 8);
> +        break;
> +    case VIA_PM_IO_GBLCTL:
> +        s->gbl_ctl = (s->gbl_ctl & 0xff00) | data;
> +        break;
> +    case VIA_PM_IO_GBLCTL + 1:
> +        data <<= 8;
> +        data &= ~(s->gbl_ctl & VIA_PM_IO_GBLCTL_INSMI);
> +        s->gbl_ctl = (s->gbl_ctl & 0x00ff) | data;
> +        break;
> +    case VIA_PM_IO_SMI_CMD:
> +        via_pm_apm_ctrl_changed(s, data);
> +        break;
> +    }
> }
>
> static uint64_t pm_io_read(void *op, hwaddr addr, unsigned size)
> {
> -    trace_via_pm_io_read(addr, 0, size);
> -    return 0;
> +    ViaPMState *s = op;
> +    uint64_t data = 0;
> +
> +    switch (addr) {
> +    case VIA_PM_IO_GBLEN:
> +        data = s->gbl_en & 0xff;
> +        break;
> +    case VIA_PM_IO_GBLEN + 1:
> +        data = s->gbl_en >> 8;
> +        break;
> +    case VIA_PM_IO_GBLCTL:
> +        data = s->gbl_ctl & 0xff;
> +        break;
> +    case VIA_PM_IO_GBLCTL + 1:
> +        data = (s->gbl_ctl >> 8) & 0xd;
> +        break;
> +    case VIA_PM_IO_SMI_CMD:
> +        data = s->smi_cmd;
> +        break;
> +    }
> +
> +    trace_via_pm_io_read(addr, data, size);
> +
> +    return data;
> }
>
> static const MemoryRegionOps pm_io_ops = {
> @@ -166,6 +242,10 @@ static void via_pm_reset(DeviceState *d)
>     /* SMBus IO base */
>     pci_set_long(s->dev.config + 0x90, 1);
>
> +    s->gbl_en = 0;
> +    s->gbl_ctl = VIA_PM_IO_GBLCTL_SMIIG;
> +    s->smi_cmd = 0;
> +
>     acpi_pm1_evt_reset(&s->ar);
>     acpi_pm1_cnt_reset(&s->ar);
>     acpi_pm_tmr_reset(&s->ar);
> @@ -194,8 +274,6 @@ 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);
> -
>     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);
>     memory_region_set_enabled(&s->io, false);
> @@ -214,6 +292,7 @@ static void via_pm_init(Object *obj)
>     ViaPMState *s = VIA_PM(obj);
>
>     qdev_init_gpio_out_named(DEVICE(obj), &s->sci_irq, "sci", 1);
> +    qdev_init_gpio_out_named(DEVICE(obj), &s->smi_irq, "smi-irq", 1);
> }
>
> typedef struct via_pm_init_info {
>

Re: [PATCH v5 0/5] VIA PM: Implement basic ACPI support

[Bernhard Beschow]

Am 28. Oktober 2023 12:58:32 UTC schrieb BALATON Zoltan <balaton@eik.bme.hu>:
>Hello,
>
>On Sat, 28 Oct 2023, Bernhard Beschow wrote:
>> This series is part of my work to bring the VIA south bridges to the PC machine
>> [1]. It implements missing ACPI functionality which ACPI-aware x86 guests
>> expect for a smooth experience. The implementation is heavily inspired by PIIX4.
>
>I think first the interrupt routing should be fixed because that may change a few things in this series so that should be the next step and then rebase this series on top of that.
>
>What I mean by fixing interrupt routing? You may remember this discussion:
>
>https://patchew.org/QEMU/cover.1677004414.git.balaton@eik.bme.hu/
>
>With pegasos2 your (over)simplification worked only because the firmware of that machine maps everythnig to one ISA IRQ and guests were happy with that. I told you that back then but could not convince you and Mark about that. Now with the amigaone machine the firmware maps VIA devices and PCI interuupt pins to different ISA IRQs so we need to go back either to my otiginal implementation or something else you come up with. You can test this trying to use USB devices with amigaone machine which only works after reverting 4e5a20b6da9b1 and 422a6e8075752. So please either propose something to fix that first or wait with this series until I can update my pathches to solve interrupt routing. I think this series should wait until after that because it adds more interrupt handling which should follow whatever way we come up with for that so it's too early fir this series yet. (If you want to try fixing it keep in mind that in both amigaone and pegasos2 the PCI buses are in the north bridge not in the VIA south bridge so don't try to force the IRQ mapping into the PCI bus. All the VIA chip needs to do is mapping its PIRQ/PINT pins to ISA IRQs as the VIA is only handling ISA IRQs and all other pci stuff is handled in the north bridge. So I think we need a via_set_isa_irq function but we could change it according to Mark's idea to pass the PCI device and use its function number to select itq source instead of the enum I had in my original series.)
>
>I have some other comments that I'll add in reply to individual patches for the future/

Hi Zoltan,

The interrupt handling introduced in this series is not related to PCI interrupt routing: The SMI is a dedicated pin on the device and the SCI is mapped internally to an ISA interrupt (note how the power management function lacks the registers for PCI interrupt information). Hence, PCI interrupt routing isn't changed in this series and therefore seems off-topic to me.

Moreover, the SMI is a new interrupt which is therefore not used in any machine yet. The SCI is deactivated if set to IRQ 0 which is the default even. If a guest configures it, it shall be aware to receive an *ISA* interrupt.

So I think this series shouldn't conflict with any previous work and should not be blocked by the PCI IRQ routing topic.

Best regards,
Bernhard

>
>Regards,
>BALATON Zoltan
>
>> Further quirks are needed in order to use the VIA south bridges in the PC
>> machine. These were deliberately left out for a future series. The idea for now
>> is to get the device model in shape for adding support for it in SeaBIOS.
>> 
>> The series is structured as follows: The first patch fixes ACPI events to be
>> signalled by SCI interrupts. The next three patches implement typical ACPI
>> event handling. The last patch adds software-based SMI triggering which is the
>> mechanism used in ACPI to transition the system into ACPI mode.
>> 
>> Testing done:
>> * `make check`
>> * `make check-avocado`
>> * `qemu-system-ppc -M pegasos2 \
>>                   -device ati-vga,romfile="" \
>>                   -cdrom morphos-3.18.iso \
>>                   -bios pegasos2.rom`
>> 
>> [1] https://github.com/shentok/qemu/tree/pc-via
>> 
>> v5:
>> * Implement software-based SMI triggering and handling of ACPI events based on
>>  v3
>> 
>> v4:
>> * Alternative proposal (Zoltan)
>> 
>> v3: https://patchew.org/QEMU/20231005115159.81202-1-shentey@gmail.com/
>> * Rename SCI irq attribute to sci_irq (Zoltan)
>> * Fix confusion about location of ACPI interrupt select register (Zoltan)
>> * Model SCI as named GPIO (Bernhard)
>> * Perform upcast via macro rather than sub structure selection (Bernhard)
>> 
>> v2:
>> * Introduce named constants for the ACPI interrupt select register at offset
>>  0x42 (Phil)
>> 
>> Bernhard Beschow (5):
>>  hw/isa/vt82c686: Respect SCI interrupt assignment
>>  hw/isa/vt82c686: Add missing initialization of ACPI general purpose
>>    event registers
>>  hw/isa/vt82c686: Reuse acpi_update_sci()
>>  hw/isa/vt82c686: Implement ACPI powerdown
>>  hw/isa/vt82c686: Implement software-based SMI triggering
>> 
>> hw/isa/vt82c686.c | 179 ++++++++++++++++++++++++++++++++++++----------
>> 1 file changed, 142 insertions(+), 37 deletions(-)
>> 
>> --
>> 2.42.0
>> 
>> 
>> 

Re: [PATCH v5 3/5] hw/isa/vt82c686: Reuse acpi_update_sci()

[Bernhard Beschow]

Am 28. Oktober 2023 12:59:56 UTC schrieb BALATON Zoltan <balaton@eik.bme.hu>:
>On Sat, 28 Oct 2023, Bernhard Beschow wrote:
>> acpi_update_sci() covers everything pm_update_sci() does. It implements common
>> ACPI funtionality in a generic fashion. Note that it agnostic to any
>> Frankenstein usage of the general purpose event registers in other device
>> models. It just implements a generic mechanism which can be wired to arbitrary
>> functionality.
>> 
>> Signed-off-by: Bernhard Beschow <shentey@gmail.com>
>> ---
>> hw/isa/vt82c686.c | 20 ++------------------
>> 1 file changed, 2 insertions(+), 18 deletions(-)
>> 
>> diff --git a/hw/isa/vt82c686.c b/hw/isa/vt82c686.c
>> index 60ca781e03..7b44ad9485 100644
>> --- a/hw/isa/vt82c686.c
>> +++ b/hw/isa/vt82c686.c
>> @@ -145,26 +145,10 @@ static const MemoryRegionOps pm_io_ops = {
>>     },
>> };
>> 
>> -static void pm_update_sci(ViaPMState *s)
>> -{
>> -    int sci_level, pmsts;
>> -
>> -    pmsts = acpi_pm1_evt_get_sts(&s->ar);
>> -    sci_level = (((pmsts & s->ar.pm1.evt.en) &
>> -                  (ACPI_BITMASK_RT_CLOCK_ENABLE |
>> -                   ACPI_BITMASK_POWER_BUTTON_ENABLE |
>> -                   ACPI_BITMASK_GLOBAL_LOCK_ENABLE |
>> -                   ACPI_BITMASK_TIMER_ENABLE)) != 0);
>
>The level calculation in acpi_update_sci() is different than the above. Which one is correct and why?

acpi_update_sci() just covers the GPE registers in addition which are standard ACPI registers. As written in the commit message these aren't currently mapped by the device model so shouldn't cause any interferences.

Best regards,
Bernhard

>
>Regards,
>BALATON Zoltan
>
>> -    qemu_set_irq(s->sci_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));
>> -}
>> -
>> static void pm_tmr_timer(ACPIREGS *ar)
>> {
>>     ViaPMState *s = container_of(ar, ViaPMState, ar);
>> -    pm_update_sci(s);
>> +    acpi_update_sci(&s->ar, s->sci_irq);
>> }
>> 
>> static void via_pm_reset(DeviceState *d)
>> @@ -182,7 +166,7 @@ static void via_pm_reset(DeviceState *d)
>>     acpi_pm1_cnt_reset(&s->ar);
>>     acpi_pm_tmr_reset(&s->ar);
>>     acpi_gpe_reset(&s->ar);
>> -    pm_update_sci(s);
>> +    acpi_update_sci(&s->ar, s->sci_irq);
>> 
>>     pm_io_space_update(s);
>>     smb_io_space_update(s);
>> 

Re: [PATCH v5 5/5] hw/isa/vt82c686: Implement software-based SMI triggering

[Bernhard Beschow]

Am 28. Oktober 2023 13:03:41 UTC schrieb BALATON Zoltan <balaton@eik.bme.hu>:
>On Sat, 28 Oct 2023, Bernhard Beschow wrote:
>> If enabled, SMIs can be triggered via software by writing to an IO-mapped port.
>> SMIs usually trigger execution of BIOS code. If appropriate values are written
>> to the port, the BIOS transitions the system into or out of ACPI mode.
>> 
>> Note that APMState implements Intel-specific behavior where there are two IO
>> ports which are mapped at fixed addresses. In VIA, there is only one such port
>> which is located inside a relocatable IO-mapped region. Hence, there is no point
>> in reusing APMState.
>> 
>> Signed-off-by: Bernhard Beschow <shentey@gmail.com>
>> ---
>> hw/isa/vt82c686.c | 95 +++++++++++++++++++++++++++++++++++++++++++----
>> 1 file changed, 87 insertions(+), 8 deletions(-)
>> 
>> diff --git a/hw/isa/vt82c686.c b/hw/isa/vt82c686.c
>> index e8ec63dea9..361b3bed0a 100644
>> --- a/hw/isa/vt82c686.c
>> +++ b/hw/isa/vt82c686.c
>> @@ -27,7 +27,6 @@
>> #include "hw/timer/i8254.h"
>> #include "hw/rtc/mc146818rtc.h"
>> #include "migration/vmstate.h"
>> -#include "hw/isa/apm.h"
>> #include "hw/acpi/acpi.h"
>> #include "hw/i2c/pm_smbus.h"
>> #include "qapi/error.h"
>> @@ -42,6 +41,16 @@
>> #define TYPE_VIA_PM "via-pm"
>> OBJECT_DECLARE_SIMPLE_TYPE(ViaPMState, VIA_PM)
>> 
>> +#define VIA_PM_IO_GBLEN 0x2a
>> +#define VIA_PM_IO_GBLEN_SW_SMI_EN (1 << 6)
>> +
>> +#define VIA_PM_IO_GBLCTL 0x2c
>> +#define VIA_PM_IO_GBLCTL_SMI_EN 1
>> +#define VIA_PM_IO_GBLCTL_SMIIG (1 << 4)
>> +#define VIA_PM_IO_GBLCTL_INSMI (1 << 8)
>> +
>> +#define VIA_PM_IO_SMI_CMD 0x2f
>> +
>> #define VIA_PM_GPE_LEN 4
>> 
>> #define VIA_PM_SCI_SELECT_OFS 0x42
>
>If we'll make a copy of the data sheet in form of #defines could these be in the header to less clutter the source?

Last time I did that I was asked to move the defines back into the source file. I can't find the link right now, otherwise I'd have placed it here.

Best regards,
Bernhard

>
>Regards,
>BALATON Zoltan
>
>> @@ -49,14 +58,19 @@ OBJECT_DECLARE_SIMPLE_TYPE(ViaPMState, VIA_PM)
>> 
>> struct ViaPMState {
>>     PCIDevice dev;
>> +
>>     MemoryRegion io;
>>     ACPIREGS ar;
>> -    APMState apm;
>> +    uint16_t gbl_en;
>> +    uint16_t gbl_ctl;
>> +    uint8_t smi_cmd;
>> +
>>     PMSMBus smb;
>> 
>>     Notifier powerdown_notifier;
>> 
>>     qemu_irq sci_irq;
>> +    qemu_irq smi_irq;
>> };
>> 
>> static void pm_io_space_update(ViaPMState *s)
>> @@ -90,7 +104,7 @@ static int vmstate_acpi_post_load(void *opaque, int version_id)
>> 
>> static const VMStateDescription vmstate_acpi = {
>>     .name = "vt82c686b_pm",
>> -    .version_id = 1,
>> +    .version_id = 2,
>>     .minimum_version_id = 1,
>>     .post_load = vmstate_acpi_post_load,
>>     .fields = (VMStateField[]) {
>> @@ -98,9 +112,11 @@ static const VMStateDescription vmstate_acpi = {
>>         VMSTATE_UINT16(ar.pm1.evt.sts, ViaPMState),
>>         VMSTATE_UINT16(ar.pm1.evt.en, ViaPMState),
>>         VMSTATE_UINT16(ar.pm1.cnt.cnt, ViaPMState),
>> -        VMSTATE_STRUCT(apm, ViaPMState, 0, vmstate_apm, APMState),
>>         VMSTATE_TIMER_PTR(ar.tmr.timer, ViaPMState),
>>         VMSTATE_INT64(ar.tmr.overflow_time, ViaPMState),
>> +        VMSTATE_UINT16(gbl_en, ViaPMState),
>> +        VMSTATE_UINT16(gbl_ctl, ViaPMState),
>> +        VMSTATE_UINT8(smi_cmd, ViaPMState),
>>         VMSTATE_END_OF_LIST()
>>     }
>> };
>> @@ -128,15 +144,75 @@ static void pm_write_config(PCIDevice *d, uint32_t addr, uint32_t val, int len)
>>     }
>> }
>> 
>> +static void via_pm_apm_ctrl_changed(ViaPMState *s, uint8_t val)
>> +{
>> +    s->smi_cmd = val;
>> +
>> +    if (s->gbl_en & VIA_PM_IO_GBLEN_SW_SMI_EN
>> +        && s->gbl_ctl & VIA_PM_IO_GBLCTL_SMI_EN
>> +        && !(s->gbl_ctl & VIA_PM_IO_GBLCTL_SMIIG
>> +             && s->gbl_ctl & VIA_PM_IO_GBLCTL_INSMI)) {
>> +        s->gbl_ctl |= VIA_PM_IO_GBLCTL_INSMI;
>> +
>> +        if (s->smi_irq) {
>> +            qemu_irq_raise(s->smi_irq);
>> +        }
>> +    }
>> +}
>> +
>> static void pm_io_write(void *op, hwaddr addr, uint64_t data, unsigned size)
>> {
>> +    ViaPMState *s = op;
>> +
>>     trace_via_pm_io_write(addr, data, size);
>> +
>> +    switch (addr) {
>> +    case VIA_PM_IO_GBLEN:
>> +        s->gbl_en = (s->gbl_en & 0xff00) | data;
>> +        break;
>> +    case VIA_PM_IO_GBLEN + 1:
>> +        s->gbl_en = (s->gbl_en & 0x00ff) | (data << 8);
>> +        break;
>> +    case VIA_PM_IO_GBLCTL:
>> +        s->gbl_ctl = (s->gbl_ctl & 0xff00) | data;
>> +        break;
>> +    case VIA_PM_IO_GBLCTL + 1:
>> +        data <<= 8;
>> +        data &= ~(s->gbl_ctl & VIA_PM_IO_GBLCTL_INSMI);
>> +        s->gbl_ctl = (s->gbl_ctl & 0x00ff) | data;
>> +        break;
>> +    case VIA_PM_IO_SMI_CMD:
>> +        via_pm_apm_ctrl_changed(s, data);
>> +        break;
>> +    }
>> }
>> 
>> static uint64_t pm_io_read(void *op, hwaddr addr, unsigned size)
>> {
>> -    trace_via_pm_io_read(addr, 0, size);
>> -    return 0;
>> +    ViaPMState *s = op;
>> +    uint64_t data = 0;
>> +
>> +    switch (addr) {
>> +    case VIA_PM_IO_GBLEN:
>> +        data = s->gbl_en & 0xff;
>> +        break;
>> +    case VIA_PM_IO_GBLEN + 1:
>> +        data = s->gbl_en >> 8;
>> +        break;
>> +    case VIA_PM_IO_GBLCTL:
>> +        data = s->gbl_ctl & 0xff;
>> +        break;
>> +    case VIA_PM_IO_GBLCTL + 1:
>> +        data = (s->gbl_ctl >> 8) & 0xd;
>> +        break;
>> +    case VIA_PM_IO_SMI_CMD:
>> +        data = s->smi_cmd;
>> +        break;
>> +    }
>> +
>> +    trace_via_pm_io_read(addr, data, size);
>> +
>> +    return data;
>> }
>> 
>> static const MemoryRegionOps pm_io_ops = {
>> @@ -166,6 +242,10 @@ static void via_pm_reset(DeviceState *d)
>>     /* SMBus IO base */
>>     pci_set_long(s->dev.config + 0x90, 1);
>> 
>> +    s->gbl_en = 0;
>> +    s->gbl_ctl = VIA_PM_IO_GBLCTL_SMIIG;
>> +    s->smi_cmd = 0;
>> +
>>     acpi_pm1_evt_reset(&s->ar);
>>     acpi_pm1_cnt_reset(&s->ar);
>>     acpi_pm_tmr_reset(&s->ar);
>> @@ -194,8 +274,6 @@ 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);
>> -
>>     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);
>>     memory_region_set_enabled(&s->io, false);
>> @@ -214,6 +292,7 @@ static void via_pm_init(Object *obj)
>>     ViaPMState *s = VIA_PM(obj);
>> 
>>     qdev_init_gpio_out_named(DEVICE(obj), &s->sci_irq, "sci", 1);
>> +    qdev_init_gpio_out_named(DEVICE(obj), &s->smi_irq, "smi-irq", 1);
>> }
>> 
>> typedef struct via_pm_init_info {
>> 

Re: [PATCH v5 5/5] hw/isa/vt82c686: Implement software-based SMI triggering

[BALATON Zoltan]

On Sat, 28 Oct 2023, Bernhard Beschow wrote:
> Am 28. Oktober 2023 13:03:41 UTC schrieb BALATON Zoltan <balaton@eik.bme.hu>:
>> On Sat, 28 Oct 2023, Bernhard Beschow wrote:
>>> If enabled, SMIs can be triggered via software by writing to an IO-mapped port.
>>> SMIs usually trigger execution of BIOS code. If appropriate values are written
>>> to the port, the BIOS transitions the system into or out of ACPI mode.
>>>
>>> Note that APMState implements Intel-specific behavior where there are two IO
>>> ports which are mapped at fixed addresses. In VIA, there is only one such port
>>> which is located inside a relocatable IO-mapped region. Hence, there is no point
>>> in reusing APMState.
>>>
>>> Signed-off-by: Bernhard Beschow <shentey@gmail.com>
>>> ---
>>> hw/isa/vt82c686.c | 95 +++++++++++++++++++++++++++++++++++++++++++----
>>> 1 file changed, 87 insertions(+), 8 deletions(-)
>>>
>>> diff --git a/hw/isa/vt82c686.c b/hw/isa/vt82c686.c
>>> index e8ec63dea9..361b3bed0a 100644
>>> --- a/hw/isa/vt82c686.c
>>> +++ b/hw/isa/vt82c686.c
>>> @@ -27,7 +27,6 @@
>>> #include "hw/timer/i8254.h"
>>> #include "hw/rtc/mc146818rtc.h"
>>> #include "migration/vmstate.h"
>>> -#include "hw/isa/apm.h"
>>> #include "hw/acpi/acpi.h"
>>> #include "hw/i2c/pm_smbus.h"
>>> #include "qapi/error.h"
>>> @@ -42,6 +41,16 @@
>>> #define TYPE_VIA_PM "via-pm"
>>> OBJECT_DECLARE_SIMPLE_TYPE(ViaPMState, VIA_PM)
>>>
>>> +#define VIA_PM_IO_GBLEN 0x2a
>>> +#define VIA_PM_IO_GBLEN_SW_SMI_EN (1 << 6)
>>> +
>>> +#define VIA_PM_IO_GBLCTL 0x2c
>>> +#define VIA_PM_IO_GBLCTL_SMI_EN 1
>>> +#define VIA_PM_IO_GBLCTL_SMIIG (1 << 4)
>>> +#define VIA_PM_IO_GBLCTL_INSMI (1 << 8)
>>> +
>>> +#define VIA_PM_IO_SMI_CMD 0x2f
>>> +
>>> #define VIA_PM_GPE_LEN 4
>>>
>>> #define VIA_PM_SCI_SELECT_OFS 0x42
>>
>> If we'll make a copy of the data sheet in form of #defines could these be in the header to less clutter the source?
>
> Last time I did that I was asked to move the defines back into the 
> source file. I can't find the link right now, otherwise I'd have placed 
> it here.

Yeah, the public header is probably not a good place for these either. 
Maybe add a local vt82c686_regs,h or similar private header in hw/isa next 
to the .c file for these? I'd just say we could use the constant values 
directly as reviewing them will need to look up the data sheet anyway but 
Philippe was in favour of adding defines for constants. This isn't a big 
deal though.

Regards,
BALATON Zoltan

Re: [PATCH v5 0/5] VIA PM: Implement basic ACPI support

[BALATON Zoltan]

[-- Attachment #1: Type: text/plain, Size: 3820 bytes --]

On Sat, 28 Oct 2023, Bernhard Beschow wrote:
> Am 28. Oktober 2023 12:58:32 UTC schrieb BALATON Zoltan <balaton@eik.bme.hu>:
>> Hello,
>>
>> On Sat, 28 Oct 2023, Bernhard Beschow wrote:
>>> This series is part of my work to bring the VIA south bridges to the PC machine
>>> [1]. It implements missing ACPI functionality which ACPI-aware x86 guests
>>> expect for a smooth experience. The implementation is heavily inspired by PIIX4.
>>
>> I think first the interrupt routing should be fixed because that may change a few things in this series so that should be the next step and then rebase this series on top of that.
>>
>> What I mean by fixing interrupt routing? You may remember this discussion:
>>
>> https://patchew.org/QEMU/cover.1677004414.git.balaton@eik.bme.hu/
>>
>> With pegasos2 your (over)simplification worked only because the firmware of that machine maps everythnig to one ISA IRQ and guests were happy with that. I told you that back then but could not convince you and Mark about that. Now with the amigaone machine the firmware maps VIA devices and PCI interuupt pins to different ISA IRQs so we need to go back either to my otiginal implementation or something else you come up with. You can test this trying to use USB devices with amigaone machine which only works after reverting 4e5a20b6da9b1 and 422a6e8075752. So please either propose something to fix that first or wait with this series until I can update my pathches to solve interrupt routing. I think this series should wait until after that because it adds more interrupt handling which should follow whatever way we come up with for that so it's too early fir this series yet. (If you want to try fixing it keep in mind that in both amigaone and pegasos2 the PCI buses are in the north brid
 ge not in the VIA south bridge so don't try to force the IRQ mapping into the PCI bus. All the VIA chip needs to do is mapping its PIRQ/PINT pins to ISA IRQs as the VIA is only handling ISA IRQs and all other pci stuff is handled in the north bridge. So I think we need a via_set_isa_irq function but we could change it according to Mark's idea to pass the PCI device and use its function number to select itq source instead of the enum I had in my original series.)
>>
>> I have some other comments that I'll add in reply to individual patches for the future/
>
> Hi Zoltan,
>
> The interrupt handling introduced in this series is not related to PCI interrupt routing: The SMI is a dedicated pin on the device and the SCI is mapped internally to an ISA interrupt (note how the power management function lacks the registers for PCI interrupt information). Hence, PCI interrupt routing isn't changed in this series and therefore seems off-topic to me.
>
> Moreover, the SMI is a new interrupt which is therefore not used in any machine yet. The SCI is deactivated if set to IRQ 0 which is the default even. If a guest configures it, it shall be aware to receive an *ISA* interrupt.
>
> So I think this series shouldn't conflict with any previous work and should not be blocked by the PCI IRQ routing topic.

The topic I've raised is not about routing PCI interrupts but routing 
different IRQ sources in VIA chip (such as different functions plus the 
PIRQ/PINT pins) to ISA interrupts so that would conflict with how the PM 
func interrupts are routed. I think only the isa function should have 
qemu_irqs and it should handle mapping of the different sources to the 
appropriate ISA IRQ so the different sources (functions) should not have 
their own qemu_irqs or gpios but they would just call via_isa_set_irq with 
their PCIDevice, pun and level and then the isa model would do the 
routing. I plan to do this eventually but it you're adding more things 
that would need to be reverted then it becomes more difficult.

Regards,
BALATON Zoltan

Re: [PATCH v5 0/5] VIA PM: Implement basic ACPI support

[BALATON Zoltan]

On Sat, 28 Oct 2023, BALATON Zoltan wrote:
> On Sat, 28 Oct 2023, Bernhard Beschow wrote:
>> Am 28. Oktober 2023 12:58:32 UTC schrieb BALATON Zoltan 
>> <balaton@eik.bme.hu>:
>>> Hello,
>>> 
>>> On Sat, 28 Oct 2023, Bernhard Beschow wrote:
>>>> This series is part of my work to bring the VIA south bridges to the PC 
>>>> machine
>>>> [1]. It implements missing ACPI functionality which ACPI-aware x86 guests
>>>> expect for a smooth experience. The implementation is heavily inspired by 
>>>> PIIX4.
>>> 
>>> I think first the interrupt routing should be fixed because that may 
>>> change a few things in this series so that should be the next step and 
>>> then rebase this series on top of that.
>>> 
>>> What I mean by fixing interrupt routing? You may remember this discussion:
>>> 
>>> https://patchew.org/QEMU/cover.1677004414.git.balaton@eik.bme.hu/
>>> 
>>> With pegasos2 your (over)simplification worked only because the firmware 
>>> of that machine maps everythnig to one ISA IRQ and guests were happy with 
>>> that. I told you that back then but could not convince you and Mark about 
>>> that. Now with the amigaone machine the firmware maps VIA devices and PCI 
>>> interuupt pins to different ISA IRQs so we need to go back either to my 
>>> otiginal implementation or something else you come up with. You can test 
>>> this trying to use USB devices with amigaone machine which only works 
>>> after reverting 4e5a20b6da9b1 and 422a6e8075752. So please either propose 
>>> something to fix that first or wait with this series until I can update my 
>>> pathches to solve interrupt routing. I think this series should wait until 
>>> after that because it adds more interrupt handling which should follow 
>>> whatever way we come up with for that so it's too early fir this series 
>>> yet. (If you want to try fixing it keep in mind that in both amigaone and 
>>> pegasos2 the PCI buses are in the north brid
> ge not in the VIA south bridge so don't try to force the IRQ mapping into the 
> PCI bus. All the VIA chip needs to do is mapping its PIRQ/PINT pins to ISA 
> IRQs as the VIA is only handling ISA IRQs and all other pci stuff is handled 
> in the north bridge. So I think we need a via_set_isa_irq function but we 
> could change it according to Mark's idea to pass the PCI device and use its 
> function number to select itq source instead of the enum I had in my original 
> series.)
>>> 
>>> I have some other comments that I'll add in reply to individual patches 
>>> for the future/
>> 
>> Hi Zoltan,
>> 
>> The interrupt handling introduced in this series is not related to PCI 
>> interrupt routing: The SMI is a dedicated pin on the device and the SCI is 
>> mapped internally to an ISA interrupt (note how the power management 
>> function lacks the registers for PCI interrupt information). Hence, PCI 
>> interrupt routing isn't changed in this series and therefore seems 
>> off-topic to me.
>> 
>> Moreover, the SMI is a new interrupt which is therefore not used in any 
>> machine yet. The SCI is deactivated if set to IRQ 0 which is the default 
>> even. If a guest configures it, it shall be aware to receive an *ISA* 
>> interrupt.
>> 
>> So I think this series shouldn't conflict with any previous work and should 
>> not be blocked by the PCI IRQ routing topic.
>
> The topic I've raised is not about routing PCI interrupts but routing 
> different IRQ sources in VIA chip (such as different functions plus the 
> PIRQ/PINT pins) to ISA interrupts so that would conflict with how the PM func 
> interrupts are routed. I think only the isa function should have qemu_irqs 
> and it should handle mapping of the different sources to the appropriate ISA 
> IRQ so the different sources (functions) should not have their own qemu_irqs 
> or gpios but they would just call via_isa_set_irq with their PCIDevice, pun 
> and level and then the isa model would do the routing. I plan to do this 
> eventually but it you're adding more things that would need to be reverted 
> then it becomes more difficult.

I've submitted a series with these changes that I think should be the way 
to go. The PM device could then do what I've proposed before and the 
routing of sci to ISA or SMI can be done in the switch in via_isa_set_irq. 
I've left the via-ide device alone for now to avoid conflicting with 
Mark's series but maybe in the future that could also be converted back at 
some point. It's not important at the moment as that's using separate 
interrupts on all machines so should work either way for now so can be 
done any time later.

Regards,
BALATON Zoltan

Re: [PATCH v5 3/5] hw/isa/vt82c686: Reuse acpi_update_sci()

[BALATON Zoltan]

On Sat, 28 Oct 2023, Bernhard Beschow wrote:
> acpi_update_sci() covers everything pm_update_sci() does. It implements common
> ACPI funtionality in a generic fashion. Note that it agnostic to any
> Frankenstein usage of the general purpose event registers in other device
> models. It just implements a generic mechanism which can be wired to arbitrary
> functionality.
>
> Signed-off-by: Bernhard Beschow <shentey@gmail.com>
> ---
> hw/isa/vt82c686.c | 20 ++------------------
> 1 file changed, 2 insertions(+), 18 deletions(-)
>
> diff --git a/hw/isa/vt82c686.c b/hw/isa/vt82c686.c
> index 60ca781e03..7b44ad9485 100644
> --- a/hw/isa/vt82c686.c
> +++ b/hw/isa/vt82c686.c
> @@ -145,26 +145,10 @@ static const MemoryRegionOps pm_io_ops = {
>     },
> };
>
> -static void pm_update_sci(ViaPMState *s)
> -{
> -    int sci_level, pmsts;
> -
> -    pmsts = acpi_pm1_evt_get_sts(&s->ar);
> -    sci_level = (((pmsts & s->ar.pm1.evt.en) &
> -                  (ACPI_BITMASK_RT_CLOCK_ENABLE |
> -                   ACPI_BITMASK_POWER_BUTTON_ENABLE |
> -                   ACPI_BITMASK_GLOBAL_LOCK_ENABLE |
> -                   ACPI_BITMASK_TIMER_ENABLE)) != 0);
> -    qemu_set_irq(s->sci_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));
> -}
> -
> static void pm_tmr_timer(ACPIREGS *ar)
> {
>     ViaPMState *s = container_of(ar, ViaPMState, ar);
> -    pm_update_sci(s);
> +    acpi_update_sci(&s->ar, s->sci_irq);

To avoid needing an interrupt here maybe you could modify 
acpi_update_sci() to allow NULL irq then call via_isa_set_irq here so the 
interrupt routing can be done within the ISA bridge.

Regards,
BALATON Zoltan

> }
>
> static void via_pm_reset(DeviceState *d)
> @@ -182,7 +166,7 @@ static void via_pm_reset(DeviceState *d)
>     acpi_pm1_cnt_reset(&s->ar);
>     acpi_pm_tmr_reset(&s->ar);
>     acpi_gpe_reset(&s->ar);
> -    pm_update_sci(s);
> +    acpi_update_sci(&s->ar, s->sci_irq);
>
>     pm_io_space_update(s);
>     smb_io_space_update(s);
>

Re: [PATCH v5 0/5] VIA PM: Implement basic ACPI support

[Bernhard Beschow]

Am 28. Oktober 2023 17:47:15 UTC schrieb BALATON Zoltan <balaton@eik.bme.hu>:
>On Sat, 28 Oct 2023, Bernhard Beschow wrote:
>> Am 28. Oktober 2023 12:58:32 UTC schrieb BALATON Zoltan <balaton@eik.bme.hu>:
>>> Hello,
>>> 
>>> On Sat, 28 Oct 2023, Bernhard Beschow wrote:
>>>> This series is part of my work to bring the VIA south bridges to the PC machine
>>>> [1]. It implements missing ACPI functionality which ACPI-aware x86 guests
>>>> expect for a smooth experience. The implementation is heavily inspired by PIIX4.
>>> 
>>> I think first the interrupt routing should be fixed because that may change a few things in this series so that should be the next step and then rebase this series on top of that.
>>> 
>>> What I mean by fixing interrupt routing? You may remember this discussion:
>>> 
>>> https://patchew.org/QEMU/cover.1677004414.git.balaton@eik.bme.hu/
>>> 
>>> With pegasos2 your (over)simplification worked only because the firmware of that machine maps everythnig to one ISA IRQ and guests were happy with that. I told you that back then but could not convince you and Mark about that. Now with the amigaone machine the firmware maps VIA devices and PCI interuupt pins to different ISA IRQs so we need to go back either to my otiginal implementation or something else you come up with. You can test this trying to use USB devices with amigaone machine which only works after reverting 4e5a20b6da9b1 and 422a6e8075752. So please either propose something to fix that first or wait with this series until I can update my pathches to solve interrupt routing. I think this series should wait until after that because it adds more interrupt handling which should follow whatever way we come up with for that so it's too early fir this series yet. (If you want to try fixing it keep in mind that in both amigaone and pegasos2 the PCI buses are in the north brid
>ge not in the VIA south bridge so don't try to force the IRQ mapping into the PCI bus. All the VIA chip needs to do is mapping its PIRQ/PINT pins to ISA IRQs as the VIA is only handling ISA IRQs and all other pci stuff is handled in the north bridge. So I think we need a via_set_isa_irq function but we could change it according to Mark's idea to pass the PCI device and use its function number to select itq source instead of the enum I had in my original series.)
>>> 
>>> I have some other comments that I'll add in reply to individual patches for the future/
>> 
>> Hi Zoltan,
>> 
>> The interrupt handling introduced in this series is not related to PCI interrupt routing: The SMI is a dedicated pin on the device and the SCI is mapped internally to an ISA interrupt (note how the power management function lacks the registers for PCI interrupt information). Hence, PCI interrupt routing isn't changed in this series and therefore seems off-topic to me.
>> 
>> Moreover, the SMI is a new interrupt which is therefore not used in any machine yet. The SCI is deactivated if set to IRQ 0 which is the default even. If a guest configures it, it shall be aware to receive an *ISA* interrupt.
>> 
>> So I think this series shouldn't conflict with any previous work and should not be blocked by the PCI IRQ routing topic.
>
>The topic I've raised is not about routing PCI interrupts but routing different IRQ sources in VIA chip (such as different functions plus the PIRQ/PINT pins) to ISA interrupts so that would conflict with how the PM func interrupts are routed. I think only the isa function should have qemu_irqs and it should handle mapping of the different sources to the appropriate ISA IRQ so the different sources (functions) should not have their own qemu_irqs or gpios but they would just call via_isa_set_irq with their PCIDevice, pun and level and then the isa model would do the routing. I plan to do this eventually but it you're adding more things that would need to be reverted then it becomes more difficult.

We've had lenghty discussions about this topic before and we -- together -- ended up with the current solution. This series adds the last missing feature to the VIA south bridges before they can be integrated into the PC machine. Delaying progress by reopening the same topics over and over again really seems unfair to me. Instead, let's be optimistic that we'll end up with a solution that suits all needs well.

That said, I've ran both the pegasos2.rom and the u-boot.bin for amigaone that is used in the Avocado test. I've traced both with '-trace "pci_cfg_*"'. The result is that neither BIOS pokes the SCI routing register in the ISA function which means that the interrupt stays deactivated. Hence, it is very unlikely that the changes introduced in this series would interfer with guests on these machines.

In summary, I don't see any blockers so far for merging this series for the upcoming release.

Best regards,
Bernhard

>
>Regards,
>BALATON Zoltan

Re: [PATCH v5 3/5] hw/isa/vt82c686: Reuse acpi_update_sci()

[Bernhard Beschow]

Am 29. Oktober 2023 00:07:00 UTC schrieb BALATON Zoltan <balaton@eik.bme.hu>:
>On Sat, 28 Oct 2023, Bernhard Beschow wrote:
>> acpi_update_sci() covers everything pm_update_sci() does. It implements common
>> ACPI funtionality in a generic fashion. Note that it agnostic to any
>> Frankenstein usage of the general purpose event registers in other device
>> models. It just implements a generic mechanism which can be wired to arbitrary
>> functionality.
>> 
>> Signed-off-by: Bernhard Beschow <shentey@gmail.com>
>> ---
>> hw/isa/vt82c686.c | 20 ++------------------
>> 1 file changed, 2 insertions(+), 18 deletions(-)
>> 
>> diff --git a/hw/isa/vt82c686.c b/hw/isa/vt82c686.c
>> index 60ca781e03..7b44ad9485 100644
>> --- a/hw/isa/vt82c686.c
>> +++ b/hw/isa/vt82c686.c
>> @@ -145,26 +145,10 @@ static const MemoryRegionOps pm_io_ops = {
>>     },
>> };
>> 
>> -static void pm_update_sci(ViaPMState *s)
>> -{
>> -    int sci_level, pmsts;
>> -
>> -    pmsts = acpi_pm1_evt_get_sts(&s->ar);
>> -    sci_level = (((pmsts & s->ar.pm1.evt.en) &
>> -                  (ACPI_BITMASK_RT_CLOCK_ENABLE |
>> -                   ACPI_BITMASK_POWER_BUTTON_ENABLE |
>> -                   ACPI_BITMASK_GLOBAL_LOCK_ENABLE |
>> -                   ACPI_BITMASK_TIMER_ENABLE)) != 0);
>> -    qemu_set_irq(s->sci_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));
>> -}
>> -
>> static void pm_tmr_timer(ACPIREGS *ar)
>> {
>>     ViaPMState *s = container_of(ar, ViaPMState, ar);
>> -    pm_update_sci(s);
>> +    acpi_update_sci(&s->ar, s->sci_irq);
>
>To avoid needing an interrupt here maybe you could modify acpi_update_sci() to allow NULL irq then call via_isa_set_irq here so the interrupt routing can be done within the ISA bridge.

Why? This function works well as is for other device models.

Best regards,
Bernhard

>
>Regards,
>BALATON Zoltan
>
>> }
>> 
>> static void via_pm_reset(DeviceState *d)
>> @@ -182,7 +166,7 @@ static void via_pm_reset(DeviceState *d)
>>     acpi_pm1_cnt_reset(&s->ar);
>>     acpi_pm_tmr_reset(&s->ar);
>>     acpi_gpe_reset(&s->ar);
>> -    pm_update_sci(s);
>> +    acpi_update_sci(&s->ar, s->sci_irq);
>> 
>>     pm_io_space_update(s);
>>     smb_io_space_update(s);
>> 

Re: [PATCH v5 0/5] VIA PM: Implement basic ACPI support

[BALATON Zoltan]

[-- Attachment #1: Type: text/plain, Size: 6364 bytes --]

On Sun, 29 Oct 2023, Bernhard Beschow wrote:
> Am 28. Oktober 2023 17:47:15 UTC schrieb BALATON Zoltan <balaton@eik.bme.hu>:
>> On Sat, 28 Oct 2023, Bernhard Beschow wrote:
>>> Am 28. Oktober 2023 12:58:32 UTC schrieb BALATON Zoltan <balaton@eik.bme.hu>:
>>>> Hello,
>>>>
>>>> On Sat, 28 Oct 2023, Bernhard Beschow wrote:
>>>>> This series is part of my work to bring the VIA south bridges to the PC machine
>>>>> [1]. It implements missing ACPI functionality which ACPI-aware x86 guests
>>>>> expect for a smooth experience. The implementation is heavily inspired by PIIX4.
>>>>
>>>> I think first the interrupt routing should be fixed because that may change a few things in this series so that should be the next step and then rebase this series on top of that.
>>>>
>>>> What I mean by fixing interrupt routing? You may remember this discussion:
>>>>
>>>> https://patchew.org/QEMU/cover.1677004414.git.balaton@eik.bme.hu/
>>>>
>>>> With pegasos2 your (over)simplification worked only because the firmware of that machine maps everythnig to one ISA IRQ and guests were happy with that. I told you that back then but could not convince you and Mark about that. Now with the amigaone machine the firmware maps VIA devices and PCI interuupt pins to different ISA IRQs so we need to go back either to my otiginal implementation or something else you come up with. You can test this trying to use USB devices with amigaone machine which only works after reverting 4e5a20b6da9b1 and 422a6e8075752. So please either propose something to fix that first or wait with this series until I can update my pathches to solve interrupt routing. I think this series should wait until after that because it adds more interrupt handling which should follow whatever way we come up with for that so it's too early fir this series yet. (If you want to try fixing it keep in mind that in both amigaone and pegasos2 the PCI buses are in the north br
 id
>> ge not in the VIA south bridge so don't try to force the IRQ mapping into the PCI bus. All the VIA chip needs to do is mapping its PIRQ/PINT pins to ISA IRQs as the VIA is only handling ISA IRQs and all other pci stuff is handled in the north bridge. So I think we need a via_set_isa_irq function but we could change it according to Mark's idea to pass the PCI device and use its function number to select itq source instead of the enum I had in my original series.)
>>>>
>>>> I have some other comments that I'll add in reply to individual patches for the future/
>>>
>>> Hi Zoltan,
>>>
>>> The interrupt handling introduced in this series is not related to PCI interrupt routing: The SMI is a dedicated pin on the device and the SCI is mapped internally to an ISA interrupt (note how the power management function lacks the registers for PCI interrupt information). Hence, PCI interrupt routing isn't changed in this series and therefore seems off-topic to me.
>>>
>>> Moreover, the SMI is a new interrupt which is therefore not used in any machine yet. The SCI is deactivated if set to IRQ 0 which is the default even. If a guest configures it, it shall be aware to receive an *ISA* interrupt.
>>>
>>> So I think this series shouldn't conflict with any previous work and should not be blocked by the PCI IRQ routing topic.
>>
>> The topic I've raised is not about routing PCI interrupts but routing different IRQ sources in VIA chip (such as different functions plus the PIRQ/PINT pins) to ISA interrupts so that would conflict with how the PM func interrupts are routed. I think only the isa function should have qemu_irqs and it should handle mapping of the different sources to the appropriate ISA IRQ so the different sources (functions) should not have their own qemu_irqs or gpios but they would just call via_isa_set_irq with their PCIDevice, pun and level and then the isa model would do the routing. I plan to do this eventually but it you're adding more things that would need to be reverted then it becomes more difficult.
>
> We've had lenghty discussions about this topic before and we -- together 
> -- ended up with the current solution.

Which does not work as demonstrated by the amigaone machine now. I've told 
you that before but you did not accept my arguments so for the sake of 
being able to merge pegasos2 in time for release I've accepted your 
alternative that was still OK for pegasos2 and let this be fixed later 
when we need it. That is about now that we have the amigaone machine. The 
amigaone still boots and usable without fixing this IRQ mapping but some 
devices like USB does not work due to not getting the expected interrupt 
correctly. But if you want to change the via device further I'd like to 
fix it before that to avoid having to rewrtite what you add now.

> This series adds the last missing 
> feature to the VIA south bridges before they can be integrated into the 
> PC machine. Delaying progress by reopening the same topics over and over 
> again really seems unfair to me. Instead, let's be optimistic that we'll 
> end up with a solution that suits all needs well.

I'm sorry if it seems unfair to you but I did not bring this up tp delay 
your work but to avoid more work in the future and fix something that is 
broken to improve the device model. If you PC machine wants to map 
internal functions to different IRQs then you will also get this problem 
so it will need to be fixed and fixing it will also simplify your ACPI 
patches. To help with this I've spent some time now to do the fix I think 
would work so you could just rebase your series on top of that.

> That said, I've ran both the pegasos2.rom and the u-boot.bin for 
> amigaone that is used in the Avocado test. I've traced both with '-trace 
> "pci_cfg_*"'. The result is that neither BIOS pokes the SCI routing 
> register in the ISA function which means that the interrupt stays 
> deactivated. Hence, it is very unlikely that the changes introduced in 
> this series would interfer with guests on these machines.

It does not interfere with guests, it inteferes with fixing probelms with 
interrupt routing that amigaone (and liekly your PC machine) has so it 
makes sense to fix that first because it changes the way SMI should be 
added.

> In summary, I don't see any blockers so far for merging this series for 
> the upcoming release.

I hope I could explain above.

Regards,
BALATON Zoltan

Re: [PATCH v5 3/5] hw/isa/vt82c686: Reuse acpi_update_sci()

[BALATON Zoltan]

On Sun, 29 Oct 2023, Bernhard Beschow wrote:
> Am 29. Oktober 2023 00:07:00 UTC schrieb BALATON Zoltan <balaton@eik.bme.hu>:
>> On Sat, 28 Oct 2023, Bernhard Beschow wrote:
>>> acpi_update_sci() covers everything pm_update_sci() does. It implements common
>>> ACPI funtionality in a generic fashion. Note that it agnostic to any
>>> Frankenstein usage of the general purpose event registers in other device
>>> models. It just implements a generic mechanism which can be wired to arbitrary
>>> functionality.
>>>
>>> Signed-off-by: Bernhard Beschow <shentey@gmail.com>
>>> ---
>>> hw/isa/vt82c686.c | 20 ++------------------
>>> 1 file changed, 2 insertions(+), 18 deletions(-)
>>>
>>> diff --git a/hw/isa/vt82c686.c b/hw/isa/vt82c686.c
>>> index 60ca781e03..7b44ad9485 100644
>>> --- a/hw/isa/vt82c686.c
>>> +++ b/hw/isa/vt82c686.c
>>> @@ -145,26 +145,10 @@ static const MemoryRegionOps pm_io_ops = {
>>>     },
>>> };
>>>
>>> -static void pm_update_sci(ViaPMState *s)
>>> -{
>>> -    int sci_level, pmsts;
>>> -
>>> -    pmsts = acpi_pm1_evt_get_sts(&s->ar);
>>> -    sci_level = (((pmsts & s->ar.pm1.evt.en) &
>>> -                  (ACPI_BITMASK_RT_CLOCK_ENABLE |
>>> -                   ACPI_BITMASK_POWER_BUTTON_ENABLE |
>>> -                   ACPI_BITMASK_GLOBAL_LOCK_ENABLE |
>>> -                   ACPI_BITMASK_TIMER_ENABLE)) != 0);
>>> -    qemu_set_irq(s->sci_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));
>>> -}
>>> -
>>> static void pm_tmr_timer(ACPIREGS *ar)
>>> {
>>>     ViaPMState *s = container_of(ar, ViaPMState, ar);
>>> -    pm_update_sci(s);
>>> +    acpi_update_sci(&s->ar, s->sci_irq);
>>
>> To avoid needing an interrupt here maybe you could modify acpi_update_sci() to allow NULL irq then call via_isa_set_irq here so the interrupt routing can be done within the ISA bridge.
>
> Why? This function works well as is for other device models.

To avoid adding another qemu_irq which is not needed otherwise to keep the 
via model which is already complex relatively simple.

Regards,
ALATON Zoltan

>>> }
>>>
>>> static void via_pm_reset(DeviceState *d)
>>> @@ -182,7 +166,7 @@ static void via_pm_reset(DeviceState *d)
>>>     acpi_pm1_cnt_reset(&s->ar);
>>>     acpi_pm_tmr_reset(&s->ar);
>>>     acpi_gpe_reset(&s->ar);
>>> -    pm_update_sci(s);
>>> +    acpi_update_sci(&s->ar, s->sci_irq);
>>>
>>>     pm_io_space_update(s);
>>>     smb_io_space_update(s);
>>>
>
>

Re: [PATCH v5 3/5] hw/isa/vt82c686: Reuse acpi_update_sci()

[Bernhard Beschow]

Am 29. Oktober 2023 00:07:00 UTC schrieb BALATON Zoltan <balaton@eik.bme.hu>:
>On Sat, 28 Oct 2023, Bernhard Beschow wrote:
>> acpi_update_sci() covers everything pm_update_sci() does. It implements common
>> ACPI funtionality in a generic fashion. Note that it agnostic to any
>> Frankenstein usage of the general purpose event registers in other device
>> models. It just implements a generic mechanism which can be wired to arbitrary
>> functionality.
>> 
>> Signed-off-by: Bernhard Beschow <shentey@gmail.com>
>> ---
>> hw/isa/vt82c686.c | 20 ++------------------
>> 1 file changed, 2 insertions(+), 18 deletions(-)
>> 
>> diff --git a/hw/isa/vt82c686.c b/hw/isa/vt82c686.c
>> index 60ca781e03..7b44ad9485 100644
>> --- a/hw/isa/vt82c686.c
>> +++ b/hw/isa/vt82c686.c
>> @@ -145,26 +145,10 @@ static const MemoryRegionOps pm_io_ops = {
>>     },
>> };
>> 
>> -static void pm_update_sci(ViaPMState *s)
>> -{
>> -    int sci_level, pmsts;
>> -
>> -    pmsts = acpi_pm1_evt_get_sts(&s->ar);
>> -    sci_level = (((pmsts & s->ar.pm1.evt.en) &
>> -                  (ACPI_BITMASK_RT_CLOCK_ENABLE |
>> -                   ACPI_BITMASK_POWER_BUTTON_ENABLE |
>> -                   ACPI_BITMASK_GLOBAL_LOCK_ENABLE |
>> -                   ACPI_BITMASK_TIMER_ENABLE)) != 0);
>> -    qemu_set_irq(s->sci_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));
>> -}
>> -
>> static void pm_tmr_timer(ACPIREGS *ar)
>> {
>>     ViaPMState *s = container_of(ar, ViaPMState, ar);
>> -    pm_update_sci(s);
>> +    acpi_update_sci(&s->ar, s->sci_irq);
>
>To avoid needing an interrupt here maybe you could modify acpi_update_sci() to allow NULL irq then call via_isa_set_irq here so the interrupt routing can be done within the ISA bridge.

Given the interesting behavior of the amigaone boot loader I'd respin this series with the last two patches only.

Best regards,
Bernhard

>
>Regards,
>BALATON Zoltan
>
>> }
>> 
>> static void via_pm_reset(DeviceState *d)
>> @@ -182,7 +166,7 @@ static void via_pm_reset(DeviceState *d)
>>     acpi_pm1_cnt_reset(&s->ar);
>>     acpi_pm_tmr_reset(&s->ar);
>>     acpi_gpe_reset(&s->ar);
>> -    pm_update_sci(s);
>> +    acpi_update_sci(&s->ar, s->sci_irq);
>> 
>>     pm_io_space_update(s);
>>     smb_io_space_update(s);
>> 

Re: [PATCH v5 3/5] hw/isa/vt82c686: Reuse acpi_update_sci()

[BALATON Zoltan]

On Mon, 30 Oct 2023, Bernhard Beschow wrote:
> Am 29. Oktober 2023 00:07:00 UTC schrieb BALATON Zoltan <balaton@eik.bme.hu>:
>> On Sat, 28 Oct 2023, Bernhard Beschow wrote:
>>> acpi_update_sci() covers everything pm_update_sci() does. It implements common
>>> ACPI funtionality in a generic fashion. Note that it agnostic to any
>>> Frankenstein usage of the general purpose event registers in other device
>>> models. It just implements a generic mechanism which can be wired to arbitrary
>>> functionality.
>>>
>>> Signed-off-by: Bernhard Beschow <shentey@gmail.com>
>>> ---
>>> hw/isa/vt82c686.c | 20 ++------------------
>>> 1 file changed, 2 insertions(+), 18 deletions(-)
>>>
>>> diff --git a/hw/isa/vt82c686.c b/hw/isa/vt82c686.c
>>> index 60ca781e03..7b44ad9485 100644
>>> --- a/hw/isa/vt82c686.c
>>> +++ b/hw/isa/vt82c686.c
>>> @@ -145,26 +145,10 @@ static const MemoryRegionOps pm_io_ops = {
>>>     },
>>> };
>>>
>>> -static void pm_update_sci(ViaPMState *s)
>>> -{
>>> -    int sci_level, pmsts;
>>> -
>>> -    pmsts = acpi_pm1_evt_get_sts(&s->ar);
>>> -    sci_level = (((pmsts & s->ar.pm1.evt.en) &
>>> -                  (ACPI_BITMASK_RT_CLOCK_ENABLE |
>>> -                   ACPI_BITMASK_POWER_BUTTON_ENABLE |
>>> -                   ACPI_BITMASK_GLOBAL_LOCK_ENABLE |
>>> -                   ACPI_BITMASK_TIMER_ENABLE)) != 0);
>>> -    qemu_set_irq(s->sci_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));
>>> -}
>>> -
>>> static void pm_tmr_timer(ACPIREGS *ar)
>>> {
>>>     ViaPMState *s = container_of(ar, ViaPMState, ar);
>>> -    pm_update_sci(s);
>>> +    acpi_update_sci(&s->ar, s->sci_irq);
>>
>> To avoid needing an interrupt here maybe you could modify acpi_update_sci() to allow NULL irq then call via_isa_set_irq here so the interrupt routing can be done within the ISA bridge.
>
> Given the interesting behavior of the amigaone boot loader I'd respin this series with the last two patches only.

I guess you could do that just modeling the register and leave out SMI for 
now until we resolve the IRQ routing so then it should not conflict. I 
think you said that nothing uses the interrupt and you just need to be 
able to trigger poweroff with register write?

If you add SMI I think the qemu_irq for that should be in VIAISAState 
where the other irqs are and PM func should call via_isa_set_irq where it 
decides to route the SCI to ISA or SMI based on reg values. You can do 
this in the swich under case PCI_FUNC(of PM device) but if you don't need 
the IRQ this can be added later in a follow up I think.

Regards,
BALATON Zoltan