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

From: "Jan Beulich" <JBeulich@suse.com>
To: David Vrabel <david.vrabel@citrix.com>,
	Ian Campbell <Ian.Campbell@citrix.com>
Cc: "xen-devel@lists.xensource.com" <xen-devel@lists.xensource.com>,
	Konrad Rzeszutek Wilk <konrad.wilk@oracle.com>
Subject: Re: [PATCH] xen: correctly check for pending events when restoring irq flags
Date: Fri, 27 Apr 2012 12:42:09 +0100	[thread overview]
Message-ID: <4F9AA23102000078000806DA@nat28.tlf.novell.com> (raw)
In-Reply-To: <1335516436.28015.169.camel@zakaz.uk.xensource.com>

>>> On 27.04.12 at 10:47, Ian Campbell <Ian.Campbell@citrix.com> wrote:
> On Thu, 2012-04-26 at 19:44 +0100, David Vrabel wrote:
>> From: David Vrabel <david.vrabel@citrix.com>
>> 
>> 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".

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.

Jan

> Acked-by: Ian Campbell <ian.campbell@citrix.com>
> 
> Does xen_irq_enable_direct have the same sort of issue? No, in that case
> we are doing a straight forward test of pending without involving masked
> (since it has just unmasked) and so jz is correct.
> 
> Ian.
> 
> 
> _______________________________________________
> Xen-devel mailing list
> Xen-devel@lists.xen.org 
> http://lists.xen.org/xen-devel 