On Oct 17 15:57, Peter Maydell wrote: > On 30 September 2017 at 03:08, Aaron Lindsay <alind...@codeaurora.org> 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.
Yes, I am hoping so. > 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. Using hooks/callbacks is clearly a better solution. I shied away from using *el_change_hook in this patchset because A) it seemed to be marked explicitly for GICv3 emulation, B) the one-hook limitation you mentioned, and C) it doesn't currently provide you with which EL you're coming from. If everyone is amenable to changing how the hook is structured, I don't think any of those reasons stand. My initial thought is that the best way to fix C) is to add a pre_el_change_hook to be called at the top of the exception_return and cpsr_write_eret helpers in target/arm/op_helper.c to drive the first call to pmu_sync() (or whatever I rename the first half of that pair to based on your suggestions on that patch). If I don't receive any additional feedback before then, I'll do my best to adapt the existing hooks to allow for a cleaner approach to filtering for v3. -Aaron -- Qualcomm Datacenter Technologies as an affiliate of Qualcomm Technologies, Inc. Qualcomm Technologies, Inc. is a member of the Code Aurora Forum, a Linux Foundation Collaborative Project.
