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
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
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
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
>
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
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/
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
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
>>
>
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
>>>
>>
>
© 2016 - 2024 Red Hat, Inc.