| Message ID | 1468515285-173356-5-git-send-email-imammedo@redhat.com (mailing list archive) |
|---|---|
| State | New, archived |
| Headers | show |
Igor Mammedov <imammedo@redhat.com> writes: > BSP is assumed to always present in QEMU code, so > untile that assumptions are gone, deny removal request. > In another words QEMU won't support BSP hot-unplug. > > Signed-off-by: Igor Mammedov <imammedo@redhat.com> > --- > hw/i386/pc.c | 7 +++++++ > 1 file changed, 7 insertions(+) > > diff --git a/hw/i386/pc.c b/hw/i386/pc.c > index 5a67f15..33c5f97 100644 > --- a/hw/i386/pc.c > +++ b/hw/i386/pc.c > @@ -1751,10 +1751,17 @@ out: > static void pc_cpu_unplug_request_cb(HotplugHandler *hotplug_dev, > DeviceState *dev, Error **errp) > { > + int idx; > HotplugHandlerClass *hhc; > Error *local_err = NULL; > PCMachineState *pcms = PC_MACHINE(hotplug_dev); > > + pc_find_cpu_slot(pcms, CPU(dev), &idx); > + if (idx == 0) { > + error_setg(&local_err, "1st CPU (BSP) is unpluggable"); > + goto out; > + } Nit: Boot CPU or simply Bootstrap Processor sounds better IMO. > hhc = HOTPLUG_HANDLER_GET_CLASS(pcms->acpi_dev); > hhc->unplug_request(HOTPLUG_HANDLER(pcms->acpi_dev), dev, &local_err);
On Thu, Jul 14, 2016 at 06:54:33PM +0200, Igor Mammedov wrote: > BSP is assumed to always present in QEMU code, so > untile that assumptions are gone, deny removal request. > In another words QEMU won't support BSP hot-unplug. > > Signed-off-by: Igor Mammedov <imammedo@redhat.com> > --- > hw/i386/pc.c | 7 +++++++ > 1 file changed, 7 insertions(+) > > diff --git a/hw/i386/pc.c b/hw/i386/pc.c > index 5a67f15..33c5f97 100644 > --- a/hw/i386/pc.c > +++ b/hw/i386/pc.c > @@ -1751,10 +1751,17 @@ out: > static void pc_cpu_unplug_request_cb(HotplugHandler *hotplug_dev, > DeviceState *dev, Error **errp) > { > + int idx; > HotplugHandlerClass *hhc; > Error *local_err = NULL; > PCMachineState *pcms = PC_MACHINE(hotplug_dev); > > + pc_find_cpu_slot(pcms, CPU(dev), &idx); Looks fragile: if one day we create any TYPE_CPU object that is not in possible_cpus array, idx is undefined. I suggest initializing idx to -1 above. I can change it when committing, if that's OK for you. > + if (idx == 0) { > + error_setg(&local_err, "1st CPU (BSP) is unpluggable"); > + goto out; > + } > + > hhc = HOTPLUG_HANDLER_GET_CLASS(pcms->acpi_dev); > hhc->unplug_request(HOTPLUG_HANDLER(pcms->acpi_dev), dev, &local_err); > > -- > 2.7.4 >
Eduardo Habkost <ehabkost@redhat.com> writes: > On Thu, Jul 14, 2016 at 06:54:33PM +0200, Igor Mammedov wrote: >> BSP is assumed to always present in QEMU code, so >> untile that assumptions are gone, deny removal request. >> In another words QEMU won't support BSP hot-unplug. >> >> Signed-off-by: Igor Mammedov <imammedo@redhat.com> >> --- >> hw/i386/pc.c | 7 +++++++ >> 1 file changed, 7 insertions(+) >> >> diff --git a/hw/i386/pc.c b/hw/i386/pc.c >> index 5a67f15..33c5f97 100644 >> --- a/hw/i386/pc.c >> +++ b/hw/i386/pc.c >> @@ -1751,10 +1751,17 @@ out: >> static void pc_cpu_unplug_request_cb(HotplugHandler *hotplug_dev, >> DeviceState *dev, Error **errp) >> { >> + int idx; >> HotplugHandlerClass *hhc; >> Error *local_err = NULL; >> PCMachineState *pcms = PC_MACHINE(hotplug_dev); >> >> + pc_find_cpu_slot(pcms, CPU(dev), &idx); > > Looks fragile: if one day we create any TYPE_CPU object that is > not in possible_cpus array, idx is undefined. I suggest > initializing idx to -1 above. Or just let pc_find_cpu_slot universally set it to -1 since this series assumes that -1 means index isn't valid. > I can change it when committing, if that's OK for you. > > >> + if (idx == 0) { >> + error_setg(&local_err, "1st CPU (BSP) is unpluggable"); >> + goto out; >> + } >> + >> hhc = HOTPLUG_HANDLER_GET_CLASS(pcms->acpi_dev); >> hhc->unplug_request(HOTPLUG_HANDLER(pcms->acpi_dev), dev, &local_err); >> >> -- >> 2.7.4 >>
On Thu, Jul 14, 2016 at 02:16:39PM -0400, Bandan Das wrote: > Eduardo Habkost <ehabkost@redhat.com> writes: > > > On Thu, Jul 14, 2016 at 06:54:33PM +0200, Igor Mammedov wrote: > >> BSP is assumed to always present in QEMU code, so > >> untile that assumptions are gone, deny removal request. > >> In another words QEMU won't support BSP hot-unplug. > >> > >> Signed-off-by: Igor Mammedov <imammedo@redhat.com> > >> --- > >> hw/i386/pc.c | 7 +++++++ > >> 1 file changed, 7 insertions(+) > >> > >> diff --git a/hw/i386/pc.c b/hw/i386/pc.c > >> index 5a67f15..33c5f97 100644 > >> --- a/hw/i386/pc.c > >> +++ b/hw/i386/pc.c > >> @@ -1751,10 +1751,17 @@ out: > >> static void pc_cpu_unplug_request_cb(HotplugHandler *hotplug_dev, > >> DeviceState *dev, Error **errp) > >> { > >> + int idx; > >> HotplugHandlerClass *hhc; > >> Error *local_err = NULL; > >> PCMachineState *pcms = PC_MACHINE(hotplug_dev); > >> > >> + pc_find_cpu_slot(pcms, CPU(dev), &idx); > > > > Looks fragile: if one day we create any TYPE_CPU object that is > > not in possible_cpus array, idx is undefined. I suggest > > initializing idx to -1 above. > > Or just let pc_find_cpu_slot universally set it to -1 since > this series assumes that -1 means index isn't valid. I think it would be more intuitive if pc_find_cpu_slot() didn't touch *idx if no slot is found. But both ways sound good to me.
Eduardo Habkost <ehabkost@redhat.com> writes: ... >> >> DeviceState *dev, Error **errp) >> >> { >> >> + int idx; >> >> HotplugHandlerClass *hhc; >> >> Error *local_err = NULL; >> >> PCMachineState *pcms = PC_MACHINE(hotplug_dev); >> >> >> >> + pc_find_cpu_slot(pcms, CPU(dev), &idx); >> > >> > Looks fragile: if one day we create any TYPE_CPU object that is >> > not in possible_cpus array, idx is undefined. I suggest >> > initializing idx to -1 above. >> >> Or just let pc_find_cpu_slot universally set it to -1 since >> this series assumes that -1 means index isn't valid. > > I think it would be more intuitive if pc_find_cpu_slot() didn't > touch *idx if no slot is found. But both ways sound good to me. The caller then has to always take care of making sure there is no bogus value in idx. Maybe, always calling if (pc_find_cpu_slot()) is better.
On Thu, 14 Jul 2016 14:54:53 -0300 Eduardo Habkost <ehabkost@redhat.com> wrote: > On Thu, Jul 14, 2016 at 06:54:33PM +0200, Igor Mammedov wrote: > > BSP is assumed to always present in QEMU code, so > > untile that assumptions are gone, deny removal request. > > In another words QEMU won't support BSP hot-unplug. > > > > Signed-off-by: Igor Mammedov <imammedo@redhat.com> > > --- > > hw/i386/pc.c | 7 +++++++ > > 1 file changed, 7 insertions(+) > > > > diff --git a/hw/i386/pc.c b/hw/i386/pc.c > > index 5a67f15..33c5f97 100644 > > --- a/hw/i386/pc.c > > +++ b/hw/i386/pc.c > > @@ -1751,10 +1751,17 @@ out: > > static void pc_cpu_unplug_request_cb(HotplugHandler *hotplug_dev, > > DeviceState *dev, Error **errp) > > { > > + int idx; > > HotplugHandlerClass *hhc; > > Error *local_err = NULL; > > PCMachineState *pcms = PC_MACHINE(hotplug_dev); > > > > + pc_find_cpu_slot(pcms, CPU(dev), &idx); > > Looks fragile: if one day we create any TYPE_CPU object that is > not in possible_cpus array, idx is undefined. I suggest > initializing idx to -1 above. not that we expect that ever happen, but it won't hurt to initialize it to -1. > > I can change it when committing, if that's OK for you. sounds good to me, thanks. > > > > + if (idx == 0) { > > + error_setg(&local_err, "1st CPU (BSP) is unpluggable"); > > + goto out; > > + } > > + > > hhc = HOTPLUG_HANDLER_GET_CLASS(pcms->acpi_dev); > > hhc->unplug_request(HOTPLUG_HANDLER(pcms->acpi_dev), dev, &local_err); > > > > -- > > 2.7.4 > > >
diff --git a/hw/i386/pc.c b/hw/i386/pc.c index 5a67f15..33c5f97 100644 --- a/hw/i386/pc.c +++ b/hw/i386/pc.c @@ -1751,10 +1751,17 @@ out: static void pc_cpu_unplug_request_cb(HotplugHandler *hotplug_dev, DeviceState *dev, Error **errp) { + int idx; HotplugHandlerClass *hhc; Error *local_err = NULL; PCMachineState *pcms = PC_MACHINE(hotplug_dev); + pc_find_cpu_slot(pcms, CPU(dev), &idx); + if (idx == 0) { + error_setg(&local_err, "1st CPU (BSP) is unpluggable"); + goto out; + } + hhc = HOTPLUG_HANDLER_GET_CLASS(pcms->acpi_dev); hhc->unplug_request(HOTPLUG_HANDLER(pcms->acpi_dev), dev, &local_err);
BSP is assumed to always present in QEMU code, so untile that assumptions are gone, deny removal request. In another words QEMU won't support BSP hot-unplug. Signed-off-by: Igor Mammedov <imammedo@redhat.com> --- hw/i386/pc.c | 7 +++++++ 1 file changed, 7 insertions(+)