[PATCH v3 0/6] vhpet: add support for level triggered interrupts

[PATCH v3 0/6] vhpet: add support for level triggered interrupts

[Roger Pau Monne]

Hello,

The following series contain a couple of bug fixes for the vhpet/vpt
code, and also add support for level trigger interrupts to vhpet. Level
triggered interrupts are not an optional feature of the hpet spec, so
they must be implemented in order to have a complete emulated hpet
implementation in Xen.

Last patch adds some basic testing of hpet (mainly the level triggered
interrupts) to xtf.

Thanks, Roger.

Roger Pau Monne (6):
  vpt: fix create_periodic_time to use the irq parameter
  vhpet: check that the set interrupt route is valid
  vpt: convert periodic_time fields to bool
  vpt: split part of pt_intr_post into a separate helper
  vpt: add support for level interrupts
  vhpet: add support for level triggered interrupts

 xen/arch/x86/hvm/hpet.c       |  87 +++++++++++++++++++++----
 xen/arch/x86/hvm/i8254.c      |   4 +-
 xen/arch/x86/hvm/irq.c        |  15 +++++
 xen/arch/x86/hvm/rtc.c        |   2 +-
 xen/arch/x86/hvm/vlapic.c     |   8 +--
 xen/arch/x86/hvm/vpt.c        | 116 ++++++++++++++++++++++------------
 xen/include/asm-x86/hvm/irq.h |   3 +-
 xen/include/asm-x86/hvm/vpt.h |  13 ++--
 8 files changed, 182 insertions(+), 66 deletions(-)

-- 
2.17.1


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

[PATCH v3 1/6] vpt: fix create_periodic_time to use the irq parameter

[Roger Pau Monne]

Instead of the stale value inside the periodic_time struct.

Signed-off-by: Roger Pau Monné <roger.pau@citrix.com>
---
Cc: Jan Beulich <jbeulich@suse.com>
Cc: Andrew Cooper <andrew.cooper3@citrix.com>
---
 xen/arch/x86/hvm/vpt.c | 4 ++--
 1 file changed, 2 insertions(+), 2 deletions(-)

diff --git a/xen/arch/x86/hvm/vpt.c b/xen/arch/x86/hvm/vpt.c
index d5363caec7..a0cc61fd28 100644
--- a/xen/arch/x86/hvm/vpt.c
+++ b/xen/arch/x86/hvm/vpt.c
@@ -445,8 +445,8 @@ void create_periodic_time(
     uint64_t period, uint8_t irq, time_cb *cb, void *data)
 {
     if ( !pt->source ||
-         (pt->irq >= NR_ISAIRQS && pt->source == PTSRC_isa) ||
-         (pt->irq >= hvm_domain_irq(v->domain)->nr_gsis &&
+         (irq >= NR_ISAIRQS && pt->source == PTSRC_isa) ||
+         (irq >= hvm_domain_irq(v->domain)->nr_gsis &&
           pt->source == PTSRC_ioapic) )
     {
         ASSERT_UNREACHABLE();
-- 
2.17.1


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

[PATCH v3 2/6] vhpet: check that the set interrupt route is valid

[Roger Pau Monne]

The value written by the guest must be valid according to the mask
provided in the interrupt routing capabilities register. If the
interrupt is not valid set it to the first valid IRQ in the
capabilities field if the timer is enabled, else just clear the field.

Also refuse to start any timer that has an invalid interrupt route.

Signed-off-by: Roger Pau Monné <roger.pau@citrix.com>
---
Cc: Jan Beulich <jbeulich@suse.com>
Cc: Andrew Cooper <andrew.cooper3@citrix.com>
---
Changes since v1:
 - Use int_route instead of plain int to avoid confusion with integer.
 - Use find_first_bit instead of ffs.
 - Use timer_config.
---
 xen/arch/x86/hvm/hpet.c | 30 ++++++++++++++++++++++++++++++
 1 file changed, 30 insertions(+)

diff --git a/xen/arch/x86/hvm/hpet.c b/xen/arch/x86/hvm/hpet.c
index 28377091ca..f7ef4f7514 100644
--- a/xen/arch/x86/hvm/hpet.c
+++ b/xen/arch/x86/hvm/hpet.c
@@ -73,6 +73,9 @@
     ((timer_config(h, n) & HPET_TN_INT_ROUTE_CAP_MASK) \
         >> HPET_TN_INT_ROUTE_CAP_SHIFT)
 
+#define timer_int_route_valid(h, n) \
+    ((1u << timer_int_route(h, n)) & timer_int_route_cap(h, n))
+
 static inline uint64_t hpet_read_maincounter(HPETState *h, uint64_t guest_time)
 {
     ASSERT(rw_is_locked(&h->lock));
@@ -244,6 +247,12 @@ static void hpet_set_timer(HPETState *h, unsigned int tn,
     if ( !timer_enabled(h, tn) )
         return;
 
+    if ( !timer_int_route_valid(h, tn) )
+    {
+        ASSERT_UNREACHABLE();
+        return;
+    }
+
     tn_cmp   = hpet_get_comparator(h, tn, guest_time);
     cur_tick = hpet_read_maincounter(h, guest_time);
     if ( timer_is_32bit(h, tn) )
@@ -304,6 +313,24 @@ static inline uint64_t hpet_fixup_reg(
     return new;
 }
 
+static void timer_sanitize_int_route(HPETState *h, unsigned int tn)
+{
+    if ( timer_int_route_valid(h, tn) )
+        return;
+
+    timer_config(h, tn) &= ~HPET_TN_ROUTE;
+    if ( !timer_enabled(h, tn) )
+        return;
+
+    /*
+     * If the requested interrupt is not valid and the timer is
+     * enabled pick the first irq.
+     */
+    timer_config(h, tn) |=
+        MASK_INSR(find_first_set_bit(timer_int_route_cap(h, tn)),
+                  HPET_TN_ROUTE);
+}
+
 static int hpet_write(
     struct vcpu *v, unsigned long addr,
     unsigned int length, unsigned long val)
@@ -386,6 +413,8 @@ static int hpet_write(
 
         h->hpet.timers[tn].config = hpet_fixup_reg(new_val, old_val, 0x3f4e);
 
+        timer_sanitize_int_route(h, tn);
+
         if ( timer_level(h, tn) )
         {
             gdprintk(XENLOG_ERR,
@@ -621,6 +650,7 @@ static int hpet_load(struct domain *d, hvm_domain_context_t *h)
         if ( timer_is_32bit(hp, i) )
             cmp = (uint32_t)cmp;
         hp->hpet.timers[i].cmp = cmp;
+        timer_sanitize_int_route(hp, i);
     }
 #undef C
 
-- 
2.17.1


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

[PATCH v3 3/6] vpt: convert periodic_time fields to bool

[Roger Pau Monne]

No functional change.

Signed-off-by: Roger Pau Monné <roger.pau@citrix.com>
---
Cc: Jan Beulich <jbeulich@suse.com>
Cc: Andrew Cooper <andrew.cooper3@citrix.com>
---
 xen/include/asm-x86/hvm/vpt.h | 10 +++++-----
 1 file changed, 5 insertions(+), 5 deletions(-)

diff --git a/xen/include/asm-x86/hvm/vpt.h b/xen/include/asm-x86/hvm/vpt.h
index 0eb5ff632e..f693c0bcf1 100644
--- a/xen/include/asm-x86/hvm/vpt.h
+++ b/xen/include/asm-x86/hvm/vpt.h
@@ -37,11 +37,11 @@ typedef void time_cb(struct vcpu *v, void *opaque);
 
 struct periodic_time {
     struct list_head list;
-    bool_t on_list;
-    bool_t one_shot;
-    bool_t do_not_freeze;
-    bool_t irq_issued;
-    bool_t warned_timeout_too_short;
+    bool on_list;
+    bool one_shot;
+    bool do_not_freeze;
+    bool irq_issued;
+    bool warned_timeout_too_short;
 #define PTSRC_isa    1 /* ISA time source */
 #define PTSRC_lapic  2 /* LAPIC time source */
 #define PTSRC_ioapic 3 /* IOAPIC time source */
-- 
2.17.1


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

[PATCH v3 4/6] vpt: split part of pt_intr_post into a separate helper

[Roger Pau Monne]

No functional change.

Signed-off-by: Roger Pau Monné <roger.pau@citrix.com>
---
Cc: Jan Beulich <jbeulich@suse.com>
Cc: Andrew Cooper <andrew.cooper3@citrix.com>
---
 xen/arch/x86/hvm/vpt.c | 67 +++++++++++++++++++++++-------------------
 1 file changed, 36 insertions(+), 31 deletions(-)

diff --git a/xen/arch/x86/hvm/vpt.c b/xen/arch/x86/hvm/vpt.c
index a0cc61fd28..2565f7237e 100644
--- a/xen/arch/x86/hvm/vpt.c
+++ b/xen/arch/x86/hvm/vpt.c
@@ -265,6 +265,41 @@ static void pt_timer_fn(void *data)
     pt_unlock(pt);
 }
 
+static void pt_irq_fired(struct vcpu *v, struct periodic_time *pt)
+{
+    pt->irq_issued = 0;
+
+    if ( pt->one_shot )
+    {
+        if ( pt->on_list )
+            list_del(&pt->list);
+        pt->on_list = 0;
+        pt->pending_intr_nr = 0;
+    }
+    else if ( mode_is(v->domain, one_missed_tick_pending) ||
+              mode_is(v->domain, no_missed_ticks_pending) )
+    {
+        pt->last_plt_gtime = hvm_get_guest_time(v);
+        pt_process_missed_ticks(pt);
+        pt->pending_intr_nr = 0; /* 'collapse' all missed ticks */
+        set_timer(&pt->timer, pt->scheduled);
+    }
+    else
+    {
+        pt->last_plt_gtime += pt->period;
+        if ( --pt->pending_intr_nr == 0 )
+        {
+            pt_process_missed_ticks(pt);
+            if ( pt->pending_intr_nr == 0 )
+                set_timer(&pt->timer, pt->scheduled);
+        }
+    }
+
+    if ( mode_is(v->domain, delay_for_missed_ticks) &&
+         (hvm_get_guest_time(v) < pt->last_plt_gtime) )
+        hvm_set_guest_time(v, pt->last_plt_gtime);
+}
+
 int pt_update_irq(struct vcpu *v)
 {
     struct list_head *head = &v->arch.hvm_vcpu.tm_list;
@@ -386,37 +421,7 @@ void pt_intr_post(struct vcpu *v, struct hvm_intack intack)
         return;
     }
 
-    pt->irq_issued = 0;
-
-    if ( pt->one_shot )
-    {
-        if ( pt->on_list )
-            list_del(&pt->list);
-        pt->on_list = 0;
-        pt->pending_intr_nr = 0;
-    }
-    else if ( mode_is(v->domain, one_missed_tick_pending) ||
-              mode_is(v->domain, no_missed_ticks_pending) )
-    {
-        pt->last_plt_gtime = hvm_get_guest_time(v);
-        pt_process_missed_ticks(pt);
-        pt->pending_intr_nr = 0; /* 'collapse' all missed ticks */
-        set_timer(&pt->timer, pt->scheduled);
-    }
-    else
-    {
-        pt->last_plt_gtime += pt->period;
-        if ( --pt->pending_intr_nr == 0 )
-        {
-            pt_process_missed_ticks(pt);
-            if ( pt->pending_intr_nr == 0 )
-                set_timer(&pt->timer, pt->scheduled);
-        }
-    }
-
-    if ( mode_is(v->domain, delay_for_missed_ticks) &&
-         (hvm_get_guest_time(v) < pt->last_plt_gtime) )
-        hvm_set_guest_time(v, pt->last_plt_gtime);
+    pt_irq_fired(v, pt);
 
     cb = pt->cb;
     cb_priv = pt->priv;
-- 
2.17.1


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

[PATCH v3 5/6] vpt: add support for level interrupts

[Roger Pau Monne]

Level trigger interrupts will be asserted regardless of whether the
interrupt is masked, and thus the callback will also be executed.

Add a new 'level' parameter to create_periodic_time in order to create
level triggered timers.

Note that none of the current users of vpt are switched to use level
triggered interrupts yet.

Signed-off-by: Roger Pau Monné <roger.pau@citrix.com>
---
Cc: Jan Beulich <jbeulich@suse.com>
Cc: Andrew Cooper <andrew.cooper3@citrix.com>
---
 xen/arch/x86/hvm/hpet.c       |  2 +-
 xen/arch/x86/hvm/i8254.c      |  4 ++--
 xen/arch/x86/hvm/rtc.c        |  2 +-
 xen/arch/x86/hvm/vlapic.c     |  8 +++----
 xen/arch/x86/hvm/vpt.c        | 45 ++++++++++++++++++++++++++++-------
 xen/include/asm-x86/hvm/vpt.h |  3 ++-
 6 files changed, 47 insertions(+), 17 deletions(-)

diff --git a/xen/arch/x86/hvm/hpet.c b/xen/arch/x86/hvm/hpet.c
index f7ef4f7514..72209350ba 100644
--- a/xen/arch/x86/hvm/hpet.c
+++ b/xen/arch/x86/hvm/hpet.c
@@ -302,7 +302,7 @@ 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, NULL, NULL, false);
 }
 
 static inline uint64_t hpet_fixup_reg(
diff --git a/xen/arch/x86/hvm/i8254.c b/xen/arch/x86/hvm/i8254.c
index 992f08dd6c..b8ec56f8d3 100644
--- a/xen/arch/x86/hvm/i8254.c
+++ b/xen/arch/x86/hvm/i8254.c
@@ -191,14 +191,14 @@ static void pit_load_count(PITState *pit, int channel, int val)
         /* Periodic timer. */
         TRACE_2D(TRC_HVM_EMUL_PIT_START_TIMER, period, period);
         create_periodic_time(v, &pit->pt0, period, period, 0, pit_time_fired, 
-                             &pit->count_load_time[channel]);
+                             &pit->count_load_time[channel], false);
         break;
     case 1:
     case 4:
         /* One-shot timer. */
         TRACE_2D(TRC_HVM_EMUL_PIT_START_TIMER, period, 0);
         create_periodic_time(v, &pit->pt0, period, 0, 0, pit_time_fired,
-                             &pit->count_load_time[channel]);
+                             &pit->count_load_time[channel], false);
         break;
     default:
         TRACE_0D(TRC_HVM_EMUL_PIT_STOP_TIMER);
diff --git a/xen/arch/x86/hvm/rtc.c b/xen/arch/x86/hvm/rtc.c
index cb75b99ed1..96921bb5b5 100644
--- a/xen/arch/x86/hvm/rtc.c
+++ b/xen/arch/x86/hvm/rtc.c
@@ -156,7 +156,7 @@ static void rtc_timer_update(RTCState *s)
                 {
                     TRACE_2D(TRC_HVM_EMUL_RTC_START_TIMER, delta, period);
                     create_periodic_time(v, &s->pt, delta, period,
-                                         RTC_IRQ, rtc_pf_callback, s);
+                                         RTC_IRQ, rtc_pf_callback, s, false);
                 }
                 else
                     s->check_ticks_since = now;
diff --git a/xen/arch/x86/hvm/vlapic.c b/xen/arch/x86/hvm/vlapic.c
index 1b9f00a0e4..d2ac4b8625 100644
--- a/xen/arch/x86/hvm/vlapic.c
+++ b/xen/arch/x86/hvm/vlapic.c
@@ -762,7 +762,7 @@ static void vlapic_update_timer(struct vlapic *vlapic, uint32_t lvtt,
         create_periodic_time(current, &vlapic->pt, delta,
                              is_periodic ? period : 0, vlapic->pt.irq,
                              is_periodic ? vlapic_pt_cb : NULL,
-                             &vlapic->timer_last_update);
+                             &vlapic->timer_last_update, false);
 
         vlapic->timer_last_update = vlapic->pt.last_plt_gtime;
         if ( !tmict_updated )
@@ -1166,7 +1166,7 @@ void vlapic_tdt_msr_set(struct vlapic *vlapic, uint64_t value)
                         TRC_PAR_LONG(0LL), vlapic->pt.irq);
         create_periodic_time(v, &vlapic->pt, delta, 0,
                              vlapic->pt.irq, vlapic_tdt_pt_cb,
-                             &vlapic->timer_last_update);
+                             &vlapic->timer_last_update, false);
         vlapic->timer_last_update = vlapic->pt.last_plt_gtime;
     }
     else
@@ -1180,7 +1180,7 @@ void vlapic_tdt_msr_set(struct vlapic *vlapic, uint64_t value)
                             TRC_PAR_LONG(0LL), vlapic->pt.irq);
             create_periodic_time(v, &vlapic->pt, 0, 0,
                                  vlapic->pt.irq, vlapic_tdt_pt_cb,
-                                 &vlapic->timer_last_update);
+                                 &vlapic->timer_last_update, false);
             vlapic->timer_last_update = vlapic->pt.last_plt_gtime;
         }
         else
@@ -1431,7 +1431,7 @@ static void lapic_rearm(struct vlapic *s)
                          vlapic_lvtt_period(s) ? period : 0,
                          s->pt.irq,
                          vlapic_lvtt_period(s) ? vlapic_pt_cb : NULL,
-                         &s->timer_last_update);
+                         &s->timer_last_update, false);
     s->timer_last_update = s->pt.last_plt_gtime;
 }
 
diff --git a/xen/arch/x86/hvm/vpt.c b/xen/arch/x86/hvm/vpt.c
index 2565f7237e..e98bc41b1c 100644
--- a/xen/arch/x86/hvm/vpt.c
+++ b/xen/arch/x86/hvm/vpt.c
@@ -306,6 +306,7 @@ int pt_update_irq(struct vcpu *v)
     struct periodic_time *pt, *temp, *earliest_pt;
     uint64_t max_lag;
     int irq, pt_vector = -1;
+    bool level;
 
     spin_lock(&v->arch.hvm_vcpu.tm_lock);
 
@@ -316,7 +317,9 @@ int pt_update_irq(struct vcpu *v)
         if ( pt->pending_intr_nr )
         {
             /* RTC code takes care of disabling the timer itself. */
-            if ( (pt->irq != RTC_IRQ || !pt->priv) && pt_irq_masked(pt) )
+            if ( (pt->irq != RTC_IRQ || !pt->priv) && pt_irq_masked(pt) &&
+                 /* Level interrupts should be asserted even if masked. */
+                 !pt->level )
             {
                 /* suspend timer emulation */
                 list_del(&pt->list);
@@ -341,6 +344,7 @@ int pt_update_irq(struct vcpu *v)
 
     earliest_pt->irq_issued = 1;
     irq = earliest_pt->irq;
+    level = earliest_pt->level;
 
     spin_unlock(&v->arch.hvm_vcpu.tm_lock);
 
@@ -374,13 +378,36 @@ int pt_update_irq(struct vcpu *v)
         break;
 
     case PTSRC_ioapic:
-        /*
-         * NB: At the moment IO-APIC routed interrupts generated by vpt devices
-         * (HPET) are edge-triggered.
-         */
-        pt_vector = hvm_ioapic_assert(v->domain, irq, false);
+        pt_vector = hvm_ioapic_assert(v->domain, irq, level);
         if ( pt_vector < 0 || !vlapic_test_irq(vcpu_vlapic(v), pt_vector) )
+        {
             pt_vector = -1;
+            if ( level )
+            {
+                /*
+                 * Level interrupts are asserted even if the interrupt is
+                 * masked, so also execute the callback associated with the
+                 * timer.
+                 */
+                time_cb *cb = NULL;
+                void *cb_priv;
+
+                spin_lock(&v->arch.hvm_vcpu.tm_lock);
+                /* Make sure the timer is still on the list. */
+                list_for_each_entry ( pt, &v->arch.hvm_vcpu.tm_list, list )
+                    if ( pt == earliest_pt )
+                    {
+                        pt_irq_fired(v, pt);
+                        cb = pt->cb;
+                        cb_priv = pt->priv;
+                        break;
+                    }
+                spin_unlock(&v->arch.hvm_vcpu.tm_lock);
+
+                if ( cb != NULL )
+                    cb(v, cb_priv);
+            }
+        }
         break;
     }
 
@@ -447,12 +474,13 @@ void pt_migrate(struct vcpu *v)
 
 void create_periodic_time(
     struct vcpu *v, struct periodic_time *pt, uint64_t delta,
-    uint64_t period, uint8_t irq, time_cb *cb, void *data)
+    uint64_t period, uint8_t irq, time_cb *cb, void *data, bool level)
 {
     if ( !pt->source ||
          (irq >= NR_ISAIRQS && pt->source == PTSRC_isa) ||
          (irq >= hvm_domain_irq(v->domain)->nr_gsis &&
-          pt->source == PTSRC_ioapic) )
+          pt->source == PTSRC_ioapic) ||
+         (level && pt->source != PTSRC_ioapic) )
     {
         ASSERT_UNREACHABLE();
         return;
@@ -480,6 +508,7 @@ void create_periodic_time(
     pt->last_plt_gtime = hvm_get_guest_time(pt->vcpu);
     pt->irq = irq;
     pt->one_shot = !period;
+    pt->level = level;
     pt->scheduled = NOW() + delta;
 
     if ( !pt->one_shot )
diff --git a/xen/include/asm-x86/hvm/vpt.h b/xen/include/asm-x86/hvm/vpt.h
index f693c0bcf1..61c26ed8b2 100644
--- a/xen/include/asm-x86/hvm/vpt.h
+++ b/xen/include/asm-x86/hvm/vpt.h
@@ -42,6 +42,7 @@ struct periodic_time {
     bool do_not_freeze;
     bool irq_issued;
     bool warned_timeout_too_short;
+    bool level;
 #define PTSRC_isa    1 /* ISA time source */
 #define PTSRC_lapic  2 /* LAPIC time source */
 #define PTSRC_ioapic 3 /* IOAPIC time source */
@@ -169,7 +170,7 @@ void pt_may_unmask_irq(struct domain *d, struct periodic_time *vlapic_pt);
  */
 void create_periodic_time(
     struct vcpu *v, struct periodic_time *pt, uint64_t delta,
-    uint64_t period, uint8_t irq, time_cb *cb, void *data);
+    uint64_t period, uint8_t irq, time_cb *cb, void *data, bool level);
 void destroy_periodic_time(struct periodic_time *pt);
 
 int pv_pit_handler(int port, int data, int write);
-- 
2.17.1


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

[PATCH v3 6/6] vhpet: add support for level triggered interrupts

[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       | 59 ++++++++++++++++++++++++++++-------
 xen/arch/x86/hvm/irq.c        | 15 +++++++++
 xen/include/asm-x86/hvm/irq.h |  3 +-
 3 files changed, 64 insertions(+), 13 deletions(-)

diff --git a/xen/arch/x86/hvm/hpet.c b/xen/arch/x86/hvm/hpet.c
index 72209350ba..9720efb5c6 100644
--- a/xen/arch/x86/hvm/hpet.c
+++ b/xen/arch/x86/hvm/hpet.c
@@ -223,6 +223,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)
@@ -244,7 +255,8 @@ 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) ||
+         (timer_level(h, tn) && test_bit(tn, &h->hpet.isr)) )
         return;
 
     if ( !timer_int_route_valid(h, tn) )
@@ -293,8 +305,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 :
@@ -302,7 +318,8 @@ 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, false);
+                         irq, timer_level(h, tn) ? hpet_timer_fired : NULL,
+                         (void *)(unsigned long)tn, timer_level(h, tn));
 }
 
 static inline uint64_t hpet_fixup_reg(
@@ -338,7 +355,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;
@@ -394,6 +411,32 @@ static int hpet_write(
         }
         break;
 
+    case HPET_STATUS:
+        /* write 1 to clear. */
+        while ( new_val )
+        {
+            bool active;
+
+            i = find_first_set_bit(new_val);
+            if ( i >= HPET_TIMER_NUM )
+                break;
+            __clear_bit(i, &new_val);
+            active = __test_and_clear_bit(i, &h->hpet.isr);
+            if ( active )
+            {
+                /*
+                 * Should pt->irq better be used here in case the guest changes
+                 * the configured IRQ while it's active? Guest changing the IRQ
+                 * while the interrupt is active is not documented.
+                 */
+                hvm_ioapic_deassert(v->domain, timer_int_route(h, i));
+                if ( hpet_enabled(h) && timer_enabled(h, i) &&
+                     timer_level(h, i) && timer_is_periodic(h, i) )
+                    set_start_timer(i);
+            }
+        }
+        break;
+
     case HPET_COUNTER:
         h->hpet.mc64 = new_val;
         if ( hpet_enabled(h) )
@@ -415,14 +458,6 @@ static int hpet_write(
 
         timer_sanitize_int_route(h, tn);
 
-        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;
diff --git a/xen/arch/x86/hvm/irq.c b/xen/arch/x86/hvm/irq.c
index c85d004402..8095c829b6 100644
--- a/xen/arch/x86/hvm/irq.c
+++ b/xen/arch/x86/hvm/irq.c
@@ -61,6 +61,21 @@ int hvm_ioapic_assert(struct domain *d, unsigned int gsi, bool level)
     return vector;
 }
 
+void hvm_ioapic_deassert(struct domain *d, unsigned int gsi)
+{
+    struct hvm_irq *hvm_irq = hvm_domain_irq(d);
+
+    if ( gsi >= hvm_irq->nr_gsis )
+    {
+        ASSERT_UNREACHABLE();
+        return;
+    }
+
+    spin_lock(&d->arch.hvm_domain.irq_lock);
+    hvm_irq->gsi_assert_count[gsi]--;
+    spin_unlock(&d->arch.hvm_domain.irq_lock);
+}
+
 static void assert_irq(struct domain *d, unsigned ioapic_gsi, unsigned pic_irq)
 {
     assert_gsi(d, ioapic_gsi);
diff --git a/xen/include/asm-x86/hvm/irq.h b/xen/include/asm-x86/hvm/irq.h
index 1a52ec6045..8a43cb97af 100644
--- a/xen/include/asm-x86/hvm/irq.h
+++ b/xen/include/asm-x86/hvm/irq.h
@@ -207,8 +207,9 @@ int hvm_set_pci_link_route(struct domain *d, u8 link, u8 isa_irq);
 
 int hvm_inject_msi(struct domain *d, uint64_t addr, uint32_t data);
 
-/* Assert an IO APIC pin. */
+/* Assert/deassert an IO APIC pin. */
 int hvm_ioapic_assert(struct domain *d, unsigned int gsi, bool level);
+void hvm_ioapic_deassert(struct domain *d, unsigned int gsi);
 
 void hvm_maybe_deassert_evtchn_irq(void);
 void hvm_assert_evtchn_irq(struct vcpu *v);
-- 
2.17.1


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

[PATCH XTF] add HPET functional test

[Roger Pau Monne]

In order to test HPET level trigger interrupts.

Note that the test doesn't check that the interrupt is injected
correctly, only that the status bits are properly set an acknowledged
when using HPET with level triggered interrupts.

Signed-off-by: Roger Pau Monné <roger.pau@citrix.com>
---
Cc: Andrew Cooper <andrew.cooper3@citrix.com>
---
 arch/x86/hpet.c              |  3 ++
 arch/x86/include/arch/hpet.h | 55 +++++++++++++++++++++
 docs/all-tests.dox           |  2 +
 include/xtf/libc.h           |  3 ++
 tests/hpet/Makefile          |  9 ++++
 tests/hpet/main.c            | 96 ++++++++++++++++++++++++++++++++++++
 6 files changed, 168 insertions(+)
 create mode 100644 tests/hpet/Makefile
 create mode 100644 tests/hpet/main.c

diff --git a/arch/x86/hpet.c b/arch/x86/hpet.c
index 8c18dac..f45927e 100644
--- a/arch/x86/hpet.c
+++ b/arch/x86/hpet.c
@@ -68,6 +68,9 @@ void hpet_init_timer(unsigned int nr, unsigned int irq, uint64_t ticks,
     hpet_write64(HPET_CFG, cfg & ~HPET_CFG_ENABLE);
     hpet_write64(HPET_COUNTER, 0);
 
+    /* Clear any pending status bit. */
+    hpet_timer_status_clear(nr);
+
     /* Configure timer and setup comparator. */
     hpet_write32(HPET_Tn_CFG(nr), tm.raw);
     hpet_write64(HPET_Tn_CMP(nr), ticks);
diff --git a/arch/x86/include/arch/hpet.h b/arch/x86/include/arch/hpet.h
index ea882ef..8264b0e 100644
--- a/arch/x86/include/arch/hpet.h
+++ b/arch/x86/include/arch/hpet.h
@@ -20,9 +20,12 @@
 #define HPET_CFG                0x010
 #define   HPET_CFG_ENABLE       0x001
 
+#define HPET_STATUS             0x020
+
 #define HPET_COUNTER            0x0f0
 
 #define HPET_Tn_CFG(n)         (0x100 + (n) * 0x20)
+#define   HPET_TN_PERIODIC_CAP  0x010
 
 #define HPET_Tn_CMP(n)         (0x108 + (n) * 0x20)
 
@@ -76,6 +79,58 @@ static inline uint64_t hpet_read_counter(void)
     }
 }
 
+/**
+ * Fetch the HPET comparator register of a timer.
+ */
+static inline uint64_t hpet_read_cmp(unsigned int timer)
+{
+    if ( IS_DEFINED(CONFIG_64BIT) )
+        return hpet_read64(HPET_Tn_CMP(timer));
+    else
+    {
+        uint32_t lo, hi;
+
+        do {
+            hi = hpet_read32(HPET_Tn_CMP(timer) + 4);
+            lo = hpet_read32(HPET_Tn_CMP(timer));
+        } while ( hi != hpet_read32(HPET_Tn_CMP(timer) + 4) );
+
+        return ((uint64_t)hi << 32) | lo;
+    }
+}
+
+/**
+ * Fetch the bitmap of supported IRQs of a timer.
+ */
+static inline uint32_t hpet_timer_irqs(unsigned int timer)
+{
+    return hpet_read32(HPET_Tn_CFG(timer) + 4);
+}
+
+/**
+ * Check if a timer supports periodic mode.
+ */
+static inline bool hpet_timer_periodic(unsigned int timer)
+{
+    return hpet_read32(HPET_Tn_CFG(timer)) & HPET_TN_PERIODIC_CAP;
+}
+
+/**
+ * Check the status bit of a timer (only valid for level mode).
+ */
+static inline bool hpet_timer_status(unsigned int timer)
+{
+    return (hpet_read32(HPET_STATUS) >> timer) & 1;
+}
+
+/**
+ * Clear the status bit of a timer.
+ */
+static inline void hpet_timer_status_clear(unsigned int timer)
+{
+    hpet_write32(HPET_STATUS, 1u << timer);
+}
+
 /**
  * Setup and enable a specific HPET timer.
  */
diff --git a/docs/all-tests.dox b/docs/all-tests.dox
index f8a495a..624c365 100644
--- a/docs/all-tests.dox
+++ b/docs/all-tests.dox
@@ -20,6 +20,8 @@ and functionality.
 
 @subpage test-fpu-exception-emulation - FPU Exception Emulation.  Covers XSA-190.
 
+@subpage test-hpet - HPET functional test.
+
 @subpage test-invlpg - `invlpg` instruction behaviour.
 
 @subpage test-lbr-tsx-vmentry - Haswell and later LBR/TSX Vmentry failure test.
diff --git a/include/xtf/libc.h b/include/xtf/libc.h
index 66f834b..55328f4 100644
--- a/include/xtf/libc.h
+++ b/include/xtf/libc.h
@@ -35,6 +35,9 @@ void *memcpy(void *dst, const void *src, size_t n);
 int memcmp(const void *s1, const void *s2, size_t n);
 #define memcmp(s1, s2, n)           __builtin_memcmp(s1, s2, n)
 
+int ffs(int v);
+#define ffs(v)                      __builtin_ffs(v)
+
 size_t strnlen(const char *str, size_t max);
 
 int __printf(3, 0)
diff --git a/tests/hpet/Makefile b/tests/hpet/Makefile
new file mode 100644
index 0000000..4e71587
--- /dev/null
+++ b/tests/hpet/Makefile
@@ -0,0 +1,9 @@
+include $(ROOT)/build/common.mk
+
+NAME      := hpet
+CATEGORY  := functional
+TEST-ENVS := hvm64
+
+obj-perenv += main.o
+
+include $(ROOT)/build/gen.mk
diff --git a/tests/hpet/main.c b/tests/hpet/main.c
new file mode 100644
index 0000000..197b3db
--- /dev/null
+++ b/tests/hpet/main.c
@@ -0,0 +1,96 @@
+/**
+ * @file tests/hpet/main.c
+ * @ref test-hpet - HPET functional test
+ *
+ * @page test-hpet HPET functional test
+ *
+ * HPET functional test.
+ *
+ * Only one timer is tested. No interrupt delivery tests.
+ *
+ * @see tests/hpet/main.c
+ */
+#include <xtf.h>
+
+#include <arch/hpet.h>
+
+const char test_title[] = "Test HPET";
+
+static void wait_cmp(unsigned int timer)
+{
+    uint64_t cmp = hpet_read_cmp(timer);
+
+    while ( hpet_read_counter() < cmp );
+}
+
+void test_main(void)
+{
+    unsigned int irq, timer;
+    uint64_t counter;
+
+    if ( hpet_init() || !hpet_nr_timers )
+        return xtf_skip("Error: cannot find working HPET\n");
+
+    /* Pick a timer that supports periodic mode. */
+    for ( timer = 0; timer < hpet_nr_timers; timer++ )
+        if ( hpet_timer_periodic(timer) )
+            break;
+
+    if ( timer == hpet_nr_timers )
+        return xtf_skip("Error: cannot find timer with periodic mode\n");
+
+    /* Select a valid IRQ. */
+    irq = ffs(hpet_timer_irqs(timer));
+    if ( !irq )
+        return xtf_skip("Error: cannot find a valid IRQ\n");
+    irq--;
+
+    printk("Routing timer %u to IRQ %u\n", timer, irq);
+
+    /* Test oneshot mode. */
+    hpet_init_timer(timer, irq, 1, true, false, false);
+    wait_cmp(timer);
+    if ( !hpet_timer_status(timer) )
+        return xtf_failure(
+"Fail: status bit unset for level interrupt in oneshot mode\n");
+
+    /* Try periodic mode. */
+    hpet_init_timer(timer, irq, 1, true, true, false);
+    wait_cmp(timer);
+    if ( !hpet_timer_status(timer) )
+        return xtf_failure(
+"Fail: status bit unset for level interrupt in periodic mode\n");
+
+    /*
+     * The comparator register should continue to be updated despite the status
+     * not being cleared.
+     */
+    counter = hpet_read_counter();
+    if ( hpet_read_cmp(timer) < counter )
+        return xtf_failure("Fail: comparator not updated in periodic mode\n");
+
+    /* Clear the status bit and wait for it to be set again. */
+    hpet_timer_status_clear(timer);
+    wait_cmp(timer);
+    if ( !hpet_timer_status(timer) )
+        return xtf_failure(
+"Fail: status bit unset for repeated level interrupt in periodic mode\n");
+
+    /* Switch to edge mode and check status bit it's not set again. */
+    hpet_init_timer(timer, irq, 1, false, false, false);
+    wait_cmp(timer);
+    if ( hpet_timer_status(timer) )
+        return xtf_failure("Fail: status bit set for edge interrupt\n");
+
+    xtf_success(NULL);
+}
+
+/*
+ * Local variables:
+ * mode: C
+ * c-file-style: "BSD"
+ * c-basic-offset: 4
+ * tab-width: 4
+ * indent-tabs-mode: nil
+ * End:
+ */
-- 
2.17.1


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

Re: [PATCH v3 1/6] vpt: fix create_periodic_time to use the irq parameter

[Jan Beulich]

>>> On 08.06.18 at 17:07, <roger.pau@citrix.com> wrote:
> Instead of the stale value inside the periodic_time struct.

I think this wants its title changed, because ...

> --- a/xen/arch/x86/hvm/vpt.c
> +++ b/xen/arch/x86/hvm/vpt.c
> @@ -445,8 +445,8 @@ void create_periodic_time(
>      uint64_t period, uint8_t irq, time_cb *cb, void *data)
>  {
>      if ( !pt->source ||
> -         (pt->irq >= NR_ISAIRQS && pt->source == PTSRC_isa) ||
> -         (pt->irq >= hvm_domain_irq(v->domain)->nr_gsis &&
> +         (irq >= NR_ISAIRQS && pt->source == PTSRC_isa) ||
> +         (irq >= hvm_domain_irq(v->domain)->nr_gsis &&
>            pt->source == PTSRC_ioapic) )
>      {
>          ASSERT_UNREACHABLE();

... further down from here there is a use of the parameter. Maybe
"check" instead of "use"? With that (easy enough to do while
committing)
Reviewed-by: Jan Beulich <jbeulich@suse.com>

Jan



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

Re: [PATCH v3 2/6] vhpet: check that the set interrupt route is valid

[Jan Beulich]

>>> On 08.06.18 at 17:07, <roger.pau@citrix.com> wrote:
> The value written by the guest must be valid according to the mask
> provided in the interrupt routing capabilities register. If the
> interrupt is not valid set it to the first valid IRQ in the
> capabilities field if the timer is enabled, else just clear the field.
> 
> Also refuse to start any timer that has an invalid interrupt route.
> 
> Signed-off-by: Roger Pau Monné <roger.pau@citrix.com>

Reviewed-by: Jan Beulich <jbeulich@suse.com>


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

Re: [PATCH v3 3/6] vpt: convert periodic_time fields to bool

[Jan Beulich]

>>> On 08.06.18 at 17:07, <roger.pau@citrix.com> wrote:
> No functional change.
> 
> Signed-off-by: Roger Pau Monné <roger.pau@citrix.com>

Acked-by: Jan Beulich <jbeulich@suse.com>


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

Re: [PATCH v3 4/6] vpt: split part of pt_intr_post into a separate helper

[Jan Beulich]

>>> On 08.06.18 at 17:07, <roger.pau@citrix.com> wrote:
> No functional change.
> 
> Signed-off-by: Roger Pau Monné <roger.pau@citrix.com>

Assuming a subsequent patch re-uses the function
Acked-by: Jan Beulich <jbeulich@suse.com>

It would be nice though to convert ...

> --- a/xen/arch/x86/hvm/vpt.c
> +++ b/xen/arch/x86/hvm/vpt.c
> @@ -265,6 +265,41 @@ static void pt_timer_fn(void *data)
>      pt_unlock(pt);
>  }
>  
> +static void pt_irq_fired(struct vcpu *v, struct periodic_time *pt)
> +{
> +    pt->irq_issued = 0;

... this and ...

> +    if ( pt->one_shot )
> +    {
> +        if ( pt->on_list )
> +            list_del(&pt->list);
> +        pt->on_list = 0;

... this to "false" in the course of moving the code. Once again an
adjustment that perhaps can be done while committing.

Jan


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

Re: [PATCH v3 5/6] vpt: add support for level interrupts

[Jan Beulich]

>>> On 08.06.18 at 17:07, <roger.pau@citrix.com> wrote:
> @@ -316,7 +317,9 @@ int pt_update_irq(struct vcpu *v)
>          if ( pt->pending_intr_nr )
>          {
>              /* RTC code takes care of disabling the timer itself. */
> -            if ( (pt->irq != RTC_IRQ || !pt->priv) && pt_irq_masked(pt) )
> +            if ( (pt->irq != RTC_IRQ || !pt->priv) && pt_irq_masked(pt) &&
> +                 /* Level interrupts should be asserted even if masked. */
> +                 !pt->level )
>              {
>                  /* suspend timer emulation */

Especially with this comment I'm not convinced this change is fully
correct: Once a level triggered interrupt is latched in IRR, no
further assertions are meaningful, and hence emulation could (and
hence should) still be stopped to reduce resource consumption.

> @@ -374,13 +378,36 @@ int pt_update_irq(struct vcpu *v)
>          break;
>  
>      case PTSRC_ioapic:
> -        /*
> -         * NB: At the moment IO-APIC routed interrupts generated by vpt devices
> -         * (HPET) are edge-triggered.
> -         */
> -        pt_vector = hvm_ioapic_assert(v->domain, irq, false);
> +        pt_vector = hvm_ioapic_assert(v->domain, irq, level);
>          if ( pt_vector < 0 || !vlapic_test_irq(vcpu_vlapic(v), pt_vector) )
> +        {
>              pt_vector = -1;
> +            if ( level )
> +            {
> +                /*
> +                 * Level interrupts are asserted even if the interrupt is
> +                 * masked, so also execute the callback associated with the
> +                 * timer.
> +                 */
> +                time_cb *cb = NULL;
> +                void *cb_priv;
> +
> +                spin_lock(&v->arch.hvm_vcpu.tm_lock);
> +                /* Make sure the timer is still on the list. */
> +                list_for_each_entry ( pt, &v->arch.hvm_vcpu.tm_list, list )
> +                    if ( pt == earliest_pt )
> +                    {
> +                        pt_irq_fired(v, pt);
> +                        cb = pt->cb;
> +                        cb_priv = pt->priv;
> +                        break;
> +                    }
> +                spin_unlock(&v->arch.hvm_vcpu.tm_lock);
> +
> +                if ( cb != NULL )
> +                    cb(v, cb_priv);
> +            }
> +        }
>          break;

I'm not fully convinced, especially in the case that hvm_ioapic_assert()
returned a negative value: Either the callback needs to be called in all
cases (even if an edge triggered interrupt was not successfully asserted),
or only when an interrupt really gets surfaced to the guest.

> @@ -447,12 +474,13 @@ void pt_migrate(struct vcpu *v)
>  
>  void create_periodic_time(
>      struct vcpu *v, struct periodic_time *pt, uint64_t delta,
> -    uint64_t period, uint8_t irq, time_cb *cb, void *data)
> +    uint64_t period, uint8_t irq, time_cb *cb, void *data, bool level)
>  {
>      if ( !pt->source ||
>           (irq >= NR_ISAIRQS && pt->source == PTSRC_isa) ||
>           (irq >= hvm_domain_irq(v->domain)->nr_gsis &&
> -          pt->source == PTSRC_ioapic) )
> +          pt->source == PTSRC_ioapic) ||
> +         (level && pt->source != PTSRC_ioapic) )

Could I talk you into avoiding the double checking against PTSRC_ioapic,
by using a conditional expression?

Jan



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

Re: [PATCH v3 6/6] vhpet: add support for level triggered interrupts

[Jan Beulich]

>>> On 08.06.18 at 17:07, <roger.pau@citrix.com> wrote:
> --- a/xen/arch/x86/hvm/hpet.c
> +++ b/xen/arch/x86/hvm/hpet.c
> @@ -223,6 +223,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;

I don't think this cast will go through without warning on all gcc versions we
care about.

> +    HPETState *h = vcpu_vhpet(v);
> +
> +    write_lock(&h->lock);
> +    ASSERT(!test_bit(tn, &h->hpet.isr));
> +    __set_bit(tn, &h->hpet.isr);

    if ( __test_and_set_bit() )
        ASSERT_UNREACHABLE();

?

Seeing this I can understand why you want to call the callback the way
you do in the previous patch. I continue to be unconvinced this second
call is generally correct (and sufficient). Simply consider the RTC case,
where in theory the IRQ could also be level triggered.

> @@ -394,6 +411,32 @@ static int hpet_write(
>          }
>          break;
>  
> +    case HPET_STATUS:
> +        /* write 1 to clear. */
> +        while ( new_val )
> +        {
> +            bool active;
> +
> +            i = find_first_set_bit(new_val);
> +            if ( i >= HPET_TIMER_NUM )
> +                break;
> +            __clear_bit(i, &new_val);
> +            active = __test_and_clear_bit(i, &h->hpet.isr);
> +            if ( active )
> +            {
> +                /*
> +                 * Should pt->irq better be used here in case the guest changes
> +                 * the configured IRQ while it's active? Guest changing the IRQ
> +                 * while the interrupt is active is not documented.
> +                 */

I think it's better the way you have it, to base things on what is recorded
in h->hpet.isr. After all that's what has been asserted. In fact I don't see
how using pt->irq would address the situation: Isn't it that what changes
first, and hence the de-assert done here would go out of sync with the
prior assert?

> +                hvm_ioapic_deassert(v->domain, timer_int_route(h, i));
> +                if ( hpet_enabled(h) && timer_enabled(h, i) &&
> +                     timer_level(h, i) && timer_is_periodic(h, i) )
> +                    set_start_timer(i);
> +            }
> +        }
> +        break;

What I'm wondering though: Does there really need to be a loop here?
How would more than one bit get set in h->hpet.isr?

Jan



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

Re: [PATCH v3 5/6] vpt: add support for level interrupts

[Roger Pau Monné]

On Fri, Jun 22, 2018 at 08:23:02AM -0600, Jan Beulich wrote:
> >>> On 08.06.18 at 17:07, <roger.pau@citrix.com> wrote:
> > @@ -316,7 +317,9 @@ int pt_update_irq(struct vcpu *v)
> >          if ( pt->pending_intr_nr )
> >          {
> >              /* RTC code takes care of disabling the timer itself. */
> > -            if ( (pt->irq != RTC_IRQ || !pt->priv) && pt_irq_masked(pt) )
> > +            if ( (pt->irq != RTC_IRQ || !pt->priv) && pt_irq_masked(pt) &&
> > +                 /* Level interrupts should be asserted even if masked. */
> > +                 !pt->level )
> >              {
> >                  /* suspend timer emulation */
> 
> Especially with this comment I'm not convinced this change is fully
> correct: Once a level triggered interrupt is latched in IRR, no
> further assertions are meaningful, and hence emulation could (and
> hence should) still be stopped to reduce resource consumption.

Hm, I can see your point, but that's going to make the implementation
of HPET level trigger interrupts more complex.

From my reading of the spec, when a level triggered HPET interrupt
fires the ISR bit must be set, regardless of whether the IRR of the
io-apic entry is set or not.

Maybe the right solution is to add a pt_irq_pending that checks the
IRR bit, and a new flag that the caller can set in order to requests
callbacks regardless of whether the interrupt has been injected or
not. Would you agree to that plan?

> > @@ -374,13 +378,36 @@ int pt_update_irq(struct vcpu *v)
> >          break;
> >  
> >      case PTSRC_ioapic:
> > -        /*
> > -         * NB: At the moment IO-APIC routed interrupts generated by vpt devices
> > -         * (HPET) are edge-triggered.
> > -         */
> > -        pt_vector = hvm_ioapic_assert(v->domain, irq, false);
> > +        pt_vector = hvm_ioapic_assert(v->domain, irq, level);
> >          if ( pt_vector < 0 || !vlapic_test_irq(vcpu_vlapic(v), pt_vector) )
> > +        {
> >              pt_vector = -1;
> > +            if ( level )
> > +            {
> > +                /*
> > +                 * Level interrupts are asserted even if the interrupt is
> > +                 * masked, so also execute the callback associated with the
> > +                 * timer.
> > +                 */
> > +                time_cb *cb = NULL;
> > +                void *cb_priv;
> > +
> > +                spin_lock(&v->arch.hvm_vcpu.tm_lock);
> > +                /* Make sure the timer is still on the list. */
> > +                list_for_each_entry ( pt, &v->arch.hvm_vcpu.tm_list, list )
> > +                    if ( pt == earliest_pt )
> > +                    {
> > +                        pt_irq_fired(v, pt);
> > +                        cb = pt->cb;
> > +                        cb_priv = pt->priv;
> > +                        break;
> > +                    }
> > +                spin_unlock(&v->arch.hvm_vcpu.tm_lock);
> > +
> > +                if ( cb != NULL )
> > +                    cb(v, cb_priv);
> > +            }
> > +        }
> >          break;
> 
> I'm not fully convinced, especially in the case that hvm_ioapic_assert()
> returned a negative value: Either the callback needs to be called in all
> cases (even if an edge triggered interrupt was not successfully asserted),
> or only when an interrupt really gets surfaced to the guest.

See my proposal above for adding a new option to signal whether the
callback should be executed regardless of whether interrupt injection
succeeded or not.

> 
> > @@ -447,12 +474,13 @@ void pt_migrate(struct vcpu *v)
> >  
> >  void create_periodic_time(
> >      struct vcpu *v, struct periodic_time *pt, uint64_t delta,
> > -    uint64_t period, uint8_t irq, time_cb *cb, void *data)
> > +    uint64_t period, uint8_t irq, time_cb *cb, void *data, bool level)
> >  {
> >      if ( !pt->source ||
> >           (irq >= NR_ISAIRQS && pt->source == PTSRC_isa) ||
> >           (irq >= hvm_domain_irq(v->domain)->nr_gsis &&
> > -          pt->source == PTSRC_ioapic) )
> > +          pt->source == PTSRC_ioapic) ||
> > +         (level && pt->source != PTSRC_ioapic) )
> 
> Could I talk you into avoiding the double checking against PTSRC_ioapic,
> by using a conditional expression?

So something like:

pt->source == PTSRC_ioapic ? irq >= hvm_domain_irq(v->domain)->nr_gsis : level

Thanks, Roger.

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

Re: [PATCH v3 6/6] vhpet: add support for level triggered interrupts

[Roger Pau Monné]

On Fri, Jun 22, 2018 at 09:00:27AM -0600, Jan Beulich wrote:
> >>> On 08.06.18 at 17:07, <roger.pau@citrix.com> wrote:
> > --- a/xen/arch/x86/hvm/hpet.c
> > +++ b/xen/arch/x86/hvm/hpet.c
> > @@ -223,6 +223,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;
> 
> I don't think this cast will go through without warning on all gcc versions we
> care about.

Hm, should be casted to unsigned long I guess so it's the same size.

> > +    HPETState *h = vcpu_vhpet(v);
> > +
> > +    write_lock(&h->lock);
> > +    ASSERT(!test_bit(tn, &h->hpet.isr));
> > +    __set_bit(tn, &h->hpet.isr);
> 
>     if ( __test_and_set_bit() )
>         ASSERT_UNREACHABLE();
> 
> ?
> 
> Seeing this I can understand why you want to call the callback the way
> you do in the previous patch. I continue to be unconvinced this second
> call is generally correct (and sufficient). Simply consider the RTC case,
> where in theory the IRQ could also be level triggered.

See my reply to the other patch.

> > @@ -394,6 +411,32 @@ static int hpet_write(
> >          }
> >          break;
> >  
> > +    case HPET_STATUS:
> > +        /* write 1 to clear. */
> > +        while ( new_val )
> > +        {
> > +            bool active;
> > +
> > +            i = find_first_set_bit(new_val);
> > +            if ( i >= HPET_TIMER_NUM )
> > +                break;
> > +            __clear_bit(i, &new_val);
> > +            active = __test_and_clear_bit(i, &h->hpet.isr);
> > +            if ( active )
> > +            {
> > +                /*
> > +                 * Should pt->irq better be used here in case the guest changes
> > +                 * the configured IRQ while it's active? Guest changing the IRQ
> > +                 * while the interrupt is active is not documented.
> > +                 */
> 
> I think it's better the way you have it, to base things on what is recorded
> in h->hpet.isr. After all that's what has been asserted. In fact I don't see
> how using pt->irq would address the situation: Isn't it that what changes
> first, and hence the de-assert done here would go out of sync with the
> prior assert?

What's in the HPET state can be changed by guest writes, so it might
be more accurate to use pt->irq, which is the IRQ that was asserted by
the vpt code. In any case, I'm not specially trilled because a guest
changing this while the interrupt is active is certainly asking for
trouble.

> > +                hvm_ioapic_deassert(v->domain, timer_int_route(h, i));
> > +                if ( hpet_enabled(h) && timer_enabled(h, i) &&
> > +                     timer_level(h, i) && timer_is_periodic(h, i) )
> > +                    set_start_timer(i);
> > +            }
> > +        }
> > +        break;
> 
> What I'm wondering though: Does there really need to be a loop here?
> How would more than one bit get set in h->hpet.isr?

The current HPET code exposes 3 timers, and all of them can be set to
level triggered, so in theory you could clear the 3 ISR bits with one
write, hence the loop.

Roger.

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

Re: [PATCH v3 5/6] vpt: add support for level interrupts

[Jan Beulich]

>>> On 22.06.18 at 17:24, <roger.pau@citrix.com> wrote:
> On Fri, Jun 22, 2018 at 08:23:02AM -0600, Jan Beulich wrote:
>> >>> On 08.06.18 at 17:07, <roger.pau@citrix.com> wrote:
>> > @@ -316,7 +317,9 @@ int pt_update_irq(struct vcpu *v)
>> >          if ( pt->pending_intr_nr )
>> >          {
>> >              /* RTC code takes care of disabling the timer itself. */
>> > -            if ( (pt->irq != RTC_IRQ || !pt->priv) && pt_irq_masked(pt) )
>> > +            if ( (pt->irq != RTC_IRQ || !pt->priv) && pt_irq_masked(pt) &&
>> > +                 /* Level interrupts should be asserted even if masked. */
>> > +                 !pt->level )
>> >              {
>> >                  /* suspend timer emulation */
>> 
>> Especially with this comment I'm not convinced this change is fully
>> correct: Once a level triggered interrupt is latched in IRR, no
>> further assertions are meaningful, and hence emulation could (and
>> hence should) still be stopped to reduce resource consumption.
> 
> Hm, I can see your point, but that's going to make the implementation
> of HPET level trigger interrupts more complex.
> 
> From my reading of the spec, when a level triggered HPET interrupt
> fires the ISR bit must be set, regardless of whether the IRR of the
> io-apic entry is set or not.
> 
> Maybe the right solution is to add a pt_irq_pending that checks the
> IRR bit, and a new flag that the caller can set in order to requests
> callbacks regardless of whether the interrupt has been injected or
> not. Would you agree to that plan?

That sounds like a reasonable option. Another would be to add a
parameter to the callback allowing the context to be identified to the
callee.

Jan



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

Re: [PATCH v3 5/6] vpt: add support for level interrupts

[Roger Pau Monné]

On Fri, Jun 22, 2018 at 09:53:26AM -0600, Jan Beulich wrote:
> >>> On 22.06.18 at 17:24, <roger.pau@citrix.com> wrote:
> > On Fri, Jun 22, 2018 at 08:23:02AM -0600, Jan Beulich wrote:
> >> >>> On 08.06.18 at 17:07, <roger.pau@citrix.com> wrote:
> >> > @@ -316,7 +317,9 @@ int pt_update_irq(struct vcpu *v)
> >> >          if ( pt->pending_intr_nr )
> >> >          {
> >> >              /* RTC code takes care of disabling the timer itself. */
> >> > -            if ( (pt->irq != RTC_IRQ || !pt->priv) && pt_irq_masked(pt) )
> >> > +            if ( (pt->irq != RTC_IRQ || !pt->priv) && pt_irq_masked(pt) &&
> >> > +                 /* Level interrupts should be asserted even if masked. */
> >> > +                 !pt->level )
> >> >              {
> >> >                  /* suspend timer emulation */
> >> 
> >> Especially with this comment I'm not convinced this change is fully
> >> correct: Once a level triggered interrupt is latched in IRR, no
> >> further assertions are meaningful, and hence emulation could (and
> >> hence should) still be stopped to reduce resource consumption.
> > 
> > Hm, I can see your point, but that's going to make the implementation
> > of HPET level trigger interrupts more complex.
> > 
> > From my reading of the spec, when a level triggered HPET interrupt
> > fires the ISR bit must be set, regardless of whether the IRR of the
> > io-apic entry is set or not.
> > 
> > Maybe the right solution is to add a pt_irq_pending that checks the
> > IRR bit, and a new flag that the caller can set in order to requests
> > callbacks regardless of whether the interrupt has been injected or
> > not. Would you agree to that plan?
> 
> That sounds like a reasonable option. Another would be to add a
> parameter to the callback allowing the context to be identified to the
> callee.

Yes, but that seems less optimal since all callback will be executed
and current callers would just exit if the interrupt was masked. I
will add the new option an send a new version.

Thanks, Roger.

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

Re: [PATCH v3 6/6] vhpet: add support for level triggered interrupts

[Jan Beulich]

>>> On 22.06.18 at 17:31, <roger.pau@citrix.com> wrote:
> On Fri, Jun 22, 2018 at 09:00:27AM -0600, Jan Beulich wrote:
>> >>> On 08.06.18 at 17:07, <roger.pau@citrix.com> wrote:
>> > --- a/xen/arch/x86/hvm/hpet.c
>> > +++ b/xen/arch/x86/hvm/hpet.c
>> > @@ -223,6 +223,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;
>> 
>> I don't think this cast will go through without warning on all gcc versions 
> we
>> care about.
> 
> Hm, should be casted to unsigned long I guess so it's the same size.
> 
>> > +    HPETState *h = vcpu_vhpet(v);
>> > +
>> > +    write_lock(&h->lock);
>> > +    ASSERT(!test_bit(tn, &h->hpet.isr));
>> > +    __set_bit(tn, &h->hpet.isr);
>> 
>>     if ( __test_and_set_bit() )
>>         ASSERT_UNREACHABLE();
>> 
>> ?
>> 
>> Seeing this I can understand why you want to call the callback the way
>> you do in the previous patch. I continue to be unconvinced this second
>> call is generally correct (and sufficient). Simply consider the RTC case,
>> where in theory the IRQ could also be level triggered.
> 
> See my reply to the other patch.
> 
>> > @@ -394,6 +411,32 @@ static int hpet_write(
>> >          }
>> >          break;
>> >  
>> > +    case HPET_STATUS:
>> > +        /* write 1 to clear. */
>> > +        while ( new_val )
>> > +        {
>> > +            bool active;
>> > +
>> > +            i = find_first_set_bit(new_val);
>> > +            if ( i >= HPET_TIMER_NUM )
>> > +                break;
>> > +            __clear_bit(i, &new_val);
>> > +            active = __test_and_clear_bit(i, &h->hpet.isr);
>> > +            if ( active )
>> > +            {
>> > +                /*
>> > +                 * Should pt->irq better be used here in case the guest changes
>> > +                 * the configured IRQ while it's active? Guest changing the IRQ
>> > +                 * while the interrupt is active is not documented.
>> > +                 */
>> 
>> I think it's better the way you have it, to base things on what is recorded
>> in h->hpet.isr. After all that's what has been asserted. In fact I don't see
>> how using pt->irq would address the situation: Isn't it that what changes
>> first, and hence the de-assert done here would go out of sync with the
>> prior assert?
> 
> What's in the HPET state can be changed by guest writes,

Not exactly - the only way to modify h->hpet.isr is through the code above.

> so it might
> be more accurate to use pt->irq, which is the IRQ that was asserted by
> the vpt code.

I.e. I'm viewing it exactly the other way around - the guest can affect
pt->irq directly.

>> > +                hvm_ioapic_deassert(v->domain, timer_int_route(h, i));
>> > +                if ( hpet_enabled(h) && timer_enabled(h, i) &&
>> > +                     timer_level(h, i) && timer_is_periodic(h, i) )
>> > +                    set_start_timer(i);
>> > +            }
>> > +        }
>> > +        break;
>> 
>> What I'm wondering though: Does there really need to be a loop here?
>> How would more than one bit get set in h->hpet.isr?
> 
> The current HPET code exposes 3 timers, and all of them can be set to
> level triggered, so in theory you could clear the 3 ISR bits with one
> write, hence the loop.

Oh, right, I was mislead by the comment above saying pt->irq when you
really mean pt[tn].irq

Jan



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

Re: [PATCH v3 6/6] vhpet: add support for level triggered interrupts

[Roger Pau Monné]

On Fri, Jun 22, 2018 at 10:00:41AM -0600, Jan Beulich wrote:
> >>> On 22.06.18 at 17:31, <roger.pau@citrix.com> wrote:
> > On Fri, Jun 22, 2018 at 09:00:27AM -0600, Jan Beulich wrote:
> >> >>> On 08.06.18 at 17:07, <roger.pau@citrix.com> wrote:
> >> > --- a/xen/arch/x86/hvm/hpet.c
> >> > +++ b/xen/arch/x86/hvm/hpet.c
> >> > @@ -223,6 +223,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;
> >> 
> >> I don't think this cast will go through without warning on all gcc versions 
> > we
> >> care about.
> > 
> > Hm, should be casted to unsigned long I guess so it's the same size.
> > 
> >> > +    HPETState *h = vcpu_vhpet(v);
> >> > +
> >> > +    write_lock(&h->lock);
> >> > +    ASSERT(!test_bit(tn, &h->hpet.isr));
> >> > +    __set_bit(tn, &h->hpet.isr);
> >> 
> >>     if ( __test_and_set_bit() )
> >>         ASSERT_UNREACHABLE();
> >> 
> >> ?
> >> 
> >> Seeing this I can understand why you want to call the callback the way
> >> you do in the previous patch. I continue to be unconvinced this second
> >> call is generally correct (and sufficient). Simply consider the RTC case,
> >> where in theory the IRQ could also be level triggered.
> > 
> > See my reply to the other patch.
> > 
> >> > @@ -394,6 +411,32 @@ static int hpet_write(
> >> >          }
> >> >          break;
> >> >  
> >> > +    case HPET_STATUS:
> >> > +        /* write 1 to clear. */
> >> > +        while ( new_val )
> >> > +        {
> >> > +            bool active;
> >> > +
> >> > +            i = find_first_set_bit(new_val);
> >> > +            if ( i >= HPET_TIMER_NUM )
> >> > +                break;
> >> > +            __clear_bit(i, &new_val);
> >> > +            active = __test_and_clear_bit(i, &h->hpet.isr);
> >> > +            if ( active )
> >> > +            {
> >> > +                /*
> >> > +                 * Should pt->irq better be used here in case the guest changes
> >> > +                 * the configured IRQ while it's active? Guest changing the IRQ
> >> > +                 * while the interrupt is active is not documented.
> >> > +                 */
> >> 
> >> I think it's better the way you have it, to base things on what is recorded
> >> in h->hpet.isr. After all that's what has been asserted. In fact I don't see
> >> how using pt->irq would address the situation: Isn't it that what changes
> >> first, and hence the de-assert done here would go out of sync with the
> >> prior assert?
> > 
> > What's in the HPET state can be changed by guest writes,
> 
> Not exactly - the only way to modify h->hpet.isr is through the code above.

Right, but ISR only signals which timer has fired, not which IRQ the
timer was using. That's stored in pt->irq or h->tn[n].irq, and it's
what Xen needs in order to perform the deassert.

Roger.

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

Re: [PATCH v3 6/6] vhpet: add support for level triggered interrupts

[Jan Beulich]

>>> On 22.06.18 at 18:07, <roger.pau@citrix.com> wrote:
> On Fri, Jun 22, 2018 at 10:00:41AM -0600, Jan Beulich wrote:
>> >>> On 22.06.18 at 17:31, <roger.pau@citrix.com> wrote:
>> > On Fri, Jun 22, 2018 at 09:00:27AM -0600, Jan Beulich wrote:
>> >> >>> On 08.06.18 at 17:07, <roger.pau@citrix.com> wrote:
>> >> > @@ -394,6 +411,32 @@ static int hpet_write(
>> >> >          }
>> >> >          break;
>> >> >  
>> >> > +    case HPET_STATUS:
>> >> > +        /* write 1 to clear. */
>> >> > +        while ( new_val )
>> >> > +        {
>> >> > +            bool active;
>> >> > +
>> >> > +            i = find_first_set_bit(new_val);
>> >> > +            if ( i >= HPET_TIMER_NUM )
>> >> > +                break;
>> >> > +            __clear_bit(i, &new_val);
>> >> > +            active = __test_and_clear_bit(i, &h->hpet.isr);
>> >> > +            if ( active )
>> >> > +            {
>> >> > +                /*
>> >> > +                 * Should pt->irq better be used here in case the guest changes
>> >> > +                 * the configured IRQ while it's active? Guest changing the IRQ
>> >> > +                 * while the interrupt is active is not documented.
>> >> > +                 */
>> >> 
>> >> I think it's better the way you have it, to base things on what is recorded
>> >> in h->hpet.isr. After all that's what has been asserted. In fact I don't see
>> >> how using pt->irq would address the situation: Isn't it that what changes
>> >> first, and hence the de-assert done here would go out of sync with the
>> >> prior assert?
>> > 
>> > What's in the HPET state can be changed by guest writes,
>> 
>> Not exactly - the only way to modify h->hpet.isr is through the code above.
> 
> Right, but ISR only signals which timer has fired, not which IRQ the
> timer was using. That's stored in pt->irq or h->tn[n].irq, and it's
> what Xen needs in order to perform the deassert.

Hmm, that's one way to look at things. The other is that the needed
information can be derived from just HPET data, by using the ISR bit
and check what GSI the respective channel is configured to use. The
status bits are (intentionally afaict) described as "Timer <N> Interrupt
Active" - once the timer fires, the interrupt gets asserted no matter
what, it may just not make it through because of being masked. I'm
not sure which of the two approaches are better in particular for the
case when the interrupt becomes unmasked in the IO-APIC when it's
already asserted. Perhaps, when getting reconfigured while already
asserted (but masked), gsi_assert_count[] would need to be adjusted?
But as you've said before - the spec doesn't mention this case, so
quite likely we can get away without taking special precautions...

Jan



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

Re: [PATCH v3 6/6] vhpet: add support for level triggered interrupts

[Roger Pau Monné]

On Mon, Jun 25, 2018 at 02:26:00AM -0600, Jan Beulich wrote:
> >>> On 22.06.18 at 18:07, <roger.pau@citrix.com> wrote:
> > On Fri, Jun 22, 2018 at 10:00:41AM -0600, Jan Beulich wrote:
> >> >>> On 22.06.18 at 17:31, <roger.pau@citrix.com> wrote:
> >> > On Fri, Jun 22, 2018 at 09:00:27AM -0600, Jan Beulich wrote:
> >> >> >>> On 08.06.18 at 17:07, <roger.pau@citrix.com> wrote:
> >> >> > @@ -394,6 +411,32 @@ static int hpet_write(
> >> >> >          }
> >> >> >          break;
> >> >> >  
> >> >> > +    case HPET_STATUS:
> >> >> > +        /* write 1 to clear. */
> >> >> > +        while ( new_val )
> >> >> > +        {
> >> >> > +            bool active;
> >> >> > +
> >> >> > +            i = find_first_set_bit(new_val);
> >> >> > +            if ( i >= HPET_TIMER_NUM )
> >> >> > +                break;
> >> >> > +            __clear_bit(i, &new_val);
> >> >> > +            active = __test_and_clear_bit(i, &h->hpet.isr);
> >> >> > +            if ( active )
> >> >> > +            {
> >> >> > +                /*
> >> >> > +                 * Should pt->irq better be used here in case the guest changes
> >> >> > +                 * the configured IRQ while it's active? Guest changing the IRQ
> >> >> > +                 * while the interrupt is active is not documented.
> >> >> > +                 */
> >> >> 
> >> >> I think it's better the way you have it, to base things on what is recorded
> >> >> in h->hpet.isr. After all that's what has been asserted. In fact I don't see
> >> >> how using pt->irq would address the situation: Isn't it that what changes
> >> >> first, and hence the de-assert done here would go out of sync with the
> >> >> prior assert?
> >> > 
> >> > What's in the HPET state can be changed by guest writes,
> >> 
> >> Not exactly - the only way to modify h->hpet.isr is through the code above.
> > 
> > Right, but ISR only signals which timer has fired, not which IRQ the
> > timer was using. That's stored in pt->irq or h->tn[n].irq, and it's
> > what Xen needs in order to perform the deassert.
> 
> Hmm, that's one way to look at things. The other is that the needed
> information can be derived from just HPET data, by using the ISR bit
> and check what GSI the respective channel is configured to use. The
> status bits are (intentionally afaict) described as "Timer <N> Interrupt
> Active" - once the timer fires, the interrupt gets asserted no matter
> what, it may just not make it through because of being masked. I'm
> not sure which of the two approaches are better in particular for the
> case when the interrupt becomes unmasked in the IO-APIC when it's
> already asserted. Perhaps, when getting reconfigured while already
> asserted (but masked), gsi_assert_count[] would need to be adjusted?

As you say, maybe we should do a deassert of the previous interrupt if
ISR is set when reconfiguring?

> But as you've said before - the spec doesn't mention this case, so
> quite likely we can get away without taking special precautions...

IMO, as a starting implementation I would leave this as-is (no
deassert on reconfigure). We can always add more logic later if issues
are discovered.

Thanks, Roger.

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

Re: [PATCH v3 5/6] vpt: add support for level interrupts

[Roger Pau Monné]

On Fri, Jun 22, 2018 at 08:23:02AM -0600, Jan Beulich wrote:
> >>> On 08.06.18 at 17:07, <roger.pau@citrix.com> wrote:
> > @@ -316,7 +317,9 @@ int pt_update_irq(struct vcpu *v)
> >          if ( pt->pending_intr_nr )
> >          {
> >              /* RTC code takes care of disabling the timer itself. */
> > -            if ( (pt->irq != RTC_IRQ || !pt->priv) && pt_irq_masked(pt) )
> > +            if ( (pt->irq != RTC_IRQ || !pt->priv) && pt_irq_masked(pt) &&
> > +                 /* Level interrupts should be asserted even if masked. */
> > +                 !pt->level )
> >              {
> >                  /* suspend timer emulation */
> 
> Especially with this comment I'm not convinced this change is fully
> correct: Once a level triggered interrupt is latched in IRR, no
> further assertions are meaningful, and hence emulation could (and
> hence should) still be stopped to reduce resource consumption.

I've been thinking about this and I think the above comment is not
fully correct. Assertion of latched level interrupts is meaningful
because gsi_assert_count should be increased regardless of the state
of IRR.

Your comment made me realize that periodic level triggered interrupts
don't make much sense in vpt. IO-APIC level triggered interrupts will
require external deassertion of the line, in which case such
deassertion should also take care of reprogramming the timer if
required (like it's done in the next patch for vhpet when clearing
ISR).

> > @@ -374,13 +378,36 @@ int pt_update_irq(struct vcpu *v)
> >          break;
> >  
> >      case PTSRC_ioapic:
> > -        /*
> > -         * NB: At the moment IO-APIC routed interrupts generated by vpt devices
> > -         * (HPET) are edge-triggered.
> > -         */
> > -        pt_vector = hvm_ioapic_assert(v->domain, irq, false);
> > +        pt_vector = hvm_ioapic_assert(v->domain, irq, level);
> >          if ( pt_vector < 0 || !vlapic_test_irq(vcpu_vlapic(v), pt_vector) )
> > +        {
> >              pt_vector = -1;
> > +            if ( level )
> > +            {
> > +                /*
> > +                 * Level interrupts are asserted even if the interrupt is
> > +                 * masked, so also execute the callback associated with the
> > +                 * timer.
> > +                 */
> > +                time_cb *cb = NULL;
> > +                void *cb_priv;
> > +
> > +                spin_lock(&v->arch.hvm_vcpu.tm_lock);
> > +                /* Make sure the timer is still on the list. */
> > +                list_for_each_entry ( pt, &v->arch.hvm_vcpu.tm_list, list )
> > +                    if ( pt == earliest_pt )
> > +                    {
> > +                        pt_irq_fired(v, pt);
> > +                        cb = pt->cb;
> > +                        cb_priv = pt->priv;
> > +                        break;
> > +                    }
> > +                spin_unlock(&v->arch.hvm_vcpu.tm_lock);
> > +
> > +                if ( cb != NULL )
> > +                    cb(v, cb_priv);
> > +            }
> > +        }
> >          break;
> 
> I'm not fully convinced, especially in the case that hvm_ioapic_assert()
> returned a negative value: Either the callback needs to be called in all
> cases (even if an edge triggered interrupt was not successfully asserted),
> or only when an interrupt really gets surfaced to the guest.

Even if hvm_ioapic_assert returns -1 gsi_assert_count will be
increased, so I think it makes sense to also call the callback in this
case, because the line has been asserted.

Roger.

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

Re: [PATCH v3 5/6] vpt: add support for level interrupts

[Roger Pau Monné]

Ping?

On Mon, Jun 25, 2018 at 01:19:19PM +0200, Roger Pau Monné wrote:
> On Fri, Jun 22, 2018 at 08:23:02AM -0600, Jan Beulich wrote:
> > >>> On 08.06.18 at 17:07, <roger.pau@citrix.com> wrote:
> > > @@ -316,7 +317,9 @@ int pt_update_irq(struct vcpu *v)
> > >          if ( pt->pending_intr_nr )
> > >          {
> > >              /* RTC code takes care of disabling the timer itself. */
> > > -            if ( (pt->irq != RTC_IRQ || !pt->priv) && pt_irq_masked(pt) )
> > > +            if ( (pt->irq != RTC_IRQ || !pt->priv) && pt_irq_masked(pt) &&
> > > +                 /* Level interrupts should be asserted even if masked. */
> > > +                 !pt->level )
> > >              {
> > >                  /* suspend timer emulation */
> > 
> > Especially with this comment I'm not convinced this change is fully
> > correct: Once a level triggered interrupt is latched in IRR, no
> > further assertions are meaningful, and hence emulation could (and
> > hence should) still be stopped to reduce resource consumption.
> 
> I've been thinking about this and I think the above comment is not
> fully correct. Assertion of latched level interrupts is meaningful
> because gsi_assert_count should be increased regardless of the state
> of IRR.
> 
> Your comment made me realize that periodic level triggered interrupts
> don't make much sense in vpt. IO-APIC level triggered interrupts will
> require external deassertion of the line, in which case such
> deassertion should also take care of reprogramming the timer if
> required (like it's done in the next patch for vhpet when clearing
> ISR).
> 
> > > @@ -374,13 +378,36 @@ int pt_update_irq(struct vcpu *v)
> > >          break;
> > >  
> > >      case PTSRC_ioapic:
> > > -        /*
> > > -         * NB: At the moment IO-APIC routed interrupts generated by vpt devices
> > > -         * (HPET) are edge-triggered.
> > > -         */
> > > -        pt_vector = hvm_ioapic_assert(v->domain, irq, false);
> > > +        pt_vector = hvm_ioapic_assert(v->domain, irq, level);
> > >          if ( pt_vector < 0 || !vlapic_test_irq(vcpu_vlapic(v), pt_vector) )
> > > +        {
> > >              pt_vector = -1;
> > > +            if ( level )
> > > +            {
> > > +                /*
> > > +                 * Level interrupts are asserted even if the interrupt is
> > > +                 * masked, so also execute the callback associated with the
> > > +                 * timer.
> > > +                 */
> > > +                time_cb *cb = NULL;
> > > +                void *cb_priv;
> > > +
> > > +                spin_lock(&v->arch.hvm_vcpu.tm_lock);
> > > +                /* Make sure the timer is still on the list. */
> > > +                list_for_each_entry ( pt, &v->arch.hvm_vcpu.tm_list, list )
> > > +                    if ( pt == earliest_pt )
> > > +                    {
> > > +                        pt_irq_fired(v, pt);
> > > +                        cb = pt->cb;
> > > +                        cb_priv = pt->priv;
> > > +                        break;
> > > +                    }
> > > +                spin_unlock(&v->arch.hvm_vcpu.tm_lock);
> > > +
> > > +                if ( cb != NULL )
> > > +                    cb(v, cb_priv);
> > > +            }
> > > +        }
> > >          break;
> > 
> > I'm not fully convinced, especially in the case that hvm_ioapic_assert()
> > returned a negative value: Either the callback needs to be called in all
> > cases (even if an edge triggered interrupt was not successfully asserted),
> > or only when an interrupt really gets surfaced to the guest.
> 
> Even if hvm_ioapic_assert returns -1 gsi_assert_count will be
> increased, so I think it makes sense to also call the callback in this
> case, because the line has been asserted.
> 
> Roger.
> 
> _______________________________________________
> Xen-devel mailing list
> Xen-devel@lists.xenproject.org
> https://lists.xenproject.org/mailman/listinfo/xen-devel

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

Re: [PATCH v3 5/6] vpt: add support for level interrupts

[Jan Beulich]

>>> On 02.07.18 at 16:54, <roger.pau@citrix.com> wrote:
> Ping?

I don't understand: There's no open question in the quoted mail.

Jan

> On Mon, Jun 25, 2018 at 01:19:19PM +0200, Roger Pau Monné wrote:
>> On Fri, Jun 22, 2018 at 08:23:02AM -0600, Jan Beulich wrote:
>> > >>> On 08.06.18 at 17:07, <roger.pau@citrix.com> wrote:
>> > > @@ -316,7 +317,9 @@ int pt_update_irq(struct vcpu *v)
>> > >          if ( pt->pending_intr_nr )
>> > >          {
>> > >              /* RTC code takes care of disabling the timer itself. */
>> > > -            if ( (pt->irq != RTC_IRQ || !pt->priv) && pt_irq_masked(pt) )
>> > > +            if ( (pt->irq != RTC_IRQ || !pt->priv) && pt_irq_masked(pt) &&
>> > > +                 /* Level interrupts should be asserted even if masked. */
>> > > +                 !pt->level )
>> > >              {
>> > >                  /* suspend timer emulation */
>> > 
>> > Especially with this comment I'm not convinced this change is fully
>> > correct: Once a level triggered interrupt is latched in IRR, no
>> > further assertions are meaningful, and hence emulation could (and
>> > hence should) still be stopped to reduce resource consumption.
>> 
>> I've been thinking about this and I think the above comment is not
>> fully correct. Assertion of latched level interrupts is meaningful
>> because gsi_assert_count should be increased regardless of the state
>> of IRR.
>> 
>> Your comment made me realize that periodic level triggered interrupts
>> don't make much sense in vpt. IO-APIC level triggered interrupts will
>> require external deassertion of the line, in which case such
>> deassertion should also take care of reprogramming the timer if
>> required (like it's done in the next patch for vhpet when clearing
>> ISR).
>> 
>> > > @@ -374,13 +378,36 @@ int pt_update_irq(struct vcpu *v)
>> > >          break;
>> > >  
>> > >      case PTSRC_ioapic:
>> > > -        /*
>> > > -         * NB: At the moment IO-APIC routed interrupts generated by vpt devices
>> > > -         * (HPET) are edge-triggered.
>> > > -         */
>> > > -        pt_vector = hvm_ioapic_assert(v->domain, irq, false);
>> > > +        pt_vector = hvm_ioapic_assert(v->domain, irq, level);
>> > >          if ( pt_vector < 0 || !vlapic_test_irq(vcpu_vlapic(v), pt_vector) )
>> > > +        {
>> > >              pt_vector = -1;
>> > > +            if ( level )
>> > > +            {
>> > > +                /*
>> > > +                 * Level interrupts are asserted even if the interrupt is
>> > > +                 * masked, so also execute the callback associated with the
>> > > +                 * timer.
>> > > +                 */
>> > > +                time_cb *cb = NULL;
>> > > +                void *cb_priv;
>> > > +
>> > > +                spin_lock(&v->arch.hvm_vcpu.tm_lock);
>> > > +                /* Make sure the timer is still on the list. */
>> > > +                list_for_each_entry ( pt, &v->arch.hvm_vcpu.tm_list, list )
>> > > +                    if ( pt == earliest_pt )
>> > > +                    {
>> > > +                        pt_irq_fired(v, pt);
>> > > +                        cb = pt->cb;
>> > > +                        cb_priv = pt->priv;
>> > > +                        break;
>> > > +                    }
>> > > +                spin_unlock(&v->arch.hvm_vcpu.tm_lock);
>> > > +
>> > > +                if ( cb != NULL )
>> > > +                    cb(v, cb_priv);
>> > > +            }
>> > > +        }
>> > >          break;
>> > 
>> > I'm not fully convinced, especially in the case that hvm_ioapic_assert()
>> > returned a negative value: Either the callback needs to be called in all
>> > cases (even if an edge triggered interrupt was not successfully asserted),
>> > or only when an interrupt really gets surfaced to the guest.
>> 
>> Even if hvm_ioapic_assert returns -1 gsi_assert_count will be
>> increased, so I think it makes sense to also call the callback in this
>> case, because the line has been asserted.
>> 
>> Roger.
>> 
>> _______________________________________________
>> Xen-devel mailing list
>> Xen-devel@lists.xenproject.org 
>> https://lists.xenproject.org/mailman/listinfo/xen-devel 




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

Re: [PATCH v3 5/6] vpt: add support for level interrupts

[Roger Pau Monné]

On Mon, Jul 02, 2018 at 09:09:31AM -0600, Jan Beulich wrote:
> >>> On 02.07.18 at 16:54, <roger.pau@citrix.com> wrote:
> > Ping?
> 
> I don't understand: There's no open question in the quoted mail.

It's more of a refute of your argument about assert of level triggered
interrupts not being meaningful if the interrupt is latched in IRR,
see below.

> > On Mon, Jun 25, 2018 at 01:19:19PM +0200, Roger Pau Monné wrote:
> >> On Fri, Jun 22, 2018 at 08:23:02AM -0600, Jan Beulich wrote:
> >> > >>> On 08.06.18 at 17:07, <roger.pau@citrix.com> wrote:
> >> > > @@ -316,7 +317,9 @@ int pt_update_irq(struct vcpu *v)
> >> > >          if ( pt->pending_intr_nr )
> >> > >          {
> >> > >              /* RTC code takes care of disabling the timer itself. */
> >> > > -            if ( (pt->irq != RTC_IRQ || !pt->priv) && pt_irq_masked(pt) )
> >> > > +            if ( (pt->irq != RTC_IRQ || !pt->priv) && pt_irq_masked(pt) &&
> >> > > +                 /* Level interrupts should be asserted even if masked. */
> >> > > +                 !pt->level )
> >> > >              {
> >> > >                  /* suspend timer emulation */
> >> > 
> >> > Especially with this comment I'm not convinced this change is fully
> >> > correct: Once a level triggered interrupt is latched in IRR, no
> >> > further assertions are meaningful, and hence emulation could (and
> >> > hence should) still be stopped to reduce resource consumption.
> >> 
> >> I've been thinking about this and I think the above comment is not
> >> fully correct. Assertion of latched level interrupts is meaningful
> >> because gsi_assert_count should be increased regardless of the state
> >> of IRR.

As said, I think that an assert of a level triggered interrupt is
still meaningful, because gsi_assert_count should be incremented
regardless of the state of the IRR.

> >> Your comment made me realize that periodic level triggered interrupts
> >> don't make much sense in vpt. IO-APIC level triggered interrupts will
> >> require external deassertion of the line, in which case such
> >> deassertion should also take care of reprogramming the timer if
> >> required (like it's done in the next patch for vhpet when clearing
> >> ISR).

I think level triggered interrupts should not be allowed in periodic
mode in vpt, since a level triggered interrupt requires an external
deassertion, that should take care of setting the vpt timer while
deasserting the line.

> >> > > @@ -374,13 +378,36 @@ int pt_update_irq(struct vcpu *v)
> >> > >          break;
> >> > >  
> >> > >      case PTSRC_ioapic:
> >> > > -        /*
> >> > > -         * NB: At the moment IO-APIC routed interrupts generated by vpt devices
> >> > > -         * (HPET) are edge-triggered.
> >> > > -         */
> >> > > -        pt_vector = hvm_ioapic_assert(v->domain, irq, false);
> >> > > +        pt_vector = hvm_ioapic_assert(v->domain, irq, level);
> >> > >          if ( pt_vector < 0 || !vlapic_test_irq(vcpu_vlapic(v), pt_vector) )
> >> > > +        {
> >> > >              pt_vector = -1;
> >> > > +            if ( level )
> >> > > +            {
> >> > > +                /*
> >> > > +                 * Level interrupts are asserted even if the interrupt is
> >> > > +                 * masked, so also execute the callback associated with the
> >> > > +                 * timer.
> >> > > +                 */
> >> > > +                time_cb *cb = NULL;
> >> > > +                void *cb_priv;
> >> > > +
> >> > > +                spin_lock(&v->arch.hvm_vcpu.tm_lock);
> >> > > +                /* Make sure the timer is still on the list. */
> >> > > +                list_for_each_entry ( pt, &v->arch.hvm_vcpu.tm_list, list )
> >> > > +                    if ( pt == earliest_pt )
> >> > > +                    {
> >> > > +                        pt_irq_fired(v, pt);
> >> > > +                        cb = pt->cb;
> >> > > +                        cb_priv = pt->priv;
> >> > > +                        break;
> >> > > +                    }
> >> > > +                spin_unlock(&v->arch.hvm_vcpu.tm_lock);
> >> > > +
> >> > > +                if ( cb != NULL )
> >> > > +                    cb(v, cb_priv);
> >> > > +            }
> >> > > +        }
> >> > >          break;
> >> > 
> >> > I'm not fully convinced, especially in the case that hvm_ioapic_assert()
> >> > returned a negative value: Either the callback needs to be called in all
> >> > cases (even if an edge triggered interrupt was not successfully asserted),
> >> > or only when an interrupt really gets surfaced to the guest.
> >> 
> >> Even if hvm_ioapic_assert returns -1 gsi_assert_count will be
> >> increased, so I think it makes sense to also call the callback in this
> >> case, because the line has been asserted.
> >> 
> >> Roger.
> >> 
> >> _______________________________________________
> >> Xen-devel mailing list
> >> Xen-devel@lists.xenproject.org 
> >> https://lists.xenproject.org/mailman/listinfo/xen-devel 
> 
> 
> 

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

Re: [PATCH v3 5/6] vpt: add support for level interrupts

[Jan Beulich]

>>> On 02.07.18 at 18:22, <roger.pau@citrix.com> wrote:
> On Mon, Jul 02, 2018 at 09:09:31AM -0600, Jan Beulich wrote:
>> >>> On 02.07.18 at 16:54, <roger.pau@citrix.com> wrote:
>> > Ping?
>> 
>> I don't understand: There's no open question in the quoted mail.
> 
> It's more of a refute of your argument about assert of level triggered
> interrupts not being meaningful if the interrupt is latched in IRR,
> see below.

I see. And I can certainly follow the arguments you've made.

Jan



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