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

[Cornelia Huck <cohuck@redhat.com>]

On Wed, 19 Aug 2020 10:39:04 +0200
Laszlo Ersek <lersek@redhat.com> wrote:

> 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
>   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.

I consider any patch that adds compat options without adding all compat
machines first to be buggy. We had that in the past, and it had been
painful to sort it out again later. That's why I usually post a compat
machines patch during hardfreeze, to be picked up by anyone who needs
it.

(See
https://lore.kernel.org/qemu-devel/20200728094645.272149-1-cohuck@redhat.com/
-- this is the patch that Daniel re-posted.)

> 
> 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.

Ack. Any series introducing compat options should have the machines
patch as the first patch; depending on merge order, it can then be
dropped again. (Usually, one of the architecture pull requests merges
it; we just don't have anything merged yet.)

> 
> 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.

Just pick my original patch, it has a bunch of acks already.

> 
> (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