Re: [PATCH v6 11/16] cpus: remove checks for non-NULL cpus_accel

[Claudio Fontana <cfontana@suse.de>]

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