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

[Laszlo Ersek]

On 08/24/20 13:00, 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>
> ---
> v4:
>   - fix 5.2 machine types so they won't apply pc_compat_5_1
>      (Laszlo Ersek <lersek@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>)
>
> fixup
> ---
>  include/hw/i386/ich9.h |  2 ++
>  hw/i386/pc.c           |  4 +++-
>  hw/isa/lpc_ich9.c      | 13 +++++++++++++
>  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 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/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 doesn't apply with git-am, as of commit 44423107e7b5 ("Merge
remote-tracking branch 'remotes/xtensa/tags/20200821-xtensa' into
staging", 2020-08-24).

The reason is that commit 2becc36a3e53 ("meson: infrastructure for
building emulators", 2020-08-21) introduced

#include CONFIG_DEVICES

to "hw/i386/pc.c", just above the (then) "pc_compat_5_0" array.

Then Cornelia's commit 3ff3c5d31740 ("hw: add compat machines for 5.2",
2020-08-19), which introduced "pc_compat_5_1" independently of the Meson
conversion, needed explicit resolution against CONFIG_DEVICES for
merging into master. That was covered in merge commit ca489cd037e4
("Merge remote-tracking branch
'remotes/ehabkost/tags/machine-next-pull-request' into staging",
2020-08-22).

So the patch applies on top of 3ff3c5d31740, but not on the merge
(ca489cd037e4) that brought 3ff3c5d31740 into master.

Not a problem though: I can apply the patch on 3ff3c5d31740, and then
cleanly (automatically) rebase to current HEAD (44423107e7b5) from
there. This is the range-diff across the rebase:

> 1:  9066fa4ccb49 ! 1:  05188fffe6aa x86: lpc9: let firmware negotiate 'CPU 
> hotplug with SMI' features
>     @@ -31,8 +31,8 @@
>      --- a/hw/i386/pc.c
>      +++ b/hw/i386/pc.c
>      @@
>     - #include "fw_cfg.h"
>       #include "trace.h"
>     + #include CONFIG_DEVICES
>
>      -GlobalProperty pc_compat_5_1[] = {};
>      +GlobalProperty pc_compat_5_1[] = {


So it's indeed just a context update.

Having applied this series to QEMU, my test results are:

  OVMF has 5ba203b54e59  machine type  plug gate  unplug gate  gating result
  ---------------------  ------------  ---------  -----------  -------------
  no                     pc-q35-5.1    reject     reject       pass
  no                     pc-q35-5.2    reject     reject       pass
  yes                    pc-q35-5.1    reject     reject       pass
  yes                    pc-q35-5.2    accept     reject       pass

The results are the same after S3 suspend/resume. (This is relevant
because the features are re-negotiated during S3 resume.)

Thus, for this one patch, so far:

- (just to confirm:)
Reviewed-by: Laszlo Ersek <lersek@redhat.com>

- also:
Tested-by: Laszlo Ersek <lersek@redhat.com>

Thanks!
Laszlo