| Message ID | 20240226164913.94077-5-philmd@linaro.org |
|---|---|
| State | New |
| Headers | show |
| Series | hw/i386/q35: Decouple virtual SMI# lines and wire them to ICH9 chipset | expand |
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 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 >>> >> >
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 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 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/
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",
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(-)