Re: [Qemu-devel] [RFC PATCH] target-arm: kvm: Differentiate registers based on write-back levels

[Christoffer Dall <christoffer.dall@linaro.org>]

On Fri, Jul 10, 2015 at 12:22:31PM +0100, Peter Maydell wrote:
> On 10 July 2015 at 12:00, Christoffer Dall <christoffer.dall@linaro.org> wrote:
> > Some registers like the CNTVCT register should only be written to the
> > kernel as part of machine initialization or on vmload operations, but
> > never during runtime, as this can potentially make time go backwards or
> > create inconsistent time observations between VCPUs.
> >
> > Introduce a list of registers that should not be written back at runtime
> > and check this list on syncing the register state to the KVM state.
> 
> Thanks for picking this one up...
> 
> > Signed-off-by: Christoffer Dall <christoffer.dall@linaro.org>
> > ---
> >  target-arm/kvm.c     | 34 +++++++++++++++++++++++++++++++++-
> >  target-arm/kvm32.c   |  2 +-
> >  target-arm/kvm64.c   |  2 +-
> >  target-arm/kvm_arm.h |  3 ++-
> >  target-arm/machine.c |  2 +-
> >  5 files changed, 38 insertions(+), 5 deletions(-)
> >
> > diff --git a/target-arm/kvm.c b/target-arm/kvm.c
> > index 548bfd7..2e92699 100644
> > --- a/target-arm/kvm.c
> > +++ b/target-arm/kvm.c
> > @@ -409,7 +409,35 @@ bool write_kvmstate_to_list(ARMCPU *cpu)
> >      return ok;
> >  }
> >
> > -bool write_list_to_kvmstate(ARMCPU *cpu)
> > +typedef struct cpreg_state_level {
> > +    uint64_t kvm_idx;
> > +    int level;
> > +} cpreg_state_level;
> 
> (QEMU's coding style prefers CPRegStateLevel for struct types.)
> 

ok

> > +
> > +/* All system registers not listed in the following table are assumed to be
> > + * of the level KVM_PUT_RUNTIME_STATE, a register should be written less
> > + * often, you must add it to this table with a state of either
> > + * KVM_PUT_RESET_STATE or KVM_PUT_FULL_STATE.
> > + */
> > +cpreg_state_level non_runtime_cpregs[] = {
> > +    { KVM_REG_ARM_TIMER_CNT, KVM_PUT_FULL_STATE },
> 
> This should be KVM_PUT_RESET_STATE, right?
> 
should it?  If you reset a real machine, you will not necessarily see a
counter value of zero will you?

I guess this depends on whether QEMU reset means power the system
completely off and then on again, or some softer reset?

> > +};
> 
> The other option here would be to keep the level information
> in the cpreg structs (which is where we put everything else
> we know about cpregs); we'd probably need to then initialise
> some other data structure if we wanted to avoid the hash
> table lookup for every register in write_list_to_kvmstate.
> 
> I guess if we expect this list to remain a fairly small
> set of exceptional cases then this is OK (and vaguely
> comparable to the existing kvm_arm_reg_syncs_via-cpreg_list
> handling).

I thought about this too, and sent this as an RFC for exactly this
reason.  I did it this way initially for two reasons: (1) I don't
understand the hash-table register initialization flow for aarch64 and
(2) I could really only identify this single register for now that needs
to be marked as a non-runtime register, and then this is less invasive.

> 
> Don't we need separate 32-bit and 64-bit versions of
> this list?
> 
Do we?  I thought this file would compile separately for the 32-bit and
64-bit versions and the register index define is the same name for both
architectures, did I get this wrong?

Of course, for other registers with unique-to-32-bit-or-64-bit reg index
defines, yes, we would need a separate table.  Should they then be
defined in the kvm32.c and kvm64.c and passed in as a pointer to
write_kvmstate_to_list() ?

Thanks,
-Christoffer