Re: [Qemu-arm] [PATCH v4 03/21] target/arm: Reorganize PMCCNTR accesses

[Aaron Lindsay]

On Apr 20 11:17, Peter Maydell wrote:
> On 17 April 2018 at 21:37, 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
> > ensure 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>
> > ---
> >  target/arm/cpu.h    |  28 ++++++++++-----
> >  target/arm/helper.c | 100 
> > ++++++++++++++++++++++++++++------------------------
> >  2 files changed, 73 insertions(+), 55 deletions(-)
> >
> > diff --git a/target/arm/cpu.h b/target/arm/cpu.h
> > index 19a0c03..04041db 100644
> > --- a/target/arm/cpu.h
> > +++ b/target/arm/cpu.h
> > @@ -454,10 +454,20 @@ typedef struct CPUARMState {
> >          uint64_t oslsr_el1; /* OS Lock Status */
> >          uint64_t mdcr_el2;
> >          uint64_t mdcr_el3;
> > -        /* If the counter is enabled, this stores the last time the counter
> > -         * was reset. Otherwise it stores the counter value
> > +        /* Stores the architectural value of the counter *the last time it 
> > was
> > +         * updated* by pmccntr_op_start. Accesses should always be 
> > surrounded
> > +         * by pmccntr_op_start/pmccntr_op_finish to guarantee the latest
> > +         * architecturally-corect value is being read/set.
> >           */
> >          uint64_t c15_ccnt;
> > +        /* Stores the delta between the architectural value and the 
> > underlying
> > +         * cycle count during normal operation. It is used to update 
> > c15_ccnt
> > +         * to be the correct architectural value before accesses. During
> > +         * accesses, c15_ccnt_delta contains the underlying count being 
> > used
> > +         * for the access, after which it reverts to the delta value in
> > +         * pmccntr_op_finish.
> > +         */
> > +        uint64_t c15_ccnt_delta;
> 
> So the key question here is: how does this work for VM migration?

To be honest, I'm not sure I fully understand the things I need to be
looking out for with VM migration.

My guess, though, is that this current implementation is not sufficient.
Perhaps there needs to be logic to ensure that c15_ccnt is the current
architectural value before migration and also to setup c15_ccnt_delta to
be the delta between that architectural value and the underlying cycle
count upon inbound migration. Does that sound like an approach which
would fit well within the rest of the migration framework?

-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.