From: Philippe Mathieu-Daudé <philmd@linaro.org>
v2:
- do not check for SMI features if hotplug happens when
SMI is not enabled. (matters for qtest and possibly seabios)
- removing property also removes default ICH9_LPC_SMI_F_BROADCAST_BIT
put default back in place only set it initfn() instead
The ICH9_LPC_SMI_F_BROADCAST_BIT feature bit was only set
in the pc_compat_2_8[] array, via the 'x-smi-broadcast=off'
property. We removed all machines using that array, lets remove
that property and all the code around it.
Signed-off-by: Philippe Mathieu-Daudé <philmd@linaro.org>
Signed-off-by: Igor Mammedov <imammedo@redhat.com>
---
hw/acpi/ich9.c | 4 ++--
hw/isa/lpc_ich9.c | 23 ++++-------------------
2 files changed, 6 insertions(+), 21 deletions(-)
diff --git a/hw/acpi/ich9.c b/hw/acpi/ich9.c
index bbb1bd60a2..87afe86bcc 100644
--- a/hw/acpi/ich9.c
+++ b/hw/acpi/ich9.c
@@ -432,7 +432,7 @@ void ich9_pm_device_pre_plug_cb(HotplugHandler *hotplug_dev, DeviceState *dev,
if (object_dynamic_cast(OBJECT(dev), TYPE_CPU)) {
uint64_t negotiated = lpc->smi_negotiated_features;
- if (negotiated & BIT_ULL(ICH9_LPC_SMI_F_BROADCAST_BIT) &&
+ if (lpc->pm.smi_en & ICH9_PMIO_SMI_EN_APMC_EN &&
!(negotiated & BIT_ULL(ICH9_LPC_SMI_F_CPU_HOTPLUG_BIT))) {
error_setg(errp, "cpu hotplug with SMI wasn't enabled by firmware");
error_append_hint(errp, "update machine type to newer than 5.1 "
@@ -476,7 +476,7 @@ void ich9_pm_device_unplug_request_cb(HotplugHandler *hotplug_dev,
} else if (object_dynamic_cast(OBJECT(dev), TYPE_CPU)) {
uint64_t negotiated = lpc->smi_negotiated_features;
- if (negotiated & BIT_ULL(ICH9_LPC_SMI_F_BROADCAST_BIT) &&
+ if (lpc->pm.smi_en & ICH9_PMIO_SMI_EN_APMC_EN &&
!(negotiated & BIT_ULL(ICH9_LPC_SMI_F_CPU_HOT_UNPLUG_BIT))) {
error_setg(errp, "cpu hot-unplug with SMI wasn't enabled "
"by firmware");
diff --git a/hw/isa/lpc_ich9.c b/hw/isa/lpc_ich9.c
index 51dc680029..04169ffa24 100644
--- a/hw/isa/lpc_ich9.c
+++ b/hw/isa/lpc_ich9.c
@@ -404,15 +404,6 @@ static void smi_features_ok_callback(void *opaque)
guest_cpu_hotplug_features = guest_features &
(BIT_ULL(ICH9_LPC_SMI_F_CPU_HOTPLUG_BIT) |
BIT_ULL(ICH9_LPC_SMI_F_CPU_HOT_UNPLUG_BIT));
- if (!(guest_features & BIT_ULL(ICH9_LPC_SMI_F_BROADCAST_BIT)) &&
- guest_cpu_hotplug_features) {
- /*
- * cpu hot-[un]plug with SMI requires SMI broadcast,
- * leave @features_ok at zero
- */
- return;
- }
-
if (guest_cpu_hotplug_features ==
BIT_ULL(ICH9_LPC_SMI_F_CPU_HOT_UNPLUG_BIT)) {
/* cpu hot-unplug is unsupported without cpu-hotplug */
@@ -474,14 +465,9 @@ static void ich9_apm_ctrl_changed(uint32_t val, void *arg)
/* SMI_EN = PMBASE + 30. SMI control and enable register */
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);
- }
- } else {
- cpu_interrupt(current_cpu, CPU_INTERRUPT_SMI);
+ CPUState *cs;
+ CPU_FOREACH(cs) {
+ cpu_interrupt(cs, CPU_INTERRUPT_SMI);
}
}
}
@@ -685,6 +671,7 @@ static void ich9_lpc_initfn(Object *obj)
static const uint8_t acpi_enable_cmd = ICH9_APM_ACPI_ENABLE;
static const uint8_t acpi_disable_cmd = ICH9_APM_ACPI_DISABLE;
+ lpc->smi_host_features = BIT_ULL(ICH9_LPC_SMI_F_BROADCAST_BIT);
object_initialize_child(obj, "rtc", &lpc->rtc, TYPE_MC146818_RTC);
@@ -834,8 +821,6 @@ static const Property ich9_lpc_properties[] = {
DEFINE_PROP_BOOL("noreboot", ICH9LPCState, pin_strap.spkr_hi, false),
DEFINE_PROP_BOOL("smm-compat", ICH9LPCState, pm.smm_compat, false),
DEFINE_PROP_BOOL("smm-enabled", ICH9LPCState, pm.smm_enabled, false),
- DEFINE_PROP_BIT64("x-smi-broadcast", ICH9LPCState, smi_host_features,
- ICH9_LPC_SMI_F_BROADCAST_BIT, true),
DEFINE_PROP_BIT64("x-smi-cpu-hotplug", ICH9LPCState, smi_host_features,
ICH9_LPC_SMI_F_CPU_HOTPLUG_BIT, true),
DEFINE_PROP_BIT64("x-smi-cpu-hotunplug", ICH9LPCState, smi_host_features,
--
2.47.3
On 03/03/2026 15.00, Igor Mammedov wrote:
> From: Philippe Mathieu-Daudé <philmd@linaro.org>
>
> v2:
> - do not check for SMI features if hotplug happens when
> SMI is not enabled. (matters for qtest and possibly seabios)
> - removing property also removes default ICH9_LPC_SMI_F_BROADCAST_BIT
> put default back in place only set it initfn() instead
>
> The ICH9_LPC_SMI_F_BROADCAST_BIT feature bit was only set
> in the pc_compat_2_8[] array, via the 'x-smi-broadcast=off'
> property. We removed all machines using that array, lets remove
> that property and all the code around it.
>
> Signed-off-by: Philippe Mathieu-Daudé <philmd@linaro.org>
> Signed-off-by: Igor Mammedov <imammedo@redhat.com>
> ---
> hw/acpi/ich9.c | 4 ++--
> hw/isa/lpc_ich9.c | 23 ++++-------------------
> 2 files changed, 6 insertions(+), 21 deletions(-)
>
> diff --git a/hw/acpi/ich9.c b/hw/acpi/ich9.c
> index bbb1bd60a2..87afe86bcc 100644
> --- a/hw/acpi/ich9.c
> +++ b/hw/acpi/ich9.c
> @@ -432,7 +432,7 @@ void ich9_pm_device_pre_plug_cb(HotplugHandler *hotplug_dev, DeviceState *dev,
> if (object_dynamic_cast(OBJECT(dev), TYPE_CPU)) {
> uint64_t negotiated = lpc->smi_negotiated_features;
>
> - if (negotiated & BIT_ULL(ICH9_LPC_SMI_F_BROADCAST_BIT) &&
> + if (lpc->pm.smi_en & ICH9_PMIO_SMI_EN_APMC_EN &&
> !(negotiated & BIT_ULL(ICH9_LPC_SMI_F_CPU_HOTPLUG_BIT))) {
> error_setg(errp, "cpu hotplug with SMI wasn't enabled by firmware");
> error_append_hint(errp, "update machine type to newer than 5.1 "
> @@ -476,7 +476,7 @@ void ich9_pm_device_unplug_request_cb(HotplugHandler *hotplug_dev,
> } else if (object_dynamic_cast(OBJECT(dev), TYPE_CPU)) {
> uint64_t negotiated = lpc->smi_negotiated_features;
>
> - if (negotiated & BIT_ULL(ICH9_LPC_SMI_F_BROADCAST_BIT) &&
> + if (lpc->pm.smi_en & ICH9_PMIO_SMI_EN_APMC_EN &&
> !(negotiated & BIT_ULL(ICH9_LPC_SMI_F_CPU_HOT_UNPLUG_BIT))) {
> error_setg(errp, "cpu hot-unplug with SMI wasn't enabled "
> "by firmware");
> diff --git a/hw/isa/lpc_ich9.c b/hw/isa/lpc_ich9.c
> index 51dc680029..04169ffa24 100644
> --- a/hw/isa/lpc_ich9.c
> +++ b/hw/isa/lpc_ich9.c
> @@ -404,15 +404,6 @@ static void smi_features_ok_callback(void *opaque)
> guest_cpu_hotplug_features = guest_features &
> (BIT_ULL(ICH9_LPC_SMI_F_CPU_HOTPLUG_BIT) |
> BIT_ULL(ICH9_LPC_SMI_F_CPU_HOT_UNPLUG_BIT));
> - if (!(guest_features & BIT_ULL(ICH9_LPC_SMI_F_BROADCAST_BIT)) &&
> - guest_cpu_hotplug_features) {
> - /*
> - * cpu hot-[un]plug with SMI requires SMI broadcast,
> - * leave @features_ok at zero
> - */
> - return;
> - }
> -
> if (guest_cpu_hotplug_features ==
> BIT_ULL(ICH9_LPC_SMI_F_CPU_HOT_UNPLUG_BIT)) {
> /* cpu hot-unplug is unsupported without cpu-hotplug */
> @@ -474,14 +465,9 @@ static void ich9_apm_ctrl_changed(uint32_t val, void *arg)
>
> /* SMI_EN = PMBASE + 30. SMI control and enable register */
> 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);
> - }
> - } else {
> - cpu_interrupt(current_cpu, CPU_INTERRUPT_SMI);
> + CPUState *cs;
> + CPU_FOREACH(cs) {
> + cpu_interrupt(cs, CPU_INTERRUPT_SMI);
> }
> }
> }
> @@ -685,6 +671,7 @@ static void ich9_lpc_initfn(Object *obj)
>
> static const uint8_t acpi_enable_cmd = ICH9_APM_ACPI_ENABLE;
> static const uint8_t acpi_disable_cmd = ICH9_APM_ACPI_DISABLE;
> + lpc->smi_host_features = BIT_ULL(ICH9_LPC_SMI_F_BROADCAST_BIT);
Shouldn't that rather be a "|=" instead of the "=" so that we don't lose the
other bits?
Thomas
> object_initialize_child(obj, "rtc", &lpc->rtc, TYPE_MC146818_RTC);
>
> @@ -834,8 +821,6 @@ static const Property ich9_lpc_properties[] = {
> DEFINE_PROP_BOOL("noreboot", ICH9LPCState, pin_strap.spkr_hi, false),
> DEFINE_PROP_BOOL("smm-compat", ICH9LPCState, pm.smm_compat, false),
> DEFINE_PROP_BOOL("smm-enabled", ICH9LPCState, pm.smm_enabled, false),
> - DEFINE_PROP_BIT64("x-smi-broadcast", ICH9LPCState, smi_host_features,
> - ICH9_LPC_SMI_F_BROADCAST_BIT, true),
> DEFINE_PROP_BIT64("x-smi-cpu-hotplug", ICH9LPCState, smi_host_features,
> ICH9_LPC_SMI_F_CPU_HOTPLUG_BIT, true),
> DEFINE_PROP_BIT64("x-smi-cpu-hotunplug", ICH9LPCState, smi_host_features,
On Wed, 4 Mar 2026 18:20:27 +0100
Thomas Huth <thuth@redhat.com> wrote:
> On 03/03/2026 15.00, Igor Mammedov wrote:
> > From: Philippe Mathieu-Daudé <philmd@linaro.org>
> >
> > v2:
> > - do not check for SMI features if hotplug happens when
> > SMI is not enabled. (matters for qtest and possibly seabios)
> > - removing property also removes default ICH9_LPC_SMI_F_BROADCAST_BIT
> > put default back in place only set it initfn() instead
> >
> > The ICH9_LPC_SMI_F_BROADCAST_BIT feature bit was only set
> > in the pc_compat_2_8[] array, via the 'x-smi-broadcast=off'
> > property. We removed all machines using that array, lets remove
> > that property and all the code around it.
> >
> > Signed-off-by: Philippe Mathieu-Daudé <philmd@linaro.org>
> > Signed-off-by: Igor Mammedov <imammedo@redhat.com>
> > ---
> > hw/acpi/ich9.c | 4 ++--
> > hw/isa/lpc_ich9.c | 23 ++++-------------------
> > 2 files changed, 6 insertions(+), 21 deletions(-)
> >
> > diff --git a/hw/acpi/ich9.c b/hw/acpi/ich9.c
> > index bbb1bd60a2..87afe86bcc 100644
> > --- a/hw/acpi/ich9.c
> > +++ b/hw/acpi/ich9.c
> > @@ -432,7 +432,7 @@ void ich9_pm_device_pre_plug_cb(HotplugHandler *hotplug_dev, DeviceState *dev,
> > if (object_dynamic_cast(OBJECT(dev), TYPE_CPU)) {
> > uint64_t negotiated = lpc->smi_negotiated_features;
> >
> > - if (negotiated & BIT_ULL(ICH9_LPC_SMI_F_BROADCAST_BIT) &&
> > + if (lpc->pm.smi_en & ICH9_PMIO_SMI_EN_APMC_EN &&
> > !(negotiated & BIT_ULL(ICH9_LPC_SMI_F_CPU_HOTPLUG_BIT))) {
> > error_setg(errp, "cpu hotplug with SMI wasn't enabled by firmware");
> > error_append_hint(errp, "update machine type to newer than 5.1 "
> > @@ -476,7 +476,7 @@ void ich9_pm_device_unplug_request_cb(HotplugHandler *hotplug_dev,
> > } else if (object_dynamic_cast(OBJECT(dev), TYPE_CPU)) {
> > uint64_t negotiated = lpc->smi_negotiated_features;
> >
> > - if (negotiated & BIT_ULL(ICH9_LPC_SMI_F_BROADCAST_BIT) &&
> > + if (lpc->pm.smi_en & ICH9_PMIO_SMI_EN_APMC_EN &&
> > !(negotiated & BIT_ULL(ICH9_LPC_SMI_F_CPU_HOT_UNPLUG_BIT))) {
> > error_setg(errp, "cpu hot-unplug with SMI wasn't enabled "
> > "by firmware");
> > diff --git a/hw/isa/lpc_ich9.c b/hw/isa/lpc_ich9.c
> > index 51dc680029..04169ffa24 100644
> > --- a/hw/isa/lpc_ich9.c
> > +++ b/hw/isa/lpc_ich9.c
> > @@ -404,15 +404,6 @@ static void smi_features_ok_callback(void *opaque)
> > guest_cpu_hotplug_features = guest_features &
> > (BIT_ULL(ICH9_LPC_SMI_F_CPU_HOTPLUG_BIT) |
> > BIT_ULL(ICH9_LPC_SMI_F_CPU_HOT_UNPLUG_BIT));
> > - if (!(guest_features & BIT_ULL(ICH9_LPC_SMI_F_BROADCAST_BIT)) &&
> > - guest_cpu_hotplug_features) {
> > - /*
> > - * cpu hot-[un]plug with SMI requires SMI broadcast,
> > - * leave @features_ok at zero
> > - */
> > - return;
> > - }
> > -
> > if (guest_cpu_hotplug_features ==
> > BIT_ULL(ICH9_LPC_SMI_F_CPU_HOT_UNPLUG_BIT)) {
> > /* cpu hot-unplug is unsupported without cpu-hotplug */
> > @@ -474,14 +465,9 @@ static void ich9_apm_ctrl_changed(uint32_t val, void *arg)
> >
> > /* SMI_EN = PMBASE + 30. SMI control and enable register */
> > 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);
> > - }
> > - } else {
> > - cpu_interrupt(current_cpu, CPU_INTERRUPT_SMI);
> > + CPUState *cs;
> > + CPU_FOREACH(cs) {
> > + cpu_interrupt(cs, CPU_INTERRUPT_SMI);
> > }
> > }
> > }
> > @@ -685,6 +671,7 @@ static void ich9_lpc_initfn(Object *obj)
> >
> > static const uint8_t acpi_enable_cmd = ICH9_APM_ACPI_ENABLE;
> > static const uint8_t acpi_disable_cmd = ICH9_APM_ACPI_DISABLE;
> > + lpc->smi_host_features = BIT_ULL(ICH9_LPC_SMI_F_BROADCAST_BIT);
>
> Shouldn't that rather be a "|=" instead of the "=" so that we don't lose the
> other bits?
Indeed it should be '|=',
can you fix it up on merge or should I better send fixed up version?
> Thomas
>
>
>
> > object_initialize_child(obj, "rtc", &lpc->rtc, TYPE_MC146818_RTC);
> >
> > @@ -834,8 +821,6 @@ static const Property ich9_lpc_properties[] = {
> > DEFINE_PROP_BOOL("noreboot", ICH9LPCState, pin_strap.spkr_hi, false),
> > DEFINE_PROP_BOOL("smm-compat", ICH9LPCState, pm.smm_compat, false),
> > DEFINE_PROP_BOOL("smm-enabled", ICH9LPCState, pm.smm_enabled, false),
> > - DEFINE_PROP_BIT64("x-smi-broadcast", ICH9LPCState, smi_host_features,
> > - ICH9_LPC_SMI_F_BROADCAST_BIT, true),
> > DEFINE_PROP_BIT64("x-smi-cpu-hotplug", ICH9LPCState, smi_host_features,
> > ICH9_LPC_SMI_F_CPU_HOTPLUG_BIT, true),
> > DEFINE_PROP_BIT64("x-smi-cpu-hotunplug", ICH9LPCState, smi_host_features,
>
On 05/03/2026 13.10, Igor Mammedov wrote:
> On Wed, 4 Mar 2026 18:20:27 +0100
> Thomas Huth <thuth@redhat.com> wrote:
>
>> On 03/03/2026 15.00, Igor Mammedov wrote:
>>> From: Philippe Mathieu-Daudé <philmd@linaro.org>
>>>
>>> v2:
>>> - do not check for SMI features if hotplug happens when
>>> SMI is not enabled. (matters for qtest and possibly seabios)
>>> - removing property also removes default ICH9_LPC_SMI_F_BROADCAST_BIT
>>> put default back in place only set it initfn() instead
>>>
>>> The ICH9_LPC_SMI_F_BROADCAST_BIT feature bit was only set
>>> in the pc_compat_2_8[] array, via the 'x-smi-broadcast=off'
>>> property. We removed all machines using that array, lets remove
>>> that property and all the code around it.
>>>
>>> Signed-off-by: Philippe Mathieu-Daudé <philmd@linaro.org>
>>> Signed-off-by: Igor Mammedov <imammedo@redhat.com>
>>> ---
>>> hw/acpi/ich9.c | 4 ++--
>>> hw/isa/lpc_ich9.c | 23 ++++-------------------
>>> 2 files changed, 6 insertions(+), 21 deletions(-)
>>>
>>> diff --git a/hw/acpi/ich9.c b/hw/acpi/ich9.c
>>> index bbb1bd60a2..87afe86bcc 100644
>>> --- a/hw/acpi/ich9.c
>>> +++ b/hw/acpi/ich9.c
>>> @@ -432,7 +432,7 @@ void ich9_pm_device_pre_plug_cb(HotplugHandler *hotplug_dev, DeviceState *dev,
>>> if (object_dynamic_cast(OBJECT(dev), TYPE_CPU)) {
>>> uint64_t negotiated = lpc->smi_negotiated_features;
>>>
>>> - if (negotiated & BIT_ULL(ICH9_LPC_SMI_F_BROADCAST_BIT) &&
>>> + if (lpc->pm.smi_en & ICH9_PMIO_SMI_EN_APMC_EN &&
>>> !(negotiated & BIT_ULL(ICH9_LPC_SMI_F_CPU_HOTPLUG_BIT))) {
>>> error_setg(errp, "cpu hotplug with SMI wasn't enabled by firmware");
>>> error_append_hint(errp, "update machine type to newer than 5.1 "
>>> @@ -476,7 +476,7 @@ void ich9_pm_device_unplug_request_cb(HotplugHandler *hotplug_dev,
>>> } else if (object_dynamic_cast(OBJECT(dev), TYPE_CPU)) {
>>> uint64_t negotiated = lpc->smi_negotiated_features;
>>>
>>> - if (negotiated & BIT_ULL(ICH9_LPC_SMI_F_BROADCAST_BIT) &&
>>> + if (lpc->pm.smi_en & ICH9_PMIO_SMI_EN_APMC_EN &&
>>> !(negotiated & BIT_ULL(ICH9_LPC_SMI_F_CPU_HOT_UNPLUG_BIT))) {
>>> error_setg(errp, "cpu hot-unplug with SMI wasn't enabled "
>>> "by firmware");
>>> diff --git a/hw/isa/lpc_ich9.c b/hw/isa/lpc_ich9.c
>>> index 51dc680029..04169ffa24 100644
>>> --- a/hw/isa/lpc_ich9.c
>>> +++ b/hw/isa/lpc_ich9.c
>>> @@ -404,15 +404,6 @@ static void smi_features_ok_callback(void *opaque)
>>> guest_cpu_hotplug_features = guest_features &
>>> (BIT_ULL(ICH9_LPC_SMI_F_CPU_HOTPLUG_BIT) |
>>> BIT_ULL(ICH9_LPC_SMI_F_CPU_HOT_UNPLUG_BIT));
>>> - if (!(guest_features & BIT_ULL(ICH9_LPC_SMI_F_BROADCAST_BIT)) &&
>>> - guest_cpu_hotplug_features) {
>>> - /*
>>> - * cpu hot-[un]plug with SMI requires SMI broadcast,
>>> - * leave @features_ok at zero
>>> - */
>>> - return;
>>> - }
>>> -
>>> if (guest_cpu_hotplug_features ==
>>> BIT_ULL(ICH9_LPC_SMI_F_CPU_HOT_UNPLUG_BIT)) {
>>> /* cpu hot-unplug is unsupported without cpu-hotplug */
>>> @@ -474,14 +465,9 @@ static void ich9_apm_ctrl_changed(uint32_t val, void *arg)
>>>
>>> /* SMI_EN = PMBASE + 30. SMI control and enable register */
>>> 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);
>>> - }
>>> - } else {
>>> - cpu_interrupt(current_cpu, CPU_INTERRUPT_SMI);
>>> + CPUState *cs;
>>> + CPU_FOREACH(cs) {
>>> + cpu_interrupt(cs, CPU_INTERRUPT_SMI);
>>> }
>>> }
>>> }
>>> @@ -685,6 +671,7 @@ static void ich9_lpc_initfn(Object *obj)
>>>
>>> static const uint8_t acpi_enable_cmd = ICH9_APM_ACPI_ENABLE;
>>> static const uint8_t acpi_disable_cmd = ICH9_APM_ACPI_DISABLE;
>>> + lpc->smi_host_features = BIT_ULL(ICH9_LPC_SMI_F_BROADCAST_BIT);
>>
>> Shouldn't that rather be a "|=" instead of the "=" so that we don't lose the
>> other bits?
>
> Indeed it should be '|=',
> can you fix it up on merge or should I better send fixed up version?
I tried to fix it, but I still see an issue with the bios-tables-test qtest
during "make check" - it does not finish anymore and is finally killed by
the timeout. Does "make check-qtest" work for you?
Thomas
On Thu, 5 Mar 2026 13:33:21 +0100
Thomas Huth <thuth@redhat.com> wrote:
> On 05/03/2026 13.10, Igor Mammedov wrote:
> > On Wed, 4 Mar 2026 18:20:27 +0100
> > Thomas Huth <thuth@redhat.com> wrote:
> >
> >> On 03/03/2026 15.00, Igor Mammedov wrote:
> >>> From: Philippe Mathieu-Daudé <philmd@linaro.org>
> >>>
> >>> v2:
> >>> - do not check for SMI features if hotplug happens when
> >>> SMI is not enabled. (matters for qtest and possibly seabios)
> >>> - removing property also removes default ICH9_LPC_SMI_F_BROADCAST_BIT
> >>> put default back in place only set it initfn() instead
> >>>
> >>> The ICH9_LPC_SMI_F_BROADCAST_BIT feature bit was only set
> >>> in the pc_compat_2_8[] array, via the 'x-smi-broadcast=off'
> >>> property. We removed all machines using that array, lets remove
> >>> that property and all the code around it.
> >>>
> >>> Signed-off-by: Philippe Mathieu-Daudé <philmd@linaro.org>
> >>> Signed-off-by: Igor Mammedov <imammedo@redhat.com>
> >>> ---
> >>> hw/acpi/ich9.c | 4 ++--
> >>> hw/isa/lpc_ich9.c | 23 ++++-------------------
> >>> 2 files changed, 6 insertions(+), 21 deletions(-)
> >>>
> >>> diff --git a/hw/acpi/ich9.c b/hw/acpi/ich9.c
> >>> index bbb1bd60a2..87afe86bcc 100644
> >>> --- a/hw/acpi/ich9.c
> >>> +++ b/hw/acpi/ich9.c
> >>> @@ -432,7 +432,7 @@ void ich9_pm_device_pre_plug_cb(HotplugHandler *hotplug_dev, DeviceState *dev,
> >>> if (object_dynamic_cast(OBJECT(dev), TYPE_CPU)) {
> >>> uint64_t negotiated = lpc->smi_negotiated_features;
> >>>
> >>> - if (negotiated & BIT_ULL(ICH9_LPC_SMI_F_BROADCAST_BIT) &&
> >>> + if (lpc->pm.smi_en & ICH9_PMIO_SMI_EN_APMC_EN &&
> >>> !(negotiated & BIT_ULL(ICH9_LPC_SMI_F_CPU_HOTPLUG_BIT))) {
> >>> error_setg(errp, "cpu hotplug with SMI wasn't enabled by firmware");
> >>> error_append_hint(errp, "update machine type to newer than 5.1 "
> >>> @@ -476,7 +476,7 @@ void ich9_pm_device_unplug_request_cb(HotplugHandler *hotplug_dev,
> >>> } else if (object_dynamic_cast(OBJECT(dev), TYPE_CPU)) {
> >>> uint64_t negotiated = lpc->smi_negotiated_features;
> >>>
> >>> - if (negotiated & BIT_ULL(ICH9_LPC_SMI_F_BROADCAST_BIT) &&
> >>> + if (lpc->pm.smi_en & ICH9_PMIO_SMI_EN_APMC_EN &&
> >>> !(negotiated & BIT_ULL(ICH9_LPC_SMI_F_CPU_HOT_UNPLUG_BIT))) {
> >>> error_setg(errp, "cpu hot-unplug with SMI wasn't enabled "
> >>> "by firmware");
> >>> diff --git a/hw/isa/lpc_ich9.c b/hw/isa/lpc_ich9.c
> >>> index 51dc680029..04169ffa24 100644
> >>> --- a/hw/isa/lpc_ich9.c
> >>> +++ b/hw/isa/lpc_ich9.c
> >>> @@ -404,15 +404,6 @@ static void smi_features_ok_callback(void *opaque)
> >>> guest_cpu_hotplug_features = guest_features &
> >>> (BIT_ULL(ICH9_LPC_SMI_F_CPU_HOTPLUG_BIT) |
> >>> BIT_ULL(ICH9_LPC_SMI_F_CPU_HOT_UNPLUG_BIT));
> >>> - if (!(guest_features & BIT_ULL(ICH9_LPC_SMI_F_BROADCAST_BIT)) &&
> >>> - guest_cpu_hotplug_features) {
> >>> - /*
> >>> - * cpu hot-[un]plug with SMI requires SMI broadcast,
> >>> - * leave @features_ok at zero
> >>> - */
> >>> - return;
> >>> - }
> >>> -
> >>> if (guest_cpu_hotplug_features ==
> >>> BIT_ULL(ICH9_LPC_SMI_F_CPU_HOT_UNPLUG_BIT)) {
> >>> /* cpu hot-unplug is unsupported without cpu-hotplug */
> >>> @@ -474,14 +465,9 @@ static void ich9_apm_ctrl_changed(uint32_t val, void *arg)
> >>>
> >>> /* SMI_EN = PMBASE + 30. SMI control and enable register */
> >>> 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);
> >>> - }
> >>> - } else {
> >>> - cpu_interrupt(current_cpu, CPU_INTERRUPT_SMI);
> >>> + CPUState *cs;
> >>> + CPU_FOREACH(cs) {
> >>> + cpu_interrupt(cs, CPU_INTERRUPT_SMI);
> >>> }
> >>> }
> >>> }
> >>> @@ -685,6 +671,7 @@ static void ich9_lpc_initfn(Object *obj)
> >>>
> >>> static const uint8_t acpi_enable_cmd = ICH9_APM_ACPI_ENABLE;
> >>> static const uint8_t acpi_disable_cmd = ICH9_APM_ACPI_DISABLE;
> >>> + lpc->smi_host_features = BIT_ULL(ICH9_LPC_SMI_F_BROADCAST_BIT);
> >>
> >> Shouldn't that rather be a "|=" instead of the "=" so that we don't lose the
> >> other bits?
> >
> > Indeed it should be '|=',
> > can you fix it up on merge or should I better send fixed up version?
>
> I tried to fix it, but I still see an issue with the bios-tables-test qtest
> during "make check" - it does not finish anymore and is finally killed by
> the timeout. Does "make check-qtest" work for you?
cpu hotplug test did work, let me fix and check tests again
>
> Thomas
>
On Thu, 5 Mar 2026 15:32:18 +0100
Igor Mammedov <imammedo@redhat.com> wrote:
> On Thu, 5 Mar 2026 13:33:21 +0100
> Thomas Huth <thuth@redhat.com> wrote:
>
> > On 05/03/2026 13.10, Igor Mammedov wrote:
> > > On Wed, 4 Mar 2026 18:20:27 +0100
> > > Thomas Huth <thuth@redhat.com> wrote:
> > >
> > >> On 03/03/2026 15.00, Igor Mammedov wrote:
> > >>> From: Philippe Mathieu-Daudé <philmd@linaro.org>
> > >>>
> > >>> v2:
> > >>> - do not check for SMI features if hotplug happens when
> > >>> SMI is not enabled. (matters for qtest and possibly seabios)
> > >>> - removing property also removes default ICH9_LPC_SMI_F_BROADCAST_BIT
> > >>> put default back in place only set it initfn() instead
> > >>>
> > >>> The ICH9_LPC_SMI_F_BROADCAST_BIT feature bit was only set
> > >>> in the pc_compat_2_8[] array, via the 'x-smi-broadcast=off'
> > >>> property. We removed all machines using that array, lets remove
> > >>> that property and all the code around it.
> > >>>
> > >>> Signed-off-by: Philippe Mathieu-Daudé <philmd@linaro.org>
> > >>> Signed-off-by: Igor Mammedov <imammedo@redhat.com>
> > >>> ---
> > >>> hw/acpi/ich9.c | 4 ++--
> > >>> hw/isa/lpc_ich9.c | 23 ++++-------------------
> > >>> 2 files changed, 6 insertions(+), 21 deletions(-)
> > >>>
> > >>> diff --git a/hw/acpi/ich9.c b/hw/acpi/ich9.c
> > >>> index bbb1bd60a2..87afe86bcc 100644
> > >>> --- a/hw/acpi/ich9.c
> > >>> +++ b/hw/acpi/ich9.c
> > >>> @@ -432,7 +432,7 @@ void ich9_pm_device_pre_plug_cb(HotplugHandler *hotplug_dev, DeviceState *dev,
> > >>> if (object_dynamic_cast(OBJECT(dev), TYPE_CPU)) {
> > >>> uint64_t negotiated = lpc->smi_negotiated_features;
> > >>>
> > >>> - if (negotiated & BIT_ULL(ICH9_LPC_SMI_F_BROADCAST_BIT) &&
> > >>> + if (lpc->pm.smi_en & ICH9_PMIO_SMI_EN_APMC_EN &&
> > >>> !(negotiated & BIT_ULL(ICH9_LPC_SMI_F_CPU_HOTPLUG_BIT))) {
> > >>> error_setg(errp, "cpu hotplug with SMI wasn't enabled by firmware");
> > >>> error_append_hint(errp, "update machine type to newer than 5.1 "
> > >>> @@ -476,7 +476,7 @@ void ich9_pm_device_unplug_request_cb(HotplugHandler *hotplug_dev,
> > >>> } else if (object_dynamic_cast(OBJECT(dev), TYPE_CPU)) {
> > >>> uint64_t negotiated = lpc->smi_negotiated_features;
> > >>>
> > >>> - if (negotiated & BIT_ULL(ICH9_LPC_SMI_F_BROADCAST_BIT) &&
> > >>> + if (lpc->pm.smi_en & ICH9_PMIO_SMI_EN_APMC_EN &&
> > >>> !(negotiated & BIT_ULL(ICH9_LPC_SMI_F_CPU_HOT_UNPLUG_BIT))) {
> > >>> error_setg(errp, "cpu hot-unplug with SMI wasn't enabled "
> > >>> "by firmware");
> > >>> diff --git a/hw/isa/lpc_ich9.c b/hw/isa/lpc_ich9.c
> > >>> index 51dc680029..04169ffa24 100644
> > >>> --- a/hw/isa/lpc_ich9.c
> > >>> +++ b/hw/isa/lpc_ich9.c
> > >>> @@ -404,15 +404,6 @@ static void smi_features_ok_callback(void *opaque)
> > >>> guest_cpu_hotplug_features = guest_features &
> > >>> (BIT_ULL(ICH9_LPC_SMI_F_CPU_HOTPLUG_BIT) |
> > >>> BIT_ULL(ICH9_LPC_SMI_F_CPU_HOT_UNPLUG_BIT));
> > >>> - if (!(guest_features & BIT_ULL(ICH9_LPC_SMI_F_BROADCAST_BIT)) &&
> > >>> - guest_cpu_hotplug_features) {
> > >>> - /*
> > >>> - * cpu hot-[un]plug with SMI requires SMI broadcast,
> > >>> - * leave @features_ok at zero
> > >>> - */
> > >>> - return;
> > >>> - }
> > >>> -
> > >>> if (guest_cpu_hotplug_features ==
> > >>> BIT_ULL(ICH9_LPC_SMI_F_CPU_HOT_UNPLUG_BIT)) {
> > >>> /* cpu hot-unplug is unsupported without cpu-hotplug */
> > >>> @@ -474,14 +465,9 @@ static void ich9_apm_ctrl_changed(uint32_t val, void *arg)
> > >>>
> > >>> /* SMI_EN = PMBASE + 30. SMI control and enable register */
> > >>> 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);
> > >>> - }
> > >>> - } else {
> > >>> - cpu_interrupt(current_cpu, CPU_INTERRUPT_SMI);
> > >>> + CPUState *cs;
> > >>> + CPU_FOREACH(cs) {
> > >>> + cpu_interrupt(cs, CPU_INTERRUPT_SMI);
> > >>> }
> > >>> }
> > >>> }
> > >>> @@ -685,6 +671,7 @@ static void ich9_lpc_initfn(Object *obj)
> > >>>
> > >>> static const uint8_t acpi_enable_cmd = ICH9_APM_ACPI_ENABLE;
> > >>> static const uint8_t acpi_disable_cmd = ICH9_APM_ACPI_DISABLE;
> > >>> + lpc->smi_host_features = BIT_ULL(ICH9_LPC_SMI_F_BROADCAST_BIT);
> > >>
> > >> Shouldn't that rather be a "|=" instead of the "=" so that we don't lose the
> > >> other bits?
> > >
> > > Indeed it should be '|=',
> > > can you fix it up on merge or should I better send fixed up version?
> >
> > I tried to fix it, but I still see an issue with the bios-tables-test qtest
> > during "make check" - it does not finish anymore and is finally killed by
> > the timeout. Does "make check-qtest" work for you?
>
> cpu hotplug test did work, let me fix and check tests again
what breaks is '-M q35 -smp >1 + seabios",
where the later asks for SMI => unexpected broadcast with this patch
and as result unhappy SeaBIOS, I'll send v3 as reply here.
>
> >
> > Thomas
> >
>
From: Philippe Mathieu-Daudé <philmd@linaro.org>
The ICH9_LPC_SMI_F_BROADCAST_BIT feature bit was only set
in the pc_compat_2_8[] array, via the 'x-smi-broadcast=off'
property. We removed all machines using that array, lets remove
that property.
However the code related to ICH9_LPC_SMI_F_BROADCAST_BIT
(OVMF optimization hack) has to stay as SMI broadcast is not what
hw do by default, and it will cause guest reboots if SMI is triggered
if firmware is not aware about it (SeaBIOS and pretty much everything
else except of OVMF).
Signed-off-by: Philippe Mathieu-Daudé <philmd@linaro.org>
Signed-off-by: Igor Mammedov <imammedo@redhat.com>
---
v3:
- use |= when initializing smi_host_features (Thomas)
- send SMI broadcast is OVMF hack, and has be be negotiated before it's used
v2:
- do not check for SMI features if hotplug happens when
SMI is not enabled. (matters for qtest and possibly seabios)
- removing property also removes default ICH9_LPC_SMI_F_BROADCAST_BIT
put default back in place only set it initfn() instead
---
hw/acpi/ich9.c | 4 ++--
hw/isa/lpc_ich9.c | 6 +++---
2 files changed, 5 insertions(+), 5 deletions(-)
diff --git a/hw/acpi/ich9.c b/hw/acpi/ich9.c
index bbb1bd60a2..87afe86bcc 100644
--- a/hw/acpi/ich9.c
+++ b/hw/acpi/ich9.c
@@ -432,7 +432,7 @@ void ich9_pm_device_pre_plug_cb(HotplugHandler *hotplug_dev, DeviceState *dev,
if (object_dynamic_cast(OBJECT(dev), TYPE_CPU)) {
uint64_t negotiated = lpc->smi_negotiated_features;
- if (negotiated & BIT_ULL(ICH9_LPC_SMI_F_BROADCAST_BIT) &&
+ if (lpc->pm.smi_en & ICH9_PMIO_SMI_EN_APMC_EN &&
!(negotiated & BIT_ULL(ICH9_LPC_SMI_F_CPU_HOTPLUG_BIT))) {
error_setg(errp, "cpu hotplug with SMI wasn't enabled by firmware");
error_append_hint(errp, "update machine type to newer than 5.1 "
@@ -476,7 +476,7 @@ void ich9_pm_device_unplug_request_cb(HotplugHandler *hotplug_dev,
} else if (object_dynamic_cast(OBJECT(dev), TYPE_CPU)) {
uint64_t negotiated = lpc->smi_negotiated_features;
- if (negotiated & BIT_ULL(ICH9_LPC_SMI_F_BROADCAST_BIT) &&
+ if (lpc->pm.smi_en & ICH9_PMIO_SMI_EN_APMC_EN &&
!(negotiated & BIT_ULL(ICH9_LPC_SMI_F_CPU_HOT_UNPLUG_BIT))) {
error_setg(errp, "cpu hot-unplug with SMI wasn't enabled "
"by firmware");
diff --git a/hw/isa/lpc_ich9.c b/hw/isa/lpc_ich9.c
index 51dc680029..8517ec9cae 100644
--- a/hw/isa/lpc_ich9.c
+++ b/hw/isa/lpc_ich9.c
@@ -476,11 +476,12 @@ 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)) {
+ /* optimization for OVMF with ICH9_LPC_SMI_F_BROADCAST_BIT */
CPUState *cs;
CPU_FOREACH(cs) {
cpu_interrupt(cs, CPU_INTERRUPT_SMI);
}
- } else {
+ } else { /* default ich9 behavior, SeaBIOS doesn't have SMI broadcast */
cpu_interrupt(current_cpu, CPU_INTERRUPT_SMI);
}
}
@@ -685,6 +686,7 @@ static void ich9_lpc_initfn(Object *obj)
static const uint8_t acpi_enable_cmd = ICH9_APM_ACPI_ENABLE;
static const uint8_t acpi_disable_cmd = ICH9_APM_ACPI_DISABLE;
+ lpc->smi_host_features |= BIT_ULL(ICH9_LPC_SMI_F_BROADCAST_BIT);
object_initialize_child(obj, "rtc", &lpc->rtc, TYPE_MC146818_RTC);
@@ -834,8 +836,6 @@ static const Property ich9_lpc_properties[] = {
DEFINE_PROP_BOOL("noreboot", ICH9LPCState, pin_strap.spkr_hi, false),
DEFINE_PROP_BOOL("smm-compat", ICH9LPCState, pm.smm_compat, false),
DEFINE_PROP_BOOL("smm-enabled", ICH9LPCState, pm.smm_enabled, false),
- DEFINE_PROP_BIT64("x-smi-broadcast", ICH9LPCState, smi_host_features,
- ICH9_LPC_SMI_F_BROADCAST_BIT, true),
DEFINE_PROP_BIT64("x-smi-cpu-hotplug", ICH9LPCState, smi_host_features,
ICH9_LPC_SMI_F_CPU_HOTPLUG_BIT, true),
DEFINE_PROP_BIT64("x-smi-cpu-hotunplug", ICH9LPCState, smi_host_features,
--
2.47.3
© 2016 - 2026 Red Hat, Inc.