Re: [Xen-devel] [PATCH] xen/arm: remove physical timer offset

[Jeff Kubascik <jeff.kubascik@xxxxxxxxxxxxxxx>]

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@xxxxxxxxxxxxxxx>
>> ---
>>   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@xxxxxxxxxxxxxxxxxxxx
https://lists.xenproject.org/mailman/listinfo/xen-devel