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 <imamm...@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 <ler...@redhat.com>
