[RFC PATCH 0/5] hw/i386/q35: Decouple virtual SMI# lines and wire them to ICH9 chipset

[RFC PATCH 0/5] hw/i386/q35: Decouple virtual SMI# lines and wire them to ICH9 chipset

[Philippe Mathieu-Daudé]

Hi,

This is an experimental series to reduce calls to the
cpu_interrupt() API from generic HW/. I'm trying to use
the ICH9 chipset from a non-x86 machine. Without this
experiment, we can not because cpu_interrupt() is target
specific. Here the interrupt is decoupled using the QDev
GPIO API. Even if the SMI# line is left unconnected, the
device is still usable by a guest.

Based-on: <20240226111416.39217-1-philmd@linaro.org>

Philippe Mathieu-Daudé (5):
  target/i386/cpu: Expose SMI# IRQ line via QDev
  hw/i386/piix: Set CPU SMI# interrupt using QDev GPIO API
  hw/ahci/ich9_tco: Set CPU SMI# interrupt using QDev GPIO API
  hw/i386/q35: Wire virtual SMI# lines to ICH9 chipset
  hw/isa: Build ich9_lpc.c once

 include/hw/acpi/ich9.h        |  1 +
 include/hw/acpi/ich9_tco.h    |  4 ++--
 include/hw/i386/pc.h          |  2 --
 include/hw/isa/ich9_lpc.h     | 12 ++++++++++++
 include/hw/southbridge/ich9.h |  1 +
 target/i386/cpu-internal.h    |  1 +
 hw/acpi/ich9.c                |  3 ++-
 hw/acpi/ich9_tco.c            | 13 ++++++++++---
 hw/i386/pc.c                  |  9 ---------
 hw/i386/pc_piix.c             |  4 ++--
 hw/i386/pc_q35.c              | 26 ++++++++++++++++++++++++++
 hw/isa/ich9_lpc.c             | 15 ++++-----------
 hw/southbridge/ich9.c         |  1 +
 target/i386/cpu-sysemu.c      | 11 +++++++++++
 target/i386/cpu.c             |  2 ++
 hw/isa/meson.build            |  3 +--
 16 files changed, 76 insertions(+), 32 deletions(-)

-- 
2.41.0

[PATCH 1/5] target/i386/cpu: Expose SMI# IRQ line via QDev

[Philippe Mathieu-Daudé]

In order to remove calls to cpu_interrupt() from hw/ code,
expose the SMI# interrupt via QDev as named GPIO.

Signed-off-by: Philippe Mathieu-Daudé <philmd@linaro.org>
---
 target/i386/cpu-internal.h |  1 +
 target/i386/cpu-sysemu.c   | 11 +++++++++++
 target/i386/cpu.c          |  2 ++
 3 files changed, 14 insertions(+)

diff --git a/target/i386/cpu-internal.h b/target/i386/cpu-internal.h
index 9baac5c0b4..9d76bb77cf 100644
--- a/target/i386/cpu-internal.h
+++ b/target/i386/cpu-internal.h
@@ -62,6 +62,7 @@ GuestPanicInformation *x86_cpu_get_crash_info(CPUState *cs);
 void x86_cpu_get_crash_info_qom(Object *obj, Visitor *v,
                                 const char *name, void *opaque, Error **errp);
 
+void x86_cpu_smi_irq(void *opaque, int irq, int level);
 void x86_cpu_apic_create(X86CPU *cpu, Error **errp);
 void x86_cpu_apic_realize(X86CPU *cpu, Error **errp);
 void x86_cpu_machine_reset_cb(void *opaque);
diff --git a/target/i386/cpu-sysemu.c b/target/i386/cpu-sysemu.c
index 7422096737..7684a7f01e 100644
--- a/target/i386/cpu-sysemu.c
+++ b/target/i386/cpu-sysemu.c
@@ -370,3 +370,14 @@ void x86_cpu_get_crash_info_qom(Object *obj, Visitor *v,
     qapi_free_GuestPanicInformation(panic_info);
 }
 
+void x86_cpu_smi_irq(void *opaque, int irq, int level)
+{
+    DeviceState *dev = opaque;
+    CPUState *cs = CPU(dev);
+
+    if (level) {
+        cpu_interrupt(cs, CPU_INTERRUPT_SMI);
+    } else {
+        cpu_reset_interrupt(cs, CPU_INTERRUPT_SMI);
+    }
+}
diff --git a/target/i386/cpu.c b/target/i386/cpu.c
index 7f90823676..6b4462d533 100644
--- a/target/i386/cpu.c
+++ b/target/i386/cpu.c
@@ -7463,6 +7463,8 @@ static void x86_cpu_realizefn(DeviceState *dev, Error **errp)
     }
 
 #ifndef CONFIG_USER_ONLY
+    qdev_init_gpio_in_named(dev, x86_cpu_smi_irq, "SMI", 1);
+
     x86_cpu_apic_realize(cpu, &local_err);
     if (local_err != NULL) {
         goto out;
-- 
2.41.0

[PATCH 2/5] hw/i386/piix: Set CPU SMI# interrupt using QDev GPIO API

[Philippe Mathieu-Daudé]

Use the CPU "SMI" IRQ, removing a call to cpu_interrupt()
from hw/.

Signed-off-by: Philippe Mathieu-Daudé <philmd@linaro.org>
---
 include/hw/i386/pc.h | 2 --
 hw/i386/pc.c         | 9 ---------
 hw/i386/pc_piix.c    | 4 ++--
 3 files changed, 2 insertions(+), 13 deletions(-)

diff --git a/include/hw/i386/pc.h b/include/hw/i386/pc.h
index 4bb1899602..c8227e18c3 100644
--- a/include/hw/i386/pc.h
+++ b/include/hw/i386/pc.h
@@ -148,8 +148,6 @@ GSIState *pc_gsi_create(qemu_irq **irqs, bool pci_enabled);
 /* pc.c */
 extern int fd_bootchk;
 
-void pc_acpi_smi_interrupt(void *opaque, int irq, int level);
-
 #define PCI_HOST_PROP_RAM_MEM          "ram-mem"
 #define PCI_HOST_PROP_PCI_MEM          "pci-mem"
 #define PCI_HOST_PROP_SYSTEM_MEM       "system-mem"
diff --git a/hw/i386/pc.c b/hw/i386/pc.c
index 880e95de26..1df9f5ba90 100644
--- a/hw/i386/pc.c
+++ b/hw/i386/pc.c
@@ -651,15 +651,6 @@ static gboolean pc_init_ne2k_isa(ISABus *bus, NICInfo *nd, Error **errp)
     return true;
 }
 
-void pc_acpi_smi_interrupt(void *opaque, int irq, int level)
-{
-    X86CPU *cpu = opaque;
-
-    if (level) {
-        cpu_interrupt(CPU(cpu), CPU_INTERRUPT_SMI);
-    }
-}
-
 static
 void pc_machine_done(Notifier *notifier, void *data)
 {
diff --git a/hw/i386/pc_piix.c b/hw/i386/pc_piix.c
index ce6aad758d..447130ade1 100644
--- a/hw/i386/pc_piix.c
+++ b/hw/i386/pc_piix.c
@@ -345,9 +345,9 @@ static void pc_init1(MachineState *machine,
     pc_cmos_init(pcms, x86ms->rtc);
 
     if (piix4_pm) {
-        smi_irq = qemu_allocate_irq(pc_acpi_smi_interrupt, first_cpu, 0);
-
+        smi_irq = qdev_get_gpio_in_named(DEVICE(first_cpu), "SMI", 0);
         qdev_connect_gpio_out_named(DEVICE(piix4_pm), "smi-irq", 0, smi_irq);
+
         pcms->smbus = I2C_BUS(qdev_get_child_bus(DEVICE(piix4_pm), "i2c"));
         /* TODO: Populate SPD eeprom data.  */
         smbus_eeprom_init(pcms->smbus, 8, NULL, 0);
-- 
2.41.0

[RFC PATCH 3/5] hw/ahci/ich9_tco: Set CPU SMI# interrupt using QDev GPIO API

[Philippe Mathieu-Daudé]

Use the CPU "SMI" IRQ, removing a call to cpu_interrupt()
from hw/. Keep a reference to the IRQ in the TCOIORegs
structure, which while being names with the Regs suffix
doesn't contain only registers. Remove ich9_generate_smi().

Signed-off-by: Philippe Mathieu-Daudé <philmd@linaro.org>
---
I suppose ideally ICH9 TCO would be a QOM object...
---
 include/hw/acpi/ich9.h     |  1 +
 include/hw/acpi/ich9_tco.h |  4 ++--
 hw/acpi/ich9.c             |  3 ++-
 hw/acpi/ich9_tco.c         | 13 ++++++++++---
 hw/isa/ich9_lpc.c          |  5 -----
 5 files changed, 15 insertions(+), 11 deletions(-)

diff --git a/include/hw/acpi/ich9.h b/include/hw/acpi/ich9.h
index 3587a35c9f..84e1557257 100644
--- a/include/hw/acpi/ich9.h
+++ b/include/hw/acpi/ich9.h
@@ -49,6 +49,7 @@ typedef struct ICH9LPCPMRegs {
     uint32_t smi_sts;
 
     qemu_irq irq;      /* SCI */
+    qemu_irq smi;      /* SMI */
 
     uint32_t pm_io_base;
     Notifier powerdown_notifier;
diff --git a/include/hw/acpi/ich9_tco.h b/include/hw/acpi/ich9_tco.h
index 68ee64942f..31730b8e14 100644
--- a/include/hw/acpi/ich9_tco.h
+++ b/include/hw/acpi/ich9_tco.h
@@ -73,10 +73,10 @@ typedef struct TCOIORegs {
     uint8_t timeouts_no;
 
     MemoryRegion io;
+    qemu_irq smi;
 } TCOIORegs;
 
-void ich9_acpi_pm_tco_init(TCOIORegs *tr, MemoryRegion *parent);
-void ich9_generate_smi(void);
+void ich9_acpi_pm_tco_init(TCOIORegs *tr, MemoryRegion *parent, qemu_irq smi);
 
 extern const VMStateDescription vmstate_ich9_sm_tco;
 
diff --git a/hw/acpi/ich9.c b/hw/acpi/ich9.c
index 1f41ab49c4..e0b3838365 100644
--- a/hw/acpi/ich9.c
+++ b/hw/acpi/ich9.c
@@ -318,7 +318,8 @@ void ich9_pm_init(PCIDevice *lpc_pci, ICH9LPCPMRegs *pm, qemu_irq sci_irq)
     memory_region_add_subregion(&pm->io, ICH9_PMIO_SMI_EN, &pm->io_smi);
 
     if (pm->enable_tco) {
-        ich9_acpi_pm_tco_init(&pm->tco_regs, &pm->io);
+        pm->smi = qdev_get_gpio_in_named(DEVICE(first_cpu), "SMI", 0);
+        ich9_acpi_pm_tco_init(&pm->tco_regs, &pm->io, pm->smi);
     }
 
     if (pm->acpi_pci_hotplug.use_acpi_hotplug_bridge) {
diff --git a/hw/acpi/ich9_tco.c b/hw/acpi/ich9_tco.c
index 7499ec17db..1061b18b7e 100644
--- a/hw/acpi/ich9_tco.c
+++ b/hw/acpi/ich9_tco.c
@@ -14,6 +14,7 @@
 
 #include "hw/acpi/ich9_tco.h"
 #include "hw/isa/ich9_lpc.h"
+#include "hw/irq.h"
 #include "trace.h"
 
 enum {
@@ -31,6 +32,11 @@ enum {
     SW_IRQ_GEN_DEFAULT      = 0x03,
 };
 
+static void ich9_generate_smi(TCOIORegs *tr)
+{
+    qemu_irq_raise(tr->smi);
+}
+
 static inline void tco_timer_reload(TCOIORegs *tr)
 {
     int ticks = tr->tco.tmr & TCO_TMR_MASK;
@@ -72,7 +78,7 @@ static void tco_timer_expired(void *opaque)
     }
 
     if (pm->smi_en & ICH9_PMIO_SMI_EN_TCO_EN) {
-        ich9_generate_smi();
+        ich9_generate_smi(tr);
     }
     tr->tco.rld = tr->tco.tmr;
     tco_timer_reload(tr);
@@ -154,7 +160,7 @@ static void tco_ioport_writew(TCOIORegs *tr, uint32_t addr, uint32_t val)
     case TCO_DAT_IN:
         tr->tco.din = val;
         tr->tco.sts1 |= SW_TCO_SMI;
-        ich9_generate_smi();
+        ich9_generate_smi(tr);
         break;
     case TCO_DAT_OUT:
         tr->tco.dout = val;
@@ -225,7 +231,7 @@ static const MemoryRegionOps tco_io_ops = {
     .endianness = DEVICE_LITTLE_ENDIAN,
 };
 
-void ich9_acpi_pm_tco_init(TCOIORegs *tr, MemoryRegion *parent)
+void ich9_acpi_pm_tco_init(TCOIORegs *tr, MemoryRegion *parent, qemu_irq smi)
 {
     *tr = (TCOIORegs) {
         .tco = {
@@ -245,6 +251,7 @@ void ich9_acpi_pm_tco_init(TCOIORegs *tr, MemoryRegion *parent)
         .tco_timer     = timer_new_ns(QEMU_CLOCK_VIRTUAL, tco_timer_expired, tr),
         .expire_time   = -1,
         .timeouts_no   = 0,
+        .smi = smi,
     };
     memory_region_init_io(&tr->io, memory_region_owner(parent),
                           &tco_io_ops, tr, "sm-tco", ICH9_PMIO_TCO_LEN);
diff --git a/hw/isa/ich9_lpc.c b/hw/isa/ich9_lpc.c
index 2339f66e0f..b1f41158c5 100644
--- a/hw/isa/ich9_lpc.c
+++ b/hw/isa/ich9_lpc.c
@@ -353,11 +353,6 @@ static PCIINTxRoute ich9_route_intx_pin_to_irq(void *opaque, int pirq_pin)
     return route;
 }
 
-void ich9_generate_smi(void)
-{
-    cpu_interrupt(first_cpu, CPU_INTERRUPT_SMI);
-}
-
 /* Returns -1 on error, IRQ number on success */
 static int ich9_lpc_sci_irq(ICH9LPCState *lpc)
 {
-- 
2.41.0

[RFC PATCH 4/5] hw/i386/q35: Wire virtual SMI# lines to ICH9 chipset

[Philippe Mathieu-Daudé]

We use virtual SMI lines for the virtualized q35 machine
(see commit 5ce45c7a2b "hw/isa/lpc_ich9: add broadcast
SMI feature").

Expose them as QDev GPIO at the machine level. Wire them
to the ICH9 chipset. This allows removing a pair of calls
to cpu_interrupt() from the ICH9 model and make it target
agnostic.

Signed-off-by: Philippe Mathieu-Daudé <philmd@linaro.org>
---
 include/hw/isa/ich9_lpc.h     | 12 ++++++++++++
 include/hw/southbridge/ich9.h |  1 +
 hw/i386/pc_q35.c              | 26 ++++++++++++++++++++++++++
 hw/isa/ich9_lpc.c             | 10 ++++------
 hw/southbridge/ich9.c         |  1 +
 5 files changed, 44 insertions(+), 6 deletions(-)

diff --git a/include/hw/isa/ich9_lpc.h b/include/hw/isa/ich9_lpc.h
index b64d88b395..f11ae7e762 100644
--- a/include/hw/isa/ich9_lpc.h
+++ b/include/hw/isa/ich9_lpc.h
@@ -21,6 +21,17 @@ OBJECT_DECLARE_SIMPLE_TYPE(ICH9LPCState, ICH9_LPC_DEVICE)
 
 #define ICH9_CC_SIZE (16 * 1024) /* 16KB. Chipset configuration registers */
 
+/*
+ * Real ICH9 contains a single SMI output line and doesn't broadcast CPUs.
+ * Virtualized ICH9 allows broadcasting upon negatiation with guest, see
+ * commit 5ce45c7a2b.
+ */
+enum {
+    ICH9_VIRT_SMI_BROADCAST,
+    ICH9_VIRT_SMI_CURRENT,
+#define ICH9_VIRT_SMI_COUNT 2
+};
+
 struct ICH9LPCState {
     /* ICH9 LPC PCI to ISA bridge */
     PCIDevice d;
@@ -71,6 +82,7 @@ struct ICH9LPCState {
     Notifier machine_ready;
 
     qemu_irq gsi[IOAPIC_NUM_PINS];
+    qemu_irq virt_smi[ICH9_VIRT_SMI_COUNT];
 };
 
 #define ICH9_MASK(bit, ms_bit, ls_bit) \
diff --git a/include/hw/southbridge/ich9.h b/include/hw/southbridge/ich9.h
index a8da4a8665..48a4212ed8 100644
--- a/include/hw/southbridge/ich9.h
+++ b/include/hw/southbridge/ich9.h
@@ -17,6 +17,7 @@ OBJECT_DECLARE_SIMPLE_TYPE(ICH9State, ICH9_SOUTHBRIDGE)
 #define ICH9_PCIE_FUNC_MAX                      6
 
 #define ICH9_GPIO_GSI "gsi"
+#define ICH9_VIRT_SMI "x-virt-smi"
 
 #define ICH9_LPC_SMI_NEGOTIATED_FEAT_PROP "x-smi-negotiated-features"
 
diff --git a/hw/i386/pc_q35.c b/hw/i386/pc_q35.c
index 31ab0ae77b..77fe8932e8 100644
--- a/hw/i386/pc_q35.c
+++ b/hw/i386/pc_q35.c
@@ -50,6 +50,7 @@
 #include "hw/ide/ahci-pci.h"
 #include "hw/intc/ioapic.h"
 #include "hw/southbridge/ich9.h"
+#include "hw/isa/ich9_lpc.h"
 #include "qapi/error.h"
 #include "qemu/error-report.h"
 #include "sysemu/numa.h"
@@ -58,6 +59,25 @@
 #include "hw/i386/acpi-build.h"
 #include "target/i386/cpu.h"
 
+/*
+ * Kludge IRQ handler for ICH9 virtual SMI delivery.
+ * IRQ#0: broadcast
+ * IRQ#1: deliver to current CPU
+ */
+static void pc_q35_ich9_virt_smi(void *opaque, int irq, int level)
+{
+    assert(level);
+    if (irq) {
+        cpu_interrupt(current_cpu, CPU_INTERRUPT_SMI);
+    } else {
+        CPUState *cs;
+
+        CPU_FOREACH(cs) {
+            cpu_interrupt(cs, CPU_INTERRUPT_SMI);
+        }
+    }
+}
+
 /* PC hardware initialisation */
 static void pc_q35_init(MachineState *machine)
 {
@@ -65,6 +85,7 @@ static void pc_q35_init(MachineState *machine)
     PCMachineClass *pcmc = PC_MACHINE_GET_CLASS(pcms);
     X86MachineState *x86ms = X86_MACHINE(machine);
     Object *phb;
+    qemu_irq *smi_irq;
     DeviceState *ich9;
     Object *lpc_obj;
     MemoryRegion *system_memory = get_system_memory();
@@ -160,6 +181,8 @@ static void pc_q35_init(MachineState *machine)
 
     /* irq lines */
     gsi_state = pc_gsi_create(&x86ms->gsi, true);
+    smi_irq = qemu_allocate_irqs(pc_q35_ich9_virt_smi, NULL,
+                                 ICH9_VIRT_SMI_COUNT);
 
     ich9 = qdev_new(TYPE_ICH9_SOUTHBRIDGE);
     object_property_add_child(OBJECT(machine), "ich9", OBJECT(ich9));
@@ -168,6 +191,9 @@ static void pc_q35_init(MachineState *machine)
     for (i = 0; i < IOAPIC_NUM_PINS; i++) {
         qdev_connect_gpio_out_named(ich9, ICH9_GPIO_GSI, i, x86ms->gsi[i]);
     }
+    for (i = 0; i < ICH9_VIRT_SMI_COUNT; i++) {
+        qdev_connect_gpio_out_named(ich9, ICH9_VIRT_SMI, i, smi_irq[i]);
+    }
     qdev_prop_set_bit(ich9, "d2p-enabled", false);
     qdev_prop_set_bit(ich9, "smm-enabled", x86_machine_is_smm_enabled(x86ms));
     qdev_prop_set_bit(ich9, "sata-enabled", pcms->sata_enabled);
diff --git a/hw/isa/ich9_lpc.c b/hw/isa/ich9_lpc.c
index b1f41158c5..599cb0ee86 100644
--- a/hw/isa/ich9_lpc.c
+++ b/hw/isa/ich9_lpc.c
@@ -30,7 +30,6 @@
 
 #include "qemu/osdep.h"
 #include "qemu/log.h"
-#include "cpu.h"
 #include "qapi/error.h"
 #include "qapi/visitor.h"
 #include "qemu/range.h"
@@ -495,12 +494,9 @@ static void ich9_apm_ctrl_changed(uint32_t val, void *arg)
     if (lpc->pm.smi_en & ICH9_PMIO_SMI_EN_APMC_EN) {
         if (lpc->smi_negotiated_features &
             (UINT64_C(1) << ICH9_LPC_SMI_F_BROADCAST_BIT)) {
-            CPUState *cs;
-            CPU_FOREACH(cs) {
-                cpu_interrupt(cs, CPU_INTERRUPT_SMI);
-            }
+            qemu_irq_raise(lpc->virt_smi[ICH9_VIRT_SMI_BROADCAST]);
         } else {
-            cpu_interrupt(current_cpu, CPU_INTERRUPT_SMI);
+            qemu_irq_raise(lpc->virt_smi[ICH9_VIRT_SMI_CURRENT]);
         }
     }
 }
@@ -700,6 +696,8 @@ static void ich9_lpc_initfn(Object *obj)
 
     qdev_init_gpio_out_named(DEVICE(lpc), lpc->gsi, ICH9_GPIO_GSI,
                              IOAPIC_NUM_PINS);
+    qdev_init_gpio_out_named(DEVICE(lpc), lpc->virt_smi,
+                             ICH9_VIRT_SMI, ARRAY_SIZE(lpc->virt_smi));
 
     object_property_add_uint8_ptr(obj, ACPI_PM_PROP_SCI_INT,
                                   &lpc->sci_gsi, OBJ_PROP_FLAG_READ);
diff --git a/hw/southbridge/ich9.c b/hw/southbridge/ich9.c
index 521925b462..d5e131cff3 100644
--- a/hw/southbridge/ich9.c
+++ b/hw/southbridge/ich9.c
@@ -64,6 +64,7 @@ static void ich9_init(Object *obj)
 
     object_initialize_child(obj, "lpc", &s->lpc, TYPE_ICH9_LPC_DEVICE);
     qdev_pass_gpios(DEVICE(&s->lpc), DEVICE(s), ICH9_GPIO_GSI);
+    qdev_pass_gpios(DEVICE(&s->lpc), DEVICE(s), ICH9_VIRT_SMI);
     qdev_prop_set_int32(DEVICE(&s->lpc), "addr", ICH9_LPC_DEVFN);
     qdev_prop_set_bit(DEVICE(&s->lpc), "multifunction", true);
     object_property_add_alias(obj, "smm-enabled",
-- 
2.41.0

Re: [RFC PATCH 4/5] hw/i386/q35: Wire virtual SMI# lines to ICH9 chipset

[Zhao Liu]

Hi Philippe,

> +/*
> + * Real ICH9 contains a single SMI output line and doesn't broadcast CPUs.
> + * Virtualized ICH9 allows broadcasting upon negatiation with guest, see
> + * commit 5ce45c7a2b.
> + */
> +enum {
> +    ICH9_VIRT_SMI_BROADCAST,
> +    ICH9_VIRT_SMI_CURRENT,
> +#define ICH9_VIRT_SMI_COUNT 2
> +};
> +

Just quick look here. Shouldn't ICH9_VIRT_SMI_COUNT be defined outside of
enum {}?

-Zhao

Re: [RFC PATCH 4/5] hw/i386/q35: Wire virtual SMI# lines to ICH9 chipset

[Thomas Huth]

On 28/02/2024 17.43, Zhao Liu wrote:
> Hi Philippe,
> 
>> +/*
>> + * Real ICH9 contains a single SMI output line and doesn't broadcast CPUs.
>> + * Virtualized ICH9 allows broadcasting upon negatiation with guest, see
>> + * commit 5ce45c7a2b.
>> + */
>> +enum {
>> +    ICH9_VIRT_SMI_BROADCAST,
>> +    ICH9_VIRT_SMI_CURRENT,
>> +#define ICH9_VIRT_SMI_COUNT 2
>> +};
>> +
> 
> Just quick look here. Shouldn't ICH9_VIRT_SMI_COUNT be defined outside of
> enum {}?

Or even better, do it without a #define:

enum {
     ICH9_VIRT_SMI_BROADCAST,
     ICH9_VIRT_SMI_CURRENT,
     ICH9_VIRT_SMI_COUNT
};

  Thomas

Re: [RFC PATCH 4/5] hw/i386/q35: Wire virtual SMI# lines to ICH9 chipset

[Philippe Mathieu-Daudé]

On 7/3/24 20:43, Thomas Huth wrote:
> On 28/02/2024 17.43, Zhao Liu wrote:
>> Hi Philippe,
>>
>>> +/*
>>> + * Real ICH9 contains a single SMI output line and doesn't broadcast 
>>> CPUs.
>>> + * Virtualized ICH9 allows broadcasting upon negatiation with guest, 
>>> see
>>> + * commit 5ce45c7a2b.
>>> + */
>>> +enum {
>>> +    ICH9_VIRT_SMI_BROADCAST,
>>> +    ICH9_VIRT_SMI_CURRENT,
>>> +#define ICH9_VIRT_SMI_COUNT 2
>>> +};
>>> +
>>
>> Just quick look here. Shouldn't ICH9_VIRT_SMI_COUNT be defined outside of
>> enum {}?
> 
> Or even better, do it without a #define:
> 
> enum {
>      ICH9_VIRT_SMI_BROADCAST,
>      ICH9_VIRT_SMI_CURRENT,
>      ICH9_VIRT_SMI_COUNT

This form isn't recommended as it confuses static analyzers,
considering ICH9_VIRT_SMI_COUNT as part of the enum.

> };
> 
>   Thomas
> 

Re: [RFC PATCH 4/5] hw/i386/q35: Wire virtual SMI# lines to ICH9 chipset

[Laszlo Ersek]

On 3/8/24 09:08, Philippe Mathieu-Daudé wrote:
> On 7/3/24 20:43, Thomas Huth wrote:
>> On 28/02/2024 17.43, Zhao Liu wrote:
>>> Hi Philippe,
>>>
>>>> +/*
>>>> + * Real ICH9 contains a single SMI output line and doesn't
>>>> broadcast CPUs.
>>>> + * Virtualized ICH9 allows broadcasting upon negatiation with
>>>> guest, see
>>>> + * commit 5ce45c7a2b.
>>>> + */
>>>> +enum {
>>>> +    ICH9_VIRT_SMI_BROADCAST,
>>>> +    ICH9_VIRT_SMI_CURRENT,
>>>> +#define ICH9_VIRT_SMI_COUNT 2
>>>> +};
>>>> +
>>>
>>> Just quick look here. Shouldn't ICH9_VIRT_SMI_COUNT be defined
>>> outside of
>>> enum {}?
>>
>> Or even better, do it without a #define:
>>
>> enum {
>>      ICH9_VIRT_SMI_BROADCAST,
>>      ICH9_VIRT_SMI_CURRENT,
>>      ICH9_VIRT_SMI_COUNT
> 
> This form isn't recommended as it confuses static analyzers,
> considering ICH9_VIRT_SMI_COUNT as part of the enum.

Side comment: I didn't know about this (so thanks for the info), but
that's really a shame for those static analyzers. It's an ancient and
valid pattern. :/

> 
>> };
>>
>>   Thomas
>>
> 

Re: [RFC PATCH 4/5] hw/i386/q35: Wire virtual SMI# lines to ICH9 chipset

[Bernhard Beschow]

Am 8. März 2024 08:10:24 UTC schrieb Laszlo Ersek <lersek@redhat.com>:
>On 3/8/24 09:08, Philippe Mathieu-Daudé wrote:
>> On 7/3/24 20:43, Thomas Huth wrote:
>>> On 28/02/2024 17.43, Zhao Liu wrote:
>>>> Hi Philippe,
>>>>
>>>>> +/*
>>>>> + * Real ICH9 contains a single SMI output line and doesn't
>>>>> broadcast CPUs.
>>>>> + * Virtualized ICH9 allows broadcasting upon negatiation with
>>>>> guest, see
>>>>> + * commit 5ce45c7a2b.
>>>>> + */
>>>>> +enum {
>>>>> +    ICH9_VIRT_SMI_BROADCAST,
>>>>> +    ICH9_VIRT_SMI_CURRENT,
>>>>> +#define ICH9_VIRT_SMI_COUNT 2
>>>>> +};
>>>>> +
>>>>
>>>> Just quick look here. Shouldn't ICH9_VIRT_SMI_COUNT be defined
>>>> outside of
>>>> enum {}?
>>>
>>> Or even better, do it without a #define:
>>>
>>> enum {
>>>      ICH9_VIRT_SMI_BROADCAST,
>>>      ICH9_VIRT_SMI_CURRENT,
>>>      ICH9_VIRT_SMI_COUNT
>> 
>> This form isn't recommended as it confuses static analyzers,
>> considering ICH9_VIRT_SMI_COUNT as part of the enum.
>
>Side comment: I didn't know about this (so thanks for the info), but
>that's really a shame for those static analyzers. It's an ancient and
>valid pattern. :/

Another pattern would be:

    enum {
        ICH9_VIRT_SMI_BROADCAST,
        ICH9_VIRT_SMI_CURRENT,
        ICH9_VIRT_SMI_LAST = ICH9_VIRT_SMI_CURRENT
    };

which should also work with GCC's `-Wswitch-enum`.

Best regards,
Bernhard

>
>> 
>>> };
>>>
>>>   Thomas
>>>
>> 
>

Re: [RFC PATCH 4/5] hw/i386/q35: Wire virtual SMI# lines to ICH9 chipset

[Thomas Huth]

On 08/03/2024 09.08, Philippe Mathieu-Daudé wrote:
> On 7/3/24 20:43, Thomas Huth wrote:
>> On 28/02/2024 17.43, Zhao Liu wrote:
>>> Hi Philippe,
>>>
>>>> +/*
>>>> + * Real ICH9 contains a single SMI output line and doesn't broadcast CPUs.
>>>> + * Virtualized ICH9 allows broadcasting upon negatiation with guest, see
>>>> + * commit 5ce45c7a2b.
>>>> + */
>>>> +enum {
>>>> +    ICH9_VIRT_SMI_BROADCAST,
>>>> +    ICH9_VIRT_SMI_CURRENT,
>>>> +#define ICH9_VIRT_SMI_COUNT 2
>>>> +};
>>>> +
>>>
>>> Just quick look here. Shouldn't ICH9_VIRT_SMI_COUNT be defined outside of
>>> enum {}?
>>
>> Or even better, do it without a #define:
>>
>> enum {
>>      ICH9_VIRT_SMI_BROADCAST,
>>      ICH9_VIRT_SMI_CURRENT,
>>      ICH9_VIRT_SMI_COUNT
> 
> This form isn't recommended as it confuses static analyzers,
> considering ICH9_VIRT_SMI_COUNT as part of the enum.

Never heard of that before. We're using it all over the place, e.g.:

typedef enum {
     THROTTLE_BPS_TOTAL,
     THROTTLE_BPS_READ,
     THROTTLE_BPS_WRITE,
     THROTTLE_OPS_TOTAL,
     THROTTLE_OPS_READ,
     THROTTLE_OPS_WRITE,
     BUCKETS_COUNT,
} BucketType;

... and even in our generated QAPI code, e.g.:

typedef enum QCryptoHashAlgorithm {
     QCRYPTO_HASH_ALG_MD5,
     QCRYPTO_HASH_ALG_SHA1,
     QCRYPTO_HASH_ALG_SHA224,
     QCRYPTO_HASH_ALG_SHA256,
     QCRYPTO_HASH_ALG_SHA384,
     QCRYPTO_HASH_ALG_SHA512,
     QCRYPTO_HASH_ALG_RIPEMD160,
     QCRYPTO_HASH_ALG__MAX,
} QCryptoHashAlgorithm;

Where did you see here a problem with static analyzers?

  Thomas

Re: [RFC PATCH 4/5] hw/i386/q35: Wire virtual SMI# lines to ICH9 chipset

[Peter Maydell]

On Fri, 8 Mar 2024 at 16:06, Thomas Huth <thuth@redhat.com> wrote:
>
> On 08/03/2024 09.08, Philippe Mathieu-Daudé wrote:
> > This form isn't recommended as it confuses static analyzers,
> > considering ICH9_VIRT_SMI_COUNT as part of the enum.
>
> Never heard of that before. We're using it all over the place, e.g.:
>
> typedef enum {
>      THROTTLE_BPS_TOTAL,
>      THROTTLE_BPS_READ,
>      THROTTLE_BPS_WRITE,
>      THROTTLE_OPS_TOTAL,
>      THROTTLE_OPS_READ,
>      THROTTLE_OPS_WRITE,
>      BUCKETS_COUNT,
> } BucketType;
>
> ... and even in our generated QAPI code, e.g.:
>
> typedef enum QCryptoHashAlgorithm {
>      QCRYPTO_HASH_ALG_MD5,
>      QCRYPTO_HASH_ALG_SHA1,
>      QCRYPTO_HASH_ALG_SHA224,
>      QCRYPTO_HASH_ALG_SHA256,
>      QCRYPTO_HASH_ALG_SHA384,
>      QCRYPTO_HASH_ALG_SHA512,
>      QCRYPTO_HASH_ALG_RIPEMD160,
>      QCRYPTO_HASH_ALG__MAX,
> } QCryptoHashAlgorithm;
>
> Where did you see here a problem with static analyzers?

Coverity tends to dislike this pattern if the enum is used
as an index into an array; for example commit b12635ff08ab2
("migration: fix coverity migrate_mode finding") is
essentially a workaround for the way the QAPI generated code
puts the MAX value inside the enum. Coverity assumes that
if you have a variable foo which is a SomeEnum then it can take
any of the valid values of the enum, so if you use foo
as an index into an array that was defined as
array[SOME_ENUM_MAX] where SOME_ENUM_MAX is a value of the
enum type, and you don't explicitly check that foo
is not SOME_ENUM_MAX, then it is an overrun.

thanks
-- PMM

Re: [RFC PATCH 4/5] hw/i386/q35: Wire virtual SMI# lines to ICH9 chipset

[Philippe Mathieu-Daudé]

On 8/3/24 17:06, Thomas Huth wrote:
> On 08/03/2024 09.08, Philippe Mathieu-Daudé wrote:
>> On 7/3/24 20:43, Thomas Huth wrote:
>>> On 28/02/2024 17.43, Zhao Liu wrote:
>>>> Hi Philippe,
>>>>
>>>>> +/*
>>>>> + * Real ICH9 contains a single SMI output line and doesn't 
>>>>> broadcast CPUs.
>>>>> + * Virtualized ICH9 allows broadcasting upon negatiation with 
>>>>> guest, see
>>>>> + * commit 5ce45c7a2b.
>>>>> + */
>>>>> +enum {
>>>>> +    ICH9_VIRT_SMI_BROADCAST,
>>>>> +    ICH9_VIRT_SMI_CURRENT,
>>>>> +#define ICH9_VIRT_SMI_COUNT 2
>>>>> +};
>>>>> +
>>>>
>>>> Just quick look here. Shouldn't ICH9_VIRT_SMI_COUNT be defined 
>>>> outside of
>>>> enum {}?
>>>
>>> Or even better, do it without a #define:
>>>
>>> enum {
>>>      ICH9_VIRT_SMI_BROADCAST,
>>>      ICH9_VIRT_SMI_CURRENT,
>>>      ICH9_VIRT_SMI_COUNT
>>
>> This form isn't recommended as it confuses static analyzers,
>> considering ICH9_VIRT_SMI_COUNT as part of the enum.
> 
> Never heard of that before. We're using it all over the place, e.g.:
> 
> typedef enum {
>      THROTTLE_BPS_TOTAL,
>      THROTTLE_BPS_READ,
>      THROTTLE_BPS_WRITE,
>      THROTTLE_OPS_TOTAL,
>      THROTTLE_OPS_READ,
>      THROTTLE_OPS_WRITE,
>      BUCKETS_COUNT,
> } BucketType;
> 
> ... and even in our generated QAPI code, e.g.:
> 
> typedef enum QCryptoHashAlgorithm {
>      QCRYPTO_HASH_ALG_MD5,
>      QCRYPTO_HASH_ALG_SHA1,
>      QCRYPTO_HASH_ALG_SHA224,
>      QCRYPTO_HASH_ALG_SHA256,
>      QCRYPTO_HASH_ALG_SHA384,
>      QCRYPTO_HASH_ALG_SHA512,
>      QCRYPTO_HASH_ALG_RIPEMD160,
>      QCRYPTO_HASH_ALG__MAX,
> } QCryptoHashAlgorithm;

We tried to remove it:

https://lore.kernel.org/qemu-devel/20230315112811.22355-4-philmd@linaro.org/

But there is a problem with generated empty enums...
https://lore.kernel.org/qemu-devel/87sfdx9w58.fsf@pond.sub.org/

[RFC PATCH 5/5] hw/isa: Build ich9_lpc.c once

[Philippe Mathieu-Daudé]

ich9_lpc.c doesn't contain any target-specific calls,
lets build it once.

Signed-off-by: Philippe Mathieu-Daudé <philmd@linaro.org>
---
 hw/isa/meson.build | 3 +--
 1 file changed, 1 insertion(+), 2 deletions(-)

diff --git a/hw/isa/meson.build b/hw/isa/meson.build
index 3b5504f78d..df58287d62 100644
--- a/hw/isa/meson.build
+++ b/hw/isa/meson.build
@@ -3,9 +3,8 @@ system_ss.add(when: 'CONFIG_FDC37M81X', if_true: files('fdc37m81x-superio.c'))
 system_ss.add(when: 'CONFIG_I82378', if_true: files('i82378.c'))
 system_ss.add(when: 'CONFIG_ISA_BUS', if_true: files('isa-bus.c'))
 system_ss.add(when: 'CONFIG_ISA_SUPERIO', if_true: files('isa-superio.c'))
+system_ss.add(when: 'CONFIG_LPC_ICH9', if_true: files('ich9_lpc.c'))
 system_ss.add(when: 'CONFIG_PC87312', if_true: files('pc87312.c'))
 system_ss.add(when: 'CONFIG_PIIX', if_true: files('piix.c'))
 system_ss.add(when: 'CONFIG_SMC37C669', if_true: files('smc37c669-superio.c'))
 system_ss.add(when: 'CONFIG_VT82C686', if_true: files('vt82c686.c'))
-
-specific_ss.add(when: 'CONFIG_LPC_ICH9', if_true: files('ich9_lpc.c'))
-- 
2.41.0

Re: [RFC PATCH 0/5] hw/i386/q35: Decouple virtual SMI# lines and wire them to ICH9 chipset

[Laszlo Ersek]

Hi Phil,

On 2/26/24 17:49, Philippe Mathieu-Daudé wrote:
> Hi,
>
> This is an experimental series to reduce calls to the
> cpu_interrupt() API from generic HW/. I'm trying to use
> the ICH9 chipset from a non-x86 machine. Without this
> experiment, we can not because cpu_interrupt() is target
> specific. Here the interrupt is decoupled using the QDev
> GPIO API. Even if the SMI# line is left unconnected, the
> device is still usable by a guest.
>
> Based-on: <20240226111416.39217-1-philmd@linaro.org>
>
> Philippe Mathieu-Daudé (5):
>   target/i386/cpu: Expose SMI# IRQ line via QDev
>   hw/i386/piix: Set CPU SMI# interrupt using QDev GPIO API
>   hw/ahci/ich9_tco: Set CPU SMI# interrupt using QDev GPIO API
>   hw/i386/q35: Wire virtual SMI# lines to ICH9 chipset
>   hw/isa: Build ich9_lpc.c once
>
>  include/hw/acpi/ich9.h        |  1 +
>  include/hw/acpi/ich9_tco.h    |  4 ++--
>  include/hw/i386/pc.h          |  2 --
>  include/hw/isa/ich9_lpc.h     | 12 ++++++++++++
>  include/hw/southbridge/ich9.h |  1 +
>  target/i386/cpu-internal.h    |  1 +
>  hw/acpi/ich9.c                |  3 ++-
>  hw/acpi/ich9_tco.c            | 13 ++++++++++---
>  hw/i386/pc.c                  |  9 ---------
>  hw/i386/pc_piix.c             |  4 ++--
>  hw/i386/pc_q35.c              | 26 ++++++++++++++++++++++++++
>  hw/isa/ich9_lpc.c             | 15 ++++-----------
>  hw/southbridge/ich9.c         |  1 +
>  target/i386/cpu-sysemu.c      | 11 +++++++++++
>  target/i386/cpu.c             |  2 ++
>  hw/isa/meson.build            |  3 +--
>  16 files changed, 76 insertions(+), 32 deletions(-)
>

This series is over my head for a review, so the best I could offer
would be to test it.

However, even testing it seems like a challenge. First, I've found that,
when building QEMU at dccbaf0cc0f1, my usual libvirt guests don't start
-- I needed to search the web for the error message, and then apply the
revert series

  [PATCH 0/2] Revert "hw/i386/pc: Confine system flash handling to pc_sysfw"
  https://patchew.org/QEMU/20240226215909.30884-1-shentey@gmail.com/

With that, I managed to establish a "baseline" (test some OVMF SMM
stuff, such as UEFI variable services, ACPI S3 suspend/resume, VCPU
hotplug/hot-unplug).

Then I wanted to apply this series (on top of those reverts on top of
dccbaf0cc0f1). It doesn't apply.

Then I noticed you mentioned the dependency on:

  [PATCH v2 00/15] hw/southbridge: Extract ICH9 QOM container model
  https://patchew.org/QEMU/20240226111416.39217-1-philmd@linaro.org/

That only seems to make things more complicated:

- patchew says "Failed in applying to current master"

- in the blurb, you mention "Rebased on top of Bernhard patches";
however, the above reverts appear to undo some of those patches
precisely, so I'm unsure how stable that foundation should be
considered.

I'd prefer waiting until all these patches stabilized a bit, and the
foundation all went upstream, and then I'd have to apply (a new version
of) this particular series only, on the then-master branch, for testing.

Laszlo