Re: [Xen-devel] [RFC PATCH 17/49] ARM: timer: Handle level triggered IRQs correctly

[Andre Przywara <andre.przywara@xxxxxxxxxx>]

Hi,

On 12/02/18 15:19, Julien Grall wrote:
> Hi Andre,
> 
> On 09/02/18 14:39, Andre Przywara wrote:
>> The ARM Generic Timer uses a level-sensitive interrupt semantic. We
>> easily catch when the line goes high, as this triggers the hardware IRQ.
>> However we have to sync the state of the interrupt condition at certain
>> points to catch when the line goes low and we can remove the vtimer vIRQ
>> from the vGIC (and the LR).
>> The VGIC in Xen so far only implemented edge triggered vIRQs, really, so
>> we need to add new functionality to re-sample the interrupt state.
> 
> You might want to make a summary of the discussion we had with Marc Z.
> today here. This would help the other to understand why sample the
> interrupt state is necessary :).

Yes, I just saw that I somehow missed copying the elaborate comment from
Christoffer. Fixed now, indeed without this background it's next to
impossible to understand this ;-)

> Also do we need to do that for the emulated physical timer?

Mmh, good question. I believe this whole timer story needs a good think
again.

>>
>> Signed-off-by: Andre Przywara <andre.przywara@xxxxxxxxxx>
>> ---
>>   xen/arch/arm/time.c     | 34 ++++++++++++++++++++++++++++++++++
>>   xen/arch/arm/traps.c    |  1 +
>>   xen/include/xen/timer.h |  2 ++
>>   3 files changed, 37 insertions(+)
>>
>> diff --git a/xen/arch/arm/time.c b/xen/arch/arm/time.c
>> index c11fcfeadd..98ebb4305d 100644
>> --- a/xen/arch/arm/time.c
>> +++ b/xen/arch/arm/time.c
>> @@ -263,6 +263,40 @@ static void vtimer_interrupt(int irq, void
>> *dev_id, struct cpu_user_regs *regs)
>>       vgic_inject_irq(current->domain, current,
>> current->arch.virt_timer.irq, true);
>>   }
>>   +/**
> 
> One * is enough.

That's kernel-doc comment style:
https://www.kernel.org/doc/html/v4.9/kernel-documentation.html#writing-kernel-doc-comments

That allows tools to scan the tree and extract function documentation in
an automated way. A bit like markdown: still perfectly readable by
humans, but parse-able by scripts as well.

I was hoping that it wouldn't hurt to have this in Xen as well, as I
copied this already in other parts of this code.

>> + * vtimer_sync() - update the state of the virtual timer after a
>> guest run
>> + * @vcpu: The VCPU to sync the arch timer state
>> + *
>> + * After returning from a guest, update the state of the virtual
>> interrupt
>> + * line, to model the level triggered interrupt correctly.
>> + * If the guest has handled a timer interrupt, the virtual interrupt
>> line
>> + * needs to be lowered explicitly. vgic_inject_irq() takes care of that.
>> + */
>> +void vtimer_sync(struct vcpu *vcpu)
>> +{
>> +    struct vtimer *vtimer = &vcpu->arch.virt_timer;
>> +    bool level;
>> +
>> +    vtimer->ctl = READ_SYSREG32(CNTV_CTL_EL0);
>> +    vtimer->cval = READ_SYSREG64(CNTV_CVAL_EL0);
> 
> Why do you need to save cval?

Originally I was copying KVM code which checked the actual IRQ condition
(by comparing the counter with CVAL). So this might be a leftover from
there. Need to check whether we actually need an up-to-date value of this.

>> +
>> +    /*
>> +     * Technically we should mask with 0x7 here, to catch if the timer
>> +     * interrupt is masked. However Xen always masks the timer upon
>> entering
>> +     * the hypervisor, leaving it up to the guest to un-mask it.
>> +     * So we would always read a "low" level, despite the condition
>> being
>> +     * actually "high". Igoring the mask bit solves this (for now).
> 
> s/Igoring/Ignoring/
> 
>> +     * Another possible check would be to compare the value of
>> CNTVCT_EL0
>> +     * against vtimer->cval and derive the interrupt state from that.
>> +     *
>> +     * TODO: The proper fix for this is to make vtimer vIRQ hardware
>> mapped,
>> +     * but this requires reworking the arch timer to implement this.
> 
> That something we should look at it once the vGIC is done :).

Indeed, looking forward to it (well, somewhat ... ) ;-)

>> +     */
>> +    level = (vtimer->ctl & 0x5) == (CNTx_CTL_ENABLE | CNTx_CTL_PENDING);
> 
> Can you please use the proper define rather than plain value?

Ah, right, that was a leftover from experimentation.
Also a test to see if reviewers are really reading this ;-)

>> +
>> +    vgic_inject_irq(vcpu->domain, vcpu, vtimer->irq, level);
>> +}
>> +
>>   /*
>>    * Arch timer interrupt really ought to be level triggered, since the
>>    * design of the timer/comparator mechanism is based around that
>> diff --git a/xen/arch/arm/traps.c b/xen/arch/arm/traps.c
>> index 1cba7e584d..2d770a14a5 100644
>> --- a/xen/arch/arm/traps.c
>> +++ b/xen/arch/arm/traps.c
>> @@ -2024,6 +2024,7 @@ static void enter_hypervisor_head(struct
>> cpu_user_regs *regs)
>>           if ( current->arch.hcr_el2 & HCR_VA )
>>               current->arch.hcr_el2 = READ_SYSREG(HCR_EL2);
>>   
> 
> You need to sample the virtual timer before clearing the LRs, right? If
> so, you likely want to add a comment here to avoid reshuffling the code.

Yes, good point.

>> +        vtimer_sync(current);
> 
> I am a bit worry about re-sampling the virtual interrupt state at every
> traps. It might be worth thinking to do the re-sample when syncing the
> LRs (as you do for HW level interrupt in patch #25). Probably once we
> get the new vGIC merged.

I share your concerns, but didn't dare to optimise this yet. But indeed
it is something worth to think about.

Cheers,
Andre.

>>           gic_clear_lrs(current);
>>       }
>>   }
>> diff --git a/xen/include/xen/timer.h b/xen/include/xen/timer.h
>> index 4513260b0d..eddbbf3903 100644
>> --- a/xen/include/xen/timer.h
>> +++ b/xen/include/xen/timer.h
>> @@ -94,6 +94,8 @@ DECLARE_PER_CPU(s_time_t, timer_deadline);
>>   /* Arch-defined function to reprogram timer hardware for new
>> deadline. */
>>   int reprogram_timer(s_time_t timeout);
>>   +void vtimer_sync(struct vcpu *vcpu);
>> +
>>   /* Calculate the aligned first tick time for a given periodic timer. */
>>   s_time_t align_timer(s_time_t firsttick, uint64_t period);
>>  
> 
> Cheers,
> 

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