Re: [PATCH 11/14] KVM: arm/arm64: timer: Rework data structures for multiple timers

[Christoffer Dall <christoffer.dall@arm.com>]

On Mon, Feb 18, 2019 at 03:10:16PM +0000, André Przywara wrote:
> On Thu, 24 Jan 2019 15:00:29 +0100
> Christoffer Dall <christoffer.dall@arm.com> wrote:
> 
> Hi,
> 
> I already looked at most of these patches earlier, without finding
> serious issues, but figured I would give those without any Reviewed-by:
> or Acked-by: tags a closer look.
> (This patch just carries a S-o-b: tag from Marc in the kvm-arm git repo.)
> 
> > Prepare for having 4 timer data structures (2 for now).
> > 
> > Change loaded to an enum so that we know not just whether *some* state
> > is loaded on the CPU, but also *which* state is loaded.
> > 
> > Move loaded to the cpu data structure and not the individual timer
> > structure, in preparation for assigning the EL1 phys timer as well.
> > 
> > Signed-off-by: Christoffer Dall <christoffer.dall@arm.com>
> > Acked-by: Marc Zyngier <marc.zyngier@arm.com>
> > ---
> >  include/kvm/arm_arch_timer.h | 44 ++++++++++++++-------------
> >  virt/kvm/arm/arch_timer.c    | 58 +++++++++++++++++++-----------------
> >  2 files changed, 54 insertions(+), 48 deletions(-)
> > 
> > diff --git a/include/kvm/arm_arch_timer.h b/include/kvm/arm_arch_timer.h
> > index d26b7fde9935..d40fe57a2d0d 100644
> > --- a/include/kvm/arm_arch_timer.h
> > +++ b/include/kvm/arm_arch_timer.h
> > @@ -36,6 +36,8 @@ enum kvm_arch_timer_regs {
> >  };
> >  
> >  struct arch_timer_context {
> > +	struct kvm_vcpu			*vcpu;
> > +
> >  	/* Registers: control register, timer value */
> >  	u32				cnt_ctl;
> >  	u64				cnt_cval;
> > @@ -43,32 +45,34 @@ struct arch_timer_context {
> >  	/* Timer IRQ */
> >  	struct kvm_irq_level		irq;
> >  
> > -	/*
> > -	 * We have multiple paths which can save/restore the timer state
> > -	 * onto the hardware, so we need some way of keeping track of
> > -	 * where the latest state is.
> > -	 *
> > -	 * loaded == true:  State is loaded on the hardware registers.
> > -	 * loaded == false: State is stored in memory.
> > -	 */
> > -	bool			loaded;
> > -
> >  	/* Virtual offset */
> > -	u64			cntvoff;
> > +	u64				cntvoff;
> > +
> > +	/* Emulated Timer (may be unused) */
> > +	struct hrtimer			hrtimer;
> > +};
> > +
> > +enum loaded_timer_state {
> > +	TIMER_NOT_LOADED,
> > +	TIMER_EL1_LOADED,
> 
> So this gets reverted in PATCH 13/14, and I don't see it reappearing in
> the nv series later on.
> Is that just needed for assigning the phys timer in the next patch, and
> gets obsolete with the timer_map?
> Or is this a rebase artefact and we don't actually need this?

I think this is a rebase problem and we could have optimized this out to
reduce the patch diff.  The end result is the same though.

> 
> The rest of the patch looks like valid transformations to me.
> 
Thanks for having a look.

    Christoffer
_______________________________________________
kvmarm mailing list
kvmarm@lists.cs.columbia.edu
https://lists.cs.columbia.edu/mailman/listinfo/kvmarm