On 11/12/17 21:51, Auger Eric wrote:
> Hi Christoffer,
> On 07/12/17 11:54, Christoffer Dall wrote:
>> The timer was modeled after a strict idea of modelling an interrupt line
>> level in software, meaning that only transitions in the level needed to
>> be reported to the VGIC. This works well for the timer, because the
>> arch timer code is in complete control of the device and can track the
>> transitions of the line.
>>
>> However, as we are about to support using the HW bit in the VGIC not
>> just for the timer, but also for VFIO which cannot track transitions of
>> the interrupt line, we have to decide on an interface for level
>> triggered mapped interrupts to the GIC, which both the timer and VFIO
>> can use.
>>
>> VFIO only sees an asserting transition of the physical interrupt line,
>> and tells the VGIC when that happens. That means that part of the
>> interrupt flow is offloaded to the hardware.
>>
>> To use the same interface for VFIO devices and the timer, we therefore
>> have to change the timer (we cannot change VFIO because it doesn't know
>> the details of the device it is assigning to a VM).
>>
>> Luckily, changing the timer is simple, we just need to stop 'caching'
>> the line level, but instead let the VGIC know the state of the timer
>> every time there is a potential change in the line level, and when the
>> line level should be asserted from the timer ISR. The VGIC can ignore
>> extra notifications using its validate mechanism.
>
> I was confused by the fact we say we stop caching the line level but
> vtimer->irq.level still exists, is updated in the vtimer host ISR and
> kvm_timer_update_state() and read in many places.
>
> I feel difficult to figure out if each time we use the vtimer->irq.level
> value it is safe to use it.
>
> Also for the validate() to succeed we need the vgic irq->line_level to
> to be 0. I understand this is properly handled for mapped level irqs in
> next patch which does that on the populate_lr. However I currently fail
> to understand why the timer level sensitive mapped IRQ does not require
> the next patch to work.
OK reading again "[PATCH v7 7/8] KVM: arm/arm64: Provide a
get_input_level for the arch timer", I now understand it works because
we had the
kvm_timer_sync_hwstate toggling down the line on VM exit. After the
changes of next patch this can be safely removed.
Not related to this patch but I noticed
Documentation/virtual/kvm/arm/vgic-mapped-irqs.txt now is outdated.
>
> Thanks
>
> Eric
>
>>
>> Reviewed-by: Andre Przywara <andre.przywara@xxxxxxx>
>> Signed-off-by: Christoffer Dall <christoffer.dall@xxxxxxxxxx>
>> ---
>> virt/kvm/arm/arch_timer.c | 20 +++++++++++++-------
>> 1 file changed, 13 insertions(+), 7 deletions(-)
>>
>> diff --git a/virt/kvm/arm/arch_timer.c b/virt/kvm/arm/arch_timer.c
>> index 4151250ce8da..dd5aca05c500 100644
>> --- a/virt/kvm/arm/arch_timer.c
>> +++ b/virt/kvm/arm/arch_timer.c
>> @@ -99,11 +99,9 @@ static irqreturn_t kvm_arch_timer_handler(int irq, void *dev_id)
>> }
>> vtimer = vcpu_vtimer(vcpu);
>>
>> - if (!vtimer->irq.level) {
>> - vtimer->cnt_ctl = read_sysreg_el0(cntv_ctl);
>> - if (kvm_timer_irq_can_fire(vtimer))
>> - kvm_timer_update_irq(vcpu, true, vtimer);
>> - }
>> + vtimer->cnt_ctl = read_sysreg_el0(cntv_ctl);
>> + if (kvm_timer_irq_can_fire(vtimer))
>> + kvm_timer_update_irq(vcpu, true, vtimer);
>>
>> if (unlikely(!irqchip_in_kernel(vcpu->kvm)))
>> kvm_vtimer_update_mask_user(vcpu);
>> @@ -324,12 +322,20 @@ static void kvm_timer_update_state(struct kvm_vcpu *vcpu)
>> struct arch_timer_cpu *timer = &vcpu->arch.timer_cpu;
>> struct arch_timer_context *vtimer = vcpu_vtimer(vcpu);
>> struct arch_timer_context *ptimer = vcpu_ptimer(vcpu);
>> + bool level;
>>
>> if (unlikely(!timer->enabled))
>> return;
>>
>> - if (kvm_timer_should_fire(vtimer) != vtimer->irq.level)
>> - kvm_timer_update_irq(vcpu, !vtimer->irq.level, vtimer);
>> + /*
>> + * The vtimer virtual interrupt is a 'mapped' interrupt, meaning part
>> + * of its lifecycle is offloaded to the hardware, and we therefore may
>> + * not have lowered the irq.level value before having to signal a new
>> + * interrupt, but have to signal an interrupt every time the level is
>> + * asserted.
>> + */
>> + level = kvm_timer_should_fire(vtimer);
>> + kvm_timer_update_irq(vcpu, level, vtimer);
>>
>> if (kvm_timer_should_fire(ptimer) != ptimer->irq.level)
>> kvm_timer_update_irq(vcpu, !ptimer->irq.level, ptimer);
>>
