Re: [Qemu-devel] [PATCH v4 7/7] hw/i386/pc_q35: advertise broadcast SMI if VCPU hotplug is turned off

[Laszlo Ersek]

On 12/02/16 13:18, Igor Mammedov wrote:
> On Thu, 1 Dec 2016 21:42:58 +0100
> Laszlo Ersek <address@hidden> wrote:
> 
>> On 12/01/16 20:13, Eduardo Habkost wrote:
>>> On Thu, Dec 01, 2016 at 06:06:24PM +0100, Laszlo Ersek wrote:  
>>>> For the time being, we cannot handle SMIs in OVMF if VCPUs can show up
>>>> after boot. Otherwise, advertise ICH9_LPC_SMI_F_BROADCAST.
>>>>
>>>> Implement this generally, by introducing a new PCMachineClass method,
>>>> namely get_smi_host_features(), and implement the above logic for
>>>> pc-q35-2.9 and later. The idea is that future machine types might want to
>>>> calculate (the same or different) SMI host features from different
>>>> information, and that shouldn't affect earlier machine types.
>>>>
>>>> In turn, validating guest feature requests (inter-feature dependencies)
>>>> should be possible purely based on the available host feature set. For
>>>> example, in the future we might enforce that the guest select
>>>> ICH9_LPC_SMI_F_VCPU_PARKING as a prerequisite for
>>>> ICH9_LPC_SMI_F_BROADCAST, but only if the machine type itself advertises
>>>> ICH9_LPC_SMI_F_VCPU_PARKING.
>>>>
>>>> Cc: "Michael S. Tsirkin" <address@hidden>
>>>> Cc: Eduardo Habkost <address@hidden>
>>>> Cc: Gerd Hoffmann <address@hidden>
>>>> Cc: Igor Mammedov <address@hidden>
>>>> Cc: Paolo Bonzini <address@hidden>
>>>> Signed-off-by: Laszlo Ersek <address@hidden>
>>>> ---
>>>>  include/hw/i386/pc.h |  1 +
>>>>  hw/i386/pc_q35.c     | 24 +++++++++++++++++++++++-
>>>>  2 files changed, 24 insertions(+), 1 deletion(-)
>>>>
>>>> diff --git a/include/hw/i386/pc.h b/include/hw/i386/pc.h
>>>> index 430735e501dd..e164947116b6 100644
>>>> --- a/include/hw/i386/pc.h
>>>> +++ b/include/hw/i386/pc.h
>>>> @@ -116,10 +116,11 @@ struct PCMachineClass {
>>>>      /*< public >*/
>>>>  
>>>>      /* Methods: */
>>>>      HotplugHandler *(*get_hotplug_handler)(MachineState *machine,
>>>>                                             DeviceState *dev);
>>>> +    uint64_t (*get_smi_host_features)(void);  
>>>
>>> I'd prefer to encode the differences between machine-types as
>>> data, instead of code, to make introspection easier in the
>>> future. Is it possible to encode this in a simple PCMachineClass
>>> struct data field or (even bettter) a QOM property?
>>>   
>>
>> I don't know how else to capture the (smp_cpus == max_cpus) question,
>> for saying that "this machine type supports SMI broadcast, as long as
>> VCPU hotplug is disabled in the configuration".
> (smp_cpus == max_cpus) doesn't mean that CPU hotplug is disabled
> (it actually can't be disabled at all).
> 
> In addition, it's possible to start machine with
>  -smp 1,sockets=2,max_cpus=2 -device mycputype,socket=2
> 
> where all cpus will be coldpugged
> It would be better to use PCMachineState.boot_cpus which contains
> present cpus count.
> 
> I'd drop hotplug check (as usecase is broken in many places anyway)
> and even won't touch PCMachineState, instead add a property like
> ICH9_LPC.enable_smi_broadcast(on by default) and disable it for
> old machine types using compat props.

I'll look into that, thanks!
Laszlo

> 
> We can work on making CPU hotplug and OVMF work in follow up patches.
> 
>> Technically we could give PCMC two new (data) fields, a host features
>> bitmap for when VCPU hotplug is enabled, and another for when VCPU
>> hotplug is disabled. Then board code would take the right field and pass
>> it on to ich9_lpc_pm_init().
>>
>> Thanks
>> Laszlo
>