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

[David Gibson <david@gibson.dropbear.id.au>]

[-- Attachment #1: Type: text/plain, Size: 2795 bytes --]

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 <imammedo@redhat.com>
> Reported-by: David Gibson <david@gibson.dropbear.id.au>

Looks correct, although I wonder a bit about changing QTAILQ_REMOVE()
for everyone for the sake of this one use case.

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

[-- Attachment #2: signature.asc --]
[-- Type: application/pgp-signature, Size: 819 bytes --]