| Message ID | 20200710161704.309824-2-imammedo@redhat.com |
|---|---|
| State | New |
| Headers | show |
| Series | x86: fix cpu hotplug with secure boot | expand |
On 07/10/20 18:17, Igor Mammedov wrote: > It will allow firmware to notify QEMU that firmware requires SMI > being triggered on CPU hotplug, so that it would be able to account > for hotplugged CPU and relocate it to new SMM base. > > Using the negotiated feature, 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 | 1 + > hw/i386/pc.c | 4 +++- > hw/isa/lpc_ich9.c | 7 +++++++ > 3 files changed, 11 insertions(+), 1 deletion(-) > > diff --git a/include/hw/i386/ich9.h b/include/hw/i386/ich9.h > index a98d10b252..422891a152 100644 > --- a/include/hw/i386/ich9.h > +++ b/include/hw/i386/ich9.h > @@ -247,5 +247,6 @@ 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 > > #endif /* HW_ICH9_H */ > diff --git a/hw/i386/pc.c b/hw/i386/pc.c > index d7f27bc16b..6fe80c84d7 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_0[] = {}; > +GlobalProperty pc_compat_5_0[] = { > + { "ICH9-LPC", "x-smi-cpu-hotplug", "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..83da7346ab 100644 > --- a/hw/isa/lpc_ich9.c > +++ b/hw/isa/lpc_ich9.c > @@ -373,6 +373,11 @@ 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)) { > + /* cpu hotplug 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 +752,8 @@ 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_END_OF_LIST(), > }; > > This patch looks good to me: Reviewed-by: Laszlo Ersek <lersek@redhat.com> However I'd suggest that we introduce ICH9_LPC_SMI_F_CPU_HOT_UNPLUG_BIT at once (bit position 2). We don't have to handle it for real, but we should catch it in patch#2, and reject it gracefully. For that, the property would be called "x-smi-cpu-hot-unplug", and the error check in smi_features_ok_callback() would go like: 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))) { /* either one of the "CPU hotplug with SMI" and "CPU hot-unplug * with SMI" features requires SMI broadcast; leave @features_ok * at zero */ return; } The reason I'm suggesting catching LPC_SMI_F_CPU_HOT_UNPLUG_BIT at once is that people will inevitably try to un-plug, when they see plug succeed. The firmware itself does catch the (as yet unimplemented) un-plug attempt, but the way the firmware fails is not -- cannot be -- graceful. It hangs, intentionally. Failing the device_del QMP command is more graceful. (For this series, I'll provide test results once I've read the patches.) Thanks! Laszlo
diff --git a/include/hw/i386/ich9.h b/include/hw/i386/ich9.h index a98d10b252..422891a152 100644 --- a/include/hw/i386/ich9.h +++ b/include/hw/i386/ich9.h @@ -247,5 +247,6 @@ 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 #endif /* HW_ICH9_H */ diff --git a/hw/i386/pc.c b/hw/i386/pc.c index d7f27bc16b..6fe80c84d7 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_0[] = {}; +GlobalProperty pc_compat_5_0[] = { + { "ICH9-LPC", "x-smi-cpu-hotplug", "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..83da7346ab 100644 --- a/hw/isa/lpc_ich9.c +++ b/hw/isa/lpc_ich9.c @@ -373,6 +373,11 @@ 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)) { + /* cpu hotplug 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 +752,8 @@ 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_END_OF_LIST(), };
It will allow firmware to notify QEMU that firmware requires SMI being triggered on CPU hotplug, so that it would be able to account for hotplugged CPU and relocate it to new SMM base. Using the negotiated feature, 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 | 1 + hw/i386/pc.c | 4 +++- hw/isa/lpc_ich9.c | 7 +++++++ 3 files changed, 11 insertions(+), 1 deletion(-)