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

Groups > linux.kernel > #1567849 > unrolled thread

[RFC v2 10/10] KVM: arm/arm64: Emulate the EL1 phys timer register access

Back to article view | Back to linux.kernel

This discussion starts older than the indexed window; earlier articles aren't shown. The article labeled Started by below is the oldest one visible, not the original post.

Contents

  [RFC v2 10/10] KVM: arm/arm64: Emulate the EL1 phys timer register access Jintack Lim <jintack@cs.columbia.edu> - 2017-01-27 02:10 +0100
    Re: [RFC v2 10/10] KVM: arm/arm64: Emulate the EL1 phys timer register access Marc Zyngier <marc.zyngier@arm.com> - 2017-01-29 16:50 +0100
      Re: [RFC v2 10/10] KVM: arm/arm64: Emulate the EL1 phys timer  register access Jintack Lim <jintack@cs.columbia.edu> - 2017-01-30 18:10 +0100
        Re: [RFC v2 10/10] KVM: arm/arm64: Emulate the EL1 phys timer  register access Peter Maydell <peter.maydell@linaro.org> - 2017-01-30 18:30 +0100
          Re: [RFC v2 10/10] KVM: arm/arm64: Emulate the EL1 phys timer  register access Jintack Lim <jintack@cs.columbia.edu> - 2017-01-30 18:40 +0100
          Re: [RFC v2 10/10] KVM: arm/arm64: Emulate the EL1 phys timer  register access Marc Zyngier <marc.zyngier@arm.com> - 2017-01-30 18:50 +0100

#1567849 — [RFC v2 10/10] KVM: arm/arm64: Emulate the EL1 phys timer register access

Emulate read and write operations to CNTP_TVAL, CNTP_CVAL and CNTP_CTL.
Now VMs are able to use the EL1 physical timer.

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

diff --git a/arch/arm64/kvm/sys_regs.c b/arch/arm64/kvm/sys_regs.c
index fd9e747..adf009f 100644
--- a/arch/arm64/kvm/sys_regs.c
+++ b/arch/arm64/kvm/sys_regs.c
@@ -824,7 +824,14 @@ static bool access_cntp_tval(struct kvm_vcpu *vcpu,
 		struct sys_reg_params *p,
 		const struct sys_reg_desc *r)
 {
-	kvm_inject_undefined(vcpu);
+	struct arch_timer_context *ptimer = vcpu_ptimer(vcpu);
+	u64 now = kvm_phys_timer_read();
+
+	if (p->is_write)
+		ptimer->cnt_cval = p->regval + now;
+	else
+		p->regval = ptimer->cnt_cval - now;
+
 	return true;
 }
 
@@ -832,7 +839,20 @@ static bool access_cntp_ctl(struct kvm_vcpu *vcpu,
 		struct sys_reg_params *p,
 		const struct sys_reg_desc *r)
 {
-	kvm_inject_undefined(vcpu);
+	struct arch_timer_context *ptimer = vcpu_ptimer(vcpu);
+
+	if (p->is_write) {
+		/* ISTATUS bit is read-only */
+		ptimer->cnt_ctl = p->regval & ~ARCH_TIMER_CTRL_IT_STAT;
+	} else {
+		u64 now = kvm_phys_timer_read();
+
+		p->regval = ptimer->cnt_ctl;
+		/* Set ISTATUS bit if it's expired */
+		if (ptimer->cnt_cval <= now)
+			p->regval |= ARCH_TIMER_CTRL_IT_STAT;
+	}
+
 	return true;
 }
 
@@ -840,7 +860,13 @@ static bool access_cntp_cval(struct kvm_vcpu *vcpu,
 		struct sys_reg_params *p,
 		const struct sys_reg_desc *r)
 {
-	kvm_inject_undefined(vcpu);
+	struct arch_timer_context *ptimer = vcpu_ptimer(vcpu);
+
+	if (p->is_write)
+		ptimer->cnt_cval = p->regval;
+	else
+		p->regval = ptimer->cnt_cval;
+
 	return true;
 }
 
diff --git a/include/kvm/arm_arch_timer.h b/include/kvm/arm_arch_timer.h
index a364593..fec99f2 100644
--- a/include/kvm/arm_arch_timer.h
+++ b/include/kvm/arm_arch_timer.h
@@ -74,6 +74,8 @@ bool kvm_timer_should_fire(struct kvm_vcpu *vcpu,
 void kvm_timer_schedule(struct kvm_vcpu *vcpu);
 void kvm_timer_unschedule(struct kvm_vcpu *vcpu);
 
+u64 kvm_phys_timer_read(void);
+
 void kvm_timer_vcpu_put(struct kvm_vcpu *vcpu);
 
 void kvm_timer_init_vhe(void);
diff --git a/virt/kvm/arm/arch_timer.c b/virt/kvm/arm/arch_timer.c
index b366bb2..9eec063 100644
--- a/virt/kvm/arm/arch_timer.c
+++ b/virt/kvm/arm/arch_timer.c
@@ -40,7 +40,7 @@ void kvm_timer_vcpu_put(struct kvm_vcpu *vcpu)
 	vcpu_vtimer(vcpu)->active_cleared_last = false;
 }
 
-static u64 kvm_phys_timer_read(void)
+u64 kvm_phys_timer_read(void)
 {
 	return timecounter->cc->read(timecounter->cc);
 }
-- 
1.9.1

[toc] | [next] | [standalone]

#1569221

On Fri, Jan 27 2017 at 01:05:00 AM, Jintack Lim <jintack@cs.columbia.edu> wrote:
> Emulate read and write operations to CNTP_TVAL, CNTP_CVAL and CNTP_CTL.
> Now VMs are able to use the EL1 physical timer.
>
> Signed-off-by: Jintack Lim <jintack@cs.columbia.edu>
> ---
>  arch/arm64/kvm/sys_regs.c    | 32 +++++++++++++++++++++++++++++---
>  include/kvm/arm_arch_timer.h |  2 ++
>  virt/kvm/arm/arch_timer.c    |  2 +-
>  3 files changed, 32 insertions(+), 4 deletions(-)
>
> diff --git a/arch/arm64/kvm/sys_regs.c b/arch/arm64/kvm/sys_regs.c
> index fd9e747..adf009f 100644
> --- a/arch/arm64/kvm/sys_regs.c
> +++ b/arch/arm64/kvm/sys_regs.c
> @@ -824,7 +824,14 @@ static bool access_cntp_tval(struct kvm_vcpu *vcpu,
>  		struct sys_reg_params *p,
>  		const struct sys_reg_desc *r)
>  {
> -	kvm_inject_undefined(vcpu);
> +	struct arch_timer_context *ptimer = vcpu_ptimer(vcpu);
> +	u64 now = kvm_phys_timer_read();
> +
> +	if (p->is_write)
> +		ptimer->cnt_cval = p->regval + now;
> +	else
> +		p->regval = ptimer->cnt_cval - now;
> +
>  	return true;
>  }
>  
> @@ -832,7 +839,20 @@ static bool access_cntp_ctl(struct kvm_vcpu *vcpu,
>  		struct sys_reg_params *p,
>  		const struct sys_reg_desc *r)
>  {
> -	kvm_inject_undefined(vcpu);
> +	struct arch_timer_context *ptimer = vcpu_ptimer(vcpu);
> +
> +	if (p->is_write) {
> +		/* ISTATUS bit is read-only */
> +		ptimer->cnt_ctl = p->regval & ~ARCH_TIMER_CTRL_IT_STAT;
> +	} else {
> +		u64 now = kvm_phys_timer_read();
> +
> +		p->regval = ptimer->cnt_ctl;
> +		/* Set ISTATUS bit if it's expired */
> +		if (ptimer->cnt_cval <= now)
> +			p->regval |= ARCH_TIMER_CTRL_IT_STAT;
> +	}

Shouldn't we take the ENABLE bit into account? The ARMv8 ARM version I
have at hand (version h) seems to indicate that we should, but we should
check with the latest and greatest...

> +
>  	return true;
>  }
>  
> @@ -840,7 +860,13 @@ static bool access_cntp_cval(struct kvm_vcpu *vcpu,
>  		struct sys_reg_params *p,
>  		const struct sys_reg_desc *r)
>  {
> -	kvm_inject_undefined(vcpu);
> +	struct arch_timer_context *ptimer = vcpu_ptimer(vcpu);
> +
> +	if (p->is_write)
> +		ptimer->cnt_cval = p->regval;
> +	else
> +		p->regval = ptimer->cnt_cval;
> +
>  	return true;
>  }
>  
> diff --git a/include/kvm/arm_arch_timer.h b/include/kvm/arm_arch_timer.h
> index a364593..fec99f2 100644
> --- a/include/kvm/arm_arch_timer.h
> +++ b/include/kvm/arm_arch_timer.h
> @@ -74,6 +74,8 @@ bool kvm_timer_should_fire(struct kvm_vcpu *vcpu,
>  void kvm_timer_schedule(struct kvm_vcpu *vcpu);
>  void kvm_timer_unschedule(struct kvm_vcpu *vcpu);
>  
> +u64 kvm_phys_timer_read(void);
> +
>  void kvm_timer_vcpu_put(struct kvm_vcpu *vcpu);
>  
>  void kvm_timer_init_vhe(void);
> diff --git a/virt/kvm/arm/arch_timer.c b/virt/kvm/arm/arch_timer.c
> index b366bb2..9eec063 100644
> --- a/virt/kvm/arm/arch_timer.c
> +++ b/virt/kvm/arm/arch_timer.c
> @@ -40,7 +40,7 @@ void kvm_timer_vcpu_put(struct kvm_vcpu *vcpu)
>  	vcpu_vtimer(vcpu)->active_cleared_last = false;
>  }
>  
> -static u64 kvm_phys_timer_read(void)
> +u64 kvm_phys_timer_read(void)
>  {
>  	return timecounter->cc->read(timecounter->cc);
>  }

Thanks,

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

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

#1569906 — Re: [RFC v2 10/10] KVM: arm/arm64: Emulate the EL1 phys timer register access

Hi Marc,

On Sun, Jan 29, 2017 at 10:44 AM, Marc Zyngier <marc.zyngier@arm.com> wrote:
> On Fri, Jan 27 2017 at 01:05:00 AM, Jintack Lim <jintack@cs.columbia.edu> wrote:
>> Emulate read and write operations to CNTP_TVAL, CNTP_CVAL and CNTP_CTL.
>> Now VMs are able to use the EL1 physical timer.
>>
>> Signed-off-by: Jintack Lim <jintack@cs.columbia.edu>
>> ---
>>  arch/arm64/kvm/sys_regs.c    | 32 +++++++++++++++++++++++++++++---
>>  include/kvm/arm_arch_timer.h |  2 ++
>>  virt/kvm/arm/arch_timer.c    |  2 +-
>>  3 files changed, 32 insertions(+), 4 deletions(-)
>>
>> diff --git a/arch/arm64/kvm/sys_regs.c b/arch/arm64/kvm/sys_regs.c
>> index fd9e747..adf009f 100644
>> --- a/arch/arm64/kvm/sys_regs.c
>> +++ b/arch/arm64/kvm/sys_regs.c
>> @@ -824,7 +824,14 @@ static bool access_cntp_tval(struct kvm_vcpu *vcpu,
>>               struct sys_reg_params *p,
>>               const struct sys_reg_desc *r)
>>  {
>> -     kvm_inject_undefined(vcpu);
>> +     struct arch_timer_context *ptimer = vcpu_ptimer(vcpu);
>> +     u64 now = kvm_phys_timer_read();
>> +
>> +     if (p->is_write)
>> +             ptimer->cnt_cval = p->regval + now;
>> +     else
>> +             p->regval = ptimer->cnt_cval - now;
>> +
>>       return true;
>>  }
>>
>> @@ -832,7 +839,20 @@ static bool access_cntp_ctl(struct kvm_vcpu *vcpu,
>>               struct sys_reg_params *p,
>>               const struct sys_reg_desc *r)
>>  {
>> -     kvm_inject_undefined(vcpu);
>> +     struct arch_timer_context *ptimer = vcpu_ptimer(vcpu);
>> +
>> +     if (p->is_write) {
>> +             /* ISTATUS bit is read-only */
>> +             ptimer->cnt_ctl = p->regval & ~ARCH_TIMER_CTRL_IT_STAT;
>> +     } else {
>> +             u64 now = kvm_phys_timer_read();
>> +
>> +             p->regval = ptimer->cnt_ctl;
>> +             /* Set ISTATUS bit if it's expired */
>> +             if (ptimer->cnt_cval <= now)
>> +                     p->regval |= ARCH_TIMER_CTRL_IT_STAT;
>> +     }
>
> Shouldn't we take the ENABLE bit into account? The ARMv8 ARM version I
> have at hand (version h) seems to indicate that we should, but we should
> check with the latest and greatest...

Thanks! I was not clear about this. I have ARM ARM version k, and it
says that 'When the value of the ENABLE bit is 0, the ISTATUS field is
UNKNOWN.' So I thought the istatus value doesn't matter if ENABLE is
0, and just set istatus bit regardless of ENABLE bit. If this is not
what the manual meant, then I'm happy to fix this.

Thanks,
Jintack

>
>> +
>>       return true;
>>  }
>>
>> @@ -840,7 +860,13 @@ static bool access_cntp_cval(struct kvm_vcpu *vcpu,
>>               struct sys_reg_params *p,
>>               const struct sys_reg_desc *r)
>>  {
>> -     kvm_inject_undefined(vcpu);
>> +     struct arch_timer_context *ptimer = vcpu_ptimer(vcpu);
>> +
>> +     if (p->is_write)
>> +             ptimer->cnt_cval = p->regval;
>> +     else
>> +             p->regval = ptimer->cnt_cval;
>> +
>>       return true;
>>  }
>>
>> diff --git a/include/kvm/arm_arch_timer.h b/include/kvm/arm_arch_timer.h
>> index a364593..fec99f2 100644
>> --- a/include/kvm/arm_arch_timer.h
>> +++ b/include/kvm/arm_arch_timer.h
>> @@ -74,6 +74,8 @@ bool kvm_timer_should_fire(struct kvm_vcpu *vcpu,
>>  void kvm_timer_schedule(struct kvm_vcpu *vcpu);
>>  void kvm_timer_unschedule(struct kvm_vcpu *vcpu);
>>
>> +u64 kvm_phys_timer_read(void);
>> +
>>  void kvm_timer_vcpu_put(struct kvm_vcpu *vcpu);
>>
>>  void kvm_timer_init_vhe(void);
>> diff --git a/virt/kvm/arm/arch_timer.c b/virt/kvm/arm/arch_timer.c
>> index b366bb2..9eec063 100644
>> --- a/virt/kvm/arm/arch_timer.c
>> +++ b/virt/kvm/arm/arch_timer.c
>> @@ -40,7 +40,7 @@ void kvm_timer_vcpu_put(struct kvm_vcpu *vcpu)
>>       vcpu_vtimer(vcpu)->active_cleared_last = false;
>>  }
>>
>> -static u64 kvm_phys_timer_read(void)
>> +u64 kvm_phys_timer_read(void)
>>  {
>>       return timecounter->cc->read(timecounter->cc);
>>  }
>
> Thanks,
>
>         M.
> --
> Jazz is not dead. It just smells funny.
>

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

#1569937 — Re: [RFC v2 10/10] KVM: arm/arm64: Emulate the EL1 phys timer register access

On 30 January 2017 at 17:08, Jintack Lim <jintack@cs.columbia.edu> wrote:
> On Sun, Jan 29, 2017 at 10:44 AM, Marc Zyngier <marc.zyngier@arm.com> wrote:
>> Shouldn't we take the ENABLE bit into account? The ARMv8 ARM version I
>> have at hand (version h) seems to indicate that we should, but we should
>> check with the latest and greatest...
>
> Thanks! I was not clear about this. I have ARM ARM version k, and it
> says that 'When the value of the ENABLE bit is 0, the ISTATUS field is
> UNKNOWN.' So I thought the istatus value doesn't matter if ENABLE is
> 0, and just set istatus bit regardless of ENABLE bit. If this is not
> what the manual meant, then I'm happy to fix this.

It looks like the spec has been relaxed between the doc version
that Marc was looking at and the current one. So it's OK for
an implementation to either (a) set ISTATUS to 0 if ENABLE
is 0, or (b) do what you've done and set ISTATUS according
to the timer comparison whether ENABLE is clear or not
(or even (c) set ISTATUS to a random value if ENABLE is clear,
and other less likely choices).
I think we should add a comment to note that it's architecturally
UNKNOWN and we've made a choice for our implementation convenience.

thanks
-- PMM

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

#1569940 — Re: [RFC v2 10/10] KVM: arm/arm64: Emulate the EL1 phys timer register access

Hi Peter,

On Mon, Jan 30, 2017 at 12:26 PM, Peter Maydell
<peter.maydell@linaro.org> wrote:
> On 30 January 2017 at 17:08, Jintack Lim <jintack@cs.columbia.edu> wrote:
>> On Sun, Jan 29, 2017 at 10:44 AM, Marc Zyngier <marc.zyngier@arm.com> wrote:
>>> Shouldn't we take the ENABLE bit into account? The ARMv8 ARM version I
>>> have at hand (version h) seems to indicate that we should, but we should
>>> check with the latest and greatest...
>>
>> Thanks! I was not clear about this. I have ARM ARM version k, and it
>> says that 'When the value of the ENABLE bit is 0, the ISTATUS field is
>> UNKNOWN.' So I thought the istatus value doesn't matter if ENABLE is
>> 0, and just set istatus bit regardless of ENABLE bit. If this is not
>> what the manual meant, then I'm happy to fix this.
>
> It looks like the spec has been relaxed between the doc version
> that Marc was looking at and the current one. So it's OK for
> an implementation to either (a) set ISTATUS to 0 if ENABLE
> is 0, or (b) do what you've done and set ISTATUS according
> to the timer comparison whether ENABLE is clear or not
> (or even (c) set ISTATUS to a random value if ENABLE is clear,
> and other less likely choices).
> I think we should add a comment to note that it's architecturally
> UNKNOWN and we've made a choice for our implementation convenience.

Thanks for the clarification. I'll put a comment in v3.

>
> thanks
> -- PMM
>

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

#1569959 — Re: [RFC v2 10/10] KVM: arm/arm64: Emulate the EL1 phys timer register access

On 30/01/17 17:26, Peter Maydell wrote:
> On 30 January 2017 at 17:08, Jintack Lim <jintack@cs.columbia.edu> wrote:
>> On Sun, Jan 29, 2017 at 10:44 AM, Marc Zyngier <marc.zyngier@arm.com> wrote:
>>> Shouldn't we take the ENABLE bit into account? The ARMv8 ARM version I
>>> have at hand (version h) seems to indicate that we should, but we should
>>> check with the latest and greatest...
>>
>> Thanks! I was not clear about this. I have ARM ARM version k, and it
>> says that 'When the value of the ENABLE bit is 0, the ISTATUS field is
>> UNKNOWN.' So I thought the istatus value doesn't matter if ENABLE is
>> 0, and just set istatus bit regardless of ENABLE bit. If this is not
>> what the manual meant, then I'm happy to fix this.
> 
> It looks like the spec has been relaxed between the doc version
> that Marc was looking at and the current one. So it's OK for
> an implementation to either (a) set ISTATUS to 0 if ENABLE
> is 0, or (b) do what you've done and set ISTATUS according
> to the timer comparison whether ENABLE is clear or not
> (or even (c) set ISTATUS to a random value if ENABLE is clear,
> and other less likely choices).
> I think we should add a comment to note that it's architecturally
> UNKNOWN and we've made a choice for our implementation convenience.

In that case, the proposed implementation is perfectly fine. I'll retire
the old ARMv8 ARM from my laptop (funnily enough, I didn't fancy
downloading version k while on the train and having my phone as my link
to the outside world... ;-).

Thanks,

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

[toc] | [prev] | [standalone]

Back to top | Article view | linux.kernel

csiph-web