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

[Laszlo Ersek <lersek@redhat.com>]

Hi Igor,

On 08/20/20 16:56, 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>
> ---
> v3:
>   - rebase on top of "[PATCH v2] hw: add compat machines for 5.2"
>     so apply that before this patch
> v2:
>   - rebase on top of 5.1 (move compat values to 5.1 machine)
>   - make "x-smi-cpu-hotunplug" false by default (Laszlo Ersek <lersek@redhat.com>)
> ---
>  include/hw/i386/ich9.h |  2 ++
>  hw/i386/pc.c           |  4 +++-
>  hw/i386/pc_piix.c      |  1 +
>  hw/i386/pc_q35.c       |  1 +
>  hw/isa/lpc_ich9.c      | 13 +++++++++++++
>  5 files changed, 20 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 9aa813949c..583db11d28 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_1[] = {};
> +GlobalProperty pc_compat_5_1[] = {
> +    { "ICH9-LPC", "x-smi-cpu-hotplug", "off" },
> +};
>  const size_t pc_compat_5_1_len = G_N_ELEMENTS(pc_compat_5_1);
>  
>  GlobalProperty pc_compat_5_0[] = {
> diff --git a/hw/i386/pc_piix.c b/hw/i386/pc_piix.c
> index c5ba70ca17..68f8ba1bf9 100644
> --- a/hw/i386/pc_piix.c
> +++ b/hw/i386/pc_piix.c
> @@ -433,6 +433,7 @@ static void pc_i440fx_5_2_machine_options(MachineClass *m)
>      m->alias = "pc";
>      m->is_default = true;
>      pcmc->default_cpu_version = 1;
> +    compat_props_add(m->compat_props, pc_compat_5_1, pc_compat_5_1_len);
>  }
>  
>  DEFINE_I440FX_MACHINE(v5_2, "pc-i440fx-5.2", NULL,
> diff --git a/hw/i386/pc_q35.c b/hw/i386/pc_q35.c
> index 0cb9c18cd4..b729cf9a58 100644
> --- a/hw/i386/pc_q35.c
> +++ b/hw/i386/pc_q35.c
> @@ -359,6 +359,7 @@ static void pc_q35_5_2_machine_options(MachineClass *m)
>      pc_q35_machine_options(m);
>      m->alias = "q35";
>      pcmc->default_cpu_version = 1;
> +    compat_props_add(m->compat_props, pc_compat_5_1, pc_compat_5_1_len);
>  }
>  
>  DEFINE_Q35_MACHINE(v5_2, "pc-q35-5.2", NULL,

Sorry about the late response, I was away yesterday (public holiday in
my country).

The above two hunks should be dropped.

Cornelia's patch already handles both of:
- hw_compat_5_1,
- pc_compat_5_1.
in both of:
- pc_i440fx_5_1_machine_options(),
- pc_q35_5_1_machine_options().

So you don't need to modify those functions any longer.

Furthermore, the machine-options functions for 5.2 machine types should
not be modified *at all*. The effect of the above two hunks is that the
5.2 machine types will inherit the 5.1 compat props (such as

  ICH9-LPC.x-smi-cpu-hotplug=off

) and we do not want that.

So please drop the above two hunks.

If you could post a v4 of this patch today, that would be awesome; I
would like to play with this series extensively still today. "tonight"
is definitely an option, so if you can post the patch in the evening
(CEST), that's still great from my POV!

Thanks!
Laszlo

> diff --git a/hw/isa/lpc_ich9.c b/hw/isa/lpc_ich9.c
> index cd6e169d47..19f32bed3e 100644
> --- a/hw/isa/lpc_ich9.c
> +++ b/hw/isa/lpc_ich9.c
> @@ -373,6 +373,15 @@ 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 +756,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, false),
>      DEFINE_PROP_END_OF_LIST(),
>  };
>  
>