| Message ID | 20200901072201.7133-12-cfontana@suse.de |
|---|---|
| State | New |
| Headers | show |
| Series | QEMU cpus.c refactoring part2 | expand |
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 <cfontana@suse.de> > --- > 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 <r.bolshakov@yadro.com> but I still find the condition (if cpus_accel->func) redundant, is it feasible to drop it? Regards, Roman
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 <cfontana@suse.de> >> --- >> 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 <r.bolshakov@yadro.com> > > 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
On 9/1/20 12:21 AM, 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 <cfontana@suse.de> > --- > softmmu/cpus.c | 33 +++++++++++++++++++++------------ > 1 file changed, 21 insertions(+), 12 deletions(-) Reviewed-by: Richard Henderson <richard.henderson@linaro.org> r~
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);
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 <cfontana@suse.de> --- softmmu/cpus.c | 33 +++++++++++++++++++++------------ 1 file changed, 21 insertions(+), 12 deletions(-)