Re: [Qemu-ppc] [Qemu-devel] [PATCH 2/8] exec: don't use cpu_index to detect if cpu_exec_init()'s been called for cpu

[Igor Mammedov]

On Fri, 22 Jul 2016 17:46:40 -0300
Eduardo Habkost <address@hidden> wrote:

> 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.
I've looked at non qtail users of the field
 blockdev.c
 net/filter.c

and they also use NULL in tqe_prev as a sign that element
is not in list and net/filter.c removes element from list after
check (i.e the similar pattern as here.)

> 
> 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).
I'll do v2 with local fixup as reply to this thread
and look later at QTAIL_REMOVE (if I won't forget).

> 
> >   
> > > ---
> > >  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  
> 
> 
>