Re: [Xen-devel] [PATCH] xen: correctly check for pending events when restoring irq flags

[Ian Campbell <Ian.Campbell@xxxxxxxxxx>]

On Fri, 2012-04-27 at 13:46 +0100, Jan Beulich wrote:
> >>> On 27.04.12 at 13:58, Ian Campbell <Ian.Campbell@xxxxxxxxxx> wrote:
> > On Fri, 2012-04-27 at 12:42 +0100, Jan Beulich wrote:
> >> >>> On 27.04.12 at 10:47, Ian Campbell <Ian.Campbell@xxxxxxxxxx> wrote:
> >> > On Thu, 2012-04-26 at 19:44 +0100, David Vrabel wrote:
> >> >> From: David Vrabel <david.vrabel@xxxxxxxxxx>
> >> >> 
> >> >> In xen_restore_fl_direct(), xen_force_evtchn_callback() was being
> >> >> called even if no events were pending.
> >> > 
> >> > In actual fact it seems that the callback was actually being called if
> >> > and only if no events were pending? Which makes me wonder how it used to
> >> > work at all!
> >> > 
> >> >> diff --git a/arch/x86/xen/xen-asm.S b/arch/x86/xen/xen-asm.S
> >> >> index 79d7362..3e45aa0 100644
> >> >> --- a/arch/x86/xen/xen-asm.S
> >> >> +++ b/arch/x86/xen/xen-asm.S
> >> >> @@ -96,7 +96,7 @@ ENTRY(xen_restore_fl_direct)
> >> >>  
> >> >>         /* check for unmasked and pending */
> >> >>         cmpw $0x0001, PER_CPU_VAR(xen_vcpu_info) + XEN_vcpu_info_pending
> >> >> -       jz 1f
> >> >> +       jnz 1f
> >> >>  2:     call check_events
> >> >>  1:
> >> > 
> >> > Took me a while, this is a bit tricksy (and it may well be too early for
> >> > me to be decoding it) since the check here is trying to check both
> >> > pending and masked in a single cmpw, but I think this is correct. It
> >> > will call check_events now only when the combined mask+pending word is
> >> > 0x0001 (aka unmasked, pending).
> >> 
> >> It is _too much_ trickery, as it implies that the pending field, when set,
> >> will always be 1. This is not sanctioned by the specification (quoting
> >> the hypervisor's xen/include/public/xen.h):
> >> 
> >>      * 'evtchn_upcall_pending' is written non-zero by Xen to indicate
> >>      * a pending notification for a particular VCPU. It is then cleared 
> >>      * by the guest OS /before/ checking for pending work, thus avoiding
> >> 
> >> Note that it says "non-zero", not "1".
> > 
> > Hrm, has it ever not been 1 in practice?
> 
> I don't think so.
> 
> > i.e. could we legitimately tighten the spec?
> 
> I wouldn't want to recommend this. In particular, we can't all of the
> sudden keep guests from storing other non-zero values in here.

Could they be doing this? Whatever they put there is going to get
clobbered by Xen whenever it injects an event, isn't it? Or do you mean
that a guest can force an upcall by writing non-zero itself? Do guests
actually do that? (why?)

> While I'm not in favor of this either, what we could do is specify that
> Xen will only ever write 0 or 1 in here, while other non-zero values
> are okay to be used by guests.

Does Xen ever clear an upcall, isn't that always the guest? Xen only
ever writes 1, at least as far as I can see...

What do you think of the following?

diff -r 107285938c50 xen/include/public/xen.h
--- a/xen/include/public/xen.h  Thu Apr 26 10:03:08 2012 +0100
+++ b/xen/include/public/xen.h  Fri Apr 27 14:09:00 2012 +0100
@@ -559,16 +559,18 @@ typedef struct vcpu_time_info vcpu_time_
 
 struct vcpu_info {
     /*
-     * 'evtchn_upcall_pending' is written non-zero by Xen to indicate
-     * a pending notification for a particular VCPU. It is then cleared 
-     * by the guest OS /before/ checking for pending work, thus avoiding
-     * a set-and-check race. Note that the mask is only accessed by Xen
-     * on the CPU that is currently hosting the VCPU. This means that the
-     * pending and mask flags can be updated by the guest without special
-     * synchronisation (i.e., no need for the x86 LOCK prefix).
-     * This may seem suboptimal because if the pending flag is set by
-     * a different CPU then an IPI may be scheduled even when the mask
-     * is set. However, note:
+     * 'evtchn_upcall_pending' is written to '1' by Xen to indicate a
+     * pending notification for a particular VCPU. Xen will never
+     * write any other value but it is legitimate for a guest to store
+     * any other non-zero value here to trigger an upcall. It is then
+     * cleared by the guest OS /before/ checking for pending work,
+     * thus avoiding a set-and-check race. Note that the mask is only
+     * accessed by Xen on the CPU that is currently hosting the
+     * VCPU. This means that the pending and mask flags can be updated
+     * by the guest without special synchronisation (i.e., no need for
+     * the x86 LOCK prefix).  This may seem suboptimal because if the
+     * pending flag is set by a different CPU then an IPI may be
+     * scheduled even when the mask is set. However, note:
      *  1. The task of 'interrupt holdoff' is covered by the per-event-
      *     channel mask bits. A 'noisy' event that is continually being
      *     triggered can be masked at source at this very precise



_______________________________________________
Xen-devel mailing list
Xen-devel@xxxxxxxxxxxxx
http://lists.xen.org/xen-devel