On Mon, Feb 18, 2019 at 03:10:16PM +0000, André Przywara wrote:

On Thu, 24 Jan 2019 15:00:29 +0100
Christoffer Dall [off-list ref] 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.)

quoted

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 <redacted>
Acked-by: Marc Zyngier <redacted>
---
 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

_______________________________________________
linux-arm-kernel mailing list
linux-arm-kernel@lists.infradead.org
http://lists.infradead.org/mailman/listinfo/linux-arm-kernel