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@xxxxxxx> 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@xxxxxxx>
> > Acked-by: Marc Zyngier <marc.zyngier@xxxxxxx>
> > ---
> > 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
