| Message ID | 20200901072201.7133-1-cfontana@suse.de |
|---|---|
| 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:38 AM, Roman Bolshakov wrote: > On Tue, Sep 01, 2020 at 09:21:57AM +0200, Claudio Fontana wrote: >> kvm: uses the generic handler >> qtest: uses the generic handler >> whpx: changed to use the generic handler (identical implementation) >> hax: changed to use the generic handler (identical implementation) >> hvf: changed to use the generic handler (identical implementation) >> tcg: adapt tcg-cpus to point to the tcg-specific handler >> >> Signed-off-by: Claudio Fontana <cfontana@suse.de> >> --- >> accel/tcg/tcg-all.c | 26 -------------------------- >> accel/tcg/tcg-cpus.c | 28 ++++++++++++++++++++++++++++ >> hw/core/cpu.c | 13 ------------- >> include/hw/core/cpu.h | 14 -------------- >> include/sysemu/cpus.h | 2 ++ >> softmmu/cpus.c | 18 ++++++++++++++++++ >> target/i386/hax-all.c | 10 ---------- >> target/i386/hvf/hvf.c | 9 --------- >> target/i386/whpx-all.c | 10 ---------- >> 9 files changed, 48 insertions(+), 82 deletions(-) >> >> diff --git a/accel/tcg/tcg-all.c b/accel/tcg/tcg-all.c >> index 01957b130d..af9bf5c5bb 100644 >> --- a/accel/tcg/tcg-all.c >> +++ b/accel/tcg/tcg-all.c >> @@ -47,31 +47,6 @@ typedef struct TCGState { >> #define TCG_STATE(obj) \ >> OBJECT_CHECK(TCGState, (obj), TYPE_TCG_ACCEL) >> >> -/* mask must never be zero, except for A20 change call */ >> -static void tcg_handle_interrupt(CPUState *cpu, int mask) >> -{ >> - int old_mask; >> - g_assert(qemu_mutex_iothread_locked()); >> - >> - old_mask = cpu->interrupt_request; >> - cpu->interrupt_request |= mask; >> - >> - /* >> - * If called from iothread context, wake the target cpu in >> - * case its halted. >> - */ >> - if (!qemu_cpu_is_self(cpu)) { >> - qemu_cpu_kick(cpu); >> - } else { >> - atomic_set(&cpu_neg(cpu)->icount_decr.u16.high, -1); >> - if (icount_enabled() && >> - !cpu->can_do_io >> - && (mask & ~old_mask) != 0) { >> - cpu_abort(cpu, "Raised interrupt while not in I/O function"); >> - } >> - } >> -} >> - >> /* >> * We default to false if we know other options have been enabled >> * which are currently incompatible with MTTCG. Otherwise when each >> @@ -128,7 +103,6 @@ static int tcg_init(MachineState *ms) >> TCGState *s = TCG_STATE(current_accel()); >> >> tcg_exec_init(s->tb_size * 1024 * 1024); >> - cpu_interrupt_handler = tcg_handle_interrupt; >> mttcg_enabled = s->mttcg_enabled; >> cpus_register_accel(&tcg_cpus); >> >> diff --git a/accel/tcg/tcg-cpus.c b/accel/tcg/tcg-cpus.c >> index 72696f6d86..2bb209e2c6 100644 >> --- a/accel/tcg/tcg-cpus.c >> +++ b/accel/tcg/tcg-cpus.c >> @@ -533,9 +533,37 @@ static int64_t tcg_get_elapsed_ticks(void) >> return cpu_get_ticks(); >> } >> >> +/* mask must never be zero, except for A20 change call */ >> +static void tcg_handle_interrupt(CPUState *cpu, int mask) >> +{ >> + int old_mask; >> + g_assert(qemu_mutex_iothread_locked()); >> + >> + old_mask = cpu->interrupt_request; >> + cpu->interrupt_request |= mask; >> + >> + /* >> + * If called from iothread context, wake the target cpu in >> + * case its halted. >> + */ >> + if (!qemu_cpu_is_self(cpu)) { >> + qemu_cpu_kick(cpu); >> + } else { >> + atomic_set(&cpu_neg(cpu)->icount_decr.u16.high, -1); >> + if (icount_enabled() && >> + !cpu->can_do_io >> + && (mask & ~old_mask) != 0) { >> + cpu_abort(cpu, "Raised interrupt while not in I/O function"); >> + } >> + } >> +} >> + >> const CpusAccel tcg_cpus = { >> .create_vcpu_thread = tcg_start_vcpu_thread, >> .kick_vcpu_thread = tcg_kick_vcpu_thread, >> + >> + .handle_interrupt = tcg_handle_interrupt, >> + >> .get_virtual_clock = tcg_get_virtual_clock, >> .get_elapsed_ticks = tcg_get_elapsed_ticks, >> }; >> diff --git a/hw/core/cpu.c b/hw/core/cpu.c >> index fa8602493b..451b3d5ee7 100644 >> --- a/hw/core/cpu.c >> +++ b/hw/core/cpu.c >> @@ -35,8 +35,6 @@ >> #include "qemu/plugin.h" >> #include "sysemu/hw_accel.h" >> >> -CPUInterruptHandler cpu_interrupt_handler; >> - >> CPUState *cpu_by_arch_id(int64_t id) >> { >> CPUState *cpu; >> @@ -394,17 +392,6 @@ static vaddr cpu_adjust_watchpoint_address(CPUState *cpu, vaddr addr, int len) >> return addr; >> } >> >> -static void generic_handle_interrupt(CPUState *cpu, int mask) >> -{ >> - cpu->interrupt_request |= mask; >> - >> - if (!qemu_cpu_is_self(cpu)) { >> - qemu_cpu_kick(cpu); >> - } >> -} >> - >> -CPUInterruptHandler cpu_interrupt_handler = generic_handle_interrupt; >> - >> static void cpu_class_init(ObjectClass *klass, void *data) >> { >> DeviceClass *dc = DEVICE_CLASS(klass); >> diff --git a/include/hw/core/cpu.h b/include/hw/core/cpu.h >> index 8f145733ce..efd33d87fd 100644 >> --- a/include/hw/core/cpu.h >> +++ b/include/hw/core/cpu.h >> @@ -838,12 +838,6 @@ bool cpu_exists(int64_t id); >> */ >> CPUState *cpu_by_arch_id(int64_t id); >> >> -#ifndef CONFIG_USER_ONLY >> - >> -typedef void (*CPUInterruptHandler)(CPUState *, int); >> - >> -extern CPUInterruptHandler cpu_interrupt_handler; >> - >> /** >> * cpu_interrupt: >> * @cpu: The CPU to set an interrupt on. >> @@ -851,17 +845,9 @@ extern CPUInterruptHandler cpu_interrupt_handler; >> * >> * Invokes the interrupt handler. >> */ >> -static inline void cpu_interrupt(CPUState *cpu, int mask) >> -{ >> - cpu_interrupt_handler(cpu, mask); >> -} >> - >> -#else /* USER_ONLY */ >> >> void cpu_interrupt(CPUState *cpu, int mask); >> >> -#endif /* USER_ONLY */ >> - >> #ifdef NEED_CPU_H >> >> #ifdef CONFIG_SOFTMMU >> diff --git a/include/sysemu/cpus.h b/include/sysemu/cpus.h >> index 26171697f5..231685955d 100644 >> --- a/include/sysemu/cpus.h >> +++ b/include/sysemu/cpus.h >> @@ -16,6 +16,8 @@ typedef struct CpusAccel { >> void (*synchronize_state)(CPUState *cpu); >> void (*synchronize_pre_loadvm)(CPUState *cpu); >> >> + void (*handle_interrupt)(CPUState *cpu, int mask); >> + >> int64_t (*get_virtual_clock)(void); >> int64_t (*get_elapsed_ticks)(void); >> } CpusAccel; >> diff --git a/softmmu/cpus.c b/softmmu/cpus.c >> index f32ecb4bb9..7068666579 100644 >> --- a/softmmu/cpus.c >> +++ b/softmmu/cpus.c >> @@ -225,6 +225,24 @@ int64_t cpus_get_elapsed_ticks(void) >> return cpu_get_ticks(); >> } >> >> +static void generic_handle_interrupt(CPUState *cpu, int mask) >> +{ >> + cpu->interrupt_request |= mask; >> + >> + if (!qemu_cpu_is_self(cpu)) { >> + qemu_cpu_kick(cpu); >> + } >> +} >> + >> +void cpu_interrupt(CPUState *cpu, int mask) >> +{ >> + if (cpus_accel->handle_interrupt) { >> + cpus_accel->handle_interrupt(cpu, mask); >> + } else { >> + generic_handle_interrupt(cpu, mask); >> + } >> +} >> + > > The handlers is not something that dynamically changes, the functions > can be assigned once during accel registration. Data struct is now const though, they are never assigned, only initialized.. > Accel registraton is > also the place where the checks can take place. > > Regards, > Roman > >> static int do_vm_stop(RunState state, bool send_stop) >> { >> int ret = 0; >> diff --git a/target/i386/hax-all.c b/target/i386/hax-all.c >> index b66ddeb8bf..fd1ab673d7 100644 >> --- a/target/i386/hax-all.c >> +++ b/target/i386/hax-all.c >> @@ -297,15 +297,6 @@ int hax_vm_destroy(struct hax_vm *vm) >> return 0; >> } >> >> -static void hax_handle_interrupt(CPUState *cpu, int mask) >> -{ >> - cpu->interrupt_request |= mask; >> - >> - if (!qemu_cpu_is_self(cpu)) { >> - qemu_cpu_kick(cpu); >> - } >> -} >> - >> static int hax_init(ram_addr_t ram_size, int max_cpus) >> { >> struct hax_state *hax = NULL; >> @@ -350,7 +341,6 @@ static int hax_init(ram_addr_t ram_size, int max_cpus) >> qversion.cur_version = hax_cur_version; >> qversion.min_version = hax_min_version; >> hax_notify_qemu_version(hax->vm->fd, &qversion); >> - cpu_interrupt_handler = hax_handle_interrupt; >> >> return ret; >> error: >> diff --git a/target/i386/hvf/hvf.c b/target/i386/hvf/hvf.c >> index 7ac6987c1b..ed9356565c 100644 >> --- a/target/i386/hvf/hvf.c >> +++ b/target/i386/hvf/hvf.c >> @@ -262,14 +262,6 @@ static void update_apic_tpr(CPUState *cpu) >> >> #define VECTORING_INFO_VECTOR_MASK 0xff >> >> -static void hvf_handle_interrupt(CPUState * cpu, int mask) >> -{ >> - cpu->interrupt_request |= mask; >> - if (!qemu_cpu_is_self(cpu)) { >> - qemu_cpu_kick(cpu); >> - } >> -} >> - >> void hvf_handle_io(CPUArchState *env, uint16_t port, void *buffer, >> int direction, int size, int count) >> { >> @@ -894,7 +886,6 @@ static int hvf_accel_init(MachineState *ms) >> } >> >> hvf_state = s; >> - cpu_interrupt_handler = hvf_handle_interrupt; >> memory_listener_register(&hvf_memory_listener, &address_space_memory); >> cpus_register_accel(&hvf_cpus); >> return 0; >> diff --git a/target/i386/whpx-all.c b/target/i386/whpx-all.c >> index 8b6986c864..b3d17fbe04 100644 >> --- a/target/i386/whpx-all.c >> +++ b/target/i386/whpx-all.c >> @@ -1413,15 +1413,6 @@ static void whpx_memory_init(void) >> memory_listener_register(&whpx_memory_listener, &address_space_memory); >> } >> >> -static void whpx_handle_interrupt(CPUState *cpu, int mask) >> -{ >> - cpu->interrupt_request |= mask; >> - >> - if (!qemu_cpu_is_self(cpu)) { >> - qemu_cpu_kick(cpu); >> - } >> -} >> - >> /* >> * Load the functions from the given library, using the given handle. If a >> * handle is provided, it is used, otherwise the library is opened. The >> @@ -1576,7 +1567,6 @@ static int whpx_accel_init(MachineState *ms) >> >> whpx_memory_init(); >> >> - cpu_interrupt_handler = whpx_handle_interrupt; >> cpus_register_accel(&whpx_cpus); >> >> printf("Windows Hypervisor Platform accelerator is operational\n"); >> -- >> 2.26.2 >>
Claudio Fontana <cfontana@suse.de> writes: > Signed-off-by: Claudio Fontana <cfontana@suse.de> Reviewed-by: Alex Bennée <alex.bennee@linaro.org>
Claudio Fontana <cfontana@suse.de> writes: > Signed-off-by: Claudio Fontana <cfontana@suse.de> > Reviewed-by: Richard Henderson <richard.henderson@linaro.org> Reviewed-by: Alex Bennée <alex.bennee@linaro.org>
On 9/1/20 12:20 PM, Alex Bennée wrote: > > Claudio Fontana <cfontana@suse.de> writes: > >> refactoring of cpus.c continues with cpu timer state extraction. >> >> cpu-timers: responsible for the softmmu cpu timers state, >> including cpu clocks and ticks. >> >> icount: counts the TCG instructions executed. As such it is specific to >> the TCG accelerator. Therefore, it is built only under CONFIG_TCG. >> >> One complication is due to qtest, which uses an icount field to warp time >> as part of qtest (qtest_clock_warp). >> >> In order to solve this problem, provide a separate counter for qtest. >> >> This requires fixing assumptions scattered in the code that >> qtest_enabled() implies icount_enabled(), checking each specific case. >> >> Signed-off-by: Claudio Fontana <cfontana@suse.de> >> --- > <snip> >> + >> +void qemu_timer_notify_cb(void *opaque, QEMUClockType type) >> +{ >> + if (!icount_enabled() || type != QEMU_CLOCK_VIRTUAL) { >> + qemu_notify_event(); >> + return; >> + } >> + >> + if (qemu_in_vcpu_thread()) { >> + /* >> + * A CPU is currently running; kick it back out to the >> + * tcg_cpu_exec() loop so it will recalculate its >> + * icount deadline immediately. >> + */ >> + qemu_cpu_kick(current_cpu); >> + } else if (first_cpu) { >> + /* >> + * qemu_cpu_kick is not enough to kick a halted CPU out of >> + * qemu_tcg_wait_io_event. async_run_on_cpu, instead, >> + * causes cpu_thread_is_idle to return false. This way, >> + * handle_icount_deadline can run. >> + * If we have no CPUs at all for some reason, we don't >> + * need to do anything. >> + */ >> + async_run_on_cpu(first_cpu, do_nothing, RUN_ON_CPU_NULL); >> + } >> +} >> + > > See bellow for comments on stub. > >> diff --git a/softmmu/vl.c b/softmmu/vl.c >> index 0cc86b0766..ff94cf4983 100644 >> --- a/softmmu/vl.c >> +++ b/softmmu/vl.c >> @@ -74,6 +74,7 @@ >> #include "hw/audio/soundhw.h" >> #include "audio/audio.h" >> #include "sysemu/cpus.h" >> +#include "sysemu/cpu-timers.h" >> #include "migration/colo.h" >> #include "migration/postcopy-ram.h" >> #include "sysemu/kvm.h" >> @@ -2802,7 +2803,7 @@ static void configure_accelerators(const char *progname) >> error_report("falling back to %s", ac->name); >> } >> >> - if (use_icount && !(tcg_enabled() || qtest_enabled())) { >> + if (icount_enabled() && !tcg_enabled()) { >> error_report("-icount is not allowed with hardware virtualization"); >> exit(1); >> } >> @@ -4237,7 +4238,11 @@ void qemu_init(int argc, char **argv, char **envp) >> semihosting_arg_fallback(kernel_filename, kernel_cmdline); >> } >> >> - cpu_ticks_init(); >> + /* initialize cpu timers and VCPU throttle modules */ >> + cpu_timers_init(); >> + >> + /* spice needs the timers to be initialized by this point */ >> + qemu_spice_init(); > > This seems to be an additional fix? This seems to be a mistake on my part, some rebase leftover, the initialization already happened before. Will remove/fix, thanks for the catch! > > <snip> >> diff --git a/stubs/cpu-get-icount.c b/stubs/cpu-get-icount.c >> deleted file mode 100644 >> index b35f844638..0000000000 >> --- a/stubs/cpu-get-icount.c >> +++ /dev/null >> @@ -1,21 +0,0 @@ >> -#include "qemu/osdep.h" >> -#include "qemu/timer.h" >> -#include "sysemu/cpus.h" >> -#include "qemu/main-loop.h" >> - >> -int use_icount; >> - >> -int64_t cpu_get_icount(void) >> -{ >> - abort(); >> -} >> - >> -int64_t cpu_get_icount_raw(void) >> -{ >> - abort(); >> -} >> - >> -void qemu_timer_notify_cb(void *opaque, QEMUClockType type) >> -{ >> - qemu_notify_event(); >> -} > > Hmm so this was existing behaviour for stubs - I find it slightly weird > that a stub function actually does something - although of course that > might be stubbed as well :-/ I am puzzled as well to see this. I tried removing the qemu_notify_event() call, and it does not seem to break anything for me, but I'd keep it unchanged for now, maybe it was added for some reason? > >> diff --git a/stubs/icount.c b/stubs/icount.c >> new file mode 100644 >> index 0000000000..61e28cbaf9 >> --- /dev/null >> +++ b/stubs/icount.c >> @@ -0,0 +1,45 @@ >> +#include "qemu/osdep.h" >> +#include "qapi/error.h" >> +#include "sysemu/cpu-timers.h" >> + >> +/* icount - Instruction Counter API */ >> + >> +int use_icount; >> + >> +void cpu_update_icount(CPUState *cpu) >> +{ >> + abort(); >> +} >> +void configure_icount(QemuOpts *opts, Error **errp) >> +{ >> + /* signal error */ >> + error_setg(errp, "cannot configure icount, TCG support not available"); >> +} >> +int64_t cpu_get_icount_raw(void) >> +{ >> + abort(); >> + return 0; >> +} >> +int64_t cpu_get_icount(void) >> +{ >> + abort(); >> + return 0; >> +} >> +int64_t cpu_icount_to_ns(int64_t icount) >> +{ >> + abort(); >> + return 0; >> +} >> +int64_t qemu_icount_round(int64_t count) >> +{ >> + abort(); >> + return 0; >> +} >> +void qemu_start_warp_timer(void) >> +{ >> + abort(); >> +} >> +void qemu_account_warp_timer(void) >> +{ >> + abort(); >> +} >> diff --git a/stubs/meson.build b/stubs/meson.build >> index 019bd79c7a..57fec4f8c8 100644 >> --- a/stubs/meson.build >> +++ b/stubs/meson.build >> @@ -3,10 +3,10 @@ stub_ss.add(files('bdrv-next-monitor-owned.c')) >> stub_ss.add(files('blk-commit-all.c')) >> stub_ss.add(files('blockdev-close-all-bdrv-states.c')) >> stub_ss.add(files('change-state-handler.c')) >> -stub_ss.add(files('clock-warp.c')) >> stub_ss.add(files('cmos.c')) >> stub_ss.add(files('cpu-get-clock.c')) >> -stub_ss.add(files('cpu-get-icount.c')) >> +stub_ss.add(files('qemu-timer-notify-cb.c')) >> +stub_ss.add(files('icount.c')) >> stub_ss.add(files('dump.c')) >> stub_ss.add(files('error-printf.c')) >> stub_ss.add(files('fd-register.c')) >> diff --git a/stubs/qemu-timer-notify-cb.c b/stubs/qemu-timer-notify-cb.c >> new file mode 100644 >> index 0000000000..845e46f8e0 >> --- /dev/null >> +++ b/stubs/qemu-timer-notify-cb.c >> @@ -0,0 +1,8 @@ >> +#include "qemu/osdep.h" >> +#include "sysemu/cpu-timers.h" >> +#include "qemu/main-loop.h" >> + >> +void qemu_timer_notify_cb(void *opaque, QEMUClockType type) >> +{ >> + qemu_notify_event(); >> +} >> diff --git a/stubs/qtest.c b/stubs/qtest.c >> index 891eb954fb..4666a49d7d 100644 >> --- a/stubs/qtest.c >> +++ b/stubs/qtest.c >> @@ -18,3 +18,8 @@ bool qtest_driver(void) >> { >> return false; >> } >> + >> +int64_t qtest_get_virtual_clock(void) >> +{ >> + return 0; >> +} >> diff --git a/target/alpha/translate.c b/target/alpha/translate.c >> index 8870284f57..36be602179 100644 >> --- a/target/alpha/translate.c >> +++ b/target/alpha/translate.c >> @@ -20,6 +20,7 @@ >> #include "qemu/osdep.h" >> #include "cpu.h" >> #include "sysemu/cpus.h" >> +#include "sysemu/cpu-timers.h" >> #include "disas/disas.h" >> #include "qemu/host-utils.h" >> #include "exec/exec-all.h" >> @@ -1329,7 +1330,7 @@ static DisasJumpType gen_mfpr(DisasContext *ctx, TCGv va, int regno) >> case 249: /* VMTIME */ >> helper = gen_helper_get_vmtime; >> do_helper: >> - if (use_icount) { >> + if (icount_enabled()) { >> gen_io_start(); >> helper(va); >> return DISAS_PC_STALE; >> diff --git a/target/arm/helper.c b/target/arm/helper.c >> index 44d666627a..dc2b91084c 100644 >> --- a/target/arm/helper.c >> +++ b/target/arm/helper.c >> @@ -24,6 +24,7 @@ >> #include "hw/irq.h" >> #include "hw/semihosting/semihost.h" >> #include "sysemu/cpus.h" >> +#include "sysemu/cpu-timers.h" >> #include "sysemu/kvm.h" >> #include "sysemu/tcg.h" >> #include "qemu/range.h" >> @@ -1206,7 +1207,7 @@ static int64_t cycles_ns_per(uint64_t cycles) >> >> static bool instructions_supported(CPUARMState *env) >> { >> - return use_icount == 1 /* Precise instruction counting */; >> + return icount_enabled() == 1; /* Precise instruction counting */ >> } >> >> static uint64_t instructions_get_count(CPUARMState *env) >> diff --git a/target/riscv/csr.c b/target/riscv/csr.c >> index 200001de74..5231404a70 100644 >> --- a/target/riscv/csr.c >> +++ b/target/riscv/csr.c >> @@ -299,7 +299,7 @@ static int write_vstart(CPURISCVState *env, int csrno, target_ulong val) >> static int read_instret(CPURISCVState *env, int csrno, target_ulong *val) >> { >> #if !defined(CONFIG_USER_ONLY) >> - if (use_icount) { >> + if (icount_enabled()) { >> *val = cpu_get_icount(); >> } else { >> *val = cpu_get_host_ticks(); >> @@ -314,7 +314,7 @@ static int read_instret(CPURISCVState *env, int csrno, target_ulong *val) >> static int read_instreth(CPURISCVState *env, int csrno, target_ulong *val) >> { >> #if !defined(CONFIG_USER_ONLY) >> - if (use_icount) { >> + if (icount_enabled()) { >> *val = cpu_get_icount() >> 32; >> } else { >> *val = cpu_get_host_ticks() >> 32; >> diff --git a/tests/ptimer-test-stubs.c b/tests/ptimer-test-stubs.c >> index ed393d9082..e935a1395e 100644 >> --- a/tests/ptimer-test-stubs.c >> +++ b/tests/ptimer-test-stubs.c >> @@ -12,6 +12,7 @@ >> #include "qemu/main-loop.h" >> #include "sysemu/replay.h" >> #include "migration/vmstate.h" >> +#include "sysemu/cpu-timers.h" >> >> #include "ptimer-test.h" >> >> @@ -30,8 +31,8 @@ QEMUTimerListGroup main_loop_tlg; >> >> int64_t ptimer_test_time_ns; >> >> -/* Do not artificially limit period - see hw/core/ptimer.c. */ >> -int use_icount = 1; >> +/* under qtest_enabled(), will not artificially limit period - see hw/core/ptimer.c. */ >> +int use_icount; >> bool qtest_allowed; >> >> void timer_init_full(QEMUTimer *ts, >> diff --git a/tests/test-timed-average.c b/tests/test-timed-average.c >> index e2bcf5fe13..82c92500df 100644 >> --- a/tests/test-timed-average.c >> +++ b/tests/test-timed-average.c >> @@ -11,7 +11,7 @@ >> */ >> >> #include "qemu/osdep.h" >> - >> +#include "sysemu/cpu-timers.h" >> #include "qemu/timed-average.h" >> >> /* This is the clock for QEMU_CLOCK_VIRTUAL */ >> diff --git a/util/main-loop.c b/util/main-loop.c >> index f69f055013..4015d58967 100644 >> --- a/util/main-loop.c >> +++ b/util/main-loop.c >> @@ -27,7 +27,7 @@ >> #include "qemu/cutils.h" >> #include "qemu/timer.h" >> #include "sysemu/qtest.h" >> -#include "sysemu/cpus.h" >> +#include "sysemu/cpu-timers.h" >> #include "sysemu/replay.h" >> #include "qemu/main-loop.h" >> #include "block/aio.h" >> @@ -517,9 +517,13 @@ void main_loop_wait(int nonblocking) >> mlpoll.state = ret < 0 ? MAIN_LOOP_POLL_ERR : MAIN_LOOP_POLL_OK; >> notifier_list_notify(&main_loop_poll_notifiers, &mlpoll); >> >> - /* CPU thread can infinitely wait for event after >> - missing the warp */ >> - qemu_start_warp_timer(); >> + if (icount_enabled()) { >> + /* >> + * CPU thread can infinitely wait for event after >> + * missing the warp >> + */ >> + qemu_start_warp_timer(); >> + } >> qemu_clock_run_all_timers(); >> } >> >> diff --git a/util/qemu-timer.c b/util/qemu-timer.c >> index f62b4feecd..100a4eba22 100644 >> --- a/util/qemu-timer.c >> +++ b/util/qemu-timer.c >> @@ -26,8 +26,10 @@ >> #include "qemu/main-loop.h" >> #include "qemu/timer.h" >> #include "qemu/lockable.h" >> +#include "sysemu/cpu-timers.h" >> #include "sysemu/replay.h" >> #include "sysemu/cpus.h" >> +#include "sysemu/qtest.h" >> >> #ifdef CONFIG_POSIX >> #include <pthread.h> >> @@ -134,7 +136,7 @@ static void qemu_clock_init(QEMUClockType type, QEMUTimerListNotifyCB *notify_cb >> >> bool qemu_clock_use_for_deadline(QEMUClockType type) >> { >> - return !(use_icount && (type == QEMU_CLOCK_VIRTUAL)); >> + return !(icount_enabled() && (type == QEMU_CLOCK_VIRTUAL)); >> } >> >> void qemu_clock_notify(QEMUClockType type) >> @@ -416,7 +418,7 @@ static bool timer_mod_ns_locked(QEMUTimerList *timer_list, >> static void timerlist_rearm(QEMUTimerList *timer_list) >> { >> /* Interrupt execution to force deadline recalculation. */ >> - if (timer_list->clock->type == QEMU_CLOCK_VIRTUAL) { >> + if (icount_enabled() && timer_list->clock->type == QEMU_CLOCK_VIRTUAL) { >> qemu_start_warp_timer(); >> } >> timerlist_notify(timer_list); >> @@ -633,8 +635,10 @@ int64_t qemu_clock_get_ns(QEMUClockType type) >> return get_clock(); >> default: >> case QEMU_CLOCK_VIRTUAL: >> - if (use_icount) { >> + if (icount_enabled()) { >> return cpu_get_icount(); >> + } else if (qtest_enabled()) { /* for qtest_clock_warp */ >> + return qtest_get_virtual_clock(); >> } else { >> return cpu_get_clock(); >> } > > Otherwise: > > Reviewed-by: Alex Bennée <alex.bennee@linaro.org> > Thanks! Claudio
On Tue 01 Sep 2020 09:21:45 AM CEST, Claudio Fontana wrote: > * in some cases the virtual clock is queried before an accelerator > is set or ticks are enabled with > > qemu_clock_get_ms(QEMU_CLOCK_VIRTUAL) > > by the qcow2.c code (ending up in 0); maybe this should not happen > at all, as it could hurt migrations with the clock jumping up from > 0? Should it be QEMU_CLOCK_REALTIME? (Berto, Paolo) As far as I can see the only place in the qcow2 code that uses QEMU_CLOCK_VIRTUAL is in the timer that clears unused cache entries periodically. I used QEMU_CLOCK_VIRTUAL because it didn't make sense to me to expire cache entries when the VM is stopped. I'm not sure about what would happen during a migration, is the qcow2 cache migrated at all? And if it is, would the clock jump up suddenly? If it's just that I understand that the effect would be that the timer would be fired earlier than expected, but I don't think it's a big deal. Berto
On 9/1/20 12:21 AM, Claudio Fontana wrote: > refactoring of cpus.c continues with cpu timer state extraction. > > cpu-timers: responsible for the softmmu cpu timers state, > including cpu clocks and ticks. > > icount: counts the TCG instructions executed. As such it is specific to > the TCG accelerator. Therefore, it is built only under CONFIG_TCG. > > One complication is due to qtest, which uses an icount field to warp time > as part of qtest (qtest_clock_warp). > > In order to solve this problem, provide a separate counter for qtest. > > This requires fixing assumptions scattered in the code that > qtest_enabled() implies icount_enabled(), checking each specific case. > > Signed-off-by: Claudio Fontana <cfontana@suse.de> > --- Reviewed-by: Richard Henderson <richard.henderson@linaro.org> r~
On 9/1/20 12:21 AM, Claudio Fontana wrote: > kvm: uses the generic handler > qtest: uses the generic handler > whpx: changed to use the generic handler (identical implementation) > hax: changed to use the generic handler (identical implementation) > hvf: changed to use the generic handler (identical implementation) > tcg: adapt tcg-cpus to point to the tcg-specific handler > > Signed-off-by: Claudio Fontana <cfontana@suse.de> > --- > accel/tcg/tcg-all.c | 26 -------------------------- > accel/tcg/tcg-cpus.c | 28 ++++++++++++++++++++++++++++ > hw/core/cpu.c | 13 ------------- > include/hw/core/cpu.h | 14 -------------- > include/sysemu/cpus.h | 2 ++ > softmmu/cpus.c | 18 ++++++++++++++++++ > target/i386/hax-all.c | 10 ---------- > target/i386/hvf/hvf.c | 9 --------- > target/i386/whpx-all.c | 10 ---------- > 9 files changed, 48 insertions(+), 82 deletions(-) Reviewed-by: Richard Henderson <richard.henderson@linaro.org> r~
Motivation and higher level steps: https://lists.gnu.org/archive/html/qemu-devel/2020-05/msg04628.html Main Open topics: * in some cases the virtual clock is queried before an accelerator is set or ticks are enabled with qemu_clock_get_ms(QEMU_CLOCK_VIRTUAL) by the qcow2.c code (ending up in 0); maybe this should not happen at all, as it could hurt migrations with the clock jumping up from 0? Should it be QEMU_CLOCK_REALTIME? (Berto, Paolo) * currently the per-accelerator CpusAccel structs use NULL to mean "default", and only explicitly define non-default behavior in the methods. Should this be changed? (Richard / Paolo) * refactoring of tcg start vcpu code, so that more common parts can be reused in providing multiple structs for normal, icount, mttcg. Could this be part of another series (before or after this)? (Alex, Richard) * question around naming of functions for the cpus module, currently using cpus_ in the newly implemented functionality. (Roman) Ciao, Claudio PATCH v5 -> PATCH v6: * moved to mason build system * patch (new): cpus: remove checks for non-NULL cpus_accel (Richard) This has however a big caveat: in some cases the virtual clock is queried before an accelerator is set or ticks are enabled; this is currently special cased (keeping the NULL check in cpus_get_virtual_clock), but maybe this should not happen at all? (Paolo, Berto) * in patch "cpu-timers, icount: new modules" do not change (yet) icount_enabled() to a function. Mimic instead what is done with tcg_enabled(). (Richard) * split the changes into two separate patches, with name-only changes extracted out into a separate patch (Richard). Removed existing Reviewed-by because of these changes (Alex)- Alex are you ok with them? * in patch "cpus: prepare new CpusAccel cpu accelerator interface" remove some unneeded stubs from stubs/cpu-synchronize-state.c Use const for the CpusAccel interface. (Richard) * in patch "cpus: extract out TCG-specific code to accel/tcg" use const for the CpusAccel interface. (Richard) * in patch "cpus: extract out qtest-specific code to accel/qtest" use const for the CpusAccel interface; use g_assert_not_reached (Richard) * in patch "cpus: extract out kvm-specific code to accel/kvm" use const for the CpusAccel interface. (Richard) * in patch "cpus: extract out hax-specific code to target/i386/" use const for the CpusAccel interface. (Richard) * in patch "cpus: extract out whpx-specific code to target/i386/" use const for the CpusAccel interface. (Richard) * in patch "cpus: extract out hvf-specific code to target/i386/hvf/" use const for the CpusAccel interface. (Richard) RFC v4 -> PATCH v5: * in patch 2, move comment about cpus_get_elapsed_ticks from patch 3 (Philippe) * in patch 11-14, do not create separate xxx-int.h files, instead use the xxx-cpus.h files (Philippe) RFC v3 -> v4: * added patch 9: cleanup unneeded includes * added patch 10: add handle_interrupt to the interface (Roman) * added patch 11-14: remove accelerator specific internal functions from global includes (Roman) * in patch 2, removed leftover "if hvf_enabled" hunk * in patch 2, convert if (!tcg_enabled) with more punctual if (hax_enabled) when eating dummy APC ---- RFC v2 -> v3: * provided defaults for all methods. Only create_vcpu_thread is now a mandatory field. (Paolo) * separated new CpusAccel patch from its first user, new patch nr. 2: "cpus: prepare new CpusAccel cpu accelerator interface" * new CpusAccel methods: get_virtual_clock and get_elapsed_ticks. (Paolo) In this series, get_virtual_clock has a separate implementation between TCG/icount and qtest, while get_elapsed_ticks only returns a virtual counter for icount. Looking for more comments in this area. ---- RFC v1 -> v2: * split the cpus.c accelerator refactoring into 6 patches. * other minor changes to be able to proceed step by step. ---- * Rebased on commit 255ae6e2158c743717bed76c9a2365ee4bcd326e, "replay: notify the main loop when there are no instructions" [SPLIT into part1 and part2] ---- v6 -> v7: * rebased changes on top of Pavel Dovgalyuk changes to dma-helpers.c "icount: make dma reads deterministic" ---- v5 -> v6: * rebased changes on top of Emilio G. Cota changes to cpus.c "cpu: convert queued work to a QSIMPLEQ" * keep a pointer in cpus.c instead of a copy of CpusAccel (Alex) ---- v4 -> v5: rebase on latest master * rebased changes on top of roman series to remove one of the extra states for hvf. (Is the result now functional for HVF?) * rebased changes on top of icount changes and fixes to icount_configure and the new shift vmstate. (Markus) v3 -> v4: * overall: added copyright headers to all files that were missing them (used copyright and license of the module the stuff was extracted from). For the new interface files, added SUSE LLC. * 1/4 (move softmmu only files from root): MAINTAINERS: moved softmmu/cpus.c to its final location (from patch 2) * 2/4 (cpu-throttle): MAINTAINERS (to patch 1), copyright Fabrice Bellard and license from cpus.c * 3/4 (cpu-timers, icount): - MAINTAINERS: add cpu-timers.c and icount.c to Paolo - break very long lines (patchew) - add copyright SUSE LLC, GPLv2 to cpu-timers.h - add copyright Fabrice Bellard and license from cpus.c to timers-state.h as it is lifted from cpus.c - vl.c: in configure_accelerators bail out if icount_enabled() and !tcg_enabled() as qtest does not enable icount anymore. * 4/4 (accel stuff to accel): - add copyright SUSE LLC to files that mostly only consist of the new interface. Add whatever copyright was in the accelerator code if instead they mostly consist of accelerator code. - change a comment to mention the result of the AccelClass experiment - moved qtest accelerator into accel/qtest/ , make it like the others. - rename xxx-cpus-interface to xxx-cpus (remove "interface" from names) - rename accel_int to cpus_accel - rename CpusAccel functions from cpu_synchronize_* to synchronize_* -------- v2 -> v3: * turned into a 4 patch series, adding a first patch moving softmmu code currently in top_srcdir to softmmu/ * cpu-throttle: moved to softmmu/ * cpu-timers, icount: - moved to softmmu/ - fixed assumption of qtest_enabled() => icount_enabled() causing the failure of check-qtest-arm goal, in test-arm-mptimer.c Fix is in hw/core/ptimer.c, where the artificial timeout rate limit should not be applied under qtest_enabled(), in a similar way to how it is not applied for icount_enabled(). * CpuAccelInterface: no change. -------- v1 -> v2: * 1/3 (cpu-throttle): provide a description in the commit message * 2/3 (cpu-timers, icount): in this v2 separate icount from cpu-timers, as icount is actually TCG-specific. Only build it under CONFIG_TCG. To do this, qtest had to be detached from icount. To this end, a trivial global counter for qtest has been introduced. * 3/3 (CpuAccelInterface): provided a description. This is point 8) in that plan. The idea is to extract the unrelated parts in cpus, and register interfaces from each single accelerator to the main cpus module (cpus.c). While doing this RFC, I noticed some assumptions about Windows being either TCG or HAX (not considering WHPX) that might need to be revisited. I added a comment there. The thing builds successfully based on Linux cross-compilations for windows/hax, windows/whpx, and I got a good build on Darwin/hvf. Tests run successully for tcg and kvm configurations, but did not test on windows or darwin. Welcome your feedback and help on this, Claudio Claudio Fontana (16): cpu-timers, icount: new modules icount: rename functions to be consistent with the module name cpus: prepare new CpusAccel cpu accelerator interface cpus: extract out TCG-specific code to accel/tcg cpus: extract out qtest-specific code to accel/qtest cpus: extract out kvm-specific code to accel/kvm cpus: extract out hax-specific code to target/i386/ cpus: extract out whpx-specific code to target/i386/ cpus: extract out hvf-specific code to target/i386/hvf/ cpus: cleanup now unneeded includes cpus: remove checks for non-NULL cpus_accel cpus: add handle_interrupt to the CpusAccel interface hvf: remove hvf specific functions from global includes whpx: remove whpx specific functions from global includes hax: remove hax specific functions from global includes kvm: remove kvm specific functions from global includes MAINTAINERS | 5 +- accel/kvm/kvm-all.c | 14 +- accel/kvm/kvm-cpus.c | 88 ++ accel/kvm/kvm-cpus.h | 24 + accel/kvm/meson.build | 5 +- accel/meson.build | 2 +- accel/qtest/meson.build | 7 + accel/qtest/qtest-cpus.c | 91 ++ accel/qtest/qtest-cpus.h | 17 + accel/{ => qtest}/qtest.c | 13 +- accel/stubs/hax-stub.c | 10 - accel/stubs/hvf-stub.c | 30 - accel/stubs/kvm-stub.c | 23 - accel/stubs/meson.build | 2 - accel/stubs/whpx-stub.c | 47 - accel/tcg/cpu-exec.c | 43 +- accel/tcg/meson.build | 2 +- accel/tcg/tcg-all.c | 43 +- accel/tcg/tcg-cpus.c | 569 +++++++++++ accel/tcg/tcg-cpus.h | 17 + accel/tcg/translate-all.c | 3 +- dma-helpers.c | 4 +- docs/replay.txt | 6 +- exec.c | 4 - hw/core/cpu.c | 14 +- hw/core/ptimer.c | 8 +- hw/i386/x86.c | 3 +- include/exec/cpu-all.h | 4 + include/exec/exec-all.h | 4 +- include/hw/core/cpu.h | 14 - include/qemu/timer.h | 24 +- include/sysemu/cpu-timers.h | 90 ++ include/sysemu/cpus.h | 50 +- include/sysemu/hax.h | 17 - include/sysemu/hvf.h | 8 - include/sysemu/hw_accel.h | 69 +- include/sysemu/kvm.h | 7 - include/sysemu/qtest.h | 2 + include/sysemu/replay.h | 4 +- include/sysemu/whpx.h | 19 - replay/replay.c | 6 +- softmmu/cpu-timers.c | 279 ++++++ softmmu/cpus.c | 1697 +++----------------------------- softmmu/icount.c | 492 +++++++++ softmmu/meson.build | 10 +- softmmu/qtest.c | 34 +- softmmu/timers-state.h | 69 ++ softmmu/vl.c | 11 +- stubs/clock-warp.c | 7 - stubs/cpu-get-clock.c | 3 +- stubs/cpu-get-icount.c | 21 - stubs/cpu-synchronize-state.c | 9 + stubs/cpus-get-virtual-clock.c | 8 + stubs/icount.c | 45 + stubs/meson.build | 6 +- stubs/qemu-timer-notify-cb.c | 8 + stubs/qtest.c | 5 + target/alpha/translate.c | 3 +- target/arm/helper.c | 7 +- target/i386/hax-all.c | 17 +- target/i386/hax-cpus.c | 84 ++ target/i386/hax-cpus.h | 33 + target/i386/hax-i386.h | 2 + target/i386/hax-mem.c | 2 +- target/i386/hax-posix.c | 13 +- target/i386/hax-windows.c | 22 +- target/i386/hax-windows.h | 2 + target/i386/hvf/hvf-cpus.c | 131 +++ target/i386/hvf/hvf-cpus.h | 25 + target/i386/hvf/hvf.c | 12 +- target/i386/hvf/meson.build | 1 + target/i386/hvf/x86hvf.c | 2 + target/i386/hvf/x86hvf.h | 1 - target/i386/meson.build | 14 +- target/i386/whpx-all.c | 13 +- target/i386/whpx-cpus.c | 96 ++ target/i386/whpx-cpus.h | 34 + target/riscv/csr.c | 8 +- tests/ptimer-test-stubs.c | 5 +- tests/test-timed-average.c | 2 +- util/main-loop.c | 12 +- util/qemu-timer.c | 14 +- 82 files changed, 2640 insertions(+), 2031 deletions(-) create mode 100644 accel/kvm/kvm-cpus.c create mode 100644 accel/kvm/kvm-cpus.h create mode 100644 accel/qtest/meson.build create mode 100644 accel/qtest/qtest-cpus.c create mode 100644 accel/qtest/qtest-cpus.h rename accel/{ => qtest}/qtest.c (81%) delete mode 100644 accel/stubs/hvf-stub.c delete mode 100644 accel/stubs/whpx-stub.c create mode 100644 accel/tcg/tcg-cpus.c create mode 100644 accel/tcg/tcg-cpus.h create mode 100644 include/sysemu/cpu-timers.h create mode 100644 softmmu/cpu-timers.c create mode 100644 softmmu/icount.c create mode 100644 softmmu/timers-state.h delete mode 100644 stubs/clock-warp.c delete mode 100644 stubs/cpu-get-icount.c create mode 100644 stubs/cpu-synchronize-state.c create mode 100644 stubs/cpus-get-virtual-clock.c create mode 100644 stubs/icount.c create mode 100644 stubs/qemu-timer-notify-cb.c create mode 100644 target/i386/hax-cpus.c create mode 100644 target/i386/hax-cpus.h create mode 100644 target/i386/hvf/hvf-cpus.c create mode 100644 target/i386/hvf/hvf-cpus.h create mode 100644 target/i386/whpx-cpus.c create mode 100644 target/i386/whpx-cpus.h