Re: [Qemu-devel] [PATCH 4/5] pc: add cpu hotplug handler to PC_MACHINE

[Gu Zheng]

Hi Igor,

On 09/12/2014 10:28 PM, Igor Mammedov wrote:

> On Fri, 12 Sep 2014 11:02:09 +0800
> Gu Zheng <address@hidden> wrote:
> 
>> Hi Igor,
>> On 09/10/2014 09:55 PM, Igor Mammedov wrote:
>>
>>> On Wed,  3 Sep 2014 17:06:16 +0800
>>> Gu Zheng <address@hidden> wrote:
>>>
>>>> Add cpu hotplug handler to PC_MACHINE, which will perform the acpi
>>>> cpu hotplug callback via hotplug_handler API.
>>>>
>>>> Signed-off-by: Gu Zheng <address@hidden>
>>>> ---
>>>>  hw/i386/pc.c |   26 +++++++++++++++++++++++++-
>>>>  qom/cpu.c    |    1 -
>>>>  2 files changed, 25 insertions(+), 2 deletions(-)
>>>>
>>>> diff --git a/hw/i386/pc.c b/hw/i386/pc.c
>>>> index 8fa8d2f..c2956f9 100644
>>>> --- a/hw/i386/pc.c
>>>> +++ b/hw/i386/pc.c
>>>> @@ -1607,11 +1607,34 @@ out:
>>>>      error_propagate(errp, local_err);
>>>>  }
>>>>  
>>>> +static void pc_cpu_plug(HotplugHandler *hotplug_dev,
>>>> +                         DeviceState *dev, Error **errp)
>>>> +{
>>>> +    HotplugHandlerClass *hhc;
>>>> +    Error *local_err = NULL;
>>>> +    PCMachineState *pcms = PC_MACHINE(hotplug_dev);
>>>> +
>>>> +    if (dev->hotplugged) {
>>> for startup CPUs we set gpe_cpu status bits in AcpiCpuHotplug_init()
>>> and for hotpluged in AcpiCpuHotplug_add() which is duplicating
>>> essentially the same code.
>>>
>>> Could you drop above check and make AcpiCpuHotplug_init() take care
>>> about startup CPUs as well, keeping bit setting in one place.
>>
>> Yeah, with the above change, code will be more neat, but I think it is not
>> a serious problem.:)
>> And pcms->acpi_dev is set when PC hardware initialisation (e.g. pc_init1),
>> for start up cpus, the plug handler is not valid, so the check is needed
>> here to avoid trigger error by start cpu.
> 
> then check for !pcms->acpi_dev and exit if it not set (i.e. as pc_dimm_plug() 
> does)
> since there isn't acpi hardware to update for startup CPUs either.
> 
> may be something along this lines:
> 
> if (!pcms->acpi_dev) {
>   if (dev->hotplugged) error_setg(&local_err, "CPU hotplug is not supported 
> without ACPI");
>   goto out;
> }

This is more neat, thanks.

> 
>>
>>>
>>>> +        if (!pcms->acpi_dev) {
>>>> +            error_setg(&local_err,
>>>> +                   "cpu hotplug is not enabled: missing acpi device");
>>>> +            goto out;
>>>> +        }
>>>> +
>>>> +        hhc = HOTPLUG_HANDLER_GET_CLASS(pcms->acpi_dev);
>>>> +        hhc->plug(HOTPLUG_HANDLER(pcms->acpi_dev), dev, &local_err);
>>>> +out:
>>>> +        error_propagate(errp, local_err);
>>>> +    }
>>>> +}
>>>> +
>>>>  static void pc_machine_device_plug_cb(HotplugHandler *hotplug_dev,
>>>>                                        DeviceState *dev, Error **errp)
>>>>  {
>>>>      if (object_dynamic_cast(OBJECT(dev), TYPE_PC_DIMM)) {
>>>>          pc_dimm_plug(hotplug_dev, dev, errp);
>>>> +    } else if (object_dynamic_cast(OBJECT(dev), TYPE_CPU)) {
>>>> +        pc_cpu_plug(hotplug_dev, dev, errp);
>>>>      }
>>>>  }
>>>>  
>>>> @@ -1620,7 +1643,8 @@ static HotplugHandler 
>>>> *pc_get_hotpug_handler(MachineState *machine,
>>>>  {
>>>>      PCMachineClass *pcmc = PC_MACHINE_GET_CLASS(machine);
>>>>  
>>>> -    if (object_dynamic_cast(OBJECT(dev), TYPE_PC_DIMM)) {
>>>> +    if (object_dynamic_cast(OBJECT(dev), TYPE_PC_DIMM)
>>>> +        || object_dynamic_cast(OBJECT(dev), TYPE_CPU)) {
>>>>          return HOTPLUG_HANDLER(machine);
>>>>      }
>>>>  
>>>> diff --git a/qom/cpu.c b/qom/cpu.c
>>>> index b32dd0a..af8e83f 100644
>>>> --- a/qom/cpu.c
>>>> +++ b/qom/cpu.c
>>>> @@ -304,7 +304,6 @@ static void cpu_common_realizefn(DeviceState *dev, 
>>>> Error **errp)
>>>>      if (dev->hotplugged) {
>>>>          cpu_synchronize_post_init(cpu);
>>>>          notifier_list_notify(&cpu_added_notifiers, dev);
>>>> -        cpu_resume(cpu);
>>>>      }
>>>>  }
>>>>  
>>>
>>> I don't see what sets PCMachine as hotplug_handler for CPU,
>>> maybe series is missing a patch?
>>
>> Previous memory hotplug has built the frame work, here just adding the case
>> to handle CPU.
> You still need to set hotplug handler for CPU device or ICCBus
> grep for qbus_set_hotplug_handler()

Current implementation is the same as bus-less device's (e.g. memory), via the
machine's get_hotplug_handler method to discover a hotplug handler controller,
and it works fine.
Shall we still need to set hotplug handler ICCBus, which seems duplicated?
Or switch to set hotplug handler to ICCBus, and drop current implementation?

Thanks,
Gu

> 
>>
>>  static void pc_machine_device_plug_cb(HotplugHandler *hotplug_dev,
>>                                        DeviceState *dev, Error **errp)
>>  {
>>      if (object_dynamic_cast(OBJECT(dev), TYPE_PC_DIMM)) {
>>          pc_dimm_plug(hotplug_dev, dev, errp);
>> +    } else if (object_dynamic_cast(OBJECT(dev), TYPE_CPU)) {
>> +        pc_cpu_plug(hotplug_dev, dev, errp);
>>      }
>>
>> Thanks,
>> Gu
>>
>>>
>>> .
>>>
>>
>>
> 
> .
>