On 9/1/20 11:34 AM, Roman Bolshakov wrote: > On Tue, Sep 01, 2020 at 09:21:56AM +0200, Claudio Fontana wrote: >> now that all accelerators support the CpusAccel interface, >> we can remove most checks for non-NULL cpus_accel, >> we just add a sanity check/assert at vcpu creation. >> >> Signed-off-by: Claudio Fontana <[email protected]> >> --- >> softmmu/cpus.c | 33 +++++++++++++++++++++------------ >> 1 file changed, 21 insertions(+), 12 deletions(-) >> >> diff --git a/softmmu/cpus.c b/softmmu/cpus.c >> index 3d8350fba9..f32ecb4bb9 100644 >> --- a/softmmu/cpus.c >> +++ b/softmmu/cpus.c >> @@ -166,34 +166,46 @@ void cpu_synchronize_all_pre_loadvm(void) >> >> void cpu_synchronize_state(CPUState *cpu) >> { >> - if (cpus_accel && cpus_accel->synchronize_state) { >> + if (cpus_accel->synchronize_state) { >> cpus_accel->synchronize_state(cpu); >> } >> } >> >> void cpu_synchronize_post_reset(CPUState *cpu) >> { >> - if (cpus_accel && cpus_accel->synchronize_post_reset) { >> + if (cpus_accel->synchronize_post_reset) { >> cpus_accel->synchronize_post_reset(cpu); >> } >> } >> >> void cpu_synchronize_post_init(CPUState *cpu) >> { >> - if (cpus_accel && cpus_accel->synchronize_post_init) { >> + if (cpus_accel->synchronize_post_init) { >> cpus_accel->synchronize_post_init(cpu); >> } >> } >> >> void cpu_synchronize_pre_loadvm(CPUState *cpu) >> { >> - if (cpus_accel && cpus_accel->synchronize_pre_loadvm) { >> + if (cpus_accel->synchronize_pre_loadvm) { >> cpus_accel->synchronize_pre_loadvm(cpu); >> } >> } >> >> int64_t cpus_get_virtual_clock(void) >> { >> + /* >> + * XXX >> + * >> + * need to check that cpus_accel is not NULL, because qcow2 calls >> + * qemu_get_clock_ns(CLOCK_VIRTUAL) without any accel initialized and >> + * with ticks disabled in some io-tests: >> + * 030 040 041 060 099 120 127 140 156 161 172 181 191 192 195 203 229 >> 249 256 267 >> + * >> + * is this expected? >> + * >> + * XXX >> + */ >> if (cpus_accel && cpus_accel->get_virtual_clock) { >> return cpus_accel->get_virtual_clock(); >> } >> @@ -207,7 +219,7 @@ int64_t cpus_get_virtual_clock(void) >> */ >> int64_t cpus_get_elapsed_ticks(void) >> { >> - if (cpus_accel && cpus_accel->get_elapsed_ticks) { >> + if (cpus_accel->get_elapsed_ticks) { >> return cpus_accel->get_elapsed_ticks(); >> } >> return cpu_get_ticks(); >> @@ -399,7 +411,7 @@ void cpus_kick_thread(CPUState *cpu) >> void qemu_cpu_kick(CPUState *cpu) >> { >> qemu_cond_broadcast(cpu->halt_cond); >> - if (cpus_accel && cpus_accel->kick_vcpu_thread) { >> + if (cpus_accel->kick_vcpu_thread) { >> cpus_accel->kick_vcpu_thread(cpu); >> } else { /* default */ >> cpus_kick_thread(cpu); >> @@ -573,12 +585,9 @@ void qemu_init_vcpu(CPUState *cpu) >> cpu_address_space_init(cpu, 0, "cpu-memory", cpu->memory); >> } >> >> - if (cpus_accel) { >> - /* accelerator already implements the CpusAccel interface */ >> - cpus_accel->create_vcpu_thread(cpu); >> - } else { >> - g_assert_not_reached(); >> - } >> + /* accelerators all implement the CpusAccel interface */ >> + g_assert(cpus_accel != NULL && cpus_accel->create_vcpu_thread != NULL); >> + cpus_accel->create_vcpu_thread(cpu); >> >> while (!cpu->created) { >> qemu_cond_wait(&qemu_cpu_cond, &qemu_global_mutex); >> -- >> 2.26.2 >> > > Reviewed-by: Roman Bolshakov <[email protected]> > > but I still find the condition (if cpus_accel->func) redundant, is it > feasible to drop it? > > Regards, > Roman >
Hi Roman, indeed currently not, because currently we use a NULL function pointer to mean "use generic/default behaviour". This is one of the open questions in the cover letter. It has the advantage that only "interesting" information is present in each data structure, with only non-default behaviour being explicit, this has been changed to satisfy Paolo's requirement. It has the disadvantage of an additional check. I am ok with both outcomes, but I'd like Paolo's take on this if we are to change this again? Thanks, Claudio
