Groups | Search | Server Info | Keyboard shortcuts | Login | Register [http] [https] [nntp] [nntps]

Groups > linux.kernel > #1567855 > unrolled thread

[RFC v2 00/10] Provide the EL1 physical timer to the VM

Back to article view | Back to linux.kernel

Contents

  [RFC v2 00/10] Provide the EL1 physical timer to the VM Jintack Lim <jintack@cs.columbia.edu> - 2017-01-27 02:10 +0100
    [RFC v2 04/10] KVM: arm/arm64: Add the EL1 physical timer context Jintack Lim <jintack@cs.columbia.edu> - 2017-01-27 02:10 +0100
    [RFC v2 05/10] KVM: arm/arm64: Initialize the emulated EL1 physical timer Jintack Lim <jintack@cs.columbia.edu> - 2017-01-27 02:10 +0100
      Re: [RFC v2 05/10] KVM: arm/arm64: Initialize the emulated EL1 physical timer Marc Zyngier <marc.zyngier@arm.com> - 2017-01-29 13:20 +0100
        Re: [RFC v2 05/10] KVM: arm/arm64: Initialize the emulated EL1  physical timer Christoffer Dall <christoffer.dall@linaro.org> - 2017-01-30 16:00 +0100
          Re: [RFC v2 05/10] KVM: arm/arm64: Initialize the emulated EL1  physical timer Marc Zyngier <marc.zyngier@arm.com> - 2017-01-30 18:50 +0100
            Re: [RFC v2 05/10] KVM: arm/arm64: Initialize the emulated EL1  physical timer Christoffer Dall <christoffer.dall@linaro.org> - 2017-01-30 20:10 +0100
              Re: [RFC v2 05/10] KVM: arm/arm64: Initialize the emulated EL1  physical timer Marc Zyngier <marc.zyngier@arm.com> - 2017-02-01 11:10 +0100
    [RFC v2 01/10] KVM: arm/arm64: Abstract virtual timer context into separate structure Jintack Lim <jintack@cs.columbia.edu> - 2017-01-27 02:10 +0100
      Re: [RFC v2 01/10] KVM: arm/arm64: Abstract virtual timer context into separate structure Marc Zyngier <marc.zyngier@arm.com> - 2017-01-29 12:50 +0100
    Re: [RFC v2 00/10] Provide the EL1 physical timer to the VM Marc Zyngier <marc.zyngier@arm.com> - 2017-01-29 17:00 +0100
      Re: [RFC v2 00/10] Provide the EL1 physical timer to the VM Jintack Lim <jintack@cs.columbia.edu> - 2017-01-30 20:20 +0100

#1567855 — [RFC v2 00/10] Provide the EL1 physical timer to the VM

The ARM architecture defines the EL1 physical timer and the virtual timer,
and it is reasonable for an OS to expect to be able to access both.
However, the current KVM implementation does not provide the EL1 physical
timer to VMs but terminates VMs on access to the timer.

This patch series enables VMs to use the EL1 physical timer through
trap-and-emulate.  The KVM host emulates each EL1 physical timer register
access and sets up the background timer accordingly.  When the background
timer expires, the KVM host injects EL1 physical timer interrupts to the
VM.  Alternatively, it's also possible to allow VMs to access the EL1
physical timer without trapping.  However, this requires somehow using the
EL2 physical timer for the Linux host while running the VM instead of the
EL1 physical timer.  Right now I just implemented trap-and-emulate because
this was straightforward to do, and I leave it to future work to determine
if transferring the EL1 physical timer state to the EL2 timer provides any
performance benefit.

This feature will be useful for any OS that wishes to access the EL1
physical timer. Nested virtualization is one of those use cases. A nested
hypervisor running inside a VM would think it has full access to the
hardware and naturally tries to use the EL1 physical timer as Linux would
do. Other nested hypervisors may try to use the EL2 physical timer as Xen
would do, but supporting the EL2 physical timer to the VM is out of scope
of this patch series. This patch series will make it easy to add the EL2
timer support in the future, though.

Note that Linux VMs booting in EL1 will be unaffected by this patch series
and will continue to use only the virtual timer and this patch series will
therefore not introduce any performance degredation as a result of
trap-and-emulate.

v1 => v2:
 - Rebase on kvm-arm-for-4.10-rc4
 - To make it simple, schedule the background timer for the EL1 physical timer
   emulation on every entry to the VM and cancel it on exit.
 - Change timer_context structure to have cntvoff and restore enable field back
   to arch_timer_cpu structure

Jintack Lim (10):
  KVM: arm/arm64: Abstract virtual timer context into separate structure
  KVM: arm/arm64: Move cntvoff to each timer context
  KVM: arm/arm64: Decouple kvm timer functions from virtual timer
  KVM: arm/arm64: Add the EL1 physical timer context
  KVM: arm/arm64: Initialize the emulated EL1 physical timer
  KVM: arm/arm64: Update the physical timer interrupt level
  KVM: arm/arm64: Set a background timer to the earliest timer
    expiration
  KVM: arm/arm64: Set up a background timer for the physical timer
    emulation
  KVM: arm64: Add the EL1 physical timer access handler
  KVM: arm/arm64: Emulate the EL1 phys timer register access

 arch/arm/include/asm/kvm_host.h   |   6 +-
 arch/arm/kvm/arm.c                |   3 +-
 arch/arm/kvm/reset.c              |   9 +-
 arch/arm64/include/asm/kvm_host.h |   4 +-
 arch/arm64/kvm/reset.c            |   9 +-
 arch/arm64/kvm/sys_regs.c         |  60 +++++++++++
 include/kvm/arm_arch_timer.h      |  39 ++++----
 virt/kvm/arm/arch_timer.c         | 204 ++++++++++++++++++++++++++++----------
 virt/kvm/arm/hyp/timer-sr.c       |  13 +--
 9 files changed, 261 insertions(+), 86 deletions(-)

-- 
1.9.1

[toc] | [next] | [standalone]

#1567856 — [RFC v2 04/10] KVM: arm/arm64: Add the EL1 physical timer context

Add the EL1 physical timer context.

Signed-off-by: Jintack Lim <jintack@cs.columbia.edu>
---
 include/kvm/arm_arch_timer.h | 2 ++
 1 file changed, 2 insertions(+)

diff --git a/include/kvm/arm_arch_timer.h b/include/kvm/arm_arch_timer.h
index d921d20..69f648b 100644
--- a/include/kvm/arm_arch_timer.h
+++ b/include/kvm/arm_arch_timer.h
@@ -40,6 +40,7 @@ struct arch_timer_context {
 
 struct arch_timer_cpu {
 	struct arch_timer_context	vtimer;
+	struct arch_timer_context	ptimer;
 
 	/* Background timer used when the guest is not running */
 	struct hrtimer			timer;
@@ -77,4 +78,5 @@ bool kvm_timer_should_fire(struct kvm_vcpu *vcpu,
 void kvm_timer_init_vhe(void);
 
 #define vcpu_vtimer(v)	(&(v)->arch.timer_cpu.vtimer)
+#define vcpu_ptimer(v)	(&(v)->arch.timer_cpu.ptimer)
 #endif
-- 
1.9.1

[toc] | [prev] | [next] | [standalone]

#1567857 — [RFC v2 05/10] KVM: arm/arm64: Initialize the emulated EL1 physical timer

Initialize the emulated EL1 physical timer with the default irq number.

Signed-off-by: Jintack Lim <jintack@cs.columbia.edu>
---
 arch/arm/kvm/reset.c         | 9 ++++++++-
 arch/arm64/kvm/reset.c       | 9 ++++++++-
 include/kvm/arm_arch_timer.h | 3 ++-
 virt/kvm/arm/arch_timer.c    | 9 +++++++--
 4 files changed, 25 insertions(+), 5 deletions(-)

diff --git a/arch/arm/kvm/reset.c b/arch/arm/kvm/reset.c
index 4b5e802..1da8b2d 100644
--- a/arch/arm/kvm/reset.c
+++ b/arch/arm/kvm/reset.c
@@ -37,6 +37,11 @@
 	.usr_regs.ARM_cpsr = SVC_MODE | PSR_A_BIT | PSR_I_BIT | PSR_F_BIT,
 };
 
+static const struct kvm_irq_level cortexa_ptimer_irq = {
+	{ .irq = 30 },
+	.level = 1,
+};
+
 static const struct kvm_irq_level cortexa_vtimer_irq = {
 	{ .irq = 27 },
 	.level = 1,
@@ -58,6 +63,7 @@ int kvm_reset_vcpu(struct kvm_vcpu *vcpu)
 {
 	struct kvm_regs *reset_regs;
 	const struct kvm_irq_level *cpu_vtimer_irq;
+	const struct kvm_irq_level *cpu_ptimer_irq;
 
 	switch (vcpu->arch.target) {
 	case KVM_ARM_TARGET_CORTEX_A7:
@@ -65,6 +71,7 @@ int kvm_reset_vcpu(struct kvm_vcpu *vcpu)
 		reset_regs = &cortexa_regs_reset;
 		vcpu->arch.midr = read_cpuid_id();
 		cpu_vtimer_irq = &cortexa_vtimer_irq;
+		cpu_ptimer_irq = &cortexa_ptimer_irq;
 		break;
 	default:
 		return -ENODEV;
@@ -77,5 +84,5 @@ int kvm_reset_vcpu(struct kvm_vcpu *vcpu)
 	kvm_reset_coprocs(vcpu);
 
 	/* Reset arch_timer context */
-	return kvm_timer_vcpu_reset(vcpu, cpu_vtimer_irq);
+	return kvm_timer_vcpu_reset(vcpu, cpu_vtimer_irq, cpu_ptimer_irq);
 }
diff --git a/arch/arm64/kvm/reset.c b/arch/arm64/kvm/reset.c
index e95d4f6..d9e9697 100644
--- a/arch/arm64/kvm/reset.c
+++ b/arch/arm64/kvm/reset.c
@@ -46,6 +46,11 @@
 			COMPAT_PSR_I_BIT | COMPAT_PSR_F_BIT),
 };
 
+static const struct kvm_irq_level default_ptimer_irq = {
+	.irq	= 30,
+	.level	= 1,
+};
+
 static const struct kvm_irq_level default_vtimer_irq = {
 	.irq	= 27,
 	.level	= 1,
@@ -104,6 +109,7 @@ int kvm_arch_dev_ioctl_check_extension(struct kvm *kvm, long ext)
 int kvm_reset_vcpu(struct kvm_vcpu *vcpu)
 {
 	const struct kvm_irq_level *cpu_vtimer_irq;
+	const struct kvm_irq_level *cpu_ptimer_irq;
 	const struct kvm_regs *cpu_reset;
 
 	switch (vcpu->arch.target) {
@@ -117,6 +123,7 @@ int kvm_reset_vcpu(struct kvm_vcpu *vcpu)
 		}
 
 		cpu_vtimer_irq = &default_vtimer_irq;
+		cpu_ptimer_irq = &default_ptimer_irq;
 		break;
 	}
 
@@ -130,5 +137,5 @@ int kvm_reset_vcpu(struct kvm_vcpu *vcpu)
 	kvm_pmu_vcpu_reset(vcpu);
 
 	/* Reset timer */
-	return kvm_timer_vcpu_reset(vcpu, cpu_vtimer_irq);
+	return kvm_timer_vcpu_reset(vcpu, cpu_vtimer_irq, cpu_ptimer_irq);
 }
diff --git a/include/kvm/arm_arch_timer.h b/include/kvm/arm_arch_timer.h
index 69f648b..a364593 100644
--- a/include/kvm/arm_arch_timer.h
+++ b/include/kvm/arm_arch_timer.h
@@ -59,7 +59,8 @@ struct arch_timer_cpu {
 int kvm_timer_enable(struct kvm_vcpu *vcpu);
 void kvm_timer_init(struct kvm *kvm);
 int kvm_timer_vcpu_reset(struct kvm_vcpu *vcpu,
-			 const struct kvm_irq_level *irq);
+			 const struct kvm_irq_level *virt_irq,
+			 const struct kvm_irq_level *phys_irq);
 void kvm_timer_vcpu_init(struct kvm_vcpu *vcpu);
 void kvm_timer_flush_hwstate(struct kvm_vcpu *vcpu);
 void kvm_timer_sync_hwstate(struct kvm_vcpu *vcpu);
diff --git a/virt/kvm/arm/arch_timer.c b/virt/kvm/arm/arch_timer.c
index f72005a..0f6e935 100644
--- a/virt/kvm/arm/arch_timer.c
+++ b/virt/kvm/arm/arch_timer.c
@@ -329,9 +329,11 @@ void kvm_timer_sync_hwstate(struct kvm_vcpu *vcpu)
 }
 
 int kvm_timer_vcpu_reset(struct kvm_vcpu *vcpu,
-			 const struct kvm_irq_level *irq)
+			 const struct kvm_irq_level *virt_irq,
+			 const struct kvm_irq_level *phys_irq)
 {
 	struct arch_timer_context *vtimer = vcpu_vtimer(vcpu);
+	struct arch_timer_context *ptimer = vcpu_ptimer(vcpu);
 
 	/*
 	 * The vcpu timer irq number cannot be determined in
@@ -339,7 +341,8 @@ int kvm_timer_vcpu_reset(struct kvm_vcpu *vcpu,
 	 * kvm_vcpu_set_target(). To handle this, we determine
 	 * vcpu timer irq number when the vcpu is reset.
 	 */
-	vtimer->irq.irq = irq->irq;
+	vtimer->irq.irq = virt_irq->irq;
+	ptimer->irq.irq = phys_irq->irq;
 
 	/*
 	 * The bits in CNTV_CTL are architecturally reset to UNKNOWN for ARMv8
@@ -348,6 +351,7 @@ int kvm_timer_vcpu_reset(struct kvm_vcpu *vcpu,
 	 * the ARMv7 architecture.
 	 */
 	vtimer->cnt_ctl = 0;
+	ptimer->cnt_ctl = 0;
 	kvm_timer_update_state(vcpu);
 
 	return 0;
@@ -369,6 +373,7 @@ void kvm_timer_vcpu_init(struct kvm_vcpu *vcpu)
 	struct arch_timer_cpu *timer = &vcpu->arch.timer_cpu;
 
 	update_vtimer_cntvoff(vcpu, kvm_phys_timer_read());
+	vcpu_ptimer(vcpu)->cntvoff = 0;
 
 	INIT_WORK(&timer->expired, kvm_timer_inject_irq_work);
 	hrtimer_init(&timer->timer, CLOCK_MONOTONIC, HRTIMER_MODE_ABS);
-- 
1.9.1

[toc] | [prev] | [next] | [standalone]

#1569179 — Re: [RFC v2 05/10] KVM: arm/arm64: Initialize the emulated EL1 physical timer

On Fri, Jan 27 2017 at 01:04:55 AM, Jintack Lim <jintack@cs.columbia.edu> wrote:
> Initialize the emulated EL1 physical timer with the default irq number.
>
> Signed-off-by: Jintack Lim <jintack@cs.columbia.edu>
> ---
>  arch/arm/kvm/reset.c         | 9 ++++++++-
>  arch/arm64/kvm/reset.c       | 9 ++++++++-
>  include/kvm/arm_arch_timer.h | 3 ++-
>  virt/kvm/arm/arch_timer.c    | 9 +++++++--
>  4 files changed, 25 insertions(+), 5 deletions(-)
>
> diff --git a/arch/arm/kvm/reset.c b/arch/arm/kvm/reset.c
> index 4b5e802..1da8b2d 100644
> --- a/arch/arm/kvm/reset.c
> +++ b/arch/arm/kvm/reset.c
> @@ -37,6 +37,11 @@
>  	.usr_regs.ARM_cpsr = SVC_MODE | PSR_A_BIT | PSR_I_BIT | PSR_F_BIT,
>  };
>  
> +static const struct kvm_irq_level cortexa_ptimer_irq = {
> +	{ .irq = 30 },
> +	.level = 1,
> +};

At some point, we'll have to make that discoverable/configurable. Maybe
the time for a discoverable arch timer has come (see below).

> +
>  static const struct kvm_irq_level cortexa_vtimer_irq = {
>  	{ .irq = 27 },
>  	.level = 1,
> @@ -58,6 +63,7 @@ int kvm_reset_vcpu(struct kvm_vcpu *vcpu)
>  {
>  	struct kvm_regs *reset_regs;
>  	const struct kvm_irq_level *cpu_vtimer_irq;
> +	const struct kvm_irq_level *cpu_ptimer_irq;
>  
>  	switch (vcpu->arch.target) {
>  	case KVM_ARM_TARGET_CORTEX_A7:
> @@ -65,6 +71,7 @@ int kvm_reset_vcpu(struct kvm_vcpu *vcpu)
>  		reset_regs = &cortexa_regs_reset;
>  		vcpu->arch.midr = read_cpuid_id();
>  		cpu_vtimer_irq = &cortexa_vtimer_irq;
> +		cpu_ptimer_irq = &cortexa_ptimer_irq;
>  		break;
>  	default:
>  		return -ENODEV;
> @@ -77,5 +84,5 @@ int kvm_reset_vcpu(struct kvm_vcpu *vcpu)
>  	kvm_reset_coprocs(vcpu);
>  
>  	/* Reset arch_timer context */
> -	return kvm_timer_vcpu_reset(vcpu, cpu_vtimer_irq);
> +	return kvm_timer_vcpu_reset(vcpu, cpu_vtimer_irq, cpu_ptimer_irq);
>  }
> diff --git a/arch/arm64/kvm/reset.c b/arch/arm64/kvm/reset.c
> index e95d4f6..d9e9697 100644
> --- a/arch/arm64/kvm/reset.c
> +++ b/arch/arm64/kvm/reset.c
> @@ -46,6 +46,11 @@
>  			COMPAT_PSR_I_BIT | COMPAT_PSR_F_BIT),
>  };
>  
> +static const struct kvm_irq_level default_ptimer_irq = {
> +	.irq	= 30,
> +	.level	= 1,
> +};
> +
>  static const struct kvm_irq_level default_vtimer_irq = {
>  	.irq	= 27,
>  	.level	= 1,
> @@ -104,6 +109,7 @@ int kvm_arch_dev_ioctl_check_extension(struct kvm *kvm, long ext)
>  int kvm_reset_vcpu(struct kvm_vcpu *vcpu)
>  {
>  	const struct kvm_irq_level *cpu_vtimer_irq;
> +	const struct kvm_irq_level *cpu_ptimer_irq;
>  	const struct kvm_regs *cpu_reset;
>  
>  	switch (vcpu->arch.target) {
> @@ -117,6 +123,7 @@ int kvm_reset_vcpu(struct kvm_vcpu *vcpu)
>  		}
>  
>  		cpu_vtimer_irq = &default_vtimer_irq;
> +		cpu_ptimer_irq = &default_ptimer_irq;
>  		break;
>  	}
>  
> @@ -130,5 +137,5 @@ int kvm_reset_vcpu(struct kvm_vcpu *vcpu)
>  	kvm_pmu_vcpu_reset(vcpu);
>  
>  	/* Reset timer */
> -	return kvm_timer_vcpu_reset(vcpu, cpu_vtimer_irq);
> +	return kvm_timer_vcpu_reset(vcpu, cpu_vtimer_irq, cpu_ptimer_irq);
>  }
> diff --git a/include/kvm/arm_arch_timer.h b/include/kvm/arm_arch_timer.h
> index 69f648b..a364593 100644
> --- a/include/kvm/arm_arch_timer.h
> +++ b/include/kvm/arm_arch_timer.h
> @@ -59,7 +59,8 @@ struct arch_timer_cpu {
>  int kvm_timer_enable(struct kvm_vcpu *vcpu);
>  void kvm_timer_init(struct kvm *kvm);
>  int kvm_timer_vcpu_reset(struct kvm_vcpu *vcpu,
> -			 const struct kvm_irq_level *irq);
> +			 const struct kvm_irq_level *virt_irq,
> +			 const struct kvm_irq_level *phys_irq);
>  void kvm_timer_vcpu_init(struct kvm_vcpu *vcpu);
>  void kvm_timer_flush_hwstate(struct kvm_vcpu *vcpu);
>  void kvm_timer_sync_hwstate(struct kvm_vcpu *vcpu);
> diff --git a/virt/kvm/arm/arch_timer.c b/virt/kvm/arm/arch_timer.c
> index f72005a..0f6e935 100644
> --- a/virt/kvm/arm/arch_timer.c
> +++ b/virt/kvm/arm/arch_timer.c
> @@ -329,9 +329,11 @@ void kvm_timer_sync_hwstate(struct kvm_vcpu *vcpu)
>  }
>  
>  int kvm_timer_vcpu_reset(struct kvm_vcpu *vcpu,
> -			 const struct kvm_irq_level *irq)
> +			 const struct kvm_irq_level *virt_irq,
> +			 const struct kvm_irq_level *phys_irq)
>  {
>  	struct arch_timer_context *vtimer = vcpu_vtimer(vcpu);
> +	struct arch_timer_context *ptimer = vcpu_ptimer(vcpu);
>  
>  	/*
>  	 * The vcpu timer irq number cannot be determined in
> @@ -339,7 +341,8 @@ int kvm_timer_vcpu_reset(struct kvm_vcpu *vcpu,
>  	 * kvm_vcpu_set_target(). To handle this, we determine
>  	 * vcpu timer irq number when the vcpu is reset.
>  	 */
> -	vtimer->irq.irq = irq->irq;
> +	vtimer->irq.irq = virt_irq->irq;
> +	ptimer->irq.irq = phys_irq->irq;
>  
>  	/*
>  	 * The bits in CNTV_CTL are architecturally reset to UNKNOWN for ARMv8
> @@ -348,6 +351,7 @@ int kvm_timer_vcpu_reset(struct kvm_vcpu *vcpu,
>  	 * the ARMv7 architecture.
>  	 */
>  	vtimer->cnt_ctl = 0;
> +	ptimer->cnt_ctl = 0;
>  	kvm_timer_update_state(vcpu);
>  
>  	return 0;
> @@ -369,6 +373,7 @@ void kvm_timer_vcpu_init(struct kvm_vcpu *vcpu)
>  	struct arch_timer_cpu *timer = &vcpu->arch.timer_cpu;
>  
>  	update_vtimer_cntvoff(vcpu, kvm_phys_timer_read());
> +	vcpu_ptimer(vcpu)->cntvoff = 0;

This is quite contentious, IMHO. Do we really want to expose the delta
between the virtual and physical counters? That's a clear indication to
the guest that it is virtualized. I"m not sure it matters, but I think
we should at least make a conscious choice, and maybe give the
opportunity to userspace to select the desired behaviour.

>  
>  	INIT_WORK(&timer->expired, kvm_timer_inject_irq_work);
>  	hrtimer_init(&timer->timer, CLOCK_MONOTONIC, HRTIMER_MODE_ABS);

Thanks,

	M.
-- 
Jazz is not dead. It just smells funny.

[toc] | [prev] | [next] | [standalone]

#1569801 — Re: [RFC v2 05/10] KVM: arm/arm64: Initialize the emulated EL1 physical timer

On Sun, Jan 29, 2017 at 12:07:48PM +0000, Marc Zyngier wrote:
> On Fri, Jan 27 2017 at 01:04:55 AM, Jintack Lim <jintack@cs.columbia.edu> wrote:
> > Initialize the emulated EL1 physical timer with the default irq number.
> >
> > Signed-off-by: Jintack Lim <jintack@cs.columbia.edu>
> > ---
> >  arch/arm/kvm/reset.c         | 9 ++++++++-
> >  arch/arm64/kvm/reset.c       | 9 ++++++++-
> >  include/kvm/arm_arch_timer.h | 3 ++-
> >  virt/kvm/arm/arch_timer.c    | 9 +++++++--
> >  4 files changed, 25 insertions(+), 5 deletions(-)
> >
> > diff --git a/arch/arm/kvm/reset.c b/arch/arm/kvm/reset.c
> > index 4b5e802..1da8b2d 100644
> > --- a/arch/arm/kvm/reset.c
> > +++ b/arch/arm/kvm/reset.c
> > @@ -37,6 +37,11 @@
> >  	.usr_regs.ARM_cpsr = SVC_MODE | PSR_A_BIT | PSR_I_BIT | PSR_F_BIT,
> >  };
> >  
> > +static const struct kvm_irq_level cortexa_ptimer_irq = {
> > +	{ .irq = 30 },
> > +	.level = 1,
> > +};
> 
> At some point, we'll have to make that discoverable/configurable. Maybe
> the time for a discoverable arch timer has come (see below).
> 
> > +
> >  static const struct kvm_irq_level cortexa_vtimer_irq = {
> >  	{ .irq = 27 },
> >  	.level = 1,
> > @@ -58,6 +63,7 @@ int kvm_reset_vcpu(struct kvm_vcpu *vcpu)
> >  {
> >  	struct kvm_regs *reset_regs;
> >  	const struct kvm_irq_level *cpu_vtimer_irq;
> > +	const struct kvm_irq_level *cpu_ptimer_irq;
> >  
> >  	switch (vcpu->arch.target) {
> >  	case KVM_ARM_TARGET_CORTEX_A7:
> > @@ -65,6 +71,7 @@ int kvm_reset_vcpu(struct kvm_vcpu *vcpu)
> >  		reset_regs = &cortexa_regs_reset;
> >  		vcpu->arch.midr = read_cpuid_id();
> >  		cpu_vtimer_irq = &cortexa_vtimer_irq;
> > +		cpu_ptimer_irq = &cortexa_ptimer_irq;
> >  		break;
> >  	default:
> >  		return -ENODEV;
> > @@ -77,5 +84,5 @@ int kvm_reset_vcpu(struct kvm_vcpu *vcpu)
> >  	kvm_reset_coprocs(vcpu);
> >  
> >  	/* Reset arch_timer context */
> > -	return kvm_timer_vcpu_reset(vcpu, cpu_vtimer_irq);
> > +	return kvm_timer_vcpu_reset(vcpu, cpu_vtimer_irq, cpu_ptimer_irq);
> >  }
> > diff --git a/arch/arm64/kvm/reset.c b/arch/arm64/kvm/reset.c
> > index e95d4f6..d9e9697 100644
> > --- a/arch/arm64/kvm/reset.c
> > +++ b/arch/arm64/kvm/reset.c
> > @@ -46,6 +46,11 @@
> >  			COMPAT_PSR_I_BIT | COMPAT_PSR_F_BIT),
> >  };
> >  
> > +static const struct kvm_irq_level default_ptimer_irq = {
> > +	.irq	= 30,
> > +	.level	= 1,
> > +};
> > +
> >  static const struct kvm_irq_level default_vtimer_irq = {
> >  	.irq	= 27,
> >  	.level	= 1,
> > @@ -104,6 +109,7 @@ int kvm_arch_dev_ioctl_check_extension(struct kvm *kvm, long ext)
> >  int kvm_reset_vcpu(struct kvm_vcpu *vcpu)
> >  {
> >  	const struct kvm_irq_level *cpu_vtimer_irq;
> > +	const struct kvm_irq_level *cpu_ptimer_irq;
> >  	const struct kvm_regs *cpu_reset;
> >  
> >  	switch (vcpu->arch.target) {
> > @@ -117,6 +123,7 @@ int kvm_reset_vcpu(struct kvm_vcpu *vcpu)
> >  		}
> >  
> >  		cpu_vtimer_irq = &default_vtimer_irq;
> > +		cpu_ptimer_irq = &default_ptimer_irq;
> >  		break;
> >  	}
> >  
> > @@ -130,5 +137,5 @@ int kvm_reset_vcpu(struct kvm_vcpu *vcpu)
> >  	kvm_pmu_vcpu_reset(vcpu);
> >  
> >  	/* Reset timer */
> > -	return kvm_timer_vcpu_reset(vcpu, cpu_vtimer_irq);
> > +	return kvm_timer_vcpu_reset(vcpu, cpu_vtimer_irq, cpu_ptimer_irq);
> >  }
> > diff --git a/include/kvm/arm_arch_timer.h b/include/kvm/arm_arch_timer.h
> > index 69f648b..a364593 100644
> > --- a/include/kvm/arm_arch_timer.h
> > +++ b/include/kvm/arm_arch_timer.h
> > @@ -59,7 +59,8 @@ struct arch_timer_cpu {
> >  int kvm_timer_enable(struct kvm_vcpu *vcpu);
> >  void kvm_timer_init(struct kvm *kvm);
> >  int kvm_timer_vcpu_reset(struct kvm_vcpu *vcpu,
> > -			 const struct kvm_irq_level *irq);
> > +			 const struct kvm_irq_level *virt_irq,
> > +			 const struct kvm_irq_level *phys_irq);
> >  void kvm_timer_vcpu_init(struct kvm_vcpu *vcpu);
> >  void kvm_timer_flush_hwstate(struct kvm_vcpu *vcpu);
> >  void kvm_timer_sync_hwstate(struct kvm_vcpu *vcpu);
> > diff --git a/virt/kvm/arm/arch_timer.c b/virt/kvm/arm/arch_timer.c
> > index f72005a..0f6e935 100644
> > --- a/virt/kvm/arm/arch_timer.c
> > +++ b/virt/kvm/arm/arch_timer.c
> > @@ -329,9 +329,11 @@ void kvm_timer_sync_hwstate(struct kvm_vcpu *vcpu)
> >  }
> >  
> >  int kvm_timer_vcpu_reset(struct kvm_vcpu *vcpu,
> > -			 const struct kvm_irq_level *irq)
> > +			 const struct kvm_irq_level *virt_irq,
> > +			 const struct kvm_irq_level *phys_irq)
> >  {
> >  	struct arch_timer_context *vtimer = vcpu_vtimer(vcpu);
> > +	struct arch_timer_context *ptimer = vcpu_ptimer(vcpu);
> >  
> >  	/*
> >  	 * The vcpu timer irq number cannot be determined in
> > @@ -339,7 +341,8 @@ int kvm_timer_vcpu_reset(struct kvm_vcpu *vcpu,
> >  	 * kvm_vcpu_set_target(). To handle this, we determine
> >  	 * vcpu timer irq number when the vcpu is reset.
> >  	 */
> > -	vtimer->irq.irq = irq->irq;
> > +	vtimer->irq.irq = virt_irq->irq;
> > +	ptimer->irq.irq = phys_irq->irq;
> >  
> >  	/*
> >  	 * The bits in CNTV_CTL are architecturally reset to UNKNOWN for ARMv8
> > @@ -348,6 +351,7 @@ int kvm_timer_vcpu_reset(struct kvm_vcpu *vcpu,
> >  	 * the ARMv7 architecture.
> >  	 */
> >  	vtimer->cnt_ctl = 0;
> > +	ptimer->cnt_ctl = 0;
> >  	kvm_timer_update_state(vcpu);
> >  
> >  	return 0;
> > @@ -369,6 +373,7 @@ void kvm_timer_vcpu_init(struct kvm_vcpu *vcpu)
> >  	struct arch_timer_cpu *timer = &vcpu->arch.timer_cpu;
> >  
> >  	update_vtimer_cntvoff(vcpu, kvm_phys_timer_read());
> > +	vcpu_ptimer(vcpu)->cntvoff = 0;
> 
> This is quite contentious, IMHO. Do we really want to expose the delta
> between the virtual and physical counters? That's a clear indication to
> the guest that it is virtualized. I"m not sure it matters, but I think
> we should at least make a conscious choice, and maybe give the
> opportunity to userspace to select the desired behaviour.
> 

So my understanding of the architecture is that you should always have
two timer/counter pairs available at EL1.  They may be synchronized, and
they may not.  If you want an accurate reading of wall clock time, look
at the physical counter, and that can generally be expected to be fast,
precise, and syncrhonized (on working hardware, of course).

Now, there can be a constant or potentially monotonically increasing
offset between the physial and virtual counters, which may mean you're
running under a hypervisor or (in the first case) that firmware
programmed or neglected to program cntvoff.  I don't think it's an
inherent problem to expose that difference to a guest, and I think it's
more important that reading the physical counter is fast and doesn't
trap.

The question is which contract we can have with a guest OS, and which
legacy we have to keep supporting (Linux, UEFI, ?).

Probably Linux should keep relying on the virtual counter/timer only,
unless something is advertised in DT/ACPI, about it being able to use
the physical timer/counter pair, even when booted at EL1.  We could
explore the opportunities to build on that to let the guest figure
out stolen time by reading the two counters and by programming the
proper timer depending on the desired semantics (i.e. virtual or
physical time).

In terms of this patch, I actually think it's fine, but we may need to
build something more on top later.  It is possible, though, that I'm
entirely missing the point about Linux timekeeping infrastructure and
that my reading of the architecture is bogus.

What do you think?

Thanks,
-Christoffer

[toc] | [prev] | [next] | [standalone]

#1569951 — Re: [RFC v2 05/10] KVM: arm/arm64: Initialize the emulated EL1 physical timer

On 30/01/17 14:58, Christoffer Dall wrote:
> On Sun, Jan 29, 2017 at 12:07:48PM +0000, Marc Zyngier wrote:
>> On Fri, Jan 27 2017 at 01:04:55 AM, Jintack Lim <jintack@cs.columbia.edu> wrote:
>>> Initialize the emulated EL1 physical timer with the default irq number.
>>>
>>> Signed-off-by: Jintack Lim <jintack@cs.columbia.edu>
>>> ---
>>>  arch/arm/kvm/reset.c         | 9 ++++++++-
>>>  arch/arm64/kvm/reset.c       | 9 ++++++++-
>>>  include/kvm/arm_arch_timer.h | 3 ++-
>>>  virt/kvm/arm/arch_timer.c    | 9 +++++++--
>>>  4 files changed, 25 insertions(+), 5 deletions(-)
>>>
>>> diff --git a/arch/arm/kvm/reset.c b/arch/arm/kvm/reset.c
>>> index 4b5e802..1da8b2d 100644
>>> --- a/arch/arm/kvm/reset.c
>>> +++ b/arch/arm/kvm/reset.c
>>> @@ -37,6 +37,11 @@
>>>  	.usr_regs.ARM_cpsr = SVC_MODE | PSR_A_BIT | PSR_I_BIT | PSR_F_BIT,
>>>  };
>>>  
>>> +static const struct kvm_irq_level cortexa_ptimer_irq = {
>>> +	{ .irq = 30 },
>>> +	.level = 1,
>>> +};
>>
>> At some point, we'll have to make that discoverable/configurable. Maybe
>> the time for a discoverable arch timer has come (see below).
>>
>>> +
>>>  static const struct kvm_irq_level cortexa_vtimer_irq = {
>>>  	{ .irq = 27 },
>>>  	.level = 1,
>>> @@ -58,6 +63,7 @@ int kvm_reset_vcpu(struct kvm_vcpu *vcpu)
>>>  {
>>>  	struct kvm_regs *reset_regs;
>>>  	const struct kvm_irq_level *cpu_vtimer_irq;
>>> +	const struct kvm_irq_level *cpu_ptimer_irq;
>>>  
>>>  	switch (vcpu->arch.target) {
>>>  	case KVM_ARM_TARGET_CORTEX_A7:
>>> @@ -65,6 +71,7 @@ int kvm_reset_vcpu(struct kvm_vcpu *vcpu)
>>>  		reset_regs = &cortexa_regs_reset;
>>>  		vcpu->arch.midr = read_cpuid_id();
>>>  		cpu_vtimer_irq = &cortexa_vtimer_irq;
>>> +		cpu_ptimer_irq = &cortexa_ptimer_irq;
>>>  		break;
>>>  	default:
>>>  		return -ENODEV;
>>> @@ -77,5 +84,5 @@ int kvm_reset_vcpu(struct kvm_vcpu *vcpu)
>>>  	kvm_reset_coprocs(vcpu);
>>>  
>>>  	/* Reset arch_timer context */
>>> -	return kvm_timer_vcpu_reset(vcpu, cpu_vtimer_irq);
>>> +	return kvm_timer_vcpu_reset(vcpu, cpu_vtimer_irq, cpu_ptimer_irq);
>>>  }
>>> diff --git a/arch/arm64/kvm/reset.c b/arch/arm64/kvm/reset.c
>>> index e95d4f6..d9e9697 100644
>>> --- a/arch/arm64/kvm/reset.c
>>> +++ b/arch/arm64/kvm/reset.c
>>> @@ -46,6 +46,11 @@
>>>  			COMPAT_PSR_I_BIT | COMPAT_PSR_F_BIT),
>>>  };
>>>  
>>> +static const struct kvm_irq_level default_ptimer_irq = {
>>> +	.irq	= 30,
>>> +	.level	= 1,
>>> +};
>>> +
>>>  static const struct kvm_irq_level default_vtimer_irq = {
>>>  	.irq	= 27,
>>>  	.level	= 1,
>>> @@ -104,6 +109,7 @@ int kvm_arch_dev_ioctl_check_extension(struct kvm *kvm, long ext)
>>>  int kvm_reset_vcpu(struct kvm_vcpu *vcpu)
>>>  {
>>>  	const struct kvm_irq_level *cpu_vtimer_irq;
>>> +	const struct kvm_irq_level *cpu_ptimer_irq;
>>>  	const struct kvm_regs *cpu_reset;
>>>  
>>>  	switch (vcpu->arch.target) {
>>> @@ -117,6 +123,7 @@ int kvm_reset_vcpu(struct kvm_vcpu *vcpu)
>>>  		}
>>>  
>>>  		cpu_vtimer_irq = &default_vtimer_irq;
>>> +		cpu_ptimer_irq = &default_ptimer_irq;
>>>  		break;
>>>  	}
>>>  
>>> @@ -130,5 +137,5 @@ int kvm_reset_vcpu(struct kvm_vcpu *vcpu)
>>>  	kvm_pmu_vcpu_reset(vcpu);
>>>  
>>>  	/* Reset timer */
>>> -	return kvm_timer_vcpu_reset(vcpu, cpu_vtimer_irq);
>>> +	return kvm_timer_vcpu_reset(vcpu, cpu_vtimer_irq, cpu_ptimer_irq);
>>>  }
>>> diff --git a/include/kvm/arm_arch_timer.h b/include/kvm/arm_arch_timer.h
>>> index 69f648b..a364593 100644
>>> --- a/include/kvm/arm_arch_timer.h
>>> +++ b/include/kvm/arm_arch_timer.h
>>> @@ -59,7 +59,8 @@ struct arch_timer_cpu {
>>>  int kvm_timer_enable(struct kvm_vcpu *vcpu);
>>>  void kvm_timer_init(struct kvm *kvm);
>>>  int kvm_timer_vcpu_reset(struct kvm_vcpu *vcpu,
>>> -			 const struct kvm_irq_level *irq);
>>> +			 const struct kvm_irq_level *virt_irq,
>>> +			 const struct kvm_irq_level *phys_irq);
>>>  void kvm_timer_vcpu_init(struct kvm_vcpu *vcpu);
>>>  void kvm_timer_flush_hwstate(struct kvm_vcpu *vcpu);
>>>  void kvm_timer_sync_hwstate(struct kvm_vcpu *vcpu);
>>> diff --git a/virt/kvm/arm/arch_timer.c b/virt/kvm/arm/arch_timer.c
>>> index f72005a..0f6e935 100644
>>> --- a/virt/kvm/arm/arch_timer.c
>>> +++ b/virt/kvm/arm/arch_timer.c
>>> @@ -329,9 +329,11 @@ void kvm_timer_sync_hwstate(struct kvm_vcpu *vcpu)
>>>  }
>>>  
>>>  int kvm_timer_vcpu_reset(struct kvm_vcpu *vcpu,
>>> -			 const struct kvm_irq_level *irq)
>>> +			 const struct kvm_irq_level *virt_irq,
>>> +			 const struct kvm_irq_level *phys_irq)
>>>  {
>>>  	struct arch_timer_context *vtimer = vcpu_vtimer(vcpu);
>>> +	struct arch_timer_context *ptimer = vcpu_ptimer(vcpu);
>>>  
>>>  	/*
>>>  	 * The vcpu timer irq number cannot be determined in
>>> @@ -339,7 +341,8 @@ int kvm_timer_vcpu_reset(struct kvm_vcpu *vcpu,
>>>  	 * kvm_vcpu_set_target(). To handle this, we determine
>>>  	 * vcpu timer irq number when the vcpu is reset.
>>>  	 */
>>> -	vtimer->irq.irq = irq->irq;
>>> +	vtimer->irq.irq = virt_irq->irq;
>>> +	ptimer->irq.irq = phys_irq->irq;
>>>  
>>>  	/*
>>>  	 * The bits in CNTV_CTL are architecturally reset to UNKNOWN for ARMv8
>>> @@ -348,6 +351,7 @@ int kvm_timer_vcpu_reset(struct kvm_vcpu *vcpu,
>>>  	 * the ARMv7 architecture.
>>>  	 */
>>>  	vtimer->cnt_ctl = 0;
>>> +	ptimer->cnt_ctl = 0;
>>>  	kvm_timer_update_state(vcpu);
>>>  
>>>  	return 0;
>>> @@ -369,6 +373,7 @@ void kvm_timer_vcpu_init(struct kvm_vcpu *vcpu)
>>>  	struct arch_timer_cpu *timer = &vcpu->arch.timer_cpu;
>>>  
>>>  	update_vtimer_cntvoff(vcpu, kvm_phys_timer_read());
>>> +	vcpu_ptimer(vcpu)->cntvoff = 0;
>>
>> This is quite contentious, IMHO. Do we really want to expose the delta
>> between the virtual and physical counters? That's a clear indication to
>> the guest that it is virtualized. I"m not sure it matters, but I think
>> we should at least make a conscious choice, and maybe give the
>> opportunity to userspace to select the desired behaviour.
>>
> 
> So my understanding of the architecture is that you should always have
> two timer/counter pairs available at EL1.  They may be synchronized, and
> they may not.  If you want an accurate reading of wall clock time, look
> at the physical counter, and that can generally be expected to be fast,
> precise, and syncrhonized (on working hardware, of course).
> 
> Now, there can be a constant or potentially monotonically increasing
> offset between the physial and virtual counters, which may mean you're
> running under a hypervisor or (in the first case) that firmware
> programmed or neglected to program cntvoff.  I don't think it's an
> inherent problem to expose that difference to a guest, and I think it's
> more important that reading the physical counter is fast and doesn't
> trap.
> 
> The question is which contract we can have with a guest OS, and which
> legacy we have to keep supporting (Linux, UEFI, ?).
> 
> Probably Linux should keep relying on the virtual counter/timer only,
> unless something is advertised in DT/ACPI, about it being able to use
> the physical timer/counter pair, even when booted at EL1.  We could
> explore the opportunities to build on that to let the guest figure
> out stolen time by reading the two counters and by programming the
> proper timer depending on the desired semantics (i.e. virtual or
> physical time).
> 
> In terms of this patch, I actually think it's fine, but we may need to
> build something more on top later.  It is possible, though, that I'm
> entirely missing the point about Linux timekeeping infrastructure and
> that my reading of the architecture is bogus.
> 
> What do you think?

I don't disagree with any of this (hopefully I was clearer in my reply
to the cover letter). Wventually, we'll have to support an offset-able
physical counter to support nested virtualization, but this can come at
a later time.

Thanks,

	M.
-- 
Jazz is not dead. It just smells funny...

[toc] | [prev] | [next] | [standalone]

#1570034 — Re: [RFC v2 05/10] KVM: arm/arm64: Initialize the emulated EL1 physical timer

On Mon, Jan 30, 2017 at 05:44:20PM +0000, Marc Zyngier wrote:
> On 30/01/17 14:58, Christoffer Dall wrote:
> > On Sun, Jan 29, 2017 at 12:07:48PM +0000, Marc Zyngier wrote:
> >> On Fri, Jan 27 2017 at 01:04:55 AM, Jintack Lim <jintack@cs.columbia.edu> wrote:
> >>> Initialize the emulated EL1 physical timer with the default irq number.
> >>>
> >>> Signed-off-by: Jintack Lim <jintack@cs.columbia.edu>
> >>> ---
> >>>  arch/arm/kvm/reset.c         | 9 ++++++++-
> >>>  arch/arm64/kvm/reset.c       | 9 ++++++++-
> >>>  include/kvm/arm_arch_timer.h | 3 ++-
> >>>  virt/kvm/arm/arch_timer.c    | 9 +++++++--
> >>>  4 files changed, 25 insertions(+), 5 deletions(-)
> >>>
> >>> diff --git a/arch/arm/kvm/reset.c b/arch/arm/kvm/reset.c
> >>> index 4b5e802..1da8b2d 100644
> >>> --- a/arch/arm/kvm/reset.c
> >>> +++ b/arch/arm/kvm/reset.c
> >>> @@ -37,6 +37,11 @@
> >>>  	.usr_regs.ARM_cpsr = SVC_MODE | PSR_A_BIT | PSR_I_BIT | PSR_F_BIT,
> >>>  };
> >>>  
> >>> +static const struct kvm_irq_level cortexa_ptimer_irq = {
> >>> +	{ .irq = 30 },
> >>> +	.level = 1,
> >>> +};
> >>
> >> At some point, we'll have to make that discoverable/configurable. Maybe
> >> the time for a discoverable arch timer has come (see below).
> >>
> >>> +
> >>>  static const struct kvm_irq_level cortexa_vtimer_irq = {
> >>>  	{ .irq = 27 },
> >>>  	.level = 1,
> >>> @@ -58,6 +63,7 @@ int kvm_reset_vcpu(struct kvm_vcpu *vcpu)
> >>>  {
> >>>  	struct kvm_regs *reset_regs;
> >>>  	const struct kvm_irq_level *cpu_vtimer_irq;
> >>> +	const struct kvm_irq_level *cpu_ptimer_irq;
> >>>  
> >>>  	switch (vcpu->arch.target) {
> >>>  	case KVM_ARM_TARGET_CORTEX_A7:
> >>> @@ -65,6 +71,7 @@ int kvm_reset_vcpu(struct kvm_vcpu *vcpu)
> >>>  		reset_regs = &cortexa_regs_reset;
> >>>  		vcpu->arch.midr = read_cpuid_id();
> >>>  		cpu_vtimer_irq = &cortexa_vtimer_irq;
> >>> +		cpu_ptimer_irq = &cortexa_ptimer_irq;
> >>>  		break;
> >>>  	default:
> >>>  		return -ENODEV;
> >>> @@ -77,5 +84,5 @@ int kvm_reset_vcpu(struct kvm_vcpu *vcpu)
> >>>  	kvm_reset_coprocs(vcpu);
> >>>  
> >>>  	/* Reset arch_timer context */
> >>> -	return kvm_timer_vcpu_reset(vcpu, cpu_vtimer_irq);
> >>> +	return kvm_timer_vcpu_reset(vcpu, cpu_vtimer_irq, cpu_ptimer_irq);
> >>>  }
> >>> diff --git a/arch/arm64/kvm/reset.c b/arch/arm64/kvm/reset.c
> >>> index e95d4f6..d9e9697 100644
> >>> --- a/arch/arm64/kvm/reset.c
> >>> +++ b/arch/arm64/kvm/reset.c
> >>> @@ -46,6 +46,11 @@
> >>>  			COMPAT_PSR_I_BIT | COMPAT_PSR_F_BIT),
> >>>  };
> >>>  
> >>> +static const struct kvm_irq_level default_ptimer_irq = {
> >>> +	.irq	= 30,
> >>> +	.level	= 1,
> >>> +};
> >>> +
> >>>  static const struct kvm_irq_level default_vtimer_irq = {
> >>>  	.irq	= 27,
> >>>  	.level	= 1,
> >>> @@ -104,6 +109,7 @@ int kvm_arch_dev_ioctl_check_extension(struct kvm *kvm, long ext)
> >>>  int kvm_reset_vcpu(struct kvm_vcpu *vcpu)
> >>>  {
> >>>  	const struct kvm_irq_level *cpu_vtimer_irq;
> >>> +	const struct kvm_irq_level *cpu_ptimer_irq;
> >>>  	const struct kvm_regs *cpu_reset;
> >>>  
> >>>  	switch (vcpu->arch.target) {
> >>> @@ -117,6 +123,7 @@ int kvm_reset_vcpu(struct kvm_vcpu *vcpu)
> >>>  		}
> >>>  
> >>>  		cpu_vtimer_irq = &default_vtimer_irq;
> >>> +		cpu_ptimer_irq = &default_ptimer_irq;
> >>>  		break;
> >>>  	}
> >>>  
> >>> @@ -130,5 +137,5 @@ int kvm_reset_vcpu(struct kvm_vcpu *vcpu)
> >>>  	kvm_pmu_vcpu_reset(vcpu);
> >>>  
> >>>  	/* Reset timer */
> >>> -	return kvm_timer_vcpu_reset(vcpu, cpu_vtimer_irq);
> >>> +	return kvm_timer_vcpu_reset(vcpu, cpu_vtimer_irq, cpu_ptimer_irq);
> >>>  }
> >>> diff --git a/include/kvm/arm_arch_timer.h b/include/kvm/arm_arch_timer.h
> >>> index 69f648b..a364593 100644
> >>> --- a/include/kvm/arm_arch_timer.h
> >>> +++ b/include/kvm/arm_arch_timer.h
> >>> @@ -59,7 +59,8 @@ struct arch_timer_cpu {
> >>>  int kvm_timer_enable(struct kvm_vcpu *vcpu);
> >>>  void kvm_timer_init(struct kvm *kvm);
> >>>  int kvm_timer_vcpu_reset(struct kvm_vcpu *vcpu,
> >>> -			 const struct kvm_irq_level *irq);
> >>> +			 const struct kvm_irq_level *virt_irq,
> >>> +			 const struct kvm_irq_level *phys_irq);
> >>>  void kvm_timer_vcpu_init(struct kvm_vcpu *vcpu);
> >>>  void kvm_timer_flush_hwstate(struct kvm_vcpu *vcpu);
> >>>  void kvm_timer_sync_hwstate(struct kvm_vcpu *vcpu);
> >>> diff --git a/virt/kvm/arm/arch_timer.c b/virt/kvm/arm/arch_timer.c
> >>> index f72005a..0f6e935 100644
> >>> --- a/virt/kvm/arm/arch_timer.c
> >>> +++ b/virt/kvm/arm/arch_timer.c
> >>> @@ -329,9 +329,11 @@ void kvm_timer_sync_hwstate(struct kvm_vcpu *vcpu)
> >>>  }
> >>>  
> >>>  int kvm_timer_vcpu_reset(struct kvm_vcpu *vcpu,
> >>> -			 const struct kvm_irq_level *irq)
> >>> +			 const struct kvm_irq_level *virt_irq,
> >>> +			 const struct kvm_irq_level *phys_irq)
> >>>  {
> >>>  	struct arch_timer_context *vtimer = vcpu_vtimer(vcpu);
> >>> +	struct arch_timer_context *ptimer = vcpu_ptimer(vcpu);
> >>>  
> >>>  	/*
> >>>  	 * The vcpu timer irq number cannot be determined in
> >>> @@ -339,7 +341,8 @@ int kvm_timer_vcpu_reset(struct kvm_vcpu *vcpu,
> >>>  	 * kvm_vcpu_set_target(). To handle this, we determine
> >>>  	 * vcpu timer irq number when the vcpu is reset.
> >>>  	 */
> >>> -	vtimer->irq.irq = irq->irq;
> >>> +	vtimer->irq.irq = virt_irq->irq;
> >>> +	ptimer->irq.irq = phys_irq->irq;
> >>>  
> >>>  	/*
> >>>  	 * The bits in CNTV_CTL are architecturally reset to UNKNOWN for ARMv8
> >>> @@ -348,6 +351,7 @@ int kvm_timer_vcpu_reset(struct kvm_vcpu *vcpu,
> >>>  	 * the ARMv7 architecture.
> >>>  	 */
> >>>  	vtimer->cnt_ctl = 0;
> >>> +	ptimer->cnt_ctl = 0;
> >>>  	kvm_timer_update_state(vcpu);
> >>>  
> >>>  	return 0;
> >>> @@ -369,6 +373,7 @@ void kvm_timer_vcpu_init(struct kvm_vcpu *vcpu)
> >>>  	struct arch_timer_cpu *timer = &vcpu->arch.timer_cpu;
> >>>  
> >>>  	update_vtimer_cntvoff(vcpu, kvm_phys_timer_read());
> >>> +	vcpu_ptimer(vcpu)->cntvoff = 0;
> >>
> >> This is quite contentious, IMHO. Do we really want to expose the delta
> >> between the virtual and physical counters? That's a clear indication to
> >> the guest that it is virtualized. I"m not sure it matters, but I think
> >> we should at least make a conscious choice, and maybe give the
> >> opportunity to userspace to select the desired behaviour.
> >>
> > 
> > So my understanding of the architecture is that you should always have
> > two timer/counter pairs available at EL1.  They may be synchronized, and
> > they may not.  If you want an accurate reading of wall clock time, look
> > at the physical counter, and that can generally be expected to be fast,
> > precise, and syncrhonized (on working hardware, of course).
> > 
> > Now, there can be a constant or potentially monotonically increasing
> > offset between the physial and virtual counters, which may mean you're
> > running under a hypervisor or (in the first case) that firmware
> > programmed or neglected to program cntvoff.  I don't think it's an
> > inherent problem to expose that difference to a guest, and I think it's
> > more important that reading the physical counter is fast and doesn't
> > trap.
> > 
> > The question is which contract we can have with a guest OS, and which
> > legacy we have to keep supporting (Linux, UEFI, ?).
> > 
> > Probably Linux should keep relying on the virtual counter/timer only,
> > unless something is advertised in DT/ACPI, about it being able to use
> > the physical timer/counter pair, even when booted at EL1.  We could
> > explore the opportunities to build on that to let the guest figure
> > out stolen time by reading the two counters and by programming the
> > proper timer depending on the desired semantics (i.e. virtual or
> > physical time).
> > 
> > In terms of this patch, I actually think it's fine, but we may need to
> > build something more on top later.  It is possible, though, that I'm
> > entirely missing the point about Linux timekeeping infrastructure and
> > that my reading of the architecture is bogus.
> > 
> > What do you think?
> 
> I don't disagree with any of this (hopefully I was clearer in my reply
> to the cover letter).

Yeah, my long-winded reply was sort of to convince myself about my own
understanding :)


> Wventually, we'll have to support an offset-able
> physical counter to support nested virtualization, but this can come at
> a later time.
> 
Why do we need the offset-able physical counter for nested
virtualization?  I would think for nested virt we just need to support
respecting how the guest hypervisor programs CNTVOFF?

Thanks,
-Christoffer

[toc] | [prev] | [next] | [standalone]

#1571433 — Re: [RFC v2 05/10] KVM: arm/arm64: Initialize the emulated EL1 physical timer

On 30/01/17 19:04, Christoffer Dall wrote:
> On Mon, Jan 30, 2017 at 05:44:20PM +0000, Marc Zyngier wrote:

>> Wventually, we'll have to support an offset-able
>> physical counter to support nested virtualization, but this can come at
>> a later time.
>>
> Why do we need the offset-able physical counter for nested
> virtualization?  I would think for nested virt we just need to support
> respecting how the guest hypervisor programs CNTVOFF?

Ah, I see what you mean. Yes, once the guest hypervisor is in control of
its own CNTVOFF, we get everything we need. So let's just ignore this
for the time being, and we should be pretty good for this series.

Thanks,

	M.
-- 
Jazz is not dead. It just smells funny...

[toc] | [prev] | [next] | [standalone]

#1567858 — [RFC v2 01/10] KVM: arm/arm64: Abstract virtual timer context into separate structure

Abstract virtual timer context into a separate structure and change all
callers referring to timer registers, irq state and so on. No change in
functionality.

This is about to become very handy when adding the EL1 physical timer.

Signed-off-by: Jintack Lim <jintack@cs.columbia.edu>
Acked-by: Christoffer Dall <christoffer.dall@linaro.org>
---
 include/kvm/arm_arch_timer.h | 27 ++++++++---------
 virt/kvm/arm/arch_timer.c    | 69 +++++++++++++++++++++++---------------------
 virt/kvm/arm/hyp/timer-sr.c  | 10 ++++---
 3 files changed, 56 insertions(+), 50 deletions(-)

diff --git a/include/kvm/arm_arch_timer.h b/include/kvm/arm_arch_timer.h
index 5c970ce..daad3c1 100644
--- a/include/kvm/arm_arch_timer.h
+++ b/include/kvm/arm_arch_timer.h
@@ -28,15 +28,20 @@ struct arch_timer_kvm {
 	u64			cntvoff;
 };
 
-struct arch_timer_cpu {
+struct arch_timer_context {
 	/* Registers: control register, timer value */
-	u32				cntv_ctl;	/* Saved/restored */
-	u64				cntv_cval;	/* Saved/restored */
+	u32				cnt_ctl;
+	u64				cnt_cval;
+
+	/* Timer IRQ */
+	struct kvm_irq_level		irq;
+
+	/* Active IRQ state caching */
+	bool				active_cleared_last;
+};
 
-	/*
-	 * Anything that is not used directly from assembly code goes
-	 * here.
-	 */
+struct arch_timer_cpu {
+	struct arch_timer_context	vtimer;
 
 	/* Background timer used when the guest is not running */
 	struct hrtimer			timer;
@@ -47,12 +52,6 @@ struct arch_timer_cpu {
 	/* Background timer active */
 	bool				armed;
 
-	/* Timer IRQ */
-	struct kvm_irq_level		irq;
-
-	/* Active IRQ state caching */
-	bool				active_cleared_last;
-
 	/* Is the timer enabled */
 	bool			enabled;
 };
@@ -77,4 +76,6 @@ int kvm_timer_vcpu_reset(struct kvm_vcpu *vcpu,
 void kvm_timer_vcpu_put(struct kvm_vcpu *vcpu);
 
 void kvm_timer_init_vhe(void);
+
+#define vcpu_vtimer(v)	(&(v)->arch.timer_cpu.vtimer)
 #endif
diff --git a/virt/kvm/arm/arch_timer.c b/virt/kvm/arm/arch_timer.c
index 6a084cd..6740efa 100644
--- a/virt/kvm/arm/arch_timer.c
+++ b/virt/kvm/arm/arch_timer.c
@@ -37,7 +37,7 @@
 
 void kvm_timer_vcpu_put(struct kvm_vcpu *vcpu)
 {
-	vcpu->arch.timer_cpu.active_cleared_last = false;
+	vcpu_vtimer(vcpu)->active_cleared_last = false;
 }
 
 static u64 kvm_phys_timer_read(void)
@@ -102,7 +102,7 @@ static u64 kvm_timer_compute_delta(struct kvm_vcpu *vcpu)
 {
 	u64 cval, now;
 
-	cval = vcpu->arch.timer_cpu.cntv_cval;
+	cval = vcpu_vtimer(vcpu)->cnt_cval;
 	now = kvm_phys_timer_read() - vcpu->kvm->arch.timer.cntvoff;
 
 	if (now < cval) {
@@ -144,21 +144,21 @@ static enum hrtimer_restart kvm_timer_expire(struct hrtimer *hrt)
 
 static bool kvm_timer_irq_can_fire(struct kvm_vcpu *vcpu)
 {
-	struct arch_timer_cpu *timer = &vcpu->arch.timer_cpu;
+	struct arch_timer_context *vtimer = vcpu_vtimer(vcpu);
 
-	return !(timer->cntv_ctl & ARCH_TIMER_CTRL_IT_MASK) &&
-		(timer->cntv_ctl & ARCH_TIMER_CTRL_ENABLE);
+	return !(vtimer->cnt_ctl & ARCH_TIMER_CTRL_IT_MASK) &&
+		(vtimer->cnt_ctl & ARCH_TIMER_CTRL_ENABLE);
 }
 
 bool kvm_timer_should_fire(struct kvm_vcpu *vcpu)
 {
-	struct arch_timer_cpu *timer = &vcpu->arch.timer_cpu;
+	struct arch_timer_context *vtimer = vcpu_vtimer(vcpu);
 	u64 cval, now;
 
 	if (!kvm_timer_irq_can_fire(vcpu))
 		return false;
 
-	cval = timer->cntv_cval;
+	cval = vtimer->cnt_cval;
 	now = kvm_phys_timer_read() - vcpu->kvm->arch.timer.cntvoff;
 
 	return cval <= now;
@@ -167,17 +167,17 @@ bool kvm_timer_should_fire(struct kvm_vcpu *vcpu)
 static void kvm_timer_update_irq(struct kvm_vcpu *vcpu, bool new_level)
 {
 	int ret;
-	struct arch_timer_cpu *timer = &vcpu->arch.timer_cpu;
+	struct arch_timer_context *vtimer = vcpu_vtimer(vcpu);
 
 	BUG_ON(!vgic_initialized(vcpu->kvm));
 
-	timer->active_cleared_last = false;
-	timer->irq.level = new_level;
-	trace_kvm_timer_update_irq(vcpu->vcpu_id, timer->irq.irq,
-				   timer->irq.level);
+	vtimer->active_cleared_last = false;
+	vtimer->irq.level = new_level;
+	trace_kvm_timer_update_irq(vcpu->vcpu_id, vtimer->irq.irq,
+				   vtimer->irq.level);
 	ret = kvm_vgic_inject_mapped_irq(vcpu->kvm, vcpu->vcpu_id,
-					 timer->irq.irq,
-					 timer->irq.level);
+					 vtimer->irq.irq,
+					 vtimer->irq.level);
 	WARN_ON(ret);
 }
 
@@ -188,18 +188,19 @@ static void kvm_timer_update_irq(struct kvm_vcpu *vcpu, bool new_level)
 static int kvm_timer_update_state(struct kvm_vcpu *vcpu)
 {
 	struct arch_timer_cpu *timer = &vcpu->arch.timer_cpu;
+	struct arch_timer_context *vtimer = vcpu_vtimer(vcpu);
 
 	/*
 	 * If userspace modified the timer registers via SET_ONE_REG before
-	 * the vgic was initialized, we mustn't set the timer->irq.level value
+	 * the vgic was initialized, we mustn't set the vtimer->irq.level value
 	 * because the guest would never see the interrupt.  Instead wait
 	 * until we call this function from kvm_timer_flush_hwstate.
 	 */
 	if (!vgic_initialized(vcpu->kvm) || !timer->enabled)
 		return -ENODEV;
 
-	if (kvm_timer_should_fire(vcpu) != timer->irq.level)
-		kvm_timer_update_irq(vcpu, !timer->irq.level);
+	if (kvm_timer_should_fire(vcpu) != vtimer->irq.level)
+		kvm_timer_update_irq(vcpu, !vtimer->irq.level);
 
 	return 0;
 }
@@ -249,7 +250,7 @@ void kvm_timer_unschedule(struct kvm_vcpu *vcpu)
  */
 void kvm_timer_flush_hwstate(struct kvm_vcpu *vcpu)
 {
-	struct arch_timer_cpu *timer = &vcpu->arch.timer_cpu;
+	struct arch_timer_context *vtimer = vcpu_vtimer(vcpu);
 	bool phys_active;
 	int ret;
 
@@ -273,8 +274,8 @@ void kvm_timer_flush_hwstate(struct kvm_vcpu *vcpu)
 	* to ensure that hardware interrupts from the timer triggers a guest
 	* exit.
 	*/
-	phys_active = timer->irq.level ||
-			kvm_vgic_map_is_active(vcpu, timer->irq.irq);
+	phys_active = vtimer->irq.level ||
+			kvm_vgic_map_is_active(vcpu, vtimer->irq.irq);
 
 	/*
 	 * We want to avoid hitting the (re)distributor as much as
@@ -296,7 +297,7 @@ void kvm_timer_flush_hwstate(struct kvm_vcpu *vcpu)
 	 * - cached value is "active clear"
 	 * - value to be programmed is "active clear"
 	 */
-	if (timer->active_cleared_last && !phys_active)
+	if (vtimer->active_cleared_last && !phys_active)
 		return;
 
 	ret = irq_set_irqchip_state(host_vtimer_irq,
@@ -304,7 +305,7 @@ void kvm_timer_flush_hwstate(struct kvm_vcpu *vcpu)
 				    phys_active);
 	WARN_ON(ret);
 
-	timer->active_cleared_last = !phys_active;
+	vtimer->active_cleared_last = !phys_active;
 }
 
 /**
@@ -330,7 +331,7 @@ void kvm_timer_sync_hwstate(struct kvm_vcpu *vcpu)
 int kvm_timer_vcpu_reset(struct kvm_vcpu *vcpu,
 			 const struct kvm_irq_level *irq)
 {
-	struct arch_timer_cpu *timer = &vcpu->arch.timer_cpu;
+	struct arch_timer_context *vtimer = vcpu_vtimer(vcpu);
 
 	/*
 	 * The vcpu timer irq number cannot be determined in
@@ -338,7 +339,7 @@ int kvm_timer_vcpu_reset(struct kvm_vcpu *vcpu,
 	 * kvm_vcpu_set_target(). To handle this, we determine
 	 * vcpu timer irq number when the vcpu is reset.
 	 */
-	timer->irq.irq = irq->irq;
+	vtimer->irq.irq = irq->irq;
 
 	/*
 	 * The bits in CNTV_CTL are architecturally reset to UNKNOWN for ARMv8
@@ -346,7 +347,7 @@ int kvm_timer_vcpu_reset(struct kvm_vcpu *vcpu,
 	 * resets the timer to be disabled and unmasked and is compliant with
 	 * the ARMv7 architecture.
 	 */
-	timer->cntv_ctl = 0;
+	vtimer->cnt_ctl = 0;
 	kvm_timer_update_state(vcpu);
 
 	return 0;
@@ -368,17 +369,17 @@ static void kvm_timer_init_interrupt(void *info)
 
 int kvm_arm_timer_set_reg(struct kvm_vcpu *vcpu, u64 regid, u64 value)
 {
-	struct arch_timer_cpu *timer = &vcpu->arch.timer_cpu;
+	struct arch_timer_context *vtimer = vcpu_vtimer(vcpu);
 
 	switch (regid) {
 	case KVM_REG_ARM_TIMER_CTL:
-		timer->cntv_ctl = value;
+		vtimer->cnt_ctl = value;
 		break;
 	case KVM_REG_ARM_TIMER_CNT:
 		vcpu->kvm->arch.timer.cntvoff = kvm_phys_timer_read() - value;
 		break;
 	case KVM_REG_ARM_TIMER_CVAL:
-		timer->cntv_cval = value;
+		vtimer->cnt_cval = value;
 		break;
 	default:
 		return -1;
@@ -390,15 +391,15 @@ int kvm_arm_timer_set_reg(struct kvm_vcpu *vcpu, u64 regid, u64 value)
 
 u64 kvm_arm_timer_get_reg(struct kvm_vcpu *vcpu, u64 regid)
 {
-	struct arch_timer_cpu *timer = &vcpu->arch.timer_cpu;
+	struct arch_timer_context *vtimer = vcpu_vtimer(vcpu);
 
 	switch (regid) {
 	case KVM_REG_ARM_TIMER_CTL:
-		return timer->cntv_ctl;
+		return vtimer->cnt_ctl;
 	case KVM_REG_ARM_TIMER_CNT:
 		return kvm_phys_timer_read() - vcpu->kvm->arch.timer.cntvoff;
 	case KVM_REG_ARM_TIMER_CVAL:
-		return timer->cntv_cval;
+		return vtimer->cnt_cval;
 	}
 	return (u64)-1;
 }
@@ -462,14 +463,16 @@ int kvm_timer_hyp_init(void)
 void kvm_timer_vcpu_terminate(struct kvm_vcpu *vcpu)
 {
 	struct arch_timer_cpu *timer = &vcpu->arch.timer_cpu;
+	struct arch_timer_context *vtimer = vcpu_vtimer(vcpu);
 
 	timer_disarm(timer);
-	kvm_vgic_unmap_phys_irq(vcpu, timer->irq.irq);
+	kvm_vgic_unmap_phys_irq(vcpu, vtimer->irq.irq);
 }
 
 int kvm_timer_enable(struct kvm_vcpu *vcpu)
 {
 	struct arch_timer_cpu *timer = &vcpu->arch.timer_cpu;
+	struct arch_timer_context *vtimer = vcpu_vtimer(vcpu);
 	struct irq_desc *desc;
 	struct irq_data *data;
 	int phys_irq;
@@ -497,7 +500,7 @@ int kvm_timer_enable(struct kvm_vcpu *vcpu)
 	 * Tell the VGIC that the virtual interrupt is tied to a
 	 * physical interrupt. We do that once per VCPU.
 	 */
-	ret = kvm_vgic_map_phys_irq(vcpu, timer->irq.irq, phys_irq);
+	ret = kvm_vgic_map_phys_irq(vcpu, vtimer->irq.irq, phys_irq);
 	if (ret)
 		return ret;
 
diff --git a/virt/kvm/arm/hyp/timer-sr.c b/virt/kvm/arm/hyp/timer-sr.c
index 63e28dd..0cf0895 100644
--- a/virt/kvm/arm/hyp/timer-sr.c
+++ b/virt/kvm/arm/hyp/timer-sr.c
@@ -25,11 +25,12 @@
 void __hyp_text __timer_save_state(struct kvm_vcpu *vcpu)
 {
 	struct arch_timer_cpu *timer = &vcpu->arch.timer_cpu;
+	struct arch_timer_context *vtimer = vcpu_vtimer(vcpu);
 	u64 val;
 
 	if (timer->enabled) {
-		timer->cntv_ctl = read_sysreg_el0(cntv_ctl);
-		timer->cntv_cval = read_sysreg_el0(cntv_cval);
+		vtimer->cnt_ctl = read_sysreg_el0(cntv_ctl);
+		vtimer->cnt_cval = read_sysreg_el0(cntv_cval);
 	}
 
 	/* Disable the virtual timer */
@@ -54,6 +55,7 @@ void __hyp_text __timer_restore_state(struct kvm_vcpu *vcpu)
 {
 	struct kvm *kvm = kern_hyp_va(vcpu->kvm);
 	struct arch_timer_cpu *timer = &vcpu->arch.timer_cpu;
+	struct arch_timer_context *vtimer = vcpu_vtimer(vcpu);
 	u64 val;
 
 	/* Those bits are already configured at boot on VHE-system */
@@ -70,8 +72,8 @@ void __hyp_text __timer_restore_state(struct kvm_vcpu *vcpu)
 
 	if (timer->enabled) {
 		write_sysreg(kvm->arch.timer.cntvoff, cntvoff_el2);
-		write_sysreg_el0(timer->cntv_cval, cntv_cval);
+		write_sysreg_el0(vtimer->cnt_cval, cntv_cval);
 		isb();
-		write_sysreg_el0(timer->cntv_ctl, cntv_ctl);
+		write_sysreg_el0(vtimer->cnt_ctl, cntv_ctl);
 	}
 }
-- 
1.9.1

[toc] | [prev] | [next] | [standalone]

#1569175 — Re: [RFC v2 01/10] KVM: arm/arm64: Abstract virtual timer context into separate structure

On Fri, Jan 27 2017 at 01:04:51 AM, Jintack Lim <jintack@cs.columbia.edu> wrote:
> Abstract virtual timer context into a separate structure and change all
> callers referring to timer registers, irq state and so on. No change in
> functionality.
>
> This is about to become very handy when adding the EL1 physical timer.
>
> Signed-off-by: Jintack Lim <jintack@cs.columbia.edu>
> Acked-by: Christoffer Dall <christoffer.dall@linaro.org>

Acked-by: Marc Zyngier <marc.zyngier@arm.com>

          M.
-- 
Jazz is not dead. It just smells funny.

[toc] | [prev] | [next] | [standalone]

#1569222

Hi Jintack,

On Fri, Jan 27 2017 at 01:04:50 AM, Jintack Lim <jintack@cs.columbia.edu> wrote:
> The ARM architecture defines the EL1 physical timer and the virtual timer,
> and it is reasonable for an OS to expect to be able to access both.
> However, the current KVM implementation does not provide the EL1 physical
> timer to VMs but terminates VMs on access to the timer.
>
> This patch series enables VMs to use the EL1 physical timer through
> trap-and-emulate.  The KVM host emulates each EL1 physical timer register
> access and sets up the background timer accordingly.  When the background
> timer expires, the KVM host injects EL1 physical timer interrupts to the
> VM.  Alternatively, it's also possible to allow VMs to access the EL1
> physical timer without trapping.  However, this requires somehow using the
> EL2 physical timer for the Linux host while running the VM instead of the
> EL1 physical timer.  Right now I just implemented trap-and-emulate because
> this was straightforward to do, and I leave it to future work to determine
> if transferring the EL1 physical timer state to the EL2 timer provides any
> performance benefit.
>
> This feature will be useful for any OS that wishes to access the EL1
> physical timer. Nested virtualization is one of those use cases. A nested
> hypervisor running inside a VM would think it has full access to the
> hardware and naturally tries to use the EL1 physical timer as Linux would
> do. Other nested hypervisors may try to use the EL2 physical timer as Xen
> would do, but supporting the EL2 physical timer to the VM is out of scope
> of this patch series. This patch series will make it easy to add the EL2
> timer support in the future, though.
>
> Note that Linux VMs booting in EL1 will be unaffected by this patch series
> and will continue to use only the virtual timer and this patch series will
> therefore not introduce any performance degredation as a result of
> trap-and-emulate.

Thanks for respining this series. Overall, this looks quite good, and
the couple of comments I have should be easy to address.

My main concern is that we do expose a timer that doesn't hide
CNTVOFF. I appreciate that that was already the case, since CNTPCT was
always available (and this avoided trapping the counter), but maybe we
should have a way for userspace to ask for a mode where CNTPCT=CNTVCT,
byt trapping the physical counter and taking CNTVOFF in all physical
timer calculations.

I'm pretty sure you've addressed this one way or another in your nested
virt series, so maybe extracting the relevant patches and adding them on
top of this series could be an option?

Thanks,

        M.
-- 
Jazz is not dead. It just smells funny.

[toc] | [prev] | [next] | [standalone]

#1570046

Hi Marc,

On Sun, Jan 29, 2017 at 10:55 AM, Marc Zyngier <marc.zyngier@arm.com> wrote:
> Hi Jintack,
>
> On Fri, Jan 27 2017 at 01:04:50 AM, Jintack Lim <jintack@cs.columbia.edu> wrote:
>> The ARM architecture defines the EL1 physical timer and the virtual timer,
>> and it is reasonable for an OS to expect to be able to access both.
>> However, the current KVM implementation does not provide the EL1 physical
>> timer to VMs but terminates VMs on access to the timer.
>>
>> This patch series enables VMs to use the EL1 physical timer through
>> trap-and-emulate.  The KVM host emulates each EL1 physical timer register
>> access and sets up the background timer accordingly.  When the background
>> timer expires, the KVM host injects EL1 physical timer interrupts to the
>> VM.  Alternatively, it's also possible to allow VMs to access the EL1
>> physical timer without trapping.  However, this requires somehow using the
>> EL2 physical timer for the Linux host while running the VM instead of the
>> EL1 physical timer.  Right now I just implemented trap-and-emulate because
>> this was straightforward to do, and I leave it to future work to determine
>> if transferring the EL1 physical timer state to the EL2 timer provides any
>> performance benefit.
>>
>> This feature will be useful for any OS that wishes to access the EL1
>> physical timer. Nested virtualization is one of those use cases. A nested
>> hypervisor running inside a VM would think it has full access to the
>> hardware and naturally tries to use the EL1 physical timer as Linux would
>> do. Other nested hypervisors may try to use the EL2 physical timer as Xen
>> would do, but supporting the EL2 physical timer to the VM is out of scope
>> of this patch series. This patch series will make it easy to add the EL2
>> timer support in the future, though.
>>
>> Note that Linux VMs booting in EL1 will be unaffected by this patch series
>> and will continue to use only the virtual timer and this patch series will
>> therefore not introduce any performance degredation as a result of
>> trap-and-emulate.
>
> Thanks for respining this series. Overall, this looks quite good, and
> the couple of comments I have should be easy to address.

Thanks for the review!

>
> My main concern is that we do expose a timer that doesn't hide
> CNTVOFF. I appreciate that that was already the case, since CNTPCT was
> always available (and this avoided trapping the counter), but maybe we
> should have a way for userspace to ask for a mode where CNTPCT=CNTVCT,
> byt trapping the physical counter and taking CNTVOFF in all physical
> timer calculations.

As discussed in the other thread, I think we can expose CNTVOFF to the
guest OS. I have a patch that lets the guest hypervisor observe CNTVCT
= CNTPCT - offset (virtual CNTVOFF_EL2) and I will include it in the
next nesting patch series.

Thanks,
Jintack

>
> I'm pretty sure you've addressed this one way or another in your nested
> virt series, so maybe extracting the relevant patches and adding them on
> top of this series could be an option?
>
> Thanks,
>
>         M.
> --
> Jazz is not dead. It just smells funny.
>

[toc] | [prev] | [standalone]

Back to top | Article view | linux.kernel

csiph-web