Re: [Qemu-devel] [PATCH v7 02/12] target/arm: Reorganize PMCCNTR accesses

[Peter Maydell]

On 5 November 2018 at 18:51, Aaron Lindsay <address@hidden> wrote:
> pmccntr_read and pmccntr_write contained duplicate code that was already
> being handled by pmccntr_sync. Consolidate the duplicated code into two
> functions: pmccntr_op_start and pmccntr_op_finish. Add a companion to
> c15_ccnt in CPUARMState so that we can simultaneously save both the
> architectural register value and the last underlying cycle count - this
> ensures time isn't lost and will also allow us to access the 'old'
> architectural register value in order to detect overflows in later
> patches.
>
> Signed-off-by: Aaron Lindsay <address@hidden>
> Signed-off-by: Aaron Lindsay <address@hidden>


>  /**
> - * pmccntr_sync
> + * pmccntr_op_start/finish
> + * @env: CPUARMState
> + *
> + * Convert the counter in the PMCCNTR between its delta form (the typical 
> mode
> + * when it's enabled) and the guest-visible value. These two calls must 
> always
> + * surround any action which might affect the counter.
> + */
> +void pmccntr_op_start(CPUARMState *env);
> +void pmccntr_op_finish(CPUARMState *env);
> +
> +/**
> + * pmu_op_start/finish
>   * @env: CPUARMState
>   *
> - * Synchronises the counter in the PMCCNTR. This must always be called twice,
> - * once before any action that might affect the timer and again afterwards.
> - * The function is used to swap the state of the register if required.
> - * This only happens when not in user mode (!CONFIG_USER_ONLY)
> + * Convert all PMU counters between their delta form (the typical mode when
> + * they are enabled) and the guest-visible values. These two calls must
> + * surround any action which might affect the counters, and the return value
> + * from pmu_op_start must be supplied as the second argument to 
> pmu_op_finish.
>   */
> -void pmccntr_sync(CPUARMState *env);
> +void pmu_op_start(CPUARMState *env);
> +void pmu_op_finish(CPUARMState *env);

The comment says that pmu_op_start returns a value and pmu_op_finish
has a second argument, but the prototypes disagree...

Otherwise
Reviewed-by: Peter Maydell <address@hidden>
so if this turns out to be the only problem I can fix it up when
I apply it to my tree, if you provide suitable new comment text.

thanks
-- PMM