Re: [RFC 1/3] x86: lpc9: let firmware negotiate CPU hotplug SMI feature

[Laszlo Ersek <lersek@redhat.com>]

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