Re: [Qemu-devel] [PATCH v11 for-4.0 02/11] qemu_thread: supplement error handling for qemu_X_start_vcpu

[fei]

> 在 2019年2月1日，20:33，Markus Armbruster <address@hidden> 写道：
> 
> Fei Li <address@hidden> writes:
> 
>> From: Fei Li <address@hidden>
>> 
>> The callers of qemu_init_vcpu() already passed the **errp to handle
>> errors. In view of this, add a new Error parameter to qemu_init_vcpu()
>> and all qemu_X_start_vcpu() functions called by qemu_init_vcpu() to
>> propagate the error and let the further callers check it.
>> 
>> Besides, make qemu_init_vcpu() return a Boolean value to let its
>> callers know whether it succeeds.
>> 
>> Cc: Paolo Bonzini <address@hidden>
>> Signed-off-by: Fei Li <address@hidden>
>> Reviewed-by: Fam Zheng <address@hidden>
>> Reviewed-by: Juan Quintela <address@hidden>
>> ---
>> accel/tcg/user-exec-stub.c      |  3 +-
>> cpus.c                          | 74 +++++++++++++++++++--------------
>> include/qom/cpu.h               |  2 +-
>> target/alpha/cpu.c              |  4 +-
>> target/arm/cpu.c                |  4 +-
>> target/cris/cpu.c               |  4 +-
>> target/hppa/cpu.c               |  4 +-
>> target/i386/cpu.c               |  4 +-
>> target/lm32/cpu.c               |  4 +-
>> target/m68k/cpu.c               |  4 +-
>> target/microblaze/cpu.c         |  4 +-
>> target/mips/cpu.c               |  4 +-
>> target/moxie/cpu.c              |  4 +-
>> target/nios2/cpu.c              |  4 +-
>> target/openrisc/cpu.c           |  4 +-
>> target/ppc/translate_init.inc.c |  4 +-
>> target/riscv/cpu.c              |  4 +-
>> target/s390x/cpu.c              |  4 +-
>> target/sh4/cpu.c                |  4 +-
>> target/sparc/cpu.c              |  4 +-
>> target/tilegx/cpu.c             |  4 +-
>> target/tricore/cpu.c            |  4 +-
>> target/unicore32/cpu.c          |  4 +-
>> target/xtensa/cpu.c             |  4 +-
>> 24 files changed, 108 insertions(+), 55 deletions(-)
>> 
>> diff --git a/accel/tcg/user-exec-stub.c b/accel/tcg/user-exec-stub.c
>> index a32b4496af..f8c38a375c 100644
>> --- a/accel/tcg/user-exec-stub.c
>> +++ b/accel/tcg/user-exec-stub.c
>> @@ -10,8 +10,9 @@ void cpu_resume(CPUState *cpu)
>> {
>> }
>> 
>> -void qemu_init_vcpu(CPUState *cpu)
>> +bool qemu_init_vcpu(CPUState *cpu, Error **errp)
>> {
>> +    return true;
>> }
>> 
>> /* User mode emulation does not support record/replay yet.  */
>> diff --git a/cpus.c b/cpus.c
>> index 843a0f06a2..4ed7d62e58 100644
>> --- a/cpus.c
>> +++ b/cpus.c
>> @@ -1931,7 +1931,7 @@ void cpu_remove_sync(CPUState *cpu)
>> /* For temporary buffers for forming a name */
>> #define VCPU_THREAD_NAME_SIZE 16
>> 
>> -static void qemu_tcg_init_vcpu(CPUState *cpu)
>> +static void qemu_tcg_init_vcpu(CPUState *cpu, Error **errp)
>> {
>>     char thread_name[VCPU_THREAD_NAME_SIZE];
>>     static QemuCond *single_tcg_halt_cond;
>> @@ -1961,17 +1961,20 @@ static void qemu_tcg_init_vcpu(CPUState *cpu)
>>             snprintf(thread_name, VCPU_THREAD_NAME_SIZE, "CPU %d/TCG",
>>                  cpu->cpu_index);
>> 
>> -            /* TODO: let the callers handle the error instead of abort() 
>> here */
>> -            qemu_thread_create(cpu->thread, thread_name, 
>> qemu_tcg_cpu_thread_fn,
>> -                               cpu, QEMU_THREAD_JOINABLE, &error_abort);
>> +            if (qemu_thread_create(cpu->thread, thread_name,
>> +                                   qemu_tcg_cpu_thread_fn, cpu,
>> +                                   QEMU_THREAD_JOINABLE, errp) < 0) {
>> +                return;
>> +            }
>> 
>>         } else {
>>             /* share a single thread for all cpus with TCG */
>>             snprintf(thread_name, VCPU_THREAD_NAME_SIZE, "ALL CPUs/TCG");
>> -            /* TODO: let the callers handle the error instead of abort() 
>> here */
>> -            qemu_thread_create(cpu->thread, thread_name,
>> -                               qemu_tcg_rr_cpu_thread_fn,
>> -                               cpu, QEMU_THREAD_JOINABLE, &error_abort);
>> +            if (qemu_thread_create(cpu->thread, thread_name,
>> +                                   qemu_tcg_rr_cpu_thread_fn, cpu,
>> +                                   QEMU_THREAD_JOINABLE, errp) < 0) {
>> +                return;
>> +            }
>> 
>>             single_tcg_halt_cond = cpu->halt_cond;
>>             single_tcg_cpu_thread = cpu->thread;
>> @@ -1989,7 +1992,7 @@ static void qemu_tcg_init_vcpu(CPUState *cpu)
>>     }
>> }
>> 
>> -static void qemu_hax_start_vcpu(CPUState *cpu)
>> +static void qemu_hax_start_vcpu(CPUState *cpu, Error **errp)
>> {
>>     char thread_name[VCPU_THREAD_NAME_SIZE];
>> 
>> @@ -1999,15 +2002,16 @@ static void qemu_hax_start_vcpu(CPUState *cpu)
>> 
>>     snprintf(thread_name, VCPU_THREAD_NAME_SIZE, "CPU %d/HAX",
>>              cpu->cpu_index);
>> -    /* TODO: let the further caller handle the error instead of abort() 
>> here */
>> -    qemu_thread_create(cpu->thread, thread_name, qemu_hax_cpu_thread_fn,
>> -                       cpu, QEMU_THREAD_JOINABLE, &error_abort);
>> +    if (qemu_thread_create(cpu->thread, thread_name, qemu_hax_cpu_thread_fn,
>> +                           cpu, QEMU_THREAD_JOINABLE, errp) < 0) {
>> +        return;
>> +    }
>> #ifdef _WIN32
>>     cpu->hThread = qemu_thread_get_handle(cpu->thread);
>> #endif
>> }
>> 
>> -static void qemu_kvm_start_vcpu(CPUState *cpu)
>> +static void qemu_kvm_start_vcpu(CPUState *cpu, Error **errp)
>> {
>>     char thread_name[VCPU_THREAD_NAME_SIZE];
>> 
>> @@ -2016,12 +2020,11 @@ static void qemu_kvm_start_vcpu(CPUState *cpu)
>>     qemu_cond_init(cpu->halt_cond);
>>     snprintf(thread_name, VCPU_THREAD_NAME_SIZE, "CPU %d/KVM",
>>              cpu->cpu_index);
>> -    /* TODO: let the further caller handle the error instead of abort() 
>> here */
>>     qemu_thread_create(cpu->thread, thread_name, qemu_kvm_cpu_thread_fn,
>> -                       cpu, QEMU_THREAD_JOINABLE, &error_abort);
>> +                       cpu, QEMU_THREAD_JOINABLE, errp);
>> }
>> 
>> -static void qemu_hvf_start_vcpu(CPUState *cpu)
>> +static void qemu_hvf_start_vcpu(CPUState *cpu, Error **errp)
>> {
>>     char thread_name[VCPU_THREAD_NAME_SIZE];
>> 
>> @@ -2035,12 +2038,11 @@ static void qemu_hvf_start_vcpu(CPUState *cpu)
>> 
>>     snprintf(thread_name, VCPU_THREAD_NAME_SIZE, "CPU %d/HVF",
>>              cpu->cpu_index);
>> -    /* TODO: let the further caller handle the error instead of abort() 
>> here */
>>     qemu_thread_create(cpu->thread, thread_name, qemu_hvf_cpu_thread_fn,
>> -                       cpu, QEMU_THREAD_JOINABLE, &error_abort);
>> +                       cpu, QEMU_THREAD_JOINABLE, errp);
>> }
>> 
>> -static void qemu_whpx_start_vcpu(CPUState *cpu)
>> +static void qemu_whpx_start_vcpu(CPUState *cpu, Error **errp)
>> {
>>     char thread_name[VCPU_THREAD_NAME_SIZE];
>> 
>> @@ -2049,15 +2051,16 @@ static void qemu_whpx_start_vcpu(CPUState *cpu)
>>     qemu_cond_init(cpu->halt_cond);
>>     snprintf(thread_name, VCPU_THREAD_NAME_SIZE, "CPU %d/WHPX",
>>              cpu->cpu_index);
>> -    /* TODO: let the further caller handle the error instead of abort() 
>> here */
>> -    qemu_thread_create(cpu->thread, thread_name, qemu_whpx_cpu_thread_fn,
>> -                       cpu, QEMU_THREAD_JOINABLE, &error_abort);
>> +    if (qemu_thread_create(cpu->thread, thread_name, 
>> qemu_whpx_cpu_thread_fn,
>> +                           cpu, QEMU_THREAD_JOINABLE, errp) < 0) {
>> +        return;
>> +    }
>> #ifdef _WIN32
>>     cpu->hThread = qemu_thread_get_handle(cpu->thread);
>> #endif
>> }
>> 
>> -static void qemu_dummy_start_vcpu(CPUState *cpu)
>> +static void qemu_dummy_start_vcpu(CPUState *cpu, Error **errp)
>> {
>>     char thread_name[VCPU_THREAD_NAME_SIZE];
>> 
>> @@ -2066,16 +2069,16 @@ static void qemu_dummy_start_vcpu(CPUState *cpu)
>>     qemu_cond_init(cpu->halt_cond);
>>     snprintf(thread_name, VCPU_THREAD_NAME_SIZE, "CPU %d/DUMMY",
>>              cpu->cpu_index);
>> -    /* TODO: let the further caller handle the error instead of abort() 
>> here */
>>     qemu_thread_create(cpu->thread, thread_name, qemu_dummy_cpu_thread_fn,
>> -                       cpu, QEMU_THREAD_JOINABLE, &error_abort);
>> +                       cpu, QEMU_THREAD_JOINABLE, errp);
>> }
>> 
>> -void qemu_init_vcpu(CPUState *cpu)
>> +bool qemu_init_vcpu(CPUState *cpu, Error **errp)
>> {
>>     cpu->nr_cores = smp_cores;
>>     cpu->nr_threads = smp_threads;
>>     cpu->stopped = true;
>> +    Error *local_err = NULL;
>> 
>>     if (!cpu->as) {
>>         /* If the target cpu hasn't set up any address spaces itself,
>> @@ -2086,22 +2089,29 @@ void qemu_init_vcpu(CPUState *cpu)
>>     }
>> 
>>     if (kvm_enabled()) {
>> -        qemu_kvm_start_vcpu(cpu);
>> +        qemu_kvm_start_vcpu(cpu, &local_err);
>>     } else if (hax_enabled()) {
>> -        qemu_hax_start_vcpu(cpu);
>> +        qemu_hax_start_vcpu(cpu, &local_err);
>>     } else if (hvf_enabled()) {
>> -        qemu_hvf_start_vcpu(cpu);
>> +        qemu_hvf_start_vcpu(cpu, &local_err);
>>     } else if (tcg_enabled()) {
>> -        qemu_tcg_init_vcpu(cpu);
>> +        qemu_tcg_init_vcpu(cpu, &local_err);
>>     } else if (whpx_enabled()) {
>> -        qemu_whpx_start_vcpu(cpu);
>> +        qemu_whpx_start_vcpu(cpu, &local_err);
>>     } else {
>> -        qemu_dummy_start_vcpu(cpu);
>> +        qemu_dummy_start_vcpu(cpu, &local_err);
>> +    }
>> +
>> +    if (local_err) {
>> +        error_propagate(errp, local_err);
>> +        return false;
>>     }
>> 
>>     while (!cpu->created) {
>>         qemu_cond_wait(&qemu_cpu_cond, &qemu_global_mutex);
>>     }
>> +
>> +    return true;
>> }
>> 
>> void cpu_stop_current(void)
> 
> If the qemu_FOO_init_vcpu() returned success / failure like their callee
> qemu_thread_create() and their caller qemu_init_vcpu() do, then this
> code would be simpler.
> 
> But it's not wrong, and we're at v11, so
> Reviewed-by: Markus Armbruster <address@hidden>
> 
> [...]
Ok, and thanks for the review.

Have a nice day
Fei