On Mon, 29 Sep 2014 10:58:04 +0800
Gu Zheng <guz.f...@cn.fujitsu.com> wrote:
> Hi Igor,
> On 09/26/2014 09:06 PM, Igor Mammedov wrote:
>
> > On Wed, 17 Sep 2014 09:24:00 +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.
> >>
> >> v3:
> >> -deal with start up cpus in a more neat way as Igor suggested.
> >> v2:
> >> -just rebase.
> >>
> >> Signed-off-by: Gu Zheng <guz.f...@cn.fujitsu.com>
> >> ---
> >> hw/i386/pc.c | 26 +++++++++++++++++++++++++-
> >> 1 files changed, 25 insertions(+), 1 deletions(-)
> >>
> >> diff --git a/hw/i386/pc.c b/hw/i386/pc.c
> >> index b6c9b61..e25e71b 100644
> >> --- a/hw/i386/pc.c
> >> +++ b/hw/i386/pc.c
> >> @@ -1613,11 +1613,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 (!pcms->acpi_dev) {
> >> + if (dev->hotplugged) {
> >> + 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);
> > above call this would rise SCI unconditionally whether CPU is
> > hotplugged or not.
>
> Above precheck can avoid this, startup CPUs won't run into this:
> if (!pcms->acpi_dev) {
> if (dev->hotplugged) {
> error_setg(&local_err,
> "cpu hotplug is not enabled: missing acpi
> device"); }
> goto out;
> }
that's true, but it robs plug handler of initializing startup CPUs'
state in acpi_dev and leads to code duplication between
AcpiCpuHotplug_init() and acpi_cpu_plug_cb().
>
> Thanks,
> Gu
>
> > perhaps checking for this in acpi_cpu_plug_cb() and rising IRQ only
> > for hotplugged CPUs would be better.
> >
> >> +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);
> >> }
> >> }
> >>
> >> @@ -1626,7 +1649,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);
> >> }
> >>
> >
> > .
> >
>
>
