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

[Laszlo Ersek]

Hi Igor,

(CC'ing Daniel, Cornelia, David, Peter)

On 08/18/20 14:22, 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>
> Reviewed-by: Laszlo Ersek <lersek@redhat.com>
> ---
> 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 ++
>  include/hw/i386/pc.h   |  3 +++
>  hw/i386/pc.c           |  6 +++++-
>  hw/i386/pc_piix.c      |  1 +
>  hw/i386/pc_q35.c       |  1 +
>  hw/isa/lpc_ich9.c      | 13 +++++++++++++
>  6 files changed, 25 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/include/hw/i386/pc.h b/include/hw/i386/pc.h
> index 3d7ed3a55e..fe52e165b2 100644
> --- a/include/hw/i386/pc.h
> +++ b/include/hw/i386/pc.h
> @@ -193,6 +193,9 @@ void pc_system_firmware_init(PCMachineState *pcms, 
> MemoryRegion *rom_memory);
>  void pc_madt_cpu_entry(AcpiDeviceIf *adev, int uid,
>                         const CPUArchIdList *apic_ids, GArray *entry);
>
> +extern GlobalProperty pc_compat_5_1[];
> +extern const size_t pc_compat_5_1_len;
> +
>  extern GlobalProperty pc_compat_5_0[];
>  extern const size_t pc_compat_5_0_len;
>
> diff --git a/hw/i386/pc.c b/hw/i386/pc.c
> index 47c5ca3e34..99c6bdbab4 100644
> --- a/hw/i386/pc.c
> +++ b/hw/i386/pc.c
> @@ -97,8 +97,12 @@
>  #include "fw_cfg.h"
>  #include "trace.h"
>
> -GlobalProperty pc_compat_5_0[] = {
> +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[] = { };
>  const size_t pc_compat_5_0_len = G_N_ELEMENTS(pc_compat_5_0);
>
>  GlobalProperty pc_compat_4_2[] = {
> diff --git a/hw/i386/pc_piix.c b/hw/i386/pc_piix.c
> index b789e83f9a..d56f2e1b96 100644
> --- a/hw/i386/pc_piix.c
> +++ b/hw/i386/pc_piix.c
> @@ -433,6 +433,7 @@ static void pc_i440fx_5_1_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_1, "pc-i440fx-5.1", NULL,
> diff --git a/hw/i386/pc_q35.c b/hw/i386/pc_q35.c
> index a3e607a544..0ca1146a59 100644
> --- a/hw/i386/pc_q35.c
> +++ b/hw/i386/pc_q35.c
> @@ -359,6 +359,7 @@ static void pc_q35_5_1_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_1, "pc-q35-5.1", NULL,
> 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(),
>  };
>
>

this patch does the right thing for the 5.1 PC machine types, but it
does not introduce any 5.2 machine types, so I can't enable the hotplug
feature bit without messing with the x-smi-cpu-hotplug property
manually.

Now... looking at the mailing list, I can see the following patch
pending review (posted by Daniel ~5 days ago):

  [PATCH 01/10] hw: add compat machines for 5.2
  20200814205424.543857-2-danielhb413@gmail.com">http://mid.mail-archive.com/20200814205424.543857-2-danielhb413@gmail.com

That patch introduces the 5.2 PC machine types.

However, I can't just apply your series on top of that one patch,
because they conflict at least on the "pc_compat_5_1" array.

Given that Daniel's patch was posted before yours, and also that
Daniel's patch introduces 5.2 machine types for arm, ppc and s390x too
(and the "hw_compat_5_1" array in addition to the "pc_compat_5_1"
array), I'd like to request:

- that we please review and/or merge Daniel's patch in isolation (just
  the first patch in the containing series, not the entire series!),

- and that you please rebase your series on top of Daniel's patch.

If Daniel's patch is approved as-is on the list, then I'm OK applying it
locally in isolation from the rest of the containing series, as a basis
for locally applying your v3.

(I could resolve the conflicts myself at once I guess, and proceed with
the review / testing -- but that's not really useful if I want to give a
formal Tested-by. I wouldn't be testing the patches as they were
posted.)

Thanks!
Laszlo