correcting myself a bit, with Drew and Thomas CC'd as well: On 08/19/20 10:39, Laszlo Ersek 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 <imamm...@redhat.com> >> Reviewed-by: Laszlo Ersek <ler...@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 >> <ler...@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.)
Sorry, I've just realized: the patch in question was originally written by Cornelia. And it's been on the list for a good time now -- it's been multiply reviewed, and also picked up in other series several times: Original posting, with reviews: http://mid.mail-archive.com/20200728094645.272149-1-cohuck@redhat.com Picked into another series (different from Daniel's): http://mid.mail-archive.com/20200805091640.11134-2-drjones@redhat.com Peter, would you please consider pushing that one patch directly? It seems to be a choke point. Thanks! Laszlo