| Message ID | 20180305160415.16760-25-andre.przywara@linaro.org |
|---|---|
| State | Superseded |
| Headers | show |
| Series | New VGIC(-v2) implementation | expand |
Hi Andre, On 05/03/18 16:03, 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. As requested on the previous series, can you please explain in the commit message/code why sampling is necessary? Also, do we need to do that for the emulated physical timer? > > Signed-off-by: Andre Przywara <andre.przywara@linaro.org> > --- > Changelog RFC ... v1: > - extend comments > - don't read CNTV_CVAL_EL0 > - use symbolic names for constants > > xen/arch/arm/time.c | 36 ++++++++++++++++++++++++++++++++++++ > xen/arch/arm/traps.c | 6 ++++++ > xen/include/xen/timer.h | 2 ++ > 3 files changed, 44 insertions(+) > > diff --git a/xen/arch/arm/time.c b/xen/arch/arm/time.c > index c11fcfeadd..c0ae781ecd 100644 > --- a/xen/arch/arm/time.c > +++ b/xen/arch/arm/time.c > @@ -263,6 +263,42 @@ 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); > } > > +/** > + * 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; > + uint32_t vtimer_ctl = READ_SYSREG32(CNTV_CTL_EL0); > + bool level; > + > + /* > + * Technically the mask should include the CNTx_CTL_MASK bit 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". > + * Ignoring the mask bit solves this (for now). > + * Another possible check would be to compare the value of CNTVCT_EL0 > + * against vtimer->cval and derive the interrupt state from that. > + */ > + vtimer_ctl &= (CNTx_CTL_ENABLE | CNTx_CTL_PENDING); > + level = (vtimer_ctl == (CNTx_CTL_ENABLE | CNTx_CTL_PENDING)); > + > + /* > + * TODO: The proper fix for this is to make vtimer vIRQ hardware mapped, > + * but this requires reworking the arch timer to implement this. > + */ The comment seems to be offset by one space. > + > + 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 7411bff7a7..0713723bb7 100644 > --- a/xen/arch/arm/traps.c > +++ b/xen/arch/arm/traps.c > @@ -2024,6 +2024,12 @@ 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); > > + /* > + * We need to update the state of our emulated devices using level > + * triggered interrupts before syncing back the VGIC state. > + */ > + vtimer_sync(current); Am I wondering if it would be worth to #ifdef the code so it is only used for the new vGIC? After all, this will be a nop for it, right? > + > vgic_sync_from_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,
On 06/03/18 17:15, Julien Grall wrote: > On 05/03/18 16:03, Andre Przywara wrote: >> /* >> * 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 7411bff7a7..0713723bb7 100644 >> --- a/xen/arch/arm/traps.c >> +++ b/xen/arch/arm/traps.c >> @@ -2024,6 +2024,12 @@ 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); >> + /* >> + * We need to update the state of our emulated devices using >> level >> + * triggered interrupts before syncing back the VGIC state. >> + */ >> + vtimer_sync(current); > > Am I wondering if it would be worth to #ifdef the code so it is only > used for the new vGIC? After all, this will be a nop for it, right? Also, I would like to see a TODO in the code and the cover letter about optimizing this (see RFC patch #17): "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." It is not a deal breaker right now. But I don't want see to be forgotten once after we merge it. Cheers,
diff --git a/xen/arch/arm/time.c b/xen/arch/arm/time.c index c11fcfeadd..c0ae781ecd 100644 --- a/xen/arch/arm/time.c +++ b/xen/arch/arm/time.c @@ -263,6 +263,42 @@ 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); } +/** + * 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; + uint32_t vtimer_ctl = READ_SYSREG32(CNTV_CTL_EL0); + bool level; + + /* + * Technically the mask should include the CNTx_CTL_MASK bit 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". + * Ignoring the mask bit solves this (for now). + * Another possible check would be to compare the value of CNTVCT_EL0 + * against vtimer->cval and derive the interrupt state from that. + */ + vtimer_ctl &= (CNTx_CTL_ENABLE | CNTx_CTL_PENDING); + level = (vtimer_ctl == (CNTx_CTL_ENABLE | CNTx_CTL_PENDING)); + + /* + * TODO: The proper fix for this is to make vtimer vIRQ hardware mapped, + * but this requires reworking the arch timer to implement 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 7411bff7a7..0713723bb7 100644 --- a/xen/arch/arm/traps.c +++ b/xen/arch/arm/traps.c @@ -2024,6 +2024,12 @@ 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); + /* + * We need to update the state of our emulated devices using level + * triggered interrupts before syncing back the VGIC state. + */ + vtimer_sync(current); + vgic_sync_from_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);
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. Signed-off-by: Andre Przywara <andre.przywara@linaro.org> --- Changelog RFC ... v1: - extend comments - don't read CNTV_CVAL_EL0 - use symbolic names for constants xen/arch/arm/time.c | 36 ++++++++++++++++++++++++++++++++++++ xen/arch/arm/traps.c | 6 ++++++ xen/include/xen/timer.h | 2 ++ 3 files changed, 44 insertions(+)