Re: [Qemu-devel] [PATCH 06/13] target/arm: Filter cycle counter based on PMCCFILTR_EL0

[Peter Maydell]

On 30 September 2017 at 03:08, Aaron Lindsay <address@hidden> wrote:
> The pmu_counter_filtered and pmu_sync functions are generic (as opposed
> to PMCCNTR-specific) to allow for the implementation of other events.
>
> RFC: I know that many of the locations of the calls to pmu_sync are
> problematic when icount is enabled because can_do_io will not be set.
> The documentation says that for deterministic execution, IO must only be
> performed by the last instruction of a thread block.

Yes. You need to arrange that gen_io_start() and gen_io_end()
are called around the generation of code for operations that might
do IO or care about the state of the clock, and that we end the TB
after gen_io_end().

> Because
> cpu_handle_interrupt() and cpu_handle_exception() are actually made
> outside of a thread block, is it safe to set can_do_io=1 for them to
> allow this to succeed? Is there a better mechanism for handling this?

>From my reading of the code, can_do_io should already be 1
when these functions are called. It's only set to 0 just
before we call tcg_qemu_tb_exec() and then set back to 1
immediately after (see cpu_tb_exec()).

In general, the approach you have here looks like it's going to
be pretty invasive and also hard to keep working right. I think
we can come up with something a bit better.

Specifically, the filtering criteria effectively only change
when we change exception level, right? (since you can only
change security state as part of an exception level change).
We already have a mechanism for getting a callback when the EL
changes -- arm_register_el_change_hook(). (We would need to
upgrade it to support more than one hook function.)

You still need to get the io-count handling right, but there
are many fewer places that need to change (just the codegen
for calls to exception-return helpers, I think) and they already
end the TB, so you don't have the complexity of trying to end the
TB in places you didn't before, you just need the gen_io_start/end.

thanks
-- PMM