* [PATCH v2 0/2] hpet: add support for level triggered interrupts @ 2018-03-30 12:35 Roger Pau Monne 2018-03-30 12:35 ` [PATCH v2 1/2] x86/vpt: execute callbacks for masked interrupts Roger Pau Monne 2018-03-30 12:36 ` [PATCH v2 2/2] vhpet: add support for level triggered interrupts Roger Pau Monne 0 siblings, 2 replies; 7+ messages in thread From: Roger Pau Monne @ 2018-03-30 12:35 UTC (permalink / raw) To: xen-devel; +Cc: Roger Pau Monne Hello, Patch 1 is a pre-requisite for patch 2 that adds support for level triggered interrupts to the emulated HPET code. Thanks, Roger. Roger Pau Monne (2): x86/vpt: execute callbacks for masked interrupts vhpet: add support for level triggered interrupts xen/arch/x86/hvm/hpet.c | 46 ++++++++++++++++++++++++++++++++++------------ xen/arch/x86/hvm/vpt.c | 33 ++++++++++++++++++++++++++++++++- 2 files changed, 66 insertions(+), 13 deletions(-) -- 2.16.3 _______________________________________________ Xen-devel mailing list Xen-devel@lists.xenproject.org https://lists.xenproject.org/mailman/listinfo/xen-devel ^ permalink raw reply [flat|nested] 7+ messages in thread
* [PATCH v2 1/2] x86/vpt: execute callbacks for masked interrupts 2018-03-30 12:35 [PATCH v2 0/2] hpet: add support for level triggered interrupts Roger Pau Monne @ 2018-03-30 12:35 ` Roger Pau Monne 2018-04-09 15:34 ` Jan Beulich 2018-03-30 12:36 ` [PATCH v2 2/2] vhpet: add support for level triggered interrupts Roger Pau Monne 1 sibling, 1 reply; 7+ messages in thread From: Roger Pau Monne @ 2018-03-30 12:35 UTC (permalink / raw) To: xen-devel; +Cc: Andrew Cooper, Stefan Bader, Jan Beulich, Roger Pau Monne Execute periodic_time callbacks even if the interrupt is not actually injected because the IRQ is masked. Current callbacks from emulated timer devices only update emulated registers, which from my reading of the specs should happen regardless of whether the interrupt has been injected or not. Signed-off-by: Roger Pau Monné <roger.pau@citrix.com> --- Cc: Jan Beulich <jbeulich@suse.com> Cc: Andrew Cooper <andrew.cooper3@citrix.com> Cc: Stefan Bader <stefan.bader@canonical.com> --- Changes since v1: - s/purged/to_purge/. - Force caller of execute_callbacks to hold the lock. --- xen/arch/x86/hvm/vpt.c | 33 ++++++++++++++++++++++++++++++++- 1 file changed, 32 insertions(+), 1 deletion(-) diff --git a/xen/arch/x86/hvm/vpt.c b/xen/arch/x86/hvm/vpt.c index 9d27b483b2..e6c2aa01ac 100644 --- a/xen/arch/x86/hvm/vpt.c +++ b/xen/arch/x86/hvm/vpt.c @@ -247,9 +247,29 @@ static void pt_timer_fn(void *data) pt_unlock(pt); } +static void execute_callbacks(struct vcpu *v, struct list_head *tm) +{ + while ( !list_empty(tm) ) + { + struct periodic_time *pt = list_first_entry(tm, struct periodic_time, + list); + time_cb *cb = pt->cb; + void *cb_priv = pt->priv; + + list_del(&pt->list); + pt->on_list = 0; + spin_unlock(&v->arch.hvm_vcpu.tm_lock); + + cb(v, cb_priv); + + spin_lock(&v->arch.hvm_vcpu.tm_lock); + } +} + int pt_update_irq(struct vcpu *v) { struct list_head *head = &v->arch.hvm_vcpu.tm_list; + LIST_HEAD(to_purge); struct periodic_time *pt, *temp, *earliest_pt; uint64_t max_lag; int irq, is_lapic, pt_vector; @@ -267,7 +287,10 @@ int pt_update_irq(struct vcpu *v) { /* suspend timer emulation */ list_del(&pt->list); - pt->on_list = 0; + if ( pt->cb ) + list_add(&pt->list, &to_purge); + else + pt->on_list = 0; } else { @@ -282,6 +305,12 @@ int pt_update_irq(struct vcpu *v) if ( earliest_pt == NULL ) { + /* + * NB: although the to_purge list is local, calls to + * destroy_periodic_time can still remove items from the list, hence + * the need to hold the lock while accessing it. + */ + execute_callbacks(v, &to_purge); spin_unlock(&v->arch.hvm_vcpu.tm_lock); return -1; } @@ -290,6 +319,8 @@ int pt_update_irq(struct vcpu *v) irq = earliest_pt->irq; is_lapic = (earliest_pt->source == PTSRC_lapic); + execute_callbacks(v, &to_purge); + spin_unlock(&v->arch.hvm_vcpu.tm_lock); /* -- 2.16.3 _______________________________________________ Xen-devel mailing list Xen-devel@lists.xenproject.org https://lists.xenproject.org/mailman/listinfo/xen-devel ^ permalink raw reply related [flat|nested] 7+ messages in thread
* Re: [PATCH v2 1/2] x86/vpt: execute callbacks for masked interrupts 2018-03-30 12:35 ` [PATCH v2 1/2] x86/vpt: execute callbacks for masked interrupts Roger Pau Monne @ 2018-04-09 15:34 ` Jan Beulich 2018-04-10 8:53 ` Roger Pau Monné 0 siblings, 1 reply; 7+ messages in thread From: Jan Beulich @ 2018-04-09 15:34 UTC (permalink / raw) To: Roger Pau Monne; +Cc: Andrew Cooper, Stefan Bader, xen-devel >>> On 30.03.18 at 14:35, <roger.pau@citrix.com> wrote: > Execute periodic_time callbacks even if the interrupt is not actually > injected because the IRQ is masked. > > Current callbacks from emulated timer devices only update emulated > registers, which from my reading of the specs should happen regardless > of whether the interrupt has been injected or not. While generally I agree, it also means extra work done. Looking at the PIT case, for example, there's no strict need to do the update when the IRQ is masked, as the value being updated is only used to subtract from get_guest_time()'s return value. Similarly for the LAPIC case. In the RTC case your change actually looks risky, due to the pt_dead_ticks logic. I can't help getting the impression that the IRQ being off for 10 ticks would lead to no RTC interrupts at all anymore for the guest (until something resets that counter), which seems wrong to me. > @@ -282,6 +305,12 @@ int pt_update_irq(struct vcpu *v) > > if ( earliest_pt == NULL ) > { > + /* > + * NB: although the to_purge list is local, calls to > + * destroy_periodic_time can still remove items from the list, hence pt_adjust_vcpu() as well as it looks. > + * the need to hold the lock while accessing it. > + */ > + execute_callbacks(v, &to_purge); > spin_unlock(&v->arch.hvm_vcpu.tm_lock); > return -1; > } > @@ -290,6 +319,8 @@ int pt_update_irq(struct vcpu *v) > irq = earliest_pt->irq; > is_lapic = (earliest_pt->source == PTSRC_lapic); > > + execute_callbacks(v, &to_purge); > + > spin_unlock(&v->arch.hvm_vcpu.tm_lock); It seems to me that with your addition some code restructuring would actually be desirable, such that execute_callbacks() (and the lock release) would occur just once. Perhaps the mid-function return could be avoided altogether. Jan _______________________________________________ Xen-devel mailing list Xen-devel@lists.xenproject.org https://lists.xenproject.org/mailman/listinfo/xen-devel ^ permalink raw reply [flat|nested] 7+ messages in thread
* Re: [PATCH v2 1/2] x86/vpt: execute callbacks for masked interrupts 2018-04-09 15:34 ` Jan Beulich @ 2018-04-10 8:53 ` Roger Pau Monné 2018-04-10 9:33 ` Jan Beulich 0 siblings, 1 reply; 7+ messages in thread From: Roger Pau Monné @ 2018-04-10 8:53 UTC (permalink / raw) To: Jan Beulich; +Cc: Andrew Cooper, Stefan Bader, xen-devel On Mon, Apr 09, 2018 at 09:34:57AM -0600, Jan Beulich wrote: > >>> On 30.03.18 at 14:35, <roger.pau@citrix.com> wrote: > > Execute periodic_time callbacks even if the interrupt is not actually > > injected because the IRQ is masked. > > > > Current callbacks from emulated timer devices only update emulated > > registers, which from my reading of the specs should happen regardless > > of whether the interrupt has been injected or not. > > While generally I agree, it also means extra work done. Looking > at the PIT case, for example, there's no strict need to do the > update when the IRQ is masked, as the value being updated is > only used to subtract from get_guest_time()'s return value. > Similarly for the LAPIC case. > > In the RTC case your change actually looks risky, due to the > pt_dead_ticks logic. I can't help getting the impression that the > IRQ being off for 10 ticks would lead to no RTC interrupts at all > anymore for the guest (until something resets that counter), > which seems wrong to me. Hm, right. The RTC is already handled specially in order to not disable the timer but also don't call the handler if the IRQ is masked. Maybe the right solution is to add some flags to the vpt code, something like: - DISABLE_ON_MASKED: only valid for periodic interrupts. Destroy the timer if the IRQ is masked when the timer fires. - SKIP_CALLBACK_ON_MASKED: do not execute the timer callback if the IRQ is masked when the timer fires. That AFAICT should allow Xen to keep the previous behaviour for existing timer code (and remove the RTC special casing). > > @@ -282,6 +305,12 @@ int pt_update_irq(struct vcpu *v) > > > > if ( earliest_pt == NULL ) > > { > > + /* > > + * NB: although the to_purge list is local, calls to > > + * destroy_periodic_time can still remove items from the list, hence > > pt_adjust_vcpu() as well as it looks. > > > + * the need to hold the lock while accessing it. > > + */ > > + execute_callbacks(v, &to_purge); > > spin_unlock(&v->arch.hvm_vcpu.tm_lock); > > return -1; > > } > > @@ -290,6 +319,8 @@ int pt_update_irq(struct vcpu *v) > > irq = earliest_pt->irq; > > is_lapic = (earliest_pt->source == PTSRC_lapic); > > > > + execute_callbacks(v, &to_purge); > > + > > spin_unlock(&v->arch.hvm_vcpu.tm_lock); > > It seems to me that with your addition some code restructuring > would actually be desirable, such that execute_callbacks() (and > the lock release) would occur just once. Perhaps the mid-function > return could be avoided altogether. OK, I can do that. Let's first agree on the interface though. Thanks, Roger. _______________________________________________ Xen-devel mailing list Xen-devel@lists.xenproject.org https://lists.xenproject.org/mailman/listinfo/xen-devel ^ permalink raw reply [flat|nested] 7+ messages in thread
* Re: [PATCH v2 1/2] x86/vpt: execute callbacks for masked interrupts 2018-04-10 8:53 ` Roger Pau Monné @ 2018-04-10 9:33 ` Jan Beulich 0 siblings, 0 replies; 7+ messages in thread From: Jan Beulich @ 2018-04-10 9:33 UTC (permalink / raw) To: Roger Pau Monné; +Cc: Andrew Cooper, Stefan Bader, xen-devel >>> On 10.04.18 at 10:53, <roger.pau@citrix.com> wrote: > On Mon, Apr 09, 2018 at 09:34:57AM -0600, Jan Beulich wrote: >> >>> On 30.03.18 at 14:35, <roger.pau@citrix.com> wrote: >> > Execute periodic_time callbacks even if the interrupt is not actually >> > injected because the IRQ is masked. >> > >> > Current callbacks from emulated timer devices only update emulated >> > registers, which from my reading of the specs should happen regardless >> > of whether the interrupt has been injected or not. >> >> While generally I agree, it also means extra work done. Looking >> at the PIT case, for example, there's no strict need to do the >> update when the IRQ is masked, as the value being updated is >> only used to subtract from get_guest_time()'s return value. >> Similarly for the LAPIC case. >> >> In the RTC case your change actually looks risky, due to the >> pt_dead_ticks logic. I can't help getting the impression that the >> IRQ being off for 10 ticks would lead to no RTC interrupts at all >> anymore for the guest (until something resets that counter), >> which seems wrong to me. > > Hm, right. The RTC is already handled specially in order to not > disable the timer but also don't call the handler if the IRQ is > masked. > > Maybe the right solution is to add some flags to the vpt code, > something like: > > - DISABLE_ON_MASKED: only valid for periodic interrupts. Destroy the > timer if the IRQ is masked when the timer fires. > - SKIP_CALLBACK_ON_MASKED: do not execute the timer callback if the > IRQ is masked when the timer fires. > > That AFAICT should allow Xen to keep the previous behaviour for > existing timer code (and remove the RTC special casing). Something like this, yes (I don't really like the names you suggest, but I also can't suggest any better ones right away). Jan _______________________________________________ Xen-devel mailing list Xen-devel@lists.xenproject.org https://lists.xenproject.org/mailman/listinfo/xen-devel ^ permalink raw reply [flat|nested] 7+ messages in thread
* [PATCH v2 2/2] vhpet: add support for level triggered interrupts 2018-03-30 12:35 [PATCH v2 0/2] hpet: add support for level triggered interrupts Roger Pau Monne 2018-03-30 12:35 ` [PATCH v2 1/2] x86/vpt: execute callbacks for masked interrupts Roger Pau Monne @ 2018-03-30 12:36 ` Roger Pau Monne 2018-04-09 15:51 ` Jan Beulich 1 sibling, 1 reply; 7+ messages in thread From: Roger Pau Monne @ 2018-03-30 12:36 UTC (permalink / raw) To: xen-devel; +Cc: Andrew Cooper, Stefan Bader, Jan Beulich, Roger Pau Monne Level triggered interrupts are not an optional feature of HPET, and must be implemented in order to comply with the HPET specification. Implement them by adding a callback to the timer which sets the interrupt bit in the general interrupt status register. Further interrupts (in case of periodic mode) will not be injected until the bit is cleared. In order to reset the interrupts when the status bit is clear Xen must also detect accesses to such register. While there convert tn and i in hpet_write to unsigned. Reported-by: Stefan Bader <stefan.bader@canonical.com> Signed-off-by: Roger Pau Monné <roger.pau@citrix.com> --- Cc: Jan Beulich <jbeulich@suse.com> Cc: Andrew Cooper <andrew.cooper3@citrix.com> Cc: Stefan Bader <stefan.bader@canonical.com> --- xen/arch/x86/hvm/hpet.c | 46 ++++++++++++++++++++++++++++++++++------------ 1 file changed, 34 insertions(+), 12 deletions(-) diff --git a/xen/arch/x86/hvm/hpet.c b/xen/arch/x86/hvm/hpet.c index f7aed7f69e..1cfd72592e 100644 --- a/xen/arch/x86/hvm/hpet.c +++ b/xen/arch/x86/hvm/hpet.c @@ -220,6 +220,17 @@ static void hpet_stop_timer(HPETState *h, unsigned int tn, hpet_get_comparator(h, tn, guest_time); } +static void hpet_timer_fired(struct vcpu *v, void *data) +{ + unsigned int tn = (unsigned int)data; + HPETState *h = vcpu_vhpet(v); + + write_lock(&h->lock); + ASSERT(!test_bit(tn, &h->hpet.isr)); + __set_bit(tn, &h->hpet.isr); + write_unlock(&h->lock); +} + /* the number of HPET tick that stands for * 1/(2^10) second, namely, 0.9765625 milliseconds */ #define HPET_TINY_TIME_SPAN ((h->stime_freq >> 10) / STIME_PER_HPET_TICK) @@ -241,7 +252,7 @@ static void hpet_set_timer(HPETState *h, unsigned int tn, pit_stop_channel0_irq(&vhpet_domain(h)->arch.vpit); } - if ( !timer_enabled(h, tn) ) + if ( !timer_enabled(h, tn) || test_bit(tn, &h->hpet.isr) ) return; tn_cmp = hpet_get_comparator(h, tn, guest_time); @@ -277,8 +288,12 @@ static void hpet_set_timer(HPETState *h, unsigned int tn, * timer we also need the period which may be different because time may * have elapsed between the time the comparator was written and the timer * being enabled (now). + * + * NB: set periodic timers as oneshot if interrupt type is set to level + * because the user must ack the interrupt (by writing 1 to the interrupt + * status register) before another interrupt can be delivered. */ - oneshot = !timer_is_periodic(h, tn); + oneshot = !timer_is_periodic(h, tn) || timer_level(h, tn); TRACE_2_LONG_4D(TRC_HVM_EMUL_HPET_START_TIMER, tn, irq, TRC_PAR_LONG(hpet_tick_to_ns(h, diff)), TRC_PAR_LONG(oneshot ? 0LL : @@ -286,7 +301,9 @@ static void hpet_set_timer(HPETState *h, unsigned int tn, create_periodic_time(vhpet_vcpu(h), &h->pt[tn], hpet_tick_to_ns(h, diff), oneshot ? 0 : hpet_tick_to_ns(h, h->hpet.period[tn]), - irq, NULL, NULL); + irq, + timer_level(h, tn) ? hpet_timer_fired : NULL, + (void *)(unsigned long)tn); } static inline uint64_t hpet_fixup_reg( @@ -304,7 +321,7 @@ static int hpet_write( HPETState *h = vcpu_vhpet(v); uint64_t old_val, new_val; uint64_t guest_time; - int tn, i; + unsigned int tn, i; /* Acculumate a bit mask of timers whos state is changed by this write. */ unsigned long start_timers = 0; @@ -360,6 +377,19 @@ static int hpet_write( } break; + case HPET_STATUS: + /* write 1 to clear. */ + while (new_val) + { + i = find_first_set_bit(new_val); + if ( i >= HPET_TIMER_NUM ) + break; + __clear_bit(i, &new_val); + if ( __test_and_clear_bit(i, &h->hpet.isr) ) + set_start_timer(i); + } + break; + case HPET_COUNTER: h->hpet.mc64 = new_val; if ( hpet_enabled(h) ) @@ -379,14 +409,6 @@ static int hpet_write( h->hpet.timers[tn].config = hpet_fixup_reg(new_val, old_val, 0x3f4e); - if ( timer_level(h, tn) ) - { - gdprintk(XENLOG_ERR, - "HPET: level triggered interrupt not supported now\n"); - domain_crash(current->domain); - break; - } - if ( new_val & HPET_TN_32BIT ) { h->hpet.timers[tn].cmp = (uint32_t)h->hpet.timers[tn].cmp; -- 2.16.3 _______________________________________________ Xen-devel mailing list Xen-devel@lists.xenproject.org https://lists.xenproject.org/mailman/listinfo/xen-devel ^ permalink raw reply related [flat|nested] 7+ messages in thread
* Re: [PATCH v2 2/2] vhpet: add support for level triggered interrupts 2018-03-30 12:36 ` [PATCH v2 2/2] vhpet: add support for level triggered interrupts Roger Pau Monne @ 2018-04-09 15:51 ` Jan Beulich 0 siblings, 0 replies; 7+ messages in thread From: Jan Beulich @ 2018-04-09 15:51 UTC (permalink / raw) To: Roger Pau Monne; +Cc: Andrew Cooper, Stefan Bader, xen-devel >>> On 30.03.18 at 14:36, <roger.pau@citrix.com> wrote: > Level triggered interrupts are not an optional feature of HPET, and > must be implemented in order to comply with the HPET specification. > > Implement them by adding a callback to the timer which sets the > interrupt bit in the general interrupt status register. Further > interrupts (in case of periodic mode) will not be injected until the > bit is cleared. > > In order to reset the interrupts when the status bit is clear Xen must > also detect accesses to such register. s/detect/handle/ and s/such/the status/ ? > --- a/xen/arch/x86/hvm/hpet.c > +++ b/xen/arch/x86/hvm/hpet.c > @@ -220,6 +220,17 @@ static void hpet_stop_timer(HPETState *h, unsigned int tn, > hpet_get_comparator(h, tn, guest_time); > } > > +static void hpet_timer_fired(struct vcpu *v, void *data) No need for the hpet_ prefix when the function is static. > +{ > + unsigned int tn = (unsigned int)data; Most gcc versions I'm aware of will warn about casts between pointers and integers of different size - the cast needs to be to unsigned long (while the variable type can remain as is). Oddly enough you go through unsigned long when casting in the other direction. > @@ -360,6 +377,19 @@ static int hpet_write( > } > break; > > + case HPET_STATUS: > + /* write 1 to clear. */ > + while (new_val) Missing blanks. Jan _______________________________________________ Xen-devel mailing list Xen-devel@lists.xenproject.org https://lists.xenproject.org/mailman/listinfo/xen-devel ^ permalink raw reply [flat|nested] 7+ messages in thread
end of thread, other threads:[~2018-04-10 9:33 UTC | newest] Thread overview: 7+ messages (download: mbox.gz / follow: Atom feed) -- links below jump to the message on this page -- 2018-03-30 12:35 [PATCH v2 0/2] hpet: add support for level triggered interrupts Roger Pau Monne 2018-03-30 12:35 ` [PATCH v2 1/2] x86/vpt: execute callbacks for masked interrupts Roger Pau Monne 2018-04-09 15:34 ` Jan Beulich 2018-04-10 8:53 ` Roger Pau Monné 2018-04-10 9:33 ` Jan Beulich 2018-03-30 12:36 ` [PATCH v2 2/2] vhpet: add support for level triggered interrupts Roger Pau Monne 2018-04-09 15:51 ` Jan Beulich
This is an external index of several public inboxes, see mirroring instructions on how to clone and mirror all data and code used by this external index.