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

[Igor Mammedov <imammedo@redhat.com>]

On Mon, 15 Sep 2014 11:57:56 +0800
Gu Zheng <guz.fnst@cn.fujitsu.com> wrote:

> Hi Igor,
> 
> On 09/12/2014 10:28 PM, Igor Mammedov wrote:
> 
> > On Fri, 12 Sep 2014 11:02:09 +0800
> > Gu Zheng <guz.fnst@cn.fujitsu.com> wrote:
> > 
> >> Hi Igor,
> >> On 09/10/2014 09:55 PM, Igor Mammedov wrote:
> >>
> >>> On Wed,  3 Sep 2014 17:06:16 +0800
> >>> Gu Zheng <guz.fnst@cn.fujitsu.com> 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 <guz.fnst@cn.fujitsu.com>
> >>>> ---
> >>>>  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.
I'm sorry, I've forgot about default machine hotplug handler.

> Shall we still need to set hotplug handler ICCBus, which seems duplicated?
> Or switch to set hotplug handler to ICCBus, and drop current implementation?
Taking in account that we should access RTC device from handler,
machine's hotplug handler serves purpose just fine.

> 
> 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
> >>
> >>>
> >>> .
> >>>
> >>
> >>
> > 
> > .
> > 
> 
>