On 12/02/16 13:18, Igor Mammedov wrote:
> On Thu, 1 Dec 2016 21:42:58 +0100
> Laszlo Ersek <ler...@redhat.com> 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" <m...@redhat.com>
>>>> Cc: Eduardo Habkost <ehabk...@redhat.com>
>>>> Cc: Gerd Hoffmann <kra...@redhat.com>
>>>> Cc: Igor Mammedov <imamm...@redhat.com>
>>>> Cc: Paolo Bonzini <pbonz...@redhat.com>
>>>> Signed-off-by: Laszlo Ersek <ler...@redhat.com>
>>>> ---
>>>> 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
>
