| Message ID | 20200720141610.574308-2-imammedo@redhat.com |
|---|---|
| State | New |
| Headers | show |
| Series | x86: fix cpu hotplug with secure boot | expand |
On 07/20/20 16:16, 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> > --- > include/hw/i386/ich9.h | 2 ++ > hw/i386/pc.c | 5 ++++- > hw/isa/lpc_ich9.c | 12 ++++++++++++ > 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 3d419d5991..57d50fad6b 100644 > --- a/hw/i386/pc.c > +++ b/hw/i386/pc.c > @@ -97,7 +97,10 @@ > #include "fw_cfg.h" > #include "trace.h" > > -GlobalProperty pc_compat_5_0[] = {}; > +GlobalProperty pc_compat_5_0[] = { > + { "ICH9-LPC", "x-smi-cpu-hotplug", "off" }, > + { "ICH9-LPC", "x-smi-cpu-hotunplug", "off" }, > +}; > const size_t pc_compat_5_0_len = G_N_ELEMENTS(pc_compat_5_0); > > GlobalProperty pc_compat_4_2[] = { > diff --git a/hw/isa/lpc_ich9.c b/hw/isa/lpc_ich9.c > index cd6e169d47..c9305080b5 100644 > --- a/hw/isa/lpc_ich9.c > +++ b/hw/isa/lpc_ich9.c > @@ -373,6 +373,14 @@ 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 +755,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, true), > DEFINE_PROP_END_OF_LIST(), > }; > > (1) I think that, at this time, the default value for "x-smi-cpu-hotunplug" should be false. Accordingly, the "x-smi-cpu-hotunplug" compat property should be dropped. The reason is that in this series, QEMU won't actually learn to handle CPU hot-unplug with SMI. We shouldn't advertize support for it. We should only recognize the feature, so that the QMP command can rely on it for rejecting hot-unplug attempts. If (later) we have a more advanced OVMF binary with unplug support (so that OVMF would try to negotiate unplug), but the user runs it on QEMU-5.1 (or more precisely with an 5.1 machine type), which will support plug, but not unplug, then QEMU should reject the device_del QMP command. In brief, we need both properties because we want both device_add and device_del to fail gracefully, but the default values (and accordingly the compat properties) should reflect the actual feature support. So unplug should default to false at this point. Thanks, Laszlo
On 07/22/20 14:03, Laszlo Ersek wrote: > On 07/20/20 16:16, 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> >> --- >> include/hw/i386/ich9.h | 2 ++ >> hw/i386/pc.c | 5 ++++- >> hw/isa/lpc_ich9.c | 12 ++++++++++++ >> 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 3d419d5991..57d50fad6b 100644 >> --- a/hw/i386/pc.c >> +++ b/hw/i386/pc.c >> @@ -97,7 +97,10 @@ >> #include "fw_cfg.h" >> #include "trace.h" >> >> -GlobalProperty pc_compat_5_0[] = {}; >> +GlobalProperty pc_compat_5_0[] = { >> + { "ICH9-LPC", "x-smi-cpu-hotplug", "off" }, >> + { "ICH9-LPC", "x-smi-cpu-hotunplug", "off" }, >> +}; >> const size_t pc_compat_5_0_len = G_N_ELEMENTS(pc_compat_5_0); >> >> GlobalProperty pc_compat_4_2[] = { >> diff --git a/hw/isa/lpc_ich9.c b/hw/isa/lpc_ich9.c >> index cd6e169d47..c9305080b5 100644 >> --- a/hw/isa/lpc_ich9.c >> +++ b/hw/isa/lpc_ich9.c >> @@ -373,6 +373,14 @@ 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 +755,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, true), >> DEFINE_PROP_END_OF_LIST(), >> }; >> >> > > (1) I think that, at this time, the default value for > "x-smi-cpu-hotunplug" should be false. Accordingly, the > "x-smi-cpu-hotunplug" compat property should be dropped. > > The reason is that in this series, QEMU won't actually learn to handle > CPU hot-unplug with SMI. We shouldn't advertize support for it. > > We should only recognize the feature, so that the QMP command can rely > on it for rejecting hot-unplug attempts. If (later) we have a more > advanced OVMF binary with unplug support (so that OVMF would try to > negotiate unplug), but the user runs it on QEMU-5.1 (or more precisely > with an 5.1 machine type), which will support plug, but not unplug, then > QEMU should reject the device_del QMP command. > > In brief, we need both properties because we want both device_add and > device_del to fail gracefully, but the default values (and accordingly > the compat properties) should reflect the actual feature support. So > unplug should default to false at this point. With (1) updated: Reviewed-by: Laszlo Ersek <lersek@redhat.com>
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 3d419d5991..57d50fad6b 100644 --- a/hw/i386/pc.c +++ b/hw/i386/pc.c @@ -97,7 +97,10 @@ #include "fw_cfg.h" #include "trace.h" -GlobalProperty pc_compat_5_0[] = {}; +GlobalProperty pc_compat_5_0[] = { + { "ICH9-LPC", "x-smi-cpu-hotplug", "off" }, + { "ICH9-LPC", "x-smi-cpu-hotunplug", "off" }, +}; const size_t pc_compat_5_0_len = G_N_ELEMENTS(pc_compat_5_0); GlobalProperty pc_compat_4_2[] = { diff --git a/hw/isa/lpc_ich9.c b/hw/isa/lpc_ich9.c index cd6e169d47..c9305080b5 100644 --- a/hw/isa/lpc_ich9.c +++ b/hw/isa/lpc_ich9.c @@ -373,6 +373,14 @@ 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 +755,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, true), DEFINE_PROP_END_OF_LIST(), };
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> --- include/hw/i386/ich9.h | 2 ++ hw/i386/pc.c | 5 ++++- hw/isa/lpc_ich9.c | 12 ++++++++++++ 3 files changed, 18 insertions(+), 1 deletion(-)