On 07/10/20 18:17, Igor Mammedov wrote: > It will allow firmware to notify QEMU that firmware requires SMI > being triggered on CPU hotplug, so that it would be able to account > for hotplugged CPU and relocate it to new SMM base. > > Using the negotiated feature, follow up patches will insert SMI upcall > into AML code, to make sure that firmware processes hotplug before > guest OS would attempt to use new CPU. > > Signed-off-by: Igor Mammedov <imamm...@redhat.com> > --- > include/hw/i386/ich9.h | 1 + > hw/i386/pc.c | 4 +++- > hw/isa/lpc_ich9.c | 7 +++++++ > 3 files changed, 11 insertions(+), 1 deletion(-) > > diff --git a/include/hw/i386/ich9.h b/include/hw/i386/ich9.h > index a98d10b252..422891a152 100644 > --- a/include/hw/i386/ich9.h > +++ b/include/hw/i386/ich9.h > @@ -247,5 +247,6 @@ typedef struct ICH9LPCState { > > /* bit positions used in fw_cfg SMI feature negotiation */ > #define ICH9_LPC_SMI_F_BROADCAST_BIT 0 > +#define ICH9_LPC_SMI_F_CPU_HOTPLUG_BIT 1 > > #endif /* HW_ICH9_H */ > diff --git a/hw/i386/pc.c b/hw/i386/pc.c > index d7f27bc16b..6fe80c84d7 100644 > --- a/hw/i386/pc.c > +++ b/hw/i386/pc.c > @@ -97,7 +97,9 @@ > #include "fw_cfg.h" > #include "trace.h" > > -GlobalProperty pc_compat_5_0[] = {}; > +GlobalProperty pc_compat_5_0[] = { > + { "ICH9-LPC", "x-smi-cpu-hotplug", "off" }, > +}; > const size_t pc_compat_5_0_len = G_N_ELEMENTS(pc_compat_5_0); > > GlobalProperty pc_compat_4_2[] = { > diff --git a/hw/isa/lpc_ich9.c b/hw/isa/lpc_ich9.c > index cd6e169d47..83da7346ab 100644 > --- a/hw/isa/lpc_ich9.c > +++ b/hw/isa/lpc_ich9.c > @@ -373,6 +373,11 @@ static void smi_features_ok_callback(void *opaque) > /* guest requests invalid features, leave @features_ok at zero */ > return; > } > + if (!(guest_features & BIT_ULL(ICH9_LPC_SMI_F_BROADCAST_BIT)) && > + guest_features & BIT_ULL(ICH9_LPC_SMI_F_CPU_HOTPLUG_BIT)) { > + /* cpu hotplug SMI requires SMI broadcast, leave @features_ok at > zero */ > + return; > + } > > /* valid feature subset requested, lock it down, report success */ > lpc->smi_negotiated_features = guest_features; > @@ -747,6 +752,8 @@ static Property ich9_lpc_properties[] = { > DEFINE_PROP_BOOL("noreboot", ICH9LPCState, pin_strap.spkr_hi, true), > 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_END_OF_LIST(), > }; > >
This patch looks good to me: Reviewed-by: Laszlo Ersek <ler...@redhat.com> However I'd suggest that we introduce ICH9_LPC_SMI_F_CPU_HOT_UNPLUG_BIT at once (bit position 2). We don't have to handle it for real, but we should catch it in patch#2, and reject it gracefully. For that, the property would be called "x-smi-cpu-hot-unplug", and the error check in smi_features_ok_callback() would go like: if (!(guest_features & BIT_ULL(ICH9_LPC_SMI_F_BROADCAST_BIT)) && guest_features & (BIT_ULL(ICH9_LPC_SMI_F_CPU_HOTPLUG_BIT) | BIT_ULL(ICH9_LPC_SMI_F_CPU_HOT_UNPLUG_BIT))) { /* either one of the "CPU hotplug with SMI" and "CPU hot-unplug * with SMI" features requires SMI broadcast; leave @features_ok * at zero */ return; } The reason I'm suggesting catching LPC_SMI_F_CPU_HOT_UNPLUG_BIT at once is that people will inevitably try to un-plug, when they see plug succeed. The firmware itself does catch the (as yet unimplemented) un-plug attempt, but the way the firmware fails is not -- cannot be -- graceful. It hangs, intentionally. Failing the device_del QMP command is more graceful. (For this series, I'll provide test results once I've read the patches.) Thanks! Laszlo