Re: [PATCH 1/6] x86: lpc9: let firmware negotiate 'CPU hotplug with SMI' features

[Laszlo Ersek]

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>