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 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.e. could we legitimately
tighten the spec?

> But yes, this isn't the fault of the patch here, so this is also not an
> objection to this patch.
> 
> And yes, it can still be done with a single compare afaict, just not
> directly on the memory operand.

Code size is also a concern here since this sequence of instructions is
used for inline patching (not sure what the size limit actually is
though).

Ian.


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