[Date Prev][Date Next][Thread Prev][Thread Next][Date Index][Thread Index]
Re: [Qemu-ppc] [PATCH 2/8] exec: don't use cpu_index to detect if cpu_ex
From: |
Eduardo Habkost |
Subject: |
Re: [Qemu-ppc] [PATCH 2/8] exec: don't use cpu_index to detect if cpu_exec_init()'s been called for cpu |
Date: |
Fri, 22 Jul 2016 17:46:40 -0300 |
User-agent: |
Mutt/1.6.1 (2016-04-27) |
On Fri, Jul 22, 2016 at 11:32:48AM +1000, David Gibson wrote:
> On Thu, Jul 21, 2016 at 05:54:33PM +0200, Igor Mammedov wrote:
> > Instead use QTAIL's tqe_prev field to detect if cpu's been
> > placed in list by cpu_exec_init() which is always set if
> > QTAIL element is in list.
> >
> > Fixes SIGSEGV on failure path in case cpu_index is assigned
> > by board and cpu.relalize() fails before cpu_exec_init() is called.
> >
> > In follow up patches, cpu_index will be assigned by boards that
> > support cpu hot(un)plug and need stable cpu_index that doesn't
> > depend on order cpus are created/removed.
> >
> > Signed-off-by: Igor Mammedov <address@hidden>
> > Reported-by: David Gibson <address@hidden>
>
> Looks correct, although I wonder a bit about changing QTAILQ_REMOVE()
> for everyone for the sake of this one use case.
I don't mind changing it, but it's better to do it as a separate
patch so it can get the attention from other people. There are a
few cases where code checks tqe_prev directly and I don't know
how it could affect them.
Considering we don't want to take too long to include those
changes, I think we should add the tqe_prev=NULL assignment to
cpu_exec_exit() first (it's not the first time we do this in QEMU
code, see commit f8aa905a), and propose moving it to
QTAILQ_REMOVE later (and give it enough time for people to review
it).
>
> > ---
> > include/qemu/queue.h | 2 ++
> > exec.c | 4 ++--
> > 2 files changed, 4 insertions(+), 2 deletions(-)
> >
> > diff --git a/include/qemu/queue.h b/include/qemu/queue.h
> > index c2b6c81..2c2c74b 100644
> > --- a/include/qemu/queue.h
> > +++ b/include/qemu/queue.h
> > @@ -407,6 +407,7 @@ struct {
> > \
> > else \
> > (head)->tqh_last = (elm)->field.tqe_prev; \
> > *(elm)->field.tqe_prev = (elm)->field.tqe_next; \
> > + (elm)->field.tqe_prev = NULL; \
> > } while (/*CONSTCOND*/0)
> >
> > #define QTAILQ_FOREACH(var, head, field) \
> > @@ -430,6 +431,7 @@ struct {
> > \
> > #define QTAILQ_EMPTY(head) ((head)->tqh_first == NULL)
> > #define QTAILQ_FIRST(head) ((head)->tqh_first)
> > #define QTAILQ_NEXT(elm, field) ((elm)->field.tqe_next)
> > +#define QTAILQ_IN_USE(elm, field) ((elm)->field.tqe_prev)
> >
> > #define QTAILQ_LAST(head, headname) \
> > (*(((struct headname *)((head)->tqh_last))->tqh_last))
> > diff --git a/exec.c b/exec.c
> > index 2f57c62..8c5da32 100644
> > --- a/exec.c
> > +++ b/exec.c
> > @@ -643,8 +643,8 @@ void cpu_exec_exit(CPUState *cpu)
> > CPUClass *cc = CPU_GET_CLASS(cpu);
> >
> > cpu_list_lock();
> > - if (cpu->cpu_index == -1) {
> > - /* cpu_index was never allocated by this @cpu or was already
> > freed. */
> > + if (!QTAILQ_IN_USE(cpu, node)) {
> > + /* there is nothing to undo since cpu_exec_init() hasn't been
> > called */
> > cpu_list_unlock();
> > return;
> > }
>
> --
> David Gibson | I'll have my music baroque, and my code
> david AT gibson.dropbear.id.au | minimalist, thank you. NOT _the_
> _other_
> | _way_ _around_!
> http://www.ozlabs.org/~dgibson
--
Eduardo
- [Qemu-ppc] [PATCH 0/8] Fix migration issues with arbitrary cpu-hot(un)plug, Igor Mammedov, 2016/07/21
- [Qemu-ppc] [PATCH 1/8] exec: reduce CONFIG_USER_ONLY ifdeffenery, Igor Mammedov, 2016/07/21
- [Qemu-ppc] [PATCH 3/8] exec: set cpu_index only if it's been explictly set, Igor Mammedov, 2016/07/21
- [Qemu-ppc] [PATCH 5/8] pc: init CPUState->cpu_index with index in possible_cpus[], Igor Mammedov, 2016/07/21
- [Qemu-ppc] [PATCH 6/8] spapr: init CPUState->cpu_index with index relative to core-id, Igor Mammedov, 2016/07/21