* [Xen-devel] [PATCH] xen/arm: remove physical timer offset
@ 2019-11-25 16:14 Jeff Kubascik
2019-11-25 22:07 ` Julien Grall
0 siblings, 1 reply; 3+ messages in thread
From: Jeff Kubascik @ 2019-11-25 16:14 UTC (permalink / raw)
To: xen-devel
Cc: Stewart Hildebrand, Volodymyr Babchuk, Stefano Stabellini, Julien Grall
The physical timer traps apply an offset so that time starts at 0 for
the guest. However, this offset is not currently applied to the physical
counter. Per the ARMv8 Arch Reference Manual, the offset between the
physical timer and counter should be 0. This removes the offset to make
the timer and counter consistent.
Xen time is at offset boot_count from the physical counter, so we need
to take this into account when reading/writing to CNTP_CVAL.
Signed-off-by: Jeff Kubascik <jeff.kubascik@dornerworks.com>
---
xen/arch/arm/vtimer.c | 18 ++++++------------
xen/include/asm-arm/domain.h | 3 ---
2 files changed, 6 insertions(+), 15 deletions(-)
diff --git a/xen/arch/arm/vtimer.c b/xen/arch/arm/vtimer.c
index e6aebdac9e..4790b5ce58 100644
--- a/xen/arch/arm/vtimer.c
+++ b/xen/arch/arm/vtimer.c
@@ -62,7 +62,6 @@ static void virt_timer_expired(void *data)
int domain_vtimer_init(struct domain *d, struct xen_arch_domainconfig *config)
{
- d->arch.phys_timer_base.offset = NOW();
d->arch.virt_timer_base.offset = READ_SYSREG64(CNTPCT_EL0);
d->time_offset_seconds = ticks_to_ns(d->arch.virt_timer_base.offset - boot_count);
do_div(d->time_offset_seconds, 1000000000);
@@ -184,8 +183,7 @@ static bool vtimer_cntp_ctl(struct cpu_user_regs *regs, uint32_t *r, bool read)
if ( v->arch.phys_timer.ctl & CNTx_CTL_ENABLE )
{
- set_timer(&v->arch.phys_timer.timer,
- v->arch.phys_timer.cval + v->domain->arch.phys_timer_base.offset);
+ set_timer(&v->arch.phys_timer.timer, v->arch.phys_timer.cval);
}
else
stop_timer(&v->arch.phys_timer.timer);
@@ -202,7 +200,7 @@ static bool vtimer_cntp_tval(struct cpu_user_regs *regs, uint32_t *r,
if ( !ACCESS_ALLOWED(regs, EL0PTEN) )
return false;
- now = NOW() - v->domain->arch.phys_timer_base.offset;
+ now = NOW();
if ( read )
{
@@ -214,9 +212,7 @@ static bool vtimer_cntp_tval(struct cpu_user_regs *regs, uint32_t *r,
if ( v->arch.phys_timer.ctl & CNTx_CTL_ENABLE )
{
v->arch.phys_timer.ctl &= ~CNTx_CTL_PENDING;
- set_timer(&v->arch.phys_timer.timer,
- v->arch.phys_timer.cval +
- v->domain->arch.phys_timer_base.offset);
+ set_timer(&v->arch.phys_timer.timer, v->arch.phys_timer.cval);
}
}
return true;
@@ -232,17 +228,15 @@ static bool vtimer_cntp_cval(struct cpu_user_regs *regs, uint64_t *r,
if ( read )
{
- *r = ns_to_ticks(v->arch.phys_timer.cval);
+ *r = ns_to_ticks(v->arch.phys_timer.cval) + boot_count;
}
else
{
- v->arch.phys_timer.cval = ticks_to_ns(*r);
+ v->arch.phys_timer.cval = ticks_to_ns(*r - boot_count);
if ( v->arch.phys_timer.ctl & CNTx_CTL_ENABLE )
{
v->arch.phys_timer.ctl &= ~CNTx_CTL_PENDING;
- set_timer(&v->arch.phys_timer.timer,
- v->arch.phys_timer.cval +
- v->domain->arch.phys_timer_base.offset);
+ set_timer(&v->arch.phys_timer.timer, v->arch.phys_timer.cval);
}
}
return true;
diff --git a/xen/include/asm-arm/domain.h b/xen/include/asm-arm/domain.h
index 86ebdd2bcf..16a7150a95 100644
--- a/xen/include/asm-arm/domain.h
+++ b/xen/include/asm-arm/domain.h
@@ -65,9 +65,6 @@ struct arch_domain
RELMEM_done,
} relmem;
- struct {
- uint64_t offset;
- } phys_timer_base;
struct {
uint64_t offset;
} virt_timer_base;
--
2.17.1
_______________________________________________
Xen-devel mailing list
Xen-devel@lists.xenproject.org
https://lists.xenproject.org/mailman/listinfo/xen-devel
^ permalink raw reply related [flat|nested] 3+ messages in thread* Re: [Xen-devel] [PATCH] xen/arm: remove physical timer offset
2019-11-25 16:14 [Xen-devel] [PATCH] xen/arm: remove physical timer offset Jeff Kubascik
@ 2019-11-25 22:07 ` Julien Grall
2019-11-26 15:19 ` Jeff Kubascik
0 siblings, 1 reply; 3+ messages in thread
From: Julien Grall @ 2019-11-25 22:07 UTC (permalink / raw)
To: Jeff Kubascik, xen-devel
Cc: Stewart Hildebrand, Stefano Stabellini, Volodymyr Babchuk
Hi,
On 25/11/2019 16:14, Jeff Kubascik wrote:
> The physical timer traps apply an offset so that time starts at 0 for
> the guest. However, this offset is not currently applied to the physical
> counter. Per the ARMv8 Arch Reference Manual, the offset between the
Which bit of the Arm Arm do you refer to here? In general, I would
recommend to give the exact section and version of the manual you use to
avoid any misunderstanding.
> physical timer and counter should be 0. This removes the offset to make
> the timer and counter consistent.
>
> Xen time is at offset boot_count from the physical counter, so we need
> to take this into account when reading/writing to CNTP_CVAL.
>
> Signed-off-by: Jeff Kubascik <jeff.kubascik@dornerworks.com>
> ---
> xen/arch/arm/vtimer.c | 18 ++++++------------
> xen/include/asm-arm/domain.h | 3 ---
> 2 files changed, 6 insertions(+), 15 deletions(-)
>
> diff --git a/xen/arch/arm/vtimer.c b/xen/arch/arm/vtimer.c
> index e6aebdac9e..4790b5ce58 100644
> --- a/xen/arch/arm/vtimer.c
> +++ b/xen/arch/arm/vtimer.c
> @@ -62,7 +62,6 @@ static void virt_timer_expired(void *data)
>
> int domain_vtimer_init(struct domain *d, struct xen_arch_domainconfig *config)
> {
> - d->arch.phys_timer_base.offset = NOW();
> d->arch.virt_timer_base.offset = READ_SYSREG64(CNTPCT_EL0);
> d->time_offset_seconds = ticks_to_ns(d->arch.virt_timer_base.offset - boot_count);
> do_div(d->time_offset_seconds, 1000000000);
> @@ -184,8 +183,7 @@ static bool vtimer_cntp_ctl(struct cpu_user_regs *regs, uint32_t *r, bool read)
>
> if ( v->arch.phys_timer.ctl & CNTx_CTL_ENABLE )
> {
> - set_timer(&v->arch.phys_timer.timer,
> - v->arch.phys_timer.cval + v->domain->arch.phys_timer_base.offset);
> + set_timer(&v->arch.phys_timer.timer, v->arch.phys_timer.cval);
> }
> else
> stop_timer(&v->arch.phys_timer.timer);
> @@ -202,7 +200,7 @@ static bool vtimer_cntp_tval(struct cpu_user_regs *regs, uint32_t *r,
> if ( !ACCESS_ALLOWED(regs, EL0PTEN) )
> return false;
>
> - now = NOW() - v->domain->arch.phys_timer_base.offset;
> + now = NOW();
>
> if ( read )
> {
> @@ -214,9 +212,7 @@ static bool vtimer_cntp_tval(struct cpu_user_regs *regs, uint32_t *r,
> if ( v->arch.phys_timer.ctl & CNTx_CTL_ENABLE )
> {
> v->arch.phys_timer.ctl &= ~CNTx_CTL_PENDING;
> - set_timer(&v->arch.phys_timer.timer,
> - v->arch.phys_timer.cval +
> - v->domain->arch.phys_timer_base.offset);
> + set_timer(&v->arch.phys_timer.timer, v->arch.phys_timer.cval);
> }
> }
> return true;
> @@ -232,17 +228,15 @@ static bool vtimer_cntp_cval(struct cpu_user_regs *regs, uint64_t *r,
>
> if ( read )
> {
> - *r = ns_to_ticks(v->arch.phys_timer.cval);
> + *r = ns_to_ticks(v->arch.phys_timer.cval) + boot_count;
> }
> else
> {
> - v->arch.phys_timer.cval = ticks_to_ns(*r);
> + v->arch.phys_timer.cval = ticks_to_ns(*r - boot_count);
I know that this is already like that in the code. But it feels weird
(to not say wrong) that cval will have a different meaning between the
virtual timer and physical timer.
Indeed, in the former case it is an exact copy of the hardware value
whilst in the latter it is the hardware value - NOW().
While you are reworking a big chunk of the physical timer emulation,
could you looking at removing this discrepancy?
> if ( v->arch.phys_timer.ctl & CNTx_CTL_ENABLE )
> {
> v->arch.phys_timer.ctl &= ~CNTx_CTL_PENDING;
> - set_timer(&v->arch.phys_timer.timer,
> - v->arch.phys_timer.cval +
> - v->domain->arch.phys_timer_base.offset);
> + set_timer(&v->arch.phys_timer.timer, v->arch.phys_timer.cval);
> }
> }
> return true;
> diff --git a/xen/include/asm-arm/domain.h b/xen/include/asm-arm/domain.h
> index 86ebdd2bcf..16a7150a95 100644
> --- a/xen/include/asm-arm/domain.h
> +++ b/xen/include/asm-arm/domain.h
> @@ -65,9 +65,6 @@ struct arch_domain
> RELMEM_done,
> } relmem;
>
> - struct {
> - uint64_t offset;
> - } phys_timer_base;
> struct {
> uint64_t offset;
> } virt_timer_base;
>
Cheers,
--
Julien Grall
_______________________________________________
Xen-devel mailing list
Xen-devel@lists.xenproject.org
https://lists.xenproject.org/mailman/listinfo/xen-devel
^ permalink raw reply [flat|nested] 3+ messages in thread* Re: [Xen-devel] [PATCH] xen/arm: remove physical timer offset
2019-11-25 22:07 ` Julien Grall
@ 2019-11-26 15:19 ` Jeff Kubascik
0 siblings, 0 replies; 3+ messages in thread
From: Jeff Kubascik @ 2019-11-26 15:19 UTC (permalink / raw)
To: Julien Grall, xen-devel
Cc: Stewart Hildebrand, Stefano Stabellini, Volodymyr Babchuk
On 11/25/2019 5:07 PM, Julien Grall wrote:
> Hi,
>
> On 25/11/2019 16:14, Jeff Kubascik wrote:
>> The physical timer traps apply an offset so that time starts at 0 for
>> the guest. However, this offset is not currently applied to the physical
>> counter. Per the ARMv8 Arch Reference Manual, the offset between the
>
> Which bit of the Arm Arm do you refer to here? In general, I would
> recommend to give the exact section and version of the manual you use to
> avoid any misunderstanding.
Fair point, I'll clarify this.
>> physical timer and counter should be 0. This removes the offset to make
>> the timer and counter consistent.
>>
>> Xen time is at offset boot_count from the physical counter, so we need
>> to take this into account when reading/writing to CNTP_CVAL.
>>
>> Signed-off-by: Jeff Kubascik <jeff.kubascik@dornerworks.com>
>> ---
>> xen/arch/arm/vtimer.c | 18 ++++++------------
>> xen/include/asm-arm/domain.h | 3 ---
>> 2 files changed, 6 insertions(+), 15 deletions(-)
>>
>> diff --git a/xen/arch/arm/vtimer.c b/xen/arch/arm/vtimer.c
>> index e6aebdac9e..4790b5ce58 100644
>> --- a/xen/arch/arm/vtimer.c
>> +++ b/xen/arch/arm/vtimer.c
>> @@ -62,7 +62,6 @@ static void virt_timer_expired(void *data)
>>
>> int domain_vtimer_init(struct domain *d, struct xen_arch_domainconfig *config)
>> {
>> - d->arch.phys_timer_base.offset = NOW();
>> d->arch.virt_timer_base.offset = READ_SYSREG64(CNTPCT_EL0);
>> d->time_offset_seconds = ticks_to_ns(d->arch.virt_timer_base.offset - boot_count);
>> do_div(d->time_offset_seconds, 1000000000);
>> @@ -184,8 +183,7 @@ static bool vtimer_cntp_ctl(struct cpu_user_regs *regs, uint32_t *r, bool read)
>>
>> if ( v->arch.phys_timer.ctl & CNTx_CTL_ENABLE )
>> {
>> - set_timer(&v->arch.phys_timer.timer,
>> - v->arch.phys_timer.cval + v->domain->arch.phys_timer_base.offset);
>> + set_timer(&v->arch.phys_timer.timer, v->arch.phys_timer.cval);
>> }
>> else
>> stop_timer(&v->arch.phys_timer.timer);
>> @@ -202,7 +200,7 @@ static bool vtimer_cntp_tval(struct cpu_user_regs *regs, uint32_t *r,
>> if ( !ACCESS_ALLOWED(regs, EL0PTEN) )
>> return false;
>>
>> - now = NOW() - v->domain->arch.phys_timer_base.offset;
>> + now = NOW();
>>
>> if ( read )
>> {
>> @@ -214,9 +212,7 @@ static bool vtimer_cntp_tval(struct cpu_user_regs *regs, uint32_t *r,
>> if ( v->arch.phys_timer.ctl & CNTx_CTL_ENABLE )
>> {
>> v->arch.phys_timer.ctl &= ~CNTx_CTL_PENDING;
>> - set_timer(&v->arch.phys_timer.timer,
>> - v->arch.phys_timer.cval +
>> - v->domain->arch.phys_timer_base.offset);
>> + set_timer(&v->arch.phys_timer.timer, v->arch.phys_timer.cval);
>> }
>> }
>> return true;
>> @@ -232,17 +228,15 @@ static bool vtimer_cntp_cval(struct cpu_user_regs *regs, uint64_t *r,
>>
>> if ( read )
>> {
>> - *r = ns_to_ticks(v->arch.phys_timer.cval);
>> + *r = ns_to_ticks(v->arch.phys_timer.cval) + boot_count;
>> }
>> else
>> {
>> - v->arch.phys_timer.cval = ticks_to_ns(*r);
>> + v->arch.phys_timer.cval = ticks_to_ns(*r - boot_count);
>
> I know that this is already like that in the code. But it feels weird
> (to not say wrong) that cval will have a different meaning between the
> virtual timer and physical timer.
>
> Indeed, in the former case it is an exact copy of the hardware value
> whilst in the latter it is the hardware value - NOW().
I noticed this discrepancy as well. Even worse, the virtual timer cval is in
ticks and the physical timer cval is Xen system time in ns.
I believe that changing the physical timer cval to be the hardware value in
ticks would be the more correct approach. The conversion to Xen system time is
only needed for the timer APIs.
> While you are reworking a big chunk of the physical timer emulation,
> could you looking at removing this discrepancy?
>
>> if ( v->arch.phys_timer.ctl & CNTx_CTL_ENABLE )
>> {
>> v->arch.phys_timer.ctl &= ~CNTx_CTL_PENDING;
>> - set_timer(&v->arch.phys_timer.timer,
>> - v->arch.phys_timer.cval +
>> - v->domain->arch.phys_timer_base.offset);
>> + set_timer(&v->arch.phys_timer.timer, v->arch.phys_timer.cval);
>> }
>> }
>> return true;
>> diff --git a/xen/include/asm-arm/domain.h b/xen/include/asm-arm/domain.h
>> index 86ebdd2bcf..16a7150a95 100644
>> --- a/xen/include/asm-arm/domain.h
>> +++ b/xen/include/asm-arm/domain.h
>> @@ -65,9 +65,6 @@ struct arch_domain
>> RELMEM_done,
>> } relmem;
>>
>> - struct {
>> - uint64_t offset;
>> - } phys_timer_base;
>> struct {
>> uint64_t offset;
>> } virt_timer_base;
>>
>
> Cheers,
>
> --
> Julien Grall
>
Sure thing, I'll update the patch accordingly.
Sincerely,
Jeff Kubascik
_______________________________________________
Xen-devel mailing list
Xen-devel@lists.xenproject.org
https://lists.xenproject.org/mailman/listinfo/xen-devel
^ permalink raw reply [flat|nested] 3+ messages in thread
end of thread, other threads:[~2019-11-26 15:19 UTC | newest]
Thread overview: 3+ messages (download: mbox.gz / follow: Atom feed)
-- links below jump to the message on this page --
2019-11-25 16:14 [Xen-devel] [PATCH] xen/arm: remove physical timer offset Jeff Kubascik
2019-11-25 22:07 ` Julien Grall
2019-11-26 15:19 ` Jeff Kubascik
This is an external index of several public inboxes,
see mirroring instructions on how to clone and mirror
all data and code used by this external index.