| Message ID | 20180209143937.28866-18-andre.przywara@linaro.org |
|---|---|
| State | New |
| Headers | show |
| Series | New VGIC(-v2) implementation | expand |
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 :). Also do we need to do that for the emulated physical timer? > > Signed-off-by: Andre Przywara <andre.przywara@linaro.org> > --- > 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. > + * 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? > + > + /* > + * 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 :). > + */ > + level = (vtimer->ctl & 0x5) == (CNTx_CTL_ENABLE | CNTx_CTL_PENDING); Can you please use the proper define rather than plain value? > + > + 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. > + 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. > 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,
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@linaro.org> >> --- >> 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, >
On 12/02/18 18:23, Andre Przywara wrote: > Hi, Hi Andre, > 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@linaro.org> >>> --- >>> 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. I was blindly following the CODING_STYLE requirements :). But let's keep /** if it helps script. Cheers,
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); } +/** + * 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); + + /* + * 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). + * 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. + */ + level = (vtimer->ctl & 0x5) == (CNTx_CTL_ENABLE | CNTx_CTL_PENDING); + + 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); + vtimer_sync(current); 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);
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> --- xen/arch/arm/time.c | 34 ++++++++++++++++++++++++++++++++++ xen/arch/arm/traps.c | 1 + xen/include/xen/timer.h | 2 ++ 3 files changed, 37 insertions(+)