Re: [PATCH 1/6] x86: lpc9: let firmware negotiate 'CPU hotplug with SMI' features

[Laszlo Ersek]

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 <imammedo@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