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

2018-04-10 Thread Jan Beulich 
>>> On 10.04.18 at 10:53,  wrote:
> On Mon, Apr 09, 2018 at 09:34:57AM -0600, Jan Beulich wrote:
>> >>> On 30.03.18 at 14:35,  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

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

2018-04-10 Thread Roger Pau Monné 
On Mon, Apr 09, 2018 at 09:34:57AM -0600, Jan Beulich wrote:
> >>> On 30.03.18 at 14:35,  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, _purge);
> >  spin_unlock(>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, _purge);
> > +
> >  spin_unlock(>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: [Xen-devel] [PATCH v2 1/2] x86/vpt: execute callbacks for masked interrupts

2018-04-09 Thread Jan Beulich 
>>> On 30.03.18 at 14:35,  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, _purge);
>  spin_unlock(>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, _purge);
> +
>  spin_unlock(>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

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

2018-03-30 Thread 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é 
---
Cc: Jan Beulich 
Cc: Andrew Cooper 
Cc: Stefan Bader 
---
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(>list);
+pt->on_list = 0;
+spin_unlock(>arch.hvm_vcpu.tm_lock);
+
+cb(v, cb_priv);
+
+spin_lock(>arch.hvm_vcpu.tm_lock);
+}
+}
+
 int pt_update_irq(struct vcpu *v)
 {
 struct list_head *head = >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(>list);
-pt->on_list = 0;
+if ( pt->cb )
+list_add(>list, _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, _purge);
 spin_unlock(>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, _purge);
+
 spin_unlock(>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