| Message ID | 20200818122208.1243901-2-imammedo@redhat.com |
|---|---|
| State | New |
| Headers | show |
| Series | x86: fix cpu hotplug with secure boot | expand |
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. 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
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 <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. > > 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
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
On 08/19/20 10:58, Cornelia Huck wrote: > 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.) Indeed, thank you -- in the end, I did notice that Daniel's patch started with "From: Cornelia" :) and then I checked my qemu-devel folders for more emails with the same subject. > Just pick my original patch, it has a bunch of acks already. True, but Igor's series wouldn't apply without manual conflict resolution, and I can't do that if I want to respond with a Tested-by. (Of course, picking up your patch and including it in v3 is feasible for Igor.) Thanks! Laszlo
On Wed, 19 Aug 2020 11:14:45 +0200 Laszlo Ersek <lersek@redhat.com> wrote: > On 08/19/20 10:58, Cornelia Huck wrote: > > > 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.) > > Indeed, thank you -- in the end, I did notice that Daniel's patch > started with "From: Cornelia" :) and then I checked my qemu-devel > folders for more emails with the same subject. > > > Just pick my original patch, it has a bunch of acks already. > > True, but Igor's series wouldn't apply without manual conflict > resolution, and I can't do that if I want to respond with a Tested-by. > > (Of course, picking up your patch and including it in v3 is feasible for > Igor.) I'll rebase this path on top 5.2 machine patch and post it here. > > Thanks! > Laszlo
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(), };