| Message ID | 1474435766-9727-1-git-send-email-david@gibson.dropbear.id.au (mailing list archive) |
|---|---|
| State | New, archived |
| Headers | show |
On Wed, 21 Sep 2016 15:29:26 +1000 David Gibson <david@gibson.dropbear.id.au> wrote: > Now that we allow CPU hot unplug on a few platforms, we can end up in a > situation where we don't have a CPU with index 0. Or at least we could, > if we didn't have code to explicitly prohibit unplug of CPU 0. > > Longer term we want to allow CPU 0 unplug, this patch is an early step in > allowing this, by removing an assumption in the monitor code that CPU 0 > always exists. > > Signed-off-by: Cédric Le Goater <clg@kaod.org> > [dwg: Rewrote commit message to better explain background] > Signed-off-by: David Gibson <david@gibson.dropbear.id.au> Reviewed-by: Igor Mammedov <imammedo@redhat.com> > --- > monitor.c | 2 +- > 1 file changed, 1 insertion(+), 1 deletion(-) > > Anyone want to volunteer to take this through their tree? If not, I > can take it through my ppc tree. > > diff --git a/monitor.c b/monitor.c > index 8bb8bbf..83c4edf 100644 > --- a/monitor.c > +++ b/monitor.c > @@ -1025,7 +1025,7 @@ int monitor_set_cpu(int cpu_index) > CPUState *mon_get_cpu(void) > { > if (!cur_mon->mon_cpu) { > - monitor_set_cpu(0); > + monitor_set_cpu(first_cpu->cpu_index); > } > cpu_synchronize_state(cur_mon->mon_cpu); > return cur_mon->mon_cpu;
On Wed, Sep 21, 2016 at 03:29:26PM +1000, David Gibson wrote: > Now that we allow CPU hot unplug on a few platforms, we can end up in a > situation where we don't have a CPU with index 0. Or at least we could, > if we didn't have code to explicitly prohibit unplug of CPU 0. > > Longer term we want to allow CPU 0 unplug, this patch is an early step in > allowing this, by removing an assumption in the monitor code that CPU 0 > always exists. > > Signed-off-by: Cédric Le Goater <clg@kaod.org> > [dwg: Rewrote commit message to better explain background] > Signed-off-by: David Gibson <david@gibson.dropbear.id.au> > --- > monitor.c | 2 +- > 1 file changed, 1 insertion(+), 1 deletion(-) > > Anyone want to volunteer to take this through their tree? If not, I > can take it through my ppc tree. > > diff --git a/monitor.c b/monitor.c > index 8bb8bbf..83c4edf 100644 > --- a/monitor.c > +++ b/monitor.c > @@ -1025,7 +1025,7 @@ int monitor_set_cpu(int cpu_index) > CPUState *mon_get_cpu(void) > { > if (!cur_mon->mon_cpu) { > - monitor_set_cpu(0); > + monitor_set_cpu(first_cpu->cpu_index); So, we are replacing the "CPU 0 always exists" assumption with a "first_cpu is always non-NULL" assumption. But considering that the first_cpu assumption already exists elsewhere and those cases can be found easily using grep, I think this is OK. So: Reviewed-by: Eduardo Habkost <ehabkost@redhat.com> BTW, it is also possible to crash QEMU by unplugging the current monitor CPU; (qemu) device_add qemu64-x86_64-cpu,socket-id=2,core-id=0,thread-id=0,id=mycpu (qemu) cpu 2 (qemu) device_del mycpu (qemu) info registers qemu:qemu_cpu_kick_thread: No such process $
On Wed, 21 Sep 2016 15:29:26 +1000 David Gibson <david@gibson.dropbear.id.au> wrote: > Now that we allow CPU hot unplug on a few platforms, we can end up in a > situation where we don't have a CPU with index 0. Or at least we could, > if we didn't have code to explicitly prohibit unplug of CPU 0. > > Longer term we want to allow CPU 0 unplug, this patch is an early step in > allowing this, by removing an assumption in the monitor code that CPU 0 > always exists. > > Signed-off-by: Cédric Le Goater <clg@kaod.org> > [dwg: Rewrote commit message to better explain background] > Signed-off-by: David Gibson <david@gibson.dropbear.id.au> > --- > monitor.c | 2 +- > 1 file changed, 1 insertion(+), 1 deletion(-) > > Anyone want to volunteer to take this through their tree? If not, I > can take it through my ppc tree. Please do. Reviewed-by: Luiz Capitulino <lcapitulino@redhat.com> > > diff --git a/monitor.c b/monitor.c > index 8bb8bbf..83c4edf 100644 > --- a/monitor.c > +++ b/monitor.c > @@ -1025,7 +1025,7 @@ int monitor_set_cpu(int cpu_index) > CPUState *mon_get_cpu(void) > { > if (!cur_mon->mon_cpu) { > - monitor_set_cpu(0); > + monitor_set_cpu(first_cpu->cpu_index); > } > cpu_synchronize_state(cur_mon->mon_cpu); > return cur_mon->mon_cpu;
On Wed, Sep 21, 2016 at 09:14:02AM -0300, Eduardo Habkost wrote: > On Wed, Sep 21, 2016 at 03:29:26PM +1000, David Gibson wrote: > > Now that we allow CPU hot unplug on a few platforms, we can end up in a > > situation where we don't have a CPU with index 0. Or at least we could, > > if we didn't have code to explicitly prohibit unplug of CPU 0. > > > > Longer term we want to allow CPU 0 unplug, this patch is an early step in > > allowing this, by removing an assumption in the monitor code that CPU 0 > > always exists. > > > > Signed-off-by: Cédric Le Goater <clg@kaod.org> > > [dwg: Rewrote commit message to better explain background] > > Signed-off-by: David Gibson <david@gibson.dropbear.id.au> > > --- > > monitor.c | 2 +- > > 1 file changed, 1 insertion(+), 1 deletion(-) > > > > Anyone want to volunteer to take this through their tree? If not, I > > can take it through my ppc tree. > > > > diff --git a/monitor.c b/monitor.c > > index 8bb8bbf..83c4edf 100644 > > --- a/monitor.c > > +++ b/monitor.c > > @@ -1025,7 +1025,7 @@ int monitor_set_cpu(int cpu_index) > > CPUState *mon_get_cpu(void) > > { > > if (!cur_mon->mon_cpu) { > > - monitor_set_cpu(0); > > + monitor_set_cpu(first_cpu->cpu_index); > > So, we are replacing the "CPU 0 always exists" assumption with a > "first_cpu is always non-NULL" assumption. Well, we're replacing "CPU 0 is always present" assumption with "At least one CPU is always present", which is a strictly weaker constraint. > But considering that the first_cpu assumption already exists > elsewhere and those cases can be found easily using grep, I think > this is OK. So: > > Reviewed-by: Eduardo Habkost <ehabkost@redhat.com> > > BTW, it is also possible to crash QEMU by unplugging the current > monitor CPU; Ah... good point. > (qemu) device_add qemu64-x86_64-cpu,socket-id=2,core-id=0,thread-id=0,id=mycpu > (qemu) cpu 2 > (qemu) device_del mycpu > (qemu) info registers > qemu:qemu_cpu_kick_thread: No such process > $ >
On Wed, Sep 21, 2016 at 10:50:49AM -0400, Luiz Capitulino wrote: > On Wed, 21 Sep 2016 15:29:26 +1000 > David Gibson <david@gibson.dropbear.id.au> wrote: > > > Now that we allow CPU hot unplug on a few platforms, we can end up in a > > situation where we don't have a CPU with index 0. Or at least we could, > > if we didn't have code to explicitly prohibit unplug of CPU 0. > > > > Longer term we want to allow CPU 0 unplug, this patch is an early step in > > allowing this, by removing an assumption in the monitor code that CPU 0 > > always exists. > > > > Signed-off-by: Cédric Le Goater <clg@kaod.org> > > [dwg: Rewrote commit message to better explain background] > > Signed-off-by: David Gibson <david@gibson.dropbear.id.au> > > --- > > monitor.c | 2 +- > > 1 file changed, 1 insertion(+), 1 deletion(-) > > > > Anyone want to volunteer to take this through their tree? If not, I > > can take it through my ppc tree. > > Please do. Ok, merged to ppc-for-2.8. > > Reviewed-by: Luiz Capitulino <lcapitulino@redhat.com> > > > > > diff --git a/monitor.c b/monitor.c > > index 8bb8bbf..83c4edf 100644 > > --- a/monitor.c > > +++ b/monitor.c > > @@ -1025,7 +1025,7 @@ int monitor_set_cpu(int cpu_index) > > CPUState *mon_get_cpu(void) > > { > > if (!cur_mon->mon_cpu) { > > - monitor_set_cpu(0); > > + monitor_set_cpu(first_cpu->cpu_index); > > } > > cpu_synchronize_state(cur_mon->mon_cpu); > > return cur_mon->mon_cpu; >
diff --git a/monitor.c b/monitor.c index 8bb8bbf..83c4edf 100644 --- a/monitor.c +++ b/monitor.c @@ -1025,7 +1025,7 @@ int monitor_set_cpu(int cpu_index) CPUState *mon_get_cpu(void) { if (!cur_mon->mon_cpu) { - monitor_set_cpu(0); + monitor_set_cpu(first_cpu->cpu_index); } cpu_synchronize_state(cur_mon->mon_cpu); return cur_mon->mon_cpu;