On 07/20/20 16:16, Igor Mammedov wrote:
> It will allow firmware to notify QEMU that firmware requires SMI
> being triggered on CPU hot[un]plug, so that it would be able to account
> for hotplugged CPU and relocate it to new SMM base and/or safely remove
> CPU on unplug.
>
> Using negotiated features, 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 | 2 ++
> hw/i386/pc.c | 5 ++++-
> hw/isa/lpc_ich9.c | 12 ++++++++++++
> 3 files changed, 18 insertions(+), 1 deletion(-)
>
> diff --git a/include/hw/i386/ich9.h b/include/hw/i386/ich9.h
> index a98d10b252..d1bb3f7bf0 100644
> --- a/include/hw/i386/ich9.h
> +++ b/include/hw/i386/ich9.h
> @@ -247,5 +247,7 @@ 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
> +#define ICH9_LPC_SMI_F_CPU_HOT_UNPLUG_BIT 2
>
> #endif /* HW_ICH9_H */
> diff --git a/hw/i386/pc.c b/hw/i386/pc.c
> index 3d419d5991..57d50fad6b 100644
> --- a/hw/i386/pc.c
> +++ b/hw/i386/pc.c
> @@ -97,7 +97,10 @@
> #include "fw_cfg.h"
> #include "trace.h"
>
> -GlobalProperty pc_compat_5_0[] = {};
> +GlobalProperty pc_compat_5_0[] = {
> + { "ICH9-LPC", "x-smi-cpu-hotplug", "off" },
> + { "ICH9-LPC", "x-smi-cpu-hotunplug", "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..c9305080b5 100644
> --- a/hw/isa/lpc_ich9.c
> +++ b/hw/isa/lpc_ich9.c
> @@ -373,6 +373,14 @@ 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) |
> + BIT_ULL(ICH9_LPC_SMI_F_CPU_HOT_UNPLUG_BIT))) {
> + /* cpu hot-[un]plug with 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 +755,10 @@ 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_BIT64("x-smi-cpu-hotunplug", ICH9LPCState, smi_host_features,
> + ICH9_LPC_SMI_F_CPU_HOT_UNPLUG_BIT, true),
> DEFINE_PROP_END_OF_LIST(),
> };
>
>
(1) I think that, at this time, the default value for
"x-smi-cpu-hotunplug" should be false. Accordingly, the
"x-smi-cpu-hotunplug" compat property should be dropped.
The reason is that in this series, QEMU won't actually learn to handle
CPU hot-unplug with SMI. We shouldn't advertize support for it.
We should only recognize the feature, so that the QMP command can rely
on it for rejecting hot-unplug attempts. If (later) we have a more
advanced OVMF binary with unplug support (so that OVMF would try to
negotiate unplug), but the user runs it on QEMU-5.1 (or more precisely
with an 5.1 machine type), which will support plug, but not unplug, then
QEMU should reject the device_del QMP command.
In brief, we need both properties because we want both device_add and
device_del to fail gracefully, but the default values (and accordingly
the compat properties) should reflect the actual feature support. So
unplug should default to false at this point.
Thanks,
Laszlo
