On Fri, 12 Sep 2014 11:02:09 +0800
Gu Zheng <guz.f...@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.f...@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.f...@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;
}
>
> >
> >> + 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()
>
> 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
>
> >
> > .
> >
>
>
