[Date Prev][Date Next][Thread Prev][Thread Next][Date Index][Thread Index]
Re: [PATCH v6 11/16] cpus: remove checks for non-NULL cpus_accel
From: |
Claudio Fontana |
Subject: |
Re: [PATCH v6 11/16] cpus: remove checks for non-NULL cpus_accel |
Date: |
Tue, 1 Sep 2020 11:44:33 +0200 |
User-agent: |
Mozilla/5.0 (X11; Linux x86_64; rv:68.0) Gecko/20100101 Thunderbird/68.11.0 |
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
- Re: [PATCH v6 10/16] cpus: cleanup now unneeded includes, (continued)
- [PATCH v6 01/16] cpu-timers, icount: new modules, Claudio Fontana, 2020/09/01
- [PATCH v6 06/16] cpus: extract out kvm-specific code to accel/kvm, Claudio Fontana, 2020/09/01
- [PATCH v6 08/16] cpus: extract out whpx-specific code to target/i386/, Claudio Fontana, 2020/09/01
- [PATCH v6 09/16] cpus: extract out hvf-specific code to target/i386/hvf/, Claudio Fontana, 2020/09/01
- [PATCH v6 11/16] cpus: remove checks for non-NULL cpus_accel, Claudio Fontana, 2020/09/01
- [PATCH v6 12/16] cpus: add handle_interrupt to the CpusAccel interface, Claudio Fontana, 2020/09/01
- [PATCH v6 13/16] hvf: remove hvf specific functions from global includes, Claudio Fontana, 2020/09/01
- [PATCH v6 14/16] whpx: remove whpx specific functions from global includes, Claudio Fontana, 2020/09/01
- [PATCH v6 15/16] hax: remove hax specific functions from global includes, Claudio Fontana, 2020/09/01
- [PATCH v6 16/16] kvm: remove kvm specific functions from global includes, Claudio Fontana, 2020/09/01