[PATCH v2 0/2] hpet: add support for level triggered interrupts

[PATCH v2 0/2] hpet: add support for level triggered interrupts

[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

[PATCH v2 1/2] x86/vpt: execute callbacks for masked interrupts

[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

Re: [PATCH v2 1/2] x86/vpt: execute callbacks for masked interrupts

[Jan Beulich]

>>> 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

Re: [PATCH v2 1/2] x86/vpt: execute callbacks for masked interrupts

[Roger Pau Monné]

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

Re: [PATCH v2 1/2] x86/vpt: execute callbacks for masked interrupts

[Jan Beulich]

>>> 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

[PATCH v2 2/2] 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 | 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

Re: [PATCH v2 2/2] vhpet: add support for level triggered interrupts

[Jan Beulich]

>>> 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