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
