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

[Jeff Kubascik <jeff.kubascik@xxxxxxxxxxxxxxx>]

On 1/23/2020 7:32 AM, Julien Grall wrote:
> Hi,
> 
> On 21/01/2020 15:07, 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 Reference Manual (ARM DDI 0487E.a), section
>> D11.2.4 Timers, the "Offset" between the counter and timer should be
>> zero for a physical timer. This removes the offset to make the timer and
>> counter consistent.
>>
>> This also cleans up the physical timer implementation to better match
>> the virtual timer - both cval's now hold the hardware value.
>>
>> Signed-off-by: Jeff Kubascik <jeff.kubascik@xxxxxxxxxxxxxxx>
>> ---
>>   xen/arch/arm/vtimer.c        | 34 ++++++++++++++++++----------------
>>   xen/include/asm-arm/domain.h |  3 ---
>>   2 files changed, 18 insertions(+), 19 deletions(-)
>>
>> diff --git a/xen/arch/arm/vtimer.c b/xen/arch/arm/vtimer.c
>> index 240a850b6e..0c78a65863 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);
>> @@ -108,7 +107,6 @@ int vcpu_vtimer_init(struct vcpu *v)
>>
>>       init_timer(&t->timer, phys_timer_expired, t, v->processor);
>>       t->ctl = 0;
>> -    t->cval = NOW();
>>       t->irq = d0
>>           ? timer_get_irq(TIMER_PHYS_NONSECURE_PPI)
>>           : GUEST_TIMER_PHYS_NS_PPI;
>> @@ -167,6 +165,7 @@ void virt_timer_restore(struct vcpu *v)
>>   static bool vtimer_cntp_ctl(struct cpu_user_regs *regs, uint32_t *r, bool 
>> read)
>>   {
>>       struct vcpu *v = current;
>> +    s_time_t expires;
>>
>>       if ( !ACCESS_ALLOWED(regs, EL0PTEN) )
>>           return false;
>> @@ -184,8 +183,9 @@ 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);
>> +            expires = v->arch.phys_timer.cval > boot_count
>> +                      ? ticks_to_ns(v->arch.phys_timer.cval - boot_count) : 
>> 0;
>> +            set_timer(&v->arch.phys_timer.timer, expires);
> 
> While the factoring was optional, my request of a comment wasn't (even
> if it requires to duplicate 3 times).

Understood.

> If you suggest a comment and an update of the commit message, I can
> merge it in this patch on commit.

For the comment:

/* If cval is before the point Xen started, expire timer immediately */

For the commit message:

In the case the guest sets cval to a time before Xen started, the correct
behavior is to expire the timer immediately. To do this, we set the expires
argument of set_timer to zero.

> Cheers,
> 
> --
> Julien Grall
> 

Sincerely,
Jeff Kubascik

_______________________________________________
Xen-devel mailing list
Xen-devel@xxxxxxxxxxxxxxxxxxxx
https://lists.xenproject.org/mailman/listinfo/xen-devel