|
[Date Prev][Date Next][Thread Prev][Thread Next][Date Index][Thread Index] Re: [Xen-devel] [PATCH v3 1/2] xen/arm: remove physical timer offset
On 12/18/2019 9:20 AM, Julien Grall wrote:
> Hi Jeff,
>
> On 11/12/2019 21:13, 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 e6aebdac9e..21b98ec20a 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);
>> }
>> else
>> stop_timer(&v->arch.phys_timer.timer);
>> @@ -197,26 +197,27 @@ static bool vtimer_cntp_tval(struct cpu_user_regs
>> *regs, uint32_t *r,
>> bool read)
>> {
>> struct vcpu *v = current;
>> - s_time_t now;
>> + uint64_t cntpct;
>> + s_time_t expires;
>>
>> if ( !ACCESS_ALLOWED(regs, EL0PTEN) )
>> return false;
>>
>> - now = NOW() - v->domain->arch.phys_timer_base.offset;
>> + cntpct = get_cycles();
>>
>> if ( read )
>> {
>> - *r = (uint32_t)(ns_to_ticks(v->arch.phys_timer.cval - now) &
>> 0xffffffffull);
>> + *r = (uint32_t)((v->arch.phys_timer.cval - cntpct) & 0xffffffffull);
>> }
>> else
>> {
>> - v->arch.phys_timer.cval = now + ticks_to_ns(*r);
>> + v->arch.phys_timer.cval = cntpct + *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);
>> + expires = v->arch.phys_timer.cval > boot_count
>> + ? ticks_to_ns(v->arch.phys_timer.cval - boot_count) :
>> 0;
>
> You probably want a comment to explain why you set to 0 here.
This code is repeated in 3 places - it probably doesn't make sense to have 3
duplicate comments as well. I think it would fit well with your function
suggestion below.
>> + set_timer(&v->arch.phys_timer.timer, expires);
>> }
>> }
>> return true;
>> @@ -226,23 +227,24 @@ static bool vtimer_cntp_cval(struct cpu_user_regs
>> *regs, uint64_t *r,
>> bool read)
>> {
>> struct vcpu *v = current;
>> + s_time_t expires;
>>
>> if ( !ACCESS_ALLOWED(regs, EL0PTEN) )
>> return false;
>>
>> if ( read )
>> {
>> - *r = ns_to_ticks(v->arch.phys_timer.cval);
>> + *r = v->arch.phys_timer.cval;
>> }
>> else
>> {
>> - v->arch.phys_timer.cval = ticks_to_ns(*r);
>> + v->arch.phys_timer.cval = *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);
>> + expires = v->arch.phys_timer.cval > boot_count
>> + ? ticks_to_ns(v->arch.phys_timer.cval - boot_count) :
>> 0;
>
> Same here. But I am wondering whether we could factor this code in a
> function. This would avoid code duplication and make the code simpler.
>
> This can be done as a follow-up as we may want to backport the fix.
This is a great idea. However, I would consider this further scope creep for
this patch set - I think what is here is already a great improvement. I am
currently focused on wrapping up a project; unfortunately, this change would be
low on the priority list for me and I may not be able to get to it.
>> + set_timer(&v->arch.phys_timer.timer, expires);
>> }
>> }
>> return true;
>> diff --git a/xen/include/asm-arm/domain.h b/xen/include/asm-arm/domain.h
>> index f3f3fb7d7f..adc7fe7210 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
>
Sincerely,
Jeff Kubascik
_______________________________________________
Xen-devel mailing list
Xen-devel@xxxxxxxxxxxxxxxxxxxx
https://lists.xenproject.org/mailman/listinfo/xen-devel
|
 |
Lists.xenproject.org is hosted with RackSpace, monitoring our |